Skip to content

Commit 84b8a5b

Browse files
tchaptchap
committed
certrotation: Fix using a wrong annotation value
This is actually not affecting the logic, but a wrong error is returned. Improved the unit test to check events and annotation keys.
1 parent 14a789e commit 84b8a5b

File tree

2 files changed

+153
-93
lines changed

2 files changed

+153
-93
lines changed

‎pkg/operator/certrotation/signer.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -188,7 +188,7 @@ func getValidityFromAnnotations(annotations map[string]string) (notBefore time.T
188188
return notBefore, notAfter, fmt.Sprintf("bad expiry: %q", notAfterString)
189189
}
190190
notBeforeString := annotations[CertificateNotBeforeAnnotation]
191-
if len(notAfterString) == 0 {
191+
if len(notBeforeString) == 0 {
192192
return notBefore, notAfter, "missing notBefore"
193193
}
194194
notBefore, err = time.Parse(time.RFC3339, notBeforeString)

‎pkg/operator/certrotation/signer_test.go‎

Lines changed: 152 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,14 @@ package certrotation
22

33
import (
44
"context"
5-
clocktesting "k8s.io/utils/clock/testing"
65
"strings"
76
"testing"
87
"time"
98

9+
"github.com/google/go-cmp/cmp"
10+
"k8s.io/apimachinery/pkg/util/sets"
11+
clocktesting "k8s.io/utils/clock/testing"
12+
1013
"github.com/davecgh/go-spew/spew"
1114

1215
corev1 "k8s.io/api/core/v1"
@@ -21,56 +24,96 @@ import (
2124
)
2225

2326
func TestEnsureSigningCertKeyPair(t *testing.T) {
27+
verifyActionFunc := func(check func(t *testing.T, action clienttesting.Action)) func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
28+
return func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
29+
t.Helper()
30+
actions := client.Actions()
31+
if len(actions) != 1 {
32+
t.Fatal(spew.Sdump(actions))
33+
}
34+
35+
if !controllerUpdatedSecret {
36+
t.Errorf("expected controller to update secret")
37+
}
38+
39+
if check != nil {
40+
check(t, actions[0])
41+
}
42+
43+
actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.Secret)
44+
if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeSigner {
45+
t.Errorf("expected certificate type 'signer', got: %v", certType)
46+
}
47+
if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 {
48+
t.Error(actual.Data)
49+
}
50+
51+
expectedAnnotationKeys := sets.New[string](
52+
"auth.openshift.io/certificate-not-after",
53+
"auth.openshift.io/certificate-not-before",
54+
"auth.openshift.io/certificate-issuer",
55+
"certificates.openshift.io/refresh-period",
56+
"openshift.io/owning-component",
57+
)
58+
if keys := sets.KeySet(actual.Annotations); !keys.Equal(expectedAnnotationKeys) {
59+
t.Errorf("Annotation keys don't match:\n%s", cmp.Diff(expectedAnnotationKeys, keys))
60+
}
61+
62+
ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]
63+
if !found {
64+
t.Errorf("expected secret to have ownership annotations, got: %v", actual.Annotations)
65+
}
66+
if ownershipValue != "test" {
67+
t.Errorf("expected ownership annotation to be 'test', got: %v", ownershipValue)
68+
}
69+
if len(actual.OwnerReferences) != 1 {
70+
t.Errorf("expected to have exactly one owner reference")
71+
}
72+
if actual.OwnerReferences[0].Name != "operator" {
73+
t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
74+
}
75+
}
76+
}
77+
78+
verifyActionsOnCreated := verifyActionFunc(func(t *testing.T, action clienttesting.Action) {
79+
t.Helper()
80+
if !action.Matches("create", "secrets") {
81+
t.Error(action)
82+
}
83+
})
84+
85+
verifyActionsOnUpdated := verifyActionFunc(func(t *testing.T, action clienttesting.Action) {
86+
t.Helper()
87+
if !action.Matches("update", "secrets") {
88+
t.Error(action)
89+
}
90+
})
91+
92+
verifyActionsEmpty := func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
93+
t.Helper()
94+
actions := client.Actions()
95+
if len(actions) != 0 {
96+
t.Fatal(spew.Sdump(actions))
97+
}
98+
}
99+
24100
tests := []struct {
25101
name string
26102

27103
initialSecret *corev1.Secret
28104
RefreshOnlyWhenExpired bool
29105

30-
verifyActions func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool)
31-
expectedError string
106+
verifyActions func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool)
107+
expectedError string
108+
expectedEvents []*corev1.Event
32109
}{
33110
{
34111
name: "initial create",
35112
RefreshOnlyWhenExpired: false,
36-
verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
37-
t.Helper()
38-
actions := client.Actions()
39-
if len(actions) != 1 {
40-
t.Fatal(spew.Sdump(actions))
41-
}
42-
43-
if !controllerUpdatedSecret {
44-
t.Errorf("expected controller to update secret")
45-
}
46-
47-
if !actions[0].Matches("create", "secrets") {
48-
t.Error(actions[0])
49-
}
50-
51-
actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.Secret)
52-
if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeSigner {
53-
t.Errorf("expected certificate type 'signer', got: %v", certType)
54-
}
55-
if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 {
56-
t.Error(actual.Data)
57-
}
58-
if len(actual.Annotations) == 0 {
59-
t.Errorf("expected certificates to be annotated")
60-
}
61-
ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]
62-
if !found {
63-
t.Errorf("expected secret to have ownership annotations, got: %v", actual.Annotations)
64-
}
65-
if ownershipValue != "test" {
66-
t.Errorf("expected ownership annotation to be 'test', got: %v", ownershipValue)
67-
}
68-
if len(actual.OwnerReferences) != 1 {
69-
t.Errorf("expected to have exactly one owner reference")
70-
}
71-
if actual.OwnerReferences[0].Name != "operator" {
72-
t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
73-
}
113+
verifyActions: verifyActionsOnCreated,
114+
expectedEvents: []*corev1.Event{
115+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: secret doesn't exist`},
116+
{Reason: "SecretCreated", Message: `Created Secret/signer -n ns because it was missing`},
74117
},
75118
},
76119
{
@@ -81,40 +124,48 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
81124
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
82125
},
83126
RefreshOnlyWhenExpired: false,
84-
verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
85-
t.Helper()
86-
actions := client.Actions()
87-
if len(actions) != 1 {
88-
t.Fatal(spew.Sdump(actions))
89-
}
90-
91-
if !actions[0].Matches("update", "secrets") {
92-
t.Error(actions[0])
93-
}
94-
if !controllerUpdatedSecret {
95-
t.Errorf("expected controller to update secret")
96-
}
97-
98-
actual := actions[0].(clienttesting.UpdateAction).GetObject().(*corev1.Secret)
99-
if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeSigner {
100-
t.Errorf("expected certificate type 'signer', got: %v", certType)
101-
}
102-
if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 {
103-
t.Error(actual.Data)
104-
}
105-
ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]
106-
if !found {
107-
t.Errorf("expected secret to have ownership annotations, got: %v", actual.Annotations)
108-
}
109-
if ownershipValue != "test" {
110-
t.Errorf("expected ownership annotation to be 'test', got: %v", ownershipValue)
111-
}
112-
if len(actual.OwnerReferences) != 1 {
113-
t.Errorf("expected to have exactly one owner reference")
114-
}
115-
if actual.OwnerReferences[0].Name != "operator" {
116-
t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
117-
}
127+
verifyActions: verifyActionsOnUpdated,
128+
expectedEvents: []*corev1.Event{
129+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notAfter`},
130+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
131+
},
132+
},
133+
{
134+
name: "update on missing notAfter annotation",
135+
initialSecret: &corev1.Secret{
136+
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10",
137+
Annotations: map[string]string{
138+
"auth.openshift.io/certificate-not-before": "2108-09-08T20:47:31-07:00",
139+
annotations.OpenShiftComponent: "test",
140+
},
141+
},
142+
Type: corev1.SecretTypeTLS,
143+
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
144+
},
145+
RefreshOnlyWhenExpired: false,
146+
verifyActions: verifyActionsOnUpdated,
147+
expectedEvents: []*corev1.Event{
148+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notAfter`},
149+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
150+
},
151+
},
152+
{
153+
name: "update on missing notBefore annotation",
154+
initialSecret: &corev1.Secret{
155+
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10",
156+
Annotations: map[string]string{
157+
"auth.openshift.io/certificate-not-after": "2108-09-08T22:47:31-07:00",
158+
annotations.OpenShiftComponent: "test",
159+
},
160+
},
161+
Type: corev1.SecretTypeTLS,
162+
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
163+
},
164+
RefreshOnlyWhenExpired: false,
165+
verifyActions: verifyActionsOnUpdated,
166+
expectedEvents: []*corev1.Event{
167+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notBefore`},
168+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
118169
},
119170
},
120171
{
@@ -135,14 +186,8 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
135186
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
136187
},
137188
RefreshOnlyWhenExpired: false,
138-
verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
139-
t.Helper()
140-
actions := client.Actions()
141-
if len(actions) != 0 {
142-
t.Fatal(spew.Sdump(actions))
143-
}
144-
},
145-
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
189+
verifyActions: verifyActionsEmpty,
190+
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
146191
},
147192
{
148193
name: "update with RefreshOnlyWhenExpired set",
@@ -160,14 +205,8 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
160205
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
161206
},
162207
RefreshOnlyWhenExpired: true,
163-
verifyActions: func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
164-
t.Helper()
165-
actions := client.Actions()
166-
if len(actions) != 0 {
167-
t.Fatal(spew.Sdump(actions))
168-
}
169-
},
170-
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
208+
verifyActions: verifyActionsEmpty,
209+
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
171210
},
172211
}
173212

@@ -181,14 +220,16 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
181220
client = kubefake.NewSimpleClientset(test.initialSecret)
182221
}
183222

223+
recorder := events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now()))
224+
184225
c := &RotatedSigningCASecret{
185226
Namespace: "ns",
186227
Name: "signer",
187228
Validity: 24 * time.Hour,
188229
Refresh: 12 * time.Hour,
189230
Client: client.CoreV1(),
190231
Lister: corev1listers.NewSecretLister(indexer),
191-
EventRecorder: events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now())),
232+
EventRecorder: recorder,
192233
AdditionalAnnotations: AdditionalAnnotations{
193234
JiraComponent: "test",
194235
},
@@ -209,6 +250,25 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
209250
}
210251

211252
test.verifyActions(t, client, updated)
253+
254+
if events := pruneEventFieldsForComparison(recorder.Events()); !cmp.Equal(events, test.expectedEvents) {
255+
t.Errorf("Events mismatch (-want +got):\n%s", cmp.Diff(test.expectedEvents, events))
256+
}
212257
})
213258
}
214259
}
260+
261+
func pruneEventFieldsForComparison(events []*corev1.Event) []*corev1.Event {
262+
if len(events) == 0 {
263+
return nil
264+
}
265+
266+
out := make([]*corev1.Event, len(events))
267+
for i, event := range events {
268+
out[i] = &corev1.Event{
269+
Reason: event.Reason,
270+
Message: event.Message,
271+
}
272+
}
273+
return out
274+
}

0 commit comments

Comments
 (0)