From fdc51d6f9539756c38b777f091534c804eb2f9a3 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Sun, 25 Oct 2026 22:34:44 +0100 Subject: [PATCH] [management] shift network map data store onto the read replica (#8302) --- .../network_map_db/network_map_data_test.go | 75 ++++- .../network_map/controller/controller.go | 2 +- management/internals/modules/peers/manager.go | 4 + .../internals/modules/peers/manager_test.go | 32 ++ .../service/manager/l4_port_test.go | 3 +- .../reverseproxy/service/manager/manager.go | 31 ++ .../service/manager/manager_test.go | 58 +++- .../network_map_db/factory/db_store.go | 45 ++- .../internals/network_map_db/metrics.go | 96 ++++++ .../network_map_db/network_map_data.go | 163 ++++++++-- .../network_map_db/network_map_data_test.go | 278 +++++++++++++++++- .../internals/network_map_db/shared_types.go | 17 +- management/internals/server/boot.go | 24 ++ management/internals/shared/db/open.go | 5 +- .../internals/shared/requestbuffer/buffer.go | 31 ++ .../shared/requestbuffer/buffer_test.go | 71 +++++ management/server/account.go | 17 +- management/server/account_test.go | 45 ++- management/server/peer.go | 42 ++- management/server/peer_test.go | 68 +++++ management/server/user.go | 13 +- management/server/user_test.go | 22 ++ 22 files changed, 1095 insertions(+), 47 deletions(-) create mode 100644 management/internals/network_map_db/metrics.go diff --git a/integration_tests/management/network_map_db/network_map_data_test.go b/integration_tests/management/network_map_db/network_map_data_test.go index 00c0ec03f..96172bd49 100644 --- a/integration_tests/management/network_map_db/network_map_data_test.go +++ b/integration_tests/management/network_map_db/network_map_data_test.go @@ -10,14 +10,21 @@ import ( "path/filepath" "runtime" "strings" + "sync" "testing" + "time" + "github.com/jackc/pgx/v5/pgxpool" networkmapdb "github.com/netbirdio/netbird/management/internals/network_map_db" "github.com/netbirdio/netbird/management/server/integrations/integrated_validator" "github.com/netbirdio/netbird/management/server/settings" "github.com/netbirdio/netbird/management/server/types" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/metricdata" "go.uber.org/mock/gomock" ) @@ -29,6 +36,19 @@ var goldenNMap string const EnvUpdateGoldenData = "NMAP_UPDATE_GOLDEN_DATA" +var seedNetworkMapDataOnce sync.Once + +func seedNetworkMapData(t *testing.T, ctx context.Context) { + t.Helper() + seedNetworkMapDataOnce.Do(func() { + for _, query := range strings.Split(nmapData, ";") { + if err := store(t).Exec(ctx, query); err != nil { + log.Fatalf("error initializing nmap test: %s", err.Error()) + } + } + }) +} + func TestGetNetworkMapData(t *testing.T) { ctx := context.TODO() @@ -51,11 +71,7 @@ func TestGetNetworkMapData(t *testing.T) { IntegratedPeerValidator: peerValidators, } - for _, query := range strings.Split(nmapData, ";") { - if err := store(t).Exec(ctx, query); err != nil { - log.Fatalf("error initializing nmap test: %s", err.Error()) - } - } + seedNetworkMapData(t, ctx) nmap, err := storeImpl.GetNetworkMapData(ctx, "account-33") assert.NoError(t, err) @@ -72,3 +88,52 @@ func TestGetNetworkMapData(t *testing.T) { } assert.Equal(t, goldenNMap, string(serializedNMap)) } + +func TestGetNetworkMapData_FromReplica(t *testing.T) { + if engine != string(types.PostgresStoreEngine) { + t.Skip("replicas exist only for Postgres") + } + ctx := context.TODO() + + extraSettingsManager := settings.NewMockManager(gomock.NewController(t)) + extraSettingsManager.EXPECT().GetExtraSettings(gomock.Any(), gomock.Any()).Return(&types.ExtraSettings{}, nil) + peerValidators := integrated_validator.NewMockIntegratedValidator(gomock.NewController(t)) + peerValidators.EXPECT().GetValidatedPeers(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(map[string]struct{}{}, nil) + + seedNetworkMapData(t, ctx) + + reader := sdkmetric.NewManualReader() + metrics, err := networkmapdb.NewMetrics(sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)).Meter("test"), + map[string]*pgxpool.Pool{"replica": pgstore.Pool}) + require.NoError(t, err) + + primary := store(t) + storeImpl := networkmapdb.NetworkMapDBStoreImpl{ + Metrics: metrics, + Store: primary, + Replica: primary, + ReplicaWait: time.Second, + ExtraSettingsManager: extraSettingsManager, + IntegratedPeerValidator: peerValidators, + } + + nmap, err := storeImpl.GetNetworkMapData(ctx, "account-33") + require.NoError(t, err) + assert.NotEmpty(t, nmap.Peers, "data must be read through the replica transaction after its attempt deadline is released") + + var rm metricdata.ResourceMetrics + require.NoError(t, reader.Collect(ctx, &rm)) + var totalConns int64 + for _, sm := range rm.ScopeMetrics { + for _, m := range sm.Metrics { + if gauge, ok := m.Data.(metricdata.Gauge[int64]); ok { + for _, dp := range gauge.DataPoints { + if state, _ := dp.Attributes.Value(attribute.Key("state")); state.AsString() == "total" { + totalConns += dp.Value + } + } + } + } + } + assert.Positive(t, totalConns, "the replica pool gauge reports its open connections") +} diff --git a/management/internals/controllers/network_map/controller/controller.go b/management/internals/controllers/network_map/controller/controller.go index 08d018e5f..0554811db 100644 --- a/management/internals/controllers/network_map/controller/controller.go +++ b/management/internals/controllers/network_map/controller/controller.go @@ -110,7 +110,7 @@ func NewController(ctx context.Context, store store.Store, metrics telemetry.App if nmdataStore != nil { interval := requestbuffer.Interval(ctx, "NB_NETWORK_MAP_DATA_BUFFER_INTERVAL", defaultNetworkMapDataBufferInterval) log.WithContext(ctx).Infof("set network map data request buffer interval to %s", interval) - c.nmdataBuffer = requestbuffer.New(ctx, "network map data request buffer", interval, c.fetchNetworkMapData) + c.nmdataBuffer = requestbuffer.NewSerialized(ctx, "network map data request buffer", interval, c.fetchNetworkMapData) } return c diff --git a/management/internals/modules/peers/manager.go b/management/internals/modules/peers/manager.go index 6f9705efe..b49667948 100644 --- a/management/internals/modules/peers/manager.go +++ b/management/internals/modules/peers/manager.go @@ -199,6 +199,10 @@ func (m *managerImpl) DeletePeers(ctx context.Context, accountID string, peerIDs return err } + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + log.WithContext(ctx).Debugf("DeletePeers: deleted peer %s", peerID) if !(peer.ProxyMeta.Embedded || peer.Meta.KernelVersion == "wasm") { diff --git a/management/internals/modules/peers/manager_test.go b/management/internals/modules/peers/manager_test.go index 2645276b7..eba565fbf 100644 --- a/management/internals/modules/peers/manager_test.go +++ b/management/internals/modules/peers/manager_test.go @@ -5,6 +5,8 @@ import ( "fmt" "testing" + "github.com/netbirdio/netbird/management/internals/controllers/network_map" + "github.com/netbirdio/netbird/management/server/account" "github.com/netbirdio/netbird/management/server/peer" "github.com/netbirdio/netbird/management/server/permissions" "github.com/netbirdio/netbird/management/server/permissions/modules" @@ -195,3 +197,33 @@ func TestManager_GetPeersPaginated_NoPermission_User_IsRestrictable_ViewBlocked( assert.Equal(t, 0, total) assert.NoError(t, err) } + +func TestManager_DeletePeers_IncrementsNetworkSerial(t *testing.T) { + ctrl := gomock.NewController(t) + mockStore := store.NewMockStore(ctrl) + txStore := store.NewMockStore(ctrl) + accountManager := account.NewMockManager(ctrl) + networkMapController := network_map.NewMockController(ctrl) + + m := NewManager(mockStore, permissions.NewMockManager(ctrl)).(*managerImpl) + m.accountManager = accountManager + m.networkMapController = networkMapController + + ctx := context.Background() + mockStore.EXPECT().GetAccountSettings(gomock.Any(), store.LockingStrengthNone, "test-account").Return(&types.Settings{}, nil) + networkMapController.EXPECT().GetDNSDomain(gomock.Any()).Return("netbird.selfhosted") + mockStore.EXPECT().ExecuteInTransaction(gomock.Any(), gomock.Any()).Times(2). + DoAndReturn(func(_ context.Context, fn func(store.Store) error) error { return fn(txStore) }) + + for _, id := range []string{"peer-1", "peer-2"} { + txStore.EXPECT().GetPeerByID(gomock.Any(), store.LockingStrengthNone, "test-account", id).Return(&peer.Peer{ID: id, Status: &peer.PeerStatus{}}, nil) + txStore.EXPECT().RemovePeerFromAllGroups(gomock.Any(), id).Return(nil) + txStore.EXPECT().GetPolicyRulesByResourceID(gomock.Any(), store.LockingStrengthNone, "test-account", id).Return(nil, nil) + txStore.EXPECT().DeletePeer(gomock.Any(), "test-account", id).Return(nil) + } + txStore.EXPECT().IncrementNetworkSerial(gomock.Any(), "test-account").Times(2).Return(nil) + accountManager.EXPECT().StoreEvent(gomock.Any(), "test-user", gomock.Any(), "test-account", gomock.Any(), gomock.Any()).Times(2) + accountManager.EXPECT().UpdateAccountPeers(gomock.Any(), "test-account", gomock.Any()) + + assert.NoError(t, m.DeletePeers(ctx, "test-account", []string{"peer-1", "peer-2"}, "test-user", false)) +} diff --git a/management/internals/modules/reverseproxy/service/manager/l4_port_test.go b/management/internals/modules/reverseproxy/service/manager/l4_port_test.go index a44e759c4..5ec34a073 100644 --- a/management/internals/modules/reverseproxy/service/manager/l4_port_test.go +++ b/management/internals/modules/reverseproxy/service/manager/l4_port_test.go @@ -6,9 +6,9 @@ import ( "testing" "time" - "go.uber.org/mock/gomock" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/proxy" rpservice "github.com/netbirdio/netbird/management/internals/modules/reverseproxy/service" @@ -38,6 +38,7 @@ func setupL4Test(t *testing.T, customPortsSupported *bool) (*Manager, store.Stor err = testStore.SaveAccount(ctx, &types.Account{ Id: testAccountID, CreatedBy: testUserID, + Network: types.NewNetwork(), Settings: &types.Settings{ PeerExposeEnabled: true, PeerExposeGroups: []string{testGroupID}, diff --git a/management/internals/modules/reverseproxy/service/manager/manager.go b/management/internals/modules/reverseproxy/service/manager/manager.go index ebb52c74b..6173a381d 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager.go +++ b/management/internals/modules/reverseproxy/service/manager/manager.go @@ -363,6 +363,10 @@ func (m *Manager) persistNewService(ctx context.Context, accountID string, svc * return fmt.Errorf("create service: %w", err) } + if err := transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + return nil }) } @@ -534,6 +538,10 @@ func (m *Manager) persistNewEphemeralService(ctx context.Context, accountID, pee return fmt.Errorf("create service: %w", err) } + if err := transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + return nil }) } @@ -744,6 +752,10 @@ func (m *Manager) executeServiceUpdate(ctx context.Context, transaction store.St return fmt.Errorf("update service: %w", err) } + if err := transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + return nil } @@ -1027,6 +1039,10 @@ func (m *Manager) DeleteService(ctx context.Context, accountID, userID, serviceI return fmt.Errorf("failed to delete service: %w", err) } + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + return nil }) if err != nil { @@ -1069,6 +1085,12 @@ func (m *Manager) DeleteAllServices(ctx context.Context, accountID, userID strin } } + if len(services) > 0 { + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + } + return nil }) if err != nil { @@ -1425,6 +1447,10 @@ func (m *Manager) deletePeerService(ctx context.Context, accountID, peerID, serv return fmt.Errorf("delete service: %w", err) } + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + return nil }) if err != nil { @@ -1477,6 +1503,11 @@ func (m *Manager) deleteExpiredPeerService(ctx context.Context, accountID, peerI if err = transaction.DeleteService(ctx, accountID, serviceID); err != nil { return fmt.Errorf("delete service: %w", err) } + + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + deleted = true return nil diff --git a/management/internals/modules/reverseproxy/service/manager/manager_test.go b/management/internals/modules/reverseproxy/service/manager/manager_test.go index f9273d594..85d7c4574 100644 --- a/management/internals/modules/reverseproxy/service/manager/manager_test.go +++ b/management/internals/modules/reverseproxy/service/manager/manager_test.go @@ -227,6 +227,13 @@ func TestCheckDomainAvailable_EdgeCases(t *testing.T) { }) } +func networkSerial(t *testing.T, s store.Store, accountID string) uint64 { + t.Helper() + account, err := s.GetAccount(context.Background(), accountID) + require.NoError(t, err) + return account.Network.Serial +} + func TestPersistNewService(t *testing.T) { ctx := context.Background() accountID := "test-account" @@ -254,6 +261,9 @@ func TestPersistNewService(t *testing.T) { txMock.EXPECT(). CreateService(ctx, service). Return(nil) + txMock.EXPECT(). + IncrementNetworkSerial(ctx, accountID). + Return(nil) return fn(txMock) }) @@ -465,6 +475,9 @@ func TestDeletePeerService_SourcePeerValidation(t *testing.T) { txMock.EXPECT(). DeleteService(ctx, accountID, serviceID). Return(nil) + txMock.EXPECT(). + IncrementNetworkSerial(ctx, accountID). + Return(nil) return fn(txMock) }) mockStore.EXPECT(). @@ -570,6 +583,9 @@ func TestDeletePeerService_SourcePeerValidation(t *testing.T) { txMock.EXPECT(). DeleteService(ctx, accountID, serviceID). Return(nil) + txMock.EXPECT(). + IncrementNetworkSerial(ctx, accountID). + Return(nil) return fn(txMock) }) mockStore.EXPECT(). @@ -617,6 +633,9 @@ func TestDeletePeerService_SourcePeerValidation(t *testing.T) { txMock.EXPECT(). DeleteService(ctx, accountID, serviceID). Return(nil) + txMock.EXPECT(). + IncrementNetworkSerial(ctx, accountID). + Return(nil) return fn(txMock) }) mockStore.EXPECT(). @@ -679,6 +698,7 @@ func setupIntegrationTest(t *testing.T) (*Manager, store.Store) { err = testStore.SaveAccount(ctx, &types.Account{ Id: testAccountID, CreatedBy: testUserID, + Network: types.NewNetwork(), Settings: &types.Settings{ PeerExposeEnabled: true, PeerExposeGroups: []string{testGroupID}, @@ -843,6 +863,7 @@ func TestCreateServiceFromPeer(t *testing.T) { assert.Equal(t, rpservice.SourceEphemeral, persisted.Source, "source should be ephemeral") assert.Equal(t, testPeerID, persisted.SourcePeer, "source peer should be set") assert.NotNil(t, persisted.Meta.LastRenewedAt, "last renewed should be set") + assert.Equal(t, uint64(1), networkSerial(t, testStore, testAccountID), "network serial should be incremented") }) t.Run("creates service with custom domain", func(t *testing.T) { @@ -1049,8 +1070,10 @@ func TestDeleteService_DeletesEphemeralExpose(t *testing.T) { svc, err := testStore.GetServiceByDomain(ctx, resp.Domain) require.NoError(t, err) + serial := networkSerial(t, testStore, testAccountID) err = mgr.DeleteService(ctx, testAccountID, testUserID, svc.ID) require.NoError(t, err) + assert.Equal(t, serial+1, networkSerial(t, testStore, testAccountID), "network serial should be incremented") count, err = mgr.store.CountEphemeralServicesByPeer(ctx, store.LockingStrengthNone, testAccountID, testPeerID) require.NoError(t, err) @@ -1065,7 +1088,7 @@ func TestDeleteService_DeletesEphemeralExpose(t *testing.T) { func TestDeleteAllServices_DeletesEphemeralExposes(t *testing.T) { ctx := context.Background() - mgr, _ := setupIntegrationTest(t) + mgr, testStore := setupIntegrationTest(t) for i := range 3 { _, err := mgr.CreateServiceFromPeer(ctx, testAccountID, testPeerID, &rpservice.ExposeServiceRequest{ @@ -1079,8 +1102,10 @@ func TestDeleteAllServices_DeletesEphemeralExposes(t *testing.T) { require.NoError(t, err) assert.Equal(t, int64(3), count, "all ephemeral services should exist") + serial := networkSerial(t, testStore, testAccountID) err = mgr.DeleteAllServices(ctx, testAccountID, testUserID) require.NoError(t, err) + assert.Equal(t, serial+1, networkSerial(t, testStore, testAccountID), "network serial should be incremented") count, err = mgr.store.CountEphemeralServicesByPeer(ctx, store.LockingStrengthNone, testAccountID, testPeerID) require.NoError(t, err) @@ -1223,8 +1248,10 @@ func TestDeleteExpiredPeerService_DeletesTargets(t *testing.T) { require.Len(t, targets, 1, "ephemeral peer-exposed service should have exactly one persisted target before reaping") expireEphemeralService(t, testStore, testAccountID, resp.Domain) + serial := networkSerial(t, testStore, testAccountID) err = mgr.deleteExpiredPeerService(ctx, testAccountID, testPeerID, svcID) require.NoError(t, err) + assert.Equal(t, serial+1, networkSerial(t, testStore, testAccountID), "network serial should be incremented") _, err = testStore.GetServiceByDomain(ctx, resp.Domain) require.Error(t, err, "expired peer-exposed service should be deleted") @@ -1253,8 +1280,10 @@ func TestDeleteServiceFromPeer_DeletesTargets(t *testing.T) { require.NoError(t, err) require.Len(t, targets, 1, "ephemeral peer-exposed service should have exactly one persisted target before stopping") + serial := networkSerial(t, testStore, testAccountID) err = mgr.StopServiceFromPeer(ctx, testAccountID, testPeerID, svcID) require.NoError(t, err) + assert.Equal(t, serial+1, networkSerial(t, testStore, testAccountID), "network serial should be incremented") _, err = testStore.GetServiceByDomain(ctx, resp.Domain) require.Error(t, err, "stopped peer-exposed service should be deleted") @@ -1521,3 +1550,30 @@ func TestReloadAllServicesForAccount_ContinuesAfterFailedService(t *testing.T) { assert.Contains(t, err.Error(), "svc-broken") assert.NotContains(t, err.Error(), "svc-healthy") } + +func TestServiceWrites_IncrementNetworkSerial(t *testing.T) { + ctx := context.Background() + mgr, testStore, _ := setupL4Test(t, nil) + + created, err := mgr.CreateService(ctx, testAccountID, testUserID, &rpservice.Service{ + Name: "serial", Domain: "serial.test.netbird.io", Mode: rpservice.ModeHTTP, Enabled: true, + Targets: []*rpservice.Target{{ + AccountID: testAccountID, TargetId: testPeerID, + TargetType: rpservice.TargetTypePeer, Protocol: "http", Port: 8080, Enabled: true, + }}, + }) + require.NoError(t, err) + serial := networkSerial(t, testStore, testAccountID) + assert.Equal(t, uint64(1), serial, "create should increment the network serial") + + updated := created.Copy() + updated.Name = "serial renamed" + _, err = mgr.UpdateService(ctx, testAccountID, testUserID, updated) + require.NoError(t, err) + assert.Equal(t, serial+1, networkSerial(t, testStore, testAccountID), "update should increment the network serial") + serial++ + + require.NoError(t, mgr.SetStatus(ctx, testAccountID, created.ID, rpservice.StatusActive)) + require.NoError(t, mgr.SetCertificateIssuedAt(ctx, testAccountID, created.ID)) + assert.Equal(t, serial, networkSerial(t, testStore, testAccountID), "meta-only writes must not increment the network serial") +} diff --git a/management/internals/network_map_db/factory/db_store.go b/management/internals/network_map_db/factory/db_store.go index 3eea0ae69..e60c1f6f9 100644 --- a/management/internals/network_map_db/factory/db_store.go +++ b/management/internals/network_map_db/factory/db_store.go @@ -5,10 +5,15 @@ import ( "errors" "fmt" "os" + "time" + + "github.com/jackc/pgx/v5/pgxpool" + "go.opentelemetry.io/otel/metric" networkmapdb "github.com/netbirdio/netbird/management/internals/network_map_db" networkmap_pgsql "github.com/netbirdio/netbird/management/internals/network_map_db/pgsql" networkmap_sqlite "github.com/netbirdio/netbird/management/internals/network_map_db/sqlite" + "github.com/netbirdio/netbird/management/internals/shared/requestbuffer" "github.com/netbirdio/netbird/management/server/integrations/integrated_validator" "github.com/netbirdio/netbird/management/server/settings" "github.com/netbirdio/netbird/management/server/store" @@ -16,7 +21,16 @@ import ( log "github.com/sirupsen/logrus" ) -const storeSqliteFileName = "store.db" +const ( + storeSqliteFileName = "store.db" + + // ReplicaDsnEnv names the Postgres DSN of a read replica serving network map data reads. + ReplicaDsnEnv = "NB_STORE_ENGINE_POSTGRES_REPLICA_DSN" + // replicaWaitEnv bounds how long a network map data read waits for the replica. + replicaWaitEnv = "NB_NETWORK_MAP_DATA_REPLICA_WAIT" + + defaultReplicaWait = 2 * time.Second +) var ErrNotSupportedStoreEngine = errors.New("unsupported store engine") @@ -25,7 +39,9 @@ func NewNetworkMapDBStore( kind types.Engine, dataDir string, integratedPeerValidator integrated_validator.IntegratedValidator, - extraSettingsManager settings.Manager) (*networkmapdb.NetworkMapDBStoreImpl, error) { + extraSettingsManager settings.Manager, + replicaPool *pgxpool.Pool, + meter metric.Meter) (*networkmapdb.NetworkMapDBStoreImpl, error) { switch kind { case types.SqliteStoreEngine: log.WithContext(ctx).Info("networkmap store is using SQLite") @@ -54,11 +70,32 @@ func NewNetworkMapDBStore( return nil, err } - return &networkmapdb.NetworkMapDBStoreImpl{ + impl := &networkmapdb.NetworkMapDBStoreImpl{ Store: store, IntegratedPeerValidator: integratedPeerValidator, ExtraSettingsManager: extraSettingsManager, - }, nil + } + + pools := map[string]*pgxpool.Pool{"primary": store.Pool} + if replicaPool != nil { + impl.Replica = &networkmap_pgsql.PgStore{Pool: replicaPool} + impl.ReplicaWait = requestbuffer.Interval(ctx, replicaWaitEnv, defaultReplicaWait) + pools["replica"] = replicaPool + replicaConfig := replicaPool.Config().ConnConfig + log.WithContext(ctx).Infof("networkmap store reads from Postgres replica %s:%d/%s, waiting up to %s for replication", + replicaConfig.Host, replicaConfig.Port, replicaConfig.Database, impl.ReplicaWait) + } else { + log.WithContext(ctx).Info("networkmap store reads from the primary, no replica configured") + } + + if meter != nil { + impl.Metrics, err = networkmapdb.NewMetrics(meter, pools) + if err != nil { + return nil, fmt.Errorf("create network map data metrics: %w", err) + } + } + + return impl, nil } return nil, fmt.Errorf("networkmap store doesn't support engine %s, %w", kind, ErrNotSupportedStoreEngine) diff --git a/management/internals/network_map_db/metrics.go b/management/internals/network_map_db/metrics.go new file mode 100644 index 000000000..0cfd8f92d --- /dev/null +++ b/management/internals/network_map_db/metrics.go @@ -0,0 +1,96 @@ +package networkmapdb + +import ( + "context" + "fmt" + "time" + + "github.com/jackc/pgx/v5/pgxpool" + "go.opentelemetry.io/otel/attribute" + "go.opentelemetry.io/otel/metric" +) + +const ( + readSourceReplica = "replica" + readSourcePrimary = "primary" + + readReasonNoReplica = "no_replica" + readReasonCaughtUp = "caught_up" + readReasonLagTimeout = "lag_timeout" + readReasonReplicaError = "replica_error" + readReasonSerialReadError = "serial_read_error" +) + +// Metrics records which database serves network map data reads, how long reads +// wait for the replica and the state of the connection pools. +type Metrics struct { + reads metric.Int64Counter + replicaWait metric.Int64Histogram +} + +// NewMetrics creates the network map data metrics on meter and observes the +// connection pools keyed by pool name. +func NewMetrics(meter metric.Meter, pools map[string]*pgxpool.Pool) (*Metrics, error) { + reads, err := meter.Int64Counter("management.nmdata.reads", + metric.WithUnit("1"), + metric.WithDescription("Network map data reads by the database that served them and the reason it was chosen"), + ) + if err != nil { + return nil, fmt.Errorf("create reads counter: %w", err) + } + + replicaWait, err := meter.Int64Histogram("management.nmdata.replica.wait.ms", + metric.WithUnit("milliseconds"), + metric.WithDescription("Time a network map data read waited for the replica to replicate the primary's network serial"), + ) + if err != nil { + return nil, fmt.Errorf("create replica wait histogram: %w", err) + } + + connections, err := meter.Int64ObservableGauge("management.nmdata.pool.connections", + metric.WithUnit("1"), + metric.WithDescription("Connections of the network map data connection pools by state"), + ) + if err != nil { + return nil, fmt.Errorf("create pool connections gauge: %w", err) + } + + _, err = meter.RegisterCallback(func(_ context.Context, observer metric.Observer) error { + for name, pool := range pools { + stat := pool.Stat() + observePool(observer, connections, name, "acquired", stat.AcquiredConns()) + observePool(observer, connections, name, "idle", stat.IdleConns()) + observePool(observer, connections, name, "total", stat.TotalConns()) + } + return nil + }, connections) + if err != nil { + return nil, fmt.Errorf("create pool connections callback: %w", err) + } + + return &Metrics{reads: reads, replicaWait: replicaWait}, nil +} + +func observePool(observer metric.Observer, gauge metric.Int64ObservableGauge, pool, state string, value int32) { + observer.ObserveInt64(gauge, int64(value), metric.WithAttributes( + attribute.String("pool", pool), + attribute.String("state", state), + )) +} + +func (m *Metrics) countRead(ctx context.Context, source, reason string) { + if m == nil { + return + } + m.reads.Add(ctx, 1, metric.WithAttributes( + attribute.String("source", source), + attribute.String("reason", reason), + )) +} + +func (m *Metrics) recordReplicaWait(ctx context.Context, wait time.Duration) { + if m == nil { + return + } + m.replicaWait.Record(ctx, wait.Milliseconds()) +} diff --git a/management/internals/network_map_db/network_map_data.go b/management/internals/network_map_db/network_map_data.go index 1e83bfb34..2c1e7abcc 100644 --- a/management/internals/network_map_db/network_map_data.go +++ b/management/internals/network_map_db/network_map_data.go @@ -2,7 +2,9 @@ package networkmapdb import ( "context" + "errors" "fmt" + "time" log "github.com/sirupsen/logrus" "golang.org/x/exp/maps" @@ -11,12 +13,147 @@ import ( "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" ) +// GetNetworkMapData reads the network map data of the account. With a Replica +// configured it reads from the replica once that has replicated the account's +// current network serial, and from Store when it does not catch up within +// ReplicaWait or the replica read fails. func (s *NetworkMapDBStoreImpl) GetNetworkMapData(ctx context.Context, accountId string) (*networkmap.NetworkMapData, error) { - tx, err := s.Store.BeginTx(ctx) + toret, err := s.readNetworkMapData(ctx, accountId) if err != nil { return nil, err } + extraSettings, err := s.ExtraSettingsManager.GetExtraSettings(ctx, accountId) + if err != nil { + return nil, err + } + + toret.ValidatedPeers, err = s.IntegratedPeerValidator.GetValidatedPeers(ctx, accountId, maps.Values(toret.Groups), maps.Values(toret.Peers), extraSettings) + if err != nil { + return nil, err + } + + return toret, nil +} + +func (s *NetworkMapDBStoreImpl) readNetworkMapData(ctx context.Context, accountId string) (*networkmap.NetworkMapData, error) { + reason := readReasonNoReplica + if s.Replica != nil { + toret, err := s.readFromReplica(ctx, accountId) + if err == nil { + s.Metrics.countRead(ctx, readSourceReplica, readReasonCaughtUp) + return toret, nil + } + if ctx.Err() != nil { + return nil, ctx.Err() + } + reason = fallbackReason(err) + if reason == readReasonLagTimeout { + log.WithContext(ctx).Debugf("reading network map data of account %s from primary: %v", accountId, err) + } else { + log.WithContext(ctx).Warnf("reading network map data of account %s from primary: %v", accountId, err) + } + } + + tx, network, err := beginRead(ctx, s.Store, accountId) + if err != nil { + return nil, err + } + toret, err := readFromTx(ctx, tx, accountId, network) + if err != nil { + return nil, err + } + s.Metrics.countRead(ctx, readSourcePrimary, reason) + return toret, nil +} + +func fallbackReason(err error) string { + switch { + case errors.Is(err, errReplicaLagging): + return readReasonLagTimeout + case errors.Is(err, errPrimarySerialRead): + return readReasonSerialReadError + default: + return readReasonReplicaError + } +} + +func (s *NetworkMapDBStoreImpl) readFromReplica(ctx context.Context, accountId string) (*networkmap.NetworkMapData, error) { + tx, network, err := s.beginOnReplica(ctx, accountId) + if err != nil { + return nil, err + } + return readFromTx(ctx, tx, accountId, network) +} + +// beginOnReplica opens a read transaction on the replica whose snapshot holds at +// least the account's network serial on the primary at the time of the call. +func (s *NetworkMapDBStoreImpl) beginOnReplica(ctx context.Context, accountId string) (NetworkMapDBStoreConn, nmdata.Network, error) { + primaryTx, primaryNetwork, err := beginRead(ctx, s.Store, accountId) + if err != nil { + return nil, nmdata.Network{}, fmt.Errorf("%w: %w", errPrimarySerialRead, err) + } + rollbackTx(ctx, primaryTx) + + start := time.Now() + deadline := start.Add(s.ReplicaWait) + + for { + tx, network, err := beginReadBefore(ctx, s.Replica, accountId, deadline) + if err != nil { + if ctx.Err() == nil && errors.Is(err, context.DeadlineExceeded) { + s.Metrics.recordReplicaWait(ctx, time.Since(start)) + return nil, nmdata.Network{}, fmt.Errorf("%w: no answer within %s: %w", errReplicaLagging, s.ReplicaWait, err) + } + return nil, nmdata.Network{}, err + } + if network.Serial >= primaryNetwork.Serial { + s.Metrics.recordReplicaWait(ctx, time.Since(start)) + return tx, network, nil + } + rollbackTx(ctx, tx) + + remaining := time.Until(deadline) + if remaining <= 0 { + s.Metrics.recordReplicaWait(ctx, time.Since(start)) + return nil, nmdata.Network{}, fmt.Errorf("%w: at network serial %d after %s, primary at %d", errReplicaLagging, network.Serial, s.ReplicaWait, primaryNetwork.Serial) + } + + select { + case <-ctx.Done(): + return nil, nmdata.Network{}, ctx.Err() + case <-time.After(min(replicaPollInterval, remaining)): + } + } +} + +// beginReadBefore runs beginRead with its connect, begin and network read bounded +// by deadline, so a stalled replica cannot hold the read past it. +func beginReadBefore(ctx context.Context, store NetworkMapDBStore, accountId string, deadline time.Time) (NetworkMapDBStoreConn, nmdata.Network, error) { + attemptCtx, cancel := context.WithDeadline(ctx, deadline) + defer cancel() + return beginRead(attemptCtx, store, accountId) +} + +// beginRead opens a read transaction and reads the account's network as its +// first statement, so the network serial belongs to the transaction's snapshot. +func beginRead(ctx context.Context, store NetworkMapDBStore, accountId string) (NetworkMapDBStoreConn, nmdata.Network, error) { + tx, err := store.BeginTx(ctx) + if err != nil { + return nil, nmdata.Network{}, err + } + + network, err := tx.GetNetwork(ctx, accountId) + if err != nil { + rollbackTx(ctx, tx) + return nil, nmdata.Network{}, fmt.Errorf("get network: %w", err) + } + + return tx, network, nil +} + +// readFromTx reads the remaining network map data in tx and ends it. +func readFromTx(ctx context.Context, tx NetworkMapDBStoreConn, accountId string, network nmdata.Network) (*networkmap.NetworkMapData, error) { acctSettings, err := tx.GetAccountSettings(ctx, accountId) if err != nil { return rollbackAndReturnError(ctx, tx, fmt.Errorf("failed to get account settings: %w", err)) @@ -41,10 +178,6 @@ func (s *NetworkMapDBStoreImpl) GetNetworkMapData(ctx context.Context, accountId if err != nil { return rollbackAndReturnError(ctx, tx, fmt.Errorf("failed to get network routers: %w", err)) } - network, err := tx.GetNetwork(ctx, accountId) - if err != nil { - return rollbackAndReturnError(ctx, tx, fmt.Errorf("failed to get network: %w", err)) - } peers, _, err := tx.GetPeers(ctx, accountId) if err != nil { return rollbackAndReturnError(ctx, tx, fmt.Errorf("failed to get peers: %w", err)) @@ -111,26 +244,20 @@ func (s *NetworkMapDBStoreImpl) GetNetworkMapData(ctx context.Context, accountId ProxyTargetedDomainResourceIDs: proxyTargetedDomainResourceIDs, } - extraSettings, err := s.ExtraSettingsManager.GetExtraSettings(ctx, accountId) - if err != nil { - return nil, err - } - - toret.ValidatedPeers, err = s.IntegratedPeerValidator.GetValidatedPeers(ctx, accountId, maps.Values(toret.Groups), maps.Values(toret.Peers), extraSettings) - if err != nil { - return nil, err - } - return &toret, nil } func rollbackAndReturnError(ctx context.Context, tx NetworkMapDBStoreConn, err error) (*networkmap.NetworkMapData, error) { - if errr := tx.RollbackTx(ctx); errr != nil { - log.WithContext(ctx).Warnf("failed to rollback network map read transaction: %v", errr) - } + rollbackTx(ctx, tx) return nil, err } +func rollbackTx(ctx context.Context, tx NetworkMapDBStoreConn) { + if err := tx.RollbackTx(ctx); err != nil { + log.WithContext(ctx).Warnf("failed to rollback network map read transaction: %v", err) + } +} + func toMap[T any](all []T, id func(t T) string) map[string]*T { toret := make(map[string]*T, len(all)) for _, t := range all { diff --git a/management/internals/network_map_db/network_map_data_test.go b/management/internals/network_map_db/network_map_data_test.go index 2b694f5bc..d2d24ecc6 100644 --- a/management/internals/network_map_db/network_map_data_test.go +++ b/management/internals/network_map_db/network_map_data_test.go @@ -1,10 +1,21 @@ package networkmapdb import ( + "context" + "errors" + "sync/atomic" "testing" + "testing/synctest" + "time" - "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "go.opentelemetry.io/otel/attribute" + sdkmetric "go.opentelemetry.io/otel/sdk/metric" + "go.opentelemetry.io/otel/sdk/metric/metricdata" + + "github.com/netbirdio/netbird/shared/management/networkmap" + "github.com/netbirdio/netbird/shared/management/networkmap/nmdata" ) // disabled network resource shouldn't be in the resulting map @@ -156,3 +167,268 @@ func TestBuildResourcePolicies_ViaPolicyToDestinationGroupIdx(t *testing.T) { }, }, resourceToPolicies) } + +type fakeNmdataStore struct { + name string + serial atomic.Int64 + readErr error + stall bool + begins atomic.Int32 + finishes atomic.Int32 +} + +func (f *fakeNmdataStore) BeginTx(ctx context.Context) (NetworkMapDBStoreConn, error) { + if f.stall { + <-ctx.Done() + return nil, ctx.Err() + } + f.begins.Add(1) + return &fakeNmdataConn{store: f}, nil +} + +func (f *fakeNmdataStore) Exec(context.Context, string, ...any) error { return nil } + +type fakeNmdataConn struct { + store *fakeNmdataStore +} + +func (c *fakeNmdataConn) GetNetwork(context.Context, string) (nmdata.Network, error) { + return nmdata.Network{Identifier: c.store.name, Serial: c.store.serial.Load()}, nil +} + +func (c *fakeNmdataConn) GetPeers(context.Context, string) ([]nmdata.Peer, map[string][]*nmdata.Peer, error) { + return nil, nil, c.store.readErr +} + +func (c *fakeNmdataConn) GetGroups(context.Context, string) ([]nmdata.Group, map[string]map[string]any, error) { + return nil, nil, nil +} +func (c *fakeNmdataConn) GetDomains(context.Context, string) ([]Domain, error) { return nil, nil } +func (c *fakeNmdataConn) GetPolicies(context.Context, string) ([]nmdata.Policy, map[string]map[string]any, map[string]map[string]any, error) { + return nil, nil, nil, nil +} +func (c *fakeNmdataConn) GetRoutes(context.Context, string) ([]nmdata.Route, error) { return nil, nil } +func (c *fakeNmdataConn) GetNameServerGroups(context.Context, string) ([]nmdata.NameServerGroup, error) { + return nil, nil +} +func (c *fakeNmdataConn) GetNetworkResources(context.Context, string) ([]nmdata.NetworkResource, error) { + return nil, nil +} +func (c *fakeNmdataConn) GetNetworkRouters(context.Context, string) (map[string]map[string]*nmdata.NetworkRouter, error) { + return map[string]map[string]*nmdata.NetworkRouter{}, nil +} +func (c *fakeNmdataConn) GetAppliedZoneCandidates(context.Context, string) ([]networkmap.AppliedZoneCandidate, error) { + return nil, nil +} +func (c *fakeNmdataConn) GetAccountSettings(context.Context, string) (nmdata.AccountSettingsInfo, error) { + return nmdata.AccountSettingsInfo{}, nil +} +func (c *fakeNmdataConn) GetPostureChecks(context.Context, string) ([]nmdata.PostureChecks, map[string]string, error) { + return nil, nil, nil +} +func (c *fakeNmdataConn) GetAllowedUsers(context.Context, string) (map[string]struct{}, map[string][]string, error) { + return nil, nil, nil +} +func (c *fakeNmdataConn) GetDnsSettings(context.Context, string) (nmdata.DNSSettings, error) { + return nmdata.DNSSettings{}, nil +} +func (c *fakeNmdataConn) GetNetworkXIDToPublicIdMap(context.Context, string) (map[string]string, error) { + return map[string]string{}, nil +} +func (c *fakeNmdataConn) GetPrivateServices(context.Context, string) ([]Service, error) { + return nil, nil +} +func (c *fakeNmdataConn) GetProxyTargetedDomainResourceIDs(context.Context, string) (map[string]struct{}, error) { + return map[string]struct{}{}, nil +} + +func (c *fakeNmdataConn) CommitTx(context.Context) error { + c.store.finishes.Add(1) + return nil +} + +func (c *fakeNmdataConn) RollbackTx(context.Context) error { + c.store.finishes.Add(1) + return nil +} + +func newReplicaTestStores(primarySerial, replicaSerial int64) (*NetworkMapDBStoreImpl, *fakeNmdataStore, *fakeNmdataStore) { + primary := &fakeNmdataStore{name: "primary"} + primary.serial.Store(primarySerial) + replica := &fakeNmdataStore{name: "replica"} + replica.serial.Store(replicaSerial) + return &NetworkMapDBStoreImpl{Store: primary, Replica: replica, ReplicaWait: time.Second}, primary, replica +} + +func TestReadNetworkMapData_ReplicaCaughtUp(t *testing.T) { + s, primary, replica := newReplicaTestStores(5, 5) + + data, err := s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + assert.Equal(t, "replica", data.Network.Identifier) + assert.Equal(t, int32(1), replica.begins.Load()) + assert.Equal(t, primary.begins.Load(), primary.finishes.Load()) + assert.Equal(t, replica.begins.Load(), replica.finishes.Load()) +} + +func TestReadNetworkMapData_WaitsForReplica(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + s, _, replica := newReplicaTestStores(5, 4) + + time.AfterFunc(100*time.Millisecond, func() { replica.serial.Store(5) }) + + start := time.Now() + data, err := s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + assert.Equal(t, "replica", data.Network.Identifier) + assert.Equal(t, int64(5), data.Network.Serial) + assert.GreaterOrEqual(t, time.Since(start), 100*time.Millisecond) + assert.Equal(t, replica.begins.Load(), replica.finishes.Load()) + }) +} + +func TestReadNetworkMapData_FallsBackToPrimaryWhenReplicaLags(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + s, primary, replica := newReplicaTestStores(5, 4) + + start := time.Now() + data, err := s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + assert.Equal(t, "primary", data.Network.Identifier) + assert.GreaterOrEqual(t, time.Since(start), s.ReplicaWait) + assert.Equal(t, primary.begins.Load(), primary.finishes.Load()) + assert.Equal(t, replica.begins.Load(), replica.finishes.Load()) + }) +} + +func TestReadNetworkMapData_FallsBackToPrimaryOnReplicaReadError(t *testing.T) { + s, primary, replica := newReplicaTestStores(5, 5) + replica.readErr = errors.New("canceling statement due to conflict with recovery") + + data, err := s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + assert.Equal(t, "primary", data.Network.Identifier) + assert.Equal(t, primary.begins.Load(), primary.finishes.Load()) + assert.Equal(t, replica.begins.Load(), replica.finishes.Load()) +} + +func TestReadNetworkMapData_WithoutReplicaReadsPrimaryOnce(t *testing.T) { + primary := &fakeNmdataStore{name: "primary"} + s := &NetworkMapDBStoreImpl{Store: primary} + + data, err := s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + assert.Equal(t, "primary", data.Network.Identifier) + assert.Equal(t, int32(1), primary.begins.Load()) + assert.Equal(t, int32(1), primary.finishes.Load()) +} + +func TestReadNetworkMapData_CancelledWhileWaitingForReplica(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + s, primary, replica := newReplicaTestStores(5, 4) + + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + _, err := s.readNetworkMapData(ctx, "account") + require.ErrorIs(t, err, context.DeadlineExceeded) + + assert.Equal(t, int32(1), primary.begins.Load(), "primary should only be read for the serial") + assert.Equal(t, primary.begins.Load(), primary.finishes.Load()) + assert.Equal(t, replica.begins.Load(), replica.finishes.Load()) + }) +} + +func TestReadNetworkMapData_FallsBackToPrimaryWhenReplicaStalls(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + reader := sdkmetric.NewManualReader() + metrics, err := NewMetrics(sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)).Meter("test"), nil) + require.NoError(t, err) + + s, primary, replica := newReplicaTestStores(5, 5) + s.Metrics = metrics + replica.stall = true + + start := time.Now() + data, err := s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + assert.Equal(t, "primary", data.Network.Identifier) + assert.Equal(t, s.ReplicaWait, time.Since(start), "a stalled replica must be abandoned at the wait deadline") + assert.Equal(t, primary.begins.Load(), primary.finishes.Load()) + + reads, waits := collectReadMetrics(t, reader) + assert.Equal(t, map[[2]string]int64{{readSourcePrimary, readReasonLagTimeout}: 1}, reads) + assert.Equal(t, uint64(1), waits) + }) +} + +func TestReadNetworkMapData_CountsReadsBySourceAndReason(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + reader := sdkmetric.NewManualReader() + metrics, err := NewMetrics(sdkmetric.NewMeterProvider(sdkmetric.WithReader(reader)).Meter("test"), nil) + require.NoError(t, err) + + s, _, replica := newReplicaTestStores(5, 5) + s.Metrics = metrics + + _, err = s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + replica.serial.Store(4) + _, err = s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + replica.serial.Store(5) + replica.readErr = errors.New("canceling statement due to conflict with recovery") + _, err = s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + s.Replica = nil + _, err = s.readNetworkMapData(context.Background(), "account") + require.NoError(t, err) + + reads, waits := collectReadMetrics(t, reader) + + assert.Equal(t, map[[2]string]int64{ + {readSourceReplica, readReasonCaughtUp}: 1, + {readSourcePrimary, readReasonLagTimeout}: 1, + {readSourcePrimary, readReasonReplicaError}: 1, + {readSourcePrimary, readReasonNoReplica}: 1, + }, reads) + assert.Equal(t, uint64(3), waits, "every replica attempt that reached the serial check records its wait") + }) +} + +// collectReadMetrics returns the read counts keyed by source and reason and the +// number of recorded replica waits. +func collectReadMetrics(t *testing.T, reader *sdkmetric.ManualReader) (map[[2]string]int64, uint64) { + t.Helper() + var rm metricdata.ResourceMetrics + require.NoError(t, reader.Collect(context.Background(), &rm)) + + reads := map[[2]string]int64{} + var waits uint64 + for _, sm := range rm.ScopeMetrics { + for _, m := range sm.Metrics { + switch data := m.Data.(type) { + case metricdata.Sum[int64]: + for _, dp := range data.DataPoints { + source, _ := dp.Attributes.Value(attribute.Key("source")) + reason, _ := dp.Attributes.Value(attribute.Key("reason")) + reads[[2]string{source.AsString(), reason.AsString()}] = dp.Value + } + case metricdata.Histogram[int64]: + for _, dp := range data.DataPoints { + waits += dp.Count + } + } + } + } + return reads, waits +} diff --git a/management/internals/network_map_db/shared_types.go b/management/internals/network_map_db/shared_types.go index dc065073c..ca92d4eea 100644 --- a/management/internals/network_map_db/shared_types.go +++ b/management/internals/network_map_db/shared_types.go @@ -7,6 +7,7 @@ import ( "errors" "fmt" "reflect" + "time" "github.com/miekg/dns" "github.com/netbirdio/netbird/management/server/integrations/integrated_validator" @@ -17,6 +18,13 @@ import ( var ErrDnsUnsupportedRecordType = errors.New("unsupported record type") +const replicaPollInterval = 10 * time.Millisecond + +var ( + errReplicaLagging = errors.New("replica lagging") + errPrimarySerialRead = errors.New("read primary network serial") +) + type NetworkMapDBStore interface { //nolint:revive // established name across the codebase BeginTx(ctx context.Context) (NetworkMapDBStoreConn, error) Exec(ctx context.Context, query string, args ...any) error @@ -46,7 +54,14 @@ type NetworkMapDBStoreConn interface { //nolint:revive // established name acros } type NetworkMapDBStoreImpl struct { //nolint:revive // established name across the codebase - Store NetworkMapDBStore + Store NetworkMapDBStore + // Replica is an optional read replica of Store that serves reads once it has + // replicated the account's current network serial. + Replica NetworkMapDBStore + // ReplicaWait bounds how long a read waits for Replica before it falls back to Store. + ReplicaWait time.Duration + // Metrics records how reads are served. It is optional. + Metrics *Metrics IntegratedPeerValidator integrated_validator.IntegratedValidator ExtraSettingsManager settings.Manager } diff --git a/management/internals/server/boot.go b/management/internals/server/boot.go index 28f6294d6..64d5ee4bc 100644 --- a/management/internals/server/boot.go +++ b/management/internals/server/boot.go @@ -8,12 +8,14 @@ import ( "errors" "net/http" "net/netip" + "os" "slices" "time" "github.com/gorilla/mux" grpcMiddleware "github.com/grpc-ecosystem/go-grpc-middleware/v2" "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/realip" + "github.com/jackc/pgx/v5/pgxpool" "github.com/rs/cors" "github.com/rs/xid" log "github.com/sirupsen/logrus" @@ -44,6 +46,7 @@ import ( "github.com/netbirdio/netbird/management/server/idp" "github.com/netbirdio/netbird/management/server/store" "github.com/netbirdio/netbird/management/server/telemetry" + "github.com/netbirdio/netbird/management/server/types" mgmtProto "github.com/netbirdio/netbird/shared/management/proto" "github.com/netbirdio/netbird/shared/ratelimit" "github.com/netbirdio/netbird/util/crypt" @@ -130,6 +133,8 @@ func (s *BaseServer) NetworkMapStore() *networkmapdb.NetworkMapDBStoreImpl { s.Config.Datadir, s.IntegratedValidator(), s.SettingsManager(), + s.NetworkMapReplicaPool(), + s.Metrics().GetMeter(), ) // networkmap db store supports postgres and sqlite backends only // for other backends a fallback is used, so NotSupportedStoreEngineError @@ -141,6 +146,25 @@ func (s *BaseServer) NetworkMapStore() *networkmapdb.NetworkMapDBStoreImpl { }) } +// NetworkMapReplicaPool opens the pgx pool to the Postgres read replica serving +// network map data reads. It is nil unless the store engine is Postgres and +// networkmapdbfactory.ReplicaDsnEnv is set. +func (s *BaseServer) NetworkMapReplicaPool() *pgxpool.Pool { + return CreateNamed(s, "NetworkMapReplica", func() *pgxpool.Pool { + dsn := os.Getenv(networkmapdbfactory.ReplicaDsnEnv) + if dsn == "" || s.Config.StoreConfig.Engine != types.PostgresStoreEngine { + return nil + } + pool, err := db.OpenPgxPool(context.Background(), dsn, db.DefaultPoolConfig) + if err != nil { + log.Errorf("network map data reads use the primary, connecting to the replica failed: %v", err) + return nil + } + s.OnStop(pool.Close) + return pool + }) +} + func (s *BaseServer) EventStore() activity.Store { return Create(s, func() activity.Store { var err error diff --git a/management/internals/shared/db/open.go b/management/internals/shared/db/open.go index 3c3299bb9..f813b0ce8 100644 --- a/management/internals/shared/db/open.go +++ b/management/internals/shared/db/open.go @@ -106,7 +106,7 @@ func OpenPostgres(ctx context.Context, dsn string, pool PoolConfig, opts ...Opti if err != nil { return nil, err } - pgxPool, err := newPgxPool(ctx, dsn, pool) + pgxPool, err := OpenPgxPool(ctx, dsn, pool) if err != nil { closeGorm(gormDB) return nil, err @@ -135,7 +135,8 @@ func OpenPostgresWithoutPool(ctx context.Context, dsn string, opts ...Option) (* return conn, nil } -func newPgxPool(ctx context.Context, dsn string, cfg PoolConfig) (*pgxpool.Pool, error) { +// OpenPgxPool opens a pgx pool to dsn sized by cfg and verifies it with a ping. +func OpenPgxPool(ctx context.Context, dsn string, cfg PoolConfig) (*pgxpool.Pool, error) { config, err := pgxpool.ParseConfig(dsn) if err != nil { return nil, fmt.Errorf("unable to parse database config: %w", err) diff --git a/management/internals/shared/requestbuffer/buffer.go b/management/internals/shared/requestbuffer/buffer.go index c3823776c..22f9d1952 100644 --- a/management/internals/shared/requestbuffer/buffer.go +++ b/management/internals/shared/requestbuffer/buffer.go @@ -26,6 +26,9 @@ type Buffer[T any] struct { mu sync.Mutex waiting map[string][]chan result[T] + // inFlight holds the keys with a running fetch and whether their next flush + // is due once it ends. It is nil unless fetches are serialized. + inFlight map[string]bool } type result[T any] struct { @@ -45,6 +48,15 @@ func New[T any](ctx context.Context, name string, interval time.Duration, fetch } } +// NewSerialized returns a Buffer like New that runs at most one fetch per key at +// a time. Requests arriving during a fetch form the next batch, which is fetched +// once both its window closed and the running fetch ended. +func NewSerialized[T any](ctx context.Context, name string, interval time.Duration, fetch FetchFunc[T]) *Buffer[T] { + b := New(ctx, name, interval, fetch) + b.inFlight = make(map[string]bool) + return b +} + // Get returns the value for key, sharing one fetch with the other callers of // the current batch. The value is shared as is, so callers must treat it as // read-only unless the fetch hands out copies. @@ -71,8 +83,16 @@ func (b *Buffer[T]) Get(ctx context.Context, key string) (T, error) { func (b *Buffer[T]) flush(key string) { b.mu.Lock() + if _, running := b.inFlight[key]; running { + b.inFlight[key] = true + b.mu.Unlock() + return + } waiting := b.waiting[key] delete(b.waiting, key) + if b.inFlight != nil && len(waiting) > 0 { + b.inFlight[key] = false + } b.mu.Unlock() if len(waiting) == 0 { @@ -86,6 +106,17 @@ func (b *Buffer[T]) flush(key string) { for _, ch := range waiting { ch <- result[T]{value: value, err: err} } + + if b.inFlight == nil { + return + } + b.mu.Lock() + due := b.inFlight[key] + delete(b.inFlight, key) + b.mu.Unlock() + if due { + go b.flush(key) + } } // Interval reads a buffer interval from envVar, falling back to def. diff --git a/management/internals/shared/requestbuffer/buffer_test.go b/management/internals/shared/requestbuffer/buffer_test.go index 9e356145e..3bfb73de4 100644 --- a/management/internals/shared/requestbuffer/buffer_test.go +++ b/management/internals/shared/requestbuffer/buffer_test.go @@ -6,6 +6,7 @@ import ( "sync" "sync/atomic" "testing" + "testing/synctest" "time" "github.com/stretchr/testify/assert" @@ -104,3 +105,73 @@ func TestBufferHonorsCallerContext(t *testing.T) { _, err := buffer.Get(ctx, "account") assert.ErrorIs(t, err, context.DeadlineExceeded) } + +func TestSerializedBufferQueuesRequestsDuringFetch(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + var fetches, running, maxRunning atomic.Int32 + buffer := NewSerialized(context.Background(), "test", 10*time.Millisecond, + func(ctx context.Context, key string) (int32, error) { + n := running.Add(1) + defer running.Add(-1) + if n > maxRunning.Load() { + maxRunning.Store(n) + } + time.Sleep(time.Second) + return fetches.Add(1), nil + }) + + var wg sync.WaitGroup + get := func(want int32) { + wg.Add(1) + go func() { + defer wg.Done() + value, err := buffer.Get(context.Background(), "account") + assert.NoError(t, err) + assert.Equal(t, want, value) + }() + } + + get(1) + time.Sleep(100 * time.Millisecond) + synctest.Wait() + require.Equal(t, int32(1), running.Load(), "first fetch should be running") + + for range 5 { + get(2) + } + wg.Wait() + + assert.Equal(t, int32(2), fetches.Load(), "requests queued during the fetch should share one fetch") + assert.Equal(t, int32(1), maxRunning.Load(), "fetches of a key should not overlap") + }) +} + +func TestUnserializedBufferOverlapsFetches(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + var running, maxRunning atomic.Int32 + buffer := New(context.Background(), "test", 10*time.Millisecond, + func(ctx context.Context, key string) (string, error) { + n := running.Add(1) + defer running.Add(-1) + if n > maxRunning.Load() { + maxRunning.Store(n) + } + time.Sleep(time.Second) + return key, nil + }) + + var wg sync.WaitGroup + wg.Go(func() { + _, err := buffer.Get(context.Background(), "account") + assert.NoError(t, err) + }) + time.Sleep(100 * time.Millisecond) + wg.Go(func() { + _, err := buffer.Get(context.Background(), "account") + assert.NoError(t, err) + }) + wg.Wait() + + assert.Equal(t, int32(2), maxRunning.Load()) + }) +} diff --git a/management/server/account.go b/management/server/account.go index ddc8d4bbc..21b9fa431 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -1825,10 +1825,10 @@ func (am *DefaultAccountManager) SyncUserJWTGroups(ctx context.Context, userAuth return fmt.Errorf("reconcile IPv6 for group changes: %w", err) } change.ChangedPeerIDs = append(change.ChangedPeerIDs, ipv6Changed...) + } - if err = transaction.IncrementNetworkSerial(ctx, userAuth.AccountId); err != nil { - return fmt.Errorf("error incrementing network serial: %w", err) - } + if err = transaction.IncrementNetworkSerial(ctx, userAuth.AccountId); err != nil { + return fmt.Errorf("error incrementing network serial: %w", err) } if snap, err = affectedpeers.Load(ctx, transaction, userAuth.AccountId, change); err != nil { @@ -2844,6 +2844,10 @@ func (am *DefaultAccountManager) savePeerIPUpdate(ctx context.Context, transacti return fmt.Errorf("save peer: %w", err) } + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + eventMeta["old_ip"] = oldIP eventMeta["ip"] = newIP.String() am.StoreEvent(ctx, userID, peer.ID, accountID, activity.PeerIPUpdated, eventMeta) @@ -2884,7 +2888,8 @@ func (am *DefaultAccountManager) UpdatePeerIPv6(ctx context.Context, accountID, // updatePeerIPv6InTransaction validates and applies an IPv6 address change within a store transaction. func (am *DefaultAccountManager) updatePeerIPv6InTransaction(ctx context.Context, transaction store.Store, accountID, peerID string, newIPv6 netip.Addr) (bool, error) { - network, err := transaction.GetAccountNetwork(ctx, store.LockingStrengthShare, accountID) + // Locked for update because the serial increment below writes the same row. + network, err := transaction.GetAccountNetwork(ctx, store.LockingStrengthUpdate, accountID) if err != nil { return false, fmt.Errorf("get network: %w", err) } @@ -2928,6 +2933,10 @@ func (am *DefaultAccountManager) updatePeerIPv6InTransaction(ctx context.Context return false, fmt.Errorf("save peer: %w", err) } + if err := transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return false, fmt.Errorf("increment network serial: %w", err) + } + return true, nil } diff --git a/management/server/account_test.go b/management/server/account_test.go index 7acc72d1f..c343575b8 100644 --- a/management/server/account_test.go +++ b/management/server/account_test.go @@ -3690,7 +3690,7 @@ func createManagerWithNetworkMapStore(t testing.TB) (*DefaultAccountManager, *up store, err := createStoreAt(t, dataDir) require.NoError(t, err) - nmdataStore, err := networkmapdbfactory.NewNetworkMapDBStore(context.Background(), types.SqliteStoreEngine, dataDir, MockIntegratedValidator{}, newSettingsMockManager(t)) + nmdataStore, err := networkmapdbfactory.NewNetworkMapDBStore(context.Background(), types.SqliteStoreEngine, dataDir, MockIntegratedValidator{}, newSettingsMockManager(t), nil, nil) require.NoError(t, err) sqliteStore, ok := nmdataStore.Store.(*networkmap_sqlite.SqliteStore) require.True(t, ok, "network map store is %T, want *networkmap_sqlite.SqliteStore", nmdataStore.Store) @@ -5110,3 +5110,46 @@ func TestDefaultAccountManager_ExpirationJobsStopForDeletedAccount(t *testing.T) }) } } + +func networkSerial(t *testing.T, manager *DefaultAccountManager, accountID string) uint64 { + t.Helper() + + network, err := manager.Store.GetAccountNetwork(context.Background(), store.LockingStrengthNone, accountID) + require.NoError(t, err) + return network.Serial +} + +func TestDefaultAccountManager_UpdatePeerIP_BumpsNetworkSerial(t *testing.T) { + manager, _, account, peer1, peer2, peer3 := setupNetworkMapTest(t) + + newIP, err := types.AllocatePeerIP(netip.MustParsePrefix(account.Network.Net.String()), []netip.Addr{peer1.IP, peer2.IP, peer3.IP}) + require.NoError(t, err) + + before := networkSerial(t, manager, account.Id) + require.NoError(t, manager.UpdatePeerIP(context.Background(), account.Id, userID, peer1.ID, newIP)) + assert.Greater(t, networkSerial(t, manager, account.Id), before) +} + +func TestDefaultAccountManager_SyncUserJWTGroups_BumpsNetworkSerialWithoutPropagation(t *testing.T) { + manager, _, err := createManager(t) + require.NoError(t, err) + + accountID, err := manager.GetAccountIDByUserID(context.Background(), auth.UserAuth{UserId: "jwt-user", Domain: "test.domain"}) + require.NoError(t, err) + acc, err := manager.Store.GetAccount(context.Background(), accountID) + require.NoError(t, err) + acc.Settings.JWTGroupsEnabled = true + acc.Settings.JWTGroupsClaimName = "idp-groups" + acc.Settings.GroupsPropagationEnabled = false + require.NoError(t, manager.Store.SaveAccount(context.Background(), acc)) + + before := networkSerial(t, manager, accountID) + err = manager.SyncUserJWTGroups(context.Background(), auth.UserAuth{ + AccountId: accountID, + Domain: "test.domain", + UserId: "jwt-user", + Groups: []string{"group1"}, + }) + require.NoError(t, err) + assert.Greater(t, networkSerial(t, manager, accountID), before) +} diff --git a/management/server/peer.go b/management/server/peer.go index 5b33ca9f8..c3454179e 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -103,7 +103,9 @@ func (am *DefaultAccountManager) MarkPeerConnected(ctx context.Context, peerPubK return err } - updated, err := am.Store.MarkPeerConnectedIfNewerSession(ctx, accountID, peer.ID, sessionStartedAt) + updated, err := am.writePeerStatus(ctx, accountID, peer, func(st store.Store) (bool, error) { + return st.MarkPeerConnectedIfNewerSession(ctx, accountID, peer.ID, sessionStartedAt) + }) if err != nil { am.metrics.AccountManagerMetrics().CountPeerStatusUpdate(telemetry.PeerStatusConnect, telemetry.PeerStatusError) return err @@ -140,6 +142,28 @@ func (am *DefaultAccountManager) MarkPeerConnected(ctx context.Context, peerPubK return nil } +// writePeerStatus runs a peer status write. The connection state of an embedded +// proxy peer is part of the network map, so for those peers an applied write also +// increments the account network serial in the same transaction. +func (am *DefaultAccountManager) writePeerStatus(ctx context.Context, accountID string, peer *nbpeer.Peer, write func(store.Store) (bool, error)) (bool, error) { + if !peer.ProxyMeta.Embedded { + return write(am.Store) + } + + var updated bool + err := am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error { + var err error + if updated, err = write(transaction); err != nil || !updated { + return err + } + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + return nil + }) + return updated, err +} + // schedulePeerExpirations arms the account's login/inactivity expiration // timers for an SSO peer that just connected. func (am *DefaultAccountManager) schedulePeerExpirations(ctx context.Context, accountID string, peer *nbpeer.Peer) error { @@ -179,7 +203,9 @@ func (am *DefaultAccountManager) MarkPeerDisconnected(ctx context.Context, peerP return err } - updated, err := am.Store.MarkPeerDisconnectedIfSameSession(ctx, accountID, peer.ID, sessionStartedAt) + updated, err := am.writePeerStatus(ctx, accountID, peer, func(st store.Store) (bool, error) { + return st.MarkPeerDisconnectedIfSameSession(ctx, accountID, peer.ID, sessionStartedAt) + }) if err != nil { am.metrics.AccountManagerMetrics().CountPeerStatusUpdate(telemetry.PeerStatusDisconnect, telemetry.PeerStatusError) return err @@ -349,7 +375,7 @@ func (am *DefaultAccountManager) UpdatePeer(ctx context.Context, accountID, user } if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { - return fmt.Errorf("failed to increment network serial: %w", err) + return fmt.Errorf("increment network serial: %w", err) } return transaction.SavePeer(ctx, accountID, peer) @@ -618,7 +644,7 @@ func (am *DefaultAccountManager) deletePeerInTransaction(ctx context.Context, ac } if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { - return fmt.Errorf("failed to increment network serial: %w", err) + return fmt.Errorf("increment network serial: %w", err) } return nil @@ -956,7 +982,7 @@ func (am *DefaultAccountManager) AddPeer(ctx context.Context, accountID, setupKe err = transaction.IncrementNetworkSerial(ctx, accountID) if err != nil { - return fmt.Errorf("failed to increment network serial: %w", err) + return fmt.Errorf("increment network serial: %w", err) } if ephemeral { @@ -1097,6 +1123,9 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync types.PeerSy if err = transaction.SavePeer(ctx, accountID, peer); err != nil { return err } + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } } return nil }) @@ -1272,6 +1301,9 @@ func (am *DefaultAccountManager) LoginPeer(ctx context.Context, login types.Peer if err = transaction.SavePeer(ctx, accountID, peer); err != nil { return err } + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } } return nil diff --git a/management/server/peer_test.go b/management/server/peer_test.go index b75dd1b72..bf8093c1d 100644 --- a/management/server/peer_test.go +++ b/management/server/peer_test.go @@ -3255,3 +3255,71 @@ func TestAddPeer_ExhaustedIPAllocationFails(t *testing.T) { require.Len(t, peers, 1) assert.Equal(t, netip.MustParseAddr("100.64.0.1"), peers[0].IP) } + +func TestPeerWrites_BumpNetworkSerial(t *testing.T) { + manager, _, account, peer1, peer2, _ := setupNetworkMapTest(t) + ctx := context.Background() + + t.Run("SyncPeer with changed meta bumps, unchanged does not", func(t *testing.T) { + current, err := manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, account.Id, peer1.ID) + require.NoError(t, err) + + before := networkSerial(t, manager, account.Id) + _, _, _, _, err = manager.SyncPeer(ctx, types.PeerSync{WireGuardPubKey: peer1.Key, Meta: current.Meta}, account.Id) + require.NoError(t, err) + assert.Equal(t, before, networkSerial(t, manager, account.Id)) + + meta := current.Meta + meta.Hostname = "renamed-host" + _, _, _, _, err = manager.SyncPeer(ctx, types.PeerSync{WireGuardPubKey: peer1.Key, Meta: meta}, account.Id) + require.NoError(t, err) + assert.Greater(t, networkSerial(t, manager, account.Id), before) + }) + + t.Run("LoginPeer with changed ssh key bumps, unchanged does not", func(t *testing.T) { + current, err := manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, account.Id, peer1.ID) + require.NoError(t, err) + + before := networkSerial(t, manager, account.Id) + _, _, _, _, err = manager.LoginPeer(ctx, types.PeerLogin{WireGuardPubKey: peer1.Key, SSHKey: current.SSHKey, Meta: current.Meta}) + require.NoError(t, err) + assert.Equal(t, before, networkSerial(t, manager, account.Id)) + + _, _, _, _, err = manager.LoginPeer(ctx, types.PeerLogin{WireGuardPubKey: peer1.Key, SSHKey: "ssh-ed25519 AAAAnew", Meta: current.Meta}) + require.NoError(t, err) + assert.Greater(t, networkSerial(t, manager, account.Id), before) + }) + + t.Run("MarkPeerConnected and MarkPeerDisconnected", func(t *testing.T) { + session := time.Now().UTC().UnixNano() + + before := networkSerial(t, manager, account.Id) + require.NoError(t, manager.MarkPeerConnected(ctx, peer2.Key, account.Id, session, nil)) + require.NoError(t, manager.MarkPeerDisconnected(ctx, peer2.Key, account.Id, session)) + assert.Equal(t, before, networkSerial(t, manager, account.Id), "regular peers must not bump") + + expired, err := manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, account.Id, peer2.ID) + require.NoError(t, err) + expired.Status.LoginExpired = true + require.NoError(t, manager.Store.SavePeer(ctx, account.Id, expired)) + + session++ + require.NoError(t, manager.MarkPeerConnected(ctx, peer2.Key, account.Id, session, nil)) + afterExpired := networkSerial(t, manager, account.Id) + assert.Equal(t, before, afterExpired, "login-expired reconnect writes no network map data") + + embedded, err := manager.Store.GetPeerByID(ctx, store.LockingStrengthNone, account.Id, peer2.ID) + require.NoError(t, err) + embedded.Status.LoginExpired = false + embedded.ProxyMeta.Embedded = true + require.NoError(t, manager.Store.SavePeer(ctx, account.Id, embedded)) + + require.NoError(t, manager.MarkPeerDisconnected(ctx, peer2.Key, account.Id, session)) + afterDisconnect := networkSerial(t, manager, account.Id) + assert.Greater(t, afterDisconnect, afterExpired, "embedded proxy disconnect must bump") + + session++ + require.NoError(t, manager.MarkPeerConnected(ctx, peer2.Key, account.Id, session, nil)) + assert.Greater(t, networkSerial(t, manager, account.Id), afterDisconnect, "embedded proxy connect must bump") + }) +} diff --git a/management/server/user.go b/management/server/user.go index 3e84fcd03..89149367d 100644 --- a/management/server/user.go +++ b/management/server/user.go @@ -670,6 +670,12 @@ func (am *DefaultAccountManager) SaveOrAddUsers(ctx context.Context, accountID, var updatedUsersInfo = make([]*types.UserInfo, 0, len(usersToSave)) + if len(usersToSave) > 0 { + if err = am.Store.IncrementNetworkSerial(ctx, accountID); err != nil { + return nil, fmt.Errorf("increment network serial: %w", err) + } + } + userInfos, err := am.GetUsersFromAccount(ctx, accountID, initiatorUserID) if err != nil { return nil, err @@ -693,9 +699,6 @@ func (am *DefaultAccountManager) SaveOrAddUsers(ctx context.Context, accountID, return nil, err } } else if len(usersToSave) > 0 { - if err = am.Store.IncrementNetworkSerial(ctx, accountID); err != nil { - return nil, fmt.Errorf("failed to increment network serial: %w", err) - } go am.dispatchAffectedPeerIDs(ctx, accountID, affected.ids()) } @@ -1425,6 +1428,10 @@ func (am *DefaultAccountManager) deleteRegularUser(ctx context.Context, accountI return fmt.Errorf("failed to delete user: %s %w", targetUserInfo.ID, err) } + if err = transaction.IncrementNetworkSerial(ctx, accountID); err != nil { + return fmt.Errorf("increment network serial: %w", err) + } + return nil }) if err != nil { diff --git a/management/server/user_test.go b/management/server/user_test.go index 305ed5a24..2e1b51572 100644 --- a/management/server/user_test.go +++ b/management/server/user_test.go @@ -14,6 +14,7 @@ import ( "github.com/netbirdio/netbird/management/internals/controllers/network_map" nbcache "github.com/netbirdio/netbird/management/server/cache" + nbcontext "github.com/netbirdio/netbird/management/server/context" "github.com/netbirdio/netbird/management/server/permissions" "github.com/netbirdio/netbird/management/server/permissions/modules" "github.com/netbirdio/netbird/management/server/permissions/roles" @@ -2352,3 +2353,24 @@ func TestUser_Operations_WithEmbeddedIDP(t *testing.T) { t.Logf("Duplicate email error: %v", err) }) } + +func TestDefaultAccountManager_UserMutations_BumpNetworkSerial(t *testing.T) { + manager, _, account, _, _, _ := setupNetworkMapTest(t) + + account, err := manager.Store.GetAccount(context.Background(), account.Id) + require.NoError(t, err) + account.Users["regular-user"] = types.NewRegularUser("regular-user", "", "") + require.NoError(t, manager.Store.SaveAccount(context.Background(), account)) + + ctx := nbcontext.WithRole(context.Background(), string(types.UserRoleOwner)) + + before := networkSerial(t, manager, account.Id) + _, err = manager.SaveUser(ctx, account.Id, userID, &types.User{Id: "regular-user", Role: types.UserRoleUser, Blocked: true}) + require.NoError(t, err) + afterSave := networkSerial(t, manager, account.Id) + assert.Greater(t, afterSave, before, "SaveUser must bump the serial") + + _, err = manager.deleteRegularUser(ctx, account.Id, userID, &types.UserInfo{ID: "regular-user"}) + require.NoError(t, err) + assert.Greater(t, networkSerial(t, manager, account.Id), afterSave, "deleteRegularUser must bump the serial") +}