[KMS]: validate duplicate kms config name for v1 and v2 when reload=true

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

staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation.go

@@ -55,7 +55,7 @@ var (
 )
 
 // ValidateEncryptionConfiguration validates a v1.EncryptionConfiguration.
-func ValidateEncryptionConfiguration(c *config.EncryptionConfiguration) field.ErrorList {
+func ValidateEncryptionConfiguration(c *config.EncryptionConfiguration, reload bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 
 	if c == nil {
@@ -88,7 +88,7 @@ func ValidateEncryptionConfiguration(c *config.EncryptionConfiguration) field.Er
 
 			switch {
 			case provider.KMS != nil:
-				allErrs = append(allErrs, validateKMSConfiguration(provider.KMS, path.Child("kms"), kmsProviderNames)...)
+				allErrs = append(allErrs, validateKMSConfiguration(provider.KMS, path.Child("kms"), kmsProviderNames, reload)...)
 				kmsProviderNames.Insert(provider.KMS.Name)
 			case provider.AESGCM != nil:
 				allErrs = append(allErrs, validateKeys(provider.AESGCM.Keys, path.Child("aesgcm").Child("keys"), aesKeySizes)...)
@@ -182,10 +182,10 @@ func validateKey(key config.Key, fieldPath *field.Path, expectedLen []int) field
 	return allErrs
 }
 
-func validateKMSConfiguration(c *config.KMSConfiguration, fieldPath *field.Path, kmsProviderNames sets.String) field.ErrorList {
+func validateKMSConfiguration(c *config.KMSConfiguration, fieldPath *field.Path, kmsProviderNames sets.String, reload bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 
-	allErrs = append(allErrs, validateKMSConfigName(c, fieldPath.Child("name"), kmsProviderNames)...)
+	allErrs = append(allErrs, validateKMSConfigName(c, fieldPath.Child("name"), kmsProviderNames, reload)...)
 	allErrs = append(allErrs, validateKMSTimeout(c, fieldPath.Child("timeout"))...)
 	allErrs = append(allErrs, validateKMSEndpoint(c, fieldPath.Child("endpoint"))...)
 	allErrs = append(allErrs, validateKMSCacheSize(c, fieldPath.Child("cachesize"))...)
@@ -238,19 +238,20 @@ func validateKMSAPIVersion(c *config.KMSConfiguration, fieldPath *field.Path) fi
 	return allErrs
 }
 
-func validateKMSConfigName(c *config.KMSConfiguration, fieldPath *field.Path, kmsProviderNames sets.String) field.ErrorList {
+func validateKMSConfigName(c *config.KMSConfiguration, fieldPath *field.Path, kmsProviderNames sets.String, reload bool) field.ErrorList {
 	allErrs := field.ErrorList{}
 	if c.Name == "" {
 		allErrs = append(allErrs, field.Required(fieldPath, fmt.Sprintf(mandatoryFieldErrFmt, "name", "provider")))
 	}
 
-	if c.APIVersion != "v1" {
+	// kms v2 providers are not allowed to have a ":" in their name
-		// kms v2 providers are not allowed to have a ":" in their name
+	if c.APIVersion != "v1" && strings.Contains(c.Name, ":") {
-		if strings.Contains(c.Name, ":") {
+		allErrs = append(allErrs, field.Invalid(fieldPath, c.Name, fmt.Sprintf(invalidKMSConfigNameErrFmt, 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)
+	// kms v2 providers name must always 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
+	// kms v1 provider names must be unique across all kms providers (v1 and v2) when hot reloading of encryption configuration is enabled (reload=true)
+	if reload || c.APIVersion != "v1" {
 		if kmsProviderNames.Has(c.Name) {
 			allErrs = append(allErrs, field.Invalid(fieldPath, c.Name, fmt.Sprintf(duplicateKMSConfigNameErrFmt, c.Name)))
 		}

staging/src/k8s.io/apiserver/pkg/apis/config/validation/validation_test.go

@@ -32,9 +32,10 @@ func TestStructure(t *testing.T) {
 	firstResourcePath := root.Index(0)
 	cacheSize := int32(1)
 	testCases := []struct {
-		desc string
+		desc   string
-		in   *config.EncryptionConfiguration
+		in     *config.EncryptionConfiguration
-		want field.ErrorList
+		reload bool
+		want   field.ErrorList
 	}{
 		{
 			desc: "nil encryption config",
@@ -340,11 +341,46 @@ func TestStructure(t *testing.T) {
 			},
 			want: field.ErrorList{},
 		},
+		{
+			desc: "duplicate kms v1 config names should error when reload=true",
+			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",
+								},
+							},
+						},
+					},
+				},
+			},
+			reload: true,
+			want: field.ErrorList{
+				field.Invalid(root.Index(0).Child("providers").Index(1).Child("kms").Child("name"),
+					"foo", fmt.Sprintf(duplicateKMSConfigNameErrFmt, "foo")),
+			},
+		},
 	}
 
 	for _, tt := range testCases {
 		t.Run(tt.desc, func(t *testing.T) {
-			got := ValidateEncryptionConfiguration(tt.in)
+			got := ValidateEncryptionConfiguration(tt.in, tt.reload)
 			if d := cmp.Diff(tt.want, got); d != "" {
 				t.Fatalf("EncryptionConfiguration validation results mismatch (-want +got):\n%s", d)
 			}
@@ -573,6 +609,7 @@ func TestKMSProviderName(t *testing.T) {
 	testCases := []struct {
 		desc             string
 		in               *config.KMSConfiguration
+		reload           bool
 		kmsProviderNames sets.String
 		want             field.ErrorList
 	}{
@@ -601,8 +638,32 @@ func TestKMSProviderName(t *testing.T) {
 			want: field.ErrorList{},
 		},
 		{
-			desc:             "duplicate name",
+			desc:             "duplicate name, kms v2, reload=false",
-			in:               &config.KMSConfiguration{Name: "foo"},
+			in:               &config.KMSConfiguration{APIVersion: "v2", Name: "foo"},
+			kmsProviderNames: sets.NewString("foo"),
+			want: field.ErrorList{
+				field.Invalid(nameField, "foo", fmt.Sprintf(duplicateKMSConfigNameErrFmt, "foo")),
+			},
+		},
+		{
+			desc:             "duplicate name, kms v2, reload=true",
+			in:               &config.KMSConfiguration{APIVersion: "v2", Name: "foo"},
+			reload:           true,
+			kmsProviderNames: sets.NewString("foo"),
+			want: field.ErrorList{
+				field.Invalid(nameField, "foo", fmt.Sprintf(duplicateKMSConfigNameErrFmt, "foo")),
+			},
+		},
+		{
+			desc:             "duplicate name, kms v1, reload=false",
+			in:               &config.KMSConfiguration{APIVersion: "v1", Name: "foo"},
+			kmsProviderNames: sets.NewString("foo"),
+			want:             field.ErrorList{},
+		},
+		{
+			desc:             "duplicate name, kms v1, reload=true",
+			in:               &config.KMSConfiguration{APIVersion: "v1", Name: "foo"},
+			reload:           true,
 			kmsProviderNames: sets.NewString("foo"),
 			want: field.ErrorList{
 				field.Invalid(nameField, "foo", fmt.Sprintf(duplicateKMSConfigNameErrFmt, "foo")),
@@ -612,7 +673,7 @@ func TestKMSProviderName(t *testing.T) {
 
 	for _, tt := range testCases {
 		t.Run(tt.desc, func(t *testing.T) {
-			got := validateKMSConfigName(tt.in, nameField, tt.kmsProviderNames)
+			got := validateKMSConfigName(tt.in, nameField, tt.kmsProviderNames, tt.reload)
 			if d := cmp.Diff(tt.want, got); d != "" {
 				t.Fatalf("KMS Provider validation mismatch (-want +got):\n%s", d)
 			}

staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config.go

@@ -117,7 +117,7 @@ func (h *kmsv2PluginProbe) toHealthzCheck(idx int) healthz.HealthChecker {
 // It may launch multiple go routines whose lifecycle is controlled by stopCh.
 // If reload is true, or KMS v2 plugins are used with no KMS v1 plugins, the returned slice of health checkers will always be of length 1.
 func LoadEncryptionConfig(filepath string, reload bool, stopCh <-chan struct{}) (map[schema.GroupResource]value.Transformer, []healthz.HealthChecker, error) {
-	config, err := loadConfig(filepath)
+	config, err := loadConfig(filepath, reload)
 	if err != nil {
 		return nil, nil, fmt.Errorf("error while parsing file: %w", err)
 	}
@@ -262,7 +262,7 @@ func isKMSv2ProviderHealthy(name string, response *envelopekmsv2.StatusResponse)
 	return nil
 }
 
-func loadConfig(filepath string) (*apiserverconfig.EncryptionConfiguration, error) {
+func loadConfig(filepath string, reload bool) (*apiserverconfig.EncryptionConfiguration, error) {
 	f, err := os.Open(filepath)
 	if err != nil {
 		return nil, fmt.Errorf("error opening encryption provider configuration file %q: %w", filepath, err)
@@ -291,7 +291,7 @@ func loadConfig(filepath string) (*apiserverconfig.EncryptionConfiguration, erro
 		return nil, fmt.Errorf("got unexpected config type: %v", gvk)
 	}
 
-	return config, validation.ValidateEncryptionConfiguration(config).ToAggregate()
+	return config, validation.ValidateEncryptionConfiguration(config, reload).ToAggregate()
 }
 
 func prefixTransformersAndProbes(config apiserverconfig.ResourceConfiguration, stopCh <-chan struct{}) ([]value.PrefixTransformer, []healthChecker, *kmsState, error) {

staging/src/k8s.io/apiserver/pkg/server/options/encryptionconfig/config_test.go

@@ -114,7 +114,7 @@ func newMockErrorEnvelopeKMSv2Service(endpoint string, timeout time.Duration) (e
 
 func TestLegacyConfig(t *testing.T) {
 	legacyV1Config := "testdata/valid-configs/legacy.yaml"
-	legacyConfigObject, err := loadConfig(legacyV1Config)
+	legacyConfigObject, err := loadConfig(legacyV1Config, false)
 	cacheSize := int32(10)
 	if err != nil {
 		t.Fatalf("error while parsing configuration file: %s.\nThe file was:\n%s", err, legacyV1Config)
@@ -323,7 +323,7 @@ func TestKMSPluginHealthz(t *testing.T) {
 
 	for _, tt := range testCases {
 		t.Run(tt.desc, func(t *testing.T) {
-			config, err := loadConfig(tt.config)
+			config, err := loadConfig(tt.config, false)
 			if errStr := errString(err); errStr != tt.wantErr {
 				t.Fatalf("unexpected error state got=%s want=%s", errStr, tt.wantErr)
 			}