Merge pull request #2093 from alvaroaleman/recover-panic-globally

⚠️ Allow configuring RecoverPanic for controllers globally

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

@@ -25,11 +25,14 @@ import (
 	. "github.com/onsi/gomega"
 	"go.uber.org/goleak"
 	corev1 "k8s.io/api/core/v1"
+	"k8s.io/utils/pointer"
 
 	"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 +145,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: pointer.Bool(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: pointer.Bool(true)}})
+			Expect(err).NotTo(HaveOccurred())
+
+			c, err := controller.New("new-controller", m, controller.Options{
+				Reconciler:   reconcile.Func(nil),
+				RecoverPanic: pointer.Bool(false),
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			ctrl, ok := c.(*internalcontroller.Controller)
+			Expect(ok).To(BeTrue())
+
+			Expect(ctrl.RecoverPanic).NotTo(BeNil())
+			Expect(*ctrl.RecoverPanic).To(BeFalse())
+		})
 	})
 })
 

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

@@ -33,6 +33,7 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/util/workqueue"
+	"k8s.io/utils/pointer"
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	"sigs.k8s.io/controller-runtime/pkg/cache/informertest"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -114,7 +115,7 @@ var _ = Describe("controller", func() {
 			defer func() {
 				Expect(recover()).To(BeNil())
 			}()
-			ctrl.RecoverPanic = true
+			ctrl.RecoverPanic = pointer.Bool(true)
 			ctrl.Do = reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) {
 				var res *reconcile.Result
 				return *res, nil