Validate trigger filters are immutable

cmd/webhook/broker/main.go

@@ -35,18 +35,19 @@ import (
 )
 
 var ourTypes = map[schema.GroupVersionKind]resourcesemantics.GenericCRD{
-	v1.SchemeGroupVersion.WithKind("Broker"): &rabbitv1.RabbitBroker{},
-}
-
-var callbacks = map[schema.GroupVersionKind]validation.Callback{
-	v1.SchemeGroupVersion.WithKind("Broker"): validation.NewCallback(rabbitv1.ValidateFunc, webhook.Create, webhook.Update),
+	v1.SchemeGroupVersion.WithKind("Broker"):  &rabbitv1.RabbitBroker{},
+	v1.SchemeGroupVersion.WithKind("Trigger"): &v1.Trigger{},
 }
 
 func NewValidationAdmissionController(ctx context.Context, cmw configmap.Watcher) *controller.Impl {
 	// A function that infuses the context passed to ConvertTo/ConvertFrom/SetDefaults with custom metadata.
 	ctxFunc := func(ctx context.Context) context.Context {
 		return ctx
 	}
+	callbacks := map[schema.GroupVersionKind]validation.Callback{
+		v1.SchemeGroupVersion.WithKind("Broker"):  validation.NewCallback(rabbitv1.ValidateBroker, webhook.Create, webhook.Update),
+		v1.SchemeGroupVersion.WithKind("Trigger"): validation.NewCallback(rabbitv1.ValidateTrigger(ctx), webhook.Create, webhook.Update),
+	}
 	return validation.NewAdmissionController(ctx,
 
 		// Name of the resource webhook.

config/broker/200-webhook-clusterrole.yaml

@@ -79,3 +79,13 @@ rules:
     resources:
       - "leases"
     verbs: *everything
+
+    # For validating only our triggers need to lookup the broker
+  - apiGroups:
+      - "eventing.knative.dev"
+    resources:
+      - "brokers"
+    verbs:
+      - "get"
+      - "list"
+      - "watch"

pkg/apis/eventing/v1/broker_validation.go

@@ -110,15 +110,18 @@ func (b *RabbitBroker) Validate(ctx context.Context) *apis.FieldError {
 	return errs
 }
 
-func ValidateFunc(ctx context.Context, unstructured *unstructured.Unstructured) error {
+func ValidateBroker(ctx context.Context, unstructured *unstructured.Unstructured) error {
+	return validate(ctx, unstructured, &RabbitBroker{})
+}
+
+func validate(ctx context.Context, unstructured *unstructured.Unstructured, t apis.Validatable) error {
 	if unstructured == nil {
 		return nil
 	}
-	var b RabbitBroker
-	if err := duck.FromUnstructured(unstructured, &b); err != nil {
+	if err := duck.FromUnstructured(unstructured, t); err != nil {
 		return err
 	}
-	err := b.Validate(ctx)
+	err := t.Validate(ctx)
 	if err == nil {
 		return nil
 	}

pkg/apis/eventing/v1/broker_validation_test.go

@@ -317,7 +317,7 @@ func TestValidateFunc(t *testing.T) {
 			if test.original != nil {
 				ctx = apis.WithinUpdate(ctx, test.original)
 			}
-			got := ValidateFunc(ctx, test.b)
+			got := ValidateBroker(ctx, test.b)
 			if test.want.Error() != "" && got == nil {
 				t.Errorf("Broker.Validate want: %q got nil", test.want.Error())
 			} else if test.want.Error() == "" && got != nil {

pkg/apis/eventing/v1/trigger_validation.go

@@ -0,0 +1,78 @@
+/*
+Copyright 2021 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1
+
+import (
+	"context"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1"
+	"knative.dev/eventing/pkg/client/clientset/versioned"
+	"knative.dev/eventing/pkg/client/injection/client"
+	"knative.dev/pkg/apis"
+	"knative.dev/pkg/kmp"
+)
+
+// TODO: This is technically variable, see https://github.com/knative-sandbox/eventing-rabbitmq/issues/466
+const BrokerClass = "RabbitMQBroker"
+
+func ValidateTrigger(ctx context.Context) func(context.Context, *unstructured.Unstructured) error {
+	c := client.Get(ctx)
+
+	return func(ctx context.Context, unstructured *unstructured.Unstructured) error {
+		return validate(ctx, unstructured, &RabbitTrigger{Client: c})
+	}
+}
+
+type RabbitTrigger struct {
+	eventingv1.Trigger
+	Client versioned.Interface
+}
+
+var (
+	_ apis.Validatable = (*RabbitTrigger)(nil)
+)
+
+func (t *RabbitTrigger) Validate(ctx context.Context) *apis.FieldError {
+	broker, err := t.Client.EventingV1().Brokers(t.Namespace).Get(ctx, t.Spec.Broker, metav1.GetOptions{})
+	if err != nil {
+		return nil
+	}
+	bc, ok := broker.GetAnnotations()[eventingv1.BrokerClassAnnotationKey]
+	if !ok || bc != BrokerClass {
+		// Not my broker
+		return nil
+	}
+	if apis.IsInUpdate(ctx) {
+		original := apis.GetBaseline(ctx).(*eventingv1.Trigger)
+		if diff, err := kmp.ShortDiff(original.Spec.Filter, t.Spec.Filter); err != nil {
+			return &apis.FieldError{
+				Message: "Failed to diff Trigger",
+				Paths:   []string{"spec", "filter"},
+				Details: err.Error(),
+			}
+		} else if diff != "" {
+			return &apis.FieldError{
+				Message: "Immutable fields changed (-old +new)",
+				Paths:   []string{"spec", "filter"},
+				Details: diff,
+			}
+		}
+	}
+	return nil
+}

pkg/apis/eventing/v1/trigger_validation_test.go

@@ -0,0 +1,131 @@
+/*
+Copyright 2021 The Knative Authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package v1_test
+
+import (
+	"context"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+	"github.com/google/go-cmp/cmp/cmpopts"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	v1 "knative.dev/eventing-rabbitmq/pkg/apis/eventing/v1"
+	eventingv1 "knative.dev/eventing/pkg/apis/eventing/v1"
+	"knative.dev/eventing/pkg/client/clientset/versioned/fake"
+	"knative.dev/pkg/apis"
+)
+
+func TestTriggerValidate(t *testing.T) {
+	tests := []struct {
+		name string
+		trigger,
+		original *v1.RabbitTrigger
+		err *apis.FieldError
+	}{
+		{
+			name:    "broker not found gets ignored",
+			trigger: trigger(withBroker("foo")),
+		},
+		{
+			name: "different broker class gets ignored",
+			trigger: trigger(
+				withBroker("foo"),
+				withClientObjects(&eventingv1.Broker{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:        "foo",
+						Annotations: map[string]string{eventingv1.BrokerClassAnnotationKey: "some-other-broker"},
+					},
+				})),
+		},
+		{
+			name:     "filters are immutable",
+			trigger:  trigger(withBroker("foo"), brokerExistsAndIsValid),
+			original: trigger(withBroker("foo"), brokerExistsAndIsValid, withFilters(filter("x", "y"))),
+			err: &apis.FieldError{
+				Message: "Immutable fields changed (-old +new)",
+				Paths:   []string{"spec", "filter"},
+				Details: "{*v1.TriggerFilter}:\n\t-: \"&{Attributes:map[x:y]}\"\n\t+: \"<nil>\"\n",
+			},
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			ctx := context.Background()
+			if tc.original != nil {
+				ctx = apis.WithinUpdate(ctx, &tc.original.Trigger)
+			}
+			err := tc.trigger.Validate(ctx)
+			if diff := cmp.Diff(tc.err, err, cmpopts.IgnoreUnexported(apis.FieldError{})); diff != "" {
+				t.Error("Trigger.Validate (-want, +got) =", diff)
+			}
+		})
+	}
+}
+
+type triggerOpt func(*v1.RabbitTrigger)
+
+func trigger(opts ...triggerOpt) *v1.RabbitTrigger {
+	t := &v1.RabbitTrigger{
+		Trigger: eventingv1.Trigger{
+			Spec: eventingv1.TriggerSpec{},
+		},
+		Client: fake.NewSimpleClientset(),
+	}
+	for _, o := range opts {
+		o(t)
+	}
+	return t
+}
+func withClientObjects(objects ...runtime.Object) triggerOpt {
+	return func(t *v1.RabbitTrigger) {
+		t.Client = fake.NewSimpleClientset(objects...)
+	}
+}
+
+func withBroker(name string) triggerOpt {
+	return func(t *v1.RabbitTrigger) {
+		t.Spec.Broker = name
+	}
+}
+
+func brokerExistsAndIsValid(t *v1.RabbitTrigger) {
+	withClientObjects(&eventingv1.Broker{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:        t.Spec.Broker,
+			Annotations: map[string]string{eventingv1.BrokerClassAnnotationKey: v1.BrokerClass},
+		},
+	})(t)
+}
+
+func filter(k, v string) []string {
+	return []string{k, v}
+}
+
+func withFilters(filters ...[]string) triggerOpt {
+	return func(t *v1.RabbitTrigger) {
+		if t.Spec.Filter == nil {
+			t.Spec.Filter = &eventingv1.TriggerFilter{}
+		}
+		if t.Spec.Filter.Attributes == nil {
+			t.Spec.Filter.Attributes = map[string]string{}
+		}
+		for _, filter := range filters {
+			t.Spec.Filter.Attributes[filter[0]] = filter[1]
+		}
+	}
+}