[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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SyX26FCpMQxiBoC7r5K1A7 Signed-off-by: Stan Grams <sjg@haxx.space>
This commit is contained in:
@@ -153,7 +153,7 @@ async fn async_init() -> DynResult<AppState> {
|
||||
}
|
||||
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<AppState> {
|
||||
.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()
|
||||
|
||||
@@ -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<BoundSocket> {
|
||||
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<RemoteEntry> {
|
||||
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();
|
||||
|
||||
@@ -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 "<model>_<index>",
|
||||
// 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#"
|
||||
|
||||
@@ -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<String>) -> 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());
|
||||
|
||||
@@ -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(),
|
||||
|
||||
Reference in New Issue
Block a user