diff --git a/client/internal/mapsync.go b/client/internal/mapsync.go index 5d8a8340a..a60f55f7a 100644 --- a/client/internal/mapsync.go +++ b/client/internal/mapsync.go @@ -6,7 +6,6 @@ import ( "time" log "github.com/sirupsen/logrus" - "google.golang.org/protobuf/proto" mgmProto "github.com/netbirdio/netbird/shared/management/proto" ) @@ -110,22 +109,31 @@ 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 { - // 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 { + // Nothing pending to preserve (no prev, or prev already fully applied): plain replace. + if prev == nil || update == nil || m.targetGen == m.appliedGen { return update } - merged, ok := proto.Clone(update).(*mgmProto.SyncResponse) - if !ok { - return update + // prev still has unapplied state (targetGen > appliedGen). In the sync protocol a + // nil component means "no change", so if `update` omits a component that prev + // carried, carry prev's forward — otherwise coalescing an update that superseded a + // not-yet-applied one would silently drop the map or config it uniquely brought. + // A present component in `update` is newer and wins. Management may send map-only + // updates (nil config) and config-only updates (nil map); both are handled here. + // A nil component in `update` means "no change", so fill it in from prev — otherwise + // coalescing an update that superseded a not-yet-applied one would drop the map or + // config it uniquely carried. A present component in `update` is newer and wins. + // 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 { + update.NetworkMap = prev.GetNetworkMap() + update.Checks = prev.Checks // checks travel with the map } - merged.NetworkMap = prev.GetNetworkMap() - merged.Checks = prev.Checks - return merged + if update.GetNetbirdConfig() == nil && prev.GetNetbirdConfig() != nil { + update.NetbirdConfig = prev.GetNetbirdConfig() + } + return update } // 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 84362b7fd..d776fef5a 100644 --- a/client/internal/mapsync_test.go +++ b/client/internal/mapsync_test.go @@ -12,32 +12,43 @@ 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) { +// mergeTarget fills components missing from the incoming update with the pending +// (not-yet-applied) prev's, in place, so a coalesced/superseded update does not drop +// the map or config it uniquely carried. +func TestMapStateManager_MergeTargetPreservesPendingState(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 + // config-only update while a full map is still converging (targetGen > appliedGen): + // the pending map (+ checks) is filled into the update in place 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") + prev := &mgmProto.SyncResponse{NetworkMap: &mgmProto.NetworkMap{Serial: 5}} + update := &mgmProto.SyncResponse{NetbirdConfig: &mgmProto.NetbirdConfig{}} + merged := m.mergeTarget(prev, update) + require.Same(t, update, merged, "merges in place, returns the update") + require.EqualValues(t, 5, merged.GetNetworkMap().GetSerial(), "pending map preserved") + require.NotNil(t, merged.GetNetbirdConfig(), "new config kept") - // already converged (targetGen == appliedGen): nothing pending -> plain replace + // symmetric: map-only update while a config-only update is pending -> keep the config + m.targetGen, m.appliedGen = 5, 4 + prev = &mgmProto.SyncResponse{NetbirdConfig: &mgmProto.NetbirdConfig{}} + update = &mgmProto.SyncResponse{NetworkMap: &mgmProto.NetworkMap{Serial: 7}} + merged = m.mergeTarget(prev, update) + require.EqualValues(t, 7, merged.GetNetworkMap().GetSerial(), "new map kept") + require.NotNil(t, merged.GetNetbirdConfig(), "pending config preserved") + + // prev already applied (targetGen == appliedGen): plain replace, no fill-in m.targetGen, m.appliedGen = 5, 5 - require.Same(t, configOnly, m.mergeTarget(fullMap, configOnly)) + prev = &mgmProto.SyncResponse{NetworkMap: &mgmProto.NetworkMap{Serial: 5}} + update = &mgmProto.SyncResponse{NetbirdConfig: &mgmProto.NetbirdConfig{}} + merged = m.mergeTarget(prev, update) + require.Same(t, update, merged) + require.Nil(t, merged.GetNetworkMap(), "no map grafted when prev already applied") - // a full map always replaces - newFull := &mgmProto.SyncResponse{NetworkMap: &mgmProto.NetworkMap{Serial: 6}} + // nothing to carry (update has a map, prev has no config): plain replace m.targetGen, m.appliedGen = 5, 4 - require.Same(t, newFull, m.mergeTarget(fullMap, newFull)) + prev = &mgmProto.SyncResponse{NetworkMap: &mgmProto.NetworkMap{Serial: 5}} + update = &mgmProto.SyncResponse{NetworkMap: &mgmProto.NetworkMap{Serial: 6}} + require.Same(t, update, m.mergeTarget(prev, update)) } // converges over the bounded passes (apply returns more until the 3rd pass),