-
Notifications
You must be signed in to change notification settings - Fork 347
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
Global RateLimit Xds translation #726
Conversation
b8ff9c2
to
f7288b8
Compare
Codecov Report
@@ Coverage Diff @@
## main #726 +/- ##
==========================================
+ Coverage 63.93% 63.97% +0.03%
==========================================
Files 51 52 +1
Lines 6863 7136 +273
==========================================
+ Hits 4388 4565 +177
- Misses 2201 2286 +85
- Partials 274 285 +11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
f7288b8
to
2af765e
Compare
@@ -211,6 +212,9 @@ type HTTPRoute struct { | |||
Destinations []*RouteDestination | |||
// Rewrite to be changed for this route. | |||
URLRewrite *URLRewrite | |||
// RateLimit defines the more specific match conditions as well as limits for ratelimiting | |||
// the requests on this route. | |||
RateLimit *RateLimit |
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.
@arkodg I don't understand why RateLimit doesn't use any of the RateLimitFilter fields. For example, #813 uses JwtAuthenticationFilterProvider instead of recreating the type in the IR. Since we own extension APIs, e.g. RateLimitFilter, we should leverage these types as much as possible.
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.
creating a decoupling from Front End as well as Back End API, that was the main goal of the IR, which is why we created custom structures even for known structures such as StringMatch
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.
as you mentioned, since the project owns the structure, should be safe to reuse it, but the structure fields are also arranged differently to facilitate better translation (more machine friendly than user friendly)
@arkodg regarding #726 (comment), thanks for the suggestion. #813 focuses solely on the xds IR changes. I plan to submit a follow-on PR that adds AuthenticationFilter support to the xds translator. I'll look at following the pattern here for the follow-on PR. |
* Enhance `XdsIR` with `RateLimit` to hold rate limiting config. * Translate IR field into route level rate limit actions * Add `BuildRateLimitServiceConfig` which translates the XdsIR into configuration for the envoy rate limit service. Relates to envoyproxy#670 Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Signed-off-by: Arko Dasgupta <arko@tetrate.io>
Enhance
XdsIR
withRateLimit
to hold rate limiting config.Translate IR field into route level rate limit actions
This PR specifically implements this points
stated in #858
Relates to #670
Signed-off-by: Arko Dasgupta arko@tetrate.io