Repository navigation
fix(gatewayapi): return 500 for route rules with invalid filters - #10454
vshalpnjabi wants to merge 2 commits into
Conversation
Follows the Gateway API guidance that implementations SHOULD treat a route rule with an invalid filter combination or order as invalid. Filter-processing errors now discard any redirect and install a 500 DirectResponse on both HTTPRoute and GRPCRoute, matching the direction agreed on envoyproxy#9460. Signed-off-by: Vishal Punjabi <10079537+vshalpnjabi@users.noreply.github.com>
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
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. |
![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: f0344a30d1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| switch { | ||
| // return 500 if any filter processing error occurred | ||
| // Invalid filters make the rule invalid: return 500 and drop any redirect. |
There was a problem hiding this comment.
When a DirectResponse ExtensionRef precedes the bad filter, ProcessHTTPFilters and ProcessGRPCFilters stop on the existing direct response at filters.go:142-145 and filters.go:269-272, leaving processFilterError nil, so this new 500 path is never run. Reversing the two filters in either new fixture therefore still serves the configured 418/404 and reports the rule as valid; continue validating later filters (while suppressing their effects) so filter order cannot bypass the invalid-rule response.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for pointing this out. Filters after a DirectResponse are now still validated without applying their effects, so an invalid filter later in the list returns 500. Added HTTPRoute and GRPCRoute tests for that order.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10454 +/- ##
==========================================
- Coverage 82.28% 82.27% -0.01%
==========================================
Files 277 277
Lines 43617 43617
==========================================
- Hits 35891 35887 -4
- Misses 7725 7729 +4
Partials 1 1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
A valid DirectResponse stopped later filter processing, so listing it before an invalid filter still served 418/404 and left the rule accepted. Keep validating remaining filters without applying their IR effects so filter order cannot bypass the invalid-rule 500. Signed-off-by: Vishal Punjabi <10079537+vshalpnjabi@users.noreply.github.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdfec4aff2
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return &HTTPFiltersContext{ | ||
| ParentRef: ctx.ParentRef, | ||
| Route: ctx.Route, | ||
| RuleIdx: ctx.RuleIdx, | ||
| HTTPFilterIR: &HTTPFilterIR{}, |
There was a problem hiding this comment.
The prior terminal break is gone, but this helper creates a brand-new empty HTTPFilterIR for every filter after a DirectResponse. If a valid RequestMirror (or gRPC JSON transcoder) follows that response, its state is discarded before the post-loop incompatibility checks in ProcessHTTPFilters, so the pair is accepted and the configured response is served; the same filters in reverse order are rejected. Keep one cumulative validation context containing the earlier terminal state while suppressing only emitted effects.
Useful? React with 👍 / 👎.
| irRoute.Redirect = nil | ||
| irRoute.DirectResponse = &ir.CustomResponse{ | ||
| StatusCode: new(uint32(500)), |
There was a problem hiding this comment.
When this is the only invalid GRPCRoute rule, the 500 built here never reaches xDS: processGRPCRouteParentRefs sets Accepted=False and takes the lines 1458-1460 continue before processHTTPRouteParentRefListener. Both new GRPC fixtures add a second valid rule specifically to avoid that path, so a normal single-rule route still has no matching 500 even though the added release note promises a 500 while Accepted remains false. Attach these generated error routes before the all-rules-invalid short-circuit, as the HTTPRoute flow already does.
Useful? React with 👍 / 👎.
| irRoute.Redirect = nil | ||
| irRoute.DirectResponse = &ir.CustomResponse{ | ||
| StatusCode: new(uint32(500)), | ||
| } |
There was a problem hiding this comment.
When a DirectResponse with contentType or header precedes the bad filter, applyHTTPFiltersContextToIRRoute has already copied those values into irRoute.AddResponseHeaders; this branch replaces only the DirectResponse object and clears Redirect. The new httproute-with-directresponse-then-invalid-filter.out.yaml golden therefore emits Content-Type: text/plain on the generated 500, and arbitrary DirectResponse headers such as Set-Cookie would likewise survive even though that response was rejected. Remove the old DirectResponse header mutations when installing the error response.
Useful? React with 👍 / 👎.
What this PR does / why we need it:
Gateway API says implementations SHOULD treat a route rule with an invalid filter combination or order as invalid. Today an HTTPRoute filter error still leaves a RequestRedirect on the IR next to the 500, and GRPCRoute skips the 500 entirely if a filter already set DirectResponse or Redirect.
This follows the direction agreed on #9460 (thanks @Aias00): for both HTTPRoute and GRPCRoute, a filter-processing error discards any redirect, replaces a filter-set direct response, and programs 500. Accepted=False (or PartiallyInvalid when some rules are still valid) is unchanged. Destination and no-ready-endpoint handling is left as-is; the spec text is about filters, not backends.
Which issue(s) this PR fixes:
Fixes #9454
PR Checklist
git commit -s). See DCO: Sign your work.go testandgo test -raceon./internal/gatewayapi/,make lint.golint,gofmt, the release-notes filename check, and yamllint and codespell on the changed files all pass. The new goldens fail on main and pass with the fix. I didn't runmake generate gen-checkor the fullmake lintlocally, since there are no API or generated changes.httproute-with-invalid-filter-and-redirectandgrpcroute-with-invalid-filter-and-directresponsetranslator goldens, and dropped the leftover redirect fromhttproutefilter-with-grpc-json-transcoder-and-url-rewrite.release-notes/current/bug_fixes/9454-invalid-filter-rule-500.md.