Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion client/internal/conn_mgr.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,10 @@ func (e *ConnMgr) RemovePeerConn(peerKey string) {
if !ok {
return
}
defer conn.Close(false)
// Permanent removal: drop the WG peer entry too. The peer is gone for
// good and the route-manager's refcounter teardown will release any
// AllowedIPs it had appended along the same path.
defer conn.Close(false, false)

if !e.isStartedWithLazyMgr() {
return
Expand Down
6 changes: 5 additions & 1 deletion client/internal/engine.go
Original file line number Diff line number Diff line change
Expand Up @@ -1603,7 +1603,11 @@ func (e *Engine) addNewPeer(peerConfig *mgmProto.RemotePeerConfig) error {
}

if exists := e.connMgr.AddPeerConn(e.ctx, peerKey, conn); exists {
conn.Close(false)
// AddPeerConn race-loser cleanup: a different Conn for this peer is
// already in the store and owns the WG peer entry. Pass keepWgPeer=
// true so this rejected Conn does not tear down the live one's WG
// state (including any AllowedIPs the route-manager has appended).
conn.Close(false, true)
return fmt.Errorf("peer already exists: %s", peerKey)
}

Expand Down
26 changes: 22 additions & 4 deletions client/internal/peer/conn.go
Original file line number Diff line number Diff line change
Expand Up @@ -230,8 +230,24 @@ func (conn *Conn) Open(engineCtx context.Context) error {
return nil
}

// Close closes this peer Conn issuing a close event to the Conn closeCh
func (conn *Conn) Close(signalToRemote bool) {
// Close closes this peer Conn issuing a close event to the Conn closeCh.
//
// keepWgPeer controls whether the WireGuard peer entry is removed at the
// iface layer. Pass true on lazy-suspend paths (lazy-mgr deactivate) so
// that any AllowedIPs the route-manager appended in-place to the peer
// entry (advertised subnets for a routing peer) survive the wake/sleep
// cycle. Pass false on permanent-removal paths so the peer is fully
// dropped from the WG iface.
//
// Background: when the peer is also a routing peer, the route-manager's
// allowedIPsRefCounter calls WgInterface.AddAllowedIP(peerKey, prefix)
// to extend the peer's AllowedIPs in place. RemovePeer wipes the entire
// peer entry, so a subsequent lazy-wake re-opens the connection with
// only the basic peer-IP /32 from the original PeerConfig — the
// route-manager's refcounter is unaware of the round-trip and does not
// re-apply the routed prefixes, so traffic to those prefixes is
// silently dropped by WG until the next mgmt-side reconcile.
func (conn *Conn) Close(signalToRemote, keepWgPeer bool) {
conn.mu.Lock()
defer conn.wgWatcherWg.Wait()
defer conn.mu.Unlock()
Expand All @@ -247,7 +263,7 @@ func (conn *Conn) Close(signalToRemote bool) {
}
}

conn.Log.Infof("close peer connection")
conn.Log.Infof("close peer connection (keepWgPeer=%v)", keepWgPeer)
conn.ctxCancel()

if conn.wgWatcherCancel != nil {
Expand All @@ -274,7 +290,9 @@ func (conn *Conn) Close(signalToRemote bool) {
conn.wgProxyICE = nil
}

if err := conn.endpointUpdater.RemoveWgPeer(); err != nil {
if keepWgPeer {
conn.Log.Debugf("keep WG peer entry across lazy-suspend so route-manager-applied AllowedIPs survive")
} else if err := conn.endpointUpdater.RemoveWgPeer(); err != nil {
conn.Log.Errorf("failed to remove wg endpoint: %v", err)
}

Expand Down
85 changes: 85 additions & 0 deletions client/internal/peer/conn_close_keepwgpeer_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
package peer

import (
"os"
"reflect"
"strings"
"testing"
)

// TestConn_Close_KeepWgPeerSignature pins the second parameter of
// (*Conn).Close so that the lazy-suspend caller chain (peerstore.PeerConn{Idle,Close}
// and engine race-loser cleanup) cannot be silently reverted to the
// single-bool form. Reflection is used so that the check survives any
// future renaming of the parameter as long as the type stays bool.
//
// The underlying bug, if the second parameter is dropped: when a routing
// peer is also a lazy-managed peer, Conn.Close calls
// endpointUpdater.RemoveWgPeer unconditionally, removing the WG peer
// entry and discarding all AllowedIPs the route-manager has appended
// in-place via WgInterface.AddAllowedIP. The next lazy-wake reopens the
// peer using only its original PeerConfig AllowedIPs (the peer-IP /32),
// so traffic to the advertised subnets is silently dropped by WG until
// the next mgmt-side reconcile re-applies the prefixes.
func TestConn_Close_KeepWgPeerSignature(t *testing.T) {
m, ok := reflect.TypeOf((*Conn)(nil)).MethodByName("Close")
if !ok {
t.Fatal("(*Conn).Close not found")
}

// Method values include the receiver as the first input.
const wantIn = 3 // receiver, signalToRemote bool, keepWgPeer bool
if got := m.Type.NumIn(); got != wantIn {
t.Fatalf("(*Conn).Close expected %d parameters (receiver + signalToRemote + keepWgPeer); got %d", wantIn, got)
}

boolType := reflect.TypeOf(true)
for i := 1; i < wantIn; i++ {
if got := m.Type.In(i); got != boolType {
t.Errorf("(*Conn).Close parameter %d: want bool, got %s", i, got)
}
}
}

// TestConn_Close_KeepWgPeerGate confirms the body of Close routes the
// RemoveWgPeer call through the keepWgPeer guard. Reflection cannot see
// inside a method body, so this is a textual landmark test against
// conn.go. It is intentionally permissive about formatting (either an
// `if keepWgPeer { ... } else` or an `if !keepWgPeer { ... }` shape is
// accepted) but strict about the two invariants:
//
// 1. The keepWgPeer identifier appears in the same source area as the
// RemoveWgPeer call (within ~25 lines).
// 2. RemoveWgPeer is still reachable for the keepWgPeer=false path —
// i.e. the call has not been deleted outright.
//
// If either invariant fails, the lazy-suspend AllowedIPs preservation
// is at risk.
func TestConn_Close_KeepWgPeerGate(t *testing.T) {
src, err := os.ReadFile("conn.go")
if err != nil {
t.Fatalf("read conn.go: %v", err)
}
body := string(src)

const removeCall = "endpointUpdater.RemoveWgPeer()"
idx := strings.Index(body, removeCall)
if idx < 0 {
t.Fatalf("conn.go no longer contains %q — the permanent-removal path is missing or has moved", removeCall)
}

// Look for the keepWgPeer identifier in a 25-line window above the
// RemoveWgPeer call. 25 lines comfortably brackets the if/else shape
// in the current implementation without becoming a whole-file scan.
const window = 25
start := idx
for i, lines := idx, 0; i > 0 && lines < window; i-- {
if body[i] == '\n' {
lines++
start = i
}
}
if !strings.Contains(body[start:idx], "keepWgPeer") {
t.Errorf("conn.go: %q is no longer gated by keepWgPeer within %d lines — every Close call would remove the WG peer entry, dropping route-manager-applied AllowedIPs on lazy-suspend", removeCall, window)
}
}
11 changes: 9 additions & 2 deletions client/internal/peerstore/store.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ func (s *Store) PeerConnOpen(ctx context.Context, pubKey string) {

}

// PeerConnIdle is invoked by the lazy-manager when a peer's idle timer
// expires. The data path is suspended but the WG peer entry stays so
// any AllowedIPs the route-manager has appended (advertised subnets for
// a routing peer) survive the wake/sleep cycle.
func (s *Store) PeerConnIdle(pubKey string) {
s.peerConnsMu.RLock()
defer s.peerConnsMu.RUnlock()
Expand All @@ -103,9 +107,12 @@ func (s *Store) PeerConnIdle(pubKey string) {
if !ok {
return
}
p.Close(true)
p.Close(true, true)
}

// PeerConnClose is invoked by the lazy-manager when a peer must be
// closed without notifying the remote side (e.g. excluded from lazy on
// re-evaluation). Same lazy-suspend semantics: keep the WG peer entry.
func (s *Store) PeerConnClose(pubKey string) {
s.peerConnsMu.RLock()
defer s.peerConnsMu.RUnlock()
Expand All @@ -114,7 +121,7 @@ func (s *Store) PeerConnClose(pubKey string) {
if !ok {
return
}
p.Close(false)
p.Close(false, true)
}

func (s *Store) PeersPubKey() []string {
Expand Down