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: stop iteration if local response sent #13873

Closed
wants to merge 1 commit into from
Closed

wasm: stop iteration if local response sent #13873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 22 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 @@ -1568,6 +1568,7 @@ WasmResult Context::sendLocalResponse(uint32_t response_code, absl::string_view
details = std::string(details)] {
decoder_callbacks_->sendLocalReply(static_cast<Envoy::Http::Code>(response_code), body_text,
modify_headers, grpc_status, details);
http_local_response_sent_ = true;
});
}
return WasmResult::Ok;
Expand All @@ -1579,9 +1580,12 @@ Http::FilterHeadersStatus Context::decodeHeaders(Http::RequestHeaderMap& headers
request_headers_ = &headers;
end_of_stream_ = end_stream;
auto result = convertFilterHeadersStatus(onRequestHeaders(headerSize(&headers), end_stream));
if (result == Http::FilterHeadersStatus::Continue) {
if (http_local_response_sent_) {
return Http::FilterHeadersStatus::StopIteration;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move the override into convertFilter{Headers,Data,Trailers}Status() calls instead of doing it inline?

} else if (result == Http::FilterHeadersStatus::Continue) {
request_headers_ = nullptr;
}

return result;
}

Expand All @@ -1594,6 +1598,9 @@ Http::FilterDataStatus Context::decodeData(::Envoy::Buffer::Instance& data, bool
const auto buffer = getBuffer(WasmBufferType::HttpRequestBody);
const auto buffer_size = (buffer == nullptr) ? 0 : buffer->size();
auto result = convertFilterDataStatus(onRequestBody(buffer_size, end_stream));
if (http_local_response_sent_) {
return Http::FilterDataStatus::StopIterationNoBuffer;
}
buffering_request_body_ = false;
switch (result) {
case Http::FilterDataStatus::Continue:
Expand All @@ -1615,7 +1622,9 @@ Http::FilterTrailersStatus Context::decodeTrailers(Http::RequestTrailerMap& trai
}
request_trailers_ = &trailers;
auto result = convertFilterTrailersStatus(onRequestTrailers(headerSize(&trailers)));
if (result == Http::FilterTrailersStatus::Continue) {
if (http_local_response_sent_) {
return Http::FilterTrailersStatus::StopIteration;
} else if (result == Http::FilterTrailersStatus::Continue) {
request_trailers_ = nullptr;
}
return result;
Expand Down Expand Up @@ -1649,7 +1658,9 @@ Http::FilterHeadersStatus Context::encodeHeaders(Http::ResponseHeaderMap& header
response_headers_ = &headers;
end_of_stream_ = end_stream;
auto result = convertFilterHeadersStatus(onResponseHeaders(headerSize(&headers), end_stream));
if (result == Http::FilterHeadersStatus::Continue) {
if (http_local_response_sent_) {
return Http::FilterHeadersStatus::StopIteration;
} else if (result == Http::FilterHeadersStatus::Continue) {
response_headers_ = nullptr;
}
return result;
Expand All @@ -1664,6 +1675,10 @@ Http::FilterDataStatus Context::encodeData(::Envoy::Buffer::Instance& data, bool
const auto buffer = getBuffer(WasmBufferType::HttpResponseBody);
const auto buffer_size = (buffer == nullptr) ? 0 : buffer->size();
auto result = convertFilterDataStatus(onResponseBody(buffer_size, end_stream));
if (http_local_response_sent_) {
return Http::FilterDataStatus::StopIterationNoBuffer;
}

buffering_response_body_ = false;
switch (result) {
case Http::FilterDataStatus::Continue:
Expand All @@ -1685,9 +1700,13 @@ Http::FilterTrailersStatus Context::encodeTrailers(Http::ResponseTrailerMap& tra
}
response_trailers_ = &trailers;
auto result = convertFilterTrailersStatus(onResponseTrailers(headerSize(&trailers)));
if (http_local_response_sent_) {
return Http::FilterTrailersStatus::StopIteration;
}
if (result == Http::FilterTrailersStatus::Continue) {
response_trailers_ = nullptr;
}

return result;
}

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 @@ -425,6 +425,7 @@ class Context : public proxy_wasm::ContextBase,

// HTTP filter state.
bool http_request_started_ = false; // When decodeHeaders() is called the request is "started".
bool http_local_response_sent_ = false; // indicates if the local response is sent
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the only case when this can happen or should we call it something more general?

Http::RequestHeaderMap* request_headers_{};
Http::ResponseHeaderMap* response_headers_{};
::Envoy::Buffer::Instance* request_body_buffer_{};
Expand Down