From a2d253d587799a3a586ebbedc883abe83887e0e5 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 9 Aug 2024 17:52:08 -0700 Subject: [PATCH 1/2] directory: simplify newImageDestination The current code is unnecessarily complicated: - it does unnecessary checks if a file/directory exists before opening it; - it uses three helper functions not used anywhere else; - directory contents is read twice. Untangle all this, making the code simpler. Also, move the logic of initializing a destination directory to a separate function. Now, since the logic of handing version file is now encapsulated in a single function, remove versionPath method and const version, as well as the corresponding test. This also fixes the subtle issue of using ref.path for constructing the version file path, but ref.resolvedPath for all other operations. Signed-off-by: Kir Kolyshkin --- directory/directory_dest.go | 145 +++++++++++--------------- directory/directory_transport.go | 5 - directory/directory_transport_test.go | 7 -- 3 files changed, 60 insertions(+), 97 deletions(-) diff --git a/directory/directory_dest.go b/directory/directory_dest.go index c9b3903185..dacd78c591 100644 --- a/directory/directory_dest.go +++ b/directory/directory_dest.go @@ -1,6 +1,7 @@ package directory import ( + "bytes" "context" "errors" "fmt" @@ -15,13 +16,10 @@ import ( "github.com/containers/image/v5/internal/putblobdigest" "github.com/containers/image/v5/internal/signature" "github.com/containers/image/v5/types" - "github.com/containers/storage/pkg/fileutils" "github.com/opencontainers/go-digest" "github.com/sirupsen/logrus" ) -const version = "Directory Transport Version: 1.1\n" - // ErrNotContainerImageDir indicates that the directory doesn't match the expected contents of a directory created // using the 'dir' transport var ErrNotContainerImageDir = errors.New("not a containers image directory, don't want to overwrite important data") @@ -51,52 +49,8 @@ func newImageDestination(sys *types.SystemContext, ref dirReference) (private.Im } } - // If directory exists check if it is empty - // if not empty, check whether the contents match that of a container image directory and overwrite the contents - // if the contents don't match throw an error - dirExists, err := pathExists(ref.resolvedPath) - if err != nil { - return nil, fmt.Errorf("checking for path %q: %w", ref.resolvedPath, err) - } - if dirExists { - isEmpty, err := isDirEmpty(ref.resolvedPath) - if err != nil { - return nil, err - } - - if !isEmpty { - versionExists, err := pathExists(ref.versionPath()) - if err != nil { - return nil, fmt.Errorf("checking if path exists %q: %w", ref.versionPath(), err) - } - if versionExists { - contents, err := os.ReadFile(ref.versionPath()) - if err != nil { - return nil, err - } - // check if contents of version file is what we expect it to be - if string(contents) != version { - return nil, ErrNotContainerImageDir - } - } else { - return nil, ErrNotContainerImageDir - } - // delete directory contents so that only one image is in the directory at a time - if err = removeDirContents(ref.resolvedPath); err != nil { - return nil, fmt.Errorf("erasing contents in %q: %w", ref.resolvedPath, err) - } - logrus.Debugf("overwriting existing container image directory %q", ref.resolvedPath) - } - } else { - // create directory if it doesn't exist - if err := os.MkdirAll(ref.resolvedPath, 0755); err != nil { - return nil, fmt.Errorf("unable to create directory %q: %w", ref.resolvedPath, err) - } - } - // create version file - err = os.WriteFile(ref.versionPath(), []byte(version), 0644) - if err != nil { - return nil, fmt.Errorf("creating version file %q: %w", ref.versionPath(), err) + if err := initDestDir(ref.resolvedPath); err != nil { + return nil, err } d := &dirImageDestination{ @@ -116,6 +70,63 @@ func newImageDestination(sys *types.SystemContext, ref dirReference) (private.Im return d, nil } +func initDestDir(dir string) error { + const versionContents = "Directory Transport Version: 1.1\n" + versionFile := filepath.Join(dir, "version") + + // If dir exists, checks if it is empty. + // If not empty, check whether the contents match that of a container + // image directory and overwrite the contents. + // If the contents don't match, throw ErrNotContainerImageDir. + fd, err := os.Open(dir) + switch { + case err == nil: // Directory exists. + contents, err := fd.Readdirnames(-1) + _ = fd.Close() + if err != nil { // Unexpected error. + return fmt.Errorf("%q: %w", dir, err) + } + if len(contents) == 0 { + break + } + // Check if contents of version file is what we expect it to be. + ver, err := os.ReadFile(versionFile) + if err != nil { + if errors.Is(err, os.ErrNotExist) { + return ErrNotContainerImageDir + } + return fmt.Errorf("%q: %w", versionFile, err) + } + if !bytes.Equal(ver, []byte(versionContents)) { + return ErrNotContainerImageDir + } + // Indeed an image directory. Reuse by removing all the old contents + // (including the versionFile, which will be recreated below). + logrus.Debugf("overwriting existing container image directory %q", dir) + for _, name := range contents { + rm := filepath.Join(dir, name) + err := os.RemoveAll(rm) + if err != nil { + return fmt.Errorf("%q: %w", rm, err) + } + } + case errors.Is(err, os.ErrNotExist): // Directory does not exist; create it. + if err := os.MkdirAll(dir, 0o755); err != nil { + return fmt.Errorf("%q: %w", dir, err) + } + default: + // Unexpected error. + return fmt.Errorf("%q: %w", dir, err) + } + + err = os.WriteFile(versionFile, []byte(versionContents), 0o644) + if err != nil { + return fmt.Errorf("%q: %w", versionFile, err) + } + + return nil +} + // Reference returns the reference used to set up this destination. Note that this should directly correspond to user's intent, // e.g. it should use the public hostname instead of the result of resolving CNAMEs or following redirects. func (d *dirImageDestination) Reference() types.ImageReference { @@ -261,39 +272,3 @@ func (d *dirImageDestination) PutSignaturesWithFormat(ctx context.Context, signa func (d *dirImageDestination) Commit(context.Context, types.UnparsedImage) error { return nil } - -// returns true if path exists -func pathExists(path string) (bool, error) { - err := fileutils.Exists(path) - if err == nil { - return true, nil - } - if os.IsNotExist(err) { - return false, nil - } - return false, err -} - -// returns true if directory is empty -func isDirEmpty(path string) (bool, error) { - files, err := os.ReadDir(path) - if err != nil { - return false, err - } - return len(files) == 0, nil -} - -// deletes the contents of a directory -func removeDirContents(path string) error { - files, err := os.ReadDir(path) - if err != nil { - return err - } - - for _, file := range files { - if err := os.RemoveAll(filepath.Join(path, file.Name())); err != nil { - return err - } - } - return nil -} diff --git a/directory/directory_transport.go b/directory/directory_transport.go index 4f7d596b44..6898d31be8 100644 --- a/directory/directory_transport.go +++ b/directory/directory_transport.go @@ -190,8 +190,3 @@ func (ref dirReference) signaturePath(index int, instanceDigest *digest.Digest) } return filepath.Join(ref.path, fmt.Sprintf("signature-%d", index+1)), nil } - -// versionPath returns a path for the version file within a directory using our conventions. -func (ref dirReference) versionPath() string { - return filepath.Join(ref.path, "version") -} diff --git a/directory/directory_transport_test.go b/directory/directory_transport_test.go index 9b6d63b99e..025cfc2431 100644 --- a/directory/directory_transport_test.go +++ b/directory/directory_transport_test.go @@ -243,10 +243,3 @@ func TestReferenceSignaturePath(t *testing.T) { _, err = dirRef.signaturePath(0, &invalidDigest) assert.Error(t, err) } - -func TestReferenceVersionPath(t *testing.T) { - ref, tmpDir := refToTempDir(t) - dirRef, ok := ref.(dirReference) - require.True(t, ok) - assert.Equal(t, tmpDir+"/version", dirRef.versionPath()) -} From e77668f3da38190a1f1ffeccd66ac7b3fc603a52 Mon Sep 17 00:00:00 2001 From: Kir Kolyshkin Date: Fri, 9 Aug 2024 17:50:14 -0700 Subject: [PATCH 2/2] directory: test case nit This needs to be "require" rather than "assert", as if error is returned, test needs to fail now. Otherwise, close will panic. Signed-off-by: Kir Kolyshkin --- directory/directory_transport_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/directory/directory_transport_test.go b/directory/directory_transport_test.go index 025cfc2431..01c0397617 100644 --- a/directory/directory_transport_test.go +++ b/directory/directory_transport_test.go @@ -181,7 +181,7 @@ func TestReferenceNewImageSource(t *testing.T) { func TestReferenceNewImageDestination(t *testing.T) { ref, _ := refToTempDir(t) dest, err := ref.NewImageDestination(context.Background(), nil) - assert.NoError(t, err) + require.NoError(t, err) defer dest.Close() }