address comments

Author: Mengqi Yu

pkg/webhook/bootstrap.go

@@ -17,7 +17,6 @@ limitations under the License.
 package webhook
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"net/http"
@@ -30,19 +29,24 @@ import (
 	"k8s.io/api/admissionregistration/v1beta1"
 	admissionregistration "k8s.io/api/admissionregistration/v1beta1"
 	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/config"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/internal/cert"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/internal/cert/generator"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/internal/cert/writer"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/types"
 )
 
 // setDefault does defaulting for the Server.
 func (s *Server) setDefault() {
+	s.setServerDefault()
+	s.setBootstrappingDefault()
+}
+
+// setServerDefault does defaulting for the ServerOptions.
+func (s *Server) setServerDefault() {
 	if len(s.Name) == 0 {
 		s.Name = "default-k8s-webhook-server"
 	}
@@ -61,10 +65,22 @@ func (s *Server) setDefault() {
 	if s.KVMap == nil {
 		s.KVMap = map[string]interface{}{}
 	}
-	s.setBootstrappingDefault()
+
+	if s.Client == nil {
+		cfg, err := config.GetConfig()
+		if err != nil {
+			s.err = err
+			return
+		}
+		s.Client, err = client.New(cfg, client.Options{})
+		if err != nil {
+			s.err = err
+			return
+		}
+	}
 }
 
-// setBootstrappingDefault does defaulting for the Server  bootstrapping.
+// setBootstrappingDefault does defaulting for the Server bootstrapping.
 func (s *Server) setBootstrappingDefault() {
 	if len(s.MutatingWebhookConfigName) == 0 {
 		s.MutatingWebhookConfigName = "mutating-webhook-configuration"
@@ -81,147 +97,71 @@ func (s *Server) setBootstrappingDefault() {
 	if s.Service != nil && s.Port != 443 {
 		s.Port = 443
 	}
-	if s.certProvisioner == nil {
-		s.certProvisioner = &cert.Provisioner{}
+
+	var certWriter writer.CertWriter
+	var err error
+	if s.Secret != nil {
+		certWriter, err = writer.NewSecretCertWriter(
+			writer.SecretCertWriterOptions{
+				Secret: s.Secret,
+			})
+	} else {
+		certWriter, err = writer.NewFSCertWriter(
+			writer.FSCertWriterOptions{
+				Path: s.CertDir,
+			})
+	}
+	if err != nil {
+		s.err = err
+		return
 	}
-	if s.CertGenerator == nil {
-		s.CertGenerator = &generator.SelfSignedCertGenerator{}
+	s.certProvisioner = &cert.Provisioner{
+		CertWriter: certWriter,
 	}
 	if s.Writer == nil {
 		s.Writer = os.Stdout
 	}
 }
 
-// bootstrap returns the configuration of admissionWebhookConfiguration in yaml format if dryrun is true.
+// bootstrap writes the configuration of admissionWebhookConfiguration in yaml format if dryrun is true.
 // Otherwise, it creates the the admissionWebhookConfiguration objects and service if any.
-// It also provisions the certificate for your admission server.
+// It also provisions the certificate for the admission server.
 func (s *Server) bootstrap(dryrun bool) error {
 	// do defaulting if necessary
 	s.once.Do(s.setDefault)
+	if s.err != nil {
+		return s.err
+	}
 
 	var err error
-	s.mutatingWebhookConfig, err = s.mutatingWHConfigs()
+	s.webhookConfigurations, err = s.whConfigs()
 	if err != nil {
 		return err
 	}
+	svc := s.service()
+	objects := append(s.webhookConfigurations, svc)
 
-	s.validatingWebhookConfig, err = s.validatingWHConfigs()
+	cc, err := s.getClientConfig()
 	if err != nil {
 		return err
 	}
-
-	cc, err := s.clientConfig()
-
-	s.certProvisioner = &cert.Provisioner{
-		ClientConfig:  cc,
-		CertGenerator: s.CertGenerator,
-	}
-
-	if s.Secret != nil {
-		s.certWriter, err = writer.NewSecretCertWriter(
-			writer.SecretCertWriterOptions{
-				Secret: s.Secret,
-			})
-	} else {
-		s.certWriter, err = writer.NewFSCertWriter(
-			writer.FSCertWriterOptions{
-				Path: s.CertDir,
-			})
-	}
 	// Provision the cert by creating new one or refreshing existing one.
-	s.certProvisioner.Provision(false)
-	// Inject the cert into MutatingWebhookConfiguration, ValidatingWebhookConfiguration and ConversionWebhook
-	s.certProvisioner.Inject(s.mutatingWebhookConfig, s.validatingWebhookConfig)
-
-	svc := s.service()
+	_, err = s.certProvisioner.Provision(cert.Options{
+		ClientConfig: cc,
+		Objects:      s.webhookConfigurations,
+		Dryrun:       dryrun,
+	})
+	if err != nil {
+		return err
+	}
 
 	if dryrun {
 		// TODO: print here
 		// if dryrun, return the AdmissionWebhookConfiguration in yaml format.
-		s.genYamlConfig([]runtime.Object{s.mutatingWebhookConfig, s.validatingWebhookConfig, svc})
-		return nil
-	}
-
-	if s.Client == nil {
-		return errors.New("client needs to be set for bootstrapping")
-	}
-
-	objects := []runtime.Object{s.mutatingWebhookConfig, s.validatingWebhookConfig, svc}
-
-	for _, obj := range objects {
-		err = createOrReplace(s.Client, obj)
-		if err != nil {
-			return err
-		}
+		return s.genYamlConfig(objects)
 	}
-	return nil
-}
-
-type mutateFn func(current, desired runtime.Object) error
-
-var serviceFn = func(current, desired runtime.Object) error {
-	typedC := current.(*corev1.Service)
-	typedD := desired.(*corev1.Service)
-	typedC.Spec.Selector = typedD.Spec.Selector
-	return nil
-}
-
-var mutatingWebhookConfigFn = func(current, desired runtime.Object) error {
-	typedC := current.(*admissionregistration.MutatingWebhookConfiguration)
-	typedD := desired.(*admissionregistration.MutatingWebhookConfiguration)
-	typedC.Webhooks = typedD.Webhooks
-	return nil
-}
 
-var validatingWebhookConfigFn = func(current, desired runtime.Object) error {
-	typedC := current.(*admissionregistration.ValidatingWebhookConfiguration)
-	typedD := desired.(*admissionregistration.ValidatingWebhookConfiguration)
-	typedC.Webhooks = typedD.Webhooks
-	return nil
-}
-
-// createOrReplace creates the object if it doesn't exist;
-// otherwise, it will replace it.
-// When replacing, it knows how to preserve existing fields in the object GET from the APIServer.
-func createOrReplace(c client.Client, obj runtime.Object) error {
-	switch obj.(type) {
-	case *admissionregistration.MutatingWebhookConfiguration:
-		return createOrReplaceHelper(c, obj, mutatingWebhookConfigFn)
-	case *admissionregistration.ValidatingWebhookConfiguration:
-		return createOrReplaceHelper(c, obj, validatingWebhookConfigFn)
-	case *corev1.Service:
-		return createOrReplaceHelper(c, obj, serviceFn)
-	}
-	return nil
-}
-
-// createOrReplaceHelper creates the object if it doesn't exist;
-// otherwise, it will replace it.
-// When replacing, fn  should know how to preserve existing fields in the object GET from the APIServer.
-// TODO: use the helper in #98 when it merges.
-func createOrReplaceHelper(c client.Client, obj runtime.Object, fn mutateFn) error {
-	if obj == nil {
-		return nil
-	}
-	err := c.Create(context.Background(), obj)
-	if apierrors.IsAlreadyExists(err) {
-		// TODO: retry mutiple times with backoff if necessary.
-		existing := obj.DeepCopyObject()
-		objectKey, err := client.ObjectKeyFromObject(obj)
-		if err != nil {
-			return err
-		}
-		err = c.Get(context.Background(), objectKey, existing)
-		if err != nil {
-			return err
-		}
-		err = fn(existing, obj)
-		if err != nil {
-			return err
-		}
-		return c.Update(context.Background(), existing)
-	}
-	return err
+	return batchCreateOrReplace(s.Client, objects...)
 }
 
 // genYamlConfig generates yaml config for admissionWebhookConfiguration
@@ -243,7 +183,7 @@ func (s *Server) genYamlConfig(objs []runtime.Object) error {
 	return nil
 }
 
-func (s *Server) clientConfig() (*admissionregistration.WebhookClientConfig, error) {
+func (s *Server) getClientConfig() (*admissionregistration.WebhookClientConfig, error) {
 	if s.Host != nil && s.Service != nil {
 		return nil, errors.New("URL and Service can't be set at the same time")
 	}
@@ -268,14 +208,17 @@ func (s *Server) clientConfig() (*admissionregistration.WebhookClientConfig, err
 	return cc, nil
 }
 
-func (s *Server) clientConfigWithPath(path string) (*admissionregistration.WebhookClientConfig, error) {
-	cc, err := s.clientConfig()
+// getClientConfigWithPath constructs a WebhookClientConfig based on the server options.
+// It will use path to the set the path in WebhookClientConfig.
+func (s *Server) getClientConfigWithPath(path string) (*admissionregistration.WebhookClientConfig, error) {
+	cc, err := s.getClientConfig()
 	if err != nil {
 		return nil, err
 	}
 	return cc, setPath(cc, path)
 }
 
+// setPath sets the path in the WebhookClientConfig.
 func setPath(cc *admissionregistration.WebhookClientConfig, path string) error {
 	if cc.URL != nil {
 		u, err := url.Parse(*cc.URL)
@@ -294,6 +237,25 @@ func setPath(cc *admissionregistration.WebhookClientConfig, path string) error {
 
 // whConfigs creates a mutatingWebhookConfiguration and(or) a validatingWebhookConfiguration based on registry.
 // For the same type of webhook configuration, it generates a webhook entry per endpoint.
+func (s *Server) whConfigs() ([]runtime.Object, error) {
+	objs := []runtime.Object{}
+	mutatingWH, err := s.mutatingWHConfigs()
+	if err != nil {
+		return nil, err
+	}
+	if mutatingWH != nil {
+		objs = append(objs, mutatingWH)
+	}
+	validatingWH, err := s.validatingWHConfigs()
+	if err != nil {
+		return nil, err
+	}
+	if validatingWH != nil {
+		objs = append(objs, validatingWH)
+	}
+	return objs, nil
+}
+
 func (s *Server) mutatingWHConfigs() (runtime.Object, error) {
 	mutatingWebhooks := []v1beta1.Webhook{}
 	for path, webhook := range s.registry {
@@ -306,11 +268,6 @@ func (s *Server) mutatingWHConfigs() (runtime.Object, error) {
 		if err != nil {
 			return nil, err
 		}
-		cc, err := s.clientConfigWithPath(path)
-		if err != nil {
-			return nil, err
-		}
-		wh.ClientConfig = *cc
 		mutatingWebhooks = append(mutatingWebhooks, *wh)
 	}
 
@@ -342,11 +299,6 @@ func (s *Server) validatingWHConfigs() (runtime.Object, error) {
 		if err != nil {
 			return nil, err
 		}
-		cc, err := s.clientConfigWithPath(path)
-		if err != nil {
-			return nil, err
-		}
-		wh.ClientConfig = *cc
 		validatingWebhooks = append(validatingWebhooks, *wh)
 	}
 
@@ -377,11 +329,7 @@ func (s *Server) admissionWebhook(path string, wh *admission.Webhook) (*admissio
 			CABundle: []byte{},
 		},
 	}
-	cc, err := s.clientConfig()
-	if err != nil {
-		return nil, err
-	}
-	err = setPath(cc, path)
+	cc, err := s.getClientConfigWithPath(path)
 	if err != nil {
 		return nil, err
 	}

pkg/webhook/doc.go

@@ -64,7 +64,6 @@ Start the server by starting the manager
 	if err != nil {
 		// handle error
 	}
-
 */
 package webhook
 

pkg/webhook/internal/cert/doc.go

@@ -17,25 +17,18 @@ limitations under the License.
 /*
 Package cert provides functions to manage certificates for webhookClientConfiguration.
 
-Create a Provisioner with the webhookClientConfig cc you want to manage.
+Create a Provisioner with a CertWriter.
 
 	provisioner := Provisioner{
 		CertWriter: admission.NewSecretCertWriter(admission.SecretCertWriterOptions{...}),
-		ClientConfig: cc,
 	}
 
 Provision the certificates for the webhookClientConfig
 
-	err := provisioner.Provision(false)
-	if err != nil {
-		// handle error
-	}
-
-Inject the webhookClientConfig into objects
-
-	// mutatingWebhookConfiguration is an instance of admissionregistritionv1beta1.MutatingWebhookConfiguration
-	// validatingWebhookConfiguration is an instance of admissionregistritionv1beta1.ValidatingWebhookConfiguration
-	err = provisioner.Inject(mutatingWebhookConfiguration, validatingWebhookConfiguration)
+	err := provisioner.Provision(Options{
+		ClientConfig: webhookClientConfig,
+		Objects: []runtime.Object{mutatingWebhookConfiguration, validatingWebhookConfiguration}
+	})
 	if err != nil {
 		// handle error
 	}

pkg/webhook/internal/cert/generator/doc.go

@@ -17,7 +17,7 @@ limitations under the License.
 /*
 Package generator provides an interface and implementation to provision certificates.
 
-Create an instance of CertGenerator.
+Create an instance of certGenerator.
 
 	cg := SelfSignedCertGenerator{}