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

appsec: remove "appsec" build tag requirement #2354

Merged
merged 16 commits into from Nov 22, 2023

Conversation

RomainMuller
Copy link
Contributor

@RomainMuller RomainMuller commented Nov 14, 2023

What does this PR do?

Removes the appsec build tag. The in-app WAF can be disabled using a build-tag defined by the github.com/DataDog/go-libddwaf library (making it a no-op implementation). This build tag named datadog.no_waf has been made to obtain the same result than before but this makes appsec build requirements (which does not include cgo anymore) opt-out instead of opt-in.

AppSec still is disabled by default, and can be enabled with remote activation or DD_APPSEC_ENABLED=true.

Motivation

This makes ASM available in all builds by default, while still allowing an opt-out of code-level support when necessary. ASM support can be enabled by default as it no longer requires cgo to build.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • 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.

For Datadog employees:

  • If this PR touches code that handles credentials of any kind, such as Datadog API keys, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.

Unsure? Have a question? Request a review!

@pr-commenter
Copy link

pr-commenter bot commented Nov 14, 2023

Benchmarks

Benchmark execution time: 2023-11-22 13:10:48

Comparing candidate commit 382e61c in PR branch romain.marcadier/build-tag/APPSEC-9332 with baseline commit c418382 in branch main.

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

scenario:BenchmarkOTelApiWithCustomTags/datadog_otel_api-24

  • 🟩 execution_time [-212.868ns; -141.532ns] or [-4.148%; -2.758%]

scenario:BenchmarkOTelApiWithCustomTags/otel_api-24

  • 🟩 execution_time [-292.126ns; -190.874ns] or [-3.898%; -2.547%]

scenario:BenchmarkPartialFlushing/Enabled-24

  • 🟥 execution_time [+5.761ms; +10.327ms] or [+2.045%; +3.666%]

scenario:BenchmarkSingleSpanRetention/no-rules-24

  • 🟥 execution_time [+10.676µs; +13.423µs] or [+4.348%; +5.466%]

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

  • 🟥 execution_time [+11.510µs; +13.927µs] or [+4.657%; +5.635%]

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

  • 🟥 execution_time [+10.466µs; +13.049µs] or [+4.229%; +5.273%]

scenario:BenchmarkStartSpan-24

  • 🟥 execution_time [+87.450ns; +128.750ns] or [+3.753%; +5.526%]

scenario:BenchmarkTracerAddSpans-24

  • 🟥 execution_time [+93.382ns; +143.418ns] or [+2.325%; +3.570%]

@Julio-Guerra
Copy link
Contributor

Thanks for bootstrapping this.

Strong opinion here: I don't want asm to be used instead of appsec anywhere in the engineering side of things, and especially in the software code/doc/tools, due to how ambiguous this is in the software world (actually it's not: it means assembly and nothing else ;-p).

appsec is clearer for readers (well known short name in the sec world as opposed to ASM which is a Datadog product idea to mimic APM to try to create/influence a new sec category), better referencing, aligned with DD_APPSEC_*, etc.

PS: I wasn't part of the renaming of all our GH teams into asm-* (they did it during my PTO and I'd have blocked it for the same reasons).

test.sh Outdated Show resolved Hide resolved
@RomainMuller RomainMuller changed the title appsec: re-work build tag for feature enablement appsec: remove "appsec" build tag requirement Nov 15, 2023
@eliottness eliottness marked this pull request as ready for review November 17, 2023 14:18
@eliottness eliottness requested a review from a team November 17, 2023 14:18
@eliottness eliottness requested review from a team as code owners November 17, 2023 14:18
@eliottness eliottness force-pushed the romain.marcadier/build-tag/APPSEC-9332 branch from bfa0250 to addf14e Compare November 17, 2023 14:25
@eliottness eliottness force-pushed the romain.marcadier/build-tag/APPSEC-9332 branch 2 times, most recently from cac24d3 to 4b98137 Compare November 17, 2023 14:54
@eliottness eliottness force-pushed the romain.marcadier/build-tag/APPSEC-9332 branch 2 times, most recently from b8dff8e to d161834 Compare November 20, 2023 09:12
test.sh Outdated Show resolved Hide resolved
Copy link
Contributor

@nsrip-dd nsrip-dd left a comment

Choose a reason for hiding this comment

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

I don't see any profiling-related changes. go.mod + CI stuff looks good to me. Please ping if you need a re-review.

Copy link
Contributor

@knusbaum knusbaum left a comment

Choose a reason for hiding this comment

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

Great news!

@eliottness eliottness force-pushed the romain.marcadier/build-tag/APPSEC-9332 branch from 7d10bd1 to 1311ac9 Compare November 21, 2023 14:50
RomainMuller and others added 8 commits November 22, 2023 11:36
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
eliottness and others added 8 commits November 22, 2023 11:36
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com>
@eliottness eliottness force-pushed the romain.marcadier/build-tag/APPSEC-9332 branch 2 times, most recently from d54f69b to 382e61c Compare November 22, 2023 12:56
@eliottness
Copy link
Contributor

After investigating the performance loss it does not seem to be related to this PR because we are able to get back to usual performances level by removing this commit on main from our history. We believe that this PR does not impact overall performances as it does not add any code that should slow benchmarks.

@eliottness eliottness merged commit 3241a9f into main Nov 22, 2023
464 checks passed
@eliottness eliottness deleted the romain.marcadier/build-tag/APPSEC-9332 branch November 22, 2023 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants