Remove strict subscriber feature flag

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

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"
 )

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"))
 		}

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,26 +192,34 @@ 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{
 			Channel:    getValidChannelRef(),
 			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,
 				APIVersion: channelAPIVersion,
 			},
 		},
 		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,26 +364,34 @@ 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{
 			Channel:    getValidChannelRef(),
 			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,
 				APIVersion: channelAPIVersion,
 			},
 		},
 		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)
-			}
-		})
-	}
-}