Skip to content

rate_limit_quota: Quota based rate limiting, part 1#22135

Merged
yanavlasov merged 72 commits intoenvoyproxy:mainfrom
tyxia:rls_init
Dec 1, 2022
Merged

rate_limit_quota: Quota based rate limiting, part 1#22135
yanavlasov merged 72 commits intoenvoyproxy:mainfrom
tyxia:rls_init

Conversation

@tyxia
Copy link
Copy Markdown
Member

@tyxia tyxia commented Jul 12, 2022

First part of quota-based rate limiting feature. The majority of this PR is to lay out the basic framework and shape of major components with unit test coverage, other functionalities (e.g., periodically usage report, thread local cache, quota assignment, etc) will be added in next PRs. This PR includes:

  • Rate limit quota filter basic framework:
    Filter factory, filter creation and registration, filter configuration, etc.
  • Bidirectional gRPC basic framework :
    gRPC stream start and close, message send and receive, callback for the client and filter interaction, etc
  • Request matching:
    1. matcher tree creation
    2. perform request matching with request header and exact value_match field from the config
    3. on_no_match support (empty bucketId with noassignmentBehavior for now for illustration purpose, will be
      modified in the next PRs)
  • BucketId generation:
    Generates the BucketId via either static method (string_value) or dynamic method (custom_value with
    typed_config).
  • Quota assignment caching:
    UseBucketId as the key with customized hash/equal function. Will change to thread local storage
  • Unit tests for the config, filter and grpc client,
  • Doc changes has been completed and merged in PR rate_limit_quota: [Doc] add the docs for rate limit quota feature #22539.

Note to reviewers:
In sprit of small PRs and making review easier, I plan to divide the whole implementation into several parts. Hence, I

  • Removed some refer link in the proto for the ease of a boilerplate doc generation (this feature is WIP). Also, I have left
    some TODOs that will be addressed in next PRs
  • Temporarily reduced the code coverage requirement for this extension as this is first part of the several PR. Currently, it is ~75% coverage.

Signed-off-by: Tianyu Xia tyxia@google.com

@repokitteh-read-only
Copy link
Copy Markdown

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #22135 was opened by tyxia.

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #22135 was opened by tyxia.

see: more, trace.

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Jul 12, 2022

/unassign @mattklein123

Initial commit that just adds back the docs as part of the implementation. Not ready for review yet.

phlax and others added 3 commits July 12, 2022 16:40
envoyproxy#22116)

Revert "build(deps): bump orjson from 3.6.7 to 3.7.7 in /tools/base (envoyproxy#22112)"

This reverts commit 2f8b986.

Signed-off-by: Ryan Northey <ryan@synca.io>
Signed-off-by: Tianyu Xia <tyxia@google.com>
This reverts commit 3af90c1.

Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
@repokitteh-read-only repokitteh-read-only Bot added the deps Approval required for changes to Envoy's external dependencies label Jul 12, 2022
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @mattklein123

🐱

Caused by: #22135 was synchronize by tyxia.

see: more, trace.

tyxia added 2 commits July 12, 2022 16:43
Signed-off-by: Tianyu Xia <tyxia@google.com>
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Jul 12, 2022

/unassign @mattklein123

Initial commit that just adds back the docs as part of the implementation. Not ready for review yet.

@tyxia tyxia changed the title Initial implementation of rate limiting quota filter [WIP, Not ready for review] Initial implementation of rate limiting quota filter Jul 20, 2022
@tyxia tyxia changed the title [WIP, Not ready for review] Initial implementation of rate limiting quota filter [WIP] Initial implementation of rate limiting quota filter Jul 25, 2022
tyxia added 9 commits August 3, 2022 15:28
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
Comment thread api/envoy/extensions/filters/http/rate_limit_quota/v3/rate_limit_quota.proto Outdated
Copy link
Copy Markdown
Member Author

@tyxia tyxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review!

Comment thread api/envoy/extensions/filters/http/rate_limit_quota/v3/rate_limit_quota.proto Outdated
Comment thread source/extensions/filters/http/rate_limit_quota/filter.h Outdated
Comment thread source/extensions/filters/http/rate_limit_quota/filter.cc Outdated
Comment thread source/extensions/filters/http/rate_limit_quota/client.h Outdated
Comment thread source/extensions/filters/http/rate_limit_quota/client_impl.cc
Signed-off-by: Tianyu Xia <tyxia@google.com>
tyxia added 2 commits November 7, 2022 21:43
Signed-off-by: Tianyu Xia <tyxia@google.com>
Signed-off-by: Tianyu Xia <tyxia@google.com>
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 7, 2022

@yanavlasov PTAL. Thanks!

I have addressed the review comments. The CI doc failure in this PR will be resolved once PR #22539 is merged (That PR is also under review now).

Signed-off-by: tyxia <tyxia@google.com>
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 22, 2022

/retest

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Check envoy-presubmit didn't fail.

🐱

Caused by: a #22135 (comment) was created by @tyxia.

see: more, trace.

@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 30, 2022

/retest

The CI failure should has nothing to do with this PR

@repokitteh-read-only
Copy link
Copy Markdown

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #22135 (comment) was created by @tyxia.

see: more, trace.

@phlax
Copy link
Copy Markdown
Member

phlax commented Nov 30, 2022

@tyxia i think the tsan problem has been fixed on main here #24252

the other problem is coverage, which is saying:

Code coverage 96.0 is lower than limit of 96.1

is it possible this fail is genuine ?

Signed-off-by: tyxia <tyxia@google.com>
@tyxia
Copy link
Copy Markdown
Member Author

tyxia commented Nov 30, 2022

@tyxia i think the tsan problem has been fixed on main here #24252

the other problem is coverage, which is saying:

Code coverage 96.0 is lower than limit of 96.1

is it possible this fail is genuine ?

Thanks for information, @phlax. There was an accidental change to coverage file in my last merge. I just reverted it. Let see if CD/CI is happy this time.

Signed-off-by: tyxia <tyxia@google.com>
Signed-off-by: tyxia <tyxia@google.com>
@yanavlasov yanavlasov merged commit 9f1283f into envoyproxy:main Dec 1, 2022
Comment thread test/per_file_coverage.sh
alyssawilk added a commit to alyssawilk/envoy that referenced this pull request Dec 5, 2022
alyssawilk added a commit to alyssawilk/envoy that referenced this pull request Dec 5, 2022
@tyxia tyxia deleted the rls_init branch December 26, 2024 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api deps Approval required for changes to Envoy's external dependencies

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants