From 7951b10aa83626487100cd4031478edadd05c44c Mon Sep 17 00:00:00 2001 From: dd di cesare Date: Wed, 25 Sep 2024 16:36:43 +0200 Subject: [PATCH 1/2] [refactor] `Actions` inside `Rules` * Building the descriptors are not relying on Filter anymore * Getting the properties for the descriptors directly using `hostcalls::get_property` * Since Filter is not passed, we are not logging `context_id` anymore Signed-off-by: dd di cesare --- src/configuration.rs | 213 +++++++++++++++++-------------------------- src/policy.rs | 79 +++++++--------- src/policy_index.rs | 8 +- 3 files changed, 118 insertions(+), 182 deletions(-) diff --git a/src/configuration.rs b/src/configuration.rs index e026254d..c171f854 100644 --- a/src/configuration.rs +++ b/src/configuration.rs @@ -462,10 +462,12 @@ impl TryFrom for FilterConfig { for rlp in config.policies.iter() { for rule in &rlp.rules { - for datum in &rule.data { - let result = datum.item.compile(); - if result.is_err() { - return Err(result.err().unwrap()); + for action in &rule.actions { + for datum in &action.data { + let result = datum.item.compile(); + if result.is_err() { + return Err(result.err().unwrap()); + } } } for condition in &rule.conditions { @@ -534,7 +536,7 @@ pub struct Extension { pub struct Action { pub extension: String, #[allow(dead_code)] - pub data: DataType, + pub data: Vec, } #[cfg(test)] @@ -575,30 +577,24 @@ mod test { "value": "cars.toystore.com" }] }], - "data": [ - { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - }, - { - "selector": { - "selector": "auth.metadata.username" - } - }] - }], - "actions": [ + "actions": [ { "extension": "limitador", - "data": { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" + "data": [ + { + "static": { + "key": "rlp-ns-A/rlp-name-A", + "value": "1" + } + }, + { + "selector": { + "selector": "auth.metadata.username" + } } - } - } - ] + ] + }] + }] }] }"#; @@ -622,7 +618,7 @@ mod test { let all_of_conditions = &conditions[0].all_of; assert_eq!(all_of_conditions.len(), 3); - let data_items = &rules[0].data; + let data_items = &rules[0].actions[0].data; assert_eq!(data_items.len(), 2); // TODO(eastizle): DataItem does not implement PartialEq, add it only for testing? @@ -685,26 +681,19 @@ mod test { "hostnames": ["*.toystore.com", "example.com"], "rules": [ { - "data": [ + "actions": [ { - "selector": { - "selector": "my.selector.path", - "key": "mykey", - "default": "my_selector_default_value" - } + "extension": "limitador", + "data": [ + { + "selector": { + "selector": "my.selector.path", + "key": "mykey", + "default": "my_selector_default_value" + } + }] }] - }], - "actions": [ - { - "extension": "limitador", - "data": { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - } - } - ] + }] }] }"#; let res = serde_json::from_str::(config); @@ -719,7 +708,7 @@ mod test { let rules = &filter_config.policies[0].rules; assert_eq!(rules.len(), 1); - let data_items = &rules[0].data; + let data_items = &rules[0].actions[0].data; assert_eq!(data_items.len(), 1); if let DataType::Selector(selector_item) = &data_items[0].item { @@ -780,19 +769,12 @@ mod test { "value": "*.com" }] }], - "data": [ { "selector": { "selector": "my.selector.path" } }] - }], - "actions": [ - { - "extension": "limitador", - "data": { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - } - } - ] + "actions": [ + { + "extension": "limitador", + "data": [ { "selector": { "selector": "my.selector.path" } }] + }] + }] }] }"#; let res = serde_json::from_str::(config); @@ -846,30 +828,23 @@ mod test { "hostnames": ["*.toystore.com", "example.com"], "rules": [ { - "data": [ + "actions": [ { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - }, - { - "selector": { - "selector": "auth.metadata.username" - } + "extension": "limitador", + "data": [ + { + "static": { + "key": "rlp-ns-A/rlp-name-A", + "value": "1" + } + }, + { + "selector": { + "selector": "auth.metadata.username" + } + }] }] - }], - "actions": [ - { - "extension": "limitador", - "data": { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - } - } - ] + }] }] }"#; let res = serde_json::from_str::(config); @@ -906,28 +881,22 @@ mod test { "hostnames": ["*.toystore.com", "example.com"], "rules": [ { - "data": [ - { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - }, - "selector": { - "selector": "auth.metadata.username" - } - }] - }], - "actions": [ - { - "extension": "limitador", - "data": { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } + "actions": [ + { + "extension": "limitador", + "data": [ + { + "static": { + "key": "rlp-ns-A/rlp-name-A", + "value": "1" + }, + "selector": { + "selector": "auth.metadata.username" + } + }] } - } - ] + ] + }] }] }"#; let res = serde_json::from_str::(bad_config); @@ -950,25 +919,18 @@ mod test { "hostnames": ["*.toystore.com", "example.com"], "rules": [ { - "data": [ - { - "unknown": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - }] - }], - "actions": [ + "actions": [ { "extension": "limitador", - "data": { - "static": { + "data": [ + { + "unknown": { "key": "rlp-ns-A/rlp-name-A", "value": "1" } - } - } - ] + }] + }] + }] }] }"#; let res = serde_json::from_str::(bad_config); @@ -999,19 +961,12 @@ mod test { "value": "/admin/toy" }] }], - "data": [ { "selector": { "selector": "my.selector.path" } }] - }], - "actions": [ - { - "extension": "limitador", - "data": { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - } - } - ] + "actions": [ + { + "extension": "limitador", + "data": [ { "selector": { "selector": "my.selector.path" } }] + }] + }] }] }"#; let res = serde_json::from_str::(bad_config); diff --git a/src/policy.rs b/src/policy.rs index 353928ea..ae90fe21 100644 --- a/src/policy.rs +++ b/src/policy.rs @@ -1,9 +1,9 @@ use crate::attribute::Attribute; use crate::configuration::{Action, DataItem, DataType, PatternExpression}; use crate::envoy::{RateLimitDescriptor, RateLimitDescriptor_Entry}; -use crate::filter::http_context::Filter; use log::debug; -use proxy_wasm::traits::Context; +use protobuf::RepeatedField; +use proxy_wasm::hostcalls; use serde::Deserialize; #[derive(Deserialize, Debug, Clone)] @@ -18,7 +18,7 @@ pub struct Rule { #[serde(default)] pub conditions: Vec, // - pub data: Vec, + pub actions: Vec, } #[derive(Deserialize, Debug, Clone)] @@ -28,41 +28,33 @@ pub struct Policy { pub domain: String, pub hostnames: Vec, pub rules: Vec, - pub actions: Vec, } impl Policy { #[cfg(test)] - pub fn new( - name: String, - domain: String, - hostnames: Vec, - rules: Vec, - actions: Vec, - ) -> Self { + pub fn new(name: String, domain: String, hostnames: Vec, rules: Vec) -> Self { Policy { name, domain, hostnames, rules, - actions, } } - pub fn build_descriptors( - &self, - filter: &Filter, - ) -> protobuf::RepeatedField { + pub fn find_rule_that_applies(&self) -> Option<&Rule> { self.rules .iter() - .filter(|rule: &&Rule| self.filter_rule_by_conditions(filter, &rule.conditions)) - // Mapping 1 Rule -> 1 Descriptor - // Filter out empty descriptors - .filter_map(|rule| self.build_single_descriptor(filter, &rule.data)) + .find(|rule: &&Rule| self.filter_rule_by_conditions(&rule.conditions)) + } + + pub fn build_descriptors(&self, rule: &Rule) -> RepeatedField { + rule.actions + .iter() + .filter_map(|action| self.build_single_descriptor(&action.data)) .collect() } - fn filter_rule_by_conditions(&self, filter: &Filter, conditions: &[Condition]) -> bool { + fn filter_rule_by_conditions(&self, conditions: &[Condition]) -> bool { if conditions.is_empty() { // no conditions is equivalent to matching all the requests. return true; @@ -70,27 +62,28 @@ impl Policy { conditions .iter() - .any(|condition| self.condition_applies(filter, condition)) + .any(|condition| self.condition_applies(condition)) } - fn condition_applies(&self, filter: &Filter, condition: &Condition) -> bool { + fn condition_applies(&self, condition: &Condition) -> bool { condition .all_of .iter() - .all(|pattern_expression| self.pattern_expression_applies(filter, pattern_expression)) + .all(|pattern_expression| self.pattern_expression_applies(pattern_expression)) } - fn pattern_expression_applies(&self, filter: &Filter, p_e: &PatternExpression) -> bool { + fn pattern_expression_applies(&self, p_e: &PatternExpression) -> bool { let attribute_path = p_e.path(); debug!( - "#{} get_property: selector: {} path: {:?}", - filter.context_id, p_e.selector, attribute_path + "get_property: selector: {} path: {:?}", + p_e.selector, attribute_path ); - let attribute_value = match filter.get_property(attribute_path) { + let attribute_value = match hostcalls::get_property(attribute_path).unwrap() { + //TODO(didierofrivia): Replace hostcalls by DI None => { debug!( - "#{} pattern_expression_applies: selector not found: {}, defaulting to ``", - filter.context_id, p_e.selector + "pattern_expression_applies: selector not found: {}, defaulting to ``", + p_e.selector ); b"".to_vec() } @@ -98,21 +91,14 @@ impl Policy { }; match p_e.eval(attribute_value) { Err(e) => { - debug!( - "#{} pattern_expression_applies failed: {}", - filter.context_id, e - ); + debug!("pattern_expression_applies failed: {}", e); false } Ok(result) => result, } } - fn build_single_descriptor( - &self, - filter: &Filter, - data_list: &[DataItem], - ) -> Option { + fn build_single_descriptor(&self, data_list: &[DataItem]) -> Option { let mut entries = ::protobuf::RepeatedField::default(); // iterate over data items to allow any data item to skip the entire descriptor @@ -132,14 +118,15 @@ impl Policy { let attribute_path = selector_item.path(); debug!( - "#{} get_property: selector: {} path: {:?}", - filter.context_id, selector_item.selector, attribute_path + "get_property: selector: {} path: {:?}", + selector_item.selector, attribute_path ); - let value = match filter.get_property(attribute_path.tokens()) { + let value = match hostcalls::get_property(attribute_path.tokens()).unwrap() { + //TODO(didierofrivia): Replace hostcalls by DI None => { debug!( - "#{} build_single_descriptor: selector not found: {}", - filter.context_id, attribute_path + "build_single_descriptor: selector not found: {}", + attribute_path ); match &selector_item.default { None => return None, // skipping the entire descriptor @@ -151,8 +138,8 @@ impl Policy { Some(attribute_bytes) => match Attribute::parse(attribute_bytes) { Ok(attr_str) => attr_str, Err(e) => { - debug!("#{} build_single_descriptor: failed to parse selector value: {}, error: {}", - filter.context_id, attribute_path, e); + debug!("build_single_descriptor: failed to parse selector value: {}, error: {}", + attribute_path, e); return None; } }, diff --git a/src/policy_index.rs b/src/policy_index.rs index 3620179e..58b31d94 100644 --- a/src/policy_index.rs +++ b/src/policy_index.rs @@ -41,13 +41,7 @@ mod tests { use crate::policy_index::PolicyIndex; fn build_ratelimit_policy(name: &str) -> Policy { - Policy::new( - name.to_owned(), - "".to_owned(), - Vec::new(), - Vec::new(), - Vec::new(), - ) + Policy::new(name.to_owned(), "".to_owned(), Vec::new(), Vec::new()) } #[test] From 47cb63d26202f75c1b2738bb5f2c8743c9e82ace Mon Sep 17 00:00:00 2001 From: dd di cesare Date: Wed, 25 Sep 2024 16:38:34 +0200 Subject: [PATCH 2/2] [refactor] Wiring everything together Signed-off-by: dd di cesare --- src/filter/http_context.rs | 21 +++--- src/operation_dispatcher.rs | 9 +-- tests/rate_limited.rs | 130 +++++++++++++++--------------------- 3 files changed, 69 insertions(+), 91 deletions(-) diff --git a/src/filter/http_context.rs b/src/filter/http_context.rs index 1680540c..4a379193 100644 --- a/src/filter/http_context.rs +++ b/src/filter/http_context.rs @@ -30,18 +30,21 @@ impl Filter { } fn process_policy(&self, policy: &Policy) -> Action { - let descriptors = policy.build_descriptors(self); - if descriptors.is_empty() { - debug!( - "#{} process_rate_limit_policy: empty descriptors", - self.context_id - ); + if let Some(rule) = policy.find_rule_that_applies() { + //TODO: Move the building of descriptors at operation trigger time + let descriptors = policy.build_descriptors(rule); + if descriptors.is_empty() { + debug!("#{} process_policy: empty descriptors", self.context_id); + return Action::Continue; + } + + self.operation_dispatcher + .build_operations(policy.domain.clone(), rule, descriptors); + } else { + debug!("#{} process_policy: no rule applied", self.context_id); return Action::Continue; } - self.operation_dispatcher - .build_operations(policy, descriptors); - if let Some(operation) = self.operation_dispatcher.next() { match operation.get_result() { Ok(call_id) => { diff --git a/src/operation_dispatcher.rs b/src/operation_dispatcher.rs index 764a3d08..312c182e 100644 --- a/src/operation_dispatcher.rs +++ b/src/operation_dispatcher.rs @@ -1,6 +1,6 @@ use crate::configuration::{Extension, ExtensionType, FailureMode}; use crate::envoy::RateLimitDescriptor; -use crate::policy::Policy; +use crate::policy::Rule; use crate::service::grpc_message::GrpcMessageRequest; use crate::service::{GetMapValuesBytesFn, GrpcCallFn, GrpcServiceHandler}; use protobuf::RepeatedField; @@ -122,16 +122,17 @@ impl OperationDispatcher { pub fn build_operations( &self, - policy: &Policy, + domain: String, // TODO(didierofrivia): See if can work with &str + rule: &Rule, descriptors: RepeatedField, ) { let mut operations: Vec = vec![]; - policy.actions.iter().for_each(|action| { + rule.actions.iter().for_each(|action| { // TODO(didierofrivia): Error handling if let Some(service) = self.service_handlers.get(&action.extension) { let message = GrpcMessageRequest::new( service.get_extension_type(), - policy.domain.clone(), + domain.clone(), descriptors.clone(), ); operations.push(Operation::new( diff --git a/tests/rate_limited.rs b/tests/rate_limited.rs index 47ec8f99..3f98c9d7 100644 --- a/tests/rate_limited.rs +++ b/tests/rate_limited.rs @@ -124,26 +124,19 @@ fn it_limits() { }] } ], - "data": [ - { - "static": { - "key": "admin", - "value": "1" + "actions": [ + { + "extension": "limitador", + "data": [ + { + "static": { + "key": "admin", + "value": "1" + } + }] } - } ] - }], - "actions": [ - { - "extension": "limitador", - "data": { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - } - } - ] + }] }] }"#; @@ -191,15 +184,15 @@ fn it_limits() { ) .expect_log( Some(LogLevel::Debug), - Some("#2 get_property: selector: request.url_path path: [\"request\", \"url_path\"]"), + Some("get_property: selector: request.url_path path: [\"request\", \"url_path\"]"), ) .expect_log( Some(LogLevel::Debug), - Some("#2 get_property: selector: request.host path: [\"request\", \"host\"]"), + Some("get_property: selector: request.host path: [\"request\", \"host\"]"), ) .expect_log( Some(LogLevel::Debug), - Some("#2 get_property: selector: request.method path: [\"request\", \"method\"]"), + Some("get_property: selector: request.method path: [\"request\", \"method\"]"), ) .expect_grpc_call( Some("limitador-cluster"), @@ -290,26 +283,20 @@ fn it_passes_additional_headers() { }] } ], - "data": [ - { - "static": { - "key": "admin", - "value": "1" + "actions": [ + { + "extension": "limitador", + "data": [ + { + "static": { + "key": "admin", + "value": "1" + } + } + ] } - } ] - }], - "actions": [ - { - "extension": "limitador", - "data": { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - } - } - ] + }] }] }"#; @@ -357,15 +344,15 @@ fn it_passes_additional_headers() { ) .expect_log( Some(LogLevel::Debug), - Some("#2 get_property: selector: request.url_path path: [\"request\", \"url_path\"]"), + Some("get_property: selector: request.url_path path: [\"request\", \"url_path\"]"), ) .expect_log( Some(LogLevel::Debug), - Some("#2 get_property: selector: request.host path: [\"request\", \"host\"]"), + Some("get_property: selector: request.host path: [\"request\", \"host\"]"), ) .expect_log( Some(LogLevel::Debug), - Some("#2 get_property: selector: request.method path: [\"request\", \"method\"]"), + Some("get_property: selector: request.method path: [\"request\", \"method\"]"), ) .expect_grpc_call( Some("limitador-cluster"), @@ -450,26 +437,19 @@ fn it_rate_limits_with_empty_conditions() { "hostnames": ["*.com"], "rules": [ { - "data": [ - { - "static": { - "key": "admin", - "value": "1" - } - } - ] - }], - "actions": [ + "actions": [ { "extension": "limitador", - "data": { + "data": [ + { "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" + "key": "admin", + "value": "1" } - } - } - ] + } + ] + }] + }] }] }"#; @@ -578,25 +558,19 @@ fn it_does_not_rate_limits_when_selector_does_not_exist_and_misses_default_value "hostnames": ["*.com"], "rules": [ { - "data": [ - { - "selector": { - "selector": "unknown.path" + "actions": [ + { + "extension": "limitador", + "data": [ + { + "selector": { + "selector": "unknown.path" + } + } + ] } - } ] - }], - "actions": [ - { - "extension": "limitador", - "data": { - "static": { - "key": "rlp-ns-A/rlp-name-A", - "value": "1" - } - } - } - ] + }] }] }"#; @@ -634,15 +608,15 @@ fn it_does_not_rate_limits_when_selector_does_not_exist_and_misses_default_value ) .expect_log( Some(LogLevel::Debug), - Some("#2 get_property: selector: unknown.path path: Path { tokens: [\"unknown\", \"path\"] }"), + Some("get_property: selector: unknown.path path: Path { tokens: [\"unknown\", \"path\"] }"), ) .expect_log( Some(LogLevel::Debug), - Some("#2 build_single_descriptor: selector not found: unknown.path"), + Some("build_single_descriptor: selector not found: unknown.path"), ) .expect_log( Some(LogLevel::Debug), - Some("#2 process_rate_limit_policy: empty descriptors"), + Some("#2 process_policy: empty descriptors"), ) .execute_and_expect(ReturnType::Action(Action::Continue)) .unwrap();