[WiP] L7 routing straw man. #4
Conversation
The base type messages are inlined, they will be split out into separate packages before we commit. This is very early WiP, focused on the needs of header/path matching only. Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
|
|
||
| // TODO(htuch): what other parts of RouteAction are shared by known DPLBs? | ||
|
|
||
| // DPLB specific behaviors, e.g. traffic mirroring, rate limiting, request |
There was a problem hiding this comment.
I would consider this to be the most controversial feature, as it allow a full escape hatch for proxy specific forwarding actions. This simultaneously provides significant flexibility and provides potential for fragmentation of control planes (or the need to condition on clients).
There was a problem hiding this comment.
I don't see anyway around this. Similar likely for additional match types below? I think this probably should be a repeated also?
There was a problem hiding this comment.
Yeah, I think we should have these in more places, I'll add. Why repeated? I'm not really thinking of this as a chain of extensions, rather a single opaque value for all the DPLB route action specifics.
There was a problem hiding this comment.
I was just thinking that we could then have a list of individual things vs. a container type, but I agree it doesn't really matter so whatever you think is best.
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
There was a problem hiding this comment.
Thanks this is awesome and definitely more in line with what I had in mind. I left some additional comments for discussion and I think at this point it would be worth it to beef up the comments on each field so it would be easier for new folks to come in and read and comment? WDYT?
|
|
||
| // TODO(htuch): what other parts of RouteAction are shared by known DPLBs? | ||
|
|
||
| // DPLB specific behaviors, e.g. traffic mirroring, rate limiting, request |
There was a problem hiding this comment.
I don't see anyway around this. Similar likely for additional match types below? I think this probably should be a repeated also?
| HeaderMatcher header_matcher = 1; | ||
| } | ||
|
|
||
| // Fall-thru action if no other match occurs. |
There was a problem hiding this comment.
Is this required? Or is it implementation dependent what happens if there is no fall-thru?
There was a problem hiding this comment.
Should we just say that a request will 404 in the event of no match? There are other situations where a default behavior would make sense, so either we document it or promote to the top-level RouteConfiguration.
There was a problem hiding this comment.
Actually, it's still useful to have this as optional, in any case, the point is that having well defined "no match action" is a useful thing.
| // matchers. | ||
| message LinearMatcher { | ||
| message MatchAction { | ||
| message HeaderMatcher { |
There was a problem hiding this comment.
Do you think there is a way we can share HeaderMatcher between hierarchical mode and linear mode? I get there are subtle differences, but it seems like we should be able to do this somehow? WDYT?
There was a problem hiding this comment.
I'll think about it; I'd like to be able to simplify, just not sure if I can see the best way to do this.
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
| // Redirects are a copy+paste of the Envoy v2 xDS | ||
| // envoy.api.v2.route.RedirectAction message. While this could be rewritten | ||
| // significantly for clarity, the idea is that the behaviors here seem very | ||
| // implementation non-specific. TODO(htuch): Can all the DPLB implementations |
There was a problem hiding this comment.
gRPC does not support redirects.
Rather than baking this into the protocol, I think it would make more sense to have the Action be only an extension point to define action types. UDPA can also provide some common action type protos that can be used by implementations that support those action types.
| option (validate.required) = true; | ||
|
|
||
| // Idempotent return of the header value. | ||
| google.protobuf.Empty exact = 1; |
There was a problem hiding this comment.
Empty? Did you mean String for these two fields?
| repeated MatchAction match_actions = 1; | ||
| } | ||
|
|
||
| message Matcher { |
There was a problem hiding this comment.
It's worth noting that the current xDS data model actually uses these matchers for two things:
- Determining configuration parameters for things like timeouts and retries.
- Determining how to route requests (e.g., which cluster to send the request to).
Note that routing could actually be done by the LB policy instead of being done here. If we wind up going with the extensible hierarchical LB config approach that @snowp and I are discussing in envoyproxy/envoy#5598, then this would even provide a clean and extensible way to choose between linear and hierarchical matching -- we could just implement each level of the hierarchical match in its own LB policy, so that we would not even need a separate configuration structure for linear vs. hierarchical matching.
I think that we would still need some matching criteria here for determining configuration parameters for things like timeouts and retries. But if we push most of this complexity into the LB policy, can we make what remains here much simpler? I am wondering if we can live with just some very basic criteria here, such as prefix path matching only, to select the configuration. This would allow a bit less flexibility in selecting the configuration parameters, but it would make the routing much more extensible, and it would address a fairly signficant mismatch between the xDS data model and the gRPC architecture that is causing some friction.
We ran into this just recently as we started looking into supporting some of this matching in gRPC, and we quickly realized that we have a bit of a chicken-and-egg problem: we handle config selection via a resolver plugin, which does not have access to per-request data, and we handle request routing via an LB policy plugin, which runs after config selection has already been performed. For now, we're going to handle this by supporting only very simple path-prefix matching, which is essentially the mechanism we currently use to select configs on a per-method basis. But because the current xDS data model ties the config selection and routing matching together, this means that we can't support things like regexp matching or matching on headers, which we could otherwise support for request routing. (There are a number of reasons why we don't want to support more complex matching for config selection, mainly related to performance and complexity.) If we could decouple these two things, then we could support the more flexible matching for request routing.
So I guess the question here is, could we make the matching criteria here simpler if they were going to be used only for selecting configuration parameters like timeouts and retries?
There was a problem hiding this comment.
I can see the appeal behind such an approach, but I worry that it's too big of a change given how Envoy operates today: for HTTP proxies today each filter has access to the routing decision (i.e. which routing action to make, which cluster to target, etc.) and is able to act on that.
The load balancing choice today happens at the terminal filter, and the interface essentially just returns a handle to the upstream host to connect to. By pushing more of what is currently expressed in the route table into the load balancer none of that information would be made available to the various filters.
Now, it might be feasible to split routing and config selection apart without pushing it into the load balancer. I imagine we could have two separate resolution steps, one for determining the config and one for routing. That way on the Envoy side both results could be cached as the route selection is today, and gRPC would able to implement each resolver where it makes the most sense.
|
|
||
| // Match-action table for string values. | ||
| message StringMatcher { | ||
| // How will the string value be matched? Regex matcher is not allo |
|
Closing this out in favor of the new generic matchers added to xDS. |
The base type messages are inlined, they will be split out into separate
packages before we commit.
This is very early WiP, focused on the needs of header/path matching
only.
Signed-off-by: Harvey Tuch [email protected]