⚠️ Allow configuring RecoverPanic for controllers globally

This change allows configuring the RecoverPanic setting for controllers
globally. It is a breaking change as it requires changing the field type
of the setting to *bool from bool in order to be able to check if it has
been set already.

Author: alvaroaleman

pkg/config/v1alpha1/types.go

@@ -94,6 +94,10 @@ type ControllerConfigurationSpec struct {
 	// Defaults to 2 minutes if not set.
 	// +optional
 	CacheSyncTimeout *time.Duration `json:"cacheSyncTimeout,omitempty"`
+
+	// RecoverPanic indicates if panics should be recovered.
+	// +optional
+	RecoverPanic *bool `json:"recoverPanic,omitempty"`
 }
 
 // ControllerMetrics defines the metrics configs.

pkg/controller/controller.go

@@ -56,7 +56,8 @@ type Options struct {
 	CacheSyncTimeout time.Duration
 
 	// RecoverPanic indicates whether the panic caused by reconcile should be recovered.
-	RecoverPanic bool
+	// Defaults to the Controller.RecoverPanic setting from the Manager if unset.
+	RecoverPanic *bool
 }
 
 // Controller implements a Kubernetes API.  A Controller manages a work queue fed reconcile.Requests
@@ -139,6 +140,10 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller
 		return nil, err
 	}
 
+	if options.RecoverPanic == nil {
+		options.RecoverPanic = mgr.GetControllerOptions().RecoverPanic
+	}
+
 	// Create controller with dependencies set
 	return &controller.Controller{
 		Do: options.Reconciler,

pkg/controller/controller_test.go

@@ -27,9 +27,11 @@ import (
 	corev1 "k8s.io/api/core/v1"
 
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/config/v1alpha1"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
+	internalcontroller "sigs.k8s.io/controller-runtime/pkg/internal/controller"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 	"sigs.k8s.io/controller-runtime/pkg/runtime/inject"
@@ -142,6 +144,39 @@ var _ = Describe("controller.Controller", func() {
 			clientTransport.CloseIdleConnections()
 			Eventually(func() error { return goleak.Find(currentGRs) }).Should(Succeed())
 		})
+
+		It("should default RecoverPanic from the manager", func() {
+			m, err := manager.New(cfg, manager.Options{Controller: v1alpha1.ControllerConfigurationSpec{RecoverPanic: ptr(true)}})
+			Expect(err).NotTo(HaveOccurred())
+
+			c, err := controller.New("new-controller", m, controller.Options{
+				Reconciler: reconcile.Func(nil),
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			ctrl, ok := c.(*internalcontroller.Controller)
+			Expect(ok).To(BeTrue())
+
+			Expect(ctrl.RecoverPanic).NotTo(BeNil())
+			Expect(*ctrl.RecoverPanic).To(BeTrue())
+		})
+
+		It("should not override RecoverPanic on the controller", func() {
+			m, err := manager.New(cfg, manager.Options{Controller: v1alpha1.ControllerConfigurationSpec{RecoverPanic: ptr(true)}})
+			Expect(err).NotTo(HaveOccurred())
+
+			c, err := controller.New("new-controller", m, controller.Options{
+				Reconciler:   reconcile.Func(nil),
+				RecoverPanic: ptr(false),
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			ctrl, ok := c.(*internalcontroller.Controller)
+			Expect(ok).To(BeTrue())
+
+			Expect(ctrl.RecoverPanic).NotTo(BeNil())
+			Expect(*ctrl.RecoverPanic).To(BeFalse())
+		})
 	})
 })
 
@@ -157,3 +192,7 @@ func (*failRec) Reconcile(context.Context, reconcile.Request) (reconcile.Result,
 func (*failRec) InjectClient(client.Client) error {
 	return fmt.Errorf("expected error")
 }
+
+func ptr[T any](to T) *T {
+	return &to
+}

pkg/internal/controller/controller.go

@@ -92,7 +92,7 @@ type Controller struct {
 	LogConstructor func(request *reconcile.Request) logr.Logger
 
 	// RecoverPanic indicates whether the panic caused by reconcile should be recovered.
-	RecoverPanic bool
+	RecoverPanic *bool
 }
 
 // watchDescription contains all the information necessary to start a watch.
@@ -106,7 +106,7 @@ type watchDescription struct {
 func (c *Controller) Reconcile(ctx context.Context, req reconcile.Request) (_ reconcile.Result, err error) {
 	defer func() {
 		if r := recover(); r != nil {
-			if c.RecoverPanic {
+			if c.RecoverPanic != nil && *c.RecoverPanic {
 				for _, fn := range utilruntime.PanicHandlers {
 					fn(r)
 				}

pkg/internal/controller/controller_test.go

@@ -114,7 +114,7 @@ var _ = Describe("controller", func() {
 			defer func() {
 				Expect(recover()).To(BeNil())
 			}()
-			ctrl.RecoverPanic = true
+			ctrl.RecoverPanic = ptr(true)
 			ctrl.Do = reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) {
 				var res *reconcile.Result
 				return *res, nil
@@ -996,3 +996,7 @@ func (c *cacheWithIndefinitelyBlockingGetInformer) GetInformer(ctx context.Conte
 	<-ctx.Done()
 	return nil, errors.New("GetInformer timed out")
 }
+
+func ptr[T any](in T) *T {
+	return &in
+}