Add node self-reported service catalog and console mnemonics for node labels - #392
fer-marino wants to merge 4 commits into
Conversation
control plane's MeshAdapter.DiscoverServices/GetNodeStatus are real interface methods that were never implemented (both P2PMeshAdapter and NopMeshAdapter just return nil, nil) and never called from anywhere - scaffolding for mesh-wide service visibility that was never finished. A faithful implementation would need the control plane to become a DHT participant itself, which is a much larger change. This takes a simpler, push-based path instead: a node already knows its own local service list (ListLocalServices, the same data list_local_services answers on the node itself) and already runs periodic control-plane check-ins (see startPolicySyncLoop). Add a sibling loop that POSTs that list to a new /nodes/catalog endpoint, authenticated by the node's own biscuit so it can only ever report on itself. The control plane caches it in memory (live-status, not authoritative - lost on restart, refreshed on the node's next report) and surfaces it via HandleAdminStatus's existing JSON response. Console gets a new Services view joining peer ID + reported services, where today there is no service visibility in the admin UI at all.
Peer IDs are the only identifier for a node in the Nodes table today, and now also in the new Services table - both hard to tell apart at a glance. EnrolledNode.Labels already exists and is populated from sam-node.yaml's labels: key, but was never rendered anywhere. Show it as a small key=value line under the peer ID in both tables. This is a display-only convenience: labels carry no cryptographic proof and are not wired into any authorization path (see api.TargetFactRules), so they must never be treated as a trust primitive, only ever as an operator-facing lookup aid.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a push-based service catalog feature where nodes periodically self-report their locally registered services to the control plane via a new POST /nodes/catalog endpoint, which is then displayed in a new "Services" tab in the admin console. Feedback on this PR highlights the lack of unit tests for the new catalog endpoint and reporting loop, violating the repository's test pyramid rule. Additionally, defensive checks are recommended in both the Go backend and JavaScript frontend to handle potential null or falsy service elements safely, and the reported timestamp in the UI should be formatted consistently with other dates in the console.
- Filter nil elements out of a catalog report's services array before
caching it (HandleNodeCatalog): a malformed report like
{"services": [null]} unmarshals cleanly into a nil slice element,
which would otherwise panic the console when it renders that peer's
entry.
- Skip falsy service entries in the same spot on the console side
(renderServicesTable), as defense in depth.
- Format the Services table's Reported At column with
toLocaleString(), matching every other date the console renders
instead of a raw RFC3339 string.
- Add unit tests for HandleNodeCatalog (auth, admission, method,
nil-filtering) and ReportNodeCatalog (request shape, HTTP error
propagation), per the repo's test-pyramid rule.
|
Thanks for the review jimmy - addressed all four points in 632b8dd. Filtered nil elements out of a catalog report's services array server-side before caching (HandleNodeCatalog), and added the matching defensive skip on the console side. Reported At in the Services table now uses toLocaleString(), consistent with the rest of the console. Added unit tests: TestHandleNodeCatalog (plus auth/admission/method-not-allowed/nil-filtering variants) in internal/controlplane/catalog_test.go, and TestReportNodeCatalog (plus HTTP error propagation) in internal/node/controlplane_test.go. |
hahah do we need to add something to |
peerCell (the label sub-line under a peer ID) was injecting an inline style= attribute via innerHTML, which breaks under any style-src CSP without 'unsafe-inline' - exactly the pattern tests/ui/console.spec.js's "no inline style attribute" test exists to keep out of the codebase, just not caught there since that test only scans the served index.html, not app.js's generated markup. Moved it to a real class (.cell-subtext). Also answers aojea's question on the PR about test/UI coverage: added two Playwright tests pinning that the Services view is reachable from the nav and deep-linkable, and renders its empty state without a JS error. A real populated Services table needs an actual node self-reporting, which this suite has no fixture for (no other admin table - Nodes, Enrollments - gets that kind of coverage here either); that path is already covered at the Go level by TestHandleNodeCatalog (internal/controlplane) and TestReportNodeCatalog (internal/node). Verified locally end-to-end: sam-control-plane + sam-console (built from this branch) against a local mock OIDC issuer, full tests/ui/console.spec.js suite green (17/17) including the two new tests.
|
Good question - pushed 6188cca which does two things. First, a real bug this made me look for: peerCell (the label line under a peer ID) was injecting an inline style= attribute via innerHTML, which would break under any style-src CSP without 'unsafe-inline' - exactly what tests/ui/console.spec.js's "no inline style attribute" test exists to prevent, just not caught there since that test only scans the served index.html, not app.js's generated markup. Moved it to a real class (.cell-subtext). Second, added two Playwright tests: the Services view is reachable from the nav, and it's deep-linkable via the hash, both asserting the empty state renders without a JS error. I couldn't do more than that from the UI suite - a populated Services table needs a real node self-reporting, and this suite has no fixture for that (Nodes and Enrollments don't get that kind of coverage here either, same reason). The actual reporting path is already covered at the Go level from the earlier review round: TestHandleNodeCatalog in internal/controlplane and TestReportNodeCatalog in internal/node. Verified all of this locally end to end rather than just by inspection: built sam-control-plane + sam-console from this branch, ran them against a local mock OIDC issuer, and the full tests/ui/console.spec.js suite is green (17/17, including the two new tests). |
this make things like this easier #360 ;) just building that component gives you a standalone control plane with the console |
|
Ha, noticed that once I went looking - #360 is exactly why building just cmd/sam-control-plane and cmd/sam-console gave me a working standalone stack in two build commands. Nice payoff. |
|
@fer-marino I hope you don't mind I took over this PR in #408 , I made sure your authored work remains , all credit to you thanks |
|
2 participants
Why
The admin console has no service-level visibility at all today: an operator can see enrolled nodes and routers, but has no way to answer "what services is this mesh actually running, and where". Separately, both the Nodes table and (now) the new Services table only ever show a bare peer ID, which is unreadable at a glance - there is no node mnemonic/name anywhere in SAM.
MeshAdapter.DiscoverServices/GetNodeStatusare real interface methods scaffolded for exactly the first problem, but bothNopMeshAdapterandP2PMeshAdapteronlyreturn nil, niland neither is called from anywhere. A faithful implementation would require the control plane to become a DHT participant itself (it currently isn't), which is a much bigger change than this gap warrants.What this does
Push-based service catalog. A node already knows its own local service list (
ListLocalServices) and already runs a periodic control-plane check-in (startPolicySyncLoop). This adds a sibling loop (CatalogReportInterval, default 1m) that POSTs that list to a newPOST /nodes/catalogendpoint, authenticated with the node's own biscuit so a node can only ever report on itself (peer ID is extracted server-side from the verified token, never trusted from the request body). The control plane caches the result in memory, keyed by peer ID - this is a live-status view, not authoritative state, so it's fine to lose on restart; the node re-reports on its next tick.Console "Services" view. Joins the cached per-node catalog into a flat table (service name, type, description, reporting node, last-reported timestamp), using the existing
HandleAdminStatusJSON response as the transport (no proto/wire changes - this is plain JSON like/admin/bootstrap-tokensalready is).Node labels as mnemonics.
EnrolledNode.Labelsalready exists and is already populated fromsam-node.yaml'slabels:key, but was never surfaced anywhere. Both the Nodes table and the new Services table now show it as a smallkey=valueline under the peer ID.Labels are display-only. They carry no cryptographic proof and are not wired into
api.TargetFactRulesor any other authorization path, so they must never become a trust primitive - only ever an operator-facing lookup aid alongside the real identifier (the peer ID).Testing
go build ./...passes.go test ./internal/controlplane/... ./internal/node/... ./internal/console/...shows the same pre-existing/environmental failures as on main (unrelated to this change, e.g. TestNodeProactiveTokenRefresh, a handful of internal/node backend/socket tests) and no new failures. Manually verified end-to-end against a local sam-one instance with two nodes reporting real services (screenshots below).