New Reconciler Option: SkipPrimaryGVKSchemeRegistration

[mtesseract]

Add a new Reconciler option SkipGenericGVKSchemeRegistration , which can be used by users who prefer their
custom scheme setup over the generic scheme setup currently being done by the reconciler.

Signed-off-by: Moritz Clasmeier moritz@stackrox.com

[joelanford]

Hi @mtesseract, this seems straightforward enough, but could you include a use case and maybe a code example of what the custom setup would look like if this option is enabled?

Is there a case where an operator author wouldn't need to add unstructured.Unstructed{} to the scheme for the primary objects?

[coveralls]

Pull Request Test Coverage Report for Build 1945818205

• 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.04%) to 89.604%

---

Totals
Change from base Build 1923150559:	0.04%
Covered Lines:	1560
Relevant Lines:	1741

---

💛 - Coveralls

[mtesseract]

Hi @joelanford,

Our use case is that we are developing a hybrid operator and we want to use custom types for configuring our custom resource instead of using the unstructured type for modelling underlying Helm chart's values.

Code for adding custom types to scheme:

var (
	SchemeGroupVersion = schema.GroupVersion{Group: ..., Version: ...}
	SchemeBuilder = runtime.NewSchemeBuilder(addKnownTypes)
	AddToScheme = SchemeBuilder.AddToScheme
)

func addKnownTypes(scheme *runtime.Scheme) error {
	scheme.AddKnownTypes(SchemeGroupVersion,
		&MyCustomType{},
	)

	metav1.AddToGroupVersion(scheme, SchemeGroupVersion)
	return nil
}

Then we set up new scheme and create a new manager for it:

var (
    scheme         = runtime.NewScheme()
)

// [...]

AddToScheme(scheme)

// [...]

mgr, err := ctrl.NewManager(ctrl.GetConfigOrDie(), ctrl.Options{
		Scheme:                 scheme,
                 // [...]
	})

Set up new reconciler for this scheme:

func SetupReconcilerWithManager(mgr ctrl.Manager, gvk schema.GroupVersionKind) error {

// [...]

reconcilerOpts := []reconciler.Option{
		reconciler.WithChart(*chart),
		reconciler.SkipSchemeSetup(true),
		// [...]
	        }
// [...]

r, err = reconciler.New(reconcilerOpts...)
// [...]

err = r.SetupWithManager(mgr)
// [...]

Is this sufficient? Thanks!

[joelanford]

@mtesseract Does the operator work if unstructured.Unstructured is not registered with the scheme as the primary GVK? The reason I ask is because the reconciler hardcodes usage of unstructured.Unstructured as the object type for the primary GVK when interacting with the client.

Semi-related question: Is it possible to register two different objects to the same GVK? If you add the concrete type to the scheme while still allowing the reconciler to add the unstructured type to the scheme for the same GVK, does that result in an error?

Lastly, could you add some tests in the reconciler, so we can verify the expected behavior now and avoid regressions in the future?

[joelanford]

Lastly, it looks like this PR needs to rebased. go-apidiff is reporting an API being removed that seems unrelated to this PR.

[mtesseract]

Hi.

Please excuse the late reply -- the pandemic forced me to take a care work leave for a few weeks.

Let me answer this here right away:

Semi-related question: Is it possible to register two different objects to the same GVK? If you add the concrete type to the scheme while still allowing the reconciler to add the unstructured type to the scheme for the same GVK, does that result in an error?

I hope I'm understanding your question correctly. So, my answer is: This is not possible, with the current implementation of the reconciler. It throws this:

logger.go:42: 12:40:41 | | panic: Double registration of different types for [...] in scheme "pkg/runtime/scheme.go:100"

The relevant piece of code is this:

func (s *Scheme) AddKnownTypeWithName(gvk schema.GroupVersionKind, obj Object) {
    [...]
	t := reflect.TypeOf(obj)
    [...]
	if oldT, found := s.gvkToType[gvk]; found && oldT != t {
		panic(fmt.Sprintf("Double registration of different types for %v: old=%v.%v, new=%v.%v in scheme %q", gvk, oldT.PkgPath(), oldT.Name(), t.PkgPath(), t.Name(), s.schemeName))
	}
    [...]
}

[mtesseract]

@mtesseract Does the operator work if unstructured.Unstructured is not registered with the scheme as the primary GVK? The reason I ask is because the reconciler hardcodes usage of unstructured.Unstructured as the object type for the primary GVK when interacting with the client.

Yes, the underlying object -- even if a custom type -- continues to be accessible as an unstructured.Unstructured (tagging @SimonBaeumer, who has more hands-on experience with this part of the code).

[joelanford]

@SimonBaeumer and I chatted about this in the community helm-operator meeting, and he successfully convinced me this is a necessary change.

There are a few things I think still need to be done.

Improvement: can we augment the reconciler tests with a scenario that runs the reconciler with this option enabled to show that registering the GVK with a concrete API type doesn't break the reconciler's use of unstructured.Unstructured?

Nits:

1. Can we iterate a bit on the name to make it more specific (e.g. SkipPrimaryGVKSchemeRegistration). That might be too verbose, but something along those lines?
2. Can we improve the GoDoc on the option function to go into more detail about why someone might want to disable the built-in registration, and an example of what they would need to do to manually register the GVK separately?

[mtesseract]

@joelanford , regarding the naming, going with something longer but more descriptive is of course fine for me.
I would suggest a minor modification of your proposal: SkipGenericGVKSchemeRegistration -- what do you think?

[mtesseract]

@joelanford, looking forward to your feedback.

[joelanford]

@mtesseract There's another case that comes to mind.

Scenario:

• Consider a helm chart that has some manifests for some custom APIs
• In a post-hook, you want to use the controller-runtime client to fetch one of these custom objects that was applied via the helm release.
• client.Get fails because the GVK for that object is not registered in the scheme

What would we expect to happen here? I can think of two approaches.

1. A library user MUST configure the scheme for these kinds of objects because they are the ones providing the post-hook that attempts to fetch the object.
2. A library user MAY configure the scheme so that their post-hook could use something other than unstructured.Unstructured.

I'm fairly sure option 1 is the existing (perhaps unintended) implementation. We currently make no attempt to setup the scheme for a anything other than the primary object being reconciled.

However I think option 2 is possible if we wanted to go that route (i.e. similar to how we dynamically register watches for release object types, we could also check and update the scheme, always using unstructured for anything not already registered).

---

^ Does that all make sense and seem like a reasonable use case?

If so, does that impact the name we choose for this? If we go with SkipGenericGVKSchemeRegistration, that would seemingly apply to all scheme registration done by the reconciler.

On the otherhand, SkipPrimaryGVKSchemeRegistration would seemingly only apply to the primary CR type.

If we think we need to distinguish, I think Primary is better than Generic
If we think we don't need to distinguish, then perhaps just the original SkipSchemeSetup name suffices.

WDYT?

[mtesseract]

I have updated the PR to use the name you have suggested (SkipPrimaryGVKSchemeRegistration).

Also, regarding your example, I think you are right in that it would be user's responsibility to register any types in the scheme that are to be used, e.g. in hooks.

[asmacdo]

/ok-to-test

[asmacdo]

/cc @jmrodri

[joelanford]

/lgtm