From 5acbda312728fc2aa377b715b5d8fa3bcd2c3ab5 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Fri, 10 Jul 2020 16:16:25 -0700 Subject: [PATCH 1/9] fix sigpipe issue on darwin Signed-off-by: Michael Rebello --- source/common/network/socket_option_factory.cc | 7 +++++++ source/common/network/socket_option_factory.h | 1 + source/common/network/socket_option_impl.h | 6 ++++++ source/common/upstream/upstream_impl.cc | 2 ++ source/server/listener_impl.cc | 1 + 5 files changed, 17 insertions(+) diff --git a/source/common/network/socket_option_factory.cc b/source/common/network/socket_option_factory.cc index 0a888525c8e8..c3c52faf9365 100644 --- a/source/common/network/socket_option_factory.cc +++ b/source/common/network/socket_option_factory.cc @@ -61,6 +61,13 @@ std::unique_ptr SocketOptionFactory::buildSocketMarkOptions(uin return options; } +std::unique_ptr SocketOptionFactory::buildSocketSigpipeOptions() { + std::unique_ptr options = std::make_unique(); + options->push_back(std::make_shared( + envoy::config::core::v3::SocketOption::STATE_PREBIND, ENVOY_SOCKET_SO_NOSIGPIPE, 1)); + return options; +} + std::unique_ptr SocketOptionFactory::buildLiteralOptions( const Protobuf::RepeatedPtrField& socket_options) { auto options = std::make_unique(); diff --git a/source/common/network/socket_option_factory.h b/source/common/network/socket_option_factory.h index e93885e844b9..b7a31dd7d4e8 100644 --- a/source/common/network/socket_option_factory.h +++ b/source/common/network/socket_option_factory.h @@ -26,6 +26,7 @@ class SocketOptionFactory : Logger::Loggable { static std::unique_ptr buildIpFreebindOptions(); static std::unique_ptr buildIpTransparentOptions(); static std::unique_ptr buildSocketMarkOptions(uint32_t mark); + static std::unique_ptr buildSocketSigpipeOptions(); static std::unique_ptr buildTcpFastOpenOptions(uint32_t queue_length); static std::unique_ptr buildLiteralOptions( const Protobuf::RepeatedPtrField& socket_options); diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index 95338adf6f9d..042ca419df81 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -47,6 +47,12 @@ namespace Network { #define ENVOY_SOCKET_SO_MARK Network::SocketOptionName() #endif +#ifdef SO_NOSIGPIPE +#define ENVOY_SOCKET_SO_NOSIGPIPE ENVOY_MAKE_SOCKET_OPTION_NAME(SOL_SOCKET, SO_NOSIGPIPE) +#else +#define ENVOY_SOCKET_SO_NOSIGPIPE Network::SocketOptionName() +#endif + #ifdef SO_REUSEPORT #define ENVOY_SOCKET_SO_REUSEPORT ENVOY_MAKE_SOCKET_OPTION_NAME(SOL_SOCKET, SO_REUSEPORT) #else diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 69ef701a5cf1..9f20fc29a41a 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -102,6 +102,8 @@ parseClusterSocketOptions(const envoy::config::cluster::v3::Cluster& config, const envoy::config::core::v3::BindConfig bind_config) { Network::ConnectionSocket::OptionsSharedPtr cluster_options = std::make_shared(); + Network::Socket::appendOptions(cluster_options, + Network::SocketOptionFactory::buildSocketSigpipeOptions()); // Cluster IP_FREEBIND settings, when set, will override the cluster manager wide settings. if ((bind_config.freebind().value() && !config.upstream_bind_config().has_freebind()) || config.upstream_bind_config().freebind().value()) { diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index fd98d5526210..3f29db411f66 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -371,6 +371,7 @@ void ListenerImpl::buildUdpListenerFactory(Network::Socket::Type socket_type, } void ListenerImpl::buildListenSocketOptions(Network::Socket::Type socket_type) { + addListenSocketOptions(Network::SocketOptionFactory::buildSocketSigpipeOptions()); if (PROTOBUF_GET_WRAPPED_OR_DEFAULT(config_, transparent, false)) { addListenSocketOptions(Network::SocketOptionFactory::buildIpTransparentOptions()); } From e76a0ed1e393e14c14db3c3798234356249f37a4 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Tue, 14 Jul 2020 09:48:29 -0700 Subject: [PATCH 2/9] CR Signed-off-by: Michael Rebello --- source/common/network/socket_option_factory.cc | 3 ++- source/common/network/socket_option_factory.h | 2 +- source/common/upstream/upstream_impl.cc | 4 +++- source/server/listener_impl.cc | 4 +++- 4 files changed, 9 insertions(+), 4 deletions(-) diff --git a/source/common/network/socket_option_factory.cc b/source/common/network/socket_option_factory.cc index c3c52faf9365..227ca6114c49 100644 --- a/source/common/network/socket_option_factory.cc +++ b/source/common/network/socket_option_factory.cc @@ -61,7 +61,8 @@ std::unique_ptr SocketOptionFactory::buildSocketMarkOptions(uin return options; } -std::unique_ptr SocketOptionFactory::buildSocketSigpipeOptions() { +std::unique_ptr SocketOptionFactory::buildSocketNoSigpipeOptions() { + // Provide additional handling for SIGPIPE at the socket layer by converting it to EPIPE. std::unique_ptr options = std::make_unique(); options->push_back(std::make_shared( envoy::config::core::v3::SocketOption::STATE_PREBIND, ENVOY_SOCKET_SO_NOSIGPIPE, 1)); diff --git a/source/common/network/socket_option_factory.h b/source/common/network/socket_option_factory.h index b7a31dd7d4e8..7e54acac820e 100644 --- a/source/common/network/socket_option_factory.h +++ b/source/common/network/socket_option_factory.h @@ -26,7 +26,7 @@ class SocketOptionFactory : Logger::Loggable { static std::unique_ptr buildIpFreebindOptions(); static std::unique_ptr buildIpTransparentOptions(); static std::unique_ptr buildSocketMarkOptions(uint32_t mark); - static std::unique_ptr buildSocketSigpipeOptions(); + static std::unique_ptr buildSocketNoSigpipeOptions(); static std::unique_ptr buildTcpFastOpenOptions(uint32_t queue_length); static std::unique_ptr buildLiteralOptions( const Protobuf::RepeatedPtrField& socket_options); diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 9f20fc29a41a..d0583ee79cc2 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -102,8 +102,10 @@ parseClusterSocketOptions(const envoy::config::cluster::v3::Cluster& config, const envoy::config::core::v3::BindConfig bind_config) { Network::ConnectionSocket::OptionsSharedPtr cluster_options = std::make_shared(); + // The process-wide `signal(SIGPIPE, SIG_IGN)` may fail to handle SIGPIPE if overridden elsewhere + // in the process (i.e., on a mobile client). Some OSes support handling it at the socket layer: Network::Socket::appendOptions(cluster_options, - Network::SocketOptionFactory::buildSocketSigpipeOptions()); + Network::SocketOptionFactory::buildSocketNoSigpipeOptions()); // Cluster IP_FREEBIND settings, when set, will override the cluster manager wide settings. if ((bind_config.freebind().value() && !config.upstream_bind_config().has_freebind()) || config.upstream_bind_config().freebind().value()) { diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 3f29db411f66..b0b11c3e051b 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -371,7 +371,9 @@ void ListenerImpl::buildUdpListenerFactory(Network::Socket::Type socket_type, } void ListenerImpl::buildListenSocketOptions(Network::Socket::Type socket_type) { - addListenSocketOptions(Network::SocketOptionFactory::buildSocketSigpipeOptions()); + // The process-wide `signal(SIGPIPE, SIG_IGN)` may fail to handle SIGPIPE if overridden elsewhere + // in the process (i.e., on a mobile client). Some OSes support handling it at the socket layer: + addListenSocketOptions(Network::SocketOptionFactory::buildSocketNoSigpipeOptions()); if (PROTOBUF_GET_WRAPPED_OR_DEFAULT(config_, transparent, false)) { addListenSocketOptions(Network::SocketOptionFactory::buildIpTransparentOptions()); } From e1ea93e5cc7e095616cbb552b1e085b863073195 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Tue, 14 Jul 2020 10:31:42 -0700 Subject: [PATCH 3/9] spellcheck Signed-off-by: Michael Rebello --- source/common/upstream/upstream_impl.cc | 2 +- source/server/listener_impl.cc | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index d0583ee79cc2..27b4110ee85e 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -102,7 +102,7 @@ parseClusterSocketOptions(const envoy::config::cluster::v3::Cluster& config, const envoy::config::core::v3::BindConfig bind_config) { Network::ConnectionSocket::OptionsSharedPtr cluster_options = std::make_shared(); - // The process-wide `signal(SIGPIPE, SIG_IGN)` may fail to handle SIGPIPE if overridden elsewhere + // The process-wide `signal()` handling may fail to handle SIGPIPE if overridden // in the process (i.e., on a mobile client). Some OSes support handling it at the socket layer: Network::Socket::appendOptions(cluster_options, Network::SocketOptionFactory::buildSocketNoSigpipeOptions()); diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index b0b11c3e051b..164d3a2e31d4 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -371,7 +371,7 @@ void ListenerImpl::buildUdpListenerFactory(Network::Socket::Type socket_type, } void ListenerImpl::buildListenSocketOptions(Network::Socket::Type socket_type) { - // The process-wide `signal(SIGPIPE, SIG_IGN)` may fail to handle SIGPIPE if overridden elsewhere + // The process-wide `signal()` handling may fail to handle SIGPIPE if overridden // in the process (i.e., on a mobile client). Some OSes support handling it at the socket layer: addListenSocketOptions(Network::SocketOptionFactory::buildSocketNoSigpipeOptions()); if (PROTOBUF_GET_WRAPPED_OR_DEFAULT(config_, transparent, false)) { From ae9bb7f1e8e21a4e96b03825702d94264f881039 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Tue, 14 Jul 2020 11:08:08 -0700 Subject: [PATCH 4/9] try Signed-off-by: Michael Rebello --- source/common/network/socket_option_factory.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/socket_option_factory.cc b/source/common/network/socket_option_factory.cc index 227ca6114c49..5c33463a0d85 100644 --- a/source/common/network/socket_option_factory.cc +++ b/source/common/network/socket_option_factory.cc @@ -62,7 +62,7 @@ std::unique_ptr SocketOptionFactory::buildSocketMarkOptions(uin } std::unique_ptr SocketOptionFactory::buildSocketNoSigpipeOptions() { - // Provide additional handling for SIGPIPE at the socket layer by converting it to EPIPE. + // Provide additional handling for `SIGPIPE` at the socket layer by converting it to `EPIPE`. std::unique_ptr options = std::make_unique(); options->push_back(std::make_shared( envoy::config::core::v3::SocketOption::STATE_PREBIND, ENVOY_SOCKET_SO_NOSIGPIPE, 1)); From 02c78c087c7feaa81efb0ac1ef49d92fb4588206 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Wed, 15 Jul 2020 16:12:50 -0700 Subject: [PATCH 5/9] fix some tests Signed-off-by: Michael Rebello --- .../upstream/cluster_manager_impl_test.cc | 53 ++++++++----------- 1 file changed, 21 insertions(+), 32 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 37a078501974..20ad5dfe60c4 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -67,7 +67,7 @@ class ClusterManagerImplTest : public testing::Test { address: socket_address: address: 127.0.0.1 - port_value: 11002 + port_value: 11002 )EOF"; const std::string merge_window_enabled = R"EOF( common_lb_config: @@ -3102,7 +3102,6 @@ class SockoptsTest : public ClusterManagerImplTest { // TODO(tschroed): Extend this to support socket state as well. void expectSetsockopts(const std::vector>& names_vals) { - NiceMock os_sys_calls; TestThreadsafeSingletonInjector os_calls(&os_sys_calls); NiceMock socket; @@ -3146,22 +3145,14 @@ class SockoptsTest : public ClusterManagerImplTest { void expectSetsockoptFreebind() { std::vector> names_vals{ - {ENVOY_SOCKET_IP_FREEBIND, 1}}; + {ENVOY_SOCKET_SO_NOSIGPIPE, 1}, {ENVOY_SOCKET_IP_FREEBIND, 1}}; expectSetsockopts(names_vals); } void expectNoSocketOptions() { - EXPECT_CALL(factory_.tls_.dispatcher_, createClientConnection_(_, _, _, _)) - .WillOnce( - Invoke([this](Network::Address::InstanceConstSharedPtr, - Network::Address::InstanceConstSharedPtr, Network::TransportSocketPtr&, - const Network::ConnectionSocket::OptionsSharedPtr& options) - -> Network::ClientConnection* { - EXPECT_EQ(nullptr, options.get()); - return connection_; - })); - auto conn_data = cluster_manager_->tcpConnForCluster("SockoptsCluster", nullptr); - EXPECT_EQ(connection_, conn_data.connection_.get()); + std::vector> names_vals{ + {ENVOY_SOCKET_SO_NOSIGPIPE, 1}}; + expectSetsockopts(names_vals); } Network::MockClientConnection* connection_ = new NiceMock(); @@ -3290,7 +3281,9 @@ TEST_F(SockoptsTest, SockoptsClusterOnly) { )EOF"; initialize(yaml); std::vector> names_vals{ - {ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; + {ENVOY_SOCKET_SO_NOSIGPIPE, 1}, + {ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, + {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; expectSetsockopts(names_vals); } @@ -3319,7 +3312,9 @@ TEST_F(SockoptsTest, SockoptsClusterManagerOnly) { )EOF"; initialize(yaml); std::vector> names_vals{ - {ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; + {ENVOY_SOCKET_SO_NOSIGPIPE, 1}, + {ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, + {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; expectSetsockopts(names_vals); } @@ -3350,7 +3345,9 @@ TEST_F(SockoptsTest, SockoptsClusterOverride) { )EOF"; initialize(yaml); std::vector> names_vals{ - {ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; + {ENVOY_SOCKET_SO_NOSIGPIPE, 1}, + {ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, + {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; expectSetsockopts(names_vals); } @@ -3399,6 +3396,12 @@ class TcpKeepaliveTest : public ClusterManagerImplTest { options, socket, envoy::config::core::v3::SocketOption::STATE_PREBIND))); return connection_; })); + EXPECT_CALL(socket, setSocketOption(ENVOY_SOCKET_SO_NOSIGPIPE.level(), + ENVOY_SOCKET_SO_NOSIGPIPE.option(), _, sizeof(int))) + .WillOnce(Invoke([](int, int, const void* optval, socklen_t) -> Api::SysCallIntResult { + EXPECT_EQ(1, *static_cast(optval)); + return {0, 0}; + })); EXPECT_CALL(socket, setSocketOption(ENVOY_SOCKET_SO_KEEPALIVE.level(), ENVOY_SOCKET_SO_KEEPALIVE.option(), _, sizeof(int))) .WillOnce(Invoke([](int, int, const void* optval, socklen_t) -> Api::SysCallIntResult { @@ -3436,20 +3439,6 @@ class TcpKeepaliveTest : public ClusterManagerImplTest { EXPECT_EQ(connection_, conn_data.connection_.get()); } - void expectNoSocketOptions() { - EXPECT_CALL(factory_.tls_.dispatcher_, createClientConnection_(_, _, _, _)) - .WillOnce( - Invoke([this](Network::Address::InstanceConstSharedPtr, - Network::Address::InstanceConstSharedPtr, Network::TransportSocketPtr&, - const Network::ConnectionSocket::OptionsSharedPtr& options) - -> Network::ClientConnection* { - EXPECT_EQ(nullptr, options.get()); - return connection_; - })); - auto conn_data = cluster_manager_->tcpConnForCluster("TcpKeepaliveCluster", nullptr); - EXPECT_EQ(connection_, conn_data.connection_.get()); - } - Network::MockClientConnection* connection_ = new NiceMock(); }; @@ -3472,7 +3461,7 @@ TEST_F(TcpKeepaliveTest, TcpKeepaliveUnset) { port_value: 11001 )EOF"; initialize(yaml); - expectNoSocketOptions(); + expectSetsockoptSoKeepalive({}, {}, {}); } TEST_F(TcpKeepaliveTest, TcpKeepaliveCluster) { From b5a4081625288ff43e316aab60a5a48367bc1e52 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Wed, 15 Jul 2020 16:26:10 -0700 Subject: [PATCH 6/9] green locally Signed-off-by: Michael Rebello --- .../upstream/cluster_manager_impl_test.cc | 31 +++++++++++++++++-- 1 file changed, 28 insertions(+), 3 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 20ad5dfe60c4..034d9a29c09f 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3149,7 +3149,7 @@ class SockoptsTest : public ClusterManagerImplTest { expectSetsockopts(names_vals); } - void expectNoSocketOptions() { + void expectOnlyNoSigpipeOptions() { std::vector> names_vals{ {ENVOY_SOCKET_SO_NOSIGPIPE, 1}}; expectSetsockopts(names_vals); @@ -3177,7 +3177,7 @@ TEST_F(SockoptsTest, SockoptsUnset) { port_value: 11001 )EOF"; initialize(yaml); - expectNoSocketOptions(); + expectOnlyNoSigpipeOptions(); } TEST_F(SockoptsTest, FreebindClusterOnly) { @@ -3440,6 +3440,31 @@ class TcpKeepaliveTest : public ClusterManagerImplTest { } Network::MockClientConnection* connection_ = new NiceMock(); + + void expectOnlyNoSigpipeOptions() { + NiceMock os_sys_calls; + TestThreadsafeSingletonInjector os_calls(&os_sys_calls); + NiceMock socket; + EXPECT_CALL(factory_.tls_.dispatcher_, createClientConnection_(_, _, _, _)) + .WillOnce(Invoke([this, &socket](Network::Address::InstanceConstSharedPtr, + Network::Address::InstanceConstSharedPtr, + Network::TransportSocketPtr&, + const Network::ConnectionSocket::OptionsSharedPtr& options) + -> Network::ClientConnection* { + EXPECT_NE(nullptr, options.get()); + EXPECT_TRUE((Network::Socket::applyOptions( + options, socket, envoy::config::core::v3::SocketOption::STATE_PREBIND))); + return connection_; + })); + EXPECT_CALL(socket, setSocketOption(ENVOY_SOCKET_SO_NOSIGPIPE.level(), + ENVOY_SOCKET_SO_NOSIGPIPE.option(), _, sizeof(int))) + .WillOnce(Invoke([](int, int, const void* optval, socklen_t) -> Api::SysCallIntResult { + EXPECT_EQ(1, *static_cast(optval)); + return {0, 0}; + })); + auto conn_data = cluster_manager_->tcpConnForCluster("TcpKeepaliveCluster", nullptr); + EXPECT_EQ(connection_, conn_data.connection_.get()); + } }; TEST_F(TcpKeepaliveTest, TcpKeepaliveUnset) { @@ -3461,7 +3486,7 @@ TEST_F(TcpKeepaliveTest, TcpKeepaliveUnset) { port_value: 11001 )EOF"; initialize(yaml); - expectSetsockoptSoKeepalive({}, {}, {}); + expectOnlyNoSigpipeOptions(); } TEST_F(TcpKeepaliveTest, TcpKeepaliveCluster) { From 41a32cb1b2dde623f0b213349091372b239153a7 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Wed, 15 Jul 2020 16:32:50 -0700 Subject: [PATCH 7/9] formatting Signed-off-by: Michael Rebello --- test/common/upstream/cluster_manager_impl_test.cc | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index 034d9a29c09f..d4089330118b 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3439,11 +3439,7 @@ class TcpKeepaliveTest : public ClusterManagerImplTest { EXPECT_EQ(connection_, conn_data.connection_.get()); } - Network::MockClientConnection* connection_ = new NiceMock(); - void expectOnlyNoSigpipeOptions() { - NiceMock os_sys_calls; - TestThreadsafeSingletonInjector os_calls(&os_sys_calls); NiceMock socket; EXPECT_CALL(factory_.tls_.dispatcher_, createClientConnection_(_, _, _, _)) .WillOnce(Invoke([this, &socket](Network::Address::InstanceConstSharedPtr, @@ -3465,6 +3461,8 @@ class TcpKeepaliveTest : public ClusterManagerImplTest { auto conn_data = cluster_manager_->tcpConnForCluster("TcpKeepaliveCluster", nullptr); EXPECT_EQ(connection_, conn_data.connection_.get()); } + + Network::MockClientConnection* connection_ = new NiceMock(); }; TEST_F(TcpKeepaliveTest, TcpKeepaliveUnset) { From 218658c304e3900f5bf5208c55cf3ba279777da0 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 16 Jul 2020 17:04:02 -0700 Subject: [PATCH 8/9] fmt is messed up Signed-off-by: Jose Nino --- source/common/upstream/BUILD | 5 +- source/common/upstream/upstream_impl.cc | 7 +- source/server/BUILD | 5 +- source/server/listener_impl.cc | 5 +- .../upstream/cluster_manager_impl_test.cc | 94 ++++++++++++++----- 5 files changed, 84 insertions(+), 32 deletions(-) diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 14eb0236b542..10e24cbe368c 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -1,11 +1,11 @@ +licenses(["notice"]) # Apache 2 + load( "//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_package", ) -licenses(["notice"]) # Apache 2 - envoy_package() envoy_cc_library( @@ -438,6 +438,7 @@ envoy_cc_library( "//source/common/network:address_lib", "//source/common/network:resolver_lib", "//source/common/network:socket_option_factory_lib", + "//source/common/network:socket_option_lib", "//source/common/network:utility_lib", "//source/common/protobuf", "//source/common/protobuf:utility_lib", diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 27b4110ee85e..269745bd765a 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -37,6 +37,7 @@ #include "common/network/address_impl.h" #include "common/network/resolver_impl.h" #include "common/network/socket_option_factory.h" +#include "common/network/socket_option_impl.h" #include "common/protobuf/protobuf.h" #include "common/protobuf/utility.h" #include "common/router/config_utility.h" @@ -104,8 +105,10 @@ parseClusterSocketOptions(const envoy::config::cluster::v3::Cluster& config, std::make_shared(); // The process-wide `signal()` handling may fail to handle SIGPIPE if overridden // in the process (i.e., on a mobile client). Some OSes support handling it at the socket layer: - Network::Socket::appendOptions(cluster_options, - Network::SocketOptionFactory::buildSocketNoSigpipeOptions()); + if (ENVOY_SOCKET_SO_NOSIGPIPE.hasValue()) { + Network::Socket::appendOptions(cluster_options, + Network::SocketOptionFactory::buildSocketNoSigpipeOptions()); + } // Cluster IP_FREEBIND settings, when set, will override the cluster manager wide settings. if ((bind_config.freebind().value() && !config.upstream_bind_config().has_freebind()) || config.upstream_bind_config().freebind().value()) { diff --git a/source/server/BUILD b/source/server/BUILD index 16e4ddfb8d63..c1f33a4a52ea 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -1,3 +1,5 @@ +licenses(["notice"]) # Apache 2 + load( "//bazel:envoy_build_system.bzl", "envoy_cc_library", @@ -6,8 +8,6 @@ load( "envoy_select_hot_restart", ) -licenses(["notice"]) # Apache 2 - envoy_package() envoy_cc_library( @@ -47,6 +47,7 @@ envoy_cc_library( "//source/common/config:utility_lib", "//source/common/network:resolver_lib", "//source/common/network:socket_option_factory_lib", + "//source/common/network:socket_option_lib", "//source/common/network:utility_lib", "//source/common/protobuf:utility_lib", "@envoy_api//envoy/config/bootstrap/v3:pkg_cc_proto", diff --git a/source/server/listener_impl.cc b/source/server/listener_impl.cc index 164d3a2e31d4..e25db906f9b3 100644 --- a/source/server/listener_impl.cc +++ b/source/server/listener_impl.cc @@ -16,6 +16,7 @@ #include "common/network/connection_balancer_impl.h" #include "common/network/resolver_impl.h" #include "common/network/socket_option_factory.h" +#include "common/network/socket_option_impl.h" #include "common/network/utility.h" #include "common/protobuf/utility.h" #include "common/runtime/runtime_features.h" @@ -373,7 +374,9 @@ void ListenerImpl::buildUdpListenerFactory(Network::Socket::Type socket_type, void ListenerImpl::buildListenSocketOptions(Network::Socket::Type socket_type) { // The process-wide `signal()` handling may fail to handle SIGPIPE if overridden // in the process (i.e., on a mobile client). Some OSes support handling it at the socket layer: - addListenSocketOptions(Network::SocketOptionFactory::buildSocketNoSigpipeOptions()); + if (ENVOY_SOCKET_SO_NOSIGPIPE.hasValue()) { + addListenSocketOptions(Network::SocketOptionFactory::buildSocketNoSigpipeOptions()); + } if (PROTOBUF_GET_WRAPPED_OR_DEFAULT(config_, transparent, false)) { addListenSocketOptions(Network::SocketOptionFactory::buildIpTransparentOptions()); } diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index d4089330118b..20949ce5aa25 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -3089,6 +3089,8 @@ TEST_F(ClusterManagerInitHelperTest, RemoveClusterWithinInitLoop) { init_helper_.startInitializingSecondaryClusters(); } +using NameVals = std::vector>; + // Validate that when options are set in the ClusterManager and/or Cluster, we see the socket option // propagated to setsockopt(). This is as close to an end-to-end test as we have for this feature, // due to the complexity of creating an integration test involving the network stack. We only test @@ -3101,7 +3103,7 @@ class SockoptsTest : public ClusterManagerImplTest { void TearDown() override { factory_.tls_.shutdownThread(); } // TODO(tschroed): Extend this to support socket state as well. - void expectSetsockopts(const std::vector>& names_vals) { + void expectSetsockopts(const NameVals& names_vals) { NiceMock os_sys_calls; TestThreadsafeSingletonInjector os_calls(&os_sys_calls); NiceMock socket; @@ -3144,17 +3146,32 @@ class SockoptsTest : public ClusterManagerImplTest { } void expectSetsockoptFreebind() { - std::vector> names_vals{ - {ENVOY_SOCKET_SO_NOSIGPIPE, 1}, {ENVOY_SOCKET_IP_FREEBIND, 1}}; + NameVals names_vals{{ENVOY_SOCKET_IP_FREEBIND, 1}}; + if (ENVOY_SOCKET_SO_NOSIGPIPE.hasValue()) { + names_vals.emplace_back(std::make_pair(ENVOY_SOCKET_SO_NOSIGPIPE, 1)); + } expectSetsockopts(names_vals); } void expectOnlyNoSigpipeOptions() { - std::vector> names_vals{ - {ENVOY_SOCKET_SO_NOSIGPIPE, 1}}; + NameVals names_vals{{std::make_pair(ENVOY_SOCKET_SO_NOSIGPIPE, 1)}}; expectSetsockopts(names_vals); } + void expectNoSocketOptions() { + EXPECT_CALL(factory_.tls_.dispatcher_, createClientConnection_(_, _, _, _)) + .WillOnce( + Invoke([this](Network::Address::InstanceConstSharedPtr, + Network::Address::InstanceConstSharedPtr, Network::TransportSocketPtr&, + const Network::ConnectionSocket::OptionsSharedPtr& options) + -> Network::ClientConnection* { + EXPECT_EQ(nullptr, options.get()); + return connection_; + })); + auto conn_data = cluster_manager_->tcpConnForCluster("SockoptsCluster", nullptr); + EXPECT_EQ(connection_, conn_data.connection_.get()); + } + Network::MockClientConnection* connection_ = new NiceMock(); }; @@ -3177,7 +3194,11 @@ TEST_F(SockoptsTest, SockoptsUnset) { port_value: 11001 )EOF"; initialize(yaml); - expectOnlyNoSigpipeOptions(); + if (ENVOY_SOCKET_SO_NOSIGPIPE.hasValue()) { + expectOnlyNoSigpipeOptions(); + } else { + expectNoSocketOptions(); + } } TEST_F(SockoptsTest, FreebindClusterOnly) { @@ -3280,10 +3301,11 @@ TEST_F(SockoptsTest, SockoptsClusterOnly) { )EOF"; initialize(yaml); - std::vector> names_vals{ - {ENVOY_SOCKET_SO_NOSIGPIPE, 1}, - {ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, - {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; + NameVals names_vals{{ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, + {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; + if (ENVOY_SOCKET_SO_NOSIGPIPE.hasValue()) { + names_vals.emplace_back(std::make_pair(ENVOY_SOCKET_SO_NOSIGPIPE, 1)); + } expectSetsockopts(names_vals); } @@ -3311,10 +3333,11 @@ TEST_F(SockoptsTest, SockoptsClusterManagerOnly) { { level: 4, name: 5, int_value: 6, state: STATE_PREBIND }] )EOF"; initialize(yaml); - std::vector> names_vals{ - {ENVOY_SOCKET_SO_NOSIGPIPE, 1}, - {ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, - {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; + NameVals names_vals{{ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, + {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; + if (ENVOY_SOCKET_SO_NOSIGPIPE.hasValue()) { + names_vals.emplace_back(std::make_pair(ENVOY_SOCKET_SO_NOSIGPIPE, 1)); + } expectSetsockopts(names_vals); } @@ -3344,10 +3367,11 @@ TEST_F(SockoptsTest, SockoptsClusterOverride) { socket_options: [{ level: 7, name: 8, int_value: 9, state: STATE_PREBIND }] )EOF"; initialize(yaml); - std::vector> names_vals{ - {ENVOY_SOCKET_SO_NOSIGPIPE, 1}, - {ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, - {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; + NameVals names_vals{{ENVOY_MAKE_SOCKET_OPTION_NAME(1, 2), 3}, + {ENVOY_MAKE_SOCKET_OPTION_NAME(4, 5), 6}}; + if (ENVOY_SOCKET_SO_NOSIGPIPE.hasValue()) { + names_vals.emplace_back(std::make_pair(ENVOY_SOCKET_SO_NOSIGPIPE, 1)); + } expectSetsockopts(names_vals); } @@ -3396,12 +3420,14 @@ class TcpKeepaliveTest : public ClusterManagerImplTest { options, socket, envoy::config::core::v3::SocketOption::STATE_PREBIND))); return connection_; })); - EXPECT_CALL(socket, setSocketOption(ENVOY_SOCKET_SO_NOSIGPIPE.level(), - ENVOY_SOCKET_SO_NOSIGPIPE.option(), _, sizeof(int))) - .WillOnce(Invoke([](int, int, const void* optval, socklen_t) -> Api::SysCallIntResult { - EXPECT_EQ(1, *static_cast(optval)); - return {0, 0}; - })); + if (ENVOY_SOCKET_SO_NOSIGPIPE.hasValue()) { + EXPECT_CALL(socket, setSocketOption(ENVOY_SOCKET_SO_NOSIGPIPE.level(), + ENVOY_SOCKET_SO_NOSIGPIPE.option(), _, sizeof(int))) + .WillOnce(Invoke([](int, int, const void* optval, socklen_t) -> Api::SysCallIntResult { + EXPECT_EQ(1, *static_cast(optval)); + return {0, 0}; + })); + } EXPECT_CALL(socket, setSocketOption(ENVOY_SOCKET_SO_KEEPALIVE.level(), ENVOY_SOCKET_SO_KEEPALIVE.option(), _, sizeof(int))) .WillOnce(Invoke([](int, int, const void* optval, socklen_t) -> Api::SysCallIntResult { @@ -3462,6 +3488,20 @@ class TcpKeepaliveTest : public ClusterManagerImplTest { EXPECT_EQ(connection_, conn_data.connection_.get()); } + void expectNoSocketOptions() { + EXPECT_CALL(factory_.tls_.dispatcher_, createClientConnection_(_, _, _, _)) + .WillOnce( + Invoke([this](Network::Address::InstanceConstSharedPtr, + Network::Address::InstanceConstSharedPtr, Network::TransportSocketPtr&, + const Network::ConnectionSocket::OptionsSharedPtr& options) + -> Network::ClientConnection* { + EXPECT_EQ(nullptr, options.get()); + return connection_; + })); + auto conn_data = cluster_manager_->tcpConnForCluster("TcpKeepaliveCluster", nullptr); + EXPECT_EQ(connection_, conn_data.connection_.get()); + } + Network::MockClientConnection* connection_ = new NiceMock(); }; @@ -3484,7 +3524,11 @@ TEST_F(TcpKeepaliveTest, TcpKeepaliveUnset) { port_value: 11001 )EOF"; initialize(yaml); - expectOnlyNoSigpipeOptions(); + if (ENVOY_SOCKET_SO_NOSIGPIPE.hasValue()) { + expectOnlyNoSigpipeOptions(); + } else { + expectNoSocketOptions(); + } } TEST_F(TcpKeepaliveTest, TcpKeepaliveCluster) { From b0e152030d2c91e0e4743b67e92c2edb69deea68 Mon Sep 17 00:00:00 2001 From: Michael Rebello Date: Thu, 16 Jul 2020 21:15:50 -0700 Subject: [PATCH 9/9] format Signed-off-by: Michael Rebello --- source/common/upstream/BUILD | 4 ++-- source/server/BUILD | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 10e24cbe368c..dfd3aec724b3 100644 --- a/source/common/upstream/BUILD +++ b/source/common/upstream/BUILD @@ -1,11 +1,11 @@ -licenses(["notice"]) # Apache 2 - load( "//bazel:envoy_build_system.bzl", "envoy_cc_library", "envoy_package", ) +licenses(["notice"]) # Apache 2 + envoy_package() envoy_cc_library( diff --git a/source/server/BUILD b/source/server/BUILD index c1f33a4a52ea..33266f13f3f8 100644 --- a/source/server/BUILD +++ b/source/server/BUILD @@ -1,5 +1,3 @@ -licenses(["notice"]) # Apache 2 - load( "//bazel:envoy_build_system.bzl", "envoy_cc_library", @@ -8,6 +6,8 @@ load( "envoy_select_hot_restart", ) +licenses(["notice"]) # Apache 2 + envoy_package() envoy_cc_library(