chore: cleanup legacy secret to use pattern more similar to refs#5856
Conversation
e75fd00 to
ba48c47
Compare
ba48c47 to
97433ee
Compare
|
/assign @cheftako |
| func (r *Legacy) ReadSecretValue(ctx context.Context, fieldPath string, namespace string, reader client.Reader) (*string, error) { | ||
| value := r.Value | ||
| valueFrom := r.ValueFrom | ||
| if value != nil && valueFrom != nil { |
There was a problem hiding this comment.
This is not thread safe with NormalizeSecret (if we care)
There was a problem hiding this comment.
NormalizeSecret is not thread-safe because it mutates in-place. We normally do that on a mutable copy of the object that we have mapped from unstructured. I wish go had const :-)
| if err != nil { | ||
| return nil, err | ||
| } | ||
| value := string(secretValue) |
There was a problem hiding this comment.
Is there a reason we leaving the Legacy with valueFrom set and value not? Seems like this may cause us to repeatedly look up the secret value.
There was a problem hiding this comment.
So the refs pattern would probably use NormalizeSecret and not have ReadSecretValue. Even NormalizeSecret normally operates on a copy, so we are going to look up the secret repeatedly (i.e. once per Reconcile).
WDYT about marking ReadSecretValue as deprecated? There's some risk that we write back to the object if we call NormalizeSecret, but if we were to do that accidentally we should catch it in our tests, and we don't do it on any other references so ....
There was a problem hiding this comment.
+1. I think we should probably have some sort of checks in mocks and controllers which errors out if we ever attempt to update any KRM secret.value field. Also we should probably try to avoid ever adding a new secret.value field. "it is much more convenient for users to be able to specify the secret inline." Its nice for demos. I think however its enough of an anti-pattern that we should look to avoid it going forward.
There was a problem hiding this comment.
once per Reconcile
I'm fine with once per reconcile. That should mean once per 10 minutes (normally). I was more worried that over time that once becomes twice, thrice, etc
|
|
||
| // Resolve sensitive field 'spec.initialUser.password' when it is set. | ||
| if err := direct.ResolveSensitiveField(ctx, obj.Spec.InitialUser.Password, "spec.initialUser.password", obj.Namespace, a.reader); err != nil { | ||
| if err := obj.Spec.InitialUser.Password.NormalizeSecret(ctx, "spec.initialUser.password", obj.Namespace, a.reader); err != nil { |
There was a problem hiding this comment.
I realize its to late to remove 'initialUser.password.value' but do we have any checks to prevent this mistake from being repeated on other resources? Also should we update the description to indicate that users should use 'initialUser.password.valueFrom' instead?
There was a problem hiding this comment.
We can add a check in our API checks (go test -v ./tests/apichecks/...) and we can deprecate the whole type LegacyValueFromSecretKeyRef (and friends).
But it is much more convenient for users to be able to specify the secret inline. But maybe we should make the less secure things not super-easy :-)
| /* Reference to a value with the given key in the given Secret in the resource's namespace. */ | ||
| // +optional | ||
| SecretKeyRef *deprecatedrefs.SecretKeyRef `json:"secretKeyRef,omitempty"` | ||
| SecretKeyRef *LegacyValueFromSecretKeyRef `json:"secretKeyRef,omitempty"` |
There was a problem hiding this comment.
This was the big thing, I wanted to stop using the generated client as that's just too circular.
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheftako The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
dd8d8e2
into
GoogleCloudPlatform:master
Uh oh!
There was an error while loading. Please reload this page.