Merge pull request #3010 from weaveworks/issues/3003-anonymous-claim

Fix use of '_' as container ID in claim outside range

ipam/allocator_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	"github.com/weaveworks/weave/api"
 	"github.com/weaveworks/weave/common"
 	"github.com/weaveworks/weave/net/address"
 	"github.com/weaveworks/weave/testing/gossip"
@@ -30,6 +31,10 @@ func (alloc *Allocator) SimplyClaim(ident string, cidr address.CIDR) error {
 	return alloc.Claim(ident, cidr, true, true, returnFalse)
 }
 
+func (alloc *Allocator) AnonymousClaim(cidr address.CIDR) error {
+	return alloc.Claim(api.NoContainerID, cidr, false, true, returnFalse)
+}
+
 func TestAllocFree(t *testing.T) {
 	const (
 		container1 = "abcdef"
@@ -170,6 +175,8 @@ func TestAllocatorClaim(t *testing.T) {
 		universe   = "10.0.3.0/24"
 		testAddr1  = "10.0.3.5/24"
 		testAddr2  = "10.0.4.2/24"
+		testAddr4  = "10.0.2.3/23"
+		subnet4    = "10.0.2.0/23"
 		testPre    = "10.0.3.1/24"
 	)
 
@@ -203,6 +210,9 @@ func TestAllocatorClaim(t *testing.T) {
 	// claim for a different container should fail
 	err = alloc.SimplyClaim(container1, addr1)
 	require.Error(t, err)
+	// claim for an anonymous container, will assume it's the same one
+	err = alloc.AnonymousClaim(addr1)
+	require.NoError(t, err)
 	// claiming the address allocated on the other peer should fail
 	err = alloc.SimplyClaim(container1, address.MakeCIDR(subnet, addrx))
 	require.Error(t, err, "claiming address allocated on other peer should fail")
@@ -215,6 +225,15 @@ func TestAllocatorClaim(t *testing.T) {
 	addr2, _ := address.ParseCIDR(testAddr2)
 	err = alloc.SimplyClaim(container1, addr2)
 	require.NoError(t, err)
+	err = alloc.SimplyClaim(container3, addr2)
+	require.Error(t, err)
+	// Anonymous claim outside of our universe
+	addr4, _ := address.ParseCIDR(testAddr4)
+	subnet4cidr, _ := address.ParseCIDR(subnet4)
+	require.NoError(t, alloc.AnonymousClaim(addr4))
+	addr5, err := alloc.Allocate(api.NoContainerID, subnet4cidr, false, returnFalse)
+	require.NoError(t, err)
+	require.NotEqual(t, addr4.Start(), addr5)
 }
 
 func (alloc *Allocator) pause() func() {

ipam/claim.go

@@ -40,10 +40,29 @@ func (c *claim) Try(alloc *Allocator) bool {
 		return true
 	}
 
+	addOwned := func() {
+		if c.ident == api.NoContainerID {
+			alloc.addOwned(c.cidr.Addr.String(), c.cidr, c.isContainer)
+		} else {
+			alloc.addOwned(c.ident, c.cidr, c.isContainer)
+		}
+	}
+
 	if !alloc.ring.Contains(c.cidr.Addr) {
-		// Address not within our universe; assume user knows what they are doing
 		alloc.infof("Address %s claimed by %s - not in our range", c.cidr, c.ident)
-		alloc.addOwned(c.ident, c.cidr, c.isContainer)
+		previousOwner := alloc.findOwner(c.cidr.Addr)
+		switch {
+		case previousOwner == "":
+			addOwned()
+		case previousOwner == c.ident: // already owned by this ID
+		case c.ident == api.NoContainerID: // already owned by anonymous container
+			// do nothing (no automatic fall-through in Go)
+		case !alloc.dead[previousOwner].IsZero(): // already owned by dead container
+			alloc.removeOwned(previousOwner, c.cidr.Addr)
+			addOwned()
+		default:
+			c.sendResult(fmt.Errorf("address %s already in use by %s", c.cidr, previousOwner))
+		}
 		c.sendResult(nil)
 		return true
 	}
@@ -85,11 +104,7 @@ func (c *claim) Try(alloc *Allocator) bool {
 		// 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)
-			if c.ident == api.NoContainerID {
-				alloc.addOwned(c.cidr.Addr.String(), c.cidr, c.isContainer)
-			} else {
-				alloc.addOwned(c.ident, c.cidr, c.isContainer)
-			}
+			addOwned()
 			c.sendResult(nil)
 		} else {
 			c.sendResult(err)