Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
ba55e0a
Add request headers timeout to HCM
akonradi Sep 30, 2020
256924b
Merge remote-tracking branch 'upstream/master' into http-request-head…
akonradi Sep 30, 2020
97de0f2
Update documentation, fix formatting
akonradi Oct 12, 2020
a1e78ee
Merge remote-tracking branch 'upstream/master' into http-request-head…
akonradi Oct 12, 2020
773ded2
Use unique_ptr::reset() instead of disabling
akonradi Oct 12, 2020
f780c24
Address review feedback
akonradi Oct 12, 2020
0735254
Revert "Use unique_ptr::reset() instead of disabling"
akonradi Oct 13, 2020
068dbde
Merge remote-tracking branch 'upstream/master' into http-request-head…
akonradi Oct 13, 2020
bd8b3c8
Address review feedback
akonradi Oct 19, 2020
06e150f
Add validation annotation
akonradi Oct 19, 2020
99ec46b
Revert to sendLocalReply
akonradi Oct 23, 2020
df9c705
Add integration test
akonradi Oct 23, 2020
7924ff3
Merge remote-tracking branch 'upstream/master' into http-request-head…
akonradi Oct 23, 2020
9e65d85
Fix protodoc_manifest.yaml
akonradi Oct 23, 2020
a28a251
Fix formatting
akonradi Oct 23, 2020
991e437
Merge remote-tracking branch 'upstream/master' into http-request-head…
akonradi Oct 27, 2020
39dc1be
Don't reset request header timer on timeout
akonradi Oct 27, 2020
ccd2599
Address feedback
akonradi Oct 28, 2020
f478d1f
Re-add reset() after headers complete
akonradi Oct 28, 2020
97e07fd
Use RawConnectionSocket utility for test
akonradi Oct 30, 2020
e1411ff
Merge remote-tracking branch 'upstream/master' into http-request-head…
akonradi Oct 30, 2020
576f43b
Check message, fix stream info
akonradi Nov 4, 2020
b0d5025
Add header timeout to timeouts.rst
akonradi Nov 6, 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
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE;
// HTTP connection manager :ref:`configuration overview <config_http_conn_man>`.
// [#extension: envoy.filters.network.http_connection_manager]

// [#next-free-field: 41]
// [#next-free-field: 42]
message HttpConnectionManager {
option (udpa.annotations.versioning).previous_message_type =
"envoy.config.filter.network.http_connection_manager.v2.HttpConnectionManager";
Expand Down Expand Up @@ -360,6 +360,15 @@ message HttpConnectionManager {
google.protobuf.Duration request_timeout = 28
[(udpa.annotations.security).configure_for_untrusted_downstream = true];

// The amount of time that Envoy will wait for the request headers to be received. The timer is
// activated when the first byte of the headers is received, and is disarmed when the last byte of
// the headers has been received (i.e. all decoding filters have processed the headers). If not

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the info in parens is not accurate. Maybe just remove?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

// specified or set to 0, this timeout is disabled.
google.protobuf.Duration request_headers_timeout = 41 [
(validate.rules).duration = {gte {}},
(udpa.annotations.security).configure_for_untrusted_downstream = true
];

// The time that Envoy will wait between sending an HTTP/2 “shutdown
// notification” (GOAWAY frame with max stream ID) and a final GOAWAY frame.
// This is used so that Envoy provides a grace period for new streams that
Expand Down

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

3 changes: 3 additions & 0 deletions docs/protodoc_manifest.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,9 @@ fields:
edge_config:
example: 300s # 5 mins
envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.request_timeout:
edge_config:
example: 10s # 10 seconds
Comment thread
antoniovicente marked this conversation as resolved.
envoy.extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.request_headers_timeout:
edge_config:
note: >
This timeout is not compatible with streaming requests.
Expand Down
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ New Features
------------
* grpc: implemented header value syntax support when defining :ref:`initial metadata <envoy_v3_api_field_config.core.v3.GrpcService.initial_metadata>` for gRPC-based `ext_authz` :ref:`HTTP <envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.grpc_service>` and :ref:`network <envoy_v3_api_field_extensions.filters.network.ext_authz.v3.ExtAuthz.grpc_service>` filters, and :ref:`ratelimit <envoy_v3_api_field_config.ratelimit.v3.RateLimitServiceConfig.grpc_service>` filters.
* health_check: added option to use :ref:`no_traffic_healthy_interval <envoy_v3_api_field_config.core.v3.HealthCheck.no_traffic_healthy_interval>` which allows a different no traffic interval when the host is healthy.
* http: added HCM :ref:`timeout config field <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.request_headers_timeout>` to control how long a downstream has to finish sending headers before the stream is cancelled.
Comment thread
alyssawilk marked this conversation as resolved.
* mongo_proxy: the list of commands to produce metrics for is now :ref:`configurable <envoy_v3_api_field_extensions.filters.network.mongo_proxy.v3.MongoProxy.commands>`.

Deprecated
Expand Down

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

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

2 changes: 2 additions & 0 deletions include/envoy/stream_info/stream_info.h
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,8 @@ struct ResponseCodeDetailValues {
const std::string DownstreamLocalDisconnect = "downstream_local_disconnect";
// The max connection duration was exceeded.
const std::string DurationTimeout = "duration_timeout";
// The max request downstream header duration was exceeded.
const std::string DownstreamHeaderTimeout = "downstream_header_timeout";
// The response was generated by the admin filter.
const std::string AdminFilterResponse = "admin_filter_response";
// The original stream was replaced with an internal redirect.
Expand Down
7 changes: 7 additions & 0 deletions source/common/http/conn_manager_config.h
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,7 @@ namespace Http {
COUNTER(downstream_rq_response_before_rq_complete) \
COUNTER(downstream_rq_rx_reset) \
COUNTER(downstream_rq_timeout) \
COUNTER(downstream_rq_header_timeout) \
COUNTER(downstream_rq_too_large) \
COUNTER(downstream_rq_total) \
COUNTER(downstream_rq_tx_reset) \
Expand Down Expand Up @@ -289,6 +290,12 @@ class ConnectionManagerConfig {
*/
virtual std::chrono::milliseconds requestTimeout() const PURE;

/**
* @return request header timeout for incoming connection manager connections. Zero indicates a
* disabled request header timeout.
*/
virtual std::chrono::milliseconds requestHeadersTimeout() const PURE;

/**
* @return delayed close timeout for downstream HTTP connections. Zero indicates a disabled
* timeout. See http_connection_manager.proto for a detailed description of this timeout.
Expand Down
28 changes: 26 additions & 2 deletions source/common/http/conn_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,10 @@ void ConnectionManagerImpl::doDeferredStreamDestroy(ActiveStream& stream) {
stream.stream_idle_timer_ = nullptr;
}
stream.filter_manager_.disarmRequestTimeout();
if (stream.request_header_timer_ != nullptr) {
stream.request_header_timer_->disableTimer();
stream.request_header_timer_ = nullptr;
}

stream.completeRequest();
stream.filter_manager_.onStreamComplete();
Expand Down Expand Up @@ -614,10 +618,19 @@ ConnectionManagerImpl::ActiveStream::ActiveStream(ConnectionManagerImpl& connect
}

if (connection_manager_.config_.requestTimeout().count()) {
std::chrono::milliseconds request_timeout_ms_ = connection_manager_.config_.requestTimeout();
std::chrono::milliseconds request_timeout = connection_manager_.config_.requestTimeout();
request_timer_ = connection_manager.read_callbacks_->connection().dispatcher().createTimer(
[this]() -> void { onRequestTimeout(); });
request_timer_->enableTimer(request_timeout_ms_, this);
request_timer_->enableTimer(request_timeout, this);
}

if (connection_manager_.config_.requestHeadersTimeout().count()) {
std::chrono::milliseconds request_headers_timeout =
connection_manager_.config_.requestHeadersTimeout();
request_header_timer_ =
connection_manager.read_callbacks_->connection().dispatcher().createTimer(
[this]() -> void { onRequestHeaderTimeout(); });
request_header_timer_->enableTimer(request_headers_timeout, this);
}

const auto max_stream_duration = connection_manager_.config_.maxStreamDuration();
Expand Down Expand Up @@ -711,6 +724,13 @@ void ConnectionManagerImpl::ActiveStream::onRequestTimeout() {
StreamInfo::ResponseCodeDetails::get().RequestOverallTimeout);
}

void ConnectionManagerImpl::ActiveStream::onRequestHeaderTimeout() {
connection_manager_.stats_.named_.downstream_rq_header_timeout_.inc();
filter_manager_.streamInfo().setResponseCodeDetails(
StreamInfo::ResponseCodeDetails::get().DownstreamHeaderTimeout);
connection_manager_.doEndStream(*this);
}

void ConnectionManagerImpl::ActiveStream::onStreamMaxDurationReached() {
ENVOY_STREAM_LOG(debug, "Stream max duration time reached", *this);
connection_manager_.stats_.named_.downstream_rq_max_duration_reached_.inc();
Expand Down Expand Up @@ -794,6 +814,10 @@ void ConnectionManagerImpl::ActiveStream::decodeHeaders(RequestHeaderMapPtr&& he
connection_manager_.read_callbacks_->connection().dispatcher());
request_headers_ = std::move(headers);
filter_manager_.requestHeadersInitialized();
if (request_header_timer_ != nullptr) {
request_header_timer_->disableTimer();
request_header_timer_.reset();
}
Comment thread
antoniovicente marked this conversation as resolved.

Upstream::HostDescriptionConstSharedPtr upstream_host =
connection_manager_.read_callbacks_->upstreamHost();
Expand Down
15 changes: 12 additions & 3 deletions source/common/http/conn_manager_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,8 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
void onIdleTimeout();
// Per-stream request timeout callback.
void onRequestTimeout();
// Per-stream request header timeout callback.
void onRequestHeaderTimeout();
// Per-stream alive duration reached.
void onStreamMaxDurationReached();
bool hasCachedRoute() { return cached_route_.has_value() && cached_route_.value(); }
Expand Down Expand Up @@ -354,11 +356,18 @@ class ConnectionManagerImpl : Logger::Loggable<Logger::Id::http>,
Tracing::SpanPtr active_span_;
ResponseEncoder* response_encoder_{};
Stats::TimespanPtr request_response_timespan_;
// Per-stream idle timeout.
// Per-stream idle timeout. This timer gets reset whenever activity occurs on the stream, and,
// when triggered, will close the stream.
Event::TimerPtr stream_idle_timer_;
// Per-stream request timeout.
// Per-stream request timeout. This timer is enabled when the stream is created and disabled
// when the downstream closes the connection. If triggered, it will close the stream.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there are some errors in this comment. This seems to be a stream timeout, but there are references to downstream closing the connection.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, fixed.

Event::TimerPtr request_timer_;
// Per-stream alive duration.
// Per-stream request header timeout. This timer is enabled when the stream is created and
// disabled when the downstream finishes sending headers. If triggered, it will close the
// stream.
Event::TimerPtr request_header_timer_;
// Per-stream alive duration. This timer is enabled once when the stream is created and, if
// triggered, will close the stream.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

optional, think it's worth mentioning which of these try to send a reply?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to punt on this since whether or not a reply is sent for some of these is dependent on a runtime override.

Event::TimerPtr max_stream_duration_timer_;
std::chrono::milliseconds idle_timeout_ms_{};
State state_;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,8 @@ HttpConnectionManagerConfig::HttpConnectionManagerConfig(
stream_idle_timeout_(
PROTOBUF_GET_MS_OR_DEFAULT(config, stream_idle_timeout, StreamIdleTimeoutMs)),
request_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, request_timeout, RequestTimeoutMs)),
request_headers_timeout_(
PROTOBUF_GET_MS_OR_DEFAULT(config, request_headers_timeout, RequestHeaderTimeoutMs)),
drain_timeout_(PROTOBUF_GET_MS_OR_DEFAULT(config, drain_timeout, 5000)),
generate_request_id_(PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, generate_request_id, true)),
preserve_external_request_id_(config.preserve_external_request_id()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,9 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
}
std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }
std::chrono::milliseconds requestTimeout() const override { return request_timeout_; }
std::chrono::milliseconds requestHeadersTimeout() const override {
return request_headers_timeout_;
}
absl::optional<std::chrono::milliseconds> maxStreamDuration() const override {
return max_stream_duration_;
}
Expand Down Expand Up @@ -233,6 +236,7 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
absl::optional<std::chrono::milliseconds> max_stream_duration_;
std::chrono::milliseconds stream_idle_timeout_;
std::chrono::milliseconds request_timeout_;
std::chrono::milliseconds request_headers_timeout_;
Router::RouteConfigProviderSharedPtr route_config_provider_;
Config::ConfigProviderPtr scoped_routes_config_provider_;
std::chrono::milliseconds drain_timeout_;
Expand All @@ -255,6 +259,8 @@ class HttpConnectionManagerConfig : Logger::Loggable<Logger::Id::config>,
static const uint64_t StreamIdleTimeoutMs = 5 * 60 * 1000;
// request timeout is disabled by default
static const uint64_t RequestTimeoutMs = 0;
// request header timeout is disabled by default
static const uint64_t RequestHeaderTimeoutMs = 0;
};

/**
Expand Down
1 change: 1 addition & 0 deletions source/server/admin/admin.h
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ class AdminImpl : public Admin,
uint32_t maxRequestHeadersCount() const override { return max_request_headers_count_; }
std::chrono::milliseconds streamIdleTimeout() const override { return {}; }
std::chrono::milliseconds requestTimeout() const override { return {}; }
std::chrono::milliseconds requestHeadersTimeout() const override { return {}; }
std::chrono::milliseconds delayedCloseTimeout() const override { return {}; }
absl::optional<std::chrono::milliseconds> maxStreamDuration() const override {
return max_stream_duration_;
Expand Down
4 changes: 4 additions & 0 deletions test/common/http/conn_manager_impl_fuzz_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -154,6 +154,9 @@ class FuzzConfig : public ConnectionManagerConfig {
}
std::chrono::milliseconds streamIdleTimeout() const override { return stream_idle_timeout_; }
std::chrono::milliseconds requestTimeout() const override { return request_timeout_; }
std::chrono::milliseconds requestHeadersTimeout() const override {
return request_headers_timeout_;
}
std::chrono::milliseconds delayedCloseTimeout() const override { return delayed_close_timeout_; }
Router::RouteConfigProvider* routeConfigProvider() override {
if (use_srds_) {
Expand Down Expand Up @@ -232,6 +235,7 @@ class FuzzConfig : public ConnectionManagerConfig {
absl::optional<std::chrono::milliseconds> max_stream_duration_;
std::chrono::milliseconds stream_idle_timeout_{};
std::chrono::milliseconds request_timeout_{};
std::chrono::milliseconds request_headers_timeout_{};
std::chrono::milliseconds delayed_close_timeout_{};
bool use_remote_address_{true};
Http::ForwardClientCertType forward_client_cert_;
Expand Down
62 changes: 62 additions & 0 deletions test/common/http/conn_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2835,6 +2835,68 @@ TEST_F(HttpConnectionManagerImplTest, RequestTimeoutIsDisarmedOnConnectionTermin
filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
}

TEST_F(HttpConnectionManagerImplTest, RequestHeaderTimeoutDisarmedAfterHeaders) {
request_headers_timeout_ = std::chrono::milliseconds(10);
setup(false, "");

Event::MockTimer* request_header_timer;
EXPECT_CALL(*codec_, dispatch(_))
.WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status {
request_header_timer = setUpTimer();
EXPECT_CALL(*request_header_timer, enableTimer(request_headers_timeout_, _));

decoder_ = &conn_manager_->newStream(response_encoder_);
return Http::okStatus();
}))
.WillOnce(Return(Http::okStatus()))
.WillOnce([&](Buffer::Instance&) {
RequestHeaderMapPtr headers{new TestRequestHeaderMapImpl{
{":authority", "localhost:8080"}, {":path", "/"}, {":method", "GET"}}};

EXPECT_CALL(*request_header_timer, disableTimer).Times(1);
decoder_->decodeHeaders(std::move(headers), false);
return Http::okStatus();
});

Buffer::OwnedImpl first_line("GET /HTTP/1.1\r\n");
Buffer::OwnedImpl second_line("Host: localhost:8080\r\n");
Buffer::OwnedImpl empty_line("\r\n");
conn_manager_->onData(first_line, false);
EXPECT_TRUE(request_header_timer->enabled_);
conn_manager_->onData(second_line, false);
EXPECT_TRUE(request_header_timer->enabled_);
conn_manager_->onData(empty_line, false);
EXPECT_FALSE(request_header_timer->enabled_);

expectOnDestroy();
EXPECT_CALL(*request_header_timer, disableTimer).Times(1);
filter_callbacks_.connection_.raiseEvent(Network::ConnectionEvent::RemoteClose);
}

TEST_F(HttpConnectionManagerImplTest, RequestHeaderTimeoutCallbackDisarmsAndReturns408) {
request_headers_timeout_ = std::chrono::milliseconds(10);
setup(false, "");

Event::MockTimer* request_header_timer;
EXPECT_CALL(*codec_, dispatch(_)).WillOnce(Invoke([&](Buffer::Instance&) -> Http::Status {
request_header_timer = setUpTimer();
EXPECT_CALL(*request_header_timer, enableTimer(request_headers_timeout_, _)).Times(1);

conn_manager_->newStream(response_encoder_);
EXPECT_CALL(filter_callbacks_.connection_.dispatcher_, setTrackedObject(_)).Times(2);
return Http::okStatus();
}));

Buffer::OwnedImpl fake_input("GET /resource HTTP/1.1\r\n\r\n");
Comment thread
antoniovicente marked this conversation as resolved.
conn_manager_->onData(fake_input, false); // kick off request

// The client took too long to send headers.
EXPECT_CALL(*request_header_timer, disableTimer).Times(1);
request_header_timer->invokeCallback();

EXPECT_EQ(1U, stats_.named_.downstream_rq_header_timeout_.value());
}

TEST_F(HttpConnectionManagerImplTest, MaxStreamDurationDisabledIfSetToZero) {
max_stream_duration_ = std::chrono::milliseconds(0);
setup(false, "");
Expand Down
Loading