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

Add access rate limit #676

Merged
merged 11 commits into from
Dec 5, 2021
Merged

Conversation

truongdd03
Copy link
Contributor

@truongdd03 truongdd03 commented Nov 27, 2021

Description

This PR uses express-rate-limit module to set two access limits. The first one (200 requests/ 5 seconds) limits the rate of overall requests to OED. At the moment, the current limit is high due to issue #681. After resolving it, testings are needed to adjust the rate limit. The second limit is specifically for raw export (5 requests/ 5 seconds).

Fixes #266

Type of change

  • Note merging this changes the node modules
  • Note merging this changes the database configuration.
  • This change requires a documentation update

Checklist

  • I have followed the OED pull request ideas
  • I have removed text in ( ) from the issue request

Limitations

Note that the rate limit may make future automatic tests return the value of 429 (too many requests). In that case, the limiters need to be increased.

truongdd03 and others added 6 commits November 27, 2021 00:03
I took the basic script from @truongdd03 and made it a little more
general. I felt it was good to have it in the testing directory so
others would have access to it. Note this is currently only manual
testing but automating at some point might be good, esp. if we
limit specific routes.
@huss
Copy link
Member

huss commented Dec 2, 2021

This is good work and does basic rate limiting. Testing indicates it works as expected. I generalized the test script written by @truongdd03 and put it in the testing directory for future use.

I have two thoughts on this PR:

  1. Why is the overall limit set at 300/minute? It seems like it may be fine but was there testings to see if this would work for sites? Can a single user request may count as multiple requests toward the limit if it makes api calls internally?Just want to know for the record.
  2. Do people know if changing the time frame for the reset (currently 1 minute) would help with accidentally hitting it with valid requests at a site?
  3. I do think the overall limit cannot be too low as we want log api requests to succeed so log messages are not lost.
  4. In the long-term, we might want to have a default value and allow the admin to modify that value. That is beyond this PR but just noting.
  5. An overall limit seems good but I think specific routes should probably also have limits. For example, asking for the raw data is going to be much more draining on resources than asking for bar graphic data. The default size to download without a login is 5MB so at 300 requests/min that would be 1.5 GB/min which would certainly tax the system. We don't expect a lot of these requests so maybe limiting to 5-10 per minute might be better. I don't see any other obvious routes that doe not require a login that are an issue like this.

src/server/app.js Outdated Show resolved Hide resolved
- add test for new raw limit.
- remove extra echos and commented out code.
@huss huss added this to the 0.8 release milestone Dec 5, 2021
@huss
Copy link
Member

huss commented Dec 5, 2021

Good work. I appreciate the effort to test and get all the details working. This rate limits overall requests (allowing a lot) and raw download (only a few). I updated the test script a little to deal with items. It all tests out fine so ready to merge.

@huss huss merged commit 565cbb7 into OpenEnergyDashboard:development Dec 5, 2021
@truongdd03 truongdd03 deleted the rate_limiting branch December 8, 2021 02:18
@truongdd03 truongdd03 restored the rate_limiting branch December 8, 2021 02:19
@truongdd03 truongdd03 deleted the rate_limiting branch December 8, 2021 02:19
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.

Rate Limiting
2 participants