From f07915d77a55f20082890564b70cea3fa38b6114 Mon Sep 17 00:00:00 2001 From: dd di cesare Date: Tue, 29 Oct 2024 10:55:31 +0100 Subject: [PATCH] [refactor] Implementing OperationError Signed-off-by: dd di cesare --- src/configuration.rs | 2 +- src/filter/http_context.rs | 20 +++++++----- src/operation_dispatcher.rs | 63 ++++++++++++++++++++++++++++--------- src/service.rs | 8 ++--- src/service/auth.rs | 2 +- src/service/rate_limit.rs | 2 +- 6 files changed, 67 insertions(+), 30 deletions(-) diff --git a/src/configuration.rs b/src/configuration.rs index 22c23380..cbaa6eff 100644 --- a/src/configuration.rs +++ b/src/configuration.rs @@ -518,7 +518,7 @@ impl TryFrom for FilterConfig { } } -#[derive(Deserialize, Debug, Clone, Default, PartialEq)] +#[derive(Deserialize, Debug, Copy, Clone, Default, PartialEq)] #[serde(rename_all = "lowercase")] pub enum FailureMode { #[default] diff --git a/src/filter/http_context.rs b/src/filter/http_context.rs index a49970f3..aeff9c4a 100644 --- a/src/filter/http_context.rs +++ b/src/filter/http_context.rs @@ -1,6 +1,6 @@ use crate::configuration::action_set::ActionSet; use crate::configuration::{FailureMode, FilterConfig}; -use crate::operation_dispatcher::OperationDispatcher; +use crate::operation_dispatcher::{OperationDispatcher, OperationError}; use crate::service::GrpcService; use log::{debug, warn}; use proxy_wasm::traits::{Context, HttpContext}; @@ -65,12 +65,16 @@ impl Filter { } } } else { - match res { - Err((Status::Empty, _)) => Action::Continue, - Err((_, failure_mode)) => { - if failure_mode == FailureMode::Deny { - self.send_http_response(500, vec![], Some(b"Internal Server Error.\n")) - } + match res.unwrap_err() { + OperationError { + status: Status::Empty, + .. + } => Action::Continue, + OperationError { + failure_mode: FailureMode::Deny, + .. + } => { + self.send_http_response(500, vec![], Some(b"Internal Server Error.\n")); Action::Continue } _ => Action::Continue, @@ -131,7 +135,7 @@ impl Context for Filter { } } else { warn!("No Operation found with token_id: {token_id}"); - GrpcService::handle_error_on_grpc_response(&FailureMode::Deny); // TODO(didierofrivia): Decide on what's the default failure mode + GrpcService::handle_error_on_grpc_response(FailureMode::Deny); // TODO(didierofrivia): Decide on what's the default failure mode } } } diff --git a/src/operation_dispatcher.rs b/src/operation_dispatcher.rs index 07c96858..81202685 100644 --- a/src/operation_dispatcher.rs +++ b/src/operation_dispatcher.rs @@ -7,6 +7,7 @@ use proxy_wasm::hostcalls; use proxy_wasm::types::{Bytes, MapType, Status}; use std::cell::RefCell; use std::collections::HashMap; +use std::fmt; use std::rc::Rc; use std::time::Duration; @@ -31,10 +32,10 @@ impl State { } } -#[derive(Clone)] +#[derive(Clone, Debug)] pub(crate) struct Operation { state: RefCell, - result: RefCell>, + result: RefCell>, service: Rc, action: Action, service_handler: Rc, @@ -63,7 +64,7 @@ impl Operation { } } - fn trigger(&self) -> Result { + fn trigger(&self) -> Result { if let Some(message) = (self.grpc_message_build_fn)(self.get_service_type(), &self.action) { let res = self.service_handler.send( self.get_map_values_bytes_fn, @@ -71,9 +72,14 @@ impl Operation { message, self.service.timeout.0, ); - self.set_result(res); + match res { + Ok(token_id) => self.set_result(Ok(token_id)), + Err(status) => { + self.set_result(Err(OperationError::new(status, self.get_failure_mode()))) + } + } self.next_state(); - res + self.get_result() } else { self.done(); self.get_result() @@ -92,11 +98,11 @@ impl Operation { *self.state.borrow() } - pub fn get_result(&self) -> Result { + pub fn get_result(&self) -> Result { *self.result.borrow() } - fn set_result(&self, result: Result) { + fn set_result(&self, result: Result) { *self.result.borrow_mut() = result; } @@ -104,8 +110,35 @@ impl Operation { &self.service.service_type } - pub fn get_failure_mode(&self) -> &FailureMode { - &self.service.failure_mode + pub fn get_failure_mode(&self) -> FailureMode { + self.service.failure_mode + } +} +#[derive(Copy, Clone, Debug, PartialEq)] +pub struct OperationError { + pub status: Status, + pub failure_mode: FailureMode, +} + +impl OperationError { + fn new(status: Status, failure_mode: FailureMode) -> Self { + Self { + status, + failure_mode, + } + } +} + +impl fmt::Display for OperationError { + fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { + match self.status { + Status::Empty => { + write!(f, "No more operations to perform ") + } + _ => { + write!(f, "Error triggering the operation. {:?}", self.status) + } + } } } @@ -147,7 +180,7 @@ impl OperationDispatcher { self.operations.extend(operations); } - pub fn next(&mut self) -> Result, (Status, FailureMode)> { + pub fn next(&mut self) -> Result, OperationError> { if let Some((i, operation)) = self.operations.iter_mut().enumerate().next() { match operation.get_state() { State::Pending => { @@ -167,9 +200,9 @@ impl OperationDispatcher { State::Done => self.next(), } } - Err(status) => { - error!("{status:?}"); - Err((status, operation.get_failure_mode().clone())) + Err(err) => { + error!("{err:?}"); + Err(err) } } } else { @@ -191,7 +224,7 @@ impl OperationDispatcher { } } } else { - Err((Status::Empty, FailureMode::default())) // No more operations + Err(OperationError::new(Status::Empty, FailureMode::default())) // No more operations } } @@ -333,7 +366,7 @@ mod tests { assert_eq!(operation.get_state(), State::Pending); assert_eq!(*operation.get_service_type(), ServiceType::RateLimit); - assert_eq!(*operation.get_failure_mode(), FailureMode::Deny); + assert_eq!(operation.get_failure_mode(), FailureMode::Deny); assert_eq!(operation.get_result(), Ok(0)); } diff --git a/src/service.rs b/src/service.rs index 5ca9635e..e84d9a72 100644 --- a/src/service.rs +++ b/src/service.rs @@ -18,7 +18,7 @@ use std::cell::OnceCell; use std::rc::Rc; use std::time::Duration; -#[derive(Default)] +#[derive(Default, Debug)] pub struct GrpcService { service: Rc, name: &'static str, @@ -81,7 +81,7 @@ impl GrpcService { } } - pub fn handle_error_on_grpc_response(failure_mode: &FailureMode) { + pub fn handle_error_on_grpc_response(failure_mode: FailureMode) { match failure_mode { FailureMode::Deny => { hostcalls::send_http_response(500, vec![], Some(b"Internal Server Error.\n")) @@ -105,7 +105,7 @@ pub type GetMapValuesBytesFn = fn(map_type: MapType, key: &str) -> Result