feat(deploy): make coordination redis a first-class chart and terraform surface - #32662
Conversation
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Greptile SummaryThis PR promotes
Confidence Score: 4/5Safe to merge for the common case; the one edge case (bundled Redis with auth disabled) is non-default and would have already caused issues before this PR. The sentinel bug fix and coordination_redis injection are correct and well-tested. The only gap is that the coordination_redis block unconditionally references os.environ/REDIS_PASSWORD even when redis.auth.enabled is false, which would cause a proxy startup failure for that non-default configuration. All other paths are covered by the new test suite. helm/litellm-helm/templates/configmap-litellm.yaml — the unconditional password env ref in the coordination_redis block warrants a second look for auth-disabled Redis deployments.
|
| Filename | Overview |
|---|---|
| helm/litellm-helm/templates/_helpers.tpl | Bug fix: sentinel service name now correctly keys off redis.sentinel.enabled alone instead of the impossible standalone + sentinel condition; helper logic is clean and the two new tests verify both code paths. |
| helm/litellm-helm/templates/configmap-litellm.yaml | New coordination_redis injection logic; correctly uses deepCopy and user-block guard. Password env ref is unconditional and will break proxy startup if redis.auth.enabled is false. |
| helm/litellm-helm/tests/coordination_redis_tests.yaml | Comprehensive new test suite covering standalone, sentinel, opt-out, and user-supplied-block cases; YAML pattern assertions match alphabetical serialization order. |
| helm/litellm-helm/values.yaml | Adds redis.coordination.enabled (default true) with clear comments explaining sentinel, env-fallback, and caching independence; no structural issues. |
| helm/litellm/templates/_helpers.tpl | Comment-only update clarifying that REDIS_CLUSTER_NODES feeds coordination fallback, not Cache(); no logic changes. |
| helm/litellm/tests/redis_env_tests.yaml | New test suite covering REDIS_HOST/PORT/PASSWORD emission, cluster-node seeding, and auth-less omission for both gateway and backend deployments. |
| terraform/litellm/README.md | New Coordination Redis row in the parity table and a new prose section documenting the automatic REDIS_* export and override pattern; accurate and well-scoped. |
Reviews (1): Last reviewed commit: "feat(deploy): make coordination redis a ..." | Re-trigger Greptile
| {{- $coordinationRedis := dict "host" "os.environ/REDIS_HOST" "port" "os.environ/REDIS_PORT" "password" "os.environ/REDIS_PASSWORD" }} | ||
| {{- if .Values.redis.sentinel.enabled }} | ||
| {{- $sentinelNode := list (include "litellm.redis.serviceName" .) (include "litellm.redis.port" . | int) }} | ||
| {{- $coordinationRedis = dict "sentinel_nodes" (list $sentinelNode) "service_name" (default "mymaster" .Values.redis.sentinel.masterSet) "password" "os.environ/REDIS_PASSWORD" }} |
There was a problem hiding this comment.
Password ref always emitted regardless of Redis auth config
Both the standalone and sentinel branches unconditionally add password: os.environ/REDIS_PASSWORD to the coordination_redis block. If a user deploys the bundled Redis with redis.auth.enabled: false (disabling Bitnami's auth), the Bitnami subchart will not create the <release>-redis secret and the REDIS_PASSWORD env var will not be set in the pod. At startup, litellm will attempt to resolve os.environ/REDIS_PASSWORD and fail with a key error, preventing the proxy from starting.
Consider guarding the password field on redis.auth.enabled (which defaults to true in the Bitnami subchart): {{- if ne (dig "auth" "enabled" true .Values.redis) false }}. The same guard would apply in the sentinel branch.
…rm surface Render a general_settings.coordination_redis block into the litellm-helm proxy config when the bundled Redis is enabled, gated on a new redis.coordination.enabled value and skipped when the user already supplies their own block. Sentinel deployments render sentinel_nodes and service_name rather than a host/port pair. Also fixes litellm.redis.serviceName, which gated its sentinel branch on standalone architecture. The bundled Redis subchart only serves sentinel in replication mode, and renders no master Service there, so REDIS_HOST pointed at a Service that never existed for every sentinel user. Documents the coordination redis in the componentized chart and in the terraform modules, whose existing REDIS_* exports now feed it directly. Adds helm-unittest coverage for both charts' redis wiring, which had none
2648bf1 to
4b00127
Compare
| {{- $sentinelNode := list (include "litellm.redis.serviceName" .) (include "litellm.redis.port" . | int) }} | ||
| {{- $coordinationRedis = dict "sentinel_nodes" (list $sentinelNode) "service_name" (default "mymaster" .Values.redis.sentinel.masterSet) "password" "os.environ/REDIS_PASSWORD" }} | ||
| {{- end }} | ||
| {{- $_ := set $generalSettings "coordination_redis" $coordinationRedis }} |
There was a problem hiding this comment.
Medium: Redis coordination config is ignored
coordination_redis is rendered under general_settings, but the proxy startup path does not read that key when attaching Redis to the internal usage cache used by rate-limit and spend coordination. In a multi-pod chart install that relies on this new setting, an authenticated user can exceed configured RPM/TPM limits by sending traffic across pods; either wire this block into the proxy's Redis usage cache initialization or have the chart render the existing Redis cache configuration path that startup already consumes. The same env-only assumption appears in helm/litellm/templates/_helpers.tpl.
PR overviewThis pull request promotes coordination Redis configuration into the deployment surfaces for the LiteLLM Helm chart and Terraform-based installs. It adds chart rendering for the new coordination Redis settings used by multi-pod deployments. There is one open security issue: the Helm chart renders the new Redis coordination configuration in a location that the proxy startup path does not consume, so multi-pod installs may silently run without shared Redis-backed coordination. In affected deployments, an authenticated user could distribute traffic across pods to exceed configured rate or spend limits. No issues have been addressed yet, so the PR still needs the Redis configuration path to be wired into the runtime behavior or rendered through an existing consumed cache configuration path. Open issues (1)
Fixed/addressed: 0 · PR risk: 6/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)Merge after #32661, which adds the
general_settings.coordination_redisblock this renders. The charts keep working against an older proxy either way, since an unrecognized general_settings key is ignored and the existing REDIS_* env vars still drive the fallbackScreenshots / Proof of Fix
The deploy artifacts here are chart and module templates, so the proof is tool-native rendering rather than a live proxy
Standalone Redis enabled renders the block into the proxy config
Sentinel renders sentinel_nodes and service_name instead of a host and port, because pointing a plain client at a sentinel port does not work
Chart tests and lint
The new suites were mutation tested rather than trusted green: reverting the sentinel helper gate, ignoring
redis.coordination.enabled, dropping the user-block guard, dropping the cluster gate, always emitting REDIS_PASSWORD, and hardcoding the cluster seed port each fail at least one testType
🚄 Infrastructure
Changes
The proxy is gaining
general_settings.coordination_redis, an explicit block for the Redis behind cross-pod rate limits, spend tracking, and the pod lock manager. This makes it a first-class deploy surfacehelm/litellm-helmrenders a coordination_redis stanza into the proxy config when the bundled Redis is enabled, gated on a newredis.coordination.enabledvalue that defaults to true, and skipped entirely when the user already supplies their own block inproxy_config. Sentinel deployments rendersentinel_nodesandservice_name. The existing REDIS_* deployment env stays for back-compathelm/litellmkeeps its external-Redis env wiring; its stale comment claiming the cluster nodes variable feedsCache()is replaced with a description of the coordination fallback, and the values comments name coordination explicitlyThe terraform modules already export REDIS_HOST, REDIS_PORT and REDIS_SSL from ElastiCache and Memorystore, so the environment fallback covers them with no resource changes. The README gains a coordination-redis entry and both default examples show the explicit block
This also fixes a pre-existing bug.
litellm.redis.serviceNamegated its sentinel branch on the standalone architecture, but the bundled Redis subchart only serves sentinel in replication mode, and renders no master Service there. REDIS_HOST therefore pointed at a Service that never existed for every sentinel user. The helper now keys offredis.sentinel.enabledalone, which changes REDIS_HOST on that path from<release>-redis-masterto<release>-redis; two tests cover itNeither chart asserted a single Redis environment variable before, so the new suites also cover the existing wiring