Skip to content

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.

@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