From cf8d92fbb070a4747b5e6f66cdd82bb38367a983 Mon Sep 17 00:00:00 2001 From: riccardom Date: Tue, 30 Jun 2026 23:43:44 +0200 Subject: [PATCH] Fixup case of config only map superseeding nm map --- client/internal/mapsync.go | 18 +++++++++++++++++- client/internal/mapsync_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 1 deletion(-) diff --git a/client/internal/mapsync.go b/client/internal/mapsync.go index cd4c2b01e..5d8a8340a 100644 --- a/client/internal/mapsync.go +++ b/client/internal/mapsync.go @@ -6,6 +6,7 @@ import ( "time" log "github.com/sirupsen/logrus" + "google.golang.org/protobuf/proto" mgmProto "github.com/netbirdio/netbird/shared/management/proto" ) @@ -109,7 +110,22 @@ func (m *mapStateManager) SetTarget(update *mgmProto.SyncResponse) error { // rest of the manager (generation tracking, convergence, signaling) is unaffected // because it already treats target as "the complete desired state, whatever it is". func (m *mapStateManager) mergeTarget(prev, update *mgmProto.SyncResponse) *mgmProto.SyncResponse { - return update + // Plain replace unless a config-only update (no map) arrives while the pending + // target still has an unconverged map (targetGen > appliedGen). In that case graft + // the pending map (and its checks) onto the incoming config update, so the next pass + // keeps converging the map instead of settling on a nil-map target and stranding the + // remaining peer batches until another full map arrives. + if update == nil || update.GetNetworkMap() != nil || prev == nil || prev.GetNetworkMap() == nil || m.targetGen == m.appliedGen { + return update + } + + merged, ok := proto.Clone(update).(*mgmProto.SyncResponse) + if !ok { + return update + } + merged.NetworkMap = prev.GetNetworkMap() + merged.Checks = prev.Checks + return merged } // run drives convergence until ctx is done. It is meant to run in its own goroutine. diff --git a/client/internal/mapsync_test.go b/client/internal/mapsync_test.go index 7ae551ef5..84362b7fd 100644 --- a/client/internal/mapsync_test.go +++ b/client/internal/mapsync_test.go @@ -12,6 +12,34 @@ import ( mgmProto "github.com/netbirdio/netbird/shared/management/proto" ) +// a config-only update arriving while a full map is still converging must keep the +// pending map (so its remaining peer batches still apply); once converged or when the +// pending target has no map, it replaces as usual. +func TestMapStateManager_MergeTargetPreservesPendingMap(t *testing.T) { + m := newMapStateManager(nil, nil, nil) + + fullMap := &mgmProto.SyncResponse{NetworkMap: &mgmProto.NetworkMap{Serial: 5}} + configOnly := &mgmProto.SyncResponse{NetbirdConfig: &mgmProto.NetbirdConfig{}} + + // still converging the full map (targetGen > appliedGen): graft the map onto the + // incoming config-only update instead of dropping it + m.targetGen, m.appliedGen = 5, 4 + merged := m.mergeTarget(fullMap, configOnly) + require.NotNil(t, merged.GetNetworkMap(), "pending map must be preserved") + require.EqualValues(t, 5, merged.GetNetworkMap().GetSerial()) + require.NotNil(t, merged.GetNetbirdConfig(), "new config must be carried") + require.NotSame(t, configOnly, merged, "must not mutate the received update in place") + + // already converged (targetGen == appliedGen): nothing pending -> plain replace + m.targetGen, m.appliedGen = 5, 5 + require.Same(t, configOnly, m.mergeTarget(fullMap, configOnly)) + + // a full map always replaces + newFull := &mgmProto.SyncResponse{NetworkMap: &mgmProto.NetworkMap{Serial: 6}} + m.targetGen, m.appliedGen = 5, 4 + require.Same(t, newFull, m.mergeTarget(fullMap, newFull)) +} + // converges over the bounded passes (apply returns more until the 3rd pass), // fires onConverged exactly once, then blocks (no further apply) until a new target. func TestMapStateManager_ConvergesThenStops(t *testing.T) {