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

extensions: add http bandwidth limit filter #14096

Closed
wants to merge 53 commits into from
Closed

extensions: add http bandwidth limit filter #14096

wants to merge 53 commits into from

Conversation

nitgoy
Copy link
Contributor

@nitgoy nitgoy commented Nov 19, 2020 •
edited
Loading

Commit Message: adds bandwidth limit filter core functionality and UTs
Additional Description: the filter smoothens the flow of data in both direction up to the specified bandwidth limit.
Risk Level:
Testing: UTs added. Adding more UTs and integration tests.
Docs Changes: Pending
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #13604]
[Optional Deprecated:]

@repokitteh-read-only
Copy link

Hi @nitgoy, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #14096 was opened by nitgoy.

see: more, trace.

@repokitteh-read-only
Copy link

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

🐱

Caused by: #14096 was opened by nitgoy.

see: more, trace.

@nitgoy nitgoy mentioned this pull request Nov 19, 2020
Closed
tonya11en
tonya11en reviewed Nov 23, 2020
View reviewed changes
Copy link
Member

@tonya11en tonya11en left a comment

Choose a reason for hiding this comment

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

Looks great!

You'll want to see how other filters implement their config as an alpha version so the proto isn't locked in.

Let's use this PR for the whole filter review! I'll stand by for the rest of it. Thanks for doing this.

api/envoy/extensions/filters/http/bandwidth_limit/v3/bandwidth_limit.proto Outdated Show resolved Hide resolved
api/envoy/extensions/filters/http/bandwidth_limit/v3/bandwidth_limit.proto Outdated Show resolved Hide resolved
api/envoy/extensions/filters/http/bandwidth_limit/v3/bandwidth_limit.proto Outdated Show resolved Hide resolved
api/envoy/extensions/filters/http/bandwidth_limit/v3/bandwidth_limit.proto Outdated Show resolved Hide resolved
@nitgoy nitgoy force-pushed the bandwidthlimitproto branch from 1526a7e to 1f11d00 Compare December 2, 2020 05:09
nitgoy and others added 7 commits December 3, 2020 19:14
@nitgoy
config and test related changes
3aafb4e 
Signed-off-by: Nitin <nigoyal@microsoft.com>
@nitgoy
fix spellings
42e46d9 
Signed-off-by: Nitin <nigoyal@microsoft.com>
@nitgoy
update generated config
87c2751 
Signed-off-by: Nitin <nigoyal@microsoft.com>
@nitgoy
move configs to v3alpha
5da9feb
@nitgoy
bandwidth limit filter config
036fbed 
Signed-off-by: Nitin <nigoyal@microsoft.com>

Signed-off-by: Nitin Goyal <nrk.goyal@gmail.com>
@nitgoy
Address comments: remove enforce threshold
6b95e43 
Signed-off-by: Nitin <nigoyal@microsoft.com>

Signed-off-by: Nitin Goyal <nrk.goyal@gmail.com>
@nitgoy
move proto to alpha
45840d2 
Signed-off-by: Nitin <nigoyal@microsoft.com>

Signed-off-by: Nitin Goyal <nrk.goyal@gmail.com>
nitgoy added 5 commits January 5, 2021 18:54
@nitgoy
make token bucket conditionally thread-safe and add comment for strea…
56c50ea 
…m rate limiter

Signed-off-by: Nitin <nigoyal@microsoft.com>
@nitgoy
fix reset
662bf2b 
Signed-off-by: Nitin <nigoyal@microsoft.com>
@nitgoy
make token bucket optionally thread-safe and add comment for stream r…
8a0904b 
…ate limiter

Signed-off-by: Nitin <nigoyal@microsoft.com>
@nitgoy
remove some absl guards
bd537f9 
Signed-off-by: Nitin <nigoyal@microsoft.com>
@nitgoy
Merge branch 'bwprotodev' into bandwidthlimitproto
91e5b25 
Signed-off-by: Nitin <nigoyal@microsoft.com>
Base automatically changed from master to main January 15, 2021 23:01
@nitgoy
Copy link
Contributor Author

nitgoy commented Jan 26, 2021 •
edited
Loading

As suggested, I've created two separate PRs:
Token bucket: #14827
Stream rate limiter: #14828

Hope to get those merged while I address the remaining comments here.

@nitgoy nitgoy mentioned this pull request Jan 27, 2021
Merged
@mattklein123
Copy link
Member

Sorry I haven't looked at this yet, but will put this in waiting until the base PRs are merged and then we can circle back.

/wait

@leoribeiro2
Copy link

leoribeiro2 commented Mar 10, 2021 •
edited
Loading

Hi guys!
Have you any updates about that feat?

@nitgoy

@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 10, 2021
@nitgoy
Copy link
Contributor Author

nitgoy commented Apr 11, 2021 •
edited
Loading

Hi guys!
Have you any updates about that feat?

@nitgoy

Sorry it's taken so long. We're now using it for our product so that helped to uncover some functional issues. Hoping to push out new changes pretty soon. Most likely after completing this dependent PR: #14828

@github-actions github-actions bot removed the stale stalebot believes this issue/PR has not been touched recently label Apr 12, 2021
@nitgoy
merge from upstream
36438b6 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@mattklein123 mattklein123 removed their assignment Apr 16, 2021
nitgoy added 2 commits April 22, 2021 14:24
@nitgoy
merge streamlimiter changes from upstream
abee221 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
update filter
6b4eb66 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
update proto
4c2a96d 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy nitgoy marked this pull request as ready for review May 4, 2021 21:56
nitgoy added 3 commits May 5, 2021 22:39
@nitgoy
add docs
a48c556 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
proto and doc fixes
890ce21 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
test fix
2cbf38f 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
Copy link
Contributor Author

nitgoy commented May 6, 2021

I have opened a replacement PR for this one after addressing most of the comments: #16358. I was running into some DCO issues which were getting non-trivial to fix. Please take a look at the new PR, will close this one.

@nitgoy nitgoy closed this May 6, 2021
@nitgoy nitgoy deleted the bandwidthlimitproto branch June 3, 2021 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phlax phlax phlax left review comments

@tonya11en tonya11en tonya11en requested changes

Assignees

@tonya11en tonya11en

Labels
api
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

how to config bandwidth limit
5 participants
@nitgoy @tonya11en @mattklein123 @leoribeiro2 @phlax