fix: Add overlap status for duplicate HTTPRoutes#8373
fix: Add overlap status for duplicate HTTPRoutes#8373jukie wants to merge 3 commits intoenvoyproxy:mainfrom
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 canceled.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8373 +/- ##
==========================================
- Coverage 74.18% 74.15% -0.04%
==========================================
Files 242 242
Lines 37579 37697 +118
==========================================
+ Hits 27879 27953 +74
- Misses 7760 7790 +30
- Partials 1940 1954 +14 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
/retest |
|
Ignore above, it's fixed by #8186 |
| // indicating that they match the same set of requests. | ||
| // This detects routes with the same hostname and identical exact path, header, query param, | ||
| // and cookie match conditions. Only routes with explicit exact path matches are checked; | ||
| // prefix path matches can intentionally overlap across HTTPRoute resources |
There was a problem hiding this comment.
intersection/overlap case seems like a useful signal in status
internal/gatewayapi/route.go
Outdated
| } | ||
|
|
||
| if hasOverlap { | ||
| routeStatus := GetRouteStatus(httpRoute) |
There was a problem hiding this comment.
should we keep resolvedRefs as as, and add an additonal warning condition ?
There was a problem hiding this comment.
+1
Probably just set Accepted as False with the conflicted message?
There was a problem hiding this comment.
accepted=false has a bigger impact imo, this is more of an Ignore case, similar to the Overriden condition in Policy which is tied to a subset of rules and matches in the route
| status: "True" | ||
| type: Accepted | ||
| - lastTransitionTime: null | ||
| message: Overlapping match conditions with another HTTPRoute on the same listener |
There was a problem hiding this comment.
Could we include the name of the conflicting routes in the status message? That would make it much easier for users to identify the source of the conflict when this happens.
Signed-off-by: jukie <isaac.wilson514@gmail.com>
Signed-off-by: jukie <isaac.wilson514@gmail.com>
| overlap: false, | ||
| }, | ||
| { | ||
| name: "prefix paths are not detected as overlap", |
There was a problem hiding this comment.
Should two prefix matches with the same value also be considered overlapping? This could also cause simliar confusion for users, like exact does in #8101 when the backends are different.
| overlap: false, | ||
| }, | ||
| { | ||
| name: "nil path matches are not detected as overlap", |
There was a problem hiding this comment.
Same here, should this also be considered overlapping?
| overlap: true, | ||
| }, | ||
| { | ||
| name: "regex path matches are not detected as overlap", |
There was a problem hiding this comment.
Same here, should this also be considered overlapping?
4 participants
What this PR does / why we need it:
The first route that gets applied will take precedence but subsequent routes don't show any indication that they're being ignored. This adds a an
Overlapstatus to ResolvedRefs to communicate this behavior to users.Fixes #8101
Release Notes: Yes