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 1 commit
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
6 changes: 4 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,7 +155,8 @@ 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 ((socket_type == Network::Address::SocketType::Datagram && concurrency > 1) ||
config.reuse_port()) {

@mattklein123 mattklein123 Dec 26, 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.

Won't this break restart cases in which the user still expects to be able to rebind? I think we probably need to make the option actually work for UDP also such that it can be disabled in config for the the tests?

@danzh2010 danzh2010 Dec 26, 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.

How does restart case work if port is 0 in config? I would suspect in the old way, we can't guarantee rebinding to the same port either. Does restarting actually depends on SO_REUSEPORT?
If that's the case, for UDP we can make it work by prioritizing reuse_port in config. If that field is set to false, RELEASE_ASSERT failure if concurrency > 1, if concurrency == 1 not set SO_REUSEPORT. In this way, a config with concurrency > 1 and reuse_port not specified or set to false will crash at loading config.

@mattklein123 mattklein123 Dec 26, 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.

I'm not worried about the port 0 case, just the case in which someone wants to start a new proxy with SO_REUSEPORT and then shut down the old proxy. I'm pretty sure there are people doing this, and I don't think we should break that case for UDP and concurrency 1?

@danzh2010 danzh2010 Dec 26, 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.

How did user achieve that? SO_REUSEPORT only allows port re-bind within same process.

@mattklein123 mattklein123 Dec 26, 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.

I could be wrong, but I'm pretty sure that's not true. I think it has to be part of the same process group, same owner, or something like that.

@danzh2010 danzh2010 Dec 27, 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.

I changed the logic to only set SO_REUSEPORT if reuse_port == true in config.

addListenSocketOptions(Network::SocketOptionFactory::buildReusePortOptions());
}
if (!config.socket_options().empty()) {
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