add global rate limiting support #3298

skriss · 2021-02-01T22:30:38Z

Adds support for configuring an external Rate Limit
Service (RLS) that is used for making global rate
limit decisions. Adds support to HTTPProxy for
global rate limit policies, that generate descriptors
for requests to be sent to the RLS for rate-limit
decisions.

Closes #370.

Signed-off-by: Steve Kriss krisss@vmware.com

codecov · 2021-02-01T22:34:47Z

Codecov Report

Merging #3298 (8fd4f79) into main (d31d2b6) will increase coverage by 0.22%.
The diff coverage is 89.01%.

@@            Coverage Diff             @@
##             main    #3298      +/-   ##
==========================================
+ Coverage   75.54%   75.77%   +0.22%     
==========================================
  Files          98       98              
  Lines        6417     6535     +118     
==========================================
+ Hits         4848     4952     +104     
- Misses       1460     1475      +15     
+ Partials      109      108       -1

Impacted Files	Coverage Δ
cmd/contour/serve.go	`0.00% <0.00%> (ø)`
internal/dag/dag.go	`96.80% <ø> (ø)`
internal/dag/extension_processor.go	`94.52% <100.00%> (ø)`
internal/dag/httpproxy_processor.go	`91.56% <100.00%> (ø)`
internal/dag/policy.go	`96.98% <100.00%> (+0.47%)`	⬆️
internal/envoy/v3/ratelimit.go	`100.00% <100.00%> (+8.33%)`	⬆️
internal/envoy/v3/route.go	`98.92% <100.00%> (+0.01%)`	⬆️
internal/featuretests/v3/envoy.go	`100.00% <100.00%> (ø)`
internal/xdscache/v3/listener.go	`95.58% <100.00%> (+0.18%)`	⬆️
internal/xdscache/v3/route.go	`84.32% <100.00%> (+0.23%)`	⬆️
... and 2 more

skriss · 2021-02-04T23:10:16Z

@stevesloka @youngnick @sunjayBhatia I'd appreciate an initial review on this. I am still working on documentation.

skriss · 2021-02-09T20:58:12Z

Ran through a full test of the guide, fixed a couple small things.

sunjayBhatia

Looks good overall! Lots of changes but you're right mostly generated etc.

Just a couple small comments

site/docs/main/config/api-reference.html

sunjayBhatia · 2021-02-09T21:01:32Z

pkg/config/parameters.go

+	Domain string `yaml:"domain,omitempty"`
+
+	// FailOpen defines whether to allow requests to proceed when the
+	// Rate Limit Service fails to respond properly.


nit: Maybe define properly? (timeout etc.)

👍 also reminds me I forgot to add to site/docs/main/configuration.md and the example config file, will do that as well.

sunjayBhatia · 2021-02-09T22:48:30Z

internal/xdscache/v3/listener.go

+				AllowChunkedLength(v.ListenerConfig.AllowChunkedLength).
+				NumTrustedHops(v.ListenerConfig.XffNumTrustedHops)
+
+			if rlc := v.RateLimitConfig; rlc != nil {


nit: Could this be collapsed so we don't need to add the if (here and above)?

e.g. envoy_v3.GlobalRateLimitFilter could take just a RateLimitConfig and do the nil check there? and builder.AddFilter does nothing if it gets nil?

Yeah good idea, that'd clean this up; though as-is that would create an import cycle so need to re-jigger things a bit.

sunjayBhatia · 2021-02-10T15:21:00Z

cmd/contour/serve.go

@@ -294,6 +296,36 @@ func doServe(log logrus.FieldLogger, ctx *serveContext) error {
 		XffNumTrustedHops:             ctx.Config.Network.XffNumTrustedHops,
 	}

+	if ctx.Config.RateLimitService.ExtensionService != "" {
+		namespacedName := k8s.NamespacedNameFrom(ctx.Config.RateLimitService.ExtensionService)


Would it make sense to pull this sort of logic (fetching a service/checking for existence/getting some spec params) into a common helper? I'm thinking this logic could possibly be duplicated elsewhere?

The existing extension service code gets its ExtensionService via the DAG cache, so it's a different code path / not really duplicated with this logic. Maybe we can keep an eye on this for refactoring when we add our next extension service impl.

stevesloka

Overall looks great to me @skriss! Just a couple questions / thoughts.

stevesloka · 2021-02-11T18:22:47Z

cmd/contour/serve.go

+		// ensure the specified ExtensionService exists
+		res, err := client.Get(context.Background(), namespacedName.Name, metav1.GetOptions{})
+		if err != nil {
+			return fmt.Errorf("error getting rate limit extension service %s: %v", namespacedName, err)


Should this be a fatal error or not configure rate limiting until the extension service exists?

Agree it's not ideal to require the extension service to exist at startup. I think we could change the flow somewhat to resolve the extension service ref during the DAG build instead. If it's OK with you, I'll file a follow-up issue to look at that?

yup sure thing!

stevesloka · 2021-02-11T18:23:31Z

examples/contour/01-contour-config.yaml

@@ -99,3 +99,7 @@ data:
    #   Configure the number of additional ingress proxy hops from the
    #   right side of the x-forwarded-for HTTP header to trust.
    #   num-trusted-hops: 0
+    # rateLimitService:
+    #   extensionService: projectcontour/ratelimit


We have comments for other bits in this config explaining what they mean, maybe match that a bit?

stevesloka · 2021-02-11T18:25:08Z

examples/ratelimit/04-contour-config.yaml

@@ -0,0 +1,11 @@
+apiVersion: v1


Not sure if we should include this or just reference what to setup. I guess it's a nice copy/paste way to follow a guide so probably good.

stevesloka · 2021-02-11T18:31:26Z

site/_guides/global-rate-limiting.md

+
+## Deploy an RLS
+
+For this guide, we'll deploy the [Envoy rate limit service][4] as our RLS.


Maybe just give a couple more sentences on how the RLS works just so it's clear what is happening? (i.e. The RLS uses a Redis cache internally. Blah blah?)

copied an excerpt from the Envoy RLS README to give some more color.

site/docs/main/config/rate-limiting.md

youngnick

LGTM, nice work.

youngnick · 2021-02-15T02:33:21Z

cmd/contour/serve.go

+		// ensure the specified ExtensionService exists
+		res, err := client.Get(context.Background(), namespacedName.Name, metav1.GetOptions{})
+		if err != nil {
+			return fmt.Errorf("error getting rate limit extension service %s: %v", namespacedName, err)


If this is not fatal, then we need instead to do this similarly to the Envoy service status watcher - a separate goroutine that looks for the defined ExtensionService and sets up the config once it exists and is working.

I'd say that's quite a bit of extra work for minimal gain. If you're configuring the feature, you should have access to restart Contour, and you should be able to ensure everything is set up correctly.

See #3298 (comment), I think we could make some improvements here but I'm not considering it blocking.

skriss · 2021-02-17T22:04:46Z

I believe I've addressed everyone's review comments now, please take another look!

Adds support for configuring an external Rate Limit Service (RLS) that is used for making global rate limit decisions. Adds support to HTTPProxy for global rate limit policies, that generate descriptors for requests to be sent to the RLS for rate-limit decisions. Closes projectcontour#370. Signed-off-by: Steve Kriss <krisss@vmware.com>

Signed-off-by: Steve Kriss <krisss@vmware.com>

skriss · 2021-02-19T17:16:10Z

😅

Adds support for configuring an external Rate Limit Service (RLS) that is used for making global rate limit decisions. Adds support to HTTPProxy for global rate limit policies, that generate descriptors for requests to be sent to the RLS for rate-limit decisions. Closes projectcontour#370. Signed-off-by: Steve Kriss <krisss@vmware.com> Signed-off-by: iyacontrol <gaohj2015@yeah.net>

skriss added this to the 1.13.0 milestone Feb 1, 2021

skriss force-pushed the global-rate-limiting branch from cd3cc9d to f5fe2ca Compare February 1, 2021 22:32

skriss force-pushed the global-rate-limiting branch 4 times, most recently from 5131308 to dc96013 Compare February 4, 2021 22:54

skriss requested review from stevesloka, sunjayBhatia and youngnick February 4, 2021 23:09

skriss added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Feb 4, 2021

skriss force-pushed the global-rate-limiting branch 4 times, most recently from 9cdc0ea to f535ccf Compare February 9, 2021 17:22

skriss changed the title ~~[WIP] add global rate limiting support~~ add global rate limiting support Feb 9, 2021

skriss marked this pull request as ready for review February 9, 2021 17:23

skriss requested a review from a team as a code owner February 9, 2021 17:23

skriss force-pushed the global-rate-limiting branch from f535ccf to eb79b61 Compare February 9, 2021 20:56

sunjayBhatia reviewed Feb 9, 2021

View reviewed changes

sunjayBhatia reviewed Feb 10, 2021

View reviewed changes

skriss force-pushed the global-rate-limiting branch from eb79b61 to 883f443 Compare February 10, 2021 21:21

youngnick assigned skriss Feb 11, 2021

sunjayBhatia added this to In progress in Contour Project Board via automation Feb 11, 2021

sunjayBhatia moved this from In progress to 1.13 Release in Contour Project Board Feb 11, 2021

stevesloka reviewed Feb 11, 2021

View reviewed changes

youngnick approved these changes Feb 15, 2021

View reviewed changes

skriss force-pushed the global-rate-limiting branch from 883f443 to 465536c Compare February 17, 2021 16:07

skriss force-pushed the global-rate-limiting branch 2 times, most recently from 07a9a10 to 5036185 Compare February 17, 2021 16:45

skriss force-pushed the global-rate-limiting branch from 5036185 to ae633ba Compare February 18, 2021 17:17

sunjayBhatia approved these changes Feb 19, 2021

View reviewed changes

skriss added 4 commits February 19, 2021 08:19

codereview: simplify adding ratelimit filter to HCM

626c55e

Signed-off-by: Steve Kriss <krisss@vmware.com>

codereview: improve config param documentation

1a62097

Signed-off-by: Steve Kriss <krisss@vmware.com>

codereview: improve docs

fc921c6

Signed-off-by: Steve Kriss <krisss@vmware.com>

skriss force-pushed the global-rate-limiting branch from ae633ba to fc921c6 Compare February 19, 2021 15:19

stevesloka approved these changes Feb 19, 2021

View reviewed changes

stevesloka merged commit 4fafe5f into projectcontour:main Feb 19, 2021

skriss deleted the global-rate-limiting branch February 24, 2021 15:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add global rate limiting support #3298

add global rate limiting support #3298

skriss commented Feb 1, 2021 •

edited

codecov bot commented Feb 1, 2021 •

edited

skriss commented Feb 4, 2021

skriss commented Feb 9, 2021

sunjayBhatia left a comment

sunjayBhatia Feb 9, 2021

skriss Feb 10, 2021

sunjayBhatia Feb 9, 2021

skriss Feb 10, 2021 •

edited

sunjayBhatia Feb 10, 2021

skriss Feb 11, 2021

stevesloka left a comment

stevesloka Feb 11, 2021

skriss Feb 17, 2021

stevesloka Feb 19, 2021

stevesloka Feb 11, 2021

skriss Feb 17, 2021

stevesloka Feb 11, 2021

stevesloka Feb 11, 2021

skriss Feb 17, 2021

youngnick left a comment

youngnick Feb 15, 2021

skriss Feb 17, 2021

skriss commented Feb 17, 2021

skriss commented Feb 19, 2021


		## Deploy an RLS

		For this guide, we'll deploy the [Envoy rate limit service][4] as our RLS.

add global rate limiting support #3298

add global rate limiting support #3298

Conversation

skriss commented Feb 1, 2021 • edited

codecov bot commented Feb 1, 2021 • edited

Codecov Report

skriss commented Feb 4, 2021

skriss commented Feb 9, 2021

sunjayBhatia left a 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

skriss Feb 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevesloka left a 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

youngnick left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skriss commented Feb 17, 2021

skriss commented Feb 19, 2021

skriss commented Feb 1, 2021 •

edited

codecov bot commented Feb 1, 2021 •

edited

skriss Feb 10, 2021 •

edited