Skip to content

UPSTREAM: <drop> Ensure catalog caches are valid after restart #254

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

Closed
wants to merge 1 commit into from
Closed

UPSTREAM: <drop> Ensure catalog caches are valid after restart #254

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
5 changes: 3 additions & 2 deletions catalogd/cmd/catalogd/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to avoid introducing util and have the new functions located in a more meaningful place(s)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you have some suggestions? As always, naming is a PITA... I'm struggling to find a good name for a package that contains common shared utility functions used by various other packages in the project XD should we just call the package fs?

Copy link
Contributor

Choose a reason for hiding this comment

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

if the functions / utils deal with filesystem then fs makes sense as package name to me

)

var (
Expand Down Expand Up @@ -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)
}

Expand Down
81 changes: 13 additions & 68 deletions catalogd/internal/source/containers_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
}

//////////////////////////////////////////////////////
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand Down
10 changes: 8 additions & 2 deletions cmd/operator-controller/main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -361,7 +367,7 @@ func main() {
crdupgradesafety.NewPreflight(aeClient.CustomResourceDefinitions()),
}

applier := &applier.Helm{
helmApplier := &applier.Helm{
ActionClientGetter: acg,
Preflights: preflights,
}
Expand All @@ -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,
Expand Down
80 changes: 12 additions & 68 deletions internal/rukpak/source/containers_image.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -62,12 +64,10 @@ 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)
}

//////////////////////////////////////////////////////
Expand Down Expand Up @@ -140,7 +140,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)
Expand Down Expand Up @@ -168,7 +168,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 {
Expand Down Expand Up @@ -251,8 +251,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)
Expand All @@ -270,10 +270,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
Expand Down Expand Up @@ -310,65 +310,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)
}
11 changes: 10 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 @@ -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) {
Expand Down
Loading