From be1010fc3b93665cfe5361cb23f9b514cca6ce19 Mon Sep 17 00:00:00 2001 From: Evan Cordell Date: Tue, 30 Apr 2024 17:12:58 -0400 Subject: [PATCH 1/2] Add non-panicing versions of ListerFor, IndexerFor and prefix the panicing versions with Must* --- adopt/adopt_test.go | 2 +- component/ensure_component_test.go | 2 +- handler/handler.go | 2 +- queue/controls_test.go | 4 +- typed/registry.go | 112 +++++++++++++++++++++++++++-- typed/registry_test.go | 73 ++++++++++++++++++- 6 files changed, 180 insertions(+), 15 deletions(-) diff --git a/adopt/adopt_test.go b/adopt/adopt_test.go index 9db32f5..48669d1 100644 --- a/adopt/adopt_test.go +++ b/adopt/adopt_test.go @@ -46,7 +46,7 @@ func TestSecretAdopterHandler(t *testing.T) { err error } - secretNotFound := func(name string) error { + secretNotFound := func(_ string) error { return apierrors.NewNotFound( corev1.SchemeGroupVersion.WithResource("secrets").GroupResource(), "test") diff --git a/component/ensure_component_test.go b/component/ensure_component_test.go index fb424cd..5f211d0 100644 --- a/component/ensure_component_test.go +++ b/component/ensure_component_test.go @@ -122,7 +122,7 @@ func TestEnsureServiceHandler(t *testing.T) { client := clientfake.NewSimpleDynamicClient(scheme, tt.existingServices...) informerFactory := dynamicinformer.NewDynamicSharedInformerFactory(client, 0) require.NoError(t, informerFactory.ForResource(serviceGVR).Informer().AddIndexers(map[string]cache.IndexFunc{ - ownerIndex: func(obj interface{}) ([]string, error) { + ownerIndex: func(_ interface{}) ([]string, error) { return []string{types.NamespacedName{Namespace: "test", Name: "owner"}.String()}, nil }, })) diff --git a/handler/handler.go b/handler/handler.go index 37d56f1..5225ec4 100644 --- a/handler/handler.go +++ b/handler/handler.go @@ -127,7 +127,7 @@ func (f Builder) Handler(id Key) Handler { } // NoopHandler is a handler that does nothing -var NoopHandler = NewHandler(ContextHandlerFunc(func(ctx context.Context) {}), NextKey) +var NoopHandler = NewHandler(ContextHandlerFunc(func(_ context.Context) {}), NextKey) // Key is used to identify a given Handler in a set of Handlers type Key string diff --git a/queue/controls_test.go b/queue/controls_test.go index 915e60e..ee1bba3 100644 --- a/queue/controls_test.go +++ b/queue/controls_test.go @@ -28,7 +28,7 @@ func ExampleNewOperations() { }, cancel) // typically called from a handler - handler.NewHandlerFromFunc(func(ctx context.Context) { + handler.NewHandlerFromFunc(func(_ context.Context) { // do some work operations.Done() }, "example").Handle(ctx) @@ -60,7 +60,7 @@ func ExampleNewQueueOperationsCtx() { }, cancel)) // queue controls are passed via context - handler.NewHandlerFromFunc(func(ctx context.Context) { + handler.NewHandlerFromFunc(func(_ context.Context) { // do some work CtxQueue.Done() }, "example").Handle(ctx) diff --git a/typed/registry.go b/typed/registry.go index 65a6542..b9515aa 100644 --- a/typed/registry.go +++ b/typed/registry.go @@ -94,13 +94,43 @@ func (r *Registry) NewFilteredDynamicSharedInformerFactory(key FactoryKey, clien } // ListerFor returns a typed Lister from a Registry +// Deprecated: Use MustListerForKey instead func ListerFor[K runtime.Object](r *Registry, key RegistryKey) *Lister[K] { - return NewLister[K](r.ListerFor(key)) + return MustListerForKey[K](r, key) +} + +// MustListerForKey returns a typed Lister from a Registry, or panics if the key is not found +func MustListerForKey[K runtime.Object](r *Registry, key RegistryKey) *Lister[K] { + return NewLister[K](r.MustListerForKey(key)) +} + +// ListerForKey returns a typed Lister from a Registry, or an error if the key is not found +func ListerForKey[K runtime.Object](r *Registry, key RegistryKey) (*Lister[K], error) { + lister, err := r.ListerForKey(key) + if err != nil { + return nil, err + } + return NewLister[K](lister), nil } // IndexerFor returns a typed Indexer from a Registry +// Deprecated: Use MustIndexerForKey instead func IndexerFor[K runtime.Object](r *Registry, key RegistryKey) *Indexer[K] { - return NewIndexer[K](r.InformerFor(key).GetIndexer()) + return MustIndexerForKey[K](r, key) +} + +// MustIndexerForKey returns a typed Indexer from a Registry, or panics if the key is not found +func MustIndexerForKey[K runtime.Object](r *Registry, key RegistryKey) *Indexer[K] { + return NewIndexer[K](r.MustIndexerForKey(key)) +} + +// IndexerForKey returns a typed Indexer from a Registry, or an error if the key is not found +func IndexerForKey[K runtime.Object](r *Registry, key RegistryKey) (*Indexer[K], error) { + indexer, err := r.IndexerForKey(key) + if err != nil { + return nil, err + } + return NewIndexer[K](indexer), nil } // Add adds a factory to the registry under the given FactoryKey @@ -124,27 +154,95 @@ func (r *Registry) Remove(key FactoryKey) { } // InformerFactoryFor returns GVR-specific InformerFactory from the Registry. +// Deprecated: use MustInformerFactoryForKey instead. func (r *Registry) InformerFactoryFor(key RegistryKey) informers.GenericInformer { + return r.MustInformerFactoryForKey(key) +} + +// MustInformerFactoryForKey returns GVR-specific InformerFactory from the Registry +// or panics if the key is not found. +func (r *Registry) MustInformerFactoryForKey(key RegistryKey) informers.GenericInformer { + informer, err := r.InformerFactoryForKey(key) + if err != nil { + panic(err) + } + return informer +} + +// InformerFactoryForKey returns GVR-specific InformerFactory from the Registry +// or returns an error if the key is not found. +func (r *Registry) InformerFactoryForKey(key RegistryKey) (informers.GenericInformer, error) { r.RLock() defer r.RUnlock() factory, ok := r.factories[key.FactoryKey] if !ok { - panic(fmt.Errorf("InformerFactoryFor called with unknown key %s", key)) + return nil, fmt.Errorf("InformerFactoryFor called with unknown key %s", key) } - return factory.ForResource(key.GroupVersionResource) + return factory.ForResource(key.GroupVersionResource), nil } // ListerFor returns the GVR-specific Lister from the Registry +// Deprecated: use MustListerForKey instead. func (r *Registry) ListerFor(key RegistryKey) cache.GenericLister { - return r.InformerFactoryFor(key).Lister() + return r.MustInformerFactoryForKey(key).Lister() +} + +// MustListerForKey returns the GVR-specific Lister from the Registry, or panics +// if the key is not found. +func (r *Registry) MustListerForKey(key RegistryKey) cache.GenericLister { + return r.MustInformerFactoryForKey(key).Lister() +} + +// ListerForKey returns the GVR-specific Lister from the Registry, or an error +// if the key is not found. +func (r *Registry) ListerForKey(key RegistryKey) (cache.GenericLister, error) { + factory, err := r.InformerFactoryForKey(key) + if err != nil { + return nil, err + } + return factory.Lister(), nil } // InformerFor returns the GVR-specific Informer from the Registry +// Deprecated: use MustInformerForKey instead. func (r *Registry) InformerFor(key RegistryKey) cache.SharedIndexInformer { - return r.InformerFactoryFor(key).Informer() + return r.MustInformerFactoryForKey(key).Informer() +} + +// MustInformerForKey returns the GVR-specific Informer from the Registry, or panics +// if the key is not found. +func (r *Registry) MustInformerForKey(key RegistryKey) cache.SharedIndexInformer { + return r.MustInformerFactoryForKey(key).Informer() +} + +// InformerForKey returns the GVR-specific Informer from the Registry, or an error +// if the key is not found. +func (r *Registry) InformerForKey(key RegistryKey) (cache.SharedIndexInformer, error) { + factory, err := r.InformerFactoryForKey(key) + if err != nil { + return nil, err + } + return factory.Informer(), nil } // IndexerFor returns the GVR-specific Indexer from the Registry +// Deprecated: use MustIndexerForKey instead. func (r *Registry) IndexerFor(key RegistryKey) cache.Indexer { - return r.InformerFactoryFor(key).Informer().GetIndexer() + return r.MustInformerForKey(key).GetIndexer() +} + +// MustIndexerForKey returns the GVR-specific Indexer from the Registry, or panics +// if the key is not found. +func (r *Registry) MustIndexerForKey(key RegistryKey) cache.Indexer { + return r.MustInformerForKey(key).GetIndexer() +} + +// IndexerForKey returns the GVR-specific Indexer from the Registry, or an error +// if the key is not found. +func (r *Registry) IndexerForKey(key RegistryKey) (cache.Indexer, error) { + informer, err := r.InformerForKey(key) + if err != nil { + return nil, err + } + return informer.GetIndexer(), nil } diff --git a/typed/registry_test.go b/typed/registry_test.go index 7a3c12a..e4ae298 100644 --- a/typed/registry_test.go +++ b/typed/registry_test.go @@ -5,6 +5,7 @@ import ( "fmt" "testing" + "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" @@ -92,7 +93,7 @@ func ExampleListerFor() { dependentSecretKey := NewRegistryKey(dependentObjectKey, secretGVR) - cachedSecret, _ := ListerFor[*corev1.Secret](registry, dependentSecretKey).ByNamespace("example").Get("mysecret") + cachedSecret, _ := MustListerForKey[*corev1.Secret](registry, dependentSecretKey).ByNamespace("example").Get("mysecret") fmt.Printf("%T %s/%s", cachedSecret, cachedSecret.GetNamespace(), cachedSecret.GetName()) // Output: *v1.Secret example/mysecret } @@ -131,7 +132,7 @@ func ExampleIndexerFor() { const indexName = "ExampleIndex" const constantIndexValue = "indexVal" if err := informerFactory.ForResource(secretGVR).Informer().AddIndexers(map[string]cache.IndexFunc{ - indexName: func(obj interface{}) ([]string, error) { + indexName: func(_ interface{}) ([]string, error) { return []string{constantIndexValue}, nil }, }); err != nil { @@ -143,7 +144,7 @@ func ExampleIndexerFor() { dependentSecretKey := NewRegistryKey(dependentObjectKey, secretGVR) - matchingCachedSecrets, _ := IndexerFor[*corev1.Secret](registry, dependentSecretKey).ByIndex(indexName, constantIndexValue) + matchingCachedSecrets, _ := MustIndexerForKey[*corev1.Secret](registry, dependentSecretKey).ByIndex(indexName, constantIndexValue) fmt.Printf("%T %s/%s", matchingCachedSecrets, matchingCachedSecrets[0].GetNamespace(), matchingCachedSecrets[0].GetName()) // Output: []*v1.Secret example/mysecret } @@ -176,3 +177,69 @@ func TestRemove(_ *testing.T) { registry.Remove(dependentObjectKey) } + +func TestForKey(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + registry := NewRegistry() + + secretGVR := corev1.SchemeGroupVersion.WithResource("secrets") + scheme := runtime.NewScheme() + if err := corev1.AddToScheme(scheme); err != nil { + panic(err) + } + client := fake.NewSimpleDynamicClient(scheme) + dependentObjectKey := NewFactoryKey("my-controller", "localCluster", "dependentObjects") + informerFactory := registry.MustNewFilteredDynamicSharedInformerFactory( + dependentObjectKey, + client, + 0, + metav1.NamespaceAll, + func(options *metav1.ListOptions) { + options.LabelSelector = "my-controller.com/related-to=myobjecttype" + }, + ) + informerFactory.ForResource(secretGVR) + informerFactory.Start(ctx.Done()) + informerFactory.WaitForCacheSync(ctx.Done()) + + // good key works + dependentSecretKey := NewRegistryKey(dependentObjectKey, secretGVR) + require.NotPanics(t, func() { + registry.MustInformerFactoryForKey(dependentSecretKey) + registry.MustListerForKey(dependentSecretKey) + registry.MustIndexerForKey(dependentSecretKey) + MustListerForKey[*corev1.Secret](registry, dependentSecretKey) + MustIndexerForKey[*corev1.Secret](registry, dependentSecretKey) + }) + + // bad key panics Must*ForKey + badKey := NewRegistryKey(NewFactoryKey("other-controller", "othercluster", "dependentObjects"), corev1.SchemeGroupVersion.WithResource("pods")) + require.Panics(t, func() { + registry.MustInformerFactoryForKey(badKey) + }) + require.Panics(t, func() { + registry.MustListerForKey(badKey) + }) + require.Panics(t, func() { + registry.MustIndexerForKey(badKey) + }) + require.Panics(t, func() { + MustListerForKey[*corev1.Pod](registry, badKey) + }) + require.Panics(t, func() { + MustIndexerForKey[*corev1.Pod](registry, badKey) + }) + + // bad key returns error for *ForKey + _, err := registry.InformerFactoryForKey(badKey) + require.Error(t, err) + _, err = registry.ListerForKey(badKey) + require.Error(t, err) + _, err = registry.IndexerForKey(badKey) + require.Error(t, err) + _, err = ListerForKey[*corev1.Pod](registry, badKey) + require.Error(t, err) + _, err = IndexerForKey[*corev1.Pod](registry, badKey) + require.Error(t, err) +} From 596172c30b3c01163322c51f8935afb36cbcdedc Mon Sep 17 00:00:00 2001 From: Evan Cordell Date: Wed, 1 May 2024 09:13:43 -0400 Subject: [PATCH 2/2] fix linter errors --- adopt/adopt.go | 4 ++-- adopt/adopt_test.go | 8 ++++---- bootstrap/crds.go | 2 +- component/ensure_component_test.go | 8 ++++---- manager/controller_test.go | 7 ++++--- pause/pause_test.go | 6 +++--- static/controller.go | 6 +++--- 7 files changed, 21 insertions(+), 20 deletions(-) diff --git a/adopt/adopt.go b/adopt/adopt.go index d2227aa..228a055 100644 --- a/adopt/adopt.go +++ b/adopt/adopt.go @@ -86,11 +86,11 @@ const Owned = "owned" var ( // AlwaysExistsFunc is an ExistsFunc that always returns nil - AlwaysExistsFunc ExistsFunc = func(ctx context.Context, nn types.NamespacedName) error { + AlwaysExistsFunc ExistsFunc = func(_ context.Context, _ types.NamespacedName) error { return nil } // NoopObjectMissingFunc is an ObjectMissing func that does nothing - NoopObjectMissingFunc = func(ctx context.Context, err error) {} + NoopObjectMissingFunc = func(_ context.Context, _ error) {} ) // AdoptionHandler implements handler.Handler to "adopt" an existing resource diff --git a/adopt/adopt_test.go b/adopt/adopt_test.go index 48669d1..6252029 100644 --- a/adopt/adopt_test.go +++ b/adopt/adopt_test.go @@ -324,21 +324,21 @@ func TestSecretAdopterHandler(t *testing.T) { applyCallIndex := 0 s := NewSecretAdoptionHandler( recorder, - func(ctx context.Context) (*corev1.Secret, error) { + func(_ context.Context) (*corev1.Secret, error) { return tt.secretInCache, tt.cacheErr }, - func(ctx context.Context, err error) { + func(_ context.Context, err error) { require.Equal(t, tt.expectObjectMissingErr, err) }, typed.NewIndexer[*corev1.Secret](indexer), - func(ctx context.Context, secret *applycorev1.SecretApplyConfiguration, opts metav1.ApplyOptions) (result *corev1.Secret, err error) { + func(_ context.Context, secret *applycorev1.SecretApplyConfiguration, _ metav1.ApplyOptions) (result *corev1.Secret, err error) { defer func() { applyCallIndex++ }() call := tt.applyCalls[applyCallIndex] call.called = true require.Equal(t, call.input, secret, "error on call %d", applyCallIndex) return call.result, call.err }, - func(ctx context.Context, nn types.NamespacedName) error { + func(_ context.Context, _ types.NamespacedName) error { return tt.secretExistsErr }, handler.NewHandlerFromFunc(func(ctx context.Context) { diff --git a/bootstrap/crds.go b/bootstrap/crds.go index f3380da..eb84a31 100644 --- a/bootstrap/crds.go +++ b/bootstrap/crds.go @@ -117,7 +117,7 @@ func waitForDiscovery(ctx context.Context, config *rest.Config, crds []*apiexten return err } - return wait.PollUntilContextTimeout(ctx, crdInstallPollInterval, maxCRDInstallTime, true, func(ctx context.Context) (done bool, err error) { + return wait.PollUntilContextTimeout(ctx, crdInstallPollInterval, maxCRDInstallTime, true, func(_ context.Context) (done bool, err error) { _, serverGVRs, err := discoveryClient.ServerGroupsAndResources() if err != nil { return false, nil diff --git a/component/ensure_component_test.go b/component/ensure_component_test.go index 5f211d0..f1bbcc7 100644 --- a/component/ensure_component_test.go +++ b/component/ensure_component_test.go @@ -137,7 +137,7 @@ func TestEnsureServiceHandler(t *testing.T) { NewIndexedComponent( indexer, ownerIndex, - func(ctx context.Context) labels.Selector { + func(_ context.Context) labels.Selector { return labels.SelectorFromSet(map[string]string{ "example.com/component": "the-main-service-component", }) @@ -145,15 +145,15 @@ func TestEnsureServiceHandler(t *testing.T) { hash.NewObjectHash(), hashKey), ctxOwner, queueOps, - func(ctx context.Context, apply *applycorev1.ServiceApplyConfiguration) (*corev1.Service, error) { + func(_ context.Context, _ *applycorev1.ServiceApplyConfiguration) (*corev1.Service, error) { applyCalled = true return nil, nil }, - func(ctx context.Context, nn types.NamespacedName) error { + func(_ context.Context, _ types.NamespacedName) error { deleteCalled = true return nil }, - func(ctx context.Context) *applycorev1.ServiceApplyConfiguration { + func(_ context.Context) *applycorev1.ServiceApplyConfiguration { return applycorev1.Service("test", "test"). WithLabels(map[string]string{ "example.com/component": "the-main-service-component", diff --git a/manager/controller_test.go b/manager/controller_test.go index 123a768..12a687f 100644 --- a/manager/controller_test.go +++ b/manager/controller_test.go @@ -32,8 +32,8 @@ func ExampleNewOwnedResourceController() { // the controller processes objects on the queue, but doesn't set up any // informers by default. - controller := NewOwnedResourceController(klogr.New(), "my-controller", gvr, CtxQueue, registry, broadcaster, func(ctx context.Context, gvr schema.GroupVersionResource, namespace, name string) { - // process object + controller := NewOwnedResourceController(klogr.New(), "my-controller", gvr, CtxQueue, registry, broadcaster, func(_ context.Context, gvr schema.GroupVersionResource, namespace, name string) { + fmt.Println("processing", gvr, namespace, name) }) mgr := NewManager(ctrlmanageropts.RecommendedDebuggingOptions().DebuggingConfiguration, ":", broadcaster, eventSink) @@ -54,7 +54,8 @@ func TestControllerQueueDone(t *testing.T) { broadcaster := record.NewBroadcaster() eventSink := &typedcorev1.EventSinkImpl{Interface: fake.NewSimpleClientset().CoreV1().Events("")} - controller := NewOwnedResourceController(klogr.New(), "my-controller", gvr, CtxQueue, registry, broadcaster, func(ctx context.Context, gvr schema.GroupVersionResource, namespace, name string) { + controller := NewOwnedResourceController(klogr.New(), "my-controller", gvr, CtxQueue, registry, broadcaster, func(_ context.Context, gvr schema.GroupVersionResource, namespace, name string) { + fmt.Println("processing", gvr, namespace, name) }) mgr := NewManager(ctrlmanageropts.RecommendedDebuggingOptions().DebuggingConfiguration, ":", broadcaster, eventSink) diff --git a/pause/pause_test.go b/pause/pause_test.go index 69a93f3..6577c0b 100644 --- a/pause/pause_test.go +++ b/pause/pause_test.go @@ -44,7 +44,7 @@ func ExampleNewPauseContextHandler() { queueOperations.Key, "example.com/paused", ctxObject, - func(ctx context.Context, patch *MyObject) error { + func(_ context.Context, _ *MyObject) error { // update status return nil }, @@ -184,7 +184,7 @@ func TestPauseHandler(t *testing.T) { ctrls := &fake.FakeInterface{} patchCalled := false - patchStatus := func(ctx context.Context, patch *MyObject) error { + patchStatus := func(_ context.Context, patch *MyObject) error { patchCalled = true if tt.patchError != nil { @@ -209,7 +209,7 @@ func TestPauseHandler(t *testing.T) { ctx = ctxMyObject.WithValue(ctx, tt.obj) var called handler.Key - NewPauseContextHandler(queueOps.Key, PauseLabelKey, ctxMyObject, patchStatus, handler.ContextHandlerFunc(func(ctx context.Context) { + NewPauseContextHandler(queueOps.Key, PauseLabelKey, ctxMyObject, patchStatus, handler.ContextHandlerFunc(func(_ context.Context) { called = nextKey })).Handle(ctx) diff --git a/static/controller.go b/static/controller.go index 5f1ef3e..9f12288 100644 --- a/static/controller.go +++ b/static/controller.go @@ -49,9 +49,9 @@ func NewStaticController[K bootstrap.KubeResourceObject](log logr.Logger, name s func (c *Controller[K]) Start(ctx context.Context, _ int) { inf := c.fileInformerFactory.ForResource(c.staticClusterResource).Informer() _, err := inf.AddEventHandler(cache.ResourceEventHandlerFuncs{ - AddFunc: func(obj interface{}) { c.handleStaticResource(ctx) }, - UpdateFunc: func(_, obj interface{}) { c.handleStaticResource(ctx) }, - DeleteFunc: func(obj interface{}) { c.handleStaticResource(ctx) }, + AddFunc: func(_ any) { c.handleStaticResource(ctx) }, + UpdateFunc: func(_, _ any) { c.handleStaticResource(ctx) }, + DeleteFunc: func(_ any) { c.handleStaticResource(ctx) }, }) if err != nil { panic("failed to add handlers: " + err.Error())