diff --git a/pkg/builder/controller.go b/pkg/builder/controller.go index 9259a83622..2ac178ea1e 100644 --- a/pkg/builder/controller.go +++ b/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 diff --git a/pkg/builder/controller_test.go b/pkg/builder/controller_test.go index 388ecbf463..cbeb1c43bc 100644 --- a/pkg/builder/controller_test.go +++ b/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 { diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index adf66c16c1..d31199012f 100644 --- a/pkg/controller/controller.go +++ b/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, diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index c27181a0ef..4ab62909a8 100644 --- a/pkg/controller/controller_test.go +++ b/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()) diff --git a/pkg/controller/name.go b/pkg/controller/name.go new file mode 100644 index 0000000000..0e71a01c66 --- /dev/null +++ b/pkg/controller/name.go @@ -0,0 +1,43 @@ +/* +Copyright 2020 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package controller + +import ( + "fmt" + "sync" + + "k8s.io/apimachinery/pkg/util/sets" +) + +var nameLock sync.Mutex +var usedNames sets.Set[string] + +func checkName(name string) error { + nameLock.Lock() + defer nameLock.Unlock() + if usedNames == nil { + usedNames = sets.Set[string]{} + } + + if usedNames.Has(name) { + return fmt.Errorf("controller with name %s already exists. Controller names must be unique to avoid multiple controllers reporting to the same metric", name) + } + + usedNames.Insert(name) + + return nil +}