From 9fb5f861d9ea2af5a03db0e074407936f6aba28c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christoph=20St=C3=A4bler?= Date: Tue, 24 Jan 2023 09:19:38 +0100 Subject: [PATCH] Populate a Subscriptions subscriber and reply namespace only if not set already (#6671) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #6539 In case a Subscription with a `.spec.subscriber.ref` pointing to a service in another namespace, the Subscription will not become ready because it is trying to find the service on the subscription's namespace (see #6539). Same happens with a subscriptions `.spec.reply.ref`. This PR addresses it and only uses the namespace of the Subscription, if no namespace was given for the subscriber/reply. ## Proposed Changes * :bug: Populate a subscriptions subscriber & reply namespace field with the Subscriptions namespace only in case it is not set ### Pre-review Checklist - [ ] **At least 80% unit test coverage** - [ ] **E2E tests** for any new behavior - [ ] **Docs PR** for any user-facing impact - [ ] **Spec PR** for any new API feature - [ ] **Conformance test** for any change to the spec **Release Note** ```release-note ``` **How to verify** 1. Install a eventing with this patch 2. Create a sequence with steps and a reply in different namespaces (e.g. https://gist.github.com/creydr/57235f35b03ab8f1285eee0a64350576) 3. Check that the subscriptions and sequence becomes ready Signed-off-by: Christoph Stäbler (cherry picked from commit bd674502e6b8753730c36cb594c946d4af25d718) --- pkg/reconciler/subscription/subscription.go | 8 +- .../subscription/subscription_test.go | 132 ++++++++++++++++++ 2 files changed, 136 insertions(+), 4 deletions(-) diff --git a/pkg/reconciler/subscription/subscription.go b/pkg/reconciler/subscription/subscription.go index e4ed3705dd0..bcbe662b096 100644 --- a/pkg/reconciler/subscription/subscription.go +++ b/pkg/reconciler/subscription/subscription.go @@ -196,8 +196,8 @@ func (r *Reconciler) resolveSubscriber(ctx context.Context, subscription *v1.Sub // Resolve Subscriber. subscriber := subscription.Spec.Subscriber.DeepCopy() if !isNilOrEmptyDestination(subscriber) { - // Populate the namespace for the subscriber since it is in the namespace - if subscriber.Ref != nil { + // Populate the namespace for the subscriber if required + if subscriber.Ref != nil && subscriber.Ref.Namespace == "" { subscriber.Ref.Namespace = subscription.Namespace } @@ -238,8 +238,8 @@ func (r *Reconciler) resolveReply(ctx context.Context, subscription *v1.Subscrip // Resolve Reply. reply := subscription.Spec.Reply.DeepCopy() if !isNilOrEmptyDestination(reply) { - // Populate the namespace for the subscriber since it is in the namespace - if reply.Ref != nil { + // Populate the namespace for the subscriber if required + if reply.Ref != nil && reply.Ref.Namespace == "" { reply.Ref.Namespace = subscription.Namespace } replyURI, err := r.destinationResolver.URIFromDestinationV1(ctx, *reply, subscription) diff --git a/pkg/reconciler/subscription/subscription_test.go b/pkg/reconciler/subscription/subscription_test.go index 1f6d69c8b86..566c7b42266 100644 --- a/pkg/reconciler/subscription/subscription_test.go +++ b/pkg/reconciler/subscription/subscription_test.go @@ -210,6 +210,138 @@ func TestAllCases(t *testing.T) { MarkSubscriptionReady, ), }}, + }, { + Name: "subscription goes ready with subscriber in different namespace", + Objects: []runtime.Object{ + NewSubscription(subscriptionName, testNS, + WithSubscriptionUID(subscriptionUID), + WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(subscriberGVK, subscriberName, testNS+"-2"), + WithSubscriptionReply(imcV1GVK, replyName, testNS), + WithInitSubscriptionConditions, + WithSubscriptionFinalizers(finalizerName), + MarkReferencesResolved, + MarkAddedToChannel, + WithSubscriptionPhysicalSubscriptionSubscriber(subscriberURI), + WithSubscriptionPhysicalSubscriptionReply(replyURI), + ), + // Subscriber + NewUnstructured(subscriberGVK, subscriberName, testNS+"-2", + WithUnstructuredAddressable(subscriberDNS), + ), + // Reply + NewInMemoryChannel(replyName, testNS, + WithInitInMemoryChannelConditions, + WithInMemoryChannelAddress(replyDNS), + ), + // Channel + NewInMemoryChannel(channelName, testNS, + WithInitInMemoryChannelConditions, + WithInMemoryChannelReady(channelDNS), + WithInMemoryChannelSubscribers([]eventingduck.SubscriberSpec{{ + UID: subscriptionUID, + Generation: 0, + SubscriberURI: subscriberURI, + ReplyURI: replyURI, + }, { + UID: "34c5aec8-deb6-11e8-9f32-f2801f1b9fd1", + Generation: 1, + SubscriberURI: apis.HTTP("call2"), + ReplyURI: apis.HTTP("sink2"), + }}), + WithInMemoryChannelStatusSubscribers([]eventingduck.SubscriberStatus{{ + UID: subscriptionUID, + ObservedGeneration: 0, + Ready: "True", + }, { + UID: "34c5aec8-deb6-11e8-9f32-f2801f1b9fd1", + ObservedGeneration: 1, + Ready: "True", + }}), + ), + }, + Key: testNS + "/" + subscriptionName, + WantErr: false, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: NewSubscription(subscriptionName, testNS, + WithSubscriptionUID(subscriptionUID), + WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(subscriberGVK, subscriberName, testNS+"-2"), + WithSubscriptionReply(imcV1GVK, replyName, testNS), + WithInitSubscriptionConditions, + WithSubscriptionFinalizers(finalizerName), + WithSubscriptionPhysicalSubscriptionSubscriber(subscriberURI), + WithSubscriptionPhysicalSubscriptionReply(replyURI), + // - Status Update - + MarkSubscriptionReady, + ), + }}, + }, { + Name: "subscription goes ready with reply in different namespace", + Objects: []runtime.Object{ + NewSubscription(subscriptionName, testNS, + WithSubscriptionUID(subscriptionUID), + WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(subscriberGVK, subscriberName, testNS), + WithSubscriptionReply(imcV1GVK, replyName, testNS+"-2"), + WithInitSubscriptionConditions, + WithSubscriptionFinalizers(finalizerName), + MarkReferencesResolved, + MarkAddedToChannel, + WithSubscriptionPhysicalSubscriptionSubscriber(subscriberURI), + WithSubscriptionPhysicalSubscriptionReply(replyURI), + ), + // Subscriber + NewUnstructured(subscriberGVK, subscriberName, testNS, + WithUnstructuredAddressable(subscriberDNS), + ), + // Reply + NewInMemoryChannel(replyName, testNS+"-2", + WithInitInMemoryChannelConditions, + WithInMemoryChannelAddress(replyDNS), + ), + // Channel + NewInMemoryChannel(channelName, testNS, + WithInitInMemoryChannelConditions, + WithInMemoryChannelReady(channelDNS), + WithInMemoryChannelSubscribers([]eventingduck.SubscriberSpec{{ + UID: subscriptionUID, + Generation: 0, + SubscriberURI: subscriberURI, + ReplyURI: replyURI, + }, { + UID: "34c5aec8-deb6-11e8-9f32-f2801f1b9fd1", + Generation: 1, + SubscriberURI: apis.HTTP("call2"), + ReplyURI: apis.HTTP("sink2"), + }}), + WithInMemoryChannelStatusSubscribers([]eventingduck.SubscriberStatus{{ + UID: subscriptionUID, + ObservedGeneration: 0, + Ready: "True", + }, { + UID: "34c5aec8-deb6-11e8-9f32-f2801f1b9fd1", + ObservedGeneration: 1, + Ready: "True", + }}), + ), + }, + Key: testNS + "/" + subscriptionName, + WantErr: false, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: NewSubscription(subscriptionName, testNS, + WithSubscriptionUID(subscriptionUID), + WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(subscriberGVK, subscriberName, testNS), + WithSubscriptionReply(imcV1GVK, replyName, testNS+"-2"), + WithInitSubscriptionConditions, + WithSubscriptionFinalizers(finalizerName), + WithSubscriptionPhysicalSubscriptionSubscriber(subscriberURI), + WithSubscriptionPhysicalSubscriptionReply(replyURI), + // - Status Update - + MarkSubscriptionReady, + ), + }}, }, { Name: "subscription goes ready without api version", Ctx: feature.ToContext(context.TODO(), feature.Flags{