From 9a331bbf59cbe1ffbeb5be18f3eac30fd78c1ddd Mon Sep 17 00:00:00 2001 From: Anish Ramasekar Date: Thu, 16 Jan 2025 14:37:34 -0800 Subject: [PATCH 1/2] credential provider config: validate duplicate names early and preserve provider order Signed-off-by: Anish Ramasekar --- pkg/credentialprovider/plugin/config.go | 16 +++-- pkg/credentialprovider/plugin/config_test.go | 69 ++++++++++++++------ pkg/credentialprovider/plugins.go | 37 +++++------ 3 files changed, 75 insertions(+), 47 deletions(-) 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) } } From 92e35e7618ff30e1c8e79f5efb3788dfedf24bb3 Mon Sep 17 00:00:00 2001 From: Anish Ramasekar Date: Thu, 16 Jan 2025 14:54:37 -0800 Subject: [PATCH 2/2] update credential provider godoc with unique provider name req Signed-off-by: Anish Ramasekar --- pkg/generated/openapi/zz_generated.openapi.go | 12 ++++++------ pkg/kubelet/apis/config/types.go | 3 ++- staging/src/k8s.io/kubelet/config/v1/types.go | 3 ++- staging/src/k8s.io/kubelet/config/v1alpha1/types.go | 3 ++- staging/src/k8s.io/kubelet/config/v1beta1/types.go | 3 ++- 5 files changed, 14 insertions(+), 10 deletions(-) diff --git a/pkg/generated/openapi/zz_generated.openapi.go b/pkg/generated/openapi/zz_generated.openapi.go index c63a3136719..356e5eaa785 100644 --- a/pkg/generated/openapi/zz_generated.openapi.go +++ b/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{ diff --git a/pkg/kubelet/apis/config/types.go b/pkg/kubelet/apis/config/types.go index a7024cc993c..132cc3f1094 100644 --- a/pkg/kubelet/apis/config/types.go +++ b/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 diff --git a/staging/src/k8s.io/kubelet/config/v1/types.go b/staging/src/k8s.io/kubelet/config/v1/types.go index 1b59a7d8dc6..ca3a3e92f1e 100644 --- a/staging/src/k8s.io/kubelet/config/v1/types.go +++ b/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 diff --git a/staging/src/k8s.io/kubelet/config/v1alpha1/types.go b/staging/src/k8s.io/kubelet/config/v1alpha1/types.go index 92daa99667e..6de4250c6cb 100644 --- a/staging/src/k8s.io/kubelet/config/v1alpha1/types.go +++ b/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 diff --git a/staging/src/k8s.io/kubelet/config/v1beta1/types.go b/staging/src/k8s.io/kubelet/config/v1beta1/types.go index a2cd80972d8..ce2d3121373 100644 --- a/staging/src/k8s.io/kubelet/config/v1beta1/types.go +++ b/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