diff --git a/config/core/configmaps/features.yaml b/config/core/configmaps/features.yaml index c2a5cdfb8a9..f8f632ce8a9 100644 --- a/config/core/configmaps/features.yaml +++ b/config/core/configmaps/features.yaml @@ -40,10 +40,6 @@ data: # For more details: https://github.com/knative/eventing/issues/5593 kreference-mapping: "disabled" - # BETA feature: The subscriber-strict flag force subscriptions to define a subscriber - # For more details: https://github.com/knative/eventing/issues/5756 - strict-subscriber: "enabled" - # ALPHA feature: The new-trigger-filters flag allows you to use the new `filters` field # in Trigger objects with its rich filtering capabilities. # For more details: https://github.com/knative/eventing/issues/5204 diff --git a/pkg/apis/feature/flag_names.go b/pkg/apis/feature/flag_names.go index e69d71959ac..65a78d06f5c 100644 --- a/pkg/apis/feature/flag_names.go +++ b/pkg/apis/feature/flag_names.go @@ -21,6 +21,5 @@ const ( DeliveryRetryAfter = "delivery-retryafter" DeliveryTimeout = "delivery-timeout" KReferenceMapping = "kreference-mapping" - StrictSubscriber = "strict-subscriber" NewTriggerFilters = "new-trigger-filters" ) diff --git a/pkg/apis/messaging/v1/subscription_validation.go b/pkg/apis/messaging/v1/subscription_validation.go index a29363caf03..d2e2b6627ab 100644 --- a/pkg/apis/messaging/v1/subscription_validation.go +++ b/pkg/apis/messaging/v1/subscription_validation.go @@ -19,8 +19,6 @@ package v1 import ( "context" - "knative.dev/eventing/pkg/apis/feature" - "github.com/google/go-cmp/cmp/cmpopts" "k8s.io/apimachinery/pkg/api/equality" "knative.dev/pkg/apis" @@ -50,31 +48,18 @@ func (ss *SubscriptionSpec) Validate(ctx context.Context) *apis.FieldError { errs = errs.Also(fe.ViaField("channel")) } - missingSubscriber := isDestinationNilOrEmpty(ss.Subscriber) - missingReply := isDestinationNilOrEmpty(ss.Reply) - - // Check if StrictSubscriber flag is enabled, if so we follow the spec and check for a valid reference to a subscriber - if missingSubscriber && feature.FromContext(ctx).IsEnabled(feature.StrictSubscriber) { + // Check if we follow the spec and have a valid reference to a subscriber + if isDestinationNilOrEmpty(ss.Subscriber) { fe := apis.ErrMissingField("subscriber") fe.Details = "the Subscription must reference a subscriber" errs = errs.Also(fe) - - } else { // if the flag is not set, we use pre 0.26 behavior - if missingSubscriber && missingReply { - fe := apis.ErrMissingField("reply", "subscriber") - fe.Details = "the Subscription must reference at least one of (reply or a subscriber)" - errs = errs.Also(fe) - } - - } - - if !missingSubscriber { + } else { if fe := ss.Subscriber.Validate(ctx); fe != nil { errs = errs.Also(fe.ViaField("subscriber")) } } - if !missingReply { + if !isDestinationNilOrEmpty(ss.Reply) { if fe := ss.Reply.Validate(ctx); fe != nil { errs = errs.Also(fe.ViaField("reply")) } diff --git a/pkg/apis/messaging/v1/subscription_validation_test.go b/pkg/apis/messaging/v1/subscription_validation_test.go index 89815024895..bf68a5d9ca1 100644 --- a/pkg/apis/messaging/v1/subscription_validation_test.go +++ b/pkg/apis/messaging/v1/subscription_validation_test.go @@ -162,8 +162,8 @@ func TestSubscriptionSpecValidation(t *testing.T) { Channel: getValidChannelRef(), }, want: func() *apis.FieldError { - fe := apis.ErrMissingField("reply", "subscriber") - fe.Details = "the Subscription must reference at least one of (reply or a subscriber)" + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" return fe }(), }, { @@ -174,8 +174,8 @@ func TestSubscriptionSpecValidation(t *testing.T) { Reply: &duckv1.Destination{}, }, want: func() *apis.FieldError { - fe := apis.ErrMissingField("reply", "subscriber") - fe.Details = "the Subscription must reference at least one of (reply or a subscriber)" + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" return fe }(), }, { @@ -192,7 +192,11 @@ func TestSubscriptionSpecValidation(t *testing.T) { Channel: getValidChannelRef(), Reply: getValidReply(), }, - want: nil, + want: func() *apis.FieldError { + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" + return fe + }(), }, { name: "empty Subscriber", c: &SubscriptionSpec{ @@ -200,9 +204,13 @@ func TestSubscriptionSpecValidation(t *testing.T) { Subscriber: &duckv1.Destination{}, Reply: getValidReply(), }, - want: nil, + want: func() *apis.FieldError { + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" + return fe + }(), }, { - name: "missing name in channel, and missing subscriber, reply", + name: "missing name in channel, and missing subscriber", c: &SubscriptionSpec{ Channel: duckv1.KReference{ Kind: channelKind, @@ -210,8 +218,8 @@ func TestSubscriptionSpecValidation(t *testing.T) { }, }, want: func() *apis.FieldError { - fe := apis.ErrMissingField("reply", "subscriber") - fe.Details = "the Subscription must reference at least one of (reply or a subscriber)" + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" return apis.ErrMissingField("channel.name").Also(fe) }(), }, { @@ -319,8 +327,8 @@ func TestSubscriptionSpecValidationWithKRefGroupFeatureEnabled(t *testing.T) { Channel: getValidChannelRef(), }, want: func() *apis.FieldError { - fe := apis.ErrMissingField("reply", "subscriber") - fe.Details = "the Subscription must reference at least one of (reply or a subscriber)" + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" return fe }(), }, { @@ -331,8 +339,8 @@ func TestSubscriptionSpecValidationWithKRefGroupFeatureEnabled(t *testing.T) { Reply: &duckv1.Destination{}, }, want: func() *apis.FieldError { - fe := apis.ErrMissingField("reply", "subscriber") - fe.Details = "the Subscription must reference at least one of (reply or a subscriber)" + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" return fe }(), }, { @@ -356,7 +364,11 @@ func TestSubscriptionSpecValidationWithKRefGroupFeatureEnabled(t *testing.T) { Channel: getValidChannelRef(), Reply: getValidReply(), }, - want: nil, + want: func() *apis.FieldError { + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" + return fe + }(), }, { name: "empty Subscriber", c: &SubscriptionSpec{ @@ -364,9 +376,13 @@ func TestSubscriptionSpecValidationWithKRefGroupFeatureEnabled(t *testing.T) { Subscriber: &duckv1.Destination{}, Reply: getValidReply(), }, - want: nil, + want: func() *apis.FieldError { + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" + return fe + }(), }, { - name: "missing name in channel, and missing subscriber, reply", + name: "missing name in channel, and missing subscriber", c: &SubscriptionSpec{ Channel: duckv1.KReference{ Kind: channelKind, @@ -374,8 +390,8 @@ func TestSubscriptionSpecValidationWithKRefGroupFeatureEnabled(t *testing.T) { }, }, want: func() *apis.FieldError { - fe := apis.ErrMissingField("reply", "subscriber") - fe.Details = "the Subscription must reference at least one of (reply or a subscriber)" + fe := apis.ErrMissingField("subscriber") + fe.Details = "the Subscription must reference a subscriber" return apis.ErrMissingField("channel.name").Also(fe) }(), }, { @@ -488,8 +504,9 @@ func TestSubscriptionImmutable(t *testing.T) { name: "valid, new Reply", c: &Subscription{ Spec: SubscriptionSpec{ - Channel: getValidChannelRef(), - Reply: getValidReply(), + Channel: getValidChannelRef(), + Subscriber: getValidDestination(), + Reply: getValidReply(), }, }, og: &Subscription{ @@ -499,21 +516,6 @@ func TestSubscriptionImmutable(t *testing.T) { }, }, want: nil, - }, { - name: "valid, have Reply, remove and replace with Subscriber", - c: &Subscription{ - Spec: SubscriptionSpec{ - Channel: getValidChannelRef(), - Reply: getValidReply(), - }, - }, - og: &Subscription{ - Spec: SubscriptionSpec{ - Channel: getValidChannelRef(), - Subscriber: getValidDestination(), - }, - }, - want: nil, }, { name: "valid, have Subscriber, remove and replace with Reply", c: &Subscription{ @@ -648,80 +650,3 @@ func TestValidChannel(t *testing.T) { }) } } - -func TestSubscriptionSpecValidationWithStrictSubscriber(t *testing.T) { - tests := []struct { - name string - c *SubscriptionSpec - want *apis.FieldError - }{{ - name: "missing Subscriber and Reply", - c: &SubscriptionSpec{ - Channel: getValidChannelRef(), - }, - want: func() *apis.FieldError { - fe := apis.ErrMissingField("subscriber") - fe.Details = "the Subscription must reference a subscriber" - return fe - }(), - }, { - name: "empty Subscriber and Reply", - c: &SubscriptionSpec{ - Channel: getValidChannelRef(), - Subscriber: &duckv1.Destination{}, - Reply: &duckv1.Destination{}, - }, - want: func() *apis.FieldError { - fe := apis.ErrMissingField("subscriber") - fe.Details = "the Subscription must reference a subscriber" - return fe - }(), - }, { - name: "missing Subscriber", - c: &SubscriptionSpec{ - Channel: getValidChannelRef(), - Reply: getValidReply(), - }, - want: func() *apis.FieldError { - fe := apis.ErrMissingField("subscriber") - fe.Details = "the Subscription must reference a subscriber" - return fe - }(), - }, { - name: "empty Subscriber", - c: &SubscriptionSpec{ - Channel: getValidChannelRef(), - Subscriber: &duckv1.Destination{}, - Reply: getValidReply(), - }, - want: func() *apis.FieldError { - fe := apis.ErrMissingField("subscriber") - fe.Details = "the Subscription must reference a subscriber" - return fe - }(), - }, { - name: "missing name in channel, and missing subscriber, reply", - c: &SubscriptionSpec{ - Channel: duckv1.KReference{ - Kind: channelKind, - APIVersion: channelAPIVersion, - }, - }, - want: func() *apis.FieldError { - fe := apis.ErrMissingField("subscriber") - fe.Details = "the Subscription must reference a subscriber" - return apis.ErrMissingField("channel.name").Also(fe) - }(), - }} - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - ctx := feature.ToContext(context.TODO(), feature.Flags{ - feature.StrictSubscriber: feature.Enabled, - }) - got := test.c.Validate(ctx) - if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" { - t.Errorf("%s: strictSubscriber (-want, +got) = %v", test.name, diff) - } - }) - } -} diff --git a/pkg/reconciler/subscription/subscription_test.go b/pkg/reconciler/subscription/subscription_test.go index 1f6d69c8b86..f5bf2e8b1e0 100644 --- a/pkg/reconciler/subscription/subscription_test.go +++ b/pkg/reconciler/subscription/subscription_test.go @@ -964,49 +964,11 @@ func TestAllCases(t *testing.T) { NewSubscription(subscriptionName, testNS, WithSubscriptionUID(subscriptionUID), WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(subscriberGVK, subscriberName, testNS), WithSubscriptionReply(imcV1GVK, replyName, testNS), ), - NewInMemoryChannel(channelName, testNS, - WithInitInMemoryChannelConditions, - WithInMemoryChannelAddress(channelDNS), - WithInMemoryChannelReadySubscriber(subscriptionUID), - ), - NewInMemoryChannel(replyName, testNS, - WithInitInMemoryChannelConditions, - WithInMemoryChannelAddress(replyDNS), - ), - }, - Key: testNS + "/" + subscriptionName, - WantErr: false, - WantEvents: []string{ - Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", subscriptionName), - Eventf(corev1.EventTypeNormal, "SubscriberSync", "Subscription was synchronized to channel %q", channelName), - }, - WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: NewSubscription(subscriptionName, testNS, - WithSubscriptionUID(subscriptionUID), - WithSubscriptionChannel(imcV1GVK, channelName), - WithSubscriptionReply(imcV1GVK, replyName, testNS), - // The first reconciliation will initialize the status conditions. - WithInitSubscriptionConditions, - MarkReferencesResolved, - MarkAddedToChannel, - WithSubscriptionPhysicalSubscriptionReply(replyURI), - ), - }}, - WantPatches: []clientgotesting.PatchActionImpl{ - patchSubscribers(testNS, channelName, []eventingduck.SubscriberSpec{ - {UID: subscriptionUID, ReplyURI: replyURI}, - }), - patchFinalizers(testNS, subscriptionName), - }, - }, { - Name: "v1 imc+reply - not deprecated", - Objects: []runtime.Object{ - NewSubscription(subscriptionName, testNS, - WithSubscriptionUID(subscriptionUID), - WithSubscriptionChannel(imcV1GVK, channelName), - WithSubscriptionReply(imcV1GVK, replyName, testNS), + NewUnstructured(subscriberGVK, subscriberName, testNS, + WithUnstructuredAddressable(subscriberDNS), ), NewInMemoryChannel(channelName, testNS, WithInitInMemoryChannelConditions, @@ -1028,17 +990,19 @@ func TestAllCases(t *testing.T) { Object: NewSubscription(subscriptionName, testNS, WithSubscriptionUID(subscriptionUID), WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(subscriberGVK, subscriberName, testNS), WithSubscriptionReply(imcV1GVK, replyName, testNS), // The first reconciliation will initialize the status conditions. WithInitSubscriptionConditions, MarkReferencesResolved, MarkAddedToChannel, WithSubscriptionPhysicalSubscriptionReply(replyURI), + WithSubscriptionPhysicalSubscriptionSubscriber(subscriberURI), ), }}, WantPatches: []clientgotesting.PatchActionImpl{ patchSubscribers(testNS, channelName, []eventingduck.SubscriberSpec{ - {UID: subscriptionUID, ReplyURI: replyURI}, + {UID: subscriptionUID, ReplyURI: replyURI, SubscriberURI: subscriberURI}, }), patchFinalizers(testNS, subscriptionName), }, @@ -1138,56 +1102,6 @@ func TestAllCases(t *testing.T) { {UID: subscriptionUID, Generation: subscriptionGeneration, SubscriberURI: subscriberURI}, }), }, - }, { - Name: "v1 imc+remove subscriber", - Objects: []runtime.Object{ - NewSubscription(subscriptionName, testNS, - WithSubscriptionUID(subscriptionUID), - WithSubscriptionGeneration(subscriptionGeneration), - WithSubscriptionChannel(imcV1GVK, channelName), - WithInitSubscriptionConditions, - WithSubscriptionReply(imcV1GVK, replyName, testNS), - MarkSubscriptionReady, - WithSubscriptionPhysicalSubscriptionSubscriber(subscriberURI), - WithSubscriptionPhysicalSubscriptionReply(replyURI), - ), - NewInMemoryChannel(channelName, testNS, - WithInitInMemoryChannelConditions, - WithInMemoryChannelAddress(channelDNS), - WithInMemoryChannelSubscribers([]eventingduck.SubscriberSpec{ - {UID: subscriptionUID, SubscriberURI: subscriberURI, ReplyURI: replyURI}, - }), - WithInMemoryChannelReadySubscriberAndGeneration(subscriptionUID, subscriptionGeneration), - ), - NewInMemoryChannel(replyName, testNS, - WithInitInMemoryChannelConditions, - WithInMemoryChannelAddress(replyDNS), - ), - }, - Key: testNS + "/" + subscriptionName, - WantErr: false, - WantEvents: []string{ - Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", subscriptionName), - Eventf(corev1.EventTypeNormal, "SubscriberSync", "Subscription was synchronized to channel %q", channelName), - }, - WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ - Object: NewSubscription(subscriptionName, testNS, - WithSubscriptionUID(subscriptionUID), - WithSubscriptionGeneration(subscriptionGeneration), - WithSubscriptionChannel(imcV1GVK, channelName), - WithSubscriptionReply(imcV1GVK, replyName, testNS), - WithInitSubscriptionConditions, - MarkSubscriptionReady, - WithSubscriptionPhysicalSubscriptionReply(replyURI), - WithSubscriptionStatusObservedGeneration(subscriptionGeneration), - ), - }}, - WantPatches: []clientgotesting.PatchActionImpl{ - patchSubscribers(testNS, channelName, []eventingduck.SubscriberSpec{ - {UID: subscriptionUID, Generation: subscriptionGeneration, ReplyURI: replyURI}, - }), - patchFinalizers(testNS, subscriptionName), - }, }, { Name: "v1 imc+subscriber as service", Objects: []runtime.Object{ diff --git a/test/experimental/config/features.yaml b/test/experimental/config/features.yaml index 1926ccfdd75..5048f96be6f 100644 --- a/test/experimental/config/features.yaml +++ b/test/experimental/config/features.yaml @@ -25,5 +25,4 @@ data: kreference-group: "enabled" delivery-retryafter: "enabled" delivery-timeout: "enabled" - strict-subscriber: "enabled" new-trigger-filters: "enabled"