return an additional bool in EnsureCerts

pkg/admission/cert/writer/certwriter.go

@@ -45,7 +45,11 @@ const (
 // CertWriter provides method to handle webhooks.
 type CertWriter interface {
 	// EnsureCert ensures that the webhooks have proper certificates.
-	EnsureCerts(runtime.Object) error
+	// It takes a runtime.Object that the underlying type should be
+	// either MutatingWebhookConfiguration or ValidatingWebhookConfiguration.
+	// It returns a boolean indicating if the certificate has been updated.
+	// This is helpful to signal the server to reload with the new certificate.
+	EnsureCerts(runtime.Object) (bool, error)
 }
 
 // Options are options for configuring a CertWriter.
@@ -81,31 +85,33 @@ func NewCertWriter(ops Options) (CertWriter, error) {
 
 // handleCommon ensures the given webhook has a proper certificate.
 // It uses the given certReadWriter to read and (or) write the certificate.
-func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWriter) error {
+// It returns if the certificate has been updated by the certReadWriter and a potential error.
+func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWriter) (bool, error) {
 	if webhook == nil {
-		return nil
+		return false, nil
 	}
 	if ch == nil {
-		return errors.New("certReaderWriter should not be nil")
+		return false, errors.New("certReaderWriter should not be nil")
 	}
 
-	certs, err := createIfNotExists(webhook.Name, ch)
+	certs, changed, err := createIfNotExists(webhook.Name, ch)
 	if err != nil {
-		return err
+		return changed, err
 	}
 
 	dnsName, err := dnsNameForWebhook(&webhook.ClientConfig)
 	if err != nil {
-		return err
+		return changed, err
 	}
 	// Recreate the cert if it's invalid.
 	valid := validCert(certs, dnsName)
 	if !valid {
 		log.Printf("cert is invalid or expiring, regenerating a new one")
 		certs, err = ch.overwrite(webhook.Name)
 		if err != nil {
-			return err
+			return changed, err
 		}
+		changed = true
 	}
 
 	// Ensure the CA bundle in the webhook configuration has the signing CA.
@@ -114,10 +120,11 @@ func handleCommon(webhook *admissionregistrationv1beta1.Webhook, ch certReadWrit
 	if !bytes.Contains(caBundle, caCert) {
 		webhook.ClientConfig.CABundle = append(caBundle, caCert...)
 	}
-	return nil
+	return changed, nil
 }
 
-func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifacts, error) {
+// The returning boolean indicates if the certificate has been updated by either this thread or another racing thread.
+func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifacts, bool, error) {
 	// Try to read first
 	certs, err := ch.read(webhookName)
 	if isNotFound(err) {
@@ -127,16 +134,18 @@ func createIfNotExists(webhookName string, ch certReadWriter) (*generator.Artifa
 		// This may happen if there is another racer.
 		case isAlreadyExists(err):
 			certs, err = ch.read(webhookName)
-			if err != nil {
-				return certs, err
-			}
+			// return true here, since we want to signal the caller when there is a change
+			// even it is not changed by the current thread.
+			return certs, true, err
 		case err != nil:
-			return certs, err
+			return certs, false, err
+		default:
+			return certs, true, err
 		}
 	} else if err != nil {
-		return certs, err
+		return certs, false, err
 	}
-	return certs, nil
+	return certs, false, nil
 }
 
 // certReadWriter provides methods for reading and writing certificates.

pkg/admission/cert/writer/certwriter_test.go

@@ -159,14 +159,15 @@ var _ = Describe("handleCommon", func() {
 	Context("when webhook is nil", func() {
 		It("should return no error", func() {
 			certrw := &fakeCertReadWriter{}
-			err := handleCommon(nil, certrw)
+			updated, err := handleCommon(nil, certrw)
 			Expect(err).NotTo(HaveOccurred())
+			Expect(updated).To(BeFalse())
 		})
 	})
 
 	Context("when certReadWriter is nil", func() {
 		It("should return an error", func() {
-			err := handleCommon(webhook, nil)
+			_, err := handleCommon(webhook, nil)
 			Expect(err).To(MatchError(goerrors.New("certReaderWriter should not be nil")))
 		})
 	})
@@ -186,8 +187,9 @@ var _ = Describe("handleCommon", func() {
 				},
 			}
 
-			err := handleCommon(webhook, certrw)
+			updated, err := handleCommon(webhook, certrw)
 			Expect(err).NotTo(HaveOccurred())
+			Expect(updated).To(BeTrue())
 			Expect(certrw.numReadCalled).To(Equal(1))
 			Expect(certrw.numWriteCalled).To(Equal(1))
 			Expect(certrw.numOverwriteCalled).To(Equal(0))
@@ -207,7 +209,7 @@ var _ = Describe("handleCommon", func() {
 				},
 			}
 
-			err := handleCommon(webhook, certrw)
+			_, err := handleCommon(webhook, certrw)
 			Expect(err).To(MatchError(goerrors.New("failed to write")))
 			Expect(certrw.numReadCalled).To(Equal(1))
 			Expect(certrw.numWriteCalled).To(Equal(1))
@@ -225,8 +227,9 @@ var _ = Describe("handleCommon", func() {
 				},
 			}
 
-			err := handleCommon(webhook, certrw)
+			updated, err := handleCommon(webhook, certrw)
 			Expect(err).NotTo(HaveOccurred())
+			Expect(updated).To(BeFalse())
 			Expect(certrw.numReadCalled).To(Equal(1))
 			Expect(certrw.numWriteCalled).To(Equal(0))
 			Expect(certrw.numOverwriteCalled).To(Equal(0))
@@ -241,7 +244,7 @@ var _ = Describe("handleCommon", func() {
 				},
 			}
 
-			err := handleCommon(webhook, certrw)
+			_, err := handleCommon(webhook, certrw)
 			Expect(err).To(MatchError(goerrors.New("failed to read")))
 			Expect(certrw.numReadCalled).To(Equal(1))
 			Expect(certrw.numWriteCalled).To(Equal(0))
@@ -264,8 +267,9 @@ var _ = Describe("handleCommon", func() {
 				},
 			}
 
-			err := handleCommon(webhook, certrw)
+			updated, err := handleCommon(webhook, certrw)
 			Expect(err).NotTo(HaveOccurred())
+			Expect(updated).To(BeTrue())
 			Expect(certrw.numReadCalled).To(Equal(1))
 			Expect(certrw.numWriteCalled).To(Equal(0))
 			Expect(certrw.numOverwriteCalled).To(Equal(1))
@@ -285,8 +289,9 @@ var _ = Describe("handleCommon", func() {
 				},
 			}
 
-			err := handleCommon(webhook, certrw)
+			updated, err := handleCommon(webhook, certrw)
 			Expect(err).NotTo(HaveOccurred())
+			Expect(updated).To(BeTrue())
 			Expect(certrw.numReadCalled).To(Equal(1))
 			Expect(certrw.numWriteCalled).To(Equal(0))
 			Expect(certrw.numOverwriteCalled).To(Equal(1))
@@ -306,7 +311,7 @@ var _ = Describe("handleCommon", func() {
 				},
 			}
 
-			err := handleCommon(webhook, certrw)
+			_, err := handleCommon(webhook, certrw)
 			Expect(err).To(MatchError(goerrors.New("failed to overwrite")))
 			Expect(certrw.numReadCalled).To(Equal(1))
 			Expect(certrw.numOverwriteCalled).To(Equal(1))
@@ -331,7 +336,8 @@ var _ = Describe("handleCommon", func() {
 				},
 			}
 
-			err := handleCommon(webhook, certrw)
+			updated, err := handleCommon(webhook, certrw)
+			Expect(updated).To(BeTrue())
 			Expect(err).NotTo(HaveOccurred())
 			Expect(certrw.numReadCalled).To(Equal(2))
 			Expect(certrw.numWriteCalled).To(Equal(1))
@@ -354,7 +360,7 @@ var _ = Describe("handleCommon", func() {
 				},
 			}
 
-			err := handleCommon(webhook, certrw)
+			_, err := handleCommon(webhook, certrw)
 			Expect(err).To(MatchError(goerrors.New("failed to read")))
 			Expect(certrw.numReadCalled).To(Equal(2))
 			Expect(certrw.numWriteCalled).To(Equal(1))

pkg/admission/cert/writer/fs.go

@@ -49,23 +49,24 @@ type FSCertWriter struct {
 var _ CertWriter = &FSCertWriter{}
 
 // EnsureCerts provisions certificates for a webhook configuration by writing them in the filesystem.
-func (f *FSCertWriter) EnsureCerts(webhookConfig runtime.Object) error {
+// It returns if the certificate has been updated by the certReadWriter and a potential error.
+func (f *FSCertWriter) EnsureCerts(webhookConfig runtime.Object) (bool, error) {
 	if webhookConfig == nil {
-		return errors.New("unexpected nil webhook configuration object")
+		return false, errors.New("unexpected nil webhook configuration object")
 	}
 
 	fsWebhookMap := map[string]*webhookAndPath{}
 	accessor, err := meta.Accessor(webhookConfig)
 	if err != nil {
-		return err
+		return false, err
 	}
 	annotations := accessor.GetAnnotations()
 	// Parse the annotations to extract info
 	f.parseAnnotations(annotations, fsWebhookMap)
 
 	webhooks, err := getWebhooksFromObject(webhookConfig)
 	if err != nil {
-		return err
+		return false, err
 	}
 	for i, webhook := range webhooks {
 		if p, found := fsWebhookMap[webhook.Name]; found {
@@ -76,7 +77,7 @@ func (f *FSCertWriter) EnsureCerts(webhookConfig runtime.Object) error {
 	// validation
 	for k, v := range fsWebhookMap {
 		if v.webhook == nil {
-			return fmt.Errorf("expecting a webhook named %q", k)
+			return false, fmt.Errorf("expecting a webhook named %q", k)
 		}
 	}
 
@@ -119,15 +120,16 @@ type webhookAndPath struct {
 
 var _ certReadWriter = &fsCertWriter{}
 
-func (f *fsCertWriter) ensureCert() error {
-	var err error
+func (f *fsCertWriter) ensureCert() (bool, error) {
+	anyChanged := false
 	for _, v := range f.webhookMap {
-		err = handleCommon(v.webhook, f)
+		changed, err := handleCommon(v.webhook, f)
+		anyChanged = anyChanged || changed
 		if err != nil {
-			return err
+			return anyChanged, err
 		}
 	}
-	return nil
+	return anyChanged, nil
 }
 
 func (f *fsCertWriter) write(webhookName string) (*certgenerator.Artifacts, error) {

pkg/admission/cert/writer/fs_test.go

@@ -113,14 +113,14 @@ var _ = Describe("FSCertWriter", func() {
 
 			Describe("nil object", func() {
 				It("should return error", func() {
-					err := certWriter.EnsureCerts(nil)
+					_, err := certWriter.EnsureCerts(nil)
 					Expect(err).To(MatchError("unexpected nil webhook configuration object"))
 				})
 			})
 
 			Describe("non-webhook configuration object", func() {
 				It("should return error", func() {
-					err := certWriter.EnsureCerts(&corev1.Pod{})
+					_, err := certWriter.EnsureCerts(&corev1.Pod{})
 					Expect(err).To(MatchError(fmt.Errorf("unsupported type: %T, only support v1beta1.MutatingWebhookConfiguration and v1beta1.ValidatingWebhookConfiguration",
 						&corev1.Pod{})))
 				})
@@ -133,9 +133,9 @@ var _ = Describe("FSCertWriter", func() {
 					close(done)
 				})
 				It("should return error", func() {
-					err := certWriter.EnsureCerts(mwc)
+					_, err := certWriter.EnsureCerts(mwc)
 					Expect(err).To(MatchError(fmt.Errorf("expecting a webhook named %q", "webhook-does-not-exist")))
-					err = certWriter.EnsureCerts(vwc)
+					_, err = certWriter.EnsureCerts(vwc)
 					Expect(err).To(MatchError(fmt.Errorf("expecting a webhook named %q", "webhook-does-not-exist")))
 				})
 			})
@@ -175,16 +175,17 @@ var _ = Describe("FSCertWriter", func() {
 				})
 
 				It("should default it and return no error", func() {
-					err := certWriter.EnsureCerts(mwc)
+					updated, err := certWriter.EnsureCerts(mwc)
 					Expect(err).NotTo(HaveOccurred())
-
+					Expect(updated).To(BeTrue())
 				})
 			})
 
 			Context("no existing secret", func() {
 				It("should create new secrets with certs", func() {
-					err := certWriter.EnsureCerts(mwc)
+					updated, err := certWriter.EnsureCerts(mwc)
 					Expect(err).NotTo(HaveOccurred())
+					Expect(updated).To(BeTrue())
 					caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
 					Expect(err).NotTo(HaveOccurred())
 					Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -208,8 +209,9 @@ var _ = Describe("FSCertWriter", func() {
 							})
 
 							It("should replace with new certs", func() {
-								err := certWriter.EnsureCerts(mwc)
+								updated, err := certWriter.EnsureCerts(mwc)
 								Expect(err).NotTo(HaveOccurred())
+								Expect(updated).To(BeTrue())
 								caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
 								Expect(err).NotTo(HaveOccurred())
 								Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -237,8 +239,9 @@ var _ = Describe("FSCertWriter", func() {
 							})
 
 							It("should replace with new certs", func() {
-								err := certWriter.EnsureCerts(mwc)
+								updated, err := certWriter.EnsureCerts(mwc)
 								Expect(err).NotTo(HaveOccurred())
+								Expect(updated).To(BeTrue())
 								caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
 								Expect(err).NotTo(HaveOccurred())
 								Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -262,8 +265,9 @@ var _ = Describe("FSCertWriter", func() {
 							})
 
 							It("should replace with new certs", func() {
-								err := certWriter.EnsureCerts(mwc)
+								updated, err := certWriter.EnsureCerts(mwc)
 								Expect(err).NotTo(HaveOccurred())
+								Expect(updated).To(BeTrue())
 								caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
 								Expect(err).NotTo(HaveOccurred())
 								Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -297,8 +301,9 @@ var _ = Describe("FSCertWriter", func() {
 							})
 
 							It("should replace with new certs", func() {
-								err := certWriter.EnsureCerts(mwc)
+								updated, err := certWriter.EnsureCerts(mwc)
 								Expect(err).NotTo(HaveOccurred())
+								Expect(updated).To(BeTrue())
 								caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
 								Expect(err).NotTo(HaveOccurred())
 								Expect(caBytes).To(Equal([]byte(certs2.CACert)))
@@ -326,8 +331,9 @@ var _ = Describe("FSCertWriter", func() {
 						close(done)
 					})
 					It("should keep the secret", func() {
-						err := certWriter.EnsureCerts(mwc)
+						updated, err := certWriter.EnsureCerts(mwc)
 						Expect(err).NotTo(HaveOccurred())
+						Expect(updated).To(BeFalse())
 						caBytes, err := ioutil.ReadFile(path.Join(testingDir, CACertName))
 						Expect(err).NotTo(HaveOccurred())
 						Expect(caBytes).To(Equal([]byte(certs2.CACert)))

pkg/admission/cert/writer/multiple.go

@@ -29,13 +29,15 @@ type MultiCertWriter struct {
 var _ CertWriter = &MultiCertWriter{}
 
 // EnsureCerts provisions certificates for a webhook configuration by invoking each CertWrite.
-func (s *MultiCertWriter) EnsureCerts(webhookConfig runtime.Object) error {
-	var err error
+// It returns if the certificate has been updated by the certReadWriter and a potential error.
+func (s *MultiCertWriter) EnsureCerts(webhookConfig runtime.Object) (bool, error) {
+	anyChanged := false
 	for _, certWriter := range s.CertWriters {
-		err = certWriter.EnsureCerts(webhookConfig)
+		changed, err := certWriter.EnsureCerts(webhookConfig)
+		anyChanged = anyChanged || changed
 		if err != nil {
-			return err
+			return anyChanged, err
 		}
 	}
-	return nil
+	return anyChanged, nil
 }