-
Notifications
You must be signed in to change notification settings - Fork 25
UDP proxy support added #53
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
base: main
Are you sure you want to change the base?
Conversation
…mutex. CTRL-C signal listening added to connect_udp.
I want to use dumbpipe to punch holes for sending syslog and I'd rather do syslog with UDP than TCP in this scenario (don't mind if I lose logs; dont want the TCP backpressure issues) Having UDP support would be great, so I hope this gets merged. |
@@ -15,6 +15,7 @@ rust-version = "1.81" | |||
|
|||
[dependencies] | |||
anyhow = "1.0.75" | |||
bytes = "1.10.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no big deal since we heavily use bytes inside iroh.
Thanks for the PR, and sorry for not looking at it earlier. In general, looks good. The code is nicely separated from the rest, so even if it makes dumbpipe quite a bit bigger it would be easy enough for somebody to strip it out if they want to use it as a starting point for their own work. I have one concern however. You are using the datagram extension. But as far as I can tell from reading the RFC, the size of a datagram is 1. limited by a transport parameter to typically 64 KiB, but 2. limited further to a MTU. https://datatracker.ietf.org/doc/html/rfc9221#section-5
So this whole thing will only work for small UDP packets, and I am not quite sure what the behaviour will be for larger packets. Also, I don't know what you are trying to use this for. It might be that it works well for certain use cases but completely falls flat for others. Another, somewhat weird, way to solve this would be to just 1 QUIC stream per packet. Not sure. Maybe @flub can enlighten us. |
There's Realistically any application which uses datagrams has to do it's own MTU discovery, or use a small maximum size like DNS does. So my gut reaction is "shrug, this is probably fine and is how the internet works". |
|
||
// send the handshake unless we are using a custom alpn | ||
if handshake { | ||
connection.send_datagram(Bytes::from_static(&dumbpipe::HANDSHAKE))?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess failing here on TooLarge
is not a good call, because that is a normal thing that might happen. You just drop such datagrams and move on I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, sorry. Wrong place. Here it probably is reasonable to error out. It's on the forwarding paths that you need to handle this.
Any feedback is highly appreciated since this is my hello-world Rust and Iroh project, I was learning and implementing concepts as I discover them.
Needs some updates on README.md to state udp proxy is supported and how it's used before merging the PR.