:warn: Flatten fields in controller.Options

Currently, `controller.Options` embedds `config.Controller`. This makes
its usage pretty annoying, to set `MaxConcurrentReconciles` for example,
one has to do it like this:

```
controller.Options{config.Options{MaxConcurrentReconciles: 8}}
```

This makes it harder to find what options exist and causes a lot of
churn for downstream consumers. Re-Define the fields from
`config.Controller` in `controller.Options` instead to avoid that.

This also fixes some defaulting bugs where we wouldn't default
`MaxConcurrentReconciles` and `CacheSyncTimeout` from the
`config.Controller` setting in the manager.

Author: alvaroaleman

pkg/builder/controller_test.go

@@ -217,7 +217,7 @@ var _ = Describe("application", func() {
 			instance, err := ControllerManagedBy(m).
 				For(&appsv1.ReplicaSet{}).
 				Owns(&appsv1.ReplicaSet{}).
-				WithOptions(controller.Options{Controller: config.Controller{MaxConcurrentReconciles: maxConcurrentReconciles}}).
+				WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles}).
 				Build(noop)
 			Expect(err).NotTo(HaveOccurred())
 			Expect(instance).NotTo(BeNil())

pkg/controller/controller.go

@@ -25,7 +25,6 @@ import (
 	"k8s.io/client-go/util/workqueue"
 	"k8s.io/klog/v2"
 
-	"sigs.k8s.io/controller-runtime/pkg/config"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/internal/controller"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -37,7 +36,20 @@ import (
 
 // Options are the arguments for creating a new Controller.
 type Options struct {
-	config.Controller
+	// MaxConcurrentReconciles is the maximum number of concurrent Reconciles which can be run. Defaults to 1.
+	MaxConcurrentReconciles int
+
+	// CacheSyncTimeout refers to the time limit set to wait for syncing caches.
+	// Defaults to 2 minutes if not set.
+	CacheSyncTimeout time.Duration
+
+	// RecoverPanic indicates whether the panic caused by reconcile should be recovered.
+	// Defaults to the Controller.RecoverPanic setting from the Manager if unset.
+	RecoverPanic *bool
+
+	// NeedLeaderElection indicates whether the controller needs to use leader election.
+	// Defaults to true, which means the controller will use leader election.
+	NeedLeaderElection *bool
 
 	// Reconciler reconciles an object
 	Reconciler reconcile.Reconciler
@@ -116,11 +128,19 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller
 	}
 
 	if options.MaxConcurrentReconciles <= 0 {
-		options.MaxConcurrentReconciles = 1
+		if mgr.GetControllerOptions().MaxConcurrentReconciles > 0 {
+			options.MaxConcurrentReconciles = mgr.GetControllerOptions().MaxConcurrentReconciles
+		} else {
+			options.MaxConcurrentReconciles = 1
+		}
 	}
 
 	if options.CacheSyncTimeout == 0 {
-		options.CacheSyncTimeout = 2 * time.Minute
+		if mgr.GetControllerOptions().CacheSyncTimeout != 0 {
+			options.CacheSyncTimeout = mgr.GetControllerOptions().CacheSyncTimeout
+		} else {
+			options.CacheSyncTimeout = 2 * time.Minute
+		}
 	}
 
 	if options.RateLimiter == nil {

pkg/controller/controller_test.go

@@ -154,10 +154,8 @@ var _ = Describe("controller.Controller", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			c, err := controller.New("new-controller", m, controller.Options{
-				Controller: config.Controller{
-					RecoverPanic: pointer.Bool(false),
-				},
-				Reconciler: reconcile.Func(nil),
+				RecoverPanic: pointer.Bool(false),
+				Reconciler:   reconcile.Func(nil),
 			})
 			Expect(err).NotTo(HaveOccurred())
 
@@ -168,6 +166,98 @@ var _ = Describe("controller.Controller", func() {
 			Expect(*ctrl.RecoverPanic).To(BeFalse())
 		})
 
+		It("Should default MaxConcurrentReconciles from the manager if set", func() {
+			m, err := manager.New(cfg, manager.Options{Controller: config.Controller{MaxConcurrentReconciles: 5}})
+			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.MaxConcurrentReconciles).To(BeEquivalentTo(5))
+		})
+
+		It("Should default MaxConcurrentReconciles to 1 if unset", func() {
+			m, err := manager.New(cfg, manager.Options{})
+			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.MaxConcurrentReconciles).To(BeEquivalentTo(1))
+		})
+
+		It("Should leave MaxConcurrentReconciles if set", func() {
+			m, err := manager.New(cfg, manager.Options{})
+			Expect(err).NotTo(HaveOccurred())
+
+			c, err := controller.New("new-controller", m, controller.Options{
+				Reconciler:              reconcile.Func(nil),
+				MaxConcurrentReconciles: 5,
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			ctrl, ok := c.(*internalcontroller.Controller)
+			Expect(ok).To(BeTrue())
+
+			Expect(ctrl.MaxConcurrentReconciles).To(BeEquivalentTo(5))
+		})
+
+		It("Should default CacheSyncTimeout from the manager if set", func() {
+			m, err := manager.New(cfg, manager.Options{Controller: config.Controller{CacheSyncTimeout: 5}})
+			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.CacheSyncTimeout).To(BeEquivalentTo(5))
+		})
+
+		It("Should default CacheSyncTimeout to 2 minutes if unset", func() {
+			m, err := manager.New(cfg, manager.Options{})
+			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.CacheSyncTimeout).To(BeEquivalentTo(2 * time.Minute))
+		})
+
+		It("Should leave CacheSyncTimeout if set", func() {
+			m, err := manager.New(cfg, manager.Options{})
+			Expect(err).NotTo(HaveOccurred())
+
+			c, err := controller.New("new-controller", m, controller.Options{
+				Reconciler:       reconcile.Func(nil),
+				CacheSyncTimeout: 5,
+			})
+			Expect(err).NotTo(HaveOccurred())
+
+			ctrl, ok := c.(*internalcontroller.Controller)
+			Expect(ok).To(BeTrue())
+
+			Expect(ctrl.CacheSyncTimeout).To(BeEquivalentTo(5))
+		})
+
 		It("should default NeedLeaderElection on the controller to true", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
@@ -188,10 +278,8 @@ var _ = Describe("controller.Controller", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			c, err := controller.New("new-controller", m, controller.Options{
-				Controller: config.Controller{
-					NeedLeaderElection: pointer.Bool(false),
-				},
-				Reconciler: rec,
+				RecoverPanic: pointer.Bool(false),
+				Reconciler:   rec,
 			})
 			Expect(err).NotTo(HaveOccurred())