Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

honour routes timeout if max stream duration is set with out stream duration #15585

Merged
merged 15 commits into from
Apr 8, 2021
Merged

honour routes timeout if max stream duration is set with out stream duration #15585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
8228faa
fix timeout
ramaraochavali Mar 20, 2021
92d3a18
Merge remote-tracking branch 'upstream/main' into fix/timeout
ramaraochavali Mar 21, 2021
e68a3cf
move to refreshduration
ramaraochavali Mar 24, 2021
e5d8667
remove unnecessary couts
ramaraochavali Mar 24, 2021
fe417ff
remove unnecessary remove
ramaraochavali Mar 24, 2021
48afbe0
kick ci
ramaraochavali Mar 24, 2021
43642eb
Merge remote-tracking branch 'upstream/main' into fix/timeout
ramaraochavali Mar 24, 2021
9f7e3ca
Merge remote-tracking branch 'upstream/main' into fix/timeout
ramaraochavali Mar 25, 2021
c1be6ae
wip fix
ramaraochavali Mar 30, 2021
9a02bf9
fix properly and add test
ramaraochavali Apr 2, 2021
4f894bb
comment
ramaraochavali Apr 2, 2021
30630fc
remove unnecessary removal
ramaraochavali Apr 2, 2021
b7e2b4d
format
ramaraochavali Apr 2, 2021
d621a4b
fix
ramaraochavali Apr 2, 2021
0314715
unnecessary removal
ramaraochavali Apr 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions source/common/http/conn_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,6 +1192,9 @@ 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();
alyssawilk marked this conversation as resolved.
Show resolved Hide resolved
} else {
disable_timer = true;
}
Expand Down
96 changes: 95 additions & 1 deletion test/common/http/conn_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1894,7 +1894,10 @@ 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 @@ -1919,6 +1922,8 @@ 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 @@ -2062,6 +2067,8 @@ 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 @@ -2236,12 +2243,73 @@ 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 @@ -2272,6 +2340,8 @@ 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 @@ -2301,6 +2371,8 @@ 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 @@ -2342,6 +2414,8 @@ 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 @@ -2372,6 +2446,8 @@ 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 @@ -2416,6 +2492,8 @@ 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 @@ -2465,6 +2543,8 @@ 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 @@ -2616,6 +2696,8 @@ 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 @@ -2643,6 +2725,8 @@ 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 @@ -2669,6 +2753,8 @@ 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 @@ -2696,6 +2782,8 @@ 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 @@ -2725,6 +2813,8 @@ 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 @@ -2760,6 +2850,8 @@ 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 @@ -2786,6 +2878,8 @@ 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: 4 additions & 0 deletions test/common/http/conn_manager_impl_test_2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -272,6 +272,8 @@ 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 @@ -350,6 +352,8 @@ 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