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

Add dual stack support #1398

Closed
wants to merge 1 commit into from
Closed

Conversation

yaoice
Copy link
Contributor

@yaoice yaoice commented Jan 15, 2021

Add dual stack support for flannel, it provided with three mode for flannel:

  1. Only ipv4 stack as the origin.
  2. Only ipv6 stack
  3. Dual stack

Add new option for flannel daemon to support dual stack:

  • "publicIPv6": "IPv6 accessible by other nodes for
    inter-host communication"
  • "auto-detect-ipv4": "auto detect ipv4 address of the iface",
    default value is true.
  • "auto-detect-ipv6": "auto detect ipv6 address of the iface",
    default value is false

Add new option into net-conf.json configuration, like following:

{
  "EnableIPv4": true,
  "EnableIPv6": true,
  "Network": "172.16.0.0/16",
  "IPv6Network": "fc00::/48",
  "Backend": {
    "Type": "vxlan"
  }
}

EnableIPv4 default value is true for useing kube subnet manager.
EnableIpv6 default value is false.

Flannel dual stack feature has limitation, only work with vxlan backend
and kube subnet manager now. To enable flannel dual stack feature, need
to do the following step:

  1. setting flanneld daemon with --kube-subnet-mgr --auto-detect-ipv6
  2. settting EnableIPv6 and IPv6Network in net-conf.json, like the
    above configuration.
  3. setting network interface that flannel used ipv6 address and
    default ipv6 gateway in the host node.
  4. vxlan support ipv6 tunnel require kernel version >= 3.12.

It also need flannel cni plugin to support dual stack ip allocation, so it
depends on: containernetworking/plugins#570

#248

Signed-off-by: yaoice yao3690093@gmail.com

@yaoice yaoice force-pushed the support-dual-stack branch 3 times, most recently from 0d03f94 to f94890e Compare January 18, 2021 01:50
@yaoice
Copy link
Contributor Author

yaoice commented Jan 18, 2021

@rajatchopra

@yaoice yaoice force-pushed the support-dual-stack branch 2 times, most recently from f4d27b6 to 5aca51a Compare January 20, 2021 11:51
Add new option for flannel daemon to support dual stack:
- "publicIPv6": "IPv6 accessible by other nodes for
  inter-host communication"
- "auto-detect-ipv4": "auto detect ipv4 address of the iface",
  default value is true.
- "auto-detect-ipv6": "auto detect ipv6 address of the iface",
  default value is false

Add new option into `net-conf.json` configuration, like following:
{
  "EnableIPv4": true,
  "EnableIPv6": true,
  "Network": "172.16.0.0/16",
  "IPv6Network": "fc00::/48",
  "Backend": {
    "Type": "vxlan"
  }
}
EnableIPv4 default value is true for useing kube subnet manager.
EnableIpv6 default value is false.

Flannel dual stack feature has limitation, only work with vxlan backend
and kube subnet manager now. To enable flannel dual stack feature, need
to do the following step:
1. setting flanneld daemon with "--kube-subnet-mgr --auto-detect-ipv6"
2. settting "EnableIPv6" and "IPv6Network" in "net-conf.json" like the
above configuration.
3. setting network interface that flannel used ipv6 address and
default ipv6 gateway in the host node.
4. vxlan support ipv6 tunnel require kernel version >= 3.12.

Signed-off-by: yaoice <yao3690093@gmail.com>
@luthermonson
Copy link
Contributor

this looks like a good one for v0.14.x after we finish up RCs for 13 and get v0.13.1 out. probably can merge this pretty quick and RC it for testing purposes.

@unixfox
Copy link

unixfox commented Mar 31, 2021

Hello @luthermonson,
Any news about the time when you plan to start working on v0.14.x RC versions?
Kubernetes 1.21 is going to be soon released with dual stack support in beta and I would like to use flannel on a dual stack Kubernetes 1.21 cluster.
K3s is considering to add dual stack support, but it is blocked (k3s-io/k3s#3049 (comment)) by this pull request because flannel is used as a default CNI.

@rajatchopra
Copy link
Contributor

@yaoice Can you please rebase. Wonderful piece of work. Secondly, can we add an end to end test?

Copy link
Contributor

@vadorovsky vadorovsky left a comment

Choose a reason for hiding this comment

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

In general looks good, thanks for working on that feature!

I left some nit comments, mostly about wrapping errors with %w.

@@ -124,13 +124,26 @@ func (dev *vxlanDevice) Configure(ipn ip.IP4Net) error {
return nil
}

func (dev *vxlanDevice) ConfigureIPv6(ipn ip.IP6Net) error {
if err := ip.EnsureV6AddressOnLink(ipn, dev.link); err != nil {
return fmt.Errorf("failed to ensure v6 address of interface %s: %s", dev.link.Attrs().Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("failed to ensure v6 address of interface %s: %s", dev.link.Attrs().Name, err)
return fmt.Errorf("failed to ensure v6 address of interface %s: %w", dev.link.Attrs().Name, err)

}

if err := netlink.LinkSetUp(dev.link); err != nil {
return fmt.Errorf("failed to set v6 interface %s to UP state: %s", dev.link.Attrs().Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("failed to set v6 interface %s to UP state: %s", dev.link.Attrs().Name, err)
return fmt.Errorf("failed to set v6 interface %s to UP state: %w", dev.link.Attrs().Name, err)

return nil, fmt.Errorf("failed to configure interface %s: %s", dev.link.Attrs().Name, err)
if config.EnableIPv4 {
if err := dev.Configure(ip.IP4Net{IP: lease.Subnet.IP, PrefixLen: 32}); err != nil {
return nil, fmt.Errorf("failed to configure interface %s: %s", dev.link.Attrs().Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("failed to configure interface %s: %s", dev.link.Attrs().Name, err)
return nil, fmt.Errorf("failed to configure interface %s: %w", dev.link.Attrs().Name, err)

return newNetwork(be.subnetMgr, be.extIface, dev, ip.IP4Net{}, lease)
if config.EnableIPv6 {
if err := v6Dev.ConfigureIPv6(ip.IP6Net{IP: lease.IPv6Subnet.IP, PrefixLen: 128}); err != nil {
return nil, fmt.Errorf("failed to configure interface %s: %s", v6Dev.link.Attrs().Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil, fmt.Errorf("failed to configure interface %s: %s", v6Dev.link.Attrs().Name, err)
return nil, fmt.Errorf("failed to configure interface %s: %w", v6Dev.link.Attrs().Name, err)

if !existingAddr.IP.IsLinkLocalUnicast() {
if !existingAddr.Equal(addr) {
if err := netlink.AddrDel(link, &existingAddr); err != nil {
return fmt.Errorf("failed to remove v6 IP address %s from %s: %s", ipn.String(), link.Attrs().Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("failed to remove v6 IP address %s from %s: %s", ipn.String(), link.Attrs().Name, err)
return fmt.Errorf("failed to remove v6 IP address %s from %s: %w", ipn.String(), link.Attrs().Name, err)

// Actually add the desired address to the interface if needed.
if len(existingAddrs) == 0 {
if err := netlink.AddrAdd(link, &addr); err != nil {
return fmt.Errorf("failed to add v6 IP address %s to %s: %s", ipn.String(), link.Attrs().Name, err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return fmt.Errorf("failed to add v6 IP address %s to %s: %s", ipn.String(), link.Attrs().Name, err)
return fmt.Errorf("failed to add v6 IP address %s to %s: %w", ipn.String(), link.Attrs().Name, err)

t.Errorf("IPv6Network mismatch: expected %s, got %s", expectedNet, cfg.IPv6Network)
}

if cfg.IPv6SubnetMin.String() != "fc00:0:0:1::" {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe store "fc00:0:0:1::" in a variable (i.e. expectedSubnetMit), similar to expectedNet above?

attrs := event.Lease.Attrs
if attrs.BackendType != "vxlan" {
log.Warningf("ignoring non-vxlan subnet(%s): type=%v", sn, attrs.BackendType)
log.Warningf("ignoring non-vxlan v4Subnet(%s) v6Subnet: type=%v", sn, v6Sn, attrs.BackendType)
Copy link

Choose a reason for hiding this comment

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

Seems like a typo

Suggested change
log.Warningf("ignoring non-vxlan v4Subnet(%s) v6Subnet: type=%v", sn, v6Sn, attrs.BackendType)
log.Warningf("ignoring non-vxlan v4Subnet(%s) v6Subnet(%s): type=%v", sn, v6Sn, attrs.BackendType)

Comment on lines +135 to +136
flannelFlags.BoolVar(&opts.autoDetectIPv4, "auto-detect-ipv4", true, "auto detect ipv4 address of the iface")
flannelFlags.BoolVar(&opts.autoDetectIPv6, "auto-detect-ipv6", false, "auto detect ipv6 address of the iface")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I understand correctly, this flag is basically saying if flannel should be configured in ipv6, ipv4 or dual-stack, right? Can't we remove it and use what is defined under config.EnableIPv6 and config.EnableIPv4 instead?

Comment on lines +539 to +547
case dualStack:
iface, err = ip.GetInterfaceByIP(ifaceAddr)
if err != nil {
return nil, fmt.Errorf("error looking up interface %s: %s", ifname, err)
}
v6Iface, err := ip.GetInterfaceByIP6(ifaceAddr)
if err != nil {
return nil, fmt.Errorf("error looking up v6 interface %s: %s", ifname, err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I am not sure how this will work. If this function is called from line 233 with extIface, err = LookupExtIface(opts.publicIP, "", ipStack), ifaceAddr will be a net.IP ipv4 address. Therefore, when doing ip.GetInterfaceByIP6, err is not going to be nil, and thus it will return an error, right? Similar if opts.publicIP was IPv6, then when going ipGerInterfaceByIP6, things will fail

Comment on lines +680 to +683
v6Iface, err := ip.GetDefaultV6GatewayInterface()
if err != nil {
return nil, fmt.Errorf("failed to get default v6 interface: %s", err)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd better keep the same syntax as ip.GetDefaultGatewayInterface(); err != nil {

Comment on lines +712 to +717
if ifaceAddr != nil {
log.Infof("Using interface with name %s and address %s", iface.Name, ifaceAddr)
}
if ifaceV6Addr != nil {
log.Infof("Using interface with name %s and v6 address %s", iface.Name, ifaceV6Addr)
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

We could add these lines to the previous switch code and avoid the if conditions, right?

Comment on lines +342 to +350
if config.EnableIPv6 {
if err = recycleIP6Tables(config.IPv6Network, bn.Lease()); err != nil {
log.Errorf("Failed to recycle IP6Tables rules, %v", err)
cancel()
wg.Wait()
os.Exit(1)
}
log.Infof("Setting up masking ip6 rules")
go network.SetupAndEnsureIP6Tables(network.MasqIP6Rules(config.IPv6Network, bn.Lease()), opts.iptablesResyncSeconds)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why would we want ipv6 masking?

Comment on lines +145 to +147
} else if cfg.IPv6Network.PrefixLen <= 62 {
// Network is big enough to give each host a /64
cfg.IPv6SubnetLen = 64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any reason why to choose these numbers?

@manuelbuil
Copy link
Collaborator

manuelbuil commented May 28, 2021

Due to the unresponsiveness of the author, this PR #1448 supersedes this one

@manuelbuil
Copy link
Collaborator

We can close this one as #1448 was merged

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

7 participants