-
Notifications
You must be signed in to change notification settings - Fork 166
RFC 1076: fallback on http.endpoint for schema sampler #3557
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
Conversation
298e7be to
bf0898c
Compare
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (73.97%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #3557 +/- ##
==========================================
- Coverage 61.78% 61.68% -0.10%
==========================================
Files 139 139
Lines 13051 13114 +63
Branches 1712 1728 +16
==========================================
+ Hits 8063 8090 +27
- Misses 4225 4253 +28
- Partials 763 771 +8
... and 3 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
0d833f9 to
90c760b
Compare
90c760b to
05cee7e
Compare
05cee7e to
ab7aab7
Compare
|
@codex review |
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ab7aab7b46
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (_request_blocked) { | ||
| mlog_g( | ||
| dd_log_debug, "Request was blocked; not sampling for API security"); | ||
| return 0; |
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.
Skip sampling only for pre-shutdown blocks
The new _request_blocked guard here only suppresses sampling if a block/redirect was detected before _calc_sampling_key runs. However _calc_sampling_key is executed before dd_request_shutdown, and _request_blocked is only set when helper actions are processed (via dd_req_lifecycle_set_blocked in commands_helpers.c). If the WAF blocks based on response data during request_shutdown, _request_blocked is still false at this point, so a sampling key is still sent and schema sampling can occur for blocked responses. If the intent is “never sample blocked requests,” this misses response-time blocks; consider deferring sampling until after the shutdown verdict or clearing the key when a block/redirect is returned.
Useful? React with 👍 / 👎.
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.
Acceptable, the main point of not sampling blocked requests is to avoid calculation of the schema for the blocked response; that won't happen when blocking happens on rshutdown, as there won't be a subsequent libddwaf call capable of submitting the block response.
ab7aab7 to
8a3409a
Compare
8a3409a to
3df4e17
Compare
Description
Reviewer checklist