listen_socket: Support multiple options and allow socket options to be set before bind(). by jrajahalme · Pull Request #2734 · envoyproxy/envoy

jrajahalme · 2018-03-06T02:01:33Z

Support multiple options by collecting options to a list and then
applying all of them in order on the socket.

So far the listen socket options have been set in the internal
setSocket() function after the socket has already been bound. This
has the benefit of (re)setting the options whenever a listener
configuration is changed, which may involve a change in listener
filter configurarion. Some socket options, however, need to be set
before the bind() call for them to have the desired effect.

This commit adds an enum SocketState parameter to the setOption()
callback, telling the options implementation whether the options are being
set before or after the bind() call. The setOption() callback is called
on the listen socket once with state=PreBind when the socket is first
created right before the bind() call. Then, after the bind() call and
when whenever the listener socket is reused for a new or modified listener,
the setOption() callback is called as before, but now with state=PostBind.

Client connections will have the setOptions() called before the bind
(like before) and with state=PreBind. Client sockets are not recycled
like the listen sockets, so they will not have the setOptions() called
again later.

Finally, this PR adds support for setting the IP_TRANSPARENT socket
option defined in the Envoy LDS API. The same ListenerSocketOption
class can easily be extended to support other socket options as well.

Signed-off-by: Jarno Rajahalme jarno@covalent.io
Risk Level: Low

ggreenway

Overall I think the code looks good.

ggreenway · 2018-03-06T16:41:12Z

source/server/listener_manager_impl.cc

What happens if the listener configuration has changed, but in such a way that it wants to modify a listener option that must be set before binding? Should an error be raised in that case? Close/unbind the listener temporarily so the option can be changed?

I personally think that pre-bind options shouldn't be allowed to change on a listener, because that would require the socket to be closed and reopened. But I don't see a way to guarantee options aren't changed with this design.

Closing risks dropping incoming connections during a listener reconfigure. Maybe we could use SO_REUSEPORT option to allow multiple listening sockets on the same address, and create a new socket, apply pre-bind options, bind, listen, and then close the old socket?

@jrajahalme I'm not clear whether there would be problems when those multiple sockets have different options, e.g. one socket has SOL_IP,IP_TRANSPARENT and another not?

Anyway, my vote would be to go with your design as a purely internal implementation, but not expose the bound flag directly into the LDS API. And any feature that sets pre-bind options should expose specific fields in LDS, and check that those fields are not modified when updating a listener. We already do that for bind addresses, and I proposed to do that for the transparent field in #2719.
What do you think?

I would prefer to avoid the complexity around SO_REUSEPORT if possible. @rlenglet what are you proposing? That we drop this change and require any pre-bind options to have explicit settings?

I don't have a strong opinion about the internal implementation. It could be explicit settings like I did in #2719, or it could be this implementation.

However, in any case, I think we shouldn't expose the bound flag as-is in LDS, since it would make it harder to control whether an option is modified when a listener is modified.
We need to be able to compare the old and new configs of listeners to prevent changing listen socket options, like in https://github.com/envoyproxy/envoy/pull/2719/files#diff-cb0af0ff1b04bcf9f82748317d3b98e3R345.

@rlenglet what do you mean by:

However, in any case, I think we shouldn't expose the bound flag as-is in LDS, since it would make it harder to control whether an option is modified when a listener is modified.

Do you mean in the API for filters setOptions call don't pass a bound flag? If we don't do that I'm not sure how this change could work as-is? Sorry I'm confused.

So far the listen socket options have been set in the internal `setSocket()` function after the socket has already been bound. This has the benefit of (re)setting the options whenever a listener configuration is changed, which may involve a change in listener filter configurarion. Some socket options, however, need to be set before the bind() call for them to have the desired effect. This patch adds a boolean parameter to the setOptions() callback, telling the options implementation whether the socket has already been bound or not. The setOptions() callback is called on the listen socket once with `bound=false` when the socket is first created right before the bind call. Then, when whenever the listen socket is "set" the setOptions() callback is called as before, but now typicallyl with `bound=true`. This latter call will be done again whenever the same socket is re-used for a new or modified listener. For non-bound listen sockets, the setOptions() is always called with `bound=false`. Client connections will have the setOptions() called before the bind (like before) and with `bound=false`. Client sockets are not recycled like the listen sockets, so they will not have the setOptions() called again after the bind() call. It should be noted that setting any options via the setOptions() callback functionality discussed here still requires the options object be set by a listener filter. While it would be possible to create a listener filter for the sole purpose of setting socket options, no such filter exist yet. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Keep a list of options so that multiple options can be set on the same sockets, maybe by different listener filters. TODO: Test with multiple options & verify hashKey() is properly handled as well. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-03-14T02:35:58Z

Sorry for the rebase, had to change the approach a bit.

jrajahalme · 2018-03-14T02:36:03Z

@ggreenway Added support for multiple options.

jrajahalme · 2018-03-14T02:44:25Z

Updated the title and description to match the new approach.

rlenglet · 2018-03-14T03:52:21Z

include/envoy/server/filter_config.h

options -> option

rlenglet · 2018-03-14T04:07:08Z

@jrajahalme I don't think this design requires adding a listener filter to set an option. ListenerManagerImpl::addOrUpdateListener can always call the listener's setListenSocketOption() to set additional options based on the listener's config flags, right?

ggreenway

I like the direction this is going; I think this is a good step towards more flexibility in setting whatever options we end up needing to set.

ggreenway · 2018-03-14T22:08:54Z

include/envoy/network/listen_socket.h

I think there are 3 points at which we may/will want to set socket options: before bind, after bind but before listen, after listen.

I think this should be an enum instead of bool to handle this better. For this PR, you can just add the 2 enum values that are implemented so far, but I think we'll want to be able to add more as we go.

Do you have an example of options that only make sense after listen? Also, do you think some options only make sense after bind() is done?

The current sets options only once per configuration event, and let the options know if a bind() is going to happen later or not. The options visitors will not be called again right after bind(), but will be called again if the socket is reused on a new or reconfigured listener. In that case bind() never follows. Currently options will never be set between bind() and listen().

Also, some listeners do not bind. For those before/after bind() makes no sense, but pre_bind=false simply means that there is not going to be a bind call later (so no point setting options that only make a difference for bind()).

Given this I don't find it easy to come up with the definition of an enum that would be equivalent to the bool I have now. Maybe:

enum class OptionContext { PreBind, None };

Example of post-listen(): #2793

Are you sure that TCP_FASTOPEN can not be set before bind() or before listen(), i.e., at any time?

I'm pretty sure it's required on MacOS to be set after listen(); I think it can be set anytime on linux. @bmetzdorf has the details on this option.

Seems you are right: https://github.com/opensource-apple/xnu/blob/master/bsd/netinet/tcp_usrreq.c#L2272

Yep, correct.

I'll implement PreBind, PostBind for now, let you worry about PostListen...

ggreenway · 2018-03-14T22:09:59Z

include/envoy/network/listen_socket.h

std::vector is better than std::list for this, I think.

Will change.

ggreenway · 2018-03-14T22:12:13Z

source/common/network/connection_impl.cc

whitespace nit: for (const auto& option : *options) {

ggreenway · 2018-03-14T22:20:54Z

source/common/upstream/cluster_manager_impl.cc

What is the significance of >2? Please add comments

I'll have to fix this, thanks for not understanding it :-)

ggreenway · 2018-03-14T22:24:18Z

source/common/upstream/cluster_manager_impl.h

Why switch from unordered to ordered? Is it just because you don't have a hash function?

Right. std::map supports a vector as a key out of the box, for unordered map I'd need to provide the hash function, I guess.

Looked into possible hash support, boost::hash would be nice, as there is no std hash combiner. Or then just keep this as unordered map for now.

Yeah, I've run into this before too. I'm fine leaving it as std::map, although other reviewers may feel more strongly about it.

No strong opinion that this has to be unordered, unless it affect performance. std::hash does support string as key so if we need a hash function we can just reuse some from that?

Yeah, no strong preference. IMHO, the STL has a lot of inconsistency around how hashing and comparison works with containers and inbuilt types, let's fix it only when we need to for perf reasons and prefer simplicity otherwise.

Insert a Listener Socket Option if "transparent" option is requested. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Requested-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Reported-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Use an enum instead of a bool for socket state, and call the visitors in both states initially. Upon closer examination, changing the IP_TRANSPARENT option can have effects also after bind(), so we need to change it also after bind, and potentially set it to zero when set as well. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-03-15T22:20:43Z

@ggreenway Addressed all your comments :-)

ggreenway

A few more small nits, but I think the design of this is looking really good.

ggreenway · 2018-03-15T22:27:27Z

source/server/listener_manager_impl.cc

+    // If we are unable to set the option we still return success for the default value (0),
+    // as we never changed the option to begin with. This also prevents failures in all current
+    // test cases that do not set the transparent option.
+    if (rc == 0 || transparent == 0) {


Is there a case where rc != 0, transparent == 0, and transparent_ != 0? In other words, will setsockopt ever modify transparent? If not, I think it's clearer if you check against transparent_ here, so it's clear that the result doesn't depend on setsockopt possibly modifying it.

You are right, will compare against transparent_

ggreenway · 2018-03-15T22:28:57Z

source/server/listener_manager_impl.cc

-    return true;
+#endif
+    throw EnvoyException("ListenSocketOption: Error setting IP_TRANSPARENT socket option");
+    return false;


This line is unreachable. But shouldn't this just return false instead of throwing? I thought that was the convention for this function.

The caller of this callback will also throw() in case of false return. I added a more specific throw here to be able to test that this implementation is called in the test case.

I was concerned that compiler would complain about not returning, but I was wrong, leaving the return statement out works as well, so I removed it.

But isn't the API contract for this interface that it will return false on failure? If it is allowed to throw, I think the interface should return void. Or at a minimum, the documentation should specify in what cases it will throw vs when it could return false.

rlenglet · 2018-03-15T22:39:21Z

source/server/listener_manager_impl.cc

+    int transparent = transparent_;
+
+#ifdef SOL_IP
+    int rc = setsockopt(socket.fd(), SOL_IP, IP_TRANSPARENT, &transparent, sizeof(transparent));


You need to set IPV6_TRANSPARENT if the socket is IPv6, right?
Beware that IPV6_TRANSPARENT is not defined everywhere, so that will need to be guarded by an #ifdef.

Was not aware of this. Will do.

Also, it may make testing easier if you add setsockopt() to Envoy::Api::OsSysCalls so you can mock setsockopt().

Search around for TestThreadsafeSingletonInjector<Api::OsSysCallsImpl> for an example.

Will address this too, went unnoticed so far, sorry.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-03-15T23:50:25Z

Nits & other comments addressed :-)

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

"add" fits better now that we support multiple options. Suggested-by: Lizan Zhou <zlizan@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-03-20T22:59:44Z

I'll push a few commits addressing @lizan's review comments soon.

Shared vector of shared pointers kept the ownership of socket option visitors a bit unclear. Transfer the ownership of option visitors to Socket, but allow the vector of all sockets be shared. Requested-by: Lizan Zhou <zlizan@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

lizan

LGTM, modulo one nit in tests.

lizan · 2018-03-20T23:29:47Z

test/server/listener_manager_impl_test.cc

                                 Configuration::ListenerFactoryContext& context) override {
-      EXPECT_CALL(*option_, setOption(_, Network::Socket::SocketState::PreBind))
+      auto option = std::make_unique<Network::MockSocketOption>();
+      EXPECT_CALL(*(option.get()), setOption(_, Network::Socket::SocketState::PreBind))


EXPECT_CALL(*option, setOption should work, no?

You are right, an explicit get() is not needed here. Will simplify this in a commit ASAP.

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Add setsockopt() to Envoy::Api::OsSysCalls and provide mock implementation supporting boolean options defaulting to 'false'. Change the listener manager implementation test 'TransparentListener' to use the new mocks. Suggested-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

…ot available. Fix unused variable when IP_TRANSPARENT option is not available. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme · 2018-03-21T19:35:35Z

Problems with the CI:

Got error while creating host: instance not ready yet: pending
We had an unexpected error preparing a VM for this build, potentially due to our infrastructure or cloud provider.  Please retry the build in a few minutes.

jrajahalme · 2018-03-21T20:18:55Z

@ggreenway Have no idea how I "dismissed" your review, not sure if your approval still counts?

ggreenway · 2018-03-21T21:28:04Z

@jrajahalme approval is dismissed anytime a new commit is pushed. I think the theory behind this is so that you can't try to push a commit right before someone pushes the merge button (trying to sneak in an unapproved commit). Don't worry about it too much. We're still waiting on review from a senior maintainer (@alyssawilk in this case).

htuch

Thanks @jrajahalme. Mostly looks great, just a few small changes requested. In particular, would like to see the IP transparent stuff in a separate PR, it's easier to review and lowers risk of any given PR to separate concerns.

htuch · 2018-03-23T14:31:07Z

include/envoy/network/listen_socket.h

-     *         to 32 bits to allow these bits to be efficiently combined into a larger hash key
-     *         used in connection pool lookups.
+     * @param vector of bits that can be used to separate connections based on the options. Should
+     *        return zero if connections with different options can be pooled together. This is


There is no return now.

Comment updated, thanks for noticing this!

htuch · 2018-03-23T14:31:20Z

include/envoy/network/listen_socket.h

-     *         zero if connections with different options can be pooled together. This is limited
-     *         to 32 bits to allow these bits to be efficiently combined into a larger hash key
-     *         used in connection pool lookups.
+     * @param vector of bits that can be used to separate connections based on the options. Should


Nit: this is now a singular option.

htuch · 2018-03-23T14:34:33Z

source/common/upstream/cluster_manager_impl.h

Yeah, no strong preference. IMHO, the STL has a lot of inconsistency around how hashing and comparison works with containers and inbuilt types, let's fix it only when we need to for perf reasons and prefer simplicity otherwise.

htuch · 2018-03-23T14:37:34Z

source/server/listener_manager_impl.cc

+
+  // Network::Socket::Option
+  bool setOption(Network::Socket& socket, Network::Socket::SocketState state) const override {
+    // IP_TRANSPARENT has an effect on bind() (allowing bind() to non-local addresses), but


Can you split this into a separate PR? I think it's orthogonal.

Removed from this PR.

Requested-by: Harvey Tuch <htuch@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

htuch

Thanks, this is really nice.

This follows up from envoyproxy#558 which made IP_FREEBIND a BoolValue for LDS but not for upstream. I think it makes sense to have it in both places given the new socket options setup introduce in envoyproxy/envoy#2734. Some bonus docs fixups thrown in. Signed-off-by: Harvey Tuch <htuch@google.com>

This follows up from #558 which made IP_FREEBIND a BoolValue for LDS but not for upstream. I think it makes sense to have it in both places given the new socket options setup introduce in envoyproxy/envoy#2734. Some bonus docs fixups thrown in. Signed-off-by: Harvey Tuch <htuch@google.com>

This patch introduces support for setting IP_FREEBIND on both listener sockets and upstream connection sockets prior to binding. This enables the use of IP addresses that are not currently bound to the NIC for listening and initiating connections from. This is useful in environments with virtualized networking. There's also some related work on SocketOption that continues from envoyproxy#2734, which was needed to enable this to work cleanly. Risk Level: Low (no change unless enabled). Testing: Unit tests for ListenerManager, ClusterManager and SocketOptionImpl. Manual end-to-end validation with steps described in configs/freebind/README.md. API Changes: envoyproxy/data-plane-api#536 Fixes envoyproxy#528. Signed-off-by: Harvey Tuch <htuch@google.com>

…2922) This patch introduces support for setting IP_FREEBIND on both listener sockets and upstream connection sockets prior to binding. This enables the use of IP addresses that are not currently bound to the NIC for listening and initiating connections from. This is useful in environments with virtualized networking. There's also some related work on SocketOption that continues from #2734, which was needed to enable this to work cleanly. Risk Level: Low (no change unless enabled). Testing: Unit tests for ListenerManager, ClusterManager and SocketOptionImpl. Manual end-to-end validation with steps described in configs/freebind/README.md. API Changes: envoyproxy/data-plane-api#536 Fixes #528. Signed-off-by: Harvey Tuch <htuch@google.com>

Signed-off-by: Kuat Yessenov <kuat@google.com>

This follows up from #558 which made IP_FREEBIND a BoolValue for LDS but not for upstream. I think it makes sense to have it in both places given the new socket options setup introduce in envoyproxy/envoy#2734. Some bonus docs fixups thrown in. Signed-off-by: Harvey Tuch <htuch@google.com>

htuch assigned lizan, ggreenway and alyssawilk Mar 6, 2018

ggreenway reviewed Mar 6, 2018

View reviewed changes

This was referenced Mar 8, 2018

Filter HTTP in Istio sidecar proxy cilium/cilium#3065

Closed

connection: Set the SOL_IP,IP_TRANSPARENT option on listen sockets #2719

Merged

bmetzdorf mentioned this pull request Mar 13, 2018

connection: Add TCP_FASTOPEN listener option #2793

Merged

jrajahalme added 2 commits March 13, 2018 14:25

jrajahalme force-pushed the socket-options-before-and-after-bind branch from eb71c3b to 08705d1 Compare March 14, 2018 02:34

jrajahalme changed the title ~~listen_socket: Allow socket options to be set before bind().~~ listen_socket: Support multiple options and allow socket options to be set before bind(). Mar 14, 2018

rlenglet reviewed Mar 14, 2018

View reviewed changes

ggreenway requested changes Mar 14, 2018

View reviewed changes

listener: Add support for LDS API "transparent" option.

1b58b6f

Insert a Listener Socket Option if "transparent" option is requested. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme force-pushed the socket-options-before-and-after-bind branch from 08705d1 to 1b58b6f Compare March 14, 2018 22:41

jrajahalme added 2 commits March 14, 2018 16:02

listen_socket: Use a vector instead of a list for options.

a8fed75

Requested-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

cluster_manager: Fix socket options passing.

824d8c7

Reported-by: Greg Greenway <ggreenway@apple.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

dnoe mentioned this pull request Mar 15, 2018

Support IP_FREEBIND #528

Closed

jrajahalme added 2 commits March 15, 2018 12:16

cluster_manager: Fix format.

fa23036

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

ggreenway requested changes Mar 15, 2018

View reviewed changes

rlenglet reviewed Mar 15, 2018

View reviewed changes

listener_manager: Fix nits.

7524e53

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

listener_manager: Fix errno value.

d569fec

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

listen_socket: Rename Socket::setOption() as Socket::addOption().

13887b8

"add" fits better now that we support multiple options. Suggested-by: Lizan Zhou <zlizan@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

lizan approved these changes Mar 20, 2018

View reviewed changes

jrajahalme added 2 commits March 20, 2018 16:40

test: Don't use get() on unique_ptr's when not needed.

b154e73

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

ggreenway previously approved these changes Mar 21, 2018

View reviewed changes

listener_manager_impl_test: Fix build when IP_TRANSPARENT option is n…

da28038

…ot available. Fix unused variable when IP_TRANSPARENT option is not available. Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme dismissed ggreenway’s stale review via e55ed9e March 21, 2018 18:32

jrajahalme force-pushed the socket-options-before-and-after-bind branch from e55ed9e to da28038 Compare March 21, 2018 20:16

ggreenway previously approved these changes Mar 21, 2018

View reviewed changes

htuch suggested changes Mar 23, 2018

View reviewed changes

listener_manager: Remove 'transparent' handling from this PR.

a46328b

Requested-by: Harvey Tuch <htuch@google.com> Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

jrajahalme dismissed ggreenway’s stale review via a46328b March 23, 2018 18:29

listen_socket: Fix comment.

d48f9b6

Signed-off-by: Jarno Rajahalme <jarno@covalent.io>

htuch approved these changes Mar 25, 2018

View reviewed changes

htuch merged commit 5123d31 into envoyproxy:master Mar 25, 2018

htuch mentioned this pull request Mar 27, 2018

BindConfig: make upstream and downstream bind config consistent. envoyproxy/data-plane-api#583

Merged

htuch mentioned this pull request Mar 28, 2018

socket: IP_FREEBIND support for listeners and upstream connections. #2922

Merged

Shikugawa pushed a commit to Shikugawa/envoy that referenced this pull request Mar 28, 2020

allow unknown fields in configs (envoyproxy#2734)

71b7a6c

Signed-off-by: Kuat Yessenov <kuat@google.com>

Conversation

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

Uh oh!

Uh oh!

ggreenway 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!

jrajahalme commented Mar 14, 2018

Uh oh!

jrajahalme commented Mar 14, 2018

Uh oh!

jrajahalme commented Mar 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ggreenway 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!

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

jrajahalme commented Mar 6, 2018 •

edited

Loading

rlenglet commented Mar 14, 2018 •

edited

Loading