sendnrw/netbird
2
0
Fork 0
mirror of https://github.com/netbirdio/netbird.git synced 2026-08-30 19:41:30 +02:00
Code Issues Packages Projects Releases Wiki Activity

[management] Stamp proxy peer activity with the database clock

Browse Source 
The activity write took a Go-side timestamp, which is exactly what
MarkPeerConnectedIfNewerSession documents as the cause of previous ordering
bugs: a value read before the write can land after a connect that stamped
CURRENT_TIMESTAMP, dragging LastSeen backwards.

The write now uses the database clock like the other status writers, so the
column only ever moves forward. The throttle is unaffected; it reads the
peer already in hand and never needed the write's timestamp.
This commit is contained in:
mlsmaycon
2026-08-09 07:38:35 +00:00
parent 356f6bdda0
commit 48d9161056
8 changed files with 32 additions and 47 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
management/internals/modules/peers/manager.go
View File

@@ -44,8 +44,8 @@ type Manager interface {
// to. Used by the proxy's auth path to authorise a request by the calling
// peer's group memberships.
GetPeerWithGroups(ctx context.Context, accountID, peerID string) (*peer.Peer, []*types.Group, error)
// RefreshLastSeen records that a peer was seen at seenAt.
RefreshLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error
// RefreshLastSeen records that a peer was just seen.
RefreshLastSeen(ctx context.Context, accountID, peerID string) error
}
type managerImpl struct {
@@ -130,8 +130,8 @@ func (m *managerImpl) GetPeerWithGroups(ctx context.Context, accountID, peerID s
return p, groups, nil
}
func (m *managerImpl) RefreshLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error {
return m.store.RefreshPeerLastSeen(ctx, accountID, peerID, seenAt)
func (m *managerImpl) RefreshLastSeen(ctx context.Context, accountID, peerID string) error {
return m.store.RefreshPeerLastSeen(ctx, accountID, peerID)
}
func (m *managerImpl) DeletePeers(ctx context.Context, accountID string, peerIDs []string, userID string, checkConnected bool) error {

9
management/internals/modules/peers/manager_mock.go
View File

@@ -8,7 +8,6 @@ import (
context "context"
net "net"
reflect "reflect"
time "time"
gomock "github.com/golang/mock/gomock"
network_map "github.com/netbirdio/netbird/management/internals/controllers/network_map"
@@ -176,17 +175,17 @@ func (mr *MockManagerMockRecorder) GetPeersByGroupIDs(ctx, accountID, groupsIDs
}
// RefreshLastSeen mocks base method.
func (m *MockManager) RefreshLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error {
func (m *MockManager) RefreshLastSeen(ctx context.Context, accountID, peerID string) error {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "RefreshLastSeen", ctx, accountID, peerID, seenAt)
ret := m.ctrl.Call(m, "RefreshLastSeen", ctx, accountID, peerID)
ret0, _ := ret[0].(error)
return ret0
}
// RefreshLastSeen indicates an expected call of RefreshLastSeen.
func (mr *MockManagerMockRecorder) RefreshLastSeen(ctx, accountID, peerID, seenAt interface{}) *gomock.Call {
func (mr *MockManagerMockRecorder) RefreshLastSeen(ctx, accountID, peerID interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshLastSeen", reflect.TypeOf((*MockManager)(nil).RefreshLastSeen), ctx, accountID, peerID, seenAt)
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshLastSeen", reflect.TypeOf((*MockManager)(nil).RefreshLastSeen), ctx, accountID, peerID)
}
// SetAccountManager mocks base method.

2
management/internals/shared/grpc/proxy.go
View File

@@ -2094,7 +2094,7 @@ func (s *ProxyServiceServer) recordPeerSeen(ctx context.Context, accountID strin
return
}
if err := s.peersManager.RefreshLastSeen(ctx, accountID, peer.ID, time.Now().UTC()); err != nil {
if err := s.peersManager.RefreshLastSeen(ctx, accountID, peer.ID); err != nil {
log.WithContext(ctx).Debugf("record proxy activity for peer %s: %v", peer.ID, err)
}
}

10
management/internals/shared/grpc/proxy_group_access_test.go
View File

@@ -171,15 +171,15 @@ type mockTunnelPeersManager struct {
seenMarks []seenMark
}
// seenMark records a RefreshLastSeen call.
// seenMark records a RefreshLastSeen call. The timestamp is the database's, so
// there is nothing from the caller to assert beyond who was marked.
type seenMark struct {
accountID string
peerID string
at time.Time
}
func (m *mockTunnelPeersManager) RefreshLastSeen(_ context.Context, accountID, peerID string, seenAt time.Time) error {
m.seenMarks = append(m.seenMarks, seenMark{accountID: accountID, peerID: peerID, at: seenAt})
func (m *mockTunnelPeersManager) RefreshLastSeen(_ context.Context, accountID, peerID string) error {
m.seenMarks = append(m.seenMarks, seenMark{accountID: accountID, peerID: peerID})
return nil
}
@@ -848,8 +848,6 @@ func TestValidateTunnelPeerRecordsActivity(t *testing.T) {
mark := peersManager.seenMarks[0]
assert.Equal(t, accountID, mark.accountID, "activity must be recorded against the service account")
assert.Equal(t, peerID, mark.peerID, "activity must be recorded against the calling peer")
assert.Equal(t, time.UTC, mark.at.Location(), "timestamps are written in UTC")
assert.WithinDuration(t, time.Now().UTC(), mark.at, time.Minute, "seen timestamp should be now")
})
}
}

12
management/server/store/sql_store.go
View File

@@ -604,15 +604,15 @@ func (s *SqlStore) ApproveAccountPeers(ctx context.Context, accountID string) (i
// peer_status_session_started_at belong to the sync stream that owns the
// session, and a blind write here would corrupt the fencing
// MarkPeerConnectedIfNewerSession relies on.
func (s *SqlStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error {
if seenAt.IsZero() {
return nil
}
//
// LastSeen comes from the database clock for the same reason it does there: a
// Go-side timestamp is taken before the write and can land after a connect that
// used CURRENT_TIMESTAMP, dragging the column backwards.
func (s *SqlStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string) error {
result := s.db.WithContext(ctx).
Model(&nbpeer.Peer{}).
Where(accountAndIDQueryCondition, accountID, peerID).
Update("peer_status_last_seen", seenAt)
Update("peer_status_last_seen", gorm.Expr("CURRENT_TIMESTAMP"))
if result.Error != nil {
return status.Errorf(status.Internal, "refresh peer last seen: %v", result.Error)
}

25
management/server/store/sql_store_activity_test.go
View File

@@ -104,27 +104,15 @@ func TestRefreshUserLastLoginUnknownUserIsNotAnError(t *testing.T) {
func TestRefreshPeerLastSeen(t *testing.T) {
ctx := context.Background()
store := newActivityTestStore(t)
require.NoError(t, store.AddPeerToAccount(ctx, activityPeer(time.Date(2026, 3, 1, 9, 0, 0, 0, time.UTC))))
seenAt := time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC)
require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", seenAt))
peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer")
require.NoError(t, err)
assert.WithinDuration(t, seenAt, peer.Status.LastSeen.UTC(), time.Second, "unexpected stored last seen")
}
func TestRefreshPeerLastSeenZeroIsIgnored(t *testing.T) {
ctx := context.Background()
store := newActivityTestStore(t)
stored := time.Date(2026, 3, 1, 9, 0, 0, 0, time.UTC)
stored := time.Now().UTC().Add(-3 * time.Hour)
require.NoError(t, store.AddPeerToAccount(ctx, activityPeer(stored)))
require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", time.Time{}))
require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer"))
peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer")
require.NoError(t, err)
assert.WithinDuration(t, stored, peer.Status.LastSeen.UTC(), time.Second, "a zero timestamp must not clear last seen")
assert.WithinDuration(t, time.Now().UTC(), peer.Status.LastSeen.UTC(), time.Minute, "last seen should be stamped at write time")
assert.True(t, peer.Status.LastSeen.After(stored), "last seen must move forward")
}
// TestRefreshPeerLastSeenLeavesSessionStateAlone pins the column boundary: the
@@ -139,12 +127,11 @@ func TestRefreshPeerLastSeenLeavesSessionStateAlone(t *testing.T) {
stored.Status.SessionStartedAt = 1234567890
require.NoError(t, store.AddPeerToAccount(ctx, stored))
seenAt := time.Date(2026, 3, 1, 12, 0, 0, 0, time.UTC)
require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer", seenAt))
require.NoError(t, store.RefreshPeerLastSeen(ctx, activityAccountID, "activityPeer"))
peer, err := store.GetPeerByID(ctx, LockingStrengthNone, activityAccountID, "activityPeer")
require.NoError(t, err)
assert.WithinDuration(t, seenAt, peer.Status.LastSeen.UTC(), time.Second, "last seen should move forward")
assert.WithinDuration(t, time.Now().UTC(), peer.Status.LastSeen.UTC(), time.Minute, "last seen should move forward")
assert.True(t, peer.Status.Connected, "connected flag must survive an activity write")
assert.Equal(t, int64(1234567890), peer.Status.SessionStartedAt, "session token must survive an activity write")
}

5
management/server/store/store.go
View File

@@ -185,11 +185,12 @@ type Store interface {
// Returns true when the update happened, false when this stream lost
// the race against a newer session.
MarkPeerConnectedIfNewerSession(ctx context.Context, accountID, peerID string, newSessionStartedAt int64) (bool, error)
// RefreshPeerLastSeen records that a peer was seen at seenAt. Connected and
// RefreshPeerLastSeen records that a peer was just seen, stamping the
// database clock like the other status writers. Connected and
// SessionStartedAt are left alone, so this never interferes with the
// session-ownership protocol MarkPeerConnectedIfNewerSession implements.
// Callers decide how often to call it; the store does not throttle.
RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error
RefreshPeerLastSeen(ctx context.Context, accountID, peerID string) error
// MarkPeerDisconnectedIfSameSession sets the peer to disconnected and
// resets SessionStartedAt to zero, but only when the stored
// SessionStartedAt equals the given sessionStartedAt. LastSeen is

8
management/server/store/store_mock.go
View File

@@ -3204,17 +3204,17 @@ func (mr *MockStoreMockRecorder) MarkProxyAccessTokenUsed(ctx, tokenID interface
}
// RefreshPeerLastSeen mocks base method.
func (m *MockStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string, seenAt time.Time) error {
func (m *MockStore) RefreshPeerLastSeen(ctx context.Context, accountID, peerID string) error {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "RefreshPeerLastSeen", ctx, accountID, peerID, seenAt)
ret := m.ctrl.Call(m, "RefreshPeerLastSeen", ctx, accountID, peerID)
ret0, _ := ret[0].(error)
return ret0
}
// RefreshPeerLastSeen indicates an expected call of RefreshPeerLastSeen.
func (mr *MockStoreMockRecorder) RefreshPeerLastSeen(ctx, accountID, peerID, seenAt interface{}) *gomock.Call {
func (mr *MockStoreMockRecorder) RefreshPeerLastSeen(ctx, accountID, peerID interface{}) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshPeerLastSeen", reflect.TypeOf((*MockStore)(nil).RefreshPeerLastSeen), ctx, accountID, peerID, seenAt)
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "RefreshPeerLastSeen", reflect.TypeOf((*MockStore)(nil).RefreshPeerLastSeen), ctx, accountID, peerID)
}
// RefreshUserLastLogin mocks base method.