NO-JIRA: certrotation: Fix using a wrong annotation value #2061

tchap · 2025-11-27T10:47:06Z

This is actually not affecting the logic, but a wrong error is returned.

Improved the unit test to check events and annotation keys.

openshift-ci-robot · 2025-11-27T10:47:09Z

@tchap: This pull request explicitly references no jira issue.

In response to this:

This is actually not affecting the logic, but a wrong error is returned.

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 openshift-eng/jira-lifecycle-plugin repository.

openshift-ci · 2025-11-27T10:47:28Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Needs approval from an approver in each of these files:

pkg/operator/OWNERS

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

pkg/operator/certrotation/signer.go

tchap · 2025-11-27T13:33:08Z

pkg/operator/certrotation/signer_test.go


 func TestEnsureSigningCertKeyPair(t *testing.T) {
+	verifyActionFunc := func(check func(t *testing.T, action clienttesting.Action)) func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
+		return func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {


This code is just copied and refactored so that it can be used for both create and update.

tchap · 2025-11-27T13:37:07Z

pkg/operator/certrotation/signer_test.go

+				"auth.openshift.io/certificate-issuer",
+				"certificates.openshift.io/refresh-period",
+				"openshift.io/owning-component",
+			)


Added checking for annotation keys. Getting the precise values would require mocking time and what not, which is not currently possible. But it could be done if we changed some public functions.

tchap · 2025-11-27T13:37:58Z

pkg/operator/certrotation/signer_test.go

+			RefreshOnlyWhenExpired: false,
+			verifyActions:          verifyActionsOnUpdated,
+			expectedEvents: []*corev1.Event{
+				{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notBefore`},


This actually ensures the reason as fixed in this PR works now.

ic, normally i would prefer to create a valid secret and then remove the notBefore annotation but i understand that would require more work.

pkg/operator/certrotation/signer_test.go

p0lyn0mial · 2025-11-27T14:07:30Z

pkg/operator/certrotation/signer_test.go


-		verifyActions func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool)
-		expectedError string
+		verifyActions  func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool)


would it make sense to change the signature to []func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) ?

and then the usage would be:

[verifyFn1, verifyFn2]

I will rather rename it to verifyAction, because there is at most one.

Not sure I am making it better or worse now, but I updated it to use verifyAction. Not specifying any function means that no action is expected.

This is actually not affecting the logic, but a wrong error is returned. Improved the unit test to check events and annotation keys.

openshift-ci · 2025-11-27T15:51:12Z

@tchap: all tests passed!

Full PR test history. Your PR dashboard.

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.

p0lyn0mial · 2025-11-27T16:07:29Z

pkg/operator/certrotation/signer_test.go

 )

 func TestEnsureSigningCertKeyPair(t *testing.T) {
+	newVerifyActionFunc := func(check func(t *testing.T, action clienttesting.Action)) func(t *testing.T, action clienttesting.Action) {


could we change the signature of this method to accept the required object ?
in that case this function would be responsible for validating that object.

p0lyn0mial · 2025-11-27T16:08:02Z

pkg/operator/certrotation/signer_test.go

+		t.Helper()
+		if !action.Matches("create", "secrets") {
+			t.Fatalf("Expected a Create action, got %s", spew.Sdump(action))
+		}


then in this function we would validate the action, extract the object and call newVerifyActionFunc (we could rename the function to newVerifyObjectFunc`)

p0lyn0mial · 2025-11-27T16:08:11Z

pkg/operator/certrotation/signer_test.go

+		t.Helper()
+		if !action.Matches("update", "secrets") {
+			t.Fatalf("Expected an Update action, got %s", spew.Sdump(action))
+		}


p0lyn0mial · 2025-11-27T16:09:24Z

pkg/operator/certrotation/signer_test.go

-			test.verifyActions(t, client, updated)
+			actions := client.Actions()
+			if test.verifyAction == nil {
+				if len(actions) > 0 {


could we change this code to the previous version where we used the verifyEmptyAction function that was used on the test cases

p0lyn0mial · 2025-11-27T16:10:02Z

pkg/operator/certrotation/signer_test.go

+				}
+			}
+
+			if events := pruneEventFieldsForComparison(recorder.Events()); !cmp.Equal(events, test.expectedEvents) {


we usually don't verify the events.
is this the only way to verify the new test cases ?

UPDATE: I just saw #2061 (comment)

not ideal but changing it would require more work.

why have you decided to remove the validation of the updated var?

p0lyn0mial · 2025-11-27T19:31:11Z

pkg/operator/certrotation/signer_test.go

+			if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeSigner {
+				t.Errorf("expected certificate type 'signer', got: %v", certType)
+			}
+			if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 {


I know this is pre-existing (we don't have to change it now) but I would expect the tests to at least check if the cert and the key are valid.

p0lyn0mial · 2025-11-27T19:46:41Z

pkg/operator/certrotation/signer_test.go

+				}
+			}
+
+			if events := pruneEventFieldsForComparison(recorder.Events()); !cmp.Equal(events, test.expectedEvents) {


why have you decided to remove the validation of the updated var?

p0lyn0mial · 2025-11-27T19:50:00Z

pkg/operator/certrotation/signer_test.go

+			RefreshOnlyWhenExpired: false,
+			verifyActions:          verifyActionsOnUpdated,
+			expectedEvents: []*corev1.Event{
+				{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notBefore`},


ic, normally i would prefer to create a valid secret and then remove the notBefore annotation but i understand that would require more work.

tchap · 2025-11-28T13:34:27Z

In the end I opened #2062 to refactor tests first. Once merged, I will rebase and fix the issue with event messages. But this will still require checking events, I think.

tchap · 2025-11-28T14:30:28Z

^^^

/hold

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 27, 2025

openshift-ci bot requested review from bertinatto and deads2k November 27, 2025 10:47

p0lyn0mial reviewed Nov 27, 2025

View reviewed changes

pkg/operator/certrotation/signer.go Show resolved Hide resolved

tchap force-pushed the certrotation branch 2 times, most recently from 84b8a5b to 247b85e Compare November 27, 2025 13:30

tchap commented Nov 27, 2025

View reviewed changes

p0lyn0mial reviewed Nov 27, 2025

View reviewed changes

pkg/operator/certrotation/signer_test.go Outdated Show resolved Hide resolved

p0lyn0mial reviewed Nov 27, 2025

View reviewed changes

pkg/operator/certrotation/signer_test.go Outdated Show resolved Hide resolved

p0lyn0mial reviewed Nov 27, 2025

View reviewed changes

tchap force-pushed the certrotation branch 2 times, most recently from 919786a to dff2a0e Compare November 27, 2025 15:35

certrotation: Fix using a wrong annotation value

4d6d0c1

This is actually not affecting the logic, but a wrong error is returned. Improved the unit test to check events and annotation keys.

tchap force-pushed the certrotation branch from dff2a0e to 4d6d0c1 Compare November 27, 2025 15:37

p0lyn0mial reviewed Nov 27, 2025

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 28, 2025

NO-JIRA: certrotation: Fix using a wrong annotation value #2061

Are you sure you want to change the base?

NO-JIRA: certrotation: Fix using a wrong annotation value #2061

Conversation

tchap commented Nov 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Nov 27, 2025

Uh oh!

openshift-ci bot commented Nov 27, 2025

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

openshift-ci bot commented Nov 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

p0lyn0mial Nov 27, 2025 • 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!

p0lyn0mial Nov 27, 2025 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tchap commented Nov 28, 2025

Uh oh!

tchap commented Nov 28, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tchap commented Nov 27, 2025 •

edited

Loading

p0lyn0mial Nov 27, 2025 •

edited

Loading

p0lyn0mial Nov 27, 2025 •

edited

Loading