From b0518933cb25f92a6e5b8c8daacbe89a2bf832b9 Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 4 Nov 2024 17:33:05 +0100 Subject: [PATCH 1/2] add peer lock to peer meta update and fix isEqual func --- client/internal/engine.go | 15 +++++- client/internal/engine_test.go | 93 ++++++++++++++++++++++++++++++++++ management/server/account.go | 5 +- management/server/peer.go | 3 ++ 4 files changed, 113 insertions(+), 3 deletions(-) diff --git a/client/internal/engine.go b/client/internal/engine.go index 190d795cdbe..3a6f2679cbc 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -11,6 +11,7 @@ import ( "reflect" "runtime" "slices" + "sort" "strings" "sync" "sync/atomic" @@ -38,7 +39,6 @@ import ( "github.com/netbirdio/netbird/client/internal/routemanager/systemops" "github.com/netbirdio/netbird/client/internal/statemanager" - nbssh "github.com/netbirdio/netbird/client/ssh" "github.com/netbirdio/netbird/client/system" nbdns "github.com/netbirdio/netbird/dns" @@ -171,7 +171,7 @@ type Engine struct { relayManager *relayClient.Manager stateManager *statemanager.Manager - srWatcher *guard.SRWatcher + srWatcher *guard.SRWatcher } // Peer is an instance of the Connection Peer @@ -1481,6 +1481,17 @@ func (e *Engine) stopDNSServer() { // isChecksEqual checks if two slices of checks are equal. func isChecksEqual(checks []*mgmProto.Checks, oChecks []*mgmProto.Checks) bool { + for _, check := range checks { + sort.Slice(check.Files, func(i, j int) bool { + return check.Files[i] < check.Files[j] + }) + } + for _, oCheck := range oChecks { + sort.Slice(oCheck.Files, func(i, j int) bool { + return oCheck.Files[i] < oCheck.Files[j] + }) + } + return slices.EqualFunc(checks, oChecks, func(checks, oChecks *mgmProto.Checks) bool { return slices.Equal(checks.Files, oChecks.Files) }) diff --git a/client/internal/engine_test.go b/client/internal/engine_test.go index 0018af6df8f..b6c6186ea83 100644 --- a/client/internal/engine_test.go +++ b/client/internal/engine_test.go @@ -1006,6 +1006,99 @@ func Test_ParseNATExternalIPMappings(t *testing.T) { } } +func Test_CheckFilesEqual(t *testing.T) { + testCases := []struct { + name string + inputChecks1 []*mgmtProto.Checks + inputChecks2 []*mgmtProto.Checks + expectedBool bool + }{ + { + name: "Equal Files In Equal Order Should Return True", + inputChecks1: []*mgmtProto.Checks{ + { + Files: []string{ + "testfile1", + "testfile2", + }, + }, + }, + inputChecks2: []*mgmtProto.Checks{ + { + Files: []string{ + "testfile1", + "testfile2", + }, + }, + }, + expectedBool: true, + }, + { + name: "Equal Files In Reverse Order Should Return True", + inputChecks1: []*mgmtProto.Checks{ + { + Files: []string{ + "testfile1", + "testfile2", + }, + }, + }, + inputChecks2: []*mgmtProto.Checks{ + { + Files: []string{ + "testfile2", + "testfile1", + }, + }, + }, + expectedBool: true, + }, + { + name: "Unequal Files Should Return False", + inputChecks1: []*mgmtProto.Checks{ + { + Files: []string{ + "testfile1", + "testfile2", + }, + }, + }, + inputChecks2: []*mgmtProto.Checks{ + { + Files: []string{ + "testfile1", + "testfile3", + }, + }, + }, + expectedBool: false, + }, + { + name: "Compared With Empty Should Return False", + inputChecks1: []*mgmtProto.Checks{ + { + Files: []string{ + "testfile1", + "testfile2", + }, + }, + }, + inputChecks2: []*mgmtProto.Checks{ + { + Files: []string{}, + }, + }, + expectedBool: false, + }, + } + for _, testCase := range testCases { + t.Run(testCase.name, func(t *testing.T) { + result := isChecksEqual(testCase.inputChecks1, testCase.inputChecks2) + assert.Equal(t, testCase.expectedBool, result, "result should match expected bool") + }) + } +} + func createEngine(ctx context.Context, cancel context.CancelFunc, setupKey string, i int, mgmtAddr string, signalAddr string) (*Engine, error) { key, err := wgtypes.GeneratePrivateKey() if err != nil { diff --git a/management/server/account.go b/management/server/account.go index 1810c6b41ec..57f36e68450 100644 --- a/management/server/account.go +++ b/management/server/account.go @@ -2319,7 +2319,7 @@ func (am *DefaultAccountManager) OnPeerDisconnected(ctx context.Context, account err = am.MarkPeerConnected(ctx, peerPubKey, false, nil, account) if err != nil { - log.WithContext(ctx).Warnf("failed marking peer as connected %s %v", peerPubKey, err) + log.WithContext(ctx).Warnf("failed marking peer as disconnected %s %v", peerPubKey, err) } return nil @@ -2335,6 +2335,9 @@ func (am *DefaultAccountManager) SyncPeerMeta(ctx context.Context, peerPubKey st unlock := am.Store.AcquireReadLockByUID(ctx, accountID) defer unlock() + unlockPeer := am.Store.AcquireWriteLockByUID(ctx, peerPubKey) + defer unlockPeer() + account, err := am.Store.GetAccount(ctx, accountID) if err != nil { return err diff --git a/management/server/peer.go b/management/server/peer.go index 7cc2209c5e0..9922a02b7cc 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -166,6 +166,8 @@ func (am *DefaultAccountManager) updatePeerStatusAndLocation(ctx context.Context account.UpdatePeer(peer) + log.WithContext(ctx).Debugf("saving peer status for peer %s is connected: %t", peer.ID, connected) + err := am.Store.SavePeerStatus(account.Id, peer.ID, *newStatus) if err != nil { return false, err @@ -654,6 +656,7 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync PeerSync, ac updated := peer.UpdateMetaIfNew(sync.Meta) if updated { + log.WithContext(ctx).Debugf("peer %s metadata updated", peer.ID) err = am.Store.SavePeer(ctx, account.Id, peer) if err != nil { return nil, nil, nil, err From dd7daca1dd0d29a4e7143dfde17bf8fa23cf543a Mon Sep 17 00:00:00 2001 From: Pascal Fischer Date: Mon, 11 Nov 2024 17:42:58 +0100 Subject: [PATCH 2/2] logs as trace --- management/server/peer.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/management/server/peer.go b/management/server/peer.go index 9922a02b7cc..d423057a777 100644 --- a/management/server/peer.go +++ b/management/server/peer.go @@ -166,7 +166,7 @@ func (am *DefaultAccountManager) updatePeerStatusAndLocation(ctx context.Context account.UpdatePeer(peer) - log.WithContext(ctx).Debugf("saving peer status for peer %s is connected: %t", peer.ID, connected) + log.WithContext(ctx).Tracef("saving peer status for peer %s is connected: %t", peer.ID, connected) err := am.Store.SavePeerStatus(account.Id, peer.ID, *newStatus) if err != nil { @@ -656,7 +656,7 @@ func (am *DefaultAccountManager) SyncPeer(ctx context.Context, sync PeerSync, ac updated := peer.UpdateMetaIfNew(sync.Meta) if updated { - log.WithContext(ctx).Debugf("peer %s metadata updated", peer.ID) + log.WithContext(ctx).Tracef("peer %s metadata updated", peer.ID) err = am.Store.SavePeer(ctx, account.Id, peer) if err != nil { return nil, nil, nil, err