From 6486e2c41be0aa50bd5b70d859a23f36070d0ae1 Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Mon, 14 Jan 2019 03:58:35 +0100
Subject: Drop image.DecomposeString, make image.Parts private imageParts again
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Now that DecomposeString has no users, make the type private again.
Any new users of it should come with a rationale - and new users
of the "none"/"latest" handling of untagged/digested names that is
currently implemented should have an exceptionaly unusual rationale.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go | 36 +++++++++++++++---------------------
 1 file changed, 15 insertions(+), 21 deletions(-)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index b2a69f26c..9adf26fb9 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -7,12 +7,12 @@ import (
 	"github.com/containers/image/docker/reference"
 )
 
-// Parts describes the parts of an image's name
-type Parts struct {
+// imageParts describes the parts of an image's name
+type imageParts struct {
 	transport   string
-	Registry    string
+	registry    string
 	name        string
-	Tag         string
+	tag         string
 	isTagged    bool
 	hasRegistry bool
 }
@@ -34,16 +34,10 @@ func GetImageBaseName(input string) (string, error) {
 	return splitImageName[len(splitImageName)-1], nil
 }
 
-// DecomposeString decomposes a string name into imageParts description. This
-// is a wrapper for decompose
-func DecomposeString(input string) (Parts, error) {
-	return decompose(input)
-}
-
 // decompose breaks an input name into an imageParts description
-func decompose(input string) (Parts, error) {
+func decompose(input string) (imageParts, error) {
 	var (
-		parts       Parts
+		parts       imageParts
 		hasRegistry bool
 		tag         string
 	)
@@ -62,7 +56,7 @@ func decompose(input string) (Parts, error) {
 	}
 	registry := reference.Domain(imgRef.(reference.Named))
 	imageName := reference.Path(imgRef.(reference.Named))
-	// Is this a Registry or a repo?
+	// Is this a registry or a repo?
 	if isRegistry(registry) {
 		hasRegistry = true
 	} else {
@@ -71,27 +65,27 @@ func decompose(input string) (Parts, error) {
 			registry = ""
 		}
 	}
-	return Parts{
-		Registry:    registry,
+	return imageParts{
+		registry:    registry,
 		hasRegistry: hasRegistry,
 		name:        imageName,
-		Tag:         tag,
+		tag:         tag,
 		isTagged:    isTagged,
 		transport:   DefaultTransport,
 	}, nil
 }
 
 // assemble concatenates an image's parts into a string
-func (ip *Parts) assemble() string {
-	spec := fmt.Sprintf("%s:%s", ip.name, ip.Tag)
+func (ip *imageParts) assemble() string {
+	spec := fmt.Sprintf("%s:%s", ip.name, ip.tag)
 
-	if ip.Registry != "" {
-		spec = fmt.Sprintf("%s/%s", ip.Registry, spec)
+	if ip.registry != "" {
+		spec = fmt.Sprintf("%s/%s", ip.registry, spec)
 	}
 	return spec
 }
 
 // assemble concatenates an image's parts with transport into a string
-func (ip *Parts) assembleWithTransport() string {
+func (ip *imageParts) assembleWithTransport() string {
 	return fmt.Sprintf("%s%s", ip.transport, ip.assemble())
 }
-- 
cgit v1.2.3-54-g00ecf


From c19294c0113e4ddea7005bab5d8a31f7ecd6c120 Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 18:28:28 +0100
Subject: Record the original reference.Named in imageParts
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

We will eventually want to eliminate most members of imageParts
in favor of using the c/image/docker/reference API directly.

For now, just record the reference.Named value, and we will
replace uses of the other members before removing them.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index 9adf26fb9..289a02e1f 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -9,12 +9,13 @@ import (
 
 // imageParts describes the parts of an image's name
 type imageParts struct {
-	transport   string
-	registry    string
-	name        string
-	tag         string
-	isTagged    bool
-	hasRegistry bool
+	unnormalizedRef reference.Named // WARNING: Did not go through docker.io[/library] normalization
+	transport       string
+	registry        string
+	name            string
+	tag             string
+	isTagged        bool
+	hasRegistry     bool
 }
 
 // Registries must contain a ":" or a "." or be localhost
@@ -45,6 +46,7 @@ func decompose(input string) (imageParts, error) {
 	if err != nil {
 		return parts, err
 	}
+	unnormalizedNamed := imgRef.(reference.Named)
 	ntag, isTagged := imgRef.(reference.NamedTagged)
 	if !isTagged {
 		tag = "latest"
@@ -54,8 +56,8 @@ func decompose(input string) (imageParts, error) {
 	} else {
 		tag = ntag.Tag()
 	}
-	registry := reference.Domain(imgRef.(reference.Named))
-	imageName := reference.Path(imgRef.(reference.Named))
+	registry := reference.Domain(unnormalizedNamed)
+	imageName := reference.Path(unnormalizedNamed)
 	// Is this a registry or a repo?
 	if isRegistry(registry) {
 		hasRegistry = true
@@ -66,12 +68,13 @@ func decompose(input string) (imageParts, error) {
 		}
 	}
 	return imageParts{
-		registry:    registry,
-		hasRegistry: hasRegistry,
-		name:        imageName,
-		tag:         tag,
-		isTagged:    isTagged,
-		transport:   DefaultTransport,
+		unnormalizedRef: unnormalizedNamed,
+		registry:        registry,
+		hasRegistry:     hasRegistry,
+		name:            imageName,
+		tag:             tag,
+		isTagged:        isTagged,
+		transport:       DefaultTransport,
 	}, nil
 }
 
-- 
cgit v1.2.3-54-g00ecf


From 3d98c42a3f7822bf608834505adcffec7f75bf28 Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 18:47:09 +0100
Subject: Inline imageParts.assembleWithTransport into callers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

imageParts.transport is a constant, and the design of imageParts
is not transport-independent in any sense; we will want to eliminate
the transport member entirely.

As a first step, drop assembleWithTransport and inline an exact
equivalent into all callers.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go      |  5 -----
 libpod/image/parts_test.go | 18 ++++++++----------
 libpod/image/pull.go       |  4 ++--
 3 files changed, 10 insertions(+), 17 deletions(-)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index 289a02e1f..cf774030b 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -87,8 +87,3 @@ func (ip *imageParts) assemble() string {
 	}
 	return spec
 }
-
-// assemble concatenates an image's parts with transport into a string
-func (ip *imageParts) assembleWithTransport() string {
-	return fmt.Sprintf("%s%s", ip.transport, ip.assemble())
-}
diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go
index 518538f0b..3ff175f0e 100644
--- a/libpod/image/parts_test.go
+++ b/libpod/image/parts_test.go
@@ -14,39 +14,38 @@ func TestDecompose(t *testing.T) {
 		transport, registry, name, tag string
 		isTagged, hasRegistry          bool
 		assembled                      string
-		assembledWithTransport         string
 	}{
-		{"#", "", "", "", "", false, false, "", ""}, // Entirely invalid input
+		{"#", "", "", "", "", false, false, ""}, // Entirely invalid input
 		{ // Fully qualified docker.io, name-only input
 			"docker.io/library/busybox", "docker://", "docker.io", "library/busybox", "latest", false, true,
-			"docker.io/library/busybox:latest", "docker://docker.io/library/busybox:latest",
+			"docker.io/library/busybox:latest",
 		},
 		{ // Fully qualified example.com, name-only input
 			"example.com/ns/busybox", "docker://", "example.com", "ns/busybox", "latest", false, true,
-			"example.com/ns/busybox:latest", "docker://example.com/ns/busybox:latest",
+			"example.com/ns/busybox:latest",
 		},
 		{ // Unqualified single-name input
 			"busybox", "docker://", "", "busybox", "latest", false, false,
-			"busybox:latest", "docker://busybox:latest",
+			"busybox:latest",
 		},
 		{ // Unqualified namespaced input
 			"ns/busybox", "docker://", "", "ns/busybox", "latest", false, false,
-			"ns/busybox:latest", "docker://ns/busybox:latest",
+			"ns/busybox:latest",
 		},
 		{ // name:tag
 			"example.com/ns/busybox:notlatest", "docker://", "example.com", "ns/busybox", "notlatest", true, true,
-			"example.com/ns/busybox:notlatest", "docker://example.com/ns/busybox:notlatest",
+			"example.com/ns/busybox:notlatest",
 		},
 		{ // name@digest
 			// FIXME? .tag == "none"
 			"example.com/ns/busybox" + digestSuffix, "docker://", "example.com", "ns/busybox", "none", false, true,
 			// FIXME: this drops the digest and replaces it with an incorrect tag.
-			"example.com/ns/busybox:none", "docker://example.com/ns/busybox:none",
+			"example.com/ns/busybox:none",
 		},
 		{ // name:tag@digest
 			"example.com/ns/busybox:notlatest" + digestSuffix, "docker://", "example.com", "ns/busybox", "notlatest", true, true,
 			// FIXME: This drops the digest
-			"example.com/ns/busybox:notlatest", "docker://example.com/ns/busybox:notlatest",
+			"example.com/ns/busybox:notlatest",
 		},
 	} {
 		parts, err := decompose(c.input)
@@ -61,7 +60,6 @@ func TestDecompose(t *testing.T) {
 			assert.Equal(t, c.isTagged, parts.isTagged, c.input)
 			assert.Equal(t, c.hasRegistry, parts.hasRegistry, c.input)
 			assert.Equal(t, c.assembled, parts.assemble(), c.input)
-			assert.Equal(t, c.assembledWithTransport, parts.assembleWithTransport(), c.input)
 		}
 	}
 }
diff --git a/libpod/image/pull.go b/libpod/image/pull.go
index 09935fe7c..82fe0fdd6 100644
--- a/libpod/image/pull.go
+++ b/libpod/image/pull.go
@@ -288,7 +288,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG
 		if hasShaInInputName(inputName) {
 			imageName = fmt.Sprintf("%s%s", decomposedImage.transport, inputName)
 		} else {
-			imageName = decomposedImage.assembleWithTransport()
+			imageName = fmt.Sprintf("%s%s", decomposedImage.transport, decomposedImage.assemble())
 		}
 		srcRef, err := alltransports.ParseImageName(imageName)
 		if err != nil {
@@ -318,7 +318,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG
 	var refPairs []pullRefPair
 	for _, registry := range searchRegistries {
 		decomposedImage.registry = registry
-		imageName := decomposedImage.assembleWithTransport()
+		imageName := fmt.Sprintf("%s%s", decomposedImage.transport, decomposedImage.assemble())
 		if hasShaInInputName(inputName) {
 			imageName = fmt.Sprintf("%s%s/%s", decomposedImage.transport, registry, inputName)
 		}
-- 
cgit v1.2.3-54-g00ecf


From e9721b757ae5da4dad4f68c5eb721f348cfe935a Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 19:00:59 +0100
Subject: Remove imageParts.transport
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It is only ever set to DefaulTransport, and all of the code
is docker/reference-specific anyway, so there's no point in
making this a variable.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go      |  2 --
 libpod/image/parts_test.go | 27 +++++++++++++--------------
 libpod/image/pull.go       |  4 ++--
 3 files changed, 15 insertions(+), 18 deletions(-)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index cf774030b..b7c5d1252 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -10,7 +10,6 @@ import (
 // imageParts describes the parts of an image's name
 type imageParts struct {
 	unnormalizedRef reference.Named // WARNING: Did not go through docker.io[/library] normalization
-	transport       string
 	registry        string
 	name            string
 	tag             string
@@ -74,7 +73,6 @@ func decompose(input string) (imageParts, error) {
 		name:            imageName,
 		tag:             tag,
 		isTagged:        isTagged,
-		transport:       DefaultTransport,
 	}, nil
 }
 
diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go
index 3ff175f0e..733e8e855 100644
--- a/libpod/image/parts_test.go
+++ b/libpod/image/parts_test.go
@@ -10,50 +10,49 @@ func TestDecompose(t *testing.T) {
 	const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
 
 	for _, c := range []struct {
-		input                          string
-		transport, registry, name, tag string
-		isTagged, hasRegistry          bool
-		assembled                      string
+		input                 string
+		registry, name, tag   string
+		isTagged, hasRegistry bool
+		assembled             string
 	}{
-		{"#", "", "", "", "", false, false, ""}, // Entirely invalid input
+		{"#", "", "", "", false, false, ""}, // Entirely invalid input
 		{ // Fully qualified docker.io, name-only input
-			"docker.io/library/busybox", "docker://", "docker.io", "library/busybox", "latest", false, true,
+			"docker.io/library/busybox", "docker.io", "library/busybox", "latest", false, true,
 			"docker.io/library/busybox:latest",
 		},
 		{ // Fully qualified example.com, name-only input
-			"example.com/ns/busybox", "docker://", "example.com", "ns/busybox", "latest", false, true,
+			"example.com/ns/busybox", "example.com", "ns/busybox", "latest", false, true,
 			"example.com/ns/busybox:latest",
 		},
 		{ // Unqualified single-name input
-			"busybox", "docker://", "", "busybox", "latest", false, false,
+			"busybox", "", "busybox", "latest", false, false,
 			"busybox:latest",
 		},
 		{ // Unqualified namespaced input
-			"ns/busybox", "docker://", "", "ns/busybox", "latest", false, false,
+			"ns/busybox", "", "ns/busybox", "latest", false, false,
 			"ns/busybox:latest",
 		},
 		{ // name:tag
-			"example.com/ns/busybox:notlatest", "docker://", "example.com", "ns/busybox", "notlatest", true, true,
+			"example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true, true,
 			"example.com/ns/busybox:notlatest",
 		},
 		{ // name@digest
 			// FIXME? .tag == "none"
-			"example.com/ns/busybox" + digestSuffix, "docker://", "example.com", "ns/busybox", "none", false, true,
+			"example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", false, true,
 			// FIXME: this drops the digest and replaces it with an incorrect tag.
 			"example.com/ns/busybox:none",
 		},
 		{ // name:tag@digest
-			"example.com/ns/busybox:notlatest" + digestSuffix, "docker://", "example.com", "ns/busybox", "notlatest", true, true,
+			"example.com/ns/busybox:notlatest" + digestSuffix, "example.com", "ns/busybox", "notlatest", true, true,
 			// FIXME: This drops the digest
 			"example.com/ns/busybox:notlatest",
 		},
 	} {
 		parts, err := decompose(c.input)
-		if c.transport == "" {
+		if c.assembled == "" {
 			assert.Error(t, err, c.input)
 		} else {
 			assert.NoError(t, err, c.input)
-			assert.Equal(t, c.transport, parts.transport, c.input)
 			assert.Equal(t, c.registry, parts.registry, c.input)
 			assert.Equal(t, c.name, parts.name, c.input)
 			assert.Equal(t, c.tag, parts.tag, c.input)
diff --git a/libpod/image/pull.go b/libpod/image/pull.go
index d09ddd599..342ee6f17 100644
--- a/libpod/image/pull.go
+++ b/libpod/image/pull.go
@@ -290,7 +290,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG
 		} else {
 			imageName = decomposedImage.assemble()
 		}
-		srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", decomposedImage.transport, imageName))
+		srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", DefaultTransport, imageName))
 		if err != nil {
 			return nil, errors.Wrapf(err, "unable to parse '%s'", inputName)
 		}
@@ -322,7 +322,7 @@ func (ir *Runtime) pullGoalFromPossiblyUnqualifiedName(inputName string) (*pullG
 		if hasShaInInputName(inputName) {
 			imageName = fmt.Sprintf("%s/%s", registry, inputName)
 		}
-		srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", decomposedImage.transport, imageName))
+		srcRef, err := alltransports.ParseImageName(fmt.Sprintf("%s%s", DefaultTransport, imageName))
 		if err != nil {
 			return nil, errors.Wrapf(err, "unable to parse '%s'", inputName)
 		}
-- 
cgit v1.2.3-54-g00ecf


From 72777b7fee22e04edee08034927d5864ffc4bc3e Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 20:32:30 +0100
Subject: Add imageParts.referenceWithRegistry
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This is the primary goal of decompose()+assemble(), to support
qualifying an image name.

Does not have any users yet, so does not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go      | 18 ++++++++++++++++++
 libpod/image/parts_test.go | 42 ++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index b7c5d1252..8d059e35b 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -5,6 +5,7 @@ import (
 	"strings"
 
 	"github.com/containers/image/docker/reference"
+	"github.com/pkg/errors"
 )
 
 // imageParts describes the parts of an image's name
@@ -76,6 +77,23 @@ func decompose(input string) (imageParts, error) {
 	}, nil
 }
 
+// referenceWithRegistry returns a (normalized) reference.Named composed of ip (with !ip.hasRegistry)
+// qualified with registry.
+func (ip *imageParts) referenceWithRegistry(registry string) (reference.Named, error) {
+	if ip.hasRegistry {
+		return nil, errors.Errorf("internal error: referenceWithRegistry called on imageParts with a registry (%#v)", *ip)
+	}
+	// We could build a reference.WithName+WithTag/WithDigest here, but we need to round-trip via a string
+	// and a ParseNormalizedNamed anyway to get the right normalization of docker.io/library, so
+	// just use a string directly.
+	qualified := registry + "/" + ip.unnormalizedRef.String()
+	ref, err := reference.ParseNormalizedNamed(qualified)
+	if err != nil {
+		return nil, errors.Wrapf(err, "error normalizing registry+unqualified reference %#v", qualified)
+	}
+	return ref, nil
+}
+
 // assemble concatenates an image's parts into a string
 func (ip *imageParts) assemble() string {
 	spec := fmt.Sprintf("%s:%s", ip.name, ip.tag)
diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go
index 733e8e855..b2cc0f8c0 100644
--- a/libpod/image/parts_test.go
+++ b/libpod/image/parts_test.go
@@ -4,6 +4,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 )
 
 func TestDecompose(t *testing.T) {
@@ -62,3 +63,44 @@ func TestDecompose(t *testing.T) {
 		}
 	}
 }
+
+func TestImagePartsReferenceWithRegistry(t *testing.T) {
+	const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+
+	for _, c := range []struct {
+		input                     string
+		withDocker, withNonDocker string
+	}{
+		{"example.com/ns/busybox", "", ""},                                                                            // Fully-qualified input is invalid.
+		{"busybox", "docker.io/library/busybox", "example.com/busybox"},                                               // Single-name input
+		{"ns/busybox", "docker.io/ns/busybox", "example.com/ns/busybox"},                                              // Namespaced input
+		{"ns/busybox:notlatest", "docker.io/ns/busybox:notlatest", "example.com/ns/busybox:notlatest"},                // name:tag
+		{"ns/busybox" + digestSuffix, "docker.io/ns/busybox" + digestSuffix, "example.com/ns/busybox" + digestSuffix}, // name@digest
+		{ // name:tag@digest
+			"ns/busybox:notlatest" + digestSuffix,
+			"docker.io/ns/busybox:notlatest" + digestSuffix, "example.com/ns/busybox:notlatest" + digestSuffix,
+		},
+	} {
+		parts, err := decompose(c.input)
+		require.NoError(t, err)
+		if c.withDocker == "" {
+			_, err := parts.referenceWithRegistry("docker.io")
+			assert.Error(t, err, c.input)
+			_, err = parts.referenceWithRegistry("example.com")
+			assert.Error(t, err, c.input)
+		} else {
+			ref, err := parts.referenceWithRegistry("docker.io")
+			require.NoError(t, err, c.input)
+			assert.Equal(t, c.withDocker, ref.String())
+			ref, err = parts.referenceWithRegistry("example.com")
+			require.NoError(t, err, c.input)
+			assert.Equal(t, c.withNonDocker, ref.String())
+		}
+	}
+
+	// Invalid registry value
+	parts, err := decompose("busybox")
+	require.NoError(t, err)
+	_, err = parts.referenceWithRegistry("invalid@domain")
+	assert.Error(t, err)
+}
-- 
cgit v1.2.3-54-g00ecf


From e5c764ec3cc11982cdb8e127554e4ac61ce9a854 Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 21:11:32 +0100
Subject: Remove no longer used imageParts.assemble()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go      | 11 -----------
 libpod/image/parts_test.go | 15 ++-------------
 2 files changed, 2 insertions(+), 24 deletions(-)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index 8d059e35b..566a3842c 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -1,7 +1,6 @@
 package image
 
 import (
-	"fmt"
 	"strings"
 
 	"github.com/containers/image/docker/reference"
@@ -93,13 +92,3 @@ func (ip *imageParts) referenceWithRegistry(registry string) (reference.Named, e
 	}
 	return ref, nil
 }
-
-// assemble concatenates an image's parts into a string
-func (ip *imageParts) assemble() string {
-	spec := fmt.Sprintf("%s:%s", ip.name, ip.tag)
-
-	if ip.registry != "" {
-		spec = fmt.Sprintf("%s/%s", ip.registry, spec)
-	}
-	return spec
-}
diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go
index b2cc0f8c0..3ca39b1dc 100644
--- a/libpod/image/parts_test.go
+++ b/libpod/image/parts_test.go
@@ -14,43 +14,33 @@ func TestDecompose(t *testing.T) {
 		input                 string
 		registry, name, tag   string
 		isTagged, hasRegistry bool
-		assembled             string
 	}{
-		{"#", "", "", "", false, false, ""}, // Entirely invalid input
+		{"#", "", "", "", false, false}, // Entirely invalid input
 		{ // Fully qualified docker.io, name-only input
 			"docker.io/library/busybox", "docker.io", "library/busybox", "latest", false, true,
-			"docker.io/library/busybox:latest",
 		},
 		{ // Fully qualified example.com, name-only input
 			"example.com/ns/busybox", "example.com", "ns/busybox", "latest", false, true,
-			"example.com/ns/busybox:latest",
 		},
 		{ // Unqualified single-name input
 			"busybox", "", "busybox", "latest", false, false,
-			"busybox:latest",
 		},
 		{ // Unqualified namespaced input
 			"ns/busybox", "", "ns/busybox", "latest", false, false,
-			"ns/busybox:latest",
 		},
 		{ // name:tag
 			"example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true, true,
-			"example.com/ns/busybox:notlatest",
 		},
 		{ // name@digest
 			// FIXME? .tag == "none"
 			"example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", false, true,
-			// FIXME: this drops the digest and replaces it with an incorrect tag.
-			"example.com/ns/busybox:none",
 		},
 		{ // name:tag@digest
 			"example.com/ns/busybox:notlatest" + digestSuffix, "example.com", "ns/busybox", "notlatest", true, true,
-			// FIXME: This drops the digest
-			"example.com/ns/busybox:notlatest",
 		},
 	} {
 		parts, err := decompose(c.input)
-		if c.assembled == "" {
+		if c.name == "" {
 			assert.Error(t, err, c.input)
 		} else {
 			assert.NoError(t, err, c.input)
@@ -59,7 +49,6 @@ func TestDecompose(t *testing.T) {
 			assert.Equal(t, c.tag, parts.tag, c.input)
 			assert.Equal(t, c.isTagged, parts.isTagged, c.input)
 			assert.Equal(t, c.hasRegistry, parts.hasRegistry, c.input)
-			assert.Equal(t, c.assembled, parts.assemble(), c.input)
 		}
 	}
 }
-- 
cgit v1.2.3-54-g00ecf


From 1c19d19c6ef9627413e34b30a643bef9b2970acb Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 21:33:01 +0100
Subject: Add imageParts.normalizedReference()
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

This will be used in normalizeTag to work with references instead of strings.

Not used anywhere yet, should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go      | 14 ++++++++++++++
 libpod/image/parts_test.go | 28 ++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index 566a3842c..a4eb57519 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -92,3 +92,17 @@ func (ip *imageParts) referenceWithRegistry(registry string) (reference.Named, e
 	}
 	return ref, nil
 }
+
+// normalizedReference returns a (normalized) reference for ip (with ip.hasRegistry)
+func (ip *imageParts) normalizedReference() (reference.Named, error) {
+	if !ip.hasRegistry {
+		return nil, errors.Errorf("internal error: normalizedReference called on imageParts without a registry (%#v)", *ip)
+	}
+	// We need to round-trip via a string to get the right normalization of docker.io/library
+	s := ip.unnormalizedRef.String()
+	ref, err := reference.ParseNormalizedNamed(s)
+	if err != nil { // Should never happen
+		return nil, errors.Wrapf(err, "error normalizing qualified reference %#v", s)
+	}
+	return ref, nil
+}
diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go
index 3ca39b1dc..9955a20ea 100644
--- a/libpod/image/parts_test.go
+++ b/libpod/image/parts_test.go
@@ -93,3 +93,31 @@ func TestImagePartsReferenceWithRegistry(t *testing.T) {
 	_, err = parts.referenceWithRegistry("invalid@domain")
 	assert.Error(t, err)
 }
+
+func TestImagePartsNormalizedReference(t *testing.T) {
+	const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+
+	for _, c := range []struct{ input, expected string }{
+		{"busybox", ""}, // Unqualified input is invalid
+		{"docker.io/busybox", "docker.io/library/busybox"},                                 // docker.io single-name
+		{"example.com/busybox", "example.com/busybox"},                                     // example.com single-name
+		{"docker.io/ns/busybox", "docker.io/ns/busybox"},                                   // docker.io namespaced
+		{"example.com/ns/busybox", "example.com/ns/busybox"},                               // example.com namespaced
+		{"example.com/ns/busybox:notlatest", "example.com/ns/busybox:notlatest"},           // name:tag
+		{"example.com/ns/busybox" + digestSuffix, "example.com/ns/busybox" + digestSuffix}, // name@digest
+		{ // name:tag@digest
+			"example.com/ns/busybox:notlatest" + digestSuffix, "example.com/ns/busybox:notlatest" + digestSuffix,
+		},
+	} {
+		parts, err := decompose(c.input)
+		require.NoError(t, err)
+		if c.expected == "" {
+			_, err := parts.normalizedReference()
+			assert.Error(t, err, c.input)
+		} else {
+			ref, err := parts.normalizedReference()
+			require.NoError(t, err, c.input)
+			assert.Equal(t, c.expected, ref.String())
+		}
+	}
+}
-- 
cgit v1.2.3-54-g00ecf


From fa42f97507f5b5c661df2339603383b00293689f Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 19:41:50 +0100
Subject: FIXME? Introduce imageParts.suspiciousRefNameTagValuesForSearch
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Image.MatchRepoTag and findImageInRepoTags do some kind of
heuristic search; the motivation and design of both, and how they
should deal with digests, is not obvious to me.

Instead of figuring that out now, just factor it out into a
scary-named method and leave the "tag" value (with its "latest"/"none"
value) alone.

Similarly, the .registry and .name fields should typically not be used;
users should use either hasRegistry or normalized reference types;
so, isolate the difficult-to-understand search code, and computation
of these values, into this new search-specific helper.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/image.go      | 10 ++++++----
 libpod/image/parts.go      | 24 ++++++++++++++++++++++++
 libpod/image/parts_test.go | 15 ++++++++-------
 libpod/image/utils.go      | 10 ++++++----
 4 files changed, 44 insertions(+), 15 deletions(-)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/image.go b/libpod/image/image.go
index 381c28fac..ea326d820 100644
--- a/libpod/image/image.go
+++ b/libpod/image/image.go
@@ -930,21 +930,23 @@ func (i *Image) MatchRepoTag(input string) (string, error) {
 	if err != nil {
 		return "", err
 	}
+	imageRegistry, imageName, imageSuspiciousTagValueForSearch := dcImage.suspiciousRefNameTagValuesForSearch()
 	for _, repoName := range i.Names() {
 		count := 0
 		dcRepoName, err := decompose(repoName)
 		if err != nil {
 			return "", err
 		}
-		if dcRepoName.registry == dcImage.registry && dcImage.registry != "" {
+		repoNameRegistry, repoNameName, repoNameSuspiciousTagValueForSearch := dcRepoName.suspiciousRefNameTagValuesForSearch()
+		if repoNameRegistry == imageRegistry && imageRegistry != "" {
 			count++
 		}
-		if dcRepoName.name == dcImage.name && dcImage.name != "" {
+		if repoNameName == imageName && imageName != "" {
 			count++
-		} else if splitString(dcRepoName.name) == splitString(dcImage.name) {
+		} else if splitString(repoNameName) == splitString(imageName) {
 			count++
 		}
-		if dcRepoName.tag == dcImage.tag {
+		if repoNameSuspiciousTagValueForSearch == imageSuspiciousTagValueForSearch {
 			count++
 		}
 		results[count] = append(results[count], repoName)
diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index a4eb57519..ab6a86767 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -76,6 +76,30 @@ func decompose(input string) (imageParts, error) {
 	}, nil
 }
 
+// suspiciousRefNameTagValuesForSearch returns a "tag" value used in a previous implementation.
+// This exists only to preserve existing behavior in heuristic code; it’s dubious that that behavior is correct,
+// gespecially for the tag value.
+func (ip *imageParts) suspiciousRefNameTagValuesForSearch() (string, string, string) {
+	registry := reference.Domain(ip.unnormalizedRef)
+	imageName := reference.Path(ip.unnormalizedRef)
+	// ip.unnormalizedRef, because it uses reference.Parse and not reference.ParseNormalizedNamed,
+	// does not use the standard heuristics for domains vs. namespaces/repos.
+	if registry != "" && !isRegistry(registry) {
+		imageName = registry + "/" + imageName
+		registry = ""
+	}
+
+	var tag string
+	if tagged, isTagged := ip.unnormalizedRef.(reference.NamedTagged); isTagged {
+		tag = tagged.Tag()
+	} else if _, hasDigest := ip.unnormalizedRef.(reference.Digested); hasDigest {
+		tag = "none"
+	} else {
+		tag = "latest"
+	}
+	return registry, imageName, tag
+}
+
 // referenceWithRegistry returns a (normalized) reference.Named composed of ip (with !ip.hasRegistry)
 // qualified with registry.
 func (ip *imageParts) referenceWithRegistry(registry string) (reference.Named, error) {
diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go
index 9955a20ea..dd4a7b27f 100644
--- a/libpod/image/parts_test.go
+++ b/libpod/image/parts_test.go
@@ -11,9 +11,9 @@ func TestDecompose(t *testing.T) {
 	const digestSuffix = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
 
 	for _, c := range []struct {
-		input                 string
-		registry, name, tag   string
-		isTagged, hasRegistry bool
+		input                                       string
+		registry, name, suspiciousTagValueForSearch string
+		isTagged, hasRegistry                       bool
 	}{
 		{"#", "", "", "", false, false}, // Entirely invalid input
 		{ // Fully qualified docker.io, name-only input
@@ -32,7 +32,7 @@ func TestDecompose(t *testing.T) {
 			"example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true, true,
 		},
 		{ // name@digest
-			// FIXME? .tag == "none"
+			// FIXME? .suspiciousTagValueForSearch == "none"
 			"example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", false, true,
 		},
 		{ // name:tag@digest
@@ -44,9 +44,10 @@ func TestDecompose(t *testing.T) {
 			assert.Error(t, err, c.input)
 		} else {
 			assert.NoError(t, err, c.input)
-			assert.Equal(t, c.registry, parts.registry, c.input)
-			assert.Equal(t, c.name, parts.name, c.input)
-			assert.Equal(t, c.tag, parts.tag, c.input)
+			registry, name, suspiciousTagValueForSearch := parts.suspiciousRefNameTagValuesForSearch()
+			assert.Equal(t, c.registry, registry, c.input)
+			assert.Equal(t, c.name, name, c.input)
+			assert.Equal(t, c.suspiciousTagValueForSearch, suspiciousTagValueForSearch, c.input)
 			assert.Equal(t, c.isTagged, parts.isTagged, c.input)
 			assert.Equal(t, c.hasRegistry, parts.hasRegistry, c.input)
 		}
diff --git a/libpod/image/utils.go b/libpod/image/utils.go
index b944de1bb..ad027f32a 100644
--- a/libpod/image/utils.go
+++ b/libpod/image/utils.go
@@ -17,6 +17,7 @@ import (
 // findImageInRepotags takes an imageParts struct and searches images' repotags for
 // a match on name:tag
 func findImageInRepotags(search imageParts, images []*Image) (*storage.Image, error) {
+	_, searchName, searchSuspiciousTagValueForSearch := search.suspiciousRefNameTagValuesForSearch()
 	var results []*storage.Image
 	for _, image := range images {
 		for _, name := range image.Names() {
@@ -25,21 +26,22 @@ func findImageInRepotags(search imageParts, images []*Image) (*storage.Image, er
 			if err != nil {
 				continue
 			}
-			if d.name == search.name && d.tag == search.tag {
+			_, dName, dSuspiciousTagValueForSearch := d.suspiciousRefNameTagValuesForSearch()
+			if dName == searchName && dSuspiciousTagValueForSearch == searchSuspiciousTagValueForSearch {
 				results = append(results, image.image)
 				continue
 			}
 			// account for registry:/somedir/image
-			if strings.HasSuffix(d.name, search.name) && d.tag == search.tag {
+			if strings.HasSuffix(dName, searchName) && dSuspiciousTagValueForSearch == searchSuspiciousTagValueForSearch {
 				results = append(results, image.image)
 				continue
 			}
 		}
 	}
 	if len(results) == 0 {
-		return &storage.Image{}, errors.Errorf("unable to find a name and tag match for %s in repotags", search.name)
+		return &storage.Image{}, errors.Errorf("unable to find a name and tag match for %s in repotags", searchName)
 	} else if len(results) > 1 {
-		return &storage.Image{}, errors.Errorf("found multiple name and tag matches for %s in repotags", search.name)
+		return &storage.Image{}, errors.Errorf("found multiple name and tag matches for %s in repotags", searchName)
 	}
 	return results[0], nil
 }
-- 
cgit v1.2.3-54-g00ecf


From ad90c44f8d648ba17f07db7f05f28a67c16796d0 Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 22:10:53 +0100
Subject: Use imageParts.unnormalizedRef in GetImageBaseName
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

... to remove the last user of imageParts.name.

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index ab6a86767..b5b05c32d 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -30,7 +30,7 @@ func GetImageBaseName(input string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	splitImageName := strings.Split(decomposedImage.name, "/")
+	splitImageName := strings.Split(decomposedImage.unnormalizedRef.Name(), "/")
 	return splitImageName[len(splitImageName)-1], nil
 }
 
-- 
cgit v1.2.3-54-g00ecf


From 797d194050d4b7b78d255d19f4099463dc048425 Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 22:18:25 +0100
Subject: Clarify comments about isRegistry a bit.
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index b5b05c32d..62cca2757 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -17,7 +17,8 @@ type imageParts struct {
 	hasRegistry     bool
 }
 
-// Registries must contain a ":" or a "." or be localhost
+// Registries must contain a ":" or a "." or be localhost; this helper exists for users of reference.Parse.
+// For inputs that should use the docker.io[/library] normalization, use reference.ParseNormalizedNamed instead.
 func isRegistry(name string) bool {
 	return strings.ContainsAny(name, ".:") || name == "localhost"
 }
@@ -57,7 +58,9 @@ func decompose(input string) (imageParts, error) {
 	}
 	registry := reference.Domain(unnormalizedNamed)
 	imageName := reference.Path(unnormalizedNamed)
-	// Is this a registry or a repo?
+	// ip.unnormalizedRef, because it uses reference.Parse and not reference.ParseNormalizedNamed,
+	// does not use the standard heuristics for domains vs. namespaces/repos, so we need to check
+	// explicitly.
 	if isRegistry(registry) {
 		hasRegistry = true
 	} else {
-- 
cgit v1.2.3-54-g00ecf


From 449116af19305a567933effaced5a0a21a8c71be Mon Sep 17 00:00:00 2001
From: Miloslav Trmač <mitr@redhat.com>
Date: Wed, 9 Jan 2019 21:47:50 +0100
Subject: Remove imageParts.{isTagged,registry,name,tag}
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Finally, these members no longer have any users.

Future users should usually call referenceWithRegistry / normalizedReference,
and work with the returned value, instead of reintroducing these variables.
Similarly, direct uses of unnormalizedRef should be rare (only for cases
where the registry and/or path truly does not matter).

Should not change behavior.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>
---
 libpod/image/parts.go      | 35 ++---------------------------------
 libpod/image/parts_test.go | 19 +++++++++----------
 2 files changed, 11 insertions(+), 43 deletions(-)

(limited to 'libpod/image/parts.go')

diff --git a/libpod/image/parts.go b/libpod/image/parts.go
index 62cca2757..dfdf0b08a 100644
--- a/libpod/image/parts.go
+++ b/libpod/image/parts.go
@@ -10,10 +10,6 @@ import (
 // imageParts describes the parts of an image's name
 type imageParts struct {
 	unnormalizedRef reference.Named // WARNING: Did not go through docker.io[/library] normalization
-	registry        string
-	name            string
-	tag             string
-	isTagged        bool
 	hasRegistry     bool
 }
 
@@ -37,45 +33,18 @@ func GetImageBaseName(input string) (string, error) {
 
 // decompose breaks an input name into an imageParts description
 func decompose(input string) (imageParts, error) {
-	var (
-		parts       imageParts
-		hasRegistry bool
-		tag         string
-	)
 	imgRef, err := reference.Parse(input)
 	if err != nil {
-		return parts, err
+		return imageParts{}, err
 	}
 	unnormalizedNamed := imgRef.(reference.Named)
-	ntag, isTagged := imgRef.(reference.NamedTagged)
-	if !isTagged {
-		tag = "latest"
-		if _, hasDigest := imgRef.(reference.Digested); hasDigest {
-			tag = "none"
-		}
-	} else {
-		tag = ntag.Tag()
-	}
-	registry := reference.Domain(unnormalizedNamed)
-	imageName := reference.Path(unnormalizedNamed)
 	// ip.unnormalizedRef, because it uses reference.Parse and not reference.ParseNormalizedNamed,
 	// does not use the standard heuristics for domains vs. namespaces/repos, so we need to check
 	// explicitly.
-	if isRegistry(registry) {
-		hasRegistry = true
-	} else {
-		if registry != "" {
-			imageName = registry + "/" + imageName
-			registry = ""
-		}
-	}
+	hasRegistry := isRegistry(reference.Domain(unnormalizedNamed))
 	return imageParts{
 		unnormalizedRef: unnormalizedNamed,
-		registry:        registry,
 		hasRegistry:     hasRegistry,
-		name:            imageName,
-		tag:             tag,
-		isTagged:        isTagged,
 	}, nil
 }
 
diff --git a/libpod/image/parts_test.go b/libpod/image/parts_test.go
index dd4a7b27f..726e55e86 100644
--- a/libpod/image/parts_test.go
+++ b/libpod/image/parts_test.go
@@ -13,30 +13,30 @@ func TestDecompose(t *testing.T) {
 	for _, c := range []struct {
 		input                                       string
 		registry, name, suspiciousTagValueForSearch string
-		isTagged, hasRegistry                       bool
+		hasRegistry                                 bool
 	}{
-		{"#", "", "", "", false, false}, // Entirely invalid input
+		{"#", "", "", "", false}, // Entirely invalid input
 		{ // Fully qualified docker.io, name-only input
-			"docker.io/library/busybox", "docker.io", "library/busybox", "latest", false, true,
+			"docker.io/library/busybox", "docker.io", "library/busybox", "latest", true,
 		},
 		{ // Fully qualified example.com, name-only input
-			"example.com/ns/busybox", "example.com", "ns/busybox", "latest", false, true,
+			"example.com/ns/busybox", "example.com", "ns/busybox", "latest", true,
 		},
 		{ // Unqualified single-name input
-			"busybox", "", "busybox", "latest", false, false,
+			"busybox", "", "busybox", "latest", false,
 		},
 		{ // Unqualified namespaced input
-			"ns/busybox", "", "ns/busybox", "latest", false, false,
+			"ns/busybox", "", "ns/busybox", "latest", false,
 		},
 		{ // name:tag
-			"example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true, true,
+			"example.com/ns/busybox:notlatest", "example.com", "ns/busybox", "notlatest", true,
 		},
 		{ // name@digest
 			// FIXME? .suspiciousTagValueForSearch == "none"
-			"example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", false, true,
+			"example.com/ns/busybox" + digestSuffix, "example.com", "ns/busybox", "none", true,
 		},
 		{ // name:tag@digest
-			"example.com/ns/busybox:notlatest" + digestSuffix, "example.com", "ns/busybox", "notlatest", true, true,
+			"example.com/ns/busybox:notlatest" + digestSuffix, "example.com", "ns/busybox", "notlatest", true,
 		},
 	} {
 		parts, err := decompose(c.input)
@@ -48,7 +48,6 @@ func TestDecompose(t *testing.T) {
 			assert.Equal(t, c.registry, registry, c.input)
 			assert.Equal(t, c.name, name, c.input)
 			assert.Equal(t, c.suspiciousTagValueForSearch, suspiciousTagValueForSearch, c.input)
-			assert.Equal(t, c.isTagged, parts.isTagged, c.input)
 			assert.Equal(t, c.hasRegistry, parts.hasRegistry, c.input)
 		}
 	}
-- 
cgit v1.2.3-54-g00ecf