Merge pull request #7971 from deads2k/make-docker-keyring-handle-multiple-hits

pkg/credentialprovider/gcp/jwt_test.go

@@ -104,10 +104,15 @@ func TestJwtProvider(t *testing.T) {
 	// Verify that we get the expected username/password combo for
 	// a gcr.io image name.
 	registryUrl := "gcr.io/foo/bar"
-	val, ok := keyring.Lookup(registryUrl)
+	creds, ok := keyring.Lookup(registryUrl)
 	if !ok {
 		t.Errorf("Didn't find expected URL: %s", registryUrl)
+		return
 	}
+	if len(creds) > 1 {
+		t.Errorf("Got more hits than expected: %s", creds)
+	}
+	val := creds[0]
 
 	if "_token" != val.Username {
 		t.Errorf("Unexpected username value, want: _token, got: %s", val.Username)

pkg/credentialprovider/gcp/metadata_test.go

@@ -76,10 +76,15 @@ func TestDockerKeyringFromGoogleDockerConfigMetadata(t *testing.T) {
 
 	keyring.Add(provider.Provide())
 
-	val, ok := keyring.Lookup(registryUrl)
+	creds, ok := keyring.Lookup(registryUrl)
 	if !ok {
 		t.Errorf("Didn't find expected URL: %s", registryUrl)
+		return
 	}
+	if len(creds) > 1 {
+		t.Errorf("Got more hits than expected: %s", creds)
+	}
+	val := creds[0]
 
 	if username != val.Username {
 		t.Errorf("Unexpected username value, want: %s, got: %s", username, val.Username)
@@ -143,10 +148,15 @@ func TestDockerKeyringFromGoogleDockerConfigMetadataUrl(t *testing.T) {
 
 	keyring.Add(provider.Provide())
 
-	val, ok := keyring.Lookup(registryUrl)
+	creds, ok := keyring.Lookup(registryUrl)
 	if !ok {
 		t.Errorf("Didn't find expected URL: %s", registryUrl)
+		return
+	}
+	if len(creds) > 1 {
+		t.Errorf("Got more hits than expected: %s", creds)
 	}
+	val := creds[0]
 
 	if username != val.Username {
 		t.Errorf("Unexpected username value, want: %s, got: %s", username, val.Username)
@@ -211,10 +221,15 @@ func TestContainerRegistryBasics(t *testing.T) {
 
 	keyring.Add(provider.Provide())
 
-	val, ok := keyring.Lookup(registryUrl)
+	creds, ok := keyring.Lookup(registryUrl)
 	if !ok {
 		t.Errorf("Didn't find expected URL: %s", registryUrl)
+		return
+	}
+	if len(creds) > 1 {
+		t.Errorf("Got more hits than expected: %s", creds)
 	}
+	val := creds[0]
 
 	if "_token" != val.Username {
 		t.Errorf("Unexpected username value, want: %s, got: %s", "_token", val.Username)

pkg/credentialprovider/keyring.go

@@ -23,6 +23,8 @@ import (
 
 	docker "github.com/fsouza/go-dockerclient"
 	"github.com/golang/glog"
+
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
 // DockerKeyring tracks a set of docker registry credentials, maintaining a
@@ -33,13 +35,13 @@ import (
 //   most specific match for a given image
 // - iterating a map does not yield predictable results
 type DockerKeyring interface {
-	Lookup(image string) (docker.AuthConfiguration, bool)
+	Lookup(image string) ([]docker.AuthConfiguration, bool)
 }
 
 // BasicDockerKeyring is a trivial map-backed implementation of DockerKeyring
 type BasicDockerKeyring struct {
 	index []string
-	creds map[string]docker.AuthConfiguration
+	creds map[string][]docker.AuthConfiguration
 }
 
 // lazyDockerKeyring is an implementation of DockerKeyring that lazily
@@ -51,9 +53,10 @@ type lazyDockerKeyring struct {
 func (dk *BasicDockerKeyring) Add(cfg DockerConfig) {
 	if dk.index == nil {
 		dk.index = make([]string, 0)
-		dk.creds = make(map[string]docker.AuthConfiguration)
+		dk.creds = make(map[string][]docker.AuthConfiguration)
 	}
 	for loc, ident := range cfg {
+
 		creds := docker.AuthConfiguration{
 			Username: ident.Username,
 			Password: ident.Password,
@@ -73,15 +76,19 @@ func (dk *BasicDockerKeyring) Add(cfg DockerConfig) {
 		// See ResolveAuthConfig in docker/registry/auth.go.
 		if parsed.Host != "" {
 			// NOTE: foo.bar.com comes through as Path.
-			dk.creds[parsed.Host] = creds
+			dk.creds[parsed.Host] = append(dk.creds[parsed.Host], creds)
 			dk.index = append(dk.index, parsed.Host)
 		}
-		if parsed.Path != "/" {
-			dk.creds[parsed.Host+parsed.Path] = creds
-			dk.index = append(dk.index, parsed.Host+parsed.Path)
+		if (len(parsed.Path) > 0) && (parsed.Path != "/") {
+			key := parsed.Host + parsed.Path
+			dk.creds[key] = append(dk.creds[key], creds)
+			dk.index = append(dk.index, key)
 		}
 	}
 
+	eliminateDupes := util.NewStringSet(dk.index...)
+	dk.index = eliminateDupes.List()
+
 	// Update the index used to identify which credentials to use for a given
 	// image. The index is reverse-sorted so more specific paths are matched
 	// first. For example, if for the given image "quay.io/coreos/etcd",
@@ -109,11 +116,12 @@ func isDefaultRegistryMatch(image string) bool {
 	return !strings.ContainsAny(parts[0], ".:")
 }
 
-// Lookup implements the DockerKeyring method for fetching credentials
-// based on image name.
-func (dk *BasicDockerKeyring) Lookup(image string) (docker.AuthConfiguration, bool) {
-	// range over the index as iterating over a map does not provide
-	// a predictable ordering
+// Lookup implements the DockerKeyring method for fetching credentials based on image name.
+// Multiple credentials may be returned if there are multiple potentially valid credentials
+// available.  This allows for rotation.
+func (dk *BasicDockerKeyring) Lookup(image string) ([]docker.AuthConfiguration, bool) {
+	// range over the index as iterating over a map does not provide a predictable ordering
+	ret := []docker.AuthConfiguration{}
 	for _, k := range dk.index {
 		// NOTE: prefix is a sufficient check because while scheme is allowed,
 		// it is stripped as part of 'Add'
@@ -121,7 +129,11 @@ func (dk *BasicDockerKeyring) Lookup(image string) (docker.AuthConfiguration, bo
 			continue
 		}
 
-		return dk.creds[k], true
+		ret = append(ret, dk.creds[k]...)
+	}
+
+	if len(ret) > 0 {
+		return ret, true
 	}
 
 	// Use credentials for the default registry if provided, and appropriate
@@ -129,12 +141,12 @@ func (dk *BasicDockerKeyring) Lookup(image string) (docker.AuthConfiguration, bo
 		return auth, true
 	}
 
-	return docker.AuthConfiguration{}, false
+	return []docker.AuthConfiguration{}, false
 }
 
 // Lookup implements the DockerKeyring method for fetching credentials
 // based on image name.
-func (dk *lazyDockerKeyring) Lookup(image string) (docker.AuthConfiguration, bool) {
+func (dk *lazyDockerKeyring) Lookup(image string) ([]docker.AuthConfiguration, bool) {
 	keyring := &BasicDockerKeyring{}
 
 	for _, p := range dk.Providers {
@@ -145,10 +157,10 @@ func (dk *lazyDockerKeyring) Lookup(image string) (docker.AuthConfiguration, boo
 }
 
 type FakeKeyring struct {
-	auth docker.AuthConfiguration
+	auth []docker.AuthConfiguration
 	ok   bool
 }
 
-func (f *FakeKeyring) Lookup(image string) (docker.AuthConfiguration, bool) {
+func (f *FakeKeyring) Lookup(image string) ([]docker.AuthConfiguration, bool) {
 	return f.auth, f.ok
 }

pkg/credentialprovider/keyring_test.go

@@ -42,10 +42,15 @@ func TestDockerKeyringFromBytes(t *testing.T) {
 		keyring.Add(cfg)
 	}
 
-	val, ok := keyring.Lookup(url + "/foo/bar")
+	creds, ok := keyring.Lookup(url + "/foo/bar")
 	if !ok {
 		t.Errorf("Didn't find expected URL: %s", url)
+		return
 	}
+	if len(creds) > 1 {
+		t.Errorf("Got more hits than expected: %s", creds)
+	}
+	val := creds[0]
 
 	if username != val.Username {
 		t.Errorf("Unexpected username value, want: %s, got: %s", username, val.Username)
@@ -130,10 +135,15 @@ func TestKeyringHitWithUnqualifiedDockerHub(t *testing.T) {
 		keyring.Add(cfg)
 	}
 
-	val, ok := keyring.Lookup("google/docker-registry")
+	creds, ok := keyring.Lookup("google/docker-registry")
 	if !ok {
 		t.Errorf("Didn't find expected URL: %s", url)
+		return
+	}
+	if len(creds) > 1 {
+		t.Errorf("Got more hits than expected: %s", creds)
 	}
+	val := creds[0]
 
 	if username != val.Username {
 		t.Errorf("Unexpected username value, want: %s, got: %s", username, val.Username)
@@ -166,10 +176,15 @@ func TestKeyringHitWithUnqualifiedLibraryDockerHub(t *testing.T) {
 		keyring.Add(cfg)
 	}
 
-	val, ok := keyring.Lookup("jenkins")
+	creds, ok := keyring.Lookup("jenkins")
 	if !ok {
 		t.Errorf("Didn't find expected URL: %s", url)
+		return
 	}
+	if len(creds) > 1 {
+		t.Errorf("Got more hits than expected: %s", creds)
+	}
+	val := creds[0]
 
 	if username != val.Username {
 		t.Errorf("Unexpected username value, want: %s, got: %s", username, val.Username)
@@ -202,10 +217,15 @@ func TestKeyringHitWithQualifiedDockerHub(t *testing.T) {
 		keyring.Add(cfg)
 	}
 
-	val, ok := keyring.Lookup(url + "/google/docker-registry")
+	creds, ok := keyring.Lookup(url + "/google/docker-registry")
 	if !ok {
 		t.Errorf("Didn't find expected URL: %s", url)
+		return
+	}
+	if len(creds) > 2 {
+		t.Errorf("Got more hits than expected: %s", creds)
 	}
+	val := creds[0]
 
 	if username != val.Username {
 		t.Errorf("Unexpected username value, want: %s, got: %s", username, val.Username)

pkg/kubelet/dockertools/docker.go

@@ -30,6 +30,7 @@ import (
 	kubeletTypes "github.com/GoogleCloudPlatform/kubernetes/pkg/kubelet/types"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/types"
 	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
+	utilerrors "github.com/GoogleCloudPlatform/kubernetes/pkg/util/errors"
 	"github.com/docker/docker/pkg/parsers"
 	docker "github.com/fsouza/go-dockerclient"
 	"github.com/golang/glog"
@@ -124,16 +125,15 @@ func (p dockerPuller) Pull(image string) error {
 		Tag:        tag,
 	}
 
-	creds, ok := p.keyring.Lookup(repoToPull)
-	if !ok {
+	creds, haveCredentials := p.keyring.Lookup(repoToPull)
+	if !haveCredentials {
 		glog.V(1).Infof("Pulling image %s without credentials", image)
-	}
 
-	err := p.client.PullImage(opts, creds)
-	// If there was no error, or we had credentials, just return the error.
-	if err == nil || ok {
-		return err
+		err := p.client.PullImage(opts, docker.AuthConfiguration{})
+		if err == nil {
+			return nil
 		}
+
 		// Image spec: [<registry>/]<repository>/<image>[:<version] so we count '/'
 		explicitRegistry := (strings.Count(image, "/") == 2)
 		// Hack, look for a private registry, and decorate the error with the lack of
@@ -142,7 +142,22 @@ func (p dockerPuller) Pull(image string) error {
 		if explicitRegistry {
 			return fmt.Errorf("image pull failed for %s, this may be because there are no credentials on this request.  details: (%v)", image, err)
 		}
+
 		return err
+	}
+
+	var pullErrs []error
+	for _, currentCreds := range creds {
+		err := p.client.PullImage(opts, currentCreds)
+		// If there was no error, return success
+		if err == nil {
+			return nil
+		}
+
+		pullErrs = append(pullErrs, err)
+	}
+
+	return utilerrors.NewAggregate(pullErrs)
 }
 
 func (p throttledDockerPuller) Pull(image string) error {

pkg/kubelet/dockertools/docker_test.go

@@ -198,18 +198,18 @@ func TestParseImageName(t *testing.T) {
 	}
 }
 
-func TestPull(t *testing.T) {
+func TestPullWithNoSecrets(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"},
+		{"ubuntu", "ubuntu:latest using {}"},
+		{"ubuntu:2342", "ubuntu:2342 using {}"},
+		{"ubuntu:latest", "ubuntu:latest using {}"},
+		{"foo/bar:445566", "foo/bar:445566 using {}"},
+		{"registry.example.com:5000/foobar", "registry.example.com:5000/foobar:latest using {}"},
+		{"registry.example.com:5000/foobar:5342", "registry.example.com:5000/foobar:5342 using {}"},
+		{"registry.example.com:5000/foobar:latest", "registry.example.com:5000/foobar:latest using {}"},
 	}
 	for _, test := range tests {
 		fakeKeyring := &credentialprovider.FakeKeyring{}
@@ -259,7 +259,6 @@ func TestDockerKeyringLookupFails(t *testing.T) {
 }
 
 func TestDockerKeyringLookup(t *testing.T) {
-	empty := docker.AuthConfiguration{}
 
 	ada := docker.AuthConfiguration{
 		Username: "ada",
@@ -289,27 +288,27 @@ func TestDockerKeyringLookup(t *testing.T) {
 
 	tests := []struct {
 		image string
-		match docker.AuthConfiguration
+		match []docker.AuthConfiguration
 		ok    bool
 	}{
 		// direct match
-		{"bar.example.com", ada, true},
+		{"bar.example.com", []docker.AuthConfiguration{ada}, true},
 
 		// direct match deeper than other possible matches
-		{"bar.example.com/pong", grace, true},
+		{"bar.example.com/pong", []docker.AuthConfiguration{grace, ada}, true},
 
 		// no direct match, deeper path ignored
-		{"bar.example.com/ping", ada, true},
+		{"bar.example.com/ping", []docker.AuthConfiguration{ada}, true},
 
 		// match first part of path token
-		{"bar.example.com/pongz", grace, true},
+		{"bar.example.com/pongz", []docker.AuthConfiguration{grace, ada}, true},
 
 		// match regardless of sub-path
-		{"bar.example.com/pong/pang", grace, true},
+		{"bar.example.com/pong/pang", []docker.AuthConfiguration{grace, ada}, true},
 
 		// no host match
-		{"example.com", empty, false},
-		{"foo.example.com", empty, false},
+		{"example.com", []docker.AuthConfiguration{}, false},
+		{"foo.example.com", []docker.AuthConfiguration{}, false},
 	}
 
 	for i, tt := range tests {
@@ -345,15 +344,15 @@ func TestIssue3797(t *testing.T) {
 
 	tests := []struct {
 		image string
-		match docker.AuthConfiguration
+		match []docker.AuthConfiguration
 		ok    bool
 	}{
 		// direct match
-		{"quay.io", rex, true},
+		{"quay.io", []docker.AuthConfiguration{rex}, true},
 
 		// partial matches
-		{"quay.io/foo", rex, true},
-		{"quay.io/foo/bar", rex, true},
+		{"quay.io/foo", []docker.AuthConfiguration{rex}, true},
+		{"quay.io/foo/bar", []docker.AuthConfiguration{rex}, true},
 	}
 
 	for i, tt := range tests {

pkg/kubelet/dockertools/fake_docker_client.go

@@ -17,14 +17,16 @@ limitations under the License.
 package dockertools
 
 import (
+	"encoding/json"
 	"fmt"
 	"os"
 	"reflect"
 	"sort"
 	"sync"
 
-	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 	"github.com/fsouza/go-dockerclient"
+
+	"github.com/GoogleCloudPlatform/kubernetes/pkg/util"
 )
 
 // FakeDockerClient is a simple fake docker client, so that kubelet can be run for testing without requiring a real docker setup.
@@ -264,7 +266,8 @@ func (f *FakeDockerClient) PullImage(opts docker.PullImageOptions, auth docker.A
 		if len(registry) != 0 {
 			registry = registry + "/"
 		}
-		f.pulled = append(f.pulled, fmt.Sprintf("%s%s:%s", registry, opts.Repository, opts.Tag))
+		authJson, _ := json.Marshal(auth)
+		f.pulled = append(f.pulled, fmt.Sprintf("%s%s:%s using %s", registry, opts.Repository, opts.Tag, string(authJson)))
 	}
 	return err
 }

pkg/kubelet/dockertools/manager.go

@@ -723,7 +723,7 @@ func (dm *DockerManager) ListImages() ([]kubecontainer.Image, error) {
 
 // TODO(vmarmol): Consider unexporting.
 // PullImage pulls an image from network to local storage.
-func (dm *DockerManager) PullImage(image kubecontainer.ImageSpec, _ []api.Secret) error {
+func (dm *DockerManager) PullImage(image kubecontainer.ImageSpec, secrets []api.Secret) error {
 	return dm.Puller.Pull(image.Image)
 }
 
@@ -1145,6 +1145,7 @@ func (dm *DockerManager) createPodInfraContainer(pod *api.Pod) (kubeletTypes.Doc
 		return "", err
 	}
 	if !ok {
+		// TODO get the pull secrets from the container's ImageSpec and the pod's service account
 		if err := dm.PullImage(spec, nil); err != nil {
 			if ref != nil {
 				dm.recorder.Eventf(ref, "failed", "Failed to pull image %q: %v", container.Image, err)
@@ -1335,6 +1336,7 @@ func (dm *DockerManager) pullImage(pod *api.Pod, container *api.Container) error
 		return nil
 	}
 
+	// TODO get the pull secrets from the container's ImageSpec and the pod's service account
 	err = dm.PullImage(spec, nil)
 	dm.runtimeHooks.ReportImagePull(pod, container, err)
 	return err

pkg/kubelet/rkt/rkt.go

@@ -696,7 +696,13 @@ func (r *runtime) Version() (kubecontainer.Version, error) {
 
 // writeDockerAuthConfig writes the docker credentials to rkt auth config files.
 // This enables rkt to pull docker images from docker registry with credentials.
-func (r *runtime) writeDockerAuthConfig(image string, creds docker.AuthConfiguration) error {
+func (r *runtime) writeDockerAuthConfig(image string, credsSlice []docker.AuthConfiguration) error {
+	creds := docker.AuthConfiguration{}
+	// TODO handle multiple creds
+	if len(credsSlice) >= 1 {
+		creds = credsSlice[0]
+	}
+
 	registry := "index.docker.io"
 	// Image spec: [<registry>/]<repository>/<image>[:<version]
 	explicitRegistry := (strings.Count(image, "/") == 2)
@@ -735,7 +741,7 @@ func (r *runtime) writeDockerAuthConfig(image string, creds docker.AuthConfigura
 //
 // https://github.com/GoogleCloudPlatform/kubernetes/issues/7203
 //
-func (r *runtime) PullImage(image kubecontainer.ImageSpec, _ []api.Secret) error {
+func (r *runtime) PullImage(image kubecontainer.ImageSpec, pullSecrets []api.Secret) error {
 	img := image.Image
 	// TODO(yifan): The credential operation is a copy from dockertools package,
 	// Need to resolve the code duplication.