Skip to content

connection: tighten network connection buffer limits #14333

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

Merged
ggreenway merged 8 commits into from
Jan 14, 2021
Merged

connection: tighten network connection buffer limits #14333

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eab8734
Tighten ConnectionImpl watermark tests
antoniovicente Dec 29, 2020
918c53b
tighten network connection buffer limits
antoniovicente Dec 8, 2020
ecac94a
adjust test expectations
antoniovicente Dec 29, 2020
22ce683
fix test after merge
antoniovicente Dec 29, 2020
c5cff0c
Merge remote-tracking branch 'upstream/master' into network_connectio…
antoniovicente Dec 29, 2020
212054d
fix typo
antoniovicente Jan 7, 2021
10da10d
Merge remote-tracking branch 'upstream/master' into network_connectio…
antoniovicente Jan 14, 2021
ba6b3be
add release note
antoniovicente Jan 14, 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
20 changes: 7 additions & 13 deletions source/common/network/connection_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,20 +476,14 @@ void ConnectionImpl::setBufferLimits(uint32_t limit) {
// limits we err on the side of buffering more triggering watermark callbacks less often.
//
// Given the current implementation for straight up TCP proxying, the common case is reading
// |limit| bytes through the socket, passing |limit| bytes to the connection (triggering the high
// watermarks) and the immediately draining |limit| bytes to the socket (triggering the low
// watermarks). We avoid this by setting the high watermark to limit + 1 so a single read will
// not trigger watermarks if the socket is not blocked.
//
// If the connection class is changed to write to the buffer and flush to the socket in the same
// stack then instead of checking watermarks after the write and again after the flush it can
// check once after both operations complete. At that point it would be better to change the high
// watermark from |limit + 1| to |limit| as the common case (move |limit| bytes, flush |limit|
// bytes) would not trigger watermarks but a blocked socket (move |limit| bytes, flush 0 bytes)
// would result in respecting the exact buffer limit.
// |limit| bytes through the socket, passing |limit| bytes to the connection and the immediately
// draining |limit| bytes to the socket. Triggering the high watermarks and then immediately
// triggering the low watermarks would be expensive, but we narrowly avoid triggering high
// watermark when moving |limit| bytes through the connection because the high watermark
// computation checks if the size of the buffer exceeds the high watermark value.
if (limit > 0) {
write_buffer_->setWatermarks(limit + 1);
read_buffer_->setWatermarks(limit + 1);

@antoniovicente antoniovicente Dec 31, 2020 •
edited
Loading

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 should mention that the +1 on the limit can result in a uint32_t overflow. One example where this happens is source/extensions/filters/network/dubbo_proxy/conn_manager.cc which passes in UINT32_MAX to setBufferLimits. It may also happen based on config.

I expect this overflow to be handled fine, but it is not explicitly covered by tests.

write_buffer_->setWatermarks(limit);
read_buffer_->setWatermarks(limit);
}
}

Expand Down
73 changes: 59 additions & 14 deletions test/common/network/connection_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -981,38 +981,55 @@ TEST_P(ConnectionImplTest, ReadWatermarks) {
};

EXPECT_FALSE(testClientConnection()->readBuffer().highWatermarkTriggered());
EXPECT_FALSE(testClientConnection()->shouldDrainReadBuffer());
EXPECT_TRUE(client_connection_->readEnabled());
// Add 4 bytes to the buffer and verify the connection becomes read disabled.
// Add 3 bytes to the buffer so that it sits at exactly the read limit. Verify that

@ggreenway ggreenway Jan 6, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

"2 bytes"

// shouldDrainReadBuffer is true, but the connection remains read enabled.
{
Buffer::OwnedImpl buffer("data");
Buffer::OwnedImpl buffer("12");
server_connection_->write(buffer, false);
EXPECT_CALL(*client_read_filter, onData(_, false)).WillOnce(Invoke(on_filter_data_exit));
dispatcher_->run(Event::Dispatcher::RunType::Block);

EXPECT_TRUE(testClientConnection()->shouldDrainReadBuffer());
EXPECT_FALSE(testClientConnection()->readBuffer().highWatermarkTriggered());
EXPECT_TRUE(client_connection_->readEnabled());
}
// Add 1 bytes to the buffer to go over the high watermark. Verify the connection becomes read
// disabled.
{
Buffer::OwnedImpl buffer("3");
server_connection_->write(buffer, false);
EXPECT_CALL(*client_read_filter, onData(_, false)).WillOnce(Invoke(on_filter_data_exit));
dispatcher_->run(Event::Dispatcher::RunType::Block);

EXPECT_TRUE(testClientConnection()->readBuffer().highWatermarkTriggered());
EXPECT_TRUE(testClientConnection()->shouldDrainReadBuffer());
EXPECT_FALSE(client_connection_->readEnabled());
}

// Drain 3 bytes from the buffer. This bring sit below the low watermark, and
// Drain 2 bytes from the buffer. This bring sit below the low watermark, and
// read enables, as well as triggering a kick for the remaining byte.
{
testClientConnection()->readBuffer().drain(3);
testClientConnection()->readBuffer().drain(2);
EXPECT_FALSE(testClientConnection()->readBuffer().highWatermarkTriggered());
EXPECT_FALSE(testClientConnection()->shouldDrainReadBuffer());
EXPECT_TRUE(client_connection_->readEnabled());

EXPECT_CALL(*client_read_filter, onData(_, false));
dispatcher_->run(Event::Dispatcher::RunType::NonBlock);
}

// Add 3 bytes to the buffer and verify the connection becomes read disabled
// Add 2 bytes to the buffer and verify the connection becomes read disabled
// again.
{
Buffer::OwnedImpl buffer("bye");
Buffer::OwnedImpl buffer("45");
server_connection_->write(buffer, false);
EXPECT_CALL(*client_read_filter, onData(_, false)).WillOnce(Invoke(on_filter_data_exit));
dispatcher_->run(Event::Dispatcher::RunType::Block);

EXPECT_TRUE(testClientConnection()->readBuffer().highWatermarkTriggered());
EXPECT_TRUE(testClientConnection()->shouldDrainReadBuffer());
EXPECT_FALSE(client_connection_->readEnabled());
}

Expand All @@ -1021,8 +1038,9 @@ TEST_P(ConnectionImplTest, ReadWatermarks) {
// does not want to read.
{
client_connection_->readDisable(true);
testClientConnection()->readBuffer().drain(3);
testClientConnection()->readBuffer().drain(2);
EXPECT_FALSE(testClientConnection()->readBuffer().highWatermarkTriggered());
EXPECT_FALSE(testClientConnection()->shouldDrainReadBuffer());
EXPECT_FALSE(client_connection_->readEnabled());

EXPECT_CALL(*client_read_filter, onData(_, false)).Times(0);
Expand Down Expand Up @@ -1058,6 +1076,7 @@ TEST_P(ConnectionImplTest, ReadWatermarks) {
}));
dispatcher_->run(Event::Dispatcher::RunType::Block);
EXPECT_TRUE(testClientConnection()->readBuffer().highWatermarkTriggered());
EXPECT_TRUE(testClientConnection()->shouldDrainReadBuffer());
EXPECT_FALSE(client_connection_->readEnabled());

// Read disable and read enable, to set dispatch_buffered_data_ true.
Expand Down Expand Up @@ -1217,7 +1236,7 @@ TEST_P(ConnectionImplTest, WatermarkFuzzing) {
// If the current bytes buffered plus the bytes we write this loop go over
// the watermark and we're not currently above, we will get a callback for
// going above.
if (bytes_to_write + bytes_buffered > 11 && is_below) {
if (bytes_to_write + bytes_buffered > 10 && is_below) {
ENVOY_LOG_MISC(trace, "Expect onAboveWriteBufferHighWatermark");
EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark());
is_below = false;
Expand Down Expand Up @@ -2002,17 +2021,41 @@ TEST_F(MockTransportConnectionImplTest, ReadBufferResumeAfterReadDisable) {
connection_->enableHalfClose(true);
connection_->addReadFilter(read_filter);

// Add some data to the read buffer to trigger read activate calls when re-enabling read.
EXPECT_CALL(*transport_socket_, doRead(_))
.WillOnce(Invoke([](Buffer::Instance& buffer) -> IoResult {
buffer.add("0123456789");
return {PostIoAction::KeepOpen, 10, false};
buffer.add("0123");
return {PostIoAction::KeepOpen, 4, false};
}));
// Expect a change to the event mask when hitting the read buffer high-watermark.
EXPECT_CALL(*file_event_, setEnabled(Event::FileReadyType::Write));
// Buffer is under the read limit, expect no changes to the file event registration.
EXPECT_CALL(*file_event_, setEnabled(_)).Times(0);
EXPECT_CALL(*read_filter, onNewConnection()).WillOnce(Return(FilterStatus::Continue));
EXPECT_CALL(*read_filter, onData(_, false)).WillOnce(Return(FilterStatus::Continue));
file_ready_cb_(Event::FileReadyType::Read);
EXPECT_FALSE(connection_->shouldDrainReadBuffer());

// Do a second read to hit the read limit.
EXPECT_CALL(*transport_socket_, doRead(_))
.WillOnce(Invoke([](Buffer::Instance& buffer) -> IoResult {
buffer.add("4");
return {PostIoAction::KeepOpen, 1, false};
}));
// Buffer is exactly at the read limit, expect no changes to the file event registration.
EXPECT_CALL(*file_event_, setEnabled(_)).Times(0);
EXPECT_CALL(*read_filter, onData(_, false)).WillOnce(Return(FilterStatus::Continue));
file_ready_cb_(Event::FileReadyType::Read);
EXPECT_TRUE(connection_->shouldDrainReadBuffer());

// Do a third read to trigger the high watermark.
EXPECT_CALL(*transport_socket_, doRead(_))
.WillOnce(Invoke([](Buffer::Instance& buffer) -> IoResult {
buffer.add("5");
return {PostIoAction::KeepOpen, 1, false};
}));
// Expect a change to the event mask when going over the read limit.
EXPECT_CALL(*file_event_, setEnabled(Event::FileReadyType::Write));
EXPECT_CALL(*read_filter, onData(_, false)).WillOnce(Return(FilterStatus::Continue));
file_ready_cb_(Event::FileReadyType::Read);
EXPECT_TRUE(connection_->shouldDrainReadBuffer());

// Already read disabled, expect no changes to enabled events mask.
EXPECT_CALL(*file_event_, setEnabled(_)).Times(0);
Expand All @@ -2030,7 +2073,7 @@ TEST_F(MockTransportConnectionImplTest, ReadBufferResumeAfterReadDisable) {
EXPECT_CALL(*transport_socket_, doRead(_)).Times(0);
EXPECT_CALL(*read_filter, onData(_, _))
.WillRepeatedly(Invoke([&](Buffer::Instance& data, bool) -> FilterStatus {
EXPECT_EQ(10, data.length());
EXPECT_EQ(6, data.length());
data.drain(data.length() - 1);
return FilterStatus::Continue;
}));
Expand All @@ -2039,6 +2082,7 @@ TEST_F(MockTransportConnectionImplTest, ReadBufferResumeAfterReadDisable) {
EXPECT_CALL(*file_event_, setEnabled(Event::FileReadyType::Read | Event::FileReadyType::Write));
EXPECT_CALL(*file_event_, activate(Event::FileReadyType::Read));
file_ready_cb_(Event::FileReadyType::Read);
EXPECT_FALSE(connection_->shouldDrainReadBuffer());

// Drain the rest of the buffer and verify there are no spurious read activate calls.
EXPECT_CALL(*transport_socket_, doRead(_))
Expand All @@ -2050,6 +2094,7 @@ TEST_F(MockTransportConnectionImplTest, ReadBufferResumeAfterReadDisable) {
return FilterStatus::Continue;
}));
file_ready_cb_(Event::FileReadyType::Read);
EXPECT_FALSE(connection_->shouldDrainReadBuffer());

EXPECT_CALL(*file_event_, setEnabled(_));
connection_->readDisable(true);
Expand Down
2 changes: 1 addition & 1 deletion test/integration/http2_flood_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,7 @@ TEST_P(Http2FloodMitigationTest, Data) {
EXPECT_GE(22000, buffer_factory->sumMaxBufferSizes());
// Verify that all buffers have watermarks set.
EXPECT_THAT(buffer_factory->highWatermarkRange(),
testing::Pair(1024 * 1024 * 1024 + 1, 1024 * 1024 * 1024 + 1));
testing::Pair(1024 * 1024 * 1024, 1024 * 1024 * 1024));
}

// Verify that the server can detect flood triggered by a DATA frame from a decoder filter call
Expand Down