-
Notifications
You must be signed in to change notification settings - Fork 281
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
Support UDP in wsl-proxy #7618
Support UDP in wsl-proxy #7618
Conversation
5040319
to
6f2b119
Compare
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.
Should there be new tests to check that this works? It looks like we have an existing test for TCP?
) | ||
|
||
const ( | ||
defaultLogPath = "/var/log/wsl-proxy.log" | ||
defaultSocket = "/run/wsl-proxy.sock" | ||
bridgeIPAddr = "192.168.143.1" | ||
// Set UDP buffer size to 8 MB | ||
defaultUDPBufferSize = 8388608 // 8 MB in bytes |
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.
Would something like 8 * 1024 * 1024
be clearer?
) | ||
|
||
func main() { | ||
flag.BoolVar(&debug, "debug", false, "enable additional debugging.") | ||
flag.StringVar(&logFile, "logfile", defaultLogPath, "path to the logfile for wsl-proxy process") | ||
flag.StringVar(&socketFile, "socketFile", defaultSocket, "path to the .sock file for UNIX socket") | ||
flag.StringVar(&upstreamAddr, "upstreamAddress", bridgeIPAddr, "IP address of the upstream server to forward to") | ||
flag.IntVar(&udpBuffer, "udpBuffer", defaultUDPBufferSize, "max buffer size for the UDP socket io") |
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.
flag.IntVar(&udpBuffer, "udpBuffer", defaultUDPBufferSize, "max buffer size for the UDP socket io") | |
flag.IntVar(&udpBuffer, "udpBuffer", defaultUDPBufferSize, "max buffer size in bytes for UDP socket I/O") |
I started adding in bytes and then couldn't help myself LOL…
if gvisorTypes.TransportProtocol(portProto.Proto()) == gvisorTypes.TCP { | ||
p.handleTCP(portBindings, pm.Remove) | ||
} | ||
if gvisorTypes.TransportProtocol(portProto.Proto()) == gvisorTypes.UDP { |
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.
else if
? Or even a switch(….Proto())
?
if remove { | ||
p.udpConnMutex.Lock() | ||
if udpConn, exist := p.activeUDPConns[port]; exist { | ||
logrus.Debugf("closing UDPConn for port: %d", port) |
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.
Debugf
with a mutex held seems kind of heavy?
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 agree, however, since it's only going to output when debugging is on, do you still think that would be an issue? But, regardless I'll move it.
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.
There's also a potential Errorf
later. But yeah, it's probably fine.
continue | ||
} | ||
|
||
p.udpConnMutex.Lock() |
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.
Do we need the mutex held for all of this, or just the one line after?
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.
Agreed, unnecessary defer.
for { | ||
b := make([]byte, p.config.UDPBufferSize) | ||
n, addr, err := sourceConn.ReadFromUDP(b) | ||
if err != nil { |
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.
Per PacketConn
documentation:
Callers should always process the
n > 0
bytes returned before considering the errorerr
.
a499776
to
932e27b
Compare
The wsl-proxy previously only supported TCP connections. With this change, it now enables the listening and handling of UDP packets in the proxy. Signed-off-by: Nino Kodabande <[email protected]>
Signed-off-by: Nino Kodabande <[email protected]>
932e27b
to
3b49fde
Compare
for { | ||
udpMappings = portProxy.UDPPortMappings() | ||
if len(udpMappings) > 0 { | ||
close(ready) |
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 don't understand why this needs to be a separate goroutine, instead of just a normal loop, but 🤷
for len(portProxy.UDPPortMappings()) == 0 {
time.Sleep(100 * time.Millisecond)
}
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.
You are correct, my original approach was polling for Port (before implementing UDPPortMappings
) was doing things in a goroutine, but since we have the function, there is no need for a goroutine, removed.
Signed-off-by: Nino Kodabande <[email protected]>
Signed-off-by: Nino Kodabande <[email protected]>
3b49fde
to
6d9ab24
Compare
The wsl-proxy previously only supported TCP connections. With this change, it now enables the listening and handling of UDP packets in the proxy.
Fixes: #6821
Also, addresses: #7473 and #4873