Split apart defaulting and validation webhook (#2147)

Related: knative/pkg#848

cmd/webhook/main.go

@@ -37,9 +37,34 @@ import (
 	"knative.dev/pkg/webhook/certificates"
 	"knative.dev/pkg/webhook/configmaps"
 	"knative.dev/pkg/webhook/resourcesemantics"
+	"knative.dev/pkg/webhook/resourcesemantics/defaulting"
+	"knative.dev/pkg/webhook/resourcesemantics/validation"
 )
 
-func NewResourceAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
+var types = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
+	// For group eventing.knative.dev.
+	eventingv1alpha1.SchemeGroupVersion.WithKind("Broker"):    &eventingv1alpha1.Broker{},
+	eventingv1alpha1.SchemeGroupVersion.WithKind("Trigger"):   &eventingv1alpha1.Trigger{},
+	eventingv1alpha1.SchemeGroupVersion.WithKind("EventType"): &eventingv1alpha1.EventType{},
+
+	// For group messaging.knative.dev.
+	messagingv1alpha1.SchemeGroupVersion.WithKind("InMemoryChannel"): &messagingv1alpha1.InMemoryChannel{},
+	messagingv1alpha1.SchemeGroupVersion.WithKind("Sequence"):        &messagingv1alpha1.Sequence{},
+	messagingv1alpha1.SchemeGroupVersion.WithKind("Parallel"):        &messagingv1alpha1.Parallel{},
+	messagingv1alpha1.SchemeGroupVersion.WithKind("Channel"):         &messagingv1alpha1.Channel{},
+	messagingv1alpha1.SchemeGroupVersion.WithKind("Subscription"):    &messagingv1alpha1.Subscription{},
+
+	// For group sources.eventing.knative.dev.
+	sourcesv1alpha1.SchemeGroupVersion.WithKind("ApiServerSource"): &sourcesv1alpha1.ApiServerSource{},
+	sourcesv1alpha1.SchemeGroupVersion.WithKind("ContainerSource"): &sourcesv1alpha1.ContainerSource{},
+	sourcesv1alpha1.SchemeGroupVersion.WithKind("CronJobSource"):   &sourcesv1alpha1.CronJobSource{},
+
+	// For group flows.knative.dev
+	flowsv1alpha1.SchemeGroupVersion.WithKind("Parallel"): &flowsv1alpha1.Parallel{},
+	flowsv1alpha1.SchemeGroupVersion.WithKind("Sequence"): &flowsv1alpha1.Sequence{},
+}
+
+func NewDefaultingAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
 	logger := logging.FromContext(ctx)
 
 	// Decorate contexts with the current state of the config.
@@ -59,37 +84,16 @@ func NewResourceAdmissionController(ctx context.Context, cmw configmap.Watcher)
 	eventingduckv1alpha1.ChannelDefaulterSingleton = chDefaulter
 	cmw.Watch(defaultchannel.ConfigMapName, chDefaulter.UpdateConfigMap)
 
-	return resourcesemantics.NewAdmissionController(ctx,
+	return defaulting.NewAdmissionController(ctx,
 
 		// Name of the resource webhook.
-		"webhook.eventing.knative.dev",
+		"defaulting.webhook.eventing.knative.dev",
 
 		// The path on which to serve the webhook.
-		"/",
+		"/defaulting",
 
 		// The resources to validate and default.
-		map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
-			// For group eventing.knative.dev.
-			eventingv1alpha1.SchemeGroupVersion.WithKind("Broker"):    &eventingv1alpha1.Broker{},
-			eventingv1alpha1.SchemeGroupVersion.WithKind("Trigger"):   &eventingv1alpha1.Trigger{},
-			eventingv1alpha1.SchemeGroupVersion.WithKind("EventType"): &eventingv1alpha1.EventType{},
-
-			// For group messaging.knative.dev.
-			messagingv1alpha1.SchemeGroupVersion.WithKind("InMemoryChannel"): &messagingv1alpha1.InMemoryChannel{},
-			messagingv1alpha1.SchemeGroupVersion.WithKind("Sequence"):        &messagingv1alpha1.Sequence{},
-			messagingv1alpha1.SchemeGroupVersion.WithKind("Parallel"):        &messagingv1alpha1.Parallel{},
-			messagingv1alpha1.SchemeGroupVersion.WithKind("Channel"):         &messagingv1alpha1.Channel{},
-			messagingv1alpha1.SchemeGroupVersion.WithKind("Subscription"):    &messagingv1alpha1.Subscription{},
-
-			// For group sources.eventing.knative.dev.
-			sourcesv1alpha1.SchemeGroupVersion.WithKind("ApiServerSource"): &sourcesv1alpha1.ApiServerSource{},
-			sourcesv1alpha1.SchemeGroupVersion.WithKind("ContainerSource"): &sourcesv1alpha1.ContainerSource{},
-			sourcesv1alpha1.SchemeGroupVersion.WithKind("CronJobSource"):   &sourcesv1alpha1.CronJobSource{},
-
-			// For group flows.knative.dev
-			flowsv1alpha1.SchemeGroupVersion.WithKind("Parallel"): &flowsv1alpha1.Parallel{},
-			flowsv1alpha1.SchemeGroupVersion.WithKind("Sequence"): &flowsv1alpha1.Sequence{},
-		},
+		types,
 
 		// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
 		ctxFunc,
@@ -99,6 +103,29 @@ func NewResourceAdmissionController(ctx context.Context, cmw configmap.Watcher)
 	)
 }
 
+func NewValidationAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
+	return validation.NewAdmissionController(ctx,
+
+		// Name of the resource webhook.
+		"validation.webhook.eventing.knative.dev",
+
+		// The path on which to serve the webhook.
+		"/validation",
+
+		// The resources to validate and default.
+		types,
+
+		// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
+		func(ctx context.Context) context.Context {
+			// return v1.WithUpgradeViaDefaulting(store.ToContext(ctx))
+			return ctx
+		},
+
+		// Whether to disallow unknown fields.
+		true,
+	)
+}
+
 func NewConfigValidationController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
 	return configmaps.NewAdmissionController(ctx,
 
@@ -127,7 +154,8 @@ func main() {
 
 	sharedmain.MainWithContext(ctx, logconfig.WebhookName(),
 		certificates.NewController,
-		NewResourceAdmissionController,
 		NewConfigValidationController,
+		NewValidationAdmissionController,
+		NewDefaultingAdmissionController,
 	)
 }

config/500-webhook-configuration.yaml

@@ -15,7 +15,7 @@
 apiVersion: admissionregistration.k8s.io/v1beta1
 kind: MutatingWebhookConfiguration
 metadata:
-  name: webhook.eventing.knative.dev
+  name: defaulting.webhook.eventing.knative.dev
   labels:
     eventing.knative.dev/release: devel
 webhooks:
@@ -26,7 +26,23 @@ webhooks:
       name: eventing-webhook
       namespace: knative-eventing
   failurePolicy: Fail
-  name: webhook.eventing.knative.dev
+  name: defaulting.webhook.eventing.knative.dev
+---
+apiVersion: admissionregistration.k8s.io/v1beta1
+kind: ValidatingWebhookConfiguration
+metadata:
+  name: validation.webhook.eventing.knative.dev
+  labels:
+    eventing.knative.dev/release: devel
+webhooks:
+- admissionReviewVersions:
+  - v1beta1
+  clientConfig:
+    service:
+      name: eventing-webhook
+      namespace: knative-eventing
+  failurePolicy: Fail
+  name: validation.webhook.eventing.knative.dev
 ---
 apiVersion: admissionregistration.k8s.io/v1beta1
 kind: ValidatingWebhookConfiguration

pkg/apis/eventing/v1alpha1/broker_types.go

@@ -54,7 +54,6 @@ var (
 	// Check that Broker can be validated, can be defaulted, and has immutable fields.
 	_ apis.Validatable = (*Broker)(nil)
 	_ apis.Defaultable = (*Broker)(nil)
-	_ apis.Immutable   = (*Broker)(nil)
 
 	// Check that Broker can return its spec untyped.
 	_ apis.HasSpec = (*Broker)(nil)

pkg/apis/eventing/v1alpha1/broker_validation.go

@@ -54,14 +54,10 @@ func isValidChannelTemplate(dct *eventingduckv1alpha1.ChannelTemplateSpec) *apis
 	return errs
 }
 
-func (b *Broker) CheckImmutableFields(ctx context.Context, og apis.Immutable) *apis.FieldError {
-	if og == nil {
+func (b *Broker) CheckImmutableFields(ctx context.Context, original *Broker) *apis.FieldError {
+	if original == nil {
 		return nil
 	}
-	original, ok := og.(*Broker)
-	if !ok {
-		return &apis.FieldError{Message: "The provided original was not a Broker"}
-	}
 
 	if diff, err := kmp.ShortDiff(original.Spec.ChannelTemplate, b.Spec.ChannelTemplate); err != nil {
 		return &apis.FieldError{

pkg/apis/eventing/v1alpha1/broker_validation_test.go

@@ -38,12 +38,6 @@ func TestBrokerSpecValidation(t *testing.T) {
 	_ = bs.Validate(context.Background())
 }
 
-type noBroker struct{}
-
-func (nb noBroker) CheckImmutableFields(_ context.Context, _ apis.Immutable) *apis.FieldError {
-	return nil
-}
-
 func TestBrokerImmutableFields(t *testing.T) {
 	original := &Broker{
 		Spec: BrokerSpec{
@@ -57,13 +51,9 @@ func TestBrokerImmutableFields(t *testing.T) {
 	}
 
 	tests := map[string]struct {
-		og      apis.Immutable
+		og      *Broker
 		wantErr *apis.FieldError
 	}{
-		"invalid original": {
-			og:      &noBroker{},
-			wantErr: &apis.FieldError{Message: "The provided original was not a Broker"},
-		},
 		"nil original": {
 			wantErr: nil,
 		},

pkg/apis/eventing/v1alpha1/eventtype_types.go

@@ -46,7 +46,6 @@ var (
 	// Check that EventType can be validated, can be defaulted, and has immutable fields.
 	_ apis.Validatable = (*EventType)(nil)
 	_ apis.Defaultable = (*EventType)(nil)
-	_ apis.Immutable   = (*EventType)(nil)
 
 	// Check that EventType can return its spec untyped.
 	_ apis.HasSpec = (*EventType)(nil)

pkg/apis/eventing/v1alpha1/eventtype_validation.go

@@ -48,16 +48,11 @@ func (ets *EventTypeSpec) Validate(ctx context.Context) *apis.FieldError {
 	return errs
 }
 
-func (et *EventType) CheckImmutableFields(ctx context.Context, og apis.Immutable) *apis.FieldError {
-	if og == nil {
+func (et *EventType) CheckImmutableFields(ctx context.Context, original *EventType) *apis.FieldError {
+	if original == nil {
 		return nil
 	}
 
-	original, ok := og.(*EventType)
-	if !ok {
-		return &apis.FieldError{Message: "The provided original was not an EventType"}
-	}
-
 	// All but Description field immutable.
 	ignoreArguments := cmpopts.IgnoreFields(EventTypeSpec{}, "Description")
 	if diff, err := kmp.ShortDiff(original.Spec, et.Spec, ignoreArguments); err != nil {

pkg/apis/eventing/v1alpha1/eventtype_validation_test.go

@@ -99,8 +99,8 @@ func TestEventTypeSpecValidation(t *testing.T) {
 func TestEventTypeImmutableFields(t *testing.T) {
 	tests := []struct {
 		name     string
-		current  apis.Immutable
-		original apis.Immutable
+		current  *EventType
+		original *EventType
 		want     *apis.FieldError
 	}{{
 		name: "good (no change)",
@@ -133,19 +133,6 @@ func TestEventTypeImmutableFields(t *testing.T) {
 		},
 		original: nil,
 		want:     nil,
-	}, {
-		name: "invalid type",
-		current: &EventType{
-			Spec: EventTypeSpec{
-				Type:   "test-type",
-				Source: "test-source",
-				Broker: "test-broker",
-			},
-		},
-		original: &Trigger{},
-		want: &apis.FieldError{
-			Message: "The provided original was not an EventType",
-		},
 	}, {
 		name: "bad (broker change)",
 		current: &EventType{

pkg/apis/eventing/v1alpha1/trigger_types.go

@@ -57,7 +57,6 @@ var (
 	// Check that Trigger can be validated, can be defaulted, and has immutable fields.
 	_ apis.Validatable = (*Trigger)(nil)
 	_ apis.Defaultable = (*Trigger)(nil)
-	_ apis.Immutable   = (*Trigger)(nil)
 
 	// Check that Trigger can return its spec untyped.
 	_ apis.HasSpec = (*Trigger)(nil)

pkg/apis/eventing/v1alpha1/trigger_validation.go

@@ -100,16 +100,11 @@ func (ts *TriggerSpec) Validate(ctx context.Context) *apis.FieldError {
 }
 
 // CheckImmutableFields checks that any immutable fields were not changed.
-func (t *Trigger) CheckImmutableFields(ctx context.Context, og apis.Immutable) *apis.FieldError {
-	if og == nil {
+func (t *Trigger) CheckImmutableFields(ctx context.Context, original *Trigger) *apis.FieldError {
+	if original == nil {
 		return nil
 	}
 
-	original, ok := og.(*Trigger)
-	if !ok {
-		return &apis.FieldError{Message: "The provided original was not a Trigger"}
-	}
-
 	if diff, err := kmp.ShortDiff(original.Spec.Broker, t.Spec.Broker); err != nil {
 		return &apis.FieldError{
 			Message: "Failed to diff Trigger",

pkg/apis/eventing/v1alpha1/trigger_validation_test.go

@@ -419,8 +419,8 @@ func TestTriggerSpecValidation(t *testing.T) {
 func TestTriggerImmutableFields(t *testing.T) {
 	tests := []struct {
 		name     string
-		current  apis.Immutable
-		original apis.Immutable
+		current  *Trigger
+		original *Trigger
 		want     *apis.FieldError
 	}{{
 		name: "good (no change)",
@@ -444,17 +444,6 @@ func TestTriggerImmutableFields(t *testing.T) {
 		},
 		original: nil,
 		want:     nil,
-	}, {
-		name: "invalid type",
-		current: &Trigger{
-			Spec: TriggerSpec{
-				Broker: "broker",
-			},
-		},
-		original: &Broker{},
-		want: &apis.FieldError{
-			Message: "The provided original was not a Trigger",
-		},
 	}, {
 		name: "good (filter change)",
 		current: &Trigger{

pkg/apis/messaging/v1alpha1/channel_validation.go

@@ -67,16 +67,11 @@ func isValidChannelTemplate(ct *eventingduck.ChannelTemplateSpec) *apis.FieldErr
 	return errs
 }
 
-func (c *Channel) CheckImmutableFields(ctx context.Context, og apis.Immutable) *apis.FieldError {
-	if og == nil {
+func (c *Channel) CheckImmutableFields(ctx context.Context, original *Channel) *apis.FieldError {
+	if original == nil {
 		return nil
 	}
 
-	original, ok := og.(*Channel)
-	if !ok {
-		return &apis.FieldError{Message: "The provided original was not a Channel"}
-	}
-
 	ignoreArguments := cmpopts.IgnoreFields(ChannelSpec{}, "Subscribable")
 	if diff, err := kmp.ShortDiff(original.Spec, c.Spec, ignoreArguments); err != nil {
 		return &apis.FieldError{

pkg/apis/messaging/v1alpha1/channel_validation_test.go

@@ -159,8 +159,8 @@ func TestChannelValidation(t *testing.T) {
 func TestChannelImmutableFields(t *testing.T) {
 	tests := []struct {
 		name     string
-		current  apis.Immutable
-		original apis.Immutable
+		current  *Channel
+		original *Channel
 		want     *apis.FieldError
 	}{{
 		name: "good (no change)",

pkg/apis/messaging/v1alpha1/parallel_types.go

@@ -55,9 +55,6 @@ var (
 	// Check that Parallel can return its spec untyped.
 	_ apis.HasSpec = (*Parallel)(nil)
 
-	// TODO: make appropriate fields immutable.
-	//_ apis.Immutable = (*Parallel)(nil)
-
 	_ runtime.Object = (*Parallel)(nil)
 
 	// Check that we can create OwnerReferences to a Parallel.