Skip to content

Commit 891c955

Browse files
vrutkovsvrutkovs
committed
certrotation: update details specific for annotations and owners
This also removed type change, which no longer happens since 4.17
1 parent 45778d8 commit 891c955

File tree

5 files changed

+302
-50
lines changed

5 files changed

+302
-50
lines changed

‎pkg/operator/certrotation/cabundle.go‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,10 +69,8 @@ func (c CABundleConfigMap) EnsureConfigMapCABundle(ctx context.Context, signingC
6969
// run Update if metadata needs changing unless running in RefreshOnlyWhenExpired mode
7070
updateDetail := ""
7171
if !c.RefreshOnlyWhenExpired {
72-
updateRequired = ensureOwnerRefAndTLSAnnotationsForConfigMap(caBundleConfigMap, c.Owner, c.AdditionalAnnotations)
73-
if updateRequired {
74-
updateDetail = fmt.Sprintf("annotations set to %#v", c.AdditionalAnnotations)
75-
}
72+
updateDetail = ensureOwnerRefAndTLSAnnotationsForConfigMap(caBundleConfigMap, c.Owner, c.AdditionalAnnotations)
73+
updateRequired = len(updateDetail) > 0
7674
}
7775

7876
updatedCerts, err := manageCABundleConfigMap(caBundleConfigMap, signingCertKeyPair.Config.Certs[0])

‎pkg/operator/certrotation/metadata.go‎

Lines changed: 22 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,46 +1,40 @@
11
package certrotation
22

33
import (
4+
"fmt"
5+
46
corev1 "k8s.io/api/core/v1"
57
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
68
)
79

8-
func ensureOwnerRefAndTLSAnnotationsForSecret(secret *corev1.Secret, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) bool {
9-
needsMetadataUpdate := false
10+
func ensureOwnerRefAndTLSAnnotationsForSecret(secret *corev1.Secret, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) string {
11+
updateDetails := ""
1012
// no ownerReference set
11-
if owner != nil {
12-
needsMetadataUpdate = ensureOwnerReference(&secret.ObjectMeta, owner)
13+
if owner != nil && ensureOwnerReference(&secret.ObjectMeta, owner) {
14+
updateDetails = fmt.Sprintf("owner reference updated to %#v", owner)
1315
}
1416
// ownership annotations not set
15-
return additionalAnnotations.EnsureTLSMetadataUpdate(&secret.ObjectMeta) || needsMetadataUpdate
17+
if additionalAnnotations.EnsureTLSMetadataUpdate(&secret.ObjectMeta) {
18+
if len(updateDetails) > 0 {
19+
updateDetails += ", "
20+
}
21+
updateDetails += fmt.Sprintf("annotations set to %#v", additionalAnnotations)
22+
}
23+
return updateDetails
1624
}
1725

18-
func ensureOwnerRefAndTLSAnnotationsForConfigMap(configMap *corev1.ConfigMap, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) bool {
19-
needsMetadataUpdate := false
26+
func ensureOwnerRefAndTLSAnnotationsForConfigMap(configMap *corev1.ConfigMap, owner *metav1.OwnerReference, additionalAnnotations AdditionalAnnotations) string {
27+
updateDetails := ""
2028
// no ownerReference set
21-
if owner != nil {
22-
needsMetadataUpdate = ensureOwnerReference(&configMap.ObjectMeta, owner)
29+
if owner != nil && ensureOwnerReference(&configMap.ObjectMeta, owner) {
30+
updateDetails = fmt.Sprintf("owner reference updated to %#v", owner)
2331
}
2432
// ownership annotations not set
25-
return additionalAnnotations.EnsureTLSMetadataUpdate(&configMap.ObjectMeta) || needsMetadataUpdate
26-
}
27-
28-
func ensureSecretTLSTypeSet(secret *corev1.Secret) bool {
29-
// Existing secret not found - no need to update metadata (will be done by needNewSigningCertKeyPair / NeedNewTargetCertKeyPair)
30-
if len(secret.ResourceVersion) == 0 {
31-
return false
32-
}
33-
34-
// convert outdated secret type (created by pre 4.7 installer)
35-
if secret.Type != corev1.SecretTypeTLS {
36-
secret.Type = corev1.SecretTypeTLS
37-
// wipe secret contents if tls.crt and tls.key are missing
38-
_, certExists := secret.Data[corev1.TLSCertKey]
39-
_, keyExists := secret.Data[corev1.TLSPrivateKeyKey]
40-
if !certExists || !keyExists {
41-
secret.Data = map[string][]byte{}
33+
if additionalAnnotations.EnsureTLSMetadataUpdate(&configMap.ObjectMeta) {
34+
if len(updateDetails) > 0 {
35+
updateDetails += ", "
4236
}
43-
return true
37+
updateDetails += fmt.Sprintf("annotations set to %#v", additionalAnnotations)
4438
}
45-
return false
39+
return updateDetails
4640
}

‎pkg/operator/certrotation/metadata_test.go‎

Lines changed: 274 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,274 @@
1+
package certrotation
2+
3+
import (
4+
"testing"
5+
6+
"github.com/openshift/api/annotations"
7+
"github.com/stretchr/testify/require"
8+
corev1 "k8s.io/api/core/v1"
9+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
10+
)
11+
12+
func TestEnsureOwnerRefAndTLSAnnotationsForSecret(t *testing.T) {
13+
testOwner := &metav1.OwnerReference{
14+
APIVersion: "apps/v1",
15+
Kind: "Deployment",
16+
Name: "test-deployment",
17+
UID: "test-uid",
18+
}
19+
20+
testAnnotations := AdditionalAnnotations{
21+
JiraComponent: "test-component",
22+
Description: "test description",
23+
TestName: "test-name",
24+
}
25+
26+
tests := []struct {
27+
name string
28+
secret *corev1.Secret
29+
owner *metav1.OwnerReference
30+
additionalAnnotations AdditionalAnnotations
31+
expectedUpdateDetails string
32+
expectedOwnerReferences []metav1.OwnerReference
33+
expectedAnnotations map[string]string
34+
}{
35+
{
36+
name: "no updates needed - already has owner and annotations",
37+
secret: &corev1.Secret{
38+
ObjectMeta: metav1.ObjectMeta{
39+
Name: "test-secret",
40+
Namespace: "test-namespace",
41+
OwnerReferences: []metav1.OwnerReference{
42+
*testOwner,
43+
},
44+
Annotations: map[string]string{
45+
annotations.OpenShiftComponent: "test-component",
46+
annotations.OpenShiftDescription: "test description",
47+
CertificateTestNameAnnotation: "test-name",
48+
},
49+
},
50+
},
51+
owner: testOwner,
52+
additionalAnnotations: testAnnotations,
53+
expectedUpdateDetails: "",
54+
expectedOwnerReferences: []metav1.OwnerReference{
55+
*testOwner,
56+
},
57+
expectedAnnotations: map[string]string{
58+
annotations.OpenShiftComponent: "test-component",
59+
annotations.OpenShiftDescription: "test description",
60+
CertificateTestNameAnnotation: "test-name",
61+
},
62+
},
63+
{
64+
name: "only owner reference needs updating",
65+
secret: &corev1.Secret{
66+
ObjectMeta: metav1.ObjectMeta{
67+
Name: "test-secret",
68+
Namespace: "test-namespace",
69+
Annotations: map[string]string{
70+
annotations.OpenShiftComponent: "test-component",
71+
annotations.OpenShiftDescription: "test description",
72+
CertificateTestNameAnnotation: "test-name",
73+
},
74+
},
75+
},
76+
owner: testOwner,
77+
additionalAnnotations: testAnnotations,
78+
expectedUpdateDetails: "owner reference updated to &v1.OwnerReference{APIVersion:\"apps/v1\", Kind:\"Deployment\", Name:\"test-deployment\", UID:\"test-uid\", Controller:(*bool)(nil), BlockOwnerDeletion:(*bool)(nil)}",
79+
expectedOwnerReferences: []metav1.OwnerReference{
80+
*testOwner,
81+
},
82+
expectedAnnotations: map[string]string{
83+
annotations.OpenShiftComponent: "test-component",
84+
annotations.OpenShiftDescription: "test description",
85+
CertificateTestNameAnnotation: "test-name",
86+
},
87+
},
88+
{
89+
name: "only annotations need updating",
90+
secret: &corev1.Secret{
91+
ObjectMeta: metav1.ObjectMeta{
92+
Name: "test-secret",
93+
Namespace: "test-namespace",
94+
OwnerReferences: []metav1.OwnerReference{
95+
*testOwner,
96+
},
97+
},
98+
},
99+
owner: testOwner,
100+
additionalAnnotations: testAnnotations,
101+
expectedUpdateDetails: "annotations set to certrotation.AdditionalAnnotations{JiraComponent:\"test-component\", Description:\"test description\", TestName:\"test-name\", AutoRegenerateAfterOfflineExpiry:\"\", NotBefore:\"\", NotAfter:\"\", RefreshPeriod:\"\"}",
102+
expectedOwnerReferences: []metav1.OwnerReference{
103+
*testOwner,
104+
},
105+
expectedAnnotations: map[string]string{
106+
annotations.OpenShiftComponent: "test-component",
107+
annotations.OpenShiftDescription: "test description",
108+
CertificateTestNameAnnotation: "test-name",
109+
},
110+
},
111+
{
112+
name: "both owner reference and annotations need updating",
113+
secret: &corev1.Secret{
114+
ObjectMeta: metav1.ObjectMeta{
115+
Name: "test-secret",
116+
Namespace: "test-namespace",
117+
},
118+
},
119+
owner: testOwner,
120+
additionalAnnotations: testAnnotations,
121+
expectedUpdateDetails: "owner reference updated to &v1.OwnerReference{APIVersion:\"apps/v1\", Kind:\"Deployment\", Name:\"test-deployment\", UID:\"test-uid\", Controller:(*bool)(nil), BlockOwnerDeletion:(*bool)(nil)}, annotations set to certrotation.AdditionalAnnotations{JiraComponent:\"test-component\", Description:\"test description\", TestName:\"test-name\", AutoRegenerateAfterOfflineExpiry:\"\", NotBefore:\"\", NotAfter:\"\", RefreshPeriod:\"\"}",
122+
expectedOwnerReferences: []metav1.OwnerReference{
123+
*testOwner,
124+
},
125+
expectedAnnotations: map[string]string{
126+
annotations.OpenShiftComponent: "test-component",
127+
annotations.OpenShiftDescription: "test description",
128+
CertificateTestNameAnnotation: "test-name",
129+
},
130+
},
131+
}
132+
133+
for _, tt := range tests {
134+
t.Run(tt.name, func(t *testing.T) {
135+
result := ensureOwnerRefAndTLSAnnotationsForSecret(tt.secret, tt.owner, tt.additionalAnnotations)
136+
137+
require.Equal(t, result, tt.expectedUpdateDetails, "expected update detail")
138+
require.Equal(t, tt.expectedOwnerReferences, tt.secret.OwnerReferences, "expected owner references")
139+
require.Equal(t, tt.expectedAnnotations, tt.secret.Annotations, "expected owner annotations")
140+
})
141+
}
142+
}
143+
144+
func TestEnsureOwnerRefAndTLSAnnotationsForConfigMap(t *testing.T) {
145+
testOwner := &metav1.OwnerReference{
146+
APIVersion: "apps/v1",
147+
Kind: "Deployment",
148+
Name: "test-deployment",
149+
UID: "test-uid",
150+
}
151+
152+
testAnnotations := AdditionalAnnotations{
153+
JiraComponent: "test-component",
154+
Description: "test description",
155+
TestName: "test-name",
156+
}
157+
158+
tests := []struct {
159+
name string
160+
configMap *corev1.ConfigMap
161+
owner *metav1.OwnerReference
162+
additionalAnnotations AdditionalAnnotations
163+
expectedUpdateDetails string
164+
expectedOwnerReferences []metav1.OwnerReference
165+
expectedAnnotations map[string]string
166+
}{
167+
{
168+
name: "no updates needed - already has owner and annotations",
169+
configMap: &corev1.ConfigMap{
170+
ObjectMeta: metav1.ObjectMeta{
171+
Name: "test-configmap",
172+
Namespace: "test-namespace",
173+
OwnerReferences: []metav1.OwnerReference{
174+
*testOwner,
175+
},
176+
Annotations: map[string]string{
177+
annotations.OpenShiftComponent: "test-component",
178+
annotations.OpenShiftDescription: "test description",
179+
CertificateTestNameAnnotation: "test-name",
180+
},
181+
},
182+
},
183+
owner: testOwner,
184+
additionalAnnotations: testAnnotations,
185+
expectedUpdateDetails: "",
186+
expectedOwnerReferences: []metav1.OwnerReference{
187+
*testOwner,
188+
},
189+
expectedAnnotations: map[string]string{
190+
annotations.OpenShiftComponent: "test-component",
191+
annotations.OpenShiftDescription: "test description",
192+
CertificateTestNameAnnotation: "test-name",
193+
},
194+
},
195+
{
196+
name: "only owner reference needs updating",
197+
configMap: &corev1.ConfigMap{
198+
ObjectMeta: metav1.ObjectMeta{
199+
Name: "test-configmap",
200+
Namespace: "test-namespace",
201+
Annotations: map[string]string{
202+
annotations.OpenShiftComponent: "test-component",
203+
annotations.OpenShiftDescription: "test description",
204+
CertificateTestNameAnnotation: "test-name",
205+
},
206+
},
207+
},
208+
owner: testOwner,
209+
additionalAnnotations: testAnnotations,
210+
expectedUpdateDetails: "owner reference updated to &v1.OwnerReference{APIVersion:\"apps/v1\", Kind:\"Deployment\", Name:\"test-deployment\", UID:\"test-uid\", Controller:(*bool)(nil), BlockOwnerDeletion:(*bool)(nil)}",
211+
expectedOwnerReferences: []metav1.OwnerReference{
212+
*testOwner,
213+
},
214+
expectedAnnotations: map[string]string{
215+
annotations.OpenShiftComponent: "test-component",
216+
annotations.OpenShiftDescription: "test description",
217+
CertificateTestNameAnnotation: "test-name",
218+
},
219+
},
220+
{
221+
name: "only annotations need updating",
222+
configMap: &corev1.ConfigMap{
223+
ObjectMeta: metav1.ObjectMeta{
224+
Name: "test-configmap",
225+
Namespace: "test-namespace",
226+
OwnerReferences: []metav1.OwnerReference{
227+
*testOwner,
228+
},
229+
},
230+
},
231+
owner: testOwner,
232+
additionalAnnotations: testAnnotations,
233+
expectedUpdateDetails: "annotations set to certrotation.AdditionalAnnotations{JiraComponent:\"test-component\", Description:\"test description\", TestName:\"test-name\", AutoRegenerateAfterOfflineExpiry:\"\", NotBefore:\"\", NotAfter:\"\", RefreshPeriod:\"\"}",
234+
expectedOwnerReferences: []metav1.OwnerReference{
235+
*testOwner,
236+
},
237+
expectedAnnotations: map[string]string{
238+
annotations.OpenShiftComponent: "test-component",
239+
annotations.OpenShiftDescription: "test description",
240+
CertificateTestNameAnnotation: "test-name",
241+
},
242+
},
243+
{
244+
name: "both owner reference and annotations need updating",
245+
configMap: &corev1.ConfigMap{
246+
ObjectMeta: metav1.ObjectMeta{
247+
Name: "test-configmap",
248+
Namespace: "test-namespace",
249+
},
250+
},
251+
owner: testOwner,
252+
additionalAnnotations: testAnnotations,
253+
expectedUpdateDetails: "owner reference updated to &v1.OwnerReference{APIVersion:\"apps/v1\", Kind:\"Deployment\", Name:\"test-deployment\", UID:\"test-uid\", Controller:(*bool)(nil), BlockOwnerDeletion:(*bool)(nil)}, annotations set to certrotation.AdditionalAnnotations{JiraComponent:\"test-component\", Description:\"test description\", TestName:\"test-name\", AutoRegenerateAfterOfflineExpiry:\"\", NotBefore:\"\", NotAfter:\"\", RefreshPeriod:\"\"}",
254+
expectedOwnerReferences: []metav1.OwnerReference{
255+
*testOwner,
256+
},
257+
expectedAnnotations: map[string]string{
258+
annotations.OpenShiftComponent: "test-component",
259+
annotations.OpenShiftDescription: "test description",
260+
CertificateTestNameAnnotation: "test-name",
261+
},
262+
},
263+
}
264+
265+
for _, tt := range tests {
266+
t.Run(tt.name, func(t *testing.T) {
267+
result := ensureOwnerRefAndTLSAnnotationsForConfigMap(tt.configMap, tt.owner, tt.additionalAnnotations)
268+
269+
require.Equal(t, result, tt.expectedUpdateDetails, "expected update detail")
270+
require.Equal(t, tt.expectedOwnerReferences, tt.configMap.OwnerReferences, "expected owner references")
271+
require.Equal(t, tt.expectedAnnotations, tt.configMap.Annotations, "expected owner annotations")
272+
})
273+
}
274+
}

‎pkg/operator/certrotation/signer.go‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -81,15 +81,8 @@ func (c RotatedSigningCASecret) EnsureSigningCertKeyPair(ctx context.Context) (*
8181
// run Update if metadata needs changing unless we're in RefreshOnlyWhenExpired mode
8282
updateDetail := ""
8383
if !c.RefreshOnlyWhenExpired {
84-
needsMetadataUpdate := ensureOwnerRefAndTLSAnnotationsForSecret(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations)
85-
if needsMetadataUpdate {
86-
updateDetail = fmt.Sprintf("annotations update: %v", c.AdditionalAnnotations)
87-
}
88-
needsTypeChange := ensureSecretTLSTypeSet(signingCertKeyPairSecret)
89-
if needsTypeChange {
90-
updateDetail = "secret type update"
91-
}
92-
updateRequired = needsMetadataUpdate || needsTypeChange
84+
updateDetail = ensureOwnerRefAndTLSAnnotationsForSecret(signingCertKeyPairSecret, c.Owner, c.AdditionalAnnotations)
85+
updateRequired = len(updateDetail) > 0
9386
}
9487

9588
// run Update if signer content needs changing

‎pkg/operator/certrotation/target.go‎

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -115,15 +115,8 @@ func (c RotatedSelfSignedCertKeySecret) EnsureTargetCertKeyPair(ctx context.Cont
115115
// run Update if metadata needs changing unless we're in RefreshOnlyWhenExpired mode
116116
updateDetail := ""
117117
if !c.RefreshOnlyWhenExpired {
118-
needsMetadataUpdate := ensureOwnerRefAndTLSAnnotationsForSecret(targetCertKeyPairSecret, c.Owner, c.AdditionalAnnotations)
119-
if needsMetadataUpdate {
120-
updateDetail = fmt.Sprintf("annotations update: %v", c.AdditionalAnnotations)
121-
}
122-
needsTypeChange := ensureSecretTLSTypeSet(targetCertKeyPairSecret)
123-
if needsTypeChange {
124-
updateDetail = "secret type update"
125-
}
126-
updateRequired = needsMetadataUpdate || needsTypeChange
118+
updateDetail = ensureOwnerRefAndTLSAnnotationsForSecret(targetCertKeyPairSecret, c.Owner, c.AdditionalAnnotations)
119+
updateRequired = len(updateDetail) > 0
127120
}
128121

129122
if reason := c.CertCreator.NeedNewTargetCertKeyPair(targetCertKeyPairSecret, signingCertKeyPair, caBundleCerts, c.Refresh, c.RefreshOnlyWhenExpired, creationRequired); len(reason) > 0 {

0 commit comments

Comments
 (0)