webrtcprivate: add transport #2576
Conversation
sukunrt
force-pushed
the
webrtcprivate/transport
branch
9 times, most recently
from
912a8ee
to
7a47233
Compare
sukunrt
force-pushed
the
webrtcprivate/transport
branch
2 times, most recently
from
6325e4e
to
5cad220
Compare
sukunrt
force-pushed
the
webrtcprivate/transport
branch
2 times, most recently
from
5adc210
to
eb5a01d
Compare
sukunrt
force-pushed
the
webrtcprivate/transport
branch
from
eb5a01d
to
962b26a
Compare
| @@ -65,6 +67,8 @@ type Security struct { | |||
| Constructor interface{} | |||
| } | |||
|
|
|||
| type ICEServer = webrtc.ICEServer | |||
There was a problem hiding this comment.
Why do we need to redeclare this? Is it a requirement for Fx?
There was a problem hiding this comment.
Nope. Just so that we redefine it a different way later if we want. But I think it's fine to remove this and just take the pion specific object. We can make a breaking change if this needs to be changed.
There was a problem hiding this comment.
It’s a breaking change anyway if we change the underlying type. That should be fine.
| @@ -597,3 +597,11 @@ func SwarmOpts(opts ...swarm.Option) Option { | |||
| return nil | |||
| } | |||
| } | |||
|
|
|||
| func EnableWebRTCPrivate(stunServers []config.ICEServer) Option { | |||
There was a problem hiding this comment.
Why is this a separate option? We already have libp2p.Transport to configure transports. I had hoped that we don't need to introduce a separate knob here.
p2p/host/autorelay/relay_finder.go
Outdated
| @@ -808,3 +815,14 @@ func (rf *relayFinder) resetMetrics() { | |||
| rf.metricsTracer.RelayAddressCount(0) | |||
| rf.metricsTracer.ScheduledWorkUpdated(&scheduledWorkTimes{}) | |||
| } | |||
|
|
|||
| var browserProtocols []int = []int{ma.P_WEBTRANSPORT, ma.P_WEBRTC_DIRECT, ma.P_WS, ma.P_WSS} | |||
There was a problem hiding this comment.
Are you sure about /ws? Is this supposed to catch the /ws/tls case?
| var browserProtocols []int = []int{ma.P_WEBTRANSPORT, ma.P_WEBRTC_DIRECT, ma.P_WS, ma.P_WSS} | |
| var browserProtocols = []int{ma.P_WEBTRANSPORT, ma.P_WEBRTC_DIRECT, ma.P_WS, ma.P_WSS} |
There was a problem hiding this comment.
I've removed ma.P_WS. That shouldn't be here.
p2p/host/autorelay/relay_finder.go
Outdated
| for _, addr := range addrs { | ||
| pub := addr.Encapsulate(circuit) | ||
| raddrs = append(raddrs, pub) | ||
| if rf.conf.webRTCSupport && isBrowserDialableAddr(addr) { |
There was a problem hiding this comment.
Why is this needed for WebRTC and not for WebTransport?
As a general design principle, it would be nice if AutoRelay didn't need to contain any transport-specific logic.
There was a problem hiding this comment.
Yeah. This was a poor choice. I've moved this to host.Addrs. Even this is not great but I'm not sure what else to do without an address pipeline. The flow now is:
BasicHost.AllAddrs => RelayFinder.Addrs => BasicHost.AddrFactory => BasicHost.Addrs
| @@ -27,7 +27,10 @@ func (s *Swarm) TransportForDialing(a ma.Multiaddr) transport.Transport { | |||
| } | |||
| return nil | |||
| } | |||
| if isRelayAddr(a) { | |||
| if isProtocolAddr(a, ma.P_WEBRTC) { | |||
| return s.transports.m[ma.P_WEBRTC] | |||
There was a problem hiding this comment.
Why does this require extra logic here? Is it because of the certhash? Why doesn't it apply to WebTransport then?
There was a problem hiding this comment.
The peer address is a circuitv2 address. So we need to check it before the circuitv2 check but the check is the same as the circuit v2 check.
The address is of the form
/ip4/1.2.3.4/udp/1/quic-v1/p2p/<relay-id>/p2p-circuit/webrtc
So we need to ensure that we don't use the quic-v1 or the p2p-circuit transport for dialing.
There was a problem hiding this comment.
Didn't isRelayAddr already do that? Or phrased differently, the same problem would have occurred for a non-WebRTC address, why do we need special logic for WebRTC now?
There was a problem hiding this comment.
Yes but we don't want the circuit-v2 transport to handle dialing to the webrtc address.
The previous code was
if isRelayAddr => Use circuitv2 transport.
But now we need
if isWebRTCAddr => Use webrtcprivate transport.
else if isRelayAddr => Use circuitv2 transport.
This is because the webrtcprivate address has a p2p-circuit component but we want to use webrtcprivate transport. I could have written this as:
if isRelayAddr(addr):
if isWebRTCAddr(addr):
use webrtcprivate transport
else
use circuitv2 transport
The flow I've implemented is:
peerAddr: <relay-addr>/p2p-circuit/webrtc
- webrtcprivate.Transport.Dial is called with peerAddr
- webrtcprivate.Transport.Dial adds the relay address inferred from the webrtcprivate address inferred from the peerstore. Then makes a host.Connect call to establish a relay connection with the peer.
- Once relay connection is established it opens a signaling stream and establishes a webrtc connection.
I've chosen this because it is similar to circuitv2 transport which first establishes a direct connection with the relay node and then a circuitv2 connection to the peer over the relayed connection. This also has the nice property that Transport.Dial can connect to the peer over the address no matter the state of the host when Transport.Dial is called. In this case, it means if no circuit-v2 connection exists, it'll make a circuit-v2 connection first.
p2p/protocol/holepunch/svc.go
Outdated
| for _, addr := range s.host.Peerstore().Addrs(p) { | ||
| if _, err := addr.ValueForProtocol(ma.P_WEBRTC); err == nil { | ||
| ctx := network.WithForceDirectDial(s.ctx, "webrtc holepunch") | ||
| s.host.Connect(ctx, peer.AddrInfo{ID: p}) // address is already in peerstore |
There was a problem hiding this comment.
This will start multiple Connect calls if there are multiple WebRTC addresses.
There was a problem hiding this comment.
It returns immediately after this line, so this should be fine, no?
| return | ||
| } | ||
| p := conn.RemotePeer() | ||
| // Peer supports DCUtR, let it trigger holepunch |
There was a problem hiding this comment.
What happens if the peer supports DCUtR, but there are only WebRTC addresses?
There was a problem hiding this comment.
That case is handled here: https://github.com/libp2p/go-libp2p/blob/webrtcprivate/transport/p2p/protocol/holepunch/holepuncher.go#L142
In this case the peer(DCUtR initiator) is best suited to handle the call so I've kept it on that side.
sukunrt
force-pushed
the
webrtcprivate/transport
branch
2 times, most recently
from
6ac54f1
to
db7c7b2
Compare
When running with docker, the listener might see a different remote port than the port the dialer observes locally and vice versa
Some failing tests are skipped. They're failing because they require a different testing strategy. A webrtcprivate dial also requires a relay dial, causing some of the expectations on mocks to be incorrectly setup
sukunrt
force-pushed
the
webrtcprivate/transport
branch
from
db7c7b2
to
bc90626
Compare
| @@ -27,7 +27,10 @@ func (s *Swarm) TransportForDialing(a ma.Multiaddr) transport.Transport { | |||
| } | |||
| return nil | |||
| } | |||
| if isRelayAddr(a) { | |||
| if isProtocolAddr(a, ma.P_WEBRTC) { | |||
| return s.transports.m[ma.P_WEBRTC] | |||
There was a problem hiding this comment.
Didn't isRelayAddr already do that? Or phrased differently, the same problem would have occurred for a non-WebRTC address, why do we need special logic for WebRTC now?
| stream datachannel.ReadWriteCloser | ||
| channel *webrtc.DataChannel | ||
| } | ||
|
|
||
| type connection struct { | ||
| pc *webrtc.PeerConnection | ||
| transport *WebRTCTransport | ||
| transport tpt.Transport |
| var _ net.Addr = NetAddr{} | ||
|
|
||
| func (n NetAddr) Network() string { | ||
| return "libp2p-webrtc" |
There was a problem hiding this comment.
I don't think this is how it's intended in the net.Addr. The doc says:
name of the network (for example, "tcp", "udp")
| } | ||
|
|
||
| if l.transport.gater != nil && !l.transport.gater.InterceptSecured(network.DirInbound, s.Conn().RemotePeer(), conn) { | ||
| conn.Close() |
There was a problem hiding this comment.
Do we need to close the scope here?
| err = fmt.Errorf("failed to read offer: %w", err) | ||
| return nil, err | ||
| } | ||
| if msg.Type == nil || *msg.Type != pb.Message_SDP_OFFER { |
There was a problem hiding this comment.
| if msg.Type == nil || *msg.Type != pb.Message_SDP_OFFER { | |
| if msg.GetType() != pb.Message_SDP_OFFER { |
| return | ||
| } | ||
|
|
||
| if msg.Type == nil || *msg.Type != pb.Message_ICE_CANDIDATE { |
There was a problem hiding this comment.
| if msg.Type == nil || *msg.Type != pb.Message_ICE_CANDIDATE { | |
| if msg.GetType() != pb.Message_ICE_CANDIDATE { |
| // Ignore without Debuging on empty message. | ||
| // Pion has a case where OnCandidate callback may be called with a nil | ||
| // candidate | ||
| if msg.Data == nil || *msg.Data == "" { |
There was a problem hiding this comment.
| if msg.Data == nil || *msg.Data == "" { | |
| if len(msg.GetData()) == 0 { |
| } | ||
|
|
||
| var init webrtc.ICECandidateInit | ||
| if err := json.Unmarshal([]byte(*msg.Data), &init); err != nil { |
There was a problem hiding this comment.
| if err := json.Unmarshal([]byte(*msg.Data), &init); err != nil { | |
| if err := json.Unmarshal([]byte(msg.GetData()), &init); err != nil { |
|
|
||
| var _ tpt.Transport = &transport{} | ||
|
|
||
| func AddTransport(h host.Host, gater connmgr.ConnectionGater, stunServers []webrtc.ICEServer) (*transport, error) { |
There was a problem hiding this comment.
Exported function returning unexported type.
Why is this method needed? Why don't we have such a method for other transports?
There was a problem hiding this comment.
We have this method for circuit-v2 transport. We need this method for the same reason for webrtc private transport.
The peer address is <relay-addr>/p2p-circuit/webrtc. This transport will first make a relayed connection with the peer, which is why it needs access to the host. After this point it establishes the connection over the signaling stream.
| @@ -25,14 +25,14 @@ import ( | |||
| "go.uber.org/zap/zapcore" | |||
| ) | |||
|
|
|||
| type connMultiaddrs struct { | |||
| local, remote ma.Multiaddr | |||
| type ConnMultiaddrs struct { | |||
There was a problem hiding this comment.
Nit: Don't make this public. Redefine if necessary.
Part of: #2009
Depends on: #2586