From e472ab74d9acf214d077a097fa270ddb4b3970c2 Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Sun, 13 Oct 2024 01:26:37 +0800 Subject: [PATCH 01/10] wasm: clean up the code Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- .../extensions/access_loggers/wasm/config.cc | 31 ++------ .../extensions/access_loggers/wasm/config.h | 1 - .../wasm/wasm_access_log_impl.h | 21 ++---- source/extensions/common/wasm/wasm.cc | 27 +++++++ source/extensions/common/wasm/wasm.h | 72 +++++++++++++++++-- source/extensions/filters/http/wasm/config.h | 2 +- .../filters/http/wasm/wasm_filter.cc | 44 +++--------- .../filters/http/wasm/wasm_filter.h | 38 +--------- .../extensions/filters/network/wasm/config.cc | 2 +- .../filters/network/wasm/wasm_filter.cc | 26 +------ .../filters/network/wasm/wasm_filter.h | 37 +--------- .../access_loggers/wasm/config_test.cc | 4 +- .../filters/http/wasm/config_test.cc | 15 ++-- .../filters/network/wasm/config_test.cc | 33 ++++----- .../filters/network/wasm/wasm_filter_test.cc | 3 +- 15 files changed, 147 insertions(+), 209 deletions(-) diff --git a/source/extensions/access_loggers/wasm/config.cc b/source/extensions/access_loggers/wasm/config.cc index f9559951b0c8..ba72d7335f44 100644 --- a/source/extensions/access_loggers/wasm/config.cc +++ b/source/extensions/access_loggers/wasm/config.cc @@ -14,8 +14,6 @@ namespace Extensions { namespace AccessLoggers { namespace Wasm { -using Common::Wasm::PluginHandleSharedPtrThreadLocal; - AccessLog::InstanceSharedPtr WasmAccessLogFactory::createAccessLogInstance(const Protobuf::Message& proto_config, AccessLog::FilterPtr&& filter, @@ -24,31 +22,10 @@ WasmAccessLogFactory::createAccessLogInstance(const Protobuf::Message& proto_con const envoy::extensions::access_loggers::wasm::v3::WasmAccessLog&>( proto_config, context.messageValidationVisitor()); - auto plugin = std::make_shared<Common::Wasm::Plugin>( - config.config(), envoy::config::core::v3::TrafficDirection::UNSPECIFIED, - context.serverFactoryContext().localInfo(), nullptr /* listener_metadata */); - - auto access_log = std::make_shared<WasmAccessLog>(plugin, nullptr, std::move(filter)); - - auto callback = [access_log, &context, plugin](Common::Wasm::WasmHandleSharedPtr base_wasm) { - // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call. - auto tls_slot = ThreadLocal::TypedSlot<PluginHandleSharedPtrThreadLocal>::makeUnique( - context.serverFactoryContext().threadLocal()); - tls_slot->set([base_wasm, plugin](Event::Dispatcher& dispatcher) { - return std::make_shared<PluginHandleSharedPtrThreadLocal>( - Common::Wasm::getOrCreateThreadLocalPlugin(base_wasm, plugin, dispatcher)); - }); - access_log->setTlsSlot(std::move(tls_slot)); - }; - - if (!Common::Wasm::createWasm( - plugin, context.scope().createScope(""), context.serverFactoryContext().clusterManager(), - context.initManager(), context.serverFactoryContext().mainThreadDispatcher(), - context.serverFactoryContext().api(), context.serverFactoryContext().lifecycleNotifier(), - remote_data_provider_, std::move(callback))) { - throw Common::Wasm::WasmException( - fmt::format("Unable to create Wasm access log {}", plugin->name_)); - } + auto plugin_config = std::make_unique<Common::Wasm::PluginConfig>( + config.config(), context.serverFactoryContext(), context.scope(), context.initManager(), + envoy::config::core::v3::TrafficDirection::UNSPECIFIED, nullptr /* metadata */); + auto access_log = std::make_shared<WasmAccessLog>(std::move(plugin_config), std::move(filter)); context.serverFactoryContext().api().customStatNamespaces().registerStatNamespace( Extensions::Common::Wasm::CustomStatNamespace); diff --git a/source/extensions/access_loggers/wasm/config.h b/source/extensions/access_loggers/wasm/config.h index ef8257a9b3be..43fbce159ca4 100644 --- a/source/extensions/access_loggers/wasm/config.h +++ b/source/extensions/access_loggers/wasm/config.h @@ -25,7 +25,6 @@ class WasmAccessLogFactory : public AccessLog::AccessLogInstanceFactory, private: absl::flat_hash_map<std::string, std::string> convertJsonFormatToMap(ProtobufWkt::Struct config); - RemoteAsyncDataProviderPtr remote_data_provider_; }; } // namespace Wasm diff --git a/source/extensions/access_loggers/wasm/wasm_access_log_impl.h b/source/extensions/access_loggers/wasm/wasm_access_log_impl.h index fbfb17fa70dc..2654ed2c5a2f 100644 --- a/source/extensions/access_loggers/wasm/wasm_access_log_impl.h +++ b/source/extensions/access_loggers/wasm/wasm_access_log_impl.h @@ -15,10 +15,9 @@ using Envoy::Extensions::Common::Wasm::PluginSharedPtr; class WasmAccessLog : public AccessLog::Instance { public: - WasmAccessLog(const PluginSharedPtr& plugin, - ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal>&& tls_slot, + WasmAccessLog(Extensions::Common::Wasm::PluginConfigPtr plugin_config, AccessLog::FilterPtr filter) - : plugin_(plugin), tls_slot_(std::move(tls_slot)), filter_(std::move(filter)) {} + : plugin_config_(std::move(plugin_config)), filter_(std::move(filter)) {} void log(const Formatter::HttpFormatterContext& log_context, const StreamInfo::StreamInfo& stream_info) override { @@ -28,23 +27,13 @@ class WasmAccessLog : public AccessLog::Instance { } } - if (tls_slot_ != nullptr) { - if (auto handle = tls_slot_->get()->handle(); handle != nullptr) { - if (handle->wasmHandle()) { - handle->wasmHandle()->wasm()->log(plugin_, log_context, stream_info); - } - } + if (Common::Wasm::Wasm* wasm = plugin_config_->wasmOfHandle(); wasm != nullptr) { + wasm->log(plugin_config_->plugin(), log_context, stream_info); } } - void setTlsSlot(ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal>&& tls_slot) { - ASSERT(tls_slot_ == nullptr); - tls_slot_ = std::move(tls_slot); - } - private: - PluginSharedPtr plugin_; - ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal> tls_slot_; + Common::Wasm::PluginConfigPtr plugin_config_; AccessLog::FilterPtr filter_; }; diff --git a/source/extensions/common/wasm/wasm.cc b/source/extensions/common/wasm/wasm.cc index bbb45a4ac311..80f7bc2404fe 100644 --- a/source/extensions/common/wasm/wasm.cc +++ b/source/extensions/common/wasm/wasm.cc @@ -485,6 +485,33 @@ getOrCreateThreadLocalPlugin(const WasmHandleSharedPtr& base_wasm, const PluginS getPluginHandleFactory())); } +PluginConfig::PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& config, + Server::Configuration::ServerFactoryContext& server, Stats::Scope& scope, + Init::Manager& init_manager, + envoy::config::core::v3::TrafficDirection direction, + const envoy::config::core::v3::Metadata* metadata) { + tls_slot_ = ThreadLocal::TypedSlot<Common::Wasm::PluginHandleSharedPtrThreadLocal>::makeUnique( + server.threadLocal()); + + plugin_ = std::make_shared<Common::Wasm::Plugin>(config, direction, server.localInfo(), metadata); + + auto callback = [this](const Common::Wasm::WasmHandleSharedPtr& base_wasm) { + // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call. + tls_slot_->set([base_wasm, plugin = this->plugin_](Event::Dispatcher& dispatcher) { + return std::make_shared<PluginHandleSharedPtrThreadLocal>( + getOrCreateThreadLocalPlugin(base_wasm, plugin, dispatcher)); + }); + }; + + if (!Common::Wasm::createWasm(plugin_, scope.createScope(""), server.clusterManager(), + init_manager, server.mainThreadDispatcher(), server.api(), + server.lifecycleNotifier(), remote_data_provider_, + std::move(callback))) { + throw Common::Wasm::WasmException( + fmt::format("Unable to create Wasm plugin {}", plugin_->name_)); + } +} + } // namespace Wasm } // namespace Common } // namespace Extensions diff --git a/source/extensions/common/wasm/wasm.h b/source/extensions/common/wasm/wasm.h index 9e2a10c1b7b4..06e3738c725e 100644 --- a/source/extensions/common/wasm/wasm.h +++ b/source/extensions/common/wasm/wasm.h @@ -6,6 +6,7 @@ #include <memory> #include "envoy/common/exception.h" +#include "envoy/extensions/wasm/v3/wasm.pb.h" #include "envoy/extensions/wasm/v3/wasm.pb.validate.h" #include "envoy/http/filter.h" #include "envoy/server/lifecycle_notifier.h" @@ -142,6 +143,7 @@ class PluginHandle : public PluginHandleBase { std::static_pointer_cast<PluginBase>(plugin)), plugin_(plugin), wasm_handle_(wasm_handle) {} + Wasm* wasmOfHandle() { return wasm_handle_ != nullptr ? wasm_handle_->wasm().get() : nullptr; } WasmHandleSharedPtr& wasmHandle() { return wasm_handle_; } uint32_t rootContextId() { return wasm_handle_->wasm()->getRootContext(plugin_, false)->id(); } @@ -154,11 +156,10 @@ using PluginHandleSharedPtr = std::shared_ptr<PluginHandle>; class PluginHandleSharedPtrThreadLocal : public ThreadLocal::ThreadLocalObject { public: - PluginHandleSharedPtrThreadLocal(PluginHandleSharedPtr handle) : handle_(handle){}; - PluginHandleSharedPtr& handle() { return handle_; } - -private: - PluginHandleSharedPtr handle_; + PluginHandleSharedPtrThreadLocal(PluginHandleSharedPtr handle) : handle(std::move(handle)) {} + Wasm* wasmOfHandle() { return handle != nullptr ? handle->wasmOfHandle() : nullptr; } + void updateHandle(PluginHandleSharedPtr new_handle) { handle = std::move(new_handle); } + PluginHandleSharedPtr handle; }; using CreateWasmCallback = std::function<void(WasmHandleSharedPtr)>; @@ -183,6 +184,67 @@ void clearCodeCacheForTesting(); void setTimeOffsetForCodeCacheForTesting(MonotonicTime::duration d); WasmEvent toWasmEvent(const std::shared_ptr<WasmHandleBase>& wasm); +class PluginConfig : Logger::Loggable<Logger::Id::wasm> { +public: + PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& config, + Server::Configuration::ServerFactoryContext& server, Stats::Scope& scope, + Init::Manager& init_manager, envoy::config::core::v3::TrafficDirection direction, + const envoy::config::core::v3::Metadata* metadata); + + std::shared_ptr<Context> createContext() { + if (!tls_slot_->currentThreadRegistered()) { + return nullptr; + } + auto plugin_holder = tls_slot_->get(); + if (!plugin_holder.has_value()) { + return nullptr; + } + + if (plugin_holder->handle == nullptr) { + return nullptr; + } + + Wasm* wasm = plugin_holder->wasmOfHandle(); + + if (!wasm || wasm->isFailed()) { + if (plugin_->fail_open_) { + return nullptr; // Fail open skips adding this filter to callbacks. + } else { + return std::make_shared<Context>( + nullptr, 0, + plugin_holder->handle); // Fail closed is handled by an empty Context. + } + } + return std::make_shared<Context>(wasm, plugin_holder->handle->rootContextId(), + plugin_holder->handle); + } + + Wasm* wasmOfHandle() { + if (!tls_slot_->currentThreadRegistered()) { + return nullptr; + } + auto plugin_holder = tls_slot_->get(); + if (!plugin_holder.has_value()) { + return nullptr; + } + + if (plugin_holder->handle == nullptr) { + return nullptr; + } + + return plugin_holder->handle->wasmOfHandle(); + } + const PluginSharedPtr& plugin() { return plugin_; } + +private: + PluginSharedPtr plugin_; + ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal> tls_slot_; + RemoteAsyncDataProviderPtr remote_data_provider_; +}; + +using PluginConfigPtr = std::unique_ptr<PluginConfig>; +using PluginConfigSharedPtr = std::shared_ptr<PluginConfig>; + } // namespace Wasm } // namespace Common } // namespace Extensions diff --git a/source/extensions/filters/http/wasm/config.h b/source/extensions/filters/http/wasm/config.h index f3b1141465f9..e6094ad77529 100644 --- a/source/extensions/filters/http/wasm/config.h +++ b/source/extensions/filters/http/wasm/config.h @@ -55,7 +55,7 @@ class WasmFilterConfig Extensions::Common::Wasm::CustomStatNamespace); auto filter_config = std::make_shared<FilterConfig>(proto_config, context); return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { - auto filter = filter_config->createFilter(); + auto filter = filter_config->createContext(); if (!filter) { // Fail open return; } diff --git a/source/extensions/filters/http/wasm/wasm_filter.cc b/source/extensions/filters/http/wasm/wasm_filter.cc index ba1133766191..1c3fb846dd98 100644 --- a/source/extensions/filters/http/wasm/wasm_filter.cc +++ b/source/extensions/filters/http/wasm/wasm_filter.cc @@ -6,44 +6,16 @@ namespace HttpFilters { namespace Wasm { FilterConfig::FilterConfig(const envoy::extensions::filters::http::wasm::v3::Wasm& config, - Server::Configuration::FactoryContext& context) { - auto& server = context.serverFactoryContext(); - const auto plugin = std::make_shared<Common::Wasm::Plugin>( - config.config(), context.listenerInfo().direction(), server.localInfo(), - &context.listenerInfo().metadata()); - createWasm(plugin, server, context.scope().createScope(""), context.initManager()); -} + Server::Configuration::FactoryContext& context) + : Extensions::Common::Wasm::PluginConfig( + config.config(), context.serverFactoryContext(), context.scope(), context.initManager(), + context.listenerInfo().direction(), &context.listenerInfo().metadata()) {} FilterConfig::FilterConfig(const envoy::extensions::filters::http::wasm::v3::Wasm& config, - Server::Configuration::UpstreamFactoryContext& context) { - auto& server = context.serverFactoryContext(); - const auto plugin = std::make_shared<Common::Wasm::Plugin>( - config.config(), envoy::config::core::v3::TrafficDirection::OUTBOUND, server.localInfo(), - nullptr); - createWasm(plugin, server, context.scope().createScope(""), context.initManager()); -} - -void FilterConfig::createWasm(PluginSharedPtr plugin, - Envoy::Server::Configuration::ServerFactoryContext& server, - const Stats::ScopeSharedPtr& scope, - Envoy::Init::Manager& init_manager) { - tls_slot_ = ThreadLocal::TypedSlot<Common::Wasm::PluginHandleSharedPtrThreadLocal>::makeUnique( - server.threadLocal()); - auto callback = [plugin, this](const Common::Wasm::WasmHandleSharedPtr& base_wasm) { - // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call. - tls_slot_->set([base_wasm, plugin](Event::Dispatcher& dispatcher) { - return std::make_shared<PluginHandleSharedPtrThreadLocal>( - Common::Wasm::getOrCreateThreadLocalPlugin(base_wasm, plugin, dispatcher)); - }); - }; - - if (!Common::Wasm::createWasm( - plugin, scope, server.clusterManager(), init_manager, server.mainThreadDispatcher(), - server.api(), server.lifecycleNotifier(), remote_data_provider_, std::move(callback))) { - throw Common::Wasm::WasmException( - fmt::format("Unable to create Wasm HTTP filter {}", plugin->name_)); - } -} + Server::Configuration::UpstreamFactoryContext& context) + : Extensions::Common::Wasm::PluginConfig( + config.config(), context.serverFactoryContext(), context.scope(), context.initManager(), + envoy::config::core::v3::TrafficDirection::OUTBOUND, nullptr) {} } // namespace Wasm } // namespace HttpFilters diff --git a/source/extensions/filters/http/wasm/wasm_filter.h b/source/extensions/filters/http/wasm/wasm_filter.h index ad6a3a1b4c36..46ca1955f542 100644 --- a/source/extensions/filters/http/wasm/wasm_filter.h +++ b/source/extensions/filters/http/wasm/wasm_filter.h @@ -16,49 +16,13 @@ namespace Extensions { namespace HttpFilters { namespace Wasm { -using Envoy::Extensions::Common::Wasm::Context; -using Envoy::Extensions::Common::Wasm::PluginHandleSharedPtr; -using Envoy::Extensions::Common::Wasm::PluginHandleSharedPtrThreadLocal; -using Envoy::Extensions::Common::Wasm::PluginSharedPtr; -using Envoy::Extensions::Common::Wasm::Wasm; - -class FilterConfig : Logger::Loggable<Logger::Id::wasm> { +class FilterConfig : public Extensions::Common::Wasm::PluginConfig { public: FilterConfig(const envoy::extensions::filters::http::wasm::v3::Wasm& config, Server::Configuration::FactoryContext& context); FilterConfig(const envoy::extensions::filters::http::wasm::v3::Wasm& config, Server::Configuration::UpstreamFactoryContext& context); - - std::shared_ptr<Context> createFilter() { - Wasm* wasm = nullptr; - if (!tls_slot_->currentThreadRegistered()) { - return nullptr; - } - PluginHandleSharedPtr handle = tls_slot_->get()->handle(); - if (!handle) { - return nullptr; - } - if (handle->wasmHandle()) { - wasm = handle->wasmHandle()->wasm().get(); - } - if (!wasm || wasm->isFailed()) { - if (handle->plugin()->fail_open_) { - return nullptr; // Fail open skips adding this filter to callbacks. - } else { - return std::make_shared<Context>(nullptr, 0, - handle); // Fail closed is handled by an empty Context. - } - } - return std::make_shared<Context>(wasm, handle->rootContextId(), handle); - } - -private: - void createWasm(PluginSharedPtr plugin, - Envoy::Server::Configuration::ServerFactoryContext& server, - const Stats::ScopeSharedPtr& scope, Envoy::Init::Manager& init_manager); - ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal> tls_slot_; - RemoteAsyncDataProviderPtr remote_data_provider_; }; using FilterConfigSharedPtr = std::shared_ptr<FilterConfig>; diff --git a/source/extensions/filters/network/wasm/config.cc b/source/extensions/filters/network/wasm/config.cc index 0fdde8db7a79..8723bef43e21 100644 --- a/source/extensions/filters/network/wasm/config.cc +++ b/source/extensions/filters/network/wasm/config.cc @@ -20,7 +20,7 @@ Network::FilterFactoryCb WasmFilterConfig::createFilterFactoryFromProtoTyped( Extensions::Common::Wasm::CustomStatNamespace); auto filter_config = std::make_shared<FilterConfig>(proto_config, context); return [filter_config](Network::FilterManager& filter_manager) -> void { - auto filter = filter_config->createFilter(); + auto filter = filter_config->createContext(); if (filter) { filter_manager.addFilter(filter); } // else fail open diff --git a/source/extensions/filters/network/wasm/wasm_filter.cc b/source/extensions/filters/network/wasm/wasm_filter.cc index 55fe126d8253..1b8acf680ae0 100644 --- a/source/extensions/filters/network/wasm/wasm_filter.cc +++ b/source/extensions/filters/network/wasm/wasm_filter.cc @@ -7,29 +7,9 @@ namespace Wasm { FilterConfig::FilterConfig(const envoy::extensions::filters::network::wasm::v3::Wasm& config, Server::Configuration::FactoryContext& context) - : tls_slot_(ThreadLocal::TypedSlot<Common::Wasm::PluginHandleSharedPtrThreadLocal>::makeUnique( - context.serverFactoryContext().threadLocal())) { - const auto plugin = std::make_shared<Common::Wasm::Plugin>( - config.config(), context.listenerInfo().direction(), - context.serverFactoryContext().localInfo(), &context.listenerInfo().metadata()); - - auto callback = [plugin, this](Common::Wasm::WasmHandleSharedPtr base_wasm) { - // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call. - tls_slot_->set([base_wasm, plugin](Event::Dispatcher& dispatcher) { - return std::make_shared<PluginHandleSharedPtrThreadLocal>( - Common::Wasm::getOrCreateThreadLocalPlugin(base_wasm, plugin, dispatcher)); - }); - }; - - if (!Common::Wasm::createWasm( - plugin, context.scope().createScope(""), context.serverFactoryContext().clusterManager(), - context.initManager(), context.serverFactoryContext().mainThreadDispatcher(), - context.serverFactoryContext().api(), context.serverFactoryContext().lifecycleNotifier(), - remote_data_provider_, std::move(callback))) { - throw Common::Wasm::WasmException( - fmt::format("Unable to create Wasm network filter {}", plugin->name_)); - } -} + : Extensions::Common::Wasm::PluginConfig( + config.config(), context.serverFactoryContext(), context.scope(), context.initManager(), + context.listenerInfo().direction(), &context.listenerInfo().metadata()) {} } // namespace Wasm } // namespace NetworkFilters diff --git a/source/extensions/filters/network/wasm/wasm_filter.h b/source/extensions/filters/network/wasm/wasm_filter.h index 521c227fcf0f..3bdd4401997a 100644 --- a/source/extensions/filters/network/wasm/wasm_filter.h +++ b/source/extensions/filters/network/wasm/wasm_filter.h @@ -16,45 +16,10 @@ namespace Extensions { namespace NetworkFilters { namespace Wasm { -using Envoy::Extensions::Common::Wasm::Context; -using Envoy::Extensions::Common::Wasm::PluginHandleSharedPtr; -using Envoy::Extensions::Common::Wasm::PluginHandleSharedPtrThreadLocal; -using Envoy::Extensions::Common::Wasm::PluginSharedPtr; -using Envoy::Extensions::Common::Wasm::Wasm; - -class FilterConfig : Logger::Loggable<Logger::Id::wasm> { +class FilterConfig : public Extensions::Common::Wasm::PluginConfig { public: FilterConfig(const envoy::extensions::filters::network::wasm::v3::Wasm& proto_config, Server::Configuration::FactoryContext& context); - - std::shared_ptr<Context> createFilter() { - Wasm* wasm = nullptr; - if (!tls_slot_->currentThreadRegistered()) { - return nullptr; - } - PluginHandleSharedPtr handle = tls_slot_->get()->handle(); - if (!handle) { - return nullptr; - } - if (handle->wasmHandle()) { - wasm = handle->wasmHandle()->wasm().get(); - } - if (!wasm || wasm->isFailed()) { - if (handle->plugin()->fail_open_) { - return nullptr; // Fail open skips adding this filter to callbacks. - } else { - return std::make_shared<Context>(nullptr, 0, - handle); // Fail closed is handled by an empty Context. - } - } - return std::make_shared<Context>(wasm, handle->rootContextId(), handle); - } - - Wasm* wasmForTest() { return tls_slot_->get()->handle()->wasmHandle()->wasm().get(); } - -private: - ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal> tls_slot_; - RemoteAsyncDataProviderPtr remote_data_provider_; }; using FilterConfigSharedPtr = std::shared_ptr<FilterConfig>; diff --git a/test/extensions/access_loggers/wasm/config_test.cc b/test/extensions/access_loggers/wasm/config_test.cc index 4e14e1b47590..d06845105634 100644 --- a/test/extensions/access_loggers/wasm/config_test.cc +++ b/test/extensions/access_loggers/wasm/config_test.cc @@ -88,7 +88,7 @@ TEST_P(WasmAccessLogConfigTest, CreateWasmFromEmpty) { AccessLog::InstanceSharedPtr instance; EXPECT_THROW_WITH_MESSAGE( instance = factory->createAccessLogInstance(*message, std::move(filter), context_), - Common::Wasm::WasmException, "Unable to create Wasm access log "); + Common::Wasm::WasmException, "Unable to create Wasm plugin "); } TEST_P(WasmAccessLogConfigTest, CreateWasmFromWASM) { @@ -161,7 +161,7 @@ TEST_P(WasmAccessLogConfigTest, YamlLoadFromFileWasmInvalidConfig) { TestUtility::loadFromYaml(invalid_yaml, proto_config); EXPECT_THROW_WITH_MESSAGE(factory->createAccessLogInstance(proto_config, nullptr, context_), Envoy::Extensions::Common::Wasm::WasmException, - "Unable to create Wasm access log "); + "Unable to create Wasm plugin "); const std::string valid_yaml = TestEnvironment::substitute(absl::StrCat(R"EOF( config: diff --git a/test/extensions/filters/http/wasm/config_test.cc b/test/extensions/filters/http/wasm/config_test.cc index c3f819340a71..b9a51925d2df 100644 --- a/test/extensions/filters/http/wasm/config_test.cc +++ b/test/extensions/filters/http/wasm/config_test.cc @@ -967,7 +967,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteSuccessBadcodeFailOpen) { cb(filter_callback); } -TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasmCreateFilter) { +TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasmcreateContext) { const std::string code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/filters/http/wasm/test_data/test_cpp.wasm")); const std::string sha256 = Hex::encode( @@ -1008,7 +1008,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasmCreateFilter) { setupContextServerFactoryThreadLocal(threadlocal); threadlocal.registered_ = false; auto filter_config = getFilterConfig(proto_config); - EXPECT_EQ(filter_config->createFilter(), nullptr); + EXPECT_EQ(filter_config->createContext(), nullptr); EXPECT_CALL(init_watcher_, ready()); initializeContextInitManager(init_watcher_); auto response = Http::ResponseMessagePtr{new Http::ResponseMessageImpl( @@ -1017,7 +1017,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasmCreateFilter) { async_callbacks->onSuccess(request, std::move(response)); EXPECT_EQ(getContextInitManagerState(), Init::Manager::State::Initialized); threadlocal.registered_ = true; - EXPECT_NE(filter_config->createFilter(), nullptr); + EXPECT_NE(filter_config->createContext(), nullptr); } TEST_P(WasmFilterConfigTest, FailedToGetThreadLocalPlugin) { @@ -1042,11 +1042,12 @@ TEST_P(WasmFilterConfigTest, FailedToGetThreadLocalPlugin) { threadlocal.registered_ = true; auto filter_config = getFilterConfig(proto_config); ASSERT_EQ(threadlocal.current_slot_, 1); - ASSERT_NE(filter_config->createFilter(), nullptr); + ASSERT_NE(filter_config->createContext(), nullptr); - // If the thread local plugin handle returns nullptr, `createFilter` should return nullptr - threadlocal.data_[0] = std::make_shared<PluginHandleSharedPtrThreadLocal>(nullptr); - EXPECT_EQ(filter_config->createFilter(), nullptr); + // If the thread local plugin handle returns nullptr, `createContext` should return nullptr + threadlocal.data_[0] = + std::make_shared<Extensions::Common::Wasm::PluginHandleSharedPtrThreadLocal>(nullptr); + EXPECT_EQ(filter_config->createContext(), nullptr); } } // namespace Wasm diff --git a/test/extensions/filters/network/wasm/config_test.cc b/test/extensions/filters/network/wasm/config_test.cc index dfa5810f4fb9..5be348c446f2 100644 --- a/test/extensions/filters/network/wasm/config_test.cc +++ b/test/extensions/filters/network/wasm/config_test.cc @@ -154,7 +154,7 @@ TEST_P(WasmNetworkFilterConfigTest, YamlLoadInlineBadCode) { WasmFilterConfig factory; EXPECT_THROW_WITH_MESSAGE( factory.createFilterFactoryFromProto(proto_config, context_).IgnoreError(), - Extensions::Common::Wasm::WasmException, "Unable to create Wasm network filter test"); + Extensions::Common::Wasm::WasmException, "Unable to create Wasm plugin test"); } TEST_P(WasmNetworkFilterConfigTest, YamlLoadInlineBadCodeFailOpenNackConfig) { @@ -174,7 +174,7 @@ TEST_P(WasmNetworkFilterConfigTest, YamlLoadInlineBadCodeFailOpenNackConfig) { WasmFilterConfig factory; EXPECT_THROW_WITH_MESSAGE( factory.createFilterFactoryFromProto(proto_config, context_).IgnoreError(), - Extensions::Common::Wasm::WasmException, "Unable to create Wasm network filter test"); + Extensions::Common::Wasm::WasmException, "Unable to create Wasm plugin test"); } TEST_P(WasmNetworkFilterConfigTest, FilterConfigFailClosed) { @@ -194,8 +194,8 @@ TEST_P(WasmNetworkFilterConfigTest, FilterConfigFailClosed) { envoy::extensions::filters::network::wasm::v3::Wasm proto_config; TestUtility::loadFromYaml(yaml, proto_config); NetworkFilters::Wasm::FilterConfig filter_config(proto_config, context_); - filter_config.wasmForTest()->fail(proxy_wasm::FailState::RuntimeError, ""); - auto context = filter_config.createFilter(); + filter_config.wasmOfHandle()->fail(proxy_wasm::FailState::RuntimeError, ""); + auto context = filter_config.createContext(); EXPECT_EQ(context->wasm(), nullptr); EXPECT_TRUE(context->isFailed()); } @@ -218,8 +218,8 @@ TEST_P(WasmNetworkFilterConfigTest, FilterConfigFailOpen) { envoy::extensions::filters::network::wasm::v3::Wasm proto_config; TestUtility::loadFromYaml(yaml, proto_config); NetworkFilters::Wasm::FilterConfig filter_config(proto_config, context_); - filter_config.wasmForTest()->fail(proxy_wasm::FailState::RuntimeError, ""); - EXPECT_EQ(filter_config.createFilter(), nullptr); + filter_config.wasmOfHandle()->fail(proxy_wasm::FailState::RuntimeError, ""); + EXPECT_EQ(filter_config.createContext(), nullptr); } TEST_P(WasmNetworkFilterConfigTest, FilterConfigCapabilitiesUnrestrictedByDefault) { @@ -241,12 +241,12 @@ TEST_P(WasmNetworkFilterConfigTest, FilterConfigCapabilitiesUnrestrictedByDefaul envoy::extensions::filters::network::wasm::v3::Wasm proto_config; TestUtility::loadFromYaml(yaml, proto_config); NetworkFilters::Wasm::FilterConfig filter_config(proto_config, context_); - auto wasm = filter_config.wasmForTest(); + auto wasm = filter_config.wasmOfHandle(); EXPECT_TRUE(wasm->capabilityAllowed("proxy_log")); EXPECT_TRUE(wasm->capabilityAllowed("proxy_on_vm_start")); EXPECT_TRUE(wasm->capabilityAllowed("proxy_http_call")); EXPECT_TRUE(wasm->capabilityAllowed("proxy_on_log")); - EXPECT_FALSE(filter_config.createFilter() == nullptr); + EXPECT_FALSE(filter_config.createContext() == nullptr); } TEST_P(WasmNetworkFilterConfigTest, FilterConfigCapabilityRestriction) { @@ -270,12 +270,12 @@ TEST_P(WasmNetworkFilterConfigTest, FilterConfigCapabilityRestriction) { envoy::extensions::filters::network::wasm::v3::Wasm proto_config; TestUtility::loadFromYaml(yaml, proto_config); NetworkFilters::Wasm::FilterConfig filter_config(proto_config, context_); - auto wasm = filter_config.wasmForTest(); + auto wasm = filter_config.wasmOfHandle(); EXPECT_TRUE(wasm->capabilityAllowed("proxy_log")); EXPECT_TRUE(wasm->capabilityAllowed("proxy_on_new_connection")); EXPECT_FALSE(wasm->capabilityAllowed("proxy_http_call")); EXPECT_FALSE(wasm->capabilityAllowed("proxy_on_log")); - EXPECT_FALSE(filter_config.createFilter() == nullptr); + EXPECT_FALSE(filter_config.createContext() == nullptr); } TEST_P(WasmNetworkFilterConfigTest, FilterConfigAllowOnVmStart) { @@ -409,7 +409,7 @@ TEST_P(WasmNetworkFilterConfigTest, YamlLoadFromRemoteWasmCreateFilter) { .WillRepeatedly(ReturnRef(threadlocal)); threadlocal.registered_ = false; auto filter_config = std::make_unique<FilterConfig>(proto_config, context_); - EXPECT_EQ(filter_config->createFilter(), nullptr); + EXPECT_EQ(filter_config->createContext(), nullptr); EXPECT_CALL(init_watcher_, ready()); context_.initManager().initialize(init_watcher_); auto response = Http::ResponseMessagePtr{new Http::ResponseMessageImpl( @@ -418,7 +418,7 @@ TEST_P(WasmNetworkFilterConfigTest, YamlLoadFromRemoteWasmCreateFilter) { async_callbacks->onSuccess(request, std::move(response)); EXPECT_EQ(context_.initManager().state(), Init::Manager::State::Initialized); threadlocal.registered_ = true; - EXPECT_NE(filter_config->createFilter(), nullptr); + EXPECT_NE(filter_config->createContext(), nullptr); } TEST_P(WasmNetworkFilterConfigTest, FailedToGetThreadLocalPlugin) { @@ -449,11 +449,12 @@ TEST_P(WasmNetworkFilterConfigTest, FailedToGetThreadLocalPlugin) { threadlocal.registered_ = true; auto filter_config = std::make_unique<FilterConfig>(proto_config, context_); ASSERT_EQ(threadlocal.current_slot_, 1); - ASSERT_NE(filter_config->createFilter(), nullptr); + ASSERT_NE(filter_config->createContext(), nullptr); - // If the thread local plugin handle returns nullptr, `createFilter` should return nullptr - threadlocal.data_[0] = std::make_shared<PluginHandleSharedPtrThreadLocal>(nullptr); - EXPECT_EQ(filter_config->createFilter(), nullptr); + // If the thread local plugin handle returns nullptr, `createContext` should return nullptr + threadlocal.data_[0] = + std::make_shared<Extensions::Common::Wasm::PluginHandleSharedPtrThreadLocal>(nullptr); + EXPECT_EQ(filter_config->createContext(), nullptr); } } // namespace Wasm diff --git a/test/extensions/filters/network/wasm/wasm_filter_test.cc b/test/extensions/filters/network/wasm/wasm_filter_test.cc index c13975701fdf..68479616fdaf 100644 --- a/test/extensions/filters/network/wasm/wasm_filter_test.cc +++ b/test/extensions/filters/network/wasm/wasm_filter_test.cc @@ -28,7 +28,8 @@ using proxy_wasm::ContextBase; class TestFilter : public Context { public: - TestFilter(Wasm* wasm, uint32_t root_context_id, PluginHandleSharedPtr plugin_handle) + TestFilter(Wasm* wasm, uint32_t root_context_id, + Common::Wasm::PluginHandleSharedPtr plugin_handle) : Context(wasm, root_context_id, plugin_handle) {} MOCK_CONTEXT_LOG_; From 6106740a48c74686695ef709b777a10fe9be9067 Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Sun, 13 Oct 2024 10:32:52 +0800 Subject: [PATCH 02/10] fix test Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- source/extensions/common/wasm/wasm.cc | 9 +++++---- source/extensions/common/wasm/wasm.h | 11 ++++++----- test/extensions/filters/http/wasm/config_test.cc | 16 ++++++++-------- .../filters/network/wasm/config_test.cc | 2 +- 4 files changed, 20 insertions(+), 18 deletions(-) diff --git a/source/extensions/common/wasm/wasm.cc b/source/extensions/common/wasm/wasm.cc index 80f7bc2404fe..e4a8966bb69e 100644 --- a/source/extensions/common/wasm/wasm.cc +++ b/source/extensions/common/wasm/wasm.cc @@ -489,15 +489,16 @@ PluginConfig::PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& conf Server::Configuration::ServerFactoryContext& server, Stats::Scope& scope, Init::Manager& init_manager, envoy::config::core::v3::TrafficDirection direction, - const envoy::config::core::v3::Metadata* metadata) { - tls_slot_ = ThreadLocal::TypedSlot<Common::Wasm::PluginHandleSharedPtrThreadLocal>::makeUnique( - server.threadLocal()); + const envoy::config::core::v3::Metadata* metadata) + : tls_slot_(server.threadLocal()) { plugin_ = std::make_shared<Common::Wasm::Plugin>(config, direction, server.localInfo(), metadata); auto callback = [this](const Common::Wasm::WasmHandleSharedPtr& base_wasm) { + tls_slot_was_set_ = true; + // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call. - tls_slot_->set([base_wasm, plugin = this->plugin_](Event::Dispatcher& dispatcher) { + tls_slot_.set([base_wasm, plugin = this->plugin_](Event::Dispatcher& dispatcher) { return std::make_shared<PluginHandleSharedPtrThreadLocal>( getOrCreateThreadLocalPlugin(base_wasm, plugin, dispatcher)); }); diff --git a/source/extensions/common/wasm/wasm.h b/source/extensions/common/wasm/wasm.h index 06e3738c725e..0f8229277294 100644 --- a/source/extensions/common/wasm/wasm.h +++ b/source/extensions/common/wasm/wasm.h @@ -192,10 +192,10 @@ class PluginConfig : Logger::Loggable<Logger::Id::wasm> { const envoy::config::core::v3::Metadata* metadata); std::shared_ptr<Context> createContext() { - if (!tls_slot_->currentThreadRegistered()) { + if (!tls_slot_was_set_ || !tls_slot_.currentThreadRegistered()) { return nullptr; } - auto plugin_holder = tls_slot_->get(); + auto plugin_holder = tls_slot_.get(); if (!plugin_holder.has_value()) { return nullptr; } @@ -220,10 +220,10 @@ class PluginConfig : Logger::Loggable<Logger::Id::wasm> { } Wasm* wasmOfHandle() { - if (!tls_slot_->currentThreadRegistered()) { + if (!tls_slot_was_set_ || !tls_slot_.currentThreadRegistered()) { return nullptr; } - auto plugin_holder = tls_slot_->get(); + auto plugin_holder = tls_slot_.get(); if (!plugin_holder.has_value()) { return nullptr; } @@ -238,7 +238,8 @@ class PluginConfig : Logger::Loggable<Logger::Id::wasm> { private: PluginSharedPtr plugin_; - ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal> tls_slot_; + ThreadLocal::TypedSlot<PluginHandleSharedPtrThreadLocal> tls_slot_; + bool tls_slot_was_set_{}; RemoteAsyncDataProviderPtr remote_data_provider_; }; diff --git a/test/extensions/filters/http/wasm/config_test.cc b/test/extensions/filters/http/wasm/config_test.cc index b9a51925d2df..2123a78094a6 100644 --- a/test/extensions/filters/http/wasm/config_test.cc +++ b/test/extensions/filters/http/wasm/config_test.cc @@ -268,7 +268,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromFileWasmInvalidConfig) { TestUtility::loadFromYaml(invalid_yaml, proto_config); WasmFilterConfig factory; EXPECT_THROW_WITH_MESSAGE(getFilterFactoryCb(proto_config, factory).status().IgnoreError(), - WasmException, "Unable to create Wasm HTTP filter "); + WasmException, "Unable to create Wasm plugin "); const std::string valid_yaml = TestEnvironment::substitute(absl::StrCat(R"EOF( config: @@ -337,7 +337,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadInlineBadCode) { TestUtility::loadFromYaml(yaml, proto_config); WasmFilterConfig factory; EXPECT_THROW_WITH_MESSAGE(getFilterFactoryCb(proto_config, factory).status().IgnoreError(), - WasmException, "Unable to create Wasm HTTP filter "); + WasmException, "Unable to create Wasm plugin "); } TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasm) { @@ -430,7 +430,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasmFailOnUncachedThenSucceed) { })); EXPECT_THROW_WITH_MESSAGE(getFilterFactoryCb(proto_config, factory).status().IgnoreError(), - WasmException, "Unable to create Wasm HTTP filter "); + WasmException, "Unable to create Wasm plugin "); EXPECT_CALL(init_watcher_, ready()); initializeContextInitManager(init_watcher_); @@ -506,10 +506,10 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasmFailCachedThenSucceed) { // Case 1: fail and fetch in the background, got 503, cache failure. EXPECT_THROW_WITH_MESSAGE(getFilterFactoryCb(proto_config, factory).status().IgnoreError(), - WasmException, "Unable to create Wasm HTTP filter "); + WasmException, "Unable to create Wasm plugin "); // Fail a second time because we are in-progress. EXPECT_THROW_WITH_MESSAGE(getFilterFactoryCb(proto_config, factory).status().IgnoreError(), - WasmException, "Unable to create Wasm HTTP filter "); + WasmException, "Unable to create Wasm plugin "); async_callbacks->onSuccess( request, Http::ResponseMessagePtr{new Http::ResponseMessageImpl(Http::ResponseHeaderMapPtr{ new Http::TestResponseHeaderMapImpl{{":status", "503"}}})}); @@ -524,7 +524,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasmFailCachedThenSucceed) { setupContextInitManager(init_manager2); EXPECT_THROW_WITH_MESSAGE(getFilterFactoryCb(proto_config, factory).status().IgnoreError(), - WasmException, "Unable to create Wasm HTTP filter "); + WasmException, "Unable to create Wasm plugin "); EXPECT_CALL(init_watcher2, ready()); init_manager2.initialize(init_watcher2); @@ -552,7 +552,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasmFailCachedThenSucceed) { setupContextInitManager(init_manager3); EXPECT_THROW_WITH_MESSAGE(getFilterFactoryCb(proto_config, factory).status().IgnoreError(), - WasmException, "Unable to create Wasm HTTP filter "); + WasmException, "Unable to create Wasm plugin "); EXPECT_CALL(init_watcher3, ready()); init_manager3.initialize(init_watcher3); @@ -609,7 +609,7 @@ TEST_P(WasmFilterConfigTest, YamlLoadFromRemoteWasmFailCachedThenSucceed) { setupContextInitManager(init_manager5); EXPECT_THROW_WITH_MESSAGE(getFilterFactoryCb(proto_config2, factory).status().IgnoreError(), - WasmException, "Unable to create Wasm HTTP filter "); + WasmException, "Unable to create Wasm plugin "); EXPECT_CALL(init_watcher_, ready()); initializeContextInitManager(init_watcher_); diff --git a/test/extensions/filters/network/wasm/config_test.cc b/test/extensions/filters/network/wasm/config_test.cc index 5be348c446f2..fff53f8819cc 100644 --- a/test/extensions/filters/network/wasm/config_test.cc +++ b/test/extensions/filters/network/wasm/config_test.cc @@ -336,7 +336,7 @@ TEST_P(WasmNetworkFilterConfigTest, YamlLoadFromFileWasmInvalidConfig) { WasmFilterConfig factory; EXPECT_THROW_WITH_MESSAGE( factory.createFilterFactoryFromProto(proto_config, context_).IgnoreError(), - Envoy::Extensions::Common::Wasm::WasmException, "Unable to create Wasm network filter "); + Envoy::Extensions::Common::Wasm::WasmException, "Unable to create Wasm plugin "); const std::string valid_yaml = TestEnvironment::substitute(absl::StrCat(R"EOF( config: From 2cc8acbea9c7efa6fd32e5c245fcbceaeca1b6fe Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Sun, 13 Oct 2024 12:33:37 +0800 Subject: [PATCH 03/10] support singleton Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- .../extensions/access_loggers/wasm/config.cc | 2 +- source/extensions/bootstrap/wasm/config.cc | 46 +-------- source/extensions/bootstrap/wasm/config.h | 17 +--- source/extensions/common/wasm/wasm.cc | 97 ++++++++++++++++--- source/extensions/common/wasm/wasm.h | 60 +++--------- .../filters/http/wasm/wasm_filter.cc | 4 +- .../filters/network/wasm/wasm_filter.cc | 2 +- source/extensions/stat_sinks/wasm/config.cc | 32 +----- .../stat_sinks/wasm/wasm_stat_sink_impl.h | 16 ++- test/extensions/bootstrap/wasm/config_test.cc | 10 +- .../stats_sinks/wasm/config_test.cc | 4 +- 11 files changed, 126 insertions(+), 164 deletions(-) diff --git a/source/extensions/access_loggers/wasm/config.cc b/source/extensions/access_loggers/wasm/config.cc index ba72d7335f44..e79cf821d43c 100644 --- a/source/extensions/access_loggers/wasm/config.cc +++ b/source/extensions/access_loggers/wasm/config.cc @@ -24,7 +24,7 @@ WasmAccessLogFactory::createAccessLogInstance(const Protobuf::Message& proto_con auto plugin_config = std::make_unique<Common::Wasm::PluginConfig>( config.config(), context.serverFactoryContext(), context.scope(), context.initManager(), - envoy::config::core::v3::TrafficDirection::UNSPECIFIED, nullptr /* metadata */); + envoy::config::core::v3::TrafficDirection::UNSPECIFIED, nullptr /* metadata */, false); auto access_log = std::make_shared<WasmAccessLog>(std::move(plugin_config), std::move(filter)); context.serverFactoryContext().api().customStatNamespaces().registerStatNamespace( diff --git a/source/extensions/bootstrap/wasm/config.cc b/source/extensions/bootstrap/wasm/config.cc index 4f34f7e88dab..218cbecb7739 100644 --- a/source/extensions/bootstrap/wasm/config.cc +++ b/source/extensions/bootstrap/wasm/config.cc @@ -16,53 +16,15 @@ namespace Wasm { void WasmServiceExtension::onServerInitialized() { createWasm(context_); } void WasmServiceExtension::createWasm(Server::Configuration::ServerFactoryContext& context) { - auto plugin = std::make_shared<Common::Wasm::Plugin>( - config_.config(), envoy::config::core::v3::TrafficDirection::UNSPECIFIED, context.localInfo(), - nullptr); - - auto callback = [this, &context, plugin](Common::Wasm::WasmHandleSharedPtr base_wasm) { - if (!base_wasm) { - if (plugin->fail_open_) { - ENVOY_LOG(error, "Unable to create Wasm service {}", plugin->name_); - } else { - ENVOY_LOG(critical, "Unable to create Wasm service {}", plugin->name_); - } - return; - } - if (config_.singleton()) { - // Return a Wasm VM which will be stored as a singleton by the Server. - wasm_service_ = std::make_unique<WasmService>( - plugin, Common::Wasm::getOrCreateThreadLocalPlugin(base_wasm, plugin, - context.mainThreadDispatcher())); - return; - } - // Per-thread WASM VM. - // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call. - auto tls_slot = - ThreadLocal::TypedSlot<Common::Wasm::PluginHandleSharedPtrThreadLocal>::makeUnique( - context.threadLocal()); - tls_slot->set([base_wasm, plugin](Event::Dispatcher& dispatcher) { - return std::make_shared<Common::Wasm::PluginHandleSharedPtrThreadLocal>( - Common::Wasm::getOrCreateThreadLocalPlugin(base_wasm, plugin, dispatcher)); - }); - wasm_service_ = std::make_unique<WasmService>(plugin, std::move(tls_slot)); - }; - - if (!Common::Wasm::createWasm(plugin, context.scope().createScope(""), context.clusterManager(), - context.initManager(), context.mainThreadDispatcher(), - context.api(), context.lifecycleNotifier(), remote_data_provider_, - std::move(callback))) { - // NB: throw if we get a synchronous configuration failures as this is how such failures are - // reported to xDS. - throw Common::Wasm::WasmException( - fmt::format("Unable to create Wasm service {}", plugin->name_)); - } + wasm_service_ = std::make_unique<Common::Wasm::PluginConfig>( + config_.config(), context, context.scope(), context.initManager(), + envoy::config::core::v3::TrafficDirection::UNSPECIFIED, nullptr, config_.singleton()); } Server::BootstrapExtensionPtr WasmFactory::createBootstrapExtension(const Protobuf::Message& config, Server::Configuration::ServerFactoryContext& context) { - auto typed_config = + const auto& typed_config = MessageUtil::downcastAndValidate<const envoy::extensions::wasm::v3::WasmService&>( config, context.messageValidationContext().staticValidationVisitor()); context.api().customStatNamespaces().registerStatNamespace( diff --git a/source/extensions/bootstrap/wasm/config.h b/source/extensions/bootstrap/wasm/config.h index fc24e085cc7c..ca86e1208323 100644 --- a/source/extensions/bootstrap/wasm/config.h +++ b/source/extensions/bootstrap/wasm/config.h @@ -19,21 +19,7 @@ namespace Wasm { using Common::Wasm::PluginHandleSharedPtrThreadLocal; using Envoy::Extensions::Common::Wasm::PluginHandleSharedPtr; using Envoy::Extensions::Common::Wasm::PluginSharedPtr; - -class WasmService { -public: - WasmService(PluginSharedPtr plugin, PluginHandleSharedPtr singleton) - : plugin_(plugin), singleton_(std::move(singleton)) {} - WasmService(PluginSharedPtr plugin, - ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal>&& tls_slot) - : plugin_(plugin), tls_slot_(std::move(tls_slot)) {} - -private: - PluginSharedPtr plugin_; - PluginHandleSharedPtr singleton_; - ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal> tls_slot_; -}; - +using WasmService = Common::Wasm::PluginConfig; using WasmServicePtr = std::unique_ptr<WasmService>; class WasmFactory : public Server::Configuration::BootstrapExtensionFactory { @@ -65,7 +51,6 @@ class WasmServiceExtension : public Server::BootstrapExtension, Logger::Loggable envoy::extensions::wasm::v3::WasmService config_; Server::Configuration::ServerFactoryContext& context_; WasmServicePtr wasm_service_; - RemoteAsyncDataProviderPtr remote_data_provider_; }; } // namespace Wasm diff --git a/source/extensions/common/wasm/wasm.cc b/source/extensions/common/wasm/wasm.cc index e4a8966bb69e..9979bc014d44 100644 --- a/source/extensions/common/wasm/wasm.cc +++ b/source/extensions/common/wasm/wasm.cc @@ -486,33 +486,108 @@ getOrCreateThreadLocalPlugin(const WasmHandleSharedPtr& base_wasm, const PluginS } PluginConfig::PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& config, - Server::Configuration::ServerFactoryContext& server, Stats::Scope& scope, - Init::Manager& init_manager, + Server::Configuration::ServerFactoryContext& context, + Stats::Scope& scope, Init::Manager& init_manager, envoy::config::core::v3::TrafficDirection direction, - const envoy::config::core::v3::Metadata* metadata) - : tls_slot_(server.threadLocal()) { + const envoy::config::core::v3::Metadata* metadata, bool singleton) + : is_singleton_handle_(singleton) { - plugin_ = std::make_shared<Common::Wasm::Plugin>(config, direction, server.localInfo(), metadata); + plugin_ = std::make_shared<Plugin>(config, direction, context.localInfo(), metadata); - auto callback = [this](const Common::Wasm::WasmHandleSharedPtr& base_wasm) { - tls_slot_was_set_ = true; + auto callback = [this, &context](WasmHandleSharedPtr base_wasm) { + plugin_handle_initialized_ = true; + if (base_wasm == nullptr) { + ENVOY_LOG(critical, "Plugin {} failed to load", plugin_->name_); + } + + if (is_singleton_handle_) { + singleton_handle_ = + getOrCreateThreadLocalPlugin(base_wasm, plugin_, context.mainThreadDispatcher()); + return; + } + thread_local_handle_ = + ThreadLocal::TypedSlot<Common::Wasm::PluginHandleSharedPtrThreadLocal>::makeUnique( + context.threadLocal()); // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call. - tls_slot_.set([base_wasm, plugin = this->plugin_](Event::Dispatcher& dispatcher) { + thread_local_handle_->set([base_wasm, plugin = this->plugin_](Event::Dispatcher& dispatcher) { return std::make_shared<PluginHandleSharedPtrThreadLocal>( getOrCreateThreadLocalPlugin(base_wasm, plugin, dispatcher)); }); }; - if (!Common::Wasm::createWasm(plugin_, scope.createScope(""), server.clusterManager(), - init_manager, server.mainThreadDispatcher(), server.api(), - server.lifecycleNotifier(), remote_data_provider_, + if (!Common::Wasm::createWasm(plugin_, scope.createScope(""), context.clusterManager(), + init_manager, context.mainThreadDispatcher(), context.api(), + context.lifecycleNotifier(), remote_data_provider_, std::move(callback))) { throw Common::Wasm::WasmException( fmt::format("Unable to create Wasm plugin {}", plugin_->name_)); } } +std::shared_ptr<Context> PluginConfig::createContext() { + if (!plugin_handle_initialized_) { + return nullptr; + } + + if (is_singleton_handle_) { + // Use critical log because this error should not happen in production. + ENVOY_LOG(critical, "CreateContext() only works for thread local plugins."); + return nullptr; + } + + if (!thread_local_handle_->currentThreadRegistered()) { + return nullptr; + } + auto plugin_holder = thread_local_handle_->get(); + if (!plugin_holder.has_value()) { + return nullptr; + } + + if (plugin_holder->handle == nullptr) { + return nullptr; + } + + Wasm* wasm = plugin_holder->handle->wasmOfHandle(); + + if (!wasm || wasm->isFailed()) { + if (plugin_->fail_open_) { + return nullptr; // Fail open skips adding this filter to callbacks. + } else { + return std::make_shared<Context>( + nullptr, 0, + plugin_holder->handle); // Fail closed is handled by an empty Context. + } + } + return std::make_shared<Context>(wasm, plugin_holder->handle->rootContextId(), + plugin_holder->handle); +} + +Wasm* PluginConfig::wasmOfHandle() { + if (!plugin_handle_initialized_) { + return nullptr; + } + + if (singleton_handle_ != nullptr) { + return singleton_handle_->wasmOfHandle(); + } + + ASSERT(thread_local_handle_ != nullptr); + if (!thread_local_handle_->currentThreadRegistered()) { + return nullptr; + } + auto plugin_holder = thread_local_handle_->get(); + if (!plugin_holder.has_value()) { + return nullptr; + } + + if (plugin_holder->handle == nullptr) { + return nullptr; + } + + return plugin_holder->handle->wasmOfHandle(); +} + } // namespace Wasm } // namespace Common } // namespace Extensions diff --git a/source/extensions/common/wasm/wasm.h b/source/extensions/common/wasm/wasm.h index 0f8229277294..1d7d629be388 100644 --- a/source/extensions/common/wasm/wasm.h +++ b/source/extensions/common/wasm/wasm.h @@ -187,60 +187,26 @@ WasmEvent toWasmEvent(const std::shared_ptr<WasmHandleBase>& wasm); class PluginConfig : Logger::Loggable<Logger::Id::wasm> { public: PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& config, - Server::Configuration::ServerFactoryContext& server, Stats::Scope& scope, + Server::Configuration::ServerFactoryContext& context, Stats::Scope& scope, Init::Manager& init_manager, envoy::config::core::v3::TrafficDirection direction, - const envoy::config::core::v3::Metadata* metadata); - - std::shared_ptr<Context> createContext() { - if (!tls_slot_was_set_ || !tls_slot_.currentThreadRegistered()) { - return nullptr; - } - auto plugin_holder = tls_slot_.get(); - if (!plugin_holder.has_value()) { - return nullptr; - } - - if (plugin_holder->handle == nullptr) { - return nullptr; - } - - Wasm* wasm = plugin_holder->wasmOfHandle(); - - if (!wasm || wasm->isFailed()) { - if (plugin_->fail_open_) { - return nullptr; // Fail open skips adding this filter to callbacks. - } else { - return std::make_shared<Context>( - nullptr, 0, - plugin_holder->handle); // Fail closed is handled by an empty Context. - } - } - return std::make_shared<Context>(wasm, plugin_holder->handle->rootContextId(), - plugin_holder->handle); - } - - Wasm* wasmOfHandle() { - if (!tls_slot_was_set_ || !tls_slot_.currentThreadRegistered()) { - return nullptr; - } - auto plugin_holder = tls_slot_.get(); - if (!plugin_holder.has_value()) { - return nullptr; - } + const envoy::config::core::v3::Metadata* metadata, bool singleton); - if (plugin_holder->handle == nullptr) { - return nullptr; - } - - return plugin_holder->handle->wasmOfHandle(); - } + std::shared_ptr<Context> createContext(); + Wasm* wasmOfHandle(); const PluginSharedPtr& plugin() { return plugin_; } private: PluginSharedPtr plugin_; - ThreadLocal::TypedSlot<PluginHandleSharedPtrThreadLocal> tls_slot_; - bool tls_slot_was_set_{}; RemoteAsyncDataProviderPtr remote_data_provider_; + const bool is_singleton_handle_{}; + + bool plugin_handle_initialized_{}; + // Plugin handle that works for all threads. Only one of thread_local_handle_ or + // singleton_handle_ will be set. + ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal> thread_local_handle_; + // Plugin handle that works for the main. Only one of thread_local_handle_ or + // singleton_handle_ will be set. + PluginHandleSharedPtr singleton_handle_; }; using PluginConfigPtr = std::unique_ptr<PluginConfig>; diff --git a/source/extensions/filters/http/wasm/wasm_filter.cc b/source/extensions/filters/http/wasm/wasm_filter.cc index 1c3fb846dd98..2f53cd148efd 100644 --- a/source/extensions/filters/http/wasm/wasm_filter.cc +++ b/source/extensions/filters/http/wasm/wasm_filter.cc @@ -9,13 +9,13 @@ FilterConfig::FilterConfig(const envoy::extensions::filters::http::wasm::v3::Was Server::Configuration::FactoryContext& context) : Extensions::Common::Wasm::PluginConfig( config.config(), context.serverFactoryContext(), context.scope(), context.initManager(), - context.listenerInfo().direction(), &context.listenerInfo().metadata()) {} + context.listenerInfo().direction(), &context.listenerInfo().metadata(), false) {} FilterConfig::FilterConfig(const envoy::extensions::filters::http::wasm::v3::Wasm& config, Server::Configuration::UpstreamFactoryContext& context) : Extensions::Common::Wasm::PluginConfig( config.config(), context.serverFactoryContext(), context.scope(), context.initManager(), - envoy::config::core::v3::TrafficDirection::OUTBOUND, nullptr) {} + envoy::config::core::v3::TrafficDirection::OUTBOUND, nullptr, false) {} } // namespace Wasm } // namespace HttpFilters diff --git a/source/extensions/filters/network/wasm/wasm_filter.cc b/source/extensions/filters/network/wasm/wasm_filter.cc index 1b8acf680ae0..8b9772ecd947 100644 --- a/source/extensions/filters/network/wasm/wasm_filter.cc +++ b/source/extensions/filters/network/wasm/wasm_filter.cc @@ -9,7 +9,7 @@ FilterConfig::FilterConfig(const envoy::extensions::filters::network::wasm::v3:: Server::Configuration::FactoryContext& context) : Extensions::Common::Wasm::PluginConfig( config.config(), context.serverFactoryContext(), context.scope(), context.initManager(), - context.listenerInfo().direction(), &context.listenerInfo().metadata()) {} + context.listenerInfo().direction(), &context.listenerInfo().metadata(), false) {} } // namespace Wasm } // namespace NetworkFilters diff --git a/source/extensions/stat_sinks/wasm/config.cc b/source/extensions/stat_sinks/wasm/config.cc index be72fb3d1861..ef629dad80aa 100644 --- a/source/extensions/stat_sinks/wasm/config.cc +++ b/source/extensions/stat_sinks/wasm/config.cc @@ -21,36 +21,14 @@ WasmSinkFactory::createStatsSink(const Protobuf::Message& proto_config, MessageUtil::downcastAndValidate<const envoy::extensions::stat_sinks::wasm::v3::Wasm&>( proto_config, context.messageValidationContext().staticValidationVisitor()); - auto plugin = std::make_shared<Common::Wasm::Plugin>( - config.config(), envoy::config::core::v3::TrafficDirection::UNSPECIFIED, context.localInfo(), - nullptr); - - auto wasm_sink = std::make_unique<WasmStatSink>(plugin, nullptr); - - auto callback = [&wasm_sink, &context, plugin](Common::Wasm::WasmHandleSharedPtr base_wasm) { - if (!base_wasm) { - if (plugin->fail_open_) { - ENVOY_LOG(error, "Unable to create Wasm Stat Sink {}", plugin->name_); - } else { - ENVOY_LOG(critical, "Unable to create Wasm Stat Sink {}", plugin->name_); - } - return; - } - wasm_sink->setSingleton(Common::Wasm::getOrCreateThreadLocalPlugin( - base_wasm, plugin, context.mainThreadDispatcher())); - }; - - if (!Common::Wasm::createWasm(plugin, context.scope().createScope(""), context.clusterManager(), - context.initManager(), context.mainThreadDispatcher(), - context.api(), context.lifecycleNotifier(), remote_data_provider_, - std::move(callback))) { - throw Common::Wasm::WasmException( - fmt::format("Unable to create Wasm Stat Sink {}", plugin->name_)); - } + auto plugin_config = std::make_unique<Common::Wasm::PluginConfig>( + config.config(), context, context.scope(), context.initManager(), + envoy::config::core::v3::TrafficDirection::UNSPECIFIED, nullptr, true); context.api().customStatNamespaces().registerStatNamespace( Extensions::Common::Wasm::CustomStatNamespace); - return wasm_sink; + + return std::make_unique<WasmStatSink>(std::move(plugin_config)); } ProtobufTypes::MessagePtr WasmSinkFactory::createEmptyConfigProto() { diff --git a/source/extensions/stat_sinks/wasm/wasm_stat_sink_impl.h b/source/extensions/stat_sinks/wasm/wasm_stat_sink_impl.h index d9459bb45fc8..f4539a60926a 100644 --- a/source/extensions/stat_sinks/wasm/wasm_stat_sink_impl.h +++ b/source/extensions/stat_sinks/wasm/wasm_stat_sink_impl.h @@ -17,16 +17,13 @@ using Envoy::Extensions::Common::Wasm::PluginSharedPtr; class WasmStatSink : public Stats::Sink { public: - WasmStatSink(const PluginSharedPtr& plugin, PluginHandleSharedPtr singleton) - : plugin_(plugin), singleton_(singleton) {} + WasmStatSink(Common::Wasm::PluginConfigPtr plugin_config) + : plugin_config_(std::move(plugin_config)) {} void flush(Stats::MetricSnapshot& snapshot) override { - singleton_->wasmHandle()->wasm()->onStatsUpdate(plugin_, snapshot); - } - - void setSingleton(PluginHandleSharedPtr singleton) { - ASSERT(singleton != nullptr); - singleton_ = singleton; + if (Common::Wasm::Wasm* wasm = plugin_config_->wasmOfHandle(); wasm != nullptr) { + wasm->onStatsUpdate(plugin_config_->plugin(), snapshot); + } } void onHistogramComplete(const Stats::Histogram& histogram, uint64_t value) override { @@ -35,8 +32,7 @@ class WasmStatSink : public Stats::Sink { } private: - PluginSharedPtr plugin_; - PluginHandleSharedPtr singleton_; + Common::Wasm::PluginConfigPtr plugin_config_; }; } // namespace Wasm diff --git a/test/extensions/bootstrap/wasm/config_test.cc b/test/extensions/bootstrap/wasm/config_test.cc index 717ee20bb038..d02444bffdb4 100644 --- a/test/extensions/bootstrap/wasm/config_test.cc +++ b/test/extensions/bootstrap/wasm/config_test.cc @@ -101,7 +101,7 @@ TEST_P(WasmFactoryTest, MissingImport) { TestEnvironment::substitute( "{{ test_rundir }}/test/extensions/bootstrap/wasm/test_data/missing_cpp.wasm")); EXPECT_THROW_WITH_MESSAGE(initializeWithConfig(config_), Extensions::Common::Wasm::WasmException, - "Unable to create Wasm service test"); + "Unable to create Wasm plugin test"); } TEST_P(WasmFactoryTest, UnspecifiedRuntime) { @@ -117,7 +117,7 @@ TEST_P(WasmFactoryTest, UnknownRuntime) { config_.mutable_config()->mutable_vm_config()->set_runtime("envoy.wasm.runtime.invalid"); EXPECT_THROW_WITH_MESSAGE(initializeWithConfig(config_), Extensions::Common::Wasm::WasmException, - "Unable to create Wasm service test"); + "Unable to create Wasm plugin test"); } TEST_P(WasmFactoryTest, StartFailed) { @@ -127,7 +127,7 @@ TEST_P(WasmFactoryTest, StartFailed) { plugin_configuration); EXPECT_THROW_WITH_MESSAGE(initializeWithConfig(config_), Extensions::Common::Wasm::WasmException, - "Unable to create Wasm service test"); + "Unable to create Wasm plugin test"); } TEST_P(WasmFactoryTest, StartFailedOpen) { @@ -138,7 +138,7 @@ TEST_P(WasmFactoryTest, StartFailedOpen) { config_.mutable_config()->set_fail_open(true); EXPECT_THROW_WITH_MESSAGE(initializeWithConfig(config_), Extensions::Common::Wasm::WasmException, - "Unable to create Wasm service test"); + "Unable to create Wasm plugin test"); } TEST_P(WasmFactoryTest, ConfigureFailed) { @@ -147,7 +147,7 @@ TEST_P(WasmFactoryTest, ConfigureFailed) { config_.mutable_config()->mutable_configuration()->PackFrom(plugin_configuration); EXPECT_THROW_WITH_MESSAGE(initializeWithConfig(config_), Extensions::Common::Wasm::WasmException, - "Unable to create Wasm service test"); + "Unable to create Wasm plugin test"); } } // namespace Wasm diff --git a/test/extensions/stats_sinks/wasm/config_test.cc b/test/extensions/stats_sinks/wasm/config_test.cc index cb46b22b1c55..9c471783d7bf 100644 --- a/test/extensions/stats_sinks/wasm/config_test.cc +++ b/test/extensions/stats_sinks/wasm/config_test.cc @@ -71,14 +71,14 @@ INSTANTIATE_TEST_SUITE_P(Runtimes, WasmStatSinkConfigTest, TEST_P(WasmStatSinkConfigTest, CreateWasmFromEmpty) { envoy::extensions::stat_sinks::wasm::v3::Wasm config; EXPECT_THROW_WITH_MESSAGE(initializeWithConfig(config), Extensions::Common::Wasm::WasmException, - "Unable to create Wasm Stat Sink "); + "Unable to create Wasm plugin "); } TEST_P(WasmStatSinkConfigTest, CreateWasmFailOpen) { envoy::extensions::stat_sinks::wasm::v3::Wasm config; config.mutable_config()->set_fail_open(true); EXPECT_THROW_WITH_MESSAGE(initializeWithConfig(config), Extensions::Common::Wasm::WasmException, - "Unable to create Wasm Stat Sink "); + "Unable to create Wasm plugin "); } TEST_P(WasmStatSinkConfigTest, CreateWasmFromWASM) { From 3a8fd6789d08a5c3e16c5e8ffc158c3d523240d0 Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Mon, 14 Oct 2024 10:03:47 +0800 Subject: [PATCH 04/10] improve coverage Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- source/extensions/common/wasm/remote_async_datasource.cc | 6 ------ source/extensions/common/wasm/wasm.h | 2 -- 2 files changed, 8 deletions(-) diff --git a/source/extensions/common/wasm/remote_async_datasource.cc b/source/extensions/common/wasm/remote_async_datasource.cc index b3e8989e7a9a..ec51b0facb5d 100644 --- a/source/extensions/common/wasm/remote_async_datasource.cc +++ b/source/extensions/common/wasm/remote_async_datasource.cc @@ -13,12 +13,6 @@ static constexpr uint32_t RetryInitialDelayMilliseconds = 1000; static constexpr uint32_t RetryMaxDelayMilliseconds = 10 * 1000; static constexpr uint32_t RetryCount = 1; -absl::optional<std::string> getPath(const envoy::config::core::v3::DataSource& source) { - return source.specifier_case() == envoy::config::core::v3::DataSource::SpecifierCase::kFilename - ? absl::make_optional(source.filename()) - : absl::nullopt; -} - RemoteAsyncDataProvider::RemoteAsyncDataProvider( Upstream::ClusterManager& cm, Init::Manager& manager, const envoy::config::core::v3::RemoteDataSource& source, Event::Dispatcher& dispatcher, diff --git a/source/extensions/common/wasm/wasm.h b/source/extensions/common/wasm/wasm.h index 1d7d629be388..148bc3a61a0f 100644 --- a/source/extensions/common/wasm/wasm.h +++ b/source/extensions/common/wasm/wasm.h @@ -157,8 +157,6 @@ using PluginHandleSharedPtr = std::shared_ptr<PluginHandle>; class PluginHandleSharedPtrThreadLocal : public ThreadLocal::ThreadLocalObject { public: PluginHandleSharedPtrThreadLocal(PluginHandleSharedPtr handle) : handle(std::move(handle)) {} - Wasm* wasmOfHandle() { return handle != nullptr ? handle->wasmOfHandle() : nullptr; } - void updateHandle(PluginHandleSharedPtr new_handle) { handle = std::move(new_handle); } PluginHandleSharedPtr handle; }; From 91f2db42e0e64ee451f726cc44f81223efc84891 Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Mon, 14 Oct 2024 11:26:33 +0800 Subject: [PATCH 05/10] check check Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- source/extensions/common/wasm/wasm.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/common/wasm/wasm.cc b/source/extensions/common/wasm/wasm.cc index 9979bc014d44..c60100463cf8 100644 --- a/source/extensions/common/wasm/wasm.cc +++ b/source/extensions/common/wasm/wasm.cc @@ -568,8 +568,8 @@ Wasm* PluginConfig::wasmOfHandle() { return nullptr; } - if (singleton_handle_ != nullptr) { - return singleton_handle_->wasmOfHandle(); + if (is_singleton_handle_) { + return singleton_handle_ != nullptr ? singleton_handle_->wasmOfHandle() : nullptr; } ASSERT(thread_local_handle_ != nullptr); From 82700670e88992b56b109587bee039f6922cbaee Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Tue, 15 Oct 2024 11:03:01 +0800 Subject: [PATCH 06/10] address some comments Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- source/extensions/common/wasm/wasm.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/extensions/common/wasm/wasm.cc b/source/extensions/common/wasm/wasm.cc index c60100463cf8..54279adcacb5 100644 --- a/source/extensions/common/wasm/wasm.cc +++ b/source/extensions/common/wasm/wasm.cc @@ -492,6 +492,8 @@ PluginConfig::PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& conf const envoy::config::core::v3::Metadata* metadata, bool singleton) : is_singleton_handle_(singleton) { + ASSERT_IS_MAIN_OR_TEST_THREAD(); + plugin_ = std::make_shared<Plugin>(config, direction, context.localInfo(), metadata); auto callback = [this, &context](WasmHandleSharedPtr base_wasm) { @@ -520,6 +522,7 @@ PluginConfig::PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& conf init_manager, context.mainThreadDispatcher(), context.api(), context.lifecycleNotifier(), remote_data_provider_, std::move(callback))) { + // TODO(wbpcode): use absl::Status to return error rather than throw. throw Common::Wasm::WasmException( fmt::format("Unable to create Wasm plugin {}", plugin_->name_)); } From 8ad8cd0022b4eb40e37b0d35823b0b04215da686 Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Wed, 16 Oct 2024 15:52:03 +0800 Subject: [PATCH 07/10] address all comments Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- .../extensions/access_loggers/wasm/config.cc | 2 +- .../wasm/wasm_access_log_impl.h | 2 +- source/extensions/bootstrap/wasm/config.cc | 5 ++- source/extensions/bootstrap/wasm/config.h | 12 +++--- source/extensions/common/wasm/wasm.cc | 41 +++++++++++-------- source/extensions/common/wasm/wasm.h | 14 +++---- .../stat_sinks/wasm/wasm_stat_sink_impl.h | 2 +- .../filters/network/wasm/config_test.cc | 8 ++-- 8 files changed, 46 insertions(+), 40 deletions(-) diff --git a/source/extensions/access_loggers/wasm/config.cc b/source/extensions/access_loggers/wasm/config.cc index e79cf821d43c..30aa8d7cbeba 100644 --- a/source/extensions/access_loggers/wasm/config.cc +++ b/source/extensions/access_loggers/wasm/config.cc @@ -24,7 +24,7 @@ WasmAccessLogFactory::createAccessLogInstance(const Protobuf::Message& proto_con auto plugin_config = std::make_unique<Common::Wasm::PluginConfig>( config.config(), context.serverFactoryContext(), context.scope(), context.initManager(), - envoy::config::core::v3::TrafficDirection::UNSPECIFIED, nullptr /* metadata */, false); + envoy::config::core::v3::TrafficDirection::UNSPECIFIED, /*metadata=*/nullptr, false); auto access_log = std::make_shared<WasmAccessLog>(std::move(plugin_config), std::move(filter)); context.serverFactoryContext().api().customStatNamespaces().registerStatNamespace( diff --git a/source/extensions/access_loggers/wasm/wasm_access_log_impl.h b/source/extensions/access_loggers/wasm/wasm_access_log_impl.h index 2654ed2c5a2f..cca80a75c54a 100644 --- a/source/extensions/access_loggers/wasm/wasm_access_log_impl.h +++ b/source/extensions/access_loggers/wasm/wasm_access_log_impl.h @@ -27,7 +27,7 @@ class WasmAccessLog : public AccessLog::Instance { } } - if (Common::Wasm::Wasm* wasm = plugin_config_->wasmOfHandle(); wasm != nullptr) { + if (Common::Wasm::Wasm* wasm = plugin_config_->wasm(); wasm != nullptr) { wasm->log(plugin_config_->plugin(), log_context, stream_info); } } diff --git a/source/extensions/bootstrap/wasm/config.cc b/source/extensions/bootstrap/wasm/config.cc index 218cbecb7739..e78023eff620 100644 --- a/source/extensions/bootstrap/wasm/config.cc +++ b/source/extensions/bootstrap/wasm/config.cc @@ -16,9 +16,10 @@ namespace Wasm { void WasmServiceExtension::onServerInitialized() { createWasm(context_); } void WasmServiceExtension::createWasm(Server::Configuration::ServerFactoryContext& context) { - wasm_service_ = std::make_unique<Common::Wasm::PluginConfig>( + plugin_config_ = std::make_unique<Common::Wasm::PluginConfig>( config_.config(), context, context.scope(), context.initManager(), - envoy::config::core::v3::TrafficDirection::UNSPECIFIED, nullptr, config_.singleton()); + envoy::config::core::v3::TrafficDirection::UNSPECIFIED, /*metadata=*/nullptr, + config_.singleton()); } Server::BootstrapExtensionPtr diff --git a/source/extensions/bootstrap/wasm/config.h b/source/extensions/bootstrap/wasm/config.h index ca86e1208323..e9c91690e9df 100644 --- a/source/extensions/bootstrap/wasm/config.h +++ b/source/extensions/bootstrap/wasm/config.h @@ -16,11 +16,11 @@ namespace Extensions { namespace Bootstrap { namespace Wasm { +using Common::Wasm::PluginConfig; +using Common::Wasm::PluginConfigPtr; using Common::Wasm::PluginHandleSharedPtrThreadLocal; using Envoy::Extensions::Common::Wasm::PluginHandleSharedPtr; using Envoy::Extensions::Common::Wasm::PluginSharedPtr; -using WasmService = Common::Wasm::PluginConfig; -using WasmServicePtr = std::unique_ptr<WasmService>; class WasmFactory : public Server::Configuration::BootstrapExtensionFactory { public: @@ -39,9 +39,9 @@ class WasmServiceExtension : public Server::BootstrapExtension, Logger::Loggable WasmServiceExtension(const envoy::extensions::wasm::v3::WasmService& config, Server::Configuration::ServerFactoryContext& context) : config_(config), context_(context) {} - WasmService& wasmService() { - ASSERT(wasm_service_ != nullptr); - return *wasm_service_; + PluginConfig& wasmService() { + ASSERT(plugin_config_ != nullptr); + return *plugin_config_; } void onServerInitialized() override; @@ -50,7 +50,7 @@ class WasmServiceExtension : public Server::BootstrapExtension, Logger::Loggable envoy::extensions::wasm::v3::WasmService config_; Server::Configuration::ServerFactoryContext& context_; - WasmServicePtr wasm_service_; + PluginConfigPtr plugin_config_; }; } // namespace Wasm diff --git a/source/extensions/common/wasm/wasm.cc b/source/extensions/common/wasm/wasm.cc index 54279adcacb5..79092fdc5599 100644 --- a/source/extensions/common/wasm/wasm.cc +++ b/source/extensions/common/wasm/wasm.cc @@ -497,25 +497,24 @@ PluginConfig::PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& conf plugin_ = std::make_shared<Plugin>(config, direction, context.localInfo(), metadata); auto callback = [this, &context](WasmHandleSharedPtr base_wasm) { - plugin_handle_initialized_ = true; - if (base_wasm == nullptr) { ENVOY_LOG(critical, "Plugin {} failed to load", plugin_->name_); } if (is_singleton_handle_) { - singleton_handle_ = + plugin_handle_ = getOrCreateThreadLocalPlugin(base_wasm, plugin_, context.mainThreadDispatcher()); return; } - thread_local_handle_ = + auto thread_local_handle = ThreadLocal::TypedSlot<Common::Wasm::PluginHandleSharedPtrThreadLocal>::makeUnique( context.threadLocal()); // NB: the Slot set() call doesn't complete inline, so all arguments must outlive this call. - thread_local_handle_->set([base_wasm, plugin = this->plugin_](Event::Dispatcher& dispatcher) { + thread_local_handle->set([base_wasm, plugin = this->plugin_](Event::Dispatcher& dispatcher) { return std::make_shared<PluginHandleSharedPtrThreadLocal>( getOrCreateThreadLocalPlugin(base_wasm, plugin, dispatcher)); }); + plugin_handle_ = std::move(thread_local_handle); }; if (!Common::Wasm::createWasm(plugin_, scope.createScope(""), context.clusterManager(), @@ -528,8 +527,11 @@ PluginConfig::PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& conf } } +// Simple helper function to get the Wasm* from a WasmHandle. +Wasm* wasmHandleWasm(WasmHandleSharedPtr& h) { return h != nullptr ? h->wasm().get() : nullptr; } + std::shared_ptr<Context> PluginConfig::createContext() { - if (!plugin_handle_initialized_) { + if (absl::holds_alternative<absl::monostate>(plugin_handle_)) { return nullptr; } @@ -539,10 +541,13 @@ std::shared_ptr<Context> PluginConfig::createContext() { return nullptr; } - if (!thread_local_handle_->currentThreadRegistered()) { + ASSERT(absl::holds_alternative<ThreadLocalPluginHandle>(plugin_handle_)); + auto thread_local_handle = absl::get<ThreadLocalPluginHandle>(plugin_handle_).get(); + + if (!thread_local_handle->currentThreadRegistered()) { return nullptr; } - auto plugin_holder = thread_local_handle_->get(); + auto plugin_holder = thread_local_handle->get(); if (!plugin_holder.has_value()) { return nullptr; } @@ -551,7 +556,7 @@ std::shared_ptr<Context> PluginConfig::createContext() { return nullptr; } - Wasm* wasm = plugin_holder->handle->wasmOfHandle(); + Wasm* wasm = wasmHandleWasm(plugin_holder->handle->wasmHandle()); if (!wasm || wasm->isFailed()) { if (plugin_->fail_open_) { @@ -566,20 +571,24 @@ std::shared_ptr<Context> PluginConfig::createContext() { plugin_holder->handle); } -Wasm* PluginConfig::wasmOfHandle() { - if (!plugin_handle_initialized_) { +Wasm* PluginConfig::wasm() { + if (absl::holds_alternative<absl::monostate>(plugin_handle_)) { return nullptr; } if (is_singleton_handle_) { - return singleton_handle_ != nullptr ? singleton_handle_->wasmOfHandle() : nullptr; + ASSERT(absl::holds_alternative<SinglePluginHandle>(plugin_handle_)); + PluginHandleSharedPtr singleton_handle = absl::get<SinglePluginHandle>(plugin_handle_); + return singleton_handle != nullptr ? wasmHandleWasm(singleton_handle->wasmHandle()) : nullptr; } - ASSERT(thread_local_handle_ != nullptr); - if (!thread_local_handle_->currentThreadRegistered()) { + ASSERT(absl::holds_alternative<ThreadLocalPluginHandle>(plugin_handle_)); + auto thread_local_handle = absl::get<ThreadLocalPluginHandle>(plugin_handle_).get(); + + if (!thread_local_handle->currentThreadRegistered()) { return nullptr; } - auto plugin_holder = thread_local_handle_->get(); + auto plugin_holder = thread_local_handle->get(); if (!plugin_holder.has_value()) { return nullptr; } @@ -588,7 +597,7 @@ Wasm* PluginConfig::wasmOfHandle() { return nullptr; } - return plugin_holder->handle->wasmOfHandle(); + return wasmHandleWasm(plugin_holder->handle->wasmHandle()); } } // namespace Wasm diff --git a/source/extensions/common/wasm/wasm.h b/source/extensions/common/wasm/wasm.h index 148bc3a61a0f..73e359047b94 100644 --- a/source/extensions/common/wasm/wasm.h +++ b/source/extensions/common/wasm/wasm.h @@ -143,7 +143,6 @@ class PluginHandle : public PluginHandleBase { std::static_pointer_cast<PluginBase>(plugin)), plugin_(plugin), wasm_handle_(wasm_handle) {} - Wasm* wasmOfHandle() { return wasm_handle_ != nullptr ? wasm_handle_->wasm().get() : nullptr; } WasmHandleSharedPtr& wasmHandle() { return wasm_handle_; } uint32_t rootContextId() { return wasm_handle_->wasm()->getRootContext(plugin_, false)->id(); } @@ -190,21 +189,18 @@ class PluginConfig : Logger::Loggable<Logger::Id::wasm> { const envoy::config::core::v3::Metadata* metadata, bool singleton); std::shared_ptr<Context> createContext(); - Wasm* wasmOfHandle(); + Wasm* wasm(); const PluginSharedPtr& plugin() { return plugin_; } private: + using SinglePluginHandle = PluginHandleSharedPtr; + using ThreadLocalPluginHandle = ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal>; + PluginSharedPtr plugin_; RemoteAsyncDataProviderPtr remote_data_provider_; const bool is_singleton_handle_{}; - bool plugin_handle_initialized_{}; - // Plugin handle that works for all threads. Only one of thread_local_handle_ or - // singleton_handle_ will be set. - ThreadLocal::TypedSlotPtr<PluginHandleSharedPtrThreadLocal> thread_local_handle_; - // Plugin handle that works for the main. Only one of thread_local_handle_ or - // singleton_handle_ will be set. - PluginHandleSharedPtr singleton_handle_; + absl::variant<absl::monostate, SinglePluginHandle, ThreadLocalPluginHandle> plugin_handle_; }; using PluginConfigPtr = std::unique_ptr<PluginConfig>; diff --git a/source/extensions/stat_sinks/wasm/wasm_stat_sink_impl.h b/source/extensions/stat_sinks/wasm/wasm_stat_sink_impl.h index f4539a60926a..ffa372dc997b 100644 --- a/source/extensions/stat_sinks/wasm/wasm_stat_sink_impl.h +++ b/source/extensions/stat_sinks/wasm/wasm_stat_sink_impl.h @@ -21,7 +21,7 @@ class WasmStatSink : public Stats::Sink { : plugin_config_(std::move(plugin_config)) {} void flush(Stats::MetricSnapshot& snapshot) override { - if (Common::Wasm::Wasm* wasm = plugin_config_->wasmOfHandle(); wasm != nullptr) { + if (Common::Wasm::Wasm* wasm = plugin_config_->wasm(); wasm != nullptr) { wasm->onStatsUpdate(plugin_config_->plugin(), snapshot); } } diff --git a/test/extensions/filters/network/wasm/config_test.cc b/test/extensions/filters/network/wasm/config_test.cc index fff53f8819cc..d7a3a2ba5aca 100644 --- a/test/extensions/filters/network/wasm/config_test.cc +++ b/test/extensions/filters/network/wasm/config_test.cc @@ -194,7 +194,7 @@ TEST_P(WasmNetworkFilterConfigTest, FilterConfigFailClosed) { envoy::extensions::filters::network::wasm::v3::Wasm proto_config; TestUtility::loadFromYaml(yaml, proto_config); NetworkFilters::Wasm::FilterConfig filter_config(proto_config, context_); - filter_config.wasmOfHandle()->fail(proxy_wasm::FailState::RuntimeError, ""); + filter_config.wasm()->fail(proxy_wasm::FailState::RuntimeError, ""); auto context = filter_config.createContext(); EXPECT_EQ(context->wasm(), nullptr); EXPECT_TRUE(context->isFailed()); @@ -218,7 +218,7 @@ TEST_P(WasmNetworkFilterConfigTest, FilterConfigFailOpen) { envoy::extensions::filters::network::wasm::v3::Wasm proto_config; TestUtility::loadFromYaml(yaml, proto_config); NetworkFilters::Wasm::FilterConfig filter_config(proto_config, context_); - filter_config.wasmOfHandle()->fail(proxy_wasm::FailState::RuntimeError, ""); + filter_config.wasm()->fail(proxy_wasm::FailState::RuntimeError, ""); EXPECT_EQ(filter_config.createContext(), nullptr); } @@ -241,7 +241,7 @@ TEST_P(WasmNetworkFilterConfigTest, FilterConfigCapabilitiesUnrestrictedByDefaul envoy::extensions::filters::network::wasm::v3::Wasm proto_config; TestUtility::loadFromYaml(yaml, proto_config); NetworkFilters::Wasm::FilterConfig filter_config(proto_config, context_); - auto wasm = filter_config.wasmOfHandle(); + auto wasm = filter_config.wasm(); EXPECT_TRUE(wasm->capabilityAllowed("proxy_log")); EXPECT_TRUE(wasm->capabilityAllowed("proxy_on_vm_start")); EXPECT_TRUE(wasm->capabilityAllowed("proxy_http_call")); @@ -270,7 +270,7 @@ TEST_P(WasmNetworkFilterConfigTest, FilterConfigCapabilityRestriction) { envoy::extensions::filters::network::wasm::v3::Wasm proto_config; TestUtility::loadFromYaml(yaml, proto_config); NetworkFilters::Wasm::FilterConfig filter_config(proto_config, context_); - auto wasm = filter_config.wasmOfHandle(); + auto wasm = filter_config.wasm(); EXPECT_TRUE(wasm->capabilityAllowed("proxy_log")); EXPECT_TRUE(wasm->capabilityAllowed("proxy_on_new_connection")); EXPECT_FALSE(wasm->capabilityAllowed("proxy_http_call")); From 80f9eefaf2fb1c1b4a4ac80c80c9190cd193eb3a Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Wed, 16 Oct 2024 15:59:54 +0800 Subject: [PATCH 08/10] add todo Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- source/extensions/common/wasm/wasm.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/extensions/common/wasm/wasm.h b/source/extensions/common/wasm/wasm.h index 73e359047b94..01d630009370 100644 --- a/source/extensions/common/wasm/wasm.h +++ b/source/extensions/common/wasm/wasm.h @@ -183,6 +183,9 @@ WasmEvent toWasmEvent(const std::shared_ptr<WasmHandleBase>& wasm); class PluginConfig : Logger::Loggable<Logger::Id::wasm> { public: + // TODO(wbpcode): the code of PluginConfig will be shared cross all Wasm extensions (loggers, + // http filters, etc.), we may extend the constructor to takes a static string view to tell + // the type of the plugin if needed. PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& config, Server::Configuration::ServerFactoryContext& context, Stats::Scope& scope, Init::Manager& init_manager, envoy::config::core::v3::TrafficDirection direction, From f89926edd2ffd29aafcdce8ab878fa6981f58fa2 Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Wed, 16 Oct 2024 16:17:04 +0800 Subject: [PATCH 09/10] remove unneeded type name Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- test/extensions/bootstrap/wasm/config_test.cc | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/extensions/bootstrap/wasm/config_test.cc b/test/extensions/bootstrap/wasm/config_test.cc index d02444bffdb4..d05cabd95d9f 100644 --- a/test/extensions/bootstrap/wasm/config_test.cc +++ b/test/extensions/bootstrap/wasm/config_test.cc @@ -19,8 +19,6 @@ namespace Envoy { namespace Extensions { namespace Wasm { -using Extensions::Bootstrap::Wasm::WasmServicePtr; - class WasmFactoryTest : public testing::TestWithParam<std::tuple<std::string, std::string>> { protected: WasmFactoryTest() { From 0bbd11da437237e5d0110a2a1baf663d2bb2af2b Mon Sep 17 00:00:00 2001 From: wangbaiping <wangbaiping@bytedance.com> Date: Wed, 16 Oct 2024 18:44:06 +0800 Subject: [PATCH 10/10] improve coverage Signed-off-by: wangbaiping <wangbaiping@bytedance.com> --- test/extensions/common/wasm/wasm_vm_test.cc | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/extensions/common/wasm/wasm_vm_test.cc b/test/extensions/common/wasm/wasm_vm_test.cc index 7046ad9e3309..5eb298f930cb 100644 --- a/test/extensions/common/wasm/wasm_vm_test.cc +++ b/test/extensions/common/wasm/wasm_vm_test.cc @@ -27,6 +27,17 @@ namespace Common { namespace Wasm { namespace { +TEST(EnvoyWasmVmIntegrationTest, EnvoyWasmVmIntegrationTest) { + { + EnvoyWasmVmIntegration wasm_vm_integration; + for (const auto l : {spdlog::level::trace, spdlog::level::debug, spdlog::level::info, + spdlog::level::warn, spdlog::level::err, spdlog::level::critical}) { + Logger::Registry::getLog(Logger::Id::wasm).set_level(l); + EXPECT_EQ(wasm_vm_integration.getLogLevel(), static_cast<proxy_wasm::LogLevel>(l)); + } + } +} + class TestNullVmPlugin : public proxy_wasm::NullVmPlugin { public: TestNullVmPlugin() = default;