Disable helper when appsec is fully disabled.

[cataphract]

Description

Also make datadog.appsec.enabled a system config. Note that the default value for datadog.appsec.enabled in ext/configuration.h (true) is different from that in datadog.appsec.disabled (false). This is intentional. appsec chooses between three states: explicitly enabled, explicitly disabled and controlled by remote config. Though a questionable decision, the config setting is still a boolean, and the third state is detected by a hack that determines whether the value was explicitly set. Because on the ddtrace side we want to suppress the helper only if explicitly disabled, we can set the default value to true, and disable the helper when the value is false (if the value is not the default true, then it was explicitly set to false).

Reviewer checklist

• Test coverage seems ok.
• Appropriate labels assigned.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 82.60870% with 4 lines in your changes missing coverage. Please review.

Project coverage is 72.41%. Comparing base (7b487bd) to head (74be367).

Files with missing lines	Patch %	Lines
appsec/src/extension/helper_process.c	60.00%	3 Missing and 1 partial ⚠️

❗ There is a different number of reports uploaded between BASE (7b487bd) and HEAD (74be367). Click for more details.

HEAD has 1 upload less than BASE

Flag	BASE (7b487bd)	HEAD (74be367)
tracer-php	12	11

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #2935      +/-   ##
============================================
- Coverage     82.10%   72.41%   -9.70%     
  Complexity     2527     2527              
============================================
  Files           108      135      +27     
  Lines         10360    14402    +4042     
  Branches          0      991     +991     
============================================
+ Hits           8506    10429    +1923     
- Misses         1854     3427    +1573     
- Partials          0      546     +546     

Flag	Coverage Δ
appsec-extension	68.40% <82.60%> (?)
tracer-php	73.97% <ø> (-8.13%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
appsec/src/extension/commands/request_shutdown.c	67.82% <ø> (ø)
appsec/src/extension/configuration.h	100.00% <ø> (ø)
appsec/src/extension/ddappsec.c	74.77% <100.00%> (ø)
appsec/src/extension/helper_process.c	62.66% <60.00%> (ø)

... and 31 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7b487bd...74be367. Read the comment docs.

[bwoebi]

Note that the default value for datadog.appsec.enabled in ext/configuration.h (true) is different from that in datadog.appsec.disabled (false). This is intentional.

As long as this works properly ... Both extensions will write to the same ini_entry in PHP... I think it would be better to keep them in sync and do the name_index < 0 check in the tracer too. Or is there any reason we cannot do this?

[bwoebi]

I'm confused, dd_appsec_maybe_enable_helper still will have the sidecar sideload the appsec helper (if the sidecar is launched at all) - when the tracer is launched with the sidecar, there's no check in dd_appsec_maybe_enable_helper for appsec disabled?

[pr-commenter]

Benchmarks [ appsec ]

Benchmark execution time: 2024-11-11 12:24:19

Comparing candidate commit 74be367 in PR branch no-helper-with-appsec-disabled with baseline commit 7b487bd in branch master.

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

[pr-commenter]

Benchmarks [ tracer ]

Benchmark execution time: 2024-11-11 12:16:52

Comparing candidate commit 74be367 in PR branch no-helper-with-appsec-disabled with baseline commit 7b487bd in branch master.

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

scenario:MessagePackSerializationBench/benchMessagePackSerialization

• 🟥 execution_time [+4.284µs; +8.136µs] or [+2.599%; +4.936%]

[bwoebi]

Looks great to me, thanks :-) [except for the PHP 8.4 compile failure...]

[Anilm3]

lgtm