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

Global RateLimit Xds translation #726

Merged
merged 10 commits into from
Jan 9, 2023
Merged

Conversation

arkodg
Copy link
Contributor

@arkodg arkodg commented Nov 11, 2022

  • Enhance XdsIR with RateLimit to hold rate limiting config.

  • Translate IR field into route level rate limit actions

This PR specifically implements this points

 * The xDS IR will be enhanced to hold the user facing rate limit intent.
 * The xDS Translator will be enhanced to translate the rate limit field within the xDS IR into Rate limit [Actions][] as well as instantiate the [rate limit filter][].

stated in #858

Relates to #670

Signed-off-by: Arko Dasgupta [email protected]

@arkodg arkodg requested a review from a team as a code owner November 11, 2022 22:10
@arkodg arkodg marked this pull request as draft November 11, 2022 22:10
@arkodg arkodg force-pushed the ratelimit-xds branch 2 times, most recently from b8ff9c2 to f7288b8 Compare November 15, 2022 02:31
@codecov-commenter
Copy link

codecov-commenter commented Nov 15, 2022

Codecov Report

Merging #726 (8a097f3) into main (a0be2d8) will increase coverage by 0.03%.
The diff coverage is 65.54%.

@@            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     
Impacted Files Coverage Δ
internal/ir/xds.go 78.06% <0.00%> (-1.48%) ⬇️
internal/ir/zz_generated.deepcopy.go 14.84% <0.00%> (-2.47%) ⬇️
internal/xds/translator/route.go 84.78% <0.00%> (-1.13%) ⬇️
internal/xds/translator/listener.go 81.16% <25.00%> (-0.74%) ⬇️
internal/xds/translator/translator.go 74.12% <71.42%> (-0.47%) ⬇️
internal/xds/translator/ratelimit.go 96.36% <96.36%> (ø)
internal/provider/kubernetes/helpers.go 73.77% <0.00%> (-4.10%) ⬇️
internal/provider/kubernetes/controller.go 47.03% <0.00%> (ø)
internal/gatewayapi/validate.go 90.86% <0.00%> (+0.34%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@arkodg arkodg marked this pull request as ready for review November 17, 2022 03:50
@arkodg arkodg marked this pull request as draft December 8, 2022 17:35
@arkodg arkodg mentioned this pull request Dec 16, 2022
@arkodg
Copy link
Contributor Author

arkodg commented Jan 4, 2023

this PR tries to keep all API Gateway functionality in a separate file, to simplify the core xds translation logic using the pattern of patching xds resources, might be useful to peruse in #813 @danehans

@arkodg arkodg marked this pull request as ready for review January 4, 2023 19:47
@arkodg arkodg requested a review from zirain January 5, 2023 19:43
@@ -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
Copy link
Contributor

@danehans danehans Jan 5, 2023

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@arkodg arkodg Jan 5, 2023

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)

@danehans
Copy link
Contributor

danehans commented Jan 5, 2023

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

@arkodg arkodg requested a review from danehans January 6, 2023 20:28
* 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 <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
Signed-off-by: Arko Dasgupta <[email protected]>
@arkodg arkodg merged commit 4fb3853 into envoyproxy:main Jan 9, 2023
@arkodg arkodg deleted the ratelimit-xds branch January 9, 2023 17:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants