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 5 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
9 changes: 9 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,12 @@ 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.
But it could be done explicitly by call the ``clear_route_cache`` foreign function.
This behavior change could be disabled temporarily by setting the runtime flag
``envoy_reloadable_features_wasm_no_automatic_route_refresh`` to false.
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI, ext_proc has configuration named disable_clear_route_cache, so it might be a good idea to align with that.

Copy link
Member Author

Choose a reason for hiding this comment

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

It basically should be replaced by route_cache_action. And the default behavior is only clear the route cache when the external plugin require it.

    // The default behavior is to clear the route cache only when the
    // :ref:`clear_route_cache <envoy_v3_api_field_service.ext_proc.v3.CommonResponse.clear_route_cache>`
    // field is set in an external processor response.
    DEFAULT = 0;

Copy link
Contributor

Choose a reason for hiding this comment

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

@wbpcode Can you introduce this to the xDS spec then instead of the runtime flag. Personally, I think it's OK to change the default to be "do nothing", and allow control planes to set it to "ON_HEADER_OPERATION" to preserve the existing semantics. Istio and other consumers can then control the behavior via xDS which is easier for upgrades.

Would that answer your concerns @PiotrSikora ?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kyessenov Fine. But let me do it step by step. I will remove runtime flag and only keep the abi version check in this PR. Considering abi 0.3.0 is not ready now, so, this change won't break anything.

And at next one or two PR I will add the new API and let the api shepherds to review the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

@kyessenov yes, that's much better, thank you!

Having said that, if we assume that control planes handle the upgrade logic, then I still don't see the benefit of breaking the current behavior and using DO_NOTHING instead of ON_HEADER_OPERATION as the default.


minor_behavior_changes:
# *Changes that may cause incompatibilities for some users, but should not for most*
Expand Down Expand Up @@ -32,5 +38,8 @@ 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.

deprecated:
1 change: 1 addition & 0 deletions source/common/runtime/runtime_features.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ RUNTIME_GUARD(envoy_reloadable_features_use_typed_metadata_in_proxy_protocol_lis
RUNTIME_GUARD(envoy_reloadable_features_validate_connect);
RUNTIME_GUARD(envoy_reloadable_features_validate_grpc_header_before_log_grpc_status);
RUNTIME_GUARD(envoy_reloadable_features_validate_upstream_headers);
RUNTIME_GUARD(envoy_reloadable_features_wasm_no_automatic_route_refresh);
RUNTIME_GUARD(envoy_reloadable_features_xds_failover_to_primary_enabled);
RUNTIME_GUARD(envoy_reloadable_features_xdstp_path_avoid_colon_encoding);
RUNTIME_GUARD(envoy_restart_features_allow_slot_destroy_on_worker_threads);
Expand Down
16 changes: 4 additions & 12 deletions source/extensions/common/wasm/context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -732,9 +732,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 @@ -807,9 +805,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 @@ -820,9 +816,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 @@ -834,9 +828,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
16 changes: 16 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,19 @@ 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) {
if (!no_automatic_route_refresh_) {
clearRouteCache();
} else {
ENVOY_LOG_FIRST_N(critical, 20,
"Route will no be refreshed automatically when request headers are "
"modified by the wasm plugin. Refresh the route manually by calling the "
"'clear_route_cache' foreign function. ");
}
}
}

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

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

const bool no_automatic_route_refresh_{
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing the default behavior will break existing Wasm users (which is "beta" in Istio). I think we need to change the xDS API instead of the runtime key. Runtime key assumes the default behavior will eventually change but we can't be certain in this case.

Copy link
Member Author

@wbpcode wbpcode Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I do don't want break exist plugins, but I cannot find better chance to bring this back to the expected way. At least for now, the wasm still not production ready.

I also doubt if it is good choice to add new API. I think the API should be part of the wasm plugin configuration, not for the host configuration.

There are also some possible compromised way:

  1. Add a runtime key (not runtime flag) to control the global default value.
  2. Try to bump the ABI version and only change the behaviour in the new ABI version.

Copy link
Contributor

Choose a reason for hiding this comment

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

Per offline discussion, a long-living runtime key is probably better to avoid a churn in the spec. Not refreshing routes with a custom host call is a safer option, until we get better guidance from Proxy-Wasm

Copy link
Contributor

Choose a reason for hiding this comment

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

I do don't want break exist plugins, but I cannot find better chance to bring this back to the expected way.

Don't change the default, and instead add xDS configuration option to disable automatic refresh.

I don't understand why you insisting on changing the default behavior and breaking existing plugins in the process.

At least for now, the wasm still not production ready.

That's a bit of a stretch. People have been using Proxy-Wasm in Envoy in production for the last 4+ years.

I also doubt if it is good choice to add new API. I think the API should be part of the wasm plugin configuration, not for the host configuration.

It absolutely should be a new xDS API:

  1. This must be configurable on a per plugin basis (e.g. you might want to allow some plugins to clear route cache, but disable it for others).
  2. As mentioned number of times already, since this is a security feature, hosts must be able to control that.

Per offline discussion, a long-living runtime key is probably better to avoid a churn in the spec. Not refreshing routes with a custom host call is a safer option, until we get better guidance from Proxy-Wasm

To be honest, this is mostly Envoy internal change. However, the upcoming ABI v0.3 will add features (proxy-wasm/spec#55) and ability to list custom functions (proxy-wasm/spec#71), both of which could be used to let plugin know about changed behavior and addition of the custom hostcall.

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.

That's a bit of a stretch. People have been using Proxy-Wasm in Envoy in production for the last 4+ years.

I am not very sure. May Proxy-Wasm self was production ready for a while. But in the Envoy, basically this is common understanding in our inner discussion (and it is still alpha). I think our recent target is to make it production ready in the Envoy. We just fixed a very very very basic bug #36411. So I persaonlly think may it's never be widely/heavily used until now.

Don't change the default, and instead add xDS configuration option to disable automatic refresh.
I don't understand why you insisting on changing the default behavior and breaking existing plugins in the process.

Because implicitly behavior will bring lots complexity in production where multiple different plugins may be used. It require the developers know all the details or it may result in unexpected result.

So I think the correct way should be: 1) no implicitly refresh, 2) provide explict host call to do it, 3) provide additional control by the xDS if needed (mostly for security, although I think now it not a big problem, but this may be necessary in future once the wasm is widely used).

And considering current situation of wasm support of Envoy, I think this is the last timepoint to change the behavior and bring it to the expected way.

Copy link
Member Author

Choose a reason for hiding this comment

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

Of course, I also don't want to break exist plugin. So, I think we could provide different behavior for different versions' plugins.

For example, only disable the automotical refreh for plugins with ABI version >= 0.3.0. Then all exist plugins won't be effected.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am not very sure. May Proxy-Wasm self was production ready for a while. But in the Envoy, basically this is common understanding in our inner discussion (and it is still alpha). I think our recent target is to make it production ready in the Envoy. [...] So I persaonlly think may it's never be widely/heavily used until now.

AFAIK, it's "alpha" only because of the missing docs.

There are many people using it in production, but if you know better, then that's fine.

We just fixed a very very very basic bug #36411.

I've just looked at that bug and I'm a bit puzzled by it.

There were already tests for buffered and unbuffered responses, so it seems that Envoy behaves differently depending on HTTP version / codec? I'm not sure if that's desirable behavior. Also, did you check when this issue first appeared? Is it possible that it was a regression due to change from nghttp2 to oghttp2? Maybe that would explain why existing users didn't run into it yet?

Of course, I also don't want to break exist plugin. So, I think we could provide different behavior for different versions' plugins.

For example, only disable the automotical refreh for plugins with ABI version >= 0.3.0. Then all exist plugins won't be effected.

Yes, that would be more reasonable. However, I still think this should be configurable in xDS.

Copy link
Member Author

Choose a reason for hiding this comment

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

There were already tests for buffered and unbuffered responses, so it seems that Envoy behaves differently depending on HTTP version / codec? I'm not sure if that's desirable behavior. Also, did you check when this issue first appeared? Is it possible that it was a regression due to change from nghttp2 to oghttp2? Maybe that would explain why existing users didn't run into it yet?

Envoy never promise that the last chunk will be empty. Even for the HTTP/1.1, the last chunk may be modified by filters and contains data.

So, it actually is a bug. But may be the lib transferation changes the possibility of the bug occurs? Not sure.
And if you look at that fix more carefully, you may found it also fixed another simple bug: not set the response buffer correctly.

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.

Yes, that would be more reasonable. However, I still think this should be configurable in xDS.

I think we should make sure the default behavior match most cases and additional xds should only be used to provide additional control. And the ABI version also provide a way let's us to change the default behavior and without break exist plugins.

Anyway, it's hard to let everyone happy. Different people has different views.

But seems this one could be an acceptable approach for our both.

Runtime::runtimeFeatureEnabled("envoy.reloadable_features.wasm_no_automatic_route_refresh")};
};
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 = "2core",
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
39 changes: 25 additions & 14 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 Down Expand Up @@ -248,6 +249,11 @@ TEST_F(ContextTest, FindValueTest) {
}

TEST_F(ContextTest, ClearRouteCacheCalledInDownstreamConfiguration) {
TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues(
{{"envoy.reloadable_features.wasm_no_automatic_route_refresh", "false"}});

TestContext test_ctx;

Http::MockDownstreamStreamFilterCallbacks downstream_callbacks;
EXPECT_CALL(downstream_callbacks, clearRouteCache()).Times(5).WillRepeatedly(testing::Return());
Expand All @@ -256,34 +262,39 @@ 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"}};
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) {
TestScopedRuntime scoped_runtime;
scoped_runtime.mergeValues(
{{"envoy.reloadable_features.wasm_no_automatic_route_refresh", "false"}});

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
1 change: 0 additions & 1 deletion test/extensions/filters/http/wasm/wasm_filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,6 @@ TEST_P(WasmHttpFilterTest, HeadersOnlyRequestHeadersOnlyWithEnvVars) {
// Verify that route cache is cleared when modifying HTTP request headers.
NiceMock<Http::MockStreamDecoderFilterCallbacks> decoder_callbacks;
filter().setDecoderFilterCallbacks(decoder_callbacks);
EXPECT_CALL(decoder_callbacks.downstream_callbacks_, clearRouteCache()).Times(2);

Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}, {"server", "envoy"}};
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter().decodeHeaders(request_headers, true));
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