Handle the conditions error at KuadrantFilter level

Signed-off-by: Adam Cattermole <[email protected]>

Author: adam-cattermole

src/data/cel.rs

@@ -302,6 +302,8 @@ pub struct Predicate {
     expression: Expression,
 }
 
+pub type PredicateResult = Result<bool, EvaluationError>;
+
 impl Predicate {
     pub fn new(predicate: &str) -> Result<Self, ParseError> {
         Ok(Self {
@@ -318,7 +320,7 @@ impl Predicate {
         })
     }
 
-    pub fn test(&self) -> Result<bool, EvaluationError> {
+    pub fn test(&self) -> PredicateResult {
         match self.expression.eval() {
             Ok(value) => match value {
                 Value::Bool(result) => Ok(result),
@@ -336,11 +338,11 @@ impl Predicate {
 }
 
 pub trait PredicateVec {
-    fn apply(&self) -> Result<bool, EvaluationError>;
+    fn apply(&self) -> PredicateResult;
 }
 
 impl PredicateVec for Vec<Predicate> {
-    fn apply(&self) -> Result<bool, EvaluationError> {
+    fn apply(&self) -> PredicateResult {
         if self.is_empty() {
             return Ok(true);
         }

src/data/mod.rs

@@ -10,6 +10,7 @@ pub use cel::debug_all_well_known_attributes;
 
 pub use cel::Expression;
 pub use cel::Predicate;
+pub use cel::PredicateResult;
 pub use cel::PredicateVec;
 
 pub use attribute::errors::PropertyError;

src/filter/kuadrant_filter.rs

@@ -4,7 +4,7 @@ use crate::filter::operations::{
 };
 use crate::runtime_action_set::RuntimeActionSet;
 use crate::service::{GrpcErrResponse, GrpcRequest, HeaderResolver, Headers};
-use log::{debug, warn};
+use log::{debug, error, warn};
 use proxy_wasm::traits::{Context, HttpContext};
 use proxy_wasm::types::{Action, Status};
 use std::mem;
@@ -59,15 +59,26 @@ impl HttpContext for KuadrantFilter {
             .index
             .get_longest_match_action_sets(self.request_authority().as_ref())
         {
-            if let Some(action_set) = action_sets
-                .iter()
-                .find(|action_set| action_set.conditions_apply(/* self */))
-            {
-                debug!(
-                    "#{} action_set selected {}",
-                    self.context_id, action_set.name
-                );
-                action = self.start_flow(Rc::clone(action_set))
+            for action_set in action_sets {
+                match action_set.conditions_apply() {
+                    Ok(true) => {
+                        debug!(
+                            "#{} action_set selected {}",
+                            self.context_id, action_set.name
+                        );
+                        action = self.start_flow(Rc::clone(action_set));
+                        break;
+                    }
+                    Ok(false) => continue,
+                    Err(e) => {
+                        error!(
+                            "#{} on_http_request_headers: failed to apply conditions: {:?}",
+                            self.context_id, e
+                        );
+                        self.die(GrpcErrResponse::new_internal_server_error());
+                        break;
+                    }
+                }
             }
         }
 

src/runtime_action_set.rs

@@ -1,5 +1,5 @@
 use crate::configuration::{ActionSet, Service};
-use crate::data::{Predicate, PredicateVec};
+use crate::data::{Predicate, PredicateResult, PredicateVec};
 use crate::runtime_action::RuntimeAction;
 use crate::service::{GrpcErrResponse, HeaderKind, IndexedGrpcRequest};
 use std::collections::HashMap;
@@ -56,9 +56,8 @@ impl RuntimeActionSet {
         folded_actions
     }
 
-    pub fn conditions_apply(&self) -> bool {
-        //todo(adam-cattermole): do not expect
-        self.route_rule_predicates.apply().expect("REMOVE")
+    pub fn conditions_apply(&self) -> PredicateResult {
+        self.route_rule_predicates.apply()
     }
 
     pub fn find_first_grpc_request(&self) -> Option<IndexedGrpcRequest> {
@@ -105,7 +104,8 @@ mod test {
         let runtime_action_set = RuntimeActionSet::new(&action_set, &HashMap::default())
             .expect("should not happen from an empty set of actions");
 
-        assert!(runtime_action_set.conditions_apply())
+        assert!(runtime_action_set.conditions_apply().is_ok());
+        assert!(runtime_action_set.conditions_apply().unwrap());
     }
 
     #[test]
@@ -122,7 +122,8 @@ mod test {
         let runtime_action_set = RuntimeActionSet::new(&action_set, &HashMap::default())
             .expect("should not happen from an empty set of actions");
 
-        assert!(runtime_action_set.conditions_apply())
+        assert!(runtime_action_set.conditions_apply().is_ok());
+        assert!(runtime_action_set.conditions_apply().unwrap());
     }
 
     #[test]
@@ -139,12 +140,12 @@ mod test {
         let runtime_action_set = RuntimeActionSet::new(&action_set, &HashMap::default())
             .expect("should not happen from an empty set of actions");
 
-        assert!(!runtime_action_set.conditions_apply())
+        assert!(runtime_action_set.conditions_apply().is_ok());
+        assert!(!runtime_action_set.conditions_apply().unwrap());
     }
 
     #[test]
-    #[should_panic]
-    fn when_a_cel_expression_does_not_evaluate_to_bool_panics() {
+    fn when_a_cel_expression_does_not_evaluate_to_bool_returns_error() {
         let action_set = ActionSet::new(
             "some_name".to_owned(),
             RouteRuleConditions {
@@ -156,7 +157,7 @@ mod test {
 
         let runtime_action_set = RuntimeActionSet::new(&action_set, &HashMap::default())
             .expect("should not happen from an empty set of actions");
-        runtime_action_set.conditions_apply();
+        assert!(runtime_action_set.conditions_apply().is_err());
     }
 
     fn build_rl_service() -> Service {