Merge pull request #2577 from weaveworks/2548-fix-reclaim-after-using-cni

Fix reclaim after using CNI; Fixes #2548.

api/ipam.go

@@ -5,6 +5,15 @@ import (
 	"net"
 )
 
+// Special token used in place of a container identifier when:
+// - the caller does not know the container ID, or
+// - the caller is unsure whether IPAM was previously told the container ID.
+//
+// While addresses are typically stored under their container IDs, when this
+// token is used, the address will be stored under its own string, something
+// which should be kept in mind if the entry needs to be removed at any point.
+const NoContainerID string = "_"
+
 func (client *Client) ipamOp(ID string, op string) (*net.IPNet, error) {
 	ip, err := client.httpVerb(op, fmt.Sprintf("/ip/%s", ID), nil)
 	if err != nil {

ipam/allocate.go

@@ -3,6 +3,7 @@ package ipam
 import (
 	"fmt"
 
+	"github.com/weaveworks/weave/api"
 	"github.com/weaveworks/weave/net/address"
 )
 
@@ -43,7 +44,7 @@ func (g *allocate) Try(alloc *Allocator) bool {
 	if ok, addr := alloc.space.Allocate(g.r.HostRange()); ok {
 		// If caller hasn't supplied a unique ID, file it under the IP address
 		// which lets the caller then release the address using DELETE /ip/address
-		if g.ident == "_" {
+		if g.ident == api.NoContainerID {
 			g.ident = addr.String()
 		}
 		alloc.debugln("Allocated", addr, "for", g.ident, "in", g.r)

ipam/claim.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/weaveworks/mesh"
 
+	"github.com/weaveworks/weave/api"
 	"github.com/weaveworks/weave/common"
 	"github.com/weaveworks/weave/net/address"
 )
@@ -77,26 +78,34 @@ func (c *claim) Try(alloc *Allocator) bool {
 		return false
 	}
 
-	if c.ident == "_" { // Special "I don't have a unique ID" identifier
-		c.ident = c.cidr.Addr.String()
-	}
 	// We are the owner, check we haven't given it to another container
-	switch existingIdent := alloc.findOwner(c.cidr.Addr); existingIdent {
-	case "":
+	existingIdent := alloc.findOwner(c.cidr.Addr)
+	switch {
+	case existingIdent == "":
+		// Unused address, we try to claim it:
 		if err := alloc.space.Claim(c.cidr.Addr); err == nil {
 			alloc.debugln("Claimed", c.cidr, "for", c.ident)
-			alloc.addOwned(c.ident, c.cidr, c.isContainer)
+			if c.ident == api.NoContainerID {
+				alloc.addOwned(c.cidr.Addr.String(), c.cidr, c.isContainer)
+			} else {
+				alloc.addOwned(c.ident, c.cidr, c.isContainer)
+			}
 			c.sendResult(nil)
 		} else {
 			c.sendResult(err)
 		}
-	case c.ident:
+	case (existingIdent == c.ident) || (c.ident == api.NoContainerID && existingIdent == c.cidr.Addr.String()):
 		// same identifier is claiming same address; that's OK
 		alloc.debugln("Re-Claimed", c.cidr, "for", c.ident)
 		c.sendResult(nil)
-	case c.cidr.Addr.String():
-		// Address already allocated via "_" name
+	case existingIdent == c.cidr.Addr.String():
+		// Address already allocated via api.NoContainerID name and current ID is a real container ID:
 		c.sendResult(fmt.Errorf("address %s already in use", c.cidr))
+	case c.ident == api.NoContainerID:
+		// We do not know whether this is the same container or another one,
+		// but we also cannot prove otherwise, so we let it reclaim the address:
+		alloc.debugln("Re-Claimed", c.cidr, "for ID", c.ident, "having existing ID as", existingIdent)
+		c.sendResult(nil)
 	default:
 		// Addr already owned by container on this machine
 		c.sendResult(fmt.Errorf("address %s is already owned by %s", c.cidr.String(), existingIdent))

plugin/ipam/driver.go

@@ -72,9 +72,8 @@ func splitPoolID(poolID string) (subnet, iprange *net.IPNet, err error) {
 func (i *Ipam) RequestAddress(poolID string, address net.IP, options map[string]string) (ip *net.IPNet, _ map[string]string, err error) {
 	i.logReq("RequestAddress", poolID, address, options)
 	defer func() { i.logRes("RequestAddress", err, ip) }()
-	// If we pass magic string "_" to weave IPAM it stores the address under its own string
 	if poolID == "weavepool" { // old-style
-		ip, err = i.weave.AllocateIP("_")
+		ip, err = i.weave.AllocateIP(api.NoContainerID)
 		return
 	}
 	subnet, iprange, err := splitPoolID(poolID)
@@ -83,12 +82,12 @@ func (i *Ipam) RequestAddress(poolID string, address net.IP, options map[string]
 	}
 	if address != nil { // try to claim specific address requested
 		ip = &net.IPNet{IP: address, Mask: subnet.Mask}
-		if err = i.weave.ClaimIP("_", ip); err != nil {
+		if err = i.weave.ClaimIP(api.NoContainerID, ip); err != nil {
 			return
 		}
 	} else {
 		// We are lying slightly to IPAM here: the range is not a subnet
-		if ip, err = i.weave.AllocateIPInSubnet("_", iprange); err != nil {
+		if ip, err = i.weave.AllocateIPInSubnet(api.NoContainerID, iprange); err != nil {
 			return
 		}
 		ip.Mask = subnet.Mask // fix up the subnet we lied about

test/830_cni_plugin_test.sh

@@ -48,12 +48,28 @@ assert_raises "exec_on $HOST1 c2 $PING $C1IP"
 # Now remove and start a new container to see if IP address re-use breaks things
 docker_on $HOST1 rm -f c2
 
-docker_on $HOST1 run --net=none --name=c3 -dt $SMALL_IMAGE /bin/sh
+C3=$(docker_on $HOST1 run --net=none --name=c3 -dt $SMALL_IMAGE /bin/sh)
 
 cni_connect $HOST1 c3 <<EOF
 { "name": "weave", "type": "weave-net" }
 EOF
 
+C3IP=$(container_ip $HOST1 c3)
+
 assert_raises "exec_on $HOST1 c1 $PING $C2IP"
 
+
+# Ensure existing containers can reclaim their IP addresses after CNI has been used -- see #2548
+stop_weave_on $HOST1
+
+# Ensure no warning is printed to the standard error:
+ACTUAL_OUTPUT=$(weave_on $HOST1 launch-router 2>&1)
+EXPECTED_OUTPUT=$(docker_on $HOST1 inspect --format="{{.Id}}" weave)
+
+assert_raises "[ $EXPECTED_OUTPUT == $ACTUAL_OUTPUT ]"
+
+assert "$SSH $HOST1 \"curl -s -X GET 127.0.0.1:6784/ip/$C1 | grep -o -E '[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}'\"" "$C1IP"
+assert "$SSH $HOST1 \"curl -s -X GET 127.0.0.1:6784/ip/$C3 | grep -o -E '[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}'\"" "$C3IP"
+
+
 end_suite