🐛 Avoid creating a new scheme object when combining the options

[ccojocar]

This keeps the inherited scheme object created by the cluster when combining the options instead of creating a new one.
That scheme object can be used later on to register schemes.

The inherited scheme was already configured in the cluster object, by creating a new scheme object
for the cache object when combining the schemes will not allow the schemes registered afterwards into the
cluster scheme object to be visible into the cache, and it will result in an error when the controller is started (see below).

Configure the controller as follows:

	ctrlOpts := ctrl.Options{
		NewCache: cache.BuilderWithOptions(cache.Options{
			SelectorsByObject: cache.SelectorsByObject{
				&corev1.Pod{}: {
					Label: labels.SelectorFromSet(labels.Set{
						bindata.EnableRecordingLabel: "true",
					}),
				},
			},
		}),
	}

	mgr, err := ctrl.NewManager(cfg, ctrlOpts)
	if err != nil {
		return fmt.Errorf("create manager: %w", err)
	}

        if err := spodv1alpha1.AddToScheme(mgr.GetScheme()); err != nil {
		return fmt.Errorf("add SPOD config API to scheme: %w", err)
	}

        if err := mgr.Start(sigHandler); err != nil {
		return fmt.Errorf("SPOd error: %w", err)
	}

This fix will allow the controller to start properly instead of getting this error that the scheme is not registered:

E0314 10:44:24.541365 1302480 source.go:146] controller-runtime/source "msg"="kind must be registered to the Scheme" 
"error"="no kind is registered for the type v1alpha1.SecurityProfilesOperatorDaemon in scheme \"pkg/runtime/scheme.go:100"  

You can see more details in kubernetes-sigs/security-profiles-operator#1543 where we encountered this issue.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ccojocar
Once this PR has been reviewed and has the lgtm label, please assign joelanford for approval. For more information see the Kubernetes Code Review Process.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[alvaroaleman]

Please provide a testcase for the underlying bug

[ccojocar]

Please provide a testcase for the underlying bug

I'll try to add one.

[ccojocar]

@alvaroaleman I added a test case in manager. I hope that's the right place for this.

[alvaroaleman]

I don't understand what exactly this is supposed to test and how this relates to the problem you described in the PR body? It also passes without your codechanges btw. It might be easier to unittest combineScheme directly

[ccojocar]

I am trying to test the scenario mentioned above in the description.

The main issue is when a scheme is combined using the BuilderWithOptions function, a new scheme object pointer is created and that pointer is configured into the cache -

controller-runtime/pkg/cache/cache.go

Line 300 in c3c1f05

out = runtime.NewScheme()

.

The issue is that the original scheme object pointer is configured into the manager/controller and not the newly created one by combineScheme, so this means all the schemes added to mgr.GetScheme(), object are no available in the cache. As a result, the controller will fail to start, returning the error "msg"="kind must be registered to the Scheme".

I think just unit-testing the combineScheme doesn't cover this scenario end to end. Also the combineScheme has unittest which already covers this entirely.

[joelanford]

Won't this leak new scheme types into the inherited scheme? What happens here if (somehow) a conflicting scheme is required by a different cache, and that cache shares the same inherited scheme? Will we change the scheme out from under the first cache?

[joelanford]

This may be an area of the design where we didn't really consider the possibility of schemes changing dynamically.

Should adding more types to mgr.GetScheme() mean anything that inherited that scheme in the past pick up those changes? What if the inheritee has already added that type in its sub-scheme?

This seems like it could absolutely be an issue in the case of a multi-cluster controller where the schemes for different clusters may need to have conflicting registrations. I can't personally think of a valid case where the same GVK in two different clusters (or maybe even different contexts within the same cluster?) would need a different go type associated with them, but it doesn't seem like we should preclude that possibility.

[vincepri]

This seems like it could absolutely be an issue in the case of a multi-cluster controller where the schemes for different clusters may need to have conflicting registrations. I can't personally think of a valid case where the same GVK in two different clusters (or maybe even different contexts within the same cluster?) would need a different go type associated with them, but it doesn't seem like we should preclude that possibility.

It should probably be a hard limitation that the scheme for the time being has to be global.

In terms of this PR, I'm ok allowing the Scheme to be shared, given that's one of those things that's already passed around

[alvaroaleman]

The corresponding core was removed in #2300, so this isn't needed anymore.