Add defaulting to Certificate - CertificateRequest comparison #8160

inteon · 2025-10-10T10:28:18Z

This should prevent the unexpected renewals after upgrading to 1.19.x (see #8158).

How to test:

install cert-manager 1.18.2 and create a certificate
upgrade to cert-manager 1.19.0 (keep the CRDs @ 1.18.2)
upgrade the CRDs to 1.19.0 (start with Certificate CRD)

Kind

/kind bug

Release Note

BUGFIX: in case kind or group in the issuerRef of a Certificate was omitted, upgrading to 1.19.x incorrectly caused the certificate to be renewed

CyberArk tracker: VC-46054

…ting mismatches when upgrading Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

erikgb

Have you checked if this fixes #8158, @inteon?

/approve

inteon · 2025-10-10T11:31:56Z

Have you checked if this fixes #8158, @inteon?

/approve

No, I have not had the time yet, could you verify?

erikgb · 2025-10-10T11:35:45Z

Have you checked if this fixes #8158, @inteon?
/approve

No, I have not had the time yet, could you verify?

I have no idea on how to install/upgrade to a c-m version from a feature branch.

Copilot

Pull Request Overview

This PR fixes a bug where cert-manager 1.19.x incorrectly renewed certificates during upgrades when the issuerRef's kind or group fields were omitted in the original Certificate. The fix replaces a simple reflect.DeepEqual comparison with custom logic that handles default values for issuerRef fields.

Replaces reflect.DeepEqual with explicit field-by-field comparison for IssuerRef
Adds helper functions to handle default values for issuer kind and group fields
Removes unused reflect import

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pkg/util/pki/match.go

erikgb

Thanks, @inteon! Even if this issue doesn't fully resolve #8158, I think it's a good change. I also think we should cherry-pick this to the release-1.19 branch. Adding a hold in case you want to implement the suggestion from Copilot.

/lgtm
/approve
/hold
/cherry-pick release-1.19

maelvls · 2025-10-14T09:50:33Z

We plan on merging this fix before releasing v1.19.1, and backport it to v1.18.3. @wallrj proposed to work on this.

wallrj-cyberark · 2025-10-14T16:34:07Z

I've pushed the following changes to Tim's branch:

added some unit-tests
added explanatory comments.
documented the new default constants and used the same sources as are used in the CertificateRequest controller.

- Add TestRequestMatchesSpecIssuerRef covering name, kind, and group - Include cases for defaulted Kind/Group vs empty fields in CRs - Treat empty Kind/Group in CertificateRequest as 'Issuer' and - 'cert-manager.io' to avoid re-issuing certificates after 1.19 Signed-off-by: Richard Wall <richard.wall@cyberark.com>

wallrj-cyberark · 2025-10-14T17:02:51Z

We plan on merging this fix before releasing v1.19.1, and backport it to v1.18.3. @wallrj proposed to work on this.

This PR alone doesn't fix the problem. We think the original problem occurs when user upgrade to the 1.19 CRDs before upgrading the cert-manager controller causing the still running 1.18 controller to re-establish API watches and end up with inconsistent caches, where the cache contains Certificates that have the new API defaults, but CertificateRequests which have not yet been refreshed and do not have the new defaults.

erikgb · 2025-10-14T18:39:52Z

We plan on merging this fix before releasing v1.19.1, and backport it to v1.18.3. @wallrj proposed to work on this.

This PR alone doesn't fix the problem. We think the original problem occurs when user upgrade to the 1.19 CRDs before upgrading the cert-manager controller causing the still running 1.18 controller to re-establish API watches and end up with inconsistent caches, where the cache contains Certificates that have the new API defaults, but CertificateRequests which have not yet been refreshed and do not have the new defaults.

I agree, but this PR will allow us to reintroduce the API defaults in a future release - if we want. I have created a PR to revert the problematic API defaults, #8173, and think we should merge both PRs and cherry-pick them to the 1.19 release branch. This PR can even be cherry-picked to the 1.18 release branch, as I think it also fixes a bug unrelated to the API defaulting problem.

wallrj-cyberark · 2025-10-15T08:48:47Z

pkg/util/pki/match_test.go

+		})
+	}
+}
+


I cherry picked these new tests on top of #8173 and ran them to demonstrate the problem. Here's the output:

-*- mode: compilation; default-directory: "~/projects/cert-manager/cert-manager/pkg/util/pki/" -*- Compilation started at Wed Oct 15 09:47:25 go test -run='TestRequestMatchesSpecIssuerRef$' --- FAIL: TestRequestMatchesSpecIssuerRef (0.69s) --- FAIL: TestRequestMatchesSpecIssuerRef/should_not_report_any_violation_if_Certificate_issuerRef_Kind_and_Group_are_defaulted_and_CertificateRequest's_are_empty (0.00s) match_test.go:477: Error Trace: /home/richard/projects/cert-manager/cert-manager/pkg/util/pki/match_test.go:477 Error: Not equal: expected: []string(nil) actual : []string{"spec.issuerRef"} Diff: --- Expected +++ Actual @@ -1,2 +1,4 @@ -([]string) <nil> +([]string) (len=1) { + (string) (len=14) "spec.issuerRef" +} Test: TestRequestMatchesSpecIssuerRef/should_not_report_any_violation_if_Certificate_issuerRef_Kind_and_Group_are_defaulted_and_CertificateRequest's_are_empty --- FAIL: TestRequestMatchesSpecIssuerRef/should_not_report_any_violation_if_Certificate_issuerRef_Kind_and_Group_are_defaulted_and_CertificateRequest_issuerRef_Group_is_empty (0.00s) match_test.go:477: Error Trace: /home/richard/projects/cert-manager/cert-manager/pkg/util/pki/match_test.go:477 Error: Not equal: expected: []string(nil) actual : []string{"spec.issuerRef"} Diff: --- Expected +++ Actual @@ -1,2 +1,4 @@ -([]string) <nil> +([]string) (len=1) { + (string) (len=14) "spec.issuerRef" +} Test: TestRequestMatchesSpecIssuerRef/should_not_report_any_violation_if_Certificate_issuerRef_Kind_and_Group_are_defaulted_and_CertificateRequest_issuerRef_Group_is_empty --- FAIL: TestRequestMatchesSpecIssuerRef/should_not_report_any_violation_if_Certificate_issuerRef_Kind_and_Group_are_defaulted_and_CertificateRequest_issuerRef_Kind_is_empty (0.00s) match_test.go:477: Error Trace: /home/richard/projects/cert-manager/cert-manager/pkg/util/pki/match_test.go:477 Error: Not equal: expected: []string(nil) actual : []string{"spec.issuerRef"} Diff: --- Expected +++ Actual @@ -1,2 +1,4 @@ -([]string) <nil> +([]string) (len=1) { + (string) (len=14) "spec.issuerRef" +} Test: TestRequestMatchesSpecIssuerRef/should_not_report_any_violation_if_Certificate_issuerRef_Kind_and_Group_are_defaulted_and_CertificateRequest_issuerRef_Kind_is_empty FAIL exit status 1 FAIL github.com/cert-manager/cert-manager/pkg/util/pki 0.745s Compilation exited abnormally with code 1 at Wed Oct 15 09:47:27, duration 2.48 s

wallrj-cyberark · 2025-10-15T09:56:35Z

pkg/util/pki/match.go

+// These defaults are also applied at runtime by the cert-manager
+// CertificateRequest controller.
+//
+// TODO(wallrj): Move this runtime defaulting to `internal/apis/certmanager/v1/defaults.go`


@erikgb Removed TODO comment as agreed during standup. I made my point. And we hope that in future the API defaults will be used. Not runtime defaults.

wallrj-cyberark · 2025-10-15T09:56:44Z

internal/apis/certmanager/v1/defaults.go

 //
 // ... to the API struct.
+//
+// TODO(wallrj): Set the issuerRef defaults here too.


Removed TODO comment as agreed during standup. I made my point. And we hope that in future the API defaults will be used. Not runtime defaults.

Copilot

Pull Request Overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

pkg/util/pki/match.go

…esSpec - Explain that IssuerRef comparisons ignore default group and kind - Describe upgrades where CRD defaults can cause transient mismatches - Document issuerKindsEqual and issuerGroupsEqual behavior - Update test comment to describe the equivalence rationale Signed-off-by: Richard Wall <richard.wall@cyberark.com>

erikgb

/lgtm
/approve

Thanks for picking this up, @wallrj! And for the initial work, @inteon! 🚀

We should attempt an automated cherry-pick of this to a supported release branches.

cert-manager-prow · 2025-10-15T10:07:14Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [erikgb]

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

wallrj-cyberark · 2025-10-15T10:10:57Z

Thanks @erikgb

/unhold
/cherry-pick release-1.18
/cherry-pick release-1.19

cert-manager-bot · 2025-10-15T10:11:01Z

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

In response to this:

Thanks @erikgb

/unhold
/cherry-pick release-1.18
/cherry-pick release-1.19

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.

cert-manager-bot · 2025-10-15T10:14:36Z

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

In response to this:

Thanks @erikgb

/unhold
/cherry-pick release-1.18
/cherry-pick release-1.19

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.

cert-manager-bot · 2025-10-15T10:15:14Z

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

In response to this:

Thanks @erikgb

/unhold
/cherry-pick release-1.18
/cherry-pick release-1.19

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.

add defaulting to Certificate - CertificateRequest comparison, preven…

c0e4c94

…ting mismatches when upgrading Signed-off-by: Tim Ramlot <42113979+inteon@users.noreply.github.com>

cert-manager-prow bot added kind/bug Categorizes issue or PR as related to a bug. release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. labels Oct 10, 2025

inteon requested a review from erikgb October 10, 2025 10:28

cert-manager-prow bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Oct 10, 2025

erikgb reviewed Oct 10, 2025

View reviewed changes

cert-manager-prow bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 10, 2025

erikgb requested a review from Copilot October 12, 2025 09:36

Copilot AI reviewed Oct 12, 2025

View reviewed changes

pkg/util/pki/match.go Outdated Show resolved Hide resolved

erikgb reviewed Oct 12, 2025

View reviewed changes

cert-manager-prow bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Oct 12, 2025

maelvls assigned wallrj Oct 14, 2025

maelvls added the cybr Used by CyberArk-employed maintainers to report to line management what's being worked on. label Oct 14, 2025

This was referenced Oct 14, 2025

Security evaluation of Go 1.25.1's CVEs and upgrade to Go 1.25.3 cert-manager/community#56

Closed

CVEs in Go 1.25.1: Release cert-manager v1.19.1 and v1.18.3 using Go 1.25.3 #8171

Closed

erikgb mentioned this pull request Oct 14, 2025

REVERT: API defaults for issuer reference kind and group #8173

Merged

wallrj-cyberark force-pushed the fix_compare branch from f523072 to fba189f Compare October 14, 2025 16:18

cert-manager-prow bot added dco-signoff: yes Indicates that all commits in the pull request have the valid DCO sign-off message. and removed dco-signoff: no Indicates that at least one commit in this pull request is missing the DCO sign-off message. labels Oct 14, 2025

wallrj-cyberark force-pushed the fix_compare branch from fba189f to 3682691 Compare October 14, 2025 16:21

wallrj-cyberark force-pushed the fix_compare branch from 3682691 to bae74d1 Compare October 14, 2025 16:37

wallrj-cyberark reviewed Oct 15, 2025

View reviewed changes

wallrj-cyberark force-pushed the fix_compare branch from bae74d1 to e44951b Compare October 15, 2025 09:55

wallrj-cyberark reviewed Oct 15, 2025

View reviewed changes

wallrj-cyberark requested review from erikgb and wallrj October 15, 2025 09:57

wallrj requested a review from Copilot October 15, 2025 09:59

Copilot AI reviewed Oct 15, 2025

View reviewed changes

pkg/util/pki/match.go Outdated Show resolved Hide resolved

wallrj-cyberark force-pushed the fix_compare branch from e44951b to 04a06e6 Compare October 15, 2025 10:03

erikgb reviewed Oct 15, 2025

View reviewed changes

cert-manager-prow bot added the lgtm Indicates that a PR is ready to be merged. label Oct 15, 2025

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 Oct 15, 2025

cert-manager-prow bot merged commit 1514131 into cert-manager:master Oct 15, 2025
6 checks passed

cert-manager-bot mentioned this pull request Oct 15, 2025

[release-1.18] Add defaulting to Certificate - CertificateRequest comparison #8174

Merged

cert-manager-bot mentioned this pull request Oct 15, 2025

[release-1.19] Add defaulting to Certificate - CertificateRequest comparison #8175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add defaulting to Certificate - CertificateRequest comparison #8160

Add defaulting to Certificate - CertificateRequest comparison #8160

inteon commented Oct 10, 2025 •

edited by maelvls

Loading

erikgb left a comment

inteon commented Oct 10, 2025

erikgb commented Oct 10, 2025

Copilot AI left a comment

Uh oh!

erikgb left a comment

maelvls commented Oct 14, 2025 •

edited

Loading

wallrj-cyberark commented Oct 14, 2025

wallrj-cyberark commented Oct 14, 2025

erikgb commented Oct 14, 2025

wallrj-cyberark Oct 15, 2025 •

edited

Loading

wallrj-cyberark Oct 15, 2025

wallrj-cyberark Oct 15, 2025

Copilot AI left a comment

Uh oh!

erikgb left a comment

cert-manager-prow bot commented Oct 15, 2025

wallrj-cyberark commented Oct 15, 2025

cert-manager-bot commented Oct 15, 2025

Uh oh!

cert-manager-bot commented Oct 15, 2025

cert-manager-bot commented Oct 15, 2025

Labels

6 participants

Add defaulting to Certificate - CertificateRequest comparison #8160

Add defaulting to Certificate - CertificateRequest comparison #8160

Conversation

inteon commented Oct 10, 2025 • edited by maelvls Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to test:

Kind

Release Note

erikgb left a comment

Choose a reason for hiding this comment

inteon commented Oct 10, 2025

erikgb commented Oct 10, 2025

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

erikgb left a comment

Choose a reason for hiding this comment

maelvls commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

wallrj-cyberark commented Oct 14, 2025

wallrj-cyberark commented Oct 14, 2025

erikgb commented Oct 14, 2025

wallrj-cyberark Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

wallrj-cyberark Oct 15, 2025

Choose a reason for hiding this comment

wallrj-cyberark Oct 15, 2025

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

erikgb left a comment

Choose a reason for hiding this comment

cert-manager-prow bot commented Oct 15, 2025

wallrj-cyberark commented Oct 15, 2025

cert-manager-bot commented Oct 15, 2025

Uh oh!

cert-manager-bot commented Oct 15, 2025

cert-manager-bot commented Oct 15, 2025

Labels

6 participants

inteon commented Oct 10, 2025 •

edited by maelvls

Loading

maelvls commented Oct 14, 2025 •

edited

Loading

wallrj-cyberark Oct 15, 2025 •

edited

Loading