From 3b5101c41fbc3d5577622dc4aaf20dc8a8bf4529 Mon Sep 17 00:00:00 2001 From: ali Date: Mon, 5 Oct 2026 18:04:17 +0530 Subject: [PATCH 1/2] UN-4123 [FIX] The default Redis user without a password is not an error 6c0a69dec added an ERROR when a username is set without a password, to replace redis-py's opaque "DataError: Invalid input of type: 'NoneType'" on the first command with something that names the values file. The diagnostic is right for a named ACL user. It is wrong for "default". "default" is Redis's own built-in user and `AUTH default ` is equivalent to `AUTH `, so "default with no password" is not a misconfiguration -- it is the absence of ACL usage, which is the normal state for the in-cluster Redis that has no password at all. values.yaml and sample.on-prem.values.yaml both ship REDIS_USER: default, so the error fired on every default on-prem install. Measured on two staging namespaces before this fix: ~274 lines in 20 minutes in one and 400+ in the other, across eight pod types -- roughly 20k ERROR lines a day per namespace. Nothing was broken; the username SHOULD be ignored and was. The cost is log volume and real errors buried under it, which is what the diagnostic was meant to prevent. The username is still dropped either way, so behaviour is unchanged. Only the logging is conditional, and a NON-default username without a password still reports -- that is the case redis-py turns into the opaque DataError. Found by deploying rc.423 to a vanilla in-cluster on-prem namespace. Both managed configurations set REDIS_USER: "" explicitly, so neither could surface it; only the shipped default does, which is what most on-prem installs run. Mutation-checked: removing the exemption fails the three silence cases and nothing else. Co-Authored-By: Claude Opus 5 --- .../src/unstract/core/cache/redis_client.py | 27 ++++++++---- .../core/tests/test_redis_client_config.py | 43 +++++++++++++++++++ 2 files changed, 62 insertions(+), 8 deletions(-) diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index ca6c438379..d22ed98988 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -637,15 +637,26 @@ def _resolve_redis_env( # Django cache already makes for this input (apply_url_credentials returns # the URL untouched when there is no password), rather than the two # disagreeing about a config neither can honour. + # + # "default" is EXEMPT from the diagnostic, though still dropped. It is Redis's + # own built-in user, `AUTH default ` is equivalent to `AUTH `, and the + # chart ships REDIS_USER: default -- so "default with no password" is not a + # misconfiguration, it is the absence of ACL usage, which is the normal state + # for the in-cluster Redis that has no password at all. Logging it as an error + # fired on every default on-prem install: ~20k ERROR lines a day per + # namespace, across eight pod types, measured on two staging namespaces. A + # NON-default username without a password is still a real mistake and still + # reported, because that is the one redis-py turns into an opaque DataError. if username and not password: - logger.error( - "%sUSER=%r is set but no password is; Redis has no one-argument ACL " - "AUTH, so the username is being ignored. Set %sPASSWORD, or clear " - "the username.", - env_prefix, - username, - env_prefix, - ) + if username.strip().lower() != "default": + logger.error( + "%sUSER=%r is set but no password is; Redis has no one-argument " + "ACL AUTH, so the username is being ignored. Set %sPASSWORD, or " + "clear the username.", + env_prefix, + username, + env_prefix, + ) username = None prefixed_db = os.getenv(f"{env_prefix}DB", "").strip() generic_db = os.getenv("REDIS_DB", "").strip() diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index 7411369dfc..e00b95c7a1 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -1214,6 +1214,49 @@ def test_a_blank_user_falls_through_to_the_other_spelling(self, monkeypatch): assert urlsplit(build_socketio_redis_url()).username == "alice" +class TestTheDefaultUserWithoutAPasswordIsNotAnError: + """A username without a password is dropped either way; only the DIAGNOSTIC + is conditional. + + `default` is Redis's own built-in user and `AUTH default ` is equivalent + to `AUTH `, so `default` with no password is not a misconfiguration -- it + is the absence of ACL usage, which is the normal state for the in-cluster + Redis, and values.yaml ships REDIS_USER: default. Logging it at ERROR fired + on every default on-prem install: ~20k lines a day per namespace across + eight pod types, measured on two staging namespaces before this was fixed. + """ + + @pytest.mark.parametrize("raw", ["default", "DEFAULT", " default "]) + def test_the_default_user_without_a_password_is_silent( + self, monkeypatch, raw, caplog + ): + monkeypatch.setenv("REDIS_USER", raw) + monkeypatch.delenv("REDIS_PASSWORD", raising=False) + # Dropped, exactly as a named user would be -- the behaviour is unchanged. + assert _resolve_redis_env("REDIS_")["username"] is None + # Spelling and whitespace are normalised before the comparison, because + # the chart writes one spelling and a hand-edited values file another. + assert "ACL AUTH" not in caplog.text + + def test_a_named_user_without_a_password_still_errors(self, monkeypatch, caplog): + """The case the diagnostic exists for, and the one redis-py turns into an + opaque `DataError: Invalid input of type: 'NoneType'` on the first command. + """ + monkeypatch.setenv("REDIS_USER", "alice") + monkeypatch.delenv("REDIS_PASSWORD", raising=False) + assert _resolve_redis_env("REDIS_")["username"] is None + assert "ACL AUTH" in caplog.text + assert "alice" in caplog.text + + def test_the_default_user_WITH_a_password_is_kept(self, monkeypatch): + """The exemption must not reach the supported configuration: `default` + plus a password is a real two-argument AUTH and has to survive. + """ + monkeypatch.setenv("REDIS_USER", "default") + monkeypatch.setenv("REDIS_PASSWORD", "pw") + assert _resolve_redis_env("REDIS_")["username"] == "default" + + class TestBlankMeansUnsetForTlsToo: """The convention has to cover TLS, not just credentials. From 0fe3d3c4fb9c3983f0feb9a2820826550aa8bc95 Mon Sep 17 00:00:00 2001 From: ali Date: Mon, 5 Oct 2026 18:12:41 +0530 Subject: [PATCH 2/2] UN-4123 [FIX] Address review: the default-user exemption is case-sensitive Redis ACL usernames are case-sensitive, so `DEFAULT` is a named user distinct from the built-in `default`. Lowercasing the comparison suppressed the missing-password diagnostic for it -- a real mistake made harder to diagnose, which is the opposite of the point. Compare exactly. The .strip() stays. env_chain returns the RAW value deliberately (stripping it there truncated a password once, per the comment at its definition), so a hand-edited values file can carry padding around a username that was meant to be the built-in one. Whitespace is an editing artifact; case is semantic. test_a_case_distinct_default_still_errors covers it, and "DEFAULT" is out of the silence parametrize. Mutation-checked: restoring .lower() fails that test alone. Co-Authored-By: Claude Opus 5 --- .../src/unstract/core/cache/redis_client.py | 9 ++++++++- unstract/core/tests/test_redis_client_config.py | 17 ++++++++++++++--- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/unstract/core/src/unstract/core/cache/redis_client.py b/unstract/core/src/unstract/core/cache/redis_client.py index d22ed98988..4ef5c507e2 100644 --- a/unstract/core/src/unstract/core/cache/redis_client.py +++ b/unstract/core/src/unstract/core/cache/redis_client.py @@ -647,8 +647,15 @@ def _resolve_redis_env( # namespace, across eight pod types, measured on two staging namespaces. A # NON-default username without a password is still a real mistake and still # reported, because that is the one redis-py turns into an opaque DataError. + # + # CASE-SENSITIVE, deliberately: Redis ACL usernames are, so `DEFAULT` is a + # named user distinct from the built-in `default` and a missing password for + # it is a real mistake worth reporting. Whitespace is stripped because + # env_chain returns the RAW value on purpose (stripping it there truncated a + # password once), so a hand-edited values file can carry padding around a + # username that was meant to be the built-in one. if username and not password: - if username.strip().lower() != "default": + if username.strip() != "default": logger.error( "%sUSER=%r is set but no password is; Redis has no one-argument " "ACL AUTH, so the username is being ignored. Set %sPASSWORD, or " diff --git a/unstract/core/tests/test_redis_client_config.py b/unstract/core/tests/test_redis_client_config.py index e00b95c7a1..168aaa19c9 100644 --- a/unstract/core/tests/test_redis_client_config.py +++ b/unstract/core/tests/test_redis_client_config.py @@ -1226,7 +1226,7 @@ class TestTheDefaultUserWithoutAPasswordIsNotAnError: eight pod types, measured on two staging namespaces before this was fixed. """ - @pytest.mark.parametrize("raw", ["default", "DEFAULT", " default "]) + @pytest.mark.parametrize("raw", ["default", " default "]) def test_the_default_user_without_a_password_is_silent( self, monkeypatch, raw, caplog ): @@ -1234,10 +1234,21 @@ def test_the_default_user_without_a_password_is_silent( monkeypatch.delenv("REDIS_PASSWORD", raising=False) # Dropped, exactly as a named user would be -- the behaviour is unchanged. assert _resolve_redis_env("REDIS_")["username"] is None - # Spelling and whitespace are normalised before the comparison, because - # the chart writes one spelling and a hand-edited values file another. + # Padding is stripped before the comparison because env_chain returns the + # RAW value on purpose, so a hand-edited values file can carry it. assert "ACL AUTH" not in caplog.text + def test_a_case_distinct_default_still_errors(self, monkeypatch, caplog): + """Redis ACL usernames are CASE-SENSITIVE, so `DEFAULT` is a named user + distinct from the built-in `default` -- not the exemption, and a missing + password for it is a real mistake. Lowercasing the comparison hid this. + """ + monkeypatch.setenv("REDIS_USER", "DEFAULT") + monkeypatch.delenv("REDIS_PASSWORD", raising=False) + assert _resolve_redis_env("REDIS_")["username"] is None + assert "ACL AUTH" in caplog.text + assert "DEFAULT" in caplog.text + def test_a_named_user_without_a_password_still_errors(self, monkeypatch, caplog): """The case the diagnostic exists for, and the one redis-py turns into an opaque `DataError: Invalid input of type: 'NoneType'` on the first command.