Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
+150 −96
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkg/operator/certrotation/signer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ func getValidityFromAnnotations(annotations map[string]string) (notBefore time.T
return notBefore, notAfter, fmt.Sprintf("bad expiry: %q", notAfterString)
}
notBeforeString := annotations[CertificateNotBeforeAnnotation]
if len(notAfterString) == 0 {
if len(notBeforeString) == 0 {
return notBefore, notAfter, "missing notBefore"
}
notBefore, err = time.Parse(time.RFC3339, notBeforeString)
Expand Down
244 changes: 149 additions & 95 deletions pkg/operator/certrotation/signer_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,75 +2,99 @@ package certrotation

import (
"context"
clocktesting "k8s.io/utils/clock/testing"
"strings"
"testing"
"time"

"github.com/davecgh/go-spew/spew"
"github.com/google/go-cmp/cmp"

corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/util/sets"
kubefake "k8s.io/client-go/kubernetes/fake"
corev1listers "k8s.io/client-go/listers/core/v1"
clienttesting "k8s.io/client-go/testing"
"k8s.io/client-go/tools/cache"
clocktesting "k8s.io/utils/clock/testing"

"github.com/openshift/api/annotations"
"github.com/openshift/library-go/pkg/operator/events"
)

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

@p0lyn0mial p0lyn0mial Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return func(t *testing.T, action clienttesting.Action) {
t.Helper()
check(t, action)

// We can use CreateAction for all cases as UpdateAction is the very same interface.
actual := action.(clienttesting.CreateAction).GetObject().(*corev1.Secret)
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 {
Copy link
Contributor

@p0lyn0mial p0lyn0mial Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

t.Error(actual.Data)
}

expectedAnnotationKeys := sets.New[string](
"auth.openshift.io/certificate-not-after",
"auth.openshift.io/certificate-not-before",
"auth.openshift.io/certificate-issuer",
"certificates.openshift.io/refresh-period",
"openshift.io/owning-component",
)
Copy link
Contributor Author

@tchap tchap Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if keys := sets.KeySet(actual.Annotations); !keys.Equal(expectedAnnotationKeys) {
t.Errorf("Annotation keys don't match:\n%s", cmp.Diff(expectedAnnotationKeys, keys))
}

ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]
if !found {
t.Errorf("expected secret to have ownership annotations, got: %v", actual.Annotations)
}
if ownershipValue != "test" {
t.Errorf("expected ownership annotation to be 'test', got: %v", ownershipValue)
}
if len(actual.OwnerReferences) != 1 {
t.Errorf("expected to have exactly one owner reference")
}
if actual.OwnerReferences[0].Name != "operator" {
t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
}
}
}

verifyActionOnCreated := newVerifyActionFunc(func(t *testing.T, action clienttesting.Action) {
t.Helper()
if !action.Matches("create", "secrets") {
t.Fatalf("Expected a Create action, got %s", spew.Sdump(action))
}
Copy link
Contributor

@p0lyn0mial p0lyn0mial Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})

verifyActionOnUpdated := newVerifyActionFunc(func(t *testing.T, action clienttesting.Action) {
t.Helper()
if !action.Matches("update", "secrets") {
t.Fatalf("Expected an Update action, got %s", spew.Sdump(action))
}
Copy link
Contributor

@p0lyn0mial p0lyn0mial Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here.

})

tests := []struct {
name string

initialSecret *corev1.Secret
RefreshOnlyWhenExpired bool

verifyActions func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool)
expectedError string
verifyAction func(t *testing.T, action clienttesting.Action)
expectedError string
expectedEvents []*corev1.Event
}{
{
name: "initial create",
RefreshOnlyWhenExpired: false,
verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
t.Helper()
actions := client.Actions()
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !controllerUpdatedSecret {
t.Errorf("expected controller to update secret")
}

if !actions[0].Matches("create", "secrets") {
t.Error(actions[0])
}

actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.Secret)
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 {
t.Error(actual.Data)
}
if len(actual.Annotations) == 0 {
t.Errorf("expected certificates to be annotated")
}
ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]
if !found {
t.Errorf("expected secret to have ownership annotations, got: %v", actual.Annotations)
}
if ownershipValue != "test" {
t.Errorf("expected ownership annotation to be 'test', got: %v", ownershipValue)
}
if len(actual.OwnerReferences) != 1 {
t.Errorf("expected to have exactly one owner reference")
}
if actual.OwnerReferences[0].Name != "operator" {
t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
}
verifyAction: verifyActionOnCreated,
expectedEvents: []*corev1.Event{
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: secret doesn't exist`},
{Reason: "SecretCreated", Message: `Created Secret/signer -n ns because it was missing`},
},
},
{
Expand All @@ -81,40 +105,48 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
},
RefreshOnlyWhenExpired: false,
verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
t.Helper()
actions := client.Actions()
if len(actions) != 1 {
t.Fatal(spew.Sdump(actions))
}

if !actions[0].Matches("update", "secrets") {
t.Error(actions[0])
}
if !controllerUpdatedSecret {
t.Errorf("expected controller to update secret")
}

actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.Secret)
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 {
t.Error(actual.Data)
}
ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]
if !found {
t.Errorf("expected secret to have ownership annotations, got: %v", actual.Annotations)
}
if ownershipValue != "test" {
t.Errorf("expected ownership annotation to be 'test', got: %v", ownershipValue)
}
if len(actual.OwnerReferences) != 1 {
t.Errorf("expected to have exactly one owner reference")
}
if actual.OwnerReferences[0].Name != "operator" {
t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
}
verifyAction: verifyActionOnUpdated,
expectedEvents: []*corev1.Event{
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notAfter`},
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
},
},
{
name: "update on missing notAfter annotation",
initialSecret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10",
Annotations: map[string]string{
"auth.openshift.io/certificate-not-before": "2108-09-08T20:47:31-07:00",
annotations.OpenShiftComponent: "test",
},
},
Type: corev1.SecretTypeTLS,
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
},
RefreshOnlyWhenExpired: false,
verifyAction: verifyActionOnUpdated,
expectedEvents: []*corev1.Event{
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notAfter`},
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
},
},
{
name: "update on missing notBefore annotation",
initialSecret: &corev1.Secret{
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10",
Annotations: map[string]string{
"auth.openshift.io/certificate-not-after": "2108-09-08T22:47:31-07:00",
annotations.OpenShiftComponent: "test",
},
},
Type: corev1.SecretTypeTLS,
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
},
RefreshOnlyWhenExpired: false,
verifyAction: verifyActionOnUpdated,
expectedEvents: []*corev1.Event{
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notBefore`},
Copy link
Contributor Author

@tchap tchap Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@p0lyn0mial p0lyn0mial Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
},
},
{
Expand All @@ -135,14 +167,7 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
},
RefreshOnlyWhenExpired: false,
verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
t.Helper()
actions := client.Actions()
if len(actions) != 0 {
t.Fatal(spew.Sdump(actions))
}
},
expectedError: "certFile missing", // this means we tried to read the cert from the existing secret. If we created one, we fail in the client check
expectedError: "certFile missing", // this means we tried to read the cert from the existing secret. If we created one, we fail in the client check
},
{
name: "update with RefreshOnlyWhenExpired set",
Expand All @@ -160,35 +185,30 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
},
RefreshOnlyWhenExpired: true,
verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
t.Helper()
actions := client.Actions()
if len(actions) != 0 {
t.Fatal(spew.Sdump(actions))
}
},
expectedError: "certFile missing", // this means we tried to read the cert from the existing secret. If we created one, we fail in the client check
expectedError: "certFile missing", // this means we tried to read the cert from the existing secret. If we created one, we fail in the client check
},
}

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
indexer := cache.NewIndexer(cache.MetaNamespaceKeyFunc, cache.Indexers{cache.NamespaceIndex: cache.MetaNamespaceIndexFunc})

client := kubefake.NewSimpleClientset()
client := kubefake.NewClientset()
if test.initialSecret != nil {
indexer.Add(test.initialSecret)
client = kubefake.NewSimpleClientset(test.initialSecret)
client = kubefake.NewClientset(test.initialSecret)
}

recorder := events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now()))

c := &RotatedSigningCASecret{
Namespace: "ns",
Name: "signer",
Validity: 24 * time.Hour,
Refresh: 12 * time.Hour,
Client: client.CoreV1(),
Lister: corev1listers.NewSecretLister(indexer),
EventRecorder: events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now())),
EventRecorder: recorder,
AdditionalAnnotations: AdditionalAnnotations{
JiraComponent: "test",
},
Expand All @@ -208,7 +228,41 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
t.Errorf("missing %q", test.expectedError)
}

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

@p0lyn0mial p0lyn0mial Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

t.Fatalf("Expected no action, got %d:\n%s", len(actions), spew.Sdump(actions))
}
} else {
if !updated {
t.Errorf("Expected controller to update secret")
}

if len(actions) > 1 {
t.Fatalf("Expected 1 action, got %d:\n%s", len(actions), spew.Sdump(actions))
} else {
test.verifyAction(t, actions[0])
}
}

if events := pruneEventFieldsForComparison(recorder.Events()); !cmp.Equal(events, test.expectedEvents) {
Copy link
Contributor

@p0lyn0mial p0lyn0mial Nov 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@p0lyn0mial p0lyn0mial Nov 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

t.Errorf("Events mismatch (-want +got):\n%s", cmp.Diff(test.expectedEvents, events))
}
})
}
}

func pruneEventFieldsForComparison(events []*corev1.Event) []*corev1.Event {
if len(events) == 0 {
return nil
}

out := make([]*corev1.Event, len(events))
for i, event := range events {
out[i] = &corev1.Event{
Reason: event.Reason,
Message: event.Message,
}
}
return out
}