Skip to content

Add Trino cell observability to the admin console - #1126

Merged
fuziontech merged 4 commits into
mainfrom
trino-admin-observability
Aug 27, 2026
Merged

Add Trino cell observability to the admin console#1126
fuziontech merged 4 commits into
mainfrom
trino-admin-observability

Conversation

@fuziontech

Copy link
Copy Markdown
Member

Trino is becoming the primary query execution engine for duckgres, and the admin console had no Trino surface at all — everything Trino lived in provisioner/ and provisioner/opa/. A failed Trino provision was silent unless someone read duckgres_managed_warehouse_trino by hand, and a runaway tenant query could only be killed by someone with cluster access.

This adds the console's Trino views, the identity they need, and fixes an OPA gap found on the way.

What you get

Trino queries — live queries across the cell, longest-running first, each stamped with the duckgres org resolved from the Trino principal. Flags blocked queries (every driver waiting on the metadata store or S3) separately from merely slow ones. Admin-only, audited kill.

Trino cell — coordinator version / environment / uptime / starting, the failure detector's view of the fleet, and every tenant's provisioning state with the reconcile loop's own status message.

Org detail card — that org's state, status_message, ready_at, failed_at, principal, catalog, tier, cell, and live query counts.

The security decision to review

The console reads the coordinator as a new __duckgres_observer principal, deliberately not as __admin_provisioner. Trino gates all of this through the access-control SPI (/v1/query filters through FilterViewQueryOwnedBy, /v1/query/{id} on ViewQueryOwnedBy, kill on KillQueryOwnedBy, /v1/node + /v1/resourceGroupState are MANAGEMENT_READ), so a console credential with no grant sees an empty cluster.

The split is the bargain:

catalog authority query visibility
__admin_provisioner CREATE/DROP CATALOG own queries only
__duckgres_observer none cluster-wide (+ kill)

The observer holds no entry in data.group_catalogs, and the observer group is excluded from tenant_owns_catalog and from the same-org query match, so even a mistaken bundle entry grants no data access. is_observer is the same username-AND-group conjunction as is_admin. One leaked credential yields one half of that authority, never both — which is why this is a second principal rather than a widening of the first.

policy.rego is flagged as security review in CLAUDE.md, so that's the part to read closely.

Bug found along the way

The policy had no batch rule for FilterViewQueryOwnedBy. opa.policy.batched-uri is enabled (without it, filtering a catalog with >1024 tables overruns the OPA client's queue), and OpaBatchAccessControl sends query-owner candidates through it exactly like the catalog filters. With no batch rule the batched answer was the empty set, so the same-org query visibility the policy documents has been inert in production. It didn't look like a failure: Trino short-circuits self-ownership before OPA, so every tenant still saw its own query and the gap read as "org-mates' queries are missing".

allow and batch now share one predicate parameterized on the owner, and TestBatchedQueryFilteringMatchesNonBatched pins them candidate by candidate.

Implementation notes worth a look

  • Redaction at decode. SQL is redacted in toTrinoQuery, not per-caller, so no handler can leak it by forgetting. TrinoEnabledOrg.RootPasswordHash never crosses the projection boundary (there's a test that greps every payload for $2a$10$).
  • Airlift units are strings. Duration serializes as "%.2f<unit>", DataSize as an exact byte count with a B suffix. Decoding either as a JSON number gives a plausible page of zeroes. Both parsers return 0 on anything unreadable, so a unit-spelling change on a coordinator upgrade costs one column rather than the whole view.
  • Cached, and stale-over-blocking. The console polls from every open tab and /v1/query walks every query the coordinator holds. Readers during an in-flight refresh get the previous value, so a slow coordinator can't turn N tabs into N stuck requests during the incident the console exists for.
  • Degrades, doesn't error. An unreachable coordinator gives available:false + a reason, with provisioning state still served from the config store — "the cell is down" and "these tenants never provisioned" are different incidents. A 403 is reported distinctly, because that means the bundle hasn't rolled out rather than the cell being down.
  • Both sides tag X-Trino-Source (duckgres-provisioner, duckgres-admin), so control-plane traffic is separable from tenant SQL in system.runtime.queries.
  • No cell configured (DUCKGRES_TRINO_COORDINATOR_URL unset) leaves every route unregistered.

Known limits

  • /v1/node reports heartbeat health keyed by URI and carries no node id or version, so worker version skew is not visible here — the Nodes page's pod-image projection is where that lives. Called out in the UI.
  • No tests/mw-dev/e2e/harness.sh coverage, for the same reason the rest of the Trino code has none: mw-dev runs no Trino cell. The harness assertion lands with the chart that deploys one.

Deliberately not in this PR

Durable query history via the trino-http-event-listener plugin (unlocks a Trino errors page and Trino CPU/bytes attribution in billing, which is currently invisible), /metrics scraping into the metrics-proxy allow-list, and DuckLake data-layer health (small-file ratio, delete debt). All three need chart changes in PostHog/charts, so they're follow-ups.

Testing

  • golangci-lint run — 0 issues (also 0 in the new files under --build-tags kubernetes; the 62 that tag surfaces are all pre-existing).
  • Go: controlplane/... green. The 21 *Postgres failures on this machine are pre-existing — identical count on an untouched checkout, a stale local test DB missing max_hot_idle_workers.
  • UI: 160/160 vitest, tsc -b --noEmit clean, eslint clean on new files.
  • New coverage: OPA observer + batched-filtering matrix, credential mint/adopt/regenerate/fail-loud, auth-file projection, bundle-grants-no-catalog, coordinator client (airlift parsing, redaction, 403/410 handling), handler behaviour (org annotation, degradation, kill gating + audit, caching), and the UI derivations.

fuziontech and others added 3 commits August 26, 2026 22:52
…d query filtering

Trino routes operator-console reads through the same access-control SPI as
everything else: GET /v1/query filters its result through
FilterViewQueryOwnedBy, /v1/query/{id} is gated on ViewQueryOwnedBy, kill on
KillQueryOwnedBy, and /v1/node + /v1/resourceGroupState are MANAGEMENT_READ,
i.e. checkCanReadSystemInformation. A console credential with no grant sees
an empty cluster, so the capability has to exist in policy.

It is a SEPARATE principal (__duckgres_observer) rather than a widening of
__admin_provisioner, and the split is the point. The admin credential can
CREATE and DROP catalogs but by policy sees only its own queries; the
observer sees every tenant's query metadata and holds no catalog at all --
no entry in data.group_catalogs, and the observer group is excluded from
tenant_owns_catalog and from the same-org query match, so even a mistaken
bundle entry grants no data access. One leaked credential yields one half of
that authority, never both. is_observer is the same username-AND-group
conjunction as is_admin, so a projection regression that drops a tenant into
the observer group grants nothing on its own.

Also adds the missing `batch` rule for FilterViewQueryOwnedBy. opa.policy
.batched-uri is enabled -- without it, filtering a catalog with more than
~1024 tables overruns the OPA client's queue -- and OpaBatchAccessControl
sends query-owner candidates through it exactly like the catalog filters.
With no batch rule the batched answer was the empty set, which made the
same-org visibility this policy documents inert in production: Trino
short-circuits self-ownership before OPA, so each tenant still saw its own
query and the gap read as "org-mates' queries are missing" rather than as a
failure. The two entrypoints now share one predicate, parameterized on the
owner, so they cannot drift; TestBatchedQueryFilteringMatchesNonBatched pins
them candidate by candidate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The observer principal needs a password in the cell's password.db and a
group in group.db, or the console authenticates fine and is then denied
everything by the username-AND-group conjunction in the policy.

ensureAdminCredential's careful create-once-then-adopt algorithm is now
parameterized over which key pair on trino-auth it establishes, and both the
admin and the observer pair go through it. Same regenerate-if-missing
semantics, and for the same reason: the provisioner owns both sides and
nothing external consumes either value, so a lost pair self-heals within one
password-file refresh instead of wedging the cell. That is the deliberate
exception to the write-once family -- the internal-communication secret is
env-projected into long-lived pods and must still fail loud.

BuildTrinoAuthFiles now takes a TrinoClusterPrincipals struct rather than a
second bare string. The two hashes are the same type and adjacent, and
swapping them would hand the console's identity catalog-management authority
and the provisioner's identity none -- a mistake the compiler could not
otherwise catch. Neither principal joins a tier group: tier membership routes
a query to a tenant's resource group, and neither of these submits tenant
SQL.

Also stamps X-Trino-Source on the reconcile loop's own statements. Trino
records the header verbatim as system.runtime.queries.source, so tagging it
is what lets an operator tell control-plane DDL apart from tenant SQL --
including filtering it out of the console's live-query view. Untagged, every
tick's SHOW CATALOGS reads as an unattributed query from a privileged user.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Trino is becoming the primary query execution engine, and the console had no
Trino surface at all: everything Trino lived in the provisioner and the OPA
package, so a failed Trino provision was silent and a runaway tenant query
could only be killed by someone with cluster access.

Adds three views, all read as the observer principal over the coordinator's
REST API:

- Trino queries: live queries across the cell, longest-running first,
  annotated with the duckgres org resolved from the Trino principal (Trino
  only knows the principal, and the mapping lives in the config store).
  Admin-only, audited kill that delivers the operator's reason to the TENANT
  as the query's failure message, so they learn why it died rather than
  seeing an unexplained cancellation.
- Trino cell: coordinator version/uptime, the failure detector's view of the
  fleet, and every tenant's provisioning state.
- An org detail card surfacing that org's row on
  duckgres_managed_warehouse_trino -- state, status_message, ready_at,
  failed_at. Those columns have always existed and were rendered nowhere.

Three things the implementation is careful about:

SQL text is redacted at the single decode point, not per caller, so no
handler can leak it by forgetting. Query text is tenant data: table names,
filter literals, customer identifiers, and a failed CREATE SECRET carries a
credential.

Airlift value types are STRINGS on the wire. Duration serializes as
"%.2f<unit>" and DataSize as an exact byte count with a B suffix, so
decoding either into a numeric field yields a plausible-looking page of
zeroes. Both parsers return 0 for anything unreadable rather than failing --
a coordinator upgrade that changes a unit spelling should cost one column,
not the view an operator opened because something is wrong.

Every read is cached and timeout-bounded, and a refresh in flight serves the
previous value rather than queueing readers behind it. The console polls
from every open tab and /v1/query walks every query the coordinator holds;
an incident is when the most tabs are open and the scheduler can least
afford it. For the same reason an unreachable coordinator degrades to
available:false plus a reason, with provisioning state still served from the
config store -- "the cell is down" and "these tenants never provisioned" are
different incidents with different fixes.

A deployment with no cell (DUCKGRES_TRINO_COORDINATOR_URL unset) leaves the
routes unregistered and the SPA renders a "no cell configured" state.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@fuziontech
fuziontech requested a review from a team August 26, 2026 22:54
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown

Test Impact Plan

Deterministic summary of how this PR changes tests, CI runners, and coverage-risk signals.

Summary

Area Added Changed Deleted
Test files 2 3 0
E2E/journey files 0 0 0
Workflow files 0 0 0

Signals

  • Test cases: +45 / -0
  • Assertions: +162 / -6
  • Skips or known failures added: 0
  • Workflow continue-on-error added: 0
  • Workflow path filters added: 0
  • Test commands removed from justfile: 0
  • E2E/journey retry lines added: 0

Coverage risk: neutral or increased

No coverage-reduction warnings detected.

…real reach

Three findings from reviewing the preceding commits.

The `active=1` filter allowlisted {RUNNING, QUEUED}. Trino has NINE query
states and only FINISHED and FAILED are terminal, so that quietly dropped
WAITING_FOR_RESOURCES, DISPATCHING, PLANNING, STARTING and FINISHING from
the live view -- which is the console's default. PLANNING is the one that
stings: on a DuckLake-backed cell, planning talks to the tenant's metadata
Postgres, so "stuck in PLANNING" is among the most likely things an operator
opens this page to find, and it was invisible. Both sides now define active
as the complement of the terminal pair, mirroring QueryState.isDone(), so
the server's filter and the UI's kill affordance cannot disagree about what
is still in flight.

ReadSystemInformation is one operation gating EVERY MANAGEMENT_READ resource
on the coordinator, not just the two the console reads. The grant also
carries /v1/thread, /v1/announce (GET), /v1/maxActiveSplits and
/v1/integrations/gateway. All six are GETs of cluster-operational state and
none reads a catalog, a table or SQL text -- and the node-registering POST
on /v1/announce is INTERNAL_ONLY, so this cannot join a fake worker to the
cell -- but the previous comment named only two endpoints and understated
the blast radius of a grant in a file we treat as security review. The set
is now enumerated where the rule is written.

Handlers that already hold the principal index now pass it to liveQueries
instead of making it read the config store a second time. /trino/status is
polled every few seconds by every open tab, so the duplicate join was the
most-repeated query on the surface.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@fuziontech

Copy link
Copy Markdown
Member Author

Review

Read the whole diff again with fresh eyes. Three real findings, all now fixed in 4f94932.

1. active=1 hid five in-flight states (the significant one)

The server's active filter allowlisted {RUNNING, QUEUED}. Trino has nine query states and only FINISHED/FAILED are terminal (QueryState.isDone()), so the live view — which requests active=1 by default — silently dropped WAITING_FOR_RESOURCES, DISPATCHING, PLANNING, STARTING and FINISHING.

PLANNING is the one that stings. On a DuckLake-backed cell, planning talks to the tenant's metadata Postgres, so "stuck in PLANNING" is among the most likely things an operator opens this page to find — and it was invisible. Worse, the UI's isActiveTrinoQuery used a different allowlist that included PLANNING/STARTING, so the two sides disagreed about what "active" meant.

Both now define active as the complement of the terminal pair, mirroring QueryState.isDone(), so they can't drift. TestActiveFilterKeepsEveryNonTerminalState walks all nine.

2. ReadSystemInformation is wider than the comment claimed

It's a single operation gating every MANAGEMENT_READ resource, not just the two the console reads. The real set is /v1/node, /v1/resourceGroupState, /v1/thread, /v1/announce (GET), /v1/maxActiveSplits, /v1/integrations/gateway.

All six are GETs of cluster-operational state; none reads a catalog, a table, or SQL text. /v1/thread is the widest (thread names embed query/task ids). The node-registering POST on /v1/announce is INTERNAL_ONLY and needs the node-to-node shared secret, so this grant can't join a fake worker to the cell.

Not a hole, but understating the blast radius of a grant in a file we treat as security review is the wrong default. The set is now enumerated where the rule is written.

3. Duplicate config-store read per request

handleStatus, handleOrgs and handleOrgDetail each built the principal index and then had liveQueries build it again. /trino/status is polled every few seconds by every open tab, so that join was the most-repeated query on the surface. Handlers now pass the index they already hold.

Checked and found fine

  • ResourceGroupId serializes via @JsonValue getSegments() as a JSON array, so the []string decode is right (I'd initially suspected a dotted string).
  • /v1/query/{id} returns QueryInfo, which does carry resourceGroupId and every queryStats field the client decodes — the detail path is not a degraded shape.
  • liveQueries copies before stamping Org, so the shared cache slice is never mutated; the filter/sort path allocates its own backing array.
  • The cache's stale-over-blocking path is race-free (populated is read under the lock).
  • handleStatus reads /v1/info first because it's PUBLIC in Trino — that's what makes "the cell is down" distinguishable from "the console can't see it", and the 403 path does reach trinoUnavailableReason.
  • failureInfo on QueryInfo is deliberately not decoded — it can carry SQL fragments.

Verification after the fixes: golangci-lint run 0 issues, Go tests green, UI 160/160, typecheck and eslint clean.

@fuziontech
fuziontech merged commit 56d3830 into main Aug 27, 2026
31 checks passed
@fuziontech
fuziontech deleted the trino-admin-observability branch August 27, 2026 18:05
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