Skip to content

Commit

Permalink
🌱 Rename util packages and add missing unit tests (#1677)
Browse files Browse the repository at this point in the history
* 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]>
  • Loading branch information
perdasilva and Per Goncalves da Silva authored Jan 31, 2025
1 parent c929975 commit d065a49
Show file tree
Hide file tree
Showing 10 changed files with 282 additions and 97 deletions.
4 changes: 2 additions & 2 deletions catalogd/cmd/catalogd/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 (
Expand Down Expand Up @@ -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)
}
Expand Down
4 changes: 2 additions & 2 deletions catalogd/internal/source/containers_image.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand Down
4 changes: 2 additions & 2 deletions cmd/operator-controller/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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)
}
Expand Down
6 changes: 4 additions & 2 deletions internal/util/fs.go → internal/fsutil/helpers.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package util
package fsutil

import (
"io/fs"
Expand All @@ -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) {
Expand Down
47 changes: 47 additions & 0 deletions internal/fsutil/helpers_test.go
Original file line number Diff line number Diff line change
@@ -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())
}
4 changes: 2 additions & 2 deletions internal/rukpak/source/containers_image.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"}]}`)
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion internal/rukpak/source/containers_image_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
80 changes: 80 additions & 0 deletions internal/rukpak/source/helpers.go
Original file line number Diff line number Diff line change
@@ -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())
}
})
}
142 changes: 142 additions & 0 deletions internal/rukpak/source/helpers_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
Loading

0 comments on commit d065a49

Please sign in to comment.