diff --git a/api/ipam.go b/api/ipam.go index 3f15af7f..b42e1959 100644 --- a/api/ipam.go +++ b/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 { diff --git a/ipam/allocate.go b/ipam/allocate.go index c144edfd..ac542aac 100644 --- a/ipam/allocate.go +++ b/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) diff --git a/ipam/claim.go b/ipam/claim.go index fda36f15..e027f723 100644 --- a/ipam/claim.go +++ b/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)) diff --git a/plugin/ipam/driver.go b/plugin/ipam/driver.go index 31436995..417e468f 100644 --- a/plugin/ipam/driver.go +++ b/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 diff --git a/test/830_cni_plugin_test.sh b/test/830_cni_plugin_test.sh index f042c294..f6eefaf2 100755 --- a/test/830_cni_plugin_test.sh +++ b/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 <&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