🌱 Rename util packages and add missing unit tests (#1677)

* Rename util package name and file

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

* Refactor and add missing unit tests

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

---------

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

Author: perdasilva

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/util/fs.go renamed to 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,47 @@
+package fsutil_test
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/operator-framework/operator-controller/internal/fsutil"
+)
+
+func TestEnsureEmptyDirectory(t *testing.T) {
+	tempDir := t.TempDir()
+	dirPath := filepath.Join(tempDir, "testdir")
+	dirPerms := os.FileMode(0755)
+
+	t.Log("Ensure directory is created with the correct perms if it does not already exist")
+	require.NoError(t, fsutil.EnsureEmptyDirectory(dirPath, dirPerms))
+
+	stat, err := os.Stat(dirPath)
+	require.NoError(t, err)
+	require.True(t, stat.IsDir())
+	require.Equal(t, dirPerms, stat.Mode().Perm())
+
+	t.Log("Create a file inside directory")
+	file := filepath.Join(dirPath, "file1")
+	// nolint:gosec
+	require.NoError(t, os.WriteFile(file, []byte("test"), 0640))
+
+	t.Log("Create a sub-directory inside directory")
+	subDir := filepath.Join(dirPath, "subdir")
+	require.NoError(t, os.Mkdir(subDir, dirPerms))
+
+	t.Log("Call EnsureEmptyDirectory against directory with different permissions")
+	require.NoError(t, fsutil.EnsureEmptyDirectory(dirPath, 0640))
+
+	t.Log("Ensure directory is now empty")
+	entries, err := os.ReadDir(dirPath)
+	require.NoError(t, err)
+	require.Empty(t, entries)
+
+	t.Log("Ensure original directory permissions are unchanged")
+	stat, err = os.Stat(dirPath)
+	require.NoError(t, err)
+	require.Equal(t, dirPerms, stat.Mode().Perm())
+}

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

@@ -0,0 +1,80 @@
+package source
+
+import (
+	"errors"
+	"fmt"
+	"os"
+	"path/filepath"
+	"time"
+)
+
+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)
+}
+
+// IsImageUnpacked checks whether an image has been unpacked in `unpackPath`.
+// If true, time of unpack will also be returned. If false unpack time is gibberish (zero/epoch time).
+// 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 {
+		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,142 @@
+package source_test
+
+import (
+	"io/fs"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+
+	"github.com/operator-framework/operator-controller/internal/rukpak/source"
+)
+
+func TestSetReadOnlyRecursive(t *testing.T) {
+	tempDir := t.TempDir()
+	targetFilePath := filepath.Join(tempDir, "target")
+	nestedDir := filepath.Join(tempDir, "nested")
+	filePath := filepath.Join(nestedDir, "testfile")
+	symlinkPath := filepath.Join(nestedDir, "symlink")
+
+	t.Log("Create symlink target file outside directory with its own permissions")
+	// nolint:gosec
+	require.NoError(t, os.WriteFile(targetFilePath, []byte("something"), 0644))
+
+	t.Log("Create a nested directory structure that contains a file and sym. link")
+	require.NoError(t, os.Mkdir(nestedDir, source.OwnerWritableDirMode))
+	require.NoError(t, os.WriteFile(filePath, []byte("test"), source.OwnerWritableFileMode))
+	require.NoError(t, os.Symlink(targetFilePath, symlinkPath))
+
+	t.Log("Set directory structure as read-only")
+	require.NoError(t, source.SetReadOnlyRecursive(nestedDir))
+
+	t.Log("Check file permissions")
+	stat, err := os.Stat(filePath)
+	require.NoError(t, err)
+	require.Equal(t, source.OwnerReadOnlyFileMode, stat.Mode().Perm())
+
+	t.Log("Check directory permissions")
+	nestedStat, err := os.Stat(nestedDir)
+	require.NoError(t, err)
+	require.Equal(t, source.OwnerReadOnlyDirMode, nestedStat.Mode().Perm())
+
+	t.Log("Check symlink target file permissions - should not be affected")
+	stat, err = os.Stat(targetFilePath)
+	require.NoError(t, err)
+	require.Equal(t, fs.FileMode(0644), stat.Mode().Perm())
+
+	t.Log("Make directory writable to enable test clean-up")
+	require.NoError(t, source.SetWritableRecursive(tempDir))
+}
+
+func TestSetWritableRecursive(t *testing.T) {
+	tempDir := t.TempDir()
+	targetFilePath := filepath.Join(tempDir, "target")
+	nestedDir := filepath.Join(tempDir, "nested")
+	filePath := filepath.Join(nestedDir, "testfile")
+	symlinkPath := filepath.Join(nestedDir, "symlink")
+
+	t.Log("Create symlink target file outside directory with its own permissions")
+	// nolint:gosec
+	require.NoError(t, os.WriteFile(targetFilePath, []byte("something"), 0644))
+
+	t.Log("Create a nested directory (writable) structure that contains a file (read-only) and sym. link")
+	require.NoError(t, os.Mkdir(nestedDir, source.OwnerWritableDirMode))
+	require.NoError(t, os.WriteFile(filePath, []byte("test"), source.OwnerReadOnlyFileMode))
+	require.NoError(t, os.Symlink(targetFilePath, symlinkPath))
+
+	t.Log("Make directory read-only")
+	require.NoError(t, os.Chmod(nestedDir, source.OwnerReadOnlyDirMode))
+
+	t.Log("Call SetWritableRecursive")
+	require.NoError(t, source.SetWritableRecursive(nestedDir))
+
+	t.Log("Check file is writable")
+	stat, err := os.Stat(filePath)
+	require.NoError(t, err)
+	require.Equal(t, source.OwnerWritableFileMode, stat.Mode().Perm())
+
+	t.Log("Check directory is writable")
+	nestedStat, err := os.Stat(nestedDir)
+	require.NoError(t, err)
+	require.Equal(t, source.OwnerWritableDirMode, nestedStat.Mode().Perm())
+
+	t.Log("Check symlink target file permissions - should not be affected")
+	stat, err = os.Stat(targetFilePath)
+	require.NoError(t, err)
+	require.Equal(t, fs.FileMode(0644), stat.Mode().Perm())
+}
+
+func TestDeleteReadOnlyRecursive(t *testing.T) {
+	tempDir := t.TempDir()
+	nestedDir := filepath.Join(tempDir, "nested")
+	filePath := filepath.Join(nestedDir, "testfile")
+
+	t.Log("Create a nested read-only directory structure that contains a file and sym. link")
+	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))
+
+	t.Log("Set directory structure as read-only")
+	require.NoError(t, source.DeleteReadOnlyRecursive(nestedDir))
+
+	t.Log("Ensure directory was deleted")
+	_, err := os.Stat(nestedDir)
+	require.ErrorIs(t, err, os.ErrNotExist)
+}
+
+func TestIsImageUnpacked(t *testing.T) {
+	tempDir := t.TempDir()
+	unpackPath := filepath.Join(tempDir, "myimage")
+
+	t.Log("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())
+
+	t.Log("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())
+
+	t.Log("Expect file to be deleted")
+	_, err = os.Stat(unpackPath)
+	require.ErrorIs(t, err, os.ErrNotExist)
+
+	t.Log("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())
+
+	t.Log("Expect unpack time to match directory mod time")
+	stat, err := os.Stat(unpackPath)
+	require.NoError(t, err)
+	require.Equal(t, stat.ModTime(), modTime)
+}