UN-4123 [FEAT] Support TLS to Redis, and keep pooled/idle connections healthy - #2287
Open
muhammad-ali-e wants to merge 1 commit into
Open
muhammad-ali-e wants to merge 1 commit into
muhammad-ali-e wants to merge 1 commit 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.
When an operator configures only REDIS_URL as documented, this branch still builds the Django cache and Socket.IO connections from REDIS_HOST, REDIS_PORT, and REDIS_SSL. Shared clients created through create_redis_client do honor REDIS_URL, so the backend can connect its Redis consumers to different endpoints. This breaks cache, session, and WebSocket behavior or splits queues across Redis instances.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/backend/settings/base.py
Line: 574-584
Comment:
**Redis URL Is Ignored**
When an operator configures only `REDIS_URL` as documented, this branch still builds the Django cache and Socket.IO connections from `REDIS_HOST`, `REDIS_PORT`, and `REDIS_SSL`. Shared clients created through `create_redis_client` do honor `REDIS_URL`, so the backend can connect its Redis consumers to different endpoints. This breaks cache, session, and WebSocket behavior or splits queues across Redis instances.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
181
to
+190
| if ssl: | ||
| result["ssl_cert_reqs"] = os.getenv(f"{env_prefix}SSL_CERT_REQS", "required") | ||
| # Needed where the server's CA is not in the system trust store — notably | ||
| # Memorystore, whose CA is Google-managed. ElastiCache and Azure chain to | ||
| # public CAs and need nothing here. | ||
| ca_certs = os.getenv( | ||
| f"{env_prefix}SSL_CA_CERTS", os.getenv("REDIS_SSL_CA_CERTS", "") | ||
| ).strip() | ||
| if ca_certs: | ||
| result["ssl_ca_certs"] = ca_certs |
Contributor
There was a problem hiding this comment.
If a deployment uses the documented REDIS_URL=rediss://... mode with REDIS_SSL_CA_CERTS, but does not also set the redundant REDIS_SSL=true flag, this block never resolves the CA certificate. The CA is therefore not passed to Redis.from_url, and certificate verification fails for private-CA endpoints such as Memorystore.
Prompt To Fix With AI
This is a comment left during a code review.
Path: unstract/core/src/unstract/core/cache/redis_client.py
Line: 181-190
Comment:
**URL TLS Drops Custom CA**
If a deployment uses the documented `REDIS_URL=rediss://...` mode with `REDIS_SSL_CA_CERTS`, but does not also set the redundant `REDIS_SSL=true` flag, this block never resolves the CA certificate. The CA is therefore not passed to `Redis.from_url`, and certificate verification fails for private-CA endpoints such as Memorystore.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
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.
Comment on lines
+581
to
+584
| _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.
If the Redis server uses a CA outside the system trust store, this URL forwards ssl_cert_reqs but omits REDIS_SSL_CA_CERTS. The Django Redis pool immediately below receives the custom CA, so cache access can succeed while the Kombu manager cannot verify the same server, disabling WebSocket event delivery.
Prompt To Fix With AI
This is a comment left during a code review.
Path: backend/backend/settings/base.py
Line: 581-584
Comment:
**Socket.IO Omits Custom CA**
If the Redis server uses a CA outside the system trust store, this URL forwards `ssl_cert_reqs` but omits `REDIS_SSL_CA_CERTS`. The Django Redis pool immediately below receives the custom CA, so cache access can succeed while the Kombu manager cannot verify the same server, disabling WebSocket event delivery.
---
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