Remaining steps for IPv6 support #8896

justinsb · 2014-10-31T19:05:56Z

Most of the work for IPv6 has already been done; this adds support for using ip6tables, makes sure ipv6 support is enabled on the docker0 bridge, and skips the first IP even on IPv6 so we don't have to use a sub-range.

jessfraz · 2014-10-31T20:18:12Z

thanks!! reviewing now!

LK4D4 · 2014-10-31T20:28:12Z

pkg/iptables/iptables.go

@@ -34,28 +35,31 @@ func init() {
 	supportsXlock = exec.Command("iptables", "--wait", "-L", "-n").Run() == nil
 }

-func NewChain(name, bridge string) (*Chain, error) {
-	if output, err := Raw("-t", "nat", "-N", name); err != nil {
+func NewChain(ipv6 bool, name, bridge string) (*Chain, error) {


I think this should be refactored, this passing ipv6 everywhere is not very impressive. Maybe some struct which can create and delete chains. So, you just create it with ipv6 or ipv4 param and use it in code below.

ah yes I like that

I agree, it's annoying. NewChain is the object you are describing, I think, and it does embed Ipv6. So if we're using Chains, we only pass ipv4/ipv6 in once.

Unfortunately, Raw is then exported, rather than everything going through Chain. So we have to pass the ipv6 flag in here.

I did debate creating something like

func (c *Chain) Raw(args... string) ([]byte, error) { return Raw(c.Ipv6, args) }

Do you think that would help?

The bigger problem is the whole use of Raw in other packages. I was going to clean up the whole use of Raw, but then the patch would have ballooned and been cross-purpose.

ya i think that would help and we could do a cleanup PR after for the Raw calls other places

Went with the suggestion; it isn't too bad, because Raw is actually only used in bridge. Thanks!

LK4D4 · 2014-10-31T20:28:42Z

Drone failing with some cryptic error :/

jessfraz · 2014-10-31T20:34:10Z

ya im making sure the kernel has iptables with ipv6 support...

justinsb · 2014-10-31T20:37:03Z

I can't figure out the drone error either. I've obviously done something stupid.

We shouldn't even be testing IPv6 - unless you pass -ipv6, the code should short-circuit to the existing behaviour.

(Of course, we do need tests for ipv6, but I can't figure out why drone is failing)

jessfraz · 2014-10-31T20:43:46Z

ya so there's other refernces to the iptables package when the daemon is started and they would need to reflect the changes to the package args, it wouldnt throw a compile time error because the args are passed as args..

jessfraz · 2014-10-31T20:43:58Z

let me find the file and I can explain better

jessfraz · 2014-10-31T20:46:29Z

actually nm I was thinking of bridge and you got all those

jessfraz · 2014-10-31T20:52:57Z

justinsb · 2014-10-31T20:54:55Z

Found a surprising behavior of the To16() function (it converts IPv4 to a 16 bit address as well), but that still isn't it, I don't think...

jessfraz · 2014-10-31T20:56:28Z

maybe https://github.com/docker/docker/blob/master/daemon/networkdriver/portmapper/mapper.go#L175
EDIT: nooo that should be fine

jessfraz · 2014-10-31T21:05:13Z

-d -D
INFO[0000] +job serveapi(unix:///var/run/docker.sock)   
INFO[0000] Listening for HTTP on unix (/var/run/docker.sock) 
DEBU[0000] Registering GET, /images/{name:.*}/get       
DEBU[0000] Registering GET, /containers/{name:.*}/export 
DEBU[0000] Registering GET, /containers/{name:.*}/changes 
DEBU[0000] Registering GET, /events                     
DEBU[0000] Registering GET, /info                       
DEBU[0000] Registering GET, /images/json                
DEBU[0000] Registering GET, /images/search              
DEBU[0000] Registering GET, /containers/{name:.*}/logs  
DEBU[0000] Registering GET, /containers/{name:.*}/attach/ws 
DEBU[0000] Registering GET, /_ping                      
DEBU[0000] Registering GET, /images/{name:.*}/history   
DEBU[0000] Registering GET, /images/{name:.*}/json      
DEBU[0000] Registering GET, /containers/{name:.*}/json  
DEBU[0000] Registering GET, /version                    
DEBU[0000] Registering GET, /images/viz                 
DEBU[0000] Registering GET, /containers/ps              
DEBU[0000] Registering GET, /containers/json            
DEBU[0000] Registering GET, /images/get                 
DEBU[0000] Registering GET, /containers/{name:.*}/top   
DEBU[0000] Registering POST, /containers/{name:.*}/stop 
DEBU[0000] Registering POST, /containers/{name:.*}/wait 
DEBU[0000] Registering POST, /build                     
DEBU[0000] Registering POST, /containers/{name:.*}/pause 
DEBU[0000] Registering POST, /containers/{name:.*}/restart 
DEBU[0000] Registering POST, /containers/{name:.*}/attach 
DEBU[0000] Registering POST, /containers/{name:.*}/exec 
DEBU[0000] Registering POST, /images/load               
DEBU[0000] Registering POST, /containers/create         
DEBU[0000] Registering POST, /containers/{name:.*}/resize 
DEBU[0000] Registering POST, /images/{name:.*}/push     
DEBU[0000] Registering POST, /images/{name:.*}/tag      
DEBU[0000] Registering POST, /containers/{name:.*}/kill 
DEBU[0000] Registering POST, /containers/{name:.*}/unpause 
DEBU[0000] Registering POST, /containers/{name:.*}/start 
DEBU[0000] Registering POST, /auth                      
DEBU[0000] Registering POST, /commit                    
DEBU[0000] Registering POST, /images/create             
DEBU[0000] Registering POST, /exec/{name:.*}/start      
DEBU[0000] Registering POST, /containers/{name:.*}/copy 
DEBU[0000] Registering POST, /exec/{name:.*}/resize     
DEBU[0000] Registering DELETE, /containers/{name:.*}    
DEBU[0000] Registering DELETE, /images/{name:.*}        
DEBU[0000] Registering OPTIONS,                         
DEBU[0000] docker group found. gid: 999                 
DEBU[0000] Using graph driver btrfs                     
DEBU[0000] Creating images graph                        
DEBU[0000] Restored 0 elements                          
DEBU[0000] Creating repository list                     
INFO[0000] +job init_networkdriver()                    
DEBU[0000] 172.17.42.1/16 requested network overlaps with existing network 
DEBU[0000] Creating bridge docker0 with network 10.0.42.1/16 
DEBU[0000] setting bridge mac address = true            
DEBU[0000] /sbin/ip6tables, [--wait -C POSTROUTING -t nat -s 10.0.42.1/16 ! -o docker0 -j MASQUERADE] 
DEBU[0000] /sbin/ip6tables, [--wait -I POSTROUTING -t nat -s 10.0.42.1/16 ! -o docker0 -j MASQUERADE] 
Unable to enable network bridge NAT: ip6tables failed: ip6tables --wait -I POSTROUTING -t nat -s 10.0.42.1/16 ! -o docker0 -j MASQUERADE: ip6tables v1.4.21: host/network `10.0.42.1' not found
Try `ip6tables -h' or 'ip6tables --help' for more information.
 (exit status 2)
INFO[0000] -job init_networkdriver() = ERR (1)          
FATA[0000]  (exit status 2)

justinsb · 2014-10-31T21:06:52Z

Awesome - thank you. I should be able to fix it quickly now

jessfraz · 2014-10-31T21:08:10Z

daemon/networkdriver/utils.go

@@ -73,7 +73,7 @@ func NetworkRange(network *net.IPNet) (net.IP, net.IP) {
 }

 // Return the IPv4 address of a network interface
-func GetIfaceAddr(name string) (net.Addr, error) {
+func GetIfaceAddr(name string, ipv4 bool, ipv6 bool) (net.Addr, error) {


I dont think we need to pass both if it's not ipv6 it implies its ipv4

I think in future we'll likely want to use both IPv4 and IPv6 at the same time. It's only called with (name, !ipv6, ipv6) at the moment.

Happy to fix either way.

ah this is true

On Fri, Oct 31, 2014 at 2:12 PM, Justin Santa Barbara <
notifications@github.com> wrote:

In daemon/networkdriver/utils.go:

@@ -73,7 +73,7 @@ func NetworkRange(network *net.IPNet) (net.IP, net.IP) {
}

// Return the IPv4 address of a network interface
-func GetIfaceAddr(name string) (net.Addr, error) {
+func GetIfaceAddr(name string, ipv4 bool, ipv6 bool) (net.Addr, error) {

I think in future we'll likely want to use both IPv4 and IPv6 at the same
time. It's only called with (name, !ipv6, ipv6) at the moment.

Happy to fix either way.

—
Reply to this email directly or view it on GitHub
https://github.com/docker/docker/pull/8896/files#r19694007.

I think you're right, on reflection. It would have to be refactored anyway to return multiple IP addresses at the same time. Your way is better.

jessfraz · 2014-10-31T21:10:19Z

cool, if you are running locally you can make binary & make shell to
pop in the container then ./bundles/1.3.1-dev/binary/docker -d -D to run
the daemon with debug and see if it runs :)

On Fri, Oct 31, 2014 at 2:07 PM, Justin Santa Barbara <
notifications@github.com> wrote:

Awesome - thank you. I should be able to fix it quickly now

—
Reply to this email directly or view it on GitHub
#8896 (comment).

jessfraz · 2014-10-31T21:39:17Z

ping @jpetazzo would love your thoughts on this one

LK4D4 · 2014-10-31T22:14:34Z

Drone still failing :/

jessfraz · 2014-10-31T22:15:49Z

can you change the lines in integration-cli/docker_cli_links_test.go that reference iptables :)

jessfraz · 2014-10-31T22:48:00Z

awesome drone passed. I swapped this with my local binary and its pretty freaking cool. so things we need to get this in:

docs
man page updates
some integration tests
we can tag team this and I can write some tests so we can be sure if we add anything in the future we won't break ipv6 :)

jessfraz · 2014-10-31T22:49:00Z

and we need to be sure the docs specify the requirement for the iptables ipv6 module to be active

thaJeztah · 2014-10-31T23:00:08Z

and we need to be sure the docs specify the requirement for the iptables ipv6 module to be active

Will that be a requirement to run docker or only if I want to make use of ipv6? Just checking as I have some VPS in a DigitalOcean datacenter that hasn't migrated to IPv6 yet

tianon · 2014-11-03T02:25:02Z

Also, so much ♥ for all this groundwork! :D

LK4D4 · 2014-11-03T19:03:15Z

Need rebase :)

Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

Otherwise this forces use of a configured subnet. Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

This saves us from having to pass the ipv6 flag around Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

justinsb · 2014-11-03T19:11:19Z

Rebased

Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

Signed-off-by: Justin SB <justin@fathomdb.com>

MalteJ · 2014-11-04T16:19:07Z

I am working on the IPv6 support for a while and have a working version with IPv6 support.
When I saw your PR I thought it would be a good idea to provide my current status so we can pick the best of both: #8947
My IPv6 implementation is splitted up to two PRs. The first one adds IPv6 support to the daemon. The next one will enable IPv6 in the containers (assigning IPs and setting routes).
Hope this helps :-)

SvenDowideit · 2014-11-05T04:51:11Z

so - if this is the final one, it needs documentation :)

thaJeztah · 2014-11-05T07:06:43Z

@SvenDowideit If I follow correctly, @justinsb and @MalteJ are working on combining their PRs.

If I understood that correctly, then probably one should be closed to keep the discussion/progress focused in a single place.

MalteJ · 2014-11-05T10:09:30Z

I would like to keep mine (#8947) open as it has more functionality.
This one has ip6tables functionality that mine is still missing.

thaJeztah · 2014-11-05T10:32:22Z

@MalteJ I'm fine either way as long as you are both in agreement 😄. Having two PRs covering the same goal is confusing; closing one prevents the confusion and may save the maintainers some time.

As mentioned, I'm just a bystander, so I'll leave it up to you (the creators of the PRs) and the maintainers. If one PR is closed, please update the description and/or closing comment to guide people to the active PR.

Thank you both for this work, really excited to see IPv6 become a reality in Docker!

MalteJ · 2014-11-05T10:43:32Z

@thaJeztah you are right! I just have seen this PR yesterday. @justinsb and I still have to do some communication and agree on the next steps.

justinsb · 2014-11-05T14:36:45Z

@MalteJ and I are going to work to combine this patch into #8947

jessfraz · 2014-11-18T00:13:45Z

so i am going to close this one since you are combining into #8947, just so there is no confusion and the conversation doesn't get fragmented :)

This was referenced Oct 31, 2014

Skip the first IP for IPv6 as well #8891

Closed

Make sure IPv6 is enabled on the bridge, for IPv6 #8890

Closed

Ipv6 support for iptables #8888

Closed

justinsb force-pushed the ipv6 branch from 3662189 to 57d158c Compare October 31, 2014 19:09

LK4D4 reviewed Oct 31, 2014
View reviewed changes

justinsb force-pushed the ipv6 branch from cd2fcef to 7a2068b Compare October 31, 2014 20:48

justinsb force-pushed the ipv6 branch from 7a2068b to a56bd3c Compare October 31, 2014 20:53

jessfraz reviewed Oct 31, 2014
View reviewed changes

justinsb force-pushed the ipv6 branch 3 times, most recently from 61240e8 to e87ba5e Compare October 31, 2014 21:37

justinsb added 7 commits November 3, 2014 11:09

Ipv6 support for iptables

68551cf

Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

Make sure IPv6 is enabled on the bridge, for IPv6

74a0f87

Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

Skip the first IP for IPv6 as well

cfae849

Otherwise this forces use of a configured subnet. Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

Fix tests now that IPv6s start at ::2

a03eec3

Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

Check IPv6 vs IPv4 strictly, using To16 and To4

e91c4ab

Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

Simplify GetIfaceAddr - don't require both ipv4 and ipv6 args

e5f8791

Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

Create a wrapper around an iptable

2db10d4

This saves us from having to pass the ipv6 flag around Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

justinsb force-pushed the ipv6 branch from a94a9d8 to c869545 Compare November 3, 2014 19:10

Fix unit tests that use iptables

29220f1

Signed-off-by: Justin Santa Barbara <justin@fathomdb.com>

justinsb force-pushed the ipv6 branch from c869545 to 29220f1 Compare November 3, 2014 19:12

Fix rebase error

67957d4

Signed-off-by: Justin SB <justin@fathomdb.com>

MalteJ mentioned this pull request Nov 4, 2014

Adding IPv6 network support to docker daemon #8947

Merged

justinsb mentioned this pull request Nov 4, 2014

Initial ipv6 / iptables work kubernetes/kubernetes#2147

Merged

SvenDowideit added the /project/doc label Nov 5, 2014

jessfraz modified the milestone: 1.4 Nov 18, 2014

jessfraz closed this Nov 18, 2014

Remaining steps for IPv6 support #8896

Remaining steps for IPv6 support #8896

Conversation

justinsb commented Oct 31, 2014

jessfraz commented Oct 31, 2014

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

LK4D4 commented Oct 31, 2014

jessfraz commented Oct 31, 2014

justinsb commented Oct 31, 2014

jessfraz commented Oct 31, 2014

jessfraz commented Oct 31, 2014

jessfraz commented Oct 31, 2014

jessfraz commented Oct 31, 2014

justinsb commented Oct 31, 2014

jessfraz commented Oct 31, 2014

jessfraz commented Oct 31, 2014

justinsb commented Oct 31, 2014

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

jessfraz commented Oct 31, 2014

jessfraz commented Oct 31, 2014

LK4D4 commented Oct 31, 2014

jessfraz commented Oct 31, 2014

jessfraz commented Oct 31, 2014

jessfraz commented Oct 31, 2014

thaJeztah commented Oct 31, 2014

tianon commented Nov 3, 2014

LK4D4 commented Nov 3, 2014

justinsb commented Nov 3, 2014

MalteJ commented Nov 4, 2014

SvenDowideit commented Nov 5, 2014

thaJeztah commented Nov 5, 2014

MalteJ commented Nov 5, 2014

thaJeztah commented Nov 5, 2014

MalteJ commented Nov 5, 2014

justinsb commented Nov 5, 2014

jessfraz commented Nov 18, 2014