Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

chore: rate limit requests #24324

Merged
merged 4 commits into from
Aug 11, 2023
Merged

chore: rate limit requests #24324

merged 4 commits into from
Aug 11, 2023

Conversation

betodealmeida
Copy link
Member

SUMMARY

Rate limit the export dashboard endpoint.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

  • Has associated issue:
  • Required feature flags:
  • Changes UI
  • Includes DB Migration (follow approval process in SIP-59)
    • Migration is atomic, supports rollback & is backwards-compatible
    • Confirm DB migration upgrade and downgrade tested
    • Runtime estimates and downtime expectations provided
  • Introduces new feature or API
  • Removes existing feature or API

Comment on lines 268 to 274
RATELIMIT_ENABLED = True
AUTH_RATE_LIMITED = True
AUTH_RATE_LIMIT = "2 per 5 second"
RATELIMIT_APPLICATION = "50 per second"
Copy link
Member Author

Choose a reason for hiding this comment

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

@dpgaspar I know these were introduced in FAB 4.3.0. Do you have an idea of what sensible defaults would be for Superset?

Copy link
Member

Choose a reason for hiding this comment

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

Airflow implemented the same, their using auth_rate_limit = 5 per 40 second. Not sure about rate limiting the entire application this way, rate limits are better when tied to a user and state is set globally, this is possible with flask-limiter (what FAB is using for this) but needs further configuration.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, I though these configuration keys would enable flask-limiter in FAB? That's not the case? How do we do that then?

Copy link
Member

Choose a reason for hiding this comment

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

it's done exactly like that

@pull-request-size pull-request-size bot added size/S and removed size/XS labels Jul 7, 2023
Copy link
Member

@dpgaspar dpgaspar left a comment

Choose a reason for hiding this comment

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

2 per 5 seconds could be an harsh limit. Would be nice to add some docs, and reference RATELIMIT_STORAGE_URI and RATELIMIT_REQUEST_IDENTIFIER since as is flask-limiter is too simplistic. It's way better to limit an entire application by user request

@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Merging #24324 (91606e4) into master (9c54280) will decrease coverage by 0.08%.
The diff coverage is 87.50%.

❗ Current head 91606e4 differs from pull request most recent head ad89b83. Consider uploading reports for the commit ad89b83 to get more accurate results

@@            Coverage Diff             @@
##           master   #24324      +/-   ##
==========================================
- Coverage   69.00%   68.93%   -0.08%     
==========================================
  Files        1906     1906              
  Lines       74149    74153       +4     
  Branches     8211     8211              
==========================================
- Hits        51169    51114      -55     
- Misses      20856    20915      +59     
  Partials     2124     2124              
Flag Coverage Δ
hive 54.18% <50.00%> (+<0.01%) ⬆️
mysql 79.22% <87.50%> (+<0.01%) ⬆️
postgres ?
presto 54.08% <50.00%> (+<0.01%) ⬆️
python 83.21% <87.50%> (-0.17%) ⬇️
sqlite ?
unit 55.05% <50.00%> (+<0.01%) ⬆️

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

Files Changed Coverage Δ
superset/models/dashboard.py 77.90% <ø> (ø)
superset/views/dashboard/views.py 67.46% <0.00%> (ø)
superset/config.py 92.28% <100.00%> (+0.08%) ⬆️
superset/dashboards/api.py 92.60% <100.00%> (ø)
superset/utils/dashboard_import_export.py 100.00% <100.00%> (ø)

... and 18 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@betodealmeida betodealmeida merged commit 4bc4600 into master Aug 11, 2023
@michael-s-molina michael-s-molina added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Aug 11, 2023
michael-s-molina pushed a commit that referenced this pull request Aug 14, 2023
(cherry picked from commit 4bc4600)
eschutho pushed a commit that referenced this pull request Dec 2, 2023
@mistercrunch mistercrunch added 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024
@mistercrunch mistercrunch deleted the fix_dos branch March 26, 2024 18:03
vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
2.1.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/S v2.1 v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch 🍒 2.1.3 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🚢 3.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants