PGAdmin user passwords are now stored in secrets created by customer … #3895
Conversation
| if strings.Contains(stdout.String(), "User not found") || | ||
| strings.Contains(stdout.String(), "Password must be") { | ||
|
|
||
| log.Info(fmt.Sprintf("Failed to update pgAdmin user %s: %s", |
There was a problem hiding this comment.
⛏️
fmt.Sprintf("Failed to update pgAdmin user %s: %s", intentUser.Username, stdout.String()) could be pulled out and stored in a variable so any future changes only need to be made once.
There was a problem hiding this comment.
⛏️
| log.Info(fmt.Sprintf("Failed to update pgAdmin user %s: %s", | |
| log.Info("Failed to update pgAdmin user", "user", intentUser.Username, "error", stdout.String() |
|
|
||
| //+kubebuilder:rbac:groups="postgres-operator.crunchydata.com",resources="pgadmins",verbs={list} | ||
|
|
||
| // findPGAdminsForSecret returns PGAdmins that have a user or users that have their password |
There was a problem hiding this comment.
❓ Is this comment correct? If I'm understanding correctly, matching returns the PGAdmins that have a user with a password stored in a Secret, but wouldn't return for users without a PasswordRef. Maybe I'm missing something?
There was a problem hiding this comment.
If a PGAdmin has a user whose passwordRef references the secret in question (identified by the object key passed to the function), then that PGAdmin will be added to the matching list.
There was a problem hiding this comment.
I think I see what I was misunderstanding. I read it as "have a user" or "have users that have their password stored in the Secret", but the comment means "returns PGAdmins that have one or more users that have their password stored in the Secret". If that's correct, I'm good with the comment as is.
testing/kuttl/e2e-other/standalone-pgadmin-user-management/07-assert.yaml
Show resolved
Hide resolved
| @@ -132,6 +132,10 @@ type ServerGroup struct { | |||
| } | |||
|
|
|||
| type PGAdminUser struct { | |||
| // A reference to the secret that holds the user's password. | |||
| // +kubebuilder:validation:Required | |||
| PasswordRef string `json:"passwordRef"` | |||
There was a problem hiding this comment.
@cbandy Do we want this to be a corev1.SecretKeySelector like the LDAP bind password? This would allow users to provide the secret name and key
We talked about this a little on a call but didn't land on a firm answer.
There was a problem hiding this comment.
Yes, I like that. The key field might be required OOTB, and I don't mind that. The field name here LGTM.
| type: string | ||
| name: | ||
| description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names | ||
| TODO: Add other useful fields. apiVersion, kind, uid?' |
There was a problem hiding this comment.
🔧 We should remove this TODO. We have tooling in place for this message in other parts of the CRD, I think you'll just need to add another entry in build/crd/pgadmins/todos.yaml.
There was a problem hiding this comment.
Did we not catch this in any actions? 🤔
There was a problem hiding this comment.
I don't see anything in our actions that should have caught it, but probably makes sense for something to be added.
| // Make sure the password isn't nil or empty | ||
| password := userPasswordSecret.Data[user.PasswordRef.Key] | ||
| if password == nil { | ||
| log.Error(nil, `Could not retrieve password from secret. Must use "password" as key.`) |
There was a problem hiding this comment.
Does this comment need to be updated since users can choose their own key?
| if strings.Contains(stdout.String(), "User not found") || | ||
| strings.Contains(stdout.String(), "Password must be") { | ||
|
|
||
| log.Info(fmt.Sprintf("Failed to update pgAdmin user %s: %s", |
There was a problem hiding this comment.
⛏️
| log.Info(fmt.Sprintf("Failed to update pgAdmin user %s: %s", | |
| log.Info("Failed to update pgAdmin user", "user", intentUser.Username, "error", stdout.String() |
…and secret is referenced in the user spec. Updated/added appropriate go and kuttl tests.
dsessler7
force-pushed
the
pgadmin_user_passwords
branch
from
6ca71e1
to
13d374d
Compare
…and secret is referenced in the user spec. Updated/added appropriate go tests.
Checklist:
Type of Changes:
What is the current behavior (link to any open issues here)?
Currently, passwords for PGAdmin users are auto-generated and cannot be changed.
What is the new behavior (if this is a feature change)?
The customer will now set the passwords for PGAdmin users in secrets and reference the secret in the spec.
Other Information: