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

Rate limiter #278

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

andrew4699
Copy link
Contributor

@andrew4699 andrew4699 commented Sep 10, 2024

Description

Adds a rate limiter to protect against DoS-style attacks.

There are 3 implementations to choose from:

  • DefaultRateLimiter which uses the OpenTelemetry token bucket implementation
  • NoOpRateLimiter which does no rate limiting
  • MockRateLimiter which comes with a MockClock that can be used in tests

Implementations are dynamically discoverable through META-INF and there are interfaces so you can plug your own.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

  • Existing integration tests work with a rate limiter configured
  • New rate limiter integration tests
  • Manual testing

Checklist:

  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution @andrew4699!

Adding protections to the service is a great thing! However, I see a couple of issues with the approach, added a couple of comments to this PR. It's mostly about concurrency and blocking behavior.

Copy link
Member

@snazy snazy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for refactoring the CHM stuff - that's much cleaner and safer IMO.

I'm generally okay with this change.
Wonder though whether Guava's RateLimiter would fit the needs though - but I don't see it as a blocker for this PR.

I'll be somewhat unavailable in the next couple of days, so if there's anybody else who can review as well and happy with it, I'm okay with merging.

* maximum amount of tokens. Each "acquire" costs 1 token.
*/
public class TokenBucketRateLimiter implements RateLimiter {
private final double tokensPerNano;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those doubles could all be longs.

I wonder though whether com.google.common.util.concurrent.RateLimiter wouldn't satisfy the need as well. Using the Guava one could save some testing pain ;) OTOH it uses Guava's clock/watch stuff.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was looking into the different rate limiters that are already pulled in by our dependencies and did see that one, however it doesn't seem to support a custom window size. I had originally pulled in the OpenTelemetry one because it was basically equivalent to what I wanted, but didn't realize that it was an internal package. I figured since the implementation is small enough, it was better to just implement it than to try finding a new library for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated most of the doubles to longs.

Copy link

@mrcnc mrcnc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants