Skip to content

Commit

Permalink
Err out better when failing to eval CEL
Browse files Browse the repository at this point in the history 
Signed-off-by: Alex Snaps <[email protected]>
  • Loading branch information
@alexsnaps
alexsnaps committed Nov 7, 2024
1 parent da74bcd commit 517628f
Show file tree
Hide file tree
Showing 4 changed files with 110 additions and 60 deletions.
35 changes: 25 additions & 10 deletions src/configuration/action.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::configuration::{DataItem, DataType, PatternExpression};
use crate::data::Predicate;
use crate::envoy::{RateLimitDescriptor, RateLimitDescriptor_Entry};
use cel_interpreter::Value;
use log::debug;
use log::{debug, error};
use protobuf::RepeatedField;
use serde::Deserialize;
use std::cell::OnceCell;
Expand Down Expand Up @@ -31,7 +31,16 @@ impl Action {
if predicates.is_empty() {
self.conditions.is_empty() || self.conditions.iter().all(PatternExpression::applies)
} else {
predicates.iter().all(Predicate::test)
predicates
.iter()
.enumerate()
.all(|(pos, predicate)| match predicate.test() {
Ok(b) => b,
Err(err) => {
error!("Failed to evaluate {}: {}", self.predicates[pos], err);
panic!("Err out of this!")
}
})
}
}

Expand Down Expand Up @@ -60,14 +69,20 @@ impl Action {
.expect("Expression must be compiled by now")
.eval()
{
Value::Int(n) => format!("{n}"),
Value::UInt(n) => format!("{n}"),
Value::Float(n) => format!("{n}"),
// todo this probably should be a proper string literal!
Value::String(s) => (*s).clone(),
Value::Bool(b) => format!("{b}"),
Value::Null => "null".to_owned(),
_ => panic!("Only scalar values can be sent as data"),
Ok(value) => match value {
Value::Int(n) => format!("{n}"),
Value::UInt(n) => format!("{n}"),
Value::Float(n) => format!("{n}"),
// todo this probably should be a proper string literal!
Value::String(s) => (*s).clone(),
Value::Bool(b) => format!("{b}"),
Value::Null => "null".to_owned(),
_ => panic!("Only scalar values can be sent as data"),
},
Err(err) => {
error!("Failed to evaluate {}: {}", cel.value, err);
panic!("Err out of this!")
}
},
),
DataType::Selector(selector_item) => {
Expand Down
15 changes: 14 additions & 1 deletion src/configuration/action_set.rs
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use crate::configuration::action::Action;
use crate::configuration::PatternExpression;
use crate::data::Predicate;
use log::error;
use serde::Deserialize;
use std::cell::OnceCell;

Expand Down Expand Up @@ -51,7 +52,19 @@ impl ActionSet {
.iter()
.all(|m| m.applies())
} else {
predicates.iter().all(Predicate::test)
predicates
.iter()
.enumerate()
.all(|(pos, predicate)| match predicate.test() {
Ok(b) => b,
Err(err) => {
error!(
"Failed to evaluate {}: {}",
self.route_rule_conditions.predicates[pos], err
);
panic!("Err out of this!")
}
})
}
}
}
54 changes: 38 additions & 16 deletions src/data/cel.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ impl Expression {
Self::new_expression(expression, true)
}

pub fn eval(&self) -> Value {
pub fn eval(&self) -> Result<Value, String> {
let mut ctx = create_context();
if self.extended {
Self::add_extended_capabilities(&mut ctx)
Expand All @@ -70,7 +70,7 @@ impl Expression {
map.get(&binding.into()).cloned().unwrap_or(Value::Null),
);
}
Value::resolve(&self.expression, &ctx).expect("Cel expression couldn't be evaluated")
Value::resolve(&self.expression, &ctx).map_err(|err| format!("{err:?}"))
}

/// Add support for `queryMap`, see [`decode_query_string`]
Expand Down Expand Up @@ -198,10 +198,13 @@ impl Predicate {
})
}

pub fn test(&self) -> bool {
pub fn test(&self) -> Result<bool, String> {
match self.expression.eval() {
Value::Bool(result) => result,
_ => false,
Ok(value) => match value {
Value::Bool(result) => Ok(result),
_ => Err(format!("Expected boolean value, got {value:?}")),
},
Err(err) => Err(err),
}
}
}
Expand Down Expand Up @@ -582,7 +585,7 @@ mod tests {
let predicate = Predicate::new("source.port == 65432").expect("This is valid CEL!");
property::test::TEST_PROPERTY_VALUE
.set(Some(("source.port".into(), 65432_i64.to_le_bytes().into())));
assert!(predicate.test());
assert!(predicate.test().expect("This must evaluate properly!"));
}

#[test]
Expand All @@ -604,43 +607,61 @@ mod tests {
]),
"true".bytes().collect(),
)));
let value = Expression::new("auth.identity.anonymous").unwrap().eval();
let value = Expression::new("auth.identity.anonymous")
.unwrap()
.eval()
.expect("This must evaluate!");
assert_eq!(value, true.into());

property::test::TEST_PROPERTY_VALUE.set(Some((
property::Path::new(vec!["filter_state", "wasm.kuadrant.auth.identity.age"]),
"42".bytes().collect(),
)));
let value = Expression::new("auth.identity.age").unwrap().eval();
let value = Expression::new("auth.identity.age")
.unwrap()
.eval()
.expect("This must evaluate!");
assert_eq!(value, 42.into());

property::test::TEST_PROPERTY_VALUE.set(Some((
property::Path::new(vec!["filter_state", "wasm.kuadrant.auth.identity.age"]),
"42.3".bytes().collect(),
)));
let value = Expression::new("auth.identity.age").unwrap().eval();
let value = Expression::new("auth.identity.age")
.unwrap()
.eval()
.expect("This must evaluate!");
assert_eq!(value, 42.3.into());

property::test::TEST_PROPERTY_VALUE.set(Some((
property::Path::new(vec!["filter_state", "wasm.kuadrant.auth.identity.age"]),
"\"John\"".bytes().collect(),
)));
let value = Expression::new("auth.identity.age").unwrap().eval();
let value = Expression::new("auth.identity.age")
.unwrap()
.eval()
.expect("This must evaluate!");
assert_eq!(value, "John".into());

property::test::TEST_PROPERTY_VALUE.set(Some((
property::Path::new(vec!["filter_state", "wasm.kuadrant.auth.identity.name"]),
"-42".bytes().collect(),
)));
let value = Expression::new("auth.identity.name").unwrap().eval();
let value = Expression::new("auth.identity.name")
.unwrap()
.eval()
.expect("This must evaluate!");
assert_eq!(value, (-42).into());

// let's fall back to strings, as that's what we read and set in store_metadata
property::test::TEST_PROPERTY_VALUE.set(Some((
property::Path::new(vec!["filter_state", "wasm.kuadrant.auth.identity.age"]),
"some random crap".bytes().collect(),
)));
let value = Expression::new("auth.identity.age").unwrap().eval();
let value = Expression::new("auth.identity.age")
.unwrap()
.eval()
.expect("This must evaluate!");
assert_eq!(value, "some random crap".into());
}

Expand All @@ -661,7 +682,7 @@ mod tests {
",
)
.expect("This is valid!");
assert!(predicate.test());
assert!(predicate.test().expect("This must evaluate properly!"));

property::test::TEST_PROPERTY_VALUE.set(Some((
"request.query".into(),
Expand All @@ -675,15 +696,15 @@ mod tests {
",
)
.expect("This is valid!");
assert!(predicate.test());
assert!(predicate.test().expect("This must evaluate properly!"));

property::test::TEST_PROPERTY_VALUE.set(Some((
"request.query".into(),
"%F0%9F%91%BE".bytes().collect(),
)));
let predicate =
Predicate::route_rule("queryMap(request.query) == {'👾': ''}").expect("This is valid!");
assert!(predicate.test());
assert!(predicate.test().expect("This must evaluate properly!"));
}

#[test]
Expand Down Expand Up @@ -721,7 +742,8 @@ mod tests {
)));
let value = Expression::new("getHostProperty(['foo', 'bar.baz'])")
.unwrap()
.eval();
.eval()
.expect("This must evaluate!");
assert_eq!(value, Value::Bytes(Arc::new(b"\xCA\xFE".to_vec())));
}
}
66 changes: 33 additions & 33 deletions src/data/cel/strings.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,92 +275,92 @@ mod tests {
#[test]
fn extended_string_fn() {
let e = Expression::new("'abc'.charAt(1)").expect("This must be valid CEL");
assert_eq!(e.eval(), "b".into());
assert_eq!(e.eval(), Ok("b".into()));

let e = Expression::new("'hello mellow'.indexOf('')").expect("This must be valid CEL");
assert_eq!(e.eval(), 0.into());
assert_eq!(e.eval(), Ok(0.into()));
let e = Expression::new("'hello mellow'.indexOf('ello')").expect("This must be valid CEL");
assert_eq!(e.eval(), 1.into());
assert_eq!(e.eval(), Ok(1.into()));
let e = Expression::new("'hello mellow'.indexOf('jello')").expect("This must be valid CEL");
assert_eq!(e.eval(), (-1).into());
assert_eq!(e.eval(), Ok((-1).into()));
let e = Expression::new("'hello mellow'.indexOf('', 2)").expect("This must be valid CEL");
assert_eq!(e.eval(), 2.into());
assert_eq!(e.eval(), Ok(2.into()));
let e =
Expression::new("'hello mellow'.indexOf('ello', 20)").expect("This must be valid CEL");
assert_eq!(e.eval(), (-1).into());
assert_eq!(e.eval(), Ok((-1).into()));

let e = Expression::new("'hello mellow'.lastIndexOf('')").expect("This must be valid CEL");
assert_eq!(e.eval(), 12.into());
assert_eq!(e.eval(), Ok(12.into()));
let e =
Expression::new("'hello mellow'.lastIndexOf('ello')").expect("This must be valid CEL");
assert_eq!(e.eval(), 7.into());
assert_eq!(e.eval(), Ok(7.into()));
let e =
Expression::new("'hello mellow'.lastIndexOf('jello')").expect("This must be valid CEL");
assert_eq!(e.eval(), (-1).into());
assert_eq!(e.eval(), Ok((-1).into()));
let e = Expression::new("'hello mellow'.lastIndexOf('ello', 6)")
.expect("This must be valid CEL");
assert_eq!(e.eval(), 1.into());
assert_eq!(e.eval(), Ok(1.into()));
let e = Expression::new("'hello mellow'.lastIndexOf('ello', 20)")
.expect("This must be valid CEL");
assert_eq!(e.eval(), (-1).into());
assert_eq!(e.eval(), Ok((-1).into()));

let e = Expression::new("['hello', 'mellow'].join()").expect("This must be valid CEL");
assert_eq!(e.eval(), "hellomellow".into());
assert_eq!(e.eval(), Ok("hellomellow".into()));
let e = Expression::new("[].join()").expect("This must be valid CEL");
assert_eq!(e.eval(), "".into());
assert_eq!(e.eval(), Ok("".into()));
let e = Expression::new("['hello', 'mellow'].join(' ')").expect("This must be valid CEL");
assert_eq!(e.eval(), "hello mellow".into());
assert_eq!(e.eval(), Ok("hello mellow".into()));

let e = Expression::new("'TacoCat'.lowerAscii()").expect("This must be valid CEL");
assert_eq!(e.eval(), "tacocat".into());
assert_eq!(e.eval(), Ok("tacocat".into()));
let e = Expression::new("'TacoCÆt Xii'.lowerAscii()").expect("This must be valid CEL");
assert_eq!(e.eval(), "tacocÆt xii".into());
assert_eq!(e.eval(), Ok("tacocÆt xii".into()));

let e = Expression::new("'TacoCat'.upperAscii()").expect("This must be valid CEL");
assert_eq!(e.eval(), "TACOCAT".into());
assert_eq!(e.eval(), Ok("TACOCAT".into()));
let e = Expression::new("'TacoCÆt Xii'.upperAscii()").expect("This must be valid CEL");
assert_eq!(e.eval(), "TACOCÆT XII".into());
assert_eq!(e.eval(), Ok("TACOCÆT XII".into()));

let e = Expression::new("' \ttrim\n '.trim()").expect("This must be valid CEL");
assert_eq!(e.eval(), "trim".into());
assert_eq!(e.eval(), Ok("trim".into()));

let e =
Expression::new("'hello hello'.replace('he', 'we')").expect("This must be valid CEL");
assert_eq!(e.eval(), "wello wello".into());
assert_eq!(e.eval(), Ok("wello wello".into()));
let e = Expression::new("'hello hello'.replace('he', 'we', -1)")
.expect("This must be valid CEL");
assert_eq!(e.eval(), "wello wello".into());
assert_eq!(e.eval(), Ok("wello wello".into()));
let e = Expression::new("'hello hello'.replace('he', 'we', 1)")
.expect("This must be valid CEL");
assert_eq!(e.eval(), "wello hello".into());
assert_eq!(e.eval(), Ok("wello hello".into()));
let e = Expression::new("'hello hello'.replace('he', 'we', 0)")
.expect("This must be valid CEL");
assert_eq!(e.eval(), "hello hello".into());
assert_eq!(e.eval(), Ok("hello hello".into()));
let e = Expression::new("'hello hello'.replace('', '_')").expect("This must be valid CEL");
assert_eq!(e.eval(), "_h_e_l_l_o_ _h_e_l_l_o_".into());
assert_eq!(e.eval(), Ok("_h_e_l_l_o_ _h_e_l_l_o_".into()));
let e = Expression::new("'hello hello'.replace('h', '')").expect("This must be valid CEL");
assert_eq!(e.eval(), "ello ello".into());
assert_eq!(e.eval(), Ok("ello ello".into()));

let e = Expression::new("'hello hello hello'.split(' ')").expect("This must be valid CEL");
assert_eq!(e.eval(), vec!["hello", "hello", "hello"].into());
assert_eq!(e.eval(), Ok(vec!["hello", "hello", "hello"].into()));
let e =
Expression::new("'hello hello hello'.split(' ', 0)").expect("This must be valid CEL");
assert_eq!(e.eval(), Value::List(vec![].into()));
assert_eq!(e.eval(), Ok(Value::List(vec![].into())));
let e =
Expression::new("'hello hello hello'.split(' ', 1)").expect("This must be valid CEL");
assert_eq!(e.eval(), vec!["hello hello hello"].into());
assert_eq!(e.eval(), Ok(vec!["hello hello hello"].into()));
let e =
Expression::new("'hello hello hello'.split(' ', 2)").expect("This must be valid CEL");
assert_eq!(e.eval(), vec!["hello", "hello hello"].into());
assert_eq!(e.eval(), Ok(vec!["hello", "hello hello"].into()));
let e =
Expression::new("'hello hello hello'.split(' ', -1)").expect("This must be valid CEL");
assert_eq!(e.eval(), vec!["hello", "hello", "hello"].into());
assert_eq!(e.eval(), Ok(vec!["hello", "hello", "hello"].into()));

let e = Expression::new("'tacocat'.substring(4)").expect("This must be valid CEL");
assert_eq!(e.eval(), "cat".into());
assert_eq!(e.eval(), Ok("cat".into()));
let e = Expression::new("'tacocat'.substring(0, 4)").expect("This must be valid CEL");
assert_eq!(e.eval(), "taco".into());
assert_eq!(e.eval(), Ok("taco".into()));
let e = Expression::new("'ta©o©αT'.substring(2, 6)").expect("This must be valid CEL");
assert_eq!(e.eval(), "©o©α".into());
assert_eq!(e.eval(), Ok("©o©α".into()));
}
}

0 comments on commit 517628f

Please sign in to comment.