Skip to content

🐛 EnsureEmptyDirectory should recursively set writable perms prior to delete #1691

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 3, 2025
Merged

🐛 EnsureEmptyDirectory should recursively set writable perms prior to delete #1691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions catalogd/internal/source/containers_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv
//
//////////////////////////////////////////////////////
if err := i.unpackImage(ctx, unpackPath, layoutRef, specIsCanonical, srcCtx); err != nil {
if cleanupErr := source.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
if cleanupErr := fsutil.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
err = errors.Join(err, cleanupErr)
}
return nil, fmt.Errorf("error unpacking image: %w", err)
Expand Down Expand Up @@ -196,7 +196,7 @@ func successResult(unpackPath string, canonicalRef reference.Canonical, lastUnpa
}

func (i *ContainersImageRegistry) Cleanup(_ context.Context, catalog *catalogdv1.ClusterCatalog) error {
if err := source.DeleteReadOnlyRecursive(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
Expand Down Expand Up @@ -316,10 +316,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
l.Info("applied layer", "layer", i)
return nil
}(); err != nil {
return errors.Join(err, source.DeleteReadOnlyRecursive(unpackPath))
return errors.Join(err, fsutil.DeleteReadOnlyRecursive(unpackPath))
}
}
if err := source.SetReadOnlyRecursive(unpackPath); err != nil {
if err := fsutil.SetReadOnlyRecursive(unpackPath); err != nil {
return fmt.Errorf("error making unpack directory read-only: %w", err)
}
return nil
Expand Down Expand Up @@ -363,7 +363,7 @@ func (i *ContainersImageRegistry) deleteOtherImages(catalogName string, digestTo
continue
}
imgDirPath := filepath.Join(catalogPath, imgDir.Name())
if err := source.DeleteReadOnlyRecursive(imgDirPath); err != nil {
if err := fsutil.DeleteReadOnlyRecursive(imgDirPath); err != nil {
return fmt.Errorf("error removing image directory: %w", err)
}
}
Expand Down
57 changes: 56 additions & 1 deletion internal/fsutil/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package fsutil

import (
"fmt"
"io/fs"
"os"
"path/filepath"
Expand All @@ -17,9 +18,63 @@ func EnsureEmptyDirectory(path string, perm fs.FileMode) error {
return err
}
for _, entry := range entries {
if err := os.RemoveAll(filepath.Join(path, entry.Name())); err != nil {
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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: would be nice to have the prerequisite of setting to writable mentioned in the doc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting to writable is not a prerequisite of this function. This function will attempt to set everything writable before attempting to delete.

Regardless, this PR is mainly about moving some existing code around to resolve a bug with EnsureEmptyDirectory, so I'd like to defer nice-to-have changes to a follow-up.

Copy link
Contributor

@azych azych Feb 3, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • setting everything writable is a prerequisite for actually recursively deleting root, which is what the function name and its doc comment says it will do
  • it would be nice to include that in the doc comment, that shouldn't be longer than 1 line
    Still it's a nit, so feel free to go with whatever you think works best

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())
}
})
}
110 changes: 102 additions & 8 deletions internal/fsutil/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,12 @@
package fsutil_test
package fsutil

import (
"io/fs"
"os"
"path/filepath"
"testing"

"github.com/stretchr/testify/require"

"github.com/operator-framework/operator-controller/internal/fsutil"
)

func TestEnsureEmptyDirectory(t *testing.T) {
Expand All @@ -16,7 +15,7 @@ func TestEnsureEmptyDirectory(t *testing.T) {
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))
require.NoError(t, EnsureEmptyDirectory(dirPath, dirPerms))

stat, err := os.Stat(dirPath)
require.NoError(t, err)
Expand All @@ -25,15 +24,16 @@ func TestEnsureEmptyDirectory(t *testing.T) {

t.Log("Create a file inside directory")
file := filepath.Join(dirPath, "file1")
// nolint:gosec
require.NoError(t, os.WriteFile(file, []byte("test"), 0640))
// write file as read-only to verify EnsureEmptyDirectory can still delete it.
require.NoError(t, os.WriteFile(file, []byte("test"), 0400))

t.Log("Create a sub-directory inside directory")
subDir := filepath.Join(dirPath, "subdir")
require.NoError(t, os.Mkdir(subDir, dirPerms))
// write subDir as read-execute-only to verify EnsureEmptyDirectory can still delete it.
require.NoError(t, os.Mkdir(subDir, 0500))

t.Log("Call EnsureEmptyDirectory against directory with different permissions")
require.NoError(t, fsutil.EnsureEmptyDirectory(dirPath, 0640))
require.NoError(t, EnsureEmptyDirectory(dirPath, 0640))

t.Log("Ensure directory is now empty")
entries, err := os.ReadDir(dirPath)
Expand All @@ -45,3 +45,97 @@ func TestEnsureEmptyDirectory(t *testing.T) {
require.NoError(t, err)
require.Equal(t, dirPerms, stat.Mode().Perm())
}

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, ownerWritableDirMode))
require.NoError(t, os.WriteFile(filePath, []byte("test"), ownerWritableFileMode))
require.NoError(t, os.Symlink(targetFilePath, symlinkPath))

t.Log("Set directory structure as read-only")
require.NoError(t, SetReadOnlyRecursive(nestedDir))

t.Log("Check file permissions")
stat, err := os.Stat(filePath)
require.NoError(t, err)
require.Equal(t, ownerReadOnlyFileMode, stat.Mode().Perm())

t.Log("Check directory permissions")
nestedStat, err := os.Stat(nestedDir)
require.NoError(t, err)
require.Equal(t, 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, 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, ownerWritableDirMode))
require.NoError(t, os.WriteFile(filePath, []byte("test"), ownerReadOnlyFileMode))
require.NoError(t, os.Symlink(targetFilePath, symlinkPath))

t.Log("Make directory read-only")
require.NoError(t, os.Chmod(nestedDir, ownerReadOnlyDirMode))

t.Log("Call SetWritableRecursive")
require.NoError(t, SetWritableRecursive(nestedDir))

t.Log("Check file is writable")
stat, err := os.Stat(filePath)
require.NoError(t, err)
require.Equal(t, ownerWritableFileMode, stat.Mode().Perm())

t.Log("Check directory is writable")
nestedStat, err := os.Stat(nestedDir)
require.NoError(t, err)
require.Equal(t, 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, ownerWritableDirMode))
require.NoError(t, os.WriteFile(filePath, []byte("test"), ownerReadOnlyFileMode))
require.NoError(t, os.Chmod(nestedDir, ownerReadOnlyDirMode))

t.Log("Set directory structure as read-only via DeleteReadOnlyRecursive")
require.NoError(t, DeleteReadOnlyRecursive(nestedDir))

t.Log("Ensure directory was deleted")
_, err := os.Stat(nestedDir)
require.ErrorIs(t, err, os.ErrNotExist)
}
10 changes: 5 additions & 5 deletions internal/rukpak/source/containers_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour
//
//////////////////////////////////////////////////////
if err := i.unpackImage(ctx, unpackPath, layoutRef, srcCtx); err != nil {
if cleanupErr := DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
if cleanupErr := fsutil.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil {
err = errors.Join(err, cleanupErr)
}
return nil, fmt.Errorf("error unpacking image: %w", err)
Expand Down Expand Up @@ -176,7 +176,7 @@ func successResult(bundleName, unpackPath string, canonicalRef reference.Canonic
}

func (i *ContainersImageRegistry) Cleanup(_ context.Context, bundle *BundleSource) error {
return DeleteReadOnlyRecursive(i.bundlePath(bundle.Name))
return fsutil.DeleteReadOnlyRecursive(i.bundlePath(bundle.Name))
}

func (i *ContainersImageRegistry) bundlePath(bundleName string) string {
Expand Down Expand Up @@ -285,10 +285,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
l.Info("applied layer", "layer", i)
return nil
}(); err != nil {
return errors.Join(err, DeleteReadOnlyRecursive(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
Expand Down Expand Up @@ -325,7 +325,7 @@ func (i *ContainersImageRegistry) deleteOtherImages(bundleName string, digestToK
continue
}
imgDirPath := filepath.Join(bundlePath, imgDir.Name())
if err := DeleteReadOnlyRecursive(imgDirPath); err != nil {
if err := fsutil.DeleteReadOnlyRecursive(imgDirPath); err != nil {
return fmt.Errorf("error removing image directory: %w", err)
}
}
Expand Down
3 changes: 2 additions & 1 deletion internal/rukpak/source/containers_image_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"github.com/stretchr/testify/require"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

"github.com/operator-framework/operator-controller/internal/fsutil"
"github.com/operator-framework/operator-controller/internal/rukpak/source"
)

Expand Down Expand Up @@ -286,7 +287,7 @@ func TestUnpackUnexpectedFile(t *testing.T) {
require.True(t, stat.IsDir())

// Unset read-only to allow cleanup
require.NoError(t, source.SetWritableRecursive(unpackPath))
require.NoError(t, fsutil.SetWritableRecursive(unpackPath))
}

func TestUnpackCopySucceedsMountFails(t *testing.T) {
Expand Down
56 changes: 0 additions & 56 deletions internal/rukpak/source/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,37 +2,10 @@ 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.
Expand All @@ -49,32 +22,3 @@ func IsImageUnpacked(unpackPath string) (bool, time.Time, error) {
}
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())
}
})
}
Loading
Loading