From b6e1771ba896f79d0a55aa50cf381b4440884750 Mon Sep 17 00:00:00 2001 From: ggjulio Date: Mon, 2 Nov 2026 11:49:14 +0100 Subject: [PATCH] remotecache: never export a partial layer chain A missing blob below the top layer exported the layers above it as a new chain, without the image layers. Drop the whole result instead. Fixes #7350 Signed-off-by: ggjulio --- cache/remotecache/v1/chains_test.go | 77 +++++++++++++++++++++++++++++ cache/remotecache/v1/utils.go | 57 ++++++++++----------- 2 files changed, 102 insertions(+), 32 deletions(-) diff --git a/cache/remotecache/v1/chains_test.go b/cache/remotecache/v1/chains_test.go index 888ed4c72..e88de347f 100644 --- a/cache/remotecache/v1/chains_test.go +++ b/cache/remotecache/v1/chains_test.go @@ -1,14 +1,19 @@ package cacheimport import ( + "context" "encoding/json" "testing" "time" + "github.com/containerd/containerd/v2/core/content" "github.com/containerd/containerd/v2/core/images" + cerrdefs "github.com/containerd/errdefs" + cacheimporttypes "github.com/moby/buildkit/cache/remotecache/v1/types" "github.com/moby/buildkit/solver" digest "github.com/opencontainers/go-digest" ocispecs "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/pkg/errors" "github.com/stretchr/testify/require" ) @@ -162,3 +167,75 @@ func TestMarshalLayerMediaTypes(t *testing.T) { func dgst(s string) digest.Digest { return digest.FromBytes([]byte(s)) } + +// countingProvider fails Info for a blob once it has been checked more than failAfter times, modelling a blob +// that becomes unavailable while the config is being marshaled. +type countingProvider struct { + failAfter map[digest.Digest]int + calls map[digest.Digest]int +} + +func (p *countingProvider) Info(_ context.Context, dgst digest.Digest) (content.Info, error) { + p.calls[dgst]++ + if n, ok := p.failAfter[dgst]; ok && p.calls[dgst] > n { + return content.Info{}, errors.Wrapf(cerrdefs.ErrNotFound, "blob %s", dgst) + } + return content.Info{Digest: dgst}, nil +} + +func (p *countingProvider) ReaderAt(context.Context, ocispecs.Descriptor) (content.ReaderAt, error) { + return nil, cerrdefs.ErrNotImplemented +} + +func TestMarshalUnavailableBlob(t *testing.T) { + chain := []ocispecs.Descriptor{{Digest: dgst("d0")}, {Digest: dgst("d1")}, {Digest: dgst("d2")}} + + marshal := func(t *testing.T, p *countingProvider) *cacheimporttypes.CacheConfig { + cc := NewCacheChains() + base, ok, err := cc.Add(outputKey(dgst("base"), 0), nil, nil) + require.NoError(t, err) + require.True(t, ok) + // built with append: a one-element literal makes gosec report G602 on deps[i][j] in Add + var deps [][]solver.CacheLink + deps = append(deps, []solver.CacheLink{{Src: base}}) + _, ok, err = cc.Add(outputKey(dgst("run"), 0), deps, []solver.CacheExportResult{{ + CreatedAt: time.Now(), + Result: &solver.Remote{Descriptors: chain, Provider: p}, + }}) + require.NoError(t, err) + require.True(t, ok) + cfg, _, err := cc.Marshal(t.Context()) + require.NoError(t, err) + return cfg + } + + t.Run("unavailable", func(t *testing.T) { + // the bottom blob is gone: the result is dropped, nothing of its chain is exported + cfg := marshal(t, &countingProvider{failAfter: map[digest.Digest]int{dgst("d0"): 0}, calls: map[digest.Digest]int{}}) + require.Empty(t, cfg.Layers) + for _, r := range cfg.Records { + require.Empty(t, r.Results) + } + }) + + t.Run("becomes unavailable", func(t *testing.T) { + // the bottom blob passes the first check and fails afterwards: the chain is exported whole or not at all, + // never rooted on d1 or d2 + cfg := marshal(t, &countingProvider{failAfter: map[digest.Digest]int{dgst("d0"): 1}, calls: map[digest.Digest]int{}}) + for _, l := range cfg.Layers { + for l.ParentIndex != -1 { + l = cfg.Layers[l.ParentIndex] + } + require.Equal(t, dgst("d0"), l.Blob, "layer chain rooted on %s instead of the image root", l.Blob) + } + for _, r := range cfg.Records { + for _, res := range r.Results { + n, l := 1, cfg.Layers[res.LayerIndex] + for l.ParentIndex != -1 { + l, n = cfg.Layers[l.ParentIndex], n+1 + } + require.Equal(t, len(chain), n) + } + } + }) +} diff --git a/cache/remotecache/v1/utils.go b/cache/remotecache/v1/utils.go index 8e004d862..dba07903c 100644 --- a/cache/remotecache/v1/utils.go +++ b/cache/remotecache/v1/utils.go @@ -10,6 +10,7 @@ import ( cerrdefs "github.com/containerd/errdefs" cacheimporttypes "github.com/moby/buildkit/cache/remotecache/v1/types" "github.com/moby/buildkit/solver" + "github.com/moby/buildkit/util/bklog" "github.com/moby/buildkit/util/compression" digest "github.com/opencontainers/go-digest" "github.com/pkg/errors" @@ -136,48 +137,40 @@ func marshalRemote(ctx context.Context, r *solver.Remote, state *marshalState) s if r.Provider != nil { for _, d := range r.Descriptors { - if _, err := r.Provider.Info(ctx, d.Digest); err != nil { - if !cerrdefs.IsNotImplemented(err) { - return "" - } + if _, err := r.Provider.Info(ctx, d.Digest); err != nil && !cerrdefs.IsNotImplemented(err) { + bklog.G(ctx).WithError(err).Warnf("skipping cache result: blob %s is not available", d.Digest) + return "" } } } var parentID string - if len(r.Descriptors) > 1 { - r2 := &solver.Remote{ - Descriptors: r.Descriptors[:len(r.Descriptors)-1], - Provider: r.Provider, + for _, desc := range r.Descriptors { + if desc.MediaType != "" { + desc = compression.ConvertAllLayerMediaTypes(ctx, true, desc)[0] } - parentID = marshalRemote(ctx, r2, state) - } - desc := r.Descriptors[len(r.Descriptors)-1] - if desc.MediaType != "" { - desc = compression.ConvertAllLayerMediaTypes(ctx, true, desc)[0] - } - state.descriptors[desc.Digest] = DescriptorProviderPair{ - Descriptor: desc, - Provider: r.Provider, - } + state.descriptors[desc.Digest] = DescriptorProviderPair{ + Descriptor: desc, + Provider: r.Provider, + } - id := desc.Digest.String() + parentID + id := desc.Digest.String() + parentID - if _, ok := state.chainsByID[id]; ok { - return id + if _, ok := state.chainsByID[id]; !ok { + state.chainsByID[id] = len(state.layers) + l := cacheimporttypes.CacheLayer{ + Blob: desc.Digest, + ParentIndex: -1, + } + if parentID != "" { + l.ParentIndex = state.chainsByID[parentID] + } + state.layers = append(state.layers, l) + } + parentID = id } - - state.chainsByID[id] = len(state.layers) - l := cacheimporttypes.CacheLayer{ - Blob: desc.Digest, - ParentIndex: -1, - } - if parentID != "" { - l.ParentIndex = state.chainsByID[parentID] - } - state.layers = append(state.layers, l) - return id + return parentID } func marshalItem(ctx context.Context, it *item, state *marshalState) error {