webhook: use new admission-plugin backed validation and mutation handlers

Signed-off-by: James Munnelly <jmunnelly@apple.com>

cmd/webhook/app/BUILD.bazel

@@ -9,20 +9,15 @@ go_library(
         "//cmd/util:go_default_library",
         "//cmd/webhook/app/options:go_default_library",
         "//internal/apis/config/webhook:go_default_library",
+        "//internal/webhook:go_default_library",
         "//pkg/logs:go_default_library",
         "//pkg/util:go_default_library",
         "//pkg/util/feature:go_default_library",
         "//pkg/webhook:go_default_library",
         "//pkg/webhook/authority:go_default_library",
         "//pkg/webhook/configfile:go_default_library",
-        "//pkg/webhook/handlers:go_default_library",
-        "//pkg/webhook/server:go_default_library",
-        "//pkg/webhook/server/tls:go_default_library",
-        "@com_github_go_logr_logr//:go_default_library",
         "@com_github_spf13_cobra//:go_default_library",
         "@com_github_spf13_pflag//:go_default_library",
-        "@io_k8s_client_go//kubernetes:go_default_library",
-        "@io_k8s_client_go//tools/clientcmd:go_default_library",
         "@io_k8s_component_base//cli/flag:go_default_library",
     ],
 )

cmd/webhook/app/testing/BUILD.bazel

@@ -6,8 +6,8 @@ go_library(
     importpath = "github.com/jetstack/cert-manager/cmd/webhook/app/testing",
     visibility = ["//visibility:public"],
     deps = [
-        "//cmd/webhook/app:go_default_library",
         "//cmd/webhook/app/options:go_default_library",
+        "//internal/webhook:go_default_library",
         "//pkg/util/pki:go_default_library",
         "//pkg/webhook/server:go_default_library",
         "@com_github_go_logr_logr//testing:go_default_library",

cmd/webhook/app/testing/testwebhook.go

@@ -36,8 +36,8 @@ import (
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/utils/pointer"
 
-	"github.com/jetstack/cert-manager/cmd/webhook/app"
 	"github.com/jetstack/cert-manager/cmd/webhook/app/options"
+	"github.com/jetstack/cert-manager/internal/webhook"
 	"github.com/jetstack/cert-manager/pkg/util/pki"
 	"github.com/jetstack/cert-manager/pkg/webhook/server"
 )
@@ -56,7 +56,7 @@ type ServerOptions struct {
 	CAPEM []byte
 }
 
-func StartWebhookServer(t *testing.T, ctx context.Context, args []string, argumentsForNewServerWithOptions ...app.ServerOption) (ServerOptions, StopFunc) {
+func StartWebhookServer(t *testing.T, ctx context.Context, args []string, argumentsForNewServerWithOptions ...webhook.ServerOption) (ServerOptions, StopFunc) {
 	log := logtesting.NewTestLogger(t)
 
 	fs := pflag.NewFlagSet("testset", pflag.ExitOnError)
@@ -99,7 +99,7 @@ func StartWebhookServer(t *testing.T, ctx context.Context, args []string, argume
 	webhookConfig.HealthzPort = pointer.Int(0)
 
 	errCh := make(chan error)
-	srv, err := app.NewServerWithOptions(log, *webhookFlags, *webhookConfig, argumentsForNewServerWithOptions...)
+	srv, err := webhook.NewCertManagerWebhookServer(log, *webhookFlags, *webhookConfig, argumentsForNewServerWithOptions...)
 	if err != nil {
 		t.Fatal(err)
 	}

cmd/webhook/app/webhook.go

@@ -22,104 +22,28 @@ import (
 	"os"
 	"path/filepath"
 
-	"github.com/go-logr/logr"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
-	"k8s.io/client-go/kubernetes"
-	"k8s.io/client-go/tools/clientcmd"
 	cliflag "k8s.io/component-base/cli/flag"
 
 	cmdutil "github.com/jetstack/cert-manager/cmd/util"
 	"github.com/jetstack/cert-manager/cmd/webhook/app/options"
 	config "github.com/jetstack/cert-manager/internal/apis/config/webhook"
+	cmwebhook "github.com/jetstack/cert-manager/internal/webhook"
 	logf "github.com/jetstack/cert-manager/pkg/logs"
 	"github.com/jetstack/cert-manager/pkg/util"
 	utilfeature "github.com/jetstack/cert-manager/pkg/util/feature"
 	"github.com/jetstack/cert-manager/pkg/webhook"
 	"github.com/jetstack/cert-manager/pkg/webhook/authority"
 	"github.com/jetstack/cert-manager/pkg/webhook/configfile"
-	"github.com/jetstack/cert-manager/pkg/webhook/handlers"
-	"github.com/jetstack/cert-manager/pkg/webhook/server"
-	"github.com/jetstack/cert-manager/pkg/webhook/server/tls"
 )
 
-var validationHook handlers.ValidatingAdmissionHook = handlers.NewRegistryBackedValidator(logf.Log, webhook.Scheme, webhook.ValidationRegistry)
-var mutationHook handlers.MutatingAdmissionHook = handlers.NewRegistryBackedMutator(logf.Log, webhook.Scheme, webhook.MutationRegistry)
-var conversionHook handlers.ConversionHook = handlers.NewSchemeBackedConverter(logf.Log, webhook.Scheme)
-
-type ServerOption func(*server.Server)
-
-// WithConversionHandler allows you to override the handler for the `/convert`
-// endpoint in tests.
-func WithConversionHandler(handler handlers.ConversionHook) ServerOption {
-	return func(s *server.Server) {
-		s.ConversionWebhook = handler
-	}
-}
-
-func NewServerWithOptions(log logr.Logger, _ options.WebhookFlags, opts config.WebhookConfiguration, optionFunctions ...ServerOption) (*server.Server, error) {
-	restcfg, err := clientcmd.BuildConfigFromFlags(opts.APIServerHost, opts.KubeConfig)
-	if err != nil {
-		return nil, err
-	}
-
-	cl, err := kubernetes.NewForConfig(restcfg)
-	if err != nil {
-		return nil, fmt.Errorf("error creating kubernetes client: %s", err)
-	}
-	validationHook.InitPlugins(cl)
-
-	var source tls.CertificateSource
-	switch {
-	case opts.TLSConfig.FilesystemConfigProvided():
-		log.V(logf.InfoLevel).Info("using TLS certificate from local filesystem", "private_key_path", opts.TLSConfig.Filesystem.KeyFile, "certificate", opts.TLSConfig.Filesystem.CertFile)
-		source = &tls.FileCertificateSource{
-			CertPath: opts.TLSConfig.Filesystem.CertFile,
-			KeyPath:  opts.TLSConfig.Filesystem.KeyFile,
-		}
-	case opts.TLSConfig.DynamicConfigProvided():
-		restcfg, err := clientcmd.BuildConfigFromFlags("", opts.KubeConfig)
-		if err != nil {
-			return nil, err
-		}
-
-		log.V(logf.InfoLevel).Info("using dynamic certificate generating using CA stored in Secret resource", "secret_namespace", opts.TLSConfig.Dynamic.SecretNamespace, "secret_name", opts.TLSConfig.Dynamic.SecretName)
-		source = &tls.DynamicSource{
-			DNSNames: opts.TLSConfig.Dynamic.DNSNames,
-			Authority: &authority.DynamicAuthority{
-				SecretNamespace: opts.TLSConfig.Dynamic.SecretNamespace,
-				SecretName:      opts.TLSConfig.Dynamic.SecretName,
-				RESTConfig:      restcfg,
-			},
-		}
-	default:
-		log.V(logf.WarnLevel).Info("serving insecurely as tls certificate data not provided")
-	}
-
-	s := &server.Server{
-		ListenAddr:        fmt.Sprintf(":%d", *opts.SecurePort),
-		HealthzAddr:       fmt.Sprintf(":%d", *opts.HealthzPort),
-		EnablePprof:       opts.EnablePprof,
-		PprofAddr:         opts.PprofAddress,
-		CertificateSource: source,
-		CipherSuites:      opts.TLSConfig.CipherSuites,
-		MinTLSVersion:     opts.TLSConfig.MinTLSVersion,
-		ValidationWebhook: validationHook,
-		MutationWebhook:   mutationHook,
-		ConversionWebhook: conversionHook,
-	}
-	for _, f := range optionFunctions {
-		f(s)
-	}
-	return s, nil
-}
-
 const componentWebhook = "webhook"
 
 func NewServerCommand(stopCh <-chan struct{}) *cobra.Command {
 	ctx := cmdutil.ContextWithStopCh(context.Background(), stopCh)
 	log := logf.Log
-	ctx = logf.NewContext(ctx, log, "webhook")
+	ctx = logf.NewContext(ctx, log, componentWebhook)
 
 	cleanFlagSet := pflag.NewFlagSet(componentWebhook, pflag.ContinueOnError)
 	// Replaces all instances of `_` in flag names with `-`
@@ -195,7 +119,7 @@ func NewServerCommand(stopCh <-chan struct{}) *cobra.Command {
 				}
 			}
 
-			srv, err := NewServerWithOptions(log, *webhookFlags, *webhookConfig)
+			srv, err := cmwebhook.NewCertManagerWebhookServer(log, *webhookFlags, *webhookConfig)
 			if err != nil {
 				log.Error(err, "Failed initialising server")
 				os.Exit(1)

internal/webhook/BUILD.bazel

@@ -14,6 +14,7 @@ go_library(
         "//internal/apis/certmanager/install:go_default_library",
         "//internal/apis/config/webhook:go_default_library",
         "//internal/apis/meta/install:go_default_library",
+        "//internal/plugin:go_default_library",
         "//pkg/logs:go_default_library",
         "//pkg/webhook/admission:go_default_library",
         "//pkg/webhook/admission/initializer:go_default_library",

internal/webhook/webhook.go

@@ -45,9 +45,19 @@ import (
 
 var conversionHook handlers.ConversionHook = handlers.NewSchemeBackedConverter(logf.Log, Scheme)
 
+type ServerOption func(*server.Server)
+
+// WithConversionHandler allows you to override the handler for the `/convert`
+// endpoint in tests.
+func WithConversionHandler(handler handlers.ConversionHook) ServerOption {
+	return func(s *server.Server) {
+		s.ConversionWebhook = handler
+	}
+}
+
 // NewCertManagerWebhookServer creates a new webhook server configured with all cert-manager
 // resource types, validation, defaulting and conversion functions.
-func NewCertManagerWebhookServer(log logr.Logger, _ options.WebhookFlags, opts config.WebhookConfiguration) (*server.Server, error) {
+func NewCertManagerWebhookServer(log logr.Logger, _ options.WebhookFlags, opts config.WebhookConfiguration, optionFunctions ...ServerOption) (*server.Server, error) {
 	restcfg, err := clientcmd.BuildConfigFromFlags(opts.APIServerHost, opts.KubeConfig)
 	if err != nil {
 		return nil, err
@@ -59,12 +69,12 @@ func NewCertManagerWebhookServer(log logr.Logger, _ options.WebhookFlags, opts c
 	}
 
 	// Set up the admission chain
-	admissionHandler, err := buildAdmissionChain(log, cl)
+	admissionHandler, err := buildAdmissionChain(cl)
 	if err != nil {
 		return nil, err
 	}
 
-	return &server.Server{
+	s := &server.Server{
 		ListenAddr:        fmt.Sprintf(":%d", *opts.SecurePort),
 		HealthzAddr:       fmt.Sprintf(":%d", *opts.HealthzPort),
 		EnablePprof:       opts.EnablePprof,
@@ -75,13 +85,16 @@ func NewCertManagerWebhookServer(log logr.Logger, _ options.WebhookFlags, opts c
 		ValidationWebhook: admissionHandler,
 		MutationWebhook:   admissionHandler,
 		ConversionWebhook: conversionHook,
-		Log:               log,
+	}
-	}, nil
+	for _, fn := range optionFunctions {
+		fn(s)
+	}
+	return s, nil
 }
 
-func buildAdmissionChain(log logr.Logger, client kubernetes.Interface) (*admission.RequestHandler, error) {
+func buildAdmissionChain(client kubernetes.Interface) (*admission.RequestHandler, error) {
 	// Set up the admission chain
-	pluginHandler := admission.NewPlugins(log, Scheme)
+	pluginHandler := admission.NewPlugins(Scheme)
 	plugin.RegisterAllPlugins(pluginHandler)
 	authorizer, err := authorizerfactory.DelegatingAuthorizerConfig{
 		SubjectAccessReviewClient: client.AuthorizationV1(),
@@ -115,7 +128,6 @@ func buildCertificateSource(log logr.Logger, tlsConfig config.TLSConfig, restCfg
 		return &tls.FileCertificateSource{
 			CertPath: tlsConfig.Filesystem.CertFile,
 			KeyPath:  tlsConfig.Filesystem.KeyFile,
-			Log:      log,
 		}
 	case tlsConfig.DynamicConfigProvided():
 		log.V(logf.InfoLevel).Info("using dynamic certificate generating using CA stored in Secret resource", "secret_namespace", tlsConfig.Dynamic.SecretNamespace, "secret_name", tlsConfig.Dynamic.SecretName)
@@ -125,9 +137,7 @@ func buildCertificateSource(log logr.Logger, tlsConfig config.TLSConfig, restCfg
 				SecretNamespace: tlsConfig.Dynamic.SecretNamespace,
 				SecretName:      tlsConfig.Dynamic.SecretName,
 				RESTConfig:      restCfg,
-				Log:             log,
 			},
-			Log: log,
 		}
 	default:
 		log.V(logf.WarnLevel).Info("serving insecurely as tls certificate data not provided")

pkg/webhook/admission/BUILD.bazel

@@ -13,7 +13,6 @@ go_library(
     visibility = ["//:__subpackages__"],
     deps = [
         "//pkg/webhook/handlers:go_default_library",
-        "@com_github_go_logr_logr//:go_default_library",
         "@io_k8s_api//admission/v1:go_default_library",
         "@io_k8s_apimachinery//pkg/apis/meta/v1:go_default_library",
         "@io_k8s_apimachinery//pkg/runtime:go_default_library",
@@ -55,7 +54,6 @@ go_test(
     ],
     embed = [":go_default_library"],
     deps = [
-        "//pkg/logs/testing:go_default_library",
         "//pkg/webhook/admission/initializer:go_default_library",
         "//pkg/webhook/handlers/testdata/apis/testgroup:go_default_library",
         "//pkg/webhook/handlers/testdata/apis/testgroup/install:go_default_library",

pkg/webhook/admission/plugins.go

@@ -19,7 +19,6 @@ package admission
 import (
 	"fmt"
 
-	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 )
@@ -27,15 +26,13 @@ import (
 // Plugins manages initialising, registering and executing admission plugins
 // for both validation and mutation.
 type Plugins struct {
-	log     logr.Logger
 	decoder runtime.Decoder
 
 	pluginFactory map[string]Factory
 }
 
-func NewPlugins(log logr.Logger, scheme *runtime.Scheme) *Plugins {
+func NewPlugins(scheme *runtime.Scheme) *Plugins {
 	return &Plugins{
-		log:           log,
 		decoder:       serializer.NewCodecFactory(scheme).UniversalDecoder(),
 		pluginFactory: make(map[string]Factory),
 	}

pkg/webhook/admission/plugins_test.go

@@ -25,14 +25,13 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/kubernetes/fake"
 
-	log "github.com/jetstack/cert-manager/pkg/logs/testing"
 	"github.com/jetstack/cert-manager/pkg/webhook/admission"
 	"github.com/jetstack/cert-manager/pkg/webhook/admission/initializer"
 )
 
 func TestPlugins_InitializesNamedOnly(t *testing.T) {
 	scheme := runtime.NewScheme()
-	p := admission.NewPlugins(&log.TestLogger{T: t}, scheme)
+	p := admission.NewPlugins(scheme)
 
 	testPlugin1 := &testPlugin{}
 	p.Register("TestPlugin1", func() (admission.Interface, error) {
@@ -61,7 +60,7 @@ func TestPlugins_InitializesNamedOnly(t *testing.T) {
 
 func TestPlugins_FailsIfAnyPluginFails(t *testing.T) {
 	scheme := runtime.NewScheme()
-	p := admission.NewPlugins(&log.TestLogger{T: t}, scheme)
+	p := admission.NewPlugins(scheme)
 
 	testPlugin1 := &testPlugin{}
 	p.Register("TestPlugin1", func() (admission.Interface, error) {
@@ -90,7 +89,7 @@ func TestPlugins_FailsIfAnyPluginFails(t *testing.T) {
 
 func TestPlugins_FailsNonExistingPlugin(t *testing.T) {
 	scheme := runtime.NewScheme()
-	p := admission.NewPlugins(&log.TestLogger{T: t}, scheme)
+	p := admission.NewPlugins(scheme)
 
 	testPlugin1 := &testPlugin{}
 	p.Register("TestPlugin1", func() (admission.Interface, error) {
@@ -109,7 +108,7 @@ func TestPlugins_FailsNonExistingPlugin(t *testing.T) {
 
 func TestPlugins_FailsIfPluginFailsToBuild(t *testing.T) {
 	scheme := runtime.NewScheme()
-	p := admission.NewPlugins(&log.TestLogger{T: t}, scheme)
+	p := admission.NewPlugins(scheme)
 
 	testPlugin1 := &testPlugin{}
 	p.Register("TestPlugin1", func() (admission.Interface, error) {

test/integration/framework/BUILD.bazel

@@ -9,9 +9,9 @@ go_library(
     importpath = "github.com/jetstack/cert-manager/test/integration/framework",
     visibility = ["//visibility:public"],
     deps = [
-        "//cmd/webhook/app:go_default_library",
         "//cmd/webhook/app/testing:go_default_library",
         "//internal/test/paths:go_default_library",
+        "//internal/webhook:go_default_library",
         "//pkg/api:go_default_library",
         "//pkg/client/clientset/versioned:go_default_library",
         "//pkg/client/informers/externalversions:go_default_library",

test/integration/framework/apiserver.go

@@ -38,9 +38,9 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 
-	"github.com/jetstack/cert-manager/cmd/webhook/app"
 	webhooktesting "github.com/jetstack/cert-manager/cmd/webhook/app/testing"
 	"github.com/jetstack/cert-manager/internal/test/paths"
+	"github.com/jetstack/cert-manager/internal/webhook"
 	"github.com/jetstack/cert-manager/pkg/api"
 	"github.com/jetstack/cert-manager/pkg/webhook/handlers"
 	"github.com/jetstack/cert-manager/test/internal/apiserver"
@@ -87,7 +87,7 @@ func RunControlPlane(t *testing.T, ctx context.Context, optionFunctions ...RunCo
 
 	webhookOpts, stopWebhook := webhooktesting.StartWebhookServer(
 		t, ctx, []string{"--api-server-host=" + config.Host},
-		app.WithConversionHandler(options.webhookConversionHandler),
+		webhook.WithConversionHandler(options.webhookConversionHandler),
 	)
 
 	crds := readCustomResourcesAtPath(t, *options.crdsDir)