From aa91e75aa693f0b0769ed42a83ee549ac016d067 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Thu, 14 May 2020 11:11:24 -0700 Subject: [PATCH 1/3] set trigger status appropriately if they do not have broker --- cmd/controller/main.go | 1 + pkg/reconciler/trigger/trigger.go | 14 ++++ pkg/reconciler/trigger/trigger_test.go | 57 ++++++++++++++ .../helpers/trigger_no_broker_test_helper.go | 75 +++++++++++++++++++ test/e2e/trigger_no_broker_test.go | 33 ++++++++ 5 files changed, 180 insertions(+) create mode 100644 test/e2e/helpers/trigger_no_broker_test_helper.go create mode 100644 test/e2e/trigger_no_broker_test.go diff --git a/cmd/controller/main.go b/cmd/controller/main.go index f07fa17b058..9ebcd1b1f7e 100644 --- a/cmd/controller/main.go +++ b/cmd/controller/main.go @@ -43,6 +43,7 @@ func main() { // Eventing eventtype.NewController, // Trigger namespace labeler for default broker. + // Also sets the Status of Triggers that do not have a Broker. trigger.NewController, // Flows diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index e4e0d56fab9..ace5bd8652a 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -53,6 +53,20 @@ var _ triggerreconciler.Interface = (*Reconciler)(nil) func (r *Reconciler) ReconcileKind(ctx context.Context, t *v1beta1.Trigger) reconciler.Event { _, err := r.brokerLister.Brokers(t.Namespace).Get(t.Spec.Broker) if err != nil && apierrs.IsNotFound(err) { + // https://github.com/knative/eventing/issues/2996 + // Ideally we'd default the Status of the Trigger during creation but we currently + // can't, so watch for Triggers that do not have Broker for them and set their status. + // This only addresses one part of the problem (missing the Broker outright, but + // not the problem where the broker is misconfigured (wrong BrokerClass)), and hence + // it still would not get reconciled. + tt := t.DeepCopy() + tt.Status.MarkBrokerFailed("BrokerDoesNotExist", "Broker %q does not exist or there is no matching BrokerClass for it", tt.Spec.Broker) + _, te := r.eventingClientSet.EventingV1beta1().Triggers(tt.Namespace).UpdateStatus(tt) + if te != nil { + logging.FromContext(ctx).Errorw("Unable to update status for Trigger with missing broker", zap.Error(te)) + return te + } + _, needDefaultBroker := t.GetAnnotations()[v1beta1.InjectionAnnotation] if t.Spec.Broker == "default" && needDefaultBroker { if e := r.labelNamespace(ctx, t); e != nil { diff --git a/pkg/reconciler/trigger/trigger_test.go b/pkg/reconciler/trigger/trigger_test.go index 08a6cdb563c..e88d652e538 100644 --- a/pkg/reconciler/trigger/trigger_test.go +++ b/pkg/reconciler/trigger/trigger_test.go @@ -82,6 +82,12 @@ func TestAllCases(t *testing.T) { reconciletesting.WithTriggerUID(triggerUID), reconciletesting.WithTriggerSubscriberURI(subscriberURI)), }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: reconciletesting.NewTrigger(triggerName, testNS, brokerName, + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithTriggerBrokerFailed("BrokerDoesNotExist", `Broker "test-broker" does not exist or there is no matching BrokerClass for it`)), + }}, WantErr: false, }, { Name: "Default broker not found, with injection annotation enabled", @@ -96,6 +102,14 @@ func TestAllCases(t *testing.T) { reconciletesting.WithNamespaceLabeled(map[string]string{})), }, WantErr: false, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: reconciletesting.NewTrigger(triggerName, testNS, "default", + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithInjectionAnnotation(injectionAnnotation), + reconciletesting.WithInitTriggerConditions, + reconciletesting.WithTriggerBrokerFailed("BrokerDoesNotExist", `Broker "default" does not exist or there is no matching BrokerClass for it`)), + }}, WantUpdates: []clientgotesting.UpdateActionImpl{{ Object: reconciletesting.NewNamespace(testNS, reconciletesting.WithNamespaceLabeled(map[string]string{v1beta1.InjectionAnnotation: injectionAnnotation})), @@ -120,6 +134,14 @@ func TestAllCases(t *testing.T) { WantEvents: []string{ Eventf(corev1.EventTypeWarning, "InternalError", "namespace \"test-namespace\" not found"), }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: reconciletesting.NewTrigger(triggerName, testNS, "default", + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithInjectionAnnotation(injectionAnnotation), + reconciletesting.WithInitTriggerConditions, + reconciletesting.WithTriggerBrokerFailed("BrokerDoesNotExist", `Broker "default" does not exist or there is no matching BrokerClass for it`)), + }}, }, { Name: "Default broker not found, with injection annotation enabled, namespace label fail", Key: triggerKey, @@ -143,6 +165,41 @@ func TestAllCases(t *testing.T) { Object: reconciletesting.NewNamespace(testNS, reconciletesting.WithNamespaceLabeled(map[string]string{v1beta1.InjectionAnnotation: injectionAnnotation})), }}, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: reconciletesting.NewTrigger(triggerName, testNS, "default", + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithInjectionAnnotation(injectionAnnotation), + reconciletesting.WithInitTriggerConditions, + reconciletesting.WithTriggerBrokerFailed("BrokerDoesNotExist", `Broker "default" does not exist or there is no matching BrokerClass for it`)), + }}, + }, { + Name: "Default broker not found, with injection annotation enabled, trigger status update fail", + Key: triggerKey, + Objects: []runtime.Object{ + reconciletesting.NewTrigger(triggerName, testNS, "default", + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithInitTriggerConditions, + reconciletesting.WithInjectionAnnotation(injectionAnnotation)), + reconciletesting.NewNamespace(testNS, + reconciletesting.WithNamespaceLabeled(map[string]string{})), + }, + WantErr: true, + WithReactors: []clientgotesting.ReactionFunc{ + InduceFailure("update", "triggers"), + }, + WantEvents: []string{ + Eventf(corev1.EventTypeWarning, "InternalError", "inducing failure for update triggers"), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: reconciletesting.NewTrigger(triggerName, testNS, "default", + reconciletesting.WithTriggerUID(triggerUID), + reconciletesting.WithTriggerSubscriberURI(subscriberURI), + reconciletesting.WithInjectionAnnotation(injectionAnnotation), + reconciletesting.WithInitTriggerConditions, + reconciletesting.WithTriggerBrokerFailed("BrokerDoesNotExist", `Broker "default" does not exist or there is no matching BrokerClass for it`)), + }}, }, { Name: "Default broker found, with injection annotation enabled", Key: triggerKey, diff --git a/test/e2e/helpers/trigger_no_broker_test_helper.go b/test/e2e/helpers/trigger_no_broker_test_helper.go new file mode 100644 index 00000000000..0b8bd98c0d3 --- /dev/null +++ b/test/e2e/helpers/trigger_no_broker_test_helper.go @@ -0,0 +1,75 @@ +/* +Copyright 2020 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 helpers + +import ( + "strings" + "testing" + "time" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/wait" + "knative.dev/eventing/test/lib" + "knative.dev/eventing/test/lib/resources" +) + +const ( + brokerName = "notdefaultbroker" +) + +// If shouldLabelNamespace is set to true this test annotates the testing namespace so that a default broker is created. +// It then binds many triggers with different filtering patterns to the broker created by brokerCreator, and sends +// different events to the broker's address. +// Finally, it verifies that only the appropriate events are routed to the subscribers. +func TestTriggerNoBroker(t *testing.T, channel string, brokerCreator BrokerCreator) { + client := lib.Setup(t, true) + defer lib.TearDown(client) + brokerName := strings.ToLower(channel) + subscriberName := name("dumper", "", "", map[string]interface{}{}) + pod := resources.EventLoggerPod(subscriberName) + client.CreatePodOrFail(pod, lib.WithService(subscriberName)) + client.CreateTriggerOrFailV1Beta1("testtrigger", + resources.WithSubscriberServiceRefForTriggerV1Beta1(subscriberName), + resources.WithBrokerV1Beta1(brokerName), + ) + + // Then make sure the trigger is marked as not ready since there's no broker. + err := wait.PollImmediate(1*time.Second, 10*time.Second, func() (bool, error) { + trigger, err := client.Eventing.EventingV1beta1().Triggers(client.Namespace).Get("testtrigger", metav1.GetOptions{}) + if err != nil { + return false, err + } + if ready := trigger.Status.GetTopLevelCondition(); ready != nil { + if ready.Status == corev1.ConditionFalse && ready.Reason == "BrokerDoesNotExist" { + return true, nil + } + } + return false, nil + }) + if err != nil { + t.Fatalf("Trigger status did not get marked as BrokerDoesNotExist: %s", err) + } + + // Then create the Broker and just make sure they both come ready. + if bn := brokerCreator(client); bn != brokerName { + t.Fatalf("Broker created with unexpected name, wanted %q got %q", brokerName, bn) + } + + // Wait for all test resources to become ready before sending the events. + client.WaitForAllTestResourcesReadyOrFail() +} diff --git a/test/e2e/trigger_no_broker_test.go b/test/e2e/trigger_no_broker_test.go new file mode 100644 index 00000000000..0b3849aa905 --- /dev/null +++ b/test/e2e/trigger_no_broker_test.go @@ -0,0 +1,33 @@ +// +build e2e + +/* +Copyright 2020 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 e2e + +import ( + "testing" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "knative.dev/eventing/test/e2e/helpers" + "knative.dev/eventing/test/lib" +) + +func TestTriggerNoBroker(t *testing.T) { + channelTestRunner.RunTests(t, lib.FeatureBasic, func(t *testing.T, channel metav1.TypeMeta) { + helpers.TestTriggerNoBroker(t, channel.Kind, helpers.ChannelBasedBrokerCreator(channel, brokerClass)) + }) +} From 56cfd0d18c33f987b224a158132949ca7ca23c80 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Mon, 18 May 2020 09:24:15 -0700 Subject: [PATCH 2/3] Fix the comments on the test to match reality --- test/e2e/helpers/trigger_no_broker_test_helper.go | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/test/e2e/helpers/trigger_no_broker_test_helper.go b/test/e2e/helpers/trigger_no_broker_test_helper.go index 0b8bd98c0d3..3af2e8b0dcb 100644 --- a/test/e2e/helpers/trigger_no_broker_test_helper.go +++ b/test/e2e/helpers/trigger_no_broker_test_helper.go @@ -32,10 +32,9 @@ const ( brokerName = "notdefaultbroker" ) -// If shouldLabelNamespace is set to true this test annotates the testing namespace so that a default broker is created. -// It then binds many triggers with different filtering patterns to the broker created by brokerCreator, and sends -// different events to the broker's address. -// Finally, it verifies that only the appropriate events are routed to the subscribers. +// TestTriggerNoBroker will create a Trigger with a non-existent broker, then it will ensure +// the Status is correctly reflected as failed with BrokerDoesNotExist. Then it will create +// the broker and ensure that Trigger / Broker will get to Ready state. func TestTriggerNoBroker(t *testing.T, channel string, brokerCreator BrokerCreator) { client := lib.Setup(t, true) defer lib.TearDown(client) From 65510a5fb3c6773d10ea4af718fc8cef81b6b079 Mon Sep 17 00:00:00 2001 From: Ville Aikas Date: Mon, 18 May 2020 10:07:20 -0700 Subject: [PATCH 3/3] oh man... really --- pkg/reconciler/trigger/trigger.go | 8 +------- pkg/reconciler/trigger/trigger_test.go | 6 +++++- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index ace5bd8652a..74222f53877 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -59,13 +59,7 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, t *v1beta1.Trigger) reco // This only addresses one part of the problem (missing the Broker outright, but // not the problem where the broker is misconfigured (wrong BrokerClass)), and hence // it still would not get reconciled. - tt := t.DeepCopy() - tt.Status.MarkBrokerFailed("BrokerDoesNotExist", "Broker %q does not exist or there is no matching BrokerClass for it", tt.Spec.Broker) - _, te := r.eventingClientSet.EventingV1beta1().Triggers(tt.Namespace).UpdateStatus(tt) - if te != nil { - logging.FromContext(ctx).Errorw("Unable to update status for Trigger with missing broker", zap.Error(te)) - return te - } + t.Status.MarkBrokerFailed("BrokerDoesNotExist", "Broker %q does not exist or there is no matching BrokerClass for it", t.Spec.Broker) _, needDefaultBroker := t.GetAnnotations()[v1beta1.InjectionAnnotation] if t.Spec.Broker == "default" && needDefaultBroker { diff --git a/pkg/reconciler/trigger/trigger_test.go b/pkg/reconciler/trigger/trigger_test.go index e88d652e538..dd3faf9c3da 100644 --- a/pkg/reconciler/trigger/trigger_test.go +++ b/pkg/reconciler/trigger/trigger_test.go @@ -190,8 +190,12 @@ func TestAllCases(t *testing.T) { InduceFailure("update", "triggers"), }, WantEvents: []string{ - Eventf(corev1.EventTypeWarning, "InternalError", "inducing failure for update triggers"), + Eventf(corev1.EventTypeWarning, "UpdateFailed", "Failed to update status for \"test-trigger\": inducing failure for update triggers"), }, + WantUpdates: []clientgotesting.UpdateActionImpl{{ + Object: reconciletesting.NewNamespace(testNS, + reconciletesting.WithNamespaceLabeled(map[string]string{v1beta1.InjectionAnnotation: injectionAnnotation})), + }}, WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ Object: reconciletesting.NewTrigger(triggerName, testNS, "default", reconciletesting.WithTriggerUID(triggerUID),