From 63fbbd69314d7e50665ac3b0fa2f32d47842d284 Mon Sep 17 00:00:00 2001
From: Oliver Gould <ver@buoyant.io>
Date: Tue, 8 May 2018 13:54:12 -0700
Subject: [PATCH] proxy: Parse units with duration configurations (#909)

Configuration values that take durations are currently specified as
time values with no units.  So `600` may mean 600ms in some contexts and
10 minutes in others.

In order to avoid this problem, this change now requires that
configurations provide explicit units for time values such as '600ms' or
10 minutes'.

Fixes #27.
---
 Cargo.lock                     |  12 ++--
 proxy/Cargo.toml               |   4 +-
 proxy/src/config.rs            | 117 +++++++++++++++++++++++++++++----
 proxy/src/lib.rs               |   1 +
 proxy/src/transport/connect.rs |   2 +-
 proxy/tests/discovery.rs       |   4 +-
 6 files changed, 118 insertions(+), 22 deletions(-)

diff --git a/Cargo.lock b/Cargo.lock
index d1215a97db..2b9e6c50f2 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -142,7 +142,7 @@ dependencies = [
  "prost-types 0.3.2 (registry+https://github.com/rust-lang/crates.io-index)",
  "quickcheck 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "rand 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)",
- "regex 0.2.10 (registry+https://github.com/rust-lang/crates.io-index)",
+ "regex 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "tokio-connect 0.1.0 (git+https://github.com/carllerche/tokio-connect)",
  "tokio-core 0.1.12 (registry+https://github.com/rust-lang/crates.io-index)",
  "tokio-io 0.1.5 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -767,19 +767,19 @@ dependencies = [
 
 [[package]]
 name = "regex"
-version = "0.2.10"
+version = "1.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "aho-corasick 0.6.4 (registry+https://github.com/rust-lang/crates.io-index)",
  "memchr 2.0.1 (registry+https://github.com/rust-lang/crates.io-index)",
- "regex-syntax 0.5.5 (registry+https://github.com/rust-lang/crates.io-index)",
+ "regex-syntax 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)",
  "thread_local 0.3.5 (registry+https://github.com/rust-lang/crates.io-index)",
  "utf8-ranges 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)",
 ]
 
 [[package]]
 name = "regex-syntax"
-version = "0.5.5"
+version = "0.6.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
 dependencies = [
  "ucd-util 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)",
@@ -1317,8 +1317,8 @@ dependencies = [
 "checksum rand 0.4.2 (registry+https://github.com/rust-lang/crates.io-index)" = "eba5f8cb59cc50ed56be8880a5c7b496bfd9bd26394e176bc67884094145c2c5"
 "checksum redox_syscall 0.1.37 (registry+https://github.com/rust-lang/crates.io-index)" = "0d92eecebad22b767915e4d529f89f28ee96dbbf5a4810d2b844373f136417fd"
 "checksum redox_termios 0.1.1 (registry+https://github.com/rust-lang/crates.io-index)" = "7e891cfe48e9100a70a3b6eb652fef28920c117d366339687bd5576160db0f76"
-"checksum regex 0.2.10 (registry+https://github.com/rust-lang/crates.io-index)" = "aec3f58d903a7d2a9dc2bf0e41a746f4530e0cab6b615494e058f67a3ef947fb"
-"checksum regex-syntax 0.5.5 (registry+https://github.com/rust-lang/crates.io-index)" = "bd90079345f4a4c3409214734ae220fd773c6f2e8a543d07370c6c1c369cfbfb"
+"checksum regex 1.0.0 (registry+https://github.com/rust-lang/crates.io-index)" = "75ecf88252dce580404a22444fc7d626c01815debba56a7f4f536772a5ff19d3"
+"checksum regex-syntax 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)" = "8f1ac0f60d675cc6cf13a20ec076568254472551051ad5dd050364d70671bf6b"
 "checksum relay 0.1.0 (registry+https://github.com/rust-lang/crates.io-index)" = "f301bafeb60867c85170031bdb2fcf24c8041f33aee09e7b116a58d4e9f781c5"
 "checksum remove_dir_all 0.5.0 (registry+https://github.com/rust-lang/crates.io-index)" = "dfc5b3ce5d5ea144bb04ebd093a9e14e9765bcfec866aecda9b6dec43b3d1e24"
 "checksum resolv-conf 0.6.0 (registry+https://github.com/rust-lang/crates.io-index)" = "8e1b086bb6a2659d6ba66e4aa21bde8a53ec03587cd5c80b83bdc3a330f35cab"
diff --git a/proxy/Cargo.toml b/proxy/Cargo.toml
index c32cc9a17f..9573f68464 100644
--- a/proxy/Cargo.toml
+++ b/proxy/Cargo.toml
@@ -29,6 +29,9 @@ log = "0.4.1"
 indexmap = "1.0.0"
 rand = "0.4"
 
+# for config parsing
+regex = "1.0.0"
+
 tokio-core = "0.1"
 tokio-io = "0.1"
 tokio-signal = "0.1"
@@ -57,5 +60,4 @@ libc = "0.2"
 [dev-dependencies]
 quickcheck = { version = "0.6", default-features = false }
 conduit-proxy-controller-grpc = { path = "./controller-grpc" , features = ["arbitrary"] }
-regex = "0.2"
 flate2 = { version = "1.0.1", default-features = false, features = ["rust_backend"] }
diff --git a/proxy/src/config.rs b/proxy/src/config.rs
index 97d6f8c8f8..e46c0d733c 100644
--- a/proxy/src/config.rs
+++ b/proxy/src/config.rs
@@ -86,16 +86,17 @@ pub enum Error {
     InvalidEnvVar
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq)]
 pub enum ParseError {
     EnvironmentUnsupported,
+    NotADuration,
     NotANumber,
     HostIsNotAnIpAddress,
     NotUnicode,
     UrlError(UrlError),
 }
 
-#[derive(Clone, Copy, Debug)]
+#[derive(Clone, Copy, Debug, PartialEq)]
 pub enum UrlError {
     /// The URl is syntactically invalid according to general URL parsing rules.
     SyntaxError,
@@ -195,16 +196,16 @@ impl<'a> TryFrom<&'a Strings> for Config {
         let control_listener_addr = parse(strings, ENV_CONTROL_LISTENER, str::parse);
         let metrics_listener_addr = parse(strings, ENV_METRICS_LISTENER, str::parse);
         let private_forward = parse(strings, ENV_PRIVATE_FORWARD, str::parse);
-        let public_connect_timeout = parse(strings, ENV_PUBLIC_CONNECT_TIMEOUT, parse_number);
-        let private_connect_timeout = parse(strings, ENV_PRIVATE_CONNECT_TIMEOUT, parse_number);
+        let public_connect_timeout = parse(strings, ENV_PUBLIC_CONNECT_TIMEOUT, parse_duration);
+        let private_connect_timeout = parse(strings, ENV_PRIVATE_CONNECT_TIMEOUT, parse_duration);
         let inbound_disable_ports = parse(strings, ENV_INBOUND_PORTS_DISABLE_PROTOCOL_DETECTION, parse_port_set);
         let outbound_disable_ports = parse(strings, ENV_OUTBOUND_PORTS_DISABLE_PROTOCOL_DETECTION, parse_port_set);
         let inbound_router_capacity = parse(strings, ENV_INBOUND_ROUTER_CAPACITY, parse_number);
         let outbound_router_capacity = parse(strings, ENV_OUTBOUND_ROUTER_CAPACITY, parse_number);
-        let bind_timeout = parse(strings, ENV_BIND_TIMEOUT, parse_number);
+        let bind_timeout = parse(strings, ENV_BIND_TIMEOUT, parse_duration);
         let resolv_conf_path = strings.get(ENV_RESOLV_CONF);
         let event_buffer_capacity = parse(strings, ENV_EVENT_BUFFER_CAPACITY, parse_number);
-        let metrics_retain_idle = parse(strings, ENV_METRICS_RETAIN_IDLE, parse_number);
+        let metrics_retain_idle = parse(strings, ENV_METRICS_RETAIN_IDLE, parse_duration);
         let pod_namespace = strings.get(ENV_POD_NAMESPACE).and_then(|maybe_value| {
             // There cannot be a default pod namespace, and the pod namespace is required.
             maybe_value.ok_or_else(|| {
@@ -243,9 +244,9 @@ impl<'a> TryFrom<&'a Strings> for Config {
             },
             private_forward: private_forward?,
 
-            public_connect_timeout: public_connect_timeout?.map(Duration::from_millis)
+            public_connect_timeout: public_connect_timeout?
                 .unwrap_or(DEFAULT_PUBLIC_CONNECT_TIMEOUT),
-            private_connect_timeout: private_connect_timeout?.map(Duration::from_millis)
+            private_connect_timeout: private_connect_timeout?
                 .unwrap_or(DEFAULT_PRIVATE_CONNECT_TIMEOUT),
 
             inbound_ports_disable_protocol_detection: inbound_disable_ports?
@@ -264,11 +265,9 @@ impl<'a> TryFrom<&'a Strings> for Config {
             control_host_and_port: control_host_and_port?,
 
             event_buffer_capacity: event_buffer_capacity?.unwrap_or(DEFAULT_EVENT_BUFFER_CAPACITY),
-            metrics_retain_idle: metrics_retain_idle?.map(Duration::from_secs)
-                .unwrap_or(DEFAULT_METRICS_RETAIN_IDLE),
+            metrics_retain_idle: metrics_retain_idle?.unwrap_or(DEFAULT_METRICS_RETAIN_IDLE),
 
-            bind_timeout: bind_timeout?.map(Duration::from_millis)
-                .unwrap_or(DEFAULT_BIND_TIMEOUT),
+            bind_timeout: bind_timeout?.unwrap_or(DEFAULT_BIND_TIMEOUT),
 
             pod_namespace: pod_namespace?,
         })
@@ -340,6 +339,27 @@ fn parse_number<T>(s: &str) -> Result<T, ParseError> where T: FromStr {
     s.parse().map_err(|_| ParseError::NotANumber)
 }
 
+fn parse_duration(s: &str) -> Result<Duration, ParseError> {
+    use regex::Regex;
+
+    let re = Regex::new(r"^\s*(\d+)(ms|s|m|h|d)?\s*$")
+        .expect("duration regex");
+
+    let cap = re.captures(s)
+        .ok_or(ParseError::NotADuration)?;
+
+    let magnitude = parse_number(&cap[1])?;
+    match cap.get(2).map(|m| m.as_str()) {
+        None if magnitude == 0 => Ok(Duration::from_secs(0)),
+        Some("ms") => Ok(Duration::from_millis(magnitude)),
+        Some("s") => Ok(Duration::from_secs(magnitude)),
+        Some("m") => Ok(Duration::from_secs(magnitude * 60)),
+        Some("h") => Ok(Duration::from_secs(magnitude * 60 * 60)),
+        Some("d") => Ok(Duration::from_secs(magnitude * 60 * 60 * 24)),
+        _ => Err(ParseError::NotADuration),
+    }
+}
+
 fn parse_url(s: &str) -> Result<HostAndPort, ParseError> {
     let url = s.parse::<http::Uri>().map_err(|_| ParseError::UrlError(UrlError::SyntaxError))?;
     if url.scheme_part().map(|s| s.as_str()) != Some("tcp") {
@@ -380,3 +400,76 @@ fn parse<T, Parse>(strings: &Strings, name: &str, parse: Parse) -> Result<Option
         None => Ok(None),
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn test_unit<F: Fn(u64) -> Duration>(unit: &str, to_duration: F) {
+        for v in &[0, 1, 23, 456_789] {
+            let d = to_duration(*v);
+            let text = format!("{}{}", v, unit);
+            assert_eq!(parse_duration(&text), Ok(d), "text=\"{}\"", text);
+
+            let text = format!(" {}{}\t", v, unit);
+            assert_eq!(parse_duration(&text), Ok(d), "text=\"{}\"", text);
+        }
+    }
+
+    #[test]
+    fn parse_duration_unit_ms() {
+        test_unit("ms", |v| Duration::from_millis(v));
+    }
+
+    #[test]
+    fn parse_duration_unit_s() {
+        test_unit("s", |v| Duration::from_secs(v));
+    }
+
+    #[test]
+    fn parse_duration_unit_m() {
+        test_unit("m", |v| Duration::from_secs(v * 60));
+    }
+
+    #[test]
+    fn parse_duration_unit_h() {
+        test_unit("h", |v| Duration::from_secs(v * 60 * 60));
+    }
+
+    #[test]
+    fn parse_duration_unit_d() {
+        test_unit("d", |v| Duration::from_secs(v * 60 * 60 * 24));
+    }
+
+    #[test]
+    fn parse_duration_floats_invalid() {
+        assert_eq!(parse_duration(".123h"), Err(ParseError::NotADuration));
+        assert_eq!(parse_duration("1.23h"), Err(ParseError::NotADuration));
+    }
+
+    #[test]
+    fn parse_duration_space_before_unit_invalid() {
+        assert_eq!(parse_duration("1 ms"), Err(ParseError::NotADuration));
+    }
+
+    #[test]
+    fn parse_duration_overflows_invalid() {
+        assert_eq!(parse_duration("123456789012345678901234567890ms"), Err(ParseError::NotANumber));
+    }
+
+    #[test]
+    fn parse_duration_invalid_unit() {
+        assert_eq!(parse_duration("12moons"), Err(ParseError::NotADuration));
+        assert_eq!(parse_duration("12y"), Err(ParseError::NotADuration));
+    }
+
+    #[test]
+    fn parse_duration_zero_without_unit() {
+        assert_eq!(parse_duration("0"), Ok(Duration::from_secs(0)));
+    }
+
+    #[test]
+    fn parse_duration_number_without_unit_is_invalid() {
+        assert_eq!(parse_duration("1"), Err(ParseError::NotADuration));
+    }
+}
diff --git a/proxy/src/lib.rs b/proxy/src/lib.rs
index 5dfae3c1e9..6b88e1c5cc 100644
--- a/proxy/src/lib.rs
+++ b/proxy/src/lib.rs
@@ -28,6 +28,7 @@ extern crate prost_types;
 #[macro_use]
 extern crate quickcheck;
 extern crate rand;
+extern crate regex;
 extern crate tokio_connect;
 extern crate tokio_core;
 extern crate tokio_io;
diff --git a/proxy/src/transport/connect.rs b/proxy/src/transport/connect.rs
index d82981d14f..b044ec535c 100644
--- a/proxy/src/transport/connect.rs
+++ b/proxy/src/transport/connect.rs
@@ -36,7 +36,7 @@ pub enum Host {
     Ip(IpAddr),
 }
 
-#[derive(Clone, Copy, Debug)]
+#[derive(Clone, Copy, Debug, PartialEq)]
 pub enum HostAndPortError {
     /// The host is not a valid DNS name or IP address.
     InvalidHost,
diff --git a/proxy/tests/discovery.rs b/proxy/tests/discovery.rs
index c446d15a0a..ab2e4c0c66 100644
--- a/proxy/tests/discovery.rs
+++ b/proxy/tests/discovery.rs
@@ -69,7 +69,7 @@ macro_rules! generate_tests {
             let mut env = config::TestEnv::new();
 
             // set the bind timeout to 100 ms.
-            env.put(config::ENV_BIND_TIMEOUT, "100".to_owned());
+            env.put(config::ENV_BIND_TIMEOUT, "100ms".to_owned());
 
             let srv = $make_server().route("/", "hello").run();
             let ctrl = controller::new();
@@ -111,7 +111,7 @@ macro_rules! generate_tests {
             let mut env = config::TestEnv::new();
 
             // set the bind timeout to 100 ms.
-            env.put(config::ENV_BIND_TIMEOUT, "100".to_owned());
+            env.put(config::ENV_BIND_TIMEOUT, "100ms".to_owned());
 
             let srv = $make_server().route("/hi", "hello").run();
             let ctrl = controller::new();