Validate DeliverySpec in Channels & Subscriptions (#5778)

Co-authored-by: I867318 <travis.minke@sap.com>
knative · Oct 4, 2021 · 27dbb99 · 27dbb99
1 parent f031ba2
commit 27dbb99
Show file tree

Hide file tree

Showing 4 changed files with 102 additions and 19 deletions.
diff --git a/pkg/apis/messaging/v1/channel_validation.go b/pkg/apis/messaging/v1/channel_validation.go
@@ -49,6 +49,13 @@ func (cs *ChannelSpec) Validate(ctx context.Context) *apis.FieldError {
 	if len(cs.SubscribableSpec.Subscribers) > 0 {
 		errs = errs.Also(apis.ErrDisallowedFields("subscribers").ViaField("subscribable"))
 	}
+
+	if cs.Delivery != nil {
+		if fe := cs.Delivery.Validate(ctx); fe != nil {
+			errs = errs.Also(fe.ViaField("delivery"))
+		}
+	}
+
 	return errs
 }
 

diff --git a/pkg/apis/messaging/v1/channel_validation_test.go b/pkg/apis/messaging/v1/channel_validation_test.go
@@ -23,9 +23,9 @@ import (
 	"github.com/google/go-cmp/cmp"
 	v1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"knative.dev/pkg/apis"
 
 	eventingduck "knative.dev/eventing/pkg/apis/duck/v1"
-	"knative.dev/pkg/apis"
 )
 
 func TestChannelValidation(t *testing.T) {
@@ -95,6 +95,51 @@ func TestChannelValidation(t *testing.T) {
 			errs = errs.Also(apis.ErrDisallowedFields("spec.subscribable.subscribers"))
 			return errs
 		}(),
+	}, {
+		name: "invalid Delivery",
+		cr: &Channel{
+			Spec: ChannelSpec{
+				ChannelTemplate: &ChannelTemplateSpec{
+					TypeMeta: v1.TypeMeta{
+						Kind:       "Channel",
+						APIVersion: SchemeGroupVersion.String(),
+					},
+				},
+				ChannelableSpec: eventingduck.ChannelableSpec{
+					Delivery: getDelivery(backoffDelayInvalid),
+				},
+			},
+		},
+		want: apis.ErrInvalidValue(backoffDelayInvalid, "spec.delivery.backoffDelay"),
+	}, {
+		name: "valid Delivery",
+		cr: &Channel{
+			Spec: ChannelSpec{
+				ChannelTemplate: &ChannelTemplateSpec{
+					TypeMeta: v1.TypeMeta{
+						Kind:       "Channel",
+						APIVersion: SchemeGroupVersion.String(),
+					},
+				},
+				ChannelableSpec: eventingduck.ChannelableSpec{
+					Delivery: getDelivery(backoffDelayValid),
+				},
+			},
+		},
+		want: nil,
+	}, {
+		name: "valid minimal spec",
+		cr: &Channel{
+			Spec: ChannelSpec{
+				ChannelTemplate: &ChannelTemplateSpec{
+					TypeMeta: v1.TypeMeta{
+						Kind:       "Channel",
+						APIVersion: SchemeGroupVersion.String(),
+					},
+				},
+			},
+		},
+		want: nil,
 	}}
 
 	doValidateTest(t, tests)

diff --git a/pkg/apis/messaging/v1/subscription_validation.go b/pkg/apis/messaging/v1/subscription_validation.go
@@ -68,6 +68,12 @@ func (ss *SubscriptionSpec) Validate(ctx context.Context) *apis.FieldError {
 		}
 	}
 
+	if ss.Delivery != nil {
+		if fe := ss.Delivery.Validate(ctx); fe != nil {
+			errs = errs.Also(fe.ViaField("delivery"))
+		}
+	}
+
 	return errs
 }
 

diff --git a/pkg/apis/messaging/v1/subscription_validation_test.go b/pkg/apis/messaging/v1/subscription_validation_test.go
@@ -20,20 +20,26 @@ import (
 	"testing"
 
 	"github.com/google/go-cmp/cmp"
-	"knative.dev/eventing/pkg/apis/feature"
 	"knative.dev/pkg/apis"
 	duckv1 "knative.dev/pkg/apis/duck/v1"
+
+	eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1"
+	"knative.dev/eventing/pkg/apis/feature"
 )
 
 const (
-	channelKind       = "MyChannel"
-	channelAPIVersion = "messaging.knative.dev/v1"
-	routeKind         = "Route"
-	routeAPIVersion   = "serving.knative.dev/v1"
-	channelName       = "subscribedChannel"
-	replyChannelName  = "toChannel"
-	subscriberName    = "subscriber"
-	namespace         = "namespace"
+	channelKind         = "MyChannel"
+	channelAPIVersion   = "messaging.knative.dev/v1"
+	routeKind           = "Route"
+	routeAPIVersion     = "serving.knative.dev/v1"
+	channelName         = "subscribedChannel"
+	replyChannelName    = "toChannel"
+	subscriberName      = "subscriber"
+	namespace           = "namespace"
+	retryCount          = int32(3)
+	backoffPolicy       = eventingduckv1.BackoffPolicyExponential
+	backoffDelayValid   = "PT0.5S"
+	backoffDelayInvalid = "invalid-delay"
 )
 
 func getValidChannelRef() duckv1.KReference {
@@ -66,6 +72,16 @@ func getValidDestination() *duckv1.Destination {
 	}
 }
 
+func getDelivery(delay string) *eventingduckv1.DeliverySpec {
+	retry := retryCount
+	policy := backoffPolicy
+	return &eventingduckv1.DeliverySpec{
+		Retry:         &retry,
+		BackoffPolicy: &policy,
+		BackoffDelay:  &delay,
+	}
+}
+
 func TestSubscriptionValidation(t *testing.T) {
 	name := "empty channel"
 	c := &Subscription{
@@ -94,7 +110,7 @@ func TestSubscriptionSpecValidation(t *testing.T) {
 		c    *SubscriptionSpec
 		want *apis.FieldError
 	}{{
-		name: "valid",
+		name: "valid minimal spec",
 		c: &SubscriptionSpec{
 			Channel:    getValidChannelRef(),
 			Subscriber: getValidDestination(),
@@ -108,6 +124,14 @@ func TestSubscriptionSpecValidation(t *testing.T) {
 			Reply:      getValidReply(),
 		},
 		want: nil,
+	}, {
+		name: "valid with delivery",
+		c: &SubscriptionSpec{
+			Channel:    getValidChannelRef(),
+			Subscriber: getValidDestination(),
+			Delivery:   getDelivery(backoffDelayValid),
+		},
+		want: nil,
 	}, {
 		name: "empty Channel",
 		c: &SubscriptionSpec{
@@ -153,13 +177,6 @@ func TestSubscriptionSpecValidation(t *testing.T) {
 			fe.Details = "the Subscription must reference at least one of (reply or a subscriber)"
 			return fe
 		}(),
-	}, {
-		name: "missing Reply",
-		c: &SubscriptionSpec{
-			Channel:    getValidChannelRef(),
-			Subscriber: getValidDestination(),
-		},
-		want: nil,
 	}, {
 		name: "empty Reply",
 		c: &SubscriptionSpec{
@@ -218,7 +235,7 @@ func TestSubscriptionSpecValidation(t *testing.T) {
 		},
 		want: apis.ErrMissingField("subscriber.ref.name"),
 	}, {
-		name: "missing name in Subscriber.Ref",
+		name: "empty name in Subscriber.Ref",
 		c: &SubscriptionSpec{
 			Channel:    getValidChannelRef(),
 			Subscriber: getValidDestination(),
@@ -232,6 +249,14 @@ func TestSubscriptionSpecValidation(t *testing.T) {
 			},
 		},
 		want: apis.ErrMissingField("reply.ref.name"),
+	}, {
+		name: "invalid Delivery",
+		c: &SubscriptionSpec{
+			Channel:    getValidChannelRef(),
+			Subscriber: getValidDestination(),
+			Delivery:   getDelivery(backoffDelayInvalid),
+		},
+		want: apis.ErrInvalidValue(backoffDelayInvalid, "delivery.backoffDelay"),
 	}}
 
 	for _, test := range tests {