From bf77a86df9ceb8fa65f471d77224a08b429ba734 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Tue, 23 Jul 2024 10:40:20 +0200 Subject: [PATCH 1/4] refactor(iroh-cli): Improve config file behaviour - Error on unknown fields. - Make it possible to express GcPolicy in the TOML config file. This was not possible before. --- Cargo.lock | 1 + iroh-cli/Cargo.toml | 1 + iroh-cli/src/commands/start.rs | 3 +- iroh-cli/src/config.rs | 176 ++++++++++++++++++++++++++++++--- iroh-cli/src/logging.rs | 2 +- 5 files changed, 170 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9a8dfdba61..53ecaaf387 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2641,6 +2641,7 @@ dependencies = [ "tracing", "tracing-appender", "tracing-subscriber", + "url", "walkdir", ] diff --git a/iroh-cli/Cargo.toml b/iroh-cli/Cargo.toml index fe60b28514..6d4f0e5cbc 100644 --- a/iroh-cli/Cargo.toml +++ b/iroh-cli/Cargo.toml @@ -72,6 +72,7 @@ nix = { version = "0.27", features = ["signal", "process"] } rand_xorshift = "0.3.0" regex = "1.10.3" testdir = "0.9.1" +url = "2.5.0" walkdir = "2" [features] diff --git a/iroh-cli/src/commands/start.rs b/iroh-cli/src/commands/start.rs index 39449fd15c..c1d872a6f4 100644 --- a/iroh-cli/src/commands/start.rs +++ b/iroh-cli/src/commands/start.rs @@ -10,7 +10,7 @@ use iroh::{ net::relay::{RelayMap, RelayMode}, node::RpcStatus, }; -use tracing::{info_span, Instrument}; +use tracing::{info_span, trace, Instrument}; /// Whether to stop the node after running a command or run forever until stopped. #[derive(Debug, Copy, Clone, Eq, PartialEq)] @@ -79,6 +79,7 @@ where F: FnOnce(iroh::client::Iroh) -> T + Send + 'static, T: Future> + 'static, { + trace!(?config, "using config"); let relay_map = config.relay_map()?; let spinner = create_spinner("Iroh booting..."); diff --git a/iroh-cli/src/config.rs b/iroh-cli/src/config.rs index d47250d862..84a80651aa 100644 --- a/iroh-cli/src/config.rs +++ b/iroh-cli/src/config.rs @@ -6,6 +6,7 @@ use std::{ path::{Path, PathBuf}, str::FromStr, sync::Arc, + time::Duration, }; use anyhow::{anyhow, bail, Context, Result}; @@ -48,15 +49,16 @@ impl ConsolePaths { } /// The configuration for an iroh node. -#[derive(PartialEq, Eq, Debug, Deserialize, Serialize, Clone)] -#[serde(default)] +#[derive(PartialEq, Eq, Debug, Deserialize, Clone)] +#[serde(default, deny_unknown_fields)] pub(crate) struct NodeConfig { /// The nodes for relay to use. pub(crate) relay_nodes: Vec, /// How often to run garbage collection. - pub(crate) gc_policy: GcPolicy, + pub(crate) gc_policy: GcPolicyConfig, /// Bind address on which to serve Prometheus metrics pub(crate) metrics_addr: Option, + /// Configuration for the logfile. pub(crate) file_logs: super::logging::FileLogging, /// Path to dump metrics to in CSV format. pub(crate) metrics_dump_path: Option, @@ -82,7 +84,7 @@ impl Default for NodeConfig { }; Self { relay_nodes: relay_nodes.into(), - gc_policy: GcPolicy::Disabled, + gc_policy: GcPolicyConfig::default(), metrics_addr: Some(([127, 0, 0, 1], 9090).into()), file_logs: Default::default(), metrics_dump_path: None, @@ -91,8 +93,12 @@ impl Default for NodeConfig { } impl NodeConfig { - /// Create a config using defaults, and the passed in config file. - pub async fn load(file: Option<&Path>) -> Result { + /// Creates a config from default config file. + /// + /// If the *file* is `Some` the configuration will be read from it. Otherwise the + /// default config file will be loaded. If that is not present the default config will + /// be used. + pub(crate) async fn load(file: Option<&Path>) -> Result { let default_config = iroh_config_path(CONFIG_FILE_NAME)?; let config_file = match file { @@ -107,7 +113,7 @@ impl NodeConfig { }; let mut config = if let Some(file) = config_file { let config = tokio::fs::read_to_string(file).await?; - toml::from_str(&config)? + Self::load_toml(&config)? } else { Self::default() }; @@ -119,6 +125,11 @@ impl NodeConfig { Ok(config) } + fn load_toml(s: &str) -> Result { + let config = toml::from_str(s)?; + Ok(config) + } + /// Constructs a `RelayMap` based on the current configuration. pub(crate) fn relay_map(&self) -> Result> { if self.relay_nodes.is_empty() { @@ -128,6 +139,29 @@ impl NodeConfig { } } +/// Serde-compatible configuration for [`GcPolicy`]. +/// +/// The [`GcPolicy`] struct is not amenable to TOML serialisation, this covers this gap. +#[derive(PartialEq, Eq, Debug, Default, Deserialize, Clone)] +#[serde(default, deny_unknown_fields, rename = "gc_policy")] +pub(crate) struct GcPolicyConfig { + enabled: bool, + interval: Option, +} + +impl From for GcPolicy { + fn from(source: GcPolicyConfig) -> Self { + if source.enabled { + match source.interval { + Some(interval) => Self::Interval(Duration::from_secs(interval)), + None => Self::default(), + } + } else { + Self::Disabled + } + } +} + /// Environment for CLI and REPL /// /// This is cheaply cloneable and has interior mutability. If not running in the console @@ -415,12 +449,132 @@ pub(crate) fn iroh_cache_path(file_name: &Path) -> Result { #[cfg(test)] mod tests { + use std::net::{Ipv4Addr, Ipv6Addr}; + + use url::Url; + + use crate::logging::{EnvFilter, Rotation}; + use super::*; - #[tokio::test] - async fn test_default_settings() { - let config = NodeConfig::load(None).await.unwrap(); + #[test] + fn test_toml_invalid_field() { + let source = r#" + not_a_field = true + "#; + let res = NodeConfig::load_toml(&source); + assert!(res.is_err()); + } + + #[test] + fn test_toml_relay_nodes() { + let source = r#" + [[relay_nodes]] + url = "https://example.org." + stun_only = false + stun_port = 123 + "#; + let config = NodeConfig::load_toml(&source).unwrap(); + + let expected = RelayNode { + url: Url::parse("https://example.org./").unwrap().into(), + stun_only: false, + stun_port: 123, + }; + assert_eq!(config.relay_nodes, vec![expected]); + } + + #[test] + fn test_toml_gc_policy() { + let source = r#" + [gc_policy] + enabled = false + "#; + let config = NodeConfig::load_toml(&source).unwrap(); + assert_eq!(GcPolicy::from(config.gc_policy), GcPolicy::Disabled); + + // Default interval should be used. + let source = r#" + [gc_policy] + enabled = true + "#; + let config = NodeConfig::load_toml(&source).unwrap(); + let gc_policy = GcPolicy::from(config.gc_policy); + assert!(matches!(gc_policy, GcPolicy::Interval(_))); + assert_eq!(gc_policy, GcPolicy::default()); + + let source = r#" + [gc_policy] + enabled = true + interval = 1234 + "#; + let config = NodeConfig::load_toml(&source).unwrap(); + assert_eq!( + GcPolicy::from(config.gc_policy), + GcPolicy::Interval(Duration::from_secs(1234)) + ); + + let source = r#" + [gc_policy] + not_a_field = true + "#; + let res = NodeConfig::load_toml(&source); + assert!(res.is_err()); + } + + #[test] + fn test_toml_metrics_addr() { + let source = r#" + metrics_addr = "1.2.3.4:1234" + "#; + let config = NodeConfig::load_toml(&source).unwrap(); + assert_eq!( + config.metrics_addr, + Some(SocketAddr::new(Ipv4Addr::new(1, 2, 3, 4).into(), 1234)), + ); + + let source = r#" + metrics_addr = "[123:456::789:abc]:1234" + "#; + let config = NodeConfig::load_toml(&source).unwrap(); + assert_eq!( + config.metrics_addr, + Some(SocketAddr::new( + Ipv6Addr::new(0x123, 0x456, 0, 0, 0, 0, 0x789, 0xabc).into(), + 1234 + )), + ); + } - assert_eq!(config.relay_nodes.len(), 2); + #[test] + fn test_toml_file_logs() { + let source = r#" + [file_logs] + rust_log = "iroh_net=trace" + max_files = 123 + rotation = "daily" + dir = "/var/log/iroh" + "#; + let config = NodeConfig::load_toml(&source).unwrap(); + assert_eq!( + config.file_logs.rust_log, + EnvFilter::from_str("iroh_net=trace").unwrap() + ); + assert_eq!(config.file_logs.max_files, 123); + assert_eq!(config.file_logs.rotation, Rotation::Daily); + assert_eq!(config.file_logs.dir, Some(PathBuf::from("/var/log/iroh"))); + + let source = r#" + [file_logs] + rust_log = "info" + "#; + let config = NodeConfig::load_toml(&source).unwrap(); + assert_eq!( + config.file_logs.rust_log, + EnvFilter::from_str("info").unwrap() + ); + assert_eq!(config.file_logs.max_files, 4); + assert_eq!(config.file_logs.rotation, Rotation::Hourly); + assert_eq!(config.file_logs.dir, None); } } diff --git a/iroh-cli/src/logging.rs b/iroh-cli/src/logging.rs index 2f2539174c..c06bf29b1b 100644 --- a/iroh-cli/src/logging.rs +++ b/iroh-cli/src/logging.rs @@ -98,7 +98,7 @@ pub(crate) fn init_terminal_logging() -> anyhow::Result<()> { } #[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] -#[serde(default)] +#[serde(default, deny_unknown_fields)] pub(crate) struct FileLogging { /// RUST_LOG directive to filter file logs. pub(crate) rust_log: EnvFilter, From 17f2091dfcd247a03d3bcc06d9c311f1a7e4714a Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Tue, 23 Jul 2024 10:57:18 +0200 Subject: [PATCH 2/4] improve cli help - Update header of iroh. - Directly point to config file docs. - Use consistent style. --- iroh-cli/src/commands.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/iroh-cli/src/commands.rs b/iroh-cli/src/commands.rs index cf97d62fe8..59529cd671 100644 --- a/iroh-cli/src/commands.rs +++ b/iroh-cli/src/commands.rs @@ -23,7 +23,7 @@ pub(crate) mod rpc; pub(crate) mod start; pub(crate) mod tags; -/// iroh is a tool for syncing bytes +/// iroh is a tool for building distributed apps /// https://iroh.computer/docs #[derive(Parser, Debug, Clone)] #[clap(version, verbatim_doc_comment)] @@ -31,7 +31,7 @@ pub(crate) struct Cli { #[clap(subcommand)] pub(crate) command: Commands, - /// Path to the configuration file. + /// Path to the configuration file, see https://iroh.computer/docs/reference/config. #[clap(long)] pub(crate) config: Option, @@ -47,7 +47,7 @@ pub(crate) struct Cli { #[clap(long)] pub(crate) rpc_addr: Option, - /// If set, metrics will be dumped in CSV format to the specified path at regular intervals (100ms). + /// Write metrics in CSV format at 100ms intervals. Disabled by default. #[clap(long)] pub(crate) metrics_dump_path: Option, } From 5972a3e6fe569410e99f38d128f410b6482615b2 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Tue, 23 Jul 2024 12:01:09 +0200 Subject: [PATCH 3/4] Add some useful reminders --- iroh-cli/src/config.rs | 2 ++ iroh-cli/src/logging.rs | 3 +++ iroh-net/src/relay/map.rs | 2 ++ iroh/src/node/builder.rs | 2 ++ 4 files changed, 9 insertions(+) diff --git a/iroh-cli/src/config.rs b/iroh-cli/src/config.rs index 84a80651aa..5f5ec2b753 100644 --- a/iroh-cli/src/config.rs +++ b/iroh-cli/src/config.rs @@ -49,6 +49,8 @@ impl ConsolePaths { } /// The configuration for an iroh node. +// Please note that this is documented in the `iroh.computer` repository under +// `src/app/docs/reference/config/page.mdx`. Any changes to this need to be updated there. #[derive(PartialEq, Eq, Debug, Deserialize, Clone)] #[serde(default, deny_unknown_fields)] pub(crate) struct NodeConfig { diff --git a/iroh-cli/src/logging.rs b/iroh-cli/src/logging.rs index c06bf29b1b..561a8f95b4 100644 --- a/iroh-cli/src/logging.rs +++ b/iroh-cli/src/logging.rs @@ -97,6 +97,9 @@ pub(crate) fn init_terminal_logging() -> anyhow::Result<()> { Ok(()) } +/// Configuration for the logfiles. +// Please note that this is documented in the `iroh.computer` repository under +// `src/app/docs/reference/config/page.mdx`. Any changes to this need to be updated there. #[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] #[serde(default, deny_unknown_fields)] pub(crate) struct FileLogging { diff --git a/iroh-net/src/relay/map.rs b/iroh-net/src/relay/map.rs index 3e3d8a8434..ebd49f6d69 100644 --- a/iroh-net/src/relay/map.rs +++ b/iroh-net/src/relay/map.rs @@ -116,6 +116,8 @@ impl fmt::Display for RelayMap { /// Information on a specific relay server. /// /// Includes the Url where it can be dialed. +// Please note that this is documented in the `iroh.computer` repository under +// `src/app/docs/reference/config/page.mdx`. Any changes to this need to be updated there. #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, PartialOrd, Ord)] pub struct RelayNode { /// The [`RelayUrl`] where this relay server can be dialed. diff --git a/iroh/src/node/builder.rs b/iroh/src/node/builder.rs index 4623261724..a208cdaa94 100644 --- a/iroh/src/node/builder.rs +++ b/iroh/src/node/builder.rs @@ -802,6 +802,8 @@ impl ProtocolBuilder { } /// Policy for garbage collection. +// Please note that this is documented in the `iroh.computer` repository under +// `src/app/docs/reference/config/page.mdx`. Any changes to this need to be updated there. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] pub enum GcPolicy { /// Garbage collection is disabled. From 0383acaeb548ce26b420f3a53ecc2e296313e665 Mon Sep 17 00:00:00 2001 From: Floris Bruynooghe Date: Tue, 23 Jul 2024 15:02:36 +0200 Subject: [PATCH 4/4] clippy --- iroh-cli/src/config.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/iroh-cli/src/config.rs b/iroh-cli/src/config.rs index 5f5ec2b753..f99f05d371 100644 --- a/iroh-cli/src/config.rs +++ b/iroh-cli/src/config.rs @@ -464,7 +464,7 @@ mod tests { let source = r#" not_a_field = true "#; - let res = NodeConfig::load_toml(&source); + let res = NodeConfig::load_toml(source); assert!(res.is_err()); } @@ -476,7 +476,7 @@ mod tests { stun_only = false stun_port = 123 "#; - let config = NodeConfig::load_toml(&source).unwrap(); + let config = NodeConfig::load_toml(source).unwrap(); let expected = RelayNode { url: Url::parse("https://example.org./").unwrap().into(), @@ -492,7 +492,7 @@ mod tests { [gc_policy] enabled = false "#; - let config = NodeConfig::load_toml(&source).unwrap(); + let config = NodeConfig::load_toml(source).unwrap(); assert_eq!(GcPolicy::from(config.gc_policy), GcPolicy::Disabled); // Default interval should be used. @@ -500,7 +500,7 @@ mod tests { [gc_policy] enabled = true "#; - let config = NodeConfig::load_toml(&source).unwrap(); + let config = NodeConfig::load_toml(source).unwrap(); let gc_policy = GcPolicy::from(config.gc_policy); assert!(matches!(gc_policy, GcPolicy::Interval(_))); assert_eq!(gc_policy, GcPolicy::default()); @@ -510,7 +510,7 @@ mod tests { enabled = true interval = 1234 "#; - let config = NodeConfig::load_toml(&source).unwrap(); + let config = NodeConfig::load_toml(source).unwrap(); assert_eq!( GcPolicy::from(config.gc_policy), GcPolicy::Interval(Duration::from_secs(1234)) @@ -520,7 +520,7 @@ mod tests { [gc_policy] not_a_field = true "#; - let res = NodeConfig::load_toml(&source); + let res = NodeConfig::load_toml(source); assert!(res.is_err()); } @@ -529,7 +529,7 @@ mod tests { let source = r#" metrics_addr = "1.2.3.4:1234" "#; - let config = NodeConfig::load_toml(&source).unwrap(); + let config = NodeConfig::load_toml(source).unwrap(); assert_eq!( config.metrics_addr, Some(SocketAddr::new(Ipv4Addr::new(1, 2, 3, 4).into(), 1234)), @@ -538,7 +538,7 @@ mod tests { let source = r#" metrics_addr = "[123:456::789:abc]:1234" "#; - let config = NodeConfig::load_toml(&source).unwrap(); + let config = NodeConfig::load_toml(source).unwrap(); assert_eq!( config.metrics_addr, Some(SocketAddr::new( @@ -557,7 +557,7 @@ mod tests { rotation = "daily" dir = "/var/log/iroh" "#; - let config = NodeConfig::load_toml(&source).unwrap(); + let config = NodeConfig::load_toml(source).unwrap(); assert_eq!( config.file_logs.rust_log, EnvFilter::from_str("iroh_net=trace").unwrap() @@ -570,7 +570,7 @@ mod tests { [file_logs] rust_log = "info" "#; - let config = NodeConfig::load_toml(&source).unwrap(); + let config = NodeConfig::load_toml(source).unwrap(); assert_eq!( config.file_logs.rust_log, EnvFilter::from_str("info").unwrap()