Skip to content

Add env var to skip Flask rate limits#3622

Merged
gabrieldutra merged 7 commits into
masterfrom
cypress-login-rate-limit
Mar 25, 2019
Merged

Add env var to skip Flask rate limits#3622
gabrieldutra merged 7 commits into
masterfrom
cypress-login-rate-limit

Conversation

@gabrieldutra

@gabrieldutra gabrieldutra commented Mar 21, 2019 •
edited
Loading

Copy link
Copy Markdown
Member

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
Comment thread cypress/integration/dashboard/dashboard_spec.js Outdated
for (let i = 0; i < 200; i++) {
cy.login();
}
});

@gabrieldutra gabrieldutra Mar 21, 2019

Copy link
Copy Markdown
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.

Comment thread tests/__init__.py Outdated
ranbena
ranbena reviewed Mar 22, 2019
View reviewed changes
Comment thread redash/settings/__init__.py Outdated
ranbena
ranbena reviewed Mar 23, 2019
View reviewed changes
Comment thread docker-compose.yml Outdated
ranbena
ranbena reviewed Mar 23, 2019
View reviewed changes
Comment thread tests/__init__.py Outdated
os.environ['REDASH_MULTI_ORG'] = "true"

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

@ranbena ranbena Mar 23, 2019

Copy link
Copy Markdown
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

@arikfr arikfr left a comment

Copy link
Copy Markdown
Member

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

gabrieldutra commented Mar 24, 2019 •
edited
Loading

Copy link
Copy Markdown
Member Author

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

ranbena commented Mar 25, 2019

Copy link
Copy Markdown
Contributor

@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

arikfr commented Mar 25, 2019

Copy link
Copy Markdown
Member

Just resolve conflicts and it's good to go.

@gabrieldutra

gabrieldutra commented Mar 25, 2019

Copy link
Copy Markdown
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

gabrieldutra commented Mar 25, 2019

Copy link
Copy Markdown
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

@arikfr arikfr arikfr approved these changes
+1 more reviewer
@ranbena ranbena ranbena left review comments
Reviewers whose approvals may not affect merge requirements

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