Skip to content

Commit d52d0fe

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 365ae09 commit d52d0fe

File tree

4 files changed

+242
-7
lines changed

4 files changed

+242
-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{})
@@ -540,7 +673,8 @@ func runTests(admissionReviewVersion string) {
540673
var _ runtime.Object = &TestDefaulter{}
541674

542675
type TestDefaulter struct {
543-
Replica int `json:"replica,omitempty"`
676+
Replica int `json:"replica,omitempty"`
677+
Panic bool `json:"panic,omitempty"`
544678
}
545679

546680
var testDefaulterGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: "TestDefaulter"}
@@ -566,6 +700,9 @@ func (*TestDefaulterList) GetObjectKind() schema.ObjectKind { return nil }
566700
func (*TestDefaulterList) DeepCopyObject() runtime.Object { return nil }
567701

568702
func (d *TestDefaulter) Default() {
703+
if d.Panic {
704+
panic("injected panic")
705+
}
569706
if d.Replica < 2 {
570707
d.Replica = 2
571708
}
@@ -575,7 +712,8 @@ func (d *TestDefaulter) Default() {
575712
var _ runtime.Object = &TestValidator{}
576713

577714
type TestValidator struct {
578-
Replica int `json:"replica,omitempty"`
715+
Replica int `json:"replica,omitempty"`
716+
Panic bool `json:"panic,omitempty"`
579717
}
580718

581719
var testValidatorGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: "TestValidator"}
@@ -603,13 +741,19 @@ func (*TestValidatorList) DeepCopyObject() runtime.Object { return nil }
603741
var _ admission.Validator = &TestValidator{}
604742

605743
func (v *TestValidator) ValidateCreate() error {
744+
if v.Panic {
745+
panic("injected panic")
746+
}
606747
if v.Replica < 0 {
607748
return errors.New("number of replica should be greater than or equal to 0")
608749
}
609750
return nil
610751
}
611752

612753
func (v *TestValidator) ValidateUpdate(old runtime.Object) error {
754+
if v.Panic {
755+
panic("injected panic")
756+
}
613757
if v.Replica < 0 {
614758
return errors.New("number of replica should be greater than or equal to 0")
615759
}
@@ -622,6 +766,9 @@ func (v *TestValidator) ValidateUpdate(old runtime.Object) error {
622766
}
623767

624768
func (v *TestValidator) ValidateDelete() error {
769+
if v.Panic {
770+
panic("injected panic")
771+
}
625772
if v.Replica > 0 {
626773
return errors.New("number of replica should be less than or equal to 0 to delete")
627774
}

‎pkg/webhook/admission/webhook.go

Lines changed: 27 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,32 @@ 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+
defer func() {
158+
if r := recover(); r != nil {
159+
if wh.RecoverPanic {
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+
wh.log.Info(fmt.Sprintf("Observed a panic in reconciler: %v", r))
168+
panic(r)
169+
}
170+
}()
171+
146172
resp := wh.Handler.Handle(ctx, req)
147173
if err := resp.Complete(req); err != nil {
148174
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
type stringInjector interface {

0 commit comments

Comments
 (0)