Repository: containers/image (go)

Issue:
oci/layout.NewReference (and others?) doesn’t document the image=="" special case
… meaning that callers who intend to submit an image reference, and don’t specifically filter out `""`, get unexpected behavior.

At least, document this special case (and perhaps audit other `NewReference*` functions for possible other instances of this).

Arguably:
- `NewIndexReference` should be modified to actually reject an “N/A” index value <0
- `NewTagReference` should be added, where passing `""` is an error.

Pull request:
Improve handling and documentation of various transport reference corner cases
See individual commit messages for details.

(Minimally) fixes #2679 .

Patch:
diff --git a/docker/archive/transport.go b/docker/archive/transport.go
index 39e92cac..2c3926cc 100644
--- a/docker/archive/transport.go
+++ b/docker/archive/transport.go
@@ -101,6 +101,9 @@ func NewReference(path string, ref reference.NamedTagged) (types.ImageReference,
 
 // NewIndexReference returns a Docker archive reference for a path and a zero-based source manifest index.
 func NewIndexReference(path string, sourceIndex int) (types.ImageReference, error) {
+	if sourceIndex < 0 {
+		return nil, fmt.Errorf("invalid call to NewIndexReference with negative index %d", sourceIndex)
+	}
 	return newReference(path, nil, sourceIndex, nil, nil)
 }
 
diff --git a/docker/daemon/daemon_transport.go b/docker/daemon/daemon_transport.go
index 9eedff74..9fc0a92f 100644
--- a/docker/daemon/daemon_transport.go
+++ b/docker/daemon/daemon_transport.go
@@ -87,10 +87,13 @@ func ParseReference(refString string) (types.ImageReference, error) {
 
 // NewReference returns a docker-daemon reference for either the supplied image ID (config digest) or the supplied reference (which must satisfy !reference.IsNameOnly)
 func NewReference(id digest.Digest, ref reference.Named) (types.ImageReference, error) {
-	if id != "" && ref != nil {
+	switch {
+	case id != "" && ref != nil:
 		return nil, errors.New("docker-daemon: reference must not have an image ID and a reference string specified at the same time")
-	}
-	if ref != nil {
+	case id == "" && ref == nil:
+		return nil, errors.New("docker-daemon: reference must have at least one of an image ID and a reference string")
+
+	case ref != nil:
 		if reference.IsNameOnly(ref) {
 			return nil, fmt.Errorf("docker-daemon: reference %s has neither a tag nor a digest", reference.FamiliarString(ref))
 		}
diff --git a/docs/containers-transports.5.md b/docs/containers-transports.5.md
index 8073c1c8..1312d443 100644
--- a/docs/containers-transports.5.md
+++ b/docs/containers-transports.5.md
@@ -77,7 +77,6 @@ An image in a directory structure compliant with the "Open Container Image Layou
 
 The _path_ value terminates at the first `:` character; any further `:` characters are not separators, but a part of _reference_.
 The _reference_ is used to set, or match, the `org.opencontainers.image.ref.name` annotation in the top-level index.
-If _reference_ is not specified when reading an image, the directory must contain exactly one image.
 For reading images, @_source-index_ is a zero-based index in manifest (to access untagged images).
 If neither reference nor @_source_index is specified when reading an image, the path must contain exactly one image.
 
diff --git a/oci/archive/oci_transport.go b/oci/archive/oci_transport.go
index d5fee363..1cc3fa3b 100644
--- a/oci/archive/oci_transport.go
+++ b/oci/archive/oci_transport.go
@@ -52,13 +52,13 @@ func (t ociArchiveTransport) ValidatePolicyConfigurationScope(scope string) erro
 	return internal.ValidateScope(scope)
 }
 
-// ParseReference converts a string, which should not start with the ImageTransport.Name prefix, into an OCI ImageReference.
+// ParseReference converts a string, which should not start with the ImageTransport.Name prefix, into an OCI archive ImageReference.
 func ParseReference(reference string) (types.ImageReference, error) {
 	file, image := internal.SplitPathAndImage(reference)
 	return NewReference(file, image)
 }
 
-// NewReference returns an OCI reference for a file and a image.
+// NewReference returns an OCI archive reference for a file and an optional image name annotation (if not "").
 func NewReference(file, image string) (types.ImageReference, error) {
 	resolved, err := explicitfilepath.ResolvePathToFullyExplicit(file)
 	if err != nil {
diff --git a/oci/layout/oci_transport.go b/oci/layout/oci_transport.go
index 800d1aa2..832f8908 100644
--- a/oci/layout/oci_transport.go
+++ b/oci/layout/oci_transport.go
@@ -111,13 +111,13 @@ func newReference(dir, image string, sourceIndex int) (types.ImageReference, err
 
 // NewIndexReference returns an OCI reference for a path and a zero-based source manifest index.
 func NewIndexReference(dir string, sourceIndex int) (types.ImageReference, error) {
+	if sourceIndex < 0 {
+		return nil, fmt.Errorf("invalid call to NewIndexReference with negative index %d", sourceIndex)
+	}
 	return newReference(dir, "", sourceIndex)
 }
 
-// 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.
+// NewReference returns an OCI reference for a directory and an optional image name annotation (if not "").
 func NewReference(dir, image string) (types.ImageReference, error) {
 	return newReference(dir, image, -1)
 }


Test patch:
diff --git a/docker/archive/transport_test.go b/docker/archive/transport_test.go
index 59936855..6bd06802 100644
--- a/docker/archive/transport_test.go
+++ b/docker/archive/transport_test.go
@@ -135,6 +135,8 @@ func TestNewReference(t *testing.T) {
 	ntRef := namedTaggedRef(t, "busybox:latest")
 	_, err = newReference("path", ntRef, 0, nil, nil)
 	assert.Error(t, err)
+	_, err = newReference("path", nil, -2, nil, nil)
+	assert.Error(t, err)
 }
 
 func TestNewIndexReference(t *testing.T) {
@@ -145,7 +147,7 @@ func TestNewIndexReference(t *testing.T) {
 		}{
 			{0, true},
 			{9999990, true},
-			{-1, true},
+			{-1, false},
 			{-2, false},
 		} {
 			res, err := NewIndexReference(path, c.index)
diff --git a/docker/daemon/daemon_transport_test.go b/docker/daemon/daemon_transport_test.go
index 6a08d54d..d7716f48 100644
--- a/docker/daemon/daemon_transport_test.go
+++ b/docker/daemon/daemon_transport_test.go
@@ -63,7 +63,8 @@ func testParseReference(t *testing.T, fn func(string) (types.ImageReference, err
 		{"busybox@" + sha256digest, "", "docker.io/library/busybox@" + sha256digest}, // Explicit digest
 		// A github.com/distribution/reference value can have a tag and a digest at the same time!
 		// Most versions of docker/reference do not handle that (ignoring the tag), so we reject such input.
-		{"busybox:latest@" + sha256digest, "", ""},                                   // Both tag and digest
+		{"busybox:latest@" + sha256digest, "", ""}, // Both tag and digest
+		{"", "", ""}, // Empty input
 		{"docker.io/library/busybox:latest", "", "docker.io/library/busybox:latest"}, // All implied values explicitly specified
 	} {
 		ref, err := fn(c.input)
@@ -135,6 +136,9 @@ func TestNewReference(t *testing.T) {
 	require.NoError(t, err)
 	_, err = NewReference(id, parsed)
 	assert.Error(t, err)
+	// Neither provided
+	_, err = NewReference("", nil)
+	assert.Error(t, err)
 
 	// A reference with neither a tag nor digest
 	parsed, err = reference.ParseNormalizedNamed("busybox")
diff --git a/oci/layout/oci_transport_test.go b/oci/layout/oci_transport_test.go
index 5fb7b2ef..93897aed 100644
--- a/oci/layout/oci_transport_test.go
+++ b/oci/layout/oci_transport_test.go
@@ -279,19 +279,24 @@ func TestNewIndexReference(t *testing.T) {
 	assert.Equal(t, "", ociRef.image)
 	assert.Equal(t, 9999, ociRef.sourceIndex)
 
-	_, err = NewIndexReference(tmpDir+"/thisparentdoesnotexist/something", 10)
-	assert.Error(t, err)
-
-	// sourceIndex cannot be less than -1
-	_, err = NewIndexReference(tmpDir, -3)
-	assert.Error(t, err)
-
-	_, err = NewIndexReference(tmpDir+"/has:colon", 99)
-	assert.Error(t, err)
+	for _, c := range []struct {
+		dir   string
+		index int
+	}{
+		{tmpDir + "/thisparentdoesnotexist/something", 10},
+		{tmpDir, -1},
+		{tmpDir, -3},
+		{tmpDir + "/has:colon", 99},
+	} {
+		_, err = NewIndexReference(c.dir, c.index)
+		assert.Error(t, err)
+	}
 
 	// Test private newReference
 	_, err = newReference(tmpDir, imageValue, 1)
 	assert.Error(t, err)
+	_, err = newReference(tmpDir, "", -3)
+	assert.Error(t, err)
 }
 
 // refToTempOCI creates a temporary directory and returns an reference to it.