From 980107824a62ebbcd2a2e96a3bfe5b45f4b506cc Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Fri, 12 Jan 2024 11:21:38 -0500 Subject: [PATCH 01/12] Quick and dirty CEL support --- limitador/Cargo.toml | 2 + limitador/src/limit.rs | 240 ++++++++++++----------------------------- 2 files changed, 68 insertions(+), 174 deletions(-) diff --git a/limitador/Cargo.toml b/limitador/Cargo.toml index 0bb26894..c1fff3a4 100644 --- a/limitador/Cargo.toml +++ b/limitador/Cargo.toml @@ -33,6 +33,8 @@ async-trait = "0.1" cfg-if = "1" tracing = "0.1.40" metrics = "0.22.3" +cel-parser = "0.6.0" +cel-interpreter = "0.5.0" # Optional dependencies rocksdb = { version = "0.22", optional = true, features = ["multi-threaded-cf"] } diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 12adb7ff..66b4eb1a 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -1,6 +1,7 @@ -use crate::limit::conditions::{ErrorType, Literal, SyntaxError, Token, TokenType}; +use crate::limit::conditions::{ErrorType, SyntaxError, Token}; +use cel_interpreter::{Context, Expression, Value}; +use cel_parser::{parse, ParseError}; use serde::{Deserialize, Serialize, Serializer}; -use std::cmp::Ordering; use std::collections::{BTreeSet, HashMap, HashSet}; use std::error::Error; use std::fmt::{Debug, Display, Formatter}; @@ -66,12 +67,25 @@ pub struct Limit { variables: HashSet, } -#[derive(Deserialize, Serialize, PartialEq, Eq, Debug, Clone, Hash)] +#[derive(Deserialize, Serialize, Debug, Clone)] #[serde(try_from = "String", into = "String")] pub struct Condition { - var_name: String, - predicate: Predicate, - operand: String, + source: String, + expression: Expression, +} + +impl PartialEq for Condition { + fn eq(&self, other: &Self) -> bool { + self.expression == other.expression + } +} + +impl Eq for Condition {} + +impl Hash for Condition { + fn hash(&self, state: &mut H) { + self.source.hash(state) + } } #[derive(Debug)] @@ -104,146 +118,16 @@ impl TryFrom<&str> for Condition { impl TryFrom for Condition { type Error = ConditionParsingError; - fn try_from(value: String) -> Result { - match conditions::Scanner::scan(value.clone()) { - Ok(tokens) => match tokens.len().cmp(&(3_usize)) { - Ordering::Equal => { - match ( - &tokens[0].token_type, - &tokens[1].token_type, - &tokens[2].token_type, - ) { - ( - TokenType::Identifier, - TokenType::EqualEqual | TokenType::NotEqual, - TokenType::String, - ) => { - if let ( - Some(Literal::Identifier(var_name)), - Some(Literal::String(operand)), - ) = (&tokens[0].literal, &tokens[2].literal) - { - let predicate = match &tokens[1].token_type { - TokenType::EqualEqual => Predicate::Equal, - TokenType::NotEqual => Predicate::NotEqual, - _ => unreachable!(), - }; - Ok(Condition { - var_name: var_name.clone(), - predicate, - operand: operand.clone(), - }) - } else { - panic!( - "Unexpected state {tokens:?} returned from Scanner for: `{value}`" - ) - } - } - ( - TokenType::String, - TokenType::EqualEqual | TokenType::NotEqual, - TokenType::Identifier, - ) => { - if let ( - Some(Literal::String(operand)), - Some(Literal::Identifier(var_name)), - ) = (&tokens[0].literal, &tokens[2].literal) - { - let predicate = match &tokens[1].token_type { - TokenType::EqualEqual => Predicate::Equal, - TokenType::NotEqual => Predicate::NotEqual, - _ => unreachable!(), - }; - Ok(Condition { - var_name: var_name.clone(), - predicate, - operand: operand.clone(), - }) - } else { - panic!( - "Unexpected state {tokens:?} returned from Scanner for: `{value}`" - ) - } - } - #[cfg(feature = "lenient_conditions")] - (TokenType::Identifier, TokenType::EqualEqual, TokenType::Identifier) => { - if let ( - Some(Literal::Identifier(var_name)), - Some(Literal::Identifier(operand)), - ) = (&tokens[0].literal, &tokens[2].literal) - { - deprecated::deprecated_syntax_used(); - Ok(Condition { - var_name: var_name.clone(), - predicate: Predicate::Equal, - operand: operand.clone(), - }) - } else { - panic!( - "Unexpected state {tokens:?} returned from Scanner for: `{value}`" - ) - } - } - #[cfg(feature = "lenient_conditions")] - (TokenType::Identifier, TokenType::EqualEqual, TokenType::Number) => { - if let ( - Some(Literal::Identifier(var_name)), - Some(Literal::Number(operand)), - ) = (&tokens[0].literal, &tokens[2].literal) - { - deprecated::deprecated_syntax_used(); - Ok(Condition { - var_name: var_name.clone(), - predicate: Predicate::Equal, - operand: operand.to_string(), - }) - } else { - panic!( - "Unexpected state {tokens:?} returned from Scanner for: `{value}`" - ) - } - } - (t1, t2, _) => { - let faulty = match (t1, t2) { - ( - TokenType::Identifier | TokenType::String, - TokenType::EqualEqual | TokenType::NotEqual, - ) => 2, - (TokenType::Identifier | TokenType::String, _) => 1, - (_, _) => 0, - }; - Err(ConditionParsingError { - error: SyntaxError { - pos: tokens[faulty].pos, - error: ErrorType::UnexpectedToken(tokens[faulty].clone()), - }, - tokens, - condition: value, - }) - } - } - } - Ordering::Less => Err(ConditionParsingError { - error: SyntaxError { - pos: value.len(), - error: ErrorType::MissingToken, - }, - tokens, - condition: value, - }), - Ordering::Greater => Err(ConditionParsingError { - error: SyntaxError { - pos: tokens[3].pos, - error: ErrorType::UnexpectedToken(tokens[3].clone()), - }, - tokens, - condition: value, - }), - }, + fn try_from(source: String) -> Result { + match parse(&source) { + Ok(expression) => Ok(Condition { source, expression }), Err(err) => Err(ConditionParsingError { - error: err, - tokens: Vec::new(), - condition: value, + error: SyntaxError { + pos: 0, + error: ErrorType::MissingToken, + }, + tokens: vec![], + condition: source, }), } } @@ -251,17 +135,7 @@ impl TryFrom for Condition { impl From 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, quotes, condition.operand, quotes - ) + condition.source.clone() } } @@ -392,12 +266,14 @@ impl Limit { } fn condition_applies(condition: &Condition, values: &HashMap) -> bool { - let left_operand = condition.var_name.as_str(); - let right_operand = condition.operand.as_str(); + let mut context = Context::default(); + for (key, val) in values { + context.add_variable(key.clone(), val.to_owned()); + } - match values.get(left_operand) { - Some(val) => condition.predicate.test(val, right_operand), - None => false, + match Value::resolve(&condition.expression, &context) { + Ok(val) => val == true.into(), + Err(_) => false, } } } @@ -916,6 +792,24 @@ mod tests { assert!(limit.applies(&values)) } + #[test] + fn limit_applies_when_all_its_conditions_apply_with_subexpression() { + let limit = Limit::new( + "test_namespace", + 10, + 60, + vec!["x == string((11 - 1) / 2)", "y == \"2\""], + vec!["z"], + ); + + let mut values: HashMap = HashMap::new(); + values.insert("x".into(), "5".into()); + values.insert("y".into(), "2".into()); + values.insert("z".into(), "1".into()); + + assert!(limit.applies(&values)) + } + #[test] fn limit_does_not_apply_if_one_cond_doesnt() { let limit = Limit::new( @@ -940,9 +834,8 @@ mod tests { assert_eq!( result, Condition { - var_name: "x".to_string(), - predicate: Predicate::Equal, - operand: "5".to_string(), + source: "x == '5'".to_string(), + expression: parse("x == '5'").unwrap(), } ); @@ -951,9 +844,8 @@ mod tests { assert_eq!( result, Condition { - var_name: "foobar".to_string(), - predicate: Predicate::Equal, - operand: "ok".to_string(), + source: " foobar=='ok' ".to_string(), + expression: parse("foobar == 'ok'").unwrap(), } ); @@ -962,13 +854,13 @@ mod tests { assert_eq!( result, Condition { - var_name: "foobar".to_string(), - predicate: Predicate::Equal, - operand: "ok".to_string(), + source: " foobar == 'ok' ".to_string(), + expression: parse(" foobar == 'ok' ").unwrap(), } ); } + #[ignore] #[test] #[cfg(not(feature = "lenient_conditions"))] fn invalid_deprecated_condition_parsing() { @@ -977,9 +869,10 @@ mod tests { .expect("Should fail!"); } + #[ignore] #[test] fn invalid_condition_parsing() { - let result = serde_json::from_str::(r#""x != 5 && x > 12""#) + let result = serde_json::from_str::(r#""x != 5 ` x > 12""#) .expect_err("should fail parsing"); assert_eq!( result.to_string(), @@ -991,9 +884,8 @@ mod tests { #[test] fn condition_serialization() { let condition = Condition { - var_name: "foobar".to_string(), - predicate: Predicate::Equal, - operand: "ok".to_string(), + source: "foobar == \"ok\"".to_string(), + expression: parse("foobar == ok").unwrap(), }; let result = serde_json::to_string(&condition).expect("Should serialize"); assert_eq!(result, r#""foobar == \"ok\"""#.to_string()); From 5007c1539b7f23e037df10da12b51d6f721d4338 Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Wed, 5 Jun 2024 08:48:10 -0400 Subject: [PATCH 02/12] Bring back the original simple parsing, and only enable cel expression parsing if the condition has the `cel:` prefix. Signed-off-by: Hiram Chirino --- Cargo.lock | 241 ++++++++++++++++++++++++++++++++++ limitador/src/limit.rs | 190 +++++++++++++++++++++++++-- limitador/src/storage/keys.rs | 2 +- 3 files changed, 420 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 49b09ca5..935a159d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -234,6 +234,21 @@ dependencies = [ "alloc-no-stdlib", ] +[[package]] +name = "android-tzdata" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e999941b234f3131b00bc13c22d06e8c5ff726d1b6318ac7eb276997bbb4fef0" + +[[package]] +name = "android_system_properties" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "819e7219dbd41043ac279b19830f2efc897156490d7fd6ea916720117ee66311" +dependencies = [ + "libc", +] + [[package]] name = "anes" version = "0.1.6" @@ -300,6 +315,15 @@ version = "1.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "69f7f8c3906b62b754cd5326047894316021dcfe5a194c8ea52bdd94934a3457" +[[package]] +name = "ascii-canvas" +version = "3.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8824ecca2e851cec16968d54a01dd372ef8f95b244fb84b84e70128be347c3c6" +dependencies = [ + "term", +] + [[package]] name = "async-stream" version = "0.3.5" @@ -440,6 +464,21 @@ dependencies = [ "syn 2.0.59", ] +[[package]] +name = "bit-set" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" + [[package]] name = "bitflags" version = "1.3.2" @@ -536,6 +575,30 @@ dependencies = [ "libc", ] +[[package]] +name = "cel-interpreter" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "121ecc72c18bacd45494c0758bd2228d26764454d9b6cffe3ab88a850e5f568e" +dependencies = [ + "cel-parser", + "chrono", + "nom", + "paste", + "thiserror", +] + +[[package]] +name = "cel-parser" +version = "0.6.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b3a85ba148abbc551f1c32c8c0cff279dba234f6d38324bf372ba2395690879e" +dependencies = [ + "lalrpop", + "lalrpop-util", + "regex", +] + [[package]] name = "cexpr" version = "0.6.0" @@ -557,7 +620,12 @@ version = "0.4.38" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a21f936df1771bf62b77f047b726c4625ff2e8aa607c01ec06e5a05bd8463401" dependencies = [ + "android-tzdata", + "iana-time-zone", + "js-sys", "num-traits", + "wasm-bindgen", + "windows-targets 0.52.5", ] [[package]] @@ -851,6 +919,12 @@ dependencies = [ "syn 1.0.109", ] +[[package]] +name = "diff" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "56254986775e3233ffa9c4d7d3faaf6d36a2c09d30b20687e9f88bc8bafc16c8" + [[package]] name = "digest" version = "0.10.7" @@ -861,6 +935,27 @@ dependencies = [ "crypto-common", ] +[[package]] +name = "dirs-next" +version = "2.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b98cf8ebf19c3d1b223e151f99a4f9f0690dca41414773390fc824184ac833e1" +dependencies = [ + "cfg-if", + "dirs-sys-next", +] + +[[package]] +name = "dirs-sys-next" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4ebda144c4fe02d1f7ea1a7d9641b6fc6b580adcfa024ae48797ecdeb6825b4d" +dependencies = [ + "libc", + "redox_users", + "winapi", +] + [[package]] name = "either" version = "1.11.0" @@ -873,6 +968,15 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ef1a6892d9eef45c8fa6b9e0086428a2cca8491aca8f787c534a3d6d0bcb3ced" +[[package]] +name = "ena" +version = "0.14.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "3d248bdd43ce613d87415282f69b9bb99d947d290b10962dd6c56233312c2ad5" +dependencies = [ + "log", +] + [[package]] name = "encoding_rs" version = "0.8.34" @@ -1360,6 +1464,29 @@ dependencies = [ "tracing", ] +[[package]] +name = "iana-time-zone" +version = "0.1.60" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e7ffbb5a1b541ea2561f8c41c087286cc091e21e556a4f09a8f6cbf17b69b141" +dependencies = [ + "android_system_properties", + "core-foundation-sys", + "iana-time-zone-haiku", + "js-sys", + "wasm-bindgen", + "windows-core", +] + +[[package]] +name = "iana-time-zone-haiku" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f31827a206f56af32e590ba56d5d2d085f558508192593743f16b2306495269f" +dependencies = [ + "cc", +] + [[package]] name = "idna" version = "0.5.0" @@ -1489,6 +1616,37 @@ dependencies = [ "libc", ] +[[package]] +name = "lalrpop" +version = "0.19.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0a1cbf952127589f2851ab2046af368fd20645491bb4b376f04b7f94d7a9837b" +dependencies = [ + "ascii-canvas", + "bit-set", + "diff", + "ena", + "is-terminal", + "itertools 0.10.5", + "lalrpop-util", + "petgraph", + "regex", + "regex-syntax 0.6.29", + "string_cache", + "term", + "tiny-keccak", + "unicode-xid", +] + +[[package]] +name = "lalrpop-util" +version = "0.19.12" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3c48237b9604c5a4702de6b824e02006c3214327564636aef27c1028a8fa0ed" +dependencies = [ + "regex", +] + [[package]] name = "language-tags" version = "0.3.2" @@ -1523,6 +1681,16 @@ dependencies = [ "windows-targets 0.52.5", ] +[[package]] +name = "libredox" +version = "0.1.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c0ff37bd590ca25063e35af745c343cb7a0271906fb7b37e4813e8f79f00268d" +dependencies = [ + "bitflags 2.5.0", + "libc", +] + [[package]] name = "librocksdb-sys" version = "0.16.0+8.10.0" @@ -1556,6 +1724,8 @@ version = "0.8.0-dev" dependencies = [ "async-trait", "base64 0.22.1", + "cel-interpreter", + "cel-parser", "cfg-if", "criterion", "dashmap", @@ -1829,6 +1999,12 @@ dependencies = [ "tempfile", ] +[[package]] +name = "new_debug_unreachable" +version = "1.0.6" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "650eef8c711430f1a879fdd01d4745a7deea475becfb90269c06775983bbf086" + [[package]] name = "nom" version = "7.1.3" @@ -2199,6 +2375,15 @@ dependencies = [ "indexmap 2.2.6", ] +[[package]] +name = "phf_shared" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6796ad771acdc0123d2a88dc428b5e38ef24456743ddb1744ed628f9815c096" +dependencies = [ + "siphasher", +] + [[package]] name = "pin-project" version = "1.1.5" @@ -2295,6 +2480,12 @@ version = "0.2.17" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5b40af805b3121feab8a3c29f04d8ad262fa8e0561883e7653e024ae4479e6de" +[[package]] +name = "precomputed-hash" +version = "0.1.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "925383efa346730478fb4838dbe9137d2a47675ad789c546d150a6e1dd4ab31c" + [[package]] name = "prettyplease" version = "0.2.19" @@ -2530,6 +2721,17 @@ dependencies = [ "bitflags 1.3.2", ] +[[package]] +name = "redox_users" +version = "0.4.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bd283d9651eeda4b2a83a43c1c91b266c40fd76ecd39a50a8c630ae69dc72891" +dependencies = [ + "getrandom", + "libredox", + "thiserror", +] + [[package]] name = "regex" version = "1.10.4" @@ -2836,6 +3038,12 @@ dependencies = [ "libc", ] +[[package]] +name = "siphasher" +version = "0.3.11" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "38b58827f4464d87d377d175e90bf58eb00fd8716ff0a62f80356b5e61555d0d" + [[package]] name = "sketches-ddsketch" version = "0.2.2" @@ -2882,6 +3090,19 @@ version = "1.2.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a8f112729512f8e442d81f95a8a7ddf2b7c6b8a1a6f509a95864142b30cab2d3" +[[package]] +name = "string_cache" +version = "0.8.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f91138e76242f575eb1d3b38b4f1362f10d3a43f47d182a5b359af488a02293b" +dependencies = [ + "new_debug_unreachable", + "once_cell", + "parking_lot", + "phf_shared", + "precomputed-hash", +] + [[package]] name = "strsim" version = "0.11.1" @@ -2968,6 +3189,17 @@ dependencies = [ "windows-sys 0.52.0", ] +[[package]] +name = "term" +version = "0.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c59df8ac95d96ff9bede18eb7300b0fda5e5d8d90960e76f8e14ae765eedbf1f" +dependencies = [ + "dirs-next", + "rustversion", + "winapi", +] + [[package]] name = "thiserror" version = "1.0.58" @@ -3029,6 +3261,15 @@ dependencies = [ "time-core", ] +[[package]] +name = "tiny-keccak" +version = "2.0.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "2c9d3793400a45f954c52e73d068316d76b6f4e36977e3fcebb13a2721e80237" +dependencies = [ + "crunchy", +] + [[package]] name = "tinytemplate" version = "1.2.1" diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 66b4eb1a..17d8de8d 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -1,12 +1,14 @@ -use crate::limit::conditions::{ErrorType, SyntaxError, Token}; -use cel_interpreter::{Context, Expression, Value}; -use cel_parser::{parse, ParseError}; -use serde::{Deserialize, Serialize, Serializer}; +use crate::limit::conditions::{ErrorType, Literal, SyntaxError, Token, TokenType}; +use std::cmp::Ordering; use std::collections::{BTreeSet, HashMap, HashSet}; use std::error::Error; use std::fmt::{Debug, Display, Formatter}; use std::hash::{Hash, Hasher}; +use cel_interpreter::{Context, Expression, Value}; +use cel_parser::{Atom, parse}; +use cel_parser::RelationOp::{Equals, NotEquals}; +use serde::{Deserialize, Serialize, Serializer}; #[cfg(feature = "lenient_conditions")] mod deprecated { use std::sync::atomic::{AtomicBool, Ordering}; @@ -115,13 +117,12 @@ impl TryFrom<&str> for Condition { } } -impl TryFrom for Condition { - type Error = ConditionParsingError; +impl Condition { - fn try_from(source: String) -> Result { + fn try_from_cel(source: String) -> Result { match parse(&source) { Ok(expression) => Ok(Condition { source, expression }), - Err(err) => Err(ConditionParsingError { + Err(_err) => Err(ConditionParsingError { error: SyntaxError { pos: 0, error: ErrorType::MissingToken, @@ -131,6 +132,173 @@ impl TryFrom for Condition { }), } } + + fn try_from_simple(source: String) -> Result { + match conditions::Scanner::scan(source.clone()) { + Ok(tokens) => match tokens.len().cmp(&(3_usize)) { + Ordering::Equal => { + match ( + &tokens[0].token_type, + &tokens[1].token_type, + &tokens[2].token_type, + ) { + ( + TokenType::Identifier, + TokenType::EqualEqual | TokenType::NotEqual, + TokenType::String, + ) => { + if let ( + Some(Literal::Identifier(var_name)), + Some(Literal::String(operand)), + ) = (&tokens[0].literal, &tokens[2].literal) + { + let predicate = match &tokens[1].token_type { + TokenType::EqualEqual => Equals, + TokenType::NotEqual => NotEquals, + _ => unreachable!(), + }; + Ok(Condition { + source, + expression: Expression::Relation( + Box::new(Expression::Ident(var_name.clone().into())), + predicate, + Box::new(Expression::Atom(Atom::String(operand.clone().into()))), + ), + }) + } else { + panic!( + "Unexpected state {tokens:?} returned from Scanner for: `{source}`" + ) + } + } + ( + TokenType::String, + TokenType::EqualEqual | TokenType::NotEqual, + TokenType::Identifier, + ) => { + if let ( + Some(Literal::String(operand)), + Some(Literal::Identifier(var_name)), + ) = (&tokens[0].literal, &tokens[2].literal) + { + let predicate = match &tokens[1].token_type { + TokenType::EqualEqual => Equals, + TokenType::NotEqual => NotEquals, + _ => unreachable!(), + }; + Ok(Condition { + source, + expression: Expression::Relation( + Box::new(Expression::Atom(Atom::String(operand.clone().into()))), + predicate, + Box::new(Expression::Ident(var_name.clone().into())), + ), + }) + } else { + panic!( + "Unexpected state {tokens:?} returned from Scanner for: `{source}`" + ) + } + } + #[cfg(feature = "lenient_conditions")] + (TokenType::Identifier, TokenType::EqualEqual, TokenType::Identifier) => { + if let ( + Some(Literal::Identifier(var_name)), + Some(Literal::Identifier(operand)), + ) = (&tokens[0].literal, &tokens[2].literal) + { + deprecated::deprecated_syntax_used(); + Ok(Condition { + source, + expression: Expression::Relation( + Box::new(Expression::Ident(var_name.clone().into())), + Equals, + Box::new(Expression::Atom(Atom::String(operand.clone().into()))), + ), + }) + } else { + panic!( + "Unexpected state {tokens:?} returned from Scanner for: `{source}`" + ) + } + } + #[cfg(feature = "lenient_conditions")] + (TokenType::Identifier, TokenType::EqualEqual, TokenType::Number) => { + if let ( + Some(Literal::Identifier(var_name)), + Some(Literal::Number(operand)), + ) = (&tokens[0].literal, &tokens[2].literal) + { + deprecated::deprecated_syntax_used(); + Ok(Condition { + source, + expression: Expression::Relation( + Box::new(Expression::Ident(var_name.clone().into())), + Equals, + Box::new(Expression::Atom(Atom::String(operand.clone().into()))), + ), + }) + } else { + panic!( + "Unexpected state {tokens:?} returned from Scanner for: `{source}`" + ) + } + } + (t1, t2, _) => { + let faulty = match (t1, t2) { + ( + TokenType::Identifier | TokenType::String, + TokenType::EqualEqual | TokenType::NotEqual, + ) => 2, + (TokenType::Identifier | TokenType::String, _) => 1, + (_, _) => 0, + }; + Err(ConditionParsingError { + error: SyntaxError { + pos: tokens[faulty].pos, + error: ErrorType::UnexpectedToken(tokens[faulty].clone()), + }, + tokens, + condition: source, + }) + } + } + } + Ordering::Less => Err(ConditionParsingError { + error: SyntaxError { + pos: source.len(), + error: ErrorType::MissingToken, + }, + tokens, + condition: source, + }), + Ordering::Greater => Err(ConditionParsingError { + error: SyntaxError { + pos: tokens[3].pos, + error: ErrorType::UnexpectedToken(tokens[3].clone()), + }, + tokens, + condition: source, + }), + }, + Err(err) => Err(ConditionParsingError { + error: err, + tokens: Vec::new(), + condition: source, + }), + } + } +} + +impl TryFrom for Condition { + type Error = ConditionParsingError; + + fn try_from(value: String) -> Result { + if value.clone().starts_with("cel:") { + return Condition::try_from_cel(value.strip_prefix("cel:").unwrap().to_string()); + } + return Condition::try_from_simple(value); + } } impl From for String { @@ -798,7 +966,7 @@ mod tests { "test_namespace", 10, 60, - vec!["x == string((11 - 1) / 2)", "y == \"2\""], + vec!["cel:x == string((11 - 1) / 2)", "y == \"2\""], vec!["z"], ); @@ -860,7 +1028,6 @@ mod tests { ); } - #[ignore] #[test] #[cfg(not(feature = "lenient_conditions"))] fn invalid_deprecated_condition_parsing() { @@ -869,10 +1036,9 @@ mod tests { .expect("Should fail!"); } - #[ignore] #[test] fn invalid_condition_parsing() { - let result = serde_json::from_str::(r#""x != 5 ` x > 12""#) + let result = serde_json::from_str::(r#""x != 5 && x > 12""#) .expect_err("should fail parsing"); assert_eq!( result.to_string(), diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 6d32977c..8a26a0f7 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -99,7 +99,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)) } From c06300188d3d343eee2c114ebfc0a5d79f15aecb Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Wed, 5 Jun 2024 10:32:20 -0400 Subject: [PATCH 03/12] =?UTF-8?q?Don=E2=80=99t=20change=20the=20simple=20k?= =?UTF-8?q?ey=20encoding=20so=20that=20redis=20keys=20don=E2=80=99t=20chan?= =?UTF-8?q?ge.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- limitador/src/limit.rs | 33 +++++++++++++++++++++++++-------- limitador/src/storage/keys.rs | 2 +- 2 files changed, 26 insertions(+), 9 deletions(-) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 17d8de8d..79b572e0 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -6,7 +6,7 @@ use std::fmt::{Debug, Display, Formatter}; use std::hash::{Hash, Hasher}; use cel_interpreter::{Context, Expression, Value}; -use cel_parser::{Atom, parse}; +use cel_parser::{Atom, parse, RelationOp}; use cel_parser::RelationOp::{Equals, NotEquals}; use serde::{Deserialize, Serialize, Serializer}; #[cfg(feature = "lenient_conditions")] @@ -120,7 +120,7 @@ impl TryFrom<&str> for Condition { impl Condition { fn try_from_cel(source: String) -> Result { - match parse(&source) { + match parse(&source.strip_prefix("cel:").unwrap().to_string()) { Ok(expression) => Ok(Condition { source, expression }), Err(_err) => Err(ConditionParsingError { error: SyntaxError { @@ -133,6 +133,23 @@ impl Condition { } } + fn simple_source(var_name: String, op: RelationOp, lit: String) -> String { + let predicate = match op { + Equals => "==", + NotEquals => "!=", + _ => unreachable!(), + }; + let quotes = if lit.contains('"') { + '\'' + } else { + '"' + }; + format!( + "{} {} {}{}{}", + var_name, predicate, quotes, lit, quotes + ) + } + fn try_from_simple(source: String) -> Result { match conditions::Scanner::scan(source.clone()) { Ok(tokens) => match tokens.len().cmp(&(3_usize)) { @@ -158,7 +175,7 @@ impl Condition { _ => unreachable!(), }; Ok(Condition { - source, + source: Condition::simple_source(var_name.clone(), predicate.clone(), operand.clone()), expression: Expression::Relation( Box::new(Expression::Ident(var_name.clone().into())), predicate, @@ -187,7 +204,7 @@ impl Condition { _ => unreachable!(), }; Ok(Condition { - source, + source: Condition::simple_source(var_name.clone(), predicate.clone(), operand.clone()), expression: Expression::Relation( Box::new(Expression::Atom(Atom::String(operand.clone().into()))), predicate, @@ -209,7 +226,7 @@ impl Condition { { deprecated::deprecated_syntax_used(); Ok(Condition { - source, + source: Condition::simple_source(var_name.clone(), Equals, operand.clone()), expression: Expression::Relation( Box::new(Expression::Ident(var_name.clone().into())), Equals, @@ -231,11 +248,11 @@ impl Condition { { deprecated::deprecated_syntax_used(); Ok(Condition { - source, + source: Condition::simple_source(var_name.clone(), Equals, operand.to_string()), expression: Expression::Relation( Box::new(Expression::Ident(var_name.clone().into())), Equals, - Box::new(Expression::Atom(Atom::String(operand.clone().into()))), + Box::new(Expression::Atom(Atom::String(operand.to_string().into()))), ), }) } else { @@ -295,7 +312,7 @@ impl TryFrom for Condition { fn try_from(value: String) -> Result { if value.clone().starts_with("cel:") { - return Condition::try_from_cel(value.strip_prefix("cel:").unwrap().to_string()); + return Condition::try_from_cel(value); } return Condition::try_from_simple(value); } diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 8a26a0f7..6d32977c 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -99,7 +99,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)) } From c8ad128b5af39daad45278e2017dc49d60cd4d00 Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Wed, 5 Jun 2024 10:49:57 -0400 Subject: [PATCH 04/12] Drop dead code, and cargo fmt. Signed-off-by: Hiram Chirino --- limitador/src/limit.rs | 78 +++++++++++++++++++----------------------- 1 file changed, 35 insertions(+), 43 deletions(-) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 79b572e0..8688c0a2 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -6,8 +6,8 @@ use std::fmt::{Debug, Display, Formatter}; use std::hash::{Hash, Hasher}; use cel_interpreter::{Context, Expression, Value}; -use cel_parser::{Atom, parse, RelationOp}; use cel_parser::RelationOp::{Equals, NotEquals}; +use cel_parser::{parse, Atom, RelationOp}; use serde::{Deserialize, Serialize, Serializer}; #[cfg(feature = "lenient_conditions")] mod deprecated { @@ -118,7 +118,6 @@ impl TryFrom<&str> for Condition { } impl Condition { - fn try_from_cel(source: String) -> Result { match parse(&source.strip_prefix("cel:").unwrap().to_string()) { Ok(expression) => Ok(Condition { source, expression }), @@ -139,15 +138,8 @@ impl Condition { NotEquals => "!=", _ => unreachable!(), }; - let quotes = if lit.contains('"') { - '\'' - } else { - '"' - }; - format!( - "{} {} {}{}{}", - var_name, predicate, quotes, lit, quotes - ) + let quotes = if lit.contains('"') { '\'' } else { '"' }; + format!("{} {} {}{}{}", var_name, predicate, quotes, lit, quotes) } fn try_from_simple(source: String) -> Result { @@ -175,11 +167,17 @@ impl Condition { _ => unreachable!(), }; Ok(Condition { - source: Condition::simple_source(var_name.clone(), predicate.clone(), operand.clone()), + source: Condition::simple_source( + var_name.clone(), + predicate.clone(), + operand.clone(), + ), expression: Expression::Relation( Box::new(Expression::Ident(var_name.clone().into())), predicate, - Box::new(Expression::Atom(Atom::String(operand.clone().into()))), + Box::new(Expression::Atom(Atom::String( + operand.clone().into(), + ))), ), }) } else { @@ -204,9 +202,15 @@ impl Condition { _ => unreachable!(), }; Ok(Condition { - source: Condition::simple_source(var_name.clone(), predicate.clone(), operand.clone()), + source: Condition::simple_source( + var_name.clone(), + predicate.clone(), + operand.clone(), + ), expression: Expression::Relation( - Box::new(Expression::Atom(Atom::String(operand.clone().into()))), + Box::new(Expression::Atom(Atom::String( + operand.clone().into(), + ))), predicate, Box::new(Expression::Ident(var_name.clone().into())), ), @@ -226,11 +230,17 @@ impl Condition { { deprecated::deprecated_syntax_used(); Ok(Condition { - source: Condition::simple_source(var_name.clone(), Equals, operand.clone()), + source: Condition::simple_source( + var_name.clone(), + Equals, + operand.clone(), + ), expression: Expression::Relation( Box::new(Expression::Ident(var_name.clone().into())), Equals, - Box::new(Expression::Atom(Atom::String(operand.clone().into()))), + Box::new(Expression::Atom(Atom::String( + operand.clone().into(), + ))), ), }) } else { @@ -248,11 +258,17 @@ impl Condition { { deprecated::deprecated_syntax_used(); Ok(Condition { - source: Condition::simple_source(var_name.clone(), Equals, operand.to_string()), + source: Condition::simple_source( + var_name.clone(), + Equals, + operand.to_string(), + ), expression: Expression::Relation( Box::new(Expression::Ident(var_name.clone().into())), Equals, - Box::new(Expression::Atom(Atom::String(operand.to_string().into()))), + Box::new(Expression::Atom(Atom::String( + operand.to_string().into(), + ))), ), }) } else { @@ -324,30 +340,6 @@ impl From for String { } } -#[derive(PartialEq, Eq, Debug, Clone, Hash)] -pub enum Predicate { - Equal, - NotEqual, -} - -impl Predicate { - fn test(&self, lhs: &str, rhs: &str) -> bool { - match self { - Predicate::Equal => lhs == rhs, - Predicate::NotEqual => lhs != rhs, - } - } -} - -impl From for String { - fn from(op: Predicate) -> Self { - match op { - Predicate::Equal => "==".to_string(), - Predicate::NotEqual => "!=".to_string(), - } - } -} - fn ordered_condition_set(value: &HashSet, serializer: S) -> Result where S: Serializer, From d42e6d23f91e0244244e1a187e25a6c1aaeee4cb Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Wed, 5 Jun 2024 11:37:06 -0400 Subject: [PATCH 05/12] Add feature cel_conditions feature flag. Signed-off-by: Hiram Chirino --- limitador/Cargo.toml | 1 + limitador/src/limit.rs | 105 +++++++++++++++++++++++------------------ 2 files changed, 59 insertions(+), 47 deletions(-) diff --git a/limitador/Cargo.toml b/limitador/Cargo.toml index c1fff3a4..6cac8dac 100644 --- a/limitador/Cargo.toml +++ b/limitador/Cargo.toml @@ -18,6 +18,7 @@ disk_storage = ["rocksdb"] distributed_storage = ["tokio", "tokio-stream", "h2", "base64", "uuid", "tonic", "tonic-reflection", "prost", "prost-types"] redis_storage = ["redis", "r2d2", "tokio"] lenient_conditions = [] +cel_conditions = [] [dependencies] moka = { version = "0.12", features = ["sync"] } diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 8688c0a2..92210f77 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -1,4 +1,3 @@ -use crate::limit::conditions::{ErrorType, Literal, SyntaxError, Token, TokenType}; use std::cmp::Ordering; use std::collections::{BTreeSet, HashMap, HashSet}; use std::error::Error; @@ -6,9 +5,17 @@ use std::fmt::{Debug, Display, Formatter}; use std::hash::{Hash, Hasher}; use cel_interpreter::{Context, Expression, Value}; +#[cfg(feature = "cel_conditions")] +use cel_parser::parse; use cel_parser::RelationOp::{Equals, NotEquals}; -use cel_parser::{parse, Atom, RelationOp}; +use cel_parser::{Atom, RelationOp}; use serde::{Deserialize, Serialize, Serializer}; + +#[cfg(feature = "lenient_conditions")] +pub use deprecated::check_deprecated_syntax_usages_and_reset; + +use crate::limit::conditions::{ErrorType, Literal, SyntaxError, Token, TokenType}; + #[cfg(feature = "lenient_conditions")] mod deprecated { use std::sync::atomic::{AtomicBool, Ordering}; @@ -28,9 +35,6 @@ mod deprecated { } } -#[cfg(feature = "lenient_conditions")] -pub use deprecated::check_deprecated_syntax_usages_and_reset; - #[derive(Debug, Hash, Eq, PartialEq, Clone, Serialize, Deserialize)] pub struct Namespace(String); @@ -118,6 +122,7 @@ impl TryFrom<&str> for Condition { } impl Condition { + #[cfg(feature = "cel_conditions")] fn try_from_cel(source: String) -> Result { match parse(&source.strip_prefix("cel:").unwrap().to_string()) { Ok(expression) => Ok(Condition { source, expression }), @@ -172,12 +177,10 @@ impl Condition { predicate.clone(), operand.clone(), ), - expression: Expression::Relation( - Box::new(Expression::Ident(var_name.clone().into())), + expression: Self::simple_expression( + var_name.as_str(), predicate, - Box::new(Expression::Atom(Atom::String( - operand.clone().into(), - ))), + operand.as_str(), ), }) } else { @@ -207,12 +210,10 @@ impl Condition { predicate.clone(), operand.clone(), ), - expression: Expression::Relation( - Box::new(Expression::Atom(Atom::String( - operand.clone().into(), - ))), + expression: Self::simple_expression( + var_name.as_str(), predicate, - Box::new(Expression::Ident(var_name.clone().into())), + operand.as_str(), ), }) } else { @@ -235,12 +236,10 @@ impl Condition { Equals, operand.clone(), ), - expression: Expression::Relation( - Box::new(Expression::Ident(var_name.clone().into())), + expression: Self::simple_expression( + var_name.as_str(), Equals, - Box::new(Expression::Atom(Atom::String( - operand.clone().into(), - ))), + operand.as_str(), ), }) } else { @@ -263,12 +262,10 @@ impl Condition { Equals, operand.to_string(), ), - expression: Expression::Relation( - Box::new(Expression::Ident(var_name.clone().into())), + expression: Self::simple_expression( + var_name.as_str(), Equals, - Box::new(Expression::Atom(Atom::String( - operand.to_string().into(), - ))), + operand.to_string().as_str(), ), }) } else { @@ -321,12 +318,21 @@ impl Condition { }), } } + + fn simple_expression(ident: &str, op: RelationOp, lit: &str) -> Expression { + Expression::Relation( + Box::new(Expression::Ident(ident.to_string().into())), + op, + Box::new(Expression::Atom(Atom::String(lit.to_string().into()))), + ) + } } impl TryFrom for Condition { type Error = ConditionParsingError; fn try_from(value: String) -> Result { + #[cfg(feature = "cel_conditions")] if value.clone().starts_with("cel:") { return Condition::try_from_cel(value); } @@ -969,24 +975,6 @@ mod tests { assert!(limit.applies(&values)) } - #[test] - fn limit_applies_when_all_its_conditions_apply_with_subexpression() { - let limit = Limit::new( - "test_namespace", - 10, - 60, - vec!["cel:x == string((11 - 1) / 2)", "y == \"2\""], - vec!["z"], - ); - - let mut values: HashMap = HashMap::new(); - values.insert("x".into(), "5".into()); - values.insert("y".into(), "2".into()); - values.insert("z".into(), "1".into()); - - assert!(limit.applies(&values)) - } - #[test] fn limit_does_not_apply_if_one_cond_doesnt() { let limit = Limit::new( @@ -1012,7 +1000,7 @@ mod tests { result, Condition { source: "x == '5'".to_string(), - expression: parse("x == '5'").unwrap(), + expression: Condition::simple_expression("x", Equals, "5"), } ); @@ -1022,7 +1010,7 @@ mod tests { result, Condition { source: " foobar=='ok' ".to_string(), - expression: parse("foobar == 'ok'").unwrap(), + expression: Condition::simple_expression("foobar", Equals, "ok"), } ); @@ -1032,7 +1020,7 @@ mod tests { result, Condition { source: " foobar == 'ok' ".to_string(), - expression: parse(" foobar == 'ok' ").unwrap(), + expression: Condition::simple_expression("foobar", Equals, "ok"), } ); } @@ -1060,9 +1048,32 @@ mod tests { fn condition_serialization() { let condition = Condition { source: "foobar == \"ok\"".to_string(), - expression: parse("foobar == ok").unwrap(), + expression: Condition::simple_expression("foobar", Equals, "ok"), }; let result = serde_json::to_string(&condition).expect("Should serialize"); assert_eq!(result, r#""foobar == \"ok\"""#.to_string()); } + + #[cfg(feature = "cel_conditions")] + mod cel { + use super::*; + + #[test] + fn limit_applies_when_all_its_conditions_apply_with_subexpression() { + let limit = Limit::new( + "test_namespace", + 10, + 60, + vec!["cel:x == string((11 - 1) / 2)", "y == \"2\""], + vec!["z"], + ); + + let mut values: HashMap = HashMap::new(); + values.insert("x".into(), "5".into()); + values.insert("y".into(), "2".into()); + values.insert("z".into(), "1".into()); + + assert!(limit.applies(&values)) + } + } } From ff6d4cedfb11201e434009a6c202970696c9b7e5 Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Wed, 5 Jun 2024 11:53:28 -0400 Subject: [PATCH 06/12] Fix clippy warnings. Signed-off-by: Hiram Chirino --- limitador/src/limit.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 92210f77..f9654847 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -124,7 +124,7 @@ impl TryFrom<&str> for Condition { impl Condition { #[cfg(feature = "cel_conditions")] fn try_from_cel(source: String) -> Result { - match parse(&source.strip_prefix("cel:").unwrap().to_string()) { + match parse(source.strip_prefix("cel:").unwrap()) { Ok(expression) => Ok(Condition { source, expression }), Err(_err) => Err(ConditionParsingError { error: SyntaxError { @@ -336,7 +336,7 @@ impl TryFrom for Condition { if value.clone().starts_with("cel:") { return Condition::try_from_cel(value); } - return Condition::try_from_simple(value); + Condition::try_from_simple(value) } } From 954485f41b6dca0e57d9b9356dd7130b3567952d Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Wed, 5 Jun 2024 13:48:30 -0400 Subject: [PATCH 07/12] Add more cel tests, handle cases where the var variable name conflicts with cel built in expressions. Signed-off-by: Hiram Chirino --- limitador/src/limit.rs | 93 ++++++++++++++++++++++++++++++++++-------- 1 file changed, 77 insertions(+), 16 deletions(-) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index f9654847..0fa61aa8 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -8,7 +8,7 @@ use cel_interpreter::{Context, Expression, Value}; #[cfg(feature = "cel_conditions")] use cel_parser::parse; use cel_parser::RelationOp::{Equals, NotEquals}; -use cel_parser::{Atom, RelationOp}; +use cel_parser::{Atom, Member, RelationOp}; use serde::{Deserialize, Serialize, Serializer}; #[cfg(feature = "lenient_conditions")] @@ -321,7 +321,12 @@ impl Condition { fn simple_expression(ident: &str, op: RelationOp, lit: &str) -> Expression { Expression::Relation( - Box::new(Expression::Ident(ident.to_string().into())), + Box::new(Expression::Member( + Box::new(Expression::Ident("vars".to_string().into())), + Box::new(Member::Index(Box::new(Expression::Atom(Atom::String( + ident.to_string().into(), + ))))), + )), op, Box::new(Expression::Atom(Atom::String(lit.to_string().into()))), ) @@ -450,13 +455,16 @@ impl Limit { fn condition_applies(condition: &Condition, values: &HashMap) -> bool { let mut context = Context::default(); - for (key, val) in values { - context.add_variable(key.clone(), val.to_owned()); + + for (key, value) in values { + context.add_variable(key, value.clone()); } + context.add_variable("vars", values.clone()); + match Value::resolve(&condition.expression, &context) { Ok(val) => val == true.into(), - Err(_) => false, + Err(_err) => false, } } } @@ -1058,22 +1066,75 @@ mod tests { mod cel { use super::*; + fn limit_with_condition(conditions: Vec<&str>) -> Limit { + Limit::new("test_namespace", 10, 60, conditions, >::new()) + } + #[test] fn limit_applies_when_all_its_conditions_apply_with_subexpression() { - let limit = Limit::new( - "test_namespace", - 10, - 60, - vec!["cel:x == string((11 - 1) / 2)", "y == \"2\""], - vec!["z"], - ); + let limit = limit_with_condition(vec![ + r#"cel: x == string((11 - 1) / 2) "#, + r#" y == "2" "#, + ]); - let mut values: HashMap = HashMap::new(); - values.insert("x".into(), "5".into()); - values.insert("y".into(), "2".into()); - values.insert("z".into(), "1".into()); + let values = HashMap::from([ + ("x".to_string(), "5".to_string()), + ("y".to_string(), "2".to_string()), + ]); assert!(limit.applies(&values)) } + + #[test] + fn vars_with_dot_names() { + let values = HashMap::from([("req.method".to_string(), "GET".to_string())]); + + // we can't access complex variables via simple names.... + let limit = limit_with_condition(vec![r#"cel: req.method == "GET" "#]); + assert!(!limit.applies(&values)); + + // But we can access it via the vars map. + let limit = limit_with_condition(vec![r#"cel: vars["req.method"] == "GET" "#]); + assert!(limit.applies(&values)); + } + + #[test] + fn size_function() { + let values = HashMap::from([("method".to_string(), "GET".to_string())]); + let limit = limit_with_condition(vec![r#"cel: size(vars["method"]) == 3 "#]); + assert!(limit.applies(&values)); + } + + #[test] + fn size_function_and_size_var() { + let values = HashMap::from([ + ("method".to_string(), "GET".to_string()), + ("size".to_string(), "50".to_string()), + ]); + + let limit = limit_with_condition(vec![r#"cel: size(method) == 3 "#]); + assert!(limit.applies(&values)); + + // we can't access simple variables that conflict with built-ins + let limit = limit_with_condition(vec![r#"cel: size == "50" "#]); + assert!(!limit.applies(&values)); + + // But we can access it via the vars map. + let limit = limit_with_condition(vec![r#"cel: vars["size"] == "50" "#]); + assert!(limit.applies(&values)); + } + + #[test] + fn vars_var() { + let values = HashMap::from([("vars".to_string(), "hello".to_string())]); + + // we can't access simple variables that conflict with built-ins (the vars map) + let limit = limit_with_condition(vec![r#"cel: vars == "hello" "#]); + assert!(!limit.applies(&values)); + + // But we can access it via the vars map. + let limit = limit_with_condition(vec![r#"cel: vars["vars"] == "hello" "#]); + assert!(limit.applies(&values)); + } } } From bb0278ecbea42fc7f83e6e2de46a2751172ef855 Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Wed, 5 Jun 2024 19:06:07 -0400 Subject: [PATCH 08/12] Use an assert_false! Macro for better test readability Signed-off-by: Hiram Chirino --- limitador/src/limit.rs | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 0fa61aa8..a73fc091 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -886,6 +886,14 @@ mod conditions { mod tests { use super::*; + macro_rules! assert_false { + ($cond:expr $(,)?) => { + paste::item! { + assert!(!$cond) + } + }; + } + #[test] fn limit_can_have_an_optional_name() { let mut limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]); @@ -915,7 +923,7 @@ mod tests { values.insert("x".into(), "1".into()); values.insert("y".into(), "1".into()); - assert!(!limit.applies(&values)) + assert_false!(limit.applies(&values)) } #[test] @@ -927,9 +935,9 @@ mod tests { values.insert("x".into(), "1".into()); values.insert("y".into(), "1".into()); - assert!(!limit.applies(&values)); + assert_false!(limit.applies(&values)); assert!(check_deprecated_syntax_usages_and_reset()); - assert!(!check_deprecated_syntax_usages_and_reset()); + assert_false!(check_deprecated_syntax_usages_and_reset()); let limit = Limit::new("test_namespace", 10, 60, vec!["x == foobar"], vec!["y"]); @@ -939,7 +947,7 @@ mod tests { assert!(limit.applies(&values)); assert!(check_deprecated_syntax_usages_and_reset()); - assert!(!check_deprecated_syntax_usages_and_reset()); + assert_false!(check_deprecated_syntax_usages_and_reset()); } #[test] @@ -951,7 +959,7 @@ mod tests { values.insert("a".into(), "1".into()); values.insert("y".into(), "1".into()); - assert!(!limit.applies(&values)) + assert_false!(limit.applies(&values)) } #[test] @@ -962,7 +970,7 @@ mod tests { let mut values: HashMap = HashMap::new(); values.insert("x".into(), "5".into()); - assert!(!limit.applies(&values)) + assert_false!(limit.applies(&values)) } #[test] @@ -998,7 +1006,7 @@ mod tests { values.insert("y".into(), "2".into()); values.insert("z".into(), "1".into()); - assert!(!limit.applies(&values)) + assert_false!(limit.applies(&values)) } #[test] @@ -1091,7 +1099,7 @@ mod tests { // we can't access complex variables via simple names.... let limit = limit_with_condition(vec![r#"cel: req.method == "GET" "#]); - assert!(!limit.applies(&values)); + assert_false!(limit.applies(&values)); // But we can access it via the vars map. let limit = limit_with_condition(vec![r#"cel: vars["req.method"] == "GET" "#]); @@ -1117,7 +1125,7 @@ mod tests { // we can't access simple variables that conflict with built-ins let limit = limit_with_condition(vec![r#"cel: size == "50" "#]); - assert!(!limit.applies(&values)); + assert_false!(limit.applies(&values)); // But we can access it via the vars map. let limit = limit_with_condition(vec![r#"cel: vars["size"] == "50" "#]); @@ -1130,7 +1138,7 @@ mod tests { // we can't access simple variables that conflict with built-ins (the vars map) let limit = limit_with_condition(vec![r#"cel: vars == "hello" "#]); - assert!(!limit.applies(&values)); + assert_false!(limit.applies(&values)); // But we can access it via the vars map. let limit = limit_with_condition(vec![r#"cel: vars["vars"] == "hello" "#]); From 4f70031ef0c0dedbef161c8696aeac77c00a455e Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Wed, 5 Jun 2024 19:52:27 -0400 Subject: [PATCH 09/12] cel: reserve _* identifiers for future internal use like adding helper functions. Signed-off-by: Hiram Chirino --- limitador/src/limit.rs | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index a73fc091..3acee66b 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -457,6 +457,10 @@ impl Limit { let mut context = Context::default(); for (key, value) in values { + if key.starts_with('_') { + // reserve _* identifiers for future use. + continue; + } context.add_variable(key, value.clone()); } @@ -1144,5 +1148,18 @@ mod tests { let limit = limit_with_condition(vec![r#"cel: vars["vars"] == "hello" "#]); assert!(limit.applies(&values)); } + + #[test] + fn underscore_var() { + let values = HashMap::from([("_hello".to_string(), "world".to_string())]); + + // _* variables are reserved for future use + let limit = limit_with_condition(vec![r#"cel: _hello == "world" "#]); + assert_false!(limit.applies(&values)); + + // But we can access it via the vars map. + let limit = limit_with_condition(vec![r#"cel: vars["_hello"] == "world" "#]); + assert!(limit.applies(&values)); + } } } From 4ab013f3cc9deedd371fa902d1be5aeb423a867e Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Fri, 7 Jun 2024 10:51:09 -0400 Subject: [PATCH 10/12] Pickup updates cel 0.7.0 (patched) version. Signed-off-by: Hiram Chirino --- Cargo.lock | 9 ++++----- Cargo.toml | 5 +++++ limitador/Cargo.toml | 3 ++- limitador/src/limit.rs | 4 ++-- 4 files changed, 13 insertions(+), 8 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 935a159d..1dc783c5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -577,22 +577,21 @@ dependencies = [ [[package]] name = "cel-interpreter" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "121ecc72c18bacd45494c0758bd2228d26764454d9b6cffe3ab88a850e5f568e" +version = "0.7.0" +source = "git+https://github.com/chirino/cel-rust.git?branch=no-panic-compare#0cf3d6bdee9feb9a6a2107dc6795d09de6fff4cf" dependencies = [ "cel-parser", "chrono", "nom", "paste", + "serde", "thiserror", ] [[package]] name = "cel-parser" version = "0.6.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "b3a85ba148abbc551f1c32c8c0cff279dba234f6d38324bf372ba2395690879e" +source = "git+https://github.com/chirino/cel-rust.git?branch=no-panic-compare#0cf3d6bdee9feb9a6a2107dc6795d09de6fff4cf" dependencies = [ "lalrpop", "lalrpop-util", diff --git a/Cargo.toml b/Cargo.toml index 817ad31c..a24e8ab6 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,6 +2,11 @@ members = ["limitador", "limitador-server"] resolver = "2" + +[patch.crates-io] +cel-interpreter = { git = "https://github.com/chirino/cel-rust.git", branch = "no-panic-compare"} +cel-parser = { git = "https://github.com/chirino/cel-rust.git", branch = "no-panic-compare"} + [profile.release] lto = true codegen-units = 1 diff --git a/limitador/Cargo.toml b/limitador/Cargo.toml index 6cac8dac..942b93f5 100644 --- a/limitador/Cargo.toml +++ b/limitador/Cargo.toml @@ -35,7 +35,7 @@ cfg-if = "1" tracing = "0.1.40" metrics = "0.22.3" cel-parser = "0.6.0" -cel-interpreter = "0.5.0" +cel-interpreter = "0.7.0" # Optional dependencies rocksdb = { version = "0.22", optional = true, features = ["multi-threaded-cf"] } @@ -88,3 +88,4 @@ tonic-build = "0.11" name = "bench" path = "benches/bench.rs" harness = false + diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 3acee66b..9c0e79b6 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -461,10 +461,10 @@ impl Limit { // reserve _* identifiers for future use. continue; } - context.add_variable(key, value.clone()); + context.add_variable_from_value(key, value.clone()); } - context.add_variable("vars", values.clone()); + context.add_variable_from_value("vars", values.clone()); match Value::resolve(&condition.expression, &context) { Ok(val) => val == true.into(), From 93795b140baf3adbd10db02445dcc01fac39fd8a Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Fri, 7 Jun 2024 14:45:34 -0400 Subject: [PATCH 11/12] Improve assert_false! Macro. Signed-off-by: Hiram Chirino --- limitador/src/limit.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 9c0e79b6..171dc9ab 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -893,7 +893,7 @@ mod tests { macro_rules! assert_false { ($cond:expr $(,)?) => { paste::item! { - assert!(!$cond) + assert!(!$cond, "assertion failed: assert_false!({}) was true!", stringify!($cond)) } }; } From 45958e0088687af066e13e35cf29599e5630d8c7 Mon Sep 17 00:00:00 2001 From: Hiram Chirino Date: Sat, 8 Jun 2024 07:54:29 -0400 Subject: [PATCH 12/12] Show an alternative way to access the cel variable. Signed-off-by: Hiram Chirino --- limitador/src/limit.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 171dc9ab..88b133ef 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -1147,6 +1147,10 @@ mod tests { // But we can access it via the vars map. let limit = limit_with_condition(vec![r#"cel: vars["vars"] == "hello" "#]); assert!(limit.applies(&values)); + + // Or via the vars map with dot notation. + let limit = limit_with_condition(vec![r#"cel: vars.vars == "hello" "#]); + assert!(limit.applies(&values)); } #[test]