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

IPv6 support #1017

Closed
wants to merge 1 commit into from
Closed

IPv6 support #1017

wants to merge 1 commit into from

Conversation

jardous
Copy link
Contributor

@jardous jardous commented Feb 18, 2021

This PS add support for IPv6. Note that it is backwards compatible with IPv4.
Motivation is quite simple. I am running a server on AWS and IPv4 ping takes >40ms, but IPv6 ping takes just 15ms! And usability of Jamulus all depends on the ping time.

The difference between IPv4 and IPv6 is so big, that it should be recommended to all users to switch/upgrade to IPv6.

@jardous jardous mentioned this pull request Feb 18, 2021
@ann0see ann0see added this to the Release 3.7.2 milestone Feb 18, 2021
@ann0see
Copy link
Member

ann0see commented Feb 18, 2021

Hi @jardous! Thanks for opening your first PR (argh. Just mis-understood that you did already open one)! I strongly support ipv6 support! IMHO, every modern software should support it ;-).

It seems as if this code fails on Windows. Could you please have a look at it. There's also #722 which you might need to take care of.

@jardous
Copy link
Contributor Author

jardous commented Feb 18, 2021

thanks @ann0see. Unfortunately I am running Linux computers only so I'll just rely on the Windows's CodeQL/Analyze.

@ann0see
Copy link
Member

ann0see commented Feb 18, 2021

Ah. Ok. I assume you saw this error:

socket.cpp
src\socket.cpp(27): fatal error C1083: Cannot open include file: 'arpa/inet.h': No such file or directory

Probably arpa/inet.h doesn't exist on Windows.

@ann0see
Copy link
Member

ann0see commented Feb 18, 2021

@hoffie assigned you here. I hope that's ok for you. Although I know some networking stuff, I assume you're better here.

@pljones
Copy link
Collaborator

pljones commented Feb 18, 2021

What testing has been done with this? I'd expect all of the following, with each on a different network (preferably with multiple hops between each):

IPv4 client (old) <-> IPv4 server (old) <-> IPv4 client (old) ("as is")
IPv4 client (new) <-> IPv4 server (old) <-> IPv4 client (old)

IPv6 client <-> IPv4 server (old) <-> IPv4 client (old)
IPv6 client <-> IPv4 server (new) <-> IPv4 client (old)
IPv6 client <-> IPv4 server (old) <-> IPv4 client (new)
IPv6 client <-> IPv4 server (new) <-> IPv4 client (new)

IPv4 client (old) <-> IPv6 server <-> IPv4 client (old)
IPv4 client (new) <-> IPv6 server <-> IPv4 client (old)
IPv4 client (new) <-> IPv6 server <-> IPv4 client (new)

IPv6 client <-> IPv6 server <-> IPv4 client (old)
IPv6 client <-> IPv6 server <-> IPv4 client (new)

IPv6 client <-> IPv6 server <-> IPv6 client

(edit)... I think that's it.... I wish markdown here support tables...

@jardous
Copy link
Contributor Author

jardous commented Feb 18, 2021

Thanks @pljones for list of test cases.

I will need help from the community to test all of those scenarios as I only have limited setup possibilities.
It is fresh - I have started writing the PS just few hours ago. I have just done some local testing.

BTW the Windows building issue is fixed.

@hoffie
Copy link
Member

hoffie commented Feb 18, 2021

Glad to see this being worked on! Sadly, I currently do not run IPv6 anywhere and will probably not be able to test with v6 right now. Nevertheless, the main point should probably that we don't break anything for existing users. So, I fully agree with what @pljones says regarding Qt and testing.

I want to add that I think there are places in the protocol where IP addresses are shared, e.g. https://github.com/jamulussoftware/jamulus/blob/master/src/protocol.cpp#L327

Some parts are likely related to central server handling, but there may be others. I'm not saying that all of this should be addressed within this first step towards full IPv6 support, but we should probably get an overview and see how things might fail. Such known shortcomings could then be documented somehow.

@gene96817
Copy link

I just reviewed my router configuration and has both IPv4 and IPv6 running with my ISP. I guess I could help with some IPv6 testing but I have never tried it on my private network. Finally, IPv6 is coming to my home. I wonder how many other developers have IPv6 already enable in their routers and waiting to be used.

@pljones
Copy link
Collaborator

pljones commented Feb 18, 2021

Good point on the directory servers - I'd forgotten those... so we'd need four times the test cases again, then thin out the duplicates (i.e. all of the above with unregistered server, all with old code directory server, all with new code IPv4, all with new code IPv6 -- hm, and maybe doubled for the last three of those for server and client cases...).

@ghost
Copy link

ghost commented Feb 18, 2021

Maybe there should be a build option for ipv6... And a build option for ipv4 so that there are choices for all. The amount of code changed looks to be small enough to use #ifdef. This would probably be the fastest way forward. Note that I do not have ipv6, and will probably not have ipv6 untill the USA goes metric.

@jardous
Copy link
Contributor Author

jardous commented Feb 18, 2021

@DavidSavinkoff I would prefer to change it so it supports both. I'll take look - give me some time.

@atsampson
Copy link
Contributor

Another case that will need dealing with is when you're trying to connect to a server that provides both IPv4 and IPv6 (presumably the directory servers would do this in an IPv6 future), but the local network provides only IPv4 - or only IPv6 - or both. It's entirely possible (and quite common on current networks) to have a local IPv6 address but not have connectivity to public IPv6.

There's a standard way of dealing with this: the splendidly named Happy Eyeballs. But it's not trivial to implement, and ideally we'd make use of existing facilities to do the job if possible. I'd agree with @pljones that using Qt's socket classes is likely to be a better bet than trying to extend the custom ones Jamulus currently uses.

And then there's all the fun with IPv6 and DNS - e.g. you can have users behind a DNS64 setup, where A records (containing IPv4 addresses) are automatically translated into AAAA records pointing at a NAT64 server. So you'd have local Jamulus thinking it's connected by IPv6, but actually talking through a proxy to an IPv4 server...

@gene96817
Copy link

And then there's all the fun with IPv6 and DNS - e.g. you can have users behind a DNS64 setup, where A records (containing IPv4 addresses) are automatically translated into AAAA records pointing at a NAT64 server. So you'd have local Jamulus thinking it's connected by IPv6, but actually talking through a proxy to an IPv4 server...

There are plenty of mistakes being made with IPv6. Eventually, implementations will automatically work properly without users needing to know about IPv4 or IPv6. We will be making mistakes, too, until it just works.

@jardous
Copy link
Contributor Author

jardous commented Feb 19, 2021

server list fetch fixed

@jardous jardous force-pushed the ipv6support branch 3 times, most recently from 385738e to e361261 Compare February 19, 2021 11:14
@bjacke
Copy link

bjacke commented Feb 19, 2021

And then there's all the fun with IPv6 and DNS - e.g. you can have users behind a DNS64 setup, where A records (containing IPv4 addresses) are automatically translated into AAAA records pointing at a NAT64 server. So you'd have local Jamulus thinking it's connected by IPv6, but actually talking through a proxy to an IPv4 server...

but whatever IPv6 connectivity is being used there, this should be transparent for userspace applications. I think checking the Jamulus protocol ping time both via v4 and v6 should be checked and the better latency connection being used then.

For those of you who don't have IPv6 at home yet, at https://www.tunnelbroker.net/ I think it's still possible to get ipv6 tunnels, I used them for a long time, very much recommended and helpful for testing things here.

@jarmar
Copy link
Contributor

jarmar commented Feb 19, 2021

I looked into adding IPv6 support last year, but didn't finish it when I realized how much work it would be, and that I could work around it on my end instead.

At that point, I investigated what protocol messages include IPv4 addresses. I found:

  • PROTMESSID_CLM_REGISTER_SERVER/_EX, PROTMESSID_CLM_SERVER_LIST, PROTMESSID_CLM_RED_SERVER_LIST, PROTMESSID_CLM_SEND_EMPTY_MESSAGE: all related to the server list.
  • PROTMESSID_CONN_CLIENTS_LIST, PROTMESSID_CLM_CONN_CLIENTS_LIST: these messages have fields for an address, but since commit e7621af, they are always dummy addresses.

So my impression was that making IPv6 work everywhere except in the server list should be "simple", in the sense that it should be possible to do transparently and without modifying the protocol. But making the server list IPv6-aware would require a lot more up-front design.

src/socket.cpp Show resolved Hide resolved
0,
(sockaddr*) &UdpSocketOutAddr,
sizeof ( sockaddr_in ) );
if (HostAddr.InetAddr.protocol() == QAbstractSocket::IPv4Protocol) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this necessary? toIPv6Address() will convert IPv4 addresses to IPv6 mapped, so I believe it should be enough to only handle the IPv6 case.

@gilgongo gilgongo linked an issue Feb 19, 2021 that may be closed by this pull request
@jardous
Copy link
Contributor Author

jardous commented Feb 19, 2021

Hello guys,

I have just tested the changes as follows.

My network setup is:
PC (Linux, IPv4 + NAT, IPv6) -> Router (port forwarding to the PC's IPv6 address) -> IPv6 network (UnityMedia) -> AWS Debian (Frankfurt, static IPv4, IPv6)

Following scenarios:

  1. server (old, IPv4) + client (new, connect to IPv4 address) --> PASS (ping >60ms, overall delay >80ms)
  2. server (new, IPv4+IPv6) + client(new, connect to IPv6 address) --> PASS (ping 15ms, overall delay 35ms)
  3. server (new, IPv4+IPv6) + client(new, connect to IPv4 address) --> PASS (ping 60ms, overall delay 80ms)
  4. local server (new, IPv4+IPv6) + client(new, connect to ::1) --> PASS (ping 0ms, overall delay 13ms)

@jardous jardous force-pushed the ipv6support branch 3 times, most recently from e3993b2 to 45b4870 Compare February 19, 2021 22:09
@pljones
Copy link
Collaborator

pljones commented Feb 23, 2021

Part of the reason the PR hasn't been accepted is that it's not been shown to work transparently with the directory servers. Let's leave it until the code exists to worry about whether it works.

@bjacke
Copy link

bjacke commented Feb 23, 2021

you might add IPv6 support to the directory server now, your provider is Linode and they support IPv6 on all hosts.

@bjacke
Copy link

bjacke commented Feb 23, 2021

it would also be nice if the WELL_KNOWN_HOST 1.1.1.1 check would be removed from the code so that you have no IPv4 connectivity requirement there.

@pljones
Copy link
Collaborator

pljones commented Feb 23, 2021

it would also be nice if the WELL_KNOWN_HOST 1.1.1.1 check would be removed from the code so that you have no IPv4 connectivity requirement there.

We have the same requirement for the public interface address, regardless of whether it's IPv4 or IPv6 - or doesn't IPv6 addressing provide a separate address per endpoint?

@bjacke
Copy link

bjacke commented Feb 23, 2021

We have the same requirement for the public interface address, regardless of whether it's IPv4 or IPv6 - or doesn't IPv6 addressing provide a separate address per endpoint?

ah, sorry, I didn't realize that this is the magic for finding out the outgoing IP(v4) address.

@hoffie
Copy link
Member

hoffie commented Feb 23, 2021

@pljones Thanks. That is an impressive table. I think we should narrow it down to check if we do not break compatibility. At least for the first iteration so we have the feature in.
For those who know, and can run an IPv6 server, would that be a nice "extra" and they can help with testing.

I also think it would be a valid approach to add basic, optional IPv6 support in the first iteration (i.e. no IPv6 by default, no support for central servers with IPv6). This would help people who do have better IPv6 connectivity than IPv4, it would help to get an idea what works/breaks and it would help getting the feature in while still ensuring that we don't break anything.

For full support I agree with @pljones that lots of things should be validated.

@hoffie hoffie removed their assignment Apr 10, 2021
@hoffie hoffie removed this from the Release 3.7.1 milestone Apr 10, 2021
@Hk1020
Copy link
Contributor

Hk1020 commented Apr 25, 2021

A test I just did but with #1455, which ported to recent master on a private fork.

I have a server running with both ipv4 and ipv6 and a A+AAAA dns record. Connecting using the dns name establishes a ipv6 connection but connecting via the central server selects ipv4. So it looks like the client gets the IP address from the central server.

@pljones
Copy link
Collaborator

pljones commented Apr 26, 2021

The purpose of the Jamulus directory servers is to provide directory entry to IP/port mappings, keeping those details refreshed and keeping firewalls open by sending ping messages. As nothing has been done to the directory servers to add IPv6 support, it's no surprise that using them gets an IPv4 address.

@Hk1020
Copy link
Contributor

Hk1020 commented Apr 26, 2021

So the client does not do a dns lookup of the server it wants to connect to. This is unlike to what happens with entering the server name in the little connect box. From there connection to ipV6 servers seems to work fine with #1455. I could not get anything working with this PR. And with server I mean the actual Jamulus server, not the directory server.

@pljones
Copy link
Collaborator

pljones commented Apr 26, 2021

The client will do a DNS lookup if one is needed (because it passes the values to Qt to deal with -- a dotted quad "resolves" without any look up).

#1455 provides fallback in the case where IPv6 is not available - unless you tell it explicitly not to (with --ip6only).

This change currently removes IPv4 support. So unless you're connecting a client and server built from this branch, you cannot expect it to work.

@gilgongo gilgongo mentioned this pull request Jun 12, 2021
4 tasks
@pljones
Copy link
Collaborator

pljones commented Jun 19, 2021

Would we be better closing this and concentrating on #1455? That also needs further work, of course, but it makes no sense to have two open PRs for IPv6.

@gene96817
Copy link

I don't understand why https://github.com/jamulussoftware/jamulus/pull/1455 is so dependent on command flags. It should be straightforward to automate the dynamic selection of with IPv6 or IPv4.

@pljones
Copy link
Collaborator

pljones commented Jun 20, 2021

I don't understand why https://github.com/jamulussoftware/jamulus/pull/1455 is so dependent on command flags. It should be straightforward to automate the dynamic selection of with IPv6 or IPv4.

Probably better raised there.

@ghost
Copy link

ghost commented Jun 20, 2021

I don't understand why https://github.com/jamulussoftware/jamulus/pull/1455 is so dependent on command flags. It should be straightforward to automate the dynamic selection of with IPv6 or IPv4.

Some flags will be needed because 'automatic' doesn't work as it should.

IPv6 has the same problems as IPv4 by not acknowleging that the other version exists.
This theoretically should only be a IPv4 problem because IPv6 has a chance to correct the problem. IPv6 is so stupid that when IPvX happens, IPv6 will be in the same shoes as IPv4 was. This problem is in operating systems too. This is why gai.conf has to be manually tweeked.

@gene96817
Copy link

IPv6 has the same problems as IPv4 by not acknowleging that the other version exists.

I apologize for asking a very simple question. Are you saying that the following startup procedure will not work?

Initialize an IPv6 connection with the server...
on IPv6 timeout, abandon IPv6 and initialize an IPv4 connection
on IPv4 timeout,

Seems to me, all servers support IPv4s, and ervers with the new code will support IPv6 but they may not have an IPv6 connection. If there is no response to an IPv6 initiation, we fallback to the (universally available) IPv4. If we can make this process work then the Jamulus world will automatically migrate to IPv6 without additional intervention.

@ghost
Copy link

ghost commented Jun 20, 2021

IPv6 has the same problems as IPv4 by not acknowleging that the other version exists.

I apologize for asking a very simple question. Are you saying that the following startup procedure will not work?

Initialize an IPv6 connection with the server...
on IPv6 timeout, abandon IPv6 and initialize an IPv4 connection
on IPv4 timeout,

Seems to me, all servers support IPv4s, and ervers with the new code will support IPv6 but they may not have an IPv6 connection. If there is no response to an IPv6 initiation, we fallback to the (universally available) IPv4. If we can make this process work then the Jamulus world will automatically migrate to IPv6 without additional intervention.

This is exactly the problem. There shouldn't be timeouts for either IPv4 or IPv6. That is why you need command line options, and gai.conf modifications.
The time outs and fallbacks are poor design baked into IPv6 deployment. One would think that IPv6 would have solved this IPv4 problem. This is probably why IPv6 is so slow to be accepted.

@gene96817
Copy link

gene96817 commented Jun 20, 2021

There are lots of reasons that make IPv6 slow to adopt. My first IPv6 product rollout was in the 1990s... go figure.
Timeouts and fallbacks are hacks. Still, it is a pragmatic way to deal with the lack of version negotiations in IPv4 and IPv6 start-up procedures. Good implementation is automatic and permissive. Why inflict the deficiencies of IPv4/IPv6 on every Jamulus installation? Automagical functions are wonderful (as long as they are not buggy).

I do understand. Some people still prefer to drive cars with a stick shift (manual) transmission.

@ann0see
Copy link
Member

ann0see commented Aug 8, 2021

@ALL here. Please have a look at #1938 by @softins for review.

@softins
Copy link
Member

softins commented Aug 10, 2021

Closing this as it has been superseded by #1938

@softins softins closed this Aug 10, 2021
@pljones pljones added this to the Release 3.8.1 milestone Feb 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for IPv6