From 062efd8045e3e5219f89668215ccb7103256ca73 Mon Sep 17 00:00:00 2001 From: Arjen <18398758+Origami74@users.noreply.github.com> Date: Tue, 1 Sep 2026 00:38:56 +0100 Subject: [PATCH] feat(fipstop): surface interface presence in the transports view MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The observability work shipped as JSON only, which left the operator's live view saying `up` for a transport bound to nothing. That is the exact shape of the bug the whole mechanism exists to end: the original OpenWrt failure was expensive because the 802.11s link formed regardless, so nothing anyone could see said the node was deaf. Putting the data in show_transports and not in fipstop reproduced that at one remove. The list gains real columns. Instance name and the thing a transport is bound to are separate facts about separate columns, so they get separate columns: `Bound to` answers one question with different answers per transport type — a netdev for the interface-bound ones, the bound socket address for UDP and TCP, a truncated onion for Tor, a remote MAC for a link row. Packed into one label they left the netdev names ragged down the list, and that is the column an operator scans to find the interface they are looking for. The State column carries presence for an interface-bound transport rather than the lifecycle state. `up` is true from the moment the transport starts and stays true while its interface is missing, so it is precisely the wrong answer in the one case someone is scanning that column for; there is no room to show both and only one of them is news. A Policy column reads required or optional, and severity follows it: an absent interface whose absence is normal — a dock adapter that is not plugged in, a radio this board never had — is yellow, while an absent interface the config says to expect is red. That is the same split the daemon makes between staying Full and reporting Degraded; painting both red would train the operator to ignore red. Ordering is no longer arbitrary. show_transports iterated a HashMap, so the array order was whatever the hash seed produced, different on every daemon restart. Both render sites sort by ascending transport id, which is creation order, so the list groups by transport type for free — and it fixes fipsctl output as well as the view, which sorting in fipstop alone would not have. The identifying columns sit at the left. The first column was Min, so it absorbed every spare column of a wide terminal and shoved Instance and Bound-to into the middle, away from the names being scanned. It is fixed-width now, sized for the widest label that actually lives in it — a link's tree glyph and direction — with Peer taking the slack. The detail pane gains an Interface block: netdev, presence and how long it has been held, carrier, absence policy spelled out as its consequence rather than its config key, bind count (flagged when it has rebound) and failed binds when there are any. Carrier is spelled out because presence is IFF_UP — a bound interface with no carrier is a bridge with nothing plugged into it, which is normal and should not have to be inferred from silence — and the two counters separate an interface that is flapping from one that is there and refusing to bind. test(control): pin the show_transports interface block docs/reference/control-socket.md states the schema of each query response is pinned by the snapshots in src/control/snapshots/. For the interface block it was not: show_transports.json is `{"transports": []}`, because build_test_node keeps every runtime list empty, so the only thing pinned was the empty form. show_routing.json was regenerated for req_own_loopback in the same series, which is what makes the omission look accidental rather than considered. The block is emitted by two hand-duplicated sites — the live handler and the read-handle variant — that agree today with nothing enforcing it. A separate node and a separate snapshot rather than a richer build_test_node, so the nineteen existing snapshots keep the empty-state determinism they were built for. The fixture starts a real transport on an interface no host has, so presence is deterministically absent and carrier deterministically false everywhere this runs, and the captured shape is the one an operator actually meets: state `up` with the interface absent. That pairing is the whole reason the block exists, so it is worth having a committed artifact that shows it. since_secs joins VOLATILE_KEYS — it is elapsed time, so redaction pins the key's presence without pinning a value that changes between runs. insert_transport_for_test mirrors isolate_peer_acl_for_test: the snapshot tests live in crate::control and cannot reach Node's private transports map, and a narrow hook keeps the fixture honest rather than hand-authoring JSON that nothing in the daemon produces. fix(fipstop): make the transports table fit an 80-column terminal The table's fixed columns sum to 90, plus six single-column gaps: 96 against the ~77 usable inside an 80-column terminal's border and scrollbar. Ratatui resolves an over-subscribed layout by shrinking every column proportionally, so the overflow does not clip the rightmost column — it clips all of them, and `mesh0 (optional)` rendered as `mesh0 (optio`. The marker is the one thing on that row worth reading, and the absence of it is what says `required`. 80x24 is the OpenWrt serial console and the xterm/tmux default, so this is the deployment target rather than an edge case. Below 100 columns the table drops Tx and Rx and sizes the rest down. Those two are the only columns whose absence costs nothing an operator is scanning this table to find — they are byte counters, and the detail pane carries them in full — while Instance keeps 18 because `mesh0 (optional)` is 16, and Bound-to keeps 17 because that is a full MAC. Both row shapes carry the same seven cells in the same order, so the narrow variant is the same list with its tail cut, keeping one place where the column count is decided rather than two that have to agree. The detail view stacks instead of splitting below 110 columns. A 40% split of an 80-column terminal leaves the table 32 columns for a layout needing 65 even narrow, and ratatui spends all of it on the trailing columns — rendering Transport, Instance, Bound-to and State at width zero, so the operator gets blank rows. Stacking keeps both panes readable instead of both unreadable. The regression test renders at 80 and asserts the marker. That is precisely the gap that let this through: the existing test asserting `mesh0 (optional)` renders at 110, and the one rendering at 80 asserted only the netdev name, so between them neither covered the width where the layout breaks. Verified against the defect — with the narrow tier disabled, the new test fails and the other two still pass. The insert_transport_for_test hook now lands here rather than two commits earlier. Nothing called it before this commit's snapshot fixture, so the commits in between failed cargo clippy --all-targets -- -D warnings on dead code. It also carries the target gate its only caller has, so the definition and the call are present on the same platforms. Changelog entry for the view. Document the view in docs/reference/cli-fipstop.md. The reference page described the Transports tab as a tree of instances with per-link children and said nothing about an interface block, so the page and the tab disagreed the moment this commit landed. The tab-table row now points at a new Interface block section covering Interface, Presence, Carrier, Bound to, On absence and the two bind counters. --- CHANGELOG.md | 18 + docs/reference/cli-fipstop.md | 22 +- src/bin/fipstop/ui/snapshots.rs | 374 ++++++++++++++++++ src/bin/fipstop/ui/transports.rs | 328 ++++++++++++--- src/control/queries.rs | 52 +++ .../show_transports_with_interface.json | 36 ++ src/node/mod.rs | 15 + 7 files changed, 796 insertions(+), 49 deletions(-) create mode 100644 src/control/snapshots/show_transports_with_interface.json diff --git a/CHANGELOG.md b/CHANGELOG.md index 68ac4cc3..c920413c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 `failed_attempts`. The original boot-race bug was expensive precisely because nothing an operator could see said the node was deaf. +- `fipstop`'s transports view carries the same interface presence. The State + column shows an interface-bound transport's presence rather than its + lifecycle state — `up` is true from the moment the transport starts and stays + true while its interface is missing, which is precisely the wrong answer in + the one case someone is scanning that column for — and a new Policy column + reads `required` or `optional` beside it, with an absent required interface + red and an absent optional one yellow: the same split the daemon makes + between staying `Full` and reporting `Degraded`. The instance name and the + thing a transport is bound to are now separate columns, so netdev names line + up down the list instead of trailing ragged inside a packed label. The detail + pane gains an Interface block: netdev, presence and how long it has been + held, carrier, what the absence policy means rather than which key sets it, + bind count (flagged once it has rebound) and failed binds when there are any. + The table fits an 80-column terminal — the OpenWrt serial console and the + xterm and tmux default — dropping the byte counters below 100 columns and + stacking the detail pane below 110, rather than shrinking every column until + none of them can be read. + - `testing/iface-binding/` integration suite (`ci-local.sh --only iface-binding`, and a GitHub matrix leg): two daemons whose only transports are interface-bound, run against a veth pair the harness creates, downs, diff --git a/docs/reference/cli-fipstop.md b/docs/reference/cli-fipstop.md index 5291df8b..333c9fcc 100644 --- a/docs/reference/cli-fipstop.md +++ b/docs/reference/cli-fipstop.md @@ -41,7 +41,7 @@ query on its first activation and on every refresh tick while active. | --- | ----- | ----- | | **Node** | `show_status` (+ `show_listening_sockets`) | Identity, version, uptime, peer/link/session counts, sparklines for mesh size, tree depth, peer count, bytes, loss. The Traffic block on this tab is split: TUN counters on the left, the **Listening on fips0** panel on the right (see below). | | **Peers** | `show_peers` (+ `show_links`, `show_transports` cross-refs) | Authenticated peers in a table. Selecting a row and pressing Enter opens a detail view. | -| **Transports** | `show_transports` (+ `show_links`, `show_peers` cross-refs) | Tree of transport instances with per-link children when expanded. | +| **Transports** | `show_transports` (+ `show_links`, `show_peers` cross-refs) | Tree of transport instances with per-link children when expanded. An interface-bound transport also carries an interface block; see [Interface block](#interface-block-transports-tab). | | **Sessions** | `show_sessions` | End-to-end FSP sessions. | | **Tree** | `show_tree` | Spanning-tree state and per-peer coordinates. | | **Filters** | `show_bloom` | Per-peer Bloom-filter state. | @@ -128,6 +128,26 @@ the keys the current context accepts. | `e` | Expand all transports. | | `c` | Collapse all transports. | +### Interface block (Transports tab) + +A transport bound to a named interface carries an extra detail block. +It exists because the observability data shipped as JSON before it +reached this view, which left the live view reporting `up` for a +transport bound to nothing. The original OpenWrt failure was expensive +for that reason: the 802.11s link formed regardless, so nothing an +operator could see said the node was deaf. + +| Field | Meaning | +| ----- | ------- | +| Interface | The interface name the instance is bound to. | +| Presence | Whether the interface is present, and for how long. | +| Carrier | Whether a present interface has carrier. Present without carrier is a distinct state. | +| Bound to | The address bound now, or nothing while absent. | +| On absence | The policy: `required` degrades the node, `optional` does not. | +| Binds, Failed binds | Counts over the instance's life, so a flapping interface reads as churn. | + +**A transport with no interface has no block**, rather than an empty one. + ### Multi-pane scrolling tabs (Tree, Filters, Routing) Each lays out stacked panes that scroll independently. diff --git a/src/bin/fipstop/ui/snapshots.rs b/src/bin/fipstop/ui/snapshots.rs index 5aa93f72..b5d7c635 100644 --- a/src/bin/fipstop/ui/snapshots.rs +++ b/src/bin/fipstop/ui/snapshots.rs @@ -1313,3 +1313,377 @@ fn help_overlay_lists_keys() { assert!(testkit::contains_row(&buf, "quit")); assert!(testkit::contains_row(&buf, "Press ? or Esc to close")); } + +/// An interface-bound transport names its netdev in the table, so an operator +/// reading the list sees `br-lan` rather than an instance label that means +/// nothing outside the config file. +#[test] +fn transports_row_names_the_interface() { + let data = json!({ + "transports": [{ + "transport_id": 1, + "type": "ethernet", + "state": "up", + "mtu": 1497, + "name": "lan", + "interface": { + "name": "br-lan", + "presence": "present", + "carrier": true, + "policy": "required", + "since_secs": 3600, + "binds": 1, + "failed_attempts": 0 + }, + "stats": {} + }] + }); + let mut app = app_with(Tab::Transports, data); + let buf = testkit::render(80, 20, |frame, area| { + super::transports::draw(frame, &mut app, area); + }); + + assert!(testkit::contains_row(&buf, "br-lan")); + // A bound interface reads as the transport state; only the exceptional + // case displaces it, or the annotation stops being read. + assert!(testkit::contains_row(&buf, "up")); + assert!(!testkit::contains_row(&buf, "absent")); +} + +/// The narrow layout keeps the columns an operator is scanning for. +/// +/// 80x24 is the OpenWrt serial console and the xterm/tmux default. The full +/// layout's fixed columns sum to 96 against ~77 usable, and ratatui resolves +/// an over-subscribed layout by shrinking *every* column — so the overflow +/// does not clip the rightmost one, it clips all of them, and +/// `mesh0 (optional)` became `mesh0 (optio`. The marker is the one thing on +/// that row worth reading. +/// +/// This is the gap that let the regression through: the test asserting the +/// marker rendered at width 110, and the one rendering at 80 asserted only the +/// netdev name. +#[test] +fn the_optional_marker_survives_an_80_column_terminal() { + let data = json!({ + "transports": [{ + "transport_id": 2, + "type": "ethernet", + "state": "up", + "mtu": 1499, + "name": "mesh0", + "interface": { + "name": "fips-mesh0", + "presence": "absent", + "carrier": false, + "policy": "optional", + "since_secs": 252, + "binds": 0, + "failed_attempts": 0 + }, + "stats": {} + }] + }); + let mut app = app_with(Tab::Transports, data); + let buf = testkit::render(80, 20, |frame, area| { + super::transports::draw(frame, &mut app, area); + }); + + assert!( + testkit::contains_row(&buf, "mesh0 (optional)"), + "the policy marker must not be the thing that clips at 80 columns" + ); + assert!(testkit::contains_row(&buf, "fips-mesh0")); + assert!(testkit::contains_row(&buf, "absent")); +} + +/// An absent interface displaces the transport state in the State column, and +/// its severity follows the absence policy. +/// +/// `state` reads `up` from the moment the transport starts, whether or not it +/// is bound to anything, so the row would otherwise look identical to a +/// working one — the "looks healthy, reaches nothing" failure dynamic binding +/// exists to make visible. +/// +/// Optional is a warning: a dock adapter that is not plugged in, or a radio +/// this board never had, is the case `optional: true` was added to describe, +/// and the daemon stays `Full` for it. Red there would train the operator to +/// ignore red. +#[test] +fn an_absent_optional_interface_warns() { + let data = json!({ + "transports": [{ + "transport_id": 2, + "type": "ethernet", + "state": "up", + "mtu": 1499, + "name": "mesh0", + "interface": { + "name": "fips-mesh0", + "presence": "absent", + "carrier": false, + "policy": "optional", + "since_secs": 252, + "binds": 0, + "failed_attempts": 0 + }, + "stats": {} + }] + }); + let mut app = app_with(Tab::Transports, data); + let buf = testkit::render(110, 20, |frame, area| { + super::transports::draw(frame, &mut app, area); + }); + + assert!(testkit::contains_row(&buf, "fips-mesh0")); + // Policy rides with the instance name, and only the exception prints. + assert!(testkit::contains_row(&buf, "mesh0 (optional)")); + // The State column carries presence, because `up` is what it would + // otherwise say about an interface that has never existed. + assert!(testkit::contains_row(&buf, "absent")); + assert_eq!( + testkit::fg_at(&buf, "fips-mesh0"), + Some(ratatui::style::Color::Yellow), + "an interface whose absence is normal is a warning, not an error" + ); +} + +/// A required interface being absent is an error. +/// +/// Naming an interface without `optional: true` is a statement that you expect +/// it, and the daemon reports `Degraded` while it is missing. The view has to +/// agree, or the colour stops carrying the same meaning as the health state. +#[test] +fn an_absent_required_interface_errors() { + let data = json!({ + "transports": [{ + "transport_id": 2, + "type": "ethernet", + "state": "up", + "mtu": 1499, + "name": "wan", + "interface": { + "name": "eth0", + "presence": "absent", + "carrier": false, + "policy": "required", + "since_secs": 30, + "binds": 0, + "failed_attempts": 0 + }, + "stats": {} + }] + }); + let mut app = app_with(Tab::Transports, data); + let buf = testkit::render(110, 20, |frame, area| { + super::transports::draw(frame, &mut app, area); + }); + + // `required` is the default and is spelled by omission — a column of it + // on nearly every row would be a column of noise. + assert!(!testkit::contains_row(&buf, "required")); + assert!(!testkit::contains_row(&buf, "(optional)")); + assert!(testkit::contains_row(&buf, "wan")); + assert_eq!( + testkit::fg_at(&buf, "eth0"), + Some(ratatui::style::Color::Red), + "an interface the config says to expect is an error while it is gone" + ); +} + +/// The detail pane reports presence, carrier, policy and bind counts — +/// everything `state` cannot say. +#[test] +fn transport_detail_reports_interface_presence() { + let data = json!({ + "transports": [{ + "transport_id": 3, + "type": "ethernet", + "state": "up", + "mtu": 1497, + "name": "wan", + "interface": { + "name": "eth0", + "presence": "present", + "carrier": false, + "policy": "required", + "since_secs": 90, + "binds": 3, + "failed_attempts": 2 + }, + "stats": {} + }] + }); + let mut app = app_with(Tab::Transports, data); + app.detail_view = Some(crate::app::DetailView { scroll: 0 }); + let buf = testkit::render(120, 30, |frame, area| { + super::transports::draw(frame, &mut app, area); + }); + + assert!(testkit::contains_row(&buf, "Interface")); + assert!(testkit::contains_row(&buf, "eth0")); + assert!(testkit::contains_row(&buf, "present")); + // Carrier is reported rather than acted on, so "bound with no carrier" — + // a bridge with nothing plugged into it — has to be legible as its own + // state rather than inferred from silence. + assert!(testkit::contains_row(&buf, "Carrier")); + // A rebind count and a failed-bind count distinguish an interface that is + // flapping from one that is there and refusing. + assert!(testkit::contains_row(&buf, "rebound")); + assert!(testkit::contains_row(&buf, "Failed binds")); +} + +/// Instance names and interfaces line up down the list. +/// +/// Packed into one label they did not: `ethernet dongle2 en25` over +/// `ethernet wifi en0` left the netdev names ragged, which is the column an +/// operator scans to find the interface they are looking for. Separate facts, +/// separate columns. +#[test] +fn transports_columns_align_across_mixed_types() { + let data = json!({ + "transports": [ + { + "transport_id": 1, "type": "udp", "state": "up", "mtu": 1472, + "local_addr": "0.0.0.0:2121", "stats": {} + }, + { + "transport_id": 2, "type": "tcp", "state": "up", "mtu": 1400, + "local_addr": "0.0.0.0:8443", "stats": {} + }, + { + "transport_id": 3, "type": "ethernet", "state": "up", "mtu": 1497, + "name": "dongle2", + "interface": { + "name": "en25", "presence": "absent", "carrier": false, + "policy": "required", "since_secs": 12, "binds": 0, + "failed_attempts": 0 + }, + "stats": {} + }, + { + "transport_id": 4, "type": "ethernet", "state": "up", "mtu": 1497, + "name": "wifi", + "interface": { + "name": "en0", "presence": "present", "carrier": true, + "policy": "optional", "since_secs": 900, "binds": 1, + "failed_attempts": 0 + }, + "stats": {} + } + ] + }); + let mut app = app_with(Tab::Transports, data); + let buf = testkit::render(100, 20, |frame, area| { + super::transports::draw(frame, &mut app, area); + }); + + let col_of = |needle: &str| testkit::find(&buf, needle).map(|(x, _)| x); + + // The instance column starts at one x for every row that has one. + assert_eq!(col_of("dongle2"), col_of("wifi")); + // ... and so does the interface column, across long and short names and + // across transports that name a netdev and ones that name a socket. + assert_eq!(col_of("en25"), col_of("en0")); + assert_eq!(col_of("en25"), col_of("0.0.0.0:2121")); + assert_eq!(col_of("0.0.0.0:2121"), col_of("0.0.0.0:8443")); + + // The header sits over the column it names. + assert_eq!(col_of("Bound to"), col_of("en25")); + assert_eq!(col_of("Instance"), col_of("dongle2")); +} + +/// A link's remote address shares the `Bound to` column with its parent's +/// interface. +/// +/// Same question — what is this attached to — so the same column: a netdev for +/// the transport, a remote endpoint for the link. Keeping a full MAC out of +/// the first column is also what lets the three identifying columns sit +/// against the left edge rather than being pushed right by the widest link +/// row. +#[test] +fn a_link_puts_its_remote_address_in_the_bound_to_column() { + let transports = json!({ + "transports": [{ + "transport_id": 3, "type": "ethernet", "state": "up", "mtu": 1497, + "name": "wifi", + "interface": { + "name": "en0", "presence": "present", "carrier": true, + "policy": "optional", "since_secs": 60, "binds": 1, + "failed_attempts": 0 + }, + "stats": {} + }] + }); + let links = json!({ + "links": [{ + "link_id": 1, "transport_id": 3, "direction": "Outbound", + "remote_addr": "aa:bb:cc:dd:ee:ff", "state": "connected" + }] + }); + + let mut app = app_with(Tab::Transports, transports); + app.data.insert(Tab::Links, links); + app.expanded_transports.insert(3); + + let buf = testkit::render(110, 20, |frame, area| { + super::transports::draw(frame, &mut app, area); + }); + + let col_of = |needle: &str| testkit::find(&buf, needle).map(|(x, _)| x); + + // A right-aligned State clips from the left on overflow, so `connected` + // reading as `onnected` is a width bug rather than an honest truncation. + assert!(testkit::contains_row(&buf, "connected")); + + // The MAC is not truncated and sits under the same header as the netdev. + assert!(testkit::contains_row(&buf, "aa:bb:cc:dd:ee:ff")); + assert_eq!(col_of("aa:bb:cc:dd:ee:ff"), col_of("en0")); + assert_eq!(col_of("Bound to"), col_of("en0")); + + // The link keeps its direction and tree glyph in the first column, which + // is therefore narrow enough to leave the identifying columns at the left. + assert!(testkit::contains_row(&buf, "Out")); + // Packed at the left rather than floating in the middle. With the first + // column flexible, as it was, it absorbed every spare column of a wide + // terminal and pushed these two past the halfway mark. + assert!(col_of("Instance").unwrap() <= 20); + assert!( + col_of("Bound to").unwrap() < 45, + "the identifying columns must stay against the left edge" + ); +} + +/// `State` is right-aligned, so the values share a right edge and the column +/// reads as a status strip rather than as ragged text. +#[test] +fn the_state_column_is_right_aligned() { + let data = json!({ + "transports": [ + { + "transport_id": 1, "type": "udp", "state": "up", "mtu": 1472, + "local_addr": "0.0.0.0:2121", "stats": {} + }, + { + "transport_id": 2, "type": "ethernet", "state": "up", "mtu": 1499, + "name": "mesh0", + "interface": { + "name": "fips-mesh0", "presence": "absent", "carrier": false, + "policy": "optional", "since_secs": 10, "binds": 0, + "failed_attempts": 0 + }, + "stats": {} + } + ] + }); + let mut app = app_with(Tab::Transports, data); + let buf = testkit::render(110, 20, |frame, area| { + super::transports::draw(frame, &mut app, area); + }); + + let end_of = |needle: &str| testkit::find(&buf, needle).map(|(x, _)| x + needle.len() as u16); + + // Same right edge for a two-character value and a six-character one, and + // the header shares it. + assert_eq!(end_of("up"), end_of("absent")); + assert_eq!(end_of("up"), end_of("State")); +} diff --git a/src/bin/fipstop/ui/transports.rs b/src/bin/fipstop/ui/transports.rs index bc42b958..b6c50da8 100644 --- a/src/bin/fipstop/ui/transports.rs +++ b/src/bin/fipstop/ui/transports.rs @@ -1,5 +1,5 @@ use ratatui::Frame; -use ratatui::layout::{Constraint, Layout, Rect}; +use ratatui::layout::{Alignment, Constraint, Layout, Rect}; use ratatui::style::{Color, Modifier, Style}; use ratatui::text::{Line, Span}; use ratatui::widgets::{ @@ -23,6 +23,21 @@ enum TreeRow { }, } +/// Below this width the table drops its byte counters and keeps its +/// identifying columns. +/// +/// The full layout's fixed columns sum to 90, plus six single-column gaps — +/// 96 against the ~77 usable inside an 80-column terminal's border and +/// scrollbar. Ratatui resolves an over-subscribed layout by shrinking every +/// column, so the overflow does not clip the rightmost column, it clips *all* +/// of them: `mesh0 (optional)` becomes `mesh0 (optio`, losing the one marker +/// on the row worth reading. +const NARROW_TABLE_WIDTH: u16 = 100; + +/// Below this width the detail view stacks above/below the table instead of +/// beside it. +const SIDE_BY_SIDE_MIN_WIDTH: u16 = 110; + pub fn draw(frame: &mut Frame, app: &mut App, area: Rect) { let transports = get_transports(app); let links = get_links(app); @@ -33,8 +48,18 @@ pub fn draw(frame: &mut Frame, app: &mut App, area: Rect) { update_selected_tree_item(app, &tree_rows); if app.detail_view.is_some() { - let chunks = Layout::horizontal([Constraint::Percentage(40), Constraint::Percentage(60)]) - .split(area); + // Side by side only where the table half can still show its + // identifying columns. At 80 columns — the OpenWrt serial console, and + // the xterm/tmux default — a 40% split leaves the table 32 columns for + // a layout that needs 65 even in its narrow form, and ratatui resolves + // that by giving the trailing columns everything and rendering + // Transport, Instance, Bound-to and State at width zero. Stacking + // keeps both panes readable instead of keeping both unreadable. + let chunks = if area.width < SIDE_BY_SIDE_MIN_WIDTH { + Layout::vertical([Constraint::Percentage(50), Constraint::Percentage(50)]).split(area) + } else { + Layout::horizontal([Constraint::Percentage(40), Constraint::Percentage(60)]).split(area) + }; draw_table(frame, app, chunks[0], &transports, &links, &tree_rows); draw_detail(frame, app, chunks[1], &transports, &links, &tree_rows); @@ -113,6 +138,19 @@ fn update_selected_tree_item(app: &mut App, tree_rows: &[TreeRow]) { }; } +/// Build a row, dropping the trailing byte counters in the narrow layout. +/// +/// Both row shapes carry the same seven cells in the same order, so the +/// narrow variant is the same list with its tail cut — keeping one place +/// where the column count is decided, rather than two that must agree. +fn table_row<'a>(cells: Vec>, narrow: bool) -> Row<'a> { + let mut cells = cells; + if narrow { + cells.truncate(5); + } + Row::new(cells) +} + fn draw_table( frame: &mut Frame, app: &mut App, @@ -121,14 +159,24 @@ fn draw_table( links: &[serde_json::Value], tree_rows: &[TreeRow], ) { - let header = Row::new(vec![ + // Tx/Rx are the first thing to go when width is short: they are the only + // columns whose absence costs nothing an operator is scanning this table + // to find, and the detail pane carries them in full. + let narrow = area.width < NARROW_TABLE_WIDTH; + + let mut header_cells = vec![ Cell::from("Transport / Link"), - Cell::from("State"), + Cell::from("Instance"), + Cell::from("Bound to"), + Cell::from(Line::from("State").alignment(Alignment::Right)), Cell::from("Peer"), Cell::from("Tx"), Cell::from("Rx"), - ]) - .style( + ]; + if narrow { + header_cells.truncate(5); + } + let header = Row::new(header_cells).style( Style::default() .fg(Color::Yellow) .add_modifier(Modifier::BOLD), @@ -153,29 +201,72 @@ fn draw_table( let typ = helpers::str_field(t, "type"); let name = t.get("name").and_then(|v| v.as_str()).unwrap_or(""); let addr = t.get("local_addr").and_then(|v| v.as_str()).unwrap_or(""); - let label = if !name.is_empty() { - format!("{indicator}{typ} {name}") - } else if typ == "tor" { + // An interface-bound transport is identified by the netdev it + // names, not by its instance label: "ethernet lan" tells an + // operator nothing, "ethernet lan br-lan" tells them where to + // look. The presence marker is what makes the row honest — + // `state` reads `up` from the moment the transport starts, + // whether or not it is bound to anything. + let iface = t.get("interface"); + let iface_name = iface + .and_then(|i| i.get("name")) + .and_then(|v| v.as_str()) + .unwrap_or(""); + let presence = iface + .and_then(|i| i.get("presence")) + .and_then(|v| v.as_str()) + .unwrap_or(""); + let policy = iface + .and_then(|i| i.get("policy")) + .and_then(|v| v.as_str()) + .unwrap_or(""); + + // Three cells, not one packed string. The instance name and + // the thing the transport is bound to are separate facts about + // separate columns of a table, and running them together left + // the netdev names ragged down the list — the column an + // operator scans to find the interface they are looking for. + let label = if typ == "tor" { let mode = t .get("tor_mode") .and_then(|v| v.as_str()) .unwrap_or("socks5"); - let onion_hint = t - .get("onion_address") + format!("{indicator}tor({mode})") + } else { + format!("{indicator}{typ}") + }; + + // What this transport is attached to: a netdev for the + // interface-bound ones, the bound socket address for IP + // transports, an onion for tor. Different answers, one + // question, so one column. + let bound_to = if !iface_name.is_empty() { + iface_name.to_string() + } else if typ == "tor" { + t.get("onion_address") .and_then(|v| v.as_str()) .map(|a| { let short = if a.len() > 16 { &a[..16] } else { a }; - format!(" {short}..") + format!("{short}..") }) - .unwrap_or_default(); - format!("{indicator}tor({mode}){onion_hint}") + .unwrap_or_default() } else if !addr.is_empty() { - format!("{indicator}{typ} {addr}") + addr.to_string() } else { - format!("{indicator}{typ} #{transport_id}") + format!("#{transport_id}") }; - let state = helpers::str_field(t, "state"); + // The State column carries presence for an interface-bound + // transport, not the lifecycle state. `up` is true from the + // moment the transport starts and stays true while its + // interface is missing, so it is precisely the wrong answer in + // the one case an operator is scanning this column for. There + // is no room to show both, and only one of them is news. + let state = if presence.is_empty() || presence == "present" { + helpers::str_field(t, "state") + } else { + presence + }; let tx = t .get("stats") .and_then(|s| s.get("packets_sent").or_else(|| s.get("frames_sent"))) @@ -189,14 +280,45 @@ fn draw_table( .map(|n| n.to_string()) .unwrap_or_else(|| "-".into()); - Row::new(vec![ - Cell::from(label), - Cell::from(state.to_string()), - Cell::from(""), - Cell::from(tx), - Cell::from(rx), - ]) - .style(Style::default().fg(Color::White)) + // Colour follows bindability, not lifecycle: an absent + // interface is the case the operator most needs to spot, and + // it is precisely the one `state` cannot show. Severity then + // follows the absence policy, because that is what the policy + // means — a dock adapter that is not plugged in is a warning, + // an interface the config says to expect is an error. Same + // split the daemon makes between `Degraded` and silence. + let row_style = match presence { + "" | "present" => Style::default().fg(Color::White), + "binding" => Style::default().fg(Color::Yellow), + _ if policy == "optional" => Style::default().fg(Color::Yellow), + _ => Style::default().fg(Color::Red), + }; + + // Policy rides with the instance name rather than owning a + // column: `required` is the default and appears on nearly + // every row, so a column of it is a column of noise. Only the + // exception is worth printing, and its absence then means the + // rule. + let instance = match (name.is_empty(), policy == "optional") { + (true, true) => "(optional)".to_string(), + (true, false) => String::new(), + (false, true) => format!("{name} (optional)"), + (false, false) => name.to_string(), + }; + + table_row( + vec![ + Cell::from(label), + Cell::from(instance), + Cell::from(bound_to), + Cell::from(Line::from(state.to_string()).alignment(Alignment::Right)), + Cell::from(""), + Cell::from(tx), + Cell::from(rx), + ], + narrow, + ) + .style(row_style) } TreeRow::Link { index, is_last } => { let link = &links[*index]; @@ -214,28 +336,42 @@ fn draw_table( // Wide enough to render a full MAC (~17) or `hci0/MAC` // (~22) without chopping mid-octet; the link detail view // shows the untruncated address. + // The remote address goes in `Bound to`, not in the label. A + // link is bound to a remote endpoint exactly as a transport is + // bound to a netdev or a socket — same question, same column — + // and keeping a full MAC out of the first column is what lets + // the three left columns sit against the left edge instead of + // being pushed right by the widest link row. let addr = helpers::truncate_hex(helpers::str_field(link, "remote_addr"), 24); - let label = format!(" {tree_char} {dir_short} {addr}"); + let label = format!(" {tree_char} {dir_short}"); let state = helpers::str_field(link, "state"); let peer_name = lookup_peer_for_link(app, link) .map(|p| helpers::str_field(&p, "display_name").to_string()) .unwrap_or_default(); - Row::new(vec![ - Cell::from(Span::styled( - label, - Style::default().fg(if dir == "Outbound" { - Color::Cyan - } else { - Color::Green - }), - )), - Cell::from(state.to_string()), - Cell::from(peer_name), - Cell::from(""), - Cell::from(""), - ]) + table_row( + vec![ + Cell::from(Span::styled( + label, + Style::default().fg(if dir == "Outbound" { + Color::Cyan + } else { + Color::Green + }), + )), + // A link has no instance name of its own — it inherits its + // parent transport's, shown one row up — and no absence + // policy, which is a property of an interface. + Cell::from(""), + Cell::from(addr), + Cell::from(Line::from(state.to_string()).alignment(Alignment::Right)), + Cell::from(peer_name), + Cell::from(""), + Cell::from(""), + ], + narrow, + ) } }) .collect(); @@ -251,15 +387,44 @@ fn draw_table( format!(" Transports ({transport_count}) ") }; - let widths = [ - Constraint::Min(28), - Constraint::Length(12), - Constraint::Length(14), - Constraint::Length(9), - Constraint::Length(9), + // Every identifying column is fixed-width and packed against the left + // edge; `Peer` takes the slack. The first column used to be `Min`, which + // meant it absorbed all spare width and shoved Instance and Bound-to into + // the middle of the terminal, away from the names an operator is scanning. + // + // It is sized for the widest label that lives in it — a link's + // ` └─ Out` — rather than for a full MAC, because the address moved to + // `Bound to` where it belongs. `Bound to` is sized for a MAC (17), which + // also covers every netdev name and socket address that shares it. + // Narrow: the same columns, sized down to what still reads. Instance keeps + // 18 because `mesh0 (optional)` is 16 and the marker is the point; `Bound + // to` keeps 17 because that is a full MAC. + let widths: &[Constraint] = if narrow { + &[ + Constraint::Length(12), // Transport / Link + Constraint::Length(18), // Instance, plus "(optional)" + Constraint::Length(17), // Bound to: a full MAC + Constraint::Length(9), // State, right-aligned + Constraint::Min(6), // Peer — takes the slack + ] + } else { + &FULL_WIDTHS + }; + + const FULL_WIDTHS: [Constraint; 7] = [ + Constraint::Length(18), // Transport / Link + Constraint::Length(20), // Instance, plus "(optional)" where it applies + Constraint::Length(18), // Bound to: netdev, socket addr, onion, MAC + // Wide enough for the longest value that lands here — `connected` (9) + // and `binding` — because a right-aligned cell clips from the LEFT, + // so an overflow reads as `onnected` rather than as a truncation. + Constraint::Length(10), // State, right-aligned + Constraint::Min(10), // Peer — takes the slack + Constraint::Length(7), // Tx + Constraint::Length(7), // Rx ]; - let table = Table::new(rows, widths) + let table = Table::new(rows, widths.to_vec()) .header(header) .block(Block::default().borders(Borders::ALL).title(title)) .row_highlight_style( @@ -341,6 +506,73 @@ fn draw_transport_detail(frame: &mut Frame, app: &App, area: Rect, t: &serde_jso lines.push(helpers::kv_line("Local Addr", addr)); } + // Interface presence, for the transports that are bound to a netdev. + // + // `State` above answers a lifecycle question — was this transport started + // — and reads `up` for an interface that has never existed. That gap is + // the whole reason interface binding is observable at all: the original + // OpenWrt bug was expensive because the 802.11s link formed regardless, so + // nothing an operator could see said the node was deaf. This is where they + // see it. + if let Some(iface) = t.get("interface") { + lines.push(Line::from("")); + lines.push(helpers::section_header("Interface")); + lines.push(helpers::kv_line( + "Interface", + helpers::str_field(iface, "name"), + )); + + let presence = helpers::str_field(iface, "presence"); + let since = iface + .get("since_secs") + .and_then(|v| v.as_u64()) + .map(|secs| helpers::format_duration_ms(secs.saturating_mul(1000))) + .unwrap_or_else(|| "-".into()); + lines.push(helpers::kv_line( + "Presence", + &format!("{presence} for {since}"), + )); + + // Carrier is reported, never acted on: presence is IFF_UP, so a bound + // interface with no carrier is normal (a bridge with nothing plugged + // into it) rather than a fault. Saying so beats an operator inferring + // it from silence. + let carrier = iface + .get("carrier") + .and_then(|v| v.as_bool()) + .map(|c| if c { "yes" } else { "no" }) + .unwrap_or("-"); + lines.push(helpers::kv_line("Carrier", carrier)); + + // The list marks only the exception, `(optional)`, beside the + // instance name. The detail pane has room to spell out both, as the + // consequence rather than the config key: `optional` is a statement + // about what absence *means*, and someone who has opened this pane + // wants the meaning. + let policy = helpers::str_field(iface, "policy"); + let absence = if policy == "optional" { + "optional (absence is normal)" + } else { + "required (absence degrades the node)" + }; + lines.push(helpers::kv_line("On absence", absence)); + + // Binds past the first are rebinds, and a climbing failed-attempt + // count is an interface that is there and refusing — a different + // problem from one that is missing, and invisible without this. + let binds = iface.get("binds").and_then(|v| v.as_u64()).unwrap_or(0); + if binds > 1 { + lines.push(helpers::kv_line("Binds", &format!("{binds} (rebound)"))); + } else { + lines.push(helpers::kv_line("Binds", &binds.to_string())); + } + if let Some(failed) = iface.get("failed_attempts").and_then(|v| v.as_u64()) + && failed > 0 + { + lines.push(helpers::kv_line("Failed binds", &failed.to_string())); + } + } + // Tor-specific info if let Some(mode) = t.get("tor_mode").and_then(|v| v.as_str()) { lines.push(helpers::kv_line("Tor Mode", mode)); diff --git a/src/control/queries.rs b/src/control/queries.rs index 2bdccd36..df8eefc8 100644 --- a/src/control/queries.rs +++ b/src/control/queries.rs @@ -2591,6 +2591,8 @@ mod tests { "idle_ms", "first_seen_secs_ago", "last_contact_secs_ago", + // Interface presence: elapsed since the current phase began. + "since_secs", ]; /// Build a Node with a fixed identity, default config, and empty @@ -2722,6 +2724,56 @@ mod tests { // ---- 19 handler snapshot tests -------------------------------------- + /// The `interface` block, which `build_test_node` cannot produce: it + /// keeps every transport list empty, so the nineteen snapshots above pin + /// `show_transports` only in its empty form. The block is emitted by two + /// hand-duplicated sites (the live handler and the read-handle variant) + /// that agree today with nothing enforcing it, and the control-socket + /// reference states the response schema is pinned by these snapshots. + /// + /// A separate node rather than a richer `build_test_node`, so the other + /// snapshots keep their empty-state determinism. + #[cfg(any(target_os = "linux", target_os = "macos"))] + #[tokio::test] + async fn snapshot_show_transports_with_interface() { + use crate::config::EthernetConfig; + use crate::transport::ethernet::EthernetTransport; + use crate::transport::{TransportHandle, TransportId}; + + let mut node = build_test_node(); + + // An interface no host has, so presence is deterministically absent + // and carrier deterministically false on every machine this runs on. + let config = EthernetConfig { + interface: "fips-absent-x0".to_string(), + ethertype: None, + mtu: None, + recv_buf_size: None, + send_buf_size: None, + listen: Some(true), + announce: Some(false), + auto_connect: None, + accept_connections: None, + beacon_interval_secs: None, + optional: Some(false), + }; + let (tx, _rx) = crate::transport::packet_channel(8); + let mut eth = EthernetTransport::new(TransportId::new(1), Some("lab".into()), config, tx); + + // Started, because "up with its interface absent" is the state an + // operator actually meets — and the one whose shape is new here. + eth.start_async() + .await + .expect("absence is not a start failure"); + + node.insert_transport_for_test(TransportId::new(1), TransportHandle::Ethernet(eth)); + + assert_snapshot( + "show_transports_with_interface", + &render(show_transports(&node)), + ); + } + #[test] fn snapshot_show_status() { let node = build_test_node(); diff --git a/src/control/snapshots/show_transports_with_interface.json b/src/control/snapshots/show_transports_with_interface.json new file mode 100644 index 00000000..205ed5e2 --- /dev/null +++ b/src/control/snapshots/show_transports_with_interface.json @@ -0,0 +1,36 @@ +{ + "data": { + "transports": [ + { + "interface": { + "binds": 0, + "carrier": false, + "failed_attempts": 0, + "name": "fips-absent-x0", + "policy": "required", + "presence": "absent", + "since_secs": "" + }, + "mtu": 1499, + "name": "lab", + "state": "up", + "stats": { + "beacons_dropped": 0, + "beacons_recv": 0, + "beacons_sent": 0, + "bytes_recv": 0, + "bytes_sent": 0, + "frames_recv": 0, + "frames_sent": 0, + "frames_too_long": 0, + "frames_too_short": 0, + "recv_errors": 0, + "send_errors": 0 + }, + "transport_id": 1, + "type": "ethernet" + } + ] + }, + "status": "ok" +} \ No newline at end of file diff --git a/src/node/mod.rs b/src/node/mod.rs index 8fb65d94..2610aa3a 100644 --- a/src/node/mod.rs +++ b/src/node/mod.rs @@ -181,6 +181,21 @@ pub enum NodeError { NoOperationalTransports, } +impl Node { + /// Test-only: place a transport into the node's map directly. + /// + /// The snapshot tests live in `crate::control` and so cannot reach the + /// private `transports` field, but the interface-presence block they need + /// to pin only exists on a real interface-bound transport. Mirrors + /// `isolate_peer_acl_for_test`: a narrow hook, so the fixture stays honest + /// rather than the snapshot being hand-authored JSON that nothing + /// produces. + #[cfg(all(test, any(target_os = "linux", target_os = "macos")))] + pub(crate) fn insert_transport_for_test(&mut self, id: TransportId, handle: TransportHandle) { + self.transports.insert(id, handle); + } +} + impl NodeError { /// Whether this failure is expected to clear on its own. ///