diff --git a/src/bin/fipstop/ui/snapshots.rs b/src/bin/fipstop/ui/snapshots.rs index 5aa93f72..60b10dd2 100644 --- a/src/bin/fipstop/ui/snapshots.rs +++ b/src/bin/fipstop/ui/snapshots.rs @@ -1313,3 +1313,331 @@ 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")); +} + +/// 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..331e51b7 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::{ @@ -123,7 +123,9 @@ fn draw_table( ) { let header = Row::new(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"), @@ -153,29 +155,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 +234,42 @@ fn draw_table( .map(|n| n.to_string()) .unwrap_or_else(|| "-".into()); + // 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(), + }; + Row::new(vec![ Cell::from(label), - Cell::from(state.to_string()), + 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), ]) - .style(Style::default().fg(Color::White)) + .style(row_style) } TreeRow::Link { index, is_last } => { let link = &links[*index]; @@ -214,8 +287,14 @@ 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) @@ -231,7 +310,12 @@ fn draw_table( Color::Green }), )), - Cell::from(state.to_string()), + // 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(""), @@ -251,12 +335,26 @@ fn draw_table( format!(" Transports ({transport_count}) ") }; + // 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. let widths = [ - Constraint::Min(28), - Constraint::Length(12), - Constraint::Length(14), - Constraint::Length(9), - Constraint::Length(9), + 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) @@ -341,6 +439,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));