-
Notifications
You must be signed in to change notification settings - Fork 2
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
Different rate limits depending on HTTP method (#5555) #5869
base: develop
Are you sure you want to change the base?
Different rate limits depending on HTTP method (#5555) #5869
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #5869 +/- ##
===========================================
+ Coverage 85.54% 85.56% +0.02%
===========================================
Files 147 147
Lines 21226 21216 -10
===========================================
- Hits 18158 18154 -4
+ Misses 3068 3062 -6 ☔ View full report in Codecov by Sentry. |
34336cf
to
1259465
Compare
Tested against personal deployment Using
Showing POST requests are blocked while GET request are allowed:
|
|
Also, |
1259465
to
caad149
Compare
The above test was run prior to deciding to run a test with concurrent requests. While
and after the WAF starts blocking the requests, the response is
Also, it's actually PUT, not POST we want to block as an expensive request. This will be fixed before the next test with concurrent requests. |
2a95628
to
0139402
Compare
0139402
to
13e373d
Compare
Test results making concurrent manifest generation requests with the manifest Lambda's 5 concurrent threads
9 concurrent threads
10 concurrent threads
11 concurrent threads
15 concurrent threads
|
It's impossible to reproduce your experiment without the source to the script that you presumably used. Please repeat the experiment with a simple command line that uses |
4c4bfc6
to
9b79b0a
Compare
2d5008a
to
3366611
Compare
Tested 15 concurrent manifest requests with the manifest Lambda concurrency set to 10. manifest_urls.txt
Command line
Terminal log(abridged to show the all initial requests, and then the thread for 1 failed result)
CloudWatch Log Insights (/aws/lambda/azul-service-daniel)| filter @message like /START|END|Received|Returning/
(No logs for the throttled requests could be found in /aws/lambda/azul-service-daniel-manifest) azul-service_manifest_throttles-daniel.alarmAlarm notification sent to azul-group. |
3366611
to
15e202f
Compare
15e202f
to
2e74dd5
Compare
d071387
to
764d6af
Compare
463f80c
to
2afa30f
Compare
0762297
to
3f7f6ce
Compare
2afa30f
to
8abea24
Compare
8abea24
to
2ffb642
Compare
2ffb642
to
ae31fa4
Compare
3c6f903
to
72df1e4
Compare
33a7a95
to
118ade8
Compare
72df1e4
to
b2b7076
Compare
This reverts commit c0eabd5
b2b7076
to
7c47ea6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, only nits
|
||
waf_rate_rule_default = RateRule(name='rate_limit_default', | ||
limit=1000, | ||
period=300, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
period=300, | |
period=5 * 60, |
etc
|
||
assert 100 <= waf_rate_rule_limit <= 2_000_000_000 # mandated by AWS | ||
def __attrs_post_init__(self): | ||
assert 10 <= self.limit <= 2_000_000_000 # mandated by AWS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert 10 <= self.limit <= 2_000_000_000 # mandated by AWS | |
assert 10 <= self.limit <= 2_000_000_000, self # mandated by AWS |
# We are aiming for a global limit of 60 file downloads per 10 min. Based on | ||
# an observed average of 2.9 concurrently downloading IPs in a average | ||
# 10-minute window, the max limit we can allow is 20/10min, or 10/5min. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some small tweaks
# We are aiming for a global limit of 60 file downloads per 10 min. Based on | |
# an observed average of 2.9 concurrently downloading IPs in a average | |
# 10-minute window, the max limit we can allow is 20/10min, or 10/5min. | |
# We aim for a global limit of 60 file downloads per 10 minutes. Based on | |
# an observed average of 2.9 distinct downloading IPs per 10-minute window, | |
# the maximum per-IP request rate we can allow is 20/10min, or 10/5min. |
Connected issues: #5555
Checklist
Author
develop
issues/<GitHub handle of author>/<issue#>-<slug>
1 when the issue title describes a problem, the corresponding PR
title is
Fix:
followed by the issue titleAuthor (partiality)
p
tag to titles of partial commitspartial
or completely resolves all connected issuespartial
labelAuthor (chains)
base
or this PR is not chained to another PRchained
or is not chained to another PRAuthor (reindex, API changes)
r
tag to commit title or the changes introduced by this PR will not require reindexing of any deploymentreindex:dev
or the changes introduced by it will not require reindexing ofdev
reindex:anvildev
or the changes introduced by it will not require reindexing ofanvildev
reindex:anvilprod
or the changes introduced by it will not require reindexing ofanvilprod
reindex:prod
or the changes introduced by it will not require reindexing ofprod
reindex:partial
and its description documents the specific reindexing procedure fordev
,anvildev
,anvilprod
andprod
or requires a full reindex or carries none of the labelsreindex:dev
,reindex:anvildev
,reindex:anvilprod
andreindex:prod
API
or this PR does not modify a REST APIa
(A
) tag to commit title for backwards (in)compatible changes or this PR does not modify a REST APIapp.py
or this PR does not modify a REST APIAuthor (upgrading deployments)
make image_manifests.json
and committed the resulting changes or this PR does not modifyazul_docker_images
, or any other variables referenced in the definition of that variableu
tag to commit title or this PR does not require upgrading deploymentsupgrade
or does not require upgrading deploymentsdeploy:shared
or does not modifyimage_manifests.json
, and does not require deploying theshared
component for any other reasondeploy:gitlab
or does not require deploying thegitlab
componentdeploy:runner
or does not require deploying therunner
imageAuthor (hotfixes)
F
tag to main commit title or this PR does not include permanent fix for a temporary hotfixanvilprod
andprod
) have temporary hotfixes for any of the issues connected to this PRAuthor (before every review)
develop
, squashed old fixupsmake requirements_update
or this PR does not modifyrequirements*.txt
,common.mk
,Makefile
andDockerfile
R
tag to commit title or this PR does not modifyrequirements*.txt
reqs
or does not modifyrequirements*.txt
make integration_test
passes in personal deployment or this PR does not modify functionality that could affect the IT outcomePeer reviewer (after approval)
System administrator (after approval)
demo
orno demo
no demo
no sandbox
N reviews
label is accurateOperator (before pushing merge the commit)
reindex:…
labels andr
commit title tagno demo
develop
_select dev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unused
or this PR is not labeleddeploy:shared
_select dev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab apply
or this PR is not labeleddeploy:gitlab
_select anvildev.shared && CI_COMMIT_REF_NAME=develop make -C terraform/shared apply_keep_unused
or this PR is not labeleddeploy:shared
_select anvildev.gitlab && CI_COMMIT_REF_NAME=develop make -C terraform/gitlab apply
or this PR is not labeleddeploy:gitlab
deploy:gitlab
deploy:gitlab
System administrator
dev.gitlab
are complete or this PR is not labeleddeploy:gitlab
anvildev.gitlab
are complete or this PR is not labeleddeploy:gitlab
Operator (before pushing merge the commit)
_select dev.gitlab && make -C terraform/gitlab/runner
or this PR is not labeleddeploy:runner
_select anvildev.gitlab && make -C terraform/gitlab/runner
or this PR is not labeleddeploy:runner
sandbox
label or PR is labeledno sandbox
dev
or PR is labeledno sandbox
anvildev
or PR is labeledno sandbox
sandbox
deployment or PR is labeledno sandbox
anvilbox
deployment or PR is labeledno sandbox
sandbox
deployment or PR is labeledno sandbox
anvilbox
deployment or PR is labeledno sandbox
sandbox
or this PR does not remove catalogs or otherwise causes unreferenced indices indev
anvilbox
or this PR does not remove catalogs or otherwise causes unreferenced indices inanvildev
sandbox
or this PR is not labeledreindex:dev
anvilbox
or this PR is not labeledreindex:anvildev
sandbox
or this PR is not labeledreindex:dev
anvilbox
or this PR is not labeledreindex:anvildev
p
if the PR is also labeledpartial
Operator (chain shortening)
develop
or this PR is not labeledbase
chained
label from the blocked PR or this PR is not labeledbase
base
base
label from this PR or this PR is not labeledbase
Operator (after pushing the merge commit)
dev
anvildev
dev
dev
anvildev
anvildev
_select dev.shared && make -C terraform/shared apply
or this PR is not labeleddeploy:shared
_select anvildev.shared && make -C terraform/shared apply
or this PR is not labeleddeploy:shared
dev
anvildev
Operator (reindex)
dev
or this PR is neither labeledreindex:partial
norreindex:dev
anvildev
or this PR is neither labeledreindex:partial
norreindex:anvildev
dev
or this PR is neither labeledreindex:partial
norreindex:dev
anvildev
or this PR is neither labeledreindex:partial
norreindex:anvildev
dev
or this PR is neither labeledreindex:partial
norreindex:dev
anvildev
or this PR is neither labeledreindex:partial
norreindex:anvildev
dev
or this PR does not require reindexingdev
anvildev
or this PR does not require reindexinganvildev
dev
or this PR does not require reindexingdev
anvildev
or this PR does not require reindexinganvildev
dev
or this PR does not require reindexingdev
anvildev
or this PR does not require reindexinganvildev
Operator
deploy:shared
,deploy:gitlab
,deploy:runner
,reindex:partial
,reindex:anvilprod
andreindex:prod
labels to the next promotion PRs or this PR carries none of these labelsdeploy:shared
,deploy:gitlab
,deploy:runner
,reindex:partial
,reindex:anvilprod
andreindex:prod
labels, from the description of this PR to that of the next promotion PRs or this PR carries none of these labelsShorthand for review comments
L
line is too longW
line wrapping is wrongQ
bad quotesF
other formatting problem