Skip to content

Commit

Permalink
Revert "honour routes timeout if max stream duration is set with out …
Browse files Browse the repository at this point in the history 
…stream duration (envoyproxy#15585)"

This reverts commit ffa1680.

Signed-off-by: Snow Pettersen <[email protected]>
  • Loading branch information
@snowp
snowp committed Apr 22, 2021
1 parent 048f3b8 commit 8d7355d
Show file tree
Hide file tree
Showing 3 changed files with 1 addition and 102 deletions.
3 changes: 0 additions & 3 deletions source/common/http/conn_manager_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1190,9 +1190,6 @@ void ConnectionManagerImpl::ActiveStream::refreshDurationTimeout() {
const auto max_stream_duration = connection_manager_.config_.maxStreamDuration();
if (max_stream_duration.has_value() && max_stream_duration.value().count()) {
timeout = max_stream_duration.value();
} else if (route->timeout().count() != 0) {
// If max stream duration is not set either at route/HCM level, use the route timeout.
timeout = route->timeout();
} else {
disable_timer = true;
}
Expand Down
96 changes: 1 addition & 95 deletions test/common/http/conn_manager_impl_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2191,10 +2191,7 @@ TEST_F(HttpConnectionManagerImplTest, NoPath) {
// the default configuration aspects.
TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutNotConfigured) {
setup(false, "");
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));

EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, createTimer_(_)).Times(0);
EXPECT_CALL(*codec_, dispatch(_))
.WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status {
Expand All @@ -2219,8 +2216,6 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutNotConfigured) {
// headers, if it fires we don't faceplant.
TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutGlobal) {
stream_idle_timeout_ = std::chrono::milliseconds(10);
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
setup(false, "");

EXPECT_CALL(*codec_, dispatch(_)).WillRepeatedly(Invoke([&](Buffer::Instance&) -> Http::Status {
Expand Down Expand Up @@ -2364,8 +2359,6 @@ TEST_F(HttpConnectionManagerImplTest, DurationTimeout) {
// Create the stream.
EXPECT_CALL(*codec_, dispatch(_))
.WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status {
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
Event::MockTimer* idle_timer = setUpTimer();
EXPECT_CALL(*idle_timer, enableTimer(_, _));
RequestDecoder* decoder = &conn_manager_->newStream(response_encoder_);
Expand Down Expand Up @@ -2553,73 +2546,12 @@ TEST_F(HttpConnectionManagerImplTest, DurationTimeout) {
filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
}

// Test that verifies route timeout is used if if grpc timeout header is not set and
// max stream duration is not at route and HCM level.
// Regression test for https://github.com/envoyproxy/envoy/issues/15530.
// TODO(ramaraochavali): merge this with DurationTimeout test.
TEST_F(HttpConnectionManagerImplTest, DurationTimeoutUsesRouteTimeout) {
stream_idle_timeout_ = std::chrono::milliseconds(10);
setup(false, "");
setupFilterChain(1, 0);
RequestHeaderMap* latched_headers = nullptr;

EXPECT_CALL(*decoder_filters_[0], decodeHeaders(_, false))
.WillOnce(Return(FilterHeadersStatus::StopIteration));

// Create the stream.
EXPECT_CALL(*codec_, dispatch(_))
.WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status {
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
Event::MockTimer* idle_timer = setUpTimer();
EXPECT_CALL(*idle_timer, enableTimer(_, _));
RequestDecoder* decoder = &conn_manager_->newStream(response_encoder_);
EXPECT_CALL(*idle_timer, enableTimer(_, _));
EXPECT_CALL(*idle_timer, disableTimer());
RequestHeaderMapPtr headers{new TestRequestHeaderMapImpl{
{":authority", "host"}, {":path", "/"}, {":method", "GET"}}};
latched_headers = headers.get();
decoder->decodeHeaders(std::move(headers), false);

data.drain(4);
return Http::okStatus();
}));
Buffer::OwnedImpl fake_input("1234");
conn_manager_->onData(fake_input, false);

// Clear and refresh the route cache (checking clusterInfo refreshes the route cache)
decoder_filters_[0]->callbacks_->clearRouteCache();
decoder_filters_[0]->callbacks_->clusterInfo();

Event::MockTimer* timer = setUpTimer();

// With no max stream duration, route timeout is used.
max_stream_duration_ = absl::nullopt;
EXPECT_CALL(*timer, enableTimer(std::chrono::milliseconds(22), _));
EXPECT_CALL(route_config_provider_.route_config_->route_->route_entry_, maxStreamDuration())
.Times(1)
.WillRepeatedly(Return(absl::nullopt));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(22)));
decoder_filters_[0]->callbacks_->clearRouteCache();
decoder_filters_[0]->callbacks_->clusterInfo();
max_stream_duration_ = absl::nullopt;

// Cleanup.
EXPECT_CALL(*timer, disableTimer());
EXPECT_CALL(*decoder_filters_[0], onStreamComplete());
EXPECT_CALL(*decoder_filters_[0], onDestroy());
filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
}

// Per-route timeouts override the global stream idle timeout.
TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutRouteOverride) {
stream_idle_timeout_ = std::chrono::milliseconds(10);
setup(false, "");
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout())
.WillByDefault(Return(std::chrono::milliseconds(30)));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));

EXPECT_CALL(*codec_, dispatch(_))
.WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status {
Expand Down Expand Up @@ -2650,8 +2582,6 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutRouteZeroOverride) {
setup(false, "");
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));

EXPECT_CALL(*codec_, dispatch(_))
.WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Http::Status {
Expand Down Expand Up @@ -2681,8 +2611,6 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterDownstreamHeaders
setup(false, "");
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout())
.WillByDefault(Return(std::chrono::milliseconds(10)));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));

// Codec sends downstream request headers.
EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status {
Expand Down Expand Up @@ -2724,8 +2652,6 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutNormalTermination) {
setup(false, "");
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout())
.WillByDefault(Return(std::chrono::milliseconds(10)));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));

// Codec sends downstream request headers.
Event::MockTimer* idle_timer = setUpTimer();
Expand Down Expand Up @@ -2756,8 +2682,6 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterDownstreamHeaders
setup(false, "");
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout())
.WillByDefault(Return(std::chrono::milliseconds(10)));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));

// Codec sends downstream request headers.
EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status {
Expand Down Expand Up @@ -2802,8 +2726,6 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterUpstreamHeaders)
setup(false, "");
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout())
.WillByDefault(Return(std::chrono::milliseconds(10)));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));

// Store the basic request encoder during filter chain setup.
std::shared_ptr<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
Expand Down Expand Up @@ -2853,8 +2775,6 @@ TEST_F(HttpConnectionManagerImplTest, PerStreamIdleTimeoutAfterBidiData) {
setup(false, "");
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, idleTimeout())
.WillByDefault(Return(std::chrono::milliseconds(10)));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
proxy_100_continue_ = true;

// Store the basic request encoder during filter chain setup.
Expand Down Expand Up @@ -3006,8 +2926,6 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutCallbackDisarmsAndReturns408

TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsNotDisarmedOnIncompleteRequestWithHeader) {
request_timeout_ = std::chrono::milliseconds(10);
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
setup(false, "");

EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status {
Expand Down Expand Up @@ -3035,8 +2953,6 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsNotDisarmedOnIncompleteReq

TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnCompleteRequestWithHeader) {
request_timeout_ = std::chrono::milliseconds(10);
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
setup(false, "");

EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status {
Expand All @@ -3063,8 +2979,6 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnCompleteRequestW

TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnCompleteRequestWithData) {
request_timeout_ = std::chrono::milliseconds(10);
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
setup(false, "");

EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status {
Expand Down Expand Up @@ -3092,8 +3006,6 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnCompleteRequestW

TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnCompleteRequestWithTrailers) {
request_timeout_ = std::chrono::milliseconds(10);
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
setup(false, "");

EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> Http::Status {
Expand Down Expand Up @@ -3123,8 +3035,6 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnCompleteRequestW

TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnEncodeHeaders) {
request_timeout_ = std::chrono::milliseconds(10);
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
setup(false, "");
std::shared_ptr<MockStreamDecoderFilter> filter(new NiceMock<MockStreamDecoderFilter>());
EXPECT_CALL(filter_factory_, createFilterChain(_))
Expand Down Expand Up @@ -3160,8 +3070,6 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnEncodeHeaders) {

TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnConnectionTermination) {
request_timeout_ = std::chrono::milliseconds(10);
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
setup(false, "");

Event::MockTimer* request_timer = setUpTimer();
Expand All @@ -3188,8 +3096,6 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnConnectionTermin

TEST_F(HttpConnectionManagerImplTest, RequestHeaderTimeoutDisarmedAfterHeaders) {
request_headers_timeout_ = std::chrono::milliseconds(10);
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
setup(false, "");

Event::MockTimer* request_header_timer;
Expand Down
4 changes: 0 additions & 4 deletions test/common/http/conn_manager_impl_test_2.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -272,8 +272,6 @@ TEST_F(HttpConnectionManagerImplTest, IdleTimeoutNoCodec) {

TEST_F(HttpConnectionManagerImplTest, IdleTimeout) {
idle_timeout_ = (std::chrono::milliseconds(10));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
Event::MockTimer* idle_timer = setUpTimer();
EXPECT_CALL(*idle_timer, enableTimer(_, _));
setup(false, "");
Expand Down Expand Up @@ -352,8 +350,6 @@ TEST_F(HttpConnectionManagerImplTest, ConnectionDurationNoCodec) {

TEST_F(HttpConnectionManagerImplTest, ConnectionDuration) {
max_connection_duration_ = (std::chrono::milliseconds(10));
ON_CALL(route_config_provider_.route_config_->route_->route_entry_, timeout())
.WillByDefault(Return(std::chrono::milliseconds(0)));
Event::MockTimer* connection_duration_timer = setUpTimer();
EXPECT_CALL(*connection_duration_timer, enableTimer(_, _));
setup(false, "");
Expand Down

0 comments on commit 8d7355d

Please sign in to comment.