From dfc4f7cd2af04016fc7deedc2cc23925533bbf0a Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Fri, 2 Sep 2016 15:55:53 -0500 Subject: [PATCH 1/3] host-local: don't allocate past RangeEnd When RangeEnd is given, a.end = RangeEnd+1. If when getSearchRange() is called and lastReservedIP equals RangeEnd, a.nextIP() only compares lastReservedIP (which in this example is RangeEnd) against a.end (which in this example is RangeEnd+1) and they clearly don't match, so a.nextIP() returns start=RangeEnd+1 and end=RangeEnd. Get() happily allocates RangeEnd+1 because it only compares 'cur' to the end returned by getSearchRange(), not to a.end, and thus allocates past RangeEnd. Since a.end is inclusive (eg, host-local will allocate a.end) the fix is to simply set a.end equal to RangeEnd. --- plugins/ipam/host-local/allocator.go | 4 +- plugins/ipam/host-local/allocator_test.go | 74 +++++++++++++++++++++++ 2 files changed, 77 insertions(+), 1 deletion(-) diff --git a/plugins/ipam/host-local/allocator.go b/plugins/ipam/host-local/allocator.go index 3610646b..e0915ed8 100644 --- a/plugins/ipam/host-local/allocator.go +++ b/plugins/ipam/host-local/allocator.go @@ -25,7 +25,9 @@ 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 @@ -56,7 +58,7 @@ func NewIPAllocator(conf *IPAMConfig, store backend.Store) (*IPAllocator, error) return nil, err } // RangeEnd is inclusive - end = ip.NextIP(conf.RangeEnd) + end = conf.RangeEnd } return &IPAllocator{start, end, conf, store}, nil } diff --git a/plugins/ipam/host-local/allocator_test.go b/plugins/ipam/host-local/allocator_test.go index 200618a9..bb2b4f6f 100644 --- a/plugins/ipam/host-local/allocator_test.go +++ b/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" @@ -115,6 +116,79 @@ var _ = Describe("host-local ip allocator", func() { } }) + It("should allocate the last address if RangeEnd not given", 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 < 255; 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") From 959af1e6abe6ea98e6ca591abc2bcfef26dd476b Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Fri, 2 Sep 2016 16:52:29 -0500 Subject: [PATCH 2/3] host-local: ensure requested IPs are within the given range And also make sure that RangeStart and RangeEnd are sane. --- plugins/ipam/host-local/allocator.go | 72 ++++++++++++++++---- plugins/ipam/host-local/allocator_test.go | 80 +++++++++++++++++++++++ 2 files changed, 140 insertions(+), 12 deletions(-) diff --git a/plugins/ipam/host-local/allocator.go b/plugins/ipam/host-local/allocator.go index e0915ed8..ad5f0d20 100644 --- a/plugins/ipam/host-local/allocator.go +++ b/plugins/ipam/host-local/allocator.go @@ -48,25 +48,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 } 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 } @@ -94,7 +145,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 } @@ -148,12 +199,9 @@ 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() - if ip == nil { - ip = ipnet.IP.To16() - if ip == nil { - return nil, nil, fmt.Errorf("IP not v4 nor v6") - } + ip, err := canonicalizeIP(ipnet.IP) + if err != nil { + return nil, nil, fmt.Errorf("IP not v4 nor v6") } if len(ip) != len(ipnet.Mask) { @@ -188,7 +236,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 } diff --git a/plugins/ipam/host-local/allocator_test.go b/plugins/ipam/host-local/allocator_test.go index bb2b4f6f..9b7d19bf 100644 --- a/plugins/ipam/host-local/allocator_test.go +++ b/plugins/ipam/host-local/allocator_test.go @@ -207,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()) }) }) From 95a9ea0bd28f0b9d0098e76d525649bd13d6e7e3 Mon Sep 17 00:00:00 2001 From: Dan Williams Date: Fri, 2 Sep 2016 17:20:09 -0500 Subject: [PATCH 3/3] host-local: don't allocate the broadcast address or allow invalid networks There aren't any IPs to allocate in /32 or /31 networks, so don't allow them. --- plugins/ipam/host-local/allocator.go | 15 ++++++++++ plugins/ipam/host-local/allocator_test.go | 34 +++++++++++++++++------ 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/plugins/ipam/host-local/allocator.go b/plugins/ipam/host-local/allocator.go index ad5f0d20..a250ac01 100644 --- a/plugins/ipam/host-local/allocator.go +++ b/plugins/ipam/host-local/allocator.go @@ -34,6 +34,13 @@ type IPAllocator struct { } 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 @@ -195,6 +202,8 @@ 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") @@ -212,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 } diff --git a/plugins/ipam/host-local/allocator_test.go b/plugins/ipam/host-local/allocator_test.go index 9b7d19bf..2bcd2776 100644 --- a/plugins/ipam/host-local/allocator_test.go +++ b/plugins/ipam/host-local/allocator_test.go @@ -77,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", - lastIP: "10.0.0.6", + expectResult: "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", - lastIP: "10.0.0.4", + expectResult: "10.0.0.6", + 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 { @@ -116,7 +116,7 @@ var _ = Describe("host-local ip allocator", func() { } }) - It("should allocate the last address if RangeEnd not given", func() { + It("should not allocate the broadcast address", func() { subnet, err := types.ParseCIDR("192.168.1.0/24") Expect(err).ToNot(HaveOccurred()) @@ -129,7 +129,7 @@ var _ = Describe("host-local ip allocator", func() { alloc, err := NewIPAllocator(&conf, store) Expect(err).ToNot(HaveOccurred()) - for i := 1; i < 255; i++ { + for i := 1; i < 254; i++ { res, err := alloc.Get("ID") Expect(err).ToNot(HaveOccurred()) // i+1 because the gateway address is skipped @@ -318,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()) + }) + }) })