Merge pull request kubernetes-sigs#1900 from isitinschi/webhooks-recover-panics

✨ webhook: add an option to recover from panics in handler

Author: k8s-ci-robot

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",

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{})
@@ -542,7 +675,8 @@ var _ runtime.Object = &TestDefaulter{}
 const testDefaulterKind = "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: testDefaulterKind}
@@ -568,6 +702,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
 	}
@@ -579,7 +716,8 @@ var _ runtime.Object = &TestValidator{}
 const testValidatorKind = "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: testValidatorKind}
@@ -607,13 +745,19 @@ 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")
 	}
 	return nil
 }
 
 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")
 	}
@@ -626,6 +770,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")
 	}

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,29 @@ 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) {
+	if wh.RecoverPanic {
+		defer func() {
+			if r := recover(); r != nil {
+				for _, fn := range utilruntime.PanicHandlers {
+					fn(r)
+				}
+				response = Errored(http.StatusInternalServerError, fmt.Errorf("panic: %v [recovered]", r))
+				return
+			}
+		}()
+	}
+
 	resp := wh.Handler.Handle(ctx, req)
 	if err := resp.Complete(req); err != nil {
 		wh.log.Error(err, "unable to encode response")

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{})
+		})
+	})
 })
 
 var _ = Describe("Should be able to write/read admission.Request to/from context", func() {