-
Notifications
You must be signed in to change notification settings - Fork 162
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
Roadrunner appsec support #2443
Conversation
aa950e2
to
150d4c8
Compare
150d4c8
to
1f674a6
Compare
0e8be64
to
367910f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ack for the uhook changes :-)
Maybe emit a LOG() line if suppressCall is attempted on an internal function, but that's it.
fff87b3
to
18bd6b8
Compare
src/Integrations/Integrations/Roadrunner/RoadrunnerIntegration.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still a lot to look through...
DDAPPSEC_G(enabled) = get_DD_APPSEC_ENABLED() ? APPSEC_FULLY_ENABLED | ||
: APPSEC_FULLY_DISABLED; | ||
DDAPPSEC_G(active) = get_DD_APPSEC_ENABLED() ? true : false; | ||
DDAPPSEC_G(to_be_configured) = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure If I missed something but, when appsec is enabled dynamically by remote config, it status can change many times. So saying it does not have to be configure
sounds confusing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"To be configured" means "yet to be configured" (=not configured yet") or "it is to be configured" (=it's arranged/programmed for it to be configured in the future"). The purpose of this setting is just to indicate in MINIT if we got a configuration setting for the state of remote config yet (if, for instance, we can't contact the daemon, then to_be_configured will stay with the value true
). In this branch, appsec is explicitly enabled or disabled, so we're never waiting for a setting from remote config. Does this make sense? I also added some comments in bbdb003
37441cf
to
1e358a6
Compare
BenchmarksBenchmark execution time: 2024-01-16 18:34:11 Comparing candidate commit 5fa4475 in PR branch Found 2 performance improvements and 1 performance regressions! Performance is the same for 38 metrics, 49 unstable metrics. scenario:HookBench/benchHookOverheadInstallHookOnFunction
scenario:PHPRedisBench/benchRedisBaseline
scenario:PHPRedisBench/benchRedisOverhead
|
eef7c2f
to
5fa4475
Compare
Description
Reviewer checklist
Tickets
APPSEC-14734 APPSEC-12702 APPSEC-12703 APPSEC-12705