OCPBUGS-49683: cache bug fixes (round 2)

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/fsutil"
 )
 
 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 := fsutil.EnsureEmptyDirectory(cacheDir, 0700); err != nil {
+		setupLog.Error(err, "unable to ensure empty cache directory")
 		os.Exit(1)
 	}
 

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/fsutil"
+	"github.com/operator-framework/operator-controller/internal/rukpak/source"
 )
 
 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 := fsutil.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 := fsutil.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 := fsutil.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, fsutil.DeleteReadOnlyRecursive(unpackPath))
 		}
 	}
-	if err := setReadOnlyRecursive(unpackPath); err != nil {
+	if err := fsutil.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 := fsutil.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

cmd/operator-controller/main.go

@@ -63,6 +63,7 @@ import (
 	"github.com/operator-framework/operator-controller/internal/controllers"
 	"github.com/operator-framework/operator-controller/internal/features"
 	"github.com/operator-framework/operator-controller/internal/finalizers"
+	"github.com/operator-framework/operator-controller/internal/fsutil"
 	"github.com/operator-framework/operator-controller/internal/httputil"
 	"github.com/operator-framework/operator-controller/internal/resolve"
 	"github.com/operator-framework/operator-controller/internal/rukpak/preflights/crdupgradesafety"
@@ -297,6 +298,11 @@ func main() {
 		}
 	}
 
+	if err := fsutil.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,

internal/fsutil/helpers.go

@@ -0,0 +1,80 @@
+package fsutil
+
+import (
+	"fmt"
+	"io/fs"
+	"os"
+	"path/filepath"
+)
+
+// EnsureEmptyDirectory ensures the directory given by `path` is empty.
+// If the directory does not exist, it will be created with permission bits
+// given by `perm`. If the directory exists, it will not simply rm -rf && mkdir -p
+// as the calling process may not have permissions to delete the directory. E.g.
+// in the case of a pod mount. Rather, it will delete the contents of the directory.
+func EnsureEmptyDirectory(path string, perm fs.FileMode) error {
+	entries, err := os.ReadDir(path)
+	if err != nil && !os.IsNotExist(err) {
+		return err
+	}
+	for _, entry := range entries {
+		if err := DeleteReadOnlyRecursive(filepath.Join(path, entry.Name())); err != nil {
+			return err
+		}
+	}
+	return os.MkdirAll(path, perm)
+}
+
+const (
+	ownerWritableFileMode os.FileMode = 0700
+	ownerWritableDirMode  os.FileMode = 0700
+	ownerReadOnlyFileMode os.FileMode = 0400
+	ownerReadOnlyDirMode  os.FileMode = 0500
+)
+
+// SetReadOnlyRecursive recursively sets files and directories under the path given by `root` as read-only
+func SetReadOnlyRecursive(root string) error {
+	return setModeRecursive(root, ownerReadOnlyFileMode, ownerReadOnlyDirMode)
+}
+
+// SetWritableRecursive recursively sets files and directories under the path given by `root` as writable
+func SetWritableRecursive(root string) error {
+	return setModeRecursive(root, ownerWritableFileMode, ownerWritableDirMode)
+}
+
+// DeleteReadOnlyRecursive deletes read-only directory with path given by `root`
+func DeleteReadOnlyRecursive(root string) error {
+	if err := SetWritableRecursive(root); err != nil {
+		return fmt.Errorf("error making directory writable for deletion: %w", err)
+	}
+	return os.RemoveAll(root)
+}
+
+func setModeRecursive(path string, fileMode os.FileMode, dirMode os.FileMode) error {
+	return filepath.WalkDir(path, func(path string, d os.DirEntry, err error) error {
+		if os.IsNotExist(err) {
+			return nil
+		}
+		if err != nil {
+			return err
+		}
+		fi, err := d.Info()
+		if err != nil {
+			return err
+		}
+
+		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, dirMode)
+		case 0: // regular file
+			return os.Chmod(path, fileMode)
+		default:
+			return fmt.Errorf("refusing to change ownership of file %q with type %v", path, typ.String())
+		}
+	})
+}