-
Notifications
You must be signed in to change notification settings - Fork 507
feat: Implement requestBuffer #5661
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.
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5661 +/- ##
==========================================
+ Coverage 65.19% 65.34% +0.14%
==========================================
Files 214 220 +6
Lines 34321 35092 +771
==========================================
+ Hits 22377 22932 +555
- Misses 10591 10749 +158
- Partials 1353 1411 +58 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
63c5792
to
4f7ffef
Compare
6affbbe
to
d6add20
Compare
@markwinter Thanks so much for working on this great feature — really appreciate the effort you’re putting in!
This behavior is different than other types of settings in the BackendTrafficPolicy, where the settings targeting Gateway apply to all xRoutes. This inconsistent behavior of BackendTrafficPolicy could confuse users. We also can't apply a default RequestBuffer for all the xRoutes under a Gateway. Another small thing: when the Buffer filter is applied at the Gateway level, it’s configured on all Listeners. While this makes sense (since BackendTrafficPolicy doesn’t target Listeners directly), it might be a bit unexpected from a user perspective. I prefer keeping the same "Override" behavior as the other settings, and adding a Shared global rate limit: gateway/api/v1alpha1/ratelimit_types.go Lines 55 to 62 in 61474e3
cc @envoyproxy/gateway-maintainers |
hey @markwinter as @zhaohuabing suggested, can the implementation push all the intent to per Route, and we can enhance this in the future with a |
@arkodg @zhaohuabing Thanks for the feedback. On holiday for a week and will return to the PR afterwards 👍 |
hey @markwinter hope you had a good break, feature freeze for v1.4 is April 30th, curious if you youd be able to complete this by then or prefer moving this to v1.5 ? |
d6add20
to
d3ac72e
Compare
Signed-off-by: mark winter <[email protected]>
d3ac72e
to
2797b88
Compare
@arkodg @zhaohuabing Sorry for the little bit of delay in getting back. I've updated the request buffer xds patching. Now the patchHCM will add the Buffer filter but in a If the BTP target was a Gateway all routes get the BufferPerRoute filter applied, and if the BTP target was just a Route, it gets added for just that route. I took inspiration from how BasicAuth filter works which also adds a HCM filter in Disabled state, but then overrides the per route config. |
Signed-off-by: mark winter <[email protected]>
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks for adding this ! this will be part of v1.4 😅
What this PR does / why we need it:
This implements the new requestBuffer API using the Envoy's Buffer and BufferPerRoute filters.
API PR: #5537
Which issue(s) this PR fixes:
Fixes #3706
Release Notes: Yes