internal/dag: allow routes with overlapping header conditions #2600
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes a bug where an HTTPProxy with multiple routes with the same prefixes and conditions on the same headers, differing only on the condition operators or values, would result in only the last route being configured in Envoy and all others being dropped. Signed-off-by: Steve Kriss <krisss@vmware.com>
Codecov Report
@@ Coverage Diff @@
## master #2600 +/- ##
==========================================
- Coverage 76.85% 76.83% -0.02%
==========================================
Files 71 71
Lines 5499 5505 +6
==========================================
+ Hits 4226 4230 +4
- Misses 1187 1188 +1
- Partials 86 87 +1
|
| @@ -79,7 +80,14 @@ type HeaderCondition struct { | |||
| } | |||
|
|
|||
| func (hc *HeaderCondition) String() string { | |||
| return "header: " + hc.Name | |||
| details := strings.Join([]string{ | |||
There was a problem hiding this comment.
Maybe there are cases where different routes are required. So maybe this is the way to go.
However an incoming request (method:GET, Prefix:/) will match both -
(HeaderName: ":method", MatchType: "Exact", HeaderValue: "GET")
(HeaderName: ":method", MatchType: "Contains", HeaderValue: "GET")
Allowing routes differing on MatchType may create duplicates.
There was a problem hiding this comment.
I think ideally we could detect if routes created won't match and set the status on the route, just not sure how difficult determining all those scenarios are. We'd have to match on duplicate header types of different types as @chintan8saaras noted.
There was a problem hiding this comment.
Seems like there are a couple of related improvements we could make here:
- within a single route, determine when one condition is a proper subset of another (e.g. "exact: foo" and "contains: foo" on the same header), and collapse them into just a single condition (in this case, just "exact: foo").
- when two routes can be combined similarly to the above, collapse them into a single route
- when there are conflicting header conditions for a given route (e.g. both "contains: foo" and "notcontains: foo"), mark the proxy as invalid. This is already done for conflicting "exact" conditions, but we could strengthen this validation.
youngnick
approved these changes
Jun 19, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #2597
Fixes a bug where an HTTPProxy with multiple routes with the same prefixes
and conditions on the same headers, differing only on the condition operators
or values, would result in only the last route being configured in Envoy and
all others being dropped.
Signed-off-by: Steve Kriss krisss@vmware.com