refactor: modularize UDP connection handling #188
Conversation
sbruens
force-pushed
the
sbruens/modularize-udp
branch
from
5707a9c to
d74d1fd
Compare
sbruens
force-pushed
the
sbruens/modularize-udp
branch
from
d74d1fd to
bcf7e53
Compare
sbruens
force-pushed
the
sbruens/modularize-udp
branch
from
32f9df0 to
7ab36b2
Compare
There was a problem hiding this comment.
UDP is complicated. We need more elaborate encapsulation to actually make the UDP handling protocol-agnostic. If you look at the TCP code, the only place we refer to the shadowsocks package is in the authenticate method. That is not the case here, where we still have references to shadowsocks in multiple places, so the PR is not accomplishing the goal of encapsulating the protocol. I also think there are some correctness issues.
Let's brainstorm more about it. We will need some concept of authenticated "PacketConn" that is tied to the NAT, but we also need to keep the NAT logic separate from the protocol.
| @@ -35,7 +36,7 @@ type UDPMetrics interface { | |||
| ipinfo.IPInfoMap | |||
|
|
|||
| // UDP metrics | |||
| AddUDPPacketFromClient(clientInfo ipinfo.IPInfo, accessKey, status string, clientProxyBytes, proxyTargetBytes int) | |||
| AddUDPPacketFromClient(clientInfo ipinfo.IPInfo, accessKey, status string, data metrics.ProxyMetrics) | |||
There was a problem hiding this comment.
This API is broader than needed. Why pass the bytes from the target?
It also adds an unnecessary dependency on github.com/Jigsaw-Code/outline-ss-server/service/metrics.
| } | ||
| }() | ||
| func (h *packetHandler) authenticate(clientConn net.PacketConn) (net.Addr, *CipherEntry, []byte, int, *onet.ConnectionError) { | ||
| cipherBuf := make([]byte, serverUDPBufferSize) |
There was a problem hiding this comment.
This is changing from a single allocation for the port, to an allocation per packet. The single allocation was intentional. Every per-packet allocation accelerates memory growth, reducing performance and causing crashes, in particular on iOS, but also Android.
Can we keep the single allocation? Having a benchmark would also help.
| } | ||
| } | ||
| func (h *packetHandler) handleConnection(clientConn net.PacketConn) (string, ipinfo.IPInfo, metrics.ProxyMetrics, *onet.ConnectionError) { | ||
| defer func() { |
There was a problem hiding this comment.
Move this to the caller for loop, so it's better encapsulated.
| return onetErr | ||
| } | ||
| unpackStart := time.Now() | ||
| cipherEntry, textData, keyErr := findAccessKeyUDP(remoteIP, textBuf, cipherBuf[:clientProxyBytes], h.ciphers) |
There was a problem hiding this comment.
You must lookup the NAT table first, before you do the cipher search.
Split the packet handler's
Handle()method into 3 distinct parts for authentication, proxy request extraction, and proxy handling. This is similar to what was done with the TCP handler in #173.There's more refactoring that can be done in future PRs, but this should make it easier to follow what's happening.