Merge pull request #3296 from dmcgowan/fix-export-labels

Use a single custom annotation for export

images/annotations.go

@@ -20,9 +20,4 @@ const (
 	// AnnotationImageName is an annotation on a Descriptor in an index.json
 	// containing the `Name` value as used by an `Image` struct
 	AnnotationImageName = "io.containerd.image.name"
-
-	// AnnotationImageNamePrefix is used the same way as AnnotationImageName
-	// but may be used to refer to additional names in the annotation map
-	// using user-defined suffixes (i.e. "extra.1")
-	AnnotationImageNamePrefix = AnnotationImageName + "."
 )

images/archive/exporter.go

@@ -20,11 +20,9 @@ import (
 	"archive/tar"
 	"context"
 	"encoding/json"
-	"fmt"
 	"io"
 	"path"
 	"sort"
-	"strings"
 
 	"github.com/containerd/containerd/content"
 	"github.com/containerd/containerd/errdefs"
@@ -83,9 +81,8 @@ func WithImage(is images.Store, name string) ExportOpt {
 			return err
 		}
 
-		var i int
-		o.manifests, i = appendDescriptor(o.manifests, img.Target)
-		o.manifests[i].Annotations = addNameAnnotation(name, o.manifests[i].Annotations)
+		img.Target.Annotations = addNameAnnotation(name, img.Target.Annotations)
+		o.manifests = append(o.manifests, img.Target)
 
 		return nil
 	}
@@ -96,9 +93,7 @@ func WithImage(is images.Store, name string) ExportOpt {
 // descriptor if needed.
 func WithManifest(manifest ocispec.Descriptor) ExportOpt {
 	return func(ctx context.Context, o *exportOptions) error {
-		var i int
-		o.manifests, i = appendDescriptor(o.manifests, manifest)
-		o.manifests[i].Annotations = manifest.Annotations
+		o.manifests = append(o.manifests, manifest)
 		return nil
 	}
 }
@@ -107,49 +102,23 @@ func WithManifest(manifest ocispec.Descriptor) ExportOpt {
 // with the provided names.
 func WithNamedManifest(manifest ocispec.Descriptor, names ...string) ExportOpt {
 	return func(ctx context.Context, o *exportOptions) error {
-		var i int
-		o.manifests, i = appendDescriptor(o.manifests, manifest)
 		for _, name := range names {
-			o.manifests[i].Annotations = addNameAnnotation(name, o.manifests[i].Annotations)
+			manifest.Annotations = addNameAnnotation(name, manifest.Annotations)
+			o.manifests = append(o.manifests, manifest)
 		}
 
 		return nil
 	}
 }
 
-func appendDescriptor(descs []ocispec.Descriptor, desc ocispec.Descriptor) ([]ocispec.Descriptor, int) {
-	i := 0
-	for i < len(descs) {
-		if descs[i].Digest == desc.Digest {
-			return descs, i
-		}
-		i++
-	}
-	return append(descs, desc), i
-}
-
 func addNameAnnotation(name string, annotations map[string]string) map[string]string {
 	if annotations == nil {
 		annotations = map[string]string{}
 	}
 
-	i := 0
-	for {
-		key := images.AnnotationImageName
-		if i > 0 {
-			key = fmt.Sprintf("%sextra.%d", images.AnnotationImageNamePrefix, i)
-		}
-		i++
+	annotations[images.AnnotationImageName] = name
+	annotations[ocispec.AnnotationRefName] = ociReferenceName(name)
 
-		if val, ok := annotations[key]; ok {
-			if val != name {
-				continue
-			}
-		} else {
-			annotations[key] = name
-		}
-		break
-	}
 	return annotations
 }
 
@@ -168,78 +137,100 @@ func Export(ctx context.Context, store content.Provider, writer io.Writer, opts
 	}
 
 	algorithms := map[string]struct{}{}
-	manifestTags := map[string]ocispec.Descriptor{}
+	dManifests := map[digest.Digest]*exportManifest{}
+	resolvedIndex := map[digest.Digest]digest.Digest{}
 	for _, desc := range eo.manifests {
 		switch desc.MediaType {
 		case images.MediaTypeDockerSchema2Manifest, ocispec.MediaTypeImageManifest:
-			r, err := getRecords(ctx, store, desc, algorithms)
-			if err != nil {
-				return err
-			}
-			records = append(records, r...)
+			mt, ok := dManifests[desc.Digest]
+			if !ok {
+				// TODO(containerd): Skip if already added
+				r, err := getRecords(ctx, store, desc, algorithms)
+				if err != nil {
+					return err
+				}
+				records = append(records, r...)
 
-			for _, name := range imageNames(desc.Annotations) {
-				manifestTags[name] = desc
+				mt = &exportManifest{
+					manifest: desc,
+				}
+				dManifests[desc.Digest] = mt
+			}
+
+			name := desc.Annotations[images.AnnotationImageName]
+			if name != "" && !eo.skipDockerManifest {
+				mt.names = append(mt.names, name)
 			}
 		case images.MediaTypeDockerSchema2ManifestList, ocispec.MediaTypeImageIndex:
-			records = append(records, blobRecord(store, desc))
+			d, ok := resolvedIndex[desc.Digest]
+			if !ok {
+				records = append(records, blobRecord(store, desc))
 
-			p, err := content.ReadBlob(ctx, store, desc)
-			if err != nil {
-				return err
-			}
-
-			var index ocispec.Index
-			if err := json.Unmarshal(p, &index); err != nil {
-				return err
-			}
-
-			names := imageNames(desc.Annotations)
-			var manifests []ocispec.Descriptor
-			for _, m := range index.Manifests {
-				if eo.platform != nil {
-					if m.Platform == nil || eo.platform.Match(*m.Platform) {
-						manifests = append(manifests, m)
-					} else if !eo.allPlatforms {
-						continue
-					}
-				}
-
-				r, err := getRecords(ctx, store, m, algorithms)
+				p, err := content.ReadBlob(ctx, store, desc)
 				if err != nil {
 					return err
 				}
 
-				records = append(records, r...)
-			}
-
-			if len(names) > 0 && !eo.skipDockerManifest {
-				if len(manifests) >= 1 {
-					if len(manifests) > 1 {
-						sort.SliceStable(manifests, func(i, j int) bool {
-							if manifests[i].Platform == nil {
-								return false
-							}
-							if manifests[j].Platform == nil {
-								return true
-							}
-							return eo.platform.Less(*manifests[i].Platform, *manifests[j].Platform)
-						})
-					}
-					for _, name := range names {
-						manifestTags[name] = manifests[0]
-					}
-				} else if eo.platform != nil {
-					return errors.Wrap(errdefs.ErrNotFound, "no manifest found for platform")
+				var index ocispec.Index
+				if err := json.Unmarshal(p, &index); err != nil {
+					return err
 				}
+
+				var manifests []ocispec.Descriptor
+				for _, m := range index.Manifests {
+					if eo.platform != nil {
+						if m.Platform == nil || eo.platform.Match(*m.Platform) {
+							manifests = append(manifests, m)
+						} else if !eo.allPlatforms {
+							continue
+						}
+					}
+
+					r, err := getRecords(ctx, store, m, algorithms)
+					if err != nil {
+						return err
+					}
+
+					records = append(records, r...)
+				}
+
+				if !eo.skipDockerManifest {
+					if len(manifests) >= 1 {
+						if len(manifests) > 1 {
+							sort.SliceStable(manifests, func(i, j int) bool {
+								if manifests[i].Platform == nil {
+									return false
+								}
+								if manifests[j].Platform == nil {
+									return true
+								}
+								return eo.platform.Less(*manifests[i].Platform, *manifests[j].Platform)
+							})
+						}
+						d = manifests[0].Digest
+						dManifests[d] = &exportManifest{
+							manifest: manifests[0],
+						}
+					} else if eo.platform != nil {
+						return errors.Wrap(errdefs.ErrNotFound, "no manifest found for platform")
+					}
+				}
+				resolvedIndex[desc.Digest] = d
+			}
+			if d != "" {
+				if name := desc.Annotations[images.AnnotationImageName]; name != "" {
+					mt := dManifests[d]
+					mt.names = append(mt.names, name)
+				}
+
 			}
 		default:
 			return errors.Wrap(errdefs.ErrInvalidArgument, "only manifests may be exported")
 		}
 	}
 
-	if len(manifestTags) > 0 {
-		tr, err := manifestsRecord(ctx, store, manifestTags)
+	if len(dManifests) > 0 {
+		tr, err := manifestsRecord(ctx, store, dManifests)
 		if err != nil {
 			return errors.Wrap(err, "unable to create manifests file")
 		}
@@ -259,16 +250,6 @@ func Export(ctx context.Context, store content.Provider, writer io.Writer, opts
 	return writeTar(ctx, tw, records)
 }
 
-func imageNames(annotations map[string]string) []string {
-	var names []string
-	for k, v := range annotations {
-		if k == images.AnnotationImageName || strings.HasPrefix(k, images.AnnotationImageName) {
-			names = append(names, v)
-		}
-	}
-	return names
-}
-
 func getRecords(ctx context.Context, store content.Provider, desc ocispec.Descriptor, algorithms map[string]struct{}) ([]tarRecord, error) {
 	var records []tarRecord
 	exportHandler := func(ctx context.Context, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) {
@@ -394,16 +375,21 @@ func ociIndexRecord(manifests []ocispec.Descriptor) tarRecord {
 	}
 }
 
-func manifestsRecord(ctx context.Context, store content.Provider, manifests map[string]ocispec.Descriptor) (tarRecord, error) {
-	type mfst struct {
+type exportManifest struct {
+	manifest ocispec.Descriptor
+	names    []string
+}
+
+func manifestsRecord(ctx context.Context, store content.Provider, manifests map[digest.Digest]*exportManifest) (tarRecord, error) {
+	mfsts := make([]struct {
 		Config   string
 		RepoTags []string
 		Layers   []string
-	}
+	}, len(manifests))
 
-	images := map[digest.Digest]mfst{}
-	for name, m := range manifests {
-		p, err := content.ReadBlob(ctx, store, m)
+	var i int
+	for _, m := range manifests {
+		p, err := content.ReadBlob(ctx, store, m.manifest)
 		if err != nil {
 			return tarRecord{}, err
 		}
@@ -413,32 +399,26 @@ func manifestsRecord(ctx context.Context, store content.Provider, manifests map[
 			return tarRecord{}, err
 		}
 		if err := manifest.Config.Digest.Validate(); err != nil {
-			return tarRecord{}, errors.Wrapf(err, "invalid manifest %q", m.Digest)
-		}
-
-		nname, err := familiarizeReference(name)
-		if err != nil {
-			return tarRecord{}, err
+			return tarRecord{}, errors.Wrapf(err, "invalid manifest %q", m.manifest.Digest)
 		}
 
 		dgst := manifest.Config.Digest
-		mf, ok := images[dgst]
-		if !ok {
-			mf.Config = path.Join("blobs", dgst.Algorithm().String(), dgst.Encoded())
-			for _, l := range manifest.Layers {
-				path := path.Join("blobs", l.Digest.Algorithm().String(), l.Digest.Encoded())
-				mf.Layers = append(mf.Layers, path)
-			}
+		mfsts[i].Config = path.Join("blobs", dgst.Algorithm().String(), dgst.Encoded())
+		for _, l := range manifest.Layers {
+			path := path.Join("blobs", l.Digest.Algorithm().String(), l.Digest.Encoded())
+			mfsts[i].Layers = append(mfsts[i].Layers, path)
 		}
 
-		mf.RepoTags = append(mf.RepoTags, nname)
+		for _, name := range m.names {
+			nname, err := familiarizeReference(name)
+			if err != nil {
+				return tarRecord{}, err
+			}
 
-		images[dgst] = mf
-	}
+			mfsts[i].RepoTags = append(mfsts[i].RepoTags, nname)
+		}
 
-	var mfsts []mfst
-	for _, mf := range images {
-		mfsts = append(mfsts, mf)
+		i++
 	}
 
 	b, err := json.Marshal(mfsts)

images/archive/importer.go

@@ -181,7 +181,8 @@ func ImportIndex(ctx context.Context, store content.Store, reader io.Reader) (oc
 				}
 
 				mfstdesc.Annotations = map[string]string{
-					ocispec.AnnotationRefName: normalized,
+					images.AnnotationImageName: normalized,
+					ocispec.AnnotationRefName:  ociReferenceName(normalized),
 				}
 
 				idx.Manifests = append(idx.Manifests, mfstdesc)

images/archive/reference.go

@@ -19,7 +19,8 @@ package archive
 import (
 	"strings"
 
-	"github.com/docker/distribution/reference"
+	"github.com/containerd/containerd/reference"
+	distref "github.com/docker/distribution/reference"
 	"github.com/opencontainers/go-digest"
 	"github.com/pkg/errors"
 )
@@ -69,7 +70,7 @@ func isImagePrefix(s, prefix string) bool {
 
 func normalizeReference(ref string) (string, error) {
 	// TODO: Replace this function to not depend on reference package
-	normalized, err := reference.ParseDockerRef(ref)
+	normalized, err := distref.ParseDockerRef(ref)
 	if err != nil {
 		return "", errors.Wrapf(err, "normalize image ref %q", ref)
 	}
@@ -78,13 +79,28 @@ func normalizeReference(ref string) (string, error) {
 }
 
 func familiarizeReference(ref string) (string, error) {
-	named, err := reference.ParseNormalizedNamed(ref)
+	named, err := distref.ParseNormalizedNamed(ref)
 	if err != nil {
 		return "", errors.Wrapf(err, "failed to parse %q", ref)
 	}
-	named = reference.TagNameOnly(named)
+	named = distref.TagNameOnly(named)
 
-	return reference.FamiliarString(named), nil
+	return distref.FamiliarString(named), nil
+}
+
+func ociReferenceName(name string) string {
+	// OCI defines the reference name as only a tag excluding the
+	// repository. The containerd annotation contains the full image name
+	// since the tag is insufficent for correctly naming and referring to an
+	// image
+	var ociRef string
+	if spec, err := reference.Parse(name); err == nil {
+		ociRef = spec.Object
+	} else {
+		ociRef = name
+	}
+
+	return ociRef
 }
 
 // DigestTranslator creates a digest reference by adding the

import.go

@@ -20,7 +20,6 @@ import (
 	"context"
 	"encoding/json"
 	"io"
-	"strings"
 
 	"github.com/containerd/containerd/content"
 	"github.com/containerd/containerd/errdefs"
@@ -131,8 +130,8 @@ func (c *Client) Import(ctx context.Context, reader io.Reader, opts ...ImportOpt
 		}
 
 		for _, m := range idx.Manifests {
-			names := imageNames(m.Annotations, iopts.imageRefT)
-			for _, name := range names {
+			name := imageName(m.Annotations, iopts.imageRefT)
+			if name != "" {
 				imgs = append(imgs, images.Image{
 					Name:   name,
 					Target: m,
@@ -176,34 +175,16 @@ func (c *Client) Import(ctx context.Context, reader io.Reader, opts ...ImportOpt
 	return imgs, nil
 }
 
-func imageNames(annotations map[string]string, ociCleanup func(string) string) []string {
-	var names []string
-	for k, v := range annotations {
-		if k == ocispec.AnnotationRefName {
-			if ociCleanup != nil {
-				v = ociCleanup(v)
-			}
-			if v != "" {
-				names = appendSorted(names, v)
-			}
-		} else if k == images.AnnotationImageName || strings.HasPrefix(k, images.AnnotationImageNamePrefix) {
-			names = appendSorted(names, v)
-
+func imageName(annotations map[string]string, ociCleanup func(string) string) string {
+	name := annotations[images.AnnotationImageName]
+	if name != "" {
+		return name
+	}
+	name = annotations[ocispec.AnnotationRefName]
+	if name != "" {
+		if ociCleanup != nil {
+			name = ociCleanup(name)
 		}
 	}
-	return names
-}
-
-func appendSorted(arr []string, s string) []string {
-	for i, c := range arr {
-		if s < c {
-			arr = append(arr, "")
-			copy(arr[i+1:], arr[i:])
-			arr[i] = s
-			return arr
-		} else if s == c {
-			return arr
-		}
-	}
-	return append(arr, s)
+	return name
 }

import_test.go

@@ -224,7 +224,7 @@ func TestImport(t *testing.T) {
 			},
 		},
 		{
-			Name: "OCIPrefixName",
+			Name: "OCIPrefixName2",
 			Writer: tartest.TarAll(
 				tc.Dir("blobs", 0755),
 				tc.Dir("blobs/sha256", 0755),