feat(grpc-xds): gRFC A74 model validated xDS resources and XdsConfig snapshot - #2775
feat(grpc-xds): gRFC A74 model validated xDS resources and XdsConfig snapshot#2775YutaoMa wants to merge 3 commits into
Conversation
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
Looks great overall, thanks! I've found a handful of things that should be fixed, but it should all be pretty small.
| /// The atomic xDS configuration snapshot for a channel. | ||
| #[derive(Debug, Clone)] | ||
| #[non_exhaustive] | ||
| pub(crate) struct XdsConfig { |
There was a problem hiding this comment.
I am assuming this would, itself, end up in an Arc, so that it can be shared between the name resolver and all the LB policies? If so do we need Arcs on the individual fields as well?
| "sum of weighted cluster weights exceeds {}", | ||
| u32::MAX | ||
| ))); | ||
| } |
There was a problem hiding this comment.
I think there should be a validation of the total_weight field here:
| } | |
| } | |
| if let Some(tw) = wc.total_weight_opt() { | |
| let expected_total = tw.value(); | |
| if expected_total == 0 || u64::from(expected_total) != total_weight { | |
| return Err(Error::Validation(format!( | |
| "sum of weighted cluster weights ({total_weight}) does not match explicit total_weight ({expected_total})" | |
| ))); | |
| } | |
| } | |
per route_components.proto:
// Specifies the total weight across all clusters. The sum of all cluster weights must equal this // value, which must be greater than 0. Defaults to 100. google.protobuf.UInt32Value total_weight = 3 [(validate.rules).uint32 = {gte: 1}];
and A28:
- Can be Weighted_clusters
- The sum of weights must add up to the total_weight.
| && !Arc::ptr_eq(inline, &route_config) | ||
| { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
Should we also validate that if route_source is RouteSource::Rds, that the rds name matches the route_config name?
| } | ||
| let any: Any = api_listener.api_listener().to_owned(); | ||
|
|
||
| let hcm = HttpConnectionManager::parse(any.value()).map_err(|e| { |
There was a problem hiding this comment.
We should validate any's type_url before parsing this to ensure the message type is what we expect.
| } | ||
|
|
||
| fn validate_header_matcher(hm: HeaderMatcherView<'_>) -> xds_client::Result<HeaderMatcher> { | ||
| let name = hm.name().to_str().unwrap_or_default().to_string(); |
There was a problem hiding this comment.
Should we validate !name.is_empty()?
| } | ||
|
|
||
| fn validate(message: Self::Message) -> xds_client::Result<Self> { | ||
| let name = message.name().to_str().unwrap_or_default().to_string(); |
There was a problem hiding this comment.
Should we validate !name.is_empty() here as well, matching cluster/endpoint validation?
| // DEGRADED and anything unrecognized fall back to Unknown below. | ||
| EnvoyHealthStatus::Unhealthy | EnvoyHealthStatus::Timeout => Self::Unhealthy, | ||
| EnvoyHealthStatus::Draining => Self::Draining, | ||
| _ => Self::Unknown, |
There was a problem hiding this comment.
This catch-all maps Degraded to Unknown, and then we will end up using the endpoint since we are supposed to use either HEALTHY or UNKNOWN endpoints per Envoy: https://github.com/envoyproxy/envoy/blob/8f77c4a301bd96d2c7231915e6c3d1f85ea88ae1/api/envoy/config/core/v3/health_check.proto#L36
| // of 1,000,000. runtime_key is ignored (gRPC has no runtime config). | ||
| let match_fraction = rm.runtime_fraction_opt().and_then(|rf| { | ||
| if !rf.has_default_value() { | ||
| return None; |
There was a problem hiding this comment.
If there is no default value set, this should probably error. (The field is required per the protos: https://github.com/envoyproxy/envoy/blob/b67c14052c49890a7e3afe614d50979c346c024b/api/envoy/api/v2/core/base.proto#L368-L369, so this probably won't ever happen in practice)
| )); | ||
| } | ||
| let any = custom.typed_config(); | ||
| let cluster_config = AggregateClusterConfig::parse(any.value()).map_err(|e| { |
There was a problem hiding this comment.
This should also validate any.type_url
2 participants
Motivation
Ref: #2754
Per gRFC A74, to avoid config tear across xDS layers, all watches (LDS, RDS, CDS, EDS) are moved into a single resolver behind an
XdsDependencyManagerthat emits a single completeXdsConfig.Solution
This PR implements the
XdsConfigmodeling, along with its xDS resource types(the validated types corresponding to the Envoy proto types) and validation logic. This PR is a pre-req to theXdsDependencyManager.The shape of
XdsConfigclosely mirrors gRFC A74's C++ sketch, as well as other gRPC implementations such as grpc-go. A few design choices were made for Rust ergonomics, such as using an index to store selected virtual host rather than a reference to avoid self-referential borrow.