From f9e19055d2bbcd3775dc0ff6e8c0773d464f3f60 Mon Sep 17 00:00:00 2001 From: Yujong Lee Date: Thu, 5 Nov 2026 12:42:13 +0900 Subject: [PATCH] refactor(redis-resolve): name the crate error Error and keep only resolver failures in it RedisError becomes Error, the one top-level error the rust-error skill asks of each crate, and gateway nests it as litellm_redis_resolve::Error. NotPlainData moves to gateway as Error::RedisNotPlainData, since only the gateway merge turns a YAML value into JSON. Unenforceable now carries the check a Reject row asks for, so its message is not hard-coded to OCSP --- litellm-rust/crates/gateway/src/error.rs | 4 +- .../crates/gateway/src/redis/configured.rs | 13 +++-- .../crates/gateway/src/redis/layers.rs | 12 ++--- litellm-rust/crates/gateway/src/redis/mod.rs | 11 ++--- .../crates/gateway/src/redis/surface_tests.rs | 48 +++++++++++-------- litellm-rust/crates/redis-resolve/AGENTS.md | 12 ++--- litellm-rust/crates/redis-resolve/src/auth.rs | 14 +++--- .../crates/redis-resolve/src/endpoint.rs | 22 ++++----- .../crates/redis-resolve/src/error.rs | 11 +++-- litellm-rust/crates/redis-resolve/src/keys.rs | 19 +++++--- litellm-rust/crates/redis-resolve/src/lib.rs | 9 ++-- .../crates/redis-resolve/src/options.rs | 29 +++++------ litellm-rust/crates/redis-resolve/src/tls.rs | 8 ++-- .../crates/redis-resolve/tests/catalog.rs | 6 +-- .../crates/redis-resolve/tests/interpret.rs | 8 ++-- 15 files changed, 121 insertions(+), 105 deletions(-) diff --git a/litellm-rust/crates/gateway/src/error.rs b/litellm-rust/crates/gateway/src/error.rs index 73924468785..c267e79353d 100644 --- a/litellm-rust/crates/gateway/src/error.rs +++ b/litellm-rust/crates/gateway/src/error.rs @@ -13,7 +13,9 @@ pub enum Error { #[error(transparent)] Config(#[from] litellm_config::Error), #[error(transparent)] - Redis(#[from] litellm_redis_resolve::RedisError), + Redis(#[from] litellm_redis_resolve::Error), + #[error("Redis setting {setting} is not plain data")] + RedisNotPlainData { setting: String }, #[error(transparent)] RedisConnect(#[from] litellm_redis_connect::Error), #[error("MCP requires working gateway authentication")] diff --git a/litellm-rust/crates/gateway/src/redis/configured.rs b/litellm-rust/crates/gateway/src/redis/configured.rs index c6fafbf6cba..c4570c16e00 100644 --- a/litellm-rust/crates/gateway/src/redis/configured.rs +++ b/litellm-rust/crates/gateway/src/redis/configured.rs @@ -2,11 +2,12 @@ use std::collections::BTreeMap; use litellm_config::{RedisSettings, Value}; use litellm_core_utils::settings::Lookup; -use litellm_redis_resolve::{Expected, Kwargs, RedisError}; +use litellm_redis_resolve::{Expected, Kwargs}; use litellm_redis_types::{Auth, AzureAdAuth, ProviderHandle, Resolution}; use veil::Redact; use super::{layers, secrets::SecretSnapshot}; +use crate::Error; const CACHE_ONLY: &[&str] = &[ "type", @@ -44,12 +45,14 @@ pub(super) enum Nulls { impl ConfiguredRedis { /// `general_settings.coordination_redis`, which the proxy validates as a pydantic model first. /// An `os.environ/` port is only known after merging, where it is read like any other port. - pub(crate) fn coordination(settings: &RedisSettings) -> Result { + pub(crate) fn coordination( + settings: &RedisSettings, + ) -> Result { if let Some(port) = settings.fields().get("port").and_then(text) && !port.starts_with("os.environ/") && port.parse::().is_err() { - return Err(RedisError::Invalid { + return Err(litellm_redis_resolve::Error::Invalid { setting: "port".to_owned(), expected: Expected::Integer, }); @@ -113,7 +116,7 @@ impl ConfiguredRedis { /// `_merge_redis_kwargs` in `litellm/_redis.py`: the kwargs over their `REDIS_*` values with every /// `get_secret` fallback filled. - pub(crate) fn merged(&self, secrets: &SecretSnapshot) -> Result { + pub(crate) fn merged(&self, secrets: &SecretSnapshot) -> Result { layers::merged(self, secrets) } @@ -122,7 +125,7 @@ impl ConfiguredRedis { &self, secrets: &SecretSnapshot, environment: &dyn Lookup, - ) -> Result { + ) -> Result { let resolution = litellm_redis_resolve::interpret(&self.merged(secrets)?, self.credential_provider)?; Ok(match resolution.auth { diff --git a/litellm-rust/crates/gateway/src/redis/layers.rs b/litellm-rust/crates/gateway/src/redis/layers.rs index d41826eb634..1cf68b2cd4f 100644 --- a/litellm-rust/crates/gateway/src/redis/layers.rs +++ b/litellm-rust/crates/gateway/src/redis/layers.rs @@ -1,8 +1,9 @@ use litellm_config::Value; use litellm_core_utils::settings::Lookup; -use litellm_redis_resolve::{Entry, Kwargs, RedisError, SETTINGS, is_present, setting}; +use litellm_redis_resolve::{Entry, Kwargs, SETTINGS, is_present, setting}; use super::configured::{ConfiguredRedis, Nulls}; +use crate::Error; const NAMED_BY_CACHE: &[&str] = &[ "host", @@ -47,17 +48,14 @@ const AWS: &[(&str, Read)] = &[("aws_iam_region", Read::SecretStr)]; /// The configured settings over their `REDIS_*` values, `{**environment_kwargs, **env_overrides}` /// in `_merge_redis_kwargs`, with the `get_secret` fallbacks it reads filled in. A null left in the result is /// an explicit `None` that hid its environment value. -pub(super) fn merged( - configured: &ConfiguredRedis, - lookup: &dyn Lookup, -) -> Result { +pub(super) fn merged(configured: &ConfiguredRedis, lookup: &dyn Lookup) -> Result { let block: Kwargs = configured .fields .iter() .map(|(key, value)| (key, environment_reference(value, lookup))) .filter(|(key, value)| !value.is_null() || keeps_null(configured.nulls, key)) .map(|(key, value)| { - let value = serde_json::to_value(&value).map_err(|_| RedisError::NotPlainData { + let value = serde_json::to_value(&value).map_err(|_| Error::RedisNotPlainData { setting: key.clone(), })?; let entry = Entry { @@ -66,7 +64,7 @@ pub(super) fn merged( }; Ok((key.clone(), entry)) }) - .collect::>()?; + .collect::>()?; let names_a_target = ["host", "startup_nodes", "sentinel_nodes"] .iter() .any(|key| is_set(&block, key)); diff --git a/litellm-rust/crates/gateway/src/redis/mod.rs b/litellm-rust/crates/gateway/src/redis/mod.rs index 2d9266bd5d2..f00fb3bcf5e 100644 --- a/litellm-rust/crates/gateway/src/redis/mod.rs +++ b/litellm-rust/crates/gateway/src/redis/mod.rs @@ -4,7 +4,7 @@ use litellm_circuit_breaker::{Breaker, Config as BreakerConfig, State}; use litellm_config::{CacheSettings, Config, RedisSettings, Value}; use litellm_core_utils::settings::Lookup; use litellm_redis_client::RedisClient; -use litellm_redis_resolve::{Expected, RedisError}; +use litellm_redis_resolve::Expected; use litellm_redis_types::Resolution; use crate::Error; @@ -113,7 +113,7 @@ fn redis_cache_params(config: &Config) -> Option<&RedisSettings> { } } -fn resolve(configured: &ConfiguredRedis, lookup: &dyn Lookup) -> Result { +fn resolve(configured: &ConfiguredRedis, lookup: &dyn Lookup) -> Result { let secrets = SecretSnapshot::read(configured.secret_names(), lookup); let resolution = configured.resolve(&secrets, lookup)?; for ignored in &resolution.ignored { @@ -246,11 +246,11 @@ fn environment_value( name: &str, expected: Expected, parse: impl Fn(&str) -> Option, -) -> Result, RedisError> { +) -> Result, litellm_redis_resolve::Error> { lookup .get(name) .map(|value| { - parse(&value).ok_or_else(|| RedisError::Invalid { + parse(&value).ok_or_else(|| litellm_redis_resolve::Error::Invalid { setting: name.to_owned(), expected, }) @@ -264,7 +264,6 @@ mod tests { use litellm_config::Config; use litellm_redis_client::{Standalone, StaticAuth}; - use litellm_redis_resolve::RedisError; use litellm_redis_testing::RedisServer; use litellm_redis_types::{Auth, HealthCheck, Passthrough}; use rstest::rstest; @@ -472,7 +471,7 @@ mod tests { .unwrap_err(); assert!( - matches!(error, Error::Redis(RedisError::Invalid { setting, .. }) if setting == expected) + matches!(error, Error::Redis(litellm_redis_resolve::Error::Invalid { setting, .. }) if setting == expected) ); } diff --git a/litellm-rust/crates/gateway/src/redis/surface_tests.rs b/litellm-rust/crates/gateway/src/redis/surface_tests.rs index 6c4d5118c8c..5e8e2c43841 100644 --- a/litellm-rust/crates/gateway/src/redis/surface_tests.rs +++ b/litellm-rust/crates/gateway/src/redis/surface_tests.rs @@ -1,7 +1,7 @@ use std::collections::{BTreeMap, HashMap}; use litellm_config::{CacheSettings, Config, Value}; -use litellm_redis_resolve::{Expected, RedisError}; +use litellm_redis_resolve::Expected; use litellm_redis_types::{ Auth, AwsIamAuth, AzureAdAuth, Node, ProviderHandle, Resolution, StaticAuth, Target, TlsOptions, }; @@ -10,11 +10,12 @@ use rstest::rstest; use serde_json::json; use super::{configured::ConfiguredRedis, secrets::SecretSnapshot}; +use crate::Error; fn resolution( configured: &ConfiguredRedis, environment: &HashMap, -) -> Result { +) -> Result { let lookup = |name: &str| environment.get(name).cloned(); configured.resolve( &SecretSnapshot::read(configured.secret_names(), &lookup), @@ -40,14 +41,11 @@ fn redis_cache_kwargs(kwargs: serde_json::Value) -> ConfiguredRedis { fn resolve_kwargs( kwargs: serde_json::Value, environment: &HashMap, -) -> Result { +) -> Result { resolution(&redis_cache_kwargs(kwargs), environment) } -fn coordination_resolution( - yaml: &str, - environment: &[(&str, &str)], -) -> Result { +fn coordination_resolution(yaml: &str, environment: &[(&str, &str)]) -> Result { let environment: HashMap = environment .iter() .map(|(key, value)| ((*key).to_owned(), (*value).to_owned())) @@ -61,15 +59,15 @@ fn coordination_resolution( resolution(&configured, &environment) } -fn resolve(yaml: &str, environment: &[(&str, &str)]) -> Result { +fn resolve(yaml: &str, environment: &[(&str, &str)]) -> Result { coordination_resolution(yaml, environment) } -fn resolve_cache(yaml: &str) -> Result { +fn resolve_cache(yaml: &str) -> Result { resolve_cache_in(yaml, &[]) } -fn resolve_cache_in(yaml: &str, environment: &[(&str, &str)]) -> Result { +fn resolve_cache_in(yaml: &str, environment: &[(&str, &str)]) -> Result { let lookup = |name: &str| { environment .iter() @@ -209,7 +207,7 @@ fn an_ocsp_check_fails_instead_of_connecting_without_it( let error = coordination_resolution(yaml, environment).unwrap_err(); assert!( - matches!(&error, RedisError::Unenforceable { setting: named } if named == setting), + matches!(&error, Error::Redis(litellm_redis_resolve::Error::Unenforceable { setting: named, .. }) if named == setting), "{error:?}" ); } @@ -325,7 +323,10 @@ fn a_credential_provider_without_a_host_handle_fails( let error = resolve(yaml, environment).unwrap_err(); assert!( - matches!(error, RedisError::ProviderWithoutHandle), + matches!( + error, + Error::Redis(litellm_redis_resolve::Error::ProviderWithoutHandle) + ), "{error}" ); } @@ -393,7 +394,7 @@ fn an_unknown_cert_requirement_fails() { assert!(matches!( error, - RedisError::Invalid { setting, expected: Expected::CertRequirement } if setting == "ssl_cert_reqs" + Error::Redis(litellm_redis_resolve::Error::Invalid { setting, expected: Expected::CertRequirement }) if setting == "ssl_cert_reqs" )); } @@ -467,7 +468,13 @@ fn sentinels_without_a_service_name_fail() { ) .unwrap_err(); - assert!(matches!(error, RedisError::NoSentinelService), "{error:?}"); + assert!( + matches!( + error, + Error::Redis(litellm_redis_resolve::Error::NoSentinelService) + ), + "{error:?}" + ); } #[rstest] @@ -489,7 +496,7 @@ fn a_host_or_port_cannot_reach_sentinel( let error = resolve(yaml, environment).unwrap_err(); assert!( - matches!(&error, RedisError::SentinelWithHost { setting } if setting == source), + matches!(&error, Error::Redis(litellm_redis_resolve::Error::SentinelWithHost { setting }) if setting == source), "{error:?}" ); } @@ -513,7 +520,7 @@ fn invalid_sentinel_nodes_name_their_source( let error = resolve(yaml, environment).unwrap_err(); assert!( - matches!(&error, RedisError::Invalid { setting, expected: Expected::SentinelNodes } if setting == source), + matches!(&error, Error::Redis(litellm_redis_resolve::Error::Invalid { setting, expected: Expected::SentinelNodes }) if setting == source), "{error:?}" ); } @@ -530,7 +537,7 @@ fn invalid_values_name_their_source( let error = resolve(yaml, environment).unwrap_err(); assert!( - matches!(&error, RedisError::Invalid { setting, .. } if setting == source), + matches!(&error, Error::Redis(litellm_redis_resolve::Error::Invalid { setting, .. }) if setting == source), "{error:?}" ); } @@ -572,7 +579,10 @@ fn connect_timeout_is_resolved_separately( fn a_block_without_a_usable_target_fails(#[case] yaml: &str, #[case] environment: &[(&str, &str)]) { let error = resolve(yaml, environment).unwrap_err(); - assert!(matches!(error, RedisError::NoTarget), "{error:?}"); + assert!( + matches!(error, Error::Redis(litellm_redis_resolve::Error::NoTarget)), + "{error:?}" + ); } #[rstest] @@ -642,7 +652,7 @@ fn rejects_bad_socket_timeout(#[case] timeout: &str) { assert!(matches!( error, - RedisError::Invalid { setting, expected: Expected::Seconds } if setting == "socket_timeout" + Error::Redis(litellm_redis_resolve::Error::Invalid { setting, expected: Expected::Seconds }) if setting == "socket_timeout" )); } diff --git a/litellm-rust/crates/redis-resolve/AGENTS.md b/litellm-rust/crates/redis-resolve/AGENTS.md index 078d168d2ce..91d0d22d1ad 100644 --- a/litellm-rust/crates/redis-resolve/AGENTS.md +++ b/litellm-rust/crates/redis-resolve/AGENTS.md @@ -1,7 +1,7 @@ ## Interpreting merged kwargs - The boundary is merged redis-py kwargs in, `litellm_redis_types::Resolution` out. `interpret` takes the dict a host already merged the way `_merge_redis_kwargs` in `litellm/_redis.py` does, with its `REDIS_*` values and `get_secret` fallbacks filled, and stands in for redis-py's constructors and `from_url`. It never reads the environment or a secret, so both hosts share it. Every `Entry` keeps the source a warning or error names, which `kwargs` sets to the kwarg name and the gateway merge sets to `REDIS_` or the fallback name -- The `Resolution` holds the target, the auth and the timeouts Rust models, every other plain setting verbatim in `passthrough`, the names of object-valued settings in `host_objects`, and every setting the Rust client ignores as an `Ignored` with its source and consequence, which the host logs. A host passes its `credential_provider` object as a `ProviderHandle`, and a `credential_provider` key without one is `RedisError::ProviderWithoutHandle`. Azure's username is read from `os.environ` by Python, so `interpret` leaves `AzureAdAuth::username` unset and the host fills it in +- The `Resolution` holds the target, the auth and the timeouts Rust models, every other plain setting verbatim in `passthrough`, the names of object-valued settings in `host_objects`, and every setting the Rust client ignores as an `Ignored` with its source and consequence, which the host logs. A host passes its `credential_provider` object as a `ProviderHandle`, and a `credential_provider` key without one is `Error::ProviderWithoutHandle`. Azure's username is read from `os.environ` by Python, so `interpret` leaves `AzureAdAuth::username` unset and the host fills it in - `options.rs` reads the kwargs into the typed `Options` and splits the unmodeled settings into passthrough and host objects and collects ignored ones. `endpoint.rs`, `auth.rs` and `tls.rs` pick the target, the credentials and TLS from `Options` alone - Rust never introspects redis-py. Every Redis kwarg redis-py or `litellm/_redis.py` accepts is one row in `SETTINGS` in `src/keys.rs`, and that row is the whole answer for the kwarg: its `Handling` (`Read`, `Warn` with the consequence the warning names, or `Reject`), its `Shape` (plain data lands in `passthrough`, a Python object in `host_objects`), whether a url query carries it (`FromUrl`), whether `_get_redis_env_kwarg_mapping` reads it as `REDIS_`, and the names `_merge_redis_kwargs` falls back to. The last two columns are only read by the gateway merge, and they stay on the row so there is one table. A `Read` row is a field of `Options`, which an inline test pins. A kwarg with no row is not a Redis setting, so it passes through without a warning - `redis-py-kwargs.json` holds the Python facts each row answers: which redis-py versions name the kwarg in which signature, and whether Python reads its `REDIS_*` name under the locked redis-py. `test_redis_py_kwargs_catalog_matches_the_installed_redis_py` in `tests/unit/test_redis.py` checks it against each version the `redis-compat` CI matrix installs, and `tests/catalog.rs` fails until `SETTINGS` has exactly one row per kwarg in it with the same `environment`. A redis-py bump that adds a kwarg therefore cannot land without a row deciding how Rust handles it @@ -10,11 +10,11 @@ - TLS options and `db` are only read for the endpoint that uses them, so a bad `ssl_cert_reqs` never fails a url target. A cluster with a non-zero `db`, and Sentinel with a `host` or `port`, are errors because redis-py raises building those clients - An empty value (`""`, `[]`, `{}`) is the same as an unset one, like Python's `x or ...` - Precedence matches Python. The target is cluster, then URL, then Sentinel (`sentinel_nodes` plus `service_name`), then host. Auth is a `credential_provider` handle, then GCP, then Azure, then AWS, and the first one wins with a warning -- Numbers are read with `litellm_python_compat::number`, so whitespace and digit underscores parse like Python's `int()` and `float()`. A value they cannot parse (`port: abc`, `socket_timeout: slow`) is dropped with a warning and the default is used, like `_coerce_redis_kwargs_types`. A number that parses but is out of range (negative seconds, a port above 65535) fails with `RedisError::Invalid` -- Flags (`ssl`, `ssl_check_hostname`, `aws_iam_auth`, `aws_iam_serverless`) are read one way on every surface. `true`, `1` and `yes` are true and `false`, `0`, `no`, `off`, `f` and `n` are false, the text redis-py's `_str_to_bool` and the proxy's pydantic models agree on. Any other text is `RedisError::Invalid`, so a TLS setting is never guessed -- The resolver fails on a setting it cannot honor only when that setting is a security check. A `Warn` row is dropped and returned as an `Ignored` naming its source (`ssl_ciphers` or `REDIS_SSL_CIPHERS`) and the consequence on its row, because a Python deployment that worked must keep working when its stores move to Rust. Only a value that is malformed (`RedisError::Invalid`) or a target that is missing fails. Promoting a `Warn` row to `Read` is then a pure improvement with no behavior cliff +- Numbers are read with `litellm_python_compat::number`, so whitespace and digit underscores parse like Python's `int()` and `float()`. A value they cannot parse (`port: abc`, `socket_timeout: slow`) is dropped with a warning and the default is used, like `_coerce_redis_kwargs_types`. A number that parses but is out of range (negative seconds, a port above 65535) fails with `Error::Invalid` +- Flags (`ssl`, `ssl_check_hostname`, `aws_iam_auth`, `aws_iam_serverless`) are read one way on every surface. `true`, `1` and `yes` are true and `false`, `0`, `no`, `off`, `f` and `n` are false, the text redis-py's `_str_to_bool` and the proxy's pydantic models agree on. Any other text is `Error::Invalid`, so a TLS setting is never guessed +- The resolver fails on a setting it cannot honor only when that setting is a security check. A `Warn` row is dropped and returned as an `Ignored` naming its source (`ssl_ciphers` or `REDIS_SSL_CIPHERS`) and the consequence on its row, because a Python deployment that worked must keep working when its stores move to Rust. Only a value that is malformed (`Error::Invalid`) or a target that is missing fails. Promoting a `Warn` row to `Read` is then a pure improvement with no behavior cliff - A `Warn` row with a `met` check is dropped silently when its value asks for nothing more than Rust does, so `ssl_min_version` of TLS 1.2 or lower never warns - - A `Reject` row fails closed. `ssl_validate_ocsp` and `ssl_validate_ocsp_stapled` set to true, from the kwargs, `REDIS_*` or a url query, are `RedisError::Unenforceable`, because rustls has no client-side OCSP and connecting without the check would silently drop it. redis-py 5.3.1's async client refuses both with a `TypeError`, so this matches the proxy's path. Set to false they ask for nothing and are dropped without a warning + - A `Reject` row fails closed. `ssl_validate_ocsp` and `ssl_validate_ocsp_stapled` set to true, from the kwargs, `REDIS_*` or a url query, are `Error::Unenforceable`, because rustls has no client-side OCSP and connecting without the check would silently drop it. redis-py 5.3.1's async client refuses both with a `TypeError`, so this matches the proxy's path. Set to false they ask for nothing and are dropped without a warning - `ssl_ca_data` and `ssl_ca_path` are read into `TlsOptions` and trusted next to `ssl_ca_certs`. `ssl_password` is read into `TlsOptions::key_password`, which `redis-client` uses to decrypt a PKCS#8 key like the sync client's `load_cert_chain(password=...)`. redis-py 5.3.1's async client refuses it, so this is a superset of the path the proxy uses, like `ssl_ca_path` - `unix_socket_path` resolves to `Target::Unix`, which like redis-py's `UnixDomainSocketConnection` replaces host, port and TLS. Python still requires `host` next to it, so Rust does too - A url's `?db=`, `?username=` and `?password=` are read like `parse_url` reads them, so `?db=` wins over the path and the userinfo wins over `?username=` and `?password=`. Any other query parameter, including one with no row, is ignored naming `url ?`, because redis-py turns each one into a client kwarg. Which ones a url carries is the `FromUrl` column @@ -22,5 +22,5 @@ - `health_check_interval` is read as an `int` of seconds like `_coerce_redis_kwargs_types` into `HealthCheck`. Zero is `Disabled` and stays off on every target, a negative interval checks before every command like redis-py, and only `Unset` takes a default. `redis-connect` gives an unset cluster Python's 25s default and `redis-client` pings every node before the next command once the client sat idle that long, outside the breaker and the call events - Cases where Rust and Python disagree on purpose are cases in both contract tests with a different expected outcome on each side - `ssl_ca_path`: redis-py 5.3.1's async `Redis` has no such parameter and raises `TypeError` building the client, while Rust trusts the directory like `load_verify_locations(capath=...)` - - `on`, `t` and `y` for a flag are `RedisError::Invalid` on every surface. pydantic reads them as true for `coordination_redis` and `_str_to_bool` reads them as false everywhere else, so Rust refuses to pick one. Other unrecognized text is false in Python and an error here + - `on`, `t` and `y` for a flag are `Error::Invalid` on every surface. pydantic reads them as true for `coordination_redis` and `_str_to_bool` reads them as false everywhere else, so Rust refuses to pick one. Other unrecognized text is false in Python and an error here - Empty `sentinel_nodes` with no `REDIS_SENTINEL_NODES` is no target at all, so it fails as "either host or url must be specified". redis-py keeps the empty list and fails building the Sentinel client diff --git a/litellm-rust/crates/redis-resolve/src/auth.rs b/litellm-rust/crates/redis-resolve/src/auth.rs index 444de193d42..af740765216 100644 --- a/litellm-rust/crates/redis-resolve/src/auth.rs +++ b/litellm-rust/crates/redis-resolve/src/auth.rs @@ -1,17 +1,17 @@ use litellm_redis_types::{Auth, AwsIamAuth, AzureAdAuth, ProviderHandle, StaticAuth}; use crate::{ - RedisError, + Error, options::{self, AwsOptions, Options, flag, secret, text}, }; pub(super) fn redis_auth( options: &Options, provider: Option, -) -> Result { +) -> Result { match (provider, &options.credential_provider) { (Some(handle), _) => return Ok(Auth::Provider(handle)), - (None, Some(_)) => return Err(RedisError::ProviderWithoutHandle), + (None, Some(_)) => return Err(Error::ProviderWithoutHandle), (None, None) => {} } let gcp = text(options.gcp.service_account.as_ref()); @@ -54,7 +54,7 @@ pub(super) fn redis_auth( } // _uses_tls: a url's scheme decides unless cluster nodes are set, otherwise the ssl kwarg does -fn uses_tls(options: &Options) -> Result { +fn uses_tls(options: &Options) -> Result { match (&options.startup_nodes, text(options.url.as_ref())) { (None, Some(url)) => Ok(url .split_once("://") @@ -63,9 +63,9 @@ fn uses_tls(options: &Options) -> Result { } } -fn aws_iam(aws: &AwsOptions, uses_tls: bool) -> Result { +fn aws_iam(aws: &AwsOptions, uses_tls: bool) -> Result { if !uses_tls { - return Err(RedisError::AwsIamWithoutTls); + return Err(Error::AwsIamWithoutTls); } match ( text(aws.user_name.as_ref()), @@ -78,7 +78,7 @@ fn aws_iam(aws: &AwsOptions, uses_tls: bool) -> Result { region, serverless: options::flag(aws.serverless.as_ref())?, }), - (user_name, cache_name, region) => Err(RedisError::AwsIamIncomplete { + (user_name, cache_name, region) => Err(Error::AwsIamIncomplete { missing: [ ("aws_iam_user_name", user_name.is_none()), ("aws_iam_cache_name", cache_name.is_none()), diff --git a/litellm-rust/crates/redis-resolve/src/endpoint.rs b/litellm-rust/crates/redis-resolve/src/endpoint.rs index 2cdab65dfca..a7e1915f3e4 100644 --- a/litellm-rust/crates/redis-resolve/src/endpoint.rs +++ b/litellm-rust/crates/redis-resolve/src/endpoint.rs @@ -7,14 +7,14 @@ use litellm_secret_value::SecretValue; use serde_json::Value; use crate::{ - Entry, Expected, RedisError, + Entry, Error, Expected, options::{Options, as_text, flag, integer, invalid, secret, text}, tls, }; /// The target, first match wins like `get_redis_client`: cluster, url, Sentinel, then host. A /// url also returns the query parameters it ignores. -pub(super) fn target(options: &Options, auth: &Auth) -> Result<(Target, Vec), RedisError> { +pub(super) fn target(options: &Options, auth: &Auth) -> Result<(Target, Vec), Error> { if let Some(nodes) = cluster_nodes(options.startup_nodes.as_ref())? { reject_cluster_db(options.db.as_ref())?; let cluster = Cluster { @@ -41,7 +41,7 @@ pub(super) fn target(options: &Options, auth: &Auth) -> Result<(Target, Vec Target::Unix(UnixSocket { path: path.into(), @@ -57,7 +57,7 @@ pub(super) fn target(options: &Options, auth: &Auth) -> Result<(Target, Vec Result, RedisError> { +fn tls_for(options: &Options, auth: &Auth) -> Result, Error> { if !flag(options.ssl.as_ref())? { return Ok(None); } @@ -68,19 +68,19 @@ fn tls_for(options: &Options, auth: &Auth) -> Result, RedisEr tls::tls_options(&options.tls, gcp_ca_certs).map(Some) } -fn db(options: &Options) -> Result { +fn db(options: &Options) -> Result { integer(options.db.as_ref()).map(|db| db.unwrap_or(0)) } -fn sentinel(options: &Options, auth: &Auth, sentinels: &Entry) -> Result { +fn sentinel(options: &Options, auth: &Auth, sentinels: &Entry) -> Result { let monitors = sentinel_nodes(&sentinels.value) .ok_or_else(|| invalid(&sentinels.source, Expected::SentinelNodes))?; let service_name = - text(options.sentinel.service_name.as_ref()).ok_or(RedisError::NoSentinelService)?; + text(options.sentinel.service_name.as_ref()).ok_or(Error::NoSentinelService)?; // redis.asyncio.Sentinel passes the connection kwargs to each monitor's Redis(host=..., port=...), so a // host or port setting raises TypeError when litellm builds the client if let Some(entry) = options.host.as_ref().or(options.port.as_ref()) { - return Err(RedisError::SentinelWithHost { + return Err(Error::SentinelWithHost { setting: entry.source.clone(), }); } @@ -107,7 +107,7 @@ fn monitor_auth(options: &Options, auth: &Auth) -> Option { }) } -fn cluster_nodes(entry: Option<&Entry>) -> Result>, RedisError> { +fn cluster_nodes(entry: Option<&Entry>) -> Result>, Error> { let Some(entry) = entry else { return Ok(None); }; @@ -149,7 +149,7 @@ fn node_address(host: Option, port: Option) -> Option { } // redis-py's RedisCluster raises unless db is 0 or None -fn reject_cluster_db(db: Option<&Entry>) -> Result<(), RedisError> { +fn reject_cluster_db(db: Option<&Entry>) -> Result<(), Error> { let Some(entry) = db else { return Ok(()); }; @@ -161,7 +161,7 @@ fn reject_cluster_db(db: Option<&Entry>) -> Result<(), RedisError> { }; match is_zero { true => Ok(()), - false => Err(RedisError::DbInCluster { + false => Err(Error::DbInCluster { setting: entry.source.clone(), }), } diff --git a/litellm-rust/crates/redis-resolve/src/error.rs b/litellm-rust/crates/redis-resolve/src/error.rs index a554ea2eeb8..8a2ca0b38c1 100644 --- a/litellm-rust/crates/redis-resolve/src/error.rs +++ b/litellm-rust/crates/redis-resolve/src/error.rs @@ -1,5 +1,5 @@ #[derive(Debug, thiserror::Error)] -pub enum RedisError { +pub enum Error { #[error("invalid Redis setting {setting}: expected {expected}")] Invalid { setting: String, expected: Expected }, #[error("either host or url must be specified for redis")] @@ -13,11 +13,12 @@ pub enum RedisError { #[error("credential_provider is a host object, and the host passed no handle for it")] ProviderWithoutHandle, #[error( - "{setting} asks for OCSP revocation checks, which rustls cannot perform, so Redis is not connected without them" + "{setting} asks for {check}, which rustls cannot perform, so Redis is not connected without them" )] - Unenforceable { setting: String }, - #[error("Redis setting {setting} is not plain data")] - NotPlainData { setting: String }, + Unenforceable { + setting: String, + check: &'static str, + }, #[error("AWS ElastiCache IAM Redis authentication requires TLS")] AwsIamWithoutTls, #[error("AWS ElastiCache IAM Redis authentication requires: {}", missing.join(", "))] diff --git a/litellm-rust/crates/redis-resolve/src/keys.rs b/litellm-rust/crates/redis-resolve/src/keys.rs index b1d04356aee..bb711f08cae 100644 --- a/litellm-rust/crates/redis-resolve/src/keys.rs +++ b/litellm-rust/crates/redis-resolve/src/keys.rs @@ -13,7 +13,7 @@ pub enum Handling { met: Option bool>, }, /// A security check the Rust client cannot perform, so setting it to true fails instead of connecting without it. - Reject, + Reject { check: &'static str }, } /// What the value is, which decides where an unread setting lands in a `Resolution`. @@ -127,8 +127,8 @@ const fn warn_unless( ) } -const fn reject(kwarg: &'static str) -> Setting { - Setting::new(kwarg, Handling::Reject) +const fn reject(kwarg: &'static str, check: &'static str) -> Setting { + Setting::new(kwarg, Handling::Reject { check }) } const NO_EQUIVALENT: &str = "the Rust client has no equivalent"; @@ -137,6 +137,7 @@ const OWN_CONNECTION: &str = "it tunes redis-py's own connection, and the Rust c const OWN_CLUSTER: &str = "the Rust cluster client discovers, routes and retries on its own"; const MAINTENANCE: &str = "the Rust client does not follow server maintenance notifications"; const RUSTLS: &str = "rustls negotiates TLS 1.2+ with its own ECDHE AEAD suites"; +const REVOCATION: &str = "OCSP revocation checks"; const OCSP: &str = "it only configures ssl_validate_ocsp, which the Rust client refuses when set"; const VERIFY_FLAGS: &str = "rustls verifies certificates with its own fixed rules, so no extra X.509 flag such as CRL checking applies"; @@ -249,8 +250,8 @@ pub const SETTINGS: &[Setting] = &[ warn("ssl_ocsp_context", OCSP).env().object(), warn("ssl_ocsp_expected_cert", OCSP).env(), read("ssl_password").env().in_url(FromUrl::Rediss), - reject("ssl_validate_ocsp").env(), - reject("ssl_validate_ocsp_stapled").env(), + reject("ssl_validate_ocsp", REVOCATION).env(), + reject("ssl_validate_ocsp_stapled", REVOCATION).env(), read("startup_nodes").or_secrets(&["REDIS_CLUSTER_NODES"]), read("unix_socket_path").env(), read("url").env(), @@ -277,8 +278,12 @@ pub fn setting(kwarg: &str) -> Option<&'static Setting> { SETTINGS.iter().find(|setting| setting.kwarg == kwarg) } -pub(crate) fn is_refused_when_set(kwarg: &str) -> bool { - setting(kwarg).is_some_and(|setting| matches!(setting.handling, Handling::Reject)) +/// The check a `Reject` row asks for when it is set to true. +pub(crate) fn refused_check(kwarg: &str) -> Option<&'static str> { + match setting(kwarg)?.handling { + Handling::Reject { check } => Some(check), + Handling::Read | Handling::Warn { .. } => None, + } } /// A warned setting the kwargs set, with what dropping it means. A kwarg with no row is not a Redis setting, so it diff --git a/litellm-rust/crates/redis-resolve/src/lib.rs b/litellm-rust/crates/redis-resolve/src/lib.rs index 0b8fb19b70b..83db0a805ce 100644 --- a/litellm-rust/crates/redis-resolve/src/lib.rs +++ b/litellm-rust/crates/redis-resolve/src/lib.rs @@ -10,7 +10,7 @@ mod keys; mod options; mod tls; -pub use error::{Expected, RedisError}; +pub use error::{Error, Expected}; pub use keys::{FromUrl, Handling, SETTINGS, Setting, Shape, setting}; pub use options::is_present; @@ -41,10 +41,7 @@ pub fn kwargs(fields: impl IntoIterator) -> Kwargs { /// What redis-py's constructors make of kwargs a host already merged the way `_get_redis_client_logic` does. A /// host passes its `credential_provider` object as `provider`. -pub fn interpret( - kwargs: &Kwargs, - provider: Option, -) -> Result { +pub fn interpret(kwargs: &Kwargs, provider: Option) -> Result { let (options, unmodeled) = Options::read(kwargs)?; let auth = auth::redis_auth(&options, provider)?; let (target, url_ignored) = endpoint::target(&options, &auth)?; @@ -70,7 +67,7 @@ pub fn interpret( // redis-py checks health only when the interval is truthy, so zero turns it off and a negative interval checks // before every command -fn health_check(entry: Option<&Entry>) -> Result { +fn health_check(entry: Option<&Entry>) -> Result { Ok(match options::integer::(entry)? { None => HealthCheck::Unset, Some(0) => HealthCheck::Disabled, diff --git a/litellm-rust/crates/redis-resolve/src/options.rs b/litellm-rust/crates/redis-resolve/src/options.rs index 16dced92b5c..41fee1d8ded 100644 --- a/litellm-rust/crates/redis-resolve/src/options.rs +++ b/litellm-rust/crates/redis-resolve/src/options.rs @@ -6,7 +6,7 @@ use litellm_secret_value::SecretValue; use serde_json::Value; use crate::{ - Entry, Expected, Kwargs, RedisError, + Entry, Error, Expected, Kwargs, keys::{self, Shape}, }; @@ -79,7 +79,7 @@ pub(super) struct Unmodeled { } impl Unmodeled { - fn read(kwargs: &Kwargs) -> Result { + fn read(kwargs: &Kwargs) -> Result { refuse_unenforceable(kwargs.iter())?; let unmodeled = || { kwargs @@ -105,7 +105,7 @@ impl Unmodeled { impl Options { /// The typed settings and the ones the resolver leaves to the host. - pub(super) fn read(kwargs: &Kwargs) -> Result<(Self, Unmodeled), RedisError> { + pub(super) fn read(kwargs: &Kwargs) -> Result<(Self, Unmodeled), Error> { let unmodeled = Unmodeled::read(kwargs)?; let options = Self::from_lookup(|key| { kwargs @@ -171,13 +171,14 @@ impl Options { /// A `Reject` row that is set to true asks for a check Rust cannot perform. One set to false asks for nothing. pub(super) fn refuse_unenforceable<'a>( entries: impl IntoIterator, -) -> Result<(), RedisError> { +) -> Result<(), Error> { entries .into_iter() - .filter(|(key, _)| keys::is_refused_when_set(key)) - .try_for_each(|(_, entry)| match flag(Some(entry))? { - true => Err(RedisError::Unenforceable { + .filter_map(|(key, entry)| Some((keys::refused_check(key)?, entry))) + .try_for_each(|(check, entry)| match flag(Some(entry))? { + true => Err(Error::Unenforceable { setting: entry.source.clone(), + check, }), false => Ok(()), }) @@ -194,8 +195,8 @@ pub fn is_present(value: &Value) -> bool { } } -pub(super) fn invalid(setting: &str, expected: Expected) -> RedisError { - RedisError::Invalid { +pub(super) fn invalid(setting: &str, expected: Expected) -> Error { + Error::Invalid { setting: setting.to_owned(), expected, } @@ -221,7 +222,7 @@ pub(super) fn secret(entry: Option<&Entry>) -> Option { /// A flag every Python surface reads the same way. `_str_to_bool` takes only `true`, `1` and `yes` /// as true while the proxy's pydantic models also take `on`, `t` and `y`, so text they disagree on, /// or text neither accepts, is an error instead of a guess. -pub(super) fn flag(entry: Option<&Entry>) -> Result { +pub(super) fn flag(entry: Option<&Entry>) -> Result { let Some(entry) = entry else { return Ok(false); }; @@ -241,20 +242,20 @@ pub(super) fn parsed( entry: Option<&Entry>, expected: Expected, parse: impl Fn(&str) -> Option, -) -> Result, RedisError> { +) -> Result, Error> { entry .and_then(|entry| Some((as_text(&entry.value)?, &entry.source))) .map(|(text, source)| parse(&text).ok_or_else(|| invalid(source, expected))) .transpose() } -pub(super) fn integer>(entry: Option<&Entry>) -> Result, RedisError> { +pub(super) fn integer>(entry: Option<&Entry>) -> Result, Error> { coerced(entry, Expected::Integer, number::int, |number| { T::try_from(number).ok() }) } -pub(super) fn seconds(entry: Option<&Entry>) -> Result, RedisError> { +pub(super) fn seconds(entry: Option<&Entry>) -> Result, Error> { coerced(entry, Expected::Seconds, number::float, |seconds| { Duration::try_from_secs_f64(seconds).ok() }) @@ -265,7 +266,7 @@ fn coerced( expected: Expected, parse: impl Fn(&str) -> Option, convert: impl Fn(N) -> Option, -) -> Result, RedisError> { +) -> Result, Error> { let Some((entry, text)) = entry.and_then(|entry| Some((entry, as_text(&entry.value)?))) else { return Ok(None); }; diff --git a/litellm-rust/crates/redis-resolve/src/tls.rs b/litellm-rust/crates/redis-resolve/src/tls.rs index a55f1be4b88..7c31f8c12aa 100644 --- a/litellm-rust/crates/redis-resolve/src/tls.rs +++ b/litellm-rust/crates/redis-resolve/src/tls.rs @@ -5,7 +5,7 @@ use litellm_secret_value::SecretValue; use serde_json::Value; use crate::{ - Entry, Expected, RedisError, + Entry, Error, Expected, keys::{self, FromUrl}, options::{self, TlsInput, flag, parsed, secret, text}, }; @@ -14,7 +14,7 @@ use crate::{ pub(super) fn tls_options( input: &TlsInput, gcp_ca_certs: Option<&Entry>, -) -> Result { +) -> Result { Ok(TlsOptions { ca_certs: text(gcp_ca_certs.or(input.ca_certs.as_ref())).map(Into::into), ca_data: text(input.ca_data.as_ref()), @@ -27,7 +27,7 @@ pub(super) fn tls_options( }) } -fn verify_certificate(cert_reqs: Option<&Entry>) -> Result { +fn verify_certificate(cert_reqs: Option<&Entry>) -> Result { parsed(cert_reqs, Expected::CertRequirement, |text| match text { "required" | "optional" => Some(true), "none" => Some(false), @@ -45,7 +45,7 @@ pub(super) struct UrlQuery { pub(super) ignored: Vec, } -pub(super) fn url_query(url: &str) -> Result { +pub(super) fn url_query(url: &str) -> Result { let Ok(url) = url::Url::parse(url) else { return Ok(UrlQuery { tls: None, diff --git a/litellm-rust/crates/redis-resolve/tests/catalog.rs b/litellm-rust/crates/redis-resolve/tests/catalog.rs index 1ba97297329..517fc3f7bf1 100644 --- a/litellm-rust/crates/redis-resolve/tests/catalog.rs +++ b/litellm-rust/crates/redis-resolve/tests/catalog.rs @@ -1,6 +1,6 @@ use std::collections::{BTreeMap, BTreeSet}; -use litellm_redis_resolve::{Handling, RedisError, SETTINGS, Shape, interpret, kwargs}; +use litellm_redis_resolve::{Error, Handling, SETTINGS, Shape, interpret, kwargs}; use rstest::rstest; use serde::Deserialize; use serde_json::json; @@ -81,13 +81,13 @@ fn every_warned_setting_is_reported_and_still_reaches_the_host() { fn every_rejected_setting_fails_when_asked_for() { let rejected = SETTINGS .iter() - .filter(|setting| matches!(setting.handling, Handling::Reject)); + .filter(|setting| matches!(setting.handling, Handling::Reject { .. })); for setting in rejected { let error = interpret(&host_with(setting.kwarg, json!(true)), None).unwrap_err(); assert!( - matches!(&error, RedisError::Unenforceable { setting: source } if source == setting.kwarg), + matches!(&error, Error::Unenforceable { setting: source, .. } if source == setting.kwarg), "{error:?}" ); } diff --git a/litellm-rust/crates/redis-resolve/tests/interpret.rs b/litellm-rust/crates/redis-resolve/tests/interpret.rs index f9c958a9b38..a2f033644e7 100644 --- a/litellm-rust/crates/redis-resolve/tests/interpret.rs +++ b/litellm-rust/crates/redis-resolve/tests/interpret.rs @@ -1,6 +1,6 @@ use std::collections::BTreeMap; -use litellm_redis_resolve::{Entry, Expected, Kwargs, RedisError, interpret, kwargs}; +use litellm_redis_resolve::{Entry, Error, Expected, Kwargs, interpret, kwargs}; use litellm_redis_types::{ Auth, HealthCheck, Node, ProviderHandle, Resolution, StaticAuth, Target, TlsOptions, }; @@ -27,14 +27,14 @@ fn merged(fields: serde_json::Value, sources: &[(&str, &str)]) -> Kwargs { fn interpret_merged( fields: serde_json::Value, sources: &[(&str, &str)], -) -> Result { +) -> Result { let has_provider = fields .get("credential_provider") .is_some_and(|provider| !provider.is_null()); interpret(&merged(fields, sources), has_provider.then_some(PROVIDER)) } -fn interpret_kwargs(fields: serde_json::Value) -> Result { +fn interpret_kwargs(fields: serde_json::Value) -> Result { interpret_merged(fields, &[]) } @@ -93,7 +93,7 @@ fn a_url_query_db_that_is_not_an_integer_fails_like_parse_url() { let error = interpret_kwargs(json!({"url": "redis://cache:6379?db=first"})).unwrap_err(); assert!( - matches!(error, RedisError::Invalid { setting, expected: Expected::Integer } if setting == "url ?db") + matches!(error, Error::Invalid { setting, expected: Expected::Integer } if setting == "url ?db") ); }