⚠️ Validate controller names are unique

[alvaroaleman]

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.

[troy0820]

/lgtm

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 88bba7827300bc12364a32520eea18dc109de046

[alvaroaleman]

/hold

[sbueringer]

Otherwise lgtm from my side

[alvaroaleman]

/hold cancel

[sbueringer]

Thx!

/lgtm
/approve

[k8s-ci-robot]

LGTM label has been added.

Git tree hash: 4f7a6ac50b301922450983aa632b2baaebf1a055

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [alvaroaleman,sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[mythi]

FWIW, this seems to trigger an error in our envtest based tests where a new manager is started BeforeEach and its context cancelled AfterEach. I might have a race or doing something wrong, though. 🤔

AFAUI, the manager gets closed

...
I0812 16:39:27.717706 1182770 internal.go:550] "Stopping and waiting for caches"
I0812 16:39:27.718212 1182770 internal.go:554] "Stopping and waiting for webhooks"
I0812 16:39:27.718251 1182770 internal.go:557] "Stopping and waiting for HTTP servers"
I0812 16:39:27.718270 1182770 internal.go:561] "Wait completed, proceeding to shutdown the manager"

but the next test returns an error added here.

  [FAILED] Expected
      <*errors.errorString | 0xc000804160>: 
      controller with name <myname> already exists. Controller names must be unique to avoid multiple controllers reporting to the same metric
      {
          s: "controller with name <myname> already exists. Controller names must be unique to avoid multiple controllers reporting to the same metric",
      }
  to be nil

[sbueringer]

I think it's not a race condition. The problem is that the check is a singleton, but it has to be because the queue metrics are also singletons.

What you can do today is to use different controller names for each test run.

Just discussed this with @alvaroaleman, we would add an option to controller & manager (the controller option defaults to the one on the manager) to skip the name check.
(so this will be on

controller-runtime/pkg/controller/controller.go

Line 38 in 9516c0f

type TypedOptions[request comparable] struct {

&

controller-runtime/pkg/config/controller.go

Line 22 in 9516c0f

type Controller struct {

)

I assume this would help in your case?

[sbueringer]

@mythi @alvaroaleman PR: #2918

[mythi]

@mythi @alvaroaleman PR: #2918

thanks! I'll give it a try on Tue.

[RainbowMango]

Hi @alvaroaleman @sbueringer
Holding controller name with a global variable seems not a good idea, because the variable will be shared between tests in a test suite or in a test package.

To be more specific, see following the test sample below:

	for _, tt := range tests {
		t.Run(tt.name, func(t *testing.T) {
			mgr, err := controllerruntime.NewManager(&rest.Config{}, controllerruntime.Options{})

			controller := &FooController{}
			err = controller.SetupWithManager(mgr)
			assert.NoError(t, err) // raise error: Controller names must be unique to avoid multiple controllers reporting to the same metric

			// ... others
		})
	}

Even though each test creates a brand new controller manager, but the usedNamed is shared unexpected.

[RainbowMango]

My two cents:
The used controller names should be stored in a specific manager instance.

[mythi]

@RainbowMango my tests have the following: ctrl.NewManager(cfg, ctrl.Options{Scheme: scheme.Scheme, Metrics: metricsserver.Options{BindAddress: "0"}, Controller: config.Controller{SkipNameValidation: &yes}})

[RainbowMango]

Yeah, thanks. @mythi
It's not a blocker for me.
I noticed the option as well, but I think it's more like a workaround.

[qJkee]

@mythi @RainbowMango my 2 cents there as well
I did raise the same question at this issue and I really like the idea of storing controller names within one manager maybe? Or at least provide some mechanism of cleaning this map instead of disabling the check completely