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.

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

Jump to bottom

Add reader/writer for oci-archive multi image support #1381

Closed
wants to merge 1 commit into from
Closed

Add reader/writer for oci-archive multi image support #1381

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
9 changes: 7 additions & 2 deletions docs/containers-transports.5.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,14 +57,19 @@ 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 compliant with the "Open Container Image Layout Specification" at _path_.
Using a _reference_ is optional and allows for storing multiple images at the same _path_.
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]_
### **oci-archive:**_path[:{reference|@source-index}]_

An image compliant with the "Open Container Image Layout Specification" stored as a tar(1) archive at _path_.
Using a _reference_ is optional and allows for storing multiple images at the same _path_.
For reading archives, @_source-index_ is a zero-based index in archive manifest (to access untagged images).
If neither reference nor @_source_index is specified when reading an archive, the archive must contain exactly one image.

### **ostree:**_docker-reference[@/absolute/repo/path]_

Expand Down
91 changes: 41 additions & 50 deletions oci/archive/oci_dest.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,45 +3,62 @@ package archive
import (
"context"
"io"
"os"

"github.com/containers/image/v5/internal/blobinfocache"
"github.com/containers/image/v5/internal/imagedestination"
"github.com/containers/image/v5/internal/imagedestination/impl"
"github.com/containers/image/v5/internal/private"
"github.com/containers/image/v5/internal/signature"
"github.com/containers/image/v5/oci/layout"
"github.com/containers/image/v5/types"
"github.com/containers/storage/pkg/archive"
digest "github.com/opencontainers/go-digest"
perrors "github.com/pkg/errors"
"github.com/sirupsen/logrus"
)

type ociArchiveImageDestination struct {
impl.Compat

ref ociArchiveReference
unpackedDest private.ImageDestination
tempDirRef tempDirOCIRef
ref ociArchiveReference
individualWriterOrNil *Writer
unpackedDest private.ImageDestination
}

// newImageDestination returns an ImageDestination for writing to an existing directory.
func newImageDestination(ctx context.Context, sys *types.SystemContext, ref ociArchiveReference) (private.ImageDestination, error) {
tempDirRef, err := createOCIRef(sys, ref.image)
func newImageDestination(ctx context.Context, sys *types.SystemContext, ref ociArchiveReference) (types.ImageDestination, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should continue to return private.ID, not types.ID. (We do, ultimately, have a test for the return value implementing private.ID, but having it explicit here would be a bit more convenient.)

var (
archive, individualWriterOrNil *Writer
err error
)

if ref.sourceIndex != -1 {
return nil, perrors.Wrapf(invalidOciArchiveErr, "destination reference must not contain a manifest index @%d", ref.sourceIndex)
}

if ref.archiveWriter != nil {
archive = ref.archiveWriter
individualWriterOrNil = nil
} else {
archive, err = NewWriter(ctx, sys, ref.resolvedFile)
if err != nil {
return nil, err
}
individualWriterOrNil = archive
}
layoutRef, err := layout.NewReference(archive.tempDir, ref.image)
if err != nil {
return nil, perrors.Wrapf(err, "creating oci reference")
archive.Close()
return nil, err
}
unpackedDest, err := tempDirRef.ociRefExtracted.NewImageDestination(ctx, sys)
dst, err := layoutRef.NewImageDestination(ctx, sys)
if err != nil {
if err := tempDirRef.deleteTempDir(); err != nil {
return nil, perrors.Wrapf(err, "deleting temp directory %q", tempDirRef.tempDirectory)
}
archive.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should only close individualWriterOrNil if it is set, not the longer-term ref.archiveWriter.

(in both cases.)

return nil, err
}

d := &ociArchiveImageDestination{
ref: ref,
unpackedDest: imagedestination.FromPublic(unpackedDest),
tempDirRef: tempDirRef,
ref: ref,
umohnani8 marked this conversation as resolved.
Show resolved Hide resolved
individualWriterOrNil: individualWriterOrNil,
unpackedDest: imagedestination.FromPublic(dst),
}
d.Compat = impl.AddCompat(d)
return d, nil
Expand All @@ -53,13 +70,14 @@ func (d *ociArchiveImageDestination) Reference() types.ImageReference {
}

// Close removes resources associated with an initialized ImageDestination, if any
// Close deletes the temp directory of the oci-archive image
func (d *ociArchiveImageDestination) Close() error {
defer func() {
err := d.tempDirRef.deleteTempDir()
logrus.Debugf("Error deleting temporary directory: %v", err)
}()
return d.unpackedDest.Close()
if err := d.unpackedDest.Close(); err != nil {
return err
}
if d.individualWriterOrNil == nil {
return nil
}
return d.individualWriterOrNil.Close()
}

func (d *ociArchiveImageDestination) SupportedManifestMIMETypes() []string {
Expand Down Expand Up @@ -160,32 +178,5 @@ func (d *ociArchiveImageDestination) Commit(ctx context.Context, unparsedTopleve
if err := d.unpackedDest.Commit(ctx, unparsedToplevel); err != nil {
return perrors.Wrapf(err, "storing image %q", d.ref.image)
Copy link
Collaborator

Choose a reason for hiding this comment

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

pkg/errors are no longer used (throughout).

}

// path of directory to tar up
src := d.tempDirRef.tempDirectory
// path to save tarred up file
dst := d.ref.resolvedFile
return tarDirectory(src, dst)
}

// tar converts the directory at src and saves it to dst
func tarDirectory(src, dst string) error {
// input is a stream of bytes from the archive of the directory at path
input, err := archive.Tar(src, archive.Uncompressed)
if err != nil {
return perrors.Wrapf(err, "retrieving stream of bytes from %q", src)
}

// creates the tar file
outFile, err := os.Create(dst)
if err != nil {
return perrors.Wrapf(err, "creating tar file %q", dst)
}
defer outFile.Close()

// copies the contents of the directory to the tar file
// TODO: This can take quite some time, and should ideally be cancellable using a context.Context.
_, err = io.Copy(outFile, input)

return err
return nil
}
68 changes: 47 additions & 21 deletions oci/archive/oci_src.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/containers/image/v5/internal/imagesource/impl"
"github.com/containers/image/v5/internal/private"
"github.com/containers/image/v5/internal/signature"
"github.com/containers/image/v5/oci/layout"
ocilayout "github.com/containers/image/v5/oci/layout"
"github.com/containers/image/v5/types"
digest "github.com/opencontainers/go-digest"
Expand All @@ -20,30 +21,54 @@ import (
type ociArchiveImageSource struct {
impl.Compat

ref ociArchiveReference
unpackedSrc private.ImageSource
tempDirRef tempDirOCIRef
ref ociArchiveReference
unpackedSrc private.ImageSource
individualReaderOrNil *Reader
}

// newImageSource returns an ImageSource for reading from an existing directory.
// newImageSource untars the file and saves it in a temp directory
func newImageSource(ctx context.Context, sys *types.SystemContext, ref ociArchiveReference) (private.ImageSource, error) {
tempDirRef, err := createUntarTempDir(sys, ref)
if err != nil {
return nil, perrors.Wrap(err, "creating temp directory")
func newImageSource(ctx context.Context, sys *types.SystemContext, ref ociArchiveReference) (types.ImageSource, error) {
var (
archive, individualReaderOrNil *Reader
layoutRef types.ImageReference
err error
)

if ref.archiveReader != nil {
archive = ref.archiveReader
individualReaderOrNil = nil
} else {
archive, _, err = NewReaderForReference(ctx, sys, ref)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don’t see any benefit to calling NewReaderForReference if we are going to throw away that reference.

if err != nil {
return nil, err
}
individualReaderOrNil = archive
}

unpackedSrc, err := tempDirRef.ociRefExtracted.NewImageSource(ctx, sys)
if err != nil {
if err := tempDirRef.deleteTempDir(); err != nil {
umohnani8 marked this conversation as resolved.
Show resolved Hide resolved
return nil, perrors.Wrapf(err, "deleting temp directory %q", tempDirRef.tempDirectory)
if ref.sourceIndex != -1 {
layoutRef, err = layout.NewIndexReference(archive.tempDirectory, ref.sourceIndex)
if err != nil {
archive.Close()
return nil, err
}
} else {
layoutRef, err = layout.NewReference(archive.tempDirectory, ref.image)
if err != nil {
archive.Close()
Copy link
Collaborator

Choose a reason for hiding this comment

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

archive should only be closed if it is individualReaderOrNil. (in all cases.)

return nil, err
}
}

src, err := layoutRef.NewImageSource(ctx, sys)
if err != nil {
archive.Close()
return nil, err
}

s := &ociArchiveImageSource{
ref: ref,
unpackedSrc: imagesource.FromPublic(unpackedSrc),
tempDirRef: tempDirRef,
ref: ref,
unpackedSrc: imagesource.FromPublic(src),
individualReaderOrNil: individualReaderOrNil,
umohnani8 marked this conversation as resolved.
Show resolved Hide resolved
}
s.Compat = impl.AddCompat(s)
return s, nil
Expand Down Expand Up @@ -83,13 +108,14 @@ func (s *ociArchiveImageSource) Reference() types.ImageReference {
}

// Close removes resources associated with an initialized ImageSource, if any.
// Close deletes the temporary directory at dst
func (s *ociArchiveImageSource) Close() error {
umohnani8 marked this conversation as resolved.
Show resolved Hide resolved
defer func() {
err := s.tempDirRef.deleteTempDir()
logrus.Debugf("error deleting tmp dir: %v", err)
}()
return s.unpackedSrc.Close()
if err := s.unpackedSrc.Close(); err != nil {
return err
}
if s.individualReaderOrNil == nil {
return nil
}
return s.individualReaderOrNil.Close()
}

// GetManifest returns the image's manifest along with its MIME type (which may be empty when it can't be determined but the manifest is available).
Expand Down
68 changes: 54 additions & 14 deletions oci/archive/oci_transport.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"errors"
"fmt"
"io/ioutil"
"os"
"strings"

Expand All @@ -23,6 +24,8 @@ func init() {
transports.Register(Transport)
}

var invalidOciArchiveErr error = errors.New("Invalid oci-archive: reference")

// Transport is an ImageTransport for OCI archive
// it creates an oci-archive tar file by calling into the OCI transport
// tarring the directory created by oci and deleting the directory
Expand All @@ -32,9 +35,12 @@ type ociArchiveTransport struct{}

// ociArchiveReference is an ImageReference for OCI Archive paths
type ociArchiveReference struct {
file string
resolvedFile string
image string
file string
resolvedFile string
image string
sourceIndex int
archiveReader *Reader
archiveWriter *Writer
}

func (t ociArchiveTransport) Name() string {
Expand All @@ -54,12 +60,24 @@ func (t ociArchiveTransport) ValidatePolicyConfigurationScope(scope string) erro

// ParseReference converts a string, which should not start with the ImageTransport.Name prefix, into an OCI ImageReference.
func ParseReference(reference string) (types.ImageReference, error) {
file, image := internal.SplitPathAndImage(reference)
return NewReference(file, image)
file, image, index, err := internal.ParseReferenceIntoElements(reference)
if err != nil {
return nil, err
}
return newReference(file, image, index, nil, nil)
}

// NewReference returns an OCI reference for a file and a image.
// NewReference returns an OCI reference for a file and an image.
func NewReference(file, image string) (types.ImageReference, error) {
return newReference(file, image, -1, nil, nil)
}

// NewIndexReference returns an OCI reference for a file and a zero-based source manifest index.
func NewIndexReference(file string, sourceIndex int) (types.ImageReference, error) {
return newReference(file, "", sourceIndex, nil, nil)
}

func newReference(file, image string, sourceIndex int, archiveReader *Reader, archiveWriter *Writer) (types.ImageReference, error) {
resolved, err := explicitfilepath.ResolvePathToFullyExplicit(file)
if err != nil {
return nil, err
Expand All @@ -73,7 +91,20 @@ func NewReference(file, image string) (types.ImageReference, error) {
return nil, err
}

return ociArchiveReference{file: file, resolvedFile: resolved, image: image}, nil
if sourceIndex != -1 && sourceIndex < 0 {
return nil, fmt.Errorf("index @%d must not be negative: %w", sourceIndex, invalidOciArchiveErr)
}
if sourceIndex != -1 && image != "" {
return nil, fmt.Errorf("cannot set image %s and index @%d at the same time: %w", image, sourceIndex, invalidOciArchiveErr)
}
return ociArchiveReference{
file: file,
resolvedFile: resolved,
image: image,
sourceIndex: sourceIndex,
archiveReader: archiveReader,
archiveWriter: archiveWriter,
}, nil
}

func (ref ociArchiveReference) Transport() types.ImageTransport {
Expand All @@ -83,7 +114,10 @@ func (ref ociArchiveReference) Transport() types.ImageTransport {
// StringWithinTransport returns a string representation of the reference, which MUST be such that
// reference.Transport().ParseReference(reference.StringWithinTransport()) returns an equivalent reference.
func (ref ociArchiveReference) StringWithinTransport() string {
return fmt.Sprintf("%s:%s", ref.file, ref.image)
if ref.sourceIndex == -1 {
return fmt.Sprintf("%s:%s", ref.file, ref.image)
}
return fmt.Sprintf("%s:@%d", ref.file, ref.sourceIndex)
}

// DockerReference returns a Docker reference associated with this reference
Expand Down Expand Up @@ -156,14 +190,20 @@ func (t *tempDirOCIRef) deleteTempDir() error {

// createOCIRef creates the oci reference of the image
// If SystemContext.BigFilesTemporaryDir not "", overrides the temporary directory to use for storing big files
func createOCIRef(sys *types.SystemContext, image string) (tempDirOCIRef, error) {
dir, err := os.MkdirTemp(tmpdir.TemporaryDirectoryForBigFiles(sys), "oci")
func createOCIRef(sys *types.SystemContext, image string, sourceIndex int) (tempDirOCIRef, error) {
dir, err := ioutil.TempDir(tmpdir.TemporaryDirectoryForBigFiles(sys), "oci")
Copy link
Collaborator

Choose a reason for hiding this comment

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

(Merge problem) This should use the newer os.MkdirTemp name.

if err != nil {
return tempDirOCIRef{}, perrors.Wrapf(err, "creating temp directory")
}
ociRef, err := ocilayout.NewReference(dir, image)
if err != nil {
return tempDirOCIRef{}, err
var ociRef types.ImageReference
if sourceIndex > -1 {
if ociRef, err = ocilayout.NewIndexReference(dir, sourceIndex); err != nil {
return tempDirOCIRef{}, err
}
} else {
if ociRef, err = ocilayout.NewReference(dir, image); err != nil {
return tempDirOCIRef{}, err
}
}

tempDirRef := tempDirOCIRef{tempDirectory: dir, ociRefExtracted: ociRef}
Expand All @@ -172,7 +212,7 @@ func createOCIRef(sys *types.SystemContext, image string) (tempDirOCIRef, error)

// creates the temporary directory and copies the tarred content to it
func createUntarTempDir(sys *types.SystemContext, ref ociArchiveReference) (tempDirOCIRef, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICS this should be entirely replaced by uses of Reader.

Copy link
Member Author

Choose a reason for hiding this comment

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

Will do this in a follow up PR with the Reader re-work mentioned below

tempDirRef, err := createOCIRef(sys, ref.image)
tempDirRef, err := createOCIRef(sys, ref.image, ref.sourceIndex)
if err != nil {
return tempDirOCIRef{}, perrors.Wrap(err, "creating oci reference")
}
Expand Down
Loading