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

Adding IPv6 network support to docker daemon #8947

Merged
merged 1 commit into from Jan 8, 2015
Merged

Conversation

MalteJ
Copy link
Contributor

@MalteJ MalteJ commented Nov 4, 2014

This PR adds IPv6 support to the docker daemon.
Features:

  • adding CLI flags --ipv6 and --fixed-cidr-v6
  • Setting the bridge IPv6 to fe80::1/64. This IP will be used by the containers as gateway.
  • registers the fixed-cidr-v6 in the ipallocator

Missing:
Test and Documentation.
Follow-up PR for enabling the container to use IPv6. (I have the code but still have to clean it up a bit and create a PR).
I wanted to add them but #8896 popped up and I thought it would be a good idea to provide my code so we can compare and pick the best of both.

Update 2014-11-04

  • Documentation is improving
  • Container IPv6 Implementation is included in this PR as a separate commit

@justinsb
Copy link

justinsb commented Nov 4, 2014

Hi @MalteJ - this looks good to me - much more comprehensive than my patch (support for both IPv4 & IPv6 at the same time, whereas I was going to do that patch-by-patch).

The big thing that isn't in this patch seems to be the iptables stuff (which is mostly just because you have to use "ip6tables" for IPv6 tables). Do you want to incorporate that into this patch, or shall we try to land them separately?

Also, I skip the first IPv6 IP in the ip allocator, whereas you deliberately didn't. Are you OK with skipping the first IPv6?

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 4, 2014

@justinsb thank you!
Yes, I would like to use your iptables code. Do you want to send a PR to me or should I just copy your code?

Why do you want to skip the first IP in the range? Is it used by your gateway?
I would like to use the link-local addresses as gateway address. Hence I am setting the bridge IPv6 to fe80::1. It is something like the default gateway IPv6 by convention.

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 4, 2014

The additional container IPv6 implementation can be found at https://github.com/MalteJ/docker/commits/ipv6-container
I am still unsure how to organize the Pull Requests so they are small and easy to review. Create one large PR with multiple commits or going with this PR and sending a new PR with the container IPv6 code once this is merged?

Best,
Malte

@MalteJ MalteJ force-pushed the ipv6-daemon branch 2 times, most recently from 71144ea to df610a3 Compare November 4, 2014 20:58
@justinsb
Copy link

justinsb commented Nov 4, 2014

Why do you want to skip the first IP in the range? Is it used by your gateway?

If I assign an IPv6 range to the bridge, I will typically use ::1 for the bridge IP. But I am then happy for the first docker instance to be ::2. (At least I don't know of a reason why not.) If we don't skip the first one then I have to specify both --bip and --fixed-cidr (and I lose some of the bip range because fixed-cidr has to be a subnet).

Does this make sense? Am I missing something?

I am still unsure how to organize the Pull Requests so they are small and easy to review.

I'm not sure either... @jfrazelle ?

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 4, 2014

I just don't assign a global IPv6 to the bridge. In the container I am using the bridge's link-local address as gateway.
One thing that would argue for skipping the first IPv6 is that you can set this address to your host's eth0 and address the host from the outside via this address.
Talking about the ipallocator I would also like to generate the IPv6 addresses from the containers' MAC addresses once you have IPv6 subnets bigger than /80 to avoid ndp neighbor cache invalidation issues (see #8371).

@jfrazelle recommended to push the container IPv6 into this PR as a separate commit.
I have done that. You should be able to review the first commits separately as they do not override each other.

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 4, 2014

Ohh, looks like the last commit has broken some tests.

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 4, 2014

You should be able to test it anyway.

# remove your bridge
ifconfig docker0 down
brctl delbr docker0

# run docker daemon with IPv6
docker -d --ipv6 --fixed-cidr-v6="2a00:1450::/64"

# have a look at the new bridge
ifconfig docker0

and create a container

docker run -it --rm --global-ipv6 --name ipv6-test ubuntu bash
# in the container:
ifconfig eth0
route -A inet6

and on the docker host

docker inspect ipv6-test

@jessfraz
Copy link
Contributor

jessfraz commented Nov 4, 2014

let me make sure the build server has ipv6 support

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 4, 2014

I suspect the tests fail because there is something wrong with the code or the tests.

@jessfraz
Copy link
Contributor

jessfraz commented Nov 5, 2014

hmm ya looks like its failing creating the bridge, same for me locally as well, and both the build server and my host have ipv6 support

@@ -58,6 +59,9 @@ func TestAllocatePortDetection(t *testing.T) {

// Init driver
job := eng.Job("initdriver")
job.Stderr.Add(os.Stderr)
job.Stdout.Add(os.Stdout)
job.SetenvBool("EnableIPv6", true)
Copy link
Contributor

Choose a reason for hiding this comment

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

we should probably have a test with and a test without, also this is the test that is failing

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 5, 2014

I have reverted to the original test.
Testing the whole thing on another machine I could reproduce your issues when creating the bridge. I have committed a fix for that.
Are you now able to create an IPv6 enabled container on your machine using my instructions above?

I have added further documentation.
This PR is still missing tests and ip6tables.

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 5, 2014

I tried to run this implementation on Digital Ocean but their IPv6 networking does not fit my approach.
I am using the Docker host as a gateway that routes IPv6 traffic of a specified subnet to its docker0 bridge. The containers use the Docker host's bridge interface link-local IPv6 as their gateway.

On Digital Ocean you use a part (16 addresses / 4 bits) of a /64 network. But this part of the /64 network is not a subnet. You are just allowed to use those 16 IPs. This means you are expected to respond to ndp neighbor solicitation requests for those 16 IPs. There is no route set up that uses your VM as a gateway for this subnet.

The result: My approach does not work on DigitalOcean.

How do you think about this?

There is a german IaaS provider (www.jiffybox.de) that gives you a /56 subnet for your VM. Here everything works fine.
I don't really like DigitalOcean's approach as in any way you would have to hack your network settings and connect the docker bridge to the host's eth0 or do ip6tables NAT stuff or foo like this.

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 5, 2014

This said, I think I have forgotten to implement adding the route to the fixed-cidr-v6 on the docker host machine.
But that doesn't change anything on the DigitalOcean issues.

@justinsb
Copy link

justinsb commented Nov 5, 2014

Hi @MalteJ - let's coordinate a plan here: I'm happy to merge my patch into yours - I guess I should propose a patch on your repo with my ip6tables changes. Does that sound good to you?

I have an OVH box which has a /64 which I can try this out on. I've also been trying this on EC2 using protocol 41.

I have a busy day, but hopefully I can get to this tonight.

Does that work?

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 5, 2014

@justinsb sounds great! :)

@jessfraz
Copy link
Contributor

jessfraz commented Nov 5, 2014

ping @skottler do you think digital ocean will be supporting this soon?

@discordianfish
Copy link
Contributor

Yup, looks like digitalocean's IPv6 support is incomplete. Maybe something @progrium is interested in as well and could help to fix :)

@LK4D4
Copy link
Contributor

LK4D4 commented Nov 17, 2014

@MalteJ @justinsb Hello, any news about this PR? Do you have plans to add more commits here or it is ready for review?

@SvenDowideit
Copy link
Contributor

Docs LGTM - @fredlf @jamtur01

I presume that getting the Docker daemon to listen on IPv6 port will need to be done in a separate PR? (It'll be useful in b2d)

@MalteJ
Copy link
Contributor Author

MalteJ commented Nov 17, 2014

Hi @LK4D4
@justinsb wants to add ip6tables support.
I want to add some tests but hadn't have time to do so, yet.
From my side all functionality is implemented and ready to review.

I have merged the current master to this PR and resolved the conflicts. Hopefully the tests are still green.

@SvenDowideit yeah, I haven't thought of this until now.
I am okay with a separate PR. I think listening to an IPv6 port would even work right now, without this PR.

// Enable IPv6 on the bridge
procFile := "/proc/sys/net/ipv6/conf/" + iface.Name + "/disable_ipv6"
if err := ioutil.WriteFile(procFile, []byte{'0', '\n'}, 0644); err != nil {
return fmt.Errorf("Unable to enable IPv6 addresses on bridge: %s\n", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Should output start with a lower case character?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

don't know!? should it?

@MalteJ
Copy link
Contributor Author

MalteJ commented Jan 8, 2015

OK, I have tweaked the docs a bit to give some info about the accept_ra issue.

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 8, 2015

I tried also to run tests with daemon --ipv6 --fixed-cidr-v6=2a00:1450::/64.
All was Okay. I think we should run test daemon with this options and write tests for ipv6 after merge.

@@ -55,9 +57,11 @@ func (config *Config) InstallFlags() {
flag.BoolVar(&config.EnableIptables, []string{"#iptables", "-iptables"}, true, "Enable Docker's addition of iptables rules")
flag.BoolVar(&config.EnableIpForward, []string{"#ip-forward", "-ip-forward"}, true, "Enable net.ipv4.ip_forward")
Copy link
Contributor

Choose a reason for hiding this comment

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

This is now doing not only this.

@jessfraz
Copy link
Contributor

jessfraz commented Jan 8, 2015

I can add a ipv6 server into our jenkins matrix as well :)

@tianon
Copy link
Member

tianon commented Jan 8, 2015

Thanks @MalteJ ❤️

I think the information you've added would give me the appropriate warning (and information) I'd need before I jumped into this head-first. 👍

@@ -75,6 +76,7 @@ expect an integer, and they can only be specified once.
--ip-forward=true Enable net.ipv4.ip_forward
Copy link
Contributor

Choose a reason for hiding this comment

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

Here too pls :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@MalteJ MalteJ force-pushed the ipv6-daemon branch 5 times, most recently from 4232dd2 to 50ddcf1 Compare January 8, 2015 23:11
Signed-off-by: Malte Janduda <mail@janduda.net>
@MalteJ
Copy link
Contributor Author

MalteJ commented Jan 8, 2015

OK, merged, rebased and it's green again :)

@LK4D4
Copy link
Contributor

LK4D4 commented Jan 8, 2015

LGTM

LK4D4 added a commit that referenced this pull request Jan 8, 2015
Adding IPv6 network support to docker daemon
@LK4D4 LK4D4 merged commit 3fbf723 into moby:master Jan 8, 2015
@thaJeztah
Copy link
Member

🎉 awesome work @MalteJ !

@MalteJ
Copy link
Contributor Author

MalteJ commented Jan 8, 2015

Great to see this PR getting merged.

Thanks to all who participated!
Especially @jfrazelle and @LK4D4

@MalteJ MalteJ mentioned this pull request Jan 9, 2015
@elsbrock
Copy link

elsbrock commented Jan 9, 2015

👍 🎉


hw[0] ^= 0x2

return fmt.Sprintf("fe80::%x%x:%xff:fe%x:%x%x/64", hw[0], hw[1], hw[2], hw[3], hw[4], hw[5]), nil

Choose a reason for hiding this comment

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

Probable source of the problem #13211: "%x%x" with (0x1,0x2) will return the same ipv6 as (0x0,0x12)

@dhirtysix
Copy link

It looks like you guys were planning to merge ip6tables changes with this pull request but it never happened. Why didn't it happen and can we merge the ip6tables pull request now? #8896

@wrridgwa
Copy link
Contributor

@justinsb Why didn't your ip6tables patch get merged in with this?

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.

None yet