From 6e8a56c12e2d06e3d3c6eaece34cec952eecea56 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Mon, 4 May 2020 16:35:55 -0700 Subject: [PATCH 1/3] do not allow brokers to be created with channeltemplatespec --- .../eventing/v1alpha1/broker_validation.go | 20 +++++----- .../v1alpha1/broker_validation_test.go | 37 ++++++++++++++++++ test/e2e/broker_config_test.go | 2 +- .../broker_channel_flow_test_helper.go | 10 +++-- test/e2e/helpers/broker_dls_test_helper.go | 11 ++++-- ...broker_event_transformation_test_helper.go | 5 ++- test/e2e/helpers/broker_with_config_helper.go | 2 +- test/lib/resources/eventing.go | 38 +++++++++++++++---- 8 files changed, 97 insertions(+), 28 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/broker_validation.go b/pkg/apis/eventing/v1alpha1/broker_validation.go index 263c0fe4be3..ee191fff260 100644 --- a/pkg/apis/eventing/v1alpha1/broker_validation.go +++ b/pkg/apis/eventing/v1alpha1/broker_validation.go @@ -31,16 +31,16 @@ func (b *Broker) Validate(ctx context.Context) *apis.FieldError { func (bs *BrokerSpec) Validate(ctx context.Context) *apis.FieldError { var errs *apis.FieldError - // Validate the new channelTemplate. - // TODO: As part of https://github.com/knative/eventing/issues/2128 - // Also make sure this gets rejected. It would break our tests - // and assumptions to do this right now. - // if bs.ChannelTemplate == nil { - // errs = errs.Also(apis.ErrMissingField("channelTemplateSpec")) - // } else - if bs.ChannelTemplate != nil { - if cte := messagingv1beta1.IsValidChannelTemplate(bs.ChannelTemplate); cte != nil { - errs = errs.Also(cte.ViaField("channelTemplateSpec")) + // As of v0.15 do not allow creation of new Brokers with the channel template. + if apis.IsInCreate(ctx) { + if bs.ChannelTemplate != nil { + errs = errs.Also(apis.ErrDisallowedFields("channelTemplate")) + } + } else { + if bs.ChannelTemplate != nil { + if cte := messagingv1beta1.IsValidChannelTemplate(bs.ChannelTemplate); cte != nil { + errs = errs.Also(cte.ViaField("channelTemplateSpec")) + } } } diff --git a/pkg/apis/eventing/v1alpha1/broker_validation_test.go b/pkg/apis/eventing/v1alpha1/broker_validation_test.go index 3b0ce202896..e4e37070d6a 100644 --- a/pkg/apis/eventing/v1alpha1/broker_validation_test.go +++ b/pkg/apis/eventing/v1alpha1/broker_validation_test.go @@ -72,6 +72,43 @@ func TestBrokerImmutableFields(t *testing.T) { } } +func TestBrokerValidationFailsWithCreateChannelTemplate(t *testing.T) { + b := &Broker{ + Spec: BrokerSpec{ + ChannelTemplate: &messagingv1beta1.ChannelTemplateSpec{ + TypeMeta: metav1.TypeMeta{APIVersion: "myapiversion", Kind: "mykind"}, + }, + }, + } + tests := map[string]struct { + ctx context.Context + wantErr *apis.FieldError + }{ + "create fails": { + ctx: apis.WithinCreate(context.Background()), + wantErr: apis.ErrDisallowedFields("channelTemplate").ViaField("spec"), + }, + "delete works": { + ctx: apis.WithinDelete(context.Background()), + wantErr: nil, + }, + "update works": { + ctx: apis.WithinUpdate(context.Background(), b), + wantErr: nil, + }, + "no context, works": { + ctx: context.Background(), + wantErr: nil, + }, + } + for name, tc := range tests { + gotErr := b.Validate(tc.ctx) + if diff := cmp.Diff(tc.wantErr.Error(), gotErr.Error()); diff != "" { + t.Errorf("%s Broker.Validate (-want, +got) = %v", name, diff) + } + } +} + func TestBrokerValidation(t *testing.T) { /* This test should fail: TODO: https://github.com/knative/eventing/issues/2128 name: "invalid empty, missing channeltemplatespec", diff --git a/test/e2e/broker_config_test.go b/test/e2e/broker_config_test.go index bc721279247..9ce7aed6a14 100644 --- a/test/e2e/broker_config_test.go +++ b/test/e2e/broker_config_test.go @@ -23,7 +23,7 @@ import ( "knative.dev/eventing/test/e2e/helpers" ) -// TestBrokerDeadLetterSink tests Broker's DeadLetterSink +// TestBrokerWithConfig tests Broker using Config instead of channel templates. func TestBrokerWithConfig(t *testing.T) { helpers.TestBrokerWithConfig(t, brokerClass, channelTestRunner) } diff --git a/test/e2e/helpers/broker_channel_flow_test_helper.go b/test/e2e/helpers/broker_channel_flow_test_helper.go index 8dd116bf046..c3b3ca600fe 100644 --- a/test/e2e/helpers/broker_channel_flow_test_helper.go +++ b/test/e2e/helpers/broker_channel_flow_test_helper.go @@ -62,15 +62,17 @@ func BrokerChannelFlowTestHelper(t *testing.T, defer lib.TearDown(client) if brokerClass == eventing.ChannelBrokerClassValue { - // create required RBAC resources including ServiceAccounts and ClusterRoleBindings for Brokers // create required RBAC resources including ServiceAccounts and ClusterRoleBindings for Brokers client.CreateRBACResourcesForBrokers() } + // Create a configmap used by the broker. + client.CreateBrokerConfigMapOrFail(brokerName, &channel) + // create a new broker - client.CreateBrokerOrFail(brokerName, - resources.WithBrokerClassForBroker(brokerClass), - resources.WithChannelTemplateForBroker(&channel)) + client.CreateBrokerV1Beta1OrFail(brokerName, + resources.WithBrokerClassForBrokerV1Beta1(brokerClass), + resources.WithConfigMapForBrokerConfig()) client.WaitForResourceReadyOrFail(brokerName, lib.BrokerTypeMeta) // create the event we want to transform to diff --git a/test/e2e/helpers/broker_dls_test_helper.go b/test/e2e/helpers/broker_dls_test_helper.go index f575c79e931..1419c409ee8 100644 --- a/test/e2e/helpers/broker_dls_test_helper.go +++ b/test/e2e/helpers/broker_dls_test_helper.go @@ -61,11 +61,14 @@ func BrokerDeadLetterSinkTestHelper(t *testing.T, delivery := resources.Delivery(resources.WithDeadLetterSinkForDelivery(loggerPodName)) + // Create a configmap used by the broker. + client.CreateBrokerConfigMapOrFail(brokerName, &channel) + // create a new broker - client.CreateBrokerOrFail(brokerName, - resources.WithBrokerClassForBroker(brokerClass), - resources.WithChannelTemplateForBroker(&channel), - resources.WithDeliveryForBroker(delivery)) + client.CreateBrokerV1Beta1OrFail(brokerName, + resources.WithBrokerClassForBrokerV1Beta1(brokerClass), + resources.WithConfigMapForBrokerConfig(), + resources.WithDeliveryForBrokerV1Beta1(delivery)) client.WaitForResourceReadyOrFail(brokerName, lib.BrokerTypeMeta) diff --git a/test/e2e/helpers/broker_event_transformation_test_helper.go b/test/e2e/helpers/broker_event_transformation_test_helper.go index dc0724c8bc2..555c5195b7f 100644 --- a/test/e2e/helpers/broker_event_transformation_test_helper.go +++ b/test/e2e/helpers/broker_event_transformation_test_helper.go @@ -61,8 +61,11 @@ func EventTransformationForTriggerTestHelper(t *testing.T, client.CreateRBACResourcesForBrokers() } + // Create a configmap used by the broker. + config := client.CreateBrokerConfigMapOrFail(brokerName, &channel) + // create a new broker - client.CreateBrokerOrFail(brokerName, resources.WithBrokerClassForBroker(brokerClass), resources.WithChannelTemplateForBroker(&channel)) + client.CreateBrokerV1Beta1OrFail(brokerName, resources.WithBrokerClassForBrokerV1Beta1(brokerClass), resources.WithConfigForBrokerV1Beta1(config)) client.WaitForResourceReadyOrFail(brokerName, lib.BrokerTypeMeta) // create the event we want to transform to diff --git a/test/e2e/helpers/broker_with_config_helper.go b/test/e2e/helpers/broker_with_config_helper.go index 725418666a3..56f56edc8d3 100644 --- a/test/e2e/helpers/broker_with_config_helper.go +++ b/test/e2e/helpers/broker_with_config_helper.go @@ -70,7 +70,7 @@ func TestBrokerWithConfig(t *testing.T, //&channel // create a new broker - client.CreateBrokerV1Beta1OrFail(brokerName, resources.WithBrokerClassForBrokerV1Beta1(brokerClass), resources.WithChannelTemplateForBrokerV1Beta1(config)) + client.CreateBrokerV1Beta1OrFail(brokerName, resources.WithBrokerClassForBrokerV1Beta1(brokerClass), resources.WithConfigForBrokerV1Beta1(config)) client.WaitForResourceReadyOrFail(brokerName, lib.BrokerTypeMeta) // create the event we want to transform to diff --git a/test/lib/resources/eventing.go b/test/lib/resources/eventing.go index 18b75cfa2f6..43ad4494a92 100644 --- a/test/lib/resources/eventing.go +++ b/test/lib/resources/eventing.go @@ -187,6 +187,29 @@ func WithChannelTemplateForBroker(channelTypeMeta *metav1.TypeMeta) BrokerOption } } +// WithConfigMapForBrokerConfig returns a function that configures the ConfigMap +// for the Spec.Config for a given Broker. Note that the CM must exist and has +// to be in the same namespace as the Broker and have the same Name. Typically +// you'd do this by calling client.CreateBrokerConfigMapOrFail and then call this +// method. +// If those don't apply to your ConfigMap, look at WithConfigForBrokerV1Beta1 +func WithConfigMapForBrokerConfig() BrokerV1Beta1Option { + return func(b *eventingv1beta1.Broker) { + b.Spec.Config = &duckv1.KReference{ + Name: b.Name, + Namespace: b.Namespace, + Kind: "ConfigMap", + APIVersion: "v1", + } + } +} + +func WithConfigForBrokerV1Beta1(config *duckv1.KReference) BrokerV1Beta1Option { + return func(b *eventingv1beta1.Broker) { + b.Spec.Config = config + } +} + // WithBrokerClassForBrokerV1Beta1 returns a function that adds a brokerClass // annotation to the given Broker. func WithBrokerClassForBrokerV1Beta1(brokerClass string) BrokerV1Beta1Option { @@ -200,13 +223,6 @@ func WithBrokerClassForBrokerV1Beta1(brokerClass string) BrokerV1Beta1Option { } } -// WithChannelTemplateForBrokerV1Beta1 returns a function that adds a Config to the given Broker. -func WithChannelTemplateForBrokerV1Beta1(config *duckv1.KReference) BrokerV1Beta1Option { - return func(b *eventingv1beta1.Broker) { - b.Spec.Config = config - } -} - // WithDeliveryForBroker returns a function that adds a Delivery for the given Broker. func WithDeliveryForBroker(delivery *eventingduckv1beta1.DeliverySpec) BrokerOption { return func(b *eventingv1alpha1.Broker) { @@ -214,6 +230,14 @@ func WithDeliveryForBroker(delivery *eventingduckv1beta1.DeliverySpec) BrokerOpt } } +// WithDeliveryForBrokerV1Beta1 returns a function that adds a Delivery for the given +// v1beta1 Broker. +func WithDeliveryForBrokerV1Beta1(delivery *eventingduckv1beta1.DeliverySpec) BrokerV1Beta1Option { + return func(b *eventingv1beta1.Broker) { + b.Spec.Delivery = delivery + } +} + // WithBrokerClassForBroker returns a function that adds a brokerClass // annotation to the given Broker. func WithBrokerClassForBroker(brokerClass string) BrokerOption { From a960259ee657c21cd95f3b61dc983dd9651a504a Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Tue, 5 May 2020 08:32:29 -0700 Subject: [PATCH 2/3] update broker conformance tests too, doh --- test/conformance/helpers/broker_tracing_test_helper.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/conformance/helpers/broker_tracing_test_helper.go b/test/conformance/helpers/broker_tracing_test_helper.go index c6c2c9373e1..5554a91f9cb 100644 --- a/test/conformance/helpers/broker_tracing_test_helper.go +++ b/test/conformance/helpers/broker_tracing_test_helper.go @@ -88,10 +88,13 @@ func setupBrokerTracing(brokerClass string) SetupInfrastructureFunc { client.CreateConfigMapPropagationOrFail(defaultCMPName) client.CreateRBACResourcesForBrokers() } - broker := client.CreateBrokerOrFail( + // Create a configmap used by the broker. + client.CreateBrokerConfigMapOrFail("default", channel) + + broker := client.CreateBrokerV1Beta1OrFail( "br", - resources.WithBrokerClassForBroker(brokerClass), - resources.WithChannelTemplateForBroker(channel), + resources.WithBrokerClassForBrokerV1Beta1(brokerClass), + resources.WithConfigMapForBrokerConfig(), ) // Create a logger (EventRecord) Pod and a K8s Service that points to it. From fd7f897906125a7501a77f94006bfb1046477f57 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Tue, 5 May 2020 08:57:07 -0700 Subject: [PATCH 3/3] not default broker, it is br --- test/conformance/helpers/broker_tracing_test_helper.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/conformance/helpers/broker_tracing_test_helper.go b/test/conformance/helpers/broker_tracing_test_helper.go index 5554a91f9cb..e161cccc669 100644 --- a/test/conformance/helpers/broker_tracing_test_helper.go +++ b/test/conformance/helpers/broker_tracing_test_helper.go @@ -89,7 +89,7 @@ func setupBrokerTracing(brokerClass string) SetupInfrastructureFunc { client.CreateRBACResourcesForBrokers() } // Create a configmap used by the broker. - client.CreateBrokerConfigMapOrFail("default", channel) + client.CreateBrokerConfigMapOrFail("br", channel) broker := client.CreateBrokerV1Beta1OrFail( "br",