Skip to content

Commit 9019ac1

Browse files
UN-4123 [FIX] Extend the cache/client parity to Sentinel mode
The parity sweep stopped at the Sentinel branch, and I recorded the four divergences living there as "its own ticket". That was the wrong call twice over: a guarantee that stops at a mode boundary is a half-truth, and the reason I gave for deferring — "changing code no test in this file executes" — was not true. _derive's slice spans BOTH branches; it just needed an env selecting this one. "No test executes it" and "no test was written for it" are different statements, and only the second was ever the case. Four divergences from create_redis_client, all pre-existing on main: * THE SENTINEL PORT. The module-level REDIS_PORT defaults to 6379 — the STANDALONE port — while core's Sentinel path resolves with default_port="26379". With REDIS_PORT unset this cache looked for sentinels on the wrong port while every other client found them. The chart always sets REDIS_PORT, which is why it stayed hidden. * THE USERNAME, twice. REDIS_USER defaults to "default" at module scope, so this cache sent a two-argument ACL AUTH where core sends the one-argument form; and it read REDIS_USER directly, so the REDIS_USERNAME spelling platform-service ships was ignored. Both now come from the shared resolver, and the same value reaches SENTINEL_KWARGS, the LOCATION and the Socket.IO URL. * TLS REACHED NEITHER CONNECTION. A TLS-only Sentinel deployment got a plaintext cache while core encrypted both the discovery and the master connections. Carrying it in SENTINEL_KWARGS alone would encrypt discovery and leave the master in clear, so it goes to CONNECTION_POOL_KWARGS too — core builds both from one env dict for exactly this reason. BEHAVIOUR CHANGES for existing Sentinel deployments, stated rather than buried: an unset REDIS_PORT now resolves 26379 instead of 6379; no username is sent unless one is configured (Redis 6+ treats `AUTH default <pw>` and `AUTH <pw>` as the same command, and redis-py retries one-arg regardless, so this is a wire change rather than an auth change); and REDIS_SSL now applies here. Flag off is byte-identical — asserted. Backend 79 -> 87. Four mutations verified caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 815460e commit 9019ac1

2 files changed

Lines changed: 161 additions & 18 deletions

File tree

‎backend/backend/settings/base.py‎

Lines changed: 47 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -563,32 +563,57 @@ def filter(self, record):
563563

564564
REDIS_SENTINEL_MASTER_NAME = os.environ.get("REDIS_SENTINEL_MASTER_NAME", "mymaster")
565565

566-
# NOT COVERED by TestTheBackendAgreesWithCoreOnEverySharedSetting — _derive only
567-
# exercises the standalone branch below, so the "cache and client reach the same
568-
# place" property stops at this line. Four divergences from create_redis_client
569-
# live in here and are PRE-EXISTING on main, untouched by UN-4123:
570-
# * REDIS_USER defaults to "default" above, so this cache sends a two-argument
571-
# ACL AUTH where core sends the one-argument form;
572-
# * it reads REDIS_USER directly, so the REDIS_USERNAME spelling is ignored;
573-
# * REDIS_SSL is not applied here at all — a plaintext cache against a
574-
# TLS-only Sentinel deployment;
575-
# * the Sentinel default port is 26379 in core and 6379 here.
576-
# Left alone deliberately: this is the self-hosted HA path, a managed endpoint
577-
# is a single primary and does not use it, and changing behaviour here would be
578-
# changing code no test in this file executes. Its own ticket.
579566
if REDIS_SENTINEL_MODE:
567+
# This branch used to hold four divergences from create_redis_client, all
568+
# pre-existing. They are closed here because "the cache and the client reach
569+
# the same place" is not a property that can stop at a mode boundary — an
570+
# operator on Sentinel gets the same guarantee or the guarantee is a
571+
# half-truth. TestSentinelModeAgreesWithCore covers each one.
572+
#
573+
# The username comes from the shared resolver, not the module-level
574+
# REDIS_USER: that one defaults to "default", so this cache sent a
575+
# two-argument ACL AUTH where core sends the one-argument form, and it read
576+
# only REDIS_USER so the REDIS_USERNAME spelling platform-service ships was
577+
# ignored entirely.
578+
_sentinel_username = url_username_from_env()
579+
580+
# 26379 — the Sentinel port — matching core's
581+
# _resolve_redis_env(default_port="26379"). The module-level REDIS_PORT
582+
# defaults to 6379, which is the standalone port, so an unset REDIS_PORT
583+
# pointed this cache at the wrong port while every other client found the
584+
# sentinels. The chart always sets it, which is why this stayed hidden.
585+
REDIS_PORT = parse_port(os.environ.get("REDIS_PORT"), "REDIS_PORT", 26379)
586+
580587
_sentinel_kwargs = {}
581588
if REDIS_PASSWORD:
582589
_sentinel_kwargs["password"] = REDIS_PASSWORD
583-
if REDIS_USER:
584-
_sentinel_kwargs["username"] = REDIS_USER
590+
if _sentinel_username:
591+
_sentinel_kwargs["username"] = _sentinel_username
592+
593+
# TLS reached neither the discovery connections nor the master one, so a
594+
# TLS-only Sentinel deployment got a plaintext cache while core encrypted
595+
# both. Core builds them from one env dict for exactly this reason; the
596+
# same settings go to both here.
597+
_sentinel_pool_kwargs = {}
598+
if REDIS_SSL:
599+
_sentinel_tls = {
600+
"ssl": True,
601+
"ssl_cert_reqs": REDIS_SSL_CERT_REQS,
602+
"ssl_check_hostname": REDIS_SSL_CHECK_HOSTNAME,
603+
}
604+
if REDIS_SSL_CA_CERTS:
605+
_sentinel_tls["ssl_ca_certs"] = REDIS_SSL_CA_CERTS
606+
_sentinel_kwargs.update(_sentinel_tls)
607+
_sentinel_pool_kwargs.update(_sentinel_tls)
585608

586609
_redis_db = parse_db(REDIS_DB, "REDIS_")
587610

588611
# SocketIO connection manager (Kombu Sentinel URL format)
589612
_cred_prefix = ""
590-
if REDIS_USER and REDIS_PASSWORD:
591-
_cred_prefix = f"{quote(REDIS_USER, safe='')}:{quote(REDIS_PASSWORD, safe='')}@"
613+
if _sentinel_username and REDIS_PASSWORD:
614+
_cred_prefix = (
615+
f"{quote(_sentinel_username, safe='')}:{quote(REDIS_PASSWORD, safe='')}@"
616+
)
592617
elif REDIS_PASSWORD:
593618
_cred_prefix = f":{quote(REDIS_PASSWORD, safe='')}@"
594619
SOCKET_IO_MANAGER_URL = (
@@ -597,7 +622,7 @@ def filter(self, record):
597622
SOCKET_IO_TRANSPORT_OPTIONS = {"master_name": REDIS_SENTINEL_MASTER_NAME}
598623

599624
# django-redis expects username in the LOCATION URL for ACL auth
600-
_user_prefix = f"{REDIS_USER}@" if REDIS_USER else ""
625+
_user_prefix = f"{_sentinel_username}@" if _sentinel_username else ""
601626
CACHES = {
602627
"default": {
603628
"BACKEND": "django_redis.cache.RedisCache",
@@ -608,6 +633,10 @@ def filter(self, record):
608633
"CONNECTION_FACTORY": "django_redis.pool.SentinelConnectionFactory",
609634
"SENTINELS": [(REDIS_HOST, REDIS_PORT)],
610635
"SENTINEL_KWARGS": _sentinel_kwargs,
636+
# TLS for the MASTER connection. SENTINEL_KWARGS covers only the
637+
# discovery ones; core carries both, so a deployment with TLS on
638+
# got an encrypted discovery and a plaintext master.
639+
"CONNECTION_POOL_KWARGS": _sentinel_pool_kwargs,
611640
"DB": _redis_db,
612641
"PASSWORD": REDIS_PASSWORD,
613642
"SERIALIZER": "django_redis.serializers.json.JSONSerializer",

‎backend/backend/tests/test_redis_settings_derivation.py‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -778,3 +778,117 @@ def _from_cache(env: dict) -> dict:
778778
@pytest.mark.parametrize("env", _CASES, ids=lambda e: ",".join(sorted(e)))
779779
def test_the_cache_and_the_client_reach_the_same_place(self, env):
780780
assert self._from_cache(env) == self._from_core(env)
781+
782+
783+
class TestSentinelModeAgreesWithCore:
784+
"""The parity property must not stop at a mode boundary.
785+
786+
TestTheBackendAgreesWithCoreOnEverySharedSetting cannot cover Sentinel: its
787+
_from_core calls create_redis_client, which in Sentinel mode performs real
788+
discovery and blocks. So the comparison here is against the resolver rather
789+
than a built client — _resolve_redis_env with the same default_port core's
790+
Sentinel path passes — which is enough to pin the four facts that diverged.
791+
792+
The branch was NOT untestable, which an earlier comment in base.py claimed:
793+
_derive's slice spans both branches, so it just needed an env that selects
794+
this one. "No test executes it" and "no test was written for it" are
795+
different statements, and only the second was true.
796+
"""
797+
798+
@staticmethod
799+
def _core(**env: str) -> dict:
800+
import os
801+
802+
from unstract.core.cache.redis_client import _resolve_redis_env
803+
804+
saved = {k: os.environ.get(k) for k in list(os.environ) if "REDIS" in k}
805+
for key in saved:
806+
os.environ.pop(key, None)
807+
os.environ.update(env)
808+
try:
809+
return _resolve_redis_env("REDIS_", default_port="26379")
810+
finally:
811+
for key in list(os.environ):
812+
if "REDIS" in key:
813+
os.environ.pop(key, None)
814+
os.environ.update({k: v for k, v in saved.items() if v is not None})
815+
816+
def test_the_sentinel_port_default_matches_core(self):
817+
"""REDIS_PORT unset pointed this cache at 6379 — the STANDALONE port —
818+
while every other client found the sentinels on 26379."""
819+
env = {"REDIS_SENTINEL_MODE": "true", "REDIS_HOST": "sent"}
820+
sentinels = _derive(**env)["CACHES"]["default"]["OPTIONS"]["SENTINELS"]
821+
assert sentinels == [("sent", self._core(**env)["port"])]
822+
assert sentinels == [("sent", 26379)]
823+
824+
def test_an_explicit_port_still_wins(self):
825+
env = {"REDIS_SENTINEL_MODE": "true", "REDIS_HOST": "sent", "REDIS_PORT": "27000"}
826+
assert _derive(**env)["CACHES"]["default"]["OPTIONS"]["SENTINELS"] == [
827+
("sent", 27000)
828+
]
829+
830+
def test_no_username_configured_means_no_username_sent(self):
831+
"""REDIS_USER defaults to "default" at module scope, so this cache sent
832+
a two-argument ACL AUTH where core sends the one-argument form."""
833+
env = {
834+
"REDIS_SENTINEL_MODE": "true",
835+
"REDIS_HOST": "sent",
836+
"REDIS_PASSWORD": "pw",
837+
}
838+
cache = _derive(**env)["CACHES"]["default"]
839+
assert self._core(**env)["username"] is None
840+
assert "username" not in cache["OPTIONS"]["SENTINEL_KWARGS"]
841+
assert urlsplit(cache["LOCATION"]).username is None
842+
843+
@pytest.mark.parametrize("spelling", ["REDIS_USER", "REDIS_USERNAME"])
844+
def test_both_username_spellings_reach_the_sentinel_cache(self, spelling):
845+
"""platform-service ships REDIS_USERNAME; reading only REDIS_USER left
846+
this cache authenticating as `default` while core used the ACL user."""
847+
env = {
848+
"REDIS_SENTINEL_MODE": "true",
849+
"REDIS_HOST": "sent",
850+
"REDIS_PASSWORD": "pw",
851+
spelling: "alice",
852+
}
853+
derived = _derive(**env)
854+
cache = derived["CACHES"]["default"]
855+
assert self._core(**env)["username"] == "alice"
856+
assert cache["OPTIONS"]["SENTINEL_KWARGS"]["username"] == "alice"
857+
assert urlsplit(cache["LOCATION"]).username == "alice"
858+
assert urlsplit(derived["SOCKET_IO_MANAGER_URL"]).username == "alice"
859+
860+
def test_tls_reaches_both_the_sentinels_and_the_master(self):
861+
"""SENTINEL_KWARGS covers only discovery. Carrying TLS there alone would
862+
leave an encrypted discovery and a plaintext master; carrying it nowhere
863+
— which is what this branch did — leaves a plaintext cache against a
864+
TLS-only deployment while core encrypts both."""
865+
env = {
866+
"REDIS_SENTINEL_MODE": "true",
867+
"REDIS_HOST": "sent",
868+
"REDIS_SSL": "true",
869+
"REDIS_SSL_CERT_REQS": "none",
870+
}
871+
options = _derive(**env)["CACHES"]["default"]["OPTIONS"]
872+
assert self._core(**env)["ssl"] is True
873+
for where in ("SENTINEL_KWARGS", "CONNECTION_POOL_KWARGS"):
874+
assert options[where]["ssl"] is True, where
875+
assert options[where]["ssl_cert_reqs"] == "none", where
876+
877+
def test_a_ca_reaches_both_too(self):
878+
env = {
879+
"REDIS_SENTINEL_MODE": "true",
880+
"REDIS_HOST": "sent",
881+
"REDIS_SSL": "true",
882+
"REDIS_SSL_CA_CERTS": "/etc/ssl/ca.pem",
883+
}
884+
options = _derive(**env)["CACHES"]["default"]["OPTIONS"]
885+
for where in ("SENTINEL_KWARGS", "CONNECTION_POOL_KWARGS"):
886+
assert options[where]["ssl_ca_certs"] == "/etc/ssl/ca.pem", where
887+
888+
def test_no_tls_configured_adds_no_tls_keys(self):
889+
"""Flag off must leave the Sentinel cache byte-identical to before."""
890+
options = _derive(REDIS_SENTINEL_MODE="true", REDIS_HOST="sent")["CACHES"][
891+
"default"
892+
]["OPTIONS"]
893+
assert options["SENTINEL_KWARGS"] == {}
894+
assert options["CONNECTION_POOL_KWARGS"] == {}

0 commit comments

Comments
 (0)