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 https://github.com/kubernetes-sigs/controller-runtime/pull/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: https://github.com/kubernetes-sigs/controller-runtime/blob/610870ea28c1a025a4e71781327d748a9000096c/pkg/manager/manager.go#L100-L107

LE: the configuration is going through the config package actually https://github.com/kubernetes-sigs/controller-runtime/blob/610870ea28c1a025a4e71781327d748a9000096c/pkg/manager/manager.go#L267-L270

[alvaroaleman]

managerOpts.Controller.SkipNameValidation, yes.