fix: don't use docker config cache if it's empty

add one comment

test: add unit test

fix comments

fix comments

revert test change

fix comments

Author: andyzhangx

pkg/credentialprovider/azure/BUILD

@@ -34,6 +34,7 @@ go_test(
         "//vendor/github.com/Azure/azure-sdk-for-go/services/containerregistry/mgmt/2019-05-01/containerregistry:go_default_library",
         "//vendor/github.com/Azure/go-autorest/autorest/azure:go_default_library",
         "//vendor/github.com/Azure/go-autorest/autorest/to:go_default_library",
+        "//vendor/github.com/stretchr/testify/assert:go_default_library",
     ],
 )
 

pkg/credentialprovider/azure/azure_credentials.go

@@ -56,8 +56,9 @@ var (
 func init() {
 	credentialprovider.RegisterCredentialProvider("azure",
 		&credentialprovider.CachingDockerConfigProvider{
-			Provider: NewACRProvider(flagConfigFile),
-			Lifetime: 1 * time.Minute,
+			Provider:    NewACRProvider(flagConfigFile),
+			Lifetime:    1 * time.Minute,
+			ShouldCache: func(d credentialprovider.DockerConfig) bool { return len(d) > 0 },
 		})
 }
 
@@ -186,13 +187,21 @@ func (a *acrProvider) Provide(image string) credentialprovider.DockerConfig {
 	klog.V(4).Infof("try to provide secret for image %s", image)
 	cfg := credentialprovider.DockerConfig{}
 
+	defaultConfigEntry := credentialprovider.DockerConfigEntry{
+		Username: "",
+		Password: "",
+		Email:    dummyRegistryEmail,
+	}
+
 	if a.config.UseManagedIdentityExtension {
 		if loginServer := a.parseACRLoginServerFromImage(image); loginServer == "" {
 			klog.V(4).Infof("image(%s) is not from ACR, skip MSI authentication", image)
 		} else {
 			if cred, err := getACRDockerEntryFromARMToken(a, loginServer); err == nil {
 				cfg[loginServer] = *cred
 			}
+			// add ACR anonymous repo support: use empty username and password for anonymous access
+			cfg["*.azurecr.*"] = defaultConfigEntry
 		}
 	} else {
 		// Add our entry for each of the supported container registry URLs
@@ -226,13 +235,9 @@ func (a *acrProvider) Provide(image string) credentialprovider.DockerConfig {
 				cfg[customAcrSuffix] = *cred
 			}
 		}
-	}
 
-	// add ACR anonymous repo support: use empty username and password for anonymous access
-	cfg["*.azurecr.*"] = credentialprovider.DockerConfigEntry{
-		Username: "",
-		Password: "",
-		Email:    dummyRegistryEmail,
+		// add ACR anonymous repo support: use empty username and password for anonymous access
+		cfg["*.azurecr.*"] = defaultConfigEntry
 	}
 	return cfg
 }

pkg/credentialprovider/azure/azure_credentials_test.go

@@ -24,6 +24,8 @@ import (
 	"github.com/Azure/azure-sdk-for-go/services/containerregistry/mgmt/2019-05-01/containerregistry"
 	"github.com/Azure/go-autorest/autorest/azure"
 	"github.com/Azure/go-autorest/autorest/to"
+
+	"github.com/stretchr/testify/assert"
 )
 
 type fakeClient struct {
@@ -96,6 +98,42 @@ func Test(t *testing.T) {
 	}
 }
 
+func TestProvide(t *testing.T) {
+	testCases := []struct {
+		desc                string
+		configStr           string
+		expectedCredsLength int
+	}{
+		{
+			desc: "return multiple credentials using Service Principal",
+			configStr: `
+    {
+        "aadClientId": "foo",
+        "aadClientSecret": "bar"
+    }`,
+			expectedCredsLength: 5,
+		},
+		{
+			desc: "retuen 0 credential for non-ACR image using Managed Identity",
+			configStr: `
+    {
+	"UseManagedIdentityExtension": true
+    }`,
+			expectedCredsLength: 0,
+		},
+	}
+
+	for i, test := range testCases {
+		provider := &acrProvider{
+			registryClient: &fakeClient{},
+		}
+		provider.loadConfig(bytes.NewBufferString(test.configStr))
+
+		creds := provider.Provide("busybox")
+		assert.Equal(t, test.expectedCredsLength, len(creds), "TestCase[%d]: %s", i, test.desc)
+	}
+}
+
 func TestParseACRLoginServerFromImage(t *testing.T) {
 	configStr := `
     {

pkg/credentialprovider/provider.go

@@ -58,6 +58,10 @@ type CachingDockerConfigProvider struct {
 	Provider DockerConfigProvider
 	Lifetime time.Duration
 
+	// ShouldCache is an optional function that returns true if the specific config should be cached.
+	// If nil, all configs are treated as cacheable.
+	ShouldCache func(DockerConfig) bool
+
 	// cache fields
 	cacheDockerConfig DockerConfig
 	expiration        time.Time
@@ -96,7 +100,10 @@ func (d *CachingDockerConfigProvider) Provide(image string) DockerConfig {
 	}
 
 	klog.V(2).Infof("Refreshing cache for provider: %v", reflect.TypeOf(d.Provider).String())
-	d.cacheDockerConfig = d.Provider.Provide(image)
-	d.expiration = time.Now().Add(d.Lifetime)
-	return d.cacheDockerConfig
+	config := d.Provider.Provide(image)
+	if d.ShouldCache == nil || d.ShouldCache(config) {
+		d.cacheDockerConfig = config
+		d.expiration = time.Now().Add(d.Lifetime)
+	}
+	return config
 }