From 5644587e076d774a35a88fd372a69e4261a7b029 Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Wed, 3 May 2017 09:34:00 -0700 Subject: [PATCH 1/3] More dockertools cleanup Move some constants/functions to dockershim and remove unused tests. --- pkg/kubelet/dockershim/docker_container.go | 2 +- pkg/kubelet/dockershim/docker_sandbox.go | 2 +- .../helpers_linux.go} | 2 +- .../helpers_unsupported.go} | 2 +- .../helpers_windows.go} | 2 +- pkg/kubelet/dockershim/naming.go | 5 ++-- pkg/kubelet/dockertools/docker.go | 18 ++++++++---- pkg/kubelet/dockertools/docker_manager.go | 29 ------------------- test/e2e_node/image_id_test.go | 3 +- 9 files changed, 20 insertions(+), 45 deletions(-) rename pkg/kubelet/{dockertools/docker_manager_linux.go => dockershim/helpers_linux.go} (96%) rename pkg/kubelet/{dockertools/docker_manager_unsupported.go => dockershim/helpers_unsupported.go} (96%) rename pkg/kubelet/{dockertools/docker_manager_windows.go => dockershim/helpers_windows.go} (96%) delete mode 100644 pkg/kubelet/dockertools/docker_manager.go diff --git a/pkg/kubelet/dockershim/docker_container.go b/pkg/kubelet/dockershim/docker_container.go index 0e1b4a67e00..aaf59f42070 100644 --- a/pkg/kubelet/dockershim/docker_container.go +++ b/pkg/kubelet/dockershim/docker_container.go @@ -151,7 +151,7 @@ func (ds *dockerService) CreateContainer(podSandboxID string, config *runtimeapi if rOpts != nil { hc.Resources = dockercontainer.Resources{ Memory: rOpts.MemoryLimitInBytes, - MemorySwap: dockertools.DefaultMemorySwap(), + MemorySwap: DefaultMemorySwap(), CPUShares: rOpts.CpuShares, CPUQuota: rOpts.CpuQuota, CPUPeriod: rOpts.CpuPeriod, diff --git a/pkg/kubelet/dockershim/docker_sandbox.go b/pkg/kubelet/dockershim/docker_sandbox.go index 12978492eae..3d56fb20c9f 100644 --- a/pkg/kubelet/dockershim/docker_sandbox.go +++ b/pkg/kubelet/dockershim/docker_sandbox.go @@ -578,7 +578,7 @@ func sharesHostIpc(container *dockertypes.ContainerJSON) bool { func setSandboxResources(hc *dockercontainer.HostConfig) { hc.Resources = dockercontainer.Resources{ - MemorySwap: dockertools.DefaultMemorySwap(), + MemorySwap: DefaultMemorySwap(), CPUShares: defaultSandboxCPUshares, // Use docker's default cpu quota/period. } diff --git a/pkg/kubelet/dockertools/docker_manager_linux.go b/pkg/kubelet/dockershim/helpers_linux.go similarity index 96% rename from pkg/kubelet/dockertools/docker_manager_linux.go rename to pkg/kubelet/dockershim/helpers_linux.go index 5ef8f461971..8b762f545d6 100644 --- a/pkg/kubelet/dockertools/docker_manager_linux.go +++ b/pkg/kubelet/dockershim/helpers_linux.go @@ -16,7 +16,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package dockertools +package dockershim func DefaultMemorySwap() int64 { return 0 diff --git a/pkg/kubelet/dockertools/docker_manager_unsupported.go b/pkg/kubelet/dockershim/helpers_unsupported.go similarity index 96% rename from pkg/kubelet/dockertools/docker_manager_unsupported.go rename to pkg/kubelet/dockershim/helpers_unsupported.go index fe3100be9ff..f82bfeacc09 100644 --- a/pkg/kubelet/dockertools/docker_manager_unsupported.go +++ b/pkg/kubelet/dockershim/helpers_unsupported.go @@ -16,7 +16,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package dockertools +package dockershim func DefaultMemorySwap() int64 { return -1 diff --git a/pkg/kubelet/dockertools/docker_manager_windows.go b/pkg/kubelet/dockershim/helpers_windows.go similarity index 96% rename from pkg/kubelet/dockertools/docker_manager_windows.go rename to pkg/kubelet/dockershim/helpers_windows.go index 1246734b266..c5c4af13ac5 100644 --- a/pkg/kubelet/dockertools/docker_manager_windows.go +++ b/pkg/kubelet/dockershim/helpers_windows.go @@ -16,7 +16,7 @@ See the License for the specific language governing permissions and limitations under the License. */ -package dockertools +package dockershim func DefaultMemorySwap() int64 { return 0 diff --git a/pkg/kubelet/dockershim/naming.go b/pkg/kubelet/dockershim/naming.go index 012f28bd43c..f615c806428 100644 --- a/pkg/kubelet/dockershim/naming.go +++ b/pkg/kubelet/dockershim/naming.go @@ -23,7 +23,6 @@ import ( "strings" runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" - "k8s.io/kubernetes/pkg/kubelet/dockertools" "k8s.io/kubernetes/pkg/kubelet/leaky" ) @@ -51,9 +50,9 @@ const ( // Delimiter used to construct docker container names. nameDelimiter = "_" // DockerImageIDPrefix is the prefix of image id in container status. - DockerImageIDPrefix = dockertools.DockerPrefix + DockerImageIDPrefix = "docker://" // DockerPullableImageIDPrefix is the prefix of pullable image id in container status. - DockerPullableImageIDPrefix = dockertools.DockerPullablePrefix + DockerPullableImageIDPrefix = "docker-pullable://" ) func makeSandboxName(s *runtimeapi.PodSandboxConfig) string { diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index 6abd7195463..af19747dee1 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/pkg/kubelet/dockertools/docker.go @@ -37,15 +37,21 @@ import ( "k8s.io/kubernetes/pkg/credentialprovider" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" "k8s.io/kubernetes/pkg/kubelet/images" - "k8s.io/kubernetes/pkg/kubelet/leaky" ) const ( - PodInfraContainerName = leaky.PodInfraContainerName - DockerPrefix = "docker://" - DockerPullablePrefix = "docker-pullable://" - LogSuffix = "log" - ext4MaxFileNameLen = 255 + LogSuffix = "log" + ext4MaxFileNameLen = 255 + + DockerType = "docker" + + // https://docs.docker.com/engine/reference/api/docker_remote_api/ + // docker version should be at least 1.10.x + minimumDockerAPIVersion = "1.22" + + statusRunningPrefix = "Up" + statusExitedPrefix = "Exited" + statusCreatedPrefix = "Created" ) // DockerInterface is an abstract interface for testability. It abstracts the interface of docker client. diff --git a/pkg/kubelet/dockertools/docker_manager.go b/pkg/kubelet/dockertools/docker_manager.go deleted file mode 100644 index aaf4858456e..00000000000 --- a/pkg/kubelet/dockertools/docker_manager.go +++ /dev/null @@ -1,29 +0,0 @@ -/* -Copyright 2015 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 dockertools - -const ( - DockerType = "docker" - - // https://docs.docker.com/engine/reference/api/docker_remote_api/ - // docker version should be at least 1.10.x - minimumDockerAPIVersion = "1.22" - - statusRunningPrefix = "Up" - statusExitedPrefix = "Exited" - statusCreatedPrefix = "Created" -) diff --git a/test/e2e_node/image_id_test.go b/test/e2e_node/image_id_test.go index a968db5a8b1..ed473742894 100644 --- a/test/e2e_node/image_id_test.go +++ b/test/e2e_node/image_id_test.go @@ -19,7 +19,6 @@ package e2e_node import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/kubernetes/pkg/api/v1" - "k8s.io/kubernetes/pkg/kubelet/dockertools" "k8s.io/kubernetes/test/e2e/framework" "github.com/davecgh/go-spew/spew" @@ -62,6 +61,6 @@ var _ = framework.KubeDescribe("ImageID", func() { return } - Expect(status.ContainerStatuses[0].ImageID).To(Equal(dockertools.DockerPullablePrefix + busyBoxImage)) + Expect(status.ContainerStatuses[0].ImageID).To(ContainSubstring(busyBoxImage)) }) }) From 607bdd574db8edef51b70c8ec3331ae794e4aa63 Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Wed, 3 May 2017 09:55:40 -0700 Subject: [PATCH 2/3] Move docker keyring lookup test to pkg/credentailprovider Also remove unused image tests in docker_test.go --- pkg/credentialprovider/keyring_test.go | 117 ++++++++++++++++++ pkg/kubelet/dockertools/docker_test.go | 162 ------------------------- 2 files changed, 117 insertions(+), 162 deletions(-) diff --git a/pkg/credentialprovider/keyring_test.go b/pkg/credentialprovider/keyring_test.go index 4a865bedd86..771a6b60544 100644 --- a/pkg/credentialprovider/keyring_test.go +++ b/pkg/credentialprovider/keyring_test.go @@ -19,7 +19,10 @@ package credentialprovider import ( "encoding/base64" "fmt" + "reflect" "testing" + + dockertypes "github.com/docker/engine-api/types" ) func TestUrlsMatch(t *testing.T) { @@ -499,3 +502,117 @@ func TestLazyKeyring(t *testing.T) { t.Errorf("Unexpected number of Provide calls: %v", provider.Count) } } + +func TestDockerKeyringLookup(t *testing.T) { + ada := LazyAuthConfiguration{ + AuthConfig: dockertypes.AuthConfig{ + Username: "ada", + Password: "smash", + Email: "ada@example.com", + }, + } + + grace := LazyAuthConfiguration{ + AuthConfig: dockertypes.AuthConfig{ + Username: "grace", + Password: "squash", + Email: "grace@example.com", + }, + } + + dk := &BasicDockerKeyring{} + dk.Add(DockerConfig{ + "bar.example.com/pong": DockerConfigEntry{ + Username: grace.Username, + Password: grace.Password, + Email: grace.Email, + }, + "bar.example.com": DockerConfigEntry{ + Username: ada.Username, + Password: ada.Password, + Email: ada.Email, + }, + }) + + tests := []struct { + image string + match []LazyAuthConfiguration + ok bool + }{ + // direct match + {"bar.example.com", []LazyAuthConfiguration{ada}, true}, + + // direct match deeper than other possible matches + {"bar.example.com/pong", []LazyAuthConfiguration{grace, ada}, true}, + + // no direct match, deeper path ignored + {"bar.example.com/ping", []LazyAuthConfiguration{ada}, true}, + + // match first part of path token + {"bar.example.com/pongz", []LazyAuthConfiguration{grace, ada}, true}, + + // match regardless of sub-path + {"bar.example.com/pong/pang", []LazyAuthConfiguration{grace, ada}, true}, + + // no host match + {"example.com", []LazyAuthConfiguration{}, false}, + {"foo.example.com", []LazyAuthConfiguration{}, false}, + } + + for i, tt := range tests { + match, ok := dk.Lookup(tt.image) + if tt.ok != ok { + t.Errorf("case %d: expected ok=%t, got %t", i, tt.ok, ok) + } + + if !reflect.DeepEqual(tt.match, match) { + t.Errorf("case %d: expected match=%#v, got %#v", i, tt.match, match) + } + } +} + +// This validates that dockercfg entries with a scheme and url path are properly matched +// by images that only match the hostname. +// NOTE: the above covers the case of a more specific match trumping just hostname. +func TestIssue3797(t *testing.T) { + rex := LazyAuthConfiguration{ + AuthConfig: dockertypes.AuthConfig{ + Username: "rex", + Password: "tiny arms", + Email: "rex@example.com", + }, + } + + dk := &BasicDockerKeyring{} + dk.Add(DockerConfig{ + "https://quay.io/v1/": DockerConfigEntry{ + Username: rex.Username, + Password: rex.Password, + Email: rex.Email, + }, + }) + + tests := []struct { + image string + match []LazyAuthConfiguration + ok bool + }{ + // direct match + {"quay.io", []LazyAuthConfiguration{rex}, true}, + + // partial matches + {"quay.io/foo", []LazyAuthConfiguration{rex}, true}, + {"quay.io/foo/bar", []LazyAuthConfiguration{rex}, true}, + } + + for i, tt := range tests { + match, ok := dk.Lookup(tt.image) + if tt.ok != ok { + t.Errorf("case %d: expected ok=%t, got %t", i, tt.ok, ok) + } + + if !reflect.DeepEqual(tt.match, match) { + t.Errorf("case %d: expected match=%#v, got %#v", i, tt.match, match) + } + } +} diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index adf636b2f72..6ea62b442d7 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/pkg/kubelet/dockertools/docker_test.go @@ -544,168 +544,6 @@ func TestPullWithSecrets(t *testing.T) { } } -func TestDockerKeyringLookupFails(t *testing.T) { - fakeKeyring := &credentialprovider.FakeKeyring{} - fakeClient := NewFakeDockerClient() - fakeClient.InjectError("pull", fmt.Errorf("test error")) - - dp := dockerPuller{ - client: fakeClient, - keyring: fakeKeyring, - } - - err := dp.Pull("host/repository/image:version", []v1.Secret{}) - if err == nil { - t.Errorf("unexpected non-error") - } - msg := "image pull failed for host/repository/image:version, this may be because there are no credentials on this request. details: (test error)" - if err.Error() != msg { - t.Errorf("expected: %s, saw: %s", msg, err.Error()) - } -} - -func TestDockerKeyringLookup(t *testing.T) { - ada := credentialprovider.LazyAuthConfiguration{ - AuthConfig: dockertypes.AuthConfig{ - Username: "ada", - Password: "smash", - Email: "ada@example.com", - }, - } - - grace := credentialprovider.LazyAuthConfiguration{ - AuthConfig: dockertypes.AuthConfig{ - Username: "grace", - Password: "squash", - Email: "grace@example.com", - }, - } - - dk := &credentialprovider.BasicDockerKeyring{} - dk.Add(credentialprovider.DockerConfig{ - "bar.example.com/pong": credentialprovider.DockerConfigEntry{ - Username: grace.Username, - Password: grace.Password, - Email: grace.Email, - }, - "bar.example.com": credentialprovider.DockerConfigEntry{ - Username: ada.Username, - Password: ada.Password, - Email: ada.Email, - }, - }) - - tests := []struct { - image string - match []credentialprovider.LazyAuthConfiguration - ok bool - }{ - // direct match - {"bar.example.com", []credentialprovider.LazyAuthConfiguration{ada}, true}, - - // direct match deeper than other possible matches - {"bar.example.com/pong", []credentialprovider.LazyAuthConfiguration{grace, ada}, true}, - - // no direct match, deeper path ignored - {"bar.example.com/ping", []credentialprovider.LazyAuthConfiguration{ada}, true}, - - // match first part of path token - {"bar.example.com/pongz", []credentialprovider.LazyAuthConfiguration{grace, ada}, true}, - - // match regardless of sub-path - {"bar.example.com/pong/pang", []credentialprovider.LazyAuthConfiguration{grace, ada}, true}, - - // no host match - {"example.com", []credentialprovider.LazyAuthConfiguration{}, false}, - {"foo.example.com", []credentialprovider.LazyAuthConfiguration{}, false}, - } - - for i, tt := range tests { - match, ok := dk.Lookup(tt.image) - if tt.ok != ok { - t.Errorf("case %d: expected ok=%t, got %t", i, tt.ok, ok) - } - - if !reflect.DeepEqual(tt.match, match) { - t.Errorf("case %d: expected match=%#v, got %#v", i, tt.match, match) - } - } -} - -// This validates that dockercfg entries with a scheme and url path are properly matched -// by images that only match the hostname. -// NOTE: the above covers the case of a more specific match trumping just hostname. -func TestIssue3797(t *testing.T) { - rex := credentialprovider.LazyAuthConfiguration{ - AuthConfig: dockertypes.AuthConfig{ - Username: "rex", - Password: "tiny arms", - Email: "rex@example.com", - }, - } - - dk := &credentialprovider.BasicDockerKeyring{} - dk.Add(credentialprovider.DockerConfig{ - "https://quay.io/v1/": credentialprovider.DockerConfigEntry{ - Username: rex.Username, - Password: rex.Password, - Email: rex.Email, - }, - }) - - tests := []struct { - image string - match []credentialprovider.LazyAuthConfiguration - ok bool - }{ - // direct match - {"quay.io", []credentialprovider.LazyAuthConfiguration{rex}, true}, - - // partial matches - {"quay.io/foo", []credentialprovider.LazyAuthConfiguration{rex}, true}, - {"quay.io/foo/bar", []credentialprovider.LazyAuthConfiguration{rex}, true}, - } - - for i, tt := range tests { - match, ok := dk.Lookup(tt.image) - if tt.ok != ok { - t.Errorf("case %d: expected ok=%t, got %t", i, tt.ok, ok) - } - - if !reflect.DeepEqual(tt.match, match) { - t.Errorf("case %d: expected match=%#v, got %#v", i, tt.match, match) - } - } -} - -type imageTrackingDockerClient struct { - *FakeDockerClient - imageName string -} - -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 -} - -func TestGetImageRef(t *testing.T) { - cl := &imageTrackingDockerClient{NewFakeDockerClient(), ""} - puller := &dockerPuller{ - client: cl, - } - _, _ = puller.GetImageRef("abc:123") - if cl.imageName != "abc:123" { - t.Errorf("expected inspection of image abc:123, instead inspected image %v", cl.imageName) - } -} - const letterBytes = "abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ" func randStringBytes(n int) string { From 51188e6f7041971aaa2a767ddf1f270604cd869c Mon Sep 17 00:00:00 2001 From: Yu-Ju Hong Date: Wed, 3 May 2017 11:17:19 -0700 Subject: [PATCH 3/3] Update bazel files --- pkg/credentialprovider/BUILD | 1 + pkg/kubelet/dockershim/BUILD | 1 + pkg/kubelet/dockertools/BUILD | 3 --- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/pkg/credentialprovider/BUILD b/pkg/credentialprovider/BUILD index 8dd761fd809..80e82cd3a9f 100644 --- a/pkg/credentialprovider/BUILD +++ b/pkg/credentialprovider/BUILD @@ -35,6 +35,7 @@ go_test( ], library = ":go_default_library", tags = ["automanaged"], + deps = ["//vendor/github.com/docker/engine-api/types:go_default_library"], ) filegroup( diff --git a/pkg/kubelet/dockershim/BUILD b/pkg/kubelet/dockershim/BUILD index 10d0045f0a4..bafd2c884cc 100644 --- a/pkg/kubelet/dockershim/BUILD +++ b/pkg/kubelet/dockershim/BUILD @@ -23,6 +23,7 @@ go_library( "docker_streaming.go", "exec.go", "helpers.go", + "helpers_linux.go", "naming.go", "security_context.go", ], diff --git a/pkg/kubelet/dockertools/BUILD b/pkg/kubelet/dockertools/BUILD index 400f22b0aaa..0c15ab0cdf8 100644 --- a/pkg/kubelet/dockertools/BUILD +++ b/pkg/kubelet/dockertools/BUILD @@ -12,8 +12,6 @@ go_library( name = "go_default_library", srcs = [ "docker.go", - "docker_manager.go", - "docker_manager_linux.go", "fake_docker_client.go", "instrumented_docker.go", "kube_docker_client.go", @@ -24,7 +22,6 @@ go_library( "//pkg/credentialprovider:go_default_library", "//pkg/kubelet/container:go_default_library", "//pkg/kubelet/images:go_default_library", - "//pkg/kubelet/leaky:go_default_library", "//pkg/kubelet/metrics:go_default_library", "//vendor/github.com/docker/distribution/digest:go_default_library", "//vendor/github.com/docker/distribution/reference:go_default_library",