-
Notifications
You must be signed in to change notification settings - Fork 23
(feat): Support user creation on cluster init #56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
c323af4
cb2ef36
7f47abd
3b1dbf1
119b29a
5f8daae
b17b26b
2f4d593
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -73,6 +73,46 @@ type ValkeyClusterSpec struct { | |
| // +kubebuilder:default:={enabled:true} | ||
| // +optional | ||
| Exporter ExporterSpec `json:"exporter,omitempty"` | ||
|
|
||
| // Options, or config which are specific to Valkey server | ||
| // +optional | ||
| ValkeySpec ValkeySpec `json:"valkeyConfig,omitempty"` | ||
| } | ||
|
|
||
| // ValkeySpec defines any options, or configuration that is specific to valkey-server | ||
| type ValkeySpec struct { | ||
|
|
||
| // Auth-related structure for handling users, and acls | ||
| Auth AuthSpec `json:"auth,omitempty"` | ||
| } | ||
|
|
||
| // AuthSpec contains authorization, user, and ACL-related configurations | ||
| type AuthSpec struct { | ||
|
|
||
| // Users ACL Secret | ||
| // A reference name to a Secret containing raw user:permission entries, which will be processed first | ||
| // Defaults to clusterName-secret | ||
| // +optional | ||
| UsersSecretRef string `json:"usersSecretRef,omitempty"` | ||
|
|
||
| // Array of users with raw ACL, or reference to a key in the UsersSecretRef | ||
| Users map[string]UserAcl `json:"users,omitempty"` | ||
| } | ||
|
|
||
| type UserAcl struct { | ||
|
|
||
| // Raw ACL line, including password | ||
| Permissions string `json:"permissions,omitempty"` | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking out loud: I wonder if the operator should help provide an abstraction away from the syntax for permissions.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could, but I didn't want the operator to have to be responsible for validating ACL commands, and syntax. That seemed an overly complicated job for the operator. |
||
|
|
||
| // sha256 password | ||
| // kubebuilder:validation:MinLength=64 | ||
| // kubebuilder:validation:MaxLength=65 | ||
| // kubebuilder:XValidation:message="Password should be a sha256 hash" | ||
| Password string `json:"password,omitempty"` | ||
|
|
||
| // Reference to a key in UsersSecretRef containing the password for this user | ||
| // Defaults to the username | ||
| PasswordKeyRef string `json:"passwordKeyRef,omitempty"` | ||
|
Comment on lines
+111
to
+115
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ACLs can support multiple passwords for a user. Such an example case would be if the password needs to be rolled, you can have both passwords active while the client switches between them. Not critical for this PR, but so long as we're open to breaking changes while we're in alpha.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, that's an advanced operation outside the scope of the operator. If the community consensus is to have such functionality, there's no argument from me, and I'll implement it. Or we can keep things simple for now, and add rotation later on. |
||
| } | ||
|
|
||
| type ExporterSpec struct { | ||
|
|
@@ -154,6 +194,7 @@ const ( | |
| ReasonSlotsUnassigned = "SlotsUnassigned" | ||
| ReasonPrimaryLost = "PrimaryLost" | ||
| ReasonNoSlots = "NoSlotsAvailable" | ||
| ReasonUsersAclError = "UsersACLError" | ||
| ) | ||
|
|
||
| // +kubebuilder:object:root=true | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,7 @@ rules: | |
| - "" | ||
| resources: | ||
| - configmaps | ||
| - secrets | ||
| - services | ||
| verbs: | ||
| - create | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,10 +1,33 @@ | ||
| --- | ||
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: valkeycluster-sample-users | ||
| data: | ||
| alicepw: M21wdHlQQHNzdzByZA== | ||
| bob: YjFjMzY4OTZkMzk5MjMwNDU5NTVjMzczZjM2NWVkNjg2Y2I5N2RiNTA1NGE0NTJjN2EzZmQ5MjAwZTMzNGYxZAo= | ||
| --- | ||
| apiVersion: valkey.io/v1alpha1 | ||
| kind: ValkeyCluster | ||
| metadata: | ||
| name: valkeycluster-sample | ||
| spec: | ||
| shards: 3 | ||
| replicas: 1 | ||
| valkeyConfig: | ||
| auth: | ||
| usersSecretRef: valkeycluster-sample-users | ||
| users: | ||
| alice: | ||
| permissions: +@list +@connection ~jobs:* | ||
| passwordKeyRef: alicepw | ||
| bob: | ||
| permissions: ~* +@all | ||
| charlie: | ||
| permissions: -@all +get ~secretkey ~valkey:* | ||
| password: sup3rS3cr3t | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wouldn't this fail the validation that you have in place?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, so much for e2e tests... Shouldn't this have been caught by the K8S api when the e2e test attempted to apply an invalid CR? (Will remove this anyways) |
||
| david: | ||
| permissions: -@all +@admin | ||
| resources: | ||
| requests: | ||
| memory: "256Mi" | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest we use
listType=mapandlistMapKey=namehere to follow API design guidelines.https://kubernetes.io/docs/reference/using-api/server-side-apply/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.