From 2b941650bce159006c88bd3ca0d132c7bc40e947 Mon Sep 17 00:00:00 2001 From: Alvaro Aleman Date: Sat, 3 Aug 2024 17:56:45 -0400 Subject: [PATCH] :warning: Validate controller names are unique When re-using the same name among multiple controllers, they will report to the same metrics and use the same logger. This can be very confusing, might not be obvious and can happen accidentally. Validate controller names are unique at runtime to avoid all of this. --- pkg/builder/controller.go | 2 ++ pkg/builder/controller_test.go | 24 ++++++++++++---- pkg/controller/controller.go | 12 ++++++++ pkg/controller/controller_test.go | 46 ++++++++++++++++++++----------- pkg/controller/name.go | 43 +++++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 22 deletions(-) create mode 100644 pkg/controller/name.go 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 +}