diff --git a/source/common/http/filter_manager.cc b/source/common/http/filter_manager.cc index be2c9b7af44fb..d5ea8aa7a221d 100644 --- a/source/common/http/filter_manager.cc +++ b/source/common/http/filter_manager.cc @@ -1699,6 +1699,10 @@ bool ActiveStreamDecoderFilter::recreateStream(const ResponseHeaderMap* headers) return false; } + parent_.state_.decoder_filter_chain_aborted_ = true; + parent_.state_.encoder_filter_chain_aborted_ = true; + parent_.state_.recreated_stream_ = true; + parent_.streamInfo().setResponseCodeDetails( StreamInfo::ResponseCodeDetails::get().InternalRedirect); @@ -1765,6 +1769,12 @@ void ActiveStreamEncoderFilter::drainSavedResponseMetadata() { } void ActiveStreamEncoderFilter::handleMetadataAfterHeadersCallback() { + if (parent_.state_.recreated_stream_) { + // The stream has been recreated. In this case, there's no reason to encode saved metadata. + getSavedResponseMetadata()->clear(); + return; + } + // If we drain accumulated metadata, the iteration must start with the current filter. const bool saved_state = iterate_from_current_filter_; iterate_from_current_filter_ = true; diff --git a/source/common/http/filter_manager.h b/source/common/http/filter_manager.h index 82405f0cc4b2c..0e4a3cc680809 100644 --- a/source/common/http/filter_manager.h +++ b/source/common/http/filter_manager.h @@ -890,7 +890,8 @@ class FilterManager : public ScopeTrackedObject, has_1xx_headers_(false), created_filter_chain_(false), is_head_request_(false), is_grpc_request_(false), non_100_response_headers_encoded_(false), under_on_local_reply_(false), decoder_filter_chain_aborted_(false), - encoder_filter_chain_aborted_(false), saw_downstream_reset_(false) {} + encoder_filter_chain_aborted_(false), saw_downstream_reset_(false), + recreated_stream_(false) {} uint32_t filter_call_state_{0}; // Set after decoder filter chain has completed iteration. Prevents further calls to decoder @@ -928,6 +929,8 @@ class FilterManager : public ScopeTrackedObject, bool decoder_filter_chain_aborted_ : 1; bool encoder_filter_chain_aborted_ : 1; bool saw_downstream_reset_ : 1; + // True when the stream was recreated. + bool recreated_stream_ : 1; // The following 3 members are booleans rather than part of the space-saving bitfield as they // are passed as arguments to functions expecting bools. Extend State using the bitfield diff --git a/test/integration/BUILD b/test/integration/BUILD index fdfa59a6cf62f..32c769067243b 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -770,12 +770,14 @@ envoy_cc_test( "//source/common/http:header_map_lib", "//source/extensions/filters/http/buffer:config", "//test/integration/filters:add_body_filter_config_lib", + "//test/integration/filters:add_encode_metadata_filter_lib", "//test/integration/filters:add_invalid_data_filter_lib", "//test/integration/filters:assert_non_reentrant_filter_lib", "//test/integration/filters:buffer_continue_filter_lib", "//test/integration/filters:continue_after_local_reply_filter_lib", "//test/integration/filters:continue_headers_only_inject_body", "//test/integration/filters:encoder_decoder_buffer_filter_lib", + "//test/integration/filters:encoder_recreate_stream_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", diff --git a/test/integration/filter_integration_test.cc b/test/integration/filter_integration_test.cc index e83a9d231188c..5028b5eca5254 100644 --- a/test/integration/filter_integration_test.cc +++ b/test/integration/filter_integration_test.cc @@ -1575,5 +1575,85 @@ TEST_P(FilterIntegrationTest, FilterAddsDataToHeaderOnlyRequestWithIndependentHa testFilterAddsDataAndTrailersToHeaderOnlyRequest(); } +// Add metadata in the first filter before recreate the stream in the second filter, +// on response path. +TEST_P(FilterIntegrationTest, RecreateStreamAfterEncodeMetadata) { + // recreateStream is not supported in Upstream filter chain. + if (!testing_downstream_filter_) { + return; + } + + prependFilter("{ name: add-metadata-encode-headers-filter }"); + prependFilter("{ name: encoder-recreate-stream-filter }"); + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.mutable_http2_protocol_options()->set_allow_metadata(true); }); + + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + waitForNextUpstreamRequest(); + upstream_request_->encodeHeaders(default_response_headers_, true); + + // Second upstream request is triggered by recreateStream. + FakeStreamPtr upstream_request_2; + // Wait for the next stream on the upstream connection. + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_2)); + // Wait for the stream to be completely received. + ASSERT_TRUE(upstream_request_2->waitForEndStream(*dispatcher_)); + upstream_request_2->encodeHeaders(default_response_headers_, true); + + // Wait for the response to be completely received. + ASSERT_TRUE(response->waitForEndStream()); + ASSERT_TRUE(response->complete()); + + // Verify the metadata is received. + std::set expected_metadata_keys = {"headers", "duplicate"}; + EXPECT_EQ(response->metadataMap().size(), expected_metadata_keys.size()); + for (const auto& key : expected_metadata_keys) { + // keys are the same as their corresponding values. + auto it = response->metadataMap().find(key); + ASSERT_FALSE(it == response->metadataMap().end()) << "key: " << key; + EXPECT_EQ(response->metadataMap().find(key)->second, key); + } +} + +// Add metadata in the first filter on local reply path. +TEST_P(FilterIntegrationTest, EncodeMetadataOnLocalReply) { + // Local replies are not seen by upstream HTTP filters. add-metadata-encode-headers-filter will + // not be invoked if it is installed in upstream filter chain. + // Thus, this test is only applicable to downstream filter chain. + if (!testing_downstream_filter_) { + return; + } + + prependFilter("{ name: local-reply-during-decode }"); + prependFilter("{ name: add-metadata-encode-headers-filter }"); + + config_helper_.addConfigModifier( + [&](envoy::extensions::filters::network::http_connection_manager::v3::HttpConnectionManager& + hcm) -> void { hcm.mutable_http2_protocol_options()->set_allow_metadata(true); }); + + initialize(); + + codec_client_ = makeHttpConnection(lookupPort("http")); + auto response = codec_client_->makeHeaderOnlyRequest(default_request_headers_); + + ASSERT_TRUE(response->waitForEndStream()); + EXPECT_TRUE(response->complete()); + EXPECT_EQ("500", response->headers().getStatusValue()); + + // Verify the metadata is received. + std::set expected_metadata_keys = {"headers", "duplicate"}; + EXPECT_EQ(response->metadataMap().size(), expected_metadata_keys.size()); + for (const auto& key : expected_metadata_keys) { + // keys are the same as their corresponding values. + auto it = response->metadataMap().find(key); + ASSERT_FALSE(it == response->metadataMap().end()) << "key: " << key; + EXPECT_EQ(response->metadataMap().find(key)->second, key); + } +} + } // namespace } // namespace Envoy diff --git a/test/integration/filters/BUILD b/test/integration/filters/BUILD index dfebb8f118cf7..819acf97b26eb 100644 --- a/test/integration/filters/BUILD +++ b/test/integration/filters/BUILD @@ -1013,3 +1013,36 @@ envoy_cc_test_library( "//test/extensions/filters/http/common:empty_http_filter_config_lib", ], ) + +envoy_cc_test_library( + name = "add_encode_metadata_filter_lib", + srcs = [ + "add_encode_metadata_filter.cc", + ], + deps = [ + ":common_lib", + "//envoy/event:timer_interface", + "//envoy/http:filter_interface", + "//envoy/registry", + "//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 = "encoder_recreate_stream_filter_lib", + srcs = [ + "encoder_recreate_stream_filter.cc", + ], + deps = [ + ":common_lib", + "//envoy/event:timer_interface", + "//envoy/http:filter_interface", + "//envoy/registry", + "//envoy/server:filter_config_interface", + "//source/common/router:string_accessor_lib", + "//source/extensions/filters/http/common:pass_through_filter_lib", + "//test/extensions/filters/http/common:empty_http_filter_config_lib", + ], +) diff --git a/test/integration/filters/add_encode_metadata_filter.cc b/test/integration/filters/add_encode_metadata_filter.cc new file mode 100644 index 0000000000000..4a840e2caaa53 --- /dev/null +++ b/test/integration/filters/add_encode_metadata_filter.cc @@ -0,0 +1,41 @@ +#include +#include + +#include "envoy/event/timer.h" +#include "envoy/http/filter.h" +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "source/common/buffer/buffer_impl.h" +#include "source/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 add encoded metadata in encodeHeaders. +class AddEncodeMetadataFilter : public Http::PassThroughFilter { +public: + constexpr static char name[] = "add-metadata-encode-headers-filter"; + + Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap&, bool) override { + Http::MetadataMap metadata_map = {{"headers", "headers"}, {"duplicate", "duplicate"}}; + Http::MetadataMapPtr metadata_map_ptr = std::make_unique(metadata_map); + encoder_callbacks_->addEncodedMetadata(std::move(metadata_map_ptr)); + return Http::FilterHeadersStatus::Continue; + } + + Http::FilterDataStatus encodeData(Buffer::Instance&, bool) override { + return Http::FilterDataStatus::Continue; + } +}; + +constexpr char AddEncodeMetadataFilter::name[]; +static Registry::RegisterFactory, + Server::Configuration::NamedHttpFilterConfigFactory> + register_; + +} // namespace Envoy \ No newline at end of file diff --git a/test/integration/filters/encoder_recreate_stream_filter.cc b/test/integration/filters/encoder_recreate_stream_filter.cc new file mode 100644 index 0000000000000..e1d2e16014a43 --- /dev/null +++ b/test/integration/filters/encoder_recreate_stream_filter.cc @@ -0,0 +1,55 @@ +#include +#include + +#include "envoy/event/timer.h" +#include "envoy/http/filter.h" +#include "envoy/registry/registry.h" +#include "envoy/server/filter_config.h" + +#include "source/common/buffer/buffer_impl.h" +#include "source/extensions/filters/http/common/pass_through_filter.h" + +#include "test/extensions/filters/http/common/empty_http_filter_config.h" +#include "source/common/router/string_accessor_impl.h" +#include "test/integration/filters/common.h" + +#include "gtest/gtest.h" + +namespace Envoy { + +class EncoderRecreateStreamFilter : public Http::PassThroughFilter { +public: + constexpr static char name[] = "encoder-recreate-stream-filter"; + + Http::FilterHeadersStatus encodeHeaders(Http::ResponseHeaderMap&, bool) override { + const auto* filter_state = + decoder_callbacks_->streamInfo().filterState()->getDataReadOnly( + "test_key"); + + if (filter_state != nullptr) { + return ::Envoy::Http::FilterHeadersStatus::Continue; + } + + decoder_callbacks_->streamInfo().filterState()->setData( + "test_key", std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::Request); + + if (decoder_callbacks_->recreateStream(nullptr)) { + return ::Envoy::Http::FilterHeadersStatus::StopIteration; + } + + return ::Envoy::Http::FilterHeadersStatus::Continue; + } + + void setDecoderFilterCallbacks(Http::StreamDecoderFilterCallbacks& callbacks) override { + decoder_callbacks_ = &callbacks; + } +}; + +// perform static registration +constexpr char EncoderRecreateStreamFilter::name[]; +static Registry::RegisterFactory, + Server::Configuration::NamedHttpFilterConfigFactory> + register_; + +} // namespace Envoy diff --git a/tools/base/requirements.in b/tools/base/requirements.in index 608947efed3f7..1b5d79db1099a 100644 --- a/tools/base/requirements.in +++ b/tools/base/requirements.in @@ -6,7 +6,7 @@ aiodocker>=0.22.2 aiohttp>=3.8.1 aioquic>=0.9.21 cffi>=1.15.0 -clang-format==14.0.6 +clang-format==19.1.1 clang-tidy==14.0.6 colorama coloredlogs diff --git a/tools/base/requirements.txt b/tools/base/requirements.txt index 7fbe642f7f6a6..2409a03d2b90b 100644 --- a/tools/base/requirements.txt +++ b/tools/base/requirements.txt @@ -422,16 +422,22 @@ charset-normalizer==3.3.2 \ --hash=sha256:fd1abc0d89e30cc4e02e4064dc67fcc51bd941eb395c502aac3ec19fab46b519 \ --hash=sha256:ff8fa367d09b717b2a17a052544193ad76cd49979c805768879cb63d9ca50561 # via requests -clang-format==14.0.6 \ - --hash=sha256:13f2d6d4a2af004a783c65f0921afa8f0384bffcdaf500b6c2cb542edeb0b4a5 \ - --hash=sha256:810c649ab97d208cd418c897d50ab6e958eb8d96854527edd80d0dd21a75e914 \ - --hash=sha256:aaf4edecc46a24f0b572b82cf5827e292ad1c137903427627c4d5f671668cc2b \ - --hash=sha256:bd400c47665dd19afc03f98e747f78ed828abab99c6a1b07e137b35c1cd3cc26 \ - --hash=sha256:c93580945f75de7e01996f1fb3cf67e4dc424f1c864e237c85614fb99a48c7a4 \ - --hash=sha256:d5c96b500d7f8b5d2db5b75ac035be387512850ad589cdc3019666b861382136 \ - --hash=sha256:d780c04334bca80f2b60d25bf53c37bd0618520ee295a7888a11f25bde114ac4 \ - --hash=sha256:d7c1c5e404c58e55f0170f01b3c5611dce6c119e62b5d1020347e0ad97d5a047 \ - --hash=sha256:dbfd60528eb3bb7d7cfe8576faa70845fbf93601f815ef75163d36606e87f388 +clang-format==19.1.1 \ + --hash=sha256:0aa0b5faf59224591b72e90284e28a6054c6bf78869fb9b49f8fbf5da40fcfbf \ + --hash=sha256:28f37d5ddd799a0e62dfdad5f13cb065103d89a71a0b6e9f92d0282dd7055d4c \ + --hash=sha256:380c724ae53658ad51b0f6a2bd88677ac051cf5d3d55c7874ba2c0bed3cfe1e9 \ + --hash=sha256:39bca9a96498c674cc765d572ed299e80373e3c33f73961786864bd516b1eeb7 \ + --hash=sha256:3e9644587d44f5700e5b5c668ffa81b20575c83e971e07f5e512aae421cde387 \ + --hash=sha256:4d6a533ff6def4b5729bfa4edaaa1015c7058ed218b4e0280c980c4ec5bdd47c \ + --hash=sha256:631aa55a56f96f12d9c6060fb6d09f397f32705c7a4331e1944a107f6c99f139 \ + --hash=sha256:656460de9afadb918261da115bb3ccf8023da2729c27670a7eadb1e1225c65a8 \ + --hash=sha256:6e67c7aa81cea8161db3d3654331103225104ad7622257b4f4e00f8563aced52 \ + --hash=sha256:7dc424c87b53d6097e46df034a5359ed9d1932ec5dccd0ab42f1ef3655a7452f \ + --hash=sha256:8e571b591fd42afa6974a79cc162ac4946bcf4e0050f4f46ecb232b90526e540 \ + --hash=sha256:aeb3a1898ee86689602d583da3dcbd2f6cdf460538c2d7a3623a5012132ada1b \ + --hash=sha256:c4218faed21065d28d33420e756a9ef74cb60b53d80c101fd1770f2ac2198492 \ + --hash=sha256:d4ae63766cffcff4a55640b92210b0bdc0d7d9a5d9d9e0a8a998e532c204017a \ + --hash=sha256:e4967d43c986ce5987d293705ed5b76f44db71fdd111ea365dcaabc7a3c4f237 # via -r requirements.in clang-tidy==14.0.6 \ --hash=sha256:02bce40a56cc344e20d2f63bef6b85acf9837954559e0091804d6e748dfc0359 \