local rate limit HTTP filter: add config interface #10820

rgs1 · 2020-04-17T00:23:10Z

This is just the config API with an overview doc. Implementation
will follow, once we have an agreement.

Signed-off-by: Raul Gutierrez Segales rgs@pinterest.com

This is just the config API with an overview doc. Implementation will follow, once we have an agreement. See envoyproxy#10813. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only · 2020-04-17T00:23:15Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.

🐱

Caused by: #10820 was opened by rgs1.

see: more, trace.

euroelessar · 2020-04-17T03:03:16Z

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

+  // This field allows to send a HTTP response status code to the downstream client other
+  // than 429 (TooManyRequests) when the request has been rate limited.
+  type.v3.HttpStatus status = 1;


It's a possible scenario that the same rate limiter is applied to both grpc and http traffic (e.g. in case of transcoding or virtual_host-level rate limiter).
In this scenarios the status code options would conflict (as gRPC mandates the usage of 200 code).

Should grpc be introduced as first class citizen or are there other options?

You mean, a same route that does both HTTP and gRPC?

Yes, few examples:

There is a single gRPC route /service.Service/Method but it has envoy.filters.http.grpc_json_transcoder filter enabled. It allows the same route to serve both HTTP and gRPC traffic (all of it is converted to gRPC before going to upstream). Transcoding itself is potentially pricey (it requires buffering the whole request and is ≈reflection based), so one may want to put rate limiter before it.

Rate limiter is set for the whole VirtualHost, virtual host can serve traffic for different variations of traffic, e.g. it can have few gRPC and few HTTP upstreams.

So we'd want to be able to configure an HTTP and a gRPC status code -- OK. Do we do this anywhere else in the API?

Not anywhere today, though see #10841 where we are adding gRPC fault codes which is somewhat similar.

Sorry, Whether the status field could be set to universal? Then the rate-limit filter module convert status-code depends on the requeste protocol. Here is a discuss #10813 .

I couldn't get docs to build without the new filter referenced in source/extensions/extensions_build_config.bzl, so I added the boilerplate pieces of the filter to get docs built. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2020-04-17T16:13:44Z

@mattklein123 so my first question is, are we happy with using per filter configs to configure the local rate limiter or do we want to extend RouteAction or a similar core config object?

My take is that, for starters per filter configs gives us more freedom to experiment with this and once we are happy we can eventually merge this with core objects.

Thoughts?

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

rgs1 · 2020-04-17T18:01:02Z

@mattklein123 so my first question is, are we happy with using per filter configs to configure the local rate limiter or do we want to extend RouteAction or a similar core config object?

My take is that, for starters per filter configs gives us more freedom to experiment with this and once we are happy we can eventually merge this with core objects.

Thoughts?

cc: @euroelessar on this too

euroelessar · 2020-04-17T18:30:50Z

@mattklein123 so my first question is, are we happy with using per filter configs to configure the local rate limiter or do we want to extend RouteAction or a similar core config object?

It feels to me that it should be a separate filter/extension, what are the reasons it needs to be a part of the Router itself?

rgs1 · 2020-04-17T18:32:08Z

@mattklein123 so my first question is, are we happy with using per filter configs to configure the local rate limiter or do we want to extend RouteAction or a similar core config object?

It feels to me that it should be a separate filter/extension, what are the reasons it needs to be a part of the Router itself?

Actually, no reasons now that I think about it... Given it won't be core functionality. Per filter configs tend to look less natural, but I think it's the right thing here. So I take the question back :-)

htuch · 2020-04-20T00:18:18Z

api/envoy/config/filter/http/local_rate_limit/v2/BUILD

+
+api_proto_package(
+    deps = [
+        "//envoy/api/v2/core:pkg",


v2 is now frozen, so please add directly to v3, thanks.

htuch · 2020-04-20T00:19:17Z

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

+  //   refills.
+  type.v3.TokenBucket token_bucket = 2;
+
+  // This uniquely identifies a specific local rate limit configuration (e.g.: when using per route


I thought the convention was that per-filter configs use the filter name as their key, so we don't technically need this.

This is different -- lets say you are configuring local rate limiters on two different routes (/foo and /bar). This allows you to configure individual runtime keys for each route, so you can enable/enforce separately.

Is there a better way to distinguish the per-filter config of multiple routes/vhosts for things like runtime gatekeepers?

htuch · 2020-04-20T00:22:15Z

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

+  // have been rate limited.
+  repeated config.core.v3.HeaderValueOption response_headers_to_add = 10
+      [(validate.rules).repeated = {max_items: 10}];
+}


Did you consider making this a variation on the existing rate limit filter, with some additional config for local response? Asking since it might be the case that there is a bunch of shared config, e.g. request_type, failure_mode_deny.

I did, but ended up concluding that there is much more in common with the network local rate limiter (than the existing HTTP global rate limiter).

@mattklein123 thoughts?

I think originally had a similar inclination to @htuch about building this into the existing rate limit filter, but seeing this config it's simpler enough that I can buy having a separate filter for it. The only thing that would give me pause is whether someone is going to want to match on different types of things (descriptors) and whether we might be better served by constructing a descriptor as a key to a local token bucket? I'm not sure if this is worth it or not but just throwing it out there and I obviously didn't bother doing this for the network rate limit case (albeit a much simpler case). Thoughts?

mattklein123

Thanks for working on this. Flushing out a few comments/questions.

/wait

mattklein123 · 2020-04-20T23:30:36Z

api/envoy/config/filter/http/local_rate_limit/v2/local_rate_limit.proto

+  // rate limiters). It will be used to construct the
+  // :ref:`runtime keys <config_http_filters_local_rate_limit_runtime>` that enable and enforce
+  // the corresponding local rate limiter.
+  string route_key = 3 [(validate.rules).string = {min_bytes: 1}];


Should we just use RuntimeFractionalPercent or RuntimeFeatureFlag here or similar?

mattklein123 · 2020-04-20T23:32:22Z

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

+  // This field allows to send a HTTP response status code to the downstream client other
+  // than 429 (TooManyRequests) when the request has been rate limited.
+  type.v3.HttpStatus status = 1;


Not anywhere today, though see #10841 where we are adding gRPC fault codes which is somewhat similar.

mattklein123 · 2020-04-20T23:35:33Z

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

+  // have been rate limited.
+  repeated config.core.v3.HeaderValueOption response_headers_to_add = 10
+      [(validate.rules).repeated = {max_items: 10}];
+}


I think originally had a similar inclination to @htuch about building this into the existing rate limit filter, but seeing this config it's simpler enough that I can buy having a separate filter for it. The only thing that would give me pause is whether someone is going to want to match on different types of things (descriptors) and whether we might be better served by constructing a descriptor as a key to a local token bucket? I'm not sure if this is worth it or not but just throwing it out there and I obviously didn't bother doing this for the network rate limit case (albeit a much simpler case). Thoughts?

mattklein123 · 2020-04-20T23:36:46Z

docs/root/configuration/http/http_filters/local_rate_limit_filter.rst

+The local rate limit filter outputs statistics in the *cluster.<route target cluster>.local_ratelimit.* namespace.
+429 responses -- or the configured status code -- are emitted to the normal cluster :ref:`dynamic HTTP statistics
+<config_cluster_manager_cluster_stats_dynamic_http>`.


I think you might want an optional stat prefix here. If you start having multiple per-route configs, it would be very unclear which limit is being hit. Also, I would make sure we are setting the RL access log code also.

mattklein123 · 2020-04-20T23:37:22Z

source/extensions/filters/http/local_ratelimit/local_ratelimit.h

+ * HTTP local rate limit filter. Depending on the route configuration, this filter calls consults
+ * with local token bucket before allowing further filter iteration.
+ */
+class Filter : public Http::StreamFilter {


Use the pass through filter here as the base class.

wangfakang · 2020-04-22T08:09:34Z

api/envoy/config/filter/http/local_rate_limit/v2/local_rate_limit.proto

+  // This field allows to send a HTTP response status code to the downstream client other
+  // than 429 (TooManyRequests) when the request has been rate limited.
+  type.HttpStatus status = 1;
+


I have some questions and discussed by #10813.

wangfakang · 2020-04-22T08:21:50Z

api/envoy/extensions/filters/http/local_ratelimit/v3/local_rate_limit.proto

+  // This field allows to send a HTTP response status code to the downstream client other
+  // than 429 (TooManyRequests) when the request has been rate limited.
+  type.v3.HttpStatus status = 1;


Sorry, Whether the status field could be set to universal? Then the rate-limit filter module convert status-code depends on the requeste protocol. Here is a discuss #10813 .

stale · 2020-04-29T08:37:54Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

stale · 2020-05-06T11:34:58Z

This pull request has been automatically closed because it has not had activity in the last 14 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

local rate limit HTTP filter: add config interface

5756a49

This is just the config API with an overview doc. Implementation will follow, once we have an agreement. See envoyproxy#10813. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

repokitteh-read-only bot added the api label Apr 17, 2020

rgs1 mentioned this pull request Apr 17, 2020

add local http rate limit filter #10813

Closed

euroelessar reviewed Apr 17, 2020

View reviewed changes

Fix docs

09b589a

I couldn't get docs to build without the new filter referenced in source/extensions/extensions_build_config.bzl, so I added the boilerplate pieces of the filter to get docs built. Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

add filter to CODEOWNERS, make check format happy

2392efa

Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

mattklein123 self-assigned this Apr 18, 2020

htuch reviewed Apr 20, 2020

View reviewed changes

mattklein123 requested changes Apr 20, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Apr 20, 2020

wangfakang reviewed Apr 22, 2020

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 29, 2020

stale bot closed this May 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

local rate limit HTTP filter: add config interface #10820

local rate limit HTTP filter: add config interface #10820

rgs1 commented Apr 17, 2020

repokitteh-read-only bot commented Apr 17, 2020

euroelessar Apr 17, 2020

rgs1 Apr 17, 2020

euroelessar Apr 17, 2020

rgs1 Apr 20, 2020

mattklein123 Apr 20, 2020

wangfakang Apr 22, 2020

rgs1 commented Apr 17, 2020

rgs1 commented Apr 17, 2020

euroelessar commented Apr 17, 2020

rgs1 commented Apr 17, 2020

htuch Apr 20, 2020

rgs1 Apr 20, 2020

htuch Apr 20, 2020

rgs1 Apr 20, 2020

htuch Apr 20, 2020

rgs1 Apr 20, 2020

mattklein123 Apr 20, 2020

mattklein123 left a comment

mattklein123 Apr 20, 2020

mattklein123 Apr 20, 2020

mattklein123 Apr 20, 2020

mattklein123 Apr 20, 2020

mattklein123 Apr 20, 2020

wangfakang Apr 22, 2020

wangfakang Apr 22, 2020

stale bot commented Apr 29, 2020

stale bot commented May 6, 2020

local rate limit HTTP filter: add config interface #10820

local rate limit HTTP filter: add config interface #10820

Conversation

rgs1 commented Apr 17, 2020

repokitteh-read-only bot commented Apr 17, 2020

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

rgs1 commented Apr 17, 2020

rgs1 commented Apr 17, 2020

euroelessar commented Apr 17, 2020

rgs1 commented Apr 17, 2020

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

mattklein123 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

stale bot commented Apr 29, 2020

stale bot commented May 6, 2020