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.

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

Move new versions rate limits to the application #3790

Merged
merged 7 commits into from
Aug 11, 2021

Conversation

pietroalbini
Copy link
Member

This PR refactors the rate limiting code to support more kinds of rate limits, and moves the new versions rate limit from nginx to the application. See #3780 for the rationale of this change.

  • The rate limiting for new publishes of existing crates is now tracked per-user instead of per-IP, with the ability to add overrides similar to the rate limiting for new crates. The limit is still the usual 1 version/min with a burst of 30, but in practice this could lower the rate limiting, as before the limit was between 30 and 60 depending how lucky users were with the load balancing.

  • PublishRateLimit is now called RateLimiter, and for every operation it requires a LimitedAction. This allows different rate limits to exist for LimitedAction::PublishNew and LimitedAction::PublishExisting, and adding new kinds of rate limits only requires adding a new enum variant.

  • The environment variables to override the default rate limits changed to RATE_LIMITING_{KIND}_RATE_SECONDS and RATE_LIMITING_{KIND}_BURST.

There are two things that will be done in followup PRs:

  • Remove the default for the action column.
  • Rename the publish_limit_buckets to rate_limit_buckets and publish_rate_overrides to rate_limit_overrides

@rust-highfive
Copy link

r? @jtgeibel

(rust-highfive has picked a reviewer for you, use r? to override)

@pietroalbini
Copy link
Member Author

Sorry for the big PR, I tried making it easy to review the PR commit-by-commit.

@pietroalbini pietroalbini force-pushed the pa-rate-limit-updates branch from e8d382b to 1a5b0e8 Compare July 18, 2021 21:07
@Turbo87 Turbo87 added A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear labels Jul 19, 2021
@jtgeibel
Copy link
Member

Looks great!

@bors r+

@bors
Copy link
Contributor

bors commented Aug 11, 2021

📌 Commit 1a5b0e8 has been approved by jtgeibel

@bors
Copy link
Contributor

bors commented Aug 11, 2021

⌛ Testing commit 1a5b0e8 with merge 1097171...

@bors
Copy link
Contributor

bors commented Aug 11, 2021

☀️ Test successful - checks-actions
Approved by: jtgeibel
Pushing 1097171 to master...

@bors bors merged commit 1097171 into rust-lang:master Aug 11, 2021
@pietroalbini pietroalbini deleted the pa-rate-limit-updates branch August 14, 2021 20:37
bors added a commit that referenced this pull request Aug 22, 2021
…pietroalbini

Revert "Move new versions rate limits to the application"

Reverts #3790

This has been causing issues in production 😢

see https://sentry.io/organizations/rust-lang/issues/2594868566/
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-internal 🔧 Category: Nonessential work that would make the codebase more consistent or clear
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants