From 27dbb99773a0db7860d14f34df65321e145aaf48 Mon Sep 17 00:00:00 2001 From: Knative Prow Robot Date: Mon, 4 Oct 2021 06:35:27 -0700 Subject: [PATCH] Validate DeliverySpec in Channels & Subscriptions (#5778) Co-authored-by: I867318 --- pkg/apis/messaging/v1/channel_validation.go | 7 +++ .../messaging/v1/channel_validation_test.go | 47 +++++++++++++- .../messaging/v1/subscription_validation.go | 6 ++ .../v1/subscription_validation_test.go | 61 +++++++++++++------ 4 files changed, 102 insertions(+), 19 deletions(-) diff --git a/pkg/apis/messaging/v1/channel_validation.go b/pkg/apis/messaging/v1/channel_validation.go index 8220d6515aa..5f0a10b4c07 100644 --- 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 index 6b78514fefe..35dbe93af44 100644 --- 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 index 93bcdf8c0ea..03365452963 100644 --- 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 index e2871f71a93..27c974b770e 100644 --- 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 {