Skip to content

Commit 91cdd8c

Browse files
alvaroalemanalvaroaleman
committed
:warn: Allow passing a custom webhook server
Currently it is impossible to pass a custom webhook server, because we reference the concrete type rather than an interface. Change this to an interface.
1 parent 2e57de7 commit 91cdd8c

File tree

11 files changed

+142
-80
lines changed

11 files changed

+142
-80
lines changed

‎pkg/builder/webhook.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -229,10 +229,10 @@ func (blder *WebhookBuilder) getType() (runtime.Object, error) {
229229
}
230230

231231
func (blder *WebhookBuilder) isAlreadyHandled(path string) bool {
232-
if blder.mgr.GetWebhookServer().WebhookMux == nil {
232+
if blder.mgr.GetWebhookServer().WebhookMux() == nil {
233233
return false
234234
}
235-
h, p := blder.mgr.GetWebhookServer().WebhookMux.Handler(&http.Request{URL: &url.URL{Path: path}})
235+
h, p := blder.mgr.GetWebhookServer().WebhookMux().Handler(&http.Request{URL: &url.URL{Path: path}})
236236
if p == path && h != nil {
237237
return true
238238
}

‎pkg/builder/webhook_test.go‎

Lines changed: 14 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ func runTests(admissionReviewVersion string) {
125125
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
126126
req.Header.Add("Content-Type", "application/json")
127127
w := httptest.NewRecorder()
128-
svr.WebhookMux.ServeHTTP(w, req)
128+
svr.WebhookMux().ServeHTTP(w, req)
129129
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
130130
By("sanity checking the response contains reasonable fields")
131131
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
@@ -139,7 +139,7 @@ func runTests(admissionReviewVersion string) {
139139
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
140140
req.Header.Add("Content-Type", "application/json")
141141
w = httptest.NewRecorder()
142-
svr.WebhookMux.ServeHTTP(w, req)
142+
svr.WebhookMux().ServeHTTP(w, req)
143143
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))
144144
})
145145

@@ -199,7 +199,7 @@ func runTests(admissionReviewVersion string) {
199199
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
200200
req.Header.Add("Content-Type", "application/json")
201201
w := httptest.NewRecorder()
202-
svr.WebhookMux.ServeHTTP(w, req)
202+
svr.WebhookMux().ServeHTTP(w, req)
203203
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
204204
By("sanity checking the response contains reasonable fields")
205205
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
@@ -266,7 +266,7 @@ func runTests(admissionReviewVersion string) {
266266
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
267267
req.Header.Add("Content-Type", "application/json")
268268
w := httptest.NewRecorder()
269-
svr.WebhookMux.ServeHTTP(w, req)
269+
svr.WebhookMux().ServeHTTP(w, req)
270270
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
271271
By("sanity checking the response contains reasonable fields")
272272
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
@@ -281,7 +281,7 @@ func runTests(admissionReviewVersion string) {
281281
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
282282
req.Header.Add("Content-Type", "application/json")
283283
w = httptest.NewRecorder()
284-
svr.WebhookMux.ServeHTTP(w, req)
284+
svr.WebhookMux().ServeHTTP(w, req)
285285
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))
286286
})
287287

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

347347
By("sending a request to a validating webhook path")
@@ -351,7 +351,7 @@ func runTests(admissionReviewVersion string) {
351351
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
352352
req.Header.Add("Content-Type", "application/json")
353353
w = httptest.NewRecorder()
354-
svr.WebhookMux.ServeHTTP(w, req)
354+
svr.WebhookMux().ServeHTTP(w, req)
355355
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
356356
By("sanity checking the response contains reasonable field")
357357
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
@@ -415,7 +415,7 @@ func runTests(admissionReviewVersion string) {
415415
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
416416
req.Header.Add("Content-Type", "application/json")
417417
w := httptest.NewRecorder()
418-
svr.WebhookMux.ServeHTTP(w, req)
418+
svr.WebhookMux().ServeHTTP(w, req)
419419
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
420420
By("sanity checking the response contains reasonable field")
421421
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
@@ -484,7 +484,7 @@ func runTests(admissionReviewVersion string) {
484484
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
485485
req.Header.Add("Content-Type", "application/json")
486486
w := httptest.NewRecorder()
487-
svr.WebhookMux.ServeHTTP(w, req)
487+
svr.WebhookMux().ServeHTTP(w, req)
488488
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))
489489

490490
By("sending a request to a validating webhook path")
@@ -494,7 +494,7 @@ func runTests(admissionReviewVersion string) {
494494
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
495495
req.Header.Add("Content-Type", "application/json")
496496
w = httptest.NewRecorder()
497-
svr.WebhookMux.ServeHTTP(w, req)
497+
svr.WebhookMux().ServeHTTP(w, req)
498498
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
499499
By("sanity checking the response contains reasonable field")
500500
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
@@ -556,7 +556,7 @@ func runTests(admissionReviewVersion string) {
556556
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
557557
req.Header.Add("Content-Type", "application/json")
558558
w := httptest.NewRecorder()
559-
svr.WebhookMux.ServeHTTP(w, req)
559+
svr.WebhookMux().ServeHTTP(w, req)
560560
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
561561
By("sanity checking the response contains reasonable field")
562562
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
@@ -570,7 +570,7 @@ func runTests(admissionReviewVersion string) {
570570
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
571571
req.Header.Add("Content-Type", "application/json")
572572
w = httptest.NewRecorder()
573-
svr.WebhookMux.ServeHTTP(w, req)
573+
svr.WebhookMux().ServeHTTP(w, req)
574574
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
575575
By("sanity checking the response contains reasonable field")
576576
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))
@@ -632,7 +632,7 @@ func runTests(admissionReviewVersion string) {
632632
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
633633
req.Header.Add("Content-Type", "application/json")
634634
w := httptest.NewRecorder()
635-
svr.WebhookMux.ServeHTTP(w, req)
635+
svr.WebhookMux().ServeHTTP(w, req)
636636
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
637637
By("sanity checking the response contains reasonable field")
638638
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
@@ -666,7 +666,7 @@ func runTests(admissionReviewVersion string) {
666666
req = httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
667667
req.Header.Add("Content-Type", "application/json")
668668
w = httptest.NewRecorder()
669-
svr.WebhookMux.ServeHTTP(w, req)
669+
svr.WebhookMux().ServeHTTP(w, req)
670670
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
671671
By("sanity checking the response contains reasonable field")
672672
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":true`))

‎pkg/manager/internal.go‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ type controllerManager struct {
129129
// election was configured.
130130
elected chan struct{}
131131

132-
webhookServer *webhook.Server
132+
webhookServer webhook.Server
133133
// webhookServerOnce will be called in GetWebhookServer() to optionally initialize
134134
// webhookServer if unset, and Add() it to controllerManager.
135135
webhookServerOnce sync.Once
@@ -276,7 +276,7 @@ func (cm *controllerManager) GetAPIReader() client.Reader {
276276
return cm.cluster.GetAPIReader()
277277
}
278278

279-
func (cm *controllerManager) GetWebhookServer() *webhook.Server {
279+
func (cm *controllerManager) GetWebhookServer() webhook.Server {
280280
cm.webhookServerOnce.Do(func() {
281281
if cm.webhookServer == nil {
282282
panic("webhook should not be nil")

‎pkg/manager/manager.go‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,7 @@ type Manager interface {
8686
Start(ctx context.Context) error
8787

8888
// GetWebhookServer returns a webhook.Server
89-
GetWebhookServer() *webhook.Server
89+
GetWebhookServer() webhook.Server
9090

9191
// GetLogger returns this manager's logger.
9292
GetLogger() logr.Logger
@@ -306,7 +306,7 @@ type Options struct {
306306
// WebhookServer is an externally configured webhook.Server. By default,
307307
// a Manager will create a default server using Port, Host, and CertDir;
308308
// if this is set, the Manager will use this server instead.
309-
WebhookServer *webhook.Server
309+
WebhookServer webhook.Server
310310

311311
// BaseContext is the function that provides Context values to Runnables
312312
// managed by the Manager. If a BaseContext function isn't provided, Runnables
@@ -556,11 +556,11 @@ func (o Options) AndFrom(loader config.ControllerManagerConfiguration) (Options,
556556
o.CertDir = newObj.Webhook.CertDir
557557
}
558558
if o.WebhookServer == nil {
559-
o.WebhookServer = &webhook.Server{
559+
o.WebhookServer = webhook.NewServer(webhook.Options{
560560
Port: o.Port,
561561
Host: o.Host,
562562
CertDir: o.CertDir,
563-
}
563+
})
564564
}
565565

566566
if newObj.Controller != nil {
@@ -733,12 +733,12 @@ func setOptionsDefaults(options Options) Options {
733733
}
734734

735735
if options.WebhookServer == nil {
736-
options.WebhookServer = &webhook.Server{
736+
options.WebhookServer = webhook.NewServer(webhook.Options{
737737
Host: options.Host,
738738
Port: options.Port,
739739
CertDir: options.CertDir,
740740
TLSOpts: options.TLSOpts,
741-
}
741+
})
742742
}
743743

744744
return options

‎pkg/manager/manager_test.go‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -226,12 +226,12 @@ var _ = Describe("manger.Manager", func() {
226226
HealthProbeBindAddress: "5000",
227227
ReadinessEndpointName: "/readiness",
228228
LivenessEndpointName: "/liveness",
229-
WebhookServer: &webhook.Server{
229+
WebhookServer: webhook.NewServer(webhook.Options{
230230
Port: 8080,
231231
Host: "example.com",
232232
CertDir: "/pki",
233233
TLSOpts: optionsTlSOptsFuncs,
234-
},
234+
}),
235235
}.AndFrom(&fakeDeferredLoader{ccfg})
236236
Expect(err).To(BeNil())
237237

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

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

263263
By("checking options are passed to the webhook server")
264264
svr := m.GetWebhookServer()
265265
Expect(svr).NotTo(BeNil())
266-
Expect(svr.Port).To(Equal(9440))
267-
Expect(svr.Host).To(Equal("foo.com"))
266+
Expect(svr.(*webhook.DefaultServer).Options.Port).To(Equal(9440))
267+
Expect(svr.(*webhook.DefaultServer).Options.Host).To(Equal("foo.com"))
268268
})
269269

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

283283
It("should not initialize a webhook server if Options.WebhookServer is set", func() {
284284
By("creating a manager with options")
285-
srv := &webhook.Server{Port: 9440}
285+
srv := webhook.NewServer(webhook.Options{Port: 9440})
286286
m, err := New(cfg, Options{Port: 9441, WebhookServer: srv})
287287
Expect(err).NotTo(HaveOccurred())
288288
Expect(m).NotTo(BeNil())
@@ -291,7 +291,22 @@ var _ = Describe("manger.Manager", func() {
291291
svr := m.GetWebhookServer()
292292
Expect(svr).NotTo(BeNil())
293293
Expect(svr).To(Equal(srv))
294-
Expect(svr.Port).To(Equal(9440))
294+
Expect(svr.(*webhook.DefaultServer).Options.Port).To(Equal(9440))
295+
})
296+
297+
It("should allow passing a custom webhook.Server implementation", func() {
298+
type customWebhook struct {
299+
webhook.Server
300+
}
301+
m, err := New(cfg, Options{WebhookServer: customWebhook{}})
302+
Expect(err).NotTo(HaveOccurred())
303+
Expect(m).NotTo(BeNil())
304+
305+
svr := m.GetWebhookServer()
306+
Expect(svr).NotTo(BeNil())
307+
308+
_, isCustomWebhook := svr.(customWebhook)
309+
Expect(isCustomWebhook).To(BeTrue())
295310
})
296311

297312
Context("with leader election enabled", func() {

‎pkg/manager/runnable_group.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -56,7 +56,7 @@ func (r *runnables) Add(fn Runnable) error {
5656
return r.Caches.Add(fn, func(ctx context.Context) bool {
5757
return runnable.GetCache().WaitForCacheSync(ctx)
5858
})
59-
case *webhook.Server:
59+
case webhook.Server:
6060
return r.Webhooks.Add(fn, nil)
6161
case LeaderElectionRunnable:
6262
if !runnable.NeedLeaderElection() {

‎pkg/manager/runnable_group_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ var _ = Describe("runnables", func() {
2929
})
3030

3131
It("should add webhooks to the appropriate group", func() {
32-
webhook := &webhook.Server{}
32+
webhook := webhook.NewServer(webhook.Options{})
3333
r := newRunnables(defaultBaseContext, errCh)
3434
Expect(r.Add(webhook)).To(Succeed())
3535
Expect(r.Webhooks.startQueue).To(HaveLen(1))

‎pkg/webhook/example_test.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -61,9 +61,9 @@ func Example() {
6161
}
6262

6363
// Create a webhook server.
64-
hookServer := &Server{
64+
hookServer := NewServer(Options{
6565
Port: 8443,
66-
}
66+
})
6767
if err := mgr.Add(hookServer); err != nil {
6868
panic(err)
6969
}
@@ -88,9 +88,9 @@ func Example() {
8888
// tls.crt and tls.key.
8989
func ExampleServer_Start() {
9090
// Create a webhook server
91-
hookServer := &Server{
91+
hookServer := NewServer(Options{
9292
Port: 8443,
93-
}
93+
})
9494

9595
// Register the webhooks in the server.
9696
hookServer.Register("/mutating", mutatingHook)

0 commit comments

Comments
 (0)