Fix and harden client/server configuration #48
@@ -20,6 +20,7 @@ use tokio::task::JoinHandle;
|
||||
use tracing::{error, info};
|
||||
|
||||
use trx_app::{init_logging, normalize_name};
|
||||
use trx_config::ConfigFile;
|
||||
use trx_core::audio::AudioStreamInfo;
|
||||
|
||||
use trx_core::decode::DecodedMessage;
|
||||
@@ -80,7 +81,7 @@ struct Cli {
|
||||
/// rigctl frontend listen address
|
||||
#[arg(long = "rigctl-listen")]
|
||||
rigctl_listen: Option<IpAddr>,
|
||||
/// rigctl frontend listen port
|
||||
/// Deprecated: ignored, use [frontends.rigctl].rig_ports
|
||||
#[arg(long = "rigctl-port")]
|
||||
rigctl_port: Option<u16>,
|
||||
/// JSON TCP frontend listen address
|
||||
@@ -127,6 +128,7 @@ fn check_config(loaded: &trx_config::ConfigLoad<ClientConfig>) -> DynResult<()>
|
||||
}
|
||||
|
||||
let mut warnings: Vec<String> = loaded.unknown_keys.iter().map(|k| k.to_string()).collect();
|
||||
warnings.extend(ClientConfig::deprecations(&loaded.present_keys));
|
||||
|
||||
let mut cfg = loaded.config.clone();
|
||||
let mut errors = Vec::new();
|
||||
@@ -223,6 +225,13 @@ async fn async_init() -> DynResult<AppState> {
|
||||
info!("Loaded configuration from {}", path.display());
|
||||
}
|
||||
loaded.report_unknown_keys(cli.strict_config)?;
|
||||
loaded.report_deprecations();
|
||||
if cli.rigctl_port.is_some() {
|
||||
tracing::warn!(
|
||||
"--rigctl-port is ignored; give each rig its own listener via \
|
||||
[frontends.rigctl].rig_ports"
|
||||
);
|
||||
}
|
||||
|
||||
let mut cfg = loaded.config;
|
||||
// Secrets configured as *_file are read before validation, so everything
|
||||
|
||||
@@ -935,6 +935,51 @@ impl ConfigFile for ClientConfig {
|
||||
"trx-client"
|
||||
}
|
||||
|
||||
/// Keys that no longer do what they look like they do.
|
||||
fn deprecations(present: &std::collections::BTreeSet<String>) -> Vec<String> {
|
||||
let mut warnings = Vec::new();
|
||||
|
||||
if present.contains("remote") {
|
||||
if present.contains("remotes") {
|
||||
warnings.push(
|
||||
"[remote] is ignored because [[remotes]] is set; delete it".to_string(),
|
||||
);
|
||||
} else {
|
||||
warnings.push(
|
||||
"[remote] is superseded by [[remotes]], which supports several rigs; \
|
||||
it still works but will be removed in a future release"
|
||||
.to_string(),
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
if present.contains("frontends.rigctl.port") {
|
||||
warnings.push(
|
||||
"[frontends.rigctl].port is ignored; give each rig its own listener via \
|
||||
[frontends.rigctl].rig_ports"
|
||||
.to_string(),
|
||||
);
|
||||
}
|
||||
|
||||
if present.contains("frontends.audio.rig_ports") {
|
||||
warnings.push(
|
||||
"[frontends.audio].rig_ports is superseded by [frontends.audio].rig_urls, \
|
||||
which can also point at a different host"
|
||||
.to_string(),
|
||||
);
|
||||
}
|
||||
|
||||
if present.contains("frontends.http.default_rig_id") {
|
||||
warnings.push(
|
||||
"[frontends.http].default_rig_id has been renamed to default_rig_name; \
|
||||
the old name still works but will be removed in a future release"
|
||||
.to_string(),
|
||||
);
|
||||
}
|
||||
|
||||
warnings
|
||||
}
|
||||
|
||||
/// Include one `[[remotes]]` entry so unknown keys nested inside a remote
|
||||
/// entry still get a suggestion.
|
||||
fn reference_value() -> toml::Value {
|
||||
@@ -955,6 +1000,7 @@ impl ConfigFile for ClientConfig {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::ConfigFile;
|
||||
|
||||
#[test]
|
||||
fn test_default_config() {
|
||||
@@ -1404,6 +1450,51 @@ url = "remote.example.com:4530"
|
||||
.contains("poll_interval_ms must be > 0"));
|
||||
}
|
||||
|
||||
// --- Deprecation warnings ---
|
||||
|
||||
fn present(paths: &[&str]) -> std::collections::BTreeSet<String> {
|
||||
paths.iter().map(|p| p.to_string()).collect()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deprecation_flags_shadowed_remote_section() {
|
||||
let warnings = ClientConfig::deprecations(&present(&["remote", "remotes"]));
|
||||
assert!(
|
||||
warnings.iter().any(|w| w.contains("[remote] is ignored")),
|
||||
"unexpected warnings: {warnings:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deprecation_nudges_lone_remote_section() {
|
||||
let warnings = ClientConfig::deprecations(&present(&["remote"]));
|
||||
assert_eq!(warnings.len(), 1);
|
||||
assert!(warnings[0].contains("superseded by [[remotes]]"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deprecation_flags_ignored_rigctl_port() {
|
||||
let warnings = ClientConfig::deprecations(&present(&["frontends.rigctl.port"]));
|
||||
assert!(warnings.iter().any(|w| w.contains("rig_ports")));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deprecation_flags_renamed_default_rig_id() {
|
||||
let warnings = ClientConfig::deprecations(&present(&["frontends.http.default_rig_id"]));
|
||||
assert!(warnings.iter().any(|w| w.contains("default_rig_name")));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_no_deprecations_for_a_current_config() {
|
||||
let warnings = ClientConfig::deprecations(&present(&[
|
||||
"remotes",
|
||||
"frontends.http.port",
|
||||
"frontends.rigctl.rig_ports",
|
||||
"frontends.audio.rig_urls",
|
||||
]));
|
||||
assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}");
|
||||
}
|
||||
|
||||
// --- Secret indirection ---
|
||||
|
||||
fn secret_file(content: &str) -> tempfile::NamedTempFile {
|
||||
|
||||
@@ -22,7 +22,9 @@ use serde::Serialize;
|
||||
use std::path::{Path, PathBuf};
|
||||
use thiserror::Error;
|
||||
|
||||
use crate::unknown::{describe, UnknownKey};
|
||||
use std::collections::BTreeSet;
|
||||
|
||||
use crate::unknown::{describe, flatten_paths, UnknownKey};
|
||||
|
||||
/// Every section key that may appear at the root of a combined config file.
|
||||
pub const SECTION_KEYS: &[&str] = &["trx-server", "trx-client"];
|
||||
@@ -37,6 +39,18 @@ pub struct ConfigLoad<T> {
|
||||
pub path: Option<PathBuf>,
|
||||
/// Keys present in the file that no config field claimed.
|
||||
pub unknown_keys: Vec<UnknownKey>,
|
||||
/// Every key path the file actually set. Defaults are indistinguishable
|
||||
/// from explicit values once deserialized, so deprecation checks need this.
|
||||
pub present_keys: BTreeSet<String>,
|
||||
}
|
||||
|
||||
impl<T: ConfigFile> ConfigLoad<T> {
|
||||
/// Log a warning for every deprecated key the file sets.
|
||||
pub fn report_deprecations(&self) {
|
||||
for message in T::deprecations(&self.present_keys) {
|
||||
tracing::warn!("{}", message);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl<T> ConfigLoad<T> {
|
||||
@@ -97,10 +111,12 @@ fn select_section(table: &toml::Table, key: &str) -> Option<toml::Value> {
|
||||
/// Returns `Ok(Some((cfg, unknown_keys)))` when the section is present and
|
||||
/// parses cleanly, `Ok(None)` when the section is absent, or `Err` on I/O /
|
||||
/// parse failure.
|
||||
type LoadedSection<T> = (T, Vec<UnknownKey>, BTreeSet<String>);
|
||||
|
||||
fn load_section_from_file<T: ConfigFile>(
|
||||
path: &Path,
|
||||
key: &str,
|
||||
) -> Result<Option<(T, Vec<UnknownKey>)>, ConfigError> {
|
||||
) -> Result<Option<LoadedSection<T>>, ConfigError> {
|
||||
let content = std::fs::read_to_string(path)
|
||||
.map_err(|e| ConfigError::ReadError(path.to_path_buf(), e.to_string()))?;
|
||||
|
||||
@@ -116,13 +132,19 @@ fn load_section_from_file<T: ConfigFile>(
|
||||
crate::secrets::expand_env_vars(&mut section)
|
||||
.map_err(|e| ConfigError::ParseError(path.to_path_buf(), e))?;
|
||||
|
||||
let present_keys = flatten_paths(§ion);
|
||||
|
||||
// Deserialize straight from the TOML value so serde applies every default,
|
||||
// recording any key no field claimed.
|
||||
let mut ignored: Vec<String> = Vec::new();
|
||||
let cfg: T = serde_ignored::deserialize(section, |path| ignored.push(path.to_string()))
|
||||
.map_err(|e| ConfigError::ParseError(path.to_path_buf(), e.to_string()))?;
|
||||
|
||||
Ok(Some((cfg, describe(&ignored, &T::reference_value()))))
|
||||
Ok(Some((
|
||||
cfg,
|
||||
describe(&ignored, &T::reference_value()),
|
||||
present_keys,
|
||||
)))
|
||||
}
|
||||
|
||||
/// Trait for loading configuration from a `trx-rs.toml` section.
|
||||
@@ -130,6 +152,12 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize {
|
||||
/// Section key in `trx-rs.toml` (e.g. `"trx-server"` or `"trx-client"`).
|
||||
fn section_key() -> &'static str;
|
||||
|
||||
/// Warnings for deprecated keys the file sets, given every key path present
|
||||
/// in it. Defaults to none.
|
||||
fn deprecations(_present_keys: &BTreeSet<String>) -> Vec<String> {
|
||||
Vec::new()
|
||||
}
|
||||
|
||||
/// A TOML rendering of a populated config, used to suggest corrections for
|
||||
/// unknown keys. Implementations should fill in list-valued sections such
|
||||
/// as `[[rigs]]` so keys nested inside them can be suggested too.
|
||||
@@ -145,8 +173,8 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize {
|
||||
/// cannot be read, is not valid TOML, or is sectioned for some other
|
||||
/// component only.
|
||||
fn load_from_file(path: &Path) -> Result<ConfigLoad<Self>, ConfigError> {
|
||||
let (config, unknown_keys) = load_section_from_file::<Self>(path, Self::section_key())?
|
||||
.ok_or_else(|| {
|
||||
let (config, unknown_keys, present_keys) =
|
||||
load_section_from_file::<Self>(path, Self::section_key())?.ok_or_else(|| {
|
||||
ConfigError::ParseError(
|
||||
path.to_path_buf(),
|
||||
format!("missing [{}] section", Self::section_key()),
|
||||
@@ -156,6 +184,7 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize {
|
||||
config,
|
||||
path: Some(path.to_path_buf()),
|
||||
unknown_keys,
|
||||
present_keys,
|
||||
})
|
||||
}
|
||||
|
||||
@@ -166,13 +195,14 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize {
|
||||
fn load_from_default_paths() -> Result<ConfigLoad<Self>, ConfigError> {
|
||||
for path in config_search_paths() {
|
||||
if path.exists() {
|
||||
if let Some((config, unknown_keys)) =
|
||||
if let Some((config, unknown_keys, present_keys)) =
|
||||
load_section_from_file::<Self>(&path, Self::section_key())?
|
||||
{
|
||||
return Ok(ConfigLoad {
|
||||
config,
|
||||
path: Some(path),
|
||||
unknown_keys,
|
||||
present_keys,
|
||||
});
|
||||
}
|
||||
}
|
||||
@@ -181,6 +211,7 @@ pub trait ConfigFile: Sized + Default + DeserializeOwned + Serialize {
|
||||
config: Self::default(),
|
||||
path: None,
|
||||
unknown_keys: Vec::new(),
|
||||
present_keys: BTreeSet::new(),
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1092,6 +1092,33 @@ impl ConfigFile for ServerConfig {
|
||||
"trx-server"
|
||||
}
|
||||
|
||||
/// The flat per-rig sections are silently ignored once `[[rigs]]` exists,
|
||||
/// which is worth saying out loud.
|
||||
fn deprecations(present: &std::collections::BTreeSet<String>) -> Vec<String> {
|
||||
if !present.contains("rigs") {
|
||||
return Vec::new();
|
||||
}
|
||||
[
|
||||
"rig",
|
||||
"behavior",
|
||||
"audio",
|
||||
"sdr",
|
||||
"pskreporter",
|
||||
"aprsfi",
|
||||
"decode_logs",
|
||||
"decoders",
|
||||
]
|
||||
.iter()
|
||||
.filter(|section| present.contains(**section))
|
||||
.map(|section| {
|
||||
format!(
|
||||
"[{section}] is ignored because [[rigs]] is set; move it to the \
|
||||
matching [rigs.{section}] entry"
|
||||
)
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
/// Include one `[[rigs]]` entry so unknown keys nested inside a rig entry
|
||||
/// still get a suggestion.
|
||||
fn reference_value() -> toml::Value {
|
||||
@@ -1106,6 +1133,7 @@ impl ConfigFile for ServerConfig {
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use crate::ConfigFile;
|
||||
|
||||
#[test]
|
||||
fn test_default_config() {
|
||||
@@ -1751,6 +1779,28 @@ port = 4531
|
||||
);
|
||||
}
|
||||
|
||||
// --- Deprecation warnings ---
|
||||
|
||||
fn present(paths: &[&str]) -> std::collections::BTreeSet<String> {
|
||||
paths.iter().map(|p| p.to_string()).collect()
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_deprecation_flags_flat_sections_shadowed_by_rigs() {
|
||||
let warnings = ServerConfig::deprecations(&present(&["rigs", "rig", "audio"]));
|
||||
assert_eq!(warnings.len(), 2, "unexpected warnings: {warnings:?}");
|
||||
assert!(warnings.iter().any(|w| w.contains("[rig] is ignored")));
|
||||
assert!(warnings.iter().any(|w| w.contains("[audio] is ignored")));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_no_deprecation_for_single_rig_layout() {
|
||||
// The flat layout is the documented simple form; only warn when it is
|
||||
// being silently ignored.
|
||||
let warnings = ServerConfig::deprecations(&present(&["rig", "audio", "listen"]));
|
||||
assert!(warnings.is_empty(), "unexpected warnings: {warnings:?}");
|
||||
}
|
||||
|
||||
// --- Secret indirection ---
|
||||
|
||||
#[test]
|
||||
|
||||
@@ -30,6 +30,7 @@ use trx_core::audio::AudioStreamInfo;
|
||||
|
||||
use trx_app::{init_logging, normalize_name};
|
||||
use trx_config::shared::BoundSocket;
|
||||
use trx_config::ConfigFile;
|
||||
use trx_backend::{register_builtin_backends_on, RegistrationContext, RigAccess};
|
||||
use trx_core::rig::controller::{AdaptivePolling, ExponentialBackoff};
|
||||
use trx_core::rig::request::RigRequest;
|
||||
@@ -940,8 +941,10 @@ fn check_config(cli: &Cli, loaded: &trx_config::ConfigLoad<ServerConfig>) -> Dyn
|
||||
None => println!("(no config file found; checking built-in defaults)"),
|
||||
}
|
||||
|
||||
for key in &loaded.unknown_keys {
|
||||
println!(" warning: {}", key);
|
||||
let mut warnings: Vec<String> = loaded.unknown_keys.iter().map(|k| k.to_string()).collect();
|
||||
warnings.extend(ServerConfig::deprecations(&loaded.present_keys));
|
||||
for warning in &warnings {
|
||||
println!(" warning: {}", warning);
|
||||
}
|
||||
|
||||
let mut cfg = loaded.config.clone();
|
||||
@@ -966,17 +969,12 @@ fn check_config(cli: &Cli, loaded: &trx_config::ConfigLoad<ServerConfig>) -> Dyn
|
||||
rigs.len(),
|
||||
rigs.iter().map(|r| r.id.as_str()).collect::<Vec<_>>().join(", ")
|
||||
);
|
||||
if !loaded.unknown_keys.is_empty() {
|
||||
println!(" {} warning(s)", loaded.unknown_keys.len());
|
||||
if !warnings.is_empty() {
|
||||
println!(" {} warning(s)", warnings.len());
|
||||
}
|
||||
Ok(())
|
||||
} else {
|
||||
Err(format!(
|
||||
"{} error(s), {} warning(s)",
|
||||
errors.len(),
|
||||
loaded.unknown_keys.len()
|
||||
)
|
||||
.into())
|
||||
Err(format!("{} error(s), {} warning(s)", errors.len(), warnings.len()).into())
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1010,6 +1008,7 @@ async fn main() -> DynResult<()> {
|
||||
info!("Loaded configuration from {}", path.display());
|
||||
}
|
||||
loaded.report_unknown_keys(cli.strict_config)?;
|
||||
loaded.report_deprecations();
|
||||
|
||||
let mut cfg = loaded.config;
|
||||
// Secrets configured as *_file are read before validation, so everything
|
||||
|
||||
Reference in New Issue
Block a user