feat: apply ACL changes live without rolling pods - #319
Conversation
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
good catch, this was a real bug and both repros are right. fixed in 522ec5e. skipping the reload when the password hashes matched meant exactly what you describe: a permissions-only edit, or a user removed from the aclfile, kept every desired hash intact, so the node skipped the comparison can't just be widened, which is what led me down this path in the first place. so the reload is unconditional now. it's idempotent and cheap, and it converges every kind of change once the mounted volume catches up, which is also what was originally proposed on #212. the hash comparison stays, but only to drive added coverage for the case you found: passwords already matching must still issue the reload. |
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
jdheyburn
left a comment
jdheyburn
left a comment
There was a problem hiding this comment.
Thanks for taking this on, just a couple of comments.
| hashes = append(hashes, h) | ||
| } | ||
| } | ||
| slices.Sort(hashes) |
There was a problem hiding this comment.
Shall we dedupe hashes too? In case the user duplicated passwords? I think Valkey stores hashed passwords as a set on the backend, so we should do the same otherwise we'd be stuck in reconciliation forever.
There was a problem hiding this comment.
you're right, and it's worse than cosmetic: two Secret keys pointing at the same password put a repeated hash in the aclfile, Valkey reports it once, and the comparison could never converge, so the node would sit at ACLApplied=False and requeue every 10s forever. sorted and deduped on both sides now, with a test for the repeated-hash case. 1844eab.
| if err := c.LoadACL(ctx); err != nil { | ||
| return false, err | ||
| } | ||
| return passwordsInSync(ctx, c, desired) |
There was a problem hiding this comment.
Why do we only check if passwords are in sync, and not other dimensions like users added/removed, permissions updated, etc.?
There was a problem hiding this comment.
good push, and you're right i drew the line too conservatively. usernames compare exactly, so the user set is part of the check now (via ACL USERS): a user added to or removed from the aclfile that the server hasn't picked up reads as out of sync instead of being invisible to the condition.
permission rules are the part i still left out, and it isn't laziness: ACL GETUSER returns Valkey's normalised rendering of the rules, while the operator only holds the aclfile text, so comparing them properly means reimplementing Valkey's ACL parser and keeping it in step with the server.
worth stressing none of this gates the apply: the reload is unconditional (Greptile's catch earlier), so permission edits converge regardless. the check only scopes what ACLApplied can honestly claim, and the docs now say exactly that. 1844eab.
| if err != nil { | ||
| return nil, fmt.Errorf("ACL GETUSER %s passwords: %w", username, err) | ||
| } | ||
| slices.Sort(hashes) |
There was a problem hiding this comment.
Same here re deduplicating.
There was a problem hiding this comment.
done, UserPasswordHashes normalises through the same helper (sort + compact), so both sides are in the same shape. 1844eab.
| // Permissions are deliberately not compared. ACL GETUSER returns Valkey's | ||
| // normalised rendering of the rules, while the operator only holds the aclfile | ||
| // text, so comparing the two would mean reimplementing Valkey's own ACL parser | ||
| // and keeping it in step with the server. Correctness of the apply does not |
There was a problem hiding this comment.
Are we able to reuse some of the below to make comparisons?
valkey-operator/internal/controller/users.go
Lines 218 to 262 in 3a92e94
There was a problem hiding this comment.
i looked at reusing buildUserAcl for this, and it's the same wall the passwords-only check ran into. buildUserAcl renders the operator's form of the ACL, while ACL GETUSER returns Valkey's normalised form (rule order, folded categories, implied defaults like sanitize-payload, resetchannels handling). they don't compare byte-for-byte, so lining them up would still mean running our string through Valkey's own normaliser, which is the parser reimplementation i was trying to avoid.
where the reuse does help is the exact parts: the user set and the password hashes. those i already take from the aclfile the cluster controller built with buildUserAcl, and check against ACL USERS / ACL GETUSER, so the desired side is the same source of truth, just compared on the dimensions that survive normalisation. and since the reload is unconditional, the rest converges without needing a full comparison at all.
|
gentle nudge on this one, it's been ready since the last review round. CI is green, and the points from that pass are addressed: password hashes are deduped on both sides, and the sync check now covers user membership (add/remove), not just passwords. happy to rebase past the module rename in #316 whenever it's useful, or i'll do it if it goes dirty. no rush, just flagging it's waiting on another look. |
|
Sorry for the delay. I just tried to test it in a kind cluster locally, and I got these errors in the logs: It doesn't look like you've updated users.go to support the feature. Are you able to add that and perform local testing to see if it works as expected? Thank you! |
|
good catch, and thanks for running it on a real cluster. that's exactly the gap: fixed by granting the three subcommands the feature actually needs on
verified locally against a real valkey 9.0, not envtest:
pushed to the branch. mind giving it another spin when you get a moment? |
e784ac7 to
8273836
Compare
| // A permissions-only change, or a removed user, leaves every desired | ||
| // password hash untouched. The reload must not be skipped on that basis | ||
| // or those edits would never reach the server. | ||
| fake := &fakeConfigClient{aclHashes: desiredHashes} |
There was a problem hiding this comment.
can u update e2e test to check if acl test valkeycluster_test.go is actually applied live without rollout restart.
There was a problem hiding this comment.
Done, this is covered by the "live ACL propagation" spec. It brings a cluster up, records each server pod's UID, adds a user through the spec, then checks the new user shows up in a live ACL LIST and can authenticate while every pod UID stays the same afterwards (with a Consistently window, so a rollout restart would fail it). Every ValkeyNode also has to report ACLApplied=True.
|
@sandeepkunusoth good call on the e2e test, it earned its keep. writing it surfaced a gap the unit tests couldn't see. the live reload works, but adding a user to a running cluster still rolled every pod. the reason is pre-existing: fixed in this PR:
the e2e test is the one you asked for: it adds a user to a running cluster and asserts the new user is usable (present in |
e316b3c to
1909016
Compare
|
following up on my last comment: my first cut dropped the ACL-hash roll entirely, and that was too aggressive. running the full e2e suite, not just the new spec, caught it. the roll was also the only recovery path for a change that locks the operator out. delete the system-passwords secret and the operator regenerates it with a fresh password; the running server still has the old one, so the operator can neither so the fix is narrower than "drop the roll": apply ACL changes live, and roll a node only when the live apply fails with WRONGPASS. on that signal the operator stamps the current ACL hash onto the pod template, the pod restarts and re-reads the aclfile, and the node recovers. the hash is preserved across reconciles so a recovered node doesn't roll again, and it's never stamped on the happy path, so an ordinary user add or permission change still applies live without touching the pods. e2e covers both paths now: adding a user to a running cluster is live with no roll (the pods keep their UIDs), and deleting the password secret recovers through a roll (the cluster returns to Ready and the locked-out pods are recreated). both green locally on kind. |
|
Thanks, I ran another test locally with the latest commit and deployed a cluster with below: I came across a bug where there the unmanaged Are you able to add a test for this case, before adding the fix? Not for this PR, but I wonder if its worth making the |
|
good find, and thanks for the test-first nudge. reproduced it, added the test, then the fix. the mismatch is the unmanaged while i was in there i took greptile's two summary points, since they land on the same recovery path:
on always-managing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe controller applies ACL changes live with ChangesLive ACL synchronization
Sequence Diagram(s)sequenceDiagram
participant ACLSecret
participant Controller
participant ValkeyClient
participant ValkeyServer
ACLSecret->>Controller: Secret update event
Controller->>ValkeyClient: Load mounted ACL file
ValkeyClient->>ValkeyServer: ACL LOAD
ValkeyClient->>ValkeyServer: Query users and password hashes
ValkeyServer-->>Controller: ACL state
Controller-->>Controller: Set ACLApplied or requeue
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
internal/controller/valkeynode_controller.go (3)
862-864: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog the List failure instead of dropping the event silently.
If
Listfails, the function returns no requests and the ACL Secret change produces no reconcile. The node then waits for the 60s periodic requeue, and nothing records why. Log the error so the delay is diagnosable.🔧 Proposed fix
var nodes valkeyiov1alpha1.ValkeyNodeList if err := r.List(ctx, &nodes, client.InNamespace(secret.GetNamespace())); err != nil { + logf.FromContext(ctx).Error(err, "failed to list ValkeyNodes for ACL Secret event", + "secret", secret.GetName(), "namespace", secret.GetNamespace()) return nil }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/valkeynode_controller.go` around lines 862 - 864, Update the List error branch in the controller function containing client.List to log the encountered error before returning nil, using the controller’s existing logger and preserving the current no-request return behavior.
263-282: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared condition-patch helper.
setACLConditionandsetLiveConfigCondition(Lines 284-303) are identical except for the condition type and the error strings.clearLiveConfigCondition(Lines 309-322) repeats the same Get/DeepCopy/Patch shape. A single helper parameterised by condition type removes two copies and keeps the patch semantics in one place.♻️ Proposed refactor
func (r *ValkeyNodeReconciler) patchCondition( ctx context.Context, node *valkeyiov1alpha1.ValkeyNode, condType string, status metav1.ConditionStatus, reason, message string, ) error { current := &valkeyiov1alpha1.ValkeyNode{} if err := r.Get(ctx, client.ObjectKeyFromObject(node), current); err != nil { return fmt.Errorf("get ValkeyNode: %w", err) } patchBase := current.DeepCopy() if !meta.SetStatusCondition(¤t.Status.Conditions, metav1.Condition{ Type: condType, Status: status, Reason: reason, Message: message, ObservedGeneration: current.Generation, }) { return nil } if err := r.Status().Patch(ctx, current, client.MergeFrom(patchBase)); err != nil { return fmt.Errorf("patch %s condition: %w", condType, err) } return nil }Then
setACLConditionbecomes a one-line call withvalkeyiov1alpha1.ValkeyNodeConditionACLApplied.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/valkeynode_controller.go` around lines 263 - 282, Extract the shared Get, DeepCopy, SetStatusCondition, and status Patch logic from setACLCondition, setLiveConfigCondition, and clearLiveConfigCondition into a parameterized patchCondition helper accepting the condition type, status, reason, and message. Update each existing method to delegate to patchCondition with its appropriate condition type while preserving current error context and patch semantics.
460-469: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated roll-hash preservation logic in both workload paths.
ensureStatefulSetandensureDeploymentcontain the same five-line block that reads the existinghashAnnotationKeyand falls back to it whenaclRollHashis empty. The shared root cause is one preservation rule implemented twice, so a future change to the recovery-stamp semantics must be applied in two places.
internal/controller/valkeynode_controller.go#L460-L469: replace the inline fallback with a call to a shared helper, for exampleresolveACLRollHash(sts.Spec.Template.Annotations, aclRollHash).internal/controller/valkeynode_controller.go#L492-L501: call the same helper withdep.Spec.Template.Annotations.Keep the helper call before the
Specassignment in both functions. The current ordering is load-bearing, because assigningdesired.Specdiscards the existing annotations.♻️ Proposed helper
// resolveACLRollHash keeps any ACL-roll hash a prior recovery stamped, unless // the caller is forcing a recovery roll with a fresh hash. func resolveACLRollHash(existing map[string]string, aclRollHash string) string { if aclRollHash != "" { return aclRollHash } return existing[hashAnnotationKey] }Then in each workload function:
- rollHash := aclRollHash - if rollHash == "" { - rollHash = sts.Spec.Template.Annotations[hashAnnotationKey] - } + rollHash := resolveACLRollHash(sts.Spec.Template.Annotations, aclRollHash) sts.Labels = desired.Labels sts.Spec = desired.Spec🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/valkeynode_controller.go` around lines 460 - 469, The roll-hash preservation logic is duplicated across both workload paths. In internal/controller/valkeynode_controller.go:460-469, add and use a shared resolveACLRollHash helper with sts.Spec.Template.Annotations before assigning desired.Spec; in internal/controller/valkeynode_controller.go:492-501, replace the duplicate fallback with the same helper using dep.Spec.Template.Annotations before its Spec assignment. The helper must prefer a non-empty aclRollHash and otherwise return the existing hashAnnotationKey value.docs/valkeycluster.md (1)
316-321: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a convergence wait after the old key is removed.
The procedure ends at step 4. The removal of the old key is also propagated lazily, so the old password stays valid on some nodes for a period after step 4. State that the rotation completes only after every ValkeyNode reports
ACLApplied=Trueagain.📝 Proposed documentation change
1. Add the new password as a second key in `passwordSecret.keys`, keeping the old one. 2. Wait for every ValkeyNode to report `ACLApplied=True`. 3. Move your clients to the new password. 4. Remove the old key. +5. Wait for every ValkeyNode to report `ACLApplied=True` again. Until then, some nodes still accept the old password.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/valkeycluster.md` around lines 316 - 321, Update the password rotation procedure in the ValkeyCluster documentation to add a final convergence wait after removing the old key. State that rotation is complete only when every ValkeyNode reports ACLApplied=True again, reflecting propagation of the key removal.internal/controller/valkeynode_acl.go (1)
107-157: 🚀 Performance & Scalability | 🔵 TrivialConsider bounding the steady-state reload rate.
applyLiveACLruns on every reconcile, so each node issuesACL LOADplus oneACL USERSand oneACL GETUSERper user every 60 seconds, indefinitely, even when nothing changed. The unconditional reload is deliberate and correct for convergence, so this is not a defect.Two operational suggestions:
- Add a metric or counter for reloads and for
ACLAppliedtransitions. A silent reload loop is hard to observe today, and thePendingPropagationpath requeues every 10 seconds.- If the steady-state cost becomes visible on large ACLs, gate the reload on a change signal (Secret resource version observed in status) while keeping an unconditional reload on a longer interval as a safety net.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/valkeynode_acl.go` around lines 107 - 157, Add observability around applyLiveACL: record a metric or counter for each ACL LOAD and for ACLApplied state transitions, including the PendingPropagation requeue path as appropriate. Preserve the current unconditional reload and convergence behavior; do not gate reloads or change reconciliation semantics.internal/controller/valkeynode_controller_test.go (1)
1269-1353: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the ACL condition transitions in
Reconcile.These specs call
applyLiveACLdirectly, so the reconcile-level behavior added atvalkeynode_controller.goLines 230-256 stays untested:
ACLApplied=Falsewith reasonPendingPropagationand a 10 secondRequeueAfterwhen the mounted file is stale.ACLApplied=Truewith reasonAppliedand the 60 secondRequeueAfter.ACLApplied=Falsewith reasonApplyFailedand theLiveACLApplyFailedwarning event.The existing
Reconcilespecs cannot reach that code, because they return early at the!node.Status.Readycheck on Line 198.Two smaller points:
BeforeEachcreates thelive-acl-usersSecret but noAfterEachdeletes it. The Secret outlives the block in the shareddefaultnamespace.- No spec covers a Secret that exists without the
aclFilenamekey. That path returnstrueatvalkeynode_acl.goLine 144.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/controller/valkeynode_controller_test.go` around lines 1269 - 1353, Extend the applyLiveACL specs with Reconcile-level coverage by providing a ready node and asserting ACLApplied PendingPropagation with a 10-second requeue for stale files, Applied with a 60-second requeue after convergence, and ApplyFailed plus the LiveACLApplyFailed warning event on load failure. Add AfterEach cleanup for the live-acl-users Secret. Also add coverage for an existing Secret missing aclFilename and verify applyLiveACL treats it as successfully handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/valkeynode_controller.go`:
- Around line 401-428: Update recoverFromAuthLockout and the workload template
stamping used by ensureWorkload/ensureStatefulSet so recovery rolls are keyed by
a distinct lockout event rather than only the ACL Secret hash. Persist the hash
together with a truncated observed lockout timestamp or recovery counter,
reusing the same stamp for repeated reconciles within the chosen startup window
but changing it for a later lockout even when the ACL hash repeats.
In `@test/e2e/valkeycluster_test.go`:
- Around line 115-138: Move the ACL convergence assertion block containing
verifyACLApplied to immediately after the cluster-ready assertion using
verifyCrStatus, so pod startup and cluster formation are complete before
checking ACLApplied. Keep the existing validation logic and use a generous
timeout appropriate for ACL convergence after readiness.
- Around line 377-392: Wrap the pod identity comparison in the recovery
validation around podIdentities(Default) with Eventually so it waits until the
identities differ from beforePods, instead of checking once immediately after
the initial Ready state. Also validate that the cluster reaches
ClusterStateReady after the roll completes, while preserving the existing error
handling and timeout conventions.
- Around line 1736-1745: Increase the Eventually timeout from 3 minutes to 5
minutes for all three propagation-dependent assertions in the surrounding test,
including the ACL list check, credential authentication check, and the
ACLApplied=True block. Keep the existing 5-second polling interval and assertion
behavior unchanged.
---
Nitpick comments:
In `@docs/valkeycluster.md`:
- Around line 316-321: Update the password rotation procedure in the
ValkeyCluster documentation to add a final convergence wait after removing the
old key. State that rotation is complete only when every ValkeyNode reports
ACLApplied=True again, reflecting propagation of the key removal.
In `@internal/controller/valkeynode_acl.go`:
- Around line 107-157: Add observability around applyLiveACL: record a metric or
counter for each ACL LOAD and for ACLApplied state transitions, including the
PendingPropagation requeue path as appropriate. Preserve the current
unconditional reload and convergence behavior; do not gate reloads or change
reconciliation semantics.
In `@internal/controller/valkeynode_controller_test.go`:
- Around line 1269-1353: Extend the applyLiveACL specs with Reconcile-level
coverage by providing a ready node and asserting ACLApplied PendingPropagation
with a 10-second requeue for stale files, Applied with a 60-second requeue after
convergence, and ApplyFailed plus the LiveACLApplyFailed warning event on load
failure. Add AfterEach cleanup for the live-acl-users Secret. Also add coverage
for an existing Secret missing aclFilename and verify applyLiveACL treats it as
successfully handled.
In `@internal/controller/valkeynode_controller.go`:
- Around line 862-864: Update the List error branch in the controller function
containing client.List to log the encountered error before returning nil, using
the controller’s existing logger and preserving the current no-request return
behavior.
- Around line 263-282: Extract the shared Get, DeepCopy, SetStatusCondition, and
status Patch logic from setACLCondition, setLiveConfigCondition, and
clearLiveConfigCondition into a parameterized patchCondition helper accepting
the condition type, status, reason, and message. Update each existing method to
delegate to patchCondition with its appropriate condition type while preserving
current error context and patch semantics.
- Around line 460-469: The roll-hash preservation logic is duplicated across
both workload paths. In internal/controller/valkeynode_controller.go:460-469,
add and use a shared resolveACLRollHash helper with
sts.Spec.Template.Annotations before assigning desired.Spec; in
internal/controller/valkeynode_controller.go:492-501, replace the duplicate
fallback with the same helper using dep.Spec.Template.Annotations before its
Spec assignment. The helper must prefer a non-empty aclRollHash and otherwise
return the existing hashAnnotationKey value.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7dc78736-4531-44cb-885d-b5a8e7bcded3
📒 Files selected for processing (9)
api/v1alpha1/valkeynode_types.godocs/status-conditions.mddocs/valkeycluster.mdinternal/controller/users.gointernal/controller/valkeynode_acl.gointernal/controller/valkeynode_acl_test.gointernal/controller/valkeynode_controller.gointernal/controller/valkeynode_controller_test.gotest/e2e/valkeycluster_test.go
|
@melancholictheory We're going to prioritise the below PR as its a bug fix, I believe it blocks your PR since it includes logic on rolling pods if ACL has been updated. Your changes would undo that, so I think we will want to wait for this to be merged, then your change rebased against it. How does that sound? Appreciate the time you've put into this already. |
|
sounds right, and thanks for flagging it early. #338 is the better base: it makes the pod-template apply path staged and cluster-permitted, and #319 rewrites that same path (the ACL hash on the template, the recovery roll), so landing them the other way around would just have #319 stomping the permit gating. happy to wait and rebase on top. one thing i'll want to reconcile on the rebase, flagging it now so it isn't a surprise: #319's recovery roll fires when the operator is locked out of a node (WRONGPASS/NOPERM/NOAUTH after a credential change) and stamps the ACL hash to force a restart, because a restart is the only way the node reloads the aclfile and comes back. under #338 that stamp goes through the cluster permit like any other template change. that's fine for the common case, but a locked-out node is already broken, so i'll want to check the permit doesn't hold the recovery behind a health gate the broken node can't satisfy. i'll work that out against #338's actual mechanism rather than guess at it now. no rush on my end. ping me when it lands and i'll rebase. |
|
@melancholictheory #338 is now merged in, if you would like to rebase and remove the ACL component from the WorkloadRevision hash. My understanding is that ACLs should be taken out of the WorkloadRevision, because they are applied live via ACL LOAD - correct me if I am wrong. |
e1881db to
0e55136
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Thanks, rebased onto #338. You've got it right: ACL is out of the WorkloadRevision now. It's applied live with ACL LOAD instead, so an ACL edit no longer rolls the pods. What's in this revision:
Verified on a 3-shard cluster: adding a user shows up in One thing I pulled out into a follow-up rather than fold in here: recovering an operator that has locked itself out. A deleted or rotated password Secret leaves it on WRONGPASS, so it can't run ACL LOAD to fix itself. Previously the ACL-hash roll happened to recover that; without the roll it needs its own path. Two ways I can see: A. The locked-out node records the lockout and the cluster controller factors it into WorkloadRevision, so recovery goes through the same staged, one-at-a-time, failover-aware roll as everything else. B. A targeted delete of just the locked-out pod: node-local, without touching WorkloadRevision. I lean towards A, since it keeps recovery inside the staging model from #338 rather than adding an out-of-band pod delete, but B is simpler if you'd rather keep the lockout path self-contained. Which would you prefer? I also removed the old secret-deletion e2e assertion (it asserted a roll); it comes back with whichever recovery path we land on. |
0e55136 to
c7ee0c7
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/controller/valkeynode_acl_test.go`:
- Around line 28-37: Update the ACL fixture used by desiredUserPasswordHashes so
alice’s password hashes appear in reverse order, such as `#bbb` before `#aaa`, while
keeping the expected result as ["aaa", "bbb"]. This ensures the test verifies
sorting rather than preserving ACL-file order.
In `@test/e2e/valkeycluster_test.go`:
- Around line 1964-1970: Add an assertion to the existing plain-cluster e2e
scenario, after the cluster reaches Ready, that verifies every ValkeyNode has
the ACLApplied condition set to True. Keep the current managed default-user
setup unchanged and use the scenario’s existing cluster/node assertion helpers.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 248a2f8c-4ebe-4236-8f86-819b7cd51594
📒 Files selected for processing (13)
api/v1alpha1/valkeynode_types.gointernal/controller/failover.gointernal/controller/failover_test.gointernal/controller/users.gointernal/controller/valkeycluster_controller.gointernal/controller/valkeycluster_controller_test.gointernal/controller/valkeynode_acl.gointernal/controller/valkeynode_acl_test.gointernal/controller/valkeynode_controller.gointernal/controller/valkeynode_controller_test.gointernal/controller/workload_roll.gointernal/controller/workload_roll_test.gotest/e2e/valkeycluster_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- api/v1alpha1/valkeynode_types.go
- internal/controller/valkeynode_acl.go
- internal/controller/users.go
- internal/controller/valkeynode_controller.go
bjosv
left a comment
bjosv
left a comment
There was a problem hiding this comment.
In docs/valkeycluster.md we mention that configurations can be applied live without rolling pods. We could add info about this new handling as well? (under ### Users?)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/status-conditions.md`:
- Around line 212-215: Update the ACL status documentation near the
ACLApplied=False reasons to explicitly name the LiveACLApplyFailed Kubernetes
warning event, preferably alongside ApplyFailed or in the ACL event table, so
operators can correlate live ACL application failures with emitted events.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 536de949-3f87-4f85-8b65-653a09cb1d8b
📒 Files selected for processing (4)
docs/status-conditions.mddocs/valkeycluster.mdinternal/controller/valkeynode_acl_test.gotest/e2e/valkeycluster_test.go
ACL was carried in the pod template annotations, so an ACL edit changed Spec.WorkloadRevision and rolled every node one at a time (valkey-io#338). The operator can reload the ACL on a running server with ACL LOAD, so a roll is unnecessary: this takes ACL out of the WorkloadRevision hash and reloads the mounted aclfile live instead. - buildPodTemplateAnnotations no longer stamps the ACL hash, so ACL edits do not enter the workload revision and never roll a pod. - the ValkeyNode controller reloads the mounted aclfile (ACL LOAD) on reconcile and watches the internal ACL Secret, so an edit is picked up promptly rather than on the next resync. It reports an ACLApplied condition once the desired user set and password hashes are observably live. - the operator user gains +acl|load, +acl|getuser and +acl|users. - drop the now-dead ACL-secret threading through the cluster controller and the failover roll preflight. Recovering an operator locked out by a deleted password Secret needs a staged recovery through the cluster controller and is left as a follow-up. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
- The desiredUserPasswordHashes test fed already-sorted hashes, so it passed whether or not the parser sorted. Feed them out of order so the test actually exercises the sort. - The plain-cluster e2e sets no custom users, so its only server user is Valkey's unmanaged `default`. Assert every node still reaches ACLApplied=True there, covering the unmanaged-default path that the managed-user specs never hit. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
Add an ACLApplied section to docs/status-conditions.md and a note under Users in docs/valkeycluster.md explaining that ACL changes apply live via ACL LOAD without a pod roll, matching the existing live-config wording. Also correct a stale example that still listed an ACL secret hash as a workload-revision trigger; ACL no longer enters the pod template. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
- Remove the leftover pre-existing comment above buildPodTemplateAnnotations; only the rewritten one (ACL hash deliberately absent) applies now. - Rename normaliseHashes to normalizeHashes to match normalizePodTemplate and the rest of the tree. - Document the LiveACLApplyFailed warning event in docs/status-conditions.md, both in the ACL event table and the ACLApplied notes. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
7d2aa17 to
596b3a5
Compare
Removing the ACL hash from the pod template means an upgraded cluster rolls once to drop the now-unused annotation, and that roll is what reloads the aclfile and grants the operator user the ACL commands live application needs. - Document the upgrade behaviour under Users, including the one case that needs a manual restart: a cluster old enough to predate the annotation. - Add an e2e that stamps the legacy annotation on a running cluster and asserts the operator strips it (the migration roll) while the cluster stays Ready with ACLApplied=True. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
|
@melancholictheory I did some testing on this to verify it. I noticed with changing some fields that aren't "observed" that we lose their visibility on the ACLApplied condition. For example, I set up this command to watch for the condition: Then I changed fields under
For these actions, they did not change to
I believe you called out this limitation earlier in the thread. Perhaps we can document the live ACL apply limitation for now and raise issues to fix these in the future - how does that sound? I am aware a lot of good work has gone into this PR and don't want to hold it out. |
|
Sounds good, and thanks for testing it. That matches the design: I'll document that in this PR: what the condition observes exactly, and that For the follow-up, the revision sentinel I prototyped above closes exactly this gap (a hash of the whole managed ACL, so |
| It("removes a legacy internal-acl-hash annotation so an upgraded cluster migrates to live ACL", Label("acl-hash-migration"), func() { | ||
| defer func() { | ||
| _, _ = utils.Run(exec.Command("kubectl", "delete", "valkeycluster", clusterName, "--ignore-not-found=true", "--wait=false")) | ||
| _, _ = utils.Run(exec.Command("kubectl", "delete", "secret", usersSecret, "--ignore-not-found=true", "--wait=false")) | ||
| }() | ||
|
|
||
| By("creating a ValkeyCluster with a custom user set") | ||
| cmd := exec.Command("kubectl", "apply", "-f", "-") | ||
| cmd.Stdin = strings.NewReader(manifest) | ||
| _, err := utils.Run(cmd) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
|
|
||
| By("waiting for the cluster to become Ready with live ACL applied") | ||
| Eventually(func(g Gomega) { | ||
| cr, err := utils.GetValkeyClusterStatus(clusterName) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(cr.Status.State).To(Equal(valkeyiov1alpha1.ClusterStateReady)) | ||
| g.Expect(cr.Status.ReadyShards).To(Equal(int32(3))) | ||
| }, 10*time.Minute, 5*time.Second).Should(Succeed()) | ||
| Eventually(expectACLAppliedTrue, 5*time.Minute, 5*time.Second).Should(Succeed()) | ||
|
|
||
| By("stamping the legacy internal-acl-hash annotation on every server StatefulSet") | ||
| // Operator versions before live ACL stamped the ACL hash on the pod | ||
| // template. Reproduce that pre-upgrade state, then assert the current | ||
| // operator migrates off it: removing the annotation is the one-time | ||
| // roll that reloads the aclfile and grants _operator the ACL commands. | ||
| var stsNames []string | ||
| Eventually(func(g Gomega) { | ||
| stsNames = serverStatefulSets(g) | ||
| g.Expect(stsNames).To(HaveLen(6)) | ||
| }).Should(Succeed()) | ||
| for _, sts := range stsNames { | ||
| _, err := utils.Run(exec.Command("kubectl", "patch", "statefulset", sts, "--type", "merge", | ||
| "-p", `{"spec":{"template":{"metadata":{"annotations":{"valkey.io/internal-acl-hash":"simulated-legacy"}}}}}`)) | ||
| Expect(err).NotTo(HaveOccurred()) | ||
| } | ||
|
|
||
| By("the operator strips the annotation from every StatefulSet (the migration roll)") | ||
| Eventually(func(g Gomega) { | ||
| for _, sts := range serverStatefulSets(g) { | ||
| out, err := utils.Run(exec.Command("kubectl", "get", "statefulset", sts, | ||
| "-o", "jsonpath={.spec.template.metadata.annotations.valkey\\.io/internal-acl-hash}")) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(strings.TrimSpace(out)).To(BeEmpty(), | ||
| "operator must strip the legacy ACL-hash annotation from %s", sts) | ||
| } | ||
| }, 5*time.Minute, 5*time.Second).Should(Succeed()) | ||
|
|
||
| By("the cluster returns to Ready and ACL stays live after the migration") | ||
| Eventually(func(g Gomega) { | ||
| cr, err := utils.GetValkeyClusterStatus(clusterName) | ||
| g.Expect(err).NotTo(HaveOccurred()) | ||
| g.Expect(cr.Status.State).To(Equal(valkeyiov1alpha1.ClusterStateReady)) | ||
| }, 10*time.Minute, 5*time.Second).Should(Succeed()) | ||
| Eventually(expectACLAppliedTrue, 5*time.Minute, 5*time.Second).Should(Succeed()) |
There was a problem hiding this comment.
This test creates the cluster with the current _operator ACL, waits for ACLApplied=True, and only then adds the legacy workload annotation. The current ACL already grants acl|load, acl|getuser, and acl|users, so removing the annotation verifies template reconciliation but not the upgrade behavior this test is intended to protect: a legacy-running pod must be replaced and start with those newly granted commands. Construct the old operator ACL without these permissions, prove the rollout replaces the pods, and verify the three ACL commands succeed as _operator after replacement.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Artifacts
Targeted ACL migration coverage check source
- Authored executable that runs the focused ACL tests and checks whether the migration spec constructs the legacy permission state, showing the precise validation logic used.
Focused ACL tests and migration coverage check output
- Captured output from the authored check: controller ACL tests pass, while the inspected migration setup is shown to start with the current ACL and therefore not cover legacy missing permissions.
ACL-hash migration e2e attempt blocked before execution
- Captured targeted Ginkgo e2e invocation showing the suite failed in BeforeSuite because kubectl is missing and ran zero migration specs.
There was a problem hiding this comment.
Fair point, and you're right about what it covers. The spec runs against the current operator, whose _operator already holds acl|load/acl|getuser/acl|users, so it exercises the reconcile trigger (the operator strips the legacy annotation and ACL stays live across the roll) rather than the permission bootstrap itself.
I tried building the missing-permission state in-test by downgrading _operator with ACL SETUSER _operator -acl|load ..., but it isn't faithful: the operator's unconditional ACL LOAD reloads the aclfile (which still grants those commands) and heals the downgrade within a reconcile, without any roll. A real upgrade has no such window, the server boots from the old aclfile without the commands, so every ACL LOAD is NOPERM and the roll is the only thing that grants them. Reproducing that faithfully needs a second, pre-live-ACL operator build deployed first, which the single-image e2e harness can't do.
I did validate the full bootstrap end to end outside the suite: created a cluster on the operator built from current main (_operator without the grants, annotation present), upgraded to this build, and confirmed the pods rolled once, ACL GETUSER _operator then showed all three commands, and a later ACL edit applied live. That's the run I described above.
I've reworded the spec's title and comment so it claims only the trigger and the live-ACL survival, not the grant. If you'd rather I drop the spec, happy to do that too.
There was a problem hiding this comment.
That's a solid explanation and the constraint is real — you can't synthesize the pre-upgrade boot state without a second operator image, and the in-test downgrade approach correctly identified that ACL LOAD would immediately heal it before any roll could happen.
Narrowing the spec's claimed scope to "trigger + live-ACL survival" is the right call. Keeping it is fine; it does cover a real code path (the annotation strip triggers a reconcile, the cluster survives the roll, and live ACL resumes), and the comment now accurately describes what it exercises. No need to drop it.
Withdrawing the original concern.
Per review, spell out what the condition observes: only the user set and password hashes drive it through PendingPropagation, so a change to a user's enabled flag or permissions applies live but does not show a transient PendingPropagation. Tracking every field is left as a follow-up. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
|
@melancholictheory Thanks, I agree with this:
|
The spec runs against the current operator, whose _operator user already holds the ACL commands, so it cannot exercise the permission bootstrap (that needs a pre-live-ACL operator build). Reword the title and comment so it claims only what it checks: the operator strips the legacy annotation and ACL stays live across the resulting roll. Signed-off-by: melancholictheory <selimvhorst@gmail.com>
| // aclObservablyInSync reports whether the parts of the ACL that can be compared | ||
| // exactly are live on the server: the set of users, and each user's password | ||
| // hashes. | ||
| // | ||
| // Permissions are deliberately not compared. ACL GETUSER returns Valkey's | ||
| // normalized rendering of the rules, while the operator only holds the aclfile | ||
| // text, so comparing the two would mean reimplementing Valkey's own ACL parser | ||
| // and keeping it in step with the server. Correctness of the apply does not | ||
| // depend on this check either way: the reload is unconditional, so permission | ||
| // edits converge regardless. This only scopes what ACLApplied can honestly | ||
| // claim. | ||
| func aclObservablyInSync(ctx context.Context, c valkeyConfigClient, desired map[string][]string) (bool, error) { | ||
| actualUsers, err := c.UserNames(ctx) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| serverUsers := slices.Sorted(slices.Values(actualUsers)) | ||
| // Valkey always keeps a `default` user of its own, and ACL LOAD cannot | ||
| // remove it. When the aclfile does not manage `default` (it is absent from | ||
| // spec.users), ignore the server's copy; otherwise the sets never match and | ||
| // the node loops forever reporting the ACL as not yet live. | ||
| if _, managed := desired["default"]; !managed { | ||
| serverUsers = slices.DeleteFunc(serverUsers, func(u string) bool { return u == "default" }) | ||
| } | ||
| if !slices.Equal(serverUsers, slices.Sorted(maps.Keys(desired))) { | ||
| // A user was added or removed and the server has not picked it up yet. | ||
| return false, nil | ||
| } | ||
| for _, user := range slices.Sorted(maps.Keys(desired)) { | ||
| actual, err := c.UserPasswordHashes(ctx, user) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
| if !slices.Equal(actual, desired[user]) { | ||
| return false, nil | ||
| } | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
A permissions-only ACL update can set ACLApplied=True before the kubelet has refreshed the mounted Secret. When ACL LOAD reads the old file, aclObservablyInSync only compares user names and password hashes; unchanged identities therefore appear synchronized even if the old server ACL still grants a command that was just revoked. Consumers can act on the successful condition while the revoked permission remains available until a later reconcile.
Track a revision that is observable in the mounted ACL and loaded server state, or otherwise avoid reporting ACLApplied=True until the server can prove it consumed the current ACL content.
Artifacts
Focused Go reproduction source for stale ACL projection
- A temporary focused Go test harness creates a fake Secret reader and config client that models a successful ACL LOAD reading the old projected file, with the takeaway that password-only observation cannot see retained permissions.
Password-change stale projection control output
- The executed focused Go control simulates a stale projected ACL with a changed hash and passes after confirming it is not synchronized, with the takeaway that hash changes are detected.
Permissions-only stale projection reproduction output
- The executed focused Go reproduction simulates a command revocation with unchanged users and hashes and passes after confirming current code reports it synchronized, with the takeaway that ACLApplied can become true while old permissions remain live.
Initial focused ACL test patch
- The captured authored patch documents the initial function-level test scenario used to target stale mounted ACL behavior, with the takeaway that the validation remained limited to the ACL apply path.
There was a problem hiding this comment.
This is the permissions-only window we've been working through with @jdheyburn above. He tested it and confirmed exactly this: user and password changes move ACLApplied through PendingPropagation, while enabled and permission edits apply but don't drive the condition. The call for this PR is to document the limitation and fix it in a follow-up, which is where it stands: the ACLApplied docs now spell out that only the user set and password hashes drive the condition, and the revision sentinel I prototyped earlier in the thread closes the gap (a hash of the whole managed ACL, so permission edits move the condition too).
There was a problem hiding this comment.
Thanks for the context — that's clear. The docs caveat plus a tracked follow-up with the revision sentinel is the right call here; no need to block the PR on it. Acknowledged.
jdheyburn
left a comment
jdheyburn
left a comment
There was a problem hiding this comment.
Thanks once again for taking this on!
|
For the upgrade 0.4 -> 0.5 I noticed clusters that already had The Secret already includes Clears after the staged roll (or a one-off pod restart). Not blocking for us; but we may want to post that on the release notes. The errors stood out during upgrade. |
|
That's the expected upgrade path, thanks for flagging it. This version drops the ACL-hash annotation from the pod template, so every pre-existing cluster rolls once, and that roll is what restarts each pod onto the new It's written up under Users in |
|
I've updated the release notes with the same: https://github.com/valkey-io/valkey-operator/releases/tag/v0.5.0 |
|
One small thing on the release-note wording: "manual restart required" reads a bit stronger than what actually happens. On upgrade the operator drops the ACL-hash annotation from the pod template, which triggers the one-time staged roll on its own, and that roll reloads A manual restart only helps if you want it cleared immediately, or for a cluster old enough to predate that annotation (no annotation to drop means no automatic roll). Might be worth softening the note to something like: "the one-time staged roll on upgrade clears this on its own; a manual restart is only needed to clear it immediately, or for clusters that predate the ACL-hash annotation." |
5 participants
Closes #212
Summary
ACL changes only took effect when a pod restarted and reloaded the aclfile. The ValkeyNode controller now applies them in place with
ACL LOAD, so a password rotation or a permission change no longer needs a roll.Features / Behaviour Changes
ACLAppliedcondition onValkeyNodereporting whether the server's ACL matches the aclfile Secret.Implementation
Per the direction on #212:
ACL LOADof the mounted file at the end of the node reconcile, since that file is already the full desired state and is exactly what a restart would load.Two things shaped the rest:
ACL SAVEis out. The aclfile is mounted read-only from its Secret, so it isn't available, and it isn't needed either: the Secret the cluster controller writes is what a restarting pod loads. So the Secret stays the source of truth for restart, andACL LOADis purely the live path.The mounted copy lags. kubelet refreshes it lazily, so an
ACL LOADissued right after the Secret changes can read the previous contents and silently do nothing. The operator can't read the pod's file without an exec, so the running server is the source of truth for verification instead: the controller compares the password hashes fromACL GETUSERagainst the aclfile Secret, and only issues a reload when they differ, then re-checks. That makes a premature reload self-correcting, it leaves the nodeACLApplied=Falsewith reasonPendingPropagationrather than claiming applied, and the requeue retries once the volume catches up. A steady-state reconcile issues no writes at all, only the comparison.ACLApplieddeliberately does not gate the cluster's rolling update the wayLiveConfigApplieddoes: a node whose ACL is still propagating keeps serving with its previous credentials, and a roll would load the new file regardless.Rotating without an auth gap
Documented in
docs/valkeycluster.md: a Valkey user can hold several passwords at once, andpasswordSecret.keysalready accepts multiple keys, so adding the new password alongside the old, waiting forACLApplied=True, moving clients over, then dropping the old key keeps every credential valid throughout the window. Replacing the key outright locks out any client still holding the old password until it catches up.Testing
nopass, system users, non-user lines) and the in-sync comparison.applyLiveACL: no secret, missing secret, already in sync (asserts noACL LOADis issued), reload that converges, reload against a stale volume (asserts it reports not-synced rather than applied), and a failing reload.make testandmake lintpass; no generated drift.Checklist
make testandmake lintinstead)