Skip to content
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.

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

OCI layout extensions #2633

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 3 additions & 1 deletion docs/containers-transports.5.md
Original file line number Diff line number Diff line change
Expand Up @@ -71,13 +71,15 @@ An image stored in the docker daemon's internal storage.
The image must be specified as a _docker-reference_ or in an alternative _algo_`:`_digest_ format when being used as an image source.
The _algo_`:`_digest_ refers to the image ID reported by docker-inspect(1).

### **oci:**_path_[`:`_reference_]
### **oci:**_path_[`:`_reference_|`@`_source-index_}]_

An image in a directory structure compliant with the "Open Container Image Layout Specification" at _path_.

The _path_ value terminates at the first `:` character; any further `:` characters are not separators, but a part of _reference_.
The _reference_ is used to set, or match, the `org.opencontainers.image.ref.name` annotation in the top-level index.
If _reference_ is not specified when reading an image, the directory must contain exactly one image.
For reading images, @_source-index_ is a zero-based index in manifest (to access untagged images).
If neither reference nor @_source_index is specified when reading an image, the path must contain exactly one image.

### **oci-archive:**_path_[`:`_reference_]

Expand Down
30 changes: 30 additions & 0 deletions internal/reflink/reflink_linux.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
//go:build cgo

package reflink

/*
#include <linux/fs.h>

#ifndef FICLONE
#define FICLONE _IOW(0x94, 9, int)
Copy link
Collaborator

Choose a reason for hiding this comment

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

x/sys/unix.FICLONE exists; if that works, we can avoid the CGo dependency.

#endif
*/
import "C"
import (
"io"
"os"

"golang.org/x/sys/unix"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please run go mod tidy to mark the package as a direct dependency.

)

// Copy attempts to reflink the source to the destination fd.
// If reflinking fails or is unsupported, it falls back to io.Copy().
func Copy(src, dst *os.File) error {
_, _, errno := unix.Syscall(unix.SYS_IOCTL, dst.Fd(), C.FICLONE, src.Fd())
if errno == 0 {
return nil
}

_, err := io.Copy(dst, src)
return err
}
15 changes: 15 additions & 0 deletions internal/reflink/reflink_unsupported.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//go:build !linux || !cgo

package reflink

import (
"io"
"os"
)

// Copy attempts to reflink the source to the destination fd.
// If reflinking fails or is unsupported, it falls back to io.Copy().
func Copy(src, dst *os.File) error {
_, err := io.Copy(dst, src)
return err
}
29 changes: 29 additions & 0 deletions oci/internal/oci_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"path/filepath"
"regexp"
"runtime"
"strconv"
"strings"
)

Expand Down Expand Up @@ -119,3 +120,31 @@ func validateScopeNonWindows(scope string) error {

return nil
}

// parseOCIReferenceName parses the image from the oci reference.
func parseOCIReferenceName(image string) (img string, index int, err error) {
index = -1
if strings.HasPrefix(image, "@") {
idx, err := strconv.Atoi(image[1:])
if err != nil {
return "", index, fmt.Errorf("Invalid source index @%s: not an integer: %w", image[1:], err)
}
if idx < 0 {
return "", index, fmt.Errorf("Invalid source index @%d: must not be negative", idx)
}
index = idx
} else {
img = image
}
return img, index, nil
}

// ParseReferenceIntoElements splits the oci reference into location, image name and source index if exists
func ParseReferenceIntoElements(reference string) (string, string, int, error) {
vrothberg marked this conversation as resolved.
Show resolved Hide resolved
dir, image := SplitPathAndImage(reference)
image, index, err := parseOCIReferenceName(image)
if err != nil {
return "", "", -1, err
}
return dir, image, index, nil
}
28 changes: 28 additions & 0 deletions oci/internal/oci_util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ type testDataScopeValidation struct {
errMessage string
}

type testOCIReference struct {
ref string
image string
index int
}

func TestSplitReferenceIntoDirAndImageWindows(t *testing.T) {
tests := []testDataSplitReference{
{`C:\foo\bar:busybox:latest`, `C:\foo\bar`, "busybox:latest"},
Expand Down Expand Up @@ -61,3 +67,25 @@ func TestValidateScopeWindows(t *testing.T) {
}
}
}

func TestParseOCIReferenceName(t *testing.T) {
validTests := []testOCIReference{
{"@0", "", 0},
{"notlatest@1", "notlatest@1", -1},
}
for _, test := range validTests {
img, idx, err := parseOCIReferenceName(test.ref)
assert.NoError(t, err)
assert.Equal(t, img, test.image)
assert.Equal(t, idx, test.index)
}

invalidTests := []string{
"@-5",
"@invalidIndex",
}
for _, test := range invalidTests {
_, _, err := parseOCIReferenceName(test)
assert.Error(t, err)
}
}
1 change: 1 addition & 0 deletions oci/layout/fixtures/files/a.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
aaaaaaaaaaaaaaaaaaaaaaaaaaaaaa
1 change: 1 addition & 0 deletions oci/layout/fixtures/files/b.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
bbbbbbbbbbbbbbbbbbbbbbbb
89 changes: 80 additions & 9 deletions oci/layout/oci_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import (
"github.com/containers/image/v5/internal/manifest"
"github.com/containers/image/v5/internal/private"
"github.com/containers/image/v5/internal/putblobdigest"
"github.com/containers/image/v5/internal/reflink"
"github.com/containers/image/v5/types"
"github.com/containers/storage/pkg/fileutils"
digest "github.com/opencontainers/go-digest"
Expand All @@ -38,6 +39,9 @@ type ociImageDestination struct {

// newImageDestination returns an ImageDestination for writing to an existing directory.
func newImageDestination(sys *types.SystemContext, ref ociReference) (private.ImageDestination, error) {
if ref.sourceIndex != -1 {
return nil, fmt.Errorf("Destination reference must not contain a manifest index @%d", ref.sourceIndex)
}
var index *imgspecv1.Index
if indexExists(ref) {
var err error
Expand Down Expand Up @@ -138,36 +142,46 @@ func (d *ociImageDestination) PutBlobWithOptions(ctx context.Context, stream io.
if inputInfo.Size != -1 && size != inputInfo.Size {
return private.UploadedBlob{}, fmt.Errorf("Size mismatch when copying %s, expected %d, got %d", blobDigest, inputInfo.Size, size)
}
if err := blobFile.Sync(); err != nil {

if err := d.blobFileSyncAndRename(blobFile, blobDigest, &explicitClosed); err != nil {
return private.UploadedBlob{}, err
}
succeeded = true
return private.UploadedBlob{Digest: blobDigest, Size: size}, nil
}

func (d *ociImageDestination) blobFileSyncAndRename(blobFile *os.File, blobDigest digest.Digest, closed *bool) error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Documenting the interaction of blobFile and *closed would be nice.

if err := blobFile.Sync(); err != nil {
return err
}

// On POSIX systems, blobFile was created with mode 0600, so we need to make it readable.
// On Windows, the “permissions of newly created files” argument to syscall.Open is
// ignored and the file is already readable; besides, blobFile.Chmod, i.e. syscall.Fchmod,
// always fails on Windows.
if runtime.GOOS != "windows" {
if err := blobFile.Chmod(0644); err != nil {
return private.UploadedBlob{}, err
return err
}
}

blobPath, err := d.ref.blobPath(blobDigest, d.sharedBlobDir)
if err != nil {
return private.UploadedBlob{}, err
return err
}
if err := ensureParentDirectoryExists(blobPath); err != nil {
return private.UploadedBlob{}, err
return err
}

// need to explicitly close the file, since a rename won't otherwise not work on Windows
// need to explicitly close the file, since a rename won't otherwise work on Windows
blobFile.Close()
explicitClosed = true

*closed = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Nit: Move this one line above, please, to keep it immediately next to the actual .Close().)

if err := os.Rename(blobFile.Name(), blobPath); err != nil {
return private.UploadedBlob{}, err
return err
}
succeeded = true
return private.UploadedBlob{Digest: blobDigest, Size: size}, nil

return nil
}

// TryReusingBlobWithOptions checks whether the transport already contains, or can efficiently reuse, a blob, and if so, applies it to the current destination
Expand Down Expand Up @@ -300,6 +314,63 @@ func (d *ociImageDestination) CommitWithOptions(ctx context.Context, options pri
return os.WriteFile(d.ref.indexPath(), indexJSON, 0644)
}

// PutBlobFromLocalFileOptions is unused but may receive functionality in the future.
type PutBlobFromLocalFileOptions struct{}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type PutBlobFromLocalFileOptions struct{}
type PutBlobFromLocalFileOption struct{}

please, multiple options can be independent items at the call site.


// PutBlobFromLocalFile arranges the data from path to be used as blob with digest.
// It computes, and returns, the digest and size of the used file.
//
// This function can be used instead of dest.PutBlob() where the ImageDestination requires PutBlob() to be called.
func PutBlobFromLocalFile(ctx context.Context, dest types.ImageDestination, file string, options ...PutBlobFromLocalFileOptions) (digest.Digest, int64, error) {
d, ok := dest.(*ociImageDestination)
if !ok {
return "", -1, errors.New("internal error: PutBlobFromLocalFile called with a non-oci: destination")
}

blobFileClosed := false
blobFile, err := os.CreateTemp(d.ref.dir, "oci-put-blob")
if err != nil {
return "", -1, err
}
defer func() {
if !blobFileClosed {
blobFile.Close()
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

blobFile remains on the filesystem on failure; I think this needs the succeeded logic from PutBlobWithOptions.

}()

srcFile, err := os.Open(file)
if err != nil {
return "", -1, err
}
defer srcFile.Close()

// reflink.Copy will io.Copy() in case reflinking fails
err = reflink.Copy(srcFile, blobFile)
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Absolutely non-blocking, this is internal-only anyway, and it works just fine as is: Maybe the function could be named explicitly enough to make the comment unnecessary, something like ReflinkOrCopy. OTOH some linters gripe about reflink.Reflink….)

if err != nil {
return "", -1, err
}

_, err = blobFile.Seek(0, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
_, err = blobFile.Seek(0, 0)
_, err = blobFile.Seek(0, io.SeekStart)

please

if err != nil {
return "", -1, err
}
blobDigest, err := digest.FromReader(blobFile)
if err != nil {
return "", -1, err
}

fileInfo, err := blobFile.Stat()
if err != nil {
return "", -1, err
}

if err := d.blobFileSyncAndRename(blobFile, blobDigest, &blobFileClosed); err != nil {
return "", -1, err
}

return blobDigest, fileInfo.Size(), nil
}

func ensureDirectoryExists(path string) error {
if err := fileutils.Exists(path); err != nil && errors.Is(err, fs.ErrNotExist) {
if err := os.MkdirAll(path, 0755); err != nil {
Expand Down
46 changes: 42 additions & 4 deletions oci/layout/oci_dest_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ func TestPutBlobDigestFailure(t *testing.T) {
const digestErrorString = "Simulated digest error"
const blobDigest = "sha256:e692418e4cbaf90ca69d05a66403747baa33ee08806650b51fab815ad7fc331f"

ref, _ := refToTempOCI(t)
ref, _ := refToTempOCI(t, false)
dirRef, ok := ref.(ociReference)
require.True(t, ok)
blobPath, err := dirRef.blobPath(blobDigest, "")
Expand Down Expand Up @@ -71,7 +71,7 @@ func TestPutBlobDigestFailure(t *testing.T) {

// TestPutManifestAppendsToExistingManifest tests that new manifests are getting added to existing index.
func TestPutManifestAppendsToExistingManifest(t *testing.T) {
ref, tmpDir := refToTempOCI(t)
ref, tmpDir := refToTempOCI(t, false)

ociRef, ok := ref.(ociReference)
require.True(t, ok)
Expand All @@ -94,7 +94,7 @@ func TestPutManifestAppendsToExistingManifest(t *testing.T) {

// TestPutManifestTwice tests that existing manifest gets updated and not appended.
func TestPutManifestTwice(t *testing.T) {
ref, tmpDir := refToTempOCI(t)
ref, tmpDir := refToTempOCI(t, false)

ociRef, ok := ref.(ociReference)
require.True(t, ok)
Expand All @@ -109,7 +109,7 @@ func TestPutManifestTwice(t *testing.T) {
}

func TestPutTwoDifferentTags(t *testing.T) {
ref, tmpDir := refToTempOCI(t)
ref, tmpDir := refToTempOCI(t, false)

ociRef, ok := ref.(ociReference)
require.True(t, ok)
Expand Down Expand Up @@ -178,3 +178,41 @@ func putTestManifest(t *testing.T, ociRef ociReference, tmpDir string) {
digest := digest.FromBytes(data).Encoded()
assert.Contains(t, paths, filepath.Join(tmpDir, "blobs", "sha256", digest), "The OCI directory does not contain the new manifest data")
}

func TestPutblobFromLocalFile(t *testing.T) {
ref, _ := refToTempOCI(t, false)
dest, err := ref.NewImageDestination(context.Background(), nil)
require.NoError(t, err)
defer dest.Close()
ociDest, ok := dest.(*ociImageDestination)
require.True(t, ok)

for _, test := range []struct {
path string
size int64
digest string
}{
{path: "fixtures/files/a.txt", size: 31, digest: "sha256:c8a3f498ce6aaa13c803fa3a6a0d5fd6b5d75be5781f98f56c0f960efcc53174"},
{path: "fixtures/files/b.txt", size: 25, digest: "sha256:8c1e9b03116b95e6dfac68c588190d56bfc82b9cc550ada726e882e138a3b93b"},
{path: "fixtures/files/b.txt", size: 25, digest: "sha256:8c1e9b03116b95e6dfac68c588190d56bfc82b9cc550ada726e882e138a3b93b"}, // Must not fail
} {
digest, size, err := PutBlobFromLocalFile(context.Background(), dest, test.path)
require.NoError(t, err)
require.Equal(t, test.size, size)
require.Equal(t, test.digest, digest.String())

blobPath, err := ociDest.ref.blobPath(digest, ociDest.sharedBlobDir)
require.NoError(t, err)
require.FileExists(t, blobPath)

expectedContent, err := os.ReadFile(test.path)
require.NoError(t, err)
require.NotEmpty(t, expectedContent)
blobContent, err := os.ReadFile(blobPath)
require.NoError(t, err)
require.Equal(t, expectedContent, blobContent)
}

err = ociDest.CommitWithOptions(context.Background(), private.CommitOptions{})
require.NoError(t, err)
}
Loading
Loading