fixes

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

Author: Per Goncalves da Silva

catalogd/cmd/catalogd/main.go

@@ -63,7 +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"
+	"github.com/operator-framework/operator-controller/internal/fsutil"
 )
 
 var (
@@ -258,7 +258,7 @@ func main() {
 		systemNamespace = podNamespace()
 	}
 
-	if err := util.EnsureEmptyDirectory(cacheDir, 0700); err != nil {
+	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

@@ -30,8 +30,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"
-	"github.com/operator-framework/operator-controller/internal/util"
 )
 
 const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1"
@@ -297,7 +297,7 @@ 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 := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
+	if err := fsutil.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
 		return fmt.Errorf("error ensuring empty unpack directory: %w", err)
 	}
 	l := log.FromContext(ctx)

cmd/operator-controller/main.go

@@ -63,12 +63,12 @@ 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"
 	"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"
 )
 
@@ -300,7 +300,7 @@ func main() {
 		}
 	}
 
-	if err := util.EnsureEmptyDirectory(cachePath, 0700); err != nil {
+	if err := fsutil.EnsureEmptyDirectory(cachePath, 0700); err != nil {
 		setupLog.Error(err, "unable to ensure empty cache directory")
 		os.Exit(1)
 	}

internal/fsutil/helpers.go

@@ -1,4 +1,4 @@
-package util
+package fsutil
 
 import (
 	"io/fs"
@@ -8,7 +8,9 @@ import (
 
 // 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`.
+// 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) {

internal/fsutil/helpers_test.go

@@ -0,0 +1,34 @@
+package fsutil_test
+
+import (
+	"github.com/stretchr/testify/require"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestEnsureEmptyDirectory(t *testing.T) {
+	tempDir := t.TempDir()
+	dirPath := filepath.Join(tempDir, "testdir")
+
+	// Ensure directory is created if not exists
+	require.NoError(t, EnsureEmptyDirectory(dirPath, 0755))
+
+	stat, err := os.Stat(dirPath)
+	require.NoError(t, err)
+	require.True(t, stat.IsDir())
+
+	// Create files inside directory
+	file := filepath.Join(dirPath, "file1")
+	require.NoError(t, os.WriteFile(file, []byte("test"), 0644))
+
+	subDir := filepath.Join(dirPath, "subdir")
+	require.NoError(t, os.Mkdir(subDir, 0755))
+
+	// Ensure directory is emptied but not deleted
+	require.NoError(t, EnsureEmptyDirectory(dirPath, 0755))
+
+	entries, err := os.ReadDir(dirPath)
+	require.NoError(t, err)
+	require.Empty(t, entries)
+}

internal/rukpak/source/containers_image.go

@@ -25,7 +25,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
-	"github.com/operator-framework/operator-controller/internal/util"
+	"github.com/operator-framework/operator-controller/internal/fsutil"
 )
 
 var insecurePolicy = []byte(`{"default":[{"type":"insecureAcceptAnything"}]}`)
@@ -266,7 +266,7 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
 		}
 	}()
 
-	if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
+	if err := fsutil.EnsureEmptyDirectory(unpackPath, 0700); err != nil {
 		return fmt.Errorf("error ensuring empty unpack directory: %w", err)
 	}
 	l := log.FromContext(ctx)

internal/rukpak/source/containers_image_test.go

@@ -286,7 +286,7 @@ func TestUnpackUnexpectedFile(t *testing.T) {
 	require.True(t, stat.IsDir())
 
 	// Unset read-only to allow cleanup
-	require.NoError(t, source.UnsetReadOnlyRecursive(unpackPath))
+	require.NoError(t, source.SetWritableRecursive(unpackPath))
 }
 
 func TestUnpackCopySucceedsMountFails(t *testing.T) {

internal/rukpak/source/helpers.go

@@ -8,61 +8,26 @@ import (
 	"time"
 )
 
-// SetReadOnlyRecursive sets directory with path given by `root` as read-only
-func SetReadOnlyRecursive(root string) error {
-	return 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
-		}
+const (
+	OwnerWritableFileMode os.FileMode = 0700
+	OwnerWritableDirMode  os.FileMode = 0700
+	OwnerReadOnlyFileMode os.FileMode = 0400
+	OwnerReadOnlyDirMode  os.FileMode = 0500
+)
 
-		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
-	})
+// 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)
 }
 
-// UnsetReadOnlyRecursive unsets directory with path given by `root` as read-only
-func UnsetReadOnlyRecursive(root string) error {
-	return 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
-	})
+// 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 := UnsetReadOnlyRecursive(root); err != nil {
+	if err := SetWritableRecursive(root); err != nil {
 		return fmt.Errorf("error making directory writable for deletion: %w", err)
 	}
 	return os.RemoveAll(root)
@@ -73,14 +38,43 @@ func DeleteReadOnlyRecursive(root string) error {
 // If `unpackPath` is a file, it will be deleted and false will be returned without an error.
 func IsImageUnpacked(unpackPath string) (bool, time.Time, error) {
 	unpackStat, err := os.Stat(unpackPath)
+	if errors.Is(err, os.ErrNotExist) {
+		return false, time.Time{}, nil
+	}
 	if err != nil {
-		if errors.Is(err, os.ErrNotExist) {
-			return false, time.Time{}, nil
-		}
 		return false, time.Time{}, err
 	}
 	if !unpackStat.IsDir() {
 		return false, time.Time{}, os.Remove(unpackPath)
 	}
 	return true, unpackStat.ModTime(), nil
 }
+
+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())
+		}
+	})
+}

internal/rukpak/source/helpers_test.go

@@ -0,0 +1,131 @@
+package source_test
+
+import (
+	"errors"
+	"github.com/operator-framework/operator-controller/internal/rukpak/source"
+	"github.com/stretchr/testify/require"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestSetReadOnlyRecursive(t *testing.T) {
+	// Create a nested directory structure that contains a file and sym. link
+	tempDir := t.TempDir()
+	nestedDir := filepath.Join(tempDir, "nested")
+	filePath := filepath.Join(nestedDir, "testfile")
+	symlinkPath := filepath.Join(nestedDir, "symlink")
+
+	require.NoError(t, os.Mkdir(nestedDir, source.OwnerWritableDirMode))
+	require.NoError(t, os.WriteFile(filePath, []byte("test"), source.OwnerWritableFileMode))
+	require.NoError(t, os.Symlink(filePath, symlinkPath))
+
+	// Set directory structure as read-only
+	require.NoError(t, source.SetReadOnlyRecursive(tempDir))
+
+	// Check file perm
+	stat, err := os.Stat(filePath)
+	require.NoError(t, err)
+	require.Equal(t, source.OwnerReadOnlyFileMode, stat.Mode().Perm())
+
+	// Check directory perm
+	nestedStat, err := os.Stat(nestedDir)
+	require.NoError(t, err)
+	require.Equal(t, source.OwnerReadOnlyDirMode, nestedStat.Mode().Perm())
+
+	// Check sym. link perm - should not be affected
+	symlinkStat, err := os.Lstat(symlinkPath)
+	require.NoError(t, err)
+	require.True(t, symlinkStat.Mode()&os.ModeSymlink != 0)
+
+	// Make directory writable to enable test clean-up
+	require.NoError(t, source.SetWritableRecursive(tempDir))
+}
+
+func TestSetWritableRecursive(t *testing.T) {
+	// Create a nested directory structure that contains a file and sym. link
+	tempDir := t.TempDir()
+	nestedDir := filepath.Join(tempDir, "nested")
+	filePath := filepath.Join(nestedDir, "testfile")
+	symlinkPath := filepath.Join(nestedDir, "symlink")
+
+	// nested dir mode is writable while we stamp out the other filesystem elements
+	require.NoError(t, os.Mkdir(nestedDir, source.OwnerWritableDirMode))
+	require.NoError(t, os.WriteFile(filePath, []byte("test"), source.OwnerReadOnlyFileMode))
+	require.NoError(t, os.Symlink(filePath, symlinkPath))
+
+	// Now, set nested dir as read-only
+	require.NoError(t, os.Chmod(nestedDir, source.OwnerReadOnlyDirMode))
+
+	// Set directory structure as writable
+	require.NoError(t, source.SetWritableRecursive(tempDir))
+
+	// Check file perm
+	stat, err := os.Stat(filePath)
+	require.NoError(t, err)
+	require.Equal(t, source.OwnerWritableFileMode, stat.Mode().Perm())
+
+	// Check directory perm
+	nestedStat, err := os.Stat(nestedDir)
+	require.NoError(t, err)
+	require.Equal(t, source.OwnerWritableDirMode, nestedStat.Mode().Perm())
+
+	// Check sym link perm - should not be affected
+	symlinkStat, err := os.Lstat(symlinkPath)
+	require.NoError(t, err)
+	require.True(t, symlinkStat.Mode()&os.ModeSymlink != 0)
+}
+
+func TestDeleteReadOnlyRecursive(t *testing.T) {
+	// Create a nested directory structure
+	tempDir := t.TempDir()
+	nestedDir := filepath.Join(tempDir, "nested")
+	filePath := filepath.Join(nestedDir, "testfile")
+
+	require.NoError(t, os.Mkdir(nestedDir, source.OwnerWritableDirMode))
+	require.NoError(t, os.WriteFile(filePath, []byte("test"), source.OwnerReadOnlyFileMode))
+	require.NoError(t, os.Chmod(nestedDir, source.OwnerReadOnlyDirMode))
+
+	// Set directory structure as read-only
+	require.NoError(t, source.DeleteReadOnlyRecursive(tempDir))
+
+	// Ensure directory is gone
+	_, err := os.Stat(tempDir)
+	require.True(t, errors.Is(err, os.ErrNotExist))
+}
+
+func TestIsImageUnpacked(t *testing.T) {
+	tempDir := t.TempDir()
+	unpackPath := filepath.Join(tempDir, "myimage")
+
+	// Test case: unpack path does not exist
+	unpacked, modTime, err := source.IsImageUnpacked(unpackPath)
+	require.NoError(t, err)
+	require.False(t, unpacked)
+	require.True(t, modTime.IsZero())
+
+	// Test case: unpack path points to file
+	require.NoError(t, os.WriteFile(unpackPath, []byte("test"), source.OwnerWritableFileMode))
+
+	unpacked, modTime, err = source.IsImageUnpacked(filepath.Join(tempDir, "myimage"))
+	require.NoError(t, err)
+	require.False(t, unpacked)
+	require.True(t, modTime.IsZero())
+
+	// Expect file to be deleted
+	_, err = os.Stat(unpackPath)
+	require.True(t, errors.Is(err, os.ErrNotExist))
+
+	// Test case: unpack path points to directory (happy path)
+	require.NoError(t, os.Mkdir(unpackPath, source.OwnerWritableDirMode))
+
+	unpacked, modTime, err = source.IsImageUnpacked(unpackPath)
+	require.NoError(t, err)
+	require.True(t, unpacked)
+	require.False(t, modTime.IsZero())
+
+	// Expect unpack time to match directory mod time
+	stat, err := os.Stat(unpackPath)
+	require.NoError(t, err)
+	require.Equal(t, stat.ModTime(), modTime)
+}