chore(*) introduce rate limiter #2083

nickolaev · 2021-06-02T17:13:28Z

Summary

Adding the RateLimit policy as per #2039

only HTTP rate limites
no support for ExternalServices

Documentation

Link to the website documentation PR
GUI chore(*) introduce RateLimit kuma-gui#162

Testing

Unit tests
E2E tests
Manual testing on Universal
Manual testing on Kubernetes

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

…e_limiter

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

api/mesh/v1alpha1/rate_limit.proto

pkg/core/ratelimits/matcher.go

jakubdyszkiewicz · 2021-06-07T07:59:35Z

pkg/core/resources/apis/mesh/rate_limit_validator_test.go

+)
+
+var _ = Describe("RateLimit", func() {
+	Describe("Validate()", func() {


I'd like to have examples of both "positive" and "negative" test cases

jakubdyszkiewicz · 2021-06-07T08:24:45Z

test/e2e/ratelimit/ratelimit_universal.go

+         value: "true"
+`
+
+	BeforeEach(func() {


Please write this test with BeforeSuite / AfterSuite. We already learned that writing it this way is a huge win for test performance and writing new test cases.

test/e2e/ratelimit/ratelimit_universal.go

jakubdyszkiewicz · 2021-06-07T08:42:21Z

test/e2e/ratelimit/ratelimit_universal.go

+    kuma.io/service: "demo-client"
+destinations:
+- match:
+    kuma.io/service: echo-server_kuma-test_svc_8080


would be great to test many tags in sources in this test or another

jakubdyszkiewicz · 2021-06-07T08:53:23Z

pkg/xds/envoy/listeners/v3/http_inbound_route_configurer.go

-	Route   envoy_common.Route
+	Service   string
+	Route     envoy_common.Route
+	RateLimit *mesh_proto.RateLimit


Ok, I feel like here we are doing mix of a lot of things. The whole point of having configureres is a separation of concerns while building Envoy config. Here what I would do

envoy_common.Route has field RateLimit *mesh_proto.RateLimit. It is "higher level" object, IMHO TypedPerFilterConfig is just too low level at this point

HttpInboundRouteConfigurer takes Routes, not Route and it does not take RateLimit as an arguments

When calling HttpInboundRoute (refactor to HttpInboundRoutes), first - build a Routes based on all the policies (in our case it's only RateLimit so something like func BuildInboundRoutes(rateLimit) Routes).

Create a RateLimitConfigurer that configures envoy_hcm.HttpConnectionManager with envoy.filters.http.local_ratelimit

In RoutesConfigurer check if Route has RateLimit, if so, then build a config for this route.
Here I'd consider refactoring RoutesConfigurer into Configurer pattern so you can configure individual elements on the Route, but we can skip it for now and do it if we were ever to configure more stuff.

The result of this is:

HttpInboundRoute has 0 knowledge about RateLimit - separation of concerns

You now have RateLimitConfigurer and an easy way to add RateLimit on Route so you can much easier reuse this on the outbound side

jakubdyszkiewicz · 2021-06-07T08:55:17Z

test/e2e/ratelimit/ratelimit_universal.go

+		Eventually(func() bool {
+			_, _, err := cluster.Exec("", "", "demo-client", "curl", "-v", "--fail", "echo-server_kuma-test_svc_8080.mesh")
+			return err == nil
+		}, "30s", "3s").Should(BeTrue())


You cannot assume that this policy is automatically applied in 0ms latency. I think this test will be flaky.

That is the point of Eventually, to make sure that in 30s we'll get at least one pass.

I might misread the code, but I think the flow right now is like this with the second It()

we apply rateLimitPolicy in BeforeSuite

You apply new policy

We now have this eventually block that immediately will pass regardless if new policy is propagated to envoys or not

You go ahead and do the X requests. It might be the case that those requests are still using the old policy

…e_limiter

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

…e_limiter # Conflicts: # pkg/xds/envoy/route.go # pkg/xds/envoy/routes/v2/routes_configurer.go # pkg/xds/envoy/routes/v3/routes_configurer.go Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

…e_limiter

…e_limiter # Conflicts: # pkg/xds/envoy/listeners/configurers.go # pkg/xds/envoy/listeners/v2/http_inbound_routes_configurer.go # pkg/xds/envoy/routes/v2/routes_configurer.go Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

…e_limiter

tharun208 · 2021-06-08T11:30:59Z

@nickolaev also can you update the tools/docs/kumactl/gen_help.sh ?

jakubdyszkiewicz · 2021-06-08T11:37:03Z

pkg/xds/envoy/listeners/v3/rate_limit_configurer_test.go

+                - name: envoy.filters.http.router
+                statPrefix: stats`,
+		}),
+		Entry("2 policy selectors", testCase{


I think this test case is no longer valuable.

jakubdyszkiewicz · 2021-06-08T11:45:28Z

pkg/xds/envoy/routes/v3/routes_configurer.go

+			MaxTokens: rlHttp.GetRequests().GetValue(),
+			TokensPerFill: &wrappers.UInt32Value{
+				Value: 1,
+			},


I don't think this is correct. Let's say you have limit of 10 requests per 5 second.
You first have 10 tokens. Then you send 10 requests in 1s and you have 0 tokens. Then you fill 1 token per 5 seconds allowing 1req/5s.
Additionaly, I think E2E does not catch this because we are only using initial max tokens.

The Value should be GetRequests().GetValue.

I might be wrong though, let me know.

jakubdyszkiewicz · 2021-06-08T11:47:44Z

pkg/xds/envoy/routes/v3/routes_configurer.go

+	return typedPerFilterConfig, nil
+}
+
+func (c *RoutesConfigurer) createRateLimit(route *envoy_common.Route) (*any.Any, error) {


optional nit: you don't need the whole route here, I'd pass *RateLimit_Conf_Http.
It helps me to read the code if the scope is limited, here I had to double-check what else you are using from the Route.

pkg/xds/generator/inbound_proxy_generator.go

jakubdyszkiewicz · 2021-06-08T12:00:01Z

test/e2e/ratelimit/ratelimit_universal.go

+				}
+			}
+			return true
+		}, "60s", "5s").Should(BeTrue())


Let's check if rate limiting is consistently applied

requestsRateLimitted := func() bool... Eventually(requestsRateLimitted, "60s", "6s").Should(BeTrue()) time.Wait(6s) Expect(requestsRateLimitted).To(BeTrue())

…e_limiter

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

…e_limiter # Conflicts: # pkg/xds/sync/components.go Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

…e_limiter

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

…e_limiter

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

jakubdyszkiewicz · 2021-06-10T11:30:06Z

pkg/core/policy/connection_matcher.go


-	return false
+	return tagSelectorI.Rank().CompareTo(tagSelectorJ.Rank()) > 0


One more thing here. If ranks are equal, we need to compare GetCreationTime()

jakubdyszkiewicz · 2021-06-10T11:30:50Z

pkg/core/ratelimits/matcher.go

@@ -54,3 +56,21 @@ func BuildRateLimitMap(
 	}
 	return result, nil
 }
+
+func splitPoliciesBySourceMatch(rateLimits []*mesh_core.RateLimitResource) []*mesh_core.RateLimitResource {


would be nice to have a comment why we are doing this

api/mesh/v1alpha1/dataplane_helpers.go

jakubdyszkiewicz · 2021-06-10T11:32:46Z

pkg/xds/generator/inbound_proxy_generator.go

+	for _, rateLimit := range rateLimits {
+		if rateLimit.GetConf().GetHttp() != nil {
+			route := envoy_common.NewRouteFromCluster(cluster)
+			// Source


// source?

I'd leave a comment somewhere around this function that we rely on the fact that rateLimits are in order from most specific to most general

jakubdyszkiewicz · 2021-06-10T11:34:06Z

test/e2e/ratelimit/ratelimit_universal.go

+
+	It("should limit to 2 requests per 5 sec", func() {
+		Eventually(verifyRateLimit("demo-client", 5), "60s", "5s").Should(Equal(2))
+		Eventually(verifyRateLimit("demo-client", 5), "30s", "1s").Should(Equal(2))


Is there a reason for intervals to be different?

(also leave a comment that we check a second time to see if the limit is deterministic)

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

test/e2e/ratelimit/ratelimit_universal.go

pkg/core/ratelimits/matcher.go

pkg/core/policy/connection_matcher.go

jakubdyszkiewicz · 2021-06-10T12:37:45Z

test/e2e/ratelimit/ratelimit_universal.go

+	})
+
+	E2EAfterSuite(func() {
+		if ShouldSkipCleanup() {


if ShouldSkipCleanup() { is in E2EAfterSuite no need to do that again

jakubdyszkiewicz · 2021-06-10T12:49:13Z

test/e2e/ratelimit/ratelimit_universal.go

+		Eventually(verifyRateLimit("web", 5), "30s", "1s").Should(Equal(2))
+	})
+
+	It("should limit multiple source", func() {


are It() executed in the order defined in the code? I hope so, otherwise will fail. In TrafficPermission and TrafficRoute test we clean resources in AfterEach

I see, well form my experience they are run in the order they appear.

api/mesh/v1alpha1/rate_limit.proto

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev mentioned this pull request Jun 3, 2021

Rate Limit proposal #2039

Merged

nickolaev changed the title ~~Chore/introduce rate limiter~~ chore(*) introduce rate limiter Jun 3, 2021

Nikolay Nikolaev added 5 commits June 4, 2021 16:59

chore(*) add RateLimit resource

d89b707

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

chore(*) add RateLimit handling in xds

b70465d

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

fix(*) make check

34c0cf4

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

test(e2e) RateLimit

d543b94

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

chore(*) remove TCP from RateLimit

e0bc922

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev force-pushed the chore/introduce_rate_limiter branch from 6476c26 to e0bc922 Compare June 4, 2021 13:59

fix(*) api s plural

3a5fae9

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev marked this pull request as ready for review June 7, 2021 07:07

nickolaev requested a review from a team as a code owner June 7, 2021 07:07

Nikolay Nikolaev added 2 commits June 7, 2021 10:19

Merge remote-tracking branch 'origin/master' into chore/introduce_rat…

6edc2c5

…e_limiter

test(e2e) RateLimit stable testing

9df940e

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

jakubdyszkiewicz requested changes Jun 7, 2021

View reviewed changes

jakubdyszkiewicz reviewed Jun 7, 2021

View reviewed changes

Nikolay Nikolaev added 10 commits June 7, 2021 12:39

Merge remote-tracking branch 'origin/master' into chore/introduce_rat…

cc6e822

…e_limiter

fix(*) review

5536023

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

chore(*) introduce RateLimitConfigurer

feed6bc

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

chore(*) RoutesConfigurer to handle RateLimit

07a785a

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Merge remote-tracking branch 'origin/master' into chore/introduce_rat…

4438172

…e_limiter # Conflicts: # pkg/xds/envoy/route.go # pkg/xds/envoy/routes/v2/routes_configurer.go # pkg/xds/envoy/routes/v3/routes_configurer.go Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

chore(*) move Routes geneation out of HttpInboundRoutesConfigurer

91d2455

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Merge remote-tracking branch 'origin/master' into chore/introduce_rat…

6e34af8

…e_limiter

test(e2e) RateLimit tests improvements/review

3f3a385

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Merge remote-tracking branch 'origin/master' into chore/introduce_rat…

b95d64d

…e_limiter

jakubdyszkiewicz requested changes Jun 8, 2021

View reviewed changes

Nikolay Nikolaev added 3 commits June 8, 2021 15:54

Merge remote-tracking branch 'origin/master' into chore/introduce_rat…

97f43f9

…e_limiter

chore(*) gen help for RateLimit

e8fd5c8

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

chore(*) RateLimit validator

0b1ad45

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

fix(*) review

3c5e2bb

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev force-pushed the chore/introduce_rate_limiter branch from 2971628 to 3c5e2bb Compare June 9, 2021 10:04

Nikolay Nikolaev added 9 commits June 9, 2021 17:51

chore(kuma-cp) support Multiple rate limits for a destination

b5a4ff4

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Merge remote-tracking branch 'origin/master' into chore/introduce_rat…

9a1e9c3

…e_limiter # Conflicts: # pkg/xds/sync/components.go Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

test(e2e) now DemoClientUniversal sets service name

132cd1d

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Merge remote-tracking branch 'origin/master' into chore/introduce_rat…

1610168

…e_limiter

fix(*) integration test

7b40e70

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

test(e2e) RateLimit e2e fix flakiness

4cb20d5

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

chore(*) RateLimit order policies

b97a2fc

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

Merge remote-tracking branch 'origin/master' into chore/introduce_rat…

6472b49

…e_limiter

test(*) RateLimit matcher with more sources

88c20c4

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

jakubdyszkiewicz reviewed Jun 10, 2021

View reviewed changes

api/mesh/v1alpha1/dataplane_helpers.go Outdated Show resolved Hide resolved

jakubdyszkiewicz reviewed Jun 10, 2021

View reviewed changes

fix(*) review

1f779ef

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

jakubdyszkiewicz approved these changes Jun 10, 2021

View reviewed changes

fix(*) review

8a0bf20

Signed-off-by: Nikolay Nikolaev <nikolay.nikolaev@konghq.com>

nickolaev merged commit 31f79e0 into master Jun 10, 2021

nickolaev deleted the chore/introduce_rate_limiter branch June 10, 2021 14:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(*) introduce rate limiter #2083

chore(*) introduce rate limiter #2083

nickolaev commented Jun 2, 2021 •

edited

jakubdyszkiewicz Jun 7, 2021

jakubdyszkiewicz Jun 7, 2021

jakubdyszkiewicz Jun 7, 2021

jakubdyszkiewicz Jun 7, 2021

jakubdyszkiewicz Jun 7, 2021

nickolaev Jun 7, 2021

jakubdyszkiewicz Jun 7, 2021

tharun208 commented Jun 8, 2021 •

edited

jakubdyszkiewicz Jun 8, 2021

jakubdyszkiewicz Jun 8, 2021

jakubdyszkiewicz Jun 8, 2021

jakubdyszkiewicz Jun 8, 2021

jakubdyszkiewicz Jun 10, 2021

jakubdyszkiewicz Jun 10, 2021

jakubdyszkiewicz Jun 10, 2021

jakubdyszkiewicz Jun 10, 2021

jakubdyszkiewicz Jun 10, 2021

jakubdyszkiewicz Jun 10, 2021

jakubdyszkiewicz Jun 10, 2021

jakubdyszkiewicz Jun 10, 2021

nickolaev Jun 10, 2021


		return false
		return tagSelectorI.Rank().CompareTo(tagSelectorJ.Rank()) > 0

chore(*) introduce rate limiter #2083

chore(*) introduce rate limiter #2083

Conversation

nickolaev commented Jun 2, 2021 • edited

Summary

Documentation

Testing

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tharun208 commented Jun 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickolaev commented Jun 2, 2021 •

edited

tharun208 commented Jun 8, 2021 •

edited