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

Add application-level rate limits for crate updates, yanking and unyanking #6875

Merged
merged 5 commits into from
Aug 1, 2023

Conversation

pietroalbini
Copy link
Member

@pietroalbini pietroalbini commented Jul 25, 2023

This PR follows up on #6872 by adding support for multiple kinds of rate limits in RateLimiter and adding a limit for yanking and unyanking, which would've prevented last Friday's outage.

As I was already changing this code, I also moved the rate limit for publishing existing crates from nginx into the application. This makes the limit more precise (as it's not per-server anymore, potentially allowing up to 4x the limit), and allows overriding the limit just for a subset of users.

With this PR, the rate limits are:

Action Burst amount Refill time
Publish new crates 5 1 every 10 minutes
Publish updates to existing crates 30 1 every minute
Yanking or unyanking 100 1 every minute

Note that the environment variables also changed:

  • From WEB_NEW_PKG_RATE_LIMIT_RATE_MINUTES to RATE_LIMITER_PUBLISH_NEW_RATE_SECONDS
  • From WEB_NEW_PKG_RATE_LIMIT_BURST to RATE_LIMITER_PUBLISH_NEW_BURST

@Turbo87 Turbo87 added C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works A-backend ⚙️ labels Jul 26, 2023
@Turbo87
Copy link
Member

Turbo87 commented Jul 26, 2023

I was going to rebase the PR for you, but apparently you didn't check the "Maintainers are allowed to edit this pull request." checkbox... 😅

Pushing to github.com:ferrous-systems/crates.io.git
ERROR: Permission to ferrous-systems/crates.io.git denied to Turbo87.
fatal: Could not read from remote repository.
Please make sure you have the correct access rights
and the repository exists.

@pietroalbini pietroalbini force-pushed the pa-rate-limits-part2 branch from c138d0c to 6635d95 Compare July 26, 2023 11:03
@pietroalbini
Copy link
Member Author

Rebased.

@bors
Copy link
Contributor

bors commented Jul 27, 2023

☔ The latest upstream changes (presumably #6892) made this pull request unmergeable. Please resolve the merge conflicts.

src/config/server.rs Show resolved Hide resolved
src/rate_limiter.rs Outdated Show resolved Hide resolved
src/tests/krate/publish.rs Outdated Show resolved Hide resolved
src/rate_limiter.rs Outdated Show resolved Hide resolved
@pietroalbini pietroalbini force-pushed the pa-rate-limits-part2 branch from 6635d95 to 68e8ee3 Compare July 31, 2023 10:47
@pietroalbini pietroalbini force-pushed the pa-rate-limits-part2 branch from 68e8ee3 to 7ef5ac8 Compare July 31, 2023 11:17
@pietroalbini
Copy link
Member Author

Addressed all review comments.

@Turbo87 Turbo87 merged commit 281a573 into rust-lang:main Aug 1, 2023
@Turbo87
Copy link
Member

Turbo87 commented Aug 1, 2023

thanks! :)

@pietroalbini pietroalbini deleted the pa-rate-limits-part2 branch August 1, 2023 07:25
@Turbo87
Copy link
Member

Turbo87 commented Aug 1, 2023

thread 'krate::publish::publish_new_crate_rate_limited' panicked at 'assertion failed: `(left == right)`
  left: `429`,
 right: `200`', src/tests/krate/publish.rs:1024:10

it looks like one of the tests is a little flaky :-/

Turbo87 added a commit to Turbo87/crates.io that referenced this pull request Oct 17, 2023
The configuration for this nginx module was original added in rust-lang#1596, in combination with an IP-based `limit_req` rate limit. The rate limit was lately moved to the application layer (see rust-lang#6875).

Since we no longer use the `limit_req` rate limit in the nginx config, it looks like we don't need the `real_ip` config anymore either.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-backend ⚙️ C-enhancement ✨ Category: Adding new behavior or a change to the way an existing feature works
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants