#305 cleanup 2: Reference handling

copy/copy.go

@@ -347,6 +347,9 @@ func checkImageDestinationForCurrentRuntimeOS(ctx context.Context, sys *types.Sy
 
 // updateEmbeddedDockerReference handles the Docker reference embedded in Docker schema1 manifests.
 func (ic *imageCopier) updateEmbeddedDockerReference() error {
+	if ic.c.dest.IgnoresEmbeddedDockerReference() {
+		return nil // Destination would prefer us not to update the embedded reference.
+	}
 	destRef := ic.c.dest.Reference().DockerReference()
 	if destRef == nil {
 		return nil // Destination does not care about Docker references

directory/directory_dest.go

@@ -117,6 +117,13 @@ func (d *dirImageDestination) MustMatchRuntimeOS() bool {
 	return false
 }
 
+// IgnoresEmbeddedDockerReference returns true iff the destination does not care about Image.EmbeddedDockerReferenceConflicts(),
+// and would prefer to receive an unmodified manifest instead of one modified for the destination.
+// Does not make a difference if Reference().DockerReference() is nil.
+func (d *dirImageDestination) IgnoresEmbeddedDockerReference() bool {
+	return false // N/A, DockerReference() returns nil.
+}
+
 // PutBlob writes contents of stream and returns data representing the result (with all data filled in).
 // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it.
 // inputInfo.Size is the expected length of stream, if known.

docker/docker_image_dest.go

@@ -95,6 +95,13 @@ func (d *dockerImageDestination) MustMatchRuntimeOS() bool {
 	return false
 }
 
+// IgnoresEmbeddedDockerReference returns true iff the destination does not care about Image.EmbeddedDockerReferenceConflicts(),
+// and would prefer to receive an unmodified manifest instead of one modified for the destination.
+// Does not make a difference if Reference().DockerReference() is nil.
+func (d *dockerImageDestination) IgnoresEmbeddedDockerReference() bool {
+	return false // We do want the manifest updated; older registry versions refuse manifests if the embedded reference does not match.
+}
+
 // sizeCounter is an io.Writer which only counts the total size of its input.
 type sizeCounter struct{ size int64 }
 

docker/tarfile/dest.go

@@ -75,6 +75,13 @@ func (d *Destination) MustMatchRuntimeOS() bool {
 	return false
 }
 
+// IgnoresEmbeddedDockerReference returns true iff the destination does not care about Image.EmbeddedDockerReferenceConflicts(),
+// and would prefer to receive an unmodified manifest instead of one modified for the destination.
+// Does not make a difference if Reference().DockerReference() is nil.
+func (d *Destination) IgnoresEmbeddedDockerReference() bool {
+	return false // N/A, we only accept schema2 images where EmbeddedDockerReferenceConflicts() is always false.
+}
+
 // PutBlob writes contents of stream and returns data representing the result (with all data filled in).
 // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it.
 // inputInfo.Size is the expected length of stream, if known.

image/docker_schema2_test.go

@@ -386,6 +386,9 @@ func (d *memoryImageDest) AcceptsForeignLayerURLs() bool {
 func (d *memoryImageDest) MustMatchRuntimeOS() bool {
 	panic("Unexpected call to a mock function")
 }
+func (d *memoryImageDest) IgnoresEmbeddedDockerReference() bool {
+	panic("Unexpected call to a mock function")
+}
 func (d *memoryImageDest) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, isConfig bool) (types.BlobInfo, error) {
 	if d.storedBlobs == nil {
 		d.storedBlobs = make(map[digest.Digest][]byte)

oci/archive/oci_dest.go

@@ -70,6 +70,13 @@ func (d *ociArchiveImageDestination) MustMatchRuntimeOS() bool {
 	return d.unpackedDest.MustMatchRuntimeOS()
 }
 
+// IgnoresEmbeddedDockerReference returns true iff the destination does not care about Image.EmbeddedDockerReferenceConflicts(),
+// and would prefer to receive an unmodified manifest instead of one modified for the destination.
+// Does not make a difference if Reference().DockerReference() is nil.
+func (d *ociArchiveImageDestination) IgnoresEmbeddedDockerReference() bool {
+	return d.unpackedDest.IgnoresEmbeddedDockerReference()
+}
+
 // PutBlob writes contents of stream and returns data representing the result (with all data filled in).
 // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it.
 // inputInfo.Size is the expected length of stream, if known.

oci/layout/oci_dest.go

@@ -95,6 +95,13 @@ func (d *ociImageDestination) MustMatchRuntimeOS() bool {
 	return false
 }
 
+// IgnoresEmbeddedDockerReference returns true iff the destination does not care about Image.EmbeddedDockerReferenceConflicts(),
+// and would prefer to receive an unmodified manifest instead of one modified for the destination.
+// Does not make a difference if Reference().DockerReference() is nil.
+func (d *ociImageDestination) IgnoresEmbeddedDockerReference() bool {
+	return false // N/A, DockerReference() returns nil.
+}
+
 // PutBlob writes contents of stream and returns data representing the result (with all data filled in).
 // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it.
 // inputInfo.Size is the expected length of stream, if known.

openshift/openshift.go

@@ -369,6 +369,13 @@ func (d *openshiftImageDestination) MustMatchRuntimeOS() bool {
 	return false
 }
 
+// IgnoresEmbeddedDockerReference returns true iff the destination does not care about Image.EmbeddedDockerReferenceConflicts(),
+// and would prefer to receive an unmodified manifest instead of one modified for the destination.
+// Does not make a difference if Reference().DockerReference() is nil.
+func (d *openshiftImageDestination) IgnoresEmbeddedDockerReference() bool {
+	return d.docker.IgnoresEmbeddedDockerReference()
+}
+
 // PutBlob writes contents of stream and returns data representing the result (with all data filled in).
 // inputInfo.Digest can be optionally provided if known; it is not mandatory for the implementation to verify it.
 // inputInfo.Size is the expected length of stream, if known.

ostree/ostree_dest.go

@@ -125,6 +125,13 @@ func (d *ostreeImageDestination) MustMatchRuntimeOS() bool {
 	return true
 }
 
+// IgnoresEmbeddedDockerReference returns true iff the destination does not care about Image.EmbeddedDockerReferenceConflicts(),
+// and would prefer to receive an unmodified manifest instead of one modified for the destination.
+// Does not make a difference if Reference().DockerReference() is nil.
+func (d *ostreeImageDestination) IgnoresEmbeddedDockerReference() bool {
+	return false // N/A, DockerReference() returns nil.
+}
+
 func (d *ostreeImageDestination) PutBlob(ctx context.Context, stream io.Reader, inputInfo types.BlobInfo, isConfig bool) (types.BlobInfo, error) {
 	tmpDir, err := ioutil.TempDir(d.tmpDirPath, "blob")
 	if err != nil {

storage/storage_image.go

@@ -50,8 +50,7 @@ type storageImageSource struct {
 }
 
 type storageImageDestination struct {
-	imageRef       storageReference                // The reference we'll use to name the image
-	publicRef      storageReference                // The reference we return when asked about the name we'll give to the image
+	imageRef       storageReference
 	directory      string                          // Temporary directory where we store blobs until Commit() time
 	nextTempFileID int32                           // A counter that we use for computing filenames to assign to blobs
 	manifest       []byte                          // Manifest contents, temporary
@@ -243,15 +242,8 @@ func newImageDestination(imageRef storageReference) (*storageImageDestination, e
 	if err != nil {
 		return nil, errors.Wrapf(err, "error creating a temporary directory")
 	}
-	// Break reading of the reference we're writing, so that copy.Image() won't try to rewrite
-	// schema1 image manifests to remove embedded references, since that changes the manifest's
-	// digest, and that makes the image unusable if we subsequently try to access it using a
-	// reference that mentions the no-longer-correct digest.
-	publicRef := imageRef
-	publicRef.name = nil
 	image := &storageImageDestination{
 		imageRef:       imageRef,
-		publicRef:      publicRef,
 		directory:      directory,
 		blobDiffIDs:    make(map[digest.Digest]digest.Digest),
 		fileSizes:      make(map[digest.Digest]int64),
@@ -261,11 +253,10 @@ func newImageDestination(imageRef storageReference) (*storageImageDestination, e
 	return image, nil
 }
 
-// Reference returns a mostly-usable image reference that can't return a DockerReference, to
-// avoid triggering logic in copy.Image() that rewrites schema 1 image manifests in order to
-// remove image names that they contain which don't match the value we're using.
+// Reference returns the reference used to set up this destination.  Note that this should directly correspond to user's intent,
+// e.g. it should use the public hostname instead of the result of resolving CNAMEs or following redirects.
 func (s storageImageDestination) Reference() types.ImageReference {
-	return s.publicRef
+	return s.imageRef
 }
 
 // Close cleans up the temporary directory.
@@ -613,7 +604,7 @@ func (s *storageImageDestination) Commit(ctx context.Context) error {
 	if name := s.imageRef.DockerReference(); len(oldNames) > 0 || name != nil {
 		names := []string{}
 		if name != nil {
-			names = append(names, verboseName(name))
+			names = append(names, name.String())
 		}
 		if len(oldNames) > 0 {
 			names = append(names, oldNames...)
@@ -703,6 +694,13 @@ func (s *storageImageDestination) MustMatchRuntimeOS() bool {
 	return true
 }
 
+// IgnoresEmbeddedDockerReference returns true iff the destination does not care about Image.EmbeddedDockerReferenceConflicts(),
+// and would prefer to receive an unmodified manifest instead of one modified for the destination.
+// Does not make a difference if Reference().DockerReference() is nil.
+func (s *storageImageDestination) IgnoresEmbeddedDockerReference() bool {
+	return true // Yes, we want the unmodified manifest
+}
+
 // PutSignatures records the image's signatures for committing as a single data blob.
 func (s *storageImageDestination) PutSignatures(ctx context.Context, signatures [][]byte) error {
 	sizes := []int{}

storage/storage_reference.go

@@ -9,63 +9,72 @@ import (
 	"github.com/containers/image/docker/reference"
 	"github.com/containers/image/types"
 	"github.com/containers/storage"
-	digest "github.com/opencontainers/go-digest"
 	"github.com/pkg/errors"
 	"github.com/sirupsen/logrus"
 )
 
 // A storageReference holds an arbitrary name and/or an ID, which is a 32-byte
 // value hex-encoded into a 64-character string, and a reference to a Store
 // where an image is, or would be, kept.
+// Either "named" or "id" must be set.
 type storageReference struct {
 	transport storageTransport
-	reference string
+	named     reference.Named // may include a tag and/or a digest
 	id        string
-	name      reference.Named
-	tag       string
-	digest    digest.Digest
 }
 
-func newReference(transport storageTransport, reference, id string, name reference.Named, tag string, digest digest.Digest) *storageReference {
+func newReference(transport storageTransport, named reference.Named, id string) (*storageReference, error) {
+	if named == nil && id == "" {
+		return nil, ErrInvalidReference
+	}
 	// We take a copy of the transport, which contains a pointer to the
 	// store that it used for resolving this reference, so that the
 	// transport that we'll return from Transport() won't be affected by
 	// further calls to the original transport's SetStore() method.
 	return &storageReference{
 		transport: transport,
-		reference: reference,
+		named:     named,
 		id:        id,
-		name:      name,
-		tag:       tag,
-		digest:    digest,
+	}, nil
+}
+
+// imageMatchesRepo returns true iff image.Names contains an element with the same repo as ref
+func imageMatchesRepo(image *storage.Image, ref reference.Named) bool {
+	repo := ref.Name()
+	for _, name := range image.Names {
+		if named, err := reference.ParseNormalizedNamed(name); err == nil {
+			if named.Name() == repo {
+				return true
+			}
+		}
 	}
+	return false
 }
 
 // Resolve the reference's name to an image ID in the store, if there's already
 // one present with the same name or ID, and return the image.
 func (s *storageReference) resolveImage() (*storage.Image, error) {
+	var loadedImage *storage.Image
 	if s.id == "" {
 		// Look for an image that has the expanded reference name as an explicit Name value.
-		image, err := s.transport.store.Image(s.reference)
+		image, err := s.transport.store.Image(s.named.String())
 		if image != nil && err == nil {
+			loadedImage = image
 			s.id = image.ID
 		}
 	}
-	if s.id == "" && s.name != nil && s.digest != "" {
-		// Look for an image with the specified digest that has the same name,
-		// though possibly with a different tag or digest, as a Name value, so
-		// that the canonical reference can be implicitly resolved to the image.
-		images, err := s.transport.store.ImagesByDigest(s.digest)
-		if images != nil && err == nil {
-			repo := reference.FamiliarName(reference.TrimNamed(s.name))
-		search:
-			for _, image := range images {
-				for _, name := range image.Names {
-					if named, err := reference.ParseNormalizedNamed(name); err == nil {
-						if reference.FamiliarName(reference.TrimNamed(named)) == repo {
-							s.id = image.ID
-							break search
-						}
+	if s.id == "" && s.named != nil {
+		if digested, ok := s.named.(reference.Digested); ok {
+			// Look for an image with the specified digest that has the same name,
+			// though possibly with a different tag or digest, as a Name value, so
+			// that the canonical reference can be implicitly resolved to the image.
+			images, err := s.transport.store.ImagesByDigest(digested.Digest())
+			if images != nil && err == nil {
+				for _, image := range images {
+					if imageMatchesRepo(image, s.named) {
+						loadedImage = image
+						s.id = image.ID
+						break
 					}
 				}
 			}
@@ -75,27 +84,20 @@ func (s *storageReference) resolveImage() (*storage.Image, error) {
 		logrus.Debugf("reference %q does not resolve to an image ID", s.StringWithinTransport())
 		return nil, errors.Wrapf(ErrNoSuchImage, "reference %q does not resolve to an image ID", s.StringWithinTransport())
 	}
-	img, err := s.transport.store.Image(s.id)
-	if err != nil {
-		return nil, errors.Wrapf(err, "error reading image %q", s.id)
-	}
-	if s.name != nil {
-		repo := reference.FamiliarName(reference.TrimNamed(s.name))
-		nameMatch := false
-		for _, name := range img.Names {
-			if named, err := reference.ParseNormalizedNamed(name); err == nil {
-				if reference.FamiliarName(reference.TrimNamed(named)) == repo {
-					nameMatch = true
-					break
-				}
-			}
+	if loadedImage == nil {
+		img, err := s.transport.store.Image(s.id)
+		if err != nil {
+			return nil, errors.Wrapf(err, "error reading image %q", s.id)
 		}
-		if !nameMatch {
+		loadedImage = img
+	}
+	if s.named != nil {
+		if !imageMatchesRepo(loadedImage, s.named) {
 			logrus.Errorf("no image matching reference %q found", s.StringWithinTransport())
 			return nil, ErrNoSuchImage
 		}
 	}
-	return img, nil
+	return loadedImage, nil
 }
 
 // Return a Transport object that defaults to using the same store that we used
@@ -110,20 +112,7 @@ func (s storageReference) Transport() types.ImageTransport {
 
 // Return a name with a tag or digest, if we have either, else return it bare.
 func (s storageReference) DockerReference() reference.Named {
-	if s.name == nil {
-		return nil
-	}
-	if s.tag != "" {
-		if namedTagged, err := reference.WithTag(s.name, s.tag); err == nil {
-			return namedTagged
-		}
-	}
-	if s.digest != "" {
-		if canonical, err := reference.WithDigest(s.name, s.digest); err == nil {
-			return canonical
-		}
-	}
-	return s.name
+	return s.named
 }
 
 // Return a name with a tag, prefixed with the graph root and driver name, to
@@ -135,25 +124,25 @@ func (s storageReference) StringWithinTransport() string {
 	if len(options) > 0 {
 		optionsList = ":" + strings.Join(options, ",")
 	}
-	storeSpec := "[" + s.transport.store.GraphDriverName() + "@" + s.transport.store.GraphRoot() + "+" + s.transport.store.RunRoot() + optionsList + "]"
-	if s.reference == "" {
-		return storeSpec + "@" + s.id
+	res := "[" + s.transport.store.GraphDriverName() + "@" + s.transport.store.GraphRoot() + "+" + s.transport.store.RunRoot() + optionsList + "]"
+	if s.named != nil {
+		res = res + s.named.String()
 	}
-	if s.id == "" {
-		return storeSpec + s.reference
+	if s.id != "" {
+		res = res + "@" + s.id
 	}
-	return storeSpec + s.reference + "@" + s.id
+	return res
 }
 
 func (s storageReference) PolicyConfigurationIdentity() string {
-	storeSpec := "[" + s.transport.store.GraphDriverName() + "@" + s.transport.store.GraphRoot() + "]"
-	if s.name == nil {
-		return storeSpec + "@" + s.id
+	res := "[" + s.transport.store.GraphDriverName() + "@" + s.transport.store.GraphRoot() + "]"
+	if s.named != nil {
+		res = res + s.named.String()
 	}
-	if s.id == "" {
-		return storeSpec + s.reference
+	if s.id != "" {
+		res = res + "@" + s.id
 	}
-	return storeSpec + s.reference + "@" + s.id
+	return res
 }
 
 // Also accept policy that's tied to the combination of the graph root and
@@ -164,9 +153,17 @@ func (s storageReference) PolicyConfigurationNamespaces() []string {
 	storeSpec := "[" + s.transport.store.GraphDriverName() + "@" + s.transport.store.GraphRoot() + "]"
 	driverlessStoreSpec := "[" + s.transport.store.GraphRoot() + "]"
 	namespaces := []string{}
-	if s.name != nil {
-		name := reference.TrimNamed(s.name)
-		components := strings.Split(name.String(), "/")
+	if s.named != nil {
+		if s.id != "" {
+			// The reference without the ID is also a valid namespace.
+			namespaces = append(namespaces, storeSpec+s.named.String())
+		}
+		tagged, isTagged := s.named.(reference.Tagged)
+		_, isDigested := s.named.(reference.Digested)
+		if isTagged && isDigested { // s.named is "name:tag@digest"; add a "name:tag" parent namespace.
+			namespaces = append(namespaces, storeSpec+s.named.Name()+":"+tagged.Tag())
+		}
+		components := strings.Split(s.named.Name(), "/")
 		for len(components) > 0 {
 			namespaces = append(namespaces, storeSpec+strings.Join(components, "/"))
 			components = components[:len(components)-1]