(feat) Support for creating users on cluster init by utdrmac · Pull Request #82 · valkey-io/valkey-operator

utdrmac · 2026-02-10T17:28:23Z

Resolves #36

This feature allows for the creation of Valkey users on cluster initialization. The feature abstracts Valkey ACL permissions into several objects, giving better readability, and creation flexibility to users that may not be intimately familiar with Valkey ACLs.

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac · 2026-02-10T17:29:35Z

e2e tests pass in my local kind cluster

=== RUN   TestE2E
  Starting valkey-operator e2e test suite
Running Suite: e2e suite - /home/utdrmac/dockers/valkey-operator/test/e2e
=======================================================================
Random Seed: 1770743918

Will run 10 of 10 specs
------------------------------
...
Ran 10 of 10 Specs in 135.056 seconds
SUCCESS! -- 10 Passed | 0 Failed | 0 Pending | 0 Skipped
--- PASS: TestE2E (135.06s)
PASS
ok  	valkey.io/valkey-operator/test/e2e	135.065s
make cleanup-test-e2e
...

utdrmac · 2026-02-10T17:32:10Z

test/e2e/e2e_suite_test.go

 var _ = BeforeSuite(func() {
-	By("building the manager image")
-	cmd := exec.Command("make", "docker-build", fmt.Sprintf("IMG=%s", managerImage))
+	By("purging old events")


K8S holds on to events for an extended period of time, even after a cluster is deleted. This call to purge events was added because subsequent e2e tests would fail if a previous test failed due to the e2e test fetching all events.

Good catch. We could move it to valkeycluster_test.go to have it close to where its needed.
This is also the only testcase/context where we use involvedObject.name=valkeycluster-sample.
I guess it needs to be in a BeforeEach instead then.

utdrmac · 2026-02-10T17:33:37Z

Makefile


 .PHONY: build
-build: manifests generate fmt vet ## Build manager binary.
+build: manifests generate fmt vet lint ## Build manager binary.


The golangci-lint was only ran during github e2e tests, and it was frustrating for this to fail remotely. Simple change to check this locally first.

utdrmac

I added some comments for 2 changes outside the scope of the PR.

internal/controller/valkeycluster_controller.go

jdheyburn

This is great to see, thank you for putting the time in! Just left some comments for clarity

internal/controller/users.go

internal/controller/valkeycluster_controller.go

internal/controller/users.go

sandeepkunusoth · 2026-02-11T17:37:10Z

api/v1alpha1/valkeyacls_types.go

+	// Do not apply a password to this user
+	// +kubebuilder:default=false
+	NoPassword bool `json:"nopass,omitempty"`
+


i think passwordEnabled or enabledPassword would be good naming here instead of NoPassword. Also i am not sure this is general use case of having nopass attached to user. if they really want to acheieve this they can always use rawacl.

It might not be general or recommended, but if Valkey exposes it as an option it might be useful to abstract it away.

https://valkey.io/topics/acl/#configure-acls-with-the-acl-command

This boolean is used in the PR in a few places. If we were to remove NoPassword here, would we change those conditions to be 'nopass' in RawAcl (pseudocode) ?

I think the idea was since the ACL keyword is nopass then this follows-ish 1:1. Since the default is false if not specified, is there a strong reason to remove it? As the code is now, if removed, then passwords are always searched for even in a nopass=true case. The code would then need to check rawAcl for presence of nopass to achieve same logic.

internal/controller/utils.go

Signed-off-by: utdrmac <matthew.boehm@percona.com>

jdheyburn · 2026-02-12T10:45:25Z

I think we're almost there with this PR, just pending @sandeepkunusoth approval from their remaining comments

bjosv · 2026-02-12T10:07:06Z

test/e2e/e2e_suite_test.go

 var _ = BeforeSuite(func() {
-	By("building the manager image")
-	cmd := exec.Command("make", "docker-build", fmt.Sprintf("IMG=%s", managerImage))
+	By("purging old events")


Good catch. We could move it to valkeycluster_test.go to have it close to where its needed.
This is also the only testcase/context where we use involvedObject.name=valkeycluster-sample.
I guess it needs to be in a BeforeEach instead then.

bjosv · 2026-02-12T10:26:30Z

internal/valkey/clusterstate.go

 	for _, shard := range s.Shards {
 		for _, slot := range shard.Slots {
-			var next []SlotsRange
+			var next []SlotsRange //nolint:prealloc


Interesting that this isn't showing in CI, our current main (given by kubebuilder) uses

make lint ... Downloading github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.7.2 0 issues.

which is using
go: downloading github.com/alexkohler/prealloc v1.0.0

Is this seen with newer versions of golangci-lint/prealloc?

Must be. I'm using golangci-lint has version 2.8.0 built with go1.25.5

bjosv

LGTM, my previous comments are nits

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac · 2026-02-19T03:32:53Z

Resolved conflict with c679809

Signed-off-by: utdrmac <matthew.boehm@percona.com>

jdheyburn · 2026-02-23T17:18:01Z

@utdrmac I think there is a bad merge pulling in some functions that no longer exist. Would you mind double checking to see what can be removed? Once that's done I'll approve and merge. Thanks again!

create users on cluster init

6851fe9

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac commented Feb 10, 2026

View reviewed changes

sandeepkunusoth reviewed Feb 11, 2026

View reviewed changes

internal/controller/valkeycluster_controller.go Show resolved Hide resolved

jdheyburn reviewed Feb 11, 2026

View reviewed changes

sandeepkunusoth reviewed Feb 11, 2026

View reviewed changes

internal/controller/utils.go Outdated Show resolved Hide resolved

updates from feedback

68a0a18

Signed-off-by: utdrmac <matthew.boehm@percona.com>

bjosv reviewed Feb 12, 2026

View reviewed changes

bjosv previously approved these changes Feb 13, 2026

View reviewed changes

merge origin/main

c679809

Signed-off-by: utdrmac <matthew.boehm@percona.com>

fmt fix

09cc75c

Signed-off-by: utdrmac <matthew.boehm@percona.com>

utdrmac dismissed bjosv’s stale review via 09cc75c February 19, 2026 14:44

Comments

Conversation

utdrmac commented Feb 10, 2026

Uh oh!

utdrmac commented Feb 10, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

utdrmac left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jdheyburn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jdheyburn commented Feb 12, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjosv left a comment

Choose a reason for hiding this comment

Uh oh!

utdrmac commented Feb 19, 2026

Uh oh!

jdheyburn commented Feb 23, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

utdrmac left a comment •

edited

Loading