Support docker reference with both tag and digest.

Refers to containers#783.

Changes:
- Move the code that returns an error when both tag and digest are
  present from `newReference` to `reference.newImageDestination`
  allowing pulls but no pushes.
- Use digest instead of tag as `DockerReferenceIdentity` or rather
  for policy matching.
- Adjusted tests.

docker/docker_image_dest.go

@@ -36,6 +36,15 @@ type dockerImageDestination struct {
 
 // newImageDestination creates a new ImageDestination for the specified image reference.
 func newImageDestination(sys *types.SystemContext, ref dockerReference) (types.ImageDestination, error) {
+	// A github.com/distribution/reference value can have a tag and a digest at the same time!
+	// While some tools (namely skaffold) rely on this docker behaviour to pull the correct image
+	// it is not supported to push an image using such a reference.
+	_, isTagged := ref.ref.(reference.NamedTagged)
+	_, isDigested := ref.ref.(reference.Canonical)
+	if isTagged && isDigested {
+		return nil, errors.Errorf("Docker reference with both a tag and digest is not supported as image destination")
+	}
+
 	c, err := newDockerClientFromRef(sys, ref, true, "pull,push")
 	if err != nil {
 		return nil, err

docker/docker_transport.go

@@ -69,17 +69,6 @@ func newReference(ref reference.Named) (dockerReference, error) {
 	if reference.IsNameOnly(ref) {
 		return dockerReference{}, errors.Errorf("Docker reference %s has neither a tag nor a digest", reference.FamiliarString(ref))
 	}
-	// A github.com/distribution/reference value can have a tag and a digest at the same time!
-	// The docker/distribution API does not really support that (we can’t ask for an image with a specific
-	// tag and digest), so fail.  This MAY be accepted in the future.
-	// (Even if it were supported, the semantics of policy namespaces are unclear - should we drop
-	// the tag or the digest first?)
-	_, isTagged := ref.(reference.NamedTagged)
-	_, isDigested := ref.(reference.Canonical)
-	if isTagged && isDigested {
-		return dockerReference{}, errors.Errorf("Docker references with both a tag and digest are currently not supported")
-	}
-
 	return dockerReference{
 		ref: ref,
 	}, nil

docker/docker_transport_test.go

@@ -27,6 +27,7 @@ func TestTransportValidatePolicyConfigurationScope(t *testing.T) {
 	for _, scope := range []string{
 		"docker.io/library/busybox" + sha256digest,
 		"docker.io/library/busybox:notlatest",
+		"docker.io/library/busybox:latest" + sha256digest,
 		"docker.io/library/busybox",
 		"docker.io/library",
 		"docker.io",
@@ -48,11 +49,10 @@ func testParseReference(t *testing.T, fn func(string) (types.ImageReference, err
 		{"//busybox" + sha256digest, "docker.io/library/busybox" + sha256digest}, // Explicit digest
 		{"//busybox", "docker.io/library/busybox:latest"},                        // Default tag
 		// A github.com/distribution/reference value can have a tag and a digest at the same time!
-		// The docker/distribution API does not really support that (we can’t ask for an image with a specific
-		// tag and digest), so fail.  This MAY be accepted in the future.
-		{"//busybox:latest" + sha256digest, ""},                                    // Both tag and digest
-		{"//docker.io/library/busybox:latest", "docker.io/library/busybox:latest"}, // All implied values explicitly specified
-		{"//UPPERCASEISINVALID", ""},                                               // Invalid input
+		// This is docker behaviour: If there's a digest the tag is ignored when pulling an image.
+		{"//busybox:latest" + sha256digest, "docker.io/library/busybox:latest" + sha256digest}, // Both tag and digest
+		{"//docker.io/library/busybox:latest", "docker.io/library/busybox:latest"},             // All implied values explicitly specified
+		{"//UPPERCASEISINVALID", ""}, // Invalid input
 	} {
 		ref, err := fn(c.input)
 		if c.expected == "" {
@@ -68,10 +68,11 @@ func testParseReference(t *testing.T, fn func(string) (types.ImageReference, err
 
 // A common list of reference formats to test for the various ImageReference methods.
 var validReferenceTestCases = []struct{ input, dockerRef, stringWithinTransport string }{
-	{"busybox:notlatest", "docker.io/library/busybox:notlatest", "//busybox:notlatest"},                // Explicit tag
-	{"busybox" + sha256digest, "docker.io/library/busybox" + sha256digest, "//busybox" + sha256digest}, // Explicit digest
-	{"docker.io/library/busybox:latest", "docker.io/library/busybox:latest", "//busybox:latest"},       // All implied values explicitly specified
-	{"example.com/ns/foo:bar", "example.com/ns/foo:bar", "//example.com/ns/foo:bar"},                   // All values explicitly specified
+	{"busybox:notlatest", "docker.io/library/busybox:notlatest", "//busybox:notlatest"},                                     // Explicit tag
+	{"busybox" + sha256digest, "docker.io/library/busybox" + sha256digest, "//busybox" + sha256digest},                      // Explicit digest
+	{"busybox:latest" + sha256digest, "docker.io/library/busybox:latest" + sha256digest, "//busybox:latest" + sha256digest}, // Both tag and digest
+	{"docker.io/library/busybox:latest", "docker.io/library/busybox:latest", "//busybox:latest"},                            // All implied values explicitly specified
+	{"example.com/ns/foo:bar", "example.com/ns/foo:bar", "//example.com/ns/foo:bar"},                                        // All values explicitly specified
 }
 
 func TestNewReference(t *testing.T) {
@@ -99,7 +100,7 @@ func TestNewReference(t *testing.T) {
 	_, ok = parsed.(reference.NamedTagged)
 	require.True(t, ok)
 	_, err = NewReference(parsed)
-	assert.Error(t, err)
+	assert.NoError(t, err)
 }
 
 func TestReferenceTransport(t *testing.T) {
@@ -181,6 +182,14 @@ func TestReferenceNewImageDestination(t *testing.T) {
 	defer dest.Close()
 }
 
+func TestReferenceNewImageDestinationWithTagAndDigest(t *testing.T) {
+	ref, err := ParseReference("//busybox:latest" + sha256digest)
+	require.NoError(t, err)
+	_, err = ref.NewImageDestination(context.Background(),
+		&types.SystemContext{RegistriesDirPath: "/this/doesnt/exist", DockerPerHostCertDirPath: "/this/doesnt/exist"})
+	require.Error(t, err)
+}
+
 func TestReferenceTagOrDigest(t *testing.T) {
 	for input, expected := range map[string]string{
 		"//busybox:notlatest":      "notlatest",

docker/policyconfiguration/naming.go

@@ -15,14 +15,12 @@ func DockerReferenceIdentity(ref reference.Named) (string, error) {
 	tagged, isTagged := ref.(reference.NamedTagged)
 	digested, isDigested := ref.(reference.Canonical)
 	switch {
-	case isTagged && isDigested: // Note that this CAN actually happen.
-		return "", errors.Errorf("Unexpected Docker reference %s with both a name and a digest", reference.FamiliarString(ref))
 	case !isTagged && !isDigested: // This should not happen, the caller is expected to ensure !reference.IsNameOnly()
 		return "", errors.Errorf("Internal inconsistency: Docker reference %s with neither a tag nor a digest", reference.FamiliarString(ref))
+	case isDigested: // Note that (isTagged && isDigested) CAN actually happen as well while the digest should be used in this case.
+		res = res + "@" + digested.Digest().String()
 	case isTagged:
 		res = res + ":" + tagged.Tag()
-	case isDigested:
-		res = res + "@" + digested.Digest().String()
 	default: // Coverage: The above was supposed to be exhaustive.
 		return "", errors.New("Internal inconsistency, unexpected default branch")
 	}

docker/policyconfiguration/naming_test.go

@@ -32,8 +32,9 @@ func TestDockerReference(t *testing.T) {
 		"repo":                   {"docker.io/library/repo", "docker.io/library", "docker.io"},
 	} {
 		for inputSuffix, mappedSuffix := range map[string]string{
-			":tag":       ":tag",
-			sha256Digest: sha256Digest,
+			":tag":                ":tag",
+			sha256Digest:          sha256Digest,
+			":tag" + sha256Digest: sha256Digest, // tag with digest should be treated as digest
 		} {
 			fullInput := inputName + inputSuffix
 			ref, err := reference.ParseNormalizedNamed(fullInput)
@@ -74,6 +75,6 @@ func TestDockerReferenceIdentity(t *testing.T) {
 	_, ok = parsed.(reference.NamedTagged)
 	require.True(t, ok)
 	id, err = DockerReferenceIdentity(parsed)
-	assert.Equal(t, "", id)
-	assert.Error(t, err)
+	assert.Equal(t, "docker.io/library/busybox@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef", id)
+	assert.NoError(t, err)
 }

transports/alltransports/alltransports_test.go

@@ -9,6 +9,8 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
+const sha256digest = "@sha256:0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef"
+
 func TestParseImageName(t *testing.T) {
 	// This primarily tests error handling, TestImageNameHandling is a table-driven
 	// test for the expected values.
@@ -30,6 +32,8 @@ func TestImageNameHandling(t *testing.T) {
 		{"dir", "/etc", "/etc"},
 		{"docker", "//busybox", "//busybox:latest"},
 		{"docker", "//busybox:notlatest", "//busybox:notlatest"}, // This also tests handling of multiple ":" characters
+		{"docker", "//busybox" + sha256digest, "//busybox" + sha256digest},
+		{"docker", "//busybox:v1-dirty" + sha256digest, "//busybox:v1-dirty" + sha256digest},
 		{"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:someimage", "/etc:someimage"},