incorrect check for duplicate controller names

[nicks]

Repro steps

Add the following test:

package scratch

import (
	"context"
	"testing"

	"github.com/stretchr/testify/assert"
	"k8s.io/client-go/rest"

	"sigs.k8s.io/controller-runtime/pkg/controller"
	"sigs.k8s.io/controller-runtime/pkg/manager"
	"sigs.k8s.io/controller-runtime/pkg/reconcile"
)

var rec = reconcile.Func(func(context.Context, reconcile.Request) (reconcile.Result, error) {
	return reconcile.Result{}, nil
})
var cfg = &rest.Config{}

func TestManager(t *testing.T) {
	m, err := manager.New(cfg, manager.Options{})
	assert.NoError(t, err)

	_, err = controller.New("c1", m, controller.Options{Reconciler: rec})
	assert.NoError(t, err)
}

func TestManager2(t *testing.T) {
	m, err := manager.New(cfg, manager.Options{})
	assert.NoError(t, err)

	_, err = controller.New("c1", m, controller.Options{Reconciler: rec})
	assert.NoError(t, err)
}

Expected result
Tests pass

Actual result
Tests fail

controller with name c1 already exists. Controller names must be unique to avoid multiple controllers reporting to the same metric

Additional info

controller-runtime seems to have implemented a uniqueness check based on global state with no way to reset the global state for tests :(

[bigkevmcd]

You can disable the name verification (which prevents the name being added to the set of previously seen names).

This PR landed the change #2918

_, err = controller.New("c1", m, controller.Options{Reconciler: rec, SkipNameValidation: ptr.To(true)})

[AndreiBarbuOz]

You can disable the name verification (which prevents the name being added to the set of previously seen names).

This is not a particularly appealing option. I'd rather not mix my test code (where the controllers need to skip validation) and production code (where the validation should be executed)

If you have a project which follows the Kubebuilder book, you're very likely to use the builder pattern, and not have an easy way of injecting configuration to all controllers

[alvaroaleman]

f you have a project which follows the Kubebuilder book, you're very likely to use the builder pattern, and not have an easy way of injecting configuration to all controllers

It is also an option on the manager, setting it there will disable it for all controllers managed by this manager.

[AndreiBarbuOz]

@alvaroaleman , can you indicate where that option resides?

i assumed it would be part of this structure, but it is not the case:

controller-runtime/pkg/manager/manager.go

Lines 100 to 107 in 610870e

	// Options are the arguments for creating a new Manager.
	type Options struct {
	// Scheme is the scheme used to resolve runtime.Objects to GroupVersionKinds / Resources.
	// Defaults to the kubernetes/client-go scheme.Scheme, but it's almost always better
	// to pass your own scheme in. See the documentation in pkg/scheme for more information.
	//
	// If set, the Scheme will be used to create the default Client and Cache.
	Scheme *runtime.Scheme

LE: the configuration is going through the config package actually

controller-runtime/pkg/manager/manager.go

Lines 267 to 270 in 610870e

	// Controller contains global configuration options for controllers
	// registered within this manager.
	// +optional
	Controller config.Controller

[alvaroaleman]

managerOpts.Controller.SkipNameValidation, yes.

[shatil]

You can disable the name verification (which prevents the name being added to the set of previously seen names).

This is not a particularly appealing option. I'd rather not mix my test code (where the controllers need to skip validation) and production code (where the validation should be executed)

@AndreiBarbuOz, I encountered the same error when running multiple controllers that reconcile the same object.

controller with name $name already exists. Controller names must be unique to avoid multiple controllers reporting to the same metric

My workaround was to assign the controllers a unique name:

ctrl.NewControllerManagedBy(mgr).
	Named(r.Name()). // this assigns the controller a unique name
	For(&MyType{}).
	Complete(r)

Can you use the test name as your controller name to avoid the conflict?

-	_, err = controller.New("c1", m, controller.Options{Reconciler: rec})
+	_, err = controller.New(t.Name(), m, controller.Options{Reconciler: rec})