mirror of
https://github.com/jmcorgan/fips.git
synced 2026-10-05 19:18:25 +00:00
fix(control): report the bound peer's traffic counters in show links
show_links rendered packets_sent, packets_recv, bytes_sent, bytes_recv and last_recv_ms from the LinkStats held on the Link record, but nothing on the data plane ever wrote that copy. Every send and receive counter write goes to the separate LinkStats held on the active peer, which show_peers reads. So every link reported zero however much traffic it carried, while show_peers showed the traffic for the same link_id (GitHub issue #158). The two copies were never connected: the Link counters have had no production writer since they were added, and show_links read them from the day it was introduced. Both render sites now take a link's counters from the peer bound to it, and fall back to the link's own (zero) counters when no peer is bound yet, as for a link still in handshake. That covers the on-loop handler and the tick-published snapshot the control socket serves from, which must stay byte-identical. A small helper on Node maps each peer's link_id to its counters once per render. The fix reads the counters rather than writing a second copy at each send and receive site. Writing both would add a link lookup per packet on the hot path and keep two sources of truth that every future writer must keep in step, which is how this defect arose. Removing the unused Link counters would change public methods, so that is left for a separate cleanup. The counters follow the peer across address changes, while the row's transport_id and remote_addr remain those the link was created with; that matches how show_peers already keys the same counters by link_id. The counters cover authenticated link frames only, so they are not expected to equal the transport totals in show_transports. The response shape is unchanged. The new test establishes a real two-node link over the loopback transport and checks each node's show_links row against that node's peer counters, after first requiring the counters to be non-zero so a run with no traffic cannot pass. It also requires node 1's received count to be non-zero and no greater than node 0's sent count, a bound that does not come from the same node's peer copy. It fails on the unfixed code with packets_sent 0 against 3, and breaking only the snapshot site fails its on-loop and snapshot equality check. A second test keeps a link with no peer in the output with zero counters on both render paths. Fixes #158
This commit is contained in:
@@ -30,6 +30,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
|
|||||||
interval instead. That retry interval gates only a peer whose last attempt
|
interval instead. That retry interval gates only a peer whose last attempt
|
||||||
failed, so it cannot clamp a `heartbeat_interval_secs` configured below it.
|
failed, so it cannot clamp a `heartbeat_interval_secs` configured below it.
|
||||||
|
|
||||||
|
#### Control socket
|
||||||
|
|
||||||
|
- `show_links` (`fipsctl show links`) now reports the traffic a link has
|
||||||
|
carried. Its `packets_sent`, `packets_recv`, `bytes_sent`, `bytes_recv` and
|
||||||
|
`last_recv_ms` were read from counters on the link record that nothing on
|
||||||
|
the data plane ever wrote, so every link reported zero however much traffic
|
||||||
|
it carried, while `show_peers` counted the same traffic on the peer. A link
|
||||||
|
bound to an authenticated peer now reports that peer's counters, so the two
|
||||||
|
queries agree for the same `link_id`; a link still in handshake has no peer
|
||||||
|
yet and still reports zero. The counters follow the peer across address
|
||||||
|
changes, while the row's `transport_id` and `remote_addr` stay those the
|
||||||
|
link was created with. The counters cover authenticated link frames only, so
|
||||||
|
they are not expected to match the transport totals in `show_transports`.
|
||||||
|
The response shape is unchanged.
|
||||||
|
|
||||||
### Changed
|
### Changed
|
||||||
|
|
||||||
- The lockfile moves `chacha20` from 0.10.1 to 0.10.2, because 0.10.1 is yanked.
|
- The lockfile moves `chacha20` from 0.10.1 to 0.10.2, because 0.10.1 is yanked.
|
||||||
|
|||||||
+47
-1
@@ -567,10 +567,14 @@ pub(crate) fn show_peers_from_handle(handle: &super::read_handle::ControlReadHan
|
|||||||
|
|
||||||
/// `show_links` — Active links.
|
/// `show_links` — Active links.
|
||||||
pub fn show_links(node: &Node) -> Value {
|
pub fn show_links(node: &Node) -> Value {
|
||||||
|
let counters = node.link_counters();
|
||||||
let links: Vec<Value> = node
|
let links: Vec<Value> = node
|
||||||
.links()
|
.links()
|
||||||
.map(|link| {
|
.map(|link| {
|
||||||
let stats = link.stats();
|
let stats = counters
|
||||||
|
.get(&link.link_id())
|
||||||
|
.copied()
|
||||||
|
.unwrap_or_else(|| link.stats());
|
||||||
json!({
|
json!({
|
||||||
"link_id": link.link_id().as_u64(),
|
"link_id": link.link_id().as_u64(),
|
||||||
"transport_id": link.transport_id().as_u32(),
|
"transport_id": link.transport_id().as_u32(),
|
||||||
@@ -3419,6 +3423,48 @@ mod tests {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// A link with no authenticated peer yet (one still in handshake) keeps its
|
||||||
|
/// row in `show_links`, with zero traffic counters, on both the on-loop and
|
||||||
|
/// the snapshot render. Guards against dropping unbound rows.
|
||||||
|
#[test]
|
||||||
|
fn show_links_keeps_a_link_with_no_bound_peer_and_reports_zero_counters() {
|
||||||
|
use crate::transport::{Link, LinkDirection, LinkId, TransportAddr, TransportId};
|
||||||
|
|
||||||
|
let mut node = build_test_node();
|
||||||
|
let link_id = LinkId::new(7);
|
||||||
|
node.add_link(Link::connectionless(
|
||||||
|
link_id,
|
||||||
|
TransportId::new(1),
|
||||||
|
TransportAddr::from_string("127.0.0.1:2121"),
|
||||||
|
LinkDirection::Outbound,
|
||||||
|
Duration::from_millis(50),
|
||||||
|
))
|
||||||
|
.expect("an empty node has room for a link");
|
||||||
|
|
||||||
|
let links = show_links(&node);
|
||||||
|
let rows = links["links"].as_array().expect("links array");
|
||||||
|
assert_eq!(rows.len(), 1, "the unbound link must be listed");
|
||||||
|
let row = &rows[0];
|
||||||
|
assert_eq!(row["link_id"], link_id.as_u64());
|
||||||
|
for key in [
|
||||||
|
"packets_sent",
|
||||||
|
"packets_recv",
|
||||||
|
"bytes_sent",
|
||||||
|
"bytes_recv",
|
||||||
|
"last_recv_ms",
|
||||||
|
] {
|
||||||
|
assert_eq!(row["stats"][key], 0, "{key} for a link with no peer");
|
||||||
|
}
|
||||||
|
|
||||||
|
node.record_stats_history();
|
||||||
|
let handle = node.control_read_handle();
|
||||||
|
assert_eq!(
|
||||||
|
render(show_links(&node)),
|
||||||
|
render(show_links_from_handle(&handle)),
|
||||||
|
"off-loop show_links must match on-loop output for an unbound link"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
// ---- native datagram API coverage ------------------------------------
|
// ---- native datagram API coverage ------------------------------------
|
||||||
|
|
||||||
/// Freshness + fidelity: after a `record_stats_history()` tick (the native
|
/// Freshness + fidelity: after a `record_stats_history()` tick (the native
|
||||||
|
|||||||
+17
-1
@@ -2277,10 +2277,14 @@ impl Node {
|
|||||||
.collect();
|
.collect();
|
||||||
|
|
||||||
// --- links (show_links) ---
|
// --- links (show_links) ---
|
||||||
|
let counters = self.link_counters();
|
||||||
let link_rows: Vec<snap::LinkRow> = self
|
let link_rows: Vec<snap::LinkRow> = self
|
||||||
.links()
|
.links()
|
||||||
.map(|link| {
|
.map(|link| {
|
||||||
let stats = link.stats();
|
let stats = counters
|
||||||
|
.get(&link.link_id())
|
||||||
|
.copied()
|
||||||
|
.unwrap_or_else(|| link.stats());
|
||||||
snap::LinkRow {
|
snap::LinkRow {
|
||||||
link_id: link.link_id().as_u64(),
|
link_id: link.link_id().as_u64(),
|
||||||
transport_id: link.transport_id().as_u32(),
|
transport_id: link.transport_id().as_u32(),
|
||||||
@@ -2728,6 +2732,18 @@ impl Node {
|
|||||||
self.links.values()
|
self.links.values()
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// Traffic counters for each link bound to an active peer, keyed by link.
|
||||||
|
///
|
||||||
|
/// The data plane counts a link's authenticated traffic on the peer that
|
||||||
|
/// owns it (`ActivePeer::link_stats_mut`), not on the `Link` record, so a
|
||||||
|
/// link still in handshake has no entry here.
|
||||||
|
pub(crate) fn link_counters(&self) -> HashMap<LinkId, &crate::transport::LinkStats> {
|
||||||
|
self.peers
|
||||||
|
.values()
|
||||||
|
.map(|p| (p.link_id(), p.link_stats()))
|
||||||
|
.collect()
|
||||||
|
}
|
||||||
|
|
||||||
// === Connection Management (Handshake Phase) ===
|
// === Connection Management (Handshake Phase) ===
|
||||||
|
|
||||||
/// Whether `link_id` has a pending handshake, read through the control
|
/// Whether `link_id` has a pending handshake, read through the control
|
||||||
|
|||||||
@@ -311,3 +311,113 @@ async fn test_api_disconnect_unknown_peer_changes_nothing() {
|
|||||||
assert_eq!(node.peer_machines.len(), machines_before);
|
assert_eq!(node.peer_machines.len(), machines_before);
|
||||||
assert_eq!(node.links.len(), links_before);
|
assert_eq!(node.links.len(), links_before);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/// The `show_links` row whose `link_id` is `link_id`.
|
||||||
|
fn link_row(links: &serde_json::Value, link_id: LinkId) -> &serde_json::Value {
|
||||||
|
links["links"]
|
||||||
|
.as_array()
|
||||||
|
.expect("show_links returns a links array")
|
||||||
|
.iter()
|
||||||
|
.find(|row| row["link_id"] == link_id.as_u64())
|
||||||
|
.expect("show_links lists the link bound to the peer")
|
||||||
|
}
|
||||||
|
|
||||||
|
/// Check one node's `show_links` row for the link it shares with `peer_idx`
|
||||||
|
/// against the counters the data plane kept on that peer, and return the row.
|
||||||
|
fn assert_link_row_matches_peer(
|
||||||
|
nodes: &[TestNode],
|
||||||
|
node_idx: usize,
|
||||||
|
peer_idx: usize,
|
||||||
|
) -> serde_json::Value {
|
||||||
|
let peer_addr = *nodes[peer_idx].node.node_addr();
|
||||||
|
let peer = nodes[node_idx]
|
||||||
|
.node
|
||||||
|
.get_peer(&peer_addr)
|
||||||
|
.expect("the tree test establishes the peer");
|
||||||
|
let link_id = peer.link_id();
|
||||||
|
let expected = peer.link_stats().clone();
|
||||||
|
|
||||||
|
// Without traffic every counter is zero on both copies and the comparison
|
||||||
|
// below would pass whether or not show_links reads the right one.
|
||||||
|
assert!(expected.packets_sent > 0, "node {node_idx} sent no frames");
|
||||||
|
assert!(
|
||||||
|
expected.packets_recv > 0,
|
||||||
|
"node {node_idx} received no frames"
|
||||||
|
);
|
||||||
|
assert!(expected.bytes_sent > 0, "node {node_idx} sent no bytes");
|
||||||
|
assert!(expected.bytes_recv > 0, "node {node_idx} received no bytes");
|
||||||
|
assert!(
|
||||||
|
expected.last_recv_ms > 0,
|
||||||
|
"node {node_idx} stamped no receive time"
|
||||||
|
);
|
||||||
|
|
||||||
|
let links = crate::control::queries::show_links(&nodes[node_idx].node);
|
||||||
|
let row = link_row(&links, link_id).clone();
|
||||||
|
let stats = &row["stats"];
|
||||||
|
assert_eq!(
|
||||||
|
stats["packets_sent"], expected.packets_sent,
|
||||||
|
"node {node_idx}"
|
||||||
|
);
|
||||||
|
assert_eq!(
|
||||||
|
stats["packets_recv"], expected.packets_recv,
|
||||||
|
"node {node_idx}"
|
||||||
|
);
|
||||||
|
assert_eq!(stats["bytes_sent"], expected.bytes_sent, "node {node_idx}");
|
||||||
|
assert_eq!(stats["bytes_recv"], expected.bytes_recv, "node {node_idx}");
|
||||||
|
assert_eq!(
|
||||||
|
stats["last_recv_ms"], expected.last_recv_ms,
|
||||||
|
"node {node_idx}"
|
||||||
|
);
|
||||||
|
row
|
||||||
|
}
|
||||||
|
|
||||||
|
/// `show_links` reports the traffic a link has carried, not zero: for a link
|
||||||
|
/// bound to an authenticated peer its counters are the ones the data plane
|
||||||
|
/// keeps on that peer, on both ends of the link, and the tick-published
|
||||||
|
/// snapshot render agrees with the on-loop render.
|
||||||
|
#[tokio::test]
|
||||||
|
async fn show_links_reports_the_traffic_counters_of_the_peer_bound_to_each_link() {
|
||||||
|
let mut nodes = run_tree_test(2, &[(0, 1)], false).await;
|
||||||
|
|
||||||
|
let row0 = assert_link_row_matches_peer(&nodes, 0, 1);
|
||||||
|
|
||||||
|
// The off-loop render comes from the snapshot published on the tick.
|
||||||
|
nodes[0].node.record_stats_history();
|
||||||
|
let handle = nodes[0].node.control_read_handle();
|
||||||
|
let on_loop = crate::control::queries::show_links(&nodes[0].node);
|
||||||
|
let off_loop = crate::control::queries::show_links_from_handle(&handle);
|
||||||
|
assert_eq!(
|
||||||
|
off_loop, on_loop,
|
||||||
|
"the snapshot render of show_links must match the on-loop render"
|
||||||
|
);
|
||||||
|
let link_id = nodes[0]
|
||||||
|
.node
|
||||||
|
.get_peer(nodes[1].node.node_addr())
|
||||||
|
.expect("node 0 still has node 1")
|
||||||
|
.link_id();
|
||||||
|
let off_row = link_row(&off_loop, link_id);
|
||||||
|
assert!(
|
||||||
|
off_row["stats"]["packets_recv"].as_u64().unwrap_or(0) > 0,
|
||||||
|
"the snapshot render must carry the link's receive count, got {off_row}"
|
||||||
|
);
|
||||||
|
|
||||||
|
let row1 = assert_link_row_matches_peer(&nodes, 1, 0);
|
||||||
|
|
||||||
|
// A check that does not come from the same node's peer copy: every frame
|
||||||
|
// node 1 counted as received from node 0 was counted as sent by node 0.
|
||||||
|
// Loopback is lossless, but a frame sent before node 1 promoted node 0 is
|
||||||
|
// counted by the sender only, so the bound is not an equality.
|
||||||
|
let sent0 = row0["stats"]["packets_sent"]
|
||||||
|
.as_u64()
|
||||||
|
.expect("packets_sent is a number");
|
||||||
|
let recv1 = row1["stats"]["packets_recv"]
|
||||||
|
.as_u64()
|
||||||
|
.expect("packets_recv is a number");
|
||||||
|
assert!(recv1 > 0, "node 1's link row shows no frames received");
|
||||||
|
assert!(
|
||||||
|
recv1 <= sent0,
|
||||||
|
"node 1's link row counts {recv1} frames received, more than the {sent0} node 0 sent"
|
||||||
|
);
|
||||||
|
|
||||||
|
cleanup_nodes(&mut nodes).await;
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user