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 {