From 6d1e6a0702b26778bf1f1c6461f77e80cd3f5653 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 15 Mar 2021 18:56:29 +0000 Subject: [PATCH 1/4] http: allow any FilterHeaderStatus::Stop* after sending a local reply. Fixes #14957. Signed-off-by: Piotr Sikora --- source/common/http/filter_manager.cc | 9 ++++++--- test/integration/protocol_integration_test.cc | 8 +++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 5f8c5840e308..a513e5270d0b 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -504,9 +504,12 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead ASSERT(!(status == FilterHeadersStatus::ContinueAndDontEndStream && !(*entry)->end_stream_), "Filters should not return FilterHeadersStatus::ContinueAndDontEndStream from " "decodeHeaders when end_stream is already false"); - ENVOY_BUG( - !state_.local_complete_ || status == FilterHeadersStatus::StopIteration, - "Filters should return FilterHeadersStatus::StopIteration after sending a local reply."); + ENVOY_BUG(!state_.local_complete_ || status == FilterHeadersStatus::StopIteration || + status == FilterHeadersStatus::StopAllIterationAndBuffer || + status == FilterHeadersStatus::StopAllIterationAndWatermark, + "Filters should return " + "FilterHeadersStatus::{StopIteration,StopAllIterationAndBuffer," + "StopAllIterationAndWatermark} after sending a local reply."); state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders; ENVOY_STREAM_LOG(trace, "decode headers called: filter={} status={}", *this, diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index f7b151a3c597..084e5c166713 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -282,9 +282,11 @@ TEST_P(DownstreamProtocolIntegrationTest, ContinueAfterLocalReply) { EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); }, - "envoy bug failure: !state_.local_complete_ || status == " - "FilterHeadersStatus::StopIteration. Details: Filters should return " - "FilterHeadersStatus::StopIteration after sending a local reply."); + "envoy bug failure: !state_.local_complete_ || status == FilterHeadersStatus::StopIteration " + "|| status == FilterHeadersStatus::StopAllIterationAndBuffer || status == " + "FilterHeadersStatus::StopAllIterationAndWatermark. Details: Filters should return " + "FilterHeadersStatus::{StopIteration,StopAllIterationAndBuffer,StopAllIterationAndWatermark} " + "after sending a local reply."); } TEST_P(ProtocolIntegrationTest, AddEncodedTrailers) { From 71fba33dbdb2d5d3bcd70301452c93664c0e1f11 Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Mon, 22 Mar 2021 23:24:38 +0000 Subject: [PATCH 2/4] review: address comments from Asra. Signed-off-by: Piotr Sikora --- source/common/http/filter_manager.cc | 16 ++++++---------- test/integration/protocol_integration_test.cc | 7 ++----- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 3cebf47af6c9..9ba3e97702b2 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -507,12 +507,10 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead ASSERT(!(status == FilterHeadersStatus::ContinueAndDontEndStream && !(*entry)->end_stream_), "Filters should not return FilterHeadersStatus::ContinueAndDontEndStream from " "decodeHeaders when end_stream is already false"); - ENVOY_BUG(!state_.local_complete_ || status == FilterHeadersStatus::StopIteration || - status == FilterHeadersStatus::StopAllIterationAndBuffer || - status == FilterHeadersStatus::StopAllIterationAndWatermark, - "Filters should return " - "FilterHeadersStatus::{StopIteration,StopAllIterationAndBuffer," - "StopAllIterationAndWatermark} after sending a local reply."); + + const auto continue_iteration = (*entry)->commonHandleAfterHeadersCallback(status, end_stream); + ENVOY_BUG(!continue_iteration || !state_.local_complete_, + "Filter did not return StopAll or StopIteration after sending a local reply."); state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders; ENVOY_STREAM_LOG(trace, "decode headers called: filter={} status={}", *this, @@ -520,9 +518,6 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead (*entry)->decode_headers_called_ = true; - const auto continue_iteration = - (*entry)->commonHandleAfterHeadersCallback(status, end_stream) && !state_.local_complete_; - // If this filter ended the stream, decodeComplete() should be called for it. if ((*entry)->end_stream_) { (*entry)->handle_->decodeComplete(); @@ -541,7 +536,8 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead addDecodedData(*((*entry).get()), empty_data, true); } - if (!continue_iteration && std::next(entry) != decoder_filters_.end()) { + if ((!continue_iteration || state_.local_complete_) && + std::next(entry) != decoder_filters_.end()) { // Stop iteration IFF this is not the last filter. If it is the last filter, continue with // processing since we need to handle the case where a terminal filter wants to buffer, but // a previous filter has added body. diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index bf67c2f3bef7..48db2b6bd2c1 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -282,11 +282,8 @@ TEST_P(DownstreamProtocolIntegrationTest, ContinueAfterLocalReply) { EXPECT_TRUE(response->complete()); EXPECT_EQ("200", response->headers().getStatusValue()); }, - "envoy bug failure: !state_.local_complete_ || status == FilterHeadersStatus::StopIteration " - "|| status == FilterHeadersStatus::StopAllIterationAndBuffer || status == " - "FilterHeadersStatus::StopAllIterationAndWatermark. Details: Filters should return " - "FilterHeadersStatus::{StopIteration,StopAllIterationAndBuffer,StopAllIterationAndWatermark} " - "after sending a local reply."); + "envoy bug failure: !continue_iteration || !state_.local_complete_. " + "Details: Filter did not return StopAll or StopIteration after sending a local reply."); } TEST_P(ProtocolIntegrationTest, AddEncodedTrailers) { From 0062c858e0188113820d0f648b87dfdf129e189c Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Tue, 23 Mar 2021 01:20:23 +0000 Subject: [PATCH 3/4] reviwe: reorder to fix metadata processing. Signed-off-by: Piotr Sikora --- source/common/http/filter_manager.cc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index 9ba3e97702b2..754ff6e8f594 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -508,16 +508,16 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead "Filters should not return FilterHeadersStatus::ContinueAndDontEndStream from " "decodeHeaders when end_stream is already false"); - const auto continue_iteration = (*entry)->commonHandleAfterHeadersCallback(status, end_stream); - ENVOY_BUG(!continue_iteration || !state_.local_complete_, - "Filter did not return StopAll or StopIteration after sending a local reply."); - state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders; ENVOY_STREAM_LOG(trace, "decode headers called: filter={} status={}", *this, static_cast((*entry).get()), static_cast(status)); (*entry)->decode_headers_called_ = true; + const auto continue_iteration = (*entry)->commonHandleAfterHeadersCallback(status, end_stream); + ENVOY_BUG(!continue_iteration || !state_.local_complete_, + "Filter did not return StopAll or StopIteration after sending a local reply."); + // If this filter ended the stream, decodeComplete() should be called for it. if ((*entry)->end_stream_) { (*entry)->handle_->decodeComplete(); From be883a1b0222835fda7f2b060b06dc6de19e8faf Mon Sep 17 00:00:00 2001 From: Piotr Sikora Date: Wed, 24 Mar 2021 00:23:57 +0000 Subject: [PATCH 4/4] review: add test with headers and body. Signed-off-by: Piotr Sikora --- test/integration/protocol_integration_test.cc | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 48db2b6bd2c1..8263d0bc3aba 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -422,6 +422,28 @@ TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReply) { EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("InvalidHeaderFilter_ready\n")); } +TEST_P(DownstreamProtocolIntegrationTest, MissingHeadersLocalReplyWithBody) { + useAccessLog("%RESPONSE_CODE_DETAILS%"); + config_helper_.addFilter("{ name: invalid-header-filter, typed_config: { \"@type\": " + "type.googleapis.com/google.protobuf.Empty } }"); + initialize(); + codec_client_ = makeHttpConnection(lookupPort("http")); + + // Missing method + auto response = + codec_client_->makeRequestWithBody(Http::TestRequestHeaderMapImpl{{":method", "GET"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"remove-method", "yes"}, + {"send-reply", "yes"}}, + 1024); + response->waitForEndStream(); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().getStatusValue()); + EXPECT_THAT(waitForAccessLog(access_log_name_), HasSubstr("InvalidHeaderFilter_ready\n")); +} + // Regression test for https://github.com/envoyproxy/envoy/issues/10270 TEST_P(ProtocolIntegrationTest, LongHeaderValueWithSpaces) { // Header with at least 20kb of spaces surrounded by non-whitespace characters to ensure that