Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

socket: move connection id interface to SocketAddressProvider #17231

Merged
merged 6 commits into from
Jul 8, 2021
Merged

socket: move connection id interface to SocketAddressProvider #17231

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8666914
Add connection id interface to socket address provider
soulxu Jul 4, 2021
957631d
Replace streaminfo's setConnectionID with SocketAddressProvider one
soulxu Jul 4, 2021
0baa14c
Remove stream info connectionID() instead with SocketAddressProvider one
soulxu Jul 4, 2021
e1a583b
fix format
soulxu Jul 4, 2021
ac01cc5
remove connection id member from stream info
soulxu Jul 4, 2021
f6d7982
Add todo
soulxu Jul 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions envoy/network/socket.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,11 @@ class SocketAddressProvider {
*/
virtual absl::string_view requestedServerName() const PURE;

/**
* @return Connection ID of the downstream connection, or unset if not available.
**/
virtual absl::optional<uint64_t> connectionID() const PURE;

/**
* Dumps the state of the SocketAddressProvider to the given ostream.
*
Expand Down Expand Up @@ -121,6 +126,11 @@ class SocketAddressSetter : public SocketAddressProvider {
* @param SNI value requested.
*/
virtual void setRequestedServerName(const absl::string_view requested_server_name) PURE;

/**
* @param id Connection ID of the downstream connection.
**/
virtual void setConnectionID(uint64_t id) PURE;
};

using SocketAddressSetterSharedPtr = std::shared_ptr<SocketAddressSetter>;
Expand Down
10 changes: 0 additions & 10 deletions envoy/stream_info/stream_info.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,16 +584,6 @@ class StreamInfo {
*/
virtual Tracing::Reason traceReason() const PURE;

/**
* @return Connection ID of the downstream connection, or unset if not available.
**/
virtual absl::optional<uint64_t> connectionID() const PURE;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not keep connectionID() around, but make it a facade for downstreamAddressProvider().connectionID() call? This way StreamInfo gets uncoupled from an AddressProvider (see here for an example of such a coupling: https://github.com/envoyproxy/envoy/pull/17231/files#diff-888dc1d9b82ce89abbe565c8c52a11dd16120d2d23b794e1a8c46e5ac0bb039aR817)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emm...I'm hesitant when writing the PR. I don't a strong reason to keep the StreamInfo::connectionID or not. In the end, I follow the localAddress()/remoteAddress(), they don't have a facade in StreamInfo.

envoy/source/common/formatter/substitution_formatter.cc

Line 782 in a0ca08b

return stream_info.downstreamAddressProvider().localAddress();

If we keep connectionID, I thought it should be able uncouple the calling code and AddressProvider. But AddressProvider interface is quite single, so I think it should be ok. But let me know how you see the benefit of uncouple streaminfo and addressProvider, I will update the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @soulxu. I'm not sure there is any reason to have a facade here?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this PR alone the chain of calls stream_info.downstreamAddressProvider()... is used in five places, not counting tests. There are probably going to be more with time? It's not huge for sure, but makes changing and testing the code harder.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm ok with both ways, if we have a facade for connection id, what about those localaddress/remoteaddress? @mattklein123 what do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it doesn't really matter, but all things being equal I don't think it's better to have more methods vs. some slightly longer call chains. I think it's fine as is.


/**
* @param id Connection ID of the downstream connection.
**/
virtual void setConnectionID(uint64_t id) PURE;

/**
* @param filter_chain_name Network filter chain name of the downstream connection.
*/
Expand Down
2 changes: 1 addition & 1 deletion source/common/formatter/substitution_formatter.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -814,7 +814,7 @@ StreamInfoFormatter::StreamInfoFormatter(const std::string& field_name) {
} else if (field_name == "CONNECTION_ID") {
field_extractor_ = std::make_unique<StreamInfoUInt64FieldExtractor>(
[](const StreamInfo::StreamInfo& stream_info) {
return stream_info.connectionID().value_or(0);
return stream_info.downstreamAddressProvider().connectionID().value_or(0);
});
} else if (field_name == "REQUESTED_SERVER_NAME") {
field_extractor_ = std::make_unique<StreamInfoStringFieldExtractor>(
Expand Down
3 changes: 0 additions & 3 deletions source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -651,9 +651,6 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
filter_manager_.streamInfo().setDownstreamSslConnection(
connection_manager_.read_callbacks_->connection().ssl());

filter_manager_.streamInfo().setConnectionID(
connection_manager_.read_callbacks_->connection().id());

if (connection_manager_.config_.streamIdleTimeout().count()) {
idle_timeout_ms_ = connection_manager_.config_.streamIdleTimeout();
stream_idle_timer_ =
Expand Down
3 changes: 3 additions & 0 deletions source/common/http/filter_manager.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -621,6 +621,9 @@ class OverridableRemoteSocketAddressSetterStreamInfo : public StreamInfo::Stream
absl::string_view requestedServerName() const override {
return StreamInfoImpl::downstreamAddressProvider().requestedServerName();
}
absl::optional<uint64_t> connectionID() const override {
return StreamInfoImpl::downstreamAddressProvider().connectionID();
}
void dumpState(std::ostream& os, int indent_level) const override {
StreamInfoImpl::dumpState(os, indent_level);

Expand Down
2 changes: 2 additions & 0 deletions source/common/network/connection_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,8 @@ ConnectionImpl::ConnectionImpl(Event::Dispatcher& dispatcher, ConnectionSocketPt
Event::FileReadyType::Read | Event::FileReadyType::Write);

transport_socket_->setTransportSocketCallbacks(*this);

socket_->addressProvider().setConnectionID(id());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a much larger change, but is there any reason to not just have the ID be generated as part of the "address provider" and then we don't need a setter or any of the optional stuff? Perhaps a TODO if you agree with that?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need to move the 'next_global_id_' (at line 69) into the SocketAddressProvider. And SocketImpl will create an instance of SocketAddressProvider, which means every socket will have a connection id. The only thing I found is ListenerSocket is based on SocketImpl, so every ListenerSocket will have a connection id, which doesn't match the concept 'connection' a little bit. If we feel this is ok, then this should be doable. WDYT, if you think it is ok, let me add a TODO, then I will work on it on a separate PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah IMO this would be better overall. I would just add a small TODO and we can potentially do this later. It's not a huge deal. Thank you!

/wait

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got it, updated, thanks for the review!

}

ConnectionImpl::~ConnectionImpl() {
Expand Down
3 changes: 3 additions & 0 deletions source/common/network/socket_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,13 +49,16 @@ class SocketAddressSetterImpl : public SocketAddressSetter {
void setRequestedServerName(const absl::string_view requested_server_name) override {
server_name_ = std::string(requested_server_name);
}
absl::optional<uint64_t> connectionID() const override { return connection_id_; }
void setConnectionID(uint64_t id) override { connection_id_ = id; }

private:
Address::InstanceConstSharedPtr local_address_;
bool local_address_restored_{false};
Address::InstanceConstSharedPtr remote_address_;
Address::InstanceConstSharedPtr direct_remote_address_;
std::string server_name_;
absl::optional<uint64_t> connection_id_;
};

class SocketImpl : public virtual Socket {
Expand Down
5 changes: 0 additions & 5 deletions source/common/stream_info/stream_info_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,10 +274,6 @@ struct StreamInfoImpl : public StreamInfo {
return upstream_cluster_info_;
}

void setConnectionID(uint64_t id) override { connection_id_ = id; }

absl::optional<uint64_t> connectionID() const override { return connection_id_; }

void setFilterChainName(absl::string_view filter_chain_name) override {
filter_chain_name_ = std::string(filter_chain_name);
}
Expand Down Expand Up @@ -336,7 +332,6 @@ struct StreamInfoImpl : public StreamInfo {
UpstreamTiming upstream_timing_;
std::string upstream_transport_failure_reason_;
absl::optional<Upstream::ClusterInfoConstSharedPtr> upstream_cluster_info_;
absl::optional<uint64_t> connection_id_;
std::string filter_chain_name_;
Tracing::Reason trace_reason_;
};
Expand Down
2 changes: 1 addition & 1 deletion source/extensions/filters/common/expr/context.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ absl::optional<CelValue> ConnectionWrapper::operator[](CelValue key) const {
} else if (value == RequestedServerName) {
return CelValue::CreateStringView(info_.downstreamAddressProvider().requestedServerName());
} else if (value == ID) {
auto id = info_.connectionID();
auto id = info_.downstreamAddressProvider().connectionID();
if (id.has_value()) {
return CelValue::CreateUint64(id.value());
}
Expand Down
1 change: 0 additions & 1 deletion source/server/active_tcp_listener.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,6 @@ ActiveTcpConnection::ActiveTcpConnection(ActiveConnections& active_connections,
listener.stats_.downstream_cx_active_.inc();
listener.per_worker_stats_.downstream_cx_total_.inc();
listener.per_worker_stats_.downstream_cx_active_.inc();
stream_info_->setConnectionID(connection_->id());

// Active connections on the handler (not listener). The per listener connections have already
// been incremented at this point either via the connection balancer or in the socket accept
Expand Down
2 changes: 1 addition & 1 deletion test/common/formatter/substitution_formatter_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,7 +663,7 @@ TEST(SubstitutionFormatterTest, streamInfoFormatter) {
{
StreamInfoFormatter upstream_format("CONNECTION_ID");
uint64_t id = 123;
EXPECT_CALL(stream_info, connectionID()).WillRepeatedly(Return(id));
stream_info.downstream_address_provider_->setConnectionID(id);
EXPECT_EQ("123", upstream_format.format(request_headers, response_headers, response_trailers,
stream_info, body));
EXPECT_THAT(upstream_format.formatValue(request_headers, response_headers, response_trailers,
Expand Down
4 changes: 2 additions & 2 deletions test/common/http/conn_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,7 +293,6 @@ TEST_F(HttpConnectionManagerImplTest, 100ContinueResponseWithDecoderPause) {
// When create new stream, the stream info will be populated from the connection.
TEST_F(HttpConnectionManagerImplTest, PopulateStreamInfo) {
setup(true, "", false);
EXPECT_CALL(filter_callbacks_.connection_, id()).WillRepeatedly(Return(1234));

// Set up the codec.
Buffer::OwnedImpl fake_input("input");
Expand All @@ -303,7 +302,8 @@ TEST_F(HttpConnectionManagerImplTest, PopulateStreamInfo) {

EXPECT_EQ(requestIDExtension().get(), decoder_->streamInfo().getRequestIDProvider());
EXPECT_EQ(ssl_connection_, decoder_->streamInfo().downstreamSslConnection());
EXPECT_EQ(1234U, decoder_->streamInfo().connectionID());
EXPECT_EQ(filter_callbacks_.connection_.id_,
decoder_->streamInfo().downstreamAddressProvider().connectionID());
EXPECT_EQ(server_name_, decoder_->streamInfo().downstreamAddressProvider().requestedServerName());

// Clean up.
Expand Down
8 changes: 0 additions & 8 deletions test/common/stream_info/stream_info_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -252,14 +252,6 @@ TEST_F(StreamInfoImplTest, DefaultRequestIDExtensionTest) {
EXPECT_EQ(nullptr, stream_info.getRequestIDProvider());
}

TEST_F(StreamInfoImplTest, ConnectionID) {
StreamInfoImpl stream_info(test_time_.timeSystem(), nullptr);
EXPECT_FALSE(stream_info.connectionID().has_value());
uint64_t id = 123;
stream_info.setConnectionID(id);
EXPECT_EQ(id, stream_info.connectionID());
}

TEST_F(StreamInfoImplTest, Details) {
StreamInfoImpl stream_info(test_time_.timeSystem(), nullptr);
EXPECT_FALSE(stream_info.responseCodeDetails().has_value());
Expand Down
4 changes: 0 additions & 4 deletions test/common/stream_info/test_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,10 +213,6 @@ class TestStreamInfo : public StreamInfo::StreamInfo {
return upstream_cluster_info_;
}

void setConnectionID(uint64_t id) override { connection_id_ = id; }

absl::optional<uint64_t> connectionID() const override { return connection_id_; }

void setFilterChainName(absl::string_view filter_chain_name) override {
filter_chain_name_ = std::string(filter_chain_name);
}
Expand Down
1 change: 1 addition & 0 deletions test/extensions/filters/common/expr/context_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,7 @@ TEST(Context, ConnectionAttributes) {
EXPECT_CALL(info, upstreamTransportFailureReason())
.WillRepeatedly(ReturnRef(upstream_transport_failure_reason));
EXPECT_CALL(info, connectionID()).WillRepeatedly(Return(123));
info.downstream_address_provider_->setConnectionID(123);
const absl::optional<std::string> connection_termination_details = "unauthorized";
EXPECT_CALL(info, connectionTerminationDetails())
.WillRepeatedly(ReturnRef(connection_termination_details));
Expand Down
1 change: 1 addition & 0 deletions test/mocks/network/connection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ template <class T> static void initializeMockConnection(T& connection) {
connection.raiseEvent(Network::ConnectionEvent::LocalClose);
}));
ON_CALL(connection, id()).WillByDefault(Return(connection.next_id_));
connection.stream_info_.downstream_address_provider_->setConnectionID(connection.id_);
ON_CALL(connection, state()).WillByDefault(ReturnPointee(&connection.state_));

// The real implementation will move the buffer data into the socket.
Expand Down
1 change: 0 additions & 1 deletion test/mocks/stream_info/mocks.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,6 @@ MockStreamInfo::MockStreamInfo()
ON_CALL(*this, getRouteName()).WillByDefault(ReturnRef(route_name_));
ON_CALL(*this, upstreamTransportFailureReason())
.WillByDefault(ReturnRef(upstream_transport_failure_reason_));
ON_CALL(*this, connectionID()).WillByDefault(Return(connection_id_));
ON_CALL(*this, setConnectionID(_)).WillByDefault(Invoke([this](uint64_t id) {
connection_id_ = id;
}));
Expand Down