Skip to content
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

wasm: removed automatical route refreshment and add a foreign function to clear the route cache #36671

Merged
merged 12 commits into from
Oct 30, 2024
Merged

wasm: removed automatical route refreshment and add a foreign function to clear the route cache #36671

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
de74d9f
dirty commit
Oct 17, 2024
b8c210c
Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-d…
Oct 17, 2024
dad1f7b
complete test
Oct 17, 2024
8080459
add log and change log
Oct 17, 2024
5c80bc7
fix test
Oct 17, 2024
27830ab
address comments and fix test
Oct 18, 2024
13e26ea
address comments
Oct 19, 2024
1f78fad
add more tests and only change default behavior for proxy-wasm > 0.2.1
Oct 22, 2024
1da41e1
Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-d…
Oct 22, 2024
8f5eafa
address comments
Oct 23, 2024
ead4115
Merge branch 'main' of https://github.com/envoyproxy/envoy into dev-d…
Oct 23, 2024
87a3a55
fix format
Oct 23, 2024
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
7 changes: 7 additions & 0 deletions changelogs/current.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@ date: Pending

behavior_changes:
# *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required*
- area: wasm
change: |
The route cache will not be cleared by default if the wasm extension modified the request headers and
the ABI version of wasm extension is larger then 0.2.1.
- area: wasm
change: |
Remove previously deprecated xDS attributes from ``get_property``, use ``xds`` attributes instead.
Expand Down Expand Up @@ -49,6 +53,9 @@ new_features:
- area: tls
change: |
Added support for P-384 and P-521 curves for TLS server certificates.
- area: wasm
change: |
added ``clear_route_cache`` foreign function to clear the route cache.
- area: access_log
change: |
Added %DOWNSTREAM_LOCAL_EMAIL_SAN%, %DOWNSTREAM_PEER_EMAIL_SAN%, %DOWNSTREAM_LOCAL_OTHERNAME_SAN% and
Expand Down
31 changes: 17 additions & 14 deletions source/extensions/common/wasm/context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,23 @@ WasmResult Buffer::copyFrom(size_t start, size_t length, std::string_view data)
}

Context::Context() = default;
Context::Context(Wasm* wasm) : ContextBase(wasm) {}
Context::Context(Wasm* wasm) : ContextBase(wasm) {
if (wasm != nullptr) {
abi_version_ = wasm->abi_version_;
}
Comment on lines +157 to +159
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this is available via wasm()->abiVersion() in the callbacks, so you shouldn't need to retain it.

Copy link
Member Author

@wbpcode wbpcode Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, you are completely right at the production code view. But this is helpful for our tests now to allow us to mock different abi versions.

Our tests also need to be improved to avoid this, but, I want to put this to a refactoring only PR, for our tests/Plugin class/Wasm class etc.

(Much things need to do... :(

Copy link
Contributor

@mpwarres mpwarres Oct 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(optional) WDYT about instead defining a virtual Context::getAbiVersion() method whose default impl calls wasm()->abiVersion(), but can be overridden by TestContext?

The minor quibble I have with adding an abi_version_ field here is that now the information is stored in multiple places, as opposed to one authoritative source.

Copy link
Member Author

Choose a reason for hiding this comment

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

(optional) WDYT about instead defining a virtual Context::getAbiVersion() method whose default impl calls wasm()->abiVersion(), but can be overridden by TestContext?

The minor quibble I have with adding an abi_version_ field here is that now the information is stored in multiple places, as opposed to one authoritative source.

SGTM. But I want to did some refactoring to these code anyway, will defer this to future PR.

}
Context::Context(Wasm* wasm, const PluginSharedPtr& plugin) : ContextBase(wasm, plugin) {
if (wasm != nullptr) {
abi_version_ = wasm->abi_version_;
}
root_local_info_ = &std::static_pointer_cast<Plugin>(plugin)->localInfo();
}
Context::Context(Wasm* wasm, uint32_t root_context_id, PluginHandleSharedPtr plugin_handle)
: ContextBase(wasm, root_context_id, plugin_handle), plugin_handle_(plugin_handle) {}
: ContextBase(wasm, root_context_id, plugin_handle), plugin_handle_(plugin_handle) {
if (wasm != nullptr) {
abi_version_ = wasm->abi_version_;
}
}

Wasm* Context::wasm() const { return static_cast<Wasm*>(wasm_); }
Plugin* Context::plugin() const { return static_cast<Plugin*>(plugin_.get()); }
Expand Down Expand Up @@ -671,9 +682,7 @@ WasmResult Context::addHeaderMapValue(WasmHeaderMapType type, std::string_view k
}
const Http::LowerCaseString lower_key{std::string(key)};
map->addCopy(lower_key, std::string(value));
if (type == WasmHeaderMapType::RequestHeaders) {
clearRouteCache();
}
onHeadersModified(type);
return WasmResult::Ok;
}

Expand Down Expand Up @@ -746,9 +755,7 @@ WasmResult Context::setHeaderMapPairs(WasmHeaderMapType type, const Pairs& pairs
const Http::LowerCaseString lower_key{std::string(p.first)};
map->addCopy(lower_key, std::string(p.second));
}
if (type == WasmHeaderMapType::RequestHeaders) {
clearRouteCache();
}
onHeadersModified(type);
return WasmResult::Ok;
}

Expand All @@ -759,9 +766,7 @@ WasmResult Context::removeHeaderMapValue(WasmHeaderMapType type, std::string_vie
}
const Http::LowerCaseString lower_key{std::string(key)};
map->remove(lower_key);
if (type == WasmHeaderMapType::RequestHeaders) {
clearRouteCache();
}
onHeadersModified(type);
return WasmResult::Ok;
}

Expand All @@ -773,9 +778,7 @@ WasmResult Context::replaceHeaderMapValue(WasmHeaderMapType type, std::string_vi
}
const Http::LowerCaseString lower_key{std::string(key)};
map->setCopy(lower_key, toAbslStringView(value));
if (type == WasmHeaderMapType::RequestHeaders) {
clearRouteCache();
}
onHeadersModified(type);
return WasmResult::Ok;
}

Expand Down
10 changes: 10 additions & 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 @@ -374,6 +374,14 @@ class Context : public proxy_wasm::ContextBase,
Http::HeaderMap* getMap(WasmHeaderMapType type);
const Http::HeaderMap* getConstMap(WasmHeaderMapType type);

void onHeadersModified(WasmHeaderMapType type) {
if (type != WasmHeaderMapType::RequestHeaders ||
abi_version_ > proxy_wasm::AbiVersion::ProxyWasm_0_2_1) {
return;
}
clearRouteCache();
}

const LocalInfo::LocalInfo* root_local_info_{nullptr}; // set only for root_context.
PluginHandleSharedPtr plugin_handle_{nullptr};

Expand Down Expand Up @@ -450,6 +458,8 @@ class Context : public proxy_wasm::ContextBase,
// Filter state prototype declaration.
absl::flat_hash_map<std::string, Filters::Common::Expr::CelStatePrototypeConstPtr>
state_prototypes_;

proxy_wasm::AbiVersion abi_version_{proxy_wasm::AbiVersion::Unknown};
};
using ContextSharedPtr = std::shared_ptr<Context>;

Expand Down
8 changes: 8 additions & 0 deletions source/extensions/common/wasm/foreign.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,14 @@ RegisterForeignFunction registerSetEnvoyFilterStateForeignFunction(
return WasmResult::BadArgument;
});

RegisterForeignFunction registerClearRouteCacheForeignFunction(
"clear_route_cache",
[](WasmBase&, std::string_view, const std::function<void*(size_t size)>&) -> WasmResult {
auto context = static_cast<Context*>(proxy_wasm::current_context_);
context->clearRouteCache();
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Is this safe to call at any point in execution? This is untrusted code calling out, we might need to harden it if it's not already done.

Copy link
Member Author

Choose a reason for hiding this comment

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

I remember I did check in the clearRouteCache self. Will re-check it again.

Copy link
Member Author

Choose a reason for hiding this comment

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

The clearRouteCache will be forbided after the response headers is processed. But I agree we should harden it continuously to ensure the interface that's provided to extension is robust enough.

For example, only allow the head filter to clear the route cache. I will do this with another PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think from the separate discussion in proxy-wasm-spec, what we need is a control knob on the filter xDS with several options:

  1. Clear route cache on every header operation (past default).
  2. No clear route cache at all (future default).
  3. Permit clear route cache external function (to disallow untrusted extensions from causing harm by clearing routes).
  4. Automatic clear route after callback (to mimic ext_proc, not needed for your use case but it will be easier to handle dynamic metadata routing, for example).

We can split this into several booleans as well (e.g. 3 - could be a separate knob).
@PiotrSikora - does that look right to you?

Copy link
Member Author

Choose a reason for hiding this comment

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

Now, this PR removed the automatic route refresh and provide a manual way. This should be good enough as a default way to handle the route cache.

New options (3 and 4) could be treated as independent requirement of Envoy-self and be implemented in the future.

I think a basic rule is listening the feedbacks from acutal practices and responding them quickly, so, if some users have actual related requirements, I am happy to do it. :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@kyessenov yeah, that sounds good, although I'm not sure I understand the difference between 1 and 4.

Copy link
Member Author

@wbpcode wbpcode Oct 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@kyessenov yeah, that sounds good, although I'm not sure I understand the difference between 1 and 4.

The difference is that in which case we need additional xDS control. In the new implementation, the automical refresh should be enabled by an explict option.

seems I didn't got it correctly. 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

@PiotrSikora The difference is that 1) clears route only on header operations but 4) should clear routes on any dynamic metadata or other non-HTTP attribute update

return WasmResult::Ok;
});

#if defined(WASM_USE_CEL_PARSER)
class ExpressionFactory : public Logger::Loggable<Logger::Id::wasm> {
protected:
Expand Down
7 changes: 7 additions & 0 deletions test/extensions/common/wasm/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ envoy_cc_test(
"//test/mocks/http:http_mocks",
"//test/mocks/network:network_mocks",
"//test/mocks/stream_info:stream_info_mocks",
"//test/test_common:test_runtime_lib",
],
)

Expand All @@ -141,19 +142,25 @@ envoy_cc_test(
"-DWASM_USE_CEL_PARSER",
],
}),
data = envoy_select_wasm_cpp_tests([
"//test/extensions/common/wasm/test_data:test_context_cpp.wasm",
]),
rbe_pool = "6gig",
tags = ["skip_on_windows"],
deps = [
":wasm_runtime",
"//source/common/network:filter_state_dst_address_lib",
"//source/common/tcp_proxy",
"//source/extensions/clusters/original_dst:original_dst_cluster_lib",
"//source/extensions/common/wasm:wasm_hdr",
"//source/extensions/common/wasm:wasm_lib",
"//test/extensions/common/wasm/test_data:test_context_cpp_plugin",
"//test/mocks/http:http_mocks",
"//test/mocks/local_info:local_info_mocks",
"//test/mocks/network:network_mocks",
"//test/mocks/upstream:upstream_mocks",
"//test/test_common:environment_lib",
"//test/test_common:wasm_lib",
"@proxy_wasm_cpp_host//:base_lib",
],
)
Expand Down
61 changes: 46 additions & 15 deletions test/extensions/common/wasm/context_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#include "test/mocks/http/mocks.h"
#include "test/mocks/network/mocks.h"
#include "test/mocks/stream_info/mocks.h"
#include "test/test_common/test_runtime.h"

#include "eval/public/cel_value.h"
#include "gmock/gmock.h"
Expand All @@ -21,6 +22,12 @@ using google::api::expr::runtime::CelValue;

class TestContext : public Context {
public:
TestContext(absl::optional<proxy_wasm::AbiVersion> mock_abi_version = {}) {
if (mock_abi_version.has_value()) {
abi_version_ = mock_abi_version.value();
}
}

void setEncoderFilterCallbacksPtr(Envoy::Http::StreamEncoderFilterCallbacks* cb) {
encoder_callbacks_ = cb;
}
Expand Down Expand Up @@ -247,7 +254,8 @@ TEST_F(ContextTest, FindValueTest) {
EXPECT_FALSE(ctx_.FindValue("plugin_name", &arena).has_value());
}

TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfiguration) {
TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfigurationForLegacyWasmPlugin) {
TestContext test_ctx(proxy_wasm::AbiVersion::ProxyWasm_0_2_1);

Http::MockDownstreamStreamFilterCallbacks downstream_callbacks;
EXPECT_CALL(downstream_callbacks, clearRouteCache()).Times(5).WillRepeatedly(testing::Return());
Expand All @@ -256,34 +264,57 @@ TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfiguration) {
EXPECT_CALL(decoder_callbacks, downstreamCallbacks())
.WillRepeatedly(testing::Return(
makeOptRef(dynamic_cast<Http::DownstreamStreamFilterCallbacks&>(downstream_callbacks))));
ctx_.setDecoderFilterCallbacksPtr(&decoder_callbacks);
test_ctx.setDecoderFilterCallbacksPtr(&decoder_callbacks);

Http::TestRequestHeaderMapImpl request_headers{{":path", "/123"}};
test_ctx.setRequestHeaders(&request_headers);

test_ctx.clearRouteCache();
test_ctx.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value");
test_ctx.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}});
test_ctx.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2");
test_ctx.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key");
}

TEST_F(ContextTest, NoAutoClearRouteCacheCalledInDownstreamConfiguration) {
TestContext test_ctx;

Http::MockDownstreamStreamFilterCallbacks downstream_callbacks;
EXPECT_CALL(downstream_callbacks, clearRouteCache()).WillOnce(testing::Return());

Http::MockStreamDecoderFilterCallbacks decoder_callbacks;
EXPECT_CALL(decoder_callbacks, downstreamCallbacks())
.WillRepeatedly(testing::Return(
makeOptRef(dynamic_cast<Http::DownstreamStreamFilterCallbacks&>(downstream_callbacks))));
test_ctx.setDecoderFilterCallbacksPtr(&decoder_callbacks);

Http::TestRequestHeaderMapImpl request_headers{{":path", "/123"}};
ctx_.setRequestHeaders(&request_headers);
test_ctx.setRequestHeaders(&request_headers);

ctx_.clearRouteCache();
ctx_.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value");
ctx_.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}});
ctx_.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2");
ctx_.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key");
test_ctx.clearRouteCache();
test_ctx.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value");
test_ctx.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}});
test_ctx.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2");
test_ctx.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key");
}

TEST_F(ContextTest, ClearRouteCacheDoesNothingInUpstreamConfiguration) {
TestContext test_ctx;

Http::MockStreamDecoderFilterCallbacks decoder_callbacks;
EXPECT_CALL(decoder_callbacks, downstreamCallbacks())
.WillRepeatedly(testing::Return(absl::nullopt));
ctx_.setDecoderFilterCallbacksPtr(&decoder_callbacks);
test_ctx.setDecoderFilterCallbacksPtr(&decoder_callbacks);

Http::TestRequestHeaderMapImpl request_headers{{":path", "/123"}};
ctx_.setRequestHeaders(&request_headers);
test_ctx.setRequestHeaders(&request_headers);

// Calling methods should be safe.
ctx_.clearRouteCache();
ctx_.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value");
ctx_.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}});
ctx_.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2");
ctx_.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key");
test_ctx.clearRouteCache();
test_ctx.addHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value");
test_ctx.setHeaderMapPairs(WasmHeaderMapType::RequestHeaders, Pairs{{"key2", "value2"}});
test_ctx.replaceHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key", "value2");
test_ctx.removeHeaderMapValue(WasmHeaderMapType::RequestHeaders, "key");
}

} // namespace Wasm
Expand Down
32 changes: 32 additions & 0 deletions test/extensions/common/wasm/foreign_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,13 @@
#include "source/extensions/common/wasm/ext/set_envoy_filter_state.pb.h"
#include "source/extensions/common/wasm/wasm.h"

#include "test/extensions/common/wasm/wasm_runtime.h"
#include "test/mocks/http/mocks.h"
#include "test/mocks/local_info/mocks.h"
#include "test/mocks/server/factory_context.h"
#include "test/mocks/upstream/cluster_manager.h"
#include "test/test_common/utility.h"
#include "test/test_common/wasm_base.h"

#include "gmock/gmock.h"
#include "gtest/gtest.h"
Expand Down Expand Up @@ -113,6 +117,34 @@ TEST_F(ForeignTest, ForeignFunctionSetEnvoyFilterTest) {
Upstream::OriginalDstClusterFilterStateKey));
}

class StreamForeignTest : public WasmPluginConfigTestBase<
testing::TestWithParam<std::tuple<std::string, std::string>>> {
public:
StreamForeignTest() = default;
};

INSTANTIATE_TEST_SUITE_P(PluginConfigRuntimes, StreamForeignTest,
Envoy::Extensions::Common::Wasm::runtime_and_cpp_values,
Envoy::Extensions::Common::Wasm::wasmTestParamsToString);

TEST_P(StreamForeignTest, ForeignFunctionClearRouteCache) {
auto [runtime, language] = GetParam();

auto plugin_config = getWasmPluginConfigForTest(
runtime, "test/extensions/common/wasm/test_data/test_context_cpp.wasm",
"CommonWasmTestContextCpp", "send local reply twice");

setUp(plugin_config);

createStreamContext();

proxy_wasm::current_context_ = context_.get();
auto function = proxy_wasm::getForeignFunction("clear_route_cache");

EXPECT_CALL(decoder_callbacks_.downstream_callbacks_, clearRouteCache());
function(*plugin_config_->wasm(), "", [](size_t size) { return malloc(size); });
}

} // namespace Wasm
} // namespace Common
} // namespace Extensions
Expand Down
64 changes: 64 additions & 0 deletions test/test_common/wasm_base.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,70 @@ class WasmNetworkFilterTestBase : public WasmTestBase<Base> {
NiceMock<Network::MockWriteFilterCallbacks> write_filter_callbacks_;
};

inline envoy::extensions::wasm::v3::PluginConfig
getWasmPluginConfigForTest(absl::string_view runtime, absl::string_view wasm_file_path,
absl::string_view wasm_module_name, absl::string_view plugin_root_id,
bool singleton = false, absl::string_view plugin_configuration = {}) {

const std::string plugin_config_yaml = fmt::format(
R"EOF(
name: 'test_wasm_singleton_{}'
root_id: '{}'
vm_config:
runtime: 'envoy.wasm.runtime.{}'
configuration:
"@type": "type.googleapis.com/google.protobuf.StringValue"
value: '{}'
)EOF",
singleton, plugin_root_id, runtime, plugin_configuration);

envoy::extensions::wasm::v3::PluginConfig plugin_config;
TestUtility::loadFromYaml(plugin_config_yaml, plugin_config);

if (runtime == "null") {
plugin_config.mutable_vm_config()->mutable_code()->mutable_local()->set_inline_bytes(
std::string(wasm_module_name));
} else {
const std::string code = TestEnvironment::readFileToStringForTest(TestEnvironment::substitute(
absl::StrCat("{{ test_rundir }}/" + std::string(wasm_file_path))));
plugin_config.mutable_vm_config()->mutable_code()->mutable_local()->set_inline_bytes(code);
}

return plugin_config;
}

template <typename Base = testing::Test> class WasmPluginConfigTestBase : public Base {
public:
WasmPluginConfigTestBase() = default;

// NOLINTNEXTLINE(readability-identifier-naming)
void SetUp() override { clearCodeCacheForTesting(); }

void setUp(const envoy::extensions::wasm::v3::PluginConfig plugin_config,
bool singleton = false) {
plugin_config_ = std::make_shared<PluginConfig>(
plugin_config, server_, server_.scope(), server_.initManager(),
envoy::config::core::v3::TrafficDirection::UNSPECIFIED, /*metadata=*/nullptr, singleton);
}

void createStreamContext() {
context_ = plugin_config_->createContext();
if (context_ != nullptr) {
context_->setDecoderFilterCallbacks(decoder_callbacks_);
context_->setEncoderFilterCallbacks(encoder_callbacks_);
context_->onCreate();
}
}

NiceMock<Server::Configuration::MockServerFactoryContext> server_;
PluginConfigSharedPtr plugin_config_;

NiceMock<Http::MockStreamDecoderFilterCallbacks> decoder_callbacks_;
NiceMock<Http::MockStreamEncoderFilterCallbacks> encoder_callbacks_;

std::shared_ptr<Context> context_;
};

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