Merge pull request #121 from Kuadrant/validate_limits

Validate limits
Kuadrant · Sep 8, 2022 · 474bec2 · 474bec2
2 parents becfb44 + 7a4ea16
commit 474bec2
Show file tree

Hide file tree

Showing 4 changed files with 108 additions and 13 deletions.
diff --git a/limitador-server/src/http_api/mod.rs b/limitador-server/src/http_api/mod.rs
@@ -4,4 +4,6 @@
 #[allow(clippy::field_reassign_with_default)]
 mod request_types;
 
+pub use request_types::Limit as LimitVO;
+
 pub mod server;
diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs
@@ -205,16 +205,22 @@ impl Limiter {
             Ok(f) => {
                 let parsed_limits: Result<Vec<Limit>, _> = serde_yaml::from_reader(f);
                 match parsed_limits {
-                    Ok(limits) => {
-                        match &self {
-                            Self::Blocking(limiter) => limiter.configure_with(limits)?,
-                            Self::Async(limiter) => limiter.configure_with(limits).await?,
-                        }
-                        if limitador::limit::check_deprecated_syntax_usages_and_reset() {
-                            error!("You are using deprecated syntax for your conditions! See the migration guide https://kudrant.io/migration/limitador/constraints")
+                    Ok(limits) => match find_first_negative_limit(&limits) {
+                        None => {
+                            match &self {
+                                Self::Blocking(limiter) => limiter.configure_with(limits)?,
+                                Self::Async(limiter) => limiter.configure_with(limits).await?,
+                            }
+                            if limitador::limit::check_deprecated_syntax_usages_and_reset() {
+                                error!("You are using deprecated syntax for your conditions! See the migration guide https://kudrant.io/migration/limitador/constraints")
+                            }
+                            Ok(())
                         }
-                        Ok(())
-                    }
+                        Some(index) => Err(LimitadorServerError::ConfigFile(format!(
+                            ".[{}]: invalid value for `max_value`: positive integer expected",
+                            index
+                        ))),
+                    },
                     Err(e) => Err(LimitadorServerError::ConfigFile(format!(
                         "Couldn't parse: {}",
                         e
@@ -230,6 +236,15 @@ impl Limiter {
     }
 }
 
+fn find_first_negative_limit(limits: &[Limit]) -> Option<usize> {
+    for (index, limit) in limits.iter().enumerate() {
+        if limit.max_value() < 0 {
+            return Some(index);
+        }
+    }
+    None
+}
+
 #[actix_rt::main]
 async fn main() -> Result<(), Box<dyn std::error::Error>> {
     let config = {
@@ -463,6 +478,12 @@ fn create_config() -> (Configuration, String) {
                 .display_order(6)
                 .help("Sets the level of verbosity"),
         )
+        .arg(
+            Arg::with_name("validate")
+                .long("validate")
+                .display_order(7)
+                .help("Validates the LIMITS_FILE and exits"),
+        )
         .subcommand(
             SubCommand::with_name("memory")
                 .display_order(1)
@@ -553,6 +574,47 @@ fn create_config() -> (Configuration, String) {
     let matches = cmdline.get_matches();
 
     let limits_file = matches.value_of("LIMITS_FILE").unwrap();
+
+    if matches.is_present("validate") {
+        let error = match std::fs::File::open(limits_file) {
+            Ok(f) => {
+                let parsed_limits: Result<Vec<Limit>, _> = serde_yaml::from_reader(f);
+                match parsed_limits {
+                    Ok(limits) => match find_first_negative_limit(&limits) {
+                        Some(index) => LimitadorServerError::ConfigFile(format!(
+                            ".[{}]: invalid value for `max_value`: positive integer expected",
+                            index
+                        )),
+                        None => {
+                            if limitador::limit::check_deprecated_syntax_usages_and_reset() {
+                                eprintln!("Deprecated syntax for conditions corrected!\n")
+                            }
+
+                            let output: Vec<http_api::LimitVO> =
+                                limits.iter().map(|l| l.into()).collect();
+                            match serde_yaml::to_string(&output) {
+                                Ok(cfg) => {
+                                    println!("{}", cfg);
+                                }
+                                Err(err) => {
+                                    eprintln!("Config file is valid, but can't be output: {}", err);
+                                }
+                            }
+                            process::exit(0);
+                        }
+                    },
+                    Err(e) => LimitadorServerError::ConfigFile(format!("Couldn't parse: {}", e)),
+                }
+            }
+            Err(e) => LimitadorServerError::ConfigFile(format!(
+                "Couldn't read file '{}': {}",
+                limits_file, e
+            )),
+        };
+        eprintln!("{}", error);
+        process::exit(1);
+    }
+
     let storage = match matches.subcommand() {
         Some(("redis", sub)) => StorageConfiguration::Redis(RedisStorageConfiguration {
             url: sub.value_of("URL").unwrap().to_owned(),
@@ -661,3 +723,29 @@ fn env_option_is_enabled(env_name: &str) -> bool {
         Err(_) => false,
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::find_first_negative_limit;
+    use limitador::limit::Limit;
+
+    #[test]
+    fn finds_negative_limits() {
+        let variables: [&str; 0] = [];
+        let mut limits: Vec<Limit> = vec![
+            Limit::new::<_, &str>("foo", 42, 10, [], variables),
+            Limit::new::<_, &str>("foo", -42, 10, [], variables),
+        ];
+
+        assert_eq!(find_first_negative_limit(&limits), Some(1));
+        limits[0].set_max_value(-42);
+        assert_eq!(find_first_negative_limit(&limits), Some(0));
+        limits[1].set_max_value(42);
+        assert_eq!(find_first_negative_limit(&limits), Some(0));
+        limits[0].set_max_value(42);
+        assert_eq!(find_first_negative_limit(&limits), None);
+
+        let nothing: [Limit; 0] = [];
+        assert_eq!(find_first_negative_limit(&nothing), None);
+    }
+}
diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs
@@ -258,9 +258,14 @@ impl From<Condition> for String {
     fn from(condition: Condition) -> Self {
         let p = &condition.predicate;
         let predicate: String = p.clone().into();
+        let quotes = if condition.operand.contains('"') {
+            '\''
+        } else {
+            '"'
+        };
         format!(
-            "{} {} '{}'",
-            condition.var_name, predicate, condition.operand
+            "{} {} {}{}{}",
+            condition.var_name, predicate, quotes, condition.operand, quotes
         )
     }
 }
@@ -1005,6 +1010,6 @@ mod tests {
             operand: "ok".to_string(),
         };
         let result = serde_json::to_string(&condition).expect("Should serialize");
-        assert_eq!(result, r#""foobar == 'ok'""#.to_string());
+        assert_eq!(result, r#""foobar == \"ok\"""#.to_string());
     }
 }
diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs
@@ -63,7 +63,7 @@ mod tests {
             vec!["app_id"],
         );
         assert_eq!(
-            "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req.method == 'GET'\"],\"variables\":[\"app_id\"]}",
+            "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req.method == \\\"GET\\\"\"],\"variables\":[\"app_id\"]}",
             key_for_counters_of_limit(&limit))
     }
 }