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 #16358

Merged
merged 42 commits into from
May 25, 2021
Merged

extensions: add http bandwidth limit filter #16358

merged 42 commits into from
May 25, 2021

Conversation

nitgoy
Copy link
Contributor

@nitgoy nitgoy commented May 6, 2021 •
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: Low, new filter
Testing: UTs added. Adding more UTs and integration tests.
Docs Changes: Added
Release Notes: Added
Fixes #13604

@nitgoy
add bandwidth limit filter core functionality and UTs
4f21be4 
* Add bandwidthlimit filter skeleton

* fix build

* Add core logic

* fix build

* minor fix

* Add bandwidthlimit filter skeleton

* fix build

* Add core logic

* fix build

* minor fix

* minor

* add generated configs

* final build fixes

* config and test related changes

Signed-off-by: Nitin <nigoyal@microsoft.com>

* fix spellings

Signed-off-by: Nitin <nigoyal@microsoft.com>

* update generated config

Signed-off-by: Nitin <nigoyal@microsoft.com>

* move configs to v3alpha

* bandwidth limit filter config

Signed-off-by: Nitin <nigoyal@microsoft.com>

Signed-off-by: Nitin Goyal <nrk.goyal@gmail.com>

* Address comments: remove enforce threshold

Signed-off-by: Nitin <nigoyal@microsoft.com>

Signed-off-by: Nitin Goyal <nrk.goyal@gmail.com>

* move proto to alpha

Signed-off-by: Nitin <nigoyal@microsoft.com>

Signed-off-by: Nitin Goyal <nrk.goyal@gmail.com>

* bandwidth limit filter config

Signed-off-by: Nitin <nigoyal@microsoft.com>

* Address comments: remove enforce threshold

Signed-off-by: Nitin <nigoyal@microsoft.com>

* change limit field to optional

Signed-off-by: Nitin <nigoyal@microsoft.com>

Signed-off-by: Nitin Goyal <nrk.goyal@gmail.com>

* delete v3 file

Signed-off-by: Nitin <nigoyal@microsoft.com>

* generated configs

Signed-off-by: Nitin <nigoyal@microsoft.com>

* add spelling

Signed-off-by: Nitin <nigoyal@microsoft.com>

* Add v4alpha proto

Signed-off-by: Nitin <nigoyal@microsoft.com>

* add generated v4alpha configs

Signed-off-by: Nitin <nigoyal@microsoft.com>

* fix test and build

Signed-off-by: Nitin <nigoyal@microsoft.com>

* fix config tests

Signed-off-by: Nitin <nigoyal@microsoft.com>

* Add tests

Signed-off-by: Nitin <nigoyal@microsoft.com>

* fix tests

Signed-off-by: Nitin <nigoyal@microsoft.com>

* fix format

Signed-off-by: Nitin <nigoyal@microsoft.com>

* Add bi-directional test

Signed-off-by: Nitin <nigoyal@microsoft.com>

* proto format

Signed-off-by: Nitin <nigoyal@microsoft.com>

* remove soft limit config

Signed-off-by: Nitin <nigoyal@microsoft.com>

* extension: add http bandwidth limit filter (#3)

Add bandwidth limit filter core functionality and UTs. The filter smoothens the flow of data in both direction up to the specified bandwidth limit.

Signed-off-by: Nitin <nigoyal@microsoft.com>

Signed-off-by: Nitin Goyal <nrk.goyal@gmail.com>

* make token bucket conditionally thread-safe and add comment for stream rate limiter

Signed-off-by: Nitin <nigoyal@microsoft.com>

* fix reset

Signed-off-by: Nitin <nigoyal@microsoft.com>

* remove some absl guards

Signed-off-by: Nitin <nigoyal@microsoft.com>

* update filter

Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>

* update proto

Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>

* add docs

Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>

* proto and doc fixes

Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>

* test fix

Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #16358 was opened by nitgoy.

see: more, trace.

@nitgoy nitgoy mentioned this pull request May 6, 2021
Closed
@nitgoy
docs and format fix
e2e9ff3 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@yanavlasov yanavlasov self-assigned this May 6, 2021
@nitgoy
Copy link
Contributor Author

nitgoy commented May 6, 2021 •
edited
Loading

Any idea about the format_pre and docs check failure? The tools/code_format/check_format.py fix and docs/build.sh commands are passing locally.

@nitgoy
Copy link
Contributor Author

nitgoy commented May 6, 2021

Actually found this warning with docs build command:
generated/rst/api-v3/extensions/filters/http/bandwidth_limit/v3alpha/bandwidth_limit.proto.rst: WARNING: document isn't included in any toctree
Not sure what am I missing since the file is auto-generated.

@adisuissa
Copy link
Contributor

Actually found this warning with docs build command:
generated/rst/api-v3/extensions/filters/http/bandwidth_limit/v3alpha/bandwidth_limit.proto.rst: WARNING: document isn't included in any toctree
Not sure what am I missing since the file is auto-generated.

Not an expert, but I think the idea is that the entire doc will be a tree (not a forest), and so everything can be reached from the main docs page.
Maybe something similar to envoy/docs/root/api-v3/config/accesslog/accesslog.rst will work in this case.

@nitgoy
try fix docs precheck error
42ad169 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
Copy link
Contributor Author

nitgoy commented May 6, 2021 •
edited
Loading

Actually found this warning with docs build command:
generated/rst/api-v3/extensions/filters/http/bandwidth_limit/v3alpha/bandwidth_limit.proto.rst: WARNING: document isn't included in any toctree
Not sure what am I missing since the file is auto-generated.

Not an expert, but I think the idea is that the entire doc will be a tree (not a forest), and so everything can be reached from the main docs page.
Maybe something similar to envoy/docs/root/api-v3/config/accesslog/accesslog.rst will work in this case.

I see. It didn't get picked up because http.rst only looks in the v3 folder but not v3alpha folder. Changed that one but not sure if it's acceptable. Please review.

@nitgoy
Copy link
Contributor Author

nitgoy commented May 6, 2021

@adisuissa The docs error got fixed. Any insight into the format_pre failure or how to reproduce it locally?

@adisuissa
Copy link
Contributor

@adisuissa The docs error got fixed. Any insight into the format_pre failure or how to reproduce it locally?

According to the pipelines errors:

mixed tabs and spaces: docs/root/configuration/http/http_filters/bandwidth_limit_filter.rst

To reproduce locally, you cant try running: ci/run_envoy_docker.sh 'ci/do_ci.sh format_pre'

nitgoy added 2 commits May 6, 2021 18:05
@nitgoy
fix format
bcb06b3 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
fix spelling
efc4d21 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
nitgoy added 4 commits May 7, 2021 17:00
@nitgoy
use duration type for fill_interval proto field
0e3ae50 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
revert token bucket changes
cab2a33 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
format fix
5059bc2 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
try format fix again
f4cb248 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy nitgoy requested a review from yanavlasov May 8, 2021 00:59
nitgoy added 3 commits May 9, 2021 18:32
@nitgoy
fix format pre
e038f7d 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
try fix clang tidy
7052748 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
fix build errors
d128ac8 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
Copy link
Contributor Author

nitgoy commented May 11, 2021

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #16358 (comment) was created by @nitgoy.

see: more, trace.

@nitgoy
Merge from upstream
e6d805b 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy
Copy link
Contributor Author

nitgoy commented May 13, 2021

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #16358 (comment) was created by @nitgoy.

see: more, trace.

@nitgoy
address comment
ce53ad6 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
phlax
phlax approved these changes May 21, 2021
View reviewed changes
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

lgtm from docs pov - thanks @nitgoy

yanavlasov
yanavlasov previously approved these changes May 24, 2021
View reviewed changes
phlax
phlax requested changes May 24, 2021
View reviewed changes
Copy link
Member

@phlax phlax left a comment

Choose a reason for hiding this comment

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

apologies for last minute nits @nitgoy - i would really appreciate if you could fix the literal - it looks wrong with italics

see here:

https://storage.googleapis.com/envoy-pr/ce53ad6/docs/configuration/http/http_filters/bandwidth_limit_filter.html?highlight=bandwidth_limit#statistics

docs/root/configuration/http/http_filters/bandwidth_limit_filter.rst Outdated Show resolved Hide resolved
@nitgoy
address doc comments
f7e39d9 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
yanavlasov
yanavlasov previously approved these changes May 24, 2021
View reviewed changes
@nitgoy
rename proto enum values
4fc4837 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy nitgoy requested review from phlax, lizan and yanavlasov May 24, 2021 20:32
@nitgoy
Copy link
Contributor Author

nitgoy commented May 24, 2021

/retest

@repokitteh-read-only
Copy link

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

🐱

Caused by: a #16358 (comment) was created by @nitgoy.

see: more, trace.

yanavlasov
yanavlasov previously approved these changes May 25, 2021
View reviewed changes
@nitgoy
make getConfig return ref
54302e9 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
phlax
phlax previously approved these changes May 25, 2021
View reviewed changes
Copy link
Member

@phlax phlax 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.

lgtm for docs, thanks for cleanups @nitgoy

@nitgoy
rename enum and stats values
2a70c24 
Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
@nitgoy nitgoy requested a review from lizan May 25, 2021 16:12
@lizan lizan merged commit a467b0e into envoyproxy:main May 25, 2021
@nitgoy nitgoy deleted the bwlfilter branch June 3, 2021 20:20
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
@nitgoy
extensions: add http bandwidth limit filter (envoyproxy#16358)
0077d04 
the filter smoothens the flow of data in both direction up to the specified bandwidth limit.

Risk Level: Low, new filter
Testing: UTs added. Adding more UTs and integration tests.
Docs Changes: Added
Release Notes: Added
Fixes envoyproxy#13604

Signed-off-by: Nitin Goyal <nigoyal@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yanavlasov yanavlasov yanavlasov approved these changes

@lizan lizan lizan approved these changes

@phlax phlax phlax left review comments

Assignees

@yanavlasov yanavlasov

@markdroth markdroth

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

how to config bandwidth limit
6 participants
@nitgoy @adisuissa @yanavlasov @phlax @lizan @markdroth