Skip to content
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

Skip Quic IPv6 tests on unsupported platforms #75341

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

rzikm
Copy link
Member

@rzikm rzikm commented Sep 9, 2022

This PR skips tests using IPv6 addresses on platforms where it would lead to EADDRNOTAVAIL errors.

@ghost
Copy link

ghost commented Sep 9, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: rzikm
Assignees: -
Labels:

area-System.Net.Quic

Milestone: -

Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wfurt
Copy link
Member

wfurt commented Sep 9, 2022

btw

(openSUSE.15.2.Amd64.Open)ubuntu.1604.amd64.open@mcr.microsoft.com/dotnet-buildtools/prereqs:opensuse-15.2-helix-amd64-20211018152525-9cc02fe

16.04 is old and way out of support. We should move to 18.04 or 20.04 base OS.

@rzikm
Copy link
Member Author

rzikm commented Sep 9, 2022

Does the base OS matter when running in a container?

@wfurt
Copy link
Member

wfurt commented Sep 9, 2022

yes, somewhat. It will share Linux kernel and interface auto-configuration and bridging is also done by the host AFAIK.

@rzikm
Copy link
Member Author

rzikm commented Sep 9, 2022

I see, can we simply change the base OS when updating the Helix image reference?

@rzikm rzikm merged commit 752cb7e into dotnet:main Sep 9, 2022
@wfurt
Copy link
Member

wfurt commented Sep 9, 2022

yes. You can then look at the console and see if tests were skipped or not. (or Kusto if we store such info)

@AustinWise
Copy link
Contributor

Are you sure the GetIsIPv6Available method is correct? Usually SocketType.Dgram is used with ProtocolType.Udp When GetIsIPv6Available is run as commited on macOS, I get this error:

Unhandled exception. System.Net.Sockets.SocketException (41): Protocol wrong type for socket

I think what was intended was:

using Socket s = new Socket(AddressFamily.InterNetworkV6, SocketType.Dgram, ProtocolType.Udp);

Note the Udp at the end of the line. Since you this test is related to Quic, I assume you meant to test for UDP.

AustinWise added a commit to AustinWise/runtime that referenced this pull request Sep 9, 2022
In dotnet#75341, some QUIC tests were changed to only run when IPv6 is supported.
IPv6 was detected by binding a socket. However the socket was bound with an
invalid combination of `SocketType.Dgram` and `ProtocolType.Tcp`. On macOS,
this fails with the exception:

```
System.Net.Sockets.SocketException (41): Protocol wrong type for socket
```

See the docs for listing of which combinations are valid to use together:

https://docs.microsoft.com/dotnet/api/system.net.sockets.sockettype

Since QUIC uses UDP, I think the intention was to bind a UDP socket.
rzikm pushed a commit that referenced this pull request Sep 12, 2022
In #75341, some QUIC tests were changed to only run when IPv6 is supported.
IPv6 was detected by binding a socket. However the socket was bound with an
invalid combination of `SocketType.Dgram` and `ProtocolType.Tcp`. On macOS,
this fails with the exception:

```
System.Net.Sockets.SocketException (41): Protocol wrong type for socket
```

See the docs for listing of which combinations are valid to use together:

https://docs.microsoft.com/dotnet/api/system.net.sockets.sockettype

Since QUIC uses UDP, I think the intention was to bind a UDP socket.
@ghost ghost locked as resolved and limited conversation to collaborators Oct 10, 2022
@karelz karelz added this to the 8.0.0 milestone Mar 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants