From e7c7c75ec674c2232373c94902c8b1bbe2e8fe4b Mon Sep 17 00:00:00 2001 From: riccardom Date: Tue, 20 Oct 2026 10:18:03 +0200 Subject: [PATCH] [client] Treat a components envelope sync as carrying a network map Envelope syncs leave NetworkMap nil and ship the map in NetworkMapEnvelope, so the nil-map checks read every one of them as config-only: they were never persisted, and a config-only update coalescing over a pending envelope sync dropped its map. The version, top-level peer config and checks travel with the envelope, and a nil session deadline is "no change" like the other optional fields, so the pending one is carried forward too. --- client/internal/engine.go | 6 ++--- client/internal/engine_test.go | 20 +++++++++++++++++ client/internal/mapsync.go | 21 +++++++++++++---- client/internal/mapsync_test.go | 40 +++++++++++++++++++++++++++++++++ 4 files changed, 80 insertions(+), 7 deletions(-) diff --git a/client/internal/engine.go b/client/internal/engine.go index 2bc6e723f..03f6f44ce 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -1192,9 +1192,9 @@ func (e *Engine) updateNetbirdConfig(wCfg *mgmProto.NetbirdConfig) error { // engine close) mid-call and have this write resurrect a file that was just removed. func (e *Engine) persistSyncResponse(update *mgmProto.SyncResponse) { // Only persist updates that carry a network map. Config-only updates (e.g. relay - // token rotation, STUN/TURN) have a nil NetworkMap; persisting them would overwrite - // the last full map on disk and break restore-on-restart. - if update.GetNetworkMap() == nil { + // token rotation, STUN/TURN) carry none; persisting them would overwrite the last + // full map on disk and break restore-on-restart. + if !syncCarriesNetworkMap(update) { return } diff --git a/client/internal/engine_test.go b/client/internal/engine_test.go index 78ba4a8f7..3f56749b3 100644 --- a/client/internal/engine_test.go +++ b/client/internal/engine_test.go @@ -37,6 +37,7 @@ import ( "github.com/netbirdio/netbird/monotime" "github.com/netbirdio/netbird/route" mgmt "github.com/netbirdio/netbird/shared/management/client" + sharedgrpc "github.com/netbirdio/netbird/shared/management/grpc" mgmtProto "github.com/netbirdio/netbird/shared/management/proto" "github.com/netbirdio/netbird/shared/netiputil" relayClient "github.com/netbirdio/netbird/shared/relay/client" @@ -1683,3 +1684,22 @@ func TestEngine_SyncResponsePersistence(t *testing.T) { _, err = e.GetLatestSyncResponse() require.Error(t, err) } + +// An envelope sync leaves NetworkMap nil and carries the map in +// NetworkMapEnvelope. It must still be persisted, and a later config-only +// update must not overwrite it. +func TestEngine_SyncResponsePersistenceKeepsEnvelope(t *testing.T) { + e := &Engine{} + e.SetSyncResponsePersistence(true) + + e.persistSyncResponse(&mgmtProto.SyncResponse{ + NetworkMapEnvelope: &mgmtProto.NetworkMapEnvelope{}, + Version: int32(sharedgrpc.ComponentNetworkMap), + }) + e.persistSyncResponse(&mgmtProto.SyncResponse{NetbirdConfig: &mgmtProto.NetbirdConfig{}}) + + got, err := e.GetLatestSyncResponse() + require.NoError(t, err) + assert.NotNil(t, got.GetNetworkMapEnvelope(), "the envelope sync should be the persisted one") + assert.Equal(t, int32(sharedgrpc.ComponentNetworkMap), got.GetVersion(), "the persisted sync should keep its wire version") +} diff --git a/client/internal/mapsync.go b/client/internal/mapsync.go index f91814078..4f95b7538 100644 --- a/client/internal/mapsync.go +++ b/client/internal/mapsync.go @@ -41,7 +41,7 @@ type mapStateManager struct { // persist snapshots an update to disk for restore-on-restart. Called once per // update received from management (in SetTarget), including ones later coalesced // or skipped from apply, so the on-disk state mirrors what management last sent. - // The impl skips config-only updates (nil NetworkMap). May be nil. + // The impl skips config-only updates (no network map). May be nil. persist func(*mgmProto.SyncResponse) mu sync.Mutex @@ -75,7 +75,7 @@ func (m *mapStateManager) SetTarget(update *mgmProto.SyncResponse) error { } m.target = m.mergeTarget(m.target, update) // Bump an internal generation counter, NOT the map serial: config-only updates - // (relay token rotation, STUN/TURN) arrive with NetworkMap == nil and carry no + // (relay token rotation, STUN/TURN) arrive without a network map and carry no // serial, yet must still be applied. Every SetTarget is therefore a distinct // target regardless of payload. Map-serial staleness is enforced separately // inside apply (updateNetworkMap). @@ -92,7 +92,7 @@ func (m *mapStateManager) SetTarget(update *mgmProto.SyncResponse) error { // pass), and including ones that get coalesced/skipped from apply, so the on-disk // state always reflects the latest map management sent. Done after waking the loop // so convergence can start in parallel with the disk write. The persist impl skips - // config-only updates (nil NetworkMap). + // config-only updates (no network map). if m.persist != nil { m.persist(update) } @@ -126,16 +126,29 @@ func (m *mapStateManager) mergeTarget(prev, update *mgmProto.SyncResponse) *mgmP // We mutate `update` in place: it is a fresh per-message allocation from the sync // stream (see receiveUpdatesEvents — not reused), and persisting this squashed target // is correct, since it is the current full (superset) desired state. - if update.GetNetworkMap() == nil && prev.GetNetworkMap() != nil { + if !syncCarriesNetworkMap(update) && syncCarriesNetworkMap(prev) { update.NetworkMap = prev.GetNetworkMap() + update.NetworkMapEnvelope = prev.GetNetworkMapEnvelope() + update.Version = prev.GetVersion() + update.PeerConfig = prev.GetPeerConfig() update.Checks = prev.Checks // checks travel with the map } if update.GetNetbirdConfig() == nil && prev.GetNetbirdConfig() != nil { update.NetbirdConfig = prev.GetNetbirdConfig() } + if update.GetSessionExpiresAt() == nil && prev.GetSessionExpiresAt() != nil { + update.SessionExpiresAt = prev.GetSessionExpiresAt() + } return update } +// syncCarriesNetworkMap reports whether the update carries a network map in +// either wire shape: the legacy NetworkMap or the components envelope, which +// leaves NetworkMap nil. +func syncCarriesNetworkMap(update *mgmProto.SyncResponse) bool { + return update.GetNetworkMap() != nil || update.GetNetworkMapEnvelope() != nil +} + // run drives convergence until ctx is done. It is meant to run in its own goroutine. func (m *mapStateManager) run(ctx context.Context) { // passGen is the generation of the most recent apply() call (0 = none). A pass is diff --git a/client/internal/mapsync_test.go b/client/internal/mapsync_test.go index 30b838b2b..07607a181 100644 --- a/client/internal/mapsync_test.go +++ b/client/internal/mapsync_test.go @@ -8,7 +8,9 @@ import ( "time" "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/timestamppb" + sharedgrpc "github.com/netbirdio/netbird/shared/management/grpc" mgmProto "github.com/netbirdio/netbird/shared/management/proto" ) @@ -309,3 +311,41 @@ func TestMapStateManager_StopsBetweenPassesOnCancel(t *testing.T) { } require.EqualValues(t, 1, passes.Load(), "no pass should run after the cancelling one") } + +// A config-only update superseding a pending envelope sync must carry the +// envelope forward together with the fields that travel with it, and a nil +// session deadline means "no change" just like a nil map or config. +func TestMapStateManager_MergeTargetPreservesPendingEnvelope(t *testing.T) { + m := newMapStateManager(nil, nil, nil) + m.targetGen, m.appliedGen = 5, 4 + + envelope := &mgmProto.NetworkMapEnvelope{} + peerConfig := &mgmProto.PeerConfig{Fqdn: "peer.netbird.cloud"} + checks := []*mgmProto.Checks{{Files: []string{"/etc/hosts"}}} + deadline := timestamppb.New(time.Unix(1_900_000_000, 0)) + prev := &mgmProto.SyncResponse{ + NetworkMapEnvelope: envelope, + Version: int32(sharedgrpc.ComponentNetworkMap), + PeerConfig: peerConfig, + Checks: checks, + SessionExpiresAt: deadline, + } + update := &mgmProto.SyncResponse{NetbirdConfig: &mgmProto.NetbirdConfig{}} + + merged := m.mergeTarget(prev, update) + require.Same(t, update, merged, "merges in place, returns the update") + require.Same(t, envelope, merged.GetNetworkMapEnvelope(), "pending envelope preserved") + require.Equal(t, int32(sharedgrpc.ComponentNetworkMap), merged.GetVersion(), "wire version travels with the envelope") + require.Same(t, peerConfig, merged.GetPeerConfig(), "top-level peer config travels with the envelope") + require.Equal(t, checks, merged.GetChecks(), "checks travel with the map") + require.Same(t, deadline, merged.GetSessionExpiresAt(), "pending session deadline preserved") + require.NotNil(t, merged.GetNetbirdConfig(), "new config kept") + + // a newer envelope wins over the pending one + m.targetGen, m.appliedGen = 6, 5 + newer := &mgmProto.NetworkMapEnvelope{} + update = &mgmProto.SyncResponse{NetworkMapEnvelope: newer, Version: int32(sharedgrpc.ComponentNetworkMap)} + merged = m.mergeTarget(prev, update) + require.Same(t, newer, merged.GetNetworkMapEnvelope(), "new envelope kept") + require.Nil(t, merged.GetPeerConfig(), "the pending peer config is not grafted onto a newer map") +}