wasm: fix multiple bug of vm key and plugin key calculation

api/envoy/extensions/wasm/v3/wasm.proto

@@ -71,7 +71,7 @@ message SanitizationConfig {
 }
 
 // Configuration for a Wasm VM.
-// [#next-free-field: 8]
+// [#next-free-field: 9]
 message VmConfig {
   // An ID which will be used along with a hash of the wasm code (or the name of the registered Null
   // VM plugin) to determine which VM will be used for the plugin. All plugins which use the same
@@ -137,6 +137,12 @@ message VmConfig {
   // .. warning::
   //   Envoy rejects the configuration if there's conflict of key space.
   EnvironmentVariables environment_variables = 7;
+
+  // Configuration for restricting Proxy-Wasm capabilities available to modules.
+  //
+  // The restrictions are applied when the VM is created and are shared by every plugin running in
+  // that VM, so they are a property of the VM rather than of an individual plugin.
+  CapabilityRestrictionConfig capability_restriction_config = 8;
 }
 
 message EnvironmentVariables {
@@ -192,7 +198,14 @@ message PluginConfig {
   ReloadConfig reload_config = 8;
 
   // Configuration for restricting Proxy-Wasm capabilities available to modules.
-  CapabilityRestrictionConfig capability_restriction_config = 6;
+  //
+  // This field is deprecated in favor of the :ref:`vm_config.capability_restriction_config
+  // <envoy_v3_api_field_extensions.wasm.v3.VmConfig.capability_restriction_config>` field, because
+  // the restrictions are applied to the Wasm VM and are therefore shared by every plugin running in
+  // it. If this field is set and ``vm_config.capability_restriction_config`` is not, this field is
+  // used to populate it.
+  CapabilityRestrictionConfig capability_restriction_config = 6
+      [deprecated = true, (envoy.annotations.deprecated_at_minor_version) = "3.0"];
 
   // Whether or not to allow plugin onRequestHeaders and onResponseHeaders callbacks to return
   // FilterHeadersStatus::StopIteration.

changelogs/current/bug_fixes/wasm__vm-key-derived-from-vm-config.rst

@@ -0,0 +1,7 @@
+Fixed a bug where Wasm plugins whose :ref:`VM configurations
+<envoy_v3_api_msg_extensions.wasm.v3.VmConfig>` differed could still share a single Wasm VM, and so
+silently run with the VM configuration of whichever plugin happened to be configured first. The
+:ref:`runtime <envoy_v3_api_field_extensions.wasm.v3.VmConfig.runtime>` and the :ref:`capability
+restrictions <envoy_v3_api_field_extensions.wasm.v3.VmConfig.capability_restriction_config>` are now
+part of the VM identity, alongside the ``vm_id``, the ``configuration``, the ``code`` and the
+``environment_variables``, so plugins differing in either of them no longer share a VM.

changelogs/current/deprecated/wasm__plugin-level-capability-restriction.rst

@@ -0,0 +1,7 @@
+The :ref:`PluginConfig.capability_restriction_config
+<envoy_v3_api_field_extensions.wasm.v3.PluginConfig.capability_restriction_config>` field is
+deprecated in favor of the new :ref:`VmConfig.capability_restriction_config
+<envoy_v3_api_field_extensions.wasm.v3.VmConfig.capability_restriction_config>` field. The
+restrictions are applied when the Wasm VM is created and are shared by every plugin running in that
+VM, so they are a property of the VM rather than of an individual plugin. The deprecated field keeps
+working: when it is set and the VM level field is not, it is used to populate the VM level one.

changelogs/current/minor_behavior_changes/wasm__plugin-key-derived-from-plugin-config.rst

@@ -0,0 +1,7 @@
+The identity of a Wasm plugin (which plugin configurations share a single root context and
+thread-local plugin instance inside a Wasm VM) is now derived from the whole
+:ref:`plugin configuration <envoy_v3_api_msg_extensions.wasm.v3.PluginConfig>` instead of from the
+plugin name and the traffic direction of the listener the plugin was configured on. Configurations
+that differ in any field other than :ref:`vm_config
+<envoy_v3_api_field_extensions.wasm.v3.PluginConfig.vm_config>` no longer share an instance, and
+identical configurations now share one regardless of the traffic direction they are configured on.

contrib/stat_sinks/wasm_filter/source/config.cc

@@ -29,8 +29,7 @@ WasmFilterSinkFactory::createStatsSink(const Protobuf::Message& proto_config,
   setGlobalTags(&startup_tags);
 
   auto plugin_config = std::make_unique<Common::Wasm::PluginConfig>(
-      config.wasm_config(), context, context.scope(), context.initManager(),
-      envoy::config::core::v3::TrafficDirection::UNSPECIFIED, true);
+      config.wasm_config(), context, context.scope(), context.initManager(), true);
 
   setGlobalTags(nullptr);
 

source/extensions/access_loggers/wasm/config.cc

@@ -23,9 +23,9 @@ WasmAccessLogFactory::createAccessLogInstance(const Protobuf::Message& proto_con
       const envoy::extensions::access_loggers::wasm::v3::WasmAccessLog&>(
       proto_config, context.messageValidationVisitor());
 
-  auto plugin_config = std::make_unique<Common::Wasm::PluginConfig>(
-      config.config(), context.serverFactoryContext(), context.scope(), context.initManager(),
-      envoy::config::core::v3::TrafficDirection::UNSPECIFIED, false);
+  auto plugin_config =
+      std::make_unique<Common::Wasm::PluginConfig>(config.config(), context.serverFactoryContext(),
+                                                   context.scope(), context.initManager(), false);
   auto access_log = std::make_shared<WasmAccessLog>(std::move(plugin_config), std::move(filter));
 
   context.serverFactoryContext().api().customStatNamespaces().registerStatNamespace(

source/extensions/bootstrap/wasm/config.cc

@@ -17,8 +17,7 @@ void WasmServiceExtension::onServerInitialized(Server::Instance&) { createWasm(c
 
 void WasmServiceExtension::createWasm(Server::Configuration::ServerFactoryContext& context) {
   plugin_config_ = std::make_unique<Common::Wasm::PluginConfig>(
-      config_.config(), context, context.scope(), context.initManager(),
-      envoy::config::core::v3::TrafficDirection::UNSPECIFIED, config_.singleton());
+      config_.config(), context, context.scope(), context.initManager(), config_.singleton());
 }
 
 Server::BootstrapExtensionPtr

source/extensions/common/wasm/BUILD

@@ -47,7 +47,6 @@ envoy_cc_library(
         "//source/common/version:version_includes",
         "//source/extensions/filters/common/expr:cel_state_lib",
         "//source/extensions/filters/common/expr:evaluator_lib",
-        "@envoy_api//envoy/config/core/v3:pkg_cc_proto",
         "@envoy_api//envoy/extensions/wasm/v3:pkg_cc_proto",
         "@proxy-wasm-cpp-host//:headers",
         "@proxy-wasm-cpp-sdk//:common_lib",

source/extensions/common/wasm/plugin.cc

@@ -2,15 +2,37 @@
 
 #include "envoy/common/exception.h"
 
+#include "absl/strings/str_cat.h"
 #include "include/proxy-wasm/wasm.h"
 
 namespace Envoy {
 namespace Extensions {
 namespace Common {
 namespace Wasm {
 
-WasmConfig::WasmConfig(const envoy::extensions::wasm::v3::PluginConfig& config) : config_(config) {
-  for (auto& capability : config_.capability_restriction_config().allowed_capabilities()) {
+envoy::extensions::wasm::v3::PluginConfig
+normalizeConfig(const envoy::extensions::wasm::v3::PluginConfig& config) {
+  // The capability restrictions are applied when the Wasm VM is created and are shared by every
+  // plugin running in it, so they belong to the VM configuration. The plugin level field is
+  // deprecated in favor of the VM level one: move it there and clear it, so that the VM level field
+  // is the only place the restrictions are ever read from.
+  if (!config.has_capability_restriction_config()) {
+    return config;
+  }
+  envoy::extensions::wasm::v3::PluginConfig normalized = config;
+  // The VM level restrictions win when both are set.
+  if (!config.vm_config().has_capability_restriction_config()) {
+    *normalized.mutable_vm_config()->mutable_capability_restriction_config() =
+        config.capability_restriction_config();
+  }
+  normalized.clear_capability_restriction_config();
+  return normalized;
+}
+
+WasmConfig::WasmConfig(const envoy::extensions::wasm::v3::PluginConfig& config)
+    : config_(normalizeConfig(config)) {
+  for (auto& capability :
+       config_.vm_config().capability_restriction_config().allowed_capabilities()) {
     // TODO(rapilado): Set the SanitizationConfig fields once sanitization is implemented.
     allowed_capabilities_[capability.first] = proxy_wasm::SanitizationConfig();
   }
@@ -54,6 +76,23 @@ WasmConfig::WasmConfig(const envoy::extensions::wasm::v3::PluginConfig& config)
   }
 }
 
+std::string Plugin::createPluginKey(const envoy::extensions::wasm::v3::PluginConfig& config) {
+  // Every field of the plugin configuration is part of the plugin identity, so that distinct
+  // configurations never share an instance and identical configurations always do (which keeps the
+  // plugin reusable across xDS updates and bounds the number of root contexts). `vm_config` is
+  // excluded because two plugins can only share a root context when they share a VM, and VM
+  // identity is the VM key that proxy-wasm prepends to this key when caching thread-local plugins.
+  // That key covers the fields of `vm_config` the VM is built from (see the `makeVmKey()` call in
+  // wasm.cc), so between the two keys the only fields left out are the ones that affect neither the
+  // VM nor the plugin, namely `allow_precompiled` and `nack_on_code_cache_miss`.
+  //
+  // The copy below only happens when a plugin is configured, and the plugin configuration is deep
+  // copied by WasmConfig anyway.
+  envoy::extensions::wasm::v3::PluginConfig key_config = config;
+  key_config.clear_vm_config();
+  return absl::StrCat(config.name(), "||", MessageUtil::hash(key_config));
+}
+
 } // namespace Wasm
 } // namespace Common
 } // namespace Extensions

source/extensions/common/wasm/plugin.h

@@ -2,7 +2,6 @@
 
 #include <memory>
 
-#include "envoy/config/core/v3/base.pb.h"
 #include "envoy/extensions/wasm/v3/wasm.pb.validate.h"
 #include "envoy/local_info/local_info.h"
 
@@ -39,23 +38,19 @@ using WasmConfigPtr = std::unique_ptr<WasmConfig>;
 class Plugin : public proxy_wasm::PluginBase {
 public:
   Plugin(const envoy::extensions::wasm::v3::PluginConfig& config,
-         envoy::config::core::v3::TrafficDirection direction,
          const LocalInfo::LocalInfo& local_info)
       : PluginBase(
             config.name(), config.root_id(), config.vm_config().vm_id(),
             config.vm_config().runtime(),
             THROW_OR_RETURN_VALUE(MessageUtil::anyToBytes(config.configuration()), std::string),
-            config.fail_open(), createPluginKey(config, direction)),
+            config.fail_open(), createPluginKey(config)),
         local_info_(local_info), wasm_config_(std::make_unique<WasmConfig>(config)) {}
 
   const LocalInfo::LocalInfo& localInfo() { return local_info_; }
   WasmConfig& wasmConfig() { return *wasm_config_; }
 
 private:
-  static std::string createPluginKey(const envoy::extensions::wasm::v3::PluginConfig& config,
-                                     envoy::config::core::v3::TrafficDirection direction) {
-    return config.name() + "||" + envoy::config::core::v3::TrafficDirection_Name(direction);
-  }
+  static std::string createPluginKey(const envoy::extensions::wasm::v3::PluginConfig& config);
 
 private:
   const LocalInfo::LocalInfo& local_info_;

source/extensions/common/wasm/wasm.cc

@@ -374,13 +374,19 @@ bool createWasm(const PluginSharedPtr& plugin, const Stats::ScopeSharedPtr& scop
                  .value_or(code.empty() ? EMPTY_STRING : INLINE_STRING);
   }
 
-  // Include environment_variables in the vm_key so that a change to env vars triggers VM
-  // recreation, the same way a code change does. The env vars hash is appended to vm_id (which is
-  // small) rather than to code (which can be O(MB)), with a separator to avoid key collisions.
-  const std::size_t env_vars_hash = MessageUtil::hash(vm_config.environment_variables());
-  const std::string vm_id_with_env = absl::StrCat(vm_config.vm_id(), "|", env_vars_hash);
+  // Ideally, all fields of vm_config that affect the Wasm VM should be part of the vm_key, but
+  // the proxy_wasm::makeVmKey() takes the configuration and the code as separate arguments, so
+  // we only include the other fields here. If any other fields are added to vm_config that affect
+  // the Wasm VM, they should be added here.
+  envoy::extensions::wasm::v3::VmConfig vm_key_config;
+  vm_key_config.set_runtime(vm_config.runtime());
+  *vm_key_config.mutable_environment_variables() = vm_config.environment_variables();
+  *vm_key_config.mutable_capability_restriction_config() =
+      vm_config.capability_restriction_config();
+  const std::string vm_id_with_config =
+      absl::StrCat(vm_config.vm_id(), "|", MessageUtil::hash(vm_key_config));
   auto vm_key = proxy_wasm::makeVmKey(
-      vm_id_with_env,
+      vm_id_with_config,
       THROW_OR_RETURN_VALUE(MessageUtil::anyToBytes(vm_config.configuration()), std::string), code);
   auto complete_cb = [cb, vm_key, plugin, scope, &api, &cluster_manager, &dispatcher,
                       &lifecycle_notifier, create_root_context_for_testing,
@@ -567,8 +573,7 @@ std::pair<OptRef<PluginConfig::SinglePluginHandle>, Wasm*> PluginConfig::getPlug
 
 PluginConfig::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, bool singleton)
+                           Stats::Scope& scope, Init::Manager& init_manager, bool singleton)
     : is_singleton_handle_(singleton) {
 
   if (config.fail_open()) {
@@ -612,7 +617,7 @@ PluginConfig::PluginConfig(const envoy::extensions::wasm::v3::PluginConfig& conf
   }
 
   stats_handler_ = std::make_shared<StatsHandler>(scope, absl::StrCat("wasm.", config.name(), "."));
-  plugin_ = std::make_shared<Plugin>(config, direction, context.localInfo());
+  plugin_ = std::make_shared<Plugin>(config, context.localInfo());
 
   auto callback = [this, &context](WasmHandleSharedPtr base_wasm) {
     base_wasm_ = base_wasm;

source/extensions/common/wasm/wasm.h

@@ -191,8 +191,7 @@ class PluginConfig : Logger::Loggable<Logger::Id::wasm> {
   // 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,
-               bool singleton);
+               Init::Manager& init_manager, bool singleton);
 
   std::shared_ptr<Context> createContext();
   Wasm* wasm();

source/extensions/filters/http/wasm/config.h

@@ -18,51 +18,19 @@ namespace Wasm {
  * Config registration for the Wasm filter. @see NamedHttpFilterConfigFactory.
  */
 class WasmFilterConfig
-    : public Common::CommonFactoryBase<envoy::extensions::filters::http::wasm::v3::Wasm>,
-      public Server::Configuration::NamedHttpFilterConfigFactory,
-      public Server::Configuration::UpstreamHttpFilterConfigFactory {
+    : public Common::UnifiedFactoryBase<envoy::extensions::filters::http::wasm::v3::Wasm> {
 public:
   WasmFilterConfig()
-      : Common::CommonFactoryBase<envoy::extensions::filters::http::wasm::v3::Wasm>(
+      : Common::UnifiedFactoryBase<envoy::extensions::filters::http::wasm::v3::Wasm>(
             "envoy.filters.http.wasm") {}
 
-  absl::StatusOr<Envoy::Http::FilterFactoryCb>
-  createFilterFactoryFromProto(const Protobuf::Message& proto_config,
-                               const std::string& stats_prefix,
-                               Server::Configuration::FactoryContext& context) override {
-    return createFilterFactoryFromProtoTyped(
-        MessageUtil::downcastAndValidate<const envoy::extensions::filters::http::wasm::v3::Wasm&>(
-            proto_config, context.messageValidationVisitor()),
-        stats_prefix, context, context.serverFactoryContext());
-  }
-
-  absl::StatusOr<Envoy::Http::FilterFactoryCb>
-  createFilterFactoryFromProto(const Protobuf::Message& proto_config,
-                               const std::string& stats_prefix,
-                               Server::Configuration::UpstreamFactoryContext& context) override {
-    return createFilterFactoryFromProtoTyped(
-        MessageUtil::downcastAndValidate<const envoy::extensions::filters::http::wasm::v3::Wasm&&>(
-            proto_config, context.serverFactoryContext().messageValidationVisitor()),
-        stats_prefix, context, context.serverFactoryContext());
-  }
-
-  absl::StatusOr<Envoy::Http::FilterFactoryCb> createHttpFilterFactoryFromProto(
-      const Protobuf::Message& proto_config, Server::Configuration::ServerFactoryContext& context,
+  absl::StatusOr<Envoy::Http::FilterFactoryCb> createHttpFilterFactoryFromProtoTyped(
+      const envoy::extensions::filters::http::wasm::v3::Wasm& proto_config,
+      Server::Configuration::ServerFactoryContext& context,
       Server::Configuration::ExtraFactoryContext& extra_context) override {
-    return createFilterFactoryFromProtoTyped(
-        MessageUtil::downcastAndValidate<const envoy::extensions::filters::http::wasm::v3::Wasm&>(
-            proto_config, context.messageValidationVisitor()),
-        extra_context.stats_prefix, context, context);
-  }
-
-private:
-  template <class FactoryContext>
-  Http::FilterFactoryCb createFilterFactoryFromProtoTyped(
-      const envoy::extensions::filters::http::wasm::v3::Wasm& proto_config, const std::string&,
-      FactoryContext& context, Server::Configuration::ServerFactoryContext& server_context) {
-    server_context.api().customStatNamespaces().registerStatNamespace(
+    context.api().customStatNamespaces().registerStatNamespace(
         Extensions::Common::Wasm::CustomStatNamespace);
-    auto filter_config = std::make_shared<FilterConfig>(proto_config, context);
+    auto filter_config = std::make_shared<FilterConfig>(proto_config, context, extra_context);
     return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void {
       auto filter = filter_config->createContext();
       if (!filter) { // Fail open