Skip to content
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

Take ownership of an existing destination secret #545

Merged
merged 3 commits into from
Jan 31, 2024

Conversation

benashz
Copy link
Collaborator

@benashz benashz commented Jan 10, 2024

Adds a new configuration option spec.destination.overwrite that when set to true VSO will replace an existing destination secret that it does not currently own. VSO will then take ownership of the destination secret's life-cycle.

Closes #337

@benashz benashz requested a review from a team as a code owner January 10, 2024 22:39
@benashz benashz added this to the v0.5.0 milestone Jan 10, 2024
@benashz benashz force-pushed the VAULT-19280/add-support-for-destination-clobber branch from 09b7d9e to 8dc7c1c Compare January 10, 2024 23:23
@benashz benashz requested review from tvoran and kschoche January 11, 2024 15:01
Adds a new configuration option spec.destination.overwrite that when set
to true VSO will replace an existing destination secret that it does not
currently own. VSO will then take ownership of the destination secret's
life-cycle.
@benashz benashz force-pushed the VAULT-19280/add-support-for-destination-clobber branch from 8dc7c1c to ca485aa Compare January 12, 2024 19:38
Copy link
Member

@tvoran tvoran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a couple thoughts, but it looks great!

internal/helpers/secrets.go Outdated Show resolved Hide resolved
Comment on lines +429 to +432
wantErr: func(t assert.TestingT, err error, i ...interface{}) bool {
return assert.ErrorContains(t, err,
"not the owner of the destination Secret foo/baz")
},
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the operator check the type of object that owns a Secret? Just thinking it might be good to allow overwriting ownership if the owner is a non-VSO object, to help when migrating to VSO. Or I suppose it could allow multiple owner references, as long as only one is the VSO object being reconciled.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe with this change VSO will only take ownership of the destination secret if it does not have the generic VSO owner labels.

Allowing other Kinds to own the secret is an interesting idea, but I am not sure how that would work in the case where the "other" Kind is deleted - VSO would probably resolve that on the next reconciliation.

@benashz benashz merged commit 83a28ae into main Jan 31, 2024
39 checks passed
@benashz benashz deleted the VAULT-19280/add-support-for-destination-clobber branch January 31, 2024 18:27
kishoregv pushed a commit to kishoregv/vault-secrets-operator that referenced this pull request Feb 12, 2024
Adds a new configuration option spec.destination.overwrite that when set
to true VSO will replace an existing destination secret that it does not
currently own. VSO will then take ownership of the destination secret's
life-cycle.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot overwrite existing secrets
3 participants