diff --git a/cmd/manager/main.go b/cmd/manager/main.go index b03472dfc..c353a4cb0 100644 --- a/cmd/manager/main.go +++ b/cmd/manager/main.go @@ -314,8 +314,9 @@ func main() { } if err = (&controllers.ClusterCatalogReconciler{ - Client: cl, - Cache: catalogClientBackend, + Client: cl, + CatalogCache: catalogClientBackend, + CatalogCachePopulator: catalogClient, }).SetupWithManager(mgr); err != nil { setupLog.Error(err, "unable to create controller", "controller", "ClusterCatalog") os.Exit(1) diff --git a/internal/catalogmetadata/client/client.go b/internal/catalogmetadata/client/client.go index fee3b1cc2..a68c6c989 100644 --- a/internal/catalogmetadata/client/client.go +++ b/internal/catalogmetadata/client/client.go @@ -65,17 +65,10 @@ func (c *Client) GetPackage(ctx context.Context, catalog *catalogd.ClusterCatalo catalogFsys, err := c.cache.Get(catalog.Name, catalog.Status.ResolvedSource.Image.Ref) if err != nil { - return nil, fmt.Errorf("error retrieving catalog cache: %v", err) + return nil, fmt.Errorf("error retrieving cache for catalog %q: %v", catalog.Name, err) } if catalogFsys == nil { - // TODO: https://github.com/operator-framework/operator-controller/pull/1284 - // For now we are still populating cache (if absent) on-demand, - // but we might end up just returning a "cache not found" error here - // once we implement cache population in the controller. - catalogFsys, err = c.PopulateCache(ctx, catalog) - if err != nil { - return nil, fmt.Errorf("error fetching catalog contents: %v", err) - } + return nil, fmt.Errorf("cache for catalog %q not found", catalog.Name) } pkgFsys, err := fs.Sub(catalogFsys, pkgName) diff --git a/internal/catalogmetadata/client/client_test.go b/internal/catalogmetadata/client/client_test.go index 33f43c1cf..15430d7c1 100644 --- a/internal/catalogmetadata/client/client_test.go +++ b/internal/catalogmetadata/client/client_test.go @@ -62,7 +62,7 @@ func TestClientGetPackage(t *testing.T) { catalog: defaultCatalog, cache: &fakeCache{getErr: errors.New("fetch error")}, assert: func(t *testing.T, dc *declcfg.DeclarativeConfig, err error) { - assert.ErrorContains(t, err, `error retrieving catalog cache`) + assert.ErrorContains(t, err, `error retrieving cache for catalog "catalog-1"`) }, }, { @@ -114,18 +114,7 @@ func TestClientGetPackage(t *testing.T) { return testFS, nil }}, assert: func(t *testing.T, fbc *declcfg.DeclarativeConfig, err error) { - require.NoError(t, err) - assert.Equal(t, &declcfg.DeclarativeConfig{Packages: []declcfg.Package{{Schema: declcfg.SchemaPackage, Name: "pkg-present"}}}, fbc) - }, - }, - { - name: "cache unpopulated and fails to populate", - catalog: defaultCatalog, - pkgName: "pkg-present", - cache: &fakeCache{putErr: errors.New("fake cache put error")}, - assert: func(t *testing.T, fbc *declcfg.DeclarativeConfig, err error) { - assert.Nil(t, fbc) - assert.ErrorContains(t, err, "error fetching catalog contents") + assert.ErrorContains(t, err, `cache for catalog "catalog-1" not found`) }, }, } { @@ -278,7 +267,6 @@ type fakeCache struct { getErr error putFunc func(source string, errToCache error) (fs.FS, error) - putErr error } func (c *fakeCache) Get(catalogName, resolvedRef string) (fs.FS, error) { @@ -293,9 +281,6 @@ func (c *fakeCache) Put(catalogName, resolvedRef string, source io.Reader, errTo } return c.putFunc(buf.String(), errToCache) } - if c.putErr != nil { - return nil, c.putErr - } return nil, errors.New("unexpected error") } diff --git a/internal/controllers/clustercatalog_controller.go b/internal/controllers/clustercatalog_controller.go index 0f7a26a6c..dc86ed59f 100644 --- a/internal/controllers/clustercatalog_controller.go +++ b/internal/controllers/clustercatalog_controller.go @@ -18,25 +18,30 @@ package controllers import ( "context" + "fmt" + "io/fs" apierrors "k8s.io/apimachinery/pkg/api/errors" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/predicate" catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1" ) -type CatalogCacheRemover interface { +type CatalogCache interface { + Get(catalogName, resolvedRef string) (fs.FS, error) Remove(catalogName string) error } +type CatalogCachePopulator interface { + PopulateCache(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) +} + // ClusterCatalogReconciler reconciles a ClusterCatalog object type ClusterCatalogReconciler struct { client.Client - Cache CatalogCacheRemover + CatalogCache CatalogCache + CatalogCachePopulator CatalogCachePopulator } //+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clustercatalogs,verbs=get;list;watch @@ -45,31 +50,44 @@ func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Reque existingCatalog := &catalogd.ClusterCatalog{} err := r.Client.Get(ctx, req.NamespacedName, existingCatalog) if apierrors.IsNotFound(err) { - return ctrl.Result{}, r.Cache.Remove(req.Name) + if err := r.CatalogCache.Remove(req.Name); err != nil { + return ctrl.Result{}, fmt.Errorf("error removing cache for catalog %q: %v", req.Name, err) + } + return ctrl.Result{}, nil } if err != nil { return ctrl.Result{}, err } + + if existingCatalog.Status.ResolvedSource == nil || + existingCatalog.Status.ResolvedSource.Image == nil || + existingCatalog.Status.ResolvedSource.Image.Ref == "" { + // Reference is not known yet - skip cache population with no error. + // Once the reference is resolved another reconcile cycle + // will be triggered and we will progress further. + return ctrl.Result{}, nil + } + + catalogFsys, err := r.CatalogCache.Get(existingCatalog.Name, existingCatalog.Status.ResolvedSource.Image.Ref) + if err != nil { + return ctrl.Result{}, fmt.Errorf("error retrieving cache for catalog %q: %v", existingCatalog.Name, err) + } + if catalogFsys != nil { + // Cache already exists so we do not need to populate it + return ctrl.Result{}, nil + } + + if _, err = r.CatalogCachePopulator.PopulateCache(ctx, existingCatalog); err != nil { + return ctrl.Result{}, fmt.Errorf("error populating cache for catalog %q: %v", existingCatalog.Name, err) + } + return ctrl.Result{}, nil } // SetupWithManager sets up the controller with the Manager. func (r *ClusterCatalogReconciler) SetupWithManager(mgr ctrl.Manager) error { _, err := ctrl.NewControllerManagedBy(mgr). - For(&catalogd.ClusterCatalog{}, builder.WithPredicates(predicate.Funcs{ - CreateFunc: func(e event.CreateEvent) bool { - return false - }, - UpdateFunc: func(e event.UpdateEvent) bool { - return false - }, - DeleteFunc: func(e event.DeleteEvent) bool { - return true - }, - GenericFunc: func(e event.GenericEvent) bool { - return false - }, - })). + For(&catalogd.ClusterCatalog{}). Build(r) return err diff --git a/internal/controllers/clustercatalog_controller_test.go b/internal/controllers/clustercatalog_controller_test.go index 762fa15ec..639f20c96 100644 --- a/internal/controllers/clustercatalog_controller_test.go +++ b/internal/controllers/clustercatalog_controller_test.go @@ -3,7 +3,9 @@ package controllers_test import ( "context" "errors" + "io/fs" "testing" + "testing/fstest" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -19,38 +21,142 @@ import ( ) func TestClusterCatalogReconcilerFinalizers(t *testing.T) { + const fakeResolvedRef = "fake/catalog@sha256:fakesha1" catalogKey := types.NamespacedName{Name: "test-catalog"} for _, tt := range []struct { - name string - catalog *catalogd.ClusterCatalog - cacheRemoveFunc func(catalogName string) error - wantCacheRemoveCalled bool - wantErr string + name string + catalog *catalogd.ClusterCatalog + catalogCache mockCatalogCache + catalogCachePopulator mockCatalogCachePopulator + wantGetCacheCalled bool + wantRemoveCacheCalled bool + wantPopulateCacheCalled bool + wantErr string }{ { - name: "catalog exists", + name: "catalog exists - cache unpopulated", catalog: &catalogd.ClusterCatalog{ ObjectMeta: metav1.ObjectMeta{ Name: catalogKey.Name, }, + Status: catalogd.ClusterCatalogStatus{ + ResolvedSource: &catalogd.ResolvedCatalogSource{ + Image: &catalogd.ResolvedImageSource{ + Ref: fakeResolvedRef, + }, + }, + }, + }, + catalogCachePopulator: mockCatalogCachePopulator{ + populateCacheFunc: func(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) { + assert.Equal(t, catalogKey.Name, catalog.Name) + return nil, nil + }, + }, + wantGetCacheCalled: true, + wantPopulateCacheCalled: true, + }, + { + name: "catalog exists - cache already populated", + catalog: &catalogd.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: catalogKey.Name, + }, + Status: catalogd.ClusterCatalogStatus{ + ResolvedSource: &catalogd.ResolvedCatalogSource{ + Image: &catalogd.ResolvedImageSource{ + Ref: fakeResolvedRef, + }, + }, + }, + }, + catalogCache: mockCatalogCache{ + getFunc: func(catalogName, resolvedRef string) (fs.FS, error) { + assert.Equal(t, catalogKey.Name, catalogName) + assert.Equal(t, fakeResolvedRef, resolvedRef) + // Just any non-nil fs.FS to simulate existence of cache + return fstest.MapFS{}, nil + }, + }, + wantGetCacheCalled: true, + }, + { + name: "catalog exists - catalog not yet resolved", + catalog: &catalogd.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: catalogKey.Name, + }, + }, + }, + { + name: "catalog exists - error on cache population", + catalog: &catalogd.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: catalogKey.Name, + }, + Status: catalogd.ClusterCatalogStatus{ + ResolvedSource: &catalogd.ResolvedCatalogSource{ + Image: &catalogd.ResolvedImageSource{ + Ref: fakeResolvedRef, + }, + }, + }, }, + catalogCachePopulator: mockCatalogCachePopulator{ + populateCacheFunc: func(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) { + assert.Equal(t, catalogKey.Name, catalog.Name) + return nil, errors.New("fake error from populate cache function") + }, + }, + wantGetCacheCalled: true, + wantPopulateCacheCalled: true, + wantErr: "error populating cache for catalog", + }, + { + name: "catalog exists - error on cache get", + catalog: &catalogd.ClusterCatalog{ + ObjectMeta: metav1.ObjectMeta{ + Name: catalogKey.Name, + }, + Status: catalogd.ClusterCatalogStatus{ + ResolvedSource: &catalogd.ResolvedCatalogSource{ + Image: &catalogd.ResolvedImageSource{ + Ref: fakeResolvedRef, + }, + }, + }, + }, + catalogCache: mockCatalogCache{ + getFunc: func(catalogName, resolvedRef string) (fs.FS, error) { + assert.Equal(t, catalogKey.Name, catalogName) + assert.Equal(t, fakeResolvedRef, resolvedRef) + return nil, errors.New("fake error from cache get function") + }, + }, + wantGetCacheCalled: true, + wantErr: "error retrieving cache for catalog", }, { name: "catalog does not exist", - cacheRemoveFunc: func(catalogName string) error { - assert.Equal(t, catalogKey.Name, catalogName) - return nil + catalogCache: mockCatalogCache{ + removeFunc: func(catalogName string) error { + assert.Equal(t, catalogKey.Name, catalogName) + return nil + }, }, - wantCacheRemoveCalled: true, + wantRemoveCacheCalled: true, }, { name: "catalog does not exist - error on removal", - cacheRemoveFunc: func(catalogName string) error { - return errors.New("fake error from remove") + catalogCache: mockCatalogCache{ + removeFunc: func(catalogName string) error { + assert.Equal(t, catalogKey.Name, catalogName) + return errors.New("fake error from remove") + }, }, - wantCacheRemoveCalled: true, - wantErr: "fake error from remove", + wantRemoveCacheCalled: true, + wantErr: "error removing cache for catalog", }, } { t.Run(tt.name, func(t *testing.T) { @@ -62,13 +168,10 @@ func TestClusterCatalogReconcilerFinalizers(t *testing.T) { } cl := clientBuilder.Build() - cacheRemover := &mockCatalogCacheRemover{ - removeFunc: tt.cacheRemoveFunc, - } - reconciler := &controllers.ClusterCatalogReconciler{ - Client: cl, - Cache: cacheRemover, + Client: cl, + CatalogCache: controllers.CatalogCache(&tt.catalogCache), + CatalogCachePopulator: controllers.CatalogCachePopulator(&tt.catalogCachePopulator), } result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey}) @@ -79,17 +182,48 @@ func TestClusterCatalogReconcilerFinalizers(t *testing.T) { } require.Equal(t, ctrl.Result{}, result) - assert.Equal(t, tt.wantCacheRemoveCalled, cacheRemover.called) + assert.Equal(t, tt.wantRemoveCacheCalled, tt.catalogCache.removeFuncCalled) + assert.Equal(t, tt.wantGetCacheCalled, tt.catalogCache.getFuncCalled) + assert.Equal(t, tt.wantPopulateCacheCalled, tt.catalogCachePopulator.populateCacheCalled) }) } } -type mockCatalogCacheRemover struct { - called bool - removeFunc func(catalogName string) error +type mockCatalogCache struct { + removeFuncCalled bool + removeFunc func(catalogName string) error + getFuncCalled bool + getFunc func(catalogName, resolvedRef string) (fs.FS, error) +} + +func (m *mockCatalogCache) Remove(catalogName string) error { + m.removeFuncCalled = true + if m.removeFunc != nil { + return m.removeFunc(catalogName) + } + + return nil +} + +func (m *mockCatalogCache) Get(catalogName, resolvedRef string) (fs.FS, error) { + m.getFuncCalled = true + if m.getFunc != nil { + return m.getFunc(catalogName, resolvedRef) + } + + return nil, nil } -func (m *mockCatalogCacheRemover) Remove(catalogName string) error { - m.called = true - return m.removeFunc(catalogName) +type mockCatalogCachePopulator struct { + populateCacheCalled bool + populateCacheFunc func(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) +} + +func (m *mockCatalogCachePopulator) PopulateCache(ctx context.Context, catalog *catalogd.ClusterCatalog) (fs.FS, error) { + m.populateCacheCalled = true + if m.populateCacheFunc != nil { + return m.populateCacheFunc(ctx, catalog) + } + + return nil, nil }