Merge pull request #583 from containernetworking/bugfix/wrong_startrange

host-local: remove redundant startRange in RangeIterator to avoid mismatching with startIP

plugins/ipam/host-local/backend/allocator/allocator.go

@@ -22,6 +22,7 @@ import (
 	"strconv"
 
 	current "github.com/containernetworking/cni/pkg/types/100"
+
 	"github.com/containernetworking/plugins/pkg/ip"
 	"github.com/containernetworking/plugins/plugins/ipam/host-local/backend"
 )
@@ -132,9 +133,8 @@ type RangeIter struct {
 	// Our current position
 	cur net.IP
 
-	// The IP and range index where we started iterating; if we hit this again, we're done.
+	// The IP where we started iterating; if we hit this again, we're done.
-	startIP    net.IP
+	startIP net.IP
-	startRange int
 }
 
 // GetIter encapsulates the strategy for this allocator.
@@ -164,7 +164,6 @@ func (a *IPAllocator) GetIter() (*RangeIter, error) {
 		for i, r := range *a.rangeset {
 			if r.Contains(lastReservedIP) {
 				iter.rangeIdx = i
-				iter.startRange = i
 
 				// We advance the cursor on every Next(), so the first call
 				// to next() will return lastReservedIP + 1
@@ -174,7 +173,6 @@ func (a *IPAllocator) GetIter() (*RangeIter, error) {
 		}
 	} else {
 		iter.rangeIdx = 0
-		iter.startRange = 0
 		iter.startIP = (*a.rangeset)[0].RangeStart
 	}
 	return &iter, nil
@@ -210,7 +208,7 @@ func (i *RangeIter) Next() (*net.IPNet, net.IP) {
 
 	if i.startIP == nil {
 		i.startIP = i.cur
-	} else if i.rangeIdx == i.startRange && i.cur.Equal(i.startIP) {
+	} else if i.cur.Equal(i.startIP) {
 		// IF we've looped back to where we started, give up
 		return nil, nil
 	}

plugins/ipam/host-local/backend/allocator/allocator_test.go

@@ -49,6 +49,23 @@ func mkalloc() IPAllocator {
 	return alloc
 }
 
+func newAllocatorWithMultiRanges() IPAllocator {
+	p := RangeSet{
+		Range{RangeStart: net.IP{192, 168, 1, 0}, RangeEnd: net.IP{192, 168, 1, 3}, Subnet: mustSubnet("192.168.1.1/30")},
+		Range{RangeStart: net.IP{192, 168, 2, 0}, RangeEnd: net.IP{192, 168, 2, 3}, Subnet: mustSubnet("192.168.2.1/30")},
+	}
+	_ = p.Canonicalize()
+	store := fakestore.NewFakeStore(map[string]string{}, map[string]net.IP{})
+
+	alloc := IPAllocator{
+		rangeset: &p,
+		store:    store,
+		rangeID:  "rangeid",
+	}
+
+	return alloc
+}
+
 func (t AllocatorTestCase) run(idx int) (*current.IPConfig, error) {
 	fmt.Fprintln(GinkgoWriter, "Index:", idx)
 	p := RangeSet{}
@@ -320,6 +337,39 @@ var _ = Describe("host-local ip allocator", func() {
 			}
 		})
 	})
+
+	Context("when lastReservedIP is at the end of one of multi ranges", func() {
+		It("should use the first IP of next range as startIP after Next", func() {
+			a := newAllocatorWithMultiRanges()
+
+			// reserve the last IP of the first range
+			reserved, err := a.store.Reserve("ID", "eth0", net.IP{192, 168, 1, 3}, a.rangeID)
+			Expect(reserved).To(BeTrue())
+			Expect(err).NotTo(HaveOccurred())
+
+			// get range iterator and do the first Next
+			r, err := a.GetIter()
+			Expect(err).NotTo(HaveOccurred())
+			ip := r.nextip()
+			Expect(ip).NotTo(BeNil())
+
+			Expect(r.startIP).To(Equal(net.IP{192, 168, 2, 0}))
+		})
+	})
+
+	Context("when no lastReservedIP", func() {
+		It("should use the first IP of the first range as startIP after Next", func() {
+			a := newAllocatorWithMultiRanges()
+
+			// get range iterator and do the first Next
+			r, err := a.GetIter()
+			Expect(err).NotTo(HaveOccurred())
+			ip := r.nextip()
+			Expect(ip).NotTo(BeNil())
+
+			Expect(r.startIP).To(Equal(net.IP{192, 168, 1, 0}))
+		})
+	})
 })
 
 // nextip is a convenience function used for testing