⚠️ Validate controller names are unique

pkg/builder/controller.go

@@ -251,6 +251,8 @@ func (blder *TypedBuilder[request]) WithLogConstructor(logConstructor func(*requ
 // (underscores and alphanumeric characters only).
 //
 // By default, controllers are named using the lowercase version of their kind.
+//
+// The name must be unique as it is used to identify the controller in metrics and logs.
 func (blder *TypedBuilder[request]) Named(name string) *TypedBuilder[request] {
 	blder.name = name
 	return blder

pkg/builder/controller_test.go

@@ -142,7 +142,7 @@ var _ = Describe("application", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			instance, err := ControllerManagedBy(m).
-				Named("my_controller").
+				Named("my_new_controller").
 				Build(noop)
 			Expect(err).To(MatchError(ContainSubstring("there are no watches configured, controller will never get triggered. Use For(), Owns(), Watches() or WatchesRawSource() to set them up")))
 			Expect(instance).To(BeNil())
@@ -154,7 +154,7 @@ var _ = Describe("application", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			instance, err := ControllerManagedBy(m).
-				Named("my_controller").
+				Named("my_other_controller").
 				Watches(&appsv1.ReplicaSet{}, &handler.EnqueueRequestForObject{}).
 				Build(noop)
 			Expect(err).NotTo(HaveOccurred())
@@ -186,6 +186,7 @@ var _ = Describe("application", func() {
 
 			instance, err := TypedControllerManagedBy[empty](m).
 				For(&appsv1.ReplicaSet{}).
+				Named("last_controller").
 				Build(typedNoop)
 			Expect(err).To(MatchError(ContainSubstring("For() can only be used with reconcile.Request, got builder.empty")))
 			Expect(instance).To(BeNil())
@@ -197,7 +198,7 @@ var _ = Describe("application", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			instance, err := TypedControllerManagedBy[empty](m).
-				Named("my_controller").
+				Named("my_controller-0").
 				Owns(&appsv1.ReplicaSet{}).
 				Build(typedNoop)
 				// If we ever allow Owns() without For() we need to update the code to error
@@ -213,7 +214,7 @@ var _ = Describe("application", func() {
 			Expect(err).NotTo(HaveOccurred())
 
 			instance, err := TypedControllerManagedBy[empty](m).
-				Named("my_controller").
+				Named("my_controller-1").
 				WatchesRawSource(
 					source.TypedKind(
 						m.GetCache(),
@@ -263,6 +264,7 @@ var _ = Describe("application", func() {
 
 			builder := ControllerManagedBy(m).
 				For(&appsv1.ReplicaSet{}).
+				Named("replicaset-4").
 				Owns(&appsv1.ReplicaSet{}).
 				WithOptions(controller.Options{MaxConcurrentReconciles: maxConcurrentReconciles})
 			builder.newController = newController
@@ -294,6 +296,7 @@ var _ = Describe("application", func() {
 
 			builder := ControllerManagedBy(m).
 				For(&appsv1.ReplicaSet{}).
+				Named("replicaset-3").
 				Owns(&appsv1.ReplicaSet{})
 			builder.newController = newController
 
@@ -317,6 +320,7 @@ var _ = Describe("application", func() {
 
 			builder := ControllerManagedBy(m).
 				For(&appsv1.ReplicaSet{}).
+				Named("replicaset-2").
 				Owns(&appsv1.ReplicaSet{}).
 				WithOptions(controller.Options{RateLimiter: rateLimiter})
 			builder.newController = newController
@@ -341,6 +345,7 @@ var _ = Describe("application", func() {
 
 			builder := ControllerManagedBy(m).
 				For(&appsv1.ReplicaSet{}).
+				Named("replicaset-0").
 				Owns(&appsv1.ReplicaSet{}).
 				WithLogConstructor(func(request *reconcile.Request) logr.Logger {
 					return logr.New(logger)
@@ -358,6 +363,7 @@ var _ = Describe("application", func() {
 
 			builder := ControllerManagedBy(m).
 				For(&appsv1.ReplicaSet{}).
+				Named("replicaset-1").
 				Owns(&appsv1.ReplicaSet{}).
 				WithOptions(controller.Options{Reconciler: noop})
 			instance, err := builder.Build(noop)
@@ -387,6 +393,7 @@ var _ = Describe("application", func() {
 			By("creating the 2nd controller")
 			ctrl2, err := ControllerManagedBy(m).
 				For(&TestDefaultValidator{}).
+				Named("test-default-validator-1").
 				Owns(&appsv1.ReplicaSet{}).
 				Build(noop)
 			Expect(err).NotTo(HaveOccurred())
@@ -401,6 +408,7 @@ var _ = Describe("application", func() {
 
 			bldr := ControllerManagedBy(m).
 				For(&appsv1.Deployment{}).
+				Named("deployment-0").
 				Owns(&appsv1.ReplicaSet{})
 
 			ctx, cancel := context.WithCancel(context.Background())
@@ -414,6 +422,7 @@ var _ = Describe("application", func() {
 
 			bldr := ControllerManagedBy(m).
 				For(&appsv1.Deployment{}).
+				Named("deployment-1").
 				Owns(&appsv1.ReplicaSet{}, MatchEveryOwner)
 
 			ctx, cancel := context.WithCancel(context.Background())
@@ -443,6 +452,7 @@ var _ = Describe("application", func() {
 
 			bldr := ControllerManagedBy(m).
 				Named("Deployment").
+				Named("deployment-2").
 				Watches( // Equivalent of For
 						&appsv1.Deployment{}, &handler.EnqueueRequestForObject{}).
 				Watches( // Equivalent of Owns
@@ -503,6 +513,7 @@ var _ = Describe("application", func() {
 
 			bldr := ControllerManagedBy(m).
 				For(&appsv1.Deployment{}, WithPredicates(deployPrct)).
+				Named("deployment-3").
 				Owns(&appsv1.ReplicaSet{}, WithPredicates(replicaSetPrct)).
 				WithEventFilter(allPrct)
 
@@ -527,8 +538,8 @@ var _ = Describe("application", func() {
 		})
 
 		It("should support multiple controllers watching the same metadata kind", func() {
-			bldr1 := ControllerManagedBy(mgr).For(&appsv1.Deployment{}, OnlyMetadata)
-			bldr2 := ControllerManagedBy(mgr).For(&appsv1.Deployment{}, OnlyMetadata)
+			bldr1 := ControllerManagedBy(mgr).For(&appsv1.Deployment{}, OnlyMetadata).Named("deployment-4")
+			bldr2 := ControllerManagedBy(mgr).For(&appsv1.Deployment{}, OnlyMetadata).Named("deployment-5")
 
 			ctx, cancel := context.WithCancel(context.Background())
 			defer cancel()
@@ -541,6 +552,7 @@ var _ = Describe("application", func() {
 
 			bldr := ControllerManagedBy(mgr).
 				For(&appsv1.Deployment{}, OnlyMetadata).
+				Named("deployment-6").
 				Owns(&appsv1.ReplicaSet{}, OnlyMetadata).
 				Watches(&appsv1.StatefulSet{},
 					handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, o client.Object) []reconcile.Request {

pkg/controller/controller.go

@@ -100,11 +100,15 @@ type TypedController[request comparable] interface {
 
 // New returns a new Controller registered with the Manager.  The Manager will ensure that shared Caches have
 // been synced before the Controller is Started.
+//
+// The name must be unique as it is used to identify the controller in metrics and logs.
 func New(name string, mgr manager.Manager, options Options) (Controller, error) {
 	return NewTyped(name, mgr, options)
 }
 
 // NewTyped returns a new typed controller registered with the Manager,
+//
+// The name must be unique as it is used to identify the controller in metrics and logs.
 func NewTyped[request comparable](name string, mgr manager.Manager, options TypedOptions[request]) (TypedController[request], error) {
 	c, err := NewTypedUnmanaged(name, mgr, options)
 	if err != nil {
@@ -117,11 +121,15 @@ func NewTyped[request comparable](name string, mgr manager.Manager, options Type
 
 // NewUnmanaged returns a new controller without adding it to the manager. The
 // caller is responsible for starting the returned controller.
+//
+// The name must be unique as it is used to identify the controller in metrics and logs.
 func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller, error) {
 	return NewTypedUnmanaged(name, mgr, options)
 }
 
 // NewTypedUnmanaged returns a new typed controller without adding it to the manager.
+//
+// The name must be unique as it is used to identify the controller in metrics and logs.
 func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, options TypedOptions[request]) (TypedController[request], error) {
 	if options.Reconciler == nil {
 		return nil, fmt.Errorf("must specify Reconciler")
@@ -131,6 +139,10 @@ func NewTypedUnmanaged[request comparable](name string, mgr manager.Manager, opt
 		return nil, fmt.Errorf("must specify Name for Controller")
 	}
 
+	if err := checkName(name); err != nil {
+		return nil, err
+	}
+
 	if options.LogConstructor == nil {
 		log := mgr.GetLogger().WithValues(
 			"controller", name,

pkg/controller/controller_test.go

@@ -60,6 +60,20 @@ var _ = Describe("controller.Controller", func() {
 			Expect(err.Error()).To(ContainSubstring("must specify Reconciler"))
 		})
 
+		It("should return an error if two controllers are registered with the same name", func() {
+			m, err := manager.New(cfg, manager.Options{})
+			Expect(err).NotTo(HaveOccurred())
+
+			c1, err := controller.New("c3", m, controller.Options{Reconciler: rec})
+			Expect(err).NotTo(HaveOccurred())
+			Expect(c1).ToNot(BeNil())
+
+			c2, err := controller.New("c3", m, controller.Options{Reconciler: rec})
+			Expect(err).To(HaveOccurred())
+			Expect(err.Error()).To(ContainSubstring("controller with name c3 already exists"))
+			Expect(c2).To(BeNil())
+		})
+
 		It("should not return an error if two controllers are registered with different names", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
@@ -99,7 +113,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{Reconciler: rec})
+			c, err := controller.New("new-controller-0", m, controller.Options{Reconciler: rec})
 			Expect(c.Watch(watch)).To(Succeed())
 			Expect(err).NotTo(HaveOccurred())
 
@@ -125,7 +139,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			_, err = controller.New("new-controller", m, controller.Options{Reconciler: rec})
+			_, err = controller.New("new-controller-1", m, controller.Options{Reconciler: rec})
 			Expect(err).NotTo(HaveOccurred())
 
 			// force-close keep-alive connections.  These'll time anyway (after
@@ -138,7 +152,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-2", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 			})
 			Expect(err).NotTo(HaveOccurred())
@@ -161,7 +175,7 @@ var _ = Describe("controller.Controller", func() {
 				return nil
 			}
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-3", m, controller.Options{
 				Reconciler:  reconcile.Func(nil),
 				RateLimiter: customRateLimiter,
 				NewQueue:    customNewQueue,
@@ -180,7 +194,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{Controller: config.Controller{RecoverPanic: ptr.To(true)}})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-4", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 			})
 			Expect(err).NotTo(HaveOccurred())
@@ -213,7 +227,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{Controller: config.Controller{NeedLeaderElection: ptr.To(true)}})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-5", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 			})
 			Expect(err).NotTo(HaveOccurred())
@@ -228,7 +242,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{Controller: config.Controller{NeedLeaderElection: ptr.To(true)}})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-6", m, controller.Options{
 				NeedLeaderElection: ptr.To(false),
 				Reconciler:         reconcile.Func(nil),
 			})
@@ -244,7 +258,7 @@ var _ = Describe("controller.Controller", 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{
+			c, err := controller.New("new-controller-7", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 			})
 			Expect(err).NotTo(HaveOccurred())
@@ -259,7 +273,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-8", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 			})
 			Expect(err).NotTo(HaveOccurred())
@@ -274,7 +288,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-9", m, controller.Options{
 				Reconciler:              reconcile.Func(nil),
 				MaxConcurrentReconciles: 5,
 			})
@@ -290,7 +304,7 @@ var _ = Describe("controller.Controller", 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{
+			c, err := controller.New("new-controller-10", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 			})
 			Expect(err).NotTo(HaveOccurred())
@@ -305,7 +319,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-11", m, controller.Options{
 				Reconciler: reconcile.Func(nil),
 			})
 			Expect(err).NotTo(HaveOccurred())
@@ -320,7 +334,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-12", m, controller.Options{
 				Reconciler:       reconcile.Func(nil),
 				CacheSyncTimeout: 5,
 			})
@@ -336,7 +350,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-13", m, controller.Options{
 				Reconciler: rec,
 			})
 			Expect(err).NotTo(HaveOccurred())
@@ -351,7 +365,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-14", m, controller.Options{
 				NeedLeaderElection: ptr.To(false),
 				Reconciler:         rec,
 			})
@@ -367,7 +381,7 @@ var _ = Describe("controller.Controller", func() {
 			m, err := manager.New(cfg, manager.Options{})
 			Expect(err).NotTo(HaveOccurred())
 
-			c, err := controller.New("new-controller", m, controller.Options{
+			c, err := controller.New("new-controller-15", m, controller.Options{
 				Reconciler: rec,
 			})
 			Expect(err).NotTo(HaveOccurred())