-
Notifications
You must be signed in to change notification settings - Fork 592
fix: prevent configuring requestMirror filter and directResponse/RequestRedirect filter together #7474
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7474 +/- ##
==========================================
- Coverage 72.28% 72.26% -0.02%
==========================================
Files 231 231
Lines 34084 34088 +4
==========================================
- Hits 24636 24633 -3
- Misses 7674 7679 +5
- Partials 1774 1776 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9d8cf8c to
532a1f2
Compare
532a1f2 to
f7a0e10
Compare
07b6933 to
1983322
Compare
262675f to
cd6cef5
Compare
…ether Signed-off-by: Huabing Zhao <[email protected]>
Signed-off-by: Huabing Zhao <[email protected]>
cd6cef5 to
66471dc
Compare
|
/retest |
| name: gateway-1 | ||
| sectionName: http | ||
| rules: | ||
| - matches: |
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.
can we add a mixed rule case, where one rule is valid and one isnt, to make sure the valid rule is not impacted
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.
According to the Gateway API specific, The HTTPRouteAccepted condition should be set to false with IncompatibleFilters reason when there are incompatible filters present on a route rule.
And EG skips the HTTPRoutes with Accepted status as False, which is reasonable as the entire HTTPRoute has been rejected.
gateway/internal/gatewayapi/route.go
Lines 147 to 149 in 36a23a6
| if parentRef.HasCondition(httpRoute, gwapiv1.RouteConditionAccepted, metav1.ConditionFalse) { | |
| continue | |
| } |
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.
Checked the Gateway API spec again, it seems that we should set Accepted as true as long as at least one rule is valid and implemented, even if some other rules in the same HTTPRoute are invalid.
// A Route MUST be considered "Accepted" if at least one of the Route's
// rules is implemented by the Gateway.
Changing this behavior would require a global update and could affect other cases as well. We'd better handle it in a separate PR. If we agree on this, I can raise a PR to address it first.
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.
yeah can we tackle this as part of #7545 and then start adding more fail fast cases
Fixes #7473
Release Notes: Yes