From b46dbbec1b6259d23f7fdb8d83f5e2ee4bf7cdc9 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Fri, 30 Sep 2016 14:30:48 -0400 Subject: [PATCH 1/2] Add method to inspect Docker images by ID Previously, the `InspectImage` method of the Docker interface expected a "pullable" image ref (name, tag, or manifest digest). If you tried to inspect an image by its ID (config digest), the inspect would fail to validate the image against the input identifier. This commit changes the original method to be named `InspectImageByRef`, and introduces a new method called `InspectImageByID` which validates that the input identifier was an image ID. --- pkg/kubelet/dockertools/docker.go | 48 +++++++++- pkg/kubelet/dockertools/docker_manager.go | 4 +- pkg/kubelet/dockertools/docker_test.go | 89 ++++++++++++++++++- pkg/kubelet/dockertools/fake_docker_client.go | 14 ++- .../dockertools/instrumented_docker.go | 13 ++- pkg/kubelet/dockertools/kube_docker_client.go | 34 +++++-- 6 files changed, 185 insertions(+), 17 deletions(-) diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index ebe50bc10fd..425f433ce65 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/pkg/kubelet/dockertools/docker.go @@ -66,7 +66,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 +137,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 +198,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 +288,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 } diff --git a/pkg/kubelet/dockertools/docker_manager.go b/pkg/kubelet/dockertools/docker_manager.go index c8346a382bf..2ed8705e505 100644 --- a/pkg/kubelet/dockertools/docker_manager.go +++ b/pkg/kubelet/dockertools/docker_manager.go @@ -913,7 +913,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 +2414,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 } diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index 745c5cf6dce..4516cef7164 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/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 } diff --git a/pkg/kubelet/dockertools/fake_docker_client.go b/pkg/kubelet/dockertools/fake_docker_client.go index 85bea450b0c..5a6c64164d1 100644 --- a/pkg/kubelet/dockertools/fake_docker_client.go +++ b/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"}) diff --git a/pkg/kubelet/dockertools/instrumented_docker.go b/pkg/kubelet/dockertools/instrumented_docker.go index eb15226498a..66de3aecfea 100644 --- a/pkg/kubelet/dockertools/instrumented_docker.go +++ b/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 } diff --git a/pkg/kubelet/dockertools/kube_docker_client.go b/pkg/kubelet/dockertools/kube_docker_client.go index e6355748c93..32297fa28c2 100644 --- a/pkg/kubelet/dockertools/kube_docker_client.go +++ b/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() From 135f87dc15186b494b27a47b298ed24d95cbf889 Mon Sep 17 00:00:00 2001 From: Solly Ross Date: Fri, 30 Sep 2016 14:51:26 -0400 Subject: [PATCH 2/2] Kubelet: Use RepoDigest for ImageID when available 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://`) --- pkg/kubelet/dockertools/docker.go | 1 + pkg/kubelet/dockertools/docker_manager.go | 17 ++++- pkg/kubelet/dockertools/fake_docker_client.go | 3 + test/e2e_node/image_id_test.go | 66 +++++++++++++++++++ test/e2e_node/image_list.go | 1 + 5 files changed, 87 insertions(+), 1 deletion(-) create mode 100644 test/e2e_node/image_id_test.go diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index 425f433ce65..89aae7f7e22 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/pkg/kubelet/dockertools/docker.go @@ -43,6 +43,7 @@ import ( const ( PodInfraContainerName = leaky.PodInfraContainerName DockerPrefix = "docker://" + DockerPullablePrefix = "docker-pullable://" LogSuffix = "log" ext4MaxFileNameLen = 255 ) diff --git a/pkg/kubelet/dockertools/docker_manager.go b/pkg/kubelet/dockertools/docker_manager.go index 2ed8705e505..438d20d92cf 100644 --- a/pkg/kubelet/dockertools/docker_manager.go +++ b/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, diff --git a/pkg/kubelet/dockertools/fake_docker_client.go b/pkg/kubelet/dockertools/fake_docker_client.go index 5a6c64164d1..0f6908c739e 100644 --- a/pkg/kubelet/dockertools/fake_docker_client.go +++ b/pkg/kubelet/dockertools/fake_docker_client.go @@ -86,6 +86,9 @@ func newClientWithVersionAndClock(version, apiVersion string, c clock.Clock) *Fa Errors: make(map[string]error), ContainerMap: make(map[string]*dockertypes.ContainerJSON), Clock: c, + + // default this to an empty result, so that we never have a nil non-error response from InspectImage + Image: &dockertypes.ImageInspect{}, } } diff --git a/test/e2e_node/image_id_test.go b/test/e2e_node/image_id_test.go new file mode 100644 index 00000000000..d81ac51a688 --- /dev/null +++ b/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)) + }) +}) diff --git a/test/e2e_node/image_list.go b/test/e2e_node/image_list.go index a6031703661..b6a8b93bf40 100644 --- a/test/e2e_node/image_list.go +++ b/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",