Merge pull request #33014 from DirectXMan12/feature/set-image-id-manifest-digest

Automatic merge from submit-queue

Kubelet: Use RepoDigest for ImageID when available

**Release note**:
```release-note
Use manifest digest (as `docker-pullable://`) as ImageID when available (exposes a canonical, pullable image ID for containers).
```

Previously, we used the docker config digest (also called "image ID"
by Docker) for the value of the `ImageID` field in the container status.
This was not particularly useful, since the config manifest is not
what's used to identify the image in a registry, which uses the manifest
digest instead.  Docker 1.12+ always populates the RepoDigests field
with the manifest digests, and Docker 1.10 and 1.11 populate it when
images are pulled by digest.

This commit changes `ImageID` to point to the the manifest digest when
available, using the prefix `docker-pullable://` (instead of
`docker://`)

Related to #32159

pkg/kubelet/dockertools/docker.go

@@ -43,6 +43,7 @@ import (
 const (
 	PodInfraContainerName = leaky.PodInfraContainerName
 	DockerPrefix          = "docker://"
+	DockerPullablePrefix  = "docker-pullable://"
 	LogSuffix             = "log"
 	ext4MaxFileNameLen    = 255
 )
@@ -66,7 +67,8 @@ type DockerInterface interface {
 	StartContainer(id string) error
 	StopContainer(id string, timeout int) error
 	RemoveContainer(id string, opts dockertypes.ContainerRemoveOptions) error
-	InspectImage(image string) (*dockertypes.ImageInspect, error)
+	InspectImageByRef(imageRef string) (*dockertypes.ImageInspect, error)
+	InspectImageByID(imageID string) (*dockertypes.ImageInspect, error)
 	ListImages(opts dockertypes.ImageListOptions) ([]dockertypes.Image, error)
 	PullImage(image string, auth dockertypes.AuthConfig, opts dockertypes.ImagePullOptions) error
 	RemoveImage(image string, opts dockertypes.ImageRemoveOptions) ([]dockertypes.ImageDelete, error)
@@ -136,7 +138,11 @@ func filterHTTPError(err error, image string) error {
 	}
 }
 
-// Check if the inspected image matches what we are looking for
+// matchImageTagOrSHA checks if the given image specifier is a valid image ref,
+// and that it matches the given image. It should fail on things like image IDs
+// (config digests) and other digest-only references, but succeed on image names
+// (`foo`), tag references (`foo:bar`), and manifest digest references
+// (`foo@sha256:xyz`).
 func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool {
 	// The image string follows the grammar specified here
 	// https://github.com/docker/distribution/blob/master/reference/reference.go#L4
@@ -193,6 +199,43 @@ func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool {
 	return false
 }
 
+// matchImageIDOnly checks that the given image specifier is a digest-only
+// reference, and that it matches the given image.
+func matchImageIDOnly(inspected dockertypes.ImageInspect, image string) bool {
+	// If the image ref is literally equal to the inspected image's ID,
+	// just return true here (this might be the case for Docker 1.9,
+	// where we won't have a digest for the ID)
+	if inspected.ID == image {
+		return true
+	}
+
+	// Otherwise, we should try actual parsing to be more correct
+	ref, err := dockerref.Parse(image)
+	if err != nil {
+		glog.V(4).Infof("couldn't parse image reference %q: %v", image, err)
+		return false
+	}
+
+	digest, isDigested := ref.(dockerref.Digested)
+	if !isDigested {
+		glog.V(4).Infof("the image reference %q was not a digest reference")
+		return false
+	}
+
+	id, err := dockerdigest.ParseDigest(inspected.ID)
+	if err != nil {
+		glog.V(4).Infof("couldn't parse image ID reference %q: %v", id, err)
+		return false
+	}
+
+	if digest.Digest().Algorithm().String() == id.Algorithm().String() && digest.Digest().Hex() == id.Hex() {
+		return true
+	}
+
+	glog.V(4).Infof("The reference %s does not directly refer to the given image's ID (%q)", image, inspected.ID)
+	return false
+}
+
 func (p dockerPuller) Pull(image string, secrets []api.Secret) error {
 	keyring, err := credentialprovider.MakeDockerKeyring(secrets, p.keyring)
 	if err != nil {
@@ -246,7 +289,7 @@ func (p dockerPuller) Pull(image string, secrets []api.Secret) error {
 }
 
 func (p dockerPuller) IsImagePresent(image string) (bool, error) {
-	_, err := p.client.InspectImage(image)
+	_, err := p.client.InspectImageByRef(image)
 	if err == nil {
 		return true, nil
 	}

pkg/kubelet/dockertools/docker_manager.go

@@ -397,11 +397,26 @@ func (dm *DockerManager) inspectContainer(id string, podName, podNamespace strin
 		parseTimestampError("FinishedAt", iResult.State.FinishedAt)
 	}
 
+	// default to the image ID, but try and inspect for the RepoDigests
+	imageID := DockerPrefix + iResult.Image
+	imgInspectResult, err := dm.client.InspectImageByID(iResult.Image)
+	if err != nil {
+		utilruntime.HandleError(fmt.Errorf("unable to inspect docker image %q while inspecting docker container %q: %v", containerName, iResult.Image, err))
+	} else {
+		if len(imgInspectResult.RepoDigests) > 1 {
+			glog.V(4).Infof("Container %q had more than one associated RepoDigest (%v), only using the first", containerName, imgInspectResult.RepoDigests)
+		}
+
+		if len(imgInspectResult.RepoDigests) > 0 {
+			imageID = DockerPullablePrefix + imgInspectResult.RepoDigests[0]
+		}
+	}
+
 	status := kubecontainer.ContainerStatus{
 		Name:         containerName,
 		RestartCount: containerInfo.RestartCount,
 		Image:        iResult.Config.Image,
-		ImageID:      DockerPrefix + iResult.Image,
+		ImageID:      imageID,
 		ID:           kubecontainer.DockerID(id).ContainerID(),
 		ExitCode:     iResult.State.ExitCode,
 		CreatedAt:    createdAt,
@@ -913,7 +928,7 @@ func (dm *DockerManager) IsImagePresent(image kubecontainer.ImageSpec) (bool, er
 // Removes the specified image.
 func (dm *DockerManager) RemoveImage(image kubecontainer.ImageSpec) error {
 	// If the image has multiple tags, we need to remove all the tags
-	if inspectImage, err := dm.client.InspectImage(image.Image); err == nil && len(inspectImage.RepoTags) > 1 {
+	if inspectImage, err := dm.client.InspectImageByID(image.Image); err == nil && len(inspectImage.RepoTags) > 1 {
 		for _, tag := range inspectImage.RepoTags {
 			if _, err := dm.client.RemoveImage(tag, dockertypes.ImageRemoveOptions{PruneChildren: true}); err != nil {
 				return err
@@ -2414,7 +2429,7 @@ func (dm *DockerManager) verifyNonRoot(container *api.Container) error {
 // or the user is set to root.  If there is an error inspecting the image this method will return
 // false and return the error.
 func (dm *DockerManager) isImageRoot(image string) (bool, error) {
-	img, err := dm.client.InspectImage(image)
+	img, err := dm.client.InspectImageByRef(image)
 	if err != nil {
 		return false, err
 	}

pkg/kubelet/dockertools/docker_manager_test.go

@@ -151,6 +151,9 @@ func createTestDockerManager(fakeHTTPClient *fakeHTTP, fakeDocker *FakeDockerCli
 		fakeHTTPClient,
 		flowcontrol.NewBackOff(time.Second, 300*time.Second))
 
+	// default this to an empty result, so that we never have a nil non-error response from InspectImage
+	fakeDocker.Image = &dockertypes.ImageInspect{}
+
 	return dockerManager, fakeDocker
 }
 

pkg/kubelet/dockertools/docker_test.go

@@ -315,6 +315,85 @@ func TestMatchImageTagOrSHA(t *testing.T) {
 	}
 }
 
+func TestMatchImageIDOnly(t *testing.T) {
+	for i, testCase := range []struct {
+		Inspected dockertypes.ImageInspect
+		Image     string
+		Output    bool
+	}{
+		// shouldn't match names or tagged names
+		{
+			Inspected: dockertypes.ImageInspect{RepoTags: []string{"ubuntu:latest"}},
+			Image:     "ubuntu",
+			Output:    false,
+		},
+		{
+			Inspected: dockertypes.ImageInspect{RepoTags: []string{"colemickens/hyperkube-amd64:217.9beff63"}},
+			Image:     "colemickens/hyperkube-amd64:217.9beff63",
+			Output:    false,
+		},
+		// should match name@digest refs if they refer to the image ID (but only the full ID)
+		{
+			Inspected: dockertypes.ImageInspect{
+				ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",
+			},
+			Image:  "myimage@sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",
+			Output: true,
+		},
+		{
+			Inspected: dockertypes.ImageInspect{
+				ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",
+			},
+			Image:  "myimage@sha256:2208f7a29005",
+			Output: false,
+		},
+		{
+			Inspected: dockertypes.ImageInspect{
+				ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",
+			},
+			Image:  "myimage@sha256:2208",
+			Output: false,
+		},
+		// should match when the IDs are literally the same
+		{
+			Inspected: dockertypes.ImageInspect{
+				ID: "foobar",
+			},
+			Image:  "foobar",
+			Output: true,
+		},
+		// shouldn't match mismatched IDs
+		{
+			Inspected: dockertypes.ImageInspect{
+				ID: "sha256:2208f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",
+			},
+			Image:  "myimage@sha256:0000f7a29005d226d1ee33a63e33af1f47af6156c740d7d23c7948e8d282d53d",
+			Output: false,
+		},
+		// shouldn't match invalid IDs or refs
+		{
+			Inspected: dockertypes.ImageInspect{
+				ID: "sha256:unparseable",
+			},
+			Image:  "myimage@sha256:unparseable",
+			Output: false,
+		},
+		// shouldn't match against repo digests
+		{
+			Inspected: dockertypes.ImageInspect{
+				ID:          "sha256:9bbdf247c91345f0789c10f50a57e36a667af1189687ad1de88a6243d05a2227",
+				RepoDigests: []string{"centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf"},
+			},
+			Image:  "centos/ruby-23-centos7@sha256:940584acbbfb0347272112d2eb95574625c0c60b4e2fdadb139de5859cf754bf",
+			Output: false,
+		},
+	} {
+		match := matchImageIDOnly(testCase.Inspected, testCase.Image)
+		assert.Equal(t, testCase.Output, match, fmt.Sprintf("%s is not a match (%d)", testCase.Image, i))
+	}
+
+}
+
 func TestPullWithNoSecrets(t *testing.T) {
 	tests := []struct {
 		imageName     string
@@ -614,8 +693,14 @@ type imageTrackingDockerClient struct {
 	imageName string
 }
 
-func (f *imageTrackingDockerClient) InspectImage(name string) (image *dockertypes.ImageInspect, err error) {
-	image, err = f.FakeDockerClient.InspectImage(name)
+func (f *imageTrackingDockerClient) InspectImageByID(name string) (image *dockertypes.ImageInspect, err error) {
+	image, err = f.FakeDockerClient.InspectImageByID(name)
+	f.imageName = name
+	return
+}
+
+func (f *imageTrackingDockerClient) InspectImageByRef(name string) (image *dockertypes.ImageInspect, err error) {
+	image, err = f.FakeDockerClient.InspectImageByRef(name)
 	f.imageName = name
 	return
 }

pkg/kubelet/dockertools/fake_docker_client.go

@@ -310,9 +310,19 @@ func (f *FakeDockerClient) InspectContainer(id string) (*dockertypes.ContainerJS
 	return nil, fmt.Errorf("container %q not found", id)
 }
 
-// InspectImage is a test-spy implementation of DockerInterface.InspectImage.
+// InspectImageByRef is a test-spy implementation of DockerInterface.InspectImageByRef.
 // It adds an entry "inspect" to the internal method call record.
-func (f *FakeDockerClient) InspectImage(name string) (*dockertypes.ImageInspect, error) {
+func (f *FakeDockerClient) InspectImageByRef(name string) (*dockertypes.ImageInspect, error) {
+	f.Lock()
+	defer f.Unlock()
+	f.called = append(f.called, calledDetail{name: "inspect_image"})
+	err := f.popError("inspect_image")
+	return f.Image, err
+}
+
+// InspectImageByID is a test-spy implementation of DockerInterface.InspectImageByID.
+// It adds an entry "inspect" to the internal method call record.
+func (f *FakeDockerClient) InspectImageByID(name string) (*dockertypes.ImageInspect, error) {
 	f.Lock()
 	defer f.Unlock()
 	f.called = append(f.called, calledDetail{name: "inspect_image"})

pkg/kubelet/dockertools/instrumented_docker.go

@@ -107,11 +107,20 @@ func (in instrumentedDockerInterface) RemoveContainer(id string, opts dockertype
 	return err
 }
 
-func (in instrumentedDockerInterface) InspectImage(image string) (*dockertypes.ImageInspect, error) {
+func (in instrumentedDockerInterface) InspectImageByRef(image string) (*dockertypes.ImageInspect, error) {
 	const operation = "inspect_image"
 	defer recordOperation(operation, time.Now())
 
-	out, err := in.client.InspectImage(image)
+	out, err := in.client.InspectImageByRef(image)
+	recordError(operation, err)
+	return out, err
+}
+
+func (in instrumentedDockerInterface) InspectImageByID(image string) (*dockertypes.ImageInspect, error) {
+	const operation = "inspect_image"
+	defer recordOperation(operation, time.Now())
+
+	out, err := in.client.InspectImageByID(image)
 	recordError(operation, err)
 	return out, err
 }

pkg/kubelet/dockertools/kube_docker_client.go

@@ -182,25 +182,47 @@ func (d *kubeDockerClient) RemoveContainer(id string, opts dockertypes.Container
 	return err
 }
 
-func (d *kubeDockerClient) InspectImage(image string) (*dockertypes.ImageInspect, error) {
+func (d *kubeDockerClient) inspectImageRaw(ref string) (*dockertypes.ImageInspect, error) {
 	ctx, cancel := d.getTimeoutContext()
 	defer cancel()
-	resp, _, err := d.client.ImageInspectWithRaw(ctx, image, true)
+	resp, _, err := d.client.ImageInspectWithRaw(ctx, ref, true)
 	if ctxErr := contextError(ctx); ctxErr != nil {
 		return nil, ctxErr
 	}
 	if err != nil {
 		if dockerapi.IsErrImageNotFound(err) {
-			err = imageNotFoundError{ID: image}
+			err = imageNotFoundError{ID: ref}
 		}
 		return nil, err
 	}
-	if !matchImageTagOrSHA(resp, image) {
-		return nil, imageNotFoundError{ID: image}
-	}
+
 	return &resp, nil
 }
 
+func (d *kubeDockerClient) InspectImageByID(imageID string) (*dockertypes.ImageInspect, error) {
+	resp, err := d.inspectImageRaw(imageID)
+	if err != nil {
+		return nil, err
+	}
+
+	if !matchImageIDOnly(*resp, imageID) {
+		return nil, imageNotFoundError{ID: imageID}
+	}
+	return resp, nil
+}
+
+func (d *kubeDockerClient) InspectImageByRef(imageRef string) (*dockertypes.ImageInspect, error) {
+	resp, err := d.inspectImageRaw(imageRef)
+	if err != nil {
+		return nil, err
+	}
+
+	if !matchImageTagOrSHA(*resp, imageRef) {
+		return nil, imageNotFoundError{ID: imageRef}
+	}
+	return resp, nil
+}
+
 func (d *kubeDockerClient) ImageHistory(id string) ([]dockertypes.ImageHistory, error) {
 	ctx, cancel := d.getTimeoutContext()
 	defer cancel()

test/e2e_node/image_id_test.go

@@ -0,0 +1,66 @@
+/*
+Copyright 2016 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package e2e_node
+
+import (
+	"k8s.io/kubernetes/pkg/api"
+	"k8s.io/kubernetes/pkg/kubelet/dockertools"
+	"k8s.io/kubernetes/test/e2e/framework"
+
+	"github.com/davecgh/go-spew/spew"
+	. "github.com/onsi/ginkgo"
+	. "github.com/onsi/gomega"
+)
+
+var _ = framework.KubeDescribe("ImageID", func() {
+
+	busyBoxImage := "gcr.io/google_containers/busybox@sha256:4bdd623e848417d96127e16037743f0cd8b528c026e9175e22a84f639eca58ff"
+
+	f := framework.NewDefaultFramework("image-id-test")
+
+	It("should be set to the manifest digest (from RepoDigests) when available", func() {
+		podDesc := &api.Pod{
+			ObjectMeta: api.ObjectMeta{
+				Name: "pod-with-repodigest",
+			},
+			Spec: api.PodSpec{
+				Containers: []api.Container{{
+					Name:    "test",
+					Image:   busyBoxImage,
+					Command: []string{"sh"},
+				}},
+				RestartPolicy: api.RestartPolicyNever,
+			},
+		}
+
+		pod := f.PodClient().Create(podDesc)
+
+		framework.ExpectNoError(framework.WaitTimeoutForPodNoLongerRunningInNamespace(
+			f.Client, pod.Name, f.Namespace.Name, "", framework.PodStartTimeout))
+		runningPod, err := f.PodClient().Get(pod.Name)
+		framework.ExpectNoError(err)
+
+		status := runningPod.Status
+
+		if len(status.ContainerStatuses) == 0 {
+			framework.Failf("Unexpected pod status; %s", spew.Sdump(status))
+			return
+		}
+
+		Expect(status.ContainerStatuses[0].ImageID).To(Equal(dockertools.DockerPullablePrefix + busyBoxImage))
+	})
+})

test/e2e_node/image_list.go

@@ -41,6 +41,7 @@ var NodeImageWhiteList = sets.NewString(
 	"google/cadvisor:latest",
 	"gcr.io/google-containers/stress:v1",
 	"gcr.io/google_containers/busybox:1.24",
+	"gcr.io/google_containers/busybox@sha256:4bdd623e848417d96127e16037743f0cd8b528c026e9175e22a84f639eca58ff",
 	"gcr.io/google_containers/nginx-slim:0.7",
 	"gcr.io/google_containers/serve_hostname:v1.4",
 	"gcr.io/google_containers/netexec:1.7",