diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 9de959b48f..70d0dc2602 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -166,6 +166,15 @@ func NewTyped[request comparable](name string, mgr manager.Manager, options Type return nil, err } + // Validate controller name uniqueness within this manager + if options.SkipNameValidation == nil || !*options.SkipNameValidation { + if validator, ok := mgr.(manager.ControllerNameValidator); ok { + if err := validator.ValidateControllerName(name); err != nil { + return nil, err + } + } + } + // Add the controller as a Manager components return c, mgr.Add(c) } @@ -190,12 +199,6 @@ func NewTypedUnmanaged[request comparable](name string, options TypedOptions[req return nil, fmt.Errorf("must specify Name for Controller") } - if options.SkipNameValidation == nil || !*options.SkipNameValidation { - if err := checkName(name); err != nil { - return nil, err - } - } - if options.LogConstructor == nil { log := options.Logger.WithValues( "controller", name, diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index 1c5b11d709..b964016ba4 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -136,6 +136,30 @@ var _ = Describe("controller.Controller", func() { Expect(c2).ToNot(BeNil()) }) + It("should allow controllers with same name in different manager instances", func() { + // Create two separate manager instances + m1, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + m2, err := manager.New(cfg, manager.Options{}) + Expect(err).NotTo(HaveOccurred()) + + // Both managers should be able to have controllers with the same name + c1, err := controller.New("c1", m1, controller.Options{Reconciler: rec}) + Expect(err).NotTo(HaveOccurred()) + Expect(c1).ToNot(BeNil()) + + c2, err := controller.New("c1", m2, controller.Options{Reconciler: rec}) + Expect(err).NotTo(HaveOccurred()) + Expect(c2).ToNot(BeNil()) + + // Verify that trying to create another controller with the same name in the same manager fails + c3, err := controller.New("c1", m1, controller.Options{Reconciler: rec}) + Expect(err).To(HaveOccurred()) + Expect(err.Error()).To(ContainSubstring("controller with name c1 already exists in this manager")) + Expect(c3).To(BeNil()) + }) + It("should not leak goroutines when stopped", func() { currentGRs := goleak.IgnoreCurrent() diff --git a/pkg/controller/name.go b/pkg/controller/name.go deleted file mode 100644 index 00ca655128..0000000000 --- a/pkg/controller/name.go +++ /dev/null @@ -1,43 +0,0 @@ -/* -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 the same metric. This validation can be disabled via the SkipNameValidation option", name) - } - - usedNames.Insert(name) - - return nil -} diff --git a/pkg/manager/internal.go b/pkg/manager/internal.go index e5204a7506..a58587cfef 100644 --- a/pkg/manager/internal.go +++ b/pkg/manager/internal.go @@ -31,6 +31,7 @@ import ( "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/runtime" kerrors "k8s.io/apimachinery/pkg/util/errors" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/rest" "k8s.io/client-go/tools/leaderelection" "k8s.io/client-go/tools/leaderelection/resourcelock" @@ -110,6 +111,10 @@ type controllerManager struct { // If none is set, it defaults to log.Log global logger. logger logr.Logger + // usedControllerNames tracks controller names within this manager instance + usedControllerNames sets.Set[string] + usedNamesMutex sync.RWMutex + // leaderElectionStopped is an internal channel used to signal the stopping procedure that the // LeaderElection.Run(...) function has returned and the shutdown can proceed. leaderElectionStopped chan struct{} @@ -287,6 +292,23 @@ func (cm *controllerManager) GetControllerOptions() config.Controller { return cm.controllerConfig } +// ValidateControllerName validates that a controller name is unique within this manager. +func (cm *controllerManager) ValidateControllerName(name string) error { + cm.usedNamesMutex.Lock() + defer cm.usedNamesMutex.Unlock() + + if cm.usedControllerNames == nil { + cm.usedControllerNames = sets.Set[string]{} + } + + if cm.usedControllerNames.Has(name) { + return fmt.Errorf("controller with name %s already exists in this manager. Controller names must be unique within the same controller manager to avoid multiple controllers reporting the same metric. This validation can be disabled via the SkipNameValidation option", name) + } + + cm.usedControllerNames.Insert(name) + return nil +} + func (cm *controllerManager) addHealthProbeServer() error { mux := http.NewServeMux() srv := httpserver.New(mux) diff --git a/pkg/manager/manager.go b/pkg/manager/manager.go index c3ae317b04..e20a05b0f7 100644 --- a/pkg/manager/manager.go +++ b/pkg/manager/manager.go @@ -35,6 +35,7 @@ import ( "k8s.io/utils/ptr" metricsserver "sigs.k8s.io/controller-runtime/pkg/metrics/server" + "k8s.io/apimachinery/pkg/util/sets" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/cluster" @@ -95,6 +96,18 @@ type Manager interface { // GetControllerOptions returns controller global configuration options. GetControllerOptions() config.Controller + + ControllerNameValidator +} + +// ControllerNameValidator defines the interface for validating controller names +type ControllerNameValidator interface { + // ValidateControllerName validates that a controller name is unique within this manager. + // Returns an error if the name is already in use. + // This validation can be skipped by setting SkipNameValidation to true in either: + // - Manager options (config.Controller.SkipNameValidation) - applies to all controllers + // - Controller options (controller.Options.SkipNameValidation) - overrides manager setting + ValidateControllerName(name string) error } // Options are the arguments for creating a new Manager. @@ -428,6 +441,7 @@ func New(config *rest.Config, options Options) (Manager, error) { metricsServer: metricsServer, controllerConfig: options.Controller, logger: options.Logger, + usedControllerNames: sets.Set[string]{}, elected: make(chan struct{}), webhookServer: options.WebhookServer, leaderElectionID: options.LeaderElectionID,