Skip to content

wasm: fix root_context_id that was incorrectly cached across threads. #15016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
lizan merged 8 commits into from
Feb 17, 2021
Merged

wasm: fix root_context_id that was incorrectly cached across threads. #15016

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b34b064
wasm: fix config_id that was incorrectly cached across threads.
PiotrSikora Feb 10, 2021
66f8446
review: fix format.
PiotrSikora Feb 10, 2021
6daf09d
review: update tests.
PiotrSikora Feb 10, 2021
2dd0b04
review: separate code path for fail closed.
PiotrSikora Feb 10, 2021
3053236
review: fix style.
PiotrSikora Feb 11, 2021
03cf80c
review: improve coverage.
PiotrSikora Feb 11, 2021
76bff7a
review: use merged commit.
PiotrSikora Feb 11, 2021
b49a1a9
review: rename config_id back to root_context_id.
PiotrSikora Feb 11, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions bazel/repository_locations.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -907,8 +907,8 @@ REPOSITORY_LOCATIONS_SPEC = dict(
project_name = "WebAssembly for Proxies (C++ host implementation)",
project_desc = "WebAssembly for Proxies (C++ host implementation)",
project_url = "https://github.com/proxy-wasm/proxy-wasm-cpp-host",
version = "5a53cf4b231599e1d2a1f2f4598fdfbb727ff948",
sha256 = "600dbc651a2837e6f1db964eb7e1078e5e338049a34c9ab47415dfa7f3de5478",
version = "c51fbca35e9e7968fc5319258ed7a38b1bc1ec7a",
sha256 = "533944a9084c2f75c36bda627152b9f31047ff3554b6361a88a542f16dee9483",
strip_prefix = "proxy-wasm-cpp-host-{version}",
urls = ["https://github.com/proxy-wasm/proxy-wasm-cpp-host/archive/{version}.tar.gz"],
use_category = ["dataplane_ext"],
Expand All @@ -923,7 +923,7 @@ REPOSITORY_LOCATIONS_SPEC = dict(
"envoy.wasm.runtime.wavm",
"envoy.wasm.runtime.wasmtime",
],
release_date = "2021-01-12",
release_date = "2021-02-11",
cpe = "N/A",
),
proxy_wasm_rust_sdk = dict(
Expand Down
1 change: 1 addition & 0 deletions source/extensions/common/wasm/context.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ using GrpcService = envoy::config::core::v3::GrpcService;

class Wasm;

using PluginBaseSharedPtr = std::shared_ptr<PluginBase>;
using PluginHandleBaseSharedPtr = std::shared_ptr<PluginHandleBase>;
using WasmHandleBaseSharedPtr = std::shared_ptr<WasmHandleBase>;

Expand Down
10 changes: 6 additions & 4 deletions source/extensions/common/wasm/wasm.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,10 +282,12 @@ getCloneFactory(WasmExtension* wasm_extension, Event::Dispatcher& dispatcher,

static proxy_wasm::PluginHandleFactory getPluginFactory(WasmExtension* wasm_extension) {
auto wasm_plugin_factory = wasm_extension->pluginFactory();
return [wasm_plugin_factory](WasmHandleBaseSharedPtr base_wasm,
absl::string_view plugin_key) -> std::shared_ptr<PluginHandleBase> {
return wasm_plugin_factory(std::static_pointer_cast<WasmHandle>(base_wasm), plugin_key);
};
return
[wasm_plugin_factory](WasmHandleBaseSharedPtr base_wasm,
PluginBaseSharedPtr base_plugin) -> std::shared_ptr<PluginHandleBase> {
return wasm_plugin_factory(std::static_pointer_cast<WasmHandle>(base_wasm),
std::static_pointer_cast<Plugin>(base_plugin));
};
}

WasmEvent toWasmEvent(const std::shared_ptr<WasmHandleBase>& wasm) {
Expand Down
10 changes: 7 additions & 3 deletions source/extensions/common/wasm/wasm.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,15 +142,19 @@ using WasmHandleSharedPtr = std::shared_ptr<WasmHandle>;

class PluginHandle : public PluginHandleBase, public ThreadLocal::ThreadLocalObject {
public:
explicit PluginHandle(const WasmHandleSharedPtr& wasm_handle, absl::string_view plugin_key)
: PluginHandleBase(std::static_pointer_cast<WasmHandleBase>(wasm_handle), plugin_key),
wasm_handle_(wasm_handle) {}
explicit PluginHandle(const WasmHandleSharedPtr& wasm_handle, const PluginSharedPtr& plugin)
: PluginHandleBase(std::static_pointer_cast<WasmHandleBase>(wasm_handle),
std::static_pointer_cast<PluginBase>(plugin)),
wasm_handle_(wasm_handle),
root_context_id_(wasm_handle->wasm()->getRootContext(plugin, false)->id()) {}

WasmSharedPtr& wasm() { return wasm_handle_->wasm(); }
WasmHandleSharedPtr& wasmHandleForTest() { return wasm_handle_; }
uint32_t rootContextId() { return root_context_id_; }

private:
WasmHandleSharedPtr wasm_handle_;
const uint32_t root_context_id_;
};

using PluginHandleSharedPtr = std::shared_ptr<PluginHandle>;
Expand Down
6 changes: 3 additions & 3 deletions source/extensions/common/wasm/wasm_extension.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,10 +40,10 @@ RegisterWasmExtension::RegisterWasmExtension(WasmExtension* extension) {
}

PluginHandleExtensionFactory EnvoyWasm::pluginFactory() {
return [](const WasmHandleSharedPtr& base_wasm,
absl::string_view plugin_key) -> PluginHandleBaseSharedPtr {
return [](const WasmHandleSharedPtr& wasm_handle,
const PluginSharedPtr& plugin) -> PluginHandleBaseSharedPtr {
return std::static_pointer_cast<PluginHandleBase>(
std::make_shared<PluginHandle>(base_wasm, plugin_key));
std::make_shared<PluginHandle>(wasm_handle, plugin));
};
}

Expand Down
2 changes: 1 addition & 1 deletion source/extensions/common/wasm/wasm_extension.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ using WasmHandleSharedPtr = std::shared_ptr<WasmHandle>;
using CreateContextFn =
std::function<ContextBase*(Wasm* wasm, const std::shared_ptr<Plugin>& plugin)>;
using PluginHandleExtensionFactory = std::function<PluginHandleBaseSharedPtr(
const WasmHandleSharedPtr& base_wasm, absl::string_view plugin_key)>;
const WasmHandleSharedPtr& wasm_handle, const PluginSharedPtr& plugin)>;
using WasmHandleExtensionFactory = std::function<WasmHandleBaseSharedPtr(
const VmConfig& vm_config, const CapabilityRestrictionConfig& capability_restriction_config,
const Stats::ScopeSharedPtr& scope, Upstream::ClusterManager& cluster_manager,
Expand Down
16 changes: 9 additions & 7 deletions source/extensions/filters/http/wasm/wasm_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,19 @@ class FilterConfig : Logger::Loggable<Logger::Id::wasm> {
if (handle.has_value()) {
wasm = handle->wasm().get();
}
if (plugin_->fail_open_ && (!wasm || wasm->isFailed())) {
return nullptr;
if (!wasm || wasm->isFailed()) {
Copy link
Member

@mathetake mathetake Feb 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: is this change something to do with #14947?

Copy link
Contributor Author

@PiotrSikora PiotrSikora Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, there are functional changes here, only style.

Previously, we had:

if (plugin_->fail_open_ && (!wasm || wasm->isFailed())) {
  return nullptr;
}

which handled the fail open case, and

return std::make_shared<Context>(wasm, root_context_id_, plugin_);

which handled both the valid case, and the fail closed case (where wasm == nullptr and the rest didn't matter).

I've split the latter to make it more readable, since the new version is:

return std::make_shared<Context>(wasm, handle->configId(), plugin_);

and handle isn't valid in the fail closed case.

Alternatively, I could undo the style changes and turn this into:

return std::make_shared<Context>(wasm, handle.has_value() ? handle->configId() : 0, plugin_);

but that seems more confusing than having separate codepath.

Let me know what you prefer.

if (plugin_->fail_open_) {
// Fail open skips adding this filter to callbacks.
return nullptr;
} else {
// Fail closed is handled by an empty Context.
return std::make_shared<Context>(nullptr, 0, plugin_);
}
}
if (wasm && !root_context_id_) {
root_context_id_ = wasm->getRootContext(plugin_, false)->id();
}
return std::make_shared<Context>(wasm, root_context_id_, plugin_);
return std::make_shared<Context>(wasm, handle->rootContextId(), plugin_);
}

private:
uint32_t root_context_id_{0};
PluginSharedPtr plugin_;
ThreadLocal::TypedSlotPtr<PluginHandle> tls_slot_;
Config::DataSource::RemoteAsyncDataProviderPtr remote_data_provider_;
Expand Down
16 changes: 9 additions & 7 deletions source/extensions/filters/network/wasm/wasm_filter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,21 @@ class FilterConfig : Logger::Loggable<Logger::Id::wasm> {
if (handle.has_value()) {
wasm = handle->wasm().get();
}
if (plugin_->fail_open_ && (!wasm || wasm->isFailed())) {
return nullptr;
if (!wasm || wasm->isFailed()) {
if (plugin_->fail_open_) {
// Fail open skips adding this filter to callbacks.
return nullptr;
} else {
// Fail closed is handled by an empty Context.
return std::make_shared<Context>(nullptr, 0, plugin_);
}
}
if (wasm && !root_context_id_) {
root_context_id_ = wasm->getRootContext(plugin_, false)->id();
}
return std::make_shared<Context>(wasm, root_context_id_, plugin_);
return std::make_shared<Context>(wasm, handle->rootContextId(), plugin_);
}

Wasm* wasmForTest() { return tls_slot_->get()->wasm().get(); }

private:
uint32_t root_context_id_{0};
PluginSharedPtr plugin_;
ThreadLocal::TypedSlotPtr<PluginHandle> tls_slot_;
Config::DataSource::RemoteAsyncDataProviderPtr remote_data_provider_;
Expand Down
24 changes: 12 additions & 12 deletions test/extensions/common/wasm/wasm_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -711,10 +711,10 @@ TEST_P(WasmCommonTest, VmCache) {
});
return std::make_shared<WasmHandle>(wasm);
},
[](const WasmHandleBaseSharedPtr& base_wasm,
absl::string_view plugin_key) -> PluginHandleBaseSharedPtr {
return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(base_wasm),
plugin_key);
[](const WasmHandleBaseSharedPtr& wasm_handle,
const PluginBaseSharedPtr& plugin) -> PluginHandleBaseSharedPtr {
return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(wasm_handle),
std::static_pointer_cast<Plugin>(plugin));
});
wasm_handle.reset();
wasm_handle2.reset();
Expand Down Expand Up @@ -818,10 +818,10 @@ TEST_P(WasmCommonTest, RemoteCode) {
});
return std::make_shared<WasmHandle>(wasm);
},
[](const WasmHandleBaseSharedPtr& base_wasm,
absl::string_view plugin_key) -> PluginHandleBaseSharedPtr {
return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(base_wasm),
plugin_key);
[](const WasmHandleBaseSharedPtr& wasm_handle,
const PluginBaseSharedPtr& plugin) -> PluginHandleBaseSharedPtr {
return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(wasm_handle),
std::static_pointer_cast<Plugin>(plugin));
});
wasm_handle.reset();

Expand Down Expand Up @@ -936,10 +936,10 @@ TEST_P(WasmCommonTest, RemoteCodeMultipleRetry) {
});
return std::make_shared<WasmHandle>(wasm);
},
[](const WasmHandleBaseSharedPtr& base_wasm,
absl::string_view plugin_key) -> PluginHandleBaseSharedPtr {
return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(base_wasm),
plugin_key);
[](const WasmHandleBaseSharedPtr& wasm_handle,
const PluginBaseSharedPtr& plugin) -> PluginHandleBaseSharedPtr {
return std::make_shared<PluginHandle>(std::static_pointer_cast<WasmHandle>(wasm_handle),
std::static_pointer_cast<Plugin>(plugin));
});
wasm_handle.reset();

Expand Down
23 changes: 23 additions & 0 deletions test/extensions/filters/network/wasm/config_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,29 @@ TEST_P(WasmNetworkFilterConfigTest, YamlLoadInlineBadCodeFailOpenNackConfig) {
"Unable to create Wasm network filter test");
}

TEST_P(WasmNetworkFilterConfigTest, FilterConfigFailClosed) {
if (GetParam() == "null") {
return;
}
const std::string yaml = TestEnvironment::substitute(absl::StrCat(R"EOF(
config:
vm_config:
runtime: "envoy.wasm.runtime.)EOF",
GetParam(), R"EOF("
code:
local:
filename: "{{ test_rundir }}/test/extensions/filters/network/wasm/test_data/test_cpp.wasm"
)EOF"));

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();
EXPECT_EQ(context->wasm(), nullptr);
EXPECT_TRUE(context->isFailed());
}

TEST_P(WasmNetworkFilterConfigTest, FilterConfigFailOpen) {
if (GetParam() == "null") {
return;
Expand Down