network: add socket interface by florincoras · Pull Request #11278 · envoyproxy/envoy

florincoras · 2020-05-20T21:37:25Z

Move socket calls that involve raw fds from address to socket.

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

Move socket calls that involve raw fds from address to socket. Signed-off-by: Florin Coras <fcoras@cisco.com>

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

mattklein123

Epic thanks, I can see the tech debt evaporating in front of me. :)

A few high level comments. Thank you!

/wait

mattklein123 · 2020-05-20T22:46:15Z

      return {getVersionFromAddress(socket.localAddress())};
    } else {
-      return {getVersionFromAddress(Address::addressFromFd(socket.ioHandle().fd()))};
+      return {getVersionFromAddress(SocketInterface::addressFromFd(socket.ioHandle().fd()))};


Do you want to fix this fd indirection in this PR or a follow up?

I was planning on doing it in a subsequent PR (after the last in this series). Obviously, if that's fine with you :-)

Yes that's fine.

mattklein123 · 2020-05-20T22:48:09Z

+  /**
+   * @return true if address is Ipv6 and Ipv4 compatibility is disabled, false otherwise
+   */
+  virtual bool v6only() const PURE;


Can't this just be on the IPv6 interface? Why do we need it on the top level IP interface?

At a high level, I didn't want to change the semantics of one of the existing socket functions which looks at "v6only()" property of the address to setsockopt(,IPV6_V6ONLY,). Not sure if this still makes sense after we finish moving everything to socket, but that's probably for a later time discussion.

So, with respect to the comment above, I did things this way because I didn't know how else to extract the field from the private IpHelper struct. Note though that I am somewhat C++ challenged :-(

I guess that one alternative would be to move the field to the Ipv6Helper struct and that should be then retrievable via a new function, e.g., address.ip().ipv6().v6only(). I didn't do this because I couldn't tell if somebody else relies on Ipv6Helper being nothing more than sockaddr_in6.

I guess that one alternative would be to move the field to the Ipv6Helper struct and that should be then retrievable via a new function, e.g., address.ip().ipv6().v6only(). I didn't do this because I couldn't tell if somebody else relies on Ipv6Helper being nothing more than sockaddr_in6.

Right this is what I would do. The helper is just an implementation of an interface and I think this can live on the IPv6 interface?

mattklein123 · 2020-05-20T22:51:31Z

 namespace Envoy {
 namespace Network {

+namespace SocketInterface {


Do you want this to be an actual abstract class that you can inject later? Or you want to do that as a follow up? Ultimately I suspect you want this to hang off of the Api interface where we can handle platform injection more easily. Not sure if it's better to do it now or wait until later. I don't feel strongly about it.

I was planning on leaving it for later, especially because I'm not entirely sure about the best approach.

Originally, I was hoping I could hide all of the "raw" socket calls behind the SocketImpl constructors, but then I stumbled upon some places that only care about the IoHandles (and rely on syscalls to retrieve addresses and whatnot). So, I decided to postpone the decision until I completely migrate everything to SocketImpl.

OK we can do the injection later.

mattklein123 · 2020-05-20T22:54:07Z

+  return io_handle;
+}
+
+bool SocketInterface::ipFamilySupported(int domain) {


This is a dup of the function in the address file, right? Can we collapse this?

Yup, it tries to reproduce what Ipv6Instance::socket() was doing. I think that the v6 specific logic should eventually be handled with socket->setOption(setv6only) so this could be cleaned up. However, v6only is true by default, so that could mean a lot of changes.

Sorry what I mean is this is the same function as https://github.com/envoyproxy/envoy/pull/11278/files#diff-3a2eb9279fdb086a43b5b47dcbe278cdR22-R32, right? Can you just have this be a utility function in network::utility or something?

Sorry, I completely missed that you were pointing at ipFamilySupported.

I was about to mark this as done, but then noticed that the socket functions were built such that they cannot fail (RELEASE_ASSERT on the socket() return value), therefore we can't rely on this logic in the utility function to test if a socket type is usable.

So we're left with three options:

change SocketInterface::socket() to allow for failures (not sure about the impact in the rest of the codebase ..)

do the af test in a different way, but that will probably involve introducing a new SocketInterface function, i.e., something like SocketInterface::socket() that can fail.

keep on using this for now ...

Sorry I'm really confused. AFAICT the body of this function is exactly the same as the one I pointed to. Why can't they be shared?

As usual, I'm the one confused (I thought you felt this function didn't belong to SocketInterface).

The duplication is actually a byproduct of how I split the original PR. In the next PR, I'll completely remove the validateIpvXSupported check from the address, together with the remaining socket api calls, and wanted to have that conversation once we get there. If you think we shouldn't remove the validations, I can add a utility function now.

OK if you are going to deal with this in a follow up it's fine to leave. SGTM. Can you push the other change and I can take a look?

Here's a draft patch

I can also rebase the patch and push a separate PR if that works better.

We can just do it as a follow up. Thank you!

Thank you for the quick replies! And apologies again for the confusion.

- move v6only to the Ipv6 interface Signed-off-by: Florin Coras <fcoras@cisco.com>

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

mattklein123

Thanks!

mattklein123 · 2020-05-21T21:00:54Z

/retest

repokitteh-read-only · 2020-05-21T21:00:57Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #11278 (comment) was created by @mattklein123.

see: more, trace.

mattklein123 · 2020-05-21T23:13:58Z

This has failed coverage twice, so might not be a flake. Can you see if any coverage is missing now? The report should be in CI artifacts.

/wait

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

florincoras · 2020-05-22T04:06:12Z

/retest

repokitteh-read-only · 2020-05-22T04:06:15Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #11278 (comment) was created by @florincoras.

see: more, trace.

florincoras · 2020-05-22T06:44:37Z

Seems to be passing now but not sure if this is just a fluke or not. From the logs, it appears the hot restart tests did not run and because of that the overall code coverage dropped under 97%.

I managed to run the coverage script on one of my severs and, the one time I tried it, the hot restart test did fail. Weirdly enough, it does not fail when running it in isolation or as part of the whole test suite (timeouts maybe?).

I also tried the coverage script with the next PR and, again only one sample, that one passed.

mattklein123 · 2020-05-22T14:06:25Z

OK thanks. I think there is a known flake where hot restart coverage is not always being counted. cc @lizan

florincoras · 2020-05-22T15:45:56Z

Thanks a lot, @mattklein123! Pushed last PR in this first series.

network: add socket interface

761832f

Move socket calls that involve raw fds from address to socket. Signed-off-by: Florin Coras <fcoras@cisco.com>

florincoras requested review from alyssawilk and mattklein123 as code owners May 20, 2020 21:37

mattklein123 self-assigned this May 20, 2020

network: fix quic listeners

aacb9e6

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

mattklein123 requested changes May 20, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label May 20, 2020

addressing comments

57f37f6

- move v6only to the Ipv6 interface Signed-off-by: Florin Coras <fcoras@cisco.com>

repokitteh-read-only Bot removed the waiting label May 21, 2020

fix statsd test

d93cc44

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

mattklein123 previously approved these changes May 21, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label May 21, 2020

florincoras dismissed mattklein123’s stale review via 1e6695a May 22, 2020 00:52

repokitteh-read-only Bot removed the waiting label May 22, 2020

fix peerAddressFromFd

1e6695a

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

mattklein123 approved these changes May 22, 2020

View reviewed changes

mattklein123 merged commit 89150d1 into envoyproxy:master May 22, 2020

florincoras deleted the sock_interface branch May 28, 2020 15:47

Conversation

florincoras commented May 20, 2020

Uh oh!

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

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented May 21, 2020

Uh oh!

repokitteh-read-only Bot commented May 21, 2020

Uh oh!

mattklein123 commented May 21, 2020

Uh oh!

florincoras commented May 22, 2020

Uh oh!

repokitteh-read-only Bot commented May 22, 2020

Uh oh!

florincoras commented May 22, 2020

Uh oh!

mattklein123 commented May 22, 2020

Uh oh!

florincoras commented May 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants