Introduce listener TCP connection buffer configuration and implement … #558

htuch · 2017-03-10T01:43:49Z

…read buffer limits (#150).

Baby steps towards flow control. This patch plumbs the TCP connection buffer limits to the listener
and enforces it on the read buffer only. Later patches will also plumb to the upstream cluster
connection and cover the write buffer (which requires the watermark API).

…read buffer limits (envoyproxy#150). Baby steps towards flow control. This patch plumbs the TCP connection buffer limits to the listener and enforces it on the read buffer only. Later patches will also plumb to the upstream cluster connection and cover the write buffer (which requires the watermark API).

mattklein123 · 2017-03-10T21:16:06Z

I'm trying to unbury myself. I will look at this later today or this weekend.

ccaraman · 2017-03-10T21:23:19Z

docs/configuration/listeners/listeners.rst

@@ -52,6 +52,9 @@ use_original_dst
  destination port. If there is no listener associated with the original destination port, the
  connection is handled by the listener that receives it. Default is false.

+per_connection_buffer_limit_bytes
+  *(optional, integer)* Soft limit on size of the listener's new connection read and write buffers. If unspecified, an implementation defined default is applied.


Please specify the default value here.

ccaraman · 2017-03-10T21:24:12Z

source/common/network/connection_impl.h

+    return read_buffer_limit_ > 0 && read_buffer_.length() >= read_buffer_limit_;
+  }
+  // Mark read buffer ready to read in the event loop. This is used when yielding following
+  // shouldDrainReadBuffer(). TODO(htuch): While this is the basis for also yielding to other


nit: put todo on new line.

mattklein123

Looks good. A few (annoying, sorry) comments.

mattklein123 · 2017-03-11T03:29:56Z

docs/configuration/listeners/listeners.rst

@@ -52,6 +52,9 @@ use_original_dst
  destination port. If there is no listener associated with the original destination port, the
  connection is handled by the listener that receives it. Default is false.

+per_connection_buffer_limit_bytes
+  *(optional, integer)* Soft limit on size of the listener's new connection read and write buffers. If unspecified, an implementation defined default is applied (1MB).


nit: 100 col line break

mattklein123 · 2017-03-11T03:31:29Z

include/envoy/event/dispatcher.h

   * @return Network::ListenerPtr a new listener that is owned by the caller.
   */
  virtual Network::ListenerPtr createListener(Network::ConnectionHandler& conn_handler,
                                              Network::ListenSocket& socket,
                                              Network::ListenerCallbacks& cb,
                                              Stats::Store& stats_store, bool bind_to_port,
-                                              bool use_proxy_proto, bool use_orig_dst) PURE;
+                                              bool use_proxy_proto, bool use_orig_dst,


Sorry, to ask you to do this, but I have a feeling we are going to keep adding params/options here. (I know soon we will split use_original_dst into use_orginal_port/use_original_address, etc.). Can we define a struct ListenerOptions and just pass that. That way we don't have to change a gazillion mocks and callsites the next time we add a param.

mattklein123 · 2017-03-11T03:32:28Z

include/envoy/event/dispatcher.h

   * @return Network::ListenerPtr a new listener that is owned by the caller.
   */
  virtual Network::ListenerPtr createListener(Network::ConnectionHandler& conn_handler,
                                              Network::ListenSocket& socket,
                                              Network::ListenerCallbacks& cb,
                                              Stats::Store& stats_store, bool bind_to_port,
-                                              bool use_proxy_proto, bool use_orig_dst) PURE;
+                                              bool use_proxy_proto, bool use_orig_dst,
+                                              size_t per_connection_buffer_limit_bytes) PURE;


Instead of size_t, can we be specific with either uint32_t or uint64_t (probably uint32_t). Same applies all the other places we reference this.

mattklein123 · 2017-03-11T03:33:56Z

source/common/json/config_schemas.cc

@@ -42,7 +42,12 @@ const std::string Json::Schema::LISTENER_SCHEMA(R"EOF(
       "ssl_context" : {"$ref" : "#/definitions/ssl_context"},
       "bind_to_port" : {"type": "boolean"},
       "use_proxy_proto" : {"type" : "boolean"},
-       "use_original_dst" : {"type" : "boolean"}
+       "use_original_dst" : {"type" : "boolean"},
+       "per_connection_buffer_limit_bytes" : {


"per_connection_read_buffer_limit_bytes" ? Next we will have write high/low watermark so might get confusing?

I was planning on using per_connection_buffer_limit_bytes to drive both the watermark for the write buffer (automatically setting low watermark at a fraction like 0.5) and the read buffer, under the assumption that we want to keep config simple and that read/write buffer limits should be somewhat symmetric in general. I know there are scenarios where this isn't true, but this could be reasonable for v1.

OK sounds good.

mattklein123 · 2017-03-13T19:14:10Z

include/envoy/network/connection.h

+   * Set a soft limit on the size of the read buffer prior to flushing to further stages in the
+   * processing pipeline.
+   */
+  virtual void setReadBufferLimit(size_t limit) PURE;


mattklein123 · 2017-03-13T19:14:28Z

include/envoy/network/listener.h

+    return {.bind_to_port_ = true,
+            .use_proxy_proto_ = false,
+            .use_original_dst_ = false,
+            .per_connection_buffer_limit_bytes_ = false};


mattklein123 · 2017-03-13T19:15:27Z

source/common/network/connection_impl.h

@@ -87,6 +98,7 @@ class ConnectionImpl : public virtual Connection,
  Address::InstancePtr local_address_;
  Buffer::OwnedImpl read_buffer_;
  Buffer::OwnedImpl write_buffer_;
+  size_t read_buffer_limit_ = 0;


mattklein123 · 2017-03-13T19:16:04Z

source/common/network/connection_impl.h

@@ -63,6 +63,7 @@ class ConnectionImpl : public virtual Connection,
  Ssl::Connection* ssl() override { return nullptr; }
  State state() override;
  void write(Buffer::Instance& data) override;
+  void setReadBufferLimit(size_t limit) override { read_buffer_limit_ = limit; }


mattklein123 · 2017-03-13T19:16:55Z

source/common/network/listener_impl.h

@@ -47,6 +47,7 @@ class ListenerImpl : public Listener {
  const bool use_proxy_proto_;
  ProxyProtocol proxy_protocol_;
  const bool use_original_dst_;
+  const size_t per_connection_buffer_limit_bytes_;


Just have a ListenerOptions as a member var and then you can just copy it in via passed in options.

Signed-off-by: Shikugawa <Shikugawa@gmail.com>

Because we shared the `release` bazel config between both iOS and Android, Android builds were incorrectly having llvm bitcode enabled for them. Signed-off-by: Keith Smiley <keithbsmiley@gmail.com> Signed-off-by: JP Simard <jp@jpsim.com>

ccaraman reviewed Mar 10, 2017

View reviewed changes

htuch added 2 commits March 10, 2017 17:10

@ccaraman review feedback.

a7b3478

Some configuration tests for the per connection buffer limit.

5df5456

mattklein123 reviewed Mar 11, 2017

View reviewed changes

htuch added 2 commits March 13, 2017 14:03

@mattklein123 review feedback.

9c66fb6

Merge remote-tracking branch 'upstream/master' into flow-control-config

0b27b80

mattklein123 reviewed Mar 13, 2017

View reviewed changes

Further @mattklein123 review feedback.

0f14891

mattklein123 approved these changes Mar 13, 2017

View reviewed changes

mattklein123 merged commit fd58242 into envoyproxy:master Mar 13, 2017

htuch deleted the flow-control-config branch March 14, 2017 15:09

mattklein123 mentioned this pull request Mar 18, 2017

SSL connection write event crash #585

Closed

lambdai pushed a commit to lambdai/envoy-dai that referenced this pull request Jul 21, 2020

bazel: wasm to wasm32 (envoyproxy#558)

1fc841d

Signed-off-by: Shikugawa <Shikugawa@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce listener TCP connection buffer configuration and implement … #558

Introduce listener TCP connection buffer configuration and implement … #558

htuch commented Mar 10, 2017

mattklein123 commented Mar 10, 2017

ccaraman Mar 10, 2017

ccaraman Mar 10, 2017

mattklein123 left a comment

mattklein123 Mar 11, 2017

mattklein123 Mar 11, 2017

mattklein123 Mar 11, 2017

mattklein123 Mar 11, 2017

htuch Mar 13, 2017

mattklein123 Mar 13, 2017

mattklein123 Mar 13, 2017

mattklein123 Mar 13, 2017

mattklein123 Mar 13, 2017

mattklein123 Mar 13, 2017

mattklein123 Mar 13, 2017

Introduce listener TCP connection buffer configuration and implement … #558

Introduce listener TCP connection buffer configuration and implement … #558

Conversation

htuch commented Mar 10, 2017

mattklein123 commented Mar 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment