Update tests and docs for StandaloneServer

Author: kevindelgado

pkg/webhook/admission/webhook.go

@@ -113,6 +113,9 @@ func (f HandlerFunc) Handle(ctx context.Context, req Request) Response {
 }
 
 // Webhook represents each individual webhook.
+//
+// It must be registered with a webhook.Server or
+// populated by StandaloneWebhook to be ran on an arbitrary HTTP server.
 type Webhook struct {
 	// Handler actually processes an admission request returning whether it was allowed or denied,
 	// and potentially patches to apply to the handler.
@@ -221,8 +224,11 @@ type StandaloneOptions struct {
 	Path string
 }
 
-// StandaloneWebhook transforms a Webhook that needs to be registered
-// on a webhook.Server into one that can be ran on any arbitrary mux.
+// StandaloneWebhook prepares a webhook for use without a webhook.Server,
+// passing in the information normally populated by webhook.Server
+// and instrumenting the webhook with metrics.
+//
+// Use this to attach your webhook to an arbitrary HTTP server or mux.
 func StandaloneWebhook(hook *Webhook, opts StandaloneOptions) (http.Handler, error) {
 	if opts.Scheme == nil {
 		opts.Scheme = scheme.Scheme

pkg/webhook/server.go

@@ -30,10 +30,10 @@ import (
 	"sync"
 
 	"k8s.io/apimachinery/pkg/runtime"
-	"k8s.io/client-go/kubernetes/scheme"
+	kscheme "k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/certwatcher"
 	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
-	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/internal/metrics"
 )
 
 // DefaultPort is the default port that the webhook server serves.
@@ -105,67 +105,6 @@ func (s *Server) setDefaults() {
 	}
 }
 
-// Options are the subset of fields on the controller that can be
-// configured when running an unmanaged webhook server (i.e. webhook.NewUnmanaged())
-type Options struct {
-	// Host is the address that the server will listen on.
-	// Defaults to "" - all addresses.
-	Host string
-
-	// Port is the port number that the server will serve.
-	// It will be defaulted to 9443 if unspecified.
-	Port int
-
-	// CertDir is the directory that contains the server key and certificate. The
-	// server key and certificate.
-	CertDir string
-
-	// CertName is the server certificate name. Defaults to tls.crt.
-	CertName string
-
-	// KeyName is the server key name. Defaults to tls.key.
-	KeyName string
-
-	// ClientCAName is the CA certificate name which server used to verify remote(client)'s certificate.
-	// Defaults to "", which means server does not verify client's certificate.
-	ClientCAName string
-
-	// WebhookMux is the multiplexer that handles different webhooks.
-	WebhookMux *http.ServeMux
-
-	// Scheme is the scheme used to resolve runtime.Objects to GroupVersionKinds / Resources
-	// Defaults to the kubernetes/client-go scheme.Scheme, but it's almost always better
-	// idea to pass your own scheme in.  See the documentation in pkg/scheme for more information.
-	Scheme *runtime.Scheme
-}
-
-// NewUnmanaged provides a webhook server that can be ran without
-// a controller manager.
-func NewUnmanaged(options Options) (*Server, error) {
-	server := &Server{
-		Host:       options.Host,
-		Port:       options.Port,
-		CertDir:    options.CertDir,
-		CertName:   options.CertName,
-		KeyName:    options.KeyName,
-		WebhookMux: options.WebhookMux,
-	}
-	server.setDefaults()
-	// Use the Kubernetes client-go scheme if none is specified
-	if options.Scheme == nil {
-		options.Scheme = scheme.Scheme
-	}
-
-	// TODO: can we do this without dep injection?
-	server.InjectFunc(func(i interface{}) error {
-		if _, err := inject.SchemeInto(options.Scheme, i); err != nil {
-			return err
-		}
-		return nil
-	})
-	return server, nil
-}
-
 // NeedLeaderElection implements the LeaderElectionRunnable interface, which indicates
 // the webhook server doesn't need leader election.
 func (*Server) NeedLeaderElection() bool {
@@ -185,7 +124,7 @@ func (s *Server) Register(path string, hook http.Handler) {
 	}
 	// TODO(directxman12): call setfields if we've already started the server
 	s.webhooks[path] = hook
-	s.WebhookMux.Handle(path, admission.InstrumentedHook(path, hook))
+	s.WebhookMux.Handle(path, metrics.InstrumentedHook(path, hook))
 
 	regLog := log.WithValues("path", path)
 	regLog.Info("registering webhook")
@@ -210,6 +149,26 @@ func (s *Server) Register(path string, hook http.Handler) {
 	}
 }
 
+// StartStandalone runs a webhook server without
+// a controller manager.
+func (s *Server) StartStandalone(ctx context.Context, scheme *runtime.Scheme) error {
+	// Use the Kubernetes client-go scheme if none is specified
+	if scheme == nil {
+		scheme = kscheme.Scheme
+	}
+
+	if err := s.InjectFunc(func(i interface{}) error {
+		if _, err := inject.SchemeInto(scheme, i); err != nil {
+			return err
+		}
+		return nil
+	}); err != nil {
+		return err
+	}
+
+	return s.Start(ctx)
+}
+
 // Start runs the server.
 // It will install the webhook related resources depend on the server configuration.
 func (s *Server) Start(ctx context.Context) error {

pkg/webhook/server_test.go

@@ -25,6 +25,7 @@ import (
 
 	. "github.com/onsi/ginkgo"
 	. "github.com/onsi/gomega"
+	"k8s.io/client-go/kubernetes/scheme"
 	"k8s.io/client-go/rest"
 	"sigs.k8s.io/controller-runtime/pkg/envtest"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
@@ -68,12 +69,12 @@ var _ = Describe("Webhook Server", func() {
 		Expect(servingOpts.Cleanup()).To(Succeed())
 	})
 
-	startServer := func() (done <-chan struct{}) {
+	genericStartServer := func(f func(ctx context.Context)) (done <-chan struct{}) {
 		doneCh := make(chan struct{})
 		go func() {
 			defer GinkgoRecover()
 			defer close(doneCh)
-			Expect(server.Start(ctx)).To(Succeed())
+			f(ctx)
 		}()
 		// wait till we can ping the server to start the test
 		Eventually(func() error {
@@ -93,6 +94,12 @@ var _ = Describe("Webhook Server", func() {
 		return doneCh
 	}
 
+	startServer := func() (done <-chan struct{}) {
+		return genericStartServer(func(ctx context.Context) {
+			Expect(server.Start(ctx)).To(Succeed())
+		})
+	}
+
 	// TODO(directxman12): figure out a good way to test all the serving setup
 	// with httptest.Server to get all the niceness from that.
 
@@ -175,32 +182,26 @@ var _ = Describe("Webhook Server", func() {
 		})
 	})
 
-	Context("when using an unmanaged webhook server", func() {
-		It("should serve a webhook on the requested path", func() {
-			opts := webhook.Options{
-				Host:    servingOpts.LocalServingHost,
-				Port:    servingOpts.LocalServingPort,
-				CertDir: servingOpts.LocalServingCertDir,
-			}
-			var err error
-			// overwrite the server so that startServer() starts it
-			server, err = webhook.NewUnmanaged(opts)
+	It("should serve be able to serve in unmanaged mode", func() {
+		server = &webhook.Server{
+			Host:    servingOpts.LocalServingHost,
+			Port:    servingOpts.LocalServingPort,
+			CertDir: servingOpts.LocalServingCertDir,
+		}
+		server.Register("/somepath", &testHandler{})
+		doneCh := genericStartServer(func(ctx context.Context) {
+			Expect(server.StartStandalone(ctx, scheme.Scheme))
+		})
 
+		Eventually(func() ([]byte, error) {
+			resp, err := client.Get(fmt.Sprintf("https://%s/somepath", testHostPort))
 			Expect(err).NotTo(HaveOccurred())
-			server.Register("/somepath", &testHandler{})
-			doneCh := startServer()
-
-			Eventually(func() ([]byte, error) {
-				resp, err := client.Get(fmt.Sprintf("https://%s/somepath", testHostPort))
-				Expect(err).NotTo(HaveOccurred())
-				defer resp.Body.Close()
-				return ioutil.ReadAll(resp.Body)
-			}).Should(Equal([]byte("gadzooks!")))
-
-			ctxCancel()
-			Eventually(doneCh, "4s").Should(BeClosed())
-		})
+			defer resp.Body.Close()
+			return ioutil.ReadAll(resp.Body)
+		}).Should(Equal([]byte("gadzooks!")))
 
+		ctxCancel()
+		Eventually(doneCh, "4s").Should(BeClosed())
 	})
 })
 

pkg/webhook/webhook_integration_test.go

@@ -16,6 +16,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/certwatcher"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -86,22 +87,20 @@ var _ = Describe("Webhook", func() {
 	})
 	Context("when running a webhook server without a manager", func() {
 		It("should reject create request for webhook that rejects all requests", func(done Done) {
-			opts := webhook.Options{
+			server := webhook.Server{
 				Port:    testenv.WebhookInstallOptions.LocalServingPort,
 				Host:    testenv.WebhookInstallOptions.LocalServingHost,
 				CertDir: testenv.WebhookInstallOptions.LocalServingCertDir,
 			}
-			server, err := webhook.NewUnmanaged(opts)
-			Expect(err).NotTo(HaveOccurred())
 			server.Register("/failing", &webhook.Admission{Handler: &rejectingValidator{}})
 
 			ctx, cancel := context.WithCancel(context.Background())
 			go func() {
-				_ = server.Start(ctx)
+				_ = server.StartStandalone(ctx, scheme.Scheme)
 			}()
 
 			Eventually(func() bool {
-				err = c.Create(context.TODO(), obj)
+				err := c.Create(context.TODO(), obj)
 				return errors.ReasonForError(err) == metav1.StatusReason("Always denied")
 			}, 1*time.Second).Should(BeTrue())