Add application credential finalizer management#897
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
5b62de2 to
ddd3396
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/3f8e7ca5c69048048521ae38bacccecb ❌ openstack-k8s-operators-content-provider FAILURE in 19m 23s |
| instance.Status.Conditions.MarkTrue(condition.ServiceConfigReadyCondition, condition.ServiceConfigReadyMessage) | ||
|
|
||
| // Manage AC consumer finalizer, the AC data was already read and rendered to the service config | ||
| if err := keystonev1.ManageACSecretFinalizer(ctx, helper, instance.Namespace, |
There was a problem hiding this comment.
should we call this block conditionally? if instance.Spec.Auth.ApplicationCredentialSecret is not set, we could skip this block (until L1005) entirely.
I'm also wondering if we should detect issues when err != nil and reflect it int the .Status through a dedicated condition.
There was a problem hiding this comment.
@Deydra71 after a better look into this, I think we can save us from setting up a new condition for the finalizer, but we could reuse condition.ServiceConfigReadyCondition.
The finalizer management runs after ServiceConfigReadyCondition is marked to true (L996). If ManageACSecretFinalizer fails, we technically marked the condition as true but the finalizer wasn't applied. This might generate a problem, so I was thinking about moving L996 after L1005, and setting ServiceConfigReady to false if there's a problem at this stage (L1003).
What do you think?
| } | ||
|
|
||
| // Remove consumer finalizer from the AC secret GlanceAPI was consuming. | ||
| if err := keystonev1.RemoveACSecretConsumerFinalizer(ctx, helper, instance.Namespace, |
There was a problem hiding this comment.
The concern here is that you might have a misaligned .Spec.Auth.ApplicationCredentials with .Status.ApplicationCredentialSecret. On a rotation, if the reconciliation stops before being able to assign the LastAppliedCredentialSecret, and you delete the CR (e.g. oc delete glance glance), the finalizer might not be removed from the last created secret.
To be honest this is just theory, I didn't test it, but I'm wondering if we should take a note here for a follow up where we introduce or check ObservedGeneration in the reconciliation.
| // GlanceCachePruner - | ||
| GlanceCachePruner = "/usr/bin/glance-cache-pruner" | ||
| // ACConsumerFinalizer is added to AC secrets that GlanceAPI is actively consuming | ||
| ACConsumerFinalizer = "openstack.org/glanceapi-ac-consumer" |
There was a problem hiding this comment.
This is weird to me, and I have to check it more.
In the past I used to add/remove/update finalizers using controllerutil (see [1] as an example).
That way you can simply pass to the function exposed by keystone an helper and instance.APIName(), and you don't need to define a constant because the group is inferred by controllerutil.
However I didn't see the keystone function exposed here, so I might be totally wrong.
I'll check that part.
There was a problem hiding this comment.
@Deydra71 also using a constant in the form glanceapi-ac-consumer might be problematic for two reasons:
- we use to have / pattern, and we're breaking it
- assuming you have multiple glance APIs (which is pretty common in DCN, DZ and other custom topologies), you can technically use the same appCred for all of them by referencing the same secret. This can create a problem because you lose track of which api is using that secret from a finalizer perspective, and if I remove one of these APIs, I remove this constant finalizer from the secret for all of them, defeating the purpose of this patch.
Scenario#2 seems a blocker to me, and this is why we should use instance.APIName() as a finalizer for a given secret.
| // GlanceCachePruner - | ||
| GlanceCachePruner = "/usr/bin/glance-cache-pruner" | ||
| // ACConsumerFinalizer is added to AC secrets that GlanceAPI is actively consuming | ||
| ACConsumerFinalizer = "openstack.org/glanceapi-ac-consumer" |
There was a problem hiding this comment.
@Deydra71 also using a constant in the form glanceapi-ac-consumer might be problematic for two reasons:
- we use to have / pattern, and we're breaking it
- assuming you have multiple glance APIs (which is pretty common in DCN, DZ and other custom topologies), you can technically use the same appCred for all of them by referencing the same secret. This can create a problem because you lose track of which api is using that secret from a finalizer perspective, and if I remove one of these APIs, I remove this constant finalizer from the secret for all of them, defeating the purpose of this patch.
Scenario#2 seems a blocker to me, and this is why we should use instance.APIName() as a finalizer for a given secret.
Deydra71
force-pushed
the
appcred-finalizer
branch
from
ddd3396 to
1717993
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: Deydra71 The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @fmount ! Thank you the revieww. I didn't initially realize that Glance creates multiple GlanceAPI instances per CR, so the original static As for the guard finalizer block - so we skip the call when there's no AC secret. Reordered ServiceConfigReadyCondition - moved Removed finalizer from both spec and status secrets on delete - I will also incorporate last three mentioned fixes to barbican, manila, cinder and swift operators since it's affecting them the same way. |
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/9b0f066666f14a84b25869004f53cd53 ❌ openstack-k8s-operators-content-provider FAILURE in 15m 53s |
Deydra71
force-pushed
the
appcred-finalizer
branch
from
1717993 to
f70943e
Compare
Signed-off-by: Veronika Fisarova <vfisarov@redhat.com>
Deydra71
force-pushed
the
appcred-finalizer
branch
from
f70943e to
ca38c47
Compare
|
Build failed (check pipeline). Post https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/6f0aed477bf2409dabb1d23ee21929ca ❌ openstack-k8s-operators-content-provider FAILURE in 14m 15s |
|
PR needs rebase. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
2 participants
Jira: OSPRH-27509
Application Credential dev-doc: https://github.com/openstack-k8s-operators/dev-docs/blob/main/application_credentials.md
Status.ApplicationCredentialSecretopenstack.org/glance-ac-consumerfinalizer to the AC secret after service config is renderedThis ensures that the keystone-operator cannot revoke a rotated AC secret while Glance is still consuming it.
Depends-On: openstack-k8s-operators/keystone-operator#685
Assisted-by: Claude Opus 4.6 noreply@anthropic.com