From a0624a30b08a532dff6564b79972debc41a28e72 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Tue, 26 Oct 2021 20:42:38 +0800 Subject: [PATCH 1/6] socket option: add support for directly creating ipv4/ipv6 pairs Signed-off-by: Mike Schore --- envoy/network/socket.h | 8 ++++++++ .../addr_family_aware_socket_option_impl.cc | 19 +++++++++---------- .../addr_family_aware_socket_option_impl.h | 12 ++++++++---- source/common/network/socket_option_impl.h | 5 +---- .../win32_redirect_records_option_impl.h | 2 +- .../original_src/original_src_socket_option.h | 1 + 6 files changed, 28 insertions(+), 19 deletions(-) diff --git a/envoy/network/socket.h b/envoy/network/socket.h index 225b76297a06..a529d7cfb808 100644 --- a/envoy/network/socket.h +++ b/envoy/network/socket.h @@ -299,8 +299,16 @@ class Socket { virtual absl::optional
getOptionDetails(const Socket& socket, envoy::config::core::v3::SocketOption::SocketState state) const PURE; + + /** + * Implementations should typically return true. Unsupported or placeholder implementations + * may indicate such by returning false. + * @return Whether this is an actual socket option. + */ + virtual bool isSupported() const PURE; }; + using OptionConstPtr = std::unique_ptr; using OptionConstSharedPtr = std::shared_ptr; using Options = std::vector; using OptionsSharedPtr = std::shared_ptr; diff --git a/source/common/network/addr_family_aware_socket_option_impl.cc b/source/common/network/addr_family_aware_socket_option_impl.cc index 8c4c756a1093..ac234a6d783b 100644 --- a/source/common/network/addr_family_aware_socket_option_impl.cc +++ b/source/common/network/addr_family_aware_socket_option_impl.cc @@ -1,7 +1,7 @@ #include "source/common/network/addr_family_aware_socket_option_impl.h" #include "envoy/common/exception.h" -#include "envoy/common/platform.h" +#include "envoy/common/optref.h" #include "envoy/config/core/v3/base.pb.h" #include "source/common/api/os_sys_calls_impl.h" @@ -14,11 +14,11 @@ namespace Network { namespace { -SocketOptionImplOptRef getOptionForSocket(const Socket& socket, SocketOptionImpl& ipv4_option, - SocketOptionImpl& ipv6_option) { + OptRef getOptionForSocket(const Socket& socket, const Socket::Option& ipv4_option, + const Socket::Option& ipv6_option) { auto version = socket.ipVersion(); if (!version.has_value()) { - return absl::nullopt; + return {}; } // If the FD is v4, we can only try the IPv4 variant. @@ -38,7 +38,7 @@ SocketOptionImplOptRef getOptionForSocket(const Socket& socket, SocketOptionImpl bool AddrFamilyAwareSocketOptionImpl::setOption( Socket& socket, envoy::config::core::v3::SocketOption::SocketState state) const { - return setIpSocketOption(socket, state, ipv4_option_, ipv6_option_); + return setIpSocketOption(socket, state, *ipv4_option_, *ipv6_option_); } absl::optional AddrFamilyAwareSocketOptionImpl::getOptionDetails( @@ -49,21 +49,20 @@ absl::optional AddrFamilyAwareSocketOptionImpl::getOpti return absl::nullopt; } - return option->get().getOptionDetails(socket, state); + return option.value().get().getOptionDetails(socket, state); } bool AddrFamilyAwareSocketOptionImpl::setIpSocketOption( Socket& socket, envoy::config::core::v3::SocketOption::SocketState state, - const std::unique_ptr& ipv4_option, - const std::unique_ptr& ipv6_option) { - auto option = getOptionForSocket(socket, *ipv4_option, *ipv6_option); + const Socket::Option& ipv4_option, const Socket::Option& ipv6_option) { + auto option = getOptionForSocket(socket, ipv4_option, ipv6_option); if (!option.has_value()) { ENVOY_LOG(warn, "Failed to set IP socket option on non-IP socket"); return false; } - return option->get().setOption(socket, state); + return option.value().get().setOption(socket, state); } } // namespace Network diff --git a/source/common/network/addr_family_aware_socket_option_impl.h b/source/common/network/addr_family_aware_socket_option_impl.h index ff7ada58a947..5c7c5975467e 100644 --- a/source/common/network/addr_family_aware_socket_option_impl.h +++ b/source/common/network/addr_family_aware_socket_option_impl.h @@ -29,6 +29,9 @@ class AddrFamilyAwareSocketOptionImpl : public Socket::Option, SocketOptionName ipv6_optname, absl::string_view ipv6_value) : ipv4_option_(std::make_unique(in_state, ipv4_optname, ipv4_value)), ipv6_option_(std::make_unique(in_state, ipv6_optname, ipv6_value)) {} + AddrFamilyAwareSocketOptionImpl(Socket::OptionConstPtr&& ipv4_option, + Socket::OptionConstPtr&& ipv6_option) + : ipv4_option_(std::move(ipv4_option)), ipv6_option_(std::move(ipv6_option)) {} // Socket::Option bool setOption(Socket& socket, @@ -41,6 +44,7 @@ class AddrFamilyAwareSocketOptionImpl : public Socket::Option, absl::optional
getOptionDetails(const Socket& socket, envoy::config::core::v3::SocketOption::SocketState state) const override; + bool isSupported() const override { return true; } /** * Set a socket option that applies at both IPv4 and IPv6 socket levels. When the underlying FD @@ -59,12 +63,12 @@ class AddrFamilyAwareSocketOptionImpl : public Socket::Option, */ static bool setIpSocketOption(Socket& socket, envoy::config::core::v3::SocketOption::SocketState state, - const std::unique_ptr& ipv4_option, - const std::unique_ptr& ipv6_option); + const Socket::Option& ipv4_option, + const Socket::Option& ipv6_option); private: - const std::unique_ptr ipv4_option_; - const std::unique_ptr ipv6_option_; + const Socket::OptionConstPtr ipv4_option_; + const Socket::OptionConstPtr ipv6_option_; }; } // namespace Network diff --git a/source/common/network/socket_option_impl.h b/source/common/network/socket_option_impl.h index fd42517c7bd9..8fe7574c729c 100644 --- a/source/common/network/socket_option_impl.h +++ b/source/common/network/socket_option_impl.h @@ -138,8 +138,7 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable getOptionDetails(const Socket& socket, envoy::config::core::v3::SocketOption::SocketState state) const override; - - bool isSupported() const; + bool isSupported() const override; /** * Set the option on the given socket. @@ -162,7 +161,5 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable value_; }; -using SocketOptionImplOptRef = absl::optional>; - } // namespace Network } // namespace Envoy diff --git a/source/common/network/win32_redirect_records_option_impl.h b/source/common/network/win32_redirect_records_option_impl.h index efa88048d970..6ec1707aab77 100644 --- a/source/common/network/win32_redirect_records_option_impl.h +++ b/source/common/network/win32_redirect_records_option_impl.h @@ -25,8 +25,8 @@ class Win32RedirectRecordsOptionImpl : public Socket::Option, absl::optional
getOptionDetails(const Socket& socket, envoy::config::core::v3::SocketOption::SocketState) const override; + bool isSupported() const override; - bool isSupported() const; static const Network::SocketOptionName& optionName(); private: diff --git a/source/extensions/filters/common/original_src/original_src_socket_option.h b/source/extensions/filters/common/original_src/original_src_socket_option.h index 8e86dc87d719..8e1ffc16b020 100644 --- a/source/extensions/filters/common/original_src/original_src_socket_option.h +++ b/source/extensions/filters/common/original_src/original_src_socket_option.h @@ -36,6 +36,7 @@ class OriginalSrcSocketOption : public Network::Socket::Option { absl::optional
getOptionDetails(const Network::Socket& socket, envoy::config::core::v3::SocketOption::SocketState state) const override; + bool isSupported() const override { return true; } private: Network::Address::InstanceConstSharedPtr src_address_; From b296c2125de72a3be2ef92d2900cec86c6e5f9ce Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Wed, 27 Oct 2021 01:49:10 +0800 Subject: [PATCH 2/6] fix format Signed-off-by: Mike Schore --- .../common/network/addr_family_aware_socket_option_impl.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/source/common/network/addr_family_aware_socket_option_impl.cc b/source/common/network/addr_family_aware_socket_option_impl.cc index ac234a6d783b..a820d012f770 100644 --- a/source/common/network/addr_family_aware_socket_option_impl.cc +++ b/source/common/network/addr_family_aware_socket_option_impl.cc @@ -14,8 +14,9 @@ namespace Network { namespace { - OptRef getOptionForSocket(const Socket& socket, const Socket::Option& ipv4_option, - const Socket::Option& ipv6_option) { +OptRef getOptionForSocket(const Socket& socket, + const Socket::Option& ipv4_option, + const Socket::Option& ipv6_option) { auto version = socket.ipVersion(); if (!version.has_value()) { return {}; From 879582ea5d68207cea1059a0048eb77f29b6e390 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Wed, 27 Oct 2021 17:15:14 +0800 Subject: [PATCH 3/6] update comment Signed-off-by: Mike Schore --- envoy/network/socket.h | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/envoy/network/socket.h b/envoy/network/socket.h index a529d7cfb808..e9527716cdec 100644 --- a/envoy/network/socket.h +++ b/envoy/network/socket.h @@ -301,9 +301,9 @@ class Socket { envoy::config::core::v3::SocketOption::SocketState state) const PURE; /** - * Implementations should typically return true. Unsupported or placeholder implementations - * may indicate such by returning false. - * @return Whether this is an actual socket option. + * Whether the socket implementation is supported. Real implementations should typically return true. Placeholder implementations + * may indicate such by returning false. Note this does NOT inherently prevent an option from being applied if it's passed to socket/connection interfaces. + * @return Whether this is a supported socket option. */ virtual bool isSupported() const PURE; }; From 72213b1af84ccdba0a63cc15af90434087c08c32 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Wed, 27 Oct 2021 17:19:30 +0800 Subject: [PATCH 4/6] fix format Signed-off-by: Mike Schore --- envoy/network/socket.h | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/envoy/network/socket.h b/envoy/network/socket.h index e9527716cdec..e57899b7f9ea 100644 --- a/envoy/network/socket.h +++ b/envoy/network/socket.h @@ -301,8 +301,10 @@ class Socket { envoy::config::core::v3::SocketOption::SocketState state) const PURE; /** - * Whether the socket implementation is supported. Real implementations should typically return true. Placeholder implementations - * may indicate such by returning false. Note this does NOT inherently prevent an option from being applied if it's passed to socket/connection interfaces. + * Whether the socket implementation is supported. Real implementations should typically return + * true. Placeholder implementations may indicate such by returning false. Note this does NOT + * inherently prevent an option from being applied if it's passed to socket/connection + * interfaces. * @return Whether this is a supported socket option. */ virtual bool isSupported() const PURE; From 4606edeb8050f717519e0bb5fc0eb7c32f049638 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Thu, 28 Oct 2021 02:04:05 +0800 Subject: [PATCH 5/6] try CI again Signed-off-by: Mike Schore From e3ef40f327ce319659ef8657d8e04aae20452364 Mon Sep 17 00:00:00 2001 From: Mike Schore Date: Thu, 28 Oct 2021 02:17:16 +0800 Subject: [PATCH 6/6] fix mock Signed-off-by: Mike Schore --- test/mocks/network/mocks.h | 1 + 1 file changed, 1 insertion(+) diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 820322ff59b0..f22effb4e804 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -306,6 +306,7 @@ class MockSocketOption : public Socket::Option { MOCK_METHOD(void, hashKey, (std::vector&), (const)); MOCK_METHOD(absl::optional, getOptionDetails, (const Socket&, envoy::config::core::v3::SocketOption::SocketState state), (const)); + MOCK_METHOD(bool, isSupported, (), (const)); }; class MockConnectionSocket : public ConnectionSocket {