From 134386ddd08ff936ac1fa890cfc1eab9d5874955 Mon Sep 17 00:00:00 2001 From: Mikalai Radchuk <509198+m1kola@users.noreply.github.com> Date: Fri, 20 Sep 2024 14:59:56 +0200 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Remove=20cache=20when=20catalog?= =?UTF-8?q?=20is=20deleted=20(#1207)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Add `Remove` into `filesystemCache` Enables us to deletes cache directory for a given catalog from the filesystem. Signed-off-by: Mikalai Radchuk * Add a finaliser to ClusterCatalog New finaliser allows us to remove catalog cache from filesystem on catalog deletion. Signed-off-by: Mikalai Radchuk --------- Signed-off-by: Mikalai Radchuk --- cmd/manager/main.go | 14 ++- config/base/rbac/role.yaml | 1 + internal/catalogmetadata/cache/cache.go | 27 +++++- internal/catalogmetadata/cache/cache_test.go | 62 +++++++++++- .../controllers/clustercatalog_controller.go | 76 +++++++++++++++ .../clustercatalog_controller_test.go | 95 +++++++++++++++++++ 6 files changed, 270 insertions(+), 5 deletions(-) create mode 100644 internal/controllers/clustercatalog_controller.go create mode 100644 internal/controllers/clustercatalog_controller_test.go diff --git a/cmd/manager/main.go b/cmd/manager/main.go index db25c3ad0..55951ca83 100644 --- a/cmd/manager/main.go +++ b/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 { diff --git a/config/base/rbac/role.yaml b/config/base/rbac/role.yaml index 38d394780..ee0a59833 100644 --- a/config/base/rbac/role.yaml +++ b/config/base/rbac/role.yaml @@ -21,6 +21,7 @@ rules: resources: - clustercatalogs verbs: + - get - list - watch - apiGroups: diff --git a/internal/catalogmetadata/cache/cache.go b/internal/catalogmetadata/cache/cache.go index f5c8a52eb..e986c9714 100644 --- a/internal/catalogmetadata/cache/cache.go +++ b/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) +} diff --git a/internal/catalogmetadata/cache/cache_test.go b/internal/catalogmetadata/cache/cache_test.go index 51b554721..74f7d79c0 100644 --- a/internal/catalogmetadata/cache/cache_test.go +++ b/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 { diff --git a/internal/controllers/clustercatalog_controller.go b/internal/controllers/clustercatalog_controller.go new file mode 100644 index 000000000..0f7a26a6c --- /dev/null +++ b/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 +} diff --git a/internal/controllers/clustercatalog_controller_test.go b/internal/controllers/clustercatalog_controller_test.go new file mode 100644 index 000000000..762fa15ec --- /dev/null +++ b/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) +}