Support for 429 Retry-After header

[travis-minke-sap]

Hi All,

During today's Knative Eventing WG meeting I started a conversation about whether / how we might support the 429 Retry-After header as described in the CloudEvent WebHook Specification. Specifically in Section 2.2 it states...

If the delivery target is unable to process the request due to exceeding a request rate limit,
it SHOULD return a 429 Too Many Requests status code and MUST include the Retry-After header.
The sender MUST observe the value of the Retry-After header and refrain from sending further 
requests until the indicated time.

The Retry-After header is a standard header as described here...
https://tools.ietf.org/html/rfc7231#section-7.1.3

It seems as though respecting this header and delaying subsequent requests would provide some minimal form of back-pressure sanity short of the full rate-limiting auth handshake that is described in Sections 4.1.4 and 4.2.2 of the specification.

My initial inclination was to enhance the current eventing/pkg/channel/message_dispatcher.go logic which performs retries so that it respected this header and waited whichever duration was greater of the Retry-After value and the next retry attempt. This would, though, synchronously block the DispatchMessageWithRetries() request which may or may not be desirable. It also wouldn't guarantee any coordination between other threads attempting to send to that WebHook (eventing-kafka's distributed kafkachannel has scalable Dispatcher replicas).

Would love to hear your thoughts - thanks !

Questions...

1. Does this belong in the shared eventing code (message_dispatcher.go), or in individual channel implementations, in downstream intermediaries, or maybe somewhere else?

   • If it is in the channel implementation then those channels effectively have to write their own retry logic as they wouldn't be able to call DispatchMessageWithRetries().

2. If it is added to the shared eventing code, should this configurable and at what level?

   • Could be a parameter to DispatchMessageWithRetries() for channels to hardcode or expose as needed?
   • Could be a new boolean field in the Subscription.Delivery spec like RespectRetryAfter
   • Other ideas?

3. Would this take effect ALWAYS or ONLY if retry was enabled in Subscription.Delivery?

4. Would we want an override capability if the Retry-After value was excessively large? Something that says... "support Retry-After but max out at X seconds/minutes/etc" ?

Notes...

• There are a couple of retry decision logic functions in the eventing/pkg/kncloudevents/message_sender.go implementation, some of which only retry above 300, while others don't retry on 300 and instead include a few 400s (specifically 404, 429, 409).

[travis-minke-sap]

Sooo many opinions!!! ; )

I created PR #5285 with a proposed initial step - doesn't change the api shape or current behavior - is opt-in for now...

[travis-minke-sap]

fwiw... @slinkydeveloper pointed out in the PR above that there's a bit more history and scope involved here...

May 2020 - Knative & CloudEvents
Knative-Eventing Issue #3092
CloudEvents SDK PR #492