Skip to content

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

Merged
lakhansamani merged 1 commit into
mainfrom
refactor/audit-log-index-strategy
Oct 14, 2026
Merged

lakhansamani merged 1 commit into
mainfrom
refactor/audit-log-index-strategy

Conversation

@lakhansamani

Copy link
Copy Markdown
Contributor

Refs #805. Takes the two parts of that issue that can be settled with evidence, and leaves the part that needs measurement open.

1. The scan-hint decision is now driven by an explicit indexed-column set

The ALLOW FILTERING decision was positional: len(clauses) > 1, plus a flag the timestamp bounds set. It knew nothing about which columns actually carry an index.

That is the coupling #805 identifies, and it is sharper than a perf nit: a hint-less equality against a column with no index is rejected by Cassandra, not merely slow. So dropping any index would silently turn a working filter into an error. Nothing could be removed until this was fixed.

buildAuditLogWhere now derives the hint from auditLogIndexedColumns. Three things force it:

  1. an equality on a column not in the set;
  2. any 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 the set;
  3. more than one restriction — one query, one index.

Adding or dropping an index is now a one-line change to the set.

The helper also returns the hint as a flag rather than baked into the clause, which deletes the strings.TrimSuffix(whereClause, " ALLOW FILTERING") dance the paged query needed (CQL grammar puts LIMIT before ALLOW FILTERING). Net simplification.

2. The Cassandra created_at index is dropped

It could never be used. A Cassandra secondary index serves equality only, and every query this table issues against created_at is a range:

  • ListAuditLogs — created_at>=? / created_at<=?, already on the scan path
  • DeleteAuditLogsBefore — created_at < ? ALLOW FILTERING (audit_log.go)

So it was pure write amplification on the highest-write table in the schema — a write per audit row, for an index no query could read.

This is non-destructive. Removing the CREATE INDEX IF NOT EXISTS stops new deployments from creating it; existing deployments keep theirs until dropped by hand. The CQL is in a comment at the removal site:

DROP INDEX IF EXISTS authorizer.authorizer_audit_log_created_at;

Automatic DROP INDEX on every boot would be destructive DDL run unattended, so it is deliberately not done.

Couchbase keeps its created_at GSI

#805 says "whatever is decided should be applied to both backends together". That is wrong for this column, and the PR deliberately diverges:

  • N1QL serves range predicates from a GSI, unlike a Cassandra 2i.
  • couchbase/audit_log.go puts ORDER BY created_at DESC on every ListAuditLogs query, which the index serves.
  • DELETE ... WHERE created_at < $before is also index-served there.

Dropping it on Couchbase would regress exactly the query path it exists for. The AGENTS.md parity rule is about comparable query cost across backends, not identical index lists — and here identical lists would produce divergent cost.

3. Left open in #805

The action (~60 distinct values) and resource_type (~17) low-cardinality MV hotspots. Both predate #802's shape, and the right input is MV partition sizes measured on a real cluster, not a guess. With change (1) in place, acting on them later is a one-line edit to the set plus the CREATE INDEX removal.

Testing

  • TestBuildAuditLogWhereAllowFilteringDecision — table test over the hint rule: single indexed equality stays index-served; two equalities take the hint; a created_at range always takes it; all six filters compose; empty-string filters are ignored rather than bound. Mutation-checked: reverting the range rule makes the created_at case fail.
  • TestAuditLogIndexedColumnsMatchCreatedIndexes — reads provider.go and asserts the set matches the CREATE INDEX statements in both directions, and that created_at has not come back. Mutation-checked: re-adding created_at to the set fails it. This is the guard that keeps the Review audit_log secondary-index strategy on Cassandra/Scylla (low-cardinality hotspots) #805 coupling from re-forming.
  • make test-scylladb against a real ScyllaDB container, three runs. Audit-log subtests 9/9 in every run.
  • go build ./..., go vet ./..., go test ./internal/storage/db/cassandradb/ clean.

One flake seen, unrelated to this change

Run 1 of 3 had two failures — Org_Domain_Operations and User_SCIM_Fields — both green in runs 2 and 3, and both green on main and on #810's branch in earlier runs. They are assertion failures about a write not being visible on an immediate read (ListOrgDomainsByOrg returning 2 rows after a delete; IsActive=false not persisting), not driver errors, and they sit in OrgDomain / UpdateUser paths this diff does not touch — it changes audit-log query construction, removes one audit-log DDL statement, and adds a test file. Reported separately rather than hidden here; OrgDomain.org_id is a Scylla materialized view, so MV write-propagation lag is the likely cause.

Merge order

Needs a rebase after #811 (golang.org/x/net bump) lands — like #810, this branch inherits main's failing govulncheck, which is unrelated to its own diff.

The ALLOW FILTERING decision was positional — `len(clauses) > 1` plus
a flag set by the timestamp bounds. It knew nothing about which
columns actually have an index, so dropping one would emit a
hint-less equality against an unindexed column, which Cassandra
REJECTS rather than merely running slowly. That coupling is what
blocked any index change.

- add auditLogIndexedColumns, and derive the hint from it: an
  equality on an unindexed column, any range restriction, or more
  than one clause. Adding or dropping an index is now a one-line
  change to the set.
- extract buildAuditLogWhere, which returns the hint as a flag
  rather than baked into the clause. This deletes the
  TrimSuffix(" ALLOW FILTERING") dance the paged query needed,
  since CQL grammar puts LIMIT before the hint.
- drop the created_at index. A Cassandra secondary index serves
  equality only, and every query this table issues against
  created_at is a range — the ListAuditLogs bounds and
  DeleteAuditLogsBefore, both already on the scan path. The index
  could never be used while costing a write on every row of the
  highest-write table in the schema.

Couchbase keeps its created_at GSI: N1QL serves range predicates
from a GSI, and every ListAuditLogs query there carries
ORDER BY created_at DESC. Identical index lists are not the parity
rule; comparable query cost is, and dropping it there would regress.

The action and resource_type hotspots stay open in #805 — those need
MV partition sizes measured on a real cluster, not a guess.

Refs #805
@lakhansamani
lakhansamani force-pushed the refactor/audit-log-index-strategy branch from c72ab3f to 49c5188 Compare October 14, 2026 09:07
@lakhansamani
lakhansamani merged commit 3aaa1f8 into main Oct 14, 2026
4 checks passed
lakhansamani added a commit that referenced this pull request Oct 14, 2026
Sixteen PRs merged since the 2.4.1 tag; [Unreleased] documented three
of them and none of the five advisory fixes. A security release whose
notes omit its security fixes gives nobody a reason to upgrade, which
is the operator-facing half of the failure SECURITY.md triage exists
to prevent.

Renames [Unreleased] to 2.4.2 and documents all of it:

- Security: the six advisories closed (#814, #799, #798, #796 x2,
  #815) and the three reachable dependency advisories (#797, #807,
  #811), plus the Go 1.27 builder (#800).
- Fixed: the audit-filter parity bugs (#802) and the Cassandra index
  probe that never waited (#810).
- Changed: direct-gRPC x-authorizer-url now yields to --url (#814),
  and the dropped Cassandra audit_log(created_at) index (#812).
- Added: the audit plumbing (#806), stated as writing no new records
  and changing no behaviour, plus two deployment entries.
- Upgrade notes: eight items, including the three that were missing
  entirely — that ANY two audit filters now scan on Cassandra (not
  only the timestamp range, which the previous note implied), the
  longer first start on ScyllaDB now that probes genuinely block, and
  the manual DROP INDEX for the removed index.

Section order leads with Security, matching 2.4.1 — the file orders by
what matters for the release at hand rather than a fixed sequence.

Deliberately omitted: #791 (scorecard action bump) and #809 (the
previous changelog edit). Neither is user-facing; Keep a Changelog
asks for notable changes, not a commit log.

The ScyllaDB view-build note is scoped to ScyllaDB: global secondary
indexes are materialized views there, while Apache Cassandra builds
local 2i with no view-build step and no nodetool viewbuildstatus.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant