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.

Sign up for GitHub

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

Jump to bottom

Add env var to skip Flask rate limits #3622

Merged
merged 7 commits into from
Mar 25, 2019
Merged

Add env var to skip Flask rate limits #3622

merged 7 commits into from
Mar 25, 2019

Conversation

gabrieldutra
Copy link
Member

@gabrieldutra gabrieldutra commented Mar 21, 2019 •
edited
Loading

What type of PR is this? (check all applicable)

  • Other

Description

This was supposed to be a Draft PR, though I pressed the enter key accidentally 😕

Well, the idea here is to suggest a fix to "Too Many Requests" error in Cypress (this).

This adds REDASH_SKIP_LOGIN_RATE_LIMIT env var (not sure if this will be the best name here) and uses it in the docker-compose's for dev and for cypress.

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

@ghost ghost assigned gabrieldutra Mar 21, 2019
@ghost ghost added the in progress label Mar 21, 2019
@gabrieldutra gabrieldutra changed the title Skip login rate limit for Dev environment and Cypress tests WIP: Skip login rate limit for Dev environment and Cypress tests Mar 21, 2019
gabrieldutra
gabrieldutra commented Mar 21, 2019
View reviewed changes
cypress/integration/dashboard/dashboard_spec.js Outdated
for (let i = 0; i < 200; i++) {
cy.login();
}
});
Copy link
Member Author

Choose a reason for hiding this comment

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

Just to test if it works for Cypress.

tests/__init__.py Outdated Show resolved Hide resolved
ranbena
ranbena reviewed Mar 23, 2019
View reviewed changes
docker-compose.yml Outdated Show resolved Hide resolved
ranbena
ranbena reviewed Mar 23, 2019
View reviewed changes
tests/__init__.py Outdated
@@ -13,6 +13,9 @@
os.environ['REDASH_GOOGLE_CLIENT_SECRET'] = "dummy"
os.environ['REDASH_MULTI_ORG'] = "true"

# Disable login rate limit skip
os.environ['REDASH_THROTTLE_LOGIN_ENABLED'] = "true"
Copy link
Contributor

Choose a reason for hiding this comment

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

If the purpose of the current test is to:
a) test the feature, then I think this should be set in/before the test.
b) make sure prod has this feature turned on, then this should not be set at all.

@ranbena ranbena mentioned this pull request Mar 23, 2019
Merged
1 task
arikfr
arikfr requested changes Mar 24, 2019
View reviewed changes
Copy link
Member

@arikfr arikfr left a comment

Choose a reason for hiding this comment

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

As we might have other rate limits in the future, I think the setting shouldn't be specific but a general kill switch for all limits. It just so happens Flask-Limiter already has one: setting RATELIMIT_ENABLED in Flask config to False will disable it.

All you need to do is to add to redash.settings:

RATELIMIT_ENABLED = parse_boolean(os.environ....)

As we already load all the values from redash.settings to app.config. The other change is to override this setting in the rate limit change.

@gabrieldutra
Copy link
Member Author

gabrieldutra commented Mar 24, 2019 •
edited
Loading

Thanks @arikfr, I've modified this to disable all rate limits instead of only login. Two questions to proceed:

  • "The other change is to override this setting in the rate limit change.". I'm not sure I understood this one, you mean force RATELIMIT_ENABLED to be true when THROTTLE_LOGIN_PATTERN env var is set?
  • Bringing up Ran's comment about tests, I'm currently just setting rate limits enabled prior to all tests. Should I cover all cases instead? (My concerns are: - existing test purpose; - it's a bigger context now that all rate limits can be disabled)

@ranbena
Copy link
Contributor

ranbena commented Mar 25, 2019

@gabrieldutra @Arik plz let's give this one priority as some of my pending PRs depend on it.

@arikfr arikfr changed the title WIP: Skip login rate limit for Dev environment and Cypress tests Skip login rate limit for Dev environment and Cypress tests Mar 25, 2019
@arikfr
Copy link
Member

arikfr commented Mar 25, 2019

Just resolve conflicts and it's good to go.

@gabrieldutra
Copy link
Member Author

Just resolve conflicts and it's good to go.

Will do in a min 🙂

@gabrieldutra gabrieldutra changed the title Skip login rate limit for Dev environment and Cypress tests Add env var to skip Flask rate limits Mar 25, 2019
@gabrieldutra
Copy link
Member Author

Done, just review the name (almost sure this is what I did 😅) and once CI is finished it's good to go

@gabrieldutra gabrieldutra merged commit 70d4c72 into master Mar 25, 2019
@gabrieldutra gabrieldutra deleted the cypress-login-rate-limit branch March 25, 2019 16:15
harveyrendell pushed a commit to pushpay/redash that referenced this pull request Nov 14, 2019
@gabrieldutra @harveyrendell
Add env var to skip Flask rate limits (getredash#3622)
772d5a5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ranbena ranbena ranbena left review comments

@arikfr arikfr arikfr approved these changes

Assignees

@gabrieldutra gabrieldutra

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gabrieldutra @ranbena @arikfr