Skip to content

Commit 4d6d0c1

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 4d6d0c1

File tree

2 files changed

+150
-96
lines changed

2 files changed

+150
-96
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: 149 additions & 95 deletions
Original file line numberDiff line numberDiff line change
@@ -2,75 +2,99 @@ package certrotation
22

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

109
"github.com/davecgh/go-spew/spew"
10+
"github.com/google/go-cmp/cmp"
1111

1212
corev1 "k8s.io/api/core/v1"
1313
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
14+
"k8s.io/apimachinery/pkg/util/sets"
1415
kubefake "k8s.io/client-go/kubernetes/fake"
1516
corev1listers "k8s.io/client-go/listers/core/v1"
1617
clienttesting "k8s.io/client-go/testing"
1718
"k8s.io/client-go/tools/cache"
19+
clocktesting "k8s.io/utils/clock/testing"
1820

1921
"github.com/openshift/api/annotations"
2022
"github.com/openshift/library-go/pkg/operator/events"
2123
)
2224

2325
func TestEnsureSigningCertKeyPair(t *testing.T) {
26+
newVerifyActionFunc := func(check func(t *testing.T, action clienttesting.Action)) func(t *testing.T, action clienttesting.Action) {
27+
return func(t *testing.T, action clienttesting.Action) {
28+
t.Helper()
29+
check(t, action)
30+
31+
// We can use CreateAction for all cases as UpdateAction is the very same interface.
32+
actual := action.(clienttesting.CreateAction).GetObject().(*corev1.Secret)
33+
if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeSigner {
34+
t.Errorf("expected certificate type 'signer', got: %v", certType)
35+
}
36+
if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 {
37+
t.Error(actual.Data)
38+
}
39+
40+
expectedAnnotationKeys := sets.New[string](
41+
"auth.openshift.io/certificate-not-after",
42+
"auth.openshift.io/certificate-not-before",
43+
"auth.openshift.io/certificate-issuer",
44+
"certificates.openshift.io/refresh-period",
45+
"openshift.io/owning-component",
46+
)
47+
if keys := sets.KeySet(actual.Annotations); !keys.Equal(expectedAnnotationKeys) {
48+
t.Errorf("Annotation keys don't match:\n%s", cmp.Diff(expectedAnnotationKeys, keys))
49+
}
50+
51+
ownershipValue, found := actual.Annotations[annotations.OpenShiftComponent]
52+
if !found {
53+
t.Errorf("expected secret to have ownership annotations, got: %v", actual.Annotations)
54+
}
55+
if ownershipValue != "test" {
56+
t.Errorf("expected ownership annotation to be 'test', got: %v", ownershipValue)
57+
}
58+
if len(actual.OwnerReferences) != 1 {
59+
t.Errorf("expected to have exactly one owner reference")
60+
}
61+
if actual.OwnerReferences[0].Name != "operator" {
62+
t.Errorf("expected owner reference to be 'operator', got %v", actual.OwnerReferences[0].Name)
63+
}
64+
}
65+
}
66+
67+
verifyActionOnCreated := newVerifyActionFunc(func(t *testing.T, action clienttesting.Action) {
68+
t.Helper()
69+
if !action.Matches("create", "secrets") {
70+
t.Fatalf("Expected a Create action, got %s", spew.Sdump(action))
71+
}
72+
})
73+
74+
verifyActionOnUpdated := newVerifyActionFunc(func(t *testing.T, action clienttesting.Action) {
75+
t.Helper()
76+
if !action.Matches("update", "secrets") {
77+
t.Fatalf("Expected an Update action, got %s", spew.Sdump(action))
78+
}
79+
})
80+
2481
tests := []struct {
2582
name string
2683

2784
initialSecret *corev1.Secret
2885
RefreshOnlyWhenExpired bool
2986

30-
verifyActions func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool)
31-
expectedError string
87+
verifyAction func(t *testing.T, action clienttesting.Action)
88+
expectedError string
89+
expectedEvents []*corev1.Event
3290
}{
3391
{
3492
name: "initial create",
3593
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-
}
94+
verifyAction: verifyActionOnCreated,
95+
expectedEvents: []*corev1.Event{
96+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: secret doesn't exist`},
97+
{Reason: "SecretCreated", Message: `Created Secret/signer -n ns because it was missing`},
7498
},
7599
},
76100
{
@@ -81,40 +105,48 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
81105
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
82106
},
83107
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-
}
108+
verifyAction: verifyActionOnUpdated,
109+
expectedEvents: []*corev1.Event{
110+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notAfter`},
111+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
112+
},
113+
},
114+
{
115+
name: "update on missing notAfter annotation",
116+
initialSecret: &corev1.Secret{
117+
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10",
118+
Annotations: map[string]string{
119+
"auth.openshift.io/certificate-not-before": "2108-09-08T20:47:31-07:00",
120+
annotations.OpenShiftComponent: "test",
121+
},
122+
},
123+
Type: corev1.SecretTypeTLS,
124+
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
125+
},
126+
RefreshOnlyWhenExpired: false,
127+
verifyAction: verifyActionOnUpdated,
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 notBefore 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-after": "2108-09-08T22: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+
verifyAction: verifyActionOnUpdated,
147+
expectedEvents: []*corev1.Event{
148+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notBefore`},
149+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
118150
},
119151
},
120152
{
@@ -135,14 +167,7 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
135167
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
136168
},
137169
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
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
146171
},
147172
{
148173
name: "update with RefreshOnlyWhenExpired set",
@@ -160,35 +185,30 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
160185
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
161186
},
162187
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
188+
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
171189
},
172190
}
173191

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

178-
client := kubefake.NewSimpleClientset()
196+
client := kubefake.NewClientset()
179197
if test.initialSecret != nil {
180198
indexer.Add(test.initialSecret)
181-
client = kubefake.NewSimpleClientset(test.initialSecret)
199+
client = kubefake.NewClientset(test.initialSecret)
182200
}
183201

202+
recorder := events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now()))
203+
184204
c := &RotatedSigningCASecret{
185205
Namespace: "ns",
186206
Name: "signer",
187207
Validity: 24 * time.Hour,
188208
Refresh: 12 * time.Hour,
189209
Client: client.CoreV1(),
190210
Lister: corev1listers.NewSecretLister(indexer),
191-
EventRecorder: events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now())),
211+
EventRecorder: recorder,
192212
AdditionalAnnotations: AdditionalAnnotations{
193213
JiraComponent: "test",
194214
},
@@ -208,7 +228,41 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
208228
t.Errorf("missing %q", test.expectedError)
209229
}
210230

211-
test.verifyActions(t, client, updated)
231+
actions := client.Actions()
232+
if test.verifyAction == nil {
233+
if len(actions) > 0 {
234+
t.Fatalf("Expected no action, got %d:\n%s", len(actions), spew.Sdump(actions))
235+
}
236+
} else {
237+
if !updated {
238+
t.Errorf("Expected controller to update secret")
239+
}
240+
241+
if len(actions) > 1 {
242+
t.Fatalf("Expected 1 action, got %d:\n%s", len(actions), spew.Sdump(actions))
243+
} else {
244+
test.verifyAction(t, actions[0])
245+
}
246+
}
247+
248+
if events := pruneEventFieldsForComparison(recorder.Events()); !cmp.Equal(events, test.expectedEvents) {
249+
t.Errorf("Events mismatch (-want +got):\n%s", cmp.Diff(test.expectedEvents, events))
250+
}
212251
})
213252
}
214253
}
254+
255+
func pruneEventFieldsForComparison(events []*corev1.Event) []*corev1.Event {
256+
if len(events) == 0 {
257+
return nil
258+
}
259+
260+
out := make([]*corev1.Event, len(events))
261+
for i, event := range events {
262+
out[i] = &corev1.Event{
263+
Reason: event.Reason,
264+
Message: event.Message,
265+
}
266+
}
267+
return out
268+
}

0 commit comments

Comments
 (0)