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

internal/appsec: refactor listeners #2862

Merged
merged 25 commits into from
Sep 24, 2024
Merged

Conversation

eliottness
Copy link
Contributor

@eliottness eliottness commented Sep 13, 2024

What does this PR do?

This reorders and moves a lot of appsec logic under dyngo for a greater flexibility and less code duplication. Noteworthy changes are:

  • No more internal/appsec/trace package, distributed between listeners packages
  • No more nested lambdas in listener packages
  • Merging of the WAF Context managment code under the emitter/waf package containing the ContextOperation which merge the code from httpsec, grpcsec and graphqlsec operations. This made possible to create the waf.RunEvent event listener that will simply to a WAF run and dispatch actions events if deemed necessary by the WAF.
  • The trace.TagsHolder became the trace.ServiceEntrySpanOperation and trace.SpanOperation. They are now always the top level operations to make the link between APM and ASM. They also can receive events with span tags from higher in the stack to create various span tags. All span tags setting code is now under the listeners because of this change.
  • A new emitter/waf/addresses package to store WAF addresses and a waf.RunAddressDataBuilder builder to provide a hassle-free API when dealing with lower-level free-form API shenanigans of the WAF.
  • Promotion of the wafEventListeners array filled at init time to a full-fledged API called Feature's (feel free to find a better name) to register listeners. Its purpose is to be a list of pure functions depending only on the merged configuration with no side-effets and ready to be re-built at each config change.
  • Separation of the user monitoring in a separate usersec package to better work on login events
  • Move of the actions events under emitter/waf/actions (with some simplification ofc)
  • Various changes across all contribs and internal/appsec to make all of this changes work
  • The start of a documentation for appsec in dd-trace-go here

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@eliottness eliottness changed the title Eliott.bouhana/refactor listeners internal/appsec: refactor listeners Sep 13, 2024
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
Signed-off-by: Eliott Bouhana <[email protected]>
@eliottness eliottness force-pushed the eliott.bouhana/refactor-listeners branch from 4cc8228 to e5b9a43 Compare September 13, 2024 14:33
Signed-off-by: Eliott Bouhana <[email protected]>
@pr-commenter
Copy link

pr-commenter bot commented Sep 19, 2024

Benchmarks

Benchmark execution time: 2024-09-23 15:08:27

Comparing candidate commit d730a85 in PR branch eliott.bouhana/refactor-listeners with baseline commit ba18110 in branch main.

Found 9 performance improvements and 1 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.

scenario:BenchmarkExtractW3C-24

  • 🟩 execution_time [-56.405ns; -51.795ns] or [-2.296%; -2.108%]

scenario:BenchmarkInjectW3C-24

  • 🟥 execution_time [+108.814ns; +133.586ns] or [+2.683%; +3.294%]

scenario:BenchmarkPartialFlushing/Disabled-24

  • 🟩 execution_time [-14.657ms; -11.961ms] or [-5.224%; -4.264%]

scenario:BenchmarkPartialFlushing/Enabled-24

  • 🟩 execution_time [-16.292ms; -13.037ms] or [-5.709%; -4.568%]

scenario:BenchmarkSetTagMetric-24

  • 🟩 execution_time [-4.658ns; -2.522ns] or [-3.904%; -2.114%]

scenario:BenchmarkSingleSpanRetention/no-rules-24

  • 🟩 execution_time [-14.258µs; -13.379µs] or [-5.734%; -5.381%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-all-24

  • 🟩 execution_time [-15.003µs; -13.836µs] or [-5.968%; -5.504%]

scenario:BenchmarkSingleSpanRetention/with-rules/match-half-24

  • 🟩 execution_time [-14.543µs; -13.246µs] or [-5.793%; -5.276%]

scenario:BenchmarkStartSpan-24

  • 🟩 execution_time [-142.109ns; -118.291ns] or [-6.046%; -5.033%]

scenario:BenchmarkTracerAddSpans-24

  • 🟩 execution_time [-280.832ns; -249.368ns] or [-6.867%; -6.097%]

@eliottness eliottness self-assigned this Sep 20, 2024
@github-actions github-actions bot added the apm:ecosystem contrib/* related feature requests or bugs label Sep 20, 2024
@eliottness
Copy link
Contributor Author

FYI The Orchestrion Job error has been identified and is independent from this PR

Signed-off-by: Eliott Bouhana <[email protected]>
@eliottness eliottness marked this pull request as ready for review September 20, 2024 16:16
@eliottness eliottness requested review from a team as code owners September 20, 2024 16:16
@eliottness eliottness requested review from RomainMuller and removed request for RomainMuller September 20, 2024 16:16
contrib/labstack/echo.v4/appsec_test.go Outdated Show resolved Hide resolved
contrib/labstack/echo.v4/appsec_test.go Outdated Show resolved Hide resolved
internal/appsec/appsec.go Outdated Show resolved Hide resolved
RequestURI: r.RequestURI,
Host: r.Host,
RemoteAddr: r.RemoteAddr,
Headers: r.Header,
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to allow use of the functor that allows synchronizing access to the headers here, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you are talking about rapid stuff it's on response headers not the request one. And if you don't talk about rapid stuff:

  • Concurrent access from the middlewares above us is not supposed to happen at all following the spec
  • I just copied the previous behaviour

internal/appsec/emitter/waf/actions/http_redirect.go Outdated Show resolved Hide resolved
internal/appsec/emitter/waf/run.go Outdated Show resolved Hide resolved
}

// RunSimple runs the WAF with the given address data and returns an error that should be forwarded to the caller
func RunSimple(ctx context.Context, addrs waf.RunAddressData, errorLog string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This function does not filter the addresses to only retain what's supported... any particular reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It will call the function (*ContextOperation).Run via the (*ContextOperation).OnEvent eventually so it's still filtered

return !ok
})

result, err := ctx.Run(addrs)
Copy link
Contributor

Choose a reason for hiding this comment

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

At this point, addrs could be empty, and if it is, do we really need to submit to the WAF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is done by in the first line of the Run function on go-libddwaf side so....

internal/appsec/features.go Outdated Show resolved Hide resolved
internal/appsec/listener/waf/tags.go Outdated Show resolved Hide resolved
@eliottness eliottness merged commit 7699f9e into main Sep 24, 2024
166 checks passed
@eliottness eliottness deleted the eliott.bouhana/refactor-listeners branch September 24, 2024 09:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
apm:ecosystem contrib/* related feature requests or bugs appsec
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants