fix(iroh-cli)!: Improve cli and configuration file (n0-computer#2532)

## Description

The configuration file behaviour is improved:

- Unknown fields will now cause an error rather than be silently
ignored.

- Make it possible to express GcPolicy in the TOML config file.  This
  was not possible before.  It was possible to disable it before, but
  it was disabled by default so that was rather moot.

The --help output is improved:

- Update header of iroh.
- Directly point to config file docs.
- Use consistent style.


<!-- A summary of what this pull request achieves and a rough list of
changes. -->

## Breaking Changes

- Unknown fields in the configuration file will now cause an error.

- Configuring the GC Policy in the configuration file has changed.

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

<!-- Any notes, remarks or open questions you have to make about the -->
<!-- PR. -->

## Change checklist

- [ ] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.

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]

iroh-cli/src/commands.rs

@@ -23,15 +23,15 @@ 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)]
 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<PathBuf>,
 
@@ -47,7 +47,7 @@ pub(crate) struct Cli {
     #[clap(long)]
     pub(crate) rpc_addr: Option<SocketAddr>,
 
-    /// 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<PathBuf>,
 }

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<Output = Result<()>> + 'static,
 {
+    trace!(?config, "using config");
     let relay_map = config.relay_map()?;
 
     let spinner = create_spinner("Iroh booting...");

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,18 @@ impl ConsolePaths {
 }
 
 /// The configuration for an iroh node.
-#[derive(PartialEq, Eq, Debug, Deserialize, Serialize, Clone)]
-#[serde(default)]
+// 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 {
     /// The nodes for relay to use.
     pub(crate) relay_nodes: Vec<RelayNode>,
     /// 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<SocketAddr>,
+    /// 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<PathBuf>,
@@ -82,7 +86,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 +95,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<NodeConfig> {
+    /// 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<NodeConfig> {
         let default_config = iroh_config_path(CONFIG_FILE_NAME)?;
 
         let config_file = match file {
@@ -107,7 +115,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 +127,11 @@ impl NodeConfig {
         Ok(config)
     }
 
+    fn load_toml(s: &str) -> Result<NodeConfig> {
+        let config = toml::from_str(s)?;
+        Ok(config)
+    }
+
     /// Constructs a `RelayMap` based on the current configuration.
     pub(crate) fn relay_map(&self) -> Result<Option<RelayMap>> {
         if self.relay_nodes.is_empty() {
@@ -128,6 +141,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<u64>,
+}
+
+impl From<GcPolicyConfig> 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 +451,132 @@ pub(crate) fn iroh_cache_path(file_name: &Path) -> Result<PathBuf> {
 
 #[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);
     }
 }

iroh-cli/src/logging.rs

@@ -97,8 +97,11 @@ 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)]
+#[serde(default, deny_unknown_fields)]
 pub(crate) struct FileLogging {
     /// RUST_LOG directive to filter file logs.
     pub(crate) rust_log: EnvFilter,

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.

iroh/src/node/builder.rs

@@ -802,6 +802,8 @@ impl<D: iroh_blobs::store::Store> ProtocolBuilder<D> {
 }
 
 /// 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.