From 5c5a7067d8aa89e068378a3024dd7e91cdbd0540 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Tue, 27 Aug 2019 14:15:47 +0200 Subject: [PATCH 01/13] router: set correct timeout for egress->ingress envoys Signed-off-by: Kateryna Nezdolii --- source/common/router/router.cc | 41 +++++++++++++++++-------------- test/common/router/router_test.cc | 13 ++++++++++ 2 files changed, 35 insertions(+), 19 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 412bed0392b7..4df065a03a13 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -151,13 +151,30 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he } timeout.per_try_timeout_ = route.retryPolicy().perTryTimeout(); - Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); + // See if there is any timeout to write in the expected timeout header. + uint64_t expected_timeout = timeout.per_try_timeout_.count(); + // Use the global timeout if no per try timeout was specified or if we're + // doing hedging when there are per try timeouts. Either of these scenarios + // mean that the upstream server can use the full global timeout. + if (per_try_timeout_hedging_enabled || expected_timeout == 0) { + expected_timeout = timeout.global_timeout_.count(); + } + // todo(nezdolik) Check if order is correct, add tests. + // Check if there is timeout set by egress envoy. If present, use that instead. 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); + Http::HeaderEntry* header_expected_timeout_entry = + request_headers.EnvoyExpectedRequestTimeoutMs(); + if (!header_expected_timeout_entry) { + Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); + if (header_timeout_entry) { + if (absl::SimpleAtoi(header_timeout_entry->value().getStringView(), &header_timeout)) { + timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); + } + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); + } + if (insert_envoy_expected_request_timeout_ms && expected_timeout > 0) { + request_headers.insertEnvoyExpectedRequestTimeoutMs().value(expected_timeout); } - request_headers.removeEnvoyUpstreamRequestTimeoutMs(); } // See if there is a per try/retry timeout. If it's >= global we just ignore it. @@ -172,20 +189,6 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he if (timeout.per_try_timeout_ >= timeout.global_timeout_) { timeout.per_try_timeout_ = std::chrono::milliseconds(0); } - - // See if there is any timeout to write in the expected timeout header. - uint64_t expected_timeout = timeout.per_try_timeout_.count(); - // Use the global timeout if no per try timeout was specified or if we're - // doing hedging when there are per try timeouts. Either of these scenarios - // mean that the upstream server can use the full global timeout. - if (per_try_timeout_hedging_enabled || expected_timeout == 0) { - expected_timeout = timeout.global_timeout_.count(); - } - - if (insert_envoy_expected_request_timeout_ms && expected_timeout > 0) { - request_headers.insertEnvoyExpectedRequestTimeoutMs().value(expected_timeout); - } - // If we've configured max_grpc_timeout, override the grpc-timeout header with // the expected timeout. This ensures that the optional per try timeout is reflected // in grpc-timeout, ensuring that the upstream gRPC server is aware of the actual timeout. diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 3292f4ce9e87..a02c8d27da00 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -3997,6 +3997,19 @@ 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-upstream-rq-timeout-ms", "15"}, + {"x-envoy-expected-rq-timeout-ms", "10"}}; + FilterUtility::TimeoutData timeout = + FilterUtility::finalTimeout(route, headers, 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("x-envoy-upstream-rq-timeout-ms")); + EXPECT_EQ("10", headers.get_("x-envoy-expected-rq-timeout-ms")); + EXPECT_FALSE(headers.has("grpc-timeout")); + } } TEST(RouterFilterUtilityTest, FinalTimeoutSupressEnvoyHeaders) { From 2b3080dc18a951fbb77843a1e529a80b39110d78 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 2 Sep 2019 15:02:46 +0200 Subject: [PATCH 02/13] apply review comments Signed-off-by: Kateryna Nezdolii --- source/common/router/router.cc | 41 ++++++++++++------- test/common/router/router_test.cc | 6 +-- .../http_timeout_integration_test.cc | 41 +++++++++++++++++++ 3 files changed, 70 insertions(+), 18 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 4df065a03a13..0ea41a16265c 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -151,32 +151,29 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he } timeout.per_try_timeout_ = route.retryPolicy().perTryTimeout(); - // See if there is any timeout to write in the expected timeout header. - uint64_t expected_timeout = timeout.per_try_timeout_.count(); - // Use the global timeout if no per try timeout was specified or if we're - // doing hedging when there are per try timeouts. Either of these scenarios - // mean that the upstream server can use the full global timeout. - if (per_try_timeout_hedging_enabled || expected_timeout == 0) { - expected_timeout = timeout.global_timeout_.count(); - } - // todo(nezdolik) Check if order is correct, add tests. - // Check if there is timeout set by egress envoy. If present, use that instead. uint64_t header_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-timeout-ms` + // header. Http::HeaderEntry* header_expected_timeout_entry = request_headers.EnvoyExpectedRequestTimeoutMs(); - if (!header_expected_timeout_entry) { + if (header_expected_timeout_entry) { + // This will prevent from overriding `x-envoy-expected-timeout-ms` header. + insert_envoy_expected_request_timeout_ms = false; + if (absl::SimpleAtoi(header_expected_timeout_entry->value().getStringView(), &header_timeout)) { + timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); + } + } else { Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); if (header_timeout_entry) { if (absl::SimpleAtoi(header_timeout_entry->value().getStringView(), &header_timeout)) { timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); } - request_headers.removeEnvoyUpstreamRequestTimeoutMs(); - } - if (insert_envoy_expected_request_timeout_ms && expected_timeout > 0) { - request_headers.insertEnvoyExpectedRequestTimeoutMs().value(expected_timeout); } } + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); + // See if there is a per try/retry timeout. If it's >= global we just ignore it. Http::HeaderEntry* per_try_timeout_entry = request_headers.EnvoyUpstreamRequestPerTryTimeoutMs(); if (per_try_timeout_entry) { @@ -189,6 +186,20 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he if (timeout.per_try_timeout_ >= timeout.global_timeout_) { timeout.per_try_timeout_ = std::chrono::milliseconds(0); } + + // See if there is any timeout to write in the expected timeout header. + uint64_t expected_timeout = timeout.per_try_timeout_.count(); + // Use the global timeout if no per try timeout was specified or if we're + // doing hedging when there are per try timeouts. Either of these scenarios + // mean that the upstream server can use the full global timeout. + if (per_try_timeout_hedging_enabled || expected_timeout == 0) { + expected_timeout = timeout.global_timeout_.count(); + } + + if (insert_envoy_expected_request_timeout_ms && expected_timeout > 0) { + request_headers.insertEnvoyExpectedRequestTimeoutMs().value(expected_timeout); + } + // If we've configured max_grpc_timeout, override the grpc-timeout header with // the expected timeout. This ensures that the optional per try timeout is reflected // in grpc-timeout, ensuring that the upstream gRPC server is aware of the actual timeout. diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index a02c8d27da00..edef35dde626 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4001,13 +4001,13 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { NiceMock route; EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}, - {"x-envoy-expected-rq-timeout-ms", "10"}}; + {"x-envoy-expected-rq-timeout-ms", "8"}}; FilterUtility::TimeoutData timeout = FilterUtility::finalTimeout(route, headers, true, false, false); - EXPECT_EQ(std::chrono::milliseconds(10), timeout.global_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_EQ("10", headers.get_("x-envoy-expected-rq-timeout-ms")); + EXPECT_EQ("8", headers.get_("x-envoy-expected-rq-timeout-ms")); EXPECT_FALSE(headers.has("grpc-timeout")); } } diff --git a/test/integration/http_timeout_integration_test.cc b/test/integration/http_timeout_integration_test.cc index 12e980b9e309..28efb749681d 100644 --- a/test/integration/http_timeout_integration_test.cc +++ b/test/integration/http_timeout_integration_test.cc @@ -47,6 +47,47 @@ 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. +// Sends a request with a global timeout specified, sleeps for longer than the +// timeout, and ensures that a timeout is received. +TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutEgressIngressEnvoys) { +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()); +} + // 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 From b36858a44cf9e8e51305185450f22b72b6de585e Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 2 Sep 2019 15:11:33 +0200 Subject: [PATCH 03/13] fix format Signed-off-by: Kateryna Nezdolii --- .../config/filter/http/router/v2/router.proto | 2 + source/common/http/async_client_impl.cc | 2 +- source/common/router/router.cc | 42 +- source/common/router/router.h | 12 +- test/common/router/router_test.cc | 88 +++- .../http/jwt_authn/filter_integration_test.cc | 1 + .../http_timeout_integration_test.cc | 478 ++++++++++-------- .../http_timeout_integration_test.h | 11 + 8 files changed, 369 insertions(+), 267 deletions(-) diff --git a/api/envoy/config/filter/http/router/v2/router.proto b/api/envoy/config/filter/http/router/v2/router.proto index fd0cadec9631..25e55c1f8e4f 100644 --- a/api/envoy/config/filter/http/router/v2/router.proto +++ b/api/envoy/config/filter/http/router/v2/router.proto @@ -63,4 +63,6 @@ message Router { "x-envoy-retry-on" ] }]; + + bool respect_expected_rq_timeout = 6; } diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 054a2ef2dede..141e8b464003 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 0ea41a16265c..a525bae41d32 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -121,7 +121,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 @@ -152,16 +153,29 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he timeout.per_try_timeout_ = route.retryPolicy().perTryTimeout(); uint64_t header_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-timeout-ms` - // header. - Http::HeaderEntry* header_expected_timeout_entry = - request_headers.EnvoyExpectedRequestTimeoutMs(); - if (header_expected_timeout_entry) { - // This will prevent from overriding `x-envoy-expected-timeout-ms` header. - insert_envoy_expected_request_timeout_ms = false; - if (absl::SimpleAtoi(header_expected_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 sanitised by egress envoy. + Http::HeaderEntry* header_expected_timeout_entry = + request_headers.EnvoyExpectedRequestTimeoutMs(); + if (header_expected_timeout_entry) { + // This will prevent from overriding `x-envoy-expected-rq-timeout-ms` header. + insert_envoy_expected_request_timeout_ms = false; + if (absl::SimpleAtoi(header_expected_timeout_entry->value().getStringView(), + &header_timeout)) { + timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); + } + } else { + Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); + if (header_timeout_entry) { + if (absl::SimpleAtoi(header_timeout_entry->value().getStringView(), &header_timeout)) { + timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); + } + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); + } } } else { Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); @@ -169,11 +183,10 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he if (absl::SimpleAtoi(header_timeout_entry->value().getStringView(), &header_timeout)) { timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); } + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); } } - request_headers.removeEnvoyUpstreamRequestTimeoutMs(); - // See if there is a per try/retry timeout. If it's >= global we just ignore it. Http::HeaderEntry* per_try_timeout_entry = request_headers.EnvoyUpstreamRequestPerTryTimeoutMs(); if (per_try_timeout_entry) { @@ -498,7 +511,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 89de83d0cf2a..59e733000298 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -140,7 +140,8 @@ 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); /** * Determine the final hedging settings after applying randomized behavior. @@ -161,12 +162,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 +189,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 +210,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 edef35dde626..c77ebbd3d29f 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -85,7 +85,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"); @@ -3703,7 +3703,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_); } @@ -3712,7 +3712,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")); @@ -3724,7 +3724,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")); @@ -3737,7 +3737,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")); @@ -3751,7 +3751,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")); @@ -3765,7 +3765,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")); @@ -3780,7 +3780,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")); @@ -3794,7 +3794,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")); @@ -3809,7 +3809,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")); @@ -3823,7 +3823,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")); @@ -3834,7 +3834,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")); @@ -3846,7 +3846,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")); @@ -3858,7 +3858,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")); @@ -3869,7 +3869,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")); @@ -3882,7 +3882,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_); } @@ -3894,7 +3894,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_); } @@ -3906,7 +3906,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")); @@ -3921,7 +3921,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")); @@ -3937,7 +3937,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")); @@ -3954,7 +3954,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")); @@ -3971,7 +3971,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")); @@ -3989,7 +3989,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")); @@ -4000,13 +4000,45 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { { NiceMock route; EXPECT_CALL(route, timeout()).WillOnce(Return(std::chrono::milliseconds(10))); - Http::TestHeaderMapImpl headers{{"x-envoy-upstream-rq-timeout-ms", "15"}, - {"x-envoy-expected-rq-timeout-ms", "8"}}; - FilterUtility::TimeoutData timeout = - FilterUtility::finalTimeout(route, headers, true, false, false); + 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-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")); } @@ -4018,7 +4050,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/extensions/filters/http/jwt_authn/filter_integration_test.cc b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc index 7f064293f3e2..b53c1ca3741e 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -83,6 +83,7 @@ std::string getFilterConfig(bool use_local_jwks) { using LocalJwksIntegrationTest = HttpProtocolIntegrationTest; +/*********/ INSTANTIATE_TEST_SUITE_P(Protocols, LocalJwksIntegrationTest, testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), HttpProtocolIntegrationTest::protocolTestParamsToString); diff --git a/test/integration/http_timeout_integration_test.cc b/test/integration/http_timeout_integration_test.cc index 28efb749681d..8b6f6a0a38e8 100644 --- a/test/integration/http_timeout_integration_test.cc +++ b/test/integration/http_timeout_integration_test.cc @@ -47,45 +47,83 @@ 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. -// Sends a request with a global timeout specified, sleeps for longer than the -// timeout, and ensures that a timeout is received. -TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutEgressIngressEnvoys) { -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 `x-envoy-expected-timeout-ms` header, set by egress envoy, is respected by ingress +// envoy. Sends a request with a global timeout specified, sleeps for longer than the timeout, and +// ensures that a timeout is received. +// TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutEgressIngressEnvoys) { +// 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()); +//} + +TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutEgressIngressEnvoys2) { + 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(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 @@ -182,186 +220,186 @@ TEST_P(HttpTimeoutIntegrationTest, PerTryTimeout) { EXPECT_TRUE(response->complete()); EXPECT_EQ("504", response->headers().Status()->value().getStringView()); } - -// With hedge_on_per_try_timeout enabled via config, sends a request with a -// global timeout and per try timeout specified, sleeps for longer than the per -// try but slightly less than the global timeout. We then have the first -// upstream request return headers and expect those to be returned downstream -// (which proves the request was not canceled when the timeout was hit). -TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeout) { - 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-retry-on", "5xx"}, - {"x-envoy-hedge-on-per-try-timeout", "true"}, - {"x-envoy-upstream-rq-timeout-ms", "500"}, - {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}); - 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 per try timeout (but not global timeout). - timeSystem().sleep(std::chrono::milliseconds(400)); - - // Trigger retry (there's a 25ms backoff before it's issued). - timeSystem().sleep(std::chrono::milliseconds(26)); - - // Wait for a second request to be sent upstream - FakeStreamPtr upstream_request2; - ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request2)); - ASSERT_TRUE(upstream_request2->waitForHeadersComplete()); - ASSERT_TRUE(upstream_request2->waitForEndStream(*dispatcher_)); - - // Encode 200 response headers for the first (timed out) request. - Http::TestHeaderMapImpl response_headers{{":status", "200"}}; - upstream_request_->encodeHeaders(response_headers, true); - - response->waitForHeaders(); - - // The second request should be reset since we used the response from the first request. - ASSERT_TRUE(upstream_request2->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("200", response->headers().Status()->value().getStringView()); -} - -TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeoutWithBodyNoBufferFirstRequestWins) { - testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 512, true); -} - -TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeoutWithBodyNoBufferSecondRequestWins) { - testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 512, false); -} - -TEST_P(HttpTimeoutIntegrationTest, - HedgedPerTryTimeoutLowUpstreamBufferLimitLargeRequestFirstRequestWins) { - config_helper_.setBufferLimits(1024, 1024 * 1024); // Set buffer limits upstream and downstream. - testRouterRequestAndResponseWithHedgedPerTryTimeout(1024 * 1024, 1024, true); -} - -TEST_P(HttpTimeoutIntegrationTest, - HedgedPerTryTimeoutLowUpstreamBufferLimitLargeRequestSecondRequestWins) { - config_helper_.setBufferLimits(1024, 1024 * 1024); // Set buffer limits upstream and downstream. - testRouterRequestAndResponseWithHedgedPerTryTimeout(1024 * 1024, 1024, false); -} - -TEST_P(HttpTimeoutIntegrationTest, - HedgedPerTryTimeoutLowDownstreamBufferLimitLargeResponseFirstRequestWins) { - config_helper_.setBufferLimits(1024 * 1024, 1024); // Set buffer limits upstream and downstream. - testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 1024 * 1024, true); -} - -TEST_P(HttpTimeoutIntegrationTest, - HedgedPerTryTimeoutLowDownstreamBufferLimitLargeResponseSecondRequestWins) { - config_helper_.setBufferLimits(1024 * 1024, 1024); // Set buffer limits upstream and downstream. - testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 1024 * 1024, false); -} - -// Sends a request with x-envoy-hedge-on-per-try-timeout, sleeps (with -// simulated time) for longer than the per try timeout but shorter than the -// global timeout, asserts that a retry is sent, and then responds with a 200 -// response on the original request and ensures the downstream sees it. -// Request/response/header size are configurable to test flow control. If -// first_request_wins is true, then the "winning" response will be sent in -// response to the first (timed out) request. If false, the second request will -// get the good response. -void HttpTimeoutIntegrationTest::testRouterRequestAndResponseWithHedgedPerTryTimeout( - uint64_t request_size, uint64_t response_size, bool first_request_wins) { - initialize(); - - codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); - Http::TestHeaderMapImpl request_headers{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}, - {"x-forwarded-for", "10.0.0.1"}, - {"x-envoy-retry-on", "5xx"}, - {"x-envoy-hedge-on-per-try-timeout", "true"}, - {"x-envoy-upstream-rq-timeout-ms", "5000"}, - {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}; - auto encoder_decoder = codec_client_->startRequest(request_headers); - - 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_, request_size, true); - - ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); - - // Trigger per try timeout (but not global timeout). - timeSystem().sleep(std::chrono::milliseconds(400)); - - FakeStreamPtr upstream_request2; - // Trigger retry (there's a 25ms backoff before it's issued). - timeSystem().sleep(std::chrono::milliseconds(26)); - - // Wait for a second request to be sent upstream - ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request2)); - ASSERT_TRUE(upstream_request2->waitForHeadersComplete()); - ASSERT_TRUE(upstream_request2->waitForEndStream(*dispatcher_)); - - Http::TestHeaderMapImpl response_headers{{":status", "200"}}; - if (first_request_wins) { - // Encode 200 response headers for the first (timed out) request. - upstream_request_->encodeHeaders(response_headers, response_size == 0); - } else { - // Encode 200 response headers for the second request. - upstream_request2->encodeHeaders(response_headers, response_size == 0); - } - - response->waitForHeaders(); - - if (first_request_wins) { - // The second request should be reset since we used the response from the first request. - ASSERT_TRUE(upstream_request2->waitForReset(std::chrono::seconds(15))); - } else { - // The first request should be reset since we used the response from the second request. - ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15))); - } - - if (response_size) { - if (first_request_wins) { - upstream_request_->encodeData(response_size, true); - } else { - upstream_request2->encodeData(response_size, true); - } - } - - response->waitForEndStream(); - - codec_client_->close(); - - EXPECT_TRUE(upstream_request_->complete()); - EXPECT_TRUE(upstream_request2->complete()); - if (first_request_wins) { - EXPECT_EQ(request_size, upstream_request_->bodyLength()); - } else { - EXPECT_EQ(request_size, upstream_request2->bodyLength()); - } - - EXPECT_TRUE(response->complete()); - EXPECT_EQ("200", response->headers().Status()->value().getStringView()); -} +// +//// With hedge_on_per_try_timeout enabled via config, sends a request with a +//// global timeout and per try timeout specified, sleeps for longer than the per +//// try but slightly less than the global timeout. We then have the first +//// upstream request return headers and expect those to be returned downstream +//// (which proves the request was not canceled when the timeout was hit). +// TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeout) { +// 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-retry-on", "5xx"}, +// {"x-envoy-hedge-on-per-try-timeout", "true"}, +// {"x-envoy-upstream-rq-timeout-ms", "500"}, +// {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}); +// 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 per try timeout (but not global timeout). +// timeSystem().sleep(std::chrono::milliseconds(400)); +// +// // Trigger retry (there's a 25ms backoff before it's issued). +// timeSystem().sleep(std::chrono::milliseconds(26)); +// +// // Wait for a second request to be sent upstream +// FakeStreamPtr upstream_request2; +// ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request2)); +// ASSERT_TRUE(upstream_request2->waitForHeadersComplete()); +// ASSERT_TRUE(upstream_request2->waitForEndStream(*dispatcher_)); +// +// // Encode 200 response headers for the first (timed out) request. +// Http::TestHeaderMapImpl response_headers{{":status", "200"}}; +// upstream_request_->encodeHeaders(response_headers, true); +// +// response->waitForHeaders(); +// +// // The second request should be reset since we used the response from the first request. +// ASSERT_TRUE(upstream_request2->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("200", response->headers().Status()->value().getStringView()); +//} +// +// TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeoutWithBodyNoBufferFirstRequestWins) { +// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 512, true); +//} +// +// TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeoutWithBodyNoBufferSecondRequestWins) { +// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 512, false); +//} +// +// TEST_P(HttpTimeoutIntegrationTest, +// HedgedPerTryTimeoutLowUpstreamBufferLimitLargeRequestFirstRequestWins) { +// config_helper_.setBufferLimits(1024, 1024 * 1024); // Set buffer limits upstream and downstream. +// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024 * 1024, 1024, true); +//} +// +// TEST_P(HttpTimeoutIntegrationTest, +// HedgedPerTryTimeoutLowUpstreamBufferLimitLargeRequestSecondRequestWins) { +// config_helper_.setBufferLimits(1024, 1024 * 1024); // Set buffer limits upstream and downstream. +// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024 * 1024, 1024, false); +//} +// +// TEST_P(HttpTimeoutIntegrationTest, +// HedgedPerTryTimeoutLowDownstreamBufferLimitLargeResponseFirstRequestWins) { +// config_helper_.setBufferLimits(1024 * 1024, 1024); // Set buffer limits upstream and downstream. +// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 1024 * 1024, true); +//} +// +// TEST_P(HttpTimeoutIntegrationTest, +// HedgedPerTryTimeoutLowDownstreamBufferLimitLargeResponseSecondRequestWins) { +// config_helper_.setBufferLimits(1024 * 1024, 1024); // Set buffer limits upstream and downstream. +// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 1024 * 1024, false); +//} +// +//// Sends a request with x-envoy-hedge-on-per-try-timeout, sleeps (with +//// simulated time) for longer than the per try timeout but shorter than the +//// global timeout, asserts that a retry is sent, and then responds with a 200 +//// response on the original request and ensures the downstream sees it. +//// Request/response/header size are configurable to test flow control. If +//// first_request_wins is true, then the "winning" response will be sent in +//// response to the first (timed out) request. If false, the second request will +//// get the good response. +// void HttpTimeoutIntegrationTest::testRouterRequestAndResponseWithHedgedPerTryTimeout( +// uint64_t request_size, uint64_t response_size, bool first_request_wins) { +// initialize(); +// +// codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); +// Http::TestHeaderMapImpl request_headers{{":method", "POST"}, +// {":path", "/test/long/url"}, +// {":scheme", "http"}, +// {":authority", "host"}, +// {"x-forwarded-for", "10.0.0.1"}, +// {"x-envoy-retry-on", "5xx"}, +// {"x-envoy-hedge-on-per-try-timeout", "true"}, +// {"x-envoy-upstream-rq-timeout-ms", "5000"}, +// {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}; +// auto encoder_decoder = codec_client_->startRequest(request_headers); +// +// 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_, request_size, true); +// +// ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); +// +// // Trigger per try timeout (but not global timeout). +// timeSystem().sleep(std::chrono::milliseconds(400)); +// +// FakeStreamPtr upstream_request2; +// // Trigger retry (there's a 25ms backoff before it's issued). +// timeSystem().sleep(std::chrono::milliseconds(26)); +// +// // Wait for a second request to be sent upstream +// ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request2)); +// ASSERT_TRUE(upstream_request2->waitForHeadersComplete()); +// ASSERT_TRUE(upstream_request2->waitForEndStream(*dispatcher_)); +// +// Http::TestHeaderMapImpl response_headers{{":status", "200"}}; +// if (first_request_wins) { +// // Encode 200 response headers for the first (timed out) request. +// upstream_request_->encodeHeaders(response_headers, response_size == 0); +// } else { +// // Encode 200 response headers for the second request. +// upstream_request2->encodeHeaders(response_headers, response_size == 0); +// } +// +// response->waitForHeaders(); +// +// if (first_request_wins) { +// // The second request should be reset since we used the response from the first request. +// ASSERT_TRUE(upstream_request2->waitForReset(std::chrono::seconds(15))); +// } else { +// // The first request should be reset since we used the response from the second request. +// ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15))); +// } +// +// if (response_size) { +// if (first_request_wins) { +// upstream_request_->encodeData(response_size, true); +// } else { +// upstream_request2->encodeData(response_size, true); +// } +// } +// +// response->waitForEndStream(); +// +// codec_client_->close(); +// +// EXPECT_TRUE(upstream_request_->complete()); +// EXPECT_TRUE(upstream_request2->complete()); +// if (first_request_wins) { +// EXPECT_EQ(request_size, upstream_request_->bodyLength()); +// } else { +// EXPECT_EQ(request_size, upstream_request2->bodyLength()); +// } +// +// EXPECT_TRUE(response->complete()); +// EXPECT_EQ("200", response->headers().Status()->value().getStringView()); +//} } // namespace Envoy diff --git a/test/integration/http_timeout_integration_test.h b/test/integration/http_timeout_integration_test.h index 230a82d2577a..f7d7950521b1 100644 --- a/test/integration/http_timeout_integration_test.h +++ b/test/integration/http_timeout_integration_test.h @@ -21,6 +21,17 @@ class HttpTimeoutIntegrationTest : public testing::TestWithParammutable_config()); + }); + HttpIntegrationTest::initialize(); + } }; } // namespace Envoy From 99cd2cd0d586f5e481b3c859923990816f264897 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 23 Sep 2019 19:17:13 +0200 Subject: [PATCH 04/13] Add more tests Signed-off-by: Kateryna Nezdolii --- .../config/filter/http/router/v2/router.proto | 4 + .../http_timeout_integration_test.cc | 217 +++++++++++------- .../http_timeout_integration_test.h | 21 +- 3 files changed, 149 insertions(+), 93 deletions(-) diff --git a/api/envoy/config/filter/http/router/v2/router.proto b/api/envoy/config/filter/http/router/v2/router.proto index 25e55c1f8e4f..c38822f11ab6 100644 --- a/api/envoy/config/filter/http/router/v2/router.proto +++ b/api/envoy/config/filter/http/router/v2/router.proto @@ -64,5 +64,9 @@ message Router { ] }]; + // If set to true, envoy first will check if `x-envoy-expected-timeout-ms` header is present + // and use it's value as timeout to upstream cluster. If header is not present or + // `respect_expected_rq_timeout` is set to false, envoy will derive timeout value from + // `x-envoy-upstream-rq-timeout-ms` header. bool respect_expected_rq_timeout = 6; } diff --git a/test/integration/http_timeout_integration_test.cc b/test/integration/http_timeout_integration_test.cc index 8b6f6a0a38e8..8ab5dd3a9d84 100644 --- a/test/integration/http_timeout_integration_test.cc +++ b/test/integration/http_timeout_integration_test.cc @@ -48,49 +48,11 @@ TEST_P(HttpTimeoutIntegrationTest, GlobalTimeout) { } // Testing that `x-envoy-expected-timeout-ms` header, set by egress envoy, is respected by ingress -// envoy. Sends a request with a global timeout specified, sleeps for longer than the timeout, and -// ensures that a timeout is received. -// TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutEgressIngressEnvoys) { -// 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()); -//} - -TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutEgressIngressEnvoys2) { +// 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"}, @@ -111,7 +73,7 @@ TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutEgressIngressEnvoys2) { ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); // Trigger global timeout, populated from `x-envoy-expected-rq-timeout-ms` header. - timeSystem().sleep(std::chrono::milliseconds(501)); + timeSystem().sleep(std::chrono::milliseconds(301)); // Ensure we got a timeout downstream and canceled the upstream request. response->waitForHeaders(); @@ -126,58 +88,52 @@ TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutEgressIngressEnvoys2) { 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 -// called on a destroyed UpstreamRequest. -TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutAfterHeadersBeforeBodyResetsUpstream) { +// 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"))); - Http::TestHeaderMapImpl request_headers{{":method", "POST"}, - {":path", "/test/long/url"}, - {":scheme", "http"}, - {":authority", "host"}, - {"x-forwarded-for", "10.0.0.1"}, - {"x-envoy-upstream-rq-timeout-ms", "100"}}; - std::pair encoder_decoder = - codec_client_->startRequest(request_headers); - + 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_, 100, true); + codec_client_->sendData(*request_encoder_, 0, true); ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); - // Respond with headers, not end of stream. - Http::TestHeaderMapImpl response_headers{{":status", "200"}}; - upstream_request_->encodeHeaders(response_headers, false); + // 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(); - EXPECT_EQ("200", response->headers().Status()->value().getStringView()); - - // Trigger global timeout. - timeSystem().sleep(std::chrono::milliseconds(200)); - ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15))); - response->waitForReset(); - 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()); } -// Sends a request with a global timeout and per try timeout specified, sleeps -// for longer than the per try but slightly less than the global timeout. -// Ensures that two requests are attempted and a timeout is returned -// downstream. -TEST_P(HttpTimeoutIntegrationTest, PerTryTimeout) { +// 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"))); @@ -187,9 +143,8 @@ TEST_P(HttpTimeoutIntegrationTest, PerTryTimeout) { {":scheme", "http"}, {":authority", "host"}, {"x-forwarded-for", "10.0.0.1"}, - {"x-envoy-retry-on", "5xx"}, {"x-envoy-upstream-rq-timeout-ms", "500"}, - {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}); + {"x-envoy-expected-rq-timeout-ms", "300"}}); auto response = std::move(encoder_decoder.second); request_encoder_ = &encoder_decoder.first; @@ -200,17 +155,12 @@ TEST_P(HttpTimeoutIntegrationTest, PerTryTimeout) { ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); - // Trigger per try timeout (but not global timeout). - timeSystem().sleep(std::chrono::milliseconds(400)); - - // Wait for a second request to be sent upstream - ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); - ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); - ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + // Trigger global timeout, populated from `x-envoy-expected-rq-timeout-ms` header. + timeSystem().sleep(std::chrono::milliseconds(501)); - // Trigger global timeout. - timeSystem().sleep(std::chrono::milliseconds(100)); + // 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(); @@ -220,6 +170,101 @@ TEST_P(HttpTimeoutIntegrationTest, PerTryTimeout) { 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 +// called on a destroyed UpstreamRequest. +TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutAfterHeadersBeforeBodyResetsUpstream) { + initialize(); + + codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); + Http::TestHeaderMapImpl request_headers{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, + {"x-envoy-upstream-rq-timeout-ms", "100"}}; + std::pair encoder_decoder = + codec_client_->startRequest(request_headers); + + 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_, 100, true); + + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + // Respond with headers, not end of stream. + Http::TestHeaderMapImpl response_headers{{":status", "200"}}; + upstream_request_->encodeHeaders(response_headers, false); + + response->waitForHeaders(); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); + + // Trigger global timeout. + timeSystem().sleep(std::chrono::milliseconds(200)); + + ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15))); + + response->waitForReset(); + + codec_client_->close(); + + EXPECT_TRUE(upstream_request_->complete()); +} +// +//// Sends a request with a global timeout and per try timeout specified, sleeps +//// for longer than the per try but slightly less than the global timeout. +//// Ensures that two requests are attempted and a timeout is returned +//// downstream. +// TEST_P(HttpTimeoutIntegrationTest, PerTryTimeout) { +// 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-retry-on", "5xx"}, +// {"x-envoy-upstream-rq-timeout-ms", "500"}, +// {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}); +// 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 per try timeout (but not global timeout). +// timeSystem().sleep(std::chrono::milliseconds(400)); +// +// // Wait for a second request to be sent upstream +// ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); +// ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); +// ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); +// +// // Trigger global timeout. +// timeSystem().sleep(std::chrono::milliseconds(100)); +// response->waitForHeaders(); +// +// 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()); +//} // //// With hedge_on_per_try_timeout enabled via config, sends a request with a //// global timeout and per try timeout specified, sleeps for longer than the per diff --git a/test/integration/http_timeout_integration_test.h b/test/integration/http_timeout_integration_test.h index f7d7950521b1..3c9aa733da11 100644 --- a/test/integration/http_timeout_integration_test.h +++ b/test/integration/http_timeout_integration_test.h @@ -23,15 +23,22 @@ class HttpTimeoutIntegrationTest : public testing::TestWithParammutable_config()); - }); + if (respect_expected_rq_timeout) { + config_helper_.addConfigModifier( + [&](envoy::config::filter::network::http_connection_manager::v2::HttpConnectionManager& + hcm) { + envoy::config::filter::http::router::v2::Router router_config; + router_config.set_respect_expected_rq_timeout(respect_expected_rq_timeout); + TestUtility::jsonConvert(router_config, *hcm.mutable_http_filters(0)->mutable_config()); + }); + } + HttpIntegrationTest::initialize(); } + + void enableRespectExpectedRqTimeout(bool enable) { respect_expected_rq_timeout = enable; } + + bool respect_expected_rq_timeout{false}; }; } // namespace Envoy From be4d59660c866ea544121a82562944e56cc3a323 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 23 Sep 2019 21:48:14 +0200 Subject: [PATCH 05/13] clean up Signed-off-by: Kateryna Nezdolii --- .../http_timeout_integration_test.cc | 458 +++++++++--------- 1 file changed, 229 insertions(+), 229 deletions(-) diff --git a/test/integration/http_timeout_integration_test.cc b/test/integration/http_timeout_integration_test.cc index 8ab5dd3a9d84..a6423a70f019 100644 --- a/test/integration/http_timeout_integration_test.cc +++ b/test/integration/http_timeout_integration_test.cc @@ -217,234 +217,234 @@ TEST_P(HttpTimeoutIntegrationTest, GlobalTimeoutAfterHeadersBeforeBodyResetsUpst EXPECT_TRUE(upstream_request_->complete()); } -// -//// Sends a request with a global timeout and per try timeout specified, sleeps -//// for longer than the per try but slightly less than the global timeout. -//// Ensures that two requests are attempted and a timeout is returned -//// downstream. -// TEST_P(HttpTimeoutIntegrationTest, PerTryTimeout) { -// 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-retry-on", "5xx"}, -// {"x-envoy-upstream-rq-timeout-ms", "500"}, -// {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}); -// 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 per try timeout (but not global timeout). -// timeSystem().sleep(std::chrono::milliseconds(400)); -// -// // Wait for a second request to be sent upstream -// ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); -// ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); -// ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); -// -// // Trigger global timeout. -// timeSystem().sleep(std::chrono::milliseconds(100)); -// response->waitForHeaders(); -// -// 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()); -//} -// -//// With hedge_on_per_try_timeout enabled via config, sends a request with a -//// global timeout and per try timeout specified, sleeps for longer than the per -//// try but slightly less than the global timeout. We then have the first -//// upstream request return headers and expect those to be returned downstream -//// (which proves the request was not canceled when the timeout was hit). -// TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeout) { -// 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-retry-on", "5xx"}, -// {"x-envoy-hedge-on-per-try-timeout", "true"}, -// {"x-envoy-upstream-rq-timeout-ms", "500"}, -// {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}); -// 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 per try timeout (but not global timeout). -// timeSystem().sleep(std::chrono::milliseconds(400)); -// -// // Trigger retry (there's a 25ms backoff before it's issued). -// timeSystem().sleep(std::chrono::milliseconds(26)); -// -// // Wait for a second request to be sent upstream -// FakeStreamPtr upstream_request2; -// ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request2)); -// ASSERT_TRUE(upstream_request2->waitForHeadersComplete()); -// ASSERT_TRUE(upstream_request2->waitForEndStream(*dispatcher_)); -// -// // Encode 200 response headers for the first (timed out) request. -// Http::TestHeaderMapImpl response_headers{{":status", "200"}}; -// upstream_request_->encodeHeaders(response_headers, true); -// -// response->waitForHeaders(); -// -// // The second request should be reset since we used the response from the first request. -// ASSERT_TRUE(upstream_request2->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("200", response->headers().Status()->value().getStringView()); -//} -// -// TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeoutWithBodyNoBufferFirstRequestWins) { -// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 512, true); -//} -// -// TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeoutWithBodyNoBufferSecondRequestWins) { -// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 512, false); -//} -// -// TEST_P(HttpTimeoutIntegrationTest, -// HedgedPerTryTimeoutLowUpstreamBufferLimitLargeRequestFirstRequestWins) { -// config_helper_.setBufferLimits(1024, 1024 * 1024); // Set buffer limits upstream and downstream. -// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024 * 1024, 1024, true); -//} -// -// TEST_P(HttpTimeoutIntegrationTest, -// HedgedPerTryTimeoutLowUpstreamBufferLimitLargeRequestSecondRequestWins) { -// config_helper_.setBufferLimits(1024, 1024 * 1024); // Set buffer limits upstream and downstream. -// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024 * 1024, 1024, false); -//} -// -// TEST_P(HttpTimeoutIntegrationTest, -// HedgedPerTryTimeoutLowDownstreamBufferLimitLargeResponseFirstRequestWins) { -// config_helper_.setBufferLimits(1024 * 1024, 1024); // Set buffer limits upstream and downstream. -// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 1024 * 1024, true); -//} -// -// TEST_P(HttpTimeoutIntegrationTest, -// HedgedPerTryTimeoutLowDownstreamBufferLimitLargeResponseSecondRequestWins) { -// config_helper_.setBufferLimits(1024 * 1024, 1024); // Set buffer limits upstream and downstream. -// testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 1024 * 1024, false); -//} -// -//// Sends a request with x-envoy-hedge-on-per-try-timeout, sleeps (with -//// simulated time) for longer than the per try timeout but shorter than the -//// global timeout, asserts that a retry is sent, and then responds with a 200 -//// response on the original request and ensures the downstream sees it. -//// Request/response/header size are configurable to test flow control. If -//// first_request_wins is true, then the "winning" response will be sent in -//// response to the first (timed out) request. If false, the second request will -//// get the good response. -// void HttpTimeoutIntegrationTest::testRouterRequestAndResponseWithHedgedPerTryTimeout( -// uint64_t request_size, uint64_t response_size, bool first_request_wins) { -// initialize(); -// -// codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); -// Http::TestHeaderMapImpl request_headers{{":method", "POST"}, -// {":path", "/test/long/url"}, -// {":scheme", "http"}, -// {":authority", "host"}, -// {"x-forwarded-for", "10.0.0.1"}, -// {"x-envoy-retry-on", "5xx"}, -// {"x-envoy-hedge-on-per-try-timeout", "true"}, -// {"x-envoy-upstream-rq-timeout-ms", "5000"}, -// {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}; -// auto encoder_decoder = codec_client_->startRequest(request_headers); -// -// 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_, request_size, true); -// -// ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); -// -// // Trigger per try timeout (but not global timeout). -// timeSystem().sleep(std::chrono::milliseconds(400)); -// -// FakeStreamPtr upstream_request2; -// // Trigger retry (there's a 25ms backoff before it's issued). -// timeSystem().sleep(std::chrono::milliseconds(26)); -// -// // Wait for a second request to be sent upstream -// ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request2)); -// ASSERT_TRUE(upstream_request2->waitForHeadersComplete()); -// ASSERT_TRUE(upstream_request2->waitForEndStream(*dispatcher_)); -// -// Http::TestHeaderMapImpl response_headers{{":status", "200"}}; -// if (first_request_wins) { -// // Encode 200 response headers for the first (timed out) request. -// upstream_request_->encodeHeaders(response_headers, response_size == 0); -// } else { -// // Encode 200 response headers for the second request. -// upstream_request2->encodeHeaders(response_headers, response_size == 0); -// } -// -// response->waitForHeaders(); -// -// if (first_request_wins) { -// // The second request should be reset since we used the response from the first request. -// ASSERT_TRUE(upstream_request2->waitForReset(std::chrono::seconds(15))); -// } else { -// // The first request should be reset since we used the response from the second request. -// ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15))); -// } -// -// if (response_size) { -// if (first_request_wins) { -// upstream_request_->encodeData(response_size, true); -// } else { -// upstream_request2->encodeData(response_size, true); -// } -// } -// -// response->waitForEndStream(); -// -// codec_client_->close(); -// -// EXPECT_TRUE(upstream_request_->complete()); -// EXPECT_TRUE(upstream_request2->complete()); -// if (first_request_wins) { -// EXPECT_EQ(request_size, upstream_request_->bodyLength()); -// } else { -// EXPECT_EQ(request_size, upstream_request2->bodyLength()); -// } -// -// EXPECT_TRUE(response->complete()); -// EXPECT_EQ("200", response->headers().Status()->value().getStringView()); -//} + +// Sends a request with a global timeout and per try timeout specified, sleeps +// for longer than the per try but slightly less than the global timeout. +// Ensures that two requests are attempted and a timeout is returned +// downstream. +TEST_P(HttpTimeoutIntegrationTest, PerTryTimeout) { + 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-retry-on", "5xx"}, + {"x-envoy-upstream-rq-timeout-ms", "500"}, + {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}); + 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 per try timeout (but not global timeout). + timeSystem().sleep(std::chrono::milliseconds(400)); + + // Wait for a second request to be sent upstream + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request_)); + ASSERT_TRUE(upstream_request_->waitForHeadersComplete()); + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + // Trigger global timeout. + timeSystem().sleep(std::chrono::milliseconds(100)); + response->waitForHeaders(); + + 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()); +} + +// With hedge_on_per_try_timeout enabled via config, sends a request with a +// global timeout and per try timeout specified, sleeps for longer than the per +// try but slightly less than the global timeout. We then have the first +// upstream request return headers and expect those to be returned downstream +// (which proves the request was not canceled when the timeout was hit). +TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeout) { + 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-retry-on", "5xx"}, + {"x-envoy-hedge-on-per-try-timeout", "true"}, + {"x-envoy-upstream-rq-timeout-ms", "500"}, + {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}); + 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 per try timeout (but not global timeout). + timeSystem().sleep(std::chrono::milliseconds(400)); + + // Trigger retry (there's a 25ms backoff before it's issued). + timeSystem().sleep(std::chrono::milliseconds(26)); + + // Wait for a second request to be sent upstream + FakeStreamPtr upstream_request2; + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request2)); + ASSERT_TRUE(upstream_request2->waitForHeadersComplete()); + ASSERT_TRUE(upstream_request2->waitForEndStream(*dispatcher_)); + + // Encode 200 response headers for the first (timed out) request. + Http::TestHeaderMapImpl response_headers{{":status", "200"}}; + upstream_request_->encodeHeaders(response_headers, true); + + response->waitForHeaders(); + + // The second request should be reset since we used the response from the first request. + ASSERT_TRUE(upstream_request2->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("200", response->headers().Status()->value().getStringView()); +} + +TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeoutWithBodyNoBufferFirstRequestWins) { + testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 512, true); +} + +TEST_P(HttpTimeoutIntegrationTest, HedgedPerTryTimeoutWithBodyNoBufferSecondRequestWins) { + testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 512, false); +} + +TEST_P(HttpTimeoutIntegrationTest, + HedgedPerTryTimeoutLowUpstreamBufferLimitLargeRequestFirstRequestWins) { + config_helper_.setBufferLimits(1024, 1024 * 1024); // Set buffer limits upstream and downstream. + testRouterRequestAndResponseWithHedgedPerTryTimeout(1024 * 1024, 1024, true); +} + +TEST_P(HttpTimeoutIntegrationTest, + HedgedPerTryTimeoutLowUpstreamBufferLimitLargeRequestSecondRequestWins) { + config_helper_.setBufferLimits(1024, 1024 * 1024); // Set buffer limits upstream and downstream. + testRouterRequestAndResponseWithHedgedPerTryTimeout(1024 * 1024, 1024, false); +} + +TEST_P(HttpTimeoutIntegrationTest, + HedgedPerTryTimeoutLowDownstreamBufferLimitLargeResponseFirstRequestWins) { + config_helper_.setBufferLimits(1024 * 1024, 1024); // Set buffer limits upstream and downstream. + testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 1024 * 1024, true); +} + +TEST_P(HttpTimeoutIntegrationTest, + HedgedPerTryTimeoutLowDownstreamBufferLimitLargeResponseSecondRequestWins) { + config_helper_.setBufferLimits(1024 * 1024, 1024); // Set buffer limits upstream and downstream. + testRouterRequestAndResponseWithHedgedPerTryTimeout(1024, 1024 * 1024, false); +} + +// Sends a request with x-envoy-hedge-on-per-try-timeout, sleeps (with +// simulated time) for longer than the per try timeout but shorter than the +// global timeout, asserts that a retry is sent, and then responds with a 200 +// response on the original request and ensures the downstream sees it. +// Request/response/header size are configurable to test flow control. If +// first_request_wins is true, then the "winning" response will be sent in +// response to the first (timed out) request. If false, the second request will +// get the good response. +void HttpTimeoutIntegrationTest::testRouterRequestAndResponseWithHedgedPerTryTimeout( + uint64_t request_size, uint64_t response_size, bool first_request_wins) { + initialize(); + + codec_client_ = makeHttpConnection(makeClientConnection(lookupPort("http"))); + Http::TestHeaderMapImpl request_headers{{":method", "POST"}, + {":path", "/test/long/url"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, + {"x-envoy-retry-on", "5xx"}, + {"x-envoy-hedge-on-per-try-timeout", "true"}, + {"x-envoy-upstream-rq-timeout-ms", "5000"}, + {"x-envoy-upstream-rq-per-try-timeout-ms", "400"}}; + auto encoder_decoder = codec_client_->startRequest(request_headers); + + 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_, request_size, true); + + ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); + + // Trigger per try timeout (but not global timeout). + timeSystem().sleep(std::chrono::milliseconds(400)); + + FakeStreamPtr upstream_request2; + // Trigger retry (there's a 25ms backoff before it's issued). + timeSystem().sleep(std::chrono::milliseconds(26)); + + // Wait for a second request to be sent upstream + ASSERT_TRUE(fake_upstream_connection_->waitForNewStream(*dispatcher_, upstream_request2)); + ASSERT_TRUE(upstream_request2->waitForHeadersComplete()); + ASSERT_TRUE(upstream_request2->waitForEndStream(*dispatcher_)); + + Http::TestHeaderMapImpl response_headers{{":status", "200"}}; + if (first_request_wins) { + // Encode 200 response headers for the first (timed out) request. + upstream_request_->encodeHeaders(response_headers, response_size == 0); + } else { + // Encode 200 response headers for the second request. + upstream_request2->encodeHeaders(response_headers, response_size == 0); + } + + response->waitForHeaders(); + + if (first_request_wins) { + // The second request should be reset since we used the response from the first request. + ASSERT_TRUE(upstream_request2->waitForReset(std::chrono::seconds(15))); + } else { + // The first request should be reset since we used the response from the second request. + ASSERT_TRUE(upstream_request_->waitForReset(std::chrono::seconds(15))); + } + + if (response_size) { + if (first_request_wins) { + upstream_request_->encodeData(response_size, true); + } else { + upstream_request2->encodeData(response_size, true); + } + } + + response->waitForEndStream(); + + codec_client_->close(); + + EXPECT_TRUE(upstream_request_->complete()); + EXPECT_TRUE(upstream_request2->complete()); + if (first_request_wins) { + EXPECT_EQ(request_size, upstream_request_->bodyLength()); + } else { + EXPECT_EQ(request_size, upstream_request2->bodyLength()); + } + + EXPECT_TRUE(response->complete()); + EXPECT_EQ("200", response->headers().Status()->value().getStringView()); +} } // namespace Envoy From de1fdf210512ac6cc8b42dd2c20955708dd4cf61 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 23 Sep 2019 21:56:12 +0200 Subject: [PATCH 06/13] clean up Signed-off-by: Kateryna Nezdolii --- .../extensions/filters/http/jwt_authn/filter_integration_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc index b53c1ca3741e..7f064293f3e2 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -83,7 +83,6 @@ std::string getFilterConfig(bool use_local_jwks) { using LocalJwksIntegrationTest = HttpProtocolIntegrationTest; -/*********/ INSTANTIATE_TEST_SUITE_P(Protocols, LocalJwksIntegrationTest, testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), HttpProtocolIntegrationTest::protocolTestParamsToString); From b0ea85adc8a489a4d9af336cb4a2570a6a93ef24 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 23 Sep 2019 22:05:09 +0200 Subject: [PATCH 07/13] fix spelling Signed-off-by: Kateryna Nezdolii --- source/common/router/router.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index a525bae41d32..0855a0241d86 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -158,7 +158,7 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he // 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 sanitised by egress envoy. + // header should have been sanitized by egress envoy. Http::HeaderEntry* header_expected_timeout_entry = request_headers.EnvoyExpectedRequestTimeoutMs(); if (header_expected_timeout_entry) { From cbb9594bb48225a7aa342661e189e2ccf1a0c88b Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 27 Sep 2019 12:51:04 +0200 Subject: [PATCH 08/13] apply review comments Signed-off-by: Kateryna Nezdolii --- .../config/filter/http/router/v2/router.proto | 6 ++---- source/common/router/router.cc | 8 +++----- test/common/router/router_test.cc | 14 ++++++++++++++ test/integration/http_timeout_integration_test.cc | 2 +- 4 files changed, 20 insertions(+), 10 deletions(-) diff --git a/api/envoy/config/filter/http/router/v2/router.proto b/api/envoy/config/filter/http/router/v2/router.proto index c38822f11ab6..718b0c615714 100644 --- a/api/envoy/config/filter/http/router/v2/router.proto +++ b/api/envoy/config/filter/http/router/v2/router.proto @@ -64,9 +64,7 @@ message Router { ] }]; - // If set to true, envoy first will check if `x-envoy-expected-timeout-ms` header is present - // and use it's value as timeout to upstream cluster. If header is not present or - // `respect_expected_rq_timeout` is set to false, envoy will derive timeout value from - // `x-envoy-upstream-rq-timeout-ms` header. + // 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/source/common/router/router.cc b/source/common/router/router.cc index 0855a0241d86..72e43c318606 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -155,15 +155,13 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he uint64_t header_timeout; if (respect_expected_rq_timeout) { - // Check if there is timeout set by egress envoy. + // 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. + // *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) { - // This will prevent from overriding `x-envoy-expected-rq-timeout-ms` header. - insert_envoy_expected_request_timeout_ms = false; if (absl::SimpleAtoi(header_expected_timeout_entry->value().getStringView(), &header_timeout)) { timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index c77ebbd3d29f..7951fdb55b97 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4010,6 +4010,20 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { 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))); diff --git a/test/integration/http_timeout_integration_test.cc b/test/integration/http_timeout_integration_test.cc index a6423a70f019..b44aef8c058f 100644 --- a/test/integration/http_timeout_integration_test.cc +++ b/test/integration/http_timeout_integration_test.cc @@ -156,7 +156,7 @@ TEST_P(HttpTimeoutIntegrationTest, IgnoreTimeoutSetByEgressEnvoy) { ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); // Trigger global timeout, populated from `x-envoy-expected-rq-timeout-ms` header. - timeSystem().sleep(std::chrono::milliseconds(501)); + timeSystem().sleep(std::chrono::milliseconds(301)); // Ensure we got a timeout downstream and canceled the upstream request. response->waitForHeaders(); From 90695dab5890dc84d30b03f031efd4a8a971f45d Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 27 Sep 2019 13:18:32 +0200 Subject: [PATCH 09/13] update docs, release notes and fix format Signed-off-by: Kateryna Nezdolii --- api/envoy/config/filter/http/router/v2/router.proto | 5 +++-- docs/root/intro/version_history.rst | 1 + test/common/router/router_test.cc | 4 ++-- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/api/envoy/config/filter/http/router/v2/router.proto b/api/envoy/config/filter/http/router/v2/router.proto index 718b0c615714..774cd2870534 100644 --- a/api/envoy/config/filter/http/router/v2/router.proto +++ b/api/envoy/config/filter/http/router/v2/router.proto @@ -64,7 +64,8 @@ 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. + // 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 7feec89f11bb..29842dcdaa9d 100644 --- a/docs/root/intro/version_history.rst +++ b/docs/root/intro/version_history.rst @@ -54,6 +54,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 check tool: add coverage reporting & enforcement. * router check tool: add comprehensive coverage reporting. * router check tool: add deprecated field check. diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 7951fdb55b97..d6749a165a3f 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -4013,8 +4013,8 @@ TEST(RouterFilterUtilityTest, FinalTimeout) { { 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"}}; + 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( From 8432d62181cab5c9432a284419303ef1c11b375b Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 27 Sep 2019 14:05:32 +0200 Subject: [PATCH 10/13] sync v2 and v3alpha Signed-off-by: Kateryna Nezdolii --- api/envoy/config/filter/http/router/v3alpha/router.proto | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/api/envoy/config/filter/http/router/v3alpha/router.proto b/api/envoy/config/filter/http/router/v3alpha/router.proto index a4ceae7dc1f7..047f7ca087e6 100644 --- a/api/envoy/config/filter/http/router/v3alpha/router.proto +++ b/api/envoy/config/filter/http/router/v3alpha/router.proto @@ -63,4 +63,9 @@ message Router { "x-envoy-retry-on" ] }]; + + // 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; } From 967acfd4dbfff552bd60f606dc3e891f20d55421 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Fri, 27 Sep 2019 17:00:45 +0200 Subject: [PATCH 11/13] fix test Signed-off-by: Kateryna Nezdolii --- test/integration/http_timeout_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/http_timeout_integration_test.cc b/test/integration/http_timeout_integration_test.cc index b44aef8c058f..097c085eb114 100644 --- a/test/integration/http_timeout_integration_test.cc +++ b/test/integration/http_timeout_integration_test.cc @@ -144,7 +144,7 @@ TEST_P(HttpTimeoutIntegrationTest, IgnoreTimeoutSetByEgressEnvoy) { {":authority", "host"}, {"x-forwarded-for", "10.0.0.1"}, {"x-envoy-upstream-rq-timeout-ms", "500"}, - {"x-envoy-expected-rq-timeout-ms", "300"}}); + {"x-envoy-expected-rq-timeout-ms", "600"}}); auto response = std::move(encoder_decoder.second); request_encoder_ = &encoder_decoder.first; @@ -156,7 +156,7 @@ TEST_P(HttpTimeoutIntegrationTest, IgnoreTimeoutSetByEgressEnvoy) { ASSERT_TRUE(upstream_request_->waitForEndStream(*dispatcher_)); // Trigger global timeout, populated from `x-envoy-expected-rq-timeout-ms` header. - timeSystem().sleep(std::chrono::milliseconds(301)); + timeSystem().sleep(std::chrono::milliseconds(501)); // Ensure we got a timeout downstream and canceled the upstream request. response->waitForHeaders(); From 1450fc94cc0ed9f7944ea93abe74838c5495c0c0 Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 7 Oct 2019 11:26:50 +0200 Subject: [PATCH 12/13] apply review comments Signed-off-by: Kateryna Nezdolii --- source/common/router/router.cc | 37 +++++++++++++++++++--------------- source/common/router/router.h | 4 +++- 2 files changed, 24 insertions(+), 17 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 72e43c318606..365c93b1351f 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -162,27 +162,21 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he Http::HeaderEntry* header_expected_timeout_entry = request_headers.EnvoyExpectedRequestTimeoutMs(); if (header_expected_timeout_entry) { - if (absl::SimpleAtoi(header_expected_timeout_entry->value().getStringView(), - &header_timeout)) { - timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); - } + trySetGlobalTimeout(header_expected_timeout_entry); } else { Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); - if (header_timeout_entry) { - if (absl::SimpleAtoi(header_timeout_entry->value().getStringView(), &header_timeout)) { - timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); - } - request_headers.removeEnvoyUpstreamRequestTimeoutMs(); - } + + if (trySetGlobalTimeout(header_timeout_entry)) { + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); + } + } } else { Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); - if (header_timeout_entry) { - if (absl::SimpleAtoi(header_timeout_entry->value().getStringView(), &header_timeout)) { - timeout.global_timeout_ = std::chrono::milliseconds(header_timeout); - } - request_headers.removeEnvoyUpstreamRequestTimeoutMs(); - } + if (trySetGlobalTimeout(header_timeout_entry)) { + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); + } + } // See if there is a per try/retry timeout. If it's >= global we just ignore it. @@ -223,6 +217,17 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he return timeout; } +bool FilterUtility::trySetGlobalTimeout(const Http::HeaderEntry& header_timeout_entry) { + 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; diff --git a/source/common/router/router.h b/source/common/router/router.h index 59e733000298..d171e46b9679 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -143,7 +143,9 @@ class FilterUtility { bool per_try_timeout_hedging_enabled, bool respect_expected_rq_timeout); - /** + static void trySetGlobalTimeout(const Http::HeaderEntry& header_timeout_entry); + + /** * Determine the final hedging settings after applying randomized behavior. * @param route supplies the request route. * @param request_headers supplies the request headers. From cc6f485ce3b31557b8d96947866d35f4c48b6c7f Mon Sep 17 00:00:00 2001 From: Kateryna Nezdolii Date: Mon, 7 Oct 2019 13:10:35 +0200 Subject: [PATCH 13/13] fix format Signed-off-by: Kateryna Nezdolii --- source/common/router/router.cc | 35 +++++++++++++++++----------------- source/common/router/router.h | 5 +++-- 2 files changed, 20 insertions(+), 20 deletions(-) diff --git a/source/common/router/router.cc b/source/common/router/router.cc index 365c93b1351f..fe85bd8ed2d2 100644 --- a/source/common/router/router.cc +++ b/source/common/router/router.cc @@ -162,21 +162,19 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he Http::HeaderEntry* header_expected_timeout_entry = request_headers.EnvoyExpectedRequestTimeoutMs(); if (header_expected_timeout_entry) { - trySetGlobalTimeout(header_expected_timeout_entry); + trySetGlobalTimeout(header_expected_timeout_entry, timeout); } else { Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); - if (trySetGlobalTimeout(header_timeout_entry)) { - request_headers.removeEnvoyUpstreamRequestTimeoutMs(); - } - + if (trySetGlobalTimeout(header_timeout_entry, timeout)) { + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); + } } } else { Http::HeaderEntry* header_timeout_entry = request_headers.EnvoyUpstreamRequestTimeoutMs(); - if (trySetGlobalTimeout(header_timeout_entry)) { - request_headers.removeEnvoyUpstreamRequestTimeoutMs(); - } - + if (trySetGlobalTimeout(header_timeout_entry, timeout)) { + request_headers.removeEnvoyUpstreamRequestTimeoutMs(); + } } // See if there is a per try/retry timeout. If it's >= global we just ignore it. @@ -217,15 +215,16 @@ FilterUtility::finalTimeout(const RouteEntry& route, Http::HeaderMap& request_he return timeout; } -bool FilterUtility::trySetGlobalTimeout(const Http::HeaderEntry& header_timeout_entry) { - 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; +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, diff --git a/source/common/router/router.h b/source/common/router/router.h index d171e46b9679..eed71f01bf0b 100644 --- a/source/common/router/router.h +++ b/source/common/router/router.h @@ -143,9 +143,10 @@ class FilterUtility { bool per_try_timeout_hedging_enabled, bool respect_expected_rq_timeout); - static void trySetGlobalTimeout(const Http::HeaderEntry& header_timeout_entry); + static bool trySetGlobalTimeout(const Http::HeaderEntry* header_timeout_entry, + TimeoutData& timeout); - /** + /** * Determine the final hedging settings after applying randomized behavior. * @param route supplies the request route. * @param request_headers supplies the request headers.