From cb5136adad4c2e048c8c77b2f4af7c70e828d0eb Mon Sep 17 00:00:00 2001 From: Eguzki Astiz Lezaun Date: Wed, 4 Dec 2024 15:53:43 +0100 Subject: [PATCH 1/4] integration test: it_runs_next_action_on_failure_when_failuremode_is_allow Signed-off-by: Eguzki Astiz Lezaun --- Cargo.lock | 4 +- tests/auth.rs | 8 +- tests/failuremode.rs | 168 ++++++++++++++++++++++++++++++++++++++++ tests/multi.rs | 10 +-- tests/rate_limited.rs | 8 +- tests/remote_address.rs | 2 +- 6 files changed, 184 insertions(+), 16 deletions(-) create mode 100644 tests/failuremode.rs diff --git a/Cargo.lock b/Cargo.lock index 4cf1694..544c508 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 3 +version = 4 [[package]] name = "addr2line" @@ -1314,7 +1314,7 @@ dependencies = [ [[package]] name = "proxy-wasm-test-framework" version = "0.1.0" -source = "git+https://github.com/Kuadrant/wasm-test-framework.git?branch=kuadrant#1bdc4d204d203342ff3d6d9b11b6ba69ee89465f" +source = "git+https://github.com/Kuadrant/wasm-test-framework.git?branch=kuadrant#bbc8079ee2cd078afaec6a0c077244ac7ad22d0a" dependencies = [ "anyhow", "lazy_static", diff --git a/tests/auth.rs b/tests/auth.rs index 9bf6db1..c11e533 100644 --- a/tests/auth.rs +++ b/tests/auth.rs @@ -179,7 +179,7 @@ fn it_auths() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), @@ -362,7 +362,7 @@ fn it_denies() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), @@ -563,7 +563,7 @@ fn it_does_not_fold_auth_actions() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), @@ -661,7 +661,7 @@ fn it_does_not_fold_auth_actions() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .execute_and_expect(ReturnType::None) .unwrap(); diff --git a/tests/failuremode.rs b/tests/failuremode.rs new file mode 100644 index 0000000..1a14882 --- /dev/null +++ b/tests/failuremode.rs @@ -0,0 +1,168 @@ +use crate::util::common::wasm_module; +use proxy_wasm_test_framework::tester; +use proxy_wasm_test_framework::types::{Action, BufferType, LogLevel, MapType, ReturnType}; +use serial_test::serial; + +pub mod util; + +#[test] +#[serial] +fn it_runs_next_action_on_failure_when_failuremode_is_allow() { + 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" + }, + "limitador-unreachable": { + "type": "ratelimit", + "endpoint": "unreachable-cluster", + "failureMode": "allow", + "timeout": "5s" + } + }, + "actionSets": [ + { + "name": "some-name", + "routeRuleConditions": { + "hostnames": ["example.com"] + }, + "actions": [ + { + "service": "limitador-unreachable", + "scope": "a", + "data": [ + { + "expression": { + "key": "l", + "value": "1" + } + } + ] + }, + { + "service": "limitador", + "scope": "a", + "data": [ + { + "expression": { + "key": "l", + "value": "1" + } + } + ] + }] + }] + }"#; + + 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(); + + let first_call_token_id = 42; + 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("example.com")) + .expect_log( + Some(LogLevel::Debug), + Some("#2 action_set selected some-name"), + ) + // retrieving tracing headers + .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) + .returning(None) + .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("tracestate")) + .returning(None) + .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("baggage")) + .returning(None) + .expect_grpc_call( + Some("unreachable-cluster"), + Some("envoy.service.ratelimit.v3.RateLimitService"), + Some("ShouldRateLimit"), + Some(&[0, 0, 0, 0]), + None, + Some(5000), + ) + .returning(Ok(first_call_token_id)) + .expect_log( + Some(LogLevel::Debug), + Some("#2 initiated gRPC call (id# 42)"), + ) + .execute_and_expect(ReturnType::Action(Action::Pause)) + .unwrap(); + + let status_code = 14; + module + .proxy_on_grpc_close(http_context, 42, status_code) + .expect_log( + 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_grpc_call( + Some("limitador-cluster"), + Some("envoy.service.ratelimit.v3.RateLimitService"), + Some("ShouldRateLimit"), + Some(&[0, 0, 0, 0]), + Some(&[ + 10, 1, 97, 18, 28, 10, 26, 10, 21, 108, 105, 109, 105, 116, 95, 116, 111, 95, 98, + 101, 95, 97, 99, 116, 105, 118, 97, 116, 101, 100, 18, 1, 49, 24, 1, + ]), + Some(5000), + ) + .returning(Ok(42)) + .execute_and_expect(ReturnType::None) + .unwrap(); + + let grpc_response: [u8; 2] = [8, 1]; + module + .call_proxy_on_grpc_receive(http_context, 42, grpc_response.len() as i32) + .expect_log( + Some(LogLevel::Debug), + Some("#2 on_grpc_call_response: received gRPC call response: token: 42, status: 0"), + ) + .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) + .returning(Some(&grpc_response)) + .execute_and_expect(ReturnType::None) + .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(); +} diff --git a/tests/multi.rs b/tests/multi.rs index d010402..4f9c294 100644 --- a/tests/multi.rs +++ b/tests/multi.rs @@ -196,7 +196,7 @@ fn it_performs_authenticated_rate_limiting() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), @@ -225,7 +225,7 @@ fn it_performs_authenticated_rate_limiting() { None, Some(5000), ) - .returning(Some(43)) + .returning(Ok(43)) .execute_and_expect(ReturnType::None) .unwrap(); @@ -393,7 +393,7 @@ fn unauthenticated_does_not_ratelimit() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), @@ -653,7 +653,7 @@ fn authenticated_one_ratelimit_action_matches() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), @@ -695,7 +695,7 @@ fn authenticated_one_ratelimit_action_matches() { None, Some(5000), ) - .returning(Some(43)) + .returning(Ok(43)) .execute_and_expect(ReturnType::None) .unwrap(); diff --git a/tests/rate_limited.rs b/tests/rate_limited.rs index 272cf07..8c89708 100644 --- a/tests/rate_limited.rs +++ b/tests/rate_limited.rs @@ -183,7 +183,7 @@ fn it_limits() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), @@ -326,7 +326,7 @@ fn it_passes_additional_headers() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), @@ -459,7 +459,7 @@ fn it_rate_limits_with_empty_predicates() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), @@ -697,7 +697,7 @@ fn it_folds_subsequent_actions_to_limitador_into_a_single_one() { None, Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), diff --git a/tests/remote_address.rs b/tests/remote_address.rs index 4e6fe82..3d25e99 100644 --- a/tests/remote_address.rs +++ b/tests/remote_address.rs @@ -121,7 +121,7 @@ fn it_limits_based_on_source_address() { ]), Some(5000), ) - .returning(Some(42)) + .returning(Ok(42)) .expect_log( Some(LogLevel::Debug), Some("#2 initiated gRPC call (id# 42)"), From b9df0814f57393470982bc447bf2088d0db9f887 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 4 Dec 2024 17:43:27 +0000 Subject: [PATCH 2/4] Use None in the test Signed-off-by: Adam Cattermole --- Cargo.lock | 2 +- tests/failuremode.rs | 18 ++++++++++-------- 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 544c508..474f52f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1,6 +1,6 @@ # This file is automatically @generated by Cargo. # It is not intended for manual editing. -version = 4 +version = 3 [[package]] name = "addr2line" diff --git a/tests/failuremode.rs b/tests/failuremode.rs index 1a14882..5a3c997 100644 --- a/tests/failuremode.rs +++ b/tests/failuremode.rs @@ -125,8 +125,9 @@ fn it_runs_next_action_on_failure_when_failuremode_is_allow() { .unwrap(); let status_code = 14; + let second_call_token_id = 43; module - .proxy_on_grpc_close(http_context, 42, status_code) + .proxy_on_grpc_close(http_context, first_call_token_id as i32, status_code) .expect_log( Some(LogLevel::Debug), Some(format!("#2 on_grpc_call_response: received gRPC call response: token: {first_call_token_id}, status: {status_code}").as_str()), @@ -138,22 +139,23 @@ fn it_runs_next_action_on_failure_when_failuremode_is_allow() { Some("envoy.service.ratelimit.v3.RateLimitService"), Some("ShouldRateLimit"), Some(&[0, 0, 0, 0]), - Some(&[ - 10, 1, 97, 18, 28, 10, 26, 10, 21, 108, 105, 109, 105, 116, 95, 116, 111, 95, 98, - 101, 95, 97, 99, 116, 105, 118, 97, 116, 101, 100, 18, 1, 49, 24, 1, - ]), + None, Some(5000), ) - .returning(Ok(42)) + .returning(Ok(second_call_token_id)) .execute_and_expect(ReturnType::None) .unwrap(); let grpc_response: [u8; 2] = [8, 1]; module - .call_proxy_on_grpc_receive(http_context, 42, grpc_response.len() as i32) + .call_proxy_on_grpc_receive( + http_context, + second_call_token_id as i32, + grpc_response.len() as i32, + ) .expect_log( Some(LogLevel::Debug), - Some("#2 on_grpc_call_response: received gRPC call response: token: 42, status: 0"), + Some(format!("#2 on_grpc_call_response: received gRPC call response: token: {second_call_token_id}, status: 0").as_str()), ) .expect_get_buffer_bytes(Some(BufferType::GrpcReceiveBuffer)) .returning(Some(&grpc_response)) From c9cc4dc2ec3749b0f9955bb186238135dc70f0c8 Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Wed, 4 Dec 2024 15:27:15 +0000 Subject: [PATCH 3/4] Do not resume on failure Signed-off-by: Adam Cattermole --- src/filter/http_context.rs | 41 ++++++++++++++++++++++++++------------ src/service.rs | 4 +--- 2 files changed, 29 insertions(+), 16 deletions(-) diff --git a/src/filter/http_context.rs b/src/filter/http_context.rs index e7fd0d7..787df8e 100644 --- a/src/filter/http_context.rs +++ b/src/filter/http_context.rs @@ -82,6 +82,21 @@ impl Filter { } } } + + 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 { @@ -134,21 +149,21 @@ impl Context for Filter { match op_res { Ok(operation) => { - if let Ok(result) = GrpcService::process_grpc_response(operation, resp_size) { - // add the response headers - self.response_headers_to_add.extend(result.response_headers); - // call the next op - 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(); + 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(op_err) => { - // If desired, we could check the error status. - GrpcService::handle_error_on_grpc_response(op_err.failure_mode); - } } } } diff --git a/src/service.rs b/src/service.rs index 461ecc2..1145c1e 100644 --- a/src/service.rs +++ b/src/service.rs @@ -100,9 +100,7 @@ impl GrpcService { hostcalls::send_http_response(500, vec![], Some(b"Internal Server Error.\n")) .expect("failed to send_http_response 500"); } - FailureMode::Allow => { - hostcalls::resume_http_request().expect("failed to resume_http_request") - } + FailureMode::Allow => {} } } } From 335f615c7a7ffa7646db5024b93f2580fb18451b Mon Sep 17 00:00:00 2001 From: Adam Cattermole Date: Thu, 5 Dec 2024 10:27:33 +0000 Subject: [PATCH 4/4] Add test case for failuremode deny Signed-off-by: Adam Cattermole --- tests/failuremode.rs | 132 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 132 insertions(+) diff --git a/tests/failuremode.rs b/tests/failuremode.rs index 5a3c997..c05544f 100644 --- a/tests/failuremode.rs +++ b/tests/failuremode.rs @@ -168,3 +168,135 @@ fn it_runs_next_action_on_failure_when_failuremode_is_allow() { .execute_and_expect(ReturnType::Action(Action::Continue)) .unwrap(); } + +#[test] +#[serial] +fn it_stops_on_failure_when_failuremode_is_deny() { + 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" + }, + "limitador-unreachable": { + "type": "ratelimit", + "endpoint": "unreachable-cluster", + "failureMode": "deny", + "timeout": "5s" + } + }, + "actionSets": [ + { + "name": "some-name", + "routeRuleConditions": { + "hostnames": ["example.com"] + }, + "actions": [ + { + "service": "limitador-unreachable", + "scope": "a", + "data": [ + { + "expression": { + "key": "l", + "value": "1" + } + } + ] + }, + { + "service": "limitador", + "scope": "a", + "data": [ + { + "expression": { + "key": "l", + "value": "1" + } + } + ] + }] + }] + }"#; + + 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("example.com")) + .expect_log( + Some(LogLevel::Debug), + Some("#2 action_set selected some-name"), + ) + // retrieving tracing headers + .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("traceparent")) + .returning(None) + .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("tracestate")) + .returning(None) + .expect_get_header_map_value(Some(MapType::HttpRequestHeaders), Some("baggage")) + .returning(None) + .expect_grpc_call( + Some("unreachable-cluster"), + Some("envoy.service.ratelimit.v3.RateLimitService"), + Some("ShouldRateLimit"), + Some(&[0, 0, 0, 0]), + None, + Some(5000), + ) + .returning(Ok(42)) + .expect_log( + Some(LogLevel::Debug), + Some("#2 initiated gRPC call (id# 42)"), + ) + .execute_and_expect(ReturnType::Action(Action::Pause)) + .unwrap(); + + let status_code = 14; + module + .proxy_on_grpc_close(http_context, 42, status_code) + .expect_log( + 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_send_local_response(Some(500), None, None, None) + .execute_and_expect(ReturnType::None) + .unwrap(); +}