Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update libddwaf gem to v1.15.0.0.0 #4052

Merged
merged 3 commits into from
Oct 31, 2024
Merged

Conversation

y9v
Copy link
Member

@y9v y9v commented Oct 31, 2024

What does this PR do?
It updates libddwaf dependency to v1.15.0.0.0. Since it had breaking changes, this PR also addresses it.

Motivation:
We have to update libddwaf gem to use the latest libddwaf binary (which is already at 1.20). This is the first step.

Change log entry
This should be considered internal change.

Additional Notes:
None

How to test the change?
Specs for appsec, and local testing for our appsec integrations.

@y9v y9v self-assigned this Oct 31, 2024
@y9v y9v requested review from a team as code owners October 31, 2024 11:16
@y9v y9v force-pushed the appsec-update-libddwaf-to-1-15 branch from 29e908d to 71e804f Compare October 31, 2024 11:21
@y9v y9v force-pushed the appsec-update-libddwaf-to-1-15 branch from 71e804f to f7a94ce Compare October 31, 2024 11:23
@codecov-commenter
Copy link

codecov-commenter commented Oct 31, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.72%. Comparing base (d361376) to head (d5c46a7).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4052      +/-   ##
==========================================
- Coverage   97.73%   97.72%   -0.01%     
==========================================
  Files        1338     1338              
  Lines       80245    80248       +3     
  Branches     4014     4016       +2     
==========================================
  Hits        78426    78426              
- Misses       1819     1822       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pr-commenter
Copy link

pr-commenter bot commented Oct 31, 2024

Benchmarks

Benchmark execution time: 2024-10-31 13:29:17

Comparing candidate commit d5c46a7 in PR branch appsec-update-libddwaf-to-1-15 with baseline commit d361376 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 29 metrics, 2 unstable metrics.

Copy link
Contributor

@Strech Strech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏🏼 Very nice, left a few comments

@@ -0,0 +1,137 @@
module Datadog
module AppSec
# rubocop:disable Metrics/ModuleLength
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# rubocop:disable Metrics/ModuleLength

module AppSec
# rubocop:disable Metrics/ModuleLength
module WAF
# retain logging proc if set properly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# retain logging proc if set properly


def self.version: () -> untyped

# rubocop:disable Metrics/MethodLength,Metrics/CyclomaticComplexity,Metrics/PerceivedComplexity
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# rubocop:disable Metrics/MethodLength,Metrics/CyclomaticComplexity,Metrics/PerceivedComplexity

@@ -30,15 +30,15 @@ module Datadog

def initialize: (WAF::Handle handle, telemetry: Core::Telemetry::Component) -> void

def run: (Hash[untyped, untyped] input, ?::Integer timeout) -> WAF::Result
def run: (Hash[untyped, untyped] persistent_data, Hash[untyped, untyped] ephemeral_data, ?::Integer timeout) -> WAF::Result
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT if we extract waf_data as a type?

type waf_data = Hash[untyped, untyped]
Suggested change
def run: (Hash[untyped, untyped] persistent_data, Hash[untyped, untyped] ephemeral_data, ?::Integer timeout) -> WAF::Result
def run: (waf_data persistent_data, waf_data ephemeral_data, ?::Integer timeout) -> WAF::Result


def extract_schema: () -> WAF::Result?

def finalize: () -> void

private

def try_run: (Hash[untyped, untyped] input, ::Integer timeout) -> [::Symbol, WAF::Result]
def try_run: (Hash[untyped, untyped] persistent_data, Hash[untyped, untyped] ephemeral_data, ::Integer timeout) -> [::Symbol, WAF::Result]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def try_run: (Hash[untyped, untyped] persistent_data, Hash[untyped, untyped] ephemeral_data, ::Integer timeout) -> [::Symbol, WAF::Result]
def try_run: (waf_data persistent_data, waf_data ephemeral_data, ::Integer timeout) -> [::Symbol, WAF::Result]

next false if v.is_a?(TrueClass) || v.is_a?(FalseClass)

v.nil? ? true : v.empty?
end

_code, result = try_run(input, timeout)
ephemeral_data.reject! do |_, v|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally agree with duplication because it should not be the part of the Context here and should be moved into WAF::Context in the future

Comment on lines 28 to 33
waf_args = {
waf_persistent_data = {
'graphql.server.all_resolvers' => arguments
}

waf_timeout = Datadog.configuration.appsec.waf_timeout
result = waf_context.run(waf_args, waf_timeout)
result = waf_context.run(waf_persistent_data, {}, waf_timeout)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe it's too much, but I would throw that suggestion anyway, WDYT if we name it like this?

Suggested change
waf_args = {
waf_persistent_data = {
'graphql.server.all_resolvers' => arguments
}
waf_timeout = Datadog.configuration.appsec.waf_timeout
result = waf_context.run(waf_args, waf_timeout)
result = waf_context.run(waf_persistent_data, {}, waf_timeout)
ephemeral_data = {}
persistent_data = {
'graphql.server.all_resolvers' => arguments
}
timeout = Datadog.configuration.appsec.waf_timeout
result = waf_context.run(persistent_data, ephemeral_data, timeout)

P.S I also think that we can drop waf_ prefix everywhere, because it's obvious.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! fixed all of this in d5c46a7

@y9v y9v merged commit 421a1cb into master Oct 31, 2024
278 of 280 checks passed
@y9v y9v deleted the appsec-update-libddwaf-to-1-15 branch October 31, 2024 16:02
@github-actions github-actions bot added this to the 2.5.0 milestone Oct 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants