diff --git a/staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation.go b/staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation.go index fc30efa92fa..b453dcfa02b 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation.go @@ -23,6 +23,7 @@ import ( "net/url" "strings" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/apis/config" ) @@ -40,6 +41,7 @@ const ( nonZeroErrFmt = "%s should be a positive value, or negative to disable" encryptionConfigNilErr = "EncryptionConfiguration can't be nil" invalidKMSConfigNameErrFmt = "invalid KMS provider name %s, must not contain ':'" + duplicateKMSConfigNameErrFmt = "duplicate KMS provider name %s, names must be unique" ) var ( @@ -66,6 +68,8 @@ func ValidateEncryptionConfiguration(c *config.EncryptionConfiguration) field.Er return allErrs } + // kmsProviderNames is used to track config names to ensure they are unique. + kmsProviderNames := sets.NewString() for i, conf := range c.Resources { r := root.Index(i).Child("resources") p := root.Index(i).Child("providers") @@ -84,7 +88,8 @@ func ValidateEncryptionConfiguration(c *config.EncryptionConfiguration) field.Er switch { case provider.KMS != nil: - allErrs = append(allErrs, validateKMSConfiguration(provider.KMS, path.Child("kms"))...) + allErrs = append(allErrs, validateKMSConfiguration(provider.KMS, path.Child("kms"), kmsProviderNames)...) + kmsProviderNames.Insert(provider.KMS.Name) case provider.AESGCM != nil: allErrs = append(allErrs, validateKeys(provider.AESGCM.Keys, path.Child("aesgcm").Child("keys"), aesKeySizes)...) case provider.AESCBC != nil: @@ -98,7 +103,7 @@ func ValidateEncryptionConfiguration(c *config.EncryptionConfiguration) field.Er return allErrs } -func validateSingleProvider(provider config.ProviderConfiguration, filedPath *field.Path) field.ErrorList { +func validateSingleProvider(provider config.ProviderConfiguration, fieldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} found := 0 @@ -119,11 +124,11 @@ func validateSingleProvider(provider config.ProviderConfiguration, filedPath *fi } if found == 0 { - return append(allErrs, field.Invalid(filedPath, provider, "provider does not contain any of the expected providers: KMS, AESGCM, AESCBC, Secretbox, Identity")) + return append(allErrs, field.Invalid(fieldPath, provider, "provider does not contain any of the expected providers: KMS, AESGCM, AESCBC, Secretbox, Identity")) } if found > 1 { - return append(allErrs, field.Invalid(filedPath, provider, moreThanOneElementErr)) + return append(allErrs, field.Invalid(fieldPath, provider, moreThanOneElementErr)) } return allErrs @@ -177,10 +182,10 @@ func validateKey(key config.Key, fieldPath *field.Path, expectedLen []int) field return allErrs } -func validateKMSConfiguration(c *config.KMSConfiguration, fieldPath *field.Path) field.ErrorList { +func validateKMSConfiguration(c *config.KMSConfiguration, fieldPath *field.Path, kmsProviderNames sets.String) field.ErrorList { allErrs := field.ErrorList{} - allErrs = append(allErrs, validateKMSConfigName(c, fieldPath.Child("name"))...) + allErrs = append(allErrs, validateKMSConfigName(c, fieldPath.Child("name"), kmsProviderNames)...) allErrs = append(allErrs, validateKMSTimeout(c, fieldPath.Child("timeout"))...) allErrs = append(allErrs, validateKMSEndpoint(c, fieldPath.Child("endpoint"))...) allErrs = append(allErrs, validateKMSCacheSize(c, fieldPath.Child("cachesize"))...) @@ -233,14 +238,22 @@ func validateKMSAPIVersion(c *config.KMSConfiguration, fieldPath *field.Path) fi return allErrs } -func validateKMSConfigName(c *config.KMSConfiguration, fieldPath *field.Path) field.ErrorList { +func validateKMSConfigName(c *config.KMSConfiguration, fieldPath *field.Path, kmsProviderNames sets.String) field.ErrorList { allErrs := field.ErrorList{} if c.Name == "" { allErrs = append(allErrs, field.Required(fieldPath, fmt.Sprintf(mandatoryFieldErrFmt, "name", "provider"))) } - if c.APIVersion != "v1" && strings.Contains(c.Name, ":") { - allErrs = append(allErrs, field.Invalid(fieldPath, c.Name, fmt.Sprintf(invalidKMSConfigNameErrFmt, c.Name))) + if c.APIVersion != "v1" { + // kms v2 providers are not allowed to have a ":" in their name + if strings.Contains(c.Name, ":") { + allErrs = append(allErrs, field.Invalid(fieldPath, c.Name, fmt.Sprintf(invalidKMSConfigNameErrFmt, c.Name))) + } + // kms v2 providers name should be unique across all kms providers (v1 and v2) + // TODO(aramase): make this check for v1 and v2 (all kms versions) once https://github.com/kubernetes/kubernetes/pull/113529 is merged + if kmsProviderNames.Has(c.Name) { + allErrs = append(allErrs, field.Invalid(fieldPath, c.Name, fmt.Sprintf(duplicateKMSConfigNameErrFmt, c.Name))) + } } return allErrs diff --git a/staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation_test.go b/staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation_test.go index 85087c4feef..01881004d4e 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation_test.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation_test.go @@ -23,12 +23,14 @@ import ( "github.com/google/go-cmp/cmp" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/validation/field" "k8s.io/apiserver/pkg/apis/config" ) func TestStructure(t *testing.T) { firstResourcePath := root.Index(0) + cacheSize := int32(1) testCases := []struct { desc string in *config.EncryptionConfiguration @@ -161,13 +163,190 @@ func TestStructure(t *testing.T) { }, want: field.ErrorList{}, }, + { + desc: "duplicate kms v2 config name with kms v1 config", + in: &config.EncryptionConfiguration{ + Resources: []config.ResourceConfiguration{ + { + Resources: []string{"secrets"}, + Providers: []config.ProviderConfiguration{ + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-1.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v1", + }, + }, + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-2.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v2", + }, + }, + }, + }, + }, + }, + want: field.ErrorList{ + field.Invalid(firstResourcePath.Child("providers").Index(1).Child("kms").Child("name"), + "foo", fmt.Sprintf(duplicateKMSConfigNameErrFmt, "foo")), + }, + }, + { + desc: "duplicate kms v2 config names", + in: &config.EncryptionConfiguration{ + Resources: []config.ResourceConfiguration{ + { + Resources: []string{"secrets"}, + Providers: []config.ProviderConfiguration{ + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-1.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v2", + }, + }, + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-2.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v2", + }, + }, + }, + }, + }, + }, + want: field.ErrorList{ + field.Invalid(firstResourcePath.Child("providers").Index(1).Child("kms").Child("name"), + "foo", fmt.Sprintf(duplicateKMSConfigNameErrFmt, "foo")), + }, + }, + { + desc: "duplicate kms v2 config name across providers", + in: &config.EncryptionConfiguration{ + Resources: []config.ResourceConfiguration{ + { + Resources: []string{"secrets"}, + Providers: []config.ProviderConfiguration{ + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-1.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v2", + }, + }, + }, + }, + { + Resources: []string{"secrets"}, + Providers: []config.ProviderConfiguration{ + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-2.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v2", + }, + }, + }, + }, + }, + }, + want: field.ErrorList{ + field.Invalid(root.Index(1).Child("providers").Index(0).Child("kms").Child("name"), + "foo", fmt.Sprintf(duplicateKMSConfigNameErrFmt, "foo")), + }, + }, + { + desc: "duplicate kms config name with v1 and v2 across providers", + in: &config.EncryptionConfiguration{ + Resources: []config.ResourceConfiguration{ + { + Resources: []string{"secrets"}, + Providers: []config.ProviderConfiguration{ + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-1.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v1", + }, + }, + }, + }, + { + Resources: []string{"secrets"}, + Providers: []config.ProviderConfiguration{ + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-2.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v2", + }, + }, + }, + }, + }, + }, + want: field.ErrorList{ + field.Invalid(root.Index(1).Child("providers").Index(0).Child("kms").Child("name"), + "foo", fmt.Sprintf(duplicateKMSConfigNameErrFmt, "foo")), + }, + }, + { + desc: "duplicate kms v1 config names shouldn't error", + in: &config.EncryptionConfiguration{ + Resources: []config.ResourceConfiguration{ + { + Resources: []string{"secrets"}, + Providers: []config.ProviderConfiguration{ + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-1.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v1", + }, + }, + { + KMS: &config.KMSConfiguration{ + Name: "foo", + Endpoint: "unix:///tmp/kms-provider-2.socket", + Timeout: &metav1.Duration{Duration: 3 * time.Second}, + CacheSize: &cacheSize, + APIVersion: "v1", + }, + }, + }, + }, + }, + }, + want: field.ErrorList{}, + }, } for _, tt := range testCases { t.Run(tt.desc, func(t *testing.T) { got := ValidateEncryptionConfiguration(tt.in) if d := cmp.Diff(tt.want, got); d != "" { - t.Fatalf("EncryptionConfiguratoin validation results mismatch (-want +got):\n%s", d) + t.Fatalf("EncryptionConfiguration validation results mismatch (-want +got):\n%s", d) } }) } @@ -392,9 +571,10 @@ func TestKMSProviderName(t *testing.T) { nameField := field.NewPath("Resource").Index(0).Child("Provider").Index(0).Child("KMS").Child("name") testCases := []struct { - desc string - in *config.KMSConfiguration - want field.ErrorList + desc string + in *config.KMSConfiguration + kmsProviderNames sets.String + want field.ErrorList }{ { desc: "valid name", @@ -415,11 +595,24 @@ func TestKMSProviderName(t *testing.T) { field.Invalid(nameField, "foo:bar", fmt.Sprintf(invalidKMSConfigNameErrFmt, "foo:bar")), }, }, + { + desc: "invalid name with : but api version is v1", + in: &config.KMSConfiguration{Name: "foo:bar", APIVersion: "v1"}, + want: field.ErrorList{}, + }, + { + desc: "duplicate name", + in: &config.KMSConfiguration{Name: "foo"}, + kmsProviderNames: sets.NewString("foo"), + want: field.ErrorList{ + field.Invalid(nameField, "foo", fmt.Sprintf(duplicateKMSConfigNameErrFmt, "foo")), + }, + }, } for _, tt := range testCases { t.Run(tt.desc, func(t *testing.T) { - got := validateKMSConfigName(tt.in, nameField) + got := validateKMSConfigName(tt.in, nameField, tt.kmsProviderNames) if d := cmp.Diff(tt.want, got); d != "" { t.Fatalf("KMS Provider validation mismatch (-want +got):\n%s", d) }