diff --git a/include/envoy/stream_info/filter_state.h b/include/envoy/stream_info/filter_state.h index 0e41d967cf19f..4fe1aeb34b854 100644 --- a/include/envoy/stream_info/filter_state.h +++ b/include/envoy/stream_info/filter_state.h @@ -30,21 +30,16 @@ class FilterState { // // - FilterChain has the shortest life span, which is as long as the filter chain lives. // - // - DownstreamRequest is longer than FilterChain. When internal redirect is enabled, one - // downstream request may create multiple filter chains. DownstreamRequest allows an object to - // survive across filter chains for bookkeeping needs. + // - Request is longer than FilterChain. When internal redirect is enabled, one + // downstream request may create multiple filter chains. Request allows an object to + // survive across filter chains for bookkeeping needs. This is not used for the upstream case. // - // - DownstreamConnection makes an object survive the entire duration of a downstream connection. + // - Connection makes an object survive the entire duration of a connection. // Any stream within this connection can see the same object. // // Note that order matters in this enum because it's assumed that life span grows as enum number // grows. - enum LifeSpan { - FilterChain, - DownstreamRequest, - DownstreamConnection, - TopSpan = DownstreamConnection - }; + enum LifeSpan { FilterChain, Request, Connection, TopSpan = Connection }; class Object { public: @@ -65,7 +60,7 @@ class FilterState { * @param data an owning pointer to the data to be stored. * @param state_type indicates whether the object is mutable or not. * @param life_span indicates the life span of the object: bound to the filter chain, a - * downstream request, or a downstream connection. + * request, or a connection. * * Note that it is an error to call setData() twice with the same * data_name, if the existing object is immutable. Similarly, it is an diff --git a/source/common/http/conn_manager_impl.cc b/source/common/http/conn_manager_impl.cc index 2b39461878463..270d9d92b53b4 100644 --- a/source/common/http/conn_manager_impl.cc +++ b/source/common/http/conn_manager_impl.cc @@ -2387,7 +2387,7 @@ bool ConnectionManagerImpl::ActiveStreamDecoderFilter::recreateStream() { // store any objects with a LifeSpan at or above DownstreamRequest. This is to avoid unnecessary // heap allocation. if (parent_.stream_info_.filter_state_->hasDataAtOrAboveLifeSpan( - StreamInfo::FilterState::LifeSpan::DownstreamRequest)) { + StreamInfo::FilterState::LifeSpan::Request)) { (*parent_.connection_manager_.streams_.begin())->stream_info_.filter_state_ = std::make_shared( parent_.stream_info_.filter_state_->parent(), diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 4171f18440be7..800b691abe19c 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -83,10 +83,9 @@ bool convertRequestHeadersForInternalRedirect(Http::RequestHeaderMap& downstream // Make sure that performing the redirect won't result in exceeding the configured number of // redirects allowed for this route. if (!filter_state.hasData(NumInternalRedirectsFilterStateName)) { - filter_state.setData(NumInternalRedirectsFilterStateName, - std::make_shared(0), - StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::DownstreamRequest); + filter_state.setData( + NumInternalRedirectsFilterStateName, std::make_shared(0), + StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Request); } StreamInfo::UInt32Accessor& num_internal_redirect = filter_state.getDataMutable(NumInternalRedirectsFilterStateName); diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index b8d11e18f21be..1199e43b239bf 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -327,6 +327,8 @@ void UpstreamRequest::onPoolReady( onUpstreamHostSelected(host); + stream_info_.setUpstreamFilterState(std::make_shared( + info.filterState().parent()->parent(), StreamInfo::FilterState::LifeSpan::Request)); stream_info_.setUpstreamLocalAddress(upstream_local_address); parent_.callbacks()->streamInfo().setUpstreamLocalAddress(upstream_local_address); diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index d181153f9a11e..eb9a672a9e365 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -36,7 +36,7 @@ struct StreamInfoImpl : public StreamInfo { start_time_monotonic_(time_source.monotonicTime()), protocol_(protocol), filter_state_(std::make_shared( FilterStateImpl::LazyCreateAncestor(parent_filter_state, - FilterState::LifeSpan::DownstreamConnection), + FilterState::LifeSpan::Connection), FilterState::LifeSpan::FilterChain)), request_id_extension_(Http::RequestIDExtensionFactory::noopInstance()) {} diff --git a/source/extensions/filters/network/sni_cluster/sni_cluster.cc b/source/extensions/filters/network/sni_cluster/sni_cluster.cc index eb1694647b75c..876eca758a942 100644 --- a/source/extensions/filters/network/sni_cluster/sni_cluster.cc +++ b/source/extensions/filters/network/sni_cluster/sni_cluster.cc @@ -21,8 +21,7 @@ Network::FilterStatus SniClusterFilter::onNewConnection() { read_callbacks_->connection().streamInfo().filterState()->setData( TcpProxy::PerConnectionCluster::key(), std::make_unique(sni), - StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::DownstreamConnection); + StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Connection); } return Network::FilterStatus::Continue; diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index a820323b4286a..99fa69b05acb0 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -5388,11 +5388,11 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionFilterState) { decoder_filters_[0]->callbacks_->streamInfo().filterState()->setData( "per_downstream_request", std::make_unique(2), StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::DownstreamRequest); + StreamInfo::FilterState::LifeSpan::Request); decoder_filters_[0]->callbacks_->streamInfo().filterState()->setData( "per_downstream_connection", std::make_unique(3), StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::DownstreamConnection); + StreamInfo::FilterState::LifeSpan::Connection); return FilterHeadersStatus::StopIteration; })); EXPECT_CALL(*decoder_filters_[1], decodeHeaders(_, true)) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 36e5eab6ddc72..9793d388121d5 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -1,5 +1,6 @@ #include #include +#include #include #include "envoy/config/core/v3/base.pb.h" @@ -303,8 +304,7 @@ class RouterTestBase : public testing::Test { callbacks_.streamInfo().filterState()->setData( "num_internal_redirects", std::make_shared(num_previous_redirects), - StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::DownstreamRequest); + StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Request); } void setIncludeAttemptCountInRequest(bool include) { @@ -4331,6 +4331,58 @@ TEST_F(RouterTest, DirectResponseWithoutLocation) { EXPECT_EQ(1UL, config_.stats_.rq_direct_response_.value()); } +// Allows verifying the state of the upstream StreamInfo +class TestAccessLog : public AccessLog::Instance { +public: + explicit TestAccessLog(std::function func) : func_(func) {} + + void log(const Http::RequestHeaderMap*, const Http::ResponseHeaderMap*, + const Http::ResponseTrailerMap*, const StreamInfo::StreamInfo& info) override { + func_(info); + } + +private: + std::function func_; +}; + +// Verifies that we propagate the upstream connection filter state to the upstream request filter +// state. +TEST_F(RouterTest, PropagatesUpstreamFilterState) { + NiceMock encoder; + Http::ResponseDecoder* response_decoder = nullptr; + + // This pattern helps ensure that we're actually invoking the callback. + bool filter_state_verified = false; + router_.config().upstream_logs_.push_back( + std::make_shared([&](const auto& stream_info) { + filter_state_verified = stream_info.upstreamFilterState()->hasDataWithName("upstream data"); + })); + + upstream_stream_info_.filterState()->setData( + "upstream data", std::make_unique(123), + StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::Connection); + expectResponseTimerCreate(); + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke( + [&](Http::ResponseDecoder& decoder, + Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder, cm_.conn_pool_.host_, upstream_stream_info_); + return nullptr; + })); + + Http::TestRequestHeaderMapImpl headers{}; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + Http::ResponseHeaderMapPtr response_headers( + new Http::TestResponseHeaderMapImpl{{":status", "200"}}); + response_decoder->decodeHeaders(std::move(response_headers), true); + EXPECT_TRUE(verifyHostUpstreamStats(1, 0)); + + EXPECT_TRUE(filter_state_verified); +} + TEST_F(RouterTest, UpstreamSSLConnection) { NiceMock encoder; Http::ResponseDecoder* response_decoder = nullptr; diff --git a/test/common/stream_info/filter_state_impl_test.cc b/test/common/stream_info/filter_state_impl_test.cc index 4c2659463adae..24590fa7c3715 100644 --- a/test/common/stream_info/filter_state_impl_test.cc +++ b/test/common/stream_info/filter_state_impl_test.cc @@ -249,15 +249,13 @@ TEST_F(FilterStateImplTest, LifeSpanInitFromParent) { filter_state().setData("test_2", std::make_unique(2), FilterState::StateType::Mutable, FilterState::LifeSpan::FilterChain); filter_state().setData("test_3", std::make_unique(3), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::DownstreamRequest); + FilterState::StateType::ReadOnly, FilterState::LifeSpan::Request); filter_state().setData("test_4", std::make_unique(4), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamRequest); + FilterState::LifeSpan::Request); filter_state().setData("test_5", std::make_unique(5), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::DownstreamConnection); + FilterState::StateType::ReadOnly, FilterState::LifeSpan::Connection); filter_state().setData("test_6", std::make_unique(6), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamConnection); + FilterState::LifeSpan::Connection); FilterStateImpl new_filter_state(filter_state().parent(), FilterState::LifeSpan::FilterChain); EXPECT_FALSE(new_filter_state.hasDataWithName("test_1")); @@ -282,15 +280,13 @@ TEST_F(FilterStateImplTest, LifeSpanInitFromGrandparent) { filter_state().setData("test_2", std::make_unique(2), FilterState::StateType::Mutable, FilterState::LifeSpan::FilterChain); filter_state().setData("test_3", std::make_unique(3), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::DownstreamRequest); + FilterState::StateType::ReadOnly, FilterState::LifeSpan::Request); filter_state().setData("test_4", std::make_unique(4), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamRequest); + FilterState::LifeSpan::Request); filter_state().setData("test_5", std::make_unique(5), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::DownstreamConnection); + FilterState::StateType::ReadOnly, FilterState::LifeSpan::Connection); filter_state().setData("test_6", std::make_unique(6), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamConnection); + FilterState::LifeSpan::Connection); FilterStateImpl new_filter_state(filter_state().parent()->parent(), FilterState::LifeSpan::FilterChain); @@ -312,18 +308,15 @@ TEST_F(FilterStateImplTest, LifeSpanInitFromNonParent) { filter_state().setData("test_2", std::make_unique(2), FilterState::StateType::Mutable, FilterState::LifeSpan::FilterChain); filter_state().setData("test_3", std::make_unique(3), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::DownstreamRequest); + FilterState::StateType::ReadOnly, FilterState::LifeSpan::Request); filter_state().setData("test_4", std::make_unique(4), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamRequest); + FilterState::LifeSpan::Request); filter_state().setData("test_5", std::make_unique(5), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::DownstreamConnection); + FilterState::StateType::ReadOnly, FilterState::LifeSpan::Connection); filter_state().setData("test_6", std::make_unique(6), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamConnection); + FilterState::LifeSpan::Connection); - FilterStateImpl new_filter_state(filter_state().parent(), - FilterState::LifeSpan::DownstreamRequest); + FilterStateImpl new_filter_state(filter_state().parent(), FilterState::LifeSpan::Request); EXPECT_FALSE(new_filter_state.hasDataWithName("test_1")); EXPECT_FALSE(new_filter_state.hasDataWithName("test_2")); EXPECT_FALSE(new_filter_state.hasDataWithName("test_3")); @@ -336,29 +329,25 @@ TEST_F(FilterStateImplTest, HasDataAtOrAboveLifeSpan) { filter_state().setData("test_1", std::make_unique(1), FilterState::StateType::ReadOnly, FilterState::LifeSpan::FilterChain); EXPECT_TRUE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::FilterChain)); - EXPECT_FALSE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::DownstreamRequest)); - EXPECT_FALSE( - filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::DownstreamConnection)); + EXPECT_FALSE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::Request)); + EXPECT_FALSE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::Connection)); filter_state().setData("test_2", std::make_unique(2), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::DownstreamRequest); + FilterState::StateType::ReadOnly, FilterState::LifeSpan::Request); EXPECT_TRUE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::FilterChain)); - EXPECT_TRUE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::DownstreamRequest)); - EXPECT_FALSE( - filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::DownstreamConnection)); + EXPECT_TRUE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::Request)); + EXPECT_FALSE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::Connection)); filter_state().setData("test_3", std::make_unique(3), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::DownstreamConnection); + FilterState::StateType::ReadOnly, FilterState::LifeSpan::Connection); EXPECT_TRUE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::FilterChain)); - EXPECT_TRUE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::DownstreamRequest)); - EXPECT_TRUE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::DownstreamConnection)); + EXPECT_TRUE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::Request)); + EXPECT_TRUE(filter_state().hasDataAtOrAboveLifeSpan(FilterState::LifeSpan::Connection)); } TEST_F(FilterStateImplTest, SetSameDataWithDifferentLifeSpan) { filter_state().setData("test_1", std::make_unique(1), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamConnection); + FilterState::LifeSpan::Connection); // Test reset on smaller LifeSpan EXPECT_THROW_WITH_MESSAGE( filter_state().setData("test_1", std::make_unique(2), @@ -367,18 +356,17 @@ TEST_F(FilterStateImplTest, SetSameDataWithDifferentLifeSpan) { "FilterState::setData called twice with conflicting life_span on the same data_name."); EXPECT_THROW_WITH_MESSAGE( filter_state().setData("test_1", std::make_unique(2), - FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamRequest), + FilterState::StateType::Mutable, FilterState::LifeSpan::Request), EnvoyException, "FilterState::setData called twice with conflicting life_span on the same data_name."); // Still mutable on the correct LifeSpan. filter_state().setData("test_1", std::make_unique(2), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamConnection); + FilterState::LifeSpan::Connection); EXPECT_EQ(2, filter_state().getDataMutable("test_1").access()); filter_state().setData("test_2", std::make_unique(1), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamRequest); + FilterState::LifeSpan::Request); // Test reset on smaller and greater LifeSpan EXPECT_THROW_WITH_MESSAGE( filter_state().setData("test_2", std::make_unique(2), @@ -387,14 +375,13 @@ TEST_F(FilterStateImplTest, SetSameDataWithDifferentLifeSpan) { "FilterState::setData called twice with conflicting life_span on the same data_name."); EXPECT_THROW_WITH_MESSAGE( filter_state().setData("test_2", std::make_unique(2), - FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamConnection), + FilterState::StateType::Mutable, FilterState::LifeSpan::Connection), EnvoyException, "FilterState::setData called twice with conflicting life_span on the same data_name."); // Still mutable on the correct LifeSpan. filter_state().setData("test_2", std::make_unique(2), FilterState::StateType::Mutable, - FilterState::LifeSpan::DownstreamRequest); + FilterState::LifeSpan::Request); EXPECT_EQ(2, filter_state().getDataMutable("test_2").access()); } diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index 34a92a7798cbf..d3aed149adcdd 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -751,8 +751,7 @@ TEST(ConfigTest, PerConnectionClusterWithTopLevelMetadataMatchConfig) { NiceMock connection; connection.stream_info_.filterState()->setData( "envoy.tcp_proxy.cluster", std::make_unique("filter_state_cluster"), - StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::DownstreamConnection); + StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Connection); const auto route = config_obj.getRouteFromEntries(connection); EXPECT_NE(nullptr, route); @@ -1719,8 +1718,7 @@ TEST_F(TcpProxyTest, ShareFilterState) { upstream_connections_.at(0)->streamInfo().filterState()->setData( "envoy.tcp_proxy.cluster", std::make_unique("filter_state_cluster"), - StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::DownstreamConnection); + StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Connection); raiseEventUpstreamConnected(0); EXPECT_EQ("filter_state_cluster", filter_callbacks_.connection_.streamInfo() @@ -1828,8 +1826,7 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(UseClusterFromPerConnectionC connection_.streamInfo().filterState()->setData( "envoy.tcp_proxy.cluster", std::make_unique("filter_state_cluster"), - StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::DownstreamConnection); + StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::Connection); // Expect filter to try to open a connection to specified cluster. EXPECT_CALL(factory_context_.cluster_manager_, @@ -1846,8 +1843,7 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(UpstreamServerName)) { connection_.streamInfo().filterState()->setData( "envoy.network.upstream_server_name", std::make_unique("www.example.com"), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::DownstreamConnection); + StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::Connection); // Expect filter to try to open a connection to a cluster with the transport socket options with // override-server-name @@ -1878,8 +1874,7 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(ApplicationProtocols)) { connection_.streamInfo().filterState()->setData( Network::ApplicationProtocols::key(), std::make_unique(std::vector{"foo", "bar"}), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::DownstreamConnection); + StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::Connection); // Expect filter to try to open a connection to a cluster with the transport socket options with // override-application-protocol