From 7b07960758ca6f70572c1dbb39b36ebe44b56974 Mon Sep 17 00:00:00 2001 From: Andy Goldstein Date: Mon, 16 Mar 2015 20:51:20 -0400 Subject: [PATCH] Use docker's ParseRepositoryTag when pulling --- pkg/kubelet/dockertools/docker.go | 44 ++--------- pkg/kubelet/dockertools/docker_test.go | 75 ++++++++++++++----- pkg/kubelet/dockertools/fake_docker_client.go | 6 +- 3 files changed, 69 insertions(+), 56 deletions(-) diff --git a/pkg/kubelet/dockertools/docker.go b/pkg/kubelet/dockertools/docker.go index 99da3ad0aeb..83cf3e0621a 100644 --- a/pkg/kubelet/dockertools/docker.go +++ b/pkg/kubelet/dockertools/docker.go @@ -35,6 +35,7 @@ import ( "github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/leaky" "github.com/GoogleCloudPlatform/kubernetes/pkg/types" "github.com/GoogleCloudPlatform/kubernetes/pkg/util" + "github.com/docker/docker/pkg/parsers" docker "github.com/fsouza/go-dockerclient" "github.com/golang/glog" ) @@ -324,8 +325,12 @@ func NewDockerContainerCommandRunner(client DockerInterface) ContainerCommandRun return &dockerContainerCommandRunner{client: client} } +func parseImageName(image string) (string, string) { + return parsers.ParseRepositoryTag(image) +} + func (p dockerPuller) Pull(image string) error { - image, tag := parseImageName(image) + repoToPull, tag := parseImageName(image) // If no tag was specified, use the default "latest". if len(tag) == 0 { @@ -333,11 +338,11 @@ func (p dockerPuller) Pull(image string) error { } opts := docker.PullImageOptions{ - Repository: image, + Repository: repoToPull, Tag: tag, } - creds, ok := p.keyring.Lookup(image) + creds, ok := p.keyring.Lookup(repoToPull) if !ok { glog.V(1).Infof("Pulling image %s without credentials", image) } @@ -378,16 +383,6 @@ func (p dockerPuller) IsImagePresent(image string) (bool, error) { return false, err } -// RequireLatestImage returns if the user wants the latest image -func RequireLatestImage(name string) bool { - _, tag := parseImageName(name) - - if tag == "latest" { - return true - } - return false -} - func (p throttledDockerPuller) IsImagePresent(name string) (bool, error) { return p.puller.IsImagePresent(name) } @@ -770,29 +765,6 @@ func GetRunningContainers(client DockerInterface, ids []string) ([]*docker.Conta return result, nil } -// Parses image name including a tag and returns image name and tag. -// TODO: Future Docker versions can parse the tag on daemon side, see -// https://github.com/dotcloud/docker/issues/6876 -// So this can be deprecated at some point. -func parseImageName(image string) (string, string) { - tag := "" - parts := strings.SplitN(image, "/", 2) - repo := "" - if len(parts) == 2 { - repo = parts[0] - image = parts[1] - } - parts = strings.SplitN(image, ":", 2) - if len(parts) == 2 { - image = parts[0] - tag = parts[1] - } - if repo != "" { - image = fmt.Sprintf("%s/%s", repo, image) - } - return image, tag -} - // Get a docker endpoint, either from the string passed in, or $DOCKER_HOST environment variables func getDockerEndpoint(dockerEndpoint string) string { var endpoint string diff --git a/pkg/kubelet/dockertools/docker_test.go b/pkg/kubelet/dockertools/docker_test.go index afb881ae730..2e11d725d68 100644 --- a/pkg/kubelet/dockertools/docker_test.go +++ b/pkg/kubelet/dockertools/docker_test.go @@ -178,26 +178,63 @@ func TestDockerContainerCommand(t *testing.T) { t.Errorf("unexpected command args: %s", cmd.Args) } } - -var parseImageNameTests = []struct { - imageName string - name string - tag string -}{ - {"ubuntu", "ubuntu", ""}, - {"ubuntu:2342", "ubuntu", "2342"}, - {"ubuntu:latest", "ubuntu", "latest"}, - {"foo/bar:445566", "foo/bar", "445566"}, - {"registry.example.com:5000/foobar", "registry.example.com:5000/foobar", ""}, - {"registry.example.com:5000/foobar:5342", "registry.example.com:5000/foobar", "5342"}, - {"registry.example.com:5000/foobar:latest", "registry.example.com:5000/foobar", "latest"}, +func TestParseImageName(t *testing.T) { + tests := []struct { + imageName string + name string + tag string + }{ + {"ubuntu", "ubuntu", ""}, + {"ubuntu:2342", "ubuntu", "2342"}, + {"ubuntu:latest", "ubuntu", "latest"}, + {"foo/bar:445566", "foo/bar", "445566"}, + {"registry.example.com:5000/foobar", "registry.example.com:5000/foobar", ""}, + {"registry.example.com:5000/foobar:5342", "registry.example.com:5000/foobar", "5342"}, + {"registry.example.com:5000/foobar:latest", "registry.example.com:5000/foobar", "latest"}, + } + for _, test := range tests { + name, tag := parseImageName(test.imageName) + if name != test.name || tag != test.tag { + t.Errorf("Expected name/tag: %s/%s, got %s/%s", test.name, test.tag, name, tag) + } + } } -func TestParseImageName(t *testing.T) { - for _, tt := range parseImageNameTests { - name, tag := parseImageName(tt.imageName) - if name != tt.name || tag != tt.tag { - t.Errorf("Expected name/tag: %s/%s, got %s/%s", tt.name, tt.tag, name, tag) +func TestPull(t *testing.T) { + tests := []struct { + imageName string + expectedImage string + }{ + {"ubuntu", "ubuntu:latest"}, + {"ubuntu:2342", "ubuntu:2342"}, + {"ubuntu:latest", "ubuntu:latest"}, + {"foo/bar:445566", "foo/bar:445566"}, + {"registry.example.com:5000/foobar", "registry.example.com:5000/foobar:latest"}, + {"registry.example.com:5000/foobar:5342", "registry.example.com:5000/foobar:5342"}, + {"registry.example.com:5000/foobar:latest", "registry.example.com:5000/foobar:latest"}, + } + for _, test := range tests { + fakeKeyring := &credentialprovider.FakeKeyring{} + fakeClient := &FakeDockerClient{} + + dp := dockerPuller{ + client: fakeClient, + keyring: fakeKeyring, + } + + err := dp.Pull(test.imageName) + if err != nil { + t.Errorf("unexpected non-nil err: %s", err) + continue + } + + if e, a := 1, len(fakeClient.pulled); e != a { + t.Errorf("%s: expected 1 pulled image, got %d: %v", test.imageName, a, fakeClient.pulled) + continue + } + + if e, a := test.expectedImage, fakeClient.pulled[0]; e != a { + t.Errorf("%s: expected pull of %q, but got %q", test.imageName, e, a) } } } @@ -217,7 +254,7 @@ func TestDockerKeyringLookupFails(t *testing.T) { if err == nil { t.Errorf("unexpected non-error") } - msg := "image pull failed for host/repository/image, this may be because there are no credentials on this request. details: (test 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()) } diff --git a/pkg/kubelet/dockertools/fake_docker_client.go b/pkg/kubelet/dockertools/fake_docker_client.go index 01d14cb2503..ad8f46915f2 100644 --- a/pkg/kubelet/dockertools/fake_docker_client.go +++ b/pkg/kubelet/dockertools/fake_docker_client.go @@ -170,7 +170,11 @@ func (f *FakeDockerClient) PullImage(opts docker.PullImageOptions, auth docker.A f.Lock() defer f.Unlock() f.called = append(f.called, "pull") - f.pulled = append(f.pulled, fmt.Sprintf("%s/%s:%s", opts.Repository, opts.Registry, opts.Tag)) + registry := opts.Registry + if len(registry) != 0 { + registry = registry + "/" + } + f.pulled = append(f.pulled, fmt.Sprintf("%s%s:%s", registry, opts.Repository, opts.Tag)) return f.Err }