-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add basic rate limiter whitelist (specific IPs only) #573
base: main
Are you sure you want to change the base?
Conversation
Hi @zguig52 |
Hi @vejja , |
NB: for the undefined value test, it is already performed with existing tests as it is the default value configured |
Hi @zguig52 NB: The bot was unable to inline the PR preview because your branch is outside of this repo but I triggered it manually. |
@zguig52 : Before we release this, I'd like to get @Baroshem's advice here because I'm not extremely knowledgeable about the Rate Limiter. Do we agree that we are allowing ip whitelists here, as opposed to domain names ? Let me know if I'm correct |
@vejja , this is an IPs CLIENT whitelist that is managed here. Main usages are for production:
If you look in the function associated to the rateLimiter, it uses client IP to determine and store all calls performed to the endpoint by a specific client and determine if rates are normal:
This is a h3 function that provide client IP https://github.com/unjs/h3/blob/77510f3c27e6884489189b300c52b5355751aefc/src/utils/request.ts#L297 So I reused this information to compare it to the whitelist and skip rateLimit checks and storage if it matches. |
I am deploying it in a qualification environment to check if it behaves as expected with public clients IPs |
Oh oh I think I missed your whole point effectively ! But if I understand correctly, the use cases are significantly larger than what I thought
Am I right here ? |
Just did some tests with this release, it works as expected (at least for a single public IP configured, which is for the moment my tests conditions). I used siege tool to overflow server API routes and it doesn't trigger the rateLimit. When I am behind the whitelisted public IP, no 429. When I use another public IP, it trigger 429! |
ok clear |
@Baroshem : whenever you think this is ready - I believe this is a feature upgrade, shall we bump to minor v2.2 ? |
I have reviewed a bit my comments on this, as I think we might be able to add more capabilities with the same configuration format, without breaking this first implementation (or maybe we already change before release for basic input config format to be able to change it without breaking changes later). |
Types of changes
Description
Partially resolves #517 (to be discussed further for more advances use cases)
Checklist: