From 9007438f87a9843584257397c64e696db38a8522 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 27 Jul 2021 14:11:31 -0700 Subject: [PATCH 01/16] wip Signed-off-by: Jose Nino --- envoy/stream_info/stream_info.h | 4 ++++ source/common/http/conn_pool_base.cc | 1 + source/common/router/router.cc | 2 +- source/common/stream_info/stream_info_impl.h | 9 +++++++++ 4 files changed, 15 insertions(+), 1 deletion(-) diff --git a/envoy/stream_info/stream_info.h b/envoy/stream_info/stream_info.h index 7a67ff946f180..6d4bce10beca9 100644 --- a/envoy/stream_info/stream_info.h +++ b/envoy/stream_info/stream_info.h @@ -242,6 +242,10 @@ class StreamInfo { public: virtual ~StreamInfo() = default; + virtual void setUpstreamConnectionId(uint64_t id) PURE; + + virtual absl::optional upstreamConnectionId() PURE; + /** * @param response_flag the response flag. Each filter can set independent response flags. The * flags are accumulated. diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc index 4a67bea9da859..e2a97f7b8a671 100644 --- a/source/common/http/conn_pool_base.cc +++ b/source/common/http/conn_pool_base.cc @@ -84,6 +84,7 @@ void HttpConnPoolImplBase::onPoolReady(Envoy::ConnectionPool::ActiveClient& clie Http::ResponseDecoder& response_decoder = *http_context.decoder_; Http::ConnectionPool::Callbacks& callbacks = *http_context.callbacks_; Http::RequestEncoder& new_encoder = http_client->newStreamEncoder(response_decoder); + http_client->codec_client_->streamInfo().setUpstreamConnectionId(id()); callbacks.onPoolReady(new_encoder, client.real_host_description_, http_client->codec_client_->streamInfo(), http_client->codec_client_->protocol()); diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 0a5bb031136bb..156b0ed6e4f71 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -1231,7 +1231,7 @@ void Filter::resetOtherUpstreams(UpstreamRequest& upstream_request) { void Filter::onUpstreamHeaders(uint64_t response_code, Http::ResponseHeaderMapPtr&& headers, UpstreamRequest& upstream_request, bool end_stream) { - ENVOY_STREAM_LOG(debug, "upstream headers complete: end_stream={}", *callbacks_, end_stream); + ENVOY_STREAM_LOG(debug, "upstream headers complete: end_stream={} upstream_conn={}", *callbacks_, end_stream, callbacks_->streamInfo().upstreamConnectionId().value_or(0)); modify_headers_(*headers); // When grpc-status appears in response headers, convert grpc-status to HTTP status code diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index 53d4351749c10..4f0b2cbabf0f1 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -68,6 +68,14 @@ struct StreamInfoImpl : public StreamInfo { start_time_monotonic_); } + void setUpstreamConnectionId(uint64_t id) override { + upstream_connection_id_ = id; + } + + absl::optional upstreamConnectionId() const override { + return upstream_connection_id_; + } + absl::optional lastDownstreamRxByteReceived() const override { return duration(last_downstream_rx_byte_received); } @@ -301,6 +309,7 @@ struct StreamInfoImpl : public StreamInfo { FilterStateSharedPtr filter_state_; FilterStateSharedPtr upstream_filter_state_; std::string route_name_; + absl::optional upstream_connection_id_; private: static Network::SocketAddressProviderSharedPtr emptyDownstreamAddressProvider() { From d5b6e79ec851c7def100879468248069a9e52c7f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 27 Jul 2021 15:30:39 -0700 Subject: [PATCH 02/16] wip Signed-off-by: Jose Nino --- envoy/stream_info/stream_info.h | 2 +- source/common/http/conn_pool_base.cc | 1 - source/common/network/connection_impl.cc | 2 ++ source/common/router/upstream_request.cc | 3 +++ 4 files changed, 6 insertions(+), 2 deletions(-) diff --git a/envoy/stream_info/stream_info.h b/envoy/stream_info/stream_info.h index 6d4bce10beca9..2148f8ef0b6a8 100644 --- a/envoy/stream_info/stream_info.h +++ b/envoy/stream_info/stream_info.h @@ -244,7 +244,7 @@ class StreamInfo { virtual void setUpstreamConnectionId(uint64_t id) PURE; - virtual absl::optional upstreamConnectionId() PURE; + virtual absl::optional upstreamConnectionId() const PURE; /** * @param response_flag the response flag. Each filter can set independent response flags. The diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc index e2a97f7b8a671..4a67bea9da859 100644 --- a/source/common/http/conn_pool_base.cc +++ b/source/common/http/conn_pool_base.cc @@ -84,7 +84,6 @@ void HttpConnPoolImplBase::onPoolReady(Envoy::ConnectionPool::ActiveClient& clie Http::ResponseDecoder& response_decoder = *http_context.decoder_; Http::ConnectionPool::Callbacks& callbacks = *http_context.callbacks_; Http::RequestEncoder& new_encoder = http_client->newStreamEncoder(response_decoder); - http_client->codec_client_->streamInfo().setUpstreamConnectionId(id()); callbacks.onPoolReady(new_encoder, client.real_host_description_, http_client->codec_client_->streamInfo(), http_client->codec_client_->protocol()); diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 5325dab742648..c69004380ea40 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -824,6 +824,8 @@ ClientConnectionImpl::ClientConnectionImpl( : ConnectionImpl(dispatcher, std::make_unique(remote_address, options), std::move(transport_socket), stream_info_, false), stream_info_(dispatcher.timeSource(), socket_->addressProviderSharedPtr()) { + + stream_info_.setUpstreamConnectionId(id()); // There are no meaningful socket options or source address semantics for // non-IP sockets, so skip. if (remote_address->ip() == nullptr) { diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 03efc074b9a96..3137686ef405d 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -420,6 +420,9 @@ void UpstreamRequest::onPoolReady( stream_info_.setUpstreamSslConnection(info.downstreamSslConnection()); parent_.callbacks()->streamInfo().setUpstreamSslConnection(info.downstreamSslConnection()); + stream_info_.setUpstreamConnectionId(info.upstreamConnectionId().value_or(0)); + parent_.callbacks()->streamInfo().setUpstreamConnectionId(info.upstreamConnectionId().value_or(0)); + if (parent_.downstreamEndStream()) { setupPerTryTimeout(); } else { From f0474f496d92029a496a61d8b57d74cc945c1785 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 27 Jul 2021 16:35:33 -0700 Subject: [PATCH 03/16] delete log test Signed-off-by: Jose Nino --- source/common/router/router.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 156b0ed6e4f71..0a5bb031136bb 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -1231,7 +1231,7 @@ void Filter::resetOtherUpstreams(UpstreamRequest& upstream_request) { void Filter::onUpstreamHeaders(uint64_t response_code, Http::ResponseHeaderMapPtr&& headers, UpstreamRequest& upstream_request, bool end_stream) { - ENVOY_STREAM_LOG(debug, "upstream headers complete: end_stream={} upstream_conn={}", *callbacks_, end_stream, callbacks_->streamInfo().upstreamConnectionId().value_or(0)); + ENVOY_STREAM_LOG(debug, "upstream headers complete: end_stream={}", *callbacks_, end_stream); modify_headers_(*headers); // When grpc-status appears in response headers, convert grpc-status to HTTP status code From 7c3eb0bde6c68039eabf0d7b80e79375417cf169 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 27 Jul 2021 16:38:25 -0700 Subject: [PATCH 04/16] optional Signed-off-by: Jose Nino --- source/common/router/upstream_request.cc | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 3137686ef405d..434612438653a 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -420,8 +420,10 @@ void UpstreamRequest::onPoolReady( stream_info_.setUpstreamSslConnection(info.downstreamSslConnection()); parent_.callbacks()->streamInfo().setUpstreamSslConnection(info.downstreamSslConnection()); - stream_info_.setUpstreamConnectionId(info.upstreamConnectionId().value_or(0)); - parent_.callbacks()->streamInfo().setUpstreamConnectionId(info.upstreamConnectionId().value_or(0)); + if (info.upstreamConnectionId().has_value()) { + stream_info_.setUpstreamConnectionId(info.upstreamConnectionId().value()); + parent_.callbacks()->streamInfo().setUpstreamConnectionId(info.upstreamConnectionId().value()); + } if (parent_.downstreamEndStream()) { setupPerTryTimeout(); From 6fd4ab620dc73596c2c787fdccfb1cf69e59d5a6 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 27 Jul 2021 16:42:30 -0700 Subject: [PATCH 05/16] fmt Signed-off-by: Jose Nino --- source/common/stream_info/stream_info_impl.h | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index 4f0b2cbabf0f1..c0b35ab1cacfe 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -68,13 +68,9 @@ struct StreamInfoImpl : public StreamInfo { start_time_monotonic_); } - void setUpstreamConnectionId(uint64_t id) override { - upstream_connection_id_ = id; - } + void setUpstreamConnectionId(uint64_t id) override { upstream_connection_id_ = id; } - absl::optional upstreamConnectionId() const override { - return upstream_connection_id_; - } + absl::optional upstreamConnectionId() const override { return upstream_connection_id_; } absl::optional lastDownstreamRxByteReceived() const override { return duration(last_downstream_rx_byte_received); From 3af163edc0ac9db173cd50d5af35df51bdcfe84c Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 30 Jul 2021 15:14:31 -0700 Subject: [PATCH 06/16] clean up and tests Signed-off-by: Jose Nino --- envoy/stream_info/stream_info.h | 14 ++++++++++---- test/common/router/router_test.cc | 1 + test/common/stream_info/stream_info_impl_test.cc | 5 +++++ test/common/stream_info/test_util.h | 6 +++++- test/mocks/stream_info/mocks.cc | 6 ++++++ test/mocks/stream_info/mocks.h | 3 +++ 6 files changed, 30 insertions(+), 5 deletions(-) diff --git a/envoy/stream_info/stream_info.h b/envoy/stream_info/stream_info.h index 2148f8ef0b6a8..24928d36cce58 100644 --- a/envoy/stream_info/stream_info.h +++ b/envoy/stream_info/stream_info.h @@ -242,10 +242,6 @@ class StreamInfo { public: virtual ~StreamInfo() = default; - virtual void setUpstreamConnectionId(uint64_t id) PURE; - - virtual absl::optional upstreamConnectionId() const PURE; - /** * @param response_flag the response flag. Each filter can set independent response flags. The * flags are accumulated. @@ -597,6 +593,16 @@ class StreamInfo { * @return Network filter chain name of the downstream connection. */ virtual const std::string& filterChainName() const PURE; + + /** + * @param connection ID of the upstream connection. + */ + virtual void setUpstreamConnectionId(uint64_t id) PURE; + + /** + * @return the ID of the upstream connection, nullopt if not available. + */ + virtual absl::optional upstreamConnectionId() const PURE; }; } // namespace StreamInfo diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index badfe10da7024..6334beddae881 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4870,6 +4870,7 @@ TEST_F(RouterTest, UpstreamSSLConnection) { ASSERT_NE(nullptr, callbacks_.streamInfo().upstreamSslConnection()); EXPECT_EQ(session_id, callbacks_.streamInfo().upstreamSslConnection()->sessionId()); + EXPECT_FALSE(callbacks_.streamInfo().upstreamConnectionId().has_value()); } // Verify that upstream timing information is set into the StreamInfo after the upstream diff --git a/test/common/stream_info/stream_info_impl_test.cc b/test/common/stream_info/stream_info_impl_test.cc index 6ff19887f4fc7..e299c57399f1e 100644 --- a/test/common/stream_info/stream_info_impl_test.cc +++ b/test/common/stream_info/stream_info_impl_test.cc @@ -192,6 +192,11 @@ TEST_F(StreamInfoImplTest, MiscSettersAndGetters) { EXPECT_CALL(*ssl_info, sessionId()).WillRepeatedly(testing::ReturnRef(session_id)); stream_info.setUpstreamSslConnection(ssl_info); EXPECT_EQ(session_id, stream_info.upstreamSslConnection()->sessionId()); + + EXPECT_FALSE(stream_info.upstreamConnectionId().has_value()); + stream_info.setUpstreamConnectionId(12345); + ASSERT_TRUE(stream_info.upstreamConnectionId().has_value()); + EXPECT_EQ(12345, stream_info.upstreamConnectionId().value()); } } diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index a4ab278e8db3e..49144cf7c2b24 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -219,6 +219,10 @@ class TestStreamInfo : public StreamInfo::StreamInfo { const std::string& filterChainName() const override { return filter_chain_name_; } + void setUpstreamConnectionId(uint64_t id) override { upstream_connection_id_ = id; } + + absl::optional upstreamConnectionId() const override { return upstream_connection_id_; } + Random::RandomGeneratorImpl random_; SystemTime start_time_; MonotonicTime start_time_monotonic_; @@ -258,9 +262,9 @@ class TestStreamInfo : public StreamInfo::StreamInfo { Envoy::Event::SimulatedTimeSystem test_time_; absl::optional upstream_cluster_info_{}; Http::RequestIdStreamInfoProviderSharedPtr request_id_provider_; - absl::optional connection_id_; std::string filter_chain_name_; Tracing::Reason trace_reason_{Tracing::Reason::NotTraceable}; + absl::optional upstream_connection_id_; }; } // namespace Envoy diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc index 28194ab7ab700..544a6f5bf27f6 100644 --- a/test/mocks/stream_info/mocks.cc +++ b/test/mocks/stream_info/mocks.cc @@ -126,6 +126,12 @@ MockStreamInfo::MockStreamInfo() filter_chain_name_ = std::string(filter_chain_name); })); ON_CALL(*this, filterChainName()).WillByDefault(ReturnRef(filter_chain_name_)); + ON_CALL(*this, setUpstreamConnectionId(_)) + .WillByDefault(Invoke( + [this](uint64_t id) { upstream_connection_id_ = id; })); + ON_CALL(*this, upstreamConnectionId()).WillByDefault(Invoke([this]() { + return upstream_connection_id_; + })); } MockStreamInfo::~MockStreamInfo() = default; diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index dbc2fec6c5bd4..fb38755c3f376 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -96,6 +96,8 @@ class MockStreamInfo : public StreamInfo { MOCK_METHOD(void, setConnectionID, (uint64_t)); MOCK_METHOD(void, setFilterChainName, (const absl::string_view)); MOCK_METHOD(const std::string&, filterChainName, (), (const)); + MOCK_METHOD(void, setUpstreamConnectionId, (uint64_t)); + MOCK_METHOD(absl::optional, upstreamConnectionId, (), (const)); std::shared_ptr> host_{ new testing::NiceMock()}; @@ -128,6 +130,7 @@ class MockStreamInfo : public StreamInfo { std::string route_name_; std::string upstream_transport_failure_reason_; std::string filter_chain_name_; + absl::optional upstream_connection_id_; }; } // namespace StreamInfo From cc4838aa282c7ab51ef8ecc0b93d14e57fa0e9c1 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 30 Jul 2021 15:19:37 -0700 Subject: [PATCH 07/16] fmt Signed-off-by: Jose Nino --- test/mocks/stream_info/mocks.cc | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/mocks/stream_info/mocks.cc b/test/mocks/stream_info/mocks.cc index 544a6f5bf27f6..00cda8f1d9a14 100644 --- a/test/mocks/stream_info/mocks.cc +++ b/test/mocks/stream_info/mocks.cc @@ -126,9 +126,9 @@ MockStreamInfo::MockStreamInfo() filter_chain_name_ = std::string(filter_chain_name); })); ON_CALL(*this, filterChainName()).WillByDefault(ReturnRef(filter_chain_name_)); - ON_CALL(*this, setUpstreamConnectionId(_)) - .WillByDefault(Invoke( - [this](uint64_t id) { upstream_connection_id_ = id; })); + ON_CALL(*this, setUpstreamConnectionId(_)).WillByDefault(Invoke([this](uint64_t id) { + upstream_connection_id_ = id; + })); ON_CALL(*this, upstreamConnectionId()).WillByDefault(Invoke([this]() { return upstream_connection_id_; })); From e1bde30e3ff744832335e51d8c2ec81eef98b79a Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Mon, 2 Aug 2021 13:31:46 -0700 Subject: [PATCH 08/16] update Signed-off-by: Jose Nino --- envoy/stream_info/stream_info.h | 2 +- source/common/router/router.cc | 8 ++++++++ source/common/router/upstream_request.h | 1 + source/common/stream_info/stream_info_impl.h | 7 ++++--- test/common/router/router_test.cc | 5 +++++ 5 files changed, 19 insertions(+), 4 deletions(-) diff --git a/envoy/stream_info/stream_info.h b/envoy/stream_info/stream_info.h index a56252232e8d6..fbb75d9cda6ff 100644 --- a/envoy/stream_info/stream_info.h +++ b/envoy/stream_info/stream_info.h @@ -600,7 +600,7 @@ class StreamInfo { virtual void setUpstreamConnectionId(uint64_t id) PURE; /** - * @return the ID of the upstream connection, nullopt if not available. + * @return the ID of the upstream connection, or absl::nullopt if not available. */ virtual absl::optional upstreamConnectionId() const PURE; }; diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 0a5bb031136bb..cab0095f43326 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -1354,6 +1354,14 @@ void Filter::onUpstreamHeaders(uint64_t response_code, Http::ResponseHeaderMapPt downstream_response_started_ = true; final_upstream_request_ = &upstream_request; + // In upstream request hedging scenarios the upstream connection ID set in onPoolReady might not + // be the connection ID of the upstream connection that ended up receiving upstream headers. Thus + // reset the upstream connection ID here with the ID of the connection that ultimately was the + // transport for the final upstream request. + if (final_upstream_request_->streamInfo().upstreamConnectionId().has_value()) { + callbacks_->streamInfo().setUpstreamConnectionId( + final_upstream_request_->streamInfo().upstreamConnectionId().value()); + } resetOtherUpstreams(upstream_request); if (end_stream) { onUpstreamComplete(upstream_request); diff --git a/source/common/router/upstream_request.h b/source/common/router/upstream_request.h index 8776089fe0434..928cf733e6468 100644 --- a/source/common/router/upstream_request.h +++ b/source/common/router/upstream_request.h @@ -119,6 +119,7 @@ class UpstreamRequest : public Logger::Loggable, } bool encodeComplete() const { return encode_complete_; } RouterFilterInterface& parent() { return parent_; } + const StreamInfo::StreamInfo& streamInfo() const { return stream_info_; } private: bool shouldSendEndStream() { diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index 623744a08e9dc..d9ca7d106712c 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -264,9 +264,10 @@ struct StreamInfoImpl : public StreamInfo { void dumpState(std::ostream& os, int indent_level = 0) const { const char* spaces = spacesForLevel(indent_level); - os << spaces << "StreamInfoImpl " << this << DUMP_OPTIONAL_MEMBER(protocol_) - << DUMP_OPTIONAL_MEMBER(response_code_) << DUMP_OPTIONAL_MEMBER(response_code_details_) - << DUMP_MEMBER(health_check_request_) << DUMP_MEMBER(route_name_) << "\n"; + os << spaces << "StreamInfoImpl " << this << DUMP_OPTIONAL_MEMBER(upstream_connection_id_) + << DUMP_OPTIONAL_MEMBER(protocol_) << DUMP_OPTIONAL_MEMBER(response_code_) + << DUMP_OPTIONAL_MEMBER(response_code_details_) << DUMP_MEMBER(health_check_request_) + << DUMP_MEMBER(route_name_) << "\n"; } void setUpstreamClusterInfo( diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 6334beddae881..0bab65dac9684 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -2246,6 +2246,7 @@ TEST_F(RouterTest, HedgedPerTryTimeoutThirdRequestSucceeds) { Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { response_decoder1 = &decoder; EXPECT_CALL(*router_.retry_state_, onHostAttempted(_)); + upstream_stream_info_.upstream_connection_id_ = 111; callbacks.onPoolReady(encoder1, cm_.thread_local_cluster_.conn_pool_.host_, upstream_stream_info_, Http::Protocol::Http10); return nullptr; @@ -2284,6 +2285,7 @@ TEST_F(RouterTest, HedgedPerTryTimeoutThirdRequestSucceeds) { Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { response_decoder2 = &decoder; EXPECT_CALL(*router_.retry_state_, onHostAttempted(_)); + upstream_stream_info_.upstream_connection_id_ = 222; callbacks.onPoolReady(encoder2, cm_.thread_local_cluster_.conn_pool_.host_, upstream_stream_info_, Http::Protocol::Http10); return nullptr; @@ -2308,6 +2310,7 @@ TEST_F(RouterTest, HedgedPerTryTimeoutThirdRequestSucceeds) { Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { response_decoder3 = &decoder; EXPECT_CALL(*router_.retry_state_, onHostAttempted(_)); + upstream_stream_info_.upstream_connection_id_ = 333; callbacks.onPoolReady(encoder3, cm_.thread_local_cluster_.conn_pool_.host_, upstream_stream_info_, Http::Protocol::Http10); return nullptr; @@ -2341,6 +2344,8 @@ TEST_F(RouterTest, HedgedPerTryTimeoutThirdRequestSucceeds) { response_decoder3->decodeHeaders(std::move(response_headers2), true); EXPECT_TRUE(verifyHostUpstreamStats(1, 1)); + EXPECT_EQ(333U, callbacks_.stream_info_.upstream_connection_id_); + // TODO: Verify hedge stats here once they are implemented. } From 4610af2429fd2eac385789f5451b6986a03cdeb7 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 3 Aug 2021 10:38:25 -0700 Subject: [PATCH 09/16] comments Signed-off-by: Jose Nino --- source/common/network/connection_impl.cc | 3 +++ source/common/router/upstream_request.cc | 4 ++++ 2 files changed, 7 insertions(+) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index c69004380ea40..4812a5d3ed601 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -825,6 +825,9 @@ ClientConnectionImpl::ClientConnectionImpl( std::move(transport_socket), stream_info_, false), stream_info_(dispatcher.timeSource(), socket_->addressProviderSharedPtr()) { + // "Upstream" doesn't make explicit sense here as the connection ID we are setting on stream_info_ + // is the ID of the connection itself. However, because this is a ClientConnection, which connects + // to an upstream "Server" upstream seems appropriate enough. stream_info_.setUpstreamConnectionId(id()); // There are no meaningful socket options or source address semantics for // non-IP sockets, so skip. diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 434612438653a..4816f5532b9f4 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -420,6 +420,10 @@ void UpstreamRequest::onPoolReady( stream_info_.setUpstreamSslConnection(info.downstreamSslConnection()); parent_.callbacks()->streamInfo().setUpstreamSslConnection(info.downstreamSslConnection()); + // info is a StreamInfo for the GenericUpstream. However, the stream info interface at this + // intersection of upstream/downstream is ambiguous. The ID being assigned here is the internal + // ID of the connection underlying the GenericUpstream selected to back this UpstreamRequest. + // Hence the use of the upstreamConnectionId() API. if (info.upstreamConnectionId().has_value()) { stream_info_.setUpstreamConnectionId(info.upstreamConnectionId().value()); parent_.callbacks()->streamInfo().setUpstreamConnectionId(info.upstreamConnectionId().value()); From bbd84a42e1e363cfd1ea8ee7a882a9978b37cc05 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 3 Aug 2021 11:38:44 -0700 Subject: [PATCH 10/16] remove crud Signed-off-by: Jose Nino --- test/mocks/stream_info/mocks.h | 1 - 1 file changed, 1 deletion(-) diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index 512ea86246040..827c967c7bc8a 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -92,7 +92,6 @@ class MockStreamInfo : public StreamInfo { (const Http::RequestIdStreamInfoProviderSharedPtr& provider)); MOCK_METHOD(void, setTraceReason, (Tracing::Reason reason)); MOCK_METHOD(Tracing::Reason, traceReason, (), (const)); - MOCK_METHOD(absl::optional, connectionID, (), (const)); MOCK_METHOD(void, setConnectionID, (uint64_t)); MOCK_METHOD(void, setFilterChainName, (const absl::string_view)); MOCK_METHOD(const std::string&, filterChainName, (), (const)); From f7d1d96a636978009848757129ce5dbfdf346703 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 3 Aug 2021 13:42:50 -0600 Subject: [PATCH 11/16] comments Signed-off-by: Jose Nino --- source/common/network/connection_impl.cc | 3 --- source/common/router/upstream_request.cc | 4 ---- source/common/router/upstream_request.h | 1 + 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 4812a5d3ed601..c69004380ea40 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -825,9 +825,6 @@ ClientConnectionImpl::ClientConnectionImpl( std::move(transport_socket), stream_info_, false), stream_info_(dispatcher.timeSource(), socket_->addressProviderSharedPtr()) { - // "Upstream" doesn't make explicit sense here as the connection ID we are setting on stream_info_ - // is the ID of the connection itself. However, because this is a ClientConnection, which connects - // to an upstream "Server" upstream seems appropriate enough. stream_info_.setUpstreamConnectionId(id()); // There are no meaningful socket options or source address semantics for // non-IP sockets, so skip. diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 4816f5532b9f4..434612438653a 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -420,10 +420,6 @@ void UpstreamRequest::onPoolReady( stream_info_.setUpstreamSslConnection(info.downstreamSslConnection()); parent_.callbacks()->streamInfo().setUpstreamSslConnection(info.downstreamSslConnection()); - // info is a StreamInfo for the GenericUpstream. However, the stream info interface at this - // intersection of upstream/downstream is ambiguous. The ID being assigned here is the internal - // ID of the connection underlying the GenericUpstream selected to back this UpstreamRequest. - // Hence the use of the upstreamConnectionId() API. if (info.upstreamConnectionId().has_value()) { stream_info_.setUpstreamConnectionId(info.upstreamConnectionId().value()); parent_.callbacks()->streamInfo().setUpstreamConnectionId(info.upstreamConnectionId().value()); diff --git a/source/common/router/upstream_request.h b/source/common/router/upstream_request.h index 928cf733e6468..f0b07e8bdacd5 100644 --- a/source/common/router/upstream_request.h +++ b/source/common/router/upstream_request.h @@ -119,6 +119,7 @@ class UpstreamRequest : public Logger::Loggable, } bool encodeComplete() const { return encode_complete_; } RouterFilterInterface& parent() { return parent_; } + // Exposes streamInfo for the upstream stream. const StreamInfo::StreamInfo& streamInfo() const { return stream_info_; } private: From 72fb7af18f2a1e61089a0c34401d43b22ce50fdf Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Tue, 3 Aug 2021 14:57:16 -0600 Subject: [PATCH 12/16] clean up in subsequent PR Signed-off-by: Jose Nino --- test/mocks/stream_info/mocks.h | 1 + 1 file changed, 1 insertion(+) diff --git a/test/mocks/stream_info/mocks.h b/test/mocks/stream_info/mocks.h index 827c967c7bc8a..512ea86246040 100644 --- a/test/mocks/stream_info/mocks.h +++ b/test/mocks/stream_info/mocks.h @@ -92,6 +92,7 @@ class MockStreamInfo : public StreamInfo { (const Http::RequestIdStreamInfoProviderSharedPtr& provider)); MOCK_METHOD(void, setTraceReason, (Tracing::Reason reason)); MOCK_METHOD(Tracing::Reason, traceReason, (), (const)); + MOCK_METHOD(absl::optional, connectionID, (), (const)); MOCK_METHOD(void, setConnectionID, (uint64_t)); MOCK_METHOD(void, setFilterChainName, (const absl::string_view)); MOCK_METHOD(const std::string&, filterChainName, (), (const)); From 84f2b4160c5ec44ccaba1a0ece7b2c8bbcb39d2b Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 4 Aug 2021 10:40:41 -0600 Subject: [PATCH 13/16] update Signed-off-by: Jose Nino --- source/common/network/connection_impl.cc | 1 - source/common/router/upstream_request.cc | 6 +++--- test/common/router/router_test.cc | 6 +++--- 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index c69004380ea40..67426b30b99ec 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -825,7 +825,6 @@ ClientConnectionImpl::ClientConnectionImpl( std::move(transport_socket), stream_info_, false), stream_info_(dispatcher.timeSource(), socket_->addressProviderSharedPtr()) { - stream_info_.setUpstreamConnectionId(id()); // There are no meaningful socket options or source address semantics for // non-IP sockets, so skip. if (remote_address->ip() == nullptr) { diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 434612438653a..1cd50fd198e84 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -420,9 +420,9 @@ void UpstreamRequest::onPoolReady( stream_info_.setUpstreamSslConnection(info.downstreamSslConnection()); parent_.callbacks()->streamInfo().setUpstreamSslConnection(info.downstreamSslConnection()); - if (info.upstreamConnectionId().has_value()) { - stream_info_.setUpstreamConnectionId(info.upstreamConnectionId().value()); - parent_.callbacks()->streamInfo().setUpstreamConnectionId(info.upstreamConnectionId().value()); + if (info.downstreamAddressProvider().connectionID().has_value()) { + stream_info_.setUpstreamConnectionId(info.downstreamAddressProvider().connectionID().value()); + parent_.callbacks()->streamInfo().setUpstreamConnectionId(info.downstreamAddressProvider().connectionID().value()); } if (parent_.downstreamEndStream()) { diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 0bab65dac9684..19c2de79aa303 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -2246,7 +2246,7 @@ TEST_F(RouterTest, HedgedPerTryTimeoutThirdRequestSucceeds) { Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { response_decoder1 = &decoder; EXPECT_CALL(*router_.retry_state_, onHostAttempted(_)); - upstream_stream_info_.upstream_connection_id_ = 111; + upstream_stream_info_.downstream_address_provider_->setConnectionID(111); callbacks.onPoolReady(encoder1, cm_.thread_local_cluster_.conn_pool_.host_, upstream_stream_info_, Http::Protocol::Http10); return nullptr; @@ -2285,7 +2285,7 @@ TEST_F(RouterTest, HedgedPerTryTimeoutThirdRequestSucceeds) { Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { response_decoder2 = &decoder; EXPECT_CALL(*router_.retry_state_, onHostAttempted(_)); - upstream_stream_info_.upstream_connection_id_ = 222; + upstream_stream_info_.downstream_address_provider_->setConnectionID(222); callbacks.onPoolReady(encoder2, cm_.thread_local_cluster_.conn_pool_.host_, upstream_stream_info_, Http::Protocol::Http10); return nullptr; @@ -2310,7 +2310,7 @@ TEST_F(RouterTest, HedgedPerTryTimeoutThirdRequestSucceeds) { Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { response_decoder3 = &decoder; EXPECT_CALL(*router_.retry_state_, onHostAttempted(_)); - upstream_stream_info_.upstream_connection_id_ = 333; + upstream_stream_info_.downstream_address_provider_->setConnectionID(333); callbacks.onPoolReady(encoder3, cm_.thread_local_cluster_.conn_pool_.host_, upstream_stream_info_, Http::Protocol::Http10); return nullptr; From 22457247a973ce76318b702045672019e774ac0e Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 4 Aug 2021 10:45:21 -0600 Subject: [PATCH 14/16] fmt Signed-off-by: Jose Nino --- source/common/router/upstream_request.cc | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 1cd50fd198e84..f558a86aa3caf 100644 --- a/source/common/router/upstream_request.cc +++ b/source/common/router/upstream_request.cc @@ -422,7 +422,8 @@ void UpstreamRequest::onPoolReady( if (info.downstreamAddressProvider().connectionID().has_value()) { stream_info_.setUpstreamConnectionId(info.downstreamAddressProvider().connectionID().value()); - parent_.callbacks()->streamInfo().setUpstreamConnectionId(info.downstreamAddressProvider().connectionID().value()); + parent_.callbacks()->streamInfo().setUpstreamConnectionId( + info.downstreamAddressProvider().connectionID().value()); } if (parent_.downstreamEndStream()) { From a66b916132ef501692469979c261e825011496da Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 4 Aug 2021 15:13:48 -0600 Subject: [PATCH 15/16] fmt Signed-off-by: Jose Nino --- source/common/stream_info/stream_info_impl.h | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/source/common/stream_info/stream_info_impl.h b/source/common/stream_info/stream_info_impl.h index c1d66e4231e16..2024754272fd6 100644 --- a/source/common/stream_info/stream_info_impl.h +++ b/source/common/stream_info/stream_info_impl.h @@ -257,8 +257,8 @@ struct StreamInfoImpl : public StreamInfo { const char* spaces = spacesForLevel(indent_level); os << spaces << "StreamInfoImpl " << this << DUMP_OPTIONAL_MEMBER(upstream_connection_id_) << DUMP_OPTIONAL_MEMBER(protocol_) << DUMP_OPTIONAL_MEMBER(response_code_) - << DUMP_OPTIONAL_MEMBER(response_code_details_) << DUMP_OPTIONAL_MEMBER(attempt_count_) << DUMP_MEMBER(health_check_request_) - << DUMP_MEMBER(route_name_) << "\n"; + << DUMP_OPTIONAL_MEMBER(response_code_details_) << DUMP_OPTIONAL_MEMBER(attempt_count_) + << DUMP_MEMBER(health_check_request_) << DUMP_MEMBER(route_name_) << "\n"; } void setUpstreamClusterInfo( @@ -301,11 +301,8 @@ struct StreamInfoImpl : public StreamInfo { FilterStateSharedPtr filter_state_; FilterStateSharedPtr upstream_filter_state_; std::string route_name_; -<<<<<<< HEAD absl::optional upstream_connection_id_; -======= absl::optional attempt_count_; ->>>>>>> main private: static Network::SocketAddressProviderSharedPtr emptyDownstreamAddressProvider() { From 4d574bcad71f1170844a0cbad53716aaa0ac6aba Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Wed, 4 Aug 2021 15:25:46 -0600 Subject: [PATCH 16/16] merge conflicts Signed-off-by: Jose Nino --- source/common/network/connection_impl.cc | 1 - test/common/stream_info/test_util.h | 4 +--- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 1ea4957e695d1..6b6787ae9492a 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -829,7 +829,6 @@ ClientConnectionImpl::ClientConnectionImpl( : ConnectionImpl(dispatcher, std::make_unique(remote_address, options), std::move(transport_socket), stream_info_, false), stream_info_(dispatcher.timeSource(), socket_->addressProviderSharedPtr()) { - // There are no meaningful socket options or source address semantics for // non-IP sockets, so skip. if (remote_address->ip() == nullptr) { diff --git a/test/common/stream_info/test_util.h b/test/common/stream_info/test_util.h index 70879c4607e30..1c8632ba6f768 100644 --- a/test/common/stream_info/test_util.h +++ b/test/common/stream_info/test_util.h @@ -211,15 +211,13 @@ class TestStreamInfo : public StreamInfo::StreamInfo { const std::string& filterChainName() const override { return filter_chain_name_; } -<<<<<<< HEAD void setUpstreamConnectionId(uint64_t id) override { upstream_connection_id_ = id; } absl::optional upstreamConnectionId() const override { return upstream_connection_id_; } -======= + void setAttemptCount(uint32_t attempt_count) override { attempt_count_ = attempt_count; } absl::optional attemptCount() const override { return attempt_count_; } ->>>>>>> main Random::RandomGeneratorImpl random_; SystemTime start_time_;