RFC6062: Implement client side for TCP allocations #311
Conversation
|
Thanks for the PR, I'm loving it! I also wanted to take up #148 myself, I'm very happy that you came forward to do that. Quick question: what is the purpose of the new My suggestion would be to remove I'll try to do a formal review later too. And thanks again for picking this PR up, it is really useful! |
Thank you for looking at the PR! I agree, specifying Protocol in the ClientConfig and having both AllocateTCP and AllocateUDP is redundant. I'll update the PR with those changes. |
I'm thinking about the semantics of the thingie that is supposed to be returned from
Wdyt? |
|
I think that makes a lot of sense. What I return right now supports just Dial() and Accept() Thinking about making it implement net.Listener, since As for net.Dialer, what are you thinking for making it a net.Dialer but also implementing net.Listener? |
This is very similar to how the
The client needs to be able to both (1) accept new connections from different peers and (2) make new connections to different peers. This is simple for UDP that is unconnected: we have a single So what I'm saying is that, for a client to accept new connections from a peer it should call While for making a new connection to a peer it has to call I'm thinking about a good name for |
|
Makes sense, I totally agree
I'm open to either, maybe |
Unfortunately Another problem tht I spotted is the hardcoded use of It would be nice to keep this generality in your implementation, but it seems there are quite a lot of places where you assume My suggestion would be the following:
I guess the server side will be OK since |
Sounds great, I will add this. One issue, currently the turn package imports client. Also for TCPRelay, you're thinking renaming |
Hmm, good catch. My gut feeling is that it should go into
I'd call the new interface that is both a Update: maybe Update2: Here is my final conclusion: let's put the |
|
As per why |
So I tried creating this interface but net.Listener's Accept() only returns a Conn and error. From net.Conn, RemoteAddr() will return the address of the turn server, not the peer. Do you have any ideas on the best way to convey from which peer we are accepting a connection? Originally I had Accept also return another address, but maybe there's a better way? |
Bad news: you'll have to implement your own Basically we need to create our own implementation of At the end we should have a default implementation for the Looking at this right now, I'm not sure we have to take all this complexity at the first step. We |
Agreed, I think getting the PRs in and then generalizing will make the code changes simpler and more iterative. I'll re-push my current changes based off your suggestions now! |
|
Hi all, I am joining the party a bit late. So I will need some more time to review the PR. Thanks @AndyLc for starting to work on it :) And @rg0now for the first review :)
We already have an interface for this defined here: https://github.com/pion/transport/blob/22a2f3b9fe3381cb4a2735420e05b64813ca3e4f/net.go#L378 Did you maybe already have a look on how we can plug this into pion/ice? |
|
You mean The definite answer is in RFC 6544 (TCP Candidates with Interactive Connectivity Establishment (ICE) but I don't have time to read it. I think the main idea is that we create two allocations on any TCP TURN server: one conventional UDP and another RFC 6062 style TCP. Then, we send two relay candidates over to the peer, one with protocol UDP and another with TCP. If we are at RFCs: here is what RFC 8835 (Transports for WebRTC) has to say on this: TURN TCP candidates, where the connection from the WebRTC endpoint's TURN server to the peer is a TCP connection, [RFC6062] MAY be supported. However, such candidates are not seen as providing any significant benefit, for the following reasons. First, use of TURN TCP candidates would only be relevant in cases where both peers are required to use TCP to establish a connection. Second, that use case is supported in a different way by both sides establishing UDP relay candidates using TURN over TCP to connect to their respective relay servers. Third, using TCP between the WebRTC endpoint's TURN server and the peer may result in more performance problems than using UDP, e.g., due to head of line blocking. |
|
Thanks @stv0g for taking a look!
If we go with this, I believe it should be pretty straightforward. In addition to calling Allocate(), we also call One concern I had with this method is, I feel we should only gather TCP relay candidates under certain conditions. For example, if we ever created an offer specifying |
The ICE agent config already allows you to specify which types of candidates should be gathered via its |
|
I took a look at the client API proposed in #143. Turns out there are some important differences.
In summary, I prefer the API of #143 for making client->peer connections since it does not require us to make the thing returned from @AndyLc: if we adopt this new client API then my advice to implement |
@rg0now Hm, is the benefit you're getting at that it would be easier to create your own implementation of net.Conn rather than transport.Dialer? I'm also utilizing a user chosen transport.Dialer in |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #311 +/- ##
==========================================
+ Coverage 68.16% 69.42% +1.26%
==========================================
Files 39 42 +3
Lines 2475 2823 +348
==========================================
+ Hits 1687 1960 +273
- Misses 657 696 +39
- Partials 131 167 +36
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Hi @AndyLc
Thanks a lot for your work. It looks great so far.
I mainly have a few nitpick comments.
This is a first round of review. I will probably have a few more comments later.
One thing, we should not forget is to add some info the README.md as well.
client.go
Outdated
| @@ -15,6 +15,7 @@ import ( | |||
| "github.com/pion/transport/v2/vnet" | |||
| "github.com/pion/turn/v2/internal/client" | |||
| "github.com/pion/turn/v2/internal/proto" | |||
| t "github.com/pion/turn/v2/internal/transport" | |||
There was a problem hiding this comment.
Please use transportx as an alias for package name collissions which have been defined within the module.
client.go
Outdated
| @@ -83,6 +86,10 @@ func NewClient(config *ClientConfig) (*Client, error) { | |||
| return nil, errNilConn | |||
| } | |||
|
|
|||
| if config.Dialer == nil { | |||
There was a problem hiding this comment.
Please avoid modifying config as the caller of NewClient might not expect this.
client_test.go
Outdated
| @@ -187,3 +187,53 @@ func TestClientNonceExpiration(t *testing.T) { | |||
| assert.NoError(t, conn.Close()) | |||
| assert.NoError(t, server.Close()) | |||
| } | |||
|
|
|||
| // Create a tcp-based allocation and verify allocation can be created | |||
There was a problem hiding this comment.
Please capitalize TCP like "TestTCPClient creates a TCP-based allocation [...]".
Also note that function comments should always have the form "FunctionName is/does XYZ".
client_test.go
Outdated
| // Create a tcp-based allocation and verify allocation can be created | ||
| func TestTCPClient(t *testing.T) { | ||
| // Setup server | ||
| tcpListener, err := net.Listen("tcp4", "0.0.0.0:3478") |
There was a problem hiding this comment.
Can we use some non-standard ports for this test here? As we can not currently perform the TCP tests using a virtual network (pion/transport/vnet) we should make sure that we are can cope with any other STUN/TURN server running in the background.
client_test.go
Outdated
| func TestTCPClient(t *testing.T) { | ||
| // Setup server | ||
| tcpListener, err := net.Listen("tcp4", "0.0.0.0:3478") | ||
| assert.NoError(t, err) |
There was a problem hiding this comment.
You probably want to use require.NoError() here and all other occurences of assert.
internal/client/tcp_conn.go
Outdated
| }, | ||
| } | ||
|
|
||
| c.log.Debugf("initial lifetime: %d seconds", int(c.lifetime().Seconds())) |
There was a problem hiding this comment.
Please start log outputs with a uppercase letter.
There was a problem hiding this comment.
Please also check other places where you go logging.
There was a problem hiding this comment.
Hm in this repo there are many instances of c.log.Debug that use lower case, and I was following that. Should we change the standard to first letter uppercase?
internal/client/tcp_conn.go
Outdated
| return err | ||
| } | ||
|
|
||
| // read exactly one STUN message, |
There was a problem hiding this comment.
Comments should start with uppercase letter
internal/client/errors.go
Outdated
| @@ -8,6 +8,7 @@ var ( | |||
| errFake = errors.New("fake error") | |||
| errTryAgain = errors.New("try again") | |||
| errClosed = errors.New("use of closed network connection") | |||
| errTCPAddrCast = errors.New("addr is not a net.TCPAddr") | |||
There was a problem hiding this comment.
Can you change the errors descriptions here to something more human friendly?
E.g. not a UDP/TCP address?
I dont think its a nice practice to have actualy Go code/types in error/log messages.
internal/client/conn.go
Outdated
| CreatePermissions(addrs ...net.Addr) error | ||
| } | ||
|
|
||
| type RelayConnContext struct { |
There was a problem hiding this comment.
I am a bit confused by the different types here Relay, RelayConn & RelayConnContext.
Can you document them? Also, shoudnt we name RelayConnContext more like RelayConnImpl?
There was a problem hiding this comment.
Or in general: why RelayX? Isnt this related to TCP connections?
There was a problem hiding this comment.
RelayX are used for both TCP and UDP connections. Both UDPConn and TCPConn use the fields in RelayConnContext. In this case, do you prefer the naming of RelayConnImpl?
In client.go, RelayConn was used to generalize between UDPConn and TCPConn. I think we can get rid of this and each client will have a UDPConn and TCPAllocation.
There was a problem hiding this comment.
I think the confusion stems from the fact that we do not have a client.UDPAllocation type.
If we would have it, client.RelayConnContext would be more appropriately named client.Allocation.
But in fact, the fields of client.RelayConnContextare basically just the clients per-allocation state.
There was a problem hiding this comment.
I am just confused why we have a per-allocation _nonce and and integrity fields here. Also the naming of ConnObserver is confusing.
I think this could need some refactoring as well.
- Rename
RelayConnContexttoAllocation - Rename
ConnObservertoClient(Just an interface withininternal/clientrepresenting the publicClienttype in order to break cyclic dependencies) - Move integrity and nonce fields from
RelayConnContextto the newClient interface - Rename
conn.gotoudp_conn.go - Move common code from
conn.gotoallocation.go
This one is on me, see: #311 (comment). Also, can you please comment on the API? It'd be nice to get your opinion on #311 (comment). |
Sure. I had to spend some time to think about it. But I am not against supporting also the ConnectionBind use case. Here is my proposal which basically combines the two approaches by moving the package turn
type Client struct { ... }
func (c *Client) AllocateTCP() (*TCPAllocation, error) { ... }
type TCPAllocation struct {
// addr is the relayed transport address
addr *net.TCPAddr
client *Client
...
}
func (a *TCPAllocation) DialTCP(network string, laddr, raddr *net.TCPAddr) (*TCPConn, error) {
conn, err := net.DialTCP(network, client.TURNServerAddr)
if err != nil {
return nil, fmt.Errorf("failed to establish connection to TURN server: %w", err)
}
return a.DialTCPWithConn(conn, network, laddr, raddr)
}
func (a *TCPAllocation) DialTCPWithConn(conn net.Conn, network string, laddr, raddr *net.TCPAddr) (*TCPConn, error) {
cid := ...
conn := &TCPConn{
TCPConn: conn,
ConnectionID: cid
allocation: a,
}
if err := a.BindConnection(conn); err != nil {
return nil, fmt.Errorf("failed to bind connection: %w", err)
}
return conn, nil
}
// AcceptTCP accepts the next incoming call and returns the new connection.
func (a *TCPAllocation) AcceptTCP() (*TCPConn, error) {
conn, err := net.DialTCP(network, client.TURNServerAddr)
if err != nil {
return nil, fmt.Errorf("failed to establish connection to TURN server: %w", err)
}
return a.AcceptTCPWithConn(conn)
}
func (a *TCPAllocation) AcceptTCPWithConn(conn net.Conn) (*TCPConn, error) {
cid := ...
conn := &TCPConn{
TCPConn: conn,
ConnectionID: cid
allocation: a,
}
if err := a.BindConnection(conn); err != nil {
return nil, fmt.Errorf("failed to bind connection: %w", err)
}
return conn, nil
}
// Addr returns the allocations relayed address.
func (a *TCPAllocation) Addr() net.Addr { ... }
// Close releases the allocation
// Any blocked Accept operations will be unblocked and return errors.
// Any opened connection via Dial/Accept will be closed.
func (a *TCPAllocation) Close() error { ... }
// SetDeadline sets the deadline associated with the listener.
// A zero time value disables the deadline.
func (a *TCPAllocation) SetDeadline(t time.Time) error { ... }
// TCPConn wraps a net.TCPConn and returns the allocations relayed
// transport address in response to TCPConn.LocalAddress()
type TCPConn struct {
*net.TCPConn
// ConnectionID uniquely identifies a peer data connection.
ConnectionID uint32
allocation *TCPAllocation
}
func (c *TCPConn) LocalAddress() net.Addr {
return c.allocation.Addr()
}
var _ transport.TCPListener = (*TCPAllocation)(nil) // Includes type check for net.Listener
var _ transport.TCPConn = (*TCPConn)(nil) // Includes type check for net.Conn
var _ transport.Dialer = (*TCPAllocation)(nil)
// TODO: We might want to add a transport.TCPDialer interface
// var _ transport.TCPDialer = (*TCPAllocation)(nil) |
And split UDP related code to udp_conn.go
|
@AndyLc thanks for the repo access. I've addressed my remaining comments myself and also rebased your Please reset your local branch as it gets messed up otherwise due to the rebase. Lets, see what the CI says.. |
|
@stv0g Thanks for making edits! I saw that some tests were still failing, so I pushed another commit to address those, mainly fixing golangci-lint issues and adding more unit tests. |
|
Hi @AndyLc :) Great! I think we are getting close merging this into master 🥳 I currently, see only one thing missing: We should add support for |
|
@stv0g Sounds good, I attempted to add vnet, let me know what you think! |
This allows us to keep Client.Net() outside of the public API.
|
Hi @AndyLc, cool :) I've found that we had two implementations of a client mock ( I also simplified this mock client by moving more of the static configuration attributes to |
|
@stv0g Looks like I don't have access to merge the PR |
Description
Builds off of #148
Implements client side support for TCP allocations.
I'm also planning on adding support to server side
For the TCP allocation implementation, I tried follow the pattern of creating a client, calling allocate, and then using that allocation for communication. I thought that instead of calling WriteTo() and ReadFrom() on a TCP allocation, it would be better to Accept and Dial, and return a net.Conn for the end user to use, so I ended up having to do some refactoring.
Any feedback is welcome!
Usage:
Start coturn or some other turn server
./bin/turnserver --lt-cred-mech -u andy:secret -r pion.ly --no-cli ./examples/ca/turn_server_cert.pem --pkey ./examples/ca/turn_server_pkey.pem --allow-loopback-peersRun tcp client, also starting up a simple tcp server to communicate relay IPs.
./tcp_alloc --host 127.0.0.1 --user andy=secret -serverRun 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=secretBoth clients should read and write a single message.
Reference issue
#143, #118