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

Lack of IPv6 support ? #89

Closed
caspChristian opened this issue Feb 2, 2023 · 8 comments
Closed

Lack of IPv6 support ? #89

caspChristian opened this issue Feb 2, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@caspChristian
Copy link

caspChristian commented Feb 2, 2023

In the listen service it is forced to udp4, any particular reason for this?
https://github.com/mattermost/rtcd/blob/master/service/rtc/server.go#L117

udpConn, err := listenConfig.ListenPacket(context.Background(), "udp4", listenAddress)

Having "udp" instead of "udp4" should work just as well and be compatible?

I'm no go dev, but a quick read of documentation suggests that ipv6 addresses needs to be escaped with [] to be parsed correctly with the port.
Quick fix would be to check for : in ICEAddressUDP and if so use "[%s]:%d" as the format string instead.

@streamer45 streamer45 added the enhancement New feature or request label Feb 2, 2023
@streamer45
Copy link
Contributor

Hi, indeed we are consciously not supporting IPv6 at the moment.

As you may have guessed, it's far from just changing a string as there's also the underlying WebRTC framework to consider and extensive testing to be done if we ever add support for it.

That said, I created an internal ticket (https://mattermost.atlassian.net/browse/MM-50294) to keep track of this possible improvement.

@caspChristian
Copy link
Author

Not "supporting" is one thing, actively blocking another.
Is there any reason for not allowing rtcd to listen to IPv6? If not, please allow it to be able to test. (that is use "udp" and not "udp4")

All browsers should have perfectly working WebRTC implementations over IPv6 (goole meet working in IPv6 only environments for one)

With RFC8200 published in 2017 IPv6 is the current internet protocol, and IPv4 should be seen as legacy.
Don't make the mistake of thinking that you can ignore IPv6. Some clients, and many servers are even running on IPv6 only these days.

Again, I'm not asking you to make the effort (yet) to test and actively support IPv6. (even if you actually should develop in an IPv6 enabled environment) but what I ask is that you don't actively block IPv6.

@streamer45
Copy link
Contributor

I hear you. All I am saying is that the change you are proposing cannot be done without some planning as we have responsibilities to customers running this software in production environments and any significant change needs to go through a process.

That said, I will bring this up to the team and give an update on this issue as soon as I have one. Thank you for the feedback so far.

@caspChristian
Copy link
Author

I understand being careful of change. But I do not understand why you are using udp4 in the first place, and you have not given any reason for why. Or do you have any case for which it didn't work, but can't share specifics?

What about making that "udp4" into a configuration option? that way it can be tested in production by those who want to?

@streamer45
Copy link
Contributor

We are using udp4 because at the time of implementation that was the only properly working option and the safest default to use (see pion/ice#518 for some context). Changing that now, as I mentioned above, requires some care. And yes, a configuration option would likely be how it gets implemented as I'd still like to default to IPv4 during the initial soaking phase.

By the way, if this is something really urgent please feel free to either fork this repository or send a PR. It's open source after all.

@caspChristian
Copy link
Author

Thanks for the reference. now that I know about the reason for forcing udp4, we have a better startingpoint.

Creating a PR without that info would have been premature ;)

@streamer45
Copy link
Contributor

Closing this as we are shipping experimental IPv6 support in the latest release. Thanks for your patience!

@caspChristian
Copy link
Author

Sounds awesome to hear about experimental support, looking forward to test.
Thank you, and sorry for not being able to provide a PR, other things kept cropping up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants