IPv6 support, phase 1 (direct connection) #1938

softins · 2021-08-02T15:02:35Z

This implementation of IPv6 on Jamulus is based on #1017 by @jardous, with further enhancements by @softins. It is an alternative to #1455 by @mstolle629.

It has been tested on Linux, Mac and Windows. It is currently running on jamminthejazz, at privjam.softins.co.uk:22123 (IPv4) and privjam6.softins.co.uk:22123 (IPv6). There is also a private instance at privjam.softins.co.uk and privjam6.softins.co.uk, which may be used for testing.

Servers running this version can accept a mix of IPv4 and IPv6 connections.

Server addresses for direct connection can be specified in the following formats:

[v6addr]
[v6addr]:port
[v4addr]
[v4addr]:port
v4addr
v4addr:port
hostname
hostname:port

A literal IPv6 address must always be enclosed in [ ], to avoid ambiguity with a port.

Phase 2 will be the addition of Directory Server support, which will require some
protocol enhancements. Phase 1 is worth adding on its own before Phase 2 is ready.

Potential ChangeLog entry

Add support for direct connections using IPv6

Documentation needed

Command line options -6 and --enableipv6
Address formats as shown above

Status of this Pull Request

Working implementation

What is missing until this pull request can be merged?

More testing is always good!

Checklist

I've verified that this Pull Request follows the general code principles
I tested my code and it does what I want
My code follows the style guide
I waited some time after this Pull Request was opened and all GitHub checks completed without errors.
I've filled all the content above

src/socket.cpp

pljones · 2021-08-02T17:56:15Z

src/client.cpp

@@ -31,6 +31,7 @@ CClient::CClient ( const quint16  iPortNumber,
                   const QString& strMIDISetup,
                   const bool     bNoAutoJackConnect,
                   const QString& strNClientName,
+                   const bool     bNEnableIPv6,


It doesn't need the ugly N, generally.

Was just following what appeared to be the majority convention. No strong view either way.

I'm not going to block it for that, either. (It's just on my list of things to get rid of.)

pljones · 2021-08-02T18:01:30Z

Um... Given IPv4 is always enabled and given that only IPv4 is used to communicate with directory servers (for now), is there any reason not to have IPv6 always enabled?

softins · 2021-08-02T18:42:01Z

I wondered that. It would fail on a system that didn't have an IPv6 stack. Such as a Linux box with a file containing "blacklist ipv6" in /etc/modprobe.d/ (I did test that on my Pi). We could try creating an AF_INET6 socket first and falling back to AF_INET, but would then need to propagate the result of that to a global variable that all classes could consult.

pljones · 2021-08-02T19:51:27Z

Well, rather than set bEnableIPv6 in main.cpp based on a command line parameter, set it based on the result of the check, leaving the rest the same? (More bIPv6Available, in other words.) Then it could be invisible to the user.

softins · 2021-08-02T22:14:21Z

I did try something like that earlier, but found that it wasn't easy to pass the result of the test back up from the CSocket, given that the client and server sockets are not newed, but instantiated as members of CClient or CServer.

I could perhaps create a static method of CSocket that just tries to create a temporary AF_INET6 socket and returns a success indication, and call that from main.cpp

But we also need to consider the implications for:

A client on a computer that is IPv6-capable, but connected via an internet connection that does not support IPv6
A server that supports IPv6 and has a routable IPv6 address, but the operator does not want their Jamulus server to listen on IPv6, for whatever reason.

pljones · 2021-08-03T06:15:11Z

You've given reasons to switch IPv6 off. Maybe the command line parameter should do that?

I'm thinking of other software with internet connectivity -- I don't have to tell my web browser or email client whether or not to support IPv6, for example.

ann0see · 2021-08-03T12:34:51Z

A client on a computer that is IPv6-capable, but connected via an internet connection that does not support IPv6

Best case if host name is given: try to request an A record via DNS. Use ipv4 as fallback. But does Jamulus need to handle that? Isn’t that an OS functionality? Not only Jamulus will have this problem?!

If I‘m giving an ipv6 address it should fail and not try to get a connection over ipv4.

How does Firefox/Chrome/whatever handle this?

Edit see this: https://blog.counterpath.com/ipv6

ann0see · 2021-08-03T12:35:56Z

A server that supports IPv6 and has a routable IPv6 address, but the operator does not want their Jamulus server to listen on IPv6, for whatever reason.

Use the bind IP option and only provide an ipv6/ipv4 address?

atsampson · 2021-08-03T13:36:02Z

How does Firefox/Chrome/whatever handle this?

Happy Eyeballs.

Firefox does have a configuration option to disable IPv6 entirely to speed things up a bit if you know you're on a network without external IPv6 connectivity (it's still not that unusual to have local IPv6 but not routable to the world).

softins · 2021-08-03T14:05:21Z

How does Firefox/Chrome/whatever handle this?

Happy Eyeballs.

Certainly very interesting, but quite complex, and I feel it's likely to be overkill for Jamulus. It's primarily concerned with resolving DNS names using both AAAA and A records, and then trying to connect with the returned addresses, preferring IPv6. In Jamulus, an operator of a private server using IPv6 will likely give his invitees a literal address or have a specific AAAA record for it. And once we extend to directory servers, they will return to the client literal IPv6 (if any) and IPv4 addresses, not DNS names.

softins · 2021-08-03T14:09:01Z

You've given reasons to switch IPv6 off. Maybe the command line parameter should do that?

Maybe, but I think the number of people with an IPv6-capable computer but an IPv4-only internet connection would be a lot larger than the number of people who specifically want to use IPv6. So we would be requiring the much larger number of people to do something different.

I would prefer to have the new IPv6 functionality be optional in the first one or two releases that contain it, and once we have gained more experience with it, we could make it the default.

ann0see · 2021-08-03T14:52:45Z

In Jamulus, an operator of a private server using IPv6 will likely give his invitees a literal address or have a specific AAAA record for it.

What makes you sure about that?

From a UX point of view, I think just giving one address (with AAAA and A records) is the preferable outcome. Maybe - but for debugging purposes only - the admin sends over an IP to force ipv6/ipv4. That’s just my opinion…

For directories we‘re skipping DNS?

Can we send ping messages over IPv6 and IPv4, and constantly calculate the time difference between both as soon as the connection window is open? The client will then know which one to choose before connecting.

Issues:

Worst case: double the ping traffic
Complexity on the client side
Constant calculation might not be necessary. We might stop after a few ping messages.

Pro:

Probably similar to "Happy eyeballs"
User doesn‘t need to decide in normal mode, but could still use the direct IP to connect to the server

ann0see · 2021-08-03T15:10:05Z

So -6 is a client and server option?

Tested ipv4 and ipv6 over a dual stack VDSL line. To me it seems as if ipv4 has a little better connectivity in terms of latency. But probably this varies depending on the provider.

softins · 2021-08-03T15:51:22Z

So -6 is a client and server option?

Yes.

softins · 2021-08-03T15:55:51Z

In Jamulus, an operator of a private server using IPv6 will likely give his invitees a literal address or have a specific AAAA record for it.

What makes you sure about that?

I'm not sure, it just seems likely to me. Most people I know with private servers haven't set up a DNS name and just give their friends the IP. There are a few more technical ones who have set up a DNS name.

For directories we‘re skipping DNS?

We can use a DNS name to find a directory, but when a server registers with a directory, it does so just by IP (the directory stores the IP it registered from). The directory then tells a requesting client just the server's IP, not a DNS name, plus other relevant information (name, private IP, port number, etc).

Can we send ping messages over IPv6 and IPv4, and constantly calculate the time difference between both as soon as the connection window is open? The client will then know which one to choose before connecting.

That might well be a useful enhancement for the future, but I don't think we should hold up the basic feature waiting for that particular lily to be gilded.

ann0see · 2021-08-06T06:53:31Z

Ok. So what really needs to be tested to get this merged?

IPv6/IPv4 over a dual stack connection seems to work.

softins · 2021-08-06T13:08:00Z

Ok. So what really needs to be tested to get this merged?

Well I've tested it quite a bit in my environment, on Linux, Windows and Mac, and it's running on my server as mentioned above. I think it's good to go, and that it is beneficial to have -6 to explicitly turn it on for now. Then others who want to try out IPv6 could do so, without it having unexpected impacts on those who don't.

So it just needs two approvals (which can be tricky), and it could be merged. I would then start thinking about Directory Servers.

IPv6/IPv4 over a dual stack connection seems to work.

Cool!

pljones · 2021-08-06T17:25:36Z

it is beneficial to have -6 to explicitly turn it on for now.

Agreed for the "experimental support" version.

If the short version is going to change to mean --disable-ipv6, then that needs to be made very clear. Or maybe for now don't have the short version, just --enable-ipv6?

pljones · 2021-08-06T17:41:13Z

src/socket.cpp

+
+        UdpPort = &UdpSocketAddr.sa6.sin6_port; // where to put the port number
+
+        // FIXME: If binding a dual-protocol interface to a specific address, does it cease to be dual-protocol?


A socket listening on an IPv4 address is only listening for IPv4 traffic. But the same interface could have a separate socket listening on an IPv6 address, I believe. The interface has the MAC address (or equivalent). The protocol has the IPv4 or IPv6 address that goes with the socket layer in use. (That's why the union is used for the address: there can be only one - on that socket.)

At least, that's my understanding.

A socket listening on an IPv4 address is only listening for IPv4 traffic. But the same interface could have a separate socket listening on an IPv6 address, I believe.

Yes it could, but Jamulus only expects to listen on one socket. So I think that makes it unable to bind to a specific interface address and remain dual-stack. I don't think it's a problem, unless someone flags up a scenario where it is. The default of binding to :: will allow it to receive both IPv4 and IPv6 traffic without needing two sockets.

Yes it could, but Jamulus only expects to listen on one socket.
The default of binding to :: will allow it to receive both IPv4 and IPv6 traffic without needing two sockets.

How does an IPv4 client know what address to use? Does the IPv6 socket "just know" it's got two addresses really? (I've some vague memory that every IPv4 address has an IPv6 address direct equivalent...)

The IPv4 client will connect in the same way as at present, by being told the IP address, either explicitly, or from a hostname lookup, or by a directory server.

Binding an AF_INET6 socket to :: (declared as in6addr_any) is the IPv6 equivalent of binding an AF_INET socket to 0.0.0.0 (INADDR_ANY), which means it will accept any incoming packet that arrives on any interface. If the socket has been setsockopted to dual stack (as we do), it will accept both IPv4 and IPv6 packets.

An IPv4 address mapped to IPv6 uses 0000:0000:0000:0000:0000:ffff as the first 96 bits, which denotes that the last 32 bits contain an IPv4 address in network byte order. Often expressed like ::ffff:192.168.1.1. This mapping is used in CSocket::SendPacket() and checked for in CSocket::OnDataReceived().

pljones · 2021-08-06T17:47:21Z

src/socket.cpp

@@ -172,6 +232,15 @@ CSocket::~CSocket()

 void CSocket::SendPacket ( const CVector<uint8_t>& vecbySendBuf, const CHostAddress& HostAddr )
 {
+    union


Can this go in a header somewhere as a type, so it can be re-used more easily? socket.h?

I guess, although that makes it more opaque when reading socket.cpp.

You could argue that for every definition that isn't in line.

ann0see

Seems simple. Nevertheless some comments:

You're removing the chinese .qm file? Any reason for that?
See comments
As it works on my ipv6 connection (and you tested it), I think the functionality is ok.

ann0see · 2021-08-08T10:49:31Z

src/main.cpp

@@ -937,6 +955,7 @@ QString UsageArguments ( char** argv )
           "  -Q, --qos             set the QoS value. Default is 128. Disable with 0\n"
           "                        (see the Jamulus website to enable QoS on Windows)\n"
           "  -t, --notranslation   disable translation (use English language)\n"
+           "  -6, --enableipv6      enable IPv6 addressing (IPv4 is always enabled)\n"


Suggested change

" -6, --enableipv6 enable IPv6 addressing (IPv4 is always enabled)\n"

" -6, --enableipv6 enable IPv6 addressing for direct connection to client/server (IPv4 is always enabled).\n"

I don't think this adds anything at all. It still requires further explanation as to what "direct connection to client/server" means, so it's useless verbosity.

But do we agree that the original proposal needs more clarification?

I'm happy with the wording Tony used. It's concise and says what it does.

I still think it's not clear. Probably the question is:
Does the user need to know that connections to public servers is still IPv4 only?
If we want to be specific: Yes. If we think it confuses the user: No.
Maybe it's worth mentioning it at least in the release announcement.

Does the user need to know that connections to public servers is still IPv4 only?

Not "public", use the word "registered". You can register with a directory no one knows about. That's not particularly public.

To operate the software, no. They can just select a listed entry from the directory and the software will use the returned IPv4 address.

If you want to use IPv6, you need to:
a) pass "-6" on the command line, thus indicating you understand how to operate with IPv6 addresses, so need no further help
b) enter an IPv6 name or address in the connection dialogue "Server Address", by hand, so you know that you've chosen a specific server rather than a registered one.

As I say, the help text need not say anything else. This is not the final form for the feature. It's intended to be somewhat experimental and the documentation should make that clear. Those who choose to use it should thus be given the credit for understanding what they've chosen to do.

src/serverlist.cpp

ann0see · 2021-08-08T10:54:28Z

src/socket.cpp

+        if ( UdpSocket == -1 )
+        {
+            // IPv6 requested but not available, throw error
+            throw CGenErr ( "IPv6 requested but not available on this system.", "Network Error" );


Suggested change

throw CGenErr ( "IPv6 requested but not available on this system.", "Network Error" );

throw CGenErr ( "You requested IPv6, but it's not available with your system configuration.", "Network Error" );

As far as I know active voice is preferred to passive voice (https://jamulus.io/contribute/Style-and-Tone)

I can't see any reference to active vs passive voice in that page, although I know some other wider contexts promote a preference for active voice. In this case, I have a slight preference for the shorter message.

I think @gilgongo mentioned it somewhere.

Maybe I halluciated it then - I recall @mulyaj mentioning it.

Yeah, I've mentioned a preference for active voice. Reading this at first glance, the subject is vague:

"IPv6 requested..." - who requested?

Then, if the subject is vague, the guideline for concise and specific is broken.

I'd be happy with "IPv6 is not available." It's issued as a response to the user making a request to use an address. It doesn't need to tell the user they used an address, just why it hasn't worked.

ann0see · 2021-08-08T10:57:11Z

src/socket.cpp

+        if ( UdpSocket == -1 )
+        {
+            // IPv4 requested but not available, throw error (should never happen, but check anyway)
+            throw CGenErr ( "IPv4 requested but not available on this system.", "Network Error" );


As above, but now with ipv4 ofc.

src/socket.cpp

src/util.cpp

ann0see · 2021-08-08T11:11:59Z

Since it's not enabled by default, I think we can tag it for 3.8.1.

softins · 2021-08-08T11:13:58Z

Seems simple. Nevertheless some comments:

You're removing the chinese .qm file? Any reason for that?

No, must be an accident. I'll correct that before merging.

See comments

Thanks, will respond individually.

As it works on my ipv6 connection (and you tested it), I think the functionality is ok.

Cool.

ann0see · 2021-08-09T11:02:30Z

As soon as the Chinese qm file is added, I think it’s ready to merge.

ann0see

Since it works and only one critical step is missing (.qm file), I‘ll approve it.

softins · 2021-08-09T21:09:14Z

I have squashed all the commits except for the first one by @jardous. I'll wait until the checks are complete (looks like they may need to be rerun) and merge tomorrow.

ann0see · 2021-08-09T21:27:58Z

I think you should use @jardous E-Mail or jardous@users.noreply.github.com to link his GitHub account to the first commit. This would add him to the contributor entry on GitHub.

softins · 2021-08-09T22:20:15Z

I think you should use @jardous E-Mail or jardous@users.noreply.github.com to link his GitHub account to the first commit. This would add him to the contributor entry on GitHub.

Not exactly sure how to do that.

pljones · 2021-08-10T06:35:48Z

I think you should use @jardous E-Mail or jardous@users.noreply.github.com to link his GitHub account to the first commit. This would add him to the contributor entry on GitHub.

Not exactly sure how to do that.

To set the values:

git commit --author="Jiri Popek <jardous@users.noreply.github.com>" --date="2021-02-18T12:34:56" ...

You'd need to do that during either a git rebase -i or manually git reset --hard master and reapplying the commits.

Note that it is backwards compatible with IPv4. Co-authored-by: Tony Mountifield <tony@mountifield.org>

Reworked ParseNetworkAddress() to use regex to validate format. Allows literal IP address (IP4 or IP6) between [ ], or else IP4 address or hostname without [ ]. Optional :port follows. Add code to determine routable IPv6 interface address. Uses Cloudflare's IPv6 DNs resolver address (no traffic sent). Use LocalHostIPv6 if no local IPv6 address Add command option and flag for enabling IPv6 Reworked sockaddr handling in OnDataReceived() Change in_port_t to uint16_t for Windows compatibility Allow IPv4 only for communicating with Directory Servers Rework dual-stack socket handling for Windows compatibility Move sockaddr union definition into socket.h

softins · 2021-08-10T14:41:51Z

I think you should use @jardous E-Mail or jardous@users.noreply.github.com to link his GitHub account to the first commit. This would add him to the contributor entry on GitHub.

OK, that seems to have worked. Just waiting for the Checks to run and then I'll merge.

softins · 2021-08-10T16:36:51Z

Well although Android and Windows failed this time, they have previously worked with identical code (I diffed the before and after of the rebases to be sure), so I'll go ahead and merge anyway.

jardous · 2021-08-10T16:45:21Z

hi. Feel free to use my real email: ***@***.*** J.

…

On Tue 10. Aug 2021 at 16:42, Tony Mountifield ***@***.***> wrote: I think you should use @jardous <https://github.com/jardous> E-Mail or ***@***.*** to link his GitHub account to the first commit. This would add him to the contributor entry on GitHub. OK, that seems to have worked. Just waiting for the Checks to run and then I'll merge. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1938 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACU74J3TCBUEETO47F6EY3T4E3DTANCNFSM5BM5NUNA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

softins · 2021-08-10T16:48:35Z

hi. Feel free to use my real email: @.*** J.

I already used your generic Github address jardous@users.noreply.github.com, which was enough to link the rebased version of your original commit to your Github ID. Thanks!

softins · 2021-08-10T18:13:58Z

@ALL, please see #1950 for discussion of the technical approach to adding IPv6 support in Directory Servers.

atsampson reviewed Aug 2, 2021

View reviewed changes

src/socket.cpp Outdated Show resolved Hide resolved

atsampson reviewed Aug 2, 2021

View reviewed changes

src/socket.cpp Outdated Show resolved Hide resolved

atsampson reviewed Aug 2, 2021

View reviewed changes

src/socket.cpp Show resolved Hide resolved

pljones reviewed Aug 2, 2021

View reviewed changes

pljones reviewed Aug 6, 2021

View reviewed changes

softins requested a review from pljones August 6, 2021 22:38

pljones approved these changes Aug 8, 2021

View reviewed changes

softins requested a review from ann0see August 8, 2021 10:42

ann0see reviewed Aug 8, 2021

View reviewed changes

ann0see mentioned this pull request Aug 8, 2021

IPv6 support #1017

Closed

ann0see mentioned this pull request Aug 8, 2021

Add support for IPv6 #46

Closed

ann0see added this to the Release 3.8.1 milestone Aug 8, 2021

ann0see mentioned this pull request Aug 8, 2021

Do not fail silently if IP in connection window is invalid #1945

Open

ann0see approved these changes Aug 9, 2021

View reviewed changes

softins force-pushed the ipv6 branch 2 times, most recently from 57fbd63 to 611e11a Compare August 9, 2021 21:02

jardous and others added 2 commits August 10, 2021 15:35

IPv6 support

9968e41

Note that it is backwards compatible with IPv4. Co-authored-by: Tony Mountifield <tony@mountifield.org>

softins force-pushed the ipv6 branch from 611e11a to 576d101 Compare August 10, 2021 14:38

softins merged commit 694473a into jamulussoftware:master Aug 10, 2021

softins mentioned this pull request Aug 10, 2021

Basic IPv6 support for Jamulus #1455

Closed

softins deleted the ipv6 branch August 16, 2021 10:18

jp8 mentioned this pull request Aug 27, 2021

Add a gui button and entry in ini file to enable/disable basic IPv6 support (client and server) #1973

Open

ann0see mentioned this pull request Sep 5, 2021

iOS sound support #1875

Merged

5 tasks


		UdpPort = &UdpSocketAddr.sa6.sin6_port; // where to put the port number

		// FIXME: If binding a dual-protocol interface to a specific address, does it cease to be dual-protocol?

	" -6, --enableipv6 enable IPv6 addressing (IPv4 is always enabled)\n"
	" -6, --enableipv6 enable IPv6 addressing for direct connection to client/server (IPv4 is always enabled).\n"

	throw CGenErr ( "IPv6 requested but not available on this system.", "Network Error" );
	throw CGenErr ( "You requested IPv6, but it's not available with your system configuration.", "Network Error" );

IPv6 support, phase 1 (direct connection) #1938

IPv6 support, phase 1 (direct connection) #1938

Conversation

softins commented Aug 2, 2021 • edited Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones commented Aug 2, 2021 • edited Loading

softins commented Aug 2, 2021 • edited Loading

pljones commented Aug 2, 2021 • edited Loading

softins commented Aug 2, 2021 • edited Loading

pljones commented Aug 3, 2021

ann0see commented Aug 3, 2021 • edited Loading

ann0see commented Aug 3, 2021

atsampson commented Aug 3, 2021

softins commented Aug 3, 2021

softins commented Aug 3, 2021 • edited Loading

ann0see commented Aug 3, 2021

ann0see commented Aug 3, 2021 • edited Loading

softins commented Aug 3, 2021

softins commented Aug 3, 2021 • edited Loading

ann0see commented Aug 6, 2021

softins commented Aug 6, 2021

pljones commented Aug 6, 2021 • edited Loading

pljones Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones Aug 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones Aug 6, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pljones Aug 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ann0see commented Aug 8, 2021

softins commented Aug 8, 2021

ann0see commented Aug 9, 2021

ann0see left a comment

Choose a reason for hiding this comment

softins commented Aug 9, 2021

ann0see commented Aug 9, 2021

softins commented Aug 9, 2021

pljones commented Aug 10, 2021

softins commented Aug 10, 2021

softins commented Aug 10, 2021

jardous commented Aug 10, 2021 via email

softins commented Aug 10, 2021

softins commented Aug 10, 2021

softins commented Aug 2, 2021 •

edited

Loading

pljones commented Aug 2, 2021 •

edited

Loading

softins commented Aug 2, 2021 •

edited

Loading

pljones commented Aug 2, 2021 •

edited

Loading

softins commented Aug 2, 2021 •

edited

Loading

ann0see commented Aug 3, 2021 •

edited

Loading

softins commented Aug 3, 2021 •

edited

Loading

ann0see commented Aug 3, 2021 •

edited

Loading

softins commented Aug 3, 2021 •

edited

Loading

pljones commented Aug 6, 2021 •

edited

Loading

pljones Aug 6, 2021 •

edited

Loading

pljones Aug 7, 2021 •

edited

Loading

pljones Aug 6, 2021 •

edited

Loading

ann0see left a comment •

edited

Loading

pljones Aug 9, 2021 •

edited

Loading