[Windows] Cache v6only option when we create ipv6 socket #11793

davinci26 · 2020-06-28T19:16:55Z

On Windows, getsockopt fails if the socket driver is performing another
non-blocking operation. For example if we query for a specific socket
option while the socket is connecting then the getsockopt is going to
fail with error (10022).

When we create an ipv6 socket we set it to be ipv6 only. Now we cache
this configuration and we query it before we connect to the socket

Signed-off-by: Sotiris Nanopoulos sonanopo@microsoft.com

Additional Description:
This fixes IpVersions/TcpClientConnectionImplTest.BadConnectConnRefused/IPv6 test on Windows.

Risk Level: Low
Testing: Existing test cover this scenario.
Docs Changes:
Release Notes: Fixes an issue on Windows that get socket option returns invalid parameters when Envoy is asynchronously connecting to it. The resolution was to cache the socket option (v6only) option when the socket is created.
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]

davinci26 · 2020-06-28T19:17:16Z

@nigriMSFT @wrowe @sunjayBhatia

davinci26 · 2020-06-28T19:17:39Z

@florincoras can you please take a look since you have been tinkering with the socket interface :)

davinci26 · 2020-06-28T19:24:18Z

source/common/network/socket_impl.cc

@@ -122,5 +127,22 @@ absl::optional<Address::IpVersion> SocketImpl::ipVersion() const {
  return absl::nullopt;
 }

+bool SocketImpl::isSocketV6Only(const Network::Address::InstanceConstSharedPtr address) const { 


This could be moved to address_impl

Because it depends on io_handle_, I'm not sure we can do the move. As per my general comment, this is needed because we "forget" what we set on the Socket/IoHandle at creation time. We could potentially improve that.

florincoras · 2020-06-28T21:27:34Z

@davinci26 thanks for the heads up! This looks okay but it got me thinking that we are explicitly setting v6only when we create the socket, so we should probably store upfront the value in the IoHandle impl. In theory, this would then avoid the need to retrieve the setting via getOption() and it would simplify localAddress() avoid complicating connect.

@ggreenway what do you think? And a more naive question: do we ever want IPV6_V6ONLY to be false in practice?

davinci26 · 2020-06-29T17:16:04Z

Awesome, your comment makes sense. I was thinking about this but then I wasnt sure if there is any scenario that we set to IPV6_V6ONLY implicitly ( maybe from a user configuration).

If there is no such scenario it makes sense to store this info at construction and remove that call entirely

ggreenway

Please fix the CI failure.

Should there be any tests added for this case? Or is this fixing a race condition, and so no new coverage is needed?

/wait

source/common/network/socket_impl.cc

davinci26 · 2020-07-06T22:50:52Z

@ggreenway I will fix the CI. I think since the race condition was found from an existing (on Windows which will be soon be part of the CI) we dont need additional test coverage. What do you think?

@florincoras I think that we need to do the query unfortunately because the default value is set by the system.

According to man/ipv6:

The default value for this flag is defined by the contents of the file /proc/sys/net/ipv6/bindv6only. The default value for that file is 0 (false).

The way I interpret this is that there might be a case for a system/environment is ipv6_only due to a different system configuration and it is not Envoy that sets the property. In that case we should be aware and query the socket to get the option.

florincoras · 2020-07-07T02:25:50Z

The way I interpret this is that there might be a case for a system/environment is ipv6_only due to a different system configuration and it is not Envoy that sets the property. In that case we should be aware and query the socket to get the option.

We are currently explicitly setting IPV6_V6ONLY when we create the socket via this api. That is, when provided with an address, we do override the system default. We should probably check the system default and store the value in the IoHandle when using any of the other apis. That would entirely avoid the checking the value in localAddress(), but I guess it could be done at a later time.

ggreenway · 2020-07-08T16:56:43Z

The build is broken.

Please add a release note about the bug being fixed.

@florincoras thanks for your input on this PR. Since you've had the most experience recently in this area, can I assign this PR to you for review?

/wait

florincoras · 2020-07-08T16:59:16Z

@ggreenway sure!

stale · 2020-07-18T12:56:00Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

davinci26 · 2020-07-18T22:35:09Z

Apologies for letting this get stale last week was crazy. I am working on the changes now.

Signed-off-by: davinci26 <sotirisnan@gmail.com>

davinci26 · 2020-07-20T18:18:19Z

@florincoras I opted for the implementation that you proposed since its way cleaner ( I get to delete code instead of writing 😄 ) I am getting the CI to be fully green and I will ping you when I am ready for a review

florincoras · 2020-07-20T18:19:58Z

@florincoras I opted for the implementation that you proposed since its way cleaner ( I get to delete code instead of writing 😄 ) I am getting the CI to be fully green and I will ping you when I am ready for a review

Sounds good! ;-)

Signed-off-by: davinci26 <sotirisnan@gmail.com>

davinci26 · 2020-07-21T00:18:16Z

@florincoras ready for review

the CI is clean minus cx_limit_integration_test which seems to be flaky #11841

davinci26 · 2020-07-21T03:40:43Z

@ggreenway I am ready for your review as well!

ggreenway · 2020-07-21T17:27:21Z

/azp run

azure-pipelines · 2020-07-21T17:27:32Z

Azure Pipelines successfully started running 1 pipeline(s).

ggreenway

This looks good overall, just one thing to fix.

/wait

include/envoy/network/socket.h

Signed-off-by: davinci26 <sotirisnan@gmail.com>

ggreenway

LGTM

davinci26 · 2020-07-23T23:59:40Z

@ggreenway can we q tsan again and see what the process it to merge it in.

I want to add it to #12172 and see if it makes the tests there less flaky

ggreenway · 2020-07-24T00:06:17Z

/azp run

azure-pipelines · 2020-07-24T00:06:25Z

Azure Pipelines successfully started running 1 pipeline(s).

…onnect

davinci26 · 2020-07-25T00:56:16Z

I merged master and pushed again and hopefully it will help with the flaky tests

On Windows, getsockopt fails if the socket driver is performing another non-blocking operation. For example if we query for a specific socket option while the socket is connecting then the getsockopt is going to fail with error (10022). When we create an ipv6 socket we set it to be ipv6 only. Now we cache this configuration and we query it before we connect to the socket This fixes IpVersions/TcpClientConnectionImplTest.BadConnectConnRefused/IPv6 test on Windows. Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

On Windows, getsockopt fails if the socket driver is performing another non-blocking operation. For example if we query for a specific socket option while the socket is connecting then the getsockopt is going to fail with error (10022). When we create an ipv6 socket we set it to be ipv6 only. Now we cache this configuration and we query it before we connect to the socket This fixes IpVersions/TcpClientConnectionImplTest.BadConnectConnRefused/IPv6 test on Windows. Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com>

On Windows, getsockopt fails if the socket driver is performing another non-blocking operation. For example if we query for a specific socket option while the socket is connecting then the getsockopt is going to fail with error (10022). When we create an ipv6 socket we set it to be ipv6 only. Now we cache this configuration and we query it before we connect to the socket This fixes IpVersions/TcpClientConnectionImplTest.BadConnectConnRefused/IPv6 test on Windows. Signed-off-by: Sotiris Nanopoulos <sonanopo@microsoft.com> Signed-off-by: chaoqinli <chaoqinli@google.com>

davinci26 commented Jun 28, 2020

View reviewed changes

mattklein123 assigned ggreenway Jun 30, 2020

ggreenway requested changes Jul 6, 2020

View reviewed changes

source/common/network/socket_impl.cc Outdated Show resolved Hide resolved

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

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

davinci26 requested review from alyssawilk and mattklein123 as code owners July 7, 2020 01:27

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

ggreenway assigned florincoras Jul 8, 2020

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jul 18, 2020

davinci26 mentioned this pull request Jul 18, 2020

Use TestEnvironment::nullDevicePath instead of /dev/null/ #12172

Merged

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jul 18, 2020

davinci26 added 2 commits July 19, 2020 12:07

save socket info

a41ba38

Signed-off-by: davinci26 <sotirisnan@gmail.com>

cascading changes and glue code

f988dc5

Signed-off-by: davinci26 <sotirisnan@gmail.com>

davinci26 force-pushed the getSockOptBeforeConnect branch from de8400c to f988dc5 Compare July 20, 2020 18:16

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

davinci26 added 2 commits July 20, 2020 11:39

clang format

153c847

Signed-off-by: davinci26 <sotirisnan@gmail.com>

clang format vol 2

69d2faf

Signed-off-by: davinci26 <sotirisnan@gmail.com>

remove the mock call from quic io handle wrapper test

36fb66a

Signed-off-by: davinci26 <sotirisnan@gmail.com>

davinci26 changed the title ~~[Windows] Query for IPV6_V6ONLY before connect~~ [Windows] Cache v6only option when we create ipv6 socket Jul 21, 2020

florincoras previously approved these changes Jul 21, 2020

View reviewed changes

ggreenway requested changes Jul 21, 2020

View reviewed changes

include/envoy/network/socket.h Outdated Show resolved Hide resolved

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

Change interface to be bool instead of int

0c2558e

Signed-off-by: davinci26 <sotirisnan@gmail.com>

davinci26 dismissed florincoras’s stale review via 0c2558e July 22, 2020 22:09

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

ggreenway approved these changes Jul 22, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into getSockOptBeforeC…

9a96bdf

…onnect

ggreenway merged commit ff0beb1 into envoyproxy:master Jul 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Windows] Cache v6only option when we create ipv6 socket #11793

[Windows] Cache v6only option when we create ipv6 socket #11793

davinci26 commented Jun 28, 2020 •

edited

Loading

davinci26 commented Jun 28, 2020

davinci26 commented Jun 28, 2020

davinci26 Jun 28, 2020 •

edited

Loading

florincoras Jun 28, 2020

florincoras commented Jun 28, 2020

davinci26 commented Jun 29, 2020

ggreenway left a comment

davinci26 commented Jul 6, 2020

florincoras commented Jul 7, 2020

ggreenway commented Jul 8, 2020

florincoras commented Jul 8, 2020

stale bot commented Jul 18, 2020

davinci26 commented Jul 18, 2020

davinci26 commented Jul 20, 2020

florincoras commented Jul 20, 2020

davinci26 commented Jul 21, 2020

davinci26 commented Jul 21, 2020

ggreenway commented Jul 21, 2020

azure-pipelines bot commented Jul 21, 2020

ggreenway left a comment

ggreenway left a comment

davinci26 commented Jul 23, 2020 •

edited

Loading

ggreenway commented Jul 24, 2020

azure-pipelines bot commented Jul 24, 2020

davinci26 commented Jul 25, 2020 •

edited

Loading

[Windows] Cache v6only option when we create ipv6 socket #11793

[Windows] Cache v6only option when we create ipv6 socket #11793

Conversation

davinci26 commented Jun 28, 2020 • edited Loading

davinci26 commented Jun 28, 2020

davinci26 commented Jun 28, 2020

davinci26 Jun 28, 2020 • edited Loading

Choose a reason for hiding this comment

florincoras Jun 28, 2020

Choose a reason for hiding this comment

florincoras commented Jun 28, 2020

davinci26 commented Jun 29, 2020

ggreenway left a comment

Choose a reason for hiding this comment

davinci26 commented Jul 6, 2020

florincoras commented Jul 7, 2020

ggreenway commented Jul 8, 2020

florincoras commented Jul 8, 2020

stale bot commented Jul 18, 2020

davinci26 commented Jul 18, 2020

davinci26 commented Jul 20, 2020

florincoras commented Jul 20, 2020

davinci26 commented Jul 21, 2020

davinci26 commented Jul 21, 2020

ggreenway commented Jul 21, 2020

azure-pipelines bot commented Jul 21, 2020

ggreenway left a comment

Choose a reason for hiding this comment

ggreenway left a comment

Choose a reason for hiding this comment

davinci26 commented Jul 23, 2020 • edited Loading

ggreenway commented Jul 24, 2020

azure-pipelines bot commented Jul 24, 2020

davinci26 commented Jul 25, 2020 • edited Loading

davinci26 commented Jun 28, 2020 •

edited

Loading

davinci26 Jun 28, 2020 •

edited

Loading

davinci26 commented Jul 23, 2020 •

edited

Loading

davinci26 commented Jul 25, 2020 •

edited

Loading