Skip to content
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

WIP: Expand UDP Single Port mode (UDPMux) to ServerReflexive and Relay #403

Closed
wants to merge 12 commits into from
Closed

WIP: Expand UDP Single Port mode (UDPMux) to ServerReflexive and Relay #403

wants to merge 12 commits into from

Conversation

mlsmaycon
Copy link

Description

This PR adds support to ServerReflexive and Relay candidate types.

  • check if a.udpMux exists and calls a.udpMux.GetConn(a.localUfrag) to create conn
  • check conn is udpMux before closing it

Reference issue

Fixes #400

@mlsmaycon
Copy link
Author

I've tested and when using higher number agents with type Relay and udpMux as I've implemented, the connections fail and I am a bit stuck here. The errors and warnings I see are:

ice INFO: 2021/12/12 15:07:17 Setting new connection state: Checking
ice WARNING: 2021/12/12 15:07:17 pingAllCandidates called with no candidate pairs. Connection is not possible yet.
ice WARNING: 2021/12/12 15:07:17 Conn is not allocated (could not get server reflexive address udp4 stun:stun.wiretrustee.com:5555: failed to get XOR-MAPPED-ADDRESS response: attribute not found
)
ice WARNING: 2021/12/12 15:07:17 pingAllCandidates called with no candidate pairs. Connection is not possible yet.
ice WARNING: 2021/12/12 15:07:17 discard message from (116.203.205.21:5555), attribute not found
ice WARNING: 2021/12/12 15:07:17 pingAllCandidates called with no candidate pairs. Connection is not possible yet.
ice WARNING: 2021/12/12 15:07:17 discard message from (135.181.92.77:5555), attribute not found
ice WARNING: 2021/12/12 15:07:17 Conn is not allocated (could not get server reflexive address udp4 turn:turn.wiretrustee.com:5555?transport=udp: failed to get XOR-MAPPED-ADDRESS response: attribute not found
)
ice WARNING: 2021/12/12 15:07:21 Discarded message from 192.168.64.1:50505, not a valid remote candidate
ice WARNING: 2021/12/12 15:07:21 Discarded message from 192.168.0.113:50505, not a valid remote candidate
ice WARNING: 2021/12/12 15:07:21 Discarded message from 192.168.0.113:50505, not a valid remote candidate
ice WARNING: 2021/12/12 15:07:21 Discarded message from 192.168.64.1:50505, not a valid remote candidate
ice WARNING: 2021/12/12 15:07:21 Conn is not allocated (Failed to allocate on turn.Client turn.wiretrustee.com:5555 attribute not found
)
ice WARNING: 2021/12/12 15:07:21 Discarded message from 192.168.64.1:50505, not a valid remote candidate
ice WARNING: 2021/12/12 15:07:22 Conn is not allocated (Failed to allocate on turn.Client turn.wiretrustee.com:5555 all retransmissions failed for Sj6djaaKyPbxnBT/
)
ice WARNING: 2021/12/12 15:07:22 Discarded message from 192.168.0.113:50505, not a valid remote candidate
ice WARNING: 2021/12/12 15:07:22 Discarded message from 192.168.64.1:50505, not a valid remote candidate
ice WARNING: 2021/12/12 15:07:22 Conn is not allocated (Failed to allocate on turn.Client turn.wiretrustee.com:5555 all retransmissions failed for 1jqqZmYYs4LWvIWL
)

@mlsmaycon mlsmaycon changed the title Expand UDP Single Port mode (UDPMux) to ServerReflexive and Relay WIP: Expand UDP Single Port mode (UDPMux) to ServerReflexive and Relay Dec 12, 2021
@mlsmaycon
Copy link
Author

When using multiple agents sharing the same port, packets are being dropped or routed to the wrong agents, it seems like I will have to work on UDPMuxDefault address and connection mappings. This is kind of tricky as STUN Binding Response doesn't carry much information that would allow for good mapping. The same goes for TURN protocol.

@@ -1,3 +1,4 @@
//go:build !js
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am a bit confused about this build tag comment here. Whats the purpose of the // +build comment?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to keep compatibility with older versions as the new directive was introduced in Go v1.7

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlsmaycon Thanks :)

@stv0g
Copy link
Member

stv0g commented Jan 12, 2022

Hi @mlsmaycon,

thanks for your great work on this :)

When using multiple agents sharing the same port, packets are being dropped or routed to the wrong agents, it seems like I will have to work on UDPMuxDefault address and connection mappings. This is kind of tricky as STUN Binding Response doesn't carry much information that would allow for good mapping. The same goes for TURN protocol.

I guess we could simply use the STUN transaction ID to facilitate the mapping?

As TURN just uses STUN methods, the packets send for TURN bindings also carry a STUN transaction ID.
So this should also be possible with TURN, I guess?

@mlsmaycon
Copy link
Author

Hi @mlsmaycon,

thanks for your great work on this :)

When using multiple agents sharing the same port, packets are being dropped or routed to the wrong agents, it seems like I will have to work on UDPMuxDefault address and connection mappings. This is kind of tricky as STUN Binding Response doesn't carry much information that would allow for good mapping. The same goes for TURN protocol.

I guess we could simply use the STUN transaction ID to facilitate the mapping?

As TURN just uses STUN methods, the packets send for TURN bindings also carry a STUN transaction ID. So this should also be possible with TURN, I guess?

Thanks, @stv0g for your comments.

I've tested a bit with Transactions ID and it could help us address the above issue but not all.

I think we would need to work on the pion/turn package as well because packets sent from it wouldn't be tracked on ICE and a response from turn ends up going to the last udpMuxedConn that mapped the TURN server's IP.

Also, mapping Transactions ID will require constant clean-up of the mapped IDs that weren't used (happens on check packets). Another point that I missed when creating the PR, was that using a single ufrag for creating the udpMuxedConn might also lead to responses being handled by the wrong candidates.

@braginini is working on this PR as well.

@braginini
Copy link
Contributor

braginini commented Jan 12, 2022

@stv0g @mlsmaycon
I think that the simplest approach would be to have a single UDPMux per candidate type - UDPMux for Host, UDPMuxSrflx for Server Reflexive, and UDPMuxRelay.
This could be the first step. I actually made it work more or less for Server Reflexive candidates - check here https://github.com/wiretrustee/ice/tree/singleport
There are a few issues tho when running on many peers simultaneously, plus no tests really. Still WIP.

Afterward, we could merge them.
Could be also UDPMux for host and srflx and UDPMuxrelay for the relay candidates

@stv0g
Copy link
Member

stv0g commented Feb 8, 2022

@braginini Having dedicated muxes per candidate type would force us to use the candidate type for all peers.

As using mixed candidate types would leave us with multiple muxes, each with their own local address.
This is problematic as our Wireguard device can only listen on a single port.

(This could maybe be worked around with some NFtables redirects/rewrites?)

@mlsmaycon
Copy link
Author

Closing in favor of #419

@mlsmaycon mlsmaycon closed this Feb 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Expand UDP Single Port mode (UDPMux) to all Candidate types
3 participants