List the Trino fleet, including worker version, from what the cell actually serves - #1133
Merged
Conversation
The Trino cluster page reports "Nodes are not reported by this cell" against prod-us. That copy is accurate — #1132 made it so — but it is not the end of the story: the cell can name its workers, the console was just asking for them at the one route this cell does not bind. Trino binds exactly one node-listing route, chosen by discovery.type: AIRLIFT_DISCOVERY /v1/node heartbeat health, + /v1/node/failed ANNOUNCE (default) /v1/announce the set of announced node URIs DNS /v1/announce same AnnounceNodeInventoryModule and DnsNodeInventoryModule both bind AnnounceNodeResource, so between the two routes every cell can list its fleet; only the detail differs. Try /v1/node, fall back to /v1/announce on the endpoint-unavailable error #1132 introduced. Both routes are declared @ResourceSecurity(MANAGEMENT_READ), which maps to checkCanReadSystemInformation, which __duckgres_observer already holds via the ReadSystemInformation grant. No policy.rego change: the tenant-isolation boundary is untouched, and the observer gains no catalog access. The announce inventory carries membership and nothing else, so the source travels with the data rather than being guessed downstream. Every heartbeat field on an announced node is a zero meaning "not measured", which is not the same as measured-and-zero: summarizing it would have the console report a 0.0 failure ratio and an "all healthy" fleet on the strength of numbers the coordinator never sent. TrinoNodeInventory.HasHealth gates that, the status payload carries node_source, and the page renders a membership-only table plus a note on what is missing and why, instead of zero-filled health columns. The Nodes stat card reads "membership only" rather than "all healthy". The remaining option, if per-node health on these cells is ever worth it, is system.runtime.nodes — node_id, http_uri, node_version, coordinator, state, so it would also close the version-skew gap. It costs the observer a scoped catalog grant and a resource-group selector that admits it, both changes to the isolation boundary, so it is deliberately not bundled here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Test Impact PlanDeterministic summary of how this PR changes tests, CI runners, and coverage-risk signals. Summary
Signals
Coverage risk: neutral or increased No coverage-reduction warnings detected. |
The announce fallback names the workers but reports nothing about them, and
it cannot report the one thing a rollout needs: which version each node is
running. system.runtime.nodes carries node_id, http_uri, node_version,
coordinator and state, and the system connector is served by every cell
regardless of discovery.type. So the order is now
/v1/node -> system.runtime.nodes -> /v1/announce
with the SQL path preferred over bare membership, and membership kept as the
last resort so a cell whose grant has not rolled out yet still lists a fleet.
This widens the observer, so the grant is written as narrowly as the OPA
operations allow: AccessCatalog on `system`, and SelectFromColumns pinned to
that one table. AccessCatalog opens nothing by itself -- every read still has
to pass SelectFromColumns -- so what stays denied is the part that matters:
system.runtime.queries tenant SQL text, unfiltered by query-owner
visibility (the REST path it already has is both
redacted and bounded by FilterViewQueryOwnedBy)
system.metadata.* enumerates every tenant catalog/schema/table/column
system.jdbc.* the same enumeration by another route
org_<tenant> unchanged; readable_catalog still excludes the
observer group, and this rule does not touch it
TestObserverSystemGrantIsPinnedToTheNodesTable pins each of those denials,
including a nodes table in another catalog and metadata browsing on `system`;
TestSystemNodesGrantIsObserverOnly pins that no tenant and not the admin gets
it, and that claiming the observer group without the observer username still
grants nothing. system.runtime.nodes itself contains no tenant identifier, so
the grant exposes no customer data even read in full.
The observer also needed its own resource-group lane. The last selector is
user `(?<org>.*)`, which matches anything, so without one the console's query
would have been admitted as a tenant into root.tenants.free.__duckgres_observer
-- and those leaves are JmxExport=true, so it would have shown up as a
phantom tenant in the per-tenant metrics added in #1124. Both operational
principals now select into the unexported admin tier ahead of the tenant
selectors.
The page gains Role / Version / State columns, and the Nodes card leads with
version skew when more than one version is present, since a stalled rollout
is the case worth seeing first. HasHealth stays false for this source: it
carries lifecycle state, not the failure detector's heartbeat ratios, and
conflating them would put a health badge on a number nothing measured.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
fuziontech
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Trino cluster page shows "Nodes are not reported by this cell". That
copy is accurate as of #1132 — but the cell can report its fleet, and can
report worker versions too. The console was asking at the one route this cell
does not bind.
Trino binds one node-listing route, chosen by
discovery.typediscovery.typeAIRLIFT_DISCOVERY/v1/node(+/v1/node/failed)ANNOUNCE(Trino's default, what these cells run)/v1/announceDNS/v1/announcesystem.runtime.nodesis served by every cell regardless ofdiscovery.type, and is the only source carryingnode_version. New order:Membership is kept as the last resort, so a cell whose grant hasn't rolled
out yet still lists a fleet instead of showing nothing.
The grant, and what stays denied
This widens the observer, so it is written as narrowly as the OPA operations
allow:
AccessCatalogonsystem, andSelectFromColumnspinned to thatone table.
AccessCatalogopens nothing by itself — every read stillpasses
SelectFromColumns.system.runtime.queriessystem.metadata.*system.jdbc.*org_<tenant>readable_catalogstill excludes the observer groupTestObserverSystemGrantIsPinnedToTheNodesTablepins each denial (including aruntime.nodesin another catalog, and metadata browsing onsystem);TestSystemNodesGrantIsObserverOnlypins that no tenant and not the admingets it, and that claiming the observer group without the observer username
still grants nothing.
system.runtime.nodesholdsnode_id,http_uri,node_version,coordinator,state— no tenant identifier, so thegrant exposes no customer data even read in full.
Resource groups
The observer needed its own lane. The last selector is user
(?<org>.*),which matches anything, so without one the console's query lands in
root.tenants.free.__duckgres_observer— and those leaves areJmxExport: true, so it would appear as a phantom tenant in theper-tenant metrics from #1124. Both operational principals now select into
the unexported admin tier ahead of the tenant selectors.
UI
Role / Version / State columns, and the Nodes card leads with version skew
when more than one version is present — a stalled rollout is the case worth
seeing first.
HasHealth()stays false for this source: it carries lifecyclestate, not heartbeat ratios, and conflating them would put a health badge on
a number nothing measured.
Verification
./controlplane/provisioner/...and./controlplane/provisioner/opa/...green./controlplane/admin/green (the*Postgresfailures are astale local schema, unrelated and pre-existing)
golangci-lint— 0 issues attributable to this changetsc -b --noEmit,eslintclean; vitest 166/166 (up from 160)/v1/announcenot queried when the systemtable answers, and not queried when
/v1/nodeanswers), nextUri draining,denied-grant falls back rather than failing the read, coordinator-first
sort, version/state decoding, and the full policy deny matrix