From f8c6ee427c500fb6ff8f8ee595110661c81da4e7 Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Thu, 29 Apr 2021 11:45:53 -0700 Subject: [PATCH 1/2] pkg/manager: add Options.WebhookServer to allow external configuration of a webhook server Signed-off-by: Eric Stroczynski --- pkg/manager/manager.go | 28 +++++++++++++++++++++++----- pkg/manager/manager_test.go | 21 +++++++++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 30ff9e2516..5ec7bfd391 100644 --- a/pkg/manager/manager.go +++ b/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 @@ -358,7 +365,7 @@ func New(config *rest.Config, options Options) (Manager, error) { return nil, err } - return &controllerManager{ + cm := &controllerManager{ cluster: cluster, recorderProvider: recorderProvider, resourceLock: resourceLock, @@ -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, @@ -379,7 +387,17 @@ func New(config *rest.Config, options Options) (Manager, error) { gracefulShutdownTimeout: *options.GracefulShutdownTimeout, internalProceduresStop: make(chan struct{}), leaderElectionStopped: make(chan struct{}), - }, nil + } + + // 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 } // 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 ce1281d224..9d9fbeff38 100644 --- a/pkg/manager/manager_test.go +++ b/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,26 @@ 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 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()) + 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{ From c51d1caa0b5f5215bb486ba3d833dc777767d538 Mon Sep 17 00:00:00 2001 From: Eric Stroczynski Date: Thu, 29 Apr 2021 12:57:19 -0700 Subject: [PATCH 2/2] use sync.Once to initialize webhook server Signed-off-by: Eric Stroczynski --- pkg/manager/internal.go | 36 +++++++++++++----------------------- pkg/manager/manager.go | 14 ++------------ pkg/manager/manager_test.go | 6 ------ 3 files changed, 15 insertions(+), 41 deletions(-) diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index 57f95ba5b3..2dc83c40b9 100644 --- a/pkg/manager/internal.go +++ b/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 { diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index 5ec7bfd391..21750a3185 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, @@ -387,17 +387,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())