diff --git a/pkg/kubelet/api/testing/fake_image_service.go b/pkg/kubelet/api/testing/fake_image_service.go index 14c35642e2d..6f9a0a63b41 100644 --- a/pkg/kubelet/api/testing/fake_image_service.go +++ b/pkg/kubelet/api/testing/fake_image_service.go @@ -18,6 +18,9 @@ package testing import ( "sync" + "testing" + + "github.com/stretchr/testify/assert" runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" "k8s.io/kubernetes/pkg/kubelet/util/sliceutils" @@ -29,6 +32,8 @@ type FakeImageService struct { FakeImageSize uint64 Called []string Images map[string]*runtimeapi.Image + + pulledImages []*pulledImage } func (r *FakeImageService) SetFakeImages(images []string) { @@ -97,6 +102,7 @@ func (r *FakeImageService) PullImage(image *runtimeapi.ImageSpec, auth *runtimea r.Called = append(r.Called, "PullImage") + r.pulledImages = append(r.pulledImages, &pulledImage{imageSpec: image, authConfig: auth}) // ImageID should be randomized for real container runtime, but here just use // image's name for easily making fake images. imageID := image.Image @@ -128,3 +134,15 @@ func (r *FakeImageService) ImageFsInfo() (*runtimeapi.FsInfo, error) { return nil, nil } + +func (r *FakeImageService) AssertImagePulledWithAuth(t *testing.T, image *runtimeapi.ImageSpec, auth *runtimeapi.AuthConfig, failMsg string) { + r.Lock() + defer r.Unlock() + expected := &pulledImage{imageSpec: image, authConfig: auth} + assert.Contains(t, r.pulledImages, expected, failMsg) +} + +type pulledImage struct { + imageSpec *runtimeapi.ImageSpec + authConfig *runtimeapi.AuthConfig +} diff --git a/pkg/kubelet/dockershim/docker_image.go b/pkg/kubelet/dockershim/docker_image.go index 70bca7fa5e2..50ac2111a0a 100644 --- a/pkg/kubelet/dockershim/docker_image.go +++ b/pkg/kubelet/dockershim/docker_image.go @@ -18,10 +18,12 @@ package dockershim import ( "fmt" + "net/http" + "github.com/docker/docker/pkg/jsonmessage" dockertypes "github.com/docker/engine-api/types" - runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" + runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" "k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker" ) @@ -80,7 +82,7 @@ func (ds *dockerService) PullImage(image *runtimeapi.ImageSpec, auth *runtimeapi dockertypes.ImagePullOptions{}, ) if err != nil { - return "", err + return "", filterHTTPError(err, image.Image) } return getImageRef(ds.client, image.Image) @@ -127,3 +129,18 @@ func getImageRef(client libdocker.Interface, image string) (string, error) { func (ds *dockerService) ImageFsInfo() (*runtimeapi.FsInfo, error) { return nil, fmt.Errorf("not implemented") } + +func filterHTTPError(err error, image string) error { + // docker/docker/pull/11314 prints detailed error info for docker pull. + // When it hits 502, it returns a verbose html output including an inline svg, + // which makes the output of kubectl get pods much harder to parse. + // Here converts such verbose output to a concise one. + jerr, ok := err.(*jsonmessage.JSONError) + if ok && (jerr.Code == http.StatusBadGateway || + jerr.Code == http.StatusServiceUnavailable || + jerr.Code == http.StatusGatewayTimeout) { + return fmt.Errorf("RegistryUnavailable: %v", err) + } + return err + +} diff --git a/pkg/kubelet/dockershim/docker_image_test.go b/pkg/kubelet/dockershim/docker_image_test.go index 2aeb5175a14..5389ed0f332 100644 --- a/pkg/kubelet/dockershim/docker_image_test.go +++ b/pkg/kubelet/dockershim/docker_image_test.go @@ -17,12 +17,14 @@ limitations under the License. package dockershim import ( + "fmt" "testing" + "github.com/docker/docker/pkg/jsonmessage" dockertypes "github.com/docker/engine-api/types" + "github.com/stretchr/testify/assert" runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" - "k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker" ) @@ -44,3 +46,29 @@ func TestRemoveImageWithMultipleTags(t *testing.T) { libdocker.NewCalledDetail("remove_image", []interface{}{"foo", dockertypes.ImageRemoveOptions{PruneChildren: true}}), libdocker.NewCalledDetail("remove_image", []interface{}{"bar", dockertypes.ImageRemoveOptions{PruneChildren: true}})) } + +func TestPullWithJSONError(t *testing.T) { + ds, fakeDocker, _ := newTestDockerService() + tests := map[string]struct { + image *runtimeapi.ImageSpec + err error + expectedError string + }{ + "Json error": { + &runtimeapi.ImageSpec{Image: "ubuntu"}, + &jsonmessage.JSONError{Code: 50, Message: "Json error"}, + "Json error", + }, + "Bad gateway": { + &runtimeapi.ImageSpec{Image: "ubuntu"}, + &jsonmessage.JSONError{Code: 502, Message: "\n\n
\n \n \nWe have been contacted of this error, feel free to check out status.docker.com\n to see if there is a bigger issue.
\n\n \n"}, + "RegistryUnavailable", + }, + } + for key, test := range tests { + fakeDocker.InjectError("pull", test.err) + _, err := ds.PullImage(test.image, &runtimeapi.AuthConfig{}) + assert.Error(t, err, fmt.Sprintf("TestCase [%s]", key)) + assert.Contains(t, err.Error(), test.expectedError) + } +} diff --git a/pkg/kubelet/dockertools/BUILD b/pkg/kubelet/dockertools/BUILD deleted file mode 100644 index f4229dd5f50..00000000000 --- a/pkg/kubelet/dockertools/BUILD +++ /dev/null @@ -1,54 +0,0 @@ -package(default_visibility = ["//visibility:public"]) - -licenses(["notice"]) - -load( - "@io_bazel_rules_go//go:def.bzl", - "go_library", - "go_test", -) - -go_library( - name = "go_default_library", - srcs = ["docker.go"], - tags = ["automanaged"], - deps = [ - "//pkg/api/v1:go_default_library", - "//pkg/credentialprovider:go_default_library", - "//pkg/kubelet/dockershim/libdocker:go_default_library", - "//pkg/kubelet/images:go_default_library", - "//vendor/github.com/docker/docker/pkg/jsonmessage:go_default_library", - "//vendor/github.com/docker/engine-api/types:go_default_library", - "//vendor/github.com/golang/glog:go_default_library", - "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library", - ], -) - -go_test( - name = "go_default_test", - srcs = ["docker_test.go"], - library = ":go_default_library", - tags = [ - "automanaged", - ], - deps = [ - "//pkg/api/v1:go_default_library", - "//pkg/credentialprovider:go_default_library", - "//pkg/kubelet/dockershim/libdocker:go_default_library", - "//pkg/kubelet/images:go_default_library", - "//vendor/github.com/docker/docker/pkg/jsonmessage:go_default_library", - ], -) - -filegroup( - name = "package-srcs", - srcs = glob(["**"]), - tags = ["automanaged"], - visibility = ["//visibility:private"], -) - -filegroup( - name = "all-srcs", - srcs = [":package-srcs"], - tags = ["automanaged"], -) diff --git a/pkg/kubelet/dockertools/OWNERS b/pkg/kubelet/dockertools/OWNERS deleted file mode 100644 index 1d8978b26f7..00000000000 --- a/pkg/kubelet/dockertools/OWNERS +++ /dev/null @@ -1,3 +0,0 @@ -approvers: -- Random-Liu -- yujuhong diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go deleted file mode 100644 index 384403cab8e..00000000000 --- a/pkg/kubelet/dockertools/docker.go +++ /dev/null @@ -1,141 +0,0 @@ -/* -Copyright 2014 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 - -import ( - "fmt" - "net/http" - "strings" - - "github.com/docker/docker/pkg/jsonmessage" - dockertypes "github.com/docker/engine-api/types" - "github.com/golang/glog" - utilerrors "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/kubernetes/pkg/api/v1" - "k8s.io/kubernetes/pkg/credentialprovider" - "k8s.io/kubernetes/pkg/kubelet/dockershim/libdocker" - "k8s.io/kubernetes/pkg/kubelet/images" -) - -// DockerPuller is an abstract interface for testability. It abstracts image pull operations. -// DockerPuller is *not* in use anywhere in the codebase. -// TODO: Examine whether we can migrate the unit tests and remove the code. -type DockerPuller interface { - Pull(image string, secrets []v1.Secret) error - GetImageRef(image string) (string, error) -} - -// dockerPuller is the default implementation of DockerPuller. -type dockerPuller struct { - client libdocker.Interface - keyring credentialprovider.DockerKeyring -} - -// newDockerPuller creates a new instance of the default implementation of DockerPuller. -func newDockerPuller(client libdocker.Interface) DockerPuller { - return &dockerPuller{ - client: client, - keyring: credentialprovider.NewDockerKeyring(), - } -} - -func filterHTTPError(err error, image string) error { - // docker/docker/pull/11314 prints detailed error info for docker pull. - // When it hits 502, it returns a verbose html output including an inline svg, - // which makes the output of kubectl get pods much harder to parse. - // Here converts such verbose output to a concise one. - jerr, ok := err.(*jsonmessage.JSONError) - if ok && (jerr.Code == http.StatusBadGateway || - jerr.Code == http.StatusServiceUnavailable || - jerr.Code == http.StatusGatewayTimeout) { - glog.V(2).Infof("Pulling image %q failed: %v", image, err) - return images.RegistryUnavailable - } else { - return err - } -} - -func (p dockerPuller) Pull(image string, secrets []v1.Secret) error { - keyring, err := credentialprovider.MakeDockerKeyring(secrets, p.keyring) - if err != nil { - return err - } - - // The only used image pull option RegistryAuth will be set in kube_docker_client - opts := dockertypes.ImagePullOptions{} - - creds, haveCredentials := keyring.Lookup(image) - if !haveCredentials { - glog.V(1).Infof("Pulling image %s without credentials", image) - - err := p.client.PullImage(image, dockertypes.AuthConfig{}, opts) - if err == nil { - // Sometimes PullImage failed with no error returned. - imageRef, ierr := p.GetImageRef(image) - if ierr != nil { - glog.Warningf("Failed to inspect image %s: %v", image, ierr) - } - if imageRef == "" { - return fmt.Errorf("image pull failed for unknown error") - } - return nil - } - - // Image spec: [We have been contacted of this error, feel free to check out status.docker.com\n to see if there is a bigger issue.
\n\n \n"}, - images.RegistryUnavailable.Error(), - }, - } - for i, test := range tests { - fakeKeyring := &credentialprovider.FakeKeyring{} - fakeClient := libdocker.NewFakeDockerClient() - fakeClient.InjectError("pull", test.err) - - puller := &dockerPuller{ - client: fakeClient, - keyring: fakeKeyring, - } - err := puller.Pull(test.imageName, []v1.Secret{}) - if err == nil || !strings.Contains(err.Error(), test.expectedError) { - t.Errorf("%s: expect error %s, got : %s", i, test.expectedError, err) - continue - } - } -} - -func TestPullWithSecrets(t *testing.T) { - // auth value is equivalent to: "username":"passed-user","password":"passed-password" - dockerCfg := map[string]map[string]string{"index.docker.io/v1/": {"email": "passed-email", "auth": "cGFzc2VkLXVzZXI6cGFzc2VkLXBhc3N3b3Jk"}} - dockercfgContent, err := json.Marshal(dockerCfg) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - - dockerConfigJson := map[string]map[string]map[string]string{"auths": dockerCfg} - dockerConfigJsonContent, err := json.Marshal(dockerConfigJson) - if err != nil { - t.Errorf("unexpected error: %v", err) - } - - tests := map[string]struct { - imageName string - passedSecrets []v1.Secret - builtInDockerConfig credentialprovider.DockerConfig - expectedPulls []string - }{ - "no matching secrets": { - "ubuntu", - []v1.Secret{}, - credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{}), - []string{"ubuntu using {}"}, - }, - "default keyring secrets": { - "ubuntu", - []v1.Secret{}, - credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{ - "index.docker.io/v1/": {Username: "built-in", Password: "password", Email: "email", Provider: nil}, - }), - []string{`ubuntu using {"username":"built-in","password":"password","email":"email"}`}, - }, - "default keyring secrets unused": { - "ubuntu", - []v1.Secret{}, - credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{ - "extraneous": {Username: "built-in", Password: "password", Email: "email", Provider: nil}, - }), - []string{`ubuntu using {}`}, - }, - "builtin keyring secrets, but use passed": { - "ubuntu", - []v1.Secret{{Type: v1.SecretTypeDockercfg, Data: map[string][]byte{v1.DockerConfigKey: dockercfgContent}}}, - credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{ - "index.docker.io/v1/": {Username: "built-in", Password: "password", Email: "email", Provider: nil}, - }), - []string{`ubuntu using {"username":"passed-user","password":"passed-password","email":"passed-email"}`}, - }, - "builtin keyring secrets, but use passed with new docker config": { - "ubuntu", - []v1.Secret{{Type: v1.SecretTypeDockerConfigJson, Data: map[string][]byte{v1.DockerConfigJsonKey: dockerConfigJsonContent}}}, - credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{ - "index.docker.io/v1/": {Username: "built-in", Password: "password", Email: "email", Provider: nil}, - }), - []string{`ubuntu using {"username":"passed-user","password":"passed-password","email":"passed-email"}`}, - }, - } - for i, test := range tests { - builtInKeyRing := &credentialprovider.BasicDockerKeyring{} - builtInKeyRing.Add(test.builtInDockerConfig) - - fakeClient := libdocker.NewFakeDockerClient() - - dp := dockerPuller{ - client: fakeClient, - keyring: builtInKeyRing, - } - - err := dp.Pull(test.imageName, test.passedSecrets) - if err != nil { - t.Errorf("%s: unexpected non-nil err: %s", i, err) - continue - } - if err := fakeClient.AssertImagesPulledMsgs(test.expectedPulls); err != nil { - t.Errorf("images pulled do not match the expected: %v", err) - } - } -} diff --git a/pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go b/pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go index ec4f067fa3d..660d1280a80 100644 --- a/pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go +++ b/pkg/kubelet/kuberuntime/fake_kuberuntime_manager.go @@ -56,7 +56,7 @@ func (f *fakePodGetter) GetPodByUID(uid types.UID) (*v1.Pod, bool) { return pod, found } -func NewFakeKubeRuntimeManager(runtimeService internalapi.RuntimeService, imageService internalapi.ImageManagerService, machineInfo *cadvisorapi.MachineInfo, osInterface kubecontainer.OSInterface, runtimeHelper kubecontainer.RuntimeHelper) (*kubeGenericRuntimeManager, error) { +func NewFakeKubeRuntimeManager(runtimeService internalapi.RuntimeService, imageService internalapi.ImageManagerService, machineInfo *cadvisorapi.MachineInfo, osInterface kubecontainer.OSInterface, runtimeHelper kubecontainer.RuntimeHelper, keyring credentialprovider.DockerKeyring) (*kubeGenericRuntimeManager, error) { recorder := &record.FakeRecorder{} kubeRuntimeManager := &kubeGenericRuntimeManager{ recorder: recorder, @@ -68,7 +68,7 @@ func NewFakeKubeRuntimeManager(runtimeService internalapi.RuntimeService, imageS runtimeHelper: runtimeHelper, runtimeService: runtimeService, imageService: imageService, - keyring: credentialprovider.NewDockerKeyring(), + keyring: keyring, } typedVersion, err := runtimeService.Version(kubeRuntimeAPIVersion) diff --git a/pkg/kubelet/kuberuntime/kuberuntime_image_test.go b/pkg/kubelet/kuberuntime/kuberuntime_image_test.go index 7ed23a9e94b..496e91fdd28 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_image_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_image_test.go @@ -17,10 +17,16 @@ limitations under the License. package kuberuntime import ( + "encoding/json" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/util/sets" + "k8s.io/kubernetes/pkg/api/v1" + "k8s.io/kubernetes/pkg/credentialprovider" + runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" ) @@ -94,3 +100,74 @@ func TestImageStats(t *testing.T) { expectedStats := &kubecontainer.ImageStats{TotalStorageBytes: imageSize * uint64(len(images))} assert.Equal(t, expectedStats, actualStats) } + +func TestPullWithSecrets(t *testing.T) { + // auth value is equivalent to: "username":"passed-user","password":"passed-password" + dockerCfg := map[string]map[string]string{"index.docker.io/v1/": {"email": "passed-email", "auth": "cGFzc2VkLXVzZXI6cGFzc2VkLXBhc3N3b3Jk"}} + dockercfgContent, err := json.Marshal(dockerCfg) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + dockerConfigJson := map[string]map[string]map[string]string{"auths": dockerCfg} + dockerConfigJsonContent, err := json.Marshal(dockerConfigJson) + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + tests := map[string]struct { + imageName string + passedSecrets []v1.Secret + builtInDockerConfig credentialprovider.DockerConfig + expectedAuth *runtimeapi.AuthConfig + }{ + "no matching secrets": { + "ubuntu", + []v1.Secret{}, + credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{}), + nil, + }, + "default keyring secrets": { + "ubuntu", + []v1.Secret{}, + credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{ + "index.docker.io/v1/": {Username: "built-in", Password: "password", Provider: nil}, + }), + &runtimeapi.AuthConfig{Username: "built-in", Password: "password"}, + }, + "default keyring secrets unused": { + "ubuntu", + []v1.Secret{}, + credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{ + "extraneous": {Username: "built-in", Password: "password", Provider: nil}, + }), + nil, + }, + "builtin keyring secrets, but use passed": { + "ubuntu", + []v1.Secret{{Type: v1.SecretTypeDockercfg, Data: map[string][]byte{v1.DockerConfigKey: dockercfgContent}}}, + credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{ + "index.docker.io/v1/": {Username: "built-in", Password: "password", Provider: nil}, + }), + &runtimeapi.AuthConfig{Username: "passed-user", Password: "passed-password"}, + }, + "builtin keyring secrets, but use passed with new docker config": { + "ubuntu", + []v1.Secret{{Type: v1.SecretTypeDockerConfigJson, Data: map[string][]byte{v1.DockerConfigJsonKey: dockerConfigJsonContent}}}, + credentialprovider.DockerConfig(map[string]credentialprovider.DockerConfigEntry{ + "index.docker.io/v1/": {Username: "built-in", Password: "password", Provider: nil}, + }), + &runtimeapi.AuthConfig{Username: "passed-user", Password: "passed-password"}, + }, + } + for description, test := range tests { + builtInKeyRing := &credentialprovider.BasicDockerKeyring{} + builtInKeyRing.Add(test.builtInDockerConfig) + _, fakeImageService, fakeManager, err := customTestRuntimeManager(builtInKeyRing) + require.NoError(t, err) + + _, err = fakeManager.PullImage(kubecontainer.ImageSpec{Image: test.imageName}, test.passedSecrets) + require.NoError(t, err) + fakeImageService.AssertImagePulledWithAuth(t, &runtimeapi.ImageSpec{Image: test.imageName}, test.expectedAuth, description) + } +} diff --git a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go index 65ec763e6a7..ffdc8c7abd6 100644 --- a/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go +++ b/pkg/kubelet/kuberuntime/kuberuntime_manager_test.go @@ -24,11 +24,13 @@ import ( cadvisorapi "github.com/google/cadvisor/info/v1" "github.com/stretchr/testify/assert" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" kubetypes "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/util/flowcontrol" "k8s.io/kubernetes/pkg/api/v1" + "k8s.io/kubernetes/pkg/credentialprovider" apitest "k8s.io/kubernetes/pkg/kubelet/api/testing" runtimeapi "k8s.io/kubernetes/pkg/kubelet/api/v1alpha1/runtime" kubecontainer "k8s.io/kubernetes/pkg/kubelet/container" @@ -40,6 +42,10 @@ var ( ) func createTestRuntimeManager() (*apitest.FakeRuntimeService, *apitest.FakeImageService, *kubeGenericRuntimeManager, error) { + return customTestRuntimeManager(&credentialprovider.BasicDockerKeyring{}) +} + +func customTestRuntimeManager(keyring *credentialprovider.BasicDockerKeyring) (*apitest.FakeRuntimeService, *apitest.FakeImageService, *kubeGenericRuntimeManager, error) { fakeRuntimeService := apitest.NewFakeRuntimeService() fakeImageService := apitest.NewFakeImageService() // Only an empty machineInfo is needed here, because in unit test all containers are besteffort, @@ -47,7 +53,7 @@ func createTestRuntimeManager() (*apitest.FakeRuntimeService, *apitest.FakeImage // we may want to set memory capacity. machineInfo := &cadvisorapi.MachineInfo{} osInterface := &containertest.FakeOS{} - manager, err := NewFakeKubeRuntimeManager(fakeRuntimeService, fakeImageService, machineInfo, osInterface, &containertest.FakeRuntimeHelper{}) + manager, err := NewFakeKubeRuntimeManager(fakeRuntimeService, fakeImageService, machineInfo, osInterface, &containertest.FakeRuntimeHelper{}, keyring) return fakeRuntimeService, fakeImageService, manager, err }