Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Err out better when failing to eval CEL #135

Merged
merged 1 commit into from
Nov 7, 2024
+110 −60
Merged

Err out better when failing to eval CEL #135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 25 additions & 10 deletions src/configuration/action.rs
View file
Open in desktop
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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and these are the real things that I wanted...

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);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and this ...

panic!("Err out of this!")
}
},
),
DataType::Selector(selector_item) => {
Expand Down
15 changes: 14 additions & 1 deletion src/configuration/action_set.rs
View file
Open in desktop
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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... and this.

);
panic!("Err out of this!")
}
})
}
}
}
54 changes: 38 additions & 16 deletions src/data/cel.rs
View file
Open in desktop
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> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only real change really...

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
Open in desktop
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()));
}
}
Loading