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

Add support for IPv6 connections #767

Closed
wants to merge 19 commits into from
Closed

Add support for IPv6 connections #767

wants to merge 19 commits into from

Conversation

Rinn
Copy link
Contributor

@Rinn Rinn commented Jul 13, 2020

Description

This change updates the networking code to accept IPv6 connections by changing the listen server to create an IPv6 socket that also accepts IPv4 connections.

I ensured the remote connection worked correctly on Windows with hostnames being resolved to both IPv4 & IPv6. I wasn't able to test with multiple machines on Linux or MacOS however all code compiled locally and the connection unit tests I added passed and the localhost connection was successful.

Any feedback is appreciated. I'll also say I'm not an expert in networking code so this was written with the help of a lot of documentation.

Please provide details for the change or fix

Support for ipv6 is desired as servers in our data center are not allocated ipv4 ip addresses by default and it's preferred we only use ipv6 networking

Core/Process/Semaphore.cpp\h

  • Update Semaphore::Wait to return false if time out occurs for use in Network.cpp

Core/Network/Network.cpp\h

  • Add GetAddressFromString helper functions to convert string to binary ip address using inet_pton
  • Add GetAddressAsString helper functions to convert binary ip address to string using inet_ntop
  • Add NameResolution helper functions to resolve hostnames using getaddrinfo
  • Update NameResolutionThreadFunc to check for completion with semaphores, to return ip addresses via shared memory instead of thread exit code, and to be responsible for freeing allocated memory
  • Attempt to resolve a hostname to ipv6 but only if the ipv4 lookup fails

Core/Network/TCPConnectionPool.cpp\h

  • Remove GetAddressAsString which was moved to Network.cpp\h
  • Update CreateSocket to add af parameter so an ipv6 socket can be created
  • Add version of Connect & CreateConnectionThread functions to use ipv6
  • Move socket wait for connection logic to helper function so it can be shared
  • Listen server now creates an ipv6 socket connection which also accepts ipv4 connections by disabling IPV6_V6ONLY
  • Replace ConnectionInfo::GetRemoteAddress with GetRemoteAddressString, as the ip address stored was only used for printing the readable string
  • Fix analyze errors when compiling with TCPCONNECTION_DEBUG defined (TCPDEBUG %i -> %u)

Core/CoreTest/TestNetwork.cpp

  • Add unit tests for Network changes

Core/CoreTest/TestTCPConnectionPool.cpp

  • Refactor test functions to take in ip address so they can be shared
  • Add test functions to connect with ipv6 instead of ipv4

Core/CoreTest/TestMain.cpp

  • Register TestNetwork tests

Tools/FBuild/FBuildCore/Protocol/Server.cpp

  • Update TCPConnectionPool::GetAddressAsString reference

Tools/FBuild/FBuildCore/Protocol/Protocol.h

  • Bump PROTOCOL_VERSION to 22

Checklist

The pull request

  • Is created against the Dev branch
  • Is self-contained
  • Compiles on Windows, OSX and Linux
  • Has accompanying tests
  • Passes existing tests
  • Keeps Windows, OSX and Linux at parity
  • Follows the code style
  • Includes documentation

@Rinn
Copy link
Contributor Author

Rinn commented Jul 13, 2020

It would appear I'm getting different test results on CI than I did locally and there's an error. I just got travis-ci working on my fork, I'll investigate.

@Rinn
Copy link
Contributor Author

Rinn commented Jul 13, 2020

Unfortunately for me it looks like ipv6 is unavailable on Travis CI https://docs.travis-ci.com/user/reference/overview/#virtualisation-environment-vs-operating-system

@Rinn
Copy link
Contributor Author

Rinn commented Jul 14, 2020

I'm still looking into the CI results, a run from my fork passed but it's possible I could have caused an intermittent error.

@Rinn Rinn marked this pull request as draft October 19, 2020 17:05
@Rinn Rinn marked this pull request as ready for review October 19, 2020 21:46
@Rinn
Copy link
Contributor Author

Rinn commented Oct 19, 2020

Okay I've update this to incorporate the changes from #770 (I needed to replace the colons in ipv6 addresses with a different character as they were not valid for a windows filename, I chose a semicolon as that is not a valid character for a hostname). Something to note, the PROTOCOL_VERSION bump could be removed, the downside would be that any newer worker with an ipv6 address would be unconnectable if the client wasn't updated, however any ipv4 connections should continue to work as before.

@Rinn Rinn closed this Nov 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants