diff --git a/internal/storage/db/cassandradb/audit_log.go b/internal/storage/db/cassandradb/audit_log.go index 531c7e64..cd57fbfd 100644 --- a/internal/storage/db/cassandradb/audit_log.go +++ b/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" } diff --git a/internal/storage/db/cassandradb/audit_log_where_test.go b/internal/storage/db/cassandradb/audit_log_where_test.go new file mode 100644 index 00000000..a618e650 --- /dev/null +++ b/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") + } +} diff --git a/internal/storage/db/cassandradb/provider.go b/internal/storage/db/cassandradb/provider.go index af9ad6a8..17e2b4ab 100644 --- a/internal/storage/db/cassandradb/provider.go +++ b/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 {