From 78da6b42ad41bedfa51d9a49f5dcc26f15f353a5 Mon Sep 17 00:00:00 2001 From: Eddie Garcia Date: Wed, 22 Jan 2025 12:57:54 -0500 Subject: [PATCH 1/3] [misc] Fix typo in test output (#3216) Fix a typo in test output --- .github/workflows/golang-test-freebsd.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/golang-test-freebsd.yml b/.github/workflows/golang-test-freebsd.yml index 7a2d3cf3ca7..0f510cb3ad1 100644 --- a/.github/workflows/golang-test-freebsd.yml +++ b/.github/workflows/golang-test-freebsd.yml @@ -33,7 +33,7 @@ jobs: time go build -o netbird client/main.go # check all component except management, since we do not support management server on freebsd time go test -timeout 1m -failfast ./base62/... - # NOTE: without -p1 `client/internal/dns` will fail becasue of `listen udp4 :33100: bind: address already in use` + # NOTE: without -p1 `client/internal/dns` will fail because of `listen udp4 :33100: bind: address already in use` time go test -timeout 8m -failfast -p 1 ./client/... time go test -timeout 1m -failfast ./dns/... time go test -timeout 1m -failfast ./encryption/... From 8c965434ae67329885cc5bfc66ad580e42fbb984 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Wed, 22 Jan 2025 19:33:20 +0100 Subject: [PATCH 2/3] [management] remove peer from group on delete (#3223) --- .../peers_handler_benchmark_test.go | 16 +++--- management/server/peer.go | 13 +++++ management/server/peer_test.go | 49 +++++++++++++++++++ 3 files changed, 70 insertions(+), 8 deletions(-) diff --git a/management/server/http/testing/benchmarks/peers_handler_benchmark_test.go b/management/server/http/testing/benchmarks/peers_handler_benchmark_test.go index 2eb50e4b4e6..23b4edefbb4 100644 --- a/management/server/http/testing/benchmarks/peers_handler_benchmark_test.go +++ b/management/server/http/testing/benchmarks/peers_handler_benchmark_test.go @@ -145,14 +145,14 @@ func BenchmarkGetAllPeers(b *testing.B) { func BenchmarkDeletePeer(b *testing.B) { var expectedMetrics = map[string]testing_tools.PerformanceMetrics{ - "Peers - XS": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, - "Peers - S": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, - "Peers - M": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, - "Peers - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, - "Groups - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, - "Users - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, - "Setup Keys - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, - "Peers - XL": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 15}, + "Peers - XS": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16}, + "Peers - S": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16}, + "Peers - M": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16}, + "Peers - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16}, + "Groups - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16}, + "Users - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16}, + "Setup Keys - L": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16}, + "Peers - XL": {MinMsPerOpLocal: 0, MaxMsPerOpLocal: 4, MinMsPerOpCICD: 2, MaxMsPerOpCICD: 16}, } log.SetOutput(io.Discard) diff --git a/management/server/peer.go b/management/server/peer.go index 5b0f1289958..e5442acea7b 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -380,6 +380,19 @@ func (am *DefaultAccountManager) DeletePeer(ctx context.Context, accountID, peer return err } + groups, err := transaction.GetPeerGroups(ctx, store.LockingStrengthUpdate, accountID, peerID) + if err != nil { + return fmt.Errorf("failed to get peer groups: %w", err) + } + + for _, group := range groups { + group.RemovePeer(peerID) + err = transaction.SaveGroup(ctx, store.LockingStrengthUpdate, group) + if err != nil { + return fmt.Errorf("failed to save group: %w", err) + } + } + eventsToStore, err = deletePeers(ctx, am, transaction, accountID, userID, []*nbpeer.Peer{peer}) return err }) diff --git a/management/server/peer_test.go b/management/server/peer_test.go index bf712f38a70..40f8d15d5a2 100644 --- a/management/server/peer_test.go +++ b/management/server/peer_test.go @@ -1728,3 +1728,52 @@ func TestPeerAccountPeersUpdate(t *testing.T) { } }) } + +func Test_DeletePeer(t *testing.T) { + manager, err := createManager(t) + if err != nil { + t.Fatal(err) + return + } + + // account with an admin and a regular user + accountID := "test_account" + adminUser := "account_creator" + account := newAccountWithId(context.Background(), accountID, adminUser, "") + account.Peers = map[string]*nbpeer.Peer{ + "peer1": { + ID: "peer1", + AccountID: accountID, + }, + "peer2": { + ID: "peer2", + AccountID: accountID, + }, + } + account.Groups = map[string]*types.Group{ + "group1": { + ID: "group1", + Name: "Group1", + Peers: []string{"peer1", "peer2"}, + }, + } + + err = manager.Store.SaveAccount(context.Background(), account) + if err != nil { + t.Fatal(err) + return + } + + err = manager.DeletePeer(context.Background(), accountID, "peer1", adminUser) + if err != nil { + t.Fatalf("DeletePeer failed: %v", err) + } + + _, err = manager.GetPeer(context.Background(), accountID, "peer1", adminUser) + assert.Error(t, err) + + group, err := manager.GetGroup(context.Background(), accountID, "group1", adminUser) + assert.NoError(t, err) + assert.NotContains(t, group.Peers, "peer1") + +} From 69f48db0a35f99028170c1e94fdb5b322993c2b0 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Wed, 22 Jan 2025 19:53:20 +0100 Subject: [PATCH 3/3] [management] disable prepareStmt for sqlite (#3228) --- go.mod | 6 +- go.sum | 11 ++-- management/server/store/sql_store.go | 14 +++-- management/server/store/sql_store_test.go | 74 ++++++++++++++++++++++- management/server/store/store.go | 2 +- 5 files changed, 92 insertions(+), 15 deletions(-) diff --git a/go.mod b/go.mod index fa573bb9cab..895ad5618a0 100644 --- a/go.mod +++ b/go.mod @@ -55,7 +55,7 @@ require ( github.com/libdns/route53 v1.5.0 github.com/libp2p/go-netroute v0.2.1 github.com/magiconair/properties v1.8.7 - github.com/mattn/go-sqlite3 v1.14.19 + github.com/mattn/go-sqlite3 v1.14.22 github.com/mdlayher/socket v0.5.1 github.com/miekg/dns v1.1.59 github.com/mitchellh/hashstructure/v2 v2.0.2 @@ -100,8 +100,8 @@ require ( gopkg.in/yaml.v3 v3.0.1 gorm.io/driver/mysql v1.5.7 gorm.io/driver/postgres v1.5.7 - gorm.io/driver/sqlite v1.5.3 - gorm.io/gorm v1.25.7 + gorm.io/driver/sqlite v1.5.7 + gorm.io/gorm v1.25.12 nhooyr.io/websocket v1.8.11 ) diff --git a/go.sum b/go.sum index a099498fb11..2aae595f09f 100644 --- a/go.sum +++ b/go.sum @@ -475,8 +475,8 @@ github.com/mailru/easyjson v0.0.0-20160728113105-d5b7844b561a/go.mod h1:C1wdFJiN github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU= github.com/mattn/go-isatty v0.0.3/go.mod h1:M+lRXTBqGeGNdLjl/ufCoiOlB5xdOkqRJdNxMWT7Zi4= github.com/mattn/go-isatty v0.0.9/go.mod h1:YNRxwqDuOph6SZLI9vUUz6OYw3QyUt7WiY2yME+cCiQ= -github.com/mattn/go-sqlite3 v1.14.19 h1:fhGleo2h1p8tVChob4I9HpmVFIAkKGpiukdrgQbWfGI= -github.com/mattn/go-sqlite3 v1.14.19/go.mod h1:2eHXhiwb8IkHr+BDWZGa96P6+rkvnG63S2DGjv9HUNg= +github.com/mattn/go-sqlite3 v1.14.22 h1:2gZY6PC6kBnID23Tichd1K+Z0oS6nE/XwU+Vz/5o4kU= +github.com/mattn/go-sqlite3 v1.14.22/go.mod h1:Uh1q+B4BYcTPb+yiD3kU8Ct7aC0hY9fxUwlHK0RXw+Y= github.com/mdlayher/genetlink v1.3.2 h1:KdrNKe+CTu+IbZnm/GVUMXSqBBLqcGpRDa0xkQy56gw= github.com/mdlayher/genetlink v1.3.2/go.mod h1:tcC3pkCrPUGIKKsCsp0B3AdaaKuHtaxoJRz3cc+528o= github.com/mdlayher/netlink v1.7.2 h1:/UtM3ofJap7Vl4QWCPDGXY8d3GIY2UGSDbK+QWmY8/g= @@ -1241,10 +1241,11 @@ gorm.io/driver/mysql v1.5.7 h1:MndhOPYOfEp2rHKgkZIhJ16eVUIRf2HmzgoPmh7FCWo= gorm.io/driver/mysql v1.5.7/go.mod h1:sEtPWMiqiN1N1cMXoXmBbd8C6/l+TESwriotuRRpkDM= gorm.io/driver/postgres v1.5.7 h1:8ptbNJTDbEmhdr62uReG5BGkdQyeasu/FZHxI0IMGnM= gorm.io/driver/postgres v1.5.7/go.mod h1:3e019WlBaYI5o5LIdNV+LyxCMNtLOQETBXL2h4chKpA= -gorm.io/driver/sqlite v1.5.3 h1:7/0dUgX28KAcopdfbRWWl68Rflh6osa4rDh+m51KL2g= -gorm.io/driver/sqlite v1.5.3/go.mod h1:qxAuCol+2r6PannQDpOP1FP6ag3mKi4esLnB/jHed+4= -gorm.io/gorm v1.25.7 h1:VsD6acwRjz2zFxGO50gPO6AkNs7KKnvfzUjHQhZDz/A= +gorm.io/driver/sqlite v1.5.7 h1:8NvsrhP0ifM7LX9G4zPB97NwovUakUxc+2V2uuf3Z1I= +gorm.io/driver/sqlite v1.5.7/go.mod h1:U+J8craQU6Fzkcvu8oLeAQmi50TkwPEhHDEjQZXDah4= gorm.io/gorm v1.25.7/go.mod h1:hbnx/Oo0ChWMn1BIhpy1oYozzpM15i4YPuHDmfYtwg8= +gorm.io/gorm v1.25.12 h1:I0u8i2hWQItBq1WfE0o2+WuL9+8L21K9e2HHSTE/0f8= +gorm.io/gorm v1.25.12/go.mod h1:xh7N7RHfYlNc5EmcI/El95gXusucDrQnHXe0+CgWcLQ= gotest.tools/v3 v3.5.0 h1:Ljk6PdHdOhAb5aDMWXjDLMMhph+BpztA4v1QdqEW2eY= gotest.tools/v3 v3.5.0/go.mod h1:isy3WKz7GK6uNw/sbHzfKBLvlvXwUyV06n6brMxxopU= gvisor.dev/gvisor v0.0.0-20231020174304-db3d49b921f9 h1:sCEaoA7ZmkuFwa2IR61pl4+RYZPwCJOiaSYT0k+BRf8= diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index 900d813221e..2179f07547e 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -956,7 +956,7 @@ func NewSqliteStore(ctx context.Context, dataDir string, metrics telemetry.AppMe } file := filepath.Join(dataDir, storeStr) - db, err := gorm.Open(sqlite.Open(file), getGormConfig()) + db, err := gorm.Open(sqlite.Open(file), getGormConfig(SqliteStoreEngine)) if err != nil { return nil, err } @@ -966,7 +966,7 @@ func NewSqliteStore(ctx context.Context, dataDir string, metrics telemetry.AppMe // NewPostgresqlStore creates a new Postgres store. func NewPostgresqlStore(ctx context.Context, dsn string, metrics telemetry.AppMetrics) (*SqlStore, error) { - db, err := gorm.Open(postgres.Open(dsn), getGormConfig()) + db, err := gorm.Open(postgres.Open(dsn), getGormConfig(PostgresStoreEngine)) if err != nil { return nil, err } @@ -976,7 +976,7 @@ func NewPostgresqlStore(ctx context.Context, dsn string, metrics telemetry.AppMe // NewMysqlStore creates a new MySQL store. func NewMysqlStore(ctx context.Context, dsn string, metrics telemetry.AppMetrics) (*SqlStore, error) { - db, err := gorm.Open(mysql.Open(dsn+"?charset=utf8&parseTime=True&loc=Local"), getGormConfig()) + db, err := gorm.Open(mysql.Open(dsn+"?charset=utf8&parseTime=True&loc=Local"), getGormConfig(MysqlStoreEngine)) if err != nil { return nil, err } @@ -984,11 +984,15 @@ func NewMysqlStore(ctx context.Context, dsn string, metrics telemetry.AppMetrics return NewSqlStore(ctx, db, MysqlStoreEngine, metrics) } -func getGormConfig() *gorm.Config { +func getGormConfig(engine Engine) *gorm.Config { + prepStmt := true + if engine == SqliteStoreEngine { + prepStmt = false + } return &gorm.Config{ Logger: logger.Default.LogMode(logger.Silent), CreateBatchSize: 400, - PrepareStmt: true, + PrepareStmt: prepStmt, } } diff --git a/management/server/store/sql_store_test.go b/management/server/store/sql_store_test.go index cb51dab5179..9350da1c89c 100644 --- a/management/server/store/sql_store_test.go +++ b/management/server/store/sql_store_test.go @@ -10,16 +10,18 @@ import ( "net/netip" "os" "runtime" + "sync" "testing" "time" "github.com/google/uuid" - "github.com/netbirdio/netbird/management/server/util" "github.com/rs/xid" log "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "github.com/netbirdio/netbird/management/server/util" + nbdns "github.com/netbirdio/netbird/dns" resourceTypes "github.com/netbirdio/netbird/management/server/networks/resources/types" routerTypes "github.com/netbirdio/netbird/management/server/networks/routers/types" @@ -2843,3 +2845,73 @@ func TestSqlStore_DeletePeer(t *testing.T) { require.Error(t, err) require.Nil(t, peer) } + +func TestSqlStore_DatabaseBlocking(t *testing.T) { + store, cleanup, err := NewTestStoreFromSQL(context.Background(), "../testdata/store_with_expired_peers.sql", t.TempDir()) + t.Cleanup(cleanup) + if err != nil { + t.Fatal(err) + } + + concurrentReads := 40 + + testRunSuccessful := false + wgSuccess := sync.WaitGroup{} + wgSuccess.Add(concurrentReads) + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + + start := make(chan struct{}) + + for i := 0; i < concurrentReads/2; i++ { + go func() { + t.Logf("Entered routine 1-%d", i) + + <-start + err := store.ExecuteInTransaction(context.Background(), func(tx Store) error { + _, err := tx.GetAccountIDByPeerID(context.Background(), LockingStrengthShare, "cfvprsrlo1hqoo49ohog") + return err + }) + if err != nil { + t.Errorf("Failed, got error: %v", err) + return + } + + t.Log("Got User from routine 1") + wgSuccess.Done() + }() + } + + for i := 0; i < concurrentReads/2; i++ { + go func() { + t.Logf("Entered routine 2-%d", i) + + <-start + _, err := store.GetAccountIDByPeerID(context.Background(), LockingStrengthShare, "cfvprsrlo1hqoo49ohog") + if err != nil { + t.Errorf("Failed, got error: %v", err) + return + } + + t.Log("Got User from routine 2") + wgSuccess.Done() + }() + } + + time.Sleep(200 * time.Millisecond) + close(start) + t.Log("Started routines") + + go func() { + wgSuccess.Wait() + testRunSuccessful = true + }() + + <-ctx.Done() + if !testRunSuccessful { + t.Fatalf("Test failed") + } + + t.Logf("Test completed") +} diff --git a/management/server/store/store.go b/management/server/store/store.go index 245df1c3e2d..4b4dcfb4f96 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -319,7 +319,7 @@ func NewTestStoreFromSQL(ctx context.Context, filename string, dataDir string) ( } file := filepath.Join(dataDir, storeStr) - db, err := gorm.Open(sqlite.Open(file), getGormConfig()) + db, err := gorm.Open(sqlite.Open(file), getGormConfig(kind)) if err != nil { return nil, nil, err }