Merge pull request #318 from umohnani8/oci_name

Fix oci to save the full image name in index.json

oci/layout/oci_dest.go

@@ -23,13 +23,16 @@ type ociImageDestination struct {
 }
 
 // newImageDestination returns an ImageDestination for writing to an existing directory.
-func newImageDestination(ref ociReference) types.ImageDestination {
+func newImageDestination(ref ociReference) (types.ImageDestination, error) {
+	if ref.image == "" {
+		return nil, errors.Errorf("cannot save image with empty image.ref.name")
+	}
 	index := imgspecv1.Index{
 		Versioned: imgspec.Versioned{
 			SchemaVersion: 2,
 		},
 	}
-	return &ociImageDestination{ref: ref, index: index}
+	return &ociImageDestination{ref: ref, index: index}, nil
 }
 
 // Reference returns the reference used to set up this destination.  Note that this should directly correspond to user's intent,
@@ -178,7 +181,7 @@ func (d *ociImageDestination) PutManifest(m []byte) error {
 	}
 
 	annotations := make(map[string]string)
-	annotations["org.opencontainers.image.ref.name"] = d.ref.tag
+	annotations["org.opencontainers.image.ref.name"] = d.ref.image
 	desc.Annotations = annotations
 	desc.Platform = &imgspecv1.Platform{
 		Architecture: runtime.GOARCH,

oci/layout/oci_transport.go

@@ -36,35 +36,37 @@ func (t ociTransport) ParseReference(reference string) (types.ImageReference, er
 	return ParseReference(reference)
 }
 
-var refRegexp = regexp.MustCompile(`^([A-Za-z0-9._-]+)+$`)
+// annotation spex from https://github.com/opencontainers/image-spec/blob/master/annotations.md#pre-defined-annotation-keys
+const (
+	separator = `(?:[-._:@+]|--)`
+	alphanum  = `(?:[A-Za-z0-9]+)`
+	component = `(?:` + alphanum + `(?:` + separator + alphanum + `)*)`
+)
+
+var refRegexp = regexp.MustCompile(`^` + component + `(?:/` + component + `)*$`)
 
 // ValidatePolicyConfigurationScope checks that scope is a valid name for a signature.PolicyTransportScopes keys
 // (i.e. a valid PolicyConfigurationIdentity() or PolicyConfigurationNamespaces() return value).
 // It is acceptable to allow an invalid value which will never be matched, it can "only" cause user confusion.
 // scope passed to this function will not be "", that value is always allowed.
 func (t ociTransport) ValidatePolicyConfigurationScope(scope string) error {
 	var dir string
-	sep := strings.LastIndex(scope, ":")
-	if sep == -1 {
-		dir = scope
-	} else {
-		dir = scope[:sep]
-		tag := scope[sep+1:]
-		if !refRegexp.MatchString(tag) {
-			return errors.Errorf("Invalid tag %s", tag)
-		}
-	}
+	sep := strings.SplitN(scope, ":", 2)
+	dir = sep[0]
 
-	if strings.Contains(dir, ":") {
-		return errors.Errorf("Invalid OCI reference %s: path contains a colon", scope)
+	if len(sep) == 2 {
+		image := sep[1]
+		if !refRegexp.MatchString(image) {
+			return errors.Errorf("Invalid image %s", image)
+		}
 	}
 
 	if !strings.HasPrefix(dir, "/") {
 		return errors.Errorf("Invalid scope %s: must be an absolute path", scope)
 	}
 	// Refuse also "/", otherwise "/" and "" would have the same semantics,
 	// and "" could be unexpectedly shadowed by the "/" entry.
-	// (Note: we do allow "/:sometag", a bit ridiculous but why refuse it?)
+	// (Note: we do allow "/:someimage", a bit ridiculous but why refuse it?)
 	if scope == "/" {
 		return errors.New(`Invalid scope "/": Use the generic default scope ""`)
 	}
@@ -85,41 +87,39 @@ type ociReference struct {
 	// (But in general, we make no attempt to be completely safe against concurrent hostile filesystem modifications.)
 	dir         string // As specified by the user. May be relative, contain symlinks, etc.
 	resolvedDir string // Absolute path with no symlinks, at least at the time of its creation. Primarily used for policy namespaces.
-	tag         string
+	image       string // If image=="", it means the only image in the index.json is used
 }
 
 // ParseReference converts a string, which should not start with the ImageTransport.Name prefix, into an OCI ImageReference.
 func ParseReference(reference string) (types.ImageReference, error) {
-	var dir, tag string
-	sep := strings.LastIndex(reference, ":")
-	if sep == -1 {
-		dir = reference
-		tag = "latest"
-	} else {
-		dir = reference[:sep]
-		tag = reference[sep+1:]
+	var dir, image string
+	sep := strings.SplitN(reference, ":", 2)
+	dir = sep[0]
+
+	if len(sep) == 2 {
+		image = sep[1]
 	}
-	return NewReference(dir, tag)
+	return NewReference(dir, image)
 }
 
-// NewReference returns an OCI reference for a directory and a tag.
+// NewReference returns an OCI reference for a directory and a image.
 //
 // We do not expose an API supplying the resolvedDir; we could, but recomputing it
 // is generally cheap enough that we prefer being confident about the properties of resolvedDir.
-func NewReference(dir, tag string) (types.ImageReference, error) {
+func NewReference(dir, image string) (types.ImageReference, error) {
 	resolved, err := explicitfilepath.ResolvePathToFullyExplicit(dir)
 	if err != nil {
 		return nil, err
 	}
 	// This is necessary to prevent directory paths returned by PolicyConfigurationNamespaces
 	// from being ambiguous with values of PolicyConfigurationIdentity.
 	if strings.Contains(resolved, ":") {
-		return nil, errors.Errorf("Invalid OCI reference %s:%s: path %s contains a colon", dir, tag, resolved)
+		return nil, errors.Errorf("Invalid OCI reference %s:%s: path %s contains a colon", dir, image, resolved)
 	}
-	if !refRegexp.MatchString(tag) {
-		return nil, errors.Errorf("Invalid tag %s", tag)
+	if len(image) > 0 && !refRegexp.MatchString(image) {
+		return nil, errors.Errorf("Invalid image %s", image)
 	}
-	return ociReference{dir: dir, resolvedDir: resolved, tag: tag}, nil
+	return ociReference{dir: dir, resolvedDir: resolved, image: image}, nil
 }
 
 func (ref ociReference) Transport() types.ImageTransport {
@@ -132,7 +132,7 @@ func (ref ociReference) Transport() types.ImageTransport {
 // e.g. default attribute values omitted by the user may be filled in in the return value, or vice versa.
 // WARNING: Do not use the return value in the UI to describe an image, it does not contain the Transport().Name() prefix.
 func (ref ociReference) StringWithinTransport() string {
-	return fmt.Sprintf("%s:%s", ref.dir, ref.tag)
+	return fmt.Sprintf("%s:%s", ref.dir, ref.image)
 }
 
 // DockerReference returns a Docker reference associated with this reference
@@ -150,7 +150,10 @@ func (ref ociReference) DockerReference() reference.Named {
 // not required/guaranteed that it will be a valid input to Transport().ParseReference().
 // Returns "" if configuration identities for these references are not supported.
 func (ref ociReference) PolicyConfigurationIdentity() string {
-	return fmt.Sprintf("%s:%s", ref.resolvedDir, ref.tag)
+	// NOTE: ref.image is not a part of the image identity, because "$dir:$someimage" and "$dir:" may mean the
+	// same image and the two can’t be statically disambiguated.  Using at least the repository directory is
+	// less granular but hopefully still useful.
+	return fmt.Sprintf("%s", ref.resolvedDir)
 }
 
 // PolicyConfigurationNamespaces returns a list of other policy configuration namespaces to search
@@ -196,26 +199,48 @@ func (ref ociReference) getManifestDescriptor() (imgspecv1.Descriptor, error) {
 	if err := json.NewDecoder(indexJSON).Decode(&index); err != nil {
 		return imgspecv1.Descriptor{}, err
 	}
+
 	var d *imgspecv1.Descriptor
-	for _, md := range index.Manifests {
-		if md.MediaType != imgspecv1.MediaTypeImageManifest {
-			continue
-		}
-		refName, ok := md.Annotations["org.opencontainers.image.ref.name"]
-		if !ok {
-			continue
+	if ref.image == "" {
+		// return manifest if only one image is in the oci directory
+		if len(index.Manifests) == 1 {
+			d = &index.Manifests[0]
+		} else {
+			// ask user to choose image when more than one image in the oci directory
+			return imgspecv1.Descriptor{}, errors.Wrapf(err, "more than one image in oci, choose an image")
 		}
-		if refName == ref.tag {
-			d = &md
-			break
+	} else {
+		// if image specified, look through all manifests for a match
+		for _, md := range index.Manifests {
+			if md.MediaType != imgspecv1.MediaTypeImageManifest {
+				continue
+			}
+			refName, ok := md.Annotations["org.opencontainers.image.ref.name"]
+			if !ok {
+				continue
+			}
+			if refName == ref.image {
+				d = &md
+				break
+			}
 		}
 	}
 	if d == nil {
-		return imgspecv1.Descriptor{}, fmt.Errorf("no descriptor found for reference %q", ref.tag)
+		return imgspecv1.Descriptor{}, fmt.Errorf("no descriptor found for reference %q", ref.image)
 	}
 	return *d, nil
 }
 
+// LoadManifestDescriptor loads the manifest descriptor to be used to retrieve the image name
+// when pulling an image
+func LoadManifestDescriptor(imgRef types.ImageReference) (imgspecv1.Descriptor, error) {
+	ociRef, ok := imgRef.(ociReference)
+	if !ok {
+		return imgspecv1.Descriptor{}, errors.Errorf("error typecasting, need type ociRef")
+	}
+	return ociRef.getManifestDescriptor()
+}
+
 // NewImageSource returns a types.ImageSource for this reference,
 // asking the backend to use a manifest from requestedManifestMIMETypes if possible.
 // nil requestedManifestMIMETypes means manifest.DefaultRequestedManifestMIMETypes.
@@ -227,7 +252,7 @@ func (ref ociReference) NewImageSource(ctx *types.SystemContext, requestedManife
 // NewImageDestination returns a types.ImageDestination for this reference.
 // The caller must call .Close() on the returned ImageDestination.
 func (ref ociReference) NewImageDestination(ctx *types.SystemContext) (types.ImageDestination, error) {
-	return newImageDestination(ref), nil
+	return newImageDestination(ref)
 }
 
 // DeleteImage deletes the named image from the registry, if supported.

oci/layout/oci_transport_test.go

@@ -38,9 +38,9 @@ func TestTransportValidatePolicyConfigurationScope(t *testing.T) {
 		"/has/./dot",
 		"/has/dot/../dot",
 		"/trailing/slash/",
-		"/etc:invalid'tag!value@",
-		"/path:with/colons",
-		"/path:with/colons/and:tag",
+		"/etc:invalid'image!value@",
+		"/etc_no_image:",
+		"/etc:multiple_:separators",
 	} {
 		err := Transport.ValidatePolicyConfigurationScope(scope)
 		assert.Error(t, err, scope)
@@ -64,48 +64,57 @@ func testParseReference(t *testing.T, fn func(string) (types.ImageReference, err
 		"relativepath",
 		tmpDir + "/thisdoesnotexist",
 	} {
-		for _, tag := range []struct{ suffix, tag string }{
-			{":notlatest", "notlatest"},
-			{"", "latest"},
+		for _, image := range []struct{ suffix, image string }{
+			{":notlatest:image", "notlatest:image"},
+			{":latestimage", "latestimage"},
+			{":", ""},
+			{"", ""},
 		} {
-			input := path + tag.suffix
+			input := path + image.suffix
 			ref, err := fn(input)
 			require.NoError(t, err, input)
 			ociRef, ok := ref.(ociReference)
 			require.True(t, ok)
 			assert.Equal(t, path, ociRef.dir, input)
-			assert.Equal(t, tag.tag, ociRef.tag, input)
+			assert.Equal(t, image.image, ociRef.image, input)
 		}
 	}
 
-	_, err = fn(tmpDir + "/with:multiple:colons:and:tag")
-	assert.Error(t, err)
-
-	_, err = fn(tmpDir + ":invalid'tag!value@")
+	_, err = fn(tmpDir + ":invalid'image!value@")
 	assert.Error(t, err)
 }
 
 func TestNewReference(t *testing.T) {
-	const tagValue = "tagValue"
+	const (
+		imageValue   = "imageValue"
+		noImageValue = ""
+	)
 
 	tmpDir, err := ioutil.TempDir("", "oci-transport-test")
 	require.NoError(t, err)
 	defer os.RemoveAll(tmpDir)
 
-	ref, err := NewReference(tmpDir, tagValue)
+	ref, err := NewReference(tmpDir, imageValue)
 	require.NoError(t, err)
 	ociRef, ok := ref.(ociReference)
 	require.True(t, ok)
 	assert.Equal(t, tmpDir, ociRef.dir)
-	assert.Equal(t, tagValue, ociRef.tag)
+	assert.Equal(t, imageValue, ociRef.image)
+
+	ref, err = NewReference(tmpDir, noImageValue)
+	require.NoError(t, err)
+	ociRef, ok = ref.(ociReference)
+	require.True(t, ok)
+	assert.Equal(t, tmpDir, ociRef.dir)
+	assert.Equal(t, noImageValue, ociRef.image)
 
-	_, err = NewReference(tmpDir+"/thisparentdoesnotexist/something", tagValue)
+	_, err = NewReference(tmpDir+"/thisparentdoesnotexist/something", imageValue)
 	assert.Error(t, err)
 
-	_, err = NewReference(tmpDir+"/has:colon", tagValue)
+	_, err = NewReference(tmpDir, "invalid'image!value@")
 	assert.Error(t, err)
 
-	_, err = NewReference(tmpDir, "invalid'tag!value@")
+	_, err = NewReference(tmpDir+"/has:colon", imageValue)
 	assert.Error(t, err)
 }
 
@@ -127,14 +136,14 @@ func refToTempOCI(t *testing.T) (ref types.ImageReference, tmpDir string) {
 				"os": "linux"
 			},
 			"annotations": {
-				"org.opencontainers.image.ref.name": "tagValue"
+				"org.opencontainers.image.ref.name": "imageValue"
 			}
 		}
 		]
 	}
 `
 	ioutil.WriteFile(filepath.Join(tmpDir, "index.json"), []byte(m), 0644)
-	ref, err = NewReference(tmpDir, "tagValue")
+	ref, err = NewReference(tmpDir, "imageValue")
 	require.NoError(t, err)
 	return ref, tmpDir
 }
@@ -151,8 +160,8 @@ func TestReferenceStringWithinTransport(t *testing.T) {
 	defer os.RemoveAll(tmpDir)
 
 	for _, c := range []struct{ input, result string }{
-		{"/dir1:notlatest", "/dir1:notlatest"}, // Explicit tag
-		{"/dir2", "/dir2:latest"},              // Default tag
+		{"/dir1:notlatest:notlatest", "/dir1:notlatest:notlatest"}, // Explicit image
+		{"/dir3:", "/dir3:"},                                       // No image
 	} {
 		ref, err := ParseReference(tmpDir + c.input)
 		require.NoError(t, err, c.input)
@@ -176,17 +185,17 @@ func TestReferencePolicyConfigurationIdentity(t *testing.T) {
 	ref, tmpDir := refToTempOCI(t)
 	defer os.RemoveAll(tmpDir)
 
-	assert.Equal(t, tmpDir+":tagValue", ref.PolicyConfigurationIdentity())
+	assert.Equal(t, tmpDir, ref.PolicyConfigurationIdentity())
 	// A non-canonical path.  Test just one, the various other cases are
 	// tested in explicitfilepath.ResolvePathToFullyExplicit.
-	ref, err := NewReference(tmpDir+"/.", "tag2")
+	ref, err := NewReference(tmpDir+"/.", "image2")
 	require.NoError(t, err)
-	assert.Equal(t, tmpDir+":tag2", ref.PolicyConfigurationIdentity())
+	assert.Equal(t, tmpDir, ref.PolicyConfigurationIdentity())
 
 	// "/" as a corner case.
-	ref, err = NewReference("/", "tag3")
+	ref, err = NewReference("/", "image3")
 	require.NoError(t, err)
-	assert.Equal(t, "/:tag3", ref.PolicyConfigurationIdentity())
+	assert.Equal(t, "/", ref.PolicyConfigurationIdentity())
 }
 
 func TestReferencePolicyConfigurationNamespaces(t *testing.T) {
@@ -208,15 +217,15 @@ func TestReferencePolicyConfigurationNamespaces(t *testing.T) {
 	for _, path := range []string{"/etc/skel", "/etc/skel/./."} {
 		_, err := os.Lstat(path)
 		require.NoError(t, err)
-		ref, err := NewReference(path, "sometag")
+		ref, err := NewReference(path, "someimage")
 		require.NoError(t, err)
 		ns := ref.PolicyConfigurationNamespaces()
 		require.NotNil(t, ns)
 		assert.Equal(t, []string{"/etc/skel", "/etc"}, ns)
 	}
 
 	// "/" as a corner case.
-	ref, err := NewReference("/", "tag3")
+	ref, err := NewReference("/", "image3")
 	require.NoError(t, err)
 	assert.Equal(t, []string{}, ref.PolicyConfigurationNamespaces())
 }

transports/alltransports/alltransports_test.go

@@ -33,7 +33,7 @@ func TestImageNameHandling(t *testing.T) {
 		{"docker-daemon", "busybox:latest", "busybox:latest"},
 		{"docker-archive", "/var/lib/oci/busybox.tar:busybox:latest", "/var/lib/oci/busybox.tar:docker.io/library/busybox:latest"},
 		{"docker-archive", "busybox.tar:busybox:latest", "busybox.tar:docker.io/library/busybox:latest"},
-		{"oci", "/etc:sometag", "/etc:sometag"},
+		{"oci", "/etc:someimage:latest", "/etc:someimage:latest"},
 		// "atomic" not tested here because it depends on per-user configuration for the default cluster.
 		// "containers-storage" not tested here because it needs to initialize various directories on the fs.
 	} {