Refactor and add missing unit tests

Signed-off-by: Per Goncalves da Silva <pegoncal@redhat.com>

Author: Per Goncalves da Silva

internal/fsutil/helpers.go

@@ -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,46 @@
+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")
+	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_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,140 @@
+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")
+	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")
+	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)
+}