credential provider config: validate duplicate names early and preserve provider order

Signed-off-by: Anish Ramasekar <[email protected]>

Author: aramase

pkg/credentialprovider/plugin/config.go

@@ -18,10 +18,10 @@ package plugin
 
 import (
 	"fmt"
-	"strings"
-
 	"os"
+	"strings"
 
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/kubernetes/pkg/credentialprovider"
 	kubeletconfig "k8s.io/kubernetes/pkg/kubelet/apis/config"
@@ -78,6 +78,7 @@ func validateCredentialProviderConfig(config *kubeletconfig.CredentialProviderCo
 	}
 
 	fieldPath := field.NewPath("providers")
+	seenProviderNames := sets.NewString()
 	for _, provider := range config.Providers {
 		if strings.Contains(provider.Name, "/") {
 			allErrs = append(allErrs, field.Invalid(fieldPath.Child("name"), provider.Name, "provider name cannot contain '/'"))
@@ -95,14 +96,15 @@ func validateCredentialProviderConfig(config *kubeletconfig.CredentialProviderCo
 			allErrs = append(allErrs, field.Invalid(fieldPath.Child("name"), provider.Name, "provider name cannot be '..'"))
 		}
 
+		if seenProviderNames.Has(provider.Name) {
+			allErrs = append(allErrs, field.Duplicate(fieldPath.Child("name"), provider.Name))
+		}
+		seenProviderNames.Insert(provider.Name)
+
 		if provider.APIVersion == "" {
 			allErrs = append(allErrs, field.Required(fieldPath.Child("apiVersion"), "apiVersion is required"))
 		} else if _, ok := apiVersions[provider.APIVersion]; !ok {
-			validAPIVersions := []string{}
-			for apiVersion := range apiVersions {
-				validAPIVersions = append(validAPIVersions, apiVersion)
-			}
-
+			validAPIVersions := sets.StringKeySet(apiVersions).List()
 			allErrs = append(allErrs, field.NotSupported(fieldPath.Child("apiVersion"), provider.APIVersion, validAPIVersions))
 		}
 

pkg/credentialprovider/plugin/config_test.go

@@ -22,6 +22,9 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
+
+	"k8s.io/apimachinery/pkg/util/errors"
 	utiltesting "k8s.io/client-go/util/testing"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -369,14 +372,15 @@ providers:
 
 func Test_validateCredentialProviderConfig(t *testing.T) {
 	testcases := []struct {
-		name      string
-		config    *kubeletconfig.CredentialProviderConfig
-		shouldErr bool
+		name                          string
+		config                        *kubeletconfig.CredentialProviderConfig
+		saTokenForCredentialProviders bool
+		expectErr                     string
 	}{
 		{
 			name:      "no providers provided",
 			config:    &kubeletconfig.CredentialProviderConfig{},
-			shouldErr: true,
+			expectErr: `providers: Required value: at least 1 item in plugins is required`,
 		},
 		{
 			name: "no matchImages provided",
@@ -390,7 +394,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: `providers.matchImages: Required value: at least 1 item in matchImages is required`,
 		},
 		{
 			name: "no default cache duration provided",
@@ -403,7 +407,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: `providers.defaultCacheDuration: Required value: defaultCacheDuration is required`,
 		},
 		{
 			name: "name contains '/'",
@@ -417,7 +421,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: `providers.name: Invalid value: "foo/../bar": provider name cannot contain '/'`,
 		},
 		{
 			name: "name is '.'",
@@ -431,7 +435,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: `providers.name: Invalid value: ".": provider name cannot be '.'`,
 		},
 		{
 			name: "name is '..'",
@@ -445,7 +449,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: `providers.name: Invalid value: "..": provider name cannot be '..'`,
 		},
 		{
 			name: "name contains spaces",
@@ -459,7 +463,27 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: `providers.name: Invalid value: "foo bar": provider name cannot contain spaces`,
+		},
+		{
+			name: "duplicate names",
+			config: &kubeletconfig.CredentialProviderConfig{
+				Providers: []kubeletconfig.CredentialProvider{
+					{
+						Name:                 "foobar",
+						MatchImages:          []string{"foobar.registry.io"},
+						DefaultCacheDuration: &metav1.Duration{Duration: time.Minute},
+						APIVersion:           "credentialprovider.kubelet.k8s.io/v1alpha1",
+					},
+					{
+						Name:                 "foobar",
+						MatchImages:          []string{"bar.registry.io"},
+						DefaultCacheDuration: &metav1.Duration{Duration: time.Minute},
+						APIVersion:           "credentialprovider.kubelet.k8s.io/v1alpha1",
+					},
+				},
+			},
+			expectErr: `providers.name: Duplicate value: "foobar"`,
 		},
 		{
 			name: "no apiVersion",
@@ -473,7 +497,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: "providers.apiVersion: Required value: apiVersion is required",
 		},
 		{
 			name: "invalid apiVersion",
@@ -487,7 +511,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: `providers.apiVersion: Unsupported value: "credentialprovider.kubelet.k8s.io/v1alpha0": supported values: "credentialprovider.kubelet.k8s.io/v1", "credentialprovider.kubelet.k8s.io/v1alpha1", "credentialprovider.kubelet.k8s.io/v1beta1"`,
 		},
 		{
 			name: "negative default cache duration",
@@ -501,7 +525,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: "providers.defaultCacheDuration: Invalid value: -1m0s: defaultCacheDuration must be greater than or equal to 0",
 		},
 		{
 			name: "invalid match image",
@@ -515,7 +539,7 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: true,
+			expectErr: `providers.matchImages: Invalid value: "%invalid%": match image is invalid: parse "https://%invalid%": invalid URL escape "%in"`,
 		},
 		{
 			name: "valid config",
@@ -529,19 +553,22 @@ func Test_validateCredentialProviderConfig(t *testing.T) {
 					},
 				},
 			},
-			shouldErr: false,
 		},
 	}
 
 	for _, testcase := range testcases {
 		t.Run(testcase.name, func(t *testing.T) {
-			errs := validateCredentialProviderConfig(testcase.config)
-
-			if testcase.shouldErr && len(errs) == 0 {
-				t.Errorf("expected error but got none")
-			} else if !testcase.shouldErr && len(errs) > 0 {
-				t.Errorf("expected no error but received errors: %v", errs.ToAggregate())
+			errs := validateCredentialProviderConfig(testcase.config).ToAggregate()
+			if d := cmp.Diff(testcase.expectErr, errString(errs)); d != "" {
+				t.Fatalf("CredentialProviderConfig validation mismatch (-want +got):\n%s", d)
 			}
 		})
 	}
 }
+
+func errString(errs errors.Aggregate) string {
+	if errs != nil {
+		return errs.Error()
+	}
+	return ""
+}

pkg/credentialprovider/plugins.go

@@ -17,32 +17,39 @@ limitations under the License.
 package credentialprovider
 
 import (
-	"reflect"
-	"sort"
 	"sync"
 
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/klog/v2"
 )
 
+type provider struct {
+	name string
+	impl DockerConfigProvider
+}
+
 // All registered credential providers.
 var providersMutex sync.Mutex
-var providers = make(map[string]DockerConfigProvider)
+var providers = make([]provider, 0)
+var seenProviderNames = sets.NewString()
 
 // RegisterCredentialProvider is called by provider implementations on
 // initialization to register themselves, like so:
 //
 //	func init() {
 //	 	RegisterCredentialProvider("name", &myProvider{...})
 //	}
-func RegisterCredentialProvider(name string, provider DockerConfigProvider) {
+func RegisterCredentialProvider(name string, p DockerConfigProvider) {
 	providersMutex.Lock()
 	defer providersMutex.Unlock()
-	_, found := providers[name]
-	if found {
+
+	if seenProviderNames.Has(name) {
 		klog.Fatalf("Credential provider %q was registered twice", name)
 	}
+	seenProviderNames.Insert(name)
+
+	providers = append(providers, provider{name, p})
 	klog.V(4).Infof("Registered credential provider %q", name)
-	providers[name] = provider
 }
 
 // NewDockerKeyring creates a DockerKeyring to use for resolving credentials,
@@ -52,18 +59,10 @@ func NewDockerKeyring() DockerKeyring {
 		Providers: make([]DockerConfigProvider, 0),
 	}
 
-	keys := reflect.ValueOf(providers).MapKeys()
-	stringKeys := make([]string, len(keys))
-	for ix := range keys {
-		stringKeys[ix] = keys[ix].String()
-	}
-	sort.Strings(stringKeys)
-
-	for _, key := range stringKeys {
-		provider := providers[key]
-		if provider.Enabled() {
-			klog.V(4).Infof("Registering credential provider: %v", key)
-			keyring.Providers = append(keyring.Providers, provider)
+	for _, p := range providers {
+		if p.impl.Enabled() {
+			klog.V(4).Infof("Registering credential provider: %v", p.name)
+			keyring.Providers = append(keyring.Providers, p.impl)
 		}
 	}