Explicit unlimited ratelimits #261

lmajercak-wish · 2021-06-09T21:37:10Z

Provide the ability to explicitly set a rate limit descriptor to be unlimited to avoid calls to redis/memcached.

#241

mattklein123

Thanks for adding this. Can you add some documentation and then we can go from there? Thank you.

/wait

examples/ratelimit/config/example.yaml

mattklein123

/wait

README.md

mattklein123 · 2021-06-17T16:27:01Z

Sorry can you fix DCO? Feel free to force push in this case and then we can continue the review from there.

/wait

Signed-off-by: lmajercak-wish <[email protected]>

lmajercak-wish · 2021-06-17T16:32:17Z

Sorry can you fix DCO? Feel free to force push in this case and then we can continue the review from there.

/wait

done

lmajercak-wish · 2021-06-28T16:27:04Z

Hi @mattklein123, would you be able to take a look again? Thanks! :)

mattklein123

Thanks LGTM with small comments.

/wait

mattklein123 · 2021-06-28T20:42:21Z

src/config/config_impl.go

 			value, present :=
 				pb.RateLimitResponse_RateLimit_Unit_value[strings.ToUpper(descriptorConfig.RateLimit.Unit)]
-			if !present || value == int32(pb.RateLimitResponse_RateLimit_UNKNOWN) {
+			if (!present || value == int32(pb.RateLimitResponse_RateLimit_UNKNOWN)) && !unlimited {


Can you split this logic out so that we actually check if unlimited is set with a unit at all and have an error message (and test) for that? I think that would be more clear to the user.

(Ping on this)

I wasn't too sure what exactly you meant, so please review this again :)

src/memcached/cache_impl.go

Signed-off-by: lmajercak-wish <[email protected]>

test/service/ratelimit_test.go

Signed-off-by: lmajercak-wish <[email protected]>

mattklein123

/wait

mattklein123 · 2021-07-02T15:14:32Z

README.md

+      requests_per_unit: 100
+```
+
+For an unlimited descriptor, the request will not be sent to the underlying cache (Redis/Memcached), but will be quickly returned locally by the ratelimit instance.


Can you mention why someone would want to do this? I assume for stats, etc.?

Well, stats yes. But also, in our case, our client will not allow a request to go through if a descriptor is not found, so we do have to have it defined.

I'll add something to the readme

mattklein123 · 2021-07-02T15:14:49Z

src/config/config_impl.go

 			value, present :=
 				pb.RateLimitResponse_RateLimit_Unit_value[strings.ToUpper(descriptorConfig.RateLimit.Unit)]
-			if !present || value == int32(pb.RateLimitResponse_RateLimit_UNKNOWN) {
+			if (!present || value == int32(pb.RateLimitResponse_RateLimit_UNKNOWN)) && !unlimited {


(Ping on this)

src/memcached/cache_impl.go

test/service/ratelimit_test.go

mattklein123 · 2021-07-06T15:12:36Z

Please do not force push after the PR is started. It breaks the review process. Just merge main and add commits. Thank you!

mattklein123

Thanks LGTM with a small doc comment.

/wait

mattklein123 · 2021-07-06T15:14:28Z

src/service/ratelimit.go

+		if isUnlimited[i] {
+			response.Statuses[i] = &pb.RateLimitResponse_DescriptorStatus{
+				Code:           pb.RateLimitResponse_OK,
+				LimitRemaining: math.MaxUint32,


Can you document this somewhere? It should probably go directly in the proto docs as a comment? Or potentially also somewhere in the README?

Not sure what you mean by the proto docs, I added a comment to the README

I mean doing a PR here also: https://github.com/envoyproxy/envoy/blob/7c1991ffe382bacf2bf415a643c5ea7428d0880e/api/envoy/service/ratelimit/v3/rls.proto#L130-L131

This is the canonical service definition for the ratelimit service.

lmajercak-wish · 2021-07-06T15:22:06Z

Please do not force push after the PR is started. It breaks the review process. Just merge main and add commits. Thank you!

I just followed the DCO steps

mattklein123 · 2021-07-07T15:50:42Z

@lmajercak-wish now that DCO is broken you will need to squash and force push, so please do that. In the future please make sure to only add commits and sign off each commit. Thank you!

/wait

Signed-off-by: lmajercak-wish <[email protected]>

mattklein123

Thanks!

mattklein123 · 2021-07-07T21:27:26Z

Oops looks like CI issue.

/wait

Signed-off-by: lmajercak-wish <[email protected]>

mattklein123

Thank you!

lmajercak-wish · 2021-07-07T23:11:33Z

Thank you!

Thank you @mattklein123 !

Signed-off-by: lmajercak-wish <[email protected]>

lmajercak-wish mentioned this pull request Jun 9, 2021

Proposal: Add explicit unlimited rate limits #241

Open

lmajercak-wish force-pushed the lmajercak_unlimited_ratelimits branch from a25a722 to 167963c Compare June 9, 2021 21:38

mattklein123 self-assigned this Jun 11, 2021

mattklein123 requested changes Jun 14, 2021

View reviewed changes

examples/ratelimit/config/example.yaml Outdated Show resolved Hide resolved

repokitteh-read-only bot added waiting and removed waiting labels Jun 14, 2021

mattklein123 reviewed Jun 16, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

repokitteh-read-only bot added waiting and removed waiting labels Jun 16, 2021

repokitteh-read-only bot added the waiting label Jun 17, 2021

lmajercak-wish added 7 commits June 17, 2021 11:27

Explicit unlimited ratelimits

a3f32e1

Signed-off-by: lmajercak-wish <[email protected]>

Cleanup

0ef96da

Signed-off-by: lmajercak-wish <[email protected]>

Remove unnecessary comment

7f577aa

Signed-off-by: lmajercak-wish <[email protected]>

Add example to README + remove requests_per_unit

8f007c7

Signed-off-by: lmajercak-wish <[email protected]>

Remove requests_per_unit

651aca7

Signed-off-by: lmajercak-wish <[email protected]>

Fix example in the README

df4f71a

Signed-off-by: lmajercak-wish <[email protected]>

Remove the need to specify unit for unlimited descriptors

6a81d9b

Signed-off-by: lmajercak-wish <[email protected]>

lmajercak-wish force-pushed the lmajercak_unlimited_ratelimits branch from d59efa8 to 6a81d9b Compare June 17, 2021 16:28

repokitteh-read-only bot removed the waiting label Jun 17, 2021

mattklein123 requested changes Jun 28, 2021

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Jun 28, 2021

Do not send unlimited descriptors to the cache

01de07f

Signed-off-by: lmajercak-wish <[email protected]>

lmajercak-wish force-pushed the lmajercak_unlimited_ratelimits branch from c854586 to 01de07f Compare June 30, 2021 15:54

Cleanup

2d18c1c

Signed-off-by: lmajercak-wish <[email protected]>

lmajercak-wish force-pushed the lmajercak_unlimited_ratelimits branch from 2290800 to 2d18c1c Compare June 30, 2021 15:55

lmajercak-wish commented Jun 30, 2021

View reviewed changes

test/service/ratelimit_test.go Outdated Show resolved Hide resolved

Add comment

8988873

Signed-off-by: lmajercak-wish <[email protected]>

mattklein123 requested changes Jul 2, 2021

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Jul 2, 2021

lmajercak-wish force-pushed the lmajercak_unlimited_ratelimits branch from 283c882 to 7adeec5 Compare July 2, 2021 22:45

mattklein123 requested changes Jul 6, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 6, 2021

repokitteh-read-only bot removed the waiting label Jul 7, 2021

lmajercak-wish force-pushed the lmajercak_unlimited_ratelimits branch from 35e338b to 5b69edf Compare July 7, 2021 15:35

repokitteh-read-only bot added the waiting label Jul 7, 2021

lmajercak-wish force-pushed the lmajercak_unlimited_ratelimits branch from 5b69edf to 629323a Compare July 7, 2021 15:52

repokitteh-read-only bot removed the waiting label Jul 7, 2021

lmajercak-wish force-pushed the lmajercak_unlimited_ratelimits branch from 629323a to 2d561da Compare July 7, 2021 16:00

Address comments

0862dd8

Signed-off-by: lmajercak-wish <[email protected]>

lmajercak-wish force-pushed the lmajercak_unlimited_ratelimits branch from 2d561da to 0862dd8 Compare July 7, 2021 16:10

mattklein123 previously approved these changes Jul 7, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 7, 2021

Fix build

c8bf7e3

Signed-off-by: lmajercak-wish <[email protected]>

lmajercak-wish dismissed mattklein123’s stale review via c8bf7e3 July 7, 2021 21:46

repokitteh-read-only bot removed the waiting label Jul 7, 2021

Fix build

e3cc523

Signed-off-by: lmajercak-wish <[email protected]>

mattklein123 approved these changes Jul 7, 2021

View reviewed changes

mattklein123 merged commit edab0ec into envoyproxy:main Jul 7, 2021

zdmytriv pushed a commit to verygoodsecurity/ratelimit that referenced this pull request Aug 2, 2021

Explicit unlimited ratelimits (envoyproxy#261)

2ef87c4

Signed-off-by: lmajercak-wish <[email protected]>

timcovar pushed a commit to goatapp/ratelimit that referenced this pull request Jan 16, 2024

Explicit unlimited ratelimits (envoyproxy#261)

cd1e134

Signed-off-by: lmajercak-wish <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explicit unlimited ratelimits #261

Explicit unlimited ratelimits #261

lmajercak-wish commented Jun 9, 2021 •

edited

Loading

mattklein123 left a comment

mattklein123 left a comment

mattklein123 commented Jun 17, 2021

lmajercak-wish commented Jun 17, 2021

lmajercak-wish commented Jun 28, 2021

mattklein123 left a comment

mattklein123 Jun 28, 2021

mattklein123 Jul 2, 2021

lmajercak-wish Jul 2, 2021

mattklein123 left a comment

mattklein123 Jul 2, 2021

lmajercak-wish Jul 2, 2021

lmajercak-wish Jul 2, 2021

lmajercak-wish Jul 2, 2021

mattklein123 Jul 2, 2021

mattklein123 commented Jul 6, 2021

mattklein123 left a comment

mattklein123 Jul 6, 2021

lmajercak-wish Jul 6, 2021

mattklein123 Jul 6, 2021

lmajercak-wish commented Jul 6, 2021

mattklein123 commented Jul 7, 2021

mattklein123 left a comment

mattklein123 commented Jul 7, 2021

mattklein123 left a comment

lmajercak-wish commented Jul 7, 2021

Explicit unlimited ratelimits #261

Explicit unlimited ratelimits #261

Conversation

lmajercak-wish commented Jun 9, 2021 • edited Loading

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 commented Jun 17, 2021

lmajercak-wish commented Jun 17, 2021

lmajercak-wish commented Jun 28, 2021

mattklein123 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

mattklein123 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

Choose a reason for hiding this comment

mattklein123 commented Jul 6, 2021

mattklein123 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

lmajercak-wish commented Jul 6, 2021

mattklein123 commented Jul 7, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

mattklein123 commented Jul 7, 2021

mattklein123 left a comment

Choose a reason for hiding this comment

lmajercak-wish commented Jul 7, 2021

lmajercak-wish commented Jun 9, 2021 •

edited

Loading