Repository: google/go-containerregistry
Pull request #935 (2021-02-05 23:23:23)

Access v1.Layer via our index impls
See #819

jonjohnsonjr: PTAL, added bunches of tests.
dlorenc: ![image](https://user-images.githubusercontent.com/1714486/107101563-5c5d8c00-67dd-11eb-932f-a3d2993018fb.png)
You love to see it
jonjohnsonjr: > You love to see it

Someone has to care.

@clrprod I got your registry code back up to 100% coverage after @ImJasonH desecrated it. Still need to drop the external dependencies back down to zero... babysteps.

Patch:
diff --git a/pkg/registry/manifest.go b/pkg/registry/manifest.go
index 6a08f5b4..66f96661 100644
--- a/pkg/registry/manifest.go
+++ b/pkg/registry/manifest.go
@@ -131,24 +131,29 @@ func (m *manifests) handle(resp http.ResponseWriter, req *http.Request) *regErro
 		// list's constituent manifests are already uploaded.
 		// This isn't strictly required by the registry API, but some
 		// registries require this.
-		if mf.contentType == string(types.OCIImageIndex) ||
-			mf.contentType == string(types.DockerManifestList) {
-
+		if types.MediaType(mf.contentType).IsIndex() {
 			im, err := v1.ParseIndexManifest(b)
 			if err != nil {
 				return &regError{
-					Status:  http.StatusNotFound,
-					Code:    "MANIFEST_UNKNOWN",
+					Status:  http.StatusBadRequest,
+					Code:    "MANIFEST_INVALID",
 					Message: err.Error(),
 				}
 			}
 			for _, desc := range im.Manifests {
-				if _, found := m.manifests[repo][desc.Digest.String()]; !found {
-					return &regError{
-						Status:  http.StatusNotFound,
-						Code:    "MANIFEST_UNKNOWN",
-						Message: fmt.Sprintf("Sub-manifest %q not found", desc.Digest),
+				if !desc.MediaType.IsDistributable() {
+					continue
+				}
+				if desc.MediaType.IsIndex() || desc.MediaType.IsImage() {
+					if _, found := m.manifests[repo][desc.Digest.String()]; !found {
+						return &regError{
+							Status:  http.StatusNotFound,
+							Code:    "MANIFEST_UNKNOWN",
+							Message: fmt.Sprintf("Sub-manifest %q not found", desc.Digest),
+						}
 					}
+				} else {
+					// TODO: Probably want to do an existence check for blobs.
 				}
 			}
 		}
diff --git a/pkg/v1/layout/write.go b/pkg/v1/layout/write.go
index 2f0a162c..9437e315 100644
--- a/pkg/v1/layout/write.go
+++ b/pkg/v1/layout/write.go
@@ -314,6 +314,14 @@ func (l Path) WriteImage(img v1.Image) error {
 	return l.WriteBlob(d, ioutil.NopCloser(bytes.NewReader(manifest)))
 }
 
+type withLayer interface {
+	Layer(v1.Hash) (v1.Layer, error)
+}
+
+type withBlob interface {
+	Blob(v1.Hash) (io.ReadCloser, error)
+}
+
 func (l Path) writeIndexToFile(indexFile string, ii v1.ImageIndex) error {
 	index, err := ii.IndexManifest()
 	if err != nil {
@@ -343,6 +351,24 @@ func (l Path) writeIndexToFile(indexFile string, ii v1.ImageIndex) error {
 		default:
 			// TODO: The layout could reference arbitrary things, which we should
 			// probably just pass through.
+
+			var blob io.ReadCloser
+			// Workaround for #819.
+			if wl, ok := ii.(withLayer); ok {
+				layer, err := wl.Layer(desc.Digest)
+				if err != nil {
+					return err
+				}
+				blob, err = layer.Compressed()
+			} else if wb, ok := ii.(withBlob); ok {
+				blob, err = wb.Blob(desc.Digest)
+			}
+			if err != nil {
+				return err
+			}
+			if err := l.WriteBlob(desc.Digest, blob); err != nil {
+				return err
+			}
 		}
 	}
 
diff --git a/pkg/v1/mutate/index.go b/pkg/v1/mutate/index.go
index 140d99aa..da3eae8d 100644
--- a/pkg/v1/mutate/index.go
+++ b/pkg/v1/mutate/index.go
@@ -16,6 +16,7 @@ package mutate
 
 import (
 	"encoding/json"
+	"fmt"
 	"strings"
 
 	"github.com/google/go-containerregistry/pkg/logs"
@@ -65,6 +66,7 @@ type index struct {
 	mediaType *types.MediaType
 	imageMap  map[v1.Hash]v1.Image
 	indexMap  map[v1.Hash]v1.ImageIndex
+	layerMap  map[v1.Hash]v1.Layer
 }
 
 var _ v1.ImageIndex = (*index)(nil)
@@ -86,6 +88,7 @@ func (i *index) compute() error {
 
 	i.imageMap = make(map[v1.Hash]v1.Image)
 	i.indexMap = make(map[v1.Hash]v1.ImageIndex)
+	i.layerMap = make(map[v1.Hash]v1.Layer)
 
 	m, err := i.base.IndexManifest()
 	if err != nil {
@@ -115,6 +118,8 @@ func (i *index) compute() error {
 			i.indexMap[desc.Digest] = idx
 		} else if img, ok := add.Add.(v1.Image); ok {
 			i.imageMap[desc.Digest] = img
+		} else if l, ok := add.Add.(v1.Layer); ok {
+			i.layerMap[desc.Digest] = l
 		} else {
 			logs.Warn.Printf("Unexpected index addendum: %T", add.Add)
 		}
@@ -151,6 +156,21 @@ func (i *index) ImageIndex(h v1.Hash) (v1.ImageIndex, error) {
 	return i.base.ImageIndex(h)
 }
 
+type withLayer interface {
+	Layer(v1.Hash) (v1.Layer, error)
+}
+
+// Workaround for #819.
+func (i *index) Layer(h v1.Hash) (v1.Layer, error) {
+	if layer, ok := i.layerMap[h]; ok {
+		return layer, nil
+	}
+	if wl, ok := i.base.(withLayer); ok {
+		return wl.Layer(h)
+	}
+	return nil, fmt.Errorf("layer not found: %s", h)
+}
+
 // Digest returns the sha256 of this image's manifest.
 func (i *index) Digest() (v1.Hash, error) {
 	if err := i.compute(); err != nil {
diff --git a/pkg/v1/remote/index.go b/pkg/v1/remote/index.go
index 7a81075c..c1393435 100644
--- a/pkg/v1/remote/index.go
+++ b/pkg/v1/remote/index.go
@@ -122,6 +122,30 @@ func (r *remoteIndex) ImageIndex(h v1.Hash) (v1.ImageIndex, error) {
 	return desc.ImageIndex()
 }
 
+// Workaround for #819.
+func (r *remoteIndex) Layer(h v1.Hash) (v1.Layer, error) {
+	index, err := r.IndexManifest()
+	if err != nil {
+		return nil, err
+	}
+	for _, childDesc := range index.Manifests {
+		if h == childDesc.Digest {
+			l, err := partial.CompressedToLayer(&remoteLayer{
+				fetcher: r.fetcher,
+				digest:  h,
+			})
+			if err != nil {
+				return nil, err
+			}
+			return &MountableLayer{
+				Layer:     l,
+				Reference: r.Ref.Context().Digest(h.String()),
+			}, nil
+		}
+	}
+	return nil, fmt.Errorf("layer not found: %s", h)
+}
+
 func (r *remoteIndex) imageByPlatform(platform v1.Platform) (v1.Image, error) {
 	desc, err := r.childByPlatform(platform)
 	if err != nil {
diff --git a/pkg/v1/remote/multi_write.go b/pkg/v1/remote/multi_write.go
index 796b2248..6b81044f 100644
--- a/pkg/v1/remote/multi_write.go
+++ b/pkg/v1/remote/multi_write.go
@@ -196,45 +196,58 @@ func addIndexBlobs(idx v1.ImageIndex, blobs map[v1.Hash]v1.Layer, repo name.Repo
 			// Also track the sub-image manifest to upload later by digest.
 			newManifests[lvl][repo.Digest(desc.Digest.String())] = img
 		default:
-			return nil, fmt.Errorf("unknown media type: %v", desc.MediaType)
+			// Workaround for #819.
+			if wl, ok := idx.(withLayer); ok {
+				layer, err := wl.Layer(desc.Digest)
+				if err != nil {
+					return nil, err
+				}
+				if err := addLayerBlob(layer, blobs, allowNondistributableArtifacts); err != nil {
+					return nil, err
+				}
+			} else {
+				return nil, fmt.Errorf("unknown media type: %v", desc.MediaType)
+			}
 		}
 	}
 	return newManifests, nil
 }
 
-func addImageBlobs(img v1.Image, blobs map[v1.Hash]v1.Layer, allowNondistributableArtifacts bool) error {
-	ls, err := img.Layers()
+func addLayerBlob(l v1.Layer, blobs map[v1.Hash]v1.Layer, allowNondistributableArtifacts bool) error {
+	// Ignore foreign layers.
+	mt, err := l.MediaType()
 	if err != nil {
 		return err
 	}
-	// Collect all layers.
-	for _, l := range ls {
-		d, err := l.Digest()
-		if err != nil {
-			return err
-		}
 
-		// Ignore foreign layers.
-		mt, err := l.MediaType()
+	if mt.IsDistributable() || allowNondistributableArtifacts {
+		d, err := l.Digest()
 		if err != nil {
 			return err
 		}
-		if !mt.IsDistributable() && !allowNondistributableArtifacts {
-			continue
-		}
 
 		blobs[d] = l
 	}
 
-	// Collect config blob.
-	cl, err := partial.ConfigLayer(img)
+	return nil
+}
+
+func addImageBlobs(img v1.Image, blobs map[v1.Hash]v1.Layer, allowNondistributableArtifacts bool) error {
+	ls, err := img.Layers()
 	if err != nil {
 		return err
 	}
-	cld, err := cl.Digest()
+	// Collect all layers.
+	for _, l := range ls {
+		if err := addLayerBlob(l, blobs, allowNondistributableArtifacts); err != nil {
+			return err
+		}
+	}
+
+	// Collect config blob.
+	cl, err := partial.ConfigLayer(img)
 	if err != nil {
 		return err
 	}
-	blobs[cld] = cl
-	return nil
+	return addLayerBlob(cl, blobs, allowNondistributableArtifacts)
 }
diff --git a/pkg/v1/remote/write.go b/pkg/v1/remote/write.go
index 55fc1afe..261e9ec4 100644
--- a/pkg/v1/remote/write.go
+++ b/pkg/v1/remote/write.go
@@ -390,6 +390,10 @@ func (w *writer) uploadOne(l v1.Layer) error {
 	return retry.Retry(tryUpload, retry.IsTemporary, backoff)
 }
 
+type withLayer interface {
+	Layer(v1.Hash) (v1.Layer, error)
+}
+
 func (w *writer) writeIndex(ref name.Reference, ii v1.ImageIndex, options ...Option) error {
 	index, err := ii.IndexManifest()
 	if err != nil {
@@ -429,6 +433,17 @@ func (w *writer) writeIndex(ref name.Reference, ii v1.ImageIndex, options ...Opt
 			if err := Write(ref, img, options...); err != nil {
 				return err
 			}
+		default:
+			// Workaround for #819.
+			if wl, ok := ii.(withLayer); ok {
+				layer, err := wl.Layer(desc.Digest)
+				if err != nil {
+					return err
+				}
+				if err := w.uploadOne(layer); err != nil {
+					return err
+				}
+			}
 		}
 	}
 
diff --git a/pkg/v1/validate/index.go b/pkg/v1/validate/index.go
index 71648461..688e0f1d 100644
--- a/pkg/v1/validate/index.go
+++ b/pkg/v1/validate/index.go
@@ -44,6 +44,10 @@ func Index(idx v1.ImageIndex) error {
 	return nil
 }
 
+type withLayer interface {
+	Layer(v1.Hash) (v1.Layer, error)
+}
+
 func validateChildren(idx v1.ImageIndex) error {
 	manifest, err := idx.IndexManifest()
 	if err != nil {
@@ -76,7 +80,23 @@ func validateChildren(idx v1.ImageIndex) error {
 				errs = append(errs, fmt.Sprintf("failed to validate image MediaType[%d](%s): %v", i, desc.Digest, err))
 			}
 		default:
-			logs.Warn.Printf("Unexpected manifest: %s", desc.MediaType)
+			// Workaround for #819.
+			if wl, ok := idx.(withLayer); ok {
+				layer, err := wl.Layer(desc.Digest)
+				if err != nil {
+					return fmt.Errorf("failed to get layer Manifests[%d]: %v", i, err)
+				}
+				if err := Layer(layer); err != nil {
+					lerr := fmt.Sprintf("failed to validate layer Manifests[%d](%s): %v", i, desc.Digest, err)
+					if desc.MediaType.IsDistributable() {
+						errs = append(errs, lerr)
+					} else {
+						logs.Warn.Printf("nondistributable layer failure: %v", lerr)
+					}
+				}
+			} else {
+				logs.Warn.Printf("Unexpected manifest: %s", desc.MediaType)
+			}
 		}
 	}
 


Test patch:
diff --git a/pkg/registry/registry_test.go b/pkg/registry/registry_test.go
index e72eb98b..cebb0b11 100644
--- a/pkg/registry/registry_test.go
+++ b/pkg/registry/registry_test.go
@@ -29,6 +29,23 @@ import (
 	"github.com/google/go-containerregistry/pkg/registry"
 )
 
+const (
+	weirdIndex = `{
+  "manifests": [
+	  {
+			"digest":"sha256:2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae",
+			"mediaType":"application/vnd.oci.image.layer.nondistributable.v1.tar+gzip"
+		},{
+			"digest":"sha256:2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae",
+			"mediaType":"application/xml"
+		},{
+			"digest":"sha256:2c26b46b68ffc68ff99b453c1d30413413422d706483bfa0f98a5e886266e7ae",
+			"mediaType":"application/vnd.oci.image.manifest.v1+json"
+		}
+	]
+}`
+)
+
 func sha256String(s string) string {
 	h := sha256.Sum256([]byte(s))
 	return hex.EncodeToString(h[:])
@@ -254,6 +271,37 @@ func TestCalls(t *testing.T) {
 			Code:        http.StatusCreated,
 			Body:        "foo",
 		},
+		{
+			Description: "create index",
+			Method:      "PUT",
+			URL:         "/v2/foo/manifests/latest",
+			Code:        http.StatusCreated,
+			Body:        weirdIndex,
+			RequestHeader: map[string]string{
+				"Content-Type": "application/vnd.oci.image.index.v1+json",
+			},
+			Manifests: map[string]string{"foo/manifests/image": "foo"},
+		},
+		{
+			Description: "create index missing child",
+			Method:      "PUT",
+			URL:         "/v2/foo/manifests/latest",
+			Code:        http.StatusNotFound,
+			Body:        weirdIndex,
+			RequestHeader: map[string]string{
+				"Content-Type": "application/vnd.oci.image.index.v1+json",
+			},
+		},
+		{
+			Description: "bad index body",
+			Method:      "PUT",
+			URL:         "/v2/foo/manifests/latest",
+			Code:        http.StatusBadRequest,
+			Body:        "foo",
+			RequestHeader: map[string]string{
+				"Content-Type": "application/vnd.oci.image.index.v1+json",
+			},
+		},
 		{
 			Description: "bad manifest method",
 			Method:      "BAR",
diff --git a/pkg/v1/layout/testdata/test_index/index.json b/pkg/v1/layout/testdata/test_index/index.json
index 8274d574..28df736a 100644
--- a/pkg/v1/layout/testdata/test_index/index.json
+++ b/pkg/v1/layout/testdata/test_index/index.json
@@ -10,7 +10,7 @@
       }
     },
     {
-      "mediaType": "application/vnd.oci.image.manifest.v1+json",
+      "mediaType": "application/vnd.oci.descriptor.v1+json",
       "size": 423,
       "digest": "sha256:32589985702551b6c56033bb3334432a0a513bf9d6aceda0f67c42b003850720",
       "annotations": {
diff --git a/pkg/v1/layout/write_test.go b/pkg/v1/layout/write_test.go
index 57eb9b29..6b1dfac6 100644
--- a/pkg/v1/layout/write_test.go
+++ b/pkg/v1/layout/write_test.go
@@ -6,7 +6,6 @@ import (
 	"io/ioutil"
 	"log"
 	"os"
-	"strconv"
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
@@ -98,7 +97,7 @@ func TestAppendDescriptorInitializesIndex(t *testing.T) {
 	}
 }
 
-func TestAppendArtifacts(t *testing.T) {
+func TestRoundtrip(t *testing.T) {
 	tmp, err := ioutil.TempDir("", "write-index-test")
 	if err != nil {
 		t.Fatal(err)
@@ -116,36 +115,10 @@ func TestAppendArtifacts(t *testing.T) {
 		t.Fatal(err)
 	}
 
-	// Let's reconstruct the original.
-	temp, err := Write(tmp, empty.Index)
-	if err != nil {
+	// Write it back.
+	if _, err := Write(tmp, original); err != nil {
 		t.Fatal(err)
 	}
-	for i, desc := range originalManifest.Manifests {
-		// Each descriptor is annotated with its position.
-		annotations := map[string]string{
-			"org.opencontainers.image.ref.name": strconv.Itoa(i + 1),
-		}
-		switch desc.MediaType {
-		case types.OCIImageIndex, types.DockerManifestList:
-			ii, err := original.ImageIndex(desc.Digest)
-			if err != nil {
-				t.Fatal(err)
-			}
-			if err := temp.AppendIndex(ii, WithAnnotations(annotations)); err != nil {
-				t.Fatal(err)
-			}
-		case types.OCIManifestSchema1, types.DockerManifestSchema2:
-			img, err := original.Image(desc.Digest)
-			if err != nil {
-				t.Fatal(err)
-			}
-			if err := temp.AppendImage(img, WithAnnotations(annotations)); err != nil {
-				t.Fatal(err)
-			}
-		}
-	}
-
 	reconstructed, err := ImageIndexFromPath(tmp)
 	if err != nil {
 		t.Fatalf("ImageIndexFromPath() = %v", err)
diff --git a/pkg/v1/mutate/index_test.go b/pkg/v1/mutate/index_test.go
index 65930e74..ab9d80c9 100644
--- a/pkg/v1/mutate/index_test.go
+++ b/pkg/v1/mutate/index_test.go
@@ -57,9 +57,14 @@ func TestAppendIndex(t *testing.T) {
 		},
 	}, mutate.IndexAddendum{
 		Add: img,
+		Descriptor: v1.Descriptor{
+			URLs: []string{"image.example.com"},
+		},
+	}, mutate.IndexAddendum{
+		Add: l,
 		Descriptor: v1.Descriptor{
 			MediaType: types.MediaType("application/xml"),
-			URLs:      []string{"image.example.com"},
+			URLs:      []string{"blob.example.com"},
 		},
 	}, mutate.IndexAddendum{
 		Add: l,
@@ -100,18 +105,19 @@ func TestAppendIndex(t *testing.T) {
 	for i, want := range map[int]string{
 		3: "index.example.com",
 		4: "image.example.com",
-		5: "layer.example.com",
+		5: "blob.example.com",
+		6: "layer.example.com",
 	} {
 		if got := m.Manifests[i].URLs[0]; got != want {
 			t.Errorf("wrong URLs[0] for Manifests[%d]: %s != %s", i, got, want)
 		}
 	}
 
-	if got, want := m.Manifests[4].MediaType, types.MediaType("application/xml"); got != want {
+	if got, want := m.Manifests[5].MediaType, types.MediaType("application/xml"); got != want {
 		t.Errorf("wrong MediaType for layer: %s != %s", got, want)
 	}
 
-	if got, want := m.Manifests[5].MediaType, types.OCIUncompressedRestrictedLayer; got != want {
+	if got, want := m.Manifests[6].MediaType, types.OCIUncompressedRestrictedLayer; got != want {
 		t.Errorf("wrong MediaType for layer: %s != %s", got, want)
 	}
 
diff --git a/pkg/v1/remote/multi_write_test.go b/pkg/v1/remote/multi_write_test.go
index 6abe8b68..468b8047 100644
--- a/pkg/v1/remote/multi_write_test.go
+++ b/pkg/v1/remote/multi_write_test.go
@@ -65,6 +65,7 @@ func TestMultiWrite(t *testing.T) {
 		mutate.IndexAddendum{Add: img1},
 		mutate.IndexAddendum{Add: img2},
 		mutate.IndexAddendum{Add: subidx},
+		mutate.IndexAddendum{Add: rl},
 	)
 
 	// Set up a fake registry.
diff --git a/pkg/v1/remote/write_test.go b/pkg/v1/remote/write_test.go
index caf0ec1e..66746e0d 100644
--- a/pkg/v1/remote/write_test.go
+++ b/pkg/v1/remote/write_test.go
@@ -1473,6 +1473,15 @@ func TestNestedIndex(t *testing.T) {
 		},
 	})
 
+	l, err := random.Layer(100, types.DockerLayer)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	parent = mutate.AppendManifests(parent, mutate.IndexAddendum{
+		Add: l,
+	})
+
 	if err := WriteIndex(srcRef, parent); err != nil {
 		t.Fatal(err)
 	}