Merge pull request #2148 from abhi/ipam-check

Adding a check for subnet pool overlap

libnetwork/ipam/allocator_test.go

@@ -241,12 +241,9 @@ func TestAddSubnets(t *testing.T) {
 			t.Fatal("returned same pool id for same subnets in different namespaces")
 		}
 
-		pid, _, _, err := a.RequestPool("abc", "10.0.0.0/8", "", nil, false)
-		if err != nil {
-			t.Fatalf("Unexpected failure requesting existing subnet: %v", err)
-		}
-		if pid != pid1 {
-			t.Fatal("returned different pool id for same subnet requests")
+		_, _, _, err = a.RequestPool("abc", "10.0.0.0/8", "", nil, false)
+		if err == nil {
+			t.Fatalf("Expected failure requesting existing subnet")
 		}
 
 		_, _, _, err = a.RequestPool("abc", "10.128.0.0/9", "", nil, false)
@@ -254,16 +251,13 @@ func TestAddSubnets(t *testing.T) {
 			t.Fatal("Expected failure on adding overlapping base subnet")
 		}
 
-		pid2, _, _, err := a.RequestPool("abc", "10.0.0.0/8", "10.128.0.0/9", nil, false)
+		_, _, _, err = a.RequestPool("abc", "10.0.0.0/8", "10.128.0.0/9", nil, false)
 		if err != nil {
 			t.Fatalf("Unexpected failure on adding sub pool: %v", err)
 		}
-		pid3, _, _, err := a.RequestPool("abc", "10.0.0.0/8", "10.128.0.0/9", nil, false)
-		if err != nil {
-			t.Fatalf("Unexpected failure on adding overlapping sub pool: %v", err)
-		}
-		if pid2 != pid3 {
-			t.Fatal("returned different pool id for same sub pool requests")
+		_, _, _, err = a.RequestPool("abc", "10.0.0.0/8", "10.128.0.0/9", nil, false)
+		if err == nil {
+			t.Fatalf("Expected failure on adding overlapping sub pool")
 		}
 
 		_, _, _, err = a.RequestPool(localAddressSpace, "10.20.2.0/24", "", nil, false)
@@ -329,7 +323,7 @@ func TestAddReleasePoolID(t *testing.T) {
 		a, err := getAllocator(store)
 		assert.NoError(t, err)
 
-		var k0, k1, k2 SubnetKey
+		var k0, k1 SubnetKey
 		aSpace, err := a.getAddrSpace(localAddressSpace)
 		if err != nil {
 			t.Fatal(err)
@@ -362,6 +356,10 @@ func TestAddReleasePoolID(t *testing.T) {
 			t.Fatal(err)
 		}
 
+		if pid0 == pid1 {
+			t.Fatalf("Incorrect poolIDs returned %s, %s", pid0, pid1)
+		}
+
 		aSpace, err = a.getAddrSpace(localAddressSpace)
 		if err != nil {
 			t.Fatal(err)
@@ -372,15 +370,9 @@ func TestAddReleasePoolID(t *testing.T) {
 			t.Fatalf("Unexpected ref count for %s: %d", k1, subnets[k1].RefCount)
 		}
 
-		pid2, _, _, err := a.RequestPool(localAddressSpace, "10.0.0.0/8", "10.0.0.0/16", nil, false)
-		if err != nil {
-			t.Fatal("Unexpected failure in adding sub pool")
-		}
-		if pid0 == pid1 || pid0 == pid2 || pid1 != pid2 {
-			t.Fatalf("Incorrect poolIDs returned %s, %s, %s", pid0, pid1, pid2)
-		}
-		if err := k2.FromString(pid2); err != nil {
-			t.Fatal(err)
+		_, _, _, err = a.RequestPool(localAddressSpace, "10.0.0.0/8", "10.0.0.0/16", nil, false)
+		if err == nil {
+			t.Fatal("Expected failure in adding sub pool")
 		}
 
 		aSpace, err = a.getAddrSpace(localAddressSpace)
@@ -389,11 +381,8 @@ func TestAddReleasePoolID(t *testing.T) {
 		}
 
 		subnets = aSpace.subnets
-		if subnets[k2].RefCount != 2 {
-			t.Fatalf("Unexpected ref count for %s: %d", k2, subnets[k2].RefCount)
-		}
 
-		if subnets[k0].RefCount != 3 {
+		if subnets[k0].RefCount != 2 {
 			t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount)
 		}
 
@@ -407,23 +396,13 @@ func TestAddReleasePoolID(t *testing.T) {
 		}
 
 		subnets = aSpace.subnets
-		if subnets[k0].RefCount != 2 {
+		if subnets[k0].RefCount != 1 {
 			t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount)
 		}
 		if err := a.ReleasePool(pid0); err != nil {
 			t.Fatal(err)
 		}
 
-		aSpace, err = a.getAddrSpace(localAddressSpace)
-		if err != nil {
-			t.Fatal(err)
-		}
-
-		subnets = aSpace.subnets
-		if subnets[k0].RefCount != 1 {
-			t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount)
-		}
-
 		pid00, _, _, err := a.RequestPool(localAddressSpace, "10.0.0.0/8", "", nil, false)
 		if err != nil {
 			t.Fatal("Unexpected failure in adding pool")
@@ -437,20 +416,6 @@ func TestAddReleasePoolID(t *testing.T) {
 			t.Fatal(err)
 		}
 
-		subnets = aSpace.subnets
-		if subnets[k0].RefCount != 2 {
-			t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount)
-		}
-
-		if err := a.ReleasePool(pid2); err != nil {
-			t.Fatal(err)
-		}
-
-		aSpace, err = a.getAddrSpace(localAddressSpace)
-		if err != nil {
-			t.Fatal(err)
-		}
-
 		subnets = aSpace.subnets
 		if subnets[k0].RefCount != 1 {
 			t.Fatalf("Unexpected ref count for %s: %d", k0, subnets[k0].RefCount)

libnetwork/ipam/structures.go

@@ -262,12 +262,13 @@ func (aSpace *addrSpace) updatePoolDBOnAdd(k SubnetKey, nw *net.IPNet, ipr *Addr
 	defer aSpace.Unlock()
 
 	// Check if already allocated
-	if p, ok := aSpace.subnets[k]; ok {
+	if _, ok := aSpace.subnets[k]; ok {
 		if pdf {
 			return nil, types.InternalMaskableErrorf("predefined pool %s is already reserved", nw)
 		}
-		aSpace.incRefCount(p, 1)
-		return func() error { return nil }, nil
+		// This means the same pool is already allocated. updatePoolDBOnAdd is called when there
+		// is request for a pool/subpool. It should ensure there is no overlap with existing pools
+		return nil, ipamapi.ErrPoolOverlap
 	}
 
 	// If master pool, check for overlap

libnetwork/libnetwork_internal_test.go

@@ -601,7 +601,7 @@ func TestIpamReleaseOnNetDriverFailures(t *testing.T) {
 	}
 
 	// Now create good bridge network with different gateway
-	ipamOpt2 := NetworkOptionIpam(ipamapi.DefaultIPAM, "", []*IpamConf{{PreferredPool: "10.34.0.0/16", Gateway: "10.34.255.253"}}, nil, nil)
+	ipamOpt2 := NetworkOptionIpam(ipamapi.DefaultIPAM, "", []*IpamConf{{PreferredPool: "10.35.0.0/16", Gateway: "10.35.255.253"}}, nil, nil)
 	gnw, err = c.NewNetwork("bridge", "goodnet2", "", ipamOpt2)
 	if err != nil {
 		t.Fatal(err)
@@ -614,7 +614,7 @@ func TestIpamReleaseOnNetDriverFailures(t *testing.T) {
 	}
 	defer ep.Delete(false)
 
-	expectedIP, _ := types.ParseCIDR("10.34.0.1/16")
+	expectedIP, _ := types.ParseCIDR("10.35.0.1/16")
 	if !types.CompareIPNet(ep.Info().Iface().Address(), expectedIP) {
 		t.Fatalf("Ipam release must have failed, endpoint has unexpected address: %v", ep.Info().Iface().Address())
 	}