Skip to content

Commit 5c73986

Browse files
author
Mengqi Yu
committed
return an additional bool in Sync and EnsureCerts
1 parent f5b79b9 commit 5c73986

File tree

10 files changed

+118
-85
lines changed

10 files changed

+118
-85
lines changed

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

Lines changed: 25 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -45,7 +45,11 @@ 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+
// It takes a runtime.Object that the underlying type should be
49+
// either MutatingWebhookConfiguration or ValidatingWebhookConfiguration.
50+
// It returns a boolean indicating if the certificate has been updated.
51+
// This is helpful to signal the server to reload with the new certificate.
52+
EnsureCerts(runtime.Object) (bool, error)
4953
}
5054

5155
// Options are options for configuring a CertWriter.
@@ -81,31 +85,33 @@ func NewCertWriter(ops Options) (CertWriter, error) {
8185

8286
// handleCommon ensures the given webhook has a proper certificate.
8387
// It uses the given certReadWriter to read and (or) write the certificate.
84-
func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWriter) error {
88+
// It returns if the certificate has been updated by the certReadWriter and a potential error.
89+
func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWriter) (bool, error) {
8590
if webhook == nil {
86-
return nil
91+
return false, nil
8792
}
8893
if ch == nil {
89-
return errors.New("certReaderWriter should not be nil")
94+
return false, errors.New("certReaderWriter should not be nil")
9095
}
9196

92-
certs, err := createIfNotExists(webhook.Name, ch)
97+
certs, changed, err := createIfNotExists(webhook.Name, ch)
9398
if err != nil {
94-
return err
99+
return changed, err
95100
}
96101

97102
dnsName, err := dnsNameForWebhook(&webhook.ClientConfig)
98103
if err != nil {
99-
return err
104+
return changed, err
100105
}
101106
// Recreate the cert if it's invalid.
102107
valid := validCert(certs, dnsName)
103108
if !valid {
104109
log.Printf("cert is invalid or expiring, regenerating a new one")
105110
certs, err = ch.overwrite(webhook.Name)
106111
if err != nil {
107-
return err
112+
return changed, err
108113
}
114+
changed = true
109115
}
110116

111117
// Ensure the CA bundle in the webhook configuration has the signing CA.
@@ -114,10 +120,11 @@ func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWrit
114120
if !bytes.Contains(caBundle, caCert) {
115121
webhook.ClientConfig.CABundle = append(caBundle, caCert...)
116122
}
117-
return nil
123+
return changed, nil
118124
}
119125

120-
func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifacts, error) {
126+
// The returning boolean indicates if the certificate has been updated by either this thread or another racing thread.
127+
func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifacts, bool, error) {
121128
// Try to read first
122129
certs, err := ch.read(webhookName)
123130
if isNotFound(err) {
@@ -127,16 +134,18 @@ func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifa
127134
// This may happen if there is another racer.
128135
case isAlreadyExists(err):
129136
certs, err = ch.read(webhookName)
130-
if err != nil {
131-
return certs, err
132-
}
137+
// return true here, since we want to signal the caller when there is a change
138+
// even it is not changed by the current thread.
139+
return certs, true, err
133140
case err != nil:
134-
return certs, err
141+
return certs, false, err
142+
default:
143+
return certs, true, err
135144
}
136145
} else if err != nil {
137-
return certs, err
146+
return certs, false, err
138147
}
139-
return certs, nil
148+
return certs, false, nil
140149
}
141150

142151
// 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: 12 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -49,23 +49,24 @@ 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+
// It returns if the certificate has been updated by the certReadWriter and a potential error.
53+
func (f *FSCertWriter) EnsureCerts(webhookConfig runtime.Object) (bool, error) {
5354
if webhookConfig == nil {
54-
return errors.New("unexpected nil webhook configuration object")
55+
return false, errors.New("unexpected nil webhook configuration object")
5556
}
5657

5758
fsWebhookMap := map[string]*webhookAndPath{}
5859
accessor, err := meta.Accessor(webhookConfig)
5960
if err != nil {
60-
return err
61+
return false, err
6162
}
6263
annotations := accessor.GetAnnotations()
6364
// Parse the annotations to extract info
6465
f.parseAnnotations(annotations, fsWebhookMap)
6566

6667
webhooks, err := getWebhooksFromObject(webhookConfig)
6768
if err != nil {
68-
return err
69+
return false, err
6970
}
7071
for i, webhook := range webhooks {
7172
if p, found := fsWebhookMap[webhook.Name]; found {
@@ -76,7 +77,7 @@ func (f *FSCertWriter) EnsureCerts(webhookConfig runtime.Object) error {
7677
// validation
7778
for k, v := range fsWebhookMap {
7879
if v.webhook == nil {
79-
return fmt.Errorf("expecting a webhook named %q", k)
80+
return false, fmt.Errorf("expecting a webhook named %q", k)
8081
}
8182
}
8283

@@ -119,15 +120,16 @@ type webhookAndPath struct {
119120

120121
var _ certReadWriter = &fsCertWriter{}
121122

122-
func (f *fsCertWriter) ensureCert() error {
123-
var err error
123+
func (f *fsCertWriter) ensureCert() (bool, error) {
124+
anyChanged := false
124125
for _, v := range f.webhookMap {
125-
err = handleCommon(v.webhook, f)
126+
changed, err := handleCommon(v.webhook, f)
127+
anyChanged = anyChanged || changed
126128
if err != nil {
127-
return err
129+
return anyChanged, err
128130
}
129131
}
130-
return nil
132+
return anyChanged, nil
131133
}
132134

133135
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: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,13 +29,15 @@ 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+
// It returns if the certificate has been updated by the certReadWriter and a potential error.
33+
func (s *MultiCertWriter) EnsureCerts(webhookConfig runtime.Object) (bool, error) {
34+
anyChanged := false
3435
for _, certWriter := range s.CertWriters {
35-
err = certWriter.EnsureCerts(webhookConfig)
36+
changed, err := certWriter.EnsureCerts(webhookConfig)
37+
anyChanged = anyChanged || changed
3638
if err != nil {
37-
return err
39+
return anyChanged, err
3840
}
3941
}
40-
return nil
42+
return anyChanged, nil
4143
}

0 commit comments

Comments
 (0)