From 831269d7508b36d88d662cd6e40d0c6294626ecf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 19 Oct 2024 01:34:40 +0200 Subject: [PATCH 1/4] Rename an options variable to imgOptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... because we will add a parameter named "options". Should not change behavior. Signed-off-by: Miloslav Trmač --- storage/storage_dest.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/storage/storage_dest.go b/storage/storage_dest.go index 5612a2a28..dd6e41992 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -1190,10 +1190,10 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t // If one of those blobs was a configuration blob, then we can try to dig out the date when the image // was originally created, in case we're just copying it. If not, no harm done. - options := &storage.ImageOptions{} + imgOptions := &storage.ImageOptions{} if inspect, err := man.Inspect(s.getConfigBlob); err == nil && inspect.Created != nil { logrus.Debugf("setting image creation date to %s", inspect.Created) - options.CreationDate = *inspect.Created + imgOptions.CreationDate = *inspect.Created } // Set up to save the non-layer blobs as data items. Since we only share layers, they should all be in files, so @@ -1210,7 +1210,7 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t if err != nil { return fmt.Errorf("copying non-layer blob %q to image: %w", blob, err) } - options.BigData = append(options.BigData, storage.ImageBigDataOption{ + imgOptions.BigData = append(imgOptions.BigData, storage.ImageBigDataOption{ Key: blob.String(), Data: v, Digest: digest.Canonical.FromBytes(v), @@ -1227,7 +1227,7 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t if err != nil { return err } - options.BigData = append(options.BigData, storage.ImageBigDataOption{ + imgOptions.BigData = append(imgOptions.BigData, storage.ImageBigDataOption{ Key: key, Data: toplevelManifest, Digest: manifestDigest, @@ -1240,19 +1240,19 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t if err != nil { return err } - options.BigData = append(options.BigData, storage.ImageBigDataOption{ + imgOptions.BigData = append(imgOptions.BigData, storage.ImageBigDataOption{ Key: key, Data: s.manifest, Digest: s.manifestDigest, }) - options.BigData = append(options.BigData, storage.ImageBigDataOption{ + imgOptions.BigData = append(imgOptions.BigData, storage.ImageBigDataOption{ Key: storage.ImageDigestBigDataKey, Data: s.manifest, Digest: s.manifestDigest, }) // Set up to save the signatures, if we have any. if len(s.signatures) > 0 { - options.BigData = append(options.BigData, storage.ImageBigDataOption{ + imgOptions.BigData = append(imgOptions.BigData, storage.ImageBigDataOption{ Key: "signatures", Data: s.signatures, Digest: digest.Canonical.FromBytes(s.signatures), @@ -1263,7 +1263,7 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t if err != nil { return err } - options.BigData = append(options.BigData, storage.ImageBigDataOption{ + imgOptions.BigData = append(imgOptions.BigData, storage.ImageBigDataOption{ Key: key, Data: signatures, Digest: digest.Canonical.FromBytes(signatures), @@ -1276,7 +1276,7 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t return fmt.Errorf("encoding metadata for image: %w", err) } if len(metadata) != 0 { - options.Metadata = string(metadata) + imgOptions.Metadata = string(metadata) } // Create the image record, pointing to the most-recently added layer. @@ -1288,7 +1288,7 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t } } oldNames := []string{} - img, err := s.imageRef.transport.store.CreateImage(intendedID, nil, lastLayer, "", options) + img, err := s.imageRef.transport.store.CreateImage(intendedID, nil, lastLayer, "", imgOptions) if err != nil { if !errors.Is(err, storage.ErrDuplicateID) { logrus.Debugf("error creating image: %q", err) @@ -1309,21 +1309,21 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t // sizes (tracked in the metadata) which might have already // been present with new values, when ideally we'd find a way // to merge them since they all apply to the same image - for _, data := range options.BigData { + for _, data := range imgOptions.BigData { if err := s.imageRef.transport.store.SetImageBigData(img.ID, data.Key, data.Data, manifest.Digest); err != nil { logrus.Debugf("error saving big data %q for image %q: %v", data.Key, img.ID, err) return fmt.Errorf("saving big data %q for image %q: %w", data.Key, img.ID, err) } } - if options.Metadata != "" { - if err := s.imageRef.transport.store.SetMetadata(img.ID, options.Metadata); err != nil { + if imgOptions.Metadata != "" { + if err := s.imageRef.transport.store.SetMetadata(img.ID, imgOptions.Metadata); err != nil { logrus.Debugf("error saving metadata for image %q: %v", img.ID, err) return fmt.Errorf("saving metadata for image %q: %w", img.ID, err) } - logrus.Debugf("saved image metadata %q", options.Metadata) + logrus.Debugf("saved image metadata %q", imgOptions.Metadata) } } else { - logrus.Debugf("created new image ID %q with metadata %q", img.ID, options.Metadata) + logrus.Debugf("created new image ID %q with metadata %q", img.ID, imgOptions.Metadata) } // Clean up the unfinished image on any error. From 91d22b295c2907feca01a468587a9ba34d334da9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Sat, 19 Oct 2024 01:37:33 +0200 Subject: [PATCH 2/4] Introduce private.ImageDestination.CommitWithOptions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This only adds an API method, it does not change behavior. Signed-off-by: Miloslav Trmač --- copy/copy.go | 4 +++- directory/directory_dest.go | 11 ++++------ docker/archive/dest.go | 26 ++++++++++------------ docker/daemon/daemon_dest.go | 20 ++++++++--------- docker/docker_image_dest.go | 11 ++++------ docker/internal/tarfile/dest.go | 28 ++++++++++++++++++------ docker/internal/tarfile/src_test.go | 2 +- docker/tarfile/dest.go | 16 +++++++++++--- internal/imagedestination/impl/compat.go | 13 +++++++++++ internal/imagedestination/wrapper.go | 8 +++++++ internal/private/private.go | 14 ++++++++++++ oci/archive/oci_dest.go | 13 +++++------ oci/layout/oci_dest.go | 11 ++++------ openshift/openshift_dest.go | 13 +++++------ ostree/ostree_dest.go | 6 ++++- pkg/blobcache/dest.go | 8 +++++-- storage/storage_dest.go | 23 +++++++++---------- 17 files changed, 138 insertions(+), 89 deletions(-) diff --git a/copy/copy.go b/copy/copy.go index 867ba73c7..ab36ddc20 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -337,7 +337,9 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } } - if err := c.dest.Commit(ctx, c.unparsedToplevel); err != nil { + if err := c.dest.CommitWithOptions(ctx, private.CommitOptions{ + UnparsedToplevel: c.unparsedToplevel, + }); err != nil { return nil, fmt.Errorf("committing the finished image: %w", err) } diff --git a/directory/directory_dest.go b/directory/directory_dest.go index c9b390318..000c8987d 100644 --- a/directory/directory_dest.go +++ b/directory/directory_dest.go @@ -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 } diff --git a/docker/archive/dest.go b/docker/archive/dest.go index 632ee7c49..9e0d32007 100644 --- a/docker/archive/dest.go +++ b/docker/archive/dest.go @@ -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, @@ -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 diff --git a/docker/daemon/daemon_dest.go b/docker/daemon/daemon_dest.go index 9b880a2e7..4a59a6a61 100644 --- a/docker/daemon/daemon_dest.go +++ b/docker/daemon/daemon_dest.go @@ -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 @@ -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 } } @@ -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 diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index ed3d4a2c0..f5e2bb61e 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -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 } diff --git a/docker/internal/tarfile/dest.go b/docker/internal/tarfile/dest.go index d5446840b..f14234680 100644 --- a/docker/internal/tarfile/dest.go +++ b/docker/internal/tarfile/dest.go @@ -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) @@ -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 @@ -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) +} diff --git a/docker/internal/tarfile/src_test.go b/docker/internal/tarfile/src_test.go index 1f8248ac5..97c312311 100644 --- a/docker/internal/tarfile/src_test.go +++ b/docker/internal/tarfile/src_test.go @@ -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) diff --git a/docker/tarfile/dest.go b/docker/tarfile/dest.go index 454765439..2507d5600 100644 --- a/docker/tarfile/dest.go +++ b/docker/tarfile/dest.go @@ -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" ) @@ -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. @@ -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) +} diff --git a/internal/imagedestination/impl/compat.go b/internal/imagedestination/impl/compat.go index 47c169a1f..70b207d9b 100644 --- a/internal/imagedestination/impl/compat.go +++ b/internal/imagedestination/impl/compat.go @@ -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, + }) +} diff --git a/internal/imagedestination/wrapper.go b/internal/imagedestination/wrapper.go index f5a38541a..0fe902e31 100644 --- a/internal/imagedestination/wrapper.go +++ b/internal/imagedestination/wrapper.go @@ -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) +} diff --git a/internal/private/private.go b/internal/private/private.go index 7390d91ef..3ab2cc478 100644 --- a/internal/private/private.go +++ b/internal/private/private.go @@ -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 @@ -146,6 +152,14 @@ 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 +} + // ImageSourceChunk is a portion of a blob. // This API is experimental and can be changed without bumping the major version number. type ImageSourceChunk struct { diff --git a/oci/archive/oci_dest.go b/oci/archive/oci_dest.go index f9c05e39d..1d1e26c84 100644 --- a/oci/archive/oci_dest.go +++ b/oci/archive/oci_dest.go @@ -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) } diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go index a096afe0f..85374cecf 100644 --- a/oci/layout/oci_dest.go +++ b/oci/layout/oci_dest.go @@ -278,14 +278,11 @@ func (d *ociImageDestination) addManifest(desc *imgspecv1.Descriptor) { d.index.Manifests = append(slices.Clone(d.index.Manifests), *desc) } -// 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 *ociImageDestination) 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 *ociImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error { layoutBytes, err := json.Marshal(imgspecv1.ImageLayout{ Version: imgspecv1.ImageLayoutVersion, }) diff --git a/openshift/openshift_dest.go b/openshift/openshift_dest.go index 833e67094..61f69e93f 100644 --- a/openshift/openshift_dest.go +++ b/openshift/openshift_dest.go @@ -236,13 +236,10 @@ func (d *openshiftImageDestination) PutSignaturesWithFormat(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 *openshiftImageDestination) Commit(ctx context.Context, unparsedToplevel types.UnparsedImage) error { - return d.docker.Commit(ctx, unparsedToplevel) +// - 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 *openshiftImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error { + return d.docker.CommitWithOptions(ctx, options) } diff --git a/ostree/ostree_dest.go b/ostree/ostree_dest.go index 951b5d098..d4ebe413b 100644 --- a/ostree/ostree_dest.go +++ b/ostree/ostree_dest.go @@ -435,7 +435,11 @@ func (d *ostreeImageDestination) PutSignaturesWithFormat(ctx context.Context, si return nil } -func (d *ostreeImageDestination) Commit(context.Context, types.UnparsedImage) 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) +func (d *ostreeImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error { runtime.LockOSThread() defer runtime.UnlockOSThread() diff --git a/pkg/blobcache/dest.go b/pkg/blobcache/dest.go index 0b229d5a9..f7103d13e 100644 --- a/pkg/blobcache/dest.go +++ b/pkg/blobcache/dest.go @@ -307,6 +307,10 @@ func (d *blobCacheDestination) PutSignaturesWithFormat(ctx context.Context, sign return d.destination.PutSignaturesWithFormat(ctx, signatures, instanceDigest) } -func (d *blobCacheDestination) Commit(ctx context.Context, unparsedToplevel types.UnparsedImage) error { - return d.destination.Commit(ctx, unparsedToplevel) +// 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 *blobCacheDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error { + return d.destination.CommitWithOptions(ctx, options) } diff --git a/storage/storage_dest.go b/storage/storage_dest.go index dd6e41992..8919066f3 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -1124,26 +1124,23 @@ func (s *storageImageDestination) untrustedLayerDiffID(layerIndex int) (digest.D return s.untrustedDiffIDValues[layerIndex], 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 (s *storageImageDestination) 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 (s *storageImageDestination) CommitWithOptions(ctx context.Context, options private.CommitOptions) error { // This function is outside of the scope of HasThreadSafePutBlob, so we don’t need to hold s.lock. if len(s.manifest) == 0 { - return errors.New("Internal error: storageImageDestination.Commit() called without PutManifest()") + return errors.New("Internal error: storageImageDestination.CommitWithOptions() called without PutManifest()") } - toplevelManifest, _, err := unparsedToplevel.Manifest(ctx) + toplevelManifest, _, err := options.UnparsedToplevel.Manifest(ctx) if err != nil { return fmt.Errorf("retrieving top-level manifest: %w", err) } // If the name we're saving to includes a digest, then check that the // manifests that we're about to save all either match the one from the - // unparsedToplevel, or match the digest in the name that we're using. + // options.UnparsedToplevel, or match the digest in the name that we're using. if s.imageRef.named != nil { if digested, ok := s.imageRef.named.(reference.Digested); ok { matches, err := manifest.MatchesDigest(s.manifest, digested.Digest()) @@ -1176,14 +1173,14 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t }, blob.Size); err != nil { return err } else if stopQueue { - return fmt.Errorf("Internal error: storageImageDestination.Commit(): commitLayer() not ready to commit for layer %q", blob.Digest) + return fmt.Errorf("Internal error: storageImageDestination.CommitWithOptions(): commitLayer() not ready to commit for layer %q", blob.Digest) } } var lastLayer string if len(layerBlobs) > 0 { // Zero-layer images rarely make sense, but it is technically possible, and may happen for non-image artifacts. prev, ok := s.indexToStorageID[len(layerBlobs)-1] if !ok { - return fmt.Errorf("Internal error: storageImageDestination.Commit(): previous layer %d hasn't been committed (lastLayer == nil)", len(layerBlobs)-1) + return fmt.Errorf("Internal error: storageImageDestination.CommitWithOptions(): previous layer %d hasn't been committed (lastLayer == nil)", len(layerBlobs)-1) } lastLayer = prev } @@ -1216,7 +1213,7 @@ func (s *storageImageDestination) Commit(ctx context.Context, unparsedToplevel t Digest: digest.Canonical.FromBytes(v), }) } - // Set up to save the unparsedToplevel's manifest if it differs from + // Set up to save the options.UnparsedToplevel's manifest if it differs from // the per-platform one, which is saved below. if len(toplevelManifest) != 0 && !bytes.Equal(toplevelManifest, s.manifest) { manifestDigest, err := manifest.Digest(toplevelManifest) From 125f8628515c55ba697c372406e918ac1f868332 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 21 Oct 2024 13:16:12 +0200 Subject: [PATCH 3/4] Return a precise reference to the created image when writing to containers-storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/copy.go | 16 +++++++++++++++- internal/private/private.go | 5 +++++ storage/storage_dest.go | 7 +++++++ 3 files changed, 27 insertions(+), 1 deletion(-) diff --git a/copy/copy.go b/copy/copy.go index ab36ddc20..75d6b8234 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -137,6 +137,16 @@ 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. + ReportResolvedReference *types.ImageReference } // OptionCompressionVariant allows to supply information about @@ -337,8 +347,12 @@ func Image(ctx context.Context, policyContext *signature.PolicyContext, destRef, } } + 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, + UnparsedToplevel: c.unparsedToplevel, + ReportResolvedReference: options.ReportResolvedReference, }); err != nil { return nil, fmt.Errorf("committing the finished image: %w", err) } diff --git a/internal/private/private.go b/internal/private/private.go index 3ab2cc478..4247a8db7 100644 --- a/internal/private/private.go +++ b/internal/private/private.go @@ -158,6 +158,11 @@ type CommitOptions struct { // 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. diff --git a/storage/storage_dest.go b/storage/storage_dest.go index 8919066f3..ce66bccec 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -1343,6 +1343,13 @@ func (s *storageImageDestination) CommitWithOptions(ctx context.Context, options } logrus.Debugf("added name %q to image %q", name, img.ID) } + if options.ReportResolvedReference != nil { + resolved, err := newReference(s.imageRef.transport, s.imageRef.named, intendedID) + if err != nil { + return fmt.Errorf("creating a resolved reference for (%s, %s): %w", s.imageRef.StringWithinTransport(), intendedID, err) + } + *options.ReportResolvedReference = resolved + } commitSucceeded = true return nil From 6ba898f74fd4b434369ae24917b9528d8f5b02eb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 4 Nov 2024 17:28:26 +0100 Subject: [PATCH 4/4] HACK: Only return an image ID from ReportResolvedReference for c/storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/copy.go | 1 + storage/storage_dest.go | 10 +++++++++- 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/copy/copy.go b/copy/copy.go index 75d6b8234..5b26abb09 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -146,6 +146,7 @@ type Options struct { // // 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 } diff --git a/storage/storage_dest.go b/storage/storage_dest.go index ce66bccec..51fac71e4 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -1344,7 +1344,15 @@ func (s *storageImageDestination) CommitWithOptions(ctx context.Context, options logrus.Debugf("added name %q to image %q", name, img.ID) } if options.ReportResolvedReference != nil { - resolved, err := newReference(s.imageRef.transport, s.imageRef.named, intendedID) + // FIXME? This is using nil for the named reference. + // It would be better to also use s.imageRef.named, because that allows us to resolve to the right + // digest / manifest (and corresponding signatures). + // The problem with that is that resolving such a reference fails if the s.imageRef.named name is moved to a different image + // (because it is a tag that moved, or because we have pulled “the same” image for a different architecture). + // Right now (2024-11), ReportResolvedReference is only used in c/common/libimage, where the caller only extracts the image ID, + // so the name does not matter; to give us options, copy.Options.ReportResolvedReference is explicitly refusing to document + // whether the value contains a name. + resolved, err := newReference(s.imageRef.transport, nil, intendedID) if err != nil { return fmt.Errorf("creating a resolved reference for (%s, %s): %w", s.imageRef.StringWithinTransport(), intendedID, err) }