fix(webhook): validate that renewal windows are reachable within the certificate lifetime - #9305

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign wallrj for approval. For more information see the Code Review Process.

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

Hi @somaz94. 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.

@somaz94 Thank you for picking this up but I feel i need to push back here. I don't really like the approach here and feels like there are a lot of factors that have not been thought through. I would love to know what made you take this approach.

For example:

• The webhook only knows spec.duration; the controller uses the issued cert's actual validity, which many issuers (ACME/LE, Venafi policy) override in either direction. For example: considering LE a 24h spec.duration with a weekly window is rejected even though the actual 90‑day LE cert would hit it every time — and the reverse (spec says 90d, CA issues 7d) sails through and still breaks. The check will both reject working configs and accept broken ones.
• Also windows are union in nature the way its implemented. So how would you handle the duration validation there?

A naive validation is easy to implement here but something like this it has the ability to break both valid and invalid ones for the wrong reasons. I am not saying that validation is a bad thing, I get the issue but we need to be careful and thoughtful before we add such strict validations. I also feel, if we are to fix the behavior mentioned in the bug we could look at readiness controller too to see how we can gracefully retry the renewal mechanism. Long term that is the better option than stricter validation in my opinion.

I understand that this was an AI assisted PR but I would prefer to understand your thought process on the PR and how you think this mechanism should work ideally.

Fair pushback, and I think you're right on both counts.

The approach came from the issue rather than from me. #9146 suggested an admission feasibility check on the grounds that spec.duration is known at admission time, and I implemented that as written. Your first point is what breaks that premise. RenewalTime takes notBefore and notAfter and works from notAfter.Sub(notBefore), which the code itself calls actualDuration, and the search bounds are derived from those too. The webhook only ever sees spec.duration and cannot know what the issuer actually handed back, so it is measuring a different quantity than the controller uses. That is not something I can patch inside a validating webhook.

The union point is right as well. I validate each window on its own, while applyRenewBeforeWithWindows accumulates across all of them, so a spec whose windows are jointly reachable can still be rejected because one of them is sparse.

The one piece I would still argue for is the cron that matches no date at all, like 0 0 30 2 *. That can never be entered whatever the issued duration turns out to be and whatever else is in the union, so it does not rest on either premise.

On how it should work ideally, I now agree with you that this belongs in the renewal path rather than in admission. Admission cannot see the value the decision actually depends on, so a check there is guessing, and guessing wrong in both directions is worse than not checking at all. I am happy to cut this down to the never matches case, or to close it and look at graceful retry in the readiness and trigger controllers instead. Which would you prefer?