From 4b656b7f9c23843fb5849926fa0b26000fb91c4d Mon Sep 17 00:00:00 2001 From: ali Date: Thu, 17 Sep 2026 12:48:39 +0530 Subject: [PATCH 1/7] UN-4123 [FEAT] Support TLS to Redis, and keep pooled/idle connections healthy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- backend/backend/settings/base.py | 49 +++- backend/sample.env | 27 +++ platform-service/sample.env | 14 ++ runner/sample.env | 27 +++ runner/src/unstract/runner/constants.py | 9 + runner/src/unstract/runner/runner.py | 21 ++ runner/tests/test_sidecar_log_transport.py | 65 ++++++ .../src/unstract/core/cache/redis_client.py | 143 +++++++++++- .../core/tests/test_redis_client_config.py | 217 ++++++++++++++++++ .../unstract/workflow_execution/constants.py | 7 + .../workflow_execution/tools_utils.py | 14 ++ workers/sample.env | 17 ++ 12 files changed, 597 insertions(+), 13 deletions(-) create mode 100644 unstract/core/tests/test_redis_client_config.py diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 1a923bc9fc..2cd43ba083 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -104,6 +104,12 @@ def get_required_setting(setting_key: str, default: str | None = None) -> str | REDIS_HOST = os.environ.get("REDIS_HOST", "localhost") REDIS_PORT = os.environ.get("REDIS_PORT", "6379") REDIS_DB = os.environ.get("REDIS_DB", "") +# TLS to Redis (UN-4123). Off by default, so the in-cluster/local server is +# untouched. `rediss://` is what actually selects TLS for both django-redis and +# kombu; this flag only decides which scheme gets built. +REDIS_SSL = os.environ.get("REDIS_SSL", "false").strip().lower() == "true" +REDIS_SSL_CERT_REQS = os.environ.get("REDIS_SSL_CERT_REQS", "required") +REDIS_SSL_CA_CERTS = os.environ.get("REDIS_SSL_CA_CERTS", "").strip() SESSION_EXPIRATION_TIME_IN_SECOND = os.environ.get( "SESSION_EXPIRATION_TIME_IN_SECOND", 3600 ) @@ -565,20 +571,45 @@ def filter(self, record): _cred_prefix = f"{quote(REDIS_USER, safe='')}:{quote(REDIS_PASSWORD, safe='')}@" elif REDIS_PASSWORD: _cred_prefix = f":{quote(REDIS_PASSWORD, safe='')}@" - SOCKET_IO_MANAGER_URL = f"redis://{_cred_prefix}{REDIS_HOST}:{REDIS_PORT}" + _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}" + ) SOCKET_IO_TRANSPORT_OPTIONS = {} + # django-redis 5.4.0 reads only PASSWORD (plus timeouts) out of OPTIONS — its + # ConnectionFactory.make_connection_params ignores USERNAME and DB entirely. + # So the db has to travel in the URL path, or this cache silently sits on db 0 + # while every other service honours REDIS_DB: workers would RPUSH + # log_history_queue to db N and the backend would LPOP an empty db 0. + # USERNAME is deliberately NOT restored: auth is password-only as the built-in + # `default` user (what managed AUTH strings are), and sending a username turns + # AUTH into its two-argument ACL form. + _cache_options = { + "CLIENT_CLASS": "django_redis.client.DefaultClient", + "SERIALIZER": "django_redis.serializers.json.JSONSerializer", + "DB": _cache_db, + "USERNAME": REDIS_USER, + "PASSWORD": REDIS_PASSWORD, + } + if REDIS_SSL: + _pool_kwargs = {"ssl_cert_reqs": REDIS_SSL_CERT_REQS} + if REDIS_SSL_CA_CERTS: + _pool_kwargs["ssl_ca_certs"] = REDIS_SSL_CA_CERTS + _cache_options["CONNECTION_POOL_KWARGS"] = _pool_kwargs + CACHES = { "default": { "BACKEND": "django_redis.cache.RedisCache", - "LOCATION": f"redis://{REDIS_HOST}:{REDIS_PORT}", - "OPTIONS": { - "CLIENT_CLASS": "django_redis.client.DefaultClient", - "SERIALIZER": "django_redis.serializers.json.JSONSerializer", - "DB": int(REDIS_DB) if REDIS_DB else 0, - "USERNAME": REDIS_USER, - "PASSWORD": REDIS_PASSWORD, - }, + "LOCATION": f"{_scheme}://{REDIS_HOST}:{REDIS_PORT}/{_cache_db}", + "OPTIONS": _cache_options, "KEY_FUNCTION": "utils.redis_cache.custom_key_function", } } diff --git a/backend/sample.env b/backend/sample.env index 700422d1a4..f63235062e 100644 --- a/backend/sample.env +++ b/backend/sample.env @@ -53,6 +53,33 @@ REDIS_RETRY_BACKOFF_FACTOR=0.5 REDIS_SENTINEL_MODE=False REDIS_SENTINEL_MASTER_NAME=mymaster +# Managed / external Redis with TLS (UN-4123). All optional — unset means the +# plaintext connection this file otherwise describes, unchanged. +# +# Two ways to configure it, pick one: +# 1. Discrete vars (what the Helm chart and these samples use). Set REDIS_SSL=true +# alongside REDIS_HOST/REDIS_PORT. No URL-encoding to get wrong. +# 2. REDIS_URL, where the SCHEME carries TLS and nothing else is needed: +# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# A URL wins over the discrete vars above. Percent-encode @ / + in the password. +# +# Auth is password-only, as Redis's built-in `default` user — that is what a managed +# AUTH string is. Leave REDIS_USER empty for a managed endpoint; named ACL users are +# not supported platform-wide. +# +# REDIS_SSL_CA_CERTS is only needed when the server's CA is not in the system trust +# store (Memorystore). ElastiCache and Azure Cache chain to public CAs. +# REDIS_SSL=false +# REDIS_SSL_CERT_REQS=required +# REDIS_SSL_CA_CERTS= +# REDIS_URL= + +# Proactive health check for pooled connections, in seconds (0 disables). +# Managed Redis fails over during maintenance and reaps idle connections — Azure +# Cache closes them at 10 minutes — so a parked connection can be dead before its +# next command. Defaults to 30. +# REDIS_HEALTH_CHECK_INTERVAL=30 + # Connector OAuth SOCIAL_AUTH_EXTRA_DATA_EXPIRATION_TIME_IN_SECOND=3600 GOOGLE_OAUTH2_KEY= diff --git a/platform-service/sample.env b/platform-service/sample.env index 72ea02d195..b6ad62985c 100644 --- a/platform-service/sample.env +++ b/platform-service/sample.env @@ -14,6 +14,20 @@ REDIS_PASSWORD= REDIS_SENTINEL_MODE=False REDIS_SENTINEL_MASTER_NAME=mymaster +# Managed / external Redis with TLS (UN-4123). Optional — unset keeps the plaintext +# connection above. Either set REDIS_SSL=true beside REDIS_HOST/REDIS_PORT, or give a +# full URL whose scheme carries TLS: +# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# A URL wins over the discrete vars. Auth is password-only as the built-in `default` +# user (what a managed AUTH string is), so leave the username empty for a managed +# endpoint. REDIS_SSL_CA_CERTS is only needed when the server's CA is not publicly +# trusted (Memorystore). +# REDIS_SSL=false +# REDIS_SSL_CERT_REQS=required +# REDIS_SSL_CA_CERTS= +# REDIS_URL= +# REDIS_HEALTH_CHECK_INTERVAL=30 + # Backend DB PG_BE_HOST=unstract-db PG_BE_PORT=5432 diff --git a/runner/sample.env b/runner/sample.env index afb58aae15..a970b16d45 100644 --- a/runner/sample.env +++ b/runner/sample.env @@ -48,6 +48,33 @@ REDIS_PASSWORD= REDIS_SENTINEL_MODE=False REDIS_SENTINEL_MASTER_NAME=mymaster +# Managed / external Redis with TLS (UN-4123). All optional — unset means the +# plaintext connection this file otherwise describes, unchanged. +# +# Two ways to configure it, pick one: +# 1. Discrete vars (what the Helm chart and these samples use). Set REDIS_SSL=true +# alongside REDIS_HOST/REDIS_PORT. No URL-encoding to get wrong. +# 2. REDIS_URL, where the SCHEME carries TLS and nothing else is needed: +# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# A URL wins over the discrete vars above. Percent-encode @ / + in the password. +# +# Auth is password-only, as Redis's built-in `default` user — that is what a managed +# AUTH string is. Leave REDIS_USER empty for a managed endpoint; named ACL users are +# not supported platform-wide. +# +# REDIS_SSL_CA_CERTS is only needed when the server's CA is not in the system trust +# store (Memorystore). ElastiCache and Azure Cache chain to public CAs. +# REDIS_SSL=false +# REDIS_SSL_CERT_REQS=required +# REDIS_SSL_CA_CERTS= +# REDIS_URL= + +# Proactive health check for pooled connections, in seconds (0 disables). +# Managed Redis fails over during maintenance and reaps idle connections — Azure +# Cache closes them at 10 minutes — so a parked connection can be dead before its +# next command. Defaults to 30. +# REDIS_HEALTH_CHECK_INTERVAL=30 + # Flask related envs # Can be 'production' or 'development' FLASK_ENV=production diff --git a/runner/src/unstract/runner/constants.py b/runner/src/unstract/runner/constants.py index e4f7449fb7..166f3bdcda 100644 --- a/runner/src/unstract/runner/constants.py +++ b/runner/src/unstract/runner/constants.py @@ -41,6 +41,15 @@ class Env: REDIS_PASSWORD = "REDIS_PASSWORD" REDIS_SENTINEL_MODE = "REDIS_SENTINEL_MODE" REDIS_SENTINEL_MASTER_NAME = "REDIS_SENTINEL_MASTER_NAME" + # UN-4123. Same allowlist trap as LOG_TRANSPORT below: the sidecar publishes tool + # logs through LogPublisher and builds its own Redis client, so without these it + # would keep connecting in PLAINTEXT to db 0 while every other process moved to + # TLS — a connection failure at best, and tool logs silently absent at worst. + REDIS_DB = "REDIS_DB" + REDIS_SSL = "REDIS_SSL" + REDIS_SSL_CERT_REQS = "REDIS_SSL_CERT_REQS" + REDIS_SSL_CA_CERTS = "REDIS_SSL_CA_CERTS" + REDIS_URL = "REDIS_URL" CELERY_BROKER_BASE_URL = "CELERY_BROKER_BASE_URL" CELERY_BROKER_USER = "CELERY_BROKER_USER" CELERY_BROKER_PASS = "CELERY_BROKER_PASS" diff --git a/runner/src/unstract/runner/runner.py b/runner/src/unstract/runner/runner.py index 2414ac7548..bc538021ac 100644 --- a/runner/src/unstract/runner/runner.py +++ b/runner/src/unstract/runner/runner.py @@ -257,6 +257,27 @@ def _get_sidecar_container_config( ), "CONTAINER_NAME": container_name, } + # UN-4123: TLS and db settings for the sidecar's OWN Redis client. It + # publishes tool logs through LogPublisher and tracks tool status in Redis, + # and this dict is a hand-picked allowlist rather than inherited env — the + # same trap that made LOG_TRANSPORT above necessary. Without these the + # sidecar would keep connecting in plaintext to db 0 after everything else + # moved to TLS. + # + # Set only when present, so an unset var leaves the sidecar's environment + # exactly as it is today rather than introducing an empty-string value — + # which os.getenv() in the sidecar would treat as configured. + for _redis_env in ( + Env.REDIS_DB, + Env.REDIS_SSL, + Env.REDIS_SSL_CERT_REQS, + Env.REDIS_SSL_CA_CERTS, + Env.REDIS_URL, + ): + _redis_value = os.getenv(_redis_env) + if _redis_value: + sidecar_env[_redis_env] = _redis_value + sidecar_config = self.client.get_container_run_config( command=[], file_execution_id=file_execution_id, diff --git a/runner/tests/test_sidecar_log_transport.py b/runner/tests/test_sidecar_log_transport.py index 01c46280b1..72e668e0cd 100644 --- a/runner/tests/test_sidecar_log_transport.py +++ b/runner/tests/test_sidecar_log_transport.py @@ -82,3 +82,68 @@ def test_celery_broker_still_forwarded_for_the_flag_off_path(self, sidecar_env): # Flag-off must stay intact: the sidecar still publishes over AMQP. envs = sidecar_env(CELERY_BROKER_BASE_URL="amqp://x") assert envs["CELERY_BROKER_BASE_URL"] == "amqp://x" + + +class TestSidecarRedisTls: + """TLS settings must reach the sidecar too (UN-4123). + + Same allowlist trap as ``LOG_TRANSPORT`` above, one layer deeper: the sidecar + builds its OWN Redis client, so when the platform moves to a TLS endpoint a + sidecar that never learned about it keeps dialling plaintext. The failure is a + connection error at best; at worst it reaches a different db and tool logs go + missing with everything else looking healthy. + """ + + def test_tls_settings_are_forwarded(self, sidecar_env): + envs = sidecar_env( + REDIS_SSL="true", + REDIS_SSL_CERT_REQS="required", + REDIS_SSL_CA_CERTS="/etc/ssl/redis-ca.pem", + ) + assert envs[Env.REDIS_SSL] == "true" + assert envs[Env.REDIS_SSL_CERT_REQS] == "required" + assert envs[Env.REDIS_SSL_CA_CERTS] == "/etc/ssl/redis-ca.pem" + + def test_db_is_forwarded(self, sidecar_env): + """Without this the sidecar sits on db 0 while everyone else honours REDIS_DB. + + Nothing errors: it simply publishes into a keyspace no consumer drains. + """ + envs = sidecar_env(REDIS_DB="3") + assert envs[Env.REDIS_DB] == "3" + + def test_url_is_forwarded(self, sidecar_env): + envs = sidecar_env(REDIS_URL="rediss://cache.example:6380/2") + assert envs[Env.REDIS_URL] == "rediss://cache.example:6380/2" + + def test_unset_values_are_omitted_entirely(self, sidecar_env, monkeypatch): + """An empty string is NOT the same as absent. + + ``os.getenv(key, fallback)`` returns "" for a key that exists but is empty, + which suppresses the fallback — so forwarding blanks would turn "inherit the + default" into "explicitly configured as nothing". + """ + for key in ( + Env.REDIS_SSL, + Env.REDIS_SSL_CERT_REQS, + Env.REDIS_SSL_CA_CERTS, + Env.REDIS_DB, + Env.REDIS_URL, + ): + monkeypatch.delenv(key, raising=False) + envs = sidecar_env() + for key in ( + Env.REDIS_SSL, + Env.REDIS_SSL_CERT_REQS, + Env.REDIS_SSL_CA_CERTS, + Env.REDIS_DB, + Env.REDIS_URL, + ): + assert key not in envs + + def test_plaintext_deployment_is_unchanged(self, sidecar_env, monkeypatch): + """The whole point: no TLS configured means the sidecar env is as it was.""" + monkeypatch.delenv(Env.REDIS_SSL, raising=False) + envs = sidecar_env(REDIS_HOST="r", REDIS_PORT="6379") + assert envs["REDIS_HOST"] == "r" + assert Env.REDIS_SSL not in envs diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index c392d6a4b2..0e7214f4a7 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -5,6 +5,12 @@ - Sentinel mode: Sentinel.master_for() when REDIS_SENTINEL_MODE=True Mode is detected from {prefix}SENTINEL_MODE env var (LLMW pattern). + +A full URL in {prefix}URL (falling back to REDIS_URL) overrides the discrete +host/port/credential vars, and `rediss://` turns on TLS by itself — the scheme is +the switch, so there is no separate "use TLS" flag to forget. Discrete vars remain +the default and primary path: they need no URL-encoding of passwords, and they are +what the Helm chart and every sample.env configure. In Sentinel mode, REDIS_HOST/REDIS_PORT point to the K8s Sentinel service endpoint. Master name defaults to "mymaster" (configurable via REDIS_SENTINEL_MASTER_NAME env var). REDIS_PASSWORD is reused for Sentinel auth. @@ -17,6 +23,7 @@ import random import time from typing import Any +from urllib.parse import urlsplit, urlunsplit import redis from redis.sentinel import Sentinel @@ -32,6 +39,45 @@ _SENTINEL_JITTER_MAX = 1.2 _DEFAULT_SENTINEL_MASTER_NAME = os.getenv("REDIS_SENTINEL_MASTER_NAME", "mymaster") +# redis-py sends a PING before reusing a connection idle for longer than this, so a +# connection killed while parked (managed-Redis failover, an idle-connection reaper — +# Azure Cache closes at 10 minutes) is discovered and replaced by the health check +# rather than by the next real command failing. 30s is redis-py's own documented +# recommendation. 0 disables it, which is what every client except the two worker +# caches used before UN-4123. +_DEFAULT_HEALTH_CHECK_INTERVAL = 30 + + +def _resolve_health_check_interval(env_prefix: str, explicit: int) -> int: + """Explicit argument wins; otherwise env, otherwise the default above.""" + if explicit: + return explicit + raw = os.getenv( + f"{env_prefix}HEALTH_CHECK_INTERVAL", + os.getenv("REDIS_HEALTH_CHECK_INTERVAL", str(_DEFAULT_HEALTH_CHECK_INTERVAL)), + ) + try: + return max(int(raw), 0) + except ValueError: + logger.warning( + "Invalid %sHEALTH_CHECK_INTERVAL=%r; using %s", + env_prefix, + raw, + _DEFAULT_HEALTH_CHECK_INTERVAL, + ) + return _DEFAULT_HEALTH_CHECK_INTERVAL + + +def _strip_url_db_path(url: str) -> str: + """Drop the / path from a Redis URL. + + redis-py resolves the db from the URL path and IGNORES a `db=` kwarg, so a + caller that asks for a specific db (sdk1 metrics uses db=1) would silently get + the URL's db instead. Stripping the path lets the explicit argument apply. + """ + parts = urlsplit(url) + return urlunsplit((parts.scheme, parts.netloc, "", parts.query, parts.fragment)) + def _is_sentinel_mode(env_prefix: str) -> bool: return os.getenv(f"{env_prefix}SENTINEL_MODE", "False").strip().lower() == "true" @@ -59,8 +105,11 @@ def create_redis_client( socket_connect_timeout: Connection timeout in seconds. socket_timeout: Socket timeout in seconds. max_connections: Optional max connections for ConnectionPool. - health_check_interval: Proactive health check interval in seconds (0=disabled). - db: Optional DB index override. + health_check_interval: Proactive health check interval in seconds. 0 means + "unset" and falls back to {env_prefix}HEALTH_CHECK_INTERVAL, then + REDIS_HEALTH_CHECK_INTERVAL, then 30s; set that env to 0 to disable. + db: Optional DB index override. Applied even when a URL carries its own + db path, which redis-py would otherwise silently prefer. Returns: Configured redis.Redis client (standalone or Sentinel-backed). @@ -68,6 +117,9 @@ def create_redis_client( Raises: RedisSentinelConnectionError: After exhausting retries in Sentinel mode. """ + health_check_interval = _resolve_health_check_interval( + env_prefix, health_check_interval + ) if _is_sentinel_mode(env_prefix): return _create_sentinel_client( env_prefix=env_prefix, @@ -106,7 +158,14 @@ def _resolve_redis_env( if db_override is not None else int(os.getenv(f"{env_prefix}DB", os.getenv("REDIS_DB", "0"))) ) - ssl = os.getenv(f"{env_prefix}SSL", "false").strip().lower() == "true" + # Falls back to REDIS_SSL: before UN-4123 each prefix needed its own *_SSL, so + # turning TLS on platform-wide meant remembering CACHE_REDIS_SSL and + # MANUAL_REVIEW_REDIS_SSL too — and a missed one fails as a plaintext client + # talking to a TLS port, not as a config error. + ssl = ( + os.getenv(f"{env_prefix}SSL", os.getenv("REDIS_SSL", "false")).strip().lower() + == "true" + ) result: dict[str, Any] = { "host": host, "port": port, @@ -114,9 +173,21 @@ def _resolve_redis_env( "username": username, "db": db, "ssl": ssl, + # A full URL, when given, is authoritative for host/port/credentials/db and + # carries TLS in its scheme (rediss://). Everything above stays the default + # path, so an unset URL changes nothing. + "url": os.getenv(f"{env_prefix}URL", os.getenv("REDIS_URL", "")).strip(), } 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 return result @@ -162,6 +233,8 @@ def _build_connection_kwargs( if env.get("ssl"): kwargs["ssl"] = True kwargs["ssl_cert_reqs"] = env.get("ssl_cert_reqs", "required") + if env.get("ssl_ca_certs"): + kwargs["ssl_ca_certs"] = env["ssl_ca_certs"] return kwargs @@ -176,6 +249,18 @@ def _create_standalone_client( ) -> redis.Redis: env = _resolve_redis_env(env_prefix, default_port="6379", db_override=db_override) + if env["url"]: + return _create_client_from_url( + url=env["url"], + decode_responses=decode_responses, + socket_connect_timeout=socket_connect_timeout, + socket_timeout=socket_timeout, + max_connections=max_connections, + health_check_interval=health_check_interval, + db_override=db_override, + ssl_ca_certs=env.get("ssl_ca_certs"), + ) + logger.info( "Redis standalone mode enabled. Connecting to %s:%s", env["host"], env["port"] ) @@ -191,12 +276,62 @@ def _create_standalone_client( kwargs["port"] = env["port"] if max_connections is not None: - pool = redis.ConnectionPool(max_connections=max_connections, **kwargs) + pool_kwargs = dict(kwargs) + # ConnectionPool hands its kwargs to the connection class, and the plain + # Connection has no `ssl` parameter — passing it raises TypeError. TLS on a + # POOLED client (platform-service sets max_connections) therefore has to be + # selected by connection class, not by a flag. + if pool_kwargs.pop("ssl", False): + pool_kwargs["connection_class"] = redis.SSLConnection + pool = redis.ConnectionPool(max_connections=max_connections, **pool_kwargs) return redis.Redis(connection_pool=pool) return redis.Redis(**kwargs) +def _create_client_from_url( + url: str, + decode_responses: bool, + socket_connect_timeout: int, + socket_timeout: int, + max_connections: int | None, + health_check_interval: int, + db_override: int | None, + ssl_ca_certs: str | None, +) -> redis.Redis: + """Build a client from a full Redis URL. + + `rediss://` selects TLS on its own — redis-py picks SSLConnection from the + scheme — so TLS needs no separate switch, and `redis://` behaves exactly as the + discrete host/port path does. TLS verification is tuned in the URL itself, e.g. + `?ssl_cert_reqs=required`. + """ + kwargs: dict[str, Any] = { + "decode_responses": decode_responses, + "socket_connect_timeout": socket_connect_timeout, + "socket_timeout": socket_timeout, + } + if health_check_interval: + kwargs["health_check_interval"] = health_check_interval + if max_connections is not None: + kwargs["max_connections"] = max_connections + if ssl_ca_certs and url.startswith("rediss://"): + kwargs["ssl_ca_certs"] = ssl_ca_certs + if db_override is not None: + # The URL path wins over a db kwarg in redis-py, so it has to go. + url = _strip_url_db_path(url) + kwargs["db"] = db_override + + parts = urlsplit(url) + logger.info( + "Redis URL mode enabled. Connecting to %s:%s (tls=%s)", + parts.hostname, + parts.port, + parts.scheme == "rediss", + ) + return redis.Redis.from_url(url, **kwargs) + + def _create_sentinel_client( env_prefix: str, decode_responses: bool, diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py new file mode 100644 index 0000000000..5d54618df4 --- /dev/null +++ b/unstract/core/tests/test_redis_client_config.py @@ -0,0 +1,217 @@ +"""Connection configuration for the shared Redis client factory (UN-4123). + +These tests build clients and inspect the resulting connection kwargs. Nothing +connects to a server: every property under test is decided at construction, which +is exactly where the bugs being locked down lived. + +The recurring theme is that a MISCONFIGURED Redis client fails quietly. A missing +TLS flag is a plaintext socket against a TLS port; an ignored db override is reads +and writes against the wrong keyspace; an empty-string env var counts as "set" to +``os.getenv`` and shadows the fallback that would have supplied the real value. +None of those raise at import, so they are asserted here instead. +""" + +import pytest +import redis + +from unstract.core.cache.redis_client import create_redis_client + +_REDIS_ENV_MARKERS = ("REDIS_", "CACHE_REDIS_", "MANUAL_REVIEW_REDIS_") + + +@pytest.fixture(autouse=True) +def _clean_redis_env(monkeypatch): + """Drop inherited REDIS_* so a developer's shell cannot change the outcome.""" + import os + + for key in list(os.environ): + if any(marker in key for marker in _REDIS_ENV_MARKERS): + monkeypatch.delenv(key, raising=False) + + +def _kwargs(client: redis.Redis) -> dict: + return client.connection_pool.connection_kwargs + + +def _connection_class(client: redis.Redis) -> str: + return client.connection_pool.connection_class.__name__ + + +class TestDefaults: + def test_no_env_is_plaintext_localhost_db0(self): + """The in-cluster/local path: unchanged by everything else in this suite.""" + client = create_redis_client() + assert _connection_class(client) == "Connection" + assert _kwargs(client)["host"] == "localhost" + assert _kwargs(client)["port"] == 6379 + assert _kwargs(client)["db"] == 0 + assert "ssl_cert_reqs" not in _kwargs(client) + + def test_health_check_is_on_by_default(self, monkeypatch): + """Guards against a connection killed while idle. + + A managed Redis fails over during maintenance and reaps idle connections + (Azure Cache at 10 minutes). Without a health check the pooled connection + is only discovered dead when a real command fails on it. + """ + assert _kwargs(create_redis_client())["health_check_interval"] == 30 + + monkeypatch.setenv("REDIS_HEALTH_CHECK_INTERVAL", "0") + # redis-py always populates this key; 0 is its "disabled" value. + assert _kwargs(create_redis_client())["health_check_interval"] == 0 + + def test_explicit_argument_beats_env(self, monkeypatch): + monkeypatch.setenv("REDIS_HEALTH_CHECK_INTERVAL", "45") + assert _kwargs(create_redis_client(health_check_interval=7))[ + "health_check_interval" + ] == 7 + + def test_unparseable_health_check_falls_back(self, monkeypatch): + """A typo must not take the platform's Redis down.""" + monkeypatch.setenv("REDIS_HEALTH_CHECK_INTERVAL", "thirty") + assert _kwargs(create_redis_client())["health_check_interval"] == 30 + + +class TestDiscreteTLS: + def test_ssl_selects_a_tls_connection(self, monkeypatch): + monkeypatch.setenv("REDIS_HOST", "cache.internal") + monkeypatch.setenv("REDIS_SSL", "true") + client = create_redis_client() + assert _connection_class(client) == "SSLConnection" + assert _kwargs(client)["ssl_cert_reqs"] == "required" + + def test_pooled_tls_client_is_usable(self, monkeypatch): + """Regression: TLS + max_connections used to fail on the FIRST COMMAND. + + ``ConnectionPool`` hands its kwargs to the connection class, and the plain + ``Connection`` has no ``ssl`` parameter. Construction succeeded, the pool + kept the non-TLS class, and the first command raised + ``TypeError: AbstractConnection.__init__() got an unexpected keyword + argument 'ssl'`` — so platform-service (max_connections=10) would have + started healthy and broken on first use. + """ + monkeypatch.setenv("REDIS_SSL", "true") + client = create_redis_client(max_connections=10) + pool = client.connection_pool + assert _connection_class(client) == "SSLConnection" + assert "ssl" not in _kwargs(client) + # Instantiating the connection is the step that used to raise; redis-py + # opens the socket lazily, so this stays offline. + assert pool.connection_class(**pool.connection_kwargs) is not None + + def test_prefixed_client_inherits_the_global_ssl_flag(self, monkeypatch): + """CACHE_REDIS_* and MANUAL_REVIEW_* must not need their own SSL flag. + + Before this fell back, enabling TLS platform-wide meant remembering every + prefix, and a forgotten one is a plaintext client against a TLS port. + """ + monkeypatch.setenv("REDIS_SSL", "true") + for prefix in ("CACHE_REDIS_", "MANUAL_REVIEW_REDIS_"): + assert _connection_class(create_redis_client(env_prefix=prefix)) == ( + "SSLConnection" + ) + + def test_prefix_can_still_override_the_global_flag(self, monkeypatch): + monkeypatch.setenv("REDIS_SSL", "true") + monkeypatch.setenv("CACHE_REDIS_SSL", "false") + assert _connection_class(create_redis_client(env_prefix="CACHE_REDIS_")) == ( + "Connection" + ) + + def test_ca_certs_reach_the_connection(self, monkeypatch): + """Needed for Memorystore, whose CA is not in the system trust store.""" + monkeypatch.setenv("REDIS_SSL", "true") + monkeypatch.setenv("REDIS_SSL_CA_CERTS", "/etc/ssl/redis-ca.pem") + assert _kwargs(create_redis_client())["ssl_ca_certs"] == "/etc/ssl/redis-ca.pem" + + def test_ca_certs_ignored_without_tls(self, monkeypatch): + monkeypatch.setenv("REDIS_SSL_CA_CERTS", "/etc/ssl/redis-ca.pem") + assert "ssl_ca_certs" not in _kwargs(create_redis_client()) + + +class TestUrlMode: + def test_rediss_scheme_turns_on_tls_without_a_flag(self, monkeypatch): + monkeypatch.setenv( + "REDIS_URL", "rediss://cache.example:6380/2?ssl_cert_reqs=required" + ) + client = create_redis_client() + assert _connection_class(client) == "SSLConnection" + assert _kwargs(client)["ssl_cert_reqs"] == "required" + assert _kwargs(client)["db"] == 2 + + def test_plain_scheme_stays_plaintext(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "redis://cache.example:6379") + assert _connection_class(create_redis_client()) == "Connection" + + def test_password_is_url_decoded(self, monkeypatch): + """Generated passwords contain @ / +, so they arrive percent-encoded.""" + monkeypatch.setenv("REDIS_URL", "rediss://:p%40ss%2Fword@cache.example:6380") + assert _kwargs(create_redis_client())["password"] == "p@ss/word" + + def test_explicit_db_argument_beats_the_url_path(self, monkeypatch): + """redis-py lets the URL path win over a ``db`` kwarg — silently. + + sdk1's metrics client asks for db=1 explicitly. Without stripping the path + it would land in the URL's db instead, writing metrics into another + service's keyspace with nothing to indicate it. + """ + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/5") + assert _kwargs(create_redis_client(db=1))["db"] == 1 + + def test_url_db_applies_when_no_override_is_given(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/5") + assert _kwargs(create_redis_client())["db"] == 5 + + def test_url_takes_precedence_over_discrete_vars(self, monkeypatch): + monkeypatch.setenv("REDIS_HOST", "in-cluster") + monkeypatch.setenv("REDIS_URL", "redis://managed.example:6379") + assert _kwargs(create_redis_client())["host"] == "managed.example" + + def test_prefixed_url_is_used_for_that_prefix_only(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "redis://shared.example:6379") + monkeypatch.setenv("CACHE_REDIS_URL", "redis://cache.example:6379") + assert _kwargs(create_redis_client())["host"] == "shared.example" + assert ( + _kwargs(create_redis_client(env_prefix="CACHE_REDIS_"))["host"] + == "cache.example" + ) + + def test_pool_size_survives_url_mode(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380") + client = create_redis_client(max_connections=10) + assert client.connection_pool.max_connections == 10 + assert _connection_class(client) == "SSLConnection" + + +class TestAuth: + def test_password_only_auth_sends_no_username(self, monkeypatch): + """Managed AUTH strings authenticate the built-in ``default`` user. + + Named ACL users are not supported platform-wide — django-redis discards + the username — so an empty REDIS_USER must stay empty rather than + defaulting to something that turns AUTH into its two-argument form. + """ + monkeypatch.setenv("REDIS_PASSWORD", "s3cr3t") + kwargs = _kwargs(create_redis_client()) + assert kwargs["password"] == "s3cr3t" + assert kwargs.get("username") is None + + def test_prefixed_client_inherits_the_global_password(self, monkeypatch): + monkeypatch.setenv("REDIS_PASSWORD", "s3cr3t") + assert _kwargs(create_redis_client(env_prefix="CACHE_REDIS_"))["password"] == ( + "s3cr3t" + ) + + def test_empty_prefixed_password_shadows_the_fallback(self, monkeypatch): + """Documents a trap rather than endorsing it. + + ``os.getenv(key, fallback)`` returns "" when the key exists but is empty, + so an empty CACHE_REDIS_PASSWORD suppresses REDIS_PASSWORD and the client + connects UNAUTHENTICATED. The Helm chart must therefore never render an + empty credential; this test fails if that behaviour ever changes, so the + chart-side guarantee can be revisited. + """ + monkeypatch.setenv("REDIS_PASSWORD", "s3cr3t") + monkeypatch.setenv("CACHE_REDIS_PASSWORD", "") + kwargs = _kwargs(create_redis_client(env_prefix="CACHE_REDIS_")) + assert kwargs.get("password") is None diff --git a/unstract/workflow-execution/src/unstract/workflow_execution/constants.py b/unstract/workflow-execution/src/unstract/workflow_execution/constants.py index 3786706cd2..120604fd6b 100644 --- a/unstract/workflow-execution/src/unstract/workflow_execution/constants.py +++ b/unstract/workflow-execution/src/unstract/workflow_execution/constants.py @@ -28,6 +28,13 @@ class ToolRuntimeVariable: REDIS_PASSWORD = "REDIS_PASSWORD" REDIS_SENTINEL_MODE = "REDIS_SENTINEL_MODE" REDIS_SENTINEL_MASTER_NAME = "REDIS_SENTINEL_MASTER_NAME" + # UN-4123 — TLS settings for tool containers, which build their own Redis + # client (sdk1 metrics). Same allowlist trap as the sidecar's. + REDIS_DB = "REDIS_DB" + REDIS_SSL = "REDIS_SSL" + REDIS_SSL_CERT_REQS = "REDIS_SSL_CERT_REQS" + REDIS_SSL_CA_CERTS = "REDIS_SSL_CA_CERTS" + REDIS_URL = "REDIS_URL" class WorkflowFileType: diff --git a/unstract/workflow-execution/src/unstract/workflow_execution/tools_utils.py b/unstract/workflow-execution/src/unstract/workflow_execution/tools_utils.py index 040c9485cb..14c2a22ca8 100644 --- a/unstract/workflow-execution/src/unstract/workflow_execution/tools_utils.py +++ b/unstract/workflow-execution/src/unstract/workflow_execution/tools_utils.py @@ -240,6 +240,20 @@ def get_tool_environment_variables(self) -> dict[str, Any]: ToolRV.REDIS_SENTINEL_MODE: self.redis_sentinel_mode or "False", ToolRV.REDIS_SENTINEL_MASTER_NAME: self.redis_sentinel_master_name or "mymaster", + # UN-4123: only the keys actually set, so an unset var leaves the tool's + # environment unchanged instead of gaining an empty string that + # os.getenv() would read as configured. + **{ + name: os.environ[name] + for name in ( + ToolRV.REDIS_DB, + ToolRV.REDIS_SSL, + ToolRV.REDIS_SSL_CERT_REQS, + ToolRV.REDIS_SSL_CA_CERTS, + ToolRV.REDIS_URL, + ) + if os.environ.get(name) + }, } # For async LLM Whisperer extraction if self.llmw_poll_interval: diff --git a/workers/sample.env b/workers/sample.env index abb09255f3..85131d8c49 100644 --- a/workers/sample.env +++ b/workers/sample.env @@ -78,6 +78,20 @@ REDIS_DB=0 REDIS_SENTINEL_MODE=False REDIS_SENTINEL_MASTER_NAME=mymaster +# Managed / external Redis with TLS (UN-4123). Optional — unset keeps the plaintext +# connection above. Either set REDIS_SSL=true beside REDIS_HOST/REDIS_PORT, or give a +# full URL whose scheme carries TLS: +# REDIS_URL=rediss://:@:6380/0?ssl_cert_reqs=required +# A URL wins over the discrete vars. Auth is password-only as the built-in `default` +# user (what a managed AUTH string is), so leave the username empty for a managed +# endpoint. REDIS_SSL_CA_CERTS is only needed when the server's CA is not publicly +# trusted (Memorystore). +# REDIS_SSL=false +# REDIS_SSL_CERT_REQS=required +# REDIS_SSL_CA_CERTS= +# REDIS_URL= +# REDIS_HEALTH_CHECK_INTERVAL=30 + # Cache-Specific Redis Configuration CACHE_REDIS_ENABLED=true CACHE_REDIS_HOST=unstract-redis @@ -85,8 +99,11 @@ CACHE_REDIS_PORT=6379 CACHE_REDIS_DB=0 CACHE_REDIS_PASSWORD= CACHE_REDIS_USERNAME= +# Falls back to REDIS_SSL when unset (UN-4123), so TLS does not have to be repeated +# per prefix — a forgotten one is a plaintext client against a TLS port. CACHE_REDIS_SSL=false CACHE_REDIS_SSL_CERT_REQS=required +# CACHE_REDIS_URL= # Cache Redis Sentinel mode for worker execution cache backend # When True: point CACHE_REDIS_HOST to Sentinel K8s service DNS, CACHE_REDIS_PORT to 26379 From bc9e0dde752fa88a9b96e18623c11475c946b362 Mon Sep 17 00:00:00 2001 From: ali Date: Mon, 21 Sep 2026 11:06:19 +0530 Subject: [PATCH 2/7] UN-4123 [FEAT] Add a local TLS Redis for dev testing, and fix the CA gap it found MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docker/docker-compose-redis-tls.yaml | 69 ++++++++++ docker/redis-tls/.gitignore | 2 + docker/redis-tls/README.md | 121 ++++++++++++++++++ .../src/unstract/core/cache/redis_client.py | 20 +-- .../core/tests/test_redis_client_config.py | 17 +++ 5 files changed, 221 insertions(+), 8 deletions(-) create mode 100644 docker/docker-compose-redis-tls.yaml create mode 100644 docker/redis-tls/.gitignore create mode 100644 docker/redis-tls/README.md diff --git a/docker/docker-compose-redis-tls.yaml b/docker/docker-compose-redis-tls.yaml new file mode 100644 index 0000000000..1c2604508d --- /dev/null +++ b/docker/docker-compose-redis-tls.yaml @@ -0,0 +1,69 @@ +# A stand-in for a managed Redis, for local development only (UN-4123). +# +# Managed offerings differ from the plain dev Redis in exactly two ways that our +# code has to cope with: they require AUTH, and they speak TLS. This container +# does both, so the TLS path can be exercised end to end — execution, log +# streaming, tool sidecars — without cloud access or a VPN. +# +# It runs ALONGSIDE the normal `unstract-redis`, on a different port, so nothing +# switches until you point services at it. That is deliberate: the point of the +# exercise is to flip between the two and confirm BOTH work. +# +# ./redis-tls/generate-certs.sh +# docker compose -f docker-compose-redis-tls.yaml up -d +# +# See redis-tls/README.md for the env to set and what to verify. +services: + redis-managed: + image: "redis:7.2.3" + container_name: unstract-redis-managed + restart: unless-stopped + # `--port 0` disables the plaintext listener entirely, which is what Azure + # Cache does by default. A service that fails to pick up the TLS settings + # therefore cannot quietly succeed over plaintext and hide the bug. + # + # `--tls-auth-clients no` mirrors the managed services: the client verifies + # the server, not the other way round. No mTLS, no client certs to ship. + command: > + redis-server + --port 0 + --tls-port 6380 + --tls-cert-file /certs/server.crt + --tls-key-file /certs/server.key + --tls-ca-cert-file /certs/ca.crt + --tls-auth-clients no + --requirepass ${REDIS_DEV_PASSWORD:-devpassword} + --loglevel notice + ports: + # Same port inside and out, so one hostname:port works from containers + # (unstract-redis-managed:6380) and from a backend on the host + # (localhost:6380). + - "6380:6380" + volumes: + - ./redis-tls/certs:/certs:ro + - redis_managed_data:/data + healthcheck: + test: + [ + "CMD", + "redis-cli", + "--tls", + "-p", "6380", + "--cacert", "/certs/ca.crt", + "-a", "${REDIS_DEV_PASSWORD:-devpassword}", + "--no-auth-warning", + "ping", + ] + interval: 10s + timeout: 5s + retries: 5 + labels: + - traefik.enable=false + +volumes: + redis_managed_data: + +networks: + default: + name: unstract-network + external: true diff --git a/docker/redis-tls/.gitignore b/docker/redis-tls/.gitignore new file mode 100644 index 0000000000..e63f64c5cf --- /dev/null +++ b/docker/redis-tls/.gitignore @@ -0,0 +1,2 @@ +# Dev-only self-signed material, regenerated by generate-certs.sh. +certs/ diff --git a/docker/redis-tls/README.md b/docker/redis-tls/README.md new file mode 100644 index 0000000000..3f01f5940a --- /dev/null +++ b/docker/redis-tls/README.md @@ -0,0 +1,121 @@ +# Testing against a managed-like Redis locally (UN-4123) + +A managed Redis (Memorystore, ElastiCache, Azure Cache) differs from the dev Redis +in exactly two ways our code has to handle: it **requires AUTH** and it **speaks +TLS**. `docker-compose-redis-tls.yaml` runs a Redis that does both, so the TLS path +can be exercised without cloud access or a VPN. + +It runs **alongside** the normal `unstract-redis` on port 6380, so nothing switches +until you point services at it — the point of the exercise is to flip between the +two and confirm both work. + +Its plaintext listener is disabled (`--port 0`), the same as Azure Cache's default. +A service that fails to pick up the TLS settings therefore *cannot* quietly succeed +over plaintext and hide the bug. + +## Start it + +```bash +cd docker +./redis-tls/generate-certs.sh +docker compose -f docker-compose-redis-tls.yaml up -d +``` + +Certificates are self-signed and dev-only. The SAN list covers both names the same +server answers to — `unstract-redis-managed` from inside the network, +`localhost` from a backend on the host — because a certificate valid for only one +fails verification from the other side, which looks like a code bug and is not one. + +Confirm it is up, and that plaintext is refused: + +```bash +docker exec unstract-redis-managed redis-cli --tls -p 6380 \ + --cacert /certs/ca.crt -a devpassword --no-auth-warning ping # PONG +docker exec unstract-redis-managed redis-cli -p 6380 \ + -a devpassword --no-auth-warning ping # I/O error +``` + +## Switch the platform onto it + +Two values differ by where the process runs: containers reach it as +`unstract-redis-managed:6380`, a backend on the host as `localhost:6380`. + +**`backend/.env`** (runs on the host): + +```bash +REDIS_URL=rediss://:devpassword@localhost:6380/0?ssl_cert_reqs=required +REDIS_SSL_CA_CERTS=/abs/path/to/unstract/docker/redis-tls/certs/ca.crt +``` + +**`workers/.env`, `platform-service/.env`** (containers): + +```bash +REDIS_URL=rediss://:devpassword@unstract-redis-managed:6380/0?ssl_cert_reqs=required +REDIS_SSL_CA_CERTS=/certs/ca.crt +CACHE_REDIS_URL=rediss://:devpassword@unstract-redis-managed:6380/1?ssl_cert_reqs=required +``` + +Those containers need the CA mounted. Add to your `docker/compose.override.yaml`: + +```yaml +services: + worker-pg-orchestrator-general: # repeat for each worker + platform-service + volumes: + - ./redis-tls/certs:/certs:ro +``` + +**`runner/.env`** — one deliberate difference: + +```bash +REDIS_URL=rediss://:devpassword@unstract-redis-managed:6380/0 +REDIS_SSL_CERT_REQS=none +``` + +The runner forwards these to each **tool sidecar** it spawns, and sidecars get no +CA mount — the runner builds their environment as an allowlist and mounts only the +shared log dir. `none` keeps the connection encrypted while skipping verification, +which still exercises the forwarding fix and the TLS handshake. Against a real +managed endpoint this does not arise: ElastiCache and Azure chain to public CAs, and +for Memorystore you would mount its CA into the sidecar image. + +Then restart: `docker compose restart` for the containers, and your usual manual +restart for the backend. + +## What to check + +1. **An execution completes.** Include a workflow that uses a **container-based + tool** (classifier or text_extractor) — a Prompt Studio structure tool runs + in-process in the executor and never spawns a sidecar, so it leaves the whole + sidecar path untested. +2. **Logs stream in the UI and land in `execution_log`.** That is the Redis-list + transport end to end, the part with no unit-test coverage. +3. **The keys are in the new server, and the old one is idle:** + + ```bash + docker exec unstract-redis-managed redis-cli --tls -p 6380 \ + --cacert /certs/ca.crt -a devpassword --no-auth-warning dbsize + docker exec unstract-redis redis-cli dbsize # should not be growing + ``` + +4. **Nothing fell back to plaintext.** The TLS-only listener makes this + self-enforcing: a component that missed the settings fails loudly instead. + +## Switch back + +Remove the `REDIS_URL` / `REDIS_SSL_*` lines, restart, and run the same execution +again. Both modes are supported and both must pass — that is the acceptance bar, +not just "TLS works". + +## Against a real managed Redis + +Same env, different endpoint. Memorystore is VPC-private, so from a laptop it needs +a tunnel: + +```bash +gcloud compute ssh -- -L 6380::6379 +REDIS_URL=rediss://:@localhost:6380/0?ssl_cert_reqs=required +``` + +Drop the `rediss` to `redis` when the instance has TLS disabled — an AUTH string +without in-transit encryption is a supported configuration and worth testing too, +since it is what a VPC-internal deployment may well run. diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index 0e7214f4a7..1f536cafdf 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -178,16 +178,20 @@ def _resolve_redis_env( # path, so an unset URL changes nothing. "url": os.getenv(f"{env_prefix}URL", os.getenv("REDIS_URL", "")).strip(), } + # Read OUTSIDE the `if ssl` below: URL mode carries TLS in the scheme and never + # sets {prefix}SSL, so gating the CA on that flag left `rediss://` verifying + # against the system trust store alone — which fails for exactly the servers + # that need a CA. Consumers decide whether it applies. + # + # Needed where the server's CA is not publicly trusted — notably Memorystore, + # whose CA is Google-managed. ElastiCache and Azure chain to public CAs. + 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 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 return result diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index 5d54618df4..eb3aa3871d 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -176,6 +176,23 @@ def test_prefixed_url_is_used_for_that_prefix_only(self, monkeypatch): == "cache.example" ) + def test_ca_certs_apply_in_url_mode(self, monkeypatch): + """Found by a live run against a TLS Redis, not by reading the code. + + URL mode carries TLS in the scheme and never sets REDIS_SSL, so while the + CA was read only inside that flag's branch, `rediss://` verified against + the system trust store alone — and failed for precisely the servers a CA + is needed for (Memorystore's CA is not publicly trusted). + """ + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/0") + monkeypatch.setenv("REDIS_SSL_CA_CERTS", "/etc/ssl/redis-ca.pem") + assert _kwargs(create_redis_client())["ssl_ca_certs"] == "/etc/ssl/redis-ca.pem" + + def test_ca_certs_ignored_for_a_plaintext_url(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "redis://cache.example:6379/0") + monkeypatch.setenv("REDIS_SSL_CA_CERTS", "/etc/ssl/redis-ca.pem") + assert "ssl_ca_certs" not in _kwargs(create_redis_client()) + def test_pool_size_survives_url_mode(self, monkeypatch): monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380") client = create_redis_client(max_connections=10) From c368cff86780ba14497508ceedced2eed8de9f8c Mon Sep 17 00:00:00 2001 From: ali Date: Mon, 21 Sep 2026 17:11:46 +0530 Subject: [PATCH 3/7] UN-4123 [FEAT] Ship generate-certs.sh, which .gitignore silently dropped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docker/redis-tls/generate-certs.sh | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100755 docker/redis-tls/generate-certs.sh diff --git a/docker/redis-tls/generate-certs.sh b/docker/redis-tls/generate-certs.sh new file mode 100755 index 0000000000..f43e260e36 --- /dev/null +++ b/docker/redis-tls/generate-certs.sh @@ -0,0 +1,44 @@ +#!/usr/bin/env bash +# Self-signed CA + server certificate for the local TLS Redis (UN-4123). +# +# Only for local development: it stands in for a managed Redis endpoint so the +# TLS path can be exercised without cloud access or a VPN. Never use these +# anywhere else — the private keys are written unencrypted, right here. +# +# The SAN list covers both names the same server answers to: containers reach it +# as `unstract-redis-managed`, while a backend running on the host reaches the +# published port as `localhost`. A certificate valid for only one of them fails +# verification from the other side, which looks like a code bug and is not one. +set -euo pipefail + +CERT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)/certs" +mkdir -p "$CERT_DIR" +cd "$CERT_DIR" + +if [[ -f server.crt && "${FORCE:-}" != "1" ]]; then + echo "Certificates already exist in $CERT_DIR (FORCE=1 to regenerate)." + exit 0 +fi + +openssl genrsa -out ca.key 4096 2>/dev/null +openssl req -x509 -new -nodes -key ca.key -sha256 -days 825 -out ca.crt \ + -subj "/CN=Unstract Local Redis Dev CA" 2>/dev/null + +openssl genrsa -out server.key 2048 2>/dev/null +openssl req -new -key server.key -out server.csr \ + -subj "/CN=unstract-redis-managed" 2>/dev/null + +cat > server.ext <<'EXT' +subjectAltName = DNS:unstract-redis-managed, DNS:localhost, IP:127.0.0.1 +extendedKeyUsage = serverAuth +EXT + +openssl x509 -req -in server.csr -CA ca.crt -CAkey ca.key -CAcreateserial \ + -out server.crt -days 825 -sha256 -extfile server.ext 2>/dev/null + +# Redis runs as uid 999 in the official image and must read the key. +chmod 644 ca.crt server.crt server.key +rm -f server.csr server.ext ca.srl + +echo "Wrote CA + server certificate to $CERT_DIR" +echo " CA (for REDIS_SSL_CA_CERTS): $CERT_DIR/ca.crt" From 2570e0f488cf9c0f5da93dfad32ca5e237965bf0 Mon Sep 17 00:00:00 2001 From: ali Date: Mon, 21 Sep 2026 17:22:09 +0530 Subject: [PATCH 4/7] UN-4123 [FIX] Honour REDIS_URL in the Django cache and Socket.IO manager MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- backend/backend/settings/base.py | 35 ++++- .../tests/test_redis_settings_derivation.py | 135 ++++++++++++++++++ 2 files changed, 165 insertions(+), 5 deletions(-) create mode 100644 backend/backend/tests/test_redis_settings_derivation.py diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 2cd43ba083..2fa013f5e4 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -110,6 +110,13 @@ def get_required_setting(setting_key: str, default: str | None = None) -> str | REDIS_SSL = os.environ.get("REDIS_SSL", "false").strip().lower() == "true" REDIS_SSL_CERT_REQS = os.environ.get("REDIS_SSL_CERT_REQS", "required") REDIS_SSL_CA_CERTS = os.environ.get("REDIS_SSL_CA_CERTS", "").strip() +# A full URL overrides the discrete vars above, exactly as it does in +# unstract.core's create_redis_client — otherwise the workers would follow the URL +# while this process stayed on REDIS_HOST, and the two would silently sit on +# DIFFERENT servers. That split is invisible until something written by one side +# is read by the other: an API deployment returns `result: null` because the +# execution's cached result was written to the URL's Redis and looked up here. +REDIS_URL = os.environ.get("REDIS_URL", "").strip() SESSION_EXPIRATION_TIME_IN_SECOND = os.environ.get( "SESSION_EXPIRATION_TIME_IN_SECOND", 3600 ) @@ -574,12 +581,26 @@ def filter(self, record): _scheme = "rediss" if REDIS_SSL else "redis" _cache_db = int(REDIS_DB) if REDIS_DB else 0 + # Both django-redis (via redis-py) and kombu read TLS settings out of the URL's + # query string, so one string configures both — verified against the pinned + # versions. The CA is appended rather than required in the URL, since it is a + # local path rather than part of the endpoint's identity. + _redis_url = REDIS_URL + if ( + _redis_url + and REDIS_SSL_CA_CERTS + and _redis_url.startswith("rediss://") + and "ssl_ca_certs=" not in _redis_url + ): + _sep = "&" if "?" in _redis_url else "?" + _redis_url = f"{_redis_url}{_sep}ssl_ca_certs={quote(REDIS_SSL_CA_CERTS, safe='/')}" + # 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 = ( + SOCKET_IO_MANAGER_URL = _redis_url or ( f"{_scheme}://{_cred_prefix}{REDIS_HOST}:{REDIS_PORT}{_socketio_tls_query}" ) SOCKET_IO_TRANSPORT_OPTIONS = {} @@ -595,10 +616,13 @@ def filter(self, record): _cache_options = { "CLIENT_CLASS": "django_redis.client.DefaultClient", "SERIALIZER": "django_redis.serializers.json.JSONSerializer", - "DB": _cache_db, - "USERNAME": REDIS_USER, - "PASSWORD": REDIS_PASSWORD, } + if not _redis_url: + # Credentials and db travel IN the URL in URL mode; passing them again + # through OPTIONS risks one of them winning over the other. + _cache_options["DB"] = _cache_db + _cache_options["USERNAME"] = REDIS_USER + _cache_options["PASSWORD"] = REDIS_PASSWORD if REDIS_SSL: _pool_kwargs = {"ssl_cert_reqs": REDIS_SSL_CERT_REQS} if REDIS_SSL_CA_CERTS: @@ -608,7 +632,8 @@ def filter(self, record): CACHES = { "default": { "BACKEND": "django_redis.cache.RedisCache", - "LOCATION": f"{_scheme}://{REDIS_HOST}:{REDIS_PORT}/{_cache_db}", + "LOCATION": _redis_url + or f"{_scheme}://{REDIS_HOST}:{REDIS_PORT}/{_cache_db}", "OPTIONS": _cache_options, "KEY_FUNCTION": "utils.redis_cache.custom_key_function", } diff --git a/backend/backend/tests/test_redis_settings_derivation.py b/backend/backend/tests/test_redis_settings_derivation.py new file mode 100644 index 0000000000..7392f1af6d --- /dev/null +++ b/backend/backend/tests/test_redis_settings_derivation.py @@ -0,0 +1,135 @@ +"""How the Redis settings block derives its URLs (UN-4123). + +The block lives inside ``settings/base.py`` and runs at import, so it cannot be +called directly; these tests execute that exact source range against a controlled +environment instead of re-implementing it, which would test a copy rather than +the thing that ships. + +The case that matters is REDIS_URL. `create_redis_client` honoured it from the +start, while this module still built its own URL from REDIS_HOST/REDIS_PORT — so +the workers moved to the configured endpoint and the backend silently stayed +behind. Nothing errors: the API deployment simply returns ``result: null``, +because the execution's cached result was written to one Redis and looked up in +the other. Found on a live run, not by reading the code. +""" + +from __future__ import annotations + +import pathlib + +import pytest + +_SETTINGS = pathlib.Path(__file__).resolve().parents[1] / "settings" / "base.py" + + +def _derive(**env: str) -> dict: + """Execute the standalone Redis block with the given env.""" + source = _SETTINGS.read_text() + start = source.index("REDIS_SENTINEL_MODE = (") + end = source.index("SESSION_ENGINE =") + + ns: dict = {} + prelude = ( + "import os\n" + "from urllib.parse import quote\n" + "REDIS_USER = os.environ.get('REDIS_USER', 'default')\n" + "REDIS_PASSWORD = os.environ.get('REDIS_PASSWORD', '')\n" + "REDIS_HOST = os.environ.get('REDIS_HOST', 'localhost')\n" + "REDIS_PORT = os.environ.get('REDIS_PORT', '6379')\n" + "REDIS_DB = os.environ.get('REDIS_DB', '')\n" + "REDIS_SSL = os.environ.get('REDIS_SSL', 'false').strip().lower() == 'true'\n" + "REDIS_SSL_CERT_REQS = os.environ.get('REDIS_SSL_CERT_REQS', 'required')\n" + "REDIS_SSL_CA_CERTS = os.environ.get('REDIS_SSL_CA_CERTS', '').strip()\n" + "REDIS_URL = os.environ.get('REDIS_URL', '').strip()\n" + ) + import os as _os + + saved = {k: _os.environ.get(k) for k in list(_os.environ) if "REDIS" in k} + for key in saved: + _os.environ.pop(key, None) + _os.environ.update(env) + try: + exec(prelude + source[start:end], ns) # noqa: S102 - the code under test + finally: + for key in list(_os.environ): + if "REDIS" in key: + _os.environ.pop(key, None) + _os.environ.update({k: v for k, v in saved.items() if v is not None}) + return ns + + +@pytest.fixture +def plain() -> dict: + return _derive(REDIS_HOST="unstract-redis", REDIS_PORT="6379") + + +class TestDiscreteVars: + def test_plaintext_is_unchanged(self, plain): + assert plain["CACHES"]["default"]["LOCATION"] == "redis://unstract-redis:6379/0" + assert plain["SOCKET_IO_MANAGER_URL"] == "redis://unstract-redis:6379" + assert "CONNECTION_POOL_KWARGS" not in plain["CACHES"]["default"]["OPTIONS"] + + def test_db_travels_in_the_location(self): + """django-redis 5.4.0 ignores OPTIONS['DB'], so the path is the only route. + + Without it this cache sits on db 0 while every other service honours + REDIS_DB — workers RPUSH log_history_queue to db N and the backend LPOPs + an empty db 0. + """ + derived = _derive(REDIS_HOST="h", REDIS_DB="3") + assert derived["CACHES"]["default"]["LOCATION"].endswith("/3") + + def test_ssl_switches_scheme_and_pool_kwargs(self): + derived = _derive(REDIS_HOST="h", REDIS_SSL="true", REDIS_SSL_CA_CERTS="/ca.pem") + cache = derived["CACHES"]["default"] + assert cache["LOCATION"].startswith("rediss://") + assert cache["OPTIONS"]["CONNECTION_POOL_KWARGS"] == { + "ssl_cert_reqs": "required", + "ssl_ca_certs": "/ca.pem", + } + + def test_socketio_pins_certificate_verification(self): + """kombu defaults rediss:// to CERT_NONE — encrypted, but unauthenticated.""" + derived = _derive(REDIS_HOST="h", REDIS_SSL="true") + assert "ssl_cert_reqs=required" in derived["SOCKET_IO_MANAGER_URL"] + + +class TestUrlMode: + def test_url_drives_both_cache_and_socketio(self): + """The regression: these two used to ignore REDIS_URL entirely.""" + url = "rediss://:pw@managed.example:6380/0?ssl_cert_reqs=required" + derived = _derive(REDIS_HOST="in-cluster", REDIS_URL=url) + assert derived["CACHES"]["default"]["LOCATION"].startswith(url) + assert derived["SOCKET_IO_MANAGER_URL"].startswith(url) + assert "in-cluster" not in derived["CACHES"]["default"]["LOCATION"] + + def test_credentials_are_not_passed_twice(self): + """In URL mode the URL is the single source for db and credentials.""" + derived = _derive(REDIS_URL="rediss://:pw@h:6380/2", REDIS_PASSWORD="other") + options = derived["CACHES"]["default"]["OPTIONS"] + assert "PASSWORD" not in options + assert "DB" not in options + + def test_ca_is_appended_for_tls_urls(self): + derived = _derive( + REDIS_URL="rediss://h:6380/0?ssl_cert_reqs=required", + REDIS_SSL_CA_CERTS="/etc/ssl/redis-ca.pem", + ) + assert "ssl_ca_certs=/etc/ssl/redis-ca.pem" in ( + derived["CACHES"]["default"]["LOCATION"] + ) + + def test_ca_is_not_appended_to_a_plaintext_url(self): + derived = _derive( + REDIS_URL="redis://h:6379/0", REDIS_SSL_CA_CERTS="/etc/ssl/redis-ca.pem" + ) + assert "ssl_ca_certs" not in derived["CACHES"]["default"]["LOCATION"] + + def test_an_explicit_ca_in_the_url_wins(self): + derived = _derive( + REDIS_URL="rediss://h:6380/0?ssl_ca_certs=/in/url.pem", + REDIS_SSL_CA_CERTS="/env/ca.pem", + ) + location = derived["CACHES"]["default"]["LOCATION"] + assert location.count("ssl_ca_certs") == 1 + assert "/in/url.pem" in location From 74ff1e3504b20f038d7a7d6415489c8fa77df3c9 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 11:57:16 +0000 Subject: [PATCH 5/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- backend/backend/settings/base.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/backend/backend/settings/base.py b/backend/backend/settings/base.py index 2fa013f5e4..7466774426 100644 --- a/backend/backend/settings/base.py +++ b/backend/backend/settings/base.py @@ -593,7 +593,9 @@ def filter(self, record): and "ssl_ca_certs=" not in _redis_url ): _sep = "&" if "?" in _redis_url else "?" - _redis_url = f"{_redis_url}{_sep}ssl_ca_certs={quote(REDIS_SSL_CA_CERTS, safe='/')}" + _redis_url = ( + f"{_redis_url}{_sep}ssl_ca_certs={quote(REDIS_SSL_CA_CERTS, safe='/')}" + ) # 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 From 20b6b484952e3a289344ec2e7c081b72c83b0a80 Mon Sep 17 00:00:00 2001 From: ali Date: Mon, 21 Sep 2026 17:39:03 +0530 Subject: [PATCH 6/7] UN-4123 [FIX] Let a prefix's own DB survive an inherited REDIS_URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Spotted while documenting the Helm keys, not at runtime — which is the only reason it was caught before someone hit it in a cluster. The chart configures ONE endpoint for the platform and sets CACHE_REDIS_DB=1 for the worker cache. With the endpoint given as REDIS_URL, the CACHE_REDIS_ prefix inherits that URL (no CACHE_REDIS_URL of its own) and took the db from its path — so the worker cache would quietly move from db 1 to db 0 and sit beside every other key. Nothing errors; the keys just relocate. A prefix's own {prefix}DB now applies to an INHERITED url. An explicit {prefix}URL is untouched, since it names its db deliberately, and an explicit db= argument still beats both (sdk1 metrics depends on that). Three tests pin the precedence chain: inherited URL + prefix db, explicit prefix URL, and the db= argument. Suite now 26. --- .../src/unstract/core/cache/redis_client.py | 15 +++++++++++- .../core/tests/test_redis_client_config.py | 24 +++++++++++++++++++ 2 files changed, 38 insertions(+), 1 deletion(-) diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index 1f536cafdf..41dbdddfc8 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -178,6 +178,15 @@ def _resolve_redis_env( # path, so an unset URL changes nothing. "url": os.getenv(f"{env_prefix}URL", os.getenv("REDIS_URL", "")).strip(), } + # A prefix that INHERITS the generic REDIS_URL must still honour its own + # {prefix}DB. The Helm chart sets CACHE_REDIS_DB=1 while configuring one + # REDIS_URL for the platform; without this the worker cache would silently + # follow the URL's db instead, landing on db 0 beside everything else. An + # explicit {prefix}URL is left alone — it names its own db deliberately. + own_url = os.getenv(f"{env_prefix}URL", "").strip() + own_db = os.getenv(f"{env_prefix}DB", "").strip() + if result["url"] and not own_url and own_db and db_override is None: + result["db_from_prefix_env"] = int(own_db) # Read OUTSIDE the `if ssl` below: URL mode carries TLS in the scheme and never # sets {prefix}SSL, so gating the CA on that flag left `rediss://` verifying # against the system trust store alone — which fails for exactly the servers @@ -261,7 +270,11 @@ def _create_standalone_client( socket_timeout=socket_timeout, max_connections=max_connections, health_check_interval=health_check_interval, - db_override=db_override, + db_override=( + db_override + if db_override is not None + else env.get("db_from_prefix_env") + ), ssl_ca_certs=env.get("ssl_ca_certs"), ) diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index eb3aa3871d..07f8c3c8f3 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -193,6 +193,30 @@ def test_ca_certs_ignored_for_a_plaintext_url(self, monkeypatch): monkeypatch.setenv("REDIS_SSL_CA_CERTS", "/etc/ssl/redis-ca.pem") assert "ssl_ca_certs" not in _kwargs(create_redis_client()) + def test_prefix_db_wins_over_an_inherited_url(self, monkeypatch): + """The Helm chart's shape: one REDIS_URL, plus CACHE_REDIS_DB=1. + + The cache prefix inherits the generic URL for its endpoint, but its own + db must still apply — otherwise the worker cache silently moves to the + URL's db and lands beside everything else on db 0. + """ + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/0") + monkeypatch.setenv("CACHE_REDIS_DB", "1") + assert _kwargs(create_redis_client(env_prefix="CACHE_REDIS_"))["db"] == 1 + assert _kwargs(create_redis_client())["db"] == 0 + + def test_an_explicit_prefix_url_keeps_its_own_db(self, monkeypatch): + """A URL written FOR this prefix names its db deliberately.""" + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/0") + monkeypatch.setenv("CACHE_REDIS_URL", "rediss://cache.example:6380/3") + monkeypatch.setenv("CACHE_REDIS_DB", "1") + assert _kwargs(create_redis_client(env_prefix="CACHE_REDIS_"))["db"] == 3 + + def test_explicit_argument_still_wins(self, monkeypatch): + monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380/0") + monkeypatch.setenv("CACHE_REDIS_DB", "1") + assert _kwargs(create_redis_client(env_prefix="CACHE_REDIS_", db=7))["db"] == 7 + def test_pool_size_survives_url_mode(self, monkeypatch): monkeypatch.setenv("REDIS_URL", "rediss://cache.example:6380") client = create_redis_client(max_connections=10) From fd8e7333cc313dd933fb92adbcf870255cb36065 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 21 Sep 2026 12:11:01 +0000 Subject: [PATCH 7/7] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- unstract/core/src/unstract/core/cache/redis_client.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index 41dbdddfc8..6bfe3be6bd 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -271,9 +271,7 @@ def _create_standalone_client( max_connections=max_connections, health_check_interval=health_check_interval, db_override=( - db_override - if db_override is not None - else env.get("db_from_prefix_env") + db_override if db_override is not None else env.get("db_from_prefix_env") ), ssl_ca_certs=env.get("ssl_ca_certs"), )