From 36eba748fff85d7f82c88ce06d650d286cf90914 Mon Sep 17 00:00:00 2001 From: Alessandro Boch Date: Mon, 27 Jul 2015 16:48:30 -0700 Subject: [PATCH 1/5] Misc fixes to ipallocator & bridge driver about FixedCIDR - NetworkRange() function on which ipallocatore relies to compute the subnet limits has a bug in computing the upper limit IP - in case container subnet is specified (fixedCIDR), bridge driver to reserve bridge and gateway addresses only if they belong to the container subnet - Make ipallocator more robust in using converting the passed network to a canonical one before using it as a key in its public APIs Signed-off-by: Alessandro Boch --- drivers/bridge/bridge.go | 6 ++---- drivers/bridge/bridge_test.go | 11 +++++++++++ drivers/bridge/setup_ipv4.go | 20 +++++++++++++------- drivers/bridge/setup_ipv4_test.go | 2 +- ipallocator/allocator.go | 11 +++++++---- netutils/utils.go | 22 ++++++++++++---------- 6 files changed, 46 insertions(+), 26 deletions(-) diff --git a/drivers/bridge/bridge.go b/drivers/bridge/bridge.go index 57a7f57..22bdf37 100644 --- a/drivers/bridge/bridge.go +++ b/drivers/bridge/bridge.go @@ -947,8 +947,7 @@ func (d *driver) CreateEndpoint(nid, eid types.UUID, epInfo driverapi.EndpointIn } // v4 address for the sandbox side pipe interface - sub := types.GetIPNetCanonical(n.bridge.bridgeIPv4) - ip4, err := ipAllocator.RequestIP(sub, nil) + ip4, err := ipAllocator.RequestIP(n.bridge.bridgeIPv4, nil) if err != nil { return err } @@ -1074,8 +1073,7 @@ func (d *driver) DeleteEndpoint(nid, eid types.UUID) error { n.releasePorts(ep) // Release the v4 address allocated to this endpoint's sandbox interface - sub := types.GetIPNetCanonical(n.bridge.bridgeIPv4) - err = ipAllocator.ReleaseIP(sub, ep.addr.IP) + err = ipAllocator.ReleaseIP(n.bridge.bridgeIPv4, ep.addr.IP) if err != nil { return err } diff --git a/drivers/bridge/bridge_test.go b/drivers/bridge/bridge_test.go index 760e3f6..ffd2c94 100644 --- a/drivers/bridge/bridge_test.go +++ b/drivers/bridge/bridge_test.go @@ -54,6 +54,17 @@ func TestCreateFullOptions(t *testing.T) { if err != nil { t.Fatalf("Failed to create bridge: %v", err) } + + // Verify the IP address allocated for the endpoint belongs to the container network + epOptions := make(map[string]interface{}) + te := &testEndpoint{ifaces: []*testInterface{}} + err = d.CreateEndpoint("dummy", "ep1", te, epOptions) + if err != nil { + t.Fatalf("Failed to create an endpoint : %s", err.Error()) + } + if !cnw.Contains(te.Interfaces()[0].Address().IP) { + t.Fatalf("endpoint got assigned address outside of container network(%s): %s", cnw.String(), te.Interfaces()[0].Address()) + } } func TestCreate(t *testing.T) { diff --git a/drivers/bridge/setup_ipv4.go b/drivers/bridge/setup_ipv4.go index cca715e..ac4535a 100644 --- a/drivers/bridge/setup_ipv4.go +++ b/drivers/bridge/setup_ipv4.go @@ -8,7 +8,6 @@ import ( log "github.com/Sirupsen/logrus" "github.com/docker/libnetwork/netutils" - "github.com/docker/libnetwork/types" "github.com/vishvananda/netlink" ) @@ -76,8 +75,12 @@ func setupBridgeIPv4(config *networkConfiguration, i *bridgeInterface) error { } func allocateBridgeIP(config *networkConfiguration, i *bridgeInterface) error { - sub := types.GetIPNetCanonical(i.bridgeIPv4) - ipAllocator.RequestIP(sub, i.bridgeIPv4.IP) + // Because of the way ipallocator manages the container address space, + // reserve bridge address only if it belongs to the container network + // (if defined), no need otherwise + if config.FixedCIDR == nil || config.FixedCIDR.Contains(i.bridgeIPv4.IP) { + ipAllocator.RequestIP(i.bridgeIPv4, i.bridgeIPv4.IP) + } return nil } @@ -112,10 +115,13 @@ func setupGatewayIPv4(config *networkConfiguration, i *bridgeInterface) error { return &ErrInvalidGateway{} } - // Pass the real network subnet to ip allocator (no host bits set) - sub := types.GetIPNetCanonical(i.bridgeIPv4) - if _, err := ipAllocator.RequestIP(sub, config.DefaultGatewayIPv4); err != nil { - return err + // Because of the way ipallocator manages the container address space, + // reserve default gw address only if it belongs to the container network + // (if defined), no need otherwise + if config.FixedCIDR == nil || config.FixedCIDR.Contains(config.DefaultGatewayIPv4) { + if _, err := ipAllocator.RequestIP(i.bridgeIPv4, config.DefaultGatewayIPv4); err != nil { + return err + } } // Store requested default gateway diff --git a/drivers/bridge/setup_ipv4_test.go b/drivers/bridge/setup_ipv4_test.go index 0d26a79..8564737 100644 --- a/drivers/bridge/setup_ipv4_test.go +++ b/drivers/bridge/setup_ipv4_test.go @@ -82,7 +82,7 @@ func TestSetupGatewayIPv4(t *testing.T) { ip, nw, _ := net.ParseCIDR("192.168.0.24/16") nw.IP = ip - gw := net.ParseIP("192.168.0.254") + gw := net.ParseIP("192.168.2.254") config := &networkConfiguration{ BridgeName: DefaultBridgeName, diff --git a/ipallocator/allocator.go b/ipallocator/allocator.go index 1560099..06bc051 100644 --- a/ipallocator/allocator.go +++ b/ipallocator/allocator.go @@ -66,7 +66,8 @@ func (a *IPAllocator) RegisterSubnet(network *net.IPNet, subnet *net.IPNet) erro a.mutex.Lock() defer a.mutex.Unlock() - key := network.String() + nw := &net.IPNet{IP: network.IP.Mask(network.Mask), Mask: network.Mask} + key := nw.String() if _, ok := a.allocatedIPs[key]; ok { return ErrNetworkAlreadyRegistered } @@ -90,10 +91,11 @@ func (a *IPAllocator) RequestIP(network *net.IPNet, ip net.IP) (net.IP, error) { a.mutex.Lock() defer a.mutex.Unlock() - key := network.String() + nw := &net.IPNet{IP: network.IP.Mask(network.Mask), Mask: network.Mask} + key := nw.String() allocated, ok := a.allocatedIPs[key] if !ok { - allocated = newAllocatedMap(network) + allocated = newAllocatedMap(nw) a.allocatedIPs[key] = allocated } @@ -109,7 +111,8 @@ func (a *IPAllocator) ReleaseIP(network *net.IPNet, ip net.IP) error { a.mutex.Lock() defer a.mutex.Unlock() - if allocated, exists := a.allocatedIPs[network.String()]; exists { + nw := &net.IPNet{IP: network.IP.Mask(network.Mask), Mask: network.Mask} + if allocated, exists := a.allocatedIPs[nw.String()]; exists { delete(allocated.p, ip.String()) } return nil diff --git a/netutils/utils.go b/netutils/utils.go index 0ef357e..cb430eb 100644 --- a/netutils/utils.go +++ b/netutils/utils.go @@ -74,20 +74,22 @@ func NetworkOverlaps(netX *net.IPNet, netY *net.IPNet) bool { // NetworkRange calculates the first and last IP addresses in an IPNet func NetworkRange(network *net.IPNet) (net.IP, net.IP) { - var netIP net.IP - if network.IP.To4() != nil { - netIP = network.IP.To4() - } else if network.IP.To16() != nil { - netIP = network.IP.To16() - } else { + if network == nil { return nil, nil } - lastIP := make([]byte, len(netIP), len(netIP)) - for i := 0; i < len(netIP); i++ { - lastIP[i] = netIP[i] | ^network.Mask[i] + firstIP := network.IP.Mask(network.Mask) + lastIP := types.GetIPCopy(firstIP) + for i := 0; i < len(firstIP); i++ { + lastIP[i] = firstIP[i] | ^network.Mask[i] } - return netIP.Mask(network.Mask), net.IP(lastIP) + + if network.IP.To4() != nil { + firstIP = firstIP.To4() + lastIP = lastIP.To4() + } + + return firstIP, lastIP } // GetIfaceAddr returns the first IPv4 address and slice of IPv6 addresses for the specified network interface From 94ebc4189c0b53952c10adfb5a503bc743a96ed7 Mon Sep 17 00:00:00 2001 From: Shijiang Wei Date: Wed, 29 Jul 2015 20:15:02 +0800 Subject: [PATCH 2/5] make sure the interfaces is cleared on error Signed-off-by: Shijiang Wei --- drivers/bridge/bridge.go | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/drivers/bridge/bridge.go b/drivers/bridge/bridge.go index 22bdf37..0faa758 100644 --- a/drivers/bridge/bridge.go +++ b/drivers/bridge/bridge.go @@ -596,21 +596,18 @@ func (d *driver) CreateNetwork(id types.UUID, option map[string]interface{}) err // networks. This step is needed now because driver might have now set the bridge // name on this config struct. And because we need to check for possible address // conflicts, so we need to check against operationa lnetworks. - if err := config.conflictsWithNetworks(id, networkList); err != nil { + if err = config.conflictsWithNetworks(id, networkList); err != nil { return err } setupNetworkIsolationRules := func(config *networkConfiguration, i *bridgeInterface) error { - defer func() { - if err != nil { - if err := network.isolateNetwork(networkList, false); err != nil { - logrus.Warnf("Failed on removing the inter-network iptables rules on cleanup: %v", err) - } + if err := network.isolateNetwork(networkList, true); err != nil { + if err := network.isolateNetwork(networkList, false); err != nil { + logrus.Warnf("Failed on removing the inter-network iptables rules on cleanup: %v", err) } - }() - - err := network.isolateNetwork(networkList, true) - return err + return err + } + return nil } // Prepare the bridge setup configuration @@ -954,7 +951,7 @@ func (d *driver) CreateEndpoint(nid, eid types.UUID, epInfo driverapi.EndpointIn ipv4Addr := &net.IPNet{IP: ip4, Mask: n.bridge.bridgeIPv4.Mask} // Down the interface before configuring mac address. - if err := netlink.LinkSetDown(sbox); err != nil { + if err = netlink.LinkSetDown(sbox); err != nil { return fmt.Errorf("could not set link down for container interface %s: %v", containerIfName, err) } @@ -967,7 +964,7 @@ func (d *driver) CreateEndpoint(nid, eid types.UUID, epInfo driverapi.EndpointIn endpoint.macAddress = mac // Up the host interface after finishing all netlink configuration - if err := netlink.LinkSetUp(host); err != nil { + if err = netlink.LinkSetUp(host); err != nil { return fmt.Errorf("could not set link up for host interface %s: %v", hostIfName, err) } From 2fd97c1dd947cdff3950c2820c0ee2b49fa7c0c8 Mon Sep 17 00:00:00 2001 From: Alessandro Boch Date: Wed, 29 Jul 2015 17:23:55 -0700 Subject: [PATCH 3/5] Incorrect kernel version check in bridge - Kernel version check logic was wrong Signed-off-by: Alessandro Boch --- drivers/bridge/setup_device.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/drivers/bridge/setup_device.go b/drivers/bridge/setup_device.go index 96eeee5..f58be98 100644 --- a/drivers/bridge/setup_device.go +++ b/drivers/bridge/setup_device.go @@ -1,6 +1,7 @@ package bridge import ( + "github.com/Sirupsen/logrus" "github.com/docker/docker/pkg/parsers/kernel" "github.com/vishvananda/netlink" ) @@ -25,8 +26,10 @@ func setupDevice(config *networkConfiguration, i *bridgeInterface) error { // Only set the bridge's MAC address if the kernel version is > 3.3, as it // was not supported before that. kv, err := kernel.GetKernelVersion() - if err == nil && (kv.Kernel >= 3 && kv.Major >= 3) { - setMac = true + if err != nil { + logrus.Errorf("Failed to check kernel versions: %v. Will not assign a MAC address to the bridge interface", err) + } else { + setMac = kv.Kernel > 3 || (kv.Kernel == 3 && kv.Major >= 3) } return ioctlCreateBridge(config.BridgeName, setMac) From f126f5ae63cad33b47b036e583d1653ba450222a Mon Sep 17 00:00:00 2001 From: Jana Radhakrishnan Date: Wed, 29 Jul 2015 23:57:50 -0700 Subject: [PATCH 4/5] Fix data race in controller sandboxes The controller sandboxes hashmap is not being protected by a lock while deleting it in `LeaveAll` call. This may result in a race whereby any other read access that happens with the lock held is also vulnerable to return random sandbox data which could result in totally unpredictable behavior. Also as part of the fix check if `s.endpoints` is empty and log an error in `rmEndpoint` so that we don't bring down the process for this unexpected error. Signed-off-by: Jana Radhakrishnan --- libnetwork_test.go | 81 ++++++++++++++++++++++++++++++++++++---------- sandboxdata.go | 14 ++++++-- 2 files changed, 75 insertions(+), 20 deletions(-) diff --git a/libnetwork_test.go b/libnetwork_test.go index a139331..3bf62bf 100644 --- a/libnetwork_test.go +++ b/libnetwork_test.go @@ -1919,14 +1919,30 @@ func createGlobalInstance(t *testing.T) { "AllowNonDefaultBridge": true, }, } - net, err := createTestNetwork(bridgeNetType, "network", netOption) + + net1, err := controller.NetworkByName("testhost") if err != nil { - t.Fatal("new network") + t.Fatal(err) } - _, err = net.CreateEndpoint("ep1") + net2, err := createTestNetwork("bridge", "network2", netOption) if err != nil { - t.Fatal("createendpoint") + t.Fatal(err) + } + + _, err = net1.CreateEndpoint("pep1") + if err != nil { + t.Fatal(err) + } + + _, err = net2.CreateEndpoint("pep2") + if err != nil { + t.Fatal(err) + } + + _, err = net2.CreateEndpoint("pep3") + if err != nil { + t.Fatal(err) } } @@ -1940,12 +1956,18 @@ func debugf(format string, a ...interface{}) (int, error) { func parallelJoin(t *testing.T, ep libnetwork.Endpoint, thrNumber int) { debugf("J%d.", thrNumber) - err := ep.Join("racing_container") + var err error + if thrNumber == first { + err = ep.Join(fmt.Sprintf("%drace", thrNumber), libnetwork.JoinOptionUseDefaultSandbox()) + } else { + err = ep.Join(fmt.Sprintf("%drace", thrNumber)) + } + runtime.LockOSThread() if err != nil { if _, ok := err.(libnetwork.ErrNoContainer); !ok { if _, ok := err.(libnetwork.ErrInvalidJoin); !ok { - t.Fatal(err) + t.Fatalf("thread %d: %v", thrNumber, err) } } debugf("JE%d(%v).", thrNumber, err) @@ -1955,12 +1977,18 @@ func parallelJoin(t *testing.T, ep libnetwork.Endpoint, thrNumber int) { func parallelLeave(t *testing.T, ep libnetwork.Endpoint, thrNumber int) { debugf("L%d.", thrNumber) - err := ep.Leave("racing_container") + var err error + if thrNumber == first { + err = ep.Leave(fmt.Sprintf("%drace", thrNumber)) + } else { + err = controller.LeaveAll(fmt.Sprintf("%drace", thrNumber)) + } + runtime.LockOSThread() if err != nil { if _, ok := err.(libnetwork.ErrNoContainer); !ok { if _, ok := err.(libnetwork.ErrInvalidJoin); !ok { - t.Fatal(err) + t.Fatalf("thread %d: %v", thrNumber, err) } } debugf("LE%d(%v).", thrNumber, err) @@ -2012,15 +2040,33 @@ func runParallelTests(t *testing.T, thrNumber int) { } defer netns.Set(origns) - net, err := controller.NetworkByName("network") + net1, err := controller.NetworkByName("testhost") if err != nil { t.Fatal(err) } - if net == nil { - t.Fatal("Could not find network") + if net1 == nil { + t.Fatal("Could not find network1") + } + + net2, err := controller.NetworkByName("network2") + if err != nil { + t.Fatal(err) + } + if net2 == nil { + t.Fatal("Could not find network2") + } + + epName := fmt.Sprintf("pep%d", thrNumber) + + //var err error + var ep libnetwork.Endpoint + + if thrNumber == first { + ep, err = net1.EndpointByName(epName) + } else { + ep, err = net2.EndpointByName(epName) } - ep, err := net.EndpointByName("ep1") if err != nil { t.Fatal(err) } @@ -2035,6 +2081,11 @@ func runParallelTests(t *testing.T, thrNumber int) { debugf("\n") + err = ep.Delete() + if err != nil { + t.Fatal(err) + } + if thrNumber == first { for thrdone := range done { select { @@ -2043,12 +2094,8 @@ func runParallelTests(t *testing.T, thrNumber int) { } testns.Close() - err = ep.Delete() - if err != nil { - t.Fatal(err) - } - if err := net.Delete(); err != nil { + if err := net2.Delete(); err != nil { t.Fatal(err) } } diff --git a/sandboxdata.go b/sandboxdata.go index 6b217a8..9b0d8ea 100644 --- a/sandboxdata.go +++ b/sandboxdata.go @@ -139,10 +139,15 @@ func (s *sandboxData) rmEndpoint(ep *endpoint) { } } - // We don't check if s.endpoints is empty here because - // it should never be empty during a rmEndpoint call and - // if it is we will rightfully panic here s.Lock() + if len(s.endpoints) == 0 { + // s.endpoints should never be empty and this is unexpected error condition + // We log an error message to note this down for debugging purposes. + logrus.Errorf("No endpoints in sandbox while trying to remove endpoint %s", ep.Name()) + s.Unlock() + return + } + highEpBefore := s.endpoints[0] var ( i int @@ -245,7 +250,10 @@ func (c *controller) LeaveAll(id string) error { } sData.sandbox().Destroy() + + c.Lock() delete(c.sandboxes, sandbox.GenerateKey(id)) + c.Unlock() return nil } From 8b4810d3ace6b300bd84f96108e28ff613c94e41 Mon Sep 17 00:00:00 2001 From: Madhu Venugopal Date: Thu, 30 Jul 2015 06:02:23 -0700 Subject: [PATCH 5/5] Prefer Netlink calls over ioctl As seen in https://github.com/docker/docker/issues/14738 there is general instability in the later kernels under race conditions when ioctl calls are used in parallel with netlink calls for various operations. (We are yet to narrow down to the exact root-cause on the kernel). For those older kernels which doesnt support some of the netlink APIs, we can fallback to using ioctl calls. Hence bringing back the original code that used netlink (https://github.com/docker/libnetwork/pull/349). Also, there was an existing bug in bridge creation using netlink which was setting bridge mac during bridge creation. That operation is not supported in the netlink library (and doesnt throw an error either). Included a fix for that condition by setting the bridge mac after creating the bridge. Signed-off-by: Madhu Venugopal --- drivers/bridge/bridge.go | 21 +++++++++++++++------ drivers/bridge/setup_device.go | 17 ++++++++++++++++- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/drivers/bridge/bridge.go b/drivers/bridge/bridge.go index 0faa758..8fc05ae 100644 --- a/drivers/bridge/bridge.go +++ b/drivers/bridge/bridge.go @@ -763,17 +763,26 @@ func (d *driver) DeleteNetwork(nid types.UUID) error { } func addToBridge(ifaceName, bridgeName string) error { - iface, err := net.InterfaceByName(ifaceName) + link, err := netlink.LinkByName(ifaceName) if err != nil { return fmt.Errorf("could not find interface %s: %v", ifaceName, err) } + if err = netlink.LinkSetMaster(link, + &netlink.Bridge{LinkAttrs: netlink.LinkAttrs{Name: bridgeName}}); err != nil { + logrus.Debugf("Failed to add %s to bridge via netlink.Trying ioctl: %v", ifaceName, err) + iface, err := net.InterfaceByName(ifaceName) + if err != nil { + return fmt.Errorf("could not find network interface %s: %v", ifaceName, err) + } - master, err := net.InterfaceByName(bridgeName) - if err != nil { - return fmt.Errorf("could not find bridge %s: %v", bridgeName, err) + master, err := net.InterfaceByName(bridgeName) + if err != nil { + return fmt.Errorf("could not find bridge %s: %v", bridgeName, err) + } + + return ioctlAddToBridge(iface, master) } - - return ioctlAddToBridge(iface, master) + return nil } func setHairpinMode(link netlink.Link, enable bool) error { diff --git a/drivers/bridge/setup_device.go b/drivers/bridge/setup_device.go index f58be98..22bf64b 100644 --- a/drivers/bridge/setup_device.go +++ b/drivers/bridge/setup_device.go @@ -1,8 +1,11 @@ package bridge import ( + "fmt" + "github.com/Sirupsen/logrus" "github.com/docker/docker/pkg/parsers/kernel" + "github.com/docker/libnetwork/netutils" "github.com/vishvananda/netlink" ) @@ -32,7 +35,19 @@ func setupDevice(config *networkConfiguration, i *bridgeInterface) error { setMac = kv.Kernel > 3 || (kv.Kernel == 3 && kv.Major >= 3) } - return ioctlCreateBridge(config.BridgeName, setMac) + if err = netlink.LinkAdd(i.Link); err != nil { + logrus.Debugf("Failed to create bridge %s via netlink. Trying ioctl", config.BridgeName) + return ioctlCreateBridge(config.BridgeName, setMac) + } + + if setMac { + hwAddr := netutils.GenerateRandomMAC() + if err = netlink.LinkSetHardwareAddr(i.Link, hwAddr); err != nil { + return fmt.Errorf("failed to set bridge mac-address %s : %s", hwAddr, err.Error()) + } + logrus.Debugf("Setting bridge mac address to %s", hwAddr) + } + return err } // SetupDeviceUp ups the given bridge interface.