Visitar URL original
fix: preserve per-occurrence layer identity in Layers() (#2299) · google/go-containerregistry@68a569e · GitHub
Skip to content

Commit 68a569e

Browse files
iahsanGilliahsanGill
authored
fix: preserve per-occurrence layer identity in Layers() (#2299)
When a base image and an appended layer share a diff ID but have different blob digests (e.g. same uncompressed tar, different compression), mutate.Image.Layers() previously returned the appended layer twice instead of [base, appended]. The manifest still listed both digests correctly, but downstream code that walks Layers() to upload blobs would upload the appended layer twice and never push the base's blob, so the PUT manifest would fail with MANIFEST_BLOB_UNKNOWN. Root cause: Layers() walked the rootfs diff IDs and resolved each via LayerByDiffID, which can't disambiguate two layers with the same diff ID — the addendum overwrites the base entry in diffIDMap. The manifest layer descriptors are already unambiguous (one entry per occurrence, each with its own digest), so walk those instead and resolve via LayerByDigest. Same length and order, but per-occurrence identity is preserved. Fixes #2034
1 parent 35b354b commit 68a569e

2 files changed

Lines changed: 144 additions & 7 deletions

File tree

‎pkg/v1/mutate/image.go‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -212,13 +212,16 @@ func (i *image) Layers() ([]v1.Layer, error) {
212212
return nil, err
213213
}
214214

215-
diffIDs, err := partial.DiffIDs(i)
216-
if err != nil {
217-
return nil, err
218-
}
219-
ls := make([]v1.Layer, 0, len(diffIDs))
220-
for _, h := range diffIDs {
221-
l, err := i.LayerByDiffID(h)
215+
// Walk manifest layer descriptors by digest rather than rootfs diff
216+
// IDs. Two layers can legitimately share a diff ID — same uncompressed
217+
// content, different compression — and produce distinct digests. The
218+
// manifest preserves the per-occurrence digest; LayerByDiffID does not,
219+
// which previously caused duplicate-diff-ID layers to collapse to a
220+
// single entry in the returned slice and break blob upload for
221+
// downstream pushers (see #2034).
222+
ls := make([]v1.Layer, 0, len(i.manifest.Layers))
223+
for _, desc := range i.manifest.Layers {
224+
l, err := i.LayerByDigest(desc.Digest)
222225
if err != nil {
223226
return nil, err
224227
}

‎pkg/v1/mutate/mutate_test.go‎

Lines changed: 134 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1104,3 +1104,137 @@ func (rc *tokenReleasingReadCloser) Close() error {
11041104
}
11051105
return nil
11061106
}
1107+
1108+
// duplicateDiffIDLayer wraps a v1.Layer so that the wrapper reports the
1109+
// inner layer's DiffID but its own (distinct) Digest, Size, and Compressed
1110+
// stream. This models the real-world case behind #2034: two layers built
1111+
// from the same uncompressed tar but with different compression settings
1112+
// share a diff ID and have distinct blob digests.
1113+
type duplicateDiffIDLayer struct {
1114+
v1.Layer
1115+
digest v1.Hash
1116+
compressed []byte
1117+
}
1118+
1119+
func (d *duplicateDiffIDLayer) Digest() (v1.Hash, error) { return d.digest, nil }
1120+
func (d *duplicateDiffIDLayer) Size() (int64, error) { return int64(len(d.compressed)), nil }
1121+
func (d *duplicateDiffIDLayer) Compressed() (io.ReadCloser, error) {
1122+
return io.NopCloser(bytes.NewReader(d.compressed)), nil
1123+
}
1124+
1125+
// TestAppendLayers_DuplicateDiffID is a regression test for #2034: when a
1126+
// base image layer and an appended layer share a diff ID but have different
1127+
// blob digests, Layers() must return both as distinct entries (one per
1128+
// manifest descriptor). Walking the rootfs diff IDs and resolving each via
1129+
// LayerByDiffID — the old behavior — collapsed the duplicate-diff-ID entry
1130+
// to a single layer, which broke downstream pushers that uploaded blobs
1131+
// based on Layers() (resulting in MANIFEST_BLOB_UNKNOWN at push time).
1132+
func TestAppendLayers_DuplicateDiffID(t *testing.T) {
1133+
base, err := random.Image(1024, 1)
1134+
if err != nil {
1135+
t.Fatal(err)
1136+
}
1137+
baseLayers, err := base.Layers()
1138+
if err != nil {
1139+
t.Fatal(err)
1140+
}
1141+
baseLayer := baseLayers[0]
1142+
1143+
baseDigest, err := baseLayer.Digest()
1144+
if err != nil {
1145+
t.Fatal(err)
1146+
}
1147+
baseDiffID, err := baseLayer.DiffID()
1148+
if err != nil {
1149+
t.Fatal(err)
1150+
}
1151+
1152+
// Construct an appended layer that reports baseLayer's diff ID but a
1153+
// fabricated, distinct digest. We don't care that the bytes are
1154+
// "really" a valid recompression — the bug is in how Layers() resolves
1155+
// per-occurrence layers from the manifest, not how the blob is
1156+
// validated.
1157+
appendBlob := []byte("a different compressed blob for the same uncompressed content")
1158+
fakeDigest := v1.Hash{Algorithm: "sha256", Hex: strings.Repeat("ab", 32)}
1159+
if fakeDigest == baseDigest {
1160+
t.Fatalf("test setup invariant broken: fabricated digest collides with base")
1161+
}
1162+
appended := &duplicateDiffIDLayer{
1163+
Layer: baseLayer,
1164+
digest: fakeDigest,
1165+
compressed: appendBlob,
1166+
}
1167+
1168+
result, err := mutate.AppendLayers(base, appended)
1169+
if err != nil {
1170+
t.Fatalf("AppendLayers failed: %v", err)
1171+
}
1172+
1173+
// Quick check: the manifest must list both digests in order.
1174+
m, err := result.Manifest()
1175+
if err != nil {
1176+
t.Fatal(err)
1177+
}
1178+
if got, want := len(m.Layers), 2; got != want {
1179+
t.Fatalf("manifest layers: got %d, want %d", got, want)
1180+
}
1181+
if m.Layers[0].Digest != baseDigest {
1182+
t.Errorf("manifest layers[0].Digest: got %s, want %s", m.Layers[0].Digest, baseDigest)
1183+
}
1184+
if m.Layers[1].Digest != fakeDigest {
1185+
t.Errorf("manifest layers[1].Digest: got %s, want %s", m.Layers[1].Digest, fakeDigest)
1186+
}
1187+
1188+
// The bug: Layers() previously walked diffIDs and resolved via
1189+
// LayerByDiffID, which collapsed both entries to the appended layer.
1190+
// Both slots returned a layer with digest=fakeDigest.
1191+
layers, err := result.Layers()
1192+
if err != nil {
1193+
t.Fatalf("Layers() failed: %v", err)
1194+
}
1195+
if got, want := len(layers), 2; got != want {
1196+
t.Fatalf("Layers(): got %d, want %d", got, want)
1197+
}
1198+
1199+
d0, err := layers[0].Digest()
1200+
if err != nil {
1201+
t.Fatal(err)
1202+
}
1203+
if d0 != baseDigest {
1204+
t.Errorf("Layers()[0].Digest(): got %s, want %s (base) — duplicate-diff-ID collapse regression", d0, baseDigest)
1205+
}
1206+
1207+
d1, err := layers[1].Digest()
1208+
if err != nil {
1209+
t.Fatal(err)
1210+
}
1211+
if d1 != fakeDigest {
1212+
t.Errorf("Layers()[1].Digest(): got %s, want %s (appended)", d1, fakeDigest)
1213+
}
1214+
1215+
// Round-trip check: both layers must also be retrievable via
1216+
// LayerByDigest using the digests reported in the manifest.
1217+
if _, err := result.LayerByDigest(baseDigest); err != nil {
1218+
t.Errorf("LayerByDigest(base): %v", err)
1219+
}
1220+
if _, err := result.LayerByDigest(fakeDigest); err != nil {
1221+
t.Errorf("LayerByDigest(appended): %v", err)
1222+
}
1223+
1224+
// The diff IDs must match — confirming the test setup actually
1225+
// exercises the duplicate-diff-ID case.
1226+
id0, err := layers[0].DiffID()
1227+
if err != nil {
1228+
t.Fatal(err)
1229+
}
1230+
id1, err := layers[1].DiffID()
1231+
if err != nil {
1232+
t.Fatal(err)
1233+
}
1234+
if id0 != id1 {
1235+
t.Fatalf("test setup invariant: expected duplicate diff IDs, got %s and %s", id0, id1)
1236+
}
1237+
if id0 != baseDiffID {
1238+
t.Fatalf("test setup invariant: expected base diff ID %s, got %s", baseDiffID, id0)
1239+
}
1240+
}

0 commit comments

Comments
 (0)