Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
89ba76c
add max stream duration on upstream connection
Shikugawa Mar 25, 2020
df7eb75
delete unused sections
Shikugawa Mar 26, 2020
7449db5
fix
Shikugawa Mar 30, 2020
9ca82f0
fix
Shikugawa Apr 2, 2020
52ae433
resolve conflict
Shikugawa Apr 2, 2020
132ad71
fix
Shikugawa Apr 6, 2020
8d7b527
Merge branch 'master' into upstream-max-stream-duration
Shikugawa Apr 6, 2020
5e6eaa5
fix
Shikugawa Apr 7, 2020
878d8f4
fix
Shikugawa Apr 7, 2020
f4ac4be
add retry
Shikugawa Apr 11, 2020
ea73692
fix
Shikugawa Apr 11, 2020
69797fd
proto
Shikugawa Apr 11, 2020
a4215ec
build
Shikugawa Apr 13, 2020
77bdd1b
fix flag
Shikugawa Apr 13, 2020
3b06d6d
Merge branch 'master' of https://github.com/envoyproxy/envoy into ups…
Shikugawa Apr 13, 2020
b4e6725
fix memory
Shikugawa Apr 13, 2020
2f54fbc
add retry test
Shikugawa Apr 14, 2020
afd20d7
fix
Shikugawa Apr 15, 2020
dce6d90
full stream retry
Shikugawa Apr 23, 2020
ae0d2c3
resolve test failure
Shikugawa Apr 23, 2020
a6a6f89
memory
Shikugawa Apr 23, 2020
147f0ab
fix
Shikugawa Apr 24, 2020
d24662d
build
Shikugawa Apr 24, 2020
42decdd
Merge branch 'master' of https://github.com/envoyproxy/envoy into ups…
Shikugawa Apr 28, 2020
f65dd0b
Merge branch 'master' of https://github.com/envoyproxy/envoy into ups…
Shikugawa Apr 29, 2020
cf9dae4
connect termination
Shikugawa Apr 29, 2020
e68d9af
Merge branch 'master' of https://github.com/envoyproxy/envoy into ups…
Shikugawa Apr 30, 2020
fb07faf
Merge branch 'master' of https://github.com/envoyproxy/envoy into ups…
Shikugawa Apr 30, 2020
382221e
fix
Shikugawa Apr 30, 2020
dc0c318
fix
Shikugawa May 1, 2020
1e475e2
fix
Shikugawa May 1, 2020
d75836c
Merge branch 'master' of https://github.com/envoyproxy/envoy into ups…
Shikugawa May 4, 2020
db8b989
fix
Shikugawa May 4, 2020
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
2 changes: 0 additions & 2 deletions api/envoy/api/v2/core/protocol.proto
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,6 @@ message HttpProtocolOptions {

// Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be
// reset independent of any other timeouts. If not specified, this value is not set.
// The current implementation implements this timeout on downstream connections only.
Comment thread
mattklein123 marked this conversation as resolved.
// [#comment:TODO(shikugawa): add this functionality to upstream.]
google.protobuf.Duration max_stream_duration = 4;
}

Expand Down
2 changes: 0 additions & 2 deletions api/envoy/config/core/v3/protocol.proto
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,6 @@ message HttpProtocolOptions {

// Total duration to keep alive an HTTP request/response stream. If the time limit is reached the stream will be
// reset independent of any other timeouts. If not specified, this value is not set.
// The current implementation implements this timeout on downstream connections only.
// [#comment:TODO(shikugawa): add this functionality to upstream.]
google.protobuf.Duration max_stream_duration = 4;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ Every cluster has a statistics tree rooted at *cluster.<name>.* with the followi
upstream_rq_cancelled, Counter, Total requests cancelled before obtaining a connection pool connection
upstream_rq_maintenance_mode, Counter, Total requests that resulted in an immediate 503 due to :ref:`maintenance mode<config_http_filters_router_runtime_maintenance_mode>`
upstream_rq_timeout, Counter, Total requests that timed out waiting for a response
upstream_rq_max_duration_reached, Counter, Total requests closed due to max duration reached
upstream_rq_per_try_timeout, Counter, Total requests that hit the per try timeout
upstream_rq_rx_reset, Counter, Total requests that were reset remotely
upstream_rq_tx_reset, Counter, Total requests that were reset locally
Expand Down
2 changes: 0 additions & 2 deletions generated_api_shadow/envoy/api/v2/core/protocol.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 0 additions & 2 deletions generated_api_shadow/envoy/config/core/v3/protocol.proto

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions include/envoy/upstream/upstream.h
Original file line number Diff line number Diff line change
Expand Up @@ -579,6 +579,7 @@ class PrioritySet {
COUNTER(upstream_rq_retry_success) \
COUNTER(upstream_rq_rx_reset) \
COUNTER(upstream_rq_timeout) \
COUNTER(upstream_rq_max_duration_reached) \
Comment thread
Shikugawa marked this conversation as resolved.
Outdated
COUNTER(upstream_rq_total) \
COUNTER(upstream_rq_tx_reset) \
GAUGE(lb_subsets_active, Accumulate) \
Expand Down Expand Up @@ -698,6 +699,11 @@ class ClusterInfo {
*/
virtual std::chrono::milliseconds connectTimeout() const PURE;

/**
* @return maximum duration time to keep alive stream
*/
virtual absl::optional<std::chrono::milliseconds> maxStreamDuration() const PURE;
Comment thread
Shikugawa marked this conversation as resolved.
Outdated

/**
* @return the idle timeout for upstream connection pool connections.
*/
Expand Down
2 changes: 1 addition & 1 deletion source/common/http/codec_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ namespace Http {
CodecClient::CodecClient(Type type, Network::ClientConnectionPtr&& connection,
Upstream::HostDescriptionConstSharedPtr host,
Event::Dispatcher& dispatcher)
: type_(type), connection_(std::move(connection)), host_(host),
: dispatcher_(dispatcher), type_(type), connection_(std::move(connection)), host_(host),
idle_timeout_(host_->cluster().idleTimeout()) {
if (type_ != Type::HTTP3) {
// Make sure upstream connections process data and then the FIN, rather than processing
Expand Down
20 changes: 19 additions & 1 deletion source/common/http/codec_client.h
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,

const StreamInfo::StreamInfo& streamInfo() { return connection_->streamInfo(); }

Event::Dispatcher& dispatcher_;
Comment thread
Shikugawa marked this conversation as resolved.
Outdated

protected:
/**
* Create a codec client and connect to a remote host/port.
Expand Down Expand Up @@ -188,7 +190,14 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
public StreamCallbacks,
public ResponseDecoderWrapper {
ActiveRequest(CodecClient& parent, ResponseDecoder& inner)
: ResponseDecoderWrapper(inner), parent_(parent) {}
: ResponseDecoderWrapper(inner), parent_(parent),
max_stream_duration_(parent_.host_->cluster().maxStreamDuration()) {
if (max_stream_duration_.has_value()) {
max_stream_duration_timer_ =
parent.dispatcher_.createTimer([this]() -> void { onStreamMaxDurationReached(); });
max_stream_duration_timer_->enableTimer(max_stream_duration_.value());
}
}

// StreamCallbacks
void onResetStream(StreamResetReason reason, absl::string_view) override {
Expand All @@ -201,8 +210,17 @@ class CodecClient : Logger::Loggable<Logger::Id::client>,
void onPreDecodeComplete() override { parent_.responseDecodeComplete(*this); }
void onDecodeComplete() override {}

void onStreamMaxDurationReached() {
parent_.host_->cluster().stats().upstream_rq_max_duration_reached_.inc();
max_stream_duration_timer_->disableTimer();
Comment thread
Shikugawa marked this conversation as resolved.
Outdated
max_stream_duration_timer_.reset();
onResetStream(StreamResetReason::ConnectionTermination, "");
}

RequestEncoder* encoder_{};
CodecClient& parent_;
Event::TimerPtr max_stream_duration_timer_;
const absl::optional<std::chrono::milliseconds> max_stream_duration_;
};

using ActiveRequestPtr = std::unique_ptr<ActiveRequest>;
Expand Down
2 changes: 2 additions & 0 deletions source/common/upstream/upstream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,8 @@ ClusterInfoImpl::ClusterInfoImpl(
Http::DEFAULT_MAX_HEADERS_COUNT))),
connect_timeout_(
std::chrono::milliseconds(PROTOBUF_GET_MS_REQUIRED(config, connect_timeout))),
max_stream_duration_(
PROTOBUF_GET_OPTIONAL_MS(config.common_http_protocol_options(), max_stream_duration)),
per_connection_buffer_limit_bytes_(
PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, per_connection_buffer_limit_bytes, 1024 * 1024)),
socket_matcher_(std::move(socket_matcher)), stats_scope_(std::move(stats_scope)),
Expand Down
4 changes: 4 additions & 0 deletions source/common/upstream/upstream_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -527,6 +527,9 @@ class ClusterInfoImpl : public ClusterInfo, protected Logger::Loggable<Logger::I
return common_lb_config_;
}
std::chrono::milliseconds connectTimeout() const override { return connect_timeout_; }
absl::optional<std::chrono::milliseconds> maxStreamDuration() const override {
return max_stream_duration_;
}
const absl::optional<std::chrono::milliseconds> idleTimeout() const override {
return idle_timeout_;
}
Expand Down Expand Up @@ -614,6 +617,7 @@ class ClusterInfoImpl : public ClusterInfo, protected Logger::Loggable<Logger::I
const uint64_t max_requests_per_connection_;
const uint32_t max_response_headers_count_;
const std::chrono::milliseconds connect_timeout_;
const absl::optional<std::chrono::milliseconds> max_stream_duration_;
absl::optional<std::chrono::milliseconds> idle_timeout_;
const uint32_t per_connection_buffer_limit_bytes_;
TransportSocketMatcherPtr socket_matcher_;
Expand Down
56 changes: 45 additions & 11 deletions test/common/http/codec_client_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@ namespace Envoy {
namespace Http {
namespace {

class CodecClientTest : public testing::Test {
template <class ClusterInfoType> class CodecClientTestBase : public testing::Test {
public:
CodecClientTest() {
CodecClientTestBase() {
connection_ = new NiceMock<Network::MockClientConnection>();

EXPECT_CALL(*connection_, detectEarlyCloseWhenReadDisabled(false));
Expand All @@ -52,29 +52,33 @@ class CodecClientTest : public testing::Test {
Invoke([this](Network::ReadFilterSharedPtr filter) -> void { filter_ = filter; }));

codec_ = new Http::MockClientConnection();

Network::ClientConnectionPtr connection{connection_};
EXPECT_CALL(dispatcher_, createTimer_(_));
client_ = std::make_unique<CodecClientForTest>(CodecClient::Type::HTTP1, std::move(connection),
codec_, nullptr, host_, dispatcher_);
ON_CALL(*connection_, streamInfo()).WillByDefault(ReturnRef(stream_info_));
}

~CodecClientTest() override { EXPECT_EQ(0U, client_->numActiveRequests()); }
~CodecClientTestBase() override { EXPECT_EQ(0U, client_->numActiveRequests()); }

Event::MockDispatcher dispatcher_;
Network::MockClientConnection* connection_;
Http::MockClientConnection* codec_;
std::unique_ptr<CodecClientForTest> client_;
Network::ConnectionCallbacks* connection_cb_;
Network::ReadFilterSharedPtr filter_;
std::shared_ptr<Upstream::MockIdleTimeEnabledClusterInfo> cluster_{
new NiceMock<Upstream::MockIdleTimeEnabledClusterInfo>()};
std::shared_ptr<ClusterInfoType> cluster_{new NiceMock<ClusterInfoType>()};
Upstream::HostDescriptionConstSharedPtr host_{
Upstream::makeTestHostDescription(cluster_, "tcp://127.0.0.1:80")};
NiceMock<StreamInfo::MockStreamInfo> stream_info_;
};

class CodecClientTest : public CodecClientTestBase<Upstream::MockIdleTimeEnabledClusterInfo> {
public:
CodecClientTest() : CodecClientTestBase<Upstream::MockIdleTimeEnabledClusterInfo>() {
Network::ClientConnectionPtr connection{connection_};
EXPECT_CALL(dispatcher_, createTimer_(_));
client_ = std::make_unique<CodecClientForTest>(CodecClient::Type::HTTP1, std::move(connection),
codec_, nullptr, host_, dispatcher_);
ON_CALL(*connection_, streamInfo()).WillByDefault(ReturnRef(stream_info_));
}
};

TEST_F(CodecClientTest, NotCallDetectEarlyCloseWhenReadDiabledUsingHttp3) {
auto connection = std::make_unique<NiceMock<Network::MockClientConnection>>();

Expand Down Expand Up @@ -282,6 +286,36 @@ TEST_F(CodecClientTest, SSLConnectionInfo) {
EXPECT_EQ(session_id, stream_info_.downstreamSslConnection()->sessionId());
}

class CodecMaxStreamDurationTest
: public CodecClientTestBase<Upstream::MockMaxStreamDurationEnabledClusterInfo> {
public:
CodecMaxStreamDurationTest()
: CodecClientTestBase<Upstream::MockMaxStreamDurationEnabledClusterInfo>() {
Network::ClientConnectionPtr connection{connection_};
client_ = std::make_unique<CodecClientForTest>(CodecClient::Type::HTTP1, std::move(connection),
codec_, nullptr, host_, dispatcher_);
ON_CALL(*connection_, streamInfo()).WillByDefault(ReturnRef(stream_info_));
}
};

TEST_F(CodecMaxStreamDurationTest, Basic) {
ResponseDecoder* inner_decoder;
NiceMock<MockRequestEncoder> inner_encoder;
EXPECT_CALL(*codec_, newStream(_))
.WillOnce(Invoke([&](ResponseDecoder& decoder) -> RequestEncoder& {
inner_decoder = &decoder;
return inner_encoder;
}));

Event::MockTimer* timer = new Event::MockTimer(&dispatcher_);
Http::MockResponseDecoder outer_decoder;
EXPECT_CALL(*timer, enableTimer(cluster_->maxStreamDuration().value(), _));
client_->newStream(outer_decoder);
EXPECT_CALL(*timer, disableTimer());
timer->invokeCallback();
EXPECT_EQ(1U, cluster_->stats_.upstream_rq_max_duration_reached_.value());
}

// Test the codec getting input from a real TCP connection.
class CodecNetworkTest : public testing::TestWithParam<Network::Address::IpVersion> {
public:
Expand Down
7 changes: 7 additions & 0 deletions test/mocks/upstream/cluster_info.cc
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ MockIdleTimeEnabledClusterInfo::MockIdleTimeEnabledClusterInfo() {

MockIdleTimeEnabledClusterInfo::~MockIdleTimeEnabledClusterInfo() = default;

MockMaxStreamDurationEnabledClusterInfo::MockMaxStreamDurationEnabledClusterInfo() {
ON_CALL(*this, maxStreamDuration())
.WillByDefault(Return(absl::optional<std::chrono::milliseconds>(50)));
}

MockMaxStreamDurationEnabledClusterInfo::~MockMaxStreamDurationEnabledClusterInfo() = default;

MockClusterInfo::MockClusterInfo()
: http2_options_(::Envoy::Http2::Utility::initializeAndValidateOptions(
envoy::config::core::v3::Http2ProtocolOptions())),
Expand Down
8 changes: 8 additions & 0 deletions test/mocks/upstream/cluster_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ class MockClusterInfo : public ClusterInfo {
// Upstream::ClusterInfo
MOCK_METHOD(bool, addedViaApi, (), (const));
MOCK_METHOD(std::chrono::milliseconds, connectTimeout, (), (const));
MOCK_METHOD(absl::optional<std::chrono::milliseconds>, maxStreamDuration, (), (const));
MOCK_METHOD(const absl::optional<std::chrono::milliseconds>, idleTimeout, (), (const));
MOCK_METHOD(uint32_t, perConnectionBufferLimitBytes, (), (const));
MOCK_METHOD(uint64_t, features, (), (const));
Expand Down Expand Up @@ -158,6 +159,7 @@ class MockClusterInfo : public ClusterInfo {
envoy::config::cluster::v3::Cluster::CommonLbConfig lb_config_;
envoy::config::core::v3::Metadata metadata_;
std::unique_ptr<Envoy::Config::TypedMetadata> typed_metadata_;
absl::optional<std::chrono::milliseconds> max_stream_duration_;
};

class MockIdleTimeEnabledClusterInfo : public MockClusterInfo {
Expand All @@ -166,5 +168,11 @@ class MockIdleTimeEnabledClusterInfo : public MockClusterInfo {
~MockIdleTimeEnabledClusterInfo() override;
};

class MockMaxStreamDurationEnabledClusterInfo : public MockClusterInfo {
public:
MockMaxStreamDurationEnabledClusterInfo();
~MockMaxStreamDurationEnabledClusterInfo() override;
};

} // namespace Upstream
} // namespace Envoy