From d4296ec83110cbba7553abbc0319c1d450b99f16 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 26 Nov 2024 13:14:38 -0500 Subject: [PATCH 01/20] Basic cel::Expression & ::Predicate impl Signed-off-by: Alex Snaps --- Cargo.lock | 178 +++++++++++++++++++++++++++++++++++++ limitador/Cargo.toml | 2 + limitador/src/limit.rs | 6 ++ limitador/src/limit/cel.rs | 162 +++++++++++++++++++++++++++++++++ 4 files changed, 348 insertions(+) create mode 100644 limitador/src/limit/cel.rs diff --git a/Cargo.lock b/Cargo.lock index ab170a96..6ed6c974 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -305,6 +305,15 @@ version = "1.7.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "69f7f8c3906b62b754cd5326047894316021dcfe5a194c8ea52bdd94934a3457" +[[package]] +name = "ascii-canvas" +version = "4.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ef1e3e699d84ab1b0911a1010c5c106aa34ae89aeac103be5ce0c3859db1e891" +dependencies = [ + "term", +] + [[package]] name = "async-stream" version = "0.3.6" @@ -498,6 +507,21 @@ dependencies = [ "syn 2.0.89", ] +[[package]] +name = "bit-set" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "08807e080ed7f9d5433fa9b275196cfc35414f66a0c79d864dc51a0d825231a3" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5e764a1d40d510daf35e07be9eb06e75770908c27d411ee6c92109c9840eaaf7" + [[package]] name = "bitflags" version = "1.3.2" @@ -595,6 +619,33 @@ dependencies = [ "shlex", ] +[[package]] +name = "cel-interpreter" +version = "0.9.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5675bdc8ece076b0a4f5ac6c20724e7373919ed698e00dbf33825682a7b3a679" +dependencies = [ + "cel-parser", + "chrono", + "nom", + "paste", + "regex", + "serde", + "thiserror 1.0.69", +] + +[[package]] +name = "cel-parser" +version = "0.8.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6e1df6c220727ff1f7b52a41699d8c71ec52e8ae58d01a9768469a916e1f22eb" +dependencies = [ + "lalrpop", + "lalrpop-util", + "regex", + "thiserror 1.0.69", +] + [[package]] name = "cexpr" version = "0.6.0" @@ -950,6 +1001,15 @@ version = "0.6.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "edd0f118536f44f5ccd48bcb8b111bdc3de888b58c74639dfb034a357d0f206d" +[[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.35" @@ -1286,6 +1346,15 @@ version = "0.4.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "fbf6a919d6cf397374f7dfeeea91d974c7c0a7221d0d0f4f20d859d329e53fcc" +[[package]] +name = "home" +version = "0.5.9" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e3d1354bf6b7235cb4a0576c2619fd4ed18183f689b12b006a0ee7329eeff9a5" +dependencies = [ + "windows-sys 0.52.0", +] + [[package]] name = "http" version = "0.2.12" @@ -1727,6 +1796,15 @@ dependencies = [ "wasm-bindgen", ] +[[package]] +name = "keccak" +version = "0.1.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ecc2af9a1119c51f12a14607e783cb977bde58bc069ff0c3da1095e635d70654" +dependencies = [ + "cpufeatures", +] + [[package]] name = "kqueue" version = "1.0.8" @@ -1747,6 +1825,38 @@ dependencies = [ "libc", ] +[[package]] +name = "lalrpop" +version = "0.22.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "06093b57658c723a21da679530e061a8c25340fa5a6f98e313b542268c7e2a1f" +dependencies = [ + "ascii-canvas", + "bit-set", + "ena", + "itertools 0.13.0", + "lalrpop-util", + "petgraph", + "pico-args", + "regex", + "regex-syntax 0.8.5", + "sha3", + "string_cache", + "term", + "unicode-xid", + "walkdir", +] + +[[package]] +name = "lalrpop-util" +version = "0.22.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "feee752d43abd0f4807a921958ab4131f692a44d4d599733d4419c5d586176ce" +dependencies = [ + "regex-automata 0.4.9", + "rustversion", +] + [[package]] name = "language-tags" version = "0.3.2" @@ -1825,6 +1935,8 @@ version = "0.8.0-dev" dependencies = [ "async-trait", "base64 0.22.1", + "cel-interpreter", + "cel-parser", "cfg-if", "criterion", "dashmap", @@ -2109,6 +2221,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" @@ -2507,6 +2625,21 @@ dependencies = [ "indexmap 2.6.0", ] +[[package]] +name = "phf_shared" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b6796ad771acdc0123d2a88dc428b5e38ef24456743ddb1744ed628f9815c096" +dependencies = [ + "siphasher", +] + +[[package]] +name = "pico-args" +version = "0.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "5be167a7af36ee22fe3115051bc51f6e6c7054c9348e28deb4f49bd6f705a315" + [[package]] name = "pin-project" version = "1.1.7" @@ -2607,6 +2740,12 @@ dependencies = [ "zerocopy", ] +[[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.25" @@ -3146,6 +3285,16 @@ version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "bbfa15b3dddfee50a0fff136974b3e1bde555604ba463834a7eb7deb6417705d" +[[package]] +name = "sha3" +version = "0.10.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75872d278a8f37ef87fa0ddbda7802605cb18344497949862c0d4dcb291eba60" +dependencies = [ + "digest", + "keccak", +] + [[package]] name = "sharded-slab" version = "0.1.7" @@ -3170,6 +3319,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" @@ -3216,6 +3371,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" @@ -3319,6 +3487,16 @@ dependencies = [ "windows-sys 0.59.0", ] +[[package]] +name = "term" +version = "1.0.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4df4175de05129f31b80458c6df371a15e7fc3fd367272e6bf938e5c351c7ea0" +dependencies = [ + "home", + "windows-sys 0.52.0", +] + [[package]] name = "thiserror" version = "1.0.69" diff --git a/limitador/Cargo.toml b/limitador/Cargo.toml index d9240b5f..a2dc0033 100644 --- a/limitador/Cargo.toml +++ b/limitador/Cargo.toml @@ -54,6 +54,8 @@ tonic = { version = "0.12.3", optional = true } tonic-reflection = { version = "0.12.3", optional = true } prost = { version = "0.13.3", optional = true } prost-types = { version = "0.13.3", optional = true } +cel-parser = "0.8.0" +cel-interpreter = { version = "0.9.0", features = ["chrono", "regex"] } [dev-dependencies] serial_test = "3.0" diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 94b95b1b..0df643d5 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -855,6 +855,12 @@ mod conditions { } } +pub use cel::Expression as CelExpression; +pub use cel::ParseError; +pub use cel::Predicate as CelPredicate; + +pub(super) mod cel; + #[cfg(test)] mod tests { use super::*; diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs new file mode 100644 index 00000000..7b08cdb9 --- /dev/null +++ b/limitador/src/limit/cel.rs @@ -0,0 +1,162 @@ +use crate::limit::cel::errors::EvaluationError; +use cel_interpreter::{ExecutionError, Value}; +pub use errors::ParseError; + +pub(super) mod errors { + use cel_interpreter::ExecutionError; + use std::error::Error; + use std::fmt::{Display, Formatter}; + + #[derive(Debug, PartialEq)] + pub enum EvaluationError { + UnexpectedValueType(String), + ExecutionError(ExecutionError), + } + + impl Display for EvaluationError { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + match self { + EvaluationError::UnexpectedValueType(value) => { + write!(f, "unexpected value of type {}", value) + } + EvaluationError::ExecutionError(error) => error.fmt(f), + } + } + } + + impl Error for EvaluationError {} + + #[derive(Debug)] + pub struct ParseError { + input: String, + source: Box, + } + + impl ParseError { + pub fn from(source: cel_parser::ParseError, input: String) -> Self { + Self { + input, + source: Box::new(source), + } + } + } + + impl Display for ParseError { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "couldn't parse {}: {}", self.input, self.source) + } + } + + impl Error for ParseError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + Some(self.source.as_ref()) + } + } + + impl From for EvaluationError { + fn from(err: ExecutionError) -> Self { + EvaluationError::ExecutionError(err) + } + } +} + +pub struct Context {} + +pub struct Expression { + expression: cel_parser::Expression, +} + +impl Expression { + pub fn parse(source: &str) -> Result { + match cel_parser::parse(source) { + Ok(expression) => Ok(Self { expression }), + Err(err) => Err(ParseError::from(err, source.to_string())), + } + } + + pub fn eval(&self, ctx: &Context) -> Result { + match self.resolve(ctx)? { + Value::Int(i) => Ok(i.to_string()), + Value::UInt(i) => Ok(i.to_string()), + Value::Float(f) => Ok(f.to_string()), + Value::String(s) => Ok(s.to_string()), + Value::Null => Ok("null".to_owned()), + Value::Bool(b) => Ok(b.to_string()), + val => Err(err_on_value(val)), + } + } + + pub fn resolve(&self, _ctx: &Context) -> Result { + let ctx = cel_interpreter::Context::default(); + Value::resolve(&self.expression, &ctx) + } +} + +fn err_on_value(val: Value) -> EvaluationError { + match val { + Value::List(list) => EvaluationError::UnexpectedValueType(format!("list: `{:?}`", *list)), + Value::Map(map) => EvaluationError::UnexpectedValueType(format!("map: `{:?}`", *map.map)), + Value::Function(ident, _) => { + EvaluationError::UnexpectedValueType(format!("function: `{}`", *ident)) + } + Value::Bytes(b) => EvaluationError::UnexpectedValueType(format!("function: `{:?}`", *b)), + Value::Duration(d) => EvaluationError::UnexpectedValueType(format!("duration: `{d}`")), + Value::Timestamp(ts) => EvaluationError::UnexpectedValueType(format!("timestamp: `{ts}`")), + Value::Int(i) => EvaluationError::UnexpectedValueType(format!("integer: `{i}`")), + Value::UInt(u) => EvaluationError::UnexpectedValueType(format!("unsigned integer: `{u}`")), + Value::Float(f) => EvaluationError::UnexpectedValueType(format!("float: `{f}`")), + Value::String(s) => EvaluationError::UnexpectedValueType(format!("string: `{s}`")), + Value::Bool(b) => EvaluationError::UnexpectedValueType(format!("bool: `{b}`")), + Value::Null => EvaluationError::UnexpectedValueType("null".to_owned()), + } +} + +pub struct Predicate(Expression); + +impl Predicate { + pub fn parse(source: &str) -> Result { + Expression::parse(source).map(Self) + } + + pub fn test(&self, ctx: &Context) -> Result { + match self.0.resolve(ctx)? { + Value::Bool(b) => Ok(b), + v => Err(err_on_value(v)), + } + } +} + +#[cfg(test)] +mod tests { + use super::{Context, Expression, Predicate}; + + #[test] + fn expression() { + let exp = Expression::parse("100").expect("failed to parse"); + assert_eq!(exp.eval(&Context {}), Ok(String::from("100"))); + } + + #[test] + fn unexpected_value_type_expression() { + let exp = Expression::parse("['100']").expect("failed to parse"); + assert_eq!( + exp.eval(&Context {}).map_err(|e| format!("{e}")), + Err("unexpected value of type list: `[String(\"100\")]`".to_string()) + ); + } + + #[test] + fn predicate() { + let pred = Predicate::parse("42 == uint('42')").expect("failed to parse"); + assert_eq!(pred.test(&Context {}), Ok(true)); + } + + #[test] + fn unexpected_value_predicate() { + let pred = Predicate::parse("42").expect("failed to parse"); + assert_eq!( + pred.test(&Context {}).map_err(|e| format!("{e}")), + Err("unexpected value of type integer: `42`".to_string()) + ); + } +} From 83c5f8ab448b0ddce8a55df1b8d589c6b543c8f1 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 26 Nov 2024 14:15:36 -0500 Subject: [PATCH 02/20] Impl Error::source for EvaluationError Signed-off-by: Alex Snaps --- limitador/src/limit/cel.rs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index 7b08cdb9..62699f78 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -24,7 +24,14 @@ pub(super) mod errors { } } - impl Error for EvaluationError {} + impl Error for EvaluationError { + fn source(&self) -> Option<&(dyn Error + 'static)> { + match self { + EvaluationError::UnexpectedValueType(_) => None, + EvaluationError::ExecutionError(err) => Some(err), + } + } + } #[derive(Debug)] pub struct ParseError { From 2a53ce834a82f16b465e72cd383d6a5843ab3472 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 05:28:32 -0500 Subject: [PATCH 03/20] Serialization and other traits for CEL types Signed-off-by: Alex Snaps --- limitador/src/limit/cel.rs | 80 +++++++++++++++++++++++++++++++++++--- 1 file changed, 75 insertions(+), 5 deletions(-) diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index 62699f78..aa834adc 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -1,6 +1,8 @@ use crate::limit::cel::errors::EvaluationError; use cel_interpreter::{ExecutionError, Value}; pub use errors::ParseError; +use serde::{Deserialize, Serialize}; +use std::cmp::Ordering; pub(super) mod errors { use cel_interpreter::ExecutionError; @@ -69,15 +71,19 @@ pub(super) mod errors { pub struct Context {} +#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(try_from = "String", into = "String")] pub struct Expression { + source: String, expression: cel_parser::Expression, } impl Expression { - pub fn parse(source: &str) -> Result { - match cel_parser::parse(source) { - Ok(expression) => Ok(Self { expression }), - Err(err) => Err(ParseError::from(err, source.to_string())), + pub fn parse(source: T) -> Result { + let source = source.to_string(); + match cel_parser::parse(&source) { + Ok(expression) => Ok(Self { source, expression }), + Err(err) => Err(ParseError::from(err, source)), } } @@ -118,10 +124,45 @@ fn err_on_value(val: Value) -> EvaluationError { } } +impl TryFrom for Expression { + type Error = ParseError; + + fn try_from(value: String) -> Result { + Self::parse(value) + } +} + +impl From for String { + fn from(value: Expression) -> Self { + value.source + } +} + +impl PartialEq for Expression { + fn eq(&self, other: &Self) -> bool { + self.cmp(other) == Ordering::Equal + } +} + +impl Eq for Expression {} + +impl PartialOrd for Expression { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for Expression { + fn cmp(&self, other: &Self) -> Ordering { + self.source.cmp(&other.source) + } +} + +#[derive(Clone, Debug, Serialize)] pub struct Predicate(Expression); impl Predicate { - pub fn parse(source: &str) -> Result { + pub fn parse(source: T) -> Result { Expression::parse(source).map(Self) } @@ -133,6 +174,26 @@ impl Predicate { } } +impl Eq for Predicate {} + +impl PartialEq for Predicate { + fn eq(&self, other: &Self) -> bool { + self.0.source == other.0.source + } +} + +impl PartialOrd for Predicate { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for Predicate { + fn cmp(&self, other: &Self) -> Ordering { + self.0.cmp(&other.0) + } +} + #[cfg(test)] mod tests { use super::{Context, Expression, Predicate}; @@ -143,6 +204,15 @@ mod tests { assert_eq!(exp.eval(&Context {}), Ok(String::from("100"))); } + #[test] + fn expression_serialization() { + let exp = Expression::parse("100").expect("failed to parse"); + let serialized = serde_json::to_string(&exp).expect("failed to serialize"); + let deserialized: Expression = + serde_json::from_str(&serialized).expect("failed to deserialize"); + assert_eq!(exp.eval(&Context {}), deserialized.eval(&Context {})); + } + #[test] fn unexpected_value_type_expression() { let exp = Expression::parse("['100']").expect("failed to parse"); From d8950988bd87ef5c55a18a02d1baabe49d1011d2 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 06:35:31 -0500 Subject: [PATCH 04/20] Replaced `Condition`s with `cel::Predicate`s Signed-off-by: Alex Snaps --- limitador/src/lib.rs | 6 +- limitador/src/limit.rs | 33 ++++---- limitador/src/limit/cel.rs | 111 ++++++++++++++++++++++---- limitador/tests/integration_tests.rs | 112 +++++++++++++-------------- 4 files changed, 172 insertions(+), 90 deletions(-) diff --git a/limitador/src/lib.rs b/limitador/src/lib.rs index 69628052..e2e634f9 100644 --- a/limitador/src/lib.rs +++ b/limitador/src/lib.rs @@ -103,7 +103,7 @@ //! "my_namespace", //! 2, //! 60, -//! vec!["req.method == 'GET'"], +//! vec!["req_method == 'GET'"], //! vec!["user_id"], //! ).unwrap(); //! rate_limiter.add_limit(limit); @@ -111,7 +111,7 @@ //! // We've defined a limit of 2. So we can report 2 times before being //! // rate-limited //! let mut values_to_report: HashMap = HashMap::new(); -//! values_to_report.insert("req.method".to_string(), "GET".to_string()); +//! values_to_report.insert("req_method".to_string(), "GET".to_string()); //! values_to_report.insert("user_id".to_string(), "1".to_string()); //! //! // Check if we can report @@ -167,7 +167,7 @@ //! "my_namespace", //! 10, //! 60, -//! vec!["req.method == 'GET'"], +//! vec!["req_method == 'GET'"], //! vec!["user_id"], //! ).unwrap(); //! diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 0df643d5..faa10e98 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -64,7 +64,7 @@ pub struct Limit { // Need to sort to generate the same object when using the JSON as a key or // value in Redis. - conditions: BTreeSet, + conditions: BTreeSet, variables: BTreeSet, } @@ -273,6 +273,7 @@ pub enum Predicate { NotEqual, } +#[allow(dead_code)] impl Predicate { fn test(&self, lhs: &str, rhs: &str) -> bool { match self { @@ -305,8 +306,11 @@ impl Limit { LimitadorError: From<>::Error>, { // the above where-clause is needed in order to call unwrap(). - let conditions: Result, _> = - conditions.into_iter().map(|cond| cond.try_into()).collect(); + let conditions: Result, _> = conditions + .into_iter() + .map(|cond| cond.try_into()) + .map(|r| r.map(|c| cel::Predicate::parse::(c.into()).unwrap())) + .collect(); match conditions { Ok(conditions) => Ok(Self { id: None, @@ -332,7 +336,12 @@ impl Limit { where LimitadorError: From<>::Error>, { - match conditions.into_iter().map(|cond| cond.try_into()).collect() { + match conditions + .into_iter() + .map(|cond| cond.try_into()) + .map(|r| r.map(|c| cel::Predicate::parse::(c.into()).unwrap())) + .collect() + { Ok(conditions) => Ok(Self { id: Some(id.into()), namespace: namespace.into(), @@ -400,25 +409,16 @@ impl Limit { } pub fn applies(&self, values: &HashMap) -> bool { + let ctx = Context::new(self, String::default(), values); let all_conditions_apply = self .conditions .iter() - .all(|cond| Self::condition_applies(cond, values)); + .all(|predicate| predicate.test(&ctx).unwrap()); let all_vars_are_set = self.variables.iter().all(|var| values.contains_key(var)); all_conditions_apply && all_vars_are_set } - - fn condition_applies(condition: &Condition, values: &HashMap) -> bool { - let left_operand = condition.var_name.as_str(); - let right_operand = condition.operand.as_str(); - - match values.get(left_operand) { - Some(val) => condition.predicate.test(val, right_operand), - None => false, - } - } } impl Hash for Limit { @@ -855,6 +855,7 @@ mod conditions { } } +use crate::limit::cel::Context; pub use cel::Expression as CelExpression; pub use cel::ParseError; pub use cel::Predicate as CelPredicate; @@ -1086,7 +1087,7 @@ mod tests { limit1.namespace.clone(), limit1.max_value + 10, limit1.seconds, - limit1.conditions.clone(), + vec!["req.method == 'GET'"], limit1.variables.clone(), ) .expect("This must be a valid limit!"); diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index aa834adc..c35b1492 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -1,8 +1,12 @@ use crate::limit::cel::errors::EvaluationError; +use crate::limit::Limit; use cel_interpreter::{ExecutionError, Value}; pub use errors::ParseError; use serde::{Deserialize, Serialize}; use std::cmp::Ordering; +use std::collections::{HashMap, HashSet}; +use std::hash::{Hash, Hasher}; +use std::sync::Arc; pub(super) mod errors { use cel_interpreter::ExecutionError; @@ -69,7 +73,40 @@ pub(super) mod errors { } } -pub struct Context {} +pub struct Context<'a> { + variables: HashSet, + ctx: cel_interpreter::Context<'a>, +} + +impl Context<'_> { + pub(crate) fn new(limit: &Limit, root: String, values: &HashMap) -> Self { + let mut ctx = cel_interpreter::Context::default(); + + if root.is_empty() { + for (binding, value) in values { + ctx.add_variable_from_value(binding, value.clone()) + } + } else { + let map = cel_interpreter::objects::Map::from(values.clone()); + ctx.add_variable_from_value(root, Value::Map(map)); + } + + let limit_data = cel_interpreter::objects::Map::from(HashMap::from([( + "name", + limit + .name + .as_ref() + .map(|n| Value::String(Arc::new(n.to_string()))) + .unwrap_or(Value::Null), + )])); + ctx.add_variable_from_value("limit", Value::Map(limit_data)); + + Self { + variables: values.keys().cloned().collect(), + ctx, + } + } +} #[derive(Clone, Debug, Serialize, Deserialize)] #[serde(try_from = "String", into = "String")] @@ -99,9 +136,8 @@ impl Expression { } } - pub fn resolve(&self, _ctx: &Context) -> Result { - let ctx = cel_interpreter::Context::default(); - Value::resolve(&self.expression, &ctx) + pub fn resolve(&self, ctx: &Context) -> Result { + Value::resolve(&self.expression, &ctx.ctx) } } @@ -158,16 +194,33 @@ impl Ord for Expression { } } -#[derive(Clone, Debug, Serialize)] -pub struct Predicate(Expression); +#[derive(Clone, Debug, Serialize, Deserialize)] +#[serde(try_from = "String", into = "String")] +pub struct Predicate { + #[serde(skip_serializing, default)] + variables: HashSet, + expression: Expression, +} impl Predicate { pub fn parse(source: T) -> Result { - Expression::parse(source).map(Self) + Expression::parse(source).map(|e| Self { + variables: e + .expression + .references() + .variables() + .into_iter() + .map(String::from) + .collect(), + expression: e, + }) } pub fn test(&self, ctx: &Context) -> Result { - match self.0.resolve(ctx)? { + if !self.variables.iter().all(|v| ctx.variables.contains(v)) { + return Ok(false); + } + match self.expression.resolve(ctx)? { Value::Bool(b) => Ok(b), v => Err(err_on_value(v)), } @@ -178,7 +231,7 @@ impl Eq for Predicate {} impl PartialEq for Predicate { fn eq(&self, other: &Self) -> bool { - self.0.source == other.0.source + self.expression.source == other.expression.source } } @@ -190,18 +243,39 @@ impl PartialOrd for Predicate { impl Ord for Predicate { fn cmp(&self, other: &Self) -> Ordering { - self.0.cmp(&other.0) + self.expression.cmp(&other.expression) + } +} + +impl Hash for Predicate { + fn hash(&self, state: &mut H) { + self.expression.source.hash(state); + } +} + +impl TryFrom for Predicate { + type Error = ParseError; + + fn try_from(value: String) -> Result { + Self::parse(value) + } +} + +impl From for String { + fn from(value: Predicate) -> Self { + value.expression.source } } #[cfg(test)] mod tests { use super::{Context, Expression, Predicate}; + use std::collections::HashSet; #[test] fn expression() { let exp = Expression::parse("100").expect("failed to parse"); - assert_eq!(exp.eval(&Context {}), Ok(String::from("100"))); + assert_eq!(exp.eval(&ctx()), Ok(String::from("100"))); } #[test] @@ -210,14 +284,14 @@ mod tests { let serialized = serde_json::to_string(&exp).expect("failed to serialize"); let deserialized: Expression = serde_json::from_str(&serialized).expect("failed to deserialize"); - assert_eq!(exp.eval(&Context {}), deserialized.eval(&Context {})); + assert_eq!(exp.eval(&ctx()), deserialized.eval(&ctx())); } #[test] fn unexpected_value_type_expression() { let exp = Expression::parse("['100']").expect("failed to parse"); assert_eq!( - exp.eval(&Context {}).map_err(|e| format!("{e}")), + exp.eval(&ctx()).map_err(|e| format!("{e}")), Err("unexpected value of type list: `[String(\"100\")]`".to_string()) ); } @@ -225,15 +299,22 @@ mod tests { #[test] fn predicate() { let pred = Predicate::parse("42 == uint('42')").expect("failed to parse"); - assert_eq!(pred.test(&Context {}), Ok(true)); + assert_eq!(pred.test(&ctx()), Ok(true)); } #[test] fn unexpected_value_predicate() { let pred = Predicate::parse("42").expect("failed to parse"); assert_eq!( - pred.test(&Context {}).map_err(|e| format!("{e}")), + pred.test(&ctx()).map_err(|e| format!("{e}")), Err("unexpected value of type integer: `42`".to_string()) ); } + + fn ctx<'a>() -> Context<'a> { + Context { + variables: HashSet::default(), + ctx: cel_interpreter::Context::default(), + } + } } diff --git a/limitador/tests/integration_tests.rs b/limitador/tests/integration_tests.rs index be458d22..491b4240 100644 --- a/limitador/tests/integration_tests.rs +++ b/limitador/tests/integration_tests.rs @@ -220,7 +220,7 @@ mod test { "first_namespace", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"), @@ -228,7 +228,7 @@ mod test { "second_namespace", 20, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"), @@ -254,7 +254,7 @@ mod test { "first_namespace", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -263,7 +263,7 @@ mod test { "second_namespace", 20, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -288,7 +288,7 @@ mod test { "test_namespace", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -309,7 +309,7 @@ mod test { "test_namespace", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], Vec::::new(), ) .expect("This must be a valid limit!"); @@ -332,7 +332,7 @@ mod test { namespace, 10, 60, - vec!["req.method == 'POST'"], + vec!["req_method == 'POST'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -341,7 +341,7 @@ mod test { namespace, 5, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -361,7 +361,7 @@ mod test { "test_namespace", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -379,7 +379,7 @@ mod test { namespace, 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -387,7 +387,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter .update_counters(namespace, &values, 1) @@ -415,7 +415,7 @@ mod test { namespace, 10, 60, - vec!["req.method == 'POST'"], + vec!["req_method == 'POST'"], vec!["app_id"], ) .expect("This must be a valid limit!"), @@ -423,7 +423,7 @@ mod test { namespace, 5, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"), @@ -469,7 +469,7 @@ mod test { namespace, 5, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -477,7 +477,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter .update_counters(namespace, &values, 1) @@ -504,7 +504,7 @@ mod test { namespace, max_hits, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -512,7 +512,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); for i in 0..max_hits { @@ -542,7 +542,7 @@ mod test { namespace, max_hits, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -550,7 +550,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); for i in 0..max_hits { @@ -580,7 +580,7 @@ mod test { namespace, max_hits, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"), @@ -588,7 +588,7 @@ mod test { namespace, max_hits + 1, 60, - vec!["req.method == 'POST'"], + vec!["req_method == 'POST'"], vec!["app_id"], ) .expect("This must be a valid limit!"), @@ -599,11 +599,11 @@ mod test { } let mut get_values: HashMap = HashMap::new(); - get_values.insert("req.method".to_string(), "GET".to_string()); + get_values.insert("req_method".to_string(), "GET".to_string()); get_values.insert("app_id".to_string(), "test_app_id".to_string()); let mut post_values: HashMap = HashMap::new(); - post_values.insert("req.method".to_string(), "POST".to_string()); + post_values.insert("req_method".to_string(), "POST".to_string()); post_values.insert("app_id".to_string(), "test_app_id".to_string()); for i in 0..max_hits { @@ -650,7 +650,7 @@ mod test { namespace, 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -658,7 +658,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); // Report 5 hits twice. The limit is 10, so the first limited call should be @@ -686,7 +686,7 @@ mod test { namespace, max, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -694,7 +694,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); assert!(rate_limiter @@ -710,7 +710,7 @@ mod test { namespace, max_hits, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -718,7 +718,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); for i in 0..max_hits { @@ -748,7 +748,7 @@ mod test { rate_limiter: &mut TestsLimiter, ) { let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); assert!(!rate_limiter .is_rate_limited("test_namespace", &values, 1) @@ -765,7 +765,7 @@ mod test { namespace, 0, // So reporting 1 more would not be allowed 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -774,7 +774,7 @@ mod test { // Notice that does not match because the method is "POST". let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "POST".to_string()); + values.insert("req_method".to_string(), "POST".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); assert!(!rate_limiter @@ -814,7 +814,7 @@ mod test { namespace, max_hits, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -822,7 +822,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); for _ in 0..max_hits { @@ -852,7 +852,7 @@ mod test { namespace, max_hits, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -860,7 +860,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); for hit in 0..max_hits { @@ -903,7 +903,7 @@ mod test { namespace, 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -913,7 +913,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("app_id".to_string(), "test_app_id".to_string()); // Does not match the limit defined - values.insert("req.method".to_string(), "POST".to_string()); + values.insert("req_method".to_string(), "POST".to_string()); assert!( !rate_limiter @@ -962,7 +962,7 @@ mod test { namespace, max_hits, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -970,7 +970,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter .update_counters(namespace, &values, hits_app_1) @@ -1020,7 +1020,7 @@ mod test { "test_namespace", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1042,7 +1042,7 @@ mod test { namespace, 10, limit_time, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1050,7 +1050,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter .update_counters(namespace, &values, 1) @@ -1068,7 +1068,7 @@ mod test { "first_namespace", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1077,7 +1077,7 @@ mod test { "second_namespace", 20, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1109,7 +1109,7 @@ mod test { namespace, max_value, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1117,7 +1117,7 @@ mod test { rate_limiter.add_limit(&limit).await; let mut values = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter .update_counters(namespace, &values, hits_to_report) @@ -1149,7 +1149,7 @@ mod test { namespace, 10, 1, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1158,7 +1158,7 @@ mod test { namespace, 20, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1185,7 +1185,7 @@ mod test { namespace, 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1194,7 +1194,7 @@ mod test { namespace, 20, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1219,7 +1219,7 @@ mod test { namespace, 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1228,7 +1228,7 @@ mod test { namespace, 20, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1237,7 +1237,7 @@ mod test { namespace, 20, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1267,7 +1267,7 @@ mod test { namespace, max_hits, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1277,7 +1277,7 @@ mod test { } let mut values: HashMap = HashMap::new(); - values.insert("req.method".to_string(), "GET".to_string()); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); for i in 0..max_hits { From 8488bae0eb4750b2e8b68add40acf418d8039073 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 06:46:44 -0500 Subject: [PATCH 05/20] req. is not valid and there is no magic Signed-off-by: Alex Snaps --- doc/server/configuration.md | 2 +- limitador-server/examples/envoy.yaml | 2 +- limitador-server/examples/limits.yaml | 2 +- limitador-server/sandbox/README.md | 8 ++++---- limitador-server/sandbox/envoy.yaml | 4 ++-- limitador-server/sandbox/envoy2.yaml | 4 ++-- limitador-server/sandbox/envoy3.yaml | 4 ++-- limitador-server/sandbox/limits.yaml | 12 ++++++------ limitador-server/sandbox/load-test.json | 4 ++-- limitador-server/sandbox/redis-otel/README.md | 2 +- limitador-server/src/envoy_rls/server.rs | 8 ++++---- limitador-server/src/http_api/server.rs | 8 ++++---- limitador/src/lib.rs | 4 ++-- limitador/src/limit.rs | 6 +++--- limitador/src/storage/disk/rocksdb_storage.rs | 2 +- limitador/src/storage/in_memory.rs | 4 ++-- limitador/src/storage/keys.rs | 18 +++++++++--------- limitador/src/storage/redis/counters_cache.rs | 2 +- limitador/src/storage/redis/redis_cached.rs | 6 +++--- 19 files changed, 51 insertions(+), 51 deletions(-) diff --git a/doc/server/configuration.md b/doc/server/configuration.md index b3374a06..3e1bc519 100644 --- a/doc/server/configuration.md +++ b/doc/server/configuration.md @@ -94,7 +94,7 @@ Here is an example of such a limit definition: max_value: 10 seconds: 60 conditions: - - "req.method == 'GET'" + - "req_method == 'GET'" variables: - user_id ``` diff --git a/limitador-server/examples/envoy.yaml b/limitador-server/examples/envoy.yaml index 6cb44a45..208d3c76 100644 --- a/limitador-server/examples/envoy.yaml +++ b/limitador-server/examples/envoy.yaml @@ -29,7 +29,7 @@ static_resources: - stage: 0 actions: - {request_headers: {header_name: "userid", descriptor_key: "user_id"}} - - {request_headers: {header_name: ":method", descriptor_key: "req.method"}} + - { request_headers: { header_name: ":method", descriptor_key: "req_method" } } http_filters: - name: envoy.filters.http.ratelimit typed_config: diff --git a/limitador-server/examples/limits.yaml b/limitador-server/examples/limits.yaml index afcb2b50..be60b900 100644 --- a/limitador-server/examples/limits.yaml +++ b/limitador-server/examples/limits.yaml @@ -10,6 +10,6 @@ max_value: 5 seconds: 60 conditions: - - "req.method == 'POST'" + - "req_method == 'POST'" variables: - user_id diff --git a/limitador-server/sandbox/README.md b/limitador-server/sandbox/README.md index 76614284..ca0cd9b9 100644 --- a/limitador-server/sandbox/README.md +++ b/limitador-server/sandbox/README.md @@ -99,11 +99,11 @@ bin/grpcurl -plaintext -d @ 127.0.0.1:18081 envoy.service.ratelimit.v3.RateLimit { "entries": [ { - "key": "req.method", + "key": "req_method", "value": "POST" }, { - "key": "req.path", + "key": "req_path", "value": "/" } ] @@ -125,11 +125,11 @@ while :; do bin/grpcurl -plaintext -d @ 127.0.0.1:18081 envoy.service.ratelimit. { "entries": [ { - "key": "req.method", + "key": "req_method", "value": "POST" }, { - "key": "req.path", + "key": "req_path", "value": "/" } ] diff --git a/limitador-server/sandbox/envoy.yaml b/limitador-server/sandbox/envoy.yaml index 7e678e10..bf5ee863 100644 --- a/limitador-server/sandbox/envoy.yaml +++ b/limitador-server/sandbox/envoy.yaml @@ -27,10 +27,10 @@ static_resources: - actions: - request_headers: header_name: :method - descriptor_key: req.method + descriptor_key: req_method - request_headers: header_name: :path - descriptor_key: req.path + descriptor_key: req_path http_filters: - name: envoy.filters.http.ratelimit typed_config: diff --git a/limitador-server/sandbox/envoy2.yaml b/limitador-server/sandbox/envoy2.yaml index d1d15958..351ba1d3 100644 --- a/limitador-server/sandbox/envoy2.yaml +++ b/limitador-server/sandbox/envoy2.yaml @@ -27,10 +27,10 @@ static_resources: - actions: - request_headers: header_name: :method - descriptor_key: req.method + descriptor_key: req_method - request_headers: header_name: :path - descriptor_key: req.path + descriptor_key: req_path http_filters: - name: envoy.filters.http.ratelimit typed_config: diff --git a/limitador-server/sandbox/envoy3.yaml b/limitador-server/sandbox/envoy3.yaml index 03180c47..ba557fb0 100644 --- a/limitador-server/sandbox/envoy3.yaml +++ b/limitador-server/sandbox/envoy3.yaml @@ -27,10 +27,10 @@ static_resources: - actions: - request_headers: header_name: :method - descriptor_key: req.method + descriptor_key: req_method - request_headers: header_name: :path - descriptor_key: req.path + descriptor_key: req_path http_filters: - name: envoy.filters.http.ratelimit typed_config: diff --git a/limitador-server/sandbox/limits.yaml b/limitador-server/sandbox/limits.yaml index 68b6df8d..853c0340 100644 --- a/limitador-server/sandbox/limits.yaml +++ b/limitador-server/sandbox/limits.yaml @@ -3,20 +3,20 @@ max_value: 10 seconds: 60 conditions: - - "req.method == 'GET'" - - "req.path != '/json'" + - "req_method == 'GET'" + - "req_path != '/json'" variables: [] - namespace: test_namespace max_value: 5 seconds: 60 conditions: - - "req.method == 'POST'" - - "req.path != '/json'" + - "req_method == 'POST'" + - "req_path != '/json'" variables: [] - namespace: test_namespace max_value: 50000 seconds: 10 conditions: - - "req.method == 'GET'" - - "req.path == '/json'" + - "req_method == 'GET'" + - "req_path == '/json'" variables: [] diff --git a/limitador-server/sandbox/load-test.json b/limitador-server/sandbox/load-test.json index b23422cc..0cd6c81b 100644 --- a/limitador-server/sandbox/load-test.json +++ b/limitador-server/sandbox/load-test.json @@ -5,11 +5,11 @@ { "entries": [ { - "key": "req.method", + "key": "req_method", "value": "GET" }, { - "key": "req.path", + "key": "req_path", "value": "/json" } ] diff --git a/limitador-server/sandbox/redis-otel/README.md b/limitador-server/sandbox/redis-otel/README.md index 8b2f10ab..1759437a 100644 --- a/limitador-server/sandbox/redis-otel/README.md +++ b/limitador-server/sandbox/redis-otel/README.md @@ -24,7 +24,7 @@ bin/grpcurl -plaintext -d @ 127.0.0.1:18081 envoy.service.ratelimit.v3.RateLimit { "entries": [ { - "key": "req.method", + "key": "req_method", "value": "POST" } ] diff --git a/limitador-server/src/envoy_rls/server.rs b/limitador-server/src/envoy_rls/server.rs index 4eef9151..37476c6d 100644 --- a/limitador-server/src/envoy_rls/server.rs +++ b/limitador-server/src/envoy_rls/server.rs @@ -253,7 +253,7 @@ mod tests { namespace, 1, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -275,7 +275,7 @@ mod tests { descriptors: vec![RateLimitDescriptor { entries: vec![ Entry { - key: "req.method".to_string(), + key: "req_method".to_string(), value: "GET".to_string(), }, Entry { @@ -336,7 +336,7 @@ mod tests { domain: "test_namespace".to_string(), descriptors: vec![RateLimitDescriptor { entries: vec![Entry { - key: "req.method".to_string(), + key: "req_method".to_string(), value: "GET".to_string(), }], limit: None, @@ -370,7 +370,7 @@ mod tests { domain: "".to_string(), descriptors: vec![RateLimitDescriptor { entries: vec![Entry { - key: "req.method".to_string(), + key: "req_method".to_string(), value: "GET".to_string(), }], limit: None, diff --git a/limitador-server/src/http_api/server.rs b/limitador-server/src/http_api/server.rs index 25a1cd76..08825b1d 100644 --- a/limitador-server/src/http_api/server.rs +++ b/limitador-server/src/http_api/server.rs @@ -384,7 +384,7 @@ mod tests { // Prepare values to check let mut values = HashMap::new(); - values.insert("req.method".into(), "GET".into()); + values.insert("req_method".into(), "GET".into()); values.insert("app_id".into(), "1".into()); let info = CheckAndReportInfo { namespace: namespace.into(), @@ -435,7 +435,7 @@ mod tests { // Prepare values to check let mut values = HashMap::new(); - values.insert("req.method".into(), "GET".into()); + values.insert("req_method".into(), "GET".into()); values.insert("app_id".into(), "1".into()); let info = CheckAndReportInfo { namespace: namespace.into(), @@ -510,7 +510,7 @@ mod tests { // Prepare values to check let mut values = HashMap::new(); - values.insert("req.method".into(), "GET".into()); + values.insert("req_method".into(), "GET".into()); values.insert("app_id".into(), "1".into()); let info = CheckAndReportInfo { namespace: namespace.into(), @@ -553,7 +553,7 @@ mod tests { namespace, max, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); diff --git a/limitador/src/lib.rs b/limitador/src/lib.rs index e2e634f9..030b9aae 100644 --- a/limitador/src/lib.rs +++ b/limitador/src/lib.rs @@ -54,7 +54,7 @@ //! "my_namespace", //! 10, //! 60, -//! vec!["req.method == 'GET'"], +//! vec!["req_method == 'GET'"], //! vec!["user_id"], //! ); //! ``` @@ -71,7 +71,7 @@ //! "my_namespace", //! 10, //! 60, -//! vec!["req.method == 'GET'"], +//! vec!["req_method == 'GET'"], //! vec!["user_id"], //! ).unwrap(); //! let mut rate_limiter = RateLimiter::new(1000); diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index faa10e98..f19b5041 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -1063,7 +1063,7 @@ mod tests { "test_namespace", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1078,7 +1078,7 @@ mod tests { "test_namespace", 42, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -1087,7 +1087,7 @@ mod tests { limit1.namespace.clone(), limit1.max_value + 10, limit1.seconds, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], limit1.variables.clone(), ) .expect("This must be a valid limit!"); diff --git a/limitador/src/storage/disk/rocksdb_storage.rs b/limitador/src/storage/disk/rocksdb_storage.rs index 12aadce0..b9344213 100644 --- a/limitador/src/storage/disk/rocksdb_storage.rs +++ b/limitador/src/storage/disk/rocksdb_storage.rs @@ -242,7 +242,7 @@ mod tests { #[test] fn opens_db_on_disk() { let namespace = "test_namespace"; - let limit = Limit::new(namespace, 1, 2, vec!["req.method == 'GET'"], vec!["app_id"]) + let limit = Limit::new(namespace, 1, 2, vec!["req_method == 'GET'"], vec!["app_id"]) .expect("This must be a valid limit!"); let counter = Counter::new(limit, HashMap::default()); diff --git a/limitador/src/storage/in_memory.rs b/limitador/src/storage/in_memory.rs index 3148b6df..c71ab3a0 100644 --- a/limitador/src/storage/in_memory.rs +++ b/limitador/src/storage/in_memory.rs @@ -252,13 +252,13 @@ mod tests { fn counters_for_multiple_limit_per_ns() { let storage = InMemoryStorage::default(); let namespace = "test_namespace"; - let limit_1 = Limit::new(namespace, 1, 1, vec!["req.method == 'GET'"], vec!["app_id"]) + let limit_1 = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) .expect("This must be a valid limit!"); let limit_2 = Limit::new( namespace, 1, 10, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 621eb202..300c6dc4 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -119,12 +119,12 @@ mod tests { "example.com", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); assert_eq!( - "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req.method == \\\"GET\\\"\"],\"variables\":[\"app_id\"]}".as_bytes(), + "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req_method == \\\"GET\\\"\"],\"variables\":[\"app_id\"]}".as_bytes(), key_for_counters_of_limit(&limit)) } @@ -135,7 +135,7 @@ mod tests { "example.com", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -148,7 +148,7 @@ mod tests { #[test] fn counter_key_and_counter_are_symmetric() { let namespace = "ns_counter:"; - let limit = Limit::new(namespace, 1, 1, vec!["req.method == 'GET'"], vec!["app_id"]) + let limit = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) .expect("This must be a valid limit!"); let counter = Counter::new(limit.clone(), HashMap::default()); let raw = key_for_counter(&counter); @@ -158,7 +158,7 @@ mod tests { #[test] fn counter_key_does_not_include_transient_state() { let namespace = "ns_counter:"; - let limit = Limit::new(namespace, 1, 1, vec!["req.method == 'GET'"], vec!["app_id"]) + let limit = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) .expect("This must be a valid limit!"); let counter = Counter::new(limit.clone(), HashMap::default()); let mut other = counter.clone(); @@ -362,7 +362,7 @@ pub mod bin { #[test] fn counter_key_and_counter_are_symmetric() { let namespace = "ns_counter:"; - let limit = Limit::new(namespace, 1, 1, vec!["req.method == 'GET'"], vec!["app_id"]) + let limit = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) .expect("This must be a valid limit!"); let mut variables = HashMap::default(); variables.insert("app_id".to_string(), "123".to_string()); @@ -374,7 +374,7 @@ pub mod bin { #[test] fn counter_key_starts_with_namespace_prefix() { let namespace = "ns_counter:"; - let limit = Limit::new(namespace, 1, 1, vec!["req.method == 'GET'"], vec!["app_id"]) + let limit = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) .expect("This must be a valid limit!"); let counter = Counter::new(limit, HashMap::default()); let serialized_counter = key_for_counter(&counter); @@ -387,14 +387,14 @@ pub mod bin { fn counters_with_id() { let namespace = "ns_counter:"; let limit_without_id = - Limit::new(namespace, 1, 1, vec!["req.method == 'GET'"], vec!["app_id"]) + Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) .expect("This must be a valid limit!"); let limit_with_id = Limit::with_id( "id200", namespace, 1, 1, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"); diff --git a/limitador/src/storage/redis/counters_cache.rs b/limitador/src/storage/redis/counters_cache.rs index 52e87136..b1e4b085 100644 --- a/limitador/src/storage/redis/counters_cache.rs +++ b/limitador/src/storage/redis/counters_cache.rs @@ -680,7 +680,7 @@ mod tests { "test_namespace", max_val, 60, - vec!["req.method == 'POST'"], + vec!["req_method == 'POST'"], vec!["app_id"], ) .expect("This must be a valid limit!"), diff --git a/limitador/src/storage/redis/redis_cached.rs b/limitador/src/storage/redis/redis_cached.rs index 8f1b58dc..1ef9afec 100644 --- a/limitador/src/storage/redis/redis_cached.rs +++ b/limitador/src/storage/redis/redis_cached.rs @@ -444,7 +444,7 @@ mod tests { "test_namespace", 10, 60, - vec!["req.method == 'GET'"], + vec!["req_method == 'GET'"], vec!["app_id"], ) .expect("This must be a valid limit!"), @@ -507,7 +507,7 @@ mod tests { "test_namespace", 10, 60, - vec!["req.method == 'POST'"], + vec!["req_method == 'POST'"], vec!["app_id"], ) .expect("This must be a valid limit!"), @@ -567,7 +567,7 @@ mod tests { "test_namespace", 10, 60, - vec!["req.method == 'POST'"], + vec!["req_method == 'POST'"], vec!["app_id"], ) .expect("This must be a valid limit!"), From 16ad79e09e19adb0947144827686b0f2d1f18744 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 06:58:42 -0500 Subject: [PATCH 06/20] =?UTF-8?q?=F0=9F=94=A5=20feature=20lenient=5Fcondit?= =?UTF-8?q?ions?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Alex Snaps --- doc/migrations/conditions.md | 19 -------- limitador-server/Cargo.toml | 2 +- limitador-server/src/main.rs | 7 --- limitador/Cargo.toml | 1 - limitador/README.md | 1 - limitador/src/limit.rs | 90 +----------------------------------- 6 files changed, 2 insertions(+), 118 deletions(-) diff --git a/doc/migrations/conditions.md b/doc/migrations/conditions.md index 29fd1a8b..ffd4bb49 100644 --- a/doc/migrations/conditions.md +++ b/doc/migrations/conditions.md @@ -23,22 +23,3 @@ case `foo` was the identifier of the variable, while `bar` was the value to eval after the operator `==` would be equally important. SO that `foo == bar` would test for a `foo ` variable being equal to ` bar` where the trailing whitespace after the identifier, and the one prefixing the value, would have been evaluated. - -## Server binary users - -The server still allows for the deprecated syntax, but warns about its usage. You can easily migrate your limits file, -using the following command: - -```commandline -limitador-server --validate old_limits.yaml > updated_limits.yaml -``` - -Which should output `Deprecated syntax for conditions corrected!` to `stderr` while `stdout` would be the limits using -the new syntax. It is recommended you manually verify the resulting `LIMITS_FILE`. - - -## Crate users - -A feature `lenient_conditions` has been added, which lets you use the syntax used in previous version of the crate. -The function `limitador::limit::check_deprecated_syntax_usages_and_reset()` lets you verify if the deprecated syntax -has been used as `limit::Limit`s are created with their condition strings using the deprecated syntax. diff --git a/limitador-server/Cargo.toml b/limitador-server/Cargo.toml index 6ed286c9..5608e939 100644 --- a/limitador-server/Cargo.toml +++ b/limitador-server/Cargo.toml @@ -18,7 +18,7 @@ distributed_storage = ["limitador/distributed_storage"] # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html [dependencies] -limitador = { path = "../limitador", features = ['lenient_conditions'] } +limitador = { path = "../limitador" } tokio = { version = "1", features = ["full"] } thiserror = "2" tonic = "0.12.3" diff --git a/limitador-server/src/main.rs b/limitador-server/src/main.rs index ec12e420..3be5c0ee 100644 --- a/limitador-server/src/main.rs +++ b/limitador-server/src/main.rs @@ -194,9 +194,6 @@ impl Limiter { 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://docs.kuadrant.io/limitador/doc/migrations/conditions/") - } Ok(()) } Err(e) => Err(LimitadorServerError::ConfigFile(format!( @@ -597,10 +594,6 @@ fn create_config() -> (Configuration, &'static str) { let parsed_limits: Result, _> = serde_yaml::from_reader(f); match parsed_limits { Ok(limits) => { - if limitador::limit::check_deprecated_syntax_usages_and_reset() { - eprintln!("Deprecated syntax for conditions corrected!\n") - } - let output: Vec = limits.iter().map(|l| l.into()).collect(); match serde_yaml::to_string(&output) { diff --git a/limitador/Cargo.toml b/limitador/Cargo.toml index a2dc0033..fdaaf03a 100644 --- a/limitador/Cargo.toml +++ b/limitador/Cargo.toml @@ -17,7 +17,6 @@ default = ["disk_storage", "redis_storage"] disk_storage = ["rocksdb"] distributed_storage = ["tokio", "tokio-stream", "h2", "base64", "uuid", "tonic", "tonic-reflection", "prost", "prost-types"] redis_storage = ["redis", "r2d2", "tokio"] -lenient_conditions = [] [dependencies] moka = { version = "0.12", features = ["sync"] } diff --git a/limitador/README.md b/limitador/README.md index fd8f6f95..305f9d45 100644 --- a/limitador/README.md +++ b/limitador/README.md @@ -11,5 +11,4 @@ For the complete documentation of the crate's API, please refer to [docs.rs](htt * `redis_storage`: support for using Redis as the data storage backend. * `disk_storage`: support for using RocksDB as a local disk storage backend. -* `lenient_conditions`: support for the deprecated syntax of `Condition`s * `default`: `redis_storage`. diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index f19b5041..b6729634 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -6,29 +6,8 @@ use std::error::Error; use std::fmt::{Debug, Display, Formatter}; use std::hash::{Hash, Hasher}; -#[cfg(feature = "lenient_conditions")] -mod deprecated { - use std::sync::atomic::{AtomicBool, Ordering}; - - static DEPRECATED_SYNTAX: AtomicBool = AtomicBool::new(false); - - pub fn check_deprecated_syntax_usages_and_reset() -> bool { - match DEPRECATED_SYNTAX.compare_exchange(true, false, Ordering::Relaxed, Ordering::Relaxed) - { - Ok(previous) => previous, - Err(previous) => previous, - } - } - - pub fn deprecated_syntax_used() { - DEPRECATED_SYNTAX.fetch_or(true, Ordering::SeqCst); - } -} - use crate::errors::LimitadorError; use crate::LimitadorResult; -#[cfg(feature = "lenient_conditions")] -pub use deprecated::check_deprecated_syntax_usages_and_reset; #[derive(Debug, Hash, Eq, PartialEq, Clone, PartialOrd, Ord, Serialize, Deserialize)] pub struct Namespace(String); @@ -167,44 +146,6 @@ impl TryFrom for Condition { ) } } - #[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) { ( @@ -902,32 +843,6 @@ mod tests { assert!(!limit.applies(&values)) } - #[test] - #[cfg(feature = "lenient_conditions")] - fn limit_does_not_apply_when_cond_is_false_deprecated_style() { - let limit = Limit::new("test_namespace", 10, 60, vec!["x == 5"], vec!["y"]) - .expect("This must be a valid limit!"); - - let mut values: HashMap = HashMap::new(); - values.insert("x".into(), "1".into()); - values.insert("y".into(), "1".into()); - - assert!(!limit.applies(&values)); - assert!(check_deprecated_syntax_usages_and_reset()); - assert!(!check_deprecated_syntax_usages_and_reset()); - - let limit = Limit::new("test_namespace", 10, 60, vec!["x == foobar"], vec!["y"]) - .expect("This must be a valid limit!"); - - let mut values: HashMap = HashMap::new(); - values.insert("x".into(), "foobar".into()); - values.insert("y".into(), "1".into()); - - assert!(limit.applies(&values)); - assert!(check_deprecated_syntax_usages_and_reset()); - assert!(!check_deprecated_syntax_usages_and_reset()); - } - #[test] fn limit_does_not_apply_when_cond_var_is_not_set() { let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]) @@ -1027,11 +942,8 @@ mod tests { } #[test] - #[cfg(not(feature = "lenient_conditions"))] fn invalid_deprecated_condition_parsing() { - let _result = serde_json::from_str::(r#""x == 5""#) - .err() - .expect("Should fail!"); + serde_json::from_str::(r#""x == 5""#).expect_err("Should fail!"); } #[test] From e212b0e6434aad421af8f1e9d0139f2cf8033be6 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 07:40:05 -0500 Subject: [PATCH 07/20] =?UTF-8?q?=F0=9F=94=A5=20module=20crate::limit::con?= =?UTF-8?q?ditions=20et=20al?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Alex Snaps --- limitador-server/src/envoy_rls/server.rs | 18 +- .../src/http_api/request_types.rs | 5 +- limitador/src/errors.rs | 11 +- limitador/src/limit.rs | 680 +----------------- limitador/src/limit/cel.rs | 11 +- limitador/src/storage/keys.rs | 2 +- limitador/tests/integration_tests.rs | 2 +- 7 files changed, 44 insertions(+), 685 deletions(-) diff --git a/limitador-server/src/envoy_rls/server.rs b/limitador-server/src/envoy_rls/server.rs index 37476c6d..3975d08e 100644 --- a/limitador-server/src/envoy_rls/server.rs +++ b/limitador-server/src/envoy_rls/server.rs @@ -253,7 +253,7 @@ mod tests { namespace, 1, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".to_string()], vec!["app_id"], ) .expect("This must be a valid limit!"); @@ -395,10 +395,16 @@ mod tests { let namespace = "test_namespace"; vec![ - Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["z"]) - .expect("This must be a valid limit!"), - Limit::new(namespace, 0, 60, vec!["x == '1'", "y == '2'"], vec!["z"]) + Limit::new(namespace, 10, 60, vec!["x == '1'".to_string()], vec!["z"]) .expect("This must be a valid limit!"), + Limit::new( + namespace, + 0, + 60, + vec!["x == '1'".to_string(), "y == '2'".to_string()], + vec!["z"], + ) + .expect("This must be a valid limit!"), ] .into_iter() .for_each(|limit| { @@ -462,7 +468,7 @@ mod tests { #[tokio::test] async fn test_takes_into_account_the_hits_addend_param() { let namespace = "test_namespace"; - let limit = Limit::new(namespace, 10, 60, vec!["x == '1'"], vec!["y"]) + let limit = Limit::new(namespace, 10, 60, vec!["x == '1'".to_string()], vec!["y"]) .expect("This must be a valid limit!"); let limiter = RateLimiter::new(10_000); @@ -532,7 +538,7 @@ mod tests { // "hits_addend" is optional according to the spec, and should default // to 1, However, with the autogenerated structs it defaults to 0. let namespace = "test_namespace"; - let limit = Limit::new(namespace, 1, 60, vec!["x == '1'"], vec!["y"]) + let limit = Limit::new(namespace, 1, 60, vec!["x == '1'".to_string()], vec!["y"]) .expect("This must be a valid limit!"); let limiter = RateLimiter::new(10_000); diff --git a/limitador-server/src/http_api/request_types.rs b/limitador-server/src/http_api/request_types.rs index 1cae899a..af80e7b5 100644 --- a/limitador-server/src/http_api/request_types.rs +++ b/limitador-server/src/http_api/request_types.rs @@ -1,6 +1,5 @@ use limitador::counter::Counter as LimitadorCounter; -use limitador::errors::LimitadorError; -use limitador::limit::Limit as LimitadorLimit; +use limitador::limit::{Limit as LimitadorLimit, ParseError}; use paperclip::actix::Apiv2Schema; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, HashMap}; @@ -42,7 +41,7 @@ impl From<&LimitadorLimit> for Limit { } impl TryFrom for LimitadorLimit { - type Error = LimitadorError; + type Error = ParseError; fn try_from(limit: Limit) -> Result { let mut limitador_limit = if let Some(id) = limit.id { diff --git a/limitador/src/errors.rs b/limitador/src/errors.rs index d590aafa..82efa829 100644 --- a/limitador/src/errors.rs +++ b/limitador/src/errors.rs @@ -1,4 +1,5 @@ -use crate::limit::ConditionParsingError; +use crate::limit::cel::EvaluationError; +use crate::limit::ParseError; use crate::storage::StorageErr; use std::convert::Infallible; use std::error::Error; @@ -7,7 +8,7 @@ use std::fmt::{Display, Formatter}; #[derive(Debug)] pub enum LimitadorError { StorageError(StorageErr), - InterpreterError(ConditionParsingError), + InterpreterError(EvaluationError), } impl Display for LimitadorError { @@ -38,13 +39,13 @@ impl From for LimitadorError { } } -impl From for LimitadorError { - fn from(err: ConditionParsingError) -> Self { +impl From for LimitadorError { + fn from(err: EvaluationError) -> Self { LimitadorError::InterpreterError(err) } } -impl From for LimitadorError { +impl From for ParseError { fn from(value: Infallible) -> Self { unreachable!("unexpected infallible value: {:?}", value) } diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index b6729634..4ed66495 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -1,14 +1,9 @@ -use crate::limit::conditions::{ErrorType, Literal, SyntaxError, Token, TokenType}; use serde::{Deserialize, Serialize}; use std::cmp::Ordering; use std::collections::{BTreeSet, HashMap, HashSet}; -use std::error::Error; -use std::fmt::{Debug, Display, Formatter}; +use std::fmt::Debug; use std::hash::{Hash, Hasher}; -use crate::errors::LimitadorError; -use crate::LimitadorResult; - #[derive(Debug, Hash, Eq, PartialEq, Clone, PartialOrd, Ord, Serialize, Deserialize)] pub struct Namespace(String); @@ -47,211 +42,22 @@ pub struct Limit { variables: BTreeSet, } -#[derive(Deserialize, Serialize, PartialEq, Eq, Debug, Clone, Hash, PartialOrd, Ord)] -#[serde(try_from = "String", into = "String")] -pub struct Condition { - var_name: String, - predicate: Predicate, - operand: String, -} - -#[derive(Debug)] -pub struct ConditionParsingError { - error: SyntaxError, - pub tokens: Vec, - condition: String, -} - -impl Display for ConditionParsingError { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "{} of condition \"{}\"", self.error, self.condition) - } -} - -impl Error for ConditionParsingError { - fn source(&self) -> Option<&(dyn Error + 'static)> { - Some(&self.error) - } -} - -impl TryFrom<&str> for Condition { - type Error = ConditionParsingError; - - fn try_from(value: &str) -> Result { - value.to_owned().try_into() - } -} - -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}`" - ) - } - } - (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, - }), - }, - Err(err) => Err(ConditionParsingError { - error: err, - tokens: Vec::new(), - condition: value, - }), - } - } -} - -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 - ) - } -} - -#[derive(PartialEq, Eq, PartialOrd, Ord, Debug, Clone, Hash)] -pub enum Predicate { - Equal, - NotEqual, -} - -#[allow(dead_code)] -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(), - } - } -} - impl Limit { - pub fn new, T: TryInto>( + pub fn new, T: TryInto>( namespace: N, max_value: u64, seconds: u64, conditions: impl IntoIterator, variables: impl IntoIterator>, - ) -> LimitadorResult + ) -> Result where >::Error: core::fmt::Debug, - >::Error: core::fmt::Debug, - LimitadorError: From<>::Error>, + >::Error: core::fmt::Debug, + ParseError: From<>::Error>, { // the above where-clause is needed in order to call unwrap(). - let conditions: Result, _> = conditions - .into_iter() - .map(|cond| cond.try_into()) - .map(|r| r.map(|c| cel::Predicate::parse::(c.into()).unwrap())) - .collect(); + let conditions: Result, _> = + conditions.into_iter().map(|cond| cond.try_into()).collect(); match conditions { Ok(conditions) => Ok(Self { id: None, @@ -266,23 +72,18 @@ impl Limit { } } - pub fn with_id, N: Into, T: TryInto>( + pub fn with_id, N: Into, T: TryInto>( id: S, namespace: N, max_value: u64, seconds: u64, conditions: impl IntoIterator, variables: impl IntoIterator>, - ) -> LimitadorResult + ) -> Result where - LimitadorError: From<>::Error>, + ParseError: From<>::Error>, { - match conditions - .into_iter() - .map(|cond| cond.try_into()) - .map(|r| r.map(|c| cel::Predicate::parse::(c.into()).unwrap())) - .collect() - { + match conditions.into_iter().map(|cond| cond.try_into()).collect() { Ok(conditions) => Ok(Self { id: Some(id.into()), namespace: namespace.into(), @@ -401,402 +202,7 @@ impl PartialEq for Limit { } } -mod conditions { - use std::error::Error; - use std::fmt::{Debug, Display, Formatter}; - use std::num::IntErrorKind; - - #[derive(Debug)] - pub struct SyntaxError { - pub pos: usize, - pub error: ErrorType, - } - - #[derive(Debug, Eq, PartialEq)] - pub enum ErrorType { - UnexpectedToken(Token), - MissingToken, - InvalidCharacter(char), - InvalidNumber, - UnclosedStringLiteral(char), - } - - impl Display for SyntaxError { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - match &self.error { - ErrorType::UnexpectedToken(token) => write!( - f, - "SyntaxError: Unexpected token `{}` at offset {}", - token, self.pos - ), - ErrorType::InvalidCharacter(char) => write!( - f, - "SyntaxError: Invalid character `{}` at offset {}", - char, self.pos - ), - ErrorType::InvalidNumber => { - write!(f, "SyntaxError: Invalid number at offset {}", self.pos) - } - ErrorType::MissingToken => { - write!(f, "SyntaxError: Expected token at offset {}", self.pos) - } - ErrorType::UnclosedStringLiteral(char) => { - write!(f, "SyntaxError: Missing closing `{}` for string literal starting at offset {}", char, self.pos) - } - } - } - } - - impl Error for SyntaxError {} - - #[derive(Clone, Eq, PartialEq, Debug)] - pub enum TokenType { - // Predicates - EqualEqual, - NotEqual, - - //Literals - Identifier, - String, - Number, - } - - #[derive(Clone, Eq, PartialEq, Debug)] - pub enum Literal { - Identifier(String), - String(String), - Number(i64), - } - - impl Display for Literal { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - match self { - Literal::Identifier(id) => write!(f, "{id}"), - Literal::String(string) => write!(f, "'{string}'"), - Literal::Number(number) => write!(f, "{number}"), - } - } - } - - #[derive(Clone, Eq, PartialEq, Debug)] - pub struct Token { - pub token_type: TokenType, - pub literal: Option, - pub pos: usize, - } - - impl Display for Token { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - match self.token_type { - TokenType::EqualEqual => write!(f, "Equality (==)"), - TokenType::NotEqual => write!(f, "Unequal (!=)"), - TokenType::Identifier => { - write!(f, "Identifier: {}", self.literal.as_ref().unwrap()) - } - TokenType::String => { - write!(f, "String literal: {}", self.literal.as_ref().unwrap()) - } - TokenType::Number => { - write!(f, "Number literal: {}", self.literal.as_ref().unwrap()) - } - } - } - } - - pub struct Scanner { - input: Vec, - pos: usize, - } - - impl Scanner { - pub fn scan(condition: String) -> Result, SyntaxError> { - let mut tokens: Vec = Vec::with_capacity(3); - let mut scanner = Scanner { - input: condition.chars().collect(), - pos: 0, - }; - while !scanner.done() { - match scanner.next_token() { - Ok(token) => { - if let Some(token) = token { - tokens.push(token) - } - } - Err(err) => { - return Err(err); - } - } - } - Ok(tokens) - } - - fn next_token(&mut self) -> Result, SyntaxError> { - let character = self.advance(); - match character { - '=' => { - if self.next_matches('=') { - Ok(Some(Token { - token_type: TokenType::EqualEqual, - literal: None, - pos: self.pos - 1, - })) - } else { - Err(SyntaxError { - pos: self.pos, - error: ErrorType::InvalidCharacter(self.input[self.pos - 1]), - }) - } - } - '!' => { - if self.next_matches('=') { - Ok(Some(Token { - token_type: TokenType::NotEqual, - literal: None, - pos: self.pos - 1, - })) - } else { - Err(SyntaxError { - pos: self.pos, - error: ErrorType::InvalidCharacter(self.input[self.pos - 1]), - }) - } - } - '"' | '\'' => self.scan_string(character).map(Some), - ' ' | '\n' | '\r' | '\t' => Ok(None), - _ => { - if character.is_alphabetic() { - self.scan_identifier().map(Some) - } else if character.is_numeric() { - self.scan_number().map(Some) - } else { - Err(SyntaxError { - pos: self.pos, - error: ErrorType::InvalidCharacter(character), - }) - } - } - } - } - - fn scan_identifier(&mut self) -> Result { - let start = self.pos; - while !self.done() && self.valid_id_char() { - self.advance(); - } - Ok(Token { - token_type: TokenType::Identifier, - literal: Some(Literal::Identifier( - self.input[start - 1..self.pos].iter().collect(), - )), - pos: start, - }) - } - - fn valid_id_char(&mut self) -> bool { - let char = self.input[self.pos]; - char.is_alphanumeric() || char == '.' || char == '_' - } - - fn scan_string(&mut self, until: char) -> Result { - let start = self.pos; - loop { - if self.done() { - return Err(SyntaxError { - pos: start, - error: ErrorType::UnclosedStringLiteral(until), - }); - } - if self.advance() == until { - return Ok(Token { - token_type: TokenType::String, - literal: Some(Literal::String( - self.input[start..self.pos - 1].iter().collect(), - )), - pos: start, - }); - } - } - } - - fn scan_number(&mut self) -> Result { - let start = self.pos; - while !self.done() && self.input[self.pos].is_numeric() { - self.advance(); - } - let number_str = self.input[start - 1..self.pos].iter().collect::(); - match number_str.parse::() { - Ok(number) => Ok(Token { - token_type: TokenType::Number, - literal: Some(Literal::Number(number)), - pos: start, - }), - Err(err) => { - let syntax_error = match err.kind() { - IntErrorKind::Empty => { - unreachable!("This means a bug in the scanner!") - } - IntErrorKind::Zero => { - unreachable!("We're parsing Numbers as i64, so 0 should always work!") - } - _ => SyntaxError { - pos: start, - error: ErrorType::InvalidNumber, - }, - }; - Err(syntax_error) - } - } - } - - fn advance(&mut self) -> char { - let char = self.input[self.pos]; - self.pos += 1; - char - } - - fn next_matches(&mut self, c: char) -> bool { - if self.done() || self.input[self.pos] != c { - return false; - } - - self.pos += 1; - true - } - - fn done(&self) -> bool { - self.pos >= self.input.len() - } - } - - #[cfg(test)] - mod tests { - use crate::limit::conditions::Literal::Identifier; - use crate::limit::conditions::{ErrorType, Literal, Scanner, Token, TokenType}; - - #[test] - fn test_scanner() { - let mut tokens = - Scanner::scan("foo=='bar '".to_owned()).expect("Should parse alright!"); - assert_eq!(tokens.len(), 3); - assert_eq!( - tokens[0], - Token { - token_type: TokenType::Identifier, - literal: Some(Identifier("foo".to_owned())), - pos: 1, - } - ); - assert_eq!( - tokens[1], - Token { - token_type: TokenType::EqualEqual, - literal: None, - pos: 4, - } - ); - assert_eq!( - tokens[2], - Token { - token_type: TokenType::String, - literal: Some(Literal::String("bar ".to_owned())), - pos: 6, - } - ); - - tokens[1].pos += 1; - tokens[2].pos += 2; - assert_eq!( - tokens, - Scanner::scan("foo == 'bar '".to_owned()).expect("Should parse alright!") - ); - - tokens[0].pos += 2; - tokens[1].pos += 2; - tokens[2].pos += 2; - assert_eq!( - tokens, - Scanner::scan(" foo == 'bar ' ".to_owned()).expect("Should parse alright!") - ); - - tokens[1].pos += 2; - tokens[2].pos += 4; - assert_eq!( - tokens, - Scanner::scan(" foo == 'bar ' ".to_owned()).expect("Should parse alright!") - ); - } - - #[test] - fn test_number_literal() { - let tokens = Scanner::scan("var == 42".to_owned()).expect("Should parse alright!"); - assert_eq!(tokens.len(), 3); - assert_eq!( - tokens[0], - Token { - token_type: TokenType::Identifier, - literal: Some(Identifier("var".to_owned())), - pos: 1, - } - ); - assert_eq!( - tokens[1], - Token { - token_type: TokenType::EqualEqual, - literal: None, - pos: 5, - } - ); - assert_eq!( - tokens[2], - Token { - token_type: TokenType::Number, - literal: Some(Literal::Number(42)), - pos: 8, - } - ); - } - - #[test] - fn test_charset() { - let tokens = - Scanner::scan(" 変数 == ' 💖 '".to_owned()).expect("Should parse alright!"); - assert_eq!(tokens.len(), 3); - assert_eq!( - tokens[0], - Token { - token_type: TokenType::Identifier, - literal: Some(Identifier("変数".to_owned())), - pos: 2, - } - ); - assert_eq!( - tokens[1], - Token { - token_type: TokenType::EqualEqual, - literal: None, - pos: 5, - } - ); - assert_eq!( - tokens[2], - Token { - token_type: TokenType::String, - literal: Some(Literal::String(" 💖 ".to_owned())), - pos: 8, - } - ); - } - - #[test] - fn unclosed_string_literal() { - let error = Scanner::scan("foo == 'ba".to_owned()).expect_err("Should fail!"); - assert_eq!(error.pos, 8); - assert_eq!(error.error, ErrorType::UnclosedStringLiteral('\'')); - } - } -} - -use crate::limit::cel::Context; +use crate::limit::cel::{Context, Predicate}; pub use cel::Expression as CelExpression; pub use cel::ParseError; pub use cel::Predicate as CelPredicate; @@ -906,68 +312,6 @@ mod tests { assert!(!limit.applies(&values)) } - #[test] - fn valid_condition_literal_parsing() { - let result: Condition = serde_json::from_str(r#""x == '5'""#).expect("Should deserialize"); - assert_eq!( - result, - Condition { - var_name: "x".to_string(), - predicate: Predicate::Equal, - operand: "5".to_string(), - } - ); - - let result: Condition = - serde_json::from_str(r#"" foobar=='ok' ""#).expect("Should deserialize"); - assert_eq!( - result, - Condition { - var_name: "foobar".to_string(), - predicate: Predicate::Equal, - operand: "ok".to_string(), - } - ); - - let result: Condition = - serde_json::from_str(r#"" foobar == 'ok' ""#).expect("Should deserialize"); - assert_eq!( - result, - Condition { - var_name: "foobar".to_string(), - predicate: Predicate::Equal, - operand: "ok".to_string(), - } - ); - } - - #[test] - fn invalid_deprecated_condition_parsing() { - serde_json::from_str::(r#""x == 5""#).expect_err("Should fail!"); - } - - #[test] - fn invalid_condition_parsing() { - let result = serde_json::from_str::(r#""x != 5 && x > 12""#) - .expect_err("should fail parsing"); - assert_eq!( - result.to_string(), - "SyntaxError: Invalid character `&` at offset 8 of condition \"x != 5 && x > 12\"" - .to_string() - ); - } - - #[test] - fn condition_serialization() { - let condition = Condition { - var_name: "foobar".to_string(), - predicate: Predicate::Equal, - operand: "ok".to_string(), - }; - let result = serde_json::to_string(&condition).expect("Should serialize"); - assert_eq!(result, r#""foobar == \"ok\"""#.to_string()); - } - #[test] fn limit_id() { let limit = Limit::with_id( diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index c35b1492..66533d22 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -1,4 +1,3 @@ -use crate::limit::cel::errors::EvaluationError; use crate::limit::Limit; use cel_interpreter::{ExecutionError, Value}; pub use errors::ParseError; @@ -73,6 +72,8 @@ pub(super) mod errors { } } +pub use errors::EvaluationError; + pub struct Context<'a> { variables: HashSet, ctx: cel_interpreter::Context<'a>, @@ -261,6 +262,14 @@ impl TryFrom for Predicate { } } +impl TryFrom<&str> for Predicate { + type Error = ParseError; + + fn try_from(value: &str) -> Result { + Self::parse(value) + } +} + impl From for String { fn from(value: Predicate) -> Self { value.expression.source diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 300c6dc4..e426f6b0 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -124,7 +124,7 @@ mod tests { ) .expect("This must be a valid limit!"); assert_eq!( - "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req_method == \\\"GET\\\"\"],\"variables\":[\"app_id\"]}".as_bytes(), + "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req_method == 'GET'\"],\"variables\":[\"app_id\"]}".as_bytes(), key_for_counters_of_limit(&limit)) } diff --git a/limitador/tests/integration_tests.rs b/limitador/tests/integration_tests.rs index 491b4240..6fa8cece 100644 --- a/limitador/tests/integration_tests.rs +++ b/limitador/tests/integration_tests.rs @@ -1267,7 +1267,7 @@ mod test { namespace, max_hits, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".to_string()], vec!["app_id"], ) .expect("This must be a valid limit!"); From d3911b975c80b9ab3a1ce60d63f946eeec5f7494 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 07:57:04 -0500 Subject: [PATCH 08/20] Inject Limit id & name into Context Signed-off-by: Alex Snaps --- limitador/src/limit/cel.rs | 71 ++++++++++++++++++++++++++++++++------ 1 file changed, 61 insertions(+), 10 deletions(-) diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index 66533d22..99a3ba21 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -92,14 +92,24 @@ impl Context<'_> { ctx.add_variable_from_value(root, Value::Map(map)); } - let limit_data = cel_interpreter::objects::Map::from(HashMap::from([( - "name", - limit - .name - .as_ref() - .map(|n| Value::String(Arc::new(n.to_string()))) - .unwrap_or(Value::Null), - )])); + let limit_data = cel_interpreter::objects::Map::from(HashMap::from([ + ( + "name", + limit + .name + .as_ref() + .map(|n| Value::String(Arc::new(n.to_string()))) + .unwrap_or(Value::Null), + ), + ( + "id", + limit + .id + .as_ref() + .map(|n| Value::String(Arc::new(n.to_string()))) + .unwrap_or(Value::Null), + ), + ])); ctx.add_variable_from_value("limit", Value::Map(limit_data)); Self { @@ -218,7 +228,12 @@ impl Predicate { } pub fn test(&self, ctx: &Context) -> Result { - if !self.variables.iter().all(|v| ctx.variables.contains(v)) { + if !self + .variables + .iter() + .filter(|binding| binding.as_str() != "limit") + .all(|v| ctx.variables.contains(v)) + { return Ok(false); } match self.expression.resolve(ctx)? { @@ -279,7 +294,8 @@ impl From for String { #[cfg(test)] mod tests { use super::{Context, Expression, Predicate}; - use std::collections::HashSet; + use crate::limit::Limit; + use std::collections::{HashMap, HashSet}; #[test] fn expression() { @@ -320,6 +336,41 @@ mod tests { ); } + #[test] + fn context_has_limit_info() { + let mut limit = Limit::new( + "ns", + 42, + 10, + vec!["limit.name == 'named_limit'"], + Vec::::default(), + ) + .expect("failed to create"); + assert!(!limit.applies(&HashMap::default())); + limit.set_name("named_limit".to_string()); + assert!(limit.applies(&HashMap::default())); + let limit = Limit::with_id( + "my_id", + "ns", + 42, + 10, + vec!["limit.id == 'my_id'"], + Vec::::default(), + ) + .expect("failed to create"); + assert!(limit.applies(&HashMap::default())); + let limit = Limit::with_id( + "my_id", + "ns", + 42, + 10, + vec!["limit.id == 'other_id'"], + Vec::::default(), + ) + .expect("failed to create"); + assert!(!limit.applies(&HashMap::default())); + } + fn ctx<'a>() -> Context<'a> { Context { variables: HashSet::default(), From a1e9304b17419bb98c4cf12ebbb6422ffa5efce8 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 09:52:14 -0500 Subject: [PATCH 09/20] Slightly rearranged/cleaned deps Signed-off-by: Alex Snaps --- limitador/src/errors.rs | 2 +- limitador/src/limit.rs | 53 ++++++++++++++++++++++++++++++++------ limitador/src/limit/cel.rs | 43 +++---------------------------- 3 files changed, 49 insertions(+), 49 deletions(-) diff --git a/limitador/src/errors.rs b/limitador/src/errors.rs index 82efa829..61070ed7 100644 --- a/limitador/src/errors.rs +++ b/limitador/src/errors.rs @@ -1,4 +1,4 @@ -use crate::limit::cel::EvaluationError; +use crate::limit::EvaluationError; use crate::limit::ParseError; use crate::storage::StorageErr; use std::convert::Infallible; diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 4ed66495..d336b5eb 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -1,9 +1,15 @@ +use cel::Context; use serde::{Deserialize, Serialize}; use std::cmp::Ordering; use std::collections::{BTreeSet, HashMap, HashSet}; use std::fmt::Debug; use std::hash::{Hash, Hasher}; +mod cel; + +pub use cel::{EvaluationError, ParseError}; +pub use cel::{Expression, Predicate}; + #[derive(Debug, Hash, Eq, PartialEq, Clone, PartialOrd, Ord, Serialize, Deserialize)] pub struct Namespace(String); @@ -38,7 +44,7 @@ pub struct Limit { // Need to sort to generate the same object when using the JSON as a key or // value in Redis. - conditions: BTreeSet, + conditions: BTreeSet, variables: BTreeSet, } @@ -202,13 +208,6 @@ impl PartialEq for Limit { } } -use crate::limit::cel::{Context, Predicate}; -pub use cel::Expression as CelExpression; -pub use cel::ParseError; -pub use cel::Predicate as CelPredicate; - -pub(super) mod cel; - #[cfg(test)] mod tests { use super::*; @@ -352,4 +351,42 @@ mod tests { assert_eq!(limit1.partial_cmp(&limit2), Some(Equal)); assert_eq!(limit1, limit2); } + + #[test] + fn conditions_have_limit_info() { + let mut limit = Limit::new( + "ns", + 42, + 10, + vec!["limit.name == 'named_limit'"], + Vec::::default(), + ) + .expect("failed to create"); + assert!(!limit.applies(&HashMap::default())); + + limit.set_name("named_limit".to_string()); + assert!(limit.applies(&HashMap::default())); + + let limit = Limit::with_id( + "my_id", + "ns", + 42, + 10, + vec!["limit.id == 'my_id'", "limit.name == null"], + Vec::::default(), + ) + .expect("failed to create"); + assert!(limit.applies(&HashMap::default())); + + let limit = Limit::with_id( + "my_id", + "ns", + 42, + 10, + vec!["limit.id == 'other_id'"], + Vec::::default(), + ) + .expect("failed to create"); + assert!(!limit.applies(&HashMap::default())); + } } diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index 99a3ba21..d910c0c9 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -1,12 +1,13 @@ use crate::limit::Limit; use cel_interpreter::{ExecutionError, Value}; -pub use errors::ParseError; use serde::{Deserialize, Serialize}; use std::cmp::Ordering; use std::collections::{HashMap, HashSet}; use std::hash::{Hash, Hasher}; use std::sync::Arc; +pub use errors::{EvaluationError, ParseError}; + pub(super) mod errors { use cel_interpreter::ExecutionError; use std::error::Error; @@ -72,8 +73,6 @@ pub(super) mod errors { } } -pub use errors::EvaluationError; - pub struct Context<'a> { variables: HashSet, ctx: cel_interpreter::Context<'a>, @@ -294,8 +293,7 @@ impl From for String { #[cfg(test)] mod tests { use super::{Context, Expression, Predicate}; - use crate::limit::Limit; - use std::collections::{HashMap, HashSet}; + use std::collections::HashSet; #[test] fn expression() { @@ -336,41 +334,6 @@ mod tests { ); } - #[test] - fn context_has_limit_info() { - let mut limit = Limit::new( - "ns", - 42, - 10, - vec!["limit.name == 'named_limit'"], - Vec::::default(), - ) - .expect("failed to create"); - assert!(!limit.applies(&HashMap::default())); - limit.set_name("named_limit".to_string()); - assert!(limit.applies(&HashMap::default())); - let limit = Limit::with_id( - "my_id", - "ns", - 42, - 10, - vec!["limit.id == 'my_id'"], - Vec::::default(), - ) - .expect("failed to create"); - assert!(limit.applies(&HashMap::default())); - let limit = Limit::with_id( - "my_id", - "ns", - 42, - 10, - vec!["limit.id == 'other_id'"], - Vec::::default(), - ) - .expect("failed to create"); - assert!(!limit.applies(&HashMap::default())); - } - fn ctx<'a>() -> Context<'a> { Context { variables: HashSet::default(), From d60e5f7d45422a0e218111e427590339b8bcc1ab Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 13:24:06 -0500 Subject: [PATCH 10/20] Parse Limit's Predicates on creation Signed-off-by: Alex Snaps --- limitador-server/src/envoy_rls/server.rs | 40 +++- .../src/http_api/request_types.rs | 13 +- limitador-server/src/http_api/server.rs | 5 +- limitador/benches/bench.rs | 23 +- limitador/src/lib.rs | 23 +- limitador/src/limit.rs | 158 +++++++------ limitador/src/storage/disk/rocksdb_storage.rs | 9 +- limitador/src/storage/in_memory.rs | 14 +- limitador/src/storage/keys.rs | 104 +++++--- limitador/src/storage/redis/counters_cache.rs | 5 +- limitador/src/storage/redis/redis_cached.rs | 15 +- limitador/tests/integration_tests.rs | 222 ++++++++---------- 12 files changed, 330 insertions(+), 301 deletions(-) diff --git a/limitador-server/src/envoy_rls/server.rs b/limitador-server/src/envoy_rls/server.rs index 3975d08e..b6c3e7a0 100644 --- a/limitador-server/src/envoy_rls/server.rs +++ b/limitador-server/src/envoy_rls/server.rs @@ -253,10 +253,9 @@ mod tests { namespace, 1, 60, - vec!["req_method == 'GET'".to_string()], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let limiter = RateLimiter::new(10_000); limiter.add_limit(limit); @@ -395,16 +394,23 @@ mod tests { let namespace = "test_namespace"; vec![ - Limit::new(namespace, 10, 60, vec!["x == '1'".to_string()], vec!["z"]) - .expect("This must be a valid limit!"), + Limit::new( + namespace, + 10, + 60, + vec!["x == '1'".try_into().expect("failed parsing!")], + vec!["z"], + ), Limit::new( namespace, 0, 60, - vec!["x == '1'".to_string(), "y == '2'".to_string()], + vec![ + "x == '1'".try_into().expect("failed parsing!"), + "y == '2'".try_into().expect("failed parsing!"), + ], vec!["z"], - ) - .expect("This must be a valid limit!"), + ), ] .into_iter() .for_each(|limit| { @@ -468,8 +474,13 @@ mod tests { #[tokio::test] async fn test_takes_into_account_the_hits_addend_param() { let namespace = "test_namespace"; - let limit = Limit::new(namespace, 10, 60, vec!["x == '1'".to_string()], vec!["y"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + namespace, + 10, + 60, + vec!["x == '1'".try_into().expect("failed parsing!")], + vec!["y"], + ); let limiter = RateLimiter::new(10_000); limiter.add_limit(limit); @@ -538,8 +549,13 @@ mod tests { // "hits_addend" is optional according to the spec, and should default // to 1, However, with the autogenerated structs it defaults to 0. let namespace = "test_namespace"; - let limit = Limit::new(namespace, 1, 60, vec!["x == '1'".to_string()], vec!["y"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + namespace, + 1, + 60, + vec!["x == '1'".try_into().expect("failed parsing!")], + vec!["y"], + ); let limiter = RateLimiter::new(10_000); limiter.add_limit(limit); diff --git a/limitador-server/src/http_api/request_types.rs b/limitador-server/src/http_api/request_types.rs index af80e7b5..99adc8b6 100644 --- a/limitador-server/src/http_api/request_types.rs +++ b/limitador-server/src/http_api/request_types.rs @@ -1,5 +1,5 @@ use limitador::counter::Counter as LimitadorCounter; -use limitador::limit::{Limit as LimitadorLimit, ParseError}; +use limitador::limit::{Limit as LimitadorLimit, ParseError, Predicate}; use paperclip::actix::Apiv2Schema; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, HashMap}; @@ -44,23 +44,26 @@ impl TryFrom for LimitadorLimit { type Error = ParseError; fn try_from(limit: Limit) -> Result { + let conditions: Result, ParseError> = + limit.conditions.into_iter().map(|p| p.try_into()).collect(); + let mut limitador_limit = if let Some(id) = limit.id { Self::with_id( id, limit.namespace, limit.max_value, limit.seconds, - limit.conditions, + conditions?, limit.variables, - )? + ) } else { Self::new( limit.namespace, limit.max_value, limit.seconds, - limit.conditions, + conditions?, limit.variables, - )? + ) }; if let Some(name) = limit.name { diff --git a/limitador-server/src/http_api/server.rs b/limitador-server/src/http_api/server.rs index 08825b1d..d2c26b03 100644 --- a/limitador-server/src/http_api/server.rs +++ b/limitador-server/src/http_api/server.rs @@ -553,10 +553,9 @@ mod tests { namespace, max, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); match &limiter { Limiter::Blocking(limiter) => limiter.add_limit(limit.clone()), diff --git a/limitador/benches/bench.rs b/limitador/benches/bench.rs index 5dcb3677..edccd9e2 100644 --- a/limitador/benches/bench.rs +++ b/limitador/benches/bench.rs @@ -529,7 +529,11 @@ fn generate_test_limits(scenario: &TestScenario) -> (Vec, Vec (Vec, Vec( - namespace, - 42, - 100, - Vec::::default(), - Vec::::default(), - ) - .expect("This must be a valid limit!"); + let l = Limit::new(namespace, 42, 100, vec![], Vec::::default()); rl.add_limit(l.clone()); let limits = rl.get_limits(&namespace.into()); assert_eq!(limits.len(), 1); diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index d336b5eb..afc3f054 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -49,57 +49,43 @@ pub struct Limit { } impl Limit { - pub fn new, T: TryInto>( + pub fn new>( namespace: N, max_value: u64, seconds: u64, - conditions: impl IntoIterator, + conditions: impl IntoIterator, variables: impl IntoIterator>, - ) -> Result + ) -> Self where - >::Error: core::fmt::Debug, - >::Error: core::fmt::Debug, - ParseError: From<>::Error>, + >::Error: Debug, { - // the above where-clause is needed in order to call unwrap(). - let conditions: Result, _> = - conditions.into_iter().map(|cond| cond.try_into()).collect(); - match conditions { - Ok(conditions) => Ok(Self { - id: None, - namespace: namespace.into(), - max_value, - seconds, - name: None, - conditions, - variables: variables.into_iter().map(|var| var.into()).collect(), - }), - Err(err) => Err(err.into()), + Self { + id: None, + namespace: namespace.into(), + max_value, + seconds, + name: None, + conditions: conditions.into_iter().collect(), + variables: variables.into_iter().map(|var| var.into()).collect(), } } - pub fn with_id, N: Into, T: TryInto>( + pub fn with_id, N: Into>( id: S, namespace: N, max_value: u64, seconds: u64, - conditions: impl IntoIterator, + conditions: impl IntoIterator, variables: impl IntoIterator>, - ) -> Result - where - ParseError: From<>::Error>, - { - match conditions.into_iter().map(|cond| cond.try_into()).collect() { - Ok(conditions) => Ok(Self { - id: Some(id.into()), - namespace: namespace.into(), - max_value, - seconds, - name: None, - conditions, - variables: variables.into_iter().map(|var| var.into()).collect(), - }), - Err(err) => Err(err.into()), + ) -> Self { + Self { + id: Some(id.into()), + namespace: namespace.into(), + max_value, + seconds, + name: None, + conditions: conditions.into_iter().collect(), + variables: variables.into_iter().map(|var| var.into()).collect(), } } @@ -215,8 +201,13 @@ mod tests { #[test] fn limit_can_have_an_optional_name() { - let mut limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]) - .expect("This must be a valid limit!"); + let mut limit = Limit::new( + "test_namespace", + 10, + 60, + vec!["x == \"5\"".try_into().expect("failed parsing!")], + vec!["y"], + ); assert!(limit.name.is_none()); let name = "Test Limit"; @@ -226,8 +217,13 @@ mod tests { #[test] fn limit_applies() { - let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + "test_namespace", + 10, + 60, + vec!["x == \"5\"".try_into().expect("failed parsing!")], + vec!["y"], + ); let mut values: HashMap = HashMap::new(); values.insert("x".into(), "5".into()); @@ -238,8 +234,13 @@ mod tests { #[test] fn limit_does_not_apply_when_cond_is_false() { - let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + "test_namespace", + 10, + 60, + vec!["x == \"5\"".try_into().expect("failed parsing!")], + vec!["y"], + ); let mut values: HashMap = HashMap::new(); values.insert("x".into(), "1".into()); @@ -250,8 +251,13 @@ mod tests { #[test] fn limit_does_not_apply_when_cond_var_is_not_set() { - let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + "test_namespace", + 10, + 60, + vec!["x == \"5\"".try_into().expect("failed parsing!")], + vec!["y"], + ); // Notice that "x" is not set let mut values: HashMap = HashMap::new(); @@ -263,8 +269,13 @@ mod tests { #[test] fn limit_does_not_apply_when_var_not_set() { - let limit = Limit::new("test_namespace", 10, 60, vec!["x == \"5\""], vec!["y"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + "test_namespace", + 10, + 60, + vec!["x == \"5\"".try_into().expect("failed parsing!")], + vec!["y"], + ); // Notice that "y" is not set let mut values: HashMap = HashMap::new(); @@ -279,10 +290,12 @@ mod tests { "test_namespace", 10, 60, - vec!["x == \"5\"", "y == \"2\""], + vec![ + "x == \"5\"".try_into().expect("failed parsing!"), + "y == \"2\"".try_into().expect("failed parsing!"), + ], vec!["z"], - ) - .expect("This must be a valid limit!"); + ); let mut values: HashMap = HashMap::new(); values.insert("x".into(), "5".into()); @@ -298,10 +311,12 @@ mod tests { "test_namespace", 10, 60, - vec!["x == \"5\"", "y == \"2\""], + vec![ + "x == \"5\"".try_into().expect("failed parsing!"), + "y == \"2\"".try_into().expect("failed parsing!"), + ], vec!["z"], - ) - .expect("This must be a valid limit!"); + ); let mut values: HashMap = HashMap::new(); values.insert("x".into(), "3".into()); @@ -318,10 +333,9 @@ mod tests { "test_namespace", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); assert_eq!(limit.id(), Some("test_id")) } @@ -333,19 +347,17 @@ mod tests { "test_namespace", 42, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let mut limit2 = Limit::new( limit1.namespace.clone(), limit1.max_value + 10, limit1.seconds, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], limit1.variables.clone(), - ) - .expect("This must be a valid limit!"); + ); limit2.set_name("Who cares?".to_string()); assert_eq!(limit1.partial_cmp(&limit2), Some(Equal)); @@ -358,10 +370,11 @@ mod tests { "ns", 42, 10, - vec!["limit.name == 'named_limit'"], + vec!["limit.name == 'named_limit'" + .try_into() + .expect("failed parsing!")], Vec::::default(), - ) - .expect("failed to create"); + ); assert!(!limit.applies(&HashMap::default())); limit.set_name("named_limit".to_string()); @@ -372,10 +385,12 @@ mod tests { "ns", 42, 10, - vec!["limit.id == 'my_id'", "limit.name == null"], + vec![ + "limit.id == 'my_id'".try_into().expect("failed parsing!"), + "limit.name == null".try_into().expect("failed parsing!"), + ], Vec::::default(), - ) - .expect("failed to create"); + ); assert!(limit.applies(&HashMap::default())); let limit = Limit::with_id( @@ -383,10 +398,11 @@ mod tests { "ns", 42, 10, - vec!["limit.id == 'other_id'"], + vec!["limit.id == 'other_id'" + .try_into() + .expect("failed parsing!")], Vec::::default(), - ) - .expect("failed to create"); + ); assert!(!limit.applies(&HashMap::default())); } } diff --git a/limitador/src/storage/disk/rocksdb_storage.rs b/limitador/src/storage/disk/rocksdb_storage.rs index b9344213..68ef7369 100644 --- a/limitador/src/storage/disk/rocksdb_storage.rs +++ b/limitador/src/storage/disk/rocksdb_storage.rs @@ -242,8 +242,13 @@ mod tests { #[test] fn opens_db_on_disk() { let namespace = "test_namespace"; - let limit = Limit::new(namespace, 1, 2, vec!["req_method == 'GET'"], vec!["app_id"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + namespace, + 1, + 2, + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], + vec!["app_id"], + ); let counter = Counter::new(limit, HashMap::default()); let tmp = TempDir::new().expect("We should have a dir!"); diff --git a/limitador/src/storage/in_memory.rs b/limitador/src/storage/in_memory.rs index c71ab3a0..dca294b7 100644 --- a/limitador/src/storage/in_memory.rs +++ b/limitador/src/storage/in_memory.rs @@ -252,16 +252,20 @@ mod tests { fn counters_for_multiple_limit_per_ns() { let storage = InMemoryStorage::default(); let namespace = "test_namespace"; - let limit_1 = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) - .expect("This must be a valid limit!"); + let limit_1 = Limit::new( + namespace, + 1, + 1, + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], + vec!["app_id"], + ); let limit_2 = Limit::new( namespace, 1, 10, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let counter_1 = Counter::new(limit_1, HashMap::default()); let counter_2 = Counter::new(limit_2, HashMap::default()); storage.update_counter(&counter_1, 1).unwrap(); diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index e426f6b0..9bbeddfa 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -119,10 +119,9 @@ mod tests { "example.com", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); assert_eq!( "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req_method == 'GET'\"],\"variables\":[\"app_id\"]}".as_bytes(), key_for_counters_of_limit(&limit)) @@ -135,10 +134,9 @@ mod tests { "example.com", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); assert_eq!( "\u{2}\u{7}test_id".as_bytes(), key_for_counters_of_limit(&limit) @@ -148,8 +146,13 @@ mod tests { #[test] fn counter_key_and_counter_are_symmetric() { let namespace = "ns_counter:"; - let limit = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + namespace, + 1, + 1, + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], + vec!["app_id"], + ); let counter = Counter::new(limit.clone(), HashMap::default()); let raw = key_for_counter(&counter); assert_eq!(counter, partial_counter_from_counter_key(&raw)); @@ -158,8 +161,13 @@ mod tests { #[test] fn counter_key_does_not_include_transient_state() { let namespace = "ns_counter:"; - let limit = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + namespace, + 1, + 1, + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], + vec!["app_id"], + ); let counter = Counter::new(limit.clone(), HashMap::default()); let mut other = counter.clone(); other.set_remaining(123); @@ -174,7 +182,7 @@ pub mod bin { use std::collections::HashMap; use crate::counter::Counter; - use crate::limit::Limit; + use crate::limit::{Limit, Predicate}; #[derive(PartialEq, Debug, Serialize, Deserialize)] struct IdCounterKey<'a> { @@ -272,8 +280,16 @@ pub mod bin { .into_iter() .map(|(var, value)| (var.to_string(), value.to_string())) .collect(); - let limit = - Limit::new(ns, u64::default(), seconds, conditions, map.keys()).unwrap(); + let limit = Limit::new( + ns, + u64::default(), + seconds, + conditions + .into_iter() + .map(|p| p.try_into().expect("condition corrupted!")) + .collect::>(), + map.keys(), + ); Counter::new(limit, map) } 2u8 => { @@ -284,15 +300,8 @@ pub mod bin { .collect(); // we are not able to rebuild the full limit since we only have the id and variables. - let limit = Limit::with_id::<&str, &str, &str>( - id, - "", - u64::default(), - 0, - vec![], - map.keys(), - ) - .unwrap(); + let limit = + Limit::with_id::<&str, &str>(id, "", u64::default(), 0, vec![], map.keys()); Counter::new(limit, map) } _ => panic!("Unknown version: {}", version), @@ -321,8 +330,17 @@ pub mod bin { .into_iter() .map(|(var, value)| (var.to_string(), value.to_string())) .collect(); - let limit = Limit::new(ns, u64::default(), seconds, conditions, map.keys()); - Counter::new(limit.unwrap(), map) + let limit = Limit::new( + ns, + u64::default(), + seconds, + conditions + .into_iter() + .map(|p| p.try_into().expect("condition corrupted!")) + .collect::>(), + map.keys(), + ); + Counter::new(limit, map) } #[cfg(test)] @@ -342,10 +360,9 @@ pub mod bin { namespace, 1, 2, - vec!["foo == 'bar'"], + vec!["foo == 'bar'".try_into().expect("failed parsing!")], vec!["app_id", "role", "wat"], - ) - .expect("This must be a valid limit!"); + ); let mut vars = HashMap::default(); vars.insert("role".to_string(), "admin".to_string()); vars.insert("app_id".to_string(), "123".to_string()); @@ -362,8 +379,13 @@ pub mod bin { #[test] fn counter_key_and_counter_are_symmetric() { let namespace = "ns_counter:"; - let limit = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + namespace, + 1, + 1, + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], + vec!["app_id"], + ); let mut variables = HashMap::default(); variables.insert("app_id".to_string(), "123".to_string()); let counter = Counter::new(limit.clone(), variables); @@ -374,8 +396,13 @@ pub mod bin { #[test] fn counter_key_starts_with_namespace_prefix() { let namespace = "ns_counter:"; - let limit = Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) - .expect("This must be a valid limit!"); + let limit = Limit::new( + namespace, + 1, + 1, + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], + vec!["app_id"], + ); let counter = Counter::new(limit, HashMap::default()); let serialized_counter = key_for_counter(&counter); @@ -386,18 +413,21 @@ pub mod bin { #[test] fn counters_with_id() { let namespace = "ns_counter:"; - let limit_without_id = - Limit::new(namespace, 1, 1, vec!["req_method == 'GET'"], vec!["app_id"]) - .expect("This must be a valid limit!"); + let limit_without_id = Limit::new( + namespace, + 1, + 1, + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], + vec!["app_id"], + ); let limit_with_id = Limit::with_id( "id200", namespace, 1, 1, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let counter_with_id = Counter::new(limit_with_id, HashMap::default()); let serialized_with_id_counter = key_for_counter(&counter_with_id); diff --git a/limitador/src/storage/redis/counters_cache.rs b/limitador/src/storage/redis/counters_cache.rs index b1e4b085..1924d63f 100644 --- a/limitador/src/storage/redis/counters_cache.rs +++ b/limitador/src/storage/redis/counters_cache.rs @@ -680,10 +680,9 @@ mod tests { "test_namespace", max_val, 60, - vec!["req_method == 'POST'"], + vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), values, ) } diff --git a/limitador/src/storage/redis/redis_cached.rs b/limitador/src/storage/redis/redis_cached.rs index 1ef9afec..742d2986 100644 --- a/limitador/src/storage/redis/redis_cached.rs +++ b/limitador/src/storage/redis/redis_cached.rs @@ -444,10 +444,9 @@ mod tests { "test_namespace", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), Default::default(), ); @@ -507,10 +506,9 @@ mod tests { "test_namespace", 10, 60, - vec!["req_method == 'POST'"], + vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), Default::default(), ); @@ -567,10 +565,9 @@ mod tests { "test_namespace", 10, 60, - vec!["req_method == 'POST'"], + vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), Default::default(), ); diff --git a/limitador/tests/integration_tests.rs b/limitador/tests/integration_tests.rs index 6fa8cece..7572d766 100644 --- a/limitador/tests/integration_tests.rs +++ b/limitador/tests/integration_tests.rs @@ -220,18 +220,16 @@ mod test { "first_namespace", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), Limit::new( "second_namespace", 20, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), ]; for limit in limits { @@ -254,19 +252,17 @@ mod test { "first_namespace", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let lim2 = Limit::new( "second_namespace", 20, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); for limit in [&lim1, &lim2] { rate_limiter.add_limit(limit).await; @@ -288,10 +284,9 @@ mod test { "test_namespace", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -309,10 +304,9 @@ mod test { "test_namespace", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], Vec::::new(), - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -332,19 +326,17 @@ mod test { namespace, 10, 60, - vec!["req_method == 'POST'"], + vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let limit_2 = Limit::new( namespace, 5, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit_1).await; rate_limiter.add_limit(&limit_2).await; @@ -361,10 +353,9 @@ mod test { "test_namespace", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -379,10 +370,9 @@ mod test { namespace, 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -415,18 +405,16 @@ mod test { namespace, 10, 60, - vec!["req_method == 'POST'"], + vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), Limit::new( namespace, 5, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), ]; for limit in limits.iter() { @@ -445,16 +433,22 @@ mod test { let namespace2 = "test_namespace_2"; rate_limiter - .add_limit( - &Limit::new(namespace1, 10, 60, vec!["x == '10'"], vec!["z"]) - .expect("This must be a valid limit!"), - ) + .add_limit(&Limit::new( + namespace1, + 10, + 60, + vec!["x == '10'".try_into().expect("failed parsing!")], + vec!["z"], + )) .await; rate_limiter - .add_limit( - &Limit::new(namespace2, 5, 60, vec!["x == '10'"], vec!["z"]) - .expect("This must be a valid limit!"), - ) + .add_limit(&Limit::new( + namespace2, + 5, + 60, + vec!["x == '10'".try_into().expect("failed parsing!")], + vec!["z"], + )) .await; rate_limiter.delete_limits(namespace1).await.unwrap(); @@ -469,10 +463,9 @@ mod test { namespace, 5, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -504,10 +497,9 @@ mod test { namespace, max_hits, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -542,10 +534,9 @@ mod test { namespace, max_hits, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -580,18 +571,16 @@ mod test { namespace, max_hits, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), Limit::new( namespace, max_hits + 1, 60, - vec!["req_method == 'POST'"], + vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"), + ), ]; for limit in limits { @@ -650,10 +639,9 @@ mod test { namespace, 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -686,10 +674,9 @@ mod test { namespace, max, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -710,10 +697,9 @@ mod test { namespace, max_hits, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -765,10 +751,9 @@ mod test { namespace, 0, // So reporting 1 more would not be allowed 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -790,10 +775,9 @@ mod test { namespace, 0, // So reporting 1 more would not be allowed 60, - Vec::::new(), // unconditional + Vec::default(), // unconditional vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -814,10 +798,9 @@ mod test { namespace, max_hits, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -852,10 +835,9 @@ mod test { namespace, max_hits, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -903,10 +885,9 @@ mod test { namespace, 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -933,10 +914,9 @@ mod test { namespace, 0, // So reporting 1 more would not be allowed 60, - Vec::::new(), // unconditional + Vec::default(), // unconditional vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -962,10 +942,9 @@ mod test { namespace, max_hits, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -1020,10 +999,9 @@ mod test { "test_namespace", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -1042,10 +1020,9 @@ mod test { namespace, 10, limit_time, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -1068,19 +1045,17 @@ mod test { "first_namespace", 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let second_limit = Limit::new( "second_namespace", 20, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter .configure_with(vec![first_limit.clone(), second_limit.clone()]) @@ -1109,10 +1084,9 @@ mod test { namespace, max_value, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit).await; @@ -1149,19 +1123,17 @@ mod test { namespace, 10, 1, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let limit_to_be_deleted = Limit::new( namespace, 20, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); for limit in [&limit_to_be_kept, &limit_to_be_deleted].iter() { rate_limiter.add_limit(limit).await; @@ -1185,19 +1157,17 @@ mod test { namespace, 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let limit_update = Limit::new( namespace, 20, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); rate_limiter.add_limit(&limit_orig).await; @@ -1219,28 +1189,25 @@ mod test { namespace, 10, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let limit_2 = Limit::new( namespace, 20, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); let mut limit_3 = Limit::new( namespace, 20, 60, - vec!["req_method == 'GET'"], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); limit_3.set_name("Name is irrelevant too".to_owned()); assert!(rate_limiter.add_limit(&limit_1).await); @@ -1267,10 +1234,9 @@ mod test { namespace, max_hits, 60, - vec!["req_method == 'GET'".to_string()], + vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id"], - ) - .expect("This must be a valid limit!"); + ); for rate_limiter in rate_limiters.iter() { rate_limiter.add_limit(&limit).await; From 1652ba872f9bd080d8f2aa8b75957cbaa4b863ac Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 14:59:01 -0500 Subject: [PATCH 11/20] Parse Limit's variables as exp and resolve Signed-off-by: Alex Snaps --- limitador-server/src/envoy_rls/server.rs | 10 +-- .../src/http_api/request_types.rs | 8 +- limitador-server/src/http_api/server.rs | 2 +- limitador/benches/bench.rs | 2 +- limitador/src/counter.rs | 52 ++++++++++- limitador/src/lib.rs | 24 +++-- limitador/src/limit.rs | 79 +++++++++++----- limitador/src/limit/cel.rs | 29 +++++- limitador/src/storage/disk/rocksdb_storage.rs | 8 +- limitador/src/storage/distributed/mod.rs | 14 ++- limitador/src/storage/in_memory.rs | 19 ++-- limitador/src/storage/keys.rs | 90 +++++++++++++------ limitador/src/storage/redis/counters_cache.rs | 3 +- limitador/src/storage/redis/redis_cached.rs | 21 +++-- limitador/tests/integration_tests.rs | 84 ++++++++--------- 15 files changed, 304 insertions(+), 141 deletions(-) diff --git a/limitador-server/src/envoy_rls/server.rs b/limitador-server/src/envoy_rls/server.rs index b6c3e7a0..8f252ce5 100644 --- a/limitador-server/src/envoy_rls/server.rs +++ b/limitador-server/src/envoy_rls/server.rs @@ -254,7 +254,7 @@ mod tests { 1, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let limiter = RateLimiter::new(10_000); @@ -399,7 +399,7 @@ mod tests { 10, 60, vec!["x == '1'".try_into().expect("failed parsing!")], - vec!["z"], + vec!["z".try_into().expect("failed parsing!")], ), Limit::new( namespace, @@ -409,7 +409,7 @@ mod tests { "x == '1'".try_into().expect("failed parsing!"), "y == '2'".try_into().expect("failed parsing!"), ], - vec!["z"], + vec!["z".try_into().expect("failed parsing!")], ), ] .into_iter() @@ -479,7 +479,7 @@ mod tests { 10, 60, vec!["x == '1'".try_into().expect("failed parsing!")], - vec!["y"], + vec!["y".try_into().expect("failed parsing!")], ); let limiter = RateLimiter::new(10_000); @@ -554,7 +554,7 @@ mod tests { 1, 60, vec!["x == '1'".try_into().expect("failed parsing!")], - vec!["y"], + vec!["y".try_into().expect("failed parsing!")], ); let limiter = RateLimiter::new(10_000); diff --git a/limitador-server/src/http_api/request_types.rs b/limitador-server/src/http_api/request_types.rs index 99adc8b6..6939ba38 100644 --- a/limitador-server/src/http_api/request_types.rs +++ b/limitador-server/src/http_api/request_types.rs @@ -1,5 +1,5 @@ use limitador::counter::Counter as LimitadorCounter; -use limitador::limit::{Limit as LimitadorLimit, ParseError, Predicate}; +use limitador::limit::{Expression, Limit as LimitadorLimit, ParseError, Predicate}; use paperclip::actix::Apiv2Schema; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, HashMap}; @@ -46,6 +46,8 @@ impl TryFrom for LimitadorLimit { fn try_from(limit: Limit) -> Result { let conditions: Result, ParseError> = limit.conditions.into_iter().map(|p| p.try_into()).collect(); + let variables: Result, ParseError> = + limit.variables.into_iter().map(|v| v.try_into()).collect(); let mut limitador_limit = if let Some(id) = limit.id { Self::with_id( @@ -54,7 +56,7 @@ impl TryFrom for LimitadorLimit { limit.max_value, limit.seconds, conditions?, - limit.variables, + variables?, ) } else { Self::new( @@ -62,7 +64,7 @@ impl TryFrom for LimitadorLimit { limit.max_value, limit.seconds, conditions?, - limit.variables, + variables?, ) }; diff --git a/limitador-server/src/http_api/server.rs b/limitador-server/src/http_api/server.rs index d2c26b03..d436e2a0 100644 --- a/limitador-server/src/http_api/server.rs +++ b/limitador-server/src/http_api/server.rs @@ -554,7 +554,7 @@ mod tests { max, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); match &limiter { diff --git a/limitador/benches/bench.rs b/limitador/benches/bench.rs index edccd9e2..b6c0cf70 100644 --- a/limitador/benches/bench.rs +++ b/limitador/benches/bench.rs @@ -540,7 +540,7 @@ fn generate_test_limits(scenario: &TestScenario) -> (Vec, Vec>>(limit: L, set_variables: HashMap) -> Self { - // TODO: check that all the variables defined in the limit are set. + pub fn new>>( + limit: L, + set_variables: HashMap, + ) -> LimitadorResult { + let limit = limit.into(); + let mut vars = set_variables; + vars.retain(|var, _| limit.has_variable(var)); + let variables = limit.resolve_variables(vars)?; + Ok(Self { + limit, + set_variables: variables, + remaining: None, + expires_in: None, + }) + } + + pub(super) fn resolved_vars>>( + limit: L, + set_variables: HashMap, + ) -> LimitadorResult { let limit = limit.into(); let mut vars = set_variables; vars.retain(|var, _| limit.has_variable(var)); - Self { + Ok(Self { limit, set_variables: vars.into_iter().collect(), remaining: None, expires_in: None, - } + }) } #[cfg(any(feature = "redis_storage", feature = "disk_storage"))] @@ -120,3 +139,28 @@ impl PartialEq for Counter { self.limit == other.limit && self.set_variables == other.set_variables } } + +#[cfg(test)] +mod tests { + use crate::counter::Counter; + use crate::limit::Limit; + use std::collections::HashMap; + + #[test] + fn resolves_variables() { + let limit = Limit::new( + "", + 10, + 60, + Vec::default(), + ["int(x) * 3".try_into().expect("failed parsing!")], + ); + let key = "x".to_string(); + let counter = Counter::new(limit, HashMap::from([(key.clone(), "14".to_string())])) + .expect("failed creating counter"); + assert_eq!( + counter.set_variables.get(&key), + Some("42".to_string()).as_ref() + ); + } +} diff --git a/limitador/src/lib.rs b/limitador/src/lib.rs index 66a82e35..c7a652b4 100644 --- a/limitador/src/lib.rs +++ b/limitador/src/lib.rs @@ -55,7 +55,7 @@ //! 10, //! 60, //! vec!["req_method == 'GET'".try_into().expect("failed parsing!")], -//! vec!["user_id"], +//! vec!["user_id".try_into().expect("failed parsing!")], //! ); //! ``` //! @@ -72,7 +72,7 @@ //! 10, //! 60, //! vec!["req_method == 'GET'".try_into().expect("failed parsing!")], -//! vec!["user_id"], +//! vec!["user_id".try_into().expect("failed parsing!")], //! ); //! let mut rate_limiter = RateLimiter::new(1000); //! @@ -104,7 +104,7 @@ //! 2, //! 60, //! vec!["req_method == 'GET'".try_into().expect("failed parsing!")], -//! vec!["user_id"], +//! vec!["user_id".try_into().expect("failed parsing!")], //! ); //! rate_limiter.add_limit(limit); //! @@ -168,7 +168,7 @@ //! 10, //! 60, //! vec!["req_method == 'GET'".try_into().expect("failed parsing!")], -//! vec!["user_id"], +//! vec!["user_id".try_into().expect("failed parsing!")], //! ); //! //! async { @@ -481,13 +481,11 @@ impl RateLimiter { ) -> LimitadorResult> { let limits = self.storage.get_limits(namespace); - let counters = limits + limits .iter() .filter(|lim| lim.applies(values)) .map(|lim| Counter::new(Arc::clone(lim), values.clone())) - .collect(); - - Ok(counters) + .collect() } } @@ -660,13 +658,11 @@ impl AsyncRateLimiter { ) -> LimitadorResult> { let limits = self.storage.get_limits(namespace); - let counters = limits + limits .iter() .filter(|lim| lim.applies(values)) .map(|lim| Counter::new(Arc::clone(lim), values.clone())) - .collect(); - - Ok(counters) + .collect() } } @@ -693,7 +689,7 @@ fn classify_limits_by_namespace( #[cfg(test)] mod test { - use crate::limit::Limit; + use crate::limit::{Expression, Limit}; use crate::RateLimiter; use std::collections::HashMap; @@ -702,7 +698,7 @@ mod test { let rl = RateLimiter::new(100); let namespace = "foo"; - let l = Limit::new(namespace, 42, 100, vec![], Vec::::default()); + let l = Limit::new(namespace, 42, 100, vec![], Vec::::default()); rl.add_limit(l.clone()); let limits = rl.get_limits(&namespace.into()); assert_eq!(limits.len(), 1); diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index afc3f054..a67af038 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -1,7 +1,7 @@ use cel::Context; use serde::{Deserialize, Serialize}; use std::cmp::Ordering; -use std::collections::{BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; use std::fmt::Debug; use std::hash::{Hash, Hasher}; @@ -45,7 +45,7 @@ pub struct Limit { // Need to sort to generate the same object when using the JSON as a key or // value in Redis. conditions: BTreeSet, - variables: BTreeSet, + variables: BTreeSet, } impl Limit { @@ -54,7 +54,7 @@ impl Limit { max_value: u64, seconds: u64, conditions: impl IntoIterator, - variables: impl IntoIterator>, + variables: impl IntoIterator, ) -> Self where >::Error: Debug, @@ -66,7 +66,7 @@ impl Limit { seconds, name: None, conditions: conditions.into_iter().collect(), - variables: variables.into_iter().map(|var| var.into()).collect(), + variables: variables.into_iter().collect(), } } @@ -76,7 +76,7 @@ impl Limit { max_value: u64, seconds: u64, conditions: impl IntoIterator, - variables: impl IntoIterator>, + variables: impl IntoIterator, ) -> Self { Self { id: Some(id.into()), @@ -85,7 +85,7 @@ impl Limit { seconds, name: None, conditions: conditions.into_iter().collect(), - variables: variables.into_iter().map(|var| var.into()).collect(), + variables: variables.into_iter().collect(), } } @@ -125,21 +125,41 @@ impl Limit { } pub fn variables(&self) -> HashSet { - self.variables.iter().map(|var| var.into()).collect() + self.variables + .iter() + .map(|var| var.source().into()) + .collect() + } + + pub fn resolve_variables( + &self, + vars: HashMap, + ) -> Result, EvaluationError> { + let ctx = Context::new(self, String::default(), &vars); + let mut map = BTreeMap::new(); + for variable in &self.variables { + let name = variable.variables().concat().to_string(); + let value = variable.eval(&ctx)?; + map.insert(name, value); + } + Ok(map) } #[cfg(feature = "disk_storage")] pub(crate) fn variables_for_key(&self) -> Vec<&str> { let mut variables = Vec::with_capacity(self.variables.len()); for var in &self.variables { - variables.push(var.as_str()); + variables.push(var.source()); } variables.sort(); variables } pub fn has_variable(&self, var: &str) -> bool { - self.variables.contains(var) + self.variables + .iter() + .flat_map(|v| v.variables()) + .any(|v| v.as_str() == var) } pub fn applies(&self, values: &HashMap) -> bool { @@ -149,7 +169,10 @@ impl Limit { .iter() .all(|predicate| predicate.test(&ctx).unwrap()); - let all_vars_are_set = self.variables.iter().all(|var| values.contains_key(var)); + let all_vars_are_set = self + .variables + .iter() + .all(|var| values.contains_key(var.source())); all_conditions_apply && all_vars_are_set } @@ -206,7 +229,7 @@ mod tests { 10, 60, vec!["x == \"5\"".try_into().expect("failed parsing!")], - vec!["y"], + vec!["y".try_into().expect("failed parsing!")], ); assert!(limit.name.is_none()); @@ -222,7 +245,7 @@ mod tests { 10, 60, vec!["x == \"5\"".try_into().expect("failed parsing!")], - vec!["y"], + vec!["y".try_into().expect("failed parsing!")], ); let mut values: HashMap = HashMap::new(); @@ -239,7 +262,7 @@ mod tests { 10, 60, vec!["x == \"5\"".try_into().expect("failed parsing!")], - vec!["y"], + vec!["y".try_into().expect("failed parsing!")], ); let mut values: HashMap = HashMap::new(); @@ -256,7 +279,7 @@ mod tests { 10, 60, vec!["x == \"5\"".try_into().expect("failed parsing!")], - vec!["y"], + vec!["y".try_into().expect("failed parsing!")], ); // Notice that "x" is not set @@ -274,7 +297,7 @@ mod tests { 10, 60, vec!["x == \"5\"".try_into().expect("failed parsing!")], - vec!["y"], + vec!["y".try_into().expect("failed parsing!")], ); // Notice that "y" is not set @@ -294,7 +317,7 @@ mod tests { "x == \"5\"".try_into().expect("failed parsing!"), "y == \"2\"".try_into().expect("failed parsing!"), ], - vec!["z"], + vec!["z".try_into().expect("failed parsing!")], ); let mut values: HashMap = HashMap::new(); @@ -315,7 +338,7 @@ mod tests { "x == \"5\"".try_into().expect("failed parsing!"), "y == \"2\"".try_into().expect("failed parsing!"), ], - vec!["z"], + vec!["z".try_into().expect("failed parsing!")], ); let mut values: HashMap = HashMap::new(); @@ -334,7 +357,7 @@ mod tests { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); assert_eq!(limit.id(), Some("test_id")) @@ -348,7 +371,7 @@ mod tests { 42, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let mut limit2 = Limit::new( @@ -364,6 +387,18 @@ mod tests { assert_eq!(limit1, limit2); } + #[test] + fn resolves_variables() { + let limit = Limit::new( + "", + 10, + 60, + Vec::default(), + ["int(x) * 3".try_into().expect("failed parsing!")], + ); + assert!(limit.has_variable("x")); + } + #[test] fn conditions_have_limit_info() { let mut limit = Limit::new( @@ -373,7 +408,7 @@ mod tests { vec!["limit.name == 'named_limit'" .try_into() .expect("failed parsing!")], - Vec::::default(), + Vec::default(), ); assert!(!limit.applies(&HashMap::default())); @@ -389,7 +424,7 @@ mod tests { "limit.id == 'my_id'".try_into().expect("failed parsing!"), "limit.name == null".try_into().expect("failed parsing!"), ], - Vec::::default(), + Vec::default(), ); assert!(limit.applies(&HashMap::default())); @@ -401,7 +436,7 @@ mod tests { vec!["limit.id == 'other_id'" .try_into() .expect("failed parsing!")], - Vec::::default(), + Vec::default(), ); assert!(!limit.applies(&HashMap::default())); } diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index d910c0c9..df0f77f2 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -149,6 +149,19 @@ impl Expression { pub fn resolve(&self, ctx: &Context) -> Result { Value::resolve(&self.expression, &ctx.ctx) } + + pub fn source(&self) -> &str { + self.source.as_str() + } + + pub fn variables(&self) -> Vec { + self.expression + .references() + .variables() + .into_iter() + .map(String::from) + .collect() + } } fn err_on_value(val: Value) -> EvaluationError { @@ -178,6 +191,14 @@ impl TryFrom for Expression { } } +impl TryFrom<&str> for Predicate { + type Error = ParseError; + + fn try_from(value: &str) -> Result { + Self::parse(value) + } +} + impl From for String { fn from(value: Expression) -> Self { value.source @@ -192,6 +213,12 @@ impl PartialEq for Expression { impl Eq for Expression {} +impl Hash for Expression { + fn hash(&self, state: &mut H) { + self.source.hash(state); + } +} + impl PartialOrd for Expression { fn partial_cmp(&self, other: &Self) -> Option { Some(self.cmp(other)) @@ -276,7 +303,7 @@ impl TryFrom for Predicate { } } -impl TryFrom<&str> for Predicate { +impl TryFrom<&str> for Expression { type Error = ParseError; fn try_from(value: &str) -> Result { diff --git a/limitador/src/storage/disk/rocksdb_storage.rs b/limitador/src/storage/disk/rocksdb_storage.rs index 68ef7369..05d04f04 100644 --- a/limitador/src/storage/disk/rocksdb_storage.rs +++ b/limitador/src/storage/disk/rocksdb_storage.rs @@ -247,9 +247,13 @@ mod tests { 1, 2, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); - let counter = Counter::new(limit, HashMap::default()); + let counter = Counter::new( + limit, + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .unwrap(); let tmp = TempDir::new().expect("We should have a dir!"); { diff --git a/limitador/src/storage/distributed/mod.rs b/limitador/src/storage/distributed/mod.rs index 020918c5..81d9d6ed 100644 --- a/limitador/src/storage/distributed/mod.rs +++ b/limitador/src/storage/distributed/mod.rs @@ -47,7 +47,8 @@ impl CounterStorage for CrInMemoryStorage { let key = encode_limit_to_key(limit); limits.entry(key.clone()).or_insert(Arc::new(CounterEntry { key, - counter: Counter::new(limit.clone(), HashMap::default()), + counter: Counter::new(limit.clone(), HashMap::default()) + .expect("counter creation can't fail! no vars to resolve!"), value: CrCounterValue::new( self.identifier.clone(), limit.max_value(), @@ -333,6 +334,15 @@ fn encode_counter_to_key(counter: &Counter) -> Vec { } fn encode_limit_to_key(limit: &Limit) -> Vec { - let counter = Counter::new(limit.clone(), HashMap::default()); + // fixme this is broken! + let counter = Counter::new( + limit.clone(), + limit + .variables() + .into_iter() + .map(|k| (k, "".to_string())) + .collect(), + ) + .expect("counter creation can't fail! faked vars!"); key_for_counter_v2(&counter) } diff --git a/limitador/src/storage/in_memory.rs b/limitador/src/storage/in_memory.rs index dca294b7..ac5e2c07 100644 --- a/limitador/src/storage/in_memory.rs +++ b/limitador/src/storage/in_memory.rs @@ -211,7 +211,8 @@ impl InMemoryStorage { for (limit, counter) in self.simple_limits.read().unwrap().iter() { if limit.namespace() == namespace { res.insert( - Counter::new(limit.clone(), HashMap::default()), + // todo fixme + Counter::new(limit.clone(), HashMap::default()).unwrap(), counter.clone(), ); } @@ -257,17 +258,25 @@ mod tests { 1, 1, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let limit_2 = Limit::new( namespace, 1, 10, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); - let counter_1 = Counter::new(limit_1, HashMap::default()); - let counter_2 = Counter::new(limit_2, HashMap::default()); + let counter_1 = Counter::new( + limit_1, + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); + let counter_2 = Counter::new( + limit_2, + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); storage.update_counter(&counter_1, 1).unwrap(); storage.update_counter(&counter_2, 1).unwrap(); diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 9bbeddfa..9d4e1d83 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -120,7 +120,7 @@ mod tests { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); assert_eq!( "namespace:{example.com},counters_of_limit:{\"namespace\":\"example.com\",\"seconds\":60,\"conditions\":[\"req_method == 'GET'\"],\"variables\":[\"app_id\"]}".as_bytes(), @@ -135,7 +135,7 @@ mod tests { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); assert_eq!( "\u{2}\u{7}test_id".as_bytes(), @@ -151,9 +151,13 @@ mod tests { 1, 1, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); - let counter = Counter::new(limit.clone(), HashMap::default()); + let counter = Counter::new( + limit.clone(), + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); let raw = key_for_counter(&counter); assert_eq!(counter, partial_counter_from_counter_key(&raw)); } @@ -166,9 +170,13 @@ mod tests { 1, 1, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); - let counter = Counter::new(limit.clone(), HashMap::default()); + let counter = Counter::new( + limit.clone(), + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); let mut other = counter.clone(); other.set_remaining(123); other.set_expires_in(Duration::from_millis(456)); @@ -286,11 +294,11 @@ pub mod bin { seconds, conditions .into_iter() - .map(|p| p.try_into().expect("condition corrupted!")) - .collect::>(), - map.keys(), + .map(|p| p.try_into().expect("condition corrupted!")), + map.keys() + .map(|var| var.as_str().try_into().expect("variable corrupted!")), ); - Counter::new(limit, map) + Counter::resolved_vars(limit, map).expect("counter creation failed!") } 2u8 => { let IdCounterKey { id, variables } = postcard::from_bytes(key).unwrap(); @@ -300,9 +308,16 @@ pub mod bin { .collect(); // we are not able to rebuild the full limit since we only have the id and variables. - let limit = - Limit::with_id::<&str, &str>(id, "", u64::default(), 0, vec![], map.keys()); - Counter::new(limit, map) + let limit = Limit::with_id::<&str, &str>( + id, + "", + u64::default(), + 0, + vec![], + map.keys() + .map(|var| var.as_str().try_into().expect("variable corrupted!")), + ); + Counter::resolved_vars(limit, map).expect("counter creation failed!") } _ => panic!("Unknown version: {}", version), } @@ -338,9 +353,10 @@ pub mod bin { .into_iter() .map(|p| p.try_into().expect("condition corrupted!")) .collect::>(), - map.keys(), + map.keys() + .map(|p| p.as_str().try_into().expect("variable corrupted!")), ); - Counter::new(limit, map) + Counter::resolved_vars(limit, map).unwrap() } #[cfg(test)] @@ -361,13 +377,17 @@ pub mod bin { 1, 2, vec!["foo == 'bar'".try_into().expect("failed parsing!")], - vec!["app_id", "role", "wat"], + vec![ + "app_id".try_into().expect("failed parsing!"), + "role".try_into().expect("failed parsing!"), + "wat".try_into().expect("failed parsing!"), + ], ); let mut vars = HashMap::default(); vars.insert("role".to_string(), "admin".to_string()); vars.insert("app_id".to_string(), "123".to_string()); vars.insert("wat".to_string(), "dunno".to_string()); - let counter = Counter::new(limit.clone(), vars); + let counter = Counter::new(limit.clone(), vars).expect("counter creation failed!"); let raw = key_for_counter(&counter); let key_back: CounterKey = @@ -384,11 +404,11 @@ pub mod bin { 1, 1, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let mut variables = HashMap::default(); variables.insert("app_id".to_string(), "123".to_string()); - let counter = Counter::new(limit.clone(), variables); + let counter = Counter::new(limit.clone(), variables).expect("counter creation failed!"); let raw = key_for_counter(&counter); assert_eq!(counter, partial_counter_from_counter_key(&raw)); } @@ -401,9 +421,13 @@ pub mod bin { 1, 1, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); - let counter = Counter::new(limit, HashMap::default()); + let counter = Counter::new( + limit, + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); let serialized_counter = key_for_counter(&counter); let prefix = prefix_for_namespace(namespace); @@ -418,7 +442,7 @@ pub mod bin { 1, 1, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let limit_with_id = Limit::with_id( "id200", @@ -426,27 +450,35 @@ pub mod bin { 1, 1, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); - let counter_with_id = Counter::new(limit_with_id, HashMap::default()); + let counter_with_id = Counter::new( + limit_with_id, + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); let serialized_with_id_counter = key_for_counter(&counter_with_id); - let counter_without_id = Counter::new(limit_without_id, HashMap::default()); + let counter_without_id = Counter::new( + limit_without_id, + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); let serialized_without_id_counter = key_for_counter(&counter_without_id); // the original key_for_counter continues to encode kinda big - assert_eq!(serialized_without_id_counter.len(), 35); - assert_eq!(serialized_with_id_counter.len(), 35); + assert_eq!(serialized_without_id_counter.len(), 46); + assert_eq!(serialized_with_id_counter.len(), 46); // serialized_counter_v2 will only encode the id.... so it will be smaller for // counters with an id. let serialized_counter_with_id_v2 = key_for_counter_v2(&counter_with_id); - assert_eq!(serialized_counter_with_id_v2.clone().len(), 8); + assert_eq!(serialized_counter_with_id_v2.clone().len(), 19); // but continues to be large for counters without an id. let serialized_counter_without_id_v2 = key_for_counter_v2(&counter_without_id); - assert_eq!(serialized_counter_without_id_v2.clone().len(), 36); + assert_eq!(serialized_counter_without_id_v2.clone().len(), 47); } } } diff --git a/limitador/src/storage/redis/counters_cache.rs b/limitador/src/storage/redis/counters_cache.rs index 1924d63f..37653024 100644 --- a/limitador/src/storage/redis/counters_cache.rs +++ b/limitador/src/storage/redis/counters_cache.rs @@ -681,9 +681,10 @@ mod tests { max_val, 60, vec!["req_method == 'POST'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), values, ) + .expect("failed creating counter") } } diff --git a/limitador/src/storage/redis/redis_cached.rs b/limitador/src/storage/redis/redis_cached.rs index 742d2986..b9b12ca7 100644 --- a/limitador/src/storage/redis/redis_cached.rs +++ b/limitador/src/storage/redis/redis_cached.rs @@ -445,10 +445,11 @@ mod tests { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), - Default::default(), - ); + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); let arc = Arc::new(CachedCounterValue::from_authority( &counter, @@ -507,10 +508,11 @@ mod tests { 10, 60, vec!["req_method == 'POST'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), - Default::default(), - ); + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); let mock_response = Value::Array(vec![ Value::Int(8), @@ -566,10 +568,11 @@ mod tests { 10, 60, vec!["req_method == 'POST'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), - Default::default(), - ); + HashMap::from([("app_id".to_string(), "foo".to_string())]), + ) + .expect("counter creation failed!"); let error: RedisError = io::Error::new(io::ErrorKind::TimedOut, "That was long!").into(); assert!(error.is_timeout()); diff --git a/limitador/tests/integration_tests.rs b/limitador/tests/integration_tests.rs index 7572d766..7a71372c 100644 --- a/limitador/tests/integration_tests.rs +++ b/limitador/tests/integration_tests.rs @@ -221,14 +221,14 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), Limit::new( "second_namespace", 20, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), ]; @@ -253,7 +253,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let lim2 = Limit::new( @@ -261,7 +261,7 @@ mod test { 20, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); for limit in [&lim1, &lim2] { @@ -285,7 +285,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -305,7 +305,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - Vec::::new(), + Vec::default(), ); rate_limiter.add_limit(&limit).await; @@ -327,7 +327,7 @@ mod test { 10, 60, vec!["req_method == 'POST'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let limit_2 = Limit::new( @@ -335,7 +335,7 @@ mod test { 5, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit_1).await; @@ -354,7 +354,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -371,7 +371,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -406,14 +406,14 @@ mod test { 10, 60, vec!["req_method == 'POST'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), Limit::new( namespace, 5, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), ]; @@ -438,7 +438,7 @@ mod test { 10, 60, vec!["x == '10'".try_into().expect("failed parsing!")], - vec!["z"], + vec!["z".try_into().expect("failed parsing!")], )) .await; rate_limiter @@ -447,7 +447,7 @@ mod test { 5, 60, vec!["x == '10'".try_into().expect("failed parsing!")], - vec!["z"], + vec!["z".try_into().expect("failed parsing!")], )) .await; @@ -464,7 +464,7 @@ mod test { 5, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -498,7 +498,7 @@ mod test { max_hits, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -535,7 +535,7 @@ mod test { max_hits, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -572,14 +572,14 @@ mod test { max_hits, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), Limit::new( namespace, max_hits + 1, 60, vec!["req_method == 'POST'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ), ]; @@ -640,7 +640,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -675,7 +675,7 @@ mod test { max, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -698,7 +698,7 @@ mod test { max_hits, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -752,7 +752,7 @@ mod test { 0, // So reporting 1 more would not be allowed 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -776,7 +776,7 @@ mod test { 0, // So reporting 1 more would not be allowed 60, Vec::default(), // unconditional - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -799,7 +799,7 @@ mod test { max_hits, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -836,7 +836,7 @@ mod test { max_hits, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -886,7 +886,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -915,7 +915,7 @@ mod test { 0, // So reporting 1 more would not be allowed 60, Vec::default(), // unconditional - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -943,7 +943,7 @@ mod test { max_hits, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -1000,7 +1000,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -1021,7 +1021,7 @@ mod test { 10, limit_time, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -1046,7 +1046,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let second_limit = Limit::new( @@ -1054,7 +1054,7 @@ mod test { 20, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter @@ -1085,7 +1085,7 @@ mod test { max_value, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit).await; @@ -1124,7 +1124,7 @@ mod test { 10, 1, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let limit_to_be_deleted = Limit::new( @@ -1132,7 +1132,7 @@ mod test { 20, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); for limit in [&limit_to_be_kept, &limit_to_be_deleted].iter() { @@ -1158,7 +1158,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let limit_update = Limit::new( @@ -1166,7 +1166,7 @@ mod test { 20, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); rate_limiter.add_limit(&limit_orig).await; @@ -1190,7 +1190,7 @@ mod test { 10, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let limit_2 = Limit::new( @@ -1198,7 +1198,7 @@ mod test { 20, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); let mut limit_3 = Limit::new( @@ -1206,7 +1206,7 @@ mod test { 20, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); limit_3.set_name("Name is irrelevant too".to_owned()); @@ -1235,7 +1235,7 @@ mod test { max_hits, 60, vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id"], + vec!["app_id".try_into().expect("failed parsing!")], ); for rate_limiter in rate_limiters.iter() { From 9fe7d80f8f246925f6dd3636e70fba8e72a42bd3 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 17:29:06 -0500 Subject: [PATCH 12/20] key variable with full expression Signed-off-by: Alex Snaps --- Cargo.lock | 8 ++++---- limitador/Cargo.toml | 4 ++-- limitador/src/counter.rs | 15 +++++++++------ limitador/src/limit.rs | 2 +- 4 files changed, 16 insertions(+), 13 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6ed6c974..3b5909bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -622,23 +622,23 @@ dependencies = [ [[package]] name = "cel-interpreter" version = "0.9.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "5675bdc8ece076b0a4f5ac6c20724e7373919ed698e00dbf33825682a7b3a679" +source = "git+https://github.com/clarkmcc/cel-rust?rev=5b02b08#5b02b0817ced05c7cfc1c72bab03bc97bbfa2dea" dependencies = [ + "base64 0.22.1", "cel-parser", "chrono", "nom", "paste", "regex", "serde", + "serde_json", "thiserror 1.0.69", ] [[package]] name = "cel-parser" version = "0.8.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "6e1df6c220727ff1f7b52a41699d8c71ec52e8ae58d01a9768469a916e1f22eb" +source = "git+https://github.com/clarkmcc/cel-rust?rev=5b02b08#5b02b0817ced05c7cfc1c72bab03bc97bbfa2dea" dependencies = [ "lalrpop", "lalrpop-util", diff --git a/limitador/Cargo.toml b/limitador/Cargo.toml index fdaaf03a..ed822b1d 100644 --- a/limitador/Cargo.toml +++ b/limitador/Cargo.toml @@ -53,8 +53,8 @@ tonic = { version = "0.12.3", optional = true } tonic-reflection = { version = "0.12.3", optional = true } prost = { version = "0.13.3", optional = true } prost-types = { version = "0.13.3", optional = true } -cel-parser = "0.8.0" -cel-interpreter = { version = "0.9.0", features = ["chrono", "regex"] } +cel-interpreter = { git = "https://github.com/clarkmcc/cel-rust", rev = "5b02b08", features = ["json", "regex", "chrono"] } +cel-parser = { git = "https://github.com/clarkmcc/cel-rust", rev = "5b02b08" } [dev-dependencies] serial_test = "3.0" diff --git a/limitador/src/counter.rs b/limitador/src/counter.rs index 2c5494bf..7dfebe0b 100644 --- a/limitador/src/counter.rs +++ b/limitador/src/counter.rs @@ -148,19 +148,22 @@ mod tests { #[test] fn resolves_variables() { + let var = "timestamp(ts).getHours()"; let limit = Limit::new( "", 10, 60, Vec::default(), - ["int(x) * 3".try_into().expect("failed parsing!")], + [var.try_into().expect("failed parsing!")], ); - let key = "x".to_string(); - let counter = Counter::new(limit, HashMap::from([(key.clone(), "14".to_string())])) - .expect("failed creating counter"); + let counter = Counter::new( + limit, + HashMap::from([("ts".to_string(), "2019-10-12T13:20:50.52Z".to_string())]), + ) + .expect("failed creating counter"); assert_eq!( - counter.set_variables.get(&key), - Some("42".to_string()).as_ref() + counter.set_variables.get(var), + Some("13".to_string()).as_ref() ); } } diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index a67af038..5ad0c56e 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -138,7 +138,7 @@ impl Limit { let ctx = Context::new(self, String::default(), &vars); let mut map = BTreeMap::new(); for variable in &self.variables { - let name = variable.variables().concat().to_string(); + let name = variable.source().into(); let value = variable.eval(&ctx)?; map.insert(name, value); } From 7c33b776c0b4f1aed4482227be058c4ef281d5c5 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Wed, 27 Nov 2024 17:32:34 -0500 Subject: [PATCH 13/20] More time needed sometimes Signed-off-by: Alex Snaps --- limitador/src/storage/redis/counters_cache.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/limitador/src/storage/redis/counters_cache.rs b/limitador/src/storage/redis/counters_cache.rs index 37653024..2645996d 100644 --- a/limitador/src/storage/redis/counters_cache.rs +++ b/limitador/src/storage/redis/counters_cache.rs @@ -569,7 +569,8 @@ mod tests { .consume(1, |items| { assert_eq!(items.len(), 1); assert!( - SystemTime::now().duration_since(start).unwrap() < Duration::from_millis(5) + SystemTime::now().duration_since(start).unwrap() + < Duration::from_millis(10) ); async { Ok::<(), ()>(()) } }) From b816ec32e1526f64b3999df714ea8c1c241e83d5 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Mon, 2 Dec 2024 19:20:59 -0500 Subject: [PATCH 14/20] Opening seam for injectable root Signed-off-by: Alex Snaps --- limitador/src/lib.rs | 13 ++++++------ limitador/src/limit.rs | 30 ++++++++++++++-------------- limitador/src/limit/cel.rs | 41 ++++++++++++++++++++++++++++++-------- 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/limitador/src/lib.rs b/limitador/src/lib.rs index c7a652b4..72507ea9 100644 --- a/limitador/src/lib.rs +++ b/limitador/src/lib.rs @@ -192,14 +192,13 @@ // TODO this needs review to reduce the bloat pulled in by dependencies #![allow(clippy::multiple_crate_versions)] -use std::collections::{HashMap, HashSet}; -use std::sync::Arc; - use crate::counter::Counter; use crate::errors::LimitadorError; use crate::limit::{Limit, Namespace}; use crate::storage::in_memory::InMemoryStorage; use crate::storage::{AsyncCounterStorage, AsyncStorage, Authorization, CounterStorage, Storage}; +use std::collections::{HashMap, HashSet}; +use std::sync::Arc; #[macro_use] extern crate core; @@ -480,10 +479,10 @@ impl RateLimiter { values: &HashMap, ) -> LimitadorResult> { let limits = self.storage.get_limits(namespace); - + let ctx = values.into(); limits .iter() - .filter(|lim| lim.applies(values)) + .filter(|lim| lim.applies(&ctx)) .map(|lim| Counter::new(Arc::clone(lim), values.clone())) .collect() } @@ -657,10 +656,10 @@ impl AsyncRateLimiter { values: &HashMap, ) -> LimitadorResult> { let limits = self.storage.get_limits(namespace); - + let ctx = values.into(); limits .iter() - .filter(|lim| lim.applies(values)) + .filter(|lim| lim.applies(&ctx)) .map(|lim| Counter::new(Arc::clone(lim), values.clone())) .collect() } diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 5ad0c56e..3b0bc25c 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -135,7 +135,7 @@ impl Limit { &self, vars: HashMap, ) -> Result, EvaluationError> { - let ctx = Context::new(self, String::default(), &vars); + let ctx = Context::new(String::default(), &vars); let mut map = BTreeMap::new(); for variable in &self.variables { let name = variable.source().into(); @@ -162,17 +162,17 @@ impl Limit { .any(|v| v.as_str() == var) } - pub fn applies(&self, values: &HashMap) -> bool { - let ctx = Context::new(self, String::default(), values); + pub fn applies(&self, ctx: &Context) -> bool { + let ctx = ctx.for_limit(self); let all_conditions_apply = self .conditions .iter() - .all(|predicate| predicate.test(&ctx).unwrap()); + .all(|predicate| predicate.test(&ctx.for_limit(self)).unwrap()); let all_vars_are_set = self .variables .iter() - .all(|var| values.contains_key(var.source())); + .all(|var| ctx.has_variable(var.source())); all_conditions_apply && all_vars_are_set } @@ -252,7 +252,7 @@ mod tests { values.insert("x".into(), "5".into()); values.insert("y".into(), "1".into()); - assert!(limit.applies(&values)) + assert!(limit.applies(&(&values).into())) } #[test] @@ -269,7 +269,7 @@ mod tests { values.insert("x".into(), "1".into()); values.insert("y".into(), "1".into()); - assert!(!limit.applies(&values)) + assert!(!limit.applies(&(&values).into())) } #[test] @@ -287,7 +287,7 @@ mod tests { values.insert("a".into(), "1".into()); values.insert("y".into(), "1".into()); - assert!(!limit.applies(&values)) + assert!(!limit.applies(&(&values).into())) } #[test] @@ -304,7 +304,7 @@ mod tests { let mut values: HashMap = HashMap::new(); values.insert("x".into(), "5".into()); - assert!(!limit.applies(&values)) + assert!(!limit.applies(&(&values).into())) } #[test] @@ -325,7 +325,7 @@ mod tests { values.insert("y".into(), "2".into()); values.insert("z".into(), "1".into()); - assert!(limit.applies(&values)) + assert!(limit.applies(&(&values).into())) } #[test] @@ -346,7 +346,7 @@ mod tests { values.insert("y".into(), "2".into()); values.insert("z".into(), "1".into()); - assert!(!limit.applies(&values)) + assert!(!limit.applies(&(&values).into())) } #[test] @@ -410,10 +410,10 @@ mod tests { .expect("failed parsing!")], Vec::default(), ); - assert!(!limit.applies(&HashMap::default())); + assert!(!limit.applies(&Context::default())); limit.set_name("named_limit".to_string()); - assert!(limit.applies(&HashMap::default())); + assert!(limit.applies(&Context::default())); let limit = Limit::with_id( "my_id", @@ -426,7 +426,7 @@ mod tests { ], Vec::default(), ); - assert!(limit.applies(&HashMap::default())); + assert!(limit.applies(&(&HashMap::default()).into())); let limit = Limit::with_id( "my_id", @@ -438,6 +438,6 @@ mod tests { .expect("failed parsing!")], Vec::default(), ); - assert!(!limit.applies(&HashMap::default())); + assert!(!limit.applies(&Context::default())); } } diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index df0f77f2..fbc2ebb4 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -1,13 +1,12 @@ use crate::limit::Limit; use cel_interpreter::{ExecutionError, Value}; +pub use errors::{EvaluationError, ParseError}; use serde::{Deserialize, Serialize}; use std::cmp::Ordering; use std::collections::{HashMap, HashSet}; use std::hash::{Hash, Hasher}; use std::sync::Arc; -pub use errors::{EvaluationError, ParseError}; - pub(super) mod errors { use cel_interpreter::ExecutionError; use std::error::Error; @@ -78,8 +77,8 @@ pub struct Context<'a> { ctx: cel_interpreter::Context<'a>, } -impl Context<'_> { - pub(crate) fn new(limit: &Limit, root: String, values: &HashMap) -> Self { +impl<'a> Context<'a> { + pub(crate) fn new(root: String, values: &HashMap) -> Self { let mut ctx = cel_interpreter::Context::default(); if root.is_empty() { @@ -91,6 +90,17 @@ impl Context<'_> { ctx.add_variable_from_value(root, Value::Map(map)); } + Self { + variables: values.keys().cloned().collect(), + ctx, + } + } + + pub(crate) fn for_limit<'b>(&'b self, limit: &Limit) -> Self + where + 'b: 'a, + { + let mut inner = self.ctx.new_inner_scope(); let limit_data = cel_interpreter::objects::Map::from(HashMap::from([ ( "name", @@ -109,13 +119,28 @@ impl Context<'_> { .unwrap_or(Value::Null), ), ])); - ctx.add_variable_from_value("limit", Value::Map(limit_data)); - + inner.add_variable_from_value("limit", Value::Map(limit_data)); Self { - variables: values.keys().cloned().collect(), - ctx, + variables: self.variables.clone(), + ctx: inner, } } + + pub(crate) fn has_variable(&self, name: &str) -> bool { + self.variables.contains(name) + } +} + +impl Default for Context<'_> { + fn default() -> Self { + Self::new(String::default(), &HashMap::default()) + } +} + +impl From<&HashMap> for Context<'_> { + fn from(value: &HashMap) -> Self { + Self::new(String::default(), value) + } } #[derive(Clone, Debug, Serialize, Deserialize)] From db9b97f9e0e3f9e9e8ecd879b68e8e70400831b4 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Mon, 2 Dec 2024 20:27:34 -0500 Subject: [PATCH 15/20] Properly check for variables when they are cel Signed-off-by: Alex Snaps --- limitador/src/limit.rs | 37 +++++++++++++++++++++++++++++++++---- limitador/src/limit/cel.rs | 4 ++-- 2 files changed, 35 insertions(+), 6 deletions(-) diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 3b0bc25c..5aeaa046 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -169,10 +169,14 @@ impl Limit { .iter() .all(|predicate| predicate.test(&ctx.for_limit(self)).unwrap()); - let all_vars_are_set = self - .variables - .iter() - .all(|var| ctx.has_variable(var.source())); + let all_vars_are_set = self.variables.iter().all(|var| { + ctx.has_variables( + &var.variables() + .iter() + .map(String::as_str) + .collect::>(), + ) + }); all_conditions_apply && all_vars_are_set } @@ -220,6 +224,7 @@ impl PartialEq for Limit { #[cfg(test)] mod tests { use super::*; + use crate::counter::Counter; use std::cmp::Ordering::Equal; #[test] @@ -440,4 +445,28 @@ mod tests { ); assert!(!limit.applies(&Context::default())); } + + #[test] + fn cel_limit_applies() { + let limit = Limit::new( + "ns", + 42, + 10, + vec!["foo.contains('bar')".try_into().expect("failed parsing!")], + vec!["bar.endsWith('baz')".try_into().expect("failed parsing!")], + ); + let map = HashMap::from([ + ("foo".to_string(), "nice bar!".to_string()), + ("bar".to_string(), "foo,baz".to_string()), + ]); + let ctx = Context::new(String::default(), &map); + assert!(limit.applies(&ctx)); + assert_eq!( + Counter::new(limit, map) + .expect("failed") + .set_variables() + .get("bar.endsWith('baz')"), + Some(&"true".to_string()) + ); + } } diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index fbc2ebb4..8a4561cf 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -126,8 +126,8 @@ impl<'a> Context<'a> { } } - pub(crate) fn has_variable(&self, name: &str) -> bool { - self.variables.contains(name) + pub(crate) fn has_variables(&self, names: &[&str]) -> bool { + names.iter().all(|name| self.variables.contains(*name)) } } From ef45382c5cd525676fe302293f3432927fddc8f6 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 3 Dec 2024 05:34:43 -0500 Subject: [PATCH 16/20] Treat CEL NoSuchKey as ignored false in pred, and miss in keys Signed-off-by: Alex Snaps --- limitador/src/counter.rs | 19 ++++--- limitador/src/lib.rs | 12 +++- limitador/src/limit.rs | 13 +++-- limitador/src/limit/cel.rs | 56 ++++++++++++++----- limitador/src/storage/disk/rocksdb_storage.rs | 3 +- limitador/src/storage/distributed/mod.rs | 6 +- limitador/src/storage/in_memory.rs | 10 +++- limitador/src/storage/keys.rs | 23 +++++--- limitador/src/storage/redis/counters_cache.rs | 1 + limitador/src/storage/redis/redis_cached.rs | 9 ++- 10 files changed, 107 insertions(+), 45 deletions(-) diff --git a/limitador/src/counter.rs b/limitador/src/counter.rs index 7dfebe0b..788bf8a7 100644 --- a/limitador/src/counter.rs +++ b/limitador/src/counter.rs @@ -20,18 +20,21 @@ impl Counter { pub fn new>>( limit: L, set_variables: HashMap, - ) -> LimitadorResult { + ) -> LimitadorResult> { let limit = limit.into(); let mut vars = set_variables; vars.retain(|var, _| limit.has_variable(var)); let variables = limit.resolve_variables(vars)?; - Ok(Self { - limit, - set_variables: variables, - remaining: None, - expires_in: None, - }) + match variables { + None => Ok(None), + Some(variables) => Ok(Some(Self { + limit, + set_variables: variables, + remaining: None, + expires_in: None, + })), + } } pub(super) fn resolved_vars>>( @@ -162,7 +165,7 @@ mod tests { ) .expect("failed creating counter"); assert_eq!( - counter.set_variables.get(var), + counter.unwrap().set_variables.get(var), Some("13".to_string()).as_ref() ); } diff --git a/limitador/src/lib.rs b/limitador/src/lib.rs index 72507ea9..d5016a18 100644 --- a/limitador/src/lib.rs +++ b/limitador/src/lib.rs @@ -483,7 +483,11 @@ impl RateLimiter { limits .iter() .filter(|lim| lim.applies(&ctx)) - .map(|lim| Counter::new(Arc::clone(lim), values.clone())) + .filter_map(|lim| match Counter::new(Arc::clone(lim), values.clone()) { + Ok(None) => None, + Ok(Some(c)) => Some(Ok(c)), + Err(e) => Some(Err(e)), + }) .collect() } } @@ -660,7 +664,11 @@ impl AsyncRateLimiter { limits .iter() .filter(|lim| lim.applies(&ctx)) - .map(|lim| Counter::new(Arc::clone(lim), values.clone())) + .filter_map(|lim| match Counter::new(Arc::clone(lim), values.clone()) { + Ok(None) => None, + Ok(Some(c)) => Some(Ok(c)), + Err(e) => Some(Err(e)), + }) .collect() } } diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 5aeaa046..6653cb07 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -134,15 +134,19 @@ impl Limit { pub fn resolve_variables( &self, vars: HashMap, - ) -> Result, EvaluationError> { + ) -> Result>, EvaluationError> { let ctx = Context::new(String::default(), &vars); let mut map = BTreeMap::new(); for variable in &self.variables { let name = variable.source().into(); - let value = variable.eval(&ctx)?; - map.insert(name, value); + match variable.eval(&ctx)? { + None => return Ok(None), + Some(value) => { + map.insert(name, value); + } + } } - Ok(map) + Ok(Some(map)) } #[cfg(feature = "disk_storage")] @@ -464,6 +468,7 @@ mod tests { assert_eq!( Counter::new(limit, map) .expect("failed") + .unwrap() .set_variables() .get("bar.endsWith('baz')"), Some(&"true".to_string()) diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index 8a4561cf..4674692a 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -159,19 +159,25 @@ impl Expression { } } - pub fn eval(&self, ctx: &Context) -> Result { - match self.resolve(ctx)? { - Value::Int(i) => Ok(i.to_string()), - Value::UInt(i) => Ok(i.to_string()), - Value::Float(f) => Ok(f.to_string()), - Value::String(s) => Ok(s.to_string()), - Value::Null => Ok("null".to_owned()), - Value::Bool(b) => Ok(b.to_string()), - val => Err(err_on_value(val)), + pub fn eval(&self, ctx: &Context) -> Result, EvaluationError> { + let result = self.resolve(ctx); + match result { + Ok(value) => match value { + Value::Int(i) => Ok(i.to_string()), + Value::UInt(i) => Ok(i.to_string()), + Value::Float(f) => Ok(f.to_string()), + Value::String(s) => Ok(s.to_string()), + Value::Null => Ok("null".to_owned()), + Value::Bool(b) => Ok(b.to_string()), + val => Err(err_on_value(val)), + } + .map(Some), + Err(ExecutionError::NoSuchKey(_)) => Ok(None), + Err(err) => Err(err.into()), } } - pub fn resolve(&self, ctx: &Context) -> Result { + pub(super) fn resolve(&self, ctx: &Context) -> Result { Value::resolve(&self.expression, &ctx.ctx) } @@ -287,9 +293,14 @@ impl Predicate { { return Ok(false); } - match self.expression.resolve(ctx)? { - Value::Bool(b) => Ok(b), - v => Err(err_on_value(v)), + + match self.expression.resolve(ctx) { + Ok(value) => match value { + Value::Bool(b) => Ok(b), + v => Err(err_on_value(v)), + }, + Err(ExecutionError::NoSuchKey(_)) => Ok(false), + Err(err) => Err(err.into()), } } } @@ -345,12 +356,12 @@ impl From for String { #[cfg(test)] mod tests { use super::{Context, Expression, Predicate}; - use std::collections::HashSet; + use std::collections::{HashMap, HashSet}; #[test] fn expression() { let exp = Expression::parse("100").expect("failed to parse"); - assert_eq!(exp.eval(&ctx()), Ok(String::from("100"))); + assert_eq!(exp.eval(&ctx()), Ok(Some(String::from("100")))); } #[test] @@ -377,6 +388,21 @@ mod tests { assert_eq!(pred.test(&ctx()), Ok(true)); } + #[test] + fn predicate_no_var() { + let pred = Predicate::parse("not_there == 42").expect("failed to parse"); + assert_eq!(pred.test(&ctx()), Ok(false)); + } + + #[test] + fn predicate_no_key() { + let pred = Predicate::parse("there.not == 42").expect("failed to parse"); + assert_eq!( + pred.test(&(&HashMap::from([("there".to_string(), String::default())])).into()), + Ok(false) + ); + } + #[test] fn unexpected_value_predicate() { let pred = Predicate::parse("42").expect("failed to parse"); diff --git a/limitador/src/storage/disk/rocksdb_storage.rs b/limitador/src/storage/disk/rocksdb_storage.rs index 05d04f04..356aaaaf 100644 --- a/limitador/src/storage/disk/rocksdb_storage.rs +++ b/limitador/src/storage/disk/rocksdb_storage.rs @@ -253,7 +253,8 @@ mod tests { limit, HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .unwrap(); + .unwrap() + .expect("must have a counter"); let tmp = TempDir::new().expect("We should have a dir!"); { diff --git a/limitador/src/storage/distributed/mod.rs b/limitador/src/storage/distributed/mod.rs index 81d9d6ed..aee13fa2 100644 --- a/limitador/src/storage/distributed/mod.rs +++ b/limitador/src/storage/distributed/mod.rs @@ -48,7 +48,8 @@ impl CounterStorage for CrInMemoryStorage { limits.entry(key.clone()).or_insert(Arc::new(CounterEntry { key, counter: Counter::new(limit.clone(), HashMap::default()) - .expect("counter creation can't fail! no vars to resolve!"), + .expect("counter creation can't fail! no vars to resolve!") + .expect("must have a counter"), value: CrCounterValue::new( self.identifier.clone(), limit.max_value(), @@ -343,6 +344,7 @@ fn encode_limit_to_key(limit: &Limit) -> Vec { .map(|k| (k, "".to_string())) .collect(), ) - .expect("counter creation can't fail! faked vars!"); + .expect("counter creation can't fail! faked vars!") + .expect("must have a counter"); key_for_counter_v2(&counter) } diff --git a/limitador/src/storage/in_memory.rs b/limitador/src/storage/in_memory.rs index ac5e2c07..3b273d9c 100644 --- a/limitador/src/storage/in_memory.rs +++ b/limitador/src/storage/in_memory.rs @@ -212,7 +212,9 @@ impl InMemoryStorage { if limit.namespace() == namespace { res.insert( // todo fixme - Counter::new(limit.clone(), HashMap::default()).unwrap(), + Counter::new(limit.clone(), HashMap::default()) + .unwrap() + .unwrap(), counter.clone(), ); } @@ -271,12 +273,14 @@ mod tests { limit_1, HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("Should have a counter"); let counter_2 = Counter::new( limit_2, HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("Should have a counter"); storage.update_counter(&counter_1, 1).unwrap(); storage.update_counter(&counter_2, 1).unwrap(); diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 9d4e1d83..58506328 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -157,7 +157,8 @@ mod tests { limit.clone(), HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("must have a counter"); let raw = key_for_counter(&counter); assert_eq!(counter, partial_counter_from_counter_key(&raw)); } @@ -176,7 +177,8 @@ mod tests { limit.clone(), HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("must have a counter"); let mut other = counter.clone(); other.set_remaining(123); other.set_expires_in(Duration::from_millis(456)); @@ -387,7 +389,9 @@ pub mod bin { vars.insert("role".to_string(), "admin".to_string()); vars.insert("app_id".to_string(), "123".to_string()); vars.insert("wat".to_string(), "dunno".to_string()); - let counter = Counter::new(limit.clone(), vars).expect("counter creation failed!"); + let counter = Counter::new(limit.clone(), vars) + .expect("counter creation failed!") + .expect("must have a counter"); let raw = key_for_counter(&counter); let key_back: CounterKey = @@ -408,7 +412,9 @@ pub mod bin { ); let mut variables = HashMap::default(); variables.insert("app_id".to_string(), "123".to_string()); - let counter = Counter::new(limit.clone(), variables).expect("counter creation failed!"); + let counter = Counter::new(limit.clone(), variables) + .expect("counter creation failed!") + .expect("must have a counter"); let raw = key_for_counter(&counter); assert_eq!(counter, partial_counter_from_counter_key(&raw)); } @@ -427,7 +433,8 @@ pub mod bin { limit, HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("must have a counter"); let serialized_counter = key_for_counter(&counter); let prefix = prefix_for_namespace(namespace); @@ -457,14 +464,16 @@ pub mod bin { limit_with_id, HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("must have a counter"); let serialized_with_id_counter = key_for_counter(&counter_with_id); let counter_without_id = Counter::new( limit_without_id, HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("must have a counter"); let serialized_without_id_counter = key_for_counter(&counter_without_id); // the original key_for_counter continues to encode kinda big diff --git a/limitador/src/storage/redis/counters_cache.rs b/limitador/src/storage/redis/counters_cache.rs index 2645996d..03cdc900 100644 --- a/limitador/src/storage/redis/counters_cache.rs +++ b/limitador/src/storage/redis/counters_cache.rs @@ -687,5 +687,6 @@ mod tests { values, ) .expect("failed creating counter") + .expect("Should have a counter") } } diff --git a/limitador/src/storage/redis/redis_cached.rs b/limitador/src/storage/redis/redis_cached.rs index b9b12ca7..70aaac75 100644 --- a/limitador/src/storage/redis/redis_cached.rs +++ b/limitador/src/storage/redis/redis_cached.rs @@ -449,7 +449,8 @@ mod tests { ), HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("must have a counter"); let arc = Arc::new(CachedCounterValue::from_authority( &counter, @@ -512,7 +513,8 @@ mod tests { ), HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("must have a counter"); let mock_response = Value::Array(vec![ Value::Int(8), @@ -572,7 +574,8 @@ mod tests { ), HashMap::from([("app_id".to_string(), "foo".to_string())]), ) - .expect("counter creation failed!"); + .expect("counter creation failed!") + .expect("must have a counter"); let error: RedisError = io::Error::new(io::ErrorKind::TimedOut, "That was long!").into(); assert!(error.is_timeout()); From 345be88c1b0be30d24d6d11711fdfff56da5aefd Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 3 Dec 2024 05:53:01 -0500 Subject: [PATCH 17/20] Support for list values as binding to cel::Context Signed-off-by: Alex Snaps --- limitador/src/limit/cel.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index 4674692a..41421b20 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -96,6 +96,19 @@ impl<'a> Context<'a> { } } + pub fn list_binding(&mut self, name: String, value: Vec>) { + let v = value + .iter() + .map(|values| { + let map = cel_interpreter::objects::Map::from(values.clone()); + Value::Map(map) + }) + .collect::>(); + self.variables.insert(name.clone()); + self.ctx + .add_variable_from_value(name, Value::List(v.into())); + } + pub(crate) fn for_limit<'b>(&'b self, limit: &Limit) -> Self where 'b: 'a, @@ -412,6 +425,21 @@ mod tests { ); } + #[test] + fn supports_list_bindings() { + let pred = Predicate::parse("root[0].key == '1' && root[1]['key'] == '2'") + .expect("failed to parse"); + let mut ctx = Context::default(); + ctx.list_binding( + "root".to_string(), + vec![ + HashMap::from([("key".to_string(), "1".to_string())]), + HashMap::from([("key".to_string(), "2".to_string())]), + ], + ); + assert_eq!(pred.test(&ctx).map_err(|e| format!("{e}")), Ok(true)); + } + fn ctx<'a>() -> Context<'a> { Context { variables: HashSet::default(), From e0ae4f6b02a0b014ea099913fd47f6a687852be8 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 3 Dec 2024 06:31:18 -0500 Subject: [PATCH 18/20] Expose Context all the way up Signed-off-by: Alex Snaps --- limitador-server/src/envoy_rls/server.rs | 6 +- limitador-server/src/http_api/server.rs | 20 ++-- limitador/benches/bench.rs | 12 +-- limitador/src/counter.rs | 20 ++-- limitador/src/lib.rs | 60 ++++++------ limitador/src/limit.rs | 13 ++- limitador/src/storage/disk/rocksdb_storage.rs | 11 +-- limitador/src/storage/distributed/mod.rs | 23 +++-- limitador/src/storage/in_memory.rs | 24 ++--- limitador/src/storage/keys.rs | 59 ++++++------ limitador/src/storage/redis/counters_cache.rs | 3 +- limitador/src/storage/redis/redis_cached.rs | 12 ++- limitador/tests/helpers/tests_limiter.rs | 33 +++---- limitador/tests/integration_tests.rs | 92 +++++++++++-------- 14 files changed, 192 insertions(+), 196 deletions(-) diff --git a/limitador-server/src/envoy_rls/server.rs b/limitador-server/src/envoy_rls/server.rs index 8f252ce5..d9925a68 100644 --- a/limitador-server/src/envoy_rls/server.rs +++ b/limitador-server/src/envoy_rls/server.rs @@ -109,10 +109,12 @@ impl RateLimitService for MyRateLimiter { req.hits_addend }; + let ctx = (&values).into(); + let rate_limited_resp = match &*self.limiter { Limiter::Blocking(limiter) => limiter.check_rate_limited_and_update( &namespace, - &values, + &ctx, u64::from(hits_addend), self.rate_limit_headers != RateLimitHeaders::None, ), @@ -120,7 +122,7 @@ impl RateLimitService for MyRateLimiter { limiter .check_rate_limited_and_update( &namespace, - &values, + &ctx, u64::from(hits_addend), self.rate_limit_headers != RateLimitHeaders::None, ) diff --git a/limitador-server/src/http_api/server.rs b/limitador-server/src/http_api/server.rs index d436e2a0..ddbd0d64 100644 --- a/limitador-server/src/http_api/server.rs +++ b/limitador-server/src/http_api/server.rs @@ -122,9 +122,10 @@ async fn check( response_headers: _, } = request.into_inner(); let namespace = namespace.into(); + let ctx = (&values).into(); let is_rate_limited_result = match state.get_ref().limiter() { - Limiter::Blocking(limiter) => limiter.is_rate_limited(&namespace, &values, delta), - Limiter::Async(limiter) => limiter.is_rate_limited(&namespace, &values, delta).await, + Limiter::Blocking(limiter) => limiter.is_rate_limited(&namespace, &ctx, delta), + Limiter::Async(limiter) => limiter.is_rate_limited(&namespace, &ctx, delta).await, }; match is_rate_limited_result { @@ -152,9 +153,10 @@ async fn report( response_headers: _, } = request.into_inner(); let namespace = namespace.into(); + let ctx = (&values).into(); let update_counters_result = match data.get_ref().limiter() { - Limiter::Blocking(limiter) => limiter.update_counters(&namespace, &values, delta), - Limiter::Async(limiter) => limiter.update_counters(&namespace, &values, delta).await, + Limiter::Blocking(limiter) => limiter.update_counters(&namespace, &ctx, delta), + Limiter::Async(limiter) => limiter.update_counters(&namespace, &ctx, delta).await, }; match update_counters_result { @@ -176,22 +178,18 @@ async fn check_and_report( response_headers, } = request.into_inner(); let namespace = namespace.into(); + let ctx = (&values).into(); let rate_limit_data = data.get_ref(); let rate_limited_and_update_result = match rate_limit_data.limiter() { Limiter::Blocking(limiter) => limiter.check_rate_limited_and_update( &namespace, - &values, + &ctx, delta, response_headers.is_some(), ), Limiter::Async(limiter) => { limiter - .check_rate_limited_and_update( - &namespace, - &values, - delta, - response_headers.is_some(), - ) + .check_rate_limited_and_update(&namespace, &ctx, delta, response_headers.is_some()) .await } }; diff --git a/limitador/benches/bench.rs b/limitador/benches/bench.rs index b6c0cf70..68a360b9 100644 --- a/limitador/benches/bench.rs +++ b/limitador/benches/bench.rs @@ -329,7 +329,7 @@ fn bench_is_rate_limited( rate_limiter .is_rate_limited( ¶ms.namespace.to_owned().into(), - ¶ms.values, + &(¶ms.values).into(), params.delta, ) .unwrap(), @@ -357,7 +357,7 @@ fn async_bench_is_rate_limited( rate_limiter .is_rate_limited( ¶ms.namespace.to_owned().into(), - ¶ms.values, + &(¶ms.values).into(), params.delta, ) .await @@ -383,7 +383,7 @@ fn bench_update_counters( rate_limiter .update_counters( ¶ms.namespace.to_owned().into(), - ¶ms.values, + &(¶ms.values).into(), params.delta, ) .unwrap(); @@ -410,7 +410,7 @@ fn async_bench_update_counters( rate_limiter .update_counters( ¶ms.namespace.to_owned().into(), - ¶ms.values, + &(¶ms.values).into(), params.delta, ) .await @@ -437,7 +437,7 @@ fn bench_check_rate_limited_and_update( rate_limiter .check_rate_limited_and_update( ¶ms.namespace.to_owned().into(), - ¶ms.values, + &(¶ms.values).into(), params.delta, false, ) @@ -467,7 +467,7 @@ fn async_bench_check_rate_limited_and_update( rate_limiter .check_rate_limited_and_update( ¶ms.namespace.to_owned().into(), - ¶ms.values, + &(¶ms.values).into(), params.delta, false, ) diff --git a/limitador/src/counter.rs b/limitador/src/counter.rs index 788bf8a7..68453a8e 100644 --- a/limitador/src/counter.rs +++ b/limitador/src/counter.rs @@ -1,4 +1,4 @@ -use crate::limit::{Limit, Namespace}; +use crate::limit::{Context, Limit, Namespace}; use crate::LimitadorResult; use serde::{Deserialize, Serialize}; use std::collections::{BTreeMap, HashMap}; @@ -17,15 +17,9 @@ pub struct Counter { } impl Counter { - pub fn new>>( - limit: L, - set_variables: HashMap, - ) -> LimitadorResult> { + pub fn new>>(limit: L, ctx: &Context) -> LimitadorResult> { let limit = limit.into(); - let mut vars = set_variables; - vars.retain(|var, _| limit.has_variable(var)); - - let variables = limit.resolve_variables(vars)?; + let variables = limit.resolve_variables(ctx)?; match variables { None => Ok(None), Some(variables) => Ok(Some(Self { @@ -159,11 +153,9 @@ mod tests { Vec::default(), [var.try_into().expect("failed parsing!")], ); - let counter = Counter::new( - limit, - HashMap::from([("ts".to_string(), "2019-10-12T13:20:50.52Z".to_string())]), - ) - .expect("failed creating counter"); + let map = HashMap::from([("ts".to_string(), "2019-10-12T13:20:50.52Z".to_string())]); + let ctx = (&map).into(); + let counter = Counter::new(limit, &ctx).expect("failed creating counter"); assert_eq!( counter.unwrap().set_variables.get(var), Some("13".to_string()).as_ref() diff --git a/limitador/src/lib.rs b/limitador/src/lib.rs index d5016a18..6902028c 100644 --- a/limitador/src/lib.rs +++ b/limitador/src/lib.rs @@ -94,7 +94,7 @@ //! //! ``` //! use limitador::RateLimiter; -//! use limitador::limit::Limit; +//! use limitador::limit::{Limit, Context}; //! use std::collections::HashMap; //! //! let mut rate_limiter = RateLimiter::new(1000); @@ -116,22 +116,23 @@ //! //! // Check if we can report //! let namespace = "my_namespace".into(); -//! assert!(!rate_limiter.is_rate_limited(&namespace, &values_to_report, 1).unwrap()); +//! let ctx = &values_to_report.into(); +//! assert!(!rate_limiter.is_rate_limited(&namespace, &ctx, 1).unwrap()); //! //! // Report -//! rate_limiter.update_counters(&namespace, &values_to_report, 1).unwrap(); +//! rate_limiter.update_counters(&namespace, &ctx, 1).unwrap(); //! //! // Check and report again -//! assert!(!rate_limiter.is_rate_limited(&namespace, &values_to_report, 1).unwrap()); -//! rate_limiter.update_counters(&namespace, &values_to_report, 1).unwrap(); +//! assert!(!rate_limiter.is_rate_limited(&namespace, &ctx, 1).unwrap()); +//! rate_limiter.update_counters(&namespace, &ctx, 1).unwrap(); //! //! // We've already reported 2, so reporting another one should not be allowed -//! assert!(rate_limiter.is_rate_limited(&namespace, &values_to_report, 1).unwrap()); +//! assert!(rate_limiter.is_rate_limited(&namespace, &ctx, 1).unwrap()); //! //! // You can also check and report if not limited in a single call. It's useful //! // for example, when calling Limitador from a proxy. Instead of doing 2 //! // separate calls, we can issue just one: -//! rate_limiter.check_rate_limited_and_update(&namespace, &values_to_report, 1, false).unwrap(); +//! rate_limiter.check_rate_limited_and_update(&namespace, &ctx, 1, false).unwrap(); //! ``` //! //! # Async @@ -194,7 +195,7 @@ use crate::counter::Counter; use crate::errors::LimitadorError; -use crate::limit::{Limit, Namespace}; +use crate::limit::{Context, Limit, Namespace}; use crate::storage::in_memory::InMemoryStorage; use crate::storage::{AsyncCounterStorage, AsyncStorage, Authorization, CounterStorage, Storage}; use std::collections::{HashMap, HashSet}; @@ -358,7 +359,7 @@ impl RateLimiter { pub fn is_rate_limited( &self, namespace: &Namespace, - values: &HashMap, + values: &Context, delta: u64, ) -> LimitadorResult { let counters = self.counters_that_apply(namespace, values)?; @@ -380,10 +381,10 @@ impl RateLimiter { pub fn update_counters( &self, namespace: &Namespace, - values: &HashMap, + ctx: &Context, delta: u64, ) -> LimitadorResult<()> { - let counters = self.counters_that_apply(namespace, values)?; + let counters = self.counters_that_apply(namespace, ctx)?; counters .iter() @@ -394,11 +395,11 @@ impl RateLimiter { pub fn check_rate_limited_and_update( &self, namespace: &Namespace, - values: &HashMap, + ctx: &Context, delta: u64, load_counters: bool, ) -> LimitadorResult { - let mut counters = self.counters_that_apply(namespace, values)?; + let mut counters = self.counters_that_apply(namespace, ctx)?; if counters.is_empty() { return Ok(CheckResult { @@ -476,14 +477,13 @@ impl RateLimiter { fn counters_that_apply( &self, namespace: &Namespace, - values: &HashMap, + ctx: &Context, ) -> LimitadorResult> { let limits = self.storage.get_limits(namespace); - let ctx = values.into(); limits .iter() - .filter(|lim| lim.applies(&ctx)) - .filter_map(|lim| match Counter::new(Arc::clone(lim), values.clone()) { + .filter(|lim| lim.applies(ctx)) + .filter_map(|lim| match Counter::new(Arc::clone(lim), ctx) { Ok(None) => None, Ok(Some(c)) => Some(Ok(c)), Err(e) => Some(Err(e)), @@ -533,10 +533,10 @@ impl AsyncRateLimiter { pub async fn is_rate_limited( &self, namespace: &Namespace, - values: &HashMap, + ctx: &Context<'_>, delta: u64, ) -> LimitadorResult { - let counters = self.counters_that_apply(namespace, values).await?; + let counters = self.counters_that_apply(namespace, ctx).await?; for counter in counters { match self.storage.is_within_limits(&counter, delta).await { @@ -554,10 +554,10 @@ impl AsyncRateLimiter { pub async fn update_counters( &self, namespace: &Namespace, - values: &HashMap, + ctx: &Context<'_>, delta: u64, ) -> LimitadorResult<()> { - let counters = self.counters_that_apply(namespace, values).await?; + let counters = self.counters_that_apply(namespace, ctx).await?; for counter in counters { self.storage.update_counter(&counter, delta).await? @@ -569,12 +569,12 @@ impl AsyncRateLimiter { pub async fn check_rate_limited_and_update( &self, namespace: &Namespace, - values: &HashMap, + ctx: &Context<'_>, delta: u64, load_counters: bool, ) -> LimitadorResult { // the above where-clause is needed in order to call unwrap(). - let mut counters = self.counters_that_apply(namespace, values).await?; + let mut counters = self.counters_that_apply(namespace, ctx).await?; if counters.is_empty() { return Ok(CheckResult { @@ -657,14 +657,13 @@ impl AsyncRateLimiter { async fn counters_that_apply( &self, namespace: &Namespace, - values: &HashMap, + ctx: &Context<'_>, ) -> LimitadorResult> { let limits = self.storage.get_limits(namespace); - let ctx = values.into(); limits .iter() - .filter(|lim| lim.applies(&ctx)) - .filter_map(|lim| match Counter::new(Arc::clone(lim), values.clone()) { + .filter(|lim| lim.applies(ctx)) + .filter_map(|lim| match Counter::new(Arc::clone(lim), ctx) { Ok(None) => None, Ok(Some(c)) => Some(Ok(c)), Err(e) => Some(Err(e)), @@ -696,9 +695,8 @@ fn classify_limits_by_namespace( #[cfg(test)] mod test { - use crate::limit::{Expression, Limit}; + use crate::limit::{Context, Expression, Limit}; use crate::RateLimiter; - use std::collections::HashMap; #[test] fn properly_updates_existing_limits() { @@ -713,7 +711,7 @@ mod test { assert_eq!(limits.iter().next().unwrap().max_value(), 42); let r = rl - .check_rate_limited_and_update(&namespace.into(), &HashMap::default(), 1, true) + .check_rate_limited_and_update(&namespace.into(), &Context::default(), 1, true) .unwrap(); assert_eq!(r.counters.first().unwrap().max_value(), 42); @@ -727,7 +725,7 @@ mod test { assert_eq!(limits.iter().next().unwrap().max_value(), 50); let r = rl - .check_rate_limited_and_update(&namespace.into(), &HashMap::default(), 1, true) + .check_rate_limited_and_update(&namespace.into(), &Context::default(), 1, true) .unwrap(); assert_eq!(r.counters.first().unwrap().max_value(), 50); } diff --git a/limitador/src/limit.rs b/limitador/src/limit.rs index 6653cb07..d6102234 100644 --- a/limitador/src/limit.rs +++ b/limitador/src/limit.rs @@ -1,14 +1,13 @@ -use cel::Context; use serde::{Deserialize, Serialize}; use std::cmp::Ordering; -use std::collections::{BTreeMap, BTreeSet, HashMap, HashSet}; +use std::collections::{BTreeMap, BTreeSet, HashSet}; use std::fmt::Debug; use std::hash::{Hash, Hasher}; mod cel; +pub use cel::{Context, Expression, Predicate}; pub use cel::{EvaluationError, ParseError}; -pub use cel::{Expression, Predicate}; #[derive(Debug, Hash, Eq, PartialEq, Clone, PartialOrd, Ord, Serialize, Deserialize)] pub struct Namespace(String); @@ -133,13 +132,12 @@ impl Limit { pub fn resolve_variables( &self, - vars: HashMap, + ctx: &Context, ) -> Result>, EvaluationError> { - let ctx = Context::new(String::default(), &vars); let mut map = BTreeMap::new(); for variable in &self.variables { let name = variable.source().into(); - match variable.eval(&ctx)? { + match variable.eval(ctx)? { None => return Ok(None), Some(value) => { map.insert(name, value); @@ -230,6 +228,7 @@ mod tests { use super::*; use crate::counter::Counter; use std::cmp::Ordering::Equal; + use std::collections::HashMap; #[test] fn limit_can_have_an_optional_name() { @@ -466,7 +465,7 @@ mod tests { let ctx = Context::new(String::default(), &map); assert!(limit.applies(&ctx)); assert_eq!( - Counter::new(limit, map) + Counter::new(limit, &ctx) .expect("failed") .unwrap() .set_variables() diff --git a/limitador/src/storage/disk/rocksdb_storage.rs b/limitador/src/storage/disk/rocksdb_storage.rs index 356aaaaf..0d758c27 100644 --- a/limitador/src/storage/disk/rocksdb_storage.rs +++ b/limitador/src/storage/disk/rocksdb_storage.rs @@ -249,12 +249,11 @@ mod tests { vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ); - let counter = Counter::new( - limit, - HashMap::from([("app_id".to_string(), "foo".to_string())]), - ) - .unwrap() - .expect("must have a counter"); + let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); + let ctx = (&map).into(); + let counter = Counter::new(limit, &ctx) + .unwrap() + .expect("must have a counter"); let tmp = TempDir::new().expect("We should have a dir!"); { diff --git a/limitador/src/storage/distributed/mod.rs b/limitador/src/storage/distributed/mod.rs index aee13fa2..1c8f23c7 100644 --- a/limitador/src/storage/distributed/mod.rs +++ b/limitador/src/storage/distributed/mod.rs @@ -9,7 +9,7 @@ use tokio::sync::mpsc::Sender; use tracing::debug; use crate::counter::Counter; -use crate::limit::Limit; +use crate::limit::{Context, Limit}; use crate::storage::distributed::cr_counter_value::CrCounterValue; use crate::storage::distributed::grpc::v1::CounterUpdate; use crate::storage::distributed::grpc::{Broker, CounterEntry}; @@ -47,7 +47,7 @@ impl CounterStorage for CrInMemoryStorage { let key = encode_limit_to_key(limit); limits.entry(key.clone()).or_insert(Arc::new(CounterEntry { key, - counter: Counter::new(limit.clone(), HashMap::default()) + counter: Counter::new(limit.clone(), &Context::default()) .expect("counter creation can't fail! no vars to resolve!") .expect("must have a counter"), value: CrCounterValue::new( @@ -336,15 +336,14 @@ fn encode_counter_to_key(counter: &Counter) -> Vec { fn encode_limit_to_key(limit: &Limit) -> Vec { // fixme this is broken! - let counter = Counter::new( - limit.clone(), - limit - .variables() - .into_iter() - .map(|k| (k, "".to_string())) - .collect(), - ) - .expect("counter creation can't fail! faked vars!") - .expect("must have a counter"); + let vars: HashMap = limit + .variables() + .into_iter() + .map(|k| (k, "".to_string())) + .collect(); + let ctx = (&vars).into(); + let counter = Counter::new(limit.clone(), &ctx) + .expect("counter creation can't fail! faked vars!") + .expect("must have a counter"); key_for_counter_v2(&counter) } diff --git a/limitador/src/storage/in_memory.rs b/limitador/src/storage/in_memory.rs index 3b273d9c..6d741f65 100644 --- a/limitador/src/storage/in_memory.rs +++ b/limitador/src/storage/in_memory.rs @@ -1,5 +1,5 @@ use crate::counter::Counter; -use crate::limit::{Limit, Namespace}; +use crate::limit::{Context, Limit, Namespace}; use crate::storage::atomic_expiring_value::AtomicExpiringValue; use crate::storage::{Authorization, CounterStorage, StorageErr}; use moka::sync::Cache; @@ -212,7 +212,7 @@ impl InMemoryStorage { if limit.namespace() == namespace { res.insert( // todo fixme - Counter::new(limit.clone(), HashMap::default()) + Counter::new(limit.clone(), &Context::default()) .unwrap() .unwrap(), counter.clone(), @@ -269,18 +269,14 @@ mod tests { vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ); - let counter_1 = Counter::new( - limit_1, - HashMap::from([("app_id".to_string(), "foo".to_string())]), - ) - .expect("counter creation failed!") - .expect("Should have a counter"); - let counter_2 = Counter::new( - limit_2, - HashMap::from([("app_id".to_string(), "foo".to_string())]), - ) - .expect("counter creation failed!") - .expect("Should have a counter"); + let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); + let ctx = (&map).into(); + let counter_1 = Counter::new(limit_1, &ctx) + .expect("counter creation failed!") + .expect("Should have a counter"); + let counter_2 = Counter::new(limit_2, &ctx) + .expect("counter creation failed!") + .expect("Should have a counter"); storage.update_counter(&counter_1, 1).unwrap(); storage.update_counter(&counter_2, 1).unwrap(); diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 58506328..247eb2f6 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -153,12 +153,11 @@ mod tests { vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ); - let counter = Counter::new( - limit.clone(), - HashMap::from([("app_id".to_string(), "foo".to_string())]), - ) - .expect("counter creation failed!") - .expect("must have a counter"); + let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); + let ctx = (&map).into(); + let counter = Counter::new(limit.clone(), &ctx) + .expect("counter creation failed!") + .expect("must have a counter"); let raw = key_for_counter(&counter); assert_eq!(counter, partial_counter_from_counter_key(&raw)); } @@ -173,12 +172,11 @@ mod tests { vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ); - let counter = Counter::new( - limit.clone(), - HashMap::from([("app_id".to_string(), "foo".to_string())]), - ) - .expect("counter creation failed!") - .expect("must have a counter"); + let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); + let ctx = (&map).into(); + let counter = Counter::new(limit.clone(), &ctx) + .expect("counter creation failed!") + .expect("must have a counter"); let mut other = counter.clone(); other.set_remaining(123); other.set_expires_in(Duration::from_millis(456)); @@ -389,7 +387,8 @@ pub mod bin { vars.insert("role".to_string(), "admin".to_string()); vars.insert("app_id".to_string(), "123".to_string()); vars.insert("wat".to_string(), "dunno".to_string()); - let counter = Counter::new(limit.clone(), vars) + let ctx = (&vars).into(); + let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -412,7 +411,8 @@ pub mod bin { ); let mut variables = HashMap::default(); variables.insert("app_id".to_string(), "123".to_string()); - let counter = Counter::new(limit.clone(), variables) + let ctx = (&variables).into(); + let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); let raw = key_for_counter(&counter); @@ -429,12 +429,11 @@ pub mod bin { vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ); - let counter = Counter::new( - limit, - HashMap::from([("app_id".to_string(), "foo".to_string())]), - ) - .expect("counter creation failed!") - .expect("must have a counter"); + let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); + let ctx = (&map).into(); + let counter = Counter::new(limit, &ctx) + .expect("counter creation failed!") + .expect("must have a counter"); let serialized_counter = key_for_counter(&counter); let prefix = prefix_for_namespace(namespace); @@ -460,20 +459,16 @@ pub mod bin { vec!["app_id".try_into().expect("failed parsing!")], ); - let counter_with_id = Counter::new( - limit_with_id, - HashMap::from([("app_id".to_string(), "foo".to_string())]), - ) - .expect("counter creation failed!") - .expect("must have a counter"); + let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); + let ctx = (&map).into(); + let counter_with_id = Counter::new(limit_with_id, &ctx) + .expect("counter creation failed!") + .expect("must have a counter"); let serialized_with_id_counter = key_for_counter(&counter_with_id); - let counter_without_id = Counter::new( - limit_without_id, - HashMap::from([("app_id".to_string(), "foo".to_string())]), - ) - .expect("counter creation failed!") - .expect("must have a counter"); + let counter_without_id = Counter::new(limit_without_id, &ctx) + .expect("counter creation failed!") + .expect("must have a counter"); let serialized_without_id_counter = key_for_counter(&counter_without_id); // the original key_for_counter continues to encode kinda big diff --git a/limitador/src/storage/redis/counters_cache.rs b/limitador/src/storage/redis/counters_cache.rs index 03cdc900..fb05300e 100644 --- a/limitador/src/storage/redis/counters_cache.rs +++ b/limitador/src/storage/redis/counters_cache.rs @@ -676,6 +676,7 @@ mod tests { if let Some(overrides) = other_values { values.extend(overrides); } + let ctx = (&values).into(); Counter::new( Limit::new( "test_namespace", @@ -684,7 +685,7 @@ mod tests { vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ), - values, + &ctx, ) .expect("failed creating counter") .expect("Should have a counter") diff --git a/limitador/src/storage/redis/redis_cached.rs b/limitador/src/storage/redis/redis_cached.rs index 70aaac75..0cc0e99e 100644 --- a/limitador/src/storage/redis/redis_cached.rs +++ b/limitador/src/storage/redis/redis_cached.rs @@ -439,6 +439,8 @@ mod tests { const LOCAL_INCREMENTS: u64 = 2; let mut counters_and_deltas = HashMap::new(); + let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); + let ctx = (&map).into(); let counter = Counter::new( Limit::new( "test_namespace", @@ -447,7 +449,7 @@ mod tests { vec!["req_method == 'GET'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ), - HashMap::from([("app_id".to_string(), "foo".to_string())]), + &ctx, ) .expect("counter creation failed!") .expect("must have a counter"); @@ -503,6 +505,8 @@ mod tests { #[tokio::test] async fn flush_batcher_and_update_counters_test() { + let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); + let ctx = (&map).into(); let counter = Counter::new( Limit::new( "test_namespace", @@ -511,7 +515,7 @@ mod tests { vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ), - HashMap::from([("app_id".to_string(), "foo".to_string())]), + &ctx, ) .expect("counter creation failed!") .expect("must have a counter"); @@ -564,6 +568,8 @@ mod tests { #[tokio::test] async fn flush_batcher_reverts_on_err() { + let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); + let ctx = (&map).into(); let counter = Counter::new( Limit::new( "test_namespace", @@ -572,7 +578,7 @@ mod tests { vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ), - HashMap::from([("app_id".to_string(), "foo".to_string())]), + &ctx, ) .expect("counter creation failed!") .expect("must have a counter"); diff --git a/limitador/tests/helpers/tests_limiter.rs b/limitador/tests/helpers/tests_limiter.rs index 2bae0c3e..89e631b1 100644 --- a/limitador/tests/helpers/tests_limiter.rs +++ b/limitador/tests/helpers/tests_limiter.rs @@ -1,8 +1,8 @@ use limitador::counter::Counter; use limitador::errors::LimitadorError; -use limitador::limit::{Limit, Namespace}; +use limitador::limit::{Context, Limit, Namespace}; use limitador::{AsyncRateLimiter, CheckResult, RateLimiter}; -use std::collections::{HashMap, HashSet}; +use std::collections::HashSet; // This exposes a struct that wraps both implementations of the rate limiter, // the blocking and the async one. This allows us to avoid duplications in the @@ -70,17 +70,15 @@ impl TestsLimiter { pub async fn is_rate_limited( &self, namespace: &str, - values: &HashMap, + ctx: &Context<'_>, delta: u64, ) -> Result { match &self.limiter_impl { LimiterImpl::Blocking(limiter) => { - limiter.is_rate_limited(&namespace.into(), values, delta) + limiter.is_rate_limited(&namespace.into(), ctx, delta) } LimiterImpl::Async(limiter) => { - limiter - .is_rate_limited(&namespace.into(), values, delta) - .await + limiter.is_rate_limited(&namespace.into(), ctx, delta).await } } } @@ -88,17 +86,15 @@ impl TestsLimiter { pub async fn update_counters( &self, namespace: &str, - values: &HashMap, + ctx: &Context<'_>, delta: u64, ) -> Result<(), LimitadorError> { match &self.limiter_impl { LimiterImpl::Blocking(limiter) => { - limiter.update_counters(&namespace.into(), values, delta) + limiter.update_counters(&namespace.into(), ctx, delta) } LimiterImpl::Async(limiter) => { - limiter - .update_counters(&namespace.into(), values, delta) - .await + limiter.update_counters(&namespace.into(), ctx, delta).await } } } @@ -106,20 +102,17 @@ impl TestsLimiter { pub async fn check_rate_limited_and_update( &self, namespace: &str, - values: &HashMap, + ctx: &Context<'_>, delta: u64, load_counters: bool, ) -> Result { match &self.limiter_impl { - LimiterImpl::Blocking(limiter) => limiter.check_rate_limited_and_update( - &namespace.into(), - values, - delta, - load_counters, - ), + LimiterImpl::Blocking(limiter) => { + limiter.check_rate_limited_and_update(&namespace.into(), ctx, delta, load_counters) + } LimiterImpl::Async(limiter) => { limiter - .check_rate_limited_and_update(&namespace.into(), values, delta, load_counters) + .check_rate_limited_and_update(&namespace.into(), ctx, delta, load_counters) .await } } diff --git a/limitador/tests/integration_tests.rs b/limitador/tests/integration_tests.rs index 7a71372c..4f8f2d4a 100644 --- a/limitador/tests/integration_tests.rs +++ b/limitador/tests/integration_tests.rs @@ -380,7 +380,7 @@ mod test { values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter - .update_counters(namespace, &values, 1) + .update_counters(namespace, &(&values).into(), 1) .await .unwrap(); @@ -473,7 +473,7 @@ mod test { values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter - .update_counters(namespace, &values, 1) + .update_counters(namespace, &(&values).into(), 1) .await .unwrap(); @@ -506,22 +506,23 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); for i in 0..max_hits { assert!( !rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap(), "Must not be limited after {i}" ); rate_limiter - .update_counters(namespace, &values, 1) + .update_counters(namespace, &ctx, 1) .await .unwrap(); } assert!(rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap()); } @@ -543,22 +544,23 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); for i in 0..max_hits { assert!( !rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap(), "Must not be limited after {i}" ); rate_limiter - .update_counters(namespace, &values, 1) + .update_counters(namespace, &ctx, 1) .await .unwrap(); } assert!(rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap()); } @@ -590,32 +592,34 @@ mod test { let mut get_values: HashMap = HashMap::new(); get_values.insert("req_method".to_string(), "GET".to_string()); get_values.insert("app_id".to_string(), "test_app_id".to_string()); + let get_ctx = (&get_values).into(); let mut post_values: HashMap = HashMap::new(); post_values.insert("req_method".to_string(), "POST".to_string()); post_values.insert("app_id".to_string(), "test_app_id".to_string()); + let post_ctx = (&post_values).into(); for i in 0..max_hits { assert!( !rate_limiter - .is_rate_limited(namespace, &get_values, 1) + .is_rate_limited(namespace, &get_ctx, 1) .await .unwrap(), "Must not be limited after {i}" ); assert!( !rate_limiter - .is_rate_limited(namespace, &post_values, 1) + .is_rate_limited(namespace, &post_ctx, 1) .await .unwrap(), "Must not be limited after {i}" ); rate_limiter - .check_rate_limited_and_update(namespace, &get_values, 1, false) + .check_rate_limited_and_update(namespace, &get_ctx, 1, false) .await .unwrap(); rate_limiter - .check_rate_limited_and_update(namespace, &post_values, 1, false) + .check_rate_limited_and_update(namespace, &post_ctx, 1, false) .await .unwrap(); } @@ -624,11 +628,11 @@ mod test { tokio::time::sleep(Duration::from_millis(40)).await; assert!(rate_limiter - .is_rate_limited(namespace, &get_values, 1) + .is_rate_limited(namespace, &get_ctx, 1) .await .unwrap()); assert!(!rate_limiter - .is_rate_limited(namespace, &post_values, 1) + .is_rate_limited(namespace, &post_ctx, 1) .await .unwrap()); } @@ -648,21 +652,22 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); // Report 5 hits twice. The limit is 10, so the first limited call should be // the third one. for _ in 0..2 { assert!(!rate_limiter - .is_rate_limited(namespace, &values, 5) + .is_rate_limited(namespace, &ctx, 5) .await .unwrap()); rate_limiter - .update_counters(namespace, &values, 5) + .update_counters(namespace, &ctx, 5) .await .unwrap(); } assert!(rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap()); } @@ -683,9 +688,10 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); assert!(rate_limiter - .is_rate_limited(namespace, &values, max + 1) + .is_rate_limited(namespace, &ctx, max + 1) .await .unwrap()) } @@ -706,6 +712,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); for i in 0..max_hits { // Add an extra value that does not apply to the limit on each @@ -714,18 +721,18 @@ mod test { assert!( !rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap(), "Must not be limited after {i}" ); rate_limiter - .update_counters(namespace, &values, 1) + .update_counters(namespace, &ctx, 1) .await .unwrap(); } assert!(rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap()); } @@ -735,9 +742,10 @@ mod test { ) { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); + let ctx = (&values).into(); assert!(!rate_limiter - .is_rate_limited("test_namespace", &values, 1) + .is_rate_limited("test_namespace", &ctx, 1) .await .unwrap()); } @@ -761,9 +769,10 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "POST".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); assert!(!rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap()); } @@ -783,9 +792,10 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); assert!(rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap()); } @@ -807,11 +817,12 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); for _ in 0..max_hits { assert!( !rate_limiter - .check_rate_limited_and_update(namespace, &values, 1, false) + .check_rate_limited_and_update(namespace, &ctx, 1, false) .await .unwrap() .limited @@ -820,7 +831,7 @@ mod test { assert!( rate_limiter - .check_rate_limited_and_update(namespace, &values, 1, false) + .check_rate_limited_and_update(namespace, &ctx, 1, false) .await .unwrap() .limited @@ -844,10 +855,11 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); for hit in 0..max_hits { let result = rate_limiter - .check_rate_limited_and_update(namespace, &values, 1, true) + .check_rate_limited_and_update(namespace, &ctx, 1, true) .await .unwrap(); assert!(!result.limited); @@ -862,7 +874,7 @@ mod test { } let result = rate_limiter - .check_rate_limited_and_update(namespace, &values, 1, true) + .check_rate_limited_and_update(namespace, &ctx, 1, true) .await .unwrap(); assert!(result.limited); @@ -895,10 +907,11 @@ mod test { values.insert("app_id".to_string(), "test_app_id".to_string()); // Does not match the limit defined values.insert("req_method".to_string(), "POST".to_string()); + let ctx = (&values).into(); assert!( !rate_limiter - .check_rate_limited_and_update(namespace, &values, 1, false) + .check_rate_limited_and_update(namespace, &ctx, 1, false) .await .unwrap() .limited @@ -922,10 +935,11 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); assert!( rate_limiter - .check_rate_limited_and_update(namespace, &values, 1, false) + .check_rate_limited_and_update(namespace, &ctx, 1, false) .await .unwrap() .limited @@ -951,14 +965,15 @@ mod test { let mut values = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); + let ctx = (&values).into(); rate_limiter - .update_counters(namespace, &values, hits_app_1) + .update_counters(namespace, &ctx, hits_app_1) .await .unwrap(); values.insert("app_id".to_string(), "2".to_string()); rate_limiter - .update_counters(namespace, &values, hits_app_2) + .update_counters(namespace, &ctx, hits_app_2) .await .unwrap(); @@ -1029,8 +1044,9 @@ mod test { let mut values = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); + let ctx = (&values).into(); rate_limiter - .update_counters(namespace, &values, 1) + .update_counters(namespace, &ctx, 1) .await .unwrap(); @@ -1093,8 +1109,9 @@ mod test { let mut values = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); + let ctx = (&values).into(); rate_limiter - .update_counters(namespace, &values, hits_to_report) + .update_counters(namespace, &ctx, hits_to_report) .await .unwrap(); @@ -1245,19 +1262,20 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); + let ctx = (&values).into(); for i in 0..max_hits { // Alternate between the two rate limiters let rate_limiter = rate_limiters.get((i % 2) as usize).unwrap(); assert!( !rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap(), "Must not be limited after {i}" ); rate_limiter - .update_counters(namespace, &values, 1) + .update_counters(namespace, &ctx, 1) .await .unwrap(); } @@ -1269,7 +1287,7 @@ mod test { || async { let rate_limiter = rate_limiters.first().unwrap(); rate_limiter - .is_rate_limited(namespace, &values, 1) + .is_rate_limited(namespace, &ctx, 1) .await .unwrap() } From 7cc4ff459d7f09e5d210ae707ba3e182233afa10 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 3 Dec 2024 06:48:25 -0500 Subject: [PATCH 19/20] Fixed test and take ownership of vars in ctx Signed-off-by: Alex Snaps --- limitador-server/src/envoy_rls/server.rs | 2 +- limitador-server/src/http_api/server.rs | 6 +-- limitador/benches/bench.rs | 20 ++++----- limitador/src/counter.rs | 2 +- limitador/src/limit.rs | 16 +++---- limitador/src/limit/cel.rs | 19 ++++---- limitador/src/storage/disk/rocksdb_storage.rs | 2 +- limitador/src/storage/distributed/mod.rs | 2 +- limitador/src/storage/in_memory.rs | 2 +- limitador/src/storage/keys.rs | 12 ++--- limitador/src/storage/redis/counters_cache.rs | 3 +- limitador/src/storage/redis/redis_cached.rs | 6 +-- limitador/tests/integration_tests.rs | 45 ++++++++++--------- 13 files changed, 70 insertions(+), 67 deletions(-) diff --git a/limitador-server/src/envoy_rls/server.rs b/limitador-server/src/envoy_rls/server.rs index d9925a68..b6d24e1b 100644 --- a/limitador-server/src/envoy_rls/server.rs +++ b/limitador-server/src/envoy_rls/server.rs @@ -109,7 +109,7 @@ impl RateLimitService for MyRateLimiter { req.hits_addend }; - let ctx = (&values).into(); + let ctx = values.into(); let rate_limited_resp = match &*self.limiter { Limiter::Blocking(limiter) => limiter.check_rate_limited_and_update( diff --git a/limitador-server/src/http_api/server.rs b/limitador-server/src/http_api/server.rs index ddbd0d64..ed413e53 100644 --- a/limitador-server/src/http_api/server.rs +++ b/limitador-server/src/http_api/server.rs @@ -122,7 +122,7 @@ async fn check( response_headers: _, } = request.into_inner(); let namespace = namespace.into(); - let ctx = (&values).into(); + let ctx = values.into(); let is_rate_limited_result = match state.get_ref().limiter() { Limiter::Blocking(limiter) => limiter.is_rate_limited(&namespace, &ctx, delta), Limiter::Async(limiter) => limiter.is_rate_limited(&namespace, &ctx, delta).await, @@ -153,7 +153,7 @@ async fn report( response_headers: _, } = request.into_inner(); let namespace = namespace.into(); - let ctx = (&values).into(); + let ctx = values.into(); let update_counters_result = match data.get_ref().limiter() { Limiter::Blocking(limiter) => limiter.update_counters(&namespace, &ctx, delta), Limiter::Async(limiter) => limiter.update_counters(&namespace, &ctx, delta).await, @@ -178,7 +178,7 @@ async fn check_and_report( response_headers, } = request.into_inner(); let namespace = namespace.into(); - let ctx = (&values).into(); + let ctx = values.into(); let rate_limit_data = data.get_ref(); let rate_limited_and_update_result = match rate_limit_data.limiter() { Limiter::Blocking(limiter) => limiter.check_rate_limited_and_update( diff --git a/limitador/benches/bench.rs b/limitador/benches/bench.rs index 68a360b9..fed3fec8 100644 --- a/limitador/benches/bench.rs +++ b/limitador/benches/bench.rs @@ -7,7 +7,7 @@ use criterion::{black_box, criterion_group, criterion_main, Bencher, BenchmarkId use rand::seq::SliceRandom; use rand::SeedableRng; -use limitador::limit::Limit; +use limitador::limit::{Context, Limit}; #[cfg(feature = "disk_storage")] use limitador::storage::disk::{DiskStorage, OptimizeFor}; #[cfg(feature = "distributed_storage")] @@ -89,9 +89,9 @@ const TEST_SCENARIOS: &[&TestScenario] = &[ }, ]; -struct TestCallParams { +struct TestCallParams<'a> { namespace: String, - values: HashMap, + ctx: Context<'a>, delta: u64, } @@ -329,7 +329,7 @@ fn bench_is_rate_limited( rate_limiter .is_rate_limited( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, ) .unwrap(), @@ -357,7 +357,7 @@ fn async_bench_is_rate_limited( rate_limiter .is_rate_limited( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, ) .await @@ -383,7 +383,7 @@ fn bench_update_counters( rate_limiter .update_counters( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, ) .unwrap(); @@ -410,7 +410,7 @@ fn async_bench_update_counters( rate_limiter .update_counters( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, ) .await @@ -437,7 +437,7 @@ fn bench_check_rate_limited_and_update( rate_limiter .check_rate_limited_and_update( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, false, ) @@ -467,7 +467,7 @@ fn async_bench_check_rate_limited_and_update( rate_limiter .check_rate_limited_and_update( ¶ms.namespace.to_owned().into(), - &(¶ms.values).into(), + ¶ms.ctx, params.delta, false, ) @@ -562,7 +562,7 @@ fn generate_test_limits(scenario: &TestScenario) -> (Vec, Vec = HashMap::new(); values.insert("x".into(), "5".into()); - assert!(!limit.applies(&(&values).into())) + assert!(!limit.applies(&values.into())) } #[test] @@ -333,7 +333,7 @@ mod tests { values.insert("y".into(), "2".into()); values.insert("z".into(), "1".into()); - assert!(limit.applies(&(&values).into())) + assert!(limit.applies(&values.into())) } #[test] @@ -354,7 +354,7 @@ mod tests { values.insert("y".into(), "2".into()); values.insert("z".into(), "1".into()); - assert!(!limit.applies(&(&values).into())) + assert!(!limit.applies(&values.into())) } #[test] @@ -434,7 +434,7 @@ mod tests { ], Vec::default(), ); - assert!(limit.applies(&(&HashMap::default()).into())); + assert!(limit.applies(&Context::default())); let limit = Limit::with_id( "my_id", @@ -462,7 +462,7 @@ mod tests { ("foo".to_string(), "nice bar!".to_string()), ("bar".to_string(), "foo,baz".to_string()), ]); - let ctx = Context::new(String::default(), &map); + let ctx = map.into(); assert!(limit.applies(&ctx)); assert_eq!( Counter::new(limit, &ctx) diff --git a/limitador/src/limit/cel.rs b/limitador/src/limit/cel.rs index 41421b20..d44609d1 100644 --- a/limitador/src/limit/cel.rs +++ b/limitador/src/limit/cel.rs @@ -78,22 +78,21 @@ pub struct Context<'a> { } impl<'a> Context<'a> { - pub(crate) fn new(root: String, values: &HashMap) -> Self { + pub(crate) fn new(root: String, values: HashMap) -> Self { let mut ctx = cel_interpreter::Context::default(); + let mut variables = HashSet::new(); if root.is_empty() { for (binding, value) in values { - ctx.add_variable_from_value(binding, value.clone()) + ctx.add_variable_from_value(binding.clone(), value.clone()); + variables.insert(binding); } } else { let map = cel_interpreter::objects::Map::from(values.clone()); ctx.add_variable_from_value(root, Value::Map(map)); } - Self { - variables: values.keys().cloned().collect(), - ctx, - } + Self { variables, ctx } } pub fn list_binding(&mut self, name: String, value: Vec>) { @@ -146,12 +145,12 @@ impl<'a> Context<'a> { impl Default for Context<'_> { fn default() -> Self { - Self::new(String::default(), &HashMap::default()) + Self::new(String::default(), HashMap::default()) } } -impl From<&HashMap> for Context<'_> { - fn from(value: &HashMap) -> Self { +impl From> for Context<'_> { + fn from(value: HashMap) -> Self { Self::new(String::default(), value) } } @@ -411,7 +410,7 @@ mod tests { fn predicate_no_key() { let pred = Predicate::parse("there.not == 42").expect("failed to parse"); assert_eq!( - pred.test(&(&HashMap::from([("there".to_string(), String::default())])).into()), + pred.test(&HashMap::from([("there".to_string(), String::default())]).into()), Ok(false) ); } diff --git a/limitador/src/storage/disk/rocksdb_storage.rs b/limitador/src/storage/disk/rocksdb_storage.rs index 0d758c27..96496693 100644 --- a/limitador/src/storage/disk/rocksdb_storage.rs +++ b/limitador/src/storage/disk/rocksdb_storage.rs @@ -250,7 +250,7 @@ mod tests { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new(limit, &ctx) .unwrap() .expect("must have a counter"); diff --git a/limitador/src/storage/distributed/mod.rs b/limitador/src/storage/distributed/mod.rs index 1c8f23c7..77f0a3ff 100644 --- a/limitador/src/storage/distributed/mod.rs +++ b/limitador/src/storage/distributed/mod.rs @@ -341,7 +341,7 @@ fn encode_limit_to_key(limit: &Limit) -> Vec { .into_iter() .map(|k| (k, "".to_string())) .collect(); - let ctx = (&vars).into(); + let ctx = vars.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation can't fail! faked vars!") .expect("must have a counter"); diff --git a/limitador/src/storage/in_memory.rs b/limitador/src/storage/in_memory.rs index 6d741f65..28dc2119 100644 --- a/limitador/src/storage/in_memory.rs +++ b/limitador/src/storage/in_memory.rs @@ -270,7 +270,7 @@ mod tests { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter_1 = Counter::new(limit_1, &ctx) .expect("counter creation failed!") .expect("Should have a counter"); diff --git a/limitador/src/storage/keys.rs b/limitador/src/storage/keys.rs index 247eb2f6..10fe5a44 100644 --- a/limitador/src/storage/keys.rs +++ b/limitador/src/storage/keys.rs @@ -154,7 +154,7 @@ mod tests { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -173,7 +173,7 @@ mod tests { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -387,7 +387,7 @@ pub mod bin { vars.insert("role".to_string(), "admin".to_string()); vars.insert("app_id".to_string(), "123".to_string()); vars.insert("wat".to_string(), "dunno".to_string()); - let ctx = (&vars).into(); + let ctx = vars.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -411,7 +411,7 @@ pub mod bin { ); let mut variables = HashMap::default(); variables.insert("app_id".to_string(), "123".to_string()); - let ctx = (&variables).into(); + let ctx = variables.into(); let counter = Counter::new(limit.clone(), &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -430,7 +430,7 @@ pub mod bin { vec!["app_id".try_into().expect("failed parsing!")], ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new(limit, &ctx) .expect("counter creation failed!") .expect("must have a counter"); @@ -460,7 +460,7 @@ pub mod bin { ); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter_with_id = Counter::new(limit_with_id, &ctx) .expect("counter creation failed!") .expect("must have a counter"); diff --git a/limitador/src/storage/redis/counters_cache.rs b/limitador/src/storage/redis/counters_cache.rs index fb05300e..49c83e45 100644 --- a/limitador/src/storage/redis/counters_cache.rs +++ b/limitador/src/storage/redis/counters_cache.rs @@ -676,7 +676,6 @@ mod tests { if let Some(overrides) = other_values { values.extend(overrides); } - let ctx = (&values).into(); Counter::new( Limit::new( "test_namespace", @@ -685,7 +684,7 @@ mod tests { vec!["req_method == 'POST'".try_into().expect("failed parsing!")], vec!["app_id".try_into().expect("failed parsing!")], ), - &ctx, + &values.into(), ) .expect("failed creating counter") .expect("Should have a counter") diff --git a/limitador/src/storage/redis/redis_cached.rs b/limitador/src/storage/redis/redis_cached.rs index 0cc0e99e..eda05161 100644 --- a/limitador/src/storage/redis/redis_cached.rs +++ b/limitador/src/storage/redis/redis_cached.rs @@ -440,7 +440,7 @@ mod tests { let mut counters_and_deltas = HashMap::new(); let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new( Limit::new( "test_namespace", @@ -506,7 +506,7 @@ mod tests { #[tokio::test] async fn flush_batcher_and_update_counters_test() { let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new( Limit::new( "test_namespace", @@ -569,7 +569,7 @@ mod tests { #[tokio::test] async fn flush_batcher_reverts_on_err() { let map = HashMap::from([("app_id".to_string(), "foo".to_string())]); - let ctx = (&map).into(); + let ctx = map.into(); let counter = Counter::new( Limit::new( "test_namespace", diff --git a/limitador/tests/integration_tests.rs b/limitador/tests/integration_tests.rs index 4f8f2d4a..a19eb756 100644 --- a/limitador/tests/integration_tests.rs +++ b/limitador/tests/integration_tests.rs @@ -380,7 +380,7 @@ mod test { values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter - .update_counters(namespace, &(&values).into(), 1) + .update_counters(namespace, &values.into(), 1) .await .unwrap(); @@ -473,7 +473,7 @@ mod test { values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); rate_limiter - .update_counters(namespace, &(&values).into(), 1) + .update_counters(namespace, &values.into(), 1) .await .unwrap(); @@ -506,7 +506,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for i in 0..max_hits { assert!( @@ -544,7 +544,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for i in 0..max_hits { assert!( @@ -592,12 +592,12 @@ mod test { let mut get_values: HashMap = HashMap::new(); get_values.insert("req_method".to_string(), "GET".to_string()); get_values.insert("app_id".to_string(), "test_app_id".to_string()); - let get_ctx = (&get_values).into(); + let get_ctx = get_values.into(); let mut post_values: HashMap = HashMap::new(); post_values.insert("req_method".to_string(), "POST".to_string()); post_values.insert("app_id".to_string(), "test_app_id".to_string()); - let post_ctx = (&post_values).into(); + let post_ctx = post_values.into(); for i in 0..max_hits { assert!( @@ -652,7 +652,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); // Report 5 hits twice. The limit is 10, so the first limited call should be // the third one. @@ -688,7 +688,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!(rate_limiter .is_rate_limited(namespace, &ctx, max + 1) @@ -712,12 +712,13 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); for i in 0..max_hits { // Add an extra value that does not apply to the limit on each // iteration. It should not affect. + let mut values = values.clone(); values.insert("does_not_apply".to_string(), i.to_string()); + let ctx = values.into(); assert!( !rate_limiter @@ -731,6 +732,7 @@ mod test { .await .unwrap(); } + let ctx = values.into(); assert!(rate_limiter .is_rate_limited(namespace, &ctx, 1) .await @@ -742,7 +744,7 @@ mod test { ) { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!(!rate_limiter .is_rate_limited("test_namespace", &ctx, 1) @@ -769,7 +771,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "POST".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!(!rate_limiter .is_rate_limited(namespace, &ctx, 1) @@ -792,7 +794,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!(rate_limiter .is_rate_limited(namespace, &ctx, 1) @@ -817,7 +819,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for _ in 0..max_hits { assert!( @@ -855,7 +857,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for hit in 0..max_hits { let result = rate_limiter @@ -907,7 +909,7 @@ mod test { values.insert("app_id".to_string(), "test_app_id".to_string()); // Does not match the limit defined values.insert("req_method".to_string(), "POST".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!( !rate_limiter @@ -935,7 +937,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); assert!( rate_limiter @@ -965,13 +967,16 @@ mod test { let mut values = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); rate_limiter .update_counters(namespace, &ctx, hits_app_1) .await .unwrap(); + let mut values = HashMap::new(); + values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "2".to_string()); + let ctx = values.into(); rate_limiter .update_counters(namespace, &ctx, hits_app_2) .await @@ -1044,7 +1049,7 @@ mod test { let mut values = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); rate_limiter .update_counters(namespace, &ctx, 1) .await @@ -1109,7 +1114,7 @@ mod test { let mut values = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "1".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); rate_limiter .update_counters(namespace, &ctx, hits_to_report) .await @@ -1262,7 +1267,7 @@ mod test { let mut values: HashMap = HashMap::new(); values.insert("req_method".to_string(), "GET".to_string()); values.insert("app_id".to_string(), "test_app_id".to_string()); - let ctx = (&values).into(); + let ctx = values.into(); for i in 0..max_hits { // Alternate between the two rate limiters From 894189840fd6a74a040931277f7e7d087fd87fda Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Tue, 3 Dec 2024 07:01:53 -0500 Subject: [PATCH 20/20] Wired proper descriptor wiring into context Signed-off-by: Alex Snaps --- limitador-server/examples/envoy.yaml | 4 +- limitador-server/examples/limits.yaml | 2 +- limitador-server/sandbox/README.md | 8 +-- limitador-server/sandbox/envoy.yaml | 4 +- limitador-server/sandbox/envoy2.yaml | 4 +- limitador-server/sandbox/envoy3.yaml | 4 +- limitador-server/sandbox/limits.yaml | 12 ++-- limitador-server/sandbox/load-test.json | 4 +- limitador-server/sandbox/redis-otel/README.md | 2 +- limitador-server/src/envoy_rls/server.rs | 65 ++++++++++++------- limitador-server/src/http_api/server.rs | 30 +++++---- 11 files changed, 82 insertions(+), 57 deletions(-) diff --git a/limitador-server/examples/envoy.yaml b/limitador-server/examples/envoy.yaml index 208d3c76..1a7d0eda 100644 --- a/limitador-server/examples/envoy.yaml +++ b/limitador-server/examples/envoy.yaml @@ -28,8 +28,8 @@ static_resources: rate_limits: - stage: 0 actions: - - {request_headers: {header_name: "userid", descriptor_key: "user_id"}} - - { request_headers: { header_name: ":method", descriptor_key: "req_method" } } + - { request_headers: { header_name: "userid", descriptor_key: "user_id" } } + - { request_headers: { header_name: ":method", descriptor_key: "descriptors[0]['method']" } } http_filters: - name: envoy.filters.http.ratelimit typed_config: diff --git a/limitador-server/examples/limits.yaml b/limitador-server/examples/limits.yaml index be60b900..c47aedf4 100644 --- a/limitador-server/examples/limits.yaml +++ b/limitador-server/examples/limits.yaml @@ -10,6 +10,6 @@ max_value: 5 seconds: 60 conditions: - - "req_method == 'POST'" + - "descriptors[0]['req.method'] == 'POST'" variables: - user_id diff --git a/limitador-server/sandbox/README.md b/limitador-server/sandbox/README.md index ca0cd9b9..76614284 100644 --- a/limitador-server/sandbox/README.md +++ b/limitador-server/sandbox/README.md @@ -99,11 +99,11 @@ bin/grpcurl -plaintext -d @ 127.0.0.1:18081 envoy.service.ratelimit.v3.RateLimit { "entries": [ { - "key": "req_method", + "key": "req.method", "value": "POST" }, { - "key": "req_path", + "key": "req.path", "value": "/" } ] @@ -125,11 +125,11 @@ while :; do bin/grpcurl -plaintext -d @ 127.0.0.1:18081 envoy.service.ratelimit. { "entries": [ { - "key": "req_method", + "key": "req.method", "value": "POST" }, { - "key": "req_path", + "key": "req.path", "value": "/" } ] diff --git a/limitador-server/sandbox/envoy.yaml b/limitador-server/sandbox/envoy.yaml index bf5ee863..7e678e10 100644 --- a/limitador-server/sandbox/envoy.yaml +++ b/limitador-server/sandbox/envoy.yaml @@ -27,10 +27,10 @@ static_resources: - actions: - request_headers: header_name: :method - descriptor_key: req_method + descriptor_key: req.method - request_headers: header_name: :path - descriptor_key: req_path + descriptor_key: req.path http_filters: - name: envoy.filters.http.ratelimit typed_config: diff --git a/limitador-server/sandbox/envoy2.yaml b/limitador-server/sandbox/envoy2.yaml index 351ba1d3..d1d15958 100644 --- a/limitador-server/sandbox/envoy2.yaml +++ b/limitador-server/sandbox/envoy2.yaml @@ -27,10 +27,10 @@ static_resources: - actions: - request_headers: header_name: :method - descriptor_key: req_method + descriptor_key: req.method - request_headers: header_name: :path - descriptor_key: req_path + descriptor_key: req.path http_filters: - name: envoy.filters.http.ratelimit typed_config: diff --git a/limitador-server/sandbox/envoy3.yaml b/limitador-server/sandbox/envoy3.yaml index ba557fb0..03180c47 100644 --- a/limitador-server/sandbox/envoy3.yaml +++ b/limitador-server/sandbox/envoy3.yaml @@ -27,10 +27,10 @@ static_resources: - actions: - request_headers: header_name: :method - descriptor_key: req_method + descriptor_key: req.method - request_headers: header_name: :path - descriptor_key: req_path + descriptor_key: req.path http_filters: - name: envoy.filters.http.ratelimit typed_config: diff --git a/limitador-server/sandbox/limits.yaml b/limitador-server/sandbox/limits.yaml index 853c0340..cb354bf2 100644 --- a/limitador-server/sandbox/limits.yaml +++ b/limitador-server/sandbox/limits.yaml @@ -3,20 +3,20 @@ max_value: 10 seconds: 60 conditions: - - "req_method == 'GET'" - - "req_path != '/json'" + - "descriptors[0]['req.method'] == 'GET'" + - "descriptors[0]['req.path'] != '/json'" variables: [] - namespace: test_namespace max_value: 5 seconds: 60 conditions: - - "req_method == 'POST'" - - "req_path != '/json'" + - "descriptors[0]['req.method'] == 'POST'" + - "descriptors[0]['req.path'] != '/json'" variables: [] - namespace: test_namespace max_value: 50000 seconds: 10 conditions: - - "req_method == 'GET'" - - "req_path == '/json'" + - "descriptors[0]['req.method'] == 'GET'" + - "descriptors[0]['req.path'] == '/json'" variables: [] diff --git a/limitador-server/sandbox/load-test.json b/limitador-server/sandbox/load-test.json index 0cd6c81b..b23422cc 100644 --- a/limitador-server/sandbox/load-test.json +++ b/limitador-server/sandbox/load-test.json @@ -5,11 +5,11 @@ { "entries": [ { - "key": "req_method", + "key": "req.method", "value": "GET" }, { - "key": "req_path", + "key": "req.path", "value": "/json" } ] diff --git a/limitador-server/sandbox/redis-otel/README.md b/limitador-server/sandbox/redis-otel/README.md index 1759437a..8b2f10ab 100644 --- a/limitador-server/sandbox/redis-otel/README.md +++ b/limitador-server/sandbox/redis-otel/README.md @@ -24,7 +24,7 @@ bin/grpcurl -plaintext -d @ 127.0.0.1:18081 envoy.service.ratelimit.v3.RateLimit { "entries": [ { - "key": "req_method", + "key": "req.method", "value": "POST" } ] diff --git a/limitador-server/src/envoy_rls/server.rs b/limitador-server/src/envoy_rls/server.rs index b6d24e1b..0528f6b7 100644 --- a/limitador-server/src/envoy_rls/server.rs +++ b/limitador-server/src/envoy_rls/server.rs @@ -3,12 +3,6 @@ use opentelemetry::propagation::Extractor; use std::collections::HashMap; use std::sync::Arc; -use limitador::CheckResult; -use tonic::codegen::http::HeaderMap; -use tonic::{transport, transport::Server, Request, Response, Status}; -use tracing::Span; -use tracing_opentelemetry::OpenTelemetrySpanExt; - use crate::envoy_rls::server::envoy::config::core::v3::HeaderValue; use crate::envoy_rls::server::envoy::service::ratelimit::v3::rate_limit_response::Code; use crate::envoy_rls::server::envoy::service::ratelimit::v3::rate_limit_service_server::{ @@ -19,6 +13,12 @@ use crate::envoy_rls::server::envoy::service::ratelimit::v3::{ }; use crate::prometheus_metrics::PrometheusMetrics; use crate::Limiter; +use limitador::limit::Context; +use limitador::CheckResult; +use tonic::codegen::http::HeaderMap; +use tonic::{transport, transport::Server, Request, Response, Status}; +use tracing::Span; +use tracing_opentelemetry::OpenTelemetrySpanExt; include!("envoy_types.rs"); @@ -72,7 +72,7 @@ impl RateLimitService for MyRateLimiter { ) -> Result, Status> { debug!("Request received: {:?}", request); - let mut values: HashMap = HashMap::new(); + let mut values: Vec> = Vec::default(); let (metadata, _ext, req) = request.into_parts(); let namespace = req.domain; let rl_headers = RateLimitRequestHeaders::new(metadata.into_headers()); @@ -96,9 +96,11 @@ impl RateLimitService for MyRateLimiter { let namespace = namespace.into(); for descriptor in &req.descriptors { + let mut map = HashMap::default(); for entry in &descriptor.entries { - values.insert(entry.key.clone(), entry.value.clone()); + map.insert(entry.key.clone(), entry.value.clone()); } + values.push(map); } // "hits_addend" is optional according to the spec, and should default @@ -109,7 +111,8 @@ impl RateLimitService for MyRateLimiter { req.hits_addend }; - let ctx = values.into(); + let mut ctx = Context::default(); + ctx.list_binding("descriptors".to_string(), values); let rate_limited_resp = match &*self.limiter { Limiter::Blocking(limiter) => limiter.check_rate_limited_and_update( @@ -255,8 +258,12 @@ mod tests { namespace, 1, 60, - vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id".try_into().expect("failed parsing!")], + vec!["descriptors[0]['req.method'] == 'GET'" + .try_into() + .expect("failed parsing!")], + vec!["descriptors[0]['app.id']" + .try_into() + .expect("failed parsing!")], ); let limiter = RateLimiter::new(10_000); @@ -276,11 +283,11 @@ mod tests { descriptors: vec![RateLimitDescriptor { entries: vec![ Entry { - key: "req_method".to_string(), + key: "req.method".to_string(), value: "GET".to_string(), }, Entry { - key: "app_id".to_string(), + key: "app.id".to_string(), value: "1".to_string(), }, ], @@ -337,7 +344,7 @@ mod tests { domain: "test_namespace".to_string(), descriptors: vec![RateLimitDescriptor { entries: vec![Entry { - key: "req_method".to_string(), + key: "req.method".to_string(), value: "GET".to_string(), }], limit: None, @@ -371,7 +378,7 @@ mod tests { domain: "".to_string(), descriptors: vec![RateLimitDescriptor { entries: vec![Entry { - key: "req_method".to_string(), + key: "req.method".to_string(), value: "GET".to_string(), }], limit: None, @@ -400,18 +407,24 @@ mod tests { namespace, 10, 60, - vec!["x == '1'".try_into().expect("failed parsing!")], - vec!["z".try_into().expect("failed parsing!")], + vec!["descriptors[0].x == '1'" + .try_into() + .expect("failed parsing!")], + vec!["descriptors[0].z".try_into().expect("failed parsing!")], ), Limit::new( namespace, 0, 60, vec![ - "x == '1'".try_into().expect("failed parsing!"), - "y == '2'".try_into().expect("failed parsing!"), + "descriptors[0].x == '1'" + .try_into() + .expect("failed parsing!"), + "descriptors[1].y == '2'" + .try_into() + .expect("failed parsing!"), ], - vec!["z".try_into().expect("failed parsing!")], + vec!["descriptors[0].z".try_into().expect("failed parsing!")], ), ] .into_iter() @@ -480,8 +493,10 @@ mod tests { namespace, 10, 60, - vec!["x == '1'".try_into().expect("failed parsing!")], - vec!["y".try_into().expect("failed parsing!")], + vec!["descriptors[0].x == '1'" + .try_into() + .expect("failed parsing!")], + vec!["descriptors[0].y".try_into().expect("failed parsing!")], ); let limiter = RateLimiter::new(10_000); @@ -555,8 +570,10 @@ mod tests { namespace, 1, 60, - vec!["x == '1'".try_into().expect("failed parsing!")], - vec!["y".try_into().expect("failed parsing!")], + vec!["descriptors[0].x == '1'" + .try_into() + .expect("failed parsing!")], + vec!["descriptors[0].y".try_into().expect("failed parsing!")], ); let limiter = RateLimiter::new(10_000); diff --git a/limitador-server/src/http_api/server.rs b/limitador-server/src/http_api/server.rs index ed413e53..c9085502 100644 --- a/limitador-server/src/http_api/server.rs +++ b/limitador-server/src/http_api/server.rs @@ -3,6 +3,7 @@ use crate::prometheus_metrics::PrometheusMetrics; use crate::Limiter; use actix_web::{http::StatusCode, HttpResponse, HttpResponseBuilder, ResponseError}; use actix_web::{App, HttpServer}; +use limitador::limit::Context; use limitador::CheckResult; use paperclip::actix::{ api_v2_errors, @@ -122,7 +123,8 @@ async fn check( response_headers: _, } = request.into_inner(); let namespace = namespace.into(); - let ctx = values.into(); + let mut ctx = Context::default(); + ctx.list_binding("descriptors".to_string(), vec![values]); let is_rate_limited_result = match state.get_ref().limiter() { Limiter::Blocking(limiter) => limiter.is_rate_limited(&namespace, &ctx, delta), Limiter::Async(limiter) => limiter.is_rate_limited(&namespace, &ctx, delta).await, @@ -153,7 +155,8 @@ async fn report( response_headers: _, } = request.into_inner(); let namespace = namespace.into(); - let ctx = values.into(); + let mut ctx = Context::default(); + ctx.list_binding("descriptors".to_string(), vec![values]); let update_counters_result = match data.get_ref().limiter() { Limiter::Blocking(limiter) => limiter.update_counters(&namespace, &ctx, delta), Limiter::Async(limiter) => limiter.update_counters(&namespace, &ctx, delta).await, @@ -178,7 +181,8 @@ async fn check_and_report( response_headers, } = request.into_inner(); let namespace = namespace.into(); - let ctx = values.into(); + let mut ctx = Context::default(); + ctx.list_binding("descriptors".to_string(), vec![values]); let rate_limit_data = data.get_ref(); let rate_limited_and_update_result = match rate_limit_data.limiter() { Limiter::Blocking(limiter) => limiter.check_rate_limited_and_update( @@ -382,8 +386,8 @@ mod tests { // Prepare values to check let mut values = HashMap::new(); - values.insert("req_method".into(), "GET".into()); - values.insert("app_id".into(), "1".into()); + values.insert("req.method".into(), "GET".into()); + values.insert("req.id".into(), "1".into()); let info = CheckAndReportInfo { namespace: namespace.into(), values, @@ -433,8 +437,8 @@ mod tests { // Prepare values to check let mut values = HashMap::new(); - values.insert("req_method".into(), "GET".into()); - values.insert("app_id".into(), "1".into()); + values.insert("req.method".into(), "GET".into()); + values.insert("app.id".into(), "1".into()); let info = CheckAndReportInfo { namespace: namespace.into(), values, @@ -508,8 +512,8 @@ mod tests { // Prepare values to check let mut values = HashMap::new(); - values.insert("req_method".into(), "GET".into()); - values.insert("app_id".into(), "1".into()); + values.insert("req.method".into(), "GET".into()); + values.insert("app.id".into(), "1".into()); let info = CheckAndReportInfo { namespace: namespace.into(), values, @@ -551,8 +555,12 @@ mod tests { namespace, max, 60, - vec!["req_method == 'GET'".try_into().expect("failed parsing!")], - vec!["app_id".try_into().expect("failed parsing!")], + vec!["descriptors[0]['req.method'] == 'GET'" + .try_into() + .expect("failed parsing!")], + vec!["descriptors[0]['app.id']" + .try_into() + .expect("failed parsing!")], ); match &limiter {