From 91b37950df985d2eb21033494308911f0cbfc23c Mon Sep 17 00:00:00 2001 From: I867318 Date: Fri, 8 Oct 2021 12:30:15 -0600 Subject: [PATCH 01/15] Retry-After Experimental-Feature --- config/core/configmaps/features.yaml | 4 + docs/eventing-api.md | 79 +++++- pkg/apis/duck/v1/delivery_types.go | 44 +++- pkg/apis/duck/v1/delivery_types_test.go | 40 ++- pkg/apis/feature/flag_names.go | 7 +- pkg/kncloudevents/message_sender.go | 61 ++++- pkg/kncloudevents/message_sender_test.go | 234 ++++++++++++++++++ pkg/kncloudevents/retries.go | 17 ++ pkg/kncloudevents/retries_test.go | 90 ++++++- pkg/reconciler/subscription/subscription.go | 15 +- .../subscription/subscription_test.go | 67 +++-- 11 files changed, 630 insertions(+), 28 deletions(-) diff --git a/config/core/configmaps/features.yaml b/config/core/configmaps/features.yaml index cceaf3d16a4..61d3493caa6 100644 --- a/config/core/configmaps/features.yaml +++ b/config/core/configmaps/features.yaml @@ -28,6 +28,10 @@ data: # For more details: https://github.com/knative/eventing/issues/5086 kreference-group: "disabled" + # ALPHA feature: The delivery-retryafter allows you to use the RetryAfter field in DeliverySpec. + # For more details: https://github.com/knative/eventing/issues/TODO!!! + delivery-retryafter: "disabled" + # ALPHA feature: The delivery-timeout allows you to use the Timeout field in DeliverySpec. # For more details: https://github.com/knative/eventing/issues/5148 delivery-timeout: "disabled" diff --git a/docs/eventing-api.md b/docs/eventing-api.md index 8d3c6bc7178..9d5cfac6ee6 100644 --- a/docs/eventing-api.md +++ b/docs/eventing-api.md @@ -353,9 +353,11 @@ string (Optional)

Timeout is the timeout of each single request. The value must be greater than 0. More information on Duration format: + - https://www.iso.org/iso-8601-date-and-time-format.html - https://en.wikipedia.org/wiki/ISO_8601

-

Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/5148

+ +

Note: This API is EXPERIMENTAL and could be deprecated in the future. For more details see https://github.com/knative/eventing/issues/5148

@@ -389,6 +391,23 @@ More information on Duration format: For exponential policy, backoff delay is backoffDelay*2^.

+ + +retryAfter
+ +RetryAfter + + + +(Optional) +

RetryAfter contains configuration controlling the handling of +"Retry-After" +headers for responses with 429 (Too Many Requests) and 503 (Service Unavailable) +response codes.

+

Note: This API is EXPERIMENTAL and could be deprecated in the future. For more details see +https://github.com/knative/eventing/issues/TODO

+ +

DeliveryStatus @@ -420,6 +439,64 @@ knative.dev/pkg/apis.URL (Optional)

DeadLetterSink is a KReference that is the reference to the native, platform specific channel where failed events are sent to.

+ + + + +

RetryAfter +

+

+(Appears on:DeliverySpec) +

RetryAfter contains configuration controlling the handling of HTTP +"Retry-After" +headers for responses with 429 (Too Many Requests) and 503 +(Service Unavailable) response codes. If not provided, the default behavior is +to simply ignore "Retry-After" headers and to retry at the configured rate +specified in the DeliverySpec.

+

Note: This API is EXPERIMENTAL and could be deprecated in the future. For more details see +https://github.com/knative/eventing/issues/TODO

+ + + + + + + + + + + + + + + diff --git a/pkg/apis/duck/v1/delivery_types.go b/pkg/apis/duck/v1/delivery_types.go index dffd0397a33..17c81b16db2 100644 --- a/pkg/apis/duck/v1/delivery_types.go +++ b/pkg/apis/duck/v1/delivery_types.go @@ -20,9 +20,10 @@ import ( "context" "github.com/rickb777/date/period" - "knative.dev/eventing/pkg/apis/feature" "knative.dev/pkg/apis" duckv1 "knative.dev/pkg/apis/duck/v1" + + "knative.dev/eventing/pkg/apis/feature" ) // DeliverySpec contains the delivery options for event senders, @@ -60,6 +61,33 @@ type DeliverySpec struct { // For exponential policy, backoff delay is backoffDelay*2^. // +optional BackoffDelay *string `json:"backoffDelay,omitempty"` + + // RetryAfter controls how "Retry-After" header durations are handled for 429 and 503 response codes. + // If not provided, the default behavior is to ignore "Retry-After" headers in responses. + // + // Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/TODO + // +optional + RetryAfter *RetryAfter `json:"retryAfter,omitempty"` +} + +// RetryAfter contains configuration related to the handling of "Retry-After" headers. +type RetryAfter struct { + // Enabled is a flag indicating whether to respect the "Retry-After" header duration. + // If enabled, the largest of the normal backoff duration and the "Retry-After" + // header value will be used when calculating the next backoff duration. This will + // only be considered when a 429 (Too Many Requests) or 503 (Service Unavailable) + // response code is received and Retry is greater than 0. + Enabled bool `json:"enabled"` + + // MaxDuration is the maximum time to wait before retrying. It is intended as an + // override to protect against excessively large "Retry-After" durations. If provided, + // the value must be greater than 0. If not provided, the largest of the "Retry-After" + // duration and the normal backoff duration will be used. + // More information on Duration format: + // - https://www.iso.org/iso-8601-date-and-time-format.html + // - https://en.wikipedia.org/wiki/ISO_8601 + // +optional + MaxDuration *string `json:"maxDuration,omitempty"` } func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError { @@ -101,6 +129,20 @@ func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError { errs = errs.Also(apis.ErrInvalidValue(*ds.BackoffDelay, "backoffDelay")) } } + + if ds.RetryAfter != nil { + if feature.FromContext(ctx).IsEnabled(feature.DeliveryRetryAfter) { + if ds.RetryAfter.MaxDuration != nil && *ds.RetryAfter.MaxDuration != "" { + m, me := period.Parse(*ds.RetryAfter.MaxDuration) + if me != nil || m.IsZero() { + errs = errs.Also(apis.ErrInvalidValue(*ds.RetryAfter.MaxDuration, "retryAfter.maxDuration")) + } + } + } else { + errs = errs.Also(apis.ErrDisallowedFields("retryAfter")) + } + } + return errs } diff --git a/pkg/apis/duck/v1/delivery_types_test.go b/pkg/apis/duck/v1/delivery_types_test.go index ca82ed2b15b..4fe8e912a32 100644 --- a/pkg/apis/duck/v1/delivery_types_test.go +++ b/pkg/apis/duck/v1/delivery_types_test.go @@ -21,15 +21,21 @@ import ( "github.com/google/go-cmp/cmp" "k8s.io/utils/pointer" - "knative.dev/eventing/pkg/apis/feature" "knative.dev/pkg/apis" duckv1 "knative.dev/pkg/apis/duck/v1" + + "knative.dev/eventing/pkg/apis/feature" ) +// TODO - add test cases for RetryAfter + func TestDeliverySpecValidation(t *testing.T) { deliveryTimeoutEnabledCtx := feature.ToContext(context.TODO(), feature.Flags{ feature.DeliveryTimeout: feature.Enabled, }) + deliveryRetryAfterEnabledCtx := feature.ToContext(context.TODO(), feature.Flags{ + feature.DeliveryRetryAfter: feature.Enabled, + }) invalidString := "invalid time" bop := BackoffPolicyExponential @@ -102,9 +108,41 @@ func TestDeliverySpecValidation(t *testing.T) { }, { name: "valid retry 0", spec: &DeliverySpec{Retry: pointer.Int32Ptr(0)}, + want: nil, }, { name: "valid retry 1", spec: &DeliverySpec{Retry: pointer.Int32Ptr(1)}, + want: nil, + }, { + name: "valid complete retryAfter", + ctx: deliveryRetryAfterEnabledCtx, + spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true, MaxDuration: &validDuration}}, + want: nil, + }, { + name: "valid sparse retryAfter", + ctx: deliveryRetryAfterEnabledCtx, + spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true}}, + want: nil, + }, { + name: "zero retryAfter.MaxDuration", + ctx: deliveryRetryAfterEnabledCtx, + spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true, MaxDuration: pointer.StringPtr("PT0S")}}, + want: func() *apis.FieldError { + return apis.ErrInvalidValue("PT0S", "retryAfter.maxDuration") + }(), + }, { + name: "invalid retryAfter.MaxDuration", + ctx: deliveryRetryAfterEnabledCtx, + spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true, MaxDuration: &invalidDuration}}, + want: func() *apis.FieldError { + return apis.ErrInvalidValue(invalidDuration, "retryAfter.maxDuration") + }(), + }, { + name: "disabled retryAfter", + spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true}}, + want: func() *apis.FieldError { + return apis.ErrDisallowedFields("retryAfter") + }(), }} for _, test := range tests { diff --git a/pkg/apis/feature/flag_names.go b/pkg/apis/feature/flag_names.go index 1595c8c049d..c5d4baf4d4a 100644 --- a/pkg/apis/feature/flag_names.go +++ b/pkg/apis/feature/flag_names.go @@ -17,7 +17,8 @@ limitations under the License. package feature const ( - KReferenceGroup = "kreference-group" - DeliveryTimeout = "delivery-timeout" - KReferenceMapping = "kreference-mapping" + KReferenceGroup = "kreference-group" + DeliveryRetryAfter = "delivery-retryafter" + DeliveryTimeout = "delivery-timeout" + KReferenceMapping = "kreference-mapping" ) diff --git a/pkg/kncloudevents/message_sender.go b/pkg/kncloudevents/message_sender.go index 6d223f7da40..50715d64a47 100644 --- a/pkg/kncloudevents/message_sender.go +++ b/pkg/kncloudevents/message_sender.go @@ -18,12 +18,16 @@ package kncloudevents import ( "context" + "fmt" nethttp "net/http" + "strconv" "time" "github.com/hashicorp/go-retryablehttp" ) +const RetryAfterHeader = "Retry-After" + // HTTPMessageSender is a wrapper for an http client that can send cloudevents.Request with retries type HTTPMessageSender struct { Client *nethttp.Client @@ -73,9 +77,7 @@ func (s *HTTPMessageSender) SendWithRetries(req *nethttp.Request, config *RetryC RetryWaitMax: defaultRetryWaitMax, RetryMax: config.RetryMax, CheckRetry: retryablehttp.CheckRetry(config.CheckRetry), - Backoff: func(_, _ time.Duration, attemptNum int, resp *nethttp.Response) time.Duration { - return config.Backoff(attemptNum, resp) - }, + Backoff: generateBackoffFn(config), ErrorHandler: func(resp *nethttp.Response, err error, numTries int) (*nethttp.Response, error) { return resp, err }, @@ -88,3 +90,56 @@ func (s *HTTPMessageSender) SendWithRetries(req *nethttp.Request, config *RetryC return retryableClient.Do(retryableReq) } + +// generateBackoffFunction returns a valid retryablehttp.Backoff implementation which +// wraps the provided RetryConfig.Backoff implementation with optional "Retry-After" +// header support. +func generateBackoffFn(config *RetryConfig) retryablehttp.Backoff { + return func(_, _ time.Duration, attemptNum int, resp *nethttp.Response) time.Duration { + + // If Response is 429 or 503 Then Parse Any Retry-After Header Durations & Enforce Optional MaxDuration + var retryAfterDuration time.Duration + if resp != nil && (resp.StatusCode == nethttp.StatusTooManyRequests || resp.StatusCode == nethttp.StatusServiceUnavailable) { + if config.RetryAfterEnabled { + retryAfterDuration, _ = parseRetryAfterDuration(resp) + if config.RetryAfterMaxDuration > 0 && config.RetryAfterMaxDuration < retryAfterDuration { + retryAfterDuration = config.RetryAfterMaxDuration + } + } + } + + // Calculate The RetryConfig Backoff Duration + backoffDuration := config.Backoff(attemptNum, resp) + + // Return The Larger Of The Two Backoff Durations + if retryAfterDuration > backoffDuration { + return retryAfterDuration + } else { + return backoffDuration + } + } +} + +// parseRetryAfterDuration returns a Duration expressing the amount of time +// requested to wait by a Retry-After header, or none if not present or invalid. +// According to the spec (https://tools.ietf.org/html/rfc7231#section-7.1.3) +// the Retry-After Header's value can be one of an HTTP-date or delay-seconds, +// both of which are supported here. +func parseRetryAfterDuration(resp *nethttp.Response) (time.Duration, error) { + var retryAfterDuration time.Duration + if resp != nil && resp.Header != nil { + retryAfterString := resp.Header.Get(RetryAfterHeader) + if len(retryAfterString) > 0 { + if retryAfterInt, parseIntErr := strconv.ParseInt(retryAfterString, 10, 64); parseIntErr == nil { + retryAfterDuration = time.Duration(retryAfterInt) * time.Second + } else { + retryAfterTime, parseTimeErr := nethttp.ParseTime(retryAfterString) // Supports http.TimeFormat, time.RFC850 & time.ANSIC + if parseTimeErr != nil { + return retryAfterDuration, fmt.Errorf("failed to parse Retry-After header: ParseInt Error = %v, ParseTime Error = %v", parseIntErr, parseTimeErr) + } + retryAfterDuration = time.Until(retryAfterTime) + } + } + } + return retryAfterDuration, nil +} diff --git a/pkg/kncloudevents/message_sender_test.go b/pkg/kncloudevents/message_sender_test.go index 60d572c28cf..5692c04aa70 100644 --- a/pkg/kncloudevents/message_sender_test.go +++ b/pkg/kncloudevents/message_sender_test.go @@ -22,6 +22,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "strconv" "sync/atomic" "testing" "time" @@ -33,6 +34,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "k8s.io/utils/pointer" + v1 "knative.dev/eventing/pkg/apis/duck/v1" ) @@ -194,6 +196,238 @@ func TestHTTPMessageSenderSendWithRetriesWithSingleRequestTimeout(t *testing.T) require.Equal(t, http.StatusOK, got.StatusCode) } +func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { + t.Parallel() + + // Create A Quick Enum For RetryAfter Format + type RetryAfterFormat int + const ( + None RetryAfterFormat = iota + Seconds + Date + Invalid + ) + + // Define The TestCases + tests := []struct { + name string + config *RetryConfig // The RetryConfig to use in for the test case. + statusCode int // The HTTP StatusCode which the Server should return. + responseFormat RetryAfterFormat // Indicates the format in which the Server should return the Retry-After header. + responseSeconds int // Indicates the number of Seconds for the Server to use when returning the Retry-After header. + wantReqCount int // The total number of expected requests, including initial request. + }{{ + name: "disabled 429 without Retry-After", + config: &RetryConfig{ + RetryMax: 2, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: None, + wantReqCount: 3, + }, { + name: "disabled 429 with Retry-After seconds", + config: &RetryConfig{ + RetryMax: 2, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Seconds, + responseSeconds: 1, + wantReqCount: 3, + }, { + name: "disabled 429 with Retry-After date", + config: &RetryConfig{ + RetryMax: 2, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Date, + responseSeconds: 2, + wantReqCount: 3, + }, { + name: "enabled 429 without Retry-After", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterEnabled: true, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: None, + wantReqCount: 3, + }, { + name: "enabled 429 with Retry-After seconds", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterEnabled: true, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Seconds, + responseSeconds: 1, + wantReqCount: 3, + }, { + name: "enabled 429 with Retry-After seconds max override", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterEnabled: true, + RetryAfterMaxDuration: 1 * time.Second, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Seconds, + responseSeconds: 4, + wantReqCount: 3, + }, { + name: "enabled 429 with Retry-After date", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterEnabled: true, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Date, + responseSeconds: 2, + wantReqCount: 3, + }, { + name: "enabled 429 with Retry-After date max override", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterEnabled: true, + RetryAfterMaxDuration: 1 * time.Second, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Date, + responseSeconds: 4, + wantReqCount: 3, + }, { + name: "enabled 429 with invalid Retry-After", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterEnabled: true, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Invalid, + wantReqCount: 3, + }, { + name: "enabled 503 with Retry-After seconds", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterEnabled: true, + }, + statusCode: http.StatusServiceUnavailable, + responseFormat: Seconds, + responseSeconds: 1, + wantReqCount: 3, + }, { + name: "enabled 503 with Retry-After date", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterEnabled: true, + }, + statusCode: http.StatusServiceUnavailable, + responseFormat: Date, + responseSeconds: 2, + wantReqCount: 3, + }, { + name: "enabled 503 with invalid Retry-After", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterEnabled: true, + }, + statusCode: http.StatusServiceUnavailable, + responseFormat: Invalid, + wantReqCount: 3, + }} + + // Execute The TestCases + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + + // Consistent CheckRetry & Backoff Implementation For All TestCases + backoffDuration := 100 * time.Millisecond + tc.config.CheckRetry = SelectiveRetry + tc.config.Backoff = func(attemptNum int, resp *http.Response) time.Duration { return backoffDuration } + + // Determine The Response Retry-After Backoff Duration From TestCase + var responseDuration time.Duration + switch tc.responseFormat { + case None, Invalid: + case Seconds, Date: + if tc.responseSeconds > 0 { + responseDuration = time.Duration(tc.responseSeconds) * time.Second + } + default: + assert.Fail(t, "TestCase with unsupported ResponseFormat '%v'", tc.responseFormat) + } + + // Tracking Variables + var previousReqTime time.Time + var reqCount int32 + + // Create A Test HTTP Server Capable Of Validating Request Interim Durations + server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + + // Determine Time Of Current Request + currentReqTime := time.Now() + if tc.config.RetryAfterEnabled && tc.responseFormat == Date && tc.responseSeconds > 0 { + currentReqTime = currentReqTime.Round(time.Second) // Round When Using Date Format To Account For RFC850 Precision + } + + // Count The Requests + atomic.AddInt32(&reqCount, 1) + + // Add Retry-After Header To Response Based On TestCase + switch tc.responseFormat { + case None: + // Don't Write Anything ; ) + case Seconds: + writer.Header().Set(RetryAfterHeader, strconv.Itoa(int(responseDuration.Seconds()))) + case Date: + writer.Header().Set(RetryAfterHeader, currentReqTime.Add(responseDuration).Format(time.RFC850)) // RFC850 Drops Millis + case Invalid: + writer.Header().Set(RetryAfterHeader, "FOO") + default: + assert.Fail(t, "TestCase with unsupported ResponseFormat '%v'", tc.responseFormat) + } + + // Calculate The Expected Request Duration Based On TestCase + expectedRequestDuration := backoffDuration + if tc.config.RetryAfterEnabled && responseDuration > 0 { + expectedRequestDuration = responseDuration + if tc.config.RetryAfterMaxDuration > 0 && tc.config.RetryAfterMaxDuration < expectedRequestDuration { + expectedRequestDuration = tc.config.RetryAfterMaxDuration + } + } + + // Validate Inter-Request Durations Meet Or Exceed Expected Minimums + actualRequestDuration := currentReqTime.Sub(previousReqTime) + assert.GreaterOrEqual(t, actualRequestDuration, expectedRequestDuration, "previousReqTime=%s, currentReqTime=%s", previousReqTime.String(), currentReqTime.String()) + + // Respond With StatusCode From TestCase & Cycle The Times + writer.WriteHeader(tc.statusCode) + previousReqTime = currentReqTime + })) + + // Initialize The Previous Request Time Back Far Enough For Consistent Time Comparisons + previousReqTime = time.Now().Add(-2 * backoffDuration) + if responseDuration > 0 { + previousReqTime = previousReqTime.Add(-2 * responseDuration) + } + + // Perform The Test - Generate And Send The Request + sender := &HTTPMessageSender{Client: http.DefaultClient} + request, err := http.NewRequest("POST", server.URL, nil) + assert.Nil(t, err) + got, err := sender.SendWithRetries(request, tc.config) + + // Verify Final Results + if err != nil { + t.Fatalf("SendWithRetries() error = %v, wantErr nil", err) + } + if got.StatusCode != tc.statusCode { + t.Fatalf("SendWithRetries() got = %v, want %v", got.StatusCode, tc.statusCode) + } + if int(atomic.LoadInt32(&reqCount)) != tc.wantReqCount { + t.Fatalf("expected %d retries got %d", tc.config.RetryMax, reqCount) + } + }) + } +} + func TestRetriesOnNetworkErrors(t *testing.T) { n := int32(10) diff --git a/pkg/kncloudevents/retries.go b/pkg/kncloudevents/retries.go index ee816efa16a..9793a8a76be 100644 --- a/pkg/kncloudevents/retries.go +++ b/pkg/kncloudevents/retries.go @@ -24,6 +24,7 @@ import ( "time" "github.com/rickb777/date/period" + v1 "knative.dev/eventing/pkg/apis/duck/v1" ) @@ -66,12 +67,17 @@ type RetryConfig struct { // RequestTimeout represents the timeout of the single request RequestTimeout time.Duration + + // Specify whether to handle "Retry-After" headers in 429/503 responses. + RetryAfterEnabled bool + RetryAfterMaxDuration time.Duration } func NoRetries() RetryConfig { return noRetries } +// RetryConfigFromDeliverySpec returns a RetryConfig based on the specified DeliverySpec. func RetryConfigFromDeliverySpec(spec v1.DeliverySpec) (RetryConfig, error) { retryConfig := NoRetries() @@ -112,6 +118,17 @@ func RetryConfigFromDeliverySpec(spec v1.DeliverySpec) (RetryConfig, error) { retryConfig.RequestTimeout, _ = timeout.Duration() } + if spec.RetryAfter != nil { + retryConfig.RetryAfterEnabled = spec.RetryAfter.Enabled + if spec.RetryAfter.MaxDuration != nil && *spec.RetryAfter.MaxDuration != "" { + maxPeriod, err := period.Parse(*spec.RetryAfter.MaxDuration) + if err != nil { + return retryConfig, fmt.Errorf("failed to parse Spec.RetryAfter.MaxDuration: %w", err) + } + retryConfig.RetryAfterMaxDuration, _ = maxPeriod.Duration() + } + } + return retryConfig, nil } diff --git a/pkg/kncloudevents/retries_test.go b/pkg/kncloudevents/retries_test.go index 6c2e493829c..397b4d79ded 100644 --- a/pkg/kncloudevents/retries_test.go +++ b/pkg/kncloudevents/retries_test.go @@ -23,19 +23,40 @@ import ( "testing" "time" + "github.com/rickb777/date/period" "github.com/stretchr/testify/assert" "k8s.io/utils/pointer" - v1 "knative.dev/eventing/pkg/apis/duck/v1" "knative.dev/pkg/ptr" + + v1 "knative.dev/eventing/pkg/apis/duck/v1" ) +// Test The NoRetries() Functionality +func TestNoRetries(t *testing.T) { + retryConfig := NoRetries() + assert.NotNil(t, retryConfig) + assert.Equal(t, 0, retryConfig.RetryMax) + assert.NotNil(t, retryConfig.CheckRetry) + result, err := retryConfig.CheckRetry(context.TODO(), nil, nil) + assert.False(t, result) + assert.Nil(t, err) + assert.NotNil(t, retryConfig.Backoff) + assert.Equal(t, time.Duration(0), retryConfig.Backoff(1, nil)) + assert.Equal(t, time.Duration(0), retryConfig.Backoff(100, nil)) +} + // Test The RetryConfigFromDeliverySpec() Functionality func TestRetryConfigFromDeliverySpec(t *testing.T) { const retry = 5 + validISO8601DurationString := "PT30S" + invalidISO8601DurationString := "FOO" + testcases := []struct { name string backoffPolicy v1.BackoffPolicyType backoffDelay string + timeout *string + retryAfter *v1.RetryAfter expectedBackoffDurations []time.Duration wantErr bool }{{ @@ -76,16 +97,67 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { backoffPolicy: v1.BackoffPolicyLinear, backoffDelay: "FOO", wantErr: true, + }, { + name: "With Valid Timeout", + backoffPolicy: v1.BackoffPolicyExponential, + backoffDelay: "PT0.5S", + timeout: &validISO8601DurationString, + expectedBackoffDurations: []time.Duration{ + 1 * time.Second, + 2 * time.Second, + 4 * time.Second, + 8 * time.Second, + 16 * time.Second, + }, + }, { + name: "With Invalid Timeout", + backoffPolicy: v1.BackoffPolicyExponential, + backoffDelay: "PT0.5S", + timeout: &invalidISO8601DurationString, + wantErr: true, + }, { + name: "With Valid Sparse Retry-After", + backoffPolicy: v1.BackoffPolicyExponential, + backoffDelay: "PT0.5S", + retryAfter: &v1.RetryAfter{Enabled: true}, + expectedBackoffDurations: []time.Duration{ + 1 * time.Second, + 2 * time.Second, + 4 * time.Second, + 8 * time.Second, + 16 * time.Second, + }, + }, { + name: "With Valid Complete Retry-After", + backoffPolicy: v1.BackoffPolicyExponential, + backoffDelay: "PT0.5S", + retryAfter: &v1.RetryAfter{Enabled: true, MaxDuration: &validISO8601DurationString}, + expectedBackoffDurations: []time.Duration{ + 1 * time.Second, + 2 * time.Second, + 4 * time.Second, + 8 * time.Second, + 16 * time.Second, + }, + }, { + name: "With Invalid Complete Retry-After", + backoffPolicy: v1.BackoffPolicyExponential, + backoffDelay: "PT0.5S", + retryAfter: &v1.RetryAfter{Enabled: true, MaxDuration: &invalidISO8601DurationString}, + wantErr: true, }} for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { + // Create The DeliverySpec To Test deliverySpec := v1.DeliverySpec{ DeadLetterSink: nil, Retry: ptr.Int32(retry), BackoffPolicy: &tc.backoffPolicy, BackoffDelay: &tc.backoffDelay, + Timeout: tc.timeout, + RetryAfter: tc.retryAfter, } // Create the RetryConfig from the deliverySpec @@ -95,6 +167,22 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { // If successful then validate the retryConfig (Max & Backoff calculations). if err == nil { assert.Equal(t, retry, retryConfig.RetryMax) + if tc.timeout != nil && *tc.timeout != "" { + expectedTimeoutPeriod, _ := period.Parse(*tc.timeout) + expectedTimeoutDuration, _ := expectedTimeoutPeriod.Duration() + assert.Equal(t, expectedTimeoutDuration, retryConfig.RequestTimeout) + } + if tc.retryAfter != nil { + assert.Equal(t, tc.retryAfter.Enabled, retryConfig.RetryAfterEnabled) + if tc.retryAfter.MaxDuration != nil && *tc.retryAfter.MaxDuration != "" { + expectedMaxPeriod, _ := period.Parse(*tc.retryAfter.MaxDuration) + expectedMaxDuration, _ := expectedMaxPeriod.Duration() + assert.Equal(t, expectedMaxDuration, retryConfig.RetryAfterMaxDuration) + } + } else { + assert.False(t, retryConfig.RetryAfterEnabled) + assert.Equal(t, time.Duration(0), retryConfig.RetryAfterMaxDuration) + } for i := 1; i < retry; i++ { expectedBackoffDuration := tc.expectedBackoffDurations[i-1] actualBackoffDuration := retryConfig.Backoff(i, nil) diff --git a/pkg/reconciler/subscription/subscription.go b/pkg/reconciler/subscription/subscription.go index 08ffd5a2d69..433a1faab51 100644 --- a/pkg/reconciler/subscription/subscription.go +++ b/pkg/reconciler/subscription/subscription.go @@ -510,7 +510,11 @@ func deliverySpec(sub *v1.Subscription, channel *eventingduckv1.Channelable) (de }, } } - if channel.Spec.Delivery.BackoffDelay != nil || channel.Spec.Delivery.Retry != nil || channel.Spec.Delivery.BackoffPolicy != nil || channel.Spec.Delivery.Timeout != nil { + if channel.Spec.Delivery.BackoffDelay != nil || + channel.Spec.Delivery.Retry != nil || + channel.Spec.Delivery.BackoffPolicy != nil || + channel.Spec.Delivery.Timeout != nil || + channel.Spec.Delivery.RetryAfter != nil { if delivery == nil { delivery = &eventingduckv1.DeliverySpec{} } @@ -518,6 +522,7 @@ func deliverySpec(sub *v1.Subscription, channel *eventingduckv1.Channelable) (de delivery.Retry = channel.Spec.Delivery.Retry delivery.BackoffDelay = channel.Spec.Delivery.BackoffDelay delivery.Timeout = channel.Spec.Delivery.Timeout + delivery.RetryAfter = channel.Spec.Delivery.RetryAfter } return } @@ -531,7 +536,12 @@ func deliverySpec(sub *v1.Subscription, channel *eventingduckv1.Channelable) (de }, } } - if sub.Spec.Delivery != nil && (sub.Spec.Delivery.BackoffDelay != nil || sub.Spec.Delivery.Retry != nil || sub.Spec.Delivery.BackoffPolicy != nil || sub.Spec.Delivery.Timeout != nil) { + if sub.Spec.Delivery != nil && + (sub.Spec.Delivery.BackoffDelay != nil || + sub.Spec.Delivery.Retry != nil || + sub.Spec.Delivery.BackoffPolicy != nil || + sub.Spec.Delivery.Timeout != nil || + sub.Spec.Delivery.RetryAfter != nil) { if delivery == nil { delivery = &eventingduckv1.DeliverySpec{} } @@ -539,6 +549,7 @@ func deliverySpec(sub *v1.Subscription, channel *eventingduckv1.Channelable) (de delivery.Retry = sub.Spec.Delivery.Retry delivery.BackoffDelay = sub.Spec.Delivery.BackoffDelay delivery.Timeout = sub.Spec.Delivery.Timeout + delivery.RetryAfter = sub.Spec.Delivery.RetryAfter } return } diff --git a/pkg/reconciler/subscription/subscription_test.go b/pkg/reconciler/subscription/subscription_test.go index 9b0df9c0c31..e544a7f8ff6 100644 --- a/pkg/reconciler/subscription/subscription_test.go +++ b/pkg/reconciler/subscription/subscription_test.go @@ -22,40 +22,37 @@ import ( "fmt" "testing" - apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" - "k8s.io/utils/pointer" - "knative.dev/eventing/pkg/apis/feature" - "knative.dev/pkg/injection/clients/dynamicclient" - "knative.dev/pkg/kref" - "knative.dev/pkg/network" - "knative.dev/pkg/tracker" - - eventingclient "knative.dev/eventing/pkg/client/injection/client" - "knative.dev/eventing/pkg/client/injection/ducks/duck/v1/channelable" - corev1 "k8s.io/api/core/v1" + apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" clientgotesting "k8s.io/client-go/testing" + "k8s.io/utils/pointer" + "knative.dev/pkg/apis" duckv1 "knative.dev/pkg/apis/duck/v1" "knative.dev/pkg/client/injection/ducks/duck/v1/addressable" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" + "knative.dev/pkg/injection/clients/dynamicclient" + "knative.dev/pkg/kref" logtesting "knative.dev/pkg/logging/testing" + "knative.dev/pkg/network" + . "knative.dev/pkg/reconciler/testing" "knative.dev/pkg/resolver" + "knative.dev/pkg/tracker" eventingduck "knative.dev/eventing/pkg/apis/duck/v1" + "knative.dev/eventing/pkg/apis/feature" messagingv1 "knative.dev/eventing/pkg/apis/messaging/v1" + eventingclient "knative.dev/eventing/pkg/client/injection/client" + "knative.dev/eventing/pkg/client/injection/ducks/duck/v1/channelable" + _ "knative.dev/eventing/pkg/client/injection/informers/messaging/v1/channel/fake" + _ "knative.dev/eventing/pkg/client/injection/informers/messaging/v1/inmemorychannel/fake" "knative.dev/eventing/pkg/client/injection/reconciler/messaging/v1/subscription" "knative.dev/eventing/pkg/duck" eventingtesting "knative.dev/eventing/pkg/reconciler/testing" - - . "knative.dev/pkg/reconciler/testing" - - _ "knative.dev/eventing/pkg/client/injection/informers/messaging/v1/channel/fake" - _ "knative.dev/eventing/pkg/client/injection/informers/messaging/v1/inmemorychannel/fake" . "knative.dev/eventing/pkg/reconciler/testing/v1" ) @@ -1419,6 +1416,10 @@ func TestAllCases(t *testing.T) { }, { Name: "v1 imc - delivery defaulting - full delivery spec", + Ctx: feature.ToContext(context.TODO(), feature.Flags{ + feature.DeliveryTimeout: feature.Enabled, + feature.DeliveryRetryAfter: feature.Enabled, + }), Objects: []runtime.Object{ NewSubscription("a-"+subscriptionName, testNS, WithSubscriptionUID("a-"+subscriptionUID), @@ -1445,6 +1446,11 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), + Timeout: pointer.StringPtr("PT2S"), + RetryAfter: &eventingduck.RetryAfter{ + Enabled: true, + MaxDuration: pointer.StringPtr("PT3S"), + }, }), ), NewService(serviceName, testNS), @@ -1480,6 +1486,11 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), + Timeout: pointer.StringPtr("PT2S"), + RetryAfter: &eventingduck.RetryAfter{ + Enabled: true, + MaxDuration: pointer.StringPtr("PT3S"), + }, }, }, }), @@ -1488,6 +1499,10 @@ func TestAllCases(t *testing.T) { }, { Name: "v1 imc - don't default delivery - full delivery spec", + Ctx: feature.ToContext(context.TODO(), feature.Flags{ + feature.DeliveryTimeout: feature.Enabled, + feature.DeliveryRetryAfter: feature.Enabled, + }), Objects: []runtime.Object{ NewSubscription("a-"+subscriptionName, testNS, WithSubscriptionUID("a-"+subscriptionUID), @@ -1505,6 +1520,11 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), + Timeout: pointer.StringPtr("PT2S"), + RetryAfter: &eventingduck.RetryAfter{ + Enabled: true, + MaxDuration: pointer.StringPtr("PT3S"), + }, }), ), NewUnstructured(subscriberGVK, dlcName, testNS, @@ -1530,6 +1550,11 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(20), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT10S"), + Timeout: pointer.StringPtr("PT20S"), + RetryAfter: &eventingduck.RetryAfter{ + Enabled: false, + MaxDuration: pointer.StringPtr("PT30S"), + }, }), ), NewService(serviceName, testNS), @@ -1562,6 +1587,11 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), + Timeout: pointer.StringPtr("PT2S"), + RetryAfter: &eventingduck.RetryAfter{ + Enabled: true, + MaxDuration: pointer.StringPtr("PT3S"), + }, }), WithSubscriptionDeadLetterSinkURI(dlcURI), ), @@ -1578,6 +1608,11 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), + Timeout: pointer.StringPtr("PT2S"), + RetryAfter: &eventingduck.RetryAfter{ + Enabled: true, + MaxDuration: pointer.StringPtr("PT3S"), + }, }, }, }), From b942a39bf408e7b6442ac3c047a2577ff7b294f4 Mon Sep 17 00:00:00 2001 From: I867318 Date: Fri, 8 Oct 2021 14:28:57 -0600 Subject: [PATCH 02/15] update-codegen --- docs/eventing-api.md | 48 +++++++++-------------- pkg/apis/duck/v1/zz_generated.deepcopy.go | 26 ++++++++++++ 2 files changed, 45 insertions(+), 29 deletions(-) diff --git a/docs/eventing-api.md b/docs/eventing-api.md index 9d5cfac6ee6..56f2f7fa7a1 100644 --- a/docs/eventing-api.md +++ b/docs/eventing-api.md @@ -353,11 +353,9 @@ string (Optional)

Timeout is the timeout of each single request. The value must be greater than 0. More information on Duration format: - - https://www.iso.org/iso-8601-date-and-time-format.html - https://en.wikipedia.org/wiki/ISO_8601

- -

Note: This API is EXPERIMENTAL and could be deprecated in the future. For more details see https://github.com/knative/eventing/issues/5148

+

Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/5148

 @@ -395,17 +393,16 @@ For exponential policy, backoff delay is backoffDelay*2^.

 @@ -447,14 +444,10 @@ where failed events are sent to.

(Appears on:DeliverySpec) -

RetryAfter contains configuration controlling the handling of HTTP -"Retry-After" -headers for responses with 429 (Too Many Requests) and 503 -(Service Unavailable) response codes. If not provided, the default behavior is -to simply ignore "Retry-After" headers and to retry at the configured rate -specified in the DeliverySpec.

-

Note: This API is EXPERIMENTAL and could be deprecated in the future. For more details see -https://github.com/knative/eventing/issues/TODO

+

+

+

RetryAfter contains configuration related to the handling of “Retry-After” headers.

+

FieldDescription
+enabled
+ +bool + +		 +

Enabled is a boolean flag indicating whether "Retry-After" headers should be +respected when calculating back-off durations while re-sending events. The +largest of the "Retry-After" duration, and the calculated linear or exponential +backoff will be used.

+
+maxDuration
+ +string + +		 +(Optional) +

MaxDuration represents an override of the "Retry-After" header value in order +to prevent excessively large durations from negatively impacting event +processing. If the "Retry-After" duration exceeds the maxDuration, then it will +be replaced with the maxDuration. This value does NOT impact the +calculated linear or exponential backoff durations. + +More information on Duration format: + +- https://www.iso.org/iso-8601-date-and-time-format.html +- https://en.wikipedia.org/wiki/ISO_8601

+
retryAfter
+ RetryAfter + 		(Optional) -

RetryAfter contains configuration controlling the handling of -"Retry-After" -headers for responses with 429 (Too Many Requests) and 503 (Service Unavailable) -response codes.

-

Note: This API is EXPERIMENTAL and could be deprecated in the future. For more details see -https://github.com/knative/eventing/issues/TODO

+

RetryAfter controls how “Retry-After” header durations are handled for 429 and 503 response codes. +If not provided, the default behavior is to ignore “Retry-After” headers in responses.

+

Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/TODO
@@ -471,10 +464,11 @@ bool @@ -486,17 +480,13 @@ string diff --git a/pkg/apis/duck/v1/zz_generated.deepcopy.go b/pkg/apis/duck/v1/zz_generated.deepcopy.go index ffd6985aed6..592cff65e6d 100644 --- a/pkg/apis/duck/v1/zz_generated.deepcopy.go +++ b/pkg/apis/duck/v1/zz_generated.deepcopy.go @@ -162,6 +162,11 @@ func (in *DeliverySpec) DeepCopyInto(out *DeliverySpec) { *out = new(string) **out = **in } + if in.RetryAfter != nil { + in, out := &in.RetryAfter, &out.RetryAfter + *out = new(RetryAfter) + (*in).DeepCopyInto(*out) + } return } @@ -196,6 +201,27 @@ func (in *DeliveryStatus) DeepCopy() *DeliveryStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *RetryAfter) DeepCopyInto(out *RetryAfter) { + *out = *in + if in.MaxDuration != nil { + in, out := &in.MaxDuration, &out.MaxDuration + *out = new(string) + **out = **in + } + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RetryAfter. +func (in *RetryAfter) DeepCopy() *RetryAfter { + if in == nil { + return nil + } + out := new(RetryAfter) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Subscribable) DeepCopyInto(out *Subscribable) { *out = *in From fea4cc57e3beb1fa158100e1d73dc3c45c3f54d7 Mon Sep 17 00:00:00 2001 From: I867318 Date: Wed, 13 Oct 2021 09:46:12 -0600 Subject: [PATCH 03/15] minor enhancements --- pkg/kncloudevents/message_sender.go | 13 ++++++------- pkg/kncloudevents/retries_test.go | 14 +++++++++----- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/pkg/kncloudevents/message_sender.go b/pkg/kncloudevents/message_sender.go index 50715d64a47..1eddc394ac2 100644 --- a/pkg/kncloudevents/message_sender.go +++ b/pkg/kncloudevents/message_sender.go @@ -97,14 +97,13 @@ func (s *HTTPMessageSender) SendWithRetries(req *nethttp.Request, config *RetryC func generateBackoffFn(config *RetryConfig) retryablehttp.Backoff { return func(_, _ time.Duration, attemptNum int, resp *nethttp.Response) time.Duration { - // If Response is 429 or 503 Then Parse Any Retry-After Header Durations & Enforce Optional MaxDuration + // If RetryAfter Is Enabled And Response is 429 / 503, Then Parse Any Retry-After Header Durations & Enforce Optional MaxDuration var retryAfterDuration time.Duration - if resp != nil && (resp.StatusCode == nethttp.StatusTooManyRequests || resp.StatusCode == nethttp.StatusServiceUnavailable) { - if config.RetryAfterEnabled { - retryAfterDuration, _ = parseRetryAfterDuration(resp) - if config.RetryAfterMaxDuration > 0 && config.RetryAfterMaxDuration < retryAfterDuration { - retryAfterDuration = config.RetryAfterMaxDuration - } + if config.RetryAfterEnabled && resp != nil && + (resp.StatusCode == nethttp.StatusTooManyRequests || resp.StatusCode == nethttp.StatusServiceUnavailable) { + retryAfterDuration, _ = parseRetryAfterDuration(resp) + if config.RetryAfterMaxDuration > 0 && config.RetryAfterMaxDuration < retryAfterDuration { + retryAfterDuration = config.RetryAfterMaxDuration } } diff --git a/pkg/kncloudevents/retries_test.go b/pkg/kncloudevents/retries_test.go index 397b4d79ded..ef71a2b5dfe 100644 --- a/pkg/kncloudevents/retries_test.go +++ b/pkg/kncloudevents/retries_test.go @@ -43,6 +43,8 @@ func TestNoRetries(t *testing.T) { assert.NotNil(t, retryConfig.Backoff) assert.Equal(t, time.Duration(0), retryConfig.Backoff(1, nil)) assert.Equal(t, time.Duration(0), retryConfig.Backoff(100, nil)) + assert.False(t, retryConfig.RetryAfterEnabled) + assert.Equal(t, time.Duration(0), retryConfig.RetryAfterMaxDuration) } // Test The RetryConfigFromDeliverySpec() Functionality @@ -98,7 +100,7 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { backoffDelay: "FOO", wantErr: true, }, { - name: "With Valid Timeout", + name: "Valid Timeout", backoffPolicy: v1.BackoffPolicyExponential, backoffDelay: "PT0.5S", timeout: &validISO8601DurationString, @@ -110,13 +112,13 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { 16 * time.Second, }, }, { - name: "With Invalid Timeout", + name: "Invalid Timeout", backoffPolicy: v1.BackoffPolicyExponential, backoffDelay: "PT0.5S", timeout: &invalidISO8601DurationString, wantErr: true, }, { - name: "With Valid Sparse Retry-After", + name: "Valid Sparse Retry-After", backoffPolicy: v1.BackoffPolicyExponential, backoffDelay: "PT0.5S", retryAfter: &v1.RetryAfter{Enabled: true}, @@ -128,7 +130,7 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { 16 * time.Second, }, }, { - name: "With Valid Complete Retry-After", + name: "Valid Complete Retry-After", backoffPolicy: v1.BackoffPolicyExponential, backoffDelay: "PT0.5S", retryAfter: &v1.RetryAfter{Enabled: true, MaxDuration: &validISO8601DurationString}, @@ -140,7 +142,7 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { 16 * time.Second, }, }, { - name: "With Invalid Complete Retry-After", + name: "Invalid Complete Retry-After", backoffPolicy: v1.BackoffPolicyExponential, backoffDelay: "PT0.5S", retryAfter: &v1.RetryAfter{Enabled: true, MaxDuration: &invalidISO8601DurationString}, @@ -178,6 +180,8 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { expectedMaxPeriod, _ := period.Parse(*tc.retryAfter.MaxDuration) expectedMaxDuration, _ := expectedMaxPeriod.Duration() assert.Equal(t, expectedMaxDuration, retryConfig.RetryAfterMaxDuration) + } else { + assert.Equal(t, time.Duration(0), retryConfig.RetryAfterMaxDuration) } } else { assert.False(t, retryConfig.RetryAfterEnabled) From a6d0d3554960ec4e2c5989628e132f8d363a6f95 Mon Sep 17 00:00:00 2001 From: I867318 Date: Wed, 13 Oct 2021 14:31:51 -0600 Subject: [PATCH 04/15] Updated Issue Number TODO --- config/core/configmaps/features.yaml | 2 +- docs/eventing-api.md | 2 +- pkg/apis/duck/v1/delivery_types.go | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/config/core/configmaps/features.yaml b/config/core/configmaps/features.yaml index 61d3493caa6..7c4644be506 100644 --- a/config/core/configmaps/features.yaml +++ b/config/core/configmaps/features.yaml @@ -29,7 +29,7 @@ data: kreference-group: "disabled" # ALPHA feature: The delivery-retryafter allows you to use the RetryAfter field in DeliverySpec. - # For more details: https://github.com/knative/eventing/issues/TODO!!! + # For more details: https://github.com/knative/eventing/issues/5811 delivery-retryafter: "disabled" # ALPHA feature: The delivery-timeout allows you to use the Timeout field in DeliverySpec. diff --git a/docs/eventing-api.md b/docs/eventing-api.md index 56f2f7fa7a1..7623b4d9149 100644 --- a/docs/eventing-api.md +++ b/docs/eventing-api.md @@ -402,7 +402,7 @@ RetryAfter (Optional)

RetryAfter controls how “Retry-After” header durations are handled for 429 and 503 response codes. If not provided, the default behavior is to ignore “Retry-After” headers in responses.

-

Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/TODO

+

Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/5811

diff --git a/pkg/apis/duck/v1/delivery_types.go b/pkg/apis/duck/v1/delivery_types.go index 17c81b16db2..b590d74edb3 100644 --- a/pkg/apis/duck/v1/delivery_types.go +++ b/pkg/apis/duck/v1/delivery_types.go @@ -65,7 +65,7 @@ type DeliverySpec struct { // RetryAfter controls how "Retry-After" header durations are handled for 429 and 503 response codes. // If not provided, the default behavior is to ignore "Retry-After" headers in responses. // - // Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/TODO + // Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/5811 // +optional RetryAfter *RetryAfter `json:"retryAfter,omitempty"` } From 3f7d714afd28b179af78430eead777cb30a9a333 Mon Sep 17 00:00:00 2001 From: I867318 Date: Thu, 21 Oct 2021 08:23:42 -0600 Subject: [PATCH 05/15] Add e2e tests --- test/experimental/config/features.yaml | 1 + .../features/retry_after/channel.go | 164 ++++++++++++++++++ test/experimental/retry_after_test.go | 63 +++++++ 3 files changed, 228 insertions(+) create mode 100644 test/experimental/features/retry_after/channel.go create mode 100644 test/experimental/retry_after_test.go diff --git a/test/experimental/config/features.yaml b/test/experimental/config/features.yaml index 76b436b94c6..45fb3ecf56f 100644 --- a/test/experimental/config/features.yaml +++ b/test/experimental/config/features.yaml @@ -23,5 +23,6 @@ metadata: knative.dev/config-category: eventing data: kreference-group: "enabled" + delivery-retryafter: "enabled" delivery-timeout: "enabled" strict-subscriber: "disabled" diff --git a/test/experimental/features/retry_after/channel.go b/test/experimental/features/retry_after/channel.go new file mode 100644 index 00000000000..7c9b9a5b11a --- /dev/null +++ b/test/experimental/features/retry_after/channel.go @@ -0,0 +1,164 @@ +/* +Copyright 2021 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package retry_after + +import ( + "context" + "fmt" + "net/http" + "strconv" + "testing" + "time" + + cetest "github.com/cloudevents/sdk-go/v2/test" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/utils/pointer" + duckv1 "knative.dev/pkg/apis/duck/v1" + "knative.dev/reconciler-test/pkg/environment" + "knative.dev/reconciler-test/pkg/eventshub" + "knative.dev/reconciler-test/pkg/eventshub/assert" + "knative.dev/reconciler-test/pkg/feature" + "knative.dev/reconciler-test/pkg/state" + + eventingduckv1 "knative.dev/eventing/pkg/apis/duck/v1" + messagingv1 "knative.dev/eventing/pkg/apis/messaging/v1" + eventingclient "knative.dev/eventing/pkg/client/injection/client" + "knative.dev/eventing/test/rekt/resources/channel" + "knative.dev/eventing/test/rekt/resources/subscription" +) + +const ( + ChannelNameKey = "ChannelNameKey" + SubscriptionNameKey = "SubscriptionNameKey" + SenderNameKey = "SenderNameKey" + ReceiverNameKey = "ReceiverNameKey" + RetryAttemptsKey = "RetryAttemptsKey" + RetryAfterSecondsKey = "RetryAfterSecondsKey" +) + +// ConfigureDataPlane creates a Feature which sets up the specified Channel, +// Subscription and EventsHub Receiver so that it is ready to receive CloudEvents. +func ConfigureDataPlane(ctx context.Context, t *testing.T) *feature.Feature { + + // Get Component Names From Context + var retryAttempts, retryAfterSeconds int + channelName := state.GetStringOrFail(ctx, t, ChannelNameKey) + subscriptionName := state.GetStringOrFail(ctx, t, SubscriptionNameKey) + receiverName := state.GetStringOrFail(ctx, t, ReceiverNameKey) + state.GetOrFail(ctx, t, RetryAttemptsKey, &retryAttempts) + state.GetOrFail(ctx, t, RetryAfterSecondsKey, &retryAfterSeconds) + + // Create A Feature To Configure The DataPlane (Channel, Subscription, Receiver) + f := feature.NewFeatureNamed("Configure Data-Plane") + f.Setup("Install An EventsHub Receiver", eventshub.Install(receiverName, + eventshub.StartReceiver, + eventshub.DropFirstN(uint(retryAttempts)), + eventshub.DropEventsResponseCode(http.StatusTooManyRequests), + eventshub.DropEventsResponseHeaders(map[string]string{"Retry-After": strconv.Itoa(retryAfterSeconds)}))) + f.Setup("Install A Channel", channel.Install(channelName)) + f.Setup("Install A Subscription", installRetryAfterSubscription(channelName, subscriptionName, receiverName, int32(retryAttempts))) + f.Assert("Channel Is Ready", channel.IsReady(channelName)) + f.Assert("Subscription Is Ready", subscription.IsReady(subscriptionName)) + + // Return The ConfigureDataPlane Feature + return f +} + +// SendEvent creates a Feature which sends a CloudEvents to the specified +// Channel and verifies the timing of its receipt in the corresponding +// EventsHub Receiver. It is assumed that the backing Channel / Subscription +// / Receiver are in place and ready to receive the event. +func SendEvent(ctx context.Context, t *testing.T) *feature.Feature { + + // Get Component Names From Context + var retryAttempts, retryAfterSeconds int + channelName := state.GetStringOrFail(ctx, t, ChannelNameKey) + senderName := state.GetStringOrFail(ctx, t, SenderNameKey) + receiverName := state.GetStringOrFail(ctx, t, ReceiverNameKey) + state.GetOrFail(ctx, t, RetryAttemptsKey, &retryAttempts) + state.GetOrFail(ctx, t, RetryAfterSecondsKey, &retryAfterSeconds) + + // Create The Base CloudEvent To Send (ID will be set by the EventsHub Sender) + event := cetest.FullEvent() + + // Mark The Current Time Plus The Expected Retry-After Duration + earliestEventTime := time.Now().Add(time.Duration(retryAfterSeconds) * time.Second) + + // Create A New Feature To Send An Event And Verify Retry-After Duration + f := feature.NewFeatureNamed("Send Events") + f.Setup("Install An EventsHub Sender", eventshub.Install(senderName, eventshub.StartSenderToResource(channel.GVR(), channelName), eventshub.InputEvent(event))) + f.Assert("Events Received", assert.OnStore(receiverName).MatchEvent(cetest.HasId(event.ID())).Exact(retryAttempts+1)) // One Successful Response + f.Assert("Event Timing Verified", assert.OnStore(receiverName).Match(receivedAfterEventInfoMatcher(earliestEventTime)).Exact(retryAttempts)) + + // Return The SendEvents Feature + return f +} + +// installRetryAfterSubscription performs the installation of a Subscription +// for the specified Channel / Sink in the test namespace. The Subscription +// is configured to specify the experimental RetryAfter behavior which the +// standard Subscription resource/yaml does not include. +func installRetryAfterSubscription(channelName, subscriptionName, sinkName string, retryAttempts int32) func(ctx context.Context, t feature.T) { + return func(ctx context.Context, t feature.T) { + namespace := environment.FromContext(ctx).Namespace() + channelAPIVersion, imcKind := channel.GVK().ToAPIVersionAndKind() + backoffPolicy := eventingduckv1.BackoffPolicyLinear + retryAfterSubscription := &messagingv1.Subscription{ + ObjectMeta: metav1.ObjectMeta{ + Name: subscriptionName, + Namespace: namespace, + }, + Spec: messagingv1.SubscriptionSpec{ + Channel: duckv1.KReference{ + APIVersion: channelAPIVersion, + Kind: imcKind, + Name: channelName, + }, + Subscriber: &duckv1.Destination{ + Ref: &duckv1.KReference{ + APIVersion: "v1", + Kind: "Service", + Name: sinkName, + }, + }, + Delivery: &eventingduckv1.DeliverySpec{ + Retry: pointer.Int32Ptr(retryAttempts), + BackoffPolicy: &backoffPolicy, + BackoffDelay: pointer.StringPtr("PT0.5S"), + RetryAfter: &eventingduckv1.RetryAfter{ + Enabled: true, + MaxDuration: pointer.StringPtr("PT30S"), + }, + }, + }, + } + _, err := eventingclient.Get(ctx).MessagingV1().Subscriptions(namespace).Create(ctx, retryAfterSubscription, metav1.CreateOptions{}) + require.NoError(t, err) + } +} + +// receivedAfterEventInfoMatcher returns an EventInfoMatcher that validates +// the receipt time of the EventInfo is AFTER the specified minimum time. +func receivedAfterEventInfoMatcher(earliestTime time.Time) eventshub.EventInfoMatcher { + return func(eventInfo eventshub.EventInfo) error { + if eventInfo.Time.Before(earliestTime) { + return fmt.Errorf("response received prior to Retry-After header duration") + } + return nil + } +} diff --git a/test/experimental/retry_after_test.go b/test/experimental/retry_after_test.go new file mode 100644 index 00000000000..3f057b39c6a --- /dev/null +++ b/test/experimental/retry_after_test.go @@ -0,0 +1,63 @@ +// +build e2e + +/* +Copyright 2021 The Knative Authors + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package experimental + +import ( + "testing" + + "knative.dev/pkg/system" + "knative.dev/reconciler-test/pkg/environment" + "knative.dev/reconciler-test/pkg/feature" + "knative.dev/reconciler-test/pkg/k8s" + "knative.dev/reconciler-test/pkg/knative" + "knative.dev/reconciler-test/pkg/state" + + "knative.dev/eventing/test/experimental/features/retry_after" +) + +func TestRetryAfter(t *testing.T) { + + // Run Test In Parallel With Others + t.Parallel() + + // Create The Test Context / Environment + ctx, env := global.Environment( + knative.WithKnativeNamespace(system.Namespace()), + knative.WithLoggingConfig, + knative.WithTracingConfig, + k8s.WithEventListener, + environment.Managed(t), + ) + + // Generate A Unique K8S Safe Prefix For The Test Components + retryAfterPrefix := feature.MakeRandomK8sName("retryafter") + + // Generate Unique Component Names And Add To Context Store + ctx = state.ContextWith(ctx, &state.KVStore{}) + state.SetOrFail(ctx, t, retry_after.ChannelNameKey, retryAfterPrefix+"-channel") + state.SetOrFail(ctx, t, retry_after.SubscriptionNameKey, retryAfterPrefix+"-subscription") + state.SetOrFail(ctx, t, retry_after.SenderNameKey, retryAfterPrefix+"-sender") + state.SetOrFail(ctx, t, retry_after.ReceiverNameKey, retryAfterPrefix+"-receiver") + state.SetOrFail(ctx, t, retry_after.RetryAttemptsKey, 3) + state.SetOrFail(ctx, t, retry_after.RetryAfterSecondsKey, 10) + + // Configure DataPlane & Send An Event + env.Test(ctx, t, retry_after.ConfigureDataPlane(ctx, t)) + env.Test(ctx, t, retry_after.SendEvent(ctx, t)) +} From d2270629ffea7412efa4595dc9b8d5852cad0e7d Mon Sep 17 00:00:00 2001 From: I867318 Date: Thu, 21 Oct 2021 08:30:00 -0600 Subject: [PATCH 06/15] build tag update --- test/experimental/retry_after_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/test/experimental/retry_after_test.go b/test/experimental/retry_after_test.go index 3f057b39c6a..4620073b840 100644 --- a/test/experimental/retry_after_test.go +++ b/test/experimental/retry_after_test.go @@ -1,3 +1,4 @@ +//go:build e2e // +build e2e /* From 9fc7448bee02aab98a1ffea4d7b37f50bad2e946 Mon Sep 17 00:00:00 2001 From: I867318 Date: Mon, 8 Nov 2021 15:06:34 -0700 Subject: [PATCH 07/15] Refactor implementation for new design based on community feedback. --- docs/eventing-api.md | 74 +---- pkg/apis/duck/v1/delivery_types.go | 47 ++- pkg/apis/duck/v1/delivery_types_test.go | 28 +- pkg/apis/duck/v1/zz_generated.deepcopy.go | 29 +- pkg/kncloudevents/message_sender.go | 51 +++- pkg/kncloudevents/message_sender_test.go | 355 ++++++++++++++-------- pkg/kncloudevents/retries.go | 23 +- pkg/kncloudevents/retries_test.go | 45 +-- 8 files changed, 349 insertions(+), 303 deletions(-) diff --git a/docs/eventing-api.md b/docs/eventing-api.md index e418badda5d..d4460eea2fc 100644 --- a/docs/eventing-api.md +++ b/docs/eventing-api.md @@ -394,18 +394,27 @@ For exponential policy, backoff delay is backoffDelay*2^.

 @@ -443,57 +452,6 @@ where failed events are sent to.

-

Enabled is a boolean flag indicating whether "Retry-After" headers should be -respected when calculating back-off durations while re-sending events. The -largest of the "Retry-After" duration, and the calculated linear or exponential -backoff will be used.

+

Enabled is a flag indicating whether to respect the “Retry-After” header duration. +If enabled, the largest of the normal backoff duration and the “Retry-After” +header value will be used when calculating the next backoff duration. This will +only be considered when a 429 (Too Many Requests) or 503 (Service Unavailable) +response code is received and Retry is greater than 0.
(Optional) -

MaxDuration represents an override of the "Retry-After" header value in order -to prevent excessively large durations from negatively impacting event -processing. If the "Retry-After" duration exceeds the maxDuration, then it will -be replaced with the maxDuration. This value does NOT impact the -calculated linear or exponential backoff durations. - +

MaxDuration is the maximum time to wait before retrying. It is intended as an +override to protect against excessively large “Retry-After” durations. If provided, +the value must be greater than 0. If not provided, the largest of the “Retry-After” +duration and the normal backoff duration will be used. More information on Duration format: - - https://www.iso.org/iso-8601-date-and-time-format.html - https://en.wikipedia.org/wiki/ISO_8601

-
-retryAfter
+retryAfterMax
- -RetryAfter - +string 		(Optional) -

RetryAfter controls how “Retry-After” header durations are handled for 429 and 503 response codes. -If not provided, the default behavior is to ignore “Retry-After” headers in responses.

-

Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/5811

+

RetryAfterMax provides an optional upper bound on the duration specified in a “Retry-After” header +when calculating backoff times for retrying 429 and 502 response codes. Setting the value to +zero (“PT0S”) can be used to opt-out of respecting “Retry-After” header values altogether. This +value only takes effect if “Retry” is configured, and also depends on specific implementations +(Channels, Sources, etc.) choosing to provide this capability.

+

Note: This API is EXPERIMENTAL and might be changed at anytime! While this experimental +feature is in the Alpha/Beta stage, you must provide a valid a value to opt-in to +supporting “Retry-After” headers. When the feature becomes Stable/GA “Retry-After” +headers will be respected by default, and you can choose to specify “PT0S” to +opt-out of supporting “Retry-After” headers. +For more details: https://github.com/knative/eventing/issues/5811

+

More information on Duration format: +- https://www.iso.org/iso-8601-date-and-time-format.html +- https://en.wikipedia.org/wiki/ISO_8601
-

RetryAfter -

-

-(Appears on:DeliverySpec) -

-

-

RetryAfter contains configuration related to the handling of “Retry-After” headers.

-

- - - - - - - - - - - - - - - - - -
FieldDescription
-enabled
- -bool - -		 -

Enabled is a flag indicating whether to respect the “Retry-After” header duration. -If enabled, the largest of the normal backoff duration and the “Retry-After” -header value will be used when calculating the next backoff duration. This will -only be considered when a 429 (Too Many Requests) or 503 (Service Unavailable) -response code is received and Retry is greater than 0.

-
-maxDuration
- -string - -		 -(Optional) -

MaxDuration is the maximum time to wait before retrying. It is intended as an -override to protect against excessively large “Retry-After” durations. If provided, -the value must be greater than 0. If not provided, the largest of the “Retry-After” -duration and the normal backoff duration will be used. -More information on Duration format: -- https://www.iso.org/iso-8601-date-and-time-format.html -- https://en.wikipedia.org/wiki/ISO_8601

-

Subscribable

diff --git a/pkg/apis/duck/v1/delivery_types.go b/pkg/apis/duck/v1/delivery_types.go index b590d74edb3..5c4f5434940 100644 --- a/pkg/apis/duck/v1/delivery_types.go +++ b/pkg/apis/duck/v1/delivery_types.go @@ -62,32 +62,25 @@ type DeliverySpec struct { // +optional BackoffDelay *string `json:"backoffDelay,omitempty"` - // RetryAfter controls how "Retry-After" header durations are handled for 429 and 503 response codes. - // If not provided, the default behavior is to ignore "Retry-After" headers in responses. + // RetryAfterMax provides an optional upper bound on the duration specified in a "Retry-After" header + // when calculating backoff times for retrying 429 and 502 response codes. Setting the value to + // zero ("PT0S") can be used to opt-out of respecting "Retry-After" header values altogether. This + // value only takes effect if "Retry" is configured, and also depends on specific implementations + // (Channels, Sources, etc.) choosing to provide this capability. + // + // Note: This API is EXPERIMENTAL and might be changed at anytime! While this experimental + // feature is in the Alpha/Beta stage, you must provide a valid a value to opt-in to + // supporting "Retry-After" headers. When the feature becomes Stable/GA "Retry-After" + // headers will be respected by default, and you can choose to specify "PT0S" to + // opt-out of supporting "Retry-After" headers. + // For more details: https://github.com/knative/eventing/issues/5811 // - // Note: This API is EXPERIMENTAL and might break anytime. For more details: https://github.com/knative/eventing/issues/5811 - // +optional - RetryAfter *RetryAfter `json:"retryAfter,omitempty"` -} - -// RetryAfter contains configuration related to the handling of "Retry-After" headers. -type RetryAfter struct { - // Enabled is a flag indicating whether to respect the "Retry-After" header duration. - // If enabled, the largest of the normal backoff duration and the "Retry-After" - // header value will be used when calculating the next backoff duration. This will - // only be considered when a 429 (Too Many Requests) or 503 (Service Unavailable) - // response code is received and Retry is greater than 0. - Enabled bool `json:"enabled"` - - // MaxDuration is the maximum time to wait before retrying. It is intended as an - // override to protect against excessively large "Retry-After" durations. If provided, - // the value must be greater than 0. If not provided, the largest of the "Retry-After" - // duration and the normal backoff duration will be used. // More information on Duration format: // - https://www.iso.org/iso-8601-date-and-time-format.html // - https://en.wikipedia.org/wiki/ISO_8601 + // // +optional - MaxDuration *string `json:"maxDuration,omitempty"` + RetryAfterMax *string `json:"retryAfterMax,omitempty"` } func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError { @@ -130,16 +123,14 @@ func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError { } } - if ds.RetryAfter != nil { + if ds.RetryAfterMax != nil { if feature.FromContext(ctx).IsEnabled(feature.DeliveryRetryAfter) { - if ds.RetryAfter.MaxDuration != nil && *ds.RetryAfter.MaxDuration != "" { - m, me := period.Parse(*ds.RetryAfter.MaxDuration) - if me != nil || m.IsZero() { - errs = errs.Also(apis.ErrInvalidValue(*ds.RetryAfter.MaxDuration, "retryAfter.maxDuration")) - } + _, me := period.Parse(*ds.RetryAfterMax) + if me != nil { + errs = errs.Also(apis.ErrInvalidValue(*ds.RetryAfterMax, "retryAfterMax")) } } else { - errs = errs.Also(apis.ErrDisallowedFields("retryAfter")) + errs = errs.Also(apis.ErrDisallowedFields("retryAfterMax")) } } diff --git a/pkg/apis/duck/v1/delivery_types_test.go b/pkg/apis/duck/v1/delivery_types_test.go index 4fe8e912a32..f2b2ab50ae2 100644 --- a/pkg/apis/duck/v1/delivery_types_test.go +++ b/pkg/apis/duck/v1/delivery_types_test.go @@ -27,8 +27,6 @@ import ( "knative.dev/eventing/pkg/apis/feature" ) -// TODO - add test cases for RetryAfter - func TestDeliverySpecValidation(t *testing.T) { deliveryTimeoutEnabledCtx := feature.ToContext(context.TODO(), feature.Flags{ feature.DeliveryTimeout: feature.Enabled, @@ -114,34 +112,34 @@ func TestDeliverySpecValidation(t *testing.T) { spec: &DeliverySpec{Retry: pointer.Int32Ptr(1)}, want: nil, }, { - name: "valid complete retryAfter", + name: "valid retryAfterMax", ctx: deliveryRetryAfterEnabledCtx, - spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true, MaxDuration: &validDuration}}, + spec: &DeliverySpec{RetryAfterMax: &validDuration}, want: nil, }, { - name: "valid sparse retryAfter", + name: "zero retryAfterMax", ctx: deliveryRetryAfterEnabledCtx, - spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true}}, + spec: &DeliverySpec{RetryAfterMax: pointer.StringPtr("PT0S")}, want: nil, }, { - name: "zero retryAfter.MaxDuration", + name: "empty retryAfterMax", ctx: deliveryRetryAfterEnabledCtx, - spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true, MaxDuration: pointer.StringPtr("PT0S")}}, + spec: &DeliverySpec{RetryAfterMax: pointer.StringPtr("")}, want: func() *apis.FieldError { - return apis.ErrInvalidValue("PT0S", "retryAfter.maxDuration") + return apis.ErrInvalidValue("", "retryAfterMax") }(), }, { - name: "invalid retryAfter.MaxDuration", + name: "invalid retryAfterMax", ctx: deliveryRetryAfterEnabledCtx, - spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true, MaxDuration: &invalidDuration}}, + spec: &DeliverySpec{RetryAfterMax: &invalidDuration}, want: func() *apis.FieldError { - return apis.ErrInvalidValue(invalidDuration, "retryAfter.maxDuration") + return apis.ErrInvalidValue(invalidDuration, "retryAfterMax") }(), }, { - name: "disabled retryAfter", - spec: &DeliverySpec{RetryAfter: &RetryAfter{Enabled: true}}, + name: "disabled feature with retryAfterMax", + spec: &DeliverySpec{RetryAfterMax: &validDuration}, want: func() *apis.FieldError { - return apis.ErrDisallowedFields("retryAfter") + return apis.ErrDisallowedFields("retryAfterMax") }(), }} diff --git a/pkg/apis/duck/v1/zz_generated.deepcopy.go b/pkg/apis/duck/v1/zz_generated.deepcopy.go index 02e9dc91ba3..8e30ee5cb49 100644 --- a/pkg/apis/duck/v1/zz_generated.deepcopy.go +++ b/pkg/apis/duck/v1/zz_generated.deepcopy.go @@ -163,10 +163,10 @@ func (in *DeliverySpec) DeepCopyInto(out *DeliverySpec) { *out = new(string) **out = **in } - if in.RetryAfter != nil { - in, out := &in.RetryAfter, &out.RetryAfter - *out = new(RetryAfter) - (*in).DeepCopyInto(*out) + if in.RetryAfterMax != nil { + in, out := &in.RetryAfterMax, &out.RetryAfterMax + *out = new(string) + **out = **in } return } @@ -202,27 +202,6 @@ func (in *DeliveryStatus) DeepCopy() *DeliveryStatus { return out } -// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. -func (in *RetryAfter) DeepCopyInto(out *RetryAfter) { - *out = *in - if in.MaxDuration != nil { - in, out := &in.MaxDuration, &out.MaxDuration - *out = new(string) - **out = **in - } - return -} - -// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new RetryAfter. -func (in *RetryAfter) DeepCopy() *RetryAfter { - if in == nil { - return nil - } - out := new(RetryAfter) - in.DeepCopyInto(out) - return out -} - // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *Subscribable) DeepCopyInto(out *Subscribable) { *out = *in diff --git a/pkg/kncloudevents/message_sender.go b/pkg/kncloudevents/message_sender.go index 1eddc394ac2..26db6fc410a 100644 --- a/pkg/kncloudevents/message_sender.go +++ b/pkg/kncloudevents/message_sender.go @@ -97,13 +97,43 @@ func (s *HTTPMessageSender) SendWithRetries(req *nethttp.Request, config *RetryC func generateBackoffFn(config *RetryConfig) retryablehttp.Backoff { return func(_, _ time.Duration, attemptNum int, resp *nethttp.Response) time.Duration { - // If RetryAfter Is Enabled And Response is 429 / 503, Then Parse Any Retry-After Header Durations & Enforce Optional MaxDuration + // + // NOTE - The following logic will need to be altered slightly once the "delivery-retryafter" + // experimental-feature graduates from Alpha/Beta to Stable/GA. This is according to + // plan as described in https://github.com/knative/eventing/issues/5811. + // + // During the Alpha/Beta stages the ability to respect Retry-After headers is "opt-in" + // requiring the DeliverySpec.RetryAfterMax to be populated. The Stable/GA behavior + // will be "opt-out" where Retry-After headers are always respected (in the context of + // calculating backoff durations for 429 / 503 responses) unless the + // DeliverySpec.RetryAfterMax is set to "PT0S". + // + // While this might seem unnecessarily complex, it achieves the following design goals... + // - Does not require an explicit "enabled" flag in the DeliverySpec. + // - Does not require implementations calling the message_sender to be aware of experimental-features. + // - Does not modify existing Knative CRs with arbitrary default "max" values. + // + // The intended behavior of RetryConfig.RetryAfterMaxDuration is as follows... + // + // RetryAfterMaxDuration Alpha/Beta Stable/GA + // --------------------- ---------- --------- + // nil Do NOT respect Retry-After headers Respect Retry-After headers without Max + // 0 Do NOT respect Retry-After headers Do NOT respect Retry-After headers + // >0 Respect Retry-After headers with Max Respect Retry-After headers with Max + // + + // If Response is 429 / 503, Then Parse Any Retry-After Header Durations & Enforce Optional MaxDuration var retryAfterDuration time.Duration - if config.RetryAfterEnabled && resp != nil && - (resp.StatusCode == nethttp.StatusTooManyRequests || resp.StatusCode == nethttp.StatusServiceUnavailable) { - retryAfterDuration, _ = parseRetryAfterDuration(resp) - if config.RetryAfterMaxDuration > 0 && config.RetryAfterMaxDuration < retryAfterDuration { - retryAfterDuration = config.RetryAfterMaxDuration + + // TODO - Remove this check when experimental-feature moves to Stable/GA to convert behavior from opt-in to opt-out + if config.RetryAfterMaxDuration != nil { + + // TODO - Keep this logic as is (no change required) when experimental-feature is Stable/GA + if resp != nil && (resp.StatusCode == nethttp.StatusTooManyRequests || resp.StatusCode == nethttp.StatusServiceUnavailable) { + retryAfterDuration = parseRetryAfterDuration(resp) + if config.RetryAfterMaxDuration != nil && *config.RetryAfterMaxDuration < retryAfterDuration { + retryAfterDuration = *config.RetryAfterMaxDuration + } } } @@ -120,11 +150,11 @@ func generateBackoffFn(config *RetryConfig) retryablehttp.Backoff { } // parseRetryAfterDuration returns a Duration expressing the amount of time -// requested to wait by a Retry-After header, or none if not present or invalid. +// requested to wait by a Retry-After header, or 0 if not present or invalid. // According to the spec (https://tools.ietf.org/html/rfc7231#section-7.1.3) // the Retry-After Header's value can be one of an HTTP-date or delay-seconds, // both of which are supported here. -func parseRetryAfterDuration(resp *nethttp.Response) (time.Duration, error) { +func parseRetryAfterDuration(resp *nethttp.Response) time.Duration { var retryAfterDuration time.Duration if resp != nil && resp.Header != nil { retryAfterString := resp.Header.Get(RetryAfterHeader) @@ -134,11 +164,12 @@ func parseRetryAfterDuration(resp *nethttp.Response) (time.Duration, error) { } else { retryAfterTime, parseTimeErr := nethttp.ParseTime(retryAfterString) // Supports http.TimeFormat, time.RFC850 & time.ANSIC if parseTimeErr != nil { - return retryAfterDuration, fmt.Errorf("failed to parse Retry-After header: ParseInt Error = %v, ParseTime Error = %v", parseIntErr, parseTimeErr) + fmt.Printf("failed to parse Retry-After header: ParseInt Error = %v, ParseTime Error = %v", parseIntErr, parseTimeErr) + return retryAfterDuration } retryAfterDuration = time.Until(retryAfterTime) } } } - return retryAfterDuration, nil + return retryAfterDuration } diff --git a/pkg/kncloudevents/message_sender_test.go b/pkg/kncloudevents/message_sender_test.go index 5692c04aa70..23d89050546 100644 --- a/pkg/kncloudevents/message_sender_test.go +++ b/pkg/kncloudevents/message_sender_test.go @@ -208,6 +208,10 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { Invalid ) + // Test Data + smallRetryAfterMaxDuration := 1 * time.Second + largeRetryAfterMaxDuration := 10 * time.Second + // Define The TestCases tests := []struct { name string @@ -216,129 +220,213 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { responseFormat RetryAfterFormat // Indicates the format in which the Server should return the Retry-After header. responseSeconds int // Indicates the number of Seconds for the Server to use when returning the Retry-After header. wantReqCount int // The total number of expected requests, including initial request. - }{{ - name: "disabled 429 without Retry-After", - config: &RetryConfig{ - RetryMax: 2, + }{ + // + // Default Max Tests (0 Duration opt-in vs opt-out) + // + + { + name: "default max 429 without Retry-After", + config: &RetryConfig{ + RetryMax: 2, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: None, + wantReqCount: 3, }, - statusCode: http.StatusTooManyRequests, - responseFormat: None, - wantReqCount: 3, - }, { - name: "disabled 429 with Retry-After seconds", - config: &RetryConfig{ - RetryMax: 2, + { + name: "default max 429 with Retry-After seconds", + config: &RetryConfig{ + RetryMax: 2, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Seconds, + responseSeconds: 1, + wantReqCount: 3, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Seconds, - responseSeconds: 1, - wantReqCount: 3, - }, { - name: "disabled 429 with Retry-After date", - config: &RetryConfig{ - RetryMax: 2, + { + name: "default max 429 with Retry-After date", + config: &RetryConfig{ + RetryMax: 2, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Date, + responseSeconds: 2, + wantReqCount: 3, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Date, - responseSeconds: 2, - wantReqCount: 3, - }, { - name: "enabled 429 without Retry-After", - config: &RetryConfig{ - RetryMax: 2, - RetryAfterEnabled: true, + { + name: "default max 429 with invalid Retry-After", + config: &RetryConfig{ + RetryMax: 2, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Invalid, + wantReqCount: 3, }, - statusCode: http.StatusTooManyRequests, - responseFormat: None, - wantReqCount: 3, - }, { - name: "enabled 429 with Retry-After seconds", - config: &RetryConfig{ - RetryMax: 2, - RetryAfterEnabled: true, + { + name: "default max 503 with Retry-After seconds", + config: &RetryConfig{ + RetryMax: 2, + }, + statusCode: http.StatusServiceUnavailable, + responseFormat: Seconds, + responseSeconds: 1, + wantReqCount: 3, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Seconds, - responseSeconds: 1, - wantReqCount: 3, - }, { - name: "enabled 429 with Retry-After seconds max override", - config: &RetryConfig{ - RetryMax: 2, - RetryAfterEnabled: true, - RetryAfterMaxDuration: 1 * time.Second, + { + name: "default max 500 without Retry-After", + config: &RetryConfig{ + RetryMax: 2, + }, + statusCode: http.StatusInternalServerError, + responseFormat: None, + wantReqCount: 3, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Seconds, - responseSeconds: 4, - wantReqCount: 3, - }, { - name: "enabled 429 with Retry-After date", - config: &RetryConfig{ - RetryMax: 2, - RetryAfterEnabled: true, + + // + // Large Max Tests (Greater Than Retry-After Value) + // + + { + name: "large max 429 without Retry-After", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &largeRetryAfterMaxDuration, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: None, + wantReqCount: 3, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Date, - responseSeconds: 2, - wantReqCount: 3, - }, { - name: "enabled 429 with Retry-After date max override", - config: &RetryConfig{ - RetryMax: 2, - RetryAfterEnabled: true, - RetryAfterMaxDuration: 1 * time.Second, + { + name: "large max 429 with Retry-After seconds", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &largeRetryAfterMaxDuration, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Seconds, + responseSeconds: 1, + wantReqCount: 3, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Date, - responseSeconds: 4, - wantReqCount: 3, - }, { - name: "enabled 429 with invalid Retry-After", - config: &RetryConfig{ - RetryMax: 2, - RetryAfterEnabled: true, + { + name: "large max 429 with Retry-After date", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &largeRetryAfterMaxDuration, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Date, + responseSeconds: 1, + wantReqCount: 3, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Invalid, - wantReqCount: 3, - }, { - name: "enabled 503 with Retry-After seconds", - config: &RetryConfig{ - RetryMax: 2, - RetryAfterEnabled: true, + { + name: "large max 429 with invalid Retry-After", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &largeRetryAfterMaxDuration, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Invalid, + wantReqCount: 3, }, - statusCode: http.StatusServiceUnavailable, - responseFormat: Seconds, - responseSeconds: 1, - wantReqCount: 3, - }, { - name: "enabled 503 with Retry-After date", - config: &RetryConfig{ - RetryMax: 2, - RetryAfterEnabled: true, + { + name: "large max 503 with Retry-After seconds", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &largeRetryAfterMaxDuration, + }, + statusCode: http.StatusServiceUnavailable, + responseFormat: Seconds, + responseSeconds: 1, + wantReqCount: 3, }, - statusCode: http.StatusServiceUnavailable, - responseFormat: Date, - responseSeconds: 2, - wantReqCount: 3, - }, { - name: "enabled 503 with invalid Retry-After", - config: &RetryConfig{ - RetryMax: 2, - RetryAfterEnabled: true, + { + name: "large max 500 without Retry-After", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &largeRetryAfterMaxDuration, + }, + statusCode: http.StatusInternalServerError, + responseFormat: None, + wantReqCount: 3, }, - statusCode: http.StatusServiceUnavailable, - responseFormat: Invalid, - wantReqCount: 3, - }} + + // + // Small Max Tests (Less Than Retry-After Value) + // + + { + name: "small max 429 without Retry-After", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &smallRetryAfterMaxDuration, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: None, + wantReqCount: 3, + }, + { + name: "small max 429 with Retry-After seconds", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &smallRetryAfterMaxDuration, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Seconds, + responseSeconds: 4, + wantReqCount: 3, + }, + { + name: "small max 429 with Retry-After date", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &smallRetryAfterMaxDuration, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Date, + responseSeconds: 2, + wantReqCount: 3, + }, + { + name: "small max 429 with invalid Retry-After", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &smallRetryAfterMaxDuration, + }, + statusCode: http.StatusTooManyRequests, + responseFormat: Invalid, + wantReqCount: 3, + }, + { + name: "small max 503 with Retry-After seconds", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &smallRetryAfterMaxDuration, + }, + statusCode: http.StatusServiceUnavailable, + responseFormat: Seconds, + responseSeconds: 4, + wantReqCount: 3, + }, + { + name: "small max 500 without Retry-After", + config: &RetryConfig{ + RetryMax: 2, + RetryAfterMaxDuration: &smallRetryAfterMaxDuration, + }, + statusCode: http.StatusInternalServerError, + responseFormat: None, + wantReqCount: 3, + }, + } // Execute The TestCases for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { // Consistent CheckRetry & Backoff Implementation For All TestCases - backoffDuration := 100 * time.Millisecond + paddingDuration := 250 * time.Millisecond // Test Execution Allowance For Resending Requests + backoffDuration := 500 * time.Millisecond // Constant Backoff Time For Simplified Testing tc.config.CheckRetry = SelectiveRetry tc.config.Backoff = func(attemptNum int, resp *http.Response) time.Duration { return backoffDuration } @@ -363,8 +451,13 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { // Determine Time Of Current Request currentReqTime := time.Now() - if tc.config.RetryAfterEnabled && tc.responseFormat == Date && tc.responseSeconds > 0 { - currentReqTime = currentReqTime.Round(time.Second) // Round When Using Date Format To Account For RFC850 Precision + // TODO - Remove this check when experimental-feature moves to Stable/GA to convert behavior from opt-in to opt-out + if tc.config.RetryAfterMaxDuration != nil { + + // TODO - Keep this logic as is (no change required) when experimental-feature is Stable/GA + if tc.responseFormat == Date && tc.responseSeconds > 0 { + currentReqTime = currentReqTime.Round(time.Second) // Round When Using Date Format To Account For RFC850 Precision + } } // Count The Requests @@ -373,7 +466,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { // Add Retry-After Header To Response Based On TestCase switch tc.responseFormat { case None: - // Don't Write Anything ; ) + // Don't Write Anything ;) case Seconds: writer.Header().Set(RetryAfterHeader, strconv.Itoa(int(responseDuration.Seconds()))) case Date: @@ -384,31 +477,45 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { assert.Fail(t, "TestCase with unsupported ResponseFormat '%v'", tc.responseFormat) } - // Calculate The Expected Request Duration Based On TestCase - expectedRequestDuration := backoffDuration - if tc.config.RetryAfterEnabled && responseDuration > 0 { - expectedRequestDuration = responseDuration - if tc.config.RetryAfterMaxDuration > 0 && tc.config.RetryAfterMaxDuration < expectedRequestDuration { - expectedRequestDuration = tc.config.RetryAfterMaxDuration + // Only Validate Timings Of Retries - Not Interested In Initial Request + if reqCount > 1 { + + // Calculate The Expected Maximum Request Duration Of TestCase + expectedMinRequestDuration := backoffDuration + // TODO - Remove this check when experimental-feature moves to Stable/GA to convert behavior from opt-in to opt-out + if tc.config.RetryAfterMaxDuration != nil { + + // TODO - Keep this logic as is (no change required) when experimental-feature is Stable/GA + if responseDuration > 0 { + expectedMinRequestDuration = responseDuration + if tc.config.RetryAfterMaxDuration != nil { + if *tc.config.RetryAfterMaxDuration == 0 { + expectedMinRequestDuration = backoffDuration + } else if *tc.config.RetryAfterMaxDuration < expectedMinRequestDuration { + expectedMinRequestDuration = *tc.config.RetryAfterMaxDuration + } + } + } } + expectedMaxRequestDuration := expectedMinRequestDuration + paddingDuration + + // Validate Inter-Request Durations Meet Or Exceed Expected Minimums + actualRequestDuration := currentReqTime.Sub(previousReqTime) + assert.GreaterOrEqual(t, actualRequestDuration, expectedMinRequestDuration, "previousReqTime =", previousReqTime.String(), "currentReqTime =", currentReqTime.String()) + assert.LessOrEqual(t, actualRequestDuration, expectedMaxRequestDuration, "previousReqTime =", previousReqTime.String(), "currentReqTime =", currentReqTime.String()) + t.Logf("Validated Request Duration %s between expected range %s - %s", + actualRequestDuration.String(), + expectedMinRequestDuration.String(), + expectedMaxRequestDuration.String()) } - // Validate Inter-Request Durations Meet Or Exceed Expected Minimums - actualRequestDuration := currentReqTime.Sub(previousReqTime) - assert.GreaterOrEqual(t, actualRequestDuration, expectedRequestDuration, "previousReqTime=%s, currentReqTime=%s", previousReqTime.String(), currentReqTime.String()) - // Respond With StatusCode From TestCase & Cycle The Times writer.WriteHeader(tc.statusCode) previousReqTime = currentReqTime })) - // Initialize The Previous Request Time Back Far Enough For Consistent Time Comparisons - previousReqTime = time.Now().Add(-2 * backoffDuration) - if responseDuration > 0 { - previousReqTime = previousReqTime.Add(-2 * responseDuration) - } - // Perform The Test - Generate And Send The Request + t.Logf("Testing %d Response With RetryAfter (%d) %ds & Max %+v", tc.statusCode, tc.responseFormat, tc.responseSeconds, tc.config.RetryAfterMaxDuration) sender := &HTTPMessageSender{Client: http.DefaultClient} request, err := http.NewRequest("POST", server.URL, nil) assert.Nil(t, err) diff --git a/pkg/kncloudevents/retries.go b/pkg/kncloudevents/retries.go index 9793a8a76be..4100077c798 100644 --- a/pkg/kncloudevents/retries.go +++ b/pkg/kncloudevents/retries.go @@ -68,16 +68,17 @@ type RetryConfig struct { // RequestTimeout represents the timeout of the single request RequestTimeout time.Duration - // Specify whether to handle "Retry-After" headers in 429/503 responses. - RetryAfterEnabled bool - RetryAfterMaxDuration time.Duration + // RetryAfterMaxDuration represents an optional override for the maximum + // value allowed for "Retry-After" headers in 429 / 503 responses. A nil + // value indicates no maximum override. A value of "0" indicates "Retry-After" + // headers are to be ignored. + RetryAfterMaxDuration *time.Duration } func NoRetries() RetryConfig { return noRetries } -// RetryConfigFromDeliverySpec returns a RetryConfig based on the specified DeliverySpec. func RetryConfigFromDeliverySpec(spec v1.DeliverySpec) (RetryConfig, error) { retryConfig := NoRetries() @@ -118,15 +119,13 @@ func RetryConfigFromDeliverySpec(spec v1.DeliverySpec) (RetryConfig, error) { retryConfig.RequestTimeout, _ = timeout.Duration() } - if spec.RetryAfter != nil { - retryConfig.RetryAfterEnabled = spec.RetryAfter.Enabled - if spec.RetryAfter.MaxDuration != nil && *spec.RetryAfter.MaxDuration != "" { - maxPeriod, err := period.Parse(*spec.RetryAfter.MaxDuration) - if err != nil { - return retryConfig, fmt.Errorf("failed to parse Spec.RetryAfter.MaxDuration: %w", err) - } - retryConfig.RetryAfterMaxDuration, _ = maxPeriod.Duration() + if spec.RetryAfterMax != nil { + maxPeriod, err := period.Parse(*spec.RetryAfterMax) + if err != nil { // Should never happen based on DeliverySpec validation + return retryConfig, fmt.Errorf("failed to parse Spec.RetryAfterMax: %w", err) } + maxDuration, _ := maxPeriod.Duration() + retryConfig.RetryAfterMaxDuration = &maxDuration } return retryConfig, nil diff --git a/pkg/kncloudevents/retries_test.go b/pkg/kncloudevents/retries_test.go index ef71a2b5dfe..fc706d3ac93 100644 --- a/pkg/kncloudevents/retries_test.go +++ b/pkg/kncloudevents/retries_test.go @@ -43,8 +43,7 @@ func TestNoRetries(t *testing.T) { assert.NotNil(t, retryConfig.Backoff) assert.Equal(t, time.Duration(0), retryConfig.Backoff(1, nil)) assert.Equal(t, time.Duration(0), retryConfig.Backoff(100, nil)) - assert.False(t, retryConfig.RetryAfterEnabled) - assert.Equal(t, time.Duration(0), retryConfig.RetryAfterMaxDuration) + assert.Nil(t, retryConfig.RetryAfterMaxDuration) } // Test The RetryConfigFromDeliverySpec() Functionality @@ -58,7 +57,7 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { backoffPolicy v1.BackoffPolicyType backoffDelay string timeout *string - retryAfter *v1.RetryAfter + retryAfterMax *string expectedBackoffDurations []time.Duration wantErr bool }{{ @@ -118,10 +117,10 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { timeout: &invalidISO8601DurationString, wantErr: true, }, { - name: "Valid Sparse Retry-After", + name: "Valid RetryAfterMax", backoffPolicy: v1.BackoffPolicyExponential, backoffDelay: "PT0.5S", - retryAfter: &v1.RetryAfter{Enabled: true}, + retryAfterMax: &validISO8601DurationString, expectedBackoffDurations: []time.Duration{ 1 * time.Second, 2 * time.Second, @@ -130,22 +129,10 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { 16 * time.Second, }, }, { - name: "Valid Complete Retry-After", + name: "Invalid RetryAfterMax", backoffPolicy: v1.BackoffPolicyExponential, backoffDelay: "PT0.5S", - retryAfter: &v1.RetryAfter{Enabled: true, MaxDuration: &validISO8601DurationString}, - expectedBackoffDurations: []time.Duration{ - 1 * time.Second, - 2 * time.Second, - 4 * time.Second, - 8 * time.Second, - 16 * time.Second, - }, - }, { - name: "Invalid Complete Retry-After", - backoffPolicy: v1.BackoffPolicyExponential, - backoffDelay: "PT0.5S", - retryAfter: &v1.RetryAfter{Enabled: true, MaxDuration: &invalidISO8601DurationString}, + retryAfterMax: &invalidISO8601DurationString, wantErr: true, }} @@ -159,7 +146,7 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { BackoffPolicy: &tc.backoffPolicy, BackoffDelay: &tc.backoffDelay, Timeout: tc.timeout, - RetryAfter: tc.retryAfter, + RetryAfterMax: tc.retryAfterMax, } // Create the RetryConfig from the deliverySpec @@ -174,19 +161,15 @@ func TestRetryConfigFromDeliverySpec(t *testing.T) { expectedTimeoutDuration, _ := expectedTimeoutPeriod.Duration() assert.Equal(t, expectedTimeoutDuration, retryConfig.RequestTimeout) } - if tc.retryAfter != nil { - assert.Equal(t, tc.retryAfter.Enabled, retryConfig.RetryAfterEnabled) - if tc.retryAfter.MaxDuration != nil && *tc.retryAfter.MaxDuration != "" { - expectedMaxPeriod, _ := period.Parse(*tc.retryAfter.MaxDuration) - expectedMaxDuration, _ := expectedMaxPeriod.Duration() - assert.Equal(t, expectedMaxDuration, retryConfig.RetryAfterMaxDuration) - } else { - assert.Equal(t, time.Duration(0), retryConfig.RetryAfterMaxDuration) - } + + if tc.retryAfterMax != nil && *tc.retryAfterMax != "" { + expectedMaxPeriod, _ := period.Parse(*tc.retryAfterMax) + expectedMaxDuration, _ := expectedMaxPeriod.Duration() + assert.Equal(t, expectedMaxDuration, *retryConfig.RetryAfterMaxDuration) } else { - assert.False(t, retryConfig.RetryAfterEnabled) - assert.Equal(t, time.Duration(0), retryConfig.RetryAfterMaxDuration) + assert.Nil(t, retryConfig.RetryAfterMaxDuration) } + for i := 1; i < retry; i++ { expectedBackoffDuration := tc.expectedBackoffDurations[i-1] actualBackoffDuration := retryConfig.Backoff(i, nil) From c28795b738ed326b63800a522928e8c6aa216949 Mon Sep 17 00:00:00 2001 From: I867318 Date: Mon, 8 Nov 2021 15:27:49 -0700 Subject: [PATCH 08/15] Refactor implementation for new design based on community feedback. --- pkg/reconciler/subscription/subscription.go | 8 ++--- .../subscription/subscription_test.go | 30 ++++--------------- 2 files changed, 10 insertions(+), 28 deletions(-) diff --git a/pkg/reconciler/subscription/subscription.go b/pkg/reconciler/subscription/subscription.go index 433a1faab51..b1ac06d1bd7 100644 --- a/pkg/reconciler/subscription/subscription.go +++ b/pkg/reconciler/subscription/subscription.go @@ -514,7 +514,7 @@ func deliverySpec(sub *v1.Subscription, channel *eventingduckv1.Channelable) (de channel.Spec.Delivery.Retry != nil || channel.Spec.Delivery.BackoffPolicy != nil || channel.Spec.Delivery.Timeout != nil || - channel.Spec.Delivery.RetryAfter != nil { + channel.Spec.Delivery.RetryAfterMax != nil { if delivery == nil { delivery = &eventingduckv1.DeliverySpec{} } @@ -522,7 +522,7 @@ func deliverySpec(sub *v1.Subscription, channel *eventingduckv1.Channelable) (de delivery.Retry = channel.Spec.Delivery.Retry delivery.BackoffDelay = channel.Spec.Delivery.BackoffDelay delivery.Timeout = channel.Spec.Delivery.Timeout - delivery.RetryAfter = channel.Spec.Delivery.RetryAfter + delivery.RetryAfterMax = channel.Spec.Delivery.RetryAfterMax } return } @@ -541,7 +541,7 @@ func deliverySpec(sub *v1.Subscription, channel *eventingduckv1.Channelable) (de sub.Spec.Delivery.Retry != nil || sub.Spec.Delivery.BackoffPolicy != nil || sub.Spec.Delivery.Timeout != nil || - sub.Spec.Delivery.RetryAfter != nil) { + sub.Spec.Delivery.RetryAfterMax != nil) { if delivery == nil { delivery = &eventingduckv1.DeliverySpec{} } @@ -549,7 +549,7 @@ func deliverySpec(sub *v1.Subscription, channel *eventingduckv1.Channelable) (de delivery.Retry = sub.Spec.Delivery.Retry delivery.BackoffDelay = sub.Spec.Delivery.BackoffDelay delivery.Timeout = sub.Spec.Delivery.Timeout - delivery.RetryAfter = sub.Spec.Delivery.RetryAfter + delivery.RetryAfterMax = sub.Spec.Delivery.RetryAfterMax } return } diff --git a/pkg/reconciler/subscription/subscription_test.go b/pkg/reconciler/subscription/subscription_test.go index 9547e2d5341..0d4152d6adc 100644 --- a/pkg/reconciler/subscription/subscription_test.go +++ b/pkg/reconciler/subscription/subscription_test.go @@ -1448,10 +1448,7 @@ func TestAllCases(t *testing.T) { BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), Timeout: pointer.StringPtr("PT2S"), - RetryAfter: &eventingduck.RetryAfter{ - Enabled: true, - MaxDuration: pointer.StringPtr("PT3S"), - }, + RetryAfterMax: pointer.StringPtr("PT3S"), }), ), NewService(serviceName, testNS), @@ -1488,10 +1485,7 @@ func TestAllCases(t *testing.T) { BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), Timeout: pointer.StringPtr("PT2S"), - RetryAfter: &eventingduck.RetryAfter{ - Enabled: true, - MaxDuration: pointer.StringPtr("PT3S"), - }, + RetryAfterMax: pointer.StringPtr("PT3S"), }, }, }), @@ -1522,10 +1516,7 @@ func TestAllCases(t *testing.T) { BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), Timeout: pointer.StringPtr("PT2S"), - RetryAfter: &eventingduck.RetryAfter{ - Enabled: true, - MaxDuration: pointer.StringPtr("PT3S"), - }, + RetryAfterMax: pointer.StringPtr("PT3S"), }), ), NewUnstructured(subscriberGVK, dlcName, testNS, @@ -1552,10 +1543,7 @@ func TestAllCases(t *testing.T) { BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT10S"), Timeout: pointer.StringPtr("PT20S"), - RetryAfter: &eventingduck.RetryAfter{ - Enabled: false, - MaxDuration: pointer.StringPtr("PT30S"), - }, + RetryAfterMax: pointer.StringPtr("PT30S"), }), ), NewService(serviceName, testNS), @@ -1589,10 +1577,7 @@ func TestAllCases(t *testing.T) { BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), Timeout: pointer.StringPtr("PT2S"), - RetryAfter: &eventingduck.RetryAfter{ - Enabled: true, - MaxDuration: pointer.StringPtr("PT3S"), - }, + RetryAfterMax: pointer.StringPtr("PT3S"), }), WithSubscriptionDeadLetterSinkURI(dlcURI), ), @@ -1610,10 +1595,7 @@ func TestAllCases(t *testing.T) { BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), Timeout: pointer.StringPtr("PT2S"), - RetryAfter: &eventingduck.RetryAfter{ - Enabled: true, - MaxDuration: pointer.StringPtr("PT3S"), - }, + RetryAfterMax: pointer.StringPtr("PT3S"), }, }, }), From 6bb059aeb3dcbbc871a9b8186e3b1de73403db92 Mon Sep 17 00:00:00 2001 From: I867318 Date: Mon, 8 Nov 2021 17:23:19 -0700 Subject: [PATCH 09/15] Refactor implementation for new design based on community feedback. --- test/experimental/features/retry_after/channel.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/test/experimental/features/retry_after/channel.go b/test/experimental/features/retry_after/channel.go index 7c9b9a5b11a..93385283166 100644 --- a/test/experimental/features/retry_after/channel.go +++ b/test/experimental/features/retry_after/channel.go @@ -140,10 +140,7 @@ func installRetryAfterSubscription(channelName, subscriptionName, sinkName strin Retry: pointer.Int32Ptr(retryAttempts), BackoffPolicy: &backoffPolicy, BackoffDelay: pointer.StringPtr("PT0.5S"), - RetryAfter: &eventingduckv1.RetryAfter{ - Enabled: true, - MaxDuration: pointer.StringPtr("PT30S"), - }, + RetryAfterMax: pointer.StringPtr("PT30S"), }, }, } From 6d5ec5ff42b0e0d7ed597c2719d7c7c208726992 Mon Sep 17 00:00:00 2001 From: I867318 Date: Fri, 12 Nov 2021 10:17:05 -0700 Subject: [PATCH 10/15] PR feedback --- docs/eventing-api.md | 4 +-- pkg/apis/duck/v1/delivery_types.go | 8 ++--- pkg/kncloudevents/message_sender.go | 45 +++++++++++++++++------------ 3 files changed, 32 insertions(+), 25 deletions(-) diff --git a/docs/eventing-api.md b/docs/eventing-api.md index d4460eea2fc..bb3e18b2a7c 100644 --- a/docs/eventing-api.md +++ b/docs/eventing-api.md @@ -406,8 +406,8 @@ when calculating backoff times for retrying 429 and 502 response codes. Setting zero (“PT0S”) can be used to opt-out of respecting “Retry-After” header values altogether. This value only takes effect if “Retry” is configured, and also depends on specific implementations (Channels, Sources, etc.) choosing to provide this capability.

-

Note: This API is EXPERIMENTAL and might be changed at anytime! While this experimental -feature is in the Alpha/Beta stage, you must provide a valid a value to opt-in to +

Note: This API is EXPERIMENTAL and might be changed at anytime. While this experimental +feature is in the Alpha/Beta stage, you must provide a valid value to opt-in for supporting “Retry-After” headers. When the feature becomes Stable/GA “Retry-After” headers will be respected by default, and you can choose to specify “PT0S” to opt-out of supporting “Retry-After” headers. diff --git a/pkg/apis/duck/v1/delivery_types.go b/pkg/apis/duck/v1/delivery_types.go index 5c4f5434940..4d1e007640f 100644 --- a/pkg/apis/duck/v1/delivery_types.go +++ b/pkg/apis/duck/v1/delivery_types.go @@ -68,8 +68,8 @@ type DeliverySpec struct { // value only takes effect if "Retry" is configured, and also depends on specific implementations // (Channels, Sources, etc.) choosing to provide this capability. // - // Note: This API is EXPERIMENTAL and might be changed at anytime! While this experimental - // feature is in the Alpha/Beta stage, you must provide a valid a value to opt-in to + // Note: This API is EXPERIMENTAL and might be changed at anytime. While this experimental + // feature is in the Alpha/Beta stage, you must provide a valid value to opt-in for // supporting "Retry-After" headers. When the feature becomes Stable/GA "Retry-After" // headers will be respected by default, and you can choose to specify "PT0S" to // opt-out of supporting "Retry-After" headers. @@ -125,8 +125,8 @@ func (ds *DeliverySpec) Validate(ctx context.Context) *apis.FieldError { if ds.RetryAfterMax != nil { if feature.FromContext(ctx).IsEnabled(feature.DeliveryRetryAfter) { - _, me := period.Parse(*ds.RetryAfterMax) - if me != nil { + p, me := period.Parse(*ds.RetryAfterMax) + if me != nil || p.IsNegative() { errs = errs.Also(apis.ErrInvalidValue(*ds.RetryAfterMax, "retryAfterMax")) } } else { diff --git a/pkg/kncloudevents/message_sender.go b/pkg/kncloudevents/message_sender.go index 26db6fc410a..7ca39375f13 100644 --- a/pkg/kncloudevents/message_sender.go +++ b/pkg/kncloudevents/message_sender.go @@ -143,9 +143,8 @@ func generateBackoffFn(config *RetryConfig) retryablehttp.Backoff { // Return The Larger Of The Two Backoff Durations if retryAfterDuration > backoffDuration { return retryAfterDuration - } else { - return backoffDuration } + return backoffDuration } } @@ -154,22 +153,30 @@ func generateBackoffFn(config *RetryConfig) retryablehttp.Backoff { // According to the spec (https://tools.ietf.org/html/rfc7231#section-7.1.3) // the Retry-After Header's value can be one of an HTTP-date or delay-seconds, // both of which are supported here. -func parseRetryAfterDuration(resp *nethttp.Response) time.Duration { - var retryAfterDuration time.Duration - if resp != nil && resp.Header != nil { - retryAfterString := resp.Header.Get(RetryAfterHeader) - if len(retryAfterString) > 0 { - if retryAfterInt, parseIntErr := strconv.ParseInt(retryAfterString, 10, 64); parseIntErr == nil { - retryAfterDuration = time.Duration(retryAfterInt) * time.Second - } else { - retryAfterTime, parseTimeErr := nethttp.ParseTime(retryAfterString) // Supports http.TimeFormat, time.RFC850 & time.ANSIC - if parseTimeErr != nil { - fmt.Printf("failed to parse Retry-After header: ParseInt Error = %v, ParseTime Error = %v", parseIntErr, parseTimeErr) - return retryAfterDuration - } - retryAfterDuration = time.Until(retryAfterTime) - } - } +func parseRetryAfterDuration(resp *nethttp.Response) (retryAfterDuration time.Duration) { + + // Return 0 Duration If No Response / Headers + if resp == nil || resp.Header == nil { + return + } + + // Return 0 Duration If No Retry-After Header + retryAfterString := resp.Header.Get(RetryAfterHeader) + if len(retryAfterString) <= 0 { + return + } + + // Attempt To Parse Retry-After Header As Seconds - Return If Successful + retryAfterInt, parseIntErr := strconv.ParseInt(retryAfterString, 10, 64) + if parseIntErr == nil { + return time.Duration(retryAfterInt) * time.Second + } + + // Attempt To Parse Retry-After Header As Timestamp (RFC850 & ANSIC) - Return If Successful + retryAfterTime, parseTimeErr := nethttp.ParseTime(retryAfterString) + if parseTimeErr != nil { + fmt.Printf("failed to parse Retry-After header: ParseInt Error = %v, ParseTime Error = %v", parseIntErr, parseTimeErr) + return } - return retryAfterDuration + return time.Until(retryAfterTime) } From 87be0ccae3cbc9c7e221ab24e233105b7228a722 Mon Sep 17 00:00:00 2001 From: I867318 Date: Wed, 17 Nov 2021 12:50:32 -0700 Subject: [PATCH 11/15] PR Feedback --- pkg/kncloudevents/message_sender.go | 4 +--- pkg/kncloudevents/message_sender_test.go | 13 ++++++++++++- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/pkg/kncloudevents/message_sender.go b/pkg/kncloudevents/message_sender.go index 7ca39375f13..2eeecef51f1 100644 --- a/pkg/kncloudevents/message_sender.go +++ b/pkg/kncloudevents/message_sender.go @@ -124,10 +124,8 @@ func generateBackoffFn(config *RetryConfig) retryablehttp.Backoff { // If Response is 429 / 503, Then Parse Any Retry-After Header Durations & Enforce Optional MaxDuration var retryAfterDuration time.Duration - // TODO - Remove this check when experimental-feature moves to Stable/GA to convert behavior from opt-in to opt-out if config.RetryAfterMaxDuration != nil { - // TODO - Keep this logic as is (no change required) when experimental-feature is Stable/GA if resp != nil && (resp.StatusCode == nethttp.StatusTooManyRequests || resp.StatusCode == nethttp.StatusServiceUnavailable) { retryAfterDuration = parseRetryAfterDuration(resp) @@ -175,7 +173,7 @@ func parseRetryAfterDuration(resp *nethttp.Response) (retryAfterDuration time.Du // Attempt To Parse Retry-After Header As Timestamp (RFC850 & ANSIC) - Return If Successful retryAfterTime, parseTimeErr := nethttp.ParseTime(retryAfterString) if parseTimeErr != nil { - fmt.Printf("failed to parse Retry-After header: ParseInt Error = %v, ParseTime Error = %v", parseIntErr, parseTimeErr) + fmt.Printf("failed to parse Retry-After header: ParseInt Error = %v, ParseTime Error = %v\n", parseIntErr, parseTimeErr) return } return time.Until(retryAfterTime) diff --git a/pkg/kncloudevents/message_sender_test.go b/pkg/kncloudevents/message_sender_test.go index 23d89050546..28b75ebfc47 100644 --- a/pkg/kncloudevents/message_sender_test.go +++ b/pkg/kncloudevents/message_sender_test.go @@ -196,8 +196,17 @@ func TestHTTPMessageSenderSendWithRetriesWithSingleRequestTimeout(t *testing.T) require.Equal(t, http.StatusOK, got.StatusCode) } +/* + * Test Retry-After Header Enforcement + * + * Note - This test is a bit complicated in that it utilizes a custom + * test Server which verifies subsequent retry attempts are + * within an expected time window. It is therefore inherently + * "time" sensitive and could become brittle. The timings + * were chosen as a balance of stability and test execution + * speed, but could require adjustment. + */ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { - t.Parallel() // Create A Quick Enum For RetryAfter Format type RetryAfterFormat int @@ -424,6 +433,8 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { + t.Parallel() + // Consistent CheckRetry & Backoff Implementation For All TestCases paddingDuration := 250 * time.Millisecond // Test Execution Allowance For Resending Requests backoffDuration := 500 * time.Millisecond // Constant Backoff Time For Simplified Testing From 094a102cd654c02b38aab693bf25c8cea0c59e46 Mon Sep 17 00:00:00 2001 From: I867318 Date: Wed, 17 Nov 2021 13:58:23 -0700 Subject: [PATCH 12/15] fix parallel data race --- pkg/kncloudevents/message_sender_test.go | 47 +++++++++++++----------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/pkg/kncloudevents/message_sender_test.go b/pkg/kncloudevents/message_sender_test.go index 28b75ebfc47..eba716fec67 100644 --- a/pkg/kncloudevents/message_sender_test.go +++ b/pkg/kncloudevents/message_sender_test.go @@ -224,7 +224,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { // Define The TestCases tests := []struct { name string - config *RetryConfig // The RetryConfig to use in for the test case. + config RetryConfig // The RetryConfig to use in for the test case. statusCode int // The HTTP StatusCode which the Server should return. responseFormat RetryAfterFormat // Indicates the format in which the Server should return the Retry-After header. responseSeconds int // Indicates the number of Seconds for the Server to use when returning the Retry-After header. @@ -236,7 +236,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { { name: "default max 429 without Retry-After", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, }, statusCode: http.StatusTooManyRequests, @@ -245,7 +245,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "default max 429 with Retry-After seconds", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, }, statusCode: http.StatusTooManyRequests, @@ -255,7 +255,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "default max 429 with Retry-After date", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, }, statusCode: http.StatusTooManyRequests, @@ -265,7 +265,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "default max 429 with invalid Retry-After", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, }, statusCode: http.StatusTooManyRequests, @@ -274,7 +274,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "default max 503 with Retry-After seconds", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, }, statusCode: http.StatusServiceUnavailable, @@ -284,7 +284,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "default max 500 without Retry-After", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, }, statusCode: http.StatusInternalServerError, @@ -298,7 +298,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { { name: "large max 429 without Retry-After", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &largeRetryAfterMaxDuration, }, @@ -308,7 +308,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "large max 429 with Retry-After seconds", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &largeRetryAfterMaxDuration, }, @@ -319,7 +319,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "large max 429 with Retry-After date", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &largeRetryAfterMaxDuration, }, @@ -330,7 +330,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "large max 429 with invalid Retry-After", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &largeRetryAfterMaxDuration, }, @@ -340,7 +340,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "large max 503 with Retry-After seconds", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &largeRetryAfterMaxDuration, }, @@ -351,7 +351,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "large max 500 without Retry-After", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &largeRetryAfterMaxDuration, }, @@ -366,7 +366,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { { name: "small max 429 without Retry-After", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &smallRetryAfterMaxDuration, }, @@ -376,7 +376,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "small max 429 with Retry-After seconds", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &smallRetryAfterMaxDuration, }, @@ -387,7 +387,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "small max 429 with Retry-After date", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &smallRetryAfterMaxDuration, }, @@ -398,7 +398,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "small max 429 with invalid Retry-After", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &smallRetryAfterMaxDuration, }, @@ -408,7 +408,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "small max 503 with Retry-After seconds", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &smallRetryAfterMaxDuration, }, @@ -419,7 +419,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { }, { name: "small max 500 without Retry-After", - config: &RetryConfig{ + config: RetryConfig{ RetryMax: 2, RetryAfterMaxDuration: &smallRetryAfterMaxDuration, }, @@ -430,11 +430,14 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { } // Execute The TestCases - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { t.Parallel() + // Clone The TestCase (Parallel Execution Race Protection) + tc := test + // Consistent CheckRetry & Backoff Implementation For All TestCases paddingDuration := 250 * time.Millisecond // Test Execution Allowance For Resending Requests backoffDuration := 500 * time.Millisecond // Constant Backoff Time For Simplified Testing @@ -530,7 +533,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { sender := &HTTPMessageSender{Client: http.DefaultClient} request, err := http.NewRequest("POST", server.URL, nil) assert.Nil(t, err) - got, err := sender.SendWithRetries(request, tc.config) + got, err := sender.SendWithRetries(request, &tc.config) // Verify Final Results if err != nil { From 1602941a0ffb259b4afda1ddb6614e1f54c282e5 Mon Sep 17 00:00:00 2001 From: I867318 Date: Wed, 17 Nov 2021 15:34:16 -0700 Subject: [PATCH 13/15] PR Feedback --- pkg/kncloudevents/message_sender_test.go | 110 ++++++------- .../subscription/subscription_test.go | 150 +++++++++++++++--- 2 files changed, 179 insertions(+), 81 deletions(-) diff --git a/pkg/kncloudevents/message_sender_test.go b/pkg/kncloudevents/message_sender_test.go index eba716fec67..53f5d1e1c6a 100644 --- a/pkg/kncloudevents/message_sender_test.go +++ b/pkg/kncloudevents/message_sender_test.go @@ -223,15 +223,15 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { // Define The TestCases tests := []struct { - name string - config RetryConfig // The RetryConfig to use in for the test case. - statusCode int // The HTTP StatusCode which the Server should return. - responseFormat RetryAfterFormat // Indicates the format in which the Server should return the Retry-After header. - responseSeconds int // Indicates the number of Seconds for the Server to use when returning the Retry-After header. - wantReqCount int // The total number of expected requests, including initial request. + name string + config RetryConfig // The RetryConfig to use in for the test case. + statusCode int // The HTTP StatusCode which the Server should return. + responseFormat RetryAfterFormat // Indicates the format in which the Server should return the Retry-After header. + responseDuration time.Duration // Indicates the Duration for the Server to use when returning the Retry-After header. + wantReqCount int // The total number of expected requests, including initial request. }{ // - // Default Max Tests (0 Duration opt-in vs opt-out) + // Default Max Tests (No RetryAfterMax Specified - opt-in vs opt-out) // { @@ -248,20 +248,20 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { config: RetryConfig{ RetryMax: 2, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Seconds, - responseSeconds: 1, - wantReqCount: 3, + statusCode: http.StatusTooManyRequests, + responseFormat: Seconds, + responseDuration: 1 * time.Second, + wantReqCount: 3, }, { name: "default max 429 with Retry-After date", config: RetryConfig{ RetryMax: 2, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Date, - responseSeconds: 2, - wantReqCount: 3, + statusCode: http.StatusTooManyRequests, + responseFormat: Date, + responseDuration: 2 * time.Second, + wantReqCount: 3, }, { name: "default max 429 with invalid Retry-After", @@ -277,10 +277,10 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { config: RetryConfig{ RetryMax: 2, }, - statusCode: http.StatusServiceUnavailable, - responseFormat: Seconds, - responseSeconds: 1, - wantReqCount: 3, + statusCode: http.StatusServiceUnavailable, + responseFormat: Seconds, + responseDuration: 1 * time.Second, + wantReqCount: 3, }, { name: "default max 500 without Retry-After", @@ -312,10 +312,10 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { RetryMax: 2, RetryAfterMaxDuration: &largeRetryAfterMaxDuration, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Seconds, - responseSeconds: 1, - wantReqCount: 3, + statusCode: http.StatusTooManyRequests, + responseFormat: Seconds, + responseDuration: 1 * time.Second, + wantReqCount: 3, }, { name: "large max 429 with Retry-After date", @@ -323,10 +323,10 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { RetryMax: 2, RetryAfterMaxDuration: &largeRetryAfterMaxDuration, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Date, - responseSeconds: 1, - wantReqCount: 3, + statusCode: http.StatusTooManyRequests, + responseFormat: Date, + responseDuration: 1 * time.Second, + wantReqCount: 3, }, { name: "large max 429 with invalid Retry-After", @@ -344,10 +344,10 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { RetryMax: 2, RetryAfterMaxDuration: &largeRetryAfterMaxDuration, }, - statusCode: http.StatusServiceUnavailable, - responseFormat: Seconds, - responseSeconds: 1, - wantReqCount: 3, + statusCode: http.StatusServiceUnavailable, + responseFormat: Seconds, + responseDuration: 1 * time.Second, + wantReqCount: 3, }, { name: "large max 500 without Retry-After", @@ -380,10 +380,10 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { RetryMax: 2, RetryAfterMaxDuration: &smallRetryAfterMaxDuration, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Seconds, - responseSeconds: 4, - wantReqCount: 3, + statusCode: http.StatusTooManyRequests, + responseFormat: Seconds, + responseDuration: 4 * time.Second, + wantReqCount: 3, }, { name: "small max 429 with Retry-After date", @@ -391,10 +391,10 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { RetryMax: 2, RetryAfterMaxDuration: &smallRetryAfterMaxDuration, }, - statusCode: http.StatusTooManyRequests, - responseFormat: Date, - responseSeconds: 2, - wantReqCount: 3, + statusCode: http.StatusTooManyRequests, + responseFormat: Date, + responseDuration: 2 * time.Second, + wantReqCount: 3, }, { name: "small max 429 with invalid Retry-After", @@ -412,10 +412,10 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { RetryMax: 2, RetryAfterMaxDuration: &smallRetryAfterMaxDuration, }, - statusCode: http.StatusServiceUnavailable, - responseFormat: Seconds, - responseSeconds: 4, - wantReqCount: 3, + statusCode: http.StatusServiceUnavailable, + responseFormat: Seconds, + responseDuration: 4 * time.Second, + wantReqCount: 3, }, { name: "small max 500 without Retry-After", @@ -444,18 +444,6 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { tc.config.CheckRetry = SelectiveRetry tc.config.Backoff = func(attemptNum int, resp *http.Response) time.Duration { return backoffDuration } - // Determine The Response Retry-After Backoff Duration From TestCase - var responseDuration time.Duration - switch tc.responseFormat { - case None, Invalid: - case Seconds, Date: - if tc.responseSeconds > 0 { - responseDuration = time.Duration(tc.responseSeconds) * time.Second - } - default: - assert.Fail(t, "TestCase with unsupported ResponseFormat '%v'", tc.responseFormat) - } - // Tracking Variables var previousReqTime time.Time var reqCount int32 @@ -469,7 +457,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { if tc.config.RetryAfterMaxDuration != nil { // TODO - Keep this logic as is (no change required) when experimental-feature is Stable/GA - if tc.responseFormat == Date && tc.responseSeconds > 0 { + if tc.responseFormat == Date && tc.responseDuration > 0 { currentReqTime = currentReqTime.Round(time.Second) // Round When Using Date Format To Account For RFC850 Precision } } @@ -482,9 +470,9 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { case None: // Don't Write Anything ;) case Seconds: - writer.Header().Set(RetryAfterHeader, strconv.Itoa(int(responseDuration.Seconds()))) + writer.Header().Set(RetryAfterHeader, strconv.Itoa(int(tc.responseDuration.Seconds()))) case Date: - writer.Header().Set(RetryAfterHeader, currentReqTime.Add(responseDuration).Format(time.RFC850)) // RFC850 Drops Millis + writer.Header().Set(RetryAfterHeader, currentReqTime.Add(tc.responseDuration).Format(time.RFC850)) // RFC850 Drops Millis case Invalid: writer.Header().Set(RetryAfterHeader, "FOO") default: @@ -500,8 +488,8 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { if tc.config.RetryAfterMaxDuration != nil { // TODO - Keep this logic as is (no change required) when experimental-feature is Stable/GA - if responseDuration > 0 { - expectedMinRequestDuration = responseDuration + if tc.responseDuration > 0 { + expectedMinRequestDuration = tc.responseDuration if tc.config.RetryAfterMaxDuration != nil { if *tc.config.RetryAfterMaxDuration == 0 { expectedMinRequestDuration = backoffDuration @@ -529,7 +517,7 @@ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { })) // Perform The Test - Generate And Send The Request - t.Logf("Testing %d Response With RetryAfter (%d) %ds & Max %+v", tc.statusCode, tc.responseFormat, tc.responseSeconds, tc.config.RetryAfterMaxDuration) + t.Logf("Testing %d Response With RetryAfter (%d) %fs & Max %+v", tc.statusCode, tc.responseFormat, tc.responseDuration.Seconds(), tc.config.RetryAfterMaxDuration) sender := &HTTPMessageSender{Client: http.DefaultClient} request, err := http.NewRequest("POST", server.URL, nil) assert.Nil(t, err) diff --git a/pkg/reconciler/subscription/subscription_test.go b/pkg/reconciler/subscription/subscription_test.go index 47199652b79..af5bbc68838 100644 --- a/pkg/reconciler/subscription/subscription_test.go +++ b/pkg/reconciler/subscription/subscription_test.go @@ -1421,10 +1421,6 @@ func TestAllCases(t *testing.T) { }, { Name: "v1 imc - delivery defaulting - full delivery spec", - Ctx: feature.ToContext(context.TODO(), feature.Flags{ - feature.DeliveryTimeout: feature.Enabled, - feature.DeliveryRetryAfter: feature.Enabled, - }), Objects: []runtime.Object{ NewSubscription("a-"+subscriptionName, testNS, WithSubscriptionUID("a-"+subscriptionUID), @@ -1451,8 +1447,6 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), - Timeout: pointer.StringPtr("PT2S"), - RetryAfterMax: pointer.StringPtr("PT3S"), }), WithInMemoryChannelStatusDLSURI(dlcURI), ), @@ -1489,8 +1483,66 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), - Timeout: pointer.StringPtr("PT2S"), - RetryAfterMax: pointer.StringPtr("PT3S"), + }, + }, + }), + patchFinalizers(testNS, "a-"+subscriptionName), + }, + }, + { + Name: "v1 imc - delivery defaulting - optional features", + Ctx: feature.ToContext(context.TODO(), feature.Flags{ + feature.DeliveryTimeout: feature.Enabled, + feature.DeliveryRetryAfter: feature.Enabled, + }), + Objects: []runtime.Object{ + NewSubscription("a-"+subscriptionName, testNS, + WithSubscriptionUID("a-"+subscriptionUID), + WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(serviceGVK, serviceName, testNS), + ), + NewUnstructured(subscriberGVK, dlcName, testNS, + WithUnstructuredAddressable(dlcDNS), + ), + NewInMemoryChannel(channelName, testNS, + WithInitInMemoryChannelConditions, + WithInMemoryChannelSubscribers(nil), + WithInMemoryChannelAddress(channelDNS), + WithInMemoryChannelReadySubscriber("a-"+subscriptionUID), + WithInMemoryChannelDelivery(&eventingduck.DeliverySpec{ + Timeout: pointer.StringPtr("PT1S"), + RetryAfterMax: pointer.StringPtr("PT2S"), + }), + WithInMemoryChannelStatusDLSURI(dlcURI), + ), + NewService(serviceName, testNS), + }, + Key: testNS + "/" + "a-" + subscriptionName, + WantErr: false, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", "a-"+subscriptionName), + Eventf(corev1.EventTypeNormal, "SubscriberSync", "Subscription was synchronized to channel %q", channelName), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: NewSubscription("a-"+subscriptionName, testNS, + WithSubscriptionUID("a-"+subscriptionUID), + WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(serviceGVK, serviceName, testNS), + // The first reconciliation will initialize the status conditions. + WithInitSubscriptionConditions, + MarkReferencesResolved, + MarkAddedToChannel, + WithSubscriptionPhysicalSubscriptionSubscriber(serviceURI), + ), + }}, + WantPatches: []clientgotesting.PatchActionImpl{ + patchSubscribers(testNS, channelName, []eventingduck.SubscriberSpec{ + { + UID: "a-" + subscriptionUID, + SubscriberURI: serviceURI, + Delivery: &eventingduck.DeliverySpec{ + Timeout: pointer.StringPtr("PT1S"), + RetryAfterMax: pointer.StringPtr("PT2S"), }, }, }), @@ -1598,10 +1650,6 @@ func TestAllCases(t *testing.T) { }, { Name: "v1 imc - don't default delivery - full delivery spec", - Ctx: feature.ToContext(context.TODO(), feature.Flags{ - feature.DeliveryTimeout: feature.Enabled, - feature.DeliveryRetryAfter: feature.Enabled, - }), Objects: []runtime.Object{ NewSubscription("a-"+subscriptionName, testNS, WithSubscriptionUID("a-"+subscriptionUID), @@ -1619,8 +1667,6 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), - Timeout: pointer.StringPtr("PT2S"), - RetryAfterMax: pointer.StringPtr("PT3S"), }), ), NewUnstructured(subscriberGVK, dlsName, testNS, @@ -1646,8 +1692,6 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(20), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT10S"), - Timeout: pointer.StringPtr("PT20S"), - RetryAfterMax: pointer.StringPtr("PT30S"), }), ), NewService(serviceName, testNS), @@ -1680,8 +1724,6 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), - Timeout: pointer.StringPtr("PT2S"), - RetryAfterMax: pointer.StringPtr("PT3S"), }), WithSubscriptionDeadLetterSinkURI(dlsURI), ), @@ -1698,8 +1740,76 @@ func TestAllCases(t *testing.T) { Retry: pointer.Int32Ptr(10), BackoffPolicy: &linear, BackoffDelay: pointer.StringPtr("PT1S"), - Timeout: pointer.StringPtr("PT2S"), - RetryAfterMax: pointer.StringPtr("PT3S"), + }, + }, + }), + patchFinalizers(testNS, "a-"+subscriptionName), + }, + }, + { + Name: "v1 imc - don't default delivery - optional features", + Ctx: feature.ToContext(context.TODO(), feature.Flags{ + feature.DeliveryTimeout: feature.Enabled, + feature.DeliveryRetryAfter: feature.Enabled, + }), + Objects: []runtime.Object{ + NewSubscription("a-"+subscriptionName, testNS, + WithSubscriptionUID("a-"+subscriptionUID), + WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(serviceGVK, serviceName, testNS), + WithSubscriptionDeliverySpec(&eventingduck.DeliverySpec{ + Timeout: pointer.StringPtr("PT1S"), + RetryAfterMax: pointer.StringPtr("PT2S"), + }), + ), + NewUnstructured(subscriberGVK, dlsName, testNS, + WithUnstructuredAddressable(dlsDNS), + ), + NewUnstructured(subscriberGVK, dlc2Name, testNS, + WithUnstructuredAddressable(dlc2DNS), + ), + NewInMemoryChannel(channelName, testNS, + WithInitInMemoryChannelConditions, + WithInMemoryChannelSubscribers(nil), + WithInMemoryChannelAddress(channelDNS), + WithInMemoryChannelReadySubscriber("a-"+subscriptionUID), + WithInMemoryChannelDelivery(&eventingduck.DeliverySpec{ + Timeout: pointer.StringPtr("PT10S"), + RetryAfterMax: pointer.StringPtr("PT20S"), + }), + ), + NewService(serviceName, testNS), + }, + Key: testNS + "/" + "a-" + subscriptionName, + WantErr: false, + WantEvents: []string{ + Eventf(corev1.EventTypeNormal, "FinalizerUpdate", "Updated %q finalizers", "a-"+subscriptionName), + Eventf(corev1.EventTypeNormal, "SubscriberSync", "Subscription was synchronized to channel %q", channelName), + }, + WantStatusUpdates: []clientgotesting.UpdateActionImpl{{ + Object: NewSubscription("a-"+subscriptionName, testNS, + WithSubscriptionUID("a-"+subscriptionUID), + WithSubscriptionChannel(imcV1GVK, channelName), + WithSubscriptionSubscriberRef(serviceGVK, serviceName, testNS), + // The first reconciliation will initialize the status conditions. + WithInitSubscriptionConditions, + MarkReferencesResolved, + MarkAddedToChannel, + WithSubscriptionPhysicalSubscriptionSubscriber(serviceURI), + WithSubscriptionDeliverySpec(&eventingduck.DeliverySpec{ + Timeout: pointer.StringPtr("PT1S"), + RetryAfterMax: pointer.StringPtr("PT2S"), + }), + ), + }}, + WantPatches: []clientgotesting.PatchActionImpl{ + patchSubscribers(testNS, channelName, []eventingduck.SubscriberSpec{ + { + UID: "a-" + subscriptionUID, + SubscriberURI: serviceURI, + Delivery: &eventingduck.DeliverySpec{ + Timeout: pointer.StringPtr("PT1S"), + RetryAfterMax: pointer.StringPtr("PT2S"), }, }, }), From 2eab560a582807f76d514918800a69c557c2e069 Mon Sep 17 00:00:00 2001 From: I867318 Date: Fri, 19 Nov 2021 11:47:45 -0700 Subject: [PATCH 14/15] Refactor RetryAfter message_sender test --- pkg/kncloudevents/message_sender_test.go | 505 +++++++++++------------ 1 file changed, 233 insertions(+), 272 deletions(-) diff --git a/pkg/kncloudevents/message_sender_test.go b/pkg/kncloudevents/message_sender_test.go index 53f5d1e1c6a..deffa576d54 100644 --- a/pkg/kncloudevents/message_sender_test.go +++ b/pkg/kncloudevents/message_sender_test.go @@ -196,343 +196,304 @@ func TestHTTPMessageSenderSendWithRetriesWithSingleRequestTimeout(t *testing.T) require.Equal(t, http.StatusOK, got.StatusCode) } +// RetryAfterFormat Enum +type RetryAfterFormat int + +const ( + None RetryAfterFormat = iota // RetryAfter Format Enum Values + Seconds + Date + Invalid + + retryBackoffDuration = 500 * time.Millisecond // Constant (vs linear/exponential) backoff Duration for simpler validation. + paddingDuration = 250 * time.Millisecond // Buffer time to allow test execution to actually send the requests. +) + +// RetryAfterValidationServer wraps a standard HTTP test server with tracking/validation logic. +type RetryAfterValidationServer struct { + *httptest.Server // Wrapped Golang HTTP Test Server. + previousReqTime time.Time // Tracking request times to validate retry intervals. + requestCount int32 // Tracking total requests for external validation of retry attempts. + minRequestDuration time.Duration // Expected minimum request interval duration. + maxRequestDuration time.Duration // Expected maximum request interval duration. +} + +// newRetryAfterValidationServer returns a new RetryAfterValidationServer with the +// specified configuration. The server tracks total request counts and validates +// inter-request durations to ensure they confirm to the expected backoff behavior. +func newRetryAfterValidationServer(t *testing.T, statusCode int, retryAfterFormat RetryAfterFormat, retryAfterDuration time.Duration, requestDuration time.Duration) *RetryAfterValidationServer { + + server := &RetryAfterValidationServer{ + minRequestDuration: requestDuration, + maxRequestDuration: requestDuration + paddingDuration, + } + + server.Server = httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + + // Determine Time Of Current Request + currentReqTime := time.Now() + // TODO - Remove this check when experimental-feature moves to Stable/GA to convert behavior from opt-in to opt-out + if server.minRequestDuration > retryBackoffDuration { + // TODO - Keep this logic as is (no change required) when experimental-feature moves to Stable/GA + if retryAfterFormat == Date && retryAfterDuration > 0 { + currentReqTime = currentReqTime.Round(time.Second) // Round When Using Date Format To Account For RFC850 Precision + } + } + + // Count The Requests + atomic.AddInt32(&server.requestCount, 1) + + // Add Retry-After Header To Response Based On Configured Format + switch retryAfterFormat { + case None: + // Don't Write Anything ;) + case Seconds: + writer.Header().Set(RetryAfterHeader, strconv.Itoa(int(retryAfterDuration.Seconds()))) + case Date: + writer.Header().Set(RetryAfterHeader, currentReqTime.Add(retryAfterDuration).Format(time.RFC850)) // RFC850 Drops Millis + case Invalid: + writer.Header().Set(RetryAfterHeader, "FOO") + default: + assert.Fail(t, "TestCase with unsupported ResponseFormat '%v'", retryAfterFormat) + } + + // Only Validate Timings Of Retries (Skip Initial Request) + if server.requestCount > 1 { + + // Validate Inter-Request Durations Meet Or Exceed Expected Minimums + actualRequestDuration := currentReqTime.Sub(server.previousReqTime) + assert.GreaterOrEqual(t, actualRequestDuration, server.minRequestDuration, "previousReqTime =", server.previousReqTime.String(), "currentReqTime =", currentReqTime.String()) + assert.LessOrEqual(t, actualRequestDuration, server.maxRequestDuration, "previousReqTime =", server.previousReqTime.String(), "currentReqTime =", currentReqTime.String()) + t.Logf("Validated Request Duration %s between expected range %s - %s", + actualRequestDuration.String(), + server.minRequestDuration.String(), + server.maxRequestDuration.String()) + } + + // Respond With StatusCode & Cycle The Times + writer.WriteHeader(statusCode) + server.previousReqTime = currentReqTime + })) + + return server +} + /* * Test Retry-After Header Enforcement * - * Note - This test is a bit complicated in that it utilizes a custom - * test Server which verifies subsequent retry attempts are - * within an expected time window. It is therefore inherently - * "time" sensitive and could become brittle. The timings - * were chosen as a balance of stability and test execution - * speed, but could require adjustment. + * This test validates that SendWithRetries() is correctly enforcing + * the Retry-After headers based on RetryConfig.RetryAfterMaxDuration. + * It does this be creating a test HTTP Server which responds with + * the desired StatusCode and Retry-After header. The server also + * validates the retry request intervals to ensure they fall within + * the expected time window. The timings were chosen as a balance of + * stability and test execution speed, but could require adjustment. */ func TestHTTPMessageSenderSendWithRetriesWithRetryAfter(t *testing.T) { - // Create A Quick Enum For RetryAfter Format - type RetryAfterFormat int - const ( - None RetryAfterFormat = iota - Seconds - Date - Invalid - ) - // Test Data - smallRetryAfterMaxDuration := 1 * time.Second - largeRetryAfterMaxDuration := 10 * time.Second + retryMax := int32(2) // Perform a couple of retries to be sure. + smallRetryAfterMaxDuration := 1 * time.Second // Value must exceed retryBackoffDuration while being less than retryAfterDuration so that the retryAfterMax value is used. + largeRetryAfterMaxDuration := 10 * time.Second // Value must exceed retryBackoffDuration and retryAfterDuration so that Retry-After header is used. // Define The TestCases - tests := []struct { - name string - config RetryConfig // The RetryConfig to use in for the test case. - statusCode int // The HTTP StatusCode which the Server should return. - responseFormat RetryAfterFormat // Indicates the format in which the Server should return the Retry-After header. - responseDuration time.Duration // Indicates the Duration for the Server to use when returning the Retry-After header. - wantReqCount int // The total number of expected requests, including initial request. + testCases := []struct { + name string + statusCode int // HTTP StatusCode which the server should return. + retryAfterFormat RetryAfterFormat // Format in which the server should return Retry-After headers. + retryAfterDuration time.Duration // Duration of the Retry-After header returned by the server. + retryAfterMaxDuration *time.Duration // DeliverySpec RetryAfterMax Duration used to calculate expected retry interval. + wantRequestDuration time.Duration // Expected minimum Request interval Duration. }{ - // - // Default Max Tests (No RetryAfterMax Specified - opt-in vs opt-out) - // + + // Nil Max Tests (opt-in / opt-out) { - name: "default max 429 without Retry-After", - config: RetryConfig{ - RetryMax: 2, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: None, - wantReqCount: 3, + name: "default max 429 without Retry-After", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: None, + retryAfterDuration: 0 * time.Second, + retryAfterMaxDuration: nil, + wantRequestDuration: retryBackoffDuration, }, { - name: "default max 429 with Retry-After seconds", - config: RetryConfig{ - RetryMax: 2, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: Seconds, - responseDuration: 1 * time.Second, - wantReqCount: 3, + name: "default max 429 with Retry-After seconds", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: Seconds, + retryAfterDuration: 1 * time.Second, + retryAfterMaxDuration: nil, + wantRequestDuration: retryBackoffDuration, // TODO - Update when experimental-feature moves to Stable/GA }, { - name: "default max 429 with Retry-After date", - config: RetryConfig{ - RetryMax: 2, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: Date, - responseDuration: 2 * time.Second, - wantReqCount: 3, + name: "default max 429 with Retry-After date", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: Date, + retryAfterDuration: 2 * time.Second, + retryAfterMaxDuration: nil, + wantRequestDuration: retryBackoffDuration, // TODO - Update when experimental-feature moves to Stable/GA }, { - name: "default max 429 with invalid Retry-After", - config: RetryConfig{ - RetryMax: 2, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: Invalid, - wantReqCount: 3, + name: "default max 429 with invalid Retry-After", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: Invalid, + retryAfterDuration: 0 * time.Second, + retryAfterMaxDuration: nil, + wantRequestDuration: retryBackoffDuration, }, { - name: "default max 503 with Retry-After seconds", - config: RetryConfig{ - RetryMax: 2, - }, - statusCode: http.StatusServiceUnavailable, - responseFormat: Seconds, - responseDuration: 1 * time.Second, - wantReqCount: 3, + name: "default max 503 with Retry-After seconds", + statusCode: http.StatusServiceUnavailable, + retryAfterFormat: Seconds, + retryAfterDuration: 1 * time.Second, + retryAfterMaxDuration: nil, + wantRequestDuration: retryBackoffDuration, // TODO - Update when experimental-feature moves to Stable/GA }, { - name: "default max 500 without Retry-After", - config: RetryConfig{ - RetryMax: 2, - }, - statusCode: http.StatusInternalServerError, - responseFormat: None, - wantReqCount: 3, + name: "default max 500 without Retry-After", + statusCode: http.StatusInternalServerError, + retryAfterFormat: None, + retryAfterDuration: 0 * time.Second, + retryAfterMaxDuration: nil, + wantRequestDuration: retryBackoffDuration, }, - // // Large Max Tests (Greater Than Retry-After Value) - // { - name: "large max 429 without Retry-After", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &largeRetryAfterMaxDuration, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: None, - wantReqCount: 3, + name: "large max 429 without Retry-After", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: None, + retryAfterDuration: 0 * time.Second, + retryAfterMaxDuration: &largeRetryAfterMaxDuration, + wantRequestDuration: retryBackoffDuration, }, { - name: "large max 429 with Retry-After seconds", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &largeRetryAfterMaxDuration, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: Seconds, - responseDuration: 1 * time.Second, - wantReqCount: 3, + name: "large max 429 with Retry-After seconds", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: Seconds, + retryAfterDuration: 1 * time.Second, + retryAfterMaxDuration: &largeRetryAfterMaxDuration, + wantRequestDuration: 1 * time.Second, }, { - name: "large max 429 with Retry-After date", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &largeRetryAfterMaxDuration, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: Date, - responseDuration: 1 * time.Second, - wantReqCount: 3, + name: "large max 429 with Retry-After date", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: Date, + retryAfterDuration: 1 * time.Second, + retryAfterMaxDuration: &largeRetryAfterMaxDuration, + wantRequestDuration: 1 * time.Second, }, { - name: "large max 429 with invalid Retry-After", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &largeRetryAfterMaxDuration, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: Invalid, - wantReqCount: 3, + name: "large max 429 with invalid Retry-After", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: Invalid, + retryAfterDuration: 0 * time.Second, + retryAfterMaxDuration: &largeRetryAfterMaxDuration, + wantRequestDuration: retryBackoffDuration, }, { - name: "large max 503 with Retry-After seconds", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &largeRetryAfterMaxDuration, - }, - statusCode: http.StatusServiceUnavailable, - responseFormat: Seconds, - responseDuration: 1 * time.Second, - wantReqCount: 3, + name: "large max 503 with Retry-After seconds", + statusCode: http.StatusServiceUnavailable, + retryAfterFormat: Seconds, + retryAfterDuration: 1 * time.Second, + retryAfterMaxDuration: &largeRetryAfterMaxDuration, + wantRequestDuration: 1 * time.Second, }, { - name: "large max 500 without Retry-After", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &largeRetryAfterMaxDuration, - }, - statusCode: http.StatusInternalServerError, - responseFormat: None, - wantReqCount: 3, + name: "large max 500 without Retry-After", + statusCode: http.StatusInternalServerError, + retryAfterFormat: None, + retryAfterDuration: 0 * time.Second, + retryAfterMaxDuration: &largeRetryAfterMaxDuration, + wantRequestDuration: retryBackoffDuration, }, - // // Small Max Tests (Less Than Retry-After Value) - // { - name: "small max 429 without Retry-After", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &smallRetryAfterMaxDuration, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: None, - wantReqCount: 3, + name: "small max 429 without Retry-After", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: None, + retryAfterDuration: 0 * time.Second, + retryAfterMaxDuration: &smallRetryAfterMaxDuration, + wantRequestDuration: retryBackoffDuration, }, { - name: "small max 429 with Retry-After seconds", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &smallRetryAfterMaxDuration, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: Seconds, - responseDuration: 4 * time.Second, - wantReqCount: 3, + name: "small max 429 with Retry-After seconds", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: Seconds, + retryAfterDuration: 4 * time.Second, + retryAfterMaxDuration: &smallRetryAfterMaxDuration, + wantRequestDuration: smallRetryAfterMaxDuration, }, { - name: "small max 429 with Retry-After date", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &smallRetryAfterMaxDuration, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: Date, - responseDuration: 2 * time.Second, - wantReqCount: 3, + name: "small max 429 with Retry-After date", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: Date, + retryAfterDuration: 2 * time.Second, + retryAfterMaxDuration: &smallRetryAfterMaxDuration, + wantRequestDuration: smallRetryAfterMaxDuration, }, { - name: "small max 429 with invalid Retry-After", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &smallRetryAfterMaxDuration, - }, - statusCode: http.StatusTooManyRequests, - responseFormat: Invalid, - wantReqCount: 3, + name: "small max 429 with invalid Retry-After", + statusCode: http.StatusTooManyRequests, + retryAfterFormat: Invalid, + retryAfterDuration: 0 * time.Second, + retryAfterMaxDuration: &smallRetryAfterMaxDuration, + wantRequestDuration: retryBackoffDuration, }, { - name: "small max 503 with Retry-After seconds", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &smallRetryAfterMaxDuration, - }, - statusCode: http.StatusServiceUnavailable, - responseFormat: Seconds, - responseDuration: 4 * time.Second, - wantReqCount: 3, + name: "small max 503 with Retry-After seconds", + statusCode: http.StatusServiceUnavailable, + retryAfterFormat: Seconds, + retryAfterDuration: 4 * time.Second, + retryAfterMaxDuration: &smallRetryAfterMaxDuration, + wantRequestDuration: smallRetryAfterMaxDuration, }, { - name: "small max 500 without Retry-After", - config: RetryConfig{ - RetryMax: 2, - RetryAfterMaxDuration: &smallRetryAfterMaxDuration, - }, - statusCode: http.StatusInternalServerError, - responseFormat: None, - wantReqCount: 3, + name: "small max 500 without Retry-After", + statusCode: http.StatusInternalServerError, + retryAfterFormat: None, + retryAfterDuration: 0 * time.Second, + retryAfterMaxDuration: &smallRetryAfterMaxDuration, + wantRequestDuration: retryBackoffDuration, }, } - // Execute The TestCases - for _, test := range tests { - t.Run(test.name, func(t *testing.T) { - - t.Parallel() - - // Clone The TestCase (Parallel Execution Race Protection) - tc := test + // Loop Over The TestCases + for _, testCase := range testCases { - // Consistent CheckRetry & Backoff Implementation For All TestCases - paddingDuration := 250 * time.Millisecond // Test Execution Allowance For Resending Requests - backoffDuration := 500 * time.Millisecond // Constant Backoff Time For Simplified Testing - tc.config.CheckRetry = SelectiveRetry - tc.config.Backoff = func(attemptNum int, resp *http.Response) time.Duration { return backoffDuration } + // Capture Range Variable For Parallel Execution + tc := testCase - // Tracking Variables - var previousReqTime time.Time - var reqCount int32 + // Execute The Individual TestCase + t.Run(tc.name, func(t *testing.T) { - // Create A Test HTTP Server Capable Of Validating Request Interim Durations - server := httptest.NewServer(http.HandlerFunc(func(writer http.ResponseWriter, request *http.Request) { + // Run TestCases In Parallel + t.Parallel() - // Determine Time Of Current Request - currentReqTime := time.Now() - // TODO - Remove this check when experimental-feature moves to Stable/GA to convert behavior from opt-in to opt-out - if tc.config.RetryAfterMaxDuration != nil { + // Create A RetryAfter Validation Server To Validate Retry Durations + server := newRetryAfterValidationServer(t, tc.statusCode, tc.retryAfterFormat, tc.retryAfterDuration, tc.wantRequestDuration) - // TODO - Keep this logic as is (no change required) when experimental-feature is Stable/GA - if tc.responseFormat == Date && tc.responseDuration > 0 { - currentReqTime = currentReqTime.Round(time.Second) // Round When Using Date Format To Account For RFC850 Precision - } - } - - // Count The Requests - atomic.AddInt32(&reqCount, 1) - - // Add Retry-After Header To Response Based On TestCase - switch tc.responseFormat { - case None: - // Don't Write Anything ;) - case Seconds: - writer.Header().Set(RetryAfterHeader, strconv.Itoa(int(tc.responseDuration.Seconds()))) - case Date: - writer.Header().Set(RetryAfterHeader, currentReqTime.Add(tc.responseDuration).Format(time.RFC850)) // RFC850 Drops Millis - case Invalid: - writer.Header().Set(RetryAfterHeader, "FOO") - default: - assert.Fail(t, "TestCase with unsupported ResponseFormat '%v'", tc.responseFormat) - } - - // Only Validate Timings Of Retries - Not Interested In Initial Request - if reqCount > 1 { - - // Calculate The Expected Maximum Request Duration Of TestCase - expectedMinRequestDuration := backoffDuration - // TODO - Remove this check when experimental-feature moves to Stable/GA to convert behavior from opt-in to opt-out - if tc.config.RetryAfterMaxDuration != nil { - - // TODO - Keep this logic as is (no change required) when experimental-feature is Stable/GA - if tc.responseDuration > 0 { - expectedMinRequestDuration = tc.responseDuration - if tc.config.RetryAfterMaxDuration != nil { - if *tc.config.RetryAfterMaxDuration == 0 { - expectedMinRequestDuration = backoffDuration - } else if *tc.config.RetryAfterMaxDuration < expectedMinRequestDuration { - expectedMinRequestDuration = *tc.config.RetryAfterMaxDuration - } - } - } - } - expectedMaxRequestDuration := expectedMinRequestDuration + paddingDuration - - // Validate Inter-Request Durations Meet Or Exceed Expected Minimums - actualRequestDuration := currentReqTime.Sub(previousReqTime) - assert.GreaterOrEqual(t, actualRequestDuration, expectedMinRequestDuration, "previousReqTime =", previousReqTime.String(), "currentReqTime =", currentReqTime.String()) - assert.LessOrEqual(t, actualRequestDuration, expectedMaxRequestDuration, "previousReqTime =", previousReqTime.String(), "currentReqTime =", currentReqTime.String()) - t.Logf("Validated Request Duration %s between expected range %s - %s", - actualRequestDuration.String(), - expectedMinRequestDuration.String(), - expectedMaxRequestDuration.String()) - } - - // Respond With StatusCode From TestCase & Cycle The Times - writer.WriteHeader(tc.statusCode) - previousReqTime = currentReqTime - })) + // Create RetryConfig With RetryAfterMax From TestCase + retryConfig := RetryConfig{ + RetryMax: int(retryMax), + CheckRetry: SelectiveRetry, + Backoff: func(attemptNum int, resp *http.Response) time.Duration { return retryBackoffDuration }, + RetryAfterMaxDuration: tc.retryAfterMaxDuration, + } - // Perform The Test - Generate And Send The Request - t.Logf("Testing %d Response With RetryAfter (%d) %fs & Max %+v", tc.statusCode, tc.responseFormat, tc.responseDuration.Seconds(), tc.config.RetryAfterMaxDuration) + // Perform The Test - Generate And Send The Initial Request + t.Logf("Testing %d Response With RetryAfter (%d) %fs & Max %+v", tc.statusCode, tc.retryAfterFormat, tc.retryAfterDuration.Seconds(), tc.retryAfterMaxDuration) sender := &HTTPMessageSender{Client: http.DefaultClient} request, err := http.NewRequest("POST", server.URL, nil) assert.Nil(t, err) - got, err := sender.SendWithRetries(request, &tc.config) + response, err := sender.SendWithRetries(request, &retryConfig) - // Verify Final Results - if err != nil { - t.Fatalf("SendWithRetries() error = %v, wantErr nil", err) - } - if got.StatusCode != tc.statusCode { - t.Fatalf("SendWithRetries() got = %v, want %v", got.StatusCode, tc.statusCode) - } - if int(atomic.LoadInt32(&reqCount)) != tc.wantReqCount { - t.Fatalf("expected %d retries got %d", tc.config.RetryMax, reqCount) - } + // Verify Final Results (Actual Retry Timing Validated In Server) + assert.Nil(t, err) + assert.Equal(t, response.StatusCode, tc.statusCode) + assert.Equal(t, retryMax+1, atomic.LoadInt32(&server.requestCount)) }) } } From 5be0e207c3a01b7d5ef1cceb4c78de11094afc65 Mon Sep 17 00:00:00 2001 From: I867318 Date: Tue, 23 Nov 2021 09:52:47 -0700 Subject: [PATCH 15/15] PR feedback --- docs/eventing-api.md | 2 +- pkg/apis/duck/v1/delivery_types.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/eventing-api.md b/docs/eventing-api.md index 392a29a32bc..4c84a143474 100644 --- a/docs/eventing-api.md +++ b/docs/eventing-api.md @@ -402,7 +402,7 @@ string (Optional)

RetryAfterMax provides an optional upper bound on the duration specified in a “Retry-After” header -when calculating backoff times for retrying 429 and 502 response codes. Setting the value to +when calculating backoff times for retrying 429 and 503 response codes. Setting the value to zero (“PT0S”) can be used to opt-out of respecting “Retry-After” header values altogether. This value only takes effect if “Retry” is configured, and also depends on specific implementations (Channels, Sources, etc.) choosing to provide this capability.

diff --git a/pkg/apis/duck/v1/delivery_types.go b/pkg/apis/duck/v1/delivery_types.go index 4d1e007640f..49c26ced5d5 100644 --- a/pkg/apis/duck/v1/delivery_types.go +++ b/pkg/apis/duck/v1/delivery_types.go @@ -63,7 +63,7 @@ type DeliverySpec struct { BackoffDelay *string `json:"backoffDelay,omitempty"` // RetryAfterMax provides an optional upper bound on the duration specified in a "Retry-After" header - // when calculating backoff times for retrying 429 and 502 response codes. Setting the value to + // when calculating backoff times for retrying 429 and 503 response codes. Setting the value to // zero ("PT0S") can be used to opt-out of respecting "Retry-After" header values altogether. This // value only takes effect if "Retry" is configured, and also depends on specific implementations // (Channels, Sources, etc.) choosing to provide this capability.