From 2cb56908873644ab1dffa998fbe28eefb260e99d Mon Sep 17 00:00:00 2001 From: Madhurish Nitish Gupta Date: Wed, 24 Jun 2026 10:28:58 -0700 Subject: [PATCH 1/5] feat(tonic-xds): parse RING_HASH lb policy from CDS (gRFC A42) Accept lb_policy: RING_HASH and parse ring_hash_lb_config into LbPolicy::RingHash { min_ring_size, max_ring_size }: - NACK a hash_function other than XX_HASH. - NACK any ring size above the 8M ceiling, and NACK min_ring_size > max_ring_size (matching grpc-go). - Default unset minimum_ring_size to 1024 and maximum_ring_size to 4096, then clamp both to the local cap of 4096. Picker selection from lb_policy is deferred (no live Stack B construction yet). --- tonic-xds/src/xds/resource/cluster.rs | 169 +++++++++++++++++++++++++- 1 file changed, 167 insertions(+), 2 deletions(-) diff --git a/tonic-xds/src/xds/resource/cluster.rs b/tonic-xds/src/xds/resource/cluster.rs index a21619e32..b8666a185 100644 --- a/tonic-xds/src/xds/resource/cluster.rs +++ b/tonic-xds/src/xds/resource/cluster.rs @@ -27,8 +27,21 @@ pub(crate) struct ClusterResource { pub(crate) enum LbPolicy { RoundRobin, LeastRequest, + /// Ring-hash (gRFC A42). Carries the validated ring bounds parsed from + /// `ring_hash_lb_config`. + RingHash { + min_ring_size: u64, + max_ring_size: u64, + }, } +/// gRFC A42 ring-hash sizing. `minimum_ring_size` defaults to 1024 and +/// `maximum_ring_size` to the local cap of 4096 when unset; both are clamped to +/// that cap, and any configured value above the 8M ceiling is rejected. +const RING_HASH_DEFAULT_MIN_SIZE: u64 = 1024; +const RING_HASH_SIZE_CAP: u64 = 4096; +const RING_HASH_SIZE_CEILING: u64 = 8 * 1024 * 1024; + impl Resource for ClusterResource { type Message = Cluster; @@ -58,6 +71,7 @@ impl Resource for ClusterResource { let lb_policy = match cluster::LbPolicy::try_from(message.lb_policy) { Ok(cluster::LbPolicy::RoundRobin) => LbPolicy::RoundRobin, Ok(cluster::LbPolicy::LeastRequest) => LbPolicy::LeastRequest, + Ok(cluster::LbPolicy::RingHash) => parse_ring_hash(message.lb_config)?, _ => { return Err(Error::Validation(format!( "unsupported load balancing policy: {}", @@ -85,10 +99,68 @@ impl ClusterResource { } } +/// Parse and validate `ring_hash_lb_config` (gRFC A42) from a Cluster's +/// `lb_config` oneof into [`LbPolicy::RingHash`]. +/// +/// Rejects a `hash_function` other than `XX_HASH` and any ring size above the +/// 8M ceiling. Unset sizes take the xDS defaults (1024 / 8M); the resolved +/// bounds are then clamped to the local cap. +fn parse_ring_hash(lb_config: Option) -> xds_client::Result { + use cluster::ring_hash_lb_config::HashFunction; + + let (min_field, max_field, hash_function) = match lb_config { + Some(cluster::LbConfig::RingHashLbConfig(c)) => { + (c.minimum_ring_size, c.maximum_ring_size, c.hash_function) + } + // No ring_hash_lb_config → defaults (XX_HASH is the proto default). + _ => (None, None, HashFunction::XxHash as i32), + }; + + match HashFunction::try_from(hash_function) { + Ok(HashFunction::XxHash) => {} + Ok(other) => { + return Err(Error::Validation(format!( + "unsupported ring_hash hash function: {}", + other.as_str_name() + ))); + } + Err(_) => { + return Err(Error::Validation(format!( + "unknown ring_hash hash function: {hash_function}" + ))); + } + } + + let min = min_field.map_or(RING_HASH_DEFAULT_MIN_SIZE, |v| v.value); + let max = max_field.map_or(RING_HASH_SIZE_CAP, |v| v.value); + if min > RING_HASH_SIZE_CEILING || max > RING_HASH_SIZE_CEILING { + return Err(Error::Validation(format!( + "ring_hash ring size exceeds the maximum of {RING_HASH_SIZE_CEILING} \ + (min_ring_size={min}, max_ring_size={max})" + ))); + } + // Checked on the resolved sizes before the cap is applied, matching grpc-go. + if min > max { + return Err(Error::Validation(format!( + "ring_hash min_ring_size ({min}) is greater than max_ring_size ({max})" + ))); + } + + Ok(LbPolicy::RingHash { + min_ring_size: min.min(RING_HASH_SIZE_CAP), + max_ring_size: max.min(RING_HASH_SIZE_CAP), + }) +} + #[cfg(test)] mod tests { use super::*; use envoy_types::pb::envoy::config::cluster::v3::cluster::EdsClusterConfig; + use envoy_types::pb::google::protobuf::UInt64Value; + + fn ring_size(value: u64) -> UInt64Value { + UInt64Value { value } + } fn make_cluster(name: &str) -> Cluster { Cluster { @@ -143,9 +215,10 @@ mod tests { #[test] fn test_unsupported_lb_policy_is_rejected() { + // A policy we don't support (e.g. MAGLEV) is still NACKed. let cluster = Cluster { - name: "rh-cluster".to_string(), - lb_policy: cluster::LbPolicy::RingHash as i32, + name: "mg-cluster".to_string(), + lb_policy: cluster::LbPolicy::Maglev as i32, ..Default::default() }; let err = ClusterResource::validate(cluster).unwrap_err(); @@ -155,6 +228,98 @@ mod tests { ); } + /// Build a RING_HASH cluster, optionally carrying a `ring_hash_lb_config`. + fn ring_hash_cluster(config: Option) -> Cluster { + Cluster { + name: "rh-cluster".to_string(), + lb_policy: cluster::LbPolicy::RingHash as i32, + lb_config: config.map(cluster::LbConfig::RingHashLbConfig), + ..Default::default() + } + } + + #[test] + fn test_ring_hash_defaults() { + // No ring_hash_lb_config → min 1024, max defaults to 8M then clamps to + // the local cap of 4096; XX_HASH is the default hash function. + let validated = ClusterResource::validate(ring_hash_cluster(None)).unwrap(); + assert_eq!( + validated.lb_policy, + LbPolicy::RingHash { + min_ring_size: 1024, + max_ring_size: 4096, + } + ); + } + + #[test] + fn test_ring_hash_custom_sizes_within_cap() { + let validated = + ClusterResource::validate(ring_hash_cluster(Some(cluster::RingHashLbConfig { + minimum_ring_size: Some(ring_size(2048)), + maximum_ring_size: Some(ring_size(3000)), + ..Default::default() + }))) + .unwrap(); + assert_eq!( + validated.lb_policy, + LbPolicy::RingHash { + min_ring_size: 2048, + max_ring_size: 3000, + } + ); + } + + #[test] + fn test_ring_hash_sizes_clamped_to_local_cap() { + // Sizes within the 8M ceiling but above the local cap clamp to 4096. + let validated = + ClusterResource::validate(ring_hash_cluster(Some(cluster::RingHashLbConfig { + minimum_ring_size: Some(ring_size(100_000)), + maximum_ring_size: Some(ring_size(100_000)), + ..Default::default() + }))) + .unwrap(); + assert_eq!( + validated.lb_policy, + LbPolicy::RingHash { + min_ring_size: 4096, + max_ring_size: 4096, + } + ); + } + + #[test] + fn test_ring_hash_rejects_non_xx_hash() { + let cluster = ring_hash_cluster(Some(cluster::RingHashLbConfig { + hash_function: cluster::ring_hash_lb_config::HashFunction::MurmurHash2 as i32, + ..Default::default() + })); + let err = ClusterResource::validate(cluster).unwrap_err(); + assert!(err.to_string().contains("hash function")); + } + + #[test] + fn test_ring_hash_rejects_size_above_ceiling() { + let cluster = ring_hash_cluster(Some(cluster::RingHashLbConfig { + maximum_ring_size: Some(ring_size(8 * 1024 * 1024 + 1)), + ..Default::default() + })); + let err = ClusterResource::validate(cluster).unwrap_err(); + assert!(err.to_string().contains("exceeds the maximum")); + } + + #[test] + fn test_ring_hash_rejects_min_greater_than_max() { + let cluster = ring_hash_cluster(Some(cluster::RingHashLbConfig { + minimum_ring_size: Some(ring_size(3000)), + maximum_ring_size: Some(ring_size(2000)), + ..Default::default() + })); + let err = ClusterResource::validate(cluster).unwrap_err(); + assert!(err.to_string().contains("greater than max_ring_size")); + } + #[test] fn test_validate_empty_name() { let cluster = make_cluster(""); From a921b5ebe67439c60b4ec2c57a49f8ee5ed0caa4 Mon Sep 17 00:00:00 2001 From: Madhurish Nitish Gupta Date: Wed, 24 Jun 2026 10:41:59 -0700 Subject: [PATCH 2/5] refactor(tonic-xds): extract LbPolicy + ring-hash validation into lb_policy Move the LbPolicy enum and ring_hash_lb_config parsing out of cluster.rs into a dedicated lb_policy module. The RingHash variant now carries a RingHashSettings payload whose validate() owns the gRFC A42 rules, so the validation lives with the config it produces and is unit-testable on its own. cluster.rs re-exports LbPolicy so existing cluster::LbPolicy paths are unchanged; ClusterResource just delegates to RingHashSettings::validate. --- tonic-xds/src/xds/resource/cluster.rs | 177 +++--------------------- tonic-xds/src/xds/resource/lb_policy.rs | 177 ++++++++++++++++++++++++ tonic-xds/src/xds/resource/mod.rs | 1 + 3 files changed, 194 insertions(+), 161 deletions(-) create mode 100644 tonic-xds/src/xds/resource/lb_policy.rs diff --git a/tonic-xds/src/xds/resource/cluster.rs b/tonic-xds/src/xds/resource/cluster.rs index b8666a185..345f0dc90 100644 --- a/tonic-xds/src/xds/resource/cluster.rs +++ b/tonic-xds/src/xds/resource/cluster.rs @@ -8,6 +8,9 @@ use xds_client::{Error, Resource}; use super::security::{ClusterSecurityConfig, parse_transport_socket}; +// Re-exported so other modules keep using the `cluster::LbPolicy` path. +pub(crate) use super::lb_policy::{LbPolicy, RingHashSettings}; + /// Validated Cluster resource. #[derive(Debug, Clone)] pub(crate) struct ClusterResource { @@ -22,26 +25,6 @@ pub(crate) struct ClusterResource { pub security: Option, } -/// Load balancing policies. -#[derive(Debug, Clone, Copy, PartialEq, Eq)] -pub(crate) enum LbPolicy { - RoundRobin, - LeastRequest, - /// Ring-hash (gRFC A42). Carries the validated ring bounds parsed from - /// `ring_hash_lb_config`. - RingHash { - min_ring_size: u64, - max_ring_size: u64, - }, -} - -/// gRFC A42 ring-hash sizing. `minimum_ring_size` defaults to 1024 and -/// `maximum_ring_size` to the local cap of 4096 when unset; both are clamped to -/// that cap, and any configured value above the 8M ceiling is rejected. -const RING_HASH_DEFAULT_MIN_SIZE: u64 = 1024; -const RING_HASH_SIZE_CAP: u64 = 4096; -const RING_HASH_SIZE_CEILING: u64 = 8 * 1024 * 1024; - impl Resource for ClusterResource { type Message = Cluster; @@ -71,7 +54,9 @@ impl Resource for ClusterResource { let lb_policy = match cluster::LbPolicy::try_from(message.lb_policy) { Ok(cluster::LbPolicy::RoundRobin) => LbPolicy::RoundRobin, Ok(cluster::LbPolicy::LeastRequest) => LbPolicy::LeastRequest, - Ok(cluster::LbPolicy::RingHash) => parse_ring_hash(message.lb_config)?, + Ok(cluster::LbPolicy::RingHash) => { + LbPolicy::RingHash(RingHashSettings::validate(message.lb_config)?) + } _ => { return Err(Error::Validation(format!( "unsupported load balancing policy: {}", @@ -99,68 +84,10 @@ impl ClusterResource { } } -/// Parse and validate `ring_hash_lb_config` (gRFC A42) from a Cluster's -/// `lb_config` oneof into [`LbPolicy::RingHash`]. -/// -/// Rejects a `hash_function` other than `XX_HASH` and any ring size above the -/// 8M ceiling. Unset sizes take the xDS defaults (1024 / 8M); the resolved -/// bounds are then clamped to the local cap. -fn parse_ring_hash(lb_config: Option) -> xds_client::Result { - use cluster::ring_hash_lb_config::HashFunction; - - let (min_field, max_field, hash_function) = match lb_config { - Some(cluster::LbConfig::RingHashLbConfig(c)) => { - (c.minimum_ring_size, c.maximum_ring_size, c.hash_function) - } - // No ring_hash_lb_config → defaults (XX_HASH is the proto default). - _ => (None, None, HashFunction::XxHash as i32), - }; - - match HashFunction::try_from(hash_function) { - Ok(HashFunction::XxHash) => {} - Ok(other) => { - return Err(Error::Validation(format!( - "unsupported ring_hash hash function: {}", - other.as_str_name() - ))); - } - Err(_) => { - return Err(Error::Validation(format!( - "unknown ring_hash hash function: {hash_function}" - ))); - } - } - - let min = min_field.map_or(RING_HASH_DEFAULT_MIN_SIZE, |v| v.value); - let max = max_field.map_or(RING_HASH_SIZE_CAP, |v| v.value); - if min > RING_HASH_SIZE_CEILING || max > RING_HASH_SIZE_CEILING { - return Err(Error::Validation(format!( - "ring_hash ring size exceeds the maximum of {RING_HASH_SIZE_CEILING} \ - (min_ring_size={min}, max_ring_size={max})" - ))); - } - // Checked on the resolved sizes before the cap is applied, matching grpc-go. - if min > max { - return Err(Error::Validation(format!( - "ring_hash min_ring_size ({min}) is greater than max_ring_size ({max})" - ))); - } - - Ok(LbPolicy::RingHash { - min_ring_size: min.min(RING_HASH_SIZE_CAP), - max_ring_size: max.min(RING_HASH_SIZE_CAP), - }) -} - #[cfg(test)] mod tests { use super::*; use envoy_types::pb::envoy::config::cluster::v3::cluster::EdsClusterConfig; - use envoy_types::pb::google::protobuf::UInt64Value; - - fn ring_size(value: u64) -> UInt64Value { - UInt64Value { value } - } fn make_cluster(name: &str) -> Cluster { Cluster { @@ -228,98 +155,26 @@ mod tests { ); } - /// Build a RING_HASH cluster, optionally carrying a `ring_hash_lb_config`. - fn ring_hash_cluster(config: Option) -> Cluster { - Cluster { + #[test] + fn test_ring_hash_lb_policy() { + // A RING_HASH cluster validates to LbPolicy::RingHash with the parsed + // (here defaulted) settings. Detailed ring_hash_lb_config validation is + // covered in the `lb_policy` module. + let cluster = Cluster { name: "rh-cluster".to_string(), lb_policy: cluster::LbPolicy::RingHash as i32, - lb_config: config.map(cluster::LbConfig::RingHashLbConfig), ..Default::default() - } - } - - #[test] - fn test_ring_hash_defaults() { - // No ring_hash_lb_config → min 1024, max defaults to 8M then clamps to - // the local cap of 4096; XX_HASH is the default hash function. - let validated = ClusterResource::validate(ring_hash_cluster(None)).unwrap(); + }; + let validated = ClusterResource::validate(cluster).unwrap(); assert_eq!( validated.lb_policy, - LbPolicy::RingHash { + LbPolicy::RingHash(RingHashSettings { min_ring_size: 1024, max_ring_size: 4096, - } + }) ); } - #[test] - fn test_ring_hash_custom_sizes_within_cap() { - let validated = - ClusterResource::validate(ring_hash_cluster(Some(cluster::RingHashLbConfig { - minimum_ring_size: Some(ring_size(2048)), - maximum_ring_size: Some(ring_size(3000)), - ..Default::default() - }))) - .unwrap(); - assert_eq!( - validated.lb_policy, - LbPolicy::RingHash { - min_ring_size: 2048, - max_ring_size: 3000, - } - ); - } - - #[test] - fn test_ring_hash_sizes_clamped_to_local_cap() { - // Sizes within the 8M ceiling but above the local cap clamp to 4096. - let validated = - ClusterResource::validate(ring_hash_cluster(Some(cluster::RingHashLbConfig { - minimum_ring_size: Some(ring_size(100_000)), - maximum_ring_size: Some(ring_size(100_000)), - ..Default::default() - }))) - .unwrap(); - assert_eq!( - validated.lb_policy, - LbPolicy::RingHash { - min_ring_size: 4096, - max_ring_size: 4096, - } - ); - } - - #[test] - fn test_ring_hash_rejects_non_xx_hash() { - let cluster = ring_hash_cluster(Some(cluster::RingHashLbConfig { - hash_function: cluster::ring_hash_lb_config::HashFunction::MurmurHash2 as i32, - ..Default::default() - })); - let err = ClusterResource::validate(cluster).unwrap_err(); - assert!(err.to_string().contains("hash function")); - } - - #[test] - fn test_ring_hash_rejects_size_above_ceiling() { - let cluster = ring_hash_cluster(Some(cluster::RingHashLbConfig { - maximum_ring_size: Some(ring_size(8 * 1024 * 1024 + 1)), - ..Default::default() - })); - let err = ClusterResource::validate(cluster).unwrap_err(); - assert!(err.to_string().contains("exceeds the maximum")); - } - - #[test] - fn test_ring_hash_rejects_min_greater_than_max() { - let cluster = ring_hash_cluster(Some(cluster::RingHashLbConfig { - minimum_ring_size: Some(ring_size(3000)), - maximum_ring_size: Some(ring_size(2000)), - ..Default::default() - })); - let err = ClusterResource::validate(cluster).unwrap_err(); - assert!(err.to_string().contains("greater than max_ring_size")); - } - #[test] fn test_validate_empty_name() { let cluster = make_cluster(""); diff --git a/tonic-xds/src/xds/resource/lb_policy.rs b/tonic-xds/src/xds/resource/lb_policy.rs new file mode 100644 index 000000000..e11012fcc --- /dev/null +++ b/tonic-xds/src/xds/resource/lb_policy.rs @@ -0,0 +1,177 @@ +//! Cluster load-balancing policy (CDS) and ring-hash config validation. + +use envoy_types::pb::envoy::config::cluster::v3::cluster; +use xds_client::Error; + +/// Load balancing policies. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) enum LbPolicy { + RoundRobin, + LeastRequest, + /// Ring-hash (gRFC A42), carrying the validated ring bounds. + RingHash(RingHashSettings), +} + +/// Validated gRFC A42 ring-hash sizing parsed from `ring_hash_lb_config`. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub(crate) struct RingHashSettings { + pub min_ring_size: u64, + pub max_ring_size: u64, +} + +/// gRFC A42 ring-hash sizing. `minimum_ring_size` defaults to 1024 and +/// `maximum_ring_size` to the local cap of 4096 when unset; both are clamped to +/// that cap, and any configured value above the 8M ceiling is rejected. +pub(crate) const RING_HASH_DEFAULT_MIN_SIZE: u64 = 1024; +pub(crate) const RING_HASH_SIZE_CAP: u64 = 4096; +pub(crate) const RING_HASH_SIZE_CEILING: u64 = 8 * 1024 * 1024; + +impl RingHashSettings { + /// Validate a Cluster's `lb_config` oneof as `ring_hash_lb_config` (gRFC + /// A42). + /// + /// Rejects a `hash_function` other than `XX_HASH`, any ring size above the + /// 8M ceiling, and `min_ring_size > max_ring_size`. Unset sizes take the + /// defaults (1024 / 4096); the resolved bounds are then clamped to the + /// local cap. + pub(crate) fn validate(lb_config: Option) -> xds_client::Result { + use cluster::ring_hash_lb_config::HashFunction; + + let (min_field, max_field, hash_function) = match lb_config { + Some(cluster::LbConfig::RingHashLbConfig(c)) => { + (c.minimum_ring_size, c.maximum_ring_size, c.hash_function) + } + // No ring_hash_lb_config → defaults (XX_HASH is the proto default). + _ => (None, None, HashFunction::XxHash as i32), + }; + + match HashFunction::try_from(hash_function) { + Ok(HashFunction::XxHash) => {} + Ok(other) => { + return Err(Error::Validation(format!( + "unsupported ring_hash hash function: {}", + other.as_str_name() + ))); + } + Err(_) => { + return Err(Error::Validation(format!( + "unknown ring_hash hash function: {hash_function}" + ))); + } + } + + let min = min_field.map_or(RING_HASH_DEFAULT_MIN_SIZE, |v| v.value); + let max = max_field.map_or(RING_HASH_SIZE_CAP, |v| v.value); + if min > RING_HASH_SIZE_CEILING || max > RING_HASH_SIZE_CEILING { + return Err(Error::Validation(format!( + "ring_hash ring size exceeds the maximum of {RING_HASH_SIZE_CEILING} \ + (min_ring_size={min}, max_ring_size={max})" + ))); + } + // Checked on the resolved sizes before the cap is applied. + if min > max { + return Err(Error::Validation(format!( + "ring_hash min_ring_size ({min}) is greater than max_ring_size ({max})" + ))); + } + + Ok(RingHashSettings { + min_ring_size: min.min(RING_HASH_SIZE_CAP), + max_ring_size: max.min(RING_HASH_SIZE_CAP), + }) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use envoy_types::pb::google::protobuf::UInt64Value; + + fn ring_size(value: u64) -> UInt64Value { + UInt64Value { value } + } + + fn lb_config(config: cluster::RingHashLbConfig) -> Option { + Some(cluster::LbConfig::RingHashLbConfig(config)) + } + + #[test] + fn ring_hash_defaults() { + // No ring_hash_lb_config → min 1024, max defaults to the cap; XX_HASH + // is the default hash function. + let settings = RingHashSettings::validate(None).unwrap(); + assert_eq!( + settings, + RingHashSettings { + min_ring_size: 1024, + max_ring_size: 4096, + } + ); + } + + #[test] + fn ring_hash_custom_sizes_within_cap() { + let settings = RingHashSettings::validate(lb_config(cluster::RingHashLbConfig { + minimum_ring_size: Some(ring_size(2048)), + maximum_ring_size: Some(ring_size(3000)), + ..Default::default() + })) + .unwrap(); + assert_eq!( + settings, + RingHashSettings { + min_ring_size: 2048, + max_ring_size: 3000, + } + ); + } + + #[test] + fn ring_hash_sizes_clamped_to_cap() { + // Sizes within the 8M ceiling but above the local cap clamp to 4096. + let settings = RingHashSettings::validate(lb_config(cluster::RingHashLbConfig { + minimum_ring_size: Some(ring_size(100_000)), + maximum_ring_size: Some(ring_size(100_000)), + ..Default::default() + })) + .unwrap(); + assert_eq!( + settings, + RingHashSettings { + min_ring_size: 4096, + max_ring_size: 4096, + } + ); + } + + #[test] + fn ring_hash_rejects_non_xx_hash() { + let err = RingHashSettings::validate(lb_config(cluster::RingHashLbConfig { + hash_function: cluster::ring_hash_lb_config::HashFunction::MurmurHash2 as i32, + ..Default::default() + })) + .unwrap_err(); + assert!(err.to_string().contains("hash function")); + } + + #[test] + fn ring_hash_rejects_size_above_ceiling() { + let err = RingHashSettings::validate(lb_config(cluster::RingHashLbConfig { + maximum_ring_size: Some(ring_size(8 * 1024 * 1024 + 1)), + ..Default::default() + })) + .unwrap_err(); + assert!(err.to_string().contains("exceeds the maximum")); + } + + #[test] + fn ring_hash_rejects_min_greater_than_max() { + let err = RingHashSettings::validate(lb_config(cluster::RingHashLbConfig { + minimum_ring_size: Some(ring_size(3000)), + maximum_ring_size: Some(ring_size(2000)), + ..Default::default() + })) + .unwrap_err(); + assert!(err.to_string().contains("greater than max_ring_size")); + } +} diff --git a/tonic-xds/src/xds/resource/mod.rs b/tonic-xds/src/xds/resource/mod.rs index e3e90daea..daf9f5627 100644 --- a/tonic-xds/src/xds/resource/mod.rs +++ b/tonic-xds/src/xds/resource/mod.rs @@ -13,6 +13,7 @@ pub(crate) mod cluster; pub(crate) mod endpoints; pub(crate) mod hash_policy; +pub(crate) mod lb_policy; pub(crate) mod listener; pub(crate) mod outlier_detection; pub(crate) mod route_config; From 1af9770b6517851a3736ce2e4085508922b6a1e3 Mon Sep 17 00:00:00 2001 From: Madhurish Nitish Gupta Date: Wed, 24 Jun 2026 14:25:58 -0700 Subject: [PATCH 3/5] refactor(tonic-xds): hoist HashFunction import to module top --- tonic-xds/src/xds/resource/lb_policy.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tonic-xds/src/xds/resource/lb_policy.rs b/tonic-xds/src/xds/resource/lb_policy.rs index e11012fcc..dae2d5333 100644 --- a/tonic-xds/src/xds/resource/lb_policy.rs +++ b/tonic-xds/src/xds/resource/lb_policy.rs @@ -1,6 +1,8 @@ //! Cluster load-balancing policy (CDS) and ring-hash config validation. -use envoy_types::pb::envoy::config::cluster::v3::cluster; +use envoy_types::pb::envoy::config::cluster::v3::cluster::{ + self, ring_hash_lb_config::HashFunction, +}; use xds_client::Error; /// Load balancing policies. @@ -35,8 +37,6 @@ impl RingHashSettings { /// defaults (1024 / 4096); the resolved bounds are then clamped to the /// local cap. pub(crate) fn validate(lb_config: Option) -> xds_client::Result { - use cluster::ring_hash_lb_config::HashFunction; - let (min_field, max_field, hash_function) = match lb_config { Some(cluster::LbConfig::RingHashLbConfig(c)) => { (c.minimum_ring_size, c.maximum_ring_size, c.hash_function) From e46263dd219068e1ca5326177c20819c846ae4ff Mon Sep 17 00:00:00 2001 From: Madhurish Nitish Gupta Date: Wed, 24 Jun 2026 14:30:21 -0700 Subject: [PATCH 4/5] fix(tonic-xds): default unset ring max to 8M before clamping Defaulting an unset maximum_ring_size directly to the local cap made a config with min_ring_size in (4096, 8M] and an unset max get NACKed (min > max), which grpc-go accepts: its CDS layer defaults unset max to 8M, so min <= max holds and both are clamped to the cap later. Restore that two-step (default 8M, then clamp to 4096) so the min > max check matches grpc-go. --- tonic-xds/src/xds/resource/lb_policy.rs | 32 +++++++++++++++++++++---- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/tonic-xds/src/xds/resource/lb_policy.rs b/tonic-xds/src/xds/resource/lb_policy.rs index dae2d5333..0d5bb9888 100644 --- a/tonic-xds/src/xds/resource/lb_policy.rs +++ b/tonic-xds/src/xds/resource/lb_policy.rs @@ -22,9 +22,13 @@ pub(crate) struct RingHashSettings { } /// gRFC A42 ring-hash sizing. `minimum_ring_size` defaults to 1024 and -/// `maximum_ring_size` to the local cap of 4096 when unset; both are clamped to -/// that cap, and any configured value above the 8M ceiling is rejected. +/// `maximum_ring_size` to the xDS default of 8M when unset; both are then +/// clamped to the local cap of 4096, and any configured value above the 8M +/// ceiling is rejected. Defaulting an unset max to 8M (rather than directly to +/// the cap) keeps the `min > max` check from rejecting a `min` in the +/// `(cap, ceiling]` range when `max` is unset. pub(crate) const RING_HASH_DEFAULT_MIN_SIZE: u64 = 1024; +pub(crate) const RING_HASH_DEFAULT_MAX_SIZE: u64 = 8 * 1024 * 1024; pub(crate) const RING_HASH_SIZE_CAP: u64 = 4096; pub(crate) const RING_HASH_SIZE_CEILING: u64 = 8 * 1024 * 1024; @@ -34,8 +38,8 @@ impl RingHashSettings { /// /// Rejects a `hash_function` other than `XX_HASH`, any ring size above the /// 8M ceiling, and `min_ring_size > max_ring_size`. Unset sizes take the - /// defaults (1024 / 4096); the resolved bounds are then clamped to the - /// local cap. + /// defaults (1024 / 8M); the resolved bounds are then clamped to the local + /// cap. pub(crate) fn validate(lb_config: Option) -> xds_client::Result { let (min_field, max_field, hash_function) = match lb_config { Some(cluster::LbConfig::RingHashLbConfig(c)) => { @@ -61,7 +65,7 @@ impl RingHashSettings { } let min = min_field.map_or(RING_HASH_DEFAULT_MIN_SIZE, |v| v.value); - let max = max_field.map_or(RING_HASH_SIZE_CAP, |v| v.value); + let max = max_field.map_or(RING_HASH_DEFAULT_MAX_SIZE, |v| v.value); if min > RING_HASH_SIZE_CEILING || max > RING_HASH_SIZE_CEILING { return Err(Error::Validation(format!( "ring_hash ring size exceeds the maximum of {RING_HASH_SIZE_CEILING} \ @@ -174,4 +178,22 @@ mod tests { .unwrap_err(); assert!(err.to_string().contains("greater than max_ring_size")); } + + #[test] + fn ring_hash_min_above_cap_with_unset_max_is_accepted() { + // min in (cap, ceiling] with max unset must not NACK: max defaults to + // 8M, so min <= max, and both then clamp to the cap. + let settings = RingHashSettings::validate(lb_config(cluster::RingHashLbConfig { + minimum_ring_size: Some(ring_size(5000)), + ..Default::default() + })) + .unwrap(); + assert_eq!( + settings, + RingHashSettings { + min_ring_size: 4096, + max_ring_size: 4096, + } + ); + } } From 422d7c85b1002a673f383822193d4176831256c6 Mon Sep 17 00:00:00 2001 From: Madhurish Nitish Gupta Date: Wed, 24 Jun 2026 15:53:49 -0700 Subject: [PATCH 5/5] refactor(tonic-xds): enforce RingHashSettings invariant via private fields MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make RingHashSettings fields private with getters so the only way to obtain a value is RingHashSettings::validate — invalid (out-of-bounds) values can no longer be constructed directly. Also collapse the duplicate 8M default/ceiling constants into a single RING_HASH_SIZE_CEILING used for both the unset-max default and the ceiling check. --- tonic-xds/src/xds/resource/cluster.rs | 12 +++++------- tonic-xds/src/xds/resource/lb_policy.rs | 24 +++++++++++++++++------- 2 files changed, 22 insertions(+), 14 deletions(-) diff --git a/tonic-xds/src/xds/resource/cluster.rs b/tonic-xds/src/xds/resource/cluster.rs index 345f0dc90..2f1c5a494 100644 --- a/tonic-xds/src/xds/resource/cluster.rs +++ b/tonic-xds/src/xds/resource/cluster.rs @@ -166,13 +166,11 @@ mod tests { ..Default::default() }; let validated = ClusterResource::validate(cluster).unwrap(); - assert_eq!( - validated.lb_policy, - LbPolicy::RingHash(RingHashSettings { - min_ring_size: 1024, - max_ring_size: 4096, - }) - ); + let LbPolicy::RingHash(settings) = validated.lb_policy else { + panic!("expected RingHash, got {:?}", validated.lb_policy); + }; + assert_eq!(settings.min_ring_size(), 1024); + assert_eq!(settings.max_ring_size(), 4096); } #[test] diff --git a/tonic-xds/src/xds/resource/lb_policy.rs b/tonic-xds/src/xds/resource/lb_policy.rs index 0d5bb9888..abe58fe82 100644 --- a/tonic-xds/src/xds/resource/lb_policy.rs +++ b/tonic-xds/src/xds/resource/lb_policy.rs @@ -15,24 +15,34 @@ pub(crate) enum LbPolicy { } /// Validated gRFC A42 ring-hash sizing parsed from `ring_hash_lb_config`. +/// +/// Fields are private so a `RingHashSettings` can only be obtained from +/// [`RingHashSettings::validate`] — i.e. every value is already within bounds. #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub(crate) struct RingHashSettings { - pub min_ring_size: u64, - pub max_ring_size: u64, + min_ring_size: u64, + max_ring_size: u64, } /// gRFC A42 ring-hash sizing. `minimum_ring_size` defaults to 1024 and -/// `maximum_ring_size` to the xDS default of 8M when unset; both are then -/// clamped to the local cap of 4096, and any configured value above the 8M -/// ceiling is rejected. Defaulting an unset max to 8M (rather than directly to +/// `maximum_ring_size` to the 8M ceiling when unset; both are then clamped to +/// the local cap of 4096, and any configured value above the ceiling is +/// rejected. Defaulting an unset max to the ceiling (rather than directly to /// the cap) keeps the `min > max` check from rejecting a `min` in the /// `(cap, ceiling]` range when `max` is unset. pub(crate) const RING_HASH_DEFAULT_MIN_SIZE: u64 = 1024; -pub(crate) const RING_HASH_DEFAULT_MAX_SIZE: u64 = 8 * 1024 * 1024; pub(crate) const RING_HASH_SIZE_CAP: u64 = 4096; pub(crate) const RING_HASH_SIZE_CEILING: u64 = 8 * 1024 * 1024; impl RingHashSettings { + pub(crate) fn min_ring_size(&self) -> u64 { + self.min_ring_size + } + + pub(crate) fn max_ring_size(&self) -> u64 { + self.max_ring_size + } + /// Validate a Cluster's `lb_config` oneof as `ring_hash_lb_config` (gRFC /// A42). /// @@ -65,7 +75,7 @@ impl RingHashSettings { } let min = min_field.map_or(RING_HASH_DEFAULT_MIN_SIZE, |v| v.value); - let max = max_field.map_or(RING_HASH_DEFAULT_MAX_SIZE, |v| v.value); + let max = max_field.map_or(RING_HASH_SIZE_CEILING, |v| v.value); if min > RING_HASH_SIZE_CEILING || max > RING_HASH_SIZE_CEILING { return Err(Error::Validation(format!( "ring_hash ring size exceeds the maximum of {RING_HASH_SIZE_CEILING} \