-
Notifications
You must be signed in to change notification settings - Fork 677
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
Add support for General RateLimit Policy #5363
Add support for General RateLimit Policy #5363
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5363 +/- ##
==========================================
+ Coverage 78.46% 78.53% +0.07%
==========================================
Files 138 138
Lines 18896 18925 +29
==========================================
+ Hits 14826 14863 +37
+ Misses 3789 3783 -6
+ Partials 281 279 -2
|
2202254
to
9275b4d
Compare
Hey @sunjayBhatia @skriss and @tsaarni |
6a8e2e6
to
5a199a5
Compare
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.
some comments around config generation
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
461ca91
to
0a37807
Compare
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
0a37807
to
0b03e57
Compare
@skriss and @sunjayBhatia |
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.
we should also add a test to contour/internal/featuretests/v3/globalratelimit_test.go
examples/ratelimit/04-default-global-ratelimit-contour-config.yaml
Outdated
Show resolved
Hide resolved
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
Hey @sunjayBhatia |
…policy Signed-off-by: shadi-altarsha <[email protected]>
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.
Thanks for the PR @shadialtarsha, still making my way through it but a couple small comments so far.
…policy Signed-off-by: shadi-altarsha <[email protected]>
Signed-off-by: shadi-altarsha <[email protected]>
}, | ||
Domain: "contour", | ||
DefaultGlobalRateLimitPolicy: &contour_api_v1.GlobalRateLimitPolicy{ | ||
Descriptors: []contour_api_v1.RateLimitDescriptor{ |
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.
a drawback of adding to this test this way is we have to set "Disabled" on the HTTPProxies in all the cases where we don't expect a rate limit to be set, not a huge thing but makes the test coverage a little different
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.
Yes, unfortunately, the way these tests are written doesn't allow multiple global rate limit services to be configured easily. All HTTPProxy uses the same RLS and that is why the opted-in by default.
Signed-off-by: shadi-altarsha <[email protected]>
@@ -1138,6 +1140,7 @@ func (s *Server) getDAGBuilder(dbc dagBuilderConfig) *dag.Builder { | |||
ConnectTimeout: dbc.connectTimeout, | |||
GlobalExternalAuthorization: dbc.globalExternalAuthorizationService, | |||
MaxRequestsPerConnection: dbc.maxRequestsPerConnection, | |||
GlobalRateLimitService: dbc.globalRateLimitService, |
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.
xref #5458 this is another instance where we should probably apply this to at least the Ingress processor as well. I'm OK logging a follow-up issue since we don't currently support rate limiting in any way on Ingress.
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.
Will do that, thanks for pointing this out!
Signed-off-by: shadi-altarsha <[email protected]>
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 @shadialtarsha!
// Make requests against the proxy, confirm a 429 response | ||
// is now gotten since we've exceeded the rate limit. | ||
res, ok := f.HTTP.RequestUntil(&e2e.HTTPRequestOpts{ | ||
Host: p.Spec.VirtualHost.Fqdn, | ||
Condition: e2e.HasStatusCode(429), | ||
}) | ||
require.NotNil(t, res, "request never succeeded") | ||
require.Truef(t, ok, "expected 429 response code, got %d", res.StatusCode) |
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.
It's tough to get perfect validation for these tests, but I think in combo with reviewing the code + the unit test coverage, this is sufficient for an E2E.
PR adds support for a global rate limit policy that can be defined in the RateLimit service configuration.
Proposal: #5359
Fixes: #5357
Signed-off-by: Shadi Altarsha [email protected]