fix(redis): decouple TLS from auth so on-prem can require a password

[scalejeff]

Pull Request Summary

On-prem Redis is plaintext but still requires a credential. The Redis helpers treated the presence of REDIS_AUTH_TOKEN as proof of in-transit encryption, so forcing auth on-prem produced a rediss:// handshake against a plaintext server. Separately, the on-prem config paths never applied the token at all, and cache_redis_host_port only stripped credentials from rediss:// URLs — so the moment a plaintext URL carried :password@, it returned :password@host:6379 and leaked the password into the KEDA ScaledObject's address.

core/celery/app.py

• Add redis_tls_enabled(), driven by REDIS_ENABLE_TLS. Unset falls back to bool(REDIS_AUTH_TOKEN), preserving today's ElastiCache behaviour.
• Extract build_redis_url() so scheme and credential are chosen independently; route get_redis_endpoint() through it.
• get_redis_instance() applies password and ssl=True separately.

common/config.py

• Add _apply_redis_auth_token(), applied to all three on-prem branches of cache_redis_url including the early cache_redis_onprem_url return. This keeps the password out of Helm values: the chart names host and db index, the app supplies the credential from its secret-backed env var. URLs already carrying userinfo are left alone.
• Make cache_redis_host_port scheme-agnostic.

Passwords are percent-encoded. kombu and redis-py both unquote userinfo, so the wire-level password is unchanged for AWS, while tokens containing @, / or # stop corrupting URL parsing — unencoded, a # truncates at the fragment and both parsers then raise ValueError on the mangled port.

Beyond the minimum fix

get_async_redis_instance() and RedisBroker._init_client() built passwordless URLs on every cloud, so both would fail NOAUTH against an authenticated broker. Both now use build_redis_url().

Chart

• Extract the Redis credential env into a shared modelEngine.redisAuthEnv helper, included from both the gateway/builder/cacher env and the celery autoscaler StatefulSet. The autoscaler had no REDIS_AUTH_TOKEN at all, so fixing RedisBroker._init_client() alone would have left it connecting anonymously.
• Resolve the scheme once in modelEngine.redisEnableTLS and feed both the app env and the KEDA scaler from it, so they cannot disagree. Where no explicit enableTLS is given, a credential configured via redis.auth/authSecretName implies TLS — mirroring the app's own fallback.
• Withhold REDIS_ENABLE_TLS entirely when the chart cannot determine it. A token supplied through the extraEnvVars hook is invisible to the chart, and emitting an explicit false there would override the app's inference and force plaintext against a TLS-only Redis.
• redis.enableTLS defaults to unset rather than false. Emitting false unconditionally would have downgraded an ElastiCache install with an auth token from TLS to plaintext.

Test Plan and Usage Guide

Chart. Rendered main and this branch across the full redis.auth × redis.enableTLS matrix, comparing the app's resolved scheme against the scaler's enableTLS:

case	main	this PR
A no-auth, unset	agree	agree
B auth, unset	drift	agree
C auth, tls=true	agree	agree
D auth, tls=false	drift	agree
E no-auth, tls=true	drift	agree
F no-auth, tls=false	agree	agree

main had three drifting cases; all six now agree. The app's resolved scheme is unchanged in every configuration — case B still yields rediss://, so ElastiCache installs with a token and no explicit enableTLS are untouched. Only the scaler moves, and only in B, where it was talking cleartext to a TLS endpoint and silently collecting no queue metrics.

Also verified: stringified booleans pass through (desired-state value overrides can produce them, and gating on a real bool would have silently dropped "false"); authSecretName implies TLS exactly as auth does; the autoscaler StatefulSet is wired; helm lint passes.

Python. ruff 0.6.8 and black 24.8.0 clean. 22 new tests in tests/unit/common/test_redis_auth.py covering TLS/auth decoupling, token injection, cache_redis_host_port across six URL shapes, and get_redis_instance across all four password/TLS combinations.

Note

The new tests were exercised against the committed function bodies via an import shim rather than in-tree, because tests/unit/conftest.py needs fastapi and the deps were not installed locally. 22/22 pass, and the 4 assertions covering cache_redis_host_port and get_redis_instance fail against the pre-fix bodies, so they are genuine regression tests. They still need one CI run to confirm the in-tree import path.

Known pre-existing issues, deliberately not fixed here

Both were flagged in review and verified against main as predating this PR. Called out rather than bundled in, to keep this change's "no AWS behaviour change" property auditable:

1. ssl_cert_reqs=none means Redis TLS certificates are never verified. Present on main (lines 214, 229) and preserved verbatim. Fixing it properly is a deploy-time behaviour change for AWS and deserves its own PR. Worth tracking — it is a real MITM exposure.
2. The AWS-secret branch of get_redis_endpoint() does not percent-encode its token. That branch is byte-identical between main and this branch.

Confidence Score: 4/5

The PR is not yet safe to merge because KEDA can attempt plaintext connections to TLS cache endpoints when redis.enableTLS is unset.

Findings

1.  KEDA Defaults TLS Off ▶

Fix with agent prompt

### Issue 1
charts/model-engine/templates/service_template_config_map.yaml:516
When `redis.enableTLS` is unset, this template always renders KEDA's `enableTLS` as `false`, even if `cache_redis_url` identifies a TLS cache endpoint such as Azure Redis with `rediss://`. KEDA receives only the scheme-stripped `${REDIS_HOST_PORT}`, so it cannot recover that transport choice and attempts a plaintext connection to the TLS endpoint. The Redis trigger then cannot read queue metrics, breaking autoscaling. Derive this metadata from the cache endpoint's scheme instead of defaulting an unrelated broker setting to `false`.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

Summary

• Adds percent-encoded Redis credentials without forcing TLS.
• Applies credentials across on-prem cache configuration paths.
• Uses the shared URL builder for synchronous and asynchronous Redis clients.
• Adds Redis auth/TLS unit coverage and increments the chart version.
• Updates Helm workload environments and KEDA Redis metadata, although the latter currently defaults TLS cache endpoints to plaintext when redis.enableTLS is unset.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[cache_redis_url with scheme] --> B[cache_redis_host_port]
  B -->|scheme removed| C[KEDA address]
  D[redis.enableTLS unset] --> E[default false]
  E --> C
  C --> F[Plaintext KEDA connection]
  A -->|rediss cache endpoint| G[TLS-only Redis]
  F -. connection fails .-> G

Loading

_{Reviews (5) · Last reviewed commit: "fix(chart): stop deriving scaler TLS fro..."}

[neelaypandit-scale]

otherwise looks good, just flagged 2 things, if you can just take a second look and dismiss if they are moot, I can approve

[greptile-apps]

KEDA Defaults TLS Off

When redis.enableTLS is unset, this template always renders KEDA's enableTLS as false, even if cache_redis_url identifies a TLS cache endpoint such as Azure Redis with rediss://. KEDA receives only the scheme-stripped ${REDIS_HOST_PORT}, so it cannot recover that transport choice and attempts a plaintext connection to the TLS endpoint. The Redis trigger then cannot read queue metrics, breaking autoscaling. Derive this metadata from the cache endpoint's scheme instead of defaulting an unrelated broker setting to false.

Prompt To Fix With AI

This is a comment left during a code review.
Path: charts/model-engine/templates/service_template_config_map.yaml
Line: 516

Comment:
**KEDA Defaults TLS Off**

When `redis.enableTLS` is unset, this template always renders KEDA's `enableTLS` as `false`, even if `cache_redis_url` identifies a TLS cache endpoint such as Azure Redis with `rediss://`. KEDA receives only the scheme-stripped `${REDIS_HOST_PORT}`, so it cannot recover that transport choice and attempts a plaintext connection to the TLS endpoint. The Redis trigger then cannot read queue metrics, breaking autoscaling. Derive this metadata from the cache endpoint's scheme instead of defaulting an unrelated broker setting to `false`.

---

For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.

[scalejeff]

This is also pre-existing. The default value for .Values.redis.enableTLS was false. Again this is the cloud branch which I am trying to leave byte identical to today. The owning team should come through and make TLS and credential support unified across all cache access.

[greptile-apps]

Understood. Since .Values.redis.enableTLS defaulting to false predates this PR and changing it would alter the existing cloud behavior this change explicitly preserves, this is not a regression introduced here. I’ll withdraw this finding; unifying TLS and credential handling across cache access should be addressed separately by the owning team.