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.

Author: isitinschi

pkg/webhook/admission/webhook.go

@@ -19,8 +19,11 @@ package admission
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net/http"
 
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
+
 	"github.com/go-logr/logr"
 	jsonpatch "gomodules.xyz/jsonpatch/v2"
 	admissionv1 "k8s.io/api/admission/v1"
@@ -121,6 +124,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
@@ -142,7 +148,22 @@ func (wh *Webhook) InjectLogger(l logr.Logger) error {
 // 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")

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 {