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.

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

overload: scale transport socket connect timeout #13800

Merged
merged 22 commits into from
Jan 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
abcdb84
Split thread_local_object.h and overload_manager.h
akonradi Oct 23, 2020
f5b8d56
Scale transport socket handshake timeout
akonradi Oct 27, 2020
7d28234
Add tests for different scaled timer types
akonradi Oct 29, 2020
5d0d600
Change PrintTo to operator<<
akonradi Nov 3, 2020
76cd8c4
Merge remote-tracking branch 'upstream/master' into tls-scaled-timeout
akonradi Nov 3, 2020
2f169bf
Merge branch 'master' of https://github.com/envoyproxy/envoy into tls…
akonradi Nov 12, 2020
0c1cf92
Address review feedback
akonradi Nov 13, 2020
3376062
Address more review feedback
akonradi Nov 16, 2020
927ea02
Make ScaledTimerMinimum wrap, not inherit
akonradi Nov 17, 2020
68e62d4
Remove unused 'using'
akonradi Nov 17, 2020
9d85d8a
Merge branch 'master' of https://github.com/envoyproxy/envoy into tls…
akonradi Nov 24, 2020
29ea543
Fix mock method expectation
akonradi Dec 1, 2020
f52c76e
Merge branch 'master' of https://github.com/envoyproxy/envoy into tls…
akonradi Dec 1, 2020
a5ccd82
Address review feedback
akonradi Dec 7, 2020
18f6022
Merge branch 'master' of https://github.com/envoyproxy/envoy into tls…
akonradi Dec 8, 2020
4ca4b11
Add integration test
akonradi Dec 10, 2020
ee93040
Merge remote-tracking branch 'upstream/main' into tls-scaled-timeout
akonradi Jan 20, 2021
ddad3e5
Fix formatting
akonradi Jan 20, 2021
9be6e8f
Merge branch 'main' of https://github.com/envoyproxy/envoy into tls-s…
akonradi Jan 21, 2021
475253b
Document new scaled timer in release notes
akonradi Jan 21, 2021
2f2ed9f
Merge branch 'main' of https://github.com/envoyproxy/envoy into tls-s…
akonradi Jan 22, 2021
6ad33cf
Remove unused operator<< methods
akonradi Jan 22, 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
6 changes: 6 additions & 0 deletions api/envoy/config/overload/v3/overload.proto
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,12 @@ message ScaleTimersOverloadActionConfig {
// :ref:`HttpConnectionManager.stream_idle_timeout
// <envoy_v3_api_field_extensions.filters.network.http_connection_manager.v3.HttpConnectionManager.stream_idle_timeout>`
HTTP_DOWNSTREAM_STREAM_IDLE = 2;

// Adjusts the timer for how long downstream clients have to finish transport-level negotiations
// before the connection is closed.
// This affects the value of
// :ref:`FilterChain.transport_socket_connect_timeout <envoy_v3_api_field_config.listener.v3.FilterChain.transport_socket_connect_timeout>`.
TRANSPORT_SOCKET_CONNECT = 3;
ggreenway marked this conversation as resolved.
Show resolved Hide resolved
}

message ScaleTimer {
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 @@ -46,6 +46,7 @@ New Features
* dispatcher: supports a stack of `Envoy::ScopeTrackedObject` instead of a single tracked object. This will allow Envoy to dump more debug information on crash.
* http: added support for :ref:`:ref:`preconnecting <envoy_v3_api_msg_config.cluster.v3.Cluster.PreconnectPolicy>`. Preconnecting is off by default, but recommended for clusters serving latency-sensitive traffic, especially if using HTTP/1.1.
* http: change frame flood and abuse checks to the upstream HTTP/2 codec to ON by default. It can be disabled by setting the `envoy.reloadable_features.upstream_http2_flood_checks` runtime key to false.
* overload: add support for scaling :ref:`transport connection timeouts<envoy_v3_api_enum_value_config.overload.v3.ScaleTimersOverloadActionConfig.TimerType.TRANSPORT_SOCKET_CONNECT>`. This can be used to reduce the TLS handshake timeout in response to overload.
* server: added :ref:`fips_mode <statistics>` statistic.
* tcp_proxy: add support for converting raw TCP streams into HTTP/1.1 CONNECT requests. See :ref:`upgrade documentation <tunneling-tcp-over-http>` for details.

Expand Down
6 changes: 6 additions & 0 deletions generated_api_shadow/envoy/config/overload/v3/overload.proto

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

5 changes: 5 additions & 0 deletions include/envoy/event/scaled_timer.h
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#pragma once

#include <chrono>
#include <ostream>

#include "common/common/interval_value.h"

Expand Down Expand Up @@ -75,6 +76,10 @@ enum class ScaledTimerType {
// The amount of time an HTTP stream from a downstream client can remain idle. This corresponds to
// the HTTP_DOWNSTREAM_STREAM_IDLE TimerType in overload.proto.
HttpDownstreamIdleStreamTimeout,
// The amount of time a connection to a downstream client can spend waiting for the transport to
// report connection establishment before the connection is closed. This corresponds to the
// TRANSPORT_SOCKET_CONNECT_TIMEOUT TimerType in overload.proto.
TransportSocketConnectTimeout,
};

using ScaledTimerTypeMap = absl::flat_hash_map<ScaledTimerType, ScaledTimerMinimum>;
Expand Down
1 change: 1 addition & 0 deletions source/common/network/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ envoy_cc_library(
"//include/envoy/event:timer_interface",
"//include/envoy/network:connection_interface",
"//include/envoy/network:filter_interface",
"//include/envoy/server/overload:thread_local_overload_state",
"//source/common/buffer:buffer_lib",
"//source/common/buffer:watermark_buffer_lib",
"//source/common/common:assert_lib",
Expand Down
4 changes: 3 additions & 1 deletion source/common/network/connection_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#include "envoy/common/exception.h"
#include "envoy/common/platform.h"
#include "envoy/config/core/v3/base.pb.h"
#include "envoy/event/scaled_range_timer_manager.h"
#include "envoy/event/timer.h"
#include "envoy/network/filter.h"
#include "envoy/network/socket.h"
Expand Down Expand Up @@ -788,7 +789,8 @@ void ServerConnectionImpl::setTransportSocketConnectTimeout(std::chrono::millise
}
if (transport_socket_connect_timer_ == nullptr) {
transport_socket_connect_timer_ =
dispatcher_.createTimer([this] { onTransportSocketConnectTimeout(); });
dispatcher_.createScaledTimer(Event::ScaledTimerType::TransportSocketConnectTimeout,
[this] { onTransportSocketConnectTimeout(); });
}
transport_socket_connect_timer_->enableTimer(timeout);
}
Expand Down
2 changes: 2 additions & 0 deletions source/server/overload_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ Event::ScaledTimerType parseTimerType(
return Event::ScaledTimerType::HttpDownstreamIdleConnectionTimeout;
case Config::HTTP_DOWNSTREAM_STREAM_IDLE:
return Event::ScaledTimerType::HttpDownstreamIdleStreamTimeout;
case Config::TRANSPORT_SOCKET_CONNECT:
return Event::ScaledTimerType::TransportSocketConnectTimeout;
default:
throw EnvoyException(fmt::format("Unknown timer type {}", config_timer_type));
}
Expand Down
11 changes: 9 additions & 2 deletions test/common/network/connection_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

#include "envoy/common/platform.h"
#include "envoy/config/core/v3/base.pb.h"
#include "envoy/event/scaled_range_timer_manager.h"
#include "envoy/network/address.h"

#include "common/api/os_sys_calls_impl.h"
Expand Down Expand Up @@ -403,7 +404,10 @@ TEST_P(ConnectionImplTest, SetServerTransportSocketTimeout) {
// Avoid setting noDelay on the fake fd of 0.
auto local_addr = std::make_shared<Network::Address::PipeInstance>("/pipe/path");

auto* mock_timer = new NiceMock<Event::MockTimer>(mocks.dispatcher_.get());
auto* mock_timer = new NiceMock<Event::MockTimer>();
Copy link
Member

Choose a reason for hiding this comment

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

i wonder if this would result into memory leak, wrap in std::make_unique?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It gets returned by the EXPECT_CALL on line 363, which wraps it in a unique_ptr.

EXPECT_CALL(*mocks.dispatcher_,
createScaledTypedTimer_(Event::ScaledTimerType::TransportSocketConnectTimeout, _))
.WillOnce(DoAll(SaveArg<1>(&mock_timer->callback_), Return(mock_timer)));
auto server_connection = std::make_unique<Network::ServerConnectionImpl>(
*mocks.dispatcher_,
std::make_unique<ConnectionSocketImpl>(std::move(io_handle), local_addr, nullptr),
Expand Down Expand Up @@ -442,7 +446,10 @@ TEST_P(ConnectionImplTest, ServerTransportSocketTimeoutDisabledOnConnect) {
IoHandlePtr io_handle = std::make_unique<IoSocketHandleImpl>(0);
auto local_addr = std::make_shared<Network::Address::PipeInstance>("/pipe/path");

auto* mock_timer = new NiceMock<Event::MockTimer>(mocks.dispatcher_.get());
auto* mock_timer = new NiceMock<Event::MockTimer>();
EXPECT_CALL(*mocks.dispatcher_,
createScaledTypedTimer_(Event::ScaledTimerType::TransportSocketConnectTimeout, _))
.WillOnce(DoAll(SaveArg<1>(&mock_timer->callback_), Return(mock_timer)));
auto server_connection = std::make_unique<Network::ServerConnectionImpl>(
*mocks.dispatcher_,
std::make_unique<ConnectionSocketImpl>(std::move(io_handle), local_addr, nullptr),
Expand Down
85 changes: 85 additions & 0 deletions test/integration/overload_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@

#include "test/common/config/dummy_config.pb.h"
#include "test/integration/http_protocol_integration.h"
#include "test/integration/ssl_utility.h"
#include "test/test_common/registry.h"

#include "absl/strings/str_cat.h"

using testing::InvokeWithoutArgs;

namespace Envoy {

using testing::HasSubstr;
Expand Down Expand Up @@ -357,4 +360,86 @@ TEST_P(OverloadScaledTimerIntegrationTest, CloseIdleHttpStream) {
EXPECT_THAT(response->body(), HasSubstr("stream timeout"));
}

TEST_P(OverloadScaledTimerIntegrationTest, TlsHandshakeTimeout) {
// Set up the Envoy to expect a TLS connection, with a 20 second timeout that can scale down to 5
// seconds.
config_helper_.addSslConfig();
config_helper_.addConfigModifier([&](envoy::config::bootstrap::v3::Bootstrap& bootstrap) -> void {
auto* filter_chain =
bootstrap.mutable_static_resources()->mutable_listeners(0)->mutable_filter_chains(0);
auto* connect_timeout = filter_chain->mutable_transport_socket_connect_timeout();
connect_timeout->set_seconds(20);
connect_timeout->set_nanos(0);
});
initializeOverloadManager(
TestUtility::parseYaml<envoy::config::overload::v3::ScaleTimersOverloadActionConfig>(R"EOF(
timer_scale_factors:
- timer: TRANSPORT_SOCKET_CONNECT
min_timeout: 5s
)EOF"));

// Set up a delinquent transport socket that causes the dispatcher to exit on every read & write
// instead of actually doing anything useful.
auto bad_transport_socket = std::make_unique<NiceMock<Network::MockTransportSocket>>();
Network::TransportSocketCallbacks* transport_callbacks;
EXPECT_CALL(*bad_transport_socket, setTransportSocketCallbacks)
.WillOnce(SaveArgAddress(&transport_callbacks));
ON_CALL(*bad_transport_socket, doRead).WillByDefault(InvokeWithoutArgs([&] {
Buffer::OwnedImpl buffer;
transport_callbacks->connection().dispatcher().exit();
// Read some amount of data; what's more important is whether the socket was remote-closed. That
// needs to be propagated to the socket.
return Network::IoResult{transport_callbacks->ioHandle().read(buffer, 2 * 1024).rc_ == 0
? Network::PostIoAction::Close
: Network::PostIoAction::KeepOpen,
0, false};
}));
ON_CALL(*bad_transport_socket, doWrite).WillByDefault(InvokeWithoutArgs([&] {
transport_callbacks->connection().dispatcher().exit();
return Network::IoResult{Network::PostIoAction::KeepOpen, 0, false};
}));

ConnectionStatusCallbacks connect_callbacks;
Network::Address::InstanceConstSharedPtr address =
Ssl::getSslAddress(version_, lookupPort("http"));
auto bad_ssl_client =
dispatcher_->createClientConnection(address, Network::Address::InstanceConstSharedPtr(),
std::move(bad_transport_socket), nullptr);
bad_ssl_client->addConnectionCallbacks(connect_callbacks);
bad_ssl_client->enableHalfClose(true);
bad_ssl_client->connect();

// Run the dispatcher until it exits due to a read/write.
dispatcher_->run(Event::Dispatcher::RunType::RunUntilExit);

// At this point, the connection should be idle but the SSL handshake isn't done.
EXPECT_FALSE(connect_callbacks.connected());

// Set the load so the timer is reduced but not to the minimum value.
updateResource(0.8);
test_server_->waitForGaugeGe("overload.envoy.overload_actions.reduce_timeouts.scale_percent", 50);

// Advancing past the minimum time shouldn't close the connection, but it shouldn't complete it
// either.
timeSystem().advanceTimeWait(std::chrono::seconds(5));
EXPECT_FALSE(connect_callbacks.connected());

// At this point, Envoy has been waiting for the (bad) client to finish the TLS handshake for 5
// seconds. Increase the load so that the minimum time has now elapsed. This should cause Envoy to
// close the connection on its end.
updateResource(0.9);
test_server_->waitForGaugeEq("overload.envoy.overload_actions.reduce_timeouts.scale_percent",
100);

// The bad client will continue attempting to read, eventually noticing the remote close and
// closing the connection.
while (!connect_callbacks.closed()) {
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
}

// The transport-level connection was never completed, and the connection was closed.
EXPECT_FALSE(connect_callbacks.connected());
EXPECT_TRUE(connect_callbacks.closed());
}

} // namespace Envoy
6 changes: 5 additions & 1 deletion test/server/overload_manager_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "envoy/config/overload/v3/overload.pb.h"
#include "envoy/event/scaled_range_timer_manager.h"
#include "envoy/server/overload/overload_manager.h"
#include "envoy/server/overload/thread_local_overload_state.h"
#include "envoy/server/resource_monitor.h"
#include "envoy/server/resource_monitor_config.h"

Expand Down Expand Up @@ -494,7 +495,9 @@ constexpr char kReducedTimeoutsConfig[] = R"YAML(
- timer: HTTP_DOWNSTREAM_CONNECTION_IDLE
min_timeout: 2s
- timer: HTTP_DOWNSTREAM_STREAM_IDLE
min_scale: { value: 10 }
min_scale: { value: 10 } # percent
- timer: TRANSPORT_SOCKET_CONNECT
min_scale: { value: 40 } # percent
triggers:
- name: "envoy.resource_monitors.fake_resource1"
scaled:
Expand All @@ -507,6 +510,7 @@ constexpr std::pair<TimerType, Event::ScaledTimerMinimum> kReducedTimeoutsMinimu
{TimerType::HttpDownstreamIdleConnectionTimeout,
Event::AbsoluteMinimum(std::chrono::seconds(2))},
{TimerType::HttpDownstreamIdleStreamTimeout, Event::ScaledMinimum(UnitFloat(0.1))},
{TimerType::TransportSocketConnectTimeout, Event::ScaledMinimum(UnitFloat(0.4))},
};
TEST_F(OverloadManagerImplTest, CreateScaledTimerManager) {
auto manager(createOverloadManager(kReducedTimeoutsConfig));
Expand Down