From ec50347d10d60f81ec3c2053ac860f62dff3426b Mon Sep 17 00:00:00 2001 From: Viktor Liu Date: Tue, 17 Mar 2026 06:46:12 +0100 Subject: [PATCH] Address PR review feedback for NAT-PMP/UPnP support --- client/internal/peer/worker_ice.go | 16 ++-- client/internal/portforward/manager.go | 23 ++--- client/internal/portforward/manager_js.go | 6 +- client/internal/portforward/manager_test.go | 97 +++++++++------------ 4 files changed, 63 insertions(+), 79 deletions(-) diff --git a/client/internal/peer/worker_ice.go b/client/internal/peer/worker_ice.go index 4d07ade95..29bf5aaaa 100644 --- a/client/internal/peer/worker_ice.go +++ b/client/internal/peer/worker_ice.go @@ -384,14 +384,6 @@ func (w *WorkerICE) onICECandidate(candidate ice.Candidate) { // injectPortForwardedCandidate signals an additional candidate using the pre-created port mapping. func (w *WorkerICE) injectPortForwardedCandidate(srflxCandidate ice.Candidate) { - w.muxAgent.Lock() - if w.portForwardAttempted { - w.muxAgent.Unlock() - return - } - w.portForwardAttempted = true - w.muxAgent.Unlock() - pfManager := w.conn.portForwardManager if pfManager == nil { return @@ -402,6 +394,14 @@ func (w *WorkerICE) injectPortForwardedCandidate(srflxCandidate ice.Candidate) { return } + w.muxAgent.Lock() + if w.portForwardAttempted { + w.muxAgent.Unlock() + return + } + w.portForwardAttempted = true + w.muxAgent.Unlock() + forwardedCandidate, err := w.createForwardedCandidate(srflxCandidate, mapping) if err != nil { w.log.Warnf("create forwarded candidate: %v", err) diff --git a/client/internal/portforward/manager.go b/client/internal/portforward/manager.go index d0768009c..019c2ad86 100644 --- a/client/internal/portforward/manager.go +++ b/client/internal/portforward/manager.go @@ -159,18 +159,6 @@ func (m *Manager) setup(ctx context.Context) (nat.NAT, *Mapping, error) { return gateway, mapping, nil } -func (m *Manager) cleanupResidual(ctx context.Context, gateway nat.NAT, state *State) error { - ctx, cancel := context.WithTimeout(ctx, 10*time.Second) - defer cancel() - - if err := gateway.DeletePortMapping(ctx, state.Protocol, int(state.InternalPort)); err != nil { - return fmt.Errorf("delete residual mapping: %w", err) - } - - log.Infof("cleaned up residual port mapping for port %d", state.InternalPort) - return nil -} - func (m *Manager) createMapping(ctx context.Context, gateway nat.NAT) (*Mapping, error) { ctx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() @@ -237,16 +225,21 @@ func (m *Manager) renewMapping(ctx context.Context, gateway nat.NAT) error { } func (m *Manager) cleanup(ctx context.Context, gateway nat.NAT) { - if m.mapping == nil { + m.mappingLock.Lock() + mapping := m.mapping + m.mapping = nil + m.mappingLock.Unlock() + + if mapping == nil { return } - if err := gateway.DeletePortMapping(ctx, m.mapping.Protocol, int(m.mapping.InternalPort)); err != nil { + if err := gateway.DeletePortMapping(ctx, mapping.Protocol, int(mapping.InternalPort)); err != nil { log.Warnf("delete port mapping on stop: %v", err) return } - log.Infof("deleted port mapping for port %d", m.mapping.InternalPort) + log.Infof("deleted port mapping for port %d", mapping.InternalPort) } func (m *Manager) startTearDown(ctx context.Context) { diff --git a/client/internal/portforward/manager_js.go b/client/internal/portforward/manager_js.go index e7fd4a64e..d5db147f2 100644 --- a/client/internal/portforward/manager_js.go +++ b/client/internal/portforward/manager_js.go @@ -22,8 +22,10 @@ func NewManager() *Manager { return &Manager{} } -// Start is a no-op on js/wasm. -func (m *Manager) Start(context.Context, uint16) {} +// Start is a no-op on js/wasm: NAT-PMP/UPnP is not available in browser environments. +func (m *Manager) Start(context.Context, uint16) { + // no NAT traversal in wasm +} // GracefullyStop is a no-op on js/wasm. func (m *Manager) GracefullyStop(context.Context) error { return nil } diff --git a/client/internal/portforward/manager_test.go b/client/internal/portforward/manager_test.go index 5548fa4d5..1029e87f5 100644 --- a/client/internal/portforward/manager_test.go +++ b/client/internal/portforward/manager_test.go @@ -11,8 +11,6 @@ import ( "github.com/libp2p/go-nat" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - - "github.com/netbirdio/netbird/client/internal/statemanager" ) type mockNAT struct { @@ -68,38 +66,13 @@ func (m *mockNAT) DeletePortMapping(ctx context.Context, protocol string, intern return nil } -func setupTestManager(t *testing.T) (*Manager, context.CancelFunc) { - tmpDir := t.TempDir() - statePath := tmpDir + "/state.json" - sm := statemanager.New(statePath) - sm.Start() - t.Cleanup(func() { - ctx, cancel := context.WithTimeout(context.Background(), time.Second) - defer cancel() - require.NoError(t, sm.Stop(ctx)) - }) - - m := NewManager(sm) - m.gateway = newMockNAT() - - ctx, cancel := context.WithCancel(context.Background()) - m.ctx = ctx - m.cancel = cancel +func TestManager_CreateMapping(t *testing.T) { + m := NewManager() m.wgPort = 51820 - sm.RegisterState(&State{}) - - return m, cancel -} - -func TestManager_CreateMapping(t *testing.T) { - m, cancel := setupTestManager(t) - defer cancel() - - err := m.createMapping(nil) + gateway := newMockNAT() + mapping, err := m.createMapping(context.Background(), gateway) require.NoError(t, err) - - mapping := m.GetMapping() require.NotNil(t, mapping) assert.Equal(t, "udp", mapping.Protocol) @@ -110,36 +83,52 @@ func TestManager_CreateMapping(t *testing.T) { } func TestManager_GetMapping_ReturnsNilWhenNotReady(t *testing.T) { - tmpDir := t.TempDir() - statePath := tmpDir + "/state.json" - sm := statemanager.New(statePath) - - m := NewManager(sm) - + m := NewManager() assert.Nil(t, m.GetMapping()) } -func TestManager_IsAvailable(t *testing.T) { - tmpDir := t.TempDir() - statePath := tmpDir + "/state.json" - sm := statemanager.New(statePath) +func TestManager_GetMapping_ReturnsCopy(t *testing.T) { + m := NewManager() + m.mapping = &Mapping{ + Protocol: "udp", + InternalPort: 51820, + ExternalPort: 51820, + } - m := NewManager(sm) + mapping := m.GetMapping() + require.NotNil(t, mapping) + assert.Equal(t, uint16(51820), mapping.InternalPort) - // Initially not available (no mapping) - assert.False(t, m.IsAvailable()) + // Mutating the returned copy should not affect the manager's mapping. + mapping.ExternalPort = 9999 + assert.Equal(t, uint16(51820), m.GetMapping().ExternalPort) +} - // Set gateway but no mapping - still not available - m.gateway = newMockNAT() - assert.False(t, m.IsAvailable()) +func TestManager_Cleanup_DeletesMapping(t *testing.T) { + m := NewManager() + m.mapping = &Mapping{ + Protocol: "udp", + InternalPort: 51820, + ExternalPort: 51820, + } - // Add mapping - now available - m.mapping = &Mapping{InternalPort: 51820} - assert.True(t, m.IsAvailable()) + gateway := newMockNAT() + // Seed the mock so we can verify deletion. + gateway.mappings[51820] = 51820 - // Clear mapping - not available again - m.mapping = nil - assert.False(t, m.IsAvailable()) + m.cleanup(context.Background(), gateway) + + _, exists := gateway.mappings[51820] + assert.False(t, exists, "mapping should be deleted from gateway") + assert.Nil(t, m.GetMapping(), "in-memory mapping should be cleared") +} + +func TestManager_Cleanup_NilMapping(t *testing.T) { + m := NewManager() + gateway := newMockNAT() + + // Should not panic or call gateway. + m.cleanup(context.Background(), gateway) } func TestState_Cleanup(t *testing.T) { @@ -147,6 +136,7 @@ func TestState_Cleanup(t *testing.T) { defer func() { discoverGateway = origDiscover }() mockGateway := newMockNAT() + mockGateway.mappings[51820] = 51820 discoverGateway = func(ctx context.Context) (nat.NAT, error) { return mockGateway, nil } @@ -159,7 +149,6 @@ func TestState_Cleanup(t *testing.T) { err := state.Cleanup() assert.NoError(t, err) - // Verify the mapping was deleted _, exists := mockGateway.mappings[51820] assert.False(t, exists, "mapping should be deleted after cleanup") }