Require webhook names to be unique in v1

pkg/apis/admissionregistration/validation/validation.go

@@ -22,6 +22,7 @@ import (
 
 	genericvalidation "k8s.io/apimachinery/pkg/api/validation"
 	metav1validation "k8s.io/apimachinery/pkg/apis/meta/v1/validation"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/sets"
 	utilvalidation "k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/apimachinery/pkg/util/validation/field"
@@ -194,28 +195,44 @@ func validateAdmissionReviewVersions(versions []string, requireRecognizedVersion
 	return allErrors
 }
 
-func ValidateValidatingWebhookConfiguration(e *admissionregistration.ValidatingWebhookConfiguration) field.ErrorList {
+// ValidateValidatingWebhookConfiguration validates a webhook before creation.
-	return validateValidatingWebhookConfiguration(e, true)
+func ValidateValidatingWebhookConfiguration(e *admissionregistration.ValidatingWebhookConfiguration, requestGV schema.GroupVersion) field.ErrorList {
+	return validateValidatingWebhookConfiguration(e, true, requireUniqueWebhookNames(requestGV))
 }
 
-func validateValidatingWebhookConfiguration(e *admissionregistration.ValidatingWebhookConfiguration, requireRecognizedVersion bool) field.ErrorList {
+func validateValidatingWebhookConfiguration(e *admissionregistration.ValidatingWebhookConfiguration, requireRecognizedVersion, requireUniqueWebhookNames bool) field.ErrorList {
 	allErrors := genericvalidation.ValidateObjectMeta(&e.ObjectMeta, false, genericvalidation.NameIsDNSSubdomain, field.NewPath("metadata"))
+	hookNames := sets.NewString()
 	for i, hook := range e.Webhooks {
 		allErrors = append(allErrors, validateValidatingWebhook(&hook, field.NewPath("webhooks").Index(i))...)
 		allErrors = append(allErrors, validateAdmissionReviewVersions(hook.AdmissionReviewVersions, requireRecognizedVersion, field.NewPath("webhooks").Index(i).Child("admissionReviewVersions"))...)
+		if requireUniqueWebhookNames && len(hook.Name) > 0 {
+			if hookNames.Has(hook.Name) {
+				allErrors = append(allErrors, field.Duplicate(field.NewPath("webhooks").Index(i).Child("name"), hook.Name))
+			}
+			hookNames.Insert(hook.Name)
+		}
 	}
 	return allErrors
 }
 
-func ValidateMutatingWebhookConfiguration(e *admissionregistration.MutatingWebhookConfiguration) field.ErrorList {
+// ValidateMutatingWebhookConfiguration validates a webhook before creation.
-	return validateMutatingWebhookConfiguration(e, true)
+func ValidateMutatingWebhookConfiguration(e *admissionregistration.MutatingWebhookConfiguration, requestGV schema.GroupVersion) field.ErrorList {
+	return validateMutatingWebhookConfiguration(e, true, requireUniqueWebhookNames(requestGV))
 }
 
-func validateMutatingWebhookConfiguration(e *admissionregistration.MutatingWebhookConfiguration, requireRecognizedVersion bool) field.ErrorList {
+func validateMutatingWebhookConfiguration(e *admissionregistration.MutatingWebhookConfiguration, requireRecognizedVersion, requireUniqueWebhookNames bool) field.ErrorList {
 	allErrors := genericvalidation.ValidateObjectMeta(&e.ObjectMeta, false, genericvalidation.NameIsDNSSubdomain, field.NewPath("metadata"))
+	hookNames := sets.NewString()
 	for i, hook := range e.Webhooks {
 		allErrors = append(allErrors, validateMutatingWebhook(&hook, field.NewPath("webhooks").Index(i))...)
 		allErrors = append(allErrors, validateAdmissionReviewVersions(hook.AdmissionReviewVersions, requireRecognizedVersion, field.NewPath("webhooks").Index(i).Child("admissionReviewVersions"))...)
+		if requireUniqueWebhookNames && len(hook.Name) > 0 {
+			if hookNames.Has(hook.Name) {
+				allErrors = append(allErrors, field.Duplicate(field.NewPath("webhooks").Index(i).Child("name"), hook.Name))
+			}
+			hookNames.Insert(hook.Name)
+		}
 	}
 	return allErrors
 }
@@ -403,10 +420,47 @@ func validatingHasAcceptedAdmissionReviewVersions(webhooks []admissionregistrati
 	return true
 }
 
-func ValidateValidatingWebhookConfigurationUpdate(newC, oldC *admissionregistration.ValidatingWebhookConfiguration) field.ErrorList {
+// mutatingHasUniqueWebhookNames returns true if all webhooks have unique names
-	return validateValidatingWebhookConfiguration(newC, validatingHasAcceptedAdmissionReviewVersions(oldC.Webhooks))
+func mutatingHasUniqueWebhookNames(webhooks []admissionregistration.MutatingWebhook) bool {
+	names := sets.NewString()
+	for _, hook := range webhooks {
+		if names.Has(hook.Name) {
+			return false
+		}
+		names.Insert(hook.Name)
+	}
+	return true
 }
 
-func ValidateMutatingWebhookConfigurationUpdate(newC, oldC *admissionregistration.MutatingWebhookConfiguration) field.ErrorList {
+// validatingHasUniqueWebhookNames returns true if all webhooks have unique names
-	return validateMutatingWebhookConfiguration(newC, mutatingHasAcceptedAdmissionReviewVersions(oldC.Webhooks))
+func validatingHasUniqueWebhookNames(webhooks []admissionregistration.ValidatingWebhook) bool {
+	names := sets.NewString()
+	for _, hook := range webhooks {
+		if names.Has(hook.Name) {
+			return false
+		}
+		names.Insert(hook.Name)
+	}
+	return true
+}
+
+func ValidateValidatingWebhookConfigurationUpdate(newC, oldC *admissionregistration.ValidatingWebhookConfiguration, requestGV schema.GroupVersion) field.ErrorList {
+	return validateValidatingWebhookConfiguration(
+		newC,
+		validatingHasAcceptedAdmissionReviewVersions(oldC.Webhooks),
+		requireUniqueWebhookNames(requestGV) && validatingHasUniqueWebhookNames(oldC.Webhooks),
+	)
+}
+
+func ValidateMutatingWebhookConfigurationUpdate(newC, oldC *admissionregistration.MutatingWebhookConfiguration, requestGV schema.GroupVersion) field.ErrorList {
+	return validateMutatingWebhookConfiguration(
+		newC,
+		mutatingHasAcceptedAdmissionReviewVersions(oldC.Webhooks),
+		requireUniqueWebhookNames(requestGV) && mutatingHasUniqueWebhookNames(oldC.Webhooks),
+	)
+}
+
+// requireUniqueWebhookNames returns true for all requests except v1beta1 (for backwards compatibility)
+func requireUniqueWebhookNames(requestGV schema.GroupVersion) bool {
+	return requestGV != (schema.GroupVersion{Group: admissionregistration.GroupName, Version: "v1beta1"})
 }

pkg/apis/admissionregistration/validation/validation_test.go

@@ -21,6 +21,7 @@ import (
 	"testing"
 
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/kubernetes/pkg/apis/admissionregistration"
 )
 
@@ -55,6 +56,7 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) {
 	tests := []struct {
 		name          string
 		config        *admissionregistration.ValidatingWebhookConfiguration
+		gv            schema.GroupVersion
 		expectedError string
 	}{
 		{
@@ -145,6 +147,40 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) {
 			}, true),
 			expectedError: `webhooks[1].name: Invalid value: "k8s.io": should be a domain with at least three segments separated by dots, webhooks[2].name: Required value`,
 		},
+		{
+			name: "Webhooks must have unique names when not created via v1beta1",
+			config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+			}, true),
+			gv:            schema.GroupVersion{Group: "foo", Version: "bar"},
+			expectedError: `webhooks[1].name: Duplicate value: "webhook.k8s.io"`,
+		},
+		{
+			name: "Webhooks can have duplicate names when created via v1beta1",
+			config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+			}, true),
+			gv:            schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"},
+			expectedError: ``,
+		},
 		{
 			name: "Operations must not be empty or nil",
 			config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{
@@ -739,7 +775,7 @@ func TestValidateValidatingWebhookConfiguration(t *testing.T) {
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			errs := ValidateValidatingWebhookConfiguration(test.config)
+			errs := ValidateValidatingWebhookConfiguration(test.config, test.gv)
 			err := errs.ToAggregate()
 			if err != nil {
 				if e, a := test.expectedError, err.Error(); !strings.Contains(a, e) || e == "" {
@@ -765,6 +801,7 @@ func TestValidateValidatingWebhookConfigurationUpdate(t *testing.T) {
 		name          string
 		oldconfig     *admissionregistration.ValidatingWebhookConfiguration
 		config        *admissionregistration.ValidatingWebhookConfiguration
+		gv            schema.GroupVersion
 		expectedError string
 	}{
 		{
@@ -845,10 +882,87 @@ func TestValidateValidatingWebhookConfigurationUpdate(t *testing.T) {
 			}, false),
 			expectedError: `Invalid value: []string{"invalid-v1"}`,
 		},
+		{
+			name: "Webhooks must have unique names when not updated via v1beta1",
+			config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+			}, true),
+			oldconfig: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+			}, false),
+			gv:            schema.GroupVersion{Group: "foo", Version: "bar"},
+			expectedError: `webhooks[1].name: Duplicate value: "webhook.k8s.io"`,
+		},
+		{
+			name: "Webhooks can have duplicate names when old config has duplicate names",
+			config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+			}, true),
+			oldconfig: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+			}, true),
+			gv:            schema.GroupVersion{Group: "foo", Version: "bar"},
+			expectedError: ``,
+		},
+		{
+			name: "Webhooks can have duplicate names when updated via v1beta1",
+			config: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+			}, true),
+			oldconfig: newValidatingWebhookConfiguration([]admissionregistration.ValidatingWebhook{
+				{
+					Name:         "webhook.k8s.io",
+					ClientConfig: validClientConfig,
+					SideEffects:  validSideEffect,
+				},
+			}, false),
+			gv:            schema.GroupVersion{Group: "admissionregistration.k8s.io", Version: "v1beta1"},
+			expectedError: ``,
+		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			errs := ValidateValidatingWebhookConfigurationUpdate(test.config, test.oldconfig)
+			errs := ValidateValidatingWebhookConfigurationUpdate(test.config, test.oldconfig, test.gv)
 			err := errs.ToAggregate()
 			if err != nil {
 				if e, a := test.expectedError, err.Error(); !strings.Contains(a, e) || e == "" {

pkg/registry/admissionregistration/mutatingwebhookconfiguration/strategy.go

@@ -21,7 +21,9 @@ import (
 	"reflect"
 
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	"k8s.io/kubernetes/pkg/apis/admissionregistration"
@@ -63,8 +65,13 @@ func (mutatingWebhookConfigurationStrategy) PrepareForUpdate(ctx context.Context
 
 // Validate validates a new mutatingWebhookConfiguration.
 func (mutatingWebhookConfigurationStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
+	var groupVersion schema.GroupVersion
+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+	}
+
 	ic := obj.(*admissionregistration.MutatingWebhookConfiguration)
-	return validation.ValidateMutatingWebhookConfiguration(ic)
+	return validation.ValidateMutatingWebhookConfiguration(ic, groupVersion)
 }
 
 // Canonicalize normalizes the object after validation.
@@ -78,7 +85,12 @@ func (mutatingWebhookConfigurationStrategy) AllowCreateOnUpdate() bool {
 
 // ValidateUpdate is the default update validation for an end user.
 func (mutatingWebhookConfigurationStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
-	return validation.ValidateMutatingWebhookConfigurationUpdate(obj.(*admissionregistration.MutatingWebhookConfiguration), old.(*admissionregistration.MutatingWebhookConfiguration))
+	var groupVersion schema.GroupVersion
+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+	}
+
+	return validation.ValidateMutatingWebhookConfigurationUpdate(obj.(*admissionregistration.MutatingWebhookConfiguration), old.(*admissionregistration.MutatingWebhookConfiguration), groupVersion)
 }
 
 // AllowUnconditionalUpdate is the default update policy for mutatingWebhookConfiguration objects. Status update should

pkg/registry/admissionregistration/validatingwebhookconfiguration/strategy.go

@@ -21,7 +21,9 @@ import (
 	"reflect"
 
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/validation/field"
+	genericapirequest "k8s.io/apiserver/pkg/endpoints/request"
 	"k8s.io/apiserver/pkg/storage/names"
 	"k8s.io/kubernetes/pkg/api/legacyscheme"
 	"k8s.io/kubernetes/pkg/apis/admissionregistration"
@@ -63,7 +65,12 @@ func (validatingWebhookConfigurationStrategy) PrepareForUpdate(ctx context.Conte
 
 // Validate validates a new validatingWebhookConfiguration.
 func (validatingWebhookConfigurationStrategy) Validate(ctx context.Context, obj runtime.Object) field.ErrorList {
-	return validation.ValidateValidatingWebhookConfiguration(obj.(*admissionregistration.ValidatingWebhookConfiguration))
+	var groupVersion schema.GroupVersion
+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+	}
+
+	return validation.ValidateValidatingWebhookConfiguration(obj.(*admissionregistration.ValidatingWebhookConfiguration), groupVersion)
 }
 
 // Canonicalize normalizes the object after validation.
@@ -77,7 +84,12 @@ func (validatingWebhookConfigurationStrategy) AllowCreateOnUpdate() bool {
 
 // ValidateUpdate is the default update validation for an end user.
 func (validatingWebhookConfigurationStrategy) ValidateUpdate(ctx context.Context, obj, old runtime.Object) field.ErrorList {
-	return validation.ValidateValidatingWebhookConfigurationUpdate(obj.(*admissionregistration.ValidatingWebhookConfiguration), old.(*admissionregistration.ValidatingWebhookConfiguration))
+	var groupVersion schema.GroupVersion
+	if requestInfo, found := genericapirequest.RequestInfoFrom(ctx); found {
+		groupVersion = schema.GroupVersion{Group: requestInfo.APIGroup, Version: requestInfo.APIVersion}
+	}
+
+	return validation.ValidateValidatingWebhookConfigurationUpdate(obj.(*admissionregistration.ValidatingWebhookConfiguration), old.(*admissionregistration.ValidatingWebhookConfiguration), groupVersion)
 }
 
 // AllowUnconditionalUpdate is the default update policy for validatingWebhookConfiguration objects. Status update should