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: Force stop iteration after local response is sent #13930

Merged
merged 7 commits into from
Nov 11, 2020
Merged

wasm: Force stop iteration after local response is sent #13930

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bd4423c
wasm: stop iteration when local reply sent
mathetake Nov 7, 2020
4ed88b7
update proxy-wasm-cpph-host repo location
mathetake Nov 9, 2020
2de4fba
Update proxy-wasm-cpp-host
mathetake Nov 10, 2020
7f8eb32
Update proxy-wasm-cpp-host
mathetake Nov 10, 2020
300440f
Update proxy-wasm-cpp-host
mathetake Nov 10, 2020
c4f64e1
Update proxy-wasm-cpp-host: point to origin
mathetake Nov 10, 2020
9977516
Update release date of proxy-wasm-cpp-host
mathetake Nov 10, 2020
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
4 changes: 2 additions & 2 deletions bazel/repository_locations.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -830,8 +830,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 = "40fd3d03842c07d65fed907a6b6ed0f89d68d531",
sha256 = "b5ae746e66b6209ea0cce86d6c21de99dacbec1da9cdadd53a9ec46bc296a3ba",
version = "b7d3d13d75bb6b50f192252258bb9583bf723fa4",
sha256 = "ae639f94a80adbe915849bccb32346720c59a579db09918e44aefafed6cbb100",
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"],
PiotrSikora marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
3 changes: 2 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 @@ -1436,7 +1436,8 @@ TEST_P(WasmHttpFilterTest, RootId2) {
setupFilter("context2");
EXPECT_CALL(filter(), log_(spdlog::level::debug, Eq(absl::string_view("onRequestHeaders2 2"))));
Http::TestRequestHeaderMapImpl request_headers;
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter().decodeHeaders(request_headers, true));
EXPECT_EQ(Http::FilterHeadersStatus::StopAllIterationAndWatermark,
filter().decodeHeaders(request_headers, true));
Copy link
Contributor

@antoniovicente antoniovicente Nov 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Given that the filter code is in a different repo, does it make sense to have assertions like this one about the return values of decodeHeaders in the main repo? It doesn't seem right to me. This PR can go in anyway, but we should rethink how this works going forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

The test filter is in this repo, this is the test case for which the behavior changed:

envoy/test/extensions/filters/http/wasm/test_data/test_cpp.cc

Lines 750 to 754 in 1d6d68b

FilterHeadersStatus Context2::onRequestHeaders(uint32_t, bool) {
logDebug(std::string("onRequestHeaders2 ") + std::to_string(id()));
CHECK_RESULT(sendLocalResponse(200, "ok", "body", {{"foo", "bar"}}));
return FilterHeadersStatus::Continue;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

So the coding error was in a wasm filter implementation, not in the wasm framework itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this protects against coding errors in user-supplied Wasm filters (i.e. FilterHeadersStatus::Continue returned after sendLocalResponse() above).

}

} // namespace Wasm
Expand Down