Split apart defaulting and validation webhook

Related: knative/pkg#848

Gopkg.toml

@@ -36,7 +36,10 @@ required = [
 # Our master branch tracks knative/pkg master or a release.
 [[override]]
   name = "knative.dev/pkg"
-  branch = "master"
+  #TODO(mattmoor): DO NOT SUBMIT
+  source = "github.com/mattmoor/pkg-1"
+  revision = "3f9bcd4d73fb89a1976eed9d7a5c6dd023435e3f"
+  # branch = "master"
 
 # TODO why is this overridden?
 [[override]]

cmd/webhook/main.go

@@ -34,9 +34,30 @@ import (
 	"knative.dev/pkg/webhook"
 	"knative.dev/pkg/webhook/certificates"
 	"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{},
+}
+
+func NewDefaultingAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
 	logger := logging.FromContext(ctx)
 
 	// Decorate contexts with the current state of the config.
@@ -56,33 +77,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{},
-		},
+		types,
 
 		// A function that infuses the context passed to Validate/SetDefaults with custom metadata.
 		ctxFunc,
@@ -92,6 +96,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 main() {
 	// Set up a signal context with our webhook options
 	ctx := webhook.WithOptions(signals.NewContext(), webhook.Options{
@@ -102,7 +129,8 @@ func main() {
 
 	sharedmain.MainWithContext(ctx, logconfig.WebhookName(),
 		certificates.NewController,
-		NewResourceAdmissionController,
+		NewDefaultingAdmissionController,
+		NewValidationAdmissionController,
 		// TODO(mattmoor): Support config validation in eventing.
 	)
 }

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: v1
 kind: Secret

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_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

@@ -438,8 +438,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)",
@@ -463,17 +463,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)",

vendor/knative.dev/pkg/RELEASING.md

@@ -88,3 +88,13 @@ their own release branches, so to update the `knative/pkg` dependency we run:
 dep ensure -update knative.dev/pkg
 ./hack/update-deps.sh
 ```
+
+## Revert to Master
+
+Post release, reverse the process. `Gopkg.toml` should look like:
+
+```toml
+[[override]]
+  name = "knative.dev/pkg"
+  branch = "master"
+```

vendor/knative.dev/pkg/apis/interfaces.go

@@ -47,11 +47,7 @@ type Convertible interface {
 // Immutable indicates that a particular type has fields that should
 // not change after creation.
 // DEPRECATED: Use WithinUpdate / GetBaseline from within Validatable instead.
-type Immutable interface {
-	// CheckImmutableFields checks that the current instance's immutable
-	// fields haven't changed from the provided original.
-	CheckImmutableFields(ctx context.Context, original Immutable) *FieldError
-}
+type Immutable interface{}
 
 // Listable indicates that a particular type can be returned via the returned
 // list type by the API server.