Merge pull request #294 from dcbw/host-local-ranges

Fix handling of host-local IP address ranges

plugins/ipam/host-local/allocator.go

@@ -25,13 +25,22 @@ import (
 )
 
 type IPAllocator struct {
+	// start is inclusive and may be allocated
 	start net.IP
+	// end is inclusive and may be allocated
 	end   net.IP
 	conf  *IPAMConfig
 	store backend.Store
 }
 
 func NewIPAllocator(conf *IPAMConfig, store backend.Store) (*IPAllocator, error) {
+	// Can't create an allocator for a network with no addresses, eg
+	// a /32 or /31
+	ones, masklen := conf.Subnet.Mask.Size()
+	if ones > masklen-2 {
+		return nil, fmt.Errorf("Network %v too small to allocate from", conf.Subnet)
+	}
+
 	var (
 		start net.IP
 		end   net.IP
@@ -46,25 +55,76 @@ func NewIPAllocator(conf *IPAMConfig, store backend.Store) (*IPAllocator, error)
 	start = ip.NextIP(start)
 
 	if conf.RangeStart != nil {
-		if err := validateRangeIP(conf.RangeStart, (*net.IPNet)(&conf.Subnet)); err != nil {
+		if err := validateRangeIP(conf.RangeStart, (*net.IPNet)(&conf.Subnet), nil, nil); err != nil {
 			return nil, err
 		}
 		start = conf.RangeStart
 	}
 	if conf.RangeEnd != nil {
-		if err := validateRangeIP(conf.RangeEnd, (*net.IPNet)(&conf.Subnet)); err != nil {
+		if err := validateRangeIP(conf.RangeEnd, (*net.IPNet)(&conf.Subnet), start, nil); err != nil {
 			return nil, err
 		}
-		// RangeEnd is inclusive
+		end = conf.RangeEnd
-		end = ip.NextIP(conf.RangeEnd)
 	}
 	return &IPAllocator{start, end, conf, store}, nil
 }
 
-func validateRangeIP(ip net.IP, ipnet *net.IPNet) error {
+func canonicalizeIP(ip net.IP) (net.IP, error) {
+	if ip.To4() != nil {
+		return ip.To4(), nil
+	} else if ip.To16() != nil {
+		return ip.To16(), nil
+	}
+	return nil, fmt.Errorf("IP %s not v4 nor v6", ip)
+}
+
+// Ensures @ip is within @ipnet, and (if given) inclusive of @start and @end
+func validateRangeIP(ip net.IP, ipnet *net.IPNet, start net.IP, end net.IP) error {
+	var err error
+
+	// Make sure we can compare IPv4 addresses directly
+	ip, err = canonicalizeIP(ip)
+	if err != nil {
+		return err
+	}
+
 	if !ipnet.Contains(ip) {
 		return fmt.Errorf("%s not in network: %s", ip, ipnet)
 	}
+
+	if start != nil {
+		start, err = canonicalizeIP(start)
+		if err != nil {
+			return err
+		}
+		if len(ip) != len(start) {
+			return fmt.Errorf("%s %d not same size IP address as start %s %d", ip, len(ip), start, len(start))
+		}
+		for i := 0; i < len(ip); i++ {
+			if ip[i] > start[i] {
+				break
+			} else if ip[i] < start[i] {
+				return fmt.Errorf("%s outside of network %s with start %s", ip, ipnet, start)
+			}
+		}
+	}
+
+	if end != nil {
+		end, err = canonicalizeIP(end)
+		if err != nil {
+			return err
+		}
+		if len(ip) != len(end) {
+			return fmt.Errorf("%s %d not same size IP address as end %s %d", ip, len(ip), end, len(end))
+		}
+		for i := 0; i < len(ip); i++ {
+			if ip[i] < end[i] {
+				break
+			} else if ip[i] > end[i] {
+				return fmt.Errorf("%s outside of network %s with end %s", ip, ipnet, end)
+			}
+		}
+	}
 	return nil
 }
 
@@ -92,7 +152,7 @@ func (a *IPAllocator) Get(id string) (*types.IPConfig, error) {
 			IP:   a.conf.Subnet.IP,
 			Mask: a.conf.Subnet.Mask,
 		}
-		err := validateRangeIP(requestedIP, &subnet)
+		err := validateRangeIP(requestedIP, &subnet, a.start, a.end)
 		if err != nil {
 			return nil, err
 		}
@@ -142,16 +202,15 @@ func (a *IPAllocator) Release(id string) error {
 	return a.store.ReleaseByID(id)
 }
 
+// Return the start and end IP addresses of a given subnet, excluding
+// the broadcast address (eg, 192.168.1.255)
 func networkRange(ipnet *net.IPNet) (net.IP, net.IP, error) {
 	if ipnet.IP == nil {
 		return nil, nil, fmt.Errorf("missing field %q in IPAM configuration", "subnet")
 	}
-	ip := ipnet.IP.To4()
+	ip, err := canonicalizeIP(ipnet.IP)
-	if ip == nil {
+	if err != nil {
-		ip = ipnet.IP.To16()
+		return nil, nil, fmt.Errorf("IP not v4 nor v6")
-		if ip == nil {
-			return nil, nil, fmt.Errorf("IP not v4 nor v6")
-		}
 	}
 
 	if len(ip) != len(ipnet.Mask) {
@@ -162,6 +221,12 @@ func networkRange(ipnet *net.IPNet) (net.IP, net.IP, error) {
 	for i := 0; i < len(ip); i++ {
 		end = append(end, ip[i]|^ipnet.Mask[i])
 	}
+
+	// Exclude the broadcast address for IPv4
+	if ip.To4() != nil {
+		end[3]--
+	}
+
 	return ipnet.IP, end, nil
 }
 
@@ -186,7 +251,7 @@ func (a *IPAllocator) getSearchRange() (net.IP, net.IP) {
 			IP:   a.conf.Subnet.IP,
 			Mask: a.conf.Subnet.Mask,
 		}
-		err := validateRangeIP(lastReservedIP, &subnet)
+		err := validateRangeIP(lastReservedIP, &subnet, a.start, a.end)
 		if err == nil {
 			startFromLastReservedIP = true
 		}

plugins/ipam/host-local/allocator_test.go

@@ -15,6 +15,7 @@
 package main
 
 import (
+	"fmt"
 	"github.com/containernetworking/cni/pkg/types"
 	fakestore "github.com/containernetworking/cni/plugins/ipam/host-local/backend/testing"
 	. "github.com/onsi/ginkgo"
@@ -76,26 +77,26 @@ var _ = Describe("host-local ip allocator", func() {
 				{
 					subnet:       "10.0.0.0/29",
 					ipmap:        map[string]string{},
-					expectResult: "10.0.0.7",
+					expectResult: "10.0.0.6",
-					lastIP:       "10.0.0.6",
+					lastIP:       "10.0.0.5",
 				},
 				{
 					subnet: "10.0.0.0/29",
 					ipmap: map[string]string{
+						"10.0.0.4": "id",
 						"10.0.0.5": "id",
-						"10.0.0.6": "id",
 					},
-					expectResult: "10.0.0.7",
+					expectResult: "10.0.0.6",
-					lastIP:       "10.0.0.4",
+					lastIP:       "10.0.0.3",
 				},
 				// round robin to the beginning
 				{
 					subnet: "10.0.0.0/29",
 					ipmap: map[string]string{
-						"10.0.0.7": "id",
+						"10.0.0.6": "id",
 					},
 					expectResult: "10.0.0.2",
-					lastIP:       "10.0.0.6",
+					lastIP:       "10.0.0.5",
 				},
 				// lastIP is out of range
 				{
@@ -115,6 +116,79 @@ var _ = Describe("host-local ip allocator", func() {
 			}
 		})
 
+		It("should not allocate the broadcast address", func() {
+			subnet, err := types.ParseCIDR("192.168.1.0/24")
+			Expect(err).ToNot(HaveOccurred())
+
+			conf := IPAMConfig{
+				Name:   "test",
+				Type:   "host-local",
+				Subnet: types.IPNet{IP: subnet.IP, Mask: subnet.Mask},
+			}
+			store := fakestore.NewFakeStore(map[string]string{}, net.ParseIP(""))
+			alloc, err := NewIPAllocator(&conf, store)
+			Expect(err).ToNot(HaveOccurred())
+
+			for i := 1; i < 254; i++ {
+				res, err := alloc.Get("ID")
+				Expect(err).ToNot(HaveOccurred())
+				// i+1 because the gateway address is skipped
+				s := fmt.Sprintf("192.168.1.%d/24", i+1)
+				Expect(s).To(Equal(res.IP.String()))
+			}
+
+			_, err = alloc.Get("ID")
+			Expect(err).To(HaveOccurred())
+		})
+
+		It("should allocate RangeStart first", func() {
+			subnet, err := types.ParseCIDR("192.168.1.0/24")
+			Expect(err).ToNot(HaveOccurred())
+
+			conf := IPAMConfig{
+				Name:       "test",
+				Type:       "host-local",
+				Subnet:     types.IPNet{IP: subnet.IP, Mask: subnet.Mask},
+				RangeStart: net.ParseIP("192.168.1.10"),
+			}
+			store := fakestore.NewFakeStore(map[string]string{}, net.ParseIP(""))
+			alloc, err := NewIPAllocator(&conf, store)
+			Expect(err).ToNot(HaveOccurred())
+
+			res, err := alloc.Get("ID")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(res.IP.String()).To(Equal("192.168.1.10/24"))
+
+			res, err = alloc.Get("ID")
+			Expect(err).ToNot(HaveOccurred())
+			Expect(res.IP.String()).To(Equal("192.168.1.11/24"))
+		})
+
+		It("should allocate RangeEnd but not past RangeEnd", func() {
+			subnet, err := types.ParseCIDR("192.168.1.0/24")
+			Expect(err).ToNot(HaveOccurred())
+
+			conf := IPAMConfig{
+				Name:     "test",
+				Type:     "host-local",
+				Subnet:   types.IPNet{IP: subnet.IP, Mask: subnet.Mask},
+				RangeEnd: net.ParseIP("192.168.1.5"),
+			}
+			store := fakestore.NewFakeStore(map[string]string{}, net.ParseIP(""))
+			alloc, err := NewIPAllocator(&conf, store)
+			Expect(err).ToNot(HaveOccurred())
+
+			for i := 1; i < 5; i++ {
+				res, err := alloc.Get("ID")
+				Expect(err).ToNot(HaveOccurred())
+				// i+1 because the gateway address is skipped
+				Expect(res.IP.String()).To(Equal(fmt.Sprintf("192.168.1.%d/24", i+1)))
+			}
+
+			_, err = alloc.Get("ID")
+			Expect(err).To(HaveOccurred())
+		})
+
 		Context("when requesting a specific IP", func() {
 			It("must allocate the requested IP", func() {
 				subnet, err := types.ParseCIDR("10.0.0.0/29")
@@ -133,6 +207,86 @@ var _ = Describe("host-local ip allocator", func() {
 				Expect(err).ToNot(HaveOccurred())
 				Expect(res.IP.IP.String()).To(Equal(requestedIP.String()))
 			})
+
+			It("must return an error when the requested IP is after RangeEnd", func() {
+				subnet, err := types.ParseCIDR("192.168.1.0/24")
+				Expect(err).ToNot(HaveOccurred())
+				ipmap := map[string]string{}
+				conf := IPAMConfig{
+					Name:     "test",
+					Type:     "host-local",
+					Subnet:   types.IPNet{IP: subnet.IP, Mask: subnet.Mask},
+					Args:     &IPAMArgs{IP: net.ParseIP("192.168.1.50")},
+					RangeEnd: net.ParseIP("192.168.1.20"),
+				}
+				store := fakestore.NewFakeStore(ipmap, nil)
+				alloc, _ := NewIPAllocator(&conf, store)
+				_, err = alloc.Get("ID")
+				Expect(err).To(HaveOccurred())
+			})
+
+			It("must return an error when the requested IP is before RangeStart", func() {
+				subnet, err := types.ParseCIDR("192.168.1.0/24")
+				Expect(err).ToNot(HaveOccurred())
+				ipmap := map[string]string{}
+				conf := IPAMConfig{
+					Name:       "test",
+					Type:       "host-local",
+					Subnet:     types.IPNet{IP: subnet.IP, Mask: subnet.Mask},
+					Args:       &IPAMArgs{IP: net.ParseIP("192.168.1.3")},
+					RangeStart: net.ParseIP("192.168.1.10"),
+				}
+				store := fakestore.NewFakeStore(ipmap, nil)
+				alloc, _ := NewIPAllocator(&conf, store)
+				_, err = alloc.Get("ID")
+				Expect(err).To(HaveOccurred())
+			})
+		})
+
+		It("RangeStart must be in the given subnet", func() {
+			subnet, err := types.ParseCIDR("192.168.1.0/24")
+			Expect(err).ToNot(HaveOccurred())
+
+			conf := IPAMConfig{
+				Name:       "test",
+				Type:       "host-local",
+				Subnet:     types.IPNet{IP: subnet.IP, Mask: subnet.Mask},
+				RangeStart: net.ParseIP("10.0.0.1"),
+			}
+			store := fakestore.NewFakeStore(map[string]string{}, net.ParseIP(""))
+			_, err = NewIPAllocator(&conf, store)
+			Expect(err).To(HaveOccurred())
+		})
+
+		It("RangeEnd must be in the given subnet", func() {
+			subnet, err := types.ParseCIDR("192.168.1.0/24")
+			Expect(err).ToNot(HaveOccurred())
+
+			conf := IPAMConfig{
+				Name:     "test",
+				Type:     "host-local",
+				Subnet:   types.IPNet{IP: subnet.IP, Mask: subnet.Mask},
+				RangeEnd: net.ParseIP("10.0.0.1"),
+			}
+			store := fakestore.NewFakeStore(map[string]string{}, net.ParseIP(""))
+			_, err = NewIPAllocator(&conf, store)
+			Expect(err).To(HaveOccurred())
+		})
+
+		It("RangeEnd must be after RangeStart in the given subnet", func() {
+			subnet, err := types.ParseCIDR("192.168.1.0/24")
+			Expect(err).ToNot(HaveOccurred())
+
+			conf := IPAMConfig{
+				Name:       "test",
+				Type:       "host-local",
+				Subnet:     types.IPNet{IP: subnet.IP, Mask: subnet.Mask},
+				RangeStart: net.ParseIP("192.168.1.10"),
+				RangeEnd:   net.ParseIP("192.168.1.3"),
+			}
+			store := fakestore.NewFakeStore(map[string]string{}, net.ParseIP(""))
+			_, err = NewIPAllocator(&conf, store)
+			Expect(err).To(HaveOccurred())
 		})
 	})
 
@@ -164,4 +318,20 @@ var _ = Describe("host-local ip allocator", func() {
 			}
 		})
 	})
+
+	Context("when given an invalid subnet", func() {
+		It("returns a meaningful error", func() {
+			subnet, err := types.ParseCIDR("192.168.1.0/31")
+			Expect(err).ToNot(HaveOccurred())
+
+			conf := IPAMConfig{
+				Name:   "test",
+				Type:   "host-local",
+				Subnet: types.IPNet{IP: subnet.IP, Mask: subnet.Mask},
+			}
+			store := fakestore.NewFakeStore(map[string]string{}, net.ParseIP(""))
+			_, err = NewIPAllocator(&conf, store)
+			Expect(err).To(HaveOccurred())
+		})
+	})
 })