From 61f940cb172e0311bb61d75903d034324b610304 Mon Sep 17 00:00:00 2001 From: Josh Gwosdz Date: Mon, 28 Apr 2025 15:45:46 +0200 Subject: [PATCH 1/4] chore: export managedcache.RefType Signed-off-by: Josh Gwosdz --- managedcache/objectboundaccess.go | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/managedcache/objectboundaccess.go b/managedcache/objectboundaccess.go index 38a06df..7af9391 100644 --- a/managedcache/objectboundaccess.go +++ b/managedcache/objectboundaccess.go @@ -24,7 +24,7 @@ import ( // ObjectBoundAccessManager manages caches and clients bound to objects. // Each object instance will receive it's own cache and client instance. -type ObjectBoundAccessManager[T refType] interface { +type ObjectBoundAccessManager[T RefType] interface { manager.Runnable // Get returns a TrackingCache for the provided object if one exists. // If one does not exist, a new Cache is created and returned. @@ -59,7 +59,7 @@ type Accessor interface { } // NewObjectBoundAccessManager returns a new ObjectBoundAccessManager for T. -func NewObjectBoundAccessManager[T refType]( +func NewObjectBoundAccessManager[T RefType]( log logr.Logger, mapConfig ConfigMapperFunc[T], baseRestConfig *rest.Config, @@ -82,20 +82,21 @@ func NewObjectBoundAccessManager[T refType]( } } -type refType interface { +// RefType constrains the owner type of an ObjectBoundAccessManager. +type RefType interface { client.Object comparable } // ConfigMapperFunc applies changes to rest.Config and cache.Options based on the given object. -type ConfigMapperFunc[T refType] func( +type ConfigMapperFunc[T RefType] func( context.Context, T, *rest.Config, cache.Options) (*rest.Config, cache.Options, error) type newClientFunc func(config *rest.Config, opts client.Options) (client.Client, error) var _ ObjectBoundAccessManager[client.Object] = (*objectBoundAccessManagerImpl[client.Object])(nil) -type objectBoundAccessManagerImpl[T refType] struct { +type objectBoundAccessManagerImpl[T RefType] struct { log logr.Logger scheme *runtime.Scheme restMapper meta.RESTMapper @@ -117,7 +118,7 @@ type accessorEntry struct { cancel func() } -type accessorRequest[T refType] struct { +type accessorRequest[T RefType] struct { owner T user client.Object gvks sets.Set[schema.GroupVersionKind] From 0b8e8708b48350f22915cb8f999253615b84c694 Mon Sep 17 00:00:00 2001 From: Josh Gwosdz Date: Mon, 28 Apr 2025 15:52:44 +0200 Subject: [PATCH 2/4] fix(managedcache): use correct user UID to store gvk entries during cache accessor requests This fixes a bug where gcCache tries to close a nil channel in the cacheWaitInFlight map. ``` 2025-04-28T13:02:17Z INFO ObjectBoundAccessManager.TrackingCache stopping informers {"gvks": [{"Group":"[apiextensions.k8s.io](http://apiextensions.k8s.io)","Version":"v1","Kind":"CustomResourceDefinition"}]} panic: close of nil channel goroutine 455 [running]: pkg.package-operator.run/boxcutter/managedcache.(*trackingCache).RemoveOtherInformers.func1({0x27ff2f0, 0xc001103da0}) pkg.package-operator.run/boxcutter@v0.1.0/managedcache/trackingcache.go:391 +0x4d9 pkg.package-operator.run/boxcutter/managedcache.(*trackingCache).Start(0xc00084ab40, {0x27ff328, 0xc00086cb90}) pkg.package-operator.run/boxcutter@v0.1.0/managedcache/trackingcache.go:142 +0x2dd pkg.package-operator.run/boxcutter/managedcache.(*objectBoundAccessManagerImpl[...]).handleAccessorRequest.func1(0xc0000584d0) pkg.package-operator.run/boxcutter@v0.1.0/managedcache/objectboundaccess.go:309 +0xaf created by pkg.package-operator.run/boxcutter/managedcache.(*objectBoundAccessManagerImpl[...]).handleAccessorRequest in goroutine 217 pkg.package-operator.run/boxcutter@v0.1.0/managedcache/objectboundaccess.go:307 +0xb6c ``` Co-Authored-By: Nico Schieder Signed-off-by: Josh Gwosdz --- managedcache/objectboundaccess.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/managedcache/objectboundaccess.go b/managedcache/objectboundaccess.go index 7af9391..d5f9d21 100644 --- a/managedcache/objectboundaccess.go +++ b/managedcache/objectboundaccess.go @@ -251,7 +251,7 @@ func (m *objectBoundAccessManagerImpl[T]) handleAccessorRequest( log.V(-1).Info("reusing cache for owner") if req.user != nil { - entry.users[req.owner.GetUID()] = req.gvks + entry.users[req.user.GetUID()] = req.gvks } return entry.accessor, m.gcCache(ctx, req.owner) From fa531aec5114e0db7a0ea01e47ac31dc0b462e6f Mon Sep 17 00:00:00 2001 From: Josh Gwosdz Date: Tue, 29 Apr 2025 18:12:18 +0200 Subject: [PATCH 3/4] fix(managedcache): synthesize an unstructured object and call cache.GetInformer instead of cache.GetInformerForKind to avoid a scheme lookup error Error: object /a-foo kind:Foo: getting k8s.erdii.net/v1, Kind=Foo: no kind \"Foo\" is registered for version \"k8s.erdii.net/v1\" in scheme \"pkg/runtime/scheme.go:100\" we're using controller-runtime's informerCache [1] and that implementation's GetInformerForKind tries to instanciate a new typed struct for the given gvk [2]. This does not work because the scheme does not contain the type. [1] https://github.com/kubernetes-sigs/controller-runtime/blob/6ad5c1dd4418489606d19dfb87bf38905b440561/pkg/cache/informer_cache.go#L67 [2] https://github.com/kubernetes-sigs/controller-runtime/blob/6ad5c1dd4418489606d19dfb87bf38905b440561/pkg/cache/informer_cache.go#L155 --- managedcache/trackingcache.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/managedcache/trackingcache.go b/managedcache/trackingcache.go index b81620a..3613df4 100644 --- a/managedcache/trackingcache.go +++ b/managedcache/trackingcache.go @@ -229,7 +229,12 @@ func (c *trackingCache) ensureCacheSyncForGVK(ctx context.Context, gvk schema.Gr return } - i, err := c.Cache.GetInformerForKind(ctx, gvk, cache.BlockUntilSynced(false)) + i, err := c.Cache.GetInformer(ctx, &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": gvk.GroupVersion().String(), + "kind": gvk.Kind, + }, + }, cache.BlockUntilSynced(false)) if err != nil { errCh <- err From accb5a882f25f6535e84890b84310149528c47a1 Mon Sep 17 00:00:00 2001 From: Josh Gwosdz Date: Fri, 2 May 2025 18:42:52 +0200 Subject: [PATCH 4/4] chore(managedcache): drop need for scheme in ObjectBoundAccessManager Signed-off-by: Josh Gwosdz --- cmd/reference/internal/reference.go | 2 +- managedcache/gvk.go | 21 ++++++++++++++++ managedcache/objectboundaccess.go | 13 +++------- managedcache/trackingcache.go | 39 +++++++++++++++++------------ 4 files changed, 49 insertions(+), 26 deletions(-) create mode 100644 managedcache/gvk.go diff --git a/cmd/reference/internal/reference.go b/cmd/reference/internal/reference.go index 5a990ff..a5f41a4 100644 --- a/cmd/reference/internal/reference.go +++ b/cmd/reference/internal/reference.go @@ -112,7 +112,7 @@ func (r *Reference) Start(ctx context.Context) error { mc := managedcache.NewObjectBoundAccessManager[*corev1.ConfigMap]( ctrl.Log, mapper, r.restConfig, cache.Options{ - Scheme: r.scheme, Mapper: mgr.GetRESTMapper(), + Mapper: mgr.GetRESTMapper(), }) if err := mgr.Add(mc); err != nil { return fmt.Errorf("adding managedcache: %w", err) diff --git a/managedcache/gvk.go b/managedcache/gvk.go new file mode 100644 index 0000000..e248bdc --- /dev/null +++ b/managedcache/gvk.go @@ -0,0 +1,21 @@ +package managedcache + +import ( + "errors" + "fmt" + + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +// ErrNonEmptyKindOrVersion is returned when either kind or version of the given object are empty. +var ErrNonEmptyKindOrVersion = errors.New("object must have non-empty kind and version") + +func gvkForObject(obj runtime.Object) (schema.GroupVersionKind, error) { + gvk := obj.GetObjectKind().GroupVersionKind() + if gvk.Kind == "" || gvk.Version == "" { + return schema.GroupVersionKind{}, fmt.Errorf("%w: %s", ErrNonEmptyKindOrVersion, gvk) + } + + return gvk, nil +} diff --git a/managedcache/objectboundaccess.go b/managedcache/objectboundaccess.go index d5f9d21..6211c06 100644 --- a/managedcache/objectboundaccess.go +++ b/managedcache/objectboundaccess.go @@ -8,14 +8,12 @@ import ( "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/meta" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/rest" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" "sigs.k8s.io/controller-runtime/pkg/predicate" @@ -28,7 +26,7 @@ type ObjectBoundAccessManager[T RefType] interface { manager.Runnable // Get returns a TrackingCache for the provided object if one exists. // If one does not exist, a new Cache is created and returned. - Get(context.Context, T) (Accessor, error) + Get(ctx context.Context, owner T) (Accessor, error) // GetWithUser returns a TrackingCache for the provided object if one exist. // If one does not exist, a new Cache is created and returned. @@ -42,14 +40,14 @@ type ObjectBoundAccessManager[T RefType] interface { // Free will stop and remove a TrackingCache for // the provided object, if one exists. - Free(context.Context, T) error + Free(ctx context.Context, owner T) error // FreeWithUser informs the manager that the given user no longer needs // a cache scoped to owner T. If the cache has no active users, it will be stopped. FreeWithUser(ctx context.Context, owner T, user client.Object) error // Source returns a controller-runtime source to watch from a controller. - Source(handler.EventHandler, ...predicate.Predicate) source.Source + Source(handler handler.EventHandler, predicates ...predicate.Predicate) source.Source } // Accessor provides write and cached read access to the cluster. @@ -67,7 +65,6 @@ func NewObjectBoundAccessManager[T RefType]( ) ObjectBoundAccessManager[T] { return &objectBoundAccessManagerImpl[T]{ log: log.WithName("ObjectBoundAccessManager"), - scheme: baseCacheOptions.Scheme, restMapper: baseCacheOptions.Mapper, mapConfig: mapConfig, baseRestConfig: baseRestConfig, @@ -98,7 +95,6 @@ var _ ObjectBoundAccessManager[client.Object] = (*objectBoundAccessManagerImpl[c type objectBoundAccessManagerImpl[T RefType] struct { log logr.Logger - scheme *runtime.Scheme restMapper meta.RESTMapper mapConfig ConfigMapperFunc[T] baseRestConfig *rest.Config @@ -269,7 +265,6 @@ func (m *objectBoundAccessManagerImpl[T]) handleAccessorRequest( } client, err := m.newClient(restConfig, client.Options{ - Scheme: m.baseCacheOptions.Scheme, Mapper: m.baseCacheOptions.Mapper, HTTPClient: m.baseCacheOptions.HTTPClient, }) @@ -356,7 +351,7 @@ func (m *objectBoundAccessManagerImpl[T]) GetWithUser( gvks := sets.Set[schema.GroupVersionKind]{} for _, obj := range usedFor { - gvk, err := apiutil.GVKForObject(obj, m.scheme) + gvk, err := gvkForObject(obj) if err != nil { return nil, err } diff --git a/managedcache/trackingcache.go b/managedcache/trackingcache.go index 3613df4..8a270d6 100644 --- a/managedcache/trackingcache.go +++ b/managedcache/trackingcache.go @@ -3,6 +3,7 @@ package managedcache import ( "context" "errors" + "fmt" "strings" "sync" @@ -10,14 +11,12 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" - "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/rest" toolscache "k8s.io/client-go/tools/cache" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/client/apiutil" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/predicate" "sigs.k8s.io/controller-runtime/pkg/source" @@ -45,7 +44,6 @@ type cacheSourcer interface { type trackingCache struct { cache.Cache log logr.Logger - scheme *runtime.Scheme restMapper meta.RESTMapper cacheSourcer cacheSourcer @@ -81,7 +79,6 @@ func newTrackingCache( ) (TrackingCache, error) { wehc := &trackingCache{ log: log.WithName("TrackingCache"), - scheme: opts.Scheme, restMapper: opts.Mapper, cacheSourcer: cacheSourcer, @@ -196,23 +193,31 @@ func (c *trackingCache) handleCacheWatchError(err error) error { } func (c *trackingCache) ensureCacheSync(ctx context.Context, obj client.Object) error { - gvk, err := apiutil.GVKForObject(obj, c.scheme) + gvk, err := gvkForObject(obj) if err != nil { return err } - return c.ensureCacheSyncForGVK(ctx, gvk) + if err := c.ensureCacheSyncForGVK(ctx, gvk); err != nil { + return fmt.Errorf("ensuring cache sync for GVK: %w", err) + } + + return nil } func (c *trackingCache) ensureCacheSyncList(ctx context.Context, list client.ObjectList) error { - gvk, err := apiutil.GVKForObject(list, c.scheme) + gvk, err := gvkForObject(list) if err != nil { return err } // We need the non-list GVK, so chop off the "List" from the end of the kind. gvk.Kind = strings.TrimSuffix(gvk.Kind, "List") - return c.ensureCacheSyncForGVK(ctx, gvk) + if err := c.ensureCacheSyncForGVK(ctx, gvk); err != nil { + return fmt.Errorf("ensuring cache sync for (list) GVK: %w", err) + } + + return nil } func (c *trackingCache) ensureCacheSyncForGVK(ctx context.Context, gvk schema.GroupVersionKind) error { @@ -229,12 +234,9 @@ func (c *trackingCache) ensureCacheSyncForGVK(ctx context.Context, gvk schema.Gr return } - i, err := c.Cache.GetInformer(ctx, &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": gvk.GroupVersion().String(), - "kind": gvk.Kind, - }, - }, cache.BlockUntilSynced(false)) + obj := &unstructured.Unstructured{} + obj.SetGroupVersionKind(gvk) + i, err := c.Cache.GetInformer(ctx, obj, cache.BlockUntilSynced(false)) if err != nil { errCh <- err @@ -289,7 +291,12 @@ func (c *trackingCache) Get( return err } - return c.Cache.Get(ctx, key, obj, opts...) + err := c.Cache.Get(ctx, key, obj, opts...) + if err != nil { + return fmt.Errorf("getting object: %w", err) + } + + return nil } func (c *trackingCache) List( @@ -338,7 +345,7 @@ func (c *trackingCache) RemoveInformer(ctx context.Context, obj client.Object) e c.accessLock.Lock() defer c.accessLock.Unlock() - gvk, err := apiutil.GVKForObject(obj, c.scheme) + gvk, err := gvkForObject(obj) if err != nil { return err }