-
Notifications
You must be signed in to change notification settings - Fork 325
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
RFC6062: Implement server side for TCP allocations #315
base: master
Are you sure you want to change the base?
Conversation
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).
You mean the
You suggest to rename this to Now, I think we again face the same problem as on the client side: the thingie returned from
But then we could also call this |
Codecov ReportPatch coverage:
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
Flags with carried forward coverage won't be shown. Click here to find out more.
☔ View full report in Codecov by Sentry. |
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 Perhaps a transport.Dialer can be specified in |
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.
@@ -227,7 +228,7 @@ func TestTCPClient(t *testing.T) { | |||
require.NoError(t, err) | |||
|
|||
client, err := NewClient(&ClientConfig{ | |||
Conn: NewSTUNConn(conn), |
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.
Isn't this breaking the API by removing NewSTUNConn
?
I agree that isnt a nice API. And I would much like to see it moved to pion/stun
(see #308 for a related issue).
I think we currently have a lot STUN related code in pion/turn
. E.g. PerformTransaction
which would be better located in pion/stun
. However, this is a different issue which we should not address in this PR.
@@ -67,7 +70,7 @@ func (c *PacketConnConfig) validate() error { | |||
// ListenerConfig is a single net.Listener to accept connections on. This will be used for TCP, TLS and DTLS listeners | |||
type ListenerConfig struct { | |||
Listener net.Listener | |||
|
|||
Protocol string // TCP, or TLS |
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.
Please indicate the expected string constants. E.g. "tcp", "tls" in the comment.
@@ -111,3 +112,67 @@ func TestAllocationLifeTime(t *testing.T) { | |||
assert.Nil(t, r.AllocationManager.GetAllocation(fiveTuple)) | |||
}) | |||
} | |||
|
|||
func TestTCPAllocation(t *testing.T) { | |||
t.Run("TCPAlloc", func(t *testing.T) { |
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 really need a sub test here if there is only one in TestTCPAllocation
?
@@ -19,7 +19,14 @@ var ( | |||
errFailedToCreateChannelData = errors.New("failed to create channel data from packet") | |||
errRelayAlreadyAllocatedForFiveTuple = errors.New("relay already allocated for 5-TUPLE") | |||
errUnsupportedTransportProtocol = errors.New("RequestedTransport must be UDP or TCP") | |||
errUnsupportedClientProtocol = errors.New("cannot allocate tcp if client connection transport is not TCP/TLS") |
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.
Please change lowercase tcp
to TCP
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.
Also, I prefer to use the following wording for errors: failed to ...
. E.g. in this instance failed to allocate TCP...
@@ -77,6 +77,7 @@ func main() { | |||
RelayAddress: net.ParseIP(*publicIP), | |||
Address: "0.0.0.0", | |||
}, | |||
Protocol: "tls", |
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 break this for existing users if they dont set the protocol?
a.RelayListener = listener | ||
a.RelayAddr = relayAddr | ||
} else { | ||
conn, relayAddr, err := m.allocatePacketConn("udp4", requestedPort) |
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.
Not a change you introduced, but I am wondering why we are listening on IPv4 only..
Seems like we are not supporting REQUESTED-ADDRESS-FAMILY
at all..
Another topic for another PR :D
AllocateListener: func(network string, requestedPort int) (net.Listener, net.Addr, error) { | ||
config := &net.ListenConfig{Control: func(network, address string, c syscall.RawConn) error { | ||
var err error | ||
c.Control(func(fd uintptr) { |
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.
Check error of c.Control()
config := &net.ListenConfig{Control: func(network, address string, c syscall.RawConn) error { | ||
var err error | ||
c.Control(func(fd uintptr) { | ||
err = syscall.SetsockoptInt(int(fd), syscall.SOL_SOCKET, unix.SO_REUSEADDR|unix.SO_REUSEPORT, 1) |
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.
Can you add a comment explaining the reasoning behind SO_REUSEADDR/PORT
?
@@ -102,6 +106,49 @@ func (r *RelayAddressGeneratorPortRange) AllocatePacketConn(network string, requ | |||
return nil, nil, errMaxRetriesExceeded | |||
} | |||
|
|||
// AllocatePacketConn generates a new PacketConn to receive traffic on and the IP/Port to populate the allocation response with |
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.
The comment does not match the function name
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
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 An alternative would be to let the user to specify a |
Description
Builds off of #148
Currently the PR includes a commit for the client side too, but my plan is to rebase off #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.AllocateListener
, (I don't think we needAllocateConn
anymore?),connectionHandler
, connection maps, etc to handle TCP-connection based allocation logic.stun_conn.go
toutils/stun_conn.go
andadd .Conn()
method to get underlyingnet.Conn
. This is because inhandleConnectionBindRequest
, after we receive the request and convert the TURN <-> Peer connection to a data connection, we need to retrieve the original TCP connection toio.Copy
to.STUNConn
'sReadFrom
does not seem appropriate.Usage
Start turn server (turn/examples/turn-server/tcp)
Run tcp client, (turn/examples/turn-client/tcp-alloc)
Run another tcp client, which gets the relay IP from the other client and sends its own.
Both clients should read and write a single message.
Reference issue
#143, #118