[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.
This commit is contained in:
riccardom
2026-10-20 10:18:03 +02:00
parent e5f788a88c
commit e7c7c75ec6
4 changed files with 80 additions and 7 deletions
+3 -3
View File
@@ -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. // engine close) mid-call and have this write resurrect a file that was just removed.
func (e *Engine) persistSyncResponse(update *mgmProto.SyncResponse) { func (e *Engine) persistSyncResponse(update *mgmProto.SyncResponse) {
// Only persist updates that carry a network map. Config-only updates (e.g. relay // 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 // token rotation, STUN/TURN) carry none; persisting them would overwrite the last
// the last full map on disk and break restore-on-restart. // full map on disk and break restore-on-restart.
if update.GetNetworkMap() == nil { if !syncCarriesNetworkMap(update) {
return return
} }
+20
View File
@@ -37,6 +37,7 @@ import (
"github.com/netbirdio/netbird/monotime" "github.com/netbirdio/netbird/monotime"
"github.com/netbirdio/netbird/route" "github.com/netbirdio/netbird/route"
mgmt "github.com/netbirdio/netbird/shared/management/client" mgmt "github.com/netbirdio/netbird/shared/management/client"
sharedgrpc "github.com/netbirdio/netbird/shared/management/grpc"
mgmtProto "github.com/netbirdio/netbird/shared/management/proto" mgmtProto "github.com/netbirdio/netbird/shared/management/proto"
"github.com/netbirdio/netbird/shared/netiputil" "github.com/netbirdio/netbird/shared/netiputil"
relayClient "github.com/netbirdio/netbird/shared/relay/client" relayClient "github.com/netbirdio/netbird/shared/relay/client"
@@ -1683,3 +1684,22 @@ func TestEngine_SyncResponsePersistence(t *testing.T) {
_, err = e.GetLatestSyncResponse() _, err = e.GetLatestSyncResponse()
require.Error(t, err) 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")
}
+17 -4
View File
@@ -41,7 +41,7 @@ type mapStateManager struct {
// persist snapshots an update to disk for restore-on-restart. Called once per // persist snapshots an update to disk for restore-on-restart. Called once per
// update received from management (in SetTarget), including ones later coalesced // update received from management (in SetTarget), including ones later coalesced
// or skipped from apply, so the on-disk state mirrors what management last sent. // 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) persist func(*mgmProto.SyncResponse)
mu sync.Mutex mu sync.Mutex
@@ -75,7 +75,7 @@ func (m *mapStateManager) SetTarget(update *mgmProto.SyncResponse) error {
} }
m.target = m.mergeTarget(m.target, update) m.target = m.mergeTarget(m.target, update)
// Bump an internal generation counter, NOT the map serial: config-only updates // 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 // serial, yet must still be applied. Every SetTarget is therefore a distinct
// target regardless of payload. Map-serial staleness is enforced separately // target regardless of payload. Map-serial staleness is enforced separately
// inside apply (updateNetworkMap). // 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 // 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 // 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 // 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 { if m.persist != nil {
m.persist(update) 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 // 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 // stream (see receiveUpdatesEvents — not reused), and persisting this squashed target
// is correct, since it is the current full (superset) desired state. // 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.NetworkMap = prev.GetNetworkMap()
update.NetworkMapEnvelope = prev.GetNetworkMapEnvelope()
update.Version = prev.GetVersion()
update.PeerConfig = prev.GetPeerConfig()
update.Checks = prev.Checks // checks travel with the map update.Checks = prev.Checks // checks travel with the map
} }
if update.GetNetbirdConfig() == nil && prev.GetNetbirdConfig() != nil { if update.GetNetbirdConfig() == nil && prev.GetNetbirdConfig() != nil {
update.NetbirdConfig = prev.GetNetbirdConfig() update.NetbirdConfig = prev.GetNetbirdConfig()
} }
if update.GetSessionExpiresAt() == nil && prev.GetSessionExpiresAt() != nil {
update.SessionExpiresAt = prev.GetSessionExpiresAt()
}
return update 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. // run drives convergence until ctx is done. It is meant to run in its own goroutine.
func (m *mapStateManager) run(ctx context.Context) { func (m *mapStateManager) run(ctx context.Context) {
// passGen is the generation of the most recent apply() call (0 = none). A pass is // passGen is the generation of the most recent apply() call (0 = none). A pass is
+40
View File
@@ -8,7 +8,9 @@ import (
"time" "time"
"github.com/stretchr/testify/require" "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" 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") 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")
}