Add application credential finalizer management#356
Conversation
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Deydra71
force-pushed
the
appcred-finalizer
branch
from
4d15dc1 to
9047d5b
Compare
| secret := &corev1.Secret{} | ||
| key := types.NamespacedName{Name: newSecretName, Namespace: instance.Namespace} | ||
| if err := h.GetClient().Get(ctx, key, secret); err != nil { | ||
| return fmt.Errorf("failed to get new AC secret %s: %w", newSecretName, err) |
There was a problem hiding this comment.
This wraps every error the same way, including NotFound. In practice, if the openstack-operator points Spec.Auth.ApplicationCredentialSecret to a new secret before the keystone-operator has actually created it, you'll get a stream of errors in the logs for something that's perfectly normal and resolves on its own.
What about treating NotFound as a softer case?
if k8s_errors.IsNotFound(err) {
Log.Info("AC secret not yet available, will retry", "secret", newSecretName)
return nil
}
return fmt.Errorf("failed to get new AC secret %s: %w", newSecretName, err)That way the logs stay clean during the normal window where keystone hasn't caught up yet.
There was a problem hiding this comment.
I'm trying to think about a scenario when this situation would actually happen...
In keystone-operator - k8s Secret is created first, then Status.SecretName is set, then the AC CR is marked Ready all in the same reconcile + deferred status.
openstack-operator reads the secretn ame from Status.SecretName only after AC is marked as Ready, then it is written to the service CR https://github.com/openstack-k8s-operators/openstack-operator/blob/main/internal/openstack/barbican.go#L107 from where the service reads it. openstack-operator never propagates a secret name that doesn't already exist, because it gates on acCR.IsReady()
| g.Expect(b.Status.ApplicationCredentialSecret).To(Equal(newACSecretName)) | ||
| }, timeout, interval).Should(Succeed()) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
Nice set of tests here - covers the add, the status tracking, and the rotation move. One thing I'm missing though: what happens when the Barbican CR itself gets deleted? The reconcileDelete logic removes the consumer finalizer from the AC secret, but there's no test exercising that path.
Something along the lines of:
It("should remove the consumer finalizer from AC secret on Barbican CR deletion", func() {
// Wait for the consumer finalizer to appear on the AC secret
// ...
// Delete the Barbican CR
// ...
// Verify the AC secret no longer carries the barbican-ac-consumer finalizer
// ...
})Would be good to have that covered so regressions don't sneak in.
mauricioharley
left a comment
mauricioharley
left a comment
There was a problem hiding this comment.
Please check the inline comments.
Deydra71
force-pushed
the
appcred-finalizer
branch
2 times, most recently
from
1c93fae to
0f410be
Compare
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
Deydra71
force-pushed
the
appcred-finalizer
branch
from
0f410be to
f8e88fd
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/1bbbb520d40b4187b469ebbbbf9135a7 ❌ openstack-k8s-operators-content-provider FAILURE in 17m 48s |
Deydra71
force-pushed
the
appcred-finalizer
branch
from
f8e88fd to
1a83ee4
Compare
Deydra71
force-pushed
the
appcred-finalizer
branch
from
1a83ee4 to
802f926
Compare
Deydra71
force-pushed
the
appcred-finalizer
branch
from
802f926 to
af69e45
Compare
Deydra71
force-pushed
the
appcred-finalizer
branch
from
af69e45 to
777ec81
Compare
|
Build failed (check pipeline). Post ❌ openstack-k8s-operators-content-provider FAILURE in 3m 54s |
|
recheck |
vakwetu
left a comment
vakwetu
left a comment
There was a problem hiding this comment.
looks good in general. Just a couple points that claude picked up on.
Deydra71
force-pushed
the
appcred-finalizer
branch
from
777ec81 to
45b421b
Compare
|
/retest |
|
Seems in line with all other operators that we just merged. @Deydra71 I assume we can land this one, it looks good, but let me know if we want to wait the green light from other reviewers. |
|
/retest |
Signed-off-by: Veronika Fisarova <vfisarov@redhat.com>
Deydra71
force-pushed
the
appcred-finalizer
branch
from
45b421b to
bd43c90
Compare
|
Had to rebase |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Deydra71, fmount, vakwetu 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 |
mauricioharley
dismissed
their stale review
Author asked me to dismiss the review.
openshift-merge-bot
Bot
merged commit 199a63d
into
openstack-k8s-operators:main
4 participants
Jira: OSPRH-27509
Application Credential dev-doc: https://github.com/openstack-k8s-operators/dev-docs/blob/main/application_credentials.md
Status.ApplicationCredentialSecretopenstack.org/barbican-ac-consumerfinalizer to the AC secret after service config is renderedThis ensures that the keystone-operator cannot revoke a rotated AC secret while Barbican is still consuming it.
Depends-On: openstack-k8s-operators/keystone-operator#685
Assisted-by: Claude Opus 4.6 noreply@anthropic.com