[proxy] add access log cleanup (#5376)

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()
 }

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 {

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()
+}

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
 	})
 }

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
 }

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
 }

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 {

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)
 }
 

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()