From 53bf987984d10fa2159fc1858bdb8be7bc2da86f Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Fri, 16 Sep 2016 22:40:44 -0700 Subject: [PATCH] Fix issue for `--fixed-cidr` when bridge has multiple addresses This fix tries to address the issue raised in: https://github.com/docker/docker/issues/26341 where multiple addresses in a bridge may cause `--fixed-cidr` to not have the correct addresses. The issue is that `netutils.ElectInterfaceAddresses(bridgeName)` only returns the first IPv4 address. This fix changes `ElectInterfaceAddresses()` and `addresses()` so that all IPv4 addresses are returned. This will allow the possibility of selectively choose the address needed. Signed-off-by: Yong Tang --- libnetwork/cmd/dnet/dnet.go | 8 +-- libnetwork/drivers/bridge/bridge_test.go | 10 +-- libnetwork/drivers/bridge/interface.go | 13 ++-- libnetwork/drivers/bridge/interface_test.go | 6 +- libnetwork/drivers/bridge/setup_ipv4.go | 19 +++++- libnetwork/drivers/bridge/setup_verify.go | 6 +- libnetwork/netutils/utils_freebsd.go | 10 +-- libnetwork/netutils/utils_linux.go | 21 ++++--- libnetwork/netutils/utils_solaris.go | 12 ++-- libnetwork/netutils/utils_test.go | 68 +++++++++++++++++---- libnetwork/netutils/utils_windows.go | 10 +-- 11 files changed, 126 insertions(+), 57 deletions(-) diff --git a/libnetwork/cmd/dnet/dnet.go b/libnetwork/cmd/dnet/dnet.go index 2df69d5115..118ea80d3a 100644 --- a/libnetwork/cmd/dnet/dnet.go +++ b/libnetwork/cmd/dnet/dnet.go @@ -507,11 +507,11 @@ func encodeData(data interface{}) (*bytes.Buffer, error) { } func ipamOption(bridgeName string) libnetwork.NetworkOption { - if nw, _, err := netutils.ElectInterfaceAddresses(bridgeName); err == nil { - ipamV4Conf := &libnetwork.IpamConf{PreferredPool: nw.String()} - hip, _ := types.GetHostPartIP(nw.IP, nw.Mask) + if nws, _, err := netutils.ElectInterfaceAddresses(bridgeName); err == nil { + ipamV4Conf := &libnetwork.IpamConf{PreferredPool: nws[0].String()} + hip, _ := types.GetHostPartIP(nws[0].IP, nws[0].Mask) if hip.IsGlobalUnicast() { - ipamV4Conf.Gateway = nw.IP.String() + ipamV4Conf.Gateway = nws[0].IP.String() } return libnetwork.NetworkOptionIpam("default", "", []*libnetwork.IpamConf{ipamV4Conf}, nil, nil) } diff --git a/libnetwork/drivers/bridge/bridge_test.go b/libnetwork/drivers/bridge/bridge_test.go index 403bdcb286..a44a595968 100644 --- a/libnetwork/drivers/bridge/bridge_test.go +++ b/libnetwork/drivers/bridge/bridge_test.go @@ -169,13 +169,13 @@ func compareBindings(a, b []types.PortBinding) bool { func getIPv4Data(t *testing.T, iface string) []driverapi.IPAMData { ipd := driverapi.IPAMData{AddressSpace: "full"} - nw, _, err := netutils.ElectInterfaceAddresses(iface) + nws, _, err := netutils.ElectInterfaceAddresses(iface) if err != nil { t.Fatal(err) } - ipd.Pool = nw + ipd.Pool = nws[0] // Set network gateway to X.X.X.1 - ipd.Gateway = types.GetIPNetCopy(nw) + ipd.Gateway = types.GetIPNetCopy(nws[0]) ipd.Gateway.IP[len(ipd.Gateway.IP)-1] = 1 return []driverapi.IPAMData{ipd} } @@ -1054,12 +1054,12 @@ func TestCreateWithExistingBridge(t *testing.T) { t.Fatalf("Failed to getNetwork(%s): %v", brName, err) } - addr4, _, err := nw.bridge.addresses() + addrs4, _, err := nw.bridge.addresses() if err != nil { t.Fatalf("Failed to get the bridge network's address: %v", err) } - if !addr4.IP.Equal(ip) { + if !addrs4[0].IP.Equal(ip) { t.Fatal("Creating bridge network with existing bridge interface unexpectedly modified the IP address of the bridge") } diff --git a/libnetwork/drivers/bridge/interface.go b/libnetwork/drivers/bridge/interface.go index 16a8c7722d..9b20900416 100644 --- a/libnetwork/drivers/bridge/interface.go +++ b/libnetwork/drivers/bridge/interface.go @@ -52,23 +52,22 @@ func (i *bridgeInterface) exists() bool { return i.Link != nil } -// addresses returns a single IPv4 address and all IPv6 addresses for the -// bridge interface. -func (i *bridgeInterface) addresses() (netlink.Addr, []netlink.Addr, error) { +// addresses returns all IPv4 addresses and all IPv6 addresses for the bridge interface. +func (i *bridgeInterface) addresses() ([]netlink.Addr, []netlink.Addr, error) { v4addr, err := i.nlh.AddrList(i.Link, netlink.FAMILY_V4) if err != nil { - return netlink.Addr{}, nil, fmt.Errorf("Failed to retrieve V4 addresses: %v", err) + return nil, nil, fmt.Errorf("Failed to retrieve V4 addresses: %v", err) } v6addr, err := i.nlh.AddrList(i.Link, netlink.FAMILY_V6) if err != nil { - return netlink.Addr{}, nil, fmt.Errorf("Failed to retrieve V6 addresses: %v", err) + return nil, nil, fmt.Errorf("Failed to retrieve V6 addresses: %v", err) } if len(v4addr) == 0 { - return netlink.Addr{}, v6addr, nil + return nil, v6addr, nil } - return v4addr[0], v6addr, nil + return v4addr, v6addr, nil } func (i *bridgeInterface) programIPv6Address() error { diff --git a/libnetwork/drivers/bridge/interface_test.go b/libnetwork/drivers/bridge/interface_test.go index 0e7dec3b5a..255a709478 100644 --- a/libnetwork/drivers/bridge/interface_test.go +++ b/libnetwork/drivers/bridge/interface_test.go @@ -37,12 +37,12 @@ func TestAddressesEmptyInterface(t *testing.T) { t.Fatalf("newInterface() failed: %v", err) } - addrv4, addrsv6, err := inf.addresses() + addrsv4, addrsv6, err := inf.addresses() if err != nil { t.Fatalf("Failed to get addresses of default interface: %v", err) } - if expected := (netlink.Addr{}); addrv4 != expected { - t.Fatalf("Default interface has unexpected IPv4: %s", addrv4) + if len(addrsv4) != 0 { + t.Fatalf("Default interface has unexpected IPv4: %s", addrsv4) } if len(addrsv6) != 0 { t.Fatalf("Default interface has unexpected IPv6: %v", addrsv6) diff --git a/libnetwork/drivers/bridge/setup_ipv4.go b/libnetwork/drivers/bridge/setup_ipv4.go index f11adc2cb1..3af5836d52 100644 --- a/libnetwork/drivers/bridge/setup_ipv4.go +++ b/libnetwork/drivers/bridge/setup_ipv4.go @@ -3,6 +3,7 @@ package bridge import ( "fmt" "io/ioutil" + "net" "path/filepath" log "github.com/Sirupsen/logrus" @@ -10,12 +11,28 @@ import ( "github.com/vishvananda/netlink" ) +func selectIPv4Address(addresses []netlink.Addr, selector *net.IPNet) (netlink.Addr, error) { + if len(addresses) == 0 { + return netlink.Addr{}, fmt.Errorf("unable to select an address as the address pool is empty") + } + if selector != nil { + for _, addr := range addresses { + if selector.Contains(addr.IP) { + return addr, nil + } + } + } + return addresses[0], nil +} + func setupBridgeIPv4(config *networkConfiguration, i *bridgeInterface) error { - addrv4, _, err := i.addresses() + addrv4List, _, err := i.addresses() if err != nil { return fmt.Errorf("failed to retrieve bridge interface addresses: %v", err) } + addrv4, _ := selectIPv4Address(addrv4List, config.AddressIPv4) + if !types.CompareIPNet(addrv4.IPNet, config.AddressIPv4) { if addrv4.IPNet != nil { if err := i.nlh.AddrDel(i.Link, &addrv4); err != nil { diff --git a/libnetwork/drivers/bridge/setup_verify.go b/libnetwork/drivers/bridge/setup_verify.go index 0bafed6011..ec37d16d06 100644 --- a/libnetwork/drivers/bridge/setup_verify.go +++ b/libnetwork/drivers/bridge/setup_verify.go @@ -11,12 +11,14 @@ import ( ) func setupVerifyAndReconcile(config *networkConfiguration, i *bridgeInterface) error { - // Fetch a single IPv4 and a slice of IPv6 addresses from the bridge. - addrv4, addrsv6, err := i.addresses() + // Fetch a slice of IPv4 addresses and a slice of IPv6 addresses from the bridge. + addrsv4, addrsv6, err := i.addresses() if err != nil { return fmt.Errorf("Failed to verify ip addresses: %v", err) } + addrv4, _ := selectIPv4Address(addrsv4, config.AddressIPv4) + // Verify that the bridge does have an IPv4 address. if addrv4.IPNet == nil { return &ErrNoIPAddr{} diff --git a/libnetwork/netutils/utils_freebsd.go b/libnetwork/netutils/utils_freebsd.go index f7a7ac75f5..02bcd32aa8 100644 --- a/libnetwork/netutils/utils_freebsd.go +++ b/libnetwork/netutils/utils_freebsd.go @@ -7,10 +7,12 @@ import ( ) // ElectInterfaceAddresses looks for an interface on the OS with the specified name -// and returns its IPv4 and IPv6 addresses in CIDR form. If the interface does not exist, -// it chooses from a predifined list the first IPv4 address which does not conflict -// with other interfaces on the system. -func ElectInterfaceAddresses(name string) (*net.IPNet, []*net.IPNet, error) { +// and returns returns all its IPv4 and IPv6 addresses in CIDR notation. +// If a failure in retrieving the addresses or no IPv4 address is found, an error is returned. +// If the interface does not exist, it chooses from a predefined +// list the first IPv4 address which does not conflict with other +// interfaces on the system. +func ElectInterfaceAddresses(name string) ([]*net.IPNet, []*net.IPNet, error) { return nil, nil, types.NotImplementedErrorf("not supported on freebsd") } diff --git a/libnetwork/netutils/utils_linux.go b/libnetwork/netutils/utils_linux.go index 4a03a3a15a..870cc12855 100644 --- a/libnetwork/netutils/utils_linux.go +++ b/libnetwork/netutils/utils_linux.go @@ -62,15 +62,15 @@ func GenerateIfaceName(nlh *netlink.Handle, prefix string, len int) (string, err } // ElectInterfaceAddresses looks for an interface on the OS with the -// specified name and returns its IPv4 and IPv6 addresses in CIDR -// form. If the interface does not exist, it chooses from a predefined +// specified name and returns returns all its IPv4 and IPv6 addresses in CIDR notation. +// If a failure in retrieving the addresses or no IPv4 address is found, an error is returned. +// If the interface does not exist, it chooses from a predefined // list the first IPv4 address which does not conflict with other // interfaces on the system. -func ElectInterfaceAddresses(name string) (*net.IPNet, []*net.IPNet, error) { +func ElectInterfaceAddresses(name string) ([]*net.IPNet, []*net.IPNet, error) { var ( - v4Net *net.IPNet + v4Nets []*net.IPNet v6Nets []*net.IPNet - err error ) defer osl.InitOSContext()() @@ -85,23 +85,24 @@ func ElectInterfaceAddresses(name string) (*net.IPNet, []*net.IPNet, error) { if err != nil { return nil, nil, err } - if len(v4addr) > 0 { - v4Net = v4addr[0].IPNet + for _, nlAddr := range v4addr { + v4Nets = append(v4Nets, nlAddr.IPNet) } for _, nlAddr := range v6addr { v6Nets = append(v6Nets, nlAddr.IPNet) } } - if link == nil || v4Net == nil { + if link == nil || len(v4Nets) == 0 { // Choose from predefined broad networks - v4Net, err = FindAvailableNetwork(ipamutils.PredefinedBroadNetworks) + v4Net, err := FindAvailableNetwork(ipamutils.PredefinedBroadNetworks) if err != nil { return nil, nil, err } + v4Nets = append(v4Nets, v4Net) } - return v4Net, v6Nets, nil + return v4Nets, v6Nets, nil } // FindAvailableNetwork returns a network from the passed list which does not diff --git a/libnetwork/netutils/utils_solaris.go b/libnetwork/netutils/utils_solaris.go index 12d453f7ce..dc67101f4b 100644 --- a/libnetwork/netutils/utils_solaris.go +++ b/libnetwork/netutils/utils_solaris.go @@ -22,10 +22,12 @@ func CheckRouteOverlaps(toCheck *net.IPNet) error { } // ElectInterfaceAddresses looks for an interface on the OS with the specified name -// and returns its IPv4 and IPv6 addresses in CIDR form. If the interface does not exist, -// it chooses from a predifined list the first IPv4 address which does not conflict -// with other interfaces on the system. -func ElectInterfaceAddresses(name string) (*net.IPNet, []*net.IPNet, error) { +// and returns returns all its IPv4 and IPv6 addresses in CIDR notation. +// If a failure in retrieving the addresses or no IPv4 address is found, an error is returned. +// If the interface does not exist, it chooses from a predefined +// list the first IPv4 address which does not conflict with other +// interfaces on the system. +func ElectInterfaceAddresses(name string) ([]*net.IPNet, []*net.IPNet, error) { var ( v4Net *net.IPNet ) @@ -63,7 +65,7 @@ func ElectInterfaceAddresses(name string) (*net.IPNet, []*net.IPNet, error) { return nil, nil, err } } - return v4Net, nil, nil + return []*net.IPNet{v4Net}, nil, nil } // FindAvailableNetwork returns a network from the passed list which does not diff --git a/libnetwork/netutils/utils_test.go b/libnetwork/netutils/utils_test.go index 87fbe6531f..272c53e1f7 100644 --- a/libnetwork/netutils/utils_test.go +++ b/libnetwork/netutils/utils_test.go @@ -5,6 +5,7 @@ package netutils import ( "bytes" "net" + "sort" "testing" "github.com/docker/libnetwork/ipamutils" @@ -265,6 +266,43 @@ func TestNetworkRequest(t *testing.T) { } } +func TestElectInterfaceAddressMultipleAddresses(t *testing.T) { + defer testutils.SetupTestOSContext(t)() + ipamutils.InitNetworks() + + nws := []string{"172.101.202.254/16", "172.102.202.254/16"} + createInterface(t, "test", nws...) + + ipv4NwList, ipv6NwList, err := ElectInterfaceAddresses("test") + if err != nil { + t.Fatal(err) + } + + if len(ipv4NwList) == 0 { + t.Fatalf("unexpected empty ipv4 network addresses") + } + + if len(ipv6NwList) == 0 { + t.Fatalf("unexpected empty ipv6 network addresses") + } + + nwList := []string{} + for _, ipv4Nw := range ipv4NwList { + nwList = append(nwList, ipv4Nw.String()) + } + sort.Strings(nws) + sort.Strings(nwList) + + if len(nws) != len(nwList) { + t.Fatalf("expected %v. got %v", nws, nwList) + } + for i, nw := range nws { + if nw != nwList[i] { + t.Fatalf("expected %v. got %v", nw, nwList[i]) + } + } +} + func TestElectInterfaceAddress(t *testing.T) { defer testutils.SetupTestOSContext(t)() ipamutils.InitNetworks() @@ -277,37 +315,43 @@ func TestElectInterfaceAddress(t *testing.T) { t.Fatal(err) } - if ipv4Nw == nil { + if len(ipv4Nw) == 0 { t.Fatalf("unexpected empty ipv4 network addresses") } if len(ipv6Nw) == 0 { - t.Fatalf("unexpected empty ipv4 network addresses") + t.Fatalf("unexpected empty ipv6 network addresses") } - if nws != ipv4Nw.String() { - t.Fatalf("expected %s. got %s", nws, ipv4Nw) + if nws != ipv4Nw[0].String() { + t.Fatalf("expected %s. got %s", nws, ipv4Nw[0]) } } -func createInterface(t *testing.T, name, nw string) { +func createInterface(t *testing.T, name string, nws ...string) { // Add interface link := &netlink.Bridge{ LinkAttrs: netlink.LinkAttrs{ Name: "test", }, } - bip, err := types.ParseCIDR(nw) - if err != nil { - t.Fatal(err) + bips := []*net.IPNet{} + for _, nw := range nws { + bip, err := types.ParseCIDR(nw) + if err != nil { + t.Fatal(err) + } + bips = append(bips, bip) } - if err = netlink.LinkAdd(link); err != nil { + if err := netlink.LinkAdd(link); err != nil { t.Fatalf("Failed to create interface via netlink: %v", err) } - if err := netlink.AddrAdd(link, &netlink.Addr{IPNet: bip}); err != nil { - t.Fatal(err) + for _, bip := range bips { + if err := netlink.AddrAdd(link, &netlink.Addr{IPNet: bip}); err != nil { + t.Fatal(err) + } } - if err = netlink.LinkSetUp(link); err != nil { + if err := netlink.LinkSetUp(link); err != nil { t.Fatal(err) } } diff --git a/libnetwork/netutils/utils_windows.go b/libnetwork/netutils/utils_windows.go index 3b4bb9d909..b6e79c7538 100644 --- a/libnetwork/netutils/utils_windows.go +++ b/libnetwork/netutils/utils_windows.go @@ -7,10 +7,12 @@ import ( ) // ElectInterfaceAddresses looks for an interface on the OS with the specified name -// and returns its IPv4 and IPv6 addresses in CIDR form. If the interface does not exist, -// it chooses from a predifined list the first IPv4 address which does not conflict -// with other interfaces on the system. -func ElectInterfaceAddresses(name string) (*net.IPNet, []*net.IPNet, error) { +// and returns returns all its IPv4 and IPv6 addresses in CIDR notation. +// If a failure in retrieving the addresses or no IPv4 address is found, an error is returned. +// If the interface does not exist, it chooses from a predefined +// list the first IPv4 address which does not conflict with other +// interfaces on the system. +func ElectInterfaceAddresses(name string) ([]*net.IPNet, []*net.IPNet, error) { return nil, nil, types.NotImplementedErrorf("not supported on windows") }