From 84115a5bfe3c2a91abc7ef42ad5842a7e8e055f1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 18:20:47 +0200 Subject: [PATCH 01/35] Move imageCopier.copyBlobFromStream into a new copy/blob.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The over-300-line-long function would benefit from splitting up, and copy/copy.go has grown entirely too long. As a first step, move it to a new copy/blob.go. Only moves unmodified code, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/blob.go | 365 +++++++++++++++++++++++++++++++++++++++++++++++++++ copy/copy.go | 348 ------------------------------------------------ 2 files changed, 365 insertions(+), 348 deletions(-) create mode 100644 copy/blob.go diff --git a/copy/blob.go b/copy/blob.go new file mode 100644 index 000000000..6ac0df78c --- /dev/null +++ b/copy/blob.go @@ -0,0 +1,365 @@ +package copy + +import ( + "context" + "io" + "strings" + + internalblobinfocache "github.com/containers/image/v5/internal/blobinfocache" + "github.com/containers/image/v5/internal/private" + "github.com/containers/image/v5/pkg/compression" + compressiontypes "github.com/containers/image/v5/pkg/compression/types" + "github.com/containers/image/v5/types" + "github.com/containers/ocicrypt" + digest "github.com/opencontainers/go-digest" + imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/pkg/errors" + "github.com/sirupsen/logrus" +) + +// errorAnnotationReader wraps the io.Reader passed to PutBlob for annotating the error happened during read. +// These errors are reported as PutBlob errors, so we would otherwise misleadingly attribute them to the copy destination. +type errorAnnotationReader struct { + reader io.Reader +} + +// Read annotates the error happened during read +func (r errorAnnotationReader) Read(b []byte) (n int, err error) { + n, err = r.reader.Read(b) + if err != io.EOF { + return n, errors.Wrapf(err, "happened during read") + } + return n, err +} + +// copyBlobFromStream copies a blob with srcInfo (with known Digest and Annotations and possibly known Size) from srcStream to dest, +// perhaps sending a copy to an io.Writer if getOriginalLayerCopyWriter != nil, +// perhaps (de/re/)compressing it if canModifyBlob, +// and returns a complete blobInfo of the copied blob. +func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo, + getOriginalLayerCopyWriter func(decompressor compressiontypes.DecompressorFunc) io.Writer, + canModifyBlob bool, isConfig bool, toEncrypt bool, bar *progressBar, layerIndex int, emptyLayer bool) (types.BlobInfo, error) { + if isConfig { // This is guaranteed by the caller, but set it here to be explicit. + canModifyBlob = false + } + + // The copying happens through a pipeline of connected io.Readers. + // === Input: srcStream + + // === Process input through digestingReader to validate against the expected digest. + // Be paranoid; in case PutBlob somehow managed to ignore an error from digestingReader, + // use a separate validation failure indicator. + // Note that for this check we don't use the stronger "validationSucceeded" indicator, because + // dest.PutBlob may detect that the layer already exists, in which case we don't + // read stream to the end, and validation does not happen. + digestingReader, err := newDigestingReader(srcStream, srcInfo.Digest) + if err != nil { + return types.BlobInfo{}, errors.Wrapf(err, "preparing to verify blob %s", srcInfo.Digest) + } + var destStream io.Reader = digestingReader + + // === Update progress bars + destStream = bar.ProxyReader(destStream) + + // === Decrypt the stream, if required. + var decrypted bool + if isOciEncrypted(srcInfo.MediaType) && c.ociDecryptConfig != nil { + newDesc := imgspecv1.Descriptor{ + Annotations: srcInfo.Annotations, + } + + var d digest.Digest + destStream, d, err = ocicrypt.DecryptLayer(c.ociDecryptConfig, destStream, newDesc, false) + if err != nil { + return types.BlobInfo{}, errors.Wrapf(err, "decrypting layer %s", srcInfo.Digest) + } + + srcInfo.Digest = d + srcInfo.Size = -1 + for k := range srcInfo.Annotations { + if strings.HasPrefix(k, "org.opencontainers.image.enc") { + delete(srcInfo.Annotations, k) + } + } + decrypted = true + } + + // === Detect compression of the input stream. + // This requires us to “peek ahead” into the stream to read the initial part, which requires us to chain through another io.Reader returned by DetectCompression. + compressionFormat, decompressor, destStream, err := compression.DetectCompressionFormat(destStream) // We could skip this in some cases, but let's keep the code path uniform + if err != nil { + return types.BlobInfo{}, errors.Wrapf(err, "reading blob %s", srcInfo.Digest) + } + isCompressed := decompressor != nil + if expectedCompressionFormat, known := expectedCompressionFormats[srcInfo.MediaType]; known && isCompressed && compressionFormat.Name() != expectedCompressionFormat.Name() { + logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedCompressionFormat.Name(), compressionFormat.Name()) + } + + // === Send a copy of the original, uncompressed, stream, to a separate path if necessary. + var originalLayerReader io.Reader // DO NOT USE this other than to drain the input if no other consumer in the pipeline has done so. + if getOriginalLayerCopyWriter != nil { + destStream = io.TeeReader(destStream, getOriginalLayerCopyWriter(decompressor)) + originalLayerReader = destStream + } + + compressionMetadata := map[string]string{} + // === Deal with layer compression/decompression if necessary + // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists + // short-circuit conditions + var inputInfo types.BlobInfo + var compressionOperation types.LayerCompression + var uploadCompressionFormat *compressiontypes.Algorithm + srcCompressorName := internalblobinfocache.Uncompressed + if isCompressed { + srcCompressorName = compressionFormat.Name() + } + var uploadCompressorName string + if canModifyBlob && isOciEncrypted(srcInfo.MediaType) { + // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted + logrus.Debugf("Using original blob without modification for encrypted blob") + compressionOperation = types.PreserveOriginal + inputInfo = srcInfo + srcCompressorName = internalblobinfocache.UnknownCompression + uploadCompressionFormat = nil + uploadCompressorName = internalblobinfocache.UnknownCompression + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !isCompressed { + logrus.Debugf("Compressing blob on the fly") + compressionOperation = types.Compress + pipeReader, pipeWriter := io.Pipe() + defer pipeReader.Close() + + if c.compressionFormat != nil { + uploadCompressionFormat = c.compressionFormat + } else { + uploadCompressionFormat = defaultCompressionFormat + } + // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, + // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, + // we don’t care. + go c.compressGoroutine(pipeWriter, destStream, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter + destStream = pipeReader + inputInfo.Digest = "" + inputInfo.Size = -1 + uploadCompressorName = uploadCompressionFormat.Name() + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && isCompressed && + c.compressionFormat != nil && c.compressionFormat.Name() != compressionFormat.Name() { + // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally + // re-compressed using the desired format. + logrus.Debugf("Blob will be converted") + + compressionOperation = types.PreserveOriginal + s, err := decompressor(destStream) + if err != nil { + return types.BlobInfo{}, err + } + defer s.Close() + + pipeReader, pipeWriter := io.Pipe() + defer pipeReader.Close() + + uploadCompressionFormat = c.compressionFormat + go c.compressGoroutine(pipeWriter, s, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter + + destStream = pipeReader + inputInfo.Digest = "" + inputInfo.Size = -1 + uploadCompressorName = uploadCompressionFormat.Name() + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && isCompressed { + logrus.Debugf("Blob will be decompressed") + compressionOperation = types.Decompress + s, err := decompressor(destStream) + if err != nil { + return types.BlobInfo{}, err + } + defer s.Close() + destStream = s + inputInfo.Digest = "" + inputInfo.Size = -1 + uploadCompressionFormat = nil + uploadCompressorName = internalblobinfocache.Uncompressed + } else { + // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. + logrus.Debugf("Using original blob without modification") + compressionOperation = types.PreserveOriginal + inputInfo = srcInfo + // Remember if the original blob was compressed, and if so how, so that if + // LayerInfosForCopy() returned something that differs from what was in the + // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), + // it will be able to correctly derive the MediaType for the copied blob. + if isCompressed { + uploadCompressionFormat = &compressionFormat + } else { + uploadCompressionFormat = nil + } + uploadCompressorName = srcCompressorName + } + + // === Encrypt the stream for valid mediatypes if ociEncryptConfig provided + var ( + encrypted bool + finalizer ocicrypt.EncryptLayerFinalizer + ) + if toEncrypt { + if decrypted { + return types.BlobInfo{}, errors.New("Unable to support both decryption and encryption in the same copy") + } + + if !isOciEncrypted(srcInfo.MediaType) && c.ociEncryptConfig != nil { + var annotations map[string]string + if !decrypted { + annotations = srcInfo.Annotations + } + desc := imgspecv1.Descriptor{ + MediaType: srcInfo.MediaType, + Digest: srcInfo.Digest, + Size: srcInfo.Size, + Annotations: annotations, + } + + s, fin, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, destStream, desc) + if err != nil { + return types.BlobInfo{}, errors.Wrapf(err, "encrypting blob %s", srcInfo.Digest) + } + + destStream = s + finalizer = fin + inputInfo.Digest = "" + inputInfo.Size = -1 + encrypted = true + } + } + + // === Report progress using the c.progress channel, if required. + if c.progress != nil && c.progressInterval > 0 { + progressReader := newProgressReader( + destStream, + c.progress, + c.progressInterval, + srcInfo, + ) + defer progressReader.reportDone() + destStream = progressReader + } + + // === Finally, send the layer stream to dest. + options := private.PutBlobOptions{ + Cache: c.blobInfoCache, + IsConfig: isConfig, + EmptyLayer: emptyLayer, + } + if !isConfig { + options.LayerIndex = &layerIndex + } + uploadedInfo, err := c.dest.PutBlobWithOptions(ctx, &errorAnnotationReader{destStream}, inputInfo, options) + if err != nil { + return types.BlobInfo{}, errors.Wrap(err, "writing blob") + } + + uploadedInfo.Annotations = srcInfo.Annotations + + uploadedInfo.CompressionOperation = compressionOperation + // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. + uploadedInfo.CompressionAlgorithm = uploadCompressionFormat + if decrypted { + uploadedInfo.CryptoOperation = types.Decrypt + } else if encrypted { + encryptAnnotations, err := finalizer() + if err != nil { + return types.BlobInfo{}, errors.Wrap(err, "Unable to finalize encryption") + } + uploadedInfo.CryptoOperation = types.Encrypt + if uploadedInfo.Annotations == nil { + uploadedInfo.Annotations = map[string]string{} + } + for k, v := range encryptAnnotations { + uploadedInfo.Annotations[k] = v + } + } + + // This is fairly horrible: the writer from getOriginalLayerCopyWriter wants to consume + // all of the input (to compute DiffIDs), even if dest.PutBlob does not need it. + // So, read everything from originalLayerReader, which will cause the rest to be + // sent there if we are not already at EOF. + if getOriginalLayerCopyWriter != nil { + logrus.Debugf("Consuming rest of the original blob to satisfy getOriginalLayerCopyWriter") + _, err := io.Copy(io.Discard, originalLayerReader) + if err != nil { + return types.BlobInfo{}, errors.Wrapf(err, "reading input blob %s", srcInfo.Digest) + } + } + + if digestingReader.validationFailed { // Coverage: This should never happen. + return types.BlobInfo{}, errors.Errorf("Internal error writing blob %s, digest verification failed but was ignored", srcInfo.Digest) + } + if inputInfo.Digest != "" && uploadedInfo.Digest != inputInfo.Digest { + return types.BlobInfo{}, errors.Errorf("Internal error writing blob %s, blob with digest %s saved with digest %s", srcInfo.Digest, inputInfo.Digest, uploadedInfo.Digest) + } + if digestingReader.validationSucceeded { + // Don’t record any associations that involve encrypted data. This is a bit crude, + // some blob substitutions (replacing pulls of encrypted data with local reuse of known decryption outcomes) + // might be safe, but it’s not trivially obvious, so let’s be conservative for now. + // This crude approach also means we don’t need to record whether a blob is encrypted + // in the blob info cache (which would probably be necessary for any more complex logic), + // and the simplicity is attractive. + if !encrypted && !decrypted { + // If compressionOperation != types.PreserveOriginal, we now have two reliable digest values: + // srcinfo.Digest describes the pre-compressionOperation input, verified by digestingReader + // uploadedInfo.Digest describes the post-compressionOperation output, computed by PutBlob + // (because inputInfo.Digest == "", this must have been computed afresh). + switch compressionOperation { + case types.PreserveOriginal: + break // Do nothing, we have only one digest and we might not have even verified it. + case types.Compress: + c.blobInfoCache.RecordDigestUncompressedPair(uploadedInfo.Digest, srcInfo.Digest) + case types.Decompress: + c.blobInfoCache.RecordDigestUncompressedPair(srcInfo.Digest, uploadedInfo.Digest) + default: + return types.BlobInfo{}, errors.Errorf("Internal error: Unexpected compressionOperation value %#v", compressionOperation) + } + } + if uploadCompressorName != "" && uploadCompressorName != internalblobinfocache.UnknownCompression { + c.blobInfoCache.RecordDigestCompressorName(uploadedInfo.Digest, uploadCompressorName) + } + if srcInfo.Digest != "" && srcCompressorName != "" && srcCompressorName != internalblobinfocache.UnknownCompression { + c.blobInfoCache.RecordDigestCompressorName(srcInfo.Digest, srcCompressorName) + } + } + + // Copy all the metadata generated by the compressor into the annotations. + if uploadedInfo.Annotations == nil { + uploadedInfo.Annotations = map[string]string{} + } + for k, v := range compressionMetadata { + uploadedInfo.Annotations[k] = v + } + + return uploadedInfo, nil +} + +// doCompression reads all input from src and writes its compressed equivalent to dest. +func doCompression(dest io.Writer, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm, compressionLevel *int) error { + compressor, err := compression.CompressStreamWithMetadata(dest, metadata, compressionFormat, compressionLevel) + if err != nil { + return err + } + + buf := make([]byte, compressionBufferSize) + + _, err = io.CopyBuffer(compressor, src, buf) // Sets err to nil, i.e. causes dest.Close() + if err != nil { + compressor.Close() + return err + } + + return compressor.Close() +} + +// compressGoroutine reads all input from src and writes its compressed equivalent to dest. +func (c *copier) compressGoroutine(dest *io.PipeWriter, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm) { + err := errors.New("Internal error: unexpected panic in compressGoroutine") + defer func() { // Note that this is not the same as {defer dest.CloseWithError(err)}; we need err to be evaluated lazily. + _ = dest.CloseWithError(err) // CloseWithError(nil) is equivalent to Close(), always returns nil + }() + + err = doCompression(dest, src, metadata, compressionFormat, c.compressionLevel) +} diff --git a/copy/copy.go b/copy/copy.go index 8c7941b58..d54859239 100644 --- a/copy/copy.go +++ b/copy/copy.go @@ -25,7 +25,6 @@ import ( "github.com/containers/image/v5/signature" "github.com/containers/image/v5/transports" "github.com/containers/image/v5/types" - "github.com/containers/ocicrypt" encconfig "github.com/containers/ocicrypt/config" digest "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" @@ -1331,350 +1330,3 @@ func computeDiffID(stream io.Reader, decompressor compressiontypes.DecompressorF return digest.Canonical.FromReader(stream) } - -// errorAnnotationReader wraps the io.Reader passed to PutBlob for annotating the error happened during read. -// These errors are reported as PutBlob errors, so we would otherwise misleadingly attribute them to the copy destination. -type errorAnnotationReader struct { - reader io.Reader -} - -// Read annotates the error happened during read -func (r errorAnnotationReader) Read(b []byte) (n int, err error) { - n, err = r.reader.Read(b) - if err != io.EOF { - return n, errors.Wrapf(err, "happened during read") - } - return n, err -} - -// copyBlobFromStream copies a blob with srcInfo (with known Digest and Annotations and possibly known Size) from srcStream to dest, -// perhaps sending a copy to an io.Writer if getOriginalLayerCopyWriter != nil, -// perhaps (de/re/)compressing it if canModifyBlob, -// and returns a complete blobInfo of the copied blob. -func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo, - getOriginalLayerCopyWriter func(decompressor compressiontypes.DecompressorFunc) io.Writer, - canModifyBlob bool, isConfig bool, toEncrypt bool, bar *progressBar, layerIndex int, emptyLayer bool) (types.BlobInfo, error) { - if isConfig { // This is guaranteed by the caller, but set it here to be explicit. - canModifyBlob = false - } - - // The copying happens through a pipeline of connected io.Readers. - // === Input: srcStream - - // === Process input through digestingReader to validate against the expected digest. - // Be paranoid; in case PutBlob somehow managed to ignore an error from digestingReader, - // use a separate validation failure indicator. - // Note that for this check we don't use the stronger "validationSucceeded" indicator, because - // dest.PutBlob may detect that the layer already exists, in which case we don't - // read stream to the end, and validation does not happen. - digestingReader, err := newDigestingReader(srcStream, srcInfo.Digest) - if err != nil { - return types.BlobInfo{}, errors.Wrapf(err, "preparing to verify blob %s", srcInfo.Digest) - } - var destStream io.Reader = digestingReader - - // === Update progress bars - destStream = bar.ProxyReader(destStream) - - // === Decrypt the stream, if required. - var decrypted bool - if isOciEncrypted(srcInfo.MediaType) && c.ociDecryptConfig != nil { - newDesc := imgspecv1.Descriptor{ - Annotations: srcInfo.Annotations, - } - - var d digest.Digest - destStream, d, err = ocicrypt.DecryptLayer(c.ociDecryptConfig, destStream, newDesc, false) - if err != nil { - return types.BlobInfo{}, errors.Wrapf(err, "decrypting layer %s", srcInfo.Digest) - } - - srcInfo.Digest = d - srcInfo.Size = -1 - for k := range srcInfo.Annotations { - if strings.HasPrefix(k, "org.opencontainers.image.enc") { - delete(srcInfo.Annotations, k) - } - } - decrypted = true - } - - // === Detect compression of the input stream. - // This requires us to “peek ahead” into the stream to read the initial part, which requires us to chain through another io.Reader returned by DetectCompression. - compressionFormat, decompressor, destStream, err := compression.DetectCompressionFormat(destStream) // We could skip this in some cases, but let's keep the code path uniform - if err != nil { - return types.BlobInfo{}, errors.Wrapf(err, "reading blob %s", srcInfo.Digest) - } - isCompressed := decompressor != nil - if expectedCompressionFormat, known := expectedCompressionFormats[srcInfo.MediaType]; known && isCompressed && compressionFormat.Name() != expectedCompressionFormat.Name() { - logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedCompressionFormat.Name(), compressionFormat.Name()) - } - - // === Send a copy of the original, uncompressed, stream, to a separate path if necessary. - var originalLayerReader io.Reader // DO NOT USE this other than to drain the input if no other consumer in the pipeline has done so. - if getOriginalLayerCopyWriter != nil { - destStream = io.TeeReader(destStream, getOriginalLayerCopyWriter(decompressor)) - originalLayerReader = destStream - } - - compressionMetadata := map[string]string{} - // === Deal with layer compression/decompression if necessary - // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists - // short-circuit conditions - var inputInfo types.BlobInfo - var compressionOperation types.LayerCompression - var uploadCompressionFormat *compressiontypes.Algorithm - srcCompressorName := internalblobinfocache.Uncompressed - if isCompressed { - srcCompressorName = compressionFormat.Name() - } - var uploadCompressorName string - if canModifyBlob && isOciEncrypted(srcInfo.MediaType) { - // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted - logrus.Debugf("Using original blob without modification for encrypted blob") - compressionOperation = types.PreserveOriginal - inputInfo = srcInfo - srcCompressorName = internalblobinfocache.UnknownCompression - uploadCompressionFormat = nil - uploadCompressorName = internalblobinfocache.UnknownCompression - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !isCompressed { - logrus.Debugf("Compressing blob on the fly") - compressionOperation = types.Compress - pipeReader, pipeWriter := io.Pipe() - defer pipeReader.Close() - - if c.compressionFormat != nil { - uploadCompressionFormat = c.compressionFormat - } else { - uploadCompressionFormat = defaultCompressionFormat - } - // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, - // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, - // we don’t care. - go c.compressGoroutine(pipeWriter, destStream, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter - destStream = pipeReader - inputInfo.Digest = "" - inputInfo.Size = -1 - uploadCompressorName = uploadCompressionFormat.Name() - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && isCompressed && - c.compressionFormat != nil && c.compressionFormat.Name() != compressionFormat.Name() { - // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally - // re-compressed using the desired format. - logrus.Debugf("Blob will be converted") - - compressionOperation = types.PreserveOriginal - s, err := decompressor(destStream) - if err != nil { - return types.BlobInfo{}, err - } - defer s.Close() - - pipeReader, pipeWriter := io.Pipe() - defer pipeReader.Close() - - uploadCompressionFormat = c.compressionFormat - go c.compressGoroutine(pipeWriter, s, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter - - destStream = pipeReader - inputInfo.Digest = "" - inputInfo.Size = -1 - uploadCompressorName = uploadCompressionFormat.Name() - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && isCompressed { - logrus.Debugf("Blob will be decompressed") - compressionOperation = types.Decompress - s, err := decompressor(destStream) - if err != nil { - return types.BlobInfo{}, err - } - defer s.Close() - destStream = s - inputInfo.Digest = "" - inputInfo.Size = -1 - uploadCompressionFormat = nil - uploadCompressorName = internalblobinfocache.Uncompressed - } else { - // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. - logrus.Debugf("Using original blob without modification") - compressionOperation = types.PreserveOriginal - inputInfo = srcInfo - // Remember if the original blob was compressed, and if so how, so that if - // LayerInfosForCopy() returned something that differs from what was in the - // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), - // it will be able to correctly derive the MediaType for the copied blob. - if isCompressed { - uploadCompressionFormat = &compressionFormat - } else { - uploadCompressionFormat = nil - } - uploadCompressorName = srcCompressorName - } - - // === Encrypt the stream for valid mediatypes if ociEncryptConfig provided - var ( - encrypted bool - finalizer ocicrypt.EncryptLayerFinalizer - ) - if toEncrypt { - if decrypted { - return types.BlobInfo{}, errors.New("Unable to support both decryption and encryption in the same copy") - } - - if !isOciEncrypted(srcInfo.MediaType) && c.ociEncryptConfig != nil { - var annotations map[string]string - if !decrypted { - annotations = srcInfo.Annotations - } - desc := imgspecv1.Descriptor{ - MediaType: srcInfo.MediaType, - Digest: srcInfo.Digest, - Size: srcInfo.Size, - Annotations: annotations, - } - - s, fin, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, destStream, desc) - if err != nil { - return types.BlobInfo{}, errors.Wrapf(err, "encrypting blob %s", srcInfo.Digest) - } - - destStream = s - finalizer = fin - inputInfo.Digest = "" - inputInfo.Size = -1 - encrypted = true - } - } - - // === Report progress using the c.progress channel, if required. - if c.progress != nil && c.progressInterval > 0 { - progressReader := newProgressReader( - destStream, - c.progress, - c.progressInterval, - srcInfo, - ) - defer progressReader.reportDone() - destStream = progressReader - } - - // === Finally, send the layer stream to dest. - options := private.PutBlobOptions{ - Cache: c.blobInfoCache, - IsConfig: isConfig, - EmptyLayer: emptyLayer, - } - if !isConfig { - options.LayerIndex = &layerIndex - } - uploadedInfo, err := c.dest.PutBlobWithOptions(ctx, &errorAnnotationReader{destStream}, inputInfo, options) - if err != nil { - return types.BlobInfo{}, errors.Wrap(err, "writing blob") - } - - uploadedInfo.Annotations = srcInfo.Annotations - - uploadedInfo.CompressionOperation = compressionOperation - // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. - uploadedInfo.CompressionAlgorithm = uploadCompressionFormat - if decrypted { - uploadedInfo.CryptoOperation = types.Decrypt - } else if encrypted { - encryptAnnotations, err := finalizer() - if err != nil { - return types.BlobInfo{}, errors.Wrap(err, "Unable to finalize encryption") - } - uploadedInfo.CryptoOperation = types.Encrypt - if uploadedInfo.Annotations == nil { - uploadedInfo.Annotations = map[string]string{} - } - for k, v := range encryptAnnotations { - uploadedInfo.Annotations[k] = v - } - } - - // This is fairly horrible: the writer from getOriginalLayerCopyWriter wants to consume - // all of the input (to compute DiffIDs), even if dest.PutBlob does not need it. - // So, read everything from originalLayerReader, which will cause the rest to be - // sent there if we are not already at EOF. - if getOriginalLayerCopyWriter != nil { - logrus.Debugf("Consuming rest of the original blob to satisfy getOriginalLayerCopyWriter") - _, err := io.Copy(io.Discard, originalLayerReader) - if err != nil { - return types.BlobInfo{}, errors.Wrapf(err, "reading input blob %s", srcInfo.Digest) - } - } - - if digestingReader.validationFailed { // Coverage: This should never happen. - return types.BlobInfo{}, errors.Errorf("Internal error writing blob %s, digest verification failed but was ignored", srcInfo.Digest) - } - if inputInfo.Digest != "" && uploadedInfo.Digest != inputInfo.Digest { - return types.BlobInfo{}, errors.Errorf("Internal error writing blob %s, blob with digest %s saved with digest %s", srcInfo.Digest, inputInfo.Digest, uploadedInfo.Digest) - } - if digestingReader.validationSucceeded { - // Don’t record any associations that involve encrypted data. This is a bit crude, - // some blob substitutions (replacing pulls of encrypted data with local reuse of known decryption outcomes) - // might be safe, but it’s not trivially obvious, so let’s be conservative for now. - // This crude approach also means we don’t need to record whether a blob is encrypted - // in the blob info cache (which would probably be necessary for any more complex logic), - // and the simplicity is attractive. - if !encrypted && !decrypted { - // If compressionOperation != types.PreserveOriginal, we now have two reliable digest values: - // srcinfo.Digest describes the pre-compressionOperation input, verified by digestingReader - // uploadedInfo.Digest describes the post-compressionOperation output, computed by PutBlob - // (because inputInfo.Digest == "", this must have been computed afresh). - switch compressionOperation { - case types.PreserveOriginal: - break // Do nothing, we have only one digest and we might not have even verified it. - case types.Compress: - c.blobInfoCache.RecordDigestUncompressedPair(uploadedInfo.Digest, srcInfo.Digest) - case types.Decompress: - c.blobInfoCache.RecordDigestUncompressedPair(srcInfo.Digest, uploadedInfo.Digest) - default: - return types.BlobInfo{}, errors.Errorf("Internal error: Unexpected compressionOperation value %#v", compressionOperation) - } - } - if uploadCompressorName != "" && uploadCompressorName != internalblobinfocache.UnknownCompression { - c.blobInfoCache.RecordDigestCompressorName(uploadedInfo.Digest, uploadCompressorName) - } - if srcInfo.Digest != "" && srcCompressorName != "" && srcCompressorName != internalblobinfocache.UnknownCompression { - c.blobInfoCache.RecordDigestCompressorName(srcInfo.Digest, srcCompressorName) - } - } - - // Copy all the metadata generated by the compressor into the annotations. - if uploadedInfo.Annotations == nil { - uploadedInfo.Annotations = map[string]string{} - } - for k, v := range compressionMetadata { - uploadedInfo.Annotations[k] = v - } - - return uploadedInfo, nil -} - -// doCompression reads all input from src and writes its compressed equivalent to dest. -func doCompression(dest io.Writer, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm, compressionLevel *int) error { - compressor, err := compression.CompressStreamWithMetadata(dest, metadata, compressionFormat, compressionLevel) - if err != nil { - return err - } - - buf := make([]byte, compressionBufferSize) - - _, err = io.CopyBuffer(compressor, src, buf) // Sets err to nil, i.e. causes dest.Close() - if err != nil { - compressor.Close() - return err - } - - return compressor.Close() -} - -// compressGoroutine reads all input from src and writes its compressed equivalent to dest. -func (c *copier) compressGoroutine(dest *io.PipeWriter, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm) { - err := errors.New("Internal error: unexpected panic in compressGoroutine") - defer func() { // Note that this is not the same as {defer dest.CloseWithError(err)}; we need err to be evaluated lazily. - _ = dest.CloseWithError(err) // CloseWithError(nil) is equivalent to Close(), always returns nil - }() - - err = doCompression(dest, src, metadata, compressionFormat, c.compressionLevel) -} From 345b1c05357e878442ac21d37d832b11202c11d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 18:28:37 +0200 Subject: [PATCH 02/35] Move errorAnnotationReader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that the file reads from top-level callers down to implementation details. Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/blob.go | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index 6ac0df78c..376a5206c 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -17,21 +17,6 @@ import ( "github.com/sirupsen/logrus" ) -// errorAnnotationReader wraps the io.Reader passed to PutBlob for annotating the error happened during read. -// These errors are reported as PutBlob errors, so we would otherwise misleadingly attribute them to the copy destination. -type errorAnnotationReader struct { - reader io.Reader -} - -// Read annotates the error happened during read -func (r errorAnnotationReader) Read(b []byte) (n int, err error) { - n, err = r.reader.Read(b) - if err != io.EOF { - return n, errors.Wrapf(err, "happened during read") - } - return n, err -} - // copyBlobFromStream copies a blob with srcInfo (with known Digest and Annotations and possibly known Size) from srcStream to dest, // perhaps sending a copy to an io.Writer if getOriginalLayerCopyWriter != nil, // perhaps (de/re/)compressing it if canModifyBlob, @@ -336,6 +321,21 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr return uploadedInfo, nil } +// errorAnnotationReader wraps the io.Reader passed to PutBlob for annotating the error happened during read. +// These errors are reported as PutBlob errors, so we would otherwise misleadingly attribute them to the copy destination. +type errorAnnotationReader struct { + reader io.Reader +} + +// Read annotates the error happened during read +func (r errorAnnotationReader) Read(b []byte) (n int, err error) { + n, err = r.reader.Read(b) + if err != io.EOF { + return n, errors.Wrapf(err, "happened during read") + } + return n, err +} + // doCompression reads all input from src and writes its compressed equivalent to dest. func doCompression(dest io.Writer, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm, compressionLevel *int) error { compressor, err := compression.CompressStreamWithMetadata(dest, metadata, compressionFormat, compressionLevel) From 52c06a9bfeb3ea1e42c64c00224db3b9bf74db39 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 18:34:18 +0200 Subject: [PATCH 03/35] Rename the srcStream parameter to srcReader MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We will use srcStream for another purpose. Signed-off-by: Miloslav Trmač --- copy/blob.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index 376a5206c..0738d4748 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -17,11 +17,11 @@ import ( "github.com/sirupsen/logrus" ) -// copyBlobFromStream copies a blob with srcInfo (with known Digest and Annotations and possibly known Size) from srcStream to dest, +// copyBlobFromStream copies a blob with srcInfo (with known Digest and Annotations and possibly known Size) from srcReader to dest, // perhaps sending a copy to an io.Writer if getOriginalLayerCopyWriter != nil, // perhaps (de/re/)compressing it if canModifyBlob, // and returns a complete blobInfo of the copied blob. -func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, srcInfo types.BlobInfo, +func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, srcInfo types.BlobInfo, getOriginalLayerCopyWriter func(decompressor compressiontypes.DecompressorFunc) io.Writer, canModifyBlob bool, isConfig bool, toEncrypt bool, bar *progressBar, layerIndex int, emptyLayer bool) (types.BlobInfo, error) { if isConfig { // This is guaranteed by the caller, but set it here to be explicit. @@ -29,7 +29,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr } // The copying happens through a pipeline of connected io.Readers. - // === Input: srcStream + // === Input: srcReader // === Process input through digestingReader to validate against the expected digest. // Be paranoid; in case PutBlob somehow managed to ignore an error from digestingReader, @@ -37,7 +37,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcStream io.Reader, sr // Note that for this check we don't use the stronger "validationSucceeded" indicator, because // dest.PutBlob may detect that the layer already exists, in which case we don't // read stream to the end, and validation does not happen. - digestingReader, err := newDigestingReader(srcStream, srcInfo.Digest) + digestingReader, err := newDigestingReader(srcReader, srcInfo.Digest) if err != nil { return types.BlobInfo{}, errors.Wrapf(err, "preparing to verify blob %s", srcInfo.Digest) } From 54c4a9e059a2650ecf2c4546565ed49939182d6d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 18:45:36 +0200 Subject: [PATCH 04/35] Introduce sourceStream, use it for the first pipeline stage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The purpose of sourceStream is to make it a bit easier to split the blob copy pipeline stages into separate functions; they will update a *sourceStream, which we can pass a single parameter. For now, build it and use it to initialize a digestingReader; then update other code that should consume the current pipeline data. NOTE: That is a judgement call, it is not always obvious what data was intended to be used and what is a bug. As a geneeral principle, use srcInfo (the original unmodified data) in user-visible error reports, notably using the original digest. Otherwise, lean towards using srcStream.info. Signed-off-by: Miloslav Trmač --- copy/blob.go | 42 +++++++++++++++++++++++++++++------------- 1 file changed, 29 insertions(+), 13 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index 0738d4748..ed64c2968 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -28,8 +28,13 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr canModifyBlob = false } - // The copying happens through a pipeline of connected io.Readers. + // The copying happens through a pipeline of connected io.Readers; + // that pipeline is built by updating stream. // === Input: srcReader + stream := sourceStream{ + reader: srcReader, + info: srcInfo, + } // === Process input through digestingReader to validate against the expected digest. // Be paranoid; in case PutBlob somehow managed to ignore an error from digestingReader, @@ -37,7 +42,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // Note that for this check we don't use the stronger "validationSucceeded" indicator, because // dest.PutBlob may detect that the layer already exists, in which case we don't // read stream to the end, and validation does not happen. - digestingReader, err := newDigestingReader(srcReader, srcInfo.Digest) + digestingReader, err := newDigestingReader(stream.reader, srcInfo.Digest) if err != nil { return types.BlobInfo{}, errors.Wrapf(err, "preparing to verify blob %s", srcInfo.Digest) } @@ -48,9 +53,9 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // === Decrypt the stream, if required. var decrypted bool - if isOciEncrypted(srcInfo.MediaType) && c.ociDecryptConfig != nil { + if isOciEncrypted(stream.info.MediaType) && c.ociDecryptConfig != nil { newDesc := imgspecv1.Descriptor{ - Annotations: srcInfo.Annotations, + Annotations: stream.info.Annotations, } var d digest.Digest @@ -59,11 +64,11 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr return types.BlobInfo{}, errors.Wrapf(err, "decrypting layer %s", srcInfo.Digest) } - srcInfo.Digest = d - srcInfo.Size = -1 - for k := range srcInfo.Annotations { + stream.info.Digest = d + stream.info.Size = -1 + for k := range stream.info.Annotations { if strings.HasPrefix(k, "org.opencontainers.image.enc") { - delete(srcInfo.Annotations, k) + delete(stream.info.Annotations, k) } } decrypted = true @@ -76,7 +81,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr return types.BlobInfo{}, errors.Wrapf(err, "reading blob %s", srcInfo.Digest) } isCompressed := decompressor != nil - if expectedCompressionFormat, known := expectedCompressionFormats[srcInfo.MediaType]; known && isCompressed && compressionFormat.Name() != expectedCompressionFormat.Name() { + if expectedCompressionFormat, known := expectedCompressionFormats[stream.info.MediaType]; known && isCompressed && compressionFormat.Name() != expectedCompressionFormat.Name() { logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedCompressionFormat.Name(), compressionFormat.Name()) } @@ -99,11 +104,11 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr srcCompressorName = compressionFormat.Name() } var uploadCompressorName string - if canModifyBlob && isOciEncrypted(srcInfo.MediaType) { + if canModifyBlob && isOciEncrypted(stream.info.MediaType) { // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted logrus.Debugf("Using original blob without modification for encrypted blob") compressionOperation = types.PreserveOriginal - inputInfo = srcInfo + inputInfo = stream.info srcCompressorName = internalblobinfocache.UnknownCompression uploadCompressionFormat = nil uploadCompressorName = internalblobinfocache.UnknownCompression @@ -166,7 +171,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. logrus.Debugf("Using original blob without modification") compressionOperation = types.PreserveOriginal - inputInfo = srcInfo + inputInfo = stream.info // Remember if the original blob was compressed, and if so how, so that if // LayerInfosForCopy() returned something that differs from what was in the // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), @@ -240,7 +245,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr return types.BlobInfo{}, errors.Wrap(err, "writing blob") } - uploadedInfo.Annotations = srcInfo.Annotations + uploadedInfo.Annotations = stream.info.Annotations uploadedInfo.CompressionOperation = compressionOperation // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. @@ -321,6 +326,17 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr return uploadedInfo, nil } +// sourceStream encapsulates an input consumed by copyBlobFromStream, in progress of being built. +// This allows handles of individual aspects to build the copy pipeline without _too much_ +// specific cooperation by the caller. +// +// We are currently very far from a generalized plug-and-play API for building/consuming the pipeline +// without specific knowledge of various aspects in copyBlobFromStream; that may come one day. +type sourceStream struct { + reader io.Reader + info types.BlobInfo // corresponding to the data available in reader. +} + // errorAnnotationReader wraps the io.Reader passed to PutBlob for annotating the error happened during read. // These errors are reported as PutBlob errors, so we would otherwise misleadingly attribute them to the copy destination. type errorAnnotationReader struct { From acdd0362761a90fc0f3a80355ecd9e3521906575 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 18:56:05 +0200 Subject: [PATCH 05/35] Use stream.reader instead of destStream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit One part of moving to use stream throughout the pipeline. Signed-off-by: Miloslav Trmač --- copy/blob.go | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index ed64c2968..e423ec581 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -46,10 +46,10 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr if err != nil { return types.BlobInfo{}, errors.Wrapf(err, "preparing to verify blob %s", srcInfo.Digest) } - var destStream io.Reader = digestingReader + stream.reader = digestingReader // === Update progress bars - destStream = bar.ProxyReader(destStream) + stream.reader = bar.ProxyReader(stream.reader) // === Decrypt the stream, if required. var decrypted bool @@ -59,7 +59,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr } var d digest.Digest - destStream, d, err = ocicrypt.DecryptLayer(c.ociDecryptConfig, destStream, newDesc, false) + stream.reader, d, err = ocicrypt.DecryptLayer(c.ociDecryptConfig, stream.reader, newDesc, false) if err != nil { return types.BlobInfo{}, errors.Wrapf(err, "decrypting layer %s", srcInfo.Digest) } @@ -76,10 +76,11 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // === Detect compression of the input stream. // This requires us to “peek ahead” into the stream to read the initial part, which requires us to chain through another io.Reader returned by DetectCompression. - compressionFormat, decompressor, destStream, err := compression.DetectCompressionFormat(destStream) // We could skip this in some cases, but let's keep the code path uniform + compressionFormat, decompressor, detectCompressionFormatReader, err := compression.DetectCompressionFormat(stream.reader) // We could skip this in some cases, but let's keep the code path uniform if err != nil { return types.BlobInfo{}, errors.Wrapf(err, "reading blob %s", srcInfo.Digest) } + stream.reader = detectCompressionFormatReader isCompressed := decompressor != nil if expectedCompressionFormat, known := expectedCompressionFormats[stream.info.MediaType]; known && isCompressed && compressionFormat.Name() != expectedCompressionFormat.Name() { logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedCompressionFormat.Name(), compressionFormat.Name()) @@ -88,8 +89,8 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // === Send a copy of the original, uncompressed, stream, to a separate path if necessary. var originalLayerReader io.Reader // DO NOT USE this other than to drain the input if no other consumer in the pipeline has done so. if getOriginalLayerCopyWriter != nil { - destStream = io.TeeReader(destStream, getOriginalLayerCopyWriter(decompressor)) - originalLayerReader = destStream + stream.reader = io.TeeReader(stream.reader, getOriginalLayerCopyWriter(decompressor)) + originalLayerReader = stream.reader } compressionMetadata := map[string]string{} @@ -126,8 +127,8 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, // we don’t care. - go c.compressGoroutine(pipeWriter, destStream, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter - destStream = pipeReader + go c.compressGoroutine(pipeWriter, stream.reader, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter + stream.reader = pipeReader inputInfo.Digest = "" inputInfo.Size = -1 uploadCompressorName = uploadCompressionFormat.Name() @@ -138,7 +139,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr logrus.Debugf("Blob will be converted") compressionOperation = types.PreserveOriginal - s, err := decompressor(destStream) + s, err := decompressor(stream.reader) if err != nil { return types.BlobInfo{}, err } @@ -150,19 +151,19 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr uploadCompressionFormat = c.compressionFormat go c.compressGoroutine(pipeWriter, s, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter - destStream = pipeReader + stream.reader = pipeReader inputInfo.Digest = "" inputInfo.Size = -1 uploadCompressorName = uploadCompressionFormat.Name() } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && isCompressed { logrus.Debugf("Blob will be decompressed") compressionOperation = types.Decompress - s, err := decompressor(destStream) + s, err := decompressor(stream.reader) if err != nil { return types.BlobInfo{}, err } defer s.Close() - destStream = s + stream.reader = s inputInfo.Digest = "" inputInfo.Size = -1 uploadCompressionFormat = nil @@ -206,12 +207,12 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr Annotations: annotations, } - s, fin, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, destStream, desc) + s, fin, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, stream.reader, desc) if err != nil { return types.BlobInfo{}, errors.Wrapf(err, "encrypting blob %s", srcInfo.Digest) } - destStream = s + stream.reader = s finalizer = fin inputInfo.Digest = "" inputInfo.Size = -1 @@ -222,13 +223,13 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // === Report progress using the c.progress channel, if required. if c.progress != nil && c.progressInterval > 0 { progressReader := newProgressReader( - destStream, + stream.reader, c.progress, c.progressInterval, srcInfo, ) defer progressReader.reportDone() - destStream = progressReader + stream.reader = progressReader } // === Finally, send the layer stream to dest. @@ -240,7 +241,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr if !isConfig { options.LayerIndex = &layerIndex } - uploadedInfo, err := c.dest.PutBlobWithOptions(ctx, &errorAnnotationReader{destStream}, inputInfo, options) + uploadedInfo, err := c.dest.PutBlobWithOptions(ctx, &errorAnnotationReader{stream.reader}, inputInfo, options) if err != nil { return types.BlobInfo{}, errors.Wrap(err, "writing blob") } From 2ee1587808c60b510d7ba533f55bf1c4c71f5192 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 19:02:53 +0200 Subject: [PATCH 06/35] Eliminate inputInfo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use stream.info throughout. Should not change behavior. (In some cases that's dubious, we are currently losing data like annotations compression/decompression. This commit doesn't change that, only adds a FIXME? to eventually review.) Signed-off-by: Miloslav Trmač --- copy/blob.go | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index e423ec581..c5ece25f4 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -97,7 +97,6 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // === Deal with layer compression/decompression if necessary // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions - var inputInfo types.BlobInfo var compressionOperation types.LayerCompression var uploadCompressionFormat *compressiontypes.Algorithm srcCompressorName := internalblobinfocache.Uncompressed @@ -109,7 +108,6 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted logrus.Debugf("Using original blob without modification for encrypted blob") compressionOperation = types.PreserveOriginal - inputInfo = stream.info srcCompressorName = internalblobinfocache.UnknownCompression uploadCompressionFormat = nil uploadCompressorName = internalblobinfocache.UnknownCompression @@ -129,8 +127,10 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // we don’t care. go c.compressGoroutine(pipeWriter, stream.reader, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter stream.reader = pipeReader - inputInfo.Digest = "" - inputInfo.Size = -1 + stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? + Digest: "", + Size: -1, + } uploadCompressorName = uploadCompressionFormat.Name() } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && isCompressed && c.compressionFormat != nil && c.compressionFormat.Name() != compressionFormat.Name() { @@ -152,8 +152,10 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr go c.compressGoroutine(pipeWriter, s, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter stream.reader = pipeReader - inputInfo.Digest = "" - inputInfo.Size = -1 + stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? + Digest: "", + Size: -1, + } uploadCompressorName = uploadCompressionFormat.Name() } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && isCompressed { logrus.Debugf("Blob will be decompressed") @@ -164,15 +166,16 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr } defer s.Close() stream.reader = s - inputInfo.Digest = "" - inputInfo.Size = -1 + stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? + Digest: "", + Size: -1, + } uploadCompressionFormat = nil uploadCompressorName = internalblobinfocache.Uncompressed } else { // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. logrus.Debugf("Using original blob without modification") compressionOperation = types.PreserveOriginal - inputInfo = stream.info // Remember if the original blob was compressed, and if so how, so that if // LayerInfosForCopy() returned something that differs from what was in the // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), @@ -212,10 +215,10 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr return types.BlobInfo{}, errors.Wrapf(err, "encrypting blob %s", srcInfo.Digest) } - stream.reader = s finalizer = fin - inputInfo.Digest = "" - inputInfo.Size = -1 + stream.reader = s + stream.info.Digest = "" + stream.info.Size = -1 encrypted = true } } @@ -241,7 +244,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr if !isConfig { options.LayerIndex = &layerIndex } - uploadedInfo, err := c.dest.PutBlobWithOptions(ctx, &errorAnnotationReader{stream.reader}, inputInfo, options) + uploadedInfo, err := c.dest.PutBlobWithOptions(ctx, &errorAnnotationReader{stream.reader}, stream.info, options) if err != nil { return types.BlobInfo{}, errors.Wrap(err, "writing blob") } @@ -282,8 +285,8 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr if digestingReader.validationFailed { // Coverage: This should never happen. return types.BlobInfo{}, errors.Errorf("Internal error writing blob %s, digest verification failed but was ignored", srcInfo.Digest) } - if inputInfo.Digest != "" && uploadedInfo.Digest != inputInfo.Digest { - return types.BlobInfo{}, errors.Errorf("Internal error writing blob %s, blob with digest %s saved with digest %s", srcInfo.Digest, inputInfo.Digest, uploadedInfo.Digest) + if stream.info.Digest != "" && uploadedInfo.Digest != stream.info.Digest { + return types.BlobInfo{}, errors.Errorf("Internal error writing blob %s, blob with digest %s saved with digest %s", srcInfo.Digest, stream.info.Digest, uploadedInfo.Digest) } if digestingReader.validationSucceeded { // Don’t record any associations that involve encrypted data. This is a bit crude, @@ -296,7 +299,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // If compressionOperation != types.PreserveOriginal, we now have two reliable digest values: // srcinfo.Digest describes the pre-compressionOperation input, verified by digestingReader // uploadedInfo.Digest describes the post-compressionOperation output, computed by PutBlob - // (because inputInfo.Digest == "", this must have been computed afresh). + // (because stream.info.Digest == "", this must have been computed afresh). switch compressionOperation { case types.PreserveOriginal: break // Do nothing, we have only one digest and we might not have even verified it. From 11c0fe0f8f85a1ecbb9ab210e2f70feb1be65314 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 19:09:24 +0200 Subject: [PATCH 07/35] Move the OCI blob decryption pipeline step into copier.blobPipelineDecryptionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only moves the code with minimal necessary adjustments, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/blob.go | 36 ++++++++---------------------------- copy/encrypt.go | 47 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 28 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index c5ece25f4..90d9761e2 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -3,7 +3,6 @@ package copy import ( "context" "io" - "strings" internalblobinfocache "github.com/containers/image/v5/internal/blobinfocache" "github.com/containers/image/v5/internal/private" @@ -11,7 +10,6 @@ import ( compressiontypes "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/types" "github.com/containers/ocicrypt" - digest "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" @@ -52,26 +50,9 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr stream.reader = bar.ProxyReader(stream.reader) // === Decrypt the stream, if required. - var decrypted bool - if isOciEncrypted(stream.info.MediaType) && c.ociDecryptConfig != nil { - newDesc := imgspecv1.Descriptor{ - Annotations: stream.info.Annotations, - } - - var d digest.Digest - stream.reader, d, err = ocicrypt.DecryptLayer(c.ociDecryptConfig, stream.reader, newDesc, false) - if err != nil { - return types.BlobInfo{}, errors.Wrapf(err, "decrypting layer %s", srcInfo.Digest) - } - - stream.info.Digest = d - stream.info.Size = -1 - for k := range stream.info.Annotations { - if strings.HasPrefix(k, "org.opencontainers.image.enc") { - delete(stream.info.Annotations, k) - } - } - decrypted = true + decryptionStep, err := c.blobPipelineDecryptionStep(&stream, srcInfo) + if err != nil { + return types.BlobInfo{}, err } // === Detect compression of the input stream. @@ -194,13 +175,13 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr finalizer ocicrypt.EncryptLayerFinalizer ) if toEncrypt { - if decrypted { + if decryptionStep.decrypting { return types.BlobInfo{}, errors.New("Unable to support both decryption and encryption in the same copy") } if !isOciEncrypted(srcInfo.MediaType) && c.ociEncryptConfig != nil { var annotations map[string]string - if !decrypted { + if !decryptionStep.decrypting { annotations = srcInfo.Annotations } desc := imgspecv1.Descriptor{ @@ -254,9 +235,8 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr uploadedInfo.CompressionOperation = compressionOperation // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. uploadedInfo.CompressionAlgorithm = uploadCompressionFormat - if decrypted { - uploadedInfo.CryptoOperation = types.Decrypt - } else if encrypted { + decryptionStep.updateCryptoOperation(&uploadedInfo.CryptoOperation) + if encrypted { encryptAnnotations, err := finalizer() if err != nil { return types.BlobInfo{}, errors.Wrap(err, "Unable to finalize encryption") @@ -295,7 +275,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // This crude approach also means we don’t need to record whether a blob is encrypted // in the blob info cache (which would probably be necessary for any more complex logic), // and the simplicity is attractive. - if !encrypted && !decrypted { + if !encrypted && !decryptionStep.decrypting { // If compressionOperation != types.PreserveOriginal, we now have two reliable digest values: // srcinfo.Digest describes the pre-compressionOperation input, verified by digestingReader // uploadedInfo.Digest describes the post-compressionOperation output, computed by PutBlob diff --git a/copy/encrypt.go b/copy/encrypt.go index a18d6f151..967042c53 100644 --- a/copy/encrypt.go +++ b/copy/encrypt.go @@ -4,6 +4,10 @@ import ( "strings" "github.com/containers/image/v5/types" + "github.com/containers/ocicrypt" + digest "github.com/opencontainers/go-digest" + imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/pkg/errors" ) // isOciEncrypted returns a bool indicating if a mediatype is encrypted @@ -22,3 +26,46 @@ func isEncrypted(i types.Image) bool { } return false } + +// bpDecryptionStepData contains data that the copy pipeline needs about the decryption step. +type bpDecryptionStepData struct { + decrypting bool // We are actually decrypting the stream +} + +// blobPipelineDecryptionStep updates *stream to decrypt if, it necessary. +// srcInfo is only used for error messages. +// Returns data for other steps; the caller should eventually use updateCryptoOperation. +func (c *copier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types.BlobInfo) (*bpDecryptionStepData, error) { + var decrypted bool + if isOciEncrypted(stream.info.MediaType) && c.ociDecryptConfig != nil { + newDesc := imgspecv1.Descriptor{ + Annotations: stream.info.Annotations, + } + + var d digest.Digest + reader, d, err := ocicrypt.DecryptLayer(c.ociDecryptConfig, stream.reader, newDesc, false) + if err != nil { + return nil, errors.Wrapf(err, "decrypting layer %s", srcInfo.Digest) + } + stream.reader = reader + + stream.info.Digest = d + stream.info.Size = -1 + for k := range stream.info.Annotations { + if strings.HasPrefix(k, "org.opencontainers.image.enc") { + delete(stream.info.Annotations, k) + } + } + decrypted = true + } + return &bpDecryptionStepData{ + decrypting: decrypted, + }, nil +} + +// updateCryptoOperation sets *operation, if necessary. +func (d *bpDecryptionStepData) updateCryptoOperation(operation *types.LayerCrypto) { + if d.decrypting { + *operation = types.Decrypt + } +} From 338d64d3c5def98edefc812534b64efeeb20f422 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 19:11:29 +0200 Subject: [PATCH 08/35] Beautify blobPipelineDecryptionStep a bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/encrypt.go | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/copy/encrypt.go b/copy/encrypt.go index 967042c53..e4b123588 100644 --- a/copy/encrypt.go +++ b/copy/encrypt.go @@ -36,7 +36,6 @@ type bpDecryptionStepData struct { // srcInfo is only used for error messages. // Returns data for other steps; the caller should eventually use updateCryptoOperation. func (c *copier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types.BlobInfo) (*bpDecryptionStepData, error) { - var decrypted bool if isOciEncrypted(stream.info.MediaType) && c.ociDecryptConfig != nil { newDesc := imgspecv1.Descriptor{ Annotations: stream.info.Annotations, @@ -56,10 +55,12 @@ func (c *copier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types. delete(stream.info.Annotations, k) } } - decrypted = true + return &bpDecryptionStepData{ + decrypting: true, + }, nil } return &bpDecryptionStepData{ - decrypting: decrypted, + decrypting: false, }, nil } From 5472fa9c2ca7c7f6692763eaf6f787362d361e88 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 19:12:36 +0200 Subject: [PATCH 09/35] Rename newDesc to desc MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This smaller function can use simpler identifiers. Signed-off-by: Miloslav Trmač --- copy/encrypt.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/copy/encrypt.go b/copy/encrypt.go index e4b123588..d0abddb37 100644 --- a/copy/encrypt.go +++ b/copy/encrypt.go @@ -37,12 +37,12 @@ type bpDecryptionStepData struct { // Returns data for other steps; the caller should eventually use updateCryptoOperation. func (c *copier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types.BlobInfo) (*bpDecryptionStepData, error) { if isOciEncrypted(stream.info.MediaType) && c.ociDecryptConfig != nil { - newDesc := imgspecv1.Descriptor{ + desc := imgspecv1.Descriptor{ Annotations: stream.info.Annotations, } var d digest.Digest - reader, d, err := ocicrypt.DecryptLayer(c.ociDecryptConfig, stream.reader, newDesc, false) + reader, d, err := ocicrypt.DecryptLayer(c.ociDecryptConfig, stream.reader, desc, false) if err != nil { return nil, errors.Wrapf(err, "decrypting layer %s", srcInfo.Digest) } From 651679a6f728f8f3f8203fbcfb8f65aa14a1cae1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 19:13:42 +0200 Subject: [PATCH 10/35] Beautify the DecryptLayer usage a bit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/encrypt.go | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/copy/encrypt.go b/copy/encrypt.go index d0abddb37..1f9f117e2 100644 --- a/copy/encrypt.go +++ b/copy/encrypt.go @@ -5,7 +5,6 @@ import ( "github.com/containers/image/v5/types" "github.com/containers/ocicrypt" - digest "github.com/opencontainers/go-digest" imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" ) @@ -40,15 +39,13 @@ func (c *copier) blobPipelineDecryptionStep(stream *sourceStream, srcInfo types. desc := imgspecv1.Descriptor{ Annotations: stream.info.Annotations, } - - var d digest.Digest - reader, d, err := ocicrypt.DecryptLayer(c.ociDecryptConfig, stream.reader, desc, false) + reader, decryptedDigest, err := ocicrypt.DecryptLayer(c.ociDecryptConfig, stream.reader, desc, false) if err != nil { return nil, errors.Wrapf(err, "decrypting layer %s", srcInfo.Digest) } - stream.reader = reader - stream.info.Digest = d + stream.reader = reader + stream.info.Digest = decryptedDigest stream.info.Size = -1 for k := range stream.info.Annotations { if strings.HasPrefix(k, "org.opencontainers.image.enc") { From 8dbbe7c3ed1dac9c9fdc791763e305a3c3c1b222 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 19:49:13 +0200 Subject: [PATCH 11/35] Move the OCI blob encryption pipeline step into copier.blobPipelineEncryptionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only moves the code with minimal necessary adjustments, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/blob.go | 58 +++++++++------------------------------------ copy/encrypt.go | 62 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 73 insertions(+), 47 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index 90d9761e2..3035828b2 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -9,8 +9,6 @@ import ( "github.com/containers/image/v5/pkg/compression" compressiontypes "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/types" - "github.com/containers/ocicrypt" - imgspecv1 "github.com/opencontainers/image-spec/specs-go/v1" "github.com/pkg/errors" "github.com/sirupsen/logrus" ) @@ -170,38 +168,14 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr } // === Encrypt the stream for valid mediatypes if ociEncryptConfig provided - var ( - encrypted bool - finalizer ocicrypt.EncryptLayerFinalizer - ) - if toEncrypt { - if decryptionStep.decrypting { - return types.BlobInfo{}, errors.New("Unable to support both decryption and encryption in the same copy") - } - - if !isOciEncrypted(srcInfo.MediaType) && c.ociEncryptConfig != nil { - var annotations map[string]string - if !decryptionStep.decrypting { - annotations = srcInfo.Annotations - } - desc := imgspecv1.Descriptor{ - MediaType: srcInfo.MediaType, - Digest: srcInfo.Digest, - Size: srcInfo.Size, - Annotations: annotations, - } - - s, fin, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, stream.reader, desc) - if err != nil { - return types.BlobInfo{}, errors.Wrapf(err, "encrypting blob %s", srcInfo.Digest) - } - - finalizer = fin - stream.reader = s - stream.info.Digest = "" - stream.info.Size = -1 - encrypted = true - } + if decryptionStep.decrypting && toEncrypt { + // If nothing else, we can only set uploadedInfo.CryptoOperation to a single value. + // Before relaxing this, see the original pull request’s review if there are other reasons to reject this. + return types.BlobInfo{}, errors.New("Unable to support both decryption and encryption in the same copy") + } + encryptionStep, err := c.blobPipelineEncryptionStep(&stream, toEncrypt, srcInfo, decryptionStep) + if err != nil { + return types.BlobInfo{}, err } // === Report progress using the c.progress channel, if required. @@ -236,18 +210,8 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. uploadedInfo.CompressionAlgorithm = uploadCompressionFormat decryptionStep.updateCryptoOperation(&uploadedInfo.CryptoOperation) - if encrypted { - encryptAnnotations, err := finalizer() - if err != nil { - return types.BlobInfo{}, errors.Wrap(err, "Unable to finalize encryption") - } - uploadedInfo.CryptoOperation = types.Encrypt - if uploadedInfo.Annotations == nil { - uploadedInfo.Annotations = map[string]string{} - } - for k, v := range encryptAnnotations { - uploadedInfo.Annotations[k] = v - } + if err := encryptionStep.updateCryptoOperationAndAnnotations(&uploadedInfo.CryptoOperation, &uploadedInfo.Annotations); err != nil { + return types.BlobInfo{}, err } // This is fairly horrible: the writer from getOriginalLayerCopyWriter wants to consume @@ -275,7 +239,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // This crude approach also means we don’t need to record whether a blob is encrypted // in the blob info cache (which would probably be necessary for any more complex logic), // and the simplicity is attractive. - if !encrypted && !decryptionStep.decrypting { + if !encryptionStep.encrypting && !decryptionStep.decrypting { // If compressionOperation != types.PreserveOriginal, we now have two reliable digest values: // srcinfo.Digest describes the pre-compressionOperation input, verified by digestingReader // uploadedInfo.Digest describes the post-compressionOperation output, computed by PutBlob diff --git a/copy/encrypt.go b/copy/encrypt.go index 1f9f117e2..01b005a88 100644 --- a/copy/encrypt.go +++ b/copy/encrypt.go @@ -67,3 +67,65 @@ func (d *bpDecryptionStepData) updateCryptoOperation(operation *types.LayerCrypt *operation = types.Decrypt } } + +// bpdData contains data that the copy pipeline needs about the encryption step. +type bpEncryptionStepData struct { + encrypting bool // We are actually encrypting the stream + finalizer ocicrypt.EncryptLayerFinalizer +} + +// blobPipelineEncryptionStep updates *stream to encrypt if, it required by toEncrypt. +// srcInfo is primarily used for error messages. +// Returns data for other steps; the caller should eventually call updateCryptoOperationAndAnnotations. +func (c *copier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool, srcInfo types.BlobInfo, + decryptionStep *bpDecryptionStepData) (*bpEncryptionStepData, error) { + var ( + encrypted bool + finalizer ocicrypt.EncryptLayerFinalizer + ) + if toEncrypt && !isOciEncrypted(srcInfo.MediaType) && c.ociEncryptConfig != nil { + var annotations map[string]string + if !decryptionStep.decrypting { + annotations = srcInfo.Annotations + } + desc := imgspecv1.Descriptor{ + MediaType: srcInfo.MediaType, + Digest: srcInfo.Digest, + Size: srcInfo.Size, + Annotations: annotations, + } + + s, fin, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, stream.reader, desc) + if err != nil { + return nil, errors.Wrapf(err, "encrypting blob %s", srcInfo.Digest) + } + + finalizer = fin + stream.reader = s + stream.info.Digest = "" + stream.info.Size = -1 + encrypted = true + } + return &bpEncryptionStepData{ + encrypting: encrypted, + finalizer: finalizer, + }, nil +} + +// updateCryptoOperationAndAnnotations sets *operation and updates *annotations, if necessary. +func (d *bpEncryptionStepData) updateCryptoOperationAndAnnotations(operation *types.LayerCrypto, annotations *map[string]string) error { + if d.encrypting { + encryptAnnotations, err := d.finalizer() + if err != nil { + return errors.Wrap(err, "Unable to finalize encryption") + } + *operation = types.Encrypt + if *annotations == nil { + *annotations = map[string]string{} + } + for k, v := range encryptAnnotations { + (*annotations)[k] = v + } + } + return nil +} From 014c23d54b42bda156e9770cd73dd366f803d3bd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 16 Jun 2022 01:37:39 +0200 Subject: [PATCH 12/35] Simplify blobPipelineEncryptionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Make the "not encrypting" case truly special, and then we don't need extra variables across the function. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/encrypt.go | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/copy/encrypt.go b/copy/encrypt.go index 01b005a88..9ab0e1ca5 100644 --- a/copy/encrypt.go +++ b/copy/encrypt.go @@ -79,10 +79,6 @@ type bpEncryptionStepData struct { // Returns data for other steps; the caller should eventually call updateCryptoOperationAndAnnotations. func (c *copier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool, srcInfo types.BlobInfo, decryptionStep *bpDecryptionStepData) (*bpEncryptionStepData, error) { - var ( - encrypted bool - finalizer ocicrypt.EncryptLayerFinalizer - ) if toEncrypt && !isOciEncrypted(srcInfo.MediaType) && c.ociEncryptConfig != nil { var annotations map[string]string if !decryptionStep.decrypting { @@ -95,20 +91,21 @@ func (c *copier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool Annotations: annotations, } - s, fin, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, stream.reader, desc) + s, finalizer, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, stream.reader, desc) if err != nil { return nil, errors.Wrapf(err, "encrypting blob %s", srcInfo.Digest) } - finalizer = fin stream.reader = s stream.info.Digest = "" stream.info.Size = -1 - encrypted = true + return &bpEncryptionStepData{ + encrypting: true, + finalizer: finalizer, + }, nil } return &bpEncryptionStepData{ - encrypting: encrypted, - finalizer: finalizer, + encrypting: false, }, nil } From e8c38812188461e7a66a6f33add57a7b1d2797c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 16 Jun 2022 01:38:06 +0200 Subject: [PATCH 13/35] Exit from updateCryptoOperationAndAnnotations early if not encrypting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/encrypt.go | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-) diff --git a/copy/encrypt.go b/copy/encrypt.go index 9ab0e1ca5..c3f2f7937 100644 --- a/copy/encrypt.go +++ b/copy/encrypt.go @@ -111,18 +111,20 @@ func (c *copier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool // updateCryptoOperationAndAnnotations sets *operation and updates *annotations, if necessary. func (d *bpEncryptionStepData) updateCryptoOperationAndAnnotations(operation *types.LayerCrypto, annotations *map[string]string) error { - if d.encrypting { - encryptAnnotations, err := d.finalizer() - if err != nil { - return errors.Wrap(err, "Unable to finalize encryption") - } - *operation = types.Encrypt - if *annotations == nil { - *annotations = map[string]string{} - } - for k, v := range encryptAnnotations { - (*annotations)[k] = v - } + if !d.encrypting { + return nil + } + + encryptAnnotations, err := d.finalizer() + if err != nil { + return errors.Wrap(err, "Unable to finalize encryption") + } + *operation = types.Encrypt + if *annotations == nil { + *annotations = map[string]string{} + } + for k, v := range encryptAnnotations { + (*annotations)[k] = v } return nil } From d6fccbdb68863df1dd57a37b3732d27b78d8987a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 19:53:53 +0200 Subject: [PATCH 14/35] Beautify blobPipelineEncryptionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/encrypt.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/copy/encrypt.go b/copy/encrypt.go index c3f2f7937..ae0576da4 100644 --- a/copy/encrypt.go +++ b/copy/encrypt.go @@ -90,13 +90,12 @@ func (c *copier) blobPipelineEncryptionStep(stream *sourceStream, toEncrypt bool Size: srcInfo.Size, Annotations: annotations, } - - s, finalizer, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, stream.reader, desc) + reader, finalizer, err := ocicrypt.EncryptLayer(c.ociEncryptConfig, stream.reader, desc) if err != nil { return nil, errors.Wrapf(err, "encrypting blob %s", srcInfo.Digest) } - stream.reader = s + stream.reader = reader stream.info.Digest = "" stream.info.Size = -1 return &bpEncryptionStepData{ From 0f854d0b7008d2ba5b365df619caf84ce41c4a65 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:04:29 +0200 Subject: [PATCH 15/35] Rename copy/encrypt.go to copy/encryption.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It contains decompression code as well. Signed-off-by: Miloslav Trmač --- copy/{encrypt.go => encryption.go} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename copy/{encrypt.go => encryption.go} (100%) diff --git a/copy/encrypt.go b/copy/encryption.go similarity index 100% rename from copy/encrypt.go rename to copy/encryption.go From f0b0bf4e975d9b5dd3afa3c37bcbc1eb8941e470 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:15:43 +0200 Subject: [PATCH 16/35] Move the compression detection step into blobPipelineDetectCompressionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only moves the code with minimal necessary adjustments, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/blob.go | 31 +++++++++++++------------------ copy/compression.go | 37 +++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 18 deletions(-) create mode 100644 copy/compression.go diff --git a/copy/blob.go b/copy/blob.go index 3035828b2..412325ba6 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -55,20 +55,15 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // === Detect compression of the input stream. // This requires us to “peek ahead” into the stream to read the initial part, which requires us to chain through another io.Reader returned by DetectCompression. - compressionFormat, decompressor, detectCompressionFormatReader, err := compression.DetectCompressionFormat(stream.reader) // We could skip this in some cases, but let's keep the code path uniform + detectedCompression, err := blobPipelineDetectCompressionStep(&stream, srcInfo) if err != nil { - return types.BlobInfo{}, errors.Wrapf(err, "reading blob %s", srcInfo.Digest) - } - stream.reader = detectCompressionFormatReader - isCompressed := decompressor != nil - if expectedCompressionFormat, known := expectedCompressionFormats[stream.info.MediaType]; known && isCompressed && compressionFormat.Name() != expectedCompressionFormat.Name() { - logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedCompressionFormat.Name(), compressionFormat.Name()) + return types.BlobInfo{}, err } // === Send a copy of the original, uncompressed, stream, to a separate path if necessary. var originalLayerReader io.Reader // DO NOT USE this other than to drain the input if no other consumer in the pipeline has done so. if getOriginalLayerCopyWriter != nil { - stream.reader = io.TeeReader(stream.reader, getOriginalLayerCopyWriter(decompressor)) + stream.reader = io.TeeReader(stream.reader, getOriginalLayerCopyWriter(detectedCompression.decompressor)) originalLayerReader = stream.reader } @@ -79,8 +74,8 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr var compressionOperation types.LayerCompression var uploadCompressionFormat *compressiontypes.Algorithm srcCompressorName := internalblobinfocache.Uncompressed - if isCompressed { - srcCompressorName = compressionFormat.Name() + if detectedCompression.isCompressed { + srcCompressorName = detectedCompression.format.Name() } var uploadCompressorName string if canModifyBlob && isOciEncrypted(stream.info.MediaType) { @@ -90,7 +85,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr srcCompressorName = internalblobinfocache.UnknownCompression uploadCompressionFormat = nil uploadCompressorName = internalblobinfocache.UnknownCompression - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !isCompressed { + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detectedCompression.isCompressed { logrus.Debugf("Compressing blob on the fly") compressionOperation = types.Compress pipeReader, pipeWriter := io.Pipe() @@ -111,14 +106,14 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr Size: -1, } uploadCompressorName = uploadCompressionFormat.Name() - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && isCompressed && - c.compressionFormat != nil && c.compressionFormat.Name() != compressionFormat.Name() { + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detectedCompression.isCompressed && + c.compressionFormat != nil && c.compressionFormat.Name() != detectedCompression.format.Name() { // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally // re-compressed using the desired format. logrus.Debugf("Blob will be converted") compressionOperation = types.PreserveOriginal - s, err := decompressor(stream.reader) + s, err := detectedCompression.decompressor(stream.reader) if err != nil { return types.BlobInfo{}, err } @@ -136,10 +131,10 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr Size: -1, } uploadCompressorName = uploadCompressionFormat.Name() - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && isCompressed { + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detectedCompression.isCompressed { logrus.Debugf("Blob will be decompressed") compressionOperation = types.Decompress - s, err := decompressor(stream.reader) + s, err := detectedCompression.decompressor(stream.reader) if err != nil { return types.BlobInfo{}, err } @@ -159,8 +154,8 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr // LayerInfosForCopy() returned something that differs from what was in the // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), // it will be able to correctly derive the MediaType for the copied blob. - if isCompressed { - uploadCompressionFormat = &compressionFormat + if detectedCompression.isCompressed { + uploadCompressionFormat = &detectedCompression.format } else { uploadCompressionFormat = nil } diff --git a/copy/compression.go b/copy/compression.go new file mode 100644 index 000000000..ad641f30b --- /dev/null +++ b/copy/compression.go @@ -0,0 +1,37 @@ +package copy + +import ( + "github.com/containers/image/v5/pkg/compression" + compressiontypes "github.com/containers/image/v5/pkg/compression/types" + "github.com/containers/image/v5/types" + "github.com/pkg/errors" + "github.com/sirupsen/logrus" +) + +// bpDetectCompressionStepData contains data that the copy pipeline needs about the “detect compression” step. +type bpDetectCompressionStepData struct { + isCompressed bool + format compressiontypes.Algorithm // Valid if isCompressed + decompressor compressiontypes.DecompressorFunc // Valid if isCompressed +} + +// blobPipelineDetectCompressionStep updates *stream to detect its current compression format. +// srcInfo is only used for error messages. +// Returns data for other steps. +func blobPipelineDetectCompressionStep(stream *sourceStream, srcInfo types.BlobInfo) (bpDetectCompressionStepData, error) { + // This requires us to “peek ahead” into the stream to read the initial part, which requires us to chain through another io.Reader returned by DetectCompression. + compressionFormat, decompressor, detectCompressionFormatReader, err := compression.DetectCompressionFormat(stream.reader) // We could skip this in some cases, but let's keep the code path uniform + if err != nil { + return bpDetectCompressionStepData{}, errors.Wrapf(err, "reading blob %s", srcInfo.Digest) + } + stream.reader = detectCompressionFormatReader + res := bpDetectCompressionStepData{ + isCompressed: decompressor != nil, + format: compressionFormat, + decompressor: decompressor, + } + if expectedCompressionFormat, known := expectedCompressionFormats[stream.info.MediaType]; known && res.isCompressed && compressionFormat.Name() != expectedCompressionFormat.Name() { + logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedCompressionFormat.Name(), compressionFormat.Name()) + } + return res, nil +} From bf130bc85003e1ce9a16c1929e2a65c5e0700fa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:17:14 +0200 Subject: [PATCH 17/35] Beautify blobPipelineDetectCompressionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We can use shorter variable names in this short function now. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/compression.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index ad641f30b..433787963 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -20,18 +20,19 @@ type bpDetectCompressionStepData struct { // Returns data for other steps. func blobPipelineDetectCompressionStep(stream *sourceStream, srcInfo types.BlobInfo) (bpDetectCompressionStepData, error) { // This requires us to “peek ahead” into the stream to read the initial part, which requires us to chain through another io.Reader returned by DetectCompression. - compressionFormat, decompressor, detectCompressionFormatReader, err := compression.DetectCompressionFormat(stream.reader) // We could skip this in some cases, but let's keep the code path uniform + format, decompressor, reader, err := compression.DetectCompressionFormat(stream.reader) // We could skip this in some cases, but let's keep the code path uniform if err != nil { return bpDetectCompressionStepData{}, errors.Wrapf(err, "reading blob %s", srcInfo.Digest) } - stream.reader = detectCompressionFormatReader + stream.reader = reader + res := bpDetectCompressionStepData{ isCompressed: decompressor != nil, - format: compressionFormat, + format: format, decompressor: decompressor, } - if expectedCompressionFormat, known := expectedCompressionFormats[stream.info.MediaType]; known && res.isCompressed && compressionFormat.Name() != expectedCompressionFormat.Name() { - logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedCompressionFormat.Name(), compressionFormat.Name()) + if expectedFormat, known := expectedCompressionFormats[stream.info.MediaType]; known && res.isCompressed && format.Name() != expectedFormat.Name() { + logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedFormat.Name(), format.Name()) } return res, nil } From 003d340aef02d8798d825deccf573615a2c0c56f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:47:17 +0200 Subject: [PATCH 18/35] Move the compression annotation update closer to the other compression edits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only moves code, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/blob.go | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index 412325ba6..d5399155e 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -204,6 +204,13 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr uploadedInfo.CompressionOperation = compressionOperation // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. uploadedInfo.CompressionAlgorithm = uploadCompressionFormat + // Copy all the metadata generated by the compressor into the annotations. + if uploadedInfo.Annotations == nil { + uploadedInfo.Annotations = map[string]string{} + } + for k, v := range compressionMetadata { + uploadedInfo.Annotations[k] = v + } decryptionStep.updateCryptoOperation(&uploadedInfo.CryptoOperation) if err := encryptionStep.updateCryptoOperationAndAnnotations(&uploadedInfo.CryptoOperation, &uploadedInfo.Annotations); err != nil { return types.BlobInfo{}, err @@ -258,14 +265,6 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr } } - // Copy all the metadata generated by the compressor into the annotations. - if uploadedInfo.Annotations == nil { - uploadedInfo.Annotations = map[string]string{} - } - for k, v := range compressionMetadata { - uploadedInfo.Annotations[k] = v - } - return uploadedInfo, nil } From 6af0c8221dc41dbc17f35961a64f292015bc754e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 16 Jun 2022 01:01:55 +0200 Subject: [PATCH 19/35] Move copier.compressGoroutine to compression.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only moves unchanged code, should not change behavior. Signed-off-by: Miloslav Trmač --- copy/blob.go | 29 ----------------------------- copy/compression.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 29 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index d5399155e..a2574d49a 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -6,7 +6,6 @@ import ( internalblobinfocache "github.com/containers/image/v5/internal/blobinfocache" "github.com/containers/image/v5/internal/private" - "github.com/containers/image/v5/pkg/compression" compressiontypes "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/types" "github.com/pkg/errors" @@ -293,31 +292,3 @@ func (r errorAnnotationReader) Read(b []byte) (n int, err error) { } return n, err } - -// doCompression reads all input from src and writes its compressed equivalent to dest. -func doCompression(dest io.Writer, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm, compressionLevel *int) error { - compressor, err := compression.CompressStreamWithMetadata(dest, metadata, compressionFormat, compressionLevel) - if err != nil { - return err - } - - buf := make([]byte, compressionBufferSize) - - _, err = io.CopyBuffer(compressor, src, buf) // Sets err to nil, i.e. causes dest.Close() - if err != nil { - compressor.Close() - return err - } - - return compressor.Close() -} - -// compressGoroutine reads all input from src and writes its compressed equivalent to dest. -func (c *copier) compressGoroutine(dest *io.PipeWriter, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm) { - err := errors.New("Internal error: unexpected panic in compressGoroutine") - defer func() { // Note that this is not the same as {defer dest.CloseWithError(err)}; we need err to be evaluated lazily. - _ = dest.CloseWithError(err) // CloseWithError(nil) is equivalent to Close(), always returns nil - }() - - err = doCompression(dest, src, metadata, compressionFormat, c.compressionLevel) -} diff --git a/copy/compression.go b/copy/compression.go index 433787963..8a325b3e7 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -1,6 +1,8 @@ package copy import ( + "io" + "github.com/containers/image/v5/pkg/compression" compressiontypes "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/types" @@ -36,3 +38,31 @@ func blobPipelineDetectCompressionStep(stream *sourceStream, srcInfo types.BlobI } return res, nil } + +// doCompression reads all input from src and writes its compressed equivalent to dest. +func doCompression(dest io.Writer, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm, compressionLevel *int) error { + compressor, err := compression.CompressStreamWithMetadata(dest, metadata, compressionFormat, compressionLevel) + if err != nil { + return err + } + + buf := make([]byte, compressionBufferSize) + + _, err = io.CopyBuffer(compressor, src, buf) // Sets err to nil, i.e. causes dest.Close() + if err != nil { + compressor.Close() + return err + } + + return compressor.Close() +} + +// compressGoroutine reads all input from src and writes its compressed equivalent to dest. +func (c *copier) compressGoroutine(dest *io.PipeWriter, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm) { + err := errors.New("Internal error: unexpected panic in compressGoroutine") + defer func() { // Note that this is not the same as {defer dest.CloseWithError(err)}; we need err to be evaluated lazily. + _ = dest.CloseWithError(err) // CloseWithError(nil) is equivalent to Close(), always returns nil + }() + + err = doCompression(dest, src, metadata, compressionFormat, c.compressionLevel) +} From e83976eeb19f21781eb6054e21b5a3a13c29fd42 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:45:36 +0200 Subject: [PATCH 20/35] Move the compression/decompression step to blobPipelineCompressionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only moves the code with minimal necessary adjustments, should not change behavior. Those adjustments are not completely trivial in this case: we could just do a "defer stream.Close()" in copyBlobFromStream, now we need a separate closers field in blobPipelineCompressionStepData. Signed-off-by: Miloslav Trmač --- copy/blob.go | 135 ++------------------------------ copy/compression.go | 185 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 192 insertions(+), 128 deletions(-) diff --git a/copy/blob.go b/copy/blob.go index a2574d49a..4aea636c6 100644 --- a/copy/blob.go +++ b/copy/blob.go @@ -4,7 +4,6 @@ import ( "context" "io" - internalblobinfocache "github.com/containers/image/v5/internal/blobinfocache" "github.com/containers/image/v5/internal/private" compressiontypes "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/types" @@ -66,100 +65,14 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr originalLayerReader = stream.reader } - compressionMetadata := map[string]string{} // === Deal with layer compression/decompression if necessary // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions - var compressionOperation types.LayerCompression - var uploadCompressionFormat *compressiontypes.Algorithm - srcCompressorName := internalblobinfocache.Uncompressed - if detectedCompression.isCompressed { - srcCompressorName = detectedCompression.format.Name() - } - var uploadCompressorName string - if canModifyBlob && isOciEncrypted(stream.info.MediaType) { - // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted - logrus.Debugf("Using original blob without modification for encrypted blob") - compressionOperation = types.PreserveOriginal - srcCompressorName = internalblobinfocache.UnknownCompression - uploadCompressionFormat = nil - uploadCompressorName = internalblobinfocache.UnknownCompression - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detectedCompression.isCompressed { - logrus.Debugf("Compressing blob on the fly") - compressionOperation = types.Compress - pipeReader, pipeWriter := io.Pipe() - defer pipeReader.Close() - - if c.compressionFormat != nil { - uploadCompressionFormat = c.compressionFormat - } else { - uploadCompressionFormat = defaultCompressionFormat - } - // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, - // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, - // we don’t care. - go c.compressGoroutine(pipeWriter, stream.reader, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter - stream.reader = pipeReader - stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? - Digest: "", - Size: -1, - } - uploadCompressorName = uploadCompressionFormat.Name() - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detectedCompression.isCompressed && - c.compressionFormat != nil && c.compressionFormat.Name() != detectedCompression.format.Name() { - // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally - // re-compressed using the desired format. - logrus.Debugf("Blob will be converted") - - compressionOperation = types.PreserveOriginal - s, err := detectedCompression.decompressor(stream.reader) - if err != nil { - return types.BlobInfo{}, err - } - defer s.Close() - - pipeReader, pipeWriter := io.Pipe() - defer pipeReader.Close() - - uploadCompressionFormat = c.compressionFormat - go c.compressGoroutine(pipeWriter, s, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter - - stream.reader = pipeReader - stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? - Digest: "", - Size: -1, - } - uploadCompressorName = uploadCompressionFormat.Name() - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detectedCompression.isCompressed { - logrus.Debugf("Blob will be decompressed") - compressionOperation = types.Decompress - s, err := detectedCompression.decompressor(stream.reader) - if err != nil { - return types.BlobInfo{}, err - } - defer s.Close() - stream.reader = s - stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? - Digest: "", - Size: -1, - } - uploadCompressionFormat = nil - uploadCompressorName = internalblobinfocache.Uncompressed - } else { - // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. - logrus.Debugf("Using original blob without modification") - compressionOperation = types.PreserveOriginal - // Remember if the original blob was compressed, and if so how, so that if - // LayerInfosForCopy() returned something that differs from what was in the - // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), - // it will be able to correctly derive the MediaType for the copied blob. - if detectedCompression.isCompressed { - uploadCompressionFormat = &detectedCompression.format - } else { - uploadCompressionFormat = nil - } - uploadCompressorName = srcCompressorName + compressionStep, err := c.blobPipelineCompressionStep(&stream, canModifyBlob, detectedCompression) + if err != nil { + return types.BlobInfo{}, err } + defer compressionStep.close() // === Encrypt the stream for valid mediatypes if ociEncryptConfig provided if decryptionStep.decrypting && toEncrypt { @@ -200,16 +113,7 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr uploadedInfo.Annotations = stream.info.Annotations - uploadedInfo.CompressionOperation = compressionOperation - // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. - uploadedInfo.CompressionAlgorithm = uploadCompressionFormat - // Copy all the metadata generated by the compressor into the annotations. - if uploadedInfo.Annotations == nil { - uploadedInfo.Annotations = map[string]string{} - } - for k, v := range compressionMetadata { - uploadedInfo.Annotations[k] = v - } + compressionStep.updateCompressionEdits(&uploadedInfo.CompressionOperation, &uploadedInfo.CompressionAlgorithm, &uploadedInfo.Annotations) decryptionStep.updateCryptoOperation(&uploadedInfo.CryptoOperation) if err := encryptionStep.updateCryptoOperationAndAnnotations(&uploadedInfo.CryptoOperation, &uploadedInfo.Annotations); err != nil { return types.BlobInfo{}, err @@ -234,33 +138,8 @@ func (c *copier) copyBlobFromStream(ctx context.Context, srcReader io.Reader, sr return types.BlobInfo{}, errors.Errorf("Internal error writing blob %s, blob with digest %s saved with digest %s", srcInfo.Digest, stream.info.Digest, uploadedInfo.Digest) } if digestingReader.validationSucceeded { - // Don’t record any associations that involve encrypted data. This is a bit crude, - // some blob substitutions (replacing pulls of encrypted data with local reuse of known decryption outcomes) - // might be safe, but it’s not trivially obvious, so let’s be conservative for now. - // This crude approach also means we don’t need to record whether a blob is encrypted - // in the blob info cache (which would probably be necessary for any more complex logic), - // and the simplicity is attractive. - if !encryptionStep.encrypting && !decryptionStep.decrypting { - // If compressionOperation != types.PreserveOriginal, we now have two reliable digest values: - // srcinfo.Digest describes the pre-compressionOperation input, verified by digestingReader - // uploadedInfo.Digest describes the post-compressionOperation output, computed by PutBlob - // (because stream.info.Digest == "", this must have been computed afresh). - switch compressionOperation { - case types.PreserveOriginal: - break // Do nothing, we have only one digest and we might not have even verified it. - case types.Compress: - c.blobInfoCache.RecordDigestUncompressedPair(uploadedInfo.Digest, srcInfo.Digest) - case types.Decompress: - c.blobInfoCache.RecordDigestUncompressedPair(srcInfo.Digest, uploadedInfo.Digest) - default: - return types.BlobInfo{}, errors.Errorf("Internal error: Unexpected compressionOperation value %#v", compressionOperation) - } - } - if uploadCompressorName != "" && uploadCompressorName != internalblobinfocache.UnknownCompression { - c.blobInfoCache.RecordDigestCompressorName(uploadedInfo.Digest, uploadCompressorName) - } - if srcInfo.Digest != "" && srcCompressorName != "" && srcCompressorName != internalblobinfocache.UnknownCompression { - c.blobInfoCache.RecordDigestCompressorName(srcInfo.Digest, srcCompressorName) + if err := compressionStep.recordValidatedDigestData(c, uploadedInfo, srcInfo, encryptionStep, decryptionStep); err != nil { + return types.BlobInfo{}, err } } diff --git a/copy/compression.go b/copy/compression.go index 8a325b3e7..8f2a0aa64 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -3,6 +3,7 @@ package copy import ( "io" + internalblobinfocache "github.com/containers/image/v5/internal/blobinfocache" "github.com/containers/image/v5/pkg/compression" compressiontypes "github.com/containers/image/v5/pkg/compression/types" "github.com/containers/image/v5/types" @@ -39,6 +40,190 @@ func blobPipelineDetectCompressionStep(stream *sourceStream, srcInfo types.BlobI return res, nil } +// bpCompressionStepData contains data that the copy pipeline needs about the compression step. +type bpCompressionStepData struct { + compressionOperation types.LayerCompression // Operation to use for updating the blob metadata. + uploadCompressionFormat *compressiontypes.Algorithm // An algorithm parameter for the compressionOperation edits. + compressionMetadata map[string]string // Annotations that should be set on the uploaded blob. WARNING: This is only set after the srcStream.reader is fully consumed. + srcCompressorName string // Compressor name to record in the blob info cache for the source blob. + uploadCompressorName string // Compressor name to record in the blob info cache for the uploaded blob. + closers []io.Closer // Objects to close after the upload is done, if any. +} + +// blobPipelineCompressionStep updates *stream to compress and/or decompress it. +// srcInfo is only used for error messages. +// Returns data for other steps; the caller should eventually call updateCompressionEdits and perhaps recordValidatedBlobData, +// and must eventually call close. +func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob bool, + detectedCompression bpDetectCompressionStepData) (*bpCompressionStepData, error) { + // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists + // short-circuit conditions + compressionMetadata := map[string]string{} + var compressionOperation types.LayerCompression + var uploadCompressionFormat *compressiontypes.Algorithm + srcCompressorName := internalblobinfocache.Uncompressed + if detectedCompression.isCompressed { + srcCompressorName = detectedCompression.format.Name() + } + var uploadCompressorName string + var closers []io.Closer + succeeded := false + defer func() { + if !succeeded { + for _, c := range closers { + c.Close() + } + } + }() + if canModifyBlob && isOciEncrypted(stream.info.MediaType) { + // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted + logrus.Debugf("Using original blob without modification for encrypted blob") + compressionOperation = types.PreserveOriginal + srcCompressorName = internalblobinfocache.UnknownCompression + uploadCompressionFormat = nil + uploadCompressorName = internalblobinfocache.UnknownCompression + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detectedCompression.isCompressed { + logrus.Debugf("Compressing blob on the fly") + compressionOperation = types.Compress + pipeReader, pipeWriter := io.Pipe() + closers = append(closers, pipeReader) + + if c.compressionFormat != nil { + uploadCompressionFormat = c.compressionFormat + } else { + uploadCompressionFormat = defaultCompressionFormat + } + // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, + // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, + // we don’t care. + go c.compressGoroutine(pipeWriter, stream.reader, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter + stream.reader = pipeReader + stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? + Digest: "", + Size: -1, + } + uploadCompressorName = uploadCompressionFormat.Name() + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detectedCompression.isCompressed && + c.compressionFormat != nil && c.compressionFormat.Name() != detectedCompression.format.Name() { + // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally + // re-compressed using the desired format. + logrus.Debugf("Blob will be converted") + + compressionOperation = types.PreserveOriginal + s, err := detectedCompression.decompressor(stream.reader) + if err != nil { + return nil, err + } + closers = append(closers, s) + + pipeReader, pipeWriter := io.Pipe() + closers = append(closers, pipeReader) + + uploadCompressionFormat = c.compressionFormat + go c.compressGoroutine(pipeWriter, s, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter + + stream.reader = pipeReader + stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? + Digest: "", + Size: -1, + } + uploadCompressorName = uploadCompressionFormat.Name() + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detectedCompression.isCompressed { + logrus.Debugf("Blob will be decompressed") + compressionOperation = types.Decompress + s, err := detectedCompression.decompressor(stream.reader) + if err != nil { + return nil, err + } + closers = append(closers, s) + stream.reader = s + stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? + Digest: "", + Size: -1, + } + uploadCompressionFormat = nil + uploadCompressorName = internalblobinfocache.Uncompressed + } else { + // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. + logrus.Debugf("Using original blob without modification") + compressionOperation = types.PreserveOriginal + // Remember if the original blob was compressed, and if so how, so that if + // LayerInfosForCopy() returned something that differs from what was in the + // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), + // it will be able to correctly derive the MediaType for the copied blob. + if detectedCompression.isCompressed { + uploadCompressionFormat = &detectedCompression.format + } else { + uploadCompressionFormat = nil + } + uploadCompressorName = srcCompressorName + } + succeeded = true + return &bpCompressionStepData{ + compressionOperation: compressionOperation, + uploadCompressionFormat: uploadCompressionFormat, + compressionMetadata: compressionMetadata, + srcCompressorName: srcCompressorName, + uploadCompressorName: uploadCompressorName, + closers: closers, + }, nil +} + +// updateCompressionEdits sets *operation, *algorithm and updates *annotations, if necessary. +func (d *bpCompressionStepData) updateCompressionEdits(operation *types.LayerCompression, algorithm **compressiontypes.Algorithm, annotations *map[string]string) { + *operation = d.compressionOperation + // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. + *algorithm = d.uploadCompressionFormat + if *annotations == nil { + *annotations = map[string]string{} + } + for k, v := range d.compressionMetadata { + (*annotations)[k] = v + } +} + +// recordValidatedBlobData updates b.blobInfoCache with data about the created uploadedInfo adnd the original srcInfo. +// This must ONLY be called if all data has been validated by OUR code, and is not comming from third parties. +func (d *bpCompressionStepData) recordValidatedDigestData(c *copier, uploadedInfo types.BlobInfo, srcInfo types.BlobInfo, + encryptionStep *bpEncryptionStepData, decryptionStep *bpDecryptionStepData) error { + // Don’t record any associations that involve encrypted data. This is a bit crude, + // some blob substitutions (replacing pulls of encrypted data with local reuse of known decryption outcomes) + // might be safe, but it’s not trivially obvious, so let’s be conservative for now. + // This crude approach also means we don’t need to record whether a blob is encrypted + // in the blob info cache (which would probably be necessary for any more complex logic), + // and the simplicity is attractive. + if !encryptionStep.encrypting && !decryptionStep.decrypting { + // If d.compressionOperation != types.PreserveOriginal, we now have two reliable digest values: + // srcinfo.Digest describes the pre-d.compressionOperation input, verified by digestingReader + // uploadedInfo.Digest describes the post-d.compressionOperation output, computed by PutBlob + // (because stream.info.Digest == "", this must have been computed afresh). + switch d.compressionOperation { + case types.PreserveOriginal: + break // Do nothing, we have only one digest and we might not have even verified it. + case types.Compress: + c.blobInfoCache.RecordDigestUncompressedPair(uploadedInfo.Digest, srcInfo.Digest) + case types.Decompress: + c.blobInfoCache.RecordDigestUncompressedPair(srcInfo.Digest, uploadedInfo.Digest) + default: + return errors.Errorf("Internal error: Unexpected d.compressionOperation value %#v", d.compressionOperation) + } + } + if d.uploadCompressorName != "" && d.uploadCompressorName != internalblobinfocache.UnknownCompression { + c.blobInfoCache.RecordDigestCompressorName(uploadedInfo.Digest, d.uploadCompressorName) + } + if srcInfo.Digest != "" && d.srcCompressorName != "" && d.srcCompressorName != internalblobinfocache.UnknownCompression { + c.blobInfoCache.RecordDigestCompressorName(srcInfo.Digest, d.srcCompressorName) + } + return nil +} + +// close closes objects that carry state throughout the compression/decompression operation. +func (d *bpCompressionStepData) close() { + for _, c := range d.closers { + c.Close() + } +} + // doCompression reads all input from src and writes its compressed equivalent to dest. func doCompression(dest io.Writer, src io.Reader, metadata map[string]string, compressionFormat compressiontypes.Algorithm, compressionLevel *int) error { compressor, err := compression.CompressStreamWithMetadata(dest, metadata, compressionFormat, compressionLevel) From 00a66fee8c0f901b7ea2d89bd039334f9e4197fe Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:49:10 +0200 Subject: [PATCH 21/35] Rename detectedCompession to detected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compression meaning is clear in context here. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/compression.go | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 8f2a0aa64..9150d646a 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -55,15 +55,15 @@ type bpCompressionStepData struct { // Returns data for other steps; the caller should eventually call updateCompressionEdits and perhaps recordValidatedBlobData, // and must eventually call close. func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob bool, - detectedCompression bpDetectCompressionStepData) (*bpCompressionStepData, error) { + detected bpDetectCompressionStepData) (*bpCompressionStepData, error) { // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions compressionMetadata := map[string]string{} var compressionOperation types.LayerCompression var uploadCompressionFormat *compressiontypes.Algorithm srcCompressorName := internalblobinfocache.Uncompressed - if detectedCompression.isCompressed { - srcCompressorName = detectedCompression.format.Name() + if detected.isCompressed { + srcCompressorName = detected.format.Name() } var uploadCompressorName string var closers []io.Closer @@ -82,7 +82,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob srcCompressorName = internalblobinfocache.UnknownCompression uploadCompressionFormat = nil uploadCompressorName = internalblobinfocache.UnknownCompression - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detectedCompression.isCompressed { + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detected.isCompressed { logrus.Debugf("Compressing blob on the fly") compressionOperation = types.Compress pipeReader, pipeWriter := io.Pipe() @@ -103,14 +103,14 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob Size: -1, } uploadCompressorName = uploadCompressionFormat.Name() - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detectedCompression.isCompressed && - c.compressionFormat != nil && c.compressionFormat.Name() != detectedCompression.format.Name() { + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detected.isCompressed && + c.compressionFormat != nil && c.compressionFormat.Name() != detected.format.Name() { // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally // re-compressed using the desired format. logrus.Debugf("Blob will be converted") compressionOperation = types.PreserveOriginal - s, err := detectedCompression.decompressor(stream.reader) + s, err := detected.decompressor(stream.reader) if err != nil { return nil, err } @@ -128,10 +128,10 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob Size: -1, } uploadCompressorName = uploadCompressionFormat.Name() - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detectedCompression.isCompressed { + } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detected.isCompressed { logrus.Debugf("Blob will be decompressed") compressionOperation = types.Decompress - s, err := detectedCompression.decompressor(stream.reader) + s, err := detected.decompressor(stream.reader) if err != nil { return nil, err } @@ -151,8 +151,8 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // LayerInfosForCopy() returned something that differs from what was in the // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), // it will be able to correctly derive the MediaType for the copied blob. - if detectedCompression.isCompressed { - uploadCompressionFormat = &detectedCompression.format + if detected.isCompressed { + uploadCompressionFormat = &detected.format } else { uploadCompressionFormat = nil } From 23a031fb0b7cffb6b115ca4da159f8594b543a3a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:51:01 +0200 Subject: [PATCH 22/35] Rename compresisonOperation to operation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/compression.go | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 9150d646a..5898ddbc4 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -42,7 +42,7 @@ func blobPipelineDetectCompressionStep(stream *sourceStream, srcInfo types.BlobI // bpCompressionStepData contains data that the copy pipeline needs about the compression step. type bpCompressionStepData struct { - compressionOperation types.LayerCompression // Operation to use for updating the blob metadata. + operation types.LayerCompression // Operation to use for updating the blob metadata. uploadCompressionFormat *compressiontypes.Algorithm // An algorithm parameter for the compressionOperation edits. compressionMetadata map[string]string // Annotations that should be set on the uploaded blob. WARNING: This is only set after the srcStream.reader is fully consumed. srcCompressorName string // Compressor name to record in the blob info cache for the source blob. @@ -59,7 +59,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions compressionMetadata := map[string]string{} - var compressionOperation types.LayerCompression + var operation types.LayerCompression var uploadCompressionFormat *compressiontypes.Algorithm srcCompressorName := internalblobinfocache.Uncompressed if detected.isCompressed { @@ -78,13 +78,13 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob if canModifyBlob && isOciEncrypted(stream.info.MediaType) { // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted logrus.Debugf("Using original blob without modification for encrypted blob") - compressionOperation = types.PreserveOriginal + operation = types.PreserveOriginal srcCompressorName = internalblobinfocache.UnknownCompression uploadCompressionFormat = nil uploadCompressorName = internalblobinfocache.UnknownCompression } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detected.isCompressed { logrus.Debugf("Compressing blob on the fly") - compressionOperation = types.Compress + operation = types.Compress pipeReader, pipeWriter := io.Pipe() closers = append(closers, pipeReader) @@ -109,7 +109,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // re-compressed using the desired format. logrus.Debugf("Blob will be converted") - compressionOperation = types.PreserveOriginal + operation = types.PreserveOriginal s, err := detected.decompressor(stream.reader) if err != nil { return nil, err @@ -130,7 +130,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob uploadCompressorName = uploadCompressionFormat.Name() } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detected.isCompressed { logrus.Debugf("Blob will be decompressed") - compressionOperation = types.Decompress + operation = types.Decompress s, err := detected.decompressor(stream.reader) if err != nil { return nil, err @@ -146,7 +146,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob } else { // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. logrus.Debugf("Using original blob without modification") - compressionOperation = types.PreserveOriginal + operation = types.PreserveOriginal // Remember if the original blob was compressed, and if so how, so that if // LayerInfosForCopy() returned something that differs from what was in the // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), @@ -160,7 +160,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob } succeeded = true return &bpCompressionStepData{ - compressionOperation: compressionOperation, + operation: operation, uploadCompressionFormat: uploadCompressionFormat, compressionMetadata: compressionMetadata, srcCompressorName: srcCompressorName, @@ -171,7 +171,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // updateCompressionEdits sets *operation, *algorithm and updates *annotations, if necessary. func (d *bpCompressionStepData) updateCompressionEdits(operation *types.LayerCompression, algorithm **compressiontypes.Algorithm, annotations *map[string]string) { - *operation = d.compressionOperation + *operation = d.operation // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. *algorithm = d.uploadCompressionFormat if *annotations == nil { @@ -193,11 +193,11 @@ func (d *bpCompressionStepData) recordValidatedDigestData(c *copier, uploadedInf // in the blob info cache (which would probably be necessary for any more complex logic), // and the simplicity is attractive. if !encryptionStep.encrypting && !decryptionStep.decrypting { - // If d.compressionOperation != types.PreserveOriginal, we now have two reliable digest values: - // srcinfo.Digest describes the pre-d.compressionOperation input, verified by digestingReader - // uploadedInfo.Digest describes the post-d.compressionOperation output, computed by PutBlob + // If d.operation != types.PreserveOriginal, we now have two reliable digest values: + // srcinfo.Digest describes the pre-d.operation input, verified by digestingReader + // uploadedInfo.Digest describes the post-d.operation output, computed by PutBlob // (because stream.info.Digest == "", this must have been computed afresh). - switch d.compressionOperation { + switch d.operation { case types.PreserveOriginal: break // Do nothing, we have only one digest and we might not have even verified it. case types.Compress: @@ -205,7 +205,7 @@ func (d *bpCompressionStepData) recordValidatedDigestData(c *copier, uploadedInf case types.Decompress: c.blobInfoCache.RecordDigestUncompressedPair(srcInfo.Digest, uploadedInfo.Digest) default: - return errors.Errorf("Internal error: Unexpected d.compressionOperation value %#v", d.compressionOperation) + return errors.Errorf("Internal error: Unexpected d.operation value %#v", d.operation) } } if d.uploadCompressorName != "" && d.uploadCompressorName != internalblobinfocache.UnknownCompression { From 78fd34d93888ccf1cec7abc0a2f00bbc84ec43a0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:51:53 +0200 Subject: [PATCH 23/35] Rename uploadCompressionFormat to uploadedAlgorithm MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/compression.go | 50 ++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 5898ddbc4..2436c7fc0 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -42,12 +42,12 @@ func blobPipelineDetectCompressionStep(stream *sourceStream, srcInfo types.BlobI // bpCompressionStepData contains data that the copy pipeline needs about the compression step. type bpCompressionStepData struct { - operation types.LayerCompression // Operation to use for updating the blob metadata. - uploadCompressionFormat *compressiontypes.Algorithm // An algorithm parameter for the compressionOperation edits. - compressionMetadata map[string]string // Annotations that should be set on the uploaded blob. WARNING: This is only set after the srcStream.reader is fully consumed. - srcCompressorName string // Compressor name to record in the blob info cache for the source blob. - uploadCompressorName string // Compressor name to record in the blob info cache for the uploaded blob. - closers []io.Closer // Objects to close after the upload is done, if any. + operation types.LayerCompression // Operation to use for updating the blob metadata. + uploadedAlgorithm *compressiontypes.Algorithm // An algorithm parameter for the compressionOperation edits. + compressionMetadata map[string]string // Annotations that should be set on the uploaded blob. WARNING: This is only set after the srcStream.reader is fully consumed. + srcCompressorName string // Compressor name to record in the blob info cache for the source blob. + uploadCompressorName string // Compressor name to record in the blob info cache for the uploaded blob. + closers []io.Closer // Objects to close after the upload is done, if any. } // blobPipelineCompressionStep updates *stream to compress and/or decompress it. @@ -60,7 +60,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // short-circuit conditions compressionMetadata := map[string]string{} var operation types.LayerCompression - var uploadCompressionFormat *compressiontypes.Algorithm + var uploadedAlgorithm *compressiontypes.Algorithm srcCompressorName := internalblobinfocache.Uncompressed if detected.isCompressed { srcCompressorName = detected.format.Name() @@ -80,7 +80,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob logrus.Debugf("Using original blob without modification for encrypted blob") operation = types.PreserveOriginal srcCompressorName = internalblobinfocache.UnknownCompression - uploadCompressionFormat = nil + uploadedAlgorithm = nil uploadCompressorName = internalblobinfocache.UnknownCompression } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detected.isCompressed { logrus.Debugf("Compressing blob on the fly") @@ -89,20 +89,20 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob closers = append(closers, pipeReader) if c.compressionFormat != nil { - uploadCompressionFormat = c.compressionFormat + uploadedAlgorithm = c.compressionFormat } else { - uploadCompressionFormat = defaultCompressionFormat + uploadedAlgorithm = defaultCompressionFormat } // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, // we don’t care. - go c.compressGoroutine(pipeWriter, stream.reader, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter + go c.compressGoroutine(pipeWriter, stream.reader, compressionMetadata, *uploadedAlgorithm) // Closes pipeWriter stream.reader = pipeReader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", Size: -1, } - uploadCompressorName = uploadCompressionFormat.Name() + uploadCompressorName = uploadedAlgorithm.Name() } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detected.isCompressed && c.compressionFormat != nil && c.compressionFormat.Name() != detected.format.Name() { // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally @@ -119,15 +119,15 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob pipeReader, pipeWriter := io.Pipe() closers = append(closers, pipeReader) - uploadCompressionFormat = c.compressionFormat - go c.compressGoroutine(pipeWriter, s, compressionMetadata, *uploadCompressionFormat) // Closes pipeWriter + uploadedAlgorithm = c.compressionFormat + go c.compressGoroutine(pipeWriter, s, compressionMetadata, *uploadedAlgorithm) // Closes pipeWriter stream.reader = pipeReader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", Size: -1, } - uploadCompressorName = uploadCompressionFormat.Name() + uploadCompressorName = uploadedAlgorithm.Name() } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detected.isCompressed { logrus.Debugf("Blob will be decompressed") operation = types.Decompress @@ -141,7 +141,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob Digest: "", Size: -1, } - uploadCompressionFormat = nil + uploadedAlgorithm = nil uploadCompressorName = internalblobinfocache.Uncompressed } else { // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. @@ -152,20 +152,20 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), // it will be able to correctly derive the MediaType for the copied blob. if detected.isCompressed { - uploadCompressionFormat = &detected.format + uploadedAlgorithm = &detected.format } else { - uploadCompressionFormat = nil + uploadedAlgorithm = nil } uploadCompressorName = srcCompressorName } succeeded = true return &bpCompressionStepData{ - operation: operation, - uploadCompressionFormat: uploadCompressionFormat, - compressionMetadata: compressionMetadata, - srcCompressorName: srcCompressorName, - uploadCompressorName: uploadCompressorName, - closers: closers, + operation: operation, + uploadedAlgorithm: uploadedAlgorithm, + compressionMetadata: compressionMetadata, + srcCompressorName: srcCompressorName, + uploadCompressorName: uploadCompressorName, + closers: closers, }, nil } @@ -173,7 +173,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob func (d *bpCompressionStepData) updateCompressionEdits(operation *types.LayerCompression, algorithm **compressiontypes.Algorithm, annotations *map[string]string) { *operation = d.operation // If we can modify the layer's blob, set the desired algorithm for it to be set in the manifest. - *algorithm = d.uploadCompressionFormat + *algorithm = d.uploadedAlgorithm if *annotations == nil { *annotations = map[string]string{} } From ce4d06689c99292aa314280e2276ada593ea0f71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:52:46 +0200 Subject: [PATCH 24/35] Rename uploadCompressorName to uploadedCompressorName MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/compression.go | 40 ++++++++++++++++++++-------------------- 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 2436c7fc0..98833c248 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -42,12 +42,12 @@ func blobPipelineDetectCompressionStep(stream *sourceStream, srcInfo types.BlobI // bpCompressionStepData contains data that the copy pipeline needs about the compression step. type bpCompressionStepData struct { - operation types.LayerCompression // Operation to use for updating the blob metadata. - uploadedAlgorithm *compressiontypes.Algorithm // An algorithm parameter for the compressionOperation edits. - compressionMetadata map[string]string // Annotations that should be set on the uploaded blob. WARNING: This is only set after the srcStream.reader is fully consumed. - srcCompressorName string // Compressor name to record in the blob info cache for the source blob. - uploadCompressorName string // Compressor name to record in the blob info cache for the uploaded blob. - closers []io.Closer // Objects to close after the upload is done, if any. + operation types.LayerCompression // Operation to use for updating the blob metadata. + uploadedAlgorithm *compressiontypes.Algorithm // An algorithm parameter for the compressionOperation edits. + compressionMetadata map[string]string // Annotations that should be set on the uploaded blob. WARNING: This is only set after the srcStream.reader is fully consumed. + srcCompressorName string // Compressor name to record in the blob info cache for the source blob. + uploadedCompressorName string // Compressor name to record in the blob info cache for the uploaded blob. + closers []io.Closer // Objects to close after the upload is done, if any. } // blobPipelineCompressionStep updates *stream to compress and/or decompress it. @@ -65,7 +65,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob if detected.isCompressed { srcCompressorName = detected.format.Name() } - var uploadCompressorName string + var uploadedCompressorName string var closers []io.Closer succeeded := false defer func() { @@ -81,7 +81,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob operation = types.PreserveOriginal srcCompressorName = internalblobinfocache.UnknownCompression uploadedAlgorithm = nil - uploadCompressorName = internalblobinfocache.UnknownCompression + uploadedCompressorName = internalblobinfocache.UnknownCompression } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detected.isCompressed { logrus.Debugf("Compressing blob on the fly") operation = types.Compress @@ -102,7 +102,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob Digest: "", Size: -1, } - uploadCompressorName = uploadedAlgorithm.Name() + uploadedCompressorName = uploadedAlgorithm.Name() } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detected.isCompressed && c.compressionFormat != nil && c.compressionFormat.Name() != detected.format.Name() { // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally @@ -127,7 +127,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob Digest: "", Size: -1, } - uploadCompressorName = uploadedAlgorithm.Name() + uploadedCompressorName = uploadedAlgorithm.Name() } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detected.isCompressed { logrus.Debugf("Blob will be decompressed") operation = types.Decompress @@ -142,7 +142,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob Size: -1, } uploadedAlgorithm = nil - uploadCompressorName = internalblobinfocache.Uncompressed + uploadedCompressorName = internalblobinfocache.Uncompressed } else { // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. logrus.Debugf("Using original blob without modification") @@ -156,16 +156,16 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob } else { uploadedAlgorithm = nil } - uploadCompressorName = srcCompressorName + uploadedCompressorName = srcCompressorName } succeeded = true return &bpCompressionStepData{ - operation: operation, - uploadedAlgorithm: uploadedAlgorithm, - compressionMetadata: compressionMetadata, - srcCompressorName: srcCompressorName, - uploadCompressorName: uploadCompressorName, - closers: closers, + operation: operation, + uploadedAlgorithm: uploadedAlgorithm, + compressionMetadata: compressionMetadata, + srcCompressorName: srcCompressorName, + uploadedCompressorName: uploadedCompressorName, + closers: closers, }, nil } @@ -208,8 +208,8 @@ func (d *bpCompressionStepData) recordValidatedDigestData(c *copier, uploadedInf return errors.Errorf("Internal error: Unexpected d.operation value %#v", d.operation) } } - if d.uploadCompressorName != "" && d.uploadCompressorName != internalblobinfocache.UnknownCompression { - c.blobInfoCache.RecordDigestCompressorName(uploadedInfo.Digest, d.uploadCompressorName) + if d.uploadedCompressorName != "" && d.uploadedCompressorName != internalblobinfocache.UnknownCompression { + c.blobInfoCache.RecordDigestCompressorName(uploadedInfo.Digest, d.uploadedCompressorName) } if srcInfo.Digest != "" && d.srcCompressorName != "" && d.srcCompressorName != internalblobinfocache.UnknownCompression { c.blobInfoCache.RecordDigestCompressorName(srcInfo.Digest, d.srcCompressorName) From 68e0d9d9258c668cd7e515121a0bb010255335c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 20:53:39 +0200 Subject: [PATCH 25/35] Rename compressionMetadata to uploadedAnnotations. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/compression.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 98833c248..9b74f7dc6 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -44,7 +44,7 @@ func blobPipelineDetectCompressionStep(stream *sourceStream, srcInfo types.BlobI type bpCompressionStepData struct { operation types.LayerCompression // Operation to use for updating the blob metadata. uploadedAlgorithm *compressiontypes.Algorithm // An algorithm parameter for the compressionOperation edits. - compressionMetadata map[string]string // Annotations that should be set on the uploaded blob. WARNING: This is only set after the srcStream.reader is fully consumed. + uploadedAnnotations map[string]string // Annotations that should be set on the uploaded blob. WARNING: This is only set after the srcStream.reader is fully consumed. srcCompressorName string // Compressor name to record in the blob info cache for the source blob. uploadedCompressorName string // Compressor name to record in the blob info cache for the uploaded blob. closers []io.Closer // Objects to close after the upload is done, if any. @@ -58,7 +58,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob detected bpDetectCompressionStepData) (*bpCompressionStepData, error) { // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions - compressionMetadata := map[string]string{} + uploadedAnnotations := map[string]string{} var operation types.LayerCompression var uploadedAlgorithm *compressiontypes.Algorithm srcCompressorName := internalblobinfocache.Uncompressed @@ -96,7 +96,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, // we don’t care. - go c.compressGoroutine(pipeWriter, stream.reader, compressionMetadata, *uploadedAlgorithm) // Closes pipeWriter + go c.compressGoroutine(pipeWriter, stream.reader, uploadedAnnotations, *uploadedAlgorithm) // Closes pipeWriter stream.reader = pipeReader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", @@ -120,7 +120,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob closers = append(closers, pipeReader) uploadedAlgorithm = c.compressionFormat - go c.compressGoroutine(pipeWriter, s, compressionMetadata, *uploadedAlgorithm) // Closes pipeWriter + go c.compressGoroutine(pipeWriter, s, uploadedAnnotations, *uploadedAlgorithm) // Closes pipeWriter stream.reader = pipeReader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? @@ -162,7 +162,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob return &bpCompressionStepData{ operation: operation, uploadedAlgorithm: uploadedAlgorithm, - compressionMetadata: compressionMetadata, + uploadedAnnotations: uploadedAnnotations, srcCompressorName: srcCompressorName, uploadedCompressorName: uploadedCompressorName, closers: closers, @@ -177,7 +177,7 @@ func (d *bpCompressionStepData) updateCompressionEdits(operation *types.LayerCom if *annotations == nil { *annotations = map[string]string{} } - for k, v := range d.compressionMetadata { + for k, v := range d.uploadedAnnotations { (*annotations)[k] = v } } From 4f0548a6137f12708bc8ec704be32fa3b817ad3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 23:30:02 +0200 Subject: [PATCH 26/35] Compute srcCompressorName already in blobPipelineDetectCompressionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Except for the encrypted content case, we already have the data available, and this will allow us to just pass around a *blobPipelineDetectCompressionStepData without an extra parameter for srcCompressorName. Signed-off-by: Miloslav Trmač --- copy/compression.go | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 9b74f7dc6..bc62c4019 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -13,9 +13,10 @@ import ( // bpDetectCompressionStepData contains data that the copy pipeline needs about the “detect compression” step. type bpDetectCompressionStepData struct { - isCompressed bool - format compressiontypes.Algorithm // Valid if isCompressed - decompressor compressiontypes.DecompressorFunc // Valid if isCompressed + isCompressed bool + format compressiontypes.Algorithm // Valid if isCompressed + decompressor compressiontypes.DecompressorFunc // Valid if isCompressed + srcCompressorName string // Compressor name to possibly record in the blob info cache for the source blob. } // blobPipelineDetectCompressionStep updates *stream to detect its current compression format. @@ -34,6 +35,12 @@ func blobPipelineDetectCompressionStep(stream *sourceStream, srcInfo types.BlobI format: format, decompressor: decompressor, } + if res.isCompressed { + res.srcCompressorName = format.Name() + } else { + res.srcCompressorName = internalblobinfocache.Uncompressed + } + if expectedFormat, known := expectedCompressionFormats[stream.info.MediaType]; known && res.isCompressed && format.Name() != expectedFormat.Name() { logrus.Debugf("blob %s with type %s should be compressed with %s, but compressor appears to be %s", srcInfo.Digest.String(), srcInfo.MediaType, expectedFormat.Name(), format.Name()) } @@ -61,10 +68,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob uploadedAnnotations := map[string]string{} var operation types.LayerCompression var uploadedAlgorithm *compressiontypes.Algorithm - srcCompressorName := internalblobinfocache.Uncompressed - if detected.isCompressed { - srcCompressorName = detected.format.Name() - } + srcCompressorName := detected.srcCompressorName var uploadedCompressorName string var closers []io.Closer succeeded := false From f93ebcea4f5b328f5d5b58be30cdae4efc3b6700 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 21:11:38 +0200 Subject: [PATCH 27/35] Return a fresh &bpCompressionStepData in each case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit That's actually shorter because in most cases we just use a field initializer instead of setting a variable; and we don't need that variable in the first place. For now, do just the minimal edit, we will clean this up a bit later. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/compression.go | 69 +++++++++++++++++++++++++++------------------ 1 file changed, 42 insertions(+), 27 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index bc62c4019..0bf4b54a0 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -66,10 +66,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions uploadedAnnotations := map[string]string{} - var operation types.LayerCompression var uploadedAlgorithm *compressiontypes.Algorithm - srcCompressorName := detected.srcCompressorName - var uploadedCompressorName string var closers []io.Closer succeeded := false defer func() { @@ -82,13 +79,16 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob if canModifyBlob && isOciEncrypted(stream.info.MediaType) { // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted logrus.Debugf("Using original blob without modification for encrypted blob") - operation = types.PreserveOriginal - srcCompressorName = internalblobinfocache.UnknownCompression - uploadedAlgorithm = nil - uploadedCompressorName = internalblobinfocache.UnknownCompression + succeeded = true + return &bpCompressionStepData{ + operation: types.PreserveOriginal, + uploadedAlgorithm: nil, + srcCompressorName: internalblobinfocache.UnknownCompression, + uploadedCompressorName: internalblobinfocache.UnknownCompression, + closers: closers, + }, nil } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detected.isCompressed { logrus.Debugf("Compressing blob on the fly") - operation = types.Compress pipeReader, pipeWriter := io.Pipe() closers = append(closers, pipeReader) @@ -106,14 +106,20 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob Digest: "", Size: -1, } - uploadedCompressorName = uploadedAlgorithm.Name() + succeeded = true + return &bpCompressionStepData{ + operation: types.Compress, + uploadedAlgorithm: uploadedAlgorithm, + srcCompressorName: detected.srcCompressorName, + uploadedCompressorName: uploadedAlgorithm.Name(), + closers: closers, + }, nil } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detected.isCompressed && c.compressionFormat != nil && c.compressionFormat.Name() != detected.format.Name() { // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally // re-compressed using the desired format. logrus.Debugf("Blob will be converted") - operation = types.PreserveOriginal s, err := detected.decompressor(stream.reader) if err != nil { return nil, err @@ -123,18 +129,24 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob pipeReader, pipeWriter := io.Pipe() closers = append(closers, pipeReader) - uploadedAlgorithm = c.compressionFormat - go c.compressGoroutine(pipeWriter, s, uploadedAnnotations, *uploadedAlgorithm) // Closes pipeWriter + go c.compressGoroutine(pipeWriter, s, uploadedAnnotations, *c.compressionFormat) // Closes pipeWriter stream.reader = pipeReader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", Size: -1, } - uploadedCompressorName = uploadedAlgorithm.Name() + succeeded = true + return &bpCompressionStepData{ + operation: types.PreserveOriginal, + uploadedAlgorithm: c.compressionFormat, + uploadedAnnotations: uploadedAnnotations, + srcCompressorName: detected.srcCompressorName, + uploadedCompressorName: c.compressionFormat.Name(), + closers: closers, + }, nil } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detected.isCompressed { logrus.Debugf("Blob will be decompressed") - operation = types.Decompress s, err := detected.decompressor(stream.reader) if err != nil { return nil, err @@ -145,12 +157,17 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob Digest: "", Size: -1, } - uploadedAlgorithm = nil - uploadedCompressorName = internalblobinfocache.Uncompressed + succeeded = true + return &bpCompressionStepData{ + operation: types.Decompress, + uploadedAlgorithm: nil, + srcCompressorName: detected.srcCompressorName, + uploadedCompressorName: internalblobinfocache.Uncompressed, + closers: closers, + }, nil } else { // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. logrus.Debugf("Using original blob without modification") - operation = types.PreserveOriginal // Remember if the original blob was compressed, and if so how, so that if // LayerInfosForCopy() returned something that differs from what was in the // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), @@ -160,17 +177,15 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob } else { uploadedAlgorithm = nil } - uploadedCompressorName = srcCompressorName + succeeded = true + return &bpCompressionStepData{ + operation: types.PreserveOriginal, + uploadedAlgorithm: uploadedAlgorithm, + srcCompressorName: detected.srcCompressorName, + uploadedCompressorName: detected.srcCompressorName, + closers: closers, + }, nil } - succeeded = true - return &bpCompressionStepData{ - operation: operation, - uploadedAlgorithm: uploadedAlgorithm, - uploadedAnnotations: uploadedAnnotations, - srcCompressorName: srcCompressorName, - uploadedCompressorName: uploadedCompressorName, - closers: closers, - }, nil } // updateCompressionEdits sets *operation, *algorithm and updates *annotations, if necessary. From 7a81dce4f72bee65e8bc3327ead52986c289e7e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 21:13:56 +0200 Subject: [PATCH 28/35] Move uploadedAnnotations inside the individual cases. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/compression.go | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 0bf4b54a0..70abb34dc 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -65,7 +65,6 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob detected bpDetectCompressionStepData) (*bpCompressionStepData, error) { // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions - uploadedAnnotations := map[string]string{} var uploadedAlgorithm *compressiontypes.Algorithm var closers []io.Closer succeeded := false @@ -97,10 +96,11 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob } else { uploadedAlgorithm = defaultCompressionFormat } + annotations := map[string]string{} // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, // we don’t care. - go c.compressGoroutine(pipeWriter, stream.reader, uploadedAnnotations, *uploadedAlgorithm) // Closes pipeWriter + go c.compressGoroutine(pipeWriter, stream.reader, annotations, *uploadedAlgorithm) // Closes pipeWriter stream.reader = pipeReader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", @@ -110,6 +110,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob return &bpCompressionStepData{ operation: types.Compress, uploadedAlgorithm: uploadedAlgorithm, + uploadedAnnotations: annotations, srcCompressorName: detected.srcCompressorName, uploadedCompressorName: uploadedAlgorithm.Name(), closers: closers, @@ -129,7 +130,8 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob pipeReader, pipeWriter := io.Pipe() closers = append(closers, pipeReader) - go c.compressGoroutine(pipeWriter, s, uploadedAnnotations, *c.compressionFormat) // Closes pipeWriter + annotations := map[string]string{} + go c.compressGoroutine(pipeWriter, s, annotations, *c.compressionFormat) // Closes pipeWriter stream.reader = pipeReader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? @@ -140,7 +142,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob return &bpCompressionStepData{ operation: types.PreserveOriginal, uploadedAlgorithm: c.compressionFormat, - uploadedAnnotations: uploadedAnnotations, + uploadedAnnotations: annotations, srcCompressorName: detected.srcCompressorName, uploadedCompressorName: c.compressionFormat.Name(), closers: closers, From 302967dc603ec8a098db425be487b9192d71421c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 21:15:50 +0200 Subject: [PATCH 29/35] Move uploadedAlgorithm inside the individual cases. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- copy/compression.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 70abb34dc..13ab8a724 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -65,7 +65,6 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob detected bpDetectCompressionStepData) (*bpCompressionStepData, error) { // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions - var uploadedAlgorithm *compressiontypes.Algorithm var closers []io.Closer succeeded := false defer func() { @@ -91,6 +90,7 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob pipeReader, pipeWriter := io.Pipe() closers = append(closers, pipeReader) + var uploadedAlgorithm *compressiontypes.Algorithm if c.compressionFormat != nil { uploadedAlgorithm = c.compressionFormat } else { @@ -174,15 +174,16 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // LayerInfosForCopy() returned something that differs from what was in the // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), // it will be able to correctly derive the MediaType for the copied blob. + var algorithm *compressiontypes.Algorithm if detected.isCompressed { - uploadedAlgorithm = &detected.format + algorithm = &detected.format } else { - uploadedAlgorithm = nil + algorithm = nil } succeeded = true return &bpCompressionStepData{ operation: types.PreserveOriginal, - uploadedAlgorithm: uploadedAlgorithm, + uploadedAlgorithm: algorithm, srcCompressorName: detected.srcCompressorName, uploadedCompressorName: detected.srcCompressorName, closers: closers, From c39fe5bc695081a15d1021e69a40bb09fbd508be Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 21:16:25 +0200 Subject: [PATCH 30/35] Clean up the control flow of blobPipelineCompressionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Don't use else after each step that returns early. The function nows looks like a set of ~special cases, with the general fallback of doing nothing. Signed-off-by: Miloslav Trmač --- copy/compression.go | 45 +++++++++++++++++++++++---------------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 13ab8a724..245afdd94 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -115,7 +115,8 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob uploadedCompressorName: uploadedAlgorithm.Name(), closers: closers, }, nil - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detected.isCompressed && + } + if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detected.isCompressed && c.compressionFormat != nil && c.compressionFormat.Name() != detected.format.Name() { // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally // re-compressed using the desired format. @@ -147,7 +148,8 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob uploadedCompressorName: c.compressionFormat.Name(), closers: closers, }, nil - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detected.isCompressed { + } + if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detected.isCompressed { logrus.Debugf("Blob will be decompressed") s, err := detected.decompressor(stream.reader) if err != nil { @@ -167,28 +169,27 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob uploadedCompressorName: internalblobinfocache.Uncompressed, closers: closers, }, nil + } + // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. + logrus.Debugf("Using original blob without modification") + // Remember if the original blob was compressed, and if so how, so that if + // LayerInfosForCopy() returned something that differs from what was in the + // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), + // it will be able to correctly derive the MediaType for the copied blob. + var algorithm *compressiontypes.Algorithm + if detected.isCompressed { + algorithm = &detected.format } else { - // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. - logrus.Debugf("Using original blob without modification") - // Remember if the original blob was compressed, and if so how, so that if - // LayerInfosForCopy() returned something that differs from what was in the - // source's manifest, and UpdatedImage() needs to call UpdateLayerInfos(), - // it will be able to correctly derive the MediaType for the copied blob. - var algorithm *compressiontypes.Algorithm - if detected.isCompressed { - algorithm = &detected.format - } else { - algorithm = nil - } - succeeded = true - return &bpCompressionStepData{ - operation: types.PreserveOriginal, - uploadedAlgorithm: algorithm, - srcCompressorName: detected.srcCompressorName, - uploadedCompressorName: detected.srcCompressorName, - closers: closers, - }, nil + algorithm = nil } + succeeded = true + return &bpCompressionStepData{ + operation: types.PreserveOriginal, + uploadedAlgorithm: algorithm, + srcCompressorName: detected.srcCompressorName, + uploadedCompressorName: detected.srcCompressorName, + closers: closers, + }, nil } // updateCompressionEdits sets *operation, *algorithm and updates *annotations, if necessary. From b6bb69bf4d71bc4ef246294d7eaf8e699ce65d7c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 21:17:51 +0200 Subject: [PATCH 31/35] Rename s to decompressed in the re-compress case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit to avoid ambiguity. Signed-off-by: Miloslav Trmač --- copy/compression.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 245afdd94..9b335f51b 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -122,17 +122,17 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // re-compressed using the desired format. logrus.Debugf("Blob will be converted") - s, err := detected.decompressor(stream.reader) + decompressed, err := detected.decompressor(stream.reader) if err != nil { return nil, err } - closers = append(closers, s) + closers = append(closers, decompressed) pipeReader, pipeWriter := io.Pipe() closers = append(closers, pipeReader) annotations := map[string]string{} - go c.compressGoroutine(pipeWriter, s, annotations, *c.compressionFormat) // Closes pipeWriter + go c.compressGoroutine(pipeWriter, decompressed, annotations, *c.compressionFormat) // Closes pipeWriter stream.reader = pipeReader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? From f70a3906127faf9e8a7a2b3e5434de3ab1ddf9fb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 21:29:23 +0200 Subject: [PATCH 32/35] Factor out copier.compressedStream from copier.blobPipelineCompressionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... so that we only implement the pipe handling once. Signed-off-by: Miloslav Trmač --- copy/compression.go | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 9b335f51b..23233c82c 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -87,21 +87,15 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob }, nil } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detected.isCompressed { logrus.Debugf("Compressing blob on the fly") - pipeReader, pipeWriter := io.Pipe() - closers = append(closers, pipeReader) - var uploadedAlgorithm *compressiontypes.Algorithm if c.compressionFormat != nil { uploadedAlgorithm = c.compressionFormat } else { uploadedAlgorithm = defaultCompressionFormat } - annotations := map[string]string{} - // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, - // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, - // we don’t care. - go c.compressGoroutine(pipeWriter, stream.reader, annotations, *uploadedAlgorithm) // Closes pipeWriter - stream.reader = pipeReader + reader, annotations := c.compressedStream(stream.reader, *uploadedAlgorithm) + closers = append(closers, reader) + stream.reader = reader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", Size: -1, @@ -128,13 +122,9 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob } closers = append(closers, decompressed) - pipeReader, pipeWriter := io.Pipe() - closers = append(closers, pipeReader) - - annotations := map[string]string{} - go c.compressGoroutine(pipeWriter, decompressed, annotations, *c.compressionFormat) // Closes pipeWriter - - stream.reader = pipeReader + recompressed, annotations := c.compressedStream(decompressed, *c.compressionFormat) + closers = append(closers, recompressed) + stream.reader = recompressed stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", Size: -1, @@ -274,3 +264,16 @@ func (c *copier) compressGoroutine(dest *io.PipeWriter, src io.Reader, metadata err = doCompression(dest, src, metadata, compressionFormat, c.compressionLevel) } + +// compressedStream returns a stream the input reader compressed using format, and a metadata map. +// The caller must close the returned reader. +// AFTER the stream is consumed, metadata will be updated with annotations to use on the data. +func (c *copier) compressedStream(reader io.Reader, algorithm compressiontypes.Algorithm) (io.ReadCloser, map[string]string) { + pipeReader, pipeWriter := io.Pipe() + annotations := map[string]string{} + // If this fails while writing data, it will do pipeWriter.CloseWithError(); if it fails otherwise, + // e.g. because we have exited and due to pipeReader.Close() above further writing to the pipe has failed, + // we don’t care. + go c.compressGoroutine(pipeWriter, reader, annotations, algorithm) // Closes pipeWriter + return pipeReader, annotations +} From 73bf1aea3880de172aa2770c447de30f72282624 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 23:20:12 +0200 Subject: [PATCH 33/35] Split blobPipelineCompressionStep into individual functions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The logic is a simple sequence of condition + decision, which is fairly regullar, and doesn't have to be in a single function. So, split it up. Eliminate the recently-introduced closers array again, in most cases. Should not change behavior. Signed-off-by: Miloslav Trmač --- copy/compression.go | 99 +++++++++++++++++++++++++++++++++------------ 1 file changed, 74 insertions(+), 25 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 23233c82c..401ec773c 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -65,27 +65,55 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob detected bpDetectCompressionStepData) (*bpCompressionStepData, error) { // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions - var closers []io.Closer - succeeded := false - defer func() { - if !succeeded { - for _, c := range closers { - c.Close() - } + if canModifyBlob { + if res := c.bpcPreserveEncrypted(stream); res != nil { + return res, nil } - }() - if canModifyBlob && isOciEncrypted(stream.info.MediaType) { + } + if canModifyBlob { + if res := c.bpcCompressUncompressed(stream, detected); res != nil { + return res, nil + } + } + if canModifyBlob { + res, err := c.bpcRecompressCompressed(stream, detected) + if err != nil { + return nil, err + } + if res != nil { + return res, nil + } + } + if canModifyBlob { + res, err := c.bpcDecompressCompressed(stream, detected) + if err != nil { + return nil, err + } + if res != nil { + return res, nil + } + } + return c.bpcPreserveOriginal(stream, detected), nil +} + +// bpcPreserveEncrypted checks if the input is encrypted, and returns a *bpCompressionStepData if so. +func (c *copier) bpcPreserveEncrypted(stream *sourceStream) *bpCompressionStepData { + if isOciEncrypted(stream.info.MediaType) { // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted logrus.Debugf("Using original blob without modification for encrypted blob") - succeeded = true return &bpCompressionStepData{ operation: types.PreserveOriginal, uploadedAlgorithm: nil, srcCompressorName: internalblobinfocache.UnknownCompression, uploadedCompressorName: internalblobinfocache.UnknownCompression, - closers: closers, - }, nil - } else if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && !detected.isCompressed { + } + } + return nil +} + +// bpcCompressUncompressed checks if we should be compressing an uncompressed input, and returns a *bpCompressionStepData if so. +func (c *copier) bpcCompressUncompressed(stream *sourceStream, detected bpDetectCompressionStepData) *bpCompressionStepData { + if c.dest.DesiredLayerCompression() == types.Compress && !detected.isCompressed { logrus.Debugf("Compressing blob on the fly") var uploadedAlgorithm *compressiontypes.Algorithm if c.compressionFormat != nil { @@ -93,29 +121,43 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob } else { uploadedAlgorithm = defaultCompressionFormat } + reader, annotations := c.compressedStream(stream.reader, *uploadedAlgorithm) - closers = append(closers, reader) + // Note: reader must be closed on all return paths. stream.reader = reader stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", Size: -1, } - succeeded = true return &bpCompressionStepData{ operation: types.Compress, uploadedAlgorithm: uploadedAlgorithm, uploadedAnnotations: annotations, srcCompressorName: detected.srcCompressorName, uploadedCompressorName: uploadedAlgorithm.Name(), - closers: closers, - }, nil + closers: []io.Closer{reader}, + } } - if canModifyBlob && c.dest.DesiredLayerCompression() == types.Compress && detected.isCompressed && + return nil +} + +// bpcRecompressCompressed checks if we should be recompressing a compressed input to another format, and returns a *bpCompressionStepData if so. +func (c *copier) bpcRecompressCompressed(stream *sourceStream, detected bpDetectCompressionStepData) (*bpCompressionStepData, error) { + if c.dest.DesiredLayerCompression() == types.Compress && detected.isCompressed && c.compressionFormat != nil && c.compressionFormat.Name() != detected.format.Name() { // When the blob is compressed, but the desired format is different, it first needs to be decompressed and finally // re-compressed using the desired format. logrus.Debugf("Blob will be converted") + var closers []io.Closer + succeeded := false + defer func() { + if !succeeded { + for _, c := range closers { + c.Close() + } + } + }() decompressed, err := detected.decompressor(stream.reader) if err != nil { return nil, err @@ -139,27 +181,36 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob closers: closers, }, nil } - if canModifyBlob && c.dest.DesiredLayerCompression() == types.Decompress && detected.isCompressed { + return nil, nil +} + +// bpcDecompressCompressed checks if we should be decompressing a compressed input, and returns a *bpCompressionStepData if so. +func (c *copier) bpcDecompressCompressed(stream *sourceStream, detected bpDetectCompressionStepData) (*bpCompressionStepData, error) { + if c.dest.DesiredLayerCompression() == types.Decompress && detected.isCompressed { logrus.Debugf("Blob will be decompressed") s, err := detected.decompressor(stream.reader) if err != nil { return nil, err } - closers = append(closers, s) + // Note: s must be closed on all return paths. stream.reader = s stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", Size: -1, } - succeeded = true return &bpCompressionStepData{ operation: types.Decompress, uploadedAlgorithm: nil, srcCompressorName: detected.srcCompressorName, uploadedCompressorName: internalblobinfocache.Uncompressed, - closers: closers, + closers: []io.Closer{s}, }, nil } + return nil, nil +} + +// bpcPreserveOriginal returns a *bpCompressionStepData for not changing the original blob. +func (c *copier) bpcPreserveOriginal(stream *sourceStream, detected bpDetectCompressionStepData) *bpCompressionStepData { // PreserveOriginal might also need to recompress the original blob if the desired compression format is different. logrus.Debugf("Using original blob without modification") // Remember if the original blob was compressed, and if so how, so that if @@ -172,14 +223,12 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob } else { algorithm = nil } - succeeded = true return &bpCompressionStepData{ operation: types.PreserveOriginal, uploadedAlgorithm: algorithm, srcCompressorName: detected.srcCompressorName, uploadedCompressorName: detected.srcCompressorName, - closers: closers, - }, nil + } } // updateCompressionEdits sets *operation, *algorithm and updates *annotations, if necessary. From 20f06389cd22394b930f76ef2d44a7e3378735e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Thu, 16 Jun 2022 01:06:28 +0200 Subject: [PATCH 34/35] Eliminate the closers array in bpcRecompressCompressed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... now that it only could effectively apply to a single element. Shoud not change behavior. Signed-off-by: Miloslav Trmač --- copy/compression.go | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 401ec773c..4100342c2 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -149,23 +149,19 @@ func (c *copier) bpcRecompressCompressed(stream *sourceStream, detected bpDetect // re-compressed using the desired format. logrus.Debugf("Blob will be converted") - var closers []io.Closer + decompressed, err := detected.decompressor(stream.reader) + if err != nil { + return nil, err + } succeeded := false defer func() { if !succeeded { - for _, c := range closers { - c.Close() - } + decompressed.Close() } }() - decompressed, err := detected.decompressor(stream.reader) - if err != nil { - return nil, err - } - closers = append(closers, decompressed) recompressed, annotations := c.compressedStream(decompressed, *c.compressionFormat) - closers = append(closers, recompressed) + // Note: recompressed must be closed on all return paths. stream.reader = recompressed stream.info = types.BlobInfo{ // FIXME? Should we preserve more data in src.info? Digest: "", @@ -178,7 +174,7 @@ func (c *copier) bpcRecompressCompressed(stream *sourceStream, detected bpDetect uploadedAnnotations: annotations, srcCompressorName: detected.srcCompressorName, uploadedCompressorName: c.compressionFormat.Name(), - closers: closers, + closers: []io.Closer{decompressed, recompressed}, }, nil } return nil, nil From 2756d705679131780f095473bd9a9aa51019115d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Wed, 15 Jun 2022 23:54:46 +0200 Subject: [PATCH 35/35] Use a loop for the alternatives in blobPipelineCompressionStep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The alternatives are, mostly, things to _try_; we can typically fall back to just using the original. So, just use a loop to take advantage of the regular structure. Signed-off-by: Miloslav Trmač --- copy/compression.go | 50 +++++++++++++++++---------------------------- 1 file changed, 19 insertions(+), 31 deletions(-) diff --git a/copy/compression.go b/copy/compression.go index 4100342c2..ddfe67982 100644 --- a/copy/compression.go +++ b/copy/compression.go @@ -66,38 +66,26 @@ func (c *copier) blobPipelineCompressionStep(stream *sourceStream, canModifyBlob // WARNING: If you are adding new reasons to change the blob, update also the OptimizeDestinationImageAlreadyExists // short-circuit conditions if canModifyBlob { - if res := c.bpcPreserveEncrypted(stream); res != nil { - return res, nil - } - } - if canModifyBlob { - if res := c.bpcCompressUncompressed(stream, detected); res != nil { - return res, nil - } - } - if canModifyBlob { - res, err := c.bpcRecompressCompressed(stream, detected) - if err != nil { - return nil, err - } - if res != nil { - return res, nil - } - } - if canModifyBlob { - res, err := c.bpcDecompressCompressed(stream, detected) - if err != nil { - return nil, err - } - if res != nil { - return res, nil + for _, fn := range []func(*sourceStream, bpDetectCompressionStepData) (*bpCompressionStepData, error){ + c.bpcPreserveEncrypted, + c.bpcCompressUncompressed, + c.bpcRecompressCompressed, + c.bpcDecompressCompressed, + } { + res, err := fn(stream, detected) + if err != nil { + return nil, err + } + if res != nil { + return res, nil + } } } return c.bpcPreserveOriginal(stream, detected), nil } // bpcPreserveEncrypted checks if the input is encrypted, and returns a *bpCompressionStepData if so. -func (c *copier) bpcPreserveEncrypted(stream *sourceStream) *bpCompressionStepData { +func (c *copier) bpcPreserveEncrypted(stream *sourceStream, _ bpDetectCompressionStepData) (*bpCompressionStepData, error) { if isOciEncrypted(stream.info.MediaType) { // PreserveOriginal due to any compression not being able to be done on an encrypted blob unless decrypted logrus.Debugf("Using original blob without modification for encrypted blob") @@ -106,13 +94,13 @@ func (c *copier) bpcPreserveEncrypted(stream *sourceStream) *bpCompressionStepDa uploadedAlgorithm: nil, srcCompressorName: internalblobinfocache.UnknownCompression, uploadedCompressorName: internalblobinfocache.UnknownCompression, - } + }, nil } - return nil + return nil, nil } // bpcCompressUncompressed checks if we should be compressing an uncompressed input, and returns a *bpCompressionStepData if so. -func (c *copier) bpcCompressUncompressed(stream *sourceStream, detected bpDetectCompressionStepData) *bpCompressionStepData { +func (c *copier) bpcCompressUncompressed(stream *sourceStream, detected bpDetectCompressionStepData) (*bpCompressionStepData, error) { if c.dest.DesiredLayerCompression() == types.Compress && !detected.isCompressed { logrus.Debugf("Compressing blob on the fly") var uploadedAlgorithm *compressiontypes.Algorithm @@ -136,9 +124,9 @@ func (c *copier) bpcCompressUncompressed(stream *sourceStream, detected bpDetect srcCompressorName: detected.srcCompressorName, uploadedCompressorName: uploadedAlgorithm.Name(), closers: []io.Closer{reader}, - } + }, nil } - return nil + return nil, nil } // bpcRecompressCompressed checks if we should be recompressing a compressed input to another format, and returns a *bpCompressionStepData if so.