From cbe959f1781d022432afba4cdcafb2d607a66cf4 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 6 Jul 2017 12:53:00 -0400 Subject: [PATCH 01/28] TCP + TLS flow control --- include/envoy/network/connection.h | 17 + source/common/filter/auth/client_ssl.h | 2 + source/common/filter/ratelimit.h | 2 + source/common/filter/tcp_proxy.cc | 26 + source/common/filter/tcp_proxy.h | 9 + source/common/http/codec_client.h | 2 + source/common/http/conn_manager_impl.h | 2 + source/common/http/http1/conn_pool.h | 2 + source/common/http/http2/conn_pool.h | 2 + source/common/mongo/proxy.h | 2 + source/common/network/connection_impl.cc | 62 ++- source/common/network/connection_impl.h | 28 +- source/common/network/listener_impl.cc | 15 + source/common/redis/conn_pool_impl.h | 4 + source/common/redis/proxy_filter.h | 2 + source/common/ssl/connection_impl.cc | 7 +- source/common/stats/statsd.h | 2 + source/common/upstream/health_checker_impl.h | 6 + source/common/upstream/upstream_impl.cc | 4 +- source/server/connection_handler_impl.h | 2 + test/common/network/BUILD | 1 + test/common/network/connection_impl_test.cc | 444 +++++++++++++----- test/config/integration/tcp_proxy.json | 34 +- test/integration/BUILD | 1 + test/integration/fake_upstream.h | 6 +- test/integration/integration.cc | 17 +- test/integration/integration.h | 7 + .../integration/tcp_proxy_integration_test.cc | 41 +- test/integration/tcp_proxy_integration_test.h | 15 +- test/integration/utility.h | 4 +- test/mocks/buffer/mocks.h | 39 ++ test/mocks/network/mocks.h | 4 + 32 files changed, 676 insertions(+), 135 deletions(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 355cb7e395f3c..9fc9d1a6197ec 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -45,6 +45,17 @@ class ConnectionCallbacks { * @param events supplies the ConnectionEvent events that occurred as a bitmask. */ virtual void onEvent(uint32_t events) PURE; + + /** + * Called when the write buffer for a connection goes over its high watermark. + */ + virtual void onAboveWriteBufferHighWatermark() PURE; + + /** + * Called when the write buffer for a connection goes from over its high + * watermark to under its low watermark. + */ + virtual void onBelowWriteBufferLowWatermark() PURE; }; /** @@ -162,6 +173,12 @@ class Connection : public Event::DeferredDeletable, public FilterManager { * Get the value set with setReadBufferLimit. */ virtual uint32_t readBufferLimit() const PURE; + + /** + * Sets the high and low watermarks which trigger onAboveWriteBufferHighWatermark + * and onBelowWriteBufferHighWatermark callbacks. + */ + virtual void setWriteBufferWatermarks(size_t low_watermark, size_t high_watermark) PURE; }; typedef std::unique_ptr ConnectionPtr; diff --git a/source/common/filter/auth/client_ssl.h b/source/common/filter/auth/client_ssl.h index 23373715c882d..7a2c625ac8aaa 100644 --- a/source/common/filter/auth/client_ssl.h +++ b/source/common/filter/auth/client_ssl.h @@ -119,6 +119,8 @@ class Instance : public Network::ReadFilter, public Network::ConnectionCallbacks // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} private: ConfigSharedPtr config_; diff --git a/source/common/filter/ratelimit.h b/source/common/filter/ratelimit.h index ece8d05850e09..751b53b87f253 100644 --- a/source/common/filter/ratelimit.h +++ b/source/common/filter/ratelimit.h @@ -82,6 +82,8 @@ class Instance : public Network::ReadFilter, // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} // RateLimit::RequestCallbacks void complete(LimitStatus status) override; diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index da4ec3eb5c30a..8c596927837b7 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -123,6 +123,32 @@ void TcpProxy::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callb config_->stats().downstream_cx_tx_bytes_buffered_}); } +void TcpProxy::readDisableUpstream(bool disable) { upstream_connection_->readDisable(disable); } + +void TcpProxy::readDisableDownstream(bool disable) { + read_callbacks_->connection().readDisable(disable); +} + +void TcpProxy::DownstreamCallbacks::onAboveWriteBufferHighWatermark() { + // If downstream has too much data buffered, stop reading on the upstream connection. + parent_.readDisableUpstream(true); +} + +void TcpProxy::DownstreamCallbacks::onBelowWriteBufferLowWatermark() { + // The downstream buffer has been drained. Resume reading from upstream. + parent_.readDisableUpstream(false); +} + +void TcpProxy::UpstreamCallbacks::onAboveWriteBufferHighWatermark() { + // There's too much data buffered in the upstream write buffer, so stop reading. + parent_.readDisableDownstream(true); +} + +void TcpProxy::UpstreamCallbacks::onBelowWriteBufferLowWatermark() { + // The upstream write buffer is drained. Resume reading. + parent_.readDisableDownstream(false); +} + Network::FilterStatus TcpProxy::initializeUpstreamConnection() { const std::string& cluster_name = config_->getRouteFromEntries(read_callbacks_->connection()); diff --git a/source/common/filter/tcp_proxy.h b/source/common/filter/tcp_proxy.h index d981472ea97e4..335d30347030c 100644 --- a/source/common/filter/tcp_proxy.h +++ b/source/common/filter/tcp_proxy.h @@ -94,12 +94,19 @@ class TcpProxy : public Network::ReadFilter, Logger::Loggable, // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} std::list active_requests_; Http::ConnectionCallbacks* codec_callbacks_{}; diff --git a/source/common/http/conn_manager_impl.h b/source/common/http/conn_manager_impl.h index fd370780478a2..15d0abc296754 100644 --- a/source/common/http/conn_manager_impl.h +++ b/source/common/http/conn_manager_impl.h @@ -279,6 +279,8 @@ class ConnectionManagerImpl : Logger::Loggable, // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} private: struct ActiveStream; diff --git a/source/common/http/http1/conn_pool.h b/source/common/http/http1/conn_pool.h index 615dc6ef5ab53..5fdabe5fbcfff 100644 --- a/source/common/http/http1/conn_pool.h +++ b/source/common/http/http1/conn_pool.h @@ -76,6 +76,8 @@ class ConnPoolImpl : Logger::Loggable, public ConnectionPool:: // Network::ConnectionCallbacks void onEvent(uint32_t events) override { parent_.onConnectionEvent(*this, events); } + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} ConnPoolImpl& parent_; CodecClientPtr codec_client_; diff --git a/source/common/http/http2/conn_pool.h b/source/common/http/http2/conn_pool.h index 89cd9be9c85b8..a0f17e087d79b 100644 --- a/source/common/http/http2/conn_pool.h +++ b/source/common/http/http2/conn_pool.h @@ -43,6 +43,8 @@ class ConnPoolImpl : Logger::Loggable, public ConnectionPool:: // Network::ConnectionCallbacks void onEvent(uint32_t events) override { parent_.onConnectionEvent(*this, events); } + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} // CodecClientCallbacks void onStreamDestroy() override { parent_.onStreamDestroy(*this); } diff --git a/source/common/mongo/proxy.h b/source/common/mongo/proxy.h index 8be56ff6e87eb..7e0d7281c88de 100644 --- a/source/common/mongo/proxy.h +++ b/source/common/mongo/proxy.h @@ -107,6 +107,8 @@ class ProxyFilter : public Network::Filter, // Network::ConnectionCallbacks void onEvent(uint32_t event) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} private: struct ActiveQuery { diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index b376a36534982..d16799a8b8371 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -98,7 +98,7 @@ void ConnectionImpl::close(ConnectionCloseType type) { return; } - uint64_t data_to_write = write_buffer_.length(); + uint64_t data_to_write = write_buffer_->length(); ENVOY_CONN_LOG(debug, "closing data_to_write={} type={}", *this, data_to_write, enumToInt(type)); if (data_to_write == 0 || type == ConnectionCloseType::NoFlush) { if (data_to_write > 0) { @@ -207,15 +207,25 @@ void ConnectionImpl::readDisable(bool disable) { // TODO(mattklein123): Potentially support half-closed TCP connections. It's unclear if this is // required for any scenarios in which Envoy will be used (I don't know of any). if (disable) { - ASSERT(read_enabled); + if (!read_enabled) { + ++read_disable_count_; + return; + } state_ &= ~InternalState::ReadEnabled; file_event_->setEnabled(Event::FileReadyType::Write | Event::FileReadyType::Closed); } else { + if (read_disable_count_ > 0) { + --read_disable_count_; + return; + } ASSERT(!read_enabled); state_ |= InternalState::ReadEnabled; // We never ask for both early close and read at the same time. If we are reading, we want to // consume all available data. file_event_->setEnabled(Event::FileReadyType::Read | Event::FileReadyType::Write); + // If the connection has data buffered there's no guarantee there's also data in the kernel + // which will kick off the filter chain. Instead fake an event to make sure the buffered data + // gets processed regardless. if (read_buffer_.length() > 0) { file_event_->activate(Event::FileReadyType::Read); } @@ -254,13 +264,51 @@ void ConnectionImpl::write(Buffer::Instance& data) { // ever changed, read the comment in Ssl::ConnectionImpl::doWriteToSocket() VERY carefully. // That code assumes that we never change existing write_buffer_ chain elements between calls // to SSL_write(). That code will have to change if we ever copy here. - write_buffer_.move(data); + write_buffer_->move(data); + checkForHighWatermark(); + if (!(state_ & InternalState::Connecting)) { file_event_->activate(Event::FileReadyType::Write); } } } +void ConnectionImpl::setWriteBufferWatermarks(size_t new_low_watermark, size_t new_high_watermark) { + ENVOY_CONN_LOG(trace, "Setting watermarks: {} {}", *this, new_low_watermark, new_high_watermark); + ASSERT(new_low_watermark < new_high_watermark); + + high_watermark_ = new_high_watermark; + low_watermark_ = new_low_watermark; + + checkForLowWatermark(); + checkForHighWatermark(); +} + +void ConnectionImpl::checkForLowWatermark() { + if (!above_high_watermark_called_ || write_buffer_->length() >= low_watermark_) { + return; + } + ENVOY_CONN_LOG(trace, "onBelowWriteBufferLowWatermark", *this); + + above_high_watermark_called_ = false; + for (ConnectionCallbacks* callback : callbacks_) { + callback->onBelowWriteBufferLowWatermark(); + } +} + +void ConnectionImpl::checkForHighWatermark() { + if (above_high_watermark_called_ || high_watermark_ == 0 || + write_buffer_->length() <= high_watermark_) { + return; + } + ENVOY_CONN_LOG(trace, "onAboveWriteBufferHighWatermark", *this); + + above_high_watermark_called_ = true; + for (ConnectionCallbacks* callback : callbacks_) { + callback->onAboveWriteBufferHighWatermark(); + } +} + void ConnectionImpl::onFileEvent(uint32_t events) { ENVOY_CONN_LOG(trace, "socket event: {}", *this, events); @@ -347,12 +395,11 @@ ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { PostIoAction action; uint64_t bytes_written = 0; do { - if (write_buffer_.length() == 0) { + if (write_buffer_->length() == 0) { action = PostIoAction::KeepOpen; break; } - - int rc = write_buffer_.write(fd_); + int rc = write_buffer_->write(fd_); ENVOY_CONN_LOG(trace, "write returns: {}", *this, rc); if (rc == -1) { ENVOY_CONN_LOG(trace, "write error: {}", *this, errno); @@ -364,6 +411,7 @@ ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { break; } else { + checkForLowWatermark(); bytes_written += rc; } } while (true); @@ -400,7 +448,7 @@ void ConnectionImpl::onWriteReady() { } IoResult result = doWriteToSocket(); - uint64_t new_buffer_size = write_buffer_.length(); + uint64_t new_buffer_size = write_buffer_->length(); updateWriteBufferStats(result.bytes_processed_, new_buffer_size); if (result.action_ == PostIoAction::Close) { diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index a3a81a76199c5..431183bbde566 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -72,11 +72,16 @@ class ConnectionImpl : public virtual Connection, void write(Buffer::Instance& data) override; void setReadBufferLimit(uint32_t limit) override { read_buffer_limit_ = limit; } uint32_t readBufferLimit() const override { return read_buffer_limit_; } + void setWriteBufferWatermarks(size_t low_watermark, size_t high_watermark) override; // Network::BufferSource Buffer::Instance& getReadBuffer() override { return read_buffer_; } Buffer::Instance& getWriteBuffer() override { return *current_write_buffer_; } + void replaceWriteBufferForTest(std::unique_ptr new_buffer) { + write_buffer_ = std::move(new_buffer); + } + protected: enum class PostIoAction { Close, KeepOpen }; @@ -99,12 +104,29 @@ class ConnectionImpl : public virtual Connection, // Reconsider how to make fairness happen. void setReadBufferReady() { file_event_->activate(Event::FileReadyType::Read); } + // Called when data is drained from the write buffer, to see if onBelowWriteBufferLowWatermark + // should be called. + void checkForLowWatermark(); + // Called when data is added to the write buffer, to see if onAboveWriteBufferHighWatermark should + // be called. + void checkForHighWatermark(); + FilterManagerImpl filter_manager_; Address::InstanceConstSharedPtr remote_address_; Address::InstanceConstSharedPtr local_address_; Buffer::OwnedImpl read_buffer_; - Buffer::OwnedImpl write_buffer_; + std::unique_ptr write_buffer_{new Buffer::OwnedImpl}; uint32_t read_buffer_limit_ = 0; + // Used for network level buffer limits (off by default). If these are non-zero, when the write + // buffer passes |high_watermark_|, onAboveWriteBufferHighWatermark will be called to disable + // reading further data. When the buffer drains below |low_watermark_|, + // onBelowWriteBufferLowWatermark will be called to resume reads. + size_t high_watermark_{0}; + size_t low_watermark_{0}; + // Tracks the latest state of watermark callbacks. + // True between the time onAboveWriteBufferHighWatermark is called until the next call to + // onBelowLowWatermark. + bool above_high_watermark_called_{false}; private: // clang-format off @@ -138,6 +160,10 @@ class ConnectionImpl : public virtual Connection, uint64_t last_read_buffer_size_{}; uint64_t last_write_buffer_size_{}; std::unique_ptr buffer_stats_; + // Tracks the number of times reads have been disabled. If N different components call + // readDisabled(true) this allows the connection to only resume reasd when readDisabled(false) + // has been called N times. + size_t read_disable_count_{0}; }; /** diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index 13499b8d36893..f092585a9a4e2 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -102,6 +102,17 @@ void ListenerImpl::newConnection(int fd, Address::InstanceConstSharedPtr remote_ Address::InstanceConstSharedPtr local_address) { ConnectionPtr new_connection(new ConnectionImpl(dispatcher_, fd, remote_address, local_address)); new_connection->setReadBufferLimit(options_.per_connection_buffer_limit_bytes_); + // Due to the fact that writes to the connection and flushing data from the connection are done + // asynchronously, we have the option of either setting the watermarks aggressively, and regularly + // enabling/disabling reads from the socket, or allowing more data, but then not triggering + // based on watermarks until 2x the data is buffered in the common case. Given these are all soft + // limits we err on the side of buffeing more and having better performace. + // If the connection class is changed to write-and-flush the high watermark should be changed to + // the buffer limit without the + 1 + if (options_.per_connection_buffer_limit_bytes_ > 0) { + new_connection->setWriteBufferWatermarks(options_.per_connection_buffer_limit_bytes_ / 2, + options_.per_connection_buffer_limit_bytes_ + 1); + } cb_.onNewConnection(std::move(new_connection)); } @@ -111,6 +122,10 @@ void SslListenerImpl::newConnection(int fd, Address::InstanceConstSharedPtr remo local_address, ssl_ctx_, Ssl::ConnectionImpl::InitialState::Server)); new_connection->setReadBufferLimit(options_.per_connection_buffer_limit_bytes_); + if (options_.per_connection_buffer_limit_bytes_ > 0) { + new_connection->setWriteBufferWatermarks(options_.per_connection_buffer_limit_bytes_ / 2, + options_.per_connection_buffer_limit_bytes_ + 1); + } cb_.onNewConnection(std::move(new_connection)); } diff --git a/source/common/redis/conn_pool_impl.h b/source/common/redis/conn_pool_impl.h index 5937ebd00e874..af2022a6c7c83 100644 --- a/source/common/redis/conn_pool_impl.h +++ b/source/common/redis/conn_pool_impl.h @@ -83,6 +83,8 @@ class ClientImpl : public Client, public DecoderCallbacks, public Network::Conne // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} Upstream::HostConstSharedPtr host_; Network::ClientConnectionPtr connection_; @@ -125,6 +127,8 @@ class InstanceImpl : public Instance { // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} ThreadLocalPool& parent_; Upstream::HostConstSharedPtr host_; diff --git a/source/common/redis/proxy_filter.h b/source/common/redis/proxy_filter.h index c8437891d5d7e..210a40f5e5e0b 100644 --- a/source/common/redis/proxy_filter.h +++ b/source/common/redis/proxy_filter.h @@ -80,6 +80,8 @@ class ProxyFilter : public Network::ReadFilter, // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} // Redis::DecoderCallbacks void onRespValue(RespValuePtr&& value) override; diff --git a/source/common/ssl/connection_impl.cc b/source/common/ssl/connection_impl.cc index c2c9716dda3a9..207ba9ca479cf 100644 --- a/source/common/ssl/connection_impl.cc +++ b/source/common/ssl/connection_impl.cc @@ -159,7 +159,7 @@ Network::ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { } } - uint64_t original_buffer_length = write_buffer_.length(); + uint64_t original_buffer_length = write_buffer_->length(); uint64_t total_bytes_written = 0; bool keep_writing = true; while ((original_buffer_length != total_bytes_written) && keep_writing) { @@ -170,7 +170,7 @@ Network::ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { // of iterations of this loop, either by pure iterations, bytes written, etc. const uint64_t MAX_SLICES = 32; Buffer::RawSlice slices[MAX_SLICES]; - uint64_t num_slices = write_buffer_.getRawSlices(slices, MAX_SLICES); + uint64_t num_slices = write_buffer_->getRawSlices(slices, MAX_SLICES); uint64_t inner_bytes_written = 0; for (uint64_t i = 0; (i < num_slices) && (original_buffer_length != total_bytes_written); i++) { @@ -205,7 +205,8 @@ Network::ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { // Draining must be done within the inner loop, otherwise we will keep getting the same slices // at the beginning of the buffer. if (inner_bytes_written > 0) { - write_buffer_.drain(inner_bytes_written); + write_buffer_->drain(inner_bytes_written); + checkForLowWatermark(); } } diff --git a/source/common/stats/statsd.h b/source/common/stats/statsd.h index 36d925319909a..7a10e6281d20a 100644 --- a/source/common/stats/statsd.h +++ b/source/common/stats/statsd.h @@ -102,6 +102,8 @@ class TcpStatsdSink : public Sink { // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} TcpStatsdSink& parent_; Event::Dispatcher& dispatcher_; diff --git a/source/common/upstream/health_checker_impl.h b/source/common/upstream/health_checker_impl.h index ab700c6cc411e..5a8cc0f1f50fc 100644 --- a/source/common/upstream/health_checker_impl.h +++ b/source/common/upstream/health_checker_impl.h @@ -176,6 +176,8 @@ class HttpHealthCheckerImpl : public HealthCheckerImplBase { // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} HttpHealthCheckerImpl& parent_; Http::CodecClientPtr client_; @@ -277,6 +279,8 @@ class TcpHealthCheckerImpl : public HealthCheckerImplBase { // Network::ConnectionCallbacks void onEvent(uint32_t events) override { parent_.onEvent(events); } + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} // Network::ReadFilter Network::FilterStatus onData(Buffer::Instance& data) override { @@ -354,6 +358,8 @@ class RedisHealthCheckerImpl : public HealthCheckerImplBase { // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} RedisHealthCheckerImpl& parent_; Redis::ConnPool::ClientPtr client_; diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index 065147c022969..bf42873ce610a 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -44,7 +44,9 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu Network::ClientConnectionPtr connection = cluster.sslContext() ? dispatcher.createSslClientConnection(*cluster.sslContext(), address) : dispatcher.createClientConnection(address); - connection->setReadBufferLimit(cluster.perConnectionBufferLimitBytes()); + uint32_t buffer_limit = cluster.perConnectionBufferLimitBytes(); + connection->setReadBufferLimit(buffer_limit); + connection->setWriteBufferWatermarks(buffer_limit / 2, buffer_limit + 1); return connection; } diff --git a/source/server/connection_handler_impl.h b/source/server/connection_handler_impl.h index 09cd27d5b2568..076184187eb9f 100644 --- a/source/server/connection_handler_impl.h +++ b/source/server/connection_handler_impl.h @@ -121,6 +121,8 @@ class ConnectionHandlerImpl : public Network::ConnectionHandler, NonCopyable { listener_.removeConnection(*this); } } + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} ActiveListener& listener_; Network::ConnectionPtr connection_; diff --git a/test/common/network/BUILD b/test/common/network/BUILD index 4226de15a7671..3814215ef6bfa 100644 --- a/test/common/network/BUILD +++ b/test/common/network/BUILD @@ -42,6 +42,7 @@ envoy_cc_test( "//source/common/network:listen_socket_lib", "//source/common/network:utility_lib", "//source/common/stats:stats_lib", + "//test/mocks/buffer:buffer_mocks", "//test/mocks/network:network_mocks", "//test/mocks/server:server_mocks", "//test/mocks/stats:stats_mocks", diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 405835b8d59b9..8c9698633c7ad 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -9,8 +9,10 @@ #include "common/network/connection_impl.h" #include "common/network/listen_socket_impl.h" #include "common/network/utility.h" +#include "common/runtime/runtime_impl.h" #include "common/stats/stats_impl.h" +#include "test/mocks/buffer/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/server/mocks.h" #include "test/mocks/stats/mocks.h" @@ -22,6 +24,8 @@ #include "gtest/gtest.h" namespace Envoy { +using testing::AnyNumber; +using testing::DoAll; using testing::InSequence; using testing::Invoke; using testing::Return; @@ -60,54 +64,91 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplDeathTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); TEST_P(ConnectionImplDeathTest, BadFd) { - Event::DispatcherImpl dispatcher; - EXPECT_DEATH(ConnectionImpl(dispatcher, -1, + Event::DispatcherImpl dispatcher_; + EXPECT_DEATH(ConnectionImpl(dispatcher_, -1, Network::Test::getCanonicalLoopbackAddress(GetParam()), Network::Test::getCanonicalLoopbackAddress(GetParam())), ".*assert failure: fd_ != -1.*"); } -class ConnectionImplTest : public testing::TestWithParam {}; +class ConnectionImplTest : public testing::TestWithParam { +public: + void setUpBasicConnection() { + listener_ = + dispatcher_.createListener(connection_handler_, socket_, listener_callbacks_, stats_store_, + Network::ListenerOptions::listenerOptionsWithBindToPort()); + + client_connection_ = dispatcher_.createClientConnection(socket_.localAddress()); + client_connection_->addConnectionCallbacks(client_callbacks_); + } + + void connect() { + client_connection_->connect(); + read_filter_.reset(new NiceMock()); + EXPECT_CALL(listener_callbacks_, onNewConnection_(_)) + .WillOnce(Invoke([&](Network::ConnectionPtr& conn) -> void { + server_connection_ = std::move(conn); + server_connection_->addConnectionCallbacks(server_callbacks_); + server_connection_->addReadFilter(read_filter_); + })); + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::Connected)); + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); + } + + void disconnect() { + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::LocalClose)); + client_connection_->close(ConnectionCloseType::NoFlush); + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); + } + + void useMockBuffer() { + dynamic_cast(client_connection_.get()) + ->replaceWriteBufferForTest(std::move(buffer_ptr_)); + } + +protected: + Event::DispatcherImpl dispatcher_; + Stats::IsolatedStoreImpl stats_store_; + Network::TcpListenSocket socket_{Network::Test::getAnyAddress(GetParam()), true}; + Network::MockListenerCallbacks listener_callbacks_; + Network::MockConnectionHandler connection_handler_; + Network::ListenerPtr listener_; + Network::ClientConnectionPtr client_connection_; + MockConnectionCallbacks client_callbacks_; + Network::ConnectionPtr server_connection_; + Network::MockConnectionCallbacks server_callbacks_; + std::shared_ptr read_filter_; + std::unique_ptr buffer_ptr_{new MockBuffer()}; + MockBuffer& buffer_{*buffer_ptr_}; +}; + INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); TEST_P(ConnectionImplTest, CloseDuringConnectCallback) { - Stats::IsolatedStoreImpl stats_store; - Event::DispatcherImpl dispatcher; - Network::TcpListenSocket socket(Network::Test::getAnyAddress(GetParam()), true); - Network::MockListenerCallbacks listener_callbacks; - Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = - dispatcher.createListener(connection_handler, socket, listener_callbacks, stats_store, - Network::ListenerOptions::listenerOptionsWithBindToPort()); - - Network::ClientConnectionPtr client_connection = - dispatcher.createClientConnection(socket.localAddress()); - MockConnectionCallbacks client_callbacks; - client_connection->addConnectionCallbacks(client_callbacks); + setUpBasicConnection(); + Buffer::OwnedImpl buffer("hello world"); - client_connection->write(buffer); - client_connection->connect(); + client_connection_->write(buffer); + client_connection_->connect(); - EXPECT_CALL(client_callbacks, onEvent(ConnectionEvent::Connected)) + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::Connected)) .WillOnce(Invoke( - [&](uint32_t) -> void { client_connection->close(ConnectionCloseType::NoFlush); })); - EXPECT_CALL(client_callbacks, onEvent(ConnectionEvent::LocalClose)); + [&](uint32_t) -> void { client_connection_->close(ConnectionCloseType::NoFlush); })); + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::LocalClose)); - Network::ConnectionPtr server_connection; - Network::MockConnectionCallbacks server_callbacks; - std::shared_ptr read_filter(new NiceMock()); - EXPECT_CALL(listener_callbacks, onNewConnection_(_)) + read_filter_.reset(new NiceMock()); + EXPECT_CALL(listener_callbacks_, onNewConnection_(_)) .WillOnce(Invoke([&](Network::ConnectionPtr& conn) -> void { - server_connection = std::move(conn); - server_connection->addConnectionCallbacks(server_callbacks); - server_connection->addReadFilter(read_filter); + server_connection_ = std::move(conn); + server_connection_->addConnectionCallbacks(server_callbacks_); + server_connection_->addReadFilter(read_filter_); })); - EXPECT_CALL(server_callbacks, onEvent(ConnectionEvent::RemoteClose)) - .WillOnce(Invoke([&](uint32_t) -> void { dispatcher.exit(); })); + EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::RemoteClose)) + .WillOnce(Invoke([&](uint32_t) -> void { dispatcher_.exit(); })); - dispatcher.run(Event::Dispatcher::RunType::Block); + dispatcher_.run(Event::Dispatcher::RunType::Block); } struct MockBufferStats { @@ -122,27 +163,16 @@ struct MockBufferStats { }; TEST_P(ConnectionImplTest, BufferStats) { - Stats::IsolatedStoreImpl stats_store; - Event::DispatcherImpl dispatcher; - Network::TcpListenSocket socket(Network::Test::getAnyAddress(GetParam()), true); - Network::MockListenerCallbacks listener_callbacks; - Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = - dispatcher.createListener(connection_handler, socket, listener_callbacks, stats_store, - Network::ListenerOptions::listenerOptionsWithBindToPort()); - - Network::ClientConnectionPtr client_connection = - dispatcher.createClientConnection(socket.localAddress()); - MockConnectionCallbacks client_callbacks; - client_connection->addConnectionCallbacks(client_callbacks); + setUpBasicConnection(); + MockBufferStats client_buffer_stats; - client_connection->setBufferStats(client_buffer_stats.toBufferStats()); - client_connection->connect(); + client_connection_->setBufferStats(client_buffer_stats.toBufferStats()); + client_connection_->connect(); std::shared_ptr write_filter(new MockWriteFilter()); std::shared_ptr filter(new MockFilter()); - client_connection->addWriteFilter(write_filter); - client_connection->addFilter(filter); + client_connection_->addWriteFilter(write_filter); + client_connection_->addFilter(filter); Sequence s1; EXPECT_CALL(*write_filter, onWrite(_)) @@ -150,101 +180,303 @@ TEST_P(ConnectionImplTest, BufferStats) { .WillOnce(Return(FilterStatus::StopIteration)); EXPECT_CALL(*write_filter, onWrite(_)).InSequence(s1).WillOnce(Return(FilterStatus::Continue)); EXPECT_CALL(*filter, onWrite(_)).InSequence(s1).WillOnce(Return(FilterStatus::Continue)); - EXPECT_CALL(client_callbacks, onEvent(ConnectionEvent::Connected)).InSequence(s1); + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::Connected)).InSequence(s1); EXPECT_CALL(client_buffer_stats.tx_total_, add(4)).InSequence(s1); - Network::ConnectionPtr server_connection; - Network::MockConnectionCallbacks server_callbacks; + read_filter_.reset(new NiceMock()); MockBufferStats server_buffer_stats; - std::shared_ptr read_filter(new MockReadFilter()); - EXPECT_CALL(listener_callbacks, onNewConnection_(_)) + EXPECT_CALL(listener_callbacks_, onNewConnection_(_)) .WillOnce(Invoke([&](Network::ConnectionPtr& conn) -> void { - server_connection = std::move(conn); - server_connection->addConnectionCallbacks(server_callbacks); - server_connection->setBufferStats(server_buffer_stats.toBufferStats()); - server_connection->addReadFilter(read_filter); - EXPECT_EQ("", server_connection->nextProtocol()); + server_connection_ = std::move(conn); + server_connection_->addConnectionCallbacks(server_callbacks_); + server_connection_->setBufferStats(server_buffer_stats.toBufferStats()); + server_connection_->addReadFilter(read_filter_); + EXPECT_EQ("", server_connection_->nextProtocol()); })); Sequence s2; EXPECT_CALL(server_buffer_stats.rx_total_, add(4)).InSequence(s2); EXPECT_CALL(server_buffer_stats.rx_current_, add(4)).InSequence(s2); EXPECT_CALL(server_buffer_stats.rx_current_, sub(4)).InSequence(s2); - EXPECT_CALL(server_callbacks, onEvent(ConnectionEvent::LocalClose)).InSequence(s2); - - EXPECT_CALL(*read_filter, onNewConnection()); - EXPECT_CALL(*read_filter, onData(_)).WillOnce(Invoke([&](Buffer::Instance& data) -> FilterStatus { - data.drain(data.length()); - server_connection->close(ConnectionCloseType::FlushWrite); - return FilterStatus::StopIteration; - })); + EXPECT_CALL(server_callbacks_, onEvent(ConnectionEvent::LocalClose)).InSequence(s2); + + EXPECT_CALL(*read_filter_, onNewConnection()); + EXPECT_CALL(*read_filter_, onData(_)) + .WillOnce(Invoke([&](Buffer::Instance& data) -> FilterStatus { + data.drain(data.length()); + server_connection_->close(ConnectionCloseType::FlushWrite); + return FilterStatus::StopIteration; + })); - EXPECT_CALL(client_callbacks, onEvent(ConnectionEvent::RemoteClose)) - .WillOnce(Invoke([&](uint32_t) -> void { dispatcher.exit(); })); + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::RemoteClose)) + .WillOnce(Invoke([&](uint32_t) -> void { dispatcher_.exit(); })); Buffer::OwnedImpl data("1234"); - client_connection->write(data); - client_connection->write(data); - dispatcher.run(Event::Dispatcher::RunType::Block); + client_connection_->write(data); + client_connection_->write(data); + dispatcher_.run(Event::Dispatcher::RunType::Block); } -class ReadBufferLimitTest : public testing::TestWithParam { +// Ensure the new counter logic in ReadDisable avoids tripping asserts in ReadDisable guarding +// against actual enabling twice in a row. +TEST_P(ConnectionImplTest, ReadDisable) { + setUpBasicConnection(); + + client_connection_->readDisable(true); + client_connection_->readDisable(false); + + client_connection_->readDisable(true); + client_connection_->readDisable(true); + client_connection_->readDisable(false); + client_connection_->readDisable(false); + + client_connection_->readDisable(true); + client_connection_->readDisable(true); + client_connection_->readDisable(false); + client_connection_->readDisable(true); + client_connection_->readDisable(false); + client_connection_->readDisable(false); + + disconnect(); +} + +// Test that as watermark levels are changed, the appropriate callbacks are triggered. +TEST_P(ConnectionImplTest, Watermarks) { + setUpBasicConnection(); + + std::unique_ptr buffer(new Buffer::OwnedImpl("hello")); + int buffer_len = buffer->length(); + dynamic_cast(client_connection_.get()) + ->replaceWriteBufferForTest(std::move(buffer)); + + { + // Go from watermarks being off to being above the high watermark. + EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()); + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(0); + client_connection_->setWriteBufferWatermarks(1, buffer_len - 2); + } + + { + // Go from above the high watermark to in between both. + EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()).Times(0); + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(0); + client_connection_->setWriteBufferWatermarks(1, buffer_len + 2); + } + + { + // Go from above the high watermark to below the low watermark. + EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()).Times(0); + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()); + client_connection_->setWriteBufferWatermarks(buffer_len + 1, buffer_len * 2); + } + + { + // Go back in between and verify neither callback is called. + EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()).Times(0); + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(0); + client_connection_->setWriteBufferWatermarks(buffer_len - 2, buffer_len * 3); + } + + disconnect(); +} + +// Write some data to the connection. It will automatically attempt to flush +// it to the upstream file descriptor via a write() call to buffer_, which is +// configured to succeed and accept all bytes read. +TEST_P(ConnectionImplTest, BasicWrite) { + setUpBasicConnection(); + + connect(); + + useMockBuffer(); + + // Send the data to the connection and verify it is sent upstream. + std::string data_to_write = "hello world"; + Buffer::OwnedImpl buffer_to_write(data_to_write); + std::string data_written; + EXPECT_CALL(buffer_, move(_)) + .WillRepeatedly(DoAll(AddBufferToStringWithoutDraining(&data_written), + Invoke(&buffer_, &MockBuffer::BaseMove))); + EXPECT_CALL(buffer_, write(_)).WillOnce(Invoke(&buffer_, &MockBuffer::TrackWrites)); + client_connection_->write(buffer_to_write); + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); + EXPECT_EQ(data_to_write, data_written); + + disconnect(); +} + +// Similar to BasicWrite, only with watermarks set. +TEST_P(ConnectionImplTest, WriteWithWatermarks) { + setUpBasicConnection(); + + connect(); + + useMockBuffer(); + + client_connection_->setWriteBufferWatermarks(1, 2); + + std::string data_to_write = "hello world"; + Buffer::OwnedImpl first_buffer_to_write(data_to_write); + std::string data_written; + EXPECT_CALL(buffer_, move(_)) + .WillRepeatedly(DoAll(AddBufferToStringWithoutDraining(&data_written), + Invoke(&buffer_, &MockBuffer::BaseMove))); + EXPECT_CALL(buffer_, write(_)).WillOnce(Invoke(&buffer_, &MockBuffer::TrackWrites)); + // The write() call on the connection will buffer enough data to bring the connection above the + // high watermark but the subsequent drain immediately brings it back below. + // A nice future performance optimization would be to latch if the socket is writable in the + // connection_impl, and try an immediate drain inside of write() to avoid thrashing here. + EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()); + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()); + client_connection_->write(first_buffer_to_write); + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); + EXPECT_EQ(data_to_write, data_written); + + // Now do the write again, but this time configure buffer_ to reject the write + // with errno set to EAGAIN via FailWrite(). This should result in going above the high + // watermark and not returning. + Buffer::OwnedImpl second_buffer_to_write(data_to_write); + EXPECT_CALL(buffer_, move(_)) + .WillRepeatedly(DoAll(AddBufferToStringWithoutDraining(&data_written), + Invoke(&buffer_, &MockBuffer::BaseMove))); + EXPECT_CALL(buffer_, write(_)).WillOnce(Invoke(&buffer_, &MockBuffer::FailWrite)); + // The write() call on the connection will buffer enough data to bring the connection above the + // high watermark and as the data will not flush it should not return below the watermark. + EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()); + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(0); + client_connection_->write(second_buffer_to_write); + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); + + // Clean up the connection. The close() will attempt to flush. The call to + // write() will succeed, bringing the connection back under the low watermark. + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::LocalClose)); + EXPECT_CALL(buffer_, write(_)).WillOnce(Invoke(&buffer_, &MockBuffer::TrackWrites)); + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(1); + client_connection_->close(ConnectionCloseType::NoFlush); + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); +} + +// Read and write random bytes and ensure we don't encounter issues. +TEST_P(ConnectionImplTest, WatermarkFuzzing) { + setUpBasicConnection(); + + connect(); + useMockBuffer(); + client_connection_->setWriteBufferWatermarks(5, 10); + + Runtime::RandomGeneratorImpl rand; + int bytes_buffered = 0; + int new_bytes_buffered = 0; + + bool is_below = true; + bool is_above = false; + + ON_CALL(buffer_, write(testing::_)) + .WillByDefault(testing::Invoke(&buffer_, &MockBuffer::FailWrite)); + + // Randomly write 1-20 bytes and read 1-30 bytes per loop. + for (int i = 0; i < 50; ++i) { + // The bytes to read this loop. + int bytes_to_write = rand.random() % 20 + 1; + // The bytes buffered at the begining of this loop. + bytes_buffered = new_bytes_buffered; + // Bytes to flush upstream. + int bytes_to_flush = std::min(rand.random() % 30 + 1, bytes_to_write + bytes_buffered); + // The number of bytes buffered at the end of this loop. + new_bytes_buffered = bytes_buffered + bytes_to_write - bytes_to_flush; + ENVOY_LOG_MISC(trace, + "Loop iteration {} bytes_to_write {} bytes_to_flush {} bytes_buffered is {} and " + "will be be {}", + i, bytes_to_write, bytes_to_flush, bytes_buffered, new_bytes_buffered); + + std::string data(bytes_to_write, 'a'); + Buffer::OwnedImpl buffer_to_write(data); + + // 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 > 10 && is_below) { + ENVOY_LOG_MISC(trace, "Expect onAboveWriteBufferHighWatermark"); + EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()); + is_below = false; + is_above = true; + } + // If after the bytes are flushed upstream the number of bytes remaining is + // below the low watermark and the bytes were not previously below the low + // watermark, expect the callback for going below. + if (new_bytes_buffered < 5 && is_above) { + ENVOY_LOG_MISC(trace, "Expect onBelowWriteBufferLowWatermark"); + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()); + is_below = true; + is_above = false; + } + + // Do the actual work. Write |buffer_to_write| bytes to the connection and + // drain |bytes_to_flush| before having the buffer FailWrite() + EXPECT_CALL(buffer_, move(_)).WillOnce(Invoke(&buffer_, &MockBuffer::BaseMove)); + EXPECT_CALL(buffer_, write(_)) + .WillOnce(DoAll(Invoke([&](int) -> void { buffer_.drain(bytes_to_flush); }), + Return(bytes_to_flush))) + .WillRepeatedly(testing::Invoke(&buffer_, &MockBuffer::FailWrite)); + client_connection_->write(buffer_to_write); + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); + } + + disconnect(); +} + +// TODO(alyssar) ensure we have a test where we drain the full read buffer, +// readDisable, new data is sent (and not read) we readEnable and resume. + +class ReadBufferLimitTest : public ConnectionImplTest { public: void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size) { const uint32_t buffer_size = 256 * 1024; - - Stats::IsolatedStoreImpl stats_store; - Event::DispatcherImpl dispatcher; - Network::TcpListenSocket socket(Network::Test::getAnyAddress(GetParam()), true); - Network::MockListenerCallbacks listener_callbacks; - Network::MockConnectionHandler connection_handler; - Network::ListenerPtr listener = - dispatcher.createListener(connection_handler, socket, listener_callbacks, stats_store, - {.bind_to_port_ = true, - .use_proxy_proto_ = false, - .use_original_dst_ = false, - .per_connection_buffer_limit_bytes_ = read_buffer_limit}); - - Network::ClientConnectionPtr client_connection = - dispatcher.createClientConnection(socket.localAddress()); - client_connection->connect(); - - Network::ConnectionPtr server_connection; - std::shared_ptr read_filter(new MockReadFilter()); - EXPECT_CALL(listener_callbacks, onNewConnection_(_)) + listener_ = + dispatcher_.createListener(connection_handler_, socket_, listener_callbacks_, stats_store_, + {.bind_to_port_ = true, + .use_proxy_proto_ = false, + .use_original_dst_ = false, + .per_connection_buffer_limit_bytes_ = read_buffer_limit}); + + client_connection_ = dispatcher_.createClientConnection(socket_.localAddress()); + client_connection_->connect(); + + read_filter_.reset(new NiceMock()); + EXPECT_CALL(listener_callbacks_, onNewConnection_(_)) .WillOnce(Invoke([&](Network::ConnectionPtr& conn) -> void { - server_connection = std::move(conn); - server_connection->addReadFilter(read_filter); - EXPECT_EQ("", server_connection->nextProtocol()); - EXPECT_EQ(read_buffer_limit, server_connection->readBufferLimit()); + server_connection_ = std::move(conn); + server_connection_->addReadFilter(read_filter_); + EXPECT_EQ("", server_connection_->nextProtocol()); + EXPECT_EQ(read_buffer_limit, server_connection_->readBufferLimit()); })); uint32_t filter_seen = 0; - EXPECT_CALL(*read_filter, onNewConnection()); - EXPECT_CALL(*read_filter, onData(_)) + EXPECT_CALL(*read_filter_, onNewConnection()); + EXPECT_CALL(*read_filter_, onData(_)) .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> FilterStatus { EXPECT_EQ(expected_chunk_size, data.length()); filter_seen += data.length(); data.drain(data.length()); if (filter_seen == buffer_size) { - server_connection->close(ConnectionCloseType::FlushWrite); + server_connection_->close(ConnectionCloseType::FlushWrite); } return FilterStatus::StopIteration; })); - MockConnectionCallbacks client_callbacks; - client_connection->addConnectionCallbacks(client_callbacks); - EXPECT_CALL(client_callbacks, onEvent(ConnectionEvent::Connected)); - EXPECT_CALL(client_callbacks, onEvent(ConnectionEvent::RemoteClose)) + MockConnectionCallbacks client_callbacks_; + client_connection_->addConnectionCallbacks(client_callbacks_); + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::Connected)); + EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::RemoteClose)) .WillOnce(Invoke([&](uint32_t) -> void { EXPECT_EQ(buffer_size, filter_seen); - dispatcher.exit(); + dispatcher_.exit(); })); Buffer::OwnedImpl data(std::string(buffer_size, 'a')); - client_connection->write(data); - dispatcher.run(Event::Dispatcher::RunType::Block); + client_connection_->write(data); + dispatcher_.run(Event::Dispatcher::RunType::Block); } }; diff --git a/test/config/integration/tcp_proxy.json b/test/config/integration/tcp_proxy.json index ee0311f9ec672..cdd929aaf3838 100644 --- a/test/config/integration/tcp_proxy.json +++ b/test/config/integration/tcp_proxy.json @@ -3,8 +3,7 @@ { "address": "tcp://{{ ip_loopback_address }}:0", "filters": [ - { "type": "read", "name": - "tcp_proxy", + { "type": "read", "name": "tcp_proxy", "config": { "stat_prefix": "test_tcp", "route_config": { @@ -20,6 +19,25 @@ }, { "address": "tcp://{{ ip_loopback_address }}:0", + "per_connection_buffer_limit_bytes": 1024, + "filters": [ + { "type": "read", "name": "tcp_proxy", + "config": { + "stat_prefix": "tcp_with_write_limits", + "route_config": { + "routes": [ + { + "cluster": "cluster_with_buffer_limits" + } + ] + } + } + } + ] + }, + { + "address": "tcp://{{ ip_loopback_address }}:0", + "per_connection_buffer_limit_bytes": 1024, "ssl_context": { "ca_cert_file": "{{ test_rundir }}/test/config/integration/certs/cacert.pem", "cert_chain_file": "{{ test_rundir }}/test/config/integration/certs/servercert.pem", @@ -34,7 +52,7 @@ "route_config": { "routes": [ { - "cluster": "cluster_1" + "cluster": "cluster_with_buffer_limits" } ] } @@ -68,7 +86,15 @@ "type": "strict_dns", "lb_type": "round_robin", "dns_lookup_family": "{{ dns_lookup_family }}", - "hosts": [{"url": "tcp://localhost:{{ upstream_1 }}"}] + "hosts": [{"url": "tcp://localhost:{{ ssl_auth }}"}] + }, + { + "name": "cluster_with_buffer_limits", + "connect_timeout_ms": 5000, + "type": "static", + "lb_type": "round_robin", + "per_connection_buffer_limit_bytes": 1024, + "hosts": [{"url": "tcp://{{ ip_loopback_address }}:{{ cluster_with_buffer_limits }}"}] }] } } diff --git a/test/integration/BUILD b/test/integration/BUILD index 3f7b7e0f8d05c..e33a7962e8b49 100644 --- a/test/integration/BUILD +++ b/test/integration/BUILD @@ -162,6 +162,7 @@ envoy_cc_test_library( "//source/server/config/network:redis_proxy_lib", "//source/server/config/network:tcp_proxy_lib", "//source/server/http:health_check_lib", + "//test/mocks/buffer:buffer_mocks", "//test/mocks/upstream:upstream_mocks", "//test/test_common:environment_lib", "//test/test_common:network_utility_lib", diff --git a/test/integration/fake_upstream.h b/test/integration/fake_upstream.h index 1bb9c3cec1bdc..8e118fb62838a 100644 --- a/test/integration/fake_upstream.h +++ b/test/integration/fake_upstream.h @@ -89,10 +89,12 @@ class QueuedConnectionWrapper : public Network::ConnectionCallbacks { Network::Connection& connection() const { return connection_; } // Network::ConnectionCallbacks - void onEvent(uint32_t events) { + void onEvent(uint32_t events) override { RELEASE_ASSERT(parented_ || (!(events & Network::ConnectionEvent::RemoteClose) && !(events & Network::ConnectionEvent::LocalClose))); } + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} private: Network::Connection& connection_; @@ -112,6 +114,8 @@ class FakeConnectionBase : public Network::ConnectionCallbacks { // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} protected: FakeConnectionBase(QueuedConnectionWrapperPtr connection_wrapper) diff --git a/test/integration/integration.cc b/test/integration/integration.cc index be18c8dc2a2e8..981aff2fc30a5 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -15,6 +15,7 @@ #include "common/api/api_impl.h" #include "common/buffer/buffer_impl.h" #include "common/common/assert.h" +#include "common/network/connection_impl.h" #include "common/network/utility.h" #include "common/upstream/upstream_impl.h" @@ -28,6 +29,8 @@ #include "gtest/gtest.h" #include "spdlog/spdlog.h" +using testing::_; + namespace Envoy { IntegrationStreamDecoder::IntegrationStreamDecoder(Event::Dispatcher& dispatcher) : dispatcher_(dispatcher) {} @@ -182,6 +185,10 @@ IntegrationTcpClient::IntegrationTcpClient(Event::Dispatcher& dispatcher, uint32 callbacks_(new ConnectionCallbacks(*this)) { connection_ = dispatcher.createClientConnection(Network::Utility::resolveUrl( fmt::format("tcp://{}:{}", Network::Test::getLoopbackAddressUrlString(version), port))); + + dynamic_cast(connection_.get()) + ->replaceWriteBufferForTest(std::move(buffer_ptr_)); + connection_->addConnectionCallbacks(*callbacks_); connection_->addReadFilter(payload_reader_); connection_->connect(); @@ -205,9 +212,15 @@ void IntegrationTcpClient::waitForDisconnect() { void IntegrationTcpClient::write(const std::string& data) { Buffer::OwnedImpl buffer(data); + EXPECT_CALL(buffer_, move(_)).Times(1); + EXPECT_CALL(buffer_, write(_)).Times(1); + + int bytes_expected = buffer_.bytes_written() + data.size(); + connection_->write(buffer); - connection_->dispatcher().run(Event::Dispatcher::RunType::NonBlock); - // NOTE: We should run blocking until all the body data is flushed. + while (buffer_.bytes_written() != bytes_expected) { + connection_->dispatcher().run(Event::Dispatcher::RunType::NonBlock); + } } void IntegrationTcpClient::ConnectionCallbacks::onEvent(uint32_t events) { diff --git a/test/integration/integration.h b/test/integration/integration.h index 3a0719063a6f4..81fbd74c8e1d6 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -14,6 +14,7 @@ #include "test/integration/fake_upstream.h" #include "test/integration/server.h" #include "test/integration/utility.h" +#include "test/mocks/buffer/mocks.h" #include "test/test_common/environment.h" #include "test/test_common/printers.h" @@ -87,6 +88,8 @@ class IntegrationCodecClient : public Http::CodecClientProd { // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} IntegrationCodecClient& parent_; }; @@ -131,6 +134,8 @@ class IntegrationTcpClient { // Network::ConnectionCallbacks void onEvent(uint32_t events) override; + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} IntegrationTcpClient& parent_; }; @@ -139,6 +144,8 @@ class IntegrationTcpClient { std::shared_ptr callbacks_; Network::ClientConnectionPtr connection_; bool disconnected_{}; + std::unique_ptr buffer_ptr_{new MockBuffer()}; + MockBuffer& buffer_{*buffer_ptr_}; }; typedef std::unique_ptr IntegrationTcpClientPtr; diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 48705282080a1..d09d78718fc4d 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -64,9 +64,31 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyDownstreamDisconnect) { }); } +TEST_P(TcpProxyIntegrationTest, TcpProxyLargeWrite) { + IntegrationTcpClientPtr tcp_client; + FakeRawConnectionPtr fake_upstream_connection; + FakeRawConnectionPtr fake_rest_connection; + std::string data(1024 * 16, 'a'); + executeActions({ + [&]() -> void { tcp_client = makeTcpConnection(lookupPort("tcp_proxy_with_write_limits")); }, + [&]() -> void { tcp_client->write(data); }, + [&]() -> void { fake_upstream_connection = fake_upstreams_[2]->waitForRawConnection(); }, + [&]() -> void { fake_upstream_connection->waitForData(data.size()); }, + [&]() -> void { fake_upstream_connection->write(data); }, + [&]() -> void { tcp_client->waitForData(data); }, [&]() -> void { tcp_client->close(); }, + [&]() -> void { fake_upstream_connection->waitForDisconnect(); }, + + // Clean up unused client_ssl_auth + [&]() -> void { fake_rest_connection = fake_upstreams_[1]->waitForRawConnection(); }, + [&]() -> void { fake_rest_connection->close(); }, + [&]() -> void { fake_rest_connection->waitForDisconnect(); }, + }); +} + // Test proxying data in both directions with envoy doing TCP and TLS // termination. -TEST_P(TcpProxyIntegrationTest, SendTlsToTlsListener) { +void TcpProxyIntegrationTest::SendAndReceiveTlsData(const std::string& data_to_send_upstream, + const std::string& data_to_send_downstream) { Network::ClientConnectionPtr ssl_client; FakeRawConnectionPtr fake_upstream_connection; FakeHttpConnectionPtr fake_rest_connection; @@ -104,20 +126,20 @@ TEST_P(TcpProxyIntegrationTest, SendTlsToTlsListener) { }, // Ship some data upstream. [&]() -> void { - Buffer::OwnedImpl buffer("hello"); + Buffer::OwnedImpl buffer(data_to_send_upstream); ssl_client->write(buffer); dispatcher_->run(Event::Dispatcher::RunType::NonBlock); }, // Make sure the data makes it upstream. - [&]() -> void { fake_upstream_connection = fake_upstreams_[0]->waitForRawConnection(); }, - [&]() -> void { fake_upstream_connection->waitForData(5); }, + [&]() -> void { fake_upstream_connection = fake_upstreams_[2]->waitForRawConnection(); }, + [&]() -> void { fake_upstream_connection->waitForData(data_to_send_upstream.size()); }, // Now send data downstream and make sure it arrives. [&]() -> void { std::shared_ptr payload_reader( new WaitForPayloadReader(*dispatcher_)); ssl_client->addReadFilter(payload_reader); - fake_upstream_connection->write("world"); - payload_reader->set_data_to_wait_for("world"); + fake_upstream_connection->write(data_to_send_downstream); + payload_reader->set_data_to_wait_for(data_to_send_downstream); ssl_client->dispatcher().run(Event::Dispatcher::RunType::Block); }, // Clean up. @@ -129,5 +151,12 @@ TEST_P(TcpProxyIntegrationTest, SendTlsToTlsListener) { }); } +TEST_P(TcpProxyIntegrationTest, SendTlsToTlsListener) { SendAndReceiveTlsData("hello", "world"); } + +TEST_P(TcpProxyIntegrationTest, LargeBidirectionalTlsWrites) { + std::string large_data(1024 * 8, 'a'); + SendAndReceiveTlsData(large_data, large_data); +} + } // namespace } // namespace Envoy diff --git a/test/integration/tcp_proxy_integration_test.h b/test/integration/tcp_proxy_integration_test.h index c7b4da7d08d4d..ba3ca0c3cb502 100644 --- a/test/integration/tcp_proxy_integration_test.h +++ b/test/integration/tcp_proxy_integration_test.h @@ -8,6 +8,7 @@ #include "gtest/gtest.h" namespace Envoy { +namespace { class TcpProxyIntegrationTest : public BaseIntegrationTest, public testing::TestWithParam { public: @@ -17,14 +18,22 @@ class TcpProxyIntegrationTest : public BaseIntegrationTest, fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_)); registerPort("upstream_0", fake_upstreams_.back()->localAddress()->ip()->port()); fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_)); - registerPort("upstream_1", fake_upstreams_.back()->localAddress()->ip()->port()); - createTestServer("test/config/integration/tcp_proxy.json", - {"tcp_proxy", "tcp_proxy_with_tls_termination"}); + registerPort("ssl_auth", fake_upstreams_.back()->localAddress()->ip()->port()); + fake_upstreams_.emplace_back(new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_)); + registerPort("cluster_with_buffer_limits", + fake_upstreams_.back()->localAddress()->ip()->port()); + createTestServer( + "test/config/integration/tcp_proxy.json", + {"tcp_proxy", "tcp_proxy_with_write_limits", "tcp_proxy_with_tls_termination"}); } void TearDown() override { test_server_.reset(); fake_upstreams_.clear(); } + + void SendAndReceiveTlsData(const std::string& data_to_send_upstream, + const std::string& data_to_send_downstream); }; +} // namespace } // namespace Envoy diff --git a/test/integration/utility.h b/test/integration/utility.h index 2b76e11a102d1..d379ded8f73bf 100644 --- a/test/integration/utility.h +++ b/test/integration/utility.h @@ -110,11 +110,13 @@ class ConnectionStatusCallbacks : public Network::ConnectionCallbacks { bool closed() const { return closed_; } // Network::ConnectionCallbacks - void onEvent(uint32_t events) { + void onEvent(uint32_t events) override { closed_ |= (events & Network::ConnectionEvent::RemoteClose || events & Network::ConnectionEvent::LocalClose); connected_ |= events & Network::ConnectionEvent::Connected; } + void onAboveWriteBufferHighWatermark() override {} + void onBelowWriteBufferLowWatermark() override {} private: bool connected_{false}; diff --git a/test/mocks/buffer/mocks.h b/test/mocks/buffer/mocks.h index e82e33d5c539d..9d9d62e0c2139 100644 --- a/test/mocks/buffer/mocks.h +++ b/test/mocks/buffer/mocks.h @@ -8,6 +8,40 @@ #include "gmock/gmock.h" namespace Envoy { + +class MockBuffer : public Buffer::OwnedImpl { +public: + MockBuffer() { + ON_CALL(*this, write(testing::_)) + .WillByDefault(testing::Invoke(this, &MockBuffer::TrackWrites)); + ON_CALL(*this, move(testing::_)).WillByDefault(testing::Invoke(this, &MockBuffer::BaseMove)); + } + + MOCK_METHOD1(write, int(int fd)); + MOCK_METHOD1(move, void(Instance& rhs)); + MOCK_METHOD2(move, void(Instance& rhs, uint64_t length)); + + void BaseMove(Instance& rhs) { Buffer::OwnedImpl::move(rhs); } + + int TrackWrites(int fd) { + int bytes_written = Buffer::OwnedImpl::write(fd); + if (bytes_written > 0) { + bytes_written_ += bytes_written; + } + return bytes_written; + } + + int FailWrite(int) { + errno = EAGAIN; + return -1; + } + + int bytes_written() const { return bytes_written_; } + +private: + int bytes_written_{0}; +}; + MATCHER_P(BufferEqual, rhs, testing::PrintToString(*rhs)) { return TestUtility::buffersEqual(arg, *rhs); } @@ -23,4 +57,9 @@ ACTION_P(AddBufferToString, target_string) { target_string->append(TestUtility::bufferToString(arg0)); arg0.drain(arg0.length()); } + +ACTION_P(AddBufferToStringWithoutDraining, target_string) { + target_string->append(TestUtility::bufferToString(arg0)); +} + } // namespace Envoy diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index b34887a30fc07..aba3db9272438 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -23,6 +23,8 @@ class MockConnectionCallbacks : public ConnectionCallbacks { // Network::ConnectionCallbacks MOCK_METHOD1(onEvent, void(uint32_t events)); + MOCK_METHOD0(onAboveWriteBufferHighWatermark, void()); + MOCK_METHOD0(onBelowWriteBufferLowWatermark, void()); }; class MockConnectionBase { @@ -65,6 +67,7 @@ class MockConnection : public Connection, public MockConnectionBase { MOCK_METHOD1(write, void(Buffer::Instance& data)); MOCK_METHOD1(setReadBufferLimit, void(uint32_t limit)); MOCK_CONST_METHOD0(readBufferLimit, uint32_t()); + MOCK_METHOD2(setWriteBufferWatermarks, void(size_t low_watermark, size_t high_watermark)); }; /** @@ -97,6 +100,7 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase MOCK_METHOD1(write, void(Buffer::Instance& data)); MOCK_METHOD1(setReadBufferLimit, void(uint32_t limit)); MOCK_CONST_METHOD0(readBufferLimit, uint32_t()); + MOCK_METHOD2(setWriteBufferWatermarks, void(size_t low_watermark, size_t high_watermark)); // Network::ClientConnection MOCK_METHOD0(connect, void()); From f2ffecb89571be5e69e9f6388bcb48581d2864b8 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 6 Jul 2017 13:01:35 -0400 Subject: [PATCH 02/28] reinstating a check --- source/common/network/connection_impl.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index d16799a8b8371..ee1476544c5bc 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -211,6 +211,7 @@ void ConnectionImpl::readDisable(bool disable) { ++read_disable_count_; return; } + ASSERT(read_enabled); state_ &= ~InternalState::ReadEnabled; file_event_->setEnabled(Event::FileReadyType::Write | Event::FileReadyType::Closed); } else { From 0a9116d345456c6d8a8f96f9fcd5b1a27bcdbc85 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 6 Jul 2017 14:37:21 -0400 Subject: [PATCH 03/28] Adding SSL unit tests --- test/common/ssl/BUILD | 1 + test/common/ssl/connection_impl_test.cc | 165 ++++++++++++++++-------- test/mocks/buffer/mocks.h | 2 + 3 files changed, 115 insertions(+), 53 deletions(-) diff --git a/test/common/ssl/BUILD b/test/common/ssl/BUILD index 44b70417a0258..2bff16c4cc947 100644 --- a/test/common/ssl/BUILD +++ b/test/common/ssl/BUILD @@ -29,6 +29,7 @@ envoy_cc_test( "//source/common/ssl:context_config_lib", "//source/common/ssl:context_lib", "//source/common/stats:stats_lib", + "//test/mocks/buffer:buffer_mocks", "//test/mocks/network:network_mocks", "//test/mocks/runtime:runtime_mocks", "//test/mocks/server:server_mocks", diff --git a/test/common/ssl/connection_impl_test.cc b/test/common/ssl/connection_impl_test.cc index a779dd566ff6f..a2165950c1614 100644 --- a/test/common/ssl/connection_impl_test.cc +++ b/test/common/ssl/connection_impl_test.cc @@ -13,6 +13,7 @@ #include "common/stats/stats_impl.h" #include "test/common/ssl/ssl_certs_test.h" +#include "test/mocks/buffer/mocks.h" #include "test/mocks/network/mocks.h" #include "test/mocks/runtime/mocks.h" #include "test/mocks/server/mocks.h" @@ -278,80 +279,61 @@ TEST_P(SslConnectionImplTest, SslError) { class SslReadBufferLimitTest : public SslCertsTest, public testing::WithParamInterface { public: - void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size, - uint32_t write_size, uint32_t num_writes, bool reserve_write_space) { - Stats::IsolatedStoreImpl stats_store; - Event::DispatcherImpl dispatcher; - Network::TcpListenSocket socket(Network::Test::getCanonicalLoopbackAddress(GetParam()), true); - Network::MockListenerCallbacks listener_callbacks; - Network::MockConnectionHandler connection_handler; - - std::string server_ctx_json = R"EOF( - { - "cert_chain_file": "{{ test_tmpdir }}/unittestcert.pem", - "private_key_file": "{{ test_tmpdir }}/unittestkey.pem", - "ca_cert_file": "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" - } - )EOF"; - Json::ObjectSharedPtr server_ctx_loader = TestEnvironment::jsonLoadFromString(server_ctx_json); - ContextConfigImpl server_ctx_config(*server_ctx_loader); - Runtime::MockLoader runtime; - ContextManagerImpl manager(runtime); - ServerContextPtr server_ctx(manager.createSslServerContext(stats_store, server_ctx_config)); - - Network::ListenerPtr listener = dispatcher.createSslListener( - connection_handler, *server_ctx, socket, listener_callbacks, stats_store, + void Initialize(uint32_t read_buffer_limit) { + server_ctx_loader_ = TestEnvironment::jsonLoadFromString(server_ctx_json_); + server_ctx_config_.reset(new ContextConfigImpl(*server_ctx_loader_)); + manager_.reset(new ContextManagerImpl(runtime_)); + server_ctx_ = manager_->createSslServerContext(stats_store_, *server_ctx_config_); + + listener_ = dispatcher_.createSslListener( + connection_handler_, *server_ctx_, socket_, listener_callbacks_, stats_store_, {.bind_to_port_ = true, .use_proxy_proto_ = false, .use_original_dst_ = false, .per_connection_buffer_limit_bytes_ = read_buffer_limit}); - std::string client_ctx_json = R"EOF( - { - "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/no_san_cert.pem", - "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/no_san_key.pem" - } - )EOF"; + client_ctx_loader_ = TestEnvironment::jsonLoadFromString(client_ctx_json_); + client_ctx_config_.reset(new ContextConfigImpl(*client_ctx_loader_)); + client_ctx_ = manager_->createSslClientContext(stats_store_, *client_ctx_config_); - Json::ObjectSharedPtr client_ctx_loader = TestEnvironment::jsonLoadFromString(client_ctx_json); - ContextConfigImpl client_ctx_config(*client_ctx_loader); - ClientContextPtr client_ctx(manager.createSslClientContext(stats_store, client_ctx_config)); + client_connection_ = + dispatcher_.createSslClientConnection(*client_ctx_, socket_.localAddress()); + client_connection_->connect(); + read_filter_.reset(new Network::MockReadFilter()); + client_connection_->addConnectionCallbacks(client_callbacks_); + EXPECT_CALL(client_callbacks_, onEvent(Network::ConnectionEvent::Connected)); + } - Network::ClientConnectionPtr client_connection = - dispatcher.createSslClientConnection(*client_ctx, socket.localAddress()); - client_connection->connect(); + void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size, + uint32_t write_size, uint32_t num_writes, bool reserve_write_space) { + Initialize(read_buffer_limit); - Network::ConnectionPtr server_connection; - std::shared_ptr read_filter(new Network::MockReadFilter()); - EXPECT_CALL(listener_callbacks, onNewConnection_(_)) + EXPECT_CALL(listener_callbacks_, onNewConnection_(_)) .WillOnce(Invoke([&](Network::ConnectionPtr& conn) -> void { - server_connection = std::move(conn); - server_connection->addReadFilter(read_filter); - EXPECT_EQ("", server_connection->nextProtocol()); - EXPECT_EQ(read_buffer_limit, server_connection->readBufferLimit()); + server_connection_ = std::move(conn); + server_connection_->addReadFilter(read_filter_); + EXPECT_EQ("", server_connection_->nextProtocol()); + EXPECT_EQ(read_buffer_limit, server_connection_->readBufferLimit()); })); uint32_t filter_seen = 0; - EXPECT_CALL(*read_filter, onNewConnection()); - EXPECT_CALL(*read_filter, onData(_)) + EXPECT_CALL(*read_filter_, onNewConnection()); + EXPECT_CALL(*read_filter_, onData(_)) .WillRepeatedly(Invoke([&](Buffer::Instance& data) -> Network::FilterStatus { EXPECT_GE(expected_chunk_size, data.length()); filter_seen += data.length(); data.drain(data.length()); if (filter_seen == (write_size * num_writes)) { - server_connection->close(Network::ConnectionCloseType::FlushWrite); + server_connection_->close(Network::ConnectionCloseType::FlushWrite); } return Network::FilterStatus::StopIteration; })); - Network::MockConnectionCallbacks client_callbacks; - client_connection->addConnectionCallbacks(client_callbacks); - EXPECT_CALL(client_callbacks, onEvent(Network::ConnectionEvent::Connected)); - EXPECT_CALL(client_callbacks, onEvent(Network::ConnectionEvent::RemoteClose)) + EXPECT_CALL(client_callbacks_, onEvent(Network::ConnectionEvent::RemoteClose)) .WillOnce(Invoke([&](uint32_t) -> void { EXPECT_EQ((write_size * num_writes), filter_seen); - dispatcher.exit(); + dispatcher_.exit(); })); for (uint32_t i = 0; i < num_writes; i++) { @@ -366,13 +348,86 @@ class SslReadBufferLimitTest : public SslCertsTest, data.commit(iovecs, 2); } - client_connection->write(data); + client_connection_->write(data); } - dispatcher.run(Event::Dispatcher::RunType::Block); + dispatcher_.run(Event::Dispatcher::RunType::Block); + + EXPECT_EQ(0UL, stats_store_.counter("ssl.connection_error").value()); + } + + void singleWriteTest(uint32_t read_buffer_limit, uint32_t bytes_to_write) { + Initialize(read_buffer_limit); + + // For watermark testing, stick limits on the client connection as well. + client_connection_->setWriteBufferWatermarks(read_buffer_limit / 2, read_buffer_limit + 1); + int times_called = bytes_to_write > read_buffer_limit ? 1 : 0; + EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()).Times(times_called); + EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(times_called); + + EXPECT_CALL(listener_callbacks_, onNewConnection_(_)) + .WillOnce(Invoke([&](Network::ConnectionPtr& conn) -> void { + server_connection_ = std::move(conn); + server_connection_->addReadFilter(read_filter_); + EXPECT_EQ("", server_connection_->nextProtocol()); + EXPECT_EQ(read_buffer_limit, server_connection_->readBufferLimit()); + })); - EXPECT_EQ(0UL, stats_store.counter("ssl.connection_error").value()); + std::unique_ptr buffer_ptr_{new MockBuffer()}; + MockBuffer& buffer_{*buffer_ptr_}; + dynamic_cast(client_connection_.get()) + ->replaceWriteBufferForTest(std::move(buffer_ptr_)); + + EXPECT_CALL(*read_filter_, onNewConnection()); + EXPECT_CALL(*read_filter_, onData(_)).Times(testing::AnyNumber()); + + std::string data_to_write(bytes_to_write, 'a'); + Buffer::OwnedImpl buffer_to_write(data_to_write); + std::string data_written; + EXPECT_CALL(buffer_, move(_)) + .WillRepeatedly(DoAll(AddBufferToStringWithoutDraining(&data_written), + Invoke(&buffer_, &MockBuffer::BaseMove))); + EXPECT_CALL(buffer_, drain(_)).WillOnce(Invoke(&buffer_, &MockBuffer::BaseDrain)); + client_connection_->write(buffer_to_write); + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); + EXPECT_EQ(data_to_write, data_written); + + EXPECT_CALL(client_callbacks_, onEvent(Network::ConnectionEvent::LocalClose)); + client_connection_->close(Network::ConnectionCloseType::NoFlush); + dispatcher_.run(Event::Dispatcher::RunType::NonBlock); } + + Stats::IsolatedStoreImpl stats_store_; + Event::DispatcherImpl dispatcher_; + Network::TcpListenSocket socket_{Network::Test::getCanonicalLoopbackAddress(GetParam()), true}; + Network::MockListenerCallbacks listener_callbacks_; + Network::MockConnectionHandler connection_handler_; + std::string server_ctx_json_ = R"EOF( + { + "cert_chain_file": "{{ test_tmpdir }}/unittestcert.pem", + "private_key_file": "{{ test_tmpdir }}/unittestkey.pem", + "ca_cert_file": "{{ test_rundir }}/test/common/ssl/test_data/ca_cert.pem" + } + )EOF"; + std::string client_ctx_json_ = R"EOF( + { + "cert_chain_file": "{{ test_rundir }}/test/common/ssl/test_data/no_san_cert.pem", + "private_key_file": "{{ test_rundir }}/test/common/ssl/test_data/no_san_key.pem" + } + )EOF"; + Runtime::MockLoader runtime_; + Json::ObjectSharedPtr server_ctx_loader_; + std::unique_ptr server_ctx_config_; + std::unique_ptr manager_; + ServerContextPtr server_ctx_; + Network::ListenerPtr listener_; + Json::ObjectSharedPtr client_ctx_loader_; + std::unique_ptr client_ctx_config_; + ClientContextPtr client_ctx_; + Network::ClientConnectionPtr client_connection_; + Network::ConnectionPtr server_connection_; + std::shared_ptr read_filter_; + Network::MockConnectionCallbacks client_callbacks_; }; INSTANTIATE_TEST_CASE_P(IpVersions, SslReadBufferLimitTest, @@ -392,5 +447,9 @@ TEST_P(SslReadBufferLimitTest, SomeLimit) { readBufferLimitTest(32 * 1024, 32 * 1024, 256 * 1024, 1, false); } +TEST_P(SslReadBufferLimitTest, WritesSmallerThanWatermark) { singleWriteTest(5 * 1024, 1024); } + +TEST_P(SslReadBufferLimitTest, WritesLargerThanWatermark) { singleWriteTest(1024, 5 * 1024); } + } // namespace Ssl } // namespace Envoy diff --git a/test/mocks/buffer/mocks.h b/test/mocks/buffer/mocks.h index 9d9d62e0c2139..a4765a8476186 100644 --- a/test/mocks/buffer/mocks.h +++ b/test/mocks/buffer/mocks.h @@ -20,8 +20,10 @@ class MockBuffer : public Buffer::OwnedImpl { MOCK_METHOD1(write, int(int fd)); MOCK_METHOD1(move, void(Instance& rhs)); MOCK_METHOD2(move, void(Instance& rhs, uint64_t length)); + MOCK_METHOD1(drain, void(uint64_t size)); void BaseMove(Instance& rhs) { Buffer::OwnedImpl::move(rhs); } + void BaseDrain(uint64_t size) { Buffer::OwnedImpl::drain(size); } int TrackWrites(int fd) { int bytes_written = Buffer::OwnedImpl::write(fd); From 24b56df882ea0d0951b920ec4fc1e3a0e0835cce Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 6 Jul 2017 15:02:35 -0400 Subject: [PATCH 04/28] seeing if pointers are ever OK --- test/integration/integration.cc | 12 +++++++----- test/integration/integration.h | 3 +-- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/test/integration/integration.cc b/test/integration/integration.cc index 981aff2fc30a5..9e31975edf11a 100644 --- a/test/integration/integration.cc +++ b/test/integration/integration.cc @@ -186,8 +186,10 @@ IntegrationTcpClient::IntegrationTcpClient(Event::Dispatcher& dispatcher, uint32 connection_ = dispatcher.createClientConnection(Network::Utility::resolveUrl( fmt::format("tcp://{}:{}", Network::Test::getLoopbackAddressUrlString(version), port))); + std::unique_ptr buffer{new MockBuffer()}; + buffer_ = buffer.get(); dynamic_cast(connection_.get()) - ->replaceWriteBufferForTest(std::move(buffer_ptr_)); + ->replaceWriteBufferForTest(std::move(buffer)); connection_->addConnectionCallbacks(*callbacks_); connection_->addReadFilter(payload_reader_); @@ -212,13 +214,13 @@ void IntegrationTcpClient::waitForDisconnect() { void IntegrationTcpClient::write(const std::string& data) { Buffer::OwnedImpl buffer(data); - EXPECT_CALL(buffer_, move(_)).Times(1); - EXPECT_CALL(buffer_, write(_)).Times(1); + EXPECT_CALL(*buffer_, move(_)).Times(1); + EXPECT_CALL(*buffer_, write(_)).Times(1); - int bytes_expected = buffer_.bytes_written() + data.size(); + int bytes_expected = buffer_->bytes_written() + data.size(); connection_->write(buffer); - while (buffer_.bytes_written() != bytes_expected) { + while (buffer_->bytes_written() != bytes_expected) { connection_->dispatcher().run(Event::Dispatcher::RunType::NonBlock); } } diff --git a/test/integration/integration.h b/test/integration/integration.h index 81fbd74c8e1d6..cdf6ba0aa373c 100644 --- a/test/integration/integration.h +++ b/test/integration/integration.h @@ -144,8 +144,7 @@ class IntegrationTcpClient { std::shared_ptr callbacks_; Network::ClientConnectionPtr connection_; bool disconnected_{}; - std::unique_ptr buffer_ptr_{new MockBuffer()}; - MockBuffer& buffer_{*buffer_ptr_}; + MockBuffer* buffer_; }; typedef std::unique_ptr IntegrationTcpClientPtr; From 2cb1ebf22d04d25183925785e26b91dc5882551b Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 6 Jul 2017 15:19:16 -0400 Subject: [PATCH 05/28] fixing test mocks --- test/common/network/connection_impl_test.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index 8c9698633c7ad..f45a305de6b3d 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -371,8 +371,9 @@ TEST_P(ConnectionImplTest, WatermarkFuzzing) { bool is_below = true; bool is_above = false; - ON_CALL(buffer_, write(testing::_)) - .WillByDefault(testing::Invoke(&buffer_, &MockBuffer::FailWrite)); + ON_CALL(buffer_, write(_)).WillByDefault(testing::Invoke(&buffer_, &MockBuffer::FailWrite)); + ON_CALL(buffer_, drain(_)).WillByDefault(testing::Invoke(&buffer_, &MockBuffer::BaseDrain)); + EXPECT_CALL(buffer_, drain(_)).Times(AnyNumber()); // Randomly write 1-20 bytes and read 1-30 bytes per loop. for (int i = 0; i < 50; ++i) { From f07608b438073fc0826aa785534570973289a268 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 6 Jul 2017 15:48:16 -0400 Subject: [PATCH 06/28] correcting docs --- include/envoy/network/connection.h | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 9fc9d1a6197ec..9ae12f2048de4 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -177,6 +177,10 @@ class Connection : public Event::DeferredDeletable, public FilterManager { /** * Sets the high and low watermarks which trigger onAboveWriteBufferHighWatermark * and onBelowWriteBufferHighWatermark callbacks. + * @param low_watermark if the connection was above the high watermark and the + * connection buffer is drained below this many bytes, onBelowWriteBufferHighWatermark will be called. + * @param high_watermark if the connection has more bytes than this buffered, + * onAboveWriteBufferHighWatermark will be called. */ virtual void setWriteBufferWatermarks(size_t low_watermark, size_t high_watermark) PURE; }; From 36ecdc3cb7d3dc8c46a3b3fa97a069118f4c6fe9 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 6 Jul 2017 16:46:33 -0400 Subject: [PATCH 07/28] fix_format --- include/envoy/network/connection.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 9ae12f2048de4..f8ea053b8f200 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -178,7 +178,8 @@ class Connection : public Event::DeferredDeletable, public FilterManager { * Sets the high and low watermarks which trigger onAboveWriteBufferHighWatermark * and onBelowWriteBufferHighWatermark callbacks. * @param low_watermark if the connection was above the high watermark and the - * connection buffer is drained below this many bytes, onBelowWriteBufferHighWatermark will be called. + * connection buffer is drained below this many bytes, onBelowWriteBufferHighWatermark will be + * called. * @param high_watermark if the connection has more bytes than this buffered, * onAboveWriteBufferHighWatermark will be called. */ From b846116761679bc9cf44bda08ba85627e5436843 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 10 Jul 2017 11:56:45 -0400 Subject: [PATCH 08/28] Addressing some review comments --- include/envoy/network/connection.h | 5 ++++- source/common/network/connection_impl.cc | 8 ++++---- source/common/network/connection_impl.h | 10 +++++----- test/common/network/connection_impl_test.cc | 8 ++++---- test/integration/tcp_proxy_integration_test.cc | 3 ++- test/mocks/network/mocks.h | 2 +- 6 files changed, 20 insertions(+), 16 deletions(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index f8ea053b8f200..b390c3087486a 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -177,13 +177,16 @@ class Connection : public Event::DeferredDeletable, public FilterManager { /** * Sets the high and low watermarks which trigger onAboveWriteBufferHighWatermark * and onBelowWriteBufferHighWatermark callbacks. + * The connection is assumed to start out with less than high_watermark + * worth of data buffered, so onAboveWriteBufferHighWatermark will always be + * called before onAboveWriteBufferHighWatermark * @param low_watermark if the connection was above the high watermark and the * connection buffer is drained below this many bytes, onBelowWriteBufferHighWatermark will be * called. * @param high_watermark if the connection has more bytes than this buffered, * onAboveWriteBufferHighWatermark will be called. */ - virtual void setWriteBufferWatermarks(size_t low_watermark, size_t high_watermark) PURE; + virtual void setWriteBufferWatermarks(uint32_t low_watermark, uint32_t high_watermark) PURE; }; typedef std::unique_ptr ConnectionPtr; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index ee1476544c5bc..9c9c7a23e064b 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -274,8 +274,8 @@ void ConnectionImpl::write(Buffer::Instance& data) { } } -void ConnectionImpl::setWriteBufferWatermarks(size_t new_low_watermark, size_t new_high_watermark) { - ENVOY_CONN_LOG(trace, "Setting watermarks: {} {}", *this, new_low_watermark, new_high_watermark); +void ConnectionImpl::setWriteBufferWatermarks(uint32_t new_low_watermark, uint32_t new_high_watermark) { + ENVOY_CONN_LOG(debug, "Setting watermarks: {} {}", *this, new_low_watermark, new_high_watermark); ASSERT(new_low_watermark < new_high_watermark); high_watermark_ = new_high_watermark; @@ -289,7 +289,7 @@ void ConnectionImpl::checkForLowWatermark() { if (!above_high_watermark_called_ || write_buffer_->length() >= low_watermark_) { return; } - ENVOY_CONN_LOG(trace, "onBelowWriteBufferLowWatermark", *this); + ENVOY_CONN_LOG(debug, "onBelowWriteBufferLowWatermark", *this); above_high_watermark_called_ = false; for (ConnectionCallbacks* callback : callbacks_) { @@ -302,7 +302,7 @@ void ConnectionImpl::checkForHighWatermark() { write_buffer_->length() <= high_watermark_) { return; } - ENVOY_CONN_LOG(trace, "onAboveWriteBufferHighWatermark", *this); + ENVOY_CONN_LOG(debug, "onAboveWriteBufferHighWatermark", *this); above_high_watermark_called_ = true; for (ConnectionCallbacks* callback : callbacks_) { diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 431183bbde566..bb68e99e404be 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -72,7 +72,7 @@ class ConnectionImpl : public virtual Connection, void write(Buffer::Instance& data) override; void setReadBufferLimit(uint32_t limit) override { read_buffer_limit_ = limit; } uint32_t readBufferLimit() const override { return read_buffer_limit_; } - void setWriteBufferWatermarks(size_t low_watermark, size_t high_watermark) override; + void setWriteBufferWatermarks(uint32_t low_watermark, uint32_t high_watermark) override; // Network::BufferSource Buffer::Instance& getReadBuffer() override { return read_buffer_; } @@ -115,14 +115,14 @@ class ConnectionImpl : public virtual Connection, Address::InstanceConstSharedPtr remote_address_; Address::InstanceConstSharedPtr local_address_; Buffer::OwnedImpl read_buffer_; - std::unique_ptr write_buffer_{new Buffer::OwnedImpl}; + Buffer::InstancePtr write_buffer_{new Buffer::OwnedImpl}; uint32_t read_buffer_limit_ = 0; // Used for network level buffer limits (off by default). If these are non-zero, when the write // buffer passes |high_watermark_|, onAboveWriteBufferHighWatermark will be called to disable // reading further data. When the buffer drains below |low_watermark_|, // onBelowWriteBufferLowWatermark will be called to resume reads. - size_t high_watermark_{0}; - size_t low_watermark_{0}; + uint32_t high_watermark_{0}; + uint32_t low_watermark_{0}; // Tracks the latest state of watermark callbacks. // True between the time onAboveWriteBufferHighWatermark is called until the next call to // onBelowLowWatermark. @@ -161,7 +161,7 @@ class ConnectionImpl : public virtual Connection, uint64_t last_write_buffer_size_{}; std::unique_ptr buffer_stats_; // Tracks the number of times reads have been disabled. If N different components call - // readDisabled(true) this allows the connection to only resume reasd when readDisabled(false) + // readDisabled(true) this allows the connection to only resume reads when readDisabled(false) // has been called N times. size_t read_disable_count_{0}; }; diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index f45a305de6b3d..9ee8a377faa3e 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -64,8 +64,8 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ConnectionImplDeathTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); TEST_P(ConnectionImplDeathTest, BadFd) { - Event::DispatcherImpl dispatcher_; - EXPECT_DEATH(ConnectionImpl(dispatcher_, -1, + Event::DispatcherImpl dispatcher; + EXPECT_DEATH(ConnectionImpl(dispatcher, -1, Network::Test::getCanonicalLoopbackAddress(GetParam()), Network::Test::getCanonicalLoopbackAddress(GetParam())), ".*assert failure: fd_ != -1.*"); @@ -466,8 +466,8 @@ class ReadBufferLimitTest : public ConnectionImplTest { return FilterStatus::StopIteration; })); - MockConnectionCallbacks client_callbacks_; - client_connection_->addConnectionCallbacks(client_callbacks_); + MockConnectionCallbacks client_callbacks; + client_connection_->addConnectionCallbacks(client_callbacks); EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::Connected)); EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::RemoteClose)) .WillOnce(Invoke([&](uint32_t) -> void { diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 2cb79730e62f0..ad26bcac2ba5c 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -76,7 +76,8 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyLargeWrite) { [&]() -> void { fake_upstream_connection = fake_upstreams_[2]->waitForRawConnection(); }, [&]() -> void { fake_upstream_connection->waitForData(data.size()); }, [&]() -> void { fake_upstream_connection->write(data); }, - [&]() -> void { tcp_client->waitForData(data); }, [&]() -> void { tcp_client->close(); }, + [&]() -> void { tcp_client->waitForData(data); }, + [&]() -> void { tcp_client->close(); }, [&]() -> void { fake_upstream_connection->waitForDisconnect(); }, // Clean up unused client_ssl_auth diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index aba3db9272438..2b0a06a9d5e46 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -67,7 +67,7 @@ class MockConnection : public Connection, public MockConnectionBase { MOCK_METHOD1(write, void(Buffer::Instance& data)); MOCK_METHOD1(setReadBufferLimit, void(uint32_t limit)); MOCK_CONST_METHOD0(readBufferLimit, uint32_t()); - MOCK_METHOD2(setWriteBufferWatermarks, void(size_t low_watermark, size_t high_watermark)); + MOCK_METHOD2(setWriteBufferWatermarks, void(uint32_t low_watermark, uint32_t high_watermark)); }; /** From 8fd62cae4b885072f9df9d40d41614e1fa8bb8fc Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 10 Jul 2017 16:48:30 -0400 Subject: [PATCH 09/28] Addressing the rest of the comments not handled by #1234 --- source/common/filter/tcp_proxy.cc | 8 ++++++++ source/common/filter/tcp_proxy.h | 2 ++ source/common/network/connection_impl.cc | 3 ++- source/common/network/connection_impl.h | 2 +- test/common/network/connection_impl_test.cc | 3 +-- test/mocks/network/mocks.h | 2 +- 6 files changed, 15 insertions(+), 5 deletions(-) diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index 8c596927837b7..b1a25e1c66097 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -130,21 +130,29 @@ void TcpProxy::readDisableDownstream(bool disable) { } void TcpProxy::DownstreamCallbacks::onAboveWriteBufferHighWatermark() { + ASSERT(!on_high_watermark_called_); + on_high_watermark_called_ = true; // If downstream has too much data buffered, stop reading on the upstream connection. parent_.readDisableUpstream(true); } void TcpProxy::DownstreamCallbacks::onBelowWriteBufferLowWatermark() { + ASSERT(on_high_watermark_called_); + on_high_watermark_called_ = false; // The downstream buffer has been drained. Resume reading from upstream. parent_.readDisableUpstream(false); } void TcpProxy::UpstreamCallbacks::onAboveWriteBufferHighWatermark() { + ASSERT(!on_high_watermark_called_); + on_high_watermark_called_ = true; // There's too much data buffered in the upstream write buffer, so stop reading. parent_.readDisableDownstream(true); } void TcpProxy::UpstreamCallbacks::onBelowWriteBufferLowWatermark() { + ASSERT(on_high_watermark_called_); + on_high_watermark_called_ = false; // The upstream write buffer is drained. Resume reading. parent_.readDisableDownstream(false); } diff --git a/source/common/filter/tcp_proxy.h b/source/common/filter/tcp_proxy.h index 335d30347030c..0ff1d60098270 100644 --- a/source/common/filter/tcp_proxy.h +++ b/source/common/filter/tcp_proxy.h @@ -109,6 +109,7 @@ class TcpProxy : public Network::ReadFilter, Logger::LoggableaddConnectionCallbacks(client_callbacks); + client_connection_->addConnectionCallbacks(client_callbacks_); EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::Connected)); EXPECT_CALL(client_callbacks_, onEvent(ConnectionEvent::RemoteClose)) .WillOnce(Invoke([&](uint32_t) -> void { diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index 2b0a06a9d5e46..d71e69ebb82d2 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -100,7 +100,7 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase MOCK_METHOD1(write, void(Buffer::Instance& data)); MOCK_METHOD1(setReadBufferLimit, void(uint32_t limit)); MOCK_CONST_METHOD0(readBufferLimit, uint32_t()); - MOCK_METHOD2(setWriteBufferWatermarks, void(size_t low_watermark, size_t high_watermark)); + MOCK_METHOD2(setWriteBufferWatermarks, void(uint32_t low_watermark, uint32_t high_watermark)); // Network::ClientConnection MOCK_METHOD0(connect, void()); From ea9aebbe7d41b860d834cce23896d86d4c20286c Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 10 Jul 2017 16:50:13 -0400 Subject: [PATCH 10/28] Fixing the new proxy test given #1232 --- test/integration/tcp_proxy_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index e44b0e4f2d44e..2a6ac44e4781a 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -83,7 +83,7 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyLargeWrite) { // Clean up unused client_ssl_auth [&]() -> void { fake_rest_connection = fake_upstreams_[1]->waitForRawConnection(); }, [&]() -> void { fake_rest_connection->close(); }, - [&]() -> void { fake_rest_connection->waitForDisconnect(); }, + [&]() -> void { fake_rest_connection->waitForDisconnect(true); }, }); } From e1a91fa3ac2bfc20d7e137ef6dbb45884bc222ec Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 11 Jul 2017 12:58:41 -0400 Subject: [PATCH 11/28] setting all buffer limits with one call --- include/envoy/network/connection.h | 27 +++++++------------ source/common/network/connection_impl.cc | 21 ++++++++++----- source/common/network/connection_impl.h | 5 ++-- source/common/network/listener_impl.cc | 19 ++----------- source/common/upstream/upstream_impl.cc | 3 +-- test/common/http/http1/conn_pool_test.cc | 2 +- test/common/http/http2/conn_pool_test.cc | 2 +- test/common/network/connection_impl_test.cc | 19 ++++++------- test/common/ssl/connection_impl_test.cc | 6 ++--- .../upstream/cluster_manager_impl_test.cc | 2 +- .../integration/tcp_proxy_integration_test.cc | 6 ++--- test/integration/tcp_proxy_integration_test.h | 2 +- test/mocks/network/mocks.h | 10 +++---- 13 files changed, 50 insertions(+), 74 deletions(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index b390c3087486a..b428ebe33eced 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -164,29 +164,20 @@ class Connection : public Event::DeferredDeletable, public FilterManager { virtual void write(Buffer::Instance& data) PURE; /** - * Set a soft limit on the size of the read buffer prior to flushing to further stages in the + * Set a soft limit on the size of buffers for the connection. + * For the read buffer, this limits the bytes read prior to flushing to further stages in the * processing pipeline. + * For the write buffer, it sets watermarks. When enough data is buffered it triggers a call to + * onAboveWriteBufferHighWatermark, which disables reads on the socket which is funneling data to + * the write butter. When enough data is drained from the write buffer, + * onBelowWriteBufferHighWatermark is called which results in resuming reads. */ - virtual void setReadBufferLimit(uint32_t limit) PURE; + virtual void setBufferLimits(uint32_t limit) PURE; /** - * Get the value set with setReadBufferLimit. + * Get the value set with setBufferLimits. */ - virtual uint32_t readBufferLimit() const PURE; - - /** - * Sets the high and low watermarks which trigger onAboveWriteBufferHighWatermark - * and onBelowWriteBufferHighWatermark callbacks. - * The connection is assumed to start out with less than high_watermark - * worth of data buffered, so onAboveWriteBufferHighWatermark will always be - * called before onAboveWriteBufferHighWatermark - * @param low_watermark if the connection was above the high watermark and the - * connection buffer is drained below this many bytes, onBelowWriteBufferHighWatermark will be - * called. - * @param high_watermark if the connection has more bytes than this buffered, - * onAboveWriteBufferHighWatermark will be called. - */ - virtual void setWriteBufferWatermarks(uint32_t low_watermark, uint32_t high_watermark) PURE; + virtual uint32_t bufferLimit() const PURE; }; typedef std::unique_ptr ConnectionPtr; diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 00c856cac7528..9cd0cf6b103e4 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -274,13 +274,20 @@ void ConnectionImpl::write(Buffer::Instance& data) { } } -void ConnectionImpl::setWriteBufferWatermarks(uint32_t new_low_watermark, - uint32_t new_high_watermark) { - ENVOY_CONN_LOG(debug, "Setting watermarks: {} {}", *this, new_low_watermark, new_high_watermark); - ASSERT(new_low_watermark < new_high_watermark); - - high_watermark_ = new_high_watermark; - low_watermark_ = new_low_watermark; +void ConnectionImpl::setBufferLimits(uint32_t limit) { + read_buffer_limit_ = limit; + + // Due to the fact that writes to the connection and flushing data from the connection are done + // asynchronously, we have the option of either setting the watermarks aggressively, and regularly + // enabling/disabling reads from the socket, or allowing more data, but then not triggering + // based on watermarks until 2x the data is buffered in the common case. Given these are all soft + // limits we err on the side of buffering more and having better performace. + // If the connection class is changed to write-and-flush the high watermark should be changed to + // the buffer limit without the + 1 + if (limit > 0) { + high_watermark_ = limit + 1; + low_watermark_ = limit / 2; + } checkForLowWatermark(); checkForHighWatermark(); diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 18473640ef033..bc059b6e7be2a 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -70,9 +70,8 @@ class ConnectionImpl : public virtual Connection, Ssl::Connection* ssl() override { return nullptr; } State state() override; void write(Buffer::Instance& data) override; - void setReadBufferLimit(uint32_t limit) override { read_buffer_limit_ = limit; } - uint32_t readBufferLimit() const override { return read_buffer_limit_; } - void setWriteBufferWatermarks(uint32_t low_watermark, uint32_t high_watermark) override; + void setBufferLimits(uint32_t limit) override; + uint32_t bufferLimit() const override { return read_buffer_limit_; } // Network::BufferSource Buffer::Instance& getReadBuffer() override { return read_buffer_; } diff --git a/source/common/network/listener_impl.cc b/source/common/network/listener_impl.cc index f092585a9a4e2..3176dcac67b24 100644 --- a/source/common/network/listener_impl.cc +++ b/source/common/network/listener_impl.cc @@ -101,18 +101,7 @@ void ListenerImpl::errorCallback(evconnlistener*, void*) { void ListenerImpl::newConnection(int fd, Address::InstanceConstSharedPtr remote_address, Address::InstanceConstSharedPtr local_address) { ConnectionPtr new_connection(new ConnectionImpl(dispatcher_, fd, remote_address, local_address)); - new_connection->setReadBufferLimit(options_.per_connection_buffer_limit_bytes_); - // Due to the fact that writes to the connection and flushing data from the connection are done - // asynchronously, we have the option of either setting the watermarks aggressively, and regularly - // enabling/disabling reads from the socket, or allowing more data, but then not triggering - // based on watermarks until 2x the data is buffered in the common case. Given these are all soft - // limits we err on the side of buffeing more and having better performace. - // If the connection class is changed to write-and-flush the high watermark should be changed to - // the buffer limit without the + 1 - if (options_.per_connection_buffer_limit_bytes_ > 0) { - new_connection->setWriteBufferWatermarks(options_.per_connection_buffer_limit_bytes_ / 2, - options_.per_connection_buffer_limit_bytes_ + 1); - } + new_connection->setBufferLimits(options_.per_connection_buffer_limit_bytes_); cb_.onNewConnection(std::move(new_connection)); } @@ -121,11 +110,7 @@ void SslListenerImpl::newConnection(int fd, Address::InstanceConstSharedPtr remo ConnectionPtr new_connection(new Ssl::ConnectionImpl(dispatcher_, fd, remote_address, local_address, ssl_ctx_, Ssl::ConnectionImpl::InitialState::Server)); - new_connection->setReadBufferLimit(options_.per_connection_buffer_limit_bytes_); - if (options_.per_connection_buffer_limit_bytes_ > 0) { - new_connection->setWriteBufferWatermarks(options_.per_connection_buffer_limit_bytes_ / 2, - options_.per_connection_buffer_limit_bytes_ + 1); - } + new_connection->setBufferLimits(options_.per_connection_buffer_limit_bytes_); cb_.onNewConnection(std::move(new_connection)); } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index bf42873ce610a..c4251fb694eda 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -45,8 +45,7 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu cluster.sslContext() ? dispatcher.createSslClientConnection(*cluster.sslContext(), address) : dispatcher.createClientConnection(address); uint32_t buffer_limit = cluster.perConnectionBufferLimitBytes(); - connection->setReadBufferLimit(buffer_limit); - connection->setWriteBufferWatermarks(buffer_limit / 2, buffer_limit + 1); + connection->setBufferLimits(buffer_limit); return connection; } diff --git a/test/common/http/http1/conn_pool_test.cc b/test/common/http/http1/conn_pool_test.cc index fbabd622c4be8..c212bfde443cd 100644 --- a/test/common/http/http1/conn_pool_test.cc +++ b/test/common/http/http1/conn_pool_test.cc @@ -208,7 +208,7 @@ TEST_F(Http1ConnPoolImplTest, VerifyBufferLimits) { ConnPoolCallbacks callbacks; conn_pool_.expectClientCreate(); EXPECT_CALL(*cluster_, perConnectionBufferLimitBytes()).WillOnce(Return(8192)); - EXPECT_CALL(*conn_pool_.test_clients_.back().connection_, setReadBufferLimit(8192)); + EXPECT_CALL(*conn_pool_.test_clients_.back().connection_, setBufferLimits(8192)); Http::ConnectionPool::Cancellable* handle = conn_pool_.newStream(outer_decoder, callbacks); EXPECT_NE(nullptr, handle); diff --git a/test/common/http/http2/conn_pool_test.cc b/test/common/http/http2/conn_pool_test.cc index ea4e8ab40952e..dc84fbea70cdf 100644 --- a/test/common/http/http2/conn_pool_test.cc +++ b/test/common/http/http2/conn_pool_test.cc @@ -140,7 +140,7 @@ TEST_F(Http2ConnPoolImplTest, VerifyConnectionTimingStats) { TEST_F(Http2ConnPoolImplTest, VerifyBufferLimits) { expectClientCreate(); EXPECT_CALL(*cluster_, perConnectionBufferLimitBytes()).WillOnce(Return(8192)); - EXPECT_CALL(*test_clients_.back().connection_, setReadBufferLimit(8192)); + EXPECT_CALL(*test_clients_.back().connection_, setBufferLimits(8192)); ActiveTestRequest r1(*this, 0); EXPECT_CALL(r1.inner_encoder_, encodeHeaders(_, true)); diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index df8515237216c..64235e3637c9c 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -253,28 +253,28 @@ TEST_P(ConnectionImplTest, Watermarks) { // Go from watermarks being off to being above the high watermark. EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()); EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(0); - client_connection_->setWriteBufferWatermarks(1, buffer_len - 2); + client_connection_->setBufferLimits(buffer_len - 3); } { // Go from above the high watermark to in between both. EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()).Times(0); EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(0); - client_connection_->setWriteBufferWatermarks(1, buffer_len + 2); + client_connection_->setBufferLimits(buffer_len + 1); } { // Go from above the high watermark to below the low watermark. EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()).Times(0); EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()); - client_connection_->setWriteBufferWatermarks(buffer_len + 1, buffer_len * 2); + client_connection_->setBufferLimits(buffer_len * 3); } { // Go back in between and verify neither callback is called. EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()).Times(0); EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(0); - client_connection_->setWriteBufferWatermarks(buffer_len - 2, buffer_len * 3); + client_connection_->setBufferLimits(buffer_len * 2); } disconnect(); @@ -313,7 +313,7 @@ TEST_P(ConnectionImplTest, WriteWithWatermarks) { useMockBuffer(); - client_connection_->setWriteBufferWatermarks(1, 2); + client_connection_->setBufferLimits(2); std::string data_to_write = "hello world"; Buffer::OwnedImpl first_buffer_to_write(data_to_write); @@ -362,7 +362,7 @@ TEST_P(ConnectionImplTest, WatermarkFuzzing) { connect(); useMockBuffer(); - client_connection_->setWriteBufferWatermarks(5, 10); + client_connection_->setBufferLimits(10); Runtime::RandomGeneratorImpl rand; int bytes_buffered = 0; @@ -396,7 +396,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 > 10 && is_below) { + if (bytes_to_write + bytes_buffered > 11 && is_below) { ENVOY_LOG_MISC(trace, "Expect onAboveWriteBufferHighWatermark"); EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()); is_below = false; @@ -426,9 +426,6 @@ TEST_P(ConnectionImplTest, WatermarkFuzzing) { disconnect(); } -// TODO(alyssar) ensure we have a test where we drain the full read buffer, -// readDisable, new data is sent (and not read) we readEnable and resume. - class ReadBufferLimitTest : public ConnectionImplTest { public: void readBufferLimitTest(uint32_t read_buffer_limit, uint32_t expected_chunk_size) { @@ -449,7 +446,7 @@ class ReadBufferLimitTest : public ConnectionImplTest { server_connection_ = std::move(conn); server_connection_->addReadFilter(read_filter_); EXPECT_EQ("", server_connection_->nextProtocol()); - EXPECT_EQ(read_buffer_limit, server_connection_->readBufferLimit()); + EXPECT_EQ(read_buffer_limit, server_connection_->bufferLimit()); })); uint32_t filter_seen = 0; diff --git a/test/common/ssl/connection_impl_test.cc b/test/common/ssl/connection_impl_test.cc index 8ff2e6c46a479..3bb56ddc08c87 100644 --- a/test/common/ssl/connection_impl_test.cc +++ b/test/common/ssl/connection_impl_test.cc @@ -313,7 +313,7 @@ class SslReadBufferLimitTest : public SslCertsTest, server_connection_ = std::move(conn); server_connection_->addReadFilter(read_filter_); EXPECT_EQ("", server_connection_->nextProtocol()); - EXPECT_EQ(read_buffer_limit, server_connection_->readBufferLimit()); + EXPECT_EQ(read_buffer_limit, server_connection_->bufferLimit()); })); uint32_t filter_seen = 0; @@ -360,7 +360,7 @@ class SslReadBufferLimitTest : public SslCertsTest, Initialize(read_buffer_limit); // For watermark testing, stick limits on the client connection as well. - client_connection_->setWriteBufferWatermarks(read_buffer_limit / 2, read_buffer_limit + 1); + client_connection_->setBufferLimits(read_buffer_limit); int times_called = bytes_to_write > read_buffer_limit ? 1 : 0; EXPECT_CALL(client_callbacks_, onAboveWriteBufferHighWatermark()).Times(times_called); EXPECT_CALL(client_callbacks_, onBelowWriteBufferLowWatermark()).Times(times_called); @@ -370,7 +370,7 @@ class SslReadBufferLimitTest : public SslCertsTest, server_connection_ = std::move(conn); server_connection_->addReadFilter(read_filter_); EXPECT_EQ("", server_connection_->nextProtocol()); - EXPECT_EQ(read_buffer_limit, server_connection_->readBufferLimit()); + EXPECT_EQ(read_buffer_limit, server_connection_->bufferLimit()); })); std::unique_ptr buffer_ptr_{new MockBuffer()}; diff --git a/test/common/upstream/cluster_manager_impl_test.cc b/test/common/upstream/cluster_manager_impl_test.cc index f7a1b39a010b5..3b5f885f8ee55 100644 --- a/test/common/upstream/cluster_manager_impl_test.cc +++ b/test/common/upstream/cluster_manager_impl_test.cc @@ -402,7 +402,7 @@ TEST_F(ClusterManagerImplTest, VerifyBufferLimits) { Json::ObjectSharedPtr loader = Json::Factory::loadFromString(json); create(*loader); Network::MockClientConnection* connection = new NiceMock(); - EXPECT_CALL(*connection, setReadBufferLimit(8192)); + EXPECT_CALL(*connection, setBufferLimits(8192)); EXPECT_CALL(factory_.tls_.dispatcher_, createClientConnection_(_)).WillOnce(Return(connection)); auto conn_data = cluster_manager_->tcpConnForCluster("cluster_1"); EXPECT_EQ(connection, conn_data.connection_.get()); diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 2a6ac44e4781a..8847a79a89f1f 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -89,7 +89,7 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyLargeWrite) { // Test proxying data in both directions with envoy doing TCP and TLS // termination. -void TcpProxyIntegrationTest::SendAndReceiveTlsData(const std::string& data_to_send_upstream, +void TcpProxyIntegrationTest::sendAndReceiveTlsData(const std::string& data_to_send_upstream, const std::string& data_to_send_downstream) { Network::ClientConnectionPtr ssl_client; FakeRawConnectionPtr fake_upstream_connection; @@ -153,11 +153,11 @@ void TcpProxyIntegrationTest::SendAndReceiveTlsData(const std::string& data_to_s }); } -TEST_P(TcpProxyIntegrationTest, SendTlsToTlsListener) { SendAndReceiveTlsData("hello", "world"); } +TEST_P(TcpProxyIntegrationTest, SendTlsToTlsListener) { sendAndReceiveTlsData("hello", "world"); } TEST_P(TcpProxyIntegrationTest, LargeBidirectionalTlsWrites) { std::string large_data(1024 * 8, 'a'); - SendAndReceiveTlsData(large_data, large_data); + sendAndReceiveTlsData(large_data, large_data); } } // namespace diff --git a/test/integration/tcp_proxy_integration_test.h b/test/integration/tcp_proxy_integration_test.h index ba3ca0c3cb502..5e0607c620735 100644 --- a/test/integration/tcp_proxy_integration_test.h +++ b/test/integration/tcp_proxy_integration_test.h @@ -32,7 +32,7 @@ class TcpProxyIntegrationTest : public BaseIntegrationTest, fake_upstreams_.clear(); } - void SendAndReceiveTlsData(const std::string& data_to_send_upstream, + void sendAndReceiveTlsData(const std::string& data_to_send_upstream, const std::string& data_to_send_downstream); }; } // namespace diff --git a/test/mocks/network/mocks.h b/test/mocks/network/mocks.h index f543cd7f9b20c..4dbfce8a17cdc 100644 --- a/test/mocks/network/mocks.h +++ b/test/mocks/network/mocks.h @@ -65,9 +65,8 @@ class MockConnection : public Connection, public MockConnectionBase { MOCK_METHOD0(ssl, Ssl::Connection*()); MOCK_METHOD0(state, State()); MOCK_METHOD1(write, void(Buffer::Instance& data)); - MOCK_METHOD1(setReadBufferLimit, void(uint32_t limit)); - MOCK_CONST_METHOD0(readBufferLimit, uint32_t()); - MOCK_METHOD2(setWriteBufferWatermarks, void(uint32_t low_watermark, uint32_t high_watermark)); + MOCK_METHOD1(setBufferLimits, void(uint32_t limit)); + MOCK_CONST_METHOD0(bufferLimit, uint32_t()); }; /** @@ -98,9 +97,8 @@ class MockClientConnection : public ClientConnection, public MockConnectionBase MOCK_METHOD0(ssl, Ssl::Connection*()); MOCK_METHOD0(state, State()); MOCK_METHOD1(write, void(Buffer::Instance& data)); - MOCK_METHOD1(setReadBufferLimit, void(uint32_t limit)); - MOCK_CONST_METHOD0(readBufferLimit, uint32_t()); - MOCK_METHOD2(setWriteBufferWatermarks, void(uint32_t low_watermark, uint32_t high_watermark)); + MOCK_METHOD1(setBufferLimits, void(uint32_t limit)); + MOCK_CONST_METHOD0(bufferLimit, uint32_t()); // Network::ClientConnection MOCK_METHOD0(connect, void()); From f5ce5f481479268bc4338a417dcabf281b8f2544 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 12 Jul 2017 14:09:07 -0400 Subject: [PATCH 12/28] adding stats --- source/common/filter/tcp_proxy.cc | 14 +++++++++++++- source/common/filter/tcp_proxy.h | 6 +++++- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index b1a25e1c66097..ae8196257835f 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -123,10 +123,22 @@ void TcpProxy::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callb config_->stats().downstream_cx_tx_bytes_buffered_}); } -void TcpProxy::readDisableUpstream(bool disable) { upstream_connection_->readDisable(disable); } +void TcpProxy::readDisableUpstream(bool disable) { + upstream_connection_->readDisable(disable); + if (disable) { + config_->stats().upstream_pause_reading_.inc(); + } else { + config_->stats().upstream_resume_reading_.inc(); + } +} void TcpProxy::readDisableDownstream(bool disable) { read_callbacks_->connection().readDisable(disable); + if (disable) { + config_->stats().downstream_pause_reading_.inc(); + } else { + config_->stats().downstream_resume_reading_.inc(); + } } void TcpProxy::DownstreamCallbacks::onAboveWriteBufferHighWatermark() { diff --git a/source/common/filter/tcp_proxy.h b/source/common/filter/tcp_proxy.h index 0ff1d60098270..a1d718ad8f69c 100644 --- a/source/common/filter/tcp_proxy.h +++ b/source/common/filter/tcp_proxy.h @@ -30,7 +30,11 @@ namespace Filter { COUNTER(downstream_cx_tx_bytes_total) \ GAUGE (downstream_cx_tx_bytes_buffered) \ COUNTER(downstream_cx_total) \ - COUNTER(downstream_cx_no_route) + COUNTER(downstream_cx_no_route) \ + COUNTER(upstream_pause_reading) \ + COUNTER(upstream_resume_reading) \ + COUNTER(downstream_pause_reading) \ + COUNTER(downstream_resume_reading) // clang-format on /** From 73f80d34a2a231567d9db98a33d66bea686e263d Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 12 Jul 2017 16:07:28 -0400 Subject: [PATCH 13/28] now with proper stats, and tests! --- .../cluster_manager/cluster_stats.rst | 2 ++ .../network_filters/tcp_proxy_filter.rst | 2 ++ include/envoy/upstream/upstream.h | 2 ++ source/common/filter/tcp_proxy.cc | 14 +++++++---- source/common/filter/tcp_proxy.h | 6 ++--- .../integration/tcp_proxy_integration_test.cc | 24 +++++++++++++++++++ 6 files changed, 42 insertions(+), 8 deletions(-) diff --git a/docs/configuration/cluster_manager/cluster_stats.rst b/docs/configuration/cluster_manager/cluster_stats.rst index 599c9cc469379..3b10bba64433a 100644 --- a/docs/configuration/cluster_manager/cluster_stats.rst +++ b/docs/configuration/cluster_manager/cluster_stats.rst @@ -53,6 +53,8 @@ Every cluster has a statistics tree rooted at *cluster..* with the followi upstream_rq_retry, Counter, Total request retries upstream_rq_retry_success, Counter, Total request retry successes upstream_rq_retry_overflow, Counter, Total requests not retried due to circuit breaking + upstream_flow_control_paused_reading_total, Counter, Total number of times flow control paused reading from upstream. + upstream_flow_control_resumed_reading_total, Counter, Total number of times flow control resumed reading from upstream. membership_change, Counter, Total cluster membership changes membership_healthy, Gauge, Current cluster healthy total (inclusive of both health checking and outlier detection) membership_total, Gauge, Current cluster membership total diff --git a/docs/configuration/network_filters/tcp_proxy_filter.rst b/docs/configuration/network_filters/tcp_proxy_filter.rst index faa9edc2148b9..752deb4964e35 100644 --- a/docs/configuration/network_filters/tcp_proxy_filter.rst +++ b/docs/configuration/network_filters/tcp_proxy_filter.rst @@ -138,4 +138,6 @@ statistics are rooted at *tcp..* with the following statistics: downstream_cx_no_route, Counter, Number of connections for which no matching route was found. downstream_cx_tx_bytes_total, Counter, Total bytes written to the downstream connection. downstream_cx_tx_bytes_buffered, Gauge, Total bytes currently buffered to the downstream connection. + downstream_flow_control_paused_reading_total, Counter, Total number of times flow control paused reading from downstream. + downstream_flow_control_resumed_reading_total, Counter, Total number of times flow control resumed reading from downstream. diff --git a/include/envoy/upstream/upstream.h b/include/envoy/upstream/upstream.h index 41f489711e64f..3df1c4f319270 100644 --- a/include/envoy/upstream/upstream.h +++ b/include/envoy/upstream/upstream.h @@ -201,6 +201,8 @@ class HostSet { COUNTER(upstream_rq_retry) \ COUNTER(upstream_rq_retry_success) \ COUNTER(upstream_rq_retry_overflow) \ + COUNTER(upstream_flow_control_paused_reading_total) \ + COUNTER(upstream_flow_control_resumed_reading_total) \ GAUGE (max_host_weight) \ COUNTER(membership_change) \ GAUGE (membership_healthy) \ diff --git a/source/common/filter/tcp_proxy.cc b/source/common/filter/tcp_proxy.cc index ae8196257835f..9a563b9b299e7 100644 --- a/source/common/filter/tcp_proxy.cc +++ b/source/common/filter/tcp_proxy.cc @@ -126,18 +126,24 @@ void TcpProxy::initializeReadFilterCallbacks(Network::ReadFilterCallbacks& callb void TcpProxy::readDisableUpstream(bool disable) { upstream_connection_->readDisable(disable); if (disable) { - config_->stats().upstream_pause_reading_.inc(); + read_callbacks_->upstreamHost() + ->cluster() + .stats() + .upstream_flow_control_paused_reading_total_.inc(); } else { - config_->stats().upstream_resume_reading_.inc(); + read_callbacks_->upstreamHost() + ->cluster() + .stats() + .upstream_flow_control_resumed_reading_total_.inc(); } } void TcpProxy::readDisableDownstream(bool disable) { read_callbacks_->connection().readDisable(disable); if (disable) { - config_->stats().downstream_pause_reading_.inc(); + config_->stats().downstream_flow_control_paused_reading_total_.inc(); } else { - config_->stats().downstream_resume_reading_.inc(); + config_->stats().downstream_flow_control_resumed_reading_total_.inc(); } } diff --git a/source/common/filter/tcp_proxy.h b/source/common/filter/tcp_proxy.h index a1d718ad8f69c..55b0c6e9347a7 100644 --- a/source/common/filter/tcp_proxy.h +++ b/source/common/filter/tcp_proxy.h @@ -31,10 +31,8 @@ namespace Filter { GAUGE (downstream_cx_tx_bytes_buffered) \ COUNTER(downstream_cx_total) \ COUNTER(downstream_cx_no_route) \ - COUNTER(upstream_pause_reading) \ - COUNTER(upstream_resume_reading) \ - COUNTER(downstream_pause_reading) \ - COUNTER(downstream_resume_reading) + COUNTER(downstream_flow_control_paused_reading_total) \ + COUNTER(downstream_flow_control_resumed_reading_total) // clang-format on /** diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 8847a79a89f1f..82f5e84658597 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -85,6 +85,30 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyLargeWrite) { [&]() -> void { fake_rest_connection->close(); }, [&]() -> void { fake_rest_connection->waitForDisconnect(true); }, }); + + uint32_t upstream_pauses = + test_server_->store() + .counter("cluster.cluster_with_buffer_limits.upstream_flow_control_paused_reading_total") + .value(); + uint32_t upstream_resumes = + test_server_->store() + .counter("cluster.cluster_with_buffer_limits.upstream_flow_control_resumed_reading_total") + .value(); + EXPECT_LT(0, upstream_pauses); + EXPECT_LT(0, upstream_resumes); + EXPECT_EQ(upstream_pauses, upstream_resumes); + + uint32_t downstream_pauses = + test_server_->store() + .counter("tcp.tcp_with_write_limits.downstream_flow_control_paused_reading_total") + .value(); + uint32_t downstream_resumes = + test_server_->store() + .counter("tcp.tcp_with_write_limits.downstream_flow_control_resumed_reading_total") + .value(); + EXPECT_LT(0, downstream_pauses); + EXPECT_LT(0, downstream_resumes); + EXPECT_EQ(downstream_pauses, downstream_resumes); } // Test proxying data in both directions with envoy doing TCP and TLS From e15f458e58f273443436c2bf7d9e82d1f387ff12 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Wed, 12 Jul 2017 17:02:10 -0400 Subject: [PATCH 14/28] Fixing a test flake - on tsan we don't always get write backups --- test/integration/tcp_proxy_integration_test.cc | 4 ---- 1 file changed, 4 deletions(-) diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 82f5e84658597..cad4b44ff42ff 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -94,8 +94,6 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyLargeWrite) { test_server_->store() .counter("cluster.cluster_with_buffer_limits.upstream_flow_control_resumed_reading_total") .value(); - EXPECT_LT(0, upstream_pauses); - EXPECT_LT(0, upstream_resumes); EXPECT_EQ(upstream_pauses, upstream_resumes); uint32_t downstream_pauses = @@ -106,8 +104,6 @@ TEST_P(TcpProxyIntegrationTest, TcpProxyLargeWrite) { test_server_->store() .counter("tcp.tcp_with_write_limits.downstream_flow_control_resumed_reading_total") .value(); - EXPECT_LT(0, downstream_pauses); - EXPECT_LT(0, downstream_resumes); EXPECT_EQ(downstream_pauses, downstream_resumes); } From f9603f7c400c92e5c6d1f6704efeade816f9e7bd Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 13 Jul 2017 11:53:21 -0400 Subject: [PATCH 15/28] Guarding against EAGAIN in ssl writes as we now do with TCP writes, using our new mockbuffer --- .../integration/tcp_proxy_integration_test.cc | 28 +++++++++++++++++-- test/mocks/buffer/mocks.h | 7 +++++ 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index cad4b44ff42ff..41818a48ab8bc 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -9,6 +9,10 @@ #include "gtest/gtest.h" +using testing::AnyNumber; +using testing::Invoke; +using testing::_; + namespace Envoy { namespace { @@ -119,9 +123,26 @@ void TcpProxyIntegrationTest::sendAndReceiveTlsData(const std::string& data_to_s FakeStreamPtr request; Ssl::ClientContextPtr context; ConnectionStatusCallbacks connect_callbacks; + MockBuffer* client_write_buffer; executeActions({ - // Set up the SSl client. + // Set up the mock buffer factory so the newly created SSL client will have a mock write + // buffer. This allows us to track the bytes actually written to the socket. [&]() -> void { + EXPECT_CALL(*mock_buffer_factory_, create_()) + .Times(2) + .WillOnce(Invoke([&]() -> Buffer::Instance* { + return new Buffer::OwnedImpl; // client read buffer. + })) + .WillOnce(Invoke([&]() -> Buffer::Instance* { + client_write_buffer = new MockBuffer; + ON_CALL(*client_write_buffer, move(_)) + .WillByDefault(Invoke(client_write_buffer, &MockBuffer::baseMove)); + ON_CALL(*client_write_buffer, drain(_)) + .WillByDefault(Invoke(client_write_buffer, &MockBuffer::trackDrains)); + return client_write_buffer; + })); + + // Set up the SSl client. Network::Address::InstanceConstSharedPtr address = Ssl::getSslAddress(version_, lookupPort("tcp_proxy_with_tls_termination")); context = Ssl::createClientSslContext(false, false, *context_manager); @@ -141,7 +162,6 @@ void TcpProxyIntegrationTest::sendAndReceiveTlsData(const std::string& data_to_s [&]() -> void { ssl_client->connect(); ssl_client->addConnectionCallbacks(connect_callbacks); - ssl_client->connect(); while (!connect_callbacks.connected()) { dispatcher_->run(Event::Dispatcher::RunType::NonBlock); } @@ -150,7 +170,9 @@ void TcpProxyIntegrationTest::sendAndReceiveTlsData(const std::string& data_to_s [&]() -> void { Buffer::OwnedImpl buffer(data_to_send_upstream); ssl_client->write(buffer); - dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + while (client_write_buffer->bytes_drained() != data_to_send_upstream.size()) { + dispatcher_->run(Event::Dispatcher::RunType::NonBlock); + } }, // Make sure the data makes it upstream. [&]() -> void { fake_upstream_connection = fake_upstreams_[2]->waitForRawConnection(); }, diff --git a/test/mocks/buffer/mocks.h b/test/mocks/buffer/mocks.h index f23001f301472..6b303b7824592 100644 --- a/test/mocks/buffer/mocks.h +++ b/test/mocks/buffer/mocks.h @@ -33,6 +33,11 @@ class MockBuffer : public Buffer::OwnedImpl { return bytes_written; } + void trackDrains(uint64_t size) { + bytes_drained_ += size; + Buffer::OwnedImpl::drain(size); + } + // A convenience function to invoke on write() which fails the write with EAGAIN. int failWrite(int) { errno = EAGAIN; @@ -40,9 +45,11 @@ class MockBuffer : public Buffer::OwnedImpl { } int bytes_written() const { return bytes_written_; } + uint64_t bytes_drained() const { return bytes_drained_; } private: int bytes_written_{0}; + uint64_t bytes_drained_{0}; }; class MockBufferFactory : public Buffer::Factory { From c0bbd2d01657bb329a76bdef56978f142854e56f Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 13 Jul 2017 12:00:56 -0400 Subject: [PATCH 16/28] and removing the right spurious connect --- test/integration/tcp_proxy_integration_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index 41818a48ab8bc..bd7496ae5f700 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -141,7 +141,7 @@ void TcpProxyIntegrationTest::sendAndReceiveTlsData(const std::string& data_to_s .WillByDefault(Invoke(client_write_buffer, &MockBuffer::trackDrains)); return client_write_buffer; })); - + std::cerr << "here1"; // Set up the SSl client. Network::Address::InstanceConstSharedPtr address = Ssl::getSslAddress(version_, lookupPort("tcp_proxy_with_tls_termination")); @@ -160,8 +160,8 @@ void TcpProxyIntegrationTest::sendAndReceiveTlsData(const std::string& data_to_s // Perform the SSL handshake. Loopback is whitelisted in tcp_proxy.json for the ssl_auth // filter so there will be no pause waiting on auth data. [&]() -> void { - ssl_client->connect(); ssl_client->addConnectionCallbacks(connect_callbacks); + ssl_client->connect(); while (!connect_callbacks.connected()) { dispatcher_->run(Event::Dispatcher::RunType::NonBlock); } From 8f18115cca229cdfec7be2be204903b0629153a1 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 13 Jul 2017 12:32:58 -0400 Subject: [PATCH 17/28] Removing debug log --- test/integration/tcp_proxy_integration_test.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/test/integration/tcp_proxy_integration_test.cc b/test/integration/tcp_proxy_integration_test.cc index bd7496ae5f700..ec7a156d9d226 100644 --- a/test/integration/tcp_proxy_integration_test.cc +++ b/test/integration/tcp_proxy_integration_test.cc @@ -141,7 +141,6 @@ void TcpProxyIntegrationTest::sendAndReceiveTlsData(const std::string& data_to_s .WillByDefault(Invoke(client_write_buffer, &MockBuffer::trackDrains)); return client_write_buffer; })); - std::cerr << "here1"; // Set up the SSl client. Network::Address::InstanceConstSharedPtr address = Ssl::getSslAddress(version_, lookupPort("tcp_proxy_with_tls_termination")); From c60495020d4faad25a553256887158635dea4662 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 13 Jul 2017 15:52:19 -0400 Subject: [PATCH 18/28] clarifying comment --- include/envoy/network/connection.h | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index b428ebe33eced..6fa961bafe67c 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -168,9 +168,10 @@ class Connection : public Event::DeferredDeletable, public FilterManager { * For the read buffer, this limits the bytes read prior to flushing to further stages in the * processing pipeline. * For the write buffer, it sets watermarks. When enough data is buffered it triggers a call to - * onAboveWriteBufferHighWatermark, which disables reads on the socket which is funneling data to - * the write butter. When enough data is drained from the write buffer, - * onBelowWriteBufferHighWatermark is called which results in resuming reads. + * onAboveWriteBufferHighWatermark, which allows subscribers to enforce flow control by disabling + * reads on the socket funneling data to the write butter. When enough data is drained from the + * write buffer, onBelowWriteBufferHighWatermark is called which similarly allows subscribers + * resuming reading. */ virtual void setBufferLimits(uint32_t limit) PURE; From 31b46da4cc50e5b0668c8a60979006ee9e3fb43c Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 13 Jul 2017 16:03:58 -0400 Subject: [PATCH 19/28] I can't believe it's not butter! --- include/envoy/network/connection.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/envoy/network/connection.h b/include/envoy/network/connection.h index 6fa961bafe67c..274efb622b1ed 100644 --- a/include/envoy/network/connection.h +++ b/include/envoy/network/connection.h @@ -169,7 +169,7 @@ class Connection : public Event::DeferredDeletable, public FilterManager { * processing pipeline. * For the write buffer, it sets watermarks. When enough data is buffered it triggers a call to * onAboveWriteBufferHighWatermark, which allows subscribers to enforce flow control by disabling - * reads on the socket funneling data to the write butter. When enough data is drained from the + * reads on the socket funneling data to the write buffer. When enough data is drained from the * write buffer, onBelowWriteBufferHighWatermark is called which similarly allows subscribers * resuming reading. */ From 872e1b3f59b2b9e9f4e2b1e8a0656aae8c3477b6 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Thu, 13 Jul 2017 21:27:34 -0400 Subject: [PATCH 20/28] Moving watermark checks to a utility class wrapping the buffer --- source/common/buffer/BUILD | 10 ++ source/common/buffer/watermark_buffer.cc | 89 ++++++++++++++ source/common/buffer/watermark_buffer.h | 64 ++++++++++ source/common/event/dispatcher_impl.cc | 1 + source/common/network/BUILD | 1 + source/common/network/connection_impl.cc | 43 +++---- source/common/network/connection_impl.h | 22 +--- source/common/ssl/connection_impl.cc | 7 +- test/common/buffer/BUILD | 9 ++ test/common/buffer/watermark_buffer_test.cc | 126 ++++++++++++++++++++ 10 files changed, 321 insertions(+), 51 deletions(-) create mode 100644 source/common/buffer/watermark_buffer.cc create mode 100644 source/common/buffer/watermark_buffer.h create mode 100644 test/common/buffer/watermark_buffer_test.cc diff --git a/source/common/buffer/BUILD b/source/common/buffer/BUILD index 2907225057df8..6a42413bb5627 100644 --- a/source/common/buffer/BUILD +++ b/source/common/buffer/BUILD @@ -8,6 +8,16 @@ load( envoy_package() +envoy_cc_library( + name = "watermark_buffer_lib", + srcs = ["watermark_buffer.cc"], + hdrs = ["watermark_buffer.h"], + deps = [ + "//include/envoy/buffer:buffer_interface", + "//source/common/common:assert_lib", + ], +) + envoy_cc_library( name = "buffer_lib", srcs = ["buffer_impl.cc"], diff --git a/source/common/buffer/watermark_buffer.cc b/source/common/buffer/watermark_buffer.cc new file mode 100644 index 0000000000000..f1a01342b39a6 --- /dev/null +++ b/source/common/buffer/watermark_buffer.cc @@ -0,0 +1,89 @@ +#include "common/buffer/watermark_buffer.h" + +#include "common/common/assert.h" + +namespace Envoy { +namespace Buffer { + +void WatermarkBuffer::add(const void* data, uint64_t size) { + wrapped_buffer_->add(data, size); + checkHighWatermark(); +} + +void WatermarkBuffer::add(const std::string& data) { + wrapped_buffer_->add(data); + checkHighWatermark(); +} + +void WatermarkBuffer::add(const Instance& data) { + wrapped_buffer_->add(data); + checkHighWatermark(); +} + +void WatermarkBuffer::commit(RawSlice* iovecs, uint64_t num_iovecs) { + wrapped_buffer_->commit(iovecs, num_iovecs); + checkHighWatermark(); +} + +void WatermarkBuffer::drain(uint64_t size) { + wrapped_buffer_->drain(size); + checkLowWatermark(); +} + +void WatermarkBuffer::move(Instance& rhs) { + wrapped_buffer_->move(rhs); + checkHighWatermark(); +} + +void WatermarkBuffer::move(Instance& rhs, uint64_t length) { + wrapped_buffer_->move(rhs, length); + checkHighWatermark(); +} + +int WatermarkBuffer::read(int fd, uint64_t max_length) { + int bytes_read = wrapped_buffer_->read(fd, max_length); + checkHighWatermark(); + return bytes_read; +} + +uint64_t WatermarkBuffer::reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) { + uint64_t bytes_reserved = wrapped_buffer_->reserve(length, iovecs, num_iovecs); + checkHighWatermark(); + return bytes_reserved; +} + +int WatermarkBuffer::write(int fd) { + int bytes_written = wrapped_buffer_->write(fd); + checkLowWatermark(); + return bytes_written; +} + +void WatermarkBuffer::setWatermarks(uint32_t low_watermark, uint32_t high_watermark) { + ASSERT(low_watermark < high_watermark); + low_watermark_ = low_watermark; + high_watermark_ = high_watermark; + checkHighWatermark(); + checkLowWatermark(); +} + +void WatermarkBuffer::checkLowWatermark() { + if (!above_high_watermark_called_ || wrapped_buffer_->length() >= low_watermark_) { + return; + } + + above_high_watermark_called_ = false; + below_low_watermark_(); +} + +void WatermarkBuffer::checkHighWatermark() { + if (above_high_watermark_called_ || high_watermark_ == 0 || + wrapped_buffer_->length() <= high_watermark_) { + return; + } + + above_high_watermark_called_ = true; + above_high_watermark_(); +} + +} // namespace Buffer +} // namespace Envoy diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h new file mode 100644 index 0000000000000..416daaf15ce43 --- /dev/null +++ b/source/common/buffer/watermark_buffer.h @@ -0,0 +1,64 @@ +#pragma once + +#include + +#include "envoy/buffer/buffer.h" + +namespace Envoy { +namespace Buffer { + +// A wrapper for an underlying buffer which does watermark validation. +// The underlying buffer's ownership is transfered to the Watermark buffer. Each time the inner +// buffer is resized (written to or drained), the watermarks are checked. As the buffer size +// transitions from under the low watermark to above the high watermark, the above_high_watermark +// function is called one time. It will not be called again until the buffer is drained below the +// low watermark, at which point the below_low_watermark function is called. +class WatermarkBuffer : public Instance { +public: + WatermarkBuffer(InstancePtr&& buffer, std::function below_low_watermark, + std::function above_high_watermark) + : wrapped_buffer_(std::move(buffer)), below_low_watermark_(below_low_watermark), + above_high_watermark_(above_high_watermark) {} + + // Instance + void add(const void* data, uint64_t size) override; + void add(const std::string& data) override; + void add(const Instance& data) override; + void commit(RawSlice* iovecs, uint64_t num_iovecs) override; + void drain(uint64_t size) override; + uint64_t getRawSlices(RawSlice* out, uint64_t out_size) const override { + return wrapped_buffer_->getRawSlices(out, out_size); + } + uint64_t length() const override { return wrapped_buffer_->length(); } + void* linearize(uint32_t size) override { return wrapped_buffer_->linearize(size); } + void move(Instance& rhs) override; + void move(Instance& rhs, uint64_t length) override; + int read(int fd, uint64_t max_length) override; + uint64_t reserve(uint64_t length, RawSlice* iovecs, uint64_t num_iovecs) override; + ssize_t search(const void* data, uint64_t size, size_t start) const override { + return wrapped_buffer_->search(data, size, start); + } + int write(int fd) override; + + void setWatermarks(uint32_t low_watermark, uint32_t high_watermark); + +private: + void checkHighWatermark(); + void checkLowWatermark(); + + InstancePtr wrapped_buffer_; + std::function below_low_watermark_; + std::function above_high_watermark_; + + // Used for enforcing buffer limits (off by default). If these are set to non-zero by a call to + // setWatermarks() the watermark callbacks will be called as described above. + uint32_t high_watermark_{0}; + uint32_t low_watermark_{0}; + // Tracks the latest state of watermark callbacks. + // True between the time above_high_watermark_ has been called until above_high_watermark_ has + // been called. + bool above_high_watermark_called_{false}; +}; + +} // namespace Buffer +} // namespace Envoy diff --git a/source/common/event/dispatcher_impl.cc b/source/common/event/dispatcher_impl.cc index 84204d3258b3e..f4aa3bd298e2b 100644 --- a/source/common/event/dispatcher_impl.cc +++ b/source/common/event/dispatcher_impl.cc @@ -10,6 +10,7 @@ #include "envoy/network/listen_socket.h" #include "envoy/network/listener.h" +#include "common/buffer/buffer_impl.h" #include "common/event/file_event_impl.h" #include "common/event/signal_impl.h" #include "common/event/timer_impl.h" diff --git a/source/common/network/BUILD b/source/common/network/BUILD index 8699a2a249b8e..cde65f48235a1 100644 --- a/source/common/network/BUILD +++ b/source/common/network/BUILD @@ -45,6 +45,7 @@ envoy_cc_library( "//include/envoy/network:connection_interface", "//include/envoy/network:filter_interface", "//source/common/buffer:buffer_lib", + "//source/common/buffer:watermark_buffer_lib", "//source/common/common:assert_lib", "//source/common/common:empty_string", "//source/common/common:enum_to_int", diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index eb2d9683b0b88..52b98186c1984 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -59,8 +59,10 @@ ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, Address::InstanceConstSharedPtr local_address) : filter_manager_(*this, *this), remote_address_(remote_address), local_address_(local_address), read_buffer_(dispatcher.getBufferFactory().create()), - write_buffer_(dispatcher.getBufferFactory().create()), dispatcher_(dispatcher), fd_(fd), - id_(++next_global_id_) { + write_buffer_(Buffer::InstancePtr{dispatcher.getBufferFactory().create()}, + [&]() -> void { this->onLowWatermark(); }, + [&]() -> void { this->onHighWatermark(); }), + dispatcher_(dispatcher), fd_(fd), id_(++next_global_id_) { // Treat the lack of a valid fd (which in practice only happens if we run out of FDs) as an OOM // condition and just crash. @@ -100,7 +102,7 @@ void ConnectionImpl::close(ConnectionCloseType type) { return; } - uint64_t data_to_write = write_buffer_->length(); + uint64_t data_to_write = write_buffer_.length(); ENVOY_CONN_LOG(debug, "closing data_to_write={} type={}", *this, data_to_write, enumToInt(type)); if (data_to_write == 0 || type == ConnectionCloseType::NoFlush) { if (data_to_write > 0) { @@ -267,8 +269,7 @@ void ConnectionImpl::write(Buffer::Instance& data) { // ever changed, read the comment in Ssl::ConnectionImpl::doWriteToSocket() VERY carefully. // That code assumes that we never change existing write_buffer_ chain elements between calls // to SSL_write(). That code will have to change if we ever copy here. - write_buffer_->move(data); - checkForHighWatermark(); + write_buffer_.move(data); if (!(state_ & InternalState::Connecting)) { file_event_->activate(Event::FileReadyType::Write); @@ -284,37 +285,22 @@ void ConnectionImpl::setBufferLimits(uint32_t limit) { // enabling/disabling reads from the socket, or allowing more data, but then not triggering // based on watermarks until 2x the data is buffered in the common case. Given these are all soft // limits we err on the side of buffering more and having better performace. - // If the connection class is changed to write-and-flush the high watermark should be changed to - // the buffer limit without the + 1 + // If the connection class is changed to write to the buffer and flush to the socket in the same + // stack, the high watermark should be changed to the buffer limit without the + 1 if (limit > 0) { - high_watermark_ = limit + 1; - low_watermark_ = limit / 2; + write_buffer_.setWatermarks(limit / 2, limit + 1); } - - checkForLowWatermark(); - checkForHighWatermark(); } -void ConnectionImpl::checkForLowWatermark() { - if (!above_high_watermark_called_ || write_buffer_->length() >= low_watermark_) { - return; - } +void ConnectionImpl::onLowWatermark() { ENVOY_CONN_LOG(debug, "onBelowWriteBufferLowWatermark", *this); - - above_high_watermark_called_ = false; for (ConnectionCallbacks* callback : callbacks_) { callback->onBelowWriteBufferLowWatermark(); } } -void ConnectionImpl::checkForHighWatermark() { - if (above_high_watermark_called_ || high_watermark_ == 0 || - write_buffer_->length() <= high_watermark_) { - return; - } +void ConnectionImpl::onHighWatermark() { ENVOY_CONN_LOG(debug, "onAboveWriteBufferHighWatermark", *this); - - above_high_watermark_called_ = true; for (ConnectionCallbacks* callback : callbacks_) { callback->onAboveWriteBufferHighWatermark(); } @@ -406,11 +392,11 @@ ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { PostIoAction action; uint64_t bytes_written = 0; do { - if (write_buffer_->length() == 0) { + if (write_buffer_.length() == 0) { action = PostIoAction::KeepOpen; break; } - int rc = write_buffer_->write(fd_); + int rc = write_buffer_.write(fd_); ENVOY_CONN_LOG(trace, "write returns: {}", *this, rc); if (rc == -1) { ENVOY_CONN_LOG(trace, "write error: {}", *this, errno); @@ -422,7 +408,6 @@ ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { break; } else { - checkForLowWatermark(); bytes_written += rc; } } while (true); @@ -459,7 +444,7 @@ void ConnectionImpl::onWriteReady() { } IoResult result = doWriteToSocket(); - uint64_t new_buffer_size = write_buffer_->length(); + uint64_t new_buffer_size = write_buffer_.length(); updateWriteBufferStats(result.bytes_processed_, new_buffer_size); if (result.action_ == PostIoAction::Close) { diff --git a/source/common/network/connection_impl.h b/source/common/network/connection_impl.h index 796df6df98d63..225e47fdcb24b 100644 --- a/source/common/network/connection_impl.h +++ b/source/common/network/connection_impl.h @@ -8,7 +8,7 @@ #include "envoy/network/connection.h" -#include "common/buffer/buffer_impl.h" +#include "common/buffer/watermark_buffer.h" #include "common/common/logger.h" #include "common/event/dispatcher_impl.h" #include "common/event/libevent.h" @@ -99,29 +99,15 @@ class ConnectionImpl : public virtual Connection, // Reconsider how to make fairness happen. void setReadBufferReady() { file_event_->activate(Event::FileReadyType::Read); } - // Called when data is drained from the write buffer, to see if onBelowWriteBufferLowWatermark - // should be called. - void checkForLowWatermark(); - // Called when data is added to the write buffer, to see if onAboveWriteBufferHighWatermark should - // be called. - void checkForHighWatermark(); + void onLowWatermark(); + void onHighWatermark(); FilterManagerImpl filter_manager_; Address::InstanceConstSharedPtr remote_address_; Address::InstanceConstSharedPtr local_address_; Buffer::InstancePtr read_buffer_; - Buffer::InstancePtr write_buffer_; + Buffer::WatermarkBuffer write_buffer_; uint32_t read_buffer_limit_ = 0; - // Used for network level buffer limits (off by default). If these are non-zero, when the write - // buffer passes |high_watermark_|, onAboveWriteBufferHighWatermark will be called to disable - // reading further data. When the buffer drains below |low_watermark_|, - // onBelowWriteBufferLowWatermark will be called to resume reads. - uint32_t high_watermark_{0}; - uint32_t low_watermark_{0}; - // Tracks the latest state of watermark callbacks. - // True between the time onAboveWriteBufferHighWatermark is called until the next call to - // onBelowLowWatermark. - bool above_high_watermark_called_{false}; private: // clang-format off diff --git a/source/common/ssl/connection_impl.cc b/source/common/ssl/connection_impl.cc index 024cb015db18f..f263688b7d278 100644 --- a/source/common/ssl/connection_impl.cc +++ b/source/common/ssl/connection_impl.cc @@ -164,7 +164,7 @@ Network::ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { } } - uint64_t original_buffer_length = write_buffer_->length(); + uint64_t original_buffer_length = write_buffer_.length(); uint64_t total_bytes_written = 0; bool keep_writing = true; while ((original_buffer_length != total_bytes_written) && keep_writing) { @@ -175,7 +175,7 @@ Network::ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { // of iterations of this loop, either by pure iterations, bytes written, etc. const uint64_t MAX_SLICES = 32; Buffer::RawSlice slices[MAX_SLICES]; - uint64_t num_slices = write_buffer_->getRawSlices(slices, MAX_SLICES); + uint64_t num_slices = write_buffer_.getRawSlices(slices, MAX_SLICES); uint64_t inner_bytes_written = 0; for (uint64_t i = 0; (i < num_slices) && (original_buffer_length != total_bytes_written); i++) { @@ -210,8 +210,7 @@ Network::ConnectionImpl::IoResult ConnectionImpl::doWriteToSocket() { // Draining must be done within the inner loop, otherwise we will keep getting the same slices // at the beginning of the buffer. if (inner_bytes_written > 0) { - write_buffer_->drain(inner_bytes_written); - checkForLowWatermark(); + write_buffer_.drain(inner_bytes_written); } } diff --git a/test/common/buffer/BUILD b/test/common/buffer/BUILD index aaedc4f077f72..1c5b68a363452 100644 --- a/test/common/buffer/BUILD +++ b/test/common/buffer/BUILD @@ -8,6 +8,15 @@ load( envoy_package() +envoy_cc_test( + name = "watermark_buffer_test", + srcs = ["watermark_buffer_test.cc"], + deps = [ + "//source/common/buffer:buffer_lib", + "//source/common/buffer:watermark_buffer_lib", + ], +) + envoy_cc_test( name = "zero_copy_input_stream_test", srcs = ["zero_copy_input_stream_test.cc"], diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc new file mode 100644 index 0000000000000..770e168af3c92 --- /dev/null +++ b/test/common/buffer/watermark_buffer_test.cc @@ -0,0 +1,126 @@ +#include "common/buffer/buffer_impl.h" +#include "common/buffer/watermark_buffer.h" + +#include "gtest/gtest.h" + +namespace Envoy { +namespace Buffer { +namespace { + +const char TEN_BYTES[] = "0123456789"; + +class WatermarkBufferTest : public testing::Test { +public: + WatermarkBufferTest() { buffer_.setWatermarks(5, 10); } + + Buffer::WatermarkBuffer buffer_{InstancePtr{new OwnedImpl()}, + [&]() -> void { ++times_low_watermark_called_; }, + [&]() -> void { ++times_high_watermark_called_; }}; + uint32_t times_low_watermark_called_{0}; + uint32_t times_high_watermark_called_{0}; +}; + +TEST_F(WatermarkBufferTest, AddChar) { + buffer_.add(TEN_BYTES, 10); + EXPECT_EQ(0, times_high_watermark_called_); + buffer_.add("a", 1); + EXPECT_EQ(1, times_high_watermark_called_); + EXPECT_EQ(11, buffer_.length()); +} + +TEST_F(WatermarkBufferTest, AddString) { + buffer_.add(std::string(TEN_BYTES)); + EXPECT_EQ(0, times_high_watermark_called_); + buffer_.add(std::string("a")); + EXPECT_EQ(1, times_high_watermark_called_); + EXPECT_EQ(11, buffer_.length()); +} + +TEST_F(WatermarkBufferTest, AddBuffer) { + OwnedImpl first(TEN_BYTES); + buffer_.add(first); + EXPECT_EQ(0, times_high_watermark_called_); + OwnedImpl second("a"); + buffer_.add(second); + EXPECT_EQ(1, times_high_watermark_called_); + EXPECT_EQ(11, buffer_.length()); +} + +TEST_F(WatermarkBufferTest, Commit) { + buffer_.add(TEN_BYTES, 10); + EXPECT_EQ(0, times_high_watermark_called_); + RawSlice out; + buffer_.reserve(10, &out, 1); + memcpy(out.mem_, &TEN_BYTES[0], 10); + out.len_ = 10; + buffer_.commit(&out, 1); + EXPECT_EQ(1, times_high_watermark_called_); + EXPECT_EQ(20, buffer_.length()); +} + +TEST_F(WatermarkBufferTest, Drain) { + // Draining from above to below the low watermark does nothing if the high + // watermark never got hit. + buffer_.add(TEN_BYTES, 10); + buffer_.drain(10); + EXPECT_EQ(0, times_high_watermark_called_); + EXPECT_EQ(0, times_low_watermark_called_); + + // Go above the high watermark then drain down to just at the low watermark. + buffer_.add(TEN_BYTES, 11); + buffer_.drain(6); + EXPECT_EQ(5, buffer_.length()); + EXPECT_EQ(0, times_low_watermark_called_); + + // Now drain below. + buffer_.drain(1); + EXPECT_EQ(1, times_low_watermark_called_); + + // Going back above should trigger the high again + buffer_.add(TEN_BYTES, 10); + EXPECT_EQ(2, times_high_watermark_called_); +} + +TEST_F(WatermarkBufferTest, MoveFullBuffer) { + buffer_.add(TEN_BYTES, 10); + OwnedImpl data("a"); + + EXPECT_EQ(0, times_high_watermark_called_); + buffer_.move(data); + EXPECT_EQ(1, times_high_watermark_called_); + EXPECT_EQ(11, buffer_.length()); +} + +TEST_F(WatermarkBufferTest, MoveOneByte) { + buffer_.add(TEN_BYTES, 9); + OwnedImpl data("ab"); + + buffer_.move(data, 1); + EXPECT_EQ(0, times_high_watermark_called_); + EXPECT_EQ(10, buffer_.length()); + + buffer_.move(data, 1); + EXPECT_EQ(1, times_high_watermark_called_); + EXPECT_EQ(11, buffer_.length()); +} + +TEST_F(WatermarkBufferTest, MoveWatermarks) { + buffer_.add(TEN_BYTES, 9); + EXPECT_EQ(0, times_high_watermark_called_); + buffer_.setWatermarks(1, 9); + EXPECT_EQ(0, times_high_watermark_called_); + buffer_.setWatermarks(1, 8); + EXPECT_EQ(1, times_high_watermark_called_); + + buffer_.setWatermarks(9, 20); + EXPECT_EQ(0, times_low_watermark_called_); + buffer_.setWatermarks(10, 20); + EXPECT_EQ(1, times_low_watermark_called_); + buffer_.setWatermarks(8, 20); + buffer_.setWatermarks(10, 20); + EXPECT_EQ(1, times_low_watermark_called_); +} + +} // namespace +} // namespace Buffer +} // namespace Envoy From 71792b3af1af13f693bd87b49f74efb1cf9dfd44 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Mon, 17 Jul 2017 10:53:14 -0400 Subject: [PATCH 21/28] now with better coverage --- test/common/buffer/watermark_buffer_test.cc | 50 +++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc index 770e168af3c92..c7fc415a02190 100644 --- a/test/common/buffer/watermark_buffer_test.cc +++ b/test/common/buffer/watermark_buffer_test.cc @@ -104,6 +104,36 @@ TEST_F(WatermarkBufferTest, MoveOneByte) { EXPECT_EQ(11, buffer_.length()); } +TEST_F(WatermarkBufferTest, WatermarkFdFunctions) { + int pipe_fds[2] = {0, 0}; + ASSERT_EQ(0, pipe(pipe_fds)); + + buffer_.add(TEN_BYTES, 10); + buffer_.add(TEN_BYTES, 10); + EXPECT_EQ(1, times_high_watermark_called_); + EXPECT_EQ(0, times_low_watermark_called_); + + int bytes_written_total = 0; + while (bytes_written_total < 20) { + int bytes_written = buffer_.write(pipe_fds[1]); + if (bytes_written < 0) { + ASSERT_EQ(EAGAIN, errno); + } else { + bytes_written_total += bytes_written; + } + } + EXPECT_EQ(1, times_high_watermark_called_); + EXPECT_EQ(1, times_low_watermark_called_); + EXPECT_EQ(0, buffer_.length()); + + int bytes_read_total = 0; + while (bytes_read_total < 20) { + bytes_read_total += buffer_.read(pipe_fds[0], 20); + } + EXPECT_EQ(2, times_high_watermark_called_); + EXPECT_EQ(20, buffer_.length()); +} + TEST_F(WatermarkBufferTest, MoveWatermarks) { buffer_.add(TEN_BYTES, 9); EXPECT_EQ(0, times_high_watermark_called_); @@ -121,6 +151,26 @@ TEST_F(WatermarkBufferTest, MoveWatermarks) { EXPECT_EQ(1, times_low_watermark_called_); } +TEST_F(WatermarkBufferTest, GetRawSlices) { + buffer_.add(TEN_BYTES, 10); + + RawSlice slices[2]; + ASSERT_EQ(1, buffer_.getRawSlices(&slices[0], 2)); + EXPECT_EQ(10, slices[0].len_); + EXPECT_EQ(0, memcmp(slices[0].mem_, &TEN_BYTES[0], 10)); + + void* data_pointer = buffer_.linearize(10); + EXPECT_EQ(data_pointer, slices[0].mem_); +} + +TEST_F(WatermarkBufferTest, Search) { + buffer_.add(TEN_BYTES, 10); + + EXPECT_EQ(1, buffer_.search(&TEN_BYTES[1], 2, 0)); + + EXPECT_EQ(-1, buffer_.search(&TEN_BYTES[1], 2, 5)); +} + } // namespace } // namespace Buffer } // namespace Envoy From c6e50dcb454b6700ab045dbd7a7ace960751d101 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 18 Jul 2017 10:38:59 -0400 Subject: [PATCH 22/28] Making move work between watermark buffers and ownedimpl --- source/common/buffer/BUILD | 2 +- source/common/buffer/buffer_impl.cc | 6 +++--- source/common/buffer/buffer_impl.h | 11 +++++++++-- source/common/buffer/watermark_buffer.h | 7 +++++-- test/common/buffer/watermark_buffer_test.cc | 9 +++++++++ 5 files changed, 27 insertions(+), 8 deletions(-) diff --git a/source/common/buffer/BUILD b/source/common/buffer/BUILD index 6a42413bb5627..53b0868e82968 100644 --- a/source/common/buffer/BUILD +++ b/source/common/buffer/BUILD @@ -13,7 +13,7 @@ envoy_cc_library( srcs = ["watermark_buffer.cc"], hdrs = ["watermark_buffer.h"], deps = [ - "//include/envoy/buffer:buffer_interface", + "//source/common/buffer:buffer_lib", "//source/common/common:assert_lib", ], ) diff --git a/source/common/buffer/buffer_impl.cc b/source/common/buffer/buffer_impl.cc index db4c8f83a867b..a9f6fd0148a87 100644 --- a/source/common/buffer/buffer_impl.cc +++ b/source/common/buffer/buffer_impl.cc @@ -67,15 +67,15 @@ void OwnedImpl::move(Instance& rhs) { // now and this is safe. Using the evbuffer move routines require having access to both evbuffers. // This is a reasonable compromise in a high performance path where we want to maintain an // abstraction in case we get rid of evbuffer later. - int rc = evbuffer_add_buffer(buffer_.get(), static_cast(rhs).buffer_.get()); + int rc = evbuffer_add_buffer(buffer_.get(), static_cast(rhs).buffer().get()); ASSERT(rc == 0); UNREFERENCED_PARAMETER(rc); } void OwnedImpl::move(Instance& rhs, uint64_t length) { // See move() above for why we do the static cast. - int rc = - evbuffer_remove_buffer(static_cast(rhs).buffer_.get(), buffer_.get(), length); + int rc = evbuffer_remove_buffer(static_cast(rhs).buffer().get(), buffer_.get(), + length); ASSERT(static_cast(rc) == length); UNREFERENCED_PARAMETER(rc); } diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index 8214eac9f7254..3da9629355fd7 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -16,17 +16,22 @@ class OwnedImplFactory : public Factory { InstancePtr create() override; }; +class LibEventInstance : public Instance { +public: + virtual Event::Libevent::BufferPtr& buffer() PURE; +}; + /** * Wraps an allocated and owned evbuffer. */ -class OwnedImpl : public Instance { +class OwnedImpl : public LibEventInstance { public: OwnedImpl(); OwnedImpl(const std::string& data); OwnedImpl(const Instance& data); OwnedImpl(const void* data, uint64_t size); - // Instance + // LibEventInstance void add(const void* data, uint64_t size) override; void add(const std::string& data) override; void add(const Instance& data) override; @@ -42,6 +47,8 @@ class OwnedImpl : public Instance { ssize_t search(const void* data, uint64_t size, size_t start) const override; int write(int fd) override; + Event::Libevent::BufferPtr& buffer() override { return buffer_; } + private: Event::Libevent::BufferPtr buffer_; }; diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index 416daaf15ce43..92afc7ea3c45e 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -2,7 +2,7 @@ #include -#include "envoy/buffer/buffer.h" +#include "common/buffer/buffer_impl.h" namespace Envoy { namespace Buffer { @@ -13,7 +13,7 @@ namespace Buffer { // transitions from under the low watermark to above the high watermark, the above_high_watermark // function is called one time. It will not be called again until the buffer is drained below the // low watermark, at which point the below_low_watermark function is called. -class WatermarkBuffer : public Instance { +class WatermarkBuffer : public LibEventInstance { public: WatermarkBuffer(InstancePtr&& buffer, std::function below_low_watermark, std::function above_high_watermark) @@ -39,6 +39,9 @@ class WatermarkBuffer : public Instance { return wrapped_buffer_->search(data, size, start); } int write(int fd) override; + Event::Libevent::BufferPtr& buffer() override { + return static_cast(*wrapped_buffer_).buffer(); + } void setWatermarks(uint32_t low_watermark, uint32_t high_watermark); diff --git a/test/common/buffer/watermark_buffer_test.cc b/test/common/buffer/watermark_buffer_test.cc index c7fc415a02190..937c077d849af 100644 --- a/test/common/buffer/watermark_buffer_test.cc +++ b/test/common/buffer/watermark_buffer_test.cc @@ -171,6 +171,15 @@ TEST_F(WatermarkBufferTest, Search) { EXPECT_EQ(-1, buffer_.search(&TEN_BYTES[1], 2, 5)); } +TEST_F(WatermarkBufferTest, MoveBack) { + buffer_.add(TEN_BYTES, 10); + OwnedImpl data("a"); + + EXPECT_EQ(0, times_high_watermark_called_); + buffer_.move(data); + data.move(buffer_); +} + } // namespace } // namespace Buffer } // namespace Envoy From c925feef9776ae2f5201417ffdf1736234b23fed Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 18 Jul 2017 11:22:10 -0400 Subject: [PATCH 23/28] Adding more comments for watermark buffers, misc review comments --- source/common/buffer/buffer_impl.h | 3 +++ source/common/buffer/watermark_buffer.h | 8 ++++++++ source/common/network/connection_impl.cc | 6 +++--- source/common/upstream/upstream_impl.cc | 3 +-- 4 files changed, 15 insertions(+), 5 deletions(-) diff --git a/source/common/buffer/buffer_impl.h b/source/common/buffer/buffer_impl.h index 3da9629355fd7..40414bc101102 100644 --- a/source/common/buffer/buffer_impl.h +++ b/source/common/buffer/buffer_impl.h @@ -23,6 +23,9 @@ class LibEventInstance : public Instance { /** * Wraps an allocated and owned evbuffer. + * + * Note that due to the internals of move() accessing buffer(), OwnedImpl is not + * compatible with non-LibEventInstance buffers. */ class OwnedImpl : public LibEventInstance { public: diff --git a/source/common/buffer/watermark_buffer.h b/source/common/buffer/watermark_buffer.h index 92afc7ea3c45e..fd7547ff44eec 100644 --- a/source/common/buffer/watermark_buffer.h +++ b/source/common/buffer/watermark_buffer.h @@ -13,6 +13,14 @@ namespace Buffer { // transitions from under the low watermark to above the high watermark, the above_high_watermark // function is called one time. It will not be called again until the buffer is drained below the // low watermark, at which point the below_low_watermark function is called. +// +// Because the internals of OwnedImpl::move() require accessing the underlying data, OwnedImpl is +// not compatible with generic Buffer::Impls. To allow compatability between WatermarkBuffer and +// OwnedImpl::move, WatermarkBuffer must implement LibEventInstance and is also not compatible +// with generic Buffer::Impls. +// +// WatermarkBuffer takes a pointer to a generic InstancePtr in the constructor to allow test mocks +// which overrides move() in any case. class WatermarkBuffer : public LibEventInstance { public: WatermarkBuffer(InstancePtr&& buffer, std::function below_low_watermark, diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 52b98186c1984..8ab54f6abdd2f 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -60,8 +60,8 @@ ConnectionImpl::ConnectionImpl(Event::DispatcherImpl& dispatcher, int fd, : filter_manager_(*this, *this), remote_address_(remote_address), local_address_(local_address), read_buffer_(dispatcher.getBufferFactory().create()), write_buffer_(Buffer::InstancePtr{dispatcher.getBufferFactory().create()}, - [&]() -> void { this->onLowWatermark(); }, - [&]() -> void { this->onHighWatermark(); }), + [this]() -> void { this->onLowWatermark(); }, + [this]() -> void { this->onHighWatermark(); }), dispatcher_(dispatcher), fd_(fd), id_(++next_global_id_) { // Treat the lack of a valid fd (which in practice only happens if we run out of FDs) as an OOM @@ -286,7 +286,7 @@ void ConnectionImpl::setBufferLimits(uint32_t limit) { // based on watermarks until 2x the data is buffered in the common case. Given these are all soft // limits we err on the side of buffering more and having better performace. // If the connection class is changed to write to the buffer and flush to the socket in the same - // stack, the high watermark should be changed to the buffer limit without the + 1 + // stack, the high watermark should be changed from |limit + 1| to |limit|. if (limit > 0) { write_buffer_.setWatermarks(limit / 2, limit + 1); } diff --git a/source/common/upstream/upstream_impl.cc b/source/common/upstream/upstream_impl.cc index c4251fb694eda..5cfd0f236ae15 100644 --- a/source/common/upstream/upstream_impl.cc +++ b/source/common/upstream/upstream_impl.cc @@ -44,8 +44,7 @@ HostImpl::createConnection(Event::Dispatcher& dispatcher, const ClusterInfo& clu Network::ClientConnectionPtr connection = cluster.sslContext() ? dispatcher.createSslClientConnection(*cluster.sslContext(), address) : dispatcher.createClientConnection(address); - uint32_t buffer_limit = cluster.perConnectionBufferLimitBytes(); - connection->setBufferLimits(buffer_limit); + connection->setBufferLimits(cluster.perConnectionBufferLimitBytes()); return connection; } From 3bf7617dd07434ad03bdffaadf82daaabc0337bf Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 18 Jul 2017 11:48:37 -0400 Subject: [PATCH 24/28] one more shot! --- source/common/network/connection_impl.cc | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 8ab54f6abdd2f..7ef4d075cd148 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -284,9 +284,19 @@ void ConnectionImpl::setBufferLimits(uint32_t limit) { // asynchronously, we have the option of either setting the watermarks aggressively, and regularly // enabling/disabling reads from the socket, or allowing more data, but then not triggering // based on watermarks until 2x the data is buffered in the common case. Given these are all soft - // limits we err on the side of buffering more and having better performace. + // limits we err on the side of buffering more triggering watermark callbacks less often. + // + // Given the current implementation, if the connection class buffers |limit| bytes it will + // immediately trigger high watermarks, and the common case where |limit| bytes are almost + // immediately flushed to the socket in the subsequent call will trigger low watermarks. We avoid + // this by setting the high watermark to limit + 1. + // // If the connection class is changed to write to the buffer and flush to the socket in the same - // stack, the high watermark should be changed from |limit + 1| to |limit|. + // 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. if (limit > 0) { write_buffer_.setWatermarks(limit / 2, limit + 1); } From b8216e911ad807b7b004d5a1f0eddd8915232d41 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 18 Jul 2017 12:35:06 -0400 Subject: [PATCH 25/28] more clarity on watermark --- source/common/network/connection_impl.cc | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/source/common/network/connection_impl.cc b/source/common/network/connection_impl.cc index 7ef4d075cd148..5b79c92abde9f 100644 --- a/source/common/network/connection_impl.cc +++ b/source/common/network/connection_impl.cc @@ -286,10 +286,11 @@ void ConnectionImpl::setBufferLimits(uint32_t limit) { // based on watermarks until 2x the data is buffered in the common case. Given these are all soft // limits we err on the side of buffering more triggering watermark callbacks less often. // - // Given the current implementation, if the connection class buffers |limit| bytes it will - // immediately trigger high watermarks, and the common case where |limit| bytes are almost - // immediately flushed to the socket in the subsequent call will trigger low watermarks. We avoid - // this by setting the high watermark to limit + 1. + // 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 From d55f9eb0a4765fdefd89e75cedfc02424ddb85da Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 18 Jul 2017 13:08:49 -0400 Subject: [PATCH 26/28] Test RNG --- test/common/network/connection_impl_test.cc | 3 ++- test/test_common/utility.cc | 11 +++++++++++ test/test_common/utility.h | 13 +++++++++++++ 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/test/common/network/connection_impl_test.cc b/test/common/network/connection_impl_test.cc index a02faa043a08d..2a49fa62cc65b 100644 --- a/test/common/network/connection_impl_test.cc +++ b/test/common/network/connection_impl_test.cc @@ -19,6 +19,7 @@ #include "test/test_common/environment.h" #include "test/test_common/network_utility.h" #include "test/test_common/printers.h" +#include "test/test_common/utility.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -392,7 +393,7 @@ TEST_P(ConnectionImplTest, WatermarkFuzzing) { connect(); client_connection_->setBufferLimits(10); - Runtime::RandomGeneratorImpl rand; + TestRandomGenerator rand; int bytes_buffered = 0; int new_bytes_buffered = 0; diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index f9a5f15cfe8c9..bb88c5977a489 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -4,6 +4,7 @@ #include #include +#include #include #include #include @@ -24,6 +25,16 @@ namespace Envoy { +static const uint64_t SEED = std::chrono::duration_cast( + std::chrono::system_clock::now().time_since_epoch()) + .count(); + +TestRandomGenerator::TestRandomGenerator() : generator_(SEED) { + std::cerr << "TestRandomGenerator running with seed " << SEED; +} + +uint64_t TestRandomGenerator::random() { return generator_(); } + bool TestUtility::buffersEqual(const Buffer::Instance& lhs, const Buffer::Instance& rhs) { if (lhs.length() != rhs.length()) { return false; diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 383c17d5eb827..489cbed9dcbf5 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -1,8 +1,11 @@ #pragma once +#include + #include #include #include +#include #include #include @@ -26,6 +29,16 @@ namespace Envoy { EXPECT_EQ(message, std::string(e.what())); \ } +class TestRandomGenerator { +public: + TestRandomGenerator(); + + uint64_t random(); + +private: + std::ranlux48 generator_; +}; + class TestUtility { public: /** From b79fc5eb4de699fc927d1a7b008953729b49db96 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 18 Jul 2017 13:45:50 -0400 Subject: [PATCH 27/28] possibly abusing the random seed flag --- test/test_common/utility.cc | 14 +++++++++----- test/test_common/utility.h | 2 ++ 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index bb88c5977a489..7ca3d38d29df2 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -23,14 +23,18 @@ #include "gtest/gtest.h" #include "spdlog/spdlog.h" +using testing::GTEST_FLAG(random_seed); + namespace Envoy { -static const uint64_t SEED = std::chrono::duration_cast( - std::chrono::system_clock::now().time_since_epoch()) - .count(); +static const int32_t SEED = std::chrono::duration_cast( + std::chrono::system_clock::now().time_since_epoch()) + .count(); -TestRandomGenerator::TestRandomGenerator() : generator_(SEED) { - std::cerr << "TestRandomGenerator running with seed " << SEED; +TestRandomGenerator::TestRandomGenerator() + : generator_(GTEST_FLAG(random_seed) == 0 ? SEED : GTEST_FLAG(random_seed)) { + const int32_t seed = GTEST_FLAG(random_seed) == 0 ? SEED : GTEST_FLAG(random_seed); + std::cerr << "TestRandomGenerator running with seed " << seed; } uint64_t TestRandomGenerator::random() { return generator_(); } diff --git a/test/test_common/utility.h b/test/test_common/utility.h index 489cbed9dcbf5..d8dc0149c412b 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -29,6 +29,8 @@ namespace Envoy { EXPECT_EQ(message, std::string(e.what())); \ } +// Random number generator which logs its seed to stderr. To repeat a test run with a non-zero seed +// one can run the test with --test_arg=--gtest_filter=[seed] class TestRandomGenerator { public: TestRandomGenerator(); From 531b61e35d47336809caa20990fff080140bf4d0 Mon Sep 17 00:00:00 2001 From: Alyssa Wilk Date: Tue, 18 Jul 2017 14:20:21 -0400 Subject: [PATCH 28/28] latching rng seed --- test/test_common/utility.cc | 5 ++--- test/test_common/utility.h | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/test/test_common/utility.cc b/test/test_common/utility.cc index 7ca3d38d29df2..bbe5b667b64aa 100644 --- a/test/test_common/utility.cc +++ b/test/test_common/utility.cc @@ -32,9 +32,8 @@ static const int32_t SEED = std::chrono::duration_cast .count(); TestRandomGenerator::TestRandomGenerator() - : generator_(GTEST_FLAG(random_seed) == 0 ? SEED : GTEST_FLAG(random_seed)) { - const int32_t seed = GTEST_FLAG(random_seed) == 0 ? SEED : GTEST_FLAG(random_seed); - std::cerr << "TestRandomGenerator running with seed " << seed; + : seed_(GTEST_FLAG(random_seed) == 0 ? SEED : GTEST_FLAG(random_seed)), generator_(seed_) { + std::cerr << "TestRandomGenerator running with seed " << seed_; } uint64_t TestRandomGenerator::random() { return generator_(); } diff --git a/test/test_common/utility.h b/test/test_common/utility.h index d8dc0149c412b..0954865825cd2 100644 --- a/test/test_common/utility.h +++ b/test/test_common/utility.h @@ -38,6 +38,7 @@ class TestRandomGenerator { uint64_t random(); private: + const int32_t seed_; std::ranlux48 generator_; };