upstream: Pass in UpstreamPriority not RouteEntry when creating conne…

…ction pools (envoyproxy#30696) upstream: Pass in UpstreamPriority not RouteEntry when creating connection pools Risk Level: N/A - Simple refactor Testing: Existing Docs Changes: N/A Release Notes: N/A Platform Specific Features: N/A Signed-off-by: Ryan Hamilton <[email protected]>
fredyw · Nov 6, 2023 · 58588bd · 58588bd
1 parent 329fd25
commit 58588bd
Show file tree

Hide file tree

Showing 17 changed files with 46 additions and 39 deletions.
diff --git a/envoy/router/router.h b/envoy/router/router.h
@@ -1562,7 +1562,7 @@ class GenericConnPoolFactory : public Envoy::Config::TypedFactory {
   virtual GenericConnPoolPtr
   createGenericConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
                         GenericConnPoolFactory::UpstreamProtocol upstream_protocol,
-                        const RouteEntry& route_entry,
+                        Upstream::ResourcePriority priority,
                         absl::optional<Http::Protocol> downstream_protocol,
                         Upstream::LoadBalancerContext* ctx) const PURE;
 };

diff --git a/source/common/router/router.cc b/source/common/router/router.cc
@@ -824,7 +824,8 @@ Filter::createConnPool(Upstream::ThreadLocalCluster& thread_local_cluster) {
       upstream_protocol = UpstreamProtocol::TCP;
     }
   }
-  return factory->createGenericConnPool(thread_local_cluster, upstream_protocol, *route_entry_,
+  return factory->createGenericConnPool(thread_local_cluster, upstream_protocol,
+                                        route_entry_->priority(),
                                         callbacks_->streamInfo().protocol(), this);
 }
 

diff --git a/source/extensions/upstreams/http/generic/config.cc b/source/extensions/upstreams/http/generic/config.cc
@@ -14,18 +14,17 @@ using UpstreamProtocol = Envoy::Router::GenericConnPoolFactory::UpstreamProtocol
 
 Router::GenericConnPoolPtr GenericGenericConnPoolFactory::createGenericConnPool(
     Upstream::ThreadLocalCluster& thread_local_cluster, UpstreamProtocol upstream_protocol,
-    const Router::RouteEntry& route_entry,
-    absl::optional<Envoy::Http::Protocol> downstream_protocol,
+    Upstream::ResourcePriority priority, absl::optional<Envoy::Http::Protocol> downstream_protocol,
     Upstream::LoadBalancerContext* ctx) const {
   Router::GenericConnPoolPtr conn_pool;
   switch (upstream_protocol) {
   case UpstreamProtocol::HTTP:
     conn_pool = std::make_unique<Upstreams::Http::Http::HttpConnPool>(
-        thread_local_cluster, route_entry, downstream_protocol, ctx);
+        thread_local_cluster, priority, downstream_protocol, ctx);
     return (conn_pool->valid() ? std::move(conn_pool) : nullptr);
   case UpstreamProtocol::TCP:
     conn_pool =
-        std::make_unique<Upstreams::Http::Tcp::TcpConnPool>(thread_local_cluster, route_entry, ctx);
+        std::make_unique<Upstreams::Http::Tcp::TcpConnPool>(thread_local_cluster, priority, ctx);
     return (conn_pool->valid() ? std::move(conn_pool) : nullptr);
   case UpstreamProtocol::UDP:
     conn_pool = std::make_unique<Upstreams::Http::Udp::UdpConnPool>(thread_local_cluster, ctx);

diff --git a/source/extensions/upstreams/http/generic/config.h b/source/extensions/upstreams/http/generic/config.h
@@ -20,7 +20,7 @@ class GenericGenericConnPoolFactory : public Router::GenericConnPoolFactory {
   Router::GenericConnPoolPtr
   createGenericConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
                         Router::GenericConnPoolFactory::UpstreamProtocol upstream_protocol,
-                        const Router::RouteEntry& route_entry,
+                        Upstream::ResourcePriority priority,
                         absl::optional<Envoy::Http::Protocol> downstream_protocol,
                         Upstream::LoadBalancerContext* ctx) const override;
 

diff --git a/source/extensions/upstreams/http/http/config.cc b/source/extensions/upstreams/http/http/config.cc
@@ -12,11 +12,10 @@ using UpstreamProtocol = Envoy::Router::GenericConnPoolFactory::UpstreamProtocol
 
 Router::GenericConnPoolPtr HttpGenericConnPoolFactory::createGenericConnPool(
     Upstream::ThreadLocalCluster& thread_local_cluster, UpstreamProtocol,
-    const Router::RouteEntry& route_entry,
-    absl::optional<Envoy::Http::Protocol> downstream_protocol,
+    Upstream::ResourcePriority priority, absl::optional<Envoy::Http::Protocol> downstream_protocol,
     Upstream::LoadBalancerContext* ctx) const {
   auto ret =
-      std::make_unique<HttpConnPool>(thread_local_cluster, route_entry, downstream_protocol, ctx);
+      std::make_unique<HttpConnPool>(thread_local_cluster, priority, downstream_protocol, ctx);
   return (ret->valid() ? std::move(ret) : nullptr);
 }
 

diff --git a/source/extensions/upstreams/http/http/config.h b/source/extensions/upstreams/http/http/config.h
@@ -20,7 +20,7 @@ class HttpGenericConnPoolFactory : public Router::GenericConnPoolFactory {
   Router::GenericConnPoolPtr
   createGenericConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
                         Router::GenericConnPoolFactory::UpstreamProtocol upstream_protocol,
-                        const Router::RouteEntry& route_entry,
+                        Upstream::ResourcePriority priority,
                         absl::optional<Envoy::Http::Protocol> downstream_protocol,
                         Upstream::LoadBalancerContext* ctx) const override;
 

diff --git a/source/extensions/upstreams/http/http/upstream_request.h b/source/extensions/upstreams/http/http/upstream_request.h
@@ -21,11 +21,10 @@ namespace Http {
 class HttpConnPool : public Router::GenericConnPool, public Envoy::Http::ConnectionPool::Callbacks {
 public:
   HttpConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
-               const Router::RouteEntry& route_entry,
+               Upstream::ResourcePriority priority,
                absl::optional<Envoy::Http::Protocol> downstream_protocol,
                Upstream::LoadBalancerContext* ctx) {
-    pool_data_ =
-        thread_local_cluster.httpConnPool(route_entry.priority(), downstream_protocol, ctx);
+    pool_data_ = thread_local_cluster.httpConnPool(priority, downstream_protocol, ctx);
   }
   ~HttpConnPool() override {
     ASSERT(conn_pool_stream_handle_ == nullptr, "conn_pool_stream_handle not null");

diff --git a/source/extensions/upstreams/http/tcp/config.cc b/source/extensions/upstreams/http/tcp/config.cc
@@ -10,9 +10,9 @@ namespace Tcp {
 
 Router::GenericConnPoolPtr TcpGenericConnPoolFactory::createGenericConnPool(
     Upstream::ThreadLocalCluster& thread_local_cluster,
-    Router::GenericConnPoolFactory::UpstreamProtocol, const Router::RouteEntry& route_entry,
+    Router::GenericConnPoolFactory::UpstreamProtocol, Upstream::ResourcePriority priority,
     absl::optional<Envoy::Http::Protocol>, Upstream::LoadBalancerContext* ctx) const {
-  auto ret = std::make_unique<TcpConnPool>(thread_local_cluster, route_entry, ctx);
+  auto ret = std::make_unique<TcpConnPool>(thread_local_cluster, priority, ctx);
   return (ret->valid() ? std::move(ret) : nullptr);
 }
 

diff --git a/source/extensions/upstreams/http/tcp/config.h b/source/extensions/upstreams/http/tcp/config.h
@@ -20,7 +20,7 @@ class TcpGenericConnPoolFactory : public Router::GenericConnPoolFactory {
   Router::GenericConnPoolPtr
   createGenericConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
                         Router::GenericConnPoolFactory::UpstreamProtocol upstream_protocol,
-                        const Router::RouteEntry& route_entry,
+                        Upstream::ResourcePriority priority,
                         absl::optional<Envoy::Http::Protocol> downstream_protocol,
                         Upstream::LoadBalancerContext* ctx) const override;
   ProtobufTypes::MessagePtr createEmptyConfigProto() override {

diff --git a/source/extensions/upstreams/http/tcp/upstream_request.h b/source/extensions/upstreams/http/tcp/upstream_request.h
@@ -23,8 +23,8 @@ namespace Tcp {
 class TcpConnPool : public Router::GenericConnPool, public Envoy::Tcp::ConnectionPool::Callbacks {
 public:
   TcpConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
-              const Router::RouteEntry& route_entry, Upstream::LoadBalancerContext* ctx) {
-    conn_pool_data_ = thread_local_cluster.tcpConnPool(route_entry.priority(), ctx);
+              Upstream::ResourcePriority priority, Upstream::LoadBalancerContext* ctx) {
+    conn_pool_data_ = thread_local_cluster.tcpConnPool(priority, ctx);
   }
   // Router::GenericConnPool
   void newStream(Router::GenericConnectionPoolCallbacks* callbacks) override {

diff --git a/source/extensions/upstreams/http/udp/config.cc b/source/extensions/upstreams/http/udp/config.cc
@@ -10,7 +10,7 @@ namespace Udp {
 
 Router::GenericConnPoolPtr UdpGenericConnPoolFactory::createGenericConnPool(
     Upstream::ThreadLocalCluster& thread_local_cluster,
-    Router::GenericConnPoolFactory::UpstreamProtocol, const Router::RouteEntry&,
+    Router::GenericConnPoolFactory::UpstreamProtocol, Upstream::ResourcePriority,
     absl::optional<Envoy::Http::Protocol>, Upstream::LoadBalancerContext* ctx) const {
   auto ret = std::make_unique<UdpConnPool>(thread_local_cluster, ctx);
   return (ret->valid() ? std::move(ret) : nullptr);

diff --git a/source/extensions/upstreams/http/udp/config.h b/source/extensions/upstreams/http/udp/config.h
@@ -20,7 +20,7 @@ class UdpGenericConnPoolFactory : public Router::GenericConnPoolFactory {
   Router::GenericConnPoolPtr
   createGenericConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
                         Router::GenericConnPoolFactory::UpstreamProtocol upstream_protocol,
-                        const Router::RouteEntry& route_entry,
+                        Upstream::ResourcePriority priority,
                         absl::optional<Envoy::Http::Protocol> downstream_protocol,
                         Upstream::LoadBalancerContext* ctx) const override;
   ProtobufTypes::MessagePtr createEmptyConfigProto() override {

diff --git a/test/common/http/hcm_router_fuzz_test.cc b/test/common/http/hcm_router_fuzz_test.cc
@@ -359,6 +359,16 @@ class FuzzClusterManager {
     return nullptr;
   }
 
+  FuzzCluster*
+  selectClusterByThreadLocalCluster(Upstream::ThreadLocalCluster& thread_local_cluster) {
+    for (auto& cluster : clusters_) {
+      if (&cluster->tlc_ == &thread_local_cluster) {
+        return cluster.get();
+      }
+    }
+    return nullptr;
+  }
+
   void reset() {
     for (auto& cluster : clusters_) {
       cluster->reset();
@@ -395,15 +405,14 @@ class FuzzGenericConnPoolFactory : public Router::GenericConnPoolFactory {
   std::string name() const override { return "envoy.filters.connection_pools.http.generic"; }
   std::string category() const override { return "envoy.upstreams"; }
   Router::GenericConnPoolPtr
-  createGenericConnPool(Upstream::ThreadLocalCluster&,
+  createGenericConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
                         Router::GenericConnPoolFactory::UpstreamProtocol upstream_protocol,
-                        const Router::RouteEntry& route_entry,
-                        absl::optional<Envoy::Http::Protocol> protocol,
+                        Upstream::ResourcePriority, absl::optional<Envoy::Http::Protocol> protocol,
                         Upstream::LoadBalancerContext*) const override {
     if (upstream_protocol != UpstreamProtocol::HTTP) {
       return nullptr;
     }
-    FuzzCluster* cluster = cluster_manager_.selectClusterByName(route_entry.clusterName());
+    FuzzCluster* cluster = cluster_manager_.selectClusterByThreadLocalCluster(thread_local_cluster);
     if (cluster == nullptr) {
       return nullptr;
     }

diff --git a/test/extensions/upstreams/http/generic/config_test.cc b/test/extensions/upstreams/http/generic/config_test.cc
@@ -21,34 +21,34 @@ class GenericGenericConnPoolFactoryTest : public ::testing::Test {
 
 protected:
   NiceMock<Upstream::MockThreadLocalCluster> thread_local_cluster_;
-  NiceMock<Router::MockRouteEntry> route_entry_;
+  Upstream::ResourcePriority priority_ = Upstream::ResourcePriority::Default;
   Upstream::HostConstSharedPtr host_;
   GenericGenericConnPoolFactory factory_;
 };
 
 TEST_F(GenericGenericConnPoolFactoryTest, CreateValidHttpConnPool) {
   EXPECT_TRUE(factory_.createGenericConnPool(thread_local_cluster_,
                                              Router::GenericConnPoolFactory::UpstreamProtocol::HTTP,
-                                             route_entry_, Envoy::Http::Protocol::Http2, nullptr));
+                                             priority_, Envoy::Http::Protocol::Http2, nullptr));
 }
 
 TEST_F(GenericGenericConnPoolFactoryTest, CreateValidTcpConnPool) {
   EXPECT_TRUE(factory_.createGenericConnPool(thread_local_cluster_,
                                              Router::GenericConnPoolFactory::UpstreamProtocol::TCP,
-                                             route_entry_, Envoy::Http::Protocol::Http2, nullptr));
+                                             priority_, Envoy::Http::Protocol::Http2, nullptr));
 }
 
 TEST_F(GenericGenericConnPoolFactoryTest, CreateValidUdpConnPool) {
   EXPECT_TRUE(factory_.createGenericConnPool(thread_local_cluster_,
                                              Router::GenericConnPoolFactory::UpstreamProtocol::UDP,
-                                             route_entry_, Envoy::Http::Protocol::Http2, nullptr));
+                                             priority_, Envoy::Http::Protocol::Http2, nullptr));
 }
 
 TEST_F(GenericGenericConnPoolFactoryTest, InvalidConnPool) {
   // Passes an invalid UpstreamProtocol and check a nullptr is returned.
   EXPECT_FALSE(factory_.createGenericConnPool(
       thread_local_cluster_, static_cast<Router::GenericConnPoolFactory::UpstreamProtocol>(0xff),
-      route_entry_, Envoy::Http::Protocol::Http2, nullptr));
+      priority_, Envoy::Http::Protocol::Http2, nullptr));
 }
 
 } // namespace Generic

diff --git a/test/extensions/upstreams/http/tcp/upstream_request_test.cc b/test/extensions/upstreams/http/tcp/upstream_request_test.cc
@@ -36,12 +36,12 @@ namespace Tcp {
 class TcpConnPoolTest : public ::testing::Test {
 public:
   TcpConnPoolTest() : host_(std::make_shared<NiceMock<Upstream::MockHost>>()) {
-    NiceMock<Router::MockRouteEntry> route_entry;
+    Upstream::ResourcePriority priority = Upstream::ResourcePriority::Default;
     NiceMock<Upstream::MockClusterManager> cm;
     cm.initializeThreadLocalClusters({"fake_cluster"});
     EXPECT_CALL(cm.thread_local_cluster_, tcpConnPool(_, _))
         .WillOnce(Return(Upstream::TcpPoolData([]() {}, &mock_pool_)));
-    conn_pool_ = std::make_unique<TcpConnPool>(cm.thread_local_cluster_, route_entry, nullptr);
+    conn_pool_ = std::make_unique<TcpConnPool>(cm.thread_local_cluster_, priority, nullptr);
   }
 
   std::unique_ptr<TcpConnPool> conn_pool_;

diff --git a/test/extensions/upstreams/http/udp/config_test.cc b/test/extensions/upstreams/http/udp/config_test.cc
@@ -22,7 +22,7 @@ class UdpGenericConnPoolFactoryTest : public ::testing::Test {
 
 protected:
   NiceMock<Upstream::MockThreadLocalCluster> thread_local_cluster_;
-  NiceMock<Router::MockRouteEntry> route_entry_;
+  Upstream::ResourcePriority priority_ = Upstream::ResourcePriority::Default;
   Upstream::HostConstSharedPtr host_;
   UdpGenericConnPoolFactory factory_;
 };
@@ -32,14 +32,14 @@ TEST_F(UdpGenericConnPoolFactoryTest, CreateValidUdpConnPool) {
   EXPECT_CALL(thread_local_cluster_.lb_, chooseHost).WillOnce(Return(host));
   EXPECT_TRUE(factory_.createGenericConnPool(thread_local_cluster_,
                                              Router::GenericConnPoolFactory::UpstreamProtocol::UDP,
-                                             route_entry_, Envoy::Http::Protocol::Http2, nullptr));
+                                             priority_, Envoy::Http::Protocol::Http2, nullptr));
 }
 
 TEST_F(UdpGenericConnPoolFactoryTest, CreateInvalidUdpConnPool) {
   EXPECT_CALL(thread_local_cluster_.lb_, chooseHost).WillOnce(Return(nullptr));
   EXPECT_FALSE(factory_.createGenericConnPool(thread_local_cluster_,
                                               Router::GenericConnPoolFactory::UpstreamProtocol::UDP,
-                                              route_entry_, Envoy::Http::Protocol::Http2, nullptr));
+                                              priority_, Envoy::Http::Protocol::Http2, nullptr));
 }
 
 } // namespace Udp

diff --git a/test/integration/upstreams/per_host_upstream_config.h b/test/integration/upstreams/per_host_upstream_config.h
@@ -71,10 +71,10 @@ class PerHostHttpUpstream : public Extensions::Upstreams::Http::Http::HttpUpstre
 class PerHostHttpConnPool : public Extensions::Upstreams::Http::Http::HttpConnPool {
 public:
   PerHostHttpConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
-                      const Router::RouteEntry& route_entry,
+                      Upstream::ResourcePriority priority,
                       absl::optional<Envoy::Http::Protocol> downstream_protocol,
                       Upstream::LoadBalancerContext* ctx)
-      : HttpConnPool(thread_local_cluster, route_entry, downstream_protocol, ctx) {}
+      : HttpConnPool(thread_local_cluster, priority, downstream_protocol, ctx) {}
 
   void onPoolReady(Envoy::Http::RequestEncoder& callbacks_encoder,
                    Upstream::HostDescriptionConstSharedPtr host, StreamInfo::StreamInfo& info,
@@ -97,15 +97,15 @@ class PerHostGenericConnPoolFactory : public Router::GenericConnPoolFactory {
   Router::GenericConnPoolPtr
   createGenericConnPool(Upstream::ThreadLocalCluster& thread_local_cluster,
                         Router::GenericConnPoolFactory::UpstreamProtocol upstream_protocol,
-                        const Router::RouteEntry& route_entry,
+                        Upstream::ResourcePriority priority,
                         absl::optional<Envoy::Http::Protocol> downstream_protocol,
                         Upstream::LoadBalancerContext* ctx) const override {
     if (upstream_protocol != UpstreamProtocol::HTTP) {
       // This example factory doesn't support terminating CONNECT/CONNECT-UDP stream.
       return nullptr;
     }
     auto upstream_http_conn_pool = std::make_unique<PerHostHttpConnPool>(
-        thread_local_cluster, route_entry, downstream_protocol, ctx);
+        thread_local_cluster, priority, downstream_protocol, ctx);
     return (upstream_http_conn_pool->valid() ? std::move(upstream_http_conn_pool) : nullptr);
   }