bridge: Add support for IPv6 to bridge plugin #10

leblancd · 2017-05-25T15:04:21Z

This change adds support for IPv6 container/pod addresses to the CNI
bridge plugin, both for dual-stack (IPv4 + IPv6) and for IPv6-only
network configurations.

The proposed changes support multiple IPv6 addresses on a container
interface. If isGW is configured, the bridge will also be configured with
gateway addresses for each IPv6 subnet.

Please note that both the dual-stack functionality and support for multiple
IPv6 container/gateway addresses depends upon containernetworking/cni
PR #451 "ipam/host-local: support multiple IP ranges".
The bridge plugin changes being proposed here could potentially be committed
independently from this host-local plugin change; however the dual-stack and
multiple IPv6 address functionality that is enabled by this change can't be
exercised/tested until the host-local plugin change is committed.

There are some dual-stack/multiple-IPv6 unit test cases that are
currently commented out in the proposed changes because these
test cases will fail without the prior commits of the multiple IP
range host-local change.

This pull request includes a temporary workaround for Kubernetes
Issue #32291 (Container IPv6 address is marked as duplicate (dadfailed).
The problem is that kubelet enables hairpin mode on bridge veth
interfaces. Hairpin mode causes the container/pod to see echos of its
IPv6 neighbor solicitation packets, so that it declares duplicate address
detection (DAD) failure. The long-term fix is to use enhanced-DAD
when that feature is readily available in kernels. The short-term fix is
to disable IPv6 DAD in the container. Unfortunately, this has to be done
unconditionally (i.e. without a check for whether hairpin mode is enabled)
because hairpin mode is turned on by kubelet after the CNI bridge plugin
has completed cmdAdd processing. Disabling DAD should be okay if
IPv6 addresses are guaranteed to be unique (which is the case for
host-local IPAM plugin).

squeed · 2017-05-31T14:22:51Z

plugins/main/bridge/bridge.go

@@ -392,7 +454,7 @@ func cmdDel(args *skel.CmdArgs) error {
 	var ipn *net.IPNet
 	err = ns.WithNetNSPath(args.Netns, func(_ ns.NetNS) error {
 		var err error
-		ipn, err = ip.DelLinkByNameAddr(args.IfName, netlink.FAMILY_V4)


dcbw · 2017-05-31T14:44:38Z

plugins/main/bridge/bridge.go

+		// Add a default route for this family using the current
+		// gateway address if necessary.
+		if n.IsDefaultGW && !gws.defaultRouteFound {
+			defaultNet.Mask = net.IPMask(defaultNet.IP)


Any reason you wouldn't do this above, where defaultNet.IP gets set?

Hmmm... I think I was looking at execution efficiency and only executing the line when necessary, but I see your point that it's much clearer to just set the defaultNet fields in one place... so I'll move it.

dcbw · 2017-05-31T14:53:54Z

plugins/main/bridge/bridge.go

+		// Multiple IPv6 addresses are allowed. For IPv4, if
+		// forceAddress is set to true then reconfigure IP address,
+		// otherwise throw error.
+		if family == netlink.FAMILY_V4 {


Is there a reason we wouldn't want to force the IPv6 address too? Obvoiusly the bridge could have an LL and a non-LL address, but I would expect ForceAddress to work for the IPv6 non-LL address as well.

Yes, good point. It is a little complicated since we're allowing multiple V6 addresses on the bridge. Let's say we're adding addresses for two V6 subnets: subnet A and subnet B. When this function wants to set a V6 address on the bridge, and the new address happens to be within subnet A... then we'd only want to clear (force) the existing address that corresponds to subnet A (or declare error if ForceAddress is false).

dcbw · 2017-05-31T14:55:58Z

plugins/main/bridge/bridge.go

-					&types.Route{Dst: *defaultNet, GW: ipc.Gateway},
-				)
-			}
+		// Temporary workaround for Kubernetes Issue #32291. Disable


While this is fine for now, soon kubelet won't do anything hairpin related (see kubernetes/kubernetes#45790 (comment)) and that'll get punted to this plugin. So we'll know in the plugin if hairpin is enabled, and if so, whether we need to disable DAD or not. Can be a follow-on PR.

Sounds good.

dcbw · 2017-05-31T14:57:22Z

plugins/main/bridge/bridge.go

-		if err := ip.SetHWAddrByIP(args.IfName, result.IPs[0].Address.IP, nil /* TODO IPv6 */); err != nil {
-			return err
+		if result.IPs[0].Address.IP.To4() != nil {
+			if err := ip.SetHWAddrByIP(args.IfName, result.IPs[0].Address.IP, nil /* TODO IPv6 */); err != nil {


Also probably fine by now, but containernetworking/cni#448 removes this whole set-HW-addr-by-IP thing. But we can make @squeed rebase that onto this :)

Or I can rebase onto his change. Either way works for me. :)

leblancd · 2017-06-01T16:34:08Z

@dcbw I've made changes according to your suggestions:

Move setting of defaultNet.Mask back to (near) the point where defaultNet.IP is set.
For setting IPv6 addresses on the bridge, check the forceAddress flag
similar to what's done for IPv4, but only if the subnet for the requested
V6 address overlaps with the subnet of an existing address. For
non-overlapping V6 subnets, addresses are additive.

leblancd · 2017-06-01T17:46:51Z

Test failures appear to be due to Travis test config issue fixed with PR #13.

squeed · 2017-06-02T16:30:42Z

FWIW, once this is merged, I will have a PR that stops changing the mac address.

leblancd · 2017-06-05T14:28:57Z

@squeed, thanks, I saw your PR for retaining MAC address (using arp) in the containernetworking/cni repo. Looks good. I did some testing with your MAC address changes patched in with the changes here to make sure that it'll work when your arp changes merge. By the way, what is the best way to trigger a new round of travis tests?

squeed · 2017-06-06T15:54:23Z

Hmm, I'm curious why you are adding the "local" route via the gateway - isn't one of the points of using a bridge that the packets don't need to be routed twice?

leblancd · 2017-06-06T20:11:32Z

@squeed : Can you clarify where the local route is being added? I am adding a default route (dest = ::0/0, not a /128) in the container via one of the addresses on the bridge. (I could have set the container's default route via the bridge's link local address, but I figured it was easier to just grab an address that I had on hand). I could be missing something.

squeed · 2017-06-07T08:35:18Z

Ah, you're right - I misread the calcGateways function.

The IsDefaultGateway flag in the bridge configuration strikes me as kind of useless (it should have been part of ipam), but oh well - now I understand why you need all that code.

LGTM.

squeed · 2017-06-21T09:05:22Z

@dcbw want to give this the second seal of approval?

leblancd

@squeed @dcbw : I enabled the UT test cases that were dependent on PR #12 getting merged. I also modified the comment for disabling IPv6 DAD to delete the Kubernetes issue (no longer applies) and add short/long term TODOs.

This change adds support for IPv6 container/pod addresses to the CNI bridge plugin, both for dual-stack (IPv4 + IPv6) and for IPv6-only network configurations. The proposed changes support multiple IPv6 addresses on a container interface. If isGW is configured, the bridge will also be configured with gateway addresses for each IPv6 subnet. Please note that both the dual-stack functionality and support for multiple IPv6 container/gateway addresses depends upon containernetworking/cni PR 451 "ipam/host-local: support multiple IP ranges". This change could potentially be committed independently from this host-local plugin change, however the dual-stack and multiple IPv6 address functionality that is enabled by this change can't be exercised/tested until the host-local plugin change is committed. There are some IPv6 unit test cases that are currently commented out in the proposed changes because these test cases will fail without the prior commits of the multiple IP range host-local change. This pull request includes a temporary workaround for Kubernetes Issue #32291 (Container IPv6 address is marked as duplicate, or dadfailed). The problem is that kubelet enables hairpin mode on bridge veth interfaces. Hairpin mode causes the container/pod to see echos of its IPv6 neighbor solicitation packets, so that it declares duplicate address detection (DAD) failure. The long-term fix is to use enhanced-DAD when that feature is readily available in kernels. The short-term fix is to disable IPv6 DAD in the container. Unfortunately, this has to be done unconditionally (i.e. without a check for whether hairpin mode is enabled) because hairpin mode is turned on by kubelet after the CNI bridge plugin has completed cmdAdd processing. Disabling DAD should be okay if IPv6 addresses are guaranteed to be unique (which is the case for host-local IPAM plugin).

dcbw · 2017-06-28T13:34:50Z

LGTM

…evert Reverts change to build_linux.sh to build.sh

leblancd mentioned this pull request May 31, 2017

bridge: Add support for IPv6 to bridge plugin containernetworking/cni#416

Closed

squeed reviewed May 31, 2017

View reviewed changes

dcbw requested changes May 31, 2017

View reviewed changes

leblancd force-pushed the v6_bridge_plugin branch from 2372076 to 0ddcdd7 Compare June 1, 2017 16:12

leblancd force-pushed the v6_bridge_plugin branch from 0ddcdd7 to 9abe113 Compare June 1, 2017 17:14

leblancd force-pushed the v6_bridge_plugin branch 2 times, most recently from 4804d31 to 37f8c0a Compare June 5, 2017 23:58

squeed mentioned this pull request Jun 7, 2017

ptp, macvlan: don't change mac address; send gratuitous arp #16

Merged

rosenhouse mentioned this pull request Jun 8, 2017

Bridge plugin needs support for IPv6 addresses containernetworking/cni#415

Closed

leblancd force-pushed the v6_bridge_plugin branch from 37f8c0a to 9a8bc97 Compare June 21, 2017 14:03

leblancd commented Jun 21, 2017

View reviewed changes

leblancd force-pushed the v6_bridge_plugin branch from 9a8bc97 to ffdc748 Compare June 21, 2017 16:49

dcbw approved these changes Jun 28, 2017

View reviewed changes

dcbw merged commit f01230d into containernetworking:master Jun 28, 2017

trusch mentioned this pull request Jun 29, 2017

stage0: add --expose flag on app level rkt/rkt#3691

Open

bboreham mentioned this pull request Aug 16, 2017

Add IPv6 support containernetworking/cni#31

Closed

5 tasks

lsm5 pushed a commit to lsm5/containernetworking-plugins that referenced this pull request Oct 24, 2019

Merge pull request containernetworking#10 from openshift/buildlinux-r…

24f7574

…evert Reverts change to build_linux.sh to build.sh

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bridge: Add support for IPv6 to bridge plugin #10

bridge: Add support for IPv6 to bridge plugin #10

leblancd commented May 25, 2017 •

edited

Loading

squeed May 31, 2017

dcbw May 31, 2017

leblancd May 31, 2017

dcbw May 31, 2017

leblancd May 31, 2017

dcbw May 31, 2017

leblancd May 31, 2017

dcbw May 31, 2017

leblancd May 31, 2017

leblancd commented Jun 1, 2017

leblancd commented Jun 1, 2017

squeed commented Jun 2, 2017

leblancd commented Jun 5, 2017

squeed commented Jun 6, 2017

leblancd commented Jun 6, 2017

squeed commented Jun 7, 2017

squeed commented Jun 21, 2017

leblancd left a comment

dcbw commented Jun 28, 2017

bridge: Add support for IPv6 to bridge plugin #10

bridge: Add support for IPv6 to bridge plugin #10

Conversation

leblancd commented May 25, 2017 • 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

leblancd commented Jun 1, 2017

leblancd commented Jun 1, 2017

squeed commented Jun 2, 2017

leblancd commented Jun 5, 2017

squeed commented Jun 6, 2017

leblancd commented Jun 6, 2017

squeed commented Jun 7, 2017

squeed commented Jun 21, 2017

leblancd left a comment

Choose a reason for hiding this comment

dcbw commented Jun 28, 2017

leblancd commented May 25, 2017 •

edited

Loading