Only remove the cleanup finalizer if the cleanup succeeds by inteon · Pull Request #7286 · cert-manager/cert-manager

inteon · 2024-09-18T18:48:25Z

This addresses two problems:

The finalizer was being removed unconditionally, regardless of whether solver.Cleanup succeeds
The finalizer was assumed to be the first in the list of finalizers, potentially resulting in the removal of foreign finalizers and resulting in the cleanup finalizer never being removed.
solver.Cleanup was invoked inconsistently from two different places, in handleFinalizer and in an acme.IsFinalState block, but in the former, the Status.Processing and Status.Presented fields were not being reset and in the latter case, the finalizer wasn't being removed if the cleanup succeeded.

Part of:

Challenge Records Not Always Cleaned Up #3640

supersedes #5126

Kind

/kind cleanup

Release Note

NONE

SgtCoDFish

Most of this looks pretty self explanatory and makes sense. Just one question from me before I'd feel comfortable merging!

SgtCoDFish · 2024-09-19T10:19:59Z

 	solver, err := c.solverFor(ch.Spec.Type)
 	if err != nil {
 		log.Error(err, "error getting solver for challenge")
-		return nil


question: Previously we swallowed the error and returned nil. Same in the if err := solver.CleanUp(ctx, ch); err != nil block below.

What's the motivation for the change to now start returning the error here (and below)? What effect will it have?

There are two situations in which a cleanup is performed:

acme.IsFinalState(ch.Status.State)

!ch.DeletionTimestamp.IsZero()

The old code swallowed errors when !ch.DeletionTimestamp.IsZero() but returned errors when acme.IsFinalState(ch.Status.State). So this change will only affect the first case.
In that case, the deletion of the challenge resource will be blocked until the handleFinalizer is successful. An error will be returned if something went wrong and the CleanUp function will be retried.
The previous behavior for the first case was that the CleanUp function was called once and the error was only logged and the finalizer deleted unconditionally.

So, the new behavior will block deletion until the cleanup succeeds & keep retrying (which was what is described here: #3640 and what the original PR tried to solve #5126).

🤔 I just found there is a counter-issue: #7234
Challenges that are stuck in their cleanup phase: cleanup is failing and the resource is not being deleted...

@erikgb convinced me that we should still move forward. I updated the PR.

inteon · 2024-09-19T10:35:05Z

/hold
There might be an unforeseen issue: #7286 (comment)

cert-manager-bot · 2024-12-18T11:34:05Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close.
/lifecycle stale

cert-manager-bot · 2025-01-17T11:48:13Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close.
/lifecycle rotten
/remove-lifecycle stale

inteon · 2026-02-04T09:45:16Z

Looks like issue #7234 will be solved in a different manner: #7234 (comment)

I think we are able to move forward with this PR.

/unhold

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

erikgb · 2026-02-05T08:56:34Z

-		Delete(cmacme.ACMEDomainQualifiedFinalizer, cmacme.ACMELegacyFinalizer)
-	return len(finalizers) > 0
+	finalizers := sets.New(ch.Finalizers...)
+	return !finalizers.Has(cmacme.ACMELegacyFinalizer) && !finalizers.Has(cmacme.ACMEDomainQualifiedFinalizer)


Suggested change

return !finalizers.Has(cmacme.ACMELegacyFinalizer) && !finalizers.Has(cmacme.ACMEDomainQualifiedFinalizer)

return !finalizers.Has(cmacme.ACMEDomainQualifiedFinalizer)

I think this is clearer, as we require all challenges to have the new finalizer.

This function is called in two places:

https://github.com/cert-manager/cert-manager/pull/7286/changes#diff-e9636826ce57cc3a7988b9d27b475a40c79c97a21eb80ba392dc9f137f770004R115 - Here, there will never be a legacy finalizer (since we drop it a few lines above)

https://github.com/cert-manager/cert-manager/pull/7286/changes#diff-e9636826ce57cc3a7988b9d27b475a40c79c97a21eb80ba392dc9f137f770004R296 - Here, we want to also cleanup in case we only have a legacy finalizer.

So, I do think that is makes sense to check for both to make sure we cleanup in the second case.

True, but the naming is confusing here. Have to look at the "surroundings" to understand how this works. But we can leave this for an eventual follow-up.

Yes, let's leave the name for now. I already reverted the name change I wanted to make.

erikgb · 2026-02-05T11:21:31Z

 						gen.DefaultTestNamespace,
 						gen.ChallengeFrom(deletedChallenge,
-							gen.SetChallengeProcessing(true),
+							gen.SetChallengeProcessing(false),


Is this change expected? Processing is part of the status subresource. This action should only assert that the finalizers are removed.

erikgb

/lgtm

Thanks @inteon. I would expect a bit of "noise" when unleashing this bugfix, but I am convinced that moving forward is correct and worth it.

inteon · 2026-02-05T12:43:10Z

/approve

cert-manager-prow · 2026-02-05T12:43:19Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inteon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [inteon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

inteon · 2026-02-05T14:29:15Z

/retest

cert-manager-prow · 2026-02-05T14:34:04Z

@inteon: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-cert-manager-master-e2e-v1-32-upgrade	`95eeab9`	link	true	`/test pull-cert-manager-master-e2e-v1-32-upgrade`
pull-cert-manager-master-e2e-v1-32	`95eeab9`	link	true	`/test pull-cert-manager-master-e2e-v1-32`
pull-cert-manager-master-e2e-v1-33-upgrade	`95eeab9`	link	true	`/test pull-cert-manager-master-e2e-v1-33-upgrade`
pull-cert-manager-master-e2e-v1-34-upgrade	`95eeab9`	link	true	`/test pull-cert-manager-master-e2e-v1-34-upgrade`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

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. I understand the commands that are listed here.

inteon · 2026-02-05T14:36:45Z

/retest

inteon force-pushed the finalizer_on_failure branch 2 times, most recently from 0470fa7 to 41f5ebc Compare September 18, 2024 18:58

SgtCoDFish mentioned this pull request Sep 19, 2024

WIP: Only remove the cleanup finalizer if the cleanup succeeds #5126

Closed

inteon force-pushed the finalizer_on_failure branch from 41f5ebc to 95eeab9 Compare September 19, 2024 10:05

SgtCoDFish reviewed Sep 19, 2024

View reviewed changes

cert-manager-prow Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 19, 2024

cert-manager-prow Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 18, 2024

cert-manager-prow Bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Jan 17, 2025

inteon added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Jan 20, 2025

wallrj mentioned this pull request May 9, 2025

graduate UseDomainQualifiedFinalizer to GA #7735

Merged

cert-manager-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 9, 2025

Peac36 mentioned this pull request Sep 15, 2025

wip: add retry mechanism for challenge solver whenever we detect unauthorized error #7897

Open

inteon mentioned this pull request Feb 3, 2026

Improve Challenge finalizer handling #8470

Closed

inteon force-pushed the finalizer_on_failure branch from 95eeab9 to 9755045 Compare February 4, 2026 09:38

cert-manager-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 4, 2026

cert-manager-prow Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 4, 2026

inteon force-pushed the finalizer_on_failure branch from 9755045 to 3aca768 Compare February 4, 2026 09:48

inteon force-pushed the finalizer_on_failure branch from 3aca768 to 2c7db23 Compare February 4, 2026 09:52

inteon requested a review from erikgb February 4, 2026 10:21

erikgb reviewed Feb 4, 2026

View reviewed changes

Comment thread pkg/controller/acmechallenges/finalizer_test.go Outdated

Comment thread pkg/controller/acmechallenges/finalizer.go Outdated

Comment thread pkg/controller/acmechallenges/finalizer.go Outdated

don't remove finalizer if CleanUp function fails

0a5bab8

Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

inteon force-pushed the finalizer_on_failure branch from 2c7db23 to 0a5bab8 Compare February 5, 2026 08:19

erikgb reviewed Feb 5, 2026

View reviewed changes

cert-manager-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Feb 5, 2026

cert-manager-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 5, 2026

cert-manager-prow Bot merged commit b89b422 into cert-manager:master Feb 5, 2026
6 checks passed

erikgb mentioned this pull request Feb 10, 2026

Challenge Records Not Always Cleaned Up #3640

Closed

rassie mentioned this pull request Mar 20, 2026

Challenge creation fails on OpenShift after #7286 — missing issuers/finalizers RBAC #8636

Closed

This was referenced Apr 20, 2026

docs(cert-manager): document DNS-01 orphan TXT investigation (#1324) Diixtra/diixtra-forge#1331

Closed

cert-manager: DNS-01 Cleanup() not running after issuance — orphan _acme-challenge TXT records accumulating Diixtra/diixtra-forge#1324

Closed

sdushay mentioned this pull request Apr 21, 2026

Challenges persist after their Order is GC'd due to Issuer ownerReference added in #7286 (v1.20+) #8737

Closed

erikgb mentioned this pull request Apr 23, 2026

Remove issuer owner reference from challenges #8743

Merged

	return !finalizers.Has(cmacme.ACMELegacyFinalizer) && !finalizers.Has(cmacme.ACMEDomainQualifiedFinalizer)
	return !finalizers.Has(cmacme.ACMEDomainQualifiedFinalizer)

Conversation

inteon commented Sep 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Kind

Release Note

Uh oh!

SgtCoDFish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inteon Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

inteon commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cert-manager-bot commented Dec 18, 2024

Uh oh!

cert-manager-bot commented Jan 17, 2025

Uh oh!

inteon commented Feb 4, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erikgb Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

erikgb left a comment

Choose a reason for hiding this comment

Uh oh!

inteon commented Feb 5, 2026

Uh oh!

cert-manager-prow Bot commented Feb 5, 2026

Uh oh!

inteon commented Feb 5, 2026

Uh oh!

cert-manager-prow Bot commented Feb 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

inteon commented Feb 5, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

inteon commented Sep 18, 2024 •

edited

Loading

inteon Sep 19, 2024 •

edited

Loading

inteon commented Sep 19, 2024 •

edited

Loading

erikgb Feb 5, 2026 •

edited

Loading

cert-manager-prow Bot commented Feb 5, 2026 •

edited

Loading