feat(iroh-relay): Rate-limit client connections #2961
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This has a hardcoded rate-limiter. Need to make it configurable.
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/2961/docs/iroh/ Last updated: 2024-11-27T17:27:21Z |
Arqu
reviewed
Nov 25, 2024
This makes misusing the state harder as the interactions are enforced more on the type-level.
Arqu
reviewed
Nov 27, 2024
| @@ -18,6 +18,9 @@ use tokio_util::codec::{Decoder, Encoder}; | |||
| /// including its on-wire framing overhead) | |||
| pub const MAX_PACKET_SIZE: usize = 64 * 1024; | |||
|
|
|||
| /// The maximum frame size. | |||
| /// | |||
| /// This is also the minimum burst size that a rate-limiter has to accept. | |||
| const MAX_FRAME_SIZE: usize = 1024 * 1024; | |||
There was a problem hiding this comment.
note to @flub and future self, we should reevaluate if this is a good number
There was a problem hiding this comment.
I think we don't currently do "GSO" or "jumbo datagrams" over the relay protocol, so it's fine until we decide to start doing this I guess?
dignifiedquire
approved these changes
Nov 27, 2024
|
@Arqu demanded that the rate limiter could be disabled. So please review the new option spaghetti. |
That *is* a bit nicer to use.
dignifiedquire
approved these changes
Nov 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Add a rate limit to the incoming data from client connections.
Breaking Changes
If not configured there is now a default rate limit for incoming data from client connections: 4KiB/s steady-stream and 16MiB burst capacity.
Notes & open questions
The choice here is made to rate-limit the incoming bytes, regardless of what they are. The benefit is that the incoming stream is slowed down, pushing back to the client over the TCP connection. The downside is that someone who is rate-limited will get a fairly bad experience since all DISCO traffic is also delayed.
Only rate-limiting non-disco traffic is an option, but it would not push back on the TCP stream, which is worse as then you'd still have to swallow all the incoming traffic. Also it would be open to abuse fairly easy as the detection of disco packets is based on a magic number which could easily be spoofed.
Maybe the
RateLimitedRelayedStreamshould live instream.rsnext to theRelayedStream? Not really sure.TODO
Change checklist