Skip to content

API server rate limiting #2031

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

Merged
merged 9 commits into from
Jan 8, 2021
Merged

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Dec 8, 2020

What do these changes do?

Trying to add some rate limiting to the API server. This will filter based on the IP present in the X-Forwarded-For header added by Traefik.

Note: this solution will not work on local deployments. All traffic is passing from the same IP.

Note2: the limit is very low. @pcrespov you might know better that me what to setup as limits.

Related issue number

How to test

Checklist

  • Did you change any service's API? Then make sure to bundle document and upgrade version (make openapi-specs, git commit ... and then make version-*)
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@codecov
Copy link

codecov bot commented Dec 8, 2020

Codecov Report

Merging #2031 (6e28da4) into master (e1933ca) will increase coverage by 0.0%.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2031   +/-   ##
======================================
  Coverage    72.9%   72.9%           
======================================
  Files         421     421           
  Lines       15487   15487           
  Branches     1568    1568           
======================================
+ Hits        11291   11296    +5     
+ Misses       3789    3785    -4     
+ Partials      407     406    -1     
Flag Coverage Δ
integrationtests 63.6% <ø> (+<0.1%) ⬆️
unittests 67.4% <ø> (-0.1%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ce_webserver/resource_manager/garbage_collector.py 76.6% <0.0%> (-1.9%) ⬇️
...simcore_service_webserver/projects/projects_api.py 84.6% <0.0%> (-0.5%) ⬇️
...webserver/computation_comp_tasks_listening_task.py 90.9% <0.0%> (+9.0%) ⬆️
..._service_webserver/projects/projects_exceptions.py 100.0% <0.0%> (+9.5%) ⬆️

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Can you create a helper with a client that produces a given request rate? That will help us tuning the limit we wish to have. Could add that e.g. in pytest-simcore/helpers

@GitHK
Copy link
Contributor Author

GitHK commented Dec 8, 2020

Can you create a helper with a client that produces a given request rate? That will help us tuning the limit we wish to have. Could add that e.g. in pytest-simcore/helpers

Yes I will try to have this in place.

@GitHK GitHK requested review from pcrespov and sanderegg December 14, 2020 09:07
@GitHK
Copy link
Contributor Author

GitHK commented Dec 14, 2020

So I've added 2 utils to manually run when testing the rate limit of the service. Based on the parameters the test could run in seconds or in hours.

If you have suggestions on how to transform it into a test I am listening. Also the parameters could be picked up from the docker-compose.yml file to test all configured services automatically. I would do this only if were have a reliable "time boxed" test for bursts and steady rate testing for the rate limits.

@pcrespov @sanderegg

@GitHK GitHK merged commit c19ebb0 into ITISFoundation:master Jan 8, 2021
@GitHK GitHK deleted the api-ratelimitis branch January 8, 2021 07:45
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.

3 participants