From 295122ce8be0e8370f4271a8112d05cc60e75563 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Sat, 4 Jul 2020 10:38:56 -0700 Subject: [PATCH 1/6] validate that brokerclass annotations exists --- pkg/apis/eventing/v1/broker_validation.go | 10 +++++- .../eventing/v1/broker_validation_test.go | 32 +++++++++++++++++-- 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_validation.go b/pkg/apis/eventing/v1/broker_validation.go index e7e0e8a3c46..af5392ba4c2 100644 --- a/pkg/apis/eventing/v1/broker_validation.go +++ b/pkg/apis/eventing/v1/broker_validation.go @@ -29,12 +29,20 @@ const ( func (b *Broker) Validate(ctx context.Context) *apis.FieldError { withNS := apis.AllowDifferentNamespace(apis.WithinParent(ctx, b.ObjectMeta)) - return b.Spec.Validate(withNS).ViaField("spec") + + var errs *apis.FieldError + if bc, ok := b.GetAnnotations()[BrokerClassAnnotationKey]; !ok || bc == "" { + errs = errs.Also(apis.ErrMissingField(BrokerClassAnnotationKey)) + } + + return errs.Also(b.Spec.Validate(withNS).ViaField("spec")) } func (bs *BrokerSpec) Validate(ctx context.Context) *apis.FieldError { var errs *apis.FieldError + // Make sure a BrokerClassAnnotation exists + // Validate the Config if bs.Config != nil { if ce := bs.Config.Validate(ctx); ce != nil { diff --git a/pkg/apis/eventing/v1/broker_validation_test.go b/pkg/apis/eventing/v1/broker_validation_test.go index 774d55fd27a..f25525433b2 100644 --- a/pkg/apis/eventing/v1/broker_validation_test.go +++ b/pkg/apis/eventing/v1/broker_validation_test.go @@ -80,11 +80,22 @@ func TestValidate(t *testing.T) { b Broker want *apis.FieldError }{{ - name: "valid empty", + name: "missing annotation", b: Broker{}, + want: apis.ErrMissingField("eventing.knative.dev/broker.class"), + }, { + name: "valid empty", + b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, + }, }, { name: "valid config", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Namespace: "namespace", @@ -97,6 +108,9 @@ func TestValidate(t *testing.T) { }, { name: "valid config, no namespace", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Name: "name", @@ -108,6 +122,9 @@ func TestValidate(t *testing.T) { }, { name: "invalid config, missing name", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Namespace: "namespace", @@ -120,6 +137,9 @@ func TestValidate(t *testing.T) { }, { name: "invalid config, missing apiVersion", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Namespace: "namespace", @@ -132,6 +152,9 @@ func TestValidate(t *testing.T) { }, { name: "invalid config, missing kind", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Namespace: "namespace", @@ -144,6 +167,9 @@ func TestValidate(t *testing.T) { }, { name: "invalid delivery, invalid delay string", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Delivery: &eventingduckv1.DeliverySpec{ BackoffDelay: &invalidString, @@ -151,12 +177,12 @@ func TestValidate(t *testing.T) { }, }, want: apis.ErrInvalidValue(invalidString, "spec.delivery.backoffDelay"), - }, {}} + }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { got := test.b.Validate(context.Background()) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("BrokerSpec.Validate (-want, +got) = %v", diff) + t.Errorf("Broker.Validate (-want, +got) = %v", diff) } }) } From cb4b65fdd75d28c9be10db1990a5d24b17adf1a8 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Sat, 4 Jul 2020 10:41:42 -0700 Subject: [PATCH 2/6] v1beta1 too --- .../eventing/v1beta1/broker_validation.go | 7 +++- .../v1beta1/broker_validation_test.go | 32 +++++++++++++++++-- 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/pkg/apis/eventing/v1beta1/broker_validation.go b/pkg/apis/eventing/v1beta1/broker_validation.go index a3cac84d95f..fadc10a2e74 100644 --- a/pkg/apis/eventing/v1beta1/broker_validation.go +++ b/pkg/apis/eventing/v1beta1/broker_validation.go @@ -29,7 +29,12 @@ const ( func (b *Broker) Validate(ctx context.Context) *apis.FieldError { withNS := apis.AllowDifferentNamespace(apis.WithinParent(ctx, b.ObjectMeta)) - return b.Spec.Validate(withNS).ViaField("spec") + var errs *apis.FieldError + if bc, ok := b.GetAnnotations()[BrokerClassAnnotationKey]; !ok || bc == "" { + errs = errs.Also(apis.ErrMissingField(BrokerClassAnnotationKey)) + } + + return errs.Also(b.Spec.Validate(withNS).ViaField("spec")) } func (bs *BrokerSpec) Validate(ctx context.Context) *apis.FieldError { diff --git a/pkg/apis/eventing/v1beta1/broker_validation_test.go b/pkg/apis/eventing/v1beta1/broker_validation_test.go index 4a73e3c89e9..55249db9757 100644 --- a/pkg/apis/eventing/v1beta1/broker_validation_test.go +++ b/pkg/apis/eventing/v1beta1/broker_validation_test.go @@ -80,11 +80,22 @@ func TestValidate(t *testing.T) { b Broker want *apis.FieldError }{{ - name: "valid empty", + name: "missing annotation", b: Broker{}, + want: apis.ErrMissingField("eventing.knative.dev/broker.class"), + }, { + name: "valid empty", + b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, + }, }, { name: "valid config", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Namespace: "namespace", @@ -97,6 +108,9 @@ func TestValidate(t *testing.T) { }, { name: "valid config, no namespace", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Name: "name", @@ -108,6 +122,9 @@ func TestValidate(t *testing.T) { }, { name: "invalid config, missing name", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Namespace: "namespace", @@ -120,6 +137,9 @@ func TestValidate(t *testing.T) { }, { name: "invalid config, missing apiVersion", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Namespace: "namespace", @@ -132,6 +152,9 @@ func TestValidate(t *testing.T) { }, { name: "invalid config, missing kind", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Config: &duckv1.KReference{ Namespace: "namespace", @@ -144,6 +167,9 @@ func TestValidate(t *testing.T) { }, { name: "invalid delivery, invalid delay string", b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, + }, Spec: BrokerSpec{ Delivery: &eventingduckv1beta1.DeliverySpec{ BackoffDelay: &invalidString, @@ -151,12 +177,12 @@ func TestValidate(t *testing.T) { }, }, want: apis.ErrInvalidValue(invalidString, "spec.delivery.backoffDelay"), - }, {}} + }} for _, test := range tests { t.Run(test.name, func(t *testing.T) { got := test.b.Validate(context.Background()) if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("BrokerSpec.Validate (-want, +got) = %v", diff) + t.Errorf("Broker.Validate (-want, +got) = %v", diff) } }) } From fc72641dcc0544601ac48c2210617376ac6a857f Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Sat, 4 Jul 2020 10:47:04 -0700 Subject: [PATCH 3/6] fix testcase names, no code changes --- pkg/apis/eventing/v1/broker_validation.go | 3 +-- pkg/apis/eventing/v1/broker_validation_test.go | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/pkg/apis/eventing/v1/broker_validation.go b/pkg/apis/eventing/v1/broker_validation.go index af5392ba4c2..404545216ad 100644 --- a/pkg/apis/eventing/v1/broker_validation.go +++ b/pkg/apis/eventing/v1/broker_validation.go @@ -30,6 +30,7 @@ const ( func (b *Broker) Validate(ctx context.Context) *apis.FieldError { withNS := apis.AllowDifferentNamespace(apis.WithinParent(ctx, b.ObjectMeta)) + // Make sure a BrokerClassAnnotation exists var errs *apis.FieldError if bc, ok := b.GetAnnotations()[BrokerClassAnnotationKey]; !ok || bc == "" { errs = errs.Also(apis.ErrMissingField(BrokerClassAnnotationKey)) @@ -41,8 +42,6 @@ func (b *Broker) Validate(ctx context.Context) *apis.FieldError { func (bs *BrokerSpec) Validate(ctx context.Context) *apis.FieldError { var errs *apis.FieldError - // Make sure a BrokerClassAnnotation exists - // Validate the Config if bs.Config != nil { if ce := bs.Config.Validate(ctx); ce != nil { diff --git a/pkg/apis/eventing/v1/broker_validation_test.go b/pkg/apis/eventing/v1/broker_validation_test.go index f25525433b2..f4834fb64e0 100644 --- a/pkg/apis/eventing/v1/broker_validation_test.go +++ b/pkg/apis/eventing/v1/broker_validation_test.go @@ -46,11 +46,11 @@ func TestBrokerImmutableFields(t *testing.T) { "nil original": { wantErr: nil, }, - "no ChannelTemplateSpec mutation": { + "no BrokerClassAnnotation mutation": { og: current, wantErr: nil, }, - "ChannelTemplateSpec mutated": { + "BrokerClassAnnotation mutated": { og: original, wantErr: &apis.FieldError{ Message: "Immutable fields changed (-old +new)", From 33c4acee6838dcce128d075ecc62df89a5ceba1e Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Sat, 4 Jul 2020 10:47:16 -0700 Subject: [PATCH 4/6] fix testcase names, no code changes --- pkg/apis/eventing/v1beta1/broker_validation.go | 2 ++ pkg/apis/eventing/v1beta1/broker_validation_test.go | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/apis/eventing/v1beta1/broker_validation.go b/pkg/apis/eventing/v1beta1/broker_validation.go index fadc10a2e74..7f408c48829 100644 --- a/pkg/apis/eventing/v1beta1/broker_validation.go +++ b/pkg/apis/eventing/v1beta1/broker_validation.go @@ -29,6 +29,8 @@ const ( func (b *Broker) Validate(ctx context.Context) *apis.FieldError { withNS := apis.AllowDifferentNamespace(apis.WithinParent(ctx, b.ObjectMeta)) + + // Make sure a BrokerClassAnnotation exists var errs *apis.FieldError if bc, ok := b.GetAnnotations()[BrokerClassAnnotationKey]; !ok || bc == "" { errs = errs.Also(apis.ErrMissingField(BrokerClassAnnotationKey)) diff --git a/pkg/apis/eventing/v1beta1/broker_validation_test.go b/pkg/apis/eventing/v1beta1/broker_validation_test.go index 55249db9757..73cea075a80 100644 --- a/pkg/apis/eventing/v1beta1/broker_validation_test.go +++ b/pkg/apis/eventing/v1beta1/broker_validation_test.go @@ -46,11 +46,11 @@ func TestBrokerImmutableFields(t *testing.T) { "nil original": { wantErr: nil, }, - "no ChannelTemplateSpec mutation": { + "no BrokerClassAnnotation mutation": { og: current, wantErr: nil, }, - "ChannelTemplateSpec mutated": { + "BrokerClassAnnotation mutated": { og: original, wantErr: &apis.FieldError{ Message: "Immutable fields changed (-old +new)", From 9e1e3c01ccf3e9c10737a34cfc01ef8c627554ef Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Sat, 4 Jul 2020 10:49:21 -0700 Subject: [PATCH 5/6] empty annotation test --- pkg/apis/eventing/v1/broker_validation_test.go | 8 ++++++++ pkg/apis/eventing/v1beta1/broker_validation_test.go | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/pkg/apis/eventing/v1/broker_validation_test.go b/pkg/apis/eventing/v1/broker_validation_test.go index f4834fb64e0..3f26a469b4f 100644 --- a/pkg/apis/eventing/v1/broker_validation_test.go +++ b/pkg/apis/eventing/v1/broker_validation_test.go @@ -83,6 +83,14 @@ func TestValidate(t *testing.T) { name: "missing annotation", b: Broker{}, want: apis.ErrMissingField("eventing.knative.dev/broker.class"), + }, { + name: "empty annotation", + b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": ""}, + }, + }, + want: apis.ErrMissingField("eventing.knative.dev/broker.class"), }, { name: "valid empty", b: Broker{ diff --git a/pkg/apis/eventing/v1beta1/broker_validation_test.go b/pkg/apis/eventing/v1beta1/broker_validation_test.go index 73cea075a80..4e0a473d578 100644 --- a/pkg/apis/eventing/v1beta1/broker_validation_test.go +++ b/pkg/apis/eventing/v1beta1/broker_validation_test.go @@ -83,6 +83,14 @@ func TestValidate(t *testing.T) { name: "missing annotation", b: Broker{}, want: apis.ErrMissingField("eventing.knative.dev/broker.class"), + }, { + name: "empty annotation", + b: Broker{ + ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": ""}, + }, + }, + want: apis.ErrMissingField("eventing.knative.dev/broker.class"), }, { name: "valid empty", b: Broker{ From 88d79f341eadee4ff4e724aa6da83c579606f136 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Sat, 4 Jul 2020 11:06:58 -0700 Subject: [PATCH 6/6] add required annotation for test resource --- pkg/reconciler/mtnamespace/resources/broker.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/reconciler/mtnamespace/resources/broker.go b/pkg/reconciler/mtnamespace/resources/broker.go index 2c0c5fedd88..3b6273ac798 100644 --- a/pkg/reconciler/mtnamespace/resources/broker.go +++ b/pkg/reconciler/mtnamespace/resources/broker.go @@ -31,6 +31,7 @@ const ( func MakeBroker(ns *corev1.Namespace) *v1beta1.Broker { return &v1beta1.Broker{ ObjectMeta: metav1.ObjectMeta{ + Annotations: map[string]string{"eventing.knative.dev/broker.class": "MTChannelBasedBroker"}, OwnerReferences: []metav1.OwnerReference{ *metav1.NewControllerRef(ns.GetObjectMeta(), schema.GroupVersionKind{ Group: corev1.SchemeGroupVersion.Group,