Skip to content

backport 1.17: wasm: fix getHeaderMapValue() in case of early client disconnect. (#1… #15395

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
Shikugawa merged 2 commits into from
Mar 15, 2021
Merged

backport 1.17: wasm: fix getHeaderMapValue() in case of early client disconnect. (#1… #15395

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
35c70e5
wasm: fix getHeaderMapValue() in case of early client disconnect. (#1…
PiotrSikora Mar 2, 2021
d3b9442
Merge branch 'release/v1.17' of https://github.com/envoyproxy/envoy i…
Shikugawa Mar 15, 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
21 changes: 18 additions & 3 deletions source/extensions/common/wasm/context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,19 +666,22 @@ Http::HeaderMap* Context::getMap(WasmHeaderMapType type) {
const Http::HeaderMap* Context::getConstMap(WasmHeaderMapType type) {
switch (type) {
case WasmHeaderMapType::RequestHeaders:
if (access_log_request_headers_) {
if (access_log_phase_) {
return access_log_request_headers_;
}
return request_headers_;
case WasmHeaderMapType::RequestTrailers:
if (access_log_phase_) {
return nullptr;
}
return request_trailers_;
case WasmHeaderMapType::ResponseHeaders:
if (access_log_response_headers_) {
if (access_log_phase_) {
return access_log_response_headers_;
}
return response_headers_;
case WasmHeaderMapType::ResponseTrailers:
if (access_log_response_trailers_) {
if (access_log_phase_) {
return access_log_response_trailers_;
}
return response_trailers_;
Expand Down Expand Up @@ -722,6 +725,16 @@ WasmResult Context::getHeaderMapValue(WasmHeaderMapType type, absl::string_view
absl::string_view* value) {
auto map = getConstMap(type);
if (!map) {
if (access_log_phase_) {
// Maps might point to nullptr in the access log phase.
if (wasm()->abiVersion() == proxy_wasm::AbiVersion::ProxyWasm_0_1_0) {
*value = "";
return WasmResult::Ok;
} else {
return WasmResult::NotFound;
}
}
// Requested map type is not currently available.
return WasmResult::BadArgument;
}
const Http::LowerCaseString lower_key{std::string(key)};
Expand Down Expand Up @@ -1479,6 +1492,7 @@ void Context::log(const Http::RequestHeaderMap* request_headers,
onCreate();
}

access_log_phase_ = true;
access_log_request_headers_ = request_headers;
// ? request_trailers ?
access_log_response_headers_ = response_headers;
Expand All @@ -1487,6 +1501,7 @@ void Context::log(const Http::RequestHeaderMap* request_headers,

onLog();

access_log_phase_ = false;
access_log_request_headers_ = nullptr;
// ? request_trailers ?
access_log_response_headers_ = nullptr;
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 @@ -450,6 +450,7 @@ class Context : public proxy_wasm::ContextBase,
Http::RequestHeaderMapPtr grpc_initial_metadata_;

// Access log state.
bool access_log_phase_ = false;
const StreamInfo::StreamInfo* access_log_stream_info_{};
const Http::RequestHeaderMap* access_log_request_headers_{};
const Http::ResponseHeaderMap* access_log_response_headers_{};
Expand Down
10 changes: 7 additions & 3 deletions test/extensions/filters/http/wasm/test_data/headers_rust.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,13 @@ impl HttpContext for TestStream {
}

fn on_log(&mut self) {
if let Some(path) = self.get_http_request_header(":path") {
warn!("onLog {} {}", self.context_id, path);
}
let path = self
.get_http_request_header(":path")
.unwrap_or(String::from(""));
let status = self
.get_http_response_header(":status")
.unwrap_or(String::from(""));
warn!("onLog {} {} {}", self.context_id, path, status);
}
}

Expand Down
4 changes: 3 additions & 1 deletion test/extensions/filters/http/wasm/test_data/test_cpp.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,7 +329,9 @@ void TestContext::onLog() {
auto test = root()->test_;
if (test == "headers") {
auto path = getRequestHeader(":path");
logWarn("onLog " + std::to_string(id()) + " " + std::string(path->view()));
auto status = getResponseHeader(":status");
logWarn("onLog " + std::to_string(id()) + " " + std::string(path->view()) + " " +
std::string(status->view()));
auto response_header = getResponseHeader("bogus-header");
if (response_header && response_header->view() != "") {
logWarn("response bogus-header found");
Expand Down
24 changes: 20 additions & 4 deletions 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 @@ -548,11 +548,11 @@ TEST_P(WasmHttpFilterTest, AccessLog) {
EXPECT_CALL(filter(),
log_(spdlog::level::debug, Eq(absl::string_view("onRequestHeaders 2 headers"))));
EXPECT_CALL(filter(), log_(spdlog::level::info, Eq(absl::string_view("header path /"))));
EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("onLog 2 /"))));
EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("onLog 2 / 200"))));
EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("onDone 2"))));

Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}};
Http::TestResponseHeaderMapImpl response_headers{};
Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}};
Http::TestResponseTrailerMapImpl response_trailers{};
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter().decodeHeaders(request_headers, false));
filter().continueStream(proxy_wasm::WasmStreamType::Response);
Expand All @@ -562,15 +562,31 @@ TEST_P(WasmHttpFilterTest, AccessLog) {
filter().onDestroy();
}

TEST_P(WasmHttpFilterTest, AccessLogClientDisconnected) {
setupTest("", "headers");
setupFilter();
EXPECT_CALL(filter(),
log_(spdlog::level::debug, Eq(absl::string_view("onRequestHeaders 2 headers"))));
EXPECT_CALL(filter(), log_(spdlog::level::info, Eq(absl::string_view("header path /"))));
EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("onLog 2 / "))));
EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("onDone 2"))));

Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}};
EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter().decodeHeaders(request_headers, false));
StreamInfo::MockStreamInfo log_stream_info;
filter().log(&request_headers, nullptr, nullptr, log_stream_info);
filter().onDestroy();
}

TEST_P(WasmHttpFilterTest, AccessLogCreate) {
setupTest("", "headers");
setupFilter();
EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("onLog 2 /"))));
EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("onLog 2 / 200"))));
EXPECT_CALL(filter(), log_(spdlog::level::warn, Eq(absl::string_view("onDone 2"))));

StreamInfo::MockStreamInfo log_stream_info;
Http::TestRequestHeaderMapImpl request_headers{{":path", "/"}};
Http::TestResponseHeaderMapImpl response_headers{};
Http::TestResponseHeaderMapImpl response_headers{{":status", "200"}};
Http::TestResponseTrailerMapImpl response_trailers{};
filter().log(&request_headers, &response_headers, &response_trailers, log_stream_info);
filter().onDestroy();
Expand Down