From 787def06fd78f3e76216495b9978856ecf0560e5 Mon Sep 17 00:00:00 2001 From: John Coffey Date: Fri, 21 Aug 2026 14:49:18 -0700 Subject: [PATCH] Add owner/admin-only log retention deletion to Settings New api/logretention package: GET /logs/retention/preview and DELETE /logs/retention, both gated to RoleAdmin (Owner satisfies it too), issue purpose-built parameterized statements against ClickHouse's logs table (a count and a synchronous ALTER TABLE ... DELETE mutation) rather than routing through querylang/executor's SELECT-only SQLRunner. Settings gets a new "Log retention" section, visible only to an owner or admin, that previews how many records a chosen age cutoff would remove before showing an explicit confirm/cancel panel -- no delete happens without that second step. Scoped to core's single-tenant ClickHouse table; enterprise/'s per-tenant routing and Tantivy's lack of a bulk-delete primitive are disclosed gaps in api/logretention/store.go's doc comment, not silently assumed to already work. --- api/cmd/api/main.go | 8 + api/logretention/handler.go | 138 ++++++++++++++++ api/logretention/handler_test.go | 207 +++++++++++++++++++++++ api/logretention/store.go | 70 ++++++++ web/src/lib/api.ts | 16 ++ web/src/routes/settings/+page.svelte | 238 ++++++++++++++++++++++++++- 6 files changed, 676 insertions(+), 1 deletion(-) create mode 100644 api/logretention/handler.go create mode 100644 api/logretention/handler_test.go create mode 100644 api/logretention/store.go diff --git a/api/cmd/api/main.go b/api/cmd/api/main.go index 7cc87c4..8de7922 100644 --- a/api/cmd/api/main.go +++ b/api/cmd/api/main.go @@ -33,6 +33,7 @@ import ( "github.com/sentry/sentry/api/httpserver" "github.com/sentry/sentry/api/internal/config" "github.com/sentry/sentry/api/localauth" + "github.com/sentry/sentry/api/logretention" "github.com/sentry/sentry/api/queryapi" "github.com/sentry/sentry/api/querylang/executor" "github.com/sentry/sentry/api/searchclient" @@ -159,6 +160,12 @@ func main() { // queryHandler's/aiHandler's nil audit loggers above. agentsHandler := agents.NewHandler(logger, agents.NewStore(pgPool), authorizer, nil) + // Same conn sqlRunner above already wraps -- logretention issues its + // own purpose-built statements against the `logs` table directly + // rather than going through sqlRunner's SELECT-only RunSQL (see + // logretention.Store's doc comment). + logRetentionHandler := logretention.NewHandler(logger, logretention.NewStore(conn), authorizer) + // One shared mux, CORS applied once around the whole thing -- see // httpserver's doc comment for why this changed from each // handler wrapping itself individually. @@ -166,6 +173,7 @@ func main() { queryHandler.RegisterRoutes(mux) dashboardsHandler.RegisterRoutes(mux) agentsHandler.RegisterRoutes(mux) + logRetentionHandler.RegisterRoutes(mux) // Only registered when local auth is actually enabled -- see // localauth.Handler.RegisterRoutes' doc comment for why a disabled diff --git a/api/logretention/handler.go b/api/logretention/handler.go new file mode 100644 index 0000000..7fd04c0 --- /dev/null +++ b/api/logretention/handler.go @@ -0,0 +1,138 @@ +package logretention + +import ( + "context" + "encoding/json" + "log/slog" + "net/http" + "strconv" + "time" + + "github.com/sentry/sentry/api/authz" +) + +// store is the narrow interface Handler depends on -- *Store (store.go) +// is the production implementation; tests use a fake, same pattern as +// agents.store/dashboards.store. +type store interface { + CountOlderThan(ctx context.Context, cutoff time.Time) (uint64, error) + DeleteOlderThan(ctx context.Context, cutoff time.Time) error +} + +// maxOlderThanHours bounds the age a caller can specify -- 10 years is +// far beyond any real retention window this feature exists for, and +// exists only to reject an obviously-wrong input (e.g. a stray extra +// digit) with a clear 400 rather than silently accepting it. +const maxOlderThanHours = 10 * 365 * 24 + +type Handler struct { + logger *slog.Logger + store store + authorizer authz.Authorizer +} + +func NewHandler(logger *slog.Logger, store store, authorizer authz.Authorizer) *Handler { + return &Handler{logger: logger, store: store, authorizer: authorizer} +} + +// RegisterRoutes: both routes are RoleAdmin -- RoleOwner satisfies it +// too (Role.Satisfies is a floor, not an exact match), matching the +// "owner and admin" requirement this feature shipped for. Permanently +// deleting log data is at least as consequential as the RBAC matrix's +// other RoleAdmin-floor actions (e.g. issuing an agent restart +// command, api/agents/handler.go), so it gets the same floor rather +// than a stricter RoleOwner-only one. +func (h *Handler) RegisterRoutes(mux *http.ServeMux) { + mux.HandleFunc("GET /logs/retention/preview", authz.RequireRole(h.authorizer, authz.RoleAdmin, h.handlePreview)) + mux.HandleFunc("DELETE /logs/retention", authz.RequireRole(h.authorizer, authz.RoleAdmin, h.handleDelete)) +} + +// parseOlderThanHours reads and validates the older_than_hours query +// param shared by both routes -- a caller must ask for at least 1 hour +// (an accidental empty/zero value must never mean "delete everything"). +func parseOlderThanHours(r *http.Request) (int, bool) { + hours, err := strconv.Atoi(r.URL.Query().Get("older_than_hours")) + if err != nil || hours < 1 || hours > maxOlderThanHours { + return 0, false + } + return hours, true +} + +type previewResponse struct { + Count uint64 `json:"count"` + Cutoff time.Time `json:"cutoff"` +} + +func (h *Handler) handlePreview(w http.ResponseWriter, r *http.Request) { + hours, ok := parseOlderThanHours(r) + if !ok { + writeError(w, http.StatusBadRequest, "older_than_hours must be a positive integer") + return + } + cutoff := time.Now().UTC().Add(-time.Duration(hours) * time.Hour) + + count, err := h.store.CountOlderThan(r.Context(), cutoff) + if err != nil { + h.logger.Error("counting logs for retention preview", "error", err) + writeError(w, http.StatusInternalServerError, "counting logs failed") + return + } + writeJSON(w, http.StatusOK, previewResponse{Count: count, Cutoff: cutoff}) +} + +type deleteResponse struct { + DeletedCount uint64 `json:"deleted_count"` + Cutoff time.Time `json:"cutoff"` +} + +// handleDelete counts immediately before deleting so the response can +// report how many records were actually removed -- ClickHouse's ALTER +// TABLE DELETE mutation itself reports no row count. A handful of +// records landing between this count and the delete would still be +// older than the fixed cutoff by the time they land, so the delete +// catches them too even though this count didn't -- an acceptable, +// disclosed margin for an admin-facing summary number, not something +// anything downstream depends on for correctness. +func (h *Handler) handleDelete(w http.ResponseWriter, r *http.Request) { + hours, ok := parseOlderThanHours(r) + if !ok { + writeError(w, http.StatusBadRequest, "older_than_hours must be a positive integer") + return + } + cutoff := time.Now().UTC().Add(-time.Duration(hours) * time.Hour) + + count, err := h.store.CountOlderThan(r.Context(), cutoff) + if err != nil { + h.logger.Error("counting logs before retention delete", "error", err) + writeError(w, http.StatusInternalServerError, "counting logs failed") + return + } + + if err := h.store.DeleteOlderThan(r.Context(), cutoff); err != nil { + h.logger.Error("deleting logs by retention age", "error", err) + writeError(w, http.StatusInternalServerError, "deleting logs failed") + return + } + + identity, _ := authz.IdentityFromContext(r.Context()) + h.logger.Info("logs deleted by retention age", + "deleted_count", count, "cutoff", cutoff, "user_id", identity.UserID, "role", identity.Role) + + writeJSON(w, http.StatusOK, deleteResponse{DeletedCount: count, Cutoff: cutoff}) +} + +func writeJSON(w http.ResponseWriter, status int, v any) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(v) +} + +type errorResponse struct { + Error string `json:"error"` +} + +func writeError(w http.ResponseWriter, status int, msg string) { + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + _ = json.NewEncoder(w).Encode(errorResponse{Error: msg}) +} diff --git a/api/logretention/handler_test.go b/api/logretention/handler_test.go new file mode 100644 index 0000000..4f1fd04 --- /dev/null +++ b/api/logretention/handler_test.go @@ -0,0 +1,207 @@ +package logretention + +import ( + "context" + "encoding/json" + "errors" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "testing" + "time" + + "github.com/sentry/sentry/api/authz" +) + +func discardLogger() *slog.Logger { + return slog.New(slog.NewTextHandler(io.Discard, nil)) +} + +// fakeStore records the cutoff it was called with so tests can assert +// the handler computed it correctly from older_than_hours, and lets a +// test inject a store error to exercise the failure paths. +type fakeStore struct { + count uint64 + countErr error + deleteErr error + countedWith []time.Time + deletedWith []time.Time +} + +func (f *fakeStore) CountOlderThan(_ context.Context, cutoff time.Time) (uint64, error) { + f.countedWith = append(f.countedWith, cutoff) + if f.countErr != nil { + return 0, f.countErr + } + return f.count, nil +} + +func (f *fakeStore) DeleteOlderThan(_ context.Context, cutoff time.Time) error { + f.deletedWith = append(f.deletedWith, cutoff) + return f.deleteErr +} + +type fakeAuthorizer struct { + role authz.Role +} + +func (f fakeAuthorizer) Authorize(*http.Request) (authz.Identity, error) { + return authz.Identity{TenantID: "default", UserID: "u1", Role: f.role}, nil +} + +func newTestHandler(s *fakeStore, role authz.Role) *Handler { + return NewHandler(discardLogger(), s, fakeAuthorizer{role: role}) +} + +func doRequest(t *testing.T, h *Handler, method, path string) *httptest.ResponseRecorder { + t.Helper() + req := httptest.NewRequest(method, path, nil) + rec := httptest.NewRecorder() + mux := http.NewServeMux() + h.RegisterRoutes(mux) + mux.ServeHTTP(rec, req) + return rec +} + +func TestPreviewReturnsCountAndCutoff(t *testing.T) { + s := &fakeStore{count: 42} + h := newTestHandler(s, authz.RoleAdmin) + + before := time.Now().UTC() + rec := doRequest(t, h, "GET", "/logs/retention/preview?older_than_hours=24") + after := time.Now().UTC() + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + var resp previewResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decoding response: %v", err) + } + if resp.Count != 42 { + t.Errorf("count = %d, want 42", resp.Count) + } + wantEarliest := before.Add(-24 * time.Hour) + wantLatest := after.Add(-24 * time.Hour) + if resp.Cutoff.Before(wantEarliest) || resp.Cutoff.After(wantLatest) { + t.Errorf("cutoff = %v, want between %v and %v", resp.Cutoff, wantEarliest, wantLatest) + } + if len(s.deletedWith) != 0 { + t.Errorf("preview must never delete anything, but DeleteOlderThan was called %d time(s)", len(s.deletedWith)) + } +} + +func TestDeleteReturnsDeletedCountAndCutoff(t *testing.T) { + s := &fakeStore{count: 7} + h := newTestHandler(s, authz.RoleAdmin) + + rec := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours=720") + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want 200, body=%s", rec.Code, rec.Body.String()) + } + var resp deleteResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("decoding response: %v", err) + } + if resp.DeletedCount != 7 { + t.Errorf("deleted_count = %d, want 7", resp.DeletedCount) + } + if len(s.deletedWith) != 1 { + t.Fatalf("expected exactly one DeleteOlderThan call, got %d", len(s.deletedWith)) + } + if len(s.countedWith) != 1 || !s.countedWith[0].Equal(s.deletedWith[0]) { + t.Errorf("count and delete must use the same cutoff: counted=%v deleted=%v", s.countedWith, s.deletedWith) + } +} + +func TestRejectsMissingOrInvalidOlderThanHours(t *testing.T) { + h := newTestHandler(&fakeStore{}, authz.RoleAdmin) + + cases := []string{ + "/logs/retention/preview", + "/logs/retention/preview?older_than_hours=0", + "/logs/retention/preview?older_than_hours=-5", + "/logs/retention/preview?older_than_hours=notanumber", + "/logs/retention/preview?older_than_hours=999999999", + } + for _, path := range cases { + rec := doRequest(t, h, "GET", path) + if rec.Code != http.StatusBadRequest { + t.Errorf("path %q: status = %d, want 400", path, rec.Code) + } + } +} + +func TestDeleteRejectsInvalidOlderThanHours(t *testing.T) { + s := &fakeStore{} + h := newTestHandler(s, authz.RoleAdmin) + + rec := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours=0") + if rec.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want 400", rec.Code) + } + if len(s.deletedWith) != 0 { + t.Error("an invalid older_than_hours must never reach the store's delete path") + } +} + +func TestDeletePropagatesStoreErrors(t *testing.T) { + s := &fakeStore{deleteErr: errors.New("clickhouse mutation failed")} + h := newTestHandler(s, authz.RoleAdmin) + + rec := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours=24") + if rec.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want 500", rec.Code) + } +} + +func TestOwnerAndAdminCanUseRetentionRoutes(t *testing.T) { + for _, role := range []authz.Role{authz.RoleAdmin, authz.RoleOwner} { + s := &fakeStore{count: 3} + h := newTestHandler(s, role) + + preview := doRequest(t, h, "GET", "/logs/retention/preview?older_than_hours=24") + if preview.Code != http.StatusOK { + t.Errorf("role %s: preview status = %d, want 200", role, preview.Code) + } + del := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours=24") + if del.Code != http.StatusOK { + t.Errorf("role %s: delete status = %d, want 200", role, del.Code) + } + } +} + +func TestViewerAndEditorAreForbiddenFromRetentionRoutes(t *testing.T) { + for _, role := range []authz.Role{authz.RoleViewer, authz.RoleEditor} { + s := &fakeStore{count: 3} + h := newTestHandler(s, role) + + preview := doRequest(t, h, "GET", "/logs/retention/preview?older_than_hours=24") + if preview.Code != http.StatusForbidden { + t.Errorf("role %s: preview status = %d, want 403", role, preview.Code) + } + del := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours=24") + if del.Code != http.StatusForbidden { + t.Errorf("role %s: delete status = %d, want 403", role, del.Code) + } + if len(s.deletedWith) != 0 { + t.Errorf("role %s: must never reach the store", role) + } + } +} + +func TestRetentionRoutesRequireAuth(t *testing.T) { + s := &fakeStore{} + h := NewHandler(discardLogger(), s, nil) + + // A nil authorizer is Phase 0-3's default-open behavior (see + // authz.RequireRole's doc comment) -- confirm that posture applies + // here too, same as every other RequireRole-wrapped route, rather + // than this package accidentally being open or closed by default in + // a way inconsistent with the rest of the API. + rec := doRequest(t, h, "DELETE", "/logs/retention?older_than_hours=24") + if rec.Code != http.StatusOK { + t.Fatalf("status with nil authorizer = %d, want 200 (default-open, matches RequireRole elsewhere)", rec.Code) + } +} diff --git a/api/logretention/store.go b/api/logretention/store.go new file mode 100644 index 0000000..bbf7ee1 --- /dev/null +++ b/api/logretention/store.go @@ -0,0 +1,70 @@ +// Package logretention lets an owner or admin permanently delete log +// records older than a chosen age -- storage/README.md has flagged "no +// TTL/retention clause yet" since Phase 0; this is the on-demand, +// operator-triggered half of that gap (not an automatic TTL, which is +// a different, engine-driven design nobody asked for here). +// +// Deliberately scoped to core's single ClickHouse `logs` table, not +// enterprise/'s per-tenant ClickHouse routing +// (enterprise/internal/chrunner.Registry) -- core has no tenant_id +// column on `logs` at all (tenant isolation there lives at the +// connection layer per /docs/phase-4-isolation-design.md), so there is +// nothing to scope a single-tenant deletion by. A tenant-aware +// equivalent for enterprise/ is real, disclosed future work, not +// silently assumed to already work there. +// +// Also disclosed, not silently ignored: deleting from ClickHouse does +// not prune the Tantivy full-text index (search/) -- that index has no +// timestamp field and no bulk/range-delete primitive today (only a +// per-record upsert), so a deleted record's record_id can keep +// resolving to nothing via free-text search until search/ grows a real +// deletion path. Closing that gap is a separate, larger piece of work +// spanning proto/search.proto, search/src/grpc.rs, and +// api/searchclient -- out of scope for this feature. +package logretention + +import ( + "context" + "time" + + "github.com/ClickHouse/clickhouse-go/v2/lib/driver" +) + +// Store issues purpose-built, parameterized statements against the +// `logs` table -- deliberately not querylang/executor.ChRunner, whose +// one method (RunSQL) is scoped to arbitrary SELECT statements for the +// query language compiler. This package only ever needs two fixed +// statements (a count and a delete), so keeping them separate avoids +// stretching ChRunner's SELECT-shaped contract to also cover a DML +// mutation. +type Store struct { + conn driver.Conn +} + +func NewStore(conn driver.Conn) *Store { + return &Store{conn: conn} +} + +// CountOlderThan reports how many log records are older than cutoff -- +// backs the "this will delete N records" preview a caller shows before +// asking for confirmation. +func (s *Store) CountOlderThan(ctx context.Context, cutoff time.Time) (uint64, error) { + row := s.conn.QueryRow(ctx, "SELECT count() FROM logs WHERE timestamp < ?", cutoff) + var n uint64 + if err := row.Scan(&n); err != nil { + return 0, err + } + return n, nil +} + +// DeleteOlderThan issues a synchronous ClickHouse mutation +// (SETTINGS mutations_sync = 1) deleting every log record older than +// cutoff. Synchronous rather than fire-and-forget so a 200 response +// means the data is actually gone, not just queued -- an owner/admin +// confirming a permanent delete should be able to trust the response. +// This does block for as long as the mutation takes, which could be a +// while against a very large table; a disclosed tradeoff for this +// deployment's homelab/small-scale target, not a hidden one. +func (s *Store) DeleteOlderThan(ctx context.Context, cutoff time.Time) error { + return s.conn.Exec(ctx, "ALTER TABLE logs DELETE WHERE timestamp < ? SETTINGS mutations_sync = 1", cutoff) +} diff --git a/web/src/lib/api.ts b/web/src/lib/api.ts index 498bda7..b60b6d5 100644 --- a/web/src/lib/api.ts +++ b/web/src/lib/api.ts @@ -482,6 +482,22 @@ export function setUserRole(id: string, role: string): Promise { }); } +// --- log retention (owner/admin only, see api/logretention) ----------- + +export type LogRetentionPreview = { count: number; cutoff: string }; +export type LogRetentionDeleteResult = { deleted_count: number; cutoff: string }; + +export function previewLogDeletion(olderThanHours: number): Promise { + return request(`/logs/retention/preview?older_than_hours=${olderThanHours}`, { credentials: 'include' }); +} + +export function deleteLogsOlderThan(olderThanHours: number): Promise { + return request(`/logs/retention?older_than_hours=${olderThanHours}`, { + method: 'DELETE', + credentials: 'include' + }); +} + // --- alerting --------------------------------------------------------- export type ConditionType = 'threshold' | 'absence'; diff --git a/web/src/routes/settings/+page.svelte b/web/src/routes/settings/+page.svelte index 1380d17..9eb61af 100644 --- a/web/src/routes/settings/+page.svelte +++ b/web/src/routes/settings/+page.svelte @@ -1,5 +1,18 @@