diff --git a/tonic-xds/src/client/channel.rs b/tonic-xds/src/client/channel.rs index 7b4b19c73..786642608 100644 --- a/tonic-xds/src/client/channel.rs +++ b/tonic-xds/src/client/channel.rs @@ -46,7 +46,7 @@ use xds_client::{ ClientConfig, MetricsRecorder, Node, ProstCodec, TokioRuntime, TonicTransportBuilder, XdsClient, }; -use crate::client::retry::{GrpcRetryPolicy, RetryLayer}; +use crate::client::retry::{GrpcRetrySharedConfig, RetryLayer}; /// Configuration for building [`XdsChannel`] / [`XdsChannelGrpc`]. #[derive(Clone, Debug)] @@ -367,6 +367,12 @@ impl XdsChannelBuilder { resource_manager: XdsResourceManager, ) -> XdsChannelGrpc { let router: Arc = Arc::new(XdsRouter::new(&cache)); + + // Fallback retry config for requests that carry no per-route config + // (non-xDS callers, or a route with no retry policy). Per-route configs + // come from RDS via the request's `RouteDecision`; see `RetryLayer`. + let retry_layer = RetryLayer::new(Arc::new(GrpcRetrySharedConfig::default())); + #[cfg(feature = "_tls-any")] let discovery: Arc< dyn ClusterDiscovery>, @@ -378,7 +384,6 @@ impl XdsChannelBuilder { let discovery: Arc< dyn ClusterDiscovery>, > = Arc::new(XdsClusterDiscovery::new(cache, GrpcMakeConnector::new())); - let retry_policy = GrpcRetryPolicy::default(); let resources = Arc::new(XdsChannelResources { _resource_manager: resource_manager, @@ -386,7 +391,6 @@ impl XdsChannelBuilder { }); let routing_layer = XdsRoutingLayer::new(router, self.pre_route.clone(), self.authority()); - let retry_layer = RetryLayer::new(retry_policy); let cluster_registry = Arc::new(ClusterClientRegistryGrpc::new()); let lb_service = XdsLbService::new(cluster_registry, discovery); let inner = ServiceBuilder::new() @@ -412,17 +416,17 @@ impl XdsChannelBuilder { } /// Builds an `XdsChannelGrpc` from the given router, cluster discovery, retry - /// policy, and optional pre-route interceptor. + /// config, and optional pre-route interceptor. #[cfg(test)] pub(crate) fn build_grpc_channel_from_parts( &self, router: Arc, discovery: Arc>>, - retry_policy: GrpcRetryPolicy, + retry: Arc, interceptor: Option>, ) -> XdsChannelGrpc { let routing_layer = XdsRoutingLayer::new(router, interceptor, self.authority()); - let retry_layer = RetryLayer::new(retry_policy); + let retry_layer = RetryLayer::new(retry); let cluster_registry = Arc::new(ClusterClientRegistryGrpc::new()); let lb_service = XdsLbService::new(cluster_registry, discovery); let inner = ServiceBuilder::new() @@ -458,7 +462,7 @@ mod tests { XdsChannelConfig::new(XdsUri::parse("xds:///test-service").unwrap()) } use crate::client::lb::{BoxDiscover, ClusterDiscovery}; - use crate::client::retry::GrpcRetryPolicy; + use crate::client::retry::GrpcRetrySharedConfig; use crate::client::route::RouteDecision; use crate::client::route::RouteInput; use crate::client::route::Router; @@ -522,11 +526,12 @@ mod tests { fn route( &self, _input: &RouteInput<'_>, - _config: &crate::xds::resource::route_config::RouteConfigResource, + _config: &crate::client::route::RoutingSnapshot, ) -> Result { Ok(RouteDecision { cluster: "test-cluster".to_string(), request_hash: None, + retry_config: None, }) } } @@ -609,7 +614,7 @@ mod tests { let xds_channel = xds_channel_builder.build_grpc_channel_from_parts( xds_manager.clone(), xds_manager.clone(), - GrpcRetryPolicy::default(), + Arc::new(GrpcRetrySharedConfig::default()), None, ); @@ -667,7 +672,7 @@ mod tests { #[tokio::test] async fn test_retry_once_on_unavailable() { - use crate::client::retry::{GrpcRetryClassifier, GrpcRetryPolicy, RetryConfig}; + use crate::client::retry::{GrpcRetryClassifier, RetryConfig}; use crate::testutil::grpc::spawn_fail_first_n_server; // Server fails the first request with UNAVAILABLE, succeeds on retry. @@ -678,17 +683,15 @@ mod tests { let servers = vec![server]; let xds_manager = Arc::new(MockXdsManager::from_test_servers(&servers)); - let retry_policy = GrpcRetryPolicy::new( + let retry = Arc::new(GrpcRetrySharedConfig::new( RetryConfig::new().num_retries(1), - GrpcRetryClassifier { - retry_on: vec![tonic::Code::Unavailable], - }, - ); + GrpcRetryClassifier::new(vec![tonic::Code::Unavailable]), + )); let xds_channel = XdsChannelBuilder::new(test_config()).build_grpc_channel_from_parts( xds_manager.clone(), xds_manager.clone(), - retry_policy, + retry, None, ); @@ -734,6 +737,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster(cluster_name.to_string()), + retry_config: None, }], }], metadata: Default::default(), @@ -790,7 +794,12 @@ mod tests { > = Arc::new(XdsClusterDiscovery::new(cache, GrpcMakeConnector::new())); let builder = XdsChannelBuilder::new(test_config()); - builder.build_grpc_channel_from_parts(router, discovery, GrpcRetryPolicy::default(), None) + builder.build_grpc_channel_from_parts( + router, + discovery, + Arc::new(GrpcRetrySharedConfig::default()), + None, + ) } /// Tests the full xDS stack (XdsRouter + XdsClusterDiscovery) with a diff --git a/tonic-xds/src/client/circuit_breaking.rs b/tonic-xds/src/client/circuit_breaking.rs index 1805acabe..a5380270e 100644 --- a/tonic-xds/src/client/circuit_breaking.rs +++ b/tonic-xds/src/client/circuit_breaking.rs @@ -731,6 +731,7 @@ mod tests { request.extensions_mut().insert(RouteDecision { cluster: CLUSTER.to_string(), request_hash: None, + retry_config: None, }); request } @@ -984,9 +985,7 @@ mod tests { RetryBackoffConfig::new(Duration::from_millis(1)) .max_interval(Duration::from_millis(1)), ), - GrpcRetryClassifier { - retry_on: vec![Code::Unavailable], - }, + GrpcRetryClassifier::new(vec![Code::Unavailable]), ); let calls = Arc::new(AtomicU32::new(0)); let call_counter = calls.clone(); @@ -998,7 +997,7 @@ mod tests { }, ); let mut service = tower::ServiceBuilder::new() - .layer(RetryLayer::new(policy)) + .layer(RetryLayer::new(policy.into_shared())) .layer(CircuitBreakingLayer::new(breakers.clone())) .service(service); diff --git a/tonic-xds/src/client/loadbalance/pickers/ring_hash.rs b/tonic-xds/src/client/loadbalance/pickers/ring_hash.rs index 68dc881d9..77f65ce13 100644 --- a/tonic-xds/src/client/loadbalance/pickers/ring_hash.rs +++ b/tonic-xds/src/client/loadbalance/pickers/ring_hash.rs @@ -223,6 +223,7 @@ mod tests { r.extensions_mut().insert(RouteDecision { cluster: "c".to_string(), request_hash: hash, + retry_config: None, }); r } diff --git a/tonic-xds/src/client/retry.rs b/tonic-xds/src/client/retry.rs index 2d0060deb..fc153fd9f 100644 --- a/tonic-xds/src/client/retry.rs +++ b/tonic-xds/src/client/retry.rs @@ -36,7 +36,6 @@ use std::sync::Arc; use std::task::{Context, Poll}; use std::time::Duration; -use arc_swap::ArcSwap; use backoff::ExponentialBackoffBuilder; use backoff::backoff::Backoff; use http::{Request, Response}; @@ -47,6 +46,8 @@ use tower::retry::Retry; use tower::{Layer, Service}; use crate::client::circuit_breaking::is_local_circuit_breaker_drop; +use crate::client::route::RouteDecision; +use crate::xds::resource::route_config::RouteRetryConfig; /// Check if an error's source chain contains a retryable connection-level error. /// @@ -128,7 +129,9 @@ impl RetryBackoffConfig { pub(crate) fn new(base_interval: Duration) -> Self { let base_interval = base_interval.max(MIN_BACKOFF); Self { - max_interval: base_interval * 10, + // `checked_mul` guards the (already range-checked) `base_interval` + // against overflow; the fallback keeps `max_interval >= base`. + max_interval: base_interval.checked_mul(10).unwrap_or(base_interval), base_interval, backoff_multiplier: 2.0, } @@ -205,10 +208,25 @@ impl Default for RetryConfig { /// Default gRPC [`RetryClassifier`]: retries on a retryable connection error or a /// retryable gRPC status code, and stamps the `grpc-previous-rpc-attempts` header /// on each retry per the gRPC spec. -#[derive(Debug, Clone, Default)] +#[derive(Debug, Clone)] pub(crate) struct GrpcRetryClassifier { /// gRPC status codes that should be retried. - pub(crate) retry_on: Vec, + retry_on: Arc<[tonic::Code]>, +} + +impl GrpcRetryClassifier { + /// Create a classifier that retries the given gRPC status codes. + pub(crate) fn new(retry_on: Vec) -> Self { + Self { + retry_on: retry_on.into(), + } + } +} + +impl Default for GrpcRetryClassifier { + fn default() -> Self { + Self::new(Vec::new()) + } } impl RetryClassifier for GrpcRetryClassifier { @@ -245,24 +263,44 @@ fn make_backoff(config: &RetryBackoffConfig) -> backoff::ExponentialBackoff { .build() } -/// Retry *decision* state — attempt cap, backoff, and body cloning — shared -/// across transports. Transport-specific decisions live in the classifier `C` -/// (see [`RetryClassifier`]). -/// -/// Wraps a [`RetryConfig`] behind an [`ArcSwap`] so that configuration can be -/// atomically updated (e.g. from xDS) without blocking in-flight requests. +/// Immutable, shared retry configuration: the transport-agnostic knobs (attempt +/// cap, backoff) plus the transport-specific classifier `C`. Built once when a +/// `RouteConfiguration` is validated (see +/// [`GrpcRetrySharedConfig::from_route_retry`]) and shared across matching +/// requests via an [`Arc`]. /// -/// Implements [`tower::retry::Policy`]. Tower's `Retry` service clones the policy -/// for each request, so `backoff` and `attempts` track per-request retry state -/// while the shared config is read from `ArcSwap` on each retry decision. The -/// retry *state machine* stays entirely in tower's `Retry`/`ResponseFuture`; -/// this type only implements the `Policy` trait, so no state machine is -/// reimplemented here. -#[derive(Clone, Debug)] -pub(crate) struct RetryPolicy { - config: Arc>, +/// Kept separate from the per-request retry state ([`RetryPolicy`]) so that +/// instantiating a policy is an `Arc` clone plus a zero-field init. +#[derive(Debug)] +pub(crate) struct RetrySharedConfig { + /// Attempt cap and backoff schedule. + config: RetryConfig, /// Decides retryability and per-retry request mutation for the transport. classifier: C, +} + +impl RetrySharedConfig { + /// Create a shared retry config from a [`RetryConfig`] and a classifier. + pub(crate) fn new(config: RetryConfig, classifier: C) -> Self { + Self { config, classifier } + } +} + +impl Default for RetrySharedConfig { + fn default() -> Self { + Self::new(RetryConfig::default(), C::default()) + } +} + +/// Per-request retry *state*: a pointer to the shared, immutable +/// [`RetrySharedConfig`] plus the mutable state for one request (backoff cursor +/// and attempt counter). Implements [`tower::retry::Policy`]; tower's `Retry` +/// clones it per request, so the state is per-request while the config stays +/// shared behind the `Arc`. +#[derive(Clone, Debug)] +pub(crate) struct RetryPolicy { + /// Immutable config shared across all requests on this route. + shared: Arc>, /// Backoff state for the current request, created from config on first retry. backoff: Option, /// Number of retry attempts made so far for the current request. @@ -270,28 +308,31 @@ pub(crate) struct RetryPolicy { } impl RetryPolicy { - /// Create a new retry policy with the given configuration and classifier. + /// Create a policy from a config and classifier, allocating the shared `Arc`. + /// Prefer [`from_shared`](Self::from_shared) on the hot path. pub(crate) fn new(config: RetryConfig, classifier: C) -> Self { + Self::from_shared(Arc::new(RetrySharedConfig::new(config, classifier))) + } + + /// Instantiate per-request state from a shared config: a pointer clone plus + /// a zero-field init. + pub(crate) fn from_shared(shared: Arc>) -> Self { Self { - config: Arc::new(ArcSwap::from(Arc::new(config))), - classifier, + shared, backoff: None, attempts: 0, } } - /// Atomically swap the configuration with a new one. - pub(crate) fn update_config(&self, config: RetryConfig) { - self.config.store(Arc::new(config)); - } - - /// Load the current configuration. - pub(crate) fn load_config(&self) -> Arc { - self.config.load_full() + /// Consume the policy and return its shared config. + pub(crate) fn into_shared(self) -> Arc> { + self.shared } - /// Get or create the backoff, and advance it to the next delay. - fn backoff_next(&mut self, backoff_config: &RetryBackoffConfig) -> Duration { + /// Get or lazily create the backoff and advance it to the next delay. Only + /// reached when a request is actually being retried. + fn backoff_next(&mut self) -> Duration { + let backoff_config = &self.shared.config.retry_backoff; let backoff = self .backoff .get_or_insert_with(|| make_backoff(backoff_config)); @@ -303,10 +344,57 @@ impl RetryPolicy { impl Default for RetryPolicy { fn default() -> Self { - Self::new(RetryConfig::default(), C::default()) + Self::from_shared(Arc::new(RetrySharedConfig::default())) + } +} + +impl RetrySharedConfig { + /// Build a shared gRPC config from a route's [`RouteRetryConfig`], mapping + /// `retry_on` to [`tonic::Code`]s; unset Envoy fields use [`RetryConfig`] + /// defaults. Returns `None` when no condition maps to a gRPC code, so the + /// route falls back to the layer default instead of masking connection + /// retries (gRFC A44). + pub(crate) fn from_route_retry(retry: &RouteRetryConfig) -> Option { + let retry_on = grpc_retry_on_codes(&retry.retry_on); + if retry_on.is_empty() { + return None; + } + let mut config = RetryConfig::new(); + if let Some(num_retries) = retry.num_retries { + config = config.num_retries(num_retries); + } + if let Some(base_interval) = retry.base_interval { + let mut backoff = RetryBackoffConfig::new(base_interval); + if let Some(max_interval) = retry.max_interval { + backoff = backoff.max_interval(max_interval); + } + config = config.retry_backoff(backoff); + } + Some(Self::new(config, GrpcRetryClassifier::new(retry_on))) } } +/// Map Envoy `retry_on` conditions (comma-separated) to gRPC [`tonic::Code`]s. +/// +/// Only the gRPC-status conditions from gRFC A44 are recognized; non-gRPC tokens +/// (e.g. `5xx`, `gateway-error`, `reset`, `connect-failure`) are ignored because +/// connection-level retries are handled separately by +/// [`is_retryable_connection_error`]. +pub(crate) fn grpc_retry_on_codes(retry_on: &str) -> Vec { + use tonic::Code; + retry_on + .split(',') + .filter_map(|token| match token.trim() { + "cancelled" => Some(Code::Cancelled), + "deadline-exceeded" => Some(Code::DeadlineExceeded), + "internal" => Some(Code::Internal), + "resource-exhausted" => Some(Code::ResourceExhausted), + "unavailable" => Some(Code::Unavailable), + _ => None, + }) + .collect() +} + impl Policy, Response, tower::BoxError> for RetryPolicy where C: RetryClassifier, @@ -319,22 +407,20 @@ where req: &mut Request, result: &mut Result, tower::BoxError>, ) -> Option { - let config = self.load_config(); - - if self.attempts >= config.num_retries { + if self.attempts >= self.shared.config.num_retries { return None; } - if !self.classifier.is_retryable(result) { + if !self.shared.classifier.is_retryable(result) { return None; } - let delay = self.backoff_next(&config.retry_backoff); + let delay = self.backoff_next(); self.attempts += 1; // Let the classifier stamp any per-retry request state (e.g. gRPC's // grpc-previous-rpc-attempts header). - self.classifier.prepare_retry(req, self.attempts); + self.shared.classifier.prepare_retry(req, self.attempts); Some(tokio::time::sleep(delay)) } @@ -347,50 +433,55 @@ where /// Non-breaking alias: existing gRPC callers keep the same name and behavior. pub(crate) type GrpcRetryPolicy = RetryPolicy; -/// Tower [`Layer`] that wraps a service with retry support. -/// -/// Converts the request body into a [`SharedBody`] (cloneable) and constructs -/// a fresh [`tower::retry::Retry`] service per request so that each request -/// gets its own retry state. +/// Shared, immutable gRPC retry config (attempt cap, backoff, retryable +/// [`tonic::Code`] set); the [`GrpcRetryClassifier`] specialization of +/// [`RetrySharedConfig`]. +pub(crate) type GrpcRetrySharedConfig = RetrySharedConfig; + +/// Tower [`Layer`] that wraps a gRPC service with retry support. /// -/// This layer is generic over the retry policy — it is not tied to gRPC. -/// The gRPC-specific behavior lives in the [`Policy`] implementation -/// (e.g. [`GrpcRetryPolicy`]). +/// Builds a fresh [`tower::retry::Retry`] per request, selecting the config from +/// the matched route's [`RouteDecision`] (stamped by the routing layer just +/// outside). Requests with no [`RouteDecision`] (non-xDS callers) or whose route +/// carries no retry policy use `fallback`. #[derive(Clone)] -pub(crate) struct RetryLayer

{ - policy: P, +pub(crate) struct RetryLayer { + /// Config used when a request carries no per-route retry config. + fallback: Arc, } -impl

RetryLayer

{ - /// Create a new retry layer with the given policy. - pub(crate) fn new(policy: P) -> Self { - Self { policy } +impl RetryLayer { + /// Create a layer with the given `fallback` config. + pub(crate) fn new(fallback: Arc) -> Self { + Self { fallback } } } -impl Layer for RetryLayer

{ - type Service = RetryService; +impl Layer for RetryLayer { + type Service = RetryService; fn layer(&self, service: S) -> Self::Service { RetryService { inner: service, - policy: self.policy.clone(), + fallback: Arc::clone(&self.fallback), } } } /// Service that converts request bodies to [`SharedBody`] and retries via -/// [`tower::retry::Retry`] with the given policy. +/// [`tower::retry::Retry`], selecting the per-request config from the matched +/// route's [`RouteDecision`] (see [`RetryLayer`]). #[derive(Clone)] -pub(crate) struct RetryService { +pub(crate) struct RetryService { inner: S, - policy: P, + /// Config used when a request carries no per-route retry config. + fallback: Arc, } -impl Service> for RetryService +impl Service> for RetryService where - P: Policy>, Response, S::Error> + Clone + Send + 'static, - P::Future: Send, + GrpcRetryPolicy: Policy>, Response, S::Error>, + >, Response, S::Error>>::Future: Send, S: Service>, Response = Response> + Clone + Send + 'static, S::Error: Debug + Send + 'static, S::Response: Send + 'static, @@ -411,7 +502,14 @@ where } fn call(&mut self, request: Request) -> Self::Future { - let mut retry_svc = Retry::new(self.policy.clone(), self.inner.clone()); + // Config the routing layer stamped for this request's route, or the fallback. + let shared = request + .extensions() + .get::() + .and_then(|decision| decision.retry_config.clone()) + .unwrap_or_else(|| Arc::clone(&self.fallback)); + let policy = RetryPolicy::from_shared(shared); + let mut retry_svc = Retry::new(policy, self.inner.clone()); let shared_request = request.map(|b| b.into_shared()); Box::pin(retry_svc.call(shared_request)) } @@ -527,9 +625,7 @@ mod tests { #[test] fn test_is_retryable_grpc_status_via_result() { - let classifier = GrpcRetryClassifier { - retry_on: vec![tonic::Code::Unavailable], - }; + let classifier = GrpcRetryClassifier::new(vec![tonic::Code::Unavailable]); let response = http::Response::builder() .header("grpc-status", "14") // UNAVAILABLE .body(()) @@ -540,9 +636,7 @@ mod tests { #[test] fn test_is_not_retryable_ok_response() { - let classifier = GrpcRetryClassifier { - retry_on: vec![tonic::Code::Unavailable], - }; + let classifier = GrpcRetryClassifier::new(vec![tonic::Code::Unavailable]); let response = http::Response::builder() .header("grpc-status", "0") // OK .body(()) @@ -553,9 +647,7 @@ mod tests { #[test] fn test_is_not_retryable_no_grpc_status_header() { - let classifier = GrpcRetryClassifier { - retry_on: vec![tonic::Code::Unavailable], - }; + let classifier = GrpcRetryClassifier::new(vec![tonic::Code::Unavailable]); let response = http::Response::builder().body(()).unwrap(); let result: Result, tower::BoxError> = Ok(response); assert!(!classifier.is_retryable(&result)); @@ -635,12 +727,11 @@ mod tests { #[test] fn test_grpc_classifier_retry_on() { - let classifier = GrpcRetryClassifier { - retry_on: vec![tonic::Code::Unavailable, tonic::Code::Cancelled], - }; + let classifier = + GrpcRetryClassifier::new(vec![tonic::Code::Unavailable, tonic::Code::Cancelled]); assert_eq!( - classifier.retry_on, - vec![tonic::Code::Unavailable, tonic::Code::Cancelled] + classifier.retry_on.as_ref(), + [tonic::Code::Unavailable, tonic::Code::Cancelled] ); } @@ -653,29 +744,84 @@ mod tests { assert_eq!(config.retry_backoff, backoff); } - // --- RetryPolicy (ArcSwap wrapper) tests --- + // --- from_route_retry tests --- #[test] - fn test_policy_load_config() { - let policy = GrpcRetryPolicy::new( - RetryConfig::new().num_retries(1), - GrpcRetryClassifier { - retry_on: vec![tonic::Code::Unavailable], - }, + fn test_from_route_retry_maps_fields() { + let retry = RouteRetryConfig { + retry_on: "unavailable".into(), + num_retries: Some(3), + base_interval: Some(Duration::from_millis(100)), + max_interval: Some(Duration::from_millis(1000)), + }; + let shared = GrpcRetrySharedConfig::from_route_retry(&retry).expect("codes present"); + assert_eq!(shared.config.num_retries, 3); + assert_eq!( + shared.config.retry_backoff.base_interval, + Duration::from_millis(100) + ); + assert_eq!( + shared.config.retry_backoff.max_interval, + Duration::from_millis(1000) + ); + assert_eq!( + shared.classifier.retry_on.as_ref(), + [tonic::Code::Unavailable] ); - let loaded = policy.load_config(); - assert_eq!(loaded.num_retries, 1); } #[test] - fn test_policy_update_config() { - let policy = GrpcRetryPolicy::default(); - assert_eq!(policy.load_config().num_retries, 1); + fn test_from_route_retry_unset_fields_use_defaults() { + let retry = RouteRetryConfig { + retry_on: "cancelled".into(), + num_retries: None, + base_interval: None, + max_interval: None, + }; + let shared = GrpcRetrySharedConfig::from_route_retry(&retry).expect("codes present"); + assert_eq!(shared.config.num_retries, 1); + assert_eq!(shared.config.retry_backoff, RetryBackoffConfig::default()); + assert_eq!( + shared.classifier.retry_on.as_ref(), + [tonic::Code::Cancelled] + ); + } - policy.update_config(RetryConfig::new().num_retries(3)); + #[test] + fn test_from_route_retry_empty_codes_yields_none() { + // `retry_on` with only non-gRPC tokens maps to no gRPC codes, so no + // policy is produced and connection retries are not masked (gRFC A44). + let retry = RouteRetryConfig { + retry_on: "5xx,reset".into(), + num_retries: Some(3), + base_interval: None, + max_interval: None, + }; + assert!(GrpcRetrySharedConfig::from_route_retry(&retry).is_none()); + } + + // --- from_shared tests --- + + #[test] + fn from_shared_instantiates_zeroed_state_sharing_config() { + let shared = Arc::new( + GrpcRetrySharedConfig::from_route_retry(&RouteRetryConfig { + retry_on: "unavailable".into(), + num_retries: Some(2), + base_interval: None, + max_interval: None, + }) + .expect("codes present"), + ); + let policy = RetryPolicy::from_shared(Arc::clone(&shared)); - let loaded = policy.load_config(); - assert_eq!(loaded.num_retries, 3); + assert_eq!(policy.attempts, 0); + assert!(policy.backoff.is_none()); + assert!(Arc::ptr_eq(&policy.shared, &shared)); + assert_eq!(policy.shared.config.num_retries, 2); + + let policy2 = RetryPolicy::from_shared(Arc::clone(&shared)); + assert!(Arc::ptr_eq(&policy.shared, &policy2.shared)); } /// Verify that two concurrent requests using the same policy get independent @@ -685,9 +831,7 @@ mod tests { async fn test_retry_state_is_per_request() { let policy = GrpcRetryPolicy::new( RetryConfig::new().num_retries(2), - GrpcRetryClassifier { - retry_on: vec![tonic::Code::Unavailable], - }, + GrpcRetryClassifier::new(vec![tonic::Code::Unavailable]), ); // Simulate two independent request sessions by cloning the policy diff --git a/tonic-xds/src/client/route.rs b/tonic-xds/src/client/route.rs index 0e8ff99e9..27600d7eb 100644 --- a/tonic-xds/src/client/route.rs +++ b/tonic-xds/src/client/route.rs @@ -22,10 +22,15 @@ * */ +use crate::client::retry::GrpcRetrySharedConfig; use crate::common::async_util::BoxFuture; -use crate::xds::resource::route_config::{RouteConfigMetadata, RouteConfigResource}; +use crate::xds::resource::route_config::{ + RouteConfigMetadata, RouteConfigResource, RouteRetryConfig, +}; use crate::xds::routing::RoutingError; use http::Request; +use std::collections::HashMap; +use std::ops::Deref; use std::sync::Arc; use std::task::{Context, Poll}; use tower::{BoxError, Layer, Service}; @@ -50,6 +55,11 @@ pub(crate) struct RouteDecision { // Populated by the routing layer; consumed by the ring-hash picker (later PR). #[allow(dead_code)] pub request_hash: Option, + /// The matched route's compiled retry config (RDS `RouteAction.retry_policy`), + /// or `None` when the route sets no retry. Resolved from the [`RoutingSnapshot`] + /// this decision was made on, so the retry layer applies the config for exactly + /// the route the request took. + pub retry_config: Option>, } /// A hook that runs before xDS route selection. @@ -64,6 +74,57 @@ pub trait PreRouteInterceptor: Send + Sync + 'static { fn on_request(&self, headers: &mut http::HeaderMap, metadata: &RouteConfigMetadata); } +/// The validated [`RouteConfigResource`] bundled with the gRPC retry configs +/// compiled from it, one per RDS update. +/// +/// Bundling both behind one `Arc` lets routing resolve a route and its retry +/// config from a single consistent RDS version, and keeps the request path to a +/// map lookup plus a pointer clone. Compiling here, rather than in the xDS +/// resource layer, keeps the resource types free of gRPC types. +#[derive(Debug, Default)] +pub(crate) struct RoutingSnapshot { + resource: Arc, + /// Compiled retry config, keyed by the identity (`Arc` address) of the + /// route's [`RouteRetryConfig`]. Routes that inherit a vhost policy share one + /// entry; a route whose `retry_on` maps to no gRPC code has none. + retry: HashMap>, +} + +impl RoutingSnapshot { + /// Compiles each distinct route retry config once. + pub(crate) fn new(resource: Arc) -> Self { + let mut retry: HashMap> = HashMap::new(); + for vhost in &resource.virtual_hosts { + for route in &vhost.routes { + if let Some(config) = &route.retry_config { + let key = Arc::as_ptr(config) as usize; + if let std::collections::hash_map::Entry::Vacant(entry) = retry.entry(key) + && let Some(shared) = GrpcRetrySharedConfig::from_route_retry(config) + { + entry.insert(Arc::new(shared)); + } + } + } + } + Self { resource, retry } + } + + /// The compiled gRPC retry config for a route's [`RouteRetryConfig`], if any. + pub(crate) fn retry_for( + &self, + config: Option<&Arc>, + ) -> Option> { + config.and_then(|config| self.retry.get(&(Arc::as_ptr(config) as usize)).cloned()) + } +} + +impl Deref for RoutingSnapshot { + type Target = RouteConfigResource; + fn deref(&self) -> &Self::Target { + &self.resource + } +} + /// A route config obtained in one step, to serve a single request with. /// /// Two cases so the common one -- a config is already in effect -- costs @@ -71,15 +132,15 @@ pub trait PreRouteInterceptor: Send + Sync + 'static { /// wait for the first config. pub(crate) enum AcquiredConfig { /// A config is already in effect. - Ready(Arc), + Ready(Arc), /// None has arrived yet; await this for the first one, bounded by an /// implementation-defined timeout. - Pending(BoxFuture, RoutingError>>), + Pending(BoxFuture, RoutingError>>), } impl AcquiredConfig { /// Resolves to the config, awaiting only when one is not already available. - pub(crate) async fn get(self) -> Result, RoutingError> { + pub(crate) async fn get(self) -> Result, RoutingError> { match self { Self::Ready(config) => Ok(config), Self::Pending(wait) => wait.await, @@ -100,7 +161,7 @@ pub(crate) trait Router: Send + Sync + 'static { fn route( &self, input: &RouteInput<'_>, - config: &RouteConfigResource, + config: &RoutingSnapshot, ) -> Result; } @@ -215,18 +276,19 @@ mod tests { impl Router for CaptureAuthorityRouter { fn acquire(&self) -> AcquiredConfig { - AcquiredConfig::Ready(Arc::new(RouteConfigResource::default())) + AcquiredConfig::Ready(Arc::new(RoutingSnapshot::default())) } fn route( &self, input: &RouteInput<'_>, - _config: &RouteConfigResource, + _config: &RoutingSnapshot, ) -> Result { *self.captured.lock().unwrap() = Some(input.authority.to_string()); Ok(RouteDecision { cluster: "test-cluster".to_string(), request_hash: None, + retry_config: None, }) } } @@ -301,6 +363,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("c".into()), + retry_config: None, }], }], metadata: RouteConfigMetadata::from_encoded( @@ -322,7 +385,7 @@ mod tests { #[tokio::test] async fn releases_the_route_config_before_calling_the_inner_service() { struct SharedConfigRouter { - config: Arc, + config: Arc, } impl Router for SharedConfigRouter { @@ -333,16 +396,17 @@ mod tests { fn route( &self, _input: &RouteInput<'_>, - _config: &RouteConfigResource, + _config: &RoutingSnapshot, ) -> Result { Ok(RouteDecision { cluster: "c".to_string(), request_hash: None, + retry_config: None, }) } } - let config = Arc::new(RouteConfigResource::default()); + let config = Arc::new(RoutingSnapshot::default()); let router: Arc = Arc::new(SharedConfigRouter { config: config.clone(), }); @@ -442,6 +506,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster(cluster.into()), + retry_config: None, } } diff --git a/tonic-xds/src/xds/cache.rs b/tonic-xds/src/xds/cache.rs index 12ae472d5..ed99b3086 100644 --- a/tonic-xds/src/xds/cache.rs +++ b/tonic-xds/src/xds/cache.rs @@ -208,6 +208,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("cluster-1".to_string()), + retry_config: None, }], }], metadata: Default::default(), diff --git a/tonic-xds/src/xds/resource/route_config.rs b/tonic-xds/src/xds/resource/route_config.rs index 68a09b53e..ca2cd08c0 100644 --- a/tonic-xds/src/xds/resource/route_config.rs +++ b/tonic-xds/src/xds/resource/route_config.rs @@ -25,11 +25,13 @@ //! Validated RouteConfiguration resource (RDS). use std::collections::{HashMap, HashSet}; +use std::sync::Arc; +use std::time::Duration; use bytes::Bytes; use envoy_types::pb::envoy::config::core::v3::Metadata; use envoy_types::pb::envoy::config::route::v3::{ - RouteConfiguration, RouteMatch, route, route_action, route_match, + RetryPolicy, RouteConfiguration, RouteMatch, route, route_action, route_match, }; use prost::Message; use regex::Regex; @@ -149,6 +151,96 @@ pub(crate) struct RouteConfigResource { pub metadata: RouteConfigMetadata, } +/// Validated Envoy retry settings (gRFC A44) parsed from a `RouteAction` or +/// `VirtualHost` `retry_policy`. A transport-neutral resource-layer type. +#[derive(Debug, Clone)] +pub(crate) struct RouteRetryConfig { + /// Envoy `retry_on` conditions, comma-separated (e.g. `"unavailable"`). + pub retry_on: String, + /// `num_retries` if set; `None` means the caller applies its own default. + pub num_retries: Option, + /// `retry_back_off.base_interval` if set. + pub base_interval: Option, + /// `retry_back_off.max_interval` if set. + pub max_interval: Option, +} + +impl RouteRetryConfig { + /// Parse and validate an Envoy `RetryPolicy` (gRFC A44). Returns + /// `Err(Validation)` — so the xDS client NACKs — when `num_retries < 1`, or + /// when `retry_back_off` is set with a `base_interval` or `max_interval` + /// that is not greater than zero. + fn from_proto(rp: &RetryPolicy) -> xds_client::Result { + let num_retries = match rp.num_retries.as_ref().map(|v| v.value) { + Some(0) => { + return Err(Error::Validation( + "retry_policy.num_retries must be >= 1".into(), + )); + } + other => other, + }; + + let (base_interval, max_interval) = match rp.retry_back_off.as_ref() { + Some(backoff) => { + let base_interval = backoff + .base_interval + .as_ref() + .and_then(proto_duration) + .filter(|d| !d.is_zero()) + .ok_or_else(|| { + Error::Validation( + "retry_policy.retry_back_off.base_interval must be greater than 0" + .into(), + ) + })?; + let max_interval = backoff + .max_interval + .as_ref() + .map(|m| { + proto_duration(m).filter(|d| !d.is_zero()).ok_or_else(|| { + Error::Validation( + "retry_policy.retry_back_off.max_interval must be greater than 0" + .into(), + ) + }) + }) + .transpose()?; + (Some(base_interval), max_interval) + } + None => (None, None), + }; + + Ok(Self { + retry_on: rp.retry_on.clone(), + num_retries, + base_interval, + max_interval, + }) + } +} + +/// Maximum `seconds` for a well-formed `google.protobuf.Duration`. +const MAX_PROTO_DURATION_SECONDS: i64 = 315_576_000_000; + +/// Convert a protobuf `Duration` to [`std::time::Duration`], returning `None` for +/// values outside the documented `google.protobuf.Duration` range. Rejected here +/// so an invalid retry policy fails validation rather than overflowing later. +fn proto_duration(d: &envoy_types::pb::google::protobuf::Duration) -> Option { + if !(0..=MAX_PROTO_DURATION_SECONDS).contains(&d.seconds) + || !(0..=999_999_999).contains(&d.nanos) + { + return None; + } + let seconds = u64::try_from(d.seconds).ok()?; + let nanos = u32::try_from(d.nanos).ok()?; + Some(Duration::new(seconds, nanos)) +} + +/// Parse and validate an Envoy `RetryPolicy` into a shared [`RouteRetryConfig`]. +fn parse_retry(rp: &RetryPolicy) -> xds_client::Result> { + Ok(Arc::new(RouteRetryConfig::from_proto(rp)?)) +} + /// Validated virtual host with domain matching and routes. #[derive(Debug, Clone)] pub(crate) struct VirtualHostConfig { @@ -162,6 +254,10 @@ pub(crate) struct VirtualHostConfig { pub(crate) struct RouteConfig { pub match_criteria: RouteConfigMatch, pub action: RouteConfigAction, + /// Validated retry settings (gRFC A44): the route's own `RouteAction.retry_policy`, + /// else the inherited `VirtualHost.retry_policy` (route-level fully overrides, + /// no merge), else `None`. Routes that inherit share one `Arc`. + pub retry_config: Option>, } /// Validated route match criteria. @@ -265,8 +361,11 @@ impl Resource for RouteConfigResource { } let mut routes = Vec::with_capacity(vh.routes.len()); + // gRFC A44: routes inherit the virtual host's retry policy unless they + // set their own. Parse it once so inheriting routes share one `Arc`. + let vh_retry = vh.retry_policy.as_ref().map(parse_retry).transpose()?; for route in vh.routes { - if let Some(validated_route) = validate_route(route)? { + if let Some(validated_route) = validate_route(route, vh_retry.as_ref())? { routes.push(validated_route); } } @@ -288,8 +387,11 @@ impl Resource for RouteConfigResource { /// Returns `Ok(None)` for routes that should be silently skipped (query param matchers, /// unsupported cluster specifiers like `cluster_header`). +/// +/// `vh_retry` is the virtual host's policy, inherited when the route sets none. fn validate_route( route: envoy_types::pb::envoy::config::route::v3::Route, + vh_retry: Option<&Arc>, ) -> xds_client::Result> { let route_match = route .r#match @@ -306,11 +408,19 @@ fn validate_route( .action .ok_or_else(|| Error::Validation("route missing action field".into()))?; - let validated_action = match action { - route::Action::Route(route_action) => match validate_route_action(route_action)? { - Some(action) => action, - None => return Ok(None), - }, + let validated_action; + let route_retry; + match action { + route::Action::Route(mut route_action) => { + // Take the retry policy before `route_action` is consumed; parse it + // only if the route is kept, so dropped routes cost no parse. + let retry_policy = route_action.retry_policy.take(); + match validate_route_action(route_action)? { + Some(action) => validated_action = action, + None => return Ok(None), + } + route_retry = retry_policy.as_ref().map(parse_retry).transpose()?; + } // Per A28: action field must be "route", otherwise NACK. _ => { return Err(Error::Validation( @@ -322,6 +432,7 @@ fn validate_route( Ok(Some(RouteConfig { match_criteria, action: validated_action, + retry_config: route_retry.or_else(|| vh_retry.cloned()), })) } @@ -498,8 +609,10 @@ impl RouteConfigResource { mod tests { use super::*; use envoy_types::pb::envoy::config::route::v3::{ - RouteAction, VirtualHost, route::Action, route_action::ClusterSpecifier, + RetryPolicy, RouteAction, VirtualHost, retry_policy::RetryBackOff, route::Action, + route_action::ClusterSpecifier, }; + use envoy_types::pb::google::protobuf::{Duration as ProtoDuration, UInt32Value}; fn make_route(prefix: &str, cluster: &str) -> envoy_types::pb::envoy::config::route::v3::Route { envoy_types::pb::envoy::config::route::v3::Route { @@ -515,6 +628,65 @@ mod tests { } } + fn retry_policy(retry_on: &str, num_retries: u32) -> RetryPolicy { + RetryPolicy { + retry_on: retry_on.to_string(), + num_retries: Some(UInt32Value { value: num_retries }), + ..Default::default() + } + } + + fn make_route_with_retry( + prefix: &str, + cluster: &str, + retry: RetryPolicy, + ) -> envoy_types::pb::envoy::config::route::v3::Route { + envoy_types::pb::envoy::config::route::v3::Route { + r#match: Some(RouteMatch { + path_specifier: Some(route_match::PathSpecifier::Prefix(prefix.to_string())), + ..Default::default() + }), + action: Some(Action::Route(RouteAction { + cluster_specifier: Some(ClusterSpecifier::Cluster(cluster.to_string())), + retry_policy: Some(retry), + ..Default::default() + })), + ..Default::default() + } + } + + fn proto_dur(seconds: i64, nanos: i32) -> ProtoDuration { + ProtoDuration { seconds, nanos } + } + + fn retry_policy_with_backoff( + base: Option, + max: Option, + ) -> RetryPolicy { + RetryPolicy { + retry_on: "unavailable".to_string(), + retry_back_off: Some(RetryBackOff { + base_interval: base, + max_interval: max, + }), + ..Default::default() + } + } + + fn validate_with_route_retry(retry: RetryPolicy) -> xds_client::Result { + let rc = RouteConfiguration { + name: "rc".to_string(), + virtual_hosts: vec![VirtualHost { + name: "vh1".to_string(), + domains: vec!["*".to_string()], + routes: vec![make_route_with_retry("/", "c1", retry)], + ..Default::default() + }], + ..Default::default() + }; + RouteConfigResource::validate(rc) + } + fn make_route_config(name: &str) -> RouteConfiguration { RouteConfiguration { name: name.to_string(), @@ -977,4 +1149,123 @@ mod tests { assert_eq!(headers.len(), 1); assert_eq!(headers[0].name, "x-env"); } + + #[test] + fn test_vhost_retry_policy_inherited_and_route_overrides() { + // gRFC A44: a route with no policy inherits the virtual host's; a route + // with its own policy completely overrides it. Fields are private, so we + // verify the *selection* structurally via `Arc` identity — value mapping + // is covered by the `from_route_retry` tests in `client::retry`. + let rc = RouteConfiguration { + name: "rc".to_string(), + virtual_hosts: vec![VirtualHost { + name: "vh1".to_string(), + domains: vec!["*".to_string()], + retry_policy: Some(retry_policy("unavailable", 3)), + routes: vec![ + make_route_with_retry("/own", "c-own", retry_policy("cancelled", 2)), + make_route("/inherit-a", "c-a"), + make_route("/inherit-b", "c-b"), + ], + ..Default::default() + }], + ..Default::default() + }; + let validated = RouteConfigResource::validate(rc).unwrap(); + let routes = &validated.virtual_hosts[0].routes; + + let own = routes[0].retry_config.as_ref().expect("route-level policy"); + let inherit_a = routes[1].retry_config.as_ref().expect("inherited policy"); + let inherit_b = routes[2].retry_config.as_ref().expect("inherited policy"); + + // The overriding route gets its own config, distinct from the vhost's. + assert!(!Arc::ptr_eq(own, inherit_a)); + // Inheriting routes share the single vhost-level config `Arc`. + assert!(Arc::ptr_eq(inherit_a, inherit_b)); + } + + #[test] + fn test_no_retry_policy_yields_none() { + // No vhost policy and no route policy => the route carries no retry config. + let rc = RouteConfiguration { + name: "rc".to_string(), + virtual_hosts: vec![VirtualHost { + name: "vh1".to_string(), + domains: vec!["*".to_string()], + routes: vec![make_route("/", "c1")], + ..Default::default() + }], + ..Default::default() + }; + let validated = RouteConfigResource::validate(rc).unwrap(); + assert!(validated.virtual_hosts[0].routes[0].retry_config.is_none()); + } + + // gRFC A44: the resource must be NACKed (validation error) when num_retries < 1 + // or a set base_interval/max_interval is not greater than zero. + + #[test] + fn test_retry_num_retries_zero_is_rejected() { + let err = validate_with_route_retry(retry_policy("unavailable", 0)).unwrap_err(); + assert!(matches!(err, Error::Validation(_))); + } + + #[test] + fn test_retry_base_interval_zero_is_rejected() { + let policy = retry_policy_with_backoff(Some(proto_dur(0, 0)), None); + assert!(validate_with_route_retry(policy).is_err()); + } + + #[test] + fn test_retry_base_interval_negative_is_rejected() { + let policy = retry_policy_with_backoff(Some(proto_dur(-1, 0)), None); + assert!(validate_with_route_retry(policy).is_err()); + } + + #[test] + fn test_retry_back_off_without_base_interval_is_rejected() { + // retry_back_off set but base_interval unset => base is 0 => rejected. + let policy = retry_policy_with_backoff(None, Some(proto_dur(1, 0))); + assert!(validate_with_route_retry(policy).is_err()); + } + + #[test] + fn test_retry_max_interval_zero_is_rejected() { + let policy = + retry_policy_with_backoff(Some(proto_dur(0, 100_000_000)), Some(proto_dur(0, 0))); + assert!(validate_with_route_retry(policy).is_err()); + } + + #[test] + fn test_retry_base_interval_below_1ms_is_accepted() { + // A44: values < 1ms are treated as 1ms (clamped), not rejected. + let policy = retry_policy_with_backoff(Some(proto_dur(0, 500_000)), None); + assert!(validate_with_route_retry(policy).is_ok()); + } + + #[test] + fn test_retry_valid_backoff_is_accepted() { + let mut policy = + retry_policy_with_backoff(Some(proto_dur(0, 100_000_000)), Some(proto_dur(1, 0))); + policy.num_retries = Some(UInt32Value { value: 2 }); + let validated = validate_with_route_retry(policy).expect("valid retry policy"); + assert!(validated.virtual_hosts[0].routes[0].retry_config.is_some()); + } + + #[test] + fn test_vhost_retry_invalid_is_rejected() { + // A virtual-host-level policy is validated the same way and NACKs on error. + let rc = RouteConfiguration { + name: "rc".to_string(), + virtual_hosts: vec![VirtualHost { + name: "vh1".to_string(), + domains: vec!["*".to_string()], + retry_policy: Some(retry_policy("unavailable", 0)), + routes: vec![make_route("/", "c1")], + ..Default::default() + }], + ..Default::default() + }; + assert!(RouteConfigResource::validate(rc).is_err()); + } } diff --git a/tonic-xds/src/xds/resource_manager.rs b/tonic-xds/src/xds/resource_manager.rs index 9f0c80f1d..ea8c847b2 100644 --- a/tonic-xds/src/xds/resource_manager.rs +++ b/tonic-xds/src/xds/resource_manager.rs @@ -341,6 +341,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster((*c).into()), + retry_config: None, }) .collect(), }], @@ -366,6 +367,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster((*c).into()), + retry_config: None, }) .collect(), }], diff --git a/tonic-xds/src/xds/routing.rs b/tonic-xds/src/xds/routing.rs index 4740487fe..4a43e9d9b 100644 --- a/tonic-xds/src/xds/routing.rs +++ b/tonic-xds/src/xds/routing.rs @@ -43,7 +43,7 @@ use std::time::Duration; use arc_swap::ArcSwapOption; use tokio::sync::watch; -use crate::client::route::{AcquiredConfig, RouteDecision, RouteInput, Router}; +use crate::client::route::{AcquiredConfig, RouteDecision, RouteInput, Router, RoutingSnapshot}; use crate::common::async_util::AbortOnDrop; use crate::xds::cache::XdsCache; use crate::xds::resource::hash_policy::HashPolicyConfig; @@ -66,7 +66,7 @@ const DEFAULT_READY_TIMEOUT: Duration = Duration::from_secs(30); /// config is available, matching standard gRPC behavior where RPCs wait for the /// resolver's first update. Subsequent RPCs read the config lock-free. pub(crate) struct XdsRouter { - route_config: Arc>, + route_config: Arc>, ready_rx: watch::Receiver, _watch_task: AbortOnDrop, } @@ -85,7 +85,7 @@ impl XdsRouter { let handle = tokio::spawn(async move { let mut ready_tx = Some(ready_tx); while let Some(config) = watcher.next().await { - rc.store(Some(config)); + rc.store(Some(Arc::new(RoutingSnapshot::new(config)))); // Signal readiness on the first config, then drop the sender. if let Some(tx) = ready_tx.take() { let _ = tx.send(true); @@ -100,7 +100,7 @@ impl XdsRouter { } /// The route config currently in effect, or `None` if none has arrived yet. - pub(crate) fn snapshot(&self) -> Option> { + pub(crate) fn snapshot(&self) -> Option> { self.route_config.load_full() } } @@ -126,7 +126,7 @@ impl Router for XdsRouter { fn route( &self, input: &RouteInput<'_>, - config: &RouteConfigResource, + config: &RoutingSnapshot, ) -> Result { resolve_route(config, input.authority, input.headers) } @@ -134,7 +134,7 @@ impl Router for XdsRouter { /// Resolve a route decision from the given config, authority, and headers. fn resolve_route( - rc: &RouteConfigResource, + rc: &RoutingSnapshot, authority: &str, headers: &http::HeaderMap, ) -> Result { @@ -142,14 +142,18 @@ fn resolve_route( .get(":path") .and_then(|v| v.to_str().ok()) .unwrap_or("/"); - let action = rc.route(authority, path, headers)?; - let cluster = match action { + let route = rc.matched_route(authority, path, headers)?; + let cluster = match &route.action { RouteConfigAction::Cluster(name) => name.clone(), RouteConfigAction::WeightedClusters(clusters) => select_weighted_cluster(clusters) .ok_or(RoutingError::EmptyWeightedClusters)? .to_string(), }; + // Retry config for the matched route, from the same snapshot we routed on so + // retry and routing act on one RDS version. + let retry_config = rc.retry_for(route.retry_config.as_ref()); + // gRFC A42 ring-hash request hash. The policy list is empty for now, so // `request_hash` resolves to `None` and the ring-hash picker falls back to a // random hash. @@ -161,6 +165,7 @@ fn resolve_route( Ok(RouteDecision { cluster, request_hash, + retry_config, }) } @@ -181,19 +186,37 @@ impl RouteConfigResource { /// Match a request and return the target cluster action. /// /// Performs domain matching on the authority, then walks routes in order - /// to find the first match. + /// to find the first match. Test-only convenience wrapper around + /// [`matched_route`](Self::matched_route); production routing uses + /// `matched_route` so it can also read the matched route's retry policy. + #[cfg(test)] pub(crate) fn route( &self, authority: &str, path: &str, headers: &http::HeaderMap, ) -> Result<&RouteConfigAction, RoutingError> { + self.matched_route(authority, path, headers) + .map(|route| &route.action) + } + + /// Match a request and return the full matched [`RouteConfig`]. + /// + /// Performs domain matching on the authority, then walks routes in order to + /// find the first match. Returns the whole matched route so callers can read + /// per-route fields (e.g. the retry policy) alongside the action. + pub(crate) fn matched_route( + &self, + authority: &str, + path: &str, + headers: &http::HeaderMap, + ) -> Result<&RouteConfig, RoutingError> { let vh = find_best_matching_virtual_host(authority, &self.virtual_hosts) .ok_or_else(|| RoutingError::NoMatchingVirtualHost(authority.to_string()))?; for route in &vh.routes { if route_matches(route, path, headers) { - return Ok(&route.action); + return Ok(route); } } @@ -391,6 +414,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster(cluster.into()), + retry_config: None, } } @@ -623,6 +647,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("c1".into()), + retry_config: None, }], }]); let headers = http::HeaderMap::new(); @@ -646,6 +671,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("c1".into()), + retry_config: None, }], }]); let headers = http::HeaderMap::new(); @@ -677,6 +703,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("cluster-prod".into()), + retry_config: None, }, simple_route("/", "cluster-default"), ], @@ -714,6 +741,7 @@ mod tests { weight: 30, }, ]), + retry_config: None, }], }]); let action = rc.route("host", "/", &http::HeaderMap::new()).unwrap(); @@ -734,6 +762,7 @@ mod tests { match_fraction: Some(0), }, action: RouteConfigAction::Cluster("never".into()), + retry_config: None, }, simple_route("/", "fallback"), ], @@ -757,6 +786,7 @@ mod tests { match_fraction: Some(1_000_000), }, action: RouteConfigAction::Cluster("always".into()), + retry_config: None, }], }]); for _ in 0..100 { @@ -786,6 +816,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("versioned".into()), + retry_config: None, }, simple_route("/", "default"), ], @@ -881,6 +912,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("grpc".into()), + retry_config: None, }, simple_route("/", "fallback"), ], @@ -916,6 +948,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("matched".into()), + retry_config: None, }], }]); @@ -953,6 +986,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("matched".into()), + retry_config: None, }, simple_route("/", "fallback"), ], @@ -1011,6 +1045,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("matched".into()), + retry_config: None, }, simple_route("/", "fallback"), ], @@ -1052,6 +1087,7 @@ mod tests { match_fraction: None, }, action: RouteConfigAction::Cluster("matched".into()), + retry_config: None, }, simple_route("/", "fallback"), ],