From da6a21eac969c96005f2b5b4deb0b1ea91926951 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 14 May 2024 10:22:56 +0100 Subject: [PATCH 1/2] refactor: [#855] show toml file location in Figment errors Before this commit we loaded configuration in Figment always using a Toml string even if the configuration cames from a toml file. WHen there is an error Figment does not show the file location and that's one of the main advantages of using Figment. All errors point to the primary source of the configuration option. This commit fixes that problem leting Figment to load the configuration from the file when the user provides a file. Sample error: ``` Loading configuration from default configuration file: `./share/default/config/tracker.development.sqlite3.toml` ... thread 'main' panicked at src/bootstrap/config.rs:45:32: called `Result::unwrap()` on an `Err` value: ConfigError { source: LocatedError { source: Error { tag: Tag(Default, 2), profile: Some(Profile(Uncased { string: "default" })), metadata: Some(Metadata { name: "TOML file", source: Some(File("/home/developer/torrust/torrust-tracker/./share/default/config/tracker.development.sqlite3.toml")), provide_location: Some(Location { file: "packages/configuration/src/v1/mod.rs", line: 330, col: 18 }), interpolater: }), path: [], kind: Message("TOML parse error at line 2, column 15\n |\n2 | enabled = truee\n | ^\nexpected newline, `#`\n"), prev: None }, location: Location { file: "packages/configuration/src/v1/mod.rs", line: 334, col: 41 } } } note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` Notice how the file path is included is the error: `/home/developer/torrust/torrust-tracker/./share/default/config/tracker.development.sqlite3.toml` --- packages/configuration/src/lib.rs | 47 ++++++++---------- packages/configuration/src/v1/mod.rs | 71 +++++++++++++++++++++++----- 2 files changed, 79 insertions(+), 39 deletions(-) diff --git a/packages/configuration/src/lib.rs b/packages/configuration/src/lib.rs index c393623df..9a00e6bbc 100644 --- a/packages/configuration/src/lib.rs +++ b/packages/configuration/src/lib.rs @@ -7,8 +7,8 @@ pub mod v1; use std::collections::HashMap; +use std::env; use std::sync::Arc; -use std::{env, fs}; use camino::Utf8PathBuf; use derive_more::Constructor; @@ -38,7 +38,8 @@ pub struct TrackerPolicy { /// Information required for loading config #[derive(Debug, Default, Clone)] pub struct Info { - tracker_toml: String, + config_toml: Option, + config_toml_path: String, api_admin_token: Option, } @@ -51,38 +52,30 @@ impl Info { /// #[allow(clippy::needless_pass_by_value)] pub fn new( - env_var_config: String, - env_var_path_config: String, - default_path_config: String, + env_var_config_toml: String, + env_var_config_toml_path: String, + default_config_toml_path: String, env_var_api_admin_token: String, ) -> Result { - let tracker_toml = if let Ok(tracker_toml) = env::var(&env_var_config) { - println!("Loading configuration from env var {env_var_config} ..."); + let config_toml = if let Ok(config_toml) = env::var(env_var_config_toml) { + println!("Loading configuration from environment variable {config_toml} ..."); + Some(config_toml) + } else { + None + }; - tracker_toml + let config_toml_path = if let Ok(config_toml_path) = env::var(env_var_config_toml_path) { + println!("Loading configuration from file: `{config_toml_path}` ..."); + config_toml_path } else { - let config_path = if let Ok(config_path) = env::var(env_var_path_config) { - println!("Loading configuration file: `{config_path}` ..."); - - config_path - } else { - println!("Loading default configuration file: `{default_path_config}` ..."); - - default_path_config - }; - - fs::read_to_string(config_path) - .map_err(|e| Error::UnableToLoadFromConfigFile { - source: (Arc::new(e) as DynError).into(), - })? - .parse() - .map_err(|_e: std::convert::Infallible| Error::Infallible)? + println!("Loading configuration from default configuration file: `{default_config_toml_path}` ..."); + default_config_toml_path }; - let api_admin_token = env::var(env_var_api_admin_token).ok(); Ok(Self { - tracker_toml, - api_admin_token, + config_toml, + config_toml_path, + api_admin_token: env::var(env_var_api_admin_token).ok(), }) } } diff --git a/packages/configuration/src/v1/mod.rs b/packages/configuration/src/v1/mod.rs index d19fedc87..8e15d65ca 100644 --- a/packages/configuration/src/v1/mod.rs +++ b/packages/configuration/src/v1/mod.rs @@ -250,6 +250,11 @@ use self::tracker_api::HttpApi; use self::udp_tracker::UdpTracker; use crate::{Error, Info}; +/// Prefix for env vars that overwrite configuration options. +const CONFIG_OVERRIDE_PREFIX: &str = "TORRUST_TRACKER_CONFIG_OVERRIDE_"; +/// Path separator in env var names for nested values in configuration. +const CONFIG_OVERRIDE_SEPARATOR: &str = "__"; + /// Core configuration for the tracker. #[derive(Serialize, Deserialize, PartialEq, Eq, Debug)] pub struct Configuration { @@ -315,9 +320,16 @@ impl Configuration { /// /// Will return `Err` if the environment variable does not exist or has a bad configuration. pub fn load(info: &Info) -> Result { - let figment = Figment::from(Serialized::defaults(Configuration::default())) - .merge(Toml::string(&info.tracker_toml)) - .merge(Env::prefixed("TORRUST_TRACKER__").split("__")); + let figment = if let Some(config_toml) = &info.config_toml { + // Config in env var has priority over config file path + Figment::from(Serialized::defaults(Configuration::default())) + .merge(Toml::string(config_toml)) + .merge(Env::prefixed(CONFIG_OVERRIDE_PREFIX).split(CONFIG_OVERRIDE_SEPARATOR)) + } else { + Figment::from(Serialized::defaults(Configuration::default())) + .merge(Toml::file(&info.config_toml_path)) + .merge(Env::prefixed(CONFIG_OVERRIDE_PREFIX).split(CONFIG_OVERRIDE_SEPARATOR)) + }; let mut config: Configuration = figment.extract()?; @@ -449,11 +461,14 @@ mod tests { #[test] fn configuration_should_use_the_default_values_when_an_empty_configuration_is_provided_by_the_user() { - figment::Jail::expect_with(|_jail| { + figment::Jail::expect_with(|jail| { + jail.create_file("tracker.toml", "")?; + let empty_configuration = String::new(); let info = Info { - tracker_toml: empty_configuration, + config_toml: Some(empty_configuration), + config_toml_path: "tracker.toml".to_string(), api_admin_token: None, }; @@ -466,28 +481,59 @@ mod tests { } #[test] - fn configuration_should_be_loaded_from_a_toml_config_file() { + fn default_configuration_could_be_overwritten_from_a_single_env_var_with_toml_contents() { figment::Jail::expect_with(|_jail| { + let config_toml = r#" + db_path = "OVERWRITTEN DEFAULT DB PATH" + "# + .to_string(); + let info = Info { - tracker_toml: default_config_toml(), + config_toml: Some(config_toml), + config_toml_path: String::new(), api_admin_token: None, }; let configuration = Configuration::load(&info).expect("Could not load configuration from file"); - assert_eq!(configuration, Configuration::default()); + assert_eq!(configuration.core.db_path, "OVERWRITTEN DEFAULT DB PATH".to_string()); + + Ok(()) + }); + } + + #[test] + fn default_configuration_could_be_overwritten_from_a_toml_config_file() { + figment::Jail::expect_with(|jail| { + jail.create_file( + "tracker.toml", + r#" + db_path = "OVERWRITTEN DEFAULT DB PATH" + "#, + )?; + + let info = Info { + config_toml: None, + config_toml_path: "tracker.toml".to_string(), + api_admin_token: None, + }; + + let configuration = Configuration::load(&info).expect("Could not load configuration from file"); + + assert_eq!(configuration.core.db_path, "OVERWRITTEN DEFAULT DB PATH".to_string()); Ok(()) }); } #[test] - fn configuration_should_allow_to_overwrite_the_default_tracker_api_token_for_admin_with_env_var() { + fn configuration_should_allow_to_overwrite_the_default_tracker_api_token_for_admin_with_an_env_var() { figment::Jail::expect_with(|jail| { - jail.set_env("TORRUST_TRACKER__HTTP_API__ACCESS_TOKENS__ADMIN", "NewToken"); + jail.set_env("TORRUST_TRACKER_CONFIG_OVERRIDE_HTTP_API__ACCESS_TOKENS__ADMIN", "NewToken"); let info = Info { - tracker_toml: default_config_toml(), + config_toml: Some(default_config_toml()), + config_toml_path: String::new(), api_admin_token: None, }; @@ -506,7 +552,8 @@ mod tests { fn configuration_should_allow_to_overwrite_the_default_tracker_api_token_for_admin_with_the_deprecated_env_var_name() { figment::Jail::expect_with(|_jail| { let info = Info { - tracker_toml: default_config_toml(), + config_toml: Some(default_config_toml()), + config_toml_path: String::new(), api_admin_token: Some("NewToken".to_owned()), }; From 4de5e7d32efd1277138ef1b92602204cdf5f8375 Mon Sep 17 00:00:00 2001 From: Jose Celano Date: Tue, 14 May 2024 10:39:20 +0100 Subject: [PATCH 2/2] refactor: move config env vars to configuration package --- packages/configuration/src/lib.rs | 24 ++++++++++++++++++------ src/bootstrap/config.rs | 22 ++-------------------- 2 files changed, 20 insertions(+), 26 deletions(-) diff --git a/packages/configuration/src/lib.rs b/packages/configuration/src/lib.rs index 9a00e6bbc..588feb87e 100644 --- a/packages/configuration/src/lib.rs +++ b/packages/configuration/src/lib.rs @@ -20,6 +20,19 @@ use torrust_tracker_located_error::{DynError, LocatedError}; /// The maximum number of returned peers for a torrent. pub const TORRENT_PEERS_LIMIT: usize = 74; +// Environment variables + +/// The whole `tracker.toml` file content. It has priority over the config file. +/// Even if the file is not on the default path. +const ENV_VAR_CONFIG: &str = "TORRUST_TRACKER_CONFIG"; + +/// The `tracker.toml` file location. +pub const ENV_VAR_PATH_CONFIG: &str = "TORRUST_TRACKER_PATH_CONFIG"; + +/// Env var to overwrite API admin token. +/// Deprecated: use `TORRUST_TRACKER_CONFIG_OVERRIDE_HTTP_API__ACCESS_TOKENS__ADMIN`. +const ENV_VAR_API_ADMIN_TOKEN: &str = "TORRUST_TRACKER_API_ADMIN_TOKEN"; + pub type Configuration = v1::Configuration; pub type UdpTracker = v1::udp_tracker::UdpTracker; pub type HttpTracker = v1::http_tracker::HttpTracker; @@ -51,12 +64,11 @@ impl Info { /// Will return `Err` if unable to obtain a configuration. /// #[allow(clippy::needless_pass_by_value)] - pub fn new( - env_var_config_toml: String, - env_var_config_toml_path: String, - default_config_toml_path: String, - env_var_api_admin_token: String, - ) -> Result { + pub fn new(default_config_toml_path: String) -> Result { + let env_var_config_toml = ENV_VAR_CONFIG.to_string(); + let env_var_config_toml_path = ENV_VAR_PATH_CONFIG.to_string(); + let env_var_api_admin_token = ENV_VAR_API_ADMIN_TOKEN.to_string(); + let config_toml = if let Ok(config_toml) = env::var(env_var_config_toml) { println!("Loading configuration from environment variable {config_toml} ..."); Some(config_toml) diff --git a/src/bootstrap/config.rs b/src/bootstrap/config.rs index 858fd59fc..03dfd9a2f 100644 --- a/src/bootstrap/config.rs +++ b/src/bootstrap/config.rs @@ -4,17 +4,6 @@ use torrust_tracker_configuration::{Configuration, Info}; -// Environment variables - -/// The whole `tracker.toml` file content. It has priority over the config file. -/// Even if the file is not on the default path. -const ENV_VAR_CONFIG: &str = "TORRUST_TRACKER_CONFIG"; -const ENV_VAR_API_ADMIN_TOKEN: &str = "TORRUST_TRACKER_API_ADMIN_TOKEN"; - -/// The `tracker.toml` file location. -pub const ENV_VAR_PATH_CONFIG: &str = "TORRUST_TRACKER_PATH_CONFIG"; - -// Default values pub const DEFAULT_PATH_CONFIG: &str = "./share/default/config/tracker.development.sqlite3.toml"; /// It loads the application configuration from the environment. @@ -34,15 +23,8 @@ pub const DEFAULT_PATH_CONFIG: &str = "./share/default/config/tracker.developmen /// `./tracker.toml` file or the env var `TORRUST_TRACKER_CONFIG`. #[must_use] pub fn initialize_configuration() -> Configuration { - let info = Info::new( - ENV_VAR_CONFIG.to_string(), - ENV_VAR_PATH_CONFIG.to_string(), - DEFAULT_PATH_CONFIG.to_string(), - ENV_VAR_API_ADMIN_TOKEN.to_string(), - ) - .unwrap(); - - Configuration::load(&info).unwrap() + let info = Info::new(DEFAULT_PATH_CONFIG.to_string()).expect("info to load configuration is not valid"); + Configuration::load(&info).expect("configuration should be loaded from provided info") } #[cfg(test)]