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.
2026-08-21 14:49:18 -07:00
parent 864e68253a
commit 787def06fd
6 changed files with 676 additions and 1 deletions
@@ -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})
+}
@@ -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)
+	}
+}
@@ -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)
+}