cleanup tests

Author: Mengqi Yu

pkg/webhook/internal/cert/provisioner.go

@@ -57,6 +57,10 @@ func (cp *Provisioner) Provision(options Options) (bool, error) {
 	if cp.CertWriter == nil {
 		return false, errors.New("CertWriter need to be set")
 	}
+	// If the objects need to be updated, just be lazy and return.
+	if len(options.Objects) == 0 {
+		return false, nil
+	}
 
 	dnsName, err := dnsNameFromClientConfig(options.ClientConfig)
 	if err != nil {
@@ -70,6 +74,8 @@ func (cp *Provisioner) Provision(options Options) (bool, error) {
 
 	caBundle := options.ClientConfig.CABundle
 	caCert := certs.CACert
+	// TODO(mengqiy): limit the size of the CABundle by GC the old CA certificate
+	// this is important since the max record size in etcd is 1MB (latest version is 1.5MB).
 	if !bytes.Contains(caBundle, caCert) {
 		// Ensure the CA bundle in the webhook configuration has the signing CA.
 		options.ClientConfig.CABundle = append(caBundle, caCert...)
@@ -90,6 +96,9 @@ func (cp *Provisioner) inject(cc *admissionregistrationv1beta1.WebhookClientConf
 			injectForEachWebhook(cc, typed.Webhooks)
 		case *admissionregistrationv1beta1.ValidatingWebhookConfiguration:
 			injectForEachWebhook(cc, typed.Webhooks)
+		default:
+			return fmt.Errorf("%#v is not supported for injecting a webhookClientConfig",
+				objs[i].GetObjectKind().GroupVersionKind())
 		}
 	}
 	return cp.CertWriter.Inject(objs...)
@@ -105,6 +114,9 @@ func injectForEachWebhook(
 }
 
 func dnsNameFromClientConfig(config *admissionregistrationv1beta1.WebhookClientConfig) (string, error) {
+	if config == nil {
+		return "", errors.New("clientConfig should not be empty")
+	}
 	if config.Service != nil && config.URL != nil {
 		return "", fmt.Errorf("service and URL can't be set at the same time in a webhook: %v", config)
 	}
@@ -114,9 +126,6 @@ func dnsNameFromClientConfig(config *admissionregistrationv1beta1.WebhookClientC
 	if config.Service != nil {
 		return generator.ServiceToCommonName(config.Service.Namespace, config.Service.Name), nil
 	}
-
-	// TODO
-	// config.URL != nil
 	u, err := url.Parse(*config.URL)
 	return u.Host, err
 }

pkg/webhook/internal/cert/provisioner_test.go

@@ -5,7 +5,7 @@ Licensed under the Apache License, Version 2.0 (the "License");
 you may not use this file except in compliance with the License.
 You may obtain a copy of the License at
 
-    http://www.apache.org/licenses/LICENSE-2.0
+  http://www.apache.org/licenses/LICENSE-2.0
 
 Unless required by applicable law or agreed to in writing, software
 distributed under the License is distributed on an "AS IS" BASIS,
@@ -17,20 +17,196 @@ limitations under the License.
 package cert
 
 import (
-	"testing"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
 
-	"k8s.io/api/admissionregistration/v1beta1"
-	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	admissionregistrationv1beta1 "k8s.io/api/admissionregistration/v1beta1"
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/internal/cert/generator"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/internal/cert/writer"
 )
 
-func TestCertProvisionerInit(t *testing.T) {
-	p := &Provisioner{
-		Client: fake.NewFakeClient(),
-	}
-	config := &v1beta1.MutatingWebhookConfiguration{}
+var _ = Describe("provisioner", func() {
+	Context("Invalid Provisioner", func() {
+		It("should return error", func() {
+			p := Provisioner{}
+			_, err := p.Provision(Options{})
+			Expect(err).To(MatchError("CertWriter need to be set"))
+		})
+	})
 
-	err := p.Sync(config)
-	if err != nil {
-		t.Fatalf("expect nil; got %q", err)
-	}
+	Context("No objects in the options", func() {
+		It("should return no error", func() {
+			fcw := &fakeCertWriter{}
+			p := Provisioner{CertWriter: fcw}
+			changed, err := p.Provision(Options{})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(changed).To(BeFalse())
+			Expect(fcw.invokedEnsureCert).To(BeFalse())
+			Expect(fcw.invokedInject).To(BeFalse())
+		})
+	})
+
+	Context("WebhookClientConfig is missing in the options", func() {
+		It("should return error", func() {
+			p := Provisioner{CertWriter: &fakeCertWriter{}}
+			_, err := p.Provision(Options{
+				Objects: []runtime.Object{
+					&corev1.Pod{},
+				},
+			})
+			Expect(err).To(MatchError("clientConfig should not be empty"))
+		})
+	})
+
+	Context("object is not support for injecting webhookClientConfig", func() {
+		It("should return no error", func() {
+			p := Provisioner{CertWriter: &fakeCertWriter{}}
+			_, err := p.Provision(Options{
+				ClientConfig: &admissionregistrationv1beta1.WebhookClientConfig{
+					Service: &admissionregistrationv1beta1.ServiceReference{
+						Namespace: "test-svc-namespace",
+						Name:      "test-service",
+					},
+				},
+				Objects: []runtime.Object{
+					&corev1.Pod{},
+				},
+			})
+			Expect(err.Error()).To(ContainSubstring("not supported for injecting a webhookClientConfig"))
+		})
+	})
+
+	Context("webhookConfig has 0 webhook", func() {
+		It("should return no error", func() {
+			fcw := &fakeCertWriter{}
+			p := Provisioner{CertWriter: fcw}
+			_, err := p.Provision(Options{
+				ClientConfig: &admissionregistrationv1beta1.WebhookClientConfig{
+					Service: &admissionregistrationv1beta1.ServiceReference{
+						Namespace: "test-svc-namespace",
+						Name:      "test-service",
+					},
+				},
+				Objects: []runtime.Object{
+					&admissionregistrationv1beta1.MutatingWebhookConfiguration{},
+				},
+			})
+			Expect(err).To(BeNil())
+			Expect(fcw.invokedEnsureCert).To(BeTrue())
+			Expect(fcw.invokedInject).To(BeTrue())
+		})
+	})
+
+	Context("happy path", func() {
+		It("should return no error", func() {
+			fcw := &fakeCertWriter{}
+			mwc := &admissionregistrationv1beta1.MutatingWebhookConfiguration{
+				Webhooks: []admissionregistrationv1beta1.Webhook{
+					{
+						Name: "foo-webhook",
+					},
+				},
+			}
+			vwc := &admissionregistrationv1beta1.ValidatingWebhookConfiguration{
+				Webhooks: []admissionregistrationv1beta1.Webhook{
+					{
+						Name: "foo-webhook",
+					},
+				},
+			}
+			p := Provisioner{CertWriter: fcw}
+			_, err := p.Provision(Options{
+				ClientConfig: &admissionregistrationv1beta1.WebhookClientConfig{
+					Service: &admissionregistrationv1beta1.ServiceReference{
+						Namespace: "test-svc-namespace",
+						Name:      "test-service",
+					},
+				},
+				Objects: []runtime.Object{mwc, vwc},
+			})
+			Expect(err).To(BeNil())
+			Expect(fcw.invokedEnsureCert).To(BeTrue())
+			Expect(fcw.invokedInject).To(BeTrue())
+		})
+	})
+})
+
+var _ = Describe("dnsNameFromClientConfig", func() {
+	Context("Invalid WebhookClientConfig", func() {
+		It("should return error", func() {
+			_, err := dnsNameFromClientConfig(nil)
+			Expect(err).To(MatchError("clientConfig should not be empty"))
+		})
+	})
+
+	Context("Neither Service nor URL is set", func() {
+		It("should return error", func() {
+			urlStr := "foo.example.com"
+			cc := &admissionregistrationv1beta1.WebhookClientConfig{
+				Service: &admissionregistrationv1beta1.ServiceReference{},
+				URL:     &urlStr,
+			}
+			_, err := dnsNameFromClientConfig(cc)
+			Expect(err.Error()).To(ContainSubstring("service and URL can't be set at the same time in a webhook"))
+		})
+	})
+
+	Context("Both Service and URL are set", func() {
+		It("should return error", func() {
+			urlStr := "https://foo.example.com"
+			cc := &admissionregistrationv1beta1.WebhookClientConfig{
+				Service: &admissionregistrationv1beta1.ServiceReference{},
+				URL:     &urlStr,
+			}
+			_, err := dnsNameFromClientConfig(cc)
+			Expect(err.Error()).To(ContainSubstring("service and URL can't be set at the same time in a webhook"))
+		})
+	})
+
+	Context("Only service is set", func() {
+		It("should return a DNS name", func() {
+			path := "somepath"
+			cc := &admissionregistrationv1beta1.WebhookClientConfig{
+				Service: &admissionregistrationv1beta1.ServiceReference{
+					Namespace: "test-svc-namespace",
+					Name:      "test-service",
+					Path:      &path,
+				},
+			}
+			dnsName, err := dnsNameFromClientConfig(cc)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(dnsName).To(Equal("test-service.test-svc-namespace.svc"))
+		})
+	})
+
+	Context("Only URL is set", func() {
+		It("should return a DNS name", func() {
+			urlStr := "https://foo.example.com/webhookendpoint"
+			cc := &admissionregistrationv1beta1.WebhookClientConfig{
+				URL: &urlStr,
+			}
+			dnsName, err := dnsNameFromClientConfig(cc)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(dnsName).To(Equal("foo.example.com"))
+		})
+	})
+})
+
+type fakeCertWriter struct {
+	invokedEnsureCert bool
+	invokedInject     bool
+}
+
+var _ writer.CertWriter = &fakeCertWriter{}
+
+func (f *fakeCertWriter) EnsureCert(dnsName string, dryrun bool) (*generator.Artifacts, bool, error) {
+	f.invokedEnsureCert = true
+	return &generator.Artifacts{}, true, nil
+}
+
+func (f *fakeCertWriter) Inject(objs ...runtime.Object) error {
+	f.invokedInject = true
+	return nil
 }

pkg/webhook/internal/cert/suite_test.go

@@ -0,0 +1,37 @@
+/*
+Copyright 2018 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package cert
+
+import (
+	"testing"
+
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+
+	"sigs.k8s.io/controller-runtime/pkg/envtest"
+	logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
+)
+
+func TestSource(t *testing.T) {
+	RegisterFailHandler(Fail)
+	RunSpecsWithDefaultAndCustomReporters(t, "Cert Provisioner Test Suite", []Reporter{envtest.NewlineReporter{}})
+}
+
+var _ = BeforeSuite(func(done Done) {
+	logf.SetLogger(logf.ZapLoggerTo(GinkgoWriter, true))
+	close(done)
+}, 60)

pkg/webhook/internal/cert/writer/certwriter.go

@@ -48,16 +48,18 @@ type CertWriter interface {
 // handleCommon ensures the given webhook has a proper certificate.
 // It uses the given certReadWriter to read and (or) write the certificate.
 func handleCommon(dnsName string, ch certReadWriter) (*generator.Artifacts, bool, error) {
+	if len(dnsName) == 0 {
+		return nil, false, errors.New("dnsName should not be empty")
+	}
 	if ch == nil {
 		return nil, false, errors.New("certReaderWriter should not be nil")
 	}
 
-	certs, err := createIfNotExists(ch)
+	certs, changed, err := createIfNotExists(ch)
 	if err != nil {
-		return nil, false, err
+		return nil, changed, err
 	}
 
-	updated := false
 	// Recreate the cert if it's invalid.
 	valid := validCert(certs, dnsName)
 	if !valid {
@@ -66,12 +68,12 @@ func handleCommon(dnsName string, ch certReadWriter) (*generator.Artifacts, bool
 		if err != nil {
 			return nil, false, err
 		}
-		updated = true
+		changed = true
 	}
-	return certs, updated, nil
+	return certs, changed, nil
 }
 
-func createIfNotExists(ch certReadWriter) (*generator.Artifacts, error) {
+func createIfNotExists(ch certReadWriter) (*generator.Artifacts, bool, error) {
 	// Try to read first
 	certs, err := ch.read()
 	if isNotFound(err) {
@@ -81,16 +83,12 @@ func createIfNotExists(ch certReadWriter) (*generator.Artifacts, error) {
 		// This may happen if there is another racer.
 		case isAlreadyExists(err):
 			certs, err = ch.read()
-			if err != nil {
-				return certs, err
-			}
-		case err != nil:
-			return certs, err
+			return certs, true, err
+		default:
+			return certs, true, err
 		}
-	} else if err != nil {
-		return certs, err
 	}
-	return certs, nil
+	return certs, false, err
 }
 
 // certReadWriter provides methods for reading and writing certificates.