conn_pool: unifying status codes (#10854)

Risk Level: Medium (minor HTTP/TCP refactor) Testing: unit tests Docs Changes: n/a Release Notes: n/a Signed-off-by: Alyssa Wilk <alyssar@chromium.org>
envoyproxy · Apr 21, 2020 · 6aee603 · 6aee603
1 parent 3bcfc2b
commit 6aee603
Show file tree

Hide file tree

Showing 29 changed files with 164 additions and 140 deletions.
diff --git a/include/envoy/common/BUILD b/include/envoy/common/BUILD
@@ -19,6 +19,11 @@ envoy_basic_cc_library(
     include_prefix = "envoy/common",
 )
 
+envoy_cc_library(
+    name = "conn_pool_interface",
+    hdrs = ["conn_pool.h"],
+)
+
 envoy_cc_library(
     name = "mutex_tracer",
     hdrs = ["mutex_tracer.h"],

diff --git a/include/envoy/common/conn_pool.h b/include/envoy/common/conn_pool.h
@@ -0,0 +1,18 @@
+#pragma once
+
+namespace Envoy {
+namespace ConnectionPool {
+
+enum class PoolFailureReason {
+  // A resource overflowed and policy prevented a new connection from being created.
+  Overflow,
+  // A local connection failure took place while creating a new connection.
+  LocalConnectionFailure,
+  // A remote connection failure took place while creating a new connection.
+  RemoteConnectionFailure,
+  // A timeout occurred while creating a new connection.
+  Timeout,
+};
+
+} // namespace ConnectionPool
+} // namespace Envoy
diff --git a/include/envoy/http/BUILD b/include/envoy/http/BUILD
@@ -50,6 +50,7 @@ envoy_cc_library(
     hdrs = ["conn_pool.h"],
     deps = [
         ":codec_interface",
+        "//include/envoy/common:conn_pool_interface",
         "//include/envoy/event:deferred_deletable",
         "//include/envoy/upstream:upstream_interface",
     ],

diff --git a/include/envoy/http/conn_pool.h b/include/envoy/http/conn_pool.h
@@ -3,6 +3,7 @@
 #include <functional>
 #include <memory>
 
+#include "envoy/common/conn_pool.h"
 #include "envoy/common/pure.h"
 #include "envoy/event/deferred_deletable.h"
 #include "envoy/http/codec.h"
@@ -25,15 +26,7 @@ class Cancellable {
   virtual void cancel() PURE;
 };
 
-/**
- * Reason that a pool stream could not be obtained.
- */
-enum class PoolFailureReason {
-  // A resource overflowed and policy prevented a new stream from being created.
-  Overflow,
-  // A connection failure took place and the stream could not be bound.
-  ConnectionFailure
-};
+using PoolFailureReason = ::Envoy::ConnectionPool::PoolFailureReason;
 
 /**
  * Pool callbacks invoked in the context of a newStream() call, either synchronously or

diff --git a/include/envoy/tcp/BUILD b/include/envoy/tcp/BUILD
@@ -13,6 +13,7 @@ envoy_cc_library(
     hdrs = ["conn_pool.h"],
     deps = [
         "//include/envoy/buffer:buffer_interface",
+        "//include/envoy/common:conn_pool_interface",
         "//include/envoy/event:deferred_deletable",
         "//include/envoy/upstream:upstream_interface",
     ],

diff --git a/include/envoy/tcp/conn_pool.h b/include/envoy/tcp/conn_pool.h
@@ -4,6 +4,7 @@
 #include <memory>
 
 #include "envoy/buffer/buffer.h"
+#include "envoy/common/conn_pool.h"
 #include "envoy/common/pure.h"
 #include "envoy/event/deferred_deletable.h"
 #include "envoy/upstream/upstream.h"
@@ -42,20 +43,6 @@ class Cancellable {
   virtual void cancel(CancelPolicy cancel_policy) PURE;
 };
 
-/**
- * Reason that a pool connection could not be obtained.
- */
-enum class PoolFailureReason {
-  // A resource overflowed and policy prevented a new connection from being created.
-  Overflow,
-  // A local connection failure took place while creating a new connection.
-  LocalConnectionFailure,
-  // A remote connection failure took place while creating a new connection.
-  RemoteConnectionFailure,
-  // A timeout occurred while creating a new connection.
-  Timeout,
-};
-
 /*
  * UpstreamCallbacks for connection pool upstream connection callbacks and data. Note that
  * onEvent(Connected) is never triggered since the event always occurs before a ConnectionPool
@@ -131,6 +118,7 @@ class ConnectionData {
 };
 
 using ConnectionDataPtr = std::unique_ptr<ConnectionData>;
+using PoolFailureReason = ::Envoy::ConnectionPool::PoolFailureReason;
 
 /**
  * Pool callbacks invoked in the context of a newConnection() call, either synchronously or

diff --git a/source/common/http/conn_pool_base.cc b/source/common/http/conn_pool_base.cc
@@ -274,14 +274,23 @@ void ConnPoolImplBase::onConnectionEvent(ConnPoolImplBase::ActiveClient& client,
       host_->cluster().stats().upstream_cx_connect_fail_.inc();
       host_->stats().cx_connect_fail_.inc();
 
+      ConnectionPool::PoolFailureReason reason;
+      if (client.timed_out_) {
+        reason = ConnectionPool::PoolFailureReason::Timeout;
+      } else if (event == Network::ConnectionEvent::RemoteClose) {
+        reason = ConnectionPool::PoolFailureReason::RemoteConnectionFailure;
+      } else {
+        reason = ConnectionPool::PoolFailureReason::LocalConnectionFailure;
+      }
+
       // Raw connect failures should never happen under normal circumstances. If we have an upstream
       // that is behaving badly, requests can get stuck here in the pending state. If we see a
       // connect failure, we purge all pending requests so that calling code can determine what to
       // do with the request.
       // NOTE: We move the existing pending requests to a temporary list. This is done so that
       //       if retry logic submits a new request to the pool, we don't fail it inline.
       purgePendingRequests(client.real_host_description_,
-                           client.codec_client_->connectionFailureReason());
+                           client.codec_client_->connectionFailureReason(), reason);
     }
 
     // We need to release our resourceManager() resources before checking below for
@@ -342,16 +351,15 @@ ConnPoolImplBase::newPendingRequest(ResponseDecoder& decoder,
 
 void ConnPoolImplBase::purgePendingRequests(
     const Upstream::HostDescriptionConstSharedPtr& host_description,
-    absl::string_view failure_reason) {
+    absl::string_view failure_reason, ConnectionPool::PoolFailureReason reason) {
   // NOTE: We move the existing pending requests to a temporary list. This is done so that
   //       if retry logic submits a new request to the pool, we don't fail it inline.
   pending_requests_to_purge_ = std::move(pending_requests_);
   while (!pending_requests_to_purge_.empty()) {
     PendingRequestPtr request =
         pending_requests_to_purge_.front()->removeFromList(pending_requests_to_purge_);
     host_->cluster().stats().upstream_rq_pending_failure_eject_.inc();
-    request->callbacks_.onPoolFailure(ConnectionPool::PoolFailureReason::ConnectionFailure,
-                                      failure_reason, host_description);
+    request->callbacks_.onPoolFailure(reason, failure_reason, host_description);
   }
 }
 
@@ -428,6 +436,7 @@ void ConnPoolImplBase::ActiveClient::releaseResources() {
 void ConnPoolImplBase::ActiveClient::onConnectTimeout() {
   ENVOY_CONN_LOG(debug, "connect timeout", *codec_client_);
   parent_.host_->cluster().stats().upstream_cx_connect_timeout_.inc();
+  timed_out_ = true;
   close();
 }
 

diff --git a/source/common/http/conn_pool_base.h b/source/common/http/conn_pool_base.h
@@ -89,6 +89,7 @@ class ConnPoolImplBase : public ConnectionPool::Instance,
     Stats::TimespanPtr conn_length_;
     Event::TimerPtr connect_timer_;
     bool resources_released_{false};
+    bool timed_out_{false};
   };
 
   using ActiveClientPtr = std::unique_ptr<ActiveClient>;
@@ -126,7 +127,8 @@ class ConnPoolImplBase : public ConnectionPool::Instance,
 
   // Fails all pending requests, calling onPoolFailure on the associated callbacks.
   void purgePendingRequests(const Upstream::HostDescriptionConstSharedPtr& host_description,
-                            absl::string_view failure_reason);
+                            absl::string_view failure_reason,
+                            ConnectionPool::PoolFailureReason pool_failure_reason);
 
   // Closes any idle connections.
   void closeIdleConnections();

diff --git a/source/common/http/conn_pool_base_legacy.cc b/source/common/http/conn_pool_base_legacy.cc
@@ -62,16 +62,18 @@ ConnPoolImplBase::newPendingRequest(ResponseDecoder& decoder,
 
 void ConnPoolImplBase::purgePendingRequests(
     const Upstream::HostDescriptionConstSharedPtr& host_description,
-    absl::string_view failure_reason) {
+    absl::string_view failure_reason, bool was_remote_close) {
   // NOTE: We move the existing pending requests to a temporary list. This is done so that
   //       if retry logic submits a new request to the pool, we don't fail it inline.
   pending_requests_to_purge_ = std::move(pending_requests_);
   while (!pending_requests_to_purge_.empty()) {
     PendingRequestPtr request =
         pending_requests_to_purge_.front()->removeFromList(pending_requests_to_purge_);
     host_->cluster().stats().upstream_rq_pending_failure_eject_.inc();
-    request->callbacks_.onPoolFailure(ConnectionPool::PoolFailureReason::ConnectionFailure,
-                                      failure_reason, host_description);
+    request->callbacks_.onPoolFailure(
+        was_remote_close ? ConnectionPool::PoolFailureReason::RemoteConnectionFailure
+                         : ConnectionPool::PoolFailureReason::LocalConnectionFailure,
+        failure_reason, host_description);
   }
 }
 

diff --git a/source/common/http/conn_pool_base_legacy.h b/source/common/http/conn_pool_base_legacy.h
@@ -63,7 +63,7 @@ class ConnPoolImplBase : protected Logger::Loggable<Logger::Id::pool> {
 
   // Fails all pending requests, calling onPoolFailure on the associated callbacks.
   void purgePendingRequests(const Upstream::HostDescriptionConstSharedPtr& host_description,
-                            absl::string_view failure_reason);
+                            absl::string_view failure_reason, bool was_remote);
 
   // Must be implemented by sub class. Attempts to drain inactive clients.
   virtual void checkForDrained() PURE;

diff --git a/source/common/http/http1/conn_pool_legacy.cc b/source/common/http/http1/conn_pool_legacy.cc
@@ -163,7 +163,8 @@ void ConnPoolImpl::onConnectionEvent(ActiveClient& client, Network::ConnectionEv
       ENVOY_CONN_LOG(debug, "purge pending, failure reason: {}", *client.codec_client_,
                      client.codec_client_->connectionFailureReason());
       purgePendingRequests(client.real_host_description_,
-                           client.codec_client_->connectionFailureReason());
+                           client.codec_client_->connectionFailureReason(),
+                           event == Network::ConnectionEvent::RemoteClose);
     }
 
     dispatcher_.deferredDelete(std::move(removed));

diff --git a/source/common/http/http2/conn_pool_legacy.cc b/source/common/http/http2/conn_pool_legacy.cc
@@ -166,8 +166,8 @@ void ConnPoolImpl::onConnectionEvent(ActiveClient& client, Network::ConnectionEv
       // do with the request.
       // NOTE: We move the existing pending requests to a temporary list. This is done so that
       //       if retry logic submits a new request to the pool, we don't fail it inline.
-      purgePendingRequests(client.real_host_description_,
-                           client.client_->connectionFailureReason());
+      purgePendingRequests(client.real_host_description_, client.client_->connectionFailureReason(),
+                           event == Network::ConnectionEvent::RemoteClose);
     }
 
     if (&client == primary_client_.get()) {

diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc
@@ -289,17 +289,21 @@ void UpstreamRequest::onPerTryTimeout() {
   }
 }
 
-void UpstreamRequest::onPoolFailure(Http::ConnectionPool::PoolFailureReason reason,
+void UpstreamRequest::onPoolFailure(ConnectionPool::PoolFailureReason reason,
                                     absl::string_view transport_failure_reason,
                                     Upstream::HostDescriptionConstSharedPtr host) {
   Http::StreamResetReason reset_reason = Http::StreamResetReason::ConnectionFailure;
   switch (reason) {
-  case Http::ConnectionPool::PoolFailureReason::Overflow:
+  case ConnectionPool::PoolFailureReason::Overflow:
     reset_reason = Http::StreamResetReason::Overflow;
     break;
-  case Http::ConnectionPool::PoolFailureReason::ConnectionFailure:
+  case ConnectionPool::PoolFailureReason::RemoteConnectionFailure:
+    FALLTHRU;
+  case ConnectionPool::PoolFailureReason::LocalConnectionFailure:
     reset_reason = Http::StreamResetReason::ConnectionFailure;
     break;
+  case ConnectionPool::PoolFailureReason::Timeout:
+    reset_reason = Http::StreamResetReason::LocalReset;
   }
 
   // Mimic an upstream reset.
@@ -487,7 +491,7 @@ bool HttpConnPool::cancelAnyPendingRequest() {
 
 absl::optional<Http::Protocol> HttpConnPool::protocol() const { return conn_pool_.protocol(); }
 
-void HttpConnPool::onPoolFailure(Http::ConnectionPool::PoolFailureReason reason,
+void HttpConnPool::onPoolFailure(ConnectionPool::PoolFailureReason reason,
                                  absl::string_view transport_failure_reason,
                                  Upstream::HostDescriptionConstSharedPtr host) {
   callbacks_->onPoolFailure(reason, transport_failure_reason, host);

diff --git a/source/common/router/upstream_request.h b/source/common/router/upstream_request.h
@@ -52,7 +52,7 @@ class GenericConnectionPoolCallbacks {
 public:
   virtual ~GenericConnectionPoolCallbacks() = default;
 
-  virtual void onPoolFailure(Http::ConnectionPool::PoolFailureReason reason,
+  virtual void onPoolFailure(ConnectionPool::PoolFailureReason reason,
                              absl::string_view transport_failure_reason,
                              Upstream::HostDescriptionConstSharedPtr host) PURE;
   virtual void onPoolReady(std::unique_ptr<GenericUpstream>&& upstream,
@@ -97,7 +97,7 @@ class UpstreamRequest : public Logger::Loggable<Logger::Id::router>,
   void enableDataFromDownstreamForFlowControl();
 
   // GenericConnPool
-  void onPoolFailure(Http::ConnectionPool::PoolFailureReason reason,
+  void onPoolFailure(ConnectionPool::PoolFailureReason reason,
                      absl::string_view transport_failure_reason,
                      Upstream::HostDescriptionConstSharedPtr host) override;
   void onPoolReady(std::unique_ptr<GenericUpstream>&& upstream,
@@ -186,7 +186,7 @@ class HttpConnPool : public GenericConnPool, public Http::ConnectionPool::Callba
   absl::optional<Http::Protocol> protocol() const override;
 
   // Http::ConnectionPool::Callbacks
-  void onPoolFailure(Http::ConnectionPool::PoolFailureReason reason,
+  void onPoolFailure(ConnectionPool::PoolFailureReason reason,
                      absl::string_view transport_failure_reason,
                      Upstream::HostDescriptionConstSharedPtr host) override;
   void onPoolReady(Http::RequestEncoder& callbacks_encoder,

diff --git a/source/common/tcp_proxy/tcp_proxy.cc b/source/common/tcp_proxy/tcp_proxy.cc
@@ -28,21 +28,6 @@
 
 namespace Envoy {
 namespace TcpProxy {
-namespace {
-
-Tcp::ConnectionPool::PoolFailureReason
-httpToTcpFailure(Http::ConnectionPool::PoolFailureReason reason) {
-  switch (reason) {
-  case Http::ConnectionPool::PoolFailureReason::Overflow:
-    return Tcp::ConnectionPool::PoolFailureReason::Overflow;
-  case Http::ConnectionPool::PoolFailureReason::ConnectionFailure:
-    // TODO(alyssawilk) It's unclear which this is.
-    return Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure;
-  }
-  return Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure;
-}
-
-} // namespace
 
 const std::string& PerConnectionCluster::key() {
   CONSTRUCT_ON_FIRST_USE(std::string, "envoy.tcp_proxy.cluster");
@@ -464,24 +449,24 @@ Network::FilterStatus Filter::initializeUpstreamConnection() {
   return Network::FilterStatus::StopIteration;
 }
 
-void Filter::onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason,
+void Filter::onPoolFailure(ConnectionPool::PoolFailureReason reason,
                            Upstream::HostDescriptionConstSharedPtr host) {
   upstream_handle_.reset();
 
   read_callbacks_->upstreamHost(host);
   getStreamInfo().onUpstreamHostSelected(host);
 
   switch (reason) {
-  case Tcp::ConnectionPool::PoolFailureReason::Overflow:
-  case Tcp::ConnectionPool::PoolFailureReason::LocalConnectionFailure:
+  case ConnectionPool::PoolFailureReason::Overflow:
+  case ConnectionPool::PoolFailureReason::LocalConnectionFailure:
     upstream_callbacks_->onEvent(Network::ConnectionEvent::LocalClose);
     break;
 
-  case Tcp::ConnectionPool::PoolFailureReason::RemoteConnectionFailure:
+  case ConnectionPool::PoolFailureReason::RemoteConnectionFailure:
     upstream_callbacks_->onEvent(Network::ConnectionEvent::RemoteClose);
     break;
 
-  case Tcp::ConnectionPool::PoolFailureReason::Timeout:
+  case ConnectionPool::PoolFailureReason::Timeout:
     onConnectTimeout();
     break;
 
@@ -514,9 +499,9 @@ void Filter::onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data,
       latched_data->connection().streamInfo().filterState());
 }
 
-void Filter::onPoolFailure(Http::ConnectionPool::PoolFailureReason failure, absl::string_view,
+void Filter::onPoolFailure(ConnectionPool::PoolFailureReason failure, absl::string_view,
                            Upstream::HostDescriptionConstSharedPtr host) {
-  onPoolFailure(httpToTcpFailure(failure), host);
+  onPoolFailure(failure, host);
 }
 
 void Filter::onPoolReady(Http::RequestEncoder& request_encoder,

diff --git a/source/common/tcp_proxy/tcp_proxy.h b/source/common/tcp_proxy/tcp_proxy.h
@@ -245,13 +245,13 @@ class Filter : public Network::ReadFilter,
   void initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callbacks) override;
 
   // Tcp::ConnectionPool::Callbacks
-  void onPoolFailure(Tcp::ConnectionPool::PoolFailureReason reason,
+  void onPoolFailure(ConnectionPool::PoolFailureReason reason,
                      Upstream::HostDescriptionConstSharedPtr host) override;
   void onPoolReady(Tcp::ConnectionPool::ConnectionDataPtr&& conn_data,
                    Upstream::HostDescriptionConstSharedPtr host) override;
 
   // Http::ConnectionPool::Callbacks,
-  void onPoolFailure(Http::ConnectionPool::PoolFailureReason reason,
+  void onPoolFailure(ConnectionPool::PoolFailureReason reason,
                      absl::string_view transport_failure_reason,
                      Upstream::HostDescriptionConstSharedPtr host) override;
   void onPoolReady(Http::RequestEncoder& request_encoder,