Combine ingress paths and route rules #661
Conversation
|
Jenkins job manager/presubmit passed |
| } | ||
| } | ||
|
|
||
| if useDefaultRoute { |
There was a problem hiding this comment.
I think we also need to add rules with a single route (same destination, 100 weight) as a catch all route.
examples of catch all routes:
- rule which has prefix /, no http headers and no path
- rule which has no match, with single route (weight 100)
- any thing else @frankbu ??
There was a problem hiding this comment.
The logic is same as it was before. I just moved catchAll to a function to simplify the control flow. Is there a specific example when the default rule is added or not added?
| // CatchAll returns true if the route matches all requests | ||
| func (route *HTTPRoute) CatchAll() bool { | ||
| return len(route.Headers) == 0 && route.Path == "" && route.Prefix == "/" | ||
| } |
There was a problem hiding this comment.
if my http route rule does not provide path/prefix/headers, will route.Prefix still be set to / ?
There was a problem hiding this comment.
The function assumes the route is valid (one of path or prefix is set). It looks like we set path and prefix in every route soon after instantiation. It's probably a good idea to move it to a factory method so that no invalid routes are created.
| { | ||
| "name": "out.66fcc955b8875b19844f9eaf6cfda47c778c609e", | ||
| "weight": 25 | ||
| } |
There was a problem hiding this comment.
How do we match headers as well here?
There was a problem hiding this comment.
If there is a route rule on destination that uses headers, there should be two routes - one with header match, and another for catch-all. Both should be added, as long as they are compatible with ingress path/prefix.
| } | ||
| return dur | ||
| } | ||
|
|
||
| func protoDurationToMS(dur *duration.Duration) int64 { |
|
jenkins rebuild manager/e2e-smoketest |
|
|
||
| if len(routes) > 0 { |
There was a problem hiding this comment.
(optional): invert conditional check and continue to reduce nesting depth
| func buildURIPathPrefix(matches *proxyconfig.MatchCondition) (path string, prefix string) { | ||
| path = "" | ||
| prefix = "/" | ||
| if matches != nil { |
There was a problem hiding this comment.
(optional): invert conditional check and continue to reduce nesting depth
|
jenkins rebuild manager/e2e-smoketest |
|
I'll follow up with a smaller PR to address some header/and code style things I noticed. |
|
jenkins rebuild manager/e2e-smoketest |
2 similar comments
|
jenkins rebuild manager/e2e-smoketest |
|
jenkins rebuild manager/e2e-smoketest |
|
jenkins rebuild manager/e2e-smoketest |
|
Jenkins job manager/presubmit passed |
|
Jenkins job manager/e2e-smoketest passed |
|
Jenkins job manager/presubmit passed |
|
Jenkins job manager/e2e-smoketest passed |
The goal is to enable route rules on ingress rule destinations. I had to make concessions since it is not trivial to combine two Envoy HTTP routes. The ingress routes have the following restrictions:
These restrictions are not serious since we do not generate ingress rules that violate any of these from the ingress controller.
Additionally, I cleaned up some of the logic related to header matching: quoting prefix in a regex, detecting if a route is a catch-all, resolve service ports during route generation rather than rule generation.
Added a test to combine ingress with a route rule on "c".