SVI/netbird
1
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-08-04 19:55:09 -04:00
Code Issues 3.0k Packages Projects Releases 10 Wiki Activity

Address PR review feedback for NAT-PMP/UPnP support

Browse Source
This commit is contained in:
Viktor Liu
2026-03-17 06:46:12 +01:00
parent 06c10d7023
commit ec50347d10
4 changed files with 63 additions and 79 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
client/internal/peer/worker_ice.go
View File

@@ -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)

23
client/internal/portforward/manager.go
View File

@@ -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) {

6
client/internal/portforward/manager_js.go
View File

@@ -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 }

97
client/internal/portforward/manager_test.go
View File

@@ -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")
}