[management] use NullBool for terminated flag (#5829)

client/internal/portforward/manager.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"fmt"
 	"net"
+	"regexp"
 	"sync"
 	"time"
 
@@ -15,19 +16,29 @@ import (
 
 const (
 	defaultMappingTTL  = 2 * time.Hour
-	renewalInterval    = defaultMappingTTL / 2
 	discoveryTimeout   = 10 * time.Second
 	mappingDescription = "NetBird"
 )
 
+// upnpErrPermanentLeaseOnly matches UPnP error 725 in SOAP fault XML,
+// allowing for whitespace/newlines between tags from different router firmware.
+var upnpErrPermanentLeaseOnly = regexp.MustCompile(`<errorCode>\s*725\s*</errorCode>`)
+
+// Mapping represents an active NAT port mapping.
 type Mapping struct {
 	Protocol     string
 	InternalPort uint16
 	ExternalPort uint16
 	ExternalIP   net.IP
 	NATType      string
+	// TTL is the lease duration. Zero means a permanent lease that never expires.
+	TTL time.Duration
 }
 
+// TODO: persist mapping state for crash recovery cleanup of permanent leases.
+// Currently not done because State.Cleanup requires NAT gateway re-discovery,
+// which blocks startup for ~10s when no gateway is present (affects all clients).
+
 type Manager struct {
 	cancel context.CancelFunc
 
@@ -43,6 +54,7 @@ type Manager struct {
 	mu sync.Mutex
 }
 
+// NewManager creates a new port forwarding manager.
 func NewManager() *Manager {
 	return &Manager{
 		stopCtx: make(chan context.Context, 1),
@@ -77,8 +89,7 @@ func (m *Manager) Start(ctx context.Context, wgPort uint16) {
 
 	gateway, mapping, err := m.setup(ctx)
 	if err != nil {
-		log.Errorf("failed to setup NAT port mapping: %v", err)
-
+		log.Infof("port forwarding setup: %v", err)
 		return
 	}
 
@@ -86,7 +97,7 @@ func (m *Manager) Start(ctx context.Context, wgPort uint16) {
 	m.mapping = mapping
 	m.mappingLock.Unlock()
 
-	m.renewLoop(ctx, gateway)
+	m.renewLoop(ctx, gateway, mapping.TTL)
 
 	select {
 	case cleanupCtx := <-m.stopCtx:
@@ -145,16 +156,14 @@ func (m *Manager) setup(ctx context.Context) (nat.NAT, *Mapping, error) {
 
 	gateway, err := nat.DiscoverGateway(discoverCtx)
 	if err != nil {
-		log.Infof("NAT gateway discovery failed: %v (port forwarding disabled)", err)
-		return nil, nil, err
+		return nil, nil, fmt.Errorf("discover gateway: %w", err)
 	}
 
 	log.Infof("discovered NAT gateway: %s", gateway.Type())
 
 	mapping, err := m.createMapping(ctx, gateway)
 	if err != nil {
-		log.Warnf("failed to create port mapping: %v", err)
-		return nil, nil, err
+		return nil, nil, fmt.Errorf("create port mapping: %w", err)
 	}
 	return gateway, mapping, nil
 }
@@ -163,9 +172,18 @@ func (m *Manager) createMapping(ctx context.Context, gateway nat.NAT) (*Mapping,
 	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()
 
-	externalPort, err := gateway.AddPortMapping(ctx, "udp", int(m.wgPort), mappingDescription, defaultMappingTTL)
+	ttl := defaultMappingTTL
+	externalPort, err := gateway.AddPortMapping(ctx, "udp", int(m.wgPort), mappingDescription, ttl)
 	if err != nil {
-		return nil, err
+		if !isPermanentLeaseRequired(err) {
+			return nil, err
+		}
+		log.Infof("gateway only supports permanent leases, retrying with indefinite duration")
+		ttl = 0
+		externalPort, err = gateway.AddPortMapping(ctx, "udp", int(m.wgPort), mappingDescription, ttl)
+		if err != nil {
+			return nil, err
+		}
 	}
 
 	externalIP, err := gateway.GetExternalAddress()
@@ -180,6 +198,7 @@ func (m *Manager) createMapping(ctx context.Context, gateway nat.NAT) (*Mapping,
 		ExternalPort: uint16(externalPort),
 		ExternalIP:   externalIP,
 		NATType:      gateway.Type(),
+		TTL:          ttl,
 	}
 
 	log.Infof("created port mapping: %d -> %d via %s (external IP: %s)",
@@ -187,8 +206,14 @@ func (m *Manager) createMapping(ctx context.Context, gateway nat.NAT) (*Mapping,
 	return mapping, nil
 }
 
-func (m *Manager) renewLoop(ctx context.Context, gateway nat.NAT) {
-	ticker := time.NewTicker(renewalInterval)
+func (m *Manager) renewLoop(ctx context.Context, gateway nat.NAT, ttl time.Duration) {
+	if ttl == 0 {
+		// Permanent mappings don't expire, just wait for cancellation.
+		<-ctx.Done()
+		return
+	}
+
+	ticker := time.NewTicker(ttl / 2)
 	defer ticker.Stop()
 
 	for {
@@ -208,7 +233,7 @@ func (m *Manager) renewMapping(ctx context.Context, gateway nat.NAT) error {
 	ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
 	defer cancel()
 
-	externalPort, err := gateway.AddPortMapping(ctx, m.mapping.Protocol, int(m.mapping.InternalPort), mappingDescription, defaultMappingTTL)
+	externalPort, err := gateway.AddPortMapping(ctx, m.mapping.Protocol, int(m.mapping.InternalPort), mappingDescription, m.mapping.TTL)
 	if err != nil {
 		return fmt.Errorf("add port mapping: %w", err)
 	}
@@ -248,3 +273,8 @@ func (m *Manager) startTearDown(ctx context.Context) {
 	default:
 	}
 }
+
+// isPermanentLeaseRequired checks if a UPnP error indicates the gateway only supports permanent leases (error 725).
+func isPermanentLeaseRequired(err error) bool {
+	return err != nil && upnpErrPermanentLeaseOnly.MatchString(err.Error())
+}

client/internal/portforward/manager_js.go

@@ -3,15 +3,18 @@ package portforward
 import (
 	"context"
 	"net"
+	"time"
 )
 
-// Mapping represents port mapping information.
+// Mapping represents an active NAT port mapping.
 type Mapping struct {
 	Protocol     string
 	InternalPort uint16
 	ExternalPort uint16
 	ExternalIP   net.IP
 	NATType      string
+	// TTL is the lease duration. Zero means a permanent lease that never expires.
+	TTL time.Duration
 }
 
 // Manager is a stub for js/wasm builds where NAT-PMP/UPnP is not supported.

client/internal/portforward/manager_test.go

@@ -4,23 +4,25 @@ package portforward
 
 import (
 	"context"
+	"fmt"
 	"net"
 	"testing"
 	"time"
 
-	"github.com/libp2p/go-nat"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
 type mockNAT struct {
-	natType          string
-	deviceAddr       net.IP
-	externalAddr     net.IP
-	internalAddr     net.IP
-	mappings         map[int]int
-	addMappingErr    error
-	deleteMappingErr error
+	natType             string
+	deviceAddr          net.IP
+	externalAddr        net.IP
+	internalAddr        net.IP
+	mappings            map[int]int
+	addMappingErr       error
+	deleteMappingErr    error
+	onlyPermanentLeases bool
+	lastTimeout         time.Duration
 }
 
 func newMockNAT() *mockNAT {
@@ -53,8 +55,12 @@ func (m *mockNAT) AddPortMapping(ctx context.Context, protocol string, internalP
 	if m.addMappingErr != nil {
 		return 0, m.addMappingErr
 	}
+	if m.onlyPermanentLeases && timeout != 0 {
+		return 0, fmt.Errorf("SOAP fault. Code:  | Explanation:  | Detail: <UPnPError xmlns=\"urn:schemas-upnp-org:control-1-0\"><errorCode>725</errorCode><errorDescription>OnlyPermanentLeasesSupported</errorDescription></UPnPError>")
+	}
 	externalPort := internalPort
 	m.mappings[internalPort] = externalPort
+	m.lastTimeout = timeout
 	return externalPort, nil
 }
 
@@ -80,6 +86,7 @@ func TestManager_CreateMapping(t *testing.T) {
 	assert.Equal(t, uint16(51820), mapping.ExternalPort)
 	assert.Equal(t, "Mock-NAT", mapping.NATType)
 	assert.Equal(t, net.ParseIP("203.0.113.50").To4(), mapping.ExternalIP.To4())
+	assert.Equal(t, defaultMappingTTL, mapping.TTL)
 }
 
 func TestManager_GetMapping_ReturnsNilWhenNotReady(t *testing.T) {
@@ -131,29 +138,64 @@ func TestManager_Cleanup_NilMapping(t *testing.T) {
 	m.cleanup(context.Background(), gateway)
 }
 
-func TestState_Cleanup(t *testing.T) {
-	origDiscover := discoverGateway
-	defer func() { discoverGateway = origDiscover }()
 
-	mockGateway := newMockNAT()
-	mockGateway.mappings[51820] = 51820
-	discoverGateway = func(ctx context.Context) (nat.NAT, error) {
-		return mockGateway, nil
-	}
+func TestManager_CreateMapping_PermanentLeaseFallback(t *testing.T) {
+	m := NewManager()
+	m.wgPort = 51820
 
-	state := &State{
-		Protocol:     "udp",
-		InternalPort: 51820,
-	}
+	gateway := newMockNAT()
+	gateway.onlyPermanentLeases = true
 
-	err := state.Cleanup()
-	assert.NoError(t, err)
+	mapping, err := m.createMapping(context.Background(), gateway)
+	require.NoError(t, err)
+	require.NotNil(t, mapping)
 
-	_, exists := mockGateway.mappings[51820]
-	assert.False(t, exists, "mapping should be deleted after cleanup")
+	assert.Equal(t, uint16(51820), mapping.InternalPort)
+	assert.Equal(t, time.Duration(0), mapping.TTL, "should return zero TTL for permanent lease")
+	assert.Equal(t, time.Duration(0), gateway.lastTimeout, "should have retried with zero duration")
 }
 
-func TestState_Name(t *testing.T) {
-	state := &State{}
-	assert.Equal(t, "port_forward_state", state.Name())
+func TestIsPermanentLeaseRequired(t *testing.T) {
+	tests := []struct {
+		name     string
+		err      error
+		expected bool
+	}{
+		{
+			name:     "nil error",
+			err:      nil,
+			expected: false,
+		},
+		{
+			name:     "UPnP error 725",
+			err:      fmt.Errorf("SOAP fault. Code:  | Detail: <UPnPError><errorCode>725</errorCode><errorDescription>OnlyPermanentLeasesSupported</errorDescription></UPnPError>"),
+			expected: true,
+		},
+		{
+			name:     "wrapped error with 725",
+			err:      fmt.Errorf("add port mapping: %w", fmt.Errorf("Detail: <errorCode>725</errorCode>")),
+			expected: true,
+		},
+		{
+			name:     "error 725 with newlines in XML",
+			err:      fmt.Errorf("<errorCode>\n  725\n</errorCode>"),
+			expected: true,
+		},
+		{
+			name:     "bare 725 without XML tag",
+			err:      fmt.Errorf("error code 725"),
+			expected: false,
+		},
+		{
+			name:     "unrelated error",
+			err:      fmt.Errorf("connection refused"),
+			expected: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.expected, isPermanentLeaseRequired(tt.err))
+		})
+	}
 }

client/internal/portforward/state.go

@@ -1,50 +0,0 @@
-//go:build !js
-
-package portforward
-
-import (
-	"context"
-	"fmt"
-
-	"github.com/libp2p/go-nat"
-	log "github.com/sirupsen/logrus"
-)
-
-// discoverGateway is the function used for NAT gateway discovery.
-// It can be replaced in tests to avoid real network operations.
-var discoverGateway = nat.DiscoverGateway
-
-// State is persisted only for crash recovery cleanup
-type State struct {
-	InternalPort uint16 `json:"internal_port,omitempty"`
-	Protocol     string `json:"protocol,omitempty"`
-}
-
-func (s *State) Name() string {
-	return "port_forward_state"
-}
-
-// Cleanup implements statemanager.CleanableState for crash recovery
-func (s *State) Cleanup() error {
-	if s.InternalPort == 0 {
-		return nil
-	}
-
-	log.Infof("cleaning up stale port mapping for port %d", s.InternalPort)
-
-	ctx, cancel := context.WithTimeout(context.Background(), discoveryTimeout)
-	defer cancel()
-
-	gateway, err := discoverGateway(ctx)
-	if err != nil {
-		// Discovery failure is not an error - gateway may not exist
-		log.Debugf("cleanup: no gateway found: %v", err)
-		return nil
-	}
-
-	if err := gateway.DeletePortMapping(ctx, s.Protocol, int(s.InternalPort)); err != nil {
-		return fmt.Errorf("delete port mapping: %w", err)
-	}
-
-	return nil
-}

client/server/state_generic.go

@@ -10,10 +10,6 @@ import (
 )
 
 // registerStates registers all states that need crash recovery cleanup.
-// Note: portforward.State is intentionally NOT registered here to avoid blocking startup
-// for up to 10 seconds during NAT gateway discovery when no gateway is present.
-// The gateway reference cannot be persisted across restarts, so cleanup requires re-discovery.
-// Port forward cleanup is handled by the Manager during normal operation instead.
 func registerStates(mgr *statemanager.Manager) {
 	mgr.RegisterState(&dns.ShutdownState{})
 	mgr.RegisterState(&systemops.ShutdownState{})

client/server/state_linux.go

@@ -12,10 +12,6 @@ import (
 )
 
 // registerStates registers all states that need crash recovery cleanup.
-// Note: portforward.State is intentionally NOT registered here to avoid blocking startup
-// for up to 10 seconds during NAT gateway discovery when no gateway is present.
-// The gateway reference cannot be persisted across restarts, so cleanup requires re-discovery.
-// Port forward cleanup is handled by the Manager during normal operation instead.
 func registerStates(mgr *statemanager.Manager) {
 	mgr.RegisterState(&dns.ShutdownState{})
 	mgr.RegisterState(&systemops.ShutdownState{})

management/server/store/sql_store.go

@@ -2099,6 +2099,7 @@ func (s *SqlStore) getServices(ctx context.Context, accountID string) ([]*rpserv
 		var createdAt, certIssuedAt sql.NullTime
 		var status, proxyCluster, sessionPrivateKey, sessionPublicKey sql.NullString
 		var mode, source, sourcePeer sql.NullString
+		var terminated sql.NullBool
 		err := row.Scan(
 			&s.ID,
 			&s.AccountID,
@@ -2119,7 +2120,7 @@ func (s *SqlStore) getServices(ctx context.Context, accountID string) ([]*rpserv
 			&s.PortAutoAssigned,
 			&source,
 			&sourcePeer,
-			&s.Terminated,
+			&terminated,
 		)
 		if err != nil {
 			return nil, err
@@ -2160,7 +2161,9 @@ func (s *SqlStore) getServices(ctx context.Context, accountID string) ([]*rpserv
 		if sourcePeer.Valid {
 			s.SourcePeer = sourcePeer.String
 		}
-
+		if terminated.Valid {
+			s.Terminated = terminated.Bool
+		}
 		s.Targets = []*rpservice.Target{}
 		return &s, nil
 	})