From d753c8ef017c8c8d59f001b4ec4f75b711553a0b Mon Sep 17 00:00:00 2001 From: Rei Shimizu Date: Wed, 3 Mar 2021 23:46:18 +0900 Subject: [PATCH] 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 Co-authored-by: alyssawilk --- docs/root/version_history/current.rst | 2 + source/common/router/upstream_request.cc | 7 ++- source/common/runtime/runtime_features.cc | 1 + test/common/router/router_test.cc | 70 +++++++++++++++++++++++ 4 files changed, 79 insertions(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 28bf35fa3441..f3d04e7ae2e9 100644 --- a/docs/root/version_history/current.rst +++ b/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. diff --git a/source/common/router/upstream_request.cc b/source/common/router/upstream_request.cc index 5722deb78372..4e0ffbd39275 100644 --- a/source/common/router/upstream_request.cc +++ b/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. diff --git a/source/common/runtime/runtime_features.cc b/source/common/runtime/runtime_features.cc index 56a7eb8189cb..35c1b8585a11 100644 --- a/source/common/runtime/runtime_features.cc +++ b/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", }; diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index f739a2c857ee..54365731d78f 100644 --- a/test/common/router/router_test.cc +++ b/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(), _)); EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_));