Improve Challenge finalizer handling#8470
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 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 |
|
/test pull-cert-manager-master-e2e-v1-35-upgrade |
|
/retest |
c0e2207 to
d38f004
Compare
|
/test pull-cert-manager-master-e2e-v1-35-upgrade |
063650c to
531222d
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves challenge finalizer handling in the ACME challenges controller by separating finalizer updates from status updates, and unconditionally using server-side apply (SSA) for finalizer modifications regardless of the ServerSideApply feature gate setting.
Changes:
- Separates finalizer updates from status updates in the challenge sync logic
- Uses SSA unconditionally for setting/unsetting finalizers with CSA-to-SSA migration support
- Ensures finalizers are only removed after successful challenge cleanup, preventing premature removal
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/controller/test/context_builder.go | Adds default "test-manager" field manager to test context builder |
| pkg/controller/certificatesigningrequests/acme/acme_test.go | Updates test assertions to use CreateActionWithOptions and include FieldManager |
| pkg/controller/certificaterequests/acme/acme_test.go | Updates test assertions to use CreateActionWithOptions and include FieldManager |
| pkg/controller/acmechallenges/update_test.go | Refactors tests to separate applyFinalizers and updateStatus test cases |
| pkg/controller/acmechallenges/update.go | Implements new applyFinalizers method using SSA with CSA migration support |
| pkg/controller/acmechallenges/sync_test.go | Updates test expectations to reflect new finalizer handling behavior |
| pkg/controller/acmechallenges/sync.go | Refactors sync logic to call finalize and applyFinalizers separately |
| pkg/controller/acmechallenges/finalizer_test.go | Updates test expectations for applyFinalizersRequired function |
| pkg/controller/acmechallenges/finalizer.go | Refactors finalizer logic to check if finalizers need to be applied |
| pkg/controller/acmechallenges/controller.go | Updates scheduler to use updateStatus instead of updateObject |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bf7925e to
55dd47d
Compare
| } | ||
| // After adding the finalizer, return and let the controller reconcile again | ||
| // so that subsequent processing happens with the finalizer already in place. | ||
| return nil |
There was a problem hiding this comment.
Any idea why we are returning here? Why not continue with the reconciliation?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
566fbf1 to
08da47e
Compare
08da47e to
1b876bf
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if _, err = c.applyFinalizers(ctx, ch, nil); err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
In the challengeFinished path, updateStatus() may set the named return err to an aggregate, but the function later does return nil, which overwrites err and drops any status update error. Consider returning err at the end of this branch (or explicitly handling/propagating the updateStatus error) so failures to persist status still cause a retry.
| return nil | |
| return err |
| func (o *defaultObjectUpdater) applyFinalizers(ctx context.Context, challenge *cmacme.Challenge, finalizers []string) (*cmacme.Challenge, error) { | ||
| if err := o.upgradeManagedFields(ctx, challenge); err != nil { | ||
| return nil, err | ||
| return nil, ignoreNotFound(err) | ||
| } | ||
|
|
||
| ac := cmacmeac.Challenge(challenge.Name, challenge.Namespace). | ||
| // Set UID to ensure we never create a new challenge. | ||
| // Apply semantics are always create-or-update, | ||
| // and the challenge might have been deleted. | ||
| WithUID(challenge.UID). | ||
| // FIXME: This will claim ownership of all finalizers, which is obviously wrong. | ||
| WithFinalizers(challenge.Finalizers...) | ||
| return o.cl.AcmeV1().Challenges(challenge.Namespace).Apply( | ||
| WithFinalizers(finalizers...) | ||
| obj, err := o.cl.AcmeV1().Challenges(challenge.Namespace).Apply( | ||
| ctx, ac, | ||
| metav1.ApplyOptions{Force: true, FieldManager: o.fieldManager}, | ||
| ) | ||
| return obj, ignoreNotFound(err) |
There was a problem hiding this comment.
applyFinalizers uses server-side apply with Force: true on metadata.finalizers, which is an atomic list. Applying a subset (or omitting it to clear) effectively takes ownership of the entire field and can unintentionally drop other controllers’/users’ finalizers. Consider switching finalizer management to a non-SSA approach that can surgically add/remove only cert-manager’s finalizer while preserving others (e.g., read-modify-write Update with conflict retry), or otherwise ensure foreign finalizers are retained.
There was a problem hiding this comment.
I am pretty sure this comment is incorrect. Finalizers is NOT an atomic list, but has set semantics with ownership tracked by finalizer.
|
/test pull-cert-manager-master-e2e-v1-35 |
Signed-off-by: Erik Godding Boye <egboye@gmail.com>
1b876bf to
f196ce7
Compare
|
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. |
|
The main goal of this PR has been achieved by #8519. /close |
|
@erikgb: Closed this PR. DetailsIn response to this:
Instructions 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. |
Pull Request Motivation
I started looking into
updateObjectfor challenges when facing issues working on eventually graduating ourServerSideApplyfeature gate in #7908. Because of serious limitations in the fake client, in particular, kubernetes/kubernetes#136672, I wanted to refactor code in this area.In this PR, I try to separate the challenge status subresource updates from
finalizersmodifications on the challenge main resource. This reduces the pain when migrating to SSA in tests using the fake ("unreal") client.To set/unset
finalizers, I propose using server-side apply unconditionally, so we ignore theServerSideApplyfeature gate here. This was done because I was unable to construct an API supporting both CSA and SSA well. The alternative would be to bring the feature gate reasoning into the functional code, but I'm personally not a huge fan of this.Kind
/kind bug
Release Note