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.

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

feat(translator): implement ratelimit costs #5035

Merged
merged 12 commits into from
Jan 15, 2025
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixes comments
Signed-off-by: Takeshi Yoneda <t.y.mathetake@gmail.com>
mathetake authored and zirain committed Jan 15, 2025
commit a0bb337c600b5af77d72a1995f787d26ab0f48b6
18 changes: 15 additions & 3 deletions internal/xds/translator/ratelimit.go
Original file line number Diff line number Diff line change
@@ -138,15 +138,27 @@ func patchRouteWithRateLimit(route *routev3.Route, irRoute *ir.HTTPRoute) error
global := irRoute.Traffic.RateLimit.Global
rateLimits, costSpecified := buildRouteRateLimits(irRoute.Name, global)
if costSpecified {
// PerRoute global rate limit configuration via typed_per_filter_config can its own rate routev3.RateLimit that overrides the route level rate limits.
// Per-descriptor level hits_addend can only be configured there: https://github.com/envoyproxy/envoy/pull/37972
// vs the "legacy" core route-embedded rate limits doesn't have this feature.
//
// This branch is only reached when the response cost is specified which allows us to assume that
// users are using Envoy >= v1.33.0 which also supports the typed_per_filter_config.
//
// https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/ratelimit/v3/rate_limit.proto#extensions-filters-http-ratelimit-v3-ratelimitperroute
//
// Though this is not explicitly documented, the rate limit functionality is the same as the core route-embedded rate limits.
// So this is a safe assumption. Only code path different is at
// https://github.com/envoyproxy/envoy/blob/47f99c5aacdb582606a48c85c6c54904fd439179/source/extensions/filters/http/ratelimit/ratelimit.cc#L93-L114
// which is identical for both core and typed_per_filter_config as we are not using virtual_host level rate limits.
return patchRouteWithRateLimitOnTypedFilterConfig(route, rateLimits)
}
xdsRouteAction.RateLimits = rateLimits
return nil
}

// 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.
// the TypedPerFilterConfig field.
func patchRouteWithRateLimitOnTypedFilterConfig(route *routev3.Route, rateLimits []*routev3.RateLimit) error { //nolint:unparam
Copy link
Contributor

Choose a reason for hiding this comment

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

cc @zhaohuabing this introduces another design pattern to do per route filter config

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah i was too lazy to do the right abstraction 😉

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

@zhaohuabing zhaohuabing Jan 14, 2025

Choose a reason for hiding this comment

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

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.

filterCfg := route.TypedPerFilterConfig
if filterCfg == nil {
@@ -157,7 +169,7 @@ func patchRouteWithRateLimitOnTypedFilterConfig(route *routev3.Route, rateLimits
// This should not happen since this is the only place where the filter
// config is added in a route.
return fmt.Errorf(
"route already contains local rate limit filter config: %s", route.Name)
"route already contains global rate limit filter config: %s", route.Name)
}

g, err := anypb.New(&ratelimitfilterv3.RateLimitPerRoute{RateLimits: rateLimits})