Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

Remove bespoke throttle exemption classes and instead create a separate oauth "unlimited" scope #585

Closed
1 task
sarayourfriend opened this issue Mar 24, 2022 · 3 comments · Fixed by #792
Closed
1 task
Assignees
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟨 priority: medium Not blocking but should be addressed soon

Comments

@sarayourfriend
Copy link
Contributor

Problem

I came up with the exemption class solution out of ignorance of the actual solution, which was to create a new OAuth2 scope to apply to tokens that would use an unlimited throttle.

Description

Remove the exemption classes scheme and implement the unlimited scope as a new oauth scope and apply the scope to the relevant API keys.

Note: first apply the scope the API keys in production that need it then deploy the change to add the new scope. Because of the existing exemption class approach, the scope is ignored on the key so it can be safely changed without affecting the existing throttling behavior.

Alternatives

Keep the extra code (but why maintain that when there's an existing solution for it?)

Implementation

  • 🙋 I would be interested in implementing this feature.
@sarayourfriend sarayourfriend added 🟨 priority: medium Not blocking but should be addressed soon 💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users labels Mar 24, 2022
@sarayourfriend
Copy link
Contributor Author

In addition to this we should update the production frontend to use a client token as well. Then we can scrap the entire custom solution.

@sarayourfriend
Copy link
Contributor Author

I've just realized that the "enhanced" token scope are already completely exempt from rate limiting 🤦

redis_set_name = "client-id-allowlist"

You can test this locally by doing the following:

  1. Enable throttling locally by setting DISABLE_GLOBAL_THROTTLING to False
  2. Confirm that you get throttled if you make requests too fast locally now (it's easy to check anonymously because the rate is very low for anonymous requesters)
  3. Create an oauth application (you can follow the instructions in the API docs at http://localhost:8000/v1/#section/Register-and-Authenticate, just make sure to change the URL to localhost instead of production API)
  4. Using Django admin, set the ThrottledApplication that you just created to have the "enhanced" rate limit model.
  5. Now make tons of requests using that token (for example, by using the tools from this PR: Add basic API load testing script openverse#257) and confirm that you never get limited.

Can someone else @WordPress/openverse-maintainers confirm that what I'm seeing is correct? If so then I will open a PR to remove the throttle exemption classes.

@sarayourfriend
Copy link
Contributor Author

Actually, please ignore that previous comment 🤦 I'm completely wrong. Enhanced tokens are still throttled, they're just throttled high enough that the load testing script doesn't hit the limit in its current configuration. If you change it to make 400 requests, it will exceed the 200/min throttle rate.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
💻 aspect: code Concerns the software code in the repository 🧰 goal: internal improvement Improvement that benefits maintainers, not users 🟨 priority: medium Not blocking but should be addressed soon
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant