[KMSv2]: add validation for duplicate kms config name

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>

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,15 +238,23 @@ 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, ":") {
+	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
 }

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)
 			}
 		})
 	}
@@ -394,6 +573,7 @@ func TestKMSProviderName(t *testing.T) {
 	testCases := []struct {
 		desc             string
 		in               *config.KMSConfiguration
+		kmsProviderNames sets.String
 		want             field.ErrorList
 	}{
 		{
@@ -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)
 			}