feat(translator): implement ratelimit costs #5035
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5035 +/- ##
==========================================
+ Coverage 66.81% 66.85% +0.03%
==========================================
Files 211 211
Lines 32854 32928 +74
==========================================
+ Hits 21952 22014 +62
- Misses 9579 9587 +8
- Partials 1323 1327 +4 ☔ View full report in Codecov by Sentry. |
|
I think we have e2e for costPerReq, able to add one for costPreResponse? |
|
can the e2e use the latest envoyproxy/ratelimit? |
IIRC, master branch always use the latest one. |
|
ok cool |
|
that was fast 😅 now that EG supports writing metadata via ext proc #5023 would be great if we can have an e2e where the ext proc inserts a fixed metadata that is used by the ratelimit API |
mathetake
force-pushed
the
translation
branch
2 times, most recently
from
543c44e to
ad5bf27
Compare
looks like applyOnStreamDone is working fine but the per-descriptor is not working (both request and response costing 1 regardless of config) |
|
from envoyproxy/envoy#37684:
maybe this is something to do with it |
|
ok @arkodg looks like @wbpcode introduced the new and only the new one currently supports per-descriptor hits_addend I think we have to either add support per-descriptor hits_addend to the legacy route/virtual_host config or migrate EG to use the new |
|
i will take a look at the envoy code monday to see how hard to support them in the legacy config |
|
I think it only requires to do some small refactoring... |
|
envoyproxy/envoy#37972 this should fix the test |
|
the required change in envoy seems a bit trickier than i thought... (the change itself is small but the build constraints by Envoy mobile makes it impossible as-is which in turn requires quite a refactoring around formatter...) |
|
@mathetake can we copy the user defined metadata into the ratelimit hits addend metadata field using a filter instead, in a per route filter way ? |
|
not sure what you meant but one workaround here is to use typed_per_filter_config RateLimitConfig instead of route-embedded-legacy rate limit only when costs are configured (which allows us to assume its latest Envoy) and i think that should work. Having said that though, I found the workaround in the envoyside and waiting for @wbpcode to review |
|
ok tomorrow i will work on the workaround mentioned ^^ |
|
ok passing now!!!!! |
| @@ -662,6 +669,68 @@ var RateLimitHeadersAndCIDRMatchTest = suite.ConformanceTest{ | |||
| }, | |||
| } | |||
|
|
|||
| var UsageRateLimitTest = suite.ConformanceTest{ | |||
There was a problem hiding this comment.
this is almost identical with the usecase with ai-gateway - except that the extproc sets the dynamic metadata on responseHeaders hook vs ai-gateway in responseBody hook. Either should work from Envoy pov as the response cost is applied when stream is closing
|
come on CI queue |
| // patchRouteWithRateLimitOnTypedFilterConfig builds rate limit actions and appends to the route via | ||
| // the TypedPerFilterConfig field. This only happens when the response cost is specified which allows us to assume that | ||
| // users are using Envoy >= v1.33.0. | ||
| func patchRouteWithRateLimitOnTypedFilterConfig(route *routev3.Route, rateLimits []*routev3.RateLimit) error { //nolint:unparam |
There was a problem hiding this comment.
cc @zhaohuabing this introduces another design pattern to do per route filter config
There was a problem hiding this comment.
yeah i was too lazy to do the right abstraction 😉
There was a problem hiding this comment.
so anyways when we can set the floor Envoy version to v1.33, then we should be able to migrate to this typed_per_filter_config global rate limit unconditionally since that's the latest way (having support for per-descriptor-hits-addend) vs the current route-embedded config is legacy one
There was a problem hiding this comment.
Yeah, typed_per_filter_config is the way to go - it aligns with the approach used by all other filters for per-route configurations.
We can address htis in a seperate PR later.
|
hey @mathetake you'll also need to run |
|
hmmm doesn't make any diff |
|
@arkodg passing all tests modulo unrelated flake so can we merge? |
|
Look good. Fixed the conflicts then we can merge it. |
mathetake
force-pushed
the
translation
branch
from
991feed to
e1bf3da
Compare
|
/retest |
...rnal/gatewayapi/testdata/backendtrafficpolicy-with-ratelimit-invalid-distinct-invert.in.yaml
Outdated
Show resolved
Hide resolved
|
come on |
|
/retest |
|
man the tests are too flaky 🤷 nothing to do with this PR |
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
Signed-off-by: Takeshi Yoneda <[email protected]>
What type of PR is this?
The API implementation
What this PR does / why we need it:
This is the follow up on #4957 and implement the API.
Which issue(s) this PR fixes:
Fixes #4756
Release Notes: Yes