Skip to content

⚠️ Allow passing a custom webhook server #2293

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 4, 2023
Merged

⚠️ Allow passing a custom webhook server #2293

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions pkg/builder/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -229,10 +229,10 @@ func (blder *WebhookBuilder) getType() (runtime.Object, error) {
}

func (blder *WebhookBuilder) isAlreadyHandled(path string) bool {
if blder.mgr.GetWebhookServer().WebhookMux == nil {
if blder.mgr.GetWebhookServer().WebhookMux() == nil {
return false
}
h, p := blder.mgr.GetWebhookServer().WebhookMux.Handler(&http.Request{URL: &url.URL{Path: path}})
h, p := blder.mgr.GetWebhookServer().WebhookMux().Handler(&http.Request{URL: &url.URL{Path: path}})
if p == path && h != nil {
return true
}
Expand Down
28 changes: 14 additions & 14 deletions pkg/builder/webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ func runTests(admissionReviewVersion string) {
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable fields")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
Expand All @@ -139,7 +139,7 @@ func runTests(admissionReviewVersion string) {
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w = httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))
})

Expand Down Expand Up @@ -199,7 +199,7 @@ func runTests(admissionReviewVersion string) {
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable fields")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
Expand Down Expand Up @@ -266,7 +266,7 @@ func runTests(admissionReviewVersion string) {
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable fields")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
Expand All @@ -281,7 +281,7 @@ func runTests(admissionReviewVersion string) {
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w = httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))
})

Expand Down Expand Up @@ -341,7 +341,7 @@ func runTests(admissionReviewVersion string) {
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))

By("sending a request to a validating webhook path")
Expand All @@ -351,7 +351,7 @@ func runTests(admissionReviewVersion string) {
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w = httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable field")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
Expand Down Expand Up @@ -415,7 +415,7 @@ func runTests(admissionReviewVersion string) {
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable field")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
Expand Down Expand Up @@ -484,7 +484,7 @@ func runTests(admissionReviewVersion string) {
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))

By("sending a request to a validating webhook path")
Expand All @@ -494,7 +494,7 @@ func runTests(admissionReviewVersion string) {
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w = httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable field")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
Expand Down Expand Up @@ -556,7 +556,7 @@ func runTests(admissionReviewVersion string) {
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable field")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
Expand All @@ -570,7 +570,7 @@ func runTests(admissionReviewVersion string) {
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w = httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable field")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
Expand Down Expand Up @@ -632,7 +632,7 @@ func runTests(admissionReviewVersion string) {
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w := httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable field")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
Expand Down Expand Up @@ -666,7 +666,7 @@ func runTests(admissionReviewVersion string) {
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
req.Header.Add("Content-Type", "application/json")
w = httptest.NewRecorder()
svr.WebhookMux.ServeHTTP(w, req)
svr.WebhookMux().ServeHTTP(w, req)
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
By("sanity checking the response contains reasonable field")
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
Expand Down
4 changes: 2 additions & 2 deletions pkg/manager/internal.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,7 @@ type controllerManager struct {
// election was configured.
elected chan struct{}

webhookServer *webhook.Server
webhookServer webhook.Server
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are some use cases that require an interface instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically anything where the default implementation isn't working. The case I have at hand is that the same binary also generates the certificates, so Start has to wait until they exist.

// webhookServerOnce will be called in GetWebhookServer() to optionally initialize
// webhookServer if unset, and Add() it to controllerManager.
webhookServerOnce sync.Once
Expand Down Expand Up @@ -276,7 +276,7 @@ func (cm *controllerManager) GetAPIReader() client.Reader {
return cm.cluster.GetAPIReader()
}

func (cm *controllerManager) GetWebhookServer() *webhook.Server {
func (cm *controllerManager) GetWebhookServer() webhook.Server {
cm.webhookServerOnce.Do(func() {
if cm.webhookServer == nil {
panic("webhook should not be nil")
Expand Down
12 changes: 6 additions & 6 deletions pkg/manager/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ type Manager interface {
Start(ctx context.Context) error

// GetWebhookServer returns a webhook.Server
GetWebhookServer() *webhook.Server
GetWebhookServer() webhook.Server

// GetLogger returns this manager's logger.
GetLogger() logr.Logger
Expand Down Expand Up @@ -306,7 +306,7 @@ type Options struct {
// 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
WebhookServer webhook.Server

// BaseContext is the function that provides Context values to Runnables
// managed by the Manager. If a BaseContext function isn't provided, Runnables
Expand Down Expand Up @@ -556,11 +556,11 @@ func (o Options) AndFrom(loader config.ControllerManagerConfiguration) (Options,
o.CertDir = newObj.Webhook.CertDir
}
if o.WebhookServer == nil {
o.WebhookServer = &webhook.Server{
o.WebhookServer = webhook.NewServer(webhook.Options{
Port: o.Port,
Host: o.Host,
CertDir: o.CertDir,
}
})
}

if newObj.Controller != nil {
Expand Down Expand Up @@ -733,12 +733,12 @@ func setOptionsDefaults(options Options) Options {
}

if options.WebhookServer == nil {
options.WebhookServer = &webhook.Server{
options.WebhookServer = webhook.NewServer(webhook.Options{
Host: options.Host,
Port: options.Port,
CertDir: options.CertDir,
TLSOpts: options.TLSOpts,
}
})
}

return options
Expand Down
41 changes: 28 additions & 13 deletions pkg/manager/manager_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,12 +226,12 @@ var _ = Describe("manger.Manager", func() {
HealthProbeBindAddress: "5000",
ReadinessEndpointName: "/readiness",
LivenessEndpointName: "/liveness",
WebhookServer: &webhook.Server{
WebhookServer: webhook.NewServer(webhook.Options{
Port: 8080,
Host: "example.com",
CertDir: "/pki",
TLSOpts: optionsTlSOptsFuncs,
},
}),
}.AndFrom(&fakeDeferredLoader{ccfg})
Expect(err).To(BeNil())

Expand All @@ -248,23 +248,23 @@ var _ = Describe("manger.Manager", func() {
Expect(m.HealthProbeBindAddress).To(Equal("5000"))
Expect(m.ReadinessEndpointName).To(Equal("/readiness"))
Expect(m.LivenessEndpointName).To(Equal("/liveness"))
Expect(m.WebhookServer.Port).To(Equal(8080))
Expect(m.WebhookServer.Host).To(Equal("example.com"))
Expect(m.WebhookServer.CertDir).To(Equal("/pki"))
Expect(m.WebhookServer.TLSOpts).To(Equal(optionsTlSOptsFuncs))
Expect(m.WebhookServer.(*webhook.DefaultServer).Options.Port).To(Equal(8080))
Expect(m.WebhookServer.(*webhook.DefaultServer).Options.Host).To(Equal("example.com"))
Expect(m.WebhookServer.(*webhook.DefaultServer).Options.CertDir).To(Equal("/pki"))
Expect(m.WebhookServer.(*webhook.DefaultServer).Options.TLSOpts).To(Equal(optionsTlSOptsFuncs))
})

It("should lazily initialize a webhook server if needed", func() {
By("creating a manager with options")
m, err := New(cfg, Options{WebhookServer: &webhook.Server{Port: 9440, Host: "foo.com"}})
m, err := New(cfg, Options{WebhookServer: webhook.NewServer(webhook.Options{Port: 9440, Host: "foo.com"})})
Expect(err).NotTo(HaveOccurred())
Expect(m).NotTo(BeNil())

By("checking options are passed to the webhook server")
svr := m.GetWebhookServer()
Expect(svr).NotTo(BeNil())
Expect(svr.Port).To(Equal(9440))
Expect(svr.Host).To(Equal("foo.com"))
Expect(svr.(*webhook.DefaultServer).Options.Port).To(Equal(9440))
Expect(svr.(*webhook.DefaultServer).Options.Host).To(Equal("foo.com"))
})

It("should lazily initialize a webhook server if needed (deprecated)", func() {
Expand All @@ -276,13 +276,13 @@ var _ = Describe("manger.Manager", func() {
By("checking options are passed to the webhook server")
svr := m.GetWebhookServer()
Expect(svr).NotTo(BeNil())
Expect(svr.Port).To(Equal(9440))
Expect(svr.Host).To(Equal("foo.com"))
Expect(svr.(*webhook.DefaultServer).Options.Port).To(Equal(9440))
Expect(svr.(*webhook.DefaultServer).Options.Host).To(Equal("foo.com"))
})

It("should not initialize a webhook server if Options.WebhookServer is set", func() {
By("creating a manager with options")
srv := &webhook.Server{Port: 9440}
srv := webhook.NewServer(webhook.Options{Port: 9440})
m, err := New(cfg, Options{Port: 9441, WebhookServer: srv})
Expect(err).NotTo(HaveOccurred())
Expect(m).NotTo(BeNil())
Expand All @@ -291,7 +291,22 @@ var _ = Describe("manger.Manager", func() {
svr := m.GetWebhookServer()
Expect(svr).NotTo(BeNil())
Expect(svr).To(Equal(srv))
Expect(svr.Port).To(Equal(9440))
Expect(svr.(*webhook.DefaultServer).Options.Port).To(Equal(9440))
})

It("should allow passing a custom webhook.Server implementation", func() {
type customWebhook struct {
webhook.Server
}
m, err := New(cfg, Options{WebhookServer: customWebhook{}})
Expect(err).NotTo(HaveOccurred())
Expect(m).NotTo(BeNil())

svr := m.GetWebhookServer()
Expect(svr).NotTo(BeNil())

_, isCustomWebhook := svr.(customWebhook)
Expect(isCustomWebhook).To(BeTrue())
})

Context("with leader election enabled", func() {
Expand Down
2 changes: 1 addition & 1 deletion pkg/manager/runnable_group.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ func (r *runnables) Add(fn Runnable) error {
return r.Caches.Add(fn, func(ctx context.Context) bool {
return runnable.GetCache().WaitForCacheSync(ctx)
})
case *webhook.Server:
case webhook.Server:
return r.Webhooks.Add(fn, nil)
case LeaderElectionRunnable:
if !runnable.NeedLeaderElection() {
Expand Down
2 changes: 1 addition & 1 deletion pkg/manager/runnable_group_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ var _ = Describe("runnables", func() {
})

It("should add webhooks to the appropriate group", func() {
webhook := &webhook.Server{}
webhook := webhook.NewServer(webhook.Options{})
r := newRunnables(defaultBaseContext, errCh)
Expect(r.Add(webhook)).To(Succeed())
Expect(r.Webhooks.startQueue).To(HaveLen(1))
Expand Down
8 changes: 4 additions & 4 deletions pkg/webhook/example_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ func Example() {
}

// Create a webhook server.
hookServer := &Server{
hookServer := NewServer(Options{
Port: 8443,
}
})
if err := mgr.Add(hookServer); err != nil {
panic(err)
}
Expand All @@ -88,9 +88,9 @@ func Example() {
// tls.crt and tls.key.
func ExampleServer_Start() {
// Create a webhook server
hookServer := &Server{
hookServer := NewServer(Options{
Port: 8443,
}
})

// Register the webhooks in the server.
hookServer.Register("/mutating", mutatingHook)
Expand Down
Loading