Fix ip range allocation in ipam

- Current logic about valid addresses is wrong

Signed-off-by: Alessandro Boch <aboch@docker.com>

libnetwork/ipam/allocator.go

@@ -211,19 +211,34 @@ retry:
 
 // Create and insert the internal subnet(s) addresses masks into the address database. Mask data may come from the bitseq datastore.
 func (a *Allocator) insertAddressMasks(parentKey subnetKey, internalSubnetList []*net.IPNet) error {
-	for _, intSub := range internalSubnetList {
-		var err error
-		ones, bits := intSub.Mask.Size()
-		numAddresses := 1 << uint(bits-ones)
-		smallKey := subnetKey{parentKey.addressSpace, parentKey.subnet, intSub.String()}
+	ipVer := getAddressVersion(internalSubnetList[0].IP)
+	num := len(internalSubnetList)
+	ones, bits := internalSubnetList[0].Mask.Size()
+	numAddresses := 1 << uint(bits-ones)
+
+	for i := 0; i < num; i++ {
+		smallKey := subnetKey{parentKey.addressSpace, parentKey.subnet, internalSubnetList[i].String()}
+		limit := uint32(numAddresses)
+
+		if ipVer == v4 && i == num-1 {
+			// Do not let broadcast address be reserved
+			limit--
+		}
 
-		// Insert the new address masks. AddressMask content may come from datastore
-		a.Lock()
-		a.addresses[smallKey], err = bitseq.NewHandle(dsDataKey, a.store, smallKey.String(), uint32(numAddresses))
-		a.Unlock()
+		// Generate the new address masks. AddressMask content may come from datastore
+		h, err := bitseq.NewHandle(dsDataKey, a.getStore(), smallKey.String(), limit)
 		if err != nil {
 			return err
 		}
+
+		if ipVer == v4 && i == 0 {
+			// Do not let network identifier address be reserved
+			h.Set(0)
+		}
+
+		a.Lock()
+		a.addresses[smallKey] = h
+		a.Unlock()
 	}
 	return nil
 }
@@ -522,30 +537,21 @@ func (a *Allocator) getAddress(subnet *net.IPNet, bitmask *bitseq.Handle, prefAd
 		err     error
 	)
 
-	// Look for free IP, skip .0 and .255, they will be automatically reserved
-	for {
-		if bitmask.Unselected() <= 0 {
-			return nil, ErrNoAvailableIPs
-		}
-		if prefAddress == nil {
-			ordinal, err = bitmask.SetAny()
-		} else {
-			hostPart, e := types.GetHostPartIP(prefAddress, subnet.Mask)
-			if e != nil {
-				return nil, fmt.Errorf("failed to allocate preferred address %s: %v", prefAddress.String(), e)
-			}
-			ordinal = ipToUint32(types.GetMinimalIP(hostPart))
-			err = bitmask.Set(ordinal)
-		}
-		if err != nil {
-			return nil, ErrNoAvailableIPs
-		}
-
-		// For v4, let reservation of .0 and .255 happen automatically
-		if ver == v4 && !isValidIP(ordinal) {
-			continue
+	if bitmask.Unselected() <= 0 {
+		return nil, ErrNoAvailableIPs
+	}
+	if prefAddress == nil {
+		ordinal, err = bitmask.SetAny()
+	} else {
+		hostPart, e := types.GetHostPartIP(prefAddress, subnet.Mask)
+		if e != nil {
+			return nil, fmt.Errorf("failed to allocate preferred address %s: %v", prefAddress.String(), e)
 		}
-		break
+		ordinal = ipToUint32(types.GetMinimalIP(hostPart))
+		err = bitmask.Set(ordinal)
+	}
+	if err != nil {
+		return nil, ErrNoAvailableIPs
 	}
 
 	// Convert IP ordinal for this subnet into IP address
@@ -567,6 +573,12 @@ func (a *Allocator) DumpDatabase() {
 	}
 }
 
+func (a *Allocator) getStore() datastore.DataStore {
+	a.Lock()
+	defer a.Unlock()
+	return a.store
+}
+
 // It generates the ip address in the passed subnet specified by
 // the passed host address ordinal
 func generateAddress(ordinal uint32, network *net.IPNet) net.IP {
@@ -592,12 +604,6 @@ func getAddressVersion(ip net.IP) ipVersion {
 	return v4
 }
 
-// .0 and .255 will return false
-func isValidIP(i uint32) bool {
-	lastByte := i & 0xff
-	return lastByte != 0xff && lastByte != 0
-}
-
 // Adds the ordinal IP to the current array
 // 192.168.0.0 + 53 => 192.168.53
 func addIntToIP(array []byte, ordinal uint32) {

libnetwork/ipam/allocator_test.go

@@ -46,22 +46,6 @@ func TestInt2IP2IntConversion(t *testing.T) {
 	}
 }
 
-func TestIsValid(t *testing.T) {
-	list := []uint32{0, 255, 256, 511, 512, 767, 768}
-	for _, i := range list {
-		if isValidIP(i) {
-			t.Fatalf("Failed to detect invalid IPv4 ordinal: %d", i)
-		}
-	}
-
-	list = []uint32{1, 254, 257, 258, 510, 513, 769, 770}
-	for _, i := range list {
-		if !isValidIP(i) {
-			t.Fatalf("Marked valid ipv4 as invalid: %d", i)
-		}
-	}
-}
-
 func TestGetAddressVersion(t *testing.T) {
 	if v4 != getAddressVersion(net.ParseIP("172.28.30.112")) {
 		t.Fatalf("Failed to detect IPv4 version")
@@ -425,20 +409,19 @@ func TestRequest(t *testing.T) {
 		lastIP string
 	}{
 		{"192.168.59.0/24", 254, "192.168.59.254"},
-		{"192.168.240.0/20", 254, "192.168.240.254"},
-		{"192.168.0.0/16", 254, "192.168.0.254"},
-		{"10.16.0.0/16", 254, "10.16.0.254"},
-		{"10.128.0.0/12", 254, "10.128.0.254"},
-		{"10.0.0.0/8", 254, "10.0.0.254"},
-		{"192.168.0.0/16", 256, "192.168.1.2"},
-		{"10.0.0.0/8", 256, "10.0.1.2"},
-
-		{"192.168.128.0/18", 4 * 254, "192.168.131.254"},
-		{"192.168.240.0/20", 16 * 254, "192.168.255.254"},
-
-		{"192.168.0.0/16", 256 * 254, "192.168.255.254"},
-		{"10.0.0.0/8", 2 * 254, "10.0.1.254"},
-		{"10.0.0.0/8", 5 * 254, "10.0.4.254"},
+		{"192.168.240.0/20", 255, "192.168.240.255"},
+		{"192.168.0.0/16", 255, "192.168.0.255"},
+		{"192.168.0.0/16", 256, "192.168.1.0"},
+		{"10.16.0.0/16", 255, "10.16.0.255"},
+		{"10.128.0.0/12", 255, "10.128.0.255"},
+		{"10.0.0.0/8", 256, "10.0.1.0"},
+
+		{"192.168.128.0/18", 4*256 - 1, "192.168.131.255"},
+		{"192.168.240.0/20", 16*256 - 2, "192.168.255.254"},
+
+		{"192.168.0.0/16", 256*256 - 2, "192.168.255.254"},
+		{"10.0.0.0/8", 2 * 256, "10.0.2.0"},
+		{"10.0.0.0/8", 5 * 256, "10.0.5.0"},
 		//{"10.0.0.0/8", 100 * 256 * 254, "10.99.255.254"},
 	}