From e77d8a59eb382f61af7aaa371f51a440432fee5e Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Mon, 14 May 2018 10:36:40 -0400 Subject: [PATCH 1/5] driver/docker: pull image with digest GH #4290 Add digest support to the docker driver image config. This commit factors out some common code to print the repo:tag (dockerImageRef) for events/logs as well as parsing the image to retreive the repo,tag (parseDockerImage) so that the results are consistent/sane for both repo:tag and repo@sha256:... references. When pulling an image with a digest, the tag is blank and the repo contains the digest. See: https://github.com/fsouza/go-dockerclient/blob/master/image_test.go#L471 --- client/driver/docker.go | 30 ++++++++++++++---- client/driver/docker_coordinator.go | 11 +++---- client/driver/docker_test.go | 48 +++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 13 deletions(-) diff --git a/client/driver/docker.go b/client/driver/docker.go index cc31e547eae3..ff4545223782 100644 --- a/client/driver/docker.go +++ b/client/driver/docker.go @@ -1500,10 +1500,7 @@ func (d *DockerDriver) Periodic() (bool, time.Duration) { // loading it from the file system func (d *DockerDriver) createImage(driverConfig *DockerDriverConfig, client *docker.Client, taskDir *allocdir.TaskDir) (string, error) { image := driverConfig.ImageName - repo, tag := docker.ParseRepositoryTag(image) - if tag == "" { - tag = "latest" - } + repo, tag := parseDockerImage(image) coordinator, callerID := d.getDockerCoordinator(client) @@ -1511,7 +1508,7 @@ func (d *DockerDriver) createImage(driverConfig *DockerDriverConfig, client *doc // is "latest", or ForcePull is set, we have to check for a new version every time so we don't // bother to check and cache the id here. We'll download first, then cache. if driverConfig.ForcePull { - d.logger.Printf("[DEBUG] driver.docker: force pull image '%s:%s' instead of inspecting local", repo, tag) + d.logger.Printf("[DEBUG] driver.docker: force pull image '%s' instead of inspecting local", dockerImageRef(repo, tag)) } else if tag != "latest" { if dockerImage, _ := client.InspectImage(image); dockerImage != nil { // Image exists so just increment its reference count @@ -1544,7 +1541,7 @@ func (d *DockerDriver) pullImage(driverConfig *DockerDriverConfig, client *docke d.logger.Printf("[DEBUG] driver.docker: did not find docker auth for repo %q", repo) } - d.emitEvent("Downloading image %s:%s", repo, tag) + d.emitEvent("Downloading image %s", dockerImageRef(repo, tag)) coordinator, callerID := d.getDockerCoordinator(client) return coordinator.PullImage(driverConfig.ImageName, authOptions, callerID, d.emitEvent) @@ -2183,3 +2180,24 @@ type createContainerClient interface { ListContainers(docker.ListContainersOptions) ([]docker.APIContainers, error) RemoveContainer(opts docker.RemoveContainerOptions) error } + +func parseDockerImage(image string) (repo, tag string) { + repo, tag = docker.ParseRepositoryTag(image) + if tag == "" { + if i := strings.IndexRune(image, '@'); i > -1 { // Has digest (@sha256:...) + // when pulling images with a digest, the repository contains the sha hash, and the tag is empty + // see: https://github.com/fsouza/go-dockerclient/blob/master/image_test.go#L471 + repo = image + } else { + tag = "latest" + } + } + return +} + +func dockerImageRef(repo string, tag string) string { + if tag == "" { + return repo + } + return fmt.Sprintf("%s:%s", repo, tag) +} diff --git a/client/driver/docker_coordinator.go b/client/driver/docker_coordinator.go index 7f975bae59f2..bbf09de3e8ad 100644 --- a/client/driver/docker_coordinator.go +++ b/client/driver/docker_coordinator.go @@ -178,10 +178,7 @@ func (d *dockerCoordinator) PullImage(image string, authOptions *docker.AuthConf func (d *dockerCoordinator) pullImageImpl(image string, authOptions *docker.AuthConfiguration, future *pullFuture) { defer d.clearPullLogger(image) // Parse the repo and tag - repo, tag := docker.ParseRepositoryTag(image) - if tag == "" { - tag = "latest" - } + repo, tag := parseDockerImage(image) ctx, cancel := context.WithCancel(context.Background()) defer cancel() @@ -206,18 +203,18 @@ func (d *dockerCoordinator) pullImageImpl(image string, authOptions *docker.Auth err := d.client.PullImage(pullOptions, auth) if ctxErr := ctx.Err(); ctxErr == context.DeadlineExceeded { - d.logger.Printf("[ERR] driver.docker: timeout pulling container %s:%s", repo, tag) + d.logger.Printf("[ERR] driver.docker: timeout pulling container %s", dockerImageRef(repo, tag)) future.set("", recoverablePullError(ctxErr, image)) return } if err != nil { - d.logger.Printf("[ERR] driver.docker: failed pulling container %s:%s: %s", repo, tag, err) + d.logger.Printf("[ERR] driver.docker: failed pulling container %s: %s", dockerImageRef(repo, tag), err) future.set("", recoverablePullError(err, image)) return } - d.logger.Printf("[DEBUG] driver.docker: docker pull %s:%s succeeded", repo, tag) + d.logger.Printf("[DEBUG] driver.docker: docker pull %s succeeded", dockerImageRef(repo, tag)) dockerImage, err := d.client.InspectImage(image) if err != nil { diff --git a/client/driver/docker_test.go b/client/driver/docker_test.go index 5595236c7ff8..7e4b16e1ec97 100644 --- a/client/driver/docker_test.go +++ b/client/driver/docker_test.go @@ -1092,6 +1092,30 @@ func TestDockerDriver_ForcePull(t *testing.T) { } } +func TestDockerDriver_ForcePull_RepoDigest(t *testing.T) { + if !tu.IsTravis() { + t.Parallel() + } + if !testutil.DockerIsConnected(t) { + t.Skip("Docker not connected") + } + + task, _, _ := dockerTask(t) + task.Config["load"] = "" + task.Config["image"] = "library/busybox@sha256:58ac43b2cc92c687a32c8be6278e50a063579655fe3090125dcb2af0ff9e1a64" + task.Config["force_pull"] = "true" + + client, handle, cleanup := dockerSetup(t, task) + defer cleanup() + + waitForExist(t, client, handle) + + _, err := client.InspectContainer(handle.ContainerID()) + if err != nil { + t.Fatalf("err: %v", err) + } +} + func TestDockerDriver_SecurityOpt(t *testing.T) { if !tu.IsTravis() { t.Parallel() @@ -2458,3 +2482,27 @@ func TestDockerDriver_AdvertiseIPv6Address(t *testing.T) { t.Fatalf("Got GlobalIPv6address %s want GlobalIPv6address with prefix %s", expectedPrefix, container.NetworkSettings.GlobalIPv6Address) } } + +func TestParseDockerImage(t *testing.T) { + tests := []struct { + Image string + Repo string + Tag string + }{ + {"library/hello-world:1.0", "library/hello-world", "1.0"}, + {"library/hello-world", "library/hello-world", "latest"}, + {"library/hello-world:latest", "library/hello-world", "latest"}, + {"library/hello-world@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77", "library/hello-world@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77", ""}, + } + for _, test := range tests { + t.Run(test.Image, func(t *testing.T) { + repo, tag := parseDockerImage(test.Image) + if repo != test.Repo { + t.Errorf("expected repo '%s' but got '%s'", test.Repo, repo) + } + if repo != test.Repo { + t.Errorf("expected tag '%s' but got '%s'", test.Tag, tag) + } + }) + } +} From ce63b79449e0b34549d97953c4cbb72948dd25c7 Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Mon, 14 May 2018 11:11:51 -0400 Subject: [PATCH 2/5] driver/docker: cleanup parseDockerImage --- client/driver/docker.go | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/client/driver/docker.go b/client/driver/docker.go index ff4545223782..064eb6aafa9b 100644 --- a/client/driver/docker.go +++ b/client/driver/docker.go @@ -2183,16 +2183,17 @@ type createContainerClient interface { func parseDockerImage(image string) (repo, tag string) { repo, tag = docker.ParseRepositoryTag(image) - if tag == "" { - if i := strings.IndexRune(image, '@'); i > -1 { // Has digest (@sha256:...) - // when pulling images with a digest, the repository contains the sha hash, and the tag is empty - // see: https://github.com/fsouza/go-dockerclient/blob/master/image_test.go#L471 - repo = image - } else { - tag = "latest" - } + if tag != "" { + return repo, tag + } + if i := strings.IndexRune(image, '@'); i > -1 { // Has digest (@sha256:...) + // when pulling images with a digest, the repository contains the sha hash, and the tag is empty + // see: https://github.com/fsouza/go-dockerclient/blob/master/image_test.go#L471 + repo = image + } else { + tag = "latest" } - return + return repo, tag } func dockerImageRef(repo string, tag string) string { From 44923339a8fca188f588acc2783e5477ed513364 Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Mon, 14 May 2018 14:23:02 -0400 Subject: [PATCH 3/5] driver/docker: fix TestDockerDriver_ForcePull_RepoDigest --- client/driver/docker_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/client/driver/docker_test.go b/client/driver/docker_test.go index 7e4b16e1ec97..e3ff2943d551 100644 --- a/client/driver/docker_test.go +++ b/client/driver/docker_test.go @@ -1103,6 +1103,7 @@ func TestDockerDriver_ForcePull_RepoDigest(t *testing.T) { task, _, _ := dockerTask(t) task.Config["load"] = "" task.Config["image"] = "library/busybox@sha256:58ac43b2cc92c687a32c8be6278e50a063579655fe3090125dcb2af0ff9e1a64" + localDigest := "sha256:8ac48589692a53a9b8c2d1ceaa6b402665aa7fe667ba51ccc03002300856d8c7" task.Config["force_pull"] = "true" client, handle, cleanup := dockerSetup(t, task) @@ -1110,10 +1111,9 @@ func TestDockerDriver_ForcePull_RepoDigest(t *testing.T) { waitForExist(t, client, handle) - _, err := client.InspectContainer(handle.ContainerID()) - if err != nil { - t.Fatalf("err: %v", err) - } + container, err := client.InspectContainer(handle.ContainerID()) + require.NoError(t, err) + require.Equal(t, localDigest, container.Image) } func TestDockerDriver_SecurityOpt(t *testing.T) { From f4670f60035e1f27c0bb74f086837817e6d5e79d Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Mon, 14 May 2018 14:23:48 -0400 Subject: [PATCH 4/5] driver/docker: fix up TestParseDockerImage --- client/driver/docker_test.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/client/driver/docker_test.go b/client/driver/docker_test.go index e3ff2943d551..388d0bcd6652 100644 --- a/client/driver/docker_test.go +++ b/client/driver/docker_test.go @@ -2497,12 +2497,8 @@ func TestParseDockerImage(t *testing.T) { for _, test := range tests { t.Run(test.Image, func(t *testing.T) { repo, tag := parseDockerImage(test.Image) - if repo != test.Repo { - t.Errorf("expected repo '%s' but got '%s'", test.Repo, repo) - } - if repo != test.Repo { - t.Errorf("expected tag '%s' but got '%s'", test.Tag, tag) - } + require.Equal(t, test.Repo, repo) + require.Equal(t, test.Tag, tag) }) } } From ba2d9570e8e5883804d0368697f3649a045014b6 Mon Sep 17 00:00:00 2001 From: Justen Walker Date: Mon, 14 May 2018 14:24:03 -0400 Subject: [PATCH 5/5] driver/docker: add test for dockerImageRef --- client/driver/docker_test.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/client/driver/docker_test.go b/client/driver/docker_test.go index 388d0bcd6652..9d7a8e168a1b 100644 --- a/client/driver/docker_test.go +++ b/client/driver/docker_test.go @@ -2502,3 +2502,21 @@ func TestParseDockerImage(t *testing.T) { }) } } + +func TestDockerImageRef(t *testing.T) { + tests := []struct { + Image string + Repo string + Tag string + }{ + {"library/hello-world:1.0", "library/hello-world", "1.0"}, + {"library/hello-world:latest", "library/hello-world", "latest"}, + {"library/hello-world@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77", "library/hello-world@sha256:f5233545e43561214ca4891fd1157e1c3c563316ed8e237750d59bde73361e77", ""}, + } + for _, test := range tests { + t.Run(test.Image, func(t *testing.T) { + image := dockerImageRef(test.Repo, test.Tag) + require.Equal(t, test.Image, image) + }) + } +}