Recover panic for controller reconcile and webhook handler

Signed-off-by: FillZpp <FillZpp.pub@gmail.com>

pkg/internal/controller/controller.go

@@ -95,7 +95,10 @@ type watchDescription struct {
 }
 
 // Reconcile implements reconcile.Reconciler.
-func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (reconcile.Result, error) {
+func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (_ reconcile.Result, err error) {
+	defer utilruntime.HandleCrash(func(r interface{}) {
+		err = fmt.Errorf("panic: %v [recovered]", r)
+	})
 	log := c.Log.WithValues("name", req.Name, "namespace", req.Namespace)
 	ctx = logf.IntoContext(ctx, log)
 	return c.Do.Reconcile(ctx, req)
@@ -295,7 +298,7 @@ func (c *Controller) reconcileHandler(ctx context.Context, obj interface{}) {
 
 	// RunInformersAndControllers the syncHandler, passing it the Namespace/Name string of the
 	// resource to be synced.
-	result, err := c.Do.Reconcile(ctx, req)
+	result, err := c.Reconcile(ctx, req)
 	switch {
 	case err != nil:
 		c.Queue.AddRateLimited(req)

pkg/internal/controller/controller_test.go

@@ -31,6 +31,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"k8s.io/client-go/util/workqueue"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/cache/informertest"
@@ -88,6 +89,42 @@ var _ = Describe("controller", func() {
 			Expect(err).NotTo(HaveOccurred())
 			Expect(result).To(Equal(reconcile.Result{Requeue: true}))
 		})
+
+		It("should not recover panic if utilruntime.ReallyCrash is true by default", func() {
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			defer func() {
+				Expect(recover()).ShouldNot(BeNil())
+			}()
+			ctrl.Do = reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) {
+				var res *reconcile.Result
+				return *res, nil
+			})
+			_, _ = ctrl.Reconcile(ctx,
+				reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}})
+			By("this should not be called")
+			Fail("this should not be called")
+		})
+
+		It("should recover panic if utilruntime.ReallyCrash is false", func() {
+			ctx, cancel := context.WithCancel(context.Background())
+			defer cancel()
+
+			utilruntime.ReallyCrash = false
+			defer func() {
+				Expect(recover()).To(BeNil())
+				utilruntime.ReallyCrash = true
+			}()
+			ctrl.Do = reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) {
+				var res *reconcile.Result
+				return *res, nil
+			})
+			_, err := ctrl.Reconcile(ctx,
+				reconcile.Request{NamespacedName: types.NamespacedName{Namespace: "foo", Name: "bar"}})
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("[recovered]"))
+		})
 	})
 
 	Describe("Start", func() {

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"
@@ -142,8 +144,11 @@ 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 {
-	resp := wh.Handler.Handle(ctx, req)
+func (wh *Webhook) Handle(ctx context.Context, req Request) (resp Response) {
+	defer utilruntime.HandleCrash(func(r interface{}) {
+		resp = Errored(http.StatusInternalServerError, fmt.Errorf("panic: %v [recovered]", r))
+	})
+	resp = wh.Handler.Handle(ctx, req)
 	if err := resp.Complete(req); err != nil {
 		wh.log.Error(err, "unable to encode response")
 		return Errored(http.StatusInternalServerError, errUnableToEncodeResponse)

pkg/webhook/admission/webhook_test.go

@@ -28,6 +28,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	machinerytypes "k8s.io/apimachinery/pkg/types"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 
 	logf "sigs.k8s.io/controller-runtime/pkg/internal/log"
 	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
@@ -125,6 +126,58 @@ var _ = Describe("Admission Webhooks", func() {
 		Expect(resp.Patch).To(Equal([]byte(`[{"op":"add","path":"/a","value":2},{"op":"replace","path":"/b","value":4}]`)))
 	})
 
+	It("should not recover panic if utilruntime.ReallyCrash is true by default", func() {
+		defer func() {
+			Expect(recover()).ShouldNot(BeNil())
+		}()
+		By("setting up a webhook with an panic handler")
+		webhook := &Webhook{
+			Handler: HandlerFunc(func(ctx context.Context, req Request) Response {
+				_ = *req.DryRun
+				return Response{
+					AdmissionResponse: admissionv1.AdmissionResponse{
+						Allowed: true,
+					},
+				}
+			}),
+			log: logf.RuntimeLog.WithName("webhook"),
+		}
+
+		By("invoking the webhook")
+		_ = webhook.Handle(context.Background(), Request{})
+
+		By("this should not be called")
+		Fail("this should not be called")
+	})
+
+	It("should recover panic if utilruntime.ReallyCrash is false", func() {
+		utilruntime.ReallyCrash = false
+		defer func() {
+			Expect(recover()).Should(BeNil())
+			utilruntime.ReallyCrash = true
+		}()
+		By("setting up a webhook with an panic handler")
+		webhook := &Webhook{
+			Handler: HandlerFunc(func(ctx context.Context, req Request) Response {
+				_ = *req.DryRun
+				return Response{
+					AdmissionResponse: admissionv1.AdmissionResponse{
+						Allowed: true,
+					},
+				}
+			}),
+			log: logf.RuntimeLog.WithName("webhook"),
+		}
+
+		By("invoking the webhook")
+		resp := webhook.Handle(context.Background(), Request{})
+
+		By("response should be error with recover")
+		Expect(resp.Allowed).To(Equal(false))
+		Expect(resp.Result.Code).To(Equal(int32(http.StatusInternalServerError)))
+		Expect(resp.Result.Message).To(ContainSubstring("[recovered]"))
+	})
+
 	Describe("dependency injection", func() {
 		It("should set dependencies passed in on the handler", func() {
 			By("setting up a webhook and injecting it with a injection func that injects a string")

pkg/webhook/conversion/conversion.go

@@ -31,6 +31,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
+	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
 	"sigs.k8s.io/controller-runtime/pkg/conversion"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 )
@@ -89,7 +90,10 @@ func (wh *Webhook) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 }
 
 // handles a version conversion request.
-func (wh *Webhook) handleConvertRequest(req *apix.ConversionRequest) (*apix.ConversionResponse, error) {
+func (wh *Webhook) handleConvertRequest(req *apix.ConversionRequest) (_ *apix.ConversionResponse, retErr error) {
+	defer utilruntime.HandleCrash(func(r interface{}) {
+		retErr = fmt.Errorf("panic: %v [recovered]", r)
+	})
 	if req == nil {
 		return nil, fmt.Errorf("conversion request is nil")
 	}