http: allow any FilterHeaderStatus::Stop* after sending a local reply. (

#15496) Fixes #14957. Risk: medium (minor filter chain refactor) Tests: new integration test Signed-off-by: Piotr Sikora <[email protected]>
envoyproxy · Mar 24, 2021 · f7bbd31 · f7bbd31
1 parent ee7918c
commit f7bbd31
Show file tree

Hide file tree

Showing 2 changed files with 29 additions and 9 deletions.
diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc
@@ -507,18 +507,16 @@ 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.");
 
     state_.filter_call_state_ &= ~FilterCallState::DecodeHeaders;
     ENVOY_STREAM_LOG(trace, "decode headers called: filter={} status={}", *this,
                      static_cast<const void*>((*entry).get()), static_cast<uint64_t>(status));
 
     (*entry)->decode_headers_called_ = true;
 
-    const auto continue_iteration =
-        (*entry)->commonHandleAfterHeadersCallback(status, end_stream) && !state_.local_complete_;
+    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_) {
@@ -538,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
@@ -288,9 +288,8 @@ 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: !continue_iteration || !state_.local_complete_. "
+      "Details: Filter did not return StopAll or StopIteration after sending a local reply.");
 }
 
 TEST_P(ProtocolIntegrationTest, AddEncodedTrailers) {
@@ -432,6 +431,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