diff --git a/management/internals/modules/reverseproxy/accesslogs/interface.go b/management/internals/modules/reverseproxy/accesslogs/interface.go index 1c51a8a7d8a..04f096bf196 100644 --- a/management/internals/modules/reverseproxy/accesslogs/interface.go +++ b/management/internals/modules/reverseproxy/accesslogs/interface.go @@ -7,4 +7,7 @@ import ( type Manager interface { SaveAccessLog(ctx context.Context, proxyLog *AccessLogEntry) error GetAllAccessLogs(ctx context.Context, accountID, userID string, filter *AccessLogFilter) ([]*AccessLogEntry, int64, error) + CleanupOldAccessLogs(ctx context.Context, retentionDays int) (int64, error) + StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) + StopPeriodicCleanup() } diff --git a/management/internals/modules/reverseproxy/accesslogs/manager/manager.go b/management/internals/modules/reverseproxy/accesslogs/manager/manager.go index 7bcdecb1bd7..e7fba7bedad 100644 --- a/management/internals/modules/reverseproxy/accesslogs/manager/manager.go +++ b/management/internals/modules/reverseproxy/accesslogs/manager/manager.go @@ -3,6 +3,7 @@ package manager import ( "context" "strings" + "time" log "github.com/sirupsen/logrus" @@ -19,6 +20,7 @@ type managerImpl struct { store store.Store permissionsManager permissions.Manager geo geolocation.Geolocation + cleanupCancel context.CancelFunc } func NewManager(store store.Store, permissionsManager permissions.Manager, geo geolocation.Geolocation) accesslogs.Manager { @@ -78,6 +80,74 @@ func (m *managerImpl) GetAllAccessLogs(ctx context.Context, accountID, userID st return logs, totalCount, nil } +// CleanupOldAccessLogs deletes access logs older than the specified retention period +func (m *managerImpl) CleanupOldAccessLogs(ctx context.Context, retentionDays int) (int64, error) { + if retentionDays <= 0 { + log.WithContext(ctx).Debug("access log cleanup skipped: retention days is 0 or negative") + return 0, nil + } + + cutoffTime := time.Now().AddDate(0, 0, -retentionDays) + deletedCount, err := m.store.DeleteOldAccessLogs(ctx, cutoffTime) + if err != nil { + log.WithContext(ctx).Errorf("failed to cleanup old access logs: %v", err) + return 0, err + } + + if deletedCount > 0 { + log.WithContext(ctx).Infof("cleaned up %d access logs older than %d days", deletedCount, retentionDays) + } + + return deletedCount, nil +} + +// StartPeriodicCleanup starts a background goroutine that periodically cleans up old access logs +func (m *managerImpl) StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) { + if retentionDays <= 0 { + log.WithContext(ctx).Debug("periodic access log cleanup disabled: retention days is 0 or negative") + return + } + + if cleanupIntervalHours <= 0 { + cleanupIntervalHours = 24 + } + + cleanupCtx, cancel := context.WithCancel(ctx) + m.cleanupCancel = cancel + + cleanupInterval := time.Duration(cleanupIntervalHours) * time.Hour + ticker := time.NewTicker(cleanupInterval) + + go func() { + defer ticker.Stop() + + // Run cleanup immediately on startup + log.WithContext(cleanupCtx).Infof("starting access log cleanup routine (retention: %d days, interval: %d hours)", retentionDays, cleanupIntervalHours) + if _, err := m.CleanupOldAccessLogs(cleanupCtx, retentionDays); err != nil { + log.WithContext(cleanupCtx).Errorf("initial access log cleanup failed: %v", err) + } + + for { + select { + case <-cleanupCtx.Done(): + log.WithContext(cleanupCtx).Info("stopping access log cleanup routine") + return + case <-ticker.C: + if _, err := m.CleanupOldAccessLogs(cleanupCtx, retentionDays); err != nil { + log.WithContext(cleanupCtx).Errorf("periodic access log cleanup failed: %v", err) + } + } + } + }() +} + +// StopPeriodicCleanup stops the periodic cleanup routine +func (m *managerImpl) StopPeriodicCleanup() { + if m.cleanupCancel != nil { + m.cleanupCancel() + } +} + // resolveUserFilters converts user email/name filters to user ID filter func (m *managerImpl) resolveUserFilters(ctx context.Context, accountID string, filter *accesslogs.AccessLogFilter) error { if filter.UserEmail == nil && filter.UserName == nil { diff --git a/management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go b/management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go new file mode 100644 index 00000000000..8fadef85f03 --- /dev/null +++ b/management/internals/modules/reverseproxy/accesslogs/manager/manager_test.go @@ -0,0 +1,281 @@ +package manager + +import ( + "context" + "testing" + "time" + + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/netbirdio/netbird/management/server/store" +) + +func TestCleanupOldAccessLogs(t *testing.T) { + tests := []struct { + name string + retentionDays int + setupMock func(*store.MockStore) + expectedCount int64 + expectedError bool + }{ + { + name: "cleanup logs older than retention period", + retentionDays: 30, + setupMock: func(mockStore *store.MockStore) { + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, olderThan time.Time) (int64, error) { + expectedCutoff := time.Now().AddDate(0, 0, -30) + timeDiff := olderThan.Sub(expectedCutoff) + if timeDiff.Abs() > time.Second { + t.Errorf("cutoff time not as expected: got %v, want ~%v", olderThan, expectedCutoff) + } + return 5, nil + }) + }, + expectedCount: 5, + expectedError: false, + }, + { + name: "no logs to cleanup", + retentionDays: 30, + setupMock: func(mockStore *store.MockStore) { + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + Return(int64(0), nil) + }, + expectedCount: 0, + expectedError: false, + }, + { + name: "zero retention days skips cleanup", + retentionDays: 0, + setupMock: func(mockStore *store.MockStore) { + // No expectations - DeleteOldAccessLogs should not be called + }, + expectedCount: 0, + expectedError: false, + }, + { + name: "negative retention days skips cleanup", + retentionDays: -10, + setupMock: func(mockStore *store.MockStore) { + // No expectations - DeleteOldAccessLogs should not be called + }, + expectedCount: 0, + expectedError: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + tt.setupMock(mockStore) + + manager := &managerImpl{ + store: mockStore, + } + + ctx := context.Background() + deletedCount, err := manager.CleanupOldAccessLogs(ctx, tt.retentionDays) + + if tt.expectedError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + assert.Equal(t, tt.expectedCount, deletedCount, "unexpected number of deleted logs") + }) + } +} + +func TestCleanupWithExactBoundary(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + DoAndReturn(func(ctx context.Context, olderThan time.Time) (int64, error) { + expectedCutoff := time.Now().AddDate(0, 0, -30) + timeDiff := olderThan.Sub(expectedCutoff) + assert.Less(t, timeDiff.Abs(), time.Second, "cutoff time should be close to expected value") + return 1, nil + }) + + manager := &managerImpl{ + store: mockStore, + } + + ctx := context.Background() + deletedCount, err := manager.CleanupOldAccessLogs(ctx, 30) + + require.NoError(t, err) + assert.Equal(t, int64(1), deletedCount) +} + +func TestStartPeriodicCleanup(t *testing.T) { + t.Run("periodic cleanup disabled with zero retention", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + // No expectations - cleanup should not run + + manager := &managerImpl{ + store: mockStore, + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + manager.StartPeriodicCleanup(ctx, 0, 1) + + time.Sleep(100 * time.Millisecond) + + // If DeleteOldAccessLogs was called, the test will fail due to unexpected call + }) + + t.Run("periodic cleanup runs immediately on start", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + Return(int64(2), nil). + Times(1) + + manager := &managerImpl{ + store: mockStore, + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + manager.StartPeriodicCleanup(ctx, 30, 24) + + time.Sleep(200 * time.Millisecond) + + // Expectations verified by gomock on defer ctrl.Finish() + }) + + t.Run("periodic cleanup stops on context cancel", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + Return(int64(1), nil). + Times(1) + + manager := &managerImpl{ + store: mockStore, + } + + ctx, cancel := context.WithCancel(context.Background()) + + manager.StartPeriodicCleanup(ctx, 30, 24) + + time.Sleep(100 * time.Millisecond) + + cancel() + + time.Sleep(200 * time.Millisecond) + + }) + + t.Run("cleanup interval defaults to 24 hours when invalid", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + Return(int64(0), nil). + Times(1) + + manager := &managerImpl{ + store: mockStore, + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + manager.StartPeriodicCleanup(ctx, 30, 0) + + time.Sleep(100 * time.Millisecond) + + manager.StopPeriodicCleanup() + }) + + t.Run("cleanup interval uses configured hours", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + Return(int64(3), nil). + Times(1) + + manager := &managerImpl{ + store: mockStore, + } + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + manager.StartPeriodicCleanup(ctx, 30, 12) + + time.Sleep(100 * time.Millisecond) + + manager.StopPeriodicCleanup() + }) +} + +func TestStopPeriodicCleanup(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockStore := store.NewMockStore(ctrl) + + mockStore.EXPECT(). + DeleteOldAccessLogs(gomock.Any(), gomock.Any()). + Return(int64(1), nil). + Times(1) + + manager := &managerImpl{ + store: mockStore, + } + + ctx := context.Background() + + manager.StartPeriodicCleanup(ctx, 30, 24) + + time.Sleep(100 * time.Millisecond) + + manager.StopPeriodicCleanup() + + time.Sleep(200 * time.Millisecond) + + // Expectations verified by gomock - would fail if more than 1 call happened +} + +func TestStopPeriodicCleanup_NotStarted(t *testing.T) { + manager := &managerImpl{} + + // Should not panic if cleanup was never started + manager.StopPeriodicCleanup() +} diff --git a/management/internals/server/boot.go b/management/internals/server/boot.go index 7da1e689830..e897a09f55c 100644 --- a/management/internals/server/boot.go +++ b/management/internals/server/boot.go @@ -197,6 +197,11 @@ func (s *BaseServer) ProxyTokenStore() *nbgrpc.OneTimeTokenStore { func (s *BaseServer) AccessLogsManager() accesslogs.Manager { return Create(s, func() accesslogs.Manager { accessLogManager := accesslogsmanager.NewManager(s.Store(), s.PermissionsManager(), s.GeoLocationManager()) + accessLogManager.StartPeriodicCleanup( + context.Background(), + s.Config.ReverseProxy.AccessLogRetentionDays, + s.Config.ReverseProxy.AccessLogCleanupIntervalHours, + ) return accessLogManager }) } diff --git a/management/internals/server/config/config.go b/management/internals/server/config/config.go index 5ed1c3ede27..0ba3932634c 100644 --- a/management/internals/server/config/config.go +++ b/management/internals/server/config/config.go @@ -200,4 +200,13 @@ type ReverseProxy struct { // request headers if the peer's address falls within one of these // trusted IP prefixes. TrustedPeers []netip.Prefix + + // AccessLogRetentionDays specifies the number of days to retain access logs. + // Logs older than this duration will be automatically deleted during cleanup. + // A value of 0 or negative means logs are kept indefinitely (no cleanup). + AccessLogRetentionDays int + + // AccessLogCleanupIntervalHours specifies how often (in hours) to run the cleanup routine. + // Defaults to 24 hours if not set or set to 0. + AccessLogCleanupIntervalHours int } diff --git a/management/server/http/handlers/proxy/auth_callback_integration_test.go b/management/server/http/handlers/proxy/auth_callback_integration_test.go index 0a9a560cde4..6a1b144f6fa 100644 --- a/management/server/http/handlers/proxy/auth_callback_integration_test.go +++ b/management/server/http/handlers/proxy/auth_callback_integration_test.go @@ -157,6 +157,18 @@ type testSetup struct { // testAccessLogManager is a minimal mock for accesslogs.Manager. type testAccessLogManager struct{} +func (m *testAccessLogManager) CleanupOldAccessLogs(ctx context.Context, retentionDays int) (int64, error) { + return 0, nil +} + +func (m *testAccessLogManager) StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) { + return +} + +func (m *testAccessLogManager) StopPeriodicCleanup() { + return +} + func (m *testAccessLogManager) SaveAccessLog(_ context.Context, _ *accesslogs.AccessLogEntry) error { return nil } diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index db7cfd32d24..e528cb4fbfc 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -5100,6 +5100,20 @@ func (s *SqlStore) GetAccountAccessLogs(ctx context.Context, lockStrength Lockin return logs, totalCount, nil } +// DeleteOldAccessLogs deletes all access logs older than the specified time +func (s *SqlStore) DeleteOldAccessLogs(ctx context.Context, olderThan time.Time) (int64, error) { + result := s.db.WithContext(ctx). + Where("timestamp < ?", olderThan). + Delete(&accesslogs.AccessLogEntry{}) + + if result.Error != nil { + log.WithContext(ctx).Errorf("failed to delete old access logs: %v", result.Error) + return 0, status.Errorf(status.Internal, "failed to delete old access logs") + } + + return result.RowsAffected, nil +} + // applyAccessLogFilters applies filter conditions to the query func (s *SqlStore) applyAccessLogFilters(query *gorm.DB, filter accesslogs.AccessLogFilter) *gorm.DB { if filter.Search != nil { diff --git a/management/server/store/store.go b/management/server/store/store.go index a8e44a438b8..2bc688a11b0 100644 --- a/management/server/store/store.go +++ b/management/server/store/store.go @@ -269,6 +269,7 @@ type Store interface { CreateAccessLog(ctx context.Context, log *accesslogs.AccessLogEntry) error GetAccountAccessLogs(ctx context.Context, lockStrength LockingStrength, accountID string, filter accesslogs.AccessLogFilter) ([]*accesslogs.AccessLogEntry, int64, error) + DeleteOldAccessLogs(ctx context.Context, olderThan time.Time) (int64, error) GetServiceTargetByTargetID(ctx context.Context, lockStrength LockingStrength, accountID string, targetID string) (*reverseproxy.Target, error) } diff --git a/management/server/store/store_mock.go b/management/server/store/store_mock.go index 2f451dc430b..79d27529872 100644 --- a/management/server/store/store_mock.go +++ b/management/server/store/store_mock.go @@ -460,6 +460,21 @@ func (mr *MockStoreMockRecorder) DeleteNetworkRouter(ctx, accountID, routerID in return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteNetworkRouter", reflect.TypeOf((*MockStore)(nil).DeleteNetworkRouter), ctx, accountID, routerID) } +// DeleteOldAccessLogs mocks base method. +func (m *MockStore) DeleteOldAccessLogs(ctx context.Context, olderThan time.Time) (int64, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteOldAccessLogs", ctx, olderThan) + ret0, _ := ret[0].(int64) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// DeleteOldAccessLogs indicates an expected call of DeleteOldAccessLogs. +func (mr *MockStoreMockRecorder) DeleteOldAccessLogs(ctx, olderThan interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteOldAccessLogs", reflect.TypeOf((*MockStore)(nil).DeleteOldAccessLogs), ctx, olderThan) +} + // DeletePAT mocks base method. func (m *MockStore) DeletePAT(ctx context.Context, userID, patID string) error { m.ctrl.T.Helper() diff --git a/proxy/management_integration_test.go b/proxy/management_integration_test.go index 53d7019f724..1163c50f462 100644 --- a/proxy/management_integration_test.go +++ b/proxy/management_integration_test.go @@ -165,6 +165,18 @@ func setupIntegrationTest(t *testing.T) *integrationTestSetup { // testAccessLogManager provides access log storage for testing. type testAccessLogManager struct{} +func (m *testAccessLogManager) CleanupOldAccessLogs(ctx context.Context, retentionDays int) (int64, error) { + return 0, nil +} + +func (m *testAccessLogManager) StartPeriodicCleanup(ctx context.Context, retentionDays, cleanupIntervalHours int) { + // noop +} + +func (m *testAccessLogManager) StopPeriodicCleanup() { + // noop +} + func (m *testAccessLogManager) SaveAccessLog(_ context.Context, _ *accesslogs.AccessLogEntry) error { return nil }