From 33c2f9025812293c6bc7ffeaffaced2366db9d87 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 18 Nov 2024 19:42:28 +0100 Subject: [PATCH 1/8] Tighten the conditions for storageImageSource.cachedManifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If the value is set to a zero-byte value, use it, instead of trying to look for a value again / elsewhere. This should not make a difference in practice, a zero-length manifest is invalid anyway; so it's just a conceptual cleanup / a microoptimization. Signed-off-by: Miloslav Trmač --- storage/storage_src.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/storage/storage_src.go b/storage/storage_src.go index 55788f8877..50dae54a27 100644 --- a/storage/storage_src.go +++ b/storage/storage_src.go @@ -247,7 +247,7 @@ func (s *storageImageSource) GetManifest(ctx context.Context, instanceDigest *di } return blob, manifest.GuessMIMEType(blob), err } - if len(s.cachedManifest) == 0 { + if s.cachedManifest == nil { // The manifest is stored as a big data item. // Prefer the manifest corresponding to the user-specified digest, if available. if s.imageRef.named != nil { @@ -267,7 +267,7 @@ func (s *storageImageSource) GetManifest(ctx context.Context, instanceDigest *di } // If the user did not specify a digest, or this is an old image stored before manifestBigDataKey was introduced, use the default manifest. // Note that the manifest may not match the expected digest, and that is likely to fail eventually, e.g. in c/image/image/UnparsedImage.Manifest(). - if len(s.cachedManifest) == 0 { + if s.cachedManifest == nil { cachedBlob, err := s.imageRef.transport.store.ImageBigData(s.image.ID, storage.ImageDigestBigDataKey) if err != nil { return nil, "", err From e9f954eb05969fdb005ac01f35c9421075eb97c3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 18 Nov 2024 19:42:52 +0100 Subject: [PATCH 2/8] Cache also the manifest MIME type in storageImageSource MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- storage/storage_src.go | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/storage/storage_src.go b/storage/storage_src.go index 50dae54a27..ff76b20667 100644 --- a/storage/storage_src.go +++ b/storage/storage_src.go @@ -35,13 +35,14 @@ type storageImageSource struct { impl.PropertyMethodsInitialize stubs.NoGetBlobAtInitialize - imageRef storageReference - image *storage.Image - systemContext *types.SystemContext // SystemContext used in GetBlob() to create temporary files - metadata storageImageMetadata - cachedManifest []byte // A cached copy of the manifest, if already known, or nil - getBlobMutex sync.Mutex // Mutex to sync state for parallel GetBlob executions - getBlobMutexProtected getBlobMutexProtected + imageRef storageReference + image *storage.Image + systemContext *types.SystemContext // SystemContext used in GetBlob() to create temporary files + metadata storageImageMetadata + cachedManifest []byte // A cached copy of the manifest, if already known, or nil + cachedManifestMIMEType string // Valid if cachedManifest != nil + getBlobMutex sync.Mutex // Mutex to sync state for parallel GetBlob executions + getBlobMutexProtected getBlobMutexProtected } // getBlobMutexProtected contains storageImageSource data protected by getBlobMutex. @@ -274,8 +275,9 @@ func (s *storageImageSource) GetManifest(ctx context.Context, instanceDigest *di } s.cachedManifest = cachedBlob } + s.cachedManifestMIMEType = manifest.GuessMIMEType(s.cachedManifest) } - return s.cachedManifest, manifest.GuessMIMEType(s.cachedManifest), err + return s.cachedManifest, s.cachedManifestMIMEType, err } // LayerInfosForCopy() returns the list of layer blobs that make up the root filesystem of From 9a1508d3b40d529fb846b134707ad767093477d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 18 Nov 2024 19:50:03 +0100 Subject: [PATCH 3/8] Simplify handling of toplevelManifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Other parts of the code already assume that the value is always valid, so don't treat an empty value specially. Signed-off-by: Miloslav Trmač --- storage/storage_dest.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/storage/storage_dest.go b/storage/storage_dest.go index 20d2576906..be19c971db 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -1232,7 +1232,7 @@ func (s *storageImageDestination) CommitWithOptions(ctx context.Context, options } // 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) { + if !bytes.Equal(toplevelManifest, s.manifest) { manifestDigest, err := manifest.Digest(toplevelManifest) if err != nil { return fmt.Errorf("digesting top-level manifest: %w", err) From 6f9016e4dde1088b5aca145fc3f2c5bfb900471f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 18 Nov 2024 19:50:33 +0100 Subject: [PATCH 4/8] Tighten the semantics of s.manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Always check for nil, not for len(0). Ensure that PutManifest always sets it to non-nil, so that valid call sequences (with an invalid empty manifest) don't show up as hard-to-explain invariant violations. Signed-off-by: Miloslav Trmač --- storage/storage_dest.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/storage/storage_dest.go b/storage/storage_dest.go index be19c971db..24fe08347d 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -57,8 +57,8 @@ type storageImageDestination struct { imageRef storageReference directory string // Temporary directory where we store blobs until Commit() time nextTempFileID atomic.Int32 // A counter that we use for computing filenames to assign to blobs - manifest []byte // Manifest contents, temporary - manifestDigest digest.Digest // Valid if len(manifest) != 0 + manifest []byte // (Per-instance) manifest contents, or nil if not yet known. + manifestDigest digest.Digest // Valid if manifest != nil untrustedDiffIDValues []digest.Digest // From config’s RootFS.DiffIDs (not even validated to be valid digest.Digest!); or nil if not read yet signatures []byte // Signature contents, temporary signatureses map[digest.Digest][]byte // Instance signature contents, temporary @@ -1148,7 +1148,7 @@ func (s *storageImageDestination) untrustedLayerDiffID(layerIndex int) (digest.D 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 { + if s.manifest == nil { return errors.New("Internal error: storageImageDestination.CommitWithOptions() called without PutManifest()") } toplevelManifest, _, err := options.UnparsedToplevel.Manifest(ctx) @@ -1387,6 +1387,9 @@ func (s *storageImageDestination) PutManifest(ctx context.Context, manifestBlob return err } s.manifest = bytes.Clone(manifestBlob) + if s.manifest == nil { // Make sure PutManifest can never succeed with s.manifest == nil + s.manifest = []byte{} + } s.manifestDigest = digest return nil } @@ -1409,7 +1412,7 @@ func (s *storageImageDestination) PutSignaturesWithFormat(ctx context.Context, s if instanceDigest == nil { s.signatures = sigblob s.metadata.SignatureSizes = sizes - if len(s.manifest) > 0 { + if s.manifest != nil { manifestDigest := s.manifestDigest instanceDigest = &manifestDigest } From 9af79e699778b569feb4ee1465dca5a3f521e883 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 18 Nov 2024 19:53:55 +0100 Subject: [PATCH 5/8] Only compute the MIME type of the committed manifest once MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We will add one more user. Should not change behavior. Signed-off-by: Miloslav Trmač --- storage/storage_dest.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/storage/storage_dest.go b/storage/storage_dest.go index 24fe08347d..71e6d14ed1 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -58,6 +58,7 @@ type storageImageDestination struct { directory string // Temporary directory where we store blobs until Commit() time nextTempFileID atomic.Int32 // A counter that we use for computing filenames to assign to blobs manifest []byte // (Per-instance) manifest contents, or nil if not yet known. + manifestMIMEType string // Valid if manifest != nil manifestDigest digest.Digest // Valid if manifest != nil untrustedDiffIDValues []digest.Digest // From config’s RootFS.DiffIDs (not even validated to be valid digest.Digest!); or nil if not read yet signatures []byte // Signature contents, temporary @@ -1105,17 +1106,16 @@ func (s *storageImageDestination) untrustedLayerDiffID(layerIndex int) (digest.D } if s.untrustedDiffIDValues == nil { - mt := manifest.GuessMIMEType(s.manifest) - if mt != imgspecv1.MediaTypeImageManifest { + if s.manifestMIMEType != imgspecv1.MediaTypeImageManifest { // We could, in principle, build an ImageSource, support arbitrary image formats using image.FromUnparsedImage, // and then use types.Image.OCIConfig so that we can parse the image. // // In practice, this should, right now, only matter for pulls of OCI images (this code path implies that a layer has annotation), // while converting to a non-OCI formats, using a manual (skopeo copy) or something similar, not (podman pull). // So it is not implemented yet. - return "", fmt.Errorf("determining DiffID for manifest type %q is not yet supported", mt) + return "", fmt.Errorf("determining DiffID for manifest type %q is not yet supported", s.manifestMIMEType) } - man, err := manifest.FromBlob(s.manifest, mt) + man, err := manifest.FromBlob(s.manifest, s.manifestMIMEType) if err != nil { return "", fmt.Errorf("parsing manifest: %w", err) } @@ -1176,7 +1176,7 @@ func (s *storageImageDestination) CommitWithOptions(ctx context.Context, options } } // Find the list of layer blobs. - man, err := manifest.FromBlob(s.manifest, manifest.GuessMIMEType(s.manifest)) + man, err := manifest.FromBlob(s.manifest, s.manifestMIMEType) if err != nil { return fmt.Errorf("parsing manifest: %w", err) } @@ -1390,6 +1390,7 @@ func (s *storageImageDestination) PutManifest(ctx context.Context, manifestBlob if s.manifest == nil { // Make sure PutManifest can never succeed with s.manifest == nil s.manifest = []byte{} } + s.manifestMIMEType = manifest.GuessMIMEType(s.manifest) s.manifestDigest = digest return nil } From 390c5bbefb40a3dbbb71944c6f7e53772db977e2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 18 Nov 2024 19:55:20 +0100 Subject: [PATCH 6/8] Allow determining DiffID values for v2s2 images MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For now this is not really relevant, but we will want to enforce the correctness of those values in the future. Signed-off-by: Miloslav Trmač --- storage/storage_dest.go | 89 ++++++++++++++++++++++++++++++++--------- 1 file changed, 69 insertions(+), 20 deletions(-) diff --git a/storage/storage_dest.go b/storage/storage_dest.go index 71e6d14ed1..e24a3468ee 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -17,8 +17,11 @@ import ( "sync/atomic" "github.com/containers/image/v5/docker/reference" + "github.com/containers/image/v5/internal/image" "github.com/containers/image/v5/internal/imagedestination/impl" "github.com/containers/image/v5/internal/imagedestination/stubs" + srcImpl "github.com/containers/image/v5/internal/imagesource/impl" + srcStubs "github.com/containers/image/v5/internal/imagesource/stubs" "github.com/containers/image/v5/internal/private" "github.com/containers/image/v5/internal/putblobdigest" "github.com/containers/image/v5/internal/set" @@ -1094,6 +1097,51 @@ func (s *storageImageDestination) createNewLayer(index int, layerDigest digest.D return layer, nil } +// uncommittedImageSource allows accessing an image’s metadata (not layers) before it has been committed, +// to allow using image.FromUnparsedImage. +type uncommittedImageSource struct { + srcImpl.Compat + srcImpl.PropertyMethodsInitialize + srcImpl.NoSignatures + srcImpl.DoesNotAffectLayerInfosForCopy + srcStubs.NoGetBlobAtInitialize + + d *storageImageDestination +} + +func newUncommittedImageSource(d *storageImageDestination) *uncommittedImageSource { + s := &uncommittedImageSource{ + PropertyMethodsInitialize: srcImpl.PropertyMethods(srcImpl.Properties{ + HasThreadSafeGetBlob: true, + }), + NoGetBlobAtInitialize: srcStubs.NoGetBlobAt(d.Reference()), + + d: d, + } + s.Compat = srcImpl.AddCompat(s) + return s +} + +func (u *uncommittedImageSource) Reference() types.ImageReference { + return u.d.Reference() +} + +func (u *uncommittedImageSource) Close() error { + return nil +} + +func (u *uncommittedImageSource) GetManifest(ctx context.Context, instanceDigest *digest.Digest) ([]byte, string, error) { + return u.d.manifest, u.d.manifestMIMEType, nil +} + +func (u *uncommittedImageSource) GetBlob(ctx context.Context, info types.BlobInfo, cache types.BlobInfoCache) (io.ReadCloser, int64, error) { + blob, err := u.d.getConfigBlob(info) + if err != nil { + return nil, -1, err + } + return io.NopCloser(bytes.NewReader(blob)), int64(len(blob)), nil +} + // untrustedLayerDiffID returns a DiffID value for layerIndex from the image’s config. // If the value is not yet available (but it can be available after s.manifets is set), it returns ("", nil). // WARNING: We don’t validate the DiffID value against the layer contents; it must not be used for any deduplication. @@ -1106,30 +1154,17 @@ func (s *storageImageDestination) untrustedLayerDiffID(layerIndex int) (digest.D } if s.untrustedDiffIDValues == nil { - if s.manifestMIMEType != imgspecv1.MediaTypeImageManifest { - // We could, in principle, build an ImageSource, support arbitrary image formats using image.FromUnparsedImage, - // and then use types.Image.OCIConfig so that we can parse the image. - // - // In practice, this should, right now, only matter for pulls of OCI images (this code path implies that a layer has annotation), - // while converting to a non-OCI formats, using a manual (skopeo copy) or something similar, not (podman pull). - // So it is not implemented yet. - return "", fmt.Errorf("determining DiffID for manifest type %q is not yet supported", s.manifestMIMEType) - } - man, err := manifest.FromBlob(s.manifest, s.manifestMIMEType) + ctx := context.Background() // This is all happening in memory, no need to worry about cancellation. + unparsed := image.UnparsedInstance(newUncommittedImageSource(s), nil) + sourced, err := image.FromUnparsedImage(ctx, nil, unparsed) if err != nil { - return "", fmt.Errorf("parsing manifest: %w", err) + return "", fmt.Errorf("parsing image to be committed: %w", err) } - - cb, err := s.getConfigBlob(man.ConfigInfo()) + configOCI, err := sourced.OCIConfig(ctx) if err != nil { - return "", err + return "", fmt.Errorf("obtaining config of image to be committed: %w", err) } - // retrieve the expected uncompressed digest from the config blob. - configOCI := &imgspecv1.Image{} - if err := json.Unmarshal(cb, configOCI); err != nil { - return "", err - } s.untrustedDiffIDValues = slices.Clone(configOCI.RootFS.DiffIDs) if s.untrustedDiffIDValues == nil { // Unlikely but possible in theory… s.untrustedDiffIDValues = []digest.Digest{} @@ -1138,7 +1173,21 @@ func (s *storageImageDestination) untrustedLayerDiffID(layerIndex int) (digest.D if layerIndex >= len(s.untrustedDiffIDValues) { return "", fmt.Errorf("image config has only %d DiffID values, but a layer with index %d exists", len(s.untrustedDiffIDValues), layerIndex) } - return s.untrustedDiffIDValues[layerIndex], nil + res := s.untrustedDiffIDValues[layerIndex] + if res == "" { + // In practice, this should, right now, only matter for pulls of OCI images + // (this code path implies that we did a partial pull because a layer has an annotation), + // So, DiffIDs should always be set. + // + // It is, though, reachable by pulling an OCI image while converting to schema1, + // using a manual (skopeo copy) or something similar, not (podman pull). + // + // Our schema1.OCIConfig code produces non-empty DiffID arrays of empty values. + // The current semantics of this function are that ("", nil) means "try again later", + // which is not what we want to happen; for now, turn that into an explicit error. + return "", fmt.Errorf("DiffID value for layer %d is unknown or explicitly empty", layerIndex) + } + return res, nil } // CommitWithOptions marks the process of storing the image as successful and asks for the image to be persisted. From e72e6a3b89a0bb8d778c7508f1d2ade5ebb696b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 18 Nov 2024 21:51:43 +0100 Subject: [PATCH 7/8] Add private.ImageDestination.NoteOriginalOCIConfig MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For now, this only adds the API, nothing actually benefits from it yet. Signed-off-by: Miloslav Trmač --- copy/single.go | 24 ++++++++++++------- directory/directory_dest.go | 1 + docker/docker_image_dest.go | 1 + docker/internal/tarfile/dest.go | 1 + .../stubs/original_oci_config.go | 16 +++++++++++++ internal/imagedestination/wrapper.go | 1 + internal/private/private.go | 7 ++++++ oci/archive/oci_dest.go | 9 +++++++ oci/layout/oci_dest.go | 1 + openshift/openshift_dest.go | 9 +++++++ pkg/blobcache/dest.go | 9 +++++++ storage/storage_dest.go | 8 +++++++ 12 files changed, 79 insertions(+), 8 deletions(-) create mode 100644 internal/imagedestination/stubs/original_oci_config.go diff --git a/copy/single.go b/copy/single.go index e008c7e86f..7bc44d1f83 100644 --- a/copy/single.go +++ b/copy/single.go @@ -109,7 +109,7 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar } } - if err := checkImageDestinationForCurrentRuntime(ctx, c.options.DestinationCtx, src, c.dest); err != nil { + if err := prepareImageConfigForDest(ctx, c.options.DestinationCtx, src, c.dest); err != nil { return copySingleImageResult{}, err } @@ -316,12 +316,15 @@ func (c *copier) copySingleImage(ctx context.Context, unparsedImage *image.Unpar return res, nil } -// checkImageDestinationForCurrentRuntime enforces dest.MustMatchRuntimeOS, if necessary. -func checkImageDestinationForCurrentRuntime(ctx context.Context, sys *types.SystemContext, src types.Image, dest types.ImageDestination) error { +// prepareImageConfigForDest enforces dest.MustMatchRuntimeOS and handles dest.NoteOriginalOCIConfig, if necessary. +func prepareImageConfigForDest(ctx context.Context, sys *types.SystemContext, src types.Image, dest private.ImageDestination) error { + ociConfig, configErr := src.OCIConfig(ctx) + // Do not fail on configErr here, this might be an artifact + // and maybe nothing needs this to be a container image and to process the config. + if dest.MustMatchRuntimeOS() { - c, err := src.OCIConfig(ctx) - if err != nil { - return fmt.Errorf("parsing image configuration: %w", err) + if configErr != nil { + return fmt.Errorf("parsing image configuration: %w", configErr) } wantedPlatforms := platform.WantedPlatforms(sys) @@ -331,7 +334,7 @@ func checkImageDestinationForCurrentRuntime(ctx context.Context, sys *types.Syst // For a transitional period, this might trigger warnings because the Variant // field was added to OCI config only recently. If this turns out to be too noisy, // revert this check to only look for (OS, Architecture). - if platform.MatchesPlatform(c.Platform, wantedPlatform) { + if platform.MatchesPlatform(ociConfig.Platform, wantedPlatform) { match = true break } @@ -339,9 +342,14 @@ func checkImageDestinationForCurrentRuntime(ctx context.Context, sys *types.Syst } if !match { logrus.Infof("Image operating system mismatch: image uses OS %q+architecture %q+%q, expecting one of %q", - c.OS, c.Architecture, c.Variant, strings.Join(options.list, ", ")) + ociConfig.OS, ociConfig.Architecture, ociConfig.Variant, strings.Join(options.list, ", ")) } } + + if err := dest.NoteOriginalOCIConfig(ociConfig, configErr); err != nil { + return err + } + return nil } diff --git a/directory/directory_dest.go b/directory/directory_dest.go index 000c8987d9..6e88aa01d1 100644 --- a/directory/directory_dest.go +++ b/directory/directory_dest.go @@ -29,6 +29,7 @@ var ErrNotContainerImageDir = errors.New("not a containers image directory, don' type dirImageDestination struct { impl.Compat impl.PropertyMethodsInitialize + stubs.IgnoresOriginalOCIConfig stubs.NoPutBlobPartialInitialize stubs.AlwaysSupportsSignatures diff --git a/docker/docker_image_dest.go b/docker/docker_image_dest.go index f5e2bb61e8..3ac43cf9fc 100644 --- a/docker/docker_image_dest.go +++ b/docker/docker_image_dest.go @@ -41,6 +41,7 @@ import ( type dockerImageDestination struct { impl.Compat impl.PropertyMethodsInitialize + stubs.IgnoresOriginalOCIConfig stubs.NoPutBlobPartialInitialize ref dockerReference diff --git a/docker/internal/tarfile/dest.go b/docker/internal/tarfile/dest.go index f142346807..8f5ba7e36e 100644 --- a/docker/internal/tarfile/dest.go +++ b/docker/internal/tarfile/dest.go @@ -24,6 +24,7 @@ import ( type Destination struct { impl.Compat impl.PropertyMethodsInitialize + stubs.IgnoresOriginalOCIConfig stubs.NoPutBlobPartialInitialize stubs.NoSignaturesInitialize diff --git a/internal/imagedestination/stubs/original_oci_config.go b/internal/imagedestination/stubs/original_oci_config.go new file mode 100644 index 0000000000..c4536e933b --- /dev/null +++ b/internal/imagedestination/stubs/original_oci_config.go @@ -0,0 +1,16 @@ +package stubs + +import ( + imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" +) + +// IgnoresOriginalOCIConfig implements NoteOriginalOCIConfig() that does nothing. +type IgnoresOriginalOCIConfig struct{} + +// NoteOriginalOCIConfig provides the config of the image, as it exists on the source, BUT converted to OCI format, +// or an error obtaining that value (e.g. if the image is an artifact and not a container image). +// The destination can use it in its TryReusingBlob/PutBlob implementations +// (otherwise it only obtains the final config after all layers are written). +func (stub IgnoresOriginalOCIConfig) NoteOriginalOCIConfig(ociConfig *imgspecv1.Image, configErr error) error { + return nil +} diff --git a/internal/imagedestination/wrapper.go b/internal/imagedestination/wrapper.go index 0fe902e31f..b2462a3bc1 100644 --- a/internal/imagedestination/wrapper.go +++ b/internal/imagedestination/wrapper.go @@ -14,6 +14,7 @@ import ( // wrapped provides the private.ImageDestination operations // for a destination that only implements types.ImageDestination type wrapped struct { + stubs.IgnoresOriginalOCIConfig stubs.NoPutBlobPartialInitialize types.ImageDestination diff --git a/internal/private/private.go b/internal/private/private.go index 4247a8db77..afd425483d 100644 --- a/internal/private/private.go +++ b/internal/private/private.go @@ -10,6 +10,7 @@ import ( compression "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/types" "github.com/opencontainers/go-digest" + imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" ) // ImageSourceInternalOnly is the part of private.ImageSource that is not @@ -41,6 +42,12 @@ type ImageDestinationInternalOnly interface { // FIXME: Add SupportsSignaturesWithFormat or something like that, to allow early failures // on unsupported formats. + // NoteOriginalOCIConfig provides the config of the image, as it exists on the source, BUT converted to OCI format, + // or an error obtaining that value (e.g. if the image is an artifact and not a container image). + // The destination can use it in its TryReusingBlob/PutBlob implementations + // (otherwise it only obtains the final config after all layers are written). + NoteOriginalOCIConfig(ociConfig *imgspecv1.Image, configErr error) error + // PutBlobWithOptions writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; if provided, and stream is read to the end without error, the digest MUST match the stream contents. // inputInfo.Size is the expected length of stream, if known. diff --git a/oci/archive/oci_dest.go b/oci/archive/oci_dest.go index 1d1e26c84f..54b2e50566 100644 --- a/oci/archive/oci_dest.go +++ b/oci/archive/oci_dest.go @@ -14,6 +14,7 @@ import ( "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/idtools" digest "github.com/opencontainers/go-digest" + imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/sirupsen/logrus" ) @@ -103,6 +104,14 @@ func (d *ociArchiveImageDestination) SupportsPutBlobPartial() bool { return d.unpackedDest.SupportsPutBlobPartial() } +// NoteOriginalOCIConfig provides the config of the image, as it exists on the source, BUT converted to OCI format, +// or an error obtaining that value (e.g. if the image is an artifact and not a container image). +// The destination can use it in its TryReusingBlob/PutBlob implementations +// (otherwise it only obtains the final config after all layers are written). +func (d *ociArchiveImageDestination) NoteOriginalOCIConfig(ociConfig *imgspecv1.Image, configErr error) error { + return d.unpackedDest.NoteOriginalOCIConfig(ociConfig, configErr) +} + // PutBlobWithOptions writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; if provided, and stream is read to the end without error, the digest MUST match the stream contents. // inputInfo.Size is the expected length of stream, if known. diff --git a/oci/layout/oci_dest.go b/oci/layout/oci_dest.go index 85374cecf0..cb2768d745 100644 --- a/oci/layout/oci_dest.go +++ b/oci/layout/oci_dest.go @@ -27,6 +27,7 @@ import ( type ociImageDestination struct { impl.Compat impl.PropertyMethodsInitialize + stubs.IgnoresOriginalOCIConfig stubs.NoPutBlobPartialInitialize stubs.NoSignaturesInitialize diff --git a/openshift/openshift_dest.go b/openshift/openshift_dest.go index 61f69e93fd..bd5e77aa8f 100644 --- a/openshift/openshift_dest.go +++ b/openshift/openshift_dest.go @@ -22,6 +22,7 @@ import ( "github.com/containers/image/v5/manifest" "github.com/containers/image/v5/types" "github.com/opencontainers/go-digest" + imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" ) type openshiftImageDestination struct { @@ -111,6 +112,14 @@ func (d *openshiftImageDestination) SupportsPutBlobPartial() bool { return d.docker.SupportsPutBlobPartial() } +// NoteOriginalOCIConfig provides the config of the image, as it exists on the source, BUT converted to OCI format, +// or an error obtaining that value (e.g. if the image is an artifact and not a container image). +// The destination can use it in its TryReusingBlob/PutBlob implementations +// (otherwise it only obtains the final config after all layers are written). +func (d *openshiftImageDestination) NoteOriginalOCIConfig(ociConfig *imgspecv1.Image, configErr error) error { + return d.docker.NoteOriginalOCIConfig(ociConfig, configErr) +} + // PutBlobWithOptions writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; if provided, and stream is read to the end without error, the digest MUST match the stream contents. // inputInfo.Size is the expected length of stream, if known. diff --git a/pkg/blobcache/dest.go b/pkg/blobcache/dest.go index f7103d13eb..54e3d294b9 100644 --- a/pkg/blobcache/dest.go +++ b/pkg/blobcache/dest.go @@ -19,6 +19,7 @@ import ( "github.com/containers/storage/pkg/archive" "github.com/containers/storage/pkg/ioutils" digest "github.com/opencontainers/go-digest" + imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/sirupsen/logrus" ) @@ -138,6 +139,14 @@ func (d *blobCacheDestination) HasThreadSafePutBlob() bool { return d.destination.HasThreadSafePutBlob() } +// NoteOriginalOCIConfig provides the config of the image, as it exists on the source, BUT converted to OCI format, +// or an error obtaining that value (e.g. if the image is an artifact and not a container image). +// The destination can use it in its TryReusingBlob/PutBlob implementations +// (otherwise it only obtains the final config after all layers are written). +func (d *blobCacheDestination) NoteOriginalOCIConfig(ociConfig *imgspecv1.Image, configErr error) error { + return d.destination.NoteOriginalOCIConfig(ociConfig, configErr) +} + // PutBlobWithOptions writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; if provided, and stream is read to the end without error, the digest MUST match the stream contents. // inputInfo.Size is the expected length of stream, if known. diff --git a/storage/storage_dest.go b/storage/storage_dest.go index e24a3468ee..de6a6d4697 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -205,6 +205,14 @@ func (s *storageImageDestination) computeNextBlobCacheFile() string { return filepath.Join(s.directory, fmt.Sprintf("%d", s.nextTempFileID.Add(1))) } +// NoteOriginalOCIConfig provides the config of the image, as it exists on the source, BUT converted to OCI format, +// or an error obtaining that value (e.g. if the image is an artifact and not a container image). +// The destination can use it in its TryReusingBlob/PutBlob implementations +// (otherwise it only obtains the final config after all layers are written). +func (s *storageImageDestination) NoteOriginalOCIConfig(ociConfig *imgspecv1.Image, configErr error) error { + return nil +} + // PutBlobWithOptions writes contents of stream and returns data representing the result. // inputInfo.Digest can be optionally provided if known; if provided, and stream is read to the end without error, the digest MUST match the stream contents. // inputInfo.Size is the expected length of stream, if known. From 85cee694453dec3d17f2f004e015d6b2a53132d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 18 Nov 2024 21:52:03 +0100 Subject: [PATCH 8/8] Use NoteOriginalOCIConfig in storageImageDestination MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Record DiffIDs early, so that we can commit partially-pulled layers immediately after staging them, and we don't have to wait for PutManifest. Signed-off-by: Miloslav Trmač --- storage/storage_dest.go | 35 ++++++++++++++++++++++++++--------- 1 file changed, 26 insertions(+), 9 deletions(-) diff --git a/storage/storage_dest.go b/storage/storage_dest.go index de6a6d4697..f494d7d4b5 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -210,6 +210,10 @@ func (s *storageImageDestination) computeNextBlobCacheFile() string { // The destination can use it in its TryReusingBlob/PutBlob implementations // (otherwise it only obtains the final config after all layers are written). func (s *storageImageDestination) NoteOriginalOCIConfig(ociConfig *imgspecv1.Image, configErr error) error { + if configErr != nil { + return fmt.Errorf("writing to c/storage without a valid image config: %w", configErr) + } + s.setUntrustedDiffIDValuesFromOCIConfig(ociConfig) return nil } @@ -1154,14 +1158,20 @@ func (u *uncommittedImageSource) GetBlob(ctx context.Context, info types.BlobInf // If the value is not yet available (but it can be available after s.manifets is set), it returns ("", nil). // WARNING: We don’t validate the DiffID value against the layer contents; it must not be used for any deduplication. func (s *storageImageDestination) untrustedLayerDiffID(layerIndex int) (digest.Digest, error) { - // At this point, we are either inside the multi-threaded scope of HasThreadSafePutBlob, and - // nothing is writing to s.manifest yet, or PutManifest has been called and s.manifest != nil. + // At this point, we are either inside the multi-threaded scope of HasThreadSafePutBlob, + // nothing is writing to s.manifest yet, and s.untrustedDiffIDValues might have been set + // by NoteOriginalOCIConfig and are not being updated any more; + // or PutManifest has been called and s.manifest != nil. // Either way this function does not need the protection of s.lock. - if s.manifest == nil { - return "", nil - } if s.untrustedDiffIDValues == nil { + // Typically, we expect untrustedDiffIDValues to be set by the generic copy code + // via NoteOriginalOCIConfig; this is a compatibility fallback for external callers + // of the public types.ImageDestination. + if s.manifest == nil { + return "", nil + } + ctx := context.Background() // This is all happening in memory, no need to worry about cancellation. unparsed := image.UnparsedInstance(newUncommittedImageSource(s), nil) sourced, err := image.FromUnparsedImage(ctx, nil, unparsed) @@ -1173,11 +1183,9 @@ func (s *storageImageDestination) untrustedLayerDiffID(layerIndex int) (digest.D return "", fmt.Errorf("obtaining config of image to be committed: %w", err) } - s.untrustedDiffIDValues = slices.Clone(configOCI.RootFS.DiffIDs) - if s.untrustedDiffIDValues == nil { // Unlikely but possible in theory… - s.untrustedDiffIDValues = []digest.Digest{} - } + s.setUntrustedDiffIDValuesFromOCIConfig(configOCI) } + if layerIndex >= len(s.untrustedDiffIDValues) { return "", fmt.Errorf("image config has only %d DiffID values, but a layer with index %d exists", len(s.untrustedDiffIDValues), layerIndex) } @@ -1198,6 +1206,15 @@ func (s *storageImageDestination) untrustedLayerDiffID(layerIndex int) (digest.D return res, nil } +// setUntrustedDiffIDValuesFromOCIConfig updates s.untrustedDiffIDvalues from config. +// The caller must ensure s.lock does not need to be held. +func (s *storageImageDestination) setUntrustedDiffIDValuesFromOCIConfig(config *imgspecv1.Image) { + s.untrustedDiffIDValues = slices.Clone(config.RootFS.DiffIDs) + if s.untrustedDiffIDValues == nil { // Unlikely but possible in theory… + s.untrustedDiffIDValues = []digest.Digest{} + } +} + // 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