Don't enforce digest match in storage.ReapplyBlob

[nalind]

When reapplying a blob to an image that's being written to the storage transport, don't enforce that the digest of the reapplied blob matches the blobinfo's digest, if there is one, because compression makes the reapply method (where we reextract the layer, and if it was originally compressed, recompress it) produce data with the same actual content, but with a different compressed digest.

This should fix cri-o/cri-o#351.

[mtrmac]

This makes sense in isolation, but we need to figure out how this relates to copy.imageCopier.canModifyManifest (i.e. this would break signatures). It might be possible to (from the user’s POV randomly) fail the copy if ReapplyBlob returns a changed digest and we can’t deal with it, or to skip ReapplyBlob and upload the full blob anyway if !canModifyManifest).

Actually storageImage{Source,Destination} are already maintaining an indirection mapping in Layers, from ddigest.Digest to a layer ID; wouldn’t it be equally possible to just add another mapping from the original digest to a new layer ID, or something like that?

(None of this fixes the difficulty that, IIRC, GetBlob(someDigest) may return a compressed blob with an entirely different digest, to be sure, but that’s somewhat independent. Separately, we have #193, where we have similar difficulties (the DiffID of the uncompressed data is known but the digest of the compressed data is not) and have ended up decompressing all blobs in GetBlob. Perhaps there is something common to the two issues?)

[nalind]

Not sure I follow - ReapplyBlob is only called after HasBlob has returned true, and that only happens when we have a digest specified for the blob. (We know this because HasBlob rejects empty values.) For those cases PutBlob's Layers map is already going to be mapping from the specified digests to the corresponding layer IDs.

[mtrmac]

I’m sorry, I was being stupid; PutBlob/putBlob returns the input digest if it is non-empty, not the recomputed hash. So ReapplyBlob always returns the input BlobInfo.Digest unchanged, and there are no manifest update / signature breakage concerns.

Though, if we are recompressing, it’s not only the digest of the compressed data that change, but the size as well, isn’t it? If so, then the blobInfo.Size check should also be conditional on (some renaming of) enforceDigest.

[nalind]

I would have thought it would, but so far I haven't run into that. But you're right, it would make sense to either have ReapplyBlob() set the size to -1 in the blobinfo that it passes to putBlob(), or as you said, make the size check conditional on the same flag, or both. Do you have a strong preference?

[nalind]

... and if the size we compute is different from an expected value that was passed in, should putBlob be returning that size instead?

[mtrmac]

We still need ReapplyBlob to return the original data unmodified so that the manifest and the layer data match, and signatures are not broken. So, that seems to mean that putBlob should receive the expected size, not -1, and return it unmodified

[nalind]

Makes sense. I'll make the changes.

[nalind]

Patch updated.

[mtrmac]

👍 pending tests.

[runcom]

👍