-
Notifications
You must be signed in to change notification settings - Fork 204
K8SPXC-1733 Ability to customize secrets generation #2227
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
Conversation
deploy/cw-bundle.yaml
Outdated
| - name: DISABLE_TELEMETRY | ||
| value: "false" | ||
| image: perconalab/percona-xtradb-cluster-operator:main | ||
| image: perconalab/percona-xtradb-cluster-operator:feat-add-generated-secrets-options |
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.
The changes to the image should be reverted to main across all the similar occurrences.
pkg/controller/pxc/secrets_test.go
Outdated
| }) | ||
| for password := range userSecret.Data { | ||
| It("Should generate user secrets without symbols", func() { | ||
| Expect(strings.ContainsAny(string(password), "!#$%&()*+,-.<=>?@[]^_{}~")).To(BeFalse()) |
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.
string(...) is not needed
pkg/apis/pxc/v1/pxc_types.go
Outdated
| CRVersion string `json:"crVersion,omitempty"` | ||
| Pause bool `json:"pause,omitempty"` | ||
| SecretsName string `json:"secretsName,omitempty"` | ||
| GeneratedSecretsOptions *GeneratedSecretsOptions `json:"generatedSecretsOptions,omitempty"` |
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.
Given that the configuration we introduce is related to the passwords that are provisioned through secrets and not for the secrets themselves, maybe we could adjust the naming of the new options accordingly. Some options:
- PasswordGenerationOptions - Most accurate and clear
- GeneratedPasswordOptions - Also clear and descriptive
- PasswordOptions - Simpler but still clear
PasswordGenerationOptions is the most suitable one IMO.
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 think PasswordGenerationOptions is good
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.
cc @hors
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 think it would be better to reuse the naming from the “Password Validation Options” feature in MySQL. What do you think about:
spec:
secretGenerationOptions:
lengthMin: 16
lengthMax: 32
allowedSymbols: "!@#$%" # Overrides or supplements the default set
or
spec:
passwordGenerationOptions:
lengthMin: 16
lengthMax: 32
allowedSymbols: "!@#$%" # Overrides or supplements the default set
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.
spec:
passwordGenerationOptions:
lengthMin: 16
lengthMax: 32
allowedSymbols: "!@#$%" # Overrides or supplements the default set
I would go with this ☝🏽
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.
option name change has been addressed
| }) | ||
| } | ||
| }) | ||
| }) |
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 believe in this test should we should focus on the following 2 scenarios:
- Default configuration of passwords with nil the new option
- The new option configured and asserting the specified symbols and lengths
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 made changes to the tests with dedicated context for both default behavior and custom password options. Were you thinking on a more detailed checks for the default behavior or is the current one sufficient?
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.
But we are not actually testing that the existing nil configuration option actually does what we want.
We merely do the following in Create cluster with default secret generation behavior
It("Should generate user secrets", func() {
userSecrets := &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Name: cr.Name + "-secrets",
Namespace: cr.Namespace,
},
}
Expect(k8sClient.Get(ctx, client.ObjectKeyFromObject(userSecrets), userSecrets)).To(Succeed())
})
Which is only fetching the secret without verifying that the default options are respected.
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 added a similar test for the default behavior
|
the unit test failures for proxysql password are unrelated to this pr and will be fixed under this pr: #2124 |
2da0e2d to
a5ad053
Compare
pkg/apis/pxc/v1/pxc_types.go
Outdated
|
|
||
| // +kubebuilder:validation:XValidation:rule="self.maxLength > self.minLength" | ||
| type PasswordGenerationOptions struct { | ||
| // When set to true (the default), special symbols such as *!$%^ will be included in password generation |
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.
this comment indicates the field is a boolean but actually is a string now, it needs to be updated
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.
comment fixed, I also added missing ones for max and min length
| // +kubebuilder:validation:Maximum=128 | ||
| // +kubebuilder:validation:Minimum=8 | ||
| // +kubebuilder:default=20 | ||
| MaxLength int `json:"maxLength"` |
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 think max of MaxLength should be 32.
If you are using MySQL Replication, be aware that a password used by a replica as part of CHANGE REPLICATION SOURCE TO is effectively limited to 32 characters in length; if the password is longer, any excess characters are truncated.
https://dev.mysql.com/doc/refman/8.4/en/assigning-passwords.html
Only replication user is used for that but I think we can limit all users to max 32 to be safe in general.
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.
@fydrah, also, please add new options to deploy/cr.yaml as a commented by default
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.
good catch for the maxlength! it has been updated. cr updated too with default values in comment
a5ad053 to
baacf62
Compare
Customize secrets generation: * keep current secret generation behavior as default * Ability to customize the special symbols to include in secret generation * Ability to customize min/max secret length Linked to percona#2222
baacf62 to
f65782e
Compare
commit: c0f6501 |
|
@fydrah thank you for your contribution! |
Customize secrets generation:
Linked to #2222
CHANGE DESCRIPTION
Problem:
Short explanation of the problem.
By default, the operator generates his secrets for system users (root, xtrabackup, proxyadmin, replication,...etc...).
The current secrets generation is based on constants, prohibiting any customization:
Cause:
Short explanation of the root cause of the issue if applicable.
Solution:
Short explanation of the solution we are providing with this PR.
Proposal would be to have a minimal customization, e.g.:
Suggested implementation would be a dedicated configuration structure for generated secrets:
CHECKLIST
Jira
Needs Doc) and QA (Needs QA)?Tests
compare/*-oc.yml)?Config/Logging/Testability