From 6851fe962dcce5a9d5527d796126a6465fc257fb Mon Sep 17 00:00:00 2001 From: utdrmac Date: Tue, 10 Feb 2026 17:05:27 +0000 Subject: [PATCH 1/3] create users on cluster init Signed-off-by: utdrmac --- Makefile | 2 +- api/v1alpha1/valkeyacls_types.go | 102 ++++++ api/v1alpha1/valkeycluster_types.go | 6 + api/v1alpha1/zz_generated.deepcopy.go | 121 +++++++ .../crd/bases/valkey.io_valkeyclusters.yaml | 91 +++++ config/rbac/role.yaml | 1 + config/samples/v1alpha1_valkeycluster.yaml | 46 +++ internal/controller/deployment.go | 13 + internal/controller/users.go | 311 ++++++++++++++++++ internal/controller/users_test.go | 107 ++++++ internal/controller/utils.go | 36 +- .../controller/valkeycluster_controller.go | 14 +- internal/valkey/clusterstate.go | 2 +- test/e2e/e2e_suite_test.go | 9 +- test/e2e/valkeycluster_test.go | 42 +++ 15 files changed, 897 insertions(+), 6 deletions(-) create mode 100644 api/v1alpha1/valkeyacls_types.go create mode 100644 internal/controller/users.go create mode 100644 internal/controller/users_test.go diff --git a/Makefile b/Makefile index 5b75291..3cc3cb8 100644 --- a/Makefile +++ b/Makefile @@ -105,7 +105,7 @@ lint-config: golangci-lint ## Verify golangci-lint linter configuration ##@ Build .PHONY: build -build: manifests generate fmt vet ## Build manager binary. +build: manifests generate fmt vet lint ## Build manager binary. go build -o bin/manager cmd/main.go .PHONY: run diff --git a/api/v1alpha1/valkeyacls_types.go b/api/v1alpha1/valkeyacls_types.go new file mode 100644 index 0000000..fc6504a --- /dev/null +++ b/api/v1alpha1/valkeyacls_types.go @@ -0,0 +1,102 @@ +/* +Copyright 2025 Valkey Contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package v1alpha1 + +// An UserAclSpec contains user, authorization, and permissions-related configurations +type UserAclSpec struct { + + // Username + // +kubebuilder:required:message=A username is required + Name string `json:"name"` + + // If the user is enabled or not + // +kubebuilder:default=true + Enabled bool `json:"enabled,omitempty"` + + // Reference information to a Secret containing user passwords + // +optional + PasswordSecret PasswordSecretSpec `json:"passwordSecret,omitempty"` + + // Do not apply a password to this user + // +kubebuilder:default=false + NoPassword bool `json:"nopass,omitempty"` + + // Valkey command categories, commands, and subcommands restrictions for this user + // +optional + Commands CommandsAclSpec `json:"commands,omitempty"` + + // Key restrictions + // +optional + Keys KeysAclSpec `json:"keys,omitempty"` + + // Channel restrictions + // +optional + Channels ChannelsAclSpec `json:"channels,omitempty"` + + // Raw ACL for (additional) permissions. Appended to anything generated. + // +optional + RawAcl string `json:"permissions,omitempty"` +} + +type PasswordSecretSpec struct { + + // Name of the referencing Secret; Defaults to clustername-users + // +optional + Name string `json:"name,omitempty"` + + // An array of keys inside the referencing Secret to find passwords; defaults to username + // Valkey supports multiple passwords per user for rotation + // +optional + Keys []string `json:"keys,omitempty"` +} + +type CommandsAclSpec struct { + + // Command categories (@all, @read, @write, @admin, etc.) + // Individual commands (get, set, ping, etc.) + // Subcommands (client|setname, config|get, etc.) + + // Allowed commands for this user + // +kubebuilder:validation:Items:Pattern=^[@a-z|]+$} + Allow []string `json:"allow,omitempty"` + + // Denied commands for this user + // +kubebuilder:validation:Items:Pattern=^[@a-z|]+$} + Deny []string `json:"deny,omitempty"` +} + +type KeysAclSpec struct { + + // Keys on which this user can read, and write; maps to Valkey: ~pattern + // +optional + ReadWrite []string `json:"readWrite,omitempty"` + + // Keys restricted to read-only; maps to Valkey: %R~pattern + // +optional + ReadOnly []string `json:"readOnly,omitempty"` + + // Keys restricted to write-only; maps to Valkey: %W~pattern + // +optional + WriteOnly []string `json:"writeOnly,omitempty"` +} + +type ChannelsAclSpec struct { + + // Pub/Sub channel patterns - maps to Valkey: &pattern + // +optional + Patterns []string `json:"patterns,omitempty"` +} diff --git a/api/v1alpha1/valkeycluster_types.go b/api/v1alpha1/valkeycluster_types.go index f5dfad5..e1f9737 100644 --- a/api/v1alpha1/valkeycluster_types.go +++ b/api/v1alpha1/valkeycluster_types.go @@ -73,6 +73,11 @@ type ValkeyClusterSpec struct { // +kubebuilder:default:={enabled:true} // +optional Exporter ExporterSpec `json:"exporter,omitempty"` + + // Users, and ACL-related configuration; see valkeyacls_types.go + // +listType=map + // +listMapKey=name + Users []UserAclSpec `json:"users,omitempty"` } type ExporterSpec struct { @@ -154,6 +159,7 @@ const ( ReasonSlotsUnassigned = "SlotsUnassigned" ReasonPrimaryLost = "PrimaryLost" ReasonNoSlots = "NoSlotsAvailable" + ReasonUsersAclError = "UsersACLError" ) // +kubebuilder:object:root=true diff --git a/api/v1alpha1/zz_generated.deepcopy.go b/api/v1alpha1/zz_generated.deepcopy.go index b239c97..1d392da 100644 --- a/api/v1alpha1/zz_generated.deepcopy.go +++ b/api/v1alpha1/zz_generated.deepcopy.go @@ -26,6 +26,51 @@ import ( runtime "k8s.io/apimachinery/pkg/runtime" ) +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *ChannelsAclSpec) DeepCopyInto(out *ChannelsAclSpec) { + *out = *in + if in.Patterns != nil { + in, out := &in.Patterns, &out.Patterns + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ChannelsAclSpec. +func (in *ChannelsAclSpec) DeepCopy() *ChannelsAclSpec { + if in == nil { + return nil + } + out := new(ChannelsAclSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *CommandsAclSpec) DeepCopyInto(out *CommandsAclSpec) { + *out = *in + if in.Allow != nil { + in, out := &in.Allow, &out.Allow + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.Deny != nil { + in, out := &in.Deny, &out.Deny + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new CommandsAclSpec. +func (in *CommandsAclSpec) DeepCopy() *CommandsAclSpec { + if in == nil { + return nil + } + out := new(CommandsAclSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ExporterSpec) DeepCopyInto(out *ExporterSpec) { *out = *in @@ -42,6 +87,75 @@ func (in *ExporterSpec) DeepCopy() *ExporterSpec { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *KeysAclSpec) DeepCopyInto(out *KeysAclSpec) { + *out = *in + if in.ReadWrite != nil { + in, out := &in.ReadWrite, &out.ReadWrite + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.ReadOnly != nil { + in, out := &in.ReadOnly, &out.ReadOnly + *out = make([]string, len(*in)) + copy(*out, *in) + } + if in.WriteOnly != nil { + in, out := &in.WriteOnly, &out.WriteOnly + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new KeysAclSpec. +func (in *KeysAclSpec) DeepCopy() *KeysAclSpec { + if in == nil { + return nil + } + out := new(KeysAclSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *PasswordSecretSpec) DeepCopyInto(out *PasswordSecretSpec) { + *out = *in + if in.Keys != nil { + in, out := &in.Keys, &out.Keys + *out = make([]string, len(*in)) + copy(*out, *in) + } +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new PasswordSecretSpec. +func (in *PasswordSecretSpec) DeepCopy() *PasswordSecretSpec { + if in == nil { + return nil + } + out := new(PasswordSecretSpec) + in.DeepCopyInto(out) + return out +} + +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *UserAclSpec) DeepCopyInto(out *UserAclSpec) { + *out = *in + in.PasswordSecret.DeepCopyInto(&out.PasswordSecret) + in.Commands.DeepCopyInto(&out.Commands) + in.Keys.DeepCopyInto(&out.Keys) + in.Channels.DeepCopyInto(&out.Channels) +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new UserAclSpec. +func (in *UserAclSpec) DeepCopy() *UserAclSpec { + if in == nil { + return nil + } + out := new(UserAclSpec) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *ValkeyCluster) DeepCopyInto(out *ValkeyCluster) { *out = *in @@ -125,6 +239,13 @@ func (in *ValkeyClusterSpec) DeepCopyInto(out *ValkeyClusterSpec) { (*in).DeepCopyInto(*out) } in.Exporter.DeepCopyInto(&out.Exporter) + if in.Users != nil { + in, out := &in.Users, &out.Users + *out = make([]UserAclSpec, len(*in)) + for i := range *in { + (*in)[i].DeepCopyInto(&(*out)[i]) + } + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new ValkeyClusterSpec. diff --git a/config/crd/bases/valkey.io_valkeyclusters.yaml b/config/crd/bases/valkey.io_valkeyclusters.yaml index f8a188c..1a1c278 100644 --- a/config/crd/bases/valkey.io_valkeyclusters.yaml +++ b/config/crd/bases/valkey.io_valkeyclusters.yaml @@ -1164,6 +1164,97 @@ spec: type: string type: object type: array + users: + description: Users, and ACL-related configuration; see valkeyacls_types.go + items: + description: An UserAclSpec contains user, authorization, and permissions-related + configurations + properties: + channels: + description: Channel restrictions + properties: + patterns: + description: 'Pub/Sub channel patterns - maps to Valkey: + &pattern' + items: + type: string + type: array + type: object + commands: + description: Valkey command categories, commands, and subcommands + restrictions for this user + properties: + allow: + description: Allowed commands for this user + items: + type: string + type: array + deny: + description: Denied commands for this user + items: + type: string + type: array + type: object + enabled: + default: true + description: If the user is enabled or not + type: boolean + keys: + description: Key restrictions + properties: + readOnly: + description: 'Keys restricted to read-only; maps to Valkey: + %R~pattern' + items: + type: string + type: array + readWrite: + description: 'Keys on which this user can read, and write; + maps to Valkey: ~pattern' + items: + type: string + type: array + writeOnly: + description: 'Keys restricted to write-only; maps to Valkey: + %W~pattern' + items: + type: string + type: array + type: object + name: + description: Username + type: string + nopass: + default: false + description: Do not apply a password to this user + type: boolean + passwordSecret: + description: Reference information to a Secret containing user + passwords + properties: + keys: + description: |- + An array of keys inside the referencing Secret to find passwords; defaults to username + Valkey supports multiple passwords per user for rotation + items: + type: string + type: array + name: + description: Name of the referencing Secret; Defaults to + clustername-users + type: string + type: object + permissions: + description: Raw ACL for (additional) permissions. Appended + to anything generated. + type: string + required: + - name + type: object + type: array + x-kubernetes-list-map-keys: + - name + x-kubernetes-list-type: map type: object status: default: diff --git a/config/rbac/role.yaml b/config/rbac/role.yaml index 578d6db..a46c736 100644 --- a/config/rbac/role.yaml +++ b/config/rbac/role.yaml @@ -8,6 +8,7 @@ rules: - "" resources: - configmaps + - secrets - services verbs: - create diff --git a/config/samples/v1alpha1_valkeycluster.yaml b/config/samples/v1alpha1_valkeycluster.yaml index 540a4d1..83db841 100644 --- a/config/samples/v1alpha1_valkeycluster.yaml +++ b/config/samples/v1alpha1_valkeycluster.yaml @@ -1,3 +1,13 @@ +--- +apiVersion: v1 +kind: Secret +metadata: + name: valkeycluster-sample-users +data: + alicepw: M21wdHlQQHNzdzByZA== + davidold: OVYqTHQlYXU4Mk5tdTlyeQ== + davidnew: VmFsa2V5I1J1bHojMjIzMw== +--- apiVersion: valkey.io/v1alpha1 kind: ValkeyCluster metadata: @@ -5,6 +15,42 @@ metadata: spec: shards: 3 replicas: 1 + users: + - name: alice + enabled: true + passwordSecret: + name: valkeycluster-sample-users + keys: [alicepw] + commands: + allow: ["@read", "@write", "@connection"] + deny: ["@admin", "@dangerous"] + keys: + readWrite: ["app:*", "cache:*"] + readOnly: ["shared:*", "config:*"] + writeOnly: ["logs:*", "metrics:*"] + channels: + patterns: ["notifications:*", "events:*"] + permissions: "+client|setname +debug" + - name: bob + nopass: true + enabled: true + permissions: "+@all -@admin ~* &*" + - name: charlie + enabled: false + commands: + allow: ["@read"] + keys: + readOnly: ["*"] + - name: david + enabled: true + passwordSecret: + name: valkeycluster-sample-users + keys: + - davidold + - davidnew + commands: + allow: ["@admin"] + resources: requests: memory: "256Mi" diff --git a/internal/controller/deployment.go b/internal/controller/deployment.go index 4f87a75..7c8bf21 100644 --- a/internal/controller/deployment.go +++ b/internal/controller/deployment.go @@ -105,6 +105,11 @@ func generateContainersDef(cluster *valkeyiov1alpha1.ValkeyCluster) []corev1.Con MountPath: "/config", ReadOnly: true, }, + { + Name: "users-acl", + MountPath: "/config/users", + ReadOnly: true, + }, }, }, } @@ -160,6 +165,14 @@ func createClusterDeployment(cluster *valkeyiov1alpha1.ValkeyCluster) *appsv1.De }, }, }, + { + Name: "users-acl", + VolumeSource: corev1.VolumeSource{ + Secret: &corev1.SecretVolumeSource{ + SecretName: getInternalSecretName(cluster.Name), + }, + }, + }, }, }, }, diff --git a/internal/controller/users.go b/internal/controller/users.go new file mode 100644 index 0000000..0016b1c --- /dev/null +++ b/internal/controller/users.go @@ -0,0 +1,311 @@ +/* +Copyright 2025 Valkey Contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "context" + "crypto/sha256" + "fmt" + "sort" + "strings" + + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + logf "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/reconcile" + valkeyiov1alpha1 "valkey.io/valkey-operator/api/v1alpha1" +) + +const ( + hashAnnotationKey = "valkey.io/internal-acl-hash" +) + +func getInternalSecretName(clusterName string) string { + return "internal-" + clusterName + "-acl" +} + +func getDefaultSecretName(clusterName string) string { + return clusterName + "-users" +} + +// When a Secret is updated, Watch() calls this function to discover +// which object should be reconciled. Because multiple secrets can be +// used by the same cluster, and a single secret used by multiple clusters, +// we grab a list of all Valkey clusters, and iterate through the ACLs +// looking for the modified secret. We return a list of clusters that +// need to be reconciled. +func (r *ValkeyClusterReconciler) findReferencedClusters(ctx context.Context, secret client.Object) []reconcile.Request { + + log := logf.FromContext(ctx) + secretName := secret.GetName() // the Secret that was updated + + log.V(1).Info("findReferencedClusters", "modified", secretName) + + // List all ValkeyClusters + valkeyClusterList := &valkeyiov1alpha1.ValkeyClusterList{} + if err := r.List(ctx, valkeyClusterList, + client.InNamespace(secret.GetNamespace()), + ); err != nil { + log.Error(err, "failed to list valkey clusters") + return []reconcile.Request{} + } + + requests := []reconcile.Request{} + + // Take our list of clusters, and iterate through them, matching against + // spec.Users[].PasswordSecret.Name. Return a list of clusters to be reconciled. + for _, cluster := range valkeyClusterList.Items { + for _, user := range cluster.Spec.Users { + if user.PasswordSecret.Name == secretName { + log.V(1).Info("adding cluster to reconcile", "name", cluster.Name) + requests = append(requests, reconcile.Request{ + NamespacedName: types.NamespacedName{ + Name: cluster.Name, + Namespace: cluster.Namespace, + }, + }) + } + } + } + + return requests +} + +func (r *ValkeyClusterReconciler) reconcileUsersAcl(ctx context.Context, cluster *valkeyiov1alpha1.ValkeyCluster) error { + + log := logf.FromContext(ctx) + + // Sort users for consistency in hash calculations + sort.Slice(cluster.Spec.Users, func(i, j int) bool { + return cluster.Spec.Users[i].Name < cluster.Spec.Users[j].Name + }) + + // Process each user, generating a complete ACL string + var usersAcls strings.Builder + for _, user := range cluster.Spec.Users { + + // Get passwords from Secret + passwords, err := fetchUserPasswords(ctx, user, r.Client, cluster.Name, cluster.Namespace) + if err != nil { + log.Error(err, "failed to fetch password", "username", user.Name) + continue + } + + // Build ACL string for this user with found password(s) + acl := buildUserAcl(user, passwords) + fmt.Fprintf(&usersAcls, "%s\n", acl) + } + usersAclsBytes := []byte(usersAcls.String()) + + // An "internal" secrets object is used for synchronization + internalSecretName := getInternalSecretName(cluster.Name) + needCreateInternal := false + + internalAclSecret := &corev1.Secret{} + if err := r.Get(ctx, types.NamespacedName{ + Name: internalSecretName, + Namespace: cluster.Namespace, + }, internalAclSecret); err != nil { + if !apierrors.IsNotFound(err) { + log.Error(err, "failed to fetch internal acl secret") + return err + } + + // Internal secret was not found. + // Init, and add metadata to the new Secret object + needCreateInternal = true + log.V(2).Info("creating internal secret", "secretName", internalSecretName) + + internalAclSecret.Data = make(map[string][]byte) + internalAclSecret.ObjectMeta = metav1.ObjectMeta{ + Name: internalSecretName, + Namespace: cluster.Namespace, + Labels: labels(cluster), + } + } + + // Register ownership of the internal Secret so that it is GC'd by K8S on CR delete + if err := controllerutil.SetControllerReference(cluster, internalAclSecret, r.Scheme); err != nil { + log.Error(err, "Failed to grab ownership of internal secret") + r.Recorder.Eventf(cluster, nil, corev1.EventTypeWarning, "InternalSecretsCreationFailed", "ReconcileUsers", "Failed to grab ownership of internal secret: %v", err) + return err + } + + // Calculate hash of the ACL file contents + internalAclHash := fmt.Sprintf("%x", sha256.Sum256(usersAclsBytes)) + + // Compare hash to the one already attached to the internal secret, if present. + // If the hashes are different, then we need to update the internal secret with + // the new file contents and update the hash annotation. If the hashes are the + // same, don't update as that would cause infinite reconciliation + + if needsUpdate := upsertAnnotation(internalAclSecret, hashAnnotationKey, internalAclHash); !needsUpdate { + log.V(1).Info("internal ACLs unchanged") + return nil + } + + // Add the acl contents to the internal secret, replacing anything preexisting + internalAclSecret.Data["users.acl"] = usersAclsBytes + + // Create the internal secret, if needed + if needCreateInternal { + if err := r.Create(ctx, internalAclSecret); err != nil { + log.Error(err, "Failed to create internal secret") + r.Recorder.Eventf(cluster, nil, corev1.EventTypeWarning, "InternalSecretsCreationFailed", "ReconcileUsers", "Failed to create internal secret: %v", err) + return err + } else { + r.Recorder.Eventf(cluster, nil, corev1.EventTypeNormal, "InternalSecretsCreated", "ReconcileUsers", "Created internal ACLs") + return nil + } + } + + // Otherwise update it + if err := r.Update(ctx, internalAclSecret); err != nil { + log.Error(err, "Failed to update internal secret") + r.Recorder.Eventf(cluster, nil, corev1.EventTypeWarning, "InternalSecretsUpdateFailed", "ReconcileUsers", "Failed to update internal secret: %v", err) + return err + } + + r.Recorder.Eventf(cluster, nil, corev1.EventTypeNormal, "InternalSecretsUpdated", "ReconcileUsers", "Synchronized internal ACLs") + + // All is good; The internal secret will be auto-mounted in the deployment + return nil +} + +// Builds a user ACL string +func buildUserAcl(user valkeyiov1alpha1.UserAclSpec, passwords []string) string { + + // Holds the ACL as we build it + var acl strings.Builder + + // Helper for repeated actions + appendAcl := func(acl *strings.Builder, permissions []string, prefix string) { + for _, permission := range permissions { + fmt.Fprintf(acl, " %s%s", prefix, permission) + } + } + + // Initial acl + fmt.Fprintf(&acl, "user %s ", user.Name) + + // Is the user enabled? + if user.Enabled { + acl.WriteString("on") + } else { + acl.WriteString("off") + } + + // If enabled, append password(s), which should already be prefix-hashed + if user.NoPassword { + fmt.Fprintf(&acl, " nopass") + } else { + appendAcl(&acl, passwords, "#") + } + + // Add key restrictions + appendAcl(&acl, user.Keys.ReadWrite, "~") + appendAcl(&acl, user.Keys.ReadOnly, "%R~") + appendAcl(&acl, user.Keys.WriteOnly, "%W~") + + // Add channel restrictions + if len(user.Channels.Patterns) > 0 { + acl.WriteString(" resetchannels") + appendAcl(&acl, user.Channels.Patterns, "&") + } + + // Build command ACLs + appendAcl(&acl, user.Commands.Allow, "+") + appendAcl(&acl, user.Commands.Deny, "-") + + // Append remaining/raw permissions + fmt.Fprintf(&acl, " %s", user.RawAcl) + + return acl.String() +} + +// Fetches a Secret, and looks for referenced passwords +func fetchUserPasswords(ctx context.Context, user valkeyiov1alpha1.UserAclSpec, apiClient client.Client, clusterName, clusterNamespace string) ([]string, error) { + + log := logf.FromContext(ctx) + + // If this user doesn't have a password, return empty + if user.NoPassword { + return []string{}, nil + } + + // Look for a Secret matching the user-provided name, or clusterName-users + userSecretName := getDefaultSecretName(clusterName) + if user.PasswordSecret.Name != "" { + userSecretName = user.PasswordSecret.Name + } + + // Query API for the referenced Secret + userSecret := &corev1.Secret{} + if err := apiClient.Get(ctx, types.NamespacedName{ + Name: userSecretName, + Namespace: clusterNamespace, + }, userSecret); err != nil { + if !apierrors.IsNotFound(err) { + log.Error(err, "failed to fetch acl secret") + return []string{}, err + } + log.V(1).Info("Users secret not found", "userSecretName", userSecretName) + + // The Secret was not found; Also, if NoPassword is false, then we cannot add this user + if !user.NoPassword { + return []string{}, fmt.Errorf("no password or reference found") + } + } + + // Sort the password keys; default to username if no keys present + passwordKeys := user.PasswordSecret.Keys + sort.Strings(passwordKeys) + if len(passwordKeys) == 0 { + passwordKeys = []string{user.Name} + } + + // Now that we have a secret, look for any reference keys. If empty, default to username + passwords := []string{} + for _, key := range passwordKeys { + + log.V(2).Info("looking for password", "user", user.Name, "secret", userSecretName, "key", key) + + // byte-string password + password, exists := userSecret.Data[key] + if !exists { + log.Error(nil, "missing password key in secret", "user", user.Name, "secret", userSecretName, "key", key) + return []string{}, fmt.Errorf("missing password key in secret") + } + + // If the Secret begins with # (byte 35) and is 65 total characters long, + // we assume this is a pre-hashed password + if password[0] == 35 && len(password) == 65 { + passwords = append(passwords, string(password[1:])) + continue + } + + // Otherwise, we assume a plaintext password, and we hash it before appending + hashedPassword := fmt.Sprintf("%x", sha256.Sum256(password)) + passwords = append(passwords, hashedPassword) + } + + return passwords, nil +} diff --git a/internal/controller/users_test.go b/internal/controller/users_test.go new file mode 100644 index 0000000..1a3a801 --- /dev/null +++ b/internal/controller/users_test.go @@ -0,0 +1,107 @@ +/* +Copyright 2025 Valkey Contributors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "strings" + "testing" + + // corev1 "k8s.io/api/core/v1" + valkeyiov1alpha1 "valkey.io/valkey-operator/api/v1alpha1" +) + +func TestBuildAclFileContents(t *testing.T) { + + passwords := map[string][]string{ + "alice": {"a71153805265764af6f55b4e0ce38858cde64e6e24b9a9b14e32262760572137"}, + "bob": {}, + "charlie": {}, + "david": { + "7447bd019c69af5975c54072b40f9c24d1105836cbd68408d6df7be76ac42ab1", + "4b31cf3c1347d94fe80efb0c848579c5730d63efef2f5eaf32f78a7ca251833b", + }, + } + + expecteds := map[string]string{ + "alice": "user alice on #a71153805265764af6f55b4e0ce38858cde64e6e24b9a9b14e32262760572137 ~app:* ~cache:* %R~shared:* %R~config:* %W~logs:* %W~metrics:* resetchannels ¬ifications:* &events:* +@read +@write +@connection -@admin -@dangerous +client|setname +debug", + "bob": "user bob on nopass +@all -@admin ~* &*", + "charlie": "user charlie off nopass +@admin", + "david": "user david on #7447bd019c69af5975c54072b40f9c24d1105836cbd68408d6df7be76ac42ab1 #4b31cf3c1347d94fe80efb0c848579c5730d63efef2f5eaf32f78a7ca251833b +@admin", + } + + // UsersAcl + users := []valkeyiov1alpha1.UserAclSpec{ + { + Name: "alice", + Enabled: true, + PasswordSecret: valkeyiov1alpha1.PasswordSecretSpec{ + Name: "alice-secret", + Keys: []string{"alice"}, + }, + NoPassword: false, + Commands: valkeyiov1alpha1.CommandsAclSpec{ + Allow: []string{"@read", "@write", "@connection"}, + Deny: []string{"@admin", "@dangerous"}, + }, + Keys: valkeyiov1alpha1.KeysAclSpec{ + ReadWrite: []string{"app:*", "cache:*"}, + ReadOnly: []string{"shared:*", "config:*"}, + WriteOnly: []string{"logs:*", "metrics:*"}, + }, + Channels: valkeyiov1alpha1.ChannelsAclSpec{ + Patterns: []string{"notifications:*", "events:*"}, + }, + RawAcl: "+client|setname +debug", + }, + { + Name: "bob", + Enabled: true, + NoPassword: true, + RawAcl: "+@all -@admin ~* &*", + }, + { + Name: "charlie", + Enabled: false, + NoPassword: true, + Commands: valkeyiov1alpha1.CommandsAclSpec{ + Allow: []string{"@admin"}, + }, + }, + { + Name: "david", + Enabled: true, + PasswordSecret: valkeyiov1alpha1.PasswordSecretSpec{ + Keys: []string{"davidold", "davidnew"}, + }, + Commands: valkeyiov1alpha1.CommandsAclSpec{ + Allow: []string{"@admin"}, + }, + }, + } + + // iterate over the users + for _, user := range users { + + userName := user.Name + acl := buildUserAcl(user, passwords[userName]) + expected := expecteds[userName] + + if strings.TrimSpace(acl) != expected { + t.Errorf("%s ACL Failed. Expected '%s'; got '%s'", userName, expected, acl) + } + } +} diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 5ef185d..aaca577 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -19,6 +19,7 @@ package controller import ( "maps" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" valkeyv1 "valkey.io/valkey-operator/api/v1alpha1" ) @@ -26,7 +27,8 @@ const appName = "valkey" // Labels returns a copy of user defined labels including recommended: // https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ -func labels(cluster *valkeyv1.ValkeyCluster) map[string]string { +// nolint:unparam +func labels(cluster *valkeyv1.ValkeyCluster, extraLabels ...map[string]string) map[string]string { if cluster.Labels == nil { cluster.Labels = make(map[string]string) } @@ -36,6 +38,12 @@ func labels(cluster *valkeyv1.ValkeyCluster) map[string]string { l["app.kubernetes.io/component"] = "valkey-cluster" l["app.kubernetes.io/part-of"] = appName l["app.kubernetes.io/managed-by"] = "valkey-operator" + + // Copy extra labels into main map, overriding duplicates + for _, e := range extraLabels { + maps.Copy(l, e) + } + return l } @@ -43,3 +51,29 @@ func labels(cluster *valkeyv1.ValkeyCluster) map[string]string { func annotations(cluster *valkeyv1.ValkeyCluster) map[string]string { return maps.Clone(cluster.Annotations) } + +// This function takes a K8S object reference (eg: pod, secret, configmap, etc), +// and a map of annotations to add to, or replace existing, within the object. +// Returns true if the annotation was added, or updated +func upsertAnnotation(o metav1.Object, key string, val string) bool { + + updated := false + + // Get current annotations + annotations := o.GetAnnotations() + if annotations == nil { + annotations = make(map[string]string) + } + + // If not found, insert, or update + if orig := annotations[key]; orig != val { + + updated = true + annotations[key] = val + + // Set annotations + o.SetAnnotations(annotations) + } + + return updated +} diff --git a/internal/controller/valkeycluster_controller.go b/internal/controller/valkeycluster_controller.go index 9ed9338..cfd5876 100644 --- a/internal/controller/valkeycluster_controller.go +++ b/internal/controller/valkeycluster_controller.go @@ -34,6 +34,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/handler" logf "sigs.k8s.io/controller-runtime/pkg/log" valkeyiov1alpha1 "valkey.io/valkey-operator/api/v1alpha1" "valkey.io/valkey-operator/internal/valkey" @@ -65,6 +66,7 @@ var scripts embed.FS // +kubebuilder:rbac:groups=valkey.io,resources=valkeyclusters/finalizers,verbs=update // +kubebuilder:rbac:groups="",resources=services,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups="",resources=configmaps,verbs=get;list;watch;create;update;patch;delete +// +kubebuilder:rbac:groups="",resources=secrets,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups="",resources=pods,verbs=get;list;watch // +kubebuilder:rbac:groups="apps",resources=deployments,verbs=get;list;watch;create;update;patch;delete // +kubebuilder:rbac:groups=events.k8s.io,resources=events,verbs=create;patch @@ -89,6 +91,11 @@ func (r *ValkeyClusterReconciler) Reconcile(ctx context.Context, req ctrl.Reques return ctrl.Result{}, err } + if err := r.reconcileUsersAcl(ctx, cluster); err != nil { + setCondition(cluster, valkeyiov1alpha1.ConditionReady, valkeyiov1alpha1.ReasonUsersAclError, err.Error(), metav1.ConditionFalse) + return ctrl.Result{}, err + } + if err := r.upsertConfigMap(ctx, cluster); err != nil { setCondition(cluster, valkeyiov1alpha1.ConditionReady, valkeyiov1alpha1.ReasonConfigMapError, err.Error(), metav1.ConditionFalse) _ = r.updateStatus(ctx, cluster, nil) @@ -249,7 +256,8 @@ func (r *ValkeyClusterReconciler) upsertConfigMap(ctx context.Context, cluster * "valkey.conf": ` cluster-enabled yes protected-mode no -cluster-node-timeout 2000`, +cluster-node-timeout 2000 +aclfile /config/users/users.acl`, }, } if err := controllerutil.SetControllerReference(cluster, cm, r.Scheme); err != nil { @@ -506,6 +514,10 @@ func (r *ValkeyClusterReconciler) SetupWithManager(mgr ctrl.Manager) error { Owns(&corev1.Service{}). Owns(&corev1.ConfigMap{}). Owns(&appsv1.Deployment{}). + Watches( + &corev1.Secret{}, + handler.EnqueueRequestsFromMapFunc(r.findReferencedClusters), + ). Named("valkeycluster"). Complete(r) } diff --git a/internal/valkey/clusterstate.go b/internal/valkey/clusterstate.go index ed385ad..eae4d45 100644 --- a/internal/valkey/clusterstate.go +++ b/internal/valkey/clusterstate.go @@ -124,7 +124,7 @@ func (s *ClusterState) GetUnassignedSlots() []SlotsRange { remaining := []SlotsRange{{0, 16383}} for _, shard := range s.Shards { for _, slot := range shard.Slots { - var next []SlotsRange + var next []SlotsRange //nolint:prealloc for _, base := range remaining { parts := subtractSlotsRange(base, slot) next = append(next, parts...) diff --git a/test/e2e/e2e_suite_test.go b/test/e2e/e2e_suite_test.go index 1ec3e5a..6388cec 100644 --- a/test/e2e/e2e_suite_test.go +++ b/test/e2e/e2e_suite_test.go @@ -65,9 +65,14 @@ func TestE2E(t *testing.T) { } var _ = BeforeSuite(func() { - By("building the manager image") - cmd := exec.Command("make", "docker-build", fmt.Sprintf("IMG=%s", managerImage)) + By("purging old events") + cmd := exec.Command("kubectl", "delete", "events", "--field-selector", "involvedObject.name=valkeycluster-sample") _, err := utils.Run(cmd) + ExpectWithOffset(1, err).NotTo(HaveOccurred(), "Failed to purge old events") + + By("building the manager image") + cmd = exec.Command("make", "docker-build", fmt.Sprintf("IMG=%s", managerImage)) + _, err = utils.Run(cmd) ExpectWithOffset(1, err).NotTo(HaveOccurred(), "Failed to build the manager image") By("loading the manager image on Kind") diff --git a/test/e2e/valkeycluster_test.go b/test/e2e/valkeycluster_test.go index b21c61e..29cde12 100644 --- a/test/e2e/valkeycluster_test.go +++ b/test/e2e/valkeycluster_test.go @@ -114,6 +114,14 @@ var _ = Describe("ValkeyCluster", Ordered, func() { Expect(err).NotTo(HaveOccurred(), "Failed to retrieve pod's information") Expect(output).To(MatchJSON(`{"limits":{"cpu":"500m","memory":"512Mi"},"requests":{"cpu":"100m","memory":"256Mi"}}`), "Incorrect pod resources configuration") + By("validating internal secret was created") + verifyInternalSecretsExists := func(g Gomega) { + cmd := exec.Command("kubectl", "get", "secrets", "internal-" + valkeyClusterName + "-acl") + _, err := utils.Run(cmd) + g.Expect(err).NotTo(HaveOccurred()) + } + Eventually(verifyInternalSecretsExists).Should(Succeed()) + By("validating the ValkeyCluster CR status") verifyCrStatus := func(g Gomega) { cr, err := utils.GetValkeyClusterStatus(valkeyClusterName) @@ -268,6 +276,40 @@ var _ = Describe("ValkeyCluster", Ordered, func() { g.Expect(output).To(ContainSubstring("cluster_state:ok")) } Eventually(verifyClusterAccess).Should(Succeed()) + + By("verifying created users") + verifyCreatedUsers := func(g Gomega) { + // Start a Valkey client pod to access the cluster and get its status. + clusterFqdn := fmt.Sprintf("%s.default.svc.cluster.local", valkeyClusterName) + + cmd := exec.Command("kubectl", "run", "client", + fmt.Sprintf("--image=%s", valkeyClientImage), "--restart=Never", "--", + "valkey-cli", "-c", "-h", clusterFqdn, "ACL", "LIST") + _, err := utils.Run(cmd) + g.Expect(err).NotTo(HaveOccurred()) + + cmd = exec.Command("kubectl", "wait", "pod/client", + "--for=jsonpath={.status.phase}=Succeeded", "--timeout=30s") + _, err = utils.Run(cmd) + g.Expect(err).NotTo(HaveOccurred()) + + cmd = exec.Command("kubectl", "logs", "client") + output, err := utils.Run(cmd) + g.Expect(err).NotTo(HaveOccurred()) + + cmd = exec.Command("kubectl", "delete", "pod", "client", + "--wait=true", "--timeout=30s") + _, err = utils.Run(cmd) + g.Expect(err).NotTo(HaveOccurred()) + + // There should be 3 defined users + g.Expect(output).To(SatisfyAll( + ContainSubstring("user alice on"), + ContainSubstring("user bob on nopass"), + ContainSubstring("user david on"), + )) + } + Eventually(verifyCreatedUsers).Should(Succeed()) }) }) From 68a0a18c4db575e1f5d91571da3aac36bbd3ddd8 Mon Sep 17 00:00:00 2001 From: utdrmac Date: Wed, 11 Feb 2026 18:15:40 +0000 Subject: [PATCH 2/3] updates from feedback Signed-off-by: utdrmac --- internal/controller/users.go | 31 +++++++++++++++++-------------- internal/controller/utils.go | 9 +-------- 2 files changed, 18 insertions(+), 22 deletions(-) diff --git a/internal/controller/users.go b/internal/controller/users.go index 0016b1c..9f43b01 100644 --- a/internal/controller/users.go +++ b/internal/controller/users.go @@ -190,19 +190,19 @@ func (r *ValkeyClusterReconciler) reconcileUsersAcl(ctx context.Context, cluster return nil } +// Helper for repeated actions +func appendAcl(acl *strings.Builder, permissions []string, prefix string) { + for _, permission := range permissions { + fmt.Fprintf(acl, " %s%s", prefix, permission) + } +} + // Builds a user ACL string func buildUserAcl(user valkeyiov1alpha1.UserAclSpec, passwords []string) string { // Holds the ACL as we build it var acl strings.Builder - // Helper for repeated actions - appendAcl := func(acl *strings.Builder, permissions []string, prefix string) { - for _, permission := range permissions { - fmt.Fprintf(acl, " %s%s", prefix, permission) - } - } - // Initial acl fmt.Fprintf(&acl, "user %s ", user.Name) @@ -269,10 +269,8 @@ func fetchUserPasswords(ctx context.Context, user valkeyiov1alpha1.UserAclSpec, } log.V(1).Info("Users secret not found", "userSecretName", userSecretName) - // The Secret was not found; Also, if NoPassword is false, then we cannot add this user - if !user.NoPassword { - return []string{}, fmt.Errorf("no password or reference found") - } + // The Secret was not found; And since NoPassword is false, then we cannot add this user + return []string{}, fmt.Errorf("no password or reference found") } // Sort the password keys; default to username if no keys present @@ -295,9 +293,8 @@ func fetchUserPasswords(ctx context.Context, user valkeyiov1alpha1.UserAclSpec, return []string{}, fmt.Errorf("missing password key in secret") } - // If the Secret begins with # (byte 35) and is 65 total characters long, - // we assume this is a pre-hashed password - if password[0] == 35 && len(password) == 65 { + // Test if the string in the Secret is a pre-hashed sha256 password + if isPreHashedPassword(password) { passwords = append(passwords, string(password[1:])) continue } @@ -309,3 +306,9 @@ func fetchUserPasswords(ctx context.Context, user valkeyiov1alpha1.UserAclSpec, return passwords, nil } + +// Check if byte-string begins with # (byte 35) and is 65 total characters long. +// If so, we assume this is a pre-hashed sha256 password. +func isPreHashedPassword(password []byte) bool { + return password[0] == 35 && len(password) == 65 +} diff --git a/internal/controller/utils.go b/internal/controller/utils.go index aaca577..86b8d9d 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -27,8 +27,7 @@ const appName = "valkey" // Labels returns a copy of user defined labels including recommended: // https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/ -// nolint:unparam -func labels(cluster *valkeyv1.ValkeyCluster, extraLabels ...map[string]string) map[string]string { +func labels(cluster *valkeyv1.ValkeyCluster) map[string]string { if cluster.Labels == nil { cluster.Labels = make(map[string]string) } @@ -38,12 +37,6 @@ func labels(cluster *valkeyv1.ValkeyCluster, extraLabels ...map[string]string) m l["app.kubernetes.io/component"] = "valkey-cluster" l["app.kubernetes.io/part-of"] = appName l["app.kubernetes.io/managed-by"] = "valkey-operator" - - // Copy extra labels into main map, overriding duplicates - for _, e := range extraLabels { - maps.Copy(l, e) - } - return l } From 09cc75cc2cd1520475a2327b11615fd86b567469 Mon Sep 17 00:00:00 2001 From: utdrmac Date: Thu, 19 Feb 2026 03:35:26 +0000 Subject: [PATCH 3/3] fmt fix Signed-off-by: utdrmac --- internal/controller/utils.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/controller/utils.go b/internal/controller/utils.go index 2ce6564..bcdf758 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -21,8 +21,8 @@ import ( "slices" "strconv" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" valkeyv1 "valkey.io/valkey-operator/api/v1alpha1" "valkey.io/valkey-operator/internal/valkey" )