From 85eab9141c7ca613adc99baba0448dc1a414279b Mon Sep 17 00:00:00 2001 From: Iuri Sitinschi Date: Fri, 13 May 2022 18:00:34 +0100 Subject: [PATCH] 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. --- pkg/builder/webhook.go | 15 ++- pkg/builder/webhook_test.go | 151 +++++++++++++++++++++++- pkg/webhook/admission/webhook.go | 28 ++++- pkg/webhook/admission/webhook_test.go | 55 +++++++++ pkg/webhook/webhook_integration_test.go | 3 +- 5 files changed, 244 insertions(+), 8 deletions(-) diff --git a/pkg/builder/webhook.go b/pkg/builder/webhook.go index 18feb1cd74..534e6d64cd 100644 --- a/pkg/builder/webhook.go +++ b/pkg/builder/webhook.go @@ -39,6 +39,7 @@ type WebhookBuilder struct { gvk schema.GroupVersionKind mgr manager.Manager config *rest.Config + recoverPanic bool } // WebhookManagedBy allows inform its manager.Manager. @@ -68,6 +69,12 @@ func (blder *WebhookBuilder) WithValidator(validator admission.CustomValidator) return blder } +// RecoverPanic indicates whether the panic caused by webhook should be recovered. +func (blder *WebhookBuilder) RecoverPanic() *WebhookBuilder { + blder.recoverPanic = true + return blder +} + // Complete builds the webhook. func (blder *WebhookBuilder) Complete() error { // Set the Config @@ -124,10 +131,10 @@ func (blder *WebhookBuilder) registerDefaultingWebhook() { func (blder *WebhookBuilder) getDefaultingWebhook() *admission.Webhook { if defaulter := blder.withDefaulter; defaulter != nil { - return admission.WithCustomDefaulter(blder.apiType, defaulter) + return admission.WithCustomDefaulter(blder.apiType, defaulter).WithRecoverPanic(blder.recoverPanic) } if defaulter, ok := blder.apiType.(admission.Defaulter); ok { - return admission.DefaultingWebhookFor(defaulter) + return admission.DefaultingWebhookFor(defaulter).WithRecoverPanic(blder.recoverPanic) } log.Info( "skip registering a mutating webhook, object does not implement admission.Defaulter or WithDefaulter wasn't called", @@ -153,10 +160,10 @@ func (blder *WebhookBuilder) registerValidatingWebhook() { func (blder *WebhookBuilder) getValidatingWebhook() *admission.Webhook { if validator := blder.withValidator; validator != nil { - return admission.WithCustomValidator(blder.apiType, validator) + return admission.WithCustomValidator(blder.apiType, validator).WithRecoverPanic(blder.recoverPanic) } if validator, ok := blder.apiType.(admission.Validator); ok { - return admission.ValidatingWebhookFor(validator) + return admission.ValidatingWebhookFor(validator).WithRecoverPanic(blder.recoverPanic) } log.Info( "skip registering a validating webhook, object does not implement admission.Validator or WithValidator wasn't called", diff --git a/pkg/builder/webhook_test.go b/pkg/builder/webhook_test.go index 80703a38ff..027d59da29 100644 --- a/pkg/builder/webhook_test.go +++ b/pkg/builder/webhook_test.go @@ -134,6 +134,72 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, w.Code).To(Equal(http.StatusNotFound)) }) + It("should scaffold a defaulting webhook which recovers from panics", func() { + By("creating a controller manager") + m, err := manager.New(cfg, manager.Options{}) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + + By("registering the type in the Scheme") + builder := scheme.Builder{GroupVersion: testDefaulterGVK.GroupVersion()} + builder.Register(&TestDefaulter{}, &TestDefaulterList{}) + err = builder.AddToScheme(m.GetScheme()) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + + err = WebhookManagedBy(m). + For(&TestDefaulter{Panic: true}). + RecoverPanic(). + Complete() + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + svr := m.GetWebhookServer() + ExpectWithOffset(1, svr).NotTo(BeNil()) + + reader := strings.NewReader(`{ + "kind":"AdmissionReview", + "apiVersion":"admission.k8s.io/` + admissionReviewVersion + `", + "request":{ + "uid":"07e52e8d-4513-11e9-a716-42010a800270", + "kind":{ + "group":"", + "version":"v1", + "kind":"TestDefaulter" + }, + "resource":{ + "group":"", + "version":"v1", + "resource":"testdefaulter" + }, + "namespace":"default", + "operation":"CREATE", + "object":{ + "replica":1, + "panic":true + }, + "oldObject":null + } +}`) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + // TODO: we may want to improve it to make it be able to inject dependencies, + // but not always try to load certs and return not found error. + err = svr.Start(ctx) + if err != nil && !os.IsNotExist(err) { + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + } + + By("sending a request to a mutating webhook path") + path := generateMutatePath(testDefaulterGVK) + 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) + ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK)) + By("sanity checking the response contains reasonable fields") + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: injected panic [recovered]`)) + }) + It("should scaffold a defaulting webhook with a custom defaulter", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) @@ -284,6 +350,73 @@ func runTests(admissionReviewVersion string) { ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":403`)) }) + It("should scaffold a validating webhook which recovers from panics", func() { + By("creating a controller manager") + m, err := manager.New(cfg, manager.Options{}) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + + By("registering the type in the Scheme") + builder := scheme.Builder{GroupVersion: testValidatorGVK.GroupVersion()} + builder.Register(&TestValidator{}, &TestValidatorList{}) + err = builder.AddToScheme(m.GetScheme()) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + + err = WebhookManagedBy(m). + For(&TestValidator{Panic: true}). + RecoverPanic(). + Complete() + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + svr := m.GetWebhookServer() + ExpectWithOffset(1, svr).NotTo(BeNil()) + + reader := strings.NewReader(`{ + "kind":"AdmissionReview", + "apiVersion":"admission.k8s.io/` + admissionReviewVersion + `", + "request":{ + "uid":"07e52e8d-4513-11e9-a716-42010a800270", + "kind":{ + "group":"", + "version":"v1", + "kind":"TestValidator" + }, + "resource":{ + "group":"", + "version":"v1", + "resource":"testvalidator" + }, + "namespace":"default", + "operation":"CREATE", + "object":{ + "replica":2, + "panic":true + } + } +}`) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + // TODO: we may want to improve it to make it be able to inject dependencies, + // but not always try to load certs and return not found error. + err = svr.Start(ctx) + if err != nil && !os.IsNotExist(err) { + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + } + + By("sending a request to a validating webhook path") + path := generateValidatePath(testValidatorGVK) + _, err = reader.Seek(0, 0) + ExpectWithOffset(1, err).NotTo(HaveOccurred()) + 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) + ExpectWithOffset(1, w.Code).To(Equal(http.StatusOK)) + By("sanity checking the response contains reasonable field") + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"allowed":false`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"code":500`)) + ExpectWithOffset(1, w.Body).To(ContainSubstring(`"message":"panic: injected panic [recovered]`)) + }) + It("should scaffold a validating webhook with a custom validator", func() { By("creating a controller manager") m, err := manager.New(cfg, manager.Options{}) @@ -540,7 +673,8 @@ func runTests(admissionReviewVersion string) { var _ runtime.Object = &TestDefaulter{} type TestDefaulter struct { - Replica int `json:"replica,omitempty"` + Replica int `json:"replica,omitempty"` + Panic bool `json:"panic,omitempty"` } var testDefaulterGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: "TestDefaulter"} @@ -566,6 +700,9 @@ func (*TestDefaulterList) GetObjectKind() schema.ObjectKind { return nil } func (*TestDefaulterList) DeepCopyObject() runtime.Object { return nil } func (d *TestDefaulter) Default() { + if d.Panic { + panic("injected panic") + } if d.Replica < 2 { d.Replica = 2 } @@ -575,7 +712,8 @@ func (d *TestDefaulter) Default() { var _ runtime.Object = &TestValidator{} type TestValidator struct { - Replica int `json:"replica,omitempty"` + Replica int `json:"replica,omitempty"` + Panic bool `json:"panic,omitempty"` } var testValidatorGVK = schema.GroupVersionKind{Group: "foo.test.org", Version: "v1", Kind: "TestValidator"} @@ -603,6 +741,9 @@ func (*TestValidatorList) DeepCopyObject() runtime.Object { return nil } var _ admission.Validator = &TestValidator{} func (v *TestValidator) ValidateCreate() error { + if v.Panic { + panic("injected panic") + } if v.Replica < 0 { return errors.New("number of replica should be greater than or equal to 0") } @@ -610,6 +751,9 @@ func (v *TestValidator) ValidateCreate() error { } func (v *TestValidator) ValidateUpdate(old runtime.Object) error { + if v.Panic { + panic("injected panic") + } if v.Replica < 0 { return errors.New("number of replica should be greater than or equal to 0") } @@ -622,6 +766,9 @@ func (v *TestValidator) ValidateUpdate(old runtime.Object) error { } func (v *TestValidator) ValidateDelete() error { + if v.Panic { + panic("injected panic") + } if v.Replica > 0 { return errors.New("number of replica should be less than or equal to 0 to delete") } diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go index 3dcff5fadd..c97ebfc772 100644 --- a/pkg/webhook/admission/webhook.go +++ b/pkg/webhook/admission/webhook.go @@ -19,6 +19,7 @@ package admission import ( "context" "errors" + "fmt" "net/http" "github.com/go-logr/logr" @@ -27,6 +28,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/util/json" + utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/client-go/kubernetes/scheme" logf "sigs.k8s.io/controller-runtime/pkg/internal/log" @@ -121,6 +123,9 @@ type Webhook struct { // and potentially patches to apply to the handler. Handler Handler + // RecoverPanic indicates whether the panic caused by webhook should be recovered. + RecoverPanic bool + // WithContextFunc will allow you to take the http.Request.Context() and // add any additional information such as passing the request path or // headers thus allowing you to read them from within the handler @@ -138,11 +143,32 @@ func (wh *Webhook) InjectLogger(l logr.Logger) error { return nil } +// WithRecoverPanic takes a bool flag which indicates whether the panic caused by webhook should be recovered. +func (wh *Webhook) WithRecoverPanic(recoverPanic bool) *Webhook { + wh.RecoverPanic = recoverPanic + return wh +} + // Handle processes AdmissionRequest. // If the webhook is mutating type, it delegates the AdmissionRequest to each handler and merge the patches. // If the webhook is validating type, it delegates the AdmissionRequest to each handler and // deny the request if anyone denies. -func (wh *Webhook) Handle(ctx context.Context, req Request) Response { +func (wh *Webhook) Handle(ctx context.Context, req Request) (response Response) { + defer func() { + if r := recover(); r != nil { + if wh.RecoverPanic { + for _, fn := range utilruntime.PanicHandlers { + fn(r) + } + response = Errored(http.StatusInternalServerError, fmt.Errorf("panic: %v [recovered]", r)) + return + } + + wh.log.Info(fmt.Sprintf("Observed a panic in reconciler: %v", r)) + panic(r) + } + }() + resp := wh.Handler.Handle(ctx, req) if err := resp.Complete(req); err != nil { wh.log.Error(err, "unable to encode response") diff --git a/pkg/webhook/admission/webhook_test.go b/pkg/webhook/admission/webhook_test.go index 73b0be1694..607422ee0c 100644 --- a/pkg/webhook/admission/webhook_test.go +++ b/pkg/webhook/admission/webhook_test.go @@ -192,6 +192,61 @@ var _ = Describe("Admission Webhooks", func() { Expect(handler.dep.decoder).NotTo(BeNil()) }) }) + + Describe("panic recovery", func() { + It("should recover panic if RecoverPanic is true", func() { + panicHandler := func() *Webhook { + handler := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + panic("injected panic") + }, + } + webhook := &Webhook{ + Handler: handler, + RecoverPanic: true, + log: logf.RuntimeLog.WithName("webhook"), + } + + return webhook + } + + By("setting up a webhook with a panicking handler") + webhook := panicHandler() + + By("invoking the webhook") + resp := webhook.Handle(context.Background(), Request{}) + + By("checking that it errored the request") + Expect(resp.Allowed).To(BeFalse()) + Expect(resp.Result.Code).To(Equal(int32(http.StatusInternalServerError))) + Expect(resp.Result.Message).To(Equal("panic: injected panic [recovered]")) + }) + + It("should not recover panic if RecoverPanic is false by default", func() { + panicHandler := func() *Webhook { + handler := &fakeHandler{ + fn: func(ctx context.Context, req Request) Response { + panic("injected panic") + }, + } + webhook := &Webhook{ + Handler: handler, + log: logf.RuntimeLog.WithName("webhook"), + } + + return webhook + } + + By("setting up a webhook with a panicking handler") + defer func() { + Expect(recover()).ShouldNot(BeNil()) + }() + webhook := panicHandler() + + By("invoking the webhook") + webhook.Handle(context.Background(), Request{}) + }) + }) }) type stringInjector interface { diff --git a/pkg/webhook/webhook_integration_test.go b/pkg/webhook/webhook_integration_test.go index 029a503b4b..2137d3c98f 100644 --- a/pkg/webhook/webhook_integration_test.go +++ b/pkg/webhook/webhook_integration_test.go @@ -181,7 +181,8 @@ var _ = Describe("Webhook", func() { &admissiontest.FakeValidator{ ErrorToReturn: errors.New("Always denied"), GVKToReturn: schema.GroupVersionKind{Group: "apps", Version: "v1", Kind: "Deployment"}, - }), admission.StandaloneOptions{}) + }, + ), admission.StandaloneOptions{}) Expect(err).NotTo(HaveOccurred()) http.Handle("/failing", hook)