From 083fb88badf9173f4b776d6499572317007385c1 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Mon, 27 Jan 2020 15:47:54 -0800 Subject: [PATCH 01/17] Support looking up filterstate from upstream filter Signed-off-by: gargnupur --- include/envoy/stream_info/stream_info.h | 10 ++++++++++ source/common/stream_info/stream_info_impl.h | 17 ++++++++++++++--- source/common/tcp_proxy/tcp_proxy.cc | 3 +++ .../filters/network/common/redis/codec_impl.cc | 4 ++-- 4 files changed, 29 insertions(+), 5 deletions(-) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index b5ffe8613f9df..d565b8132b9db 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -481,6 +481,16 @@ class StreamInfo { */ virtual FilterState& filterState() PURE; virtual const FilterState& filterState() const PURE; + virtual std::shared_ptr filterStatePtr() PURE; + + /** + * FilterState object to be shared between upstream and downstream filters. + * @param pointer to downstream connections filterstate. + * @return pointer to filterstate to be used by upstream connections. + */ + virtual std::shared_ptr upstreamFilterState() PURE; + virtual const FilterState* upstreamFilterState() const PURE; + virtual void setUpstreamFilterState(std::shared_ptr filter_state) PURE; /** * @param SNI value requested. diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index 6111a1918e4a4..2b29479db2a60 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -19,12 +19,14 @@ struct StreamInfoImpl : public StreamInfo { StreamInfoImpl(TimeSource& time_source) : time_source_(time_source), start_time_(time_source.systemTime()), start_time_monotonic_(time_source.monotonicTime()), - filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)) {} + filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)), + upstream_filter_state_(nullptr) {} StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source) : time_source_(time_source), start_time_(time_source.systemTime()), start_time_monotonic_(time_source.monotonicTime()), protocol_(protocol), - filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)) {} + filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)), + upstream_filter_state_(nullptr) {} StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source, std::shared_ptr& parent_filter_state) @@ -33,7 +35,8 @@ struct StreamInfoImpl : public StreamInfo { filter_state_(std::make_shared( FilterStateImpl::LazyCreateAncestor(parent_filter_state, FilterState::LifeSpan::DownstreamConnection), - FilterState::LifeSpan::FilterChain)) {} + FilterState::LifeSpan::FilterChain)), + upstream_filter_state_(nullptr) {} SystemTime startTime() const override { return start_time_; } @@ -218,6 +221,13 @@ struct StreamInfoImpl : public StreamInfo { FilterState& filterState() override { return *filter_state_; } const FilterState& filterState() const override { return *filter_state_; } + std::shared_ptr filterStatePtr() override { return filter_state_; } + + std::shared_ptr upstreamFilterState() override { return upstream_filter_state_; } + const FilterState* upstreamFilterState() const override { return upstream_filter_state_.get(); } + void setUpstreamFilterState(std::shared_ptr filter_state) override { + upstream_filter_state_ = filter_state; + } void setRequestedServerName(absl::string_view requested_server_name) override { requested_server_name_ = std::string(requested_server_name); @@ -262,6 +272,7 @@ struct StreamInfoImpl : public StreamInfo { const Router::RouteEntry* route_entry_{}; envoy::config::core::v3::Metadata metadata_{}; std::shared_ptr filter_state_; + std::shared_ptr upstream_filter_state_; std::string route_name_; private: diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 78bb3375620a6..f05d676256ae5 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -468,6 +468,9 @@ void Filter::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, getStreamInfo().setUpstreamLocalAddress(connection.localAddress()); getStreamInfo().setUpstreamSslConnection(connection.streamInfo().downstreamSslConnection()); + read_callbacks_->connection().streamInfo().setUpstreamFilterState( + connection.streamInfo().filterStatePtr()); + // Simulate the event that onPoolReady represents. upstream_callbacks_->onEvent(Network::ConnectionEvent::Connected); diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index 2b5e1917686c2..52c33e33632a0 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -316,8 +316,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( - const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator:: +operator!=(const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } From 74e51847ba93618005e9e1b8e52e16dc0c8810ef Mon Sep 17 00:00:00 2001 From: gargnupur Date: Mon, 27 Jan 2020 15:51:53 -0800 Subject: [PATCH 02/17] Fix comments Signed-off-by: gargnupur --- include/envoy/stream_info/stream_info.h | 2 +- source/extensions/filters/network/common/redis/codec_impl.cc | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index d565b8132b9db..08fb24e08a5bb 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -485,7 +485,7 @@ class StreamInfo { /** * FilterState object to be shared between upstream and downstream filters. - * @param pointer to downstream connections filterstate. + * @param pointer to upstream connections filterstate. * @return pointer to filterstate to be used by upstream connections. */ virtual std::shared_ptr upstreamFilterState() PURE; diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index 52c33e33632a0..504c6426d5f35 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -316,8 +316,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator:: -operator!=(const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( + const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } From 6b71939b41faa54e72df4d33fd9ff68e87f79736 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Mon, 27 Jan 2020 15:52:43 -0800 Subject: [PATCH 03/17] fix lint Signed-off-by: gargnupur --- source/extensions/filters/network/common/redis/codec_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index 504c6426d5f35..2b5e1917686c2 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -317,7 +317,7 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { } bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( - const CompositeArrayConstIterator& rhs) const { + const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } From 7d04c6465189864e7155fa5c1064c161a2189203 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 29 Jan 2020 14:06:11 -0800 Subject: [PATCH 04/17] Fixed based on feedback Signed-off-by: gargnupur --- include/envoy/stream_info/filter_state.h | 2 + include/envoy/stream_info/stream_info.h | 8 ++-- source/common/router/router.cc | 22 ++++++----- source/common/stream_info/stream_info_impl.h | 23 +++++------ source/common/tcp_proxy/tcp_proxy.cc | 7 ++-- .../http/grpc_stats/grpc_stats_filter.cc | 10 +++-- .../filters/http/jwt_authn/filter.cc | 2 +- .../network/sni_cluster/sni_cluster.cc | 4 +- test/common/http/conn_manager_impl_test.cc | 18 ++++----- test/common/router/router_test.cc | 38 +++++++++---------- .../stream_info/stream_info_impl_test.cc | 12 ++++-- test/common/stream_info/test_util.h | 16 ++++++-- test/common/tcp_proxy/tcp_proxy_test.cc | 24 +++++++----- .../filters/http/grpc_stats/config_test.cc | 14 ++++--- .../http/jwt_authn/filter_integration_test.cc | 2 +- test/mocks/stream_info/mocks.cc | 9 ++++- test/mocks/stream_info/mocks.h | 8 +++- 17 files changed, 126 insertions(+), 93 deletions(-) diff --git a/include/envoy/stream_info/filter_state.h b/include/envoy/stream_info/filter_state.h index 486d268b2e7f0..0e41d967cf19f 100644 --- a/include/envoy/stream_info/filter_state.h +++ b/include/envoy/stream_info/filter_state.h @@ -150,5 +150,7 @@ class FilterState { virtual Object* getDataMutableGeneric(absl::string_view data_name) PURE; }; +using FilterStateSharedPtr = std::shared_ptr; + } // namespace StreamInfo } // namespace Envoy diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index 08fb24e08a5bb..f4747ab9ef3fc 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -479,18 +479,16 @@ class StreamInfo { * filters (append only). Both object types can be consumed by multiple filters. * @return the filter state associated with this request. */ - virtual FilterState& filterState() PURE; + virtual const FilterStateSharedPtr& filterState() PURE; virtual const FilterState& filterState() const PURE; - virtual std::shared_ptr filterStatePtr() PURE; /** * FilterState object to be shared between upstream and downstream filters. * @param pointer to upstream connections filterstate. * @return pointer to filterstate to be used by upstream connections. */ - virtual std::shared_ptr upstreamFilterState() PURE; - virtual const FilterState* upstreamFilterState() const PURE; - virtual void setUpstreamFilterState(std::shared_ptr filter_state) PURE; + virtual const FilterStateSharedPtr& upstreamFilterState() const PURE; + virtual void setUpstreamFilterState(const FilterStateSharedPtr& filter_state) PURE; /** * @param SNI value requested. diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 3c4e29470a065..788efb9dceeb1 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -386,11 +386,12 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e // Extract debug configuration from filter state. This is used further along to determine whether // we should append cluster and host headers to the response, and whether to forward the request // upstream. - const StreamInfo::FilterState& filter_state = callbacks_->streamInfo().filterState(); + const StreamInfo::FilterStateSharedPtr& filter_state = callbacks_->streamInfo().filterState(); const DebugConfig* debug_config = - filter_state.hasData(DebugConfig::key()) - ? &(filter_state.getDataReadOnly(DebugConfig::key())) - : nullptr; + filter_state ? filter_state->hasData(DebugConfig::key()) + ? &(filter_state->getDataReadOnly(DebugConfig::key())) + : nullptr + : nullptr; // TODO: Maybe add a filter API for this. grpc_request_ = Grpc::Common::hasGrpcContentType(headers); @@ -523,11 +524,12 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e upstream_http_protocol_options.value().auto_sni()) { const auto parsed_authority = Http::Utility::parseAuthority(headers.Host()->value().getStringView()); - if (!parsed_authority.is_ip_address_) { + const StreamInfo::FilterStateSharedPtr& filterState = callbacks_->streamInfo().filterState(); + if (!parsed_authority.is_ip_address_ && filterState) { // TODO: Add SAN verification here and use it from dynamic_forward_proxy // Update filter state with the host/authority to use for setting SNI in the transport // socket options. This is referenced during the getConnPool() call below. - callbacks_->streamInfo().filterState().setData( + filterState->setData( Network::UpstreamServerName::key(), std::make_unique(parsed_authority.host_), StreamInfo::FilterState::StateType::Mutable); @@ -627,7 +629,7 @@ Http::ConnectionPool::Instance* Filter::getConnPool() { // Note: Cluster may downgrade HTTP2 to HTTP1 based on runtime configuration. Http::Protocol protocol = cluster_->upstreamHttpProtocol(callbacks_->streamInfo().protocol()); transport_socket_options_ = Network::TransportSocketOptionsUtility::fromFilterState( - callbacks_->streamInfo().filterState()); + *callbacks_->streamInfo().filterState()); return config_.cm_.httpConnPoolForCluster(route_entry_->clusterName(), route_entry_->priority(), protocol, this); @@ -1370,13 +1372,13 @@ bool Filter::setupRedirect(const Http::HeaderMap& headers, UpstreamRequest& upst attempting_internal_redirect_with_complete_stream_ = upstream_request.upstream_timing_.last_upstream_rx_byte_received_ && downstream_end_stream_; - StreamInfo::FilterState& filter_state = callbacks_->streamInfo().filterState(); + const StreamInfo::FilterStateSharedPtr& filter_state = callbacks_->streamInfo().filterState(); // As with setupRetry, redirects are not supported for streaming requests yet. if (downstream_end_stream_ && !callbacks_->decodingBuffer() && // Redirects with body not yet supported. - location != nullptr && - convertRequestHeadersForInternalRedirect(*downstream_headers_, filter_state, + location != nullptr && filter_state && + convertRequestHeadersForInternalRedirect(*downstream_headers_, *filter_state, route_entry_->maxInternalRedirects(), *location, *callbacks_->connection()) && callbacks_->recreateStream()) { diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index 2b29479db2a60..e6aaecbcf8e34 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -19,14 +19,12 @@ struct StreamInfoImpl : public StreamInfo { StreamInfoImpl(TimeSource& time_source) : time_source_(time_source), start_time_(time_source.systemTime()), start_time_monotonic_(time_source.monotonicTime()), - filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)), - upstream_filter_state_(nullptr) {} + filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)) {} StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source) : time_source_(time_source), start_time_(time_source.systemTime()), start_time_monotonic_(time_source.monotonicTime()), protocol_(protocol), - filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)), - upstream_filter_state_(nullptr) {} + filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)) {} StreamInfoImpl(Http::Protocol protocol, TimeSource& time_source, std::shared_ptr& parent_filter_state) @@ -35,8 +33,7 @@ struct StreamInfoImpl : public StreamInfo { filter_state_(std::make_shared( FilterStateImpl::LazyCreateAncestor(parent_filter_state, FilterState::LifeSpan::DownstreamConnection), - FilterState::LifeSpan::FilterChain)), - upstream_filter_state_(nullptr) {} + FilterState::LifeSpan::FilterChain)) {} SystemTime startTime() const override { return start_time_; } @@ -219,13 +216,13 @@ struct StreamInfoImpl : public StreamInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; - FilterState& filterState() override { return *filter_state_; } + const FilterStateSharedPtr& filterState() override { return filter_state_; } const FilterState& filterState() const override { return *filter_state_; } - std::shared_ptr filterStatePtr() override { return filter_state_; } - std::shared_ptr upstreamFilterState() override { return upstream_filter_state_; } - const FilterState* upstreamFilterState() const override { return upstream_filter_state_.get(); } - void setUpstreamFilterState(std::shared_ptr filter_state) override { + const FilterStateSharedPtr& upstreamFilterState() const override { + return upstream_filter_state_; + } + void setUpstreamFilterState(const FilterStateSharedPtr& filter_state) override { upstream_filter_state_ = filter_state; } @@ -271,8 +268,8 @@ struct StreamInfoImpl : public StreamInfo { bool health_check_request_{}; const Router::RouteEntry* route_entry_{}; envoy::config::core::v3::Metadata metadata_{}; - std::shared_ptr filter_state_; - std::shared_ptr upstream_filter_state_; + FilterStateSharedPtr filter_state_{}; + FilterStateSharedPtr upstream_filter_state_{}; std::string route_name_; private: diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index f05d676256ae5..435a341573ffb 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -168,10 +168,11 @@ Config::Config(const envoy::extensions::filters::network::tcp_proxy::v3::TcpProx RouteConstSharedPtr Config::getRegularRouteFromEntries(Network::Connection& connection) { // First check if the per-connection state to see if we need to route to a pre-selected cluster - if (connection.streamInfo().filterState().hasData( + if (connection.streamInfo().filterState() && + connection.streamInfo().filterState()->hasData( PerConnectionCluster::key())) { const PerConnectionCluster& per_connection_cluster = - connection.streamInfo().filterState().getDataReadOnly( + connection.streamInfo().filterState()->getDataReadOnly( PerConnectionCluster::key()); envoy::extensions::filters::network::tcp_proxy::v3::TcpProxy::DeprecatedV1::TCPRoute @@ -469,7 +470,7 @@ void Filter::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, getStreamInfo().setUpstreamSslConnection(connection.streamInfo().downstreamSslConnection()); read_callbacks_->connection().streamInfo().setUpstreamFilterState( - connection.streamInfo().filterStatePtr()); + connection.streamInfo().filterState()); // Simulate the event that onPoolReady represents. upstream_callbacks_->onEvent(Network::ConnectionEvent::Connected); diff --git a/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc b/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc index a9668fc6a3e45..3204e7a52645d 100644 --- a/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc +++ b/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc @@ -81,10 +81,12 @@ class GrpcStatsFilter : public Http::PassThroughFilter { if (filter_object_ == nullptr) { auto state = std::make_unique(); filter_object_ = state.get(); - decoder_callbacks_->streamInfo().filterState().setData( - HttpFilterNames::get().GrpcStats, std::move(state), - StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::FilterChain); + if (decoder_callbacks_->streamInfo().filterState()) { + decoder_callbacks_->streamInfo().filterState()->setData( + HttpFilterNames::get().GrpcStats, std::move(state), + StreamInfo::FilterState::StateType::Mutable, + StreamInfo::FilterState::LifeSpan::FilterChain); + } } filter_object_->request_message_count = request_counter_.frameCount(); filter_object_->response_message_count = response_counter_.frameCount(); diff --git a/source/extensions/filters/http/jwt_authn/filter.cc b/source/extensions/filters/http/jwt_authn/filter.cc index 08bf82fb3c65c..117a8c62c2bba 100644 --- a/source/extensions/filters/http/jwt_authn/filter.cc +++ b/source/extensions/filters/http/jwt_authn/filter.cc @@ -59,7 +59,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool) // Verify the JWT token, onComplete() will be called when completed. const auto* verifier = - config_->findVerifier(headers, decoder_callbacks_->streamInfo().filterState()); + config_->findVerifier(headers, *decoder_callbacks_->streamInfo().filterState()); if (!verifier) { onComplete(Status::Ok); } else { diff --git a/source/extensions/filters/network/sni_cluster/sni_cluster.cc b/source/extensions/filters/network/sni_cluster/sni_cluster.cc index b0962fb198816..649c478ebc985 100644 --- a/source/extensions/filters/network/sni_cluster/sni_cluster.cc +++ b/source/extensions/filters/network/sni_cluster/sni_cluster.cc @@ -15,10 +15,10 @@ Network::FilterStatus SniClusterFilter::onNewConnection() { ENVOY_CONN_LOG(trace, "sni_cluster: new connection with server name {}", read_callbacks_->connection(), sni); - if (!sni.empty()) { + if (!sni.empty() && read_callbacks_->connection().streamInfo().filterState()) { // Set the tcp_proxy cluster to the same value as SNI. The data is mutable to allow // other filters to change it. - read_callbacks_->connection().streamInfo().filterState().setData( + read_callbacks_->connection().streamInfo().filterState()->setData( TcpProxy::PerConnectionCluster::key(), std::make_unique(sni), StreamInfo::FilterState::StateType::Mutable, diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index 2d63ba04fc4bf..724779c73be27 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -5117,15 +5117,15 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionFilterState) { InSequence s; EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, true)) .WillOnce(Invoke([this](HeaderMap&, bool) -> FilterHeadersStatus { - decoder_filters_[0]->callbacks_->streamInfo().filterState().setData( + decoder_filters_[0]->callbacks_->streamInfo().filterState()->setData( "per_filter_chain", std::make_unique(1), StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::FilterChain); - decoder_filters_[0]->callbacks_->streamInfo().filterState().setData( + decoder_filters_[0]->callbacks_->streamInfo().filterState()->setData( "per_downstream_request", std::make_unique(2), StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::DownstreamRequest); - decoder_filters_[0]->callbacks_->streamInfo().filterState().setData( + decoder_filters_[0]->callbacks_->streamInfo().filterState()->setData( "per_downstream_connection", std::make_unique(3), StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::DownstreamConnection); @@ -5134,26 +5134,26 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionFilterState) { EXPECT_CALL(*decoder_filters_[1], decodeHeaders(_, true)) .WillOnce(Invoke([this](HeaderMap&, bool) -> FilterHeadersStatus { EXPECT_FALSE( - decoder_filters_[1]->callbacks_->streamInfo().filterState().hasData( + decoder_filters_[1]->callbacks_->streamInfo().filterState()->hasData( "per_filter_chain")); EXPECT_TRUE( - decoder_filters_[1]->callbacks_->streamInfo().filterState().hasData( + decoder_filters_[1]->callbacks_->streamInfo().filterState()->hasData( "per_downstream_request")); EXPECT_TRUE( - decoder_filters_[1]->callbacks_->streamInfo().filterState().hasData( + decoder_filters_[1]->callbacks_->streamInfo().filterState()->hasData( "per_downstream_connection")); return FilterHeadersStatus::StopIteration; })); EXPECT_CALL(*decoder_filters_[2], decodeHeaders(_, true)) .WillOnce(Invoke([this](HeaderMap&, bool) -> FilterHeadersStatus { EXPECT_FALSE( - decoder_filters_[2]->callbacks_->streamInfo().filterState().hasData( + decoder_filters_[2]->callbacks_->streamInfo().filterState()->hasData( "per_filter_chain")); EXPECT_FALSE( - decoder_filters_[2]->callbacks_->streamInfo().filterState().hasData( + decoder_filters_[2]->callbacks_->streamInfo().filterState()->hasData( "per_downstream_request")); EXPECT_TRUE( - decoder_filters_[2]->callbacks_->streamInfo().filterState().hasData( + decoder_filters_[2]->callbacks_->streamInfo().filterState()->hasData( "per_downstream_connection")); return FilterHeadersStatus::StopIteration; })); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 85e36e4ccce21..7f90c3af15339 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -233,7 +233,7 @@ class RouterTestBase : public testing::Test { } void setNumPreviousRedirect(uint32_t num_previous_redirects) { - callbacks_.streamInfo().filterState().setData( + callbacks_.streamInfo().filterState()->setData( "num_internal_redirects", std::make_shared(num_previous_redirects), StreamInfo::FilterState::StateType::Mutable, @@ -305,9 +305,9 @@ TEST_F(RouterTest, UpdateFilterState) { ON_CALL(callbacks_.stream_info_, filterState()) .WillByDefault(ReturnRef(stream_info.filterState())); EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); - stream_info.filterState().setData(Network::UpstreamServerName::key(), - std::make_unique("dummy"), - StreamInfo::FilterState::StateType::Mutable); + stream_info.filterState()->setData(Network::UpstreamServerName::key(), + std::make_unique("dummy"), + StreamInfo::FilterState::StateType::Mutable); expectResponseTimerCreate(); Http::TestHeaderMapImpl headers; @@ -316,7 +316,7 @@ TEST_F(RouterTest, UpdateFilterState) { router_.decodeHeaders(headers, true); EXPECT_EQ("host", stream_info.filterState() - .getDataReadOnly(Network::UpstreamServerName::key()) + ->getDataReadOnly(Network::UpstreamServerName::key()) .value()); EXPECT_CALL(cancellable_, cancel()); router_.onDestroy(); @@ -840,9 +840,9 @@ void RouterTestBase::testAppendCluster(absl::optional clu /* host_address_header */ absl::nullopt, /* do_not_forward */ false, /* not_forwarded_header */ absl::nullopt); - callbacks_.streamInfo().filterState().setData(DebugConfig::key(), std::move(debug_config), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); + callbacks_.streamInfo().filterState()->setData(DebugConfig::key(), std::move(debug_config), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); NiceMock encoder; Http::StreamDecoder* response_decoder = nullptr; @@ -892,9 +892,9 @@ void RouterTestBase::testAppendUpstreamHost( /* host_address_header */ host_address_header_name, /* do_not_forward */ false, /* not_forwarded_header */ absl::nullopt); - callbacks_.streamInfo().filterState().setData(DebugConfig::key(), std::move(debug_config), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); + callbacks_.streamInfo().filterState()->setData(DebugConfig::key(), std::move(debug_config), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); cm_.conn_pool_.host_->hostname_ = "scooby.doo"; NiceMock encoder; @@ -964,9 +964,9 @@ void RouterTestBase::testDoNotForward( /* host_address_header */ absl::nullopt, /* do_not_forward */ true, /* not_forwarded_header */ not_forwarded_header_name); - callbacks_.streamInfo().filterState().setData(DebugConfig::key(), std::move(debug_config), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); + callbacks_.streamInfo().filterState()->setData(DebugConfig::key(), std::move(debug_config), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); Http::TestHeaderMapImpl response_headers{ {":status", "204"}, @@ -997,9 +997,9 @@ TEST_F(RouterTest, AllDebugConfig) { /* host_address_header */ absl::nullopt, /* do_not_forward */ true, /* not_forwarded_header */ absl::nullopt); - callbacks_.streamInfo().filterState().setData(DebugConfig::key(), std::move(debug_config), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); + callbacks_.streamInfo().filterState()->setData(DebugConfig::key(), std::move(debug_config), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); cm_.conn_pool_.host_->hostname_ = "scooby.doo"; Http::TestHeaderMapImpl response_headers{{":status", "204"}, @@ -3411,7 +3411,7 @@ TEST_F(RouterTest, HttpInternalRedirectSucceeded) { router_.onDestroy(); EXPECT_EQ(3, callbacks_.streamInfo() .filterState() - .getDataMutable("num_internal_redirects") + ->getDataMutable("num_internal_redirects") .value()); } @@ -4528,7 +4528,7 @@ TEST_F(RouterTest, UpstreamSocketOptionsReturnedNonEmpty) { } TEST_F(RouterTest, ApplicationProtocols) { - callbacks_.streamInfo().filterState().setData( + callbacks_.streamInfo().filterState()->setData( Network::ApplicationProtocols::key(), std::make_unique(std::vector{"foo", "bar"}), StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::FilterChain); diff --git a/test/common/stream_info/stream_info_impl_test.cc b/test/common/stream_info/stream_info_impl_test.cc index 37831316855b8..08d784bf6fce2 100644 --- a/test/common/stream_info/stream_info_impl_test.cc +++ b/test/common/stream_info/stream_info_impl_test.cc @@ -162,10 +162,14 @@ TEST_F(StreamInfoImplTest, MiscSettersAndGetters) { stream_info.route_entry_ = &route_entry; EXPECT_EQ(&route_entry, stream_info.routeEntry()); - stream_info.filterState().setData("test", std::make_unique(1), - FilterState::StateType::ReadOnly, - FilterState::LifeSpan::FilterChain); - EXPECT_EQ(1, stream_info.filterState().getDataReadOnly("test").access()); + stream_info.filterState()->setData("test", std::make_unique(1), + FilterState::StateType::ReadOnly, + FilterState::LifeSpan::FilterChain); + EXPECT_EQ(1, stream_info.filterState()->getDataReadOnly("test").access()); + + stream_info.setUpstreamFilterState(stream_info.filterState()); + EXPECT_EQ(1, + stream_info.upstreamFilterState()->getDataReadOnly("test").access()); EXPECT_EQ("", stream_info.requestedServerName()); absl::string_view sni_name = "stubserver.org"; diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index 270fc0518c24e..859a3925510b9 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -174,8 +174,16 @@ class TestStreamInfo : public StreamInfo::StreamInfo { (*metadata_.mutable_filter_metadata())[name].MergeFrom(value); }; - const Envoy::StreamInfo::FilterState& filterState() const override { return filter_state_; } - Envoy::StreamInfo::FilterState& filterState() override { return filter_state_; } + const Envoy::StreamInfo::FilterStateSharedPtr& filterState() override { return filter_state_; } + const Envoy::StreamInfo::FilterState& filterState() const override { return *filter_state_; } + + const Envoy::StreamInfo::FilterStateSharedPtr& upstreamFilterState() const override { + return upstream_filter_state_; + } + void + setUpstreamFilterState(const Envoy::StreamInfo::FilterStateSharedPtr& filter_state) override { + upstream_filter_state_ = filter_state; + } void setRequestedServerName(const absl::string_view requested_server_name) override { requested_server_name_ = std::string(requested_server_name); @@ -224,7 +232,9 @@ class TestStreamInfo : public StreamInfo::StreamInfo { Ssl::ConnectionInfoConstSharedPtr upstream_connection_info_; const Router::RouteEntry* route_entry_{}; envoy::config::core::v3::Metadata metadata_{}; - Envoy::StreamInfo::FilterStateImpl filter_state_; + Envoy::StreamInfo::FilterStateSharedPtr filter_state_{new Envoy::StreamInfo::FilterStateImpl( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)}; + Envoy::StreamInfo::FilterStateSharedPtr upstream_filter_state_{nullptr}; Envoy::StreamInfo::UpstreamTiming upstream_timing_; std::string requested_server_name_; std::string upstream_transport_failure_reason_; diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index 0f25e2b8d748b..2769e37667037 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -750,7 +750,8 @@ TEST(ConfigTest, PerConnectionClusterWithTopLevelMetadataMatchConfig) { HashedValue hv1(v1), hv2(v2); NiceMock connection; - connection.stream_info_.filterState().setData( + EXPECT_NE(nullptr, connection.stream_info_.filterState()); + connection.stream_info_.filterState()->setData( "envoy.tcp_proxy.cluster", std::make_unique("filter_state_cluster"), StreamInfo::FilterState::StateType::Mutable, StreamInfo::FilterState::LifeSpan::DownstreamConnection); @@ -1775,10 +1776,11 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(UseClusterFromPerConnectionC initializeFilter(); NiceMock stream_info; - stream_info.filterState().setData("envoy.tcp_proxy.cluster", - std::make_unique("filter_state_cluster"), - StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::DownstreamConnection); + EXPECT_NE(nullptr, stream_info.filterState()); + stream_info.filterState()->setData("envoy.tcp_proxy.cluster", + std::make_unique("filter_state_cluster"), + StreamInfo::FilterState::StateType::Mutable, + StreamInfo::FilterState::LifeSpan::DownstreamConnection); ON_CALL(connection_, streamInfo()).WillByDefault(ReturnRef(stream_info)); EXPECT_CALL(Const(connection_), streamInfo()).WillRepeatedly(ReturnRef(stream_info)); @@ -1796,10 +1798,11 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(UpstreamServerName)) { initializeFilter(); NiceMock stream_info; - stream_info.filterState().setData("envoy.network.upstream_server_name", - std::make_unique("www.example.com"), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::DownstreamConnection); + EXPECT_NE(nullptr, stream_info.filterState()); + stream_info.filterState()->setData("envoy.network.upstream_server_name", + std::make_unique("www.example.com"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::DownstreamConnection); ON_CALL(connection_, streamInfo()).WillByDefault(ReturnRef(stream_info)); EXPECT_CALL(Const(connection_), streamInfo()).WillRepeatedly(ReturnRef(stream_info)); @@ -1831,7 +1834,8 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(ApplicationProtocols)) { initializeFilter(); NiceMock stream_info; - stream_info.filterState().setData( + EXPECT_NE(nullptr, stream_info.filterState()); + stream_info.filterState()->setData( Network::ApplicationProtocols::key(), std::make_unique(std::vector{"foo", "bar"}), StreamInfo::FilterState::StateType::ReadOnly, diff --git a/test/extensions/filters/http/grpc_stats/config_test.cc b/test/extensions/filters/http/grpc_stats/config_test.cc index 15e39bd3e0487..fa0ef0820645c 100644 --- a/test/extensions/filters/http/grpc_stats/config_test.cc +++ b/test/extensions/filters/http/grpc_stats/config_test.cc @@ -66,7 +66,8 @@ TEST_F(GrpcStatsFilterConfigTest, StatsHttp2HeaderOnlyResponse) { ->statsScope() .counter("grpc.lyft.users.BadCompanions.GetBadCompanions.total") .value()); - EXPECT_FALSE(stream_info_.filterState().hasDataWithName(HttpFilterNames::get().GrpcStats)); + EXPECT_NE(nullptr, stream_info_.filterState()); + EXPECT_FALSE(stream_info_.filterState()->hasDataWithName(HttpFilterNames::get().GrpcStats)); } TEST_F(GrpcStatsFilterConfigTest, StatsHttp2NormalResponse) { @@ -90,7 +91,8 @@ TEST_F(GrpcStatsFilterConfigTest, StatsHttp2NormalResponse) { ->statsScope() .counter("grpc.lyft.users.BadCompanions.GetBadCompanions.total") .value()); - EXPECT_FALSE(stream_info_.filterState().hasDataWithName(HttpFilterNames::get().GrpcStats)); + EXPECT_NE(nullptr, stream_info_.filterState()); + EXPECT_FALSE(stream_info_.filterState()->hasDataWithName(HttpFilterNames::get().GrpcStats)); } TEST_F(GrpcStatsFilterConfigTest, StatsHttp2ContentTypeGrpcPlusProto) { @@ -112,7 +114,8 @@ TEST_F(GrpcStatsFilterConfigTest, StatsHttp2ContentTypeGrpcPlusProto) { ->statsScope() .counter("grpc.lyft.users.BadCompanions.GetBadCompanions.total") .value()); - EXPECT_FALSE(stream_info_.filterState().hasDataWithName(HttpFilterNames::get().GrpcStats)); + EXPECT_NE(nullptr, stream_info_.filterState()); + EXPECT_FALSE(stream_info_.filterState()->hasDataWithName(HttpFilterNames::get().GrpcStats)); } TEST_F(GrpcStatsFilterConfigTest, MessageCounts) { @@ -139,8 +142,9 @@ TEST_F(GrpcStatsFilterConfigTest, MessageCounts) { ->statsScope() .counter("grpc.lyft.users.BadCompanions.GetBadCompanions.response_message_count") .value()); - const auto& data = - stream_info_.filterState().getDataReadOnly(HttpFilterNames::get().GrpcStats); + EXPECT_NE(nullptr, stream_info_.filterState()); + const auto& data = stream_info_.filterState()->getDataReadOnly( + HttpFilterNames::get().GrpcStats); EXPECT_EQ(2U, data.request_message_count); EXPECT_EQ(0U, data.response_message_count); diff --git a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc index 620b5d5c4d88f..36aeeabe484a1 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -32,7 +32,7 @@ class HeaderToFilterStateFilter : public Http::PassThroughDecoderFilter { Http::FilterHeadersStatus decodeHeaders(Http::HeaderMap& headers, bool) override { const Http::HeaderEntry* entry = headers.get(header_); if (entry) { - decoder_callbacks_->streamInfo().filterState().setData( + decoder_callbacks_->streamInfo().filterState()->setData( state_, std::make_unique(entry->value().getStringView()), StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::FilterChain); diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc index 57d2ea436ff9d..bc9d7c1996008 100644 --- a/test/mocks/stream_info/mocks.cc +++ b/test/mocks/stream_info/mocks.cc @@ -15,7 +15,7 @@ namespace Envoy { namespace StreamInfo { MockStreamInfo::MockStreamInfo() - : filter_state_(FilterState::LifeSpan::FilterChain), + : filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)), downstream_local_address_(new Network::Address::Ipv4Instance("127.0.0.2")), downstream_direct_remote_address_(new Network::Address::Ipv4Instance("127.0.0.1")), downstream_remote_address_(new Network::Address::Ipv4Instance("127.0.0.1")) { @@ -88,7 +88,12 @@ MockStreamInfo::MockStreamInfo() ON_CALL(*this, dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(Const(*this), dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, filterState()).WillByDefault(ReturnRef(filter_state_)); - ON_CALL(Const(*this), filterState()).WillByDefault(ReturnRef(filter_state_)); + ON_CALL(Const(*this), filterState()).WillByDefault(ReturnRef(*filter_state_)); + ON_CALL(*this, setUpstreamFilterState(_)) + .WillByDefault(Invoke([this](const FilterStateSharedPtr& filter_state) { + upstream_filter_state_ = filter_state; + })); + ON_CALL(*this, upstreamFilterState()).WillByDefault(ReturnRef(upstream_filter_state_)); ON_CALL(*this, setRequestedServerName(_)) .WillByDefault(Invoke([this](const absl::string_view requested_server_name) { requested_server_name_ = std::string(requested_server_name); diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index 22d7d16990bdd..67faa399b8689 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -78,8 +78,10 @@ class MockStreamInfo : public StreamInfo { MOCK_METHOD(void, setDynamicMetadata, (const std::string&, const ProtobufWkt::Struct&)); MOCK_METHOD(void, setDynamicMetadata, (const std::string&, const std::string&, const std::string&)); - MOCK_METHOD(FilterState&, filterState, ()); + MOCK_METHOD(const FilterStateSharedPtr&, filterState, ()); MOCK_METHOD(const FilterState&, filterState, (), (const)); + MOCK_METHOD(const FilterStateSharedPtr&, upstreamFilterState, ()); + MOCK_METHOD(void, setUpstreamFilterState, (const FilterStateSharedPtr&)); MOCK_METHOD(void, setRequestedServerName, (const absl::string_view)); MOCK_METHOD(const std::string&, requestedServerName, (), (const)); MOCK_METHOD(void, setUpstreamTransportFailureReason, (absl::string_view)); @@ -103,7 +105,9 @@ class MockStreamInfo : public StreamInfo { absl::optional response_code_; absl::optional response_code_details_; envoy::config::core::v3::Metadata metadata_; - FilterStateImpl filter_state_; + std::shared_ptr filter_state_{ + new FilterStateImpl(FilterState::LifeSpan::FilterChain)}; + std::shared_ptr upstream_filter_state_{nullptr}; uint64_t bytes_received_{}; uint64_t bytes_sent_{}; Network::Address::InstanceConstSharedPtr upstream_local_address_; From 401e6d8a14f7b4df7d2a3bde4ef8780b7328298c Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 29 Jan 2020 14:38:33 -0800 Subject: [PATCH 05/17] Fixed test Signed-off-by: gargnupur --- source/common/router/router.cc | 7 +++---- test/mocks/stream_info/mocks.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 788efb9dceeb1..7c4fa33c09ba5 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -529,10 +529,9 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e // TODO: Add SAN verification here and use it from dynamic_forward_proxy // Update filter state with the host/authority to use for setting SNI in the transport // socket options. This is referenced during the getConnPool() call below. - filterState->setData( - Network::UpstreamServerName::key(), - std::make_unique(parsed_authority.host_), - StreamInfo::FilterState::StateType::Mutable); + filterState->setData(Network::UpstreamServerName::key(), + std::make_unique(parsed_authority.host_), + StreamInfo::FilterState::StateType::Mutable); } } diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index 67faa399b8689..589d702d1a9c9 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -80,7 +80,7 @@ class MockStreamInfo : public StreamInfo { (const std::string&, const std::string&, const std::string&)); MOCK_METHOD(const FilterStateSharedPtr&, filterState, ()); MOCK_METHOD(const FilterState&, filterState, (), (const)); - MOCK_METHOD(const FilterStateSharedPtr&, upstreamFilterState, ()); + MOCK_METHOD(const FilterStateSharedPtr&, upstreamFilterState, (), (const)); MOCK_METHOD(void, setUpstreamFilterState, (const FilterStateSharedPtr&)); MOCK_METHOD(void, setRequestedServerName, (const absl::string_view)); MOCK_METHOD(const std::string&, requestedServerName, (), (const)); From ee733db46a90672b22d4285c42efe2d45e607511 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 29 Jan 2020 16:23:57 -0800 Subject: [PATCH 06/17] Remove addition of upstream filterstate logic Signed-off-by: gargnupur --- include/envoy/stream_info/stream_info.h | 8 -------- source/common/stream_info/stream_info_impl.h | 8 -------- source/common/tcp_proxy/tcp_proxy.cc | 3 --- test/common/stream_info/stream_info_impl_test.cc | 4 ---- test/common/stream_info/test_util.h | 12 +----------- test/mocks/stream_info/mocks.cc | 5 ----- test/mocks/stream_info/mocks.h | 6 +----- 7 files changed, 2 insertions(+), 44 deletions(-) diff --git a/include/envoy/stream_info/stream_info.h b/include/envoy/stream_info/stream_info.h index f4747ab9ef3fc..1307edaa133ac 100644 --- a/include/envoy/stream_info/stream_info.h +++ b/include/envoy/stream_info/stream_info.h @@ -482,14 +482,6 @@ class StreamInfo { virtual const FilterStateSharedPtr& filterState() PURE; virtual const FilterState& filterState() const PURE; - /** - * FilterState object to be shared between upstream and downstream filters. - * @param pointer to upstream connections filterstate. - * @return pointer to filterstate to be used by upstream connections. - */ - virtual const FilterStateSharedPtr& upstreamFilterState() const PURE; - virtual void setUpstreamFilterState(const FilterStateSharedPtr& filter_state) PURE; - /** * @param SNI value requested. */ diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index e6aaecbcf8e34..dcfcef7c25ff2 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -219,13 +219,6 @@ struct StreamInfoImpl : public StreamInfo { const FilterStateSharedPtr& filterState() override { return filter_state_; } const FilterState& filterState() const override { return *filter_state_; } - const FilterStateSharedPtr& upstreamFilterState() const override { - return upstream_filter_state_; - } - void setUpstreamFilterState(const FilterStateSharedPtr& filter_state) override { - upstream_filter_state_ = filter_state; - } - void setRequestedServerName(absl::string_view requested_server_name) override { requested_server_name_ = std::string(requested_server_name); } @@ -269,7 +262,6 @@ struct StreamInfoImpl : public StreamInfo { const Router::RouteEntry* route_entry_{}; envoy::config::core::v3::Metadata metadata_{}; FilterStateSharedPtr filter_state_{}; - FilterStateSharedPtr upstream_filter_state_{}; std::string route_name_; private: diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 435a341573ffb..57252321b3b8b 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -469,9 +469,6 @@ void Filter::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data, getStreamInfo().setUpstreamLocalAddress(connection.localAddress()); getStreamInfo().setUpstreamSslConnection(connection.streamInfo().downstreamSslConnection()); - read_callbacks_->connection().streamInfo().setUpstreamFilterState( - connection.streamInfo().filterState()); - // Simulate the event that onPoolReady represents. upstream_callbacks_->onEvent(Network::ConnectionEvent::Connected); diff --git a/test/common/stream_info/stream_info_impl_test.cc b/test/common/stream_info/stream_info_impl_test.cc index 08d784bf6fce2..40e2ed7e37ebb 100644 --- a/test/common/stream_info/stream_info_impl_test.cc +++ b/test/common/stream_info/stream_info_impl_test.cc @@ -167,10 +167,6 @@ TEST_F(StreamInfoImplTest, MiscSettersAndGetters) { FilterState::LifeSpan::FilterChain); EXPECT_EQ(1, stream_info.filterState()->getDataReadOnly("test").access()); - stream_info.setUpstreamFilterState(stream_info.filterState()); - EXPECT_EQ(1, - stream_info.upstreamFilterState()->getDataReadOnly("test").access()); - EXPECT_EQ("", stream_info.requestedServerName()); absl::string_view sni_name = "stubserver.org"; stream_info.setRequestedServerName(sni_name); diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index 859a3925510b9..7b45be5650099 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -177,14 +177,6 @@ class TestStreamInfo : public StreamInfo::StreamInfo { const Envoy::StreamInfo::FilterStateSharedPtr& filterState() override { return filter_state_; } const Envoy::StreamInfo::FilterState& filterState() const override { return *filter_state_; } - const Envoy::StreamInfo::FilterStateSharedPtr& upstreamFilterState() const override { - return upstream_filter_state_; - } - void - setUpstreamFilterState(const Envoy::StreamInfo::FilterStateSharedPtr& filter_state) override { - upstream_filter_state_ = filter_state; - } - void setRequestedServerName(const absl::string_view requested_server_name) override { requested_server_name_ = std::string(requested_server_name); } @@ -232,9 +224,7 @@ class TestStreamInfo : public StreamInfo::StreamInfo { Ssl::ConnectionInfoConstSharedPtr upstream_connection_info_; const Router::RouteEntry* route_entry_{}; envoy::config::core::v3::Metadata metadata_{}; - Envoy::StreamInfo::FilterStateSharedPtr filter_state_{new Envoy::StreamInfo::FilterStateImpl( - Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)}; - Envoy::StreamInfo::FilterStateSharedPtr upstream_filter_state_{nullptr}; + Envoy::StreamInfo::FilterStateSharedPtr filter_state_{std::make_shared(FilterState::LifeSpan::FilterChain)}; Envoy::StreamInfo::UpstreamTiming upstream_timing_; std::string requested_server_name_; std::string upstream_transport_failure_reason_; diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc index bc9d7c1996008..095394e1cce66 100644 --- a/test/mocks/stream_info/mocks.cc +++ b/test/mocks/stream_info/mocks.cc @@ -89,11 +89,6 @@ MockStreamInfo::MockStreamInfo() ON_CALL(Const(*this), dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, filterState()).WillByDefault(ReturnRef(filter_state_)); ON_CALL(Const(*this), filterState()).WillByDefault(ReturnRef(*filter_state_)); - ON_CALL(*this, setUpstreamFilterState(_)) - .WillByDefault(Invoke([this](const FilterStateSharedPtr& filter_state) { - upstream_filter_state_ = filter_state; - })); - ON_CALL(*this, upstreamFilterState()).WillByDefault(ReturnRef(upstream_filter_state_)); ON_CALL(*this, setRequestedServerName(_)) .WillByDefault(Invoke([this](const absl::string_view requested_server_name) { requested_server_name_ = std::string(requested_server_name); diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index 589d702d1a9c9..bee7ebea607dc 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -80,8 +80,6 @@ class MockStreamInfo : public StreamInfo { (const std::string&, const std::string&, const std::string&)); MOCK_METHOD(const FilterStateSharedPtr&, filterState, ()); MOCK_METHOD(const FilterState&, filterState, (), (const)); - MOCK_METHOD(const FilterStateSharedPtr&, upstreamFilterState, (), (const)); - MOCK_METHOD(void, setUpstreamFilterState, (const FilterStateSharedPtr&)); MOCK_METHOD(void, setRequestedServerName, (const absl::string_view)); MOCK_METHOD(const std::string&, requestedServerName, (), (const)); MOCK_METHOD(void, setUpstreamTransportFailureReason, (absl::string_view)); @@ -105,9 +103,7 @@ class MockStreamInfo : public StreamInfo { absl::optional response_code_; absl::optional response_code_details_; envoy::config::core::v3::Metadata metadata_; - std::shared_ptr filter_state_{ - new FilterStateImpl(FilterState::LifeSpan::FilterChain)}; - std::shared_ptr upstream_filter_state_{nullptr}; + FilterStateSharePtr filter_state_; uint64_t bytes_received_{}; uint64_t bytes_sent_{}; Network::Address::InstanceConstSharedPtr upstream_local_address_; From af1448b0015a80ca8cbff5dbd6fb49592a751ddc Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 29 Jan 2020 16:39:15 -0800 Subject: [PATCH 07/17] Fix fmt Signed-off-by: gargnupur --- test/common/stream_info/test_util.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index 7b45be5650099..ea5d5eaf13cad 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -224,7 +224,8 @@ class TestStreamInfo : public StreamInfo::StreamInfo { Ssl::ConnectionInfoConstSharedPtr upstream_connection_info_; const Router::RouteEntry* route_entry_{}; envoy::config::core::v3::Metadata metadata_{}; - Envoy::StreamInfo::FilterStateSharedPtr filter_state_{std::make_shared(FilterState::LifeSpan::FilterChain)}; + Envoy::StreamInfo::FilterStateSharedPtr filter_state_{ + std::make_shared(FilterState::LifeSpan::FilterChain)}; Envoy::StreamInfo::UpstreamTiming upstream_timing_; std::string requested_server_name_; std::string upstream_transport_failure_reason_; From 357e07b7a5a8d0dfb67949f8721030b0ae2b4f10 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 29 Jan 2020 16:53:13 -0800 Subject: [PATCH 08/17] Fix err in test Signed-off-by: gargnupur --- test/mocks/stream_info/mocks.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index bee7ebea607dc..3c11ed353a6c8 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -103,7 +103,7 @@ class MockStreamInfo : public StreamInfo { absl::optional response_code_; absl::optional response_code_details_; envoy::config::core::v3::Metadata metadata_; - FilterStateSharePtr filter_state_; + FilterStateSharedPtr filter_state_; uint64_t bytes_received_{}; uint64_t bytes_sent_{}; Network::Address::InstanceConstSharedPtr upstream_local_address_; From 58656c544f276ddc7b00ddc59662277cd6d7a01e Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 29 Jan 2020 17:24:03 -0800 Subject: [PATCH 09/17] Fix err in test Signed-off-by: gargnupur --- test/common/stream_info/test_util.h | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index ea5d5eaf13cad..6069b54e0d357 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -12,7 +12,9 @@ namespace Envoy { class TestStreamInfo : public StreamInfo::StreamInfo { public: - TestStreamInfo() : filter_state_(Envoy::StreamInfo::FilterState::LifeSpan::FilterChain) { + TestStreamInfo() + : filter_state_(std::make_shared( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)) { // Use 1999-01-01 00:00:00 +0 time_t fake_time = 915148800; start_time_ = std::chrono::system_clock::from_time_t(fake_time); @@ -225,7 +227,8 @@ class TestStreamInfo : public StreamInfo::StreamInfo { const Router::RouteEntry* route_entry_{}; envoy::config::core::v3::Metadata metadata_{}; Envoy::StreamInfo::FilterStateSharedPtr filter_state_{ - std::make_shared(FilterState::LifeSpan::FilterChain)}; + std::make_shared( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)}; Envoy::StreamInfo::UpstreamTiming upstream_timing_; std::string requested_server_name_; std::string upstream_transport_failure_reason_; From 7d1d2bc21d86999e21d336064ace49a79d6403d6 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 29 Jan 2020 22:32:18 -0800 Subject: [PATCH 10/17] Fix err in test Signed-off-by: gargnupur --- .../access_loggers/grpc/http_grpc_access_log_impl_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc index 19d83ca9c2024..bc4a6fcdd0660 100644 --- a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc +++ b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc @@ -145,11 +145,11 @@ TEST_F(HttpGrpcAccessLogTest, Marshalling) { stream_info.last_downstream_tx_byte_sent_ = 2ms; stream_info.setDownstreamLocalAddress(std::make_shared("/foo")); (*stream_info.metadata_.mutable_filter_metadata())["foo"] = ProtobufWkt::Struct(); - stream_info.filter_state_.setData("string_accessor", + stream_info.filter_state_->setData("string_accessor", std::make_unique("test_value"), StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::FilterChain); - stream_info.filter_state_.setData("serialized", std::make_unique(), + stream_info.filter_state_->setData("serialized", std::make_unique(), StreamInfo::FilterState::StateType::ReadOnly, StreamInfo::FilterState::LifeSpan::FilterChain); expectLog(R"EOF( From dc457d1fd5a7eb4f608249e4f3de71a883607891 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 29 Jan 2020 22:35:47 -0800 Subject: [PATCH 11/17] Fix fmt Signed-off-by: gargnupur --- .../grpc/http_grpc_access_log_impl_test.cc | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc index bc4a6fcdd0660..2afcb68817d80 100644 --- a/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc +++ b/test/extensions/access_loggers/grpc/http_grpc_access_log_impl_test.cc @@ -146,12 +146,12 @@ TEST_F(HttpGrpcAccessLogTest, Marshalling) { stream_info.setDownstreamLocalAddress(std::make_shared("/foo")); (*stream_info.metadata_.mutable_filter_metadata())["foo"] = ProtobufWkt::Struct(); stream_info.filter_state_->setData("string_accessor", - std::make_unique("test_value"), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); + std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); stream_info.filter_state_->setData("serialized", std::make_unique(), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); expectLog(R"EOF( common_properties: downstream_remote_address: From cfc107861e37fadda45a54ef7b11b93d54f2d24c Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 29 Jan 2020 23:24:42 -0800 Subject: [PATCH 12/17] Fix test Signed-off-by: gargnupur --- .../access_log/access_log_formatter_test.cc | 64 ++++++++++--------- 1 file changed, 33 insertions(+), 31 deletions(-) diff --git a/test/common/access_log/access_log_formatter_test.cc b/test/common/access_log/access_log_formatter_test.cc index 0529c63a63b67..cdc46732edc5d 100644 --- a/test/common/access_log/access_log_formatter_test.cc +++ b/test/common/access_log/access_log_formatter_test.cc @@ -1027,16 +1027,16 @@ TEST(AccessLogFormatterTest, DynamicMetadataFormatter) { TEST(AccessLogFormatterTest, FilterStateFormatter) { Http::TestHeaderMapImpl header; StreamInfo::MockStreamInfo stream_info; - stream_info.filter_state_.setData("key", - std::make_unique("test_value"), - StreamInfo::FilterState::StateType::ReadOnly); - stream_info.filter_state_.setData("key-struct", - std::make_unique(), - StreamInfo::FilterState::StateType::ReadOnly); - stream_info.filter_state_.setData("key-no-serialization", - std::make_unique(), - StreamInfo::FilterState::StateType::ReadOnly); - stream_info.filter_state_.setData( + stream_info.filter_state_->setData("key", + std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly); + stream_info.filter_state_->setData("key-struct", + std::make_unique(), + StreamInfo::FilterState::StateType::ReadOnly); + stream_info.filter_state_->setData("key-no-serialization", + std::make_unique(), + StreamInfo::FilterState::StateType::ReadOnly); + stream_info.filter_state_->setData( "key-serialization-error", std::make_unique(std::chrono::seconds(-281474976710656)), StreamInfo::FilterState::StateType::ReadOnly); @@ -1299,11 +1299,12 @@ TEST(AccessLogFormatterTest, JsonFormatterTypedDynamicMetadataTest) { TEST(AccessLogFormatterTets, JsonFormatterFilterStateTest) { Http::TestHeaderMapImpl header; StreamInfo::MockStreamInfo stream_info; - stream_info.filter_state_.setData("test_key", - std::make_unique("test_value"), - StreamInfo::FilterState::StateType::ReadOnly); - stream_info.filter_state_.setData("test_obj", std::make_unique(), - StreamInfo::FilterState::StateType::ReadOnly); + stream_info.filter_state_->setData("test_key", + std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly); + stream_info.filter_state_->setData("test_obj", + std::make_unique(), + StreamInfo::FilterState::StateType::ReadOnly); EXPECT_CALL(Const(stream_info), filterState()).Times(testing::AtLeast(1)); std::unordered_map expected_json_map = { @@ -1320,11 +1321,12 @@ TEST(AccessLogFormatterTets, JsonFormatterFilterStateTest) { TEST(AccessLogFormatterTets, JsonFormatterTypedFilterStateTest) { Http::TestHeaderMapImpl header; StreamInfo::MockStreamInfo stream_info; - stream_info.filter_state_.setData("test_key", - std::make_unique("test_value"), - StreamInfo::FilterState::StateType::ReadOnly); - stream_info.filter_state_.setData("test_obj", std::make_unique(), - StreamInfo::FilterState::StateType::ReadOnly); + stream_info.filter_state_->setData("test_key", + std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly); + stream_info.filter_state_->setData("test_obj", + std::make_unique(), + StreamInfo::FilterState::StateType::ReadOnly); EXPECT_CALL(Const(stream_info), filterState()).Times(testing::AtLeast(1)); std::unordered_map key_mapping = { @@ -1414,9 +1416,9 @@ TEST(AccessLogFormatterTest, JsonFormatterTypedTest) { ProtobufWkt::Struct s; (*s.mutable_fields())["list"] = list; - stream_info.filter_state_.setData("test_obj", - std::make_unique(s), - StreamInfo::FilterState::StateType::ReadOnly); + stream_info.filter_state_->setData("test_obj", + std::make_unique(s), + StreamInfo::FilterState::StateType::ReadOnly); EXPECT_CALL(Const(stream_info), filterState()).Times(testing::AtLeast(1)); std::unordered_map key_mapping = { @@ -1491,14 +1493,14 @@ TEST(AccessLogFormatterTest, CompositeFormatterSuccess) { { EXPECT_CALL(Const(stream_info), filterState()).Times(testing::AtLeast(1)); - stream_info.filter_state_.setData("testing", - std::make_unique("test_value"), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); - stream_info.filter_state_.setData("serialized", - std::make_unique(), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); + stream_info.filter_state_->setData("testing", + std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); + stream_info.filter_state_->setData("serialized", + std::make_unique(), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); const std::string format = "%FILTER_STATE(testing)%|%FILTER_STATE(serialized)%|" "%FILTER_STATE(testing):8%|%FILTER_STATE(nonexisting)%"; FormatterImpl formatter(format); From 64b9fb946ac643bc2d52f943691865b91005477c Mon Sep 17 00:00:00 2001 From: gargnupur Date: Thu, 30 Jan 2020 11:37:13 -0800 Subject: [PATCH 13/17] Fix tests Signed-off-by: gargnupur --- test/common/router/header_formatter_test.cc | 58 ++++++++++--------- .../network/sni_cluster/sni_cluster_test.cc | 6 +- 2 files changed, 34 insertions(+), 30 deletions(-) diff --git a/test/common/router/header_formatter_test.cc b/test/common/router/header_formatter_test.cc index 635e556545f55..f7d19351b2575 100644 --- a/test/common/router/header_formatter_test.cc +++ b/test/common/router/header_formatter_test.cc @@ -570,33 +570,35 @@ TEST_F(StreamInfoHeaderFormatterTest, TestFormatWithUpstreamMetadataVariableMiss } TEST_F(StreamInfoHeaderFormatterTest, TestFormatWithPerRequestStateVariable) { - Envoy::StreamInfo::FilterStateImpl filter_state( - Envoy::StreamInfo::FilterState::LifeSpan::FilterChain); - filter_state.setData("testing", std::make_unique("test_value"), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); - EXPECT_EQ("test_value", filter_state.getDataReadOnly("testing").asString()); + Envoy::StreamInfo::FilterStateSharedPtr filter_state( + std::make_shared( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)); + filter_state->setData("testing", std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); + EXPECT_EQ("test_value", filter_state->getDataReadOnly("testing").asString()); NiceMock stream_info; ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state)); - ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(filter_state)); + ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(*filter_state)); testFormatting(stream_info, "PER_REQUEST_STATE(testing)", "test_value"); testFormatting(stream_info, "PER_REQUEST_STATE(testing2)", ""); - EXPECT_EQ("test_value", filter_state.getDataReadOnly("testing").asString()); + EXPECT_EQ("test_value", filter_state->getDataReadOnly("testing").asString()); } TEST_F(StreamInfoHeaderFormatterTest, TestFormatWithNonStringPerRequestStateVariable) { - Envoy::StreamInfo::FilterStateImpl filter_state( - Envoy::StreamInfo::FilterState::LifeSpan::FilterChain); - filter_state.setData("testing", std::make_unique(1), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); - EXPECT_EQ(1, filter_state.getDataReadOnly("testing").access()); + Envoy::StreamInfo::FilterStateSharedPtr filter_state( + std::make_shared( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)); + filter_state->setData("testing", std::make_unique(1), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); + EXPECT_EQ(1, filter_state->getDataReadOnly("testing").access()); NiceMock stream_info; ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state)); - ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(filter_state)); + ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(*filter_state)); testFormatting(stream_info, "PER_REQUEST_STATE(testing)", ""); } @@ -843,13 +845,14 @@ TEST(HeaderParserTest, TestParseInternal) { const SystemTime start_time(std::chrono::milliseconds(1522796769123)); ON_CALL(stream_info, startTime()).WillByDefault(Return(start_time)); - Envoy::StreamInfo::FilterStateImpl filter_state( - Envoy::StreamInfo::FilterState::LifeSpan::FilterChain); - filter_state.setData("testing", std::make_unique("test_value"), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); + Envoy::StreamInfo::FilterStateSharedPtr filter_state( + std::make_shared( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)); + filter_state->setData("testing", std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state)); - ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(filter_state)); + ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(*filter_state)); for (const auto& test_case : test_cases) { Protobuf::RepeatedPtrField to_add; @@ -1013,13 +1016,14 @@ request_headers_to_remove: ["x-nope"] )EOF")); ON_CALL(*host, metadata()).WillByDefault(Return(metadata)); - Envoy::StreamInfo::FilterStateImpl filter_state( - Envoy::StreamInfo::FilterState::LifeSpan::FilterChain); - filter_state.setData("testing", std::make_unique("test_value"), - StreamInfo::FilterState::StateType::ReadOnly, - StreamInfo::FilterState::LifeSpan::FilterChain); + Envoy::StreamInfo::FilterStateSharedPtr filter_state( + std::make_shared( + Envoy::StreamInfo::FilterState::LifeSpan::FilterChain)); + filter_state->setData("testing", std::make_unique("test_value"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::FilterChain); ON_CALL(stream_info, filterState()).WillByDefault(ReturnRef(filter_state)); - ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(filter_state)); + ON_CALL(Const(stream_info), filterState()).WillByDefault(ReturnRef(*filter_state)); req_header_parser->evaluateHeaders(header_map, stream_info); diff --git a/test/extensions/filters/network/sni_cluster/sni_cluster_test.cc b/test/extensions/filters/network/sni_cluster/sni_cluster_test.cc index 2625e70db449c..a047869ee0544 100644 --- a/test/extensions/filters/network/sni_cluster/sni_cluster_test.cc +++ b/test/extensions/filters/network/sni_cluster/sni_cluster_test.cc @@ -49,7 +49,7 @@ TEST(SniCluster, SetTcpProxyClusterOnlyIfSniIsPresent) { .WillByDefault(Return(EMPTY_STRING)); filter.onNewConnection(); - EXPECT_FALSE(stream_info.filterState().hasData( + EXPECT_FALSE(stream_info.filterState()->hasData( TcpProxy::PerConnectionCluster::key())); } @@ -59,11 +59,11 @@ TEST(SniCluster, SetTcpProxyClusterOnlyIfSniIsPresent) { .WillByDefault(Return("filter_state_cluster")); filter.onNewConnection(); - EXPECT_TRUE(stream_info.filterState().hasData( + EXPECT_TRUE(stream_info.filterState()->hasData( TcpProxy::PerConnectionCluster::key())); auto per_connection_cluster = - stream_info.filterState().getDataReadOnly( + stream_info.filterState()->getDataReadOnly( TcpProxy::PerConnectionCluster::key()); EXPECT_EQ(per_connection_cluster.value(), "filter_state_cluster"); } From ac0bb89835f85f48662a89930953457df2d7f4b7 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Fri, 31 Jan 2020 10:10:56 -0800 Subject: [PATCH 14/17] Remove null check for shared ptr Signed-off-by: gargnupur --- source/common/router/router.cc | 19 ++++++++----------- source/common/stream_info/stream_info_impl.h | 2 +- source/common/tcp_proxy/tcp_proxy.cc | 3 +-- .../http/grpc_stats/grpc_stats_filter.cc | 10 ++++------ .../network/common/redis/codec_impl.cc | 4 ++-- .../network/sni_cluster/sni_cluster.cc | 2 +- 6 files changed, 17 insertions(+), 23 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 7c4fa33c09ba5..a05a3844258f3 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -388,10 +388,9 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e // upstream. const StreamInfo::FilterStateSharedPtr& filter_state = callbacks_->streamInfo().filterState(); const DebugConfig* debug_config = - filter_state ? filter_state->hasData(DebugConfig::key()) - ? &(filter_state->getDataReadOnly(DebugConfig::key())) - : nullptr - : nullptr; + filter_state->hasData(DebugConfig::key()) + ? &(filter_state->getDataReadOnly(DebugConfig::key())) + : nullptr; // TODO: Maybe add a filter API for this. grpc_request_ = Grpc::Common::hasGrpcContentType(headers); @@ -525,7 +524,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e const auto parsed_authority = Http::Utility::parseAuthority(headers.Host()->value().getStringView()); const StreamInfo::FilterStateSharedPtr& filterState = callbacks_->streamInfo().filterState(); - if (!parsed_authority.is_ip_address_ && filterState) { + if (!parsed_authority.is_ip_address_) { // TODO: Add SAN verification here and use it from dynamic_forward_proxy // Update filter state with the host/authority to use for setting SNI in the transport // socket options. This is referenced during the getConnPool() call below. @@ -1371,15 +1370,13 @@ bool Filter::setupRedirect(const Http::HeaderMap& headers, UpstreamRequest& upst attempting_internal_redirect_with_complete_stream_ = upstream_request.upstream_timing_.last_upstream_rx_byte_received_ && downstream_end_stream_; - const StreamInfo::FilterStateSharedPtr& filter_state = callbacks_->streamInfo().filterState(); - // As with setupRetry, redirects are not supported for streaming requests yet. if (downstream_end_stream_ && !callbacks_->decodingBuffer() && // Redirects with body not yet supported. - location != nullptr && filter_state && - convertRequestHeadersForInternalRedirect(*downstream_headers_, *filter_state, - route_entry_->maxInternalRedirects(), *location, - *callbacks_->connection()) && + location != nullptr && + convertRequestHeadersForInternalRedirect( + *downstream_headers_, *callbacks_->streamInfo().filterState(), + route_entry_->maxInternalRedirects(), *location, *callbacks_->connection()) && callbacks_->recreateStream()) { cluster_->stats().upstream_internal_redirect_succeeded_total_.inc(); return true; diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index dcfcef7c25ff2..54ad67662d01f 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -261,7 +261,7 @@ struct StreamInfoImpl : public StreamInfo { bool health_check_request_{}; const Router::RouteEntry* route_entry_{}; envoy::config::core::v3::Metadata metadata_{}; - FilterStateSharedPtr filter_state_{}; + FilterStateSharedPtr filter_state_; std::string route_name_; private: diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 57252321b3b8b..406eeca599a49 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -168,8 +168,7 @@ Config::Config(const envoy::extensions::filters::network::tcp_proxy::v3::TcpProx RouteConstSharedPtr Config::getRegularRouteFromEntries(Network::Connection& connection) { // First check if the per-connection state to see if we need to route to a pre-selected cluster - if (connection.streamInfo().filterState() && - connection.streamInfo().filterState()->hasData( + if (connection.streamInfo().filterState()->hasData( PerConnectionCluster::key())) { const PerConnectionCluster& per_connection_cluster = connection.streamInfo().filterState()->getDataReadOnly( diff --git a/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc b/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc index 3204e7a52645d..49bf996862417 100644 --- a/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc +++ b/source/extensions/filters/http/grpc_stats/grpc_stats_filter.cc @@ -81,12 +81,10 @@ class GrpcStatsFilter : public Http::PassThroughFilter { if (filter_object_ == nullptr) { auto state = std::make_unique(); filter_object_ = state.get(); - if (decoder_callbacks_->streamInfo().filterState()) { - decoder_callbacks_->streamInfo().filterState()->setData( - HttpFilterNames::get().GrpcStats, std::move(state), - StreamInfo::FilterState::StateType::Mutable, - StreamInfo::FilterState::LifeSpan::FilterChain); - } + decoder_callbacks_->streamInfo().filterState()->setData( + HttpFilterNames::get().GrpcStats, std::move(state), + StreamInfo::FilterState::StateType::Mutable, + StreamInfo::FilterState::LifeSpan::FilterChain); } filter_object_->request_message_count = request_counter_.frameCount(); filter_object_->response_message_count = response_counter_.frameCount(); diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index 2b5e1917686c2..52c33e33632a0 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -316,8 +316,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( - const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator:: +operator!=(const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } diff --git a/source/extensions/filters/network/sni_cluster/sni_cluster.cc b/source/extensions/filters/network/sni_cluster/sni_cluster.cc index 649c478ebc985..eb1694647b75c 100644 --- a/source/extensions/filters/network/sni_cluster/sni_cluster.cc +++ b/source/extensions/filters/network/sni_cluster/sni_cluster.cc @@ -15,7 +15,7 @@ Network::FilterStatus SniClusterFilter::onNewConnection() { ENVOY_CONN_LOG(trace, "sni_cluster: new connection with server name {}", read_callbacks_->connection(), sni); - if (!sni.empty() && read_callbacks_->connection().streamInfo().filterState()) { + if (!sni.empty()) { // Set the tcp_proxy cluster to the same value as SNI. The data is mutable to allow // other filters to change it. read_callbacks_->connection().streamInfo().filterState()->setData( From c747fe8a7e7e1226ad89cfbffd67369f6b8e0905 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Fri, 31 Jan 2020 10:21:23 -0800 Subject: [PATCH 15/17] make it similar as before Signed-off-by: gargnupur --- source/common/router/router.cc | 16 +++++++++------- test/common/tcp_proxy/tcp_proxy_test.cc | 3 --- .../filters/http/grpc_stats/config_test.cc | 4 ---- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index a05a3844258f3..7823c3c2fcecd 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -523,14 +523,14 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e upstream_http_protocol_options.value().auto_sni()) { const auto parsed_authority = Http::Utility::parseAuthority(headers.Host()->value().getStringView()); - const StreamInfo::FilterStateSharedPtr& filterState = callbacks_->streamInfo().filterState(); if (!parsed_authority.is_ip_address_) { // TODO: Add SAN verification here and use it from dynamic_forward_proxy // Update filter state with the host/authority to use for setting SNI in the transport // socket options. This is referenced during the getConnPool() call below. - filterState->setData(Network::UpstreamServerName::key(), - std::make_unique(parsed_authority.host_), - StreamInfo::FilterState::StateType::Mutable); + callbacks_->streamInfo().filterState()->setData( + Network::UpstreamServerName::key(), + std::make_unique(parsed_authority.host_), + StreamInfo::FilterState::StateType::Mutable); } } @@ -1370,13 +1370,15 @@ bool Filter::setupRedirect(const Http::HeaderMap& headers, UpstreamRequest& upst attempting_internal_redirect_with_complete_stream_ = upstream_request.upstream_timing_.last_upstream_rx_byte_received_ && downstream_end_stream_; + const StreamInfo::FilterStateSharedPtr& filter_state = callbacks_->streamInfo().filterState(); + // As with setupRetry, redirects are not supported for streaming requests yet. if (downstream_end_stream_ && !callbacks_->decodingBuffer() && // Redirects with body not yet supported. location != nullptr && - convertRequestHeadersForInternalRedirect( - *downstream_headers_, *callbacks_->streamInfo().filterState(), - route_entry_->maxInternalRedirects(), *location, *callbacks_->connection()) && + convertRequestHeadersForInternalRedirect(*downstream_headers_, *filter_state, + route_entry_->maxInternalRedirects(), *location, + *callbacks_->connection()) && callbacks_->recreateStream()) { cluster_->stats().upstream_internal_redirect_succeeded_total_.inc(); return true; diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index 2769e37667037..7302febb26bfa 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -1776,7 +1776,6 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(UseClusterFromPerConnectionC initializeFilter(); NiceMock stream_info; - EXPECT_NE(nullptr, stream_info.filterState()); stream_info.filterState()->setData("envoy.tcp_proxy.cluster", std::make_unique("filter_state_cluster"), StreamInfo::FilterState::StateType::Mutable, @@ -1798,7 +1797,6 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(UpstreamServerName)) { initializeFilter(); NiceMock stream_info; - EXPECT_NE(nullptr, stream_info.filterState()); stream_info.filterState()->setData("envoy.network.upstream_server_name", std::make_unique("www.example.com"), StreamInfo::FilterState::StateType::ReadOnly, @@ -1834,7 +1832,6 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(ApplicationProtocols)) { initializeFilter(); NiceMock stream_info; - EXPECT_NE(nullptr, stream_info.filterState()); stream_info.filterState()->setData( Network::ApplicationProtocols::key(), std::make_unique(std::vector{"foo", "bar"}), diff --git a/test/extensions/filters/http/grpc_stats/config_test.cc b/test/extensions/filters/http/grpc_stats/config_test.cc index fa0ef0820645c..34389c2c6e60a 100644 --- a/test/extensions/filters/http/grpc_stats/config_test.cc +++ b/test/extensions/filters/http/grpc_stats/config_test.cc @@ -66,7 +66,6 @@ TEST_F(GrpcStatsFilterConfigTest, StatsHttp2HeaderOnlyResponse) { ->statsScope() .counter("grpc.lyft.users.BadCompanions.GetBadCompanions.total") .value()); - EXPECT_NE(nullptr, stream_info_.filterState()); EXPECT_FALSE(stream_info_.filterState()->hasDataWithName(HttpFilterNames::get().GrpcStats)); } @@ -91,7 +90,6 @@ TEST_F(GrpcStatsFilterConfigTest, StatsHttp2NormalResponse) { ->statsScope() .counter("grpc.lyft.users.BadCompanions.GetBadCompanions.total") .value()); - EXPECT_NE(nullptr, stream_info_.filterState()); EXPECT_FALSE(stream_info_.filterState()->hasDataWithName(HttpFilterNames::get().GrpcStats)); } @@ -114,7 +112,6 @@ TEST_F(GrpcStatsFilterConfigTest, StatsHttp2ContentTypeGrpcPlusProto) { ->statsScope() .counter("grpc.lyft.users.BadCompanions.GetBadCompanions.total") .value()); - EXPECT_NE(nullptr, stream_info_.filterState()); EXPECT_FALSE(stream_info_.filterState()->hasDataWithName(HttpFilterNames::get().GrpcStats)); } @@ -142,7 +139,6 @@ TEST_F(GrpcStatsFilterConfigTest, MessageCounts) { ->statsScope() .counter("grpc.lyft.users.BadCompanions.GetBadCompanions.response_message_count") .value()); - EXPECT_NE(nullptr, stream_info_.filterState()); const auto& data = stream_info_.filterState()->getDataReadOnly( HttpFilterNames::get().GrpcStats); EXPECT_EQ(2U, data.request_message_count); From 4fcd48c17e3f9f929c2be56134204ac5d1d5aab9 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Fri, 31 Jan 2020 10:22:44 -0800 Subject: [PATCH 16/17] remove fmt from source/extensions/filters/network/common/redis/codec_impl.cc Signed-off-by: gargnupur --- source/extensions/filters/network/common/redis/codec_impl.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/source/extensions/filters/network/common/redis/codec_impl.cc b/source/extensions/filters/network/common/redis/codec_impl.cc index 52c33e33632a0..2b5e1917686c2 100644 --- a/source/extensions/filters/network/common/redis/codec_impl.cc +++ b/source/extensions/filters/network/common/redis/codec_impl.cc @@ -316,8 +316,8 @@ RespValue::CompositeArray::CompositeArrayConstIterator::operator++() { return *this; } -bool RespValue::CompositeArray::CompositeArrayConstIterator:: -operator!=(const CompositeArrayConstIterator& rhs) const { +bool RespValue::CompositeArray::CompositeArrayConstIterator::operator!=( + const CompositeArrayConstIterator& rhs) const { return command_ != (rhs.command_) || &array_ != &(rhs.array_) || index_ != rhs.index_ || first_ != rhs.first_; } From 6acd1cfb95d62253f23fe8bd6eda086417cf7dd1 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Fri, 31 Jan 2020 10:25:06 -0800 Subject: [PATCH 17/17] remove nullptr check from tcp_proxy_test too Signed-off-by: gargnupur --- test/common/tcp_proxy/tcp_proxy_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index 7302febb26bfa..006258812f7e7 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -750,7 +750,6 @@ TEST(ConfigTest, PerConnectionClusterWithTopLevelMetadataMatchConfig) { HashedValue hv1(v1), hv2(v2); NiceMock connection; - EXPECT_NE(nullptr, connection.stream_info_.filterState()); connection.stream_info_.filterState()->setData( "envoy.tcp_proxy.cluster", std::make_unique("filter_state_cluster"), StreamInfo::FilterState::StateType::Mutable,