Skip to content
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

ratelimit: allow rate limit names in the global rate limit response #10557

Merged
merged 1 commit into from
Apr 7, 2020
Merged

ratelimit: allow rate limit names in the global rate limit response #10557

merged 1 commit into from
Apr 7, 2020

Conversation

dweitzman
Copy link
Contributor

Description:
This part of the global rate limiter api is not currently used by envoy, but for anyone implementing the envoy global rate limiter api being able to describe what specific limit you've hit in a human-readable and/or machine-readable way will be useful for debugging, alerting, etc.

Risk Level: Low
Testing: n/a
Docs Changes: n/a
Release Notes: n/a
Fixes #10556

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10557 was opened by dweitzman.

see: more, trace.

htuch
htuch suggested changes Mar 30, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

/wait

api/envoy/service/ratelimit/v3/rls.proto Outdated
@@ -91,6 +91,9 @@ message RateLimitResponse {

// The unit of time.
Unit unit = 2;

// A name or description of this limit.
string name = 3;
Copy link
Member

Choose a reason for hiding this comment

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

This seems reasonable. Could you place it as the first field (but retain field number 3) for clarity? Thanks.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

Can you run fix_format so that the CI checks pass? Thanks. You will probably want to make the change to the v2 RLS, as fix_format currently forward propagates that to v3.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is my first envoy PR so I might be running fix format incorrectly, but it's not doing anything to these files. The format check errors on CI look unrelated?...

diff: /source/bazel-bin/tools/testdata/protoxform/envoy/v2/protos/tools/testdata/protoxform/envoy/v2/oneof.proto.v3.proto: No such file or directory

diff: /source/bazel-bin/tools/testdata/protoxform/envoy/v2/protos/tools/testdata/protoxform/envoy/v2/oneof.proto.v3.envoy_internal.proto: No such file or directory

diff: /source/bazel-bin/tools/testdata/protoxform/envoy/v2/protos/tools/testdata/protoxform/envoy/v2/discovery_service.proto.v2.proto: No such file or directory

diff: /source/bazel-bin/tools/testdata/protoxform/envoy/v2/protos/tools/testdata/protoxform/envoy/v2/discovery_service.proto.v3.proto: No such file or directory

diff: /source/bazel-bin/tools/testdata/protoxform/envoy/v2/protos/tools/testdata/protoxform/envoy/v2/discovery_service.proto.v3.proto: No such file or directory

diff: /source/bazel-bin/tools/testdata/protoxform/envoy/v2/protos/tools/testdata/protoxform/envoy/v2/discovery_service.proto.v3.envoy_internal.proto: No such file or directory

Updated v2 as well

Copy link
Member

Choose a reason for hiding this comment

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

Yes, this is strange, I'll look into this, I can replicate locally.

Copy link
Member

Choose a reason for hiding this comment

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

Fix should be in #10582. If you scroll up, I think for some reason or another, check_format was unhappy with this PR still.

Copy link
Member

Choose a reason for hiding this comment

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

Just merged #10582, can you merge master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I merged from master. Still not getting any changes recommended when I run fix formatting

The command I'm using is tools/code_format/check_format.py fix api/envoy/service/ratelimit

@htuch htuch assigned htuch and unassigned junr03 Mar 30, 2020
@dweitzman dweitzman force-pushed the ratelimit_name branch 2 times, most recently from e8b185f to ac18449 Compare March 31, 2020 06:01
@htuch
Copy link
Member

htuch commented Apr 1, 2020

@dweitzman it looks like you haven't included the changes that fix_format should also be making to generated_api_shadow/. Can you verify that these files are included in your git history?

@dweitzman
Copy link
Contributor Author

Format should be updated (managed to install xcode, sudo xcode-select -s /Applications/Xcode.app/Contents/Developer, install bazelisk, and finally run ./tools/proto_format/proto_format.sh fix), but it seems like an ongoing github outage is impacting the presubmit tests

Whenever https://www.githubstatus.com/ stays the outage is over I'll rebase to get the tests to re-run

@dweitzman
Copy link
Contributor Author

Updated. test/extensions/common/proxy_protocol/proxy_protocol_regression_test.cc seems to have a compile error in the presubmit, but it seems unrelated to these changes

For my own future reference, to get the format fixer working I did this:

  • install xcode, sudo xcode-select -s /Applications/Xcode.app/Contents/Developer, install bazelisk, and finally run ./tools/proto_format/proto_format.sh fix

@htuch
Copy link
Member

htuch commented Apr 3, 2020

@dweitzman can you do another merge master and push? We've fixed some of these issues that CI failed on (master was broken yesterday for a bit). Thanks!

@dweitzman
ratelimit: allow rate limit names in the global rate limit response
bd5289c 
This part of the global rate limiter api is not currently used by envoy, but for anyone implementing the envoy global rate limiter api being able to describe what specific limit you've hit in a human-readable and/or machine-readable way will be useful for debugging, alerting, etc.

See envoyproxy#10556

Signed-off-by: David Weitzman <dweitzman@pinterest.com>
@dweitzman
Copy link
Contributor Author

Looks like the presubmit is green now

@repokitteh-read-only repokitteh-read-only bot removed the api label Apr 7, 2020
@htuch htuch merged commit 9970602 into envoyproxy:master Apr 7, 2020
htuch added a commit to htuch/envoy that referenced this pull request Apr 7, 2020
@htuch
Fixup for merging envoyproxy#10557.
4b790de 
Signed-off-by: Harvey Tuch <htuch@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch approved these changes

Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rate limiting: include rate limit names in global rate limiter response
3 participants
@dweitzman @htuch @junr03