From e17b4ce31cb57fe7277be1ceeb5bff7bc95f730e Mon Sep 17 00:00:00 2001
From: Iuri Sitinschi <yukas66@gmail.com>
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/webhook/admission/webhook.go      | 23 ++++++++++-
 pkg/webhook/admission/webhook_test.go | 55 +++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 1 deletion(-)

diff --git a/pkg/webhook/admission/webhook.go b/pkg/webhook/admission/webhook.go
index 3dcff5fadd..6189c78eff 100644
--- a/pkg/webhook/admission/webhook.go
+++ b/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")
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 {