Skip to content

Commit 9ec90cf

Browse files
mafredrimafredri
authored
feat(coderd/database/dbpurge): make API keys retention configurable (#21037)
Replace hardcoded 7-day retention for expired API keys with configurable retention from deployment settings. Skips deletion entirely when effective retention is 0. Depends on #21021 Updates #20743
1 parent 929db24 commit 9ec90cf

File tree

4 files changed

+167
-41
lines changed

4 files changed

+167
-41
lines changed

‎coderd/database/dbpurge/dbpurge.go‎

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -82,18 +82,24 @@ func New(ctx context.Context, logger slog.Logger, db database.Store, vals *coder
8282
if err := tx.ExpirePrebuildsAPIKeys(ctx, dbtime.Time(start)); err != nil {
8383
return xerrors.Errorf("failed to expire prebuilds user api keys: %w", err)
8484
}
85-
expiredAPIKeys, err := tx.DeleteExpiredAPIKeys(ctx, database.DeleteExpiredAPIKeysParams{
86-
// Leave expired keys for a week to allow the backend to know the difference
87-
// between a 404 and an expired key. This purge code is just to bound the size of
88-
// the table to something more reasonable.
89-
Before: dbtime.Time(start.Add(time.Hour * 24 * 7 * -1)),
90-
// There could be a lot of expired keys here, so set a limit to prevent this
91-
// taking too long.
92-
// This runs every 10 minutes, so it deletes ~1.5m keys per day at most.
93-
LimitCount: 10000,
94-
})
95-
if err != nil {
96-
return xerrors.Errorf("failed to delete expired api keys: %w", err)
85+
86+
var expiredAPIKeys int64
87+
apiKeysRetention := vals.Retention.APIKeys.Value()
88+
if apiKeysRetention > 0 {
89+
// Delete keys that have been expired for at least the retention period.
90+
// A higher retention period allows the backend to return a more helpful
91+
// error message when a user tries to use an expired key.
92+
deleteExpiredKeysBefore := start.Add(-apiKeysRetention)
93+
expiredAPIKeys, err = tx.DeleteExpiredAPIKeys(ctx, database.DeleteExpiredAPIKeysParams{
94+
Before: dbtime.Time(deleteExpiredKeysBefore),
95+
// There could be a lot of expired keys here, so set a limit to prevent
96+
// this taking too long. This runs every 10 minutes, so it deletes
97+
// ~1.5m keys per day at most.
98+
LimitCount: 10000,
99+
})
100+
if err != nil {
101+
return xerrors.Errorf("failed to delete expired api keys: %w", err)
102+
}
97103
}
98104
deleteOldTelemetryLocksBefore := start.Add(-maxTelemetryHeartbeatAge)
99105
if err := tx.DeleteOldTelemetryLocks(ctx, deleteOldTelemetryLocksBefore); err != nil {

‎coderd/database/dbpurge/dbpurge_test.go‎

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1245,3 +1245,131 @@ func TestDeleteOldAuditLogs(t *testing.T) {
12451245
require.NotContains(t, logIDs, oldCreateLog.ID, "old create log should be deleted by audit logs retention")
12461246
})
12471247
}
1248+
1249+
func TestDeleteExpiredAPIKeys(t *testing.T) {
1250+
t.Parallel()
1251+
1252+
now := time.Date(2025, 1, 15, 7, 30, 0, 0, time.UTC)
1253+
1254+
testCases := []struct {
1255+
name string
1256+
retentionConfig codersdk.RetentionConfig
1257+
oldExpiredTime time.Time
1258+
recentExpiredTime *time.Time // nil means no recent expired key created
1259+
activeTime *time.Time // nil means no active key created
1260+
expectOldExpiredDeleted bool
1261+
expectedKeysRemaining int
1262+
}{
1263+
{
1264+
name: "RetentionEnabled",
1265+
retentionConfig: codersdk.RetentionConfig{
1266+
APIKeys: serpent.Duration(7 * 24 * time.Hour), // 7 days
1267+
},
1268+
oldExpiredTime: now.Add(-8 * 24 * time.Hour), // Expired 8 days ago
1269+
recentExpiredTime: ptr(now.Add(-6 * 24 * time.Hour)), // Expired 6 days ago
1270+
activeTime: ptr(now.Add(24 * time.Hour)), // Expires tomorrow
1271+
expectOldExpiredDeleted: true,
1272+
expectedKeysRemaining: 2, // recent expired + active
1273+
},
1274+
{
1275+
name: "RetentionDisabled",
1276+
retentionConfig: codersdk.RetentionConfig{
1277+
APIKeys: serpent.Duration(0),
1278+
},
1279+
oldExpiredTime: now.Add(-365 * 24 * time.Hour), // Expired 1 year ago
1280+
recentExpiredTime: nil,
1281+
activeTime: nil,
1282+
expectOldExpiredDeleted: false,
1283+
expectedKeysRemaining: 1, // old expired is kept
1284+
},
1285+
1286+
{
1287+
name: "CustomRetention30Days",
1288+
retentionConfig: codersdk.RetentionConfig{
1289+
APIKeys: serpent.Duration(30 * 24 * time.Hour), // 30 days
1290+
},
1291+
oldExpiredTime: now.Add(-31 * 24 * time.Hour), // Expired 31 days ago
1292+
recentExpiredTime: ptr(now.Add(-29 * 24 * time.Hour)), // Expired 29 days ago
1293+
activeTime: nil,
1294+
expectOldExpiredDeleted: true,
1295+
expectedKeysRemaining: 1, // only recent expired remains
1296+
},
1297+
}
1298+
1299+
for _, tc := range testCases {
1300+
t.Run(tc.name, func(t *testing.T) {
1301+
t.Parallel()
1302+
1303+
ctx := testutil.Context(t, testutil.WaitShort)
1304+
clk := quartz.NewMock(t)
1305+
clk.Set(now).MustWait(ctx)
1306+
1307+
db, _ := dbtestutil.NewDB(t, dbtestutil.WithDumpOnFailure())
1308+
logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: true})
1309+
user := dbgen.User(t, db, database.User{})
1310+
1311+
// Create API key that expired long ago.
1312+
oldExpiredKey, _ := dbgen.APIKey(t, db, database.APIKey{
1313+
UserID: user.ID,
1314+
ExpiresAt: tc.oldExpiredTime,
1315+
TokenName: "old-expired-key",
1316+
})
1317+
1318+
// Create API key that expired recently if specified.
1319+
var recentExpiredKey database.APIKey
1320+
if tc.recentExpiredTime != nil {
1321+
recentExpiredKey, _ = dbgen.APIKey(t, db, database.APIKey{
1322+
UserID: user.ID,
1323+
ExpiresAt: *tc.recentExpiredTime,
1324+
TokenName: "recent-expired-key",
1325+
})
1326+
}
1327+
1328+
// Create API key that hasn't expired yet if specified.
1329+
var activeKey database.APIKey
1330+
if tc.activeTime != nil {
1331+
activeKey, _ = dbgen.APIKey(t, db, database.APIKey{
1332+
UserID: user.ID,
1333+
ExpiresAt: *tc.activeTime,
1334+
TokenName: "active-key",
1335+
})
1336+
}
1337+
1338+
// Run the purge.
1339+
done := awaitDoTick(ctx, t, clk)
1340+
closer := dbpurge.New(ctx, logger, db, &codersdk.DeploymentValues{
1341+
Retention: tc.retentionConfig,
1342+
}, clk)
1343+
defer closer.Close()
1344+
testutil.TryReceive(ctx, t, done)
1345+
1346+
// Verify total keys remaining.
1347+
keys, err := db.GetAPIKeysLastUsedAfter(ctx, time.Time{})
1348+
require.NoError(t, err)
1349+
require.Len(t, keys, tc.expectedKeysRemaining, "unexpected number of keys remaining")
1350+
1351+
// Verify results.
1352+
_, err = db.GetAPIKeyByID(ctx, oldExpiredKey.ID)
1353+
if tc.expectOldExpiredDeleted {
1354+
require.Error(t, err, "old expired key should be deleted")
1355+
} else {
1356+
require.NoError(t, err, "old expired key should NOT be deleted")
1357+
}
1358+
1359+
if tc.recentExpiredTime != nil {
1360+
_, err = db.GetAPIKeyByID(ctx, recentExpiredKey.ID)
1361+
require.NoError(t, err, "recently expired key should be kept")
1362+
}
1363+
1364+
if tc.activeTime != nil {
1365+
_, err = db.GetAPIKeyByID(ctx, activeKey.ID)
1366+
require.NoError(t, err, "active key should be kept")
1367+
}
1368+
})
1369+
}
1370+
}
1371+
1372+
// ptr is a helper to create a pointer to a value.
1373+
func ptr[T any](v T) *T {
1374+
return &v
1375+
}

‎coderd/database/queries.sql.go‎

Lines changed: 13 additions & 16 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/apikeys.sql‎

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -85,25 +85,20 @@ DELETE FROM
8585
WHERE
8686
user_id = $1;
8787

88-
-- name: DeleteExpiredAPIKeys :one
88+
-- name: DeleteExpiredAPIKeys :execrows
8989
WITH expired_keys AS (
9090
SELECT id
9191
FROM api_keys
9292
-- expired keys only
9393
WHERE expires_at < @before::timestamptz
9494
LIMIT @limit_count
95-
),
96-
deleted_rows AS (
97-
DELETE FROM
98-
api_keys
99-
USING
100-
expired_keys
101-
WHERE
102-
api_keys.id = expired_keys.id
103-
RETURNING api_keys.id
104-
)
105-
SELECT COUNT(deleted_rows.id) AS deleted_count FROM deleted_rows;
106-
;
95+
)
96+
DELETE FROM
97+
api_keys
98+
USING
99+
expired_keys
100+
WHERE
101+
api_keys.id = expired_keys.id;
107102

108103
-- name: ExpirePrebuildsAPIKeys :exec
109104
-- Firstly, collect api_keys owned by the prebuilds user that correlate

0 commit comments

Comments
 (0)