Envoy: add local http rate limit filter

Created on 16 Apr 2020  Â·  20Comments  Â·  Source: envoyproxy/envoy

Similar to #9354, but L7. I'll be sending a PR soon.

cc: @mattklein123 (FYI -- in case someone else is working on this)
cc: @fishcakez

areratelimit help wanted

All 20 comments

@rgs1 awesome, thanks. The config/API for this is more complicated. Can you possibly do a config only PR first so we can lock that down?

Yes -- let me send that along with the empty filter code (or, I can strip that away too).

@mattklein123 I actually don't have something very different in mind, here's a first attempt: #10820.

I am thinking of using per filter configs, to be able to associate a token bucket to a given route (which is the use case we have). Other than that, plus being able to add some additional response headers, that's pretty much it (I think).

Well, additionally it might be nice to be able to override the token bucket via RTDS. But that can be done/discussed separately.

Do you have any specific needs/concerns in mind?

@rgs1 @lizan @htuch @mattklein123
I am also investigating UDPA of the rate-limit related rules recently. Here are some fields I added commit on the basis of @rgs1 PR.

As follows:

message LocalRateLimit {
   ……
  // The global_switch used to control enable or disable limit feature.
  bool global_switch = 1;

  // The dry_run filed used to control the rate limit mode.
  // if enable the dry run mode, requests processing rate is not limited and only record log. 
  bool dry_run = 2;

  // This field allows to send a HTTP response body to the downstream client
  // when the request has been rate limited.
  string body = 4;
……
}

In addition, I think the limite-key field in the limit rules should designed to be protocol-independent. We can add a filed of ResourceName, such as enum {api.PATH, api.IP, api.URI, api.ARG}, And then each specific protocol to implement the resource get method.

The meaning of api.PATH resource names in different protocols:

  • HTTP request path
  • Dubbo service name

The above are some of my ideas, I hope to discuss together, thx.

@wangfakang I reckon that it makes sense to validate your API here in xDS and then when we start diving into the data model in UDPA we can pull from this.

FYI: we ended up doing an internal implementation for this -- since we were in a bit of a hurry -- and it's been in prod for > 2 months and turned out pretty useful. I am happy to upstream, it at least for reference purpose since it'll take a bit to get agreement on the config API/interface, if there's any interest.

@rgs1 , @mattklein123 : We would like this feature too.. so how can we help get this revived again?

@rgs1 , @mattklein123 : We would like this feature too.. so how can we help get this revived again?

I would recommend syncing up with @rgs1 on open sourcing their internal impl. I think we can do something pretty easily with a per-route config for an HTTP local rate limit filter.

I think I should be able to send a PR with ours this week.

@rgs1 : cool.. thanks! Please cc me on it.

@rgs1 : Is it based on the config that was on review before? Just want to make sure,if it will cover our needs...

roughly, i think we added a few more things.

@rgs1 : Please let us know how we can help...

@gargnupur sorry, couldn't get to this last week... I'll carve out some time for tomorrow.

@rgs1 : np... thanks for the help!

@rgs1 if you are able to attach a diff or something partial, it would be still be Great help.

@rgs1 if you are able to attach a diff or something partial, it would be still be Great help.

i did make some progress on Fri cleaning up the branch, should be posting today.

@mandarjog @gargnupur here you go #13395 -- I'll make sure it's in good shape for review (CI passing, docs, etc).

Thanks @rgs1 !

Thanks @rgs1 !!!

Was this page helpful?
0 / 5 - 0 ratings