Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
87bb932
Add TCP_FASTOPEN listener option
bmetzdorf Feb 3, 2018
cdfeb38
Merge branch 'master' into tfo
ggreenway Apr 2, 2018
982a27b
Update to use new socket setting facility
ggreenway Apr 2, 2018
7cc3768
Fix crash when no options present
ggreenway Apr 3, 2018
8dcf11d
Don't set TRANSPARENT option in state Listening
ggreenway Apr 3, 2018
6f55c84
Add more test coverage
ggreenway Apr 3, 2018
dea8e2c
Make the option tunable, not just boolean
ggreenway Apr 3, 2018
cc211dd
Add test for setting Listening socket options
ggreenway Apr 3, 2018
d9ba97a
Fixes
ggreenway Apr 3, 2018
e56a8d7
test typo
ggreenway Apr 3, 2018
f2db864
Fix misspelled comments
ggreenway Apr 3, 2018
9f3c97e
one more misspelled comment
ggreenway Apr 4, 2018
d2d4a27
Updated data-plane-api commit
bmetzdorf Apr 5, 2018
a03533a
fix typo
bmetzdorf Apr 5, 2018
89a3b99
Merge branch 'master' of https://github.com/envoyproxy/envoy into tfo…
bmetzdorf Apr 5, 2018
8289daa
set listening socket options in ListenerSocketOption
bmetzdorf Apr 5, 2018
e7ff081
move ListenerSocketOption into its own file
bmetzdorf Apr 5, 2018
8f40b58
add listener_socket_option_impl_test
bmetzdorf Apr 5, 2018
42ad7e4
move ENVOY_SOCKET_TCP_FASTOPEN to listener_socket_option_impl.h
bmetzdorf Apr 5, 2018
f90b61f
document Socket::SocketState
bmetzdorf Apr 6, 2018
1d5734a
removing testing::AtLeast
bmetzdorf Apr 6, 2018
9e57d17
fix formatting
bmetzdorf Apr 6, 2018
5abe337
just keep constructor prototypes in listener_socket_option_impl.h
bmetzdorf Apr 6, 2018
2b21dd5
move "#include <netinet/tcp.h>"
bmetzdorf Apr 6, 2018
8626895
document SocketState prettier
bmetzdorf Apr 6, 2018
521faad
Merge remote-tracking branch 'upstream/master' into tfo
ggreenway Apr 10, 2018
cb298fb
Merge remote-tracking branch 'upstream/master' into tfo
ggreenway Apr 12, 2018
4219d2e
document SocketState even prettier
bmetzdorf Apr 12, 2018
2f47e3d
Merge remote-tracking branch 'upstream-public/master' into tfo-public
bmetzdorf Apr 12, 2018
b9312fd
Call parent class setOption()
ggreenway Apr 12, 2018
06d426e
Merge branch 'tfo' of https://github.com/bmetzdorf/envoy into tfo-public
bmetzdorf Apr 12, 2018
f9f608a
add os_sys_calls bazel dependency
bmetzdorf Apr 12, 2018
2ca16c3
return false if TFO is requested but not supported
bmetzdorf Apr 12, 2018
470f934
Merge remote-tracking branch 'upstream-public/master' into tfo-public
bmetzdorf Apr 16, 2018
e0dbd9f
Add TCP_FASTOPEN listener option release notes
bmetzdorf Apr 16, 2018
3dd9372
move TCP_FASTOPEN into socket_option_impl.h
bmetzdorf Apr 17, 2018
3f11ee6
Don't ignore return value from super-call
ggreenway Apr 17, 2018
ec0fa87
Merge remote-tracking branch 'upstream/master' into tfo
ggreenway Apr 17, 2018
4494d2c
ListenerSocketOptionImplTest should inherit from SocketOptionImplTest
bmetzdorf Apr 18, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,8 @@ REPOSITORY_LOCATIONS = dict(
urls = ["https://github.com/google/protobuf/archive/v3.5.0.tar.gz"],
),
envoy_api = dict(
commit = "92ffa417b5d2e8ab3912ff4da4daa6aa93f8df0f",
remote = "https://github.com/envoyproxy/data-plane-api",
commit = "28c23f754539369c57b28359067f100059ce0d0b",
remote = "https://github.com/bmetzdorf/data-plane-api.git", # TODO FIXME
),
grpc_httpjson_transcoding = dict(
commit = "e4f58aa07b9002befa493a0a82e10f2e98b51fc6",
Expand Down
2 changes: 1 addition & 1 deletion include/envoy/network/listen_socket.h
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ class Socket {
*/
virtual void close() PURE;

enum class SocketState { PreBind, PostBind };
enum class SocketState { PreBind, PostBind, Listening };

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think at this point we need clear comments on each of these to explain when they happen in the socket lifetime and relative to each other. Not sure why we need a Listening in addition to PostBind for example.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agreed.

I'll put this in the comments, but the reason for the new state is that on macOS, you cannot set this option until after listen() has been called. PostBind is run between bind() and listen().

Can you think of a better name for PostBind, now that this 3rd state is added? Or just comment it heavily and keep the name?

@htuch htuch Apr 5, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think it's fine to just comment. As long as there is a clear order in the comments wrt the socket creation -> accept life cycle it's all good.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

SocketState is documented


/**
* Visitor class for setting socket options.
Expand Down
9 changes: 9 additions & 0 deletions source/common/network/listener_impl.cc
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
#include "common/network/listener_impl.h"

#include <netinet/tcp.h>
#include <sys/un.h>

#include "envoy/common/exception.h"
Expand Down Expand Up @@ -66,6 +67,14 @@ ListenerImpl::ListenerImpl(Event::DispatcherImpl& dispatcher, Socket& socket, Li
fmt::format("cannot listen on socket: {}", socket.localAddress()->asString()));
}

for (auto& option : *socket.options()) {
if (!option->setOption(socket, Socket::SocketState::Listening)) {
throw CreateListenerException(
fmt::format("cannot set post-listen socket option on socket: {}",
socket.localAddress()->asString()));
}
}

evconnlistener_set_error_cb(listener_.get(), errorCallback);
}
}
Expand Down
19 changes: 18 additions & 1 deletion source/common/network/socket_option_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,25 @@ bool SocketOptionImpl::setOption(Socket& socket, Socket::SocketState state) cons
return false;
}
}
}
} else if (state == Socket::SocketState::Listening) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good catch!

if (accept_tcp_fast_open_.has_value()) {
const int tfo_value = accept_tcp_fast_open_.value() ? ENVOY_TCP_FASTOPEN_BACKLOG : 0;
const SocketOptionName option_name = ENVOY_SOCKET_TCP_FASTOPEN;
int error = -1;
if (option_name) {
error = Api::OsSysCallsSingleton::get().setsockopt(
socket.fd(), IPPROTO_TCP, option_name.value(),
reinterpret_cast<const void*>(&tfo_value), sizeof(tfo_value));
} else {
error = ENOTSUP;
}

if (error != 0) {
ENVOY_LOG(warn, "Setting IP_TCP_FASTOPEN on listener socket failed: {}", strerror(error));
return false;
}
}
}
return true;
}

Expand Down
23 changes: 21 additions & 2 deletions source/common/network/socket_option_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,28 @@ typedef absl::optional<int> SocketOptionName;
#define ENVOY_SOCKET_IPV6_FREEBIND Network::SocketOptionName()
#endif

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

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

#endif

// On macOS the socket MUST be listening already for TCP_FASTOPEN to be set and backlog MUST be 1
// (the actual value is set via the net.inet.tcp.fastopen_backlog kernel parameter.
// For Linux we default to 128, which libevent is using in
// https://github.com/libevent/libevent/blob/release-2.1.8-stable/listener.c#L176
#if defined(__APPLE__)
#define ENVOY_TCP_FASTOPEN_BACKLOG 1
#else
#define ENVOY_TCP_FASTOPEN_BACKLOG 128
#endif

class SocketOptionImpl : public Socket::Option, Logger::Loggable<Logger::Id::connection> {
public:
SocketOptionImpl(absl::optional<bool> transparent, absl::optional<bool> freebind)
: transparent_(transparent), freebind_(freebind) {}
SocketOptionImpl(absl::optional<bool> transparent, absl::optional<bool> freebind,
absl::optional<bool> accept_tcp_fast_open)
: transparent_(transparent), freebind_(freebind),
accept_tcp_fast_open_(accept_tcp_fast_open) {}

// Socket::Option
bool setOption(Socket& socket, Socket::SocketState state) const override;
Expand Down Expand Up @@ -72,6 +90,7 @@ class SocketOptionImpl : public Socket::Option, Logger::Loggable<Logger::Id::con
private:
const absl::optional<bool> transparent_;
const absl::optional<bool> freebind_;
const absl::optional<bool> accept_tcp_fast_open_;
};

} // namespace Network
Expand Down
8 changes: 5 additions & 3 deletions source/common/upstream/upstream_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,11 @@ uint64_t parseFeatures(const envoy::api::v2::Cluster& config,
class UpstreamSocketOption : public Network::SocketOptionImpl {
public:
UpstreamSocketOption(const ClusterInfo& cluster_info)
: Network::SocketOptionImpl({}, cluster_info.features() & ClusterInfo::Features::FREEBIND
? true
: absl::optional<bool>{}) {}
: Network::SocketOptionImpl({},
cluster_info.features() & ClusterInfo::Features::FREEBIND
? true
: absl::optional<bool>{},
{}) {}
};

} // namespace
Expand Down
4 changes: 3 additions & 1 deletion source/server/listener_manager_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,9 @@ class ListenerSocketOption : public Network::SocketOptionImpl {
ListenerSocketOption(const envoy::api::v2::Listener& config)
: Network::SocketOptionImpl(
PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, transparent, absl::optional<bool>{}),
PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional<bool>{})) {}
PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, absl::optional<bool>{}),
PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, accept_tcp_fast_open, absl::optional<bool>{})) {
}
};

ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, ListenerManagerImpl& parent,
Expand Down
110 changes: 43 additions & 67 deletions test/common/network/socket_option_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,24 @@ class SocketOptionImplTest : public testing::Test {
NiceMock<MockListenSocket> socket_;
Api::MockOsSysCalls os_sys_calls_;
TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> os_calls{&os_sys_calls_};

void testSetSocketOptionSuccess(SocketOptionImpl& socket_option, int socket_level,
Network::SocketOptionName option_name, int option_val,
Socket::SocketState when) {
if (option_name.has_value()) {
Address::Ipv4Instance address("1.2.3.4", 5678);
const int fd = address.socket(Address::SocketType::Stream);
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));
EXPECT_CALL(os_sys_calls_, setsockopt_(_, socket_level, option_name.value(), _, sizeof(int)))
.WillOnce(Invoke([option_val](int, int, int, const void* optval, socklen_t) -> int {
EXPECT_EQ(option_val, *static_cast<const int*>(optval));
return 0;
}));
EXPECT_TRUE(socket_option.setOption(socket_, when));
} else {
EXPECT_FALSE(socket_option.setOption(socket_, when));
}
}
};

// We fail to set the option if the socket FD is bad.
Expand All @@ -33,105 +51,63 @@ TEST_F(SocketOptionImplTest, BadFd) {

// Nop when there are no socket options set.
TEST_F(SocketOptionImplTest, SetOptionEmptyNop) {
SocketOptionImpl socket_option{{}, {}};
SocketOptionImpl socket_option{{}, {}, {}};
EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PostBind));
EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::Listening));
}

// We fail to set the option when the underlying setsockopt syscall fails.
TEST_F(SocketOptionImplTest, SetOptionTransparentFailure) {
SocketOptionImpl socket_option{true, {}};
SocketOptionImpl socket_option{true, {}, {}};
EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
}

// We fail to set the option when the underlying setsockopt syscall fails.
TEST_F(SocketOptionImplTest, SetOptionFreebindFailure) {
SocketOptionImpl socket_option{{}, true};
SocketOptionImpl socket_option{{}, true, {}};
EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
}

// We fail to set the tcp-fastopen option when the underlying setsockopt syscall fails.
TEST_F(SocketOptionImplTest, SetOptionTcpFastopenFailure) {
SocketOptionImpl socket_option{{}, {}, true};
EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::Listening));
}

// The happy path for setOption(); IP_TRANSPARENT is set to true.
TEST_F(SocketOptionImplTest, SetOptionTransparentSuccessTrue) {
SocketOptionImpl socket_option{true, {}};
if (ENVOY_SOCKET_IP_TRANSPARENT.has_value()) {
Address::Ipv4Instance address("1.2.3.4", 5678);
const int fd = address.socket(Address::SocketType::Stream);
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));
EXPECT_CALL(os_sys_calls_,
setsockopt_(_, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT.value(), _, sizeof(int)))
.WillOnce(Invoke([](int, int, int, const void* optval, socklen_t) -> int {
EXPECT_EQ(1, *static_cast<const int*>(optval));
return 0;
}));
EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
} else {
EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
}
SocketOptionImpl socket_option{true, {}, {}};
testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 1,
Socket::SocketState::PreBind);
}

// The happy path for setOption(); IP_FREEBIND is set to true.
TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessTrue) {
SocketOptionImpl socket_option{{}, true};
if (ENVOY_SOCKET_IP_FREEBIND.has_value()) {
Address::Ipv4Instance address("1.2.3.4", 5678);
const int fd = address.socket(Address::SocketType::Stream);
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));
EXPECT_CALL(os_sys_calls_,
setsockopt_(_, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND.value(), _, sizeof(int)))
.WillOnce(Invoke([](int, int, int, const void* optval, socklen_t) -> int {
EXPECT_EQ(1, *static_cast<const int*>(optval));
return 0;
}));
EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
} else {
EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
}
SocketOptionImpl socket_option{{}, true, {}};
testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 1,
Socket::SocketState::PreBind);
}

// The happy path for setOpion(); IP_TRANSPARENT is set to false.
TEST_F(SocketOptionImplTest, SetOptionTransparentSuccessFalse) {
SocketOptionImpl socket_option{false, {}};
if (ENVOY_SOCKET_IP_TRANSPARENT.has_value()) {
Address::Ipv4Instance address("1.2.3.4", 5678);
const int fd = address.socket(Address::SocketType::Stream);
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));
EXPECT_CALL(os_sys_calls_,
setsockopt_(_, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT.value(), _, sizeof(int)))
.Times(2)
.WillRepeatedly(Invoke([](int, int, int, const void* optval, socklen_t) -> int {
EXPECT_EQ(0, *static_cast<const int*>(optval));
return 0;
}));
EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PostBind));
} else {
EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PostBind));
}
SocketOptionImpl socket_option{false, {}, {}};
testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 0,
Socket::SocketState::PreBind);
testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_TRANSPARENT, 0,
Socket::SocketState::PostBind);
}

// The happy path for setOpion(); IP_FREEBIND is set to false.
TEST_F(SocketOptionImplTest, SetOptionFreebindSuccessFalse) {
SocketOptionImpl socket_option{{}, false};
if (ENVOY_SOCKET_IP_FREEBIND.has_value()) {
Address::Ipv4Instance address("1.2.3.4", 5678);
const int fd = address.socket(Address::SocketType::Stream);
EXPECT_CALL(socket_, fd()).WillRepeatedly(Return(fd));
EXPECT_CALL(os_sys_calls_,
setsockopt_(_, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND.value(), _, sizeof(int)))
.WillOnce(Invoke([](int, int, int, const void* optval, socklen_t) -> int {
EXPECT_EQ(0, *static_cast<const int*>(optval));
return 0;
}));
EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
} else {
EXPECT_FALSE(socket_option.setOption(socket_, Socket::SocketState::PreBind));
}
SocketOptionImpl socket_option{{}, false, {}};
testSetSocketOptionSuccess(socket_option, IPPROTO_IP, ENVOY_SOCKET_IP_FREEBIND, 0,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nice cleanup!

Socket::SocketState::PreBind);
}

// Freebind settings have no effect on post-bind behavior.
TEST_F(SocketOptionImplTest, SetOptionFreebindPostBind) {
SocketOptionImpl socket_option{{}, true};
SocketOptionImpl socket_option{{}, true, {}};
EXPECT_TRUE(socket_option.setOption(socket_, Socket::SocketState::PostBind));
}

Expand Down