Adds support for revocation of VDS secrets on delete. #143
Conversation
| logger.Info("Got deletion timestamp", "obj", o) | ||
| if err := r.handleDeletion(ctx, o); err != nil { | ||
| msg := "Failed to handle deletion" | ||
| r.Recorder.Eventf(o, corev1.EventTypeWarning, consts.ReasonSecretLeaseRevoked, msg+": %s", err) |
There was a problem hiding this comment.
I wonder if it would be better to use a reason that's not in the past tense here, since in this err handling the lease may not have been revoked yet. And it looks like this error could be either from removing the finalizer or revoking the lease.
It may be useful to emit an event on successful revoke too.
There was a problem hiding this comment.
Good point thanks, fixed in 451a326
…hicorp/vault-secrets-operator into VAULT-15424/add_revoke_option_to_vds
| if err := r.revokeLease(ctx, o, ""); err != nil { | ||
| // Do not return error, otherwise we may fail to remove the finalizer. | ||
| // Worst case at this point we will leave a dangling lease instead of a secret which | ||
| // cannot be deleted. | ||
| } |
There was a problem hiding this comment.
The comment is great, but should we log something here? Otherwise the if err := ... seems extraneous (and go-staticcheck is complaining to me about it 🙂).
There was a problem hiding this comment.
Good call, we're actually emitting an Event and logging inside this function in those cases, I'm going to change the function signature a bit so we're not "hiding anything" and adjust the comment accordingly.
| // Check to be sure all leases have been revoked. | ||
| retry.DoWithRetry(t, "waitForAllLeasesToBeRevoked", 30, time.Second, func() (string, error) { | ||
| // ensure that all leases have been revoked. | ||
| resp, err := c.Logical().ListWithContext(ctx, fmt.Sprintf("sys/leases/lookup/%s/creds/readonly", outputs.DBRole)) |
There was a problem hiding this comment.
Remind me how this works? I was thinking it would be more like sys/leases/lookup/<outputs.DBPath>/creds/<output.DBRole>.
There was a problem hiding this comment.
Ah, I grabbed this from https://developer.hashicorp.com/vault/docs/commands/lease#examples
I think you're right on that path, I'll look into it.
I have validated in the controller logs though that the leases are revoked, but this does look off thx!
There was a problem hiding this comment.
Yup that was it, thanks! Did confirm that I can see the # of leases getting smaller through that loop on shutdown!
|
Oh and don't forget a changelog entry 🙂 |
Adds support for revocation of VDS secrets on delete:
Revoketo the VDS secret Spec.Note: Introducing this behaviour requires the following policy added to each VaultAuthMethod which is referenced by VDS secrets that enable revocation:
In the absence of the above policy, revocation will fail on VDS secret deletion.
This should fix #125