From fbc4f6e3981371f78724afac4619017e1d95a9ef Mon Sep 17 00:00:00 2001 From: Stan Grams Date: Thu, 6 Aug 2026 21:02:26 +0200 Subject: [PATCH] [feat](trx-config): add a resolved-config validation phase Some things can only be checked once CLI overrides have been folded in and the rig/remote lists are final, so nothing checked them at all: - The client's per-rig maps (rigctl.rig_ports, audio.rig_urls, audio.rig_ports, decode_history_retention_min_by_rig, http.default_rig_name) are keyed by a remote's short name. A typo used to spawn a rigctl listener that injected a rig_id no remote answered to, without a word in the log. - Nothing noticed two listeners claiming one socket. [listen].port and a rig's [audio].port could both be 4530; on the client, http, http_json and each rigctl rig port could collide freely. Add validate_resolved() to both configs, run after argument parsing, plus a shared socket-conflict check that treats a wildcard address as conflicting with any address on the same port and ignores port 0. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SyX26FCpMQxiBoC7r5K1A7 Signed-off-by: Stan Grams --- src/trx-client/src/main.rs | 26 +++++- src/trx-config/src/client.rs | 167 ++++++++++++++++++++++++++++++++++- src/trx-config/src/server.rs | 85 +++++++++++++++++- src/trx-config/src/shared.rs | 87 ++++++++++++++++++ src/trx-server/src/main.rs | 29 ++++++ 5 files changed, 391 insertions(+), 3 deletions(-) diff --git a/src/trx-client/src/main.rs b/src/trx-client/src/main.rs index 5cef2e98..ac42fe5b 100644 --- a/src/trx-client/src/main.rs +++ b/src/trx-client/src/main.rs @@ -153,7 +153,7 @@ async fn async_init() -> DynResult { } loaded.report_unknown_keys(cli.strict_config)?; - let cfg = loaded.config; + let mut cfg = loaded.config; cfg.validate() .map_err(|e| format!("Invalid client configuration: {}", e))?; @@ -269,6 +269,30 @@ async fn async_init() -> DynResult { .http_json_listen .unwrap_or(cfg.frontends.http_json.listen); let http_json_port = cli.http_json_port.unwrap_or(cfg.frontends.http_json.port); + + // Fold the CLI overrides back into the config so validation and the + // frontends agree on what is about to be bound. + cfg.frontends.http.enabled = frontends.iter().any(|f| f == "http"); + cfg.frontends.rigctl.enabled = frontends.iter().any(|f| f == "rigctl"); + cfg.frontends.http_json.enabled = frontends.iter().any(|f| f == "httpjson"); + cfg.frontends.http.listen = http_listen; + cfg.frontends.http.port = http_port; + cfg.frontends.rigctl.listen = rigctl_listen; + cfg.frontends.http_json.listen = http_json_listen; + cfg.frontends.http_json.port = http_json_port; + + // Second validation phase: the per-rig maps are keyed by remote short name, + // so they can only be checked once the remote list is final. + if cli.url.is_none() { + cfg.validate_resolved(&resolved_remotes) + .map_err(|e| format!("Invalid client configuration: {}", e))?; + } else { + // --url replaces the configured remotes outright, so only the socket + // checks still apply. + trx_config::shared::check_socket_conflicts(&cfg.bound_sockets()) + .map_err(|e| format!("Invalid client configuration: {}", e))?; + } + let callsign = cli .callsign .clone() diff --git a/src/trx-config/src/client.rs b/src/trx-config/src/client.rs index 24c6ff82..8edb50f2 100644 --- a/src/trx-config/src/client.rs +++ b/src/trx-config/src/client.rs @@ -18,7 +18,7 @@ use std::time::Duration; use serde::{Deserialize, Serialize}; use crate::file::{ConfigError, ConfigFile, ConfigLoad}; -use crate::shared::{validate_log_level, validate_tokens}; +use crate::shared::{check_socket_conflicts, validate_log_level, validate_tokens, BoundSocket}; /// Top-level client configuration structure. #[derive(Debug, Clone, Default, Serialize, Deserialize)] @@ -412,6 +412,78 @@ impl ClientConfig { } } + /// Checks that only make sense once the remote list is known. + /// + /// `remotes` is the CLI-merged result of `resolved_remotes()`, so this runs + /// after argument parsing rather than at load time. Every per-rig map is + /// keyed by a remote's short name; a typo used to spawn a listener routing + /// to a rig that does not exist, in silence. + pub fn validate_resolved(&self, remotes: &[RemoteEntry]) -> Result<(), String> { + let names: std::collections::HashSet<&str> = + remotes.iter().map(|r| r.name.as_str()).collect(); + let known = || { + let mut names: Vec<&str> = remotes.iter().map(|r| r.name.as_str()).collect(); + names.sort_unstable(); + names.join(", ") + }; + let check = |value: &str, path: &str| -> Result<(), String> { + if names.contains(value) { + return Ok(()); + } + Err(format!( + "{path} refers to unknown remote \"{value}\" (configured remotes: {})", + known() + )) + }; + + if let Some(name) = &self.frontends.http.default_rig_name { + check(name, "[frontends.http].default_rig_name")?; + } + for name in self.frontends.rigctl.rig_ports.keys() { + check(name, "[frontends.rigctl].rig_ports")?; + } + for name in self.frontends.audio.rig_urls.keys() { + check(name, "[frontends.audio].rig_urls")?; + } + for name in self.frontends.audio.rig_ports.keys() { + check(name, "[frontends.audio].rig_ports")?; + } + for name in self.frontends.http.decode_history_retention_min_by_rig.keys() { + check(name, "[frontends.http].decode_history_retention_min_by_rig")?; + } + + check_socket_conflicts(&self.bound_sockets()) + } + + /// Sockets the enabled frontends will bind. + pub fn bound_sockets(&self) -> Vec { + let mut sockets = Vec::new(); + if self.frontends.http.enabled { + sockets.push(BoundSocket::new( + self.frontends.http.listen, + self.frontends.http.port, + "[frontends.http]", + )); + } + if self.frontends.http_json.enabled { + sockets.push(BoundSocket::new( + self.frontends.http_json.listen, + self.frontends.http_json.port, + "[frontends.http_json]", + )); + } + if self.frontends.rigctl.enabled { + for (name, port) in &self.frontends.rigctl.rig_ports { + sockets.push(BoundSocket::new( + self.frontends.rigctl.listen, + *port, + format!("[frontends.rigctl].rig_ports.{name}"), + )); + } + } + sockets + } + pub fn validate(&self) -> Result<(), String> { validate_log_level(self.general.log_level.as_deref())?; @@ -1203,6 +1275,99 @@ url = "remote.example.com:4530" .contains("poll_interval_ms must be > 0")); } + // --- Second-phase validation against the resolved remote list --- + + fn remotes(names: &[&str]) -> Vec { + names + .iter() + .map(|name| RemoteEntry { + name: name.to_string(), + url: "127.0.0.1:4530".to_string(), + rig_id: None, + auth: RemoteAuthConfig::default(), + poll_interval_ms: 750, + }) + .collect() + } + + #[test] + fn test_validate_resolved_accepts_matching_rig_names() { + let mut config = ClientConfig::default(); + config.frontends.http.default_rig_name = Some("hf".to_string()); + config + .frontends + .audio + .rig_ports + .insert("vhf".to_string(), 4532); + assert!(config.validate_resolved(&remotes(&["hf", "vhf"])).is_ok()); + } + + #[test] + fn test_validate_resolved_rejects_unknown_rigctl_rig() { + let mut config = ClientConfig::default(); + config.frontends.rigctl.enabled = true; + config + .frontends + .rigctl + .rig_ports + .insert("typo".to_string(), 4532); + let err = config.validate_resolved(&remotes(&["hf"])).unwrap_err(); + assert!( + err.contains("rig_ports") && err.contains("typo") && err.contains("hf"), + "unexpected error: {err}" + ); + } + + #[test] + fn test_validate_resolved_rejects_unknown_default_rig() { + let mut config = ClientConfig::default(); + config.frontends.http.default_rig_name = Some("nope".to_string()); + assert!(config.validate_resolved(&remotes(&["hf"])).is_err()); + } + + #[test] + fn test_validate_resolved_rejects_unknown_retention_rig() { + let mut config = ClientConfig::default(); + config + .frontends + .http + .decode_history_retention_min_by_rig + .insert("ghost".to_string(), 60); + assert!(config.validate_resolved(&remotes(&["hf"])).is_err()); + } + + #[test] + fn test_validate_resolved_rejects_port_collision() { + let mut config = ClientConfig::default(); + config.frontends.http.port = 8080; + config.frontends.http_json.enabled = true; + config.frontends.http_json.port = 8080; + let err = config.validate_resolved(&remotes(&["hf"])).unwrap_err(); + assert!(err.contains("8080"), "unexpected error: {err}"); + } + + #[test] + fn test_validate_resolved_rejects_rigctl_colliding_with_http() { + let mut config = ClientConfig::default(); + config.frontends.http.port = 8080; + config.frontends.rigctl.enabled = true; + config + .frontends + .rigctl + .rig_ports + .insert("hf".to_string(), 8080); + assert!(config.validate_resolved(&remotes(&["hf"])).is_err()); + } + + #[test] + fn test_bound_sockets_skips_disabled_frontends() { + let mut config = ClientConfig::default(); + config.frontends.http.enabled = false; + config.frontends.http_json.enabled = false; + config.frontends.rigctl.enabled = false; + assert!(config.bound_sockets().is_empty()); + } + #[test] fn test_validate_rejects_invalid_bandplan_region() { let mut config = ClientConfig::default(); diff --git a/src/trx-config/src/server.rs b/src/trx-config/src/server.rs index b5654570..b7834d62 100644 --- a/src/trx-config/src/server.rs +++ b/src/trx-config/src/server.rs @@ -16,7 +16,7 @@ use std::path::Path; use serde::{Deserialize, Serialize}; use crate::file::{ConfigError, ConfigFile, ConfigLoad}; -use crate::shared::{validate_log_level, validate_tokens}; +use crate::shared::{check_socket_conflicts, validate_log_level, validate_tokens, BoundSocket}; pub use trx_decode_log::DecodeLogsConfig; use trx_core::rig::state::RigMode; @@ -495,6 +495,28 @@ impl ServerConfig { Ok(()) } + /// Checks that only make sense once the rig list is known. + /// + /// `rigs` is the CLI-merged result of `resolved_rigs()`, and `sockets` are + /// the addresses the process is actually about to bind — the caller builds + /// them because `--listen` overrides both the control and audio addresses. + pub fn validate_resolved( + &self, + rigs: &[RigInstanceConfig], + sockets: &[BoundSocket], + ) -> Result<(), String> { + // Auto-generated ids can collide with explicitly configured ones, which + // only shows up after resolution. + let mut seen: std::collections::HashSet<&str> = std::collections::HashSet::new(); + for rig in rigs { + if !seen.insert(rig.id.as_str()) { + return Err(format!("duplicate rig id after resolution: \"{}\"", rig.id)); + } + } + + check_socket_conflicts(sockets) + } + /// Check that enabled `[[rigs]]` entries do not collide with one another. fn validate_rig_uniqueness(&self) -> Result<(), String> { if self.rigs.is_empty() { @@ -1401,6 +1423,67 @@ port = 4532 assert_eq!(rigs[0].id, "enabled"); } + // --- Second-phase validation against the resolved rig list --- + + #[test] + fn test_validate_resolved_rejects_listener_audio_port_collision() { + use crate::shared::BoundSocket; + let ip: std::net::IpAddr = "127.0.0.1".parse().unwrap(); + let cfg = ServerConfig::default(); + let rigs = cfg.resolved_rigs(); + let sockets = vec![ + BoundSocket::new(ip, 4530, "[listen]"), + BoundSocket::new(ip, 4530, "rig \"default\" [audio]"), + ]; + let err = cfg.validate_resolved(&rigs, &sockets).unwrap_err(); + assert!(err.contains("4530"), "unexpected error: {err}"); + } + + #[test] + fn test_validate_resolved_accepts_distinct_ports() { + use crate::shared::BoundSocket; + let ip: std::net::IpAddr = "127.0.0.1".parse().unwrap(); + let cfg = ServerConfig::default(); + let rigs = cfg.resolved_rigs(); + let sockets = vec![ + BoundSocket::new(ip, 4530, "[listen]"), + BoundSocket::new(ip, 4531, "rig \"default\" [audio]"), + ]; + assert!(cfg.validate_resolved(&rigs, &sockets).is_ok()); + } + + #[test] + fn test_validate_resolved_rejects_id_collision_after_generation() { + // The second entry has no id, so resolution names it "_", + // which the first entry has already claimed explicitly. + let toml_str = r#" +[[rigs]] +id = "ft817_1" +[rigs.rig] +model = "ft450d" +[rigs.rig.access] +type = "serial" +port = "/dev/ttyUSB0" +baud = 9600 +[rigs.audio] +port = 4531 + +[[rigs]] +[rigs.rig] +model = "ft817" +[rigs.rig.access] +type = "serial" +port = "/dev/ttyUSB1" +baud = 9600 +[rigs.audio] +port = 4532 +"#; + let cfg: ServerConfig = toml::from_str(toml_str).unwrap(); + let rigs = cfg.resolved_rigs(); + let err = cfg.validate_resolved(&rigs, &[]).unwrap_err(); + assert!(err.contains("ft817_1"), "unexpected error: {err}"); + } + #[test] fn test_validate_rejects_duplicate_rig_ids() { let toml_str = r#" diff --git a/src/trx-config/src/shared.rs b/src/trx-config/src/shared.rs index 60460b63..f1b998cb 100644 --- a/src/trx-config/src/shared.rs +++ b/src/trx-config/src/shared.rs @@ -19,6 +19,52 @@ //! would either bloat both binaries with unused fields or require a trait //! abstraction that adds complexity without clear benefit. +use std::net::IpAddr; + +/// A socket a component intends to bind, and what it is for. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct BoundSocket { + pub addr: IpAddr, + pub port: u16, + /// Human-readable owner, e.g. `[listen]` or `[frontends.http]`. + pub label: String, +} + +impl BoundSocket { + pub fn new(addr: IpAddr, port: u16, label: impl Into) -> Self { + Self { + addr, + port, + label: label.into(), + } + } +} + +/// Reject two components trying to bind the same socket. +/// +/// A wildcard address (`0.0.0.0` / `::`) conflicts with any other address on +/// the same port, since binding it claims every interface. Port 0 means "pick +/// an ephemeral port" and never conflicts. +pub fn check_socket_conflicts(sockets: &[BoundSocket]) -> Result<(), String> { + for (i, a) in sockets.iter().enumerate() { + if a.port == 0 { + continue; + } + for b in &sockets[i + 1..] { + if b.port != a.port { + continue; + } + if a.addr == b.addr || a.addr.is_unspecified() || b.addr.is_unspecified() { + return Err(format!( + "{} and {} would both bind {}:{}", + a.label, b.label, a.addr, a.port + )); + } + } + } + Ok(()) +} + /// Validate that a log level string is one of the accepted values. /// /// Returns `Ok(())` when `level` is `None` (defaulting is handled elsewhere) @@ -53,6 +99,47 @@ pub fn validate_tokens(path: &str, tokens: &[String]) -> Result<(), String> { mod tests { use super::*; + fn sock(addr: &str, port: u16, label: &str) -> BoundSocket { + BoundSocket::new(addr.parse().unwrap(), port, label) + } + + #[test] + fn test_socket_conflicts_detects_exact_duplicate() { + let err = check_socket_conflicts(&[ + sock("127.0.0.1", 8080, "[frontends.http]"), + sock("127.0.0.1", 8080, "[frontends.http_json]"), + ]) + .unwrap_err(); + assert!(err.contains("127.0.0.1:8080"), "unexpected error: {err}"); + } + + #[test] + fn test_socket_conflicts_detects_wildcard_overlap() { + assert!(check_socket_conflicts(&[ + sock("0.0.0.0", 4530, "[listen]"), + sock("127.0.0.1", 4530, "[audio]"), + ]) + .is_err()); + } + + #[test] + fn test_socket_conflicts_allows_distinct_addresses() { + assert!(check_socket_conflicts(&[ + sock("127.0.0.1", 4530, "[listen]"), + sock("192.168.1.5", 4530, "[audio]"), + ]) + .is_ok()); + } + + #[test] + fn test_socket_conflicts_ignores_ephemeral_ports() { + assert!(check_socket_conflicts(&[ + sock("127.0.0.1", 0, "[frontends.http_json]"), + sock("127.0.0.1", 0, "[frontends.http]"), + ]) + .is_ok()); + } + #[test] fn test_validate_log_level_none() { assert!(validate_log_level(None).is_ok()); diff --git a/src/trx-server/src/main.rs b/src/trx-server/src/main.rs index 8e9b156f..e1257872 100644 --- a/src/trx-server/src/main.rs +++ b/src/trx-server/src/main.rs @@ -29,6 +29,7 @@ use tracing::{error, info, warn}; use trx_core::audio::AudioStreamInfo; use trx_app::{init_logging, normalize_name}; +use trx_config::shared::BoundSocket; use trx_backend::{register_builtin_backends_on, RegistrationContext, RigAccess}; use trx_core::rig::controller::{AdaptivePolling, ExponentialBackoff}; use trx_core::rig::request::RigRequest; @@ -986,6 +987,34 @@ async fn main() -> DynResult<()> { (callsign, cfg.general.latitude, cfg.general.longitude) }; + // Second validation phase: now that CLI overrides have been folded in, check + // the things that need the final rig list — chiefly that no two listeners + // claim the same socket. + { + let mut sockets = Vec::new(); + if cfg.listen.enabled { + sockets.push(BoundSocket::new( + cli.listen.unwrap_or(cfg.listen.listen), + cli.port.unwrap_or(cfg.listen.port), + "[listen]", + )); + } + // --listen overrides the audio bind address for every rig; otherwise the + // global [audio].listen wins over the per-rig value. + let audio_ip = cli.listen.unwrap_or(cfg.audio.listen); + for rig in &resolved_rigs { + if rig.audio.enabled { + sockets.push(BoundSocket::new( + audio_ip, + rig.audio.port, + format!("rig \"{}\" [audio]", rig.id), + )); + } + } + cfg.validate_resolved(&resolved_rigs, &sockets) + .map_err(|e| format!("Invalid server configuration: {}", e))?; + } + info!( "Starting trx-server with {} rig(s): {}", resolved_rigs.len(),