Skip to content

Commit e6c0975

Browse files
author
Mengqi Yu
committed
return an additional bool in EnsureCerts
1 parent 8bfde6c commit e6c0975

File tree

8 files changed

+99
-77
lines changed

8 files changed

+99
-77
lines changed

‎pkg/admission/cert/writer/certwriter.go

Lines changed: 17 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,7 @@ const (
4545
// CertWriter provides method to handle webhooks.
4646
type CertWriter interface {
4747
// EnsureCert ensures that the webhooks have proper certificates.
48-
EnsureCerts(runtime.Object) error
48+
EnsureCerts(runtime.Object) (bool, error)
4949
}
5050

5151
// Options are options for configuring a CertWriter.
@@ -81,31 +81,32 @@ func NewCertWriter(ops Options) (CertWriter, error) {
8181

8282
// handleCommon ensures the given webhook has a proper certificate.
8383
// It uses the given certReadWriter to read and (or) write the certificate.
84-
func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWriter) error {
84+
func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWriter) (bool, error) {
8585
if webhook == nil {
86-
return nil
86+
return false, nil
8787
}
8888
if ch == nil {
89-
return errors.New("certReaderWriter should not be nil")
89+
return false, errors.New("certReaderWriter should not be nil")
9090
}
9191

92-
certs, err := createIfNotExists(webhook.Name, ch)
92+
certs, changed, err := createIfNotExists(webhook.Name, ch)
9393
if err != nil {
94-
return err
94+
return changed, err
9595
}
9696

9797
dnsName, err := dnsNameForWebhook(&webhook.ClientConfig)
9898
if err != nil {
99-
return err
99+
return changed, err
100100
}
101101
// Recreate the cert if it's invalid.
102102
valid := validCert(certs, dnsName)
103103
if !valid {
104104
log.Printf("cert is invalid or expiring, regenerating a new one")
105105
certs, err = ch.overwrite(webhook.Name)
106106
if err != nil {
107-
return err
107+
return changed, err
108108
}
109+
changed = true
109110
}
110111

111112
// Ensure the CA bundle in the webhook configuration has the signing CA.
@@ -114,10 +115,10 @@ func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWrit
114115
if !bytes.Contains(caBundle, caCert) {
115116
webhook.ClientConfig.CABundle = append(caBundle, caCert...)
116117
}
117-
return nil
118+
return changed, nil
118119
}
119120

120-
func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifacts, error) {
121+
func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifacts, bool, error) {
121122
// Try to read first
122123
certs, err := ch.read(webhookName)
123124
if isNotFound(err) {
@@ -127,16 +128,16 @@ func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifa
127128
// This may happen if there is another racer.
128129
case isAlreadyExists(err):
129130
certs, err = ch.read(webhookName)
130-
if err != nil {
131-
return certs, err
132-
}
131+
return certs, true, err
133132
case err != nil:
134-
return certs, err
133+
return certs, false, err
134+
default:
135+
return certs, true, err
135136
}
136137
} else if err != nil {
137-
return certs, err
138+
return certs, false, err
138139
}
139-
return certs, nil
140+
return certs, false, nil
140141
}
141142

142143
// certReadWriter provides methods for reading and writing certificates.

‎pkg/admission/cert/writer/certwriter_test.go

Lines changed: 17 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -159,14 +159,15 @@ var _ = Describe("handleCommon", func() {
159159
Context("when webhook is nil", func() {
160160
It("should return no error", func() {
161161
certrw := &fakeCertReadWriter{}
162-
err := handleCommon(nil, certrw)
162+
updated, err := handleCommon(nil, certrw)
163163
Expect(err).NotTo(HaveOccurred())
164+
Expect(updated).To(BeFalse())
164165
})
165166
})
166167

167168
Context("when certReadWriter is nil", func() {
168169
It("should return an error", func() {
169-
err := handleCommon(webhook, nil)
170+
_, err := handleCommon(webhook, nil)
170171
Expect(err).To(MatchError(goerrors.New("certReaderWriter should not be nil")))
171172
})
172173
})
@@ -186,8 +187,9 @@ var _ = Describe("handleCommon", func() {
186187
},
187188
}
188189

189-
err := handleCommon(webhook, certrw)
190+
updated, err := handleCommon(webhook, certrw)
190191
Expect(err).NotTo(HaveOccurred())
192+
Expect(updated).To(BeTrue())
191193
Expect(certrw.numReadCalled).To(Equal(1))
192194
Expect(certrw.numWriteCalled).To(Equal(1))
193195
Expect(certrw.numOverwriteCalled).To(Equal(0))
@@ -207,7 +209,7 @@ var _ = Describe("handleCommon", func() {
207209
},
208210
}
209211

210-
err := handleCommon(webhook, certrw)
212+
_, err := handleCommon(webhook, certrw)
211213
Expect(err).To(MatchError(goerrors.New("failed to write")))
212214
Expect(certrw.numReadCalled).To(Equal(1))
213215
Expect(certrw.numWriteCalled).To(Equal(1))
@@ -225,8 +227,9 @@ var _ = Describe("handleCommon", func() {
225227
},
226228
}
227229

228-
err := handleCommon(webhook, certrw)
230+
updated, err := handleCommon(webhook, certrw)
229231
Expect(err).NotTo(HaveOccurred())
232+
Expect(updated).To(BeFalse())
230233
Expect(certrw.numReadCalled).To(Equal(1))
231234
Expect(certrw.numWriteCalled).To(Equal(0))
232235
Expect(certrw.numOverwriteCalled).To(Equal(0))
@@ -241,7 +244,7 @@ var _ = Describe("handleCommon", func() {
241244
},
242245
}
243246

244-
err := handleCommon(webhook, certrw)
247+
_, err := handleCommon(webhook, certrw)
245248
Expect(err).To(MatchError(goerrors.New("failed to read")))
246249
Expect(certrw.numReadCalled).To(Equal(1))
247250
Expect(certrw.numWriteCalled).To(Equal(0))
@@ -264,8 +267,9 @@ var _ = Describe("handleCommon", func() {
264267
},
265268
}
266269

267-
err := handleCommon(webhook, certrw)
270+
updated, err := handleCommon(webhook, certrw)
268271
Expect(err).NotTo(HaveOccurred())
272+
Expect(updated).To(BeTrue())
269273
Expect(certrw.numReadCalled).To(Equal(1))
270274
Expect(certrw.numWriteCalled).To(Equal(0))
271275
Expect(certrw.numOverwriteCalled).To(Equal(1))
@@ -285,8 +289,9 @@ var _ = Describe("handleCommon", func() {
285289
},
286290
}
287291

288-
err := handleCommon(webhook, certrw)
292+
updated, err := handleCommon(webhook, certrw)
289293
Expect(err).NotTo(HaveOccurred())
294+
Expect(updated).To(BeTrue())
290295
Expect(certrw.numReadCalled).To(Equal(1))
291296
Expect(certrw.numWriteCalled).To(Equal(0))
292297
Expect(certrw.numOverwriteCalled).To(Equal(1))
@@ -306,7 +311,7 @@ var _ = Describe("handleCommon", func() {
306311
},
307312
}
308313

309-
err := handleCommon(webhook, certrw)
314+
_, err := handleCommon(webhook, certrw)
310315
Expect(err).To(MatchError(goerrors.New("failed to overwrite")))
311316
Expect(certrw.numReadCalled).To(Equal(1))
312317
Expect(certrw.numOverwriteCalled).To(Equal(1))
@@ -331,7 +336,8 @@ var _ = Describe("handleCommon", func() {
331336
},
332337
}
333338

334-
err := handleCommon(webhook, certrw)
339+
updated, err := handleCommon(webhook, certrw)
340+
Expect(updated).To(BeTrue())
335341
Expect(err).NotTo(HaveOccurred())
336342
Expect(certrw.numReadCalled).To(Equal(2))
337343
Expect(certrw.numWriteCalled).To(Equal(1))
@@ -354,7 +360,7 @@ var _ = Describe("handleCommon", func() {
354360
},
355361
}
356362

357-
err := handleCommon(webhook, certrw)
363+
_, err := handleCommon(webhook, certrw)
358364
Expect(err).To(MatchError(goerrors.New("failed to read")))
359365
Expect(certrw.numReadCalled).To(Equal(2))
360366
Expect(certrw.numWriteCalled).To(Equal(1))

‎pkg/admission/cert/writer/fs.go

Lines changed: 11 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,23 +49,23 @@ type FSCertWriter struct {
4949
var _ CertWriter = &FSCertWriter{}
5050

5151
// EnsureCerts provisions certificates for a webhook configuration by writing them in the filesystem.
52-
func (f *FSCertWriter) EnsureCerts(webhookConfig runtime.Object) error {
52+
func (f *FSCertWriter) EnsureCerts(webhookConfig runtime.Object) (bool, error) {
5353
if webhookConfig == nil {
54-
return errors.New("unexpected nil webhook configuration object")
54+
return false, errors.New("unexpected nil webhook configuration object")
5555
}
5656

5757
fsWebhookMap := map[string]*webhookAndPath{}
5858
accessor, err := meta.Accessor(webhookConfig)
5959
if err != nil {
60-
return err
60+
return false, err
6161
}
6262
annotations := accessor.GetAnnotations()
6363
// Parse the annotations to extract info
6464
f.parseAnnotations(annotations, fsWebhookMap)
6565

6666
webhooks, err := getWebhooksFromObject(webhookConfig)
6767
if err != nil {
68-
return err
68+
return false, err
6969
}
7070
for i, webhook := range webhooks {
7171
if p, found := fsWebhookMap[webhook.Name]; found {
@@ -76,7 +76,7 @@ func (f *FSCertWriter) EnsureCerts(webhookConfig runtime.Object) error {
7676
// validation
7777
for k, v := range fsWebhookMap {
7878
if v.webhook == nil {
79-
return fmt.Errorf("expecting a webhook named %q", k)
79+
return false, fmt.Errorf("expecting a webhook named %q", k)
8080
}
8181
}
8282

@@ -119,15 +119,16 @@ type webhookAndPath struct {
119119

120120
var _ certReadWriter = &fsCertWriter{}
121121

122-
func (f *fsCertWriter) ensureCert() error {
123-
var err error
122+
func (f *fsCertWriter) ensureCert() (bool, error) {
123+
anyChanged := false
124124
for _, v := range f.webhookMap {
125-
err = handleCommon(v.webhook, f)
125+
changed, err := handleCommon(v.webhook, f)
126+
anyChanged = anyChanged || changed
126127
if err != nil {
127-
return err
128+
return anyChanged, err
128129
}
129130
}
130-
return nil
131+
return anyChanged, nil
131132
}
132133

133134
func (f *fsCertWriter) write(webhookName string) (*certgenerator.Artifacts, error) {

‎pkg/admission/cert/writer/fs_test.go

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -113,14 +113,14 @@ var _ = Describe("FSCertWriter", func() {
113113

114114
Describe("nil object", func() {
115115
It("should return error", func() {
116-
err := certWriter.EnsureCerts(nil)
116+
_, err := certWriter.EnsureCerts(nil)
117117
Expect(err).To(MatchError("unexpected nil webhook configuration object"))
118118
})
119119
})
120120

121121
Describe("non-webhook configuration object", func() {
122122
It("should return error", func() {
123-
err := certWriter.EnsureCerts(&corev1.Pod{})
123+
_, err := certWriter.EnsureCerts(&corev1.Pod{})
124124
Expect(err).To(MatchError(fmt.Errorf("unsupported type: %T, only support v1beta1.MutatingWebhookConfiguration and v1beta1.ValidatingWebhookConfiguration",
125125
&corev1.Pod{})))
126126
})
@@ -133,9 +133,9 @@ var _ = Describe("FSCertWriter", func() {
133133
close(done)
134134
})
135135
It("should return error", func() {
136-
err := certWriter.EnsureCerts(mwc)
136+
_, err := certWriter.EnsureCerts(mwc)
137137
Expect(err).To(MatchError(fmt.Errorf("expecting a webhook named %q", "webhook-does-not-exist")))
138-
err = certWriter.EnsureCerts(vwc)
138+
_, err = certWriter.EnsureCerts(vwc)
139139
Expect(err).To(MatchError(fmt.Errorf("expecting a webhook named %q", "webhook-does-not-exist")))
140140
})
141141
})
@@ -175,16 +175,17 @@ var _ = Describe("FSCertWriter", func() {
175175
})
176176

177177
It("should default it and return no error", func() {
178-
err := certWriter.EnsureCerts(mwc)
178+
updated, err := certWriter.EnsureCerts(mwc)
179179
Expect(err).NotTo(HaveOccurred())
180-
180+
Expect(updated).To(BeTrue())
181181
})
182182
})
183183

184184
Context("no existing secret", func() {
185185
It("should create new secrets with certs", func() {
186-
err := certWriter.EnsureCerts(mwc)
186+
updated, err := certWriter.EnsureCerts(mwc)
187187
Expect(err).NotTo(HaveOccurred())
188+
Expect(updated).To(BeTrue())
188189
caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
189190
Expect(err).NotTo(HaveOccurred())
190191
Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -208,8 +209,9 @@ var _ = Describe("FSCertWriter", func() {
208209
})
209210

210211
It("should replace with new certs", func() {
211-
err := certWriter.EnsureCerts(mwc)
212+
updated, err := certWriter.EnsureCerts(mwc)
212213
Expect(err).NotTo(HaveOccurred())
214+
Expect(updated).To(BeTrue())
213215
caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
214216
Expect(err).NotTo(HaveOccurred())
215217
Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -237,8 +239,9 @@ var _ = Describe("FSCertWriter", func() {
237239
})
238240

239241
It("should replace with new certs", func() {
240-
err := certWriter.EnsureCerts(mwc)
242+
updated, err := certWriter.EnsureCerts(mwc)
241243
Expect(err).NotTo(HaveOccurred())
244+
Expect(updated).To(BeTrue())
242245
caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
243246
Expect(err).NotTo(HaveOccurred())
244247
Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -262,8 +265,9 @@ var _ = Describe("FSCertWriter", func() {
262265
})
263266

264267
It("should replace with new certs", func() {
265-
err := certWriter.EnsureCerts(mwc)
268+
updated, err := certWriter.EnsureCerts(mwc)
266269
Expect(err).NotTo(HaveOccurred())
270+
Expect(updated).To(BeTrue())
267271
caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
268272
Expect(err).NotTo(HaveOccurred())
269273
Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -297,8 +301,9 @@ var _ = Describe("FSCertWriter", func() {
297301
})
298302

299303
It("should replace with new certs", func() {
300-
err := certWriter.EnsureCerts(mwc)
304+
updated, err := certWriter.EnsureCerts(mwc)
301305
Expect(err).NotTo(HaveOccurred())
306+
Expect(updated).To(BeTrue())
302307
caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
303308
Expect(err).NotTo(HaveOccurred())
304309
Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -326,8 +331,9 @@ var _ = Describe("FSCertWriter", func() {
326331
close(done)
327332
})
328333
It("should keep the secret", func() {
329-
err := certWriter.EnsureCerts(mwc)
334+
updated, err := certWriter.EnsureCerts(mwc)
330335
Expect(err).NotTo(HaveOccurred())
336+
Expect(updated).To(BeFalse())
331337
caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
332338
Expect(err).NotTo(HaveOccurred())
333339
Expect(caBytes).To(Equal([]byte(certs2.CACert)))

‎pkg/admission/cert/writer/multiple.go

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,14 @@ type MultiCertWriter struct {
2929
var _ CertWriter = &MultiCertWriter{}
3030

3131
// EnsureCerts provisions certificates for a webhook configuration by invoking each CertWrite.
32-
func (s *MultiCertWriter) EnsureCerts(webhookConfig runtime.Object) error {
33-
var err error
32+
func (s *MultiCertWriter) EnsureCerts(webhookConfig runtime.Object) (bool, error) {
33+
anyChanged := false
3434
for _, certWriter := range s.CertWriters {
35-
err = certWriter.EnsureCerts(webhookConfig)
35+
changed, err := certWriter.EnsureCerts(webhookConfig)
36+
anyChanged = anyChanged || changed
3637
if err != nil {
37-
return err
38+
return anyChanged, err
3839
}
3940
}
40-
return nil
41+
return anyChanged, nil
4142
}

0 commit comments

Comments
 (0)