network: socket interface support for addresses #12189

florincoras · 2020-07-20T22:10:27Z

Signed-off-by: Florin Coras fcoras@cisco.com

Risk Level: Low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a

Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras · 2020-07-20T22:16:03Z

cc @yanavlasov @htuch Is this in combination with the support for configurable SocketInterface enough to solve #11618?

htuch

Looks right to me, will defer to Yan for review. Thanks.

htuch · 2020-07-21T22:05:34Z

source/common/network/address_impl.h

@@ -54,23 +60,23 @@ class Ipv4Instance : public InstanceBase {
  /**
   * Construct from an existing unix IPv4 socket address (IP v4 address and port).
   */
-  explicit Ipv4Instance(const sockaddr_in* address);
+  explicit Ipv4Instance(const sockaddr_in* address, const std::string& sock_interface = "");


Nit: absl::string_view for all const std::string&s.

yanavlasov · 2020-07-23T00:07:51Z

source/common/network/socket_impl.cc

+    if (sock_interface != nullptr) {
+      return sock_interface->socket(type, addr);
+    }
+    return nullptr;


This will cause a SIGSEGV sometime later, right? Maybe the right thing to do is to RELEASE_ASSERT here that the factory for creating IO handles is missing.

Fair point! Made it a RELEASE_ASSERT.

yanavlasov · 2020-07-23T00:09:52Z

Also would you be able to make unit test exercising this, please?

Signed-off-by: Florin Coras <fcoras@cisco.com>

- absl::string_view instead of const std::string& - RELEASE_ASSERT if address socket_interface not found - added integration test for address with socket_interface set Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras · 2020-07-23T20:48:07Z

Also would you be able to make unit test exercising this, please?

Done! Added an integration test which opens a connection with an address that explicitly asks for the default socket interface

florincoras · 2020-07-27T20:23:42Z

@yanavlasov, I suspect the arm failure is just a flake. After you get a chance to review the PR, I'll merge master and hopefully it'll go away.

yanavlasov · 2020-07-28T14:34:03Z

@florincoras thanks it looks good. Would it be possible to also have a test that tests complete plumbing with a custom SocketInterface, bootstrap extension, etc. This will also server as a documentation for anyone who needs to create their SocketInterface.
Super excited about this one. I could use it in the integration tests where the test needs to simulate TCP back-pressure from the peer.

florincoras · 2020-07-28T19:14:42Z

@florincoras thanks it looks good. Would it be possible to also have a test that tests complete plumbing with a custom SocketInterface, bootstrap extension, etc. This will also server as a documentation for anyone who needs to create their SocketInterface.

I suspect my description above was misleading. The default SocketInterface implemented here actually goes through the whole process of registering a factory which can be configured through a bootstrap extension (albeit no actual config params are supported currently) and subsequently can be looked up by name via this. So, the integration test actually exercises both bootstrap extensions and the SocketInterface lookup by name.

In short, to implement a new, configurable SocketInterface, apart from the actual socket apis, one only needs implementations for these.

Also, this static global init is why we don't always need to configure a default interface in the startup config file.

Super excited about this one. I could use it in the integration tests where the test needs to simulate TCP back-pressure from the peer.

Cool idea! :-D

mattklein123

LGTM with small comment, thanks. Please also merge main as there have been many CI fixes.

/wait

mattklein123 · 2020-07-30T18:15:27Z

source/common/network/socket_interface.h

+                                          const Address::InstanceConstSharedPtr addr) {
+  auto sock_interface_name = addr->socketInterface();
+  if (!sock_interface_name.empty()) {
+    auto sock_interface = const_cast<SocketInterface*>(socketInterface(sock_interface_name));


nit: this const_cast seems a bit off. Any reason to not just fix the socket method itself to be const?

I thought about that too but socket() could end up changing the SocketInterface object. The alternative would be to remove const from SocketInterface and trust that callers won't do anything irresponsible.

Why would it change the interface object? Does that happen in practice right now? Can we cross that bridge later if we come to it? If it really is a mutable thing I would just remove const.

I was thinking that interface objects could be tracking the sockets, e.g., they could be building some form of socket descriptor map. We're not doing that now, as we're offloading everything to the kernel, but I did hit this particular issue with a custom socket interface implementation. But okay, let's cross that bridge later :-)

- make socket apis const for now Signed-off-by: Florin Coras <fcoras@cisco.com>

Signed-off-by: Florin Coras <fcoras@cisco.com>

mattklein123

Thanks!

antoniovicente · 2020-07-31T16:34:34Z

source/common/network/socket_interface.h

+  auto sock_interface_name = addr->socketInterface();
+  if (!sock_interface_name.empty()) {
+    auto sock_interface = socketInterface(sock_interface_name);
+    RELEASE_ASSERT(sock_interface != nullptr,


Regarding this RELEASE_ASSERT, what guarantees that the interface names returned by the address are valid?

By default, the names are null. If anybody sets a name on the address, e.g., resolvers, the expectation is that those names are valid. The alternative would be to silently accept errors and use the default SocketInterface implementation but that would just hide misconfiguration. Still, we can change it if need be!

We need config validation so this doesn't turn into a runtime failure at some indeterminate time after a configuration that refers to an unknown socket interface provider is specified.

Crashing the proxy in a situation like this doesn't seem like the right thing. Terminating the connection with some critical errors may be an option, but really - we need some protections during config load to provide some better guarantees that this won't happen.

Currently, there's no way to configure a socket interface name on an address via config. The addresses have resolvers and resolvers spawn Address::Instances, with or without a socket interface name. So then, if the resolvers are misconfigured and generate broken addresses, what should we do?

But, let's assume we'll allow configuration of addresses with socket interface names and don't validate those in resolvers or wherever we spawn Address::Instances. Then, we could return nullptr IoHandlePtrs or an IoSocketHandleImpl with a -1 fd. Consequently, any place that constructs a Socket must afterwards check if the internal IoHandle was correctly initialized. A quick code check shows that ConnectionImpl RELEASE_ASSERTs that the socket it was given is valid in the constructor and similarly NetworkListenSocket RELEASE_ASSERTs that the IoHandle's fd is valid.

Do we have any other alternatives?

Possible idea: could we have the Address::Instance provide a const SocketInterface* instead of a name?

How are Address::Instance typically constructed? Via a factory that would take the socket interface name as part of its config? That factory resolving the SocketInterface as part of its config validation would allow us to catch missing factories at config validation time. It will probably even be less memory hungry since you wouldn't need to keep a copy of the socket interface name string in every Address::Instance.

I think this would allow address factories more easily verify consistency early on. A null SocketInterface returned by the address would result in this code going down the normal path.

That's an interesting idea! It's definitely doable, assuming all the places that construct Address::Instances have access to the socket_interface.h and therefore ability to lookup the SocketInterface implementations. But I'll defer this to @yanavlasov as I'm sure he has more experience with this type of problems given #11618

Signed-off-by: Florin Coras <fcoras@cisco.com>

Signed-off-by: Florin Coras <fcoras@cisco.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

network: socket interface support for addresses

dbcdcae

Signed-off-by: Florin Coras <fcoras@cisco.com>

zuercher assigned yanavlasov Jul 21, 2020

htuch reviewed Jul 21, 2020

View reviewed changes

yanavlasov requested changes Jul 23, 2020

View reviewed changes

florincoras added 2 commits July 23, 2020 09:02

Merge branch 'master' into addr_sock

18e2819

Signed-off-by: Florin Coras <fcoras@cisco.com>

addressing comments

ad9ff29

- absl::string_view instead of const std::string& - RELEASE_ASSERT if address socket_interface not found - added integration test for address with socket_interface set Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras requested a review from mattklein123 as a code owner July 23, 2020 20:43

yanavlasov previously approved these changes Jul 30, 2020

View reviewed changes

mattklein123 self-assigned this Jul 30, 2020

mattklein123 requested changes Jul 30, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 30, 2020

address comments

2eb3396

- make socket apis const for now Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras dismissed yanavlasov’s stale review via f094406 July 30, 2020 20:28

Merge branch 'master' into addr_sock

f094406

Signed-off-by: Florin Coras <fcoras@cisco.com>

repokitteh-read-only bot removed the waiting label Jul 30, 2020

mattklein123 approved these changes Jul 30, 2020

View reviewed changes

mattklein123 merged commit f4adb59 into envoyproxy:master Jul 30, 2020

antoniovicente reviewed Jul 31, 2020

View reviewed changes

florincoras mentioned this pull request Jul 31, 2020

Provide ability to override socket API #11618

Open

chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020

network: socket interface support for addresses (envoyproxy#12189)

471e3d4

Signed-off-by: Florin Coras <fcoras@cisco.com>

chaoqin-li1123 pushed a commit to chaoqin-li1123/envoy that referenced this pull request Aug 7, 2020

network: socket interface support for addresses (envoyproxy#12189)

de53b8f

Signed-off-by: Florin Coras <fcoras@cisco.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

florincoras deleted the addr_sock branch August 7, 2020 19:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

network: socket interface support for addresses #12189

network: socket interface support for addresses #12189

florincoras commented Jul 20, 2020

florincoras commented Jul 20, 2020

htuch left a comment

htuch Jul 21, 2020

florincoras Jul 23, 2020

yanavlasov Jul 23, 2020

florincoras Jul 23, 2020

yanavlasov commented Jul 23, 2020

florincoras commented Jul 23, 2020

florincoras commented Jul 27, 2020

yanavlasov commented Jul 28, 2020

florincoras commented Jul 28, 2020

mattklein123 left a comment

mattklein123 Jul 30, 2020

florincoras Jul 30, 2020

mattklein123 Jul 30, 2020

florincoras Jul 30, 2020

mattklein123 left a comment

antoniovicente Jul 31, 2020

florincoras Jul 31, 2020

antoniovicente Jul 31, 2020

florincoras Jul 31, 2020 •

edited

Loading

antoniovicente Aug 1, 2020

florincoras Aug 1, 2020

network: socket interface support for addresses #12189

network: socket interface support for addresses #12189

Conversation

florincoras commented Jul 20, 2020

florincoras commented Jul 20, 2020

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yanavlasov commented Jul 23, 2020

florincoras commented Jul 23, 2020

florincoras commented Jul 27, 2020

yanavlasov commented Jul 28, 2020

florincoras commented Jul 28, 2020

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florincoras Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

florincoras Jul 31, 2020 •

edited

Loading