Advance the network serial when a peer sync or login changes map content

client/internal/engine.go

@@ -863,40 +863,19 @@ func (e *Engine) modifyPeers(peersUpdate []*mgmProto.RemotePeerConfig) error {
 		}
 	}
 
-	// second, look up the activation state of all modified peers before removing
-	// any of them, so an unavailable state leaves the current connections intact
-	active := make(map[string]bool, len(modified))
+	// second, close all modified connections and remove them from the state map
 	for _, p := range modified {
-		peerPubKey := p.GetWgPubKey()
-		state, err := e.statusRecorder.GetPeer(peerPubKey)
+		err := e.removePeer(p.GetWgPubKey())
 		if err != nil {
-			return fmt.Errorf("get status of modified peer %s: %w", peerPubKey, err)
-		}
-		active[peerPubKey] = state.ConnStatus != peer.StatusIdle
-	}
-	// then close all modified connections and remove them from the state map
-	for _, p := range modified {
-		if err := e.removePeer(p.GetWgPubKey()); err != nil {
 			return err
 		}
 	}
-	// third, add the peer connections again, restoring each peer's activation
-	// state: under lazy connections a re-added peer starts idle, but the remote
-	// side of an established connection keeps its state and sends no further
-	// offers, so a previously active peer left idle cannot reconnect until the
-	// remote's connection expires.
+	// third, add the peer connections again
 	for _, p := range modified {
-		if err := e.addNewPeer(p); err != nil {
+		err := e.addNewPeer(p)
+		if err != nil {
 			return err
 		}
-		if !active[p.GetWgPubKey()] {
-			continue
-		}
-		conn, ok := e.peerStore.PeerConn(p.GetWgPubKey())
-		if !ok {
-			continue
-		}
-		e.connMgr.ActivatePeer(e.ctx, conn)
 	}
 	return nil
 }

client/internal/engine_test.go

@@ -26,7 +26,6 @@ import (
 	"github.com/netbirdio/netbird/client/iface/wgaddr"
 	"github.com/netbirdio/netbird/client/iface/wgproxy"
 	"github.com/netbirdio/netbird/client/internal/dns"
-	"github.com/netbirdio/netbird/client/internal/lazyconn"
 	"github.com/netbirdio/netbird/client/internal/peer"
 	"github.com/netbirdio/netbird/client/internal/peer/guard"
 	icemaker "github.com/netbirdio/netbird/client/internal/peer/ice"
@@ -467,163 +466,6 @@ func TestEngine_UpdateNetworkMap(t *testing.T) {
 	}
 }
 
-// TestEngine_ModifiedPeerKeepsActivationState verifies that a peer re-added by
-// modifyPeers keeps its previous activation state under lazy connections. A
-// modified peer is removed and re-added, and a re-add defaults to idle; the
-// remote side of an established connection keeps its state and sends no further
-// offers, so a previously active peer parked idle leaves the pair unable to
-// reconnect until the remote's connection expires.
-func TestEngine_ModifiedPeerKeepsActivationState(t *testing.T) {
-	key, err := wgtypes.GeneratePrivateKey()
-	require.NoError(t, err)
-
-	ctx, cancel := context.WithCancel(CtxInitState(context.Background()))
-	t.Cleanup(cancel)
-
-	relayMgr := relayClient.NewManager(ctx, nil, key.PublicKey().String(), iface.DefaultMTU)
-	engine := NewEngine(ctx, cancel, &EngineConfig{
-		WgIfaceName:    "utun103",
-		WgAddr:         wgaddr.MustParseWGAddress("100.64.0.1/24"),
-		WgPrivateKey:   key,
-		WgPort:         33101,
-		MTU:            iface.DefaultMTU,
-		LazyConnection: lazyconn.StateOn,
-	}, EngineServices{
-		SignalClient:   &signal.MockClient{},
-		MgmClient:      &mgmt.MockClient{},
-		RelayManager:   relayMgr,
-		StatusRecorder: peer.NewRecorder("https://mgm"),
-	}, MobileDependency{})
-
-	wgIface := &MockWGIface{
-		NameFunc: func() string { return "utun103" },
-		IsUserspaceBindFunc: func() bool {
-			return false
-		},
-		RemovePeerFunc: func(peerKey string) error {
-			return nil
-		},
-		AddressFunc: func() wgaddr.Address {
-			return wgaddr.Address{
-				IP:      netip.MustParseAddr("10.20.0.1"),
-				Network: netip.MustParsePrefix("10.20.0.0/24"),
-			}
-		},
-		UpdatePeerFunc: func(peerKey string, allowedIps []netip.Prefix, keepAlive time.Duration, endpoint *net.UDPAddr, preSharedKey *wgtypes.Key) error {
-			return nil
-		},
-	}
-	engine.wgInterface = wgIface
-	engine.routeManager = routemanager.NewManager(routemanager.ManagerConfig{
-		Context:          ctx,
-		PublicKey:        key.PublicKey().String(),
-		DNSRouteInterval: time.Minute,
-		WGInterface:      engine.wgInterface,
-		StatusRecorder:   engine.statusRecorder,
-		RelayManager:     relayMgr,
-	})
-	require.NoError(t, engine.routeManager.Init())
-	engine.dnsServer = &dns.MockServer{
-		UpdateDNSServerFunc: func(serial uint64, update nbdns.Config) error { return nil },
-	}
-	udpConn, err := net.ListenUDP("udp4", nil)
-	require.NoError(t, err)
-	t.Cleanup(func() {
-		if err := udpConn.Close(); err != nil {
-			t.Errorf("close UDP listener: %v", err)
-		}
-	})
-	engine.udpMux = udpmux.NewUniversalUDPMuxDefault(udpmux.UniversalUDPMuxParams{UDPConn: udpConn, MTU: 1280})
-	engine.ctx = ctx
-	engine.srWatcher = guard.NewSRWatcher(nil, nil, nil, icemaker.Config{})
-	engine.connMgr = NewConnMgr(engine.config, engine.statusRecorder, engine.peerStore, wgIface)
-	engine.connMgr.Start(ctx)
-	t.Cleanup(engine.connMgr.Close)
-
-	// No agent version: not lazy-capable, so the connection opens permanently.
-	activePeer := &mgmtProto.RemotePeerConfig{
-		WgPubKey:   "RRHf3Ma6z6mdLbriAJbqhX7+nM/B71lgw2+91q3LfhU=",
-		AllowedIps: []string{"100.64.0.10/24"},
-	}
-	// Lazy-capable, never activated: managed as idle.
-	idlePeer := &mgmtProto.RemotePeerConfig{
-		WgPubKey:     "LLHf3Ma6z6mdLbriAJbqhX7+nM/B71lgw2+91q3LfhU=",
-		AllowedIps:   []string{"100.64.0.11/24"},
-		AgentVersion: "development",
-	}
-
-	err = engine.updateNetworkMap(&mgmtProto.NetworkMap{
-		Serial:      1,
-		RemotePeers: []*mgmtProto.RemotePeerConfig{activePeer, idlePeer},
-	})
-	require.NoError(t, err)
-
-	state, err := engine.statusRecorder.GetPeer(activePeer.WgPubKey)
-	require.NoError(t, err)
-	require.Equal(t, peer.StatusConnecting, state.ConnStatus, "peer without lazy support should open a permanent connection")
-
-	state, err = engine.statusRecorder.GetPeer(idlePeer.WgPubKey)
-	require.NoError(t, err)
-	require.Equal(t, peer.StatusIdle, state.ConnStatus, "lazy-capable peer should be managed as idle")
-
-	// The active peer's agent version changes, as when a peer registered over the
-	// API logs in and fills in its meta; the idle peer's allowed IPs change. Both
-	// count as modified and are removed and re-added.
-	err = engine.updateNetworkMap(&mgmtProto.NetworkMap{
-		Serial: 2,
-		RemotePeers: []*mgmtProto.RemotePeerConfig{
-			{
-				WgPubKey:     activePeer.WgPubKey,
-				AllowedIps:   activePeer.AllowedIps,
-				AgentVersion: "development",
-			},
-			{
-				WgPubKey:     idlePeer.WgPubKey,
-				AllowedIps:   []string{"100.64.0.21/24"},
-				AgentVersion: "development",
-			},
-		},
-	})
-	require.NoError(t, err)
-
-	state, err = engine.statusRecorder.GetPeer(activePeer.WgPubKey)
-	require.NoError(t, err)
-	assert.NotEqual(t, peer.StatusIdle, state.ConnStatus, "previously active peer should stay active after a modify")
-
-	state, err = engine.statusRecorder.GetPeer(idlePeer.WgPubKey)
-	require.NoError(t, err)
-	assert.Equal(t, peer.StatusIdle, state.ConnStatus, "previously idle peer should stay idle after a modify")
-
-	// A missing status entry fails the modify before any connection is removed.
-	require.NoError(t, engine.statusRecorder.RemovePeer(activePeer.WgPubKey))
-	err = engine.updateNetworkMap(&mgmtProto.NetworkMap{
-		Serial: 3,
-		RemotePeers: []*mgmtProto.RemotePeerConfig{
-			{
-				WgPubKey:     activePeer.WgPubKey,
-				AllowedIps:   []string{"100.64.0.30/24"},
-				AgentVersion: "development",
-			},
-			{
-				WgPubKey:     idlePeer.WgPubKey,
-				AllowedIps:   []string{"100.64.0.31/24"},
-				AgentVersion: "development",
-			},
-		},
-	})
-	require.ErrorContains(t, err, "get status of modified peer", "a modify with an unavailable peer state should fail")
-
-	activeConn, ok := engine.peerStore.PeerConn(activePeer.WgPubKey)
-	require.True(t, ok, "peer with unavailable state should keep its connection")
-	assert.True(t, compareNetIPLists(activeConn.WgConfig().AllowedIps, activePeer.AllowedIps),
-		"peer with unavailable state should keep its allowed IPs")
-
-	idleConn, ok := engine.peerStore.PeerConn(idlePeer.WgPubKey)
-	require.True(t, ok, "the other modified peer should keep its connection")
-	assert.True(t, compareNetIPLists(idleConn.WgConfig().AllowedIps, []string{"100.64.0.21/24"}),
-		"the other modified peer should keep its allowed IPs")
-}
-
 func TestEngine_UpdateNetworkMapWithRoutes(t *testing.T) {
 	testCases := []struct {
 		name                 string

management/server/peer.go

@@ -1066,6 +1066,12 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy
 
 	metaDiffAffectsPosture := posture.AffectsPosture(ctx, &metaDiff, resPostureChecks)
 	if requiresPeerUpdate(ctx, isStatusChanged, sync.UpdateAccountPeers, ipv6CapabilityChanged, metaDiffAffectsPosture, metaDiff.VersionChanged(), metaDiff.HostnameChanged()) {
+		// The maps pushed below carry changed content (the peer's version,
+		// hostname, capabilities, or validation state). The serial versions the
+		// distributed map, so it must advance with the content.
+		if err = am.Store.IncrementNetworkSerial(ctx, accountID); err != nil {
+			return nil, nil, nil, 0, fmt.Errorf("increment network serial: %w", err)
+		}
 		changedPeerIDs := []string{peer.ID}
 		affectedPeerIDs := am.syncPeerAffectedPeers(ctx, accountID, peer.ID, nmap, peerNotValid, metaDiffAffectsPosture)
 		if err = am.networkMapController.OnPeersUpdated(ctx, accountID, changedPeerIDs, affectedPeerIDs); err != nil {
@@ -1236,6 +1242,11 @@ func (am *DefaultAccountManager) LoginPeer(ctx context.Context, login types.Peer
 	}
 
 	if shouldUpdatePeers {
+		// The maps pushed below carry changed peer content. The serial versions
+		// the distributed map, so it must advance with the content.
+		if err = am.Store.IncrementNetworkSerial(ctx, accountID); err != nil {
+			return nil, nil, nil, false, fmt.Errorf("increment network serial: %w", err)
+		}
 		changedPeerIDs := []string{peer.ID}
 		affectedPeerIDs := am.resolveAffectedPeersForPeerChanges(ctx, am.Store, accountID, changedPeerIDs)
 		if err = am.networkMapController.OnPeersUpdated(ctx, accountID, changedPeerIDs, affectedPeerIDs); err != nil {

management/server/peer_test.go

@@ -16,11 +16,11 @@ import (
 	"testing"
 	"time"
 
-	"go.uber.org/mock/gomock"
 	"github.com/rs/xid"
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"go.uber.org/mock/gomock"
 	"golang.org/x/exp/maps"
 	"golang.zx2c4.com/wireguard/wgctrl/wgtypes"
 
@@ -2828,6 +2828,46 @@ func TestSyncPeer_IPv6CapabilityChangePropagates(t *testing.T) {
 	})
 }
 
+// TestSyncPeer_PeerUpdateBumpsNetworkSerial ensures that a sync which changes
+// map-relevant peer content (agent version, hostname, capabilities, validation
+// state) advances the network serial before other peers receive the recomputed
+// map. The serial versions the distributed map, so its content must not change
+// under an unchanged serial.
+func TestSyncPeer_PeerUpdateBumpsNetworkSerial(t *testing.T) {
+	manager, _, account, _, peer2, _ := setupNetworkMapTest(t)
+
+	network, err := manager.Store.GetAccountNetwork(context.Background(), store.LockingStrengthNone, account.Id)
+	require.NoError(t, err)
+	serialBefore := network.CurrentSerial()
+
+	t.Run("no bump when nothing changed", func(t *testing.T) {
+		_, _, _, _, err := manager.SyncPeer(context.Background(), types.PeerSync{
+			WireGuardPubKey: peer2.Key,
+			Meta:            peer2.Meta,
+		}, peer2.AccountID)
+		require.NoError(t, err)
+
+		network, err := manager.Store.GetAccountNetwork(context.Background(), store.LockingStrengthNone, account.Id)
+		require.NoError(t, err)
+		assert.Equal(t, serialBefore, network.CurrentSerial(), "an unchanged sync should not advance the serial")
+	})
+
+	t.Run("bump when the agent version changes", func(t *testing.T) {
+		newMeta := peer2.Meta
+		newMeta.WtVersion = "0.99.99"
+
+		_, _, _, _, err := manager.SyncPeer(context.Background(), types.PeerSync{
+			WireGuardPubKey: peer2.Key,
+			Meta:            newMeta,
+		}, peer2.AccountID)
+		require.NoError(t, err)
+
+		network, err := manager.Store.GetAccountNetwork(context.Background(), store.LockingStrengthNone, account.Id)
+		require.NoError(t, err)
+		assert.Greater(t, network.CurrentSerial(), serialBefore, "a map-relevant meta change should advance the serial")
+	})
+}
+
 func TestUpdatePeer_DnsLabelCollisionWithFQDN(t *testing.T) {
 	manager, _, err := createManager(t)
 	require.NoError(t, err, "unable to create account manager")