Keystone: User CreateOpts for password field#709
Conversation
dlaw4608
changed the title
…password as a secret, also included passwordExpiresAt field to user.status.resource Signed-off-by: Daniel Lawton <dlawton@redhat.com>
There was a problem hiding this comment.
Hi @dlaw4608, thanks for adding this, a very important field for the user controller. I would like to highlight a few things:
- I believe we need to change the logic to reconcile the password. Testing locally on my environment, I was not able to change the user password by simply changing the value of the
passwordkey in the secret. We need to identify a change in the secret upfront and then perform a PATCH operation[1] to properly update the password. We may want to add a new reconciler to the resource. Does it make sense? - From a user perspective, I believe it would be good to give the user the ability to pass the password encoded with base64. Testing locally, I chose to use the
datakey instead ofstringData, and the password wasn't changed as expected. Not sure if this is a real requirement, but I would like to highlight this.
[1] https://docs.openstack.org/api-ref/identity/v3/index.html#update-user
edit: I just realized you intended to add only the Create option for passwords. If so, you can ignore the first bullet point at this moment.
| name: user-sample | ||
| spec: | ||
| cloudCredentialsRef: | ||
| cloudName: devstack-admin |
There was a problem hiding this comment.
Maybe a change during development. Let's stick with openstack-admin for consistency. Wdyt?
| name: user-sample | ||
| spec: | ||
| cloudCredentialsRef: | ||
| cloudName: devstack-admin |
| spec: | ||
| cloudCredentialsRef: | ||
| cloudName: openstack-admin | ||
| cloudName: devstack-admin |
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: user-sample | ||
| type: Opaque | ||
| stringData: | ||
| password: "TestPassword" |
There was a problem hiding this comment.
| apiVersion: v1 | |
| kind: Secret | |
| metadata: | |
| name: user-sample | |
| type: Opaque | |
| stringData: | |
| password: "TestPassword" | |
| apiVersion: v1 | |
| kind: Secret | |
| metadata: | |
| name: user-sample | |
| type: Opaque | |
| stringData: | |
| password: "TestPassword" |
A few blank spaces at the beginning. Let's remove them for better indentation.
|
|
||
| // password is the password set for the user | ||
| // +optional | ||
| Password *PasswordSpec `json:"password,omitempty"` |
There was a problem hiding this comment.
Why don't we keep it simple and pass the SecretRef directly here? Any advantage of using a struct?
There was a problem hiding this comment.
+1 on this, not sure there is a point in warping it in an additional struct
| apiVersion: v1 | ||
| kind: Secret | ||
| metadata: | ||
| name: user-create-full | ||
| type: Opaque | ||
| stringData: | ||
| password: "TestPassword" No newline at end of file |
There was a problem hiding this comment.
Probably you also need to add a new user-dependency-no-password-secret or something like that at 00-create-resources-missing-deps.yaml to properly test the dependency with this secret, and complete the deletion later as well.
|
|
||
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
ooc: why do we have so many blank lines here? 🧐
eshulman2
left a comment
eshulman2
left a comment
There was a problem hiding this comment.
very quick and shallow review (need to come back to this one)
|
|
||
| handleNameUpdate(&updateOpts, obj, osResource) | ||
| handleDescriptionUpdate(&updateOpts, resource, osResource) | ||
| handleEnabledUpdate(&updateOpts, resource, osResource) |
There was a problem hiding this comment.
Missing password update logic, I would suggest adding it here to allow password changing and address what @winiciusallan was mentioning about password not changing when changing the secret
|
|
||
| // password is the password set for the user | ||
| // +optional | ||
| Password *PasswordSpec `json:"password,omitempty"` |
There was a problem hiding this comment.
+1 on this, not sure there is a point in warping it in an additional struct
3 participants
Fixes: #703
Follow up for User Controller PR that was recently merged #657,
Current Progress:
password fieldworks fine (E2E tests passing, password when created will be stored as aSecretRef.