UN-4123 [FIX] The default Redis user without a password is not an error - #2315
Merged
Merged
Conversation
6c0a69d 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 <pw>` is equivalent to `AUTH <pw>`, 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 <noreply@anthropic.com>
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…itive 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 <noreply@anthropic.com>
|
Contributor
Unstract test resultsPer-group results
Critical paths
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Exempt the username
defaultfrom the "username set but no password" ERROR added in6c0a69dec(#2299). The username is still dropped; only the logging is conditional.Why
That diagnostic replaces redis-py's opaque
DataError: Invalid input of type: 'NoneType'on the first command with a message that names the values file. It is right for a named
ACL user. It is wrong for
default.defaultis Redis's own built-in user, andAUTH default <pw>is equivalent toAUTH <pw>— sodefaultwith no password is not a misconfiguration. It is theabsence of ACL usage, which is the normal state for the in-cluster Redis that has no
password at all. Both
values.yamlandsample.on-prem.values.yamlshipREDIS_USER: default, so the error fired on every default on-prem install.Measured on two staging namespaces before this fix:
Roughly 20,000 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 the opposite of what the diagnostic was for.
How it was found
Deploying rc.423 to a vanilla in-cluster on-prem namespace. Both managed-Redis
configurations set
REDIS_USER: ""explicitly, so neither could surface it — only theshipped default does, which is what most on-prem installs run.
How
username = Nonestays outside the condition, so behaviour is unchanged — a usernamewithout a password is dropped either way, because redis-py cannot send one. A
non-default username without a password still reports, since that is the case that
becomes the opaque
DataError.Spelling and surrounding whitespace are normalised before the comparison: the chart writes
one spelling and a hand-edited values file may write another.
Can this PR break any existing features? If yes, please list possible items. If no, please explain why.
No.
username = Nonewasalready applied unconditionally and still is; only whether a line is logged changes.
defaultwith a password is untouched and still sent as a two-argument AUTH —asserted by
test_the_default_user_WITH_a_password_is_kept.test_a_named_user_without_a_password_still_errors.Relevant Docs
None.
Related Issues or PRs
6c0a69dec) introduced the diagnostic this narrows.Dependencies Versions / Env Variables
None.
Notes on Testing
tests/test_redis_client_config.py: 144 passed, including 5 new.backend/tests/test_redis_settings_derivation.py: 92 passed (the backend resolvershares this code path).
silence cases and nothing else.
Also worth fixing, not in this PR
charts/unstract-platform/values.yamlin unstract-cloud carries a comment claiming "Theon-prem sample now ships
REDIS_USER: """. It does not —sample.on-prem.values.yaml:190ships
REDIS_USER: default, same as the base atvalues.yaml:207. That belongs in a cloudPR; noting it here so the inaccuracy is recorded.
🤖 Generated with Claude Code