From 2b2006d56a3a44e78779c25327557db30f5ba5c5 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Fri, 9 Sep 2022 16:05:50 +0800 Subject: [PATCH 01/11] fix sendLocalResponse in wasm Signed-off-by: johnlanni --- source/extensions/common/wasm/context.cc | 8 ++++++++ source/extensions/common/wasm/context.h | 1 + 2 files changed, 9 insertions(+) diff --git a/source/extensions/common/wasm/context.cc b/source/extensions/common/wasm/context.cc index ec9ba89e6121..a257f05e69a3 100644 --- a/source/extensions/common/wasm/context.cc +++ b/source/extensions/common/wasm/context.cc @@ -1638,6 +1638,9 @@ void Context::failStream(WasmStreamType stream_type) { WasmResult Context::sendLocalResponse(uint32_t response_code, std::string_view body_text, Pairs additional_headers, uint32_t grpc_status, std::string_view details) { + if (local_reply_hold_) { + return WasmResult::BadArgument; + } // "additional_headers" is a collection of string_views. These will no longer // be valid when "modify_headers" is finally called below, so we must // make copies of all the headers. @@ -1662,10 +1665,15 @@ WasmResult Context::sendLocalResponse(uint32_t response_code, std::string_view b modify_headers = std::move(modify_headers), grpc_status, details = StringUtil::replaceAllEmptySpace( absl::string_view(details.data(), details.size()))] { + if (local_reply_sent_) { + return; + } decoder_callbacks_->sendLocalReply(static_cast(response_code), body_text, modify_headers, grpc_status, details); + local_reply_sent_ = true; }); } + local_reply_hold_ = true; return WasmResult::Ok; } diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h index f6e27d562cc8..a93f8498552e 100644 --- a/source/extensions/common/wasm/context.h +++ b/source/extensions/common/wasm/context.h @@ -437,6 +437,7 @@ class Context : public proxy_wasm::ContextBase, bool buffering_response_body_ = false; bool end_of_stream_ = false; bool local_reply_sent_ = false; + bool local_reply_hold_ = false; ProtobufWkt::Struct temporary_metadata_; // MB: must be a node-type map as we take persistent references to the entries. From 34b16f5ae5e0cdcc400ae4f3278511fff3e8adba Mon Sep 17 00:00:00 2001 From: johnlanni Date: Fri, 30 Sep 2022 09:56:23 +0800 Subject: [PATCH 02/11] add UT Signed-off-by: johnlanni --- .../common/wasm/test_data/test_context_cpp.cc | 68 +++++++++++++------ test/extensions/common/wasm/wasm_test.cc | 62 ++++++++++++++++- 2 files changed, 108 insertions(+), 22 deletions(-) diff --git a/test/extensions/common/wasm/test_data/test_context_cpp.cc b/test/extensions/common/wasm/test_data/test_context_cpp.cc index e790fd1011b3..dbd74e8c5658 100644 --- a/test/extensions/common/wasm/test_data/test_context_cpp.cc +++ b/test/extensions/common/wasm/test_data/test_context_cpp.cc @@ -13,11 +13,6 @@ START_WASM_PLUGIN(CommonWasmTestContextCpp) -class TestContext : public EnvoyContext { -public: - explicit TestContext(uint32_t id, RootContext* root) : EnvoyContext(id, root) {} -}; - class TestRootContext : public EnvoyRootContext { public: explicit TestRootContext(uint32_t id, std::string_view root_id) : EnvoyRootContext(id, root_id) {} @@ -28,17 +23,45 @@ class TestRootContext : public EnvoyRootContext { void onQueueReady(uint32_t) override; void onResolveDns(uint32_t token, uint32_t results_size) override; + std::string test_; + private: uint32_t dns_token_; }; +class TestContext : public EnvoyContext { +public: + explicit TestContext(uint32_t id, RootContext* root) : EnvoyContext(id, root) {} + FilterDataStatus onRequestBody(size_t body_buffer_length, bool end_of_stream) override; + FilterHeadersStatus onResponseHeaders(uint32_t, bool) override; + +private: + TestRootContext* root() { return static_cast(Context::root()); } +}; + static RegisterContextFactory register_TestContext(CONTEXT_FACTORY(TestContext), ROOT_FACTORY(TestRootContext)); static RegisterContextFactory register_EmptyTestContext(CONTEXT_FACTORY(EnvoyContext), ROOT_FACTORY(EnvoyRootContext), "empty"); -bool TestRootContext::onStart(size_t) { - envoy_resolve_dns("example.com", sizeof("example.com") - 1, &dns_token_); +FilterDataStatus TestContext::onRequestBody(size_t, bool) { + auto test = root()->test_; + if (test == "duplicate_local_reply") { + sendLocalResponse(200, "ok", "body", {}); + sendLocalResponse(200, "fail", "body", {}); + } + return FilterDataStatus::Continue; +} + +FilterHeadersStatus TestContext::onResponseHeaders(uint32_t, bool) { + return FilterHeadersStatus::Continue; +} + +bool TestRootContext::onStart(size_t configuration_size) { + test_ = getBufferBytes(WasmBufferType::VmConfiguration, 0, configuration_size)->toString(); + if (test_ == "dns_resolve") { + envoy_resolve_dns("example.com", sizeof("example.com") - 1, &dns_token_); + } return true; } @@ -58,24 +81,29 @@ bool TestRootContext::onDone() { // Null VM fails on nullptr. void TestRootContext::onTick() { - if (envoy_resolve_dns(nullptr, 1, &dns_token_) != WasmResult::InvalidMemoryAccess) { - logInfo("resolve_dns should report invalid memory access"); - } - if (envoy_resolve_dns("example.com", sizeof("example.com") - 1, nullptr) != - WasmResult::InvalidMemoryAccess) { - logInfo("resolve_dns should report invalid memory access"); + if (test_ == "dns_resolve") { + if (envoy_resolve_dns(nullptr, 1, &dns_token_) != WasmResult::InvalidMemoryAccess) { + logInfo("resolve_dns should report invalid memory access"); + } + if (envoy_resolve_dns("example.com", sizeof("example.com") - 1, nullptr) != + WasmResult::InvalidMemoryAccess) { + logInfo("resolve_dns should report invalid memory access"); + } } } // V8 fails on pointer too large. void TestRootContext::onQueueReady(uint32_t) { - if (envoy_resolve_dns(reinterpret_cast(INT_MAX), 0, &dns_token_) != - WasmResult::InvalidMemoryAccess) { - logInfo("resolve_dns should report invalid memory access"); - } - if (envoy_resolve_dns("example.com", sizeof("example.com") - 1, - reinterpret_cast(INT_MAX)) != WasmResult::InvalidMemoryAccess) { - logInfo("resolve_dns should report invalid memory access"); + if (test_ == "dns_resolve") { + if (envoy_resolve_dns(reinterpret_cast(INT_MAX), 0, &dns_token_) != + WasmResult::InvalidMemoryAccess) { + logInfo("resolve_dns should report invalid memory access"); + } + if (envoy_resolve_dns("example.com", sizeof("example.com") - 1, + reinterpret_cast(INT_MAX)) != + WasmResult::InvalidMemoryAccess) { + logInfo("resolve_dns should report invalid memory access"); + } } } diff --git a/test/extensions/common/wasm/wasm_test.cc b/test/extensions/common/wasm/wasm_test.cc index f739d8127eb2..ac11c74d7193 100644 --- a/test/extensions/common/wasm/wasm_test.cc +++ b/test/extensions/common/wasm/wasm_test.cc @@ -1349,7 +1349,7 @@ TEST_P(WasmCommonContextTest, OnDnsResolve) { .WillRepeatedly( testing::DoAll(testing::SaveArg<2>(&dns_callback), Return(&active_dns_query))); - setup(code, "context"); + setup(code, "dns_resolve"); setupContext(); EXPECT_CALL(rootContext(), log_(spdlog::level::warn, Eq("TestRootContext::onResolveDns 1"))); EXPECT_CALL(rootContext(), log_(spdlog::level::warn, Eq("TestRootContext::onResolveDns 2"))); @@ -1390,7 +1390,7 @@ TEST_P(WasmCommonContextTest, EmptyContext) { } EXPECT_FALSE(code.empty()); - setup(code, "context", "empty"); + setup(code, "dns_resolve", "empty"); setupContext(); root_context_->onResolveDns(0, Envoy::Network::DnsResolver::ResolutionStatus::Success, {}); @@ -1399,6 +1399,64 @@ TEST_P(WasmCommonContextTest, EmptyContext) { root_context_->validateConfiguration("", plugin_); } +class TestFilter : public Envoy::Extensions::Common::Wasm::Context { +public: + TestFilter(Wasm* wasm, uint32_t root_context_id, PluginHandleSharedPtr plugin_handle) + : Envoy::Extensions::Common::Wasm::Context(wasm, root_context_id, plugin_handle) {} + using ::Envoy::Extensions::Common::Wasm::Context::log; + proxy_wasm::WasmResult log(uint32_t, std::string_view message) override { + std::cerr << message << "\n"; + return proxy_wasm::WasmResult::Ok; + } +}; + +class WasmCommonHttpContextTest + : public Common::Wasm::WasmHttpFilterTestBase< + testing::TestWithParam>> { +public: + WasmCommonHttpContextTest() = default; + + void setup(const std::string& code, std::string vm_configuration, std::string root_id = "") { + setRootId(root_id); + setVmConfiguration(vm_configuration); + setupBase(std::get<0>(GetParam()), code, + [](Wasm* wasm, const std::shared_ptr& plugin) -> ContextBase* { + return new NiceMock(wasm, plugin); + }); + } + void setupFilter() { setupFilterBase(); } + + TestContext& rootContext() { return *static_cast(root_context_); } + + TestFilter& filter() { return *static_cast(context_.get()); } +}; + +INSTANTIATE_TEST_SUITE_P(Runtimes, WasmCommonHttpContextTest, + Envoy::Extensions::Common::Wasm::runtime_and_cpp_values); + +TEST_P(WasmCommonHttpContextTest, DuplicateLocalReply) { + std::string code; + if (std::get<0>(GetParam()) != "null") { + code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(absl::StrCat( + "{{ test_rundir }}/test/extensions/common/wasm/test_data/test_context_cpp.wasm"))); + } else { + // The name of the Null VM plugin. + code = "CommonWasmTestContextCpp"; + } + EXPECT_FALSE(code.empty()); + + setup(code, "duplicate_local_reply"); + setupFilter(); + EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) + .WillOnce([this](Http::ResponseHeaderMap&, bool) { filter().onResponseHeaders(0, false); }); + + EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(1); + + // Create in-VM context. + filter().onCreate(); + EXPECT_EQ(proxy_wasm::FilterDataStatus::StopIterationNoBuffer, filter().onRequestBody(0, false)); +} + } // namespace Wasm } // namespace Common } // namespace Extensions From 173d72f89b0967804fdf40f49e36356e744c35f3 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Fri, 30 Sep 2022 10:15:18 +0800 Subject: [PATCH 03/11] fix UT Signed-off-by: johnlanni --- test/extensions/common/wasm/wasm_test.cc | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/extensions/common/wasm/wasm_test.cc b/test/extensions/common/wasm/wasm_test.cc index ac11c74d7193..3aea9040fe26 100644 --- a/test/extensions/common/wasm/wasm_test.cc +++ b/test/extensions/common/wasm/wasm_test.cc @@ -1449,8 +1449,7 @@ TEST_P(WasmCommonHttpContextTest, DuplicateLocalReply) { setupFilter(); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .WillOnce([this](Http::ResponseHeaderMap&, bool) { filter().onResponseHeaders(0, false); }); - - EXPECT_CALL(decoder_callbacks_, sendLocalReply(_, _, _, _, _)).Times(1); + EXPECT_CALL(decoder_callbacks_, sendLocalReply(Envoy::Http::Code::OK, "body", _, _, "ok")); // Create in-VM context. filter().onCreate(); From 00c0f99f3d2f9eed3eed172ff387cc1ce5083fd3 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Fri, 30 Sep 2022 11:28:45 +0800 Subject: [PATCH 04/11] remove unused for clang tidy Signed-off-by: johnlanni --- source/extensions/common/wasm/context.h | 1 - 1 file changed, 1 deletion(-) diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h index a93f8498552e..48e600a9b722 100644 --- a/source/extensions/common/wasm/context.h +++ b/source/extensions/common/wasm/context.h @@ -27,7 +27,6 @@ namespace Wasm { using proxy_wasm::BufferInterface; using proxy_wasm::CloseType; -using proxy_wasm::ContextBase; using proxy_wasm::Pairs; using proxy_wasm::PairsWithStringValues; using proxy_wasm::PluginBase; From 72e2250e5eb540bd2f09c2913713bfad2e4ce181 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Sat, 1 Oct 2022 16:07:32 +0800 Subject: [PATCH 05/11] Revert "remove unused for clang tidy" This reverts commit 01d9b4cbbb77f5508a285590e6f83b98c9e19496. Signed-off-by: johnlanni --- source/extensions/common/wasm/context.h | 1 + 1 file changed, 1 insertion(+) diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h index 48e600a9b722..a93f8498552e 100644 --- a/source/extensions/common/wasm/context.h +++ b/source/extensions/common/wasm/context.h @@ -27,6 +27,7 @@ namespace Wasm { using proxy_wasm::BufferInterface; using proxy_wasm::CloseType; +using proxy_wasm::ContextBase; using proxy_wasm::Pairs; using proxy_wasm::PairsWithStringValues; using proxy_wasm::PluginBase; From af8370b2998676ec00703b259f29d2288ed642e2 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Sat, 1 Oct 2022 17:11:53 +0800 Subject: [PATCH 06/11] add more UT Signed-off-by: johnlanni --- .../common/wasm/test_data/test_context_cpp.cc | 8 +++-- test/extensions/common/wasm/wasm_test.cc | 32 +++++++++++++++++-- 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/test/extensions/common/wasm/test_data/test_context_cpp.cc b/test/extensions/common/wasm/test_data/test_context_cpp.cc index dbd74e8c5658..66a638d5ad48 100644 --- a/test/extensions/common/wasm/test_data/test_context_cpp.cc +++ b/test/extensions/common/wasm/test_data/test_context_cpp.cc @@ -46,9 +46,13 @@ static RegisterContextFactory register_EmptyTestContext(CONTEXT_FACTORY(EnvoyCon FilterDataStatus TestContext::onRequestBody(size_t, bool) { auto test = root()->test_; - if (test == "duplicate_local_reply") { + if (test == "send local reply twice") { sendLocalResponse(200, "ok", "body", {}); - sendLocalResponse(200, "fail", "body", {}); + sendLocalResponse(200, "not send", "body", {}); + } else if (test == "panic after sending local reply") { + sendLocalResponse(200, "not send", "body", {}); + int* badptr = nullptr; + *badptr = 0; } return FilterDataStatus::Continue; } diff --git a/test/extensions/common/wasm/wasm_test.cc b/test/extensions/common/wasm/wasm_test.cc index 3aea9040fe26..d56110312ff7 100644 --- a/test/extensions/common/wasm/wasm_test.cc +++ b/test/extensions/common/wasm/wasm_test.cc @@ -1434,6 +1434,7 @@ class WasmCommonHttpContextTest INSTANTIATE_TEST_SUITE_P(Runtimes, WasmCommonHttpContextTest, Envoy::Extensions::Common::Wasm::runtime_and_cpp_values); +// test that we don't send the local reply twice, even though it's specified in the wasm code TEST_P(WasmCommonHttpContextTest, DuplicateLocalReply) { std::string code; if (std::get<0>(GetParam()) != "null") { @@ -1445,11 +1446,38 @@ TEST_P(WasmCommonHttpContextTest, DuplicateLocalReply) { } EXPECT_FALSE(code.empty()); - setup(code, "duplicate_local_reply"); + setup(code, "send local reply twice"); setupFilter(); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .WillOnce([this](Http::ResponseHeaderMap&, bool) { filter().onResponseHeaders(0, false); }); - EXPECT_CALL(decoder_callbacks_, sendLocalReply(Envoy::Http::Code::OK, "body", _, _, "ok")); + EXPECT_CALL(decoder_callbacks_, + sendLocalReply(Envoy::Http::Code::OK, testing::Eq("body"), _, _, testing::Eq("ok"))); + + // Create in-VM context. + filter().onCreate(); + EXPECT_EQ(proxy_wasm::FilterDataStatus::StopIterationNoBuffer, filter().onRequestBody(0, false)); +} + +// test that we don't send the local reply twice when the wasm code panics +TEST_P(WasmCommonHttpContextTest, LocalReplyWhenPanic) { + std::string code; + if (std::get<0>(GetParam()) != "null") { + code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(absl::StrCat( + "{{ test_rundir }}/test/extensions/common/wasm/test_data/test_context_cpp.wasm"))); + } else { + // no need test the Null VM plugin. + return; + } + EXPECT_FALSE(code.empty()); + + setup(code, "panic after sending local reply"); + setupFilter(); + // In the case of VM failure, failStream is called, so we need to make sure that we don't send the + // local reply twice. + EXPECT_CALL(decoder_callbacks_, + sendLocalReply(Envoy::Http::Code::ServiceUnavailable, testing::Eq(""), _, + testing::Eq(Grpc::Status::WellKnownGrpcStatus::Unavailable), + testing::Eq("wasm_fail_stream"))); // Create in-VM context. filter().onCreate(); From 59847079c8f62e227ad9e6c27ad3d8f34f9f2bf8 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Sat, 1 Oct 2022 17:28:27 +0800 Subject: [PATCH 07/11] change setup params Signed-off-by: johnlanni --- test/extensions/common/wasm/test_data/test_context_cpp.cc | 6 +++--- test/extensions/common/wasm/wasm_test.cc | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/test/extensions/common/wasm/test_data/test_context_cpp.cc b/test/extensions/common/wasm/test_data/test_context_cpp.cc index 66a638d5ad48..2833d1f03f0c 100644 --- a/test/extensions/common/wasm/test_data/test_context_cpp.cc +++ b/test/extensions/common/wasm/test_data/test_context_cpp.cc @@ -63,7 +63,7 @@ FilterHeadersStatus TestContext::onResponseHeaders(uint32_t, bool) { bool TestRootContext::onStart(size_t configuration_size) { test_ = getBufferBytes(WasmBufferType::VmConfiguration, 0, configuration_size)->toString(); - if (test_ == "dns_resolve") { + if (test_ == "resolve dns") { envoy_resolve_dns("example.com", sizeof("example.com") - 1, &dns_token_); } return true; @@ -85,7 +85,7 @@ bool TestRootContext::onDone() { // Null VM fails on nullptr. void TestRootContext::onTick() { - if (test_ == "dns_resolve") { + if (test_ == "resolve dns") { if (envoy_resolve_dns(nullptr, 1, &dns_token_) != WasmResult::InvalidMemoryAccess) { logInfo("resolve_dns should report invalid memory access"); } @@ -98,7 +98,7 @@ void TestRootContext::onTick() { // V8 fails on pointer too large. void TestRootContext::onQueueReady(uint32_t) { - if (test_ == "dns_resolve") { + if (test_ == "resolve dns") { if (envoy_resolve_dns(reinterpret_cast(INT_MAX), 0, &dns_token_) != WasmResult::InvalidMemoryAccess) { logInfo("resolve_dns should report invalid memory access"); diff --git a/test/extensions/common/wasm/wasm_test.cc b/test/extensions/common/wasm/wasm_test.cc index d56110312ff7..1dfb894839e1 100644 --- a/test/extensions/common/wasm/wasm_test.cc +++ b/test/extensions/common/wasm/wasm_test.cc @@ -1349,7 +1349,7 @@ TEST_P(WasmCommonContextTest, OnDnsResolve) { .WillRepeatedly( testing::DoAll(testing::SaveArg<2>(&dns_callback), Return(&active_dns_query))); - setup(code, "dns_resolve"); + setup(code, "resolve dns"); setupContext(); EXPECT_CALL(rootContext(), log_(spdlog::level::warn, Eq("TestRootContext::onResolveDns 1"))); EXPECT_CALL(rootContext(), log_(spdlog::level::warn, Eq("TestRootContext::onResolveDns 2"))); @@ -1390,7 +1390,7 @@ TEST_P(WasmCommonContextTest, EmptyContext) { } EXPECT_FALSE(code.empty()); - setup(code, "dns_resolve", "empty"); + setup(code, "resolve dns", "empty"); setupContext(); root_context_->onResolveDns(0, Envoy::Network::DnsResolver::ResolutionStatus::Success, {}); From b37f92dd3efff374a4ee1abe91c1dc09903f316f Mon Sep 17 00:00:00 2001 From: johnlanni Date: Mon, 3 Oct 2022 13:21:34 +0800 Subject: [PATCH 08/11] optimize UT Signed-off-by: johnlanni --- .../common/wasm/test_data/test_context_cpp.cc | 115 ++++++++++-------- test/extensions/common/wasm/wasm_test.cc | 8 +- 2 files changed, 67 insertions(+), 56 deletions(-) diff --git a/test/extensions/common/wasm/test_data/test_context_cpp.cc b/test/extensions/common/wasm/test_data/test_context_cpp.cc index 2833d1f03f0c..78e28ce2b334 100644 --- a/test/extensions/common/wasm/test_data/test_context_cpp.cc +++ b/test/extensions/common/wasm/test_data/test_context_cpp.cc @@ -13,6 +13,11 @@ START_WASM_PLUGIN(CommonWasmTestContextCpp) +class TestContext : public EnvoyContext { +public: + explicit TestContext(uint32_t id, RootContext* root) : EnvoyContext(id, root) {} +}; + class TestRootContext : public EnvoyRootContext { public: explicit TestRootContext(uint32_t id, std::string_view root_id) : EnvoyRootContext(id, root_id) {} @@ -23,49 +28,17 @@ class TestRootContext : public EnvoyRootContext { void onQueueReady(uint32_t) override; void onResolveDns(uint32_t token, uint32_t results_size) override; - std::string test_; - private: uint32_t dns_token_; }; -class TestContext : public EnvoyContext { -public: - explicit TestContext(uint32_t id, RootContext* root) : EnvoyContext(id, root) {} - FilterDataStatus onRequestBody(size_t body_buffer_length, bool end_of_stream) override; - FilterHeadersStatus onResponseHeaders(uint32_t, bool) override; - -private: - TestRootContext* root() { return static_cast(Context::root()); } -}; - static RegisterContextFactory register_TestContext(CONTEXT_FACTORY(TestContext), ROOT_FACTORY(TestRootContext)); static RegisterContextFactory register_EmptyTestContext(CONTEXT_FACTORY(EnvoyContext), ROOT_FACTORY(EnvoyRootContext), "empty"); -FilterDataStatus TestContext::onRequestBody(size_t, bool) { - auto test = root()->test_; - if (test == "send local reply twice") { - sendLocalResponse(200, "ok", "body", {}); - sendLocalResponse(200, "not send", "body", {}); - } else if (test == "panic after sending local reply") { - sendLocalResponse(200, "not send", "body", {}); - int* badptr = nullptr; - *badptr = 0; - } - return FilterDataStatus::Continue; -} - -FilterHeadersStatus TestContext::onResponseHeaders(uint32_t, bool) { - return FilterHeadersStatus::Continue; -} - -bool TestRootContext::onStart(size_t configuration_size) { - test_ = getBufferBytes(WasmBufferType::VmConfiguration, 0, configuration_size)->toString(); - if (test_ == "resolve dns") { - envoy_resolve_dns("example.com", sizeof("example.com") - 1, &dns_token_); - } +bool TestRootContext::onStart(size_t) { + envoy_resolve_dns("example.com", sizeof("example.com") - 1, &dns_token_); return true; } @@ -85,30 +58,68 @@ bool TestRootContext::onDone() { // Null VM fails on nullptr. void TestRootContext::onTick() { - if (test_ == "resolve dns") { - if (envoy_resolve_dns(nullptr, 1, &dns_token_) != WasmResult::InvalidMemoryAccess) { - logInfo("resolve_dns should report invalid memory access"); - } - if (envoy_resolve_dns("example.com", sizeof("example.com") - 1, nullptr) != - WasmResult::InvalidMemoryAccess) { - logInfo("resolve_dns should report invalid memory access"); - } + if (envoy_resolve_dns(nullptr, 1, &dns_token_) != WasmResult::InvalidMemoryAccess) { + logInfo("resolve_dns should report invalid memory access"); + } + if (envoy_resolve_dns("example.com", sizeof("example.com") - 1, nullptr) != + WasmResult::InvalidMemoryAccess) { + logInfo("resolve_dns should report invalid memory access"); } } // V8 fails on pointer too large. void TestRootContext::onQueueReady(uint32_t) { - if (test_ == "resolve dns") { - if (envoy_resolve_dns(reinterpret_cast(INT_MAX), 0, &dns_token_) != - WasmResult::InvalidMemoryAccess) { - logInfo("resolve_dns should report invalid memory access"); - } - if (envoy_resolve_dns("example.com", sizeof("example.com") - 1, - reinterpret_cast(INT_MAX)) != - WasmResult::InvalidMemoryAccess) { - logInfo("resolve_dns should report invalid memory access"); - } + if (envoy_resolve_dns(reinterpret_cast(INT_MAX), 0, &dns_token_) != + WasmResult::InvalidMemoryAccess) { + logInfo("resolve_dns should report invalid memory access"); + } + if (envoy_resolve_dns("example.com", sizeof("example.com") - 1, + reinterpret_cast(INT_MAX)) != WasmResult::InvalidMemoryAccess) { + logInfo("resolve_dns should report invalid memory access"); } } +class TestCommonContext : public RootContext { +public: + explicit TestCommonContext(uint32_t id, std::string_view root_id) : RootContext(id, root_id) {} +}; + +class DupReplyContext : public Context { +public: + explicit DupReplyContext(uint32_t id, RootContext* root) : Context(id, root) {} + FilterDataStatus onRequestBody(size_t body_buffer_length, bool end_of_stream) override; + +private: + TestCommonContext* root() { return static_cast(Context::root()); } +}; + +FilterDataStatus DupReplyContext::onRequestBody(size_t, bool) { + sendLocalResponse(200, "ok", "body", {}); + sendLocalResponse(200, "not send", "body", {}); + return FilterDataStatus::Continue; +} + +class PanicReplyContext : public Context { +public: + explicit PanicReplyContext(uint32_t id, RootContext* root) : Context(id, root) {} + FilterDataStatus onRequestBody(size_t body_buffer_length, bool end_of_stream) override; + +private: + TestCommonContext* root() { return static_cast(Context::root()); } +}; + +FilterDataStatus PanicReplyContext::onRequestBody(size_t, bool) { + sendLocalResponse(200, "not send", "body", {}); + int* badptr = nullptr; + *badptr = 0; + return FilterDataStatus::Continue; +} + +static RegisterContextFactory register_DupReplyContext(CONTEXT_FACTORY(DupReplyContext), + ROOT_FACTORY(TestCommonContext), + "send local reply twice"); +static RegisterContextFactory register_PanicReplyContext(CONTEXT_FACTORY(PanicReplyContext), + ROOT_FACTORY(TestCommonContext), + "panic after sending local reply"); + END_WASM_PLUGIN diff --git a/test/extensions/common/wasm/wasm_test.cc b/test/extensions/common/wasm/wasm_test.cc index 1dfb894839e1..5675edb2f0b1 100644 --- a/test/extensions/common/wasm/wasm_test.cc +++ b/test/extensions/common/wasm/wasm_test.cc @@ -1349,7 +1349,7 @@ TEST_P(WasmCommonContextTest, OnDnsResolve) { .WillRepeatedly( testing::DoAll(testing::SaveArg<2>(&dns_callback), Return(&active_dns_query))); - setup(code, "resolve dns"); + setup(code, "context"); setupContext(); EXPECT_CALL(rootContext(), log_(spdlog::level::warn, Eq("TestRootContext::onResolveDns 1"))); EXPECT_CALL(rootContext(), log_(spdlog::level::warn, Eq("TestRootContext::onResolveDns 2"))); @@ -1390,7 +1390,7 @@ TEST_P(WasmCommonContextTest, EmptyContext) { } EXPECT_FALSE(code.empty()); - setup(code, "resolve dns", "empty"); + setup(code, "context", "empty"); setupContext(); root_context_->onResolveDns(0, Envoy::Network::DnsResolver::ResolutionStatus::Success, {}); @@ -1446,7 +1446,7 @@ TEST_P(WasmCommonHttpContextTest, DuplicateLocalReply) { } EXPECT_FALSE(code.empty()); - setup(code, "send local reply twice"); + setup(code, "", "send local reply twice"); setupFilter(); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .WillOnce([this](Http::ResponseHeaderMap&, bool) { filter().onResponseHeaders(0, false); }); @@ -1470,7 +1470,7 @@ TEST_P(WasmCommonHttpContextTest, LocalReplyWhenPanic) { } EXPECT_FALSE(code.empty()); - setup(code, "panic after sending local reply"); + setup(code, "", "panic after sending local reply"); setupFilter(); // In the case of VM failure, failStream is called, so we need to make sure that we don't send the // local reply twice. From c94ee6c23b75591ebd54ef29b48aa82792be41ba Mon Sep 17 00:00:00 2001 From: johnlanni Date: Fri, 7 Oct 2022 12:58:34 +0800 Subject: [PATCH 09/11] optimize UT Signed-off-by: johnlanni --- .../common/wasm/test_data/test_context_cpp.cc | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/test/extensions/common/wasm/test_data/test_context_cpp.cc b/test/extensions/common/wasm/test_data/test_context_cpp.cc index 78e28ce2b334..225fda61b8c5 100644 --- a/test/extensions/common/wasm/test_data/test_context_cpp.cc +++ b/test/extensions/common/wasm/test_data/test_context_cpp.cc @@ -79,18 +79,13 @@ void TestRootContext::onQueueReady(uint32_t) { } } -class TestCommonContext : public RootContext { -public: - explicit TestCommonContext(uint32_t id, std::string_view root_id) : RootContext(id, root_id) {} -}; - class DupReplyContext : public Context { public: explicit DupReplyContext(uint32_t id, RootContext* root) : Context(id, root) {} FilterDataStatus onRequestBody(size_t body_buffer_length, bool end_of_stream) override; private: - TestCommonContext* root() { return static_cast(Context::root()); } + EnvoyRootContext* root() { return static_cast(Context::root()); } }; FilterDataStatus DupReplyContext::onRequestBody(size_t, bool) { @@ -105,7 +100,7 @@ class PanicReplyContext : public Context { FilterDataStatus onRequestBody(size_t body_buffer_length, bool end_of_stream) override; private: - TestCommonContext* root() { return static_cast(Context::root()); } + EnvoyRootContext* root() { return static_cast(Context::root()); } }; FilterDataStatus PanicReplyContext::onRequestBody(size_t, bool) { @@ -116,10 +111,10 @@ FilterDataStatus PanicReplyContext::onRequestBody(size_t, bool) { } static RegisterContextFactory register_DupReplyContext(CONTEXT_FACTORY(DupReplyContext), - ROOT_FACTORY(TestCommonContext), + ROOT_FACTORY(EnvoyRootContext), "send local reply twice"); static RegisterContextFactory register_PanicReplyContext(CONTEXT_FACTORY(PanicReplyContext), - ROOT_FACTORY(TestCommonContext), + ROOT_FACTORY(EnvoyRootContext), "panic after sending local reply"); END_WASM_PLUGIN From c1abd985245f228652efffd41cd417d6072fc195 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Sun, 9 Oct 2022 19:45:00 +0800 Subject: [PATCH 10/11] optimize UT Signed-off-by: johnlanni --- test/extensions/common/wasm/wasm_test.cc | 69 +++++------------------- 1 file changed, 14 insertions(+), 55 deletions(-) diff --git a/test/extensions/common/wasm/wasm_test.cc b/test/extensions/common/wasm/wasm_test.cc index 5675edb2f0b1..721fdeec40da 100644 --- a/test/extensions/common/wasm/wasm_test.cc +++ b/test/extensions/common/wasm/wasm_test.cc @@ -1298,7 +1298,7 @@ TEST_P(WasmCommonTest, ThreadLocalCopyRetainsEnforcement) { thread_local_wasm->start(plugin); } -class WasmCommonContextTest : public Common::Wasm::WasmTestBase< +class WasmCommonContextTest : public Common::Wasm::WasmHttpFilterTestBase< testing::TestWithParam>> { public: WasmCommonContextTest() = default; @@ -1312,16 +1312,10 @@ class WasmCommonContextTest : public Common::Wasm::WasmTestBase< }); } - void setupContext() { - context_ = - std::make_unique(wasm_->wasm().get(), root_context_->id(), plugin_handle_); - context_->onCreate(); - } + void setupContext() { setupFilterBase(); } TestContext& rootContext() { return *static_cast(root_context_); } - TestContext& context() { return *context_; } - - std::unique_ptr context_; + TestContext& context() { return *static_cast(context_.get()); } }; INSTANTIATE_TEST_SUITE_P(Runtimes, WasmCommonContextTest, @@ -1399,43 +1393,8 @@ TEST_P(WasmCommonContextTest, EmptyContext) { root_context_->validateConfiguration("", plugin_); } -class TestFilter : public Envoy::Extensions::Common::Wasm::Context { -public: - TestFilter(Wasm* wasm, uint32_t root_context_id, PluginHandleSharedPtr plugin_handle) - : Envoy::Extensions::Common::Wasm::Context(wasm, root_context_id, plugin_handle) {} - using ::Envoy::Extensions::Common::Wasm::Context::log; - proxy_wasm::WasmResult log(uint32_t, std::string_view message) override { - std::cerr << message << "\n"; - return proxy_wasm::WasmResult::Ok; - } -}; - -class WasmCommonHttpContextTest - : public Common::Wasm::WasmHttpFilterTestBase< - testing::TestWithParam>> { -public: - WasmCommonHttpContextTest() = default; - - void setup(const std::string& code, std::string vm_configuration, std::string root_id = "") { - setRootId(root_id); - setVmConfiguration(vm_configuration); - setupBase(std::get<0>(GetParam()), code, - [](Wasm* wasm, const std::shared_ptr& plugin) -> ContextBase* { - return new NiceMock(wasm, plugin); - }); - } - void setupFilter() { setupFilterBase(); } - - TestContext& rootContext() { return *static_cast(root_context_); } - - TestFilter& filter() { return *static_cast(context_.get()); } -}; - -INSTANTIATE_TEST_SUITE_P(Runtimes, WasmCommonHttpContextTest, - Envoy::Extensions::Common::Wasm::runtime_and_cpp_values); - // test that we don't send the local reply twice, even though it's specified in the wasm code -TEST_P(WasmCommonHttpContextTest, DuplicateLocalReply) { +TEST_P(WasmCommonContextTest, DuplicateLocalReply) { std::string code; if (std::get<0>(GetParam()) != "null") { code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(absl::StrCat( @@ -1446,20 +1405,20 @@ TEST_P(WasmCommonHttpContextTest, DuplicateLocalReply) { } EXPECT_FALSE(code.empty()); - setup(code, "", "send local reply twice"); - setupFilter(); + setup(code, "context", "send local reply twice"); + setupContext(); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) - .WillOnce([this](Http::ResponseHeaderMap&, bool) { filter().onResponseHeaders(0, false); }); + .WillOnce([this](Http::ResponseHeaderMap&, bool) { context().onResponseHeaders(0, false); }); EXPECT_CALL(decoder_callbacks_, sendLocalReply(Envoy::Http::Code::OK, testing::Eq("body"), _, _, testing::Eq("ok"))); // Create in-VM context. - filter().onCreate(); - EXPECT_EQ(proxy_wasm::FilterDataStatus::StopIterationNoBuffer, filter().onRequestBody(0, false)); + context().onCreate(); + EXPECT_EQ(proxy_wasm::FilterDataStatus::StopIterationNoBuffer, context().onRequestBody(0, false)); } // test that we don't send the local reply twice when the wasm code panics -TEST_P(WasmCommonHttpContextTest, LocalReplyWhenPanic) { +TEST_P(WasmCommonContextTest, LocalReplyWhenPanic) { std::string code; if (std::get<0>(GetParam()) != "null") { code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(absl::StrCat( @@ -1470,8 +1429,8 @@ TEST_P(WasmCommonHttpContextTest, LocalReplyWhenPanic) { } EXPECT_FALSE(code.empty()); - setup(code, "", "panic after sending local reply"); - setupFilter(); + setup(code, "context", "panic after sending local reply"); + setupContext(); // In the case of VM failure, failStream is called, so we need to make sure that we don't send the // local reply twice. EXPECT_CALL(decoder_callbacks_, @@ -1480,8 +1439,8 @@ TEST_P(WasmCommonHttpContextTest, LocalReplyWhenPanic) { testing::Eq("wasm_fail_stream"))); // Create in-VM context. - filter().onCreate(); - EXPECT_EQ(proxy_wasm::FilterDataStatus::StopIterationNoBuffer, filter().onRequestBody(0, false)); + context().onCreate(); + EXPECT_EQ(proxy_wasm::FilterDataStatus::StopIterationNoBuffer, context().onRequestBody(0, false)); } } // namespace Wasm From c962f83c014a8f92ddfa99407789d508a6a43777 Mon Sep 17 00:00:00 2001 From: johnlanni Date: Mon, 10 Oct 2022 16:03:29 +0800 Subject: [PATCH 11/11] add comment Signed-off-by: johnlanni --- source/extensions/common/wasm/context.cc | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/source/extensions/common/wasm/context.cc b/source/extensions/common/wasm/context.cc index a257f05e69a3..6ab577932e47 100644 --- a/source/extensions/common/wasm/context.cc +++ b/source/extensions/common/wasm/context.cc @@ -1638,6 +1638,9 @@ void Context::failStream(WasmStreamType stream_type) { WasmResult Context::sendLocalResponse(uint32_t response_code, std::string_view body_text, Pairs additional_headers, uint32_t grpc_status, std::string_view details) { + // This flag is used to avoid calling sendLocalReply() twice, even if wasm code has this + // logic. We can't reuse "local_reply_sent_" here because it can't avoid calling nested + // sendLocalReply() during encodeHeaders(). if (local_reply_hold_) { return WasmResult::BadArgument; } @@ -1665,6 +1668,8 @@ WasmResult Context::sendLocalResponse(uint32_t response_code, std::string_view b modify_headers = std::move(modify_headers), grpc_status, details = StringUtil::replaceAllEmptySpace( absl::string_view(details.data(), details.size()))] { + // When the wasm vm fails, failStream() is called if the plugin is fail-closed, we need + // this flag to avoid calling sendLocalReply() twice. if (local_reply_sent_) { return; }