Merge pull request #1500 from estroz/bugfix/inject-wh-server

🐛 add manager.Options.WebhookServer so webhook server can be set

Author: k8s-ci-robot

pkg/manager/internal.go

@@ -145,6 +145,9 @@ type controllerManager struct {
 	certDir string
 
 	webhookServer *webhook.Server
+	// webhookServerOnce will be called in GetWebhookServer() to optionally initialize
+	// webhookServer if unset, and Add() it to controllerManager.
+	webhookServerOnce sync.Once
 
 	// leaseDuration is the duration that non-leader candidates will
 	// wait to force acquire leadership.
@@ -332,32 +335,19 @@ func (cm *controllerManager) GetAPIReader() client.Reader {
 }
 
 func (cm *controllerManager) GetWebhookServer() *webhook.Server {
-	server, wasNew := func() (*webhook.Server, bool) {
-		cm.mu.Lock()
-		defer cm.mu.Unlock()
-
-		if cm.webhookServer != nil {
-			return cm.webhookServer, false
-		}
-
-		cm.webhookServer = &webhook.Server{
-			Port:    cm.port,
-			Host:    cm.host,
-			CertDir: cm.certDir,
+	cm.webhookServerOnce.Do(func() {
+		if cm.webhookServer == nil {
+			cm.webhookServer = &webhook.Server{
+				Port:    cm.port,
+				Host:    cm.host,
+				CertDir: cm.certDir,
+			}
 		}
-		return cm.webhookServer, true
-	}()
-
-	// only add the server if *we ourselves* just registered it.
-	// Add has its own lock, so just do this separately -- there shouldn't
-	// be a "race" in this lock gap because the condition is the population
-	// of cm.webhookServer, not anything to do with Add.
-	if wasNew {
-		if err := cm.Add(server); err != nil {
+		if err := cm.Add(cm.webhookServer); err != nil {
 			panic("unable to add webhook server to the controller manager")
 		}
-	}
-	return server
+	})
+	return cm.webhookServer
 }
 
 func (cm *controllerManager) GetLogger() logr.Logger {

pkg/manager/manager.go

@@ -209,17 +209,24 @@ type Options struct {
 	LivenessEndpointName string
 
 	// Port is the port that the webhook server serves at.
-	// It is used to set webhook.Server.Port.
+	// It is used to set webhook.Server.Port if WebhookServer is not set.
 	Port int
 	// Host is the hostname that the webhook server binds to.
-	// It is used to set webhook.Server.Host.
+	// It is used to set webhook.Server.Host if WebhookServer is not set.
 	Host string
 
 	// CertDir is the directory that contains the server key and certificate.
-	// if not set, webhook server would look up the server key and certificate in
+	// If not set, webhook server would look up the server key and certificate in
 	// {TempDir}/k8s-webhook-server/serving-certs. The server key and certificate
 	// must be named tls.key and tls.crt, respectively.
+	// It is used to set webhook.Server.CertDir if WebhookServer is not set.
 	CertDir string
+
+	// WebhookServer is an externally configured webhook.Server. By default,
+	// a Manager will create a default server using Port, Host, and CertDir;
+	// if this is set, the Manager will use this server instead.
+	WebhookServer *webhook.Server
+
 	// Functions to all for a user to customize the values that will be injected.
 
 	// NewCache is the function that will create the cache to be used
@@ -370,6 +377,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
 		port:                    options.Port,
 		host:                    options.Host,
 		certDir:                 options.CertDir,
+		webhookServer:           options.WebhookServer,
 		leaseDuration:           *options.LeaseDuration,
 		renewDeadline:           *options.RenewDeadline,
 		retryPeriod:             *options.RetryPeriod,

pkg/manager/manager_test.go

@@ -53,6 +53,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/recorder"
 	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
 )
 
 var _ = Describe("manger.Manager", func() {
@@ -273,6 +274,20 @@ var _ = Describe("manger.Manager", func() {
 			close(done)
 		})
 
+		It("should not initialize a webhook server if Options.WebhookServer is set", func(done Done) {
+			By("creating a manager with options")
+			m, err := New(cfg, Options{Port: 9441, WebhookServer: &webhook.Server{Port: 9440}})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(m).NotTo(BeNil())
+
+			By("checking the server contains the Port set on the webhook server and not passed to Options")
+			svr := m.GetWebhookServer()
+			Expect(svr).NotTo(BeNil())
+			Expect(svr.Port).To(Equal(9440))
+
+			close(done)
+		})
+
 		Context("with leader election enabled", func() {
 			It("should only cancel the leader election after all runnables are done", func() {
 				m, err := New(cfg, Options{