From 517628f9e02edbfaff13e6051c688bb423a10a01 Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Thu, 7 Nov 2024 09:10:10 -0500 Subject: [PATCH] Err out better when failing to eval CEL Signed-off-by: Alex Snaps --- src/configuration/action.rs | 35 ++++++++++++----- src/configuration/action_set.rs | 15 +++++++- src/data/cel.rs | 54 +++++++++++++++++++-------- src/data/cel/strings.rs | 66 ++++++++++++++++----------------- 4 files changed, 110 insertions(+), 60 deletions(-) diff --git a/src/configuration/action.rs b/src/configuration/action.rs index 4cbfe31b..3e634d2f 100644 --- a/src/configuration/action.rs +++ b/src/configuration/action.rs @@ -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; @@ -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!") + } + }) } } @@ -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) => { diff --git a/src/configuration/action_set.rs b/src/configuration/action_set.rs index c2ebba8d..04f34f78 100644 --- a/src/configuration/action_set.rs +++ b/src/configuration/action_set.rs @@ -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; @@ -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!") + } + }) } } } diff --git a/src/data/cel.rs b/src/data/cel.rs index 0f71ce16..fc91770e 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -55,7 +55,7 @@ impl Expression { Self::new_expression(expression, true) } - pub fn eval(&self) -> Value { + pub fn eval(&self) -> Result { let mut ctx = create_context(); if self.extended { Self::add_extended_capabilities(&mut ctx) @@ -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`] @@ -198,10 +198,13 @@ impl Predicate { }) } - pub fn test(&self) -> bool { + pub fn test(&self) -> Result { 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), } } } @@ -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] @@ -604,35 +607,50 @@ 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 @@ -640,7 +658,10 @@ mod tests { 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()); } @@ -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(), @@ -675,7 +696,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(), @@ -683,7 +704,7 @@ mod tests { ))); let predicate = Predicate::route_rule("queryMap(request.query) == {'👾': ''}").expect("This is valid!"); - assert!(predicate.test()); + assert!(predicate.test().expect("This must evaluate properly!")); } #[test] @@ -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()))); } } diff --git a/src/data/cel/strings.rs b/src/data/cel/strings.rs index 88fdca43..bfcccdf0 100644 --- a/src/data/cel/strings.rs +++ b/src/data/cel/strings.rs @@ -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())); } }