config: increment default rate limiting #323
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
diegodelemos
commented
Nov 23, 2020
diegodelemos
commented
- Addresses Error message "Cannot start workflow" reana#286.
Codecov Report
@@ Coverage Diff @@
## maint-0.7 #323 +/- ##
==========================================
Coverage 54.37% 54.37%
==========================================
Files 17 17
Lines 1337 1337
==========================================
Hits 727 727
Misses 610 610 |
diegodelemos
commented
Nov 23, 2020
reana_server/config.py
Outdated
| @@ -116,6 +116,10 @@ def _(x): | |||
| APP_DEFAULT_SECURE_HEADERS["content_security_policy"] = {} | |||
| APP_HEALTH_BLUEPRINT_ENABLED = False | |||
|
|
|||
| # Rate limiting configuration | |||
| # =========================== | |||
| RATELIMIT_ENABLED = False | |||
There was a problem hiding this comment.
Currently hardcoding this value.. but actually, it would be interesting to make it configurable in case someone wants to adjust it. Some thoughts about it:
Limiting with Invenio-App/Flask-limiter:
- Flexibility: Configuring means touching many different variables (limiting per endpoint, limiting auth VS anonymous users) which allow fine-tuning.
- To be adapted to Kubernetes: By default the limiter relies on in memory entries to store for the number of requests (this would become a problem if RS is scaled up). The solution is easy and it would be to connect the rate limiter to Redis (to be tested).
Limiting with Traefik rate limiter:
- Less-flexibility: We would have less granularity as doing it inside the application.
- Closer to prod setup: It would be more similar to what we do in prod with HAProxy.
- More harmony: the approach of rate-limiting inside the app wouldn't work for Interactive sessions (and future entities like them), Traefik would.
Not providing any in-cluster option to limit requests. Advice to use external service (like we do with HAProxy).
diegodelemos
force-pushed
the
disable-rate-limiting
branch
from
f5ded3e
to
d5c200c
Compare
tiborsimko
reviewed
Nov 23, 2020
| @@ -116,6 +116,11 @@ def _(x): | |||
| APP_DEFAULT_SECURE_HEADERS["content_security_policy"] = {} | |||
| APP_HEALTH_BLUEPRINT_ENABLED = False | |||
|
|
|||
| # Rate limiting configuration | |||
| # =========================== | |||
| RATELIMIT_AUTHENTICATED_USER = "20 per second" | |||
There was a problem hiding this comment.
Please amend CHANGES.rst and say something like:
- Changes rate limiting defaults to allow up to 20 connections per second.
diegodelemos
force-pushed
the
disable-rate-limiting
branch
from
d5c200c
to
e1e2020
Compare
diegodelemos
force-pushed
the
disable-rate-limiting
branch
from
e1e2020
to
3091416
Compare
tiborsimko
reviewed
Nov 23, 2020
CHANGES.rst
Outdated
| @@ -8,6 +8,8 @@ Version 0.7.1 (2020-11-10) | |||
| - Fixes restarting of Yadage and CWL workflows. | |||
| - Fixes conflicting ``kombu`` installation requirements by requiring Celery version 4. | |||
| - Changes ``/api/you`` endpoint to include REANA server version information. | |||
| - Changes rate limiting defaults to allow up to 20 connections per second. | |||
There was a problem hiding this comment.
Must go into a new section:
Version 0.7.2 (UNRELEASED)
--------------------------
- Changes rate limiting defaults to allow up to 20 connections per second.
3091416
to
106b88c
Compare
tiborsimko
approved these changes
Nov 23, 2020
There was a problem hiding this comment.
Tested locally. The situations where I simulated rate limitations before:
File /LICENSE was successfully uploaded.
File /LICENSE was successfully uploaded.
File /LICENSE was successfully uploaded.
Something went wrong while uploading .../LICENSE:
60 per 1 minute
Something went wrong while uploading .../LICENSE:
60 per 1 minute
Something went wrong while uploading .../LICENSE:
60 per 1 minute
File /LICENSE was successfully uploaded.
File /LICENSE was successfully uploaded.
File /LICENSE was successfully uploaded.
are now passing fine 👍
tiborsimko
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.