Skip to content

Commit 919786a

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 919786a

File tree

2 files changed

+148
-94
lines changed

2 files changed

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

2788
initialSecret *corev1.Secret
2889
RefreshOnlyWhenExpired bool
2990

30-
verifyActions func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool)
31-
expectedError string
91+
verifyAction func(t *testing.T, action clienttesting.Action, controllerUpdatedSecret bool)
92+
expectedError string
93+
expectedEvents []*corev1.Event
3294
}{
3395
{
3496
name: "initial create",
3597
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-
}
98+
verifyAction: verifyActionOnCreated,
99+
expectedEvents: []*corev1.Event{
100+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: secret doesn't exist`},
101+
{Reason: "SecretCreated", Message: `Created Secret/signer -n ns because it was missing`},
74102
},
75103
},
76104
{
@@ -81,40 +109,48 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
81109
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
82110
},
83111
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-
}
112+
verifyAction: verifyActionOnUpdated,
113+
expectedEvents: []*corev1.Event{
114+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notAfter`},
115+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
116+
},
117+
},
118+
{
119+
name: "update on missing notAfter annotation",
120+
initialSecret: &corev1.Secret{
121+
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10",
122+
Annotations: map[string]string{
123+
"auth.openshift.io/certificate-not-before": "2108-09-08T20:47:31-07:00",
124+
annotations.OpenShiftComponent: "test",
125+
},
126+
},
127+
Type: corev1.SecretTypeTLS,
128+
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
129+
},
130+
RefreshOnlyWhenExpired: false,
131+
verifyAction: verifyActionOnUpdated,
132+
expectedEvents: []*corev1.Event{
133+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notAfter`},
134+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
135+
},
136+
},
137+
{
138+
name: "update on missing notBefore annotation",
139+
initialSecret: &corev1.Secret{
140+
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10",
141+
Annotations: map[string]string{
142+
"auth.openshift.io/certificate-not-after": "2108-09-08T22:47:31-07:00",
143+
annotations.OpenShiftComponent: "test",
144+
},
145+
},
146+
Type: corev1.SecretTypeTLS,
147+
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
148+
},
149+
RefreshOnlyWhenExpired: false,
150+
verifyAction: verifyActionOnUpdated,
151+
expectedEvents: []*corev1.Event{
152+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notBefore`},
153+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
118154
},
119155
},
120156
{
@@ -135,14 +171,7 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
135171
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
136172
},
137173
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
174+
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
146175
},
147176
{
148177
name: "update with RefreshOnlyWhenExpired set",
@@ -160,14 +189,7 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
160189
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
161190
},
162191
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
192+
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
171193
},
172194
}
173195

@@ -181,14 +203,16 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
181203
client = kubefake.NewSimpleClientset(test.initialSecret)
182204
}
183205

206+
recorder := events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now()))
207+
184208
c := &RotatedSigningCASecret{
185209
Namespace: "ns",
186210
Name: "signer",
187211
Validity: 24 * time.Hour,
188212
Refresh: 12 * time.Hour,
189213
Client: client.CoreV1(),
190214
Lister: corev1listers.NewSecretLister(indexer),
191-
EventRecorder: events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now())),
215+
EventRecorder: recorder,
192216
AdditionalAnnotations: AdditionalAnnotations{
193217
JiraComponent: "test",
194218
},
@@ -208,7 +232,37 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
208232
t.Errorf("missing %q", test.expectedError)
209233
}
210234

211-
test.verifyActions(t, client, updated)
235+
actions := client.Actions()
236+
if test.verifyAction == nil {
237+
if len(actions) > 0 {
238+
t.Fatalf("Expected no action, got %d:\n%s", len(actions), spew.Sdump(actions))
239+
}
240+
} else {
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], updated)
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)