🐛 Remove cache when catalog is deleted (#1207)

* Add `Remove` into `filesystemCache`

Enables us to deletes cache directory
for a given catalog from the filesystem.

Signed-off-by: Mikalai Radchuk <[email protected]>

* Add a finaliser to ClusterCatalog

New finaliser allows us to remove catalog cache from filesystem
on catalog deletion.

Signed-off-by: Mikalai Radchuk <[email protected]>

---------

Signed-off-by: Mikalai Radchuk <[email protected]>

cmd/manager/main.go

@@ -219,9 +219,10 @@ func main() {
 		setupLog.Error(err, "unable to create catalogs cache directory")
 		os.Exit(1)
 	}
-	catalogClient := catalogclient.New(cache.NewFilesystemCache(catalogsCachePath, func() (*http.Client, error) {
+	cacheFetcher := cache.NewFilesystemCache(catalogsCachePath, func() (*http.Client, error) {
 		return httputil.BuildHTTPClient(certPoolWatcher)
-	}))
+	})
+	catalogClient := catalogclient.New(cacheFetcher)
 
 	resolver := &resolve.CatalogResolver{
 		WalkCatalogsFunc: resolve.CatalogWalker(
@@ -277,6 +278,15 @@ func main() {
 		setupLog.Error(err, "unable to create controller", "controller", "ClusterExtension")
 		os.Exit(1)
 	}
+
+	if err = (&controllers.ClusterCatalogReconciler{
+		Client: cl,
+		Cache:  cacheFetcher,
+	}).SetupWithManager(mgr); err != nil {
+		setupLog.Error(err, "unable to create controller", "controller", "ClusterCatalog")
+		os.Exit(1)
+	}
+
 	//+kubebuilder:scaffold:builder
 
 	if err := mgr.AddHealthzCheck("healthz", healthz.Ping); err != nil {

config/base/rbac/role.yaml

@@ -21,6 +21,7 @@ rules:
   resources:
   - clustercatalogs
   verbs:
+  - get
   - list
   - watch
 - apiGroups:

internal/catalogmetadata/cache/cache.go

@@ -25,7 +25,7 @@ var _ client.Fetcher = &filesystemCache{}
 //   - IF cached it will verify the cache is up to date. If it is up to date it will return
 //     the cached contents, if not it will fetch the new contents from the catalogd HTTP
 //     server and update the cached contents.
-func NewFilesystemCache(cachePath string, clientFunc func() (*http.Client, error)) client.Fetcher {
+func NewFilesystemCache(cachePath string, clientFunc func() (*http.Client, error)) *filesystemCache {
 	return &filesystemCache{
 		cachePath:              cachePath,
 		mutex:                  sync.RWMutex{},
@@ -80,7 +80,7 @@ func (fsc *filesystemCache) FetchCatalogContents(ctx context.Context, catalog *c
 		return nil, fmt.Errorf("error: catalog %q has a nil status.resolvedSource.image value", catalog.Name)
 	}
 
-	cacheDir := filepath.Join(fsc.cachePath, catalog.Name)
+	cacheDir := fsc.cacheDir(catalog.Name)
 	fsc.mutex.RLock()
 	if data, ok := fsc.cacheDataByCatalogName[catalog.Name]; ok {
 		if catalog.Status.ResolvedSource.Image.ResolvedRef == data.ResolvedRef {
@@ -166,3 +166,26 @@ func (fsc *filesystemCache) FetchCatalogContents(ctx context.Context, catalog *c
 
 	return os.DirFS(cacheDir), nil
 }
+
+// Remove deletes cache directory for a given catalog from the filesystem
+func (fsc *filesystemCache) Remove(catalogName string) error {
+	cacheDir := fsc.cacheDir(catalogName)
+
+	fsc.mutex.Lock()
+	defer fsc.mutex.Unlock()
+
+	if _, exists := fsc.cacheDataByCatalogName[catalogName]; !exists {
+		return nil
+	}
+
+	if err := os.RemoveAll(cacheDir); err != nil {
+		return fmt.Errorf("error removing cache directory: %v", err)
+	}
+
+	delete(fsc.cacheDataByCatalogName, catalogName)
+	return nil
+}
+
+func (fsc *filesystemCache) cacheDir(catalogName string) string {
+	return filepath.Join(fsc.cachePath, catalogName)
+}

internal/catalogmetadata/cache/cache_test.go

@@ -10,12 +10,14 @@ import (
 	"io/fs"
 	"maps"
 	"net/http"
+	"os"
 	"path/filepath"
 	"testing"
 	"testing/fstest"
 
 	"github.com/google/go-cmp/cmp"
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/sets"
 
@@ -62,7 +64,7 @@ var defaultFS = fstest.MapFS{
 	"fake1/olm.channel/stable.json":      &fstest.MapFile{Data: []byte(stableChannel)},
 }
 
-func TestFilesystemCache(t *testing.T) {
+func TestFilesystemCacheFetchCatalogContents(t *testing.T) {
 	type test struct {
 		name           string
 		catalog        *catalogd.ClusterCatalog
@@ -245,6 +247,64 @@ func TestFilesystemCache(t *testing.T) {
 	}
 }
 
+func TestFilesystemCacheRemove(t *testing.T) {
+	testCatalog := &catalogd.ClusterCatalog{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "test-catalog",
+		},
+		Status: catalogd.ClusterCatalogStatus{
+			ResolvedSource: &catalogd.ResolvedCatalogSource{
+				Type: catalogd.SourceTypeImage,
+				Image: &catalogd.ResolvedImageSource{
+					ResolvedRef: "fake/catalog@sha256:fakesha",
+				},
+			},
+		},
+	}
+
+	ctx := context.Background()
+	cacheDir := t.TempDir()
+
+	tripper := &mockTripper{}
+	tripper.content = make(fstest.MapFS)
+	maps.Copy(tripper.content, defaultFS)
+	httpClient := &http.Client{
+		Transport: tripper,
+	}
+	c := cache.NewFilesystemCache(cacheDir, func() (*http.Client, error) {
+		return httpClient, nil
+	})
+
+	catalogCachePath := filepath.Join(cacheDir, testCatalog.Name)
+
+	t.Log("Remove cache before it exists")
+	require.NoDirExists(t, catalogCachePath)
+	err := c.Remove(testCatalog.Name)
+	require.NoError(t, err)
+	assert.NoDirExists(t, catalogCachePath)
+
+	t.Log("Fetch contents to populate cache")
+	_, err = c.FetchCatalogContents(ctx, testCatalog)
+	require.NoError(t, err)
+	require.DirExists(t, catalogCachePath)
+
+	t.Log("Temporary change permissions to the cache dir to cause error")
+	require.NoError(t, os.Chmod(catalogCachePath, 0000))
+
+	t.Log("Remove cache causes an error")
+	err = c.Remove(testCatalog.Name)
+	require.ErrorContains(t, err, "error removing cache directory")
+	require.DirExists(t, catalogCachePath)
+
+	t.Log("Restore directory permissions for successful removal")
+	require.NoError(t, os.Chmod(catalogCachePath, 0777))
+
+	t.Log("Remove cache")
+	err = c.Remove(testCatalog.Name)
+	require.NoError(t, err)
+	assert.NoDirExists(t, catalogCachePath)
+}
+
 var _ http.RoundTripper = &mockTripper{}
 
 type mockTripper struct {

internal/controllers/clustercatalog_controller.go

@@ -0,0 +1,76 @@
+/*
+Copyright 2024.
+
+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 controllers
+
+import (
+	"context"
+
+	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 {
+	Remove(catalogName string) error
+}
+
+// ClusterCatalogReconciler reconciles a ClusterCatalog object
+type ClusterCatalogReconciler struct {
+	client.Client
+	Cache CatalogCacheRemover
+}
+
+//+kubebuilder:rbac:groups=olm.operatorframework.io,resources=clustercatalogs,verbs=get;list;watch
+
+func (r *ClusterCatalogReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	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 != nil {
+		return ctrl.Result{}, 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
+			},
+		})).
+		Build(r)
+
+	return err
+}

internal/controllers/clustercatalog_controller_test.go

@@ -0,0 +1,95 @@
+package controllers_test
+
+import (
+	"context"
+	"errors"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+
+	catalogd "github.com/operator-framework/catalogd/api/core/v1alpha1"
+
+	"github.com/operator-framework/operator-controller/internal/controllers"
+	"github.com/operator-framework/operator-controller/internal/scheme"
+)
+
+func TestClusterCatalogReconcilerFinalizers(t *testing.T) {
+	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: "catalog exists",
+			catalog: &catalogd.ClusterCatalog{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: catalogKey.Name,
+				},
+			},
+		},
+		{
+			name: "catalog does not exist",
+			cacheRemoveFunc: func(catalogName string) error {
+				assert.Equal(t, catalogKey.Name, catalogName)
+				return nil
+			},
+			wantCacheRemoveCalled: true,
+		},
+		{
+			name: "catalog does not exist - error on removal",
+			cacheRemoveFunc: func(catalogName string) error {
+				return errors.New("fake error from remove")
+			},
+			wantCacheRemoveCalled: true,
+			wantErr:               "fake error from remove",
+		},
+	} {
+		t.Run(tt.name, func(t *testing.T) {
+			ctx := context.Background()
+
+			clientBuilder := fake.NewClientBuilder().WithScheme(scheme.Scheme)
+			if tt.catalog != nil {
+				clientBuilder = clientBuilder.WithObjects(tt.catalog)
+			}
+			cl := clientBuilder.Build()
+
+			cacheRemover := &mockCatalogCacheRemover{
+				removeFunc: tt.cacheRemoveFunc,
+			}
+
+			reconciler := &controllers.ClusterCatalogReconciler{
+				Client: cl,
+				Cache:  cacheRemover,
+			}
+
+			result, err := reconciler.Reconcile(ctx, ctrl.Request{NamespacedName: catalogKey})
+			if tt.wantErr == "" {
+				require.NoError(t, err)
+			} else {
+				require.ErrorContains(t, err, tt.wantErr)
+			}
+			require.Equal(t, ctrl.Result{}, result)
+
+			assert.Equal(t, tt.wantCacheRemoveCalled, cacheRemover.called)
+		})
+	}
+}
+
+type mockCatalogCacheRemover struct {
+	called     bool
+	removeFunc func(catalogName string) error
+}
+
+func (m *mockCatalogCacheRemover) Remove(catalogName string) error {
+	m.called = true
+	return m.removeFunc(catalogName)
+}