diff --git a/pkg/credentialprovider/plugin/config.go b/pkg/credentialprovider/plugin/config.go index 841d5b22123..a3d03aa83cb 100644 --- a/pkg/credentialprovider/plugin/config.go +++ b/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)) } diff --git a/pkg/credentialprovider/plugin/config_test.go b/pkg/credentialprovider/plugin/config_test.go index b59feb51d74..8583964763c 100644 --- a/pkg/credentialprovider/plugin/config_test.go +++ b/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 "" +} diff --git a/pkg/credentialprovider/plugins.go b/pkg/credentialprovider/plugins.go index c2517eb4c16..05fb34d4c49 100644 --- a/pkg/credentialprovider/plugins.go +++ b/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) } }