Node self-reported service catalog and console mnemonics for node labels - #408
Merged
Merged
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.
- 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.
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.
Both the testnet HTTPRoute and the sam-mesh chart expose the control plane behind an Exact-path allow-list, so the node's new catalog push 404'd at the gateway on every tick in any Gateway deployment. Add the path to both and pin it with a chart test.
Node-to-control-plane traffic goes through api/sam.proto, like /enroll, /refresh and /policies already do. Replace the two ad-hoc JSON mirrors of the catalog request with api.NodeCatalogReport and application/x-protobuf, so the wire shape is no longer tied to protoc-gen-go's struct tags. The console now receives a plain DTO (string type name) instead of the raw proto struct, restricted to nodes that are still admitted so a banned or expired node's last report no longer lingers in the Services view; banning also evicts the cached entry outright. Cap one report at 512 services so an admitted node cannot grow the cache without bound, and state in the handler why a bare bearer biscuit is acceptable here. Tests now cover forged and unenrolled biscuits, malformed auth and body, oversized reports, replace-not-append, the /admin/status view, and the session-expired and banned paths.
Spread each tick by up to a tenth of the interval so a fleet started together does not report in lockstep, make the initial delay an Option so tests and fast-start deployments can tune it, and log a persistent failure at Warn once and then at Debug: the usual causes (control plane unreachable, path not routed) do not change from tick to tick, and one warning a minute per node buries everything else. Cover the loop end to end against an httptest control plane, the precondition errors, and the Option defaults.
Playwright intercepts api/admin/status and splices in a node_catalog fixture plus a labelled enrolled node, pinning the rendered service rows (name, type, description, peer mnemonic labels, localised report time) and that node-controlled service fields render inert - a hostile description must not inject markup into an admin page.
The catalog cache is keyed by the canonical base58 form extracted from the verified biscuit, but catalogViewFor and dropCatalogEntry looked entries up with the enrollment record's PeerID, which is stored off the wire and may be any valid encoding of the same peer (e.g. CIDv1 base32). A node enrolled under a non-canonical spelling would report services the console never showed, and banNode's eviction would silently miss the cached entry. Decode-and-restringify at the lookup boundary (styleguide rule 1), scoped to the code this PR adds; canonicalizing the stored records themselves is handled by its own in-flight PR. The regression test enrolls a peer under its CIDv1 encoding and pins both the view join and the ban eviction.
The only remaining tag-pinned action; every other step in the workflow is already hash-pinned, and the blanket policy (zizmor) rightly flags it. v3 is also deprecated in favor of v4.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request introduces a node service catalog feature, allowing enrolled nodes to periodically self-report their locally registered services to the control plane via a new /nodes/catalog endpoint. The control plane caches these reports in-memory and exposes them to the admin console under a new 'Services' view. Feedback on the changes suggests separating database errors from missing enrollment checks in the catalog handler to return correct HTTP status codes, and adding defensive nil checks for services in the catalog view to prevent potential nil pointer dereferences.
Separate storage failures from missing enrollment when resolving the reporting node, matching HandleRefresh: a database error is a logged 500, not a misleading 401. Skip nil service entries when rendering the console view.
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
2 participants
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.
Continuation of #392 by @fer-marino, whose branch was clobbered by an accidental
force-push of an unrelated branch (
alpha-hardening+ stale main). This PRcarries the original four commits, rebased onto current
main, plus sixreview-fix commits. All feature credit to the original author.
Original feature (see #392 for full rationale)
service list to
POST /nodes/catalog, authenticated with the node's ownbiscuit (peer ID comes from the verified token, never the body).
Fixes on top
/nodes/catalogadded to theExact-path allow-lists in.github/k8s/sam-control-plane-template.yamland thesam-meshchart(plus chart test); without this every node got a gateway 404 on the testnets.
api.NodeCatalogReportinsam.proto(
application/x-protobuf), per the AGENTS.md rule; the two ad-hoc JSONmirrors are gone. The console receives a plain DTO with a string type name.
/admin/statusview is restricted to still-admittednodes; banning evicts the cached entry; one report is capped at 512 services.
stored peer ID before keying, with a CIDv1 regression test.
(
CatalogReportInitialDelay), and warn-once/debug-after logging instead ofone warning per minute per node.
auth/body, oversized report, replace-not-append, admission paths), node-side
loop coverage against an httptest control plane, options defaults, and a
Playwright test rendering the Services view from a mocked
/admin/status(including that node-controlled service fields render inert).
control-plane-configuration.md.Testing
make build, dockerizedmake lint, fullgo teston the touched packages,helm-unittest(40/40),make ui-testServices specs, andverify-generatedall pass. Also verified end-to-end against a live
sam-onewith an enrollednode self-reporting a labelled service (Services view renders name/type/
description/mnemonic/reported-at).
Note for @fer-marino:
feat/node-service-catalogcurrently points at anunrelated branch state - looks like an accidental push. Your reviewed work
(
6188cca) is fully preserved here.