Proxy: Use production config parsing in tests (#25)

* Proxy: Use production config parsing in tests

Previosuly the testing code for the proxy was sensitive to the values
of environment variables unintentionally, because `Config` looked at
the environment variables. Also, the tests were largely avoiding
testing the production configuration parsing code since they were
doing their own parsing.

Now the tests avoid looking at environment variables other than
`ENV_LOG`, which makes them more resilient. Also the tests now parse
the settings using the same code as production use uses.

I validated this manually.

proxy/src/app.rs

@@ -1,7 +1,9 @@
-use config::{self, Config};
+use config::{self, Config, Env};
+use convert::TryFrom;
 use logging;
 
 pub fn init() -> Result<Config, config::Error> {
     logging::init();
-    Config::load_from_env()
+    let config_strings = Env;
+    Config::try_from(&config_strings)
 }

proxy/src/config.rs

@@ -1,3 +1,4 @@
+use std::collections::HashMap;
 use std::env;
 use std::net::SocketAddr;
 use std::path::PathBuf;
@@ -6,6 +7,8 @@ use std::time::Duration;
 
 use url::{Host, HostAndPort, Url};
 
+use convert::TryFrom;
+
 // TODO:
 //
 // * Make struct fields private.
@@ -96,14 +99,29 @@ pub enum UrlError {
     FragmentNotAllowed,
 }
 
+/// The strings used to build a configuration.
+pub trait Strings {
+    /// Retrieves the value for the key `key`.
+    ///
+    /// `key` must be one of the `ENV_` values below.
+    fn get(&self, key: &str) -> Result<Option<String>, Error>;
+}
+
+/// An implementation of `Strings` that reads the values from environment variables.
+pub struct Env;
+
+pub struct TestEnv {
+    values: HashMap<&'static str, String>
+}
+
 // Environment variables to look at when loading the configuration
 const ENV_EVENT_BUFFER_CAPACITY: &str = "CONDUIT_PROXY_EVENT_BUFFER_CAPACITY";
-const ENV_METRICS_FLUSH_INTERVAL_SECS: &str = "CONDUIT_PROXY_METRICS_FLUSH_INTERVAL_SECS";
+pub const ENV_METRICS_FLUSH_INTERVAL_SECS: &str = "CONDUIT_PROXY_METRICS_FLUSH_INTERVAL_SECS";
 const ENV_REPORT_TIMEOUT_SECS: &str = "CONDUIT_PROXY_REPORT_TIMEOUT_SECS";
-const ENV_PRIVATE_LISTENER: &str = "CONDUIT_PROXY_PRIVATE_LISTENER";
+pub const ENV_PRIVATE_LISTENER: &str = "CONDUIT_PROXY_PRIVATE_LISTENER";
-const ENV_PRIVATE_FORWARD: &str = "CONDUIT_PROXY_PRIVATE_FORWARD";
+pub const ENV_PRIVATE_FORWARD: &str = "CONDUIT_PROXY_PRIVATE_FORWARD";
-const ENV_PUBLIC_LISTENER: &str = "CONDUIT_PROXY_PUBLIC_LISTENER";
+pub const ENV_PUBLIC_LISTENER: &str = "CONDUIT_PROXY_PUBLIC_LISTENER";
-const ENV_CONTROL_LISTENER: &str = "CONDUIT_PROXY_CONTROL_LISTENER";
+pub const ENV_CONTROL_LISTENER: &str = "CONDUIT_PROXY_CONTROL_LISTENER";
 const ENV_PRIVATE_CONNECT_TIMEOUT: &str = "CONDUIT_PROXY_PRIVATE_CONNECT_TIMEOUT";
 const ENV_PUBLIC_CONNECT_TIMEOUT: &str = "CONDUIT_PROXY_PUBLIC_CONNECT_TIMEOUT";
 
@@ -112,7 +130,7 @@ pub const ENV_NODE_NAME: &str = "CONDUIT_PROXY_NODE_NAME";
 pub const ENV_POD_NAME: &str = "CONDUIT_PROXY_POD_NAME";
 pub const ENV_POD_NAMESPACE: &str = "CONDUIT_PROXY_POD_NAMESPACE";
 
-const ENV_CONTROL_URL: &str = "CONDUIT_PROXY_CONTROL_URL";
+pub const ENV_CONTROL_URL: &str = "CONDUIT_PROXY_CONTROL_URL";
 const ENV_RESOLV_CONF: &str = "CONDUIT_RESOLV_CONF";
 
 // Default values for various configuration fields
@@ -127,24 +145,25 @@ const DEFAULT_RESOLV_CONF: &str = "/etc/resolv.conf";
 
 // ===== impl Config =====
 
-impl Config {
+impl<'a> TryFrom<&'a Strings> for Config {
+    type Err = Error;
     /// Load a `Config` by reading ENV variables.
-    pub fn load_from_env() -> Result<Self, Error> {
+    fn try_from(strings: &Strings) -> Result<Self, Self::Err> {
         // Parse all the environment variables. `env_var` and `env_var_parse`
         // will log any errors so defer returning any errors until all of them
         // have been parsed.
-        let private_listener_addr = env_var_parse(ENV_PRIVATE_LISTENER, str::parse);
+        let private_listener_addr = parse(strings, ENV_PRIVATE_LISTENER, str::parse);
-        let public_listener_addr = env_var_parse(ENV_PUBLIC_LISTENER, str::parse);
+        let public_listener_addr = parse(strings, ENV_PUBLIC_LISTENER, str::parse);
-        let control_listener_addr = env_var_parse(ENV_CONTROL_LISTENER, str::parse);
+        let control_listener_addr = parse(strings, ENV_CONTROL_LISTENER, str::parse);
-        let private_forward = env_var_parse(ENV_PRIVATE_FORWARD, str::parse);
+        let private_forward = parse(strings, ENV_PRIVATE_FORWARD, str::parse);
-        let public_connect_timeout = env_var_parse(ENV_PUBLIC_CONNECT_TIMEOUT, parse_number);
+        let public_connect_timeout = parse(strings, ENV_PUBLIC_CONNECT_TIMEOUT, parse_number);
-        let private_connect_timeout = env_var_parse(ENV_PRIVATE_CONNECT_TIMEOUT, parse_number);
+        let private_connect_timeout = parse(strings, ENV_PRIVATE_CONNECT_TIMEOUT, parse_number);
-        let resolv_conf_path = env_var(ENV_RESOLV_CONF);
+        let resolv_conf_path = strings.get(ENV_RESOLV_CONF);
-        let control_host_and_port = env_var_parse(ENV_CONTROL_URL, parse_url);
+        let control_host_and_port = parse(strings, ENV_CONTROL_URL, parse_url);
-        let event_buffer_capacity = env_var_parse(ENV_EVENT_BUFFER_CAPACITY, parse_number);
+        let event_buffer_capacity = parse(strings, ENV_EVENT_BUFFER_CAPACITY, parse_number);
         let metrics_flush_interval_secs =
-            env_var_parse(ENV_METRICS_FLUSH_INTERVAL_SECS, parse_number);
+            parse(strings, ENV_METRICS_FLUSH_INTERVAL_SECS, parse_number);
-        let report_timeout = env_var_parse(ENV_REPORT_TIMEOUT_SECS, parse_number);
+        let report_timeout = parse(strings, ENV_REPORT_TIMEOUT_SECS, parse_number);
 
         Ok(Config {
             private_listener: Listener {
@@ -207,6 +226,43 @@ impl From<Addr> for SocketAddr {
     }
 }
 
+// ===== impl Env =====
+
+impl Strings for Env {
+    fn get(&self, key: &str) -> Result<Option<String>, Error> {
+        match env::var(key) {
+            Ok(value) => Ok(Some(value)),
+            Err(env::VarError::NotPresent) => Ok(None),
+            Err(env::VarError::NotUnicode(_)) => {
+                error!("{} is not encoded in Unicode", key);
+                Err(Error::InvalidEnvVar)
+            }
+        }
+    }
+}
+
+// ===== impl TestEnv =====
+
+impl TestEnv {
+    pub fn new() -> Self {
+        Self {
+            values: Default::default(),
+        }
+    }
+
+    pub fn put(&mut self, key: &'static str, value: String) {
+        self.values.insert(key, value);
+    }
+}
+
+impl Strings for TestEnv {
+    fn get(&self, key: &str) -> Result<Option<String>, Error> {
+        Ok(self.values.get(key).cloned())
+    }
+}
+
+// ===== Parsing =====
+
 fn parse_number<T>(s: &str) -> Result<T, ParseError> where T: FromStr {
     s.parse().map_err(|_| ParseError::NotANumber)
 }
@@ -232,20 +288,9 @@ fn parse_url(s: &str) -> Result<HostAndPort, ParseError> {
     })
 }
 
-fn env_var(name: &str) -> Result<Option<String>, Error> {
+fn parse<T, Parse>(strings: &Strings, name: &str, parse: Parse) -> Result<Option<T>, Error>
-    match env::var(name) {
-        Ok(value) => Ok(Some(value)),
-        Err(env::VarError::NotPresent) => Ok(None),
-        Err(env::VarError::NotUnicode(_)) => {
-            error!("{} is not encoded in Unicode", name);
-            Err(Error::InvalidEnvVar)
-        }
-    }
-}
-
-fn env_var_parse<T, Parse>(name: &str, parse: Parse) -> Result<Option<T>, Error>
     where Parse: FnOnce(&str) -> Result<T, ParseError> {
-    match env_var(name)? {
+    match strings.get(name)? {
         Some(ref s) => {
             let r = parse(s).map_err(|parse_error| {
                 error!("{} is not valid: {:?}", name, parse_error);

proxy/src/lib.rs

@@ -60,7 +60,7 @@ mod bind;
 pub mod config;
 mod connection;
 pub mod control;
-mod convert;
+pub mod convert;
 mod ctx;
 mod dns;
 mod inbound;

proxy/tests/support/proxy.rs

@@ -1,5 +1,7 @@
 use support::*;
 
+use support::conduit_proxy::convert::TryFrom;
+
 pub fn new() -> Proxy {
     Proxy::new()
 }
@@ -59,36 +61,27 @@ impl Proxy {
 }
 
 fn run(proxy: Proxy) -> Listening {
+    use self::conduit_proxy::config;
+
     let controller = proxy.controller.expect("proxy controller missing");
     let inbound = proxy.inbound;
     let outbound = proxy.outbound;
 
-    let mut config = conduit_proxy::config::Config::load_from_env().unwrap();
+    let mut env = config::TestEnv::new();
-
+    env.put(config::ENV_CONTROL_URL, format!("tcp://{}", controller.addr));
-    config.control_host_and_port = {
+    env.put(config::ENV_PRIVATE_LISTENER, "tcp://127.0.0.1:0".to_owned());
-        let control_url: url::Url = format!("tcp://{}", controller.addr).parse().unwrap();
-        url::HostAndPort {
-            host: control_url.host().unwrap().to_owned(),
-            port: control_url.port().unwrap(),
-        }
-    };
-
-    config.private_listener = conduit_proxy::config::Listener {
-        addr: "tcp://127.0.0.1:0".parse().unwrap(),
-    };
-
     if let Some(ref inbound) = inbound {
-        config.private_forward = format!("tcp://{}", inbound.addr).parse().ok();
+        env.put(config::ENV_PRIVATE_FORWARD, format!("tcp://{}", inbound.addr));
     }
+    env.put(config::ENV_PUBLIC_LISTENER, "tcp://127.0.0.1:0".to_owned());
+    env.put(config::ENV_CONTROL_LISTENER, "tcp://127.0.0.1:0".to_owned());
 
-    config.public_listener = conduit_proxy::config::Listener {
+    let mut config = config::Config::try_from(&env).unwrap();
-        addr: "tcp://127.0.0.1:0".parse().unwrap(),
-    };
-
-    config.control_listener = conduit_proxy::config::Listener {
-        addr: "tcp://127.0.0.1:0".parse().unwrap(),
-    };
 
+    // TODO: We currently can't use `config::ENV_METRICS_FLUSH_INTERVAL_SECS`
+    // because we need to be able to set the flush interval to a fraction of a
+    // second. We should change config::ENV_METRICS_FLUSH_INTERVAL_SECS so that
+    // it can support this.
     if let Some(dur) = proxy.metrics_flush_interval {
         config.metrics_flush_interval = dur;
     }