connection: Add TCP_FASTOPEN listener option by bmetzdorf · Pull Request #2793 · envoyproxy/envoy

bmetzdorf · 2018-03-13T19:03:17Z

connection: Add TCP_FASTOPEN listener option

This allows accepting TCP_FASTOPEN based connections. Works on both Linux and OSX.

Feedback on this is very welcome. It was tested manually on Linux and OSX.

API Changes: envoyproxy/data-plane-api#539

Signed-off-by: Bjoern Metzdorf bmetzdorf@apple.com
Risk Level: Low

ggreenway · 2018-03-13T20:19:31Z

@dnoe This is related to the other socket-option work you're looking at.

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

mattklein123 · 2018-03-16T19:59:28Z

Per new release note policy please add a release note with any applicable feature docs to https://github.com/envoyproxy/data-plane-api/blob/master/docs/root/intro/version_history.rst. Thank you!

bmetzdorf · 2018-03-17T12:45:24Z

@mattklein123 I've added a note in envoyproxy/data-plane-api#539

alyssawilk · 2018-03-26T13:03:15Z

@bmetzdorf #2734 is in - are you up for pulling and merging?

bmetzdorf · 2018-03-27T02:50:57Z

@alyssawilk yep! Will do in the next couple of days!

bmetzdorf · 2018-03-29T00:24:28Z

@alyssawilk I think I need to wait for #2719 to be merged.

Signed-off-by: Greg Greenway <ggreenway@apple.com>

rlenglet · 2018-04-03T00:51:59Z

source/common/network/socket_option_impl.cc

Please wrap the IP_TRANSPARENT option setting above into if (state != Socket::SocketState::Listening) { ... }.

Good catch!

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway · 2018-04-03T22:53:12Z

source/server/listener_manager_impl.cc

+
+      // Add the options to the socket_ so that SocketState::Listening options can be
+      // set in the worker after listen()/evconnlistener_new() is called.
+      socket_->addOption(option);


Does adding these options here break anything else?

ggreenway · 2018-04-03T23:03:26Z

I wrote a lot of this code, so removing myself as reviewer

bmetzdorf · 2018-04-03T23:09:32Z

I've tested this latest commit f2db8640b2e576a4c978f461f936cd6dfeaf43cf on both macOS and Linux successfully.

macOS:
tcp_fast_open_queue_length: 0 == TFO does not work
tcp_fast_open_queue_length: 1 == TFO does work
tcp_fast_open_queue_length: 2 == "Invalid argument"

Linux:
tcp_fast_open_queue_length: 0 == TFO does not work
tcp_fast_open_queue_length: 512 == TFO does work

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

htuch · 2018-04-09T14:55:52Z

include/envoy/network/listen_socket.h


-  enum class SocketState { PreBind, PostBind };
+  enum class SocketState {
+    PreBind,   // after socket creation but before binding the socket to a port


I'd suggest moving these to the line previous, e.g.

// Socket options are applied after socket creation but before... Prebind,

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

htuch

This looks great, just a few more comments.

htuch · 2018-04-13T05:49:14Z

source/server/listener_socket_option_impl.h

+#ifdef TCP_FASTOPEN
+#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName(TCP_FASTOPEN)
+#else
+#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName()


Have you tried manually forcing this option and seeing what happens to the tests? In my experience, this is a useful step.

@htuch Do you mean just enforce TCP_FASTOPEN on all listeners by default and then run the tests?

I mean as an experiment in your local client, change that #ifdef line to #if 0 and see if bazel test //test/... passes.

htuch · 2018-04-13T05:49:37Z

source/server/listener_socket_option_impl.h

+
+#include "server/configuration_impl.h"
+
+#ifdef TCP_FASTOPEN


I think this can live in socket_option_impl.h alongside the others.

htuch · 2018-04-13T05:51:00Z

test/common/network/listener_impl_test.cc

+  std::shared_ptr<MockSocketOption> option = std::make_shared<MockSocketOption>();
+  socket.addOption(option);
+  EXPECT_CALL(*option, setOption(_, Socket::SocketState::Listening)).WillOnce(Return(false));
+  EXPECT_THROW(TestListenerImpl(dispatcher, socket, listener_callbacks, true, false),


Could make this EXPECT_THROW_WITH_MESSAGE to get better precision on why we see the exception.

Yes, I'm working on this one.

htuch · 2018-04-13T05:53:01Z

test/common/network/socket_option_impl_test.cc

-TEST_F(SocketOptionImplTest, SetOptionFreebindPostBind) {
-  SocketOptionImpl socket_option{{}, true};
-  EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PostBind));
+  testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 0,


Nice cleanup!

htuch · 2018-04-13T05:54:02Z

test/server/listener_socket_option_impl_test.cc

+  Api::MockOsSysCalls os_sys_calls_;
+  TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls{&os_sys_calls_};
+
+  void testSetSocketOptionSuccess(ListenerSocketOptionImpl& socket_option, int socket_level,


Could this subclass SocketOptionImplTest and inherit this method?

@bmetzdorf can you give me a quick yes/no on the various feedback? Thanks!

Yes, I'm working on this one.

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

Signed-off-by: Greg Greenway <ggreenway@apple.com>

bmetzdorf · 2018-04-17T22:02:01Z

source/common/network/socket_option_impl.h

+#ifdef TCP_FASTOPEN
+#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName(TCP_FASTOPEN)
+#else
+#define ENVOY_SOCKET_TCP_FASTOPEN Network::SocketOptionName()


@htuch Do you mean just enforce TCP_FASTOPEN on all listeners by default and then run the tests?

htuch · 2018-04-17T22:16:19Z

source/server/listener_socket_option_impl.cc

+          ENVOY_LOG(debug, "Successfully set socket option TCP_FASTOPEN to {}", tfo_value);
+        }
+      } else {
+        ENVOY_LOG(warn, "Unsupported socket option TCP_FASTOPEN");


This case is missing coverage.

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

bmetzdorf · 2018-04-18T04:30:09Z

@htuch Please let me know if ListenerSocketOptionImplTest is now inheriting the way you envisioned.

htuch · 2018-04-18T14:03:22Z

test/common/network/socket_option_test_harness.h

+namespace Network {
+namespace {
+
+class SocketOptionTestHarness : public testing::Test {


Yep, this looks good. As a nit, I would probably call it SocketOptionTestBase or SocketOptionTest.

htuch

Thanks @bmetzdorf. Merging to unblock #3042, @bmetzdorf has signed up to address remaining minor feedback in a followup PR.

Add TCP_FASTOPEN listener option API changes for: envoyproxy/envoy#2793 Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

bmetzdorf mentioned this pull request Mar 13, 2018

lds: Add TCP_FASTOPEN listener option envoyproxy/data-plane-api#539

Merged

Add TCP_FASTOPEN listener option

87bb932

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

bmetzdorf force-pushed the tfo branch from a63745d to 87bb932 Compare March 13, 2018 23:49

ggreenway mentioned this pull request Mar 15, 2018

listen_socket: Support multiple options and allow socket options to be set before bind(). #2734

Merged

mattklein123 assigned ggreenway Mar 17, 2018

ggreenway added 2 commits April 2, 2018 15:01

Merge branch 'master' into tfo

cdfeb38

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Update to use new socket setting facility

982a27b

Signed-off-by: Greg Greenway <ggreenway@apple.com>

rlenglet reviewed Apr 3, 2018

View reviewed changes

ggreenway added 5 commits April 3, 2018 10:06

Fix crash when no options present

7cc3768

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Don't set TRANSPARENT option in state Listening

8dcf11d

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Add more test coverage

6f55c84

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Make the option tunable, not just boolean

dea8e2c

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Add test for setting Listening socket options

cc211dd

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway changed the title ~~Work in progress: connection: Add TCP_FASTOPEN listener option~~ connection: Add TCP_FASTOPEN listener option Apr 3, 2018

Fixes

d9ba97a

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway force-pushed the tfo branch from afc0755 to d9ba97a Compare April 3, 2018 22:46

ggreenway added 2 commits April 3, 2018 15:47

test typo

e56a8d7

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Fix misspelled comments

f2db864

Signed-off-by: Greg Greenway <ggreenway@apple.com>

ggreenway reviewed Apr 3, 2018

View reviewed changes

ggreenway removed their assignment Apr 3, 2018

ggreenway assigned htuch Apr 3, 2018

one more misspelled comment

9f3c97e

Signed-off-by: Greg Greenway <ggreenway@apple.com>

document SocketState prettier

8626895

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

bmetzdorf force-pushed the tfo branch from 3ce02f7 to 8626895 Compare April 6, 2018 01:05

ggreenway mentioned this pull request Apr 9, 2018

build: Disable compiler warning maybe-initialized. #3031

Merged

Merge remote-tracking branch 'upstream/master' into tfo

521faad

htuch reviewed Apr 11, 2018

View reviewed changes

ggreenway and others added 7 commits April 12, 2018 14:18

Merge remote-tracking branch 'upstream/master' into tfo

cb298fb

document SocketState even prettier

4219d2e

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

Merge remote-tracking branch 'upstream-public/master' into tfo-public

2f47e3d

Call parent class setOption()

b9312fd

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge branch 'tfo' of https://github.com/bmetzdorf/envoy into tfo-public

06d426e

add os_sys_calls bazel dependency

f9f608a

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

return false if TFO is requested but not supported

2ca16c3

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

htuch reviewed Apr 13, 2018

View reviewed changes

htuch mentioned this pull request Apr 16, 2018

Add TCP_FASTOPEN listener option release notes envoyproxy/data-plane-api#608

Closed

bmetzdorf and others added 5 commits April 16, 2018 12:59

Merge remote-tracking branch 'upstream-public/master' into tfo-public

470f934

Add TCP_FASTOPEN listener option release notes

e0dbd9f

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

move TCP_FASTOPEN into socket_option_impl.h

3dd9372

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

Don't ignore return value from super-call

3f11ee6

Signed-off-by: Greg Greenway <ggreenway@apple.com>

Merge remote-tracking branch 'upstream/master' into tfo

ec0fa87

Signed-off-by: Greg Greenway <ggreenway@apple.com>

bmetzdorf commented Apr 17, 2018

View reviewed changes

htuch reviewed Apr 17, 2018

View reviewed changes

ListenerSocketOptionImplTest should inherit from SocketOptionImplTest

4494d2c

Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>

bmetzdorf force-pushed the tfo branch from f6822d2 to 4494d2c Compare April 18, 2018 04:28

htuch reviewed Apr 18, 2018

View reviewed changes

htuch mentioned this pull request Apr 18, 2018

socket: add SO_KEEPALIVE option for upstream connections. #3042

Merged

htuch approved these changes Apr 18, 2018

View reviewed changes

htuch merged commit 55a95f4 into envoyproxy:master Apr 18, 2018

bmetzdorf mentioned this pull request Apr 20, 2018

connection: Add TCP_FASTOPEN listener option (continuation) #3157

Merged

Elite1015 pushed a commit to Elite1015/data-plane-api that referenced this pull request Feb 23, 2025

Add TCP_FASTOPEN listener option (#539)

9d3dc80

Add TCP_FASTOPEN listener option API changes for: envoyproxy/envoy#2793 Signed-off-by: Bjoern Metzdorf <bmetzdorf@apple.com>


		#include "server/configuration_impl.h"

		#ifdef TCP_FASTOPEN

Conversation

bmetzdorf commented Mar 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggreenway commented Mar 13, 2018

Uh oh!

mattklein123 commented Mar 16, 2018

Uh oh!

bmetzdorf commented Mar 17, 2018

Uh oh!

alyssawilk commented Mar 26, 2018

Uh oh!

bmetzdorf commented Mar 27, 2018

Uh oh!

bmetzdorf commented Mar 29, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ggreenway commented Apr 3, 2018

Uh oh!

bmetzdorf commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmetzdorf commented Apr 18, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bmetzdorf commented Mar 13, 2018 •

edited

Loading