Skip to content

ext_proc: buffered mode is not sending local reply with large request #39345

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
+33 −8
Open

ext_proc: buffered mode is not sending local reply with large request #39345

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
79c7a00
ext_proc: buffered mode is not sending local reply with large request
yanjunxiang-google May 5, 2025
3c93d7c
fix format
yanjunxiang-google May 5, 2025
9807c9a
fix tests
yanjunxiang-google May 6, 2025
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
3 changes: 3 additions & 0 deletions source/extensions/filters/http/ext_proc/ext_proc.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,9 @@ FilterDataStatus Filter::onData(ProcessorState& state, Buffer::Instance& data, b
// StopIterationAndWatermark as well to stop the ActiveStream from returning error when the
// last chunk added to stream buffer exceeds the buffer limit.
state.setPaused(true);
if (state.bodyMode() == ProcessingMode::BUFFERED) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add some comment here why this special treatment. I think it makes sense in most scenarios, that in happy path, we will buffer the body anyway, so no need to wait for header-response.

But in the rare case that header-response terminates the request already: error, body overrides, we may want to do some clarification here that this branch buffers unnecessarily.

return FilterDataStatus::StopIterationAndBuffer;
}
return FilterDataStatus::StopIterationAndWatermark;
}
}
Expand Down
22 changes: 22 additions & 0 deletions test/extensions/filters/http/ext_proc/ext_proc_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5726,4 +5726,26 @@ TEST_P(ExtProcIntegrationTest, RequestHeaderModeIgnoredInModeOverrideComparison)
verifyDownstreamResponse(*response, 200);
}

TEST_P(ExtProcIntegrationTest, BufferedModeOverSizeRequestLocalReply) {
proto_config_.mutable_processing_mode()->set_request_body_mode(ProcessingMode::BUFFERED);
proto_config_.mutable_processing_mode()->set_response_header_mode(ProcessingMode::SKIP);
ConfigOptions config_option = {};
config_option.http1_codec = true;
initializeConfig(config_option);
HttpIntegrationTest::initialize();
codec_client_ = makeHttpConnection(lookupPort("http"));
Http::TestRequestHeaderMapImpl default_headers;
HttpTestUtility::addDefaultHeaders(default_headers);

// Sending a request with 4MiB bytes body.
const std::string body_sent(4096 * 1024, 's');
std::pair<Http::RequestEncoder&, IntegrationStreamDecoderPtr> encoder_decoder =
codec_client_->startRequest(default_headers);
request_encoder_ = &encoder_decoder.first;
IntegrationStreamDecoderPtr response = std::move(encoder_decoder.second);
codec_client_->sendData(*request_encoder_, body_sent, true);
// Envoy sends 413: payload_too_large local reply.
verifyDownstreamResponse(*response, 413);
}

} // namespace Envoy
12 changes: 6 additions & 6 deletions test/extensions/filters/http/ext_proc/filter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1119,16 +1119,16 @@ TEST_F(HttpFilterTest, PostAndChangeRequestBodyBufferedComesFast) {
Buffer::OwnedImpl buffered_data;
setUpDecodingBuffering(buffered_data);

EXPECT_EQ(FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(req_data_1, false));
EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(req_data_1, false));
buffered_data.add(req_data_1);

// Pretend that Envoy ignores the watermark and keep sending. It often does!
EXPECT_EQ(FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(req_data_2, false));
EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(req_data_2, false));
buffered_data.add(req_data_2);
EXPECT_EQ(FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(req_data_3, false));
EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(req_data_3, false));
buffered_data.add(req_data_3);
// when end_stream is true, we still do buffering.
EXPECT_EQ(FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(req_data_4, true));
EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(req_data_4, true));
buffered_data.add(req_data_4);

processRequestHeaders(true, absl::nullopt);
Expand Down Expand Up @@ -1184,9 +1184,9 @@ TEST_F(HttpFilterTest, PostAndChangeRequestBodyBufferedComesALittleFast) {
Buffer::OwnedImpl buffered_data;
setUpDecodingBuffering(buffered_data);

EXPECT_EQ(FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(req_data_1, false));
EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(req_data_1, false));
buffered_data.add(req_data_1);
EXPECT_EQ(FilterDataStatus::StopIterationAndWatermark, filter_->decodeData(req_data_2, false));
EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->decodeData(req_data_2, false));
buffered_data.add(req_data_2);

processRequestHeaders(true, absl::nullopt);
Expand Down
4 changes: 2 additions & 2 deletions test/extensions/filters/http/ext_proc/ordering_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ TEST_F(OrderingTest, ResponseAllDataComesFast) {
sendResponseHeaders(true);
// The rest of the data might come in even before the response headers
// response comes back.
EXPECT_EQ(FilterDataStatus::StopIterationAndWatermark, filter_->encodeData(resp_body_1, true));
EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->encodeData(resp_body_1, true));

// When the response does comes back, we should immediately send the body to the server
EXPECT_CALL(stream_delegate_, send(_, false));
Expand Down Expand Up @@ -492,7 +492,7 @@ TEST_F(OrderingTest, ResponseSomeDataComesFast) {

EXPECT_CALL(stream_delegate_, send(_, false));
sendResponseHeaders(true);
EXPECT_EQ(FilterDataStatus::StopIterationAndWatermark, filter_->encodeData(resp_body_1, false));
EXPECT_EQ(FilterDataStatus::StopIterationAndBuffer, filter_->encodeData(resp_body_1, false));
sendResponseHeadersReply();

EXPECT_CALL(stream_delegate_, send(_, false));
Expand Down