diff --git a/src/configuration.rs b/src/configuration.rs index cefee85..a9c5dda 100644 --- a/src/configuration.rs +++ b/src/configuration.rs @@ -8,7 +8,6 @@ use crate::configuration::action_set::ActionSet; use crate::configuration::action_set_index::ActionSetIndex; use crate::data; use crate::data::Predicate; -use crate::data::PropertyPath; use crate::service::GrpcService; use cel_interpreter::functions::duration; use cel_interpreter::Value; @@ -37,39 +36,6 @@ impl ExpressionItem { } } -#[derive(Deserialize, Debug, Clone)] -pub struct SelectorItem { - // Selector of an attribute from the contextual properties provided by kuadrant - // during request and connection processing - pub selector: String, - - // If not set it defaults to `selector` field value as the descriptor key. - #[serde(default)] - pub key: Option, - - // An optional value to use if the selector is not found in the context. - // If not set and the selector is not found in the context, then no data is generated. - #[serde(default)] - pub default: Option, - - #[serde(skip_deserializing)] - path: OnceCell, -} - -impl SelectorItem { - pub fn compile(&self) -> Result<(), String> { - self.path - .set(self.selector.as_str().into()) - .map_err(|p| format!("Err on {p:?}")) - } - - pub fn path(&self) -> &PropertyPath { - self.path - .get() - .expect("SelectorItem wasn't previously compiled!") - } -} - #[derive(Deserialize, Debug, Clone)] pub struct StaticItem { pub value: String, @@ -81,7 +47,6 @@ pub struct StaticItem { #[serde(rename_all = "lowercase")] pub enum DataType { Static(StaticItem), - Selector(SelectorItem), Expression(ExpressionItem), } @@ -89,7 +54,6 @@ impl DataType { pub fn compile(&self) -> Result<(), String> { match self { DataType::Static(_) => Ok(()), - DataType::Selector(selector) => selector.compile(), DataType::Expression(exp) => exp.compile(), } } @@ -405,118 +369,7 @@ mod test { } #[test] - fn parse_config_data_selector() { - let config = r#"{ - "services": { - "limitador": { - "type": "ratelimit", - "endpoint": "limitador-cluster", - "failureMode": "deny" - } - }, - "actionSets": [ - { - "name": "rlp-ns-A/rlp-name-A", - "routeRuleConditions": { - "hostnames": ["*.toystore.com", "example.com"] - }, - "actions": [ - { - "service": "limitador", - "scope": "rlp-ns-A/rlp-name-A", - "data": [ - { - "selector": { - "selector": "my.selector.path", - "key": "mykey", - "default": "my_selector_default_value" - } - }] - }] - }] - }"#; - let res = serde_json::from_str::(config); - if let Err(ref e) = res { - eprintln!("{e}"); - } - assert!(res.is_ok()); - - let filter_config = res.unwrap(); - assert_eq!(filter_config.action_sets.len(), 1); - - let actions = &filter_config.action_sets[0].actions; - assert_eq!(actions.len(), 1); - - let data_items = &actions[0].data; - assert_eq!(data_items.len(), 1); - - if let DataType::Selector(selector_item) = &data_items[0].item { - assert_eq!(selector_item.selector, "my.selector.path"); - assert_eq!(selector_item.key.as_ref().unwrap(), "mykey"); - assert_eq!( - selector_item.default.as_ref().unwrap(), - "my_selector_default_value" - ); - } else { - panic!(); - } - } - - #[test] - fn parse_config_condition_selector_operators() { - let config = r#"{ - "services": { - "limitador": { - "type": "ratelimit", - "endpoint": "limitador-cluster", - "failureMode": "deny" - } - }, - "actionSets": [ - { - "name": "rlp-ns-A/rlp-name-A", - "routeRuleConditions": { - "hostnames": ["*.toystore.com", "example.com"], - "predicates": [ - "request.path == '/admin/toy'", - "request.method != 'POST'", - "request.host.startsWith('cars.')", - "request.host.endsWith('.com')", - "request.host.matches('*.com')" - ] - }, - "actions": [ - { - "service": "limitador", - "scope": "rlp-ns-A/rlp-name-A", - "predicates": [ - "auth.metadata.username == 'alice'", - "request.path.endsWith('/car')" - ], - "data": [ { "selector": { "selector": "my.selector.path" } }] - }] - }] - }"#; - let res = serde_json::from_str::(config); - if let Err(ref e) = res { - eprintln!("{e}"); - } - assert!(res.is_ok()); - - let filter_config = res.unwrap(); - assert_eq!(filter_config.action_sets.len(), 1); - - let predicates = &filter_config.action_sets[0] - .route_rule_conditions - .predicates; - assert_eq!(predicates.len(), 5); - - let predicates = &filter_config.action_sets[0].actions[0].predicates; - assert_eq!(predicates.len(), 2); - } - - #[test] - fn parse_config_conditions_optional() { + fn parse_config_predicates_optional() { let config = r#"{ "services": { "limitador": { @@ -543,8 +396,9 @@ mod test { } }, { - "selector": { - "selector": "auth.metadata.username" + "expression": { + "key": "username", + "value": "auth.metadata.username" } }] }] @@ -569,6 +423,12 @@ mod test { .route_rule_conditions .predicates; assert_eq!(predicates.len(), 0); + + let actions = &filter_config.action_sets[0].actions; + assert_eq!(actions.len(), 1); + + let action_predicates = &actions[0].predicates; + assert_eq!(action_predicates.len(), 0); } #[test] @@ -598,8 +458,9 @@ mod test { "key": "rlp-ns-A/rlp-name-A", "value": "1" }, - "selector": { - "selector": "auth.metadata.username" + "expression": { + "key": "username", + "value": "auth.metadata.username" } }] }] diff --git a/src/configuration/action.rs b/src/configuration/action.rs index b61bdda..58e893f 100644 --- a/src/configuration/action.rs +++ b/src/configuration/action.rs @@ -2,7 +2,7 @@ use crate::configuration::{DataItem, DataType}; use crate::data::Predicate; use crate::envoy::{RateLimitDescriptor, RateLimitDescriptor_Entry}; use cel_interpreter::Value; -use log::{debug, error}; +use log::error; use protobuf::RepeatedField; use serde::Deserialize; use std::cell::OnceCell; @@ -80,37 +80,6 @@ impl Action { } }, ), - DataType::Selector(selector_item) => { - let descriptor_key = match &selector_item.key { - None => selector_item.path().to_string(), - Some(key) => key.to_owned(), - }; - - let value = match crate::data::get_attribute::(selector_item.path()) - .expect("Error!") - { - //TODO(didierofrivia): Replace hostcalls by DI - None => { - debug!( - "build_single_descriptor: selector not found: {}", - selector_item.path() - ); - match &selector_item.default { - None => return None, // skipping the entire descriptor - Some(default_value) => default_value.clone(), - } - } - // TODO(eastizle): not all fields are strings - // https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/advanced/attributes - Some(attr_str) => attr_str, - // Alternative implementation (for rust >= 1.76) - // Attribute::parse(attribute_bytes) - // .inspect_err(|e| debug!("#{} build_single_descriptor: failed to parse selector value: {}, error: {}", - // filter.context_id, attribute_path, e)) - // .ok()?, - }; - (descriptor_key, value) - } }; let mut descriptor_entry = RateLimitDescriptor_Entry::new(); descriptor_entry.set_key(key); diff --git a/tests/rate_limited.rs b/tests/rate_limited.rs index 5ae17e1..f0fd84f 100644 --- a/tests/rate_limited.rs +++ b/tests/rate_limited.rs @@ -374,7 +374,7 @@ fn it_passes_additional_headers() { #[test] #[serial] -fn it_rate_limits_with_empty_conditions() { +fn it_rate_limits_with_empty_predicates() { let args = tester::MockSettings { wasm_path: wasm_module(), quiet: false, @@ -496,108 +496,7 @@ fn it_rate_limits_with_empty_conditions() { #[test] #[serial] -fn it_does_not_rate_limits_when_selector_does_not_exist_and_misses_default_value() { - let args = tester::MockSettings { - wasm_path: wasm_module(), - quiet: false, - allow_unexpected: false, - }; - let mut module = tester::mock(args).unwrap(); - - module - .call_start() - .execute_and_expect(ReturnType::None) - .unwrap(); - - let root_context = 1; - let cfg = r#"{ - "services": { - "limitador": { - "type": "ratelimit", - "endpoint": "limitador-cluster", - "failureMode": "deny", - "timeout": "5s" - } - }, - "actionSets": [ - { - "name": "some-name", - "routeRuleConditions": { - "hostnames": ["*.com"] - }, - "actions": [ - { - "service": "limitador", - "scope": "RLS-domain", - "data": [ - { - "selector": { - "selector": "unknown.path" - } - } - ] - }] - }] - }"#; - - module - .call_proxy_on_context_create(root_context, 0) - .expect_log(Some(LogLevel::Info), Some("#1 set_root_context")) - .execute_and_expect(ReturnType::None) - .unwrap(); - module - .call_proxy_on_configure(root_context, 0) - .expect_log(Some(LogLevel::Info), Some("#1 on_configure")) - .expect_get_buffer_bytes(Some(BufferType::PluginConfiguration)) - .returning(Some(cfg.as_bytes())) - .expect_log(Some(LogLevel::Info), None) - .execute_and_expect(ReturnType::Bool(true)) - .unwrap(); - - let http_context = 2; - module - .call_proxy_on_context_create(http_context, root_context) - .expect_log(Some(LogLevel::Debug), Some("#2 create_http_context")) - .execute_and_expect(ReturnType::None) - .unwrap(); - - module - .call_proxy_on_request_headers(http_context, 0, false) - .expect_log(Some(LogLevel::Debug), Some("#2 on_http_request_headers")) - .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some(":authority")) - .returning(Some("a.com")) - .expect_log( - Some(LogLevel::Debug), - Some("#2 action_set selected some-name"), - ) - // retrieving properties for RateLimitRequest - .expect_log( - Some(LogLevel::Debug), - Some("get_property: path: [\"unknown\", \"path\"]"), - ) - .expect_get_property(Some(vec!["unknown", "path"])) - .returning(None) - .expect_log( - Some(LogLevel::Debug), - Some("build_single_descriptor: selector not found: unknown.path"), - ) - .expect_log( - Some(LogLevel::Debug), - Some("grpc_message_request: empty descriptors"), - ) - .execute_and_expect(ReturnType::Action(Action::Continue)) - .unwrap(); - - module - .call_proxy_on_response_headers(http_context, 0, false) - .expect_log(Some(LogLevel::Debug), Some("#2 on_http_response_headers")) - .execute_and_expect(ReturnType::Action(Action::Continue)) - .unwrap(); -} - -#[test] -#[serial] -fn it_does_not_rate_limits_when_condition_does_not_match() { +fn it_does_not_rate_limits_when_predicates_does_not_match() { let args = tester::MockSettings { wasm_path: wasm_module(), quiet: false,