network: Udp socket implementation. #5108

conqerAtapple · 2018-11-26T03:13:42Z

Description: Implements UDP socket. This is one of the basic features needed for QUIC project.
Risk Level: Medium
Testing: Unit tests added.
Docs Changes: N/A
Release Notes: N/A
Part of #2557 #1193 #492

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Signed-off-by: Michael Warres <mpw@google.com> Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

mattklein123 · 2018-11-26T14:02:18Z

@alyssawilk do you want to take first pass? cc @cmluciano also.

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

mpwarres · 2018-11-26T15:27:12Z

FWIW, I also drafted a take on this at #5115, but am happy to abandon that in favor of this.

alyssawilk

I'm good with either PR (I don't have strong preference for the pros and cons of template ugliness to get maximum code reuse).

If we go with this one let's make sure we resolve any SO_REUSEADDR concerns, and do best-of testing

conqerAtapple · 2018-11-26T18:13:01Z

@mpwarres Didn't know there was another PR :). I like the UDP url scheme you added in your PR. If you don't mind, we can probably add your URL scheme changes and the test here. Let me know how you feel.

cmluciano

Minor typo but otherwise lgtm. I like the template for the socket.

cmluciano · 2018-11-26T18:34:43Z

test/common/network/listen_socket_impl_test.cc

+  // WARNING: This test has a small but real risk of flaky behavior if another thread or process
+  // should bind to our assigned port during the interval between closing the fd and re-binding.
+  // TODO(jamessynge): Consider adding a loop or other such approach to this test so that a
+  // bind failure (in the UdpListenSocket ctor) once isn't considered an error.


Suggested change

// bind failure (in the UdpListenSocket ctor) once isn't considered an error.

// bind failure (in the UdpListenSocket ctor) once it isn't considered an error.

@cmluciano Thanks for the suggestion. Do you want to rebase and update the change?

Does it show in the browser an option for just merging it in? If not, I can resubmit to your fork.

mattklein123 · 2018-11-26T19:09:12Z

I'm fine with either PR also, so @mpwarres @conqerAtapple let myself and @alyssawilk know which PR to focus on and we can close the other one. Thank you!

conqerAtapple · 2018-11-26T21:53:51Z

IMHO if @mpwarres can add the additional tests and UDP URI scheme support to this PR, we should be good.

mpwarres · 2018-11-26T22:52:40Z

Sure thing, will do that later this evening. Thanks!

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

mpwarres · 2018-11-27T06:44:26Z

Ok, sent conqerAtapple#1 to update #5108 with parts of #5115. My github-fu is kind of weak, so let me know if I need to redo anything. Thanks!

mpwarres · 2018-11-27T06:49:44Z

source/common/network/listen_socket_impl.cc

+template <>
+void NetworkListenSocket<
+    NetworkSocketTrait<Address::SocketType::Datagram>>::setProtocolSpecificSocketOptions() {
+  // TODO (Jojy): Do we need any UDP specific socket options?


My vote would be not to set any socket options for UDP here, per this comment from #5115. Users that want to tune UDP socket options can always do so via SocketOptionImpl.

I think to be consistent with the TCP code path, we should leave this here. Later when we move TCP's SO_REUSEADDR setup, we can completely remove the setProtocolSpecificSocketOptions method. What do you think?

Sorry, I wasn't very clear. I agree it makes sense to keep the method, I just think that currently it should be left empty. i.e. I think the current form is fine, was just weighing in on the TODO comment.

Yeah, agree, if we can't come up with anything we can just remove the TODO here.

mpwarres · 2018-11-27T06:51:37Z

test/common/network/listen_socket_impl_test.cc

@@ -26,7 +26,7 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ListenSocketImplTest,
                        testing::ValuesIn(TestEnvironment::getIpVersionsForTest()),
                        TestUtility::ipTestParamsToString);

-TEST_P(ListenSocketImplTest, BindSpecificPort) {
+TEST_P(ListenSocketImplTest, BindSpecificTcpPort) {


nit: for consistency with BindPortZeroTcp later on, maybe name this BindSpecificPortTcp?

mpwarres · 2018-11-27T06:52:39Z

test/common/network/listen_socket_impl_test.cc

@@ -67,8 +67,42 @@ TEST_P(ListenSocketImplTest, BindSpecificPort) {
  EXPECT_EQ(addr->asString(), socket3.localAddress()->asString());
 }

+TEST_P(ListenSocketImplTest, BindSpecificUDPPort) {


for consistency with BindPortZeroUdp, maybe name this BindSpecificPortUdp?

conqerAtapple · 2018-11-27T16:24:34Z

Thanks @mpwarres . I will update the PR with your changes once I build and test.

conqerAtapple · 2018-11-27T18:31:40Z

@mpwarres I have addressed your review comments. Thanks for the suggestions.

mattklein123 · 2018-11-27T19:02:31Z

@mpwarres let myself and @alyssawilk know when you are done reviewing and we can take a look. Will assign you.

mpwarres

LGTM

mpwarres · 2018-11-27T19:17:21Z

@mattklein123 and @alyssawilk , I'm all set, ready for you to take a look.

alyssawilk

Cool, I love our new best-of PR :-)

Just a few nits on my end

alyssawilk · 2018-11-27T20:02:12Z

source/common/network/listen_socket_impl.cc

+template <>
+void NetworkListenSocket<
+    NetworkSocketTrait<Address::SocketType::Datagram>>::setProtocolSpecificSocketOptions() {
+  // TODO (Jojy): Do we need any UDP specific socket options?


Yeah, agree, if we can't come up with anything we can just remove the TODO here.

alyssawilk · 2018-11-27T20:08:44Z

source/common/network/utility.cc

@@ -50,29 +53,33 @@ Address::InstanceConstSharedPtr Utility::resolveUrl(const std::string& url) {
 }

 bool Utility::urlIsTcpScheme(const std::string& url) { return url.find(TCP_SCHEME) == 0; }
-
+bool Utility::urlIsUdpScheme(const std::string& url) { return url.find(UDP_SCHEME) == 0; }


huh, I'd expect this to be absl::StartsWith but I suspect we're better off with consistency so worth leaving as-is.
Just for curiosity's sake, @mattklein123 do we have use cases where we expect the tcp:// to not be at the beginning? Did config have whitespace preface at some point?

It's only prefix. I think this was so early on there probably wasn't even StringUtil::startsWith back then. Feel free to change this to absl::StartsWith. You could also change these functions to take a string_view also if you like.

Ok, let's make UDP startsWith and add a TODO either for @conqerAtapple or for me to update the other two?
After the joys of the xff whitespace change I prefer making "this should be fine" changes to existing code in standalone PRs :-P

This is already checking that it's at position 0, so I think it can be changed without any fear? It's just a perf optimization.

Oh yeah, totally blanked on that. Yeah, let's change it!

@alyssawilk I assume it is case sensitive.

alyssawilk · 2018-11-27T20:15:04Z

test/common/network/listen_socket_impl_test.cc

@@ -67,8 +67,50 @@ TEST_P(ListenSocketImplTest, BindSpecificPort) {
  EXPECT_EQ(addr->asString(), socket3.localAddress()->asString());
 }

+TEST_P(ListenSocketImplTest, BindSpecificPortUdp) {


Mild preference for making SocketType a new argument to the TEST_P, and having a createTcpListenSocketPtr which switches on type so that rather than copy/paste/editing each test as we add them, we get them for free.

@alyssawilk agree. I wanted to do it like I did for the proxy test. Added now. Not sure if this is what you meant.

alyssawilk · 2018-11-27T20:22:25Z

test/extensions/filters/listener/proxy_protocol/proxy_protocol_test.cc

@@ -994,6 +999,8 @@ TEST_P(WildcardProxyProtocolTest, BasicV6) {
  disconnect();
 }

+// TODO (Jojy): Add tests for UDP ?


I'd take this out (and not bother renaming the tests yet).

Unfortunately as far as I know there's no great way to do this for UDP proxying. In QUIC, we can't insert the logical equivalent of the proxy header because transparent proxies aren't guaranteed to have the certs, so can't terminate the transport to insert things in. There are non-RFCified super-hacky packet-munging tricks you can play with for QUIC when we get there, but it's far enough down the line I don't think we need a TODO for it.

mattklein123

Great to see progress here. Generally LGTM. Thank you!

mattklein123 · 2018-11-27T22:36:50Z

source/common/network/utility.cc

@@ -50,29 +53,33 @@ Address::InstanceConstSharedPtr Utility::resolveUrl(const std::string& url) {
 }

 bool Utility::urlIsTcpScheme(const std::string& url) { return url.find(TCP_SCHEME) == 0; }
-
+bool Utility::urlIsUdpScheme(const std::string& url) { return url.find(UDP_SCHEME) == 0; }


It's only prefix. I think this was so early on there probably wasn't even StringUtil::startsWith back then. Feel free to change this to absl::StartsWith. You could also change these functions to take a string_view also if you like.

mattklein123 · 2018-11-27T22:39:11Z

test/common/network/listen_socket_impl_test.cc

@@ -16,66 +16,94 @@ using testing::Return;
 namespace Envoy {
 namespace Network {

+template <Network::Address::SocketType S>


nit: s/S/SocketType or something spelled out.

mattklein123 · 2018-11-27T22:41:38Z

source/common/network/listen_socket_impl.h

+    setupSocket(options, bind_to_port);
+  }
+
+  NetworkListenSocket(int fd, const Address::InstanceConstSharedPtr& address,


Don't we need to call setProtocolSpecificSocketOptions() in this variant also? Can we somehow unify the setup logic? It's a little confusing as with the constructor variants, protocol specific options, listener options, etc.

@mattklein123 Thanks for reviewing.
Looking at the original code(master), the two constructors differ in -

the constructor that takes fd does not have an option to bind

only the constructor that takes address sets up SO_REUSEADDR.

So wanted to keep the logic same here.

Hmm, I suspect that's a bug and not a feature. Can you spend a little bit of time trying to figure out where the constructors are used? I would prefer we take this opportunity to unify the logic if possible?

I think the idea is that when an fd is present, then we don't create the listening socket and assume that the socket is already bound (to that listen fd). So in that case, we should not be applying the option SO_REUSEADDR. All other options could still be applied . Although its confusing to have flag PRE_BIND for all the options.

So maybe we should rename setProtocolSpecificSocketOptions to setPrebindSocketOptions ?

That makes sense, +1 please rename.

alyssawilk · 2018-11-28T14:55:36Z

test/common/network/listen_socket_impl_test.cc

 class ListenSocketImplTest : public testing::TestWithParam<Address::IpVersion> {
 protected:
  ListenSocketImplTest() : version_(GetParam()) {}
  const Address::IpVersion version_;
+
+  template <typename... Args>


Oh, this is really interesting - new syntax to me.

I'd been suggesting something more along the lines of
testing::TestWithParam<std::tuple<Network::Address::IpVersion, Address::SocketType>>

That'd allow you to keep the various test code inline with the test cases since things like

if (NetworkSocketTrait<S>::type == Address::SocketType::Stream) {
would be more like
if (std::get<1>(GetParam() == == Address::SocketType::Stream)
so we wouldn't need all the code to be located in class functions.

While I think having test code inline with TEST_P is nice and it would reduce the diffs, I'm going to go ahead and say your current implementation solves my code duplication concerns, so if you want to leave as-is and not rewrite the tests a third time that's Ok :-)

alyssawilk · 2018-11-28T15:00:37Z

source/common/network/utility.cc

@@ -50,29 +53,33 @@ Address::InstanceConstSharedPtr Utility::resolveUrl(const std::string& url) {
 }

 bool Utility::urlIsTcpScheme(const std::string& url) { return url.find(TCP_SCHEME) == 0; }
-
+bool Utility::urlIsUdpScheme(const std::string& url) { return url.find(UDP_SCHEME) == 0; }


Ok, let's make UDP startsWith and add a TODO either for @conqerAtapple or for me to update the other two?
After the joys of the xff whitespace change I prefer making "this should be fine" changes to existing code in standalone PRs :-P

alyssawilk · 2018-11-28T15:01:18Z

test/common/network/listen_socket_impl_test.cc

+        .WillOnce(Return(true));
+    options->emplace_back(std::move(option));
+    auto socket1 = createListenSocketPtr(addr, options, true);
+    // TODO (Jojy): This is unfortunate. We should be able to templatize this


Also mind switching to TODO(conqerAtapple) when you add TODOs?

conqerAtapple · 2018-11-28T16:13:29Z

@alyssawilk re:URI scheme parsing, have we considered regex to parse the scheme?

conqerAtapple · 2018-11-29T18:35:04Z

@mattklein123 @alyssawilk Is this mergeable?

alyssawilk

Fine by me.

I believe there was an outstanding request of yours "nit: s/S/SocketType or something spelled out." I'd been waiting on but given it's test code I don't much care :-)

alyssawilk · 2018-11-29T18:45:08Z

er, of Matt's, not yours, so worth addressing :-P

mattklein123 · 2018-11-29T20:38:38Z

@conqerAtapple friendly ask in the future to not force push if possible, since it makes it harder to review. Just do merge and regular commits and we squash at the end. If you feel like fixing the nit that would be nice but I don't feel that strongly about it.

conqerAtapple · 2018-11-29T23:58:41Z

@mattklein123 re: the nit, I wanted to make sure we follow the standard way of expressing a template parameter. It is usually 1 letter. But I can change it to ST ? SocketType is already the type the template accepts so it will be confusing to have something like :

template <Network::Address::SocketType SocketType>

mattklein123 · 2018-11-30T00:03:03Z

It is usually 1 letter.

This is personal preference, but generally I only do this for super obvious cases such as class C. Everything else I would spell out.

so it will be confusing to have something like

Personally I think that's fine. Otherwise I would do Type

conqerAtapple · 2018-11-30T16:03:02Z

@mattklein123 Thanks for the suggestions. I think I have addressed all the review comments now. Please let me know otherwise.

mattklein123

Thanks!

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com> Signed-off-by: Fred Douglas <fredlas@google.com>

conqerAtapple force-pushed the udp_socket_impl branch from c0fa1e9 to a327a0d Compare November 26, 2018 03:17

conqerAtapple and others added 3 commits November 25, 2018 19:44

UDP socket implementation.

22c6319

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Added templatized protocol specific socket options.

70c6131

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Add parts of PR envoyproxy#5115 into PR envoyproxy#5108.

070ca04

Signed-off-by: Michael Warres <mpw@google.com> Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

conqerAtapple force-pushed the udp_socket_impl branch from a327a0d to ba0f338 Compare November 26, 2018 06:07

conqerAtapple added 4 commits November 25, 2018 23:18

Modified listen_socket_impl_test to use templates.

43b7538

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Reverted changes to proxy_protocol_test.cc.

117f47f

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Renamed s/setProtocolSpecificSocketOptions/setPrebindSocketOptions.

a994a45

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

Changed TODO ownership s/Jojy/conqerAtapple

cf11020

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

mattklein123 assigned mattklein123 and alyssawilk Nov 26, 2018

mattklein123 mentioned this pull request Nov 26, 2018

WIP: basic udp listener support #4898

Closed

Changed Tcp/Udp/Unix scheme test to use startsWith.

dbb25c4

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

mattklein123 mentioned this pull request Nov 26, 2018

network: continue cmluciano UdpListenSocket, adding a test #5115

Closed

mpwarres mentioned this pull request Nov 26, 2018

UDP proxying support #492

Closed

7 tasks

alyssawilk reviewed Nov 26, 2018

View reviewed changes

cmluciano reviewed Nov 26, 2018

View reviewed changes

Changed template parameter name s/S/Type.

bb4f7a9

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>

mpwarres previously approved these changes Nov 27, 2018

View reviewed changes

conqerAtapple dismissed mpwarres’s stale review via 34982ea November 27, 2018 16:34

conqerAtapple force-pushed the udp_socket_impl branch from 34982ea to 070ca04 Compare November 27, 2018 16:43

mattklein123 assigned mpwarres Nov 27, 2018

mpwarres previously approved these changes Nov 27, 2018

View reviewed changes

alyssawilk reviewed Nov 27, 2018

View reviewed changes

conqerAtapple dismissed mpwarres’s stale review via 43b7538 November 27, 2018 21:35

mattklein123 reviewed Nov 27, 2018

View reviewed changes

mattklein123 added the waiting label Nov 28, 2018

alyssawilk reviewed Nov 28, 2018

View reviewed changes

repokitteh-read-only bot removed the waiting label Nov 28, 2018

alyssawilk previously approved these changes Nov 29, 2018

View reviewed changes

conqerAtapple dismissed alyssawilk’s stale review via bb4f7a9 November 30, 2018 16:01

mattklein123 approved these changes Dec 1, 2018

View reviewed changes

mattklein123 merged commit 704e4c6 into envoyproxy:master Dec 1, 2018

fredlas pushed a commit to fredlas/envoy that referenced this pull request Mar 5, 2019

network: Udp socket implementation. (envoyproxy#5108)

b131f40

Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com> Signed-off-by: Fred Douglas <fredlas@google.com>

howels mentioned this pull request Apr 17, 2019

UDP support istio/istio#1430

Open

prakash-halan mentioned this pull request Jul 10, 2019

Feature Request - UDP support emissary-ingress/emissary#1674

Open

	// bind failure (in the UdpListenSocket ctor) once isn't considered an error.
	// bind failure (in the UdpListenSocket ctor) once it isn't considered an error.

network: Udp socket implementation. #5108

network: Udp socket implementation. #5108

Conversation

conqerAtapple commented Nov 26, 2018 • edited by alyssawilk

mattklein123 commented Nov 26, 2018

mpwarres commented Nov 26, 2018

alyssawilk left a comment

Choose a reason for hiding this comment

conqerAtapple commented Nov 26, 2018

cmluciano 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

mattklein123 commented Nov 26, 2018

conqerAtapple commented Nov 26, 2018

mpwarres commented Nov 26, 2018

mpwarres commented Nov 27, 2018

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

Choose a reason for hiding this comment

conqerAtapple commented Nov 27, 2018

conqerAtapple commented Nov 27, 2018

mattklein123 commented Nov 27, 2018

mpwarres left a comment

Choose a reason for hiding this comment

mpwarres commented Nov 27, 2018

alyssawilk 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

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conqerAtapple commented Nov 28, 2018

conqerAtapple commented Nov 29, 2018

alyssawilk left a comment

Choose a reason for hiding this comment

alyssawilk commented Nov 29, 2018 • edited

mattklein123 commented Nov 29, 2018

conqerAtapple commented Nov 29, 2018 • edited

mattklein123 commented Nov 30, 2018

conqerAtapple commented Nov 30, 2018

mattklein123 left a comment

Choose a reason for hiding this comment

conqerAtapple commented Nov 26, 2018 •

edited by alyssawilk

alyssawilk commented Nov 29, 2018 •

edited

conqerAtapple commented Nov 29, 2018 •

edited