Skip to content

✨ feat: scope controller name validation to manager instance #3241

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions pkg/controller/controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand All @@ -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,
Expand Down
24 changes: 24 additions & 0 deletions pkg/controller/controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand Down
43 changes: 0 additions & 43 deletions pkg/controller/name.go

This file was deleted.

22 changes: 22 additions & 0 deletions pkg/manager/internal.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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{}
Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When you have multiple managers, how do the metrics look? Is there a specific metric label that identifies the manager?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there is nothing. The metrics are global and not scoped to the manager, which is why the check is as it is

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback. You've highlighted an important issue regarding metric collisions. I can see that the metrics are global and not scoped to individual manager instances, which is why the original check existed.

The main goal of this PR is to improve the developer experience for the common case of running a single manager per process. The current global validation sometimes require developers to use SkipNameValidation when the test recreating managers, which can mask naming conflicts until runtime. By scoping validation to the manager, these errors can be caught early during testing.

I am aware that this change introduces challenges for the less common scenario of running multiple managers in a single process. In order to improve dev experience for single controller manger use case, this PR requires dev to manually avoid naming conflict when working with multiple controller managers. I see two possible solutions to address this:

  • Use separate Prometheus registries for each manager
  • Add a manager-specific metric label

I'm open to suggestions and looking forward to your thoughts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend to disable the name validation in tests

}

cm.usedControllerNames.Insert(name)
return nil
}

func (cm *controllerManager) addHealthProbeServer() error {
mux := http.NewServeMux()
srv := httpserver.New(mux)
Expand Down
14 changes: 14 additions & 0 deletions pkg/manager/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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,
Expand Down
Loading