From c2327e41a4dd893656699e897688a898b1aa5450 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 20 May 2024 19:21:47 +0200 Subject: [PATCH 1/3] Don't unnecessarily trust the ALS FUSE server about the TOC digest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Well, we trust it _anyway_ to actually validate the TOC digest and enforce layer consistency, but it's simpler to use the known-trusted value than to worry about the backend's trust. Also add a FIXME about the case when the value is "". Signed-off-by: Miloslav Trmač --- storage/storage_dest.go | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/storage/storage_dest.go b/storage/storage_dest.go index fe3fcf653f..a0b347410d 100644 --- a/storage/storage_dest.go +++ b/storage/storage_dest.go @@ -388,11 +388,17 @@ func (s *storageImageDestination) tryReusingBlobAsPending(blobDigest digest.Dige if err != nil && !errors.Is(err, storage.ErrLayerUnknown) { return false, private.ReusedBlob{}, fmt.Errorf(`looking for compressed layers with digest %q and labels: %w`, blobDigest, err) } else if err == nil { - d := aLayer.TOCDigest() - if d == "" { - return false, private.ReusedBlob{}, fmt.Errorf(`failed to get TOCDigest of %q: %w`, blobDigest, err) + alsTOCDigest := aLayer.TOCDigest() + if alsTOCDigest != options.TOCDigest { + // FIXME: If alsTOCDigest is "", the Additional Layer Store FUSE server is probably just too old, and we could + // probably go on reading the layer from other sources. + // + // Currently it should not be possible for alsTOCDigest to be set and not the expected value, but there’s + // not that much benefit to checking for equality — we trust the FUSE server to validate the digest either way. + return false, private.ReusedBlob{}, fmt.Errorf("additional layer for TOCDigest %q reports unexpected TOCDigest %q", + options.TOCDigest, alsTOCDigest) } - s.lockProtected.indexToTOCDigest[*options.LayerIndex] = d + s.lockProtected.indexToTOCDigest[*options.LayerIndex] = options.TOCDigest s.lockProtected.indexToAdditionalLayer[*options.LayerIndex] = aLayer return true, private.ReusedBlob{ Digest: blobDigest, From 27516f32d62471fc5939e535a9b804e8fb5b7b0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 20 May 2024 19:25:03 +0200 Subject: [PATCH 2/3] Don't modify a storage.Layer returned by c/storage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This shouldn't matter in practice because the returned struct happens to be a copy, but that's not promised by the API. Should not change behavior. Signed-off-by: Miloslav Trmač --- storage/storage_src.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/storage/storage_src.go b/storage/storage_src.go index 2971f7fe9f..e4fc5b52c5 100644 --- a/storage/storage_src.go +++ b/storage/storage_src.go @@ -307,9 +307,6 @@ func (s *storageImageSource) LayerInfosForCopy(ctx context.Context, instanceDige if err != nil { return nil, fmt.Errorf("reading layer %q in image %q: %w", layerID, s.image.ID, err) } - if layer.UncompressedSize < 0 { - layer.UncompressedSize = -1 - } blobDigest := layer.UncompressedDigest if blobDigest == "" { @@ -331,12 +328,16 @@ func (s *storageImageSource) LayerInfosForCopy(ctx context.Context, instanceDige return nil, fmt.Errorf("parsing expected diffID %q for layer %q: %w", expectedDigest, layerID, err) } } + size := layer.UncompressedSize + if size < 0 { + size = -1 + } s.getBlobMutex.Lock() s.getBlobMutexProtected.digestToLayerID[blobDigest] = layer.ID s.getBlobMutex.Unlock() blobInfo := types.BlobInfo{ Digest: blobDigest, - Size: layer.UncompressedSize, + Size: size, MediaType: uncompressedLayerType, } physicalBlobInfos = append([]types.BlobInfo{blobInfo}, physicalBlobInfos...) From 45f4f2302cdfa0d3de76d3f956926d3d3ff3739c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 20 May 2024 19:40:32 +0200 Subject: [PATCH 3/3] Don't completely ignore already-computed image size if we see an ALS layer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ... but silently ignore the ALS layer. Also add a FIXME. Signed-off-by: Miloslav Trmač --- storage/storage_src.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/storage/storage_src.go b/storage/storage_src.go index e4fc5b52c5..4f501fc22a 100644 --- a/storage/storage_src.go +++ b/storage/storage_src.go @@ -456,10 +456,13 @@ func (s *storageImageSource) getSize() (int64, error) { if (layer.TOCDigest == "" && layer.UncompressedDigest == "") || (layer.TOCDigest == "" && layer.UncompressedSize < 0) { return -1, fmt.Errorf("size for layer %q is unknown, failing getSize()", layerID) } - if layer.UncompressedSize < 0 { - sum = 0 + // FIXME: We allow layer.UncompressedSize < 0 above, because currently images in an Additional Layer Store don’t provide that value. + // Right now, various callers in Podman (and, also, newImage in this package) don’t expect the size computation to fail. + // Should we update the callers, or do we need to continue returning inaccurate information here? Or should we pay the cost + // to compute the size from the diff? + if layer.UncompressedSize >= 0 { + sum += layer.UncompressedSize } - sum += layer.UncompressedSize if layer.Parent == "" { break }