Make refNamesFromPossiblyUnqualifiedName independent from Image

... which finally makes it very easy to add comprehensive tests; so do that.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Closes: #1112
Approved by: rhatdan

libpod/image/pull.go

@@ -272,31 +272,31 @@ func hasShaInInputName(inputName string) bool {
 
 // refNamesFromPossiblyUnqualifiedName looks at a decomposed image and determines the possible
 // image names to try pulling in combination with the registries.conf file as well
-func (i *Image) refNamesFromPossiblyUnqualifiedName() ([]*pullRefName, error) {
+func refNamesFromPossiblyUnqualifiedName(inputName string) ([]*pullRefName, error) {
 	var (
 		pullNames []*pullRefName
 		imageName string
 	)
 
-	decomposedImage, err := decompose(i.InputName)
+	decomposedImage, err := decompose(inputName)
 	if err != nil {
 		return nil, err
 	}
 	if decomposedImage.hasRegistry {
-		if hasShaInInputName(i.InputName) {
+		if hasShaInInputName(inputName) {
-			imageName = fmt.Sprintf("%s%s", decomposedImage.transport, i.InputName)
+			imageName = fmt.Sprintf("%s%s", decomposedImage.transport, inputName)
 		} else {
 			imageName = decomposedImage.assembleWithTransport()
 		}
 		srcRef, err := alltransports.ParseImageName(imageName)
 		if err != nil {
-			return nil, errors.Wrapf(err, "unable to parse '%s'", i.InputName)
+			return nil, errors.Wrapf(err, "unable to parse '%s'", inputName)
 		}
 		ps := pullRefName{
-			image:  i.InputName,
+			image:  inputName,
 			srcRef: srcRef,
 		}
-		if hasShaInInputName(i.InputName) {
+		if hasShaInInputName(inputName) {
 			ps.dstName = decomposedImage.assemble()
 		} else {
 			ps.dstName = ps.image
@@ -311,12 +311,12 @@ func (i *Image) refNamesFromPossiblyUnqualifiedName() ([]*pullRefName, error) {
 		for _, registry := range searchRegistries {
 			decomposedImage.registry = registry
 			imageName := decomposedImage.assembleWithTransport()
-			if hasShaInInputName(i.InputName) {
+			if hasShaInInputName(inputName) {
-				imageName = fmt.Sprintf("%s%s/%s", decomposedImage.transport, registry, i.InputName)
+				imageName = fmt.Sprintf("%s%s/%s", decomposedImage.transport, registry, inputName)
 			}
 			srcRef, err := alltransports.ParseImageName(imageName)
 			if err != nil {
-				return nil, errors.Wrapf(err, "unable to parse '%s'", i.InputName)
+				return nil, errors.Wrapf(err, "unable to parse '%s'", inputName)
 			}
 			ps := pullRefName{
 				image:  decomposedImage.assemble(),
@@ -332,7 +332,7 @@ func (i *Image) refNamesFromPossiblyUnqualifiedName() ([]*pullRefName, error) {
 // refPairsFromPossiblyUnqualifiedName looks at a decomposed image and determines the possible
 // image references to try pulling in combination with the registries.conf file as well
 func (i *Image) refPairsFromPossiblyUnqualifiedName() ([]*pullRefPair, error) {
-	refNames, err := i.refNamesFromPossiblyUnqualifiedName()
+	refNames, err := refNamesFromPossiblyUnqualifiedName(i.InputName)
 	if err != nil {
 		return nil, err
 	}

libpod/image/pull_test.go

@@ -0,0 +1,125 @@
+package image
+
+import (
+	"io/ioutil"
+	"os"
+	"testing"
+
+	"github.com/containers/image/transports"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+const registriesConfWithSearch = `[registries.search]
+registries = ['example.com', 'docker.io']
+`
+
+func TestRefNamesFromPossiblyUnqualifiedName(t *testing.T) {
+	const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+	type pullRefStrings struct{ image, srcRef, dstName string } // pullRefName with string data only
+
+	registriesConf, err := ioutil.TempFile("", "TestRefNamesFromPossiblyUnqualifiedName")
+	require.NoError(t, err)
+	defer registriesConf.Close()
+	defer os.Remove(registriesConf.Name())
+
+	err = ioutil.WriteFile(registriesConf.Name(), []byte(registriesConfWithSearch), 0600)
+	require.NoError(t, err)
+
+	// Environment is per-process, so this looks very unsafe; actually it seems fine because tests are not
+	// run in parallel unless they opt in by calling t.Parallel().  So don’t do that.
+	oldRCP, hasRCP := os.LookupEnv("REGISTRIES_CONFIG_PATH")
+	defer func() {
+		if hasRCP {
+			os.Setenv("REGISTRIES_CONFIG_PATH", oldRCP)
+		} else {
+			os.Unsetenv("REGISTRIES_CONFIG_PATH")
+		}
+	}()
+	os.Setenv("REGISTRIES_CONFIG_PATH", registriesConf.Name())
+
+	for _, c := range []struct {
+		input    string
+		expected []pullRefStrings
+	}{
+		{"#", nil}, // Clearly invalid.
+		{ // Fully-explicit docker.io, name-only.
+			"docker.io/library/busybox",
+			// (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".)
+			[]pullRefStrings{{"docker.io/library/busybox", "docker://busybox:latest", "docker.io/library/busybox"}},
+		},
+		{ // docker.io with implied /library/, name-only.
+			"docker.io/busybox",
+			// (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".)
+			// The .dstName fields differ for the explicit/implicit /library/ cases, but StorageTransport.ParseStoreReference normalizes that.
+			[]pullRefStrings{{"docker.io/busybox", "docker://busybox:latest", "docker.io/busybox"}},
+		},
+		{ // Qualified example.com, name-only.
+			"example.com/ns/busybox",
+			[]pullRefStrings{{"example.com/ns/busybox", "docker://example.com/ns/busybox:latest", "example.com/ns/busybox"}},
+		},
+		{ // Qualified example.com, name:tag.
+			"example.com/ns/busybox:notlatest",
+			[]pullRefStrings{{"example.com/ns/busybox:notlatest", "docker://example.com/ns/busybox:notlatest", "example.com/ns/busybox:notlatest"}},
+		},
+		{ // Qualified example.com, name@digest.
+			"example.com/ns/busybox" + digestSuffix,
+			[]pullRefStrings{{"example.com/ns/busybox" + digestSuffix, "docker://example.com/ns/busybox" + digestSuffix,
+				// FIXME?! Why is .dstName dropping the digest, and adding :none?!
+				"example.com/ns/busybox:none"}},
+		},
+		// Qualified example.com, name:tag@digest.  This code is happy to try, but .srcRef parsing currently rejects such input.
+		{"example.com/ns/busybox:notlatest" + digestSuffix, nil},
+		{ // Unqualified, single-name, name-only
+			"busybox",
+			[]pullRefStrings{
+				{"example.com/busybox:latest", "docker://example.com/busybox:latest", "example.com/busybox:latest"},
+				// (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".)
+				{"docker.io/busybox:latest", "docker://busybox:latest", "docker.io/busybox:latest"},
+			},
+		},
+		{ // Unqualified, namespaced, name-only
+			"ns/busybox",
+			// FIXME: This is interpreted as "registry == ns", and actual pull happens from docker.io/ns/busybox:latest;
+			// example.com should be first in the list but isn't used at all.
+			[]pullRefStrings{
+				{"ns/busybox", "docker://ns/busybox:latest", "ns/busybox"},
+			},
+		},
+		{ // Unqualified, name:tag
+			"busybox:notlatest",
+			[]pullRefStrings{
+				{"example.com/busybox:notlatest", "docker://example.com/busybox:notlatest", "example.com/busybox:notlatest"},
+				// (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".)
+				{"docker.io/busybox:notlatest", "docker://busybox:notlatest", "docker.io/busybox:notlatest"},
+			},
+		},
+		{ // Unqualified, name@digest
+			"busybox" + digestSuffix,
+			[]pullRefStrings{
+				// FIXME?! Why is .input and .dstName dropping the digest, and adding :none?!
+				{"example.com/busybox:none", "docker://example.com/busybox" + digestSuffix, "example.com/busybox:none"},
+				// (The docker:// representation is shortened by c/image/docker.Reference but it refers to "docker.io/library".)
+				{"docker.io/busybox:none", "docker://busybox" + digestSuffix, "docker.io/busybox:none"},
+			},
+		},
+		// Unqualified, name:tag@digest. This code is happy to try, but .srcRef parsing currently rejects such input.
+		{"busybox:notlatest" + digestSuffix, nil},
+	} {
+		res, err := refNamesFromPossiblyUnqualifiedName(c.input)
+		if len(c.expected) == 0 {
+			assert.Error(t, err, c.input)
+		} else {
+			assert.NoError(t, err, c.input)
+			strings := make([]pullRefStrings, len(res))
+			for i, rn := range res {
+				strings[i] = pullRefStrings{
+					image:   rn.image,
+					srcRef:  transports.ImageName(rn.srcRef),
+					dstName: rn.dstName,
+				}
+			}
+			assert.Equal(t, c.expected, strings, c.input)
+		}
+	}
+}