backport 1.16: http: reinstating prior connect timeout behavior (#15233)

#10854 inadvertently changed the behavior of connect timeouts. This reinstates prior behavior.

Risk Level: Low (reinstating prior behavior)
Testing: added regression test
Docs Changes: n/a
Release Notes: inline

Signed-off-by: Shikugawa <rei@tetrate.io>
Co-authored-by: alyssawilk <alyssar@chromium.org>

docs/root/version_history/current.rst

@@ -14,6 +14,8 @@ Bug Fixes
 *Changes expected to improve the state of the world and are unlikely to have negative effects*
 
 * aggregate cluster: fixed a crash due to a TLS initialization issue.
+* http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false.
+
 * lua: fixed crash when Lua script contains streamInfo():downstreamSslConnection().
 * overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration.
 * tls: fix detection of the upstream connection close event.

source/common/router/upstream_request.cc

@@ -340,7 +340,12 @@ void UpstreamRequest::onPoolFailure(ConnectionPool::PoolFailureReason reason,
  reset_reason = Http::StreamResetReason::ConnectionFailure;
  break;
  case ConnectionPool::PoolFailureReason::Timeout:
- reset_reason = Http::StreamResetReason::LocalReset;
+ if (Runtime::runtimeFeatureEnabled(
+ "envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure")) {
+ reset_reason = Http::StreamResetReason::ConnectionFailure;
+ } else {
+ reset_reason = Http::StreamResetReason::LocalReset;
+ }
  }
 
  // Mimic an upstream reset.

source/common/runtime/runtime_features.cc

@@ -89,6 +89,7 @@ constexpr const char* runtime_features[] = {
  "envoy.reloadable_features.stop_faking_paths",
  "envoy.reloadable_features.strict_1xx_and_204_response_headers",
  "envoy.reloadable_features.tls_use_io_handle_bio",
+ "envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure",
  "envoy.reloadable_features.unify_grpc_handling",
  "envoy.restart_features.use_apple_api_for_dns_lookups",
 };

test/common/router/router_test.cc

@@ -510,6 +510,76 @@ TEST_F(RouterTest, PoolFailureWithPriority) {
  "upstream_reset_before_response_started{connection failure,tls version mismatch}");
 }
 
+TEST_F(RouterTest, PoolFailureDueToConnectTimeout) {
+ ON_CALL(callbacks_.route_->route_entry_, priority())
+ .WillByDefault(Return(Upstream::ResourcePriority::High));
+ EXPECT_CALL(cm_, httpConnPoolForCluster(_, Upstream::ResourcePriority::High, _, &router_));
+ EXPECT_CALL(cm_.conn_pool_, newStream(_, _))
+ .WillOnce(Invoke([&](Http::StreamDecoder&, Http::ConnectionPool::Callbacks& callbacks)
+ -> Http::ConnectionPool::Cancellable* {
+ callbacks.onPoolFailure(ConnectionPool::PoolFailureReason::Timeout, "connect_timeout",
+ cm_.conn_pool_.host_);
+ return nullptr;
+ }));
+
+ Http::TestResponseHeaderMapImpl response_headers{
+ {":status", "503"}, {"content-length", "134"}, {"content-type", "text/plain"}};
+ EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false));
+ EXPECT_CALL(callbacks_, encodeData(_, true));
+ EXPECT_CALL(callbacks_.stream_info_,
+ setResponseFlag(StreamInfo::ResponseFlag::UpstreamConnectionFailure));
+ EXPECT_CALL(callbacks_.stream_info_, onUpstreamHostSelected(_))
+ .WillOnce(Invoke([&](const Upstream::HostDescriptionConstSharedPtr host) -> void {
+ EXPECT_EQ(host_address_, host->address());
+ }));
+
+ Http::TestRequestHeaderMapImpl headers;
+ HttpTestUtility::addDefaultHeaders(headers);
+ router_.decodeHeaders(headers, true);
+ EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
+ // Pool failure, so upstream request was not initiated.
+ EXPECT_EQ(0U,
+ callbacks_.route_->route_entry_.virtual_cluster_.stats().upstream_rq_total_.value());
+ EXPECT_EQ(callbacks_.details(),
+ "upstream_reset_before_response_started{connection failure,connect_timeout}");
+}
+
+TEST_F(RouterTest, PoolFailureDueToConnectTimeoutLegacy) {
+ TestScopedRuntime scoped_runtime;
+ Runtime::LoaderSingleton::getExisting()->mergeValues(
+ {{"envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure", "false"}});
+ ON_CALL(callbacks_.route_->route_entry_, priority())
+ .WillByDefault(Return(Upstream::ResourcePriority::High));
+ EXPECT_CALL(cm_, httpConnPoolForCluster(_, Upstream::ResourcePriority::High, _, &router_));
+ EXPECT_CALL(cm_.conn_pool_, newStream(_, _))
+ .WillOnce(Invoke([&](Http::StreamDecoder&, Http::ConnectionPool::Callbacks& callbacks)
+ -> Http::ConnectionPool::Cancellable* {
+ callbacks.onPoolFailure(ConnectionPool::PoolFailureReason::Timeout, "connect_timeout",
+ cm_.conn_pool_.host_);
+ return nullptr;
+ }));
+
+ Http::TestResponseHeaderMapImpl response_headers{
+ {":status", "503"}, {"content-length", "127"}, {"content-type", "text/plain"}};
+ EXPECT_CALL(callbacks_, encodeHeaders_(HeaderMapEqualRef(&response_headers), false));
+ EXPECT_CALL(callbacks_, encodeData(_, true));
+ EXPECT_CALL(callbacks_.stream_info_, setResponseFlag(StreamInfo::ResponseFlag::LocalReset));
+ EXPECT_CALL(callbacks_.stream_info_, onUpstreamHostSelected(_))
+ .WillOnce(Invoke([&](const Upstream::HostDescriptionConstSharedPtr host) -> void {
+ EXPECT_EQ(host_address_, host->address());
+ }));
+
+ Http::TestRequestHeaderMapImpl headers;
+ HttpTestUtility::addDefaultHeaders(headers);
+ router_.decodeHeaders(headers, true);
+ EXPECT_TRUE(verifyHostUpstreamStats(0, 1));
+ // Pool failure, so upstream request was not initiated.
+ EXPECT_EQ(0U,
+ callbacks_.route_->route_entry_.virtual_cluster_.stats().upstream_rq_total_.value());
+ EXPECT_EQ(callbacks_.details(),
+ "upstream_reset_before_response_started{local reset,connect_timeout}");
+}
+
 TEST_F(RouterTest, Http1Upstream) {
  EXPECT_CALL(cm_, httpConnPoolForCluster(_, _, absl::optional<Http::Protocol>(), _));
  EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_));