Skip to content

Commit

Permalink
Clarify the logic of layer reuse restrictions in copyLayer
Browse files Browse the repository at this point in the history
AFAICS encryption doesn't _actually_ need diffIDs, at least I
can't see any encryption-related use of the computed value; the boolean
seems to only be set to avoid reuse (which risks unwanted reuse of
plaintext data, or revealing relationships between ciphertext/plaintext
to network observers).

So, use two extra variables to be clear about the logic of the code.

(Also reorder one condition to check a local variable first before calling
methods, a microoptimizaion.)

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
  • Loading branch information
mtrmac committed Apr 27, 2022
1 parent 0d80718 commit 39dd2f8
Showing 1 changed file with 10 additions and 6 deletions.
16 changes: 10 additions & 6 deletions copy/copy.go
Expand Up @@ -1139,11 +1139,15 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
}

cachedDiffID := ic.c.blobInfoCache.UncompressedDigest(srcInfo.Digest) // May be ""
// Diffs are needed if we are encrypting an image or trying to decrypt an image
diffIDIsNeeded := ic.diffIDsAreNeeded && cachedDiffID == "" || toEncrypt || (isOciEncrypted(srcInfo.MediaType) && ic.c.ociDecryptConfig != nil)

// If we already have the blob, and we don't need to compute the diffID, then we don't need to read it from the source.
if !diffIDIsNeeded {
diffIDIsNeeded := ic.diffIDsAreNeeded && cachedDiffID == ""
// When encrypting to decrypting, only use the simple code path. We might be able to optimize more
// (e.g. if we know the DiffID of an encrypted compressed layer, it might not be necessary to pull, decrypt and decompress again),
// but it’s not trivially safe to do such things, so until someone takes the effort to make a comprehensive argument, let’s not.
encryptingOrDecrypting := toEncrypt || (isOciEncrypted(srcInfo.MediaType) && ic.c.ociDecryptConfig != nil)
canAvoidProcessingCompleteLayer := !diffIDIsNeeded && !encryptingOrDecrypting

// Don’t read the layer form the source with already have the blob, and optimizations are acceptable.
if canAvoidProcessingCompleteLayer {
// TODO: at this point we don't know whether or not a blob we end up reusing is compressed using an algorithm
// that is acceptable for use on layers in the manifest that we'll be writing later, so if we end up reusing
// a blob that's compressed with e.g. zstd, but we're only allowed to write a v2s2 manifest, this will cause
Expand Down Expand Up @@ -1196,7 +1200,7 @@ func (ic *imageCopier) copyLayer(ctx context.Context, srcInfo types.BlobInfo, to
// of the source file are not known yet and must be fetched.
// Attempt a partial only when the source allows to retrieve a blob partially and
// the destination has support for it.
if ic.c.rawSource.SupportsGetBlobAt() && ic.c.dest.SupportsPutBlobPartial() && !diffIDIsNeeded {
if canAvoidProcessingCompleteLayer && ic.c.rawSource.SupportsGetBlobAt() && ic.c.dest.SupportsPutBlobPartial() {
if reused, blobInfo := func() (bool, types.BlobInfo) { // A scope for defer
bar := ic.c.createProgressBar(pool, true, srcInfo, "blob", "done")
hideProgressBar := true
Expand Down

0 comments on commit 39dd2f8

Please sign in to comment.