Added support for policy identifier #130

darrelmiller · 2023-07-19T01:58:25Z

This proposal takes what we currently have in draft-07

RateLimit-Policy: 100;w=60, 10000;w=86400
{ 50 requests and 30 seconds}
RateLimit: limit=100, remaining=50, reset=30  // protection limit
{ 1 requests and 40 seconds}
RateLimit: limit=10000, remaining=90, reset=200 // quota limit

and suggests the following alternative

RateLimit-Policy: protection;l=100;w=60, quota;l=10000;w=86400
{ 50 requests and 30 seconds}
RateLimit: protection;r=50;t=30
{ 1 request and 40 seconds}
RateLimit: quota;r=90;t=200

This proposal introduces a policy "identifier" that connects the RateLimit field to the RateLimit-Policy field. The primary purpose is to address #127.

With the change in draft-07 to use structured fields in the header we now have the opportunity to explore different approaches and features without trying to maintain similarity with past efforts.

nfriedly · 2023-11-06T14:56:10Z

I'm a big fan of this overall, I think it's a solid improvement in terms of consistency and ease of parsing.

I am curious about the reasoning for separate RateLimit and RateLimit-Policy headers? It seems to me that the policy parameters could also be appended to the RateLimit header.

draft-ietf-httpapi-ratelimit-headers.md

nfriedly · 2023-11-06T17:07:33Z

draft-ietf-httpapi-ratelimit-headers.md

+RateLimit-Limit: 12
 RateLimit-Policy: 12;w=1
+RateLimit-Remaining: 6          ; using 50% of throughput, that is 6 units/s
+RateLimit-Reset: 1
 ~~~

 If this is the case, the optimal solution is to achieve

 ~~~ example
-RateLimit: limit=12,   \
-           remaining=1 \  ; using 100% of throughput, that is 12 units/s
-           reset=1
+RateLimit-Limit: 12
 RateLimit-Policy: 12;w=1
+RateLimit-Remaining: 1          ; using 100% of throughput, that is 12 units/s
+RateLimit-Reset: 1


Why did these get reverted to the older style?

An error. I will fix.

nfriedly · 2023-11-06T17:10:17Z

draft-ietf-httpapi-ratelimit-headers.md

 ~~~

-   the key value is the one referencing the lowest limit: `100`
+   the value "sliding" identifies the policy being reported.

 11. Can we use shorter names? Why don't put everything in one field?


This question & answer should probably be revised.

darrelmiller · 2024-01-06T02:01:43Z

I am curious about the reasoning for separate RateLimit and RateLimit-Policy headers? It seems to me that the policy parameters could also be appended to the RateLimit header.

The split is due to variability of the values and HTTP/2 HPACK compression. RateLimit-Policy should rarely change and therefore can be optimized by header compression. RateLimit will change on ever request and therefore will not be optimized.

ioggstream · 2024-01-08T12:46:42Z

make returns this error.

draft-ietf-httpapi-ratelimit-headers.xml(0): Error: IDREF attribute target references an unknown ID "ratelimit-reset-keyword", at None

ioggstream · 2024-01-08T13:08:34Z

@darrelmiller I tried to fix the broken section references: now it should build. Could you please PTAL?

ioggstream · 2024-01-08T13:09:14Z

@nfriedly PR welcome for fixing inconsistencies :)

richsalz · 2024-01-08T17:45:14Z

make returns this error.

draft-ietf-httpapi-ratelimit-headers.xml(0): Error: IDREF attribute target references an unknown ID "ratelimit-reset-keyword", at None

There's a few instances still to be cleaned up:

; grep -n ratelimit-reset-keyword *.md
draft-ietf-httpapi-ratelimit-headers.md:216:     the time window reset time ({{ratelimit-reset-keyword}}).
draft-ietf-httpapi-ratelimit-headers.md:233:- inferred by the value of the [reset keyword](#ratelimit-reset-keyword) at the moment of the reset, or
draft-ietf-httpapi-ratelimit-headers.md:263:## Reset Keyword {#ratelimit-reset-keyword}
draft-ietf-httpapi-ratelimit-headers.md:440:Consider that service limit might not be restored after the moment referenced by the [reset keyword](#ratelimit-reset-keyword),
draft-ietf-httpapi-ratelimit-headers.md:579:| RateLimit | reset                 | Dictionary Key |Quota reset interval | {{ratelimit-reset-keyword}} of {{&SELF}} |       |

jidicula

👋 hey folks - @kfcampbell shared this inside GitHub's API team as open for comment. I'm new to providing feedback on IETF proposals, so thank you for your patience if any of my comments are low-value or irrelevant to the crux of the PR 🙇‍♂️.

I've got a couple of points about wording consistency in this proposal, as well as some copyedits:

draft-ietf-httpapi-ratelimit-headers.md

jidicula · 2024-05-03T17:04:54Z

draft-ietf-httpapi-ratelimit-headers.md


 ~~~

 If a response contains both the Retry-After and the RateLimit header fields, the reset keyword value SHOULD reference the same point in time as the Retry-After field value.

-When using a policy involving more than one time window, the server MUST reply with the RateLimit header fields related to the time window with the lower remaining keyword values.
+When using a policy involving more than one time window, the server MUST reply with the RateLimit header field related to the time window with the lower remaining keyword values.


Suggested change

When using a policy involving more than one time window, the server MUST reply with the RateLimit header field related to the time window with the lower remaining keyword values.

When using a policy involving more than one time window, the server MUST reply with the RateLimit header field related to the time window with the lower "r" parameter values.

Does this need to be updated to be consistent with the change in https://github.com/ietf-wg-httpapi/ratelimit-headers/pull/130/files#diff-d26a6f407efe0e4913cbcf7bf0e2a164bee5edea8bf2fdc4aa97a7ceb3acc03cR229 (s/remaining keyword/r parameter/)?

There are other instances in this document that are still referring to remaining keyword that I can't comment on because they're not part of the PR diff - do those need to be updated too?

More generally, I see some diffs where wording was changed from keyword to parameter, but keyword remains in other areas. I'm new to this side of proposal drafts, so apologies if this is just bikeshedding 😁: should this wording be consistent throughout?

Co-authored-by: Johanan <jidicula@github.com>

izuzak

Just adding two minor comments after reading the draft. ✌️ Overall, I like the draft so far -- great work! 👏

izuzak · 2024-05-23T11:19:30Z

draft-ietf-httpapi-ratelimit-headers.md


 The field is a non-empty List of Items. Each item is a [quota policy](#quota-policy).
-Two quota policies MUST NOT be associated with the same quota units value.
+Two quota policies MUST NOT be associated with the same quota units value unless they are differentiated with a unique p parameter value.


Where is the meaning of parameter "p" defined? I don't see a definition for it nor an example using it. 🤔

It is legacy. I have removed it.

draft-ietf-httpapi-ratelimit-headers.md

Co-authored-by: Johanan <jidicula@github.com>

ioggstream and others added 4 commits November 17, 2022 18:48

Fix: #64. Combine header.

a544a5a

Updated with policy parameter

0b47e95

Merge remote-tracking branch 'origin/main' into darrelmiller-policyname

6f5909b

Removed limit

08d1ada

darrelmiller changed the title ~~Darrelmiller-policyname~~ Added support for policy identifier Jul 19, 2023

darrelmiller requested a review from ioggstream July 19, 2023 02:58

Updated based on IETF117 feedback

c3fa4e5

This was referenced Nov 6, 2023

Commas, and potentially combining RateLimit and RateLimit-Policy #131

Open

Field names and combination #65

Closed

Correlating limit with corresponding policy #127

Closed

nfriedly reviewed Nov 6, 2023

View reviewed changes

draft-ietf-httpapi-ratelimit-headers.md Outdated Show resolved Hide resolved

nfriedly reviewed Nov 6, 2023

View reviewed changes

nfriedly mentioned this pull request Nov 6, 2023

Todo express-rate-limit/ratelimit-header-parser#1

Open

23 tasks

Run make fix-lint.

562114b

ioggstream added 2 commits January 8, 2024 13:52

remove unsupported 'example' sourcecode type.

02a991c

Fix section references.

0fad367

jidicula reviewed May 3, 2024

View reviewed changes

Update draft-ietf-httpapi-ratelimit-headers.md

fc0dc0b

Co-authored-by: Johanan <jidicula@github.com>

izuzak reviewed May 23, 2024

View reviewed changes

Update draft-ietf-httpapi-ratelimit-headers.md

7a94332

Co-authored-by: Johanan <jidicula@github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for policy identifier #130

Added support for policy identifier #130

darrelmiller commented Jul 19, 2023 •

edited

nfriedly commented Nov 6, 2023

nfriedly Nov 6, 2023

darrelmiller Jan 6, 2024

nfriedly Nov 6, 2023

darrelmiller commented Jan 6, 2024

ioggstream commented Jan 8, 2024

ioggstream commented Jan 8, 2024

ioggstream commented Jan 8, 2024

richsalz commented Jan 8, 2024

jidicula left a comment

jidicula May 3, 2024

izuzak left a comment

izuzak May 23, 2024

darrelmiller Jun 2, 2024

	When using a policy involving more than one time window, the server MUST reply with the RateLimit header field related to the time window with the lower remaining keyword values.
	When using a policy involving more than one time window, the server MUST reply with the RateLimit header field related to the time window with the lower "r" parameter values.

Added support for policy identifier #130

Are you sure you want to change the base?

Added support for policy identifier #130

Conversation

darrelmiller commented Jul 19, 2023 • edited

nfriedly commented Nov 6, 2023

nfriedly Nov 6, 2023

Choose a reason for hiding this comment

darrelmiller Jan 6, 2024

Choose a reason for hiding this comment

nfriedly Nov 6, 2023

Choose a reason for hiding this comment

darrelmiller commented Jan 6, 2024

ioggstream commented Jan 8, 2024

ioggstream commented Jan 8, 2024

ioggstream commented Jan 8, 2024

richsalz commented Jan 8, 2024

jidicula left a comment

Choose a reason for hiding this comment

jidicula May 3, 2024

Choose a reason for hiding this comment

izuzak left a comment

Choose a reason for hiding this comment

izuzak May 23, 2024

Choose a reason for hiding this comment

darrelmiller Jun 2, 2024

Choose a reason for hiding this comment

darrelmiller commented Jul 19, 2023 •

edited