DefaultNamespaces Cache Option doesn't allow All Namespaces for List

[manno]

I'm trying to limit the controller to watch config maps only in one namespace, while watching other resources in all namespaces. Mainly for performance reasons, I don't want the controller to trigger for any config map event in the cluster.

The cache design document describes the config as DefaultNamespaces map[string]*Config, while the code implements DefaultNamespaces map[string]Config. Since all options are nil-able I guess that's not an issue.

From the cache tests I'd expect this to allow list and get in all namespaces:

Cache: cache.Options{
	DefaultNamespaces: map[string]cache.Config{cache.AllNamespaces: {}},
},

However, it results in unable to list: test-7af124d because of unknown namespace for the cache.
I think that's because theList func in multi cache is missing the special handling, that was added to Get func:

diff --git a/pkg/cache/multi_namespace_cache.go b/pkg/cache/multi_namespace_cache.go
index 87c31a7c..8e58b0c0 100644
--- a/pkg/cache/multi_namespace_cache.go
+++ b/pkg/cache/multi_namespace_cache.go
@@ -237,6 +237,9 @@ func (c *multiNamespaceCache) List(ctx context.Context, list client.ObjectList,
        if listOpts.Namespace != corev1.NamespaceAll {
                cache, ok := c.namespaceToCache[listOpts.Namespace]
                if !ok {
+                       if global, hasGlobal := c.namespaceToCache[metav1.NamespaceAll]; hasGlobal {
+                               return global.List(ctx, list, opts...)
+                       }
                        return fmt.Errorf("unable to list: %v because of unknown namespace for the cache", listOpts.Namespace)
                }
                return cache.List(ctx, list, opts...)

If I understand the consequences of cache options correctly, they do limit Watch, but also affect the cached client. I need another client, e.g. APIReader, to retrieve configmaps in other namespaces.

[troy0820]

/kind support

[alvaroaleman]

So you are limiting the watch for configmaps to one namespace using ByObject and then use DefaultNamespaces to wach everything else in all namespaces?

The DefaultNamespaces option does nothing for a type that has the Namespaces setting set. To quote from the godocs:

controller-runtime/pkg/cache/cache.go

Lines 233 to 234 in 1b80b96

	// Namespaces maps a namespace name to cache configs. If set, only the
	// namespaces in this map will be cached.

Also, the default is to cache all namespaces without further selection logic, so putting cache.AllNamespaces: {} into DefaultNamespaces does nothing either.

And yeah, the client reads from the cache by default, hence if you limit the cache to a given namespace, you can't use the default client to read from a different namespace.

Does this answer your question? Do you think the godocs are unclear here?

[manno]

So you are limiting the watch for configmaps to one namespace using ByObject and then use DefaultNamespaces to wach everything else in all namespaces.

Yes, that was my intention, as I expect configmaps to change a lot, but I'm only interested in changes to one specific config map (the controllers "config" file).

The DefaultNamespaces option does nothing for a type that has the Namespaces setting set. To quote from the godocs:

controller-runtime/pkg/cache/cache.go

Lines 233 to 234 in 1b80b96

	// Namespaces maps a namespace name to cache configs. If set, only the
	// namespaces in this map will be cached.

I see now, that

controller-runtime/pkg/cache/cache.go

Lines 239 to 242 in 1b80b96

	// It is possible to have specific Config for just some namespaces
	// but cache all namespaces by using the AllNamespaces const as the map key.
	// This will then include all namespaces that do not have a more specific
	// setting.

only refers to AllNamespaces/NamespaceAll in ByObject, like in this test.

However, the DefaultNamespace option suggests it's possible to use both together. That still leaves me confused about this godoc:

controller-runtime/pkg/cache/cache.go

Lines 182 to 189 in 1b80b96

	// DefaultNamespaces maps namespace names to cache configs. If set, only
	// the namespaces in here will be watched and it will by used to default
	// ByObject.Namespaces for all objects if that is nil.
	//
	// It is possible to have specific Config for just some namespaces
	// but cache all namespaces by using the AllNamespaces const as the map key.
	// This will then include all namespaces that do not have a more specific
	// setting.

I read this and thought, it would apply DefaultNamespace to namespaces not listed in ByObject.Namespaces.

I'm pretty sure it works for Get, because of https://github.com/kubernetes-sigs/controller-runtime/pull/2539/files, but not for List: https://github.com/manno/cr-cache-test/blob/main/cachetest/cache_options_test.go#L65-L66