From de74d9f68aad981bbdf14f61dcf1679dbb93fb96 Mon Sep 17 00:00:00 2001 From: wangbaiping/wbpcode Date: Thu, 17 Oct 2024 14:58:59 +0800 Subject: [PATCH 1/9] dirty commit Signed-off-by: wangbaiping/wbpcode --- source/common/runtime/runtime_features.cc | 1 + source/extensions/common/wasm/context.cc | 16 ++++------------ source/extensions/common/wasm/context.h | 11 +++++++++++ source/extensions/common/wasm/foreign.cc | 8 ++++++++ 4 files changed, 24 insertions(+), 12 deletions(-) diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 772f6dd62b0b2..541412e7c2275 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -108,6 +108,7 @@ RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protocol_lis RUNTIME_GUARD(envoy_reloadable_features_validate_connect); RUNTIME_GUARD(envoy_reloadable_features_validate_grpc_header_before_log_grpc_status); RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers); +RUNTIME_GUARD(envoy_reloadable_features_wasm_no_automatic_route_refresh); RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled); RUNTIME_GUARD(envoy_reloadable_features_xdstp_path_avoid_colon_encoding); RUNTIME_GUARD(envoy_restart_features_allow_slot_destroy_on_worker_threads); diff --git a/source/extensions/common/wasm/context.cc b/source/extensions/common/wasm/context.cc index 64c34a6f20bd0..50f85bcf92eb5 100644 --- a/source/extensions/common/wasm/context.cc +++ b/source/extensions/common/wasm/context.cc @@ -732,9 +732,7 @@ WasmResult Context::addHeaderMapValue(WasmHeaderMapType type, std::string_view k } const Http::LowerCaseString lower_key{std::string(key)}; map->addCopy(lower_key, std::string(value)); - if (type == WasmHeaderMapType::RequestHeaders) { - clearRouteCache(); - } + onHeadersModified(type); return WasmResult::Ok; } @@ -807,9 +805,7 @@ WasmResult Context::setHeaderMapPairs(WasmHeaderMapType type, const Pairs& pairs const Http::LowerCaseString lower_key{std::string(p.first)}; map->addCopy(lower_key, std::string(p.second)); } - if (type == WasmHeaderMapType::RequestHeaders) { - clearRouteCache(); - } + onHeadersModified(type); return WasmResult::Ok; } @@ -820,9 +816,7 @@ WasmResult Context::removeHeaderMapValue(WasmHeaderMapType type, std::string_vie } const Http::LowerCaseString lower_key{std::string(key)}; map->remove(lower_key); - if (type == WasmHeaderMapType::RequestHeaders) { - clearRouteCache(); - } + onHeadersModified(type); return WasmResult::Ok; } @@ -834,9 +828,7 @@ WasmResult Context::replaceHeaderMapValue(WasmHeaderMapType type, std::string_vi } const Http::LowerCaseString lower_key{std::string(key)}; map->setCopy(lower_key, toAbslStringView(value)); - if (type == WasmHeaderMapType::RequestHeaders) { - clearRouteCache(); - } + onHeadersModified(type); return WasmResult::Ok; } diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h index 77dbba0e37727..dfdc4316473c5 100644 --- a/source/extensions/common/wasm/context.h +++ b/source/extensions/common/wasm/context.h @@ -374,6 +374,14 @@ class Context : public proxy_wasm::ContextBase, Http::HeaderMap* getMap(WasmHeaderMapType type); const Http::HeaderMap* getConstMap(WasmHeaderMapType type); + void onHeadersModified(WasmHeaderMapType type) { + if (type == WasmHeaderMapType::RequestHeaders) { + if (!no_automatic_route_refresh_) { + clearRouteCache(); + } + } + } + const LocalInfo::LocalInfo* root_local_info_{nullptr}; // set only for root_context. PluginHandleSharedPtr plugin_handle_{nullptr}; @@ -450,6 +458,9 @@ class Context : public proxy_wasm::ContextBase, // Filter state prototype declaration. absl::flat_hash_map state_prototypes_; + + const bool no_automatic_route_refresh_{ + Runtime::runtimeFeatureEnabled("envoy.reloadable_features.wasm_no_automatic_route_refresh")}; }; using ContextSharedPtr = std::shared_ptr; diff --git a/source/extensions/common/wasm/foreign.cc b/source/extensions/common/wasm/foreign.cc index 4780c5f8a1100..43cf9369148d3 100644 --- a/source/extensions/common/wasm/foreign.cc +++ b/source/extensions/common/wasm/foreign.cc @@ -126,6 +126,14 @@ RegisterForeignFunction registerSetEnvoyFilterStateForeignFunction( return WasmResult::BadArgument; }); +RegisterForeignFunction registerClearRouteCacheForeignFunction( + "clear_route_cache", + [](WasmBase&, std::string_view, const std::function&) -> WasmResult { + auto context = static_cast(proxy_wasm::current_context_); + context->clearRouteCache(); + return WasmResult::Ok; + }); + #if defined(WASM_USE_CEL_PARSER) class ExpressionFactory : public Logger::Loggable { protected: From dad1f7b2fb7e8769266135609988d7b12416414c Mon Sep 17 00:00:00 2001 From: wangbaiping/wbpcode Date: Thu, 17 Oct 2024 18:44:20 +0800 Subject: [PATCH 2/9] complete test Signed-off-by: wangbaiping/wbpcode --- test/extensions/common/wasm/BUILD | 7 +++ test/extensions/common/wasm/context_test.cc | 39 ++++++++----- test/extensions/common/wasm/foreign_test.cc | 32 +++++++++++ test/test_common/wasm_base.h | 64 +++++++++++++++++++++ 4 files changed, 128 insertions(+), 14 deletions(-) diff --git a/test/extensions/common/wasm/BUILD b/test/extensions/common/wasm/BUILD index 66d0664ffb3c9..816c45019aeac 100644 --- a/test/extensions/common/wasm/BUILD +++ b/test/extensions/common/wasm/BUILD @@ -129,6 +129,7 @@ envoy_cc_test( "//test/mocks/http:http_mocks", "//test/mocks/network:network_mocks", "//test/mocks/stream_info:stream_info_mocks", + "//test/test_common:test_runtime_lib", ], ) @@ -141,19 +142,25 @@ envoy_cc_test( "-DWASM_USE_CEL_PARSER", ], }), + data = envoy_select_wasm_cpp_tests([ + "//test/extensions/common/wasm/test_data:test_context_cpp.wasm", + ]), rbe_pool = "2core", tags = ["skip_on_windows"], deps = [ + ":wasm_runtime", "//source/common/network:filter_state_dst_address_lib", "//source/common/tcp_proxy", "//source/extensions/clusters/original_dst:original_dst_cluster_lib", "//source/extensions/common/wasm:wasm_hdr", "//source/extensions/common/wasm:wasm_lib", + "//test/extensions/common/wasm/test_data:test_context_cpp_plugin", "//test/mocks/http:http_mocks", "//test/mocks/local_info:local_info_mocks", "//test/mocks/network:network_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:environment_lib", + "//test/test_common:wasm_lib", "@proxy_wasm_cpp_host//:base_lib", ], ) diff --git a/test/extensions/common/wasm/context_test.cc b/test/extensions/common/wasm/context_test.cc index 75fd472a776da..510fabc393004 100644 --- a/test/extensions/common/wasm/context_test.cc +++ b/test/extensions/common/wasm/context_test.cc @@ -5,6 +5,7 @@ #include "test/mocks/http/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/stream_info/mocks.h" +#include "test/test_common/test_runtime.h" #include "eval/public/cel_value.h" #include "gmock/gmock.h" @@ -248,6 +249,11 @@ TEST_F(ContextTest, FindValueTest) { } TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfiguration) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.wasm_no_automatic_route_refresh", "false"}}); + + TestContext test_ctx; Http::MockDownstreamStreamFilterCallbacks downstream_callbacks; EXPECT_CALL(downstream_callbacks, clearRouteCache()).Times(5).WillRepeatedly(testing::Return()); @@ -256,34 +262,39 @@ TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfiguration) { EXPECT_CALL(decoder_callbacks, downstreamCallbacks()) .WillRepeatedly(testing::Return( makeOptRef(dynamic_cast(downstream_callbacks)))); - ctx_.setDecoderFilterCallbacksPtr(&decoder_callbacks); + test_ctx.setDecoderFilterCallbacksPtr(&decoder_callbacks); Http::TestRequestHeaderMapImpl request_headers{{":path", "/123"}}; - ctx_.setRequestHeaders(&request_headers); + test_ctx.setRequestHeaders(&request_headers); - ctx_.clearRouteCache(); - ctx_.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value"); - ctx_.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}}); - ctx_.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2"); - ctx_.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key"); + test_ctx.clearRouteCache(); + test_ctx.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value"); + test_ctx.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}}); + test_ctx.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2"); + test_ctx.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key"); } TEST_F(ContextTest, ClearRouteCacheDoesNothingInUpstreamConfiguration) { + TestScopedRuntime scoped_runtime; + scoped_runtime.mergeValues( + {{"envoy.reloadable_features.wasm_no_automatic_route_refresh", "false"}}); + + TestContext test_ctx; Http::MockStreamDecoderFilterCallbacks decoder_callbacks; EXPECT_CALL(decoder_callbacks, downstreamCallbacks()) .WillRepeatedly(testing::Return(absl::nullopt)); - ctx_.setDecoderFilterCallbacksPtr(&decoder_callbacks); + test_ctx.setDecoderFilterCallbacksPtr(&decoder_callbacks); Http::TestRequestHeaderMapImpl request_headers{{":path", "/123"}}; - ctx_.setRequestHeaders(&request_headers); + test_ctx.setRequestHeaders(&request_headers); // Calling methods should be safe. - ctx_.clearRouteCache(); - ctx_.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value"); - ctx_.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}}); - ctx_.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2"); - ctx_.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key"); + test_ctx.clearRouteCache(); + test_ctx.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value"); + test_ctx.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}}); + test_ctx.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2"); + test_ctx.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key"); } } // namespace Wasm diff --git a/test/extensions/common/wasm/foreign_test.cc b/test/extensions/common/wasm/foreign_test.cc index a29c20eed483c..2edea699acd26 100644 --- a/test/extensions/common/wasm/foreign_test.cc +++ b/test/extensions/common/wasm/foreign_test.cc @@ -6,9 +6,13 @@ #include "source/extensions/common/wasm/ext/set_envoy_filter_state.pb.h" #include "source/extensions/common/wasm/wasm.h" +#include "test/extensions/common/wasm/wasm_runtime.h" +#include "test/mocks/http/mocks.h" #include "test/mocks/local_info/mocks.h" +#include "test/mocks/server/factory_context.h" #include "test/mocks/upstream/cluster_manager.h" #include "test/test_common/utility.h" +#include "test/test_common/wasm_base.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -113,6 +117,34 @@ TEST_F(ForeignTest, ForeignFunctionSetEnvoyFilterTest) { Upstream::OriginalDstClusterFilterStateKey)); } +class StreamForeignTest : public WasmPluginConfigTestBase< + testing::TestWithParam>> { +public: + StreamForeignTest() = default; +}; + +INSTANTIATE_TEST_SUITE_P(PluginConfigRuntimes, StreamForeignTest, + Envoy::Extensions::Common::Wasm::runtime_and_cpp_values, + Envoy::Extensions::Common::Wasm::wasmTestParamsToString); + +TEST_P(StreamForeignTest, ForeignFunctionClearRouteCache) { + auto [runtime, language] = GetParam(); + + auto plugin_config = getWasmPluginConfigForTest( + runtime, "test/extensions/common/wasm/test_data/test_context_cpp.wasm", + "CommonWasmTestContextCpp", "send local reply twice"); + + setUp(plugin_config); + + createStreamContext(); + + proxy_wasm::current_context_ = context_.get(); + auto function = proxy_wasm::getForeignFunction("clear_route_cache"); + + EXPECT_CALL(decoder_callbacks_.downstream_callbacks_, clearRouteCache()); + function(*plugin_config_->wasm(), "", [](size_t size) { return malloc(size); }); +} + } // namespace Wasm } // namespace Common } // namespace Extensions diff --git a/test/test_common/wasm_base.h b/test/test_common/wasm_base.h index 3e2b34393c06a..4d211dd9c5321 100644 --- a/test/test_common/wasm_base.h +++ b/test/test_common/wasm_base.h @@ -170,6 +170,70 @@ class WasmNetworkFilterTestBase : public WasmTestBase { NiceMock write_filter_callbacks_; }; +inline envoy::extensions::wasm::v3::PluginConfig +getWasmPluginConfigForTest(absl::string_view runtime, absl::string_view wasm_file_path, + absl::string_view wasm_module_name, absl::string_view plugin_root_id, + bool singleton = false, absl::string_view plugin_configuration = {}) { + + const std::string plugin_config_yaml = fmt::format( + R"EOF( + name: 'test_wasm_singleton_{}' + root_id: '{}' + vm_config: + runtime: 'envoy.wasm.runtime.{}' + configuration: + "@type": "type.googleapis.com/google.protobuf.StringValue" + value: '{}' + )EOF", + singleton, plugin_root_id, runtime, plugin_configuration); + + envoy::extensions::wasm::v3::PluginConfig plugin_config; + TestUtility::loadFromYaml(plugin_config_yaml, plugin_config); + + if (runtime == "null") { + plugin_config.mutable_vm_config()->mutable_code()->mutable_local()->set_inline_bytes( + std::string(wasm_module_name)); + } else { + const std::string code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute( + absl::StrCat("{{ test_rundir }}/" + std::string(wasm_file_path)))); + plugin_config.mutable_vm_config()->mutable_code()->mutable_local()->set_inline_bytes(code); + } + + return plugin_config; +} + +template class WasmPluginConfigTestBase : public Base { +public: + WasmPluginConfigTestBase() = default; + + // NOLINTNEXTLINE(readability-identifier-naming) + void SetUp() override { clearCodeCacheForTesting(); } + + void setUp(const envoy::extensions::wasm::v3::PluginConfig plugin_config, + bool singleton = false) { + plugin_config_ = std::make_shared( + plugin_config, server_, server_.scope(), server_.initManager(), + envoy::config::core::v3::TrafficDirection::UNSPECIFIED, /*metadata=*/nullptr, singleton); + } + + void createStreamContext() { + context_ = plugin_config_->createContext(); + if (context_ != nullptr) { + context_->setDecoderFilterCallbacks(decoder_callbacks_); + context_->setEncoderFilterCallbacks(encoder_callbacks_); + context_->onCreate(); + } + } + + NiceMock server_; + PluginConfigSharedPtr plugin_config_; + + NiceMock decoder_callbacks_; + NiceMock encoder_callbacks_; + + std::shared_ptr context_; +}; + } // namespace Wasm } // namespace Common } // namespace Extensions From 8080459fbca639a5a5c56c9594ad53925d28de71 Mon Sep 17 00:00:00 2001 From: wangbaiping/wbpcode Date: Thu, 17 Oct 2024 19:53:07 +0800 Subject: [PATCH 3/9] add log and change log Signed-off-by: wangbaiping/wbpcode --- changelogs/current.yaml | 9 +++++++++ source/extensions/common/wasm/context.h | 5 +++++ 2 files changed, 14 insertions(+) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index c6cf3eac87957..a8cc6bf64acdd 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -2,6 +2,12 @@ date: Pending behavior_changes: # *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* +- area: wasm + change: | + The route cache will not be cleared by default if the wasm extension modified the request headers. + But it could be done explicitly by call the ``clear_route_cache`` foreign function. + This behavior change could be disabled temporarily by setting the runtime flag + ``envoy_reloadable_features_wasm_no_automatic_route_refresh`` to false. minor_behavior_changes: # *Changes that may cause incompatibilities for some users, but should not for most* @@ -32,5 +38,8 @@ new_features: - area: tls change: | Added support for P-384 and P-521 curves for TLS server certificates. +- area: wasm + change: | + added ``clear_route_cache`` foreign function to clear the route cache. deprecated: diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h index dfdc4316473c5..b756de454cf15 100644 --- a/source/extensions/common/wasm/context.h +++ b/source/extensions/common/wasm/context.h @@ -378,6 +378,11 @@ class Context : public proxy_wasm::ContextBase, if (type == WasmHeaderMapType::RequestHeaders) { if (!no_automatic_route_refresh_) { clearRouteCache(); + } else { + ENVOY_LOG_FIRST_N(critical, 20, + "Route will no be refreshed automatically when request headers are " + "modified by the wasm plugin. Refresh the route manually by calling the " + "'clear_route_cache' foreign function. "); } } } From 5c80bc72d316b3132066e250a0fc791d845c4413 Mon Sep 17 00:00:00 2001 From: wangbaiping/wbpcode Date: Thu, 17 Oct 2024 21:10:49 +0800 Subject: [PATCH 4/9] fix test Signed-off-by: wangbaiping/wbpcode --- test/extensions/filters/http/wasm/wasm_filter_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/extensions/filters/http/wasm/wasm_filter_test.cc b/test/extensions/filters/http/wasm/wasm_filter_test.cc index cfaee6b90df3c..ce2bd85b9a04a 100644 --- a/test/extensions/filters/http/wasm/wasm_filter_test.cc +++ b/test/extensions/filters/http/wasm/wasm_filter_test.cc @@ -139,7 +139,6 @@ TEST_P(WasmHttpFilterTest, HeadersOnlyRequestHeadersOnlyWithEnvVars) { // Verify that route cache is cleared when modifying HTTP request headers. NiceMock decoder_callbacks; filter().setDecoderFilterCallbacks(decoder_callbacks); - EXPECT_CALL(decoder_callbacks.downstream_callbacks_, clearRouteCache()).Times(2); Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}, {"server", "envoy"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter().decodeHeaders(request_headers, true)); From 27830abb7ceed9ff37e0879afc542264ae3c5177 Mon Sep 17 00:00:00 2001 From: wangbaiping/wbpcode Date: Fri, 18 Oct 2024 14:43:54 +0800 Subject: [PATCH 5/9] address comments and fix test Signed-off-by: wangbaiping/wbpcode --- source/extensions/common/wasm/context.h | 21 +++++++++++-------- .../filters/http/wasm/wasm_filter_test.cc | 1 - 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h index b756de454cf15..6dcf4a90f1ee1 100644 --- a/source/extensions/common/wasm/context.h +++ b/source/extensions/common/wasm/context.h @@ -375,15 +375,18 @@ class Context : public proxy_wasm::ContextBase, const Http::HeaderMap* getConstMap(WasmHeaderMapType type); void onHeadersModified(WasmHeaderMapType type) { - if (type == WasmHeaderMapType::RequestHeaders) { - if (!no_automatic_route_refresh_) { - clearRouteCache(); - } else { - ENVOY_LOG_FIRST_N(critical, 20, - "Route will no be refreshed automatically when request headers are " - "modified by the wasm plugin. Refresh the route manually by calling the " - "'clear_route_cache' foreign function. "); - } + if (type != WasmHeaderMapType::RequestHeaders) { + return; + } + + // Only do this for request headers. + if (!no_automatic_route_refresh_) { + clearRouteCache(); + } else { + ENVOY_LOG_FIRST_N(critical, 5, + "Route will no be refreshed automatically when request headers are " + "modified by the wasm plugin. Refresh the route manually by calling the " + "'clear_route_cache' foreign function. "); } } diff --git a/test/extensions/filters/http/wasm/wasm_filter_test.cc b/test/extensions/filters/http/wasm/wasm_filter_test.cc index ce2bd85b9a04a..eed8f43382bac 100644 --- a/test/extensions/filters/http/wasm/wasm_filter_test.cc +++ b/test/extensions/filters/http/wasm/wasm_filter_test.cc @@ -167,7 +167,6 @@ TEST_P(WasmHttpFilterTest, AllHeadersAndTrailers) { // Verify that route cache is cleared when modifying HTTP request headers. NiceMock decoder_callbacks; filter().setDecoderFilterCallbacks(decoder_callbacks); - EXPECT_CALL(decoder_callbacks.downstream_callbacks_, clearRouteCache()).Times(2); Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}, {"server", "envoy"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter().decodeHeaders(request_headers, false)); From 13e26ea0ac43b70e3afc536031ced2ee22d20761 Mon Sep 17 00:00:00 2001 From: wangbaiping/wbpcode Date: Sat, 19 Oct 2024 11:04:58 +0800 Subject: [PATCH 6/9] address comments Signed-off-by: wangbaiping/wbpcode --- source/extensions/common/wasm/context.h | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h index 6dcf4a90f1ee1..45bc1c5bcc24e 100644 --- a/source/extensions/common/wasm/context.h +++ b/source/extensions/common/wasm/context.h @@ -375,19 +375,11 @@ class Context : public proxy_wasm::ContextBase, const Http::HeaderMap* getConstMap(WasmHeaderMapType type); void onHeadersModified(WasmHeaderMapType type) { - if (type != WasmHeaderMapType::RequestHeaders) { + if (type != WasmHeaderMapType::RequestHeaders || no_auto_route_refresh_) { return; } - - // Only do this for request headers. - if (!no_automatic_route_refresh_) { - clearRouteCache(); - } else { - ENVOY_LOG_FIRST_N(critical, 5, - "Route will no be refreshed automatically when request headers are " - "modified by the wasm plugin. Refresh the route manually by calling the " - "'clear_route_cache' foreign function. "); - } + // Only do this for request headers when auto-refresh is not disabled. + clearRouteCache(); } const LocalInfo::LocalInfo* root_local_info_{nullptr}; // set only for root_context. @@ -467,7 +459,7 @@ class Context : public proxy_wasm::ContextBase, absl::flat_hash_map state_prototypes_; - const bool no_automatic_route_refresh_{ + const bool no_auto_route_refresh_{ Runtime::runtimeFeatureEnabled("envoy.reloadable_features.wasm_no_automatic_route_refresh")}; }; using ContextSharedPtr = std::shared_ptr; From 1f78fad0cfce100c72684ecc70a7c641fb3ee10e Mon Sep 17 00:00:00 2001 From: wangbaiping/wbpcode Date: Tue, 22 Oct 2024 11:48:28 +0800 Subject: [PATCH 7/9] add more tests and only change default behavior for proxy-wasm > 0.2.1 Signed-off-by: wangbaiping/wbpcode --- source/extensions/common/wasm/context.cc | 9 +++- source/extensions/common/wasm/context.h | 7 ++- test/extensions/common/wasm/context_test.cc | 52 ++++++++++++++++++- .../filters/http/wasm/wasm_filter_test.cc | 2 + 4 files changed, 66 insertions(+), 4 deletions(-) diff --git a/source/extensions/common/wasm/context.cc b/source/extensions/common/wasm/context.cc index 50f85bcf92eb5..05302ded548b3 100644 --- a/source/extensions/common/wasm/context.cc +++ b/source/extensions/common/wasm/context.cc @@ -160,8 +160,15 @@ WasmResult Buffer::copyFrom(size_t start, size_t length, std::string_view data) } Context::Context() = default; -Context::Context(Wasm* wasm) : ContextBase(wasm) {} +Context::Context(Wasm* wasm) : ContextBase(wasm) { + if (wasm != nullptr) { + abi_version_ = wasm->abi_version_; + } +} Context::Context(Wasm* wasm, const PluginSharedPtr& plugin) : ContextBase(wasm, plugin) { + if (wasm != nullptr) { + abi_version_ = wasm->abi_version_; + } root_local_info_ = &std::static_pointer_cast(plugin)->localInfo(); } Context::Context(Wasm* wasm, uint32_t root_context_id, PluginHandleSharedPtr plugin_handle) diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h index 45bc1c5bcc24e..ff3a95f605b7a 100644 --- a/source/extensions/common/wasm/context.h +++ b/source/extensions/common/wasm/context.h @@ -375,10 +375,12 @@ class Context : public proxy_wasm::ContextBase, const Http::HeaderMap* getConstMap(WasmHeaderMapType type); void onHeadersModified(WasmHeaderMapType type) { - if (type != WasmHeaderMapType::RequestHeaders || no_auto_route_refresh_) { + if (type != WasmHeaderMapType::RequestHeaders || + // No automatic route refresh if the ABI version is larger than 0.2.1 and the + // auto-refresh is disabled. + (no_auto_route_refresh_ && abi_version_ > proxy_wasm::AbiVersion::ProxyWasm_0_2_1)) { return; } - // Only do this for request headers when auto-refresh is not disabled. clearRouteCache(); } @@ -461,6 +463,7 @@ class Context : public proxy_wasm::ContextBase, const bool no_auto_route_refresh_{ Runtime::runtimeFeatureEnabled("envoy.reloadable_features.wasm_no_automatic_route_refresh")}; + proxy_wasm::AbiVersion abi_version_{proxy_wasm::AbiVersion::Unknown}; }; using ContextSharedPtr = std::shared_ptr; diff --git a/test/extensions/common/wasm/context_test.cc b/test/extensions/common/wasm/context_test.cc index 510fabc393004..6586c9fe10ded 100644 --- a/test/extensions/common/wasm/context_test.cc +++ b/test/extensions/common/wasm/context_test.cc @@ -22,6 +22,12 @@ using google::api::expr::runtime::CelValue; class TestContext : public Context { public: + TestContext(absl::optional mock_abi_version = {}) { + if (mock_abi_version.has_value()) { + abi_version_ = mock_abi_version.value(); + } + } + void setEncoderFilterCallbacksPtr(Envoy::Http::StreamEncoderFilterCallbacks* cb) { encoder_callbacks_ = cb; } @@ -248,7 +254,7 @@ TEST_F(ContextTest, FindValueTest) { EXPECT_FALSE(ctx_.FindValue("plugin_name", &arena).has_value()); } -TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfiguration) { +TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfigurationWhenFlagIsDisabled) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues( {{"envoy.reloadable_features.wasm_no_automatic_route_refresh", "false"}}); @@ -274,6 +280,50 @@ TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfiguration) { test_ctx.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key"); } +TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfigurationForLegacyWasmPlugin) { + TestContext test_ctx(proxy_wasm::AbiVersion::ProxyWasm_0_2_1); + + Http::MockDownstreamStreamFilterCallbacks downstream_callbacks; + EXPECT_CALL(downstream_callbacks, clearRouteCache()).Times(5).WillRepeatedly(testing::Return()); + + Http::MockStreamDecoderFilterCallbacks decoder_callbacks; + EXPECT_CALL(decoder_callbacks, downstreamCallbacks()) + .WillRepeatedly(testing::Return( + makeOptRef(dynamic_cast(downstream_callbacks)))); + test_ctx.setDecoderFilterCallbacksPtr(&decoder_callbacks); + + Http::TestRequestHeaderMapImpl request_headers{{":path", "/123"}}; + test_ctx.setRequestHeaders(&request_headers); + + test_ctx.clearRouteCache(); + test_ctx.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value"); + test_ctx.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}}); + test_ctx.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2"); + test_ctx.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key"); +} + +TEST_F(ContextTest, NoAutoClearRouteCacheCalledInDownstreamConfiguration) { + TestContext test_ctx; + + Http::MockDownstreamStreamFilterCallbacks downstream_callbacks; + EXPECT_CALL(downstream_callbacks, clearRouteCache()).WillOnce(testing::Return()); + + Http::MockStreamDecoderFilterCallbacks decoder_callbacks; + EXPECT_CALL(decoder_callbacks, downstreamCallbacks()) + .WillRepeatedly(testing::Return( + makeOptRef(dynamic_cast(downstream_callbacks)))); + test_ctx.setDecoderFilterCallbacksPtr(&decoder_callbacks); + + Http::TestRequestHeaderMapImpl request_headers{{":path", "/123"}}; + test_ctx.setRequestHeaders(&request_headers); + + test_ctx.clearRouteCache(); + test_ctx.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value"); + test_ctx.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}}); + test_ctx.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2"); + test_ctx.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key"); +} + TEST_F(ContextTest, ClearRouteCacheDoesNothingInUpstreamConfiguration) { TestScopedRuntime scoped_runtime; scoped_runtime.mergeValues( diff --git a/test/extensions/filters/http/wasm/wasm_filter_test.cc b/test/extensions/filters/http/wasm/wasm_filter_test.cc index eed8f43382bac..cfaee6b90df3c 100644 --- a/test/extensions/filters/http/wasm/wasm_filter_test.cc +++ b/test/extensions/filters/http/wasm/wasm_filter_test.cc @@ -139,6 +139,7 @@ TEST_P(WasmHttpFilterTest, HeadersOnlyRequestHeadersOnlyWithEnvVars) { // Verify that route cache is cleared when modifying HTTP request headers. NiceMock decoder_callbacks; filter().setDecoderFilterCallbacks(decoder_callbacks); + EXPECT_CALL(decoder_callbacks.downstream_callbacks_, clearRouteCache()).Times(2); Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}, {"server", "envoy"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter().decodeHeaders(request_headers, true)); @@ -167,6 +168,7 @@ TEST_P(WasmHttpFilterTest, AllHeadersAndTrailers) { // Verify that route cache is cleared when modifying HTTP request headers. NiceMock decoder_callbacks; filter().setDecoderFilterCallbacks(decoder_callbacks); + EXPECT_CALL(decoder_callbacks.downstream_callbacks_, clearRouteCache()).Times(2); Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}, {"server", "envoy"}}; EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter().decodeHeaders(request_headers, false)); From 8f5eafa84aa52fd7d6c5dbcabba4fb5cf616e9b9 Mon Sep 17 00:00:00 2001 From: "wangbaiping(wbpcode)" Date: Wed, 23 Oct 2024 01:58:46 +0000 Subject: [PATCH 8/9] address comments Signed-off-by: wangbaiping(wbpcode) --- changelogs/current.yaml | 6 ++--- source/common/runtime/runtime_features.cc | 1 - source/extensions/common/wasm/context.cc | 6 ++++- source/extensions/common/wasm/context.h | 6 +---- test/extensions/common/wasm/context_test.cc | 30 --------------------- 5 files changed, 8 insertions(+), 41 deletions(-) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index cd344ea14b4b8..016705ed2762e 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -4,10 +4,8 @@ behavior_changes: # *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required* - area: wasm change: | - The route cache will not be cleared by default if the wasm extension modified the request headers. - But it could be done explicitly by call the ``clear_route_cache`` foreign function. - This behavior change could be disabled temporarily by setting the runtime flag - ``envoy_reloadable_features_wasm_no_automatic_route_refresh`` to false. + The route cache will not be cleared by default if the wasm extension modified the request headers and + the ABI version of wasm extension is larger then 0.2.1. - area: wasm Remove previously deprecated xDS attributes from ``get_property``, use ``xds`` attributes instead. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 661125a8122a5..4036c8f0ca867 100644 --- a/source/common/runtime/runtime_features.cc +++ b/source/common/runtime/runtime_features.cc @@ -106,7 +106,6 @@ RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protocol_lis RUNTIME_GUARD(envoy_reloadable_features_validate_connect); RUNTIME_GUARD(envoy_reloadable_features_validate_grpc_header_before_log_grpc_status); RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers); -RUNTIME_GUARD(envoy_reloadable_features_wasm_no_automatic_route_refresh); RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled); RUNTIME_GUARD(envoy_reloadable_features_xdstp_path_avoid_colon_encoding); RUNTIME_GUARD(envoy_restart_features_allow_slot_destroy_on_worker_threads); diff --git a/source/extensions/common/wasm/context.cc b/source/extensions/common/wasm/context.cc index 7055f87e2fb2c..27389da0394dd 100644 --- a/source/extensions/common/wasm/context.cc +++ b/source/extensions/common/wasm/context.cc @@ -165,7 +165,11 @@ Context::Context(Wasm* wasm, const PluginSharedPtr& plugin) : ContextBase(wasm, root_local_info_ = &std::static_pointer_cast(plugin)->localInfo(); } Context::Context(Wasm* wasm, uint32_t root_context_id, PluginHandleSharedPtr plugin_handle) - : ContextBase(wasm, root_context_id, plugin_handle), plugin_handle_(plugin_handle) {} + : ContextBase(wasm, root_context_id, plugin_handle), plugin_handle_(plugin_handle) { + if (wasm != nullptr) { + abi_version_ = wasm->abi_version_; + } +} Wasm* Context::wasm() const { return static_cast(wasm_); } Plugin* Context::plugin() const { return static_cast(plugin_.get()); } diff --git a/source/extensions/common/wasm/context.h b/source/extensions/common/wasm/context.h index ff3a95f605b7a..fc9aaa25a61a1 100644 --- a/source/extensions/common/wasm/context.h +++ b/source/extensions/common/wasm/context.h @@ -376,9 +376,7 @@ class Context : public proxy_wasm::ContextBase, void onHeadersModified(WasmHeaderMapType type) { if (type != WasmHeaderMapType::RequestHeaders || - // No automatic route refresh if the ABI version is larger than 0.2.1 and the - // auto-refresh is disabled. - (no_auto_route_refresh_ && abi_version_ > proxy_wasm::AbiVersion::ProxyWasm_0_2_1)) { + abi_version_ > proxy_wasm::AbiVersion::ProxyWasm_0_2_1) { return; } clearRouteCache(); @@ -461,8 +459,6 @@ class Context : public proxy_wasm::ContextBase, absl::flat_hash_map state_prototypes_; - const bool no_auto_route_refresh_{ - Runtime::runtimeFeatureEnabled("envoy.reloadable_features.wasm_no_automatic_route_refresh")}; proxy_wasm::AbiVersion abi_version_{proxy_wasm::AbiVersion::Unknown}; }; using ContextSharedPtr = std::shared_ptr; diff --git a/test/extensions/common/wasm/context_test.cc b/test/extensions/common/wasm/context_test.cc index 6586c9fe10ded..53043843626b8 100644 --- a/test/extensions/common/wasm/context_test.cc +++ b/test/extensions/common/wasm/context_test.cc @@ -254,32 +254,6 @@ TEST_F(ContextTest, FindValueTest) { EXPECT_FALSE(ctx_.FindValue("plugin_name", &arena).has_value()); } -TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfigurationWhenFlagIsDisabled) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues( - {{"envoy.reloadable_features.wasm_no_automatic_route_refresh", "false"}}); - - TestContext test_ctx; - - Http::MockDownstreamStreamFilterCallbacks downstream_callbacks; - EXPECT_CALL(downstream_callbacks, clearRouteCache()).Times(5).WillRepeatedly(testing::Return()); - - Http::MockStreamDecoderFilterCallbacks decoder_callbacks; - EXPECT_CALL(decoder_callbacks, downstreamCallbacks()) - .WillRepeatedly(testing::Return( - makeOptRef(dynamic_cast(downstream_callbacks)))); - test_ctx.setDecoderFilterCallbacksPtr(&decoder_callbacks); - - Http::TestRequestHeaderMapImpl request_headers{{":path", "/123"}}; - test_ctx.setRequestHeaders(&request_headers); - - test_ctx.clearRouteCache(); - test_ctx.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value"); - test_ctx.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}}); - test_ctx.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2"); - test_ctx.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key"); -} - TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfigurationForLegacyWasmPlugin) { TestContext test_ctx(proxy_wasm::AbiVersion::ProxyWasm_0_2_1); @@ -325,10 +299,6 @@ TEST_F(ContextTest, NoAutoClearRouteCacheCalledInDownstreamConfiguration) { } TEST_F(ContextTest, ClearRouteCacheDoesNothingInUpstreamConfiguration) { - TestScopedRuntime scoped_runtime; - scoped_runtime.mergeValues( - {{"envoy.reloadable_features.wasm_no_automatic_route_refresh", "false"}}); - TestContext test_ctx; Http::MockStreamDecoderFilterCallbacks decoder_callbacks; From 87a3a5536fc10ebb6eeff8b01c03655d8f0015b2 Mon Sep 17 00:00:00 2001 From: "wangbaiping(wbpcode)" Date: Wed, 23 Oct 2024 02:47:43 +0000 Subject: [PATCH 9/9] fix format Signed-off-by: wangbaiping(wbpcode) --- changelogs/current.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/changelogs/current.yaml b/changelogs/current.yaml index fd1b7ccc2e66d..9106ec3cb4ee8 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -7,6 +7,7 @@ behavior_changes: The route cache will not be cleared by default if the wasm extension modified the request headers and the ABI version of wasm extension is larger then 0.2.1. - area: wasm + change: | Remove previously deprecated xDS attributes from ``get_property``, use ``xds`` attributes instead. minor_behavior_changes: