Add default system users (#110) by hieu2102 · Pull Request #129 · valkey-io/valkey-operator

hieu2102 · 2026-04-07T09:14:34Z

implements #110

add defaults system users _operator and _exporter
- update GetClusterState, GetNodeState functions to connect to Valkey using _operator user
- fall back to default (unauthenticated on WRONGPASS error)
- metrics-exporter container will connect using _exporter user (by setting REDIS_USER, REDIS_PASSWORD env variables)
add CEL validation to reject users with named started with _
add ACL hash annotation to pod template, when the internal ACL secret (internal-<clusterName>-acl) changed, the pods will be recreated automatically

Signed-off-by: hieu2102 <hieund2102@gmail.com>

jdheyburn

Thanks for taking a look at this! I've not tested this myself yet, but left some comments.

jdheyburn · 2026-04-08T12:38:49Z

 }
+
+func generatePassword() string {
+	randstr := rand.Text()


What does this generate for us? i.e. password length, characters, etc.

I've added a comment about this in the latest commit

I checked the generated value. Could we have it to be upper and lower case letters too? Today its just uppercase.

Signed-off-by: hieu2102 <hieund2102@gmail.com>

jdheyburn

Looks good! I've some minor comments, but would also appreciate @utdrmac taking a look since they raised the original ACL feature.

jdheyburn · 2026-04-10T16:11:05Z

 }
+
+func generatePassword() string {
+	randstr := rand.Text()


I checked the generated value. Could we have it to be upper and lower case letters too? Today its just uppercase.

jdheyburn · 2026-04-10T16:16:29Z

+			Namespace: cluster.Namespace,
+			Labels:    labels(cluster),
+		},
+		Data: map[string][]byte{},


Do we also need to specify the type here like what we did for the ACLs? @utdrmac what do you think?

Good point, let me add it

Signed-off-by: hieu2102 <hieund2102@gmail.com>

jdheyburn

Thanks for working on this!

getValkeyRole in the ValkeyNode controller connects to Valkey unauthenticated to check the replication role. This was missed in valkey-io#129 when the cluster controller was updated to use _operator credentials. Once the default user is restricted, the role check fails silently and ValkeyNode status.role is always empty. Signed-off-by: Daan Vinken <daanvinken@tythus.com>

daanvinken · 2026-04-15T15:24:27Z

I think we missed one here #137

implements valkey-io#110 - add defaults system users `_operator` and `_exporter` - update `GetClusterState`, `GetNodeState` functions to connect to Valkey using `_operator` user - fall back to `default` (unauthenticated on WRONGPASS error) - `metrics-exporter` container will connect using `_exporter` user (by setting `REDIS_USER`, `REDIS_PASSWORD` env variables) - add CEL validation to reject users with named started with `_` - add ACL hash annotation to pod template, when the internal ACL secret (`internal-<clusterName>-acl`) changed, the pods will be recreated automatically --------- Signed-off-by: hieu2102 <hieund2102@gmail.com>

getValkeyRole in the ValkeyNode controller connects to Valkey unauthenticated to check the replication role. This was missed in valkey-io#129 when the cluster controller was updated to use _operator credentials. Once the default user is restricted, the role check fails silently and ValkeyNode status.role is always empty. Signed-off-by: Daan Vinken <daanvinken@tythus.com>

This PR is a follow-up to #129 which added `_operator` system user authentication to the cluster controller but missed the `getValkeyRole` code path in the ValkeyNode controller. ### Summary `getValkeyRole` connects to Valkey unauthenticated to check the replication role. Once the default user is restricted or disabled via `spec.users`, the role check fails silently and `ValkeyNode.status.role` is always empty. ### Implementation Pass `_operator` credentials to `getValkeyRole`, matching how the cluster controller authenticates in `GetClusterState`. If the system password secret doesn't exist yet (fresh cluster before ACLs are set up), the password is empty and the connection falls back to unauthenticated. ### Limitations None. ### Testing Existing E2E tests cover this path (role is populated in ValkeyNode status). No behavioral change when the default user is open. ### Checklist Before submitting the PR make sure the following are checked: - [x] This Pull Request is related to one issue. - [x] Commit message explains what changed and why - [NA] Tests are added or updated. - [NA] Documentation files are updated. - [x] I have run pre-commit locally (`pre-commit run --all-files` or hooks on commit) Signed-off-by: Daan Vinken <daanvinken@tythus.com>

…#137) This PR is a follow-up to valkey-io#129 which added `_operator` system user authentication to the cluster controller but missed the `getValkeyRole` code path in the ValkeyNode controller. ### Summary `getValkeyRole` connects to Valkey unauthenticated to check the replication role. Once the default user is restricted or disabled via `spec.users`, the role check fails silently and `ValkeyNode.status.role` is always empty. ### Implementation Pass `_operator` credentials to `getValkeyRole`, matching how the cluster controller authenticates in `GetClusterState`. If the system password secret doesn't exist yet (fresh cluster before ACLs are set up), the password is empty and the connection falls back to unauthenticated. ### Limitations None. ### Testing Existing E2E tests cover this path (role is populated in ValkeyNode status). No behavioral change when the default user is open. ### Checklist Before submitting the PR make sure the following are checked: - [x] This Pull Request is related to one issue. - [x] Commit message explains what changed and why - [NA] Tests are added or updated. - [NA] Documentation files are updated. - [x] I have run pre-commit locally (`pre-commit run --all-files` or hooks on commit) Signed-off-by: Daan Vinken <daanvinken@tythus.com>

hieu2102 marked this pull request as draft April 7, 2026 10:08

hieu2102 added 10 commits April 8, 2026 11:01

add func upsertInternalAclSecret

f65bdfb

Signed-off-by: hieu2102 <hieund2102@gmail.com>

reconcile system users acls

4660792

Signed-off-by: hieu2102 <hieund2102@gmail.com>

add system users acl rawstrings

88ffe01

Signed-off-by: hieu2102 <hieund2102@gmail.com>

add CEL validation for user acls

09b5914

Signed-off-by: hieu2102 <hieund2102@gmail.com>

add auth support for exporter

9d7db2f

Signed-off-by: hieu2102 <hieund2102@gmail.com>

add CEL validation for username

e87aeb4

Signed-off-by: hieu2102 <hieund2102@gmail.com>

fix reconciler only create system users

a12d4a7

Signed-off-by: hieu2102 <hieund2102@gmail.com>

add auth fallback to unauthenticated

2dca81f

Signed-off-by: hieu2102 <hieund2102@gmail.com>

add acl hash annotation to pod template

6993152

Signed-off-by: hieu2102 <hieund2102@gmail.com>

add e2e test

a699a92

Signed-off-by: hieu2102 <hieund2102@gmail.com>

hieu2102 force-pushed the add-system-users branch from 39d2000 to a699a92 Compare April 8, 2026 04:01

hieu2102 added 3 commits April 8, 2026 14:05

fix lint

9b08d3d

Signed-off-by: hieu2102 <hieund2102@gmail.com>

fix ValkeyNode unit tests

d276fb2

Signed-off-by: hieu2102 <hieund2102@gmail.com>

fix e2e tests

027426a

Signed-off-by: hieu2102 <hieund2102@gmail.com>

hieu2102 marked this pull request as ready for review April 8, 2026 09:49

jdheyburn reviewed Apr 8, 2026

View reviewed changes

hieu2102 added 2 commits April 9, 2026 13:18

do not create exporter user if exporter.enabled is false

5d8d6bb

Signed-off-by: hieu2102 <hieund2102@gmail.com>

pass only LabelCluster to generateMetricsExporterContainerDef

aa8e5b7

Signed-off-by: hieu2102 <hieund2102@gmail.com>

jdheyburn reviewed Apr 10, 2026

View reviewed changes

Comment thread internal/controller/users.go

jdheyburn reviewed Apr 10, 2026

View reviewed changes

hieu2102 added 2 commits April 13, 2026 12:01

update generatePassword to include lowercase characters

952fc32

Signed-off-by: hieu2102 <hieund2102@gmail.com>

add type to system user password secret

e1816d9

Signed-off-by: hieu2102 <hieund2102@gmail.com>

jdheyburn reviewed Apr 14, 2026

View reviewed changes

Comment thread api/v1alpha1/valkeycluster_types.go Outdated

revert remove omitempty from ExporterSpec.Enabled

4985f27

Signed-off-by: hieu2102 <hieund2102@gmail.com>

jdheyburn approved these changes Apr 14, 2026

View reviewed changes

jdheyburn merged commit 5b9adec into valkey-io:main Apr 14, 2026
7 checks passed

jdheyburn mentioned this pull request Apr 14, 2026

feat: Add default system users for operator and exporter authentication #110

Closed

10 tasks

daanvinken mentioned this pull request Apr 15, 2026

fix: authenticate getValkeyRole with _operator credentials #137

Merged

3 tasks

Conversation

hieu2102 commented Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jdheyburn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jdheyburn Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

hieu2102 Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

jdheyburn Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jdheyburn left a comment

Choose a reason for hiding this comment

Uh oh!

jdheyburn Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

jdheyburn Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

hieu2102 Apr 13, 2026

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!

daanvinken commented Apr 15, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hieu2102 commented Apr 7, 2026 •

edited

Loading