feat(iroh)!: allow for limiting incoming connections on the router #3157
Conversation
|
Documentation for this PR has been generated and is available at: https://n0-computer.github.io/iroh/pr/3157/docs/iroh/ Last updated: 2025-03-14T08:27:30Z |
dignifiedquire
force-pushed
the
feat-limit-conns
branch
from
dc02471 to
54e6b66
Compare
dignifiedquire
changed the title
|
@matheus23 can check if this change is good enough to still be able to use 0-rtt from |
|
Yeah - this change makes using 0-RTT/0.5-RTT on the accepting side possible at all, since otherwise you wouldn't be able to call And all you need to do on the accepting side is call |
There was a problem hiding this comment.
I can also imagine a need for on_incoming or something like that in the future to be able to reject etc even further. But the nice thing about this design is that you can add this later without breaking backwards compatibility.
I have a minor bikeshedding concern that AccessLimit sounds too much like rate-limiting. But I don't know if there is a commonly used term that's better.
| @@ -109,10 +112,18 @@ pub struct RouterBuilder { | |||
| /// The protocol handler must then be registered on the node for an ALPN protocol with | |||
| /// [`crate::protocol::RouterBuilder::accept`]. | |||
| pub trait ProtocolHandler: Send + Sync + std::fmt::Debug + 'static { | |||
| /// Optional interception point to handle the `Connecting` state. | |||
There was a problem hiding this comment.
I'm always a fan of being extremely verbose on the docs. I would add a paragraph suggesting when you might to use this:
This enables accepting 0-RTT data from clients, among other things.
| pub struct AccessLimit<P: ProtocolHandler + Clone> { | ||
| proto: P, | ||
| #[debug("limiter")] | ||
| limiter: Arc<dyn Fn(NodeId) -> bool + Send + Sync + 'static>, |
There was a problem hiding this comment.
Not sure if this was discussed before: will we get folks upset that they can't do async stuff here? I think this itself runs in an async context.
There was a problem hiding this comment.
Of course we/they can just make an AsyncAcessLimit which is probably nicer than forcing everyone to use async.
There was a problem hiding this comment.
How would you feel if I created a PR for AsyncAccessLimit?
There was a problem hiding this comment.
Sure, why not. Don't think there'd be an objection to this.
| let remote = conn.remote_node_id()?; | ||
| let is_allowed = (this.limiter)(remote); | ||
| if !is_allowed { | ||
| conn.close(0u32.into(), b"not allowed"); |
There was a problem hiding this comment.
This needs to be documented. Probably on the struct itself. Something like:
Any refused connection will be closed with an error code of `0` and reason `not allowed`.
(I can also imagine folks wanting this configurable. But I think that's possible in a backwards-compatible way so should be fine to add later. I can also imagine a world where the function returns those values.)
There was a problem hiding this comment.
Of course we/they can just make an AsyncAcessLimit which is probably nicer than forcing everyone to use async.
yeah, this is more of an example for me
would love to hear better names.. |
Description
Changes the
ProtocolHandlertrait to allow for interceptingconnectingandconnectionstates explicitlyBreaking Changes
iroh::protocol::ProtocolHandler::acceptnow takesConnectioninstead ofConnectingNotes & open questions
The new limiter could also just be part of the test code/an example, instead of giving users an API, unclear to me
Change checklist