ietf-wg-httpapi / ratelimit-headers

Repository for IETF WG draft ratelimit-headers
Other
45 stars 5 forks source link

Clarify Limit's compatibility with existing implementations #45

Closed ucarion closed 2 years ago

ucarion commented 3 years ago

This document strives to formalize behavior that a lot of existing servers and clients already implement, i.e. the "3-integer (Limit/Remaining/Reset)" model of ratelimit HTTP headers. If existing clients already parse Limit in some or all circumstances, isn't changing Limit from an integer to a SFV list of integers a backwards-incompatible change?

Of course, there's an upgrade path possible for existing clients: they should parse up to the first , (if any) if they want to support both "old"-style Limit and the Limit in this RFC. But an upgrade path is not the same thing as backwards compatibility.

In https://github.com/ietf-wg-httpapi/ratelimit-headers/pull/34#issuecomment-798056543, @ioggstream suggests the following as behavior an interoperable client could implement:

if "Remaining" not in headers:
  remaining = headers["Limit"] 
else:
  remaining = headers["Remaining"]

rps = remaining / headers["Reset"]

It's precisely clients like this that would be affected by a change from integer-only Limit to a SFV Limit. Should we not call out that we are breaking compatibility with the de-facto standard in the wild?

ioggstream commented 3 years ago

While this is worth clarifying, the pseudocode is not a complete implementation, because the spec requires the field values to be validated before use. Eg.


limit, policies =  parse_limit(...)  #  (int, dict) or None if missing or invalid
remaining = parse_remaining(..)  #  int or None if missing or invalid
reset = parse_reset(..) # int or None if missing or invalid

if reset is None:
  raise ValueError("This implementation doesn't want to do euristic...")

if remaining or limit is false:
   raise ValueError("This implementation doesn't want to do euristic...")

if remaining is None:
  remaining = limit
else:
  remaining = remaining

rps = remaining / reset

if rps too high:
  ... do something ..

if remaining < threshold:
  .. do something, eg. wait, signal the worker to slow down, ...
unleashed commented 3 years ago

I think the usage of SFV is orthogonal to our idea of backwards compatibility regarding this information. Of course many if not all existing clients will need to parse these headers differently because of this, but that is also the case with many of them parsing only the X- prefixed headers, or those that have slight variations in the names. The important thing is that once this parsing step is completed they should have at least the same basic information they used to have without the need to modify their rate limiting logic if they don't want to.

ucarion commented 3 years ago

@unleashed I think it's worth calling out any change developers need to make to their parsing logic. That's considerably more difficult to change than just altering what headers to look for. Parsing a SFV header is non-trivial, especially compared to parsing an integer.

I'm not pushing in this PR for any changes to the spec, aside for a clarifying note about what it takes to upgrade to this spec.

unleashed commented 3 years ago

@ucarion sure, that's something to be aware of -- that said, I'd expect implementors to limit the changes to replacing the logic of a header parsing step with that from a SFV parsing library and end up with the same information - there are plenty such libraries out there by now. :)

ucarion commented 3 years ago

If we have expectations for what implementors will do, those should typically be written in the document. :)

unleashed commented 3 years ago

Agree, but... isn't that fully implied by specifying the fields follow the SFV RFC?

ioggstream commented 2 years ago

Related to #24.

@ucarion can #75 clarify/fix this?

ioggstream commented 2 years ago

PR replaced with #78