internal/appsec: refactor listeners by eliottness · Pull Request #2862 · DataDog/dd-trace-go

eliottness · 2024-09-13T12:01:58Z

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!

Signed-off-by: Eliott Bouhana <[email protected]>

pr-commenter · 2024-09-19T16:43:44Z

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%]

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness · 2024-09-20T13:53:44Z

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

Signed-off-by: Eliott Bouhana <[email protected]>

…odules Signed-off-by: Eliott Bouhana <[email protected]>

RomainMuller · 2024-09-23T09:25:49Z

+			RequestURI:  r.RequestURI,
+			Host:        r.Host,
+			RemoteAddr:  r.RemoteAddr,
+			Headers:     r.Header,


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

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

RomainMuller · 2024-09-23T09:50:34Z

+}
+
+// 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 {


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

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

RomainMuller · 2024-09-23T09:51:10Z

+		return !ok
+	})
+
+	result, err := ctx.Run(addrs)


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

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

Signed-off-by: Eliott Bouhana <[email protected]>

Co-authored-by: Romain Marcadier <[email protected]>

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness changed the title ~~Eliott.bouhana/refactor listeners~~ internal/appsec: refactor listeners Sep 13, 2024

eliottness mentioned this pull request Sep 13, 2024

internal/stacktrace: upgrade msgp and add stacktrace.GetSpanValue #2864

Merged

6 tasks

eliottness added 5 commits September 13, 2024 16:32

wip

3f5d865

Signed-off-by: Eliott Bouhana <[email protected]>

wip

fc3e62c

Signed-off-by: Eliott Bouhana <[email protected]>

wip

3deff0d

Signed-off-by: Eliott Bouhana <[email protected]>

wip

70cff5a

Signed-off-by: Eliott Bouhana <[email protected]>

wip

e5b9a43

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness force-pushed the eliott.bouhana/refactor-listeners branch from 4cc8228 to e5b9a43 Compare September 13, 2024 14:33

wip

b8a8ed4

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness commented Sep 13, 2024

View reviewed changes

Comment thread internal/appsec/emitter/waf/actions/actions.go Outdated

Comment thread internal/appsec/emitter/waf/addresses/builder.go Outdated

eliottness added 8 commits September 16, 2024 12:10

switch block http emitter action to atomic

d281a0d

Signed-off-by: Eliott Bouhana <[email protected]>

finished making tests work

590d8ba

Signed-off-by: Eliott Bouhana <[email protected]>

finished graphql

ea56f19

Signed-off-by: Eliott Bouhana <[email protected]>

fixed graphql tests

99c80ff

Signed-off-by: Eliott Bouhana <[email protected]>

started gRPC work

c2f71b9

Signed-off-by: Eliott Bouhana <[email protected]>

Merge branch 'main' into eliott.bouhana/refactor-listeners

010930e

finished working on gRPC and ran golangci-lint on the whole branch code

ce04ace

Signed-off-by: Eliott Bouhana <[email protected]>

ran go mod tidy

48a7b4f

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness commented Sep 19, 2024

View reviewed changes

Comment thread internal/appsec/listener/httpsec/http.go Outdated

eliottness commented Sep 19, 2024

View reviewed changes

Comment thread internal/appsec/listener/waf/tags.go Outdated

eliottness added 5 commits September 20, 2024 13:49

fix misc bugs

bd9be44

Signed-off-by: Eliott Bouhana <[email protected]>

remove response cookies

9171b06

Signed-off-by: Eliott Bouhana <[email protected]>

revert end-of-file fix

5225c1a

Signed-off-by: Eliott Bouhana <[email protected]>

fix ssrf missing event type

6d42d91

Signed-off-by: Eliott Bouhana <[email protected]>

change status code types when given to the WAF from int to string

5f9aad9

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness self-assigned this Sep 20, 2024

eliottness added the appsec label Sep 20, 2024

github-actions Bot added the apm:ecosystem contrib/* related feature requests or bugs label Sep 20, 2024

start the doc in internal/appsec

afc1cc7

Signed-off-by: Eliott Bouhana <[email protected]>

eliottness marked this pull request as ready for review September 20, 2024 16:16

eliottness requested review from a team as code owners September 20, 2024 16:16

eliottness requested review from RomainMuller and removed request for RomainMuller September 20, 2024 16:16

add upper case orchestrion module to the list of stacktrace skipped m…

571686c

…odules Signed-off-by: Eliott Bouhana <[email protected]>

RomainMuller approved these changes Sep 23, 2024

View reviewed changes

eliottness and others added 4 commits September 23, 2024 16:22

suggestions from @RomainMuller

b565745

Signed-off-by: Eliott Bouhana <[email protected]>

Apply suggestions from code review

3116e9d

Co-authored-by: Romain Marcadier <[email protected]>

Merge branch 'main' into eliott.bouhana/refactor-listeners

9b57c1d

run go fmt

d730a85

Signed-off-by: Eliott Bouhana <[email protected]>

rarguelloF approved these changes Sep 24, 2024

View reviewed changes

eliottness merged commit 7699f9e into main Sep 24, 2024

eliottness deleted the eliott.bouhana/refactor-listeners branch September 24, 2024 09:54

Uh oh!

Conversation

eliottness commented Sep 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Reviewer's Checklist

Uh oh!

Uh oh!

Uh oh!

pr-commenter Bot commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

scenario:BenchmarkExtractW3C-24

scenario:BenchmarkInjectW3C-24

scenario:BenchmarkPartialFlushing/Disabled-24

scenario:BenchmarkPartialFlushing/Enabled-24

scenario:BenchmarkSetTagMetric-24

scenario:BenchmarkSingleSpanRetention/no-rules-24

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

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

scenario:BenchmarkStartSpan-24

scenario:BenchmarkTracerAddSpans-24

Uh oh!

Uh oh!

Uh oh!

eliottness commented Sep 20, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RomainMuller Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

eliottness Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

RomainMuller Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

eliottness Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

RomainMuller Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

eliottness Sep 23, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eliottness commented Sep 13, 2024 •

edited

Loading

pr-commenter Bot commented Sep 19, 2024 •

edited

Loading