From c7cf8238f91d7d521755660c7eace4de1362ed93 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Tue, 27 Jun 2023 17:24:13 +0200 Subject: [PATCH 1/2] fixing bug in name of Eric Solander to avoid CLA troubles, is fixing the triger subscriber namespace bug in case the subscriber has a different namespace from the trigger --- pkg/reconciler/trigger/trigger.go | 6 +- pkg/reconciler/trigger/trigger_test.go | 77 ++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index 3510ccae76..45cc09bfda 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -271,8 +271,10 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, t *eventingv1.Trigger) p if t.Spec.Subscriber.Ref != nil { // To call URIFromDestination(dest apisv1alpha1.Destination, parent interface{}), dest.Ref must have a Namespace - // We will use the Namespace of Trigger as the Namespace of dest.Ref - t.Spec.Subscriber.Ref.Namespace = t.GetNamespace() + if t.Spec.Subscriber.Ref.Namespace == "" { + // We will use the Namespace of Trigger as the Namespace of dest.Ref if one is not provided + t.Spec.Subscriber.Ref.Namespace = t.GetNamespace() + } } subscriberURI, err := r.uriResolver.URIFromDestinationV1(ctx, t.Spec.Subscriber, t) diff --git a/pkg/reconciler/trigger/trigger_test.go b/pkg/reconciler/trigger/trigger_test.go index 97ccdf95fc..058c17d0c5 100644 --- a/pkg/reconciler/trigger/trigger_test.go +++ b/pkg/reconciler/trigger/trigger_test.go @@ -76,6 +76,7 @@ import ( const ( systemNS = "knative-testing" testNS = "test-namespace" + otherNS = "other-namespace" brokerClass = "RabbitMQBroker" brokerName = "test-broker" brokerUID = "broker-test-uid" @@ -449,6 +450,64 @@ func TestReconcile(t *testing.T) { WithTriggerSubscriberResolvedSucceeded(), WithTriggerStatusSubscriberURI(subscriberURI)), }}, + }, { + Name: fmt.Sprintf("%s: Creates everything with ref subscriber different ns", name), + Key: testKey, + Objects: []runtime.Object{ + ReadyBroker(config), + makeSubscriberAddressableAsUnstructuredWithNamespace(otherNS), + markReady(createQueue(config, false, "")), + markReady(createBinding(false, false, "")), + NewTrigger(triggerName, testNS, brokerName, + WithTriggerUID(triggerUID), + WithTriggerSubscriberRef(subscriberGVK, subscriberName, otherNS)), + createSecret(rabbitURL), + createRabbitMQBrokerConfig(""), + createRabbitMQCluster(""), + }, + WantCreates: []runtime.Object{ + createDispatcherDeployment(false, ""), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: NewTrigger(triggerName, testNS, brokerName, + WithTriggerUID(triggerUID), + WithTriggerSubscriberRef(subscriberGVK, subscriberName, otherNS), + WithTriggerBrokerReady(), + WithTriggerDeadLetterSinkNotConfigured(), + WithTriggerDependencyReady(), + WithTriggerSubscribed(), + WithTriggerSubscriberResolvedSucceeded(), + WithTriggerStatusSubscriberURI(subscriberURI)), + }}, + }, { + Name: fmt.Sprintf("%s: Creates everything with ref subscriber no ns provided", name), + Key: testKey, + Objects: []runtime.Object{ + ReadyBroker(config), + makeSubscriberAddressableAsUnstructured(), + markReady(createQueue(config, false, "")), + markReady(createBinding(false, false, "")), + NewTrigger(triggerName, testNS, brokerName, + WithTriggerUID(triggerUID), + WithTriggerSubscriberRef(subscriberGVK, subscriberName, "")), + createSecret(rabbitURL), + createRabbitMQBrokerConfig(""), + createRabbitMQCluster(""), + }, + WantCreates: []runtime.Object{ + createDispatcherDeployment(false, ""), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: NewTrigger(triggerName, testNS, brokerName, + WithTriggerUID(triggerUID), + WithTriggerSubscriberRef(subscriberGVK, subscriberName, testNS), + WithTriggerBrokerReady(), + WithTriggerDeadLetterSinkNotConfigured(), + WithTriggerDependencyReady(), + WithTriggerSubscribed(), + WithTriggerSubscriberResolvedSucceeded(), + WithTriggerStatusSubscriberURI(subscriberURI)), + }}, }, { Name: fmt.Sprintf("%s: Fails to resolve ref", name), Key: testKey, @@ -1397,6 +1456,24 @@ func makeSubscriberAddressableAsUnstructured() *unstructured.Unstructured { } } +func makeSubscriberAddressableAsUnstructuredWithNamespace(ns string) *unstructured.Unstructured { + return &unstructured.Unstructured{ + Object: map[string]interface{}{ + "apiVersion": subscriberAPIVersion, + "kind": subscriberKind, + "metadata": map[string]interface{}{ + "namespace": ns, + "name": subscriberName, + }, + "status": map[string]interface{}{ + "address": map[string]interface{}{ + "url": subscriberURI, + }, + }, + }, + } +} + func makeSubscriberNotAddressableAsUnstructured() *unstructured.Unstructured { return &unstructured.Unstructured{ Object: map[string]interface{}{ From 8af3a2bfaf3805667493717c081bd520362f67f8 Mon Sep 17 00:00:00 2001 From: Gabriel Freites Date: Tue, 27 Jun 2023 22:17:03 +0200 Subject: [PATCH 2/2] set defaults is taking care so the namespace does not get empty, so this should be good enough to pass --- pkg/reconciler/trigger/trigger.go | 8 -------- 1 file changed, 8 deletions(-) diff --git a/pkg/reconciler/trigger/trigger.go b/pkg/reconciler/trigger/trigger.go index 45cc09bfda..59d361157e 100644 --- a/pkg/reconciler/trigger/trigger.go +++ b/pkg/reconciler/trigger/trigger.go @@ -269,14 +269,6 @@ func (r *Reconciler) ReconcileKind(ctx context.Context, t *eventingv1.Trigger) p logging.FromContext(ctx).Info("Reconciled rabbitmq binding", zap.Any("binding", binding)) t.Status.MarkDependencySucceeded() - if t.Spec.Subscriber.Ref != nil { - // To call URIFromDestination(dest apisv1alpha1.Destination, parent interface{}), dest.Ref must have a Namespace - if t.Spec.Subscriber.Ref.Namespace == "" { - // We will use the Namespace of Trigger as the Namespace of dest.Ref if one is not provided - t.Spec.Subscriber.Ref.Namespace = t.GetNamespace() - } - } - subscriberURI, err := r.uriResolver.URIFromDestinationV1(ctx, t.Spec.Subscriber, t) if err != nil { logging.FromContext(ctx).Error("Unable to get the Subscriber's URI", zap.Error(err))