mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-21 22:29:08 +02:00
[client] Fix peers not being notified when the relay connection drops (#7490)
* [relay] Signal relay disconnects through the conn context AddCloseListener deduplicated listeners by comparing reflect.ValueOf(callback).Pointer(). For a method value that pointer is the address of the compiler-generated wrapper, not an identity bound to the receiver, so every peer's w.onRelayClientDisconnected compared equal. All peers on the home relay register under the same connectionURL key, so only the first registration survived and the rest were silently dropped. On a relay disconnect those peers were never notified: statusRelay stayed connected and the reconnect guard never fired. The relayed net.Conn itself was closed by closeAllConns, so nothing leaked, but the peer state machine did not learn about it. Foreign relays had the same defect scoped to the peers sharing that server. Rather than fixing the deduplication, drop the peer-level listener registry entirely. A relayed Conn now exposes Context(), cancelled when the connection is torn down, with a cancellation cause naming the reason. This is the same shape quic-go uses for its Conn and Stream types, and it removes the whole class of problems around listener identity, lifetime and deregistration: the signal belongs to the resource instead of a side table. WorkerRelay watches that context in a goroutine whose lifetime matches the connection. A watcher that wakes up for a superseded connection compares the conn pointer against the current one and returns without touching the state machine, so a fast relay reconnect cannot have a stale watcher tear down the connection that replaced it. Client.SetOnDisconnectListener stays: it is server-level and drives the reconnect guard and foreign relay eviction, unrelated to peers. handleRelayReady also checks the conn context, closing the race where the relay dies between OpenConn and the readiness handoff and the peer would otherwise build a WireGuard endpoint over a dead connection. TestNotifierDoubleAdd covered the removed mechanism and is gone. TestForeignAutoClose asserted nothing (both branches logged); it now waits for the relay to leave the client map and fails if it does not. * [relay] Fix build: return the concrete conn from Client.OpenConn OpenConn now returns *Conn, but it still went through connContainer.netConn(), which widens to net.Conn. The helper had one caller and only existed to produce the interface value the signature no longer wants, so return container.conn directly and drop it. * [relay] Assert the local-close cancellation cause explicitly The local-close test only rejected ErrServerDisconnected, so it would also have passed for ErrPeerDisconnected or a bare context.Canceled. closeConn cancels with net.ErrClosed, so assert that. * [client] Ignore relay disconnects from superseded connections The relayed conn watcher compared the conn pointer under relayLock, released it, and only then tore the connection down. A new offer could install its replacement in that window, so a watcher that validated the old pointer went on to close the proxy of the connection that had already replaced it and report the peer as disconnected while it was up. Move the decision to where the teardown happens. Conn records which relayed connection the current proxy was built from, and onRelayDisconnected takes the connection the signal belongs to and drops it under conn.mu when it is no longer the current one. Check and effect are now in the same critical section, so the verdict cannot go stale before it is acted on. This also covers the proxy read loops, whose disconnect listener took no argument and had the same defect: it now names the connection it belongs to. The WG timeout path keeps passing nil, since it deliberately tears down whatever is current. * [client] Bind the relayed conn reference to the proxy swap relayedConnRef was set at the top of the readiness path, but wgProxyRelay only changes at the end, in setRelayedProxy. The two failure returns in between — newProxy and ConfigureWGEndpoint — left the reference pointing at a connection that never became active while the old proxy was still installed. A disconnect of that old, live relay would then be dismissed as belonging to a superseded connection and never cleaned up. Set the reference in setRelayedProxy, next to the proxy it belongs to. Both success paths go through it and neither failure path does, so no failure branch has to remember to roll anything back.
This commit is contained in:
@@ -135,9 +135,10 @@ type Conn struct {
|
||||
// used to store the remote Rosenpass key for Relayed connection in case of connection update from ice
|
||||
rosenpassRemoteKey []byte
|
||||
|
||||
wgProxyICE wgproxy.Proxy
|
||||
wgProxyRelay wgproxy.Proxy
|
||||
handshaker *Handshaker
|
||||
wgProxyICE wgproxy.Proxy
|
||||
wgProxyRelay wgproxy.Proxy
|
||||
relayedConnRef *relayClient.Conn
|
||||
handshaker *Handshaker
|
||||
|
||||
guard *guard.Guard
|
||||
wg sync.WaitGroup
|
||||
@@ -560,7 +561,7 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
|
||||
conn.mu.Lock()
|
||||
defer conn.mu.Unlock()
|
||||
|
||||
if conn.ctx.Err() != nil {
|
||||
if conn.ctx.Err() != nil || rci.relayedConn.Context().Err() != nil {
|
||||
if err := rci.relayedConn.Close(); err != nil {
|
||||
conn.Log.Warnf("failed to close unnecessary relayed connection: %v", err)
|
||||
}
|
||||
@@ -575,7 +576,9 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
|
||||
conn.Log.Errorf("failed to add relayed net.Conn to local proxy: %v", err)
|
||||
return
|
||||
}
|
||||
wgProxy.SetDisconnectListener(conn.onRelayDisconnected)
|
||||
wgProxy.SetDisconnectListener(func() {
|
||||
conn.onRelayDisconnected(rci.relayedConn)
|
||||
})
|
||||
|
||||
conn.dumpState.NewLocalProxy()
|
||||
|
||||
@@ -583,7 +586,7 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
|
||||
|
||||
if conn.isICEActive() {
|
||||
conn.Log.Debugf("do not switch to relay because current priority is: %s", conn.currentConnPriority.String())
|
||||
conn.setRelayedProxy(wgProxy)
|
||||
conn.setRelayedProxy(wgProxy, rci.relayedConn)
|
||||
conn.statusRelay.SetConnected()
|
||||
conn.updateRelayStatus(rci.relayedConn.RemoteAddr().String(), rci.rosenpassPubKey, time.Now())
|
||||
return
|
||||
@@ -614,15 +617,26 @@ func (conn *Conn) onRelayConnectionIsReady(rci RelayConnInfo) {
|
||||
conn.rosenpassRemoteKey = rci.rosenpassPubKey
|
||||
conn.currentConnPriority = conntype.Relay
|
||||
conn.statusRelay.SetConnected()
|
||||
conn.setRelayedProxy(wgProxy)
|
||||
conn.setRelayedProxy(wgProxy, rci.relayedConn)
|
||||
conn.updateRelayStatus(rci.relayedConn.RemoteAddr().String(), rci.rosenpassPubKey, updateTime)
|
||||
conn.Log.Infof("start to communicate with peer via relay")
|
||||
conn.doOnConnected(rci.rosenpassPubKey, rci.rosenpassAddr, updateTime)
|
||||
}
|
||||
|
||||
func (conn *Conn) onRelayDisconnected() {
|
||||
// onRelayDisconnected reports the teardown of a relayed connection. relayedConn
|
||||
// names the connection the signal belongs to, so a signal that arrives after
|
||||
// its connection was replaced is ignored instead of tearing down its successor.
|
||||
// A nil relayedConn means the caller does not track generations and the current
|
||||
// connection is always torn down.
|
||||
func (conn *Conn) onRelayDisconnected(relayedConn *relayClient.Conn) {
|
||||
conn.mu.Lock()
|
||||
defer conn.mu.Unlock()
|
||||
|
||||
if relayedConn != nil && conn.relayedConnRef != relayedConn {
|
||||
conn.Log.Debugf("ignoring relay disconnect of a superseded connection")
|
||||
return
|
||||
}
|
||||
|
||||
conn.handleRelayDisconnectedLocked()
|
||||
}
|
||||
|
||||
@@ -646,6 +660,7 @@ func (conn *Conn) handleRelayDisconnectedLocked() {
|
||||
_ = conn.wgProxyRelay.CloseConn()
|
||||
conn.wgProxyRelay = nil
|
||||
}
|
||||
conn.relayedConnRef = nil
|
||||
|
||||
changed := conn.statusRelay.Get() != worker.StatusDisconnected
|
||||
if changed {
|
||||
@@ -930,13 +945,14 @@ func (conn *Conn) logTraceConnState() {
|
||||
}
|
||||
}
|
||||
|
||||
func (conn *Conn) setRelayedProxy(proxy wgproxy.Proxy) {
|
||||
func (conn *Conn) setRelayedProxy(proxy wgproxy.Proxy, relayedConn *relayClient.Conn) {
|
||||
if conn.wgProxyRelay != nil {
|
||||
if err := conn.wgProxyRelay.CloseConn(); err != nil {
|
||||
conn.Log.Warnf("failed to close deprecated wg proxy conn: %v", err)
|
||||
}
|
||||
}
|
||||
conn.wgProxyRelay = proxy
|
||||
conn.relayedConnRef = relayedConn
|
||||
}
|
||||
|
||||
// onWGHandshakeSuccess is called when the first WireGuard handshake is detected
|
||||
|
||||
@@ -3,7 +3,6 @@ package peer
|
||||
import (
|
||||
"context"
|
||||
"errors"
|
||||
"net"
|
||||
"net/netip"
|
||||
"sync"
|
||||
"sync/atomic"
|
||||
@@ -14,7 +13,7 @@ import (
|
||||
)
|
||||
|
||||
type RelayConnInfo struct {
|
||||
relayedConn net.Conn
|
||||
relayedConn *relayClient.Conn
|
||||
rosenpassPubKey []byte
|
||||
rosenpassAddr string
|
||||
}
|
||||
@@ -27,7 +26,7 @@ type WorkerRelay struct {
|
||||
conn *Conn
|
||||
relayManager *relayClient.Manager
|
||||
|
||||
relayedConn net.Conn
|
||||
relayedConn *relayClient.Conn
|
||||
relayLock sync.Mutex
|
||||
|
||||
relaySupportedOnRemotePeer atomic.Bool
|
||||
@@ -80,12 +79,7 @@ func (w *WorkerRelay) OnNewOffer(remoteOfferAnswer *OfferAnswer) {
|
||||
w.relayedConn = relayedConn
|
||||
w.relayLock.Unlock()
|
||||
|
||||
err = w.relayManager.AddCloseListener(srv, w.onRelayClientDisconnected)
|
||||
if err != nil {
|
||||
log.Errorf("failed to add close listener: %s", err)
|
||||
_ = relayedConn.Close()
|
||||
return
|
||||
}
|
||||
go w.watchRelayedConn(relayedConn)
|
||||
|
||||
w.log.Debugf("peer conn opened via Relay: %s", srv)
|
||||
go w.conn.onRelayConnectionIsReady(RelayConnInfo{
|
||||
@@ -109,12 +103,15 @@ func (w *WorkerRelay) RelayIsSupportedLocally() bool {
|
||||
|
||||
func (w *WorkerRelay) CloseConn() {
|
||||
w.relayLock.Lock()
|
||||
defer w.relayLock.Unlock()
|
||||
if w.relayedConn == nil {
|
||||
conn := w.relayedConn
|
||||
w.relayedConn = nil
|
||||
w.relayLock.Unlock()
|
||||
|
||||
if conn == nil {
|
||||
return
|
||||
}
|
||||
|
||||
if err := w.relayedConn.Close(); err != nil {
|
||||
if err := conn.Close(); err != nil {
|
||||
w.log.Warnf("failed to close relay connection: %v", err)
|
||||
}
|
||||
}
|
||||
@@ -133,6 +130,8 @@ func (w *WorkerRelay) preferredRelayServer(myRelayAddress, remoteRelayAddress st
|
||||
return remoteRelayAddress
|
||||
}
|
||||
|
||||
func (w *WorkerRelay) onRelayClientDisconnected() {
|
||||
go w.conn.onRelayDisconnected()
|
||||
func (w *WorkerRelay) watchRelayedConn(relayedConn *relayClient.Conn) {
|
||||
<-relayedConn.Context().Done()
|
||||
|
||||
w.conn.onRelayDisconnected(relayedConn)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user