Fix the MultiScopedCache

cmd/app/app.go

@@ -66,7 +66,6 @@ func NewCommand() *cobra.Command {
 				LeaderElection:                true,
 				LeaderElectionNamespace:       opts.Bundle.Namespace,
 				NewCache:                      bundle.NewCacheFunc(opts.Bundle),
-				ClientDisableCacheFor:         bundle.ClientDisableCacheFor(),
 				LeaderElectionID:              "trust-manager-leader-election",
 				LeaderElectionReleaseOnCancel: true,
 				ReadinessEndpointName:         opts.ReadyzPath,

pkg/bundle/internal/cache.go → pkg/bundle/cache/cache.go

@@ -14,12 +14,14 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package internal
+package cache
 
 import (
 	"context"
+	"fmt"
 	"sync"
 
+	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/client-go/rest"
@@ -36,6 +38,9 @@ var _ cache.Cache = &multiScopedCache{}
 // namespace, whilst the other Namespaced resources in all namespaces.
 // It wraps both the default and Namespaced controller-runtime Cache.
 type multiScopedCache struct {
+	// scheme is the scheme used to determine the GVK for objects.
+	scheme *runtime.Scheme
+
 	// namespacedInformers is the set of resource types that should only be
 	// watched in the namespace pool.
 	namespacedInformers []schema.GroupKind
@@ -53,21 +58,22 @@ type multiScopedCache struct {
 // namespacedInformers expects Namespaced resource types.
 func NewMultiScopedCache(namespace string, namespacedInformers []schema.GroupKind) cache.NewCacheFunc {
 	return func(config *rest.Config, opts cache.Options) (cache.Cache, error) {
-		namespacedCache, err := cache.New(config, cache.Options{
-			Scheme:            opts.Scheme,
-			Mapper:            opts.Mapper,
-			Namespace:         namespace,
-			Resync:            opts.Resync,
-			SelectorsByObject: opts.SelectorsByObject,
-		})
+		namespacedOpts := opts
+		namespacedOpts.Namespace = namespace
+		clusterOpts := opts
+		clusterOpts.Namespace = ""
+
+		namespacedCache, err := cache.New(config, namespacedOpts)
 		if err != nil {
 			return nil, err
 		}
-		clusterCache, err := cache.New(config, opts)
+		clusterCache, err := cache.New(config, clusterOpts)
 		if err != nil {
 			return nil, err
 		}
+
 		return &multiScopedCache{
+			scheme:              opts.Scheme,
 			namespacedCache:     namespacedCache,
 			clusterCache:        clusterCache,
 			namespacedInformers: namespacedInformers,
@@ -77,13 +83,26 @@ func NewMultiScopedCache(namespace string, namespacedInformers []schema.GroupKin
 
 // GetInformer returns the underlying cache's GetInformer based on resource type.
 func (b *multiScopedCache) GetInformer(ctx context.Context, obj client.Object) (cache.Informer, error) {
-	return b.cacheFromGVK(obj.GetObjectKind().GroupVersionKind()).GetInformer(ctx, obj)
+	if err := setGroupVersionKind(b.scheme, obj); err != nil {
+		return nil, err
+	}
+
+	cache, err := b.cacheFromGVK(obj.GetObjectKind().GroupVersionKind())
+	if err != nil {
+		return nil, err
+	}
+	return cache.GetInformer(ctx, obj)
 }
 
 // GetInformerForKind returns the underlying cache's GetInformerForKind based
 // on resource type.
 func (b *multiScopedCache) GetInformerForKind(ctx context.Context, gvk schema.GroupVersionKind) (cache.Informer, error) {
-	return b.cacheFromGVK(gvk).GetInformerForKind(ctx, gvk)
+
+	cache, err := b.cacheFromGVK(gvk)
+	if err != nil {
+		return nil, err
+	}
+	return cache.GetInformerForKind(ctx, gvk)
 }
 
 // Start starts both the cluster and namespaced caches. Returned is an
@@ -126,26 +145,77 @@ func (b *multiScopedCache) WaitForCacheSync(ctx context.Context) bool {
 
 // IndexField returns the underlying cache's IndexField based on resource type.
 func (b *multiScopedCache) IndexField(ctx context.Context, obj client.Object, field string, extractValue client.IndexerFunc) error {
-	return b.cacheFromGVK(obj.GetObjectKind().GroupVersionKind()).IndexField(ctx, obj, field, extractValue)
+	if err := setGroupVersionKind(b.scheme, obj); err != nil {
+		return err
+	}
+
+	cache, err := b.cacheFromGVK(obj.GetObjectKind().GroupVersionKind())
+	if err != nil {
+		return err
+	}
+	return cache.IndexField(ctx, obj, field, extractValue)
 }
 
 // Get returns the underlying cache's Get based on resource type.
 func (b *multiScopedCache) Get(ctx context.Context, key client.ObjectKey, obj client.Object) error {
-	return b.cacheFromGVK(obj.GetObjectKind().GroupVersionKind()).Get(ctx, key, obj)
+	if err := setGroupVersionKind(b.scheme, obj); err != nil {
+		return err
+	}
+
+	cache, err := b.cacheFromGVK(obj.GetObjectKind().GroupVersionKind())
+	if err != nil {
+		return err
+	}
+	return cache.Get(ctx, key, obj)
 }
 
 // List returns the underlying cache's List based on resource type.
 func (b *multiScopedCache) List(ctx context.Context, list client.ObjectList, opts ...client.ListOption) error {
-	return b.cacheFromGVK(list.GetObjectKind().GroupVersionKind()).List(ctx, list, opts...)
+	if err := setGroupVersionKind(b.scheme, list); err != nil {
+		return err
+	}
+
+	cache, err := b.cacheFromGVK(list.GetObjectKind().GroupVersionKind())
+	if err != nil {
+		return err
+	}
+	return cache.List(ctx, list, opts...)
 }
 
 // cacheFromGVK returns either the cluster or namespaced cache, based on the
 // resource type given.
-func (b *multiScopedCache) cacheFromGVK(gvk schema.GroupVersionKind) cache.Cache {
+func (b *multiScopedCache) cacheFromGVK(gvk schema.GroupVersionKind) (cache.Cache, error) {
+	if gvk.Group == "" && gvk.Kind == "" {
+		return nil, fmt.Errorf("the Group and/or Kind must be set")
+	}
+
 	for _, namespacedInformer := range b.namespacedInformers {
 		if namespacedInformer.Group == gvk.Group && namespacedInformer.Kind == gvk.Kind {
-			return b.namespacedCache
+			return b.namespacedCache, nil
 		}
 	}
-	return b.clusterCache
+	return b.clusterCache, nil
+}
+
+// setGroupVersionKind populates the Group and Kind fields of obj using the
+// scheme type registry.
+// Inspired by https://github.com/kubernetes-sigs/controller-runtime/issues/1735#issuecomment-984763173
+func setGroupVersionKind(scheme *runtime.Scheme, obj runtime.Object) error {
+	gvk := obj.GetObjectKind().GroupVersionKind()
+	if gvk.Group != "" || gvk.Kind != "" || scheme == nil {
+		return nil // eg. in case of PartialMetadata, we don't want to overwrite the Group/ Kind
+	}
+
+	gvks, unversioned, err := scheme.ObjectKinds(obj)
+	if err != nil {
+		return err
+	}
+	if unversioned {
+		return fmt.Errorf("ObjectKinds unexpectedly returned unversioned: %#v", unversioned)
+	}
+	if len(gvks) != 1 {
+		return fmt.Errorf("ObjectKinds unexpectedly returned zero or multiple gvks: %#v", gvks)
+	}
+	obj.GetObjectKind().SetGroupVersionKind(gvks[0])
+	return nil
 }

pkg/bundle/controller.go

@@ -37,7 +37,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	trustapi "github.com/cert-manager/trust-manager/pkg/apis/trust/v1alpha1"
-	"github.com/cert-manager/trust-manager/pkg/bundle/internal"
+	multiscopedcache "github.com/cert-manager/trust-manager/pkg/bundle/cache"
 	"github.com/cert-manager/trust-manager/pkg/fspkg"
 )
 
@@ -195,11 +195,5 @@ func (b *bundle) mustBundleList(ctx context.Context) *trustapi.BundleList {
 // NewCacheFunc will return a multi-scoped controller-runtime NewCacheFunc
 // where Secret resources will only be watched within the trust Namespace.
 func NewCacheFunc(opts Options) cache.NewCacheFunc {
-	return internal.NewMultiScopedCache(opts.Namespace, []schema.GroupKind{{Kind: "Secret"}})
-}
-
-// ClientDisableCacheFor returns resources which should only be watched within
-// the Trust Namespace, and not at the cluster level.
-func ClientDisableCacheFor() []client.Object {
-	return []client.Object{new(corev1.Secret)}
+	return multiscopedcache.NewMultiScopedCache(opts.Namespace, []schema.GroupKind{{Kind: "Secret"}})
 }

test/integration/bundle/cache_test.go

@@ -0,0 +1,153 @@
+/*
+Copyright 2021 The cert-manager Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package test
+
+import (
+	"context"
+	"fmt"
+	"time"
+
+	. "github.com/onsi/ginkgo/v2"
+	. "github.com/onsi/gomega"
+
+	corev1 "k8s.io/api/core/v1"
+	rbacv1 "k8s.io/api/rbac/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
+	"k8s.io/client-go/rest"
+	"sigs.k8s.io/controller-runtime/pkg/cache"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+
+	multiscopedcache "github.com/cert-manager/trust-manager/pkg/bundle/cache"
+)
+
+var _ = Describe("Integration test cache", func() {
+	It("should be possible to Get a resource without having cluster-wide List & Watch permissions", func() {
+		ctx, cancel := context.WithTimeout(context.Background(), 20*time.Second)
+		defer cancel()
+
+		namespace := "test-namespace"
+
+		// Create a service account that can only retrieve secrets in a single namespace.
+		var cacheRestConfig *rest.Config
+		{
+			godClient, err := client.New(env.Config, client.Options{})
+			Expect(err).NotTo(HaveOccurred())
+
+			ns := &corev1.Namespace{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: namespace,
+				},
+			}
+			err = godClient.Create(ctx, ns)
+			Expect(err).NotTo(HaveOccurred())
+
+			sa := &corev1.ServiceAccount{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cache-sa",
+					Namespace: namespace,
+				},
+			}
+			err = godClient.Create(ctx, sa)
+			Expect(err).NotTo(HaveOccurred())
+
+			role := &rbacv1.Role{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cache-role",
+					Namespace: namespace,
+				},
+				Rules: []rbacv1.PolicyRule{
+					{
+						Verbs:     []string{"list", "watch"},
+						APIGroups: []string{""},
+						Resources: []string{"secrets"},
+					},
+				},
+			}
+			err = godClient.Create(ctx, role)
+			Expect(err).NotTo(HaveOccurred())
+
+			rolebinding := rbacv1.RoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "cache-rolebinding",
+					Namespace: namespace,
+				},
+				RoleRef: rbacv1.RoleRef{
+					APIGroup: "rbac.authorization.k8s.io",
+					Kind:     "Role",
+					Name:     "cache-role",
+				},
+				Subjects: []rbacv1.Subject{
+					{
+						Kind:      "ServiceAccount",
+						Name:      "cache-sa",
+						Namespace: namespace,
+					},
+				},
+			}
+			err = godClient.Create(ctx, &rolebinding)
+			Expect(err).NotTo(HaveOccurred())
+
+			// Create a config that uses the service account.
+			cacheRestConfig = rest.CopyConfig(env.Config)
+			cacheRestConfig.Impersonate.UserName = fmt.Sprintf("system:serviceaccount:%s:%s", namespace, "cache-sa")
+			cacheRestConfig.Impersonate.UID = string(sa.UID)
+
+			// Create a secret that the service account can access.
+			secret := &corev1.Secret{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-secret",
+					Namespace: namespace,
+				},
+				Data: map[string][]byte{
+					"test": []byte("test"),
+				},
+			}
+			err = godClient.Create(ctx, secret)
+			Expect(err).NotTo(HaveOccurred())
+		}
+
+		newCache := multiscopedcache.NewMultiScopedCache(namespace, []schema.GroupKind{{Group: "", Kind: "Secret"}})
+
+		scheme := runtime.NewScheme()
+		Expect(corev1.AddToScheme(scheme)).NotTo(HaveOccurred())
+		cache, err := newCache(cacheRestConfig, cache.Options{
+			Scheme: scheme,
+		})
+		Expect(err).NotTo(HaveOccurred())
+
+		done := make(chan error)
+		go func() {
+			done <- cache.Start(ctx)
+		}()
+		defer func() {
+			Expect(<-done).NotTo(HaveOccurred())
+		}()
+
+		Expect(cache.WaitForCacheSync(ctx)).To(BeTrue())
+
+		secret := &corev1.Secret{}
+		err = cache.Get(ctx, client.ObjectKey{
+			Namespace: namespace,
+			Name:      "test-secret",
+		}, secret)
+		Expect(err).NotTo(HaveOccurred())
+
+		Expect(secret.Data["test"]).To(Equal([]byte("test")))
+	})
+})