Repository: google/go-containerregistry (go)

Issue:
crane rebase and mutate.Rebase fail with foreign layer in new_base image
It appears `crane rebase` fails with an image whose base layers are foreign layers. Below is hopefully a simple example using Docker Hub `hello-world:nanoserver-1809` and its `nanoserver-1809` base image (whose registry happens to be `mcr.microsoft.com`) as both new/old base.

Failure reproduction:
```
## Image-ref for `hello-world:nanoserver-1809`
app_image=index.docker.io/library/hello-world@sha256:63c287625c2b0b72900e562de73c0e381472a83b1b39217aef3856cd398eca0b

## Image-ref for `mcr.microsoft.com/windows/nanoserver:1809` (base image for above)
app_base=mcr.microsoft.com/windows/nanoserver@sha256:06281772b6a561411d4b338820d94ab1028fdeb076c85350bbc01e80c4bfa2b4

## Confirm app image layers 
crane manifest $app_image | jq ".layers[] | .digest"
## Output:
# "sha256:a35da61c356213336e646756218539950461ff2bf096badf307a23add6e70053" # <- same base layer
# "sha256:be21f08f670160cbae227e3053205b91d6bfa3de750b90c7e00bd2c511ccb63a" # COPY layer
# "sha256:10d1439be4eb8819987ec2e9c140d44d74d6b42a823d57fe1953bd99948e1bc0" # ENTRYPOINT layer

## Confirm base image layers
crane manifest $app_base | jq ".layers[] | .digest"
## Output:
# "sha256:a35da61c356213336e646756218539950461ff2bf096badf307a23add6e70053" # <- same base layer

crane rebase \
--original $app_image \
--old_base $app_base \
--new_base $app_base \
--rebased localhost:5000/rebased  # <- needs a writable registry
## Output
# 2020/03/03 16:17:35 existing blob: sha256:10d1439be4eb8819987ec2e9c140d44d74d6b42a823d57fe1953bd99948e1bc0
# 2020/03/03 16:17:35 existing blob: sha256:be21f08f670160cbae227e3053205b91d6bfa3de750b90c7e00bd2c511ccb63a
# 2020/03/03 16:17:35 existing blob: sha256:e488c333e5cb990d61a1be8d4446050befe3d7a8ffc69220eb030694d70e7beb
# 2020/03/03 16:17:35 pushing localhost:5000/rebased: PUT http://localhost:5000/v2/rebased/manifests/latest: multiple errors returned: UNKNOWN: unknown error; UNKNOWN: unknown error; map[]; MANIFEST_BLOB_UNKNOWN: blob unknown to registry; sha256:a35da61c356213336e646756218539950461ff2bf096badf307a23add6e70053 # <- failed on foreign base layer
```

Readable error:
> `2020/03/03 16:17:35 pushing localhost:5000/rebased: PUT http://localhost:5000/v2/rebased/manifests/latest: multiple errors returned: UNKNOWN: unknown error; UNKNOWN: unknown error; map[]; MANIFEST_BLOB_UNKNOWN: blob unknown to registry; sha256:a35da61c356213336e646756218539950461ff2bf096badf307a23add6e70053`

## Additional info
I don't think the separate registries are at fault since I reproduced this all on a local registry (with differing bases and many more steps and output) in this [gist](https://gist.github.com/micahyoung/c515a3c45b6a09773be08530b0ca1bed).

I chose the issue example since it is pretty copy-pastable and doesn't require a Docker Desktop WCOW daemon to reproduce, but the Docker file `hello-world:nanoserver-1809` is built from is [here](https://github.com/docker-library/hello-world/blob/9c93e37114a7fe99b5fc0d776e0b8dff99cbbb75/amd64/hello-world/nanoserver-1809/Dockerfile). 

I couldn't find any other non-Windows foreign layer images in the wild. But if needed, there is a Windows nanoserver image with foreign layers on gcr at`gcr.io/kubernetes-e2e-test-images/windows-nanoserver:v1` [origin here](https://github.com/kubernetes/kubernetes/issues/74134#issuecomment-467666811). 

Pull request:
Preserve Descriptor in remote where possible
Fixes #683 

If we resolve an index to an image, we can potentially lose a lot of
information, e.g. annotations or anything in the platform that's not in
the config, which only has os/arch.

If we return a layer from an image, we can look up the original
descriptor from that image to return as well, which might include
annotations or urls. Previously, the Mountable wrapping made this not
work.

Patch:
diff --git a/pkg/legacy/tarball/write.go b/pkg/legacy/tarball/write.go
index 710ff067..ed416d7b 100644
--- a/pkg/legacy/tarball/write.go
+++ b/pkg/legacy/tarball/write.go
@@ -158,7 +158,7 @@ func updateLayerSources(layerSources map[v1.Hash]v1.Descriptor, layer v1.Layer,
 		if err != nil {
 			return err
 		}
-		layerSources[diffid] = desc
+		layerSources[diffid] = *desc
 	}
 	return nil
 }
diff --git a/pkg/v1/partial/with.go b/pkg/v1/partial/with.go
index 08bdce3e..b24f31c7 100644
--- a/pkg/v1/partial/with.go
+++ b/pkg/v1/partial/with.go
@@ -201,22 +201,22 @@ func BlobSize(i WithManifest, h v1.Hash) (int64, error) {
 }
 
 // BlobDescriptor is a helper for implementing v1.Image
-func BlobDescriptor(i WithManifest, h v1.Hash) (v1.Descriptor, error) {
+func BlobDescriptor(i WithManifest, h v1.Hash) (*v1.Descriptor, error) {
 	m, err := i.Manifest()
 	if err != nil {
-		return v1.Descriptor{}, err
+		return nil, err
 	}
 
 	if m.Config.Digest == h {
-		return m.Config, nil
+		return &m.Config, nil
 	}
 
 	for _, l := range m.Layers {
 		if l.Digest == h {
-			return l, nil
+			return &l, nil
 		}
 	}
-	return v1.Descriptor{}, fmt.Errorf("blob %v not found", h)
+	return nil, fmt.Errorf("blob %v not found", h)
 }
 
 // WithManifestAndConfigFile defines the subset of v1.Image used by these helper methods
diff --git a/pkg/v1/remote/descriptor.go b/pkg/v1/remote/descriptor.go
index 36ee6778..19ef5dde 100644
--- a/pkg/v1/remote/descriptor.go
+++ b/pkg/v1/remote/descriptor.go
@@ -77,14 +77,12 @@ func (d *Descriptor) RawManifest() ([]byte, error) {
 // querying what kind of artifact a reference represents.
 func Get(ref name.Reference, options ...Option) (*Descriptor, error) {
 	acceptable := []types.MediaType{
-		types.DockerManifestSchema2,
-		types.OCIManifestSchema1,
-		types.DockerManifestList,
-		types.OCIImageIndex,
 		// Just to look at them.
 		types.DockerManifestSchema1,
 		types.DockerManifestSchema1Signed,
 	}
+	acceptable = append(acceptable, acceptableImageMediaTypes...)
+	acceptable = append(acceptable, acceptableIndexMediaTypes...)
 	return get(ref, acceptable, options...)
 }
 
@@ -174,8 +172,9 @@ func (d *Descriptor) remoteImage() *remoteImage {
 			Ref:    d.Ref,
 			Client: d.Client,
 		},
-		manifest:  d.Manifest,
-		mediaType: d.MediaType,
+		manifest:   d.Manifest,
+		mediaType:  d.MediaType,
+		descriptor: &d.Descriptor,
 	}
 }
 
@@ -185,8 +184,9 @@ func (d *Descriptor) remoteIndex() *remoteIndex {
 			Ref:    d.Ref,
 			Client: d.Client,
 		},
-		manifest:  d.Manifest,
-		mediaType: d.MediaType,
+		manifest:   d.Manifest,
+		mediaType:  d.MediaType,
+		descriptor: &d.Descriptor,
 	}
 }
 
diff --git a/pkg/v1/remote/image.go b/pkg/v1/remote/image.go
index 947f8bca..8506d4b0 100644
--- a/pkg/v1/remote/image.go
+++ b/pkg/v1/remote/image.go
@@ -29,6 +29,11 @@ import (
 	"github.com/google/go-containerregistry/pkg/v1/v1util"
 )
 
+var acceptableImageMediaTypes = []types.MediaType{
+	types.DockerManifestSchema2,
+	types.OCIManifestSchema1,
+}
+
 // remoteImage accesses an image from a remote registry
 type remoteImage struct {
 	fetcher
@@ -37,6 +42,7 @@ type remoteImage struct {
 	configLock   sync.Mutex // Protects config
 	config       []byte
 	mediaType    types.MediaType
+	descriptor   *v1.Descriptor
 }
 
 var _ partial.CompressedImageCore = (*remoteImage)(nil)
@@ -68,15 +74,14 @@ func (r *remoteImage) RawManifest() ([]byte, error) {
 	// NOTE(jonjohnsonjr): We should never get here because the public entrypoints
 	// do type-checking via remote.Descriptor. I've left this here for tests that
 	// directly instantiate a remoteImage.
-	acceptable := []types.MediaType{
-		types.DockerManifestSchema2,
-		types.OCIManifestSchema1,
-	}
-	manifest, desc, err := r.fetchManifest(r.Ref, acceptable)
+	manifest, desc, err := r.fetchManifest(r.Ref, acceptableImageMediaTypes)
 	if err != nil {
 		return nil, err
 	}
 
+	if r.descriptor == nil {
+		r.descriptor = desc
+	}
 	r.mediaType = desc.MediaType
 	r.manifest = manifest
 	return r.manifest, nil
@@ -107,6 +112,15 @@ func (r *remoteImage) RawConfigFile() ([]byte, error) {
 	return r.config, nil
 }
 
+// Descriptor retains the original descriptor from an index manifest.
+// See partial.Descriptor.
+func (r *remoteImage) Descriptor() (*v1.Descriptor, error) {
+	// kind of a hack, but RawManifest does appropriate locking/memoization
+	// and makes sure r.descriptor is populated.
+	_, err := r.RawManifest()
+	return r.descriptor, err
+}
+
 // remoteImageLayer implements partial.CompressedLayer
 type remoteImageLayer struct {
 	ri     *remoteImage
@@ -192,6 +206,12 @@ func (rl *remoteImageLayer) DiffID() (v1.Hash, error) {
 	return partial.BlobToDiffID(rl, rl.digest)
 }
 
+// Descriptor retains the original descriptor from an image manifest.
+// See partial.Descriptor.
+func (rl *remoteImageLayer) Descriptor() (*v1.Descriptor, error) {
+	return partial.BlobDescriptor(rl, rl.digest)
+}
+
 // LayerByDigest implements partial.CompressedLayer
 func (r *remoteImage) LayerByDigest(h v1.Hash) (partial.CompressedLayer, error) {
 	return &remoteImageLayer{
diff --git a/pkg/v1/remote/index.go b/pkg/v1/remote/index.go
index c739d112..1e4b48a9 100644
--- a/pkg/v1/remote/index.go
+++ b/pkg/v1/remote/index.go
@@ -25,22 +25,23 @@ import (
 	"github.com/google/go-containerregistry/pkg/v1/types"
 )
 
+var acceptableIndexMediaTypes = []types.MediaType{
+	types.DockerManifestList,
+	types.OCIImageIndex,
+}
+
 // remoteIndex accesses an index from a remote registry
 type remoteIndex struct {
 	fetcher
 	manifestLock sync.Mutex // Protects manifest
 	manifest     []byte
 	mediaType    types.MediaType
+	descriptor   *v1.Descriptor
 }
 
 // Index provides access to a remote index reference.
 func Index(ref name.Reference, options ...Option) (v1.ImageIndex, error) {
-	acceptable := []types.MediaType{
-		types.DockerManifestList,
-		types.OCIImageIndex,
-	}
-
-	desc, err := get(ref, acceptable, options...)
+	desc, err := get(ref, acceptableIndexMediaTypes, options...)
 	if err != nil {
 		return nil, err
 	}
@@ -73,15 +74,14 @@ func (r *remoteIndex) RawManifest() ([]byte, error) {
 	// NOTE(jonjohnsonjr): We should never get here because the public entrypoints
 	// do type-checking via remote.Descriptor. I've left this here for tests that
 	// directly instantiate a remoteIndex.
-	acceptable := []types.MediaType{
-		types.DockerManifestList,
-		types.OCIImageIndex,
-	}
-	manifest, desc, err := r.fetchManifest(r.Ref, acceptable)
+	manifest, desc, err := r.fetchManifest(r.Ref, acceptableIndexMediaTypes)
 	if err != nil {
 		return nil, err
 	}
 
+	if r.descriptor == nil {
+		r.descriptor = desc
+	}
 	r.mediaType = desc.MediaType
 	r.manifest = manifest
 	return r.manifest, nil
@@ -105,6 +105,15 @@ func (r *remoteIndex) Image(h v1.Hash) (v1.Image, error) {
 	return desc.Image()
 }
 
+// Descriptor retains the original descriptor from an index manifest.
+// See partial.Descriptor.
+func (r *remoteIndex) Descriptor() (*v1.Descriptor, error) {
+	// kind of a hack, but RawManifest does appropriate locking/memoization
+	// and makes sure r.descriptor is populated.
+	_, err := r.RawManifest()
+	return r.descriptor, err
+}
+
 func (r *remoteIndex) ImageIndex(h v1.Hash) (v1.ImageIndex, error) {
 	desc, err := r.childByHash(h)
 	if err != nil {
@@ -165,7 +174,7 @@ func (r *remoteIndex) childByHash(h v1.Hash) (*Descriptor, error) {
 // Convert one of this index's child's v1.Descriptor into a remote.Descriptor, with the given platform option.
 func (r *remoteIndex) childDescriptor(child v1.Descriptor, platform v1.Platform) (*Descriptor, error) {
 	ref := r.Ref.Context().Digest(child.Digest.String())
-	manifest, desc, err := r.fetchManifest(ref, []types.MediaType{child.MediaType})
+	manifest, _, err := r.fetchManifest(ref, []types.MediaType{child.MediaType})
 	if err != nil {
 		return nil, err
 	}
@@ -175,7 +184,7 @@ func (r *remoteIndex) childDescriptor(child v1.Descriptor, platform v1.Platform)
 			Client: r.Client,
 		},
 		Manifest:   manifest,
-		Descriptor: *desc,
+		Descriptor: child,
 		platform:   platform,
 	}, nil
 }
diff --git a/pkg/v1/remote/mount.go b/pkg/v1/remote/mount.go
index 3afda2a3..2800b330 100644
--- a/pkg/v1/remote/mount.go
+++ b/pkg/v1/remote/mount.go
@@ -17,6 +17,7 @@ package remote
 import (
 	"github.com/google/go-containerregistry/pkg/name"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
+	"github.com/google/go-containerregistry/pkg/v1/partial"
 )
 
 // MountableLayer wraps a v1.Layer in a shim that enables the layer to be
@@ -27,6 +28,12 @@ type MountableLayer struct {
 	Reference name.Reference
 }
 
+// Descriptor retains the original descriptor from an image manifest.
+// See partial.Descriptor.
+func (ml *MountableLayer) Descriptor() (*v1.Descriptor, error) {
+	return partial.Descriptor(ml.Layer)
+}
+
 // mountableImage wraps the v1.Layer references returned by the embedded v1.Image
 // in MountableLayer's so that remote.Write might attempt to mount them from their
 // source repository.
@@ -75,3 +82,9 @@ func (mi *mountableImage) LayerByDiffID(d v1.Hash) (v1.Layer, error) {
 		Reference: mi.Reference,
 	}, nil
 }
+
+// Descriptor retains the original descriptor from an index manifest.
+// See partial.Descriptor.
+func (mi *mountableImage) Descriptor() (*v1.Descriptor, error) {
+	return partial.Descriptor(mi.Image)
+}
diff --git a/pkg/v1/tarball/write.go b/pkg/v1/tarball/write.go
index 054d52b1..21b4a0a4 100644
--- a/pkg/v1/tarball/write.go
+++ b/pkg/v1/tarball/write.go
@@ -146,7 +146,7 @@ func MultiRefWrite(refToImage map[name.Reference]v1.Image, w io.Writer) error {
 				if err != nil {
 					return err
 				}
-				layerSources[diffid] = desc
+				layerSources[diffid] = *desc
 			}
 
 			r, err := l.Compressed()


Test patch:
diff --git a/pkg/v1/remote/image_test.go b/pkg/v1/remote/image_test.go
index 0095d50a..344be4ca 100644
--- a/pkg/v1/remote/image_test.go
+++ b/pkg/v1/remote/image_test.go
@@ -26,12 +26,14 @@ import (
 	"strings"
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-containerregistry/pkg/authn"
 	"github.com/google/go-containerregistry/pkg/logs"
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/registry"
 	v1 "github.com/google/go-containerregistry/pkg/v1"
 	"github.com/google/go-containerregistry/pkg/v1/mutate"
+	"github.com/google/go-containerregistry/pkg/v1/partial"
 	"github.com/google/go-containerregistry/pkg/v1/random"
 	"github.com/google/go-containerregistry/pkg/v1/types"
 	"github.com/google/go-containerregistry/pkg/v1/validate"
@@ -377,18 +379,23 @@ func TestPullingManifestList(t *testing.T) {
 	childDigest := mustIndexManifest(t, idx).Manifests[1].Digest
 	child := mustChild(t, idx, childDigest)
 	childPath := fmt.Sprintf("/v2/%s/manifests/%s", expectedRepo, childDigest)
+	fakePlatformChildDigest := mustIndexManifest(t, idx).Manifests[0].Digest
+	fakePlatformChild := mustChild(t, idx, fakePlatformChildDigest)
+	fakePlatformChildPath := fmt.Sprintf("/v2/%s/manifests/%s", expectedRepo, fakePlatformChildDigest)
 	configPath := fmt.Sprintf("/v2/%s/blobs/%s", expectedRepo, mustConfigName(t, child))
 
+	fakePlatform := v1.Platform{
+		Architecture: "not-real-arch",
+		OS:           "not-real-os",
+	}
+
 	// Rewrite the index to make sure the desired platform matches the second child.
 	manifest, err := idx.IndexManifest()
 	if err != nil {
 		t.Fatal(err)
 	}
 	// Make sure the first manifest doesn't match.
-	manifest.Manifests[0].Platform = &v1.Platform{
-		Architecture: "not-real-arch",
-		OS:           "not-real-os",
-	}
+	manifest.Manifests[0].Platform = &fakePlatform
 	// Make sure the second manifest does.
 	manifest.Manifests[1].Platform = &defaultPlatform
 	rawManifest, err := json.Marshal(manifest)
@@ -416,6 +423,11 @@ func TestPullingManifestList(t *testing.T) {
 				t.Errorf("Method; got %v, want %v", r.Method, http.MethodGet)
 			}
 			w.Write(mustRawConfigFile(t, child))
+		case fakePlatformChildPath:
+			if r.Method != http.MethodGet {
+				t.Errorf("Method; got %v, want %v", r.Method, http.MethodGet)
+			}
+			w.Write(mustRawManifest(t, fakePlatformChild))
 		default:
 			t.Fatalf("Unexpected path: %v", r.URL.Path)
 		}
@@ -439,6 +451,20 @@ func TestPullingManifestList(t *testing.T) {
 	if got, want := mustRawConfigFile(t, rmtChild), mustRawConfigFile(t, child); !bytes.Equal(got, want) {
 		t.Errorf("RawConfigFile() = %v, want %v", got, want)
 	}
+
+	// Make sure we can roundtrip platform info via Descriptor.
+	img, err := Image(tag, WithPlatform(fakePlatform))
+	if err != nil {
+		t.Fatal(err)
+	}
+	desc, err := partial.Descriptor(img)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if diff := cmp.Diff(*desc.Platform, fakePlatform); diff != "" {
+		t.Errorf("Desciptor() (-want +got) = %v", diff)
+	}
 }
 
 func TestPullingManifestListNoMatch(t *testing.T) {
diff --git a/pkg/v1/remote/layer_test.go b/pkg/v1/remote/layer_test.go
index 70c5ef75..afde497c 100644
--- a/pkg/v1/remote/layer_test.go
+++ b/pkg/v1/remote/layer_test.go
@@ -23,6 +23,9 @@ import (
 	"github.com/google/go-containerregistry/pkg/internal/compare"
 	"github.com/google/go-containerregistry/pkg/name"
 	"github.com/google/go-containerregistry/pkg/registry"
+	"github.com/google/go-containerregistry/pkg/v1/empty"
+	"github.com/google/go-containerregistry/pkg/v1/mutate"
+	"github.com/google/go-containerregistry/pkg/v1/partial"
 	"github.com/google/go-containerregistry/pkg/v1/random"
 	"github.com/google/go-containerregistry/pkg/v1/types"
 	"github.com/google/go-containerregistry/pkg/v1/validate"
@@ -76,3 +79,59 @@ func TestRemoteLayer(t *testing.T) {
 		t.Errorf("validate.Layer: %v", err)
 	}
 }
+
+func TestRemoteLayerDescriptor(t *testing.T) {
+	layer, err := random.Layer(1024, types.DockerLayer)
+	if err != nil {
+		t.Fatal(err)
+	}
+	image, err := mutate.Append(empty.Image, mutate.Addendum{
+		Layer: layer,
+		URLs:  []string{"example.com"},
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// Set up a fake registry and write what we pulled to it.
+	// This ensures we get coverage for the remoteLayer.MediaType path.
+	s := httptest.NewServer(registry.New())
+	defer s.Close()
+	u, err := url.Parse(s.URL)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	dst := fmt.Sprintf("%s/some/path:tag", u.Host)
+	ref, err := name.ParseReference(dst)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if err := Write(ref, image); err != nil {
+		t.Fatalf("failed to WriteLayer: %v", err)
+	}
+
+	pulled, err := Image(ref)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	layers, err := pulled.Layers()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	desc, err := partial.Descriptor(layers[0])
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(desc.URLs) != 1 {
+		t.Fatalf("expected url for layer[0]")
+	}
+
+	if got, want := desc.URLs[0], "example.com"; got != want {
+		t.Errorf("layer[0].urls[0] = %s != %s", got, want)
+	}
+}
diff --git a/pkg/v1/remote/write_test.go b/pkg/v1/remote/write_test.go
index 40b6cfef..bab1708b 100644
--- a/pkg/v1/remote/write_test.go
+++ b/pkg/v1/remote/write_test.go
@@ -1359,6 +1359,9 @@ func TestNestedIndex(t *testing.T) {
 	}
 	parent := mutate.AppendManifests(empty.Index, mutate.IndexAddendum{
 		Add: child,
+		Descriptor: v1.Descriptor{
+			URLs: []string{"example.com/url"},
+		},
 	})
 
 	if err := WriteIndex(srcRef, parent); err != nil {
@@ -1372,6 +1375,29 @@ func TestNestedIndex(t *testing.T) {
 	if err := validate.Index(pulled); err != nil {
 		t.Fatalf("validate.Index: %v", err)
 	}
+
+	digest, err := child.Digest()
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	pulledChild, err := pulled.ImageIndex(digest)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	desc, err := partial.Descriptor(pulledChild)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	if len(desc.URLs) != 1 {
+		t.Fatalf("expected url for pulledChild")
+	}
+
+	if want, got := "example.com/url", desc.URLs[0]; want != got {
+		t.Errorf("pulledChild.urls[0] = %s != %s", got, want)
+	}
 }
 
 func BenchmarkWrite(b *testing.B) {