Merge pull request #21286 from smarterclayton/fix_keyring

Auto commit by PR queue bot

pkg/credentialprovider/keyring.go

@@ -67,7 +67,11 @@ func (dk *BasicDockerKeyring) Add(cfg DockerConfig) {
 			Email:    ident.Email,
 		}
 
-		parsed, err := url.Parse(loc)
+		value := loc
+		if !strings.HasPrefix(value, "https://") && !strings.HasPrefix(value, "http://") {
+			value = "https://" + value
+		}
+		parsed, err := url.Parse(value)
 		if err != nil {
 			glog.Errorf("Entry %q in dockercfg invalid (%v), ignoring", loc, err)
 			continue
@@ -77,17 +81,20 @@ func (dk *BasicDockerKeyring) Add(cfg DockerConfig) {
 		//    foo.bar.com/namespace
 		// Or hostname matches:
 		//    foo.bar.com
+		// It also considers /v2/  and /v1/ equivalent to the hostname
 		// See ResolveAuthConfig in docker/registry/auth.go.
-		if parsed.Host != "" {
-			// NOTE: foo.bar.com comes through as Path.
-			dk.creds[parsed.Host] = append(dk.creds[parsed.Host], creds)
-			dk.index = append(dk.index, parsed.Host)
+		effectivePath := parsed.Path
+		if strings.HasPrefix(effectivePath, "/v2/") || strings.HasPrefix(effectivePath, "/v1/") {
+			effectivePath = effectivePath[3:]
 		}
-		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)
+		var key string
+		if (len(effectivePath) > 0) && (effectivePath != "/") {
+			key = parsed.Host + effectivePath
+		} else {
+			key = parsed.Host
 		}
+		dk.creds[key] = append(dk.creds[key], creds)
+		dk.index = append(dk.index, key)
 	}
 
 	eliminateDupes := sets.NewString(dk.index...)
@@ -100,7 +107,10 @@ func (dk *BasicDockerKeyring) Add(cfg DockerConfig) {
 	sort.Sort(sort.Reverse(sort.StringSlice(dk.index)))
 }
 
-const defaultRegistryHost = "index.docker.io/v1/"
+const (
+	defaultRegistryHost = "index.docker.io"
+	defaultRegistryKey  = defaultRegistryHost + "/v1/"
+)
 
 // isDefaultRegistryMatch determines whether the given image will
 // pull from the default registry (DockerHub) based on the
@@ -223,8 +233,10 @@ func (dk *BasicDockerKeyring) Lookup(image string) ([]docker.AuthConfiguration,
 	}
 
 	// Use credentials for the default registry if provided, and appropriate
-	if auth, ok := dk.creds[defaultRegistryHost]; ok && isDefaultRegistryMatch(image) {
-		return auth, true
+	if isDefaultRegistryMatch(image) {
+		if auth, ok := dk.creds[defaultRegistryHost]; ok {
+			return auth, true
+		}
 	}
 
 	return []docker.AuthConfiguration{}, false

pkg/credentialprovider/keyring_test.go

@@ -125,65 +125,77 @@ func TestDockerKeyringForGlob(t *testing.T) {
 		targetUrl string
 	}{
 		{
-			globUrl:   "hello.kubernetes.io",
+			globUrl:   "https://hello.kubernetes.io",
 			targetUrl: "hello.kubernetes.io",
 		},
 		{
-			globUrl:   "*.docker.io",
+			globUrl:   "https://*.docker.io",
 			targetUrl: "prefix.docker.io",
 		},
 		{
-			globUrl:   "prefix.*.io",
+			globUrl:   "https://prefix.*.io",
 			targetUrl: "prefix.docker.io",
 		},
 		{
-			globUrl:   "prefix.docker.*",
+			globUrl:   "https://prefix.docker.*",
 			targetUrl: "prefix.docker.io",
 		},
 		{
-			globUrl:   "*.docker.io/path",
+			globUrl:   "https://*.docker.io/path",
 			targetUrl: "prefix.docker.io/path",
 		},
 		{
-			globUrl:   "prefix.*.io/path",
+			globUrl:   "https://prefix.*.io/path",
 			targetUrl: "prefix.docker.io/path/subpath",
 		},
 		{
-			globUrl:   "prefix.docker.*/path",
+			globUrl:   "https://prefix.docker.*/path",
 			targetUrl: "prefix.docker.io/path",
 		},
 		{
-			globUrl:   "*.docker.io:8888",
+			globUrl:   "https://*.docker.io:8888",
 			targetUrl: "prefix.docker.io:8888",
 		},
 		{
-			globUrl:   "prefix.*.io:8888",
+			globUrl:   "https://prefix.*.io:8888",
 			targetUrl: "prefix.docker.io:8888",
 		},
 		{
-			globUrl:   "prefix.docker.*:8888",
+			globUrl:   "https://prefix.docker.*:8888",
 			targetUrl: "prefix.docker.io:8888",
 		},
 		{
-			globUrl:   "*.docker.io/path:1111",
+			globUrl:   "https://*.docker.io/path:1111",
 			targetUrl: "prefix.docker.io/path:1111",
 		},
 		{
-			globUrl:   "prefix.*.io/path:1111",
-			targetUrl: "prefix.docker.io/path/subpath:1111",
+			globUrl:   "https://*.docker.io/v1/",
+			targetUrl: "prefix.docker.io/path:1111",
 		},
 		{
-			globUrl:   "prefix.docker.*/path:1111",
+			globUrl:   "https://*.docker.io/v2/",
 			targetUrl: "prefix.docker.io/path:1111",
 		},
+		{
+			globUrl:   "https://prefix.docker.*/path:1111",
+			targetUrl: "prefix.docker.io/path:1111",
+		},
+		{
+			globUrl:   "prefix.docker.io:1111",
+			targetUrl: "prefix.docker.io:1111/path",
+		},
+		{
+			globUrl:   "*.docker.io:1111",
+			targetUrl: "prefix.docker.io:1111/path",
+		},
 	}
-	for _, test := range tests {
+	for i, test := range tests {
 		email := "foo@bar.baz"
 		username := "foo"
 		password := "bar"
 		auth := base64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", username, password)))
 		sampleDockerConfig := fmt.Sprintf(`{
-   "https://%s": {
+   "%s": {
      "email": %q,
      "auth": %q
    }
@@ -198,8 +210,8 @@ func TestDockerKeyringForGlob(t *testing.T) {
 
 		creds, ok := keyring.Lookup(test.targetUrl + "/foo/bar")
 		if !ok {
-			t.Errorf("Didn't find expected URL: %s", test.targetUrl)
-			return
+			t.Errorf("%d: Didn't find expected URL: %s", i, test.targetUrl)
+			continue
 		}
 		val := creds[0]
 
@@ -221,21 +233,29 @@ func TestKeyringMiss(t *testing.T) {
 		lookupUrl string
 	}{
 		{
-			globUrl:   "hello.kubernetes.io",
+			globUrl:   "https://hello.kubernetes.io",
 			lookupUrl: "world.mesos.org/foo/bar",
 		},
 		{
-			globUrl:   "*.docker.com",
+			globUrl:   "https://*.docker.com",
 			lookupUrl: "prefix.docker.io",
 		},
+		{
+			globUrl:   "https://suffix.*.io",
+			lookupUrl: "prefix.docker.io",
+		},
+		{
+			globUrl:   "https://prefix.docker.c*",
+			lookupUrl: "prefix.docker.io",
+		},
+		{
+			globUrl:   "https://prefix.*.io/path:1111",
+			lookupUrl: "prefix.docker.io/path/subpath:1111",
+		},
 		{
 			globUrl:   "suffix.*.io",
 			lookupUrl: "prefix.docker.io",
 		},
-		{
-			globUrl:   "prefix.docker.c*",
-			lookupUrl: "prefix.docker.io",
-		},
 	}
 	for _, test := range tests {
 		email := "foo@bar.baz"
@@ -243,7 +263,7 @@ func TestKeyringMiss(t *testing.T) {
 		password := "bar"
 		auth := base64.StdEncoding.EncodeToString([]byte(fmt.Sprintf("%s:%s", username, password)))
 		sampleDockerConfig := fmt.Sprintf(`{
-   "https://%s": {
+   "%s": {
      "email": %q,
      "auth": %q
    }
@@ -265,7 +285,7 @@ func TestKeyringMiss(t *testing.T) {
 }
 
 func TestKeyringMissWithDockerHubCredentials(t *testing.T) {
-	url := defaultRegistryHost
+	url := defaultRegistryKey
 	email := "foo@bar.baz"
 	username := "foo"
 	password := "bar"
@@ -291,7 +311,7 @@ func TestKeyringMissWithDockerHubCredentials(t *testing.T) {
 }
 
 func TestKeyringHitWithUnqualifiedDockerHub(t *testing.T) {
-	url := defaultRegistryHost
+	url := defaultRegistryKey
 	email := "foo@bar.baz"
 	username := "foo"
 	password := "bar"
@@ -332,7 +352,7 @@ func TestKeyringHitWithUnqualifiedDockerHub(t *testing.T) {
 }
 
 func TestKeyringHitWithUnqualifiedLibraryDockerHub(t *testing.T) {
-	url := defaultRegistryHost
+	url := defaultRegistryKey
 	email := "foo@bar.baz"
 	username := "foo"
 	password := "bar"
@@ -373,7 +393,7 @@ func TestKeyringHitWithUnqualifiedLibraryDockerHub(t *testing.T) {
 }
 
 func TestKeyringHitWithQualifiedDockerHub(t *testing.T) {
-	url := defaultRegistryHost
+	url := defaultRegistryKey
 	email := "foo@bar.baz"
 	username := "foo"
 	password := "bar"

pkg/kubelet/dockertools/docker_test.go

@@ -354,7 +354,7 @@ func TestPullWithSecrets(t *testing.T) {
 			[]string{`ubuntu:latest using {"username":"passed-user","password":"passed-password","email":"passed-email"}`},
 		},
 	}
-	for _, test := range tests {
+	for i, test := range tests {
 		builtInKeyRing := &credentialprovider.BasicDockerKeyring{}
 		builtInKeyRing.Add(test.builtInDockerConfig)
 
@@ -367,17 +367,17 @@ func TestPullWithSecrets(t *testing.T) {
 
 		err := dp.Pull(test.imageName, test.passedSecrets)
 		if err != nil {
-			t.Errorf("unexpected non-nil err: %s", err)
+			t.Errorf("%s: unexpected non-nil err: %s", i, 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)
+			t.Errorf("%s: expected 1 pulled image, got %d: %v", i, a, fakeClient.pulled)
 			continue
 		}
 
 		if e, a := test.expectedPulls, fakeClient.pulled; !reflect.DeepEqual(e, a) {
-			t.Errorf("%s: expected pull of %v, but got %v", test.imageName, e, a)
+			t.Errorf("%s: expected pull of %v, but got %v", i, e, a)
 		}
 	}
 }