New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: local rate limit #2258

Merged

zirain merged 21 commits into envoyproxy:main from zhaohuabing:local-rate-limit

Dec 21, 2023

Member

zhaohuabing commented Nov 30, 2023

Related issue: #1336

zhaohuabing requested a review from a team as a code owner

November 30, 2023 13:13

zhaohuabing marked this pull request as draft

November 30, 2023 13:13

zhaohuabing changed the title ~~local rate limit~~ feat: local rate limit

zhaohuabing force-pushed the local-rate-limit branch from 03dfcab to fd6b283 Compare

November 30, 2023 14:12

codecov bot commented Nov 30, 2023 •

edited

Loading

Codecov Report

Attention: 142 lines in your changes are missing coverage. Please review.

Comparison is base (c2f88f0) 64.59% compared to head (b1cdeb1) 64.71%.
Report is 2 commits behind head on main.

Files	Patch %	Lines
internal/gatewayapi/backendtrafficpolicy.go	67.66%	59 Missing and 6 partials ⚠️
internal/xds/translator/local_ratelimit.go	76.14%	39 Missing and 13 partials ⚠️
internal/ir/zz_generated.deepcopy.go	0.00%	25 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2258      +/-   ##
==========================================
+ Coverage   64.59%   64.71%   +0.11%     
==========================================
  Files         112      113       +1     
  Lines       16072    16442     +370     
==========================================
+ Hits        10382    10640     +258     
- Misses       5038     5132      +94     
- Partials      652      670      +18

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zhaohuabing force-pushed the local-rate-limit branch 6 times, most recently from 47d6937 to 9734e3a Compare

December 1, 2023 08:17

zhaohuabing marked this pull request as ready for review

December 1, 2023 08:17

zhaohuabing force-pushed the local-rate-limit branch 9 times, most recently from c363f43 to 186f0a1 Compare

December 1, 2023 12:46

zhaohuabing marked this pull request as draft

December 2, 2023 01:53

zhaohuabing force-pushed the local-rate-limit branch 8 times, most recently from e8b3dec to 726beb4 Compare

December 4, 2023 02:09

github-actions bot temporarily deployed to pull request

December 15, 2023 22:58

Inactive

arkodg reviewed

View reviewed changes

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

arkodg reviewed

View reviewed changes

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

arkodg reviewed

View reviewed changes

api/v1alpha1/ratelimit_types.go

@@ @@ -91,6 +133,7 @@ const ( @@
               	SourceMatchExact SourceMatchType = "Exact"
               	// SourceMatchDistinct Each IP Address within the specified Source IP CIDR is treated as a distinct client selector
               	// and uses a separate rate limit bucket/counter.
+              	// Note: This is only supported for Global Rate Limits.

Contributor

arkodg Dec 18, 2023

I suggest creating separate fields for Local and Global or adding CEL validation we cannot support something like Distinct for local

Member Author

zhaohuabing Dec 19, 2023

I am inclined to add a CEL validation. It's simpler.

Member Author

zhaohuabing Dec 20, 2023 •

edited

Loading

This has been validated when translating GatewayAPI to IR, and the Policy status is set to Invalid if the type is Distinct.

I tried to validate it with the below CEL rule but no luck:

// +kubebuilder:validation:XValidation:rule="self.rules.all(r, has(r.clientSelectors)? (r.clientSelectors.all(s, has(s.sourceCIDR) && has(s.sourceCIDR.type)? s.sourceCIDR.type!='Distinct' : true)) : true)", message="Distinct SourceMatchType is not supported for Local Rate Limit"

And verified it on https://playcel.undistro.io/.

However, k8s throws the below error, complaining this rule is invalid.

Error:      	Received unexpected error:
        	            	unable to install CRDs onto control plane: unable to create CRD instances: unable to create CRD "backendtrafficpolicies.gateway.envoyproxy.io": CustomResourceDefinition.apiextensions.k8s.io "backendtrafficpolicies.gateway.envoyproxy.io" is invalid: spec.validation.openAPIV3Schema.properties[spec].properties[rateLimit].properties[local].properties[rules].x-kubernetes-validations[0].rule: Invalid value: apiextensions.ValidationRule{Rule:"self.rules.all(r, has(r.clientSelectors)? (r.clientSelectors.all(s, has(s.sourceCIDR) && has(s.sourceCIDR.type)? s.sourceCIDR.type!='Distinct' : true)) : true)", Message:"Distinct SourceMatchType is not supported for Local Rate Limit", MessageExpression:""}: compilation failed: ERROR: <input>:1:5: type 'list_type:{elem_type:{message_type:"selfType61480208.@idx"}}' does not support field selection
        	            	 | self.rules.all(r, has(r.clientSelectors)? (r.clientSelectors.all(s, has(s.sourceCIDR) && has(s.sourceCIDR.type)? s.sourceCIDR.type!='Distinct' : true)) : true)

Maybe I can try CEL later in a follow-up PR.

arkodg reviewed

View reviewed changes

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

zhaohuabing added 6 commits

December 20, 2023 09:27


          Merge branch 'main' into local-rate-limit

ea42881


          address comments

0bb9b8e

Signed-off-by: huabing zhao <[email protected]>


          Merge branch 'main' into local-rate-limit

5c7ad9a


          fix e2e test

b532711

Signed-off-by: huabing zhao <[email protected]>


          fix lint

37ce393

Signed-off-by: huabing zhao <[email protected]>


          fix e2e test

a8da2c7

Signed-off-by: huabing zhao <[email protected]>

zhaohuabing force-pushed the local-rate-limit branch from ef09625 to a8da2c7 Compare

December 20, 2023 08:21

zhaohuabing added 2 commits

December 20, 2023 16:37


          fix e2e test

Signed-off-by: huabing zhao <[email protected]>


          add comments to explain global rate limit rules

e24f056

Signed-off-by: huabing zhao <[email protected]>

zhaohuabing commented

View reviewed changes

api/v1alpha1/ratelimit_types.go

+              	// rate limit decisions of all matching rules. For example, if a request
+              	// matches two rules, one rate limited and one not, the final decision will be
+              	// to rate limit the request.
+              	// +kubebuilder:validation:MaxItems=16

Member Author

zhaohuabing Dec 20, 2023 •

edited

Loading

Adding a few lines to explain what happens if a request matches multiple rules, originally from https://github.com/envoyproxy/ratelimit?tab=readme-ov-file#example-2

zhaohuabing added 2 commits

December 20, 2023 19:01


          fix gen

bac22c9

Signed-off-by: huabing zhao <[email protected]>


          add comments to explain local rate limit rules

07c8397

Signed-off-by: huabing zhao <[email protected]>

zhaohuabing commented

View reviewed changes

api/v1alpha1/ratelimit_types.go

+              	// Rules are a list of RateLimit selectors and limits. If a request matches
+              	// multiple rules, the strictest limit is applied. For example, if a request
+              	// matches two rules, one with 10rps and one with 20rps, the final limit will
+              	// be based on the rule with 10rps.

Member Author

zhaohuabing Dec 20, 2023 •

edited

Loading

Explaining how local rate limit behaves if a request matches multiple rules, originally from https://www.envoyproxy.io/docs/envoy/latest/configuration/http/http_filters/local_rate_limit_filter#using-rate-limit-descriptors-for-local-rate-limiting

arkodg reviewed

View reviewed changes

api/v1alpha1/ratelimit_types.go Show resolved Hide resolved


          address comments

4e229ba

Signed-off-by: huabing zhao <[email protected]>

zhaohuabing requested review from arkodg and zirain

December 21, 2023 00:41

zhaohuabing added 2 commits

December 21, 2023 08:41


          Merge branch 'main' into local-rate-limit

5ac3836


          Merge branch 'main' into local-rate-limit

c0bc74a

zhaohuabing force-pushed the local-rate-limit branch from 801e6a8 to c0bc74a Compare

December 21, 2023 07:37


          fix gen

b1cdeb1

Signed-off-by: huabing zhao <[email protected]>

Member Author

zhaohuabing commented Dec 21, 2023

/retest

arkodg approved these changes

View reviewed changes

Contributor

arkodg left a comment

LGTM thanks !

zirain approved these changes

View reviewed changes

Contributor

zirain left a comment

LGTM, let's finish user-face doc with following PRs.

zirain merged commit da0d0dc into envoyproxy:main

18 checks passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels