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.

Greptile Summary

This PR decouples Redis authentication from TLS so authenticated plaintext Redis works on-prem while preserving the existing authenticated ElastiCache TLS fallback.

• Centralizes Redis URL construction and independently applies credentials and TLS settings.
• Adds authentication to on-prem cache URLs, async Redis clients, and the Celery autoscaler.
• Aligns application and KEDA TLS resolution while preserving inference for externally supplied tokens.
• Prevents credentials from leaking into KEDA Redis addresses.
• Adds focused regression coverage and increments the Helm chart patch version.

Confidence Score: 5/5

The PR appears safe to merge; no new actionable failures were introduced since the previous review.

The latest change only increments the Helm chart patch version. The earlier transport-alignment and external-token findings are resolved, while the certificate-verification and AWS token-encoding threads were manually resolved as intentionally deferred, pre-existing work.

Important Files Changed

Filename	Overview
model-engine/model_engine_server/core/celery/app.py	Separates Redis TLS selection from authentication and centralizes URL/client construction.
model-engine/model_engine_server/common/config.py	Injects secret-backed credentials into on-prem Redis URLs and strips userinfo from scaler addresses.
charts/model-engine/templates/_helpers.tpl	Centralizes chart-side Redis authentication environment variables and TLS resolution.
charts/model-engine/templates/celery_autoscaler_stateful_set.yaml	Supplies Redis authentication and transport configuration to the autoscaler.
charts/model-engine/templates/service_template_config_map.yaml	Uses the shared TLS resolution for KEDA Redis metadata.
model-engine/model_engine_server/core/celery/celery_autoscaler.py	Reuses the shared authenticated Redis URL builder for autoscaler clients.
model-engine/tests/unit/common/test_redis_auth.py	Covers independent authentication/TLS combinations, URL encoding, token injection, and credential stripping.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[Redis configuration] --> B{REDIS_ENABLE_TLS explicitly set?}
    B -->|Yes| C[Use explicit TLS value]
    B -->|No| D{REDIS_AUTH_TOKEN present?}
    D -->|Yes| E[Infer TLS enabled]
    D -->|No| F[Use plaintext]
    C --> G[Build Redis URL]
    E --> G
    F --> G
    A --> H{Credential present?}
    H -->|Yes| I[Percent-encode and apply password]
    H -->|No| J[No userinfo]
    I --> G
    J --> G
    G --> K[Sync and async Redis clients]
    G --> L[Celery autoscaler broker]
    A --> M[Helm TLS resolution]
    M --> N[Application environment]
    M --> O[KEDA scaler metadata]

Loading

_{Reviews (4): Last reviewed commit: "chore(chart): bump version to 0.2.10" | Re-trigger Greptile}

[neelaypandit-scale]

Can you double check whether this regresses AWS? Specifically enableTLS: "{{ include "modelEngine.redisEnableTLS" . }}".

The concern is that the scaler and the app connect to two different Redis instances. This trigger's address is ${REDIS_HOST_PORT} = hmi_config.cache_redis_host_port (k8s_resource_types.py:1368), which derives from cache_redis_url → on AWS that's cache_redis_aws_url verbatim, untouched by this PR (config.py:150). But redisEnableTLS infers TLS from redis.auth/authSecretName, which is the message broker's credential — a different endpoint.

The app connects to the cache using whatever scheme cache_redis_aws_url names, and that path never gets a credential injected on AWS. So if that URL is redis://, the cache accepts plaintext, and telling the scaler TLS=true is wrong for the same endpoint. Both shipped examples are plaintext, on a host distinct from the broker:

• values_sample.yaml:263 → redis://llm-engine-prod-cache.use1.cache.amazonaws.com:6379/15
• values_circleci.yaml:172 → redis://redis-message-broker-master.default/15

Rendered with values_circleci.yaml --set redis.auth=… and enableTLS unset: main gives enableTLS: "false", this branch gives "true". If that's the shape of a real install, KEDA's handshake against a plaintext cache fails, the redis trigger reports no queue length, and async endpoints stop scaling off the activation threshold. AWS does take this branch — the servicebus alternative is gated on $azure_cloud_provider (:497).

[neelaypandit-scale]

Can you check if Injecting the credential into cache_redis_url leaks it into logs?

entrypoints/k8s_cache.py:108 does logger.info(f"Using cache redis url {redis_url}") with no debug_mode guard, and the cacher Deployment runs exactly that entrypoint (cacher_deployment.yaml:61-64) with REDIS_AUTH_TOKEN in its env. On main the on-prem URL carried no credential, so this is new exposure — on every cacher startup, into Datadog. service_builder/tasks_v1.py:158 repeats it behind debug_mode.

[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