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

Add Copy.Options.ReportResolvedReference #2609

Merged
merged 4 commits into from
Nov 5, 2024
Merged
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
19 changes: 18 additions & 1 deletion copy/copy.go
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,17 @@ type Options struct {
// DestinationCtx.CompressionFormat is used exclusively, and blobs of other
// compression algorithms are not reused.
ForceCompressionFormat bool

// ReportResolvedReference, if set, asks the destination transport to store
// a “resolved” (more detailed) reference to the created image
// into the value this option points to.
// What “resolved” means is transport-specific.
// Most transports don’t support this, and cause the value to be set to nil.
//
// For the containers-storage: transport, the reference contains an image ID,
// so that storage.ResolveReference returns exactly the created image.
// WARNING: It is unspecified whether the reference also contains a reference.Named element.
ReportResolvedReference *types.ImageReference
Copy link
Member

Choose a reason for hiding this comment

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

I find it somewhat confusing that we use the Options field to return information to the caller. But I guess that is needed to avoid breaking changes in the API? I also get the that this is why it needs a pointer to an interface (which itself is also a pointer) but that seems a bit confusing to use as a caller.

Anyway I don't know the code + history here. I trust you that this is the best design

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we are committed not to change the API.

Alternatively, we could add a copy.ImageWithResolvedReference returning the extra value… and dropping the manifest bytes return value? I don’t know. Most users just don’t need the value.

Sort of hiding this feature in the Options field means we still are committed to API stability, but most users won’t see this and won’t worry about the extra values. And, also, the code implementing the copy can tell whether the caller wants the value or not.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I am fine with this, you are the main maintainer here after all and if you like this approach. We don't use the merge bot here, do we? So feel free to merge and move it along into c/common.

}

// OptionCompressionVariant allows to supply information about
Expand Down Expand Up @@ -337,7 +348,13 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef,
}
}

if err := c.dest.Commit(ctx, c.unparsedToplevel); err != nil {
if options.ReportResolvedReference != nil {
*options.ReportResolvedReference = nil // The default outcome, if not specifically supported by the transport.
}
if err := c.dest.CommitWithOptions(ctx, private.CommitOptions{
UnparsedToplevel: c.unparsedToplevel,
ReportResolvedReference: options.ReportResolvedReference,
}); err != nil {
return nil, fmt.Errorf("committing the finished image: %w", err)
}

Expand Down
11 changes: 4 additions & 7 deletions directory/directory_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -251,14 +251,11 @@ func (d *dirImageDestination) PutSignaturesWithFormat(ctx context.Context, signa
return nil
}

// Commit marks the process of storing the image as successful and asks for the image to be persisted.
// unparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
// original manifest list digest, if desired.
// CommitWithOptions marks the process of storing the image as successful and asks for the image to be persisted.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before Commit() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without Commit() (i.e. rollback is allowed but not guaranteed)
func (d *dirImageDestination) Commit(context.Context, types.UnparsedImage) error {
// - Uploaded data MAY be visible to others before CommitWithOptions() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
func (d *dirImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error {
return nil
}

Expand Down
26 changes: 12 additions & 14 deletions docker/archive/dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,16 +34,17 @@ func newImageDestination(sys *types.SystemContext, ref archiveReference) (privat
writer = w
closeWriter = true
}
tarDest := tarfile.NewDestination(sys, writer.archive, ref.Transport().Name(), ref.ref)
if sys != nil && sys.DockerArchiveAdditionalTags != nil {
tarDest.AddRepoTags(sys.DockerArchiveAdditionalTags)
}
return &archiveImageDestination{
Destination: tarDest,
d := &archiveImageDestination{
ref: ref,
writer: writer,
closeWriter: closeWriter,
}, nil
}
tarDest := tarfile.NewDestination(sys, writer.archive, ref.Transport().Name(), ref.ref, d.CommitWithOptions)
if sys != nil && sys.DockerArchiveAdditionalTags != nil {
tarDest.AddRepoTags(sys.DockerArchiveAdditionalTags)
}
d.Destination = tarDest
return d, nil
}

// Reference returns the reference used to set up this destination. Note that this should directly correspond to user's intent,
Expand All @@ -60,14 +61,11 @@ func (d *archiveImageDestination) Close() error {
return nil
}

// Commit marks the process of storing the image as successful and asks for the image to be persisted.
// unparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
// original manifest list digest, if desired.
// CommitWithOptions marks the process of storing the image as successful and asks for the image to be persisted.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before Commit() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without Commit() (i.e. rollback is allowed but not guaranteed)
func (d *archiveImageDestination) Commit(ctx context.Context, unparsedToplevel types.UnparsedImage) error {
// - Uploaded data MAY be visible to others before CommitWithOptions() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
func (d *archiveImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error {
d.writer.imageCommitted()
if d.closeWriter {
// We could do this only in .Close(), but failures in .Close() are much more likely to be
Expand Down
20 changes: 9 additions & 11 deletions docker/daemon/daemon_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,16 +56,17 @@ func newImageDestination(ctx context.Context, sys *types.SystemContext, ref daem
goroutineContext, goroutineCancel := context.WithCancel(ctx)
go imageLoadGoroutine(goroutineContext, c, reader, statusChannel)

return &daemonImageDestination{
d := &daemonImageDestination{
ref: ref,
mustMatchRuntimeOS: mustMatchRuntimeOS,
Destination: tarfile.NewDestination(sys, archive, ref.Transport().Name(), namedTaggedRef),
archive: archive,
goroutineCancel: goroutineCancel,
statusChannel: statusChannel,
writer: writer,
committed: false,
}, nil
}
d.Destination = tarfile.NewDestination(sys, archive, ref.Transport().Name(), namedTaggedRef, d.CommitWithOptions)
return d, nil
}

// imageLoadGoroutine accepts tar stream on reader, sends it to c, and reports error or success by writing to statusChannel
Expand Down Expand Up @@ -146,7 +147,7 @@ func (d *daemonImageDestination) Close() error {
// immediately, and hopefully, through terminating the sending which uses "Transfer-Encoding: chunked"" without sending
// the terminating zero-length chunk, prevent the docker daemon from processing the tar stream at all.
// Whether that works or not, closing the PipeWriter seems desirable in any case.
if err := d.writer.CloseWithError(errors.New("Aborting upload, daemonImageDestination closed without a previous .Commit()")); err != nil {
if err := d.writer.CloseWithError(errors.New("Aborting upload, daemonImageDestination closed without a previous .CommitWithOptions()")); err != nil {
return err
}
}
Expand All @@ -159,14 +160,11 @@ func (d *daemonImageDestination) Reference() types.ImageReference {
return d.ref
}

// Commit marks the process of storing the image as successful and asks for the image to be persisted.
// unparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
// original manifest list digest, if desired.
// CommitWithOptions marks the process of storing the image as successful and asks for the image to be persisted.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before Commit() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without Commit() (i.e. rollback is allowed but not guaranteed)
func (d *daemonImageDestination) Commit(ctx context.Context, unparsedToplevel types.UnparsedImage) error {
// - Uploaded data MAY be visible to others before CommitWithOptions() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
func (d *daemonImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error {
logrus.Debugf("docker-daemon: Closing tar stream")
if err := d.archive.Close(); err != nil {
return err
Expand Down
11 changes: 4 additions & 7 deletions docker/docker_image_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -923,13 +923,10 @@ func (d *dockerImageDestination) putSignaturesToAPIExtension(ctx context.Context
return nil
}

// Commit marks the process of storing the image as successful and asks for the image to be persisted.
// unparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
// original manifest list digest, if desired.
// CommitWithOptions marks the process of storing the image as successful and asks for the image to be persisted.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before Commit() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without Commit() (i.e. rollback is allowed but not guaranteed)
func (d *dockerImageDestination) Commit(context.Context, types.UnparsedImage) error {
// - Uploaded data MAY be visible to others before CommitWithOptions() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
func (d *dockerImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error {
return nil
}
28 changes: 21 additions & 7 deletions docker/internal/tarfile/dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,22 +20,25 @@ import (
"github.com/sirupsen/logrus"
)

// Destination is a partial implementation of private.ImageDestination for writing to an io.Writer.
// Destination is a partial implementation of private.ImageDestination for writing to a Writer.
type Destination struct {
impl.Compat
impl.PropertyMethodsInitialize
stubs.NoPutBlobPartialInitialize
stubs.NoSignaturesInitialize

archive *Writer
repoTags []reference.NamedTagged
archive *Writer
commitWithOptions func(ctx context.Context, options private.CommitOptions) error
repoTags []reference.NamedTagged
// Other state.
config []byte
sysCtx *types.SystemContext
}

// NewDestination returns a tarfile.Destination adding images to the specified Writer.
func NewDestination(sys *types.SystemContext, archive *Writer, transportName string, ref reference.NamedTagged) *Destination {
// commitWithOptions implements ImageDestination.CommitWithOptions.
func NewDestination(sys *types.SystemContext, archive *Writer, transportName string, ref reference.NamedTagged,
commitWithOptions func(ctx context.Context, options private.CommitOptions) error) *Destination {
repoTags := []reference.NamedTagged{}
if ref != nil {
repoTags = append(repoTags, ref)
Expand All @@ -57,9 +60,10 @@ func NewDestination(sys *types.SystemContext, archive *Writer, transportName str
NoPutBlobPartialInitialize: stubs.NoPutBlobPartialRaw(transportName),
NoSignaturesInitialize: stubs.NoSignatures("Storing signatures for docker tar files is not supported"),

archive: archive,
repoTags: repoTags,
sysCtx: sys,
archive: archive,
commitWithOptions: commitWithOptions,
repoTags: repoTags,
sysCtx: sys,
}
dest.Compat = impl.AddCompat(dest)
return dest
Expand Down Expand Up @@ -179,3 +183,13 @@ func (d *Destination) PutManifest(ctx context.Context, m []byte, instanceDigest

return d.archive.ensureManifestItemLocked(man.LayersDescriptors, man.ConfigDescriptor.Digest, d.repoTags)
}

// CommitWithOptions marks the process of storing the image as successful and asks for the image to be persisted.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before CommitWithOptions() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
func (d *Destination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error {
// This indirection exists because impl.Compat expects all ImageDestinationInternalOnly methods
// to be implemented in one place.
return d.commitWithOptions(ctx, options)
}
2 changes: 1 addition & 1 deletion docker/internal/tarfile/src_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func TestSourcePrepareLayerData(t *testing.T) {
ctx := context.Background()

writer := NewWriter(&tarfileBuffer)
dest := NewDestination(nil, writer, "transport name", nil)
dest := NewDestination(nil, writer, "transport name", nil, nil)
// No layers
configInfo, err := dest.PutBlob(ctx, strings.NewReader(c.config),
types.BlobInfo{Size: -1}, cache, true)
Expand Down
16 changes: 13 additions & 3 deletions docker/tarfile/dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (

internal "github.com/containers/image/v5/docker/internal/tarfile"
"github.com/containers/image/v5/docker/reference"
"github.com/containers/image/v5/internal/private"
"github.com/containers/image/v5/types"
"github.com/opencontainers/go-digest"
)
Expand All @@ -25,10 +26,11 @@ func NewDestination(dest io.Writer, ref reference.NamedTagged) *Destination {
// NewDestinationWithContext returns a tarfile.Destination for the specified io.Writer.
func NewDestinationWithContext(sys *types.SystemContext, dest io.Writer, ref reference.NamedTagged) *Destination {
archive := internal.NewWriter(dest)
return &Destination{
internal: internal.NewDestination(sys, archive, "[An external docker/tarfile caller]", ref),
archive: archive,
d := &Destination{
archive: archive,
}
d.internal = internal.NewDestination(sys, archive, "[An external docker/tarfile caller]", ref, d.commitWithOptions)
return d
}

// AddRepoTags adds the specified tags to the destination's repoTags.
Expand Down Expand Up @@ -117,3 +119,11 @@ func (d *Destination) PutSignatures(ctx context.Context, signatures [][]byte, in
func (d *Destination) Commit(ctx context.Context) error {
return d.archive.Close()
}

// commitWithOptions marks the process of storing the image as successful and asks for the image to be persisted.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before CommitWithOptions() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
func (d *Destination) commitWithOptions(ctx context.Context, options private.CommitOptions) error {
return d.Commit(ctx)
}
13 changes: 13 additions & 0 deletions internal/imagedestination/impl/compat.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,3 +99,16 @@ func (c *Compat) PutSignatures(ctx context.Context, signatures [][]byte, instanc
}
return c.dest.PutSignaturesWithFormat(ctx, withFormat, instanceDigest)
}

// Commit marks the process of storing the image as successful and asks for the image to be persisted.
// unparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
// original manifest list digest, if desired.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before Commit() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without Commit() (i.e. rollback is allowed but not guaranteed)
func (c *Compat) Commit(ctx context.Context, unparsedToplevel types.UnparsedImage) error {
return c.dest.CommitWithOptions(ctx, private.CommitOptions{
UnparsedToplevel: unparsedToplevel,
})
}
8 changes: 8 additions & 0 deletions internal/imagedestination/wrapper.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,11 @@ func (w *wrapped) PutSignaturesWithFormat(ctx context.Context, signatures []sign
}
return w.PutSignatures(ctx, simpleSigs, instanceDigest)
}

// CommitWithOptions marks the process of storing the image as successful and asks for the image to be persisted.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before CommitWithOptions() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
func (w *wrapped) CommitWithOptions(ctx context.Context, options private.CommitOptions) error {
return w.Commit(ctx, options.UnparsedToplevel)
}
19 changes: 19 additions & 0 deletions internal/private/private.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,12 @@ type ImageDestinationInternalOnly interface {
// (when the primary manifest is a manifest list); this should always be nil if the primary manifest is not a manifest list.
// MUST be called after PutManifest (signatures may reference manifest contents).
PutSignaturesWithFormat(ctx context.Context, signatures []signature.Signature, instanceDigest *digest.Digest) error

// CommitWithOptions marks the process of storing the image as successful and asks for the image to be persisted.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before CommitWithOptions() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
CommitWithOptions(ctx context.Context, options CommitOptions) error
}

// ImageDestination is an internal extension to the types.ImageDestination
Expand Down Expand Up @@ -146,6 +152,19 @@ type ReusedBlob struct {
MatchedByTOCDigest bool // Whether the layer was reused/matched by TOC digest. Used only for UI purposes.
}

// CommitOptions are used in CommitWithOptions
type CommitOptions struct {
// UnparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
// original manifest list digest, if desired.
UnparsedToplevel types.UnparsedImage
// ReportResolvedReference, if set, asks the transport to store a “resolved” (more detailed) reference to the created image
// into the value this option points to.
// What “resolved” means is transport-specific.
// Transports which don’t support reporting resolved references can ignore the field; the generic copy code writes "nil" into the value.
ReportResolvedReference *types.ImageReference
}

// ImageSourceChunk is a portion of a blob.
// This API is experimental and can be changed without bumping the major version number.
type ImageSourceChunk struct {
Expand Down
13 changes: 6 additions & 7 deletions oci/archive/oci_dest.go
Original file line number Diff line number Diff line change
Expand Up @@ -150,13 +150,12 @@ func (d *ociArchiveImageDestination) PutSignaturesWithFormat(ctx context.Context
return d.unpackedDest.PutSignaturesWithFormat(ctx, signatures, instanceDigest)
}

// Commit marks the process of storing the image as successful and asks for the image to be persisted
// unparsedToplevel contains data about the top-level manifest of the source (which may be a single-arch image or a manifest list
// if PutManifest was only called for the single-arch image with instanceDigest == nil), primarily to allow lookups by the
// original manifest list digest, if desired.
// after the directory is made, it is tarred up into a file and the directory is deleted
func (d *ociArchiveImageDestination) Commit(ctx context.Context, unparsedToplevel types.UnparsedImage) error {
if err := d.unpackedDest.Commit(ctx, unparsedToplevel); err != nil {
// CommitWithOptions marks the process of storing the image as successful and asks for the image to be persisted.
// WARNING: This does not have any transactional semantics:
// - Uploaded data MAY be visible to others before CommitWithOptions() is called
// - Uploaded data MAY be removed or MAY remain around if Close() is called without CommitWithOptions() (i.e. rollback is allowed but not guaranteed)
func (d *ociArchiveImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error {
if err := d.unpackedDest.CommitWithOptions(ctx, options); err != nil {
return fmt.Errorf("storing image %q: %w", d.ref.image, err)
}

Expand Down
Loading