From 25fc0dc7593ae9da6ec21661bbc5922798c2073f Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Thu, 12 Mar 2020 14:13:42 -0700 Subject: [PATCH 1/2] router: add missing unit tests for x-envoy-attempt-count on upstream requests Signed-off-by: Jose Nino --- test/common/router/router_test.cc | 119 ++++++++++++++++++++++++++++++ 1 file changed, 119 insertions(+) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index 208d01f0f68ef..da866dcbe5160 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -242,6 +242,10 @@ class RouterTestBase : public testing::Test { StreamInfo::FilterState::LifeSpan::DownstreamRequest); } + void setIncludeAttemptCount(bool include) { + ON_CALL(callbacks_.route_->route_entry_, includeAttemptCount()).WillByDefault(Return(include)); + } + void enableHedgeOnPerTryTimeout() { callbacks_.route_->route_entry_.hedge_policy_.hedge_on_per_try_timeout_ = true; callbacks_.route_->route_entry_.hedge_policy_.additional_request_chance_ = @@ -868,6 +872,121 @@ TEST_F(RouterTest, EnvoyUpstreamServiceTime) { EXPECT_TRUE(verifyHostUpstreamStats(1, 0)); } +// Validate that x-envoy-attempt-count is added to request headers when option is true. +TEST_F(RouterTest, EnvoyAttemptCountInRequest) { + setIncludeAttemptCount(true); + + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); + expectResponseTimerCreate(); + + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + EXPECT_EQ(1, atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); + + // When the router filter gets reset we should cancel the pool request. + EXPECT_CALL(cancellable_, cancel()); + router_.onDestroy(); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); +} + +// Validate that x-envoy-attempt-count is overwritten by the router on request headers, if the +// header is sent from the downstream and the option is set to true. +TEST_F(RouterTest, EnvoyAttemptCountInRequestOverwritten) { + setIncludeAttemptCount(true); + + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); + expectResponseTimerCreate(); + + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + headers.setEnvoyAttemptCount(123); + router_.decodeHeaders(headers, true); + + EXPECT_EQ(1, atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); + + // When the router filter gets reset we should cancel the pool request. + EXPECT_CALL(cancellable_, cancel()); + router_.onDestroy(); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); +} + +// Validate that x-envoy-attempt-count is not overwritten by the router on request headers, if the +// header is sent from the downstream and the option is set to false. +TEST_F(RouterTest, EnvoyAttemptCountInRequestNotOverwritten) { + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); + expectResponseTimerCreate(); + + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + headers.setEnvoyAttemptCount(123); + router_.decodeHeaders(headers, true); + + EXPECT_EQ(123, atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); + + // When the router filter gets reset we should cancel the pool request. + EXPECT_CALL(cancellable_, cancel()); + router_.onDestroy(); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); +} + +TEST_F(RouterTest, EnvoyAttemptCountInRequestUpdatedInRetries) { + setIncludeAttemptCount(true); + + NiceMock encoder1; + Http::ResponseDecoder* response_decoder = nullptr; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke( + [&](Http::ResponseDecoder& decoder, + Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder1, cm_.conn_pool_.host_, upstream_stream_info_); + return nullptr; + })); + expectResponseTimerCreate(); + + Http::TestRequestHeaderMapImpl headers{{"x-envoy-retry-on", "5xx"}, {"x-envoy-internal", "true"}}; + HttpTestUtility::addDefaultHeaders(headers); + router_.decodeHeaders(headers, true); + + // Initial request has 1 attempt. + EXPECT_EQ(1, atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); + + // 5xx response. + router_.retry_state_->expectHeadersRetry(); + Http::ResponseHeaderMapPtr response_headers1( + new Http::TestResponseHeaderMapImpl{{":status", "503"}}); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(503)); + response_decoder->decodeHeaders(std::move(response_headers1), true); + EXPECT_TRUE(verifyHostUpstreamStats(0, 1)); + + // We expect the 5xx response to kick off a new request. + EXPECT_CALL(encoder1.stream_, resetStream(_)).Times(0); + NiceMock encoder2; + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)) + .WillOnce(Invoke( + [&](Http::ResponseDecoder& decoder, + Http::ConnectionPool::Callbacks& callbacks) -> Http::ConnectionPool::Cancellable* { + response_decoder = &decoder; + callbacks.onPoolReady(encoder2, cm_.conn_pool_.host_, upstream_stream_info_); + return nullptr; + })); + router_.retry_state_->callback_(); + + // The retry should cause the header to increase to 2. + EXPECT_EQ(2, atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); + + // Normal response. + EXPECT_CALL(*router_.retry_state_, shouldRetryHeaders(_, _)).WillOnce(Return(RetryStatus::No)); + EXPECT_CALL(cm_.conn_pool_.host_->health_checker_, setUnhealthy()).Times(0); + Http::ResponseHeaderMapPtr response_headers2( + new Http::TestResponseHeaderMapImpl{{":status", "200"}}); + EXPECT_CALL(cm_.conn_pool_.host_->outlier_detector_, putHttpResponseCode(200)); + response_decoder->decodeHeaders(std::move(response_headers2), true); + EXPECT_TRUE(verifyHostUpstreamStats(1, 1)); +} + // Validate that the cluster is appended to the response when configured. void RouterTestBase::testAppendCluster(absl::optional cluster_header_name) { auto debug_config = std::make_unique( From 6e5390181fabc4dcde380ed72f6a5964437ee135 Mon Sep 17 00:00:00 2001 From: Jose Nino Date: Fri, 13 Mar 2020 10:21:28 -0700 Subject: [PATCH 2/2] update tests Signed-off-by: Jose Nino --- test/common/router/router_test.cc | 82 ++++++++++++++----------------- 1 file changed, 36 insertions(+), 46 deletions(-) diff --git a/test/common/router/router_test.cc b/test/common/router/router_test.cc index da866dcbe5160..78e3cced92ba6 100644 --- a/test/common/router/router_test.cc +++ b/test/common/router/router_test.cc @@ -205,6 +205,29 @@ class RouterTestBase : public testing::Test { router_.onDestroy(); } + void verifyAttemptCountBasic(bool set_include_attempt_count, absl::optional preset_count, + int expected_count) { + setIncludeAttemptCount(set_include_attempt_count); + + EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); + expectResponseTimerCreate(); + + Http::TestRequestHeaderMapImpl headers; + HttpTestUtility::addDefaultHeaders(headers); + if (preset_count) { + headers.setEnvoyAttemptCount(preset_count.value()); + } + router_.decodeHeaders(headers, true); + + EXPECT_EQ(expected_count, + atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); + + // When the router filter gets reset we should cancel the pool request. + EXPECT_CALL(cancellable_, cancel()); + router_.onDestroy(); + EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + } + void sendRequest(bool end_stream = true) { if (end_stream) { EXPECT_CALL(callbacks_.dispatcher_, createTimer_(_)).Times(1); @@ -872,63 +895,30 @@ TEST_F(RouterTest, EnvoyUpstreamServiceTime) { EXPECT_TRUE(verifyHostUpstreamStats(1, 0)); } -// Validate that x-envoy-attempt-count is added to request headers when option is true. +// Validate that x-envoy-attempt-count is added to request headers when the option is true. TEST_F(RouterTest, EnvoyAttemptCountInRequest) { - setIncludeAttemptCount(true); - - EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); - expectResponseTimerCreate(); - - Http::TestRequestHeaderMapImpl headers; - HttpTestUtility::addDefaultHeaders(headers); - router_.decodeHeaders(headers, true); - - EXPECT_EQ(1, atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); - - // When the router filter gets reset we should cancel the pool request. - EXPECT_CALL(cancellable_, cancel()); - router_.onDestroy(); - EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + verifyAttemptCountBasic( + /* set_include_attempt_count */ true, + /* preset_count*/ absl::nullopt, + /* expected_count */ 1); } // Validate that x-envoy-attempt-count is overwritten by the router on request headers, if the // header is sent from the downstream and the option is set to true. TEST_F(RouterTest, EnvoyAttemptCountInRequestOverwritten) { - setIncludeAttemptCount(true); - - EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); - expectResponseTimerCreate(); - - Http::TestRequestHeaderMapImpl headers; - HttpTestUtility::addDefaultHeaders(headers); - headers.setEnvoyAttemptCount(123); - router_.decodeHeaders(headers, true); - - EXPECT_EQ(1, atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); - - // When the router filter gets reset we should cancel the pool request. - EXPECT_CALL(cancellable_, cancel()); - router_.onDestroy(); - EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + verifyAttemptCountBasic( + /* set_include_attempt_count */ true, + /* preset_count*/ 123, + /* expected_count */ 1); } // Validate that x-envoy-attempt-count is not overwritten by the router on request headers, if the // header is sent from the downstream and the option is set to false. TEST_F(RouterTest, EnvoyAttemptCountInRequestNotOverwritten) { - EXPECT_CALL(cm_.conn_pool_, newStream(_, _)).WillOnce(Return(&cancellable_)); - expectResponseTimerCreate(); - - Http::TestRequestHeaderMapImpl headers; - HttpTestUtility::addDefaultHeaders(headers); - headers.setEnvoyAttemptCount(123); - router_.decodeHeaders(headers, true); - - EXPECT_EQ(123, atoi(std::string(headers.EnvoyAttemptCount()->value().getStringView()).c_str())); - - // When the router filter gets reset we should cancel the pool request. - EXPECT_CALL(cancellable_, cancel()); - router_.onDestroy(); - EXPECT_TRUE(verifyHostUpstreamStats(0, 0)); + verifyAttemptCountBasic( + /* set_include_attempt_count */ false, + /* preset_count*/ 123, + /* expected_count */ 123); } TEST_F(RouterTest, EnvoyAttemptCountInRequestUpdatedInRetries) {