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

Default config: listen on IPv6 for the swarm address #1076

Merged
merged 1 commit into from
Aug 15, 2015

Conversation

zorun
Copy link

@zorun zorun commented Apr 14, 2015

No description provided.

@jbenet jbenet added ready and removed ready labels Apr 14, 2015
@jbenet
Copy link
Member

jbenet commented Apr 14, 2015

@zorun have you tested this successfully? Im not sure if this will collide on the TCP port given how we're doing listening (would be separate calls to Listen (bind))

@jbenet
Copy link
Member

jbenet commented Apr 14, 2015

And thanks! i'd be in favor of this 👍 :)

@zorun
Copy link
Author

zorun commented Apr 14, 2015

Yes, it does work, at least on Linux (Debian and Archlinux):

tcp        0      0 0.0.0.0:4001            0.0.0.0:*               LISTEN      1000       4079977    9765/ipfs           
tcp6       0      0 :::4001                 :::*                    LISTEN      1000       4079978    9765/ipfs           

Actually, there seems to be some clever code somewhere: IPFS (or Go itself) seems to activate IPV6_V6ONLY (see http://tools.ietf.org/html/rfc3493#section-5.3 ) whenever the IPv4 port is already bound.

When listening on IPv6 only (by setting Addresses.Swarm to ["/ip6/::/tcp/4001"]), IPFS is still able to talk to IPv4 peers (which is expected on Linux, see http://tools.ietf.org/html/rfc3493#section-3.7 ).

tcp        0      0 89.234.X.Y:43208    162.243.X.Y:4001    ESTABLISHED 1000       5550994    13362/ipfs          
tcp        0      0 89.234.X.Y:38602    50.137.X.Y:55032    ESTABLISHED 1000       5575210    13362/ipfs          
tcp6       0      0 :::4001                 :::*                    LISTEN      1000       5551279    13362/ipfs          
tcp6       0      0 89.234.X.Y:4001     31.18.X.Y:4001      ESTABLISHED 1000       5595203    13362/ipfs          
tcp6       0      0 89.234.X.Y:4001     80.121.X.Y:1024      ESTABLISHED 1000       5570954    13362/ipfs          

@zorun
Copy link
Author

zorun commented Apr 14, 2015

So, to sum things up, as a general rule:

  • on systems where dual-stack IPv6 sockets are not implemented or disabled (Windows up to 2003, OpenBSD, etc), you really need two different sockets for IPv4 and IPv6
  • on systems where dual-stack IPv6 sockets are implemented and activated, a single IPv6 socket allows to talk to IPv6 and IPv4 peers. Usually, you can't bind an IPv4 socket on the same port.

Here, for some reasons, binding both a dual-stack IPv6 socket and an IPv4 socket on the same port works. You're right, maybe it's a good idea to find out why before merging this patch :)

@zorun
Copy link
Author

zorun commented Apr 15, 2015

I realised that I was testing on recent Linux kernels, which allows to bind multiple sockets on the same port (SO_REUSEPORT, see https://lwn.net/Articles/542629/)

That being said, I tested again on an older kernel (3.2.0, Debian wheezy, which does not have SO_REUSEPORT), and it works, so Go seems to work this out itself.

@jbenet could you test this on other systems, for instance OS X?

@jbenet
Copy link
Member

jbenet commented Apr 17, 2015

@zorun travis can test both Linux and OSX. it may be useful to add a test case that exercises this, by opening up a swarm with both /ip4 and /ip6 addresses at the same port.

@jbenet
Copy link
Member

jbenet commented Apr 17, 2015

(And, it seems to be working on OSX on my machine.) @zorun will let you decide on the test case.

@jbenet
Copy link
Member

jbenet commented Apr 20, 2015

@zorun bump on this? would be good to have it merged in next couple of days.

@zorun
Copy link
Author

zorun commented Apr 21, 2015

I looked at the tests, but it looks too obscure for me to write one. Sorry!

If it works on Debian and OS X, that's good for me.

@jbenet
Copy link
Member

jbenet commented Apr 21, 2015

I wonder if we have any esoteric machines to test this with-- im not sure which may fail. once again i look at http://build.golang.org/ with desire.

@jbenet
Copy link
Member

jbenet commented Apr 28, 2015

I'd like to merge this, but i'm worried it will break hard on older machines. how could we make ourselves sure it won't? or that it will fail gracefully?

@cryptix
Copy link
Contributor

cryptix commented May 4, 2015

Relevant golang issue: golang/go#9334

@spikebike
Copy link

I submitted a pull request for this exact patch, because I didn't find this one, sorry.

Maybe ipfs init should mention a ipfs reconfig that asks questions, or maybe more comments/documentation in ~/.ipfs/config. Specifically include an example for all IPv6 addresses
"/ip6/::/tcp/4001" and a particular addresses to avoid problems with temporary/privacy Ipv6 addresses.

That way reasonable defaults can be chosen, but also make it easier for people to fix things.

I still think it's reasonable to enable IPv6 by default.

@jbenet
Copy link
Member

jbenet commented Jun 12, 2015

@spikebike ah sorry -- forgot this PR existed.

Still would like to make sure it's safe. what OSes do we have without IPv6 support?

@jbenet
Copy link
Member

jbenet commented Jun 12, 2015

and, +1 to the reconfig command. maybe:

ipfs config interactive

@jbenet
Copy link
Member

jbenet commented Jun 18, 2015

If binding the address fails, but the program continues, then I'm for merging this now. Could we write a test and have someone run it in a system without ip6?

@spikebike
Copy link

It's trivial to disable IPv6 on most platforms, on ubuntu:
sysctl -w net.ipv6.conf.all.disable_ipv6=1
sysctl -w net.ipv6.conf.default.disable_ipv6=1
sysctl -w net.ipv6.conf.lo.disable_ipv6=1

I'm confused by the need for writing a test. Seems like one of two things is going to happen:

  1. this is merged, ipv6 is on by default and everything works on all platforms with IPv4 and/or IPv6.
  2. this is merged, ipv6 on by default breaks some platform. This results in many tests dying because the lack of IPv6 breaks binding to an Ipv4 address.

If it's #1 we are golden, if it's #2 we should likely setup some kind of interactive configuration that asks the user about IPv4, IPv6, privacy addresses, etc.

As soon as I see it merged I'll try it on a ubuntu system with IPv4 only, IPv4+IPv6, and IPv4 + broken IPv6. I don't have any windows or OSX to test though.

@jbenet
Copy link
Member

jbenet commented Jun 18, 2015

@spikebike i'd rather we try it from the PR branch before it is merged. Many people follow + build from master so i don't want to merge things that will break on people.

@spikebike
Copy link

PR = proto-fix branch? Works for me, so @zorun should issue a pull request against that branch then interested parties can test.

@jbenet
Copy link
Member

jbenet commented Jun 18, 2015

@spikebike no i just mean:

  • test this branch zorun:fix-listen-ipv6 or your own spikebike:master
  • in a system with ipv6 disabled
  • verify it works fine despite failure to bind (ipfs daemon with a config with the ipv6 address)

if it checks out, i'll merge this.

@jbenet jbenet added the need/triage Needs initial labeling and prioritization label Jun 30, 2015
@jbenet
Copy link
Member

jbenet commented Jun 30, 2015

We'll should be able to test this with a docker container

@simonvetter
Copy link

any news on this?

@jbenet
Copy link
Member

jbenet commented Aug 15, 2015

@simonvetter i haven't tested this. Would you (or someone else) be able to?

@davidar
Copy link
Member

davidar commented Aug 15, 2015

I've tested with IPv6 enabled and connected (Debian wheezy) and IPv6 both enabled and disabled with no connectivity (Ubuntu 14.04), and I'm not seeing any major problems (other than my usual NAT-related issues). IPFS daemon starts successfully when told to listen on IPv6 even when IPv6 is disabled, and doesn't print any errors.

@jbenet
Copy link
Member

jbenet commented Aug 15, 2015

@davidar thank you! (Wish we had a test, but oh well. people will complain if this breaks)

jbenet added a commit that referenced this pull request Aug 15, 2015
Default config: listen on IPv6 for the swarm address
@jbenet jbenet merged commit 7789077 into ipfs:master Aug 15, 2015
@jbenet jbenet removed the backlog label Aug 15, 2015
@jbenet
Copy link
Member

jbenet commented Sep 10, 2015

surprise, found a machine it fails with \o/ -- #1675

@davidar
Copy link
Member

davidar commented Sep 10, 2015

Ah, I tested with ipv6 disabled via sysctl, but didn't completely unload the kernel module. Sorry @jbenet

@jbenet
Copy link
Member

jbenet commented Sep 10, 2015

@davidar no worries at all, we gave it a fair shot. we'll fix it next round :)

laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
Default config: listen on IPv6 for the swarm address
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Feb 25, 2022
Default config: listen on IPv6 for the swarm address
laurentsenta pushed a commit to laurentsenta/kubo that referenced this pull request Mar 4, 2022
Default config: listen on IPv6 for the swarm address
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need/triage Needs initial labeling and prioritization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants