ietf-wg-httpapi / ratelimit-headers

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

Put quota policies in a separate header from Limit? #79

Closed ucarion closed 2 years ago

ucarion commented 2 years ago

The draft today suggests putting quota policies, a list, into the RateLimit-Limit header. But that's a sort of breaking change, because that header's equivalent is strictly a number (not a list) in most existing implementations.

Could RateLimit-Limit be kept as a plain number (like Reset and Remaining), and instead a new header like RateLimit-Policy or some such can contain supplementary information? That way, the story for upgrading to this draft is simpler.

Sorry if this is asked and answered.

ioggstream commented 2 years ago

iirc we considered this but we initially had some requests of conflating as much as possible in fewer fields. Probably it's a more interoperable choice.

I am open in re-considering the idea, provided we get in touch with the current implementers including 3scale, kong and envoy

cc: 3scale: @unleashed , kong: @bungle , envoyproxy: @jespersoderlund

unleashed commented 2 years ago

There's no inherent problem in using a specific header for policies, and no issue in moving to use them from the 3scale side. The only downside is we're adding yet another header for rate limiting, currently 4 of them, which might raise a few eyebrows.

ioggstream commented 2 years ago

@unleashed thanks :)

unleashed commented 2 years ago

do you think that splitting is a more interoperable choice wrt to the parsing implementation / usability?

Yes, it's easier for people to pick up a new header for this to apply a policy, since as it stands they might break when seeing extra stuff in the limit one by expecting a single value rather than multiple, as @ucarion mentions above.

Do you see other practical drawbacks?

No, just dealing with people (maybe) opposing too-many-headers - I don't know if this is something that we can predictably foresee happening in the HTTPAPI WG or in the HTTP WG, but my intuition says "likely" given how we were asked to "condense" this information, so I think we might want to spend some time making a good case out of it. @ucarion other than potential breakages in pre-existing implementations, is there any additional point we could make to justify the additional header?

if we don't get feedback on this issue, can you ping the other implementers you know?

I think they are all currently listed - that being said I think porting to a new header should be an easy task, so hopefully not a problem.

ioggstream commented 2 years ago

think porting to a new header should be an easy task, so hopefully not a problem.

if almost everyone is implementing just the minimal specification, probably this could even simplify the addition of quota-policy...

ucarion commented 2 years ago

other than potential breakages in pre-existing implementations, is there any additional point we could make to justify the additional header?

The primary motivation is breakage, as you suggest, @unleashed.

The secondary motivation is ease of "upgrading" to adoption of quota policies, as @ioggstream suggests. The flipside of "easy to adopt, easy to upgrade" is that the spec would also make it easy to never upgrade at all. This is a good thing, supposing that most services can operate perfectly fine without quota policies.

A third, much weaker argument, is that the current approach of putting the "limit" and "quota policy" in the same header is that it's confusing. I would need more time to really state this argument more clearly, but what rubs me the wrong way here is that the current design basically puts two different kinds of data in the same list.

As it stands, RateLimit-Limit is a list of a numbers, but the first number (the "limit") is not like the rest of the numbers (the policy). The first number cannot have SFV parameters, but the others can (and typically will). It's an accident of SFV's syntax that this is even conceivable as an approach.

For implementors, this means that RateLimit-Limit does not simply deserialize as some sort of List<QuotaPolicy>. It deserializes into a list, where the first element is an Integer and the tail is a List<QuotaPolicy>. I fear this will at minimum confuse folks, and perhaps even lead to incorrect implementations, such as people not realizing the first element of the list is not a quota policy at all.

ioggstream commented 2 years ago

Given https://github.com/ietf-wg-httpapi/ratelimit-headers/issues/65#issuecomment-1112741654 it seems there's some consensus in providing quota policy in a separate field.