🐛 Modify multinamespaced cache to support cluster scoped resources

[varshaprasad96]

This PR modifies the multinamespacedcache implementation to:

• create a global cache mapping for an empty namespace, so that when
  cluster scoped resources are fetched, namespace is not required.
• deduplicate the objects in the List call, based on
  unique combination of resource name and namespace.

Closes: #1377
Closes: #1378

Signed-off-by: varshaprasad96 varshaprasad96@gmail.com

[varshaprasad96]

cc: @alvaroaleman @estroz

[estroz]

The behavior right now is a little weird. If I pass client.InNamespace(""), which is corev1.NamespaceAll, multiNamespaceCache.List() will first call the List() method of all the namespaced caches it knows about then that of the global cache, making those namespaced calls redundant. If I set client.InNamespace("my-ns"), then removeDuplicates() is called unnecessarily.

IMO the correct behavior is as follows:

namespaces := getNamespaces(opts)
if contains(namespaces, "") {
	return globalCache.List()
} else {
	var list []client.Object
	for _, namespace := range namespaces {
		out := c.caches[namespace].List()
		list = append(list, out...)
	}
	return list
}

This change is breaking because the caller needs to specify every namespace it wants to list from if non-global. However it results in corev1.NamespaceAll being correctly respected.

An alternative is to define const NamespaceMulti = "__multi", that can be passed like

cache.List(ctx, list, client.InNamespace(pkgcache.NamespaceMulti)

for listing from all cache namespaces.

Then the above would be

namespaces := getNamespaces(opts)
if contains(namespaces, "") {
	return globalCache.List()
} else if contains(namespaces, NamespaceMulti) {
	var list []client.Object
	for _, cache := range c.caches {
		out := cache.List()
		list = append(list, out...)
	}
	return list
} else {
	var list []client.Object
	for _, namespace := range namespaces {
		out := c.caches[namespace].List()
		list = append(list, out...)
	}
	return list
}

[varshaprasad96]

Since this PR references a bug, will do refactoring in follow up.

[varshaprasad96]

Have renamed this file from filter.go to objectutil.go to make it generic

[estroz]

This should be labelled as 🐛 @varshaprasad96

[alvaroaleman]

/lgtm
/approve
/label tide/merge-method-squash
/hold
@estroz anything to add or can we merge this?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

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

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]

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

[estroz]

Nope LGTM!
/hold cancel

[sbueringer]

@varshaprasad96 @alvaroaleman Could it be that this change leads to get calls on a _cluster-scope Namespace?

E0508 14:49:56.301011       1 reflector.go:138] external/io_k8s_client_go/tools/cache/reflector.go:167: Failed to watch *v1.Pod: failed to list *v1.Pod: pods is forbidden: User "system:serviceaccount:kube-system:dhc-resource-burster" cannot list resource "pods" in API group "" in the namespace "_cluster-scope"

with higher log-level:

I0508 17:01:13.539542 4157540 round_trippers.go:432] GET https://53.6.42.77:6443/api/v1/namespaces/_cluster-scope/pods?allowWatchBookmarks=true&resourceVersion=3148&timeoutSeconds=506&watch=true

We're creating the multinamespaced cache like this: (in this case with namespaces=kube-system)

namespacedCache = cache.MultiNamespacedCacheBuilder(strings.Split(namespaces, ","))

I only noticed the error because I'm running this controller only with rights to the kube-system namespace.

[estroz]

@sbueringer yep. It’s probably worth renaming this value to something like _multinamespace_cache_global_id to make errors like this traceable, or checking for this error type and returning a better message.

[alvaroaleman]

But that is wrong, we shouldn't be creating a cluster-scoped cache unless something cluster-scoped is actually requested?

[estroz]

Oh yeah I didn’t notice that, whoops. Agreed that looks like a bug. It doesn’t look like the global cache is being created/used though; instead, the global cache’s namespace is being used in a namespaces lookup. Is it not being filtered out when iterating through the cache set during a list?

[varshaprasad96]

The global cache is being created by default here. Only when cluster scoped objects are requested we look up there.
I think the bug is here. The global namespace shouldn't be looked at and be filtered out of the list.

[varshaprasad96]

@alvaroaleman @estroz I think #1520 should solve this, or is there something else which I am missing?

[sbueringer]

@varshaprasad96 I tried your fix locally and it wasn't enough, but it's a bit hard to say where the call is coming from. (I set a breakpoint at the reflector log line)

[sbueringer]

I think the problem is at least also that we start an informer for the "_cluster-scope namespace" here: https://github.com/kubernetes-sigs/controller-runtime/pull/1520/files#diff-cf71f1c7387ae1d9a63da1f81380effac1b826d0c3c1594c4d05da4324be2159R105-R116

[alvaroaleman]

@sbueringer that link doesn't work, can you just link to the code on a branch? Do you mean this

controller-runtime/pkg/cache/multi_namespace_cache.go

Lines 105 to 116 in 745c7c9

	func (c *multiNamespaceCache) Start(ctx context.Context) error {
	for ns, cache := range c.namespaceToCache {
	go func(ns string, cache Cache) {
	err := cache.Start(ctx)
	if err != nil {
	log.Error(err, "multinamespace cache failed to start namespaced informer", "namespace", ns)
	}
	}(ns, cache)
	}
	<-ctx.Done()
	return nil
	}

?

[sbueringer]

@alvaroaleman sorry, yes that's the part I mean