Ratelimiting Design #706

arkodg · 2022-11-07T19:07:18Z

Started a design doc highlighting the WHAT and WHY

Relates to #670

Signed-off-by: Arko Dasgupta [email protected]

docs/latest/design/ratelimit.md

arkodg · 2022-11-07T19:22:23Z

ive intentionally left the WHERE (which extension point does the Ratelimiting API get applied as) out of this document for now, will add info about it once we align on #675

docs/latest/design/ratelimit.md

danehans

@arkodg thanks for getting the RL work started. I think the doc provides good starting examples for an RL API but requires more detail to be considered a design.

docs/latest/design/ratelimit.md

arkodg · 2022-11-07T19:46:04Z

yah @danehans I'll push another commit with the exact API and kube builder annotations

docs/latest/design/ratelimit.md

api/ratelimit/v1alpha1/ratelimit_types.go

docs/latest/design/ratelimit.md

danehans · 2022-11-09T18:53:08Z

docs/latest/design/ratelimit.md

+spec:
+  type: Global
+  rules:
+  - matches:


matches is unneeded if RateLimit is applied to an HTTPRoute. For example:

apiVersion: gateway.networking.k8s.io/v1beta1 kind: HTTPRoute metadata: name: example spec: ... rules: - matches: headers: - name: x-user-tier value: one filters: - type: ExtensionRef extensionRef: group: gateway.envoyproxy.io kind: RateLimiting name: ratelimit-specific-requests backendRefs: - name: httpbin port: 80

Matching criteria can be derived from the HTTPRoute that references the RateLimiting filter and RateLimiting can define the rate limit policy, e.g. domain and descriptors (for type Global).

the matches criteria within HTTPRoute helps map a specific traffic flow to a destination backend.
Even though the RateLimit is linked to a specific HTTPRoute, the matches criteria here further maps the traffic to specific users (source ip, header etc)

It feels to me like this is the biggest decision here - adding an extra layer of matching inside the ratelimit filter rather than having it filter whatever is configured in the HTTPRoute is an interesting approach - it's not really what I expected when thinking about this, tbh.

@arkodg, do you think you could put some explanation in this doc (maybe in the decisions made section) as to why you've chosen this way rather than the one Daneyon suggested? I'm not saying we should change it, just that we should record the reasons we've decided this way.

agreed, ive added a section on it under Design Decisions

I'm ok with matches, I did a similar design in my personal project with another name (rules).

Signed-off-by: Arko Dasgupta <[email protected]>

danehans · 2022-12-21T17:40:28Z

api/v1alpha1/ratelimitfilter_types.go

+//   - "Exact": Use this type to match the exact value of the Value field against the value of the specified HTTP Header.
+//   - "RegularExpression": Use this type to match a regular expression against the value of the specified HTTP Header.
+//     The regex string must adhere to the syntax documented in https://github.com/google/re2/wiki/Syntax.
+//   - "Distinct": Use this type to match any and all possible unique values encountered in the specified HTTP Header.


It took me some time to understand the Distinct type. I have a feeling others will feel the same. What if we only supported Exact and RegularExpression? If Value is unspecified for Exact, then all values of name are selected.

same as #706 (comment)

api/v1alpha1/ratelimitfilter_types.go

danehans · 2022-12-21T18:19:14Z

api/v1alpha1/ratelimitfilter_types.go

+	// +kubebuilder:default=Exact
+	Type *HeaderMatchType `json:"type,omitempty"`
+
+	// Name of the HTTP header.


Should we provide additional context to Name as Gateway API does, e.g. multiple rules with the same header name?

xref: https://github.com/kubernetes-sigs/gateway-api/blob/v0.6.0/apis/v1alpha2/grpcroute_types.go#L385-L389

@kflynn ^ comment is another candidate to resolve in a follow-on PR.

danehans · 2022-12-21T18:20:43Z

api/v1alpha1/ratelimitfilter_types.go

+
+	// Value within the HTTP header. Due to the
+	// case-insensitivity of header names, "foo" and "Foo" are considered equivalent.
+	// Do not set this field when Type="Distinct", implying matching on any/all unique values within the header.


This comment can be removed if we drop the Distinct type. See https://github.com/envoyproxy/gateway/pull/706/files#r1054657720 for additional details.

Two of the three header matching types proposed in this PR are the same as upstream Gateway API. Do you know why Gateway API doesn't support the Distinct type? If not, it could be valuable to share our use case and get feedback.

its naming as well as use case has already been upvoted by reviewers, so I feel strongly to keep it, and proactively present it to the users, and get feedback for this v1alpha1 version.

Do you know why Gateway API doesn't support the Distinct type?

"Distinct" isn't really a matching mechanism per se, but rather denotes that each unique value for a header the RLS sees will get its own rate limiting bucket, which doesn't really fit into the idea of matching request attributes to funnel to a backend

api/v1alpha1/ratelimitfilter_types.go

Signed-off-by: Arko Dasgupta <[email protected]>

sunjayBhatia

a couple more questions on UX for a client:

Do we want to make the status code or headers a client sees when they are rate limited configurable? (Seems like there might be space as the API is today to leave this alone and add it later)
If the RLS is down/not responding, what behavior is expected, requests allowed to proceed or rejected? Should this be configurable per filter or globally? (I would err on globally if we do make it configurable but no real preference, depends on what users really need)

arkodg · 2022-12-21T22:28:46Z

thanks for the reviews everyone, this PR has been commented on over 200 times highlighting a healthy debate.
Unfortunately even after addressing multiple suggestions, im unsure how to move this alpha API forward to completion that addresses the use cases outlined in the design doc.
ccing @danehans since you've commented with the requested changes option

danehans · 2022-12-31T00:55:14Z

@arkodg thanks for your patients while we work through this design. I think the design is very close but enough questions exist to warrant a discussion during the next community meeting. I added this to the agenda and I feel confident that we'll get this PR merged afterward.

arkodg · 2023-01-03T19:01:17Z

@arkodg thanks for your patients while we work through this design. I think the design is very close but enough questions exist to warrant a discussion during the next community meeting. I added this to the agenda and I feel confident that we'll get this PR merged afterward.

sure @danehans, will answer your queries in the next community meeting if you prefer to raise them there instead of in this PR

kflynn

After discussion in the 3 January meeting, let's merge this one and do some further doc cleanup after. Thanks for the patience, @arkodg!

danehans

@arkodg thanks for all your work on the design and API! Please make sure to create an Issue/PR for the existing non-blocking review comments.

danehans · 2023-01-03T22:53:35Z

api/v1alpha1/ratelimitfilter_types.go

+	//
+	// +unionDiscriminator
+	Type RateLimitType `json:"type"`
+	// Global rate limit configuration.


It should be noted in the godocs that the global field is only applicable for type Global. I understand that Global is the only type currently supported but this will prevent confusion when other types, e.g. Local, are supported in the future. @arkodg please note this in the follow-on issue or @kflynn please resolve this in your PR.

danehans · 2023-01-03T22:55:21Z

api/v1alpha1/ratelimitfilter_types.go

+	// in a mutually exclusive way i.e. if multiple
+	// rules get selected, each of their associated
+	// limits get applied, so a single traffic request
+	// might increase the rate limit counters for multiple
+	// rules if selected.


@kflynn it would be appreciated if you can improve the godocs for this field.

api/v1alpha1/ratelimitfilter_types.go

danehans · 2023-01-03T23:02:55Z

api/v1alpha1/ratelimitfilter_types.go

+	// +kubebuilder:default=Exact
+	Type *HeaderMatchType `json:"type,omitempty"`
+
+	// Name of the HTTP header.


@kflynn ^ comment is another candidate to resolve in a follow-on PR.

danehans · 2023-01-03T23:08:19Z

api/v1alpha1/ratelimitfilter_types.go

+// * "Day"
+//
+// +kubebuilder:validation:Enum=Second;Minute;Hour;Day
+type RateLimitUnit string


@arkodg constants should be defined for the ^ enums. For example:

const ( // SecondRateLimitUnit defines the "Second" rate limit unit. SecondRateLimitUnit RateLimitUnit = "Second" ... )

cc: @kflynn

arkodg requested a review from a team as a code owner November 7, 2022 19:07

arkodg added area/api API-related issues priority/high Label used to express the "high" priority level labels Nov 7, 2022

arkodg added this to the 0.3.0-rc.1 milestone Nov 7, 2022

arkodg commented Nov 7, 2022

View reviewed changes

docs/latest/design/ratelimit.md Show resolved Hide resolved

arkodg commented Nov 7, 2022

View reviewed changes

docs/latest/design/ratelimit.md Outdated Show resolved Hide resolved

arkodg commented Nov 7, 2022

View reviewed changes

docs/latest/design/ratelimit.md Outdated Show resolved Hide resolved

arkodg mentioned this pull request Nov 7, 2022

Add Global Rate Limiting Support #670

Closed

7 tasks

arkodg commented Nov 7, 2022

View reviewed changes

docs/latest/design/ratelimit.md Outdated Show resolved Hide resolved

danehans reviewed Nov 7, 2022

View reviewed changes

docs/latest/design/ratelimit.md Outdated Show resolved Hide resolved

docs/latest/design/ratelimit.md Outdated Show resolved Hide resolved

docs/latest/design/ratelimit.md Show resolved Hide resolved

arkodg marked this pull request as draft November 7, 2022 19:46

arkodg force-pushed the rl-api-design-intro branch from 0b21880 to 30205d9 Compare November 8, 2022 21:53

arkodg marked this pull request as ready for review November 8, 2022 21:57

arkodg force-pushed the rl-api-design-intro branch from 30205d9 to b2b066f Compare November 8, 2022 21:58

arkodg changed the title ~~Overview and examples for Ratelimiting~~ Ratelimiting Design Nov 8, 2022

arkodg commented Nov 8, 2022

View reviewed changes

docs/latest/design/ratelimit.md Outdated Show resolved Hide resolved

zirain reviewed Nov 9, 2022

View reviewed changes

api/ratelimit/v1alpha1/ratelimit_types.go Outdated Show resolved Hide resolved

zirain reviewed Nov 9, 2022

View reviewed changes

api/ratelimit/v1alpha1/ratelimit_types.go Outdated Show resolved Hide resolved

zirain reviewed Nov 9, 2022

View reviewed changes

api/ratelimit/v1alpha1/ratelimit_types.go Outdated Show resolved Hide resolved

danehans suggested changes Nov 9, 2022

View reviewed changes

danehans mentioned this pull request Nov 15, 2022

Gateway API extensions: Usage of Policy Attachment vs. Filters #675

Closed

arkodg force-pushed the rl-api-design-intro branch from b2b066f to 512681c Compare November 17, 2022 00:27

arkodg requested review from Alice-Lilith, danehans and zirain and removed request for Alice-Lilith November 17, 2022 00:28

arkodg added 10 commits December 21, 2022 08:49

fix examples and add links

439ed9e

Signed-off-by: Arko Dasgupta <[email protected]>

fix example

543b1a2

Signed-off-by: Arko Dasgupta <[email protected]>

fix indent

e85c65d

Signed-off-by: Arko Dasgupta <[email protected]>

api typo

199bf5f

Signed-off-by: Arko Dasgupta <[email protected]>

add more notes on match conditions

7562b5b

Signed-off-by: Arko Dasgupta <[email protected]>

typo

9db2623

Signed-off-by: Arko Dasgupta <[email protected]>

:%s/matches/clientSelectors/g

25e5b6c

Signed-off-by: Arko Dasgupta <[email protected]>

regex docs && value max length

1e33d4d

Signed-off-by: Arko Dasgupta <[email protected]>

fix comments for global rate limit type

090dc02

Signed-off-by: Arko Dasgupta <[email protected]>

merge dependabot bumps

ef69423

Signed-off-by: Arko Dasgupta <[email protected]>

arkodg force-pushed the rl-api-design-intro branch from e1e9cd5 to ef69423 Compare December 21, 2022 16:50

arkodg requested a review from danehans December 21, 2022 16:50

danehans reviewed Dec 21, 2022

View reviewed changes

api/v1alpha1/ratelimitfilter_types.go Show resolved Hide resolved

danehans reviewed Dec 21, 2022

View reviewed changes

api/v1alpha1/ratelimitfilter_types.go Outdated Show resolved Hide resolved

fix comment on headers

364b0c5

Signed-off-by: Arko Dasgupta <[email protected]>

sunjayBhatia reviewed Dec 21, 2022

View reviewed changes

arkodg requested a review from danehans December 21, 2022 22:49

kflynn approved these changes Jan 3, 2023

View reviewed changes

danehans approved these changes Jan 3, 2023

View reviewed changes

danehans merged commit 4fdbb22 into envoyproxy:main Jan 3, 2023

arkodg mentioned this pull request Jan 4, 2023

Enhance go doc comments for RateLimitFilter #857

Closed

arkodg deleted the rl-api-design-intro branch April 20, 2023 16:31

arkodg mentioned this pull request Dec 18, 2023

feat: local rate limit #2258

Merged

arkodg mentioned this pull request Apr 10, 2024

api: Authorization API design #2652

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ratelimiting Design #706

Ratelimiting Design #706

arkodg commented Nov 7, 2022

arkodg commented Nov 7, 2022

danehans left a comment

arkodg commented Nov 7, 2022

danehans Nov 9, 2022

arkodg Nov 11, 2022

youngnick Dec 12, 2022

arkodg Dec 12, 2022

zirain Dec 20, 2022

danehans Dec 21, 2022 •

edited

Loading

arkodg Dec 21, 2022

danehans Dec 21, 2022

danehans Jan 3, 2023

danehans Dec 21, 2022

danehans Dec 21, 2022

arkodg Dec 21, 2022

sunjayBhatia Dec 21, 2022

sunjayBhatia left a comment

arkodg commented Dec 21, 2022

danehans commented Dec 31, 2022

arkodg commented Jan 3, 2023

kflynn left a comment

danehans left a comment

danehans Jan 3, 2023

danehans Jan 3, 2023

danehans Jan 3, 2023

danehans Jan 3, 2023

Ratelimiting Design #706

Ratelimiting Design #706

Conversation

arkodg commented Nov 7, 2022

arkodg commented Nov 7, 2022

danehans left a comment

Choose a reason for hiding this comment

arkodg commented Nov 7, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Dec 21, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunjayBhatia left a comment

Choose a reason for hiding this comment

arkodg commented Dec 21, 2022

danehans commented Dec 31, 2022

arkodg commented Jan 3, 2023

kflynn left a comment

Choose a reason for hiding this comment

danehans left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danehans Dec 21, 2022 •

edited

Loading