rds: Add ConfigSource for CDS. #8200
Conversation
Signed-off-by: Mark D. Roth <roth@google.com>
|
|
||
| // If specified, indicates a server to contact to get the cluster data via CDS. | ||
| // By default, Envoy will know where to get CDS data from via its bootstrap file, but | ||
| // other clients (e.g., gRPC) may not have a default. |
There was a problem hiding this comment.
I guess the question I have here is why does gRPC not have a default CDS? I can see how this feature might be possibly useful in the general case, but it seems like gRPC should just be able to deal with this via its own bootstrap.
There was a problem hiding this comment.
I think this is one of those cases where a proxy like Envoy and a proxyless client like gRPC will have a slightly different world-view.
As a proxy, Envoy wants to get data for all clusters, since it does not know in advance which of them it will need to use. The set of data it's interested in from CDS is not altered by what it may get via LDS or RDS. Therefore, Envoy is expected to make a "wildcard" query for CDS (i.e., it does not populate the DiscoveryRequest.resource_names field), so that it gets back data for all clusters. As a result, it makes sense to configure CDS in Envoy's bootstrap file, the same way that LDS is handled. Effectively, there are two different configuration "roots" that Envoy loads automatically at startup: LDS and CDS.
In contrast, a proxyless client like gRPC is only ever interested in a single virtual host, which means that it does not care about all clusters; it wants to see only those clusters that are used by the particular virtual host that it's trying to contact. As a result, instead of making a wildcard CDS query at startup, gRPC will wait for the RDS response and then make a CDS query for just the clusters it found in the RDS response. In effect, there is only one config "root" for gRPC, which is LDS; everything else is "chained" out from that point (LDS -> RDS -> CDS -> EDS).
Given that, it seems inconsistent to have CDS configured via gRPC's bootstrap file. The linkage from LDS to RDS and from CDS to EDS already happens via a ConfigSource. My intent in this PR is to make this pattern consistent so that the whole chain works the same way.
In addition, it seems like this is something we'd want to do anyway to support federation. How else would we delegate a particular set of clusters to a separate set of management servers? It seems like this is consistent with the approach we're trying to take for UDPA, so adding this capability here seems like it moves us closer to the direction we eventually want to go in anyway.
There was a problem hiding this comment.
BTW, to be clear, I am not expecting this to break any of Envoy's existing functionality. Envoy can continue to specify the CDS server in its bootstrap file. These new ConfigSource fields in RDS would be optional, and if not set, the client can fall back to some reasonable default (Envoy could use the CDS server in its bootstrap file; gRPC would probably default to the same server we're already talking to for RDS).
As a side note, I think this is another case where the self-referential mechanism in #8201 would be useful.
There was a problem hiding this comment.
Yeah, this perspective is reasonable for gRPC, but the main concern that remains for me is that if we do add this as new behavior to Envoy, this is actually a lot of implementation complexity in ClusterManager; it needs to deal with multiple CDS configuration sources and to dynamically update these based on RDS. It's hard to see Envoy taking this as an implementation change within v2/v3 API lifetimes without some significant use case to justify this.
There was a problem hiding this comment.
My perspection is that you spreading the fetchable cluster everywhere among the configs.
It could make envoy hard to fetch,warm and garbage collect cluster.
Also how can another ClusterWeight refers the same cluster? Is ClusterWeight acts a namespace?
You'd better have an example what is the advantage of this cluster config, and why the centralized xds cluster doesn't work for you
| // other clients (e.g., gRPC) may not have a default. | ||
| // [#next-major-version: In the v3 API, we should replace the name and config_source | ||
| // fields with a single ClusterSource field.] | ||
| envoy.api.v2.core.ConfigSource config_source = 11; |
There was a problem hiding this comment.
The name field acts as reference. It's cds responsibility to fetch the name cluster. What you are proposing, which is combining name with config_source , indicating a fetch operation.
If so, which component is fetching the cluster message?
There was a problem hiding this comment.
See my reply to Harvey below. I understand that Envoy would not currently use this, but gRPC would, and I think that there may be federation cases where this will be useful for Envoy in the future.
There was a problem hiding this comment.
Also how can another ClusterWeight refers the same cluster? Is ClusterWeight acts a namespace?
My proposal is that if there are two different places in the config that specify the same cluster name and the same ConfigSource for fetching the cluster, the client will de-duplicate them -- i.e., it will only fetch the data once, and it will use the same data in both places. (This might be what you meant by "acts as a namespace".)
|
|
||
| // If specified, indicates a server to contact to get the cluster data via CDS. | ||
| // By default, Envoy will know where to get CDS data from via its bootstrap file, but | ||
| // other clients (e.g., gRPC) may not have a default. |
There was a problem hiding this comment.
I think this is one of those cases where a proxy like Envoy and a proxyless client like gRPC will have a slightly different world-view.
As a proxy, Envoy wants to get data for all clusters, since it does not know in advance which of them it will need to use. The set of data it's interested in from CDS is not altered by what it may get via LDS or RDS. Therefore, Envoy is expected to make a "wildcard" query for CDS (i.e., it does not populate the DiscoveryRequest.resource_names field), so that it gets back data for all clusters. As a result, it makes sense to configure CDS in Envoy's bootstrap file, the same way that LDS is handled. Effectively, there are two different configuration "roots" that Envoy loads automatically at startup: LDS and CDS.
In contrast, a proxyless client like gRPC is only ever interested in a single virtual host, which means that it does not care about all clusters; it wants to see only those clusters that are used by the particular virtual host that it's trying to contact. As a result, instead of making a wildcard CDS query at startup, gRPC will wait for the RDS response and then make a CDS query for just the clusters it found in the RDS response. In effect, there is only one config "root" for gRPC, which is LDS; everything else is "chained" out from that point (LDS -> RDS -> CDS -> EDS).
Given that, it seems inconsistent to have CDS configured via gRPC's bootstrap file. The linkage from LDS to RDS and from CDS to EDS already happens via a ConfigSource. My intent in this PR is to make this pattern consistent so that the whole chain works the same way.
In addition, it seems like this is something we'd want to do anyway to support federation. How else would we delegate a particular set of clusters to a separate set of management servers? It seems like this is consistent with the approach we're trying to take for UDPA, so adding this capability here seems like it moves us closer to the direction we eventually want to go in anyway.
| // other clients (e.g., gRPC) may not have a default. | ||
| // [#next-major-version: In the v3 API, we should replace the name and config_source | ||
| // fields with a single ClusterSource field.] | ||
| envoy.api.v2.core.ConfigSource config_source = 11; |
There was a problem hiding this comment.
See my reply to Harvey below. I understand that Envoy would not currently use this, but gRPC would, and I think that there may be federation cases where this will be useful for Envoy in the future.
|
I discussed this with @htuch and @mattklein123 yesterday, and we agreed to close this for now. I do think that we're eventually going to need something like this anyway as part of UDPA, but we don't strictly need to deal with this right now, so we'll defer that work until later. |
Description: Adds fields to RDS response for specifying what server to talk to for CDS.
Risk Level: Low
Testing: None
Docs Changes: Inline with API protos
Release Notes: N/A
This PR is something that seems like it would be useful, although I haven't actually discussed this with anyone yet, so I'm mainly floating this as a proposal at this point. Please let me know what you think.
cc @htuch @mattklein123 @vishalpowar