Merge pull request #66429 from andyzhangx/acr-sp-fix

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. fix acr could not be listed in sp issue **What this PR does / why we need it**: after granting sp access to azure ACR , pull image from ACR would fail, and after wait about 15-30min(or restart kubelet directly), pull image would succeed. Root cause is that `servicePrincipalToken` needs to be refreshed when doing `registryClient.List`, otherwise it will always return empty registry list. Pull image error would be like following: ``` Events: Type Reason Age From Message ---- ------ ---- ---- ------- Warning FailedScheduling 8m (x3 over 8m) default-scheduler 0/1 nodes are available: 1 Insufficient cpu. Normal Scheduled 8m default-scheduler Successfully assigned nginx-server-776564f79c-zhtjk to aks-nodepool1-20881069-0 Normal SuccessfulMountVolume 8m kubelet, aks-nodepool1-20881069-0 MountVolume.SetUp succeeded for volume "default-token-4t7tk" Normal SuccessfulMountVolume 8m kubelet, aks-nodepool1-20881069-0 MountVolume.SetUp succeeded for volume "pvc-5c1f0521-739f-11e8-9b69-0a58ac1f09c2" Warning Failed 8m (x5 over 8m) kubelet, aks-nodepool1-20881069-0 Error: ImagePullBackOff Normal BackOff 8m (x5 over 8m) kubelet, aks-nodepool1-20881069-0 Back-off pulling image "andyacr.azurecr.io/nginx-server:1.0.0" Warning Failed 8m (x2 over 8m) kubelet, aks-nodepool1-20881069-0 Error: ErrImagePull Warning Failed 8m (x2 over 8m) kubelet, aks-nodepool1-20881069-0 Failed to pull image "andyacr.azurecr.io/nginx-server:1.0.0": rpc error: code = Unknown desc = Error response from daemon: Get https://andyacr.azurecr.io/v2/nginx-server/manifests/1.0.0: unauthorized: authentication required ``` **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes #65225 **Special notes for your reviewer**: After discuss with dong, `registryClient.List` won't be necessary, instead we return `{"*.azurecr.io", "*.azurecr.cn", "*.azurecr.de", "*.azurecr.us"}` like aws, gce code logic, it will do the url matching. I will cherry pick this PR to all supported version, every version has this issue. **Release note**: ``` fix acr could not be listed in sp issue ``` /sig azure /assign @feiskyer @khenidak @brendandburns @karataliu

Merge pull request #66429 from andyzhangx/acr-sp-fix
49670bee · Kubernetes Submit Queue · GitHub · 4797c8df · a7e328c2 · 49670bee
Unverified Commit 49670bee authored Jul 22, 2018 by Kubernetes Submit Queue Committed by GitHub Jul 22, 2018
Hide whitespace changes
Inline Side-by-side

Showing with 29 additions and 19 deletions

azure_credentials.go pkg/credentialprovider/azure/azure_credentials.go +20 -16

azure_credentials_test.go pkg/credentialprovider/azure/azure_credentials_test.go +9 -3

No files found.
--- a/pkg/credentialprovider/azure/azure_credentials.go
+++ b/pkg/credentialprovider/azure/azure_credentials.go
@@ -40,6 +40,8 @@ var flagConfigFile = pflag.String("azure-container-registry-config", "",
 const dummyRegistryEmail = "name@contoso.com"
+var containerRegistryUrls = []string{"*.azurecr.io", "*.azurecr.cn", "*.azurecr.de", "*.azurecr.us"}
 // init registers the various means by which credentials may
 // be resolved on Azure.
 func init() {
@@ -176,31 +178,33 @@ func (a *acrProvider) Provide() credentialprovider.DockerConfig {
 	ctx, cancel := getContextWithCancel()
 	defer cancel()
-	glog.V(4).Infof("listing registries")
+	if a.config.UseManagedIdentityExtension {
-	result, err := a.registryClient.List(ctx)
+		glog.V(4).Infof("listing registries")
-	if err != nil {
+		result, err := a.registryClient.List(ctx)
-		glog.Errorf("Failed to list registries: %v", err)
+		if err != nil {
-		return cfg
+			glog.Errorf("Failed to list registries: %v", err)
-	}
+			return cfg
+		}
-	for ix := range result {
-		loginServer := getLoginServer(result[ix])
-		var cred *credentialprovider.DockerConfigEntry
-		if a.config.UseManagedIdentityExtension {
+		for ix := range result {
-			cred, err = getACRDockerEntryFromARMToken(a, loginServer)
+			loginServer := getLoginServer(result[ix])
+			glog.V(2).Infof("loginServer: %s", loginServer)
+			cred, err := getACRDockerEntryFromARMToken(a, loginServer)
 			if err != nil {
 				continue
 			}
-		} else {
+			cfg[loginServer] = *cred
-			cred = &credentialprovider.DockerConfigEntry{
+		}
+	} else {
+		// Add our entry for each of the supported container registry URLs
+		for _, url := range containerRegistryUrls {
+			cred := &credentialprovider.DockerConfigEntry{
 				Username: a.config.AADClientID,
 				Password: a.config.AADClientSecret,
 				Email:    dummyRegistryEmail,
 			}
+			cfg[url] = *cred
 		}
-		cfg[loginServer] = *cred
 	}
 	return cfg
 }

--- a/pkg/credentialprovider/azure/azure_credentials_test.go
+++ b/pkg/credentialprovider/azure/azure_credentials_test.go
@@ -43,19 +43,25 @@ func Test(t *testing.T) {
 		{
 			Name: to.StringPtr("foo"),
 			RegistryProperties: &containerregistry.RegistryProperties{
-				LoginServer: to.StringPtr("foo-microsoft.azurecr.io"),
+				LoginServer: to.StringPtr("*.azurecr.io"),
 			},
 		},
 		{
 			Name: to.StringPtr("bar"),
 			RegistryProperties: &containerregistry.RegistryProperties{
-				LoginServer: to.StringPtr("bar-microsoft.azurecr.io"),
+				LoginServer: to.StringPtr("*.azurecr.cn"),
 			},
 		},
 		{
 			Name: to.StringPtr("baz"),
 			RegistryProperties: &containerregistry.RegistryProperties{
-				LoginServer: to.StringPtr("baz-microsoft.azurecr.io"),
+				LoginServer: to.StringPtr("*.azurecr.de"),
+			},
+		},
+		{
+			Name: to.StringPtr("bus"),
+			RegistryProperties: &containerregistry.RegistryProperties{
+				LoginServer: to.StringPtr("*.azurecr.us"),
 			},
 		},
 	}