actually make trigger.spec.broker immutable (#3517)

* actually make trigger.spec.broker immutable * change the error message/annotation based on rebase
knative · Jul 7, 2020 · 2d1047d · 2d1047d
1 parent e88f4da
commit 2d1047d
Show file tree

Hide file tree

Showing 2 changed files with 47 additions and 10 deletions.
diff --git a/pkg/apis/eventing/v1/trigger_validation.go b/pkg/apis/eventing/v1/trigger_validation.go
@@ -39,6 +39,10 @@ func (t *Trigger) Validate(ctx context.Context) *apis.FieldError {
 	errs = t.validateAnnotation(errs, DependencyAnnotation, t.validateDependencyAnnotation)
 	errs = t.validateAnnotation(errs, DeprecatedInjectionAnnotation, t.validateInjectionAnnotation)
 	errs = t.validateAnnotation(errs, InjectionAnnotation, t.validateInjectionAnnotation)
+	if apis.IsInUpdate(ctx) {
+		original := apis.GetBaseline(ctx).(*Trigger)
+		errs = errs.Also(t.CheckImmutableFields(ctx, original))
+	}
 	return errs
 }
 

diff --git a/pkg/apis/eventing/v1/trigger_validation_test.go b/pkg/apis/eventing/v1/trigger_validation_test.go
@@ -233,7 +233,31 @@ func TestTriggerValidation(t *testing.T) {
 				Paths:   []string{injectionAnnotationPath},
 				Message: "The provided injection annotation value can only be \"enabled\" or \"disabled\", not \"wut\"",
 			},
-		}, {
+		},
+		{
+			name: "invalid trigger spec, invalid dependency annotation(missing kind, name, apiVersion) and invalid injection",
+			t: &Trigger{
+				ObjectMeta: v1.ObjectMeta{
+					Namespace: "test-ns",
+					Annotations: map[string]string{
+						DependencyAnnotation: "{}",
+						InjectionAnnotation:  invalidInjectionAnnotation,
+					}},
+				Spec: TriggerSpec{Broker: "default", Subscriber: validSubscriber}},
+			want: func() *apis.FieldError {
+				var errs *apis.FieldError
+				errs = errs.Also(&apis.FieldError{
+					Message: fmt.Sprintf(`The provided injection annotation value can only be "enabled" or "disabled", not "wut"`),
+					Paths:   []string{"metadata.annotations[eventing.knative.dev/injection]"},
+				})
+
+				errs = errs.Also(apis.ErrMissingField("metadata.annotations[knative.dev/dependency].apiVersion"))
+				errs = errs.Also(apis.ErrMissingField("metadata.annotations[knative.dev/dependency].kind"))
+				errs = errs.Also(apis.ErrMissingField("metadata.annotations[knative.dev/dependency].name"))
+				return errs
+			}(),
+		},
+		{
 			name: "valid injection annotation value, non-default broker specified",
 			t: &Trigger{
 				ObjectMeta: v1.ObjectMeta{
@@ -400,20 +424,23 @@ func TestTriggerImmutableFields(t *testing.T) {
 		name: "good (no change)",
 		current: &Trigger{
 			Spec: TriggerSpec{
-				Broker: "broker",
+				Broker:     "broker",
+				Subscriber: validSubscriber,
 			},
 		},
 		original: &Trigger{
 			Spec: TriggerSpec{
-				Broker: "broker",
+				Broker:     "broker",
+				Subscriber: validSubscriber,
 			},
 		},
 		want: nil,
 	}, {
 		name: "new nil is ok",
 		current: &Trigger{
 			Spec: TriggerSpec{
-				Broker: "broker",
+				Broker:     "broker",
+				Subscriber: validSubscriber,
 			},
 		},
 		original: nil,
@@ -422,26 +449,30 @@ func TestTriggerImmutableFields(t *testing.T) {
 		name: "good (filter change)",
 		current: &Trigger{
 			Spec: TriggerSpec{
-				Broker: "broker",
+				Broker:     "broker",
+				Subscriber: validSubscriber,
 			},
 		},
 		original: &Trigger{
 			Spec: TriggerSpec{
-				Broker: "broker",
-				Filter: validAttributesFilter,
+				Broker:     "broker",
+				Filter:     validAttributesFilter,
+				Subscriber: validSubscriber,
 			},
 		},
 		want: nil,
 	}, {
 		name: "bad (broker change)",
 		current: &Trigger{
 			Spec: TriggerSpec{
-				Broker: "broker",
+				Broker:     "broker",
+				Subscriber: validSubscriber,
 			},
 		},
 		original: &Trigger{
 			Spec: TriggerSpec{
-				Broker: "original_broker",
+				Broker:     "original_broker",
+				Subscriber: validSubscriber,
 			},
 		},
 		want: &apis.FieldError{
@@ -456,7 +487,9 @@ func TestTriggerImmutableFields(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			got := test.current.CheckImmutableFields(context.TODO(), test.original)
+			ctx := context.Background()
+			ctx = apis.WithinUpdate(ctx, test.original)
+			got := test.current.Validate(ctx)
 			if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" {
 				t.Errorf("CheckImmutableFields (-want, +got) = %v", diff)
 			}