Skip to content

udp: no SO_REUSEPORT if not explicitly configured #9495

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mattklein123 merged 6 commits into from
Dec 31, 2019
Merged

udp: no SO_REUSEPORT if not explicitly configured #9495

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d504d0f
no reuseport if concurrency is 1
danzh1989 Dec 26, 2019
1770aa2
reuse_port apply to UDP
danzh1989 Dec 27, 2019
3145454
update doc
danzh1989 Dec 27, 2019
68c9d1e
comment
danzh1989 Dec 30, 2019
bee7f0f
Merge branch 'master' into fix_reuseport
danzh1989 Dec 30, 2019
19c8cbb
fix proto format
danzh1989 Dec 30, 2019
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
1 change: 0 additions & 1 deletion api/envoy/api/v2/lds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,6 @@ message Listener {
// create one socket for each worker thread. This makes inbound connections
// distribute among worker threads roughly evenly in cases where there are a high number
// of connections. When this flag is set to false, all worker threads share one socket.
// For UDP this flag is set to true forcibly.
//
// Before Linux v4.19-rc1, new TCP connections may be rejected during hot restart
// (see `3rd paragraph in 'soreuseport' commit message
Expand Down
1 change: 0 additions & 1 deletion api/envoy/api/v3alpha/lds.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,7 +238,6 @@ message Listener {
// create one socket for each worker thread. This makes inbound connections
// distribute among worker threads roughly evenly in cases where there are a high number
// of connections. When this flag is set to false, all worker threads share one socket.
// For UDP this flag is set to true forcibly.
//
// Before Linux v4.19-rc1, new TCP connections may be rejected during hot restart
// (see `3rd paragraph in 'soreuseport' commit message
Expand Down
8 changes: 6 additions & 2 deletions source/server/listener_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,8 @@ Network::SocketSharedPtr ListenSocketFactoryImpl::getListenSocket() {
ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::string& version_info,
ListenerManagerImpl& parent, const std::string& name, bool added_via_api,
bool workers_started, uint64_t hash,
ProtobufMessage::ValidationVisitor& validation_visitor)
ProtobufMessage::ValidationVisitor& validation_visitor,
uint32_t concurrency)
: parent_(parent), address_(Network::Address::resolveProtoAddress(config.address())),
filter_chain_manager_(address_), global_scope_(parent_.server_.stats().createScope("")),
listener_scope_(
Expand Down Expand Up @@ -154,8 +155,11 @@ ListenerImpl::ListenerImpl(const envoy::api::v2::Listener& config, const std::st
if (PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, freebind, false)) {
addListenSocketOptions(Network::SocketOptionFactory::buildIpFreebindOptions());
}
if ((socket_type == Network::Address::SocketType::Datagram) || config.reuse_port()) {
if (config.reuse_port()) {
addListenSocketOptions(Network::SocketOptionFactory::buildReusePortOptions());
} else if (socket_type == Network::Address::SocketType::Datagram && concurrency > 1) {
ENVOY_LOG(warn, "Listening on UDP without SO_REUSEPORT socket option may result to unstable "
"packet proxying.");

@mattklein123 mattklein123 Dec 29, 2019

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.

WDYT about the following text:

Listening on UDP without SO_REUSEPORT socket option may result in unstable packet proxying. Consider configuring the reuse_port listener option.

@danzh2010 danzh2010 Dec 30, 2019

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.

done

}
if (!config.socket_options().empty()) {
addListenSocketOptions(
Expand Down
2 changes: 1 addition & 1 deletion source/server/listener_impl.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ class ListenerImpl : public Network::ListenerConfig,
ListenerImpl(const envoy::api::v2::Listener& config, const std::string& version_info,
ListenerManagerImpl& parent, const std::string& name, bool added_via_api,
bool workers_started, uint64_t hash,
ProtobufMessage::ValidationVisitor& validation_visitor);
ProtobufMessage::ValidationVisitor& validation_visitor, uint32_t concurrency);
~ListenerImpl() override;

/**
Expand Down
9 changes: 5 additions & 4 deletions source/server/listener_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,11 @@ bool ListenerManagerImpl::addOrUpdateListenerInternal(const envoy::api::v2::List
return false;
}

ListenerImplPtr new_listener(new ListenerImpl(
config, version_info, *this, name, added_via_api, workers_started_, hash,
added_via_api ? server_.messageValidationContext().dynamicValidationVisitor()
: server_.messageValidationContext().staticValidationVisitor()));
ListenerImplPtr new_listener(
new ListenerImpl(config, version_info, *this, name, added_via_api, workers_started_, hash,
added_via_api ? server_.messageValidationContext().dynamicValidationVisitor()
: server_.messageValidationContext().staticValidationVisitor(),
server_.options().concurrency()));
ListenerImpl& new_listener_ref = *new_listener;

// We mandate that a listener with the same name must have the same configured address. This
Expand Down
3 changes: 2 additions & 1 deletion test/server/listener_manager_impl_quic_only_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ TEST_F(ListenerManagerImplQuicOnlyTest, QuicListenerFactoryAndSslContext) {
verify_subject_alt_name:
- localhost
- 127.0.0.1
reuse_port: true
udp_listener_config:
udp_listener_name: "quiche_quic_listener"
)EOF",
Expand All @@ -48,7 +49,7 @@ TEST_F(ListenerManagerImplQuicOnlyTest, QuicListenerFactoryAndSslContext) {
EXPECT_CALL(server_.random_, uuid());
expectCreateListenSocket(envoy::api::v2::core::SocketOption::STATE_PREBIND,
#ifdef SO_RXQ_OVFL
/* expected_num_options */ 3, // SO_REUSEPORT is on forcibly for UDP
/* expected_num_options */ 3, // SO_REUSEPORT is on as configured
#else
/* expected_num_options */ 2,
#endif
Expand Down
13 changes: 4 additions & 9 deletions test/server/listener_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3324,7 +3324,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, ReusePortListenerEnabledForTcp) {
/* expected_creation_params */ {true, false});
}

TEST_F(ListenerManagerImplWithRealFiltersTest, ReusePortListenerEnabledForUdp) {
TEST_F(ListenerManagerImplWithRealFiltersTest, ReusePortListenerDisabled) {

auto listener = createIPv4Listener("UdpListener");
listener.mutable_address()->mutable_socket_address()->set_protocol(
Expand All @@ -3339,9 +3339,9 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, ReusePortListenerEnabledForUdp) {
// IpPacketInfo and RxQueueOverFlow are always set if supported
expectCreateListenSocket(envoy::api::v2::core::SocketOption::STATE_PREBIND,
#ifdef SO_RXQ_OVFL
/* expected_num_options */ 3,
#else
/* expected_num_options */ 2,
#else
/* expected_num_options */ 1,
#endif
/* expected_creation_params */ {true, false});

Expand All @@ -3358,12 +3358,7 @@ TEST_F(ListenerManagerImplWithRealFiltersTest, ReusePortListenerEnabledForUdp) {
/* expected_num_calls */ 1);
#endif

expectSetsockopt(os_sys_calls_,
/* expected_sockopt_level */ SOL_SOCKET,
/* expected_sockopt_name */ SO_REUSEPORT,
/* expected_value */ 1,
/* expected_num_calls */ 1);

server_.options_.concurrency_ = 2;
manager_->addOrUpdateListener(listener, "", true);
EXPECT_EQ(1U, manager_->listeners().size());
}
Expand Down