diff --git a/integration_tests/management/network_map_db/network_router_test.go b/integration_tests/management/network_map_db/network_router_test.go index fa7ea2a04..2baf146a7 100644 --- a/integration_tests/management/network_map_db/network_router_test.go +++ b/integration_tests/management/network_map_db/network_router_test.go @@ -19,6 +19,14 @@ func TestGetNetworkRouters(t *testing.T) { execQuery(t, ctx, `insert into network_routers (id, account_id, public_id, peer, network_id, masquerade, metric, enabled, peer_groups) VALUES('test-nr-id-2','account-1','public-id-2','','network-id-2',TRUE,333,TRUE,'["group-two-resources-id","group-no-resources-id"]')`) + // empty peer_groups + execQuery(t, ctx, + `insert into network_routers (id, account_id, public_id, peer, network_id, masquerade, metric, enabled, peer_groups) + VALUES('test-nr-id-3','account-1','public-id-3','peer-id-3','network-id-3',TRUE,999,TRUE,'[]')`) + // nil peer_groups + execQuery(t, ctx, + `insert into network_routers (id, account_id, public_id, peer, network_id, masquerade, metric, enabled, peer_groups) + VALUES('test-nr-id-4','account-1','public-id-4','peer-id-4','network-id-4',TRUE,999,TRUE,null)`) routers, err := conn(t, ctx).GetNetworkRouters(ctx, "account-1") assert.NoError(t, err) @@ -30,4 +38,8 @@ func TestGetNetworkRouters(t *testing.T) { map[string]*nmdata.NetworkRouter{ "peer-id-2": {PublicID: "public-id-2", Masquerade: true, Metric: 333, Enabled: true, PeerGroups: []string{"group-two-resources-id", "group-no-resources-id"}}, "peer-id-3": {PublicID: "public-id-2", Masquerade: true, Metric: 333, Enabled: true, PeerGroups: []string{"group-two-resources-id", "group-no-resources-id"}}}) + assert.Equal(t, routers["network-id-3"], + map[string]*nmdata.NetworkRouter{"peer-id-3": {PublicID: "public-id-3", Masquerade: true, Metric: 999, Enabled: true, PeerGroups: []string{}}}) + assert.Equal(t, routers["network-id-4"], + map[string]*nmdata.NetworkRouter{"peer-id-4": {PublicID: "public-id-4", Masquerade: true, Metric: 999, Enabled: true, PeerGroups: nil}}) } diff --git a/integration_tests/management/network_map_db/user_test.go b/integration_tests/management/network_map_db/user_test.go index 132f749e2..fce1833d3 100644 --- a/integration_tests/management/network_map_db/user_test.go +++ b/integration_tests/management/network_map_db/user_test.go @@ -21,6 +21,14 @@ func TestGetAllowedUsers(t *testing.T) { execQuery(t, ctx, `insert into users (id, name, account_id, auto_groups, blocked, is_service_user) VALUES('user-3','user-3','account-1','["group-two-resources-id"]',false,false)`) + // empty auto_groups; shouldn't error out + execQuery(t, ctx, + `insert into users (id, name, account_id, auto_groups, blocked, is_service_user) + VALUES('user-31','user-31','account-1','[]',false,false)`) + // null auto_groups; shouldn't error out + execQuery(t, ctx, + `insert into users (id, name, account_id, auto_groups, blocked, is_service_user) + VALUES('user-32','user-32','account-1',null,false,false)`) // shouldn't be included as it's blocked execQuery(t, ctx, `insert into users (id, name, account_id, auto_groups, blocked, is_service_user) @@ -43,15 +51,17 @@ func TestGetAllowedUsers(t *testing.T) { assert.NoError(t, err) assert.Equal(t, userIdx, map[string]struct{}{ - "user-1": {}, - "user-2": {}, - "user-3": {}, + "user-1": {}, + "user-2": {}, + "user-3": {}, + "user-31": {}, + "user-32": {}, }) assert.Equal(t, groupIdToUserIds, map[string][]string{ "group-one-resource-id": {"user-1", "user-2"}, "group-two-resources-id": {"user-2", "user-3"}, - "all-group-1": {"user-1", "user-2", "user-3"}, - "all-group-2": {"user-1", "user-2", "user-3"}, - "all-group-3": {"user-1", "user-2", "user-3"}, + "all-group-1": {"user-1", "user-2", "user-3", "user-31", "user-32"}, + "all-group-2": {"user-1", "user-2", "user-3", "user-31", "user-32"}, + "all-group-3": {"user-1", "user-2", "user-3", "user-31", "user-32"}, }) } diff --git a/management/internals/network_map_db/network_router_store_test.go b/management/internals/network_map_db/network_router_store_test.go deleted file mode 100644 index d44aa1ebc..000000000 --- a/management/internals/network_map_db/network_router_store_test.go +++ /dev/null @@ -1,120 +0,0 @@ -package networkmapdb_test - -import ( - "context" - "net/netip" - "os" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - - 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" - routerTypes "github.com/netbirdio/netbird/management/server/networks/routers/types" - nbpeer "github.com/netbirdio/netbird/management/server/peer" - "github.com/netbirdio/netbird/management/server/store" - "github.com/netbirdio/netbird/management/server/testutil" - "github.com/netbirdio/netbird/management/server/types" -) - -// newEngineStores opens both stores on the selected engine's database. -func newEngineStores(t *testing.T) (store.Store, networkmapdb.NetworkMapDBStore) { - t.Helper() - ctx := context.Background() - - switch engine := types.Engine(os.Getenv("NETBIRD_STORE_ENGINE")); engine { - case types.PostgresStoreEngine: - cleanup, dsn, err := testutil.CreatePostgresTestContainer() - require.NoError(t, err, "start postgres test container") - t.Cleanup(cleanup) - - accountStore, err := store.NewPostgresqlStore(ctx, dsn, nil, false) - require.NoError(t, err, "connect account store") - t.Cleanup(func() { _ = accountStore.Close(ctx) }) - - nmStore, err := networkmap_pgsql.NewPostgresqlStore(ctx, dsn) - require.NoError(t, err, "connect networkmap store") - t.Cleanup(func() { nmStore.Pool.Close() }) - return accountStore, nmStore - case types.SqliteStoreEngine, "": - dataDir := t.TempDir() - accountStore, err := store.NewSqliteStore(ctx, dataDir, nil, false) - require.NoError(t, err, "open account store") - t.Cleanup(func() { _ = accountStore.Close(ctx) }) - - nmStore, err := networkmap_sqlite.NewSqliteStore("store.db", dataDir) - require.NoError(t, err, "open networkmap store") - t.Cleanup(func() { _ = nmStore.Db.Close() }) - return accountStore, nmStore - default: - t.Skipf("networkmap store does not support engine %q", engine) - return nil, nil - } -} - -// Peer-based routers must survive the network-map read on every engine. -func TestGetNetworkRouters_ServesPeerBasedRouters(t *testing.T) { - ctx := context.Background() - accountStore, nmStore := newEngineStores(t) - - const ( - accountID = "acc-nmap-routers" - groupID = "grp-router-members" - memberID = "peer-member" - ) - - // Postgres enforces the groups-to-accounts FK that SQLite ignores. - require.NoError(t, accountStore.SaveAccount(ctx, &types.Account{ - Id: accountID, - Peers: map[string]*nbpeer.Peer{ - memberID: { - ID: memberID, - AccountID: accountID, - Key: memberID + "-key", - IP: netip.MustParseAddr("100.64.0.10"), - Status: &nbpeer.PeerStatus{}, - }, - }, - Groups: map[string]*types.Group{ - groupID: { - ID: groupID, - AccountID: accountID, - Name: "router members", - Issued: types.GroupIssuedAPI, - Peers: []string{memberID}, - }, - }, - }), "seed the account the routers belong to") - - routers := []*routerTypes.NetworkRouter{ - {ID: "router-peer-nil", AccountID: accountID, NetworkID: "net-peer-nil", PublicID: "pub-peer-nil", Peer: "peer-direct-nil", Enabled: true, Metric: 9999}, - {ID: "router-peer-empty", AccountID: accountID, NetworkID: "net-peer-empty", PublicID: "pub-peer-empty", Peer: "peer-direct-empty", PeerGroups: []string{}, Enabled: true, Metric: 9999}, - {ID: "router-group", AccountID: accountID, NetworkID: "net-group", PublicID: "pub-group", PeerGroups: []string{groupID}, Enabled: true, Metric: 9999}, - } - for _, router := range routers { - require.NoError(t, accountStore.CreateNetworkRouter(ctx, router)) - } - - tx, err := nmStore.BeginTx(ctx) - require.NoError(t, err, "begin networkmap read transaction") - t.Cleanup(func() { _ = tx.RollbackTx(ctx) }) - - got, err := tx.GetNetworkRouters(ctx, accountID) - require.NoError(t, err, "read network routers") - - assert.Contains(t, got, "net-peer-nil", - "a router referencing an individual peer (peer_groups stored as NULL) must reach the network map") - assert.Contains(t, got["net-peer-nil"], "peer-direct-nil", - "the individual-peer router must be keyed by its peer") - - assert.Contains(t, got, "net-peer-empty", - "a router referencing an individual peer (peer_groups stored as '[]') must reach the network map") - assert.Contains(t, got["net-peer-empty"], "peer-direct-empty", - "the individual-peer router must be keyed by its peer") - - assert.Contains(t, got, "net-group", "a group router must reach the network map") - assert.Contains(t, got["net-group"], memberID, - "the group router must fan out to the group's member peers") -} diff --git a/management/internals/network_map_db/pgsql/dns.go b/management/internals/network_map_db/pgsql/dns.go index b22b43903..46ef0ddfa 100644 --- a/management/internals/network_map_db/pgsql/dns.go +++ b/management/internals/network_map_db/pgsql/dns.go @@ -15,6 +15,7 @@ const ( from zones left join records as r on r.zone_id = zones.id where zones.account_id=$1 and zones.enabled + order by zones.id ` ) diff --git a/management/internals/network_map_db/sqlite/user.go b/management/internals/network_map_db/sqlite/user.go index 0bdda372e..23c46bf76 100644 --- a/management/internals/network_map_db/sqlite/user.go +++ b/management/internals/network_map_db/sqlite/user.go @@ -42,17 +42,20 @@ func (sc *SqliteStoreConn) GetAllowedUsers(ctx context.Context, accountId string userIdIdx := make(map[string]struct{}) groupIdToUserIds := make(map[string][]string) for _, user := range users { + for _, allgid := range allGroupIds { + groupIdToUserIds[allgid] = append(groupIdToUserIds[allgid], user.ID) + } + userIdIdx[user.ID] = struct{}{} autogroups := make([]string, 0) + if user.AutoGroups == nil { + continue + } if err := json.Unmarshal(user.AutoGroups, &autogroups); err != nil { return nil, nil, err } - userIdIdx[user.ID] = struct{}{} for _, groupId := range autogroups { groupIdToUserIds[groupId] = append(groupIdToUserIds[groupId], user.ID) } - for _, allgid := range allGroupIds { - groupIdToUserIds[allgid] = append(groupIdToUserIds[allgid], user.ID) - } } return userIdIdx, groupIdToUserIds, nil