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

v1.18: Connection rate limiting (backport of #948) #1362

Closed
wants to merge 2 commits into from

Conversation

mergify[bot]
Copy link

@mergify mergify bot commented May 15, 2024

Problem

A client can be abusive and create the connections too fast to over load the server. Even we have per connection limit, it involves more heavy operations like taking lock in the connection table and evicting other connections. Also, a lot of different clients can collectively create too many connections too fast to overwhelm the server. This is observed especially in around the the time when the node becomes a leader.

Summary of Changes

Introduce connection rate limiter.

Limit connection rates from a single IP to 8/minutes

Limit the global connection rate to 2500/second -- 2500 is estimated from the default connection cache table size which is generous.

Fixes #


This is an automatic backport of pull request #948 done by [Mergify](https://mergify.com).

* use rate limit on connectings

use rate limit on connectings; missing file

* Change connection rate limit to 8/min instead of 4/s

* Addressed some feedback from Trent

* removed some comments

* fix test failures which are opening connections more frequently

* moved the flag up

* turn off rate limiting to debug CI

* Fix CI test failures

* differentiate of the two throttling cases in stats: across connections or per ip addr

* fmt issues

* Addressed some feedback from Trent

* Added unit tests

Cleanup connection cache rate limiter if exceeding certain threshold

missing files

CONNECITON_RATE_LIMITER_CLEANUP_THRESHOLD to 100_000

clippy issue

clippy issue

sort crates

* revert Cargo.lock changes

* Addressed some feedback from Pankaj

(cherry picked from commit f54c120)

# Conflicts:
#	streamer/src/quic.rs
#	validator/src/cli.rs
@mergify mergify bot added the conflicts label May 15, 2024
@mergify mergify bot requested a review from a team as a code owner May 15, 2024 02:26
Copy link
Author

mergify bot commented May 15, 2024

Cherry-pick of f54c120 has failed:

On branch mergify/bp/v1.18/pr-948
Your branch is up to date with 'origin/v1.18'.

You are currently cherry-picking commit f54c120450.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   Cargo.lock
	modified:   bench-tps/src/cli.rs
	modified:   client/src/connection_cache.rs
	modified:   core/src/tpu.rs
	modified:   core/src/validator.rs
	modified:   local-cluster/src/local_cluster.rs
	modified:   programs/sbf/Cargo.lock
	modified:   quic-client/tests/quic_client.rs
	modified:   streamer/Cargo.toml
	new file:   streamer/src/nonblocking/connection_rate_limiter.rs
	new file:   streamer/src/nonblocking/keyed_rate_limiter.rs
	modified:   streamer/src/nonblocking/mod.rs
	modified:   streamer/src/nonblocking/quic.rs
	new file:   streamer/src/nonblocking/rate_limiter.rs
	modified:   test-validator/src/lib.rs
	modified:   validator/src/main.rs

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   streamer/src/quic.rs
	both modified:   validator/src/cli.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

@mergify mergify bot requested a review from a team as a code owner May 15, 2024 02:26
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 86.04651% with 24 lines in your changes are missing coverage. Please review.

Project coverage is 81.6%. Comparing base (4c46f3b) to head (1f0a377).

Additional details and impacted files
@@            Coverage Diff            @@
##            v1.18    #1362     +/-   ##
=========================================
- Coverage    81.6%    81.6%   -0.1%     
=========================================
  Files         827      830      +3     
  Lines      225468   225639    +171     
=========================================
+ Hits       184147   184236     +89     
- Misses      41321    41403     +82     

@lijunwangs
Copy link

Hi @pgarg66 @t-nelson @sakridge , can you review this BP request?

@willhickey
Copy link

Closing this as stale. If you think this is relevant please re-open and let @anza-xyz/backport-reviewers know why it is important

@willhickey willhickey closed this Sep 4, 2024
@mergify mergify bot deleted the mergify/bp/v1.18/pr-948 branch September 4, 2024 16:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants