From 276def25087836b08cc28a645d75f48ea3b02e7e Mon Sep 17 00:00:00 2001 From: gargnupur Date: Thu, 6 Feb 2020 12:11:57 -0800 Subject: [PATCH 1/5] Add upstream and downstream info in parent read callbacks in tcp too Ref: https://github.com/istio/istio/issues/20802 Signed-off-by: gargnupur Add test Signed-off-by: gargnupur Use streaminfo from connection callback as compared to maintaining an extra one just for access logging Signed-off-by: gargnupur --- source/common/tcp_proxy/tcp_proxy.cc | 10 ++++-- source/common/tcp_proxy/tcp_proxy.h | 3 +- test/common/tcp_proxy/tcp_proxy_test.cc | 48 ++++++++++++++----------- test/mocks/stream_info/BUILD | 1 + test/mocks/stream_info/mocks.cc | 26 ++++++++++++-- test/mocks/stream_info/mocks.h | 4 +++ 6 files changed, 64 insertions(+), 28 deletions(-) diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index 6f23bd83abb55..d3b6c5a8d6b5c 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -204,10 +204,9 @@ UpstreamDrainManager& Config::drainManager() { return upstream_drain_manager_slot_->getTyped(); } -Filter::Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager, - TimeSource& time_source) +Filter::Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager, TimeSource&) : config_(config), cluster_manager_(cluster_manager), downstream_callbacks_(*this), - upstream_callbacks_(new UpstreamCallbacks(this)), stream_info_(time_source) { + upstream_callbacks_(new UpstreamCallbacks(this)) { ASSERT(config != nullptr); } @@ -292,6 +291,11 @@ void Filter::readDisableDownstream(bool disable) { } } +StreamInfo::StreamInfo& Filter::getStreamInfo() { + ASSERT(read_callbacks_ != nullptr); + return read_callbacks_->connection().streamInfo(); +} + void Filter::DownstreamCallbacks::onAboveWriteBufferHighWatermark() { ASSERT(!on_high_watermark_called_); on_high_watermark_called_ = true; diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h index cef36312dfe58..fab8b88015b05 100644 --- a/source/common/tcp_proxy/tcp_proxy.h +++ b/source/common/tcp_proxy/tcp_proxy.h @@ -302,7 +302,7 @@ class Filter : public Network::ReadFilter, bool on_high_watermark_called_{false}; }; - virtual StreamInfo::StreamInfo& getStreamInfo() { return stream_info_; } + virtual StreamInfo::StreamInfo& getStreamInfo(); protected: struct DownstreamCallbacks : public Network::ConnectionCallbacks { @@ -354,7 +354,6 @@ class Filter : public Network::ReadFilter, std::shared_ptr upstream_callbacks_; // shared_ptr required for passing as a // read filter. std::unique_ptr upstream_; - StreamInfo::StreamInfoImpl stream_info_; RouteConstSharedPtr route_; Network::TransportSocketOptionsSharedPtr transport_socket_options_; uint32_t connect_attempts_{}; diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index 70675d23ad9a8..6c9e2a544b468 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -953,8 +953,8 @@ class TcpProxyTest : public testing::Test { NiceMock factory_context_; ConfigSharedPtr config_; - std::unique_ptr filter_; NiceMock filter_callbacks_; + std::unique_ptr filter_; std::vector>> upstream_hosts_{}; std::vector>> upstream_connections_{}; std::vector>> @@ -1746,6 +1746,23 @@ TEST_F(TcpProxyTest, ShareFilterState) { .value()); } +// Tests that filter callback can access downstream and upstream address and ssl properties. +TEST_F(TcpProxyTest, AccessDownstreamAndUpstreamProperties) { + setup(1); + + raiseEventUpstreamConnected(0); + EXPECT_EQ(filter_callbacks_.connection().streamInfo().downstreamLocalAddress(), + filter_callbacks_.connection().localAddress()); + EXPECT_EQ(filter_callbacks_.connection().streamInfo().downstreamRemoteAddress(), + filter_callbacks_.connection().remoteAddress()); + EXPECT_EQ(filter_callbacks_.connection().streamInfo().downstreamSslConnection(), + filter_callbacks_.connection().ssl()); + EXPECT_EQ(filter_callbacks_.connection().streamInfo().upstreamLocalAddress(), + upstream_connections_.at(0)->localAddress()); + EXPECT_EQ(filter_callbacks_.connection().streamInfo().upstreamSslConnection(), + upstream_connections_.at(0)->streamInfo().downstreamSslConnection()); +} + class TcpProxyRoutingTest : public testing::Test { public: TcpProxyRoutingTest() = default; @@ -1826,13 +1843,10 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(UseClusterFromPerConnectionC setup(); 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); - ON_CALL(connection_, streamInfo()).WillByDefault(ReturnRef(stream_info)); - EXPECT_CALL(Const(connection_), streamInfo()).WillRepeatedly(ReturnRef(stream_info)); + connection_.streamInfo().filterState()->setData( + "envoy.tcp_proxy.cluster", std::make_unique("filter_state_cluster"), + StreamInfo::FilterState::StateType::Mutable, + StreamInfo::FilterState::LifeSpan::DownstreamConnection); // Expect filter to try to open a connection to specified cluster. EXPECT_CALL(factory_context_.cluster_manager_, @@ -1847,14 +1861,10 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(UpstreamServerName)) { setup(); 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); - - ON_CALL(connection_, streamInfo()).WillByDefault(ReturnRef(stream_info)); - EXPECT_CALL(Const(connection_), streamInfo()).WillRepeatedly(ReturnRef(stream_info)); + connection_.streamInfo().filterState()->setData( + "envoy.network.upstream_server_name", std::make_unique("www.example.com"), + StreamInfo::FilterState::StateType::ReadOnly, + StreamInfo::FilterState::LifeSpan::DownstreamConnection); // Expect filter to try to open a connection to a cluster with the transport socket options with // override-server-name @@ -1882,16 +1892,12 @@ TEST_F(TcpProxyRoutingTest, DEPRECATED_FEATURE_TEST(ApplicationProtocols)) { setup(); initializeFilter(); - NiceMock stream_info; - stream_info.filterState()->setData( + connection_.streamInfo().filterState()->setData( Network::ApplicationProtocols::key(), std::make_unique(std::vector{"foo", "bar"}), 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)); - // Expect filter to try to open a connection to a cluster with the transport socket options with // override-application-protocol EXPECT_CALL(factory_context_.cluster_manager_, tcpConnPoolForCluster(_, _, _)) diff --git a/test/mocks/stream_info/BUILD b/test/mocks/stream_info/BUILD index ef392a961be72..c6d56c5ac6541 100644 --- a/test/mocks/stream_info/BUILD +++ b/test/mocks/stream_info/BUILD @@ -16,6 +16,7 @@ envoy_cc_mock( "//include/envoy/stream_info:stream_info_interface", "//include/envoy/upstream:upstream_interface", "//test/mocks/upstream:host_mocks", + "//test/test_common:simulated_time_system_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc index f2bd5133d2318..701e31a899754 100644 --- a/test/mocks/stream_info/mocks.cc +++ b/test/mocks/stream_info/mocks.cc @@ -15,11 +15,17 @@ namespace Envoy { namespace StreamInfo { MockStreamInfo::MockStreamInfo() - : filter_state_(std::make_shared(FilterState::LifeSpan::FilterChain)), + : start_time_(ts_.systemTime()), + 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")) { - ON_CALL(*this, upstreamHost()).WillByDefault(ReturnPointee(&host_)); + ON_CALL(*this, setResponseFlag(_)).WillByDefault(Invoke([this](ResponseFlag response_flag) { + response_flags_ |= response_flag; + })); + ON_CALL(*this, onUpstreamHostSelected(_)) + .WillByDefault( + Invoke([this](Upstream::HostDescriptionConstSharedPtr host) { upstream_host_ = host; })); ON_CALL(*this, startTime()).WillByDefault(ReturnPointee(&start_time_)); ON_CALL(*this, startTimeMonotonic()).WillByDefault(ReturnPointee(&start_time_monotonic_)); ON_CALL(*this, lastDownstreamRxByteReceived()) @@ -37,6 +43,11 @@ MockStreamInfo::MockStreamInfo() ON_CALL(*this, lastDownstreamTxByteSent()) .WillByDefault(ReturnPointee(&last_downstream_tx_byte_sent_)); ON_CALL(*this, requestComplete()).WillByDefault(ReturnPointee(&end_time_)); + ON_CALL(*this, onRequestComplete()).WillByDefault(Invoke([this]() { + end_time_ = absl::make_optional( + std::chrono::duration_cast(ts_.systemTime() - start_time_) + .count()); + })); ON_CALL(*this, setUpstreamLocalAddress(_)) .WillByDefault( Invoke([this](const Network::Address::InstanceConstSharedPtr& upstream_local_address) { @@ -85,6 +96,17 @@ MockStreamInfo::MockStreamInfo() bytes_sent_ += bytes_sent; })); ON_CALL(*this, bytesSent()).WillByDefault(ReturnPointee(&bytes_sent_)); + ON_CALL(*this, hasResponseFlag(_)).WillByDefault(Invoke([this](ResponseFlag flag) { + return response_flags_ & flag; + })); + ON_CALL(*this, upstreamHost()).WillByDefault(Invoke([this]() { + if (upstream_host_) { + return upstream_host_; + } + ReturnPointee(&host_); + // Call should not reach here and is just to make compiler happy. + return upstream_host_; + })); ON_CALL(*this, dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(Const(*this), dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, filterState()).WillByDefault(ReturnRef(filter_state_)); diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index a064f4f0f8c06..78c2c79503a1b 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -6,6 +6,7 @@ #include "common/stream_info/filter_state_impl.h" #include "test/mocks/upstream/host.h" +#include "test/test_common/simulated_time_system.h" #include "gmock/gmock.h" @@ -91,6 +92,8 @@ class MockStreamInfo : public StreamInfo { std::shared_ptr> host_{ new testing::NiceMock()}; + Upstream::HostDescriptionConstSharedPtr upstream_host_{}; + Envoy::Event::SimulatedTimeSystem ts_; SystemTime start_time_; MonotonicTime start_time_monotonic_; absl::optional last_downstream_rx_byte_received_; @@ -104,6 +107,7 @@ class MockStreamInfo : public StreamInfo { absl::optional protocol_; absl::optional response_code_; absl::optional response_code_details_; + uint64_t response_flags_{}; envoy::config::core::v3::Metadata metadata_; FilterStateSharedPtr upstream_filter_state_; FilterStateSharedPtr filter_state_; From 0fab79cdbfc3b01f83a598738f4b11acf0be154b Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 12 Feb 2020 10:33:17 -0800 Subject: [PATCH 2/5] Fix tests Signed-off-by: gargnupur --- source/common/network/connection_impl.cc | 2 +- source/common/network/connection_impl.h | 2 +- test/common/http/conn_manager_impl_test.cc | 2 +- test/common/tcp_proxy/tcp_proxy_test.cc | 6 ++++++ test/mocks/stream_info/mocks.cc | 13 ++----------- test/mocks/stream_info/mocks.h | 1 - 6 files changed, 11 insertions(+), 15 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 91a6446a31f70..ecc89579d6f28 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -45,7 +45,7 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt TransportSocketPtr&& transport_socket, bool connected) : ConnectionImplBase(dispatcher, next_global_id_++), transport_socket_(std::move(transport_socket)), socket_(std::move(socket)), - filter_manager_(*this), stream_info_(dispatcher.timeSource()), + stream_info_(dispatcher.timeSource()), filter_manager_(*this), write_buffer_( dispatcher.getWatermarkFactory().create([this]() -> void { this->onLowWatermark(); }, [this]() -> void { this->onHighWatermark(); })), diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 450efa869e31f..9042087853e98 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -128,8 +128,8 @@ class ConnectionImpl : public ConnectionImplBase, public TransportSocketCallback TransportSocketPtr transport_socket_; ConnectionSocketPtr socket_; - FilterManagerImpl filter_manager_; StreamInfo::StreamInfoImpl stream_info_; + FilterManagerImpl filter_manager_; Buffer::OwnedImpl read_buffer_; // This must be a WatermarkBuffer, but as it is created by a factory the ConnectionImpl only has diff --git a/test/common/http/conn_manager_impl_test.cc b/test/common/http/conn_manager_impl_test.cc index c2db560ffb567..1c1ab602b867d 100644 --- a/test/common/http/conn_manager_impl_test.cc +++ b/test/common/http/conn_manager_impl_test.cc @@ -344,7 +344,7 @@ class HttpConnectionManagerImplTest : public testing::Test, public ConnectionMan bool shouldNormalizePath() const override { return normalize_path_; } bool shouldMergeSlashes() const override { return merge_slashes_; } - DangerousDeprecatedTestTime test_time_; + Envoy::Event::SimulatedTimeSystem test_time_; NiceMock route_config_provider_; std::shared_ptr route_config_{new NiceMock()}; NiceMock scoped_route_config_provider_; diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index 6c9e2a544b468..dd6b251e19bc9 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -827,6 +827,11 @@ class TcpProxyTest : public testing::Test { TcpProxyTest() { ON_CALL(*factory_context_.access_log_manager_.file_, write(_)) .WillByDefault(SaveArg<0>(&access_log_data_)); + ON_CALL(filter_callbacks_.connection_.stream_info_, onUpstreamHostSelected(_)) + .WillByDefault( + Invoke([this](Upstream::HostDescriptionConstSharedPtr host) { + upstream_host_ = host; })); + ON_CALL(filter_callbacks_.connection_.stream_info_, upstreamHost()).WillByDefault(ReturnPointee(&upstream_host_)); } ~TcpProxyTest() override { @@ -968,6 +973,7 @@ class TcpProxyTest : public testing::Test { Network::Address::InstanceConstSharedPtr upstream_remote_address_; std::list> new_connection_functions_; + Upstream::HostDescriptionConstSharedPtr upstream_host_{}; }; TEST_F(TcpProxyTest, DEPRECATED_FEATURE_TEST(DefaultRoutes)) { diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc index 701e31a899754..8500d7645061d 100644 --- a/test/mocks/stream_info/mocks.cc +++ b/test/mocks/stream_info/mocks.cc @@ -23,9 +23,6 @@ MockStreamInfo::MockStreamInfo() ON_CALL(*this, setResponseFlag(_)).WillByDefault(Invoke([this](ResponseFlag response_flag) { response_flags_ |= response_flag; })); - ON_CALL(*this, onUpstreamHostSelected(_)) - .WillByDefault( - Invoke([this](Upstream::HostDescriptionConstSharedPtr host) { upstream_host_ = host; })); ON_CALL(*this, startTime()).WillByDefault(ReturnPointee(&start_time_)); ON_CALL(*this, startTimeMonotonic()).WillByDefault(ReturnPointee(&start_time_monotonic_)); ON_CALL(*this, lastDownstreamRxByteReceived()) @@ -99,14 +96,8 @@ MockStreamInfo::MockStreamInfo() ON_CALL(*this, hasResponseFlag(_)).WillByDefault(Invoke([this](ResponseFlag flag) { return response_flags_ & flag; })); - ON_CALL(*this, upstreamHost()).WillByDefault(Invoke([this]() { - if (upstream_host_) { - return upstream_host_; - } - ReturnPointee(&host_); - // Call should not reach here and is just to make compiler happy. - return upstream_host_; - })); + ON_CALL(*this, upstreamHost()).WillByDefault(ReturnPointee(&host_)); + ON_CALL(*this, dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(Const(*this), dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, filterState()).WillByDefault(ReturnRef(filter_state_)); diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index 78c2c79503a1b..dfc547ff75a7f 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -92,7 +92,6 @@ class MockStreamInfo : public StreamInfo { std::shared_ptr> host_{ new testing::NiceMock()}; - Upstream::HostDescriptionConstSharedPtr upstream_host_{}; Envoy::Event::SimulatedTimeSystem ts_; SystemTime start_time_; MonotonicTime start_time_monotonic_; From a7b0412f79fcc13c93ce685fef17254005b9a380 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 12 Feb 2020 12:06:04 -0800 Subject: [PATCH 3/5] Fix fmt Signed-off-by: gargnupur --- source/common/network/connection_impl.cc | 2 +- test/common/tcp_proxy/tcp_proxy_test.cc | 8 ++++---- test/mocks/stream_info/mocks.cc | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index ecc89579d6f28..16a9caf32e971 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -45,7 +45,7 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt TransportSocketPtr&& transport_socket, bool connected) : ConnectionImplBase(dispatcher, next_global_id_++), transport_socket_(std::move(transport_socket)), socket_(std::move(socket)), - stream_info_(dispatcher.timeSource()), filter_manager_(*this), + stream_info_(dispatcher.timeSource()), filter_manager_(*this), write_buffer_( dispatcher.getWatermarkFactory().create([this]() -> void { this->onLowWatermark(); }, [this]() -> void { this->onHighWatermark(); })), diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index dd6b251e19bc9..7710593978260 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -828,10 +828,10 @@ class TcpProxyTest : public testing::Test { ON_CALL(*factory_context_.access_log_manager_.file_, write(_)) .WillByDefault(SaveArg<0>(&access_log_data_)); ON_CALL(filter_callbacks_.connection_.stream_info_, onUpstreamHostSelected(_)) - .WillByDefault( - Invoke([this](Upstream::HostDescriptionConstSharedPtr host) { - upstream_host_ = host; })); - ON_CALL(filter_callbacks_.connection_.stream_info_, upstreamHost()).WillByDefault(ReturnPointee(&upstream_host_)); + .WillByDefault(Invoke( + [this](Upstream::HostDescriptionConstSharedPtr host) { upstream_host_ = host; })); + ON_CALL(filter_callbacks_.connection_.stream_info_, upstreamHost()) + .WillByDefault(ReturnPointee(&upstream_host_)); } ~TcpProxyTest() override { diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc index 8500d7645061d..dce47cbaf2c94 100644 --- a/test/mocks/stream_info/mocks.cc +++ b/test/mocks/stream_info/mocks.cc @@ -97,7 +97,7 @@ MockStreamInfo::MockStreamInfo() return response_flags_ & flag; })); ON_CALL(*this, upstreamHost()).WillByDefault(ReturnPointee(&host_)); - + ON_CALL(*this, dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(Const(*this), dynamicMetadata()).WillByDefault(ReturnRef(metadata_)); ON_CALL(*this, filterState()).WillByDefault(ReturnRef(filter_state_)); From de05ee0321d0ef8aa9639cb90b3c8dde66d9c3c5 Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 12 Feb 2020 15:56:35 -0800 Subject: [PATCH 4/5] Fixed based on feedback and fix tests Signed-off-by: gargnupur --- source/common/tcp_proxy/tcp_proxy.cc | 3 +-- source/common/tcp_proxy/tcp_proxy.h | 3 +-- .../extensions/filters/network/tcp_proxy/config.cc | 4 ++-- test/common/network/filter_manager_impl_test.cc | 3 +-- test/common/stream_info/BUILD | 2 +- test/common/stream_info/test_util.h | 4 ++-- test/common/tcp_proxy/tcp_proxy_test.cc | 14 +++++++------- .../grpc/http_grpc_access_log_impl_test.cc | 4 +++- .../extensions/filters/http/lua/lua_filter_test.cc | 2 +- test/extensions/filters/http/lua/wrappers_test.cc | 2 +- .../filters/network/tcp_proxy/config_test.cc | 13 +++++++++++-- .../tracers/zipkin/zipkin_tracer_impl_test.cc | 2 +- 12 files changed, 32 insertions(+), 24 deletions(-) diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc index d3b6c5a8d6b5c..bd299ff35c9e3 100644 --- a/source/common/tcp_proxy/tcp_proxy.cc +++ b/source/common/tcp_proxy/tcp_proxy.cc @@ -204,7 +204,7 @@ UpstreamDrainManager& Config::drainManager() { return upstream_drain_manager_slot_->getTyped(); } -Filter::Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager, TimeSource&) +Filter::Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager) : config_(config), cluster_manager_(cluster_manager), downstream_callbacks_(*this), upstream_callbacks_(new UpstreamCallbacks(this)) { ASSERT(config != nullptr); @@ -292,7 +292,6 @@ void Filter::readDisableDownstream(bool disable) { } StreamInfo::StreamInfo& Filter::getStreamInfo() { - ASSERT(read_callbacks_ != nullptr); return read_callbacks_->connection().streamInfo(); } diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h index fab8b88015b05..92c2bac4c6e36 100644 --- a/source/common/tcp_proxy/tcp_proxy.h +++ b/source/common/tcp_proxy/tcp_proxy.h @@ -229,8 +229,7 @@ class Filter : public Network::ReadFilter, Tcp::ConnectionPool::Callbacks, protected Logger::Loggable { public: - Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager, - TimeSource& time_source); + Filter(ConfigSharedPtr config, Upstream::ClusterManager& cluster_manager); ~Filter() override; // Network::ReadFilter diff --git a/source/extensions/filters/network/tcp_proxy/config.cc b/source/extensions/filters/network/tcp_proxy/config.cc index 5dd5c3a116b62..6eb06da1f6811 100644 --- a/source/extensions/filters/network/tcp_proxy/config.cc +++ b/source/extensions/filters/network/tcp_proxy/config.cc @@ -22,8 +22,8 @@ Network::FilterFactoryCb ConfigFactory::createFilterFactoryFromProtoTyped( Envoy::TcpProxy::ConfigSharedPtr filter_config( std::make_shared(proto_config, context)); return [filter_config, &context](Network::FilterManager& filter_manager) -> void { - filter_manager.addReadFilter(std::make_shared( - filter_config, context.clusterManager(), context.dispatcher().timeSource())); + filter_manager.addReadFilter( + std::make_shared(filter_config, context.clusterManager())); }; } diff --git a/test/common/network/filter_manager_impl_test.cc b/test/common/network/filter_manager_impl_test.cc index 3dd955f5864e6..f8dea3442a22f 100644 --- a/test/common/network/filter_manager_impl_test.cc +++ b/test/common/network/filter_manager_impl_test.cc @@ -397,8 +397,7 @@ stat_prefix: name tcp_proxy.set_cluster("fake_cluster"); TcpProxy::ConfigSharedPtr tcp_proxy_config(new TcpProxy::Config(tcp_proxy, factory_context)); manager.addReadFilter( - std::make_shared(tcp_proxy_config, factory_context.cluster_manager_, - factory_context.dispatcher().timeSource())); + std::make_shared(tcp_proxy_config, factory_context.cluster_manager_)); Extensions::Filters::Common::RateLimit::RequestCallbacks* request_callbacks{}; EXPECT_CALL(*rl_client, limit(_, "foo", diff --git a/test/common/stream_info/BUILD b/test/common/stream_info/BUILD index 6b8f159e8667b..9c60f05c1c2b1 100644 --- a/test/common/stream_info/BUILD +++ b/test/common/stream_info/BUILD @@ -46,7 +46,7 @@ envoy_cc_test_library( "//include/envoy/stream_info:stream_info_interface", "//source/common/common:assert_lib", "//source/common/stream_info:filter_state_lib", - "//test/test_common:test_time_lib", + "//test/test_common:simulated_time_system_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", ], ) diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index 7d3805f07696b..cba0355f4bf34 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -6,7 +6,7 @@ #include "common/common/assert.h" #include "common/stream_info/filter_state_impl.h" -#include "test/test_common/test_time.h" +#include "test/test_common/simulated_time_system.h" namespace Envoy { @@ -242,7 +242,7 @@ class TestStreamInfo : public StreamInfo::StreamInfo { std::string requested_server_name_; std::string upstream_transport_failure_reason_; const Http::HeaderMap* request_headers_{}; - DangerousDeprecatedTestTime test_time_; + Envoy::Event::SimulatedTimeSystem test_time_; }; } // namespace Envoy diff --git a/test/common/tcp_proxy/tcp_proxy_test.cc b/test/common/tcp_proxy/tcp_proxy_test.cc index 7710593978260..80d676f5b97a0 100644 --- a/test/common/tcp_proxy/tcp_proxy_test.cc +++ b/test/common/tcp_proxy/tcp_proxy_test.cc @@ -910,7 +910,7 @@ class TcpProxyTest : public testing::Test { } { - filter_ = std::make_unique(config_, factory_context_.cluster_manager_, timeSystem()); + filter_ = std::make_unique(config_, factory_context_.cluster_manager_); EXPECT_CALL(filter_callbacks_.connection_, enableHalfClose(true)); EXPECT_CALL(filter_callbacks_.connection_, readDisable(true)); filter_->initializeReadFilterCallbacks(filter_callbacks_); @@ -1259,7 +1259,7 @@ TEST_F(TcpProxyTest, DEPRECATED_FEATURE_TEST(RouteWithMetadataMatch)) { {Envoy::Config::MetadataFilters::get().ENVOY_LB, metadata_struct}); configure(config); - filter_ = std::make_unique(config_, factory_context_.cluster_manager_, timeSystem()); + filter_ = std::make_unique(config_, factory_context_.cluster_manager_); filter_->initializeReadFilterCallbacks(filter_callbacks_); EXPECT_EQ(Network::FilterStatus::StopIteration, filter_->onNewConnection()); @@ -1307,7 +1307,7 @@ TEST_F(TcpProxyTest, WeightedClusterWithMetadataMatch) { v2.set_string_value("v2"); HashedValue hv0(v0), hv1(v1), hv2(v2); - filter_ = std::make_unique(config_, factory_context_.cluster_manager_, timeSystem()); + filter_ = std::make_unique(config_, factory_context_.cluster_manager_); filter_->initializeReadFilterCallbacks(filter_callbacks_); // Expect filter to try to open a connection to cluster1. @@ -1361,7 +1361,7 @@ TEST_F(TcpProxyTest, WeightedClusterWithMetadataMatch) { TEST_F(TcpProxyTest, DEPRECATED_FEATURE_TEST(DisconnectBeforeData)) { configure(defaultConfig()); - filter_ = std::make_unique(config_, factory_context_.cluster_manager_, timeSystem()); + filter_ = std::make_unique(config_, factory_context_.cluster_manager_); filter_->initializeReadFilterCallbacks(filter_callbacks_); filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose); @@ -1400,7 +1400,7 @@ TEST_F(TcpProxyTest, DEPRECATED_FEATURE_TEST(UpstreamConnectionLimit)) { 0, 0, 0, 0, 0); // setup sets up expectation for tcpConnForCluster but this test is expected to NOT call that - filter_ = std::make_unique(config_, factory_context_.cluster_manager_, timeSystem()); + filter_ = std::make_unique(config_, factory_context_.cluster_manager_); // The downstream connection closes if the proxy can't make an upstream connection. EXPECT_CALL(filter_callbacks_.connection_, close(Network::ConnectionCloseType::NoFlush)); filter_->initializeReadFilterCallbacks(filter_callbacks_); @@ -1789,7 +1789,7 @@ class TcpProxyRoutingTest : public testing::Test { void initializeFilter() { EXPECT_CALL(filter_callbacks_, connection()).WillRepeatedly(ReturnRef(connection_)); - filter_ = std::make_unique(config_, factory_context_.cluster_manager_, timeSystem()); + filter_ = std::make_unique(config_, factory_context_.cluster_manager_); filter_->initializeReadFilterCallbacks(filter_callbacks_); } @@ -1945,7 +1945,7 @@ class TcpProxyHashingTest : public testing::Test { void initializeFilter() { EXPECT_CALL(filter_callbacks_, connection()).WillRepeatedly(ReturnRef(connection_)); - filter_ = std::make_unique(config_, factory_context_.cluster_manager_, timeSystem()); + filter_ = std::make_unique(config_, factory_context_.cluster_manager_); filter_->initializeReadFilterCallbacks(filter_callbacks_); } 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 2afcb68817d80..31d506ae55698 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 @@ -85,6 +85,7 @@ class HttpGrpcAccessLogTest : public testing::Test { void expectLogRequestMethod(const std::string& request_method) { NiceMock stream_info; stream_info.host_ = nullptr; + stream_info.start_time_ = SystemTime(1h); Http::TestHeaderMapImpl request_headers{ {":method", request_method}, @@ -104,7 +105,8 @@ class HttpGrpcAccessLogTest : public testing::Test { socket_address: address: "127.0.0.2" port_value: 0 - start_time: {{}} + start_time: + seconds: 3600 request: request_method: {} request_headers_bytes: {} diff --git a/test/extensions/filters/http/lua/lua_filter_test.cc b/test/extensions/filters/http/lua/lua_filter_test.cc index 941f4806d762e..5262528193d11 100644 --- a/test/extensions/filters/http/lua/lua_filter_test.cc +++ b/test/extensions/filters/http/lua/lua_filter_test.cc @@ -1558,7 +1558,7 @@ TEST_F(LuaHttpFilterTest, SetGetDynamicMetadata) { setup(SCRIPT); Http::TestHeaderMapImpl request_headers{{":path", "/"}}; - DangerousDeprecatedTestTime test_time; + Event::SimulatedTimeSystem test_time; StreamInfo::StreamInfoImpl stream_info(Http::Protocol::Http2, test_time.timeSystem()); EXPECT_EQ(0, stream_info.dynamicMetadata().filter_metadata_size()); EXPECT_CALL(decoder_callbacks_, streamInfo()).WillOnce(ReturnRef(stream_info)); diff --git a/test/extensions/filters/http/lua/wrappers_test.cc b/test/extensions/filters/http/lua/wrappers_test.cc index 31447f450807d..32c003951e1f4 100644 --- a/test/extensions/filters/http/lua/wrappers_test.cc +++ b/test/extensions/filters/http/lua/wrappers_test.cc @@ -254,7 +254,7 @@ class LuaStreamInfoWrapperTest return metadata; } - DangerousDeprecatedTestTime test_time_; + Event::SimulatedTimeSystem test_time_; }; // Return the current request protocol. diff --git a/test/extensions/filters/network/tcp_proxy/config_test.cc b/test/extensions/filters/network/tcp_proxy/config_test.cc index 43cc30bbcb7e8..d38d88d2702bd 100644 --- a/test/extensions/filters/network/tcp_proxy/config_test.cc +++ b/test/extensions/filters/network/tcp_proxy/config_test.cc @@ -97,7 +97,11 @@ TEST_P(RouteIpListConfigTest, DEPRECATED_FEATURE_TEST(TcpProxy)) { ConfigFactory factory; Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(proto_config, context); Network::MockConnection connection; - EXPECT_CALL(connection, addReadFilter(_)); + NiceMock readFilterCallback; + EXPECT_CALL(connection, addReadFilter(_)) + .WillRepeatedly(Invoke([&readFilterCallback](Network::ReadFilterSharedPtr filter) { + filter->initializeReadFilterCallbacks(readFilterCallback); + })); cb(connection); } @@ -119,9 +123,14 @@ TEST(ConfigTest, ConfigTest) { config.set_cluster("cluster"); EXPECT_TRUE(factory.isTerminalFilter()); + Network::FilterFactoryCb cb = factory.createFilterFactoryFromProto(config, context); Network::MockConnection connection; - EXPECT_CALL(connection, addReadFilter(_)); + NiceMock readFilterCallback; + EXPECT_CALL(connection, addReadFilter(_)) + .WillRepeatedly(Invoke([&readFilterCallback](Network::ReadFilterSharedPtr filter) { + filter->initializeReadFilterCallbacks(readFilterCallback); + })); cb(connection); } diff --git a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc index 5f060f7dd45de..c7965b81421e4 100644 --- a/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc +++ b/test/extensions/tracers/zipkin/zipkin_tracer_impl_test.cc @@ -145,7 +145,7 @@ class ZipkinDriverTest : public testing::Test { NiceMock random_; NiceMock config_; - DangerousDeprecatedTestTime test_time_; + Event::SimulatedTimeSystem test_time_; TimeSource& time_source_; }; From 870db7e1634db81dc89817d51293e38ab82642bb Mon Sep 17 00:00:00 2001 From: gargnupur Date: Wed, 12 Feb 2020 23:56:58 -0800 Subject: [PATCH 5/5] Fix health check test Signed-off-by: gargnupur --- test/common/upstream/health_checker_impl_test.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/test/common/upstream/health_checker_impl_test.cc b/test/common/upstream/health_checker_impl_test.cc index 0ea1169811e63..755c62729fee4 100644 --- a/test/common/upstream/health_checker_impl_test.cc +++ b/test/common/upstream/health_checker_impl_test.cc @@ -1322,7 +1322,6 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithAdditionalHeaders) { key: value )EOF"); - std::string current_start_time; cluster_->prioritySet().getMockHostSet(0)->hosts_ = { makeTestHost(cluster_->info_, "tcp://127.0.0.1:80", metadata)}; cluster_->info_->stats().upstream_cx_total_.inc(); @@ -1348,8 +1347,10 @@ TEST_F(HttpHealthCheckerImplTest, SuccessServiceCheckWithAdditionalHeaders) { EXPECT_EQ(headers.get(downstream_local_address_without_port)->value().getStringView(), value_downstream_local_address_without_port); - EXPECT_NE(headers.get(start_time)->value().getStringView(), current_start_time); - current_start_time = std::string(headers.get(start_time)->value().getStringView()); + Envoy::DateFormatter date_formatter("%s.%9f"); + std::string current_start_time = + date_formatter.fromTime(dispatcher_.timeSource().systemTime()); + EXPECT_EQ(headers.get(start_time)->value().getStringView(), current_start_time); })); health_checker_->start();