Skip to content

Commit

Permalink
Clear cache on startup, use tempDir for unpacking
Browse files Browse the repository at this point in the history 
Signed-off-by: Per Goncalves da Silva <[email protected]>
  • Loading branch information
@oceanc80
oceanc80 authored and Per Goncalves da Silva committed Jan 30, 2025
1 parent 037b9e2 commit bc1db72
Show file tree
Hide file tree
Showing 6 changed files with 153 additions and 144 deletions.
5 changes: 3 additions & 2 deletions catalogd/cmd/catalogd/main.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"crypto/tls"
"flag"
"fmt"
"github.com/operator-framework/operator-controller/internal/util"
"log"
"net/url"
"os"
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
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"context"
"errors"
"fmt"
"github.com/operator-framework/operator-controller/internal/rukpak/source"
"github.com/operator-framework/operator-controller/internal/util"
"io"
"os"
"path"
Expand Down Expand Up @@ -76,12 +78,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 @@ -154,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 := 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 @@ -195,7 +196,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 @@ -296,8 +297,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 @@ -315,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, 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 @@ -362,69 +363,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
85 changes: 14 additions & 71 deletions internal/rukpak/source/containers_image.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@ import (
"context"
"errors"
"fmt"

Check failure on line 7 in internal/rukpak/source/containers_image.go

View workflow job for this annotation

GitHub Actions / lint 

File is not properly formatted (gci)
"io"
"os"
"path/filepath"

"github.com/containerd/containerd/archive"
"github.com/containers/image/v5/copy"
"github.com/containers/image/v5/docker"
Expand All @@ -22,6 +18,10 @@ import (
"github.com/containers/image/v5/types"
"github.com/go-logr/logr"
"github.com/opencontainers/go-digest"
"github.com/operator-framework/operator-controller/internal/util"

Check failure on line 21 in internal/rukpak/source/containers_image.go

View workflow job for this annotation

GitHub Actions / lint 

File is not properly formatted (gci)
"io"
"os"
"path/filepath"
"sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/reconcile"

Check failure on line 26 in internal/rukpak/source/containers_image.go

View workflow job for this annotation

GitHub Actions / lint 

File is not properly formatted (gci)
)
Expand Down Expand Up @@ -69,12 +69,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)
}

//////////////////////////////////////////////////////
Expand Down Expand Up @@ -147,7 +146,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 @@ -175,7 +174,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 @@ -265,8 +264,8 @@ func (i *ContainersImageRegistry) unpackImage(ctx context.Context, unpackPath st
}
}()

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 @@ -284,10 +283,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 @@ -324,65 +323,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)
}
17 changes: 14 additions & 3 deletions internal/rukpak/source/containers_image_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -256,8 +256,10 @@ func TestUnpackInvalidImageRef(t *testing.T) {
}

func TestUnpackUnexpectedFile(t *testing.T) {
imageTagRef, imageDigestRef, cleanup := setupRegistry(t)
defer cleanup()
imageTagRef, imageDigestRef, _ := setupRegistry(t)
//defer func() {
// // cleanup()
//}()

unpacker := &source.ContainersImageRegistry{
BaseCachePath: t.TempDir(),
Expand All @@ -277,7 +279,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)
assert.NoError(t, err)

Check failure on line 283 in internal/rukpak/source/containers_image_test.go

View workflow job for this annotation

GitHub Actions / lint 

require-error: for error assertions use require (testifylint)

// Ensure unpack path is now a directory
stat, err := os.Stat(unpackPath)
assert.NoError(t, err)

Check failure on line 287 in internal/rukpak/source/containers_image_test.go

View workflow job for this annotation

GitHub Actions / lint 

require-error: for error assertions use require (testifylint)
assert.True(t, stat.IsDir())

// Unset read-only to allow cleanup
assert.NoError(t, source.UnsetReadOnlyRecursive(unpackPath))
}

func TestUnpackCopySucceedsMountFails(t *testing.T) {
Expand Down
Loading

0 comments on commit bc1db72

Please sign in to comment.