Skip to content
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

Filter Idea: Rate limiting #5

Closed
markmandel opened this issue Jan 28, 2020 · 10 comments
Closed

Filter Idea: Rate limiting #5

markmandel opened this issue Jan 28, 2020 · 10 comments
Labels
area/user-experience Pertaining to developers trying to use Quilkin, e.g. cli interface, configuration, etc kind/design Proposal discussing new features / fixes and how they should be implemented kind/feature New feature or request priority/high Issues that should be addressed as soon as possible.

Comments

@markmandel
Copy link
Contributor

Since a game server / client usually has a known frequency for sending packets, having a rate limiting filter seems like an easy win for protecting game servers.

This would likely be more relevant on the receiver (game server) side, but may have applications on the client (sender) side.

@markmandel markmandel added kind/feature New feature or request area/user-experience Pertaining to developers trying to use Quilkin, e.g. cli interface, configuration, etc kind/design Proposal discussing new features / fixes and how they should be implemented labels Jan 28, 2020
iffyio added a commit that referenced this issue Jul 9, 2020
This adds a local rate limiter filter that applies to
traffic destined to a proxy's endpoints.

Fixes #5
@iffyio
Copy link
Collaborator

iffyio commented Jul 18, 2020

Todo

  • Add proper configuration from yaml
  • Add integration test
  • Add metrics
  • Add option to configure traffic direction to rate limit
  • Add ability to configure which endpoints will be rate limited

@iffyio
Copy link
Collaborator

iffyio commented Jul 18, 2020

Add ability to configure which endpoints will be rate limited

@markmandel Is the idea to turn off rate limiting for some endpoints and/or set different limits depending on the endpoints? I imagine this would require specifying each endpoint in the rate limiter's config? e.g

  filters:
    - name: quilkin.rate_limiter
      config:
        - endpoint: gs1
          max_packets: 10
          period: 1
        - endpoint: gs2
          max_packets: 20
          period: 1

vs

  filters:
    - name: quilkin.rate_limiter
      config:
        max_packets: 10
        period: 1

we could instead have the rate limiter config not concerned with endpoints and instead make it possible to configure multiple filter chains depending on endpoints.

@markmandel
Copy link
Contributor Author

Add ability to configure which endpoints will be rate limited

Apologies, I totally used the wrong words. What I meant to say, is that there could be an option to determine which Filter function the rate limiting is applied to, with a default.

So a Rate Limiter could be applied to local_receive_filter, local_send_filter, endpoint_receive_filter or endpoint_send_filter - as needed.

So something like:

  filters:
    - name: quilkin.rate_limiter
      config:
        max_packets: 10
        period: 1
        direction: local_receive

(I actually see more reason to have a rate limit on receiving than sending, personally)

I think this rate limiter only needs one "direction" (couldn't come up with a better name), as you can always add more rate limiting filters into the FilterChain.

I don't think we need to have filter config per endpoint - endpoint data is too mutable, and I don't think it's necessary, at least at this point.

@iffyio
Copy link
Collaborator

iffyio commented Aug 10, 2020

I'm not entirely sure of adding this option because it's dependent on how the filter trait is defined. Ideally, the rate limiter config would only deal with rate limiting functionality while e.g direction is handled by the filter chain mechanism. Though I'm not sure what that would look like config wise or if we need to allow rate limiting in the opposite direction

Also I'm wondering whether having 2 send and 2 receive callbacks on the Filter trait complicates things a bit.
From what I can tell, an endpoint_send_filter callback will always be at the tail end of the local_receive_filter chain while a local_send_filter callback is at the end of the endpoint_receive_filter chain.
So that for example a chain
local_receive_filter => A -> B
endpoint_send_filter => C -> D
is the same as local_receive_filter => A -> B -> C -> D

If this is the case we could consider using 1 callback each for send and receive on the filter trait since that would be simpler and make the api harder to misuse like I initially did with rate limiting on send instead of receive. WDYT?

@markmandel
Copy link
Contributor Author

So I think there are 2 things here:

  1. Should have rate limiting available on local_receive_filter and endpoint_receive_filter?

So let me give two scenarios. To stop excess or malicious data coming from a client to a game server, rate limiting on local_receive makes perfect sense.

But what I want to protect from a game server going rogue (or being hacked), and suddenly sending way more data than it should from game server to client - how do I rate limit what data is going from game server back to the client? Right now, I don't think we can with the current filter implementation.

I think this could be handled by a direction of either local or endpoint - since it's only ever going to be receive.

  1. Should we consolidate filters? This is an interesting question, but should probably be a separate ticket, to not intermingle conversations.

Thoughts?

@iffyio
Copy link
Collaborator

iffyio commented Aug 11, 2020

Makes sense, we can use the direction option to configure the rate limiter then

iffyio added a commit that referenced this issue Aug 11, 2020
* Also reject configs with period < 100ms since we can't
  really guarantee correctness at lower resolutions.

Work on #5
iffyio added a commit that referenced this issue Aug 11, 2020
* Make period optional, default to 1sec.
* Reject configs with period < 100ms since we can't
  really guarantee correctness at lower resolutions.

Work on #5
markmandel pushed a commit that referenced this issue Aug 18, 2020
* Add rate limiter config and integration test

* Make period optional, default to 1sec.
* Reject configs with period < 100ms since we can't
  really guarantee correctness at lower resolutions.

Work on #5

* Fix filter naming
iffyio added a commit that referenced this issue Aug 21, 2020
markmandel pushed a commit that referenced this issue Aug 21, 2020
markmandel added a commit that referenced this issue Aug 22, 2020
I hope I got this right, I'm not sure I understand how Prometheus
metrics work.

Work on #5
markmandel added a commit that referenced this issue Aug 22, 2020
* Documentation for LocalRateLimit Filter Metrics

I hope I got this right, I'm not sure I understand how Prometheus
metrics work.

Work on #5

* Update src/extensions/filters/local_rate_limit/mod.rs

Co-authored-by: Ifeanyi Ubah <[email protected]>
@XAMPPRocky
Copy link
Collaborator

It seems like this was implemented, should we close?

@markmandel
Copy link
Contributor Author

We are still missing the "Add option to configure traffic direction to rate limit" - The LocalRateLimit only does rate limiting on local ➡️ endpoint, but not endpoint ➡️ local.

We should probably discuss if this should be a seperate filter, or if it should be a config option on an RateLimit Filter. 🤔

@iffyio
Copy link
Collaborator

iffyio commented May 19, 2021

Ooh also it currently rate limits on the entire proxy level but likely what makes more sense is to ratelimit on the session level instead so e.g 60pps doesn't mean the proxy can only allow that muc

@XAMPPRocky XAMPPRocky added priority/medium Issues that we want to resolve, but don't require immediate resolution. priority/high Issues that should be addressed as soon as possible. and removed priority/medium Issues that we want to resolve, but don't require immediate resolution. labels Jul 5, 2021
iffyio added a commit that referenced this issue Sep 26, 2021
Instead of applying the rate limit on the entire proxy,
this applies it per source address which is the
desired behavior.

Resolves #405
Work on #5
markmandel pushed a commit that referenced this issue Sep 28, 2021
* Add a ttlmap datastructure

* Add support to rate limit per IP

Instead of applying the rate limit on the entire proxy,
this applies it per source address which is the
desired behavior.

Resolves #405
Work on #5
@XAMPPRocky
Copy link
Collaborator

We can close this now with #406 right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Quilkin, e.g. cli interface, configuration, etc kind/design Proposal discussing new features / fixes and how they should be implemented kind/feature New feature or request priority/high Issues that should be addressed as soon as possible.
Projects
None yet
Development

No branches or pull requests

3 participants