refactor(storage): drive audit-log scan hint from the indexed-column set

internal/storage/db/cassandradb/audit_log.go

@@ -37,38 +37,63 @@ func (p *provider) AddAuditLog(ctx context.Context, auditLog *schemas.AuditLog)
 }
 
 // ListAuditLogs queries audit logs with filters and pagination
-func (p *provider) ListAuditLogs(ctx context.Context, pagination *model.Pagination, filter map[string]interface{}) ([]*schemas.AuditLog, *model.Pagination, error) {
-	auditLogs := []*schemas.AuditLog{}
-	paginationClone := *pagination
-
-	// Build query with filters
-	queryBase := fmt.Sprintf("SELECT id, actor_id, actor_type, actor_email, action, resource_type, resource_id, ip_address, user_agent, metadata, created_at FROM %s", KeySpace+"."+schemas.Collections.AuditLog)
-	countBase := fmt.Sprintf("SELECT COUNT(*) FROM %s", KeySpace+"."+schemas.Collections.AuditLog)
+// auditLogIndexedColumns is the set of audit_log columns backed by a secondary
+// index in NewProvider. The ALLOW FILTERING decision in buildAuditLogWhere is
+// derived from this set rather than from the clause count alone, so adding or
+// dropping an index is a one-line change here and cannot leave a hint-less
+// equality against an unindexed column — which Cassandra rejects outright
+// rather than merely running slowly.
+//
+// created_at is deliberately absent. Its index was dropped: a Cassandra
+// secondary index serves equality only, and the only queries this table ever
+// issues against created_at are the range bounds below and the range in
+// DeleteAuditLogsBefore, so the index could never be used while still costing a
+// write on every audit row — and audit_log is the highest-write table here.
+var auditLogIndexedColumns = map[string]bool{
+	"actor_id":      true,
+	"action":        true,
+	"resource_type": true,
+	"resource_id":   true,
+}
 
-	// Every filter column below is backed by a secondary index (see
-	// provider.go), so equality restrictions need no ALLOW FILTERING — Scylla
-	// builds those indexes as materialized views and serves them directly.
-	//
-	// The created_at BOUNDS are different: a range on a non-primary-key column
-	// cannot be served by an index, so it forces ALLOW FILTERING and a scan.
-	// Only the timestamp bounds set that flag; an indexed-equality-only query
-	// keeps its existing index-served plan.
-	//
-	// ponytail: ALLOW FILTERING covers TWO cases, not just the rare one. The
-	// timestamp range is genuinely rare. Two or more equality filters is NOT —
-	// "this actor, this action" is an ordinary admin search, and it now scans
-	// where it previously errored outright. That is still the right trade here
-	// (admin-only endpoint, paginated, and an erroring filter combination is
-	// worse than a slow one), but it is a scan, not a free lunch. The upgrade
-	// path when it stops being cheap is a composite materialized view over the
-	// filter combinations that actually get used — not a bigger scan.
+// buildAuditLogWhere builds the WHERE clause and bind values for ListAuditLogs,
+// and reports whether the query needs ALLOW FILTERING.
+//
+// The hint is returned separately rather than appended to the clause because CQL
+// grammar puts LIMIT before ALLOW FILTERING, so the paged query cannot simply
+// carry the clause through.
+//
+// Three things force the hint, and the set above is what decides the first two:
+//
+//  1. An equality on a column with no secondary index.
+//  2. A range restriction. A range on a non-primary-key column cannot be served
+//     by a secondary index even when the column has one, so this holds
+//     regardless of auditLogIndexedColumns.
+//  3. More than one restriction. A single query can only be served by one
+//     index, so Cassandra/Scylla requires the hint for any second clause.
+//
+// ponytail: (3) is the common case, not the rare one. "this actor, this action"
+// is an ordinary admin search and it scans. That is the right trade for an
+// admin-only paginated endpoint — an erroring filter combination is worse than a
+// slow one, and before #802 that combination errored outright on Scylla. The
+// upgrade path when it stops being cheap is a composite materialized view over
+// the filter combinations that actually get used, not a bigger scan.
+func buildAuditLogWhere(filter map[string]interface{}) (string, []interface{}, bool) {
 	clauses := []string{}
-	filterValues := []interface{}{}
+	values := []interface{}{}
 	needsAllowFiltering := false
 
 	addEq := func(col string, v interface{}) {
 		clauses = append(clauses, col+"=?")
-		filterValues = append(filterValues, v)
+		values = append(values, v)
+		if !auditLogIndexedColumns[col] {
+			needsAllowFiltering = true
+		}
+	}
+	addRange := func(expr string, v interface{}) {
+		clauses = append(clauses, expr)
+		values = append(values, v)
+		needsAllowFiltering = true
 	}
 
 	if action, ok := filter["action"]; ok && action != "" {
@@ -84,40 +109,44 @@ func (p *provider) ListAuditLogs(ctx context.Context, pagination *model.Paginati
 		addEq("resource_id", resourceID)
 	}
 	if fromTimestamp, ok := filter["from_timestamp"]; ok {
-		clauses = append(clauses, "created_at>=?")
-		filterValues = append(filterValues, fromTimestamp)
-		needsAllowFiltering = true
+		addRange("created_at>=?", fromTimestamp)
 	}
 	if toTimestamp, ok := filter["to_timestamp"]; ok {
-		clauses = append(clauses, "created_at<=?")
-		filterValues = append(filterValues, toTimestamp)
-		needsAllowFiltering = true
+		addRange("created_at<=?", toTimestamp)
 	}
 
-	whereClause := ""
-	if len(clauses) > 0 {
-		whereClause = " WHERE " + strings.Join(clauses, " AND ")
+	if len(clauses) == 0 {
+		return "", values, false
 	}
-	// More than one restriction on non-primary-key columns cannot be served by a
-	// single index either, so Cassandra/Scylla requires the scan hint there too.
 	if len(clauses) > 1 {
 		needsAllowFiltering = true
 	}
-	if needsAllowFiltering {
-		whereClause += " ALLOW FILTERING"
-	}
+	return " WHERE " + strings.Join(clauses, " AND "), values, needsAllowFiltering
+}
+
+func (p *provider) ListAuditLogs(ctx context.Context, pagination *model.Pagination, filter map[string]interface{}) ([]*schemas.AuditLog, *model.Pagination, error) {
+	auditLogs := []*schemas.AuditLog{}
+	paginationClone := *pagination
+
+	// Build query with filters
+	queryBase := fmt.Sprintf("SELECT id, actor_id, actor_type, actor_email, action, resource_type, resource_id, ip_address, user_agent, metadata, created_at FROM %s", KeySpace+"."+schemas.Collections.AuditLog)
+	countBase := fmt.Sprintf("SELECT COUNT(*) FROM %s", KeySpace+"."+schemas.Collections.AuditLog)
+
+	whereClause, filterValues, needsAllowFiltering := buildAuditLogWhere(filter)
 
 	countQuery := countBase + whereClause
+	if needsAllowFiltering {
+		countQuery += " ALLOW FILTERING"
+	}
 	err := p.db.Query(countQuery, filterValues...).Consistency(gocql.One).Scan(&paginationClone.Total)
 	if err != nil {
 		return nil, nil, err
 	}
 
-	// Fetch with pagination
-	// CQL grammar: LIMIT precedes ALLOW FILTERING, so the hint cannot simply be
-	// carried along on whereClause here.
-	query := queryBase + strings.TrimSuffix(whereClause, " ALLOW FILTERING") +
-		fmt.Sprintf(" LIMIT %d", pagination.Limit+pagination.Offset)
+	// Fetch with pagination. CQL grammar puts LIMIT before ALLOW FILTERING, which
+	// is why buildAuditLogWhere returns the hint as a flag rather than baked into
+	// the clause.
+	query := queryBase + whereClause + fmt.Sprintf(" LIMIT %d", pagination.Limit+pagination.Offset)
 	if needsAllowFiltering {
 		query += " ALLOW FILTERING"
 	}

internal/storage/db/cassandradb/audit_log_where_test.go

@@ -0,0 +1,122 @@
+package cassandradb
+
+import (
+	"os"
+	"regexp"
+	"strings"
+	"testing"
+)
+
+// TestBuildAuditLogWhereAllowFilteringDecision pins the rule that the scan hint
+// is derived from auditLogIndexedColumns, not from the clause count alone.
+//
+// Getting this wrong is not a performance nit in either direction: a hint-less
+// equality against a column with no index is REJECTED by Cassandra, so an index
+// dropped without updating the set turns a working filter into an error. That
+// coupling is the whole reason the set exists (see #805).
+func TestBuildAuditLogWhereAllowFilteringDecision(t *testing.T) {
+	tests := []struct {
+		name      string
+		filter    map[string]interface{}
+		wantWhere string
+		wantHint  bool
+		wantVals  int
+	}{
+		{
+			name:      "no filters needs no clause and no hint",
+			filter:    map[string]interface{}{},
+			wantWhere: "",
+			wantHint:  false,
+		},
+		{
+			name:      "single indexed equality stays index-served",
+			filter:    map[string]interface{}{"action": "user.login_success"},
+			wantWhere: " WHERE action=?",
+			wantHint:  false,
+			wantVals:  1,
+		},
+		{
+			name:      "two indexed equalities need the hint: one query, one index",
+			filter:    map[string]interface{}{"action": "user.login_success", "actor_id": "u1"},
+			wantWhere: " WHERE action=? AND actor_id=?",
+			wantHint:  true,
+			wantVals:  2,
+		},
+		{
+			name:      "a range on created_at is never index-served",
+			filter:    map[string]interface{}{"from_timestamp": int64(1)},
+			wantWhere: " WHERE created_at>=?",
+			wantHint:  true,
+			wantVals:  1,
+		},
+		{
+			name:      "all six filters apply together",
+			filter:    map[string]interface{}{"action": "a", "actor_id": "b", "resource_type": "c", "resource_id": "d", "from_timestamp": int64(1), "to_timestamp": int64(2)},
+			wantWhere: " WHERE action=? AND actor_id=? AND resource_type=? AND resource_id=? AND created_at>=? AND created_at<=?",
+			wantHint:  true,
+			wantVals:  6,
+		},
+		{
+			name:      "empty string filters are ignored, not bound as empty",
+			filter:    map[string]interface{}{"action": "", "actor_id": "u1"},
+			wantWhere: " WHERE actor_id=?",
+			wantHint:  false,
+			wantVals:  1,
+		},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			where, vals, hint := buildAuditLogWhere(tc.filter)
+			if where != tc.wantWhere {
+				t.Errorf("where\n got: %q\nwant: %q", where, tc.wantWhere)
+			}
+			if hint != tc.wantHint {
+				t.Errorf("allowFiltering = %v, want %v (for %q)", hint, tc.wantHint, where)
+			}
+			if len(vals) != tc.wantVals {
+				t.Errorf("bound %d values, want %d", len(vals), tc.wantVals)
+			}
+			// The hint is returned as a flag, never baked into the clause: CQL
+			// grammar puts LIMIT before ALLOW FILTERING.
+			if strings.Contains(where, "ALLOW FILTERING") {
+				t.Errorf("where clause must not carry the hint inline: %q", where)
+			}
+		})
+	}
+}
+
+// TestAuditLogIndexedColumnsMatchCreatedIndexes keeps the set in step with the
+// indexes NewProvider actually creates, by reading the provider source.
+//
+// This is the coupling #805 names: a column listed in the set with no index
+// behind it produces a hint-less equality that Cassandra REJECTS, so dropping
+// an index without updating the set turns a working filter into an error. A
+// static check catches that without needing a live cluster.
+func TestAuditLogIndexedColumnsMatchCreatedIndexes(t *testing.T) {
+	src, err := os.ReadFile("provider.go")
+	if err != nil {
+		t.Fatalf("read provider.go: %v", err)
+	}
+	created := map[string]bool{}
+	re := regexp.MustCompile(`CREATE INDEX IF NOT EXISTS authorizer_audit_log_\w+ ON %s\.%s \((\w+)\)`)
+	for _, m := range re.FindAllStringSubmatch(string(src), -1) {
+		created[m[1]] = true
+	}
+	if len(created) == 0 {
+		t.Fatal("found no audit_log CREATE INDEX statements — did the statement format change?")
+	}
+	for col := range auditLogIndexedColumns {
+		if !created[col] {
+			t.Errorf("auditLogIndexedColumns lists %q but NewProvider creates no index for it: "+
+				"a hint-less equality on it would be rejected by Cassandra", col)
+		}
+	}
+	for col := range created {
+		if !auditLogIndexedColumns[col] {
+			t.Errorf("NewProvider indexes %q but the set omits it: queries on it take a needless scan", col)
+		}
+	}
+	if created["created_at"] {
+		t.Error("created_at index is back: a 2i serves equality only, and every query on this column is a range")
+	}
+}

internal/storage/db/cassandradb/provider.go

@@ -415,11 +415,15 @@ func NewProvider(cfg *config.Config, deps *Dependencies) (*provider, error) {
 	if err != nil {
 		return nil, err
 	}
-	auditLogTimestampIndex := fmt.Sprintf("CREATE INDEX IF NOT EXISTS authorizer_audit_log_created_at ON %s.%s (created_at)", KeySpace, schemas.Collections.AuditLog)
-	err = session.Query(auditLogTimestampIndex).Exec()
-	if err != nil {
-		return nil, err
-	}
+	// No index on created_at, deliberately. A Cassandra secondary index serves
+	// equality only, and every query this table issues against created_at is a
+	// range — the bounds in ListAuditLogs and DeleteAuditLogsBefore, both of
+	// which take the ALLOW FILTERING scan path. The index could therefore never
+	// be used, while still costing a write on every row of the highest-write
+	// table in the schema. See buildAuditLogWhere in audit_log.go.
+	//
+	// Pre-existing deployments keep the index until it is dropped by hand:
+	//   DROP INDEX IF EXISTS authorizer.authorizer_audit_log_created_at;
 	auditLogResourceTypeIndex := fmt.Sprintf("CREATE INDEX IF NOT EXISTS authorizer_audit_log_resource_type ON %s.%s (resource_type)", KeySpace, schemas.Collections.AuditLog)
 	err = session.Query(auditLogResourceTypeIndex).Exec()
 	if err != nil {