[WIP] router: support cluster picking via filter state and fallback

[AndresGuedez]

NOTE: this is a WIP PR to get feedback on a proposed API change to RouteAction.

This change introduces the following modifications to the routing API:

1. Support cluster picking via filter state.
2. Add a repeated field that enables fallback logic to be configured for cluster picking; e.g., if cluster_header fails to find an active cluster, the next selector in the list will be attempted.

[repokitteh-read-only]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11661 was opened by AndresGuedez.

see: more, trace.

[markdroth]

@htuch @mattklein123 Isn't this the same use-case that we planned to address via #8956?

[mattklein123]

In general this makes sense to me, thanks. I left some comments to discuss.

/wait

[mattklein123]

Within the selector, the types should be a required oneof, right?

Also, re: my comment below about fallback routing, I would suggest that we plan for this selector to eventually have other state provided such as when a cluster is valid. For example, it's current health state, etc. wdyt?

[AndresGuedez]

Yes, it should be a oneof. I've just pushed a new commit addressing this.

Also, re: my comment below about fallback routing, I would suggest that we plan for this selector to eventually have other state provided such as when a cluster is valid. For example, it's current health state, etc. wdyt?

Yeah, this SGTM. I'll put more thought into this proto's structure with that in mind.

[mattklein123]

min size 1?

[AndresGuedez]

Fixed.

[mattklein123]

If we go with this can we just go ahead and do the deprecation? I agree this is the right path forward.

[AndresGuedez]

SGTM.

[htuch]

+1 on deprecating existing redundant selectors.

[htuch]

I think this should be [#not-implemented-hide:] until the implementation lands.

[htuch]

I think this should be [#not-implemented-hide:] until the implementation lands.

[htuch]

I think we need some worked examples on how fallback is going to happen with WeightedCluster, either here or in the routing architecture docs. Basically, it needs to explain that we keep rolling the dice at each successive fallback, rather than rolling the dice once with the options filtered down by cluster liveness.

[htuch]

@markdroth I think there is overlap with the filter state action and #8956 (but not the generalized cluster fallback aspect).

[markdroth]

@htuch wrote:

@markdroth I think there is overlap with the filter state action and #8956 (but not the generalized cluster fallback aspect).

Maybe it would help me to understand the overall goal here. It seems like this PR is a cross between the filter state action API we added in #8956 and the aggregate cluster functionality that allows falling back from a primary cluster to a secondary one when the primary is not reachable. Is there something else here that I'm missing? If not, why is this needed? Can't this just be accomplished through the two existing mechanisms?

[htuch]

@markdroth the main thing you can't do with aggregate clusters is deal with the eventual consistency mismatch between RDS and CDS. This is not the use case of @AndresGuedez, but a simple example of this issue is how do you handle a missing cluster in RouteConfiguration while waiting for CDS to update? I think some folks have historically asked for fallback to a static cluster, this new API would allow that.

[markdroth]

@markdroth the main thing you can't do with aggregate clusters is deal with the eventual consistency mismatch between RDS and CDS. This is not the use case of @AndresGuedez, but a simple example of this issue is how do you handle a missing cluster in RouteConfiguration while waiting for CDS to update? I think some folks have historically asked for fallback to a static cluster, this new API would allow that.

I assume that the intent here is for the route to specify multiple clusters and the filter to choose between them based on the state of those clusters? If so, how is this different from the aggregate cluster functionality?

I will also note that this problem seems like a fundamental property of the wildcard-semantic approach that Envoy uses for CDS. We don't have this problem in gRPC, because we make a CDS query for whatever clusters are specified in the RouteConfiguration. If we get an RDS update containing a new cluster name, any request for that new cluster will be queued until we get the necessary CDS (and EDS) resources. If we don't get those resources within some timeout period, then we consider the cluster to be in state TRANSIENT_FAILURE, and those requests will fail. This approach works fine and does not require any API change.

[htuch]

The problem with aggregate cluster is you need to know all the constituent cluster in the aggregate cluster when delivering CDS. So, if you are late in your eventual consistent update of CDS for some new cluster, relative to RDS, it's not going to be available until the CDS update is received.

Another example, closer to what @AndresGuedez wants, is the ability to pick cluster based on header, but have a fallback cluster for when the header yields a cluster that doesn't exist on the Envoy.

[markdroth]

The problem with aggregate cluster is you need to know all the constituent cluster in the aggregate cluster when delivering CDS. So, if you are late in your eventual consistent update of CDS for some new cluster, relative to RDS, it's not going to be available until the CDS update is received.

The aggregate cluster definition comes as part of the CDS response, not the RDS response. If we're getting all of the clusters from the CDS response, including the aggregate cluster, doesn't that eliminate the consistency problem between CDS and RDS?

Another example, closer to what @AndresGuedez wants, is the ability to pick cluster based on header, but have a fallback cluster for when the header yields a cluster that doesn't exist on the Envoy.

Can't this be done using the API we added in #8956? Why do we need a separate API for this?

[AndresGuedez]

Can't this be done using the API we added in #8956? Why do we need a separate API for this?

AFAICT, this API does appear to provide a solution to the use cases that have come up as part of this discussion thus far. The caveat is that it requires a filter, but that's not an issue for my deployment environment.

It is arguably less user friendly than building the cluster picking fallback behavior directly into Envoy's core, but that's an expected tradeoff for being generic and enabling much greater flexibility.

@htuch @mattklein123 I don't have the context for the other use cases that you've alluded to that could leverage the API I've proposed in this PR, so do you have a preference in approach? Are some of these use cases not well served by FilterActions?

[htuch]

I think there are a few asks and tensions here:

• We want to keep RouteConfiguration API to a minimal set of orthogonal primitives.
• We want to simplify and decompose router.
• We probably want to have a convenient UX to be able to do cluster fallback.
• We want to be able to handle missing clusters regardless of CDS state (i.e. aggregate clusters are out of scope).

Anything else I'm missing?

[markdroth]

@htuch I would add one thing, which you've already alluded to in several of your bullet points but that I want to state explicitly: I think we should try as much as possible to have only one way to do a given thing. For example, if we are going to add support for fallback clusters here, does that eliminate the need for the aggregate cluster functionality?

[mattklein123]

For example, if we are going to add support for fallback clusters here, does that eliminate the need for the aggregate cluster functionality?

I don't have a good understanding of how aggregate clusters are used today. If we think that aggregate clusters can fully support the fallback case that is potentially fine with me, though this API as proposed seems conceptually pretty simple and easy to use for some very common fallback cases which have been requested over and over again.

[htuch]

In terms of UX, what would a FilterAction based solution actually look like? I'm thinking you would have some per-route filter config for a fallback-filter provided essentially a list of ClusterSelectors in this config. Then you would need fallback-filter to be able to do do the cluster picking - it would probably need to share a bunch of code with router in any case.

Or is there a simpler way to do achieve this kind of fallback API for cluster selectors that would work with FilterAction?

[AndresGuedez]

Or is there a simpler way to do achieve this kind of fallback API for cluster selectors that would work with FilterAction?

Which protos are expected to be specified in the Any field of the FilterAction?

My thinking is that a fallback-filter could mimic the desired fallback cluster picking logic by specifying a cluster_header based action first, attempting cluster picking (e.g., by calling StreamFilterCallbacks::clusterInfo() which itself calls refreshCachedRoute()), checking the return and then specifying a subsequent fallback action (e.g., weighted_clusters) if needed.

[markdroth]

The use-case that originally motivated the FilterAction API was to allow a filter to pick the cluster, which we would do by extracting a key from per-request headers and consulting an external control plane to pick the cluster for each key. As per discussion in #8953, I originally wanted to just add a 4th option to the cluster_specifier oneof, but the thought was that having a way for filters to generate the entire RouteAction would be a more general solution, because that would allow the filter to set not just the cluster but also things like timeout, retry, etc.

The intent of the FilterAction API is that the contents of the Any field are basically the configuration for the filter, which can be anything the filter needs, and it's the filter's job to generate the RouteAction to be used by the router. The filter should not need to duplicate code from the router; it should just need to tell the router what RouteAction to use. (I don't think anyone ever implemented the machinery in the router for this, but that was how we discussed that it would work.)

My intent for our use case was that we would put something like this inside the Any:

message FilterChoosesCluster {
  // A RouteAction with the cluster field set to "__not_used_determined_by_filter".
  // The filter will replace that with the chosen cluster and return the resulting RouteAction.
  // This provides a mechanism to set the other RouteAction fields.
  RouteAction route_action_skeleton = 1;
  // ...other config to tell the filter how to pick the cluster...
}

I think you could do something similar for the fallback case. The "other config to tell the filter how to pick the cluster" could be a repeated list of clusters to choose from.

That having been said, I would still like to understand why this use case cannot be addressed by the aggregate cluster functionality. In order for fallback to work, you need to know that there is at least one cluster that is already present in the CDS response, or else you have nothing to fall back to. If you can be sure that at least one cluster is already present in CDS, why can't that be an aggregate cluster?

[AndresGuedez]

The intent of the FilterAction API is that the contents of the Any field are basically the configuration for the filter, which can be anything the filter needs, and it's the filter's job to generate the RouteAction to be used by the router. The filter should not need to duplicate code from the router; it should just need to tell the router what RouteAction to use. (I don't think anyone ever implemented the machinery in the router for this, but that was how we discussed that it would work.)

Great, this is what I was expecting/hoping.

My intent for our use case was that we would put something like this inside the Any:

message FilterChoosesCluster {
  // A RouteAction with the cluster field set to "__not_used_determined_by_filter".
  // The filter will replace that with the chosen cluster and return the resulting RouteAction.
  // This provides a mechanism to set the other RouteAction fields.
  RouteAction route_action_skeleton = 1;
  // ...other config to tell the filter how to pick the cluster...
}

I think you could do something similar for the fallback case. The "other config to tell the filter how to pick the cluster" could be a repeated list of clusters to choose from.

Right, as long as the filter is provided an API to configure the RouteAction to use, both your example with an ordered list of fallback clusters and my use case which requires weighted cluster picking as fallback could be satisfied.

That having been said, I would still like to understand why this use case cannot be addressed by the aggregate cluster functionality. In order for fallback to work, you need to know that there is at least one cluster that is already present in the CDS response, or else you have nothing to fall back to. If you can be sure that at least one cluster is already present in CDS, why can't that be an aggregate cluster?

A guarantee that a cluster is configured, active and/or healthy is not required for my use case. We need to iterate cluster picking mechanisms, and if a cluster is not found, an error response would be returned to the client. I've emphasized "mechanisms" as that is a key difference with the use case you are describing; we first want to attempt e.g., cluster_header based picking, and then weighted cluster picking. Therefore, I don't think the aggregate cluster capability would be a viable solution for this use case.

[markdroth]

Right, as long as the filter is provided an API to configure the RouteAction to use, both your example with an ordered list of fallback clusters and my use case which requires weighted cluster picking as fallback could be satisfied.

Okay. Then maybe we should just implement the mechanism that we'll need for the FilterAction API instead of adding a separate API knob?

A guarantee that a cluster is configured, active and/or healthy is not required for my use case. We need to iterate cluster picking mechanisms, and if a cluster is not found, an error response would be returned to the client. I've emphasized "mechanisms" as that is a key difference with the use case you are describing; we first want to attempt e.g., cluster_header based picking, and then weighted cluster picking. Therefore, I don't think the aggregate cluster capability would be a viable solution for this use case.

Okay, that makes sense. It is worth noting that if we did have the kind of extensible LB policy framework discussed in #5598, then you could probably do the equivalent of both cluster_header and WeightedClusters in the LB policy tree, in which case aggregate cluster would probably do what you want. But that's unfortunately not a near-term solution.

However, if we are going to implement the mechanism that we need for FilterAction, we could consider using that to replace the aggregate cluster functionality. But I guess we'd need to talk to the folks who are using that functionality to see if that works for them.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[AndresGuedez]

Right, as long as the filter is provided an API to configure the RouteAction to use, both your example with an ordered list of fallback clusters and my use case which requires weighted cluster picking as fallback could be satisfied.

Okay. Then maybe we should just implement the mechanism that we'll need for the FilterAction API instead of adding a separate API knob?

That's fine with me.

@mattklein123 @htuch WDYT?

Please see my earlier comment for an example of how I would use FilterAction to achieve the same behavior that I am proposing with this API.

Is this use case common enough that we should have an explicit API (i.e., something like this proposal) or is achieving the same goal via the generic API enough?

[htuch]

I see some competing tensions:

1. We want to have very common functionality in the router API and usable out-of-the-box.
2. We want to decompose the router (it's becoming a monolith) and push more functionality into filters.
3. We want to converge with gRPC and support FilterAction.
   With my security hat on I would vote to preference (2), with my API shepherd hat on I would vote to preference (1) and (3) depending on how common we expect usage of fallback to be.

[markdroth]

@htuch From an API shephard perspective, I'd actually argue that the right approach is to make everything extensible and to support some common plugins to those extension points. So I would actually consider (1) to be a non-goal, and I think FilterAction supports the other two goals nicely.

[htuch]

@markdroth I think making everything extensible where the capability is not lowest common denominator common functionality makes sense. The question is whether this falls into that category.

[mattklein123]

My 2c is this comes up pretty often and we should build this into the core vs. requiring an extension to do this.

One compromise I suppose would be to build the FilterAction support and have an out of box extension which covers this common use case somehow.

[markdroth]

One compromise I suppose would be to build the FilterAction support and have an out of box extension which covers this common use case somehow.

I'd be in favor of that approach.

[htuch]

SGTM

[AndresGuedez]

SG, thanks everyone. I'll follow up with a new PR based on FilterAction.

[stale]

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

[stale]

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!