Skip to content

Commit 037bde6

Browse files
isitinschiisitinschi
committed
webhook: add an option to recover from panics in handler
Currently, a panic occcurence in a webhook handler is not recovered and crashes the webhook server. This change adds an option to Webhook to recover panics similar to how it is handled in Reconciler. It ensures that panics are converted to normal error response.
1 parent 15154aa commit 037bde6

File tree

4 files changed

+239
-7
lines changed

4 files changed

+239
-7
lines changed

‎pkg/builder/webhook.go

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ type WebhookBuilder struct {
3939
gvk schema.GroupVersionKind
4040
mgr manager.Manager
4141
config *rest.Config
42+
recoverPanic bool
4243
}
4344

4445
// WebhookManagedBy allows inform its manager.Manager.
@@ -68,6 +69,12 @@ func (blder *WebhookBuilder) WithValidator(validator admission.CustomValidator)
6869
return blder
6970
}
7071

72+
// RecoverPanic indicates whether the panic caused by webhook should be recovered.
73+
func (blder *WebhookBuilder) RecoverPanic() *WebhookBuilder {
74+
blder.recoverPanic = true
75+
return blder
76+
}
77+
7178
// Complete builds the webhook.
7279
func (blder *WebhookBuilder) Complete() error {
7380
// Set the Config
@@ -124,10 +131,10 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() {
124131

125132
func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook {
126133
if defaulter := blder.withDefaulter; defaulter != nil {
127-
return admission.WithCustomDefaulter(blder.apiType, defaulter)
134+
return admission.WithCustomDefaulter(blder.apiType, defaulter).WithRecoverPanic(blder.recoverPanic)
128135
}
129136
if defaulter, ok := blder.apiType.(admission.Defaulter); ok {
130-
return admission.DefaultingWebhookFor(defaulter)
137+
return admission.DefaultingWebhookFor(defaulter).WithRecoverPanic(blder.recoverPanic)
131138
}
132139
log.Info(
133140
"skip registering a mutating webhook, object does not implement admission.Defaulter or WithDefaulter wasn't called",
@@ -153,10 +160,10 @@ func (blder *WebhookBuilder) registerValidatingWebhook() {
153160

154161
func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook {
155162
if validator := blder.withValidator; validator != nil {
156-
return admission.WithCustomValidator(blder.apiType, validator)
163+
return admission.WithCustomValidator(blder.apiType, validator).WithRecoverPanic(blder.recoverPanic)
157164
}
158165
if validator, ok := blder.apiType.(admission.Validator); ok {
159-
return admission.ValidatingWebhookFor(validator)
166+
return admission.ValidatingWebhookFor(validator).WithRecoverPanic(blder.recoverPanic)
160167
}
161168
log.Info(
162169
"skip registering a validating webhook, object does not implement admission.Validator or WithValidator wasn't called",

‎pkg/builder/webhook_test.go

Lines changed: 149 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -134,6 +134,72 @@ func runTests(admissionReviewVersion string) {
134134
ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound))
135135
})
136136

137+
It("should scaffold a defaulting webhook which recovers from panics", func() {
138+
By("creating a controller manager")
139+
m, err := manager.New(cfg, manager.Options{})
140+
ExpectWithOffset(1, err).NotTo(HaveOccurred())
141+
142+
By("registering the type in the Scheme")
143+
builder := scheme.Builder{GroupVersion: testDefaulterGVK.GroupVersion()}
144+
builder.Register(&TestDefaulter{}, &TestDefaulterList{})
145+
err = builder.AddToScheme(m.GetScheme())
146+
ExpectWithOffset(1, err).NotTo(HaveOccurred())
147+
148+
err = WebhookManagedBy(m).
149+
For(&TestDefaulter{Panic: true}).
150+
RecoverPanic().
151+
Complete()
152+
ExpectWithOffset(1, err).NotTo(HaveOccurred())
153+
svr := m.GetWebhookServer()
154+
ExpectWithOffset(1, svr).NotTo(BeNil())
155+
156+
reader := strings.NewReader(`{
157+
"kind":"AdmissionReview",
158+
"apiVersion":"admission.k8s.io/` + admissionReviewVersion + `",
159+
"request":{
160+
"uid":"07e52e8d-4513-11e9-a716-42010a800270",
161+
"kind":{
162+
"group":"",
163+
"version":"v1",
164+
"kind":"TestDefaulter"
165+
},
166+
"resource":{
167+
"group":"",
168+
"version":"v1",
169+
"resource":"testdefaulter"
170+
},
171+
"namespace":"default",
172+
"operation":"CREATE",
173+
"object":{
174+
"replica":1,
175+
"panic":true
176+
},
177+
"oldObject":null
178+
}
179+
}`)
180+
181+
ctx, cancel := context.WithCancel(context.Background())
182+
cancel()
183+
// TODO: we may want to improve it to make it be able to inject dependencies,
184+
// but not always try to load certs and return not found error.
185+
err = svr.Start(ctx)
186+
if err != nil && !os.IsNotExist(err) {
187+
ExpectWithOffset(1, err).NotTo(HaveOccurred())
188+
}
189+
190+
By("sending a request to a mutating webhook path")
191+
path := generateMutatePath(testDefaulterGVK)
192+
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
193+
req.Header.Add("Content-Type", "application/json")
194+
w := httptest.NewRecorder()
195+
svr.WebhookMux.ServeHTTP(w, req)
196+
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
197+
By("sanity checking the response contains reasonable fields")
198+
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
199+
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`))
200+
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: injected panic [recovered]`))
201+
})
202+
137203
It("should scaffold a defaulting webhook with a custom defaulter", func() {
138204
By("creating a controller manager")
139205
m, err := manager.New(cfg, manager.Options{})
@@ -284,6 +350,73 @@ func runTests(admissionReviewVersion string) {
284350
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`))
285351
})
286352

353+
It("should scaffold a validating webhook which recovers from panics", func() {
354+
By("creating a controller manager")
355+
m, err := manager.New(cfg, manager.Options{})
356+
ExpectWithOffset(1, err).NotTo(HaveOccurred())
357+
358+
By("registering the type in the Scheme")
359+
builder := scheme.Builder{GroupVersion: testValidatorGVK.GroupVersion()}
360+
builder.Register(&TestValidator{}, &TestValidatorList{})
361+
err = builder.AddToScheme(m.GetScheme())
362+
ExpectWithOffset(1, err).NotTo(HaveOccurred())
363+
364+
err = WebhookManagedBy(m).
365+
For(&TestValidator{Panic: true}).
366+
RecoverPanic().
367+
Complete()
368+
ExpectWithOffset(1, err).NotTo(HaveOccurred())
369+
svr := m.GetWebhookServer()
370+
ExpectWithOffset(1, svr).NotTo(BeNil())
371+
372+
reader := strings.NewReader(`{
373+
"kind":"AdmissionReview",
374+
"apiVersion":"admission.k8s.io/` + admissionReviewVersion + `",
375+
"request":{
376+
"uid":"07e52e8d-4513-11e9-a716-42010a800270",
377+
"kind":{
378+
"group":"",
379+
"version":"v1",
380+
"kind":"TestValidator"
381+
},
382+
"resource":{
383+
"group":"",
384+
"version":"v1",
385+
"resource":"testvalidator"
386+
},
387+
"namespace":"default",
388+
"operation":"CREATE",
389+
"object":{
390+
"replica":2,
391+
"panic":true
392+
}
393+
}
394+
}`)
395+
396+
ctx, cancel := context.WithCancel(context.Background())
397+
cancel()
398+
// TODO: we may want to improve it to make it be able to inject dependencies,
399+
// but not always try to load certs and return not found error.
400+
err = svr.Start(ctx)
401+
if err != nil && !os.IsNotExist(err) {
402+
ExpectWithOffset(1, err).NotTo(HaveOccurred())
403+
}
404+
405+
By("sending a request to a validating webhook path")
406+
path := generateValidatePath(testValidatorGVK)
407+
_, err = reader.Seek(0, 0)
408+
ExpectWithOffset(1, err).NotTo(HaveOccurred())
409+
req := httptest.NewRequest("POST", "http://svc-name.svc-ns.svc"+path, reader)
410+
req.Header.Add("Content-Type", "application/json")
411+
w := httptest.NewRecorder()
412+
svr.WebhookMux.ServeHTTP(w, req)
413+
ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK))
414+
By("sanity checking the response contains reasonable field")
415+
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`))
416+
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`))
417+
ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: injected panic [recovered]`))
418+
})
419+
287420
It("should scaffold a validating webhook with a custom validator", func() {
288421
By("creating a controller manager")
289422
m, err := manager.New(cfg, manager.Options{})
@@ -542,7 +675,8 @@ var _ runtime.Object = &TestDefaulter{}
542675
const testDefaulterKind = "TestDefaulter"
543676

544677
type TestDefaulter struct {
545-
Replica int `json:"replica,omitempty"`
678+
Replica int `json:"replica,omitempty"`
679+
Panic bool `json:"panic,omitempty"`
546680
}
547681

548682
var testDefaulterGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: testDefaulterKind}
@@ -568,6 +702,9 @@ func (*TestDefaulterList) GetObjectKind() schema.ObjectKind { return nil }
568702
func (*TestDefaulterList) DeepCopyObject() runtime.Object { return nil }
569703

570704
func (d *TestDefaulter) Default() {
705+
if d.Panic {
706+
panic("injected panic")
707+
}
571708
if d.Replica < 2 {
572709
d.Replica = 2
573710
}
@@ -579,7 +716,8 @@ var _ runtime.Object = &TestValidator{}
579716
const testValidatorKind = "TestValidator"
580717

581718
type TestValidator struct {
582-
Replica int `json:"replica,omitempty"`
719+
Replica int `json:"replica,omitempty"`
720+
Panic bool `json:"panic,omitempty"`
583721
}
584722

585723
var testValidatorGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: testValidatorKind}
@@ -607,13 +745,19 @@ func (*TestValidatorList) DeepCopyObject() runtime.Object { return nil }
607745
var _ admission.Validator = &TestValidator{}
608746

609747
func (v *TestValidator) ValidateCreate() error {
748+
if v.Panic {
749+
panic("injected panic")
750+
}
610751
if v.Replica < 0 {
611752
return errors.New("number of replica should be greater than or equal to 0")
612753
}
613754
return nil
614755
}
615756

616757
func (v *TestValidator) ValidateUpdate(old runtime.Object) error {
758+
if v.Panic {
759+
panic("injected panic")
760+
}
617761
if v.Replica < 0 {
618762
return errors.New("number of replica should be greater than or equal to 0")
619763
}
@@ -626,6 +770,9 @@ func (v *TestValidator) ValidateUpdate(old runtime.Object) error {
626770
}
627771

628772
func (v *TestValidator) ValidateDelete() error {
773+
if v.Panic {
774+
panic("injected panic")
775+
}
629776
if v.Replica > 0 {
630777
return errors.New("number of replica should be less than or equal to 0 to delete")
631778
}

‎pkg/webhook/admission/webhook.go

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ package admission
1919
import (
2020
"context"
2121
"errors"
22+
"fmt"
2223
"net/http"
2324

2425
"github.com/go-logr/logr"
@@ -27,6 +28,7 @@ import (
2728
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2829
"k8s.io/apimachinery/pkg/runtime"
2930
"k8s.io/apimachinery/pkg/util/json"
31+
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
3032
"k8s.io/client-go/kubernetes/scheme"
3133

3234
logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
@@ -121,6 +123,9 @@ type Webhook struct {
121123
// and potentially patches to apply to the handler.
122124
Handler Handler
123125

126+
// RecoverPanic indicates whether the panic caused by webhook should be recovered.
127+
RecoverPanic bool
128+
124129
// WithContextFunc will allow you to take the http.Request.Context() and
125130
// add any additional information such as passing the request path or
126131
// headers thus allowing you to read them from within the handler
@@ -138,11 +143,29 @@ func (wh *Webhook) InjectLogger(l logr.Logger) error {
138143
return nil
139144
}
140145

146+
// WithRecoverPanic takes a bool flag which indicates whether the panic caused by webhook should be recovered.
147+
func (wh *Webhook) WithRecoverPanic(recoverPanic bool) *Webhook {
148+
wh.RecoverPanic = recoverPanic
149+
return wh
150+
}
151+
141152
// Handle processes AdmissionRequest.
142153
// If the webhook is mutating type, it delegates the AdmissionRequest to each handler and merge the patches.
143154
// If the webhook is validating type, it delegates the AdmissionRequest to each handler and
144155
// deny the request if anyone denies.
145-
func (wh *Webhook) Handle(ctx context.Context, req Request) Response {
156+
func (wh *Webhook) Handle(ctx context.Context, req Request) (response Response) {
157+
if wh.RecoverPanic {
158+
defer func() {
159+
if r := recover(); r != nil {
160+
for _, fn := range utilruntime.PanicHandlers {
161+
fn(r)
162+
}
163+
response = Errored(http.StatusInternalServerError, fmt.Errorf("panic: %v [recovered]", r))
164+
return
165+
}
166+
}()
167+
}
168+
146169
resp := wh.Handler.Handle(ctx, req)
147170
if err := resp.Complete(req); err != nil {
148171
wh.log.Error(err, "unable to encode response")

‎pkg/webhook/admission/webhook_test.go

Lines changed: 55 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,61 @@ var _ = Describe("Admission Webhooks", func() {
192192
Expect(handler.dep.decoder).NotTo(BeNil())
193193
})
194194
})
195+
196+
Describe("panic recovery", func() {
197+
It("should recover panic if RecoverPanic is true", func() {
198+
panicHandler := func() *Webhook {
199+
handler := &fakeHandler{
200+
fn: func(ctx context.Context, req Request) Response {
201+
panic("injected panic")
202+
},
203+
}
204+
webhook := &Webhook{
205+
Handler: handler,
206+
RecoverPanic: true,
207+
log: logf.RuntimeLog.WithName("webhook"),
208+
}
209+
210+
return webhook
211+
}
212+
213+
By("setting up a webhook with a panicking handler")
214+
webhook := panicHandler()
215+
216+
By("invoking the webhook")
217+
resp := webhook.Handle(context.Background(), Request{})
218+
219+
By("checking that it errored the request")
220+
Expect(resp.Allowed).To(BeFalse())
221+
Expect(resp.Result.Code).To(Equal(int32(http.StatusInternalServerError)))
222+
Expect(resp.Result.Message).To(Equal("panic: injected panic [recovered]"))
223+
})
224+
225+
It("should not recover panic if RecoverPanic is false by default", func() {
226+
panicHandler := func() *Webhook {
227+
handler := &fakeHandler{
228+
fn: func(ctx context.Context, req Request) Response {
229+
panic("injected panic")
230+
},
231+
}
232+
webhook := &Webhook{
233+
Handler: handler,
234+
log: logf.RuntimeLog.WithName("webhook"),
235+
}
236+
237+
return webhook
238+
}
239+
240+
By("setting up a webhook with a panicking handler")
241+
defer func() {
242+
Expect(recover()).ShouldNot(BeNil())
243+
}()
244+
webhook := panicHandler()
245+
246+
By("invoking the webhook")
247+
webhook.Handle(context.Background(), Request{})
248+
})
249+
})
195250
})
196251

197252
var _ = Describe("Should be able to write/read admission.Request to/from context", func() {

0 commit comments

Comments
 (0)