diff --git a/source/river/assets/test-config.kdl b/source/river/assets/test-config.kdl index e40b645..eb3a91a 100644 --- a/source/river/assets/test-config.kdl +++ b/source/river/assets/test-config.kdl @@ -8,13 +8,14 @@ system { // Path to the pidfile used when daemonizing // - // NOTE: This has issues if you use relative paths. See issue https://github.com/memorysafety/river/issues/50 + // NOTE: This must be an absolute path. + // See issue https://github.com/memorysafety/river/issues/50 pid-file "/tmp/river.pidfile" // Path to upgrade socket // - // NOTE: `upgrade` is NOT exposed in the config file, it MUST be set on the CLI - // NOTE: This has issues if you use relative paths. See issue https://github.com/memorysafety/river/issues/50 + // NOTE: This must be an absolute path. + // See issue https://github.com/memorysafety/river/issues/50 // NOTE: The upgrade command is only supported on Linux upgrade-socket "/tmp/river-upgrade.sock" } diff --git a/source/river/src/config/internal.rs b/source/river/src/config/internal.rs index bc793cf..e9c0436 100644 --- a/source/river/src/config/internal.rs +++ b/source/river/src/config/internal.rs @@ -12,6 +12,7 @@ use pingora::{ server::configuration::{Opt as PingoraOpt, ServerConf as PingoraServerConf}, upstreams::peer::HttpPeer, }; +use tracing::warn; use crate::proxy::request_selector::{null_selector, RequestSelector}; @@ -53,14 +54,14 @@ impl Config { .pid_file .as_ref() .cloned() - .unwrap_or_else(|| PathBuf::from("river.pidfile")) + .unwrap_or_else(|| PathBuf::from("/tmp/river.pidfile")) .to_string_lossy() .into(), upgrade_sock: self .upgrade_socket .as_ref() .cloned() - .unwrap_or_else(|| PathBuf::from("river-upgrade.sock")) + .unwrap_or_else(|| PathBuf::from("/tmp/river-upgrade.sock")) .to_string_lossy() .into(), user: None, @@ -73,22 +74,37 @@ impl Config { } pub fn validate(&self) { - // TODO: validation logic + // This is currently mostly ad-hoc checks, we should potentially be a bit + // more systematic about this. if self.daemonize { - assert!( - self.pid_file.is_some(), - "Daemonize commanded but no pid file set!" - ); + if let Some(pf) = self.pid_file.as_ref() { + // NOTE: currently due to https://github.com/cloudflare/pingora/issues/331, + // we are not able to use relative paths. + assert!(pf.is_absolute(), "pid file path must be absolute, see https://github.com/cloudflare/pingora/issues/331"); + } else { + panic!("Daemonize commanded but no pid file set!"); + } + } else if let Some(pf) = self.pid_file.as_ref() { + if !pf.is_absolute() { + warn!("pid file path must be absolute. Currently: {:?}, see https://github.com/cloudflare/pingora/issues/331", pf); + } } if self.upgrade { assert!( cfg!(target_os = "linux"), "Upgrade is only supported on linux!" ); - assert!( - self.upgrade_socket.is_some(), - "Upgrade commanded but no upgrade socket set!" - ); + if let Some(us) = self.upgrade_socket.as_ref() { + // NOTE: currently due to https://github.com/cloudflare/pingora/issues/331, + // we are not able to use relative paths. + assert!(us.is_absolute(), "upgrade socket path must be absolute, see https://github.com/cloudflare/pingora/issues/331"); + } else { + panic!("Upgrade commanded but upgrade socket path not set!"); + } + } else if let Some(us) = self.upgrade_socket.as_ref() { + if !us.is_absolute() { + warn!("upgrade socket path must be absolute. Currently: {:?}, see https://github.com/cloudflare/pingora/issues/331", us); + } } } } diff --git a/source/river/src/config/mod.rs b/source/river/src/config/mod.rs index 426903d..ac628f9 100644 --- a/source/river/src/config/mod.rs +++ b/source/river/src/config/mod.rs @@ -67,6 +67,8 @@ pub fn render_config() -> internal::Config { tracing::info!("Applying CLI options"); apply_cli(&mut config, &c); + // We always validate the configuration - if the user selected "validate" + // then pingora will exit when IT also validates the config. tracing::info!(?config, "Full configuration",); tracing::info!("Validating..."); config.validate(); diff --git a/source/river/src/main.rs b/source/river/src/main.rs index 2ec0bc4..3b5696e 100644 --- a/source/river/src/main.rs +++ b/source/river/src/main.rs @@ -13,12 +13,6 @@ fn main() { // Read from the various configuration files let conf = config::render_config(); - // If the user asked to validate the configs - do it. This will also - // cause pingora to exit immediately when we start - if conf.validate_configs { - conf.validate(); - } - // Start the Server, which we will add services to. let mut my_server = Server::new_with_opt_and_conf(conf.pingora_opt(), conf.pingora_server_conf());