UN-4123 [FEAT] Support TLS to Redis, and keep pooled/idle connections healthy - #2287
Open
muhammad-ali-e wants to merge 6 commits into
Open
muhammad-ali-e wants to merge 6 commits into
muhammad-ali-e wants to merge 6 commits into
Conversation
… healthy
Makes an encrypted connection to Redis possible so the platform can run against a
managed endpoint (Memorystore / ElastiCache / Azure Cache, which disables its
non-TLS port by default). Chart-side support for pointing at an external Redis is
UN-4122; password-only auth already worked, so what was missing was TLS.
Everything here is additive and inert by default: with nothing configured, the
local/in-cluster path builds exactly the client it built before.
**The scheme is the switch.** `{prefix}URL` (falling back to REDIS_URL) is handed
to redis.Redis.from_url, and `rediss://` selects TLS on its own — no separate
"use TLS" flag to forget, and `redis://` behaves as today. Discrete host/port vars
remain the primary path: they need no percent-encoding, and they are what the Helm
chart, every sample.env and the non-Python services (api-hub, llm-whisperer) read.
Two redis-py behaviours that bite silently, both handled and pinned by tests:
* The URL path beats a `db=` kwarg. sdk1 metrics asks for db=1 explicitly, so a
URL ending in /5 would have moved its keys into another service's keyspace
with nothing to show for it. The path is stripped when an override is given.
* `ssl=True` into a ConnectionPool does NOT fail at construction — the pool
defers kwargs to the connection class, so platform-service (max_connections=10)
started healthy, kept the PLAIN Connection class, and raised
`TypeError: AbstractConnection.__init__() got an unexpected keyword argument
'ssl'` on its first command. Pooled TLS now selects SSLConnection instead.
Also fixed, because they are the same class of silent failure:
* `{prefix}SSL` falls back to REDIS_SSL. Enabling TLS platform-wide previously
meant remembering CACHE_REDIS_SSL and MANUAL_REVIEW_REDIS_SSL too, and a
forgotten one is a plaintext client dialling a TLS port.
* django-redis 5.4.0 ignores both DB and USERNAME from OPTIONS (verified against
the installed 5.4.0: make_connection_params reads only PASSWORD and timeouts).
The db now travels in the LOCATION URL, so the backend cache stops sitting on
db 0 while every other service honours REDIS_DB — with REDIS_DB=N the workers
RPUSH log_history_queue to db N and the backend LPOPs an empty db 0. USERNAME
is deliberately NOT restored: auth stays password-only as the built-in
`default` user, which is what a managed AUTH string is.
* kombu reads TLS off the scheme but defaults ssl_cert_reqs to CERT_NONE —
encrypted while accepting any certificate. Socket.IO's manager URL carries an
explicit ssl_cert_reqs, since KombuManager takes a URL, not kwargs.
* The sidecar and tool-container environments are hand-picked allowlists (the
trap that made the LOG_TRANSPORT fix necessary). TLS settings and REDIS_DB now
reach both, and only when actually set — an empty string reads as "configured"
to os.getenv and would suppress the fallback.
health_check_interval now defaults to 30s, configurable via
{prefix}HEALTH_CHECK_INTERVAL. Only the two worker caches set it before, so a
connection killed while parked — managed failover, or Azure Cache's 10-minute idle
reaper — was discovered by a real command failing. This is the one intentional
behaviour change on the existing path, and it applies with or without TLS.
Retries are deliberately NOT enabled globally: retry_on_timeout would re-issue
blocking BLPOP/BLMOVE calls whose reply was lost, which risks consuming a second
message rather than recovering the first.
Tests: 21 new in unstract/core/tests/test_redis_client_config.py, 5 added to the
runner sidecar suite. Core 136, runner 10, Redis-related worker tests 51 — green.
Django settings verified by rendering both modes: plaintext yields
redis://host:6379/0 with no pool kwargs; TLS yields rediss://…/3 plus
ssl_cert_reqs/ssl_ca_certs, and the Socket.IO URL gains ?ssl_cert_reqs=required.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+574
to
+584
| _scheme = "rediss" if REDIS_SSL else "redis" | ||
| _cache_db = int(REDIS_DB) if REDIS_DB else 0 | ||
|
|
||
| # kombu reads TLS off the scheme, but defaults ssl_cert_reqs to CERT_NONE — | ||
| # encrypted while accepting ANY certificate, which is not what "TLS" is meant | ||
| # to buy. The query parameter is the only way to say otherwise here, since | ||
| # KombuManager takes a URL rather than connection kwargs. | ||
| _socketio_tls_query = f"?ssl_cert_reqs={REDIS_SSL_CERT_REQS}" if REDIS_SSL else "" | ||
| SOCKET_IO_MANAGER_URL = ( | ||
| f"{_scheme}://{_cred_prefix}{REDIS_HOST}:{REDIS_PORT}{_socketio_tls_query}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Enabling REDIS_SSL changes the backend Socket.IO manager to rediss://, but workers/log_consumer/tasks.py still builds the write-only Kombu manager with a hardcoded redis:// URL. Against a TLS-only Redis endpoint, the worker cannot publish to the channel used by the backend, so execution-log events no longer reach connected clients.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/backend/settings/base.py
Line: 574-584
Comment:
**Worker Publisher Remains Plaintext**
Enabling `REDIS_SSL` changes the backend Socket.IO manager to `rediss://`, but `workers/log_consumer/tasks.py` still builds the write-only Kombu manager with a hardcoded `redis://` URL. Against a TLS-only Redis endpoint, the worker cannot publish to the channel used by the backend, so execution-log events no longer reach connected clients.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
…gap it found
A managed Redis differs from the dev one in two ways that matter to this change —
it requires AUTH and speaks TLS — and neither was reachable from a laptop, so the
TLS path had no way to be exercised before merge. `docker-compose-redis-tls.yaml`
runs a Redis that does both on port 6380, alongside the normal `unstract-redis`,
so a developer can flip between the two and confirm BOTH still work.
Its plaintext listener is disabled (`--port 0`), matching Azure Cache's default:
a component that fails to pick up the TLS settings cannot then quietly succeed
over plaintext and hide the bug.
**Running it immediately found one.** URL mode carries TLS in the scheme and never
sets `{prefix}SSL`, but the CA was read inside that flag's branch — so a
`rediss://` URL verified against the system trust store alone and could not talk
to any server with a privately-signed certificate. That is exactly the case the CA
option exists for (Memorystore's CA is Google-managed and not publicly trusted).
The read moved out of the gate; consumers decide whether it applies, so a
`redis://` URL still ignores it. Two regression tests cover both directions.
Verified live against the container, with the new client code:
url rediss + CA -> OK [SSLConnection] roundtrip
url rediss, no CA -> FAIL CERTIFICATE_VERIFY_FAILED (expected)
discrete REDIS_SSL=true + CA -> OK [SSLConnection] roundtrip
plaintext against TLS port -> FAIL connection closed (expected)
TLS, wrong password -> FAIL AuthenticationError (expected)
pooled TLS (platform-service) -> OK [SSLConnection] ping
plain local redis (mode A) -> OK [Connection] ping
The negatives matter as much as the positives: they show a misconfigured client
fails loudly rather than silently degrading to plaintext.
redis-tls/README.md carries the switch-over runbook, including the one deliberate
asymmetry — the runner uses `ssl_cert_reqs=none`, because it forwards its settings
to tool sidecars and those get no CA mount (their environment is an allowlist and
only the shared log dir is mounted). Encrypted without verification still exercises
the forwarding fix and the handshake; a real managed endpoint does not hit this,
since ElastiCache and Azure chain to public CAs.
Certificates are gitignored — generate-certs.sh writes them locally, and the keys
are unencrypted dev material.
Core tests now 138.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
|
||
| ```bash | ||
| cd docker | ||
| ./redis-tls/generate-certs.sh |
Contributor
There was a problem hiding this comment.
The TLS setup tells developers to run ./redis-tls/generate-certs.sh, but that script is not included and there is no alternative step that creates the required ca.crt, server.crt, and server.key files. Following these instructions therefore fails before Compose can start the Redis service, making the new local TLS environment unusable as documented.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docker/redis-tls/README.md
Line: 20
Comment:
**Certificate Generator Is Missing**
The TLS setup tells developers to run `./redis-tls/generate-certs.sh`, but that script is not included and there is no alternative step that creates the required `ca.crt`, `server.crt`, and `server.key` files. Following these instructions therefore fails before Compose can start the Redis service, making the new local TLS environment unusable as documented.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Syncs with main, which has since removed the Celery execution transport from the workers, backend and SDK (UN-4078 / #2284). No conflicts: that change and this one touch different halves — it removed a transport, this one changes how the Redis CLIENT is built. Re-checked rather than assumed, because "merged cleanly" says nothing about whether the change still makes sense: * Socket.IO still rides kombu over Redis (backend/utils/log_events.py, workers/log_consumer/tasks.py) — so the rediss:// manager URL is still needed and still on the live log-streaming path. * The settings TLS block and the sidecar/tool env forwarding both survived intact. * Tests: core 160, runner 10, green. * Live re-probe against the local TLS Redis: managed-like URL mode and plain local Redis both connect.
The repo ignores *.sh, so `git add -A` skipped the script and the committed README referenced a file that did not exist in the branch — the dev harness was unusable for anyone cloning it. Force-added, as every other tracked .sh in this repo is. Found by running the harness from a fresh checkout rather than the worktree it was written in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found on a live run against the TLS Redis, not by reading the code: an API deployment came back `execution_status: COMPLETED` with `result: null`. `create_redis_client` honoured REDIS_URL from the start, but settings/base.py still built its own URL from REDIS_HOST/REDIS_PORT — so with a URL configured the WORKERS moved to the managed endpoint while the BACKEND stayed on the in-cluster one. Nothing errors in that state; the two simply stop sharing a keyspace. The execution really did run, its result really was cached — into the URL's Redis — and the backend looked for it in the other server and found nothing. Verified by key: `api_results:b2dd28d9…:d4355e01…` existed only in the TLS instance. REDIS_URL now drives both the cache LOCATION and SOCKET_IO_MANAGER_URL. The CA is appended as a query parameter rather than passed separately, because BOTH consumers parse it out of the URL — confirmed against the pinned redis-py 5.2.1 (-> SSLConnection with ssl_ca_certs) and kombu 5.5.4 (-> CERT_REQUIRED + ssl_ca_certs). An explicit ssl_ca_certs already in the URL is left alone, and a plaintext redis:// URL never gets one. In URL mode the db and credentials are dropped from OPTIONS: they travel in the URL, and passing both invites one silently winning over the other — the same class of bug as the db kwarg losing to the URL path in redis-py. Tests: backend/backend/tests/test_redis_settings_derivation.py, 9 cases. They execute the real source range from settings/base.py rather than a copy of the logic, since the block runs at import and cannot be called. Covers the plaintext default unchanged, the db-in-LOCATION fix, TLS pool kwargs, kombu's CERT_NONE default, and URL mode in both directions.
for more information, see https://pre-commit.ci
|
Comment on lines
+604
to
+605
| _socketio_tls_query = f"?ssl_cert_reqs={REDIS_SSL_CERT_REQS}" if REDIS_SSL else "" | ||
| SOCKET_IO_MANAGER_URL = _redis_url or ( |
Contributor
There was a problem hiding this comment.
When REDIS_URL uses rediss:// but omits ssl_cert_reqs, this branch passes the URL unchanged to Kombu instead of adding the configured certificate requirement. Kombu then uses CERT_NONE, so the Socket.IO Redis connection is encrypted but does not authenticate the server. An attacker who intercepts the connection could impersonate Redis and expose Redis credentials and event traffic.
How this was verified: A rediss:// URL takes the _redis_url branch without adding certificate requirements, while the consuming Kombu manager receives no separate TLS transport options.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/backend/settings/base.py
Line: 604-605
Comment:
**URL TLS Skips Verification**
When `REDIS_URL` uses `rediss://` but omits `ssl_cert_reqs`, this branch passes the URL unchanged to Kombu instead of adding the configured certificate requirement. Kombu then uses `CERT_NONE`, so the Socket.IO Redis connection is encrypted but does not authenticate the server. An attacker who intercepts the connection could impersonate Redis and expose Redis credentials and event traffic.
**How this was verified:** A `rediss://` URL takes the `_redis_url` branch without adding certificate requirements, while the consuming Kombu manager receives no separate TLS transport options.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Contributor
Unstract test resultsPer-group results
Critical paths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Makes an encrypted connection to Redis possible, so the platform can run against a managed endpoint (Memorystore, ElastiCache, Azure Cache — the last of which disables its non-TLS port by default).
The scheme is the switch.
{prefix}URL(falling back toREDIS_URL) goes toredis.Redis.from_url, andrediss://selects TLS on its own — there is no separate "use TLS" flag to forget, andredis://behaves exactly as today. Discrete host/port vars stay the primary path: they need no percent-encoding, and they are what the Helm chart, everysample.env, and the non-Python services (api-hub, llm-whisperer) read.Also in scope, because they are the same class of silent failure:
{prefix}SSLnow falls back toREDIS_SSL, plus a CA-certificate option.redis://).REDIS_DB.health_check_intervaldefaults to 30s.Why
Password-only auth to an external Redis already worked, so TLS was the missing half. Chart-side support for pointing at an external endpoint is UN-4122 (cloud repo); this is the OSS half.
Everything is additive and inert by default — with nothing configured, the local/in-cluster path builds exactly the client it built before.
Two redis-py behaviours bite silently, and both are now handled and pinned by tests:
db=kwarg.from_url('rediss://h:6380/5', db=1)yields db 5. sdk1 metrics asks fordb=1explicitly, so a URL carrying a path would have moved its keys into another service's keyspace with nothing to indicate it. The path is stripped when an override is given.ssl=Trueinto aConnectionPooldoes not fail at construction. The pool defers its kwargs to the connection class, so platform-service (max_connections=10) started healthy, kept the plainConnectionclass, and raisedTypeError: AbstractConnection.__init__() got an unexpected keyword argument 'ssl'on its first command. Pooled TLS now selectsSSLConnection.Three more silent failures fixed:
CACHE_REDIS_SSLandMANUAL_REVIEW_REDIS_SSLhad to be set separately; they now inheritREDIS_SSLand can still override it.DBandUSERNAMEfromOPTIONS. Verified against the installed version —ConnectionFactory.make_connection_paramsreads onlyPASSWORDand the two timeouts;DB: 3yieldsdb=None, whileredis://h:6379/3yieldsdb=3. The db now travels in theLOCATIONURL, so the backend cache stops sitting on db 0 while every other service honoursREDIS_DB: withREDIS_DB=N, workersRPUSH log_history_queueto db N and the backendLPOPs an empty db 0.USERNAMEis deliberately not restored — auth stays password-only as the built-indefaultuser, which is what a managed AUTH string is; named ACL users cannot work platform-wide while django-redis discards the username.ssl_cert_reqstoCERT_NONE— encrypted while accepting any certificate. The Socket.IO manager URL now carries an explicitssl_cert_reqs, sinceKombuManagertakes a URL rather than connection kwargs.How
health_check_intervaldefaults to 30s ({prefix}HEALTH_CHECK_INTERVAL, 0 disables). Only the two worker caches set it before, so a connection killed while parked — managed failover, or Azure Cache's 10-minute idle reaper — was discovered by a real command failing on it. This applies with or without TLS.Retries are deliberately not enabled globally.
retry_on_timeoutwould re-issue blockingBLPOP/BLMOVEcalls whose reply was lost, risking consuming a second message rather than recovering the first — the trap already documented atpg_queue/result_backend.py:152.Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
No. Every new setting is opt-in and the default path is unchanged — the first test in the new suite pins plaintext/localhost/db0 with no SSL kwargs.
Two intentional behaviour changes on the existing path, both called out for review:
health_check_intervalnow defaults to 30s instead of 0. Effect: redis-py sends aPINGbefore reusing a connection idle longer than that. SetREDIS_HEALTH_CHECK_INTERVAL=0to restore the old behaviour.LOCATIONnow carries the db path. ForREDIS_DBunset or0— every shipped config — the connection is identical. WhereREDIS_DB=N, the backend cache moves from db 0 to db N, which is the fix described above; that deployment is currently split-brained with its own workers.Sidecar/tool env keys are forwarded only when set, so an unconfigured deployment sees no new variables.
Database Migrations
None.
Env Config
All optional, all defaulting to current behaviour:
REDIS_URL/{prefix}URL,REDIS_SSL,REDIS_SSL_CERT_REQS,REDIS_SSL_CA_CERTS,REDIS_HEALTH_CHECK_INTERVAL. Documented inbackend/,runner/,platform-service/andworkers/sample.env.Relevant Docs
Module docstring in
unstract/core/.../cache/redis_client.pycovers URL-vs-discrete precedence and why discrete stays primary.Related Issues or PRs
UN-4123. Pairs with UN-4122 (cloud chart: external/managed Redis endpoint).
Not covered here, tracked separately: api-hub builds a credential-free
redis://URL and needs a one-line fix before AUTH is enabled anywhere, and llm-whisperer supports a password but has no TLS support. Both live in their own repos.Dependencies Versions
No changes. Behaviour verified against the pinned redis-py 5.2.1, kombu 5.5.4, django-redis 5.4.0.
Notes on Testing
unstract/core/tests/test_redis_client_config.py— defaults, discrete TLS (including the pooled regression), URL mode (scheme, percent-decoded password, db precedence, per-prefix URLs), and password-only auth.runner/tests/test_sidecar_log_transport.py, beside the existing LOG_TRANSPORT ones.redis://localhost:6379/0with no pool kwargs; TLS →rediss://cache.example:6380/3withssl_cert_reqs/ssl_ca_certs, and the Socket.IO URL gaining?ssl_cert_reqs=required.REDIS_SSL=truebefore relying on it in production, including a container-based tool so the sidecar path is exercised.🤖 Generated with Claude Code