From 848e37d61d1121a2d0bf1fb3a38a28ab9f39f8f6 Mon Sep 17 00:00:00 2001 From: grac3gao Date: Thu, 10 Oct 2019 14:53:15 -0700 Subject: [PATCH 1/9] Enable eventing when a Trigger is created for first time --- pkg/reconciler/trigger/trigger.go | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index e1a020970cc..6d690e62879 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -156,7 +156,7 @@ func (r *Reconciler) reconcile(ctx context.Context, t *v1alpha1.Trigger) error { return err } - b, err := r.brokerLister.Brokers(t.Namespace).Get(t.Spec.Broker) + b, err := r.getBroker(ctx, t) if err != nil { logging.FromContext(ctx).Error("Unable to get the Broker", zap.Error(err)) if apierrs.IsNotFound(err) { @@ -303,6 +303,27 @@ func (r *Reconciler) updateStatus(ctx context.Context, desired *v1alpha1.Trigger return trig, err } +// getBroker returns trigger's coupled broker. If the trigger is coupled with default broker, and default broker is not existed, +// it will create the default broker first by labeling namespace with kantive-eventing-injection=enabled +func (r *Reconciler) getBroker(ctx context.Context, t *v1alpha1.Trigger) (*v1alpha1.Broker, error) { + b, err := r.brokerLister.Brokers(t.Namespace).Get(t.Spec.Broker) + if t.Spec.Broker == "default" && apierrs.IsNotFound(err) { + current, _ := r.KubeClientSet.CoreV1().Namespaces().Get(t.Namespace, metav1.GetOptions{}) + if current.Labels == nil { + current.Labels = map[string]string{} + } + current.Labels["knative-eventing-injection"] = "enabled" + _, e := r.KubeClientSet.CoreV1().Namespaces().Update(current) + if e != nil { + return b, err + } + // Wait for default broker to set up + time.Sleep(10 * time.Second) + b, err = r.brokerLister.Brokers(t.Namespace).Get(t.Spec.Broker) + } + return b, err +} + // getBrokerFilterService returns the K8s service for trigger 't' if exists, // otherwise it returns an error. func (r *Reconciler) getBrokerFilterService(ctx context.Context, b *v1alpha1.Broker) (*corev1.Service, error) { From 6e4fef5086c892bb2a164dea4a6f31470180144c Mon Sep 17 00:00:00 2001 From: grac3gao Date: Fri, 11 Oct 2019 13:02:28 -0700 Subject: [PATCH 2/9] Enable eventing when a Trigger is created for first time --- pkg/reconciler/trigger/trigger.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index 6d690e62879..0172b7ee640 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -303,23 +303,25 @@ func (r *Reconciler) updateStatus(ctx context.Context, desired *v1alpha1.Trigger return trig, err } -// getBroker returns trigger's coupled broker. If the trigger is coupled with default broker, and default broker is not existed, -// it will create the default broker first by labeling namespace with kantive-eventing-injection=enabled +// getBroker returns trigger's coupled broker. If the trigger is coupled with default broker, and default broker does not exist, +// it will create the default broker first by labeling namespace with knative-eventing-injection=enabled func (r *Reconciler) getBroker(ctx context.Context, t *v1alpha1.Trigger) (*v1alpha1.Broker, error) { b, err := r.brokerLister.Brokers(t.Namespace).Get(t.Spec.Broker) if t.Spec.Broker == "default" && apierrs.IsNotFound(err) { - current, _ := r.KubeClientSet.CoreV1().Namespaces().Get(t.Namespace, metav1.GetOptions{}) + current, e := r.KubeClientSet.CoreV1().Namespaces().Get(t.Namespace, metav1.GetOptions{}) + if e != nil { + logging.FromContext(ctx).Error("Unable to get namespace resource to enable knative-eventing-injection", zap.Error(e)) + return b, err + } if current.Labels == nil { current.Labels = map[string]string{} } current.Labels["knative-eventing-injection"] = "enabled" - _, e := r.KubeClientSet.CoreV1().Namespaces().Update(current) + _, e = r.KubeClientSet.CoreV1().Namespaces().Update(current) if e != nil { + logging.FromContext(ctx).Error("Unable to label the namespace resource with knative-eventing-injection", zap.Error(e)) return b, err } - // Wait for default broker to set up - time.Sleep(10 * time.Second) - b, err = r.brokerLister.Brokers(t.Namespace).Get(t.Spec.Broker) } return b, err } From 8420602825ae0d375233b4f9a39f7ba081f960c0 Mon Sep 17 00:00:00 2001 From: grac3gao Date: Tue, 15 Oct 2019 11:35:24 -0700 Subject: [PATCH 3/9] Code change --- pkg/reconciler/trigger/controller.go | 3 ++ pkg/reconciler/trigger/controller_test.go | 4 +-- pkg/reconciler/trigger/trigger.go | 44 ++++++++++++----------- pkg/reconciler/trigger/trigger_test.go | 1 + 4 files changed, 29 insertions(+), 23 deletions(-) diff --git a/pkg/reconciler/trigger/controller.go b/pkg/reconciler/trigger/controller.go index 12e804113d3..fa577f152f9 100644 --- a/pkg/reconciler/trigger/controller.go +++ b/pkg/reconciler/trigger/controller.go @@ -34,6 +34,7 @@ import ( "knative.dev/eventing/pkg/client/injection/informers/eventing/v1alpha1/trigger" "knative.dev/eventing/pkg/client/injection/informers/messaging/v1alpha1/subscription" duckv1alpha1 "knative.dev/pkg/apis/duck/v1alpha1" + "knative.dev/pkg/client/injection/kube/informers/core/v1/namespace" "knative.dev/pkg/injection/clients/dynamicclient" ) @@ -57,6 +58,7 @@ func NewController( subscriptionInformer := subscription.Get(ctx) brokerInformer := broker.Get(ctx) serviceInformer := service.Get(ctx) + namespaceInformer := namespace.Get(ctx) resourceInformer := duck.NewResourceInformer(ctx) r := &Reconciler{ @@ -65,6 +67,7 @@ func NewController( subscriptionLister: subscriptionInformer.Lister(), brokerLister: brokerInformer.Lister(), serviceLister: serviceInformer.Lister(), + namespaceLister: namespaceInformer.Lister(), } impl := controller.NewImpl(r, r.Logger, ReconcilerName) diff --git a/pkg/reconciler/trigger/controller_test.go b/pkg/reconciler/trigger/controller_test.go index 0a50a52cae4..39b8c3cc58b 100644 --- a/pkg/reconciler/trigger/controller_test.go +++ b/pkg/reconciler/trigger/controller_test.go @@ -22,12 +22,12 @@ import ( "knative.dev/pkg/configmap" . "knative.dev/pkg/reconciler/testing" - _ "knative.dev/pkg/client/injection/kube/informers/core/v1/service/fake" - // Fake injection informers _ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1alpha1/broker/fake" _ "knative.dev/eventing/pkg/client/injection/informers/eventing/v1alpha1/trigger/fake" _ "knative.dev/eventing/pkg/client/injection/informers/messaging/v1alpha1/subscription/fake" + _ "knative.dev/pkg/client/injection/kube/informers/core/v1/namespace/fake" + _ "knative.dev/pkg/client/injection/kube/informers/core/v1/service/fake" ) func TestNew(t *testing.T) { diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index 0172b7ee640..a4ae04d448f 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -70,6 +70,7 @@ type Reconciler struct { subscriptionLister messaginglisters.SubscriptionLister brokerLister listers.BrokerLister serviceLister corev1listers.ServiceLister + namespaceLister corev1listers.NamespaceLister resourceTracker duck.ResourceTracker kresourceInformerFactory apisduck.InformerFactory } @@ -156,11 +157,16 @@ func (r *Reconciler) reconcile(ctx context.Context, t *v1alpha1.Trigger) error { return err } - b, err := r.getBroker(ctx, t) + b, err := r.brokerLister.Brokers(t.Namespace).Get(t.Spec.Broker) if err != nil { logging.FromContext(ctx).Error("Unable to get the Broker", zap.Error(err)) if apierrs.IsNotFound(err) { t.Status.MarkBrokerFailed("DoesNotExist", "Broker does not exist") + if t.Spec.Broker == "default" { + if e := r.labelNamespace(ctx, t); e != nil { + logging.FromContext(ctx).Error("Unable to label the namespace", zap.Error(e)) + } + } } else { t.Status.MarkBrokerFailed("BrokerGetFailed", "Failed to get broker") } @@ -303,27 +309,23 @@ func (r *Reconciler) updateStatus(ctx context.Context, desired *v1alpha1.Trigger return trig, err } -// getBroker returns trigger's coupled broker. If the trigger is coupled with default broker, and default broker does not exist, -// it will create the default broker first by labeling namespace with knative-eventing-injection=enabled -func (r *Reconciler) getBroker(ctx context.Context, t *v1alpha1.Trigger) (*v1alpha1.Broker, error) { - b, err := r.brokerLister.Brokers(t.Namespace).Get(t.Spec.Broker) - if t.Spec.Broker == "default" && apierrs.IsNotFound(err) { - current, e := r.KubeClientSet.CoreV1().Namespaces().Get(t.Namespace, metav1.GetOptions{}) - if e != nil { - logging.FromContext(ctx).Error("Unable to get namespace resource to enable knative-eventing-injection", zap.Error(e)) - return b, err - } - if current.Labels == nil { - current.Labels = map[string]string{} - } - current.Labels["knative-eventing-injection"] = "enabled" - _, e = r.KubeClientSet.CoreV1().Namespaces().Update(current) - if e != nil { - logging.FromContext(ctx).Error("Unable to label the namespace resource with knative-eventing-injection", zap.Error(e)) - return b, err - } +// labelNamespace will label namespace with knative-eventing-injection=enabled +func (r *Reconciler) labelNamespace(ctx context.Context, t *v1alpha1.Trigger) error { + current, err := r.namespaceLister.Get(t.Namespace) + if err != nil { + t.Status.MarkBrokerFailed("NamespaceGetFailed", "Failed to get namespace resource to enable knative-eventing-injection") + return err + } + if current.Labels == nil { + current.Labels = map[string]string{} + } + current.Labels["knative-eventing-injection"] = "enabled" + _, err = r.KubeClientSet.CoreV1().Namespaces().Update(current) + if err != nil { + t.Status.MarkBrokerFailed("NamespaceUpdateFailed", "Failed to label the namespace resource with knative-eventing-injection") + return err } - return b, err + return nil } // getBrokerFilterService returns the K8s service for trigger 't' if exists, diff --git a/pkg/reconciler/trigger/trigger_test.go b/pkg/reconciler/trigger/trigger_test.go index a604bcd5ab0..063014914ad 100644 --- a/pkg/reconciler/trigger/trigger_test.go +++ b/pkg/reconciler/trigger/trigger_test.go @@ -644,6 +644,7 @@ func TestAllCases(t *testing.T) { subscriptionLister: listers.GetSubscriptionLister(), brokerLister: listers.GetBrokerLister(), serviceLister: listers.GetK8sServiceLister(), + namespaceLister: listers.GetNamespaceLister(), resourceTracker: &MockResourceTracker{}, kresourceInformerFactory: KResourceTypedInformerFactory(ctx), } From 811e6bb74212418256d99c126322052a5350b52f Mon Sep 17 00:00:00 2001 From: grac3gao Date: Wed, 16 Oct 2019 15:26:12 -0700 Subject: [PATCH 4/9] code change --- pkg/apis/eventing/v1alpha1/trigger_types.go | 3 +++ .../eventing/v1alpha1/trigger_validation.go | 16 ++++++++++++++ .../v1alpha1/trigger_validation_test.go | 22 +++++++++++++++++++ pkg/reconciler/trigger/trigger.go | 16 ++++++++++++-- 4 files changed, 55 insertions(+), 2 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/trigger_types.go b/pkg/apis/eventing/v1alpha1/trigger_types.go index 8bb2658bba5..482d77aa971 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_types.go +++ b/pkg/apis/eventing/v1alpha1/trigger_types.go @@ -31,6 +31,9 @@ const ( // DependencyAnnotation is the annotation key used to mark the sources that the Trigger depends on. // This will be used when the kn client creates an importer and trigger pair for the user such that the trigger only receives events produced by the paired importer. DependencyAnnotation = "knative.dev/dependency" + // CreateDefaultBrokerAnnotation is the annotation key used to automatically create a default broker. + // This will be used when the client creates an trigger paired with default broker and the default broker doesn't exist in the namespace + CreateDefaultBrokerAnnotation = "eventing.knative.dev/create-default-broker" ) // +genclient diff --git a/pkg/apis/eventing/v1alpha1/trigger_validation.go b/pkg/apis/eventing/v1alpha1/trigger_validation.go index 3755a8bcbe2..5acbd8ad30a 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_validation.go +++ b/pkg/apis/eventing/v1alpha1/trigger_validation.go @@ -21,6 +21,7 @@ import ( "encoding/json" "fmt" "regexp" + "strconv" "knative.dev/pkg/apis" "knative.dev/pkg/kmp" @@ -42,6 +43,10 @@ func (t *Trigger) Validate(ctx context.Context) *apis.FieldError { dependencyAnnotationPrefix := fmt.Sprintf("metadata.annotations[%s]", DependencyAnnotation) errs = errs.Also(t.validateDependencyAnnotation(dependencyAnnotation).ViaField(dependencyAnnotationPrefix)) } + createDefaultBrokerAnnotation, ok := t.GetAnnotations()[CreateDefaultBrokerAnnotation] + if ok { + errs = errs.Also(t.validateCreateDefaultBrokerAnnotation(createDefaultBrokerAnnotation)) + } return errs } @@ -169,3 +174,14 @@ func (t *Trigger) validateDependencyAnnotation(dependencyAnnotation string) *api } return errs } + +func (t *Trigger) validateCreateDefaultBrokerAnnotation(createDefaultBrokerAnnotation string) *apis.FieldError { + _, err := strconv.ParseBool(createDefaultBrokerAnnotation) + if err != nil { + return &apis.FieldError{ + Message: fmt.Sprintf("The provided create default broker annotation value (%q) was not true/false", createDefaultBrokerAnnotation), + Paths: []string{""}, + } + } + return nil +} diff --git a/pkg/apis/eventing/v1alpha1/trigger_validation_test.go b/pkg/apis/eventing/v1alpha1/trigger_validation_test.go index 0466e8c8c3a..02fa282e294 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_validation_test.go +++ b/pkg/apis/eventing/v1alpha1/trigger_validation_test.go @@ -55,9 +55,13 @@ var ( APIVersion: "serving.knative.dev/v1alpha1", }, } + // Dependency annotation validDependencyAnnotation = "{\"kind\":\"CronJobSource\",\"name\":\"test-cronjob-source\",\"apiVersion\":\"sources.eventing.knative.dev/v1alpha1\"}" invalidDependencyAnnotation = "invalid dependency annotation" dependencyAnnotationPath = fmt.Sprintf("metadata.annotations[%s]", DependencyAnnotation) + // Create default broker annotation + validCreateDefaultBrokerAnnotation = "true" + invalidCreateDefaultBrokerAnnotation = "yes" ) func TestTriggerValidation(t *testing.T) { @@ -197,6 +201,24 @@ func TestTriggerValidation(t *testing.T) { Message: "missing field(s)", }, }, + { + name: "invalid create default broker annotation value", + t: &Trigger{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "test-ns", + Annotations: map[string]string{ + CreateDefaultBrokerAnnotation: invalidCreateDefaultBrokerAnnotation, + }}, + Spec: TriggerSpec{ + Broker: "test_broker", + Filter: validEmptyFilter, + Subscriber: validSubscriber, + }}, + want: &apis.FieldError{ + Paths: []string{""}, + Message: "The provided create default broker annotation value (\"yes\") was not true/false", + }, + }, } for _, test := range tests { diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index cf4c03589bc..a86ed529870 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -22,6 +22,7 @@ import ( "fmt" "net/url" "reflect" + "strconv" "time" "go.uber.org/zap" @@ -163,8 +164,11 @@ func (r *Reconciler) reconcile(ctx context.Context, t *v1alpha1.Trigger) error { logging.FromContext(ctx).Error("Unable to get the Broker", zap.Error(err)) if apierrs.IsNotFound(err) { t.Status.MarkBrokerFailed("DoesNotExist", "Broker does not exist") - if t.Spec.Broker == "default" { - if e := r.labelNamespace(ctx, t); e != nil { + needDefaultBroker, e := r.checkCreateDefaultBrokerAnnotation(ctx, t) + if e != nil { + logging.FromContext(ctx).Error("Unable to get create default broker annotation", zap.Error(e)) + } else if t.Spec.Broker == "default" && needDefaultBroker { + if e = r.labelNamespace(ctx, t); e != nil { logging.FromContext(ctx).Error("Unable to label the namespace", zap.Error(e)) } } @@ -311,6 +315,14 @@ func (r *Reconciler) updateStatus(ctx context.Context, desired *v1alpha1.Trigger return trig, err } +func (r *Reconciler) checkCreateDefaultBrokerAnnotation(ctx context.Context, t *v1alpha1.Trigger) (bool, error) { + if createDefaultBrokerAnnotation, ok := t.GetAnnotations()[v1alpha1.CreateDefaultBrokerAnnotation]; ok { + return strconv.ParseBool(createDefaultBrokerAnnotation) + } else { + return false, nil + } +} + // labelNamespace will label namespace with knative-eventing-injection=enabled func (r *Reconciler) labelNamespace(ctx context.Context, t *v1alpha1.Trigger) error { current, err := r.namespaceLister.Get(t.Namespace) From 83d4fb168d1773c88121bc15d2411c1fb8d0812a Mon Sep 17 00:00:00 2001 From: grac3gao Date: Thu, 17 Oct 2019 21:38:57 -0700 Subject: [PATCH 5/9] code change --- pkg/apis/eventing/v1alpha1/trigger_types.go | 4 +-- .../eventing/v1alpha1/trigger_validation.go | 18 ++++++---- .../v1alpha1/trigger_validation_test.go | 30 ++++++++++++---- pkg/reconciler/testing/trigger.go | 9 +++++ pkg/reconciler/trigger/trigger.go | 18 +++++----- pkg/reconciler/trigger/trigger_test.go | 35 ++++++++++++++++++- 6 files changed, 88 insertions(+), 26 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/trigger_types.go b/pkg/apis/eventing/v1alpha1/trigger_types.go index 482d77aa971..c86298be182 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_types.go +++ b/pkg/apis/eventing/v1alpha1/trigger_types.go @@ -31,9 +31,9 @@ const ( // DependencyAnnotation is the annotation key used to mark the sources that the Trigger depends on. // This will be used when the kn client creates an importer and trigger pair for the user such that the trigger only receives events produced by the paired importer. DependencyAnnotation = "knative.dev/dependency" - // CreateDefaultBrokerAnnotation is the annotation key used to automatically create a default broker. + // InjectionAnnotation is the annotation key used to enable knative eventing injection for a namespace and automatically create a default broker. // This will be used when the client creates an trigger paired with default broker and the default broker doesn't exist in the namespace - CreateDefaultBrokerAnnotation = "eventing.knative.dev/create-default-broker" + InjectionAnnotation = "knative-eventing-injection" ) // +genclient diff --git a/pkg/apis/eventing/v1alpha1/trigger_validation.go b/pkg/apis/eventing/v1alpha1/trigger_validation.go index 5acbd8ad30a..cb7077f335c 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_validation.go +++ b/pkg/apis/eventing/v1alpha1/trigger_validation.go @@ -21,7 +21,6 @@ import ( "encoding/json" "fmt" "regexp" - "strconv" "knative.dev/pkg/apis" "knative.dev/pkg/kmp" @@ -43,9 +42,9 @@ func (t *Trigger) Validate(ctx context.Context) *apis.FieldError { dependencyAnnotationPrefix := fmt.Sprintf("metadata.annotations[%s]", DependencyAnnotation) errs = errs.Also(t.validateDependencyAnnotation(dependencyAnnotation).ViaField(dependencyAnnotationPrefix)) } - createDefaultBrokerAnnotation, ok := t.GetAnnotations()[CreateDefaultBrokerAnnotation] + injectionAnnotation, ok := t.GetAnnotations()[InjectionAnnotation] if ok { - errs = errs.Also(t.validateCreateDefaultBrokerAnnotation(createDefaultBrokerAnnotation)) + errs = errs.Also(t.validateInjectionAnnotation(injectionAnnotation)) } return errs } @@ -175,11 +174,16 @@ func (t *Trigger) validateDependencyAnnotation(dependencyAnnotation string) *api return errs } -func (t *Trigger) validateCreateDefaultBrokerAnnotation(createDefaultBrokerAnnotation string) *apis.FieldError { - _, err := strconv.ParseBool(createDefaultBrokerAnnotation) - if err != nil { +func (t *Trigger) validateInjectionAnnotation(injectionAnnotation string) *apis.FieldError { + if injectionAnnotation != "enabled" { + return &apis.FieldError{ + Message: fmt.Sprintf("The provided injection annotation value can only be \"enabled\", not %q", injectionAnnotation), + Paths: []string{""}, + } + } + if t.Spec.Broker != "default" { return &apis.FieldError{ - Message: fmt.Sprintf("The provided create default broker annotation value (%q) was not true/false", createDefaultBrokerAnnotation), + Message: fmt.Sprintf("The provided injection annotation is only used for default borker, but non-default broker specified here: %q", t.Spec.Broker), Paths: []string{""}, } } diff --git a/pkg/apis/eventing/v1alpha1/trigger_validation_test.go b/pkg/apis/eventing/v1alpha1/trigger_validation_test.go index 02fa282e294..cc72ac245e3 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_validation_test.go +++ b/pkg/apis/eventing/v1alpha1/trigger_validation_test.go @@ -60,8 +60,8 @@ var ( invalidDependencyAnnotation = "invalid dependency annotation" dependencyAnnotationPath = fmt.Sprintf("metadata.annotations[%s]", DependencyAnnotation) // Create default broker annotation - validCreateDefaultBrokerAnnotation = "true" - invalidCreateDefaultBrokerAnnotation = "yes" + validInjectionAnnotation = "enabled" + invalidInjectionAnnotation = "disabled" ) func TestTriggerValidation(t *testing.T) { @@ -202,21 +202,39 @@ func TestTriggerValidation(t *testing.T) { }, }, { - name: "invalid create default broker annotation value", + name: "invalid injection annotation value", t: &Trigger{ ObjectMeta: v1.ObjectMeta{ Namespace: "test-ns", Annotations: map[string]string{ - CreateDefaultBrokerAnnotation: invalidCreateDefaultBrokerAnnotation, + InjectionAnnotation: invalidInjectionAnnotation, }}, Spec: TriggerSpec{ - Broker: "test_broker", + Broker: "default", + Filter: validEmptyFilter, + Subscriber: validSubscriber, + }}, + want: &apis.FieldError{ + Paths: []string{""}, + Message: "The provided injection annotation value can only be \"enabled\", not \"disabled\"", + }, + }, + { + name: "valid injection annotation value, non-default broker specified", + t: &Trigger{ + ObjectMeta: v1.ObjectMeta{ + Namespace: "test-ns", + Annotations: map[string]string{ + InjectionAnnotation: validInjectionAnnotation, + }}, + Spec: TriggerSpec{ + Broker: "test-broker", Filter: validEmptyFilter, Subscriber: validSubscriber, }}, want: &apis.FieldError{ Paths: []string{""}, - Message: "The provided create default broker annotation value (\"yes\") was not true/false", + Message: "The provided injection annotation is only used for default borker, but non-default broker specified here: \"test-broker\"", }, }, } diff --git a/pkg/reconciler/testing/trigger.go b/pkg/reconciler/testing/trigger.go index 4fbb861629f..7aff3310807 100644 --- a/pkg/reconciler/testing/trigger.go +++ b/pkg/reconciler/testing/trigger.go @@ -121,6 +121,15 @@ func WithUnmarshalFailedDependencyAnnotation() TriggerOption { } } +func WithInjectionAnnotation(injectionAnnotation string) TriggerOption { + return func(t *v1alpha1.Trigger) { + if t.Annotations == nil { + t.Annotations = make(map[string]string) + } + t.Annotations[v1alpha1.InjectionAnnotation] = injectionAnnotation + } +} + func WithDependencyAnnotation(dependencyAnnotation string) TriggerOption { return func(t *v1alpha1.Trigger) { if t.Annotations == nil { diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index a86ed529870..9ca4d9a54aa 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -22,7 +22,6 @@ import ( "fmt" "net/url" "reflect" - "strconv" "time" "go.uber.org/zap" @@ -164,11 +163,9 @@ func (r *Reconciler) reconcile(ctx context.Context, t *v1alpha1.Trigger) error { logging.FromContext(ctx).Error("Unable to get the Broker", zap.Error(err)) if apierrs.IsNotFound(err) { t.Status.MarkBrokerFailed("DoesNotExist", "Broker does not exist") - needDefaultBroker, e := r.checkCreateDefaultBrokerAnnotation(ctx, t) - if e != nil { - logging.FromContext(ctx).Error("Unable to get create default broker annotation", zap.Error(e)) - } else if t.Spec.Broker == "default" && needDefaultBroker { - if e = r.labelNamespace(ctx, t); e != nil { + needDefaultBroker := r.checkInjectionAnnotation(ctx, t) + if t.Spec.Broker == "default" && needDefaultBroker { + if e := r.labelNamespace(ctx, t); e != nil { logging.FromContext(ctx).Error("Unable to label the namespace", zap.Error(e)) } } @@ -315,11 +312,12 @@ func (r *Reconciler) updateStatus(ctx context.Context, desired *v1alpha1.Trigger return trig, err } -func (r *Reconciler) checkCreateDefaultBrokerAnnotation(ctx context.Context, t *v1alpha1.Trigger) (bool, error) { - if createDefaultBrokerAnnotation, ok := t.GetAnnotations()[v1alpha1.CreateDefaultBrokerAnnotation]; ok { - return strconv.ParseBool(createDefaultBrokerAnnotation) +// checkInjectionAnnotation will check if a default broker needs to be created +func (r *Reconciler) checkInjectionAnnotation(ctx context.Context, t *v1alpha1.Trigger) bool { + if _, ok := t.GetAnnotations()[v1alpha1.InjectionAnnotation]; ok { + return true } else { - return false, nil + return false } } diff --git a/pkg/reconciler/trigger/trigger_test.go b/pkg/reconciler/trigger/trigger_test.go index 6cafbc500da..00e2ba82b59 100644 --- a/pkg/reconciler/trigger/trigger_test.go +++ b/pkg/reconciler/trigger/trigger_test.go @@ -82,6 +82,8 @@ const ( testData = "data" sinkName = "testsink" + injectionAnnotation = "enabled" + currentGeneration = 1 outdatedGeneration = 0 ) @@ -121,7 +123,7 @@ func TestAllCases(t *testing.T) { // Eventf(corev1.EventTypeWarning, "ChannelReferenceFetchFailed", "Failed to validate spec.channel exists: s \"\" not found"), // }, }, { - Name: "Broker not found", + Name: "Non-default broker not found", Key: triggerKey, Objects: []runtime.Object{ reconciletesting.NewTrigger(triggerName, testNS, brokerName, @@ -141,6 +143,31 @@ func TestAllCases(t *testing.T) { reconciletesting.WithTriggerBrokerFailed("DoesNotExist", "Broker does not exist"), ), }}, + }, { + Name: "Default broker not found, with injection annotation enabled", + Key: triggerKey, + Objects: []runtime.Object{ + makeReadyDefaultBroker(), + reconciletesting.NewTrigger(triggerName, testNS, "default", + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithInitTriggerConditions, + reconciletesting.WithInjectionAnnotation(injectionAnnotation)), + }, + WantErr: true, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "TriggerServiceFailed", "Broker's Filter service not found"), + Eventf(corev1.EventTypeWarning, "TriggerReconcileFailed", "Trigger reconciliation failed: failed to find Broker's Filter service"), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: reconciletesting.NewTrigger(triggerName, testNS, "default", + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithInitTriggerConditions, + reconciletesting.WithTriggerBrokerReady(), + reconciletesting.WithInjectionAnnotation(injectionAnnotation), + ), + }}, }, { Name: "Broker get failure", Key: triggerKey, @@ -722,6 +749,12 @@ func makeReadyBroker() *v1alpha1.Broker { return b } +func makeReadyDefaultBroker() *v1alpha1.Broker { + b := makeReadyBroker() + b.Name = "default" + return b +} + func makeTriggerChannelRef() *corev1.ObjectReference { return &corev1.ObjectReference{ APIVersion: "eventing.knative.dev/v1alpha1", From b682ec94be55bffb80e546c9c13a0fd342995ef1 Mon Sep 17 00:00:00 2001 From: grac3gao Date: Fri, 18 Oct 2019 10:51:38 -0700 Subject: [PATCH 6/9] code change after review --- pkg/apis/eventing/v1alpha1/trigger_validation.go | 6 ++---- pkg/reconciler/trigger/trigger.go | 14 ++------------ 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/trigger_validation.go b/pkg/apis/eventing/v1alpha1/trigger_validation.go index cb7077f335c..1fa26d018a2 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_validation.go +++ b/pkg/apis/eventing/v1alpha1/trigger_validation.go @@ -37,13 +37,11 @@ var ( // Validate the Trigger. func (t *Trigger) Validate(ctx context.Context) *apis.FieldError { errs := t.Spec.Validate(ctx).ViaField("spec") - dependencyAnnotation, ok := t.GetAnnotations()[DependencyAnnotation] - if ok { + if dependencyAnnotation, ok := t.GetAnnotations()[DependencyAnnotation]; ok { dependencyAnnotationPrefix := fmt.Sprintf("metadata.annotations[%s]", DependencyAnnotation) errs = errs.Also(t.validateDependencyAnnotation(dependencyAnnotation).ViaField(dependencyAnnotationPrefix)) } - injectionAnnotation, ok := t.GetAnnotations()[InjectionAnnotation] - if ok { + if injectionAnnotation, ok := t.GetAnnotations()[InjectionAnnotation]; ok { errs = errs.Also(t.validateInjectionAnnotation(injectionAnnotation)) } return errs diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index 9ca4d9a54aa..d38a06aedc8 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -163,7 +163,7 @@ func (r *Reconciler) reconcile(ctx context.Context, t *v1alpha1.Trigger) error { logging.FromContext(ctx).Error("Unable to get the Broker", zap.Error(err)) if apierrs.IsNotFound(err) { t.Status.MarkBrokerFailed("DoesNotExist", "Broker does not exist") - needDefaultBroker := r.checkInjectionAnnotation(ctx, t) + _, needDefaultBroker := t.GetAnnotations()[v1alpha1.InjectionAnnotation] if t.Spec.Broker == "default" && needDefaultBroker { if e := r.labelNamespace(ctx, t); e != nil { logging.FromContext(ctx).Error("Unable to label the namespace", zap.Error(e)) @@ -312,15 +312,6 @@ func (r *Reconciler) updateStatus(ctx context.Context, desired *v1alpha1.Trigger return trig, err } -// checkInjectionAnnotation will check if a default broker needs to be created -func (r *Reconciler) checkInjectionAnnotation(ctx context.Context, t *v1alpha1.Trigger) bool { - if _, ok := t.GetAnnotations()[v1alpha1.InjectionAnnotation]; ok { - return true - } else { - return false - } -} - // labelNamespace will label namespace with knative-eventing-injection=enabled func (r *Reconciler) labelNamespace(ctx context.Context, t *v1alpha1.Trigger) error { current, err := r.namespaceLister.Get(t.Namespace) @@ -332,8 +323,7 @@ func (r *Reconciler) labelNamespace(ctx context.Context, t *v1alpha1.Trigger) er current.Labels = map[string]string{} } current.Labels["knative-eventing-injection"] = "enabled" - _, err = r.KubeClientSet.CoreV1().Namespaces().Update(current) - if err != nil { + if _, err = r.KubeClientSet.CoreV1().Namespaces().Update(current); err != nil { t.Status.MarkBrokerFailed("NamespaceUpdateFailed", "Failed to label the namespace resource with knative-eventing-injection") return err } From 25d3ec3abd2e47c07662c39f1745f9e704dbc5ea Mon Sep 17 00:00:00 2001 From: grac3gao Date: Fri, 18 Oct 2019 12:46:20 -0700 Subject: [PATCH 7/9] code change after review --- pkg/apis/eventing/v1alpha1/trigger_validation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/eventing/v1alpha1/trigger_validation.go b/pkg/apis/eventing/v1alpha1/trigger_validation.go index 1fa26d018a2..92b666b3863 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_validation.go +++ b/pkg/apis/eventing/v1alpha1/trigger_validation.go @@ -175,7 +175,7 @@ func (t *Trigger) validateDependencyAnnotation(dependencyAnnotation string) *api func (t *Trigger) validateInjectionAnnotation(injectionAnnotation string) *apis.FieldError { if injectionAnnotation != "enabled" { return &apis.FieldError{ - Message: fmt.Sprintf("The provided injection annotation value can only be \"enabled\", not %q", injectionAnnotation), + Message: fmt.Sprintf(`The provided injection annotation value can only be "enabled", not %q`, injectionAnnotation), Paths: []string{""}, } } From ff73aa9ce59c92c6a4fa7bc4ba637fd2119c486c Mon Sep 17 00:00:00 2001 From: grac3gao Date: Thu, 24 Oct 2019 13:48:17 -0700 Subject: [PATCH 8/9] code change after review --- pkg/apis/eventing/v1alpha1/trigger_types.go | 2 +- .../eventing/v1alpha1/trigger_validation.go | 19 ++++++++------- .../v1alpha1/trigger_validation_test.go | 7 +++--- pkg/reconciler/trigger/trigger_test.go | 24 +++++++++++++++++++ 4 files changed, 40 insertions(+), 12 deletions(-) diff --git a/pkg/apis/eventing/v1alpha1/trigger_types.go b/pkg/apis/eventing/v1alpha1/trigger_types.go index c86298be182..3cae11fc5db 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_types.go +++ b/pkg/apis/eventing/v1alpha1/trigger_types.go @@ -32,7 +32,7 @@ const ( // This will be used when the kn client creates an importer and trigger pair for the user such that the trigger only receives events produced by the paired importer. DependencyAnnotation = "knative.dev/dependency" // InjectionAnnotation is the annotation key used to enable knative eventing injection for a namespace and automatically create a default broker. - // This will be used when the client creates an trigger paired with default broker and the default broker doesn't exist in the namespace + // This will be used when the client creates a trigger paired with default broker and the default broker doesn't exist in the namespace InjectionAnnotation = "knative-eventing-injection" ) diff --git a/pkg/apis/eventing/v1alpha1/trigger_validation.go b/pkg/apis/eventing/v1alpha1/trigger_validation.go index 92b666b3863..0a2be546940 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_validation.go +++ b/pkg/apis/eventing/v1alpha1/trigger_validation.go @@ -37,13 +37,8 @@ var ( // Validate the Trigger. func (t *Trigger) Validate(ctx context.Context) *apis.FieldError { errs := t.Spec.Validate(ctx).ViaField("spec") - if dependencyAnnotation, ok := t.GetAnnotations()[DependencyAnnotation]; ok { - dependencyAnnotationPrefix := fmt.Sprintf("metadata.annotations[%s]", DependencyAnnotation) - errs = errs.Also(t.validateDependencyAnnotation(dependencyAnnotation).ViaField(dependencyAnnotationPrefix)) - } - if injectionAnnotation, ok := t.GetAnnotations()[InjectionAnnotation]; ok { - errs = errs.Also(t.validateInjectionAnnotation(injectionAnnotation)) - } + errs = t.validateAnnotation(errs, DependencyAnnotation, t.validateDependencyAnnotation) + errs = t.validateAnnotation(errs, InjectionAnnotation, t.validateInjectionAnnotation) return errs } @@ -140,6 +135,14 @@ func GetObjRefFromDependencyAnnotation(dependencyAnnotation string) (corev1.Obje return objectRef, nil } +func (t *Trigger) validateAnnotation(errs *apis.FieldError, annotation string, function func(string) *apis.FieldError) *apis.FieldError { + if annotationValue, ok := t.GetAnnotations()[annotation]; ok { + annotationPrefix := fmt.Sprintf("metadata.annotations[%s]", annotation) + errs = errs.Also(function(annotationValue).ViaField(annotationPrefix)) + } + return errs +} + func (t *Trigger) validateDependencyAnnotation(dependencyAnnotation string) *apis.FieldError { depObjRef, err := GetObjRefFromDependencyAnnotation(dependencyAnnotation) if err != nil { @@ -181,7 +184,7 @@ func (t *Trigger) validateInjectionAnnotation(injectionAnnotation string) *apis. } if t.Spec.Broker != "default" { return &apis.FieldError{ - Message: fmt.Sprintf("The provided injection annotation is only used for default borker, but non-default broker specified here: %q", t.Spec.Broker), + Message: fmt.Sprintf("The provided injection annotation is only used for default broker, but non-default broker specified here: %q", t.Spec.Broker), Paths: []string{""}, } } diff --git a/pkg/apis/eventing/v1alpha1/trigger_validation_test.go b/pkg/apis/eventing/v1alpha1/trigger_validation_test.go index cc72ac245e3..06f1857111a 100644 --- a/pkg/apis/eventing/v1alpha1/trigger_validation_test.go +++ b/pkg/apis/eventing/v1alpha1/trigger_validation_test.go @@ -62,6 +62,7 @@ var ( // Create default broker annotation validInjectionAnnotation = "enabled" invalidInjectionAnnotation = "disabled" + injectionAnnotationPath = fmt.Sprintf("metadata.annotations[%s]", InjectionAnnotation) ) func TestTriggerValidation(t *testing.T) { @@ -215,7 +216,7 @@ func TestTriggerValidation(t *testing.T) { Subscriber: validSubscriber, }}, want: &apis.FieldError{ - Paths: []string{""}, + Paths: []string{injectionAnnotationPath}, Message: "The provided injection annotation value can only be \"enabled\", not \"disabled\"", }, }, @@ -233,8 +234,8 @@ func TestTriggerValidation(t *testing.T) { Subscriber: validSubscriber, }}, want: &apis.FieldError{ - Paths: []string{""}, - Message: "The provided injection annotation is only used for default borker, but non-default broker specified here: \"test-broker\"", + Paths: []string{injectionAnnotationPath}, + Message: "The provided injection annotation is only used for default broker, but non-default broker specified here: \"test-broker\"", }, }, } diff --git a/pkg/reconciler/trigger/trigger_test.go b/pkg/reconciler/trigger/trigger_test.go index 00e2ba82b59..48323e21a19 100644 --- a/pkg/reconciler/trigger/trigger_test.go +++ b/pkg/reconciler/trigger/trigger_test.go @@ -146,6 +146,29 @@ func TestAllCases(t *testing.T) { }, { Name: "Default broker not found, with injection annotation enabled", Key: triggerKey, + Objects: []runtime.Object{ + reconciletesting.NewTrigger(triggerName, testNS, "default", + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithInitTriggerConditions, + reconciletesting.WithInjectionAnnotation(injectionAnnotation)), + }, + WantErr: true, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "TriggerReconcileFailed", "Trigger reconciliation failed: broker.eventing.knative.dev \"default\" not found"), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: reconciletesting.NewTrigger(triggerName, testNS, "default", + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithInitTriggerConditions, + reconciletesting.WithInjectionAnnotation(injectionAnnotation), + reconciletesting.WithTriggerBrokerFailed("NamespaceGetFailed", "Failed to get namespace resource to enable knative-eventing-injection"), + ), + }}, + }, { + Name: "Default broker found, with injection annotation enabled", + Key: triggerKey, Objects: []runtime.Object{ makeReadyDefaultBroker(), reconciletesting.NewTrigger(triggerName, testNS, "default", @@ -156,6 +179,7 @@ func TestAllCases(t *testing.T) { }, WantErr: true, WantEvents: []string{ + // Only check if default broker is ready (not check other resources), so failed at the next step, check for filter service Eventf(corev1.EventTypeWarning, "TriggerServiceFailed", "Broker's Filter service not found"), Eventf(corev1.EventTypeWarning, "TriggerReconcileFailed", "Trigger reconciliation failed: failed to find Broker's Filter service"), }, From 64be65aae5988e830e63d19007a07fd008e93069 Mon Sep 17 00:00:00 2001 From: grac3gao Date: Tue, 29 Oct 2019 10:45:38 -0700 Subject: [PATCH 9/9] code change after review --- pkg/reconciler/trigger/trigger.go | 1 + pkg/reconciler/trigger/trigger_test.go | 8 +++++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index d38a06aedc8..fbb55383236 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -319,6 +319,7 @@ func (r *Reconciler) labelNamespace(ctx context.Context, t *v1alpha1.Trigger) er t.Status.MarkBrokerFailed("NamespaceGetFailed", "Failed to get namespace resource to enable knative-eventing-injection") return err } + current = current.DeepCopy() if current.Labels == nil { current.Labels = map[string]string{} } diff --git a/pkg/reconciler/trigger/trigger_test.go b/pkg/reconciler/trigger/trigger_test.go index 48323e21a19..f008cf47feb 100644 --- a/pkg/reconciler/trigger/trigger_test.go +++ b/pkg/reconciler/trigger/trigger_test.go @@ -152,6 +152,8 @@ func TestAllCases(t *testing.T) { reconciletesting.WithTriggerSubscriberURI(subscriberURI), reconciletesting.WithInitTriggerConditions, reconciletesting.WithInjectionAnnotation(injectionAnnotation)), + reconciletesting.NewNamespace(testNS, + reconciletesting.WithNamespaceLabeled(map[string]string{})), }, WantErr: true, WantEvents: []string{ @@ -163,9 +165,13 @@ func TestAllCases(t *testing.T) { reconciletesting.WithTriggerSubscriberURI(subscriberURI), reconciletesting.WithInitTriggerConditions, reconciletesting.WithInjectionAnnotation(injectionAnnotation), - reconciletesting.WithTriggerBrokerFailed("NamespaceGetFailed", "Failed to get namespace resource to enable knative-eventing-injection"), + reconciletesting.WithTriggerBrokerFailed("DoesNotExist", "Broker does not exist"), ), }}, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: reconciletesting.NewNamespace(testNS, + reconciletesting.WithNamespaceLabeled(map[string]string{v1alpha1.InjectionAnnotation: injectionAnnotation})), + }}, }, { Name: "Default broker found, with injection annotation enabled", Key: triggerKey,