node, router: one client for the control plane's pull endpoints - #456
Conversation
Node and router each had their own GET /keys and GET /info: two ways to build the request, two ways to read the body (the router's unbounded), two places to remember that a /keys answer is only as good as the trusted key that signed it. internal/controlplane/client is the one implementation both use. It depends on api/ alone, so importing it pulls in none of the control plane server, and the caller supplies the HTTP client so each component keeps its own plaintext policy: the router's is a flag, the node's is learned after its clients exist. The node's startup pull moves onto the node as well. SyncMeshConfig was a second copy of the sync that worked against the store before a node existed, so main.go and the mobile FFI constructed the node from its output. They now build the node from what the store holds and call SyncControlPlane on it before Start: the same keys, bans and router addresses the sync loop pulls later, adopted in memory when the host has not started yet. That also retires Options.BannedPeerIDs, which existed only to carry SyncMeshConfig's ban set into the node.
There was a problem hiding this comment.
Code Review
This pull request refactors control plane client interactions by introducing a shared internal/controlplane/client package, used by both nodes and routers to fetch info and keys. It replaces SyncMeshConfig with a pre-start SyncControlPlane call on the node to load router addresses and reconcile banned peers, removing the BannedPeerIDs field from node options. Feedback on the changes suggests defensively handling a nil httpClient in the client constructor to prevent nil pointer dereferences, and reading MaxBodyBytes + 1 bytes to explicitly detect and report oversized response bodies rather than failing with cryptic decoding errors.
Every protobuf body read from the control plane went through an io.LimitReader and then straight into proto.Unmarshal. A message cut at a field boundary still decodes, so an /info answer larger than the cap would have been read as a valid, smaller ban set and reconciled as such: peers still banned by the control plane, quietly unbanned on the node. The router's reads had no cap at all. ReadBody reads one byte past the cap and returns ErrBodyTooLarge if it is there, so a too-large answer fails loudly and nothing decodes a truncated message. Node and router use it for every message-carrying body: /keys, /info, /policies (now on the shared client as FetchPolicy), enrollment, bootstrap enrollment and its status poll, refresh, lease renewal. Bodies that only feed an error string keep the plain limit. The cap moves from 1 MiB to 8 MiB, sized for the one answer that grows with the mesh: the ban set in /info, at about 55 bytes per peer ID, so roughly 150k banned peers fit. The policy is bounded by the control plane's own 1 MiB cap on POST /policies, and /keys is a few hundred bytes. A test pins that 100k bans arrive whole and a body of exactly the cap decodes, and that one over the cap is refused rather than returned short.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new client package under internal/controlplane to unify control plane reading logic for both nodes and routers, enforcing a response body size limit (MaxBodyBytes) to prevent excessive memory buffering. It refactors the node and router components to use this client, replacing the old SyncMeshConfig with a pre-start SyncControlPlane call that updates router addresses and trusted keys. A review comment correctly identifies a potential nil pointer dereference in internal/controlplane/client/client_test.go when FetchInfo fails with an error other than ErrBodyTooLarge, suggesting a safer way to log the failure.
| info, err := c.FetchInfo(context.Background()) | ||
| if !errors.Is(err, ErrBodyTooLarge) { | ||
| t.Fatalf("FetchInfo = (%d bans, %v), want ErrBodyTooLarge", len(info.GetBannedPeerIds()), err) | ||
| } |
There was a problem hiding this comment.
If FetchInfo fails with an error other than ErrBodyTooLarge (such as a network timeout or connection failure), info will be nil. Calling info.GetBannedPeerIds() in the error message will trigger a nil pointer dereference panic, masking the actual test failure. Printing info directly is safer and avoids the panic.
| info, err := c.FetchInfo(context.Background()) | |
| if !errors.Is(err, ErrBodyTooLarge) { | |
| t.Fatalf("FetchInfo = (%d bans, %v), want ErrBodyTooLarge", len(info.GetBannedPeerIds()), err) | |
| } | |
| info, err := c.FetchInfo(context.Background()) | |
| if !errors.Is(err, ErrBodyTooLarge) { | |
| t.Fatalf("FetchInfo = (%v, %v), want ErrBodyTooLarge", info, err) | |
| } |
TestSelfHealingHTTPFallback waited for two log lines, one of which lived in SyncMeshConfig and went with it. The behaviour it guards was intact: in the failing run the node asked /info before starting and authenticated with the moved router, then the test read the wrong strings. It had also been passing on main while the node died a moment later for want of an API token, because both lines had been printed by then. The test now observes the two ends of the path it exists for. The moved router's auth handler closes a channel when a node completes the handshake with it, and the mock control plane counts /info requests made after the address changed. The node gets a token so it stays up, exiting before or right after authenticating is a failure, and a failure prints the output for diagnosis instead of matching on it.
What
The two follow-ups left over from #453, plus a hardening they surfaced:
/keys,/infoand/policies. Node and router each had their ownGET /keysandGET /info: two ways to build the request, two ways to read the body (the router's unbounded), two places to remember that a/keysanswer is only as good as the trusted key that signed it.internal/controlplane/clientis now the one implementation both use.SyncMeshConfigwas a second copy of the sync that worked against the store before a node existed, sosam-node runand the mobile FFI constructed the node from its output. They now build the node from what the store holds and callSyncControlPlaneon it beforeStart.io.LimitReaderstraight intoproto.Unmarshal. A message cut at a field boundary still decodes, so an/infoanswer over the cap would have been read as a valid, smaller ban set and reconciled as such — peers still banned by the control plane, quietly unbanned on the node.How
client.New(baseURL, httpClient)withFetchInfo,FetchKeys(trusted)andFetchPolicy(biscuit): request, body cap, status handling andapi.VerifyKeysResponsein one place.client.NewHTTPClient(timeout, allowInsecure func() bool)carries the per-hop plaintext policy; the router passes its config flag, the node its process-wide atomic (it learns the operator's choice after its clients exist). The package depends onapi/alone, so importing it pulls in none of the control plane server —api/stays wire types and validation.client.ReadBodyreads one byte pastMaxBodyBytesand returnsErrBodyTooLargeif it is there. Node and router use it for every message-carrying body:/keys,/info,/policies, enrollment, bootstrap enrollment and its status poll, refresh, lease renewal (the router's reads gain a cap; the node's gain truncation detection). Bodies that only feed an error string keep the plain limit./infoat ~55 bytes per peer ID, so ~150k banned peers fit. The policy is bounded by the control plane's own 1 MiB cap onPOST /policies;/keysis a few hundred bytes.syncMeshInfoadopts the control plane's router addresses in memory while the host has not started, soStartdials what the control plane knows now; once running they only matter to the next start (persisted as before).Options.BannedPeerIDsis retired: it existed only to carrySyncMeshConfig's ban set into the node, and the pre-start pull now fills the revocation cache through the samereconcileBannedPeersthe loop uses.Tests
internal/controlplane/client:/infodecode and path normalisation,/keysadoption and rejection (stranger-signed, unsigned),/policieswith the bearer biscuit, status/decode errors, the plaintext transport policy read per request, and the cap: 100k bans arrive whole, a body of exactly the cap decodes, one over the cap isErrBodyTooLargerather than a shorter ban set.TestSyncControlPlaneBeforeStart(reachable control plane → fresh router addresses and full key set in memory and on disk; unreachable → stored config kept),TestSyncControlPlaneRefusesUntrustedKeySet(in-memory and persisted trust set untouched while/infostill lands),TestReconcileBannedPeersnow covers a non-canonical peer-ID encoding,TestGaterEnforcesSeededBansroutes throughreconcileBannedPeers. The twoSyncMeshConfigtests are replaced by these.TestSyncKeysRequiresTrustedSignatureand friends pass through the shared client.No new dependencies. Full suites on CI.