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

Rate Limit Quota #19793

Merged
merged 44 commits into from
Jun 30, 2022
Merged

Rate Limit Quota #19793

merged 44 commits into from
Jun 30, 2022

Conversation

sergiitk
Copy link
Contributor

@sergiitk sergiitk commented Feb 3, 2022 •
edited
Loading

This is an alternative to existing RLS, redesigned for streaming.

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link

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: #19793 was opened by sergiitk.

see: more, trace.

@repokitteh-read-only
Copy link

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

🐱

Caused by: #19793 was opened by sergiitk.

see: more, trace.

@sergiitk
Copy link
Contributor Author

sergiitk commented Feb 3, 2022

@tyxia As discussed IRL, please take a look.

@sergiitk sergiitk mentioned this pull request Feb 3, 2022
Open
markdroth
markdroth reviewed Feb 7, 2022
View reviewed changes
Copy link
Contributor

@markdroth markdroth 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 doing this!

Please let me know if you have any questions. Thanks!

tyxia
tyxia reviewed Feb 7, 2022
View reviewed changes
Copy link
Member

@tyxia tyxia left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for working on this! Some comments from me on the first pass.

PS: Oops. Just noticed some comments are overlapped with Mark's. Those were in my earlier draft(shown in edited history). Forgot to refresh the page before sending out the draft.

tyxia
tyxia reviewed Feb 8, 2022
View reviewed changes
Copy link
Member

@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 iterating on this!

I haven't gone through the PR thoroughly yet but want to send some comments for early discussion.

@tyxia
Copy link
Member

tyxia commented Feb 8, 2022

/assign @tyxia

@yanavlasov yanavlasov self-assigned this Feb 8, 2022
markdroth
markdroth reviewed Feb 8, 2022
View reviewed changes
Copy link
Contributor

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Making good progress!

Please let me know if you have any questions. Thanks!

@htuch
Copy link
Member

htuch commented Feb 9, 2022

We need a non-Googler to do API review here as well. Unassigning myself as API shepherd. Suggest @mattklein123 takes a look as someone with rate limit domain experience.

@htuch htuch removed their assignment Feb 9, 2022
@mattklein123 mattklein123 self-assigned this Feb 9, 2022
@mattklein123
Copy link
Member

I have a high level question before diving into the review. Should this be built into the existing global rate limit filter? It seems like there is a lot of overlap? If this has been discussed somewhere else please let me know and I can take a look. Thank you.

@markdroth
Copy link
Contributor

Matt, what we're building here wound up being a whole new protocol with somewhat different semantics to the existing RLS protocol. I think it makes more sense to make it a new filter rather than trying to shoe-horn it into the existing one. I suspect the two would not have much code in common.

yanavlasov
yanavlasov requested changes Feb 14, 2022
View reviewed changes
Copy link
Contributor

@yanavlasov yanavlasov left a comment

Choose a reason for hiding this comment

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

/wait

@kyessenov kyessenov mentioned this pull request Mar 1, 2022
Closed
@yanavlasov
Copy link
Contributor

@markdroth @mattklein123 I'm trying to figure out how to best move forward with this extensions. Looking at the protocols as they now, they are quite different and I do not see an easy way to make them compatible. There are maybe some way to make bucket and descriptor specification compatible, not sure how much would be gained from this.

Would it make sense to then brand this filter such that it is Google specific? I.e. GcpRateLimit or GoogleCloudRateLimit? Without reference implementation for the rate limit server that supports this protocol, nobody would be able to use this extension outside of GCP.

@sergiitk
Copy link
Contributor Author

It still unfortunately breaks code review. Please don't do it. If we need to fix DCO we can just do it at the end. Thank you!

Got it, thanks! Sorry for the inconvenience.

@sergiitk
Address API shepards' feedback round 2; improve and expand wording
699957e 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk sergiitk requested a review from mattklein123 June 24, 2022 22:15
@sergiitk
Copy link
Contributor Author

@mattklein123 All set! Ready for another round.

@sergiitk
Fix lint: spelling (though disagreeable); remove unused import
39e9f7b 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
mattklein123
mattklein123 requested changes Jun 28, 2022
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM with one remaining comment. Thank you!

/wait

@sergiitk
Feedback: filter_enforced defaults to 100%
ad0fdd1 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk
Merge branch 'main' into rlqs
ab15657 
Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk sergiitk requested a review from mattklein123 June 28, 2022 18:44
mattklein123
mattklein123 approved these changes Jun 29, 2022
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks!

@mattklein123 mattklein123 removed their assignment Jun 29, 2022
@yanavlasov yanavlasov merged commit 259d76e into envoyproxy:main Jun 30, 2022
@sergiitk
Copy link
Contributor Author

@mattklein123 We didn't coordinate removing the filter - will make a separate PR right now.

sergiitk added a commit to sergiitk/envoy that referenced this pull request Jun 30, 2022
@sergiitk
RLQS: hide unimplemented documentation
a6ef9fa 
RLQS API added in envoyproxy#19793.
This change removes the docs and adds [#not-implemented-hide:] tags
until the implementation is ready.

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
@sergiitk sergiitk mentioned this pull request Jun 30, 2022
Merged
@sergiitk sergiitk deleted the rlqs branch June 30, 2022 18:33
mattklein123 pushed a commit that referenced this pull request Jul 6, 2022
@sergiitk
RLQS: hide unimplemented documentation (#21966)
3af90c1 
RLQS API added in #19793.
This change removes the docs and adds [#not-implemented-hide:] tags
until the implementation is ready.

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
lizan pushed a commit to envoyproxy/data-plane-api that referenced this pull request Jul 6, 2022
RLQS: hide unimplemented documentation (#21966)
6c5bd3d 
RLQS API added in envoyproxy/envoy#19793.
This change removes the docs and adds [#not-implemented-hide:] tags
until the implementation is ready.

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>

Mirrored from https://github.com/envoyproxy/envoy @ 3af90c11e40ecf175aaa830a5e1daebd15551b54
@sergiitk sergiitk mentioned this pull request Jul 12, 2022
Closed
@mattklein123 mattklein123 mentioned this pull request Oct 14, 2022
Closed
oschaaf pushed a commit to maistra/envoy that referenced this pull request Oct 26, 2022
@sergiitk
RLQS: hide unimplemented documentation (#21966)
ab2dde4 
RLQS API added in envoyproxy/envoy#19793.
This change removes the docs and adds [#not-implemented-hide:] tags
until the implementation is ready.

Signed-off-by: Sergii Tkachenko <sergiitk@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@yanavlasov yanavlasov yanavlasov approved these changes

@tyxia tyxia Awaiting requested review from tyxia

@kyessenov kyessenov Awaiting requested review from kyessenov

@markdroth markdroth Awaiting requested review from markdroth

Assignees

@yanavlasov yanavlasov

@tyxia tyxia

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@sergiitk @tyxia @htuch @mattklein123 @markdroth @yanavlasov @RyanTheOptimist @kyessenov