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..74222f53877 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -53,6 +53,14 @@ 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. + 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 { 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..dd3faf9c3da 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,45 @@ 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, "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), + 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..3af2e8b0dcb --- /dev/null +++ b/test/e2e/helpers/trigger_no_broker_test_helper.go @@ -0,0 +1,74 @@ +/* +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" +) + +// 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) + 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)) + }) +}