UPSTREAM: <drop>: Clear cache on startup, use tempDir for unpacking (#1669)

Signed-off-by: Per Goncalves da Silva <[email protected]>

Author: oceanc80

Diff for: catalogd/cmd/catalogd/main.go

@@ -63,6 +63,7 @@ import (
 	"github.com/operator-framework/operator-controller/catalogd/internal/storage"
 	"github.com/operator-framework/operator-controller/catalogd/internal/version"
 	"github.com/operator-framework/operator-controller/catalogd/internal/webhook"
+	"github.com/operator-framework/operator-controller/internal/util"
 )
 
 var (
@@ -257,8 +258,8 @@ func main() {
 		systemNamespace = podNamespace()
 	}
 
-	if err := os.MkdirAll(cacheDir, 0700); err != nil {
-		setupLog.Error(err, "unable to create cache directory")
+	if err := util.EnsureEmptyDirectory(cacheDir, 0700); err != nil {
+		setupLog.Error(err, "unable to ensure empty cache directory")
 		os.Exit(1)
 	}
 

Diff for: catalogd/internal/source/containers_image.go

@@ -29,6 +29,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	catalogdv1 "github.com/operator-framework/operator-controller/catalogd/api/v1"
+	"github.com/operator-framework/operator-controller/internal/rukpak/source"
+	"github.com/operator-framework/operator-controller/internal/util"
 )
 
 const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1"
@@ -70,12 +72,11 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
 	//
 	//////////////////////////////////////////////////////
 	unpackPath := i.unpackPath(catalog.Name, canonicalRef.Digest())
-	if unpackStat, err := os.Stat(unpackPath); err == nil {
-		if !unpackStat.IsDir() {
-			panic(fmt.Sprintf("unexpected file at unpack path %q: expected a directory", unpackPath))
-		}
+	if isUnpacked, unpackTime, err := source.IsImageUnpacked(unpackPath); isUnpacked && err == nil {
 		l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String())
-		return successResult(unpackPath, canonicalRef, unpackStat.ModTime()), nil
+		return successResult(unpackPath, canonicalRef, unpackTime), nil
+	} else if err != nil {
+		return nil, fmt.Errorf("error checking image already unpacked: %w", err)
 	}
 
 	//////////////////////////////////////////////////////
@@ -148,7 +149,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
 	//
 	//////////////////////////////////////////////////////
 	if err := i.unpackImage(ctx, unpackPath, layoutRef, specIsCanonical, srcCtx); err != nil {
-		if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil {
+		if cleanupErr := source.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
 			err = errors.Join(err, cleanupErr)
 		}
 		return nil, fmt.Errorf("error unpacking image: %w", err)
@@ -189,7 +190,7 @@ func successResult(unpackPath string, canonicalRef reference.Canonical, lastUnpa
 }
 
 func (i *ContainersImageRegistry) Cleanup(_ context.Context, catalog *catalogdv1.ClusterCatalog) error {
-	if err := deleteRecursive(i.catalogPath(catalog.Name)); err != nil {
+	if err := source.DeleteReadOnlyRecursive(i.catalogPath(catalog.Name)); err != nil {
 		return fmt.Errorf("error deleting catalog cache: %w", err)
 	}
 	return nil
@@ -288,8 +289,8 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
 		return wrapTerminal(fmt.Errorf("catalog image is missing the required label %q", ConfigDirLabel), specIsCanonical)
 	}
 
-	if err := os.MkdirAll(unpackPath, 0700); err != nil {
-		return fmt.Errorf("error creating unpack directory: %w", err)
+	if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
+		return fmt.Errorf("error ensuring empty unpack directory: %w", err)
 	}
 	l := log.FromContext(ctx)
 	l.Info("unpacking image", "path", unpackPath)
@@ -307,10 +308,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
 			l.Info("applied layer", "layer", i)
 			return nil
 		}(); err != nil {
-			return errors.Join(err, deleteRecursive(unpackPath))
+			return errors.Join(err, source.DeleteReadOnlyRecursive(unpackPath))
 		}
 	}
-	if err := setReadOnlyRecursive(unpackPath); err != nil {
+	if err := source.SetReadOnlyRecursive(unpackPath); err != nil {
 		return fmt.Errorf("error making unpack directory read-only: %w", err)
 	}
 	return nil
@@ -354,69 +355,13 @@ func (i *ContainersImageRegistry) deleteOtherImages(catalogName string, digestTo
 			continue
 		}
 		imgDirPath := filepath.Join(catalogPath, imgDir.Name())
-		if err := deleteRecursive(imgDirPath); err != nil {
+		if err := source.DeleteReadOnlyRecursive(imgDirPath); err != nil {
 			return fmt.Errorf("error removing image directory: %w", err)
 		}
 	}
 	return nil
 }
 
-func setReadOnlyRecursive(root string) error {
-	if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
-		if err != nil {
-			return err
-		}
-
-		fi, err := d.Info()
-		if err != nil {
-			return err
-		}
-
-		if err := func() error {
-			switch typ := fi.Mode().Type(); typ {
-			case os.ModeSymlink:
-				// do not follow symlinks
-				// 1. if they resolve to other locations in the root, we'll find them anyway
-				// 2. if they resolve to other locations outside the root, we don't want to change their permissions
-				return nil
-			case os.ModeDir:
-				return os.Chmod(path, 0500)
-			case 0: // regular file
-				return os.Chmod(path, 0400)
-			default:
-				return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())
-			}
-		}(); err != nil {
-			return err
-		}
-		return nil
-	}); err != nil {
-		return fmt.Errorf("error making catalog cache read-only: %w", err)
-	}
-	return nil
-}
-
-func deleteRecursive(root string) error {
-	if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
-		if os.IsNotExist(err) {
-			return nil
-		}
-		if err != nil {
-			return err
-		}
-		if !d.IsDir() {
-			return nil
-		}
-		if err := os.Chmod(path, 0700); err != nil {
-			return err
-		}
-		return nil
-	}); err != nil {
-		return fmt.Errorf("error making catalog cache writable for deletion: %w", err)
-	}
-	return os.RemoveAll(root)
-}
-
 func wrapTerminal(err error, isTerminal bool) error {
 	if !isTerminal {
 		return err

Diff for: cmd/operator-controller/main.go

@@ -68,6 +68,7 @@ import (
 	"github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
 	"github.com/operator-framework/operator-controller/internal/rukpak/source"
 	"github.com/operator-framework/operator-controller/internal/scheme"
+	"github.com/operator-framework/operator-controller/internal/util"
 	"github.com/operator-framework/operator-controller/internal/version"
 )
 
@@ -297,6 +298,11 @@ func main() {
 		}
 	}
 
+	if err := util.EnsureEmptyDirectory(cachePath, 0700); err != nil {
+		setupLog.Error(err, "unable to ensure empty cache directory")
+		os.Exit(1)
+	}
+
 	unpacker := &source.ContainersImageRegistry{
 		BaseCachePath: filepath.Join(cachePath, "unpack"),
 		SourceContextFunc: func(logger logr.Logger) (*types.SystemContext, error) {
@@ -361,7 +367,7 @@ func main() {
 		crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
 	}
 
-	applier := &applier.Helm{
+	helmApplier := &applier.Helm{
 		ActionClientGetter: acg,
 		Preflights:         preflights,
 	}
@@ -381,7 +387,7 @@ func main() {
 		Client:                cl,
 		Resolver:              resolver,
 		Unpacker:              unpacker,
-		Applier:               applier,
+		Applier:               helmApplier,
 		InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg},
 		Finalizers:            clusterExtensionFinalizers,
 		Manager:               cm,

Diff for: internal/rukpak/source/containers_image.go

@@ -23,6 +23,8 @@ import (
 	"github.com/opencontainers/go-digest"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+
+	"github.com/operator-framework/operator-controller/internal/util"
 )
 
 type ContainersImageRegistry struct {
@@ -62,12 +64,11 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
 	//
 	//////////////////////////////////////////////////////
 	unpackPath := i.unpackPath(bundle.Name, canonicalRef.Digest())
-	if unpackStat, err := os.Stat(unpackPath); err == nil {
-		if !unpackStat.IsDir() {
-			panic(fmt.Sprintf("unexpected file at unpack path %q: expected a directory", unpackPath))
-		}
+	if isUnpacked, _, err := IsImageUnpacked(unpackPath); isUnpacked && err == nil {
 		l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String())
 		return successResult(bundle.Name, unpackPath, canonicalRef), nil
+	} else if err != nil {
+		return nil, fmt.Errorf("error checking bundle already unpacked: %w", err)
 	}
 
 	//////////////////////////////////////////////////////
@@ -140,7 +141,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
 	//
 	//////////////////////////////////////////////////////
 	if err := i.unpackImage(ctx, unpackPath, layoutRef, srcCtx); err != nil {
-		if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil {
+		if cleanupErr := DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
 			err = errors.Join(err, cleanupErr)
 		}
 		return nil, fmt.Errorf("error unpacking image: %w", err)
@@ -168,7 +169,7 @@ func successResult(bundleName, unpackPath string, canonicalRef reference.Canonic
 }
 
 func (i *ContainersImageRegistry) Cleanup(_ context.Context, bundle *BundleSource) error {
-	return deleteRecursive(i.bundlePath(bundle.Name))
+	return DeleteReadOnlyRecursive(i.bundlePath(bundle.Name))
 }
 
 func (i *ContainersImageRegistry) bundlePath(bundleName string) string {
@@ -251,8 +252,8 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
 		return fmt.Errorf("error creating image source: %w", err)
 	}
 
-	if err := os.MkdirAll(unpackPath, 0700); err != nil {
-		return fmt.Errorf("error creating unpack directory: %w", err)
+	if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
+		return fmt.Errorf("error ensuring empty unpack directory: %w", err)
 	}
 	l := log.FromContext(ctx)
 	l.Info("unpacking image", "path", unpackPath)
@@ -270,10 +271,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
 			l.Info("applied layer", "layer", i)
 			return nil
 		}(); err != nil {
-			return errors.Join(err, deleteRecursive(unpackPath))
+			return errors.Join(err, DeleteReadOnlyRecursive(unpackPath))
 		}
 	}
-	if err := setReadOnlyRecursive(unpackPath); err != nil {
+	if err := SetReadOnlyRecursive(unpackPath); err != nil {
 		return fmt.Errorf("error making unpack directory read-only: %w", err)
 	}
 	return nil
@@ -310,65 +311,9 @@ func (i *ContainersImageRegistry) deleteOtherImages(bundleName string, digestToK
 			continue
 		}
 		imgDirPath := filepath.Join(bundlePath, imgDir.Name())
-		if err := deleteRecursive(imgDirPath); err != nil {
+		if err := DeleteReadOnlyRecursive(imgDirPath); err != nil {
 			return fmt.Errorf("error removing image directory: %w", err)
 		}
 	}
 	return nil
 }
-
-func setReadOnlyRecursive(root string) error {
-	if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
-		if err != nil {
-			return err
-		}
-
-		fi, err := d.Info()
-		if err != nil {
-			return err
-		}
-
-		if err := func() error {
-			switch typ := fi.Mode().Type(); typ {
-			case os.ModeSymlink:
-				// do not follow symlinks
-				// 1. if they resolve to other locations in the root, we'll find them anyway
-				// 2. if they resolve to other locations outside the root, we don't want to change their permissions
-				return nil
-			case os.ModeDir:
-				return os.Chmod(path, 0500)
-			case 0: // regular file
-				return os.Chmod(path, 0400)
-			default:
-				return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())
-			}
-		}(); err != nil {
-			return err
-		}
-		return nil
-	}); err != nil {
-		return fmt.Errorf("error making bundle cache read-only: %w", err)
-	}
-	return nil
-}
-
-func deleteRecursive(root string) error {
-	if err := filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error {
-		if os.IsNotExist(err) {
-			return nil
-		}
-		if err != nil {
-			return err
-		}
-		if !d.IsDir() {
-			return nil
-		}
-		if err := os.Chmod(path, 0700); err != nil {
-			return err
-		}
-		return nil
-	}); err != nil {
-		return fmt.Errorf("error making bundle cache writable for deletion: %w", err)
-	}
-	return os.RemoveAll(root)
-}

Diff for: internal/rukpak/source/containers_image_test.go

@@ -277,7 +277,16 @@ func TestUnpackUnexpectedFile(t *testing.T) {
 	require.NoError(t, os.WriteFile(unpackPath, []byte{}, 0600))
 
 	// Attempt to pull and unpack the image
-	assert.Panics(t, func() { _, _ = unpacker.Unpack(context.Background(), bundleSource) })
+	_, err := unpacker.Unpack(context.Background(), bundleSource)
+	require.NoError(t, err)
+
+	// Ensure unpack path is now a directory
+	stat, err := os.Stat(unpackPath)
+	require.NoError(t, err)
+	require.True(t, stat.IsDir())
+
+	// Unset read-only to allow cleanup
+	require.NoError(t, source.UnsetReadOnlyRecursive(unpackPath))
 }
 
 func TestUnpackCopySucceedsMountFails(t *testing.T) {