diff --git a/api/envoy/config/filter/http/router/v2/router.proto b/api/envoy/config/filter/http/router/v2/router.proto index 7543069af029..6fe6093a26b6 100644 --- a/api/envoy/config/filter/http/router/v2/router.proto +++ b/api/envoy/config/filter/http/router/v2/router.proto @@ -65,4 +65,9 @@ message Router { } } }]; + + // If not set, ingress Envoy will ignore + // :ref:`config_http_filters_router_x-envoy-expected-rq-timeout-ms` header, populated by egress + // Envoy, when deriving timeout for upstream cluster. + bool respect_expected_rq_timeout = 6; } diff --git a/api/envoy/config/filter/http/router/v3alpha/router.proto b/api/envoy/config/filter/http/router/v3alpha/router.proto index 168bbbd8f2b1..b2f109238a50 100644 --- a/api/envoy/config/filter/http/router/v3alpha/router.proto +++ b/api/envoy/config/filter/http/router/v3alpha/router.proto @@ -65,4 +65,9 @@ message Router { } } }]; + + // If not set, ingress Envoy will ignore + // :ref:`config_http_filters_router_x-envoy-expected-rq-timeout-ms` header, populated by egress + // Envoy, when deriving timeout for upstream cluster. + bool respect_expected_rq_timeout = 6; } diff --git a/docs/root/intro/version_history.rst b/docs/root/intro/version_history.rst index 0592bc78c1aa..e1de8d60e216 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -59,6 +59,7 @@ Version history * router: added :ref:`rq_retry_skipped_request_not_complete ` counter stat to router stats. * router: :ref:`Scoped routing ` is supported. * router: added new :ref:`retriable-headers ` retry policy. Retries can now be configured to trigger by arbitrary response header matching. +* router: added :ref:`respect_expected_rq_timeout ` that instructs ingress Envoy to respect :ref:`config_http_filters_router_x-envoy-expected-rq-timeout-ms` header, populated by egress Envoy, when deriving timeout for upstream cluster. * router: added new :ref:`retriable request headers ` to retry policies. Retries can now be configured to only trigger on request header match. * router check tool: add coverage reporting & enforcement. * router check tool: add comprehensive coverage reporting. diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index e69f02ef0f66..775a48edd627 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -37,7 +37,7 @@ AsyncClientImpl::AsyncClientImpl(Upstream::ClusterInfoConstSharedPtr cluster, Router::ShadowWriterPtr&& shadow_writer, Http::Context& http_context) : cluster_(cluster), config_("http.async-client.", local_info, stats_store, cm, runtime, random, - std::move(shadow_writer), true, false, false, {}, + std::move(shadow_writer), true, false, false, false, {}, dispatcher.timeSource(), http_context), dispatcher_(dispatcher) {} diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 83e59e15d7db..db25e4f0a799 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -123,7 +123,8 @@ bool FilterUtility::shouldShadow(const ShadowPolicy& policy, Runtime::Loader& ru FilterUtility::TimeoutData FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_headers, bool insert_envoy_expected_request_timeout_ms, bool grpc_request, - bool per_try_timeout_hedging_enabled) { + bool per_try_timeout_hedging_enabled, + bool respect_expected_rq_timeout) { // See if there is a user supplied timeout in a request header. If there is we take that. // Otherwise if the request is gRPC and a maximum gRPC timeout is configured we use the timeout // in the gRPC headers (or infinity when gRPC headers have no timeout), but cap that timeout to @@ -153,13 +154,29 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he } timeout.per_try_timeout_ = route.retryPolicy().perTryTimeout(); - Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); uint64_t header_timeout; - if (header_timeout_entry) { - if (absl::SimpleAtoi(header_timeout_entry->value().getStringView(), &header_timeout)) { - timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); + + if (respect_expected_rq_timeout) { + // Check if there is timeout set by egress Envoy. + // If present, use that value as route timeout and don't override + // *x-envoy-expected-rq-timeout-ms* header. At this point *x-envoy-upstream-rq-timeout-ms* + // header should have been sanitized by egress Envoy. + Http::HeaderEntry* header_expected_timeout_entry = + request_headers.EnvoyExpectedRequestTimeoutMs(); + if (header_expected_timeout_entry) { + trySetGlobalTimeout(header_expected_timeout_entry, timeout); + } else { + Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); + + if (trySetGlobalTimeout(header_timeout_entry, timeout)) { + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); + } + } + } else { + Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); + if (trySetGlobalTimeout(header_timeout_entry, timeout)) { + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); } - request_headers.removeEnvoyUpstreamRequestTimeoutMs(); } // See if there is a per try/retry timeout. If it's >= global we just ignore it. @@ -200,6 +217,18 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he return timeout; } +bool FilterUtility::trySetGlobalTimeout(const Http::HeaderEntry* header_timeout_entry, + TimeoutData& timeout) { + if (header_timeout_entry) { + uint64_t header_timeout; + if (absl::SimpleAtoi(header_timeout_entry->value().getStringView(), &header_timeout)) { + timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); + } + return true; + } + return false; +} + FilterUtility::HedgingParams FilterUtility::finalHedgingParams(const RouteEntry& route, Http::HeaderMap& request_headers) { HedgingParams hedging_params; @@ -486,7 +515,8 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::HeaderMap& headers, bool e hedging_params_ = FilterUtility::finalHedgingParams(*route_entry_, headers); timeout_ = FilterUtility::finalTimeout(*route_entry_, headers, !config_.suppress_envoy_headers_, - grpc_request_, hedging_params_.hedge_on_per_try_timeout_); + grpc_request_, hedging_params_.hedge_on_per_try_timeout_, + config_.respect_expected_rq_timeout_); // If this header is set with any value, use an alternate response code on timeout if (headers.EnvoyUpstreamRequestTimeoutAltResponse()) { diff --git a/source/common/router/router.h b/source/common/router/router.h index 419d58fb0ccd..4dfeb2f7ae8f 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -140,7 +140,11 @@ class FilterUtility { */ static TimeoutData finalTimeout(const RouteEntry& route, Http::HeaderMap& request_headers, bool insert_envoy_expected_request_timeout_ms, bool grpc_request, - bool per_try_timeout_hedging_enabled); + bool per_try_timeout_hedging_enabled, + bool respect_expected_rq_timeout); + + static bool trySetGlobalTimeout(const Http::HeaderEntry* header_timeout_entry, + TimeoutData& timeout); /** * Determine the final hedging settings after applying randomized behavior. @@ -161,12 +165,14 @@ class FilterConfig { Stats::Scope& scope, Upstream::ClusterManager& cm, Runtime::Loader& runtime, Runtime::RandomGenerator& random, ShadowWriterPtr&& shadow_writer, bool emit_dynamic_stats, bool start_child_span, bool suppress_envoy_headers, + bool respect_expected_rq_timeout, const Protobuf::RepeatedPtrField& strict_check_headers, TimeSource& time_source, Http::Context& http_context) : scope_(scope), local_info_(local_info), cm_(cm), runtime_(runtime), random_(random), stats_{ALL_ROUTER_STATS(POOL_COUNTER_PREFIX(scope, stat_prefix))}, emit_dynamic_stats_(emit_dynamic_stats), start_child_span_(start_child_span), - suppress_envoy_headers_(suppress_envoy_headers), http_context_(http_context), + suppress_envoy_headers_(suppress_envoy_headers), + respect_expected_rq_timeout_(respect_expected_rq_timeout), http_context_(http_context), stat_name_pool_(scope_.symbolTable()), retry_(stat_name_pool_.add("retry")), zone_name_(stat_name_pool_.add(local_info_.zoneName())), empty_stat_name_(stat_name_pool_.add("")), shadow_writer_(std::move(shadow_writer)), @@ -186,8 +192,8 @@ class FilterConfig { context.runtime(), context.random(), std::move(shadow_writer), PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, dynamic_stats, true), config.start_child_span(), config.suppress_envoy_headers(), - config.strict_check_headers(), context.api().timeSource(), - context.httpContext()) { + config.respect_expected_rq_timeout(), config.strict_check_headers(), + context.api().timeSource(), context.httpContext()) { for (const auto& upstream_log : config.upstream_log()) { upstream_logs_.push_back(AccessLog::AccessLogFactory::fromProto(upstream_log, context)); } @@ -207,6 +213,7 @@ class FilterConfig { const bool emit_dynamic_stats_; const bool start_child_span_; const bool suppress_envoy_headers_; + const bool respect_expected_rq_timeout_; // TODO(xyu-stripe): Make this a bitset to keep cluster memory footprint down. HeaderVectorPtr strict_check_headers_; std::list upstream_logs_; diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index becdedc624db..b3ebd37349be 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -86,7 +86,7 @@ class RouterTestBase : public testing::Test { : http_context_(stats_store_.symbolTable()), shadow_writer_(new MockShadowWriter()), config_("test.", local_info_, stats_store_, cm_, runtime_, random_, ShadowWriterPtr{shadow_writer_}, true, start_child_span, suppress_envoy_headers, - std::move(strict_headers_to_check), test_time_.timeSystem(), http_context_), + false, std::move(strict_headers_to_check), test_time_.timeSystem(), http_context_), router_(config_) { router_.setDecoderFilterCallbacks(callbacks_); upstream_locality_.set_zone("to_az"); @@ -3704,7 +3704,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); Http::TestHeaderMapImpl headers; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, false); + FilterUtility::finalTimeout(route, headers, true, false, false, false); EXPECT_EQ(std::chrono::milliseconds(10), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); } @@ -3713,7 +3713,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, false); + FilterUtility::finalTimeout(route, headers, true, false, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3725,7 +3725,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "bad"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, false); + FilterUtility::finalTimeout(route, headers, true, false, false, false); EXPECT_EQ(std::chrono::milliseconds(10), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3738,7 +3738,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}, {"x-envoy-upstream-rq-per-try-timeout-ms", "15"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, false); + FilterUtility::finalTimeout(route, headers, true, false, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3752,7 +3752,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}, {"x-envoy-upstream-rq-per-try-timeout-ms", "5"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, false); + FilterUtility::finalTimeout(route, headers, true, false, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(5), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3766,7 +3766,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}, {"x-envoy-upstream-rq-per-try-timeout-ms", "5"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, true); + FilterUtility::finalTimeout(route, headers, true, false, true, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(5), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3781,7 +3781,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}, {"x-envoy-upstream-rq-per-try-timeout-ms", "5"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, true); + FilterUtility::finalTimeout(route, headers, true, true, true, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(5), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3795,7 +3795,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, false); + FilterUtility::finalTimeout(route, headers, true, false, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(7), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3810,7 +3810,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}, {"x-envoy-upstream-rq-per-try-timeout-ms", "5"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, false); + FilterUtility::finalTimeout(route, headers, true, false, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(5), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3824,7 +3824,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { .WillRepeatedly(Return(absl::optional(0))); Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(0), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("grpc-timeout")); @@ -3835,7 +3835,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(10), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("grpc-timeout")); @@ -3847,7 +3847,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}, {"grpc-timeout", "1000m"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(1000), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_EQ("1000m", headers.get_("grpc-timeout")); @@ -3859,7 +3859,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}, {"grpc-timeout", "1000m"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(999), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_EQ("999m", headers.get_("grpc-timeout")); @@ -3870,7 +3870,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { .WillRepeatedly(Return(absl::optional(999))); Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}, {"grpc-timeout", "0m"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(999), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_EQ("999m", headers.get_("grpc-timeout")); @@ -3883,7 +3883,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { .WillRepeatedly(Return(absl::optional(10))); Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}, {"grpc-timeout", "100m"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(90), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); } @@ -3895,7 +3895,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { .WillRepeatedly(Return(absl::optional(10))); Http::TestHeaderMapImpl headers{{"content-type", "application/grpc"}, {"grpc-timeout", "1m"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(1), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); } @@ -3907,7 +3907,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { {"grpc-timeout", "1000m"}, {"x-envoy-upstream-rq-timeout-ms", "15"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3922,7 +3922,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { {"grpc-timeout", "1000m"}, {"x-envoy-upstream-rq-timeout-ms", "bad"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(1000), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3938,7 +3938,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { {"x-envoy-upstream-rq-timeout-ms", "15"}, {"x-envoy-upstream-rq-per-try-timeout-ms", "15"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3955,7 +3955,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { {"x-envoy-upstream-rq-timeout-ms", "15"}, {"x-envoy-upstream-rq-per-try-timeout-ms", "5"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(5), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3972,7 +3972,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { {"grpc-timeout", "1000m"}, {"x-envoy-upstream-rq-timeout-ms", "15"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(7), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3990,7 +3990,7 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { {"x-envoy-upstream-rq-timeout-ms", "15"}, {"x-envoy-upstream-rq-per-try-timeout-ms", "5"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, true, false); + FilterUtility::finalTimeout(route, headers, true, true, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(5), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); @@ -3998,6 +3998,65 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { EXPECT_EQ("5", headers.get_("x-envoy-expected-rq-timeout-ms")); EXPECT_EQ("5m", headers.get_("grpc-timeout")); } + { + NiceMock route; + EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); + Http::TestHeaderMapImpl headers{{"x-envoy-expected-rq-timeout-ms", "8"}}; + // Make ingress envoy respect `x-envoy-expected-rq-timeout-ms` header. + bool respect_expected_rq_timeout = true; + FilterUtility::TimeoutData timeout = FilterUtility::finalTimeout( + route, headers, true, false, false, respect_expected_rq_timeout); + EXPECT_EQ(std::chrono::milliseconds(8), timeout.global_timeout_); + EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); + EXPECT_EQ("8", headers.get_("x-envoy-expected-rq-timeout-ms")); + EXPECT_FALSE(headers.has("grpc-timeout")); + } + { + NiceMock route; + EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); + Http::TestHeaderMapImpl headers{{"x-envoy-expected-rq-timeout-ms", "8"}, + {"x-envoy-upstream-rq-per-try-timeout-ms", "4"}}; + // Make ingress envoy respect `x-envoy-expected-rq-timeout-ms` header. + bool respect_expected_rq_timeout = true; + FilterUtility::TimeoutData timeout = FilterUtility::finalTimeout( + route, headers, true, false, false, respect_expected_rq_timeout); + EXPECT_EQ(std::chrono::milliseconds(8), timeout.global_timeout_); + EXPECT_EQ(std::chrono::milliseconds(4), timeout.per_try_timeout_); + EXPECT_EQ("4", headers.get_("x-envoy-expected-rq-timeout-ms")); + EXPECT_FALSE(headers.has("grpc-timeout")); + } + { + NiceMock route; + EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); + Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "8"}}; + // Test that ingress envoy populates `x-envoy-expected-rq-timeout-ms` header if it has not been + // set by egress envoy. + bool respect_expected_rq_timeout = true; + FilterUtility::TimeoutData timeout = FilterUtility::finalTimeout( + route, headers, true, false, false, respect_expected_rq_timeout); + EXPECT_EQ(std::chrono::milliseconds(8), timeout.global_timeout_); + EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); + EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); + EXPECT_FALSE(headers.has("x-envoy-upstream-rq-per-try-timeout-ms")); + EXPECT_EQ("8", headers.get_("x-envoy-expected-rq-timeout-ms")); + EXPECT_FALSE(headers.has("grpc-timeout")); + } + { + NiceMock route; + EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); + Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "8"}}; + // Make envoy override `x-envoy-expected-rq-timeout-ms` header. + // Test that ingress envoy sets `x-envoy-expected-rq-timeout-ms` header. + bool respect_expected_rq_timeout = false; + FilterUtility::TimeoutData timeout = FilterUtility::finalTimeout( + route, headers, true, false, false, respect_expected_rq_timeout); + EXPECT_EQ(std::chrono::milliseconds(8), timeout.global_timeout_); + EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); + EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); + EXPECT_FALSE(headers.has("x-envoy-upstream-rq-per-try-timeout-ms")); + EXPECT_EQ("8", headers.get_("x-envoy-expected-rq-timeout-ms")); + EXPECT_FALSE(headers.has("grpc-timeout")); + } } TEST(RouterFilterUtilityTest, FinalTimeoutSupressEnvoyHeaders) { @@ -4006,7 +4065,7 @@ TEST(RouterFilterUtilityTest, FinalTimeoutSupressEnvoyHeaders) { EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}}; FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, false); + FilterUtility::finalTimeout(route, headers, true, false, false, false); EXPECT_EQ(std::chrono::milliseconds(15), timeout.global_timeout_); EXPECT_EQ(std::chrono::milliseconds(0), timeout.per_try_timeout_); EXPECT_FALSE(headers.has("x-envoy-upstream-rq-timeout-ms")); diff --git a/test/integration/http_timeout_integration_test.cc b/test/integration/http_timeout_integration_test.cc index 12e980b9e309..097c085eb114 100644 --- a/test/integration/http_timeout_integration_test.cc +++ b/test/integration/http_timeout_integration_test.cc @@ -47,6 +47,130 @@ TEST_P(HttpTimeoutIntegrationTest, GlobalTimeout) { EXPECT_EQ("504", response->headers().Status()->value().getStringView()); } +// Testing that `x-envoy-expected-timeout-ms` header, set by egress envoy, is respected by ingress +// envoy when `respect_expected_rq_timeout` field is enabled. Sends a request with a global timeout +// specified, sleeps for longer than the timeout, and ensures that a timeout is received. +TEST_P(HttpTimeoutIntegrationTest, UseTimeoutSetByEgressEnvoy) { + enableRespectExpectedRqTimeout(true); + initialize(); + codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); + auto encoder_decoder = codec_client_->startRequest( + Http::TestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, + {"x-envoy-upstream-rq-timeout-ms", "500"}, + {"x-envoy-expected-rq-timeout-ms", "300"}}); + auto response = std::move(encoder_decoder.second); + request_encoder_ = &encoder_decoder.first; + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + codec_client_->sendData(*request_encoder_, 0, true); + + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + // Trigger global timeout, populated from `x-envoy-expected-rq-timeout-ms` header. + timeSystem().sleep(std::chrono::milliseconds(301)); + + // Ensure we got a timeout downstream and canceled the upstream request. + response->waitForHeaders(); + ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15))); + + codec_client_->close(); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ(0U, upstream_request_->bodyLength()); + + EXPECT_TRUE(response->complete()); + EXPECT_EQ("504", response->headers().Status()->value().getStringView()); +} + +// Testing that ingress envoy derives new timeout value and sets `x-envoy-expected-timeout-ms` +// header, when timeout has not been set by egress envoy and `respect_expected_rq_timeout` field is +// enabled. Sends a request with a global timeout specified, sleeps for longer than the timeout, and +// ensures that a timeout is received. +TEST_P(HttpTimeoutIntegrationTest, DeriveTimeoutInIngressEnvoy) { + enableRespectExpectedRqTimeout(true); + initialize(); + codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); + auto encoder_decoder = codec_client_->startRequest( + Http::TestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, + {"x-envoy-upstream-rq-timeout-ms", "500"}}); + auto response = std::move(encoder_decoder.second); + request_encoder_ = &encoder_decoder.first; + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + codec_client_->sendData(*request_encoder_, 0, true); + + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + // Trigger global timeout, populated from `x-envoy-expected-rq-timeout-ms` header. + timeSystem().sleep(std::chrono::milliseconds(501)); + + // Ensure we got a timeout downstream and canceled the upstream request. + response->waitForHeaders(); + ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15))); + + codec_client_->close(); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ(0U, upstream_request_->bodyLength()); + + EXPECT_TRUE(response->complete()); + EXPECT_EQ("504", response->headers().Status()->value().getStringView()); +} + +// Testing that `x-envoy-expected-timeout-ms` header, set by egress envoy, is ignored by ingress +// envoy and new value is derived. Sends a request with a global timeout specified, +// sleeps for longer than the timeout, and ensures that a timeout is received. +TEST_P(HttpTimeoutIntegrationTest, IgnoreTimeoutSetByEgressEnvoy) { + enableRespectExpectedRqTimeout(false); + initialize(); + + codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); + auto encoder_decoder = codec_client_->startRequest( + Http::TestHeaderMapImpl{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, + {"x-envoy-upstream-rq-timeout-ms", "500"}, + {"x-envoy-expected-rq-timeout-ms", "600"}}); + auto response = std::move(encoder_decoder.second); + request_encoder_ = &encoder_decoder.first; + + ASSERT_TRUE(fake_upstreams_[0]->waitForHttpConnection(*dispatcher_, fake_upstream_connection_)); + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + codec_client_->sendData(*request_encoder_, 0, true); + + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + // Trigger global timeout, populated from `x-envoy-expected-rq-timeout-ms` header. + timeSystem().sleep(std::chrono::milliseconds(501)); + + // Ensure we got a timeout downstream and canceled the upstream request. + response->waitForHeaders(); + ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15))); + + codec_client_->close(); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_EQ(0U, upstream_request_->bodyLength()); + + EXPECT_TRUE(response->complete()); + EXPECT_EQ("504", response->headers().Status()->value().getStringView()); +} + // Regression test for https://github.com/envoyproxy/envoy/issues/7154 in which // resetStream() was only called after a response timeout for upstream requests // that had not received headers yet. This meant that decodeData might be diff --git a/test/integration/http_timeout_integration_test.h b/test/integration/http_timeout_integration_test.h index 230a82d2577a..3c9aa733da11 100644 --- a/test/integration/http_timeout_integration_test.h +++ b/test/integration/http_timeout_integration_test.h @@ -21,6 +21,24 @@ class HttpTimeoutIntegrationTest : public testing::TestWithParammutable_config()); + }); + } + + HttpIntegrationTest::initialize(); + } + + void enableRespectExpectedRqTimeout(bool enable) { respect_expected_rq_timeout = enable; } + + bool respect_expected_rq_timeout{false}; }; } // namespace Envoy