From 9424c04b44b08e7210bb1b2a8dbb6cce75c1ba88 Mon Sep 17 00:00:00 2001
From: Eric Stroczynski <ericstroczynski@gmail.com>
Date: Thu, 29 Apr 2021 12:57:19 -0700
Subject: [PATCH] use sync.Once to initialize webhook server

Signed-off-by: Eric Stroczynski <ericstroczynski@gmail.com>
---
 pkg/manager/internal.go     | 37 ++++++++++++++-----------------------
 pkg/manager/manager.go      | 15 +++------------
 pkg/manager/manager_test.go |  6 ------
 3 files changed, 17 insertions(+), 41 deletions(-)

diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go
index 57f95ba5b3..a3748bbc22 100644
--- a/pkg/manager/internal.go
+++ b/pkg/manager/internal.go
@@ -145,6 +145,10 @@ type controllerManager struct {
 	certDir string
 
 	webhookServer *webhook.Server
+	// webhookServerNeedsAdd will only be true once webhookServer was Add()-ed to controllerManager.
+	// This will be false until GetWebhookServer() is called.
+	webhookServerNeedsAdd bool
+	webhookServerOnce     sync.Once
 
 	// leaseDuration is the duration that non-leader candidates will
 	// wait to force acquire leadership.
@@ -332,32 +336,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 {
diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go
index 5ec7bfd391..0d7e2cb762 100644
--- a/pkg/manager/manager.go
+++ b/pkg/manager/manager.go
@@ -365,7 +365,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
 		return nil, err
 	}
 
-	cm := &controllerManager{
+	return &controllerManager{
 		cluster:                 cluster,
 		recorderProvider:        recorderProvider,
 		resourceLock:            resourceLock,
@@ -378,6 +378,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
 		host:                    options.Host,
 		certDir:                 options.CertDir,
 		webhookServer:           options.WebhookServer,
+		webhookServerNeedsAdd:   options.WebhookServer != nil,
 		leaseDuration:           *options.LeaseDuration,
 		renewDeadline:           *options.RenewDeadline,
 		retryPeriod:             *options.RetryPeriod,
@@ -387,17 +388,7 @@ func New(config *rest.Config, options Options) (Manager, error) {
 		gracefulShutdownTimeout: *options.GracefulShutdownTimeout,
 		internalProceduresStop:  make(chan struct{}),
 		leaderElectionStopped:   make(chan struct{}),
-	}
-
-	// A webhook server set by New's caller should be added now
-	// so GetWebhookServer can construct a new one if unset and add it only once.
-	if cm.webhookServer != nil {
-		if err := cm.Add(cm.webhookServer); err != nil {
-			return nil, err
-		}
-	}
-
-	return cm, nil
+	}, nil
 }
 
 // AndFrom will use a supplied type and convert to Options
diff --git a/pkg/manager/manager_test.go b/pkg/manager/manager_test.go
index 9d9fbeff38..59255aa54e 100644
--- a/pkg/manager/manager_test.go
+++ b/pkg/manager/manager_test.go
@@ -280,12 +280,6 @@ var _ = Describe("manger.Manager", func() {
 			Expect(err).NotTo(HaveOccurred())
 			Expect(m).NotTo(BeNil())
 
-			By("checking the webhook server was added to non-leader-election runnables")
-			Expect(m).To(BeAssignableToTypeOf(&controllerManager{}))
-			nonLERunnables := m.(*controllerManager).nonLeaderElectionRunnables
-			Expect(nonLERunnables).To(HaveLen(1))
-			Expect(nonLERunnables[0]).To(BeAssignableToTypeOf(&webhook.Server{}))
-
 			By("checking the server contains the Port set on the webhook server and not passed to Options")
 			svr := m.GetWebhookServer()
 			Expect(svr).NotTo(BeNil())