From 1d93b1df9132f5b54bba1acebe211fee982d60b7 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 14 Dec 2021 10:15:19 -0500 Subject: [PATCH 1/3] transport_sockets: fixing alpn for wrapped sockets Signed-off-by: Alyssa Wilk --- docs/root/version_history/current.rst | 1 + .../transport_sockets/common/passthrough.h | 20 +++++++++++++++++++ .../proxy_protocol/proxy_protocol.cc | 6 +----- .../proxy_protocol/proxy_protocol.h | 4 +--- .../extensions/transport_sockets/tap/tap.cc | 10 +--------- source/extensions/transport_sockets/tap/tap.h | 8 +------- .../transport_sockets/tcp_stats/config.cc | 12 ++--------- .../transport_sockets/tcp_stats/config.h | 5 +---- .../common/passthrough_test.cc | 20 +++++++++++++++++++ 9 files changed, 48 insertions(+), 38 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 30c930490f58..041f30612fe8 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -21,6 +21,7 @@ Minor Behavior Changes * listener: destroy per network filter chain stats when a network filter chain is removed during the listener in place update. * quic: add back the support for IETF draft 29 which is guarded via ``envoy.reloadable_features.FLAGS_quic_reloadable_flag_quic_disable_version_draft_29``. It is off by default so Envoy only supports RFCv1 without flipping this runtime guard explicitly. Draft 29 is not recommended for use. * router: take elapsed time into account when setting the x-envoy-expected-rq-timeout-ms header for retries, and never send a value that's longer than the request timeout. This behavioral change can be temporarily reverted by setting runtime guard ``envoy.reloadable_features.update_expected_rq_timeout_on_retry`` to false. +* upstream: fixed a bug where auto_config didn't work for wrapped TLS sockets (e.g. if proxy proto were configured for TLS). Bug Fixes --------- diff --git a/source/extensions/transport_sockets/common/passthrough.h b/source/extensions/transport_sockets/common/passthrough.h index ed633024040c..a158df3c9800 100644 --- a/source/extensions/transport_sockets/common/passthrough.h +++ b/source/extensions/transport_sockets/common/passthrough.h @@ -9,6 +9,26 @@ namespace Envoy { namespace Extensions { namespace TransportSockets { +class PassthroughFactory : public Network::TransportSocketFactory { +public: + PassthroughFactory(Network::TransportSocketFactoryPtr&& transport_socket_factory) + : transport_socket_factory_(std::move(transport_socket_factory)) { + ASSERT(transport_socket_factory_.get()); + } + + bool implementsSecureTransport() const override { + return transport_socket_factory_->implementsSecureTransport(); + } + bool supportsAlpn() const override { return transport_socket_factory_->supportsAlpn(); } + bool usesProxyProtocolOptions() const override { + return transport_socket_factory_->usesProxyProtocolOptions(); + } + +protected: + // The wrapped factory + Network::TransportSocketFactoryPtr transport_socket_factory_; +}; + class PassthroughSocket : public Network::TransportSocket { public: PassthroughSocket(Network::TransportSocketPtr&& transport_socket); diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc index 627d70d12958..d14482e24c2c 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.cc @@ -107,7 +107,7 @@ void UpstreamProxyProtocolSocket::onConnected() { UpstreamProxyProtocolSocketFactory::UpstreamProxyProtocolSocketFactory( Network::TransportSocketFactoryPtr transport_socket_factory, ProxyProtocolConfig config) - : transport_socket_factory_(std::move(transport_socket_factory)), config_(config) {} + : PassthroughFactory(std::move(transport_socket_factory)), config_(config) {} Network::TransportSocketPtr UpstreamProxyProtocolSocketFactory::createTransportSocket( Network::TransportSocketOptionsConstSharedPtr options) const { @@ -119,10 +119,6 @@ Network::TransportSocketPtr UpstreamProxyProtocolSocketFactory::createTransportS config_.version()); } -bool UpstreamProxyProtocolSocketFactory::implementsSecureTransport() const { - return transport_socket_factory_->implementsSecureTransport(); -} - } // namespace ProxyProtocol } // namespace TransportSockets } // namespace Extensions diff --git a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h index 521804758417..43f283252912 100644 --- a/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h +++ b/source/extensions/transport_sockets/proxy_protocol/proxy_protocol.h @@ -39,7 +39,7 @@ class UpstreamProxyProtocolSocket : public TransportSockets::PassthroughSocket, ProxyProtocolConfig_Version version_{ProxyProtocolConfig_Version::ProxyProtocolConfig_Version_V1}; }; -class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactory { +class UpstreamProxyProtocolSocketFactory : public PassthroughFactory { public: UpstreamProxyProtocolSocketFactory(Network::TransportSocketFactoryPtr transport_socket_factory, ProxyProtocolConfig config); @@ -47,11 +47,9 @@ class UpstreamProxyProtocolSocketFactory : public Network::TransportSocketFactor // Network::TransportSocketFactory Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsConstSharedPtr options) const override; - bool implementsSecureTransport() const override; bool usesProxyProtocolOptions() const override { return true; } private: - Network::TransportSocketFactoryPtr transport_socket_factory_; ProxyProtocolConfig config_; }; diff --git a/source/extensions/transport_sockets/tap/tap.cc b/source/extensions/transport_sockets/tap/tap.cc index 0cbab1c8b066..982c55747c69 100644 --- a/source/extensions/transport_sockets/tap/tap.cc +++ b/source/extensions/transport_sockets/tap/tap.cc @@ -54,7 +54,7 @@ TapSocketFactory::TapSocketFactory( Network::TransportSocketFactoryPtr&& transport_socket_factory) : ExtensionConfigBase(proto_config.common_config(), std::move(config_factory), admin, singleton_manager, tls, main_thread_dispatcher), - transport_socket_factory_(std::move(transport_socket_factory)) {} + PassthroughFactory(std::move(transport_socket_factory)) {} Network::TransportSocketPtr TapSocketFactory::createTransportSocket( Network::TransportSocketOptionsConstSharedPtr options) const { @@ -62,14 +62,6 @@ Network::TransportSocketPtr TapSocketFactory::createTransportSocket( transport_socket_factory_->createTransportSocket(options)); } -bool TapSocketFactory::implementsSecureTransport() const { - return transport_socket_factory_->implementsSecureTransport(); -} - -bool TapSocketFactory::usesProxyProtocolOptions() const { - return transport_socket_factory_->usesProxyProtocolOptions(); -} - } // namespace Tap } // namespace TransportSockets } // namespace Extensions diff --git a/source/extensions/transport_sockets/tap/tap.h b/source/extensions/transport_sockets/tap/tap.h index 6e15f159e33c..a0e9b4459982 100644 --- a/source/extensions/transport_sockets/tap/tap.h +++ b/source/extensions/transport_sockets/tap/tap.h @@ -28,8 +28,7 @@ class TapSocket : public TransportSockets::PassthroughSocket { PerSocketTapperPtr tapper_; }; -class TapSocketFactory : public Network::TransportSocketFactory, - public Common::Tap::ExtensionConfigBase { +class TapSocketFactory : public Common::Tap::ExtensionConfigBase, public PassthroughFactory { public: TapSocketFactory(const envoy::extensions::transport_sockets::tap::v3::Tap& proto_config, Common::Tap::TapConfigFactoryPtr&& config_factory, Server::Admin& admin, @@ -40,11 +39,6 @@ class TapSocketFactory : public Network::TransportSocketFactory, // Network::TransportSocketFactory Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsConstSharedPtr options) const override; - bool implementsSecureTransport() const override; - bool usesProxyProtocolOptions() const override; - -private: - Network::TransportSocketFactoryPtr transport_socket_factory_; }; } // namespace Tap diff --git a/source/extensions/transport_sockets/tcp_stats/config.cc b/source/extensions/transport_sockets/tcp_stats/config.cc index a293dd5b977c..417d30891d9a 100644 --- a/source/extensions/transport_sockets/tcp_stats/config.cc +++ b/source/extensions/transport_sockets/tcp_stats/config.cc @@ -16,7 +16,7 @@ TcpStatsSocketFactory::TcpStatsSocketFactory( Server::Configuration::TransportSocketFactoryContext& context, const envoy::extensions::transport_sockets::tcp_stats::v3::Config& config, Network::TransportSocketFactoryPtr&& inner_factory) - : inner_factory_(std::move(inner_factory)) { + : PassthroughFactory(std::move(inner_factory)) { #if defined(__linux__) config_ = std::make_shared(config, context.scope()); #else @@ -29,7 +29,7 @@ TcpStatsSocketFactory::TcpStatsSocketFactory( Network::TransportSocketPtr TcpStatsSocketFactory::createTransportSocket( Network::TransportSocketOptionsConstSharedPtr options) const { #if defined(__linux__) - auto inner_socket = inner_factory_->createTransportSocket(options); + auto inner_socket = transport_socket_factory_->createTransportSocket(options); if (inner_socket == nullptr) { return nullptr; } @@ -40,14 +40,6 @@ Network::TransportSocketPtr TcpStatsSocketFactory::createTransportSocket( #endif } -bool TcpStatsSocketFactory::implementsSecureTransport() const { - return inner_factory_->implementsSecureTransport(); -} - -bool TcpStatsSocketFactory::usesProxyProtocolOptions() const { - return inner_factory_->usesProxyProtocolOptions(); -} - class TcpStatsConfigFactory : public virtual Server::Configuration::TransportSocketConfigFactory { public: std::string name() const override { return "envoy.transport_sockets.tcp_stats"; } diff --git a/source/extensions/transport_sockets/tcp_stats/config.h b/source/extensions/transport_sockets/tcp_stats/config.h index 1b5fd20c1038..d31b9104d524 100644 --- a/source/extensions/transport_sockets/tcp_stats/config.h +++ b/source/extensions/transport_sockets/tcp_stats/config.h @@ -10,7 +10,7 @@ namespace Extensions { namespace TransportSockets { namespace TcpStats { -class TcpStatsSocketFactory : public Network::TransportSocketFactory { +class TcpStatsSocketFactory : public PassthroughFactory { public: TcpStatsSocketFactory(Server::Configuration::TransportSocketFactoryContext& context, const envoy::extensions::transport_sockets::tcp_stats::v3::Config& config, @@ -18,11 +18,8 @@ class TcpStatsSocketFactory : public Network::TransportSocketFactory { Network::TransportSocketPtr createTransportSocket(Network::TransportSocketOptionsConstSharedPtr options) const override; - bool implementsSecureTransport() const override; - bool usesProxyProtocolOptions() const override; private: - Network::TransportSocketFactoryPtr inner_factory_; #if defined(__linux__) ConfigConstSharedPtr config_; #endif diff --git a/test/extensions/transport_sockets/common/passthrough_test.cc b/test/extensions/transport_sockets/common/passthrough_test.cc index 6dd45ae0f689..a7b1fa0bfd39 100644 --- a/test/extensions/transport_sockets/common/passthrough_test.cc +++ b/test/extensions/transport_sockets/common/passthrough_test.cc @@ -88,6 +88,26 @@ TEST_F(PassthroughTest, FailOnStartSecureTransport) { EXPECT_FALSE(passthrough_socket_->startSecureTransport()); } +TEST(PassthroughFactoryTest, TestDelegation) { + auto inner_factory_ptr = std::make_unique>(); + Network::MockTransportSocketFactory* inner_factory = inner_factory_ptr.get(); + Network::TransportSocketFactoryPtr factory{std::move(inner_factory_ptr)}; + + { + EXPECT_CALL(*inner_factory, implementsSecureTransport()); + factory->implementsSecureTransport(); + } + + { + EXPECT_CALL(*inner_factory, supportsAlpn()); + factory->supportsAlpn(); + } + { + EXPECT_CALL(*inner_factory, usesProxyProtocolOptions()); + factory->usesProxyProtocolOptions(); + } +} + } // namespace } // namespace TransportSockets } // namespace Extensions From fc88ccdf0294f7897dbfc452bea80a000a3a4c25 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 14 Dec 2021 12:46:01 -0500 Subject: [PATCH 2/3] comments Signed-off-by: Alyssa Wilk --- source/extensions/transport_sockets/common/passthrough.h | 4 ++-- source/extensions/transport_sockets/tcp_stats/config.h | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/source/extensions/transport_sockets/common/passthrough.h b/source/extensions/transport_sockets/common/passthrough.h index a158df3c9800..40d8ef2daa39 100644 --- a/source/extensions/transport_sockets/common/passthrough.h +++ b/source/extensions/transport_sockets/common/passthrough.h @@ -13,7 +13,7 @@ class PassthroughFactory : public Network::TransportSocketFactory { public: PassthroughFactory(Network::TransportSocketFactoryPtr&& transport_socket_factory) : transport_socket_factory_(std::move(transport_socket_factory)) { - ASSERT(transport_socket_factory_.get()); + ASSERT(transport_sockiet_factory_ != nullptr); } bool implementsSecureTransport() const override { @@ -25,7 +25,7 @@ class PassthroughFactory : public Network::TransportSocketFactory { } protected: - // The wrapped factory + // The wrapped factory. Network::TransportSocketFactoryPtr transport_socket_factory_; }; diff --git a/source/extensions/transport_sockets/tcp_stats/config.h b/source/extensions/transport_sockets/tcp_stats/config.h index d31b9104d524..5d008d4b9bf4 100644 --- a/source/extensions/transport_sockets/tcp_stats/config.h +++ b/source/extensions/transport_sockets/tcp_stats/config.h @@ -3,6 +3,7 @@ #include "envoy/extensions/transport_sockets/tcp_stats/v3/tcp_stats.pb.h" #include "envoy/server/transport_socket_config.h" +#include "source/extensions/transport_sockets/common/passthrough.h" #include "source/extensions/transport_sockets/tcp_stats/tcp_stats.h" namespace Envoy { From d58640cd3d22ccf6da1a49fab290a46d486e3ca9 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 14 Dec 2021 13:31:07 -0500 Subject: [PATCH 3/3] typo Signed-off-by: Alyssa Wilk --- source/extensions/transport_sockets/common/passthrough.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/transport_sockets/common/passthrough.h b/source/extensions/transport_sockets/common/passthrough.h index 40d8ef2daa39..4731e5ebbb0b 100644 --- a/source/extensions/transport_sockets/common/passthrough.h +++ b/source/extensions/transport_sockets/common/passthrough.h @@ -13,7 +13,7 @@ class PassthroughFactory : public Network::TransportSocketFactory { public: PassthroughFactory(Network::TransportSocketFactoryPtr&& transport_socket_factory) : transport_socket_factory_(std::move(transport_socket_factory)) { - ASSERT(transport_sockiet_factory_ != nullptr); + ASSERT(transport_socket_factory_ != nullptr); } bool implementsSecureTransport() const override {