Skip to content

Commit 247b85e

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 247b85e

File tree

2 files changed

+152
-93
lines changed

2 files changed

+152
-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: 151 additions & 92 deletions
Original file line numberDiff line numberDiff line change
@@ -2,75 +2,117 @@ 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+
verifyActionFunc := func(check func(t *testing.T, action clienttesting.Action)) func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
27+
return func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
28+
t.Helper()
29+
actions := client.Actions()
30+
if len(actions) != 1 {
31+
t.Fatal(spew.Sdump(actions))
32+
}
33+
34+
if !controllerUpdatedSecret {
35+
t.Errorf("expected controller to update secret")
36+
}
37+
38+
if check != nil {
39+
check(t, actions[0])
40+
}
41+
42+
actual := actions[0].(clienttesting.CreateAction).GetObject().(*corev1.Secret)
43+
if certType, _ := CertificateTypeFromObject(actual); certType != CertificateTypeSigner {
44+
t.Errorf("expected certificate type 'signer', got: %v", certType)
45+
}
46+
if len(actual.Data["tls.crt"]) == 0 || len(actual.Data["tls.key"]) == 0 {
47+
t.Error(actual.Data)
48+
}
49+
50+
expectedAnnotationKeys := sets.New[string](
51+
"auth.openshift.io/certificate-not-after",
52+
"auth.openshift.io/certificate-not-before",
53+
"auth.openshift.io/certificate-issuer",
54+
"certificates.openshift.io/refresh-period",
55+
"openshift.io/owning-component",
56+
)
57+
if keys := sets.KeySet(actual.Annotations); !keys.Equal(expectedAnnotationKeys) {
58+
t.Errorf("Annotation keys don't match:\n%s", cmp.Diff(expectedAnnotationKeys, keys))
59+
}
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+
}
74+
}
75+
}
76+
77+
verifyActionsOnCreated := verifyActionFunc(func(t *testing.T, action clienttesting.Action) {
78+
t.Helper()
79+
if !action.Matches("create", "secrets") {
80+
t.Error(action)
81+
}
82+
})
83+
84+
verifyActionsOnUpdated := verifyActionFunc(func(t *testing.T, action clienttesting.Action) {
85+
t.Helper()
86+
if !action.Matches("update", "secrets") {
87+
t.Error(action)
88+
}
89+
})
90+
91+
verifyActionsEmpty := func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool) {
92+
t.Helper()
93+
actions := client.Actions()
94+
if len(actions) != 0 {
95+
t.Fatal(spew.Sdump(actions))
96+
}
97+
}
98+
2499
tests := []struct {
25100
name string
26101

27102
initialSecret *corev1.Secret
28103
RefreshOnlyWhenExpired bool
29104

30-
verifyActions func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool)
31-
expectedError string
105+
verifyActions func(t *testing.T, client *kubefake.Clientset, controllerUpdatedSecret bool)
106+
expectedError string
107+
expectedEvents []*corev1.Event
32108
}{
33109
{
34110
name: "initial create",
35111
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-
}
112+
verifyActions: verifyActionsOnCreated,
113+
expectedEvents: []*corev1.Event{
114+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: secret doesn't exist`},
115+
{Reason: "SecretCreated", Message: `Created Secret/signer -n ns because it was missing`},
74116
},
75117
},
76118
{
@@ -81,40 +123,48 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
81123
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
82124
},
83125
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-
}
126+
verifyActions: verifyActionsOnUpdated,
127+
expectedEvents: []*corev1.Event{
128+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notAfter`},
129+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
130+
},
131+
},
132+
{
133+
name: "update on missing notAfter annotation",
134+
initialSecret: &corev1.Secret{
135+
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10",
136+
Annotations: map[string]string{
137+
"auth.openshift.io/certificate-not-before": "2108-09-08T20:47:31-07:00",
138+
annotations.OpenShiftComponent: "test",
139+
},
140+
},
141+
Type: corev1.SecretTypeTLS,
142+
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
143+
},
144+
RefreshOnlyWhenExpired: false,
145+
verifyActions: verifyActionsOnUpdated,
146+
expectedEvents: []*corev1.Event{
147+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notAfter`},
148+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
149+
},
150+
},
151+
{
152+
name: "update on missing notBefore annotation",
153+
initialSecret: &corev1.Secret{
154+
ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "signer", ResourceVersion: "10",
155+
Annotations: map[string]string{
156+
"auth.openshift.io/certificate-not-after": "2108-09-08T22:47:31-07:00",
157+
annotations.OpenShiftComponent: "test",
158+
},
159+
},
160+
Type: corev1.SecretTypeTLS,
161+
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
162+
},
163+
RefreshOnlyWhenExpired: false,
164+
verifyActions: verifyActionsOnUpdated,
165+
expectedEvents: []*corev1.Event{
166+
{Reason: "SignerUpdateRequired", Message: `"signer" in "ns" requires a new signing cert/key pair: missing notBefore`},
167+
{Reason: "SecretUpdated", Message: `Updated Secret/signer -n ns because it changed`},
118168
},
119169
},
120170
{
@@ -135,14 +185,8 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
135185
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
136186
},
137187
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
188+
verifyActions: verifyActionsEmpty,
189+
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
146190
},
147191
{
148192
name: "update with RefreshOnlyWhenExpired set",
@@ -160,14 +204,8 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
160204
Data: map[string][]byte{"tls.crt": {}, "tls.key": {}},
161205
},
162206
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
207+
verifyActions: verifyActionsEmpty,
208+
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
171209
},
172210
}
173211

@@ -181,14 +219,16 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
181219
client = kubefake.NewSimpleClientset(test.initialSecret)
182220
}
183221

222+
recorder := events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now()))
223+
184224
c := &RotatedSigningCASecret{
185225
Namespace: "ns",
186226
Name: "signer",
187227
Validity: 24 * time.Hour,
188228
Refresh: 12 * time.Hour,
189229
Client: client.CoreV1(),
190230
Lister: corev1listers.NewSecretLister(indexer),
191-
EventRecorder: events.NewInMemoryRecorder("test", clocktesting.NewFakePassiveClock(time.Now())),
231+
EventRecorder: recorder,
192232
AdditionalAnnotations: AdditionalAnnotations{
193233
JiraComponent: "test",
194234
},
@@ -209,6 +249,25 @@ func TestEnsureSigningCertKeyPair(t *testing.T) {
209249
}
210250

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

0 commit comments

Comments
 (0)