Merge pull request #2143 from ctelfer/overlay-race-fix

Fix race conditions in the overlay network driver

libnetwork/drivers/overlay/encryption.go

@@ -438,7 +438,7 @@ func (d *driver) setKeys(keys []*key) error {
 	d.keys = keys
 	d.secMap = &encrMap{nodes: map[string][]*spi{}}
 	d.Unlock()
-	logrus.Debugf("Initial encryption keys: %v", d.keys)
+	logrus.Debugf("Initial encryption keys: %v", keys)
 	return nil
 }
 
@@ -458,6 +458,8 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error {
 	)
 
 	d.Lock()
+	defer d.Unlock()
+
 	// add new
 	if newKey != nil {
 		d.keys = append(d.keys, newKey)
@@ -471,7 +473,6 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error {
 			delIdx = i
 		}
 	}
-	d.Unlock()
 
 	if (newKey != nil && newIdx == -1) ||
 		(primary != nil && priIdx == -1) ||
@@ -480,17 +481,18 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error {
 			"(newIdx,priIdx,delIdx):(%d, %d, %d)", newIdx, priIdx, delIdx)
 	}
 
+	if priIdx != -1 && priIdx == delIdx {
+		return types.BadRequestErrorf("attempting to both make a key (index %d) primary and delete it", priIdx)
+	}
+
 	d.secMapWalk(func(rIPs string, spis []*spi) ([]*spi, bool) {
 		rIP := net.ParseIP(rIPs)
 		return updateNodeKey(lIP, aIP, rIP, spis, d.keys, newIdx, priIdx, delIdx), false
 	})
 
-	d.Lock()
 	// swap primary
 	if priIdx != -1 {
-		swp := d.keys[0]
-		d.keys[0] = d.keys[priIdx]
-		d.keys[priIdx] = swp
+		d.keys[0], d.keys[priIdx] = d.keys[priIdx], d.keys[0]
 	}
 	// prune
 	if delIdx != -1 {
@@ -499,7 +501,6 @@ func (d *driver) updateKeys(newKey, primary, pruneKey *key) error {
 		}
 		d.keys = append(d.keys[:delIdx], d.keys[delIdx+1:]...)
 	}
-	d.Unlock()
 
 	logrus.Debugf("Updated: %v", d.keys)
 

libnetwork/drivers/overlay/ov_network.go

@@ -203,6 +203,12 @@ func (d *driver) CreateNetwork(id string, option map[string]interface{}, nInfo d
 		n.subnets = append(n.subnets, s)
 	}
 
+	d.Lock()
+	defer d.Unlock()
+	if d.networks[n.id] != nil {
+		return fmt.Errorf("attempt to create overlay network %v that already exists", n.id)
+	}
+
 	if err := n.writeToStore(); err != nil {
 		return fmt.Errorf("failed to update data store for network %v: %v", n.id, err)
 	}
@@ -217,11 +223,13 @@ func (d *driver) CreateNetwork(id string, option map[string]interface{}, nInfo d
 
 	if nInfo != nil {
 		if err := nInfo.TableEventRegister(ovPeerTable, driverapi.EndpointObject); err != nil {
+			// XXX Undo writeToStore?  No method to so.  Why?
 			return err
 		}
 	}
 
-	d.addNetwork(n)
+	d.networks[id] = n
+
 	return nil
 }
 
@@ -235,7 +243,15 @@ func (d *driver) DeleteNetwork(nid string) error {
 		return err
 	}
 
-	n := d.network(nid)
+	d.Lock()
+	defer d.Unlock()
+
+	// This is similar to d.network(), but we need to keep holding the lock
+	// until we are done removing this network.
+	n, ok := d.networks[nid]
+	if !ok {
+		n = d.restoreNetworkFromStore(nid)
+	}
 	if n == nil {
 		return fmt.Errorf("could not find network with id %s", nid)
 	}
@@ -255,7 +271,7 @@ func (d *driver) DeleteNetwork(nid string) error {
 	}
 	// flush the peerDB entries
 	d.peerFlush(nid)
-	d.deleteNetwork(nid)
+	delete(d.networks, nid)
 
 	vnis, err := n.releaseVxlanID()
 	if err != nil {
@@ -805,32 +821,25 @@ func (n *network) watchMiss(nlSock *nl.NetlinkSocket, nsPath string) {
 	}
 }
 
-func (d *driver) addNetwork(n *network) {
-	d.Lock()
-	d.networks[n.id] = n
-	d.Unlock()
-}
-
-func (d *driver) deleteNetwork(nid string) {
-	d.Lock()
-	delete(d.networks, nid)
-	d.Unlock()
+// Restore a network from the store to the driver if it is present.
+// Must be called with the driver locked!
+func (d *driver) restoreNetworkFromStore(nid string) *network {
+	n := d.getNetworkFromStore(nid)
+	if n != nil {
+		n.driver = d
+		n.endpoints = endpointTable{}
+		n.once = &sync.Once{}
+		d.networks[nid] = n
+	}
+	return n
 }
 
 func (d *driver) network(nid string) *network {
 	d.Lock()
+	defer d.Unlock()
 	n, ok := d.networks[nid]
-	d.Unlock()
 	if !ok {
-		n = d.getNetworkFromStore(nid)
-		if n != nil {
-			n.driver = d
-			n.endpoints = endpointTable{}
-			n.once = &sync.Once{}
-			d.Lock()
-			d.networks[nid] = n
-			d.Unlock()
-		}
+		n = d.restoreNetworkFromStore(nid)
 	}
 
 	return n

libnetwork/drivers/overlay/ovmanager/ovmanager.go

@@ -125,8 +125,12 @@ func (d *driver) NetworkAllocate(id string, option map[string]string, ipV4Data,
 	opts[netlabel.OverlayVxlanIDList] = val
 
 	d.Lock()
+	defer d.Unlock()
+	if _, ok := d.networks[id]; ok {
+		n.releaseVxlanID()
+		return nil, fmt.Errorf("network %s already exists", id)
+	}
 	d.networks[id] = n
-	d.Unlock()
 
 	return opts, nil
 }
@@ -137,8 +141,8 @@ func (d *driver) NetworkFree(id string) error {
 	}
 
 	d.Lock()
+	defer d.Unlock()
 	n, ok := d.networks[id]
-	d.Unlock()
 
 	if !ok {
 		return fmt.Errorf("overlay network with id %s not found", id)
@@ -147,9 +151,7 @@ func (d *driver) NetworkFree(id string) error {
 	// Release all vxlan IDs in one shot.
 	n.releaseVxlanID()
 
-	d.Lock()
 	delete(d.networks, id)
-	d.Unlock()
 
 	return nil
 }