Merge pull request #33279 from nishanttotla/fix-service-image-issues

Ensure service images get default tag and print familiar strings

client/service_create.go

@@ -24,14 +24,18 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec,
 		headers["X-Registry-Auth"] = []string{options.EncodedRegistryAuth}
 	}
 
+	// ensure that the image is tagged
+	if taggedImg := imageWithTagString(service.TaskTemplate.ContainerSpec.Image); taggedImg != "" {
+		service.TaskTemplate.ContainerSpec.Image = taggedImg
+	}
+
 	// Contact the registry to retrieve digest and platform information
 	if options.QueryRegistry {
 		distributionInspect, err := cli.DistributionInspect(ctx, service.TaskTemplate.ContainerSpec.Image, options.EncodedRegistryAuth)
 		distErr = err
 		if err == nil {
 			// now pin by digest if the image doesn't already contain a digest
-			img := imageWithDigestString(service.TaskTemplate.ContainerSpec.Image, distributionInspect.Descriptor.Digest)
+			if img := imageWithDigestString(service.TaskTemplate.ContainerSpec.Image, distributionInspect.Descriptor.Digest); img != "" {
-			if img != "" {
 				service.TaskTemplate.ContainerSpec.Image = img
 			}
 			// add platforms that are compatible with the service
@@ -55,22 +59,33 @@ func (cli *Client) ServiceCreate(ctx context.Context, service swarm.ServiceSpec,
 }
 
 // imageWithDigestString takes an image string and a digest, and updates
-// the image string if it didn't originally contain a digest. It assumes
+// the image string if it didn't originally contain a digest. It returns
-// that the image string is not an image ID
+// an empty string if there are no updates.
 func imageWithDigestString(image string, dgst digest.Digest) string {
-	ref, err := reference.ParseAnyReference(image)
+	namedRef, err := reference.ParseNormalizedNamed(image)
 	if err == nil {
-		if _, isCanonical := ref.(reference.Canonical); !isCanonical {
+		if _, isCanonical := namedRef.(reference.Canonical); !isCanonical {
-			namedRef, _ := ref.(reference.Named)
+			// ensure that image gets a default tag if none is provided
 			img, err := reference.WithDigest(namedRef, dgst)
 			if err == nil {
-				return img.String()
+				return reference.FamiliarString(img)
 			}
 		}
 	}
 	return ""
 }
 
+// imageWithTagString takes an image string, and returns a tagged image
+// string, adding a 'latest' tag if one was not provided. It returns an
+// emptry string if a canonical reference was provided
+func imageWithTagString(image string) string {
+	namedRef, err := reference.ParseNormalizedNamed(image)
+	if err == nil {
+		return reference.FamiliarString(reference.TagNameOnly(namedRef))
+	}
+	return ""
+}
+
 // updateServicePlatforms updates the Platforms in swarm.Placement to list
 // all compatible platforms for the service, as found in distributionInspect
 // and returns a pointer to the new or updated swarm.Placement struct

client/service_create_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/docker/docker/api/types"
 	registrytypes "github.com/docker/docker/api/types/registry"
 	"github.com/docker/docker/api/types/swarm"
+	"github.com/opencontainers/go-digest"
 	"github.com/opencontainers/image-spec/specs-go/v1"
 	"golang.org/x/net/context"
 )
@@ -121,3 +122,92 @@ func TestServiceCreateCompatiblePlatforms(t *testing.T) {
 		t.Fatalf("expected `service_amd64`, got %s", r.ID)
 	}
 }
+
+func TestServiceCreateDigestPinning(t *testing.T) {
+	dgst := "sha256:c0537ff6a5218ef531ece93d4984efc99bbf3f7497c0a7726c88e2bb7584dc96"
+	dgstAlt := "sha256:37ffbf3f7497c07584dc9637ffbf3f7497c0758c0537ffbf3f7497c0c88e2bb7"
+	serviceCreateImage := ""
+	pinByDigestTests := []struct {
+		img      string // input image provided by the user
+		expected string // expected image after digest pinning
+	}{
+		// default registry returns familiar string
+		{"docker.io/library/alpine", "alpine:latest@" + dgst},
+		// provided tag is preserved and digest added
+		{"alpine:edge", "alpine:edge@" + dgst},
+		// image with provided alternative digest remains unchanged
+		{"alpine@" + dgstAlt, "alpine@" + dgstAlt},
+		// image with provided tag and alternative digest remains unchanged
+		{"alpine:edge@" + dgstAlt, "alpine:edge@" + dgstAlt},
+		// image on alternative registry does not result in familiar string
+		{"alternate.registry/library/alpine", "alternate.registry/library/alpine:latest@" + dgst},
+		// unresolvable image does not get a digest
+		{"cannotresolve", "cannotresolve:latest"},
+	}
+
+	client := &Client{
+		client: newMockClient(func(req *http.Request) (*http.Response, error) {
+			if strings.HasPrefix(req.URL.Path, "/services/create") {
+				// reset and set image received by the service create endpoint
+				serviceCreateImage = ""
+				var service swarm.ServiceSpec
+				if err := json.NewDecoder(req.Body).Decode(&service); err != nil {
+					return nil, fmt.Errorf("could not parse service create request")
+				}
+				serviceCreateImage = service.TaskTemplate.ContainerSpec.Image
+
+				b, err := json.Marshal(types.ServiceCreateResponse{
+					ID: "service_id",
+				})
+				if err != nil {
+					return nil, err
+				}
+				return &http.Response{
+					StatusCode: http.StatusOK,
+					Body:       ioutil.NopCloser(bytes.NewReader(b)),
+				}, nil
+			} else if strings.HasPrefix(req.URL.Path, "/distribution/cannotresolve") {
+				// unresolvable image
+				return nil, fmt.Errorf("cannot resolve image")
+			} else if strings.HasPrefix(req.URL.Path, "/distribution/") {
+				// resolvable images
+				b, err := json.Marshal(registrytypes.DistributionInspect{
+					Descriptor: v1.Descriptor{
+						Digest: digest.Digest(dgst),
+					},
+				})
+				if err != nil {
+					return nil, err
+				}
+				return &http.Response{
+					StatusCode: http.StatusOK,
+					Body:       ioutil.NopCloser(bytes.NewReader(b)),
+				}, nil
+			}
+			return nil, fmt.Errorf("unexpected URL '%s'", req.URL.Path)
+		}),
+	}
+
+	// run pin by digest tests
+	for _, p := range pinByDigestTests {
+		r, err := client.ServiceCreate(context.Background(), swarm.ServiceSpec{
+			TaskTemplate: swarm.TaskSpec{
+				ContainerSpec: swarm.ContainerSpec{
+					Image: p.img,
+				},
+			},
+		}, types.ServiceCreateOptions{QueryRegistry: true})
+
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if r.ID != "service_id" {
+			t.Fatalf("expected `service_id`, got %s", r.ID)
+		}
+
+		if p.expected != serviceCreateImage {
+			t.Fatalf("expected image %s, got %s", p.expected, serviceCreateImage)
+		}
+	}
+}

client/service_update.go

@@ -35,6 +35,11 @@ func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version
 
 	query.Set("version", strconv.FormatUint(version.Index, 10))
 
+	// ensure that the image is tagged
+	if taggedImg := imageWithTagString(service.TaskTemplate.ContainerSpec.Image); taggedImg != "" {
+		service.TaskTemplate.ContainerSpec.Image = taggedImg
+	}
+
 	// Contact the registry to retrieve digest and platform information
 	// This happens only when the image has changed
 	if options.QueryRegistry {
@@ -42,8 +47,7 @@ func (cli *Client) ServiceUpdate(ctx context.Context, serviceID string, version
 		distErr = err
 		if err == nil {
 			// now pin by digest if the image doesn't already contain a digest
-			img := imageWithDigestString(service.TaskTemplate.ContainerSpec.Image, distributionInspect.Descriptor.Digest)
+			if img := imageWithDigestString(service.TaskTemplate.ContainerSpec.Image, distributionInspect.Descriptor.Digest); img != "" {
-			if img != "" {
 				service.TaskTemplate.ContainerSpec.Image = img
 			}
 			// add platforms that are compatible with the service