[sha512] image/copy: allow users to force digest algorithm in CLI (e.g. skopeo copy --force-digest)
#552
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,6 +18,7 @@ import ( | |
| "github.com/sirupsen/logrus" | ||
| "github.com/vbauerster/mpb/v8" | ||
| "go.podman.io/image/v5/docker/reference" | ||
| "go.podman.io/image/v5/internal/digests" | ||
| "go.podman.io/image/v5/internal/image" | ||
| "go.podman.io/image/v5/internal/pkg/platform" | ||
| "go.podman.io/image/v5/internal/private" | ||
|
|
@@ -592,12 +593,27 @@ func (ic *imageCopier) copyUpdatedConfigAndManifest(ctx context.Context, instanc | |
| return nil, "", fmt.Errorf("reading manifest: %w", err) | ||
| } | ||
|
|
||
| newConfigDigest, err := ic.copyConfig(ctx, pendingImage) | ||
| if err != nil { | ||
| return nil, "", err | ||
| } | ||
|
|
||
| // FIXME: Single image only; multi-arch needs per-instance config digest updates. | ||
| // See https://github.com/containers/container-libs/pull/552#discussion_r2611627578 | ||
|
Contributor
There was a problem hiding this comment. I don’t see how this comment matches this code. (And, below, there is |
||
| if newConfigDigest != nil { | ||
| man, err = ic.updateManifestConfigDigest(man, pendingImage, *newConfigDigest) | ||
|
Contributor
There was a problem hiding this comment. The way Instead, we can still edit (Also, conceptually — we have manifestUpdatesForConfig, err := ic.copyConfig() // see elsewhere about closely co-locating the code that changes values and that arranges for manifest updates
if ! /* generalization of noPendingManifestUpdates */(manifestUpdatesForConfig) {
if ic.cannotModifyManifestReason != "" { /* fail, just to be sure */ }
pendingImage, err = pendingImage.UpdatedImage(ctx, manifestUpdatesForConfig)
}
man, _, err := pendingImage.Manifest(ctx)And to implement that, |
||
| if err != nil { | ||
| return nil, "", fmt.Errorf("updating manifest config digest: %w", err) | ||
| } | ||
| } | ||
|
|
||
| ic.c.Printf("Writing manifest to image destination\n") | ||
| // Choose the digest algorithm based on digest options | ||
| manifestDigestAlgo, err := ic.c.options.digestOptions.Choose(digests.Situation{}) | ||
| if err != nil { | ||
| return nil, "", fmt.Errorf("choosing manifest digest algorithm: %w", err) | ||
| } | ||
| manifestDigest, err := manifest.DigestWithAlgorithm(man, manifestDigestAlgo) | ||
|
Comment on lines
+611
to
+616
Contributor
There was a problem hiding this comment. This probably doesn’t work in general — with a sha256-only transport (e.g. an old registry), we would successfully upload the manifest, compute a sha512 digest, put the sha512 digest into a manifest list — but the sha512 digest would not be resolvable by the transport. So we will need some cooperation with the transport to choose a digest, that’s easy enough ( The
Contributor
There was a problem hiding this comment. I have noted some of the registry concerns in https://issues.redhat.com/browse/PROJQUAY-10117 ; there is also pre-existing upstream discussion in opencontainers/distribution-spec#543 and opencontainers/distribution-spec#494 (although I didn’t carefully read either of them).
Contributor
There was a problem hiding this comment. Note in particular opencontainers/distribution-spec#543 (comment) . |
||
| if err != nil { | ||
| return nil, "", err | ||
| } | ||
|
|
@@ -611,13 +627,33 @@ func (ic *imageCopier) copyUpdatedConfigAndManifest(ctx context.Context, instanc | |
| return man, manifestDigest, nil | ||
| } | ||
|
|
||
| // updateManifestConfigDigest updates the config digest in the manifest using the manifest abstraction layer. | ||
| func (ic *imageCopier) updateManifestConfigDigest(manifestBlob []byte, src types.Image, newConfigDigest digest.Digest) ([]byte, error) { | ||
mtrmac marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| _, mt, err := src.Manifest(context.Background()) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("getting manifest type: %w", err) | ||
| } | ||
|
|
||
| m, err := manifest.FromBlob(manifestBlob, mt) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("parsing manifest: %w", err) | ||
| } | ||
|
|
||
| if err := m.UpdateConfigDigest(newConfigDigest); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| return m.Serialize() | ||
| } | ||
|
|
||
| // copyConfig copies config.json, if any, from src to dest. | ||
| // It returns the new config digest if it changed (due to digest algorithm forcing), or nil otherwise. | ||
| func (ic *imageCopier) copyConfig(ctx context.Context, src types.Image) (*digest.Digest, error) { | ||
| srcInfo := src.ConfigInfo() | ||
| if srcInfo.Digest != "" { | ||
| if err := ic.c.concurrentBlobCopiesSemaphore.Acquire(ctx, 1); err != nil { | ||
| // This can only fail with ctx.Err(), so no need to blame acquiring the semaphore. | ||
| return nil, fmt.Errorf("copying config: %w", err) | ||
| } | ||
| defer ic.c.concurrentBlobCopiesSemaphore.Release(1) | ||
|
|
||
|
|
@@ -645,13 +681,17 @@ func (ic *imageCopier) copyConfig(ctx context.Context, src types.Image) error { | |
| return destInfo, nil | ||
| }() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| if destInfo.Digest != srcInfo.Digest { | ||
| // If algorithms match, the whole digest values must match | ||
| if destInfo.Digest.Algorithm() == srcInfo.Digest.Algorithm() { | ||
| return nil, fmt.Errorf("Internal error: copying uncompressed config blob %s changed digest to %s", srcInfo.Digest, destInfo.Digest) | ||
| } | ||
| return &destInfo.Digest, nil | ||
| } | ||
| } | ||
| return nil, nil | ||
| } | ||
|
|
||
| // diffIDResult contains both a digest value and an error from diffIDComputationGoroutine. | ||
|
|
@@ -743,19 +783,34 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to | |
| tocDigest = *d | ||
| } | ||
|
|
||
| // FIXME: Blob reuse disabled when forcing different digest algorithm. | ||
| canTryReuse := true | ||
| if forcedAlgo := ic.c.options.digestOptions.MustUseSet(); forcedAlgo != "" { | ||
|
Contributor
There was a problem hiding this comment. I think this might check for “is not the default |
||
| if srcInfo.Digest.Algorithm() != forcedAlgo { | ||
| logrus.Debugf("Skipping blob reuse for %s: digest algorithm %s doesn't match forced algorithm %s", | ||
| srcInfo.Digest, srcInfo.Digest.Algorithm(), forcedAlgo) | ||
| canTryReuse = false | ||
|
Contributor
There was a problem hiding this comment. This should probably also check |
||
| } | ||
| } | ||
|
|
||
| reused := false | ||
| var reusedBlob private.ReusedBlob | ||
| if canTryReuse { | ||
| var err error | ||
| reused, reusedBlob, err = ic.c.dest.TryReusingBlobWithOptions(ctx, srcInfo, private.TryReusingBlobOptions{ | ||
| Cache: ic.c.blobInfoCache, | ||
| CanSubstitute: canSubstitute, | ||
| EmptyLayer: emptyLayer, | ||
| LayerIndex: &layerIndex, | ||
| SrcRef: srcRef, | ||
| PossibleManifestFormats: append([]string{ic.manifestConversionPlan.preferredMIMEType}, ic.manifestConversionPlan.otherMIMETypeCandidates...), | ||
| RequiredCompression: requiredCompression, | ||
| OriginalCompression: srcInfo.CompressionAlgorithm, | ||
| TOCDigest: tocDigest, | ||
| }) | ||
| if err != nil { | ||
| return types.BlobInfo{}, "", fmt.Errorf("trying to reuse blob %s at destination: %w", srcInfo.Digest, err) | ||
| } | ||
| } | ||
| if reused { | ||
| logrus.Debugf("Skipping blob %s (already present):", srcInfo.Digest) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Conceptually, this goes:
stream.Info.Digestis set (otherwise N/A)Sure, it’s all within effectively a series of
&&, so the ultimate behavior is the same either way, but I think it’s a good habit to only consume values when they are known to exist and to be relevant.