feat(proxy): configure the coordination redis independently of the response cache - #32661
Conversation
|
|
|
osv-scan is red here because this PR stacks on #32635, whose base predates the soupsieve 2.8.4 bump that landed on staging in #32643; the scan reads the stacked base's uv.lock (soupsieve 2.8.3). Nothing in this diff touches dependencies. It clears as soon as #32635 merges and this branch rebases onto staging, which is the intended merge order anyway |
1460176 to
e789404
Compare
…nd is not Redis Selecting a semantic (or any non-Redis-KV) response cache left redis_usage_cache unset, silently downgrading cross-pod rate limits, parallel-request limits, spend coordination, and the pod lock manager to per-pod in-memory state. Fall back to a standalone RedisCache built from REDIS_* environment variables, mirroring the existing use_redis_transaction_buffer escape hatch, which now shares the same helper. Resolves LIT-3861
Greptile SummaryThis PR decouples the proxy's coordination Redis (cross-pod rate limits, spend tracking, pod lock manager) from the response-cache backend by introducing
Confidence Score: 3/5The core decoupling logic is sound but The shared
|
| Filename | Overview |
|---|---|
| litellm/proxy/proxy_server.py | Adds coordination Redis initialization from both file config and DB; _build_redis_usage_cache silently mixes REDIS_CLUSTER_NODES env into explicit host-based configs, which can redirect the startup client and the dashboard test connection to the wrong Redis. |
| litellm/proxy/management_endpoints/coordination_redis_endpoints.py | New endpoints for GET/POST/TEST coordination Redis; credential redaction and audit logging are well-structured, but the test handler's scrub logic misses env-ref-resolved passwords in Redis error strings. |
| litellm/_redis.py | Adds an explicit-target guard that prevents REDIS_URL from the environment from overriding an explicit host/startup_nodes/sentinel_nodes argument — a clean targeted fix. |
| litellm/proxy/_types.py | Adds CoordinationRedisParams and CoordinationRedisNode Pydantic models with a has_connection_target() validator; types are well-defined and the model uses extra="allow" to pass through additional Redis kwargs. |
| litellm/types/management_endpoints/coordination_redis_endpoints.py | New field definitions and type exports for coordination Redis settings; clean and well-structured. |
| tests/test_litellm/proxy/management_endpoints/test_coordination_redis_endpoints.py | Comprehensive mock-only unit tests covering redaction, source detection, merge-over-saved, credential scrubbing, and validation edge cases; all look correct. |
| tests/test_litellm/proxy/test_proxy_server.py | Adds coordination Redis init tests; the _run_init_cache_with_backend helper was updated to use the returned value rather than reading the global, which matches the new API but loses direct global-state verification. |
| tests/test_litellm/test_redis.py | New tests for _get_redis_client_logic explicit-target precedence over env URL; straightforward and correct. |
| ui/litellm-dashboard/src/app/(dashboard)/caching/components/coordination_redis_settings/coordinationRedisUtils.ts | Form utilities for building and parsing the coordination Redis payload; boolean fields always serialize as false rather than undefined when unset, meaning ssl: false is always included in POST bodies. |
| ui/litellm-dashboard/src/app/(dashboard)/caching/components/coordination_redis_settings/index.tsx | Main coordination Redis settings UI component; form lifecycle, test connection, and save flows look correct. |
| ui/litellm-dashboard/src/components/networking.tsx | Adds three new API call helpers for coordination Redis GET/POST/test; consistent with existing patterns in the file. |
Reviews (1): Last reviewed commit: "feat(proxy): configure the coordination ..." | Re-trigger Greptile
d3514ca to
01210d1
Compare
|
|
||
|
|
||
| def _build_redis_usage_cache(redis_params: Mapping[str, object]) -> RedisCache: | ||
| """ | ||
| Builds the proxy's coordination Redis client from resolved connection | ||
| params. Cluster-mode targets (explicit `startup_nodes` or the | ||
| REDIS_CLUSTER_NODES env var) get a `RedisClusterCache`, so consumers that | ||
| branch on cluster mode (e.g. the v3 rate limiter) take the cluster path; | ||
| everything else (host/url/sentinel) gets a plain `RedisCache`. | ||
| """ | ||
| startup_nodes = redis_params.get("startup_nodes") | ||
| if startup_nodes is None: | ||
| env_cluster_nodes = get_secret_str("REDIS_CLUSTER_NODES") | ||
| if env_cluster_nodes is not None: | ||
| startup_nodes = json.loads(env_cluster_nodes) | ||
| non_node_params = {key: value for key, value in redis_params.items() if key != "startup_nodes"} | ||
| if startup_nodes: | ||
| return RedisClusterCache(startup_nodes=startup_nodes, **non_node_params) | ||
| return RedisCache(**non_node_params) | ||
|
|
||
|
|
||
| def _environment_has_redis_connection_target() -> bool: |
There was a problem hiding this comment.
REDIS_CLUSTER_NODES leaks into explicit and test-connection paths
_build_redis_usage_cache unconditionally reads REDIS_CLUSTER_NODES from the environment when startup_nodes is absent from the caller's params. This means an explicit coordination_redis: {host: "myhost"} block silently builds a RedisClusterCache against the environment cluster nodes instead of "myhost", defeating the PR's stated goal of "explicit takes precedence."
The same bug hits the /coordination_redis/settings/test endpoint: a user who types host: "new-coord-redis" in the dashboard gets a cluster ping to whatever REDIS_CLUSTER_NODES names, not to the host they specified, so the test result is for an entirely different Redis.
A simple guard fixes both call sites — only consult the environment cluster nodes when the caller provided no explicit connection target: add an allow_env_cluster: bool = False keyword argument, pass True only from _build_redis_usage_cache_from_environment, and leave the explicit and test-connection paths at the default False.
|
|
||
| Redis client errors routinely echo the connection url (password inline) or | ||
| the auth error back to the caller. | ||
| """ | ||
| scrubbed = message | ||
| for secret in _credential_values(settings): | ||
| if secret: | ||
| scrubbed = scrubbed.replace(secret, _REDACTED_VALUE) | ||
| return scrubbed | ||
|
|
There was a problem hiding this comment.
_scrub_credentials doesn't scrub resolved env-ref passwords
_credential_values collects the literal string from settings, so when a credential is stored as an os.environ/REDIS_PASSWORD reference the scrubber removes the reference string, not the actual password. If Redis echoes the real password in a connection-refused or auth-failure message (e.g. via an embedded URL), the resolved secret leaks to the caller of the test endpoint.
The resolved values from params (returned by _validated_params) should be included in the scrub set alongside the raw setting values, since those are what the Redis client actually uses and what would appear in error output.
…sponse cache Adds general_settings.coordination_redis, an explicit block for the Redis the proxy uses for cross-pod rate limits, parallel-request limits, spend tracking, the pod lock manager, and shared health checks. Resolution order is the explicit block, then a plain-Redis response-cache backend, then the REDIS_* environment. Cluster and sentinel targets are supported, and a cluster target now builds a RedisClusterCache so cluster-aware consumers take the cluster path. Admins can configure it from the Caching page of the dashboard via /coordination_redis/settings, which reports which source is in effect, redacts credentials on read, and offers a connection test. Settings saved there are read back at startup so they take effect on restart. Also fixes redis client construction so an explicitly configured host outranks REDIS_URL in the environment. Previously the url branch stripped the caller's host and port, so an explicit block, or a connection test typed into the dashboard, silently targeted whatever REDIS_URL named
e789404 to
1d69515
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
7170c01 to
1d69515
Compare
|
|
||
| await ConfigRepository(prisma_client).set_param( | ||
| param_name=_GENERAL_SETTINGS_PARAM_NAME, | ||
| param_value={**general_settings, _COORDINATION_REDIS_KEY: settings}, |
There was a problem hiding this comment.
Medium: Redis credentials stored in plaintext
settings can contain password, sentinel_password, or a credential-bearing url, and this writes them directly to LiteLLM_Config. An attacker with read access to that table or its backups can recover the Redis credentials and use them to alter shared spend and rate-limit state. Encrypt credential fields before persistence and decrypt them when loading the saved coordination configuration, following the existing cache-settings storage path.
PR overviewThis PR adds proxy management support for configuring the coordination Redis separately from the response cache. It introduces endpoints and persistence for coordination Redis settings used by shared proxy coordination state. There is one open security issue: coordination Redis credentials may be persisted in plaintext, including passwords or credential-bearing URLs. If someone can read the configuration table or its backups, they could recover those credentials and potentially modify shared spend and rate-limit coordination state. No issues have been marked fixed yet, so this credential-handling gap remains the current security concern. Open issues (1)
Fixed/addressed: 0 · PR risk: 4/10 |
Relevant issues
Linear ticket
Resolves LIT-3861
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Stacked on #32635, which is the minimal defect fix. This PR is the full decoupling and should merge after it. Review the diff against
litellm_redis_usage_cache_env_fallbackWhat this enables: two Redis roles, configured separately, running side by side. The LLM response cache (
litellm_settings.cache_params) and the coordination Redis (general_settings.coordination_redis) are now independent connections. A deployment can point the response cache at aredis-semanticbackend for semantic cache hits, and simultaneously point the coordination Redis at a plain Redis (or a cluster, or a sentinel set) for cross-pod tpm/rpm limits, parallel request limits, spend tracking, the pod lock manager and shared health checks. They may be different servers, different topologies, or the same server; nothing forces them to be the same connection any more, and configuring one no longer implies or disables the otherScreenshots / Proof of Fix
Two pods share one Postgres and one Redis 8, real OpenAI calls to
gpt-5.4-miniThe explicit block works with no response cache at all
Before this PR there was no way to get a coordination Redis without configuring a response cache. With
general_settings.coordination_redisand nolitellm_settings.cacheat all, anrpm_limit: 2key is enforced across both podsOn the parent commit the same config serves all four requests (limits fall back to per-pod memory)
A semantic response cache and a plain coordination Redis coexist
This is the case the ticket was filed for.
cache_params.type: redis-semanticdrives the response cache whilegeneral_settings.coordination_redisindependently drives rate limiting and spend, both live on the same two pods at the same time. The Redis key scan below shows the two roles side by side:litellm_semantic_cache_index:*from the response cache, and the rate limit window, parallel request slots and spend counters from the coordination RedisThe management endpoints
GETreports which source is in effect and redacts credentialsA connection test against a port with nothing listening reports unhealthy, and the submitted password never appears in the error
A block with no connection target is rejected rather than silently running without coordination
Settings saved from the dashboard take effect on restart
Saved via
POST /coordination_redis/settings, then the proxy restarted with a config file that has no coordination block and with noREDIS_*variables in the environmentFor the Admin UI, run the proxy and the dashboard, go to http://localhost:4000/ui/?page=caching, open the Coordination Redis tab, set a host and port, press Test Connection, then Save Changes, and restart the proxy
Type
🆕 New Feature
Changes
redis_usage_cachecould only ever be populated from the response-cache backend. #32635 added aREDIS_*environment fallback so a semantic cache no longer silently disables cross-pod coordination. This PR finishes the decoupling by giving the coordination Redis its own configuration surfacegeneral_settings.coordination_redisacceptshost,port,username,password,url,ssl,startup_nodes,sentinel_nodes,sentinel_passwordandservice_name, resolvesos.environ/references the waycache_paramsdoes, and is validated by aCoordinationRedisParamsmodel that rejects a block naming no connection target. Resolution order is the explicit block, then a plain-Redis response-cache backend, then the environment. A cluster target, whether fromstartup_nodesorREDIS_CLUSTER_NODES, now builds aRedisClusterCacheso consumers that branch on cluster mode take the cluster path; sentinel-only environments are covered too, closing a gap the environment fallback inherited from the transaction-buffer helperGET,POSTandPOST /testunder/coordination_redis/settingsback a new Coordination Redis tab on the Caching page of the dashboard. The read reports whether the coordination Redis comes from the explicit block, a borrowed response cache, or the environment, so an operator can see what is actually in effect. Credentials are redacted on read and scrubbed from connection-test errors. Settings saved there are persisted to the config row and read back during startup, before the coordination Redis is published to its consumers, so the restart actually applies themOne bug fix falls out of building the connection test.
_get_redis_client_logicmerged the environment before the caller's arguments but then let aurlfromREDIS_URLstrip the caller'shostandport. An explicitly configured host therefore lost to the environment, which would have made both an explicitcoordination_redisblock and a connection test typed into the dashboard silently target whateverREDIS_URLnamed. An explicit connection target now outranks the environment, while an expliciturland the environment-only path are unchangedDeploy artifacts and documentation are handled separately, in the helm and terraform branch and in the docs repo