From 143884915832ec752bb937e0469c339ac6f93020 Mon Sep 17 00:00:00 2001 From: oceanc80 Date: Thu, 30 Jan 2025 12:34:52 -0500 Subject: [PATCH 1/3] UPSTREAM: : Clear cache on startup, use tempDir for unpacking (#1669) Signed-off-by: Per Goncalves da Silva --- catalogd/cmd/catalogd/main.go | 5 +- catalogd/internal/source/containers_image.go | 81 +++-------------- cmd/operator-controller/main.go | 10 ++- internal/rukpak/source/containers_image.go | 79 +++-------------- .../rukpak/source/containers_image_test.go | 11 ++- internal/rukpak/source/util.go | 86 +++++++++++++++++++ internal/util/fs.go | 23 +++++ 7 files changed, 155 insertions(+), 140 deletions(-) create mode 100644 internal/rukpak/source/util.go create mode 100644 internal/util/fs.go diff --git a/catalogd/cmd/catalogd/main.go b/catalogd/cmd/catalogd/main.go index 35854aeae..45baed165 100644 --- a/catalogd/cmd/catalogd/main.go +++ b/catalogd/cmd/catalogd/main.go @@ -63,6 +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" ) var ( @@ -257,8 +258,8 @@ func main() { systemNamespace = podNamespace() } - if err := os.MkdirAll(cacheDir, 0700); err != nil { - setupLog.Error(err, "unable to create cache directory") + if err := util.EnsureEmptyDirectory(cacheDir, 0700); err != nil { + setupLog.Error(err, "unable to ensure empty cache directory") os.Exit(1) } diff --git a/catalogd/internal/source/containers_image.go b/catalogd/internal/source/containers_image.go index c00db5c0f..d67221efc 100644 --- a/catalogd/internal/source/containers_image.go +++ b/catalogd/internal/source/containers_image.go @@ -29,6 +29,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/rukpak/source" + "github.com/operator-framework/operator-controller/internal/util" ) const ConfigDirLabel = "operators.operatorframework.io.index.configs.v1" @@ -70,12 +72,11 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv // ////////////////////////////////////////////////////// unpackPath := i.unpackPath(catalog.Name, canonicalRef.Digest()) - if unpackStat, err := os.Stat(unpackPath); err == nil { - if !unpackStat.IsDir() { - panic(fmt.Sprintf("unexpected file at unpack path %q: expected a directory", unpackPath)) - } + if isUnpacked, unpackTime, err := source.IsImageUnpacked(unpackPath); isUnpacked && err == nil { l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String()) - return successResult(unpackPath, canonicalRef, unpackStat.ModTime()), nil + return successResult(unpackPath, canonicalRef, unpackTime), nil + } else if err != nil { + return nil, fmt.Errorf("error checking image already unpacked: %w", err) } ////////////////////////////////////////////////////// @@ -148,7 +149,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, catalog *catalogdv // ////////////////////////////////////////////////////// if err := i.unpackImage(ctx, unpackPath, layoutRef, specIsCanonical, srcCtx); err != nil { - if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil { + if cleanupErr := source.DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil { err = errors.Join(err, cleanupErr) } return nil, fmt.Errorf("error unpacking image: %w", err) @@ -189,7 +190,7 @@ func successResult(unpackPath string, canonicalRef reference.Canonical, lastUnpa } func (i *ContainersImageRegistry) Cleanup(_ context.Context, catalog *catalogdv1.ClusterCatalog) error { - if err := deleteRecursive(i.catalogPath(catalog.Name)); err != nil { + if err := source.DeleteReadOnlyRecursive(i.catalogPath(catalog.Name)); err != nil { return fmt.Errorf("error deleting catalog cache: %w", err) } return nil @@ -288,8 +289,8 @@ 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 := os.MkdirAll(unpackPath, 0700); err != nil { - return fmt.Errorf("error creating unpack directory: %w", err) + if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil { + return fmt.Errorf("error ensuring empty unpack directory: %w", err) } l := log.FromContext(ctx) l.Info("unpacking image", "path", unpackPath) @@ -307,10 +308,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st l.Info("applied layer", "layer", i) return nil }(); err != nil { - return errors.Join(err, deleteRecursive(unpackPath)) + return errors.Join(err, source.DeleteReadOnlyRecursive(unpackPath)) } } - if err := setReadOnlyRecursive(unpackPath); err != nil { + if err := source.SetReadOnlyRecursive(unpackPath); err != nil { return fmt.Errorf("error making unpack directory read-only: %w", err) } return nil @@ -354,69 +355,13 @@ func (i *ContainersImageRegistry) deleteOtherImages(catalogName string, digestTo continue } imgDirPath := filepath.Join(catalogPath, imgDir.Name()) - if err := deleteRecursive(imgDirPath); err != nil { + if err := source.DeleteReadOnlyRecursive(imgDirPath); err != nil { return fmt.Errorf("error removing image directory: %w", err) } } return nil } -func setReadOnlyRecursive(root string) error { - if err := 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 - } - - 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 - }); err != nil { - return fmt.Errorf("error making catalog cache read-only: %w", err) - } - return nil -} - -func deleteRecursive(root string) error { - if err := 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 - }); err != nil { - return fmt.Errorf("error making catalog cache writable for deletion: %w", err) - } - return os.RemoveAll(root) -} - func wrapTerminal(err error, isTerminal bool) error { if !isTerminal { return err diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index b7b8551a4..3bda706aa 100644 --- a/cmd/operator-controller/main.go +++ b/cmd/operator-controller/main.go @@ -68,6 +68,7 @@ import ( "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" ) @@ -297,6 +298,11 @@ func main() { } } + if err := util.EnsureEmptyDirectory(cachePath, 0700); err != nil { + setupLog.Error(err, "unable to ensure empty cache directory") + os.Exit(1) + } + unpacker := &source.ContainersImageRegistry{ BaseCachePath: filepath.Join(cachePath, "unpack"), SourceContextFunc: func(logger logr.Logger) (*types.SystemContext, error) { @@ -361,7 +367,7 @@ func main() { crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()), } - applier := &applier.Helm{ + helmApplier := &applier.Helm{ ActionClientGetter: acg, Preflights: preflights, } @@ -381,7 +387,7 @@ func main() { Client: cl, Resolver: resolver, Unpacker: unpacker, - Applier: applier, + Applier: helmApplier, InstalledBundleGetter: &controllers.DefaultInstalledBundleGetter{ActionClientGetter: acg}, Finalizers: clusterExtensionFinalizers, Manager: cm, diff --git a/internal/rukpak/source/containers_image.go b/internal/rukpak/source/containers_image.go index 22f072da2..12a822a2b 100644 --- a/internal/rukpak/source/containers_image.go +++ b/internal/rukpak/source/containers_image.go @@ -23,6 +23,8 @@ import ( "github.com/opencontainers/go-digest" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/reconcile" + + "github.com/operator-framework/operator-controller/internal/util" ) type ContainersImageRegistry struct { @@ -62,12 +64,11 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour // ////////////////////////////////////////////////////// unpackPath := i.unpackPath(bundle.Name, canonicalRef.Digest()) - if unpackStat, err := os.Stat(unpackPath); err == nil { - if !unpackStat.IsDir() { - panic(fmt.Sprintf("unexpected file at unpack path %q: expected a directory", unpackPath)) - } + if isUnpacked, _, err := IsImageUnpacked(unpackPath); isUnpacked && err == nil { l.Info("image already unpacked", "ref", imgRef.String(), "digest", canonicalRef.Digest().String()) return successResult(bundle.Name, unpackPath, canonicalRef), nil + } else if err != nil { + return nil, fmt.Errorf("error checking bundle already unpacked: %w", err) } ////////////////////////////////////////////////////// @@ -140,7 +141,7 @@ func (i *ContainersImageRegistry) Unpack(ctx context.Context, bundle *BundleSour // ////////////////////////////////////////////////////// if err := i.unpackImage(ctx, unpackPath, layoutRef, srcCtx); err != nil { - if cleanupErr := deleteRecursive(unpackPath); cleanupErr != nil { + if cleanupErr := DeleteReadOnlyRecursive(unpackPath); cleanupErr != nil { err = errors.Join(err, cleanupErr) } return nil, fmt.Errorf("error unpacking image: %w", err) @@ -168,7 +169,7 @@ func successResult(bundleName, unpackPath string, canonicalRef reference.Canonic } func (i *ContainersImageRegistry) Cleanup(_ context.Context, bundle *BundleSource) error { - return deleteRecursive(i.bundlePath(bundle.Name)) + return DeleteReadOnlyRecursive(i.bundlePath(bundle.Name)) } func (i *ContainersImageRegistry) bundlePath(bundleName string) string { @@ -251,8 +252,8 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st return fmt.Errorf("error creating image source: %w", err) } - if err := os.MkdirAll(unpackPath, 0700); err != nil { - return fmt.Errorf("error creating unpack directory: %w", err) + if err := util.EnsureEmptyDirectory(unpackPath, 0700); err != nil { + return fmt.Errorf("error ensuring empty unpack directory: %w", err) } l := log.FromContext(ctx) l.Info("unpacking image", "path", unpackPath) @@ -270,10 +271,10 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st l.Info("applied layer", "layer", i) return nil }(); err != nil { - return errors.Join(err, deleteRecursive(unpackPath)) + return errors.Join(err, DeleteReadOnlyRecursive(unpackPath)) } } - if err := setReadOnlyRecursive(unpackPath); err != nil { + if err := SetReadOnlyRecursive(unpackPath); err != nil { return fmt.Errorf("error making unpack directory read-only: %w", err) } return nil @@ -310,65 +311,9 @@ func (i *ContainersImageRegistry) deleteOtherImages(bundleName string, digestToK continue } imgDirPath := filepath.Join(bundlePath, imgDir.Name()) - if err := deleteRecursive(imgDirPath); err != nil { + if err := DeleteReadOnlyRecursive(imgDirPath); err != nil { return fmt.Errorf("error removing image directory: %w", err) } } return nil } - -func setReadOnlyRecursive(root string) error { - if err := 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 - } - - 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 - }); err != nil { - return fmt.Errorf("error making bundle cache read-only: %w", err) - } - return nil -} - -func deleteRecursive(root string) error { - if err := 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 - }); err != nil { - return fmt.Errorf("error making bundle cache writable for deletion: %w", err) - } - return os.RemoveAll(root) -} diff --git a/internal/rukpak/source/containers_image_test.go b/internal/rukpak/source/containers_image_test.go index ea7a69832..29f2788c6 100644 --- a/internal/rukpak/source/containers_image_test.go +++ b/internal/rukpak/source/containers_image_test.go @@ -277,7 +277,16 @@ func TestUnpackUnexpectedFile(t *testing.T) { require.NoError(t, os.WriteFile(unpackPath, []byte{}, 0600)) // Attempt to pull and unpack the image - assert.Panics(t, func() { _, _ = unpacker.Unpack(context.Background(), bundleSource) }) + _, err := unpacker.Unpack(context.Background(), bundleSource) + require.NoError(t, err) + + // Ensure unpack path is now a directory + stat, err := os.Stat(unpackPath) + require.NoError(t, err) + require.True(t, stat.IsDir()) + + // Unset read-only to allow cleanup + require.NoError(t, source.UnsetReadOnlyRecursive(unpackPath)) } func TestUnpackCopySucceedsMountFails(t *testing.T) { diff --git a/internal/rukpak/source/util.go b/internal/rukpak/source/util.go new file mode 100644 index 000000000..ca9aa9c2b --- /dev/null +++ b/internal/rukpak/source/util.go @@ -0,0 +1,86 @@ +package source + +import ( + "errors" + "fmt" + "os" + "path/filepath" + "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 + } + + 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 + }) +} + +// 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 + }) +} + +// DeleteReadOnlyRecursive deletes read-only directory with path given by `root` +func DeleteReadOnlyRecursive(root string) error { + if err := UnsetReadOnlyRecursive(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 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 +} diff --git a/internal/util/fs.go b/internal/util/fs.go new file mode 100644 index 000000000..137b0735d --- /dev/null +++ b/internal/util/fs.go @@ -0,0 +1,23 @@ +package util + +import ( + "io/fs" + "os" + "path/filepath" +) + +// 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`. +func EnsureEmptyDirectory(path string, perm fs.FileMode) error { + entries, err := os.ReadDir(path) + if err != nil && !os.IsNotExist(err) { + return err + } + for _, entry := range entries { + if err := os.RemoveAll(filepath.Join(path, entry.Name())); err != nil { + return err + } + } + return os.MkdirAll(path, perm) +} From dbe0d0f8ae4a5dafafdbec2181590b91d59b7936 Mon Sep 17 00:00:00 2001 From: Per Goncalves da Silva Date: Fri, 31 Jan 2025 18:47:45 +0100 Subject: [PATCH 2/3] UPSTREAM: : :seedling: Rename util packages and add missing unit tests (#1677) * Rename util package name and file Signed-off-by: Per Goncalves da Silva * Refactor and add missing unit tests Signed-off-by: Per Goncalves da Silva --------- Signed-off-by: Per Goncalves da Silva Co-authored-by: Per Goncalves da Silva --- catalogd/cmd/catalogd/main.go | 4 +- catalogd/internal/source/containers_image.go | 4 +- cmd/operator-controller/main.go | 4 +- internal/{util/fs.go => fsutil/helpers.go} | 6 +- internal/fsutil/helpers_test.go | 47 ++++++ internal/rukpak/source/containers_image.go | 4 +- .../rukpak/source/containers_image_test.go | 2 +- internal/rukpak/source/helpers.go | 80 ++++++++++ internal/rukpak/source/helpers_test.go | 142 ++++++++++++++++++ internal/rukpak/source/util.go | 86 ----------- 10 files changed, 282 insertions(+), 97 deletions(-) rename internal/{util/fs.go => fsutil/helpers.go} (65%) create mode 100644 internal/fsutil/helpers_test.go create mode 100644 internal/rukpak/source/helpers.go create mode 100644 internal/rukpak/source/helpers_test.go delete mode 100644 internal/rukpak/source/util.go diff --git a/catalogd/cmd/catalogd/main.go b/catalogd/cmd/catalogd/main.go index 45baed165..f149728c2 100644 --- a/catalogd/cmd/catalogd/main.go +++ b/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) } diff --git a/catalogd/internal/source/containers_image.go b/catalogd/internal/source/containers_image.go index d67221efc..ccf3e62af 100644 --- a/catalogd/internal/source/containers_image.go +++ b/catalogd/internal/source/containers_image.go @@ -29,8 +29,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" @@ -289,7 +289,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) diff --git a/cmd/operator-controller/main.go b/cmd/operator-controller/main.go index 3bda706aa..75ae54a02 100644 --- a/cmd/operator-controller/main.go +++ b/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" ) @@ -298,7 +298,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) } diff --git a/internal/util/fs.go b/internal/fsutil/helpers.go similarity index 65% rename from internal/util/fs.go rename to internal/fsutil/helpers.go index 137b0735d..55accac46 100644 --- a/internal/util/fs.go +++ b/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) { diff --git a/internal/fsutil/helpers_test.go b/internal/fsutil/helpers_test.go new file mode 100644 index 000000000..b6fda0b30 --- /dev/null +++ b/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()) +} diff --git a/internal/rukpak/source/containers_image.go b/internal/rukpak/source/containers_image.go index 12a822a2b..3fd7b7d6d 100644 --- a/internal/rukpak/source/containers_image.go +++ b/internal/rukpak/source/containers_image.go @@ -24,7 +24,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" ) type ContainersImageRegistry struct { @@ -252,7 +252,7 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st return fmt.Errorf("error creating image source: %w", err) } - 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) diff --git a/internal/rukpak/source/containers_image_test.go b/internal/rukpak/source/containers_image_test.go index 29f2788c6..ab1abbb9b 100644 --- a/internal/rukpak/source/containers_image_test.go +++ b/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) { diff --git a/internal/rukpak/source/helpers.go b/internal/rukpak/source/helpers.go new file mode 100644 index 000000000..6e87dfb87 --- /dev/null +++ b/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()) + } + }) +} diff --git a/internal/rukpak/source/helpers_test.go b/internal/rukpak/source/helpers_test.go new file mode 100644 index 000000000..a4da1e629 --- /dev/null +++ b/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) +} diff --git a/internal/rukpak/source/util.go b/internal/rukpak/source/util.go deleted file mode 100644 index ca9aa9c2b..000000000 --- a/internal/rukpak/source/util.go +++ /dev/null @@ -1,86 +0,0 @@ -package source - -import ( - "errors" - "fmt" - "os" - "path/filepath" - "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 - } - - 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 - }) -} - -// 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 - }) -} - -// DeleteReadOnlyRecursive deletes read-only directory with path given by `root` -func DeleteReadOnlyRecursive(root string) error { - if err := UnsetReadOnlyRecursive(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 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 -} From eb54466926ad8cfc4f38c4cb21a0cf7129fb2c76 Mon Sep 17 00:00:00 2001 From: Joe Lanford Date: Mon, 3 Feb 2025 12:02:33 -0500 Subject: [PATCH 3/3] UPSTREAM: : EnsureEmptyDirectory should recursively set writable perms prior to delete (#1691) Signed-off-by: Joe Lanford --- catalogd/internal/source/containers_image.go | 10 +- internal/fsutil/helpers.go | 57 ++++++++- internal/fsutil/helpers_test.go | 110 ++++++++++++++++-- internal/rukpak/source/containers_image.go | 10 +- .../rukpak/source/containers_image_test.go | 3 +- internal/rukpak/source/helpers.go | 56 --------- internal/rukpak/source/helpers_test.go | 99 +--------------- 7 files changed, 172 insertions(+), 173 deletions(-) diff --git a/catalogd/internal/source/containers_image.go b/catalogd/internal/source/containers_image.go index ccf3e62af..030be2f53 100644 --- a/catalogd/internal/source/containers_image.go +++ b/catalogd/internal/source/containers_image.go @@ -149,7 +149,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) @@ -190,7 +190,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 @@ -308,10 +308,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 @@ -355,7 +355,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) } } diff --git a/internal/fsutil/helpers.go b/internal/fsutil/helpers.go index 55accac46..6f11ce1c2 100644 --- a/internal/fsutil/helpers.go +++ b/internal/fsutil/helpers.go @@ -1,6 +1,7 @@ package fsutil import ( + "fmt" "io/fs" "os" "path/filepath" @@ -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 { + 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()) + } + }) +} diff --git a/internal/fsutil/helpers_test.go b/internal/fsutil/helpers_test.go index b6fda0b30..4f397f2aa 100644 --- a/internal/fsutil/helpers_test.go +++ b/internal/fsutil/helpers_test.go @@ -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) { @@ -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) @@ -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) @@ -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) +} diff --git a/internal/rukpak/source/containers_image.go b/internal/rukpak/source/containers_image.go index 3fd7b7d6d..81b795b8c 100644 --- a/internal/rukpak/source/containers_image.go +++ b/internal/rukpak/source/containers_image.go @@ -141,7 +141,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) @@ -169,7 +169,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 { @@ -271,10 +271,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 @@ -311,7 +311,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) } } diff --git a/internal/rukpak/source/containers_image_test.go b/internal/rukpak/source/containers_image_test.go index ab1abbb9b..772053f1b 100644 --- a/internal/rukpak/source/containers_image_test.go +++ b/internal/rukpak/source/containers_image_test.go @@ -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" ) @@ -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) { diff --git a/internal/rukpak/source/helpers.go b/internal/rukpak/source/helpers.go index 6e87dfb87..32c8d42d4 100644 --- a/internal/rukpak/source/helpers.go +++ b/internal/rukpak/source/helpers.go @@ -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. @@ -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()) - } - }) -} diff --git a/internal/rukpak/source/helpers_test.go b/internal/rukpak/source/helpers_test.go index a4da1e629..de0106091 100644 --- a/internal/rukpak/source/helpers_test.go +++ b/internal/rukpak/source/helpers_test.go @@ -1,7 +1,6 @@ package source_test import ( - "io/fs" "os" "path/filepath" "testing" @@ -11,100 +10,6 @@ import ( "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") @@ -116,7 +21,7 @@ func TestIsImageUnpacked(t *testing.T) { require.True(t, modTime.IsZero()) t.Log("Test case: unpack path points to file") - require.NoError(t, os.WriteFile(unpackPath, []byte("test"), source.OwnerWritableFileMode)) + require.NoError(t, os.WriteFile(unpackPath, []byte("test"), 0600)) unpacked, modTime, err = source.IsImageUnpacked(filepath.Join(tempDir, "myimage")) require.NoError(t, err) @@ -128,7 +33,7 @@ func TestIsImageUnpacked(t *testing.T) { 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)) + require.NoError(t, os.Mkdir(unpackPath, 0700)) unpacked, modTime, err = source.IsImageUnpacked(unpackPath) require.NoError(t, err)