diff --git a/coderd/database/dbpurge/dbpurge.go b/coderd/database/dbpurge/dbpurge.go index 4b1f9f7628e16..7f953633bf64d 100644 --- a/coderd/database/dbpurge/dbpurge.go +++ b/coderd/database/dbpurge/dbpurge.go @@ -82,18 +82,24 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, vals *coder if err := tx.ExpirePrebuildsAPIKeys(ctx, dbtime.Time(start)); err != nil { return xerrors.Errorf("failed to expire prebuilds user api keys: %w", err) } - expiredAPIKeys, err := tx.DeleteExpiredAPIKeys(ctx, database.DeleteExpiredAPIKeysParams{ - // Leave expired keys for a week to allow the backend to know the difference - // between a 404 and an expired key. This purge code is just to bound the size of - // the table to something more reasonable. - Before: dbtime.Time(start.Add(time.Hour * 24 * 7 * -1)), - // There could be a lot of expired keys here, so set a limit to prevent this - // taking too long. - // This runs every 10 minutes, so it deletes ~1.5m keys per day at most. - LimitCount: 10000, - }) - if err != nil { - return xerrors.Errorf("failed to delete expired api keys: %w", err) + + var expiredAPIKeys int64 + apiKeysRetention := vals.Retention.APIKeys.Value() + if apiKeysRetention > 0 { + // Delete keys that have been expired for at least the retention period. + // A higher retention period allows the backend to return a more helpful + // error message when a user tries to use an expired key. + deleteExpiredKeysBefore := start.Add(-apiKeysRetention) + expiredAPIKeys, err = tx.DeleteExpiredAPIKeys(ctx, database.DeleteExpiredAPIKeysParams{ + Before: dbtime.Time(deleteExpiredKeysBefore), + // There could be a lot of expired keys here, so set a limit to prevent + // this taking too long. This runs every 10 minutes, so it deletes + // ~1.5m keys per day at most. + LimitCount: 10000, + }) + if err != nil { + return xerrors.Errorf("failed to delete expired api keys: %w", err) + } } deleteOldTelemetryLocksBefore := start.Add(-maxTelemetryHeartbeatAge) if err := tx.DeleteOldTelemetryLocks(ctx, deleteOldTelemetryLocksBefore); err != nil { diff --git a/coderd/database/dbpurge/dbpurge_test.go b/coderd/database/dbpurge/dbpurge_test.go index 26e778b00a559..5b8bb35d8969f 100644 --- a/coderd/database/dbpurge/dbpurge_test.go +++ b/coderd/database/dbpurge/dbpurge_test.go @@ -1245,3 +1245,131 @@ func TestDeleteOldAuditLogs(t *testing.T) { require.NotContains(t, logIDs, oldCreateLog.ID, "old create log should be deleted by audit logs retention") }) } + +func TestDeleteExpiredAPIKeys(t *testing.T) { + t.Parallel() + + now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC) + + testCases := []struct { + name string + retentionConfig codersdk.RetentionConfig + oldExpiredTime time.Time + recentExpiredTime *time.Time // nil means no recent expired key created + activeTime *time.Time // nil means no active key created + expectOldExpiredDeleted bool + expectedKeysRemaining int + }{ + { + name: "RetentionEnabled", + retentionConfig: codersdk.RetentionConfig{ + APIKeys: serpent.Duration(7 * 24 * time.Hour), // 7 days + }, + oldExpiredTime: now.Add(-8 * 24 * time.Hour), // Expired 8 days ago + recentExpiredTime: ptr(now.Add(-6 * 24 * time.Hour)), // Expired 6 days ago + activeTime: ptr(now.Add(24 * time.Hour)), // Expires tomorrow + expectOldExpiredDeleted: true, + expectedKeysRemaining: 2, // recent expired + active + }, + { + name: "RetentionDisabled", + retentionConfig: codersdk.RetentionConfig{ + APIKeys: serpent.Duration(0), + }, + oldExpiredTime: now.Add(-365 * 24 * time.Hour), // Expired 1 year ago + recentExpiredTime: nil, + activeTime: nil, + expectOldExpiredDeleted: false, + expectedKeysRemaining: 1, // old expired is kept + }, + + { + name: "CustomRetention30Days", + retentionConfig: codersdk.RetentionConfig{ + APIKeys: serpent.Duration(30 * 24 * time.Hour), // 30 days + }, + oldExpiredTime: now.Add(-31 * 24 * time.Hour), // Expired 31 days ago + recentExpiredTime: ptr(now.Add(-29 * 24 * time.Hour)), // Expired 29 days ago + activeTime: nil, + expectOldExpiredDeleted: true, + expectedKeysRemaining: 1, // only recent expired remains + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitShort) + clk := quartz.NewMock(t) + clk.Set(now).MustWait(ctx) + + db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure()) + logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true}) + user := dbgen.User(t, db, database.User{}) + + // Create API key that expired long ago. + oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: tc.oldExpiredTime, + TokenName: "old-expired-key", + }) + + // Create API key that expired recently if specified. + var recentExpiredKey database.APIKey + if tc.recentExpiredTime != nil { + recentExpiredKey, _ = dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: *tc.recentExpiredTime, + TokenName: "recent-expired-key", + }) + } + + // Create API key that hasn't expired yet if specified. + var activeKey database.APIKey + if tc.activeTime != nil { + activeKey, _ = dbgen.APIKey(t, db, database.APIKey{ + UserID: user.ID, + ExpiresAt: *tc.activeTime, + TokenName: "active-key", + }) + } + + // Run the purge. + done := awaitDoTick(ctx, t, clk) + closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{ + Retention: tc.retentionConfig, + }, clk) + defer closer.Close() + testutil.TryReceive(ctx, t, done) + + // Verify total keys remaining. + keys, err := db.GetAPIKeysLastUsedAfter(ctx, time.Time{}) + require.NoError(t, err) + require.Len(t, keys, tc.expectedKeysRemaining, "unexpected number of keys remaining") + + // Verify results. + _, err = db.GetAPIKeyByID(ctx, oldExpiredKey.ID) + if tc.expectOldExpiredDeleted { + require.Error(t, err, "old expired key should be deleted") + } else { + require.NoError(t, err, "old expired key should NOT be deleted") + } + + if tc.recentExpiredTime != nil { + _, err = db.GetAPIKeyByID(ctx, recentExpiredKey.ID) + require.NoError(t, err, "recently expired key should be kept") + } + + if tc.activeTime != nil { + _, err = db.GetAPIKeyByID(ctx, activeKey.ID) + require.NoError(t, err, "active key should be kept") + } + }) + } +} + +// ptr is a helper to create a pointer to a value. +func ptr[T any](v T) *T { + return &v +} diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 82d8639abfeb1..d102c3283db07 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -1107,24 +1107,20 @@ func (q *sqlQuerier) DeleteApplicationConnectAPIKeysByUserID(ctx context.Context return err } -const deleteExpiredAPIKeys = `-- name: DeleteExpiredAPIKeys :one +const deleteExpiredAPIKeys = `-- name: DeleteExpiredAPIKeys :execrows WITH expired_keys AS ( SELECT id FROM api_keys -- expired keys only WHERE expires_at < $1::timestamptz LIMIT $2 -), -deleted_rows AS ( - DELETE FROM - api_keys - USING - expired_keys - WHERE - api_keys.id = expired_keys.id - RETURNING api_keys.id - ) -SELECT COUNT(deleted_rows.id) AS deleted_count FROM deleted_rows +) +DELETE FROM + api_keys +USING + expired_keys +WHERE + api_keys.id = expired_keys.id ` type DeleteExpiredAPIKeysParams struct { @@ -1133,10 +1129,11 @@ type DeleteExpiredAPIKeysParams struct { } func (q *sqlQuerier) DeleteExpiredAPIKeys(ctx context.Context, arg DeleteExpiredAPIKeysParams) (int64, error) { - row := q.db.QueryRowContext(ctx, deleteExpiredAPIKeys, arg.Before, arg.LimitCount) - var deleted_count int64 - err := row.Scan(&deleted_count) - return deleted_count, err + result, err := q.db.ExecContext(ctx, deleteExpiredAPIKeys, arg.Before, arg.LimitCount) + if err != nil { + return 0, err + } + return result.RowsAffected() } const expirePrebuildsAPIKeys = `-- name: ExpirePrebuildsAPIKeys :exec diff --git a/coderd/database/queries/apikeys.sql b/coderd/database/queries/apikeys.sql index 2e2be542c9601..226eda7ebe323 100644 --- a/coderd/database/queries/apikeys.sql +++ b/coderd/database/queries/apikeys.sql @@ -85,25 +85,20 @@ DELETE FROM WHERE user_id = $1; --- name: DeleteExpiredAPIKeys :one +-- name: DeleteExpiredAPIKeys :execrows WITH expired_keys AS ( SELECT id FROM api_keys -- expired keys only WHERE expires_at < @before::timestamptz LIMIT @limit_count -), -deleted_rows AS ( - DELETE FROM - api_keys - USING - expired_keys - WHERE - api_keys.id = expired_keys.id - RETURNING api_keys.id - ) -SELECT COUNT(deleted_rows.id) AS deleted_count FROM deleted_rows; -; +) +DELETE FROM + api_keys +USING + expired_keys +WHERE + api_keys.id = expired_keys.id; -- name: ExpirePrebuildsAPIKeys :exec -- Firstly, collect api_keys owned by the prebuilds user that correlate