fix(renew): Do not renew a certificate if its renewPolicy=Disabled

[sklirg]

Pull Request Motivation

Fixes #9031

Kind

/kind bug

Release Note

Avoid controller panic if a Certificate sets spec.renewal.policy=Disabled

[cert-manager-prow]

Hi @sklirg. Thanks for your PR.

I'm waiting for a cert-manager member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[sklirg]

Note: This does not "fix" the nil issue mentioned in #9031, caused by the fact that pki.RenewalTime can return nil, nil. It simply avoids the case where we call that function and can get nil, nil as the response in this function. 😄

I'm not sure what the expected behavior for the caller is if the function returns nil, nil. In this case, it could

renewalTime, err := pki.RenewalTime(notBefore.Time, notAfter.Time, crt.Spec.RenewBefore, crt.Spec.RenewBeforePercentage, crt.Spec.Renewal) 
if err != nil { 
 	reason = WindowError 
 	message = err.Error() 
} else if renewalTime == nil {
 	return "", "", false
}

?

[hjoshi123]

Thank you for catching the edge case @sklirg.. I have a minor nit other than that the PR looks good to me.

[hjoshi123]

/ok-to-test

[hjoshi123]

Note: This does not "fix" the nil issue mentioned in #9031, caused by the fact that pki.RenewalTime can return nil, nil. It simply avoids the case where we call that function and can get nil, nil as the response in this function. 😄

I'm not sure what the expected behavior for the caller is if the function returns nil, nil. In this case, it could

renewalTime, err := pki.RenewalTime(notBefore.Time, notAfter.Time, crt.Spec.RenewBefore, crt.Spec.RenewBeforePercentage, crt.Spec.Renewal) 
if err != nil { 
 	reason = WindowError 
 	message = err.Error() 
} else if renewalTime == nil {
 	return "", "", false
}

?

This should be fine because the only case where the renewal time and error are nil is when the policy is disabled.. in all other cases we return either one of those

[sklirg]

/retest

[hjoshi123]

/label tide/merge-method-squash

[wallrj-cyberark]

/cherry-pick release-1.21

[cert-manager-bot]

@wallrj-cyberark: once the present PR merges, I will cherry-pick it on top of release-1.21 in a new PR and assign it to you.

Details

In response to this:

/cherry-pick release-1.21

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.

[Copilot]

Pull request overview

This PR updates the certificate trigger policy evaluation to short-circuit renewal when a Certificate sets spec.renewal.policy=Disabled, preventing a nil dereference panic during renewal-time evaluation.

Changes:

• Add an early return in CurrentCertificateNearingExpiry to skip renewal when the renewal policy is Disabled.
• Introduce a new policy reason constant intended to represent “renewal disabled”.
• Add a trigger policy chain unit test case covering the disabled renewal policy scenario.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
internal/controller/certificates/policies/constants.go	Adds a new policy reason constant for the “renewal disabled” scenario.
internal/controller/certificates/policies/checks.go	Skips renewal evaluation when spec.renewal.policy is Disabled.
internal/controller/certificates/policies/checks_test.go	Adds a unit test ensuring disabled renewal policy does not trigger reissuance (and avoids panic).

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[wallrj-cyberark]

/retest

[wallrj-cyberark]

/retest

Looks like an infra flake, not related to this change — the failed e2e-v1-36 run shows a broad cluster-wide cascade (unrelated Certificates/CSR/Vault/ARI tests all timing out simultaneously, then ~1.8k connection reset by peer errors against the local API server), consistent with the kind cluster's API server dying mid-run rather than a code regression.

[wallrj-cyberark]

Verified: the fix correctly short-circuits before the nil-pointer dereference (confirmed by tracing pki.RenewalTime returning (nil, nil) for CertificateRenewalPolicyDisabled), the added test would panic without the fix, and both Copilot findings have been resolved (one addressed by the suggested-change fix, one already deliberately settled by @hjoshi123 earlier in review). CI is green across all four jobs plus DCO; the earlier e2e-v1-36 failure was a cluster-wide infra flake (unrelated tests cascading from a dead API server connection), not a regression — confirmed by a clean retest.

[wallrj]

/approve
/lgtm

[cert-manager-prow]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: wallrj, wallrj-cyberark

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 [wallrj]

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

[wallrj-cyberark]

For anyone following this from the release notes: I've opened cert-manager/website#2210, which adds worked, realistic scenarios to the renewal policies documentation — including an example of using policy: Disabled for manually-managed root/intermediate CAs, among others (regulated change-control windows, coordinating rotation with a paired system, avoiding renewal during a traffic freeze).

[cert-manager-bot]

@wallrj-cyberark: new pull request created: #9038

Details

In response to this:

/cherry-pick release-1.21

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.