Merge pull request #129669 from aramase/aramase/f/credential_provider_config_dup_validation

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

pkg/credentialprovider/plugin/config.go

@@ -18,10 +18,10 @@ package plugin
 
 import (
 	"fmt"
+	"os"
 	"strings"
 
-	"os"
-
+	"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,16 +17,21 @@ 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:
@@ -34,15 +39,17 @@ var providers = make(map[string]DockerConfigProvider)
 //	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)
 		}
 	}
 

pkg/generated/openapi/zz_generated.openapi.go

@@ -63170,7 +63170,7 @@ func schema_k8sio_kubelet_config_v1_CredentialProvider(ref common.ReferenceCallb
 				Properties: map[string]spec.Schema{
 					"name": {
 						SchemaProps: spec.SchemaProps{
-							Description: "name is the required name of the credential provider. It must match the name of the provider executable as seen by the kubelet. The executable must be in the kubelet's bin directory (set by the --image-credential-provider-bin-dir flag).",
+							Description: "name is the required name of the credential provider. It must match the name of the provider executable as seen by the kubelet. The executable must be in the kubelet's bin directory (set by the --image-credential-provider-bin-dir flag). Required to be unique across all providers.",
 							Default:     "",
 							Type:        []string{"string"},
 							Format:      "",
@@ -63266,7 +63266,7 @@ func schema_k8sio_kubelet_config_v1_CredentialProviderConfig(ref common.Referenc
 					},
 					"providers": {
 						SchemaProps: spec.SchemaProps{
-							Description: "providers is a list of credential provider plugins that will be enabled by the kubelet. Multiple providers may match against a single image, in which case credentials from all providers will be returned to the kubelet. If multiple providers are called for a single image, the results are combined. If providers return overlapping auth keys, the value from the provider earlier in this list is used.",
+							Description: "providers is a list of credential provider plugins that will be enabled by the kubelet. Multiple providers may match against a single image, in which case credentials from all providers will be returned to the kubelet. If multiple providers are called for a single image, the results are combined. If providers return overlapping auth keys, the value from the provider earlier in this list is attempted first.",
 							Type:        []string{"array"},
 							Items: &spec.SchemaOrArray{
 								Schema: &spec.Schema{
@@ -63324,7 +63324,7 @@ func schema_k8sio_kubelet_config_v1alpha1_CredentialProvider(ref common.Referenc
 				Properties: map[string]spec.Schema{
 					"name": {
 						SchemaProps: spec.SchemaProps{
-							Description: "name is the required name of the credential provider. It must match the name of the provider executable as seen by the kubelet. The executable must be in the kubelet's bin directory (set by the --image-credential-provider-bin-dir flag).",
+							Description: "name is the required name of the credential provider. It must match the name of the provider executable as seen by the kubelet. The executable must be in the kubelet's bin directory (set by the --image-credential-provider-bin-dir flag). Required to be unique across all providers.",
 							Default:     "",
 							Type:        []string{"string"},
 							Format:      "",
@@ -63420,7 +63420,7 @@ func schema_k8sio_kubelet_config_v1alpha1_CredentialProviderConfig(ref common.Re
 					},
 					"providers": {
 						SchemaProps: spec.SchemaProps{
-							Description: "providers is a list of credential provider plugins that will be enabled by the kubelet. Multiple providers may match against a single image, in which case credentials from all providers will be returned to the kubelet. If multiple providers are called for a single image, the results are combined. If providers return overlapping auth keys, the value from the provider earlier in this list is used.",
+							Description: "providers is a list of credential provider plugins that will be enabled by the kubelet. Multiple providers may match against a single image, in which case credentials from all providers will be returned to the kubelet. If multiple providers are called for a single image, the results are combined. If providers return overlapping auth keys, the value from the provider earlier in this list is attempted first.",
 							Type:        []string{"array"},
 							Items: &spec.SchemaOrArray{
 								Schema: &spec.Schema{
@@ -63498,7 +63498,7 @@ func schema_k8sio_kubelet_config_v1beta1_CredentialProvider(ref common.Reference
 				Properties: map[string]spec.Schema{
 					"name": {
 						SchemaProps: spec.SchemaProps{
-							Description: "name is the required name of the credential provider. It must match the name of the provider executable as seen by the kubelet. The executable must be in the kubelet's bin directory (set by the --image-credential-provider-bin-dir flag).",
+							Description: "name is the required name of the credential provider. It must match the name of the provider executable as seen by the kubelet. The executable must be in the kubelet's bin directory (set by the --image-credential-provider-bin-dir flag). Required to be unique across all providers.",
 							Default:     "",
 							Type:        []string{"string"},
 							Format:      "",
@@ -63594,7 +63594,7 @@ func schema_k8sio_kubelet_config_v1beta1_CredentialProviderConfig(ref common.Ref
 					},
 					"providers": {
 						SchemaProps: spec.SchemaProps{
-							Description: "providers is a list of credential provider plugins that will be enabled by the kubelet. Multiple providers may match against a single image, in which case credentials from all providers will be returned to the kubelet. If multiple providers are called for a single image, the results are combined. If providers return overlapping auth keys, the value from the provider earlier in this list is used.",
+							Description: "providers is a list of credential provider plugins that will be enabled by the kubelet. Multiple providers may match against a single image, in which case credentials from all providers will be returned to the kubelet. If multiple providers are called for a single image, the results are combined. If providers return overlapping auth keys, the value from the provider earlier in this list is attempted first.",
 							Type:        []string{"array"},
 							Items: &spec.SchemaOrArray{
 								Schema: &spec.Schema{

pkg/kubelet/apis/config/types.go

@@ -604,7 +604,7 @@ type CredentialProviderConfig struct {
 	// Multiple providers may match against a single image, in which case credentials
 	// from all providers will be returned to the kubelet. If multiple providers are called
 	// for a single image, the results are combined. If providers return overlapping
-	// auth keys, the value from the provider earlier in this list is used.
+	// auth keys, the value from the provider earlier in this list is attempted first.
 	Providers []CredentialProvider
 }
 
@@ -614,6 +614,7 @@ type CredentialProvider struct {
 	// name is the required name of the credential provider. It must match the name of the
 	// provider executable as seen by the kubelet. The executable must be in the kubelet's
 	// bin directory (set by the --credential-provider-bin-dir flag).
+	// Required to be unique across all providers.
 	Name string
 
 	// matchImages is a required list of strings used to match against images in order to

staging/src/k8s.io/kubelet/config/v1/types.go

@@ -32,7 +32,7 @@ type CredentialProviderConfig struct {
 	// Multiple providers may match against a single image, in which case credentials
 	// from all providers will be returned to the kubelet. If multiple providers are called
 	// for a single image, the results are combined. If providers return overlapping
-	// auth keys, the value from the provider earlier in this list is used.
+	// auth keys, the value from the provider earlier in this list is attempted first.
 	Providers []CredentialProvider `json:"providers"`
 }
 
@@ -42,6 +42,7 @@ type CredentialProvider struct {
 	// name is the required name of the credential provider. It must match the name of the
 	// provider executable as seen by the kubelet. The executable must be in the kubelet's
 	// bin directory (set by the --image-credential-provider-bin-dir flag).
+	// Required to be unique across all providers.
 	Name string `json:"name"`
 
 	// matchImages is a required list of strings used to match against images in order to

staging/src/k8s.io/kubelet/config/v1alpha1/types.go

@@ -32,7 +32,7 @@ type CredentialProviderConfig struct {
 	// Multiple providers may match against a single image, in which case credentials
 	// from all providers will be returned to the kubelet. If multiple providers are called
 	// for a single image, the results are combined. If providers return overlapping
-	// auth keys, the value from the provider earlier in this list is used.
+	// auth keys, the value from the provider earlier in this list is attempted first.
 	Providers []CredentialProvider `json:"providers"`
 }
 
@@ -42,6 +42,7 @@ type CredentialProvider struct {
 	// name is the required name of the credential provider. It must match the name of the
 	// provider executable as seen by the kubelet. The executable must be in the kubelet's
 	// bin directory (set by the --image-credential-provider-bin-dir flag).
+	// Required to be unique across all providers.
 	Name string `json:"name"`
 
 	// matchImages is a required list of strings used to match against images in order to

staging/src/k8s.io/kubelet/config/v1beta1/types.go

@@ -1004,7 +1004,7 @@ type CredentialProviderConfig struct {
 	// Multiple providers may match against a single image, in which case credentials
 	// from all providers will be returned to the kubelet. If multiple providers are called
 	// for a single image, the results are combined. If providers return overlapping
-	// auth keys, the value from the provider earlier in this list is used.
+	// auth keys, the value from the provider earlier in this list is attempted first.
 	Providers []CredentialProvider `json:"providers"`
 }
 
@@ -1014,6 +1014,7 @@ type CredentialProvider struct {
 	// name is the required name of the credential provider. It must match the name of the
 	// provider executable as seen by the kubelet. The executable must be in the kubelet's
 	// bin directory (set by the --image-credential-provider-bin-dir flag).
+	// Required to be unique across all providers.
 	Name string `json:"name"`
 
 	// matchImages is a required list of strings used to match against images in order to