From 457d09799940f32922cecdbae0a3374153e2f34f Mon Sep 17 00:00:00 2001 From: Alex Snaps Date: Thu, 28 Nov 2024 05:54:09 -0500 Subject: [PATCH 01/21] Start building out flow Co-authored-by: Adam Cattermole Signed-off-by: Alex Snaps --- src/filter.rs | 2 + src/filter/proposal_context.rs | 172 +++++++++++++++++++++++++++++++++ src/runtime_action.rs | 86 +++++++++++++++++ src/runtime_action_set.rs | 4 + src/service.rs | 70 ++++++++++++++ 5 files changed, 334 insertions(+) create mode 100644 src/filter/proposal_context.rs diff --git a/src/filter.rs b/src/filter.rs index ed9c241..c0f0ce5 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -1,4 +1,6 @@ pub(crate) mod http_context; +#[allow(dead_code)] +pub(crate) mod proposal_context; mod root_context; #[cfg_attr( diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs new file mode 100644 index 0000000..148b4fa --- /dev/null +++ b/src/filter/proposal_context.rs @@ -0,0 +1,172 @@ +use crate::action_set_index::ActionSetIndex; +use crate::filter::proposal_context::no_implicit_dep::{EndRequestOperation, HeadersOperation}; +use crate::service::HeaderResolver; +use log::warn; +use proxy_wasm::hostcalls; +use proxy_wasm::traits::{Context, HttpContext}; +use proxy_wasm::types::{Action, Status}; +use std::mem; +use std::rc::Rc; + +pub mod no_implicit_dep { + use crate::runtime_action_set::RuntimeActionSet; + use crate::service::GrpcRequest; + use std::rc::Rc; + + #[allow(dead_code)] + pub enum PendingOperation { + SendGrpcRequest(GrpcMessageSenderOperation), + AwaitGrpcResponse(GrpcMessageReceiverOperation), + AddHeaders(HeadersOperation), + Die(EndRequestOperation), + } + + pub struct GrpcMessageSenderOperation { + runtime_action_set: Rc, + current_index: usize, + } + + impl GrpcMessageSenderOperation { + pub fn progress(self) -> (GrpcRequest, PendingOperation) { + let action_set = self + .runtime_action_set + .runtime_actions + .get(self.current_index) + .unwrap(); + let msg = action_set.create_message(); + // todo(adam-cattermole): should this instead return only a GrpcReceiver? failure? + ( + msg, + PendingOperation::AwaitGrpcResponse(GrpcMessageReceiverOperation { + runtime_action_set: self.runtime_action_set, + current_index: self.current_index, + }), + ) + } + } + + pub struct GrpcMessageReceiverOperation { + runtime_action_set: Rc, + current_index: usize, + } + + impl GrpcMessageReceiverOperation { + pub fn progress(self, _msg: &[u8]) -> PendingOperation { + todo!() + } + + pub fn fail(self) -> PendingOperation { + PendingOperation::Die(EndRequestOperation { status: 500 }) + } + } + + pub struct HeadersOperation {} + + pub struct EndRequestOperation { + pub status: u32, + } +} + +struct Filter { + index: ActionSetIndex, + header_resolver: Rc, + + grpc_message_receiver_operation: Option, + headers_operations: Vec, +} + +impl Context for Filter { + fn on_grpc_call_response(&mut self, _token_id: u32, status_code: u32, resp_size: usize) { + let receiver = mem::take(&mut self.grpc_message_receiver_operation) + .expect("We need an operation pending a gRPC response"); + let next = if status_code != Status::Ok as u32 { + match self.get_grpc_call_response_body(0, resp_size) { + Some(response_body) => receiver.progress(response_body.as_slice()), + None => receiver.fail(), + } + } else { + receiver.fail() + }; + self.handle_operation(next); + } +} + +impl HttpContext for Filter { + fn on_http_request_headers(&mut self, _: usize, _: bool) -> Action { + if let Some(action_sets) = self + .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 */)) + { + self.handle_operation(action_set.start_flow()) + } + } + Action::Continue + } + + fn on_http_response_headers(&mut self, _num_headers: usize, _end_of_stream: bool) -> Action { + for _op in self.headers_operations.drain(..) { + todo!("Add the headers") + } + Action::Continue + } +} + +impl Filter { + fn handle_operation(&mut self, operation: no_implicit_dep::PendingOperation) { + match operation { + no_implicit_dep::PendingOperation::SendGrpcRequest(sender_op) => { + let (msg, op) = sender_op.progress(); + match self.send_grpc_request(msg) { + Ok(_token) => self.handle_operation(op), + Err(_status) => {} + } + } + no_implicit_dep::PendingOperation::AwaitGrpcResponse(receiver_op) => { + self.grpc_message_receiver_operation = Some(receiver_op) + } + no_implicit_dep::PendingOperation::AddHeaders(header_op) => { + self.headers_operations.push(header_op) + } + no_implicit_dep::PendingOperation::Die(die_op) => self.die(die_op), + } + } + + fn die(&mut self, die: EndRequestOperation) { + self.send_http_response(die.status, Vec::default(), None); + } + + fn request_authority(&self) -> String { + match self.get_http_request_header(":authority") { + None => { + warn!(":authority header not found"); + String::new() + } + Some(host) => { + let split_host = host.split(':').collect::>(); + split_host[0].to_owned() + } + } + } + + fn send_grpc_request(&self, req: crate::service::GrpcRequest) -> Result { + let headers = self + .header_resolver + .get_with_ctx(self) + .iter() + .map(|(header, value)| (*header, value.as_slice())) + .collect(); + + self.dispatch_grpc_call( + req.upstream_name(), + req.service_name(), + req.method_name(), + headers, + req.message(), + req.timeout(), + ) + } +} diff --git a/src/runtime_action.rs b/src/runtime_action.rs index 464b04f..e42699f 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -5,6 +5,7 @@ use crate::service::GrpcService; use std::collections::HashMap; use std::rc::Rc; use std::time::Duration; +use crate::filter::proposal_context::no_implicit_dep::PendingOperation; #[derive(Debug)] pub enum RuntimeAction { @@ -63,13 +64,98 @@ impl RuntimeAction { } Some(other) } + + pub fn create_message(&self) -> crate::service::GrpcRequest { + self.grpc_service().build_request(None) + } + + pub fn process(&self) -> Option { + if !self.conditions_apply() { + None + } else { + // if provided message return what? + // if no message we assume we're a sender?????????? + todo!() + } + } } + #[cfg(test)] mod test { use super::*; use crate::configuration::{Action, FailureMode, ServiceType, Timeout}; + pub enum Operation { + SendRequest(RequestSender), + ConsumeRequest(RequestConsumer), + } + type RequestSender = (); + // struct RequestSender {} + // impl RequestSender { + // + // } + + type RequestConsumer = (); + // struct RequestConsumer {} + // impl RequestConsumer { + // + // } + + #[test] + fn start_action_set_flow() { + let actions = vec![ + RuntimeAction::new(&build_action("ratelimit", "scope"), &HashMap::default()).unwrap(), + RuntimeAction::new(&build_action("ratelimit", "scope"), &HashMap::default()).unwrap(), + ]; + let mut iter = actions.iter(); + let a = iter.next().expect("get the first action"); + + let op: Result, ()> = a.create_message(); // action.? + let ret: RequestSender = match op { + Ok(_) => unreachable!("should have failed"), + Err(_) => match iter.next() { + Some(b) => b.create_message().expect("Ok").expect("Some"), + None => (), + }, + }; + + // this is caller code + + // on_http_request:find_action_set:start_flow + + // let (message_handler, req) = ret.create_request(); + // let token = send_request(req); + + let (message, handler) = ret.create_request() // SendMessageOperation -> ReceiveMessageOperation + // how does this function look? + // does it take into account current action? + + // on_grpc_response + + let response = message_handler.consume(response); + + /* bs + let next = action_set.progress(op); + action_set.actions[op.action_index].progress(op); + + struct Operation { + current: RuntimeAction, + next: Option + } + */ + } + + /* Overall + - We have Operation that transitions between different states passing the ref to ActionSet + to subsequent actions as well as an index + - The action_set has either a start_flow function, or maybe just process? + + this iterates over the actions to find the next one + - The runtime_action has the ability to create a message? + + */ + + fn build_rl_service() -> Service { Service { service_type: ServiceType::RateLimit, diff --git a/src/runtime_action_set.rs b/src/runtime_action_set.rs index 840ed6e..c6e986b 100644 --- a/src/runtime_action_set.rs +++ b/src/runtime_action_set.rs @@ -67,6 +67,10 @@ impl RuntimeActionSet { } }) } + + pub fn start_flow(&self) -> crate::filter::proposal_context::no_implicit_dep::PendingOperation { + todo!("implement me!") + } } #[cfg(test)] diff --git a/src/service.rs b/src/service.rs index 1145c1e..eea12cf 100644 --- a/src/service.rs +++ b/src/service.rs @@ -63,6 +63,15 @@ impl GrpcService { fn method(&self) -> &str { self.method } + pub fn build_request(&self, message: Option<&[u8]>) -> GrpcRequest { + GrpcRequest::new( + self.endpoint(), + self.name(), + self.method(), + self.get_timeout(), + message, + ) + } pub fn process_grpc_response( operation: Rc, @@ -105,6 +114,52 @@ impl GrpcService { } } +pub struct GrpcRequest<'a> { + upstream_name: &'a str, + service_name: &'a str, + method_name: &'a str, + timeout: Duration, + message: Option<&'a [u8]>, +} + +impl GrpcRequest { + pub fn new( + upstream_name: &str, + service_name: &str, + method_name: &str, + timeout: Duration, + message: Option<&[u8]>, + ) -> Self { + Self { + upstream_name, + service_name, + method_name, + timeout, + message, + } + } + + pub fn upstream_name(&self) -> &str { + &self.upstream_name + } + + pub fn service_name(&self) -> &str { + &self.service_name + } + + pub fn method_name(&self) -> &str { + &self.method_name + } + + pub fn timeout(&self) -> Duration { + self.timeout + } + + pub fn message(&self) -> Option<&[u8]> { + self.message + } +} + pub struct GrpcResult { pub response_headers: Vec<(String, String)>, } @@ -206,6 +261,21 @@ impl HeaderResolver { headers }) } + + pub fn get_with_ctx( + &self, + ctx: &T, + ) -> &Vec<(&'static str, Bytes)> { + self.headers.get_or_init(|| { + let mut headers = Vec::new(); + for header in TracingHeader::all() { + if let Ok(Some(value)) = ctx.get_http_request_header((*header).as_str()) { + headers.push(((*header).as_str(), value)); + } + } + headers + }) + } } // tracing headers From ee45e8993be11e7387c3d56c64e94b26cf3c1864 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 11 Dec 2024 11:37:02 +0000 Subject: [PATCH 02/21] Use context in header resolver Signed-off-by: Adam Cattermole --- src/filter/proposal_context.rs | 1 + src/runtime_action.rs | 24 ++++++------- src/service.rs | 64 ++++++++++++++++++++++++++++------ 3 files changed, 65 insertions(+), 24 deletions(-) diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs index 148b4fa..50993c6 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/proposal_context.rs @@ -27,6 +27,7 @@ pub mod no_implicit_dep { } impl GrpcMessageSenderOperation { + // todo(adam-cattermole): unwrap.. pub fn progress(self) -> (GrpcRequest, PendingOperation) { let action_set = self .runtime_action_set diff --git a/src/runtime_action.rs b/src/runtime_action.rs index e42699f..60ae201 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -1,11 +1,11 @@ use crate::auth_action::AuthAction; use crate::configuration::{Action, FailureMode, Service, ServiceType}; +use crate::filter::proposal_context::no_implicit_dep::PendingOperation; use crate::ratelimit_action::RateLimitAction; use crate::service::GrpcService; use std::collections::HashMap; use std::rc::Rc; use std::time::Duration; -use crate::filter::proposal_context::no_implicit_dep::PendingOperation; #[derive(Debug)] pub enum RuntimeAction { @@ -80,7 +80,6 @@ impl RuntimeAction { } } - #[cfg(test)] mod test { use super::*; @@ -111,14 +110,14 @@ mod test { let mut iter = actions.iter(); let a = iter.next().expect("get the first action"); - let op: Result, ()> = a.create_message(); // action.? - let ret: RequestSender = match op { - Ok(_) => unreachable!("should have failed"), - Err(_) => match iter.next() { - Some(b) => b.create_message().expect("Ok").expect("Some"), - None => (), - }, - }; + // let op: Result, ()> = a.create_message(); // action.? + // let ret: RequestSender = match op { + // Ok(_) => unreachable!("should have failed"), + // Err(_) => match iter.next() { + // Some(b) => b.create_message().expect("Ok").expect("Some"), + // None => (), + // }, + // }; // this is caller code @@ -127,13 +126,13 @@ mod test { // let (message_handler, req) = ret.create_request(); // let token = send_request(req); - let (message, handler) = ret.create_request() // SendMessageOperation -> ReceiveMessageOperation + // let (message, handler) = ret.create_request() // SendMessageOperation -> ReceiveMessageOperation // how does this function look? // does it take into account current action? // on_grpc_response - let response = message_handler.consume(response); + // let response = message_handler.consume(response); /* bs let next = action_set.progress(op); @@ -155,7 +154,6 @@ mod test { */ - fn build_rl_service() -> Service { Service { service_type: ServiceType::RateLimit, diff --git a/src/service.rs b/src/service.rs index eea12cf..b6925b3 100644 --- a/src/service.rs +++ b/src/service.rs @@ -114,12 +114,12 @@ impl GrpcService { } } -pub struct GrpcRequest<'a> { - upstream_name: &'a str, - service_name: &'a str, - method_name: &'a str, +pub struct GrpcRequest { + upstream_name: String, + service_name: String, + method_name: String, timeout: Duration, - message: Option<&'a [u8]>, + message: Option>, } impl GrpcRequest { @@ -131,11 +131,11 @@ impl GrpcRequest { message: Option<&[u8]>, ) -> Self { Self { - upstream_name, - service_name, - method_name, + upstream_name: upstream_name.to_owned(), + service_name: service_name.to_owned(), + method_name: method_name.to_owned(), timeout, - message, + message: message.map(|m| m.to_vec()), } } @@ -156,7 +156,7 @@ impl GrpcRequest { } pub fn message(&self) -> Option<&[u8]> { - self.message + self.message.as_deref() } } @@ -269,7 +269,7 @@ impl HeaderResolver { self.headers.get_or_init(|| { let mut headers = Vec::new(); for header in TracingHeader::all() { - if let Ok(Some(value)) = ctx.get_http_request_header((*header).as_str()) { + if let Some(value) = ctx.get_http_request_header_bytes((*header).as_str()) { headers.push(((*header).as_str(), value)); } } @@ -298,3 +298,45 @@ impl TracingHeader { } } } + +#[cfg(test)] +mod test { + use super::*; + use proxy_wasm::traits::Context; + use std::collections::HashMap; + + struct MockHost { + headers: HashMap<&'static str, Bytes>, + } + + impl MockHost { + pub fn new(headers: HashMap<&'static str, Bytes>) -> Self { + Self { headers } + } + } + + impl Context for MockHost {} + + impl proxy_wasm::traits::HttpContext for MockHost { + fn get_http_request_header_bytes(&self, name: &str) -> Option { + self.headers.get(name).map(|b| b.to_owned()) + } + } + + #[test] + fn read_headers() { + let header_resolver = HeaderResolver::new(); + + let headers: Vec<(&str, Bytes)> = vec![("traceparent", b"xyz".to_vec())]; + let mock_host = MockHost::new(headers.iter().cloned().collect::>()); + + let resolver_headers = header_resolver.get_with_ctx(&mock_host); + + headers.iter().zip(resolver_headers.iter()).for_each( + |((header_one, value_one), (header_two, value_two))| { + assert_eq!(header_one, header_two); + assert_eq!(value_one, value_two); + }, + ) + } +} From 9015bdafcc8a567606dc6efaa0e20b77a9d0e14b Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 11 Dec 2024 14:15:35 +0000 Subject: [PATCH 03/21] Build the messages in the RuntimeActionSet Signed-off-by: Adam Cattermole --- src/filter/proposal_context.rs | 39 ++++++++++++++++++---------------- src/runtime_action.rs | 35 +++++++++++++++++++++++------- src/runtime_action_set.rs | 10 +++++++++ src/service.rs | 6 +++--- src/service/auth.rs | 7 ++++++ src/service/rate_limit.rs | 12 ++++++++++- 6 files changed, 79 insertions(+), 30 deletions(-) diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs index 50993c6..400f3cd 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/proposal_context.rs @@ -1,6 +1,6 @@ use crate::action_set_index::ActionSetIndex; use crate::filter::proposal_context::no_implicit_dep::{EndRequestOperation, HeadersOperation}; -use crate::service::HeaderResolver; +use crate::service::{GrpcRequest, HeaderResolver}; use log::warn; use proxy_wasm::hostcalls; use proxy_wasm::traits::{Context, HttpContext}; @@ -19,6 +19,7 @@ pub mod no_implicit_dep { AwaitGrpcResponse(GrpcMessageReceiverOperation), AddHeaders(HeadersOperation), Die(EndRequestOperation), + Done(), } pub struct GrpcMessageSenderOperation { @@ -28,21 +29,19 @@ pub mod no_implicit_dep { impl GrpcMessageSenderOperation { // todo(adam-cattermole): unwrap.. - pub fn progress(self) -> (GrpcRequest, PendingOperation) { - let action_set = self - .runtime_action_set - .runtime_actions - .get(self.current_index) - .unwrap(); - let msg = action_set.create_message(); + pub fn progress(self) -> (Option, PendingOperation) { + let (index, msg) = self.runtime_action_set.process(self.current_index); + match msg { + None => (None, PendingOperation::Done()), + Some(msg) => ( + Some(msg), + PendingOperation::AwaitGrpcResponse(GrpcMessageReceiverOperation { + runtime_action_set: self.runtime_action_set, + current_index: index, + }), + ), + } // todo(adam-cattermole): should this instead return only a GrpcReceiver? failure? - ( - msg, - PendingOperation::AwaitGrpcResponse(GrpcMessageReceiverOperation { - runtime_action_set: self.runtime_action_set, - current_index: self.current_index, - }), - ) } } @@ -121,9 +120,12 @@ impl Filter { match operation { no_implicit_dep::PendingOperation::SendGrpcRequest(sender_op) => { let (msg, op) = sender_op.progress(); - match self.send_grpc_request(msg) { - Ok(_token) => self.handle_operation(op), - Err(_status) => {} + match msg { + None => panic!("invalid state!"), + Some(m) => match self.send_grpc_request(m) { + Ok(_token) => self.handle_operation(op), + Err(_status) => {} + }, } } no_implicit_dep::PendingOperation::AwaitGrpcResponse(receiver_op) => { @@ -133,6 +135,7 @@ impl Filter { self.headers_operations.push(header_op) } no_implicit_dep::PendingOperation::Die(die_op) => self.die(die_op), + no_implicit_dep::PendingOperation::Done() => (), } } diff --git a/src/runtime_action.rs b/src/runtime_action.rs index 60ae201..a9b7e7a 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -2,7 +2,12 @@ use crate::auth_action::AuthAction; use crate::configuration::{Action, FailureMode, Service, ServiceType}; use crate::filter::proposal_context::no_implicit_dep::PendingOperation; use crate::ratelimit_action::RateLimitAction; +use crate::service::auth::AuthService; +use crate::service::grpc_message::GrpcMessageRequest; +use crate::service::rate_limit::RateLimitService; use crate::service::GrpcService; +use log::{debug, warn}; +use protobuf::{Message, ProtobufResult}; use std::collections::HashMap; use std::rc::Rc; use std::time::Duration; @@ -65,17 +70,31 @@ impl RuntimeAction { Some(other) } - pub fn create_message(&self) -> crate::service::GrpcRequest { - self.grpc_service().build_request(None) - } - - pub fn process(&self) -> Option { + pub fn process(&self) -> Option { if !self.conditions_apply() { None } else { - // if provided message return what? - // if no message we assume we're a sender?????????? - todo!() + Some(self.grpc_service().build_request(self.build_message())) + } + } + + pub fn build_message(&self) -> Option> { + match self { + RuntimeAction::RateLimit(rl_action) => { + let descriptor = rl_action.build_descriptor(); + if descriptor.entries.is_empty() { + debug!("grpc_message_request: empty descriptors"); + None + } else { + RateLimitService::request_message_as_bytes( + String::from(rl_action.scope()), + vec![descriptor].into(), + ) + } + } + RuntimeAction::Auth(auth_action) => { + AuthService::request_message_as_bytes(String::from(auth_action.scope())) + } } } } diff --git a/src/runtime_action_set.rs b/src/runtime_action_set.rs index c6e986b..ee30543 100644 --- a/src/runtime_action_set.rs +++ b/src/runtime_action_set.rs @@ -1,6 +1,7 @@ use crate::configuration::{ActionSet, Service}; use crate::data::Predicate; use crate::runtime_action::RuntimeAction; +use crate::service::GrpcRequest; use log::error; use std::collections::HashMap; use std::rc::Rc; @@ -71,6 +72,15 @@ impl RuntimeActionSet { pub fn start_flow(&self) -> crate::filter::proposal_context::no_implicit_dep::PendingOperation { todo!("implement me!") } + + pub fn process(&self, start: usize) -> (usize, Option) { + for (i, action_set) in self.runtime_actions.iter().skip(start).enumerate() { + if let Some(msg) = action_set.process() { + return (start + i, Some(msg)); + } + } + (start, None) + } } #[cfg(test)] diff --git a/src/service.rs b/src/service.rs index b6925b3..cedbb4d 100644 --- a/src/service.rs +++ b/src/service.rs @@ -63,7 +63,7 @@ impl GrpcService { fn method(&self) -> &str { self.method } - pub fn build_request(&self, message: Option<&[u8]>) -> GrpcRequest { + pub fn build_request(&self, message: Option>) -> GrpcRequest { GrpcRequest::new( self.endpoint(), self.name(), @@ -128,14 +128,14 @@ impl GrpcRequest { service_name: &str, method_name: &str, timeout: Duration, - message: Option<&[u8]>, + message: Option>, ) -> Self { Self { upstream_name: upstream_name.to_owned(), service_name: service_name.to_owned(), method_name: method_name.to_owned(), timeout, - message: message.map(|m| m.to_vec()), + message, } } diff --git a/src/service/auth.rs b/src/service/auth.rs index 925c476..1980136 100644 --- a/src/service/auth.rs +++ b/src/service/auth.rs @@ -25,6 +25,13 @@ impl AuthService { AuthService::build_check_req(ce_host) } + pub fn request_message_as_bytes(ce_host: String) -> Option> { + Self::request_message(ce_host) + .write_to_bytes() + .map_err(|e| debug!("Failed to write protobuf message to bytes: {e:?}")) + .ok() + } + pub fn response_message(res_body_bytes: &Bytes) -> GrpcMessageResult { match Message::parse_from_bytes(res_body_bytes) { Ok(res) => Ok(GrpcMessageResponse::Auth(res)), diff --git a/src/service/rate_limit.rs b/src/service/rate_limit.rs index b4dcf80..68b7542 100644 --- a/src/service/rate_limit.rs +++ b/src/service/rate_limit.rs @@ -4,7 +4,7 @@ use crate::envoy::{ }; use crate::service::grpc_message::{GrpcMessageResponse, GrpcMessageResult}; use crate::service::{GrpcResult, GrpcService}; -use log::warn; +use log::{debug, warn}; use protobuf::{Message, RepeatedField}; use proxy_wasm::hostcalls; use proxy_wasm::types::Bytes; @@ -28,6 +28,16 @@ impl RateLimitService { } } + pub fn request_message_as_bytes( + domain: String, + descriptors: RepeatedField, + ) -> Option> { + Self::request_message(domain, descriptors) + .write_to_bytes() + .map_err(|e| debug!("Failed to write protobuf message to bytes: {e:?}")) + .ok() + } + pub fn response_message(res_body_bytes: &Bytes) -> GrpcMessageResult { match Message::parse_from_bytes(res_body_bytes) { Ok(res) => Ok(GrpcMessageResponse::RateLimit(res)), From 0a13e469628141bfb2e71d9eaf069a828c65694e Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Thu, 12 Dec 2024 13:11:14 +0000 Subject: [PATCH 04/21] Add trait for Vec apply Signed-off-by: Adam Cattermole --- src/auth_action.rs | 10 +--------- src/data/cel.rs | 19 ++++++++++++++++++- src/runtime_action_set.rs | 10 +--------- 3 files changed, 20 insertions(+), 19 deletions(-) diff --git a/src/auth_action.rs b/src/auth_action.rs index 94af576..6ac1cbb 100644 --- a/src/auth_action.rs +++ b/src/auth_action.rs @@ -34,15 +34,7 @@ impl AuthAction { } pub fn conditions_apply(&self) -> bool { - let predicates = &self.predicates; - predicates.is_empty() - || predicates.iter().all(|predicate| match predicate.test() { - Ok(b) => b, - Err(err) => { - error!("Failed to evaluate {:?}: {}", predicate, err); - panic!("Err out of this!") - } - }) + self.predicates.apply() } pub fn get_failure_mode(&self) -> FailureMode { diff --git a/src/data/cel.rs b/src/data/cel.rs index 6ef6302..66d80f2 100644 --- a/src/data/cel.rs +++ b/src/data/cel.rs @@ -7,7 +7,7 @@ use cel_parser::{parse, Expression as CelExpression, Member, ParseError}; use chrono::{DateTime, FixedOffset}; #[cfg(feature = "debug-host-behaviour")] use log::debug; -use log::warn; +use log::{error, warn}; use proxy_wasm::types::{Bytes, Status}; use serde_json::Value as JsonValue; use std::borrow::Cow; @@ -235,6 +235,23 @@ impl Predicate { } } +pub trait PredicateVec { + fn apply(&self) -> bool; +} + +impl PredicateVec for Vec { + fn apply(&self) -> bool { + self.is_empty() + || self.iter().all(|predicate| match predicate.test() { + Ok(b) => b, + Err(err) => { + error!("Failed to evaluate {:?}: {}", predicate, err); + panic!("Err out of this!") + } + }) + } +} + pub struct Attribute { path: Path, cel_type: Option, diff --git a/src/runtime_action_set.rs b/src/runtime_action_set.rs index ee30543..67b3220 100644 --- a/src/runtime_action_set.rs +++ b/src/runtime_action_set.rs @@ -58,15 +58,7 @@ impl RuntimeActionSet { } pub fn conditions_apply(&self) -> bool { - let predicates = &self.route_rule_predicates; - predicates.is_empty() - || predicates.iter().all(|predicate| match predicate.test() { - Ok(b) => b, - Err(err) => { - error!("Failed to evaluate {:?}: {}", predicate, err); - panic!("Err out of this!") - } - }) + self.route_rule_predicates.apply() } pub fn start_flow(&self) -> crate::filter::proposal_context::no_implicit_dep::PendingOperation { From 97e5f5ddd153da508fb20a56f862336ed5d6303d Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Thu, 12 Dec 2024 16:53:02 +0000 Subject: [PATCH 05/21] Iterate on state machine Signed-off-by: Adam Cattermole --- src/auth_action.rs | 64 ++++++- src/data/mod.rs | 1 + src/envoy/mod.rs | 5 +- src/filter.rs | 2 +- src/filter/http_context.rs | 5 +- src/filter/proposal_context.rs | 219 +++++++++++++++++++---- src/filter/root_context.rs | 22 ++- src/lib.rs | 2 + src/ratelimit_action.rs | 57 +++++- src/runtime_action.rs | 100 +++-------- src/runtime_action_set.rs | 11 +- src/runtime_config.rs | 32 ++-- src/service.rs | 18 +- utils/kustomize/limitador/limitador.yaml | 1 + 14 files changed, 362 insertions(+), 177 deletions(-) diff --git a/src/auth_action.rs b/src/auth_action.rs index 6ac1cbb..8fda05a 100644 --- a/src/auth_action.rs +++ b/src/auth_action.rs @@ -1,7 +1,12 @@ use crate::configuration::{Action, FailureMode, Service}; -use crate::data::Predicate; +use crate::data::{store_metadata, Predicate, PredicateVec}; +use crate::envoy::{CheckResponse, CheckResponse_oneof_http_response, HeaderValueOption}; +use crate::filter::proposal_context::no_implicit_dep::{ + EndRequestOperation, HeadersOperation, Operation, +}; use crate::service::GrpcService; -use log::error; +use log::debug; +use protobuf::Message; use std::rc::Rc; #[derive(Debug)] @@ -40,6 +45,61 @@ impl AuthAction { pub fn get_failure_mode(&self) -> FailureMode { self.grpc_service.get_failure_mode() } + + pub fn process_response(&self, check_response: CheckResponse) -> Operation { + //todo(adam-cattermole):error handling, ... + debug!("process_response: auth"); + + // store dynamic metadata in filter state + debug!("process_response: store_metadata"); + store_metadata(check_response.get_dynamic_metadata()); + + match check_response.http_response { + Some(CheckResponse_oneof_http_response::ok_response(ok_response)) => { + debug!("process_auth_grpc_response: received OkHttpResponse"); + if !ok_response.get_response_headers_to_add().is_empty() { + panic!("process_auth_grpc_response: response contained response_headers_to_add which is unsupported!") + } + if !ok_response.get_headers_to_remove().is_empty() { + panic!("process_auth_grpc_response: response contained headers_to_remove which is unsupported!") + } + if !ok_response.get_query_parameters_to_set().is_empty() { + panic!("process_auth_grpc_response: response contained query_parameters_to_set which is unsupported!") + } + if !ok_response.get_query_parameters_to_remove().is_empty() { + panic!("process_auth_grpc_response: response contained query_parameters_to_remove which is unsupported!") + } + + let response_headers = Self::get_header_vec(ok_response.get_headers()); + if !response_headers.is_empty() { + Operation::AddHeaders(HeadersOperation::new(response_headers)) + } else { + Operation::Done() + } + } + Some(CheckResponse_oneof_http_response::denied_response(denied_response)) => { + debug!("process_auth_grpc_response: received DeniedHttpResponse"); + let status_code = denied_response.get_status().code; + let response_headers = Self::get_header_vec(denied_response.get_headers()); + Operation::Die(EndRequestOperation::new( + status_code as u32, + response_headers, + Some(denied_response.body), + )) + } + None => Operation::Die(EndRequestOperation::default()), + } + } + + fn get_header_vec(headers: &[HeaderValueOption]) -> Vec<(String, String)> { + headers + .iter() + .map(|header| { + let hv = header.get_header(); + (hv.key.to_owned(), hv.value.to_owned()) + }) + .collect() + } } #[cfg(test)] diff --git a/src/data/mod.rs b/src/data/mod.rs index 9b7dd28..b780fe7 100644 --- a/src/data/mod.rs +++ b/src/data/mod.rs @@ -10,5 +10,6 @@ pub use cel::debug_all_well_known_attributes; pub use cel::Expression; pub use cel::Predicate; +pub use cel::PredicateVec; pub use property::Path as PropertyPath; diff --git a/src/envoy/mod.rs b/src/envoy/mod.rs index ddbcf0e..076512e 100644 --- a/src/envoy/mod.rs +++ b/src/envoy/mod.rs @@ -36,12 +36,9 @@ pub use { AttributeContext, AttributeContext_HttpRequest, AttributeContext_Peer, AttributeContext_Request, }, - base::Metadata, + base::{HeaderValue, HeaderValueOption, Metadata}, external_auth::{CheckRequest, CheckResponse, CheckResponse_oneof_http_response}, http_status::StatusCode, ratelimit::{RateLimitDescriptor, RateLimitDescriptor_Entry}, rls::{RateLimitRequest, RateLimitResponse, RateLimitResponse_Code}, }; - -#[cfg(test)] -pub use base::HeaderValue; diff --git a/src/filter.rs b/src/filter.rs index c0f0ce5..a4fc1da 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -35,7 +35,7 @@ extern "C" fn start() { info!("#{} set_root_context", context_id); Box::new(FilterRoot { context_id, - config: Default::default(), + action_set_index: Default::default(), }) }); } diff --git a/src/filter/http_context.rs b/src/filter/http_context.rs index 787df8e..f6180c1 100644 --- a/src/filter/http_context.rs +++ b/src/filter/http_context.rs @@ -1,9 +1,9 @@ +use crate::action_set_index::ActionSetIndex; use crate::configuration::FailureMode; #[cfg(feature = "debug-host-behaviour")] use crate::data; use crate::operation_dispatcher::{OperationDispatcher, OperationError}; use crate::runtime_action_set::RuntimeActionSet; -use crate::runtime_config::RuntimeConfig; use crate::service::GrpcService; use log::{debug, warn}; use proxy_wasm::traits::{Context, HttpContext}; @@ -13,7 +13,7 @@ use std::rc::Rc; pub struct Filter { pub context_id: u32, - pub config: Rc, + pub index: Rc, pub response_headers_to_add: Vec<(String, String)>, pub operation_dispatcher: RefCell, } @@ -107,7 +107,6 @@ impl HttpContext for Filter { data::debug_all_well_known_attributes(); match self - .config .index .get_longest_match_action_sets(self.request_authority().as_str()) { diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs index 400f3cd..b36443a 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/proposal_context.rs @@ -1,8 +1,9 @@ use crate::action_set_index::ActionSetIndex; -use crate::filter::proposal_context::no_implicit_dep::{EndRequestOperation, HeadersOperation}; +use crate::filter::proposal_context::no_implicit_dep::{ + EndRequestOperation, GrpcMessageSenderOperation, HeadersOperation, Operation, +}; use crate::service::{GrpcRequest, HeaderResolver}; -use log::warn; -use proxy_wasm::hostcalls; +use log::{debug, error, warn}; use proxy_wasm::traits::{Context, HttpContext}; use proxy_wasm::types::{Action, Status}; use std::mem; @@ -11,14 +12,18 @@ use std::rc::Rc; pub mod no_implicit_dep { use crate::runtime_action_set::RuntimeActionSet; use crate::service::GrpcRequest; + use log::error; + use std::cell::OnceCell; use std::rc::Rc; #[allow(dead_code)] - pub enum PendingOperation { + pub enum Operation { SendGrpcRequest(GrpcMessageSenderOperation), AwaitGrpcResponse(GrpcMessageReceiverOperation), AddHeaders(HeadersOperation), Die(EndRequestOperation), + //todo(adam-cattermole): does Done make sense? in this case no PendingOperation + // instead just Option? Done(), } @@ -28,20 +33,28 @@ pub mod no_implicit_dep { } impl GrpcMessageSenderOperation { - // todo(adam-cattermole): unwrap.. - pub fn progress(self) -> (Option, PendingOperation) { - let (index, msg) = self.runtime_action_set.process(self.current_index); + pub fn new(runtime_action_set: Rc, current_index: usize) -> Self { + Self { + runtime_action_set, + current_index, + } + } + + //todo(adam-cattermole): should this return a tuple? alternative? + pub fn next_grpc_request(self) -> (Option, Operation) { + let (index, msg) = self + .runtime_action_set + .find_next_grpc_request(self.current_index); match msg { - None => (None, PendingOperation::Done()), - Some(msg) => ( - Some(msg), - PendingOperation::AwaitGrpcResponse(GrpcMessageReceiverOperation { + None => (None, Operation::Done()), + Some(_) => ( + msg, + Operation::AwaitGrpcResponse(GrpcMessageReceiverOperation { runtime_action_set: self.runtime_action_set, current_index: index, }), ), } - // todo(adam-cattermole): should this instead return only a GrpcReceiver? failure? } } @@ -51,24 +64,121 @@ pub mod no_implicit_dep { } impl GrpcMessageReceiverOperation { - pub fn progress(self, _msg: &[u8]) -> PendingOperation { - todo!() + pub fn digest_grpc_response(self, msg: &[u8]) -> Operation { + let action = self + .runtime_action_set + .runtime_actions + .get(self.current_index) + .unwrap(); + + let next_op = action.process_response(msg); + match next_op { + Operation::AddHeaders(mut op) => { + op.set_action_set_index(self.runtime_action_set, self.current_index); + Operation::AddHeaders(op) + } + Operation::Done() => Operation::SendGrpcRequest(GrpcMessageSenderOperation::new( + self.runtime_action_set, + self.current_index + 1, + )), + _ => next_op, + } } - pub fn fail(self) -> PendingOperation { - PendingOperation::Die(EndRequestOperation { status: 500 }) + pub fn fail(self) -> Operation { + Operation::Die(EndRequestOperation::default()) } } - pub struct HeadersOperation {} + pub struct HeadersOperation { + headers: Vec<(String, String)>, + runtime_action_set: OnceCell>, + current_index: usize, + } + + impl HeadersOperation { + pub fn new(headers: Vec<(String, String)>) -> Self { + Self { + headers, + runtime_action_set: OnceCell::new(), + current_index: 0, + } + } + + pub fn set_action_set_index( + &mut self, + action_set_index: Rc, + index: usize, + ) { + match self.runtime_action_set.set(action_set_index) { + Ok(_) => self.current_index = index, + Err(_) => error!("Error setting action set index, already set"), + } + } + + pub fn progress(&self) -> Operation { + let next_op = match self.runtime_action_set.get() { + None => panic!("Invalid state, called progress without setting runtime action set"), + Some(runtime_action_set) => { + Operation::SendGrpcRequest(GrpcMessageSenderOperation::new( + Rc::clone(runtime_action_set), + self.current_index + 1, + )) + } + }; + next_op + } + + pub fn headers(self) -> Vec<(String, String)> { + self.headers + } + } pub struct EndRequestOperation { pub status: u32, + pub headers: Vec<(String, String)>, + pub body: Option, + } + + impl EndRequestOperation { + pub fn new(status: u32, headers: Vec<(String, String)>, body: Option) -> Self { + Self { + status, + headers, + body, + } + } + + pub fn new_with_status(status: u32) -> Self { + Self::new(status, Vec::default(), None) + } + + // todo(adam-cattermole): perhaps we should be more explicit with a different function? + // Default Die is with 500 Internal Server Error. + pub fn default() -> Self { + Self::new( + 500, + Vec::default(), + Some("Internal Server Error.\n".to_string()), + ) + } + + pub fn headers(&self) -> Vec<(&str, &str)> { + self.headers + .iter() + .map(|(header, value)| (header.as_str(), value.as_str())) + .collect() + } + + pub fn body(&self) -> Option<&[u8]> { + self.body.as_deref().map(|s| s.as_bytes()) + } } } -struct Filter { - index: ActionSetIndex, +pub(crate) struct Filter { + context_id: u32, + index: Rc, header_resolver: Rc, grpc_message_receiver_operation: Option, @@ -79,9 +189,9 @@ impl Context for Filter { fn on_grpc_call_response(&mut self, _token_id: u32, status_code: u32, resp_size: usize) { let receiver = mem::take(&mut self.grpc_message_receiver_operation) .expect("We need an operation pending a gRPC response"); - let next = if status_code != Status::Ok as u32 { + let next = if status_code == Status::Ok as u32 { match self.get_grpc_call_response_body(0, resp_size) { - Some(response_body) => receiver.progress(response_body.as_slice()), + Some(response_body) => receiver.digest_grpc_response(&response_body), None => receiver.fail(), } } else { @@ -101,46 +211,67 @@ impl HttpContext for Filter { .iter() .find(|action_set| action_set.conditions_apply(/* self */)) { - self.handle_operation(action_set.start_flow()) + let op = Operation::SendGrpcRequest(GrpcMessageSenderOperation::new( + Rc::clone(action_set), + 0, + )); + return self.handle_operation(op); } } Action::Continue } fn on_http_response_headers(&mut self, _num_headers: usize, _end_of_stream: bool) -> Action { - for _op in self.headers_operations.drain(..) { - todo!("Add the headers") + let headers_operations = mem::take(&mut self.headers_operations); + for op in headers_operations { + for (header, value) in &op.headers() { + self.add_http_response_header(header, value) + } } Action::Continue } } impl Filter { - fn handle_operation(&mut self, operation: no_implicit_dep::PendingOperation) { + fn handle_operation(&mut self, operation: Operation) -> Action { match operation { - no_implicit_dep::PendingOperation::SendGrpcRequest(sender_op) => { - let (msg, op) = sender_op.progress(); + Operation::SendGrpcRequest(sender_op) => { + debug!("handle_operation: SendGrpcRequest"); + let (msg, op) = sender_op.next_grpc_request(); match msg { - None => panic!("invalid state!"), + None => self.handle_operation(op), Some(m) => match self.send_grpc_request(m) { Ok(_token) => self.handle_operation(op), - Err(_status) => {} + Err(_status) => panic!("Error sending request"), }, } } - no_implicit_dep::PendingOperation::AwaitGrpcResponse(receiver_op) => { - self.grpc_message_receiver_operation = Some(receiver_op) + Operation::AwaitGrpcResponse(receiver_op) => { + debug!("handle_operation: AwaitGrpcResponse"); + self.grpc_message_receiver_operation = Some(receiver_op); + Action::Pause + } + Operation::AddHeaders(header_op) => { + debug!("handle_operation: AddHeaders"); + let next = header_op.progress(); + self.headers_operations.push(header_op); + self.handle_operation(next) } - no_implicit_dep::PendingOperation::AddHeaders(header_op) => { - self.headers_operations.push(header_op) + Operation::Die(die_op) => { + debug!("handle_operation: Die"); + self.die(die_op); + Action::Continue + } + Operation::Done() => { + debug!("handle_operation: Done"); + self.resume_http_request(); + Action::Continue } - no_implicit_dep::PendingOperation::Die(die_op) => self.die(die_op), - no_implicit_dep::PendingOperation::Done() => (), } } fn die(&mut self, die: EndRequestOperation) { - self.send_http_response(die.status, Vec::default(), None); + self.send_http_response(die.status, die.headers(), die.body()); } fn request_authority(&self) -> String { @@ -156,7 +287,7 @@ impl Filter { } } - fn send_grpc_request(&self, req: crate::service::GrpcRequest) -> Result { + fn send_grpc_request(&self, req: GrpcRequest) -> Result { let headers = self .header_resolver .get_with_ctx(self) @@ -173,4 +304,18 @@ impl Filter { req.timeout(), ) } + + pub fn new( + context_id: u32, + index: Rc, + header_resolver: Rc, + ) -> Self { + Self { + context_id, + index, + header_resolver, + grpc_message_receiver_operation: None, + headers_operations: Vec::default(), + } + } } diff --git a/src/filter/root_context.rs b/src/filter/root_context.rs index 4716387..f43a7ab 100644 --- a/src/filter/root_context.rs +++ b/src/filter/root_context.rs @@ -1,7 +1,6 @@ +use crate::action_set_index::ActionSetIndex; use crate::configuration::PluginConfiguration; -use crate::filter::http_context::Filter; -use crate::operation_dispatcher::OperationDispatcher; -use crate::runtime_config::RuntimeConfig; +use crate::filter::proposal_context::Filter; use crate::service::HeaderResolver; use const_format::formatcp; use log::{debug, error, info}; @@ -17,7 +16,7 @@ const WASM_SHIM_HEADER: &str = "Kuadrant wasm module"; pub struct FilterRoot { pub context_id: u32, - pub config: Rc, + pub action_set_index: Rc, } impl RootContext for FilterRoot { @@ -36,12 +35,11 @@ impl RootContext for FilterRoot { fn create_http_context(&self, context_id: u32) -> Option> { debug!("#{} create_http_context", context_id); let header_resolver = Rc::new(HeaderResolver::new()); - Some(Box::new(Filter { + Some(Box::new(Filter::new( context_id, - config: Rc::clone(&self.config), - response_headers_to_add: Vec::default(), - operation_dispatcher: OperationDispatcher::new(header_resolver).into(), - })) + Rc::clone(&self.action_set_index), + header_resolver, + ))) } fn on_configure(&mut self, _config_size: usize) -> bool { @@ -53,15 +51,15 @@ impl RootContext for FilterRoot { match serde_json::from_slice::(&configuration) { Ok(config) => { info!("plugin config parsed: {:?}", config); - let runtime_config = - match >::try_into(config) { + let action_set_index = + match >::try_into(config) { Ok(cfg) => cfg, Err(err) => { error!("failed to compile plugin config: {}", err); return false; } }; - self.config = Rc::new(runtime_config); + self.action_set_index = Rc::new(action_set_index); } Err(e) => { error!("failed to parse plugin config: {}", e); diff --git a/src/lib.rs b/src/lib.rs index 2062ebd..644b0da 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6,6 +6,8 @@ mod data; mod envoy; mod filter; mod glob; + +#[allow(dead_code)] mod operation_dispatcher; mod ratelimit_action; mod runtime_action; diff --git a/src/ratelimit_action.rs b/src/ratelimit_action.rs index d093396..73ad2fe 100644 --- a/src/ratelimit_action.rs +++ b/src/ratelimit_action.rs @@ -1,11 +1,17 @@ use crate::configuration::{Action, DataType, FailureMode, Service}; use crate::data::Expression; use crate::data::Predicate; -use crate::envoy::{RateLimitDescriptor, RateLimitDescriptor_Entry}; +use crate::envoy::{ + HeaderValue, RateLimitDescriptor, RateLimitDescriptor_Entry, RateLimitResponse, + RateLimitResponse_Code, StatusCode, +}; +use crate::filter::proposal_context::no_implicit_dep::{ + EndRequestOperation, HeadersOperation, Operation, +}; use crate::service::GrpcService; use cel_interpreter::Value; -use log::error; -use protobuf::RepeatedField; +use log::{debug, error}; +use protobuf::{Message, RepeatedField}; use std::rc::Rc; #[derive(Debug)] @@ -160,6 +166,51 @@ impl RateLimitAction { } Some(other) } + + pub fn process_response(&self, rate_limit_response: RateLimitResponse) -> Operation { + match rate_limit_response { + RateLimitResponse { + overall_code: RateLimitResponse_Code::UNKNOWN, + .. + } => { + debug!("process_response_rl: received UNKNOWN response"); + Operation::Die(EndRequestOperation::default()) + } + RateLimitResponse { + overall_code: RateLimitResponse_Code::OVER_LIMIT, + response_headers_to_add: rl_headers, + .. + } => { + debug!("process_response_rl: received OVER_LIMIT response"); + let response_headers = Self::get_header_vec(rl_headers); + Operation::Die(EndRequestOperation::new( + StatusCode::TooManyRequests as u32, + response_headers, + Some("Too Many Requests\n".to_string()), + )) + } + RateLimitResponse { + overall_code: RateLimitResponse_Code::OK, + response_headers_to_add: additional_headers, + .. + } => { + debug!("process_response_rl: received OK response"); + let response_headers = Self::get_header_vec(additional_headers); + if response_headers.is_empty() { + Operation::Done() + } else { + Operation::AddHeaders(HeadersOperation::new(response_headers)) + } + } + } + } + + fn get_header_vec(headers: RepeatedField) -> Vec<(String, String)> { + headers + .iter() + .map(|header| (header.key.to_owned(), header.value.to_owned())) + .collect() + } } #[cfg(test)] diff --git a/src/runtime_action.rs b/src/runtime_action.rs index a9b7e7a..c61a0c4 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -1,13 +1,13 @@ use crate::auth_action::AuthAction; use crate::configuration::{Action, FailureMode, Service, ServiceType}; -use crate::filter::proposal_context::no_implicit_dep::PendingOperation; +use crate::envoy::{CheckResponse, RateLimitResponse}; +use crate::filter::proposal_context::no_implicit_dep::Operation; use crate::ratelimit_action::RateLimitAction; use crate::service::auth::AuthService; -use crate::service::grpc_message::GrpcMessageRequest; use crate::service::rate_limit::RateLimitService; -use crate::service::GrpcService; -use log::{debug, warn}; -use protobuf::{Message, ProtobufResult}; +use crate::service::{GrpcRequest, GrpcService}; +use log::debug; +use protobuf::Message; use std::collections::HashMap; use std::rc::Rc; use std::time::Duration; @@ -70,11 +70,26 @@ impl RuntimeAction { Some(other) } - pub fn process(&self) -> Option { + pub fn process_request(&self) -> Option { if !self.conditions_apply() { None } else { - Some(self.grpc_service().build_request(self.build_message())) + self.grpc_service().build_request(self.build_message()) + } + } + + pub fn process_response(&self, msg: &[u8]) -> Operation { + match self { + Self::Auth(auth_action) => { + // todo(adam-cattermole):unwrap + let check_response: CheckResponse = Message::parse_from_bytes(msg).unwrap(); + auth_action.process_response(check_response) + } + Self::RateLimit(rl_action) => { + let rate_limit_response: RateLimitResponse = + Message::parse_from_bytes(msg).unwrap(); + rl_action.process_response(rate_limit_response) + } } } @@ -83,7 +98,7 @@ impl RuntimeAction { RuntimeAction::RateLimit(rl_action) => { let descriptor = rl_action.build_descriptor(); if descriptor.entries.is_empty() { - debug!("grpc_message_request: empty descriptors"); + debug!("build_message(rl): empty descriptors"); None } else { RateLimitService::request_message_as_bytes( @@ -104,75 +119,6 @@ mod test { use super::*; use crate::configuration::{Action, FailureMode, ServiceType, Timeout}; - pub enum Operation { - SendRequest(RequestSender), - ConsumeRequest(RequestConsumer), - } - type RequestSender = (); - // struct RequestSender {} - // impl RequestSender { - // - // } - - type RequestConsumer = (); - // struct RequestConsumer {} - // impl RequestConsumer { - // - // } - - #[test] - fn start_action_set_flow() { - let actions = vec![ - RuntimeAction::new(&build_action("ratelimit", "scope"), &HashMap::default()).unwrap(), - RuntimeAction::new(&build_action("ratelimit", "scope"), &HashMap::default()).unwrap(), - ]; - let mut iter = actions.iter(); - let a = iter.next().expect("get the first action"); - - // let op: Result, ()> = a.create_message(); // action.? - // let ret: RequestSender = match op { - // Ok(_) => unreachable!("should have failed"), - // Err(_) => match iter.next() { - // Some(b) => b.create_message().expect("Ok").expect("Some"), - // None => (), - // }, - // }; - - // this is caller code - - // on_http_request:find_action_set:start_flow - - // let (message_handler, req) = ret.create_request(); - // let token = send_request(req); - - // let (message, handler) = ret.create_request() // SendMessageOperation -> ReceiveMessageOperation - // how does this function look? - // does it take into account current action? - - // on_grpc_response - - // let response = message_handler.consume(response); - - /* bs - let next = action_set.progress(op); - action_set.actions[op.action_index].progress(op); - - struct Operation { - current: RuntimeAction, - next: Option - } - */ - } - - /* Overall - - We have Operation that transitions between different states passing the ref to ActionSet - to subsequent actions as well as an index - - The action_set has either a start_flow function, or maybe just process? - + this iterates over the actions to find the next one - - The runtime_action has the ability to create a message? - - */ - fn build_rl_service() -> Service { Service { service_type: ServiceType::RateLimit, diff --git a/src/runtime_action_set.rs b/src/runtime_action_set.rs index 67b3220..e707b7a 100644 --- a/src/runtime_action_set.rs +++ b/src/runtime_action_set.rs @@ -1,8 +1,7 @@ use crate::configuration::{ActionSet, Service}; -use crate::data::Predicate; +use crate::data::{Predicate, PredicateVec}; use crate::runtime_action::RuntimeAction; use crate::service::GrpcRequest; -use log::error; use std::collections::HashMap; use std::rc::Rc; @@ -61,13 +60,9 @@ impl RuntimeActionSet { self.route_rule_predicates.apply() } - pub fn start_flow(&self) -> crate::filter::proposal_context::no_implicit_dep::PendingOperation { - todo!("implement me!") - } - - pub fn process(&self, start: usize) -> (usize, Option) { + pub fn find_next_grpc_request(&self, start: usize) -> (usize, Option) { for (i, action_set) in self.runtime_actions.iter().skip(start).enumerate() { - if let Some(msg) = action_set.process() { + if let Some(msg) = action_set.process_request() { return (start + i, Some(msg)); } } diff --git a/src/runtime_config.rs b/src/runtime_config.rs index 25c63cb..58db3a9 100644 --- a/src/runtime_config.rs +++ b/src/runtime_config.rs @@ -3,11 +3,7 @@ use crate::configuration::PluginConfiguration; use crate::runtime_action_set::RuntimeActionSet; use std::rc::Rc; -pub(crate) struct RuntimeConfig { - pub index: ActionSetIndex, -} - -impl TryFrom for RuntimeConfig { +impl TryFrom for ActionSetIndex { type Error = String; fn try_from(config: PluginConfiguration) -> Result { @@ -19,15 +15,13 @@ impl TryFrom for RuntimeConfig { } } - Ok(Self { index }) + Ok(index) } } -impl Default for RuntimeConfig { +impl Default for ActionSetIndex { fn default() -> Self { - Self { - index: ActionSetIndex::new(), - } + ActionSetIndex::new() } } @@ -97,21 +91,15 @@ mod test { } assert!(res.is_ok()); - let result = RuntimeConfig::try_from(res.unwrap()); - let runtime_config = result.expect("That didn't work"); - let rlp_option = runtime_config - .index - .get_longest_match_action_sets("example.com"); + let result = ActionSetIndex::try_from(res.unwrap()); + let index = result.expect("That didn't work"); + let rlp_option = index.get_longest_match_action_sets("example.com"); assert!(rlp_option.is_some()); - let rlp_option = runtime_config - .index - .get_longest_match_action_sets("test.toystore.com"); + let rlp_option = index.get_longest_match_action_sets("test.toystore.com"); assert!(rlp_option.is_some()); - let rlp_option = runtime_config - .index - .get_longest_match_action_sets("unknown"); + let rlp_option = index.get_longest_match_action_sets("unknown"); assert!(rlp_option.is_none()); } @@ -151,7 +139,7 @@ mod test { } assert!(serde_res.is_ok()); - let result = RuntimeConfig::try_from(serde_res.expect("That didn't work")); + let result = ActionSetIndex::try_from(serde_res.expect("That didn't work")); assert_eq!(result.err(), Some("Unknown service: unknown".into())); } } diff --git a/src/service.rs b/src/service.rs index cedbb4d..3251a9c 100644 --- a/src/service.rs +++ b/src/service.rs @@ -63,14 +63,16 @@ impl GrpcService { fn method(&self) -> &str { self.method } - pub fn build_request(&self, message: Option>) -> GrpcRequest { - GrpcRequest::new( - self.endpoint(), - self.name(), - self.method(), - self.get_timeout(), - message, - ) + pub fn build_request(&self, message: Option>) -> Option { + message.map(|msg| { + GrpcRequest::new( + self.endpoint(), + self.name(), + self.method(), + self.get_timeout(), + Some(msg), + ) + }) } pub fn process_grpc_response( diff --git a/utils/kustomize/limitador/limitador.yaml b/utils/kustomize/limitador/limitador.yaml index 9382312..523b28d 100644 --- a/utils/kustomize/limitador/limitador.yaml +++ b/utils/kustomize/limitador/limitador.yaml @@ -4,6 +4,7 @@ kind: Limitador metadata: name: limitador spec: + image: quay.io/kuadrant/limitador:v1.6.0 verbosity: 3 listener: http: From a44c6387748b986d7eac604d8e399f7a7121c9d2 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Mon, 13 Jan 2025 13:14:12 +0000 Subject: [PATCH 06/21] Redesign process request for the auth action Signed-off-by: Adam Cattermole --- src/auth_action.rs | 224 ++++++++++++++++++++++++++++++++++++------ src/envoy/mod.rs | 6 ++ src/runtime_action.rs | 14 ++- src/service.rs | 41 ++++++++ 4 files changed, 252 insertions(+), 33 deletions(-) diff --git a/src/auth_action.rs b/src/auth_action.rs index 8fda05a..b3f1737 100644 --- a/src/auth_action.rs +++ b/src/auth_action.rs @@ -1,10 +1,7 @@ use crate::configuration::{Action, FailureMode, Service}; use crate::data::{store_metadata, Predicate, PredicateVec}; use crate::envoy::{CheckResponse, CheckResponse_oneof_http_response, HeaderValueOption}; -use crate::filter::proposal_context::no_implicit_dep::{ - EndRequestOperation, HeadersOperation, Operation, -}; -use crate::service::GrpcService; +use crate::service::{GrpcErrResponse, GrpcService}; use log::debug; use protobuf::Message; use std::rc::Rc; @@ -46,48 +43,60 @@ impl AuthAction { self.grpc_service.get_failure_mode() } - pub fn process_response(&self, check_response: CheckResponse) -> Operation { - //todo(adam-cattermole):error handling, ... - debug!("process_response: auth"); - + pub fn process_response( + &self, + check_response: CheckResponse, + ) -> Result>, GrpcErrResponse> { + //todo(adam-cattermole):hostvar resolver... // store dynamic metadata in filter state - debug!("process_response: store_metadata"); - store_metadata(check_response.get_dynamic_metadata()); + debug!("process_response(auth): store_metadata"); + //todo(adam-cattermole): COMMENTED OUT FOR TESTS TO COMPILE + // store_metadata(check_response.get_dynamic_metadata()); match check_response.http_response { + None => { + debug!("process_response(auth): received no http_response"); + match self.get_failure_mode() { + FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()), + FailureMode::Allow => { + debug!("process_response(auth): continuing as FailureMode Allow"); + Ok(None) + } + } + } + Some(CheckResponse_oneof_http_response::denied_response(denied_response)) => { + debug!("process_response(auth): received DeniedHttpResponse"); + let status_code = denied_response.get_status().get_code(); + let response_headers = Self::get_header_vec(denied_response.get_headers()); + Err(GrpcErrResponse::new( + status_code as u32, + response_headers, + denied_response.body, + )) + } Some(CheckResponse_oneof_http_response::ok_response(ok_response)) => { - debug!("process_auth_grpc_response: received OkHttpResponse"); + debug!("process_response(auth): received OkHttpResponse"); + if !ok_response.get_response_headers_to_add().is_empty() { - panic!("process_auth_grpc_response: response contained response_headers_to_add which is unsupported!") + panic!("process_response(auth): response contained response_headers_to_add which is unsupported!") } if !ok_response.get_headers_to_remove().is_empty() { - panic!("process_auth_grpc_response: response contained headers_to_remove which is unsupported!") + panic!("process_response(auth): response contained headers_to_remove which is unsupported!") } if !ok_response.get_query_parameters_to_set().is_empty() { - panic!("process_auth_grpc_response: response contained query_parameters_to_set which is unsupported!") + panic!("process_response(auth): response contained query_parameters_to_set which is unsupported!") } if !ok_response.get_query_parameters_to_remove().is_empty() { - panic!("process_auth_grpc_response: response contained query_parameters_to_remove which is unsupported!") + panic!("process_response(auth): response contained query_parameters_to_remove which is unsupported!") } let response_headers = Self::get_header_vec(ok_response.get_headers()); - if !response_headers.is_empty() { - Operation::AddHeaders(HeadersOperation::new(response_headers)) + if response_headers.is_empty() { + Ok(None) } else { - Operation::Done() + Ok(Some(response_headers)) } } - Some(CheckResponse_oneof_http_response::denied_response(denied_response)) => { - debug!("process_auth_grpc_response: received DeniedHttpResponse"); - let status_code = denied_response.get_status().code; - let response_headers = Self::get_header_vec(denied_response.get_headers()); - Operation::Die(EndRequestOperation::new( - status_code as u32, - response_headers, - Some(denied_response.body), - )) - } - None => Operation::Die(EndRequestOperation::default()), } } @@ -106,8 +115,17 @@ impl AuthAction { mod test { use super::*; use crate::configuration::{Action, FailureMode, Service, ServiceType, Timeout}; + use crate::envoy::{DeniedHttpResponse, HeaderValue, HttpStatus, OkHttpResponse, StatusCode}; + use protobuf::RepeatedField; fn build_auth_action_with_predicates(predicates: Vec) -> AuthAction { + build_auth_action_with_predicates_and_failure_mode(predicates, FailureMode::default()) + } + + fn build_auth_action_with_predicates_and_failure_mode( + predicates: Vec, + failure_mode: FailureMode, + ) -> AuthAction { let action = Action { service: "some_service".into(), scope: "some_scope".into(), @@ -118,7 +136,7 @@ mod test { let service = Service { service_type: ServiceType::Auth, endpoint: "some_endpoint".into(), - failure_mode: FailureMode::default(), + failure_mode, timeout: Timeout::default(), }; @@ -126,6 +144,56 @@ mod test { .expect("action building failed. Maybe predicates compilation?") } + fn build_check_response( + status: StatusCode, + headers: Option>, + body: Option, + ) -> CheckResponse { + let mut response = CheckResponse::new(); + match status { + StatusCode::OK => { + let mut ok_http_response = OkHttpResponse::new(); + if let Some(header_list) = headers { + ok_http_response.set_headers(build_headers(header_list)) + } + response.set_ok_response(ok_http_response); + } + StatusCode::Forbidden => { + let mut http_status = HttpStatus::new(); + http_status.set_code(status); + + let mut denied_http_response = DeniedHttpResponse::new(); + denied_http_response.set_status(http_status); + if let Some(header_list) = headers { + denied_http_response.set_headers(build_headers(header_list)); + } + denied_http_response.set_body(body.unwrap_or_default()); + response.set_denied_response(denied_http_response); + } + _ => { + // assume any other code is for error state + } + }; + response + } + + fn build_headers(headers: Vec<(&str, &str)>) -> RepeatedField { + headers + .into_iter() + .map(|(key, value)| { + let header_value = { + let mut hv = HeaderValue::new(); + hv.set_key(key.to_string()); + hv.set_value(value.to_string()); + hv + }; + let mut header_option = HeaderValueOption::new(); + header_option.set_header(header_value); + header_option + }) + .collect::>() + } + #[test] fn empty_predicates_do_apply() { let auth_action = build_auth_action_with_predicates(Vec::default()); @@ -160,4 +228,100 @@ mod test { ]); auth_action.conditions_apply(); } + + #[test] + fn process_ok_response() { + let auth_action = build_auth_action_with_predicates(Vec::default()); + let ok_response_without_headers = build_check_response(StatusCode::OK, None, None); + let result = auth_action.process_response(ok_response_without_headers); + assert!(result.is_ok()); + + let headers = result.expect("is ok"); + assert!(headers.is_none()); + + let ok_response_with_header = + build_check_response(StatusCode::OK, Some(vec![("my_header", "my_value")]), None); + let result = auth_action.process_response(ok_response_with_header); + assert!(result.is_ok()); + + let headers = result.expect("is ok"); + assert!(headers.is_some()); + + let header_vec = headers.expect("is some"); + assert_eq!( + header_vec[0], + ("my_header".to_string(), "my_value".to_string()) + ); + } + + #[test] + fn process_denied_response() { + let headers = vec![ + ("www-authenticate", "APIKEY realm=\"api-key-users\""), + ("x-ext-auth-reason", "credential not found"), + ]; + let auth_action = build_auth_action_with_predicates(Vec::default()); + let denied_response_empty = build_check_response(StatusCode::Forbidden, None, None); + let result = auth_action.process_response(denied_response_empty); + assert!(result.is_err()); + + let grpc_err_response = result.expect_err("is err"); + assert_eq!( + grpc_err_response.status_code(), + StatusCode::Forbidden as u32 + ); + assert!(grpc_err_response.headers().is_empty()); + assert_eq!(grpc_err_response.body(), String::default()); + + let denied_response_content = build_check_response( + StatusCode::Forbidden, + Some(headers.clone()), + Some("my_body".to_string()), + ); + let result = auth_action.process_response(denied_response_content); + assert!(result.is_err()); + + let grpc_err_response = result.expect_err("is err"); + assert_eq!( + grpc_err_response.status_code(), + StatusCode::Forbidden as u32 + ); + + let response_headers = grpc_err_response.headers(); + headers.iter().zip(response_headers.iter()).for_each( + |((header_one, value_one), (header_two, value_two))| { + assert_eq!(header_one, header_two); + assert_eq!(value_one, value_two); + }, + ); + + assert_eq!(grpc_err_response.body(), "my_body"); + } + + #[test] + fn process_error_response() { + let auth_action = + build_auth_action_with_predicates_and_failure_mode(Vec::default(), FailureMode::Deny); + let error_response = build_check_response(StatusCode::InternalServerError, None, None); + let result = auth_action.process_response(error_response); + assert!(result.is_err()); + + let grpc_err_response = result.expect_err("is err"); + assert_eq!( + grpc_err_response.status_code(), + StatusCode::InternalServerError as u32 + ); + + assert!(grpc_err_response.headers().is_empty()); + assert_eq!(grpc_err_response.body(), "Internal Server Error.\n"); + + let auth_action = + build_auth_action_with_predicates_and_failure_mode(Vec::default(), FailureMode::Allow); + let error_response = build_check_response(StatusCode::InternalServerError, None, None); + let result = auth_action.process_response(error_response); + assert!(result.is_ok()); + + let headers = result.expect("is ok"); + assert!(headers.is_none()); + } } diff --git a/src/envoy/mod.rs b/src/envoy/mod.rs index 076512e..8dd169e 100644 --- a/src/envoy/mod.rs +++ b/src/envoy/mod.rs @@ -42,3 +42,9 @@ pub use { ratelimit::{RateLimitDescriptor, RateLimitDescriptor_Entry}, rls::{RateLimitRequest, RateLimitResponse, RateLimitResponse_Code}, }; + +#[cfg(test)] +pub use { + external_auth::{DeniedHttpResponse, OkHttpResponse}, + http_status::HttpStatus, +}; diff --git a/src/runtime_action.rs b/src/runtime_action.rs index c61a0c4..20e61d7 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -1,11 +1,11 @@ use crate::auth_action::AuthAction; use crate::configuration::{Action, FailureMode, Service, ServiceType}; use crate::envoy::{CheckResponse, RateLimitResponse}; -use crate::filter::proposal_context::no_implicit_dep::Operation; +use crate::filter::proposal_context::no_implicit_dep::{HeadersOperation, Operation}; use crate::ratelimit_action::RateLimitAction; use crate::service::auth::AuthService; use crate::service::rate_limit::RateLimitService; -use crate::service::{GrpcRequest, GrpcService}; +use crate::service::{GrpcErrResponse, GrpcRequest, GrpcService}; use log::debug; use protobuf::Message; use std::collections::HashMap; @@ -79,7 +79,7 @@ impl RuntimeAction { } pub fn process_response(&self, msg: &[u8]) -> Operation { - match self { + let result = match self { Self::Auth(auth_action) => { // todo(adam-cattermole):unwrap let check_response: CheckResponse = Message::parse_from_bytes(msg).unwrap(); @@ -90,6 +90,14 @@ impl RuntimeAction { Message::parse_from_bytes(msg).unwrap(); rl_action.process_response(rate_limit_response) } + }; + + match result { + Ok(headers) => match headers { + None => Operation::Done(), + Some(h) => Operation::AddHeaders(HeadersOperation::new(h)), + }, + Err(grpc_err_response) => Operation::Die(grpc_err_response), } } diff --git a/src/service.rs b/src/service.rs index 3251a9c..cf7a613 100644 --- a/src/service.rs +++ b/src/service.rs @@ -116,6 +116,7 @@ impl GrpcService { } } +// GrpcRequest contains the information required to make a Grpc Call pub struct GrpcRequest { upstream_name: String, service_name: String, @@ -162,6 +163,46 @@ impl GrpcRequest { } } +#[derive(Debug)] +pub struct GrpcErrResponse { + status_code: u32, + response_headers: Vec<(String, String)>, + body: String, +} + +impl GrpcErrResponse { + pub fn new(status_code: u32, response_headers: Vec<(String, String)>, body: String) -> Self { + Self { + status_code, + response_headers, + body, + } + } + + pub fn new_internal_server_error() -> Self { + Self { + status_code: StatusCode::InternalServerError as u32, + response_headers: Vec::default(), + body: "Internal Server Error.\n".to_string(), + } + } + + pub fn status_code(&self) -> u32 { + self.status_code + } + + pub fn headers(&self) -> Vec<(&str, &str)> { + self.response_headers + .iter() + .map(|(header, value)| (header.as_str(), value.as_str())) + .collect() + } + + pub fn body(&self) -> &str { + self.body.as_str() + } +} + pub struct GrpcResult { pub response_headers: Vec<(String, String)>, } From 1ab6bf770556a9b17e85b474891d1e24ac6e19e9 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Mon, 13 Jan 2025 13:15:52 +0000 Subject: [PATCH 07/21] Redesign process request for the ratelimit action Signed-off-by: Adam Cattermole --- src/filter/proposal_context.rs | 16 +-- src/ratelimit_action.rs | 174 ++++++++++++++++++++++++++++++--- 2 files changed, 170 insertions(+), 20 deletions(-) diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs index b36443a..b6a80bc 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/proposal_context.rs @@ -2,7 +2,7 @@ use crate::action_set_index::ActionSetIndex; use crate::filter::proposal_context::no_implicit_dep::{ EndRequestOperation, GrpcMessageSenderOperation, HeadersOperation, Operation, }; -use crate::service::{GrpcRequest, HeaderResolver}; +use crate::service::{GrpcErrResponse, GrpcRequest, HeaderResolver}; use log::{debug, error, warn}; use proxy_wasm::traits::{Context, HttpContext}; use proxy_wasm::types::{Action, Status}; @@ -11,7 +11,7 @@ use std::rc::Rc; pub mod no_implicit_dep { use crate::runtime_action_set::RuntimeActionSet; - use crate::service::GrpcRequest; + use crate::service::{GrpcErrResponse, GrpcRequest}; use log::error; use std::cell::OnceCell; use std::rc::Rc; @@ -21,7 +21,7 @@ pub mod no_implicit_dep { SendGrpcRequest(GrpcMessageSenderOperation), AwaitGrpcResponse(GrpcMessageReceiverOperation), AddHeaders(HeadersOperation), - Die(EndRequestOperation), + Die(GrpcErrResponse), //todo(adam-cattermole): does Done make sense? in this case no PendingOperation // instead just Option? Done(), @@ -86,7 +86,7 @@ pub mod no_implicit_dep { } pub fn fail(self) -> Operation { - Operation::Die(EndRequestOperation::default()) + Operation::Die(GrpcErrResponse::new_internal_server_error()) } } @@ -270,8 +270,12 @@ impl Filter { } } - fn die(&mut self, die: EndRequestOperation) { - self.send_http_response(die.status, die.headers(), die.body()); + fn die(&mut self, die: GrpcErrResponse) { + self.send_http_response( + die.status_code(), + die.headers(), + Some(die.body().as_bytes()), + ); } fn request_authority(&self) -> String { diff --git a/src/ratelimit_action.rs b/src/ratelimit_action.rs index 73ad2fe..a892fac 100644 --- a/src/ratelimit_action.rs +++ b/src/ratelimit_action.rs @@ -5,10 +5,7 @@ use crate::envoy::{ HeaderValue, RateLimitDescriptor, RateLimitDescriptor_Entry, RateLimitResponse, RateLimitResponse_Code, StatusCode, }; -use crate::filter::proposal_context::no_implicit_dep::{ - EndRequestOperation, HeadersOperation, Operation, -}; -use crate::service::GrpcService; +use crate::service::{GrpcErrResponse, GrpcService}; use cel_interpreter::Value; use log::{debug, error}; use protobuf::{Message, RepeatedField}; @@ -167,26 +164,35 @@ impl RateLimitAction { Some(other) } - pub fn process_response(&self, rate_limit_response: RateLimitResponse) -> Operation { + pub fn process_response( + &self, + rate_limit_response: RateLimitResponse, + ) -> Result>, GrpcErrResponse> { match rate_limit_response { RateLimitResponse { overall_code: RateLimitResponse_Code::UNKNOWN, .. } => { - debug!("process_response_rl: received UNKNOWN response"); - Operation::Die(EndRequestOperation::default()) + debug!("process_response(rl): received UNKNOWN response"); + match self.get_failure_mode() { + FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()), + FailureMode::Allow => { + debug!("process_response(rl): continuing as FailureMode Allow"); + Ok(None) + } + } } RateLimitResponse { overall_code: RateLimitResponse_Code::OVER_LIMIT, response_headers_to_add: rl_headers, .. } => { - debug!("process_response_rl: received OVER_LIMIT response"); + debug!("process_response(rl): received OVER_LIMIT response"); let response_headers = Self::get_header_vec(rl_headers); - Operation::Die(EndRequestOperation::new( + Err(GrpcErrResponse::new( StatusCode::TooManyRequests as u32, response_headers, - Some("Too Many Requests\n".to_string()), + "Too Many Requests\n".to_string(), )) } RateLimitResponse { @@ -194,12 +200,12 @@ impl RateLimitAction { response_headers_to_add: additional_headers, .. } => { - debug!("process_response_rl: received OK response"); + debug!("process_response(rl): received OK response"); let response_headers = Self::get_header_vec(additional_headers); if response_headers.is_empty() { - Operation::Done() + Ok(None) } else { - Operation::AddHeaders(HeadersOperation::new(response_headers)) + Ok(Some(response_headers)) } } } @@ -220,12 +226,17 @@ mod test { Action, DataItem, DataType, ExpressionItem, FailureMode, Service, ServiceType, StaticItem, Timeout, }; + use crate::envoy::HeaderValueOption; fn build_service() -> Service { + build_service_with_failure_mode(FailureMode::default()) + } + + fn build_service_with_failure_mode(failure_mode: FailureMode) -> Service { Service { service_type: ServiceType::RateLimit, endpoint: "some_endpoint".into(), - failure_mode: FailureMode::default(), + failure_mode, timeout: Timeout::default(), } } @@ -239,6 +250,35 @@ mod test { } } + fn build_ratelimit_response( + status: RateLimitResponse_Code, + headers: Option>, + ) -> RateLimitResponse { + let mut response = RateLimitResponse::new(); + response.set_overall_code(status); + match status { + RateLimitResponse_Code::UNKNOWN => {} + RateLimitResponse_Code::OVER_LIMIT | RateLimitResponse_Code::OK => { + if let Some(header_list) = headers { + response.set_response_headers_to_add(build_headers(header_list)) + } + } + } + response + } + + fn build_headers(headers: Vec<(&str, &str)>) -> RepeatedField { + headers + .into_iter() + .map(|(key, value)| { + let mut hv = HeaderValue::new(); + hv.set_key(key.to_string()); + hv.set_value(value.to_string()); + hv + }) + .collect::>() + } + #[test] fn empty_predicates_do_apply() { let action = build_action(Vec::default(), Vec::default()); @@ -368,4 +408,110 @@ mod test { assert_eq!(descriptor.get_entries()[1].key, String::from("key_3")); assert_eq!(descriptor.get_entries()[1].value, String::from("value_3")); } + + #[test] + fn process_ok_response() { + let action = build_action(Vec::default(), Vec::default()); + let service = build_service(); + let rl_action = RateLimitAction::new(&action, &service) + .expect("action building failed. Maybe predicates compilation?"); + + let ok_response_without_headers = + build_ratelimit_response(RateLimitResponse_Code::OK, None); + let result = rl_action.process_response(ok_response_without_headers); + assert!(result.is_ok()); + + let headers = result.expect("is ok"); + assert!(headers.is_none()); + + let ok_response_with_header = build_ratelimit_response( + RateLimitResponse_Code::OK, + Some(vec![("my_header", "my_value")]), + ); + let result = rl_action.process_response(ok_response_with_header); + assert!(result.is_ok()); + + let headers = result.expect("is ok"); + assert!(headers.is_some()); + + let header_vec = headers.expect("is some"); + assert_eq!( + header_vec[0], + ("my_header".to_string(), "my_value".to_string()) + ); + } + + #[test] + fn process_overlimit_response() { + let headers = vec![("x-ratelimit-limit", "10"), ("x-ratelimit-remaining", "0")]; + let action = build_action(Vec::default(), Vec::default()); + let service = build_service(); + let rl_action = RateLimitAction::new(&action, &service) + .expect("action building failed. Maybe predicates compilation?"); + + let overlimit_response_empty = + build_ratelimit_response(RateLimitResponse_Code::OVER_LIMIT, None); + let result = rl_action.process_response(overlimit_response_empty); + assert!(result.is_err()); + + let grpc_err_response = result.expect_err("is err"); + assert_eq!( + grpc_err_response.status_code(), + StatusCode::TooManyRequests as u32 + ); + assert!(grpc_err_response.headers().is_empty()); + assert_eq!(grpc_err_response.body(), "Too Many Requests\n"); + + let denied_response_headers = + build_ratelimit_response(RateLimitResponse_Code::OVER_LIMIT, Some(headers.clone())); + let result = rl_action.process_response(denied_response_headers); + assert!(result.is_err()); + + let grpc_err_response = result.expect_err("is err"); + assert_eq!( + grpc_err_response.status_code(), + StatusCode::TooManyRequests as u32 + ); + + let response_headers = grpc_err_response.headers(); + headers.iter().zip(response_headers.iter()).for_each( + |((header_one, value_one), (header_two, value_two))| { + assert_eq!(header_one, header_two); + assert_eq!(value_one, value_two); + }, + ); + + assert_eq!(grpc_err_response.body(), "Too Many Requests\n"); + } + + #[test] + fn process_error_response() { + let action = build_action(Vec::default(), Vec::default()); + let deny_service = build_service_with_failure_mode(FailureMode::Deny); + let rl_action = RateLimitAction::new(&action, &deny_service) + .expect("action building failed. Maybe predicates compilation?"); + + let error_response = build_ratelimit_response(RateLimitResponse_Code::UNKNOWN, None); + let result = rl_action.process_response(error_response.clone()); + assert!(result.is_err()); + + let grpc_err_response = result.expect_err("is err"); + assert_eq!( + grpc_err_response.status_code(), + StatusCode::InternalServerError as u32 + ); + + assert!(grpc_err_response.headers().is_empty()); + assert_eq!(grpc_err_response.body(), "Internal Server Error.\n"); + + let allow_service = build_service_with_failure_mode(FailureMode::Allow); + let rl_action = RateLimitAction::new(&action, &allow_service) + .expect("action building failed. Maybe predicates compilation?"); + + let result = rl_action.process_response(error_response); + assert!(result.is_ok()); + + let headers = result.expect("is ok"); + assert!(headers.is_none()); + } } From 5e16dfb922714f51199be56695a9c2bddabbae3c Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Mon, 13 Jan 2025 16:05:07 +0000 Subject: [PATCH 08/21] Start to utilise the abstractions from actions Signed-off-by: Adam Cattermole --- src/auth_action.rs | 3 +- src/filter/proposal_context.rs | 218 +++++++++++---------------------- src/ratelimit_action.rs | 2 +- src/runtime_action.rs | 23 ++-- src/runtime_action_set.rs | 36 ++++-- src/service.rs | 19 +++ 6 files changed, 130 insertions(+), 171 deletions(-) diff --git a/src/auth_action.rs b/src/auth_action.rs index b3f1737..5ad6e99 100644 --- a/src/auth_action.rs +++ b/src/auth_action.rs @@ -3,7 +3,6 @@ use crate::data::{store_metadata, Predicate, PredicateVec}; use crate::envoy::{CheckResponse, CheckResponse_oneof_http_response, HeaderValueOption}; use crate::service::{GrpcErrResponse, GrpcService}; use log::debug; -use protobuf::Message; use std::rc::Rc; #[derive(Debug)] @@ -51,7 +50,7 @@ impl AuthAction { // store dynamic metadata in filter state debug!("process_response(auth): store_metadata"); //todo(adam-cattermole): COMMENTED OUT FOR TESTS TO COMPILE - // store_metadata(check_response.get_dynamic_metadata()); + store_metadata(check_response.get_dynamic_metadata()); match check_response.http_response { None => { diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs index b6a80bc..29a21e6 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/proposal_context.rs @@ -1,9 +1,10 @@ use crate::action_set_index::ActionSetIndex; use crate::filter::proposal_context::no_implicit_dep::{ - EndRequestOperation, GrpcMessageSenderOperation, HeadersOperation, Operation, + GrpcMessageReceiverOperation, HeadersOperation, Operation, }; -use crate::service::{GrpcErrResponse, GrpcRequest, HeaderResolver}; -use log::{debug, error, warn}; +use crate::runtime_action_set::RuntimeActionSet; +use crate::service::{GrpcErrResponse, GrpcRequestAction, HeaderResolver}; +use log::{debug, warn}; use proxy_wasm::traits::{Context, HttpContext}; use proxy_wasm::types::{Action, Status}; use std::mem; @@ -11,14 +12,11 @@ use std::rc::Rc; pub mod no_implicit_dep { use crate::runtime_action_set::RuntimeActionSet; - use crate::service::{GrpcErrResponse, GrpcRequest}; - use log::error; - use std::cell::OnceCell; + use crate::service::{GrpcErrResponse, GrpcRequestAction}; use std::rc::Rc; #[allow(dead_code)] pub enum Operation { - SendGrpcRequest(GrpcMessageSenderOperation), AwaitGrpcResponse(GrpcMessageReceiverOperation), AddHeaders(HeadersOperation), Die(GrpcErrResponse), @@ -27,12 +25,12 @@ pub mod no_implicit_dep { Done(), } - pub struct GrpcMessageSenderOperation { + pub struct GrpcMessageReceiverOperation { runtime_action_set: Rc, current_index: usize, } - impl GrpcMessageSenderOperation { + impl GrpcMessageReceiverOperation { pub fn new(runtime_action_set: Rc, current_index: usize) -> Self { Self { runtime_action_set, @@ -40,51 +38,28 @@ pub mod no_implicit_dep { } } - //todo(adam-cattermole): should this return a tuple? alternative? - pub fn next_grpc_request(self) -> (Option, Operation) { - let (index, msg) = self + pub fn digest_grpc_response( + self, + msg: &[u8], + ) -> Result<(Option, Option), Operation> { + let result = self .runtime_action_set - .find_next_grpc_request(self.current_index); - match msg { - None => (None, Operation::Done()), - Some(_) => ( - msg, - Operation::AwaitGrpcResponse(GrpcMessageReceiverOperation { - runtime_action_set: self.runtime_action_set, - current_index: index, - }), - ), - } - } - } + .process_grpc_response(self.current_index, msg); - pub struct GrpcMessageReceiverOperation { - runtime_action_set: Rc, - current_index: usize, - } - - impl GrpcMessageReceiverOperation { - pub fn digest_grpc_response(self, msg: &[u8]) -> Operation { - let action = self - .runtime_action_set - .runtime_actions - .get(self.current_index) - .unwrap(); - - let next_op = action.process_response(msg); - match next_op { - Operation::AddHeaders(mut op) => { - op.set_action_set_index(self.runtime_action_set, self.current_index); - Operation::AddHeaders(op) + match result { + Ok((next_msg, headers)) => { + let header_op = + headers.map(|hs| Operation::AddHeaders(HeadersOperation::new(hs))); + Ok((next_msg, header_op)) } - Operation::Done() => Operation::SendGrpcRequest(GrpcMessageSenderOperation::new( - self.runtime_action_set, - self.current_index + 1, - )), - _ => next_op, + Err(grpc_err_resp) => Err(Operation::Die(grpc_err_resp)), } } + pub fn runtime_action_set(&self) -> Rc { + Rc::clone(&self.runtime_action_set) + } + pub fn fail(self) -> Operation { Operation::Die(GrpcErrResponse::new_internal_server_error()) } @@ -92,88 +67,17 @@ pub mod no_implicit_dep { pub struct HeadersOperation { headers: Vec<(String, String)>, - runtime_action_set: OnceCell>, - current_index: usize, } impl HeadersOperation { pub fn new(headers: Vec<(String, String)>) -> Self { - Self { - headers, - runtime_action_set: OnceCell::new(), - current_index: 0, - } - } - - pub fn set_action_set_index( - &mut self, - action_set_index: Rc, - index: usize, - ) { - match self.runtime_action_set.set(action_set_index) { - Ok(_) => self.current_index = index, - Err(_) => error!("Error setting action set index, already set"), - } - } - - pub fn progress(&self) -> Operation { - let next_op = match self.runtime_action_set.get() { - None => panic!("Invalid state, called progress without setting runtime action set"), - Some(runtime_action_set) => { - Operation::SendGrpcRequest(GrpcMessageSenderOperation::new( - Rc::clone(runtime_action_set), - self.current_index + 1, - )) - } - }; - next_op + Self { headers } } pub fn headers(self) -> Vec<(String, String)> { self.headers } } - - pub struct EndRequestOperation { - pub status: u32, - pub headers: Vec<(String, String)>, - pub body: Option, - } - - impl EndRequestOperation { - pub fn new(status: u32, headers: Vec<(String, String)>, body: Option) -> Self { - Self { - status, - headers, - body, - } - } - - pub fn new_with_status(status: u32) -> Self { - Self::new(status, Vec::default(), None) - } - - // todo(adam-cattermole): perhaps we should be more explicit with a different function? - // Default Die is with 500 Internal Server Error. - pub fn default() -> Self { - Self::new( - 500, - Vec::default(), - Some("Internal Server Error.\n".to_string()), - ) - } - - pub fn headers(&self) -> Vec<(&str, &str)> { - self.headers - .iter() - .map(|(header, value)| (header.as_str(), value.as_str())) - .collect() - } - - pub fn body(&self) -> Option<&[u8]> { - self.body.as_deref().map(|s| s.as_bytes()) - } - } } pub(crate) struct Filter { @@ -189,15 +93,34 @@ impl Context for Filter { fn on_grpc_call_response(&mut self, _token_id: u32, status_code: u32, resp_size: usize) { let receiver = mem::take(&mut self.grpc_message_receiver_operation) .expect("We need an operation pending a gRPC response"); - let next = if status_code == Status::Ok as u32 { - match self.get_grpc_call_response_body(0, resp_size) { - Some(response_body) => receiver.digest_grpc_response(&response_body), - None => receiver.fail(), + let action_set = receiver.runtime_action_set(); + + if status_code != Status::Ok as u32 { + self.handle_operation(receiver.fail()); + return; + } + + let response_body = match self.get_grpc_call_response_body(0, resp_size) { + Some(body) => body, + None => { + self.handle_operation(receiver.fail()); + return; } - } else { - receiver.fail() }; - self.handle_operation(next); + + let result = receiver.digest_grpc_response(&response_body); + match result { + Ok((next_msg, header_op)) => { + if let Some(header_op) = header_op { + self.handle_operation(header_op); + } + let receiver_op = self.handle_next_message(action_set, next_msg); + self.handle_operation(receiver_op); + } + Err(die_op) => { + self.handle_operation(die_op); + } + } } } @@ -211,11 +134,9 @@ impl HttpContext for Filter { .iter() .find(|action_set| action_set.conditions_apply(/* self */)) { - let op = Operation::SendGrpcRequest(GrpcMessageSenderOperation::new( - Rc::clone(action_set), - 0, - )); - return self.handle_operation(op); + let request_action = action_set.start_flow(); + let next_op = self.handle_next_message(Rc::clone(action_set), request_action); + return self.handle_operation(next_op); } } Action::Continue @@ -235,17 +156,6 @@ impl HttpContext for Filter { impl Filter { fn handle_operation(&mut self, operation: Operation) -> Action { match operation { - Operation::SendGrpcRequest(sender_op) => { - debug!("handle_operation: SendGrpcRequest"); - let (msg, op) = sender_op.next_grpc_request(); - match msg { - None => self.handle_operation(op), - Some(m) => match self.send_grpc_request(m) { - Ok(_token) => self.handle_operation(op), - Err(_status) => panic!("Error sending request"), - }, - } - } Operation::AwaitGrpcResponse(receiver_op) => { debug!("handle_operation: AwaitGrpcResponse"); self.grpc_message_receiver_operation = Some(receiver_op); @@ -253,9 +163,8 @@ impl Filter { } Operation::AddHeaders(header_op) => { debug!("handle_operation: AddHeaders"); - let next = header_op.progress(); self.headers_operations.push(header_op); - self.handle_operation(next) + Action::Continue } Operation::Die(die_op) => { debug!("handle_operation: Die"); @@ -270,6 +179,23 @@ impl Filter { } } + fn handle_next_message( + &mut self, + action_set: Rc, + next_msg: Option, + ) -> Operation { + match next_msg { + Some(msg) => match self.send_grpc_request(&msg) { + Ok(_token) => Operation::AwaitGrpcResponse(GrpcMessageReceiverOperation::new( + action_set, + msg.index(), + )), + Err(_status) => panic!("Error sending request"), + }, + None => Operation::Done(), + } + } + fn die(&mut self, die: GrpcErrResponse) { self.send_http_response( die.status_code(), @@ -291,7 +217,7 @@ impl Filter { } } - fn send_grpc_request(&self, req: GrpcRequest) -> Result { + fn send_grpc_request(&self, req: &GrpcRequestAction) -> Result { let headers = self .header_resolver .get_with_ctx(self) diff --git a/src/ratelimit_action.rs b/src/ratelimit_action.rs index a892fac..699f4be 100644 --- a/src/ratelimit_action.rs +++ b/src/ratelimit_action.rs @@ -8,7 +8,7 @@ use crate::envoy::{ use crate::service::{GrpcErrResponse, GrpcService}; use cel_interpreter::Value; use log::{debug, error}; -use protobuf::{Message, RepeatedField}; +use protobuf::RepeatedField; use std::rc::Rc; #[derive(Debug)] diff --git a/src/runtime_action.rs b/src/runtime_action.rs index 20e61d7..bfaa725 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -1,7 +1,6 @@ use crate::auth_action::AuthAction; use crate::configuration::{Action, FailureMode, Service, ServiceType}; use crate::envoy::{CheckResponse, RateLimitResponse}; -use crate::filter::proposal_context::no_implicit_dep::{HeadersOperation, Operation}; use crate::ratelimit_action::RateLimitAction; use crate::service::auth::AuthService; use crate::service::rate_limit::RateLimitService; @@ -78,26 +77,22 @@ impl RuntimeAction { } } - pub fn process_response(&self, msg: &[u8]) -> Operation { - let result = match self { + pub fn process_response( + &self, + msg: &[u8], + ) -> Result>, GrpcErrResponse> { + match self { Self::Auth(auth_action) => { - // todo(adam-cattermole):unwrap - let check_response: CheckResponse = Message::parse_from_bytes(msg).unwrap(); + // todo(adam-cattermole): should this expect be here? + let check_response: CheckResponse = + Message::parse_from_bytes(msg).expect("invalid state!"); auth_action.process_response(check_response) } Self::RateLimit(rl_action) => { let rate_limit_response: RateLimitResponse = - Message::parse_from_bytes(msg).unwrap(); + Message::parse_from_bytes(msg).expect("invalid state!"); rl_action.process_response(rate_limit_response) } - }; - - match result { - Ok(headers) => match headers { - None => Operation::Done(), - Some(h) => Operation::AddHeaders(HeadersOperation::new(h)), - }, - Err(grpc_err_response) => Operation::Die(grpc_err_response), } } diff --git a/src/runtime_action_set.rs b/src/runtime_action_set.rs index e707b7a..944b8f4 100644 --- a/src/runtime_action_set.rs +++ b/src/runtime_action_set.rs @@ -1,7 +1,7 @@ use crate::configuration::{ActionSet, Service}; use crate::data::{Predicate, PredicateVec}; use crate::runtime_action::RuntimeAction; -use crate::service::GrpcRequest; +use crate::service::{GrpcErrResponse, IndexedGrpcRequest}; use std::collections::HashMap; use std::rc::Rc; @@ -60,13 +60,33 @@ impl RuntimeActionSet { self.route_rule_predicates.apply() } - pub fn find_next_grpc_request(&self, start: usize) -> (usize, Option) { - for (i, action_set) in self.runtime_actions.iter().skip(start).enumerate() { - if let Some(msg) = action_set.process_request() { - return (start + i, Some(msg)); - } - } - (start, None) + pub fn start_flow(&self) -> Option { + self.find_next_grpc_request(0) + } + + fn find_next_grpc_request(&self, start: usize) -> Option { + self.runtime_actions + .iter() + .skip(start) + .enumerate() + .find_map(|(i, action)| { + action + .process_request() + .map(|request| IndexedGrpcRequest::new(start + i, request)) + }) + } + + pub fn process_grpc_response( + &self, + index: usize, + msg: &[u8], + ) -> Result<(Option, Option>), GrpcErrResponse> { + self.runtime_actions[index] + .process_response(msg) + .map(|headers| { + let next_msg = self.find_next_grpc_request(index + 1); + (next_msg, headers) + }) } } diff --git a/src/service.rs b/src/service.rs index cf7a613..dd6f5a1 100644 --- a/src/service.rs +++ b/src/service.rs @@ -116,6 +116,25 @@ impl GrpcService { } } +pub struct IndexedGrpcRequest { + index: usize, + request: GrpcRequest, +} + +impl IndexedGrpcRequest { + pub(crate) fn new(index: usize, request: GrpcRequest) -> Self { + Self { index, request } + } + + pub fn index(&self) -> usize { + self.index + } + + pub fn request(self) -> GrpcRequest { + self.request + } +} + // GrpcRequest contains the information required to make a Grpc Call pub struct GrpcRequest { upstream_name: String, From badd6323f631d34a4460a020d4814951f09e7fb8 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Tue, 14 Jan 2025 11:40:33 +0000 Subject: [PATCH 09/21] Remove all redundant code Signed-off-by: Adam Cattermole --- src/filter.rs | 2 - src/filter/http_context.rs | 175 ------------ src/lib.rs | 3 - src/operation_dispatcher.rs | 548 ------------------------------------ src/runtime_action.rs | 9 - src/service.rs | 142 +--------- src/service/auth.rs | 82 +----- src/service/grpc_message.rs | 269 ------------------ src/service/rate_limit.rs | 64 +---- 9 files changed, 9 insertions(+), 1285 deletions(-) delete mode 100644 src/filter/http_context.rs delete mode 100644 src/operation_dispatcher.rs delete mode 100644 src/service/grpc_message.rs diff --git a/src/filter.rs b/src/filter.rs index a4fc1da..2bb7557 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -1,5 +1,3 @@ -pub(crate) mod http_context; -#[allow(dead_code)] pub(crate) mod proposal_context; mod root_context; diff --git a/src/filter/http_context.rs b/src/filter/http_context.rs deleted file mode 100644 index f6180c1..0000000 --- a/src/filter/http_context.rs +++ /dev/null @@ -1,175 +0,0 @@ -use crate::action_set_index::ActionSetIndex; -use crate::configuration::FailureMode; -#[cfg(feature = "debug-host-behaviour")] -use crate::data; -use crate::operation_dispatcher::{OperationDispatcher, OperationError}; -use crate::runtime_action_set::RuntimeActionSet; -use crate::service::GrpcService; -use log::{debug, warn}; -use proxy_wasm::traits::{Context, HttpContext}; -use proxy_wasm::types::Action; -use std::cell::RefCell; -use std::rc::Rc; - -pub struct Filter { - pub context_id: u32, - pub index: Rc, - pub response_headers_to_add: Vec<(String, String)>, - pub operation_dispatcher: RefCell, -} - -impl Filter { - fn request_authority(&self) -> String { - match self.get_http_request_header(":authority") { - None => { - warn!(":authority header not found"); - String::new() - } - Some(host) => { - let split_host = host.split(':').collect::>(); - split_host[0].to_owned() - } - } - } - - #[allow(unknown_lints, clippy::manual_inspect)] - fn process_action_sets(&self, m_set_list: &[Rc]) -> Action { - if let Some(m_set) = m_set_list.iter().find(|m_set| m_set.conditions_apply()) { - debug!("#{} action_set selected {}", self.context_id, m_set.name); - //debug!("#{} runtime action_set {:#?}", self.context_id, m_set); - self.operation_dispatcher - .borrow_mut() - .build_operations(&m_set.runtime_actions) - } else { - debug!( - "#{} process_action_sets: no action_set with conditions applies", - self.context_id - ); - return Action::Continue; - } - - match self.operation_dispatcher.borrow_mut().next() { - Ok(Some(op)) => match op.get_result() { - Ok(call_id) => { - debug!("#{} initiated gRPC call (id# {})", self.context_id, call_id); - Action::Pause - } - Err(e) => { - warn!("gRPC call failed! {e:?}"); - if let FailureMode::Deny = op.get_failure_mode() { - self.send_http_response(500, vec![], Some(b"Internal Server Error.\n")) - } - Action::Continue - } - }, - Ok(None) => { - Action::Continue // No operations left to perform - } - Err(OperationError { - failure_mode: FailureMode::Deny, - status, - }) => { - warn!("OperationError Status: {status:?}"); - self.send_http_response(500, vec![], Some(b"Internal Server Error.\n")); - Action::Continue - } - Err(OperationError { - failure_mode: FailureMode::Allow, - status, - }) => { - warn!("OperationError Status: {status:?}"); - Action::Continue - } - } - } - - fn process_next_op(&self) { - match self.operation_dispatcher.borrow_mut().next() { - Ok(some_op) => { - if some_op.is_none() { - // No more operations left in queue, resuming - self.resume_http_request(); - } - } - Err(op_err) => { - // If desired, we could check the error status. - GrpcService::handle_error_on_grpc_response(op_err.failure_mode); - } - } - } -} - -impl HttpContext for Filter { - fn on_http_request_headers(&mut self, _: usize, _: bool) -> Action { - debug!("#{} on_http_request_headers", self.context_id); - - #[cfg(feature = "debug-host-behaviour")] - data::debug_all_well_known_attributes(); - - match self - .index - .get_longest_match_action_sets(self.request_authority().as_str()) - { - None => { - debug!( - "#{} allowing request to pass because zero descriptors generated", - self.context_id - ); - Action::Continue - } - Some(m_sets) => self.process_action_sets(m_sets), - } - } - - fn on_http_response_headers(&mut self, _num_headers: usize, _end_of_stream: bool) -> Action { - debug!("#{} on_http_response_headers", self.context_id); - for (name, value) in &self.response_headers_to_add { - self.add_http_response_header(name, value); - } - Action::Continue - } - - fn on_log(&mut self) { - debug!("#{} completed.", self.context_id); - } -} - -impl Context for Filter { - fn on_grpc_call_response(&mut self, token_id: u32, status_code: u32, resp_size: usize) { - debug!( - "#{} on_grpc_call_response: received gRPC call response: token: {token_id}, status: {status_code}", - self.context_id - ); - - let op_res = self - .operation_dispatcher - .borrow() - .get_waiting_operation(token_id); - - match op_res { - Ok(operation) => { - match GrpcService::process_grpc_response(Rc::clone(&operation), resp_size) { - Ok(result) => { - // add the response headers - self.response_headers_to_add.extend(result.response_headers); - // call the next op - self.process_next_op(); - } - Err(_) => { - match operation.get_failure_mode() { - FailureMode::Deny => {} - FailureMode::Allow => { - // call the next op - self.process_next_op(); - } - } - } - } - } - Err(e) => { - warn!("No Operation found with token_id: {token_id}"); - GrpcService::handle_error_on_grpc_response(e.failure_mode); - } - } - } -} diff --git a/src/lib.rs b/src/lib.rs index 644b0da..7033a14 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -6,9 +6,6 @@ mod data; mod envoy; mod filter; mod glob; - -#[allow(dead_code)] -mod operation_dispatcher; mod ratelimit_action; mod runtime_action; mod runtime_action_set; diff --git a/src/operation_dispatcher.rs b/src/operation_dispatcher.rs deleted file mode 100644 index 156b08f..0000000 --- a/src/operation_dispatcher.rs +++ /dev/null @@ -1,548 +0,0 @@ -use crate::configuration::{FailureMode, ServiceType}; -use crate::runtime_action::RuntimeAction; -use crate::service::grpc_message::GrpcMessageRequest; -use crate::service::{ - GetMapValuesBytesFn, GrpcCallFn, GrpcMessageBuildFn, GrpcServiceHandler, HeaderResolver, -}; -use log::{debug, error}; -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; - -#[derive(PartialEq, Debug, Clone, Copy)] -pub(crate) enum State { - Pending, - Waiting, - Done, -} - -impl State { - fn next(&mut self) { - match self { - State::Pending => *self = State::Waiting, - State::Waiting => *self = State::Done, - _ => {} - } - } - - fn done(&mut self) { - *self = State::Done - } -} - -#[derive(Debug)] -pub(crate) struct Operation { - state: RefCell, - result: RefCell>, - action: Rc, - service_handler: GrpcServiceHandler, - grpc_call_fn: GrpcCallFn, - get_map_values_bytes_fn: GetMapValuesBytesFn, - grpc_message_build_fn: GrpcMessageBuildFn, - conditions_apply_fn: ConditionsApplyFn, -} - -impl Operation { - pub fn new(action: Rc, service_handler: GrpcServiceHandler) -> Self { - Self { - state: RefCell::new(State::Pending), - result: RefCell::new(Ok(0)), // Heuristics: zero represents that it's not been triggered, following `hostcalls` example - action, - service_handler, - grpc_call_fn, - get_map_values_bytes_fn, - grpc_message_build_fn, - conditions_apply_fn, - } - } - - fn trigger(&self) -> Result { - if let Some(message) = (self.grpc_message_build_fn)(&self.action) { - let res = self.service_handler.send( - self.get_map_values_bytes_fn, - self.grpc_call_fn, - message, - self.action.get_timeout(), - ); - 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(); - self.get_result() - } else { - self.done(); - self.get_result() - } - } - - fn next_state(&self) { - self.state.borrow_mut().next() - } - - fn done(&self) { - self.state.borrow_mut().done() - } - - pub fn get_state(&self) -> State { - *self.state.borrow() - } - - pub fn get_result(&self) -> Result { - *self.result.borrow() - } - - fn set_result(&self, result: Result) { - *self.result.borrow_mut() = result; - } - - pub fn get_service_type(&self) -> ServiceType { - self.action.get_service_type() - } - - pub fn get_failure_mode(&self) -> FailureMode { - self.action.get_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::ParseFailure => { - write!(f, "Error parsing configuration.") - } - _ => { - write!(f, "Error triggering the operation. {:?}", self.status) - } - } - } -} - -pub struct OperationDispatcher { - operations: Vec>, - waiting_operations: HashMap>, - header_resolver: Rc, -} - -impl OperationDispatcher { - pub fn new(header_resolver: Rc) -> Self { - Self { - operations: vec![], - waiting_operations: HashMap::new(), - header_resolver: Rc::clone(&header_resolver), - } - } - - pub fn get_waiting_operation(&self, token_id: u32) -> Result, OperationError> { - let op = self.waiting_operations.get(&token_id); - match op { - Some(op) => { - op.next_state(); - Ok(op.clone()) - } - None => Err(OperationError::new( - Status::NotFound, - FailureMode::default(), - )), - } - } - - pub fn build_operations(&mut self, actions: &[Rc]) { - let mut operations: Vec> = vec![]; - for action in actions.iter() { - operations.push(Rc::new(Operation::new( - Rc::clone(action), - GrpcServiceHandler::new(action.grpc_service(), Rc::clone(&self.header_resolver)), - ))); - } - self.push_operations(operations); - } - - pub fn push_operations(&mut self, operations: Vec>) { - self.operations.extend(operations); - } - - pub fn next(&mut self) -> Result>, OperationError> { - if let Some((i, operation)) = self.operations.iter_mut().enumerate().next() { - match operation.get_state() { - State::Pending => { - if (operation.conditions_apply_fn)(&operation.action) { - match operation.trigger() { - Ok(token_id) => { - match operation.get_state() { - State::Pending => { - panic!("Operation dispatcher reached an undefined state"); - } - State::Waiting => { - // We index only if it was just transitioned to Waiting after triggering - self.waiting_operations.insert(token_id, operation.clone()); - // TODO(didierofrivia): Decide on indexing the failed operations. - Ok(Some(operation.clone())) - } - State::Done => self.next(), - } - } - Err(err) => { - error!("{err:?}"); - Err(err) - } - } - } else { - debug!("actions conditions do not apply, skipping"); - self.operations.remove(i); - self.next() - } - } - State::Waiting => { - operation.next_state(); - Ok(Some(operation.clone())) - } - State::Done => { - if let Ok(token_id) = operation.get_result() { - self.waiting_operations.remove(&token_id); - } // If result was Err, means the operation wasn't indexed - self.operations.remove(i); - self.next() - } - } - } else { - Ok(None) - } - } - - #[cfg(test)] - pub fn default() -> Self { - OperationDispatcher { - operations: vec![], - waiting_operations: HashMap::default(), - header_resolver: Rc::new(HeaderResolver::default()), - } - } - - #[cfg(test)] - pub fn get_current_operation_state(&self) -> Option { - self.operations - .first() - .map(|operation| operation.get_state()) - } -} - -fn grpc_call_fn( - upstream_name: &str, - service_name: &str, - method_name: &str, - initial_metadata: Vec<(&str, &[u8])>, - message: Option<&[u8]>, - timeout: Duration, -) -> Result { - hostcalls::dispatch_grpc_call( - upstream_name, - service_name, - method_name, - initial_metadata, - message, - timeout, - ) -} - -fn get_map_values_bytes_fn(map_type: MapType, key: &str) -> Result, Status> { - hostcalls::get_map_value_bytes(map_type, key) -} - -fn grpc_message_build_fn(action: &RuntimeAction) -> Option { - GrpcMessageRequest::new(action) -} - -type ConditionsApplyFn = fn(action: &RuntimeAction) -> bool; - -fn conditions_apply_fn(action: &RuntimeAction) -> bool { - action.conditions_apply() -} - -#[cfg(test)] -mod tests { - use super::*; - use crate::auth_action::AuthAction; - use crate::configuration::{Action, Service, Timeout}; - use crate::envoy::RateLimitRequest; - use crate::ratelimit_action::RateLimitAction; - use protobuf::RepeatedField; - use std::rc::Rc; - use std::time::Duration; - - fn default_grpc_call_fn_stub( - _upstream_name: &str, - _service_name: &str, - _method_name: &str, - _initial_metadata: Vec<(&str, &[u8])>, - _message: Option<&[u8]>, - _timeout: Duration, - ) -> Result { - Ok(200) - } - - fn get_map_values_bytes_fn_stub( - _map_type: MapType, - _key: &str, - ) -> Result, Status> { - Ok(Some(Vec::new())) - } - - fn grpc_message_build_fn_stub(_action: &RuntimeAction) -> Option { - Some(GrpcMessageRequest::RateLimit(build_message())) - } - - fn build_grpc_service_handler() -> GrpcServiceHandler { - GrpcServiceHandler::new(Rc::new(Default::default()), Default::default()) - } - - fn conditions_apply_fn_stub(_action: &RuntimeAction) -> bool { - true - } - - fn build_message() -> RateLimitRequest { - RateLimitRequest { - domain: "example.org".to_string(), - descriptors: RepeatedField::new(), - hits_addend: 1, - unknown_fields: Default::default(), - cached_size: Default::default(), - } - } - - fn build_auth_grpc_action() -> RuntimeAction { - let service = Service { - service_type: ServiceType::Auth, - endpoint: "local".to_string(), - failure_mode: FailureMode::Deny, - timeout: Timeout(Duration::from_millis(42)), - }; - let action = Action { - service: "local".to_string(), - scope: "".to_string(), - predicates: vec![], - data: vec![], - }; - RuntimeAction::Auth( - AuthAction::new(&action, &service).expect("empty predicates should compile!"), - ) - } - - fn build_rate_limit_grpc_action() -> RuntimeAction { - let service = Service { - service_type: ServiceType::RateLimit, - endpoint: "local".to_string(), - failure_mode: FailureMode::Deny, - timeout: Timeout(Duration::from_millis(42)), - }; - let action = Action { - service: "local".to_string(), - scope: "".to_string(), - predicates: vec![], - data: vec![], - }; - RuntimeAction::RateLimit( - RateLimitAction::new(&action, &service).expect("empty predicates should compile!"), - ) - } - - fn build_operation(grpc_call_fn_stub: GrpcCallFn, action: RuntimeAction) -> Rc { - Rc::new(Operation { - state: RefCell::from(State::Pending), - result: RefCell::new(Ok(0)), - action: Rc::new(action), - service_handler: build_grpc_service_handler(), - grpc_call_fn: grpc_call_fn_stub, - get_map_values_bytes_fn: get_map_values_bytes_fn_stub, - grpc_message_build_fn: grpc_message_build_fn_stub, - conditions_apply_fn: conditions_apply_fn_stub, - }) - } - - #[test] - fn operation_getters() { - let operation = build_operation(default_grpc_call_fn_stub, build_rate_limit_grpc_action()); - - 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_result(), Ok(0)); - } - - #[test] - fn operation_transition() { - let operation = build_operation(default_grpc_call_fn_stub, build_rate_limit_grpc_action()); - assert_eq!(operation.get_result(), Ok(0)); - assert_eq!(operation.get_state(), State::Pending); - let mut res = operation.trigger(); - assert_eq!(res, Ok(200)); - assert_eq!(operation.get_state(), State::Waiting); - res = operation.trigger(); - assert_eq!(res, Ok(200)); - assert_eq!(operation.get_result(), Ok(200)); - assert_eq!(operation.get_state(), State::Done); - } - - #[test] - fn operation_dispatcher_push_actions() { - let mut operation_dispatcher = OperationDispatcher::default(); - - assert_eq!(operation_dispatcher.operations.len(), 0); - let operation = build_operation(default_grpc_call_fn_stub, build_rate_limit_grpc_action()); - operation_dispatcher.push_operations(vec![operation]); - - assert_eq!(operation_dispatcher.operations.len(), 1); - } - - #[test] - fn operation_dispatcher_get_current_action_state() { - let mut operation_dispatcher = OperationDispatcher::default(); - let operation = build_operation(default_grpc_call_fn_stub, build_rate_limit_grpc_action()); - operation_dispatcher.push_operations(vec![operation]); - assert_eq!( - operation_dispatcher.get_current_operation_state(), - Some(State::Pending) - ); - } - - #[test] - fn operation_dispatcher_next() { - let mut operation_dispatcher = OperationDispatcher::default(); - - fn grpc_call_fn_stub_66( - _upstream_name: &str, - _service_name: &str, - _method_name: &str, - _initial_metadata: Vec<(&str, &[u8])>, - _message: Option<&[u8]>, - _timeout: Duration, - ) -> Result { - Ok(66) - } - - fn grpc_call_fn_stub_77( - _upstream_name: &str, - _service_name: &str, - _method_name: &str, - _initial_metadata: Vec<(&str, &[u8])>, - _message: Option<&[u8]>, - _timeout: Duration, - ) -> Result { - Ok(77) - } - - operation_dispatcher.push_operations(vec![ - build_operation(grpc_call_fn_stub_66, build_rate_limit_grpc_action()), - build_operation(grpc_call_fn_stub_77, build_auth_grpc_action()), - ]); - - assert_eq!( - operation_dispatcher.get_current_operation_state(), - Some(State::Pending) - ); - assert_eq!(operation_dispatcher.waiting_operations.len(), 0); - - let mut op = operation_dispatcher.next(); - assert_eq!( - op.clone() - .expect("ok result") - .expect("operation is some") - .get_result(), - Ok(66) - ); - assert_eq!( - op.clone() - .expect("ok result") - .expect("operation is some") - .get_service_type(), - ServiceType::RateLimit - ); - assert_eq!( - op.expect("ok result") - .expect("operation is some") - .get_state(), - State::Waiting - ); - assert_eq!(operation_dispatcher.waiting_operations.len(), 1); - - op = operation_dispatcher.next(); - assert_eq!( - op.clone() - .expect("ok result") - .expect("operation is some") - .get_result(), - Ok(66) - ); - assert_eq!( - op.expect("ok result") - .expect("operation is some") - .get_state(), - State::Done - ); - - op = operation_dispatcher.next(); - assert_eq!( - op.clone() - .expect("ok result") - .expect("operation is some") - .get_result(), - Ok(77) - ); - assert_eq!( - op.clone() - .expect("ok result") - .expect("operation is some") - .get_service_type(), - ServiceType::Auth - ); - assert_eq!( - op.expect("ok result") - .expect("operation is some") - .get_state(), - State::Waiting - ); - assert_eq!(operation_dispatcher.waiting_operations.len(), 1); - - op = operation_dispatcher.next(); - assert_eq!( - op.clone() - .expect("ok result") - .expect("operation is some") - .get_result(), - Ok(77) - ); - assert_eq!( - op.expect("ok result") - .expect("operation is some") - .get_state(), - State::Done - ); - assert_eq!(operation_dispatcher.waiting_operations.len(), 1); - - op = operation_dispatcher.next(); - assert!(op.expect("ok result").is_none()); - assert!(operation_dispatcher.get_current_operation_state().is_none()); - assert_eq!(operation_dispatcher.waiting_operations.len(), 0); - } -} diff --git a/src/runtime_action.rs b/src/runtime_action.rs index bfaa725..26668f3 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -9,7 +9,6 @@ use log::debug; use protobuf::Message; use std::collections::HashMap; use std::rc::Rc; -use std::time::Duration; #[derive(Debug)] pub enum RuntimeAction { @@ -50,14 +49,6 @@ impl RuntimeAction { } } - pub fn get_timeout(&self) -> Duration { - self.grpc_service().get_timeout() - } - - pub fn get_service_type(&self) -> ServiceType { - self.grpc_service().get_service_type() - } - #[must_use] pub fn merge(&mut self, other: RuntimeAction) -> Option { // only makes sense for rate limiting actions diff --git a/src/service.rs b/src/service.rs index dd6f5a1..cdd484c 100644 --- a/src/service.rs +++ b/src/service.rs @@ -1,20 +1,12 @@ pub(crate) mod auth; -pub(crate) mod grpc_message; pub(crate) mod rate_limit; use crate::configuration::{FailureMode, Service, ServiceType}; use crate::envoy::StatusCode; -use crate::operation_dispatcher::Operation; -use crate::runtime_action::RuntimeAction; -use crate::service::auth::{AuthService, AUTH_METHOD_NAME, AUTH_SERVICE_NAME}; -use crate::service::grpc_message::{GrpcMessageRequest, GrpcMessageResponse}; -use crate::service::rate_limit::{RateLimitService, RATELIMIT_METHOD_NAME, RATELIMIT_SERVICE_NAME}; +use crate::service::auth::{AUTH_METHOD_NAME, AUTH_SERVICE_NAME}; +use crate::service::rate_limit::{RATELIMIT_METHOD_NAME, RATELIMIT_SERVICE_NAME}; use crate::service::TracingHeader::{Baggage, Traceparent, Tracestate}; -use log::warn; -use protobuf::Message; -use proxy_wasm::hostcalls; -use proxy_wasm::types::Status::SerializationFailure; -use proxy_wasm::types::{BufferType, Bytes, MapType, Status}; +use proxy_wasm::types::Bytes; use std::cell::OnceCell; use std::rc::Rc; use std::time::Duration; @@ -46,10 +38,6 @@ impl GrpcService { self.service.timeout.0 } - pub fn get_service_type(&self) -> ServiceType { - self.service.service_type.clone() - } - pub fn get_failure_mode(&self) -> FailureMode { self.service.failure_mode } @@ -74,46 +62,6 @@ impl GrpcService { ) }) } - - pub fn process_grpc_response( - operation: Rc, - resp_size: usize, - ) -> Result { - let failure_mode = operation.get_failure_mode(); - if let Ok(Some(res_body_bytes)) = - hostcalls::get_buffer(BufferType::GrpcReceiveBuffer, 0, resp_size) - { - match GrpcMessageResponse::new(&operation.get_service_type(), &res_body_bytes) { - Ok(res) => match operation.get_service_type() { - ServiceType::Auth => AuthService::process_auth_grpc_response(res, failure_mode), - ServiceType::RateLimit => { - RateLimitService::process_ratelimit_grpc_response(res, failure_mode) - } - }, - Err(e) => { - warn!( - "failed to parse grpc response body into GrpcMessageResponse message: {e}" - ); - GrpcService::handle_error_on_grpc_response(failure_mode); - Err(StatusCode::InternalServerError) - } - } - } else { - warn!("failed to get grpc buffer or return data is null!"); - GrpcService::handle_error_on_grpc_response(failure_mode); - Err(StatusCode::InternalServerError) - } - } - - 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")) - .expect("failed to send_http_response 500"); - } - FailureMode::Allow => {} - } - } } pub struct IndexedGrpcRequest { @@ -222,76 +170,6 @@ impl GrpcErrResponse { } } -pub struct GrpcResult { - pub response_headers: Vec<(String, String)>, -} -impl GrpcResult { - pub fn default() -> Self { - Self { - response_headers: Vec::new(), - } - } - pub fn new(response_headers: Vec<(String, String)>) -> Self { - Self { response_headers } - } -} - -pub type GrpcCallFn = fn( - upstream_name: &str, - service_name: &str, - method_name: &str, - initial_metadata: Vec<(&str, &[u8])>, - message: Option<&[u8]>, - timeout: Duration, -) -> Result; - -pub type GetMapValuesBytesFn = fn(map_type: MapType, key: &str) -> Result, Status>; - -pub type GrpcMessageBuildFn = fn(action: &RuntimeAction) -> Option; - -#[derive(Debug)] -pub struct GrpcServiceHandler { - grpc_service: Rc, - header_resolver: Rc, -} - -impl GrpcServiceHandler { - pub fn new(grpc_service: Rc, header_resolver: Rc) -> Self { - Self { - grpc_service, - header_resolver, - } - } - - pub fn send( - &self, - get_map_values_bytes_fn: GetMapValuesBytesFn, - grpc_call_fn: GrpcCallFn, - message: GrpcMessageRequest, - timeout: Duration, - ) -> Result { - let msg = Message::write_to_bytes(&message).map_err(|e| { - warn!("Failed to write protobuf message to bytes: {e:?}"); - SerializationFailure - })?; - let metadata = self - .header_resolver - .get(get_map_values_bytes_fn) - .iter() - .map(|(header, value)| (*header, value.as_slice())) - .collect(); - - grpc_call_fn( - self.grpc_service.endpoint(), - self.grpc_service.name(), - self.grpc_service.method(), - metadata, - Some(&msg), - timeout, - ) - } -} - #[derive(Debug)] pub struct HeaderResolver { headers: OnceCell>, @@ -310,20 +188,6 @@ impl HeaderResolver { } } - pub fn get(&self, get_map_values_bytes_fn: GetMapValuesBytesFn) -> &Vec<(&'static str, Bytes)> { - self.headers.get_or_init(|| { - let mut headers = Vec::new(); - for header in TracingHeader::all() { - if let Ok(Some(value)) = - get_map_values_bytes_fn(MapType::HttpRequestHeaders, (*header).as_str()) - { - headers.push(((*header).as_str(), value)); - } - } - headers - }) - } - pub fn get_with_ctx( &self, ctx: &T, diff --git a/src/service/auth.rs b/src/service/auth.rs index 1980136..9994398 100644 --- a/src/service/auth.rs +++ b/src/service/auth.rs @@ -1,18 +1,14 @@ -use crate::configuration::FailureMode; -use crate::data::{get_attribute, store_metadata}; +use crate::data::get_attribute; use crate::envoy::{ Address, AttributeContext, AttributeContext_HttpRequest, AttributeContext_Peer, - AttributeContext_Request, CheckRequest, CheckResponse_oneof_http_response, Metadata, - SocketAddress, StatusCode, + AttributeContext_Request, CheckRequest, Metadata, SocketAddress, }; -use crate::service::grpc_message::{GrpcMessageResponse, GrpcMessageResult}; -use crate::service::{GrpcResult, GrpcService}; use chrono::{DateTime, FixedOffset}; -use log::{debug, warn}; +use log::debug; use protobuf::well_known_types::Timestamp; use protobuf::Message; use proxy_wasm::hostcalls; -use proxy_wasm::types::{Bytes, MapType}; +use proxy_wasm::types::MapType; use std::collections::HashMap; pub const AUTH_SERVICE_NAME: &str = "envoy.service.auth.v3.Authorization"; @@ -32,13 +28,6 @@ impl AuthService { .ok() } - pub fn response_message(res_body_bytes: &Bytes) -> GrpcMessageResult { - match Message::parse_from_bytes(res_body_bytes) { - Ok(res) => Ok(GrpcMessageResponse::Auth(res)), - Err(e) => Err(e), - } - } - fn build_check_req(ce_host: String) -> CheckRequest { let mut auth_req = CheckRequest::default(); let mut attr = AttributeContext::default(); @@ -128,67 +117,4 @@ impl AuthService { peer.set_address(address); peer } - - pub fn process_auth_grpc_response( - auth_resp: GrpcMessageResponse, - failure_mode: FailureMode, - ) -> Result { - if let GrpcMessageResponse::Auth(check_response) = auth_resp { - // store dynamic metadata in filter state - store_metadata(check_response.get_dynamic_metadata()); - - match check_response.http_response { - Some(CheckResponse_oneof_http_response::ok_response(ok_response)) => { - debug!("process_auth_grpc_response: received OkHttpResponse"); - if !ok_response.get_response_headers_to_add().is_empty() { - panic!("process_auth_grpc_response: response contained response_headers_to_add which is unsupported!") - } - if !ok_response.get_headers_to_remove().is_empty() { - panic!("process_auth_grpc_response: response contained headers_to_remove which is unsupported!") - } - if !ok_response.get_query_parameters_to_set().is_empty() { - panic!("process_auth_grpc_response: response contained query_parameters_to_set which is unsupported!") - } - if !ok_response.get_query_parameters_to_remove().is_empty() { - panic!("process_auth_grpc_response: response contained query_parameters_to_remove which is unsupported!") - } - ok_response.get_headers().iter().for_each(|header| { - hostcalls::add_map_value( - MapType::HttpRequestHeaders, - header.get_header().get_key(), - header.get_header().get_value(), - ) - .expect("failed to add_map_value to HttpRequestHeaders") - }); - Ok(GrpcResult::default()) - } - Some(CheckResponse_oneof_http_response::denied_response(denied_response)) => { - debug!("process_auth_grpc_response: received DeniedHttpResponse"); - let mut response_headers = vec![]; - let status_code = denied_response.get_status().code; - denied_response.get_headers().iter().for_each(|header| { - response_headers.push(( - header.get_header().get_key(), - header.get_header().get_value(), - )) - }); - hostcalls::send_http_response( - status_code as u32, - response_headers, - Some(denied_response.get_body().as_ref()), - ) - .expect("failed to send_http_response"); - Err(status_code) - } - None => { - GrpcService::handle_error_on_grpc_response(failure_mode); - Err(StatusCode::InternalServerError) - } - } - } else { - warn!("not a GrpcMessageResponse::Auth(CheckResponse)!"); - GrpcService::handle_error_on_grpc_response(failure_mode); - Err(StatusCode::InternalServerError) - } - } } diff --git a/src/service/grpc_message.rs b/src/service/grpc_message.rs deleted file mode 100644 index 0014831..0000000 --- a/src/service/grpc_message.rs +++ /dev/null @@ -1,269 +0,0 @@ -use crate::configuration::ServiceType; -use crate::envoy::{CheckRequest, CheckResponse, RateLimitRequest, RateLimitResponse}; -use crate::runtime_action::RuntimeAction; -use crate::service::auth::AuthService; -use crate::service::rate_limit::RateLimitService; -use log::debug; -use protobuf::reflect::MessageDescriptor; -use protobuf::{ - Clear, CodedInputStream, CodedOutputStream, Message, ProtobufError, ProtobufResult, - UnknownFields, -}; -use proxy_wasm::types::Bytes; -use std::any::Any; - -#[derive(Clone, Debug)] -pub enum GrpcMessageRequest { - Auth(CheckRequest), - RateLimit(RateLimitRequest), -} - -impl Default for GrpcMessageRequest { - fn default() -> Self { - GrpcMessageRequest::RateLimit(RateLimitRequest::new()) - } -} - -impl Clear for GrpcMessageRequest { - fn clear(&mut self) { - match self { - GrpcMessageRequest::Auth(msg) => msg.clear(), - GrpcMessageRequest::RateLimit(msg) => msg.clear(), - } - } -} - -impl Message for GrpcMessageRequest { - fn descriptor(&self) -> &'static MessageDescriptor { - match self { - GrpcMessageRequest::Auth(msg) => msg.descriptor(), - GrpcMessageRequest::RateLimit(msg) => msg.descriptor(), - } - } - - fn is_initialized(&self) -> bool { - match self { - GrpcMessageRequest::Auth(msg) => msg.is_initialized(), - GrpcMessageRequest::RateLimit(msg) => msg.is_initialized(), - } - } - - fn merge_from(&mut self, is: &mut CodedInputStream) -> ProtobufResult<()> { - match self { - GrpcMessageRequest::Auth(msg) => msg.merge_from(is), - GrpcMessageRequest::RateLimit(msg) => msg.merge_from(is), - } - } - - fn write_to_with_cached_sizes(&self, os: &mut CodedOutputStream) -> ProtobufResult<()> { - match self { - GrpcMessageRequest::Auth(msg) => msg.write_to_with_cached_sizes(os), - GrpcMessageRequest::RateLimit(msg) => msg.write_to_with_cached_sizes(os), - } - } - - fn write_to_bytes(&self) -> ProtobufResult> { - match self { - GrpcMessageRequest::Auth(msg) => msg.write_to_bytes(), - GrpcMessageRequest::RateLimit(msg) => msg.write_to_bytes(), - } - } - - fn compute_size(&self) -> u32 { - match self { - GrpcMessageRequest::Auth(msg) => msg.compute_size(), - GrpcMessageRequest::RateLimit(msg) => msg.compute_size(), - } - } - - fn get_cached_size(&self) -> u32 { - match self { - GrpcMessageRequest::Auth(msg) => msg.get_cached_size(), - GrpcMessageRequest::RateLimit(msg) => msg.get_cached_size(), - } - } - - fn get_unknown_fields(&self) -> &UnknownFields { - match self { - GrpcMessageRequest::Auth(msg) => msg.get_unknown_fields(), - GrpcMessageRequest::RateLimit(msg) => msg.get_unknown_fields(), - } - } - - fn mut_unknown_fields(&mut self) -> &mut UnknownFields { - match self { - GrpcMessageRequest::Auth(msg) => msg.mut_unknown_fields(), - GrpcMessageRequest::RateLimit(msg) => msg.mut_unknown_fields(), - } - } - - fn as_any(&self) -> &dyn Any { - match self { - GrpcMessageRequest::Auth(msg) => msg.as_any(), - GrpcMessageRequest::RateLimit(msg) => msg.as_any(), - } - } - - fn new() -> Self - where - Self: Sized, - { - // Returning default value - GrpcMessageRequest::default() - } - - fn default_instance() -> &'static Self - where - Self: Sized, - { - #[allow(non_upper_case_globals)] - static instance: ::protobuf::rt::LazyV2 = ::protobuf::rt::LazyV2::INIT; - instance.get(|| GrpcMessageRequest::RateLimit(RateLimitRequest::new())) - } -} - -impl GrpcMessageRequest { - // Using domain as ce_host for the time being, we might pass a DataType in the future. - pub fn new(action: &RuntimeAction) -> Option { - match action { - RuntimeAction::RateLimit(rl_action) => { - let descriptor = rl_action.build_descriptor(); - if descriptor.entries.is_empty() { - debug!("grpc_message_request: empty descriptors"); - None - } else { - Some(GrpcMessageRequest::RateLimit( - RateLimitService::request_message( - String::from(rl_action.scope()), - vec![descriptor].into(), - ), - )) - } - } - RuntimeAction::Auth(auth_action) => Some(GrpcMessageRequest::Auth( - AuthService::request_message(String::from(auth_action.scope())), - )), - } - } -} - -#[derive(Clone, Debug)] -pub enum GrpcMessageResponse { - Auth(CheckResponse), - RateLimit(RateLimitResponse), -} - -impl Default for GrpcMessageResponse { - fn default() -> Self { - GrpcMessageResponse::RateLimit(RateLimitResponse::new()) - } -} - -impl Clear for GrpcMessageResponse { - fn clear(&mut self) { - todo!() - } -} - -impl Message for GrpcMessageResponse { - fn descriptor(&self) -> &'static MessageDescriptor { - match self { - GrpcMessageResponse::Auth(res) => res.descriptor(), - GrpcMessageResponse::RateLimit(res) => res.descriptor(), - } - } - - fn is_initialized(&self) -> bool { - match self { - GrpcMessageResponse::Auth(res) => res.is_initialized(), - GrpcMessageResponse::RateLimit(res) => res.is_initialized(), - } - } - - fn merge_from(&mut self, is: &mut CodedInputStream) -> ProtobufResult<()> { - match self { - GrpcMessageResponse::Auth(res) => res.merge_from(is), - GrpcMessageResponse::RateLimit(res) => res.merge_from(is), - } - } - - fn write_to_with_cached_sizes(&self, os: &mut CodedOutputStream) -> ProtobufResult<()> { - match self { - GrpcMessageResponse::Auth(res) => res.write_to_with_cached_sizes(os), - GrpcMessageResponse::RateLimit(res) => res.write_to_with_cached_sizes(os), - } - } - - fn write_to_bytes(&self) -> ProtobufResult> { - match self { - GrpcMessageResponse::Auth(res) => res.write_to_bytes(), - GrpcMessageResponse::RateLimit(res) => res.write_to_bytes(), - } - } - - fn compute_size(&self) -> u32 { - match self { - GrpcMessageResponse::Auth(res) => res.compute_size(), - GrpcMessageResponse::RateLimit(res) => res.compute_size(), - } - } - - fn get_cached_size(&self) -> u32 { - match self { - GrpcMessageResponse::Auth(res) => res.get_cached_size(), - GrpcMessageResponse::RateLimit(res) => res.get_cached_size(), - } - } - - fn get_unknown_fields(&self) -> &UnknownFields { - match self { - GrpcMessageResponse::Auth(res) => res.get_unknown_fields(), - GrpcMessageResponse::RateLimit(res) => res.get_unknown_fields(), - } - } - - fn mut_unknown_fields(&mut self) -> &mut UnknownFields { - match self { - GrpcMessageResponse::Auth(res) => res.mut_unknown_fields(), - GrpcMessageResponse::RateLimit(res) => res.mut_unknown_fields(), - } - } - - fn as_any(&self) -> &dyn Any { - match self { - GrpcMessageResponse::Auth(res) => res.as_any(), - GrpcMessageResponse::RateLimit(res) => res.as_any(), - } - } - - fn new() -> Self - where - Self: Sized, - { - // Returning default value - GrpcMessageResponse::default() - } - - fn default_instance() -> &'static Self - where - Self: Sized, - { - #[allow(non_upper_case_globals)] - static instance: ::protobuf::rt::LazyV2 = ::protobuf::rt::LazyV2::INIT; - instance.get(|| GrpcMessageResponse::RateLimit(RateLimitResponse::new())) - } -} - -impl GrpcMessageResponse { - pub fn new( - service_type: &ServiceType, - res_body_bytes: &Bytes, - ) -> GrpcMessageResult { - match service_type { - ServiceType::RateLimit => RateLimitService::response_message(res_body_bytes), - ServiceType::Auth => AuthService::response_message(res_body_bytes), - } - } -} - -pub type GrpcMessageResult = Result; diff --git a/src/service/rate_limit.rs b/src/service/rate_limit.rs index 68b7542..c4842c4 100644 --- a/src/service/rate_limit.rs +++ b/src/service/rate_limit.rs @@ -1,13 +1,6 @@ -use crate::configuration::FailureMode; -use crate::envoy::{ - RateLimitDescriptor, RateLimitRequest, RateLimitResponse, RateLimitResponse_Code, StatusCode, -}; -use crate::service::grpc_message::{GrpcMessageResponse, GrpcMessageResult}; -use crate::service::{GrpcResult, GrpcService}; -use log::{debug, warn}; +use crate::envoy::{RateLimitDescriptor, RateLimitRequest}; +use log::debug; use protobuf::{Message, RepeatedField}; -use proxy_wasm::hostcalls; -use proxy_wasm::types::Bytes; pub const RATELIMIT_SERVICE_NAME: &str = "envoy.service.ratelimit.v3.RateLimitService"; pub const RATELIMIT_METHOD_NAME: &str = "ShouldRateLimit"; @@ -37,59 +30,6 @@ impl RateLimitService { .map_err(|e| debug!("Failed to write protobuf message to bytes: {e:?}")) .ok() } - - pub fn response_message(res_body_bytes: &Bytes) -> GrpcMessageResult { - match Message::parse_from_bytes(res_body_bytes) { - Ok(res) => Ok(GrpcMessageResponse::RateLimit(res)), - Err(e) => Err(e), - } - } - - pub fn process_ratelimit_grpc_response( - rl_resp: GrpcMessageResponse, - failure_mode: FailureMode, - ) -> Result { - match rl_resp { - GrpcMessageResponse::RateLimit(RateLimitResponse { - overall_code: RateLimitResponse_Code::UNKNOWN, - .. - }) => { - GrpcService::handle_error_on_grpc_response(failure_mode); - Err(StatusCode::InternalServerError) - } - GrpcMessageResponse::RateLimit(RateLimitResponse { - overall_code: RateLimitResponse_Code::OVER_LIMIT, - response_headers_to_add: rl_headers, - .. - }) => { - let mut response_headers = vec![]; - for header in &rl_headers { - response_headers.push((header.get_key(), header.get_value())); - } - hostcalls::send_http_response(429, response_headers, Some(b"Too Many Requests\n")) - .expect("failed to send_http_response 429 while OVER_LIMIT"); - Err(StatusCode::TooManyRequests) - } - GrpcMessageResponse::RateLimit(RateLimitResponse { - overall_code: RateLimitResponse_Code::OK, - response_headers_to_add: additional_headers, - .. - }) => { - let result = GrpcResult::new( - additional_headers - .iter() - .map(|header| (header.get_key().to_owned(), header.get_value().to_owned())) - .collect(), - ); - Ok(result) - } - _ => { - warn!("not a valid GrpcMessageResponse::RateLimit(RateLimitResponse)!"); - GrpcService::handle_error_on_grpc_response(failure_mode); - Err(StatusCode::InternalServerError) - } - } - } } #[cfg(test)] From 64f0b5b420b384f89f5fd48130391a73f232795c Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Tue, 14 Jan 2025 13:51:11 +0000 Subject: [PATCH 10/21] Add set_property functions for test/notest Signed-off-by: Adam Cattermole --- src/auth_action.rs | 3 +-- src/data/attribute.rs | 3 +-- src/data/property.rs | 18 ++++++++++++++++++ src/filter/proposal_context.rs | 3 +++ src/ratelimit_action.rs | 1 - 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/src/auth_action.rs b/src/auth_action.rs index 5ad6e99..2326df5 100644 --- a/src/auth_action.rs +++ b/src/auth_action.rs @@ -46,10 +46,9 @@ impl AuthAction { &self, check_response: CheckResponse, ) -> Result>, GrpcErrResponse> { - //todo(adam-cattermole):hostvar resolver... + //todo(adam-cattermole):hostvar resolver? // store dynamic metadata in filter state debug!("process_response(auth): store_metadata"); - //todo(adam-cattermole): COMMENTED OUT FOR TESTS TO COMPILE store_metadata(check_response.get_dynamic_metadata()); match check_response.http_response { diff --git a/src/data/attribute.rs b/src/data/attribute.rs index a3a1d23..82bb051 100644 --- a/src/data/attribute.rs +++ b/src/data/attribute.rs @@ -2,7 +2,6 @@ use crate::data::PropertyPath; use chrono::{DateTime, FixedOffset}; use log::{debug, error, warn}; use protobuf::well_known_types::Struct; -use proxy_wasm::hostcalls; use serde_json::Value; pub const KUADRANT_NAMESPACE: &str = "kuadrant"; @@ -120,7 +119,7 @@ where } pub fn set_attribute(attr: &str, value: &[u8]) { - match hostcalls::set_property(PropertyPath::from(attr).tokens(), Some(value)) { + match crate::data::property::set_property(PropertyPath::from(attr), Some(value)) { Ok(_) => (), Err(_) => error!("set_attribute: failed to set property {attr}"), }; diff --git a/src/data/property.rs b/src/data/property.rs index bcfdc27..17321c5 100644 --- a/src/data/property.rs +++ b/src/data/property.rs @@ -55,6 +55,14 @@ pub fn host_get_map(path: &Path) -> Result, String> { } } +#[cfg(test)] +pub fn host_set_property(path: Path, value: Option<&[u8]>) -> Result<(), Status> { + debug!("set_property: {:?}", path); + let data = value.map(|bytes| bytes.to_vec()).unwrap_or_default(); + test::TEST_PROPERTY_VALUE.set(Some((path, data))); + Ok(()) +} + #[cfg(not(test))] pub fn host_get_map(path: &Path) -> Result, String> { match *path.tokens() { @@ -77,6 +85,12 @@ pub(super) fn host_get_property(path: &Path) -> Result>, Status> proxy_wasm::hostcalls::get_property(path.tokens()) } +#[cfg(not(test))] +pub(super) fn host_set_property(path: Path, value: Option<&[u8]>) -> Result<(), Status> { + debug!("set_property: {:?}", path); + proxy_wasm::hostcalls::set_property(path.tokens(), value) +} + pub(super) fn get_property(path: &Path) -> Result>, Status> { match *path.tokens() { ["source", "remote_address"] => remote_address(), @@ -85,6 +99,10 @@ pub(super) fn get_property(path: &Path) -> Result>, Status> { } } +pub(super) fn set_property(path: Path, value: Option<&[u8]>) -> Result<(), Status> { + host_set_property(path, value) +} + #[derive(Clone, Hash, PartialEq, Eq)] pub struct Path { tokens: Vec, diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs index 29a21e6..86fc0a9 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/proposal_context.rs @@ -61,6 +61,9 @@ pub mod no_implicit_dep { } pub fn fail(self) -> Operation { + //todo(adam-cattermole): should this take into account failure mode? + // these errors occurred at filter layer, + // i.e. error response / failed to read buffer / failed serdes Operation::Die(GrpcErrResponse::new_internal_server_error()) } } diff --git a/src/ratelimit_action.rs b/src/ratelimit_action.rs index 699f4be..f9d2426 100644 --- a/src/ratelimit_action.rs +++ b/src/ratelimit_action.rs @@ -226,7 +226,6 @@ mod test { Action, DataItem, DataType, ExpressionItem, FailureMode, Service, ServiceType, StaticItem, Timeout, }; - use crate::envoy::HeaderValueOption; fn build_service() -> Service { build_service_with_failure_mode(FailureMode::default()) From a3ecafd178692799f59e8fd46ab4388d2be00710 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 15 Jan 2025 13:11:21 +0000 Subject: [PATCH 11/21] Refactor handling message into operations Digest generates a Vec of operations we iterate over Signed-off-by: Adam Cattermole --- src/filter/proposal_context.rs | 147 ++++++++++++++++++--------------- 1 file changed, 82 insertions(+), 65 deletions(-) diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs index 86fc0a9..817264f 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/proposal_context.rs @@ -1,9 +1,8 @@ use crate::action_set_index::ActionSetIndex; use crate::filter::proposal_context::no_implicit_dep::{ - GrpcMessageReceiverOperation, HeadersOperation, Operation, + GrpcMessageReceiverOperation, GrpcMessageSenderOperation, HeadersOperation, Operation, }; -use crate::runtime_action_set::RuntimeActionSet; -use crate::service::{GrpcErrResponse, GrpcRequestAction, HeaderResolver}; +use crate::service::{GrpcErrResponse, GrpcRequest, HeaderResolver}; use log::{debug, warn}; use proxy_wasm::traits::{Context, HttpContext}; use proxy_wasm::types::{Action, Status}; @@ -11,20 +10,48 @@ use std::mem; use std::rc::Rc; pub mod no_implicit_dep { + use crate::filter::proposal_context::no_implicit_dep::Operation::SendGrpcRequest; use crate::runtime_action_set::RuntimeActionSet; - use crate::service::{GrpcErrResponse, GrpcRequestAction}; + use crate::service::{GrpcErrResponse, GrpcRequest, IndexedGrpcRequest}; use std::rc::Rc; - #[allow(dead_code)] pub enum Operation { + SendGrpcRequest(GrpcMessageSenderOperation), AwaitGrpcResponse(GrpcMessageReceiverOperation), AddHeaders(HeadersOperation), Die(GrpcErrResponse), - //todo(adam-cattermole): does Done make sense? in this case no PendingOperation - // instead just Option? + // Done indicates that we have no more operations and can resume the http request flow Done(), } + pub struct GrpcMessageSenderOperation { + runtime_action_set: Rc, + grpc_request: IndexedGrpcRequest, + } + + impl GrpcMessageSenderOperation { + pub fn new( + runtime_action_set: Rc, + indexed_request: IndexedGrpcRequest, + ) -> Self { + Self { + runtime_action_set, + grpc_request: indexed_request, + } + } + + pub fn build_receiver_operation(self) -> (GrpcRequest, Operation) { + let index = self.grpc_request.index(); + ( + self.grpc_request.request(), + Operation::AwaitGrpcResponse(GrpcMessageReceiverOperation::new( + self.runtime_action_set, + index, + )), + ) + } + } + pub struct GrpcMessageReceiverOperation { runtime_action_set: Rc, current_index: usize, @@ -38,28 +65,30 @@ pub mod no_implicit_dep { } } - pub fn digest_grpc_response( - self, - msg: &[u8], - ) -> Result<(Option, Option), Operation> { + pub fn digest_grpc_response(self, msg: &[u8]) -> Vec { let result = self .runtime_action_set .process_grpc_response(self.current_index, msg); match result { Ok((next_msg, headers)) => { - let header_op = - headers.map(|hs| Operation::AddHeaders(HeadersOperation::new(hs))); - Ok((next_msg, header_op)) + let mut operations = Vec::new(); + if let Some(headers) = headers { + operations.push(Operation::AddHeaders(HeadersOperation::new(headers))) + } + operations.push(match next_msg { + None => Operation::Done(), + Some(indexed_req) => SendGrpcRequest(GrpcMessageSenderOperation::new( + self.runtime_action_set, + indexed_req, + )), + }); + operations } - Err(grpc_err_resp) => Err(Operation::Die(grpc_err_resp)), + Err(grpc_err_resp) => vec![Operation::Die(grpc_err_resp)], } } - pub fn runtime_action_set(&self) -> Rc { - Rc::clone(&self.runtime_action_set) - } - pub fn fail(self) -> Operation { //todo(adam-cattermole): should this take into account failure mode? // these errors occurred at filter layer, @@ -88,7 +117,7 @@ pub(crate) struct Filter { index: Rc, header_resolver: Rc, - grpc_message_receiver_operation: Option, + grpc_message_receiver_operation: Option, headers_operations: Vec, } @@ -96,34 +125,20 @@ impl Context for Filter { fn on_grpc_call_response(&mut self, _token_id: u32, status_code: u32, resp_size: usize) { let receiver = mem::take(&mut self.grpc_message_receiver_operation) .expect("We need an operation pending a gRPC response"); - let action_set = receiver.runtime_action_set(); + + let mut ops = Vec::new(); if status_code != Status::Ok as u32 { - self.handle_operation(receiver.fail()); - return; + ops.push(receiver.fail()); + } else if let Some(response_body) = self.get_grpc_call_response_body(0, resp_size) { + ops.extend(receiver.digest_grpc_response(&response_body)); + } else { + ops.push(receiver.fail()); } - let response_body = match self.get_grpc_call_response_body(0, resp_size) { - Some(body) => body, - None => { - self.handle_operation(receiver.fail()); - return; - } - }; - - let result = receiver.digest_grpc_response(&response_body); - match result { - Ok((next_msg, header_op)) => { - if let Some(header_op) = header_op { - self.handle_operation(header_op); - } - let receiver_op = self.handle_next_message(action_set, next_msg); - self.handle_operation(receiver_op); - } - Err(die_op) => { - self.handle_operation(die_op); - } - } + ops.into_iter().for_each(|op| { + self.handle_operation(op); + }) } } @@ -137,9 +152,14 @@ impl HttpContext for Filter { .iter() .find(|action_set| action_set.conditions_apply(/* self */)) { - let request_action = action_set.start_flow(); - let next_op = self.handle_next_message(Rc::clone(action_set), request_action); - return self.handle_operation(next_op); + let grpc_request = action_set.start_flow(); + let op = match grpc_request { + None => Operation::Done(), + Some(indexed_req) => Operation::SendGrpcRequest( + GrpcMessageSenderOperation::new(Rc::clone(action_set), indexed_req), + ), + }; + return self.handle_operation(op); } } Action::Continue @@ -159,6 +179,20 @@ impl HttpContext for Filter { impl Filter { fn handle_operation(&mut self, operation: Operation) -> Action { match operation { + Operation::SendGrpcRequest(sender_op) => { + debug!("handle_operation: SendGrpcRequest"); + let next_op = { + let (req, receiver_op) = sender_op.build_receiver_operation(); + match self.send_grpc_request(req) { + Ok(_token) => receiver_op, + Err(status) => { + debug!("handle_operation: failed to send grpc request `{status:?}`"); + Operation::Die(GrpcErrResponse::new_internal_server_error()) + } + } + }; + self.handle_operation(next_op) + } Operation::AwaitGrpcResponse(receiver_op) => { debug!("handle_operation: AwaitGrpcResponse"); self.grpc_message_receiver_operation = Some(receiver_op); @@ -182,23 +216,6 @@ impl Filter { } } - fn handle_next_message( - &mut self, - action_set: Rc, - next_msg: Option, - ) -> Operation { - match next_msg { - Some(msg) => match self.send_grpc_request(&msg) { - Ok(_token) => Operation::AwaitGrpcResponse(GrpcMessageReceiverOperation::new( - action_set, - msg.index(), - )), - Err(_status) => panic!("Error sending request"), - }, - None => Operation::Done(), - } - } - fn die(&mut self, die: GrpcErrResponse) { self.send_http_response( die.status_code(), @@ -220,7 +237,7 @@ impl Filter { } } - fn send_grpc_request(&self, req: &GrpcRequestAction) -> Result { + fn send_grpc_request(&self, req: GrpcRequest) -> Result { let headers = self .header_resolver .get_with_ctx(self) From 918ed72a2d9110c733670ac47f0b0393cd39dcec Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 15 Jan 2025 14:43:32 +0000 Subject: [PATCH 12/21] Add failuremode check to message parsing Signed-off-by: Adam Cattermole --- src/runtime_action.rs | 38 ++++++++++++++++++++++++++------------ 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/src/runtime_action.rs b/src/runtime_action.rs index 26668f3..87e7ce9 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -1,6 +1,5 @@ use crate::auth_action::AuthAction; use crate::configuration::{Action, FailureMode, Service, ServiceType}; -use crate::envoy::{CheckResponse, RateLimitResponse}; use crate::ratelimit_action::RateLimitAction; use crate::service::auth::AuthService; use crate::service::rate_limit::RateLimitService; @@ -73,17 +72,32 @@ impl RuntimeAction { msg: &[u8], ) -> Result>, GrpcErrResponse> { match self { - Self::Auth(auth_action) => { - // todo(adam-cattermole): should this expect be here? - let check_response: CheckResponse = - Message::parse_from_bytes(msg).expect("invalid state!"); - auth_action.process_response(check_response) - } - Self::RateLimit(rl_action) => { - let rate_limit_response: RateLimitResponse = - Message::parse_from_bytes(msg).expect("invalid state!"); - rl_action.process_response(rate_limit_response) - } + Self::Auth(auth_action) => match Message::parse_from_bytes(msg) { + Ok(check_response) => auth_action.process_response(check_response), + Err(e) => { + debug!("process_response(auth): failed to parse response `{e:?}`"); + match self.get_failure_mode() { + FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()), + FailureMode::Allow => { + debug!("process_response(auth): continuing as FailureMode Allow"); + Ok(None) + } + } + } + }, + Self::RateLimit(rl_action) => match Message::parse_from_bytes(msg) { + Ok(rate_limit_response) => rl_action.process_response(rate_limit_response), + Err(e) => { + debug!("process_response(rl): failed to parse response `{e:?}`"); + match self.get_failure_mode() { + FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()), + FailureMode::Allow => { + debug!("process_response(rl): continuing as FailureMode Allow"); + Ok(None) + } + } + } + }, } } From 598e3ed562e02980b4f29d20af69f76fb32bcfd2 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 15 Jan 2025 14:51:09 +0000 Subject: [PATCH 13/21] Move header empty check up to remove Option Signed-off-by: Adam Cattermole --- src/auth_action.rs | 21 +++++++-------------- src/filter/proposal_context.rs | 2 +- src/ratelimit_action.rs | 20 +++++++------------- src/runtime_action.rs | 9 +++------ src/runtime_action_set.rs | 2 +- 5 files changed, 19 insertions(+), 35 deletions(-) diff --git a/src/auth_action.rs b/src/auth_action.rs index 2326df5..40c7d9e 100644 --- a/src/auth_action.rs +++ b/src/auth_action.rs @@ -45,7 +45,7 @@ impl AuthAction { pub fn process_response( &self, check_response: CheckResponse, - ) -> Result>, GrpcErrResponse> { + ) -> Result, GrpcErrResponse> { //todo(adam-cattermole):hostvar resolver? // store dynamic metadata in filter state debug!("process_response(auth): store_metadata"); @@ -58,7 +58,7 @@ impl AuthAction { FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()), FailureMode::Allow => { debug!("process_response(auth): continuing as FailureMode Allow"); - Ok(None) + Ok(Vec::default()) } } } @@ -87,13 +87,7 @@ impl AuthAction { if !ok_response.get_query_parameters_to_remove().is_empty() { panic!("process_response(auth): response contained query_parameters_to_remove which is unsupported!") } - - let response_headers = Self::get_header_vec(ok_response.get_headers()); - if response_headers.is_empty() { - Ok(None) - } else { - Ok(Some(response_headers)) - } + Ok(Self::get_header_vec(ok_response.get_headers())) } } } @@ -235,7 +229,7 @@ mod test { assert!(result.is_ok()); let headers = result.expect("is ok"); - assert!(headers.is_none()); + assert!(headers.is_empty()); let ok_response_with_header = build_check_response(StatusCode::OK, Some(vec![("my_header", "my_value")]), None); @@ -243,11 +237,10 @@ mod test { assert!(result.is_ok()); let headers = result.expect("is ok"); - assert!(headers.is_some()); + assert!(!headers.is_empty()); - let header_vec = headers.expect("is some"); assert_eq!( - header_vec[0], + headers[0], ("my_header".to_string(), "my_value".to_string()) ); } @@ -320,6 +313,6 @@ mod test { assert!(result.is_ok()); let headers = result.expect("is ok"); - assert!(headers.is_none()); + assert!(headers.is_empty()); } } diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs index 817264f..c5d4f4d 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/proposal_context.rs @@ -73,7 +73,7 @@ pub mod no_implicit_dep { match result { Ok((next_msg, headers)) => { let mut operations = Vec::new(); - if let Some(headers) = headers { + if !headers.is_empty() { operations.push(Operation::AddHeaders(HeadersOperation::new(headers))) } operations.push(match next_msg { diff --git a/src/ratelimit_action.rs b/src/ratelimit_action.rs index f9d2426..6d6fa0e 100644 --- a/src/ratelimit_action.rs +++ b/src/ratelimit_action.rs @@ -167,7 +167,7 @@ impl RateLimitAction { pub fn process_response( &self, rate_limit_response: RateLimitResponse, - ) -> Result>, GrpcErrResponse> { + ) -> Result, GrpcErrResponse> { match rate_limit_response { RateLimitResponse { overall_code: RateLimitResponse_Code::UNKNOWN, @@ -178,7 +178,7 @@ impl RateLimitAction { FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()), FailureMode::Allow => { debug!("process_response(rl): continuing as FailureMode Allow"); - Ok(None) + Ok(Vec::default()) } } } @@ -201,12 +201,7 @@ impl RateLimitAction { .. } => { debug!("process_response(rl): received OK response"); - let response_headers = Self::get_header_vec(additional_headers); - if response_headers.is_empty() { - Ok(None) - } else { - Ok(Some(response_headers)) - } + Ok(Self::get_header_vec(additional_headers)) } } } @@ -421,7 +416,7 @@ mod test { assert!(result.is_ok()); let headers = result.expect("is ok"); - assert!(headers.is_none()); + assert!(headers.is_empty()); let ok_response_with_header = build_ratelimit_response( RateLimitResponse_Code::OK, @@ -431,11 +426,10 @@ mod test { assert!(result.is_ok()); let headers = result.expect("is ok"); - assert!(headers.is_some()); + assert!(!headers.is_empty()); - let header_vec = headers.expect("is some"); assert_eq!( - header_vec[0], + headers[0], ("my_header".to_string(), "my_value".to_string()) ); } @@ -511,6 +505,6 @@ mod test { assert!(result.is_ok()); let headers = result.expect("is ok"); - assert!(headers.is_none()); + assert!(headers.is_empty()); } } diff --git a/src/runtime_action.rs b/src/runtime_action.rs index 87e7ce9..74b524d 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -67,10 +67,7 @@ impl RuntimeAction { } } - pub fn process_response( - &self, - msg: &[u8], - ) -> Result>, GrpcErrResponse> { + pub fn process_response(&self, msg: &[u8]) -> Result, GrpcErrResponse> { match self { Self::Auth(auth_action) => match Message::parse_from_bytes(msg) { Ok(check_response) => auth_action.process_response(check_response), @@ -80,7 +77,7 @@ impl RuntimeAction { FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()), FailureMode::Allow => { debug!("process_response(auth): continuing as FailureMode Allow"); - Ok(None) + Ok(Vec::default()) } } } @@ -93,7 +90,7 @@ impl RuntimeAction { FailureMode::Deny => Err(GrpcErrResponse::new_internal_server_error()), FailureMode::Allow => { debug!("process_response(rl): continuing as FailureMode Allow"); - Ok(None) + Ok(Vec::default()) } } } diff --git a/src/runtime_action_set.rs b/src/runtime_action_set.rs index 944b8f4..7ec272a 100644 --- a/src/runtime_action_set.rs +++ b/src/runtime_action_set.rs @@ -80,7 +80,7 @@ impl RuntimeActionSet { &self, index: usize, msg: &[u8], - ) -> Result<(Option, Option>), GrpcErrResponse> { + ) -> Result<(Option, Vec<(String, String)>), GrpcErrResponse> { self.runtime_actions[index] .process_response(msg) .map(|headers| { From 38d924fe8882ec638643106379af01699110a205 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 15 Jan 2025 14:59:38 +0000 Subject: [PATCH 14/21] Move the start_flow logic to filter function Signed-off-by: Adam Cattermole --- src/filter/proposal_context.rs | 21 +++++++++++++-------- src/runtime_action_set.rs | 2 +- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/src/filter/proposal_context.rs b/src/filter/proposal_context.rs index c5d4f4d..56fd625 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/proposal_context.rs @@ -2,6 +2,7 @@ use crate::action_set_index::ActionSetIndex; use crate::filter::proposal_context::no_implicit_dep::{ GrpcMessageReceiverOperation, GrpcMessageSenderOperation, HeadersOperation, Operation, }; +use crate::runtime_action_set::RuntimeActionSet; use crate::service::{GrpcErrResponse, GrpcRequest, HeaderResolver}; use log::{debug, warn}; use proxy_wasm::traits::{Context, HttpContext}; @@ -152,14 +153,7 @@ impl HttpContext for Filter { .iter() .find(|action_set| action_set.conditions_apply(/* self */)) { - let grpc_request = action_set.start_flow(); - let op = match grpc_request { - None => Operation::Done(), - Some(indexed_req) => Operation::SendGrpcRequest( - GrpcMessageSenderOperation::new(Rc::clone(action_set), indexed_req), - ), - }; - return self.handle_operation(op); + return self.start_flow(Rc::clone(action_set)); } } Action::Continue @@ -177,6 +171,17 @@ impl HttpContext for Filter { } impl Filter { + fn start_flow(&mut self, action_set: Rc) -> Action { + let grpc_request = action_set.find_first_grpc_request(); + let op = match grpc_request { + None => Operation::Done(), + Some(indexed_req) => { + Operation::SendGrpcRequest(GrpcMessageSenderOperation::new(action_set, indexed_req)) + } + }; + self.handle_operation(op) + } + fn handle_operation(&mut self, operation: Operation) -> Action { match operation { Operation::SendGrpcRequest(sender_op) => { diff --git a/src/runtime_action_set.rs b/src/runtime_action_set.rs index 7ec272a..e2a5837 100644 --- a/src/runtime_action_set.rs +++ b/src/runtime_action_set.rs @@ -60,7 +60,7 @@ impl RuntimeActionSet { self.route_rule_predicates.apply() } - pub fn start_flow(&self) -> Option { + pub fn find_first_grpc_request(&self) -> Option { self.find_next_grpc_request(0) } From 1c3af8f7f337a1c6cc4829c497bd2f6a3c9aa50b Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 15 Jan 2025 15:01:49 +0000 Subject: [PATCH 15/21] Rename Filter KuadrantFilter Signed-off-by: Adam Cattermole --- src/filter.rs | 3 ++- .../{proposal_context.rs => kuadrant_filter.rs} | 12 ++++++------ src/filter/root_context.rs | 4 ++-- 3 files changed, 10 insertions(+), 9 deletions(-) rename src/filter/{proposal_context.rs => kuadrant_filter.rs} (97%) diff --git a/src/filter.rs b/src/filter.rs index 2bb7557..f0f6279 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -1,4 +1,5 @@ -pub(crate) mod proposal_context; +pub(crate) mod kuadrant_filter; + mod root_context; #[cfg_attr( diff --git a/src/filter/proposal_context.rs b/src/filter/kuadrant_filter.rs similarity index 97% rename from src/filter/proposal_context.rs rename to src/filter/kuadrant_filter.rs index 56fd625..5539db8 100644 --- a/src/filter/proposal_context.rs +++ b/src/filter/kuadrant_filter.rs @@ -1,5 +1,5 @@ use crate::action_set_index::ActionSetIndex; -use crate::filter::proposal_context::no_implicit_dep::{ +use crate::filter::kuadrant_filter::no_implicit_dep::{ GrpcMessageReceiverOperation, GrpcMessageSenderOperation, HeadersOperation, Operation, }; use crate::runtime_action_set::RuntimeActionSet; @@ -11,7 +11,7 @@ use std::mem; use std::rc::Rc; pub mod no_implicit_dep { - use crate::filter::proposal_context::no_implicit_dep::Operation::SendGrpcRequest; + use crate::filter::kuadrant_filter::no_implicit_dep::Operation::SendGrpcRequest; use crate::runtime_action_set::RuntimeActionSet; use crate::service::{GrpcErrResponse, GrpcRequest, IndexedGrpcRequest}; use std::rc::Rc; @@ -113,7 +113,7 @@ pub mod no_implicit_dep { } } -pub(crate) struct Filter { +pub(crate) struct KuadrantFilter { context_id: u32, index: Rc, header_resolver: Rc, @@ -122,7 +122,7 @@ pub(crate) struct Filter { headers_operations: Vec, } -impl Context for Filter { +impl Context for KuadrantFilter { fn on_grpc_call_response(&mut self, _token_id: u32, status_code: u32, resp_size: usize) { let receiver = mem::take(&mut self.grpc_message_receiver_operation) .expect("We need an operation pending a gRPC response"); @@ -143,7 +143,7 @@ impl Context for Filter { } } -impl HttpContext for Filter { +impl HttpContext for KuadrantFilter { fn on_http_request_headers(&mut self, _: usize, _: bool) -> Action { if let Some(action_sets) = self .index @@ -170,7 +170,7 @@ impl HttpContext for Filter { } } -impl Filter { +impl KuadrantFilter { fn start_flow(&mut self, action_set: Rc) -> Action { let grpc_request = action_set.find_first_grpc_request(); let op = match grpc_request { diff --git a/src/filter/root_context.rs b/src/filter/root_context.rs index f43a7ab..714d3ac 100644 --- a/src/filter/root_context.rs +++ b/src/filter/root_context.rs @@ -1,6 +1,6 @@ use crate::action_set_index::ActionSetIndex; use crate::configuration::PluginConfiguration; -use crate::filter::proposal_context::Filter; +use crate::filter::kuadrant_filter::KuadrantFilter; use crate::service::HeaderResolver; use const_format::formatcp; use log::{debug, error, info}; @@ -35,7 +35,7 @@ impl RootContext for FilterRoot { fn create_http_context(&self, context_id: u32) -> Option> { debug!("#{} create_http_context", context_id); let header_resolver = Rc::new(HeaderResolver::new()); - Some(Box::new(Filter::new( + Some(Box::new(KuadrantFilter::new( context_id, Rc::clone(&self.action_set_index), header_resolver, From e2cf0bf750d51aa52d113dabd905571d6b8351d0 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 15 Jan 2025 15:13:15 +0000 Subject: [PATCH 16/21] Create and move code to operations module Signed-off-by: Adam Cattermole --- src/filter.rs | 2 +- src/filter/kuadrant_filter.rs | 105 +--------------------------------- src/filter/operations.rs | 100 ++++++++++++++++++++++++++++++++ 3 files changed, 102 insertions(+), 105 deletions(-) create mode 100644 src/filter/operations.rs diff --git a/src/filter.rs b/src/filter.rs index f0f6279..f13c557 100644 --- a/src/filter.rs +++ b/src/filter.rs @@ -1,5 +1,5 @@ pub(crate) mod kuadrant_filter; - +pub(crate) mod operations; mod root_context; #[cfg_attr( diff --git a/src/filter/kuadrant_filter.rs b/src/filter/kuadrant_filter.rs index 5539db8..a08ff5b 100644 --- a/src/filter/kuadrant_filter.rs +++ b/src/filter/kuadrant_filter.rs @@ -1,5 +1,5 @@ use crate::action_set_index::ActionSetIndex; -use crate::filter::kuadrant_filter::no_implicit_dep::{ +use crate::filter::operations::{ GrpcMessageReceiverOperation, GrpcMessageSenderOperation, HeadersOperation, Operation, }; use crate::runtime_action_set::RuntimeActionSet; @@ -10,109 +10,6 @@ use proxy_wasm::types::{Action, Status}; use std::mem; use std::rc::Rc; -pub mod no_implicit_dep { - use crate::filter::kuadrant_filter::no_implicit_dep::Operation::SendGrpcRequest; - use crate::runtime_action_set::RuntimeActionSet; - use crate::service::{GrpcErrResponse, GrpcRequest, IndexedGrpcRequest}; - use std::rc::Rc; - - pub enum Operation { - SendGrpcRequest(GrpcMessageSenderOperation), - AwaitGrpcResponse(GrpcMessageReceiverOperation), - AddHeaders(HeadersOperation), - Die(GrpcErrResponse), - // Done indicates that we have no more operations and can resume the http request flow - Done(), - } - - pub struct GrpcMessageSenderOperation { - runtime_action_set: Rc, - grpc_request: IndexedGrpcRequest, - } - - impl GrpcMessageSenderOperation { - pub fn new( - runtime_action_set: Rc, - indexed_request: IndexedGrpcRequest, - ) -> Self { - Self { - runtime_action_set, - grpc_request: indexed_request, - } - } - - pub fn build_receiver_operation(self) -> (GrpcRequest, Operation) { - let index = self.grpc_request.index(); - ( - self.grpc_request.request(), - Operation::AwaitGrpcResponse(GrpcMessageReceiverOperation::new( - self.runtime_action_set, - index, - )), - ) - } - } - - pub struct GrpcMessageReceiverOperation { - runtime_action_set: Rc, - current_index: usize, - } - - impl GrpcMessageReceiverOperation { - pub fn new(runtime_action_set: Rc, current_index: usize) -> Self { - Self { - runtime_action_set, - current_index, - } - } - - pub fn digest_grpc_response(self, msg: &[u8]) -> Vec { - let result = self - .runtime_action_set - .process_grpc_response(self.current_index, msg); - - match result { - Ok((next_msg, headers)) => { - let mut operations = Vec::new(); - if !headers.is_empty() { - operations.push(Operation::AddHeaders(HeadersOperation::new(headers))) - } - operations.push(match next_msg { - None => Operation::Done(), - Some(indexed_req) => SendGrpcRequest(GrpcMessageSenderOperation::new( - self.runtime_action_set, - indexed_req, - )), - }); - operations - } - Err(grpc_err_resp) => vec![Operation::Die(grpc_err_resp)], - } - } - - pub fn fail(self) -> Operation { - //todo(adam-cattermole): should this take into account failure mode? - // these errors occurred at filter layer, - // i.e. error response / failed to read buffer / failed serdes - Operation::Die(GrpcErrResponse::new_internal_server_error()) - } - } - - pub struct HeadersOperation { - headers: Vec<(String, String)>, - } - - impl HeadersOperation { - pub fn new(headers: Vec<(String, String)>) -> Self { - Self { headers } - } - - pub fn headers(self) -> Vec<(String, String)> { - self.headers - } - } -} - pub(crate) struct KuadrantFilter { context_id: u32, index: Rc, diff --git a/src/filter/operations.rs b/src/filter/operations.rs new file mode 100644 index 0000000..16206c5 --- /dev/null +++ b/src/filter/operations.rs @@ -0,0 +1,100 @@ +use crate::filter::operations::Operation::SendGrpcRequest; +use crate::runtime_action_set::RuntimeActionSet; +use crate::service::{GrpcErrResponse, GrpcRequest, IndexedGrpcRequest}; +use std::rc::Rc; + +pub enum Operation { + SendGrpcRequest(GrpcMessageSenderOperation), + AwaitGrpcResponse(GrpcMessageReceiverOperation), + AddHeaders(HeadersOperation), + Die(GrpcErrResponse), + // Done indicates that we have no more operations and can resume the http request flow + Done(), +} + +pub struct GrpcMessageSenderOperation { + runtime_action_set: Rc, + grpc_request: IndexedGrpcRequest, +} + +impl GrpcMessageSenderOperation { + pub fn new( + runtime_action_set: Rc, + indexed_request: IndexedGrpcRequest, + ) -> Self { + Self { + runtime_action_set, + grpc_request: indexed_request, + } + } + + pub fn build_receiver_operation(self) -> (GrpcRequest, Operation) { + let index = self.grpc_request.index(); + ( + self.grpc_request.request(), + Operation::AwaitGrpcResponse(GrpcMessageReceiverOperation::new( + self.runtime_action_set, + index, + )), + ) + } +} + +pub struct GrpcMessageReceiverOperation { + runtime_action_set: Rc, + current_index: usize, +} + +impl GrpcMessageReceiverOperation { + pub fn new(runtime_action_set: Rc, current_index: usize) -> Self { + Self { + runtime_action_set, + current_index, + } + } + + pub fn digest_grpc_response(self, msg: &[u8]) -> Vec { + let result = self + .runtime_action_set + .process_grpc_response(self.current_index, msg); + + match result { + Ok((next_msg, headers)) => { + let mut operations = Vec::new(); + if !headers.is_empty() { + operations.push(Operation::AddHeaders(HeadersOperation::new(headers))) + } + operations.push(match next_msg { + None => Operation::Done(), + Some(indexed_req) => SendGrpcRequest(GrpcMessageSenderOperation::new( + self.runtime_action_set, + indexed_req, + )), + }); + operations + } + Err(grpc_err_resp) => vec![Operation::Die(grpc_err_resp)], + } + } + + pub fn fail(self) -> Operation { + //todo(adam-cattermole): should this take into account failure mode? + // these errors occurred at filter layer, + // i.e. error response / failed to read buffer / failed serdes + Operation::Die(GrpcErrResponse::new_internal_server_error()) + } +} + +pub struct HeadersOperation { + headers: Vec<(String, String)>, +} + +impl HeadersOperation { + pub fn new(headers: Vec<(String, String)>) -> Self { + Self { headers } + } + + pub fn headers(self) -> Vec<(String, String)> { + self.headers + } +} From 303a689154bb44434c0303f9d410ab8f135e16e2 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 15 Jan 2025 15:23:57 +0000 Subject: [PATCH 17/21] Move filter.rs into src/filter/mod and start function into lib Signed-off-by: Adam Cattermole --- src/filter.rs | 40 ---------------------------------------- src/filter/mod.rs | 3 +++ src/lib.rs | 37 +++++++++++++++++++++++++++++++++++++ 3 files changed, 40 insertions(+), 40 deletions(-) delete mode 100644 src/filter.rs create mode 100644 src/filter/mod.rs diff --git a/src/filter.rs b/src/filter.rs deleted file mode 100644 index f13c557..0000000 --- a/src/filter.rs +++ /dev/null @@ -1,40 +0,0 @@ -pub(crate) mod kuadrant_filter; -pub(crate) mod operations; -mod root_context; - -#[cfg_attr( - all( - target_arch = "wasm32", - target_vendor = "unknown", - target_os = "unknown" - ), - export_name = "_start" -)] -#[cfg_attr( - not(all( - target_arch = "wasm32", - target_vendor = "unknown", - target_os = "unknown" - )), - allow(dead_code) -)] -// This is a C interface, so make it explicit in the fn signature (and avoid mangling) -extern "C" fn start() { - use log::info; - use proxy_wasm::traits::RootContext; - use proxy_wasm::types::LogLevel; - use root_context::FilterRoot; - - proxy_wasm::set_log_level(LogLevel::Trace); - std::panic::set_hook(Box::new(|panic_info| { - proxy_wasm::hostcalls::log(LogLevel::Critical, &panic_info.to_string()) - .expect("failed to log panic_info"); - })); - proxy_wasm::set_root_context(|context_id| -> Box { - info!("#{} set_root_context", context_id); - Box::new(FilterRoot { - context_id, - action_set_index: Default::default(), - }) - }); -} diff --git a/src/filter/mod.rs b/src/filter/mod.rs new file mode 100644 index 0000000..089a104 --- /dev/null +++ b/src/filter/mod.rs @@ -0,0 +1,3 @@ +pub(crate) mod kuadrant_filter; +pub(crate) mod operations; +pub(crate) mod root_context; diff --git a/src/lib.rs b/src/lib.rs index 7033a14..227ef35 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -12,6 +12,43 @@ mod runtime_action_set; mod runtime_config; mod service; +#[cfg_attr( + all( + target_arch = "wasm32", + target_vendor = "unknown", + target_os = "unknown" + ), + export_name = "_start" +)] +#[cfg_attr( + not(all( + target_arch = "wasm32", + target_vendor = "unknown", + target_os = "unknown" + )), + allow(dead_code) +)] +// This is a C interface, so make it explicit in the fn signature (and avoid mangling) +extern "C" fn start() { + use crate::filter::root_context::FilterRoot; + use log::info; + use proxy_wasm::traits::RootContext; + use proxy_wasm::types::LogLevel; + + proxy_wasm::set_log_level(LogLevel::Trace); + std::panic::set_hook(Box::new(|panic_info| { + proxy_wasm::hostcalls::log(LogLevel::Critical, &panic_info.to_string()) + .expect("failed to log panic_info"); + })); + proxy_wasm::set_root_context(|context_id| -> Box { + info!("#{} set_root_context", context_id); + Box::new(FilterRoot { + context_id, + action_set_index: Default::default(), + }) + }); +} + #[cfg(test)] mod tests { use crate::envoy::{HeaderValue, RateLimitResponse, RateLimitResponse_Code}; From 8a85e19c7b4a40af80089e9ac1ad517dfad43d3d Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Thu, 16 Jan 2025 13:48:11 +0000 Subject: [PATCH 18/21] Add back debug logging for primary filter entrypoints Signed-off-by: Adam Cattermole --- src/filter/kuadrant_filter.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/src/filter/kuadrant_filter.rs b/src/filter/kuadrant_filter.rs index a08ff5b..72638b8 100644 --- a/src/filter/kuadrant_filter.rs +++ b/src/filter/kuadrant_filter.rs @@ -20,7 +20,11 @@ pub(crate) struct KuadrantFilter { } impl Context for KuadrantFilter { - fn on_grpc_call_response(&mut self, _token_id: u32, status_code: u32, resp_size: usize) { + fn on_grpc_call_response(&mut self, token_id: u32, status_code: u32, resp_size: usize) { + debug!( + "#{} on_grpc_call_response: received gRPC call response: token: {token_id}, status: {status_code}", + self.context_id + ); let receiver = mem::take(&mut self.grpc_message_receiver_operation) .expect("We need an operation pending a gRPC response"); @@ -42,6 +46,11 @@ impl Context for KuadrantFilter { impl HttpContext for KuadrantFilter { fn on_http_request_headers(&mut self, _: usize, _: bool) -> Action { + debug!("#{} on_http_request_headers", self.context_id); + + #[cfg(feature = "debug-host-behaviour")] + crate::data::debug_all_well_known_attributes(); + if let Some(action_sets) = self .index .get_longest_match_action_sets(self.request_authority().as_ref()) @@ -50,6 +59,10 @@ impl HttpContext for KuadrantFilter { .iter() .find(|action_set| action_set.conditions_apply(/* self */)) { + debug!( + "#{} action_set selected {}", + self.context_id, action_set.name + ); return self.start_flow(Rc::clone(action_set)); } } @@ -57,6 +70,7 @@ impl HttpContext for KuadrantFilter { } fn on_http_response_headers(&mut self, _num_headers: usize, _end_of_stream: bool) -> Action { + debug!("#{} on_http_response_headers", self.context_id); let headers_operations = mem::take(&mut self.headers_operations); for op in headers_operations { for (header, value) in &op.headers() { From faa0375033d6fbab6059e2543f43937c91d9c8ed Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Thu, 16 Jan 2025 14:04:54 +0000 Subject: [PATCH 19/21] Add headers type alias Signed-off-by: Adam Cattermole --- src/auth_action.rs | 6 +++--- src/filter/operations.rs | 8 ++++---- src/ratelimit_action.rs | 4 ++-- src/runtime_action.rs | 4 ++-- src/runtime_action_set.rs | 4 ++-- src/service.rs | 5 +++-- 6 files changed, 16 insertions(+), 15 deletions(-) diff --git a/src/auth_action.rs b/src/auth_action.rs index 40c7d9e..5b1dbd9 100644 --- a/src/auth_action.rs +++ b/src/auth_action.rs @@ -1,7 +1,7 @@ use crate::configuration::{Action, FailureMode, Service}; use crate::data::{store_metadata, Predicate, PredicateVec}; use crate::envoy::{CheckResponse, CheckResponse_oneof_http_response, HeaderValueOption}; -use crate::service::{GrpcErrResponse, GrpcService}; +use crate::service::{GrpcErrResponse, GrpcService, Headers}; use log::debug; use std::rc::Rc; @@ -45,7 +45,7 @@ impl AuthAction { pub fn process_response( &self, check_response: CheckResponse, - ) -> Result, GrpcErrResponse> { + ) -> Result { //todo(adam-cattermole):hostvar resolver? // store dynamic metadata in filter state debug!("process_response(auth): store_metadata"); @@ -92,7 +92,7 @@ impl AuthAction { } } - fn get_header_vec(headers: &[HeaderValueOption]) -> Vec<(String, String)> { + fn get_header_vec(headers: &[HeaderValueOption]) -> Headers { headers .iter() .map(|header| { diff --git a/src/filter/operations.rs b/src/filter/operations.rs index 16206c5..0e62a29 100644 --- a/src/filter/operations.rs +++ b/src/filter/operations.rs @@ -1,6 +1,6 @@ use crate::filter::operations::Operation::SendGrpcRequest; use crate::runtime_action_set::RuntimeActionSet; -use crate::service::{GrpcErrResponse, GrpcRequest, IndexedGrpcRequest}; +use crate::service::{GrpcErrResponse, GrpcRequest, Headers, IndexedGrpcRequest}; use std::rc::Rc; pub enum Operation { @@ -86,15 +86,15 @@ impl GrpcMessageReceiverOperation { } pub struct HeadersOperation { - headers: Vec<(String, String)>, + headers: Headers, } impl HeadersOperation { - pub fn new(headers: Vec<(String, String)>) -> Self { + pub fn new(headers: Headers) -> Self { Self { headers } } - pub fn headers(self) -> Vec<(String, String)> { + pub fn headers(self) -> Headers { self.headers } } diff --git a/src/ratelimit_action.rs b/src/ratelimit_action.rs index 6d6fa0e..c8d6cf0 100644 --- a/src/ratelimit_action.rs +++ b/src/ratelimit_action.rs @@ -5,7 +5,7 @@ use crate::envoy::{ HeaderValue, RateLimitDescriptor, RateLimitDescriptor_Entry, RateLimitResponse, RateLimitResponse_Code, StatusCode, }; -use crate::service::{GrpcErrResponse, GrpcService}; +use crate::service::{GrpcErrResponse, GrpcService, Headers}; use cel_interpreter::Value; use log::{debug, error}; use protobuf::RepeatedField; @@ -206,7 +206,7 @@ impl RateLimitAction { } } - fn get_header_vec(headers: RepeatedField) -> Vec<(String, String)> { + fn get_header_vec(headers: RepeatedField) -> Headers { headers .iter() .map(|header| (header.key.to_owned(), header.value.to_owned())) diff --git a/src/runtime_action.rs b/src/runtime_action.rs index 74b524d..7b384c3 100644 --- a/src/runtime_action.rs +++ b/src/runtime_action.rs @@ -3,7 +3,7 @@ use crate::configuration::{Action, FailureMode, Service, ServiceType}; use crate::ratelimit_action::RateLimitAction; use crate::service::auth::AuthService; use crate::service::rate_limit::RateLimitService; -use crate::service::{GrpcErrResponse, GrpcRequest, GrpcService}; +use crate::service::{GrpcErrResponse, GrpcRequest, GrpcService, Headers}; use log::debug; use protobuf::Message; use std::collections::HashMap; @@ -67,7 +67,7 @@ impl RuntimeAction { } } - pub fn process_response(&self, msg: &[u8]) -> Result, GrpcErrResponse> { + pub fn process_response(&self, msg: &[u8]) -> Result { match self { Self::Auth(auth_action) => match Message::parse_from_bytes(msg) { Ok(check_response) => auth_action.process_response(check_response), diff --git a/src/runtime_action_set.rs b/src/runtime_action_set.rs index e2a5837..584e3f1 100644 --- a/src/runtime_action_set.rs +++ b/src/runtime_action_set.rs @@ -1,7 +1,7 @@ use crate::configuration::{ActionSet, Service}; use crate::data::{Predicate, PredicateVec}; use crate::runtime_action::RuntimeAction; -use crate::service::{GrpcErrResponse, IndexedGrpcRequest}; +use crate::service::{GrpcErrResponse, Headers, IndexedGrpcRequest}; use std::collections::HashMap; use std::rc::Rc; @@ -80,7 +80,7 @@ impl RuntimeActionSet { &self, index: usize, msg: &[u8], - ) -> Result<(Option, Vec<(String, String)>), GrpcErrResponse> { + ) -> Result<(Option, Headers), GrpcErrResponse> { self.runtime_actions[index] .process_response(msg) .map(|headers| { diff --git a/src/service.rs b/src/service.rs index cdd484c..5c57354 100644 --- a/src/service.rs +++ b/src/service.rs @@ -130,15 +130,16 @@ impl GrpcRequest { } } +pub type Headers = Vec<(String, String)>; #[derive(Debug)] pub struct GrpcErrResponse { status_code: u32, - response_headers: Vec<(String, String)>, + response_headers: Headers, body: String, } impl GrpcErrResponse { - pub fn new(status_code: u32, response_headers: Vec<(String, String)>, body: String) -> Self { + pub fn new(status_code: u32, response_headers: Headers, body: String) -> Self { Self { status_code, response_headers, From 8a765150e4604f529863cee37944bf8abf00e2f0 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Thu, 16 Jan 2025 15:11:48 +0000 Subject: [PATCH 20/21] Handle failuremode in operation die Signed-off-by: Adam Cattermole --- src/filter/kuadrant_filter.rs | 4 ++-- src/filter/operations.rs | 25 ++++++++++++++++--------- src/runtime_action_set.rs | 2 +- 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/src/filter/kuadrant_filter.rs b/src/filter/kuadrant_filter.rs index 72638b8..14ec6ba 100644 --- a/src/filter/kuadrant_filter.rs +++ b/src/filter/kuadrant_filter.rs @@ -100,10 +100,10 @@ impl KuadrantFilter { let next_op = { let (req, receiver_op) = sender_op.build_receiver_operation(); match self.send_grpc_request(req) { - Ok(_token) => receiver_op, + Ok(_token) => Operation::AwaitGrpcResponse(receiver_op), Err(status) => { debug!("handle_operation: failed to send grpc request `{status:?}`"); - Operation::Die(GrpcErrResponse::new_internal_server_error()) + receiver_op.fail() } } }; diff --git a/src/filter/operations.rs b/src/filter/operations.rs index 0e62a29..2d6d0ad 100644 --- a/src/filter/operations.rs +++ b/src/filter/operations.rs @@ -1,3 +1,4 @@ +use crate::configuration::FailureMode; use crate::filter::operations::Operation::SendGrpcRequest; use crate::runtime_action_set::RuntimeActionSet; use crate::service::{GrpcErrResponse, GrpcRequest, Headers, IndexedGrpcRequest}; @@ -28,14 +29,11 @@ impl GrpcMessageSenderOperation { } } - pub fn build_receiver_operation(self) -> (GrpcRequest, Operation) { + pub fn build_receiver_operation(self) -> (GrpcRequest, GrpcMessageReceiverOperation) { let index = self.grpc_request.index(); ( self.grpc_request.request(), - Operation::AwaitGrpcResponse(GrpcMessageReceiverOperation::new( - self.runtime_action_set, - index, - )), + GrpcMessageReceiverOperation::new(self.runtime_action_set, index), ) } } @@ -78,10 +76,19 @@ impl GrpcMessageReceiverOperation { } pub fn fail(self) -> Operation { - //todo(adam-cattermole): should this take into account failure mode? - // these errors occurred at filter layer, - // i.e. error response / failed to read buffer / failed serdes - Operation::Die(GrpcErrResponse::new_internal_server_error()) + match self.runtime_action_set.runtime_actions[self.current_index].get_failure_mode() { + FailureMode::Deny => Operation::Die(GrpcErrResponse::new_internal_server_error()), + FailureMode::Allow => match self + .runtime_action_set + .find_next_grpc_request(self.current_index + 1) + { + None => Operation::Done(), + Some(indexed_req) => Operation::SendGrpcRequest(GrpcMessageSenderOperation::new( + self.runtime_action_set, + indexed_req, + )), + }, + } } } diff --git a/src/runtime_action_set.rs b/src/runtime_action_set.rs index 584e3f1..ae7a136 100644 --- a/src/runtime_action_set.rs +++ b/src/runtime_action_set.rs @@ -64,7 +64,7 @@ impl RuntimeActionSet { self.find_next_grpc_request(0) } - fn find_next_grpc_request(&self, start: usize) -> Option { + pub fn find_next_grpc_request(&self, start: usize) -> Option { self.runtime_actions .iter() .skip(start) From bc55c1c9fe7c8ba30b3a43f729352e0d7c0d7183 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Thu, 16 Jan 2025 15:54:48 +0000 Subject: [PATCH 21/21] Update expected logging in integration tests Signed-off-by: Adam Cattermole --- tests/auth.rs | 53 +++++++++++++++++++++++++++++----- tests/failuremode.rs | 36 +++++++++++++++++++---- tests/failures.rs | 62 ++++++++++++++++++++++++++++++---------- tests/multi.rs | 63 +++++++++++++++++++++++++++++++++++++---- tests/rate_limited.rs | 52 ++++++++++++++++++++++++++++------ tests/remote_address.rs | 11 ++++++- 6 files changed, 233 insertions(+), 44 deletions(-) diff --git a/tests/auth.rs b/tests/auth.rs index c11e533..1a05e77 100644 --- a/tests/auth.rs +++ b/tests/auth.rs @@ -164,6 +164,10 @@ fn it_auths() { ) .expect_get_property(Some(vec!["source", "port"])) .returning(Some(data::source::port::P_45000)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -182,7 +186,7 @@ fn it_auths() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -205,8 +209,13 @@ fn it_auths() { .returning(Some(&grpc_response)) .expect_log( Some(LogLevel::Debug), - Some("process_auth_grpc_response: received OkHttpResponse"), + Some("process_response(auth): store_metadata"), ) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(auth): received OkHttpResponse"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::None) .unwrap(); @@ -347,6 +356,10 @@ fn it_denies() { ) .expect_get_property(Some(vec!["source", "port"])) .returning(Some(data::source::port::P_45000)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -365,7 +378,7 @@ fn it_denies() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -388,8 +401,13 @@ fn it_denies() { .returning(Some(&grpc_response)) .expect_log( Some(LogLevel::Debug), - Some("process_auth_grpc_response: received DeniedHttpResponse"), + Some("process_response(auth): store_metadata"), ) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(auth): received DeniedHttpResponse"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Die")) .expect_send_local_response( Some(401), None, @@ -548,6 +566,10 @@ fn it_does_not_fold_auth_actions() { ) .expect_get_property(Some(vec!["source", "port"])) .returning(Some(data::source::port::P_45000)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -566,7 +588,7 @@ fn it_does_not_fold_auth_actions() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -589,7 +611,11 @@ fn it_does_not_fold_auth_actions() { .returning(Some(&grpc_response)) .expect_log( Some(LogLevel::Debug), - Some("process_auth_grpc_response: received OkHttpResponse"), + Some("process_response(auth): store_metadata"), + ) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(auth): received OkHttpResponse"), ) .expect_get_header_map_pairs(Some(MapType::HttpRequestHeaders)) .returning(None) @@ -653,6 +679,10 @@ fn it_does_not_fold_auth_actions() { ) .expect_get_property(Some(vec!["source", "port"])) .returning(Some(data::source::port::P_45000)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) .expect_grpc_call( Some("authorino-cluster"), Some("envoy.service.auth.v3.Authorization"), @@ -662,6 +692,10 @@ fn it_does_not_fold_auth_actions() { Some(5000), ) .returning(Ok(42)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: AwaitGrpcResponse"), + ) .execute_and_expect(ReturnType::None) .unwrap(); @@ -683,8 +717,13 @@ fn it_does_not_fold_auth_actions() { .returning(Some(&grpc_response)) .expect_log( Some(LogLevel::Debug), - Some("process_auth_grpc_response: received OkHttpResponse"), + Some("process_response(auth): store_metadata"), + ) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(auth): received OkHttpResponse"), ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::None) .unwrap(); diff --git a/tests/failuremode.rs b/tests/failuremode.rs index c05544f..38daf56 100644 --- a/tests/failuremode.rs +++ b/tests/failuremode.rs @@ -101,6 +101,10 @@ fn it_runs_next_action_on_failure_when_failuremode_is_allow() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -119,7 +123,7 @@ fn it_runs_next_action_on_failure_when_failuremode_is_allow() { .returning(Ok(first_call_token_id)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -132,8 +136,10 @@ fn it_runs_next_action_on_failure_when_failuremode_is_allow() { Some(LogLevel::Debug), Some(format!("#2 on_grpc_call_response: received gRPC call response: token: {first_call_token_id}, status: {status_code}").as_str()), ) - .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) - .returning(Some(&[])) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) .expect_grpc_call( Some("limitador-cluster"), Some("envoy.service.ratelimit.v3.RateLimitService"), @@ -143,6 +149,10 @@ fn it_runs_next_action_on_failure_when_failuremode_is_allow() { Some(5000), ) .returning(Ok(second_call_token_id)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: AwaitGrpcResponse"), + ) .execute_and_expect(ReturnType::None) .unwrap(); @@ -159,6 +169,14 @@ fn it_runs_next_action_on_failure_when_failuremode_is_allow() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: Done"), + ) .execute_and_expect(ReturnType::None) .unwrap(); @@ -264,6 +282,10 @@ fn it_stops_on_failure_when_failuremode_is_deny() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -282,7 +304,7 @@ fn it_stops_on_failure_when_failuremode_is_deny() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -294,8 +316,10 @@ fn it_stops_on_failure_when_failuremode_is_deny() { Some(LogLevel::Debug), Some(format!("#2 on_grpc_call_response: received gRPC call response: token: 42, status: {status_code}").as_str()), ) - .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) - .returning(Some(&[])) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: Die"), + ) .expect_send_local_response(Some(500), None, None, None) .execute_and_expect(ReturnType::None) .unwrap(); diff --git a/tests/failures.rs b/tests/failures.rs index 3e09ea3..42dda48 100644 --- a/tests/failures.rs +++ b/tests/failures.rs @@ -84,6 +84,10 @@ fn it_fails_on_first_action_grpc_call() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -104,13 +108,10 @@ fn it_fails_on_first_action_grpc_call() { ) .returning(Err(TestStatus::ParseFailure)) .expect_log( - Some(LogLevel::Error), - Some("OperationError { status: ParseFailure, failure_mode: Deny }"), - ) - .expect_log( - Some(LogLevel::Warn), - Some("OperationError Status: ParseFailure"), + Some(LogLevel::Debug), + Some("handle_operation: failed to send grpc request `ParseFailure`"), ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Die")) .expect_send_local_response( Some(500), Some("Internal Server Error.\n"), @@ -219,6 +220,10 @@ fn it_fails_on_second_action_grpc_call() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -240,7 +245,7 @@ fn it_fails_on_second_action_grpc_call() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -254,6 +259,14 @@ fn it_fails_on_second_action_grpc_call() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) .expect_grpc_call( Some("does-not-exist"), Some("envoy.service.ratelimit.v3.RateLimitService"), @@ -267,9 +280,10 @@ fn it_fails_on_second_action_grpc_call() { ) .returning(Err(TestStatus::ParseFailure)) .expect_log( - Some(LogLevel::Error), - Some("OperationError { status: ParseFailure, failure_mode: Deny }"), + Some(LogLevel::Debug), + Some("handle_operation: failed to send grpc request `ParseFailure`"), ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Die")) .expect_send_local_response( Some(500), Some("Internal Server Error.\n"), @@ -358,6 +372,10 @@ fn it_fails_on_first_action_grpc_response() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -379,7 +397,7 @@ fn it_fails_on_first_action_grpc_response() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -391,8 +409,7 @@ fn it_fails_on_first_action_grpc_response() { Some(LogLevel::Debug), Some("#2 on_grpc_call_response: received gRPC call response: token: 42, status: 14"), ) - .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) - .returning(Some(&[])) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Die")) .expect_send_local_response( Some(500), Some("Internal Server Error.\n"), @@ -500,6 +517,10 @@ fn it_fails_on_second_action_grpc_response() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -521,7 +542,7 @@ fn it_fails_on_second_action_grpc_response() { .returning(Ok(first_call_token_id)) .expect_log( Some(LogLevel::Debug), - Some(format!("#2 initiated gRPC call (id# {first_call_token_id})").as_str()), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -540,6 +561,14 @@ fn it_fails_on_second_action_grpc_response() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) .expect_grpc_call( Some("unreachable-cluster"), Some("envoy.service.ratelimit.v3.RateLimitService"), @@ -552,6 +581,10 @@ fn it_fails_on_second_action_grpc_response() { Some(5000), ) .returning(Ok(second_call_token_id)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: AwaitGrpcResponse"), + ) .execute_and_expect(ReturnType::None) .unwrap(); @@ -564,8 +597,7 @@ fn it_fails_on_second_action_grpc_response() { "#2 on_grpc_call_response: received gRPC call response: token: {second_call_token_id}, status: {status_code}" ).as_str()), ) - .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) - .returning(Some(&[])) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Die")) .expect_send_local_response( Some(500), Some("Internal Server Error.\n"), diff --git a/tests/multi.rs b/tests/multi.rs index 4f9c294..7b9857c 100644 --- a/tests/multi.rs +++ b/tests/multi.rs @@ -181,6 +181,10 @@ fn it_performs_authenticated_rate_limiting() { ) .expect_get_property(Some(vec!["source", "port"])) .returning(Some(data::source::port::P_45000)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -199,7 +203,7 @@ fn it_performs_authenticated_rate_limiting() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -215,7 +219,15 @@ fn it_performs_authenticated_rate_limiting() { .returning(Some(&grpc_response)) .expect_log( Some(LogLevel::Debug), - Some("process_auth_grpc_response: received OkHttpResponse"), + Some("process_response(auth): store_metadata"), + ) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(auth): received OkHttpResponse"), + ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), ) .expect_grpc_call( Some("limitador-cluster"), @@ -226,6 +238,10 @@ fn it_performs_authenticated_rate_limiting() { Some(5000), ) .returning(Ok(43)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: AwaitGrpcResponse"), + ) .execute_and_expect(ReturnType::None) .unwrap(); @@ -238,6 +254,11 @@ fn it_performs_authenticated_rate_limiting() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::None) .unwrap(); @@ -378,6 +399,10 @@ fn unauthenticated_does_not_ratelimit() { ) .expect_get_property(Some(vec!["source", "port"])) .returning(Some(data::source::port::P_45000)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -396,7 +421,7 @@ fn unauthenticated_does_not_ratelimit() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -419,8 +444,13 @@ fn unauthenticated_does_not_ratelimit() { .returning(Some(&grpc_response)) .expect_log( Some(LogLevel::Debug), - Some("process_auth_grpc_response: received DeniedHttpResponse"), + Some("process_response(auth): store_metadata"), ) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(auth): received DeniedHttpResponse"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Die")) .expect_send_local_response( Some(401), None, @@ -638,6 +668,10 @@ fn authenticated_one_ratelimit_action_matches() { ) .expect_get_property(Some(vec!["source", "port"])) .returning(Some(data::source::port::P_45000)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -656,7 +690,7 @@ fn authenticated_one_ratelimit_action_matches() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -672,7 +706,11 @@ fn authenticated_one_ratelimit_action_matches() { .returning(Some(&grpc_response)) .expect_log( Some(LogLevel::Debug), - Some("process_auth_grpc_response: received OkHttpResponse"), + Some("process_response(auth): store_metadata"), + ) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(auth): received OkHttpResponse"), ) // conditions checks .expect_log( @@ -687,6 +725,10 @@ fn authenticated_one_ratelimit_action_matches() { ) .expect_get_property(Some(vec!["source", "address"])) .returning(Some("1.2.3.4:80".as_bytes())) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) .expect_grpc_call( Some("limitador-cluster"), Some("envoy.service.ratelimit.v3.RateLimitService"), @@ -696,6 +738,10 @@ fn authenticated_one_ratelimit_action_matches() { Some(5000), ) .returning(Ok(43)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: AwaitGrpcResponse"), + ) .execute_and_expect(ReturnType::None) .unwrap(); @@ -708,6 +754,11 @@ fn authenticated_one_ratelimit_action_matches() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::None) .unwrap(); diff --git a/tests/rate_limited.rs b/tests/rate_limited.rs index 8c89708..78e322d 100644 --- a/tests/rate_limited.rs +++ b/tests/rate_limited.rs @@ -53,10 +53,6 @@ fn it_loads() { .expect_log(Some(LogLevel::Debug), Some("#2 on_http_request_headers")) .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some(":authority")) .returning(Some("cars.toystore.com")) - .expect_log( - Some(LogLevel::Debug), - Some("#2 allowing request to pass because zero descriptors generated"), - ) .execute_and_expect(ReturnType::Action(Action::Continue)) .unwrap(); @@ -168,6 +164,10 @@ fn it_limits() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -186,7 +186,7 @@ fn it_limits() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -200,6 +200,11 @@ fn it_limits() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::None) .unwrap(); @@ -311,6 +316,10 @@ fn it_passes_additional_headers() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -329,7 +338,7 @@ fn it_passes_additional_headers() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -347,6 +356,12 @@ fn it_passes_additional_headers() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: AddHeaders")) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::None) .unwrap(); @@ -444,6 +459,10 @@ fn it_rate_limits_with_empty_predicates() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -462,7 +481,7 @@ fn it_rate_limits_with_empty_predicates() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -476,6 +495,11 @@ fn it_rate_limits_with_empty_predicates() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::None) .unwrap(); @@ -568,8 +592,9 @@ fn it_does_not_rate_limits_when_predicates_does_not_match() { .returning(Some(data::request::path::ADMIN)) .expect_log( Some(LogLevel::Debug), - Some("grpc_message_request: empty descriptors"), + Some("build_message(rl): empty descriptors"), ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::Action(Action::Continue)) .unwrap(); @@ -682,6 +707,10 @@ fn it_folds_subsequent_actions_to_limitador_into_a_single_one() { Some(LogLevel::Debug), Some("#2 action_set selected some-name"), ) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -700,7 +729,7 @@ fn it_folds_subsequent_actions_to_limitador_into_a_single_one() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -714,6 +743,11 @@ fn it_folds_subsequent_actions_to_limitador_into_a_single_one() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::None) .unwrap(); diff --git a/tests/remote_address.rs b/tests/remote_address.rs index 3d25e99..dc75a14 100644 --- a/tests/remote_address.rs +++ b/tests/remote_address.rs @@ -102,6 +102,10 @@ fn it_limits_based_on_source_address() { ) .expect_get_property(Some(vec!["source", "address"])) .returning(Some(data::source::ADDRESS)) + .expect_log( + Some(LogLevel::Debug), + Some("handle_operation: SendGrpcRequest"), + ) // retrieving tracing headers .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) .returning(None) @@ -124,7 +128,7 @@ fn it_limits_based_on_source_address() { .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), - Some("#2 initiated gRPC call (id# 42)"), + Some("handle_operation: AwaitGrpcResponse"), ) .execute_and_expect(ReturnType::Action(Action::Pause)) .unwrap(); @@ -138,6 +142,11 @@ fn it_limits_based_on_source_address() { ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) + .expect_log( + Some(LogLevel::Debug), + Some("process_response(rl): received OK response"), + ) + .expect_log(Some(LogLevel::Debug), Some("handle_operation: Done")) .execute_and_expect(ReturnType::None) .unwrap();