Skip to content

Commit

Permalink
Validate negative retry config (#4216)
Browse files Browse the repository at this point in the history
A user can specify a negative number of retries without getting any
validation error. This PR adds a check to validate that
`deliverySpec.retry` is a non negative integer.

Signed-off-by: Pierangelo Di Pilato <pierangelodipilato@gmail.com>
  • Loading branch information
pierDipi authored Oct 5, 2020
1 parent 5fd9ba8 commit f0dc15b
Show file tree
Hide file tree
Showing 4 changed files with 38 additions and 0 deletions.
6 changes: 6 additions & 0 deletions pkg/apis/duck/v1/delivery_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,11 @@ func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError {
if dlse := ds.DeadLetterSink.Validate(ctx); dlse != nil {
errs = errs.Also(dlse).ViaField("deadLetterSink")
}

if ds.Retry != nil && *ds.Retry < 0 {
errs = errs.Also(apis.ErrInvalidValue(*ds.Retry, "retry"))
}

if ds.BackoffPolicy != nil {
switch *ds.BackoffPolicy {
case BackoffPolicyExponential, BackoffPolicyLinear:
Expand All @@ -68,6 +73,7 @@ func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError {
errs = errs.Also(apis.ErrInvalidValue(*ds.BackoffPolicy, "backoffPolicy"))
}
}

if ds.BackoffDelay != nil {
_, te := period.Parse(*ds.BackoffDelay)
if te != nil {
Expand Down
13 changes: 13 additions & 0 deletions pkg/apis/duck/v1/delivery_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
"k8s.io/utils/pointer"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
)
Expand Down Expand Up @@ -63,6 +64,18 @@ func TestDeliverySpecValidation(t *testing.T) {
want: func() *apis.FieldError {
return apis.ErrGeneric("invalid value: "+invalidBackoffDelay, "backoffDelay")
}(),
}, {
name: "negative retry",
spec: &DeliverySpec{Retry: pointer.Int32Ptr(-1)},
want: func() *apis.FieldError {
return apis.ErrGeneric("invalid value: -1", "retry")
}(),
}, {
name: "valid retry 0",
spec: &DeliverySpec{Retry: pointer.Int32Ptr(0)},
}, {
name: "valid retry 1",
spec: &DeliverySpec{Retry: pointer.Int32Ptr(1)},
}}

for _, test := range tests {
Expand Down
6 changes: 6 additions & 0 deletions pkg/apis/duck/v1beta1/delivery_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,11 @@ func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError {
if dlse := ds.DeadLetterSink.Validate(ctx); dlse != nil {
errs = errs.Also(dlse).ViaField("deadLetterSink")
}

if ds.Retry != nil && *ds.Retry < 0 {
errs = errs.Also(apis.ErrInvalidValue(*ds.Retry, "retry"))
}

if ds.BackoffPolicy != nil {
switch *ds.BackoffPolicy {
case BackoffPolicyExponential, BackoffPolicyLinear:
Expand All @@ -69,6 +74,7 @@ func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError {
errs = errs.Also(apis.ErrInvalidValue(*ds.BackoffPolicy, "backoffPolicy"))
}
}

if ds.BackoffDelay != nil {
_, te := period.Parse(*ds.BackoffDelay)
if te != nil {
Expand Down
13 changes: 13 additions & 0 deletions pkg/apis/duck/v1beta1/delivery_types_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"testing"

"github.com/google/go-cmp/cmp"
"k8s.io/utils/pointer"
"knative.dev/pkg/apis"
duckv1 "knative.dev/pkg/apis/duck/v1"
)
Expand Down Expand Up @@ -63,6 +64,18 @@ func TestDeliverySpecValidation(t *testing.T) {
want: func() *apis.FieldError {
return apis.ErrGeneric("invalid value: "+invalidBackoffDelay, "backoffDelay")
}(),
}, {
name: "negative retry",
spec: &DeliverySpec{Retry: pointer.Int32Ptr(-1)},
want: func() *apis.FieldError {
return apis.ErrGeneric("invalid value: -1", "retry")
}(),
}, {
name: "valid retry 0",
spec: &DeliverySpec{Retry: pointer.Int32Ptr(0)},
}, {
name: "valid retry 1",
spec: &DeliverySpec{Retry: pointer.Int32Ptr(1)},
}}

for _, test := range tests {
Expand Down

0 comments on commit f0dc15b

Please sign in to comment.