From 27e15638c45a0c4f1a4b02ccbd4179e2abc9feed Mon Sep 17 00:00:00 2001 From: Yiming Jin Date: Fri, 23 Apr 2021 14:36:50 -0700 Subject: [PATCH 1/3] Skip metadata processing after sending local reply Signed-off-by: Yiming Jin --- source/common/http/filter_manager.cc | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index b03a9694986e..1bd7dd45f6d7 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -529,6 +529,12 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead (*entry)->handle_->decodeComplete(); } + // Skip processing metadata after sending local reply + if (state_.local_complete_ && std::next(entry) != decoder_filters_.end()) { + maybeContinueDecoding(continue_data_entry); + return; + } + const bool new_metadata_added = processNewlyAddedMetadata(); // If end_stream is set in headers, and a filter adds new metadata, we need to delay end_stream // in headers by inserting an empty data frame with end_stream set. The empty data frame is sent @@ -542,8 +548,7 @@ void FilterManager::decodeHeaders(ActiveStreamDecoderFilter* filter, RequestHead addDecodedData(*((*entry).get()), empty_data, true); } - if ((!continue_iteration || state_.local_complete_) && - std::next(entry) != decoder_filters_.end()) { + if (!continue_iteration && 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. From 2373b0a2eeaa40b6921cb8352d589b11890ff97f Mon Sep 17 00:00:00 2001 From: Yiming Jin Date: Mon, 3 May 2021 22:11:18 -0700 Subject: [PATCH 2/3] Add unit test to verify that decodeMetadata is not run after local reply. Signed-off-by: Yiming Jin --- test/integration/BUILD | 1 + test/integration/filters/BUILD | 15 +++++++ ..._after_local_reply_with_metadata_filter.cc | 40 +++++++++++++++++++ test/integration/protocol_integration_test.cc | 25 ++++++++++++ 4 files changed, 81 insertions(+) create mode 100644 test/integration/filters/continue_after_local_reply_with_metadata_filter.cc diff --git a/test/integration/BUILD b/test/integration/BUILD index 014e08b26d0e..77a2cb0af2ac 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -447,6 +447,7 @@ envoy_cc_test_library( "//source/extensions/filters/http/health_check:config", "//test/common/http/http2:http2_frame", "//test/integration/filters:continue_after_local_reply_filter_lib", + "//test/integration/filters:continue_after_local_reply_filter_with_metadata_lib", "//test/integration/filters:continue_headers_only_inject_body", "//test/integration/filters:encoder_decoder_buffer_filter_lib", "//test/integration/filters:invalid_header_filter_lib", diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 4722b15a3730..6aaaf8415dd5 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -69,6 +69,21 @@ envoy_cc_test_library( ], ) +envoy_cc_test_library( + name = "continue_after_local_reply_filter_with_metadata_lib", + srcs = [ + "continue_after_local_reply_with_metadata_filter.cc", + ], + deps = [ + ":common_lib", + "//include/envoy/http:filter_interface", + "//include/envoy/registry", + "//include/envoy/server:filter_config_interface", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "//test/extensions/filters/http/common:empty_http_filter_config_lib", + ], +) + envoy_cc_test_library( name = "continue_headers_only_inject_body", srcs = [ diff --git a/test/integration/filters/continue_after_local_reply_with_metadata_filter.cc b/test/integration/filters/continue_after_local_reply_with_metadata_filter.cc new file mode 100644 index 000000000000..7ce9da964ab4 --- /dev/null +++ b/test/integration/filters/continue_after_local_reply_with_metadata_filter.cc @@ -0,0 +1,40 @@ +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "extensions/filters/http/common/pass_through_filter.h" + +#include "test/extensions/filters/http/common/empty_http_filter_config.h" +#include "test/integration/filters/common.h" + +#include "gtest/gtest.h" + +namespace Envoy { + +// A filter that calls Http::FilterHeadersStatus::Continue and set metadata after a local reply. +class ContinueAfterLocalReplyWithMetadataFilter : public Http::PassThroughFilter { +public: + constexpr static char name[] = "continue-after-local-reply-with-metadata-filter"; + + Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override { + Http::MetadataMap metadata_map = {{"headers", "headers"}, {"duplicate", "duplicate"}}; + Http::MetadataMapPtr metadata_map_ptr = std::make_unique(metadata_map); + decoder_callbacks_->addDecodedMetadata().emplace_back(std::move(metadata_map_ptr)); + decoder_callbacks_->sendLocalReply(Envoy::Http::Code::OK, "", nullptr, absl::nullopt, + "ContinueAfterLocalReplyFilter is ready"); + return Http::FilterHeadersStatus::Continue; + } + + // Adds new metadata to metadata_map directly + Http::FilterMetadataStatus decodeMetadata(Http::MetadataMap& metadata_map) override { + metadata_map["data"] = "data"; + throw EnvoyException("Should not call decode metadata after local reply."); + return Http::FilterMetadataStatus::Continue; + } +}; + +constexpr char ContinueAfterLocalReplyWithMetadataFilter::name[]; +static Registry::RegisterFactory, + Server::Configuration::NamedHttpFilterConfigFactory> + register_; + +} // namespace Envoy diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index b8ab19fda6c9..58a787bd71a5 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -2314,4 +2314,29 @@ TEST_P(DownstreamProtocolIntegrationTest, HeaderNormalizationRejection) { EXPECT_EQ("400", response->headers().getStatusValue()); } +// Tests a filter that returns a FilterHeadersStatus::Continue after a local reply without +// processing new metadata generated in decodeHeader +TEST_P(DownstreamProtocolIntegrationTest, ContinueAfterLocalReplyWithMetadata) { + config_helper_.addFilter(R"EOF( + name: continue-after-local-reply-with-metadata-filter + typed_config: + "@type": type.googleapis.com/google.protobuf.Empty + )EOF"); + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + // Send a headers only request. + IntegrationStreamDecoderPtr response; + EXPECT_ENVOY_BUG( + { + response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + ASSERT_EQ("200", response->headers().getStatusValue()); + ENVOY_LOG(info, "test upstream request"); + }, + "envoy bug failure: !continue_iteration || !state_.local_complete_. " + "Details: Filter did not return StopAll or StopIteration after sending a local reply."); +} + } // namespace Envoy From 9b1039410c4dc2cf9a3095253d09e17ad908d070 Mon Sep 17 00:00:00 2001 From: Yiming Jin Date: Sat, 8 May 2021 17:42:34 -0700 Subject: [PATCH 3/3] Address review comment for unit test Signed-off-by: Yiming Jin --- test/integration/BUILD | 2 +- test/integration/filters/BUILD | 4 ++-- ...cc => local_reply_with_metadata_filter.cc} | 16 ++++++++-------- test/integration/protocol_integration_test.cc | 19 ++++++------------- 4 files changed, 17 insertions(+), 24 deletions(-) rename test/integration/filters/{continue_after_local_reply_with_metadata_filter.cc => local_reply_with_metadata_filter.cc} (63%) diff --git a/test/integration/BUILD b/test/integration/BUILD index 77a2cb0af2ac..644bb1127a5f 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -447,12 +447,12 @@ envoy_cc_test_library( "//source/extensions/filters/http/health_check:config", "//test/common/http/http2:http2_frame", "//test/integration/filters:continue_after_local_reply_filter_lib", - "//test/integration/filters:continue_after_local_reply_filter_with_metadata_lib", "//test/integration/filters:continue_headers_only_inject_body", "//test/integration/filters:encoder_decoder_buffer_filter_lib", "//test/integration/filters:invalid_header_filter_lib", "//test/integration/filters:local_reply_during_encoding_data_filter_lib", "//test/integration/filters:local_reply_during_encoding_filter_lib", + "//test/integration/filters:local_reply_with_metadata_filter_lib", "//test/integration/filters:random_pause_filter_lib", "//test/test_common:logging_lib", "//test/test_common:utility_lib", diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index 6aaaf8415dd5..04e8b0abd67c 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -70,9 +70,9 @@ envoy_cc_test_library( ) envoy_cc_test_library( - name = "continue_after_local_reply_filter_with_metadata_lib", + name = "local_reply_with_metadata_filter_lib", srcs = [ - "continue_after_local_reply_with_metadata_filter.cc", + "local_reply_with_metadata_filter.cc", ], deps = [ ":common_lib", diff --git a/test/integration/filters/continue_after_local_reply_with_metadata_filter.cc b/test/integration/filters/local_reply_with_metadata_filter.cc similarity index 63% rename from test/integration/filters/continue_after_local_reply_with_metadata_filter.cc rename to test/integration/filters/local_reply_with_metadata_filter.cc index 7ce9da964ab4..07580be29c6a 100644 --- a/test/integration/filters/continue_after_local_reply_with_metadata_filter.cc +++ b/test/integration/filters/local_reply_with_metadata_filter.cc @@ -10,30 +10,30 @@ namespace Envoy { -// A filter that calls Http::FilterHeadersStatus::Continue and set metadata after a local reply. -class ContinueAfterLocalReplyWithMetadataFilter : public Http::PassThroughFilter { +// A filter that calls Http::FilterHeadersStatus::StopAll and set metadata after a local reply. +class LocalReplyWithMetadataFilter : public Http::PassThroughFilter { public: - constexpr static char name[] = "continue-after-local-reply-with-metadata-filter"; + constexpr static char name[] = "local-reply-with-metadata-filter"; Http::FilterHeadersStatus decodeHeaders(Http::RequestHeaderMap&, bool) override { Http::MetadataMap metadata_map = {{"headers", "headers"}, {"duplicate", "duplicate"}}; Http::MetadataMapPtr metadata_map_ptr = std::make_unique(metadata_map); decoder_callbacks_->addDecodedMetadata().emplace_back(std::move(metadata_map_ptr)); decoder_callbacks_->sendLocalReply(Envoy::Http::Code::OK, "", nullptr, absl::nullopt, - "ContinueAfterLocalReplyFilter is ready"); - return Http::FilterHeadersStatus::Continue; + "LocalReplyWithMetadataFilter is ready"); + return Http::FilterHeadersStatus::StopIteration; } // Adds new metadata to metadata_map directly Http::FilterMetadataStatus decodeMetadata(Http::MetadataMap& metadata_map) override { metadata_map["data"] = "data"; - throw EnvoyException("Should not call decode metadata after local reply."); + ASSERT(false); return Http::FilterMetadataStatus::Continue; } }; -constexpr char ContinueAfterLocalReplyWithMetadataFilter::name[]; -static Registry::RegisterFactory, +constexpr char LocalReplyWithMetadataFilter::name[]; +static Registry::RegisterFactory, Server::Configuration::NamedHttpFilterConfigFactory> register_; diff --git a/test/integration/protocol_integration_test.cc b/test/integration/protocol_integration_test.cc index 58a787bd71a5..3c799a0566a5 100644 --- a/test/integration/protocol_integration_test.cc +++ b/test/integration/protocol_integration_test.cc @@ -2316,9 +2316,9 @@ TEST_P(DownstreamProtocolIntegrationTest, HeaderNormalizationRejection) { // Tests a filter that returns a FilterHeadersStatus::Continue after a local reply without // processing new metadata generated in decodeHeader -TEST_P(DownstreamProtocolIntegrationTest, ContinueAfterLocalReplyWithMetadata) { +TEST_P(DownstreamProtocolIntegrationTest, LocalReplyWithMetadata) { config_helper_.addFilter(R"EOF( - name: continue-after-local-reply-with-metadata-filter + name: local-reply-with-metadata-filter typed_config: "@type": type.googleapis.com/google.protobuf.Empty )EOF"); @@ -2326,17 +2326,10 @@ TEST_P(DownstreamProtocolIntegrationTest, ContinueAfterLocalReplyWithMetadata) { codec_client_ = makeHttpConnection(lookupPort("http")); // Send a headers only request. - IntegrationStreamDecoderPtr response; - EXPECT_ENVOY_BUG( - { - response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); - ASSERT_TRUE(response->waitForEndStream()); - ASSERT_TRUE(response->complete()); - ASSERT_EQ("200", response->headers().getStatusValue()); - ENVOY_LOG(info, "test upstream request"); - }, - "envoy bug failure: !continue_iteration || !state_.local_complete_. " - "Details: Filter did not return StopAll or StopIteration after sending a local reply."); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + ASSERT_EQ("200", response->headers().getStatusValue()); } } // namespace Envoy