From 9d8f3cf963be13380604778c37b8ddd1b49f7258 Mon Sep 17 00:00:00 2001 From: Ali Beyad Date: Tue, 27 Aug 2024 20:42:27 +0000 Subject: [PATCH] address comments Signed-off-by: Ali Beyad --- contrib/vcl/source/vcl_io_handle.cc | 2 +- contrib/vcl/source/vcl_io_handle.h | 2 +- envoy/network/io_handle.h | 4 ++-- .../network/io_socket_handle_base_impl.cc | 2 +- .../network/io_socket_handle_base_impl.h | 4 ++-- source/common/network/io_socket_handle_impl.cc | 2 +- source/common/network/utility.cc | 4 ++-- source/common/quic/quic_io_handle_wrapper.h | 2 +- .../io_socket/user_space/io_handle_impl.cc | 2 +- .../io_socket/user_space/io_handle_impl.h | 2 +- .../client_connection_factory_impl_test.cc | 18 ++++++++---------- test/common/quic/envoy_quic_utils_test.cc | 8 ++++---- .../udp/udp_proxy/udp_proxy_filter_test.cc | 4 ++-- .../http/udp/upstream_request_test.cc | 2 +- test/mocks/network/io_handle.h | 2 +- 15 files changed, 29 insertions(+), 31 deletions(-) diff --git a/contrib/vcl/source/vcl_io_handle.cc b/contrib/vcl/source/vcl_io_handle.cc index aa6b718985cd..8078d35e65a9 100644 --- a/contrib/vcl/source/vcl_io_handle.cc +++ b/contrib/vcl/source/vcl_io_handle.cc @@ -166,7 +166,7 @@ Api::IoCallUint64Result VclIoHandle::close() { bool VclIoHandle::isOpen() const { return VCL_SH_VALID(sh_); } -bool VclIoHandle::isConnected() const { return false; } +bool VclIoHandle::wasConnected() const { return false; } Api::IoCallUint64Result VclIoHandle::readv(uint64_t max_length, Buffer::RawSlice* slices, uint64_t num_slice) { diff --git a/contrib/vcl/source/vcl_io_handle.h b/contrib/vcl/source/vcl_io_handle.h index e5c67634a7f2..167f9c2370c6 100644 --- a/contrib/vcl/source/vcl_io_handle.h +++ b/contrib/vcl/source/vcl_io_handle.h @@ -31,7 +31,7 @@ class VclIoHandle : public Envoy::Network::IoHandle, os_fd_t fdDoNotUse() const override { return fd_; } Api::IoCallUint64Result close() override; bool isOpen() const override; - bool isConnected() const override; + bool wasConnected() const override; Api::IoCallUint64Result readv(uint64_t max_length, Buffer::RawSlice* slices, uint64_t num_slice) override; Api::IoCallUint64Result read(Buffer::Instance& buffer, diff --git a/envoy/network/io_handle.h b/envoy/network/io_handle.h index 711b746b2ff9..474c81f7959d 100644 --- a/envoy/network/io_handle.h +++ b/envoy/network/io_handle.h @@ -66,10 +66,10 @@ class IoHandle { virtual bool isOpen() const PURE; /** - * Return true if connect() has successfully been called on the socket. + * Return true if the socket has had connect() successfully called on it. * Use isOpen() to check if the socket is still connected or not. */ - virtual bool isConnected() const PURE; + virtual bool wasConnected() const PURE; /** * Read data into given slices. diff --git a/source/common/network/io_socket_handle_base_impl.cc b/source/common/network/io_socket_handle_base_impl.cc index a6402ac44f72..3a2e8f3e79dc 100644 --- a/source/common/network/io_socket_handle_base_impl.cc +++ b/source/common/network/io_socket_handle_base_impl.cc @@ -29,7 +29,7 @@ IoSocketHandleBaseImpl::~IoSocketHandleBaseImpl() { bool IoSocketHandleBaseImpl::isOpen() const { return SOCKET_VALID(fd_); } -bool IoSocketHandleBaseImpl::isConnected() const { return is_connected_; } +bool IoSocketHandleBaseImpl::wasConnected() const { return was_connected_; } bool IoSocketHandleBaseImpl::supportsMmsg() const { return Api::OsSysCallsSingleton::get().supportsMmsg(); diff --git a/source/common/network/io_socket_handle_base_impl.h b/source/common/network/io_socket_handle_base_impl.h index ac56352a22ac..be02fb00b1f0 100644 --- a/source/common/network/io_socket_handle_base_impl.h +++ b/source/common/network/io_socket_handle_base_impl.h @@ -19,7 +19,7 @@ class IoSocketHandleBaseImpl : public IoHandle, protected Logger::Loggable domain_; - bool is_connected_ = false; + bool was_connected_ = false; }; } // namespace Network diff --git a/source/common/network/io_socket_handle_impl.cc b/source/common/network/io_socket_handle_impl.cc index 6092cbf1bb80..deba098a38d5 100644 --- a/source/common/network/io_socket_handle_impl.cc +++ b/source/common/network/io_socket_handle_impl.cc @@ -541,7 +541,7 @@ Api::SysCallIntResult IoSocketHandleImpl::connect(Address::InstanceConstSharedPt auto result = Api::OsSysCallsSingleton::get().connect(fd_, sockaddr_to_use, sockaddr_len_to_use); if (result.return_value_ != -1) { - is_connected_ = true; + was_connected_ = true; } return result; } diff --git a/source/common/network/utility.cc b/source/common/network/utility.cc index 568b15b9f850..f177756b5f86 100644 --- a/source/common/network/utility.cc +++ b/source/common/network/utility.cc @@ -516,11 +516,11 @@ Api::IoCallUint64Result Utility::writeToSocket(IoHandle& handle, Buffer::RawSlic Api::IoCallUint64Result send_result( /*rc=*/0, /*err=*/Api::IoError::none()); - const bool is_connected = handle.isConnected(); + const bool is_connected = handle.wasConnected(); do { if (is_connected) { // The socket is already connected, so the local and peer addresses should not be specified. - // Instead, a writev/send is called. + // Instead, a writev is called. send_result = handle.writev(slices, num_slices); } else { // For non-connected sockets(), calling sendmsg with the peer address specified ensures the diff --git a/source/common/quic/quic_io_handle_wrapper.h b/source/common/quic/quic_io_handle_wrapper.h index 60cf3c7d875e..73f752d87777 100644 --- a/source/common/quic/quic_io_handle_wrapper.h +++ b/source/common/quic/quic_io_handle_wrapper.h @@ -24,7 +24,7 @@ class QuicIoHandleWrapper : public Network::IoHandle { return Api::ioCallUint64ResultNoError(); } bool isOpen() const override { return !closed_; } - bool isConnected() const override { return io_handle_.isConnected(); } + bool wasConnected() const override { return io_handle_.wasConnected(); } Api::IoCallUint64Result readv(uint64_t max_length, Buffer::RawSlice* slices, uint64_t num_slice) override { if (closed_) { diff --git a/source/extensions/io_socket/user_space/io_handle_impl.cc b/source/extensions/io_socket/user_space/io_handle_impl.cc index 571bc3a7fc89..cf86c0700dc7 100644 --- a/source/extensions/io_socket/user_space/io_handle_impl.cc +++ b/source/extensions/io_socket/user_space/io_handle_impl.cc @@ -90,7 +90,7 @@ Api::IoCallUint64Result IoHandleImpl::close() { bool IoHandleImpl::isOpen() const { return !closed_; } -bool IoHandleImpl::isConnected() const { return false; } +bool IoHandleImpl::wasConnected() const { return false; } Api::IoCallUint64Result IoHandleImpl::readv(uint64_t max_length, Buffer::RawSlice* slices, uint64_t num_slice) { diff --git a/source/extensions/io_socket/user_space/io_handle_impl.h b/source/extensions/io_socket/user_space/io_handle_impl.h index f862ee0c16fa..e2f1b8587f99 100644 --- a/source/extensions/io_socket/user_space/io_handle_impl.h +++ b/source/extensions/io_socket/user_space/io_handle_impl.h @@ -49,7 +49,7 @@ class IoHandleImpl final : public Network::IoHandle, } Api::IoCallUint64Result close() override; bool isOpen() const override; - bool isConnected() const override; + bool wasConnected() const override; Api::IoCallUint64Result readv(uint64_t max_length, Buffer::RawSlice* slices, uint64_t num_slice) override; Api::IoCallUint64Result read(Buffer::Instance& buffer, diff --git a/test/common/quic/client_connection_factory_impl_test.cc b/test/common/quic/client_connection_factory_impl_test.cc index b65d6ed879f5..455a819bcbaf 100644 --- a/test/common/quic/client_connection_factory_impl_test.cc +++ b/test/common/quic/client_connection_factory_impl_test.cc @@ -23,6 +23,8 @@ using testing::Return; namespace Envoy { namespace Quic { +constexpr int PEER_PORT = 54321; + class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, public testing::TestWithParam { protected: @@ -60,8 +62,8 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, } EXPECT_EQ(quic_ccopts, "6RTOAKD4"); - test_address_ = *Network::Utility::resolveUrl( - absl::StrCat("tcp://", Network::Test::getLoopbackAddressUrlString(GetParam()), ":30")); + test_address_ = *Network::Utility::resolveUrl(absl::StrCat( + "tcp://", Network::Test::getLoopbackAddressUrlString(GetParam()), ":", PEER_PORT)); Ssl::ClientContextSharedPtr context{new Ssl::MockClientContext()}; EXPECT_CALL(context_.context_manager_, createSslClientContext(_, _)).WillOnce(Return(context)); factory_ = *Quic::QuicClientTransportSocketFactory::create( @@ -94,10 +96,9 @@ class QuicNetworkConnectionTest : public Event::TestUsingSimulatedTime, TEST_P(QuicNetworkConnectionTest, BufferLimits) { initialize(); - const int port = 30; std::unique_ptr client_connection = createQuicNetworkConnection( *quic_info_, crypto_config_, - quic::QuicServerId{factory_->clientContextConfig()->serverNameIndication(), port, false}, + quic::QuicServerId{factory_->clientContextConfig()->serverNameIndication(), PEER_PORT, false}, dispatcher_, test_address_, test_address_, quic_stat_names_, {}, *store_.rootScope(), nullptr, nullptr, connection_id_generator_, *factory_); EnvoyQuicClientSession* session = static_cast(client_connection.get()); @@ -118,14 +119,13 @@ TEST_P(QuicNetworkConnectionTest, SocketOptions) { auto socket_option = std::make_shared(); auto socket_options = std::make_shared(); socket_options->push_back(socket_option); - const int port = 30; EXPECT_CALL(*socket_option, setOption(_, envoy::config::core::v3::SocketOption::STATE_PREBIND)); EXPECT_CALL(*socket_option, setOption(_, envoy::config::core::v3::SocketOption::STATE_BOUND)); EXPECT_CALL(*socket_option, setOption(_, envoy::config::core::v3::SocketOption::STATE_LISTENING)); std::unique_ptr client_connection = createQuicNetworkConnection( *quic_info_, crypto_config_, - quic::QuicServerId{factory_->clientContextConfig()->serverNameIndication(), port, false}, + quic::QuicServerId{factory_->clientContextConfig()->serverNameIndication(), PEER_PORT, false}, dispatcher_, test_address_, test_address_, quic_stat_names_, {}, *store_.rootScope(), socket_options, nullptr, connection_id_generator_, *factory_); EnvoyQuicClientSession* session = static_cast(client_connection.get()); @@ -136,14 +136,13 @@ TEST_P(QuicNetworkConnectionTest, SocketOptions) { TEST_P(QuicNetworkConnectionTest, LocalAddress) { initialize(); - const int port = 30; Network::Address::InstanceConstSharedPtr local_addr = (GetParam() == Network::Address::IpVersion::v6) ? Network::Utility::getIpv6LoopbackAddress() : Network::Utility::getCanonicalIpv4LoopbackAddress(); std::unique_ptr client_connection = createQuicNetworkConnection( *quic_info_, crypto_config_, - quic::QuicServerId{factory_->clientContextConfig()->serverNameIndication(), port, false}, + quic::QuicServerId{factory_->clientContextConfig()->serverNameIndication(), PEER_PORT, false}, dispatcher_, test_address_, local_addr, quic_stat_names_, {}, *store_.rootScope(), nullptr, nullptr, connection_id_generator_, *factory_); EnvoyQuicClientSession* session = static_cast(client_connection.get()); @@ -163,10 +162,9 @@ TEST_P(QuicNetworkConnectionTest, Srtt) { EXPECT_CALL(rtt_cache, getSrtt).WillOnce(Return(std::chrono::microseconds(5))); - const int port = 30; std::unique_ptr client_connection = createQuicNetworkConnection( info, crypto_config_, - quic::QuicServerId{factory_->clientContextConfig()->serverNameIndication(), port, false}, + quic::QuicServerId{factory_->clientContextConfig()->serverNameIndication(), PEER_PORT, false}, dispatcher_, test_address_, test_address_, quic_stat_names_, rtt_cache, *store_.rootScope(), nullptr, nullptr, connection_id_generator_, *factory_); diff --git a/test/common/quic/envoy_quic_utils_test.cc b/test/common/quic/envoy_quic_utils_test.cc index 627909b383e2..8035a432f826 100644 --- a/test/common/quic/envoy_quic_utils_test.cc +++ b/test/common/quic/envoy_quic_utils_test.cc @@ -289,13 +289,13 @@ TEST(EnvoyQuicUtilsTest, CreateConnectionSocket) { std::make_shared("127.0.0.1", 54321, nullptr); auto connection_socket = createConnectionSocket(peer_addr, local_addr, nullptr); EXPECT_TRUE(connection_socket->isOpen()); - EXPECT_TRUE(connection_socket->ioHandle().isConnected()); + EXPECT_TRUE(connection_socket->ioHandle().wasConnected()); connection_socket->close(); Network::Address::InstanceConstSharedPtr no_local_addr = nullptr; connection_socket = createConnectionSocket(peer_addr, no_local_addr, nullptr); EXPECT_TRUE(connection_socket->isOpen()); - EXPECT_TRUE(connection_socket->ioHandle().isConnected()); + EXPECT_TRUE(connection_socket->ioHandle().wasConnected()); EXPECT_EQ("127.0.0.1", no_local_addr->ip()->addressAsString()); connection_socket->close(); @@ -305,13 +305,13 @@ TEST(EnvoyQuicUtilsTest, CreateConnectionSocket) { std::make_shared("::1", 54321, nullptr); connection_socket = createConnectionSocket(peer_addr_v6, local_addr_v6, nullptr); EXPECT_TRUE(connection_socket->isOpen()); - EXPECT_TRUE(connection_socket->ioHandle().isConnected()); + EXPECT_TRUE(connection_socket->ioHandle().wasConnected()); connection_socket->close(); Network::Address::InstanceConstSharedPtr no_local_addr_v6 = nullptr; connection_socket = createConnectionSocket(peer_addr_v6, no_local_addr_v6, nullptr); EXPECT_TRUE(connection_socket->isOpen()); - EXPECT_TRUE(connection_socket->ioHandle().isConnected()); + EXPECT_TRUE(connection_socket->ioHandle().wasConnected()); EXPECT_EQ("::1", no_local_addr_v6->ip()->addressAsString()); connection_socket->close(); } diff --git a/test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc b/test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc index d27e76c4fda2..f887a1831c36 100644 --- a/test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc +++ b/test/extensions/filters/udp/udp_proxy/udp_proxy_filter_test.cc @@ -117,7 +117,7 @@ class UdpProxyFilterTest : public UdpProxyFilterBase { } if (!connect_sys_errno) { if (expect_connect) { - EXPECT_CALL(*socket_->io_handle_, isConnected()).WillOnce(Return(true)); + EXPECT_CALL(*socket_->io_handle_, wasConnected()).WillOnce(Return(true)); EXPECT_CALL(*socket_->io_handle_, writev(_, 1)) .WillOnce(Invoke([data, sys_errno](const Buffer::RawSlice* slices, uint64_t) -> Api::IoCallUint64Result { @@ -132,7 +132,7 @@ class UdpProxyFilterTest : public UdpProxyFilterBase { } })); } else { - EXPECT_CALL(*socket_->io_handle_, isConnected()).WillOnce(Return(false)); + EXPECT_CALL(*socket_->io_handle_, wasConnected()).WillOnce(Return(false)); EXPECT_CALL(*socket_->io_handle_, sendmsg(_, 1, 0, _, _)) .WillOnce(Invoke( [this, data, local_ip, sys_errno]( diff --git a/test/extensions/upstreams/http/udp/upstream_request_test.cc b/test/extensions/upstreams/http/udp/upstream_request_test.cc index 6ee436878cea..216d4e1fe31e 100644 --- a/test/extensions/upstreams/http/udp/upstream_request_test.cc +++ b/test/extensions/upstreams/http/udp/upstream_request_test.cc @@ -83,7 +83,7 @@ TEST_F(UdpUpstreamTest, ExchangeCapsules) { "a1a2a3a4a5a6a7" // UDP Proxying Payload ); Buffer::OwnedImpl sent_capsule(sent_capsule_fragment); - EXPECT_CALL(*mock_socket_->io_handle_, isConnected()).WillOnce(Return(true)); + EXPECT_CALL(*mock_socket_->io_handle_, wasConnected()).WillOnce(Return(true)); EXPECT_CALL(*mock_socket_->io_handle_, writev(_, _)) .WillOnce([](const Buffer::RawSlice* slices, uint64_t num_slice) { Buffer::OwnedImpl buffer(absl::HexStringToBytes("a1a2a3a4a5a6a7")); diff --git a/test/mocks/network/io_handle.h b/test/mocks/network/io_handle.h index ea813e8ca4df..8830e75584e4 100644 --- a/test/mocks/network/io_handle.h +++ b/test/mocks/network/io_handle.h @@ -22,7 +22,7 @@ class MockIoHandle : public IoHandle { MOCK_METHOD(os_fd_t, fdDoNotUse, (), (const)); MOCK_METHOD(Api::IoCallUint64Result, close, ()); MOCK_METHOD(bool, isOpen, (), (const)); - MOCK_METHOD(bool, isConnected, (), (const)); + MOCK_METHOD(bool, wasConnected, (), (const)); MOCK_METHOD(Api::IoCallUint64Result, readv, (uint64_t max_length, Buffer::RawSlice* slices, uint64_t num_slice)); MOCK_METHOD(Api::IoCallUint64Result, read,