Use docker's ParseRepositoryTag when pulling

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

pkg/kubelet/dockertools/docker_test.go

@@ -178,26 +178,63 @@ func TestDockerContainerCommand(t *testing.T) {
 		t.Errorf("unexpected command args: %s", cmd.Args)
 	}
 }
-
+func TestParseImageName(t *testing.T) {
-var parseImageNameTests = []struct {
+	tests := []struct {
-	imageName string
+		imageName string
-	name      string
+		name      string
-	tag       string
+		tag       string
-}{
+	}{
-	{"ubuntu", "ubuntu", ""},
+		{"ubuntu", "ubuntu", ""},
-	{"ubuntu:2342", "ubuntu", "2342"},
+		{"ubuntu:2342", "ubuntu", "2342"},
-	{"ubuntu:latest", "ubuntu", "latest"},
+		{"ubuntu:latest", "ubuntu", "latest"},
-	{"foo/bar:445566", "foo/bar", "445566"},
+		{"foo/bar:445566", "foo/bar", "445566"},
-	{"registry.example.com:5000/foobar", "registry.example.com:5000/foobar", ""},
+		{"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:5342", "registry.example.com:5000/foobar", "5342"},
-	{"registry.example.com:5000/foobar:latest", "registry.example.com:5000/foobar", "latest"},
+		{"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) {
+func TestPull(t *testing.T) {
-	for _, tt := range parseImageNameTests {
+	tests := []struct {
-		name, tag := parseImageName(tt.imageName)
+		imageName     string
-		if name != tt.name || tag != tt.tag {
+		expectedImage string
-			t.Errorf("Expected name/tag: %s/%s, got %s/%s", tt.name, tt.tag, name, tag)
+	}{
+		{"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())
 	}

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
 }