Skip to content

Commit

Permalink
bootstrap-extensions: fix a crash on http callout (envoyproxy#14478)
Browse files Browse the repository at this point in the history
Currently when the ServerFactoryContext is passed to bootstrap extensions, it is only partially initialized. Specifically, attempting to access the cluster manager will cause a nullptr access (and hence a crash)

This PR splits the creation and initialized to 2 seperate fucntions. Early creation is required to not break the `default_socket_interface` feature. Once created, the extension will receive the ServerFactoryContext in a different callback (the newly added `serverInitialized`), once they are fully initialized.


Commit Message:
Fix a crash that happens when bootstrap extensions perform http calls.

Additional Description:
Risk Level: Low (small bug-fix)
Testing: Unit tests updated; tested manually with the changes as well.
Docs Changes: N/A
Release Notes: N/A

Fixes envoyproxy#14420

Signed-off-by: Yuval Kohavi <yuval.kohavi@gmail.com>
  • Loading branch information
yuval-k authored and npolshakova committed Apr 22, 2021
1 parent d1314d0 commit c540778
Show file tree
Hide file tree
Showing 18 changed files with 247 additions and 39 deletions.
8 changes: 7 additions & 1 deletion include/envoy/server/bootstrap_extension_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@ namespace Server {
class BootstrapExtension {
public:
virtual ~BootstrapExtension() = default;

/**
* Called when server is done initializing and we have the ServerFactoryContext fully initialized.
*/
virtual void onServerInitialized() PURE;
};

using BootstrapExtensionPtr = std::unique_ptr<BootstrapExtension>;
Expand All @@ -34,7 +39,8 @@ class BootstrapExtensionFactory : public Config::TypedFactory {
* implementation is unable to produce a factory with the provided parameters, it should throw an
* EnvoyException. The returned pointer should never be nullptr.
* @param config the custom configuration for this bootstrap extension type.
* @param context general filter context through which persistent resources can be accessed.
* @param context is the context to use for the extension. Note that the clusterManager is not
* yet initialized at this point and **must not** be used.
*/
virtual BootstrapExtensionPtr createBootstrapExtension(const Protobuf::Message& config,
ServerFactoryContext& context) PURE;
Expand Down
2 changes: 2 additions & 0 deletions source/common/network/socket_interface.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ namespace Network {
class SocketInterfaceExtension : public Server::BootstrapExtension {
public:
SocketInterfaceExtension(SocketInterface& sock_interface) : sock_interface_(sock_interface) {}
// Server::BootstrapExtension
void onServerInitialized() override {}

protected:
SocketInterface& sock_interface_;
Expand Down
1 change: 1 addition & 0 deletions source/common/network/socket_interface_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ class SocketInterfaceImpl : public SocketInterfaceBase {
Server::BootstrapExtensionPtr
createBootstrapExtension(const Protobuf::Message& config,
Server::Configuration::ServerFactoryContext& context) override;

ProtobufTypes::MessagePtr createEmptyConfigProto() override;
std::string name() const override {
return "envoy.extensions.network.socket_interface.default_socket_interface";
Expand Down
33 changes: 13 additions & 20 deletions source/extensions/bootstrap/wasm/config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,19 +14,16 @@ namespace Extensions {
namespace Bootstrap {
namespace Wasm {

static const std::string INLINE_STRING = "<inline>";
void WasmServiceExtension::onServerInitialized() { createWasm(context_); }

void WasmFactory::createWasm(const envoy::extensions::wasm::v3::WasmService& config,
Server::Configuration::ServerFactoryContext& context,
CreateWasmServiceCallback&& cb) {
void WasmServiceExtension::createWasm(Server::Configuration::ServerFactoryContext& context) {
auto plugin = std::make_shared<Common::Wasm::Plugin>(
config.config().name(), config.config().root_id(), config.config().vm_config().vm_id(),
config.config().vm_config().runtime(),
Common::Wasm::anyToBytes(config.config().configuration()), config.config().fail_open(),
config_.config().name(), config_.config().root_id(), config_.config().vm_config().vm_id(),
config_.config().vm_config().runtime(),
Common::Wasm::anyToBytes(config_.config().configuration()), config_.config().fail_open(),
envoy::config::core::v3::TrafficDirection::UNSPECIFIED, context.localInfo(), nullptr);

bool singleton = config.singleton();
auto callback = [&context, singleton, plugin, cb](Common::Wasm::WasmHandleSharedPtr base_wasm) {
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_);
Expand All @@ -35,10 +32,11 @@ void WasmFactory::createWasm(const envoy::extensions::wasm::v3::WasmService& con
}
return;
}
if (singleton) {
if (config_.singleton()) {
// Return a Wasm VM which will be stored as a singleton by the Server.
cb(std::make_unique<WasmService>(plugin, Common::Wasm::getOrCreateThreadLocalPlugin(
base_wasm, plugin, context.dispatcher())));
wasm_service_ = std::make_unique<WasmService>(
plugin,
Common::Wasm::getOrCreateThreadLocalPlugin(base_wasm, plugin, context.dispatcher()));
return;
}
// Per-thread WASM VM.
Expand All @@ -48,11 +46,11 @@ void WasmFactory::createWasm(const envoy::extensions::wasm::v3::WasmService& con
tls_slot->set([base_wasm, plugin](Event::Dispatcher& dispatcher) {
return Common::Wasm::getOrCreateThreadLocalPlugin(base_wasm, plugin, dispatcher);
});
cb(std::make_unique<WasmService>(plugin, std::move(tls_slot)));
wasm_service_ = std::make_unique<WasmService>(plugin, std::move(tls_slot));
};

if (!Common::Wasm::createWasm(
config.config().vm_config(), plugin, context.scope().createScope(""),
config_.config().vm_config(), plugin, context.scope().createScope(""),
context.clusterManager(), context.initManager(), context.dispatcher(), 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
Expand All @@ -69,12 +67,7 @@ WasmFactory::createBootstrapExtension(const Protobuf::Message& config,
MessageUtil::downcastAndValidate<const envoy::extensions::wasm::v3::WasmService&>(
config, context.messageValidationContext().staticValidationVisitor());

auto wasm_service_extension = std::make_unique<WasmServiceExtension>();
createWasm(typed_config, context,
[extension = wasm_service_extension.get()](WasmServicePtr wasm) {
extension->wasm_service_ = std::move(wasm);
});
return wasm_service_extension;
return std::make_unique<WasmServiceExtension>(typed_config, context);
}

// /**
Expand Down
22 changes: 11 additions & 11 deletions source/extensions/bootstrap/wasm/config.h
Original file line number Diff line number Diff line change
Expand Up @@ -34,37 +34,37 @@ class WasmService {
};

using WasmServicePtr = std::unique_ptr<WasmService>;
using CreateWasmServiceCallback = std::function<void(WasmServicePtr)>;

class WasmFactory : public Server::Configuration::BootstrapExtensionFactory,
Logger::Loggable<Logger::Id::wasm> {
class WasmFactory : public Server::Configuration::BootstrapExtensionFactory {
public:
~WasmFactory() override = default;
std::string name() const override { return "envoy.bootstrap.wasm"; }
void createWasm(const envoy::extensions::wasm::v3::WasmService& config,
Server::Configuration::ServerFactoryContext& context,
CreateWasmServiceCallback&& cb);
Server::BootstrapExtensionPtr
createBootstrapExtension(const Protobuf::Message& config,
Server::Configuration::ServerFactoryContext& context) override;
ProtobufTypes::MessagePtr createEmptyConfigProto() override {
return std::make_unique<envoy::extensions::wasm::v3::WasmService>();
}

private:
Config::DataSource::RemoteAsyncDataProviderPtr remote_data_provider_;
};

class WasmServiceExtension : public Server::BootstrapExtension {
class WasmServiceExtension : public Server::BootstrapExtension, Logger::Loggable<Logger::Id::wasm> {
public:
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_;
}
void onServerInitialized() override;

private:
void createWasm(Server::Configuration::ServerFactoryContext& context);

envoy::extensions::wasm::v3::WasmService config_;
Server::Configuration::ServerFactoryContext& context_;
WasmServicePtr wasm_service_;
friend class WasmFactory;
Config::DataSource::RemoteAsyncDataProviderPtr remote_data_provider_;
};

} // namespace Wasm
Expand Down
10 changes: 9 additions & 1 deletion source/server/server.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,10 @@ InstanceImpl::~InstanceImpl() {
dispatcher_->shutdown();
}

Upstream::ClusterManager& InstanceImpl::clusterManager() { return *config_.clusterManager(); }
Upstream::ClusterManager& InstanceImpl::clusterManager() {
ASSERT(config_.clusterManager() != nullptr);
return *config_.clusterManager();
}

void InstanceImpl::drainListeners() {
ENVOY_LOG(info, "closing and draining listeners");
Expand Down Expand Up @@ -569,6 +572,11 @@ void InstanceImpl::initialize(const Options& options,
stat_flush_timer_->enableTimer(stats_config.flushInterval());
}

// Now that we are initialized, notify the bootstrap extensions.
for (auto&& bootstrap_extension : bootstrap_extensions_) {
bootstrap_extension->onServerInitialized();
}

// GuardDog (deadlock detection) object and thread setup before workers are
// started and before our own run() loop runs.
main_thread_guard_dog_ = std::make_unique<Server::GuardDogImpl>(
Expand Down
6 changes: 6 additions & 0 deletions test/config/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1154,6 +1154,12 @@ void ConfigHelper::addListenerFilter(const std::string& filter_yaml) {
}
}

void ConfigHelper::addBootstrapExtension(const std::string& config) {
RELEASE_ASSERT(!finalized_, "");
auto* extension = bootstrap_.add_bootstrap_extensions();
TestUtility::loadFromYaml(config, *extension);
}

bool ConfigHelper::loadHttpConnectionManager(
envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& hcm) {
return loadFilter<
Expand Down
3 changes: 3 additions & 0 deletions test/config/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,9 @@ class ConfigHelper {
// Add a listener filter prior to existing filters.
void addListenerFilter(const std::string& filter_yaml);

// Add a new bootstrap extension.
void addBootstrapExtension(const std::string& config);

// Sets the client codec to the specified type.
void setClientCodec(envoy::extensions::filters::network::http_connection_manager::v3::
HttpConnectionManager::CodecType type);
Expand Down
15 changes: 15 additions & 0 deletions test/extensions/bootstrap/wasm/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,21 @@ envoy_extension_cc_test(
],
)

envoy_extension_cc_test(
name = "wasm_integration_test",
srcs = ["wasm_integration_test.cc"],
data = envoy_select_wasm([
"//test/extensions/bootstrap/wasm/test_data:http_cpp.wasm",
]),
extension_name = "envoy.bootstrap.wasm",
deps = [
"//source/extensions/bootstrap/wasm:config",
"//source/extensions/common/wasm:wasm_lib",
"//test/extensions/common/wasm:wasm_runtime",
"//test/integration:http_protocol_integration_lib",
],
)

envoy_extension_cc_test(
name = "config_test",
srcs = ["config_test.cc"],
Expand Down
1 change: 1 addition & 0 deletions test/extensions/bootstrap/wasm/config_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ class WasmFactoryTest : public testing::TestWithParam<std::string> {
EXPECT_CALL(context_, lifecycleNotifier())
.WillRepeatedly(testing::ReturnRef(lifecycle_notifier_));
extension_ = factory->createBootstrapExtension(config, context_);
extension_->onServerInitialized();
static_cast<Bootstrap::Wasm::WasmServiceExtension*>(extension_.get())->wasmService();
EXPECT_CALL(init_watcher_, ready());
init_manager_.initialize(init_watcher_);
Expand Down
5 changes: 5 additions & 0 deletions test/extensions/bootstrap/wasm/test_data/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@ envoy_wasm_cc_binary(
srcs = ["emscripten_cpp.cc"],
)

envoy_wasm_cc_binary(
name = "http_cpp.wasm",
srcs = ["http_cpp.cc"],
)

envoy_wasm_cc_binary(
name = "logging_cpp.wasm",
srcs = ["logging_cpp.cc"],
Expand Down
46 changes: 46 additions & 0 deletions test/extensions/bootstrap/wasm/test_data/http_cpp.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
// NOLINT(namespace-envoy)
#include <string>

#include "proxy_wasm_intrinsics.h"

template <typename T> std::unique_ptr<T> wrap_unique(T* ptr) { return std::unique_ptr<T>(ptr); }

START_WASM_PLUGIN(WasmHttpCpp)

// Required Proxy-Wasm ABI version.
WASM_EXPORT(void, proxy_abi_version_0_1_0, ()) {}

WASM_EXPORT(uint32_t, proxy_on_configure, (uint32_t, uint32_t)) {
proxy_set_tick_period_milliseconds(100);
return 1;
}

WASM_EXPORT(void, proxy_on_tick, (uint32_t)) {
HeaderStringPairs headers;
headers.push_back(std::make_pair<std::string, std::string>(":method", "GET"));
headers.push_back(std::make_pair<std::string, std::string>(":path", "/"));
headers.push_back(std::make_pair<std::string, std::string>(":authority", "example.com"));
headers.push_back(std::make_pair<std::string, std::string>("x-test", "test"));
HeaderStringPairs trailers;
uint32_t token;
WasmResult result = makeHttpCall("wasm_cluster", headers, "", trailers, 10000, &token);
// We have sent successfully, stop timer - we only want to send one request.
if (result == WasmResult::Ok) {
proxy_set_tick_period_milliseconds(0);
}
}

WASM_EXPORT(void, proxy_on_http_call_response, (uint32_t, uint32_t, uint32_t headers, uint32_t, uint32_t)) {
if (headers != 0) {
auto status = getHeaderMapValue(WasmHeaderMapType::HttpCallResponseHeaders, "status");
if ("200" == status->view()) {
proxy_set_tick_period_milliseconds(0);
return;
}
}
// Request failed - very possibly because of the integration test not being ready.
// Try again to prevent flakes.
proxy_set_tick_period_milliseconds(100);
}

END_WASM_PLUGIN
95 changes: 95 additions & 0 deletions test/extensions/bootstrap/wasm/wasm_integration_test.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
#include "extensions/common/wasm/wasm.h"

#include "test/extensions/common/wasm/wasm_runtime.h"
#include "test/integration/http_protocol_integration.h"

#include "gtest/gtest.h"

namespace Envoy {
namespace Extensions {
namespace Wasm {
namespace {

class WasmIntegrationTest : public HttpIntegrationTest, public testing::TestWithParam<std::string> {
public:
WasmIntegrationTest()
: HttpIntegrationTest(Http::CodecClient::Type::HTTP1, Network::Address::IpVersion::v4) {}

void createUpstreams() override {
HttpIntegrationTest::createUpstreams();
addFakeUpstream(FakeHttpConnection::Type::HTTP1);
}

void cleanup() {
if (wasm_connection_ != nullptr) {
ASSERT_TRUE(wasm_connection_->close());
ASSERT_TRUE(wasm_connection_->waitForDisconnect());
}
cleanupUpstreamAndDownstream();
}
void initialize() override {
auto httpwasm = TestEnvironment::substitute(
"{{ test_rundir }}/test/extensions/bootstrap/wasm/test_data/http_cpp.wasm");
config_helper_.addConfigModifier([](envoy::config::bootstrap::v3::Bootstrap& bootstrap) {
auto* wasm = bootstrap.mutable_static_resources()->add_clusters();
wasm->MergeFrom(bootstrap.static_resources().clusters()[0]);
wasm->set_name("wasm_cluster");
});

config_helper_.addBootstrapExtension(fmt::format(R"EOF(
name: envoy.filters.http.wasm
typed_config:
'@type': type.googleapis.com/envoy.extensions.wasm.v3.WasmService
singleton: true
config:
name: "singleton"
root_id: "singleton"
configuration:
'@type': type.googleapis.com/google.protobuf.StringValue
value: ""
vm_config:
vm_id: "my_vm_id"
runtime: "envoy.wasm.runtime.{}"
code:
local:
filename: {}
)EOF",
GetParam(), httpwasm));
HttpIntegrationTest::initialize();
}

FakeHttpConnectionPtr wasm_connection_;
FakeStreamPtr wasm_request_;
IntegrationStreamDecoderPtr response_;
};

INSTANTIATE_TEST_SUITE_P(Runtimes, WasmIntegrationTest,
Envoy::Extensions::Common::Wasm::sandbox_runtime_values,
Envoy::Extensions::Common::Wasm::wasmTestParamsToString);
GTEST_ALLOW_UNINSTANTIATED_PARAMETERIZED_TEST(WasmIntegrationTest);

TEST_P(WasmIntegrationTest, FilterMakesCallInConfigureTime) {
initialize();
ASSERT_TRUE(fake_upstreams_.back()->waitForHttpConnection(*dispatcher_, wasm_connection_));

// Expect the filter to send us an HTTP request
ASSERT_TRUE(wasm_connection_->waitForNewStream(*dispatcher_, wasm_request_));
ASSERT_TRUE(wasm_request_->waitForEndStream(*dispatcher_));

EXPECT_EQ("test", wasm_request_->headers()
.get(Envoy::Http::LowerCaseString("x-test"))[0]
->value()
.getStringView());

// Respond back to the filter.
Http::TestResponseHeaderMapImpl response_headers{
{":status", "200"},
};
wasm_request_->encodeHeaders(response_headers, true);
cleanup();
}

} // namespace
} // namespace Wasm
} // namespace Extensions
} // namespace Envoy
4 changes: 4 additions & 0 deletions test/extensions/common/wasm/wasm_runtime.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,10 @@ std::vector<std::tuple<std::string, std::string>> runtimesAndLanguages() {
return values;
}

std::string wasmTestParamsToString(const ::testing::TestParamInfo<std::string>& p) {
return p.param;
}

} // namespace Wasm
} // namespace Common
} // namespace Extensions
Expand Down
Loading

0 comments on commit c540778

Please sign in to comment.