-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
network: Udp socket implementation. #5108
Conversation
c0fa1e9
to
a327a0d
Compare
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
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>
a327a0d
to
ba0f338
Compare
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
@alyssawilk do you want to take first pass? cc @cmluciano also. |
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
FWIW, I also drafted a take on this at #5115, but am happy to abandon that in favor of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor typo but otherwise lgtm. I like the template for the socket.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cmluciano Thanks for the suggestion. Do you want to rebase and update the change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it show in the browser an option for just merging it in? If not, I can resubmit to your fork.
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! |
IMHO if @mpwarres can add the additional tests and UDP URI scheme support to this PR, we should be good. |
Sure thing, will do that later this evening. Thanks! |
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com>
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! |
template <> | ||
void NetworkListenSocket< | ||
NetworkSocketTrait<Address::SocketType::Datagram>>::setProtocolSpecificSocketOptions() { | ||
// TODO (Jojy): Do we need any UDP specific socket options? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agree, if we can't come up with anything we can just remove the TODO here.
@@ -26,7 +26,7 @@ INSTANTIATE_TEST_CASE_P(IpVersions, ListenSocketImplTest, | |||
testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), | |||
TestUtility::ipTestParamsToString); | |||
|
|||
TEST_P(ListenSocketImplTest, BindSpecificPort) { | |||
TEST_P(ListenSocketImplTest, BindSpecificTcpPort) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: for consistency with BindPortZeroTcp later on, maybe name this BindSpecificPortTcp?
@@ -67,8 +67,42 @@ TEST_P(ListenSocketImplTest, BindSpecificPort) { | |||
EXPECT_EQ(addr->asString(), socket3.localAddress()->asString()); | |||
} | |||
|
|||
TEST_P(ListenSocketImplTest, BindSpecificUDPPort) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for consistency with BindPortZeroUdp, maybe name this BindSpecificPortUdp?
Thanks @mpwarres . I will update the PR with your changes once I build and test. |
34982ea
to
070ca04
Compare
@mpwarres I have addressed your review comments. Thanks for the suggestions. |
@mpwarres let myself and @alyssawilk know when you are done reviewing and we can take a look. Will assign you. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@mattklein123 and @alyssawilk , I'm all set, ready for you to take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I love our new best-of PR :-)
Just a few nits on my end
template <> | ||
void NetworkListenSocket< | ||
NetworkSocketTrait<Address::SocketType::Datagram>>::setProtocolSpecificSocketOptions() { | ||
// TODO (Jojy): Do we need any UDP specific socket options? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, agree, if we can't come up with anything we can just remove the TODO here.
source/common/network/utility.cc
Outdated
@@ -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; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, totally blanked on that. Yeah, let's change it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alyssawilk I assume it is case sensitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
@@ -67,8 +67,50 @@ TEST_P(ListenSocketImplTest, BindSpecificPort) { | |||
EXPECT_EQ(addr->asString(), socket3.localAddress()->asString()); | |||
} | |||
|
|||
TEST_P(ListenSocketImplTest, BindSpecificPortUdp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alyssawilk agree. I wanted to do it like I did for the proxy test. Added now. Not sure if this is what you meant.
@@ -994,6 +999,8 @@ TEST_P(WildcardProxyProtocolTest, BasicV6) { | |||
disconnect(); | |||
} | |||
|
|||
// TODO (Jojy): Add tests for UDP ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great to see progress here. Generally LGTM. Thank you!
source/common/network/utility.cc
Outdated
@@ -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; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -16,66 +16,94 @@ using testing::Return; | |||
namespace Envoy { | |||
namespace Network { | |||
|
|||
template <Network::Address::SocketType S> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: s/S/SocketType or something spelled out.
setupSocket(options, bind_to_port); | ||
} | ||
|
||
NetworkListenSocket(int fd, const Address::InstanceConstSharedPtr& address, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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 tobind
- only the constructor that takes
address
sets upSO_REUSEADDR
.
So wanted to keep the logic same here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense, +1 please rename.
class ListenSocketImplTest : public testing::TestWithParam<Address::IpVersion> { | ||
protected: | ||
ListenSocketImplTest() : version_(GetParam()) {} | ||
const Address::IpVersion version_; | ||
|
||
template <typename... Args> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
source/common/network/utility.cc
Outdated
@@ -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; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also mind switching to TODO(conqerAtapple) when you add TODOs?
@alyssawilk re:URI scheme parsing, have we considered regex to parse the scheme? |
@mattklein123 @alyssawilk Is this mergeable? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :-)
er, of Matt's, not yours, so worth addressing :-P |
@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. |
@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
|
This is personal preference, but generally I only do this for super obvious cases such as
Personally I think that's fine. Otherwise I would do |
@mattklein123 Thanks for the suggestions. I think I have addressed all the review comments now. Please let me know otherwise. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Signed-off-by: Jojy G Varghese <jojy_varghese@apple.com> Signed-off-by: Fred Douglas <fredlas@google.com>
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