Enforce absolute path (#53)

This PR adds some enforcement and logging for non-absolute paths. This is to mitigate cloudflare/pingora#331 until there is a resolution for it.

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"
 }

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);
+            }
         }
     }
 }

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();

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());