Repository navigation
Add block-outcome telemetry to Grizzly, Jetty, Liberty, Spring-webmvc, Undertow and Akka-http - #12527
Conversation
jandro996
added
comp: asm waf
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ff0f114442
ℹ️ 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".
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
ff0f114 to
e1564ba
Compare
8855f84 to
2b99081
Compare
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b99081893
ℹ️ 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".
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2cbed27d7
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6f02c3d270
ℹ️ 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".
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Breezy! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Blocking-response commit paths now report false results and caught exceptions. Successful blocking behavior stays unchanged.
🤖 Datadog Autotest · Commit 79478d2 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79478d24fd
ℹ️ 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".
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
- Wire reportBlockFailure() at every BlockResponseFunction.tryCommitBlockingResponse call site across akka-http, grizzly, jetty (appsec + server), liberty, spring-webmvc and undertow, resolving AppSecContext via RequestContextSlot.APPSEC instead of casting directly to AppSecRequestContext - Add UnmarshallerHelpersBlockFailureTest covering all branches of UnmarshallerHelpers.tryBlock (block committed, block failed, no BRF, foreign/null AppSec slot) - Widen UnmarshallerHelpers.tryBlock visibility via @VisibleForTesting instead of a package-private comment
Replace the repeated inline pattern (commit blocking response, then manually check AppSecContext and call reportBlockFailure() on failure) with the new BlockResponseFunction.tryCommitBlockingResponse(RequestContext, Flow.Action.RequestBlockingAction) default overload added in block-telemetry-2b (#12519). That overload already performs the AppSecContext lookup and reportBlockFailure() call internally, so call sites only need to pass the RequestContext instead of the TraceSegment. Applies to the 37 call sites identified as mechanically substitutable: akka-http-10.0, grizzly-http-2.3.20, jetty-appsec (7.0, 8.1.3, 9.2, 9.3, 9.4, 11.0), jetty-server (7.0, 7.6), liberty (20.0, 23.0), spring-webmvc (3.1, 6.0), and undertow-2.0. Excludes GrizzlyBlockingHelper (grizzly-2.0, manual reflection-based commit) and jetty-server-9.0's JettyCommitResponseInstrumentation (compound res && _committed.get() condition, not mechanically equivalent to the new overload's plain boolean return).
…nd Undertow - Move Grizzly's post-commit span/listener calls inside the try/catch so failures there also trigger reportBlockFailure() - Add missing !success reportBlockFailure() branch to Jetty's before() in 9.0.4/9.3/9.4.21/10.0 - Wrap Undertow's tryCommitBlockingResponse calls in FormDataContentHelper to catch exceptions swallowed by the advice's suppress=Throwable.class
…lure reporting to commit errors - FormDataParserInstrumentation (Undertow): wrap the doParse blocking-commit call with FormDataContentHelper.tryCommitBlockingResponse so a synchronous exception is still reported as a block failure instead of being swallowed by the advice's suppress = Throwable.class. - GrizzlyBlockingHelper: split post-commit finalization (effectivelyBlocked, SpanClosingListener.onAfterService) into its own try/catch so a failure there no longer reports block_failure for a response that was already committed successfully.
- Check the boolean return of FormDataContentHelper.tryCommitBlockingResponse in Undertow's FormDataParserInstrumentation before throwing BlockingException, matching the existing MultiPartUploadHandlerInstrumentation pattern - Add LibertyBlockingHelper.tryCommitBlockingResponse wrapper in liberty-20.0 and liberty-23.0 to report block_failure when the commit itself throws (mirrors Undertow's FormDataContentHelper contract) - Route ParsePostDataInstrumentation, ParseParametersInstrumentation and GetPartsInstrumentation (both Liberty modules) through the new wrapper and check its return value before treating the request as blocked
…aths - Add SpringBlockingHelper (spring-webmvc-3.1 and spring-webmvc-6.0) wrapping BlockResponseFunction#tryCommitBlockingResponse to guarantee reportBlockFailure() is invoked even when the commit call throws. - Route all 6 blocking call sites (HttpMessageConverter read/write, RequestMappingInfoHandlerMapping#handleMatch, UriTemplateVariablesHandlerInterceptor#preHandle, x2 modules) through the helper and only set/throw BlockingException when the commit actually succeeds. - Fix HttpMessageConverterInstrumentationTest assertions that expected the old, unconditional-throw behavior.
…fferEnter - LibertyBlockingHelper (liberty-20.0, liberty-23.0): report the block failure when thiz.reinit()/thiz.finishResponseMessage() throws during syncBufferEnter, matching the existing wrapper's behavior for the parameter/body callback path
ad7c7bb to
12f21e3
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
f44c3b5
into
master
…, Undertow and Akka-http (#12527) Add block-outcome telemetry to blocking response helpers - Wire reportBlockFailure() at every BlockResponseFunction.tryCommitBlockingResponse call site across akka-http, grizzly, jetty (appsec + server), liberty, spring-webmvc and undertow, resolving AppSecContext via RequestContextSlot.APPSEC instead of casting directly to AppSecRequestContext - Add UnmarshallerHelpersBlockFailureTest covering all branches of UnmarshallerHelpers.tryBlock (block committed, block failed, no BRF, foreign/null AppSec slot) - Widen UnmarshallerHelpers.tryBlock visibility via @VisibleForTesting instead of a package-private comment Centralize block_failure reporting via BlockResponseFunction overload Replace the repeated inline pattern (commit blocking response, then manually check AppSecContext and call reportBlockFailure() on failure) with the new BlockResponseFunction.tryCommitBlockingResponse(RequestContext, Flow.Action.RequestBlockingAction) default overload added in block-telemetry-2b (#12519). That overload already performs the AppSecContext lookup and reportBlockFailure() call internally, so call sites only need to pass the RequestContext instead of the TraceSegment. Applies to the 37 call sites identified as mechanically substitutable: akka-http-10.0, grizzly-http-2.3.20, jetty-appsec (7.0, 8.1.3, 9.2, 9.3, 9.4, 11.0), jetty-server (7.0, 7.6), liberty (20.0, 23.0), spring-webmvc (3.1, 6.0), and undertow-2.0. Excludes GrizzlyBlockingHelper (grizzly-2.0, manual reflection-based commit) and jetty-server-9.0's JettyCommitResponseInstrumentation (compound res && _committed.get() condition, not mechanically equivalent to the new overload's plain boolean return). Report block failure on unguarded exception paths in Grizzly, Jetty and Undertow - Move Grizzly's post-commit span/listener calls inside the try/catch so failures there also trigger reportBlockFailure() - Add missing !success reportBlockFailure() branch to Jetty's before() in 9.0.4/9.3/9.4.21/10.0 - Wrap Undertow's tryCommitBlockingResponse calls in FormDataContentHelper to catch exceptions swallowed by the advice's suppress=Throwable.class Guard remaining Undertow form-parse commit call and scope Grizzly failure reporting to commit errors - FormDataParserInstrumentation (Undertow): wrap the doParse blocking-commit call with FormDataContentHelper.tryCommitBlockingResponse so a synchronous exception is still reported as a block failure instead of being swallowed by the advice's suppress = Throwable.class. - GrizzlyBlockingHelper: split post-commit finalization (effectivelyBlocked, SpanClosingListener.onAfterService) into its own try/catch so a failure there no longer reports block_failure for a response that was already committed successfully. Fix block_failure reporting gaps in Undertow and Liberty exception paths - Check the boolean return of FormDataContentHelper.tryCommitBlockingResponse in Undertow's FormDataParserInstrumentation before throwing BlockingException, matching the existing MultiPartUploadHandlerInstrumentation pattern - Add LibertyBlockingHelper.tryCommitBlockingResponse wrapper in liberty-20.0 and liberty-23.0 to report block_failure when the commit itself throws (mirrors Undertow's FormDataContentHelper contract) - Route ParsePostDataInstrumentation, ParseParametersInstrumentation and GetPartsInstrumentation (both Liberty modules) through the new wrapper and check its return value before treating the request as blocked Fix block_failure reporting gaps in Spring-webmvc blocking response paths - Add SpringBlockingHelper (spring-webmvc-3.1 and spring-webmvc-6.0) wrapping BlockResponseFunction#tryCommitBlockingResponse to guarantee reportBlockFailure() is invoked even when the commit call throws. - Route all 6 blocking call sites (HttpMessageConverter read/write, RequestMappingInfoHandlerMapping#handleMatch, UriTemplateVariablesHandlerInterceptor#preHandle, x2 modules) through the helper and only set/throw BlockingException when the commit actually succeeds. - Fix HttpMessageConverterInstrumentationTest assertions that expected the old, unconditional-throw behavior. Report block_failure on Liberty direct response-commit path in syncBufferEnter - LibertyBlockingHelper (liberty-20.0, liberty-23.0): report the block failure when thiz.reinit()/thiz.finishResponseMessage() throws during syncBufferEnter, matching the existing wrapper's behavior for the parameter/body callback path Merge branch 'master' into block-telemetry-3 Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>
2 participants
What Does This Do
Wires block-outcome telemetry (
AppSecContext.reportBlockFailure()) into the six remainingframeworks: Grizzly, Jetty, Liberty, Spring-webmvc, Undertow, and Akka-http. Follows the canonical
pattern already established for Netty (#12316) and Tomcat (#12493): resolve the
BlockResponseFunction, attempt the commit throughtryCommitBlockingResponse(...), and onfalse(or on exception) report the failure viaAppSecContext- resolved throughRequestContext#getData(RequestContextSlot.APPSEC)+ aninstanceofcheck, never a direct cast toAppSecRequestContext.tryCommitBlockingResponsecall site across:grizzly-2.0,grizzly-http-2.3.20,jetty-appsec-{7.0,8.1.3,9.2,9.3,9.4,11.0},jetty-server-{7.0,7.6,9.0,9.0.4,9.3,9.4.21,10.0,11.0},liberty-{20.0,23.0},spring-webmvc-{3.1,6.0},undertow-2.0andakka-http-10.0UnmarshallerHelpersBlockFailureTest(JUnit 5) covering all branches ofUnmarshallerHelpers.tryBlock: block committed, block failed, noBlockResponseFunction,foreign/null AppSec slot
UnmarshallerHelpers.tryBlockvisibility via@VisibleForTestinginstead of apackage-private comment, matching the existing repo convention
Follow-up: try/catch wrapper helpers for Undertow, Liberty and Spring-webmvc
A round of Codex review on this PR surfaced call sites where an exception thrown while committing
the blocking response was swallowed by the advice's
suppress = Throwable.classbefore it everreached
reportBlockFailure()- so a genuine commit failure went unreported whenever the commititself threw, as opposed to returning
falsecleanly. Fixed by introducing a small per-modulewrapper (
FormDataContentHelperin Undertow,LibertyBlockingHelperin Liberty,SpringBlockingHelperin Spring-webmvc x2) that callstryCommitBlockingResponse(RequestContext, RequestBlockingAction)inside an explicittry/catch, reports the failure on either a caughtexception or a clean
false, and returns a boolean the caller can trust. Spring-webmvc'sHttpMessageConverterInstrumentationTestwas updated to assertnotThrown(BlockingException)(notthrown) on a clean commit failure, since the advice now correctly skips throwing whentryCommitBlockingResponsereturnsfalse.Follow-up: additional unguarded paths found by Codex (Grizzly, Jetty, Liberty)
Three more rounds of Codex review surfaced gaps in frameworks that have more than one independent
blocking-commit code path (request-intake vs. response-header), or where post-commit bookkeeping
needed to be separated from the actual commit attempt:
GrizzlyBlockingHelper.block(...)): the response-commit steps and the post-commitbookkeeping (
effectivelyBlocked(),SpanClosingListener.onAfterService(...)) are now in twoseparate
try/catchblocks. The first reportsblock_failureon exception; the second stillcatches (so a bookkeeping failure can't leak past the advice's
suppress = Throwable.class) butdoes NOT report
block_failure, since the response was already committed successfully by thatpoint.
(
JettyCommitResponseHelper->JettyOnCommitBlockingHelper.block(...)) that the rest of thejetty-appsec/jetty-server call sites never touch. Added the missing
!success->reportBlockFailure()branch there too. No new automated test: forcingJettyOnCommitBlockingHelper.block(...)to returnfalsedeterministically needs areflection/MethodHandle-init failure across 4 distinct Jetty-version internals, which isn't
practical without a brittle test tied to Jetty-version internals - documented as an accepted
coverage gap.
HttpInboundServiceContextImplInstrumentation.SyncAdviceBuffer->syncBufferEnter()directly,bypassing the parameter/body wrapper above entirely. Added the same guarded-exception reporting
to that path's commit attempt (
thiz.reinit()/thiz.finishResponseMessage()).Known gap: several
BlockResponseFunctionimplementations still cannot signal a genuine commit failurefound call sites (Grizzly's
ParsedBodyParametersInstrumentation, six files underjetty-appsec-{7.0,9.2,9.3,9.4,11.0}) that discard the boolean returned bytryCommitBlockingResponseand unconditionally report failure regardless of the actual outcome,plus a
JettyBlockingHelper.block()that catchesThrowableand still returnstrueunconditionally. None of this has been fixed in this PR - it needs its own follow-up under
APPSEC-62696, tracked separately from the try/catch wrapper fix above.
BlockResponseFunctionimplementation still always returnstrueafter a genuine commit attempt regardless of whether the commit actually succeeded (theonly reachable
falsevalues are pre-attempt guards: missing reflection handle, no active span,wrong request/response type, process-wide init state). This PR's wrapper helpers do make
reportBlockFailure()fire correctly on any exception thrown during the commit attempt, but acommit that returns cleanly without throwing is still assumed to have succeeded.
AkkaBlockResponseFunction.tryCommitBlockingResponsereturningtruemeans theRequestBlockingActionwas stashed for later use, not that the HTTP response was committed. Theactual response substitution happens out-of-band via
maybeCreateAlternativeResponse(). Thisreturn value is structurally incapable of signaling a genuine commit failure.
Fixing the remaining Grizzly/Jetty gaps and the Undertow/Liberty/Akka-http structural limitations is
out of scope for this PR. It should be tracked as a separate tech-debt follow-up under APPSEC-62696.
Motivation
Part of APPSEC-62696: block-outcome telemetry, extending the pattern already shipped for Netty and
Tomcat to the remaining instrumented servers.
Additional Notes
reportBlockFailure()wiring duplicated across the six frameworks isintentional: each module has its own
BlockResponseFunction, classloader scope and bytecodediscriminator, so no cross-module helper is introduced by design.
instanceofcheck plus a conditional method call on the existing commit-attempt path, not a new hot path.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: APPSEC-62696
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.