conn_pool: allowing multiple protocols in ALPN (#13901)

Extending the TransportSocketOptions to allow for multiple fallback ALPN for the upcoming connection pool which supports both HTTP/1 and HTTP/2 Risk Level: Low (data plane refactor, but a pretty minor one) Testing: new unit tests Docs Changes: n/a Release Notes: n/a Part of #3431 Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
envoyproxy · Nov 5, 2020 · 9a205da · 9a205da
1 parent 2f72126
commit 9a205da
Show file tree

Hide file tree

Showing 14 changed files with 159 additions and 51 deletions.
diff --git a/include/envoy/network/transport_socket.h b/include/envoy/network/transport_socket.h
@@ -180,18 +180,18 @@ class TransportSocketOptions {
   virtual const std::vector<std::string>& applicationProtocolListOverride() const PURE;
 
   /**
-   * The application protocol to use when negotiating an upstream connection and no other
+   * The application protocol(s) to use when negotiating an upstream connection and no other
    * application protocol has been configured. Both
    * TransportSocketOptions::applicationProtocolListOverride and application protocols configured
    * in the CommonTlsContext on the Cluster will take precedence.
    *
    * Note that this option is intended for intermediate code (e.g. the HTTP connection pools) to
    * specify a default ALPN when no specific values are specified elsewhere. As such, providing a
    * value here might not make sense prior to load balancing.
-   * @return the optional fallback for application protocols, for when they are not specified in the
-   *         TLS configuration.
+   * @return the optional fallback(s) for application protocols, for when they are not specified in
+   *         the TLS configuration.
    */
-  virtual const absl::optional<std::string>& applicationProtocolFallback() const PURE;
+  virtual const std::vector<std::string>& applicationProtocolFallback() const PURE;
 
   /**
    * @return optional PROXY protocol address information.

diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc
@@ -12,47 +12,56 @@ namespace Http {
 
 Network::TransportSocketOptionsSharedPtr
 wrapTransportSocketOptions(Network::TransportSocketOptionsSharedPtr transport_socket_options,
-                           Protocol protocol) {
+                           std::vector<Protocol> protocols) {
   if (!Runtime::runtimeFeatureEnabled("envoy.reloadable_features.http_default_alpn")) {
     return transport_socket_options;
   }
 
-  // If configured to do so, we override the ALPN to use for the upstream connection to match the
-  // selected protocol.
-  std::string alpn;
-  switch (protocol) {
-  case Http::Protocol::Http10:
-    NOT_REACHED_GCOVR_EXCL_LINE;
-  case Http::Protocol::Http11:
-    alpn = Http::Utility::AlpnNames::get().Http11;
-    break;
-  case Http::Protocol::Http2:
-    alpn = Http::Utility::AlpnNames::get().Http2;
-    break;
-  case Http::Protocol::Http3:
-    // TODO(snowp): Add once HTTP/3 upstream support is added.
-    NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
-    break;
+  std::vector<std::string> fallbacks;
+  for (auto protocol : protocols) {
+    // If configured to do so, we override the ALPN to use for the upstream connection to match the
+    // selected protocol.
+    switch (protocol) {
+    case Http::Protocol::Http10:
+      NOT_REACHED_GCOVR_EXCL_LINE;
+    case Http::Protocol::Http11:
+      fallbacks.push_back(Http::Utility::AlpnNames::get().Http11);
+      break;
+    case Http::Protocol::Http2:
+      fallbacks.push_back(Http::Utility::AlpnNames::get().Http2);
+      break;
+    case Http::Protocol::Http3:
+      // TODO(snowp): Add once HTTP/3 upstream support is added.
+      NOT_IMPLEMENTED_GCOVR_EXCL_LINE;
+      break;
+    }
   }
 
   if (transport_socket_options) {
     return std::make_shared<Network::AlpnDecoratingTransportSocketOptions>(
-        std::move(alpn), transport_socket_options);
+        std::move(fallbacks), transport_socket_options);
   } else {
     return std::make_shared<Network::TransportSocketOptionsImpl>(
-        "", std::vector<std::string>{}, std::vector<std::string>{}, std::move(alpn));
+        "", std::vector<std::string>{}, std::vector<std::string>{}, std::move(fallbacks));
   }
 }
 
 HttpConnPoolImplBase::HttpConnPoolImplBase(
     Upstream::HostConstSharedPtr host, Upstream::ResourcePriority priority,
     Event::Dispatcher& dispatcher, const Network::ConnectionSocket::OptionsSharedPtr& options,
     const Network::TransportSocketOptionsSharedPtr& transport_socket_options,
-    Random::RandomGenerator& random_generator, Http::Protocol protocol)
+    Random::RandomGenerator& random_generator, std::vector<Http::Protocol> protocols)
     : Envoy::ConnectionPool::ConnPoolImplBase(
           host, priority, dispatcher, options,
-          wrapTransportSocketOptions(transport_socket_options, protocol)),
-      random_generator_(random_generator), protocol_(protocol) {}
+          wrapTransportSocketOptions(transport_socket_options, protocols)),
+      random_generator_(random_generator) {
+  ASSERT(!protocols.empty());
+  // TODO(alyssawilk) the protocol function should probably be an optional and
+  // simply not set if there's more than one and ALPN has not been negotiated.
+  if (!protocols.empty()) {
+    protocol_ = protocols[0];
+  }
+}
 
 ConnectionPool::Cancellable*
 HttpConnPoolImplBase::newStream(Http::ResponseDecoder& response_decoder,

diff --git a/source/common/http/conn_pool_base.h b/source/common/http/conn_pool_base.h
@@ -43,7 +43,8 @@ class HttpConnPoolImplBase : public Envoy::ConnectionPool::ConnPoolImplBase,
                        Event::Dispatcher& dispatcher,
                        const Network::ConnectionSocket::OptionsSharedPtr& options,
                        const Network::TransportSocketOptionsSharedPtr& transport_socket_options,
-                       Random::RandomGenerator& random_generator, Http::Protocol protocol);
+                       Random::RandomGenerator& random_generator,
+                       std::vector<Http::Protocol> protocol);
 
   // ConnectionPool::Instance
   void addDrainedCallback(DrainedCb cb) override { addDrainedCallbackImpl(cb); }

diff --git a/source/common/http/http1/conn_pool.cc b/source/common/http/http1/conn_pool.cc
@@ -28,7 +28,7 @@ ConnPoolImpl::ConnPoolImpl(Event::Dispatcher& dispatcher, Random::RandomGenerato
                            const Network::ConnectionSocket::OptionsSharedPtr& options,
                            const Network::TransportSocketOptionsSharedPtr& transport_socket_options)
     : HttpConnPoolImplBase(std::move(host), std::move(priority), dispatcher, options,
-                           transport_socket_options, random_generator, Protocol::Http11) {}
+                           transport_socket_options, random_generator, {Protocol::Http11}) {}
 
 ConnPoolImpl::~ConnPoolImpl() { destructAllConnections(); }
 

diff --git a/source/common/http/http2/conn_pool.cc b/source/common/http/http2/conn_pool.cc
@@ -21,7 +21,7 @@ ConnPoolImpl::ConnPoolImpl(Event::Dispatcher& dispatcher, Random::RandomGenerato
                            const Network::ConnectionSocket::OptionsSharedPtr& options,
                            const Network::TransportSocketOptionsSharedPtr& transport_socket_options)
     : HttpConnPoolImplBase(std::move(host), std::move(priority), dispatcher, options,
-                           transport_socket_options, random_generator, Protocol::Http2) {}
+                           transport_socket_options, random_generator, {Protocol::Http2}) {}
 
 ConnPoolImpl::~ConnPoolImpl() { destructAllConnections(); }
 

diff --git a/source/common/network/transport_socket_options_impl.cc b/source/common/network/transport_socket_options_impl.cc
@@ -38,8 +38,10 @@ void commonHashKey(const TransportSocketOptions& options, std::vector<std::uint8
   }
 
   const auto& alpn_fallback = options.applicationProtocolFallback();
-  if (alpn_fallback.has_value()) {
-    pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(*alpn_fallback), key);
+  if (!alpn_fallback.empty()) {
+    for (const auto& protocol : alpn_fallback) {
+      pushScalarToByteVector(StringUtil::CaseInsensitiveHash()(protocol), key);
+    }
   }
 
   // Proxy protocol options should only be included in the hash if the upstream
@@ -67,6 +69,7 @@ TransportSocketOptionsUtility::fromFilterState(const StreamInfo::FilterState& fi
   absl::string_view server_name;
   std::vector<std::string> application_protocols;
   std::vector<std::string> subject_alt_names;
+  std::vector<std::string> alpn_fallback;
   absl::optional<Network::ProxyProtocolData> proxy_protocol_options;
 
   bool needs_transport_socket_options = false;
@@ -100,8 +103,8 @@ TransportSocketOptionsUtility::fromFilterState(const StreamInfo::FilterState& fi
 
   if (needs_transport_socket_options) {
     return std::make_shared<Network::TransportSocketOptionsImpl>(
-        server_name, std::move(subject_alt_names), std::move(application_protocols), absl::nullopt,
-        proxy_protocol_options);
+        server_name, std::move(subject_alt_names), std::move(application_protocols),
+        std::move(alpn_fallback), proxy_protocol_options);
   } else {
     return nullptr;
   }

diff --git a/source/common/network/transport_socket_options_impl.h b/source/common/network/transport_socket_options_impl.h
@@ -10,7 +10,7 @@ namespace Network {
 // A wrapper around another TransportSocketOptions that overrides the ALPN fallback.
 class AlpnDecoratingTransportSocketOptions : public TransportSocketOptions {
 public:
-  AlpnDecoratingTransportSocketOptions(std::string&& alpn,
+  AlpnDecoratingTransportSocketOptions(std::vector<std::string>&& alpn,
                                        TransportSocketOptionsSharedPtr inner_options)
       : alpn_fallback_(std::move(alpn)), inner_options_(std::move(inner_options)) {}
   // Network::TransportSocketOptions
@@ -23,7 +23,7 @@ class AlpnDecoratingTransportSocketOptions : public TransportSocketOptions {
   const std::vector<std::string>& applicationProtocolListOverride() const override {
     return inner_options_->applicationProtocolListOverride();
   }
-  const absl::optional<std::string>& applicationProtocolFallback() const override {
+  const std::vector<std::string>& applicationProtocolFallback() const override {
     return alpn_fallback_;
   }
   absl::optional<Network::ProxyProtocolData> proxyProtocolOptions() const override {
@@ -33,7 +33,7 @@ class AlpnDecoratingTransportSocketOptions : public TransportSocketOptions {
                const Network::TransportSocketFactory& factory) const override;
 
 private:
-  const absl::optional<std::string> alpn_fallback_;
+  const std::vector<std::string> alpn_fallback_;
   const TransportSocketOptionsSharedPtr inner_options_;
 };
 
@@ -42,8 +42,7 @@ class TransportSocketOptionsImpl : public TransportSocketOptions {
   TransportSocketOptionsImpl(
       absl::string_view override_server_name = "",
       std::vector<std::string>&& override_verify_san_list = {},
-      std::vector<std::string>&& override_alpn = {},
-      absl::optional<std::string>&& fallback_alpn = {},
+      std::vector<std::string>&& override_alpn = {}, std::vector<std::string>&& fallback_alpn = {},
       absl::optional<Network::ProxyProtocolData> proxy_proto_options = absl::nullopt)
       : override_server_name_(override_server_name.empty()
                                   ? absl::nullopt
@@ -62,7 +61,7 @@ class TransportSocketOptionsImpl : public TransportSocketOptions {
   const std::vector<std::string>& applicationProtocolListOverride() const override {
     return override_alpn_list_;
   }
-  const absl::optional<std::string>& applicationProtocolFallback() const override {
+  const std::vector<std::string>& applicationProtocolFallback() const override {
     return alpn_fallback_;
   }
   absl::optional<Network::ProxyProtocolData> proxyProtocolOptions() const override {
@@ -75,7 +74,7 @@ class TransportSocketOptionsImpl : public TransportSocketOptions {
   const absl::optional<std::string> override_server_name_;
   const std::vector<std::string> override_verify_san_list_;
   const std::vector<std::string> override_alpn_list_;
-  const absl::optional<std::string> alpn_fallback_;
+  const std::vector<std::string> alpn_fallback_;
   const absl::optional<Network::ProxyProtocolData> proxy_protocol_options_;
 };
 

diff --git a/source/extensions/transport_sockets/tls/context_impl.cc b/source/extensions/transport_sockets/tls/context_impl.cc
@@ -963,9 +963,9 @@ bssl::UniquePtr<SSL> ClientContextImpl::newSsl(const Network::TransportSocketOpt
     has_alpn_defined |= parseAndSetAlpn(options->applicationProtocolListOverride(), *ssl_con);
   }
 
-  if (options && !has_alpn_defined && options->applicationProtocolFallback().has_value()) {
+  if (options && !has_alpn_defined && !options->applicationProtocolFallback().empty()) {
     // If ALPN hasn't already been set (either through TLS context or override), use the fallback.
-    parseAndSetAlpn({*options->applicationProtocolFallback()}, *ssl_con);
+    parseAndSetAlpn(options->applicationProtocolFallback(), *ssl_con);
   }
 
   if (allow_renegotiation_) {

diff --git a/test/common/http/BUILD b/test/common/http/BUILD
@@ -397,6 +397,22 @@ envoy_cc_test(
     ],
 )
 
+envoy_cc_test(
+    name = "mixed_conn_pool_test",
+    srcs = ["mixed_conn_pool_test.cc"],
+    deps = [
+        "//source/common/http:conn_pool_base_lib",
+        "//test/common/upstream:utility_lib",
+        "//test/mocks:common_lib",
+        "//test/mocks/buffer:buffer_mocks",
+        "//test/mocks/http:http_mocks",
+        "//test/mocks/local_info:local_info_mocks",
+        "//test/mocks/router:router_mocks",
+        "//test/mocks/runtime:runtime_mocks",
+        "//test/mocks/stats:stats_mocks",
+    ],
+)
+
 envoy_proto_library(
     name = "path_utility_fuzz_proto",
     srcs = ["path_utility_fuzz.proto"],

diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc
@@ -274,7 +274,7 @@ TEST_F(Http1ConnPoolImplTest, VerifyAlpnFallback) {
       .WillOnce(Invoke(
           [](Network::TransportSocketOptionsSharedPtr options) -> Network::TransportSocketPtr {
             EXPECT_TRUE(options != nullptr);
-            EXPECT_EQ(options->applicationProtocolFallback(),
+            EXPECT_EQ(options->applicationProtocolFallback()[0],
                       Http::Utility::AlpnNames::get().Http11);
             return std::make_unique<Network::RawBufferSocket>();
           }));

diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc
@@ -329,7 +329,7 @@ TEST_F(Http2ConnPoolImplTest, VerifyAlpnFallback) {
       .WillOnce(Invoke(
           [](Network::TransportSocketOptionsSharedPtr options) -> Network::TransportSocketPtr {
             EXPECT_TRUE(options != nullptr);
-            EXPECT_EQ(options->applicationProtocolFallback(),
+            EXPECT_EQ(options->applicationProtocolFallback()[0],
                       Http::Utility::AlpnNames::get().Http2);
             return std::make_unique<Network::RawBufferSocket>();
           }));

diff --git a/test/common/http/mixed_conn_pool_test.cc b/test/common/http/mixed_conn_pool_test.cc
@@ -0,0 +1,65 @@
+#include <memory>
+
+#include "common/http/conn_pool_base.h"
+#include "common/http/utility.h"
+
+#include "test/common/upstream/utility.h"
+#include "test/mocks/common.h"
+#include "test/mocks/event/mocks.h"
+#include "test/mocks/runtime/mocks.h"
+#include "test/mocks/upstream/cluster_info.h"
+#include "test/test_common/simulated_time_system.h"
+#include "test/test_common/utility.h"
+
+#include "gmock/gmock.h"
+#include "gtest/gtest.h"
+
+namespace Envoy {
+namespace Http {
+namespace {
+
+// TODO(alyssawilk) replace this with the MixedConnectionPool once it lands.
+class ConnPoolImplForTest : public HttpConnPoolImplBase {
+public:
+  ConnPoolImplForTest(Event::MockDispatcher& dispatcher, Random::RandomGenerator& random,
+                      Upstream::ClusterInfoConstSharedPtr cluster)
+      : HttpConnPoolImplBase(Upstream::makeTestHost(cluster, "tcp://127.0.0.1:9000"),
+                             Upstream::ResourcePriority::Default, dispatcher, nullptr, nullptr,
+                             random, {Http::Protocol::Http2, Http::Protocol::Http11}) {}
+
+  Envoy::ConnectionPool::ActiveClientPtr instantiateActiveClient() override { return nullptr; }
+  Http::Protocol protocol() const override { return Http::Protocol::Http2; }
+  CodecClientPtr createCodecClient(Upstream::Host::CreateConnectionData&) override {
+    return nullptr;
+  }
+};
+
+/**
+ * Test fixture for a connection pool which can have HTTP/2 or HTTP/1.1 connections.
+ */
+class MixedConnPoolImplTest : public testing::Test {
+public:
+  MixedConnPoolImplTest()
+      : conn_pool_(std::make_unique<ConnPoolImplForTest>(dispatcher_, random_, cluster_)) {}
+
+  ~MixedConnPoolImplTest() override {
+    EXPECT_EQ("", TestUtility::nonZeroedGauges(cluster_->stats_store_.gauges()));
+  }
+
+  NiceMock<Event::MockDispatcher> dispatcher_;
+  std::shared_ptr<Upstream::MockClusterInfo> cluster_{new NiceMock<Upstream::MockClusterInfo>()};
+  std::unique_ptr<ConnPoolImplForTest> conn_pool_;
+  NiceMock<Runtime::MockLoader> runtime_;
+  NiceMock<Random::MockRandomGenerator> random_;
+};
+
+TEST_F(MixedConnPoolImplTest, AlpnTest) {
+  auto& fallback = conn_pool_->transportSocketOptions()->applicationProtocolFallback();
+  ASSERT_EQ(2, fallback.size());
+  EXPECT_EQ(fallback[0], Http::Utility::AlpnNames::get().Http2);
+  EXPECT_EQ(fallback[1], Http::Utility::AlpnNames::get().Http11);
+}
+
+} // namespace
+} // namespace Http
+} // namespace Envoy
diff --git a/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_test.cc b/test/extensions/transport_sockets/proxy_protocol/proxy_protocol_test.cc
@@ -256,7 +256,7 @@ TEST_F(ProxyProtocolTest, V1IPV4DownstreamAddresses) {
       new Network::Address::Ipv4Instance("174.2.2.222", 80));
   Network::TransportSocketOptionsSharedPtr socket_options =
       std::make_shared<Network::TransportSocketOptionsImpl>(
-          "", std::vector<std::string>{}, std::vector<std::string>{}, absl::nullopt,
+          "", std::vector<std::string>{}, std::vector<std::string>{}, std::vector<std::string>{},
           absl::optional<Network::ProxyProtocolData>(
               Network::ProxyProtocolData{src_addr, dst_addr}));
   transport_callbacks_.connection_.local_address_ =
@@ -288,7 +288,7 @@ TEST_F(ProxyProtocolTest, V1IPV6DownstreamAddresses) {
       Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv6Instance("a:b:c:d::", 80));
   Network::TransportSocketOptionsSharedPtr socket_options =
       std::make_shared<Network::TransportSocketOptionsImpl>(
-          "", std::vector<std::string>{}, std::vector<std::string>{}, absl::nullopt,
+          "", std::vector<std::string>{}, std::vector<std::string>{}, std::vector<std::string>{},
           absl::optional<Network::ProxyProtocolData>(
               Network::ProxyProtocolData{src_addr, dst_addr}));
   transport_callbacks_.connection_.local_address_ =
@@ -365,7 +365,7 @@ TEST_F(ProxyProtocolTest, V2IPV4DownstreamAddresses) {
       Network::Address::InstanceConstSharedPtr(new Network::Address::Ipv4Instance("0.1.1.2", 513));
   Network::TransportSocketOptionsSharedPtr socket_options =
       std::make_shared<Network::TransportSocketOptionsImpl>(
-          "", std::vector<std::string>{}, std::vector<std::string>{}, absl::nullopt,
+          "", std::vector<std::string>{}, std::vector<std::string>{}, std::vector<std::string>{},
           absl::optional<Network::ProxyProtocolData>(
               Network::ProxyProtocolData{src_addr, dst_addr}));
   transport_callbacks_.connection_.local_address_ =
@@ -397,7 +397,7 @@ TEST_F(ProxyProtocolTest, V2IPV6DownstreamAddresses) {
       new Network::Address::Ipv6Instance("1:100:200:3::", 2));
   Network::TransportSocketOptionsSharedPtr socket_options =
       std::make_shared<Network::TransportSocketOptionsImpl>(
-          "", std::vector<std::string>{}, std::vector<std::string>{}, absl::nullopt,
+          "", std::vector<std::string>{}, std::vector<std::string>{}, std::vector<std::string>{},
           absl::optional<Network::ProxyProtocolData>(
               Network::ProxyProtocolData{src_addr, dst_addr}));
   transport_callbacks_.connection_.local_address_ =
@@ -429,7 +429,7 @@ TEST_F(ProxyProtocolTest, OnConnectedCallsInnerOnConnected) {
       new Network::Address::Ipv6Instance("1:100:200:3::", 2));
   Network::TransportSocketOptionsSharedPtr socket_options =
       std::make_shared<Network::TransportSocketOptionsImpl>(
-          "", std::vector<std::string>{}, std::vector<std::string>{}, absl::nullopt,
+          "", std::vector<std::string>{}, std::vector<std::string>{}, std::vector<std::string>{},
           absl::optional<Network::ProxyProtocolData>(
               Network::ProxyProtocolData{src_addr, dst_addr}));
   transport_callbacks_.connection_.local_address_ =
@@ -473,4 +473,4 @@ TEST_F(ProxyProtocolSocketFactoryTest, ImplementsSecureTransportCallInnerFactory
 } // namespace ProxyProtocol
 } // namespace TransportSockets
 } // namespace Extensions
-} // namespace Envoy
+} // namespace Envoy