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. ///