RFC6062: Implement server side for TCP allocations

[AndyLc]

Description

Builds off of https://github.com/pion/turn/pull/148

Currently the PR includes a commit for the client side too, but my plan is to rebase off https://github.com/pion/turn/pull/311 once it is merged. I am opening this PR for any initial feedback/review.

Main changes include:

• CreateAllocation now takes in a Protocol, either TCP or UDP, which describes the allocation <-> peer connection`.
• allocation.NewManager now takes in a Protocol, either UDP/TCP/TLS which describes the client <-> turn connection type, not the allocation.
• Add AllocateListener, (I don't think we need AllocateConn anymore?), connectionHandler, connection maps, etc to handle TCP-connection based allocation logic.
• Move stun_conn.go to utils/stun_conn.go and add .Conn() method to get underlying net.Conn. This is because in handleConnectionBindRequest, after we receive the request and convert the TURN <-> Peer connection to a data connection, we need to retrieve the original TCP connection to io.Copy to. STUNConn's ReadFrom does not seem appropriate.

Usage

Start turn server (turn/examples/turn-server/tcp)

./tcp --public-ip 127.0.0.1 --port 3478 --users andy=secret

Run tcp client, (turn/examples/turn-client/tcp-alloc)

./tcp-alloc --host 127.0.0.1 --user andy=secret -signaling

Run another tcp client, which gets the relay IP from the other client and sends its own.

./tcp-alloc --host 127.0.0.1 --user andy=secret

Both clients should read and write a single message.

Reference issue

https://github.com/pion/turn/issues/143, https://github.com/pion/turn/issues/118

[rg0now]

Thanks a lot, this is a super-useful work!

One suggestion: is it possible to rebase this PR to on top of #311? At least for me it'd be easier to see the client and the server side changes separately (I'm much more familiar with the server-side code).

Add AllocateListener, (I don't think we need AllocateConn anymore?), connectionHandler, connection maps, etc to handle TCP-connection based allocation logic.

You mean the RelayAddressGenerator interface? Currently Allocate has the following signature:

AllocateConn(network string, requestedPort int) (net.Conn, net.Addr, error)

You suggest to rename this to AllocateListener, no?

Now, I think we again face the same problem as on the client side: the thingie returned from AllocateListener should both be able to make new connections (implement our transport.Dialer interface) and accept new connections (implement net.Listener). So supposing that we create the new Relay interface that's both a Dialer and a Listener in internal/transport the new signature for AllocateListener would be something like the below:

AllocateListener(network string, requestedPort int) (transport.Relay, net.Addr, error)

But then we could also call this AllocateRelay no? Wdyt?

[codecov[bot]]

Codecov Report

Patch coverage: 51.42% and project coverage change: -2.01 :warning:

Comparison is base (521e5ad) 69.21% compared to head (e1e061f) 67.20%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #315      +/-   ##
==========================================
- Coverage   69.21%   67.20%   -2.01%     
==========================================
  Files          42       42              
  Lines        2823     3193     +370     
==========================================
+ Hits         1954     2146     +192     
- Misses        701      858     +157     
- Partials      168      189      +21     

Flag	Coverage Δ
go	67.20% <51.42%> (-2.01%)	:arrow_down:
wasm	?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
internal/allocation/five_tuple.go	100.00% <ø> (ø)
internal/server/server.go	59.01% <0.00%> (-4.15%)	:arrow_down:
relay_address_generator_none.go	21.87% <0.00%> (-13.13%)	:arrow_down:
relay_address_generator_range.go	0.00% <0.00%> (ø)
server_config.go	41.66% <ø> (ø)
utils/stun_conn.go	62.50% <0.00%> (ø)
internal/server/turn.go	51.39% <35.33%> (-7.86%)	:arrow_down:
internal/allocation/allocation_manager.go	57.57% <53.84%> (ø)
relay_address_generator_static.go	54.34% <66.66%> (+7.91%)	:arrow_up:
server.go	69.69% <70.00%> (-0.94%)	:arrow_down:
... and 1 more

... and 1 file with indirect coverage changes

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

[AndyLc]

Hi @stv0g and @rg0now,

I think this PR should now be ready for review now that the client side is merged.

I believe we want the API to support a custom Dial (when an allocation receives a Connect request and creates a connection to peer via Dial), and a custom Listen (when an Allocation is created and listens to create data connection).

The custom listen is already possible through specifying a RelayAddressGenerator's AllocateListener that I added. What do you guys think is suitable for the custom Dial?

Perhaps a transport.Dialer can be specified in turn.ServerConfig?

[rg0now]

First of all: thanks for this big chunk of code, the effort is truly appreciated!

I've made a first pass and I concluded that I will need at least another round...:-)

My major concern right now is that the TCP dialer in the allocation handler is hard-coded to net.Dialer as per the below:

func (a *Allocation) newConnection(cid proto.ConnectionID, dst string) error {
        ...
        
	dialer := &net.Dialer{ ... }

	conn, err := dialer.Dial("tcp", dst)

        ...
}

This is problematic because the caller cannot pass in their own Dialer to implement custom dialers, e.g., to report new connections to Prometheus.

I think a better solution would be to let the user to (finally!) really implement RelayAddressGenerator.AllocateConn and call manager.allocateConn every time we would create a new connection in allocation.newConnection. This unfortunately would force the user to play the SO_REUSEPORT/SO_REUSEADDR trick inside the AllocateConn implementation they provide for us (btw I think SO_REUSEPORT is enough here but adding also SO_REUSEADDR should make no harm) but that's why we would provide the prefab relay-address-generators that would cover the straightforward use cases (i.e., generate TCP connections).

An alternative would be to let the user to specify a transport.Dialer in the RelayAddressGenerator and call the Dial function on this Dialer ourselves from allocation.newConnection, or we could even unify the AllocateListener and AllocateConn functionality by passing only a transport.Net to the allocation manager, but I guess this would be an overkill (and would break the API).