Fix preferred ip allocation in ipam

- also provided a new utility to compute the
  host part ip address which is resilient to
  input passed in different representations.

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

libnetwork/ipam/allocator.go

@@ -21,7 +21,7 @@ const (
 	minNetSizeV6Eff = 96
 	// The size of the host subnet used internally, it's the most granular sequence addresses
 	defaultInternalHostSize = 16
-	// datastore keyes for ipam obkects
+	// datastore keyes for ipam objects
 	dsConfigKey = "ipam-config" // ipam-config/<domain>/<map of subent configs>
 	dsDataKey   = "ipam-data"   // ipam-data/<domain>/<subnet>/<child-sudbnet>/<bitmask>
 )
@@ -80,8 +80,7 @@ func NewAllocator(ds datastore.DataStore) (*Allocator, error) {
 				if err != nil {
 					return fmt.Errorf("failed to load address bitmask for configured subnet %s because of %s", v.Subnet.String(), err.Error())
 				}
-				a.insertAddressMasks(k, subnetList)
-				return nil
+				return a.insertAddressMasks(k, subnetList)
 			})
 	}
 	a.Unlock()
@@ -417,28 +416,48 @@ func (a *Allocator) request(addrSpace AddressSpace, req *AddressRequest, version
 
 // Release allows releasing the address from the specified address space
 func (a *Allocator) Release(addrSpace AddressSpace, address net.IP) {
+	var (
+		space *bitseq.Handle
+		sub   *net.IPNet
+	)
+
 	if address == nil {
+		log.Debugf("Requested to remove nil address from address space %s", addrSpace)
 		return
 	}
+
 	ver := getAddressVersion(address)
 	if ver == v4 {
 		address = address.To4()
 	}
+
+	// Find the subnet containing the address
 	for _, subKey := range a.getSubnetList(addrSpace, ver) {
-		a.Lock()
-		space := a.addresses[subKey]
-		a.Unlock()
-		sub := subKey.canonicalChildSubnet()
+		sub = subKey.canonicalChildSubnet()
 		if sub.Contains(address) {
-			// Retrieve correspondent ordinal in the subnet
-			ordinal := ipToUint32(getHostPortionIP(address, sub))
-			// Release it
-			if err := space.Unset(ordinal); err != nil {
-				log.Warnf("Failed to release address %s because of internal error: %s", address.String(), err.Error())
-			}
-			return
+			a.Lock()
+			space = a.addresses[subKey]
+			a.Unlock()
+			break
 		}
 	}
+	if space == nil {
+		log.Debugf("Could not find subnet on address space %s containing %s on release", addrSpace, address.String())
+		return
+	}
+
+	// Retrieve correspondent ordinal in the subnet
+	hostPart, err := types.GetHostPartIP(address, sub.Mask)
+	if err != nil {
+		log.Warnf("Failed to release address %s on address space %s because of internal error: %v", address.String(), addrSpace, err)
+		return
+	}
+	ordinal := ipToUint32(hostPart)
+
+	// Release it
+	if err := space.Unset(ordinal); err != nil {
+		log.Warnf("Failed to release address %s on address space %s because of internal error: %v", address.String(), addrSpace, err)
+	}
 }
 
 func (a *Allocator) reserveAddress(addrSpace AddressSpace, subnet *net.IPNet, prefAddress net.IP, ver ipVersion) (net.IP, *net.IPNet, error) {
@@ -468,7 +487,7 @@ func (a *Allocator) reserveAddress(addrSpace AddressSpace, subnet *net.IPNet, pr
 		bitmask, ok := a.addresses[key]
 		a.Unlock()
 		if !ok {
-			fmt.Printf("\nDid not find a bitmask for subnet key: %s", key.String())
+			log.Warnf("Did not find a bitmask for subnet key: %s", key.String())
 			continue
 		}
 		address, err := a.getAddress(key.canonicalChildSubnet(), bitmask, prefAddress, ver)
@@ -511,7 +530,12 @@ func (a *Allocator) getAddress(subnet *net.IPNet, bitmask *bitseq.Handle, prefAd
 		if prefAddress == nil {
 			ordinal, err = bitmask.SetAny()
 		} else {
-			err = bitmask.Set(ipToUint32(getHostPortionIP(prefAddress, subnet)))
+			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
@@ -549,7 +573,7 @@ func generateAddress(ordinal uint32, network *net.IPNet) net.IP {
 	var address [16]byte
 
 	// Get network portion of IP
-	if network.IP.To4() != nil {
+	if getAddressVersion(network.IP) == v4 {
 		copy(address[:], network.IP.To4())
 	} else {
 		copy(address[:], network.IP)
@@ -592,12 +616,3 @@ func ipToUint32(ip []byte) uint32 {
 	}
 	return value
 }
-
-// Given an address and subnet, returns the host portion address
-func getHostPortionIP(address net.IP, subnet *net.IPNet) net.IP {
-	hostPortion := make([]byte, len(address))
-	for i := 0; i < len(subnet.Mask); i++ {
-		hostPortion[i] = address[i] &^ subnet.Mask[i]
-	}
-	return hostPortion
-}

libnetwork/ipam/allocator_test.go

@@ -3,15 +3,31 @@ package ipam
 import (
 	"fmt"
 	"net"
+	"os"
 	"testing"
 	"time"
 
 	"github.com/docker/libnetwork/bitseq"
+	"github.com/docker/libnetwork/config"
+	"github.com/docker/libnetwork/datastore"
 	_ "github.com/docker/libnetwork/netutils"
 )
 
+var ds datastore.DataStore
+
+// enable w/ upper case
+func testMain(m *testing.M) {
+	var err error
+	ds, err = datastore.NewDataStore(&config.DatastoreCfg{Embedded: false, Client: config.DatastoreClientCfg{Provider: "consul", Address: "127.0.0.1:8500"}})
+	if err != nil {
+		fmt.Println(err)
+	}
+
+	os.Exit(m.Run())
+}
+
 func getAllocator(t *testing.T, subnet *net.IPNet) *Allocator {
-	a, err := NewAllocator(nil)
+	a, err := NewAllocator(ds)
 	if err != nil {
 		t.Fatal(err)
 	}
@@ -273,7 +289,32 @@ func TestGetInternalSubnets(t *testing.T) {
 	for _, d := range input {
 		assertInternalSubnet(t, d.internalHostSize, d.parentSubnet, d.firstIntSubnet, d.lastIntSubnet)
 	}
+}
+
+func TestGetSameAddress(t *testing.T) {
+	a, err := NewAllocator(nil)
+	if err != nil {
+		t.Fatal(err)
+	}
 
+	addSpace := AddressSpace("giallo")
+	_, subnet, _ := net.ParseCIDR("192.168.100.0/24")
+	if err := a.AddSubnet(addSpace, &SubnetInfo{Subnet: subnet}); err != nil {
+		t.Fatal(err)
+	}
+
+	ip := net.ParseIP("192.168.100.250")
+	req := &AddressRequest{Subnet: *subnet, Address: ip}
+
+	_, err = a.Request(addSpace, req)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	_, err = a.Request(addSpace, req)
+	if err == nil {
+		t.Fatal(err)
+	}
 }
 
 func TestGetAddress(t *testing.T) {

libnetwork/types/types.go

@@ -197,6 +197,59 @@ func CompareIPNet(a, b *net.IPNet) bool {
 	return a.IP.Equal(b.IP) && bytes.Equal(a.Mask, b.Mask)
 }
 
+// GetMinimalIP returns the address in its shortest form
+func GetMinimalIP(ip net.IP) net.IP {
+	if ip != nil && ip.To4() != nil {
+		return ip.To4()
+	}
+	return ip
+}
+
+// GetMinimalIPNet returns a copy of the passed IP Network with congruent ip and mask notation
+func GetMinimalIPNet(nw *net.IPNet) *net.IPNet {
+	if nw == nil {
+		return nil
+	}
+	if len(nw.IP) == 16 && nw.IP.To4() != nil {
+		m := nw.Mask
+		if len(m) == 16 {
+			m = m[12:16]
+		}
+		return &net.IPNet{IP: nw.IP.To4(), Mask: m}
+	}
+	return nw
+}
+
+var v4inV6MaskPrefix = []byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}
+
+// GetHostPartIP returns the host portion of the ip address identified by the mask.
+// IP address representation is not modified. If address and mask are not compatible
+// an error is returned.
+func GetHostPartIP(ip net.IP, mask net.IPMask) (net.IP, error) {
+	// Find the effective starting of address and mask
+	is := 0
+	ms := 0
+	if len(ip) == net.IPv6len && ip.To4() != nil {
+		is = 12
+	}
+	if len(ip[is:]) == net.IPv4len && len(mask) == net.IPv6len && bytes.Equal(mask[:12], v4inV6MaskPrefix) {
+		ms = 12
+	}
+
+	// Check if address and mask are semantically compatible
+	if len(ip[is:]) != len(mask[ms:]) {
+		return nil, fmt.Errorf("cannot compute host portion ip address as ip and mask are not compatible: (%#v, %#v)", ip, mask)
+	}
+
+	// Compute host portion
+	out := GetIPCopy(ip)
+	for i := 0; i < len(mask[ms:]); i++ {
+		out[is+i] &= ^mask[ms+i]
+	}
+
+	return out, nil
+}
+
 const (
 	// NEXTHOP indicates a StaticRoute with an IP next hop.
 	NEXTHOP = iota

libnetwork/types/types_test.go

@@ -2,6 +2,7 @@ package types
 
 import (
 	"flag"
+	"net"
 	"testing"
 )
 
@@ -109,3 +110,61 @@ func TestErrorConstructors(t *testing.T) {
 		t.Fatal(err)
 	}
 }
+
+func TestUtilGetHostPortionIP(t *testing.T) {
+	input := []struct {
+		ip   net.IP
+		mask net.IPMask
+		host net.IP
+		err  error
+	}{
+		{ // ip in v4Inv6 representation, mask in v4 representation
+			ip:   net.IPv4(172, 28, 30, 1),
+			mask: []byte{0xff, 0xff, 0xff, 0},
+			host: net.IPv4(0, 0, 0, 1),
+		},
+		{ // ip and mask in v4Inv6 representation
+			ip:   net.IPv4(172, 28, 30, 2),
+			mask: []byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0},
+			host: net.IPv4(0, 0, 0, 2),
+		},
+		{ // ip in v4 representation, mask in v4Inv6 representation
+			ip:   net.IPv4(172, 28, 30, 3)[12:],
+			mask: []byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0},
+			host: net.IPv4(0, 0, 0, 3)[12:],
+		},
+		{ // ip and mask in v4 representation
+			ip:   net.IPv4(172, 28, 30, 4)[12:],
+			mask: []byte{0xff, 0xff, 0xff, 0},
+			host: net.IPv4(0, 0, 0, 4)[12:],
+		},
+		{ // ip and mask as v6
+			ip:   net.ParseIP("2005:2004:2002:2001:FFFF:ABCD:EEAB:00CD"),
+			mask: []byte{0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0, 0, 0},
+			host: net.ParseIP("0::AB:00CD"),
+		},
+	}
+
+	for _, i := range input {
+		h, err := GetHostPartIP(i.ip, i.mask)
+		if err != nil {
+			t.Fatal(err)
+		}
+		if !i.host.Equal(h) {
+			t.Fatalf("Failed to return expected host ip. Expected: %s. Got: %s", i.host, h)
+		}
+	}
+
+	// ip as v6 and mask as v4 are not compatible
+	if _, err := GetHostPartIP(net.ParseIP("2005:2004:2002:2001:FFFF:ABCD:EEAB:00CD"), []byte{0xff, 0xff, 0xff, 0}); err == nil {
+		t.Fatalf("Unexpected success")
+	}
+	// ip as v4 and non conventional mask
+	if _, err := GetHostPartIP(net.ParseIP("173.32.4.5"), []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 0xff, 0}); err == nil {
+		t.Fatalf("Unexpected success")
+	}
+	// ip as v4 and non conventional mask
+	if _, err := GetHostPartIP(net.ParseIP("173.32.4.5"), []byte{0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0xff, 0xff, 0xff, 0xff, 0xff, 0}); err == nil {
+		t.Fatalf("Unexpected success")
+	}
+}