From 7f8ea631e520a720e2acddbcf94e63dfe6fb219c Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Sat, 1 Jun 2019 17:37:24 +0800 Subject: [PATCH 1/4] host-local: make Store interface support to get ip list by id Signed-off-by: Bruce Ma --- .../ipam/host-local/backend/disk/backend.go | 31 ++++++++++++++++++- plugins/ipam/host-local/backend/store.go | 1 + .../host-local/backend/testing/fake_store.go | 10 ++++++ 3 files changed, 41 insertions(+), 1 deletion(-) diff --git a/plugins/ipam/host-local/backend/disk/backend.go b/plugins/ipam/host-local/backend/disk/backend.go index c3e6b496f..56f1638a5 100644 --- a/plugins/ipam/host-local/backend/disk/backend.go +++ b/plugins/ipam/host-local/backend/disk/backend.go @@ -19,10 +19,10 @@ import ( "net" "os" "path/filepath" + "runtime" "strings" "github.com/containernetworking/plugins/plugins/ipam/host-local/backend" - "runtime" ) const lastIPFilePrefix = "last_reserved_ip." @@ -172,6 +172,35 @@ func (s *Store) ReleaseByID(id string, ifname string) error { return err } +// GetByID returns the IPs which have been allocated to the specific ID +func (s *Store) GetByID(id string, ifname string) []net.IP { + var ips []net.IP + + match := strings.TrimSpace(id) + LineBreak + ifname + // matchOld for backwards compatibility + matchOld := strings.TrimSpace(id) + + // walk through all ips in this network to get the ones which belong to a specific ID + _ = filepath.Walk(s.dataDir, func(path string, info os.FileInfo, err error) error { + if err != nil || info.IsDir() { + return nil + } + data, err := ioutil.ReadFile(path) + if err != nil { + return nil + } + if strings.TrimSpace(string(data)) == match || strings.TrimSpace(string(data)) == matchOld { + _, ipString := filepath.Split(path) + if ip := net.ParseIP(ipString); ip != nil { + ips = append(ips, ip) + } + } + return nil + }) + + return ips +} + func GetEscapedPath(dataDir string, fname string) string { if runtime.GOOS == "windows" { fname = strings.Replace(fname, ":", "_", -1) diff --git a/plugins/ipam/host-local/backend/store.go b/plugins/ipam/host-local/backend/store.go index 4ea845da7..7211ddf6a 100644 --- a/plugins/ipam/host-local/backend/store.go +++ b/plugins/ipam/host-local/backend/store.go @@ -24,4 +24,5 @@ type Store interface { LastReservedIP(rangeID string) (net.IP, error) Release(ip net.IP) error ReleaseByID(id string, ifname string) error + GetByID(id string, ifname string) []net.IP } diff --git a/plugins/ipam/host-local/backend/testing/fake_store.go b/plugins/ipam/host-local/backend/testing/fake_store.go index 631fca2e0..4f3a934ea 100644 --- a/plugins/ipam/host-local/backend/testing/fake_store.go +++ b/plugins/ipam/host-local/backend/testing/fake_store.go @@ -81,6 +81,16 @@ func (s *FakeStore) ReleaseByID(id string, ifname string) error { return nil } +func (s *FakeStore) GetByID(id string, ifname string) []net.IP { + var ips []net.IP + for k, v := range s.ipMap { + if v == id { + ips = append(ips, net.ParseIP(k)) + } + } + return ips +} + func (s *FakeStore) SetIPMap(m map[string]string) { s.ipMap = m } From e8771b36a2934a8be89612f09f4adcc2a36ec93e Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Sat, 1 Jun 2019 17:53:46 +0800 Subject: [PATCH 2/4] host-local: make allocation idempotent to multiple requests with same id Signed-off-by: Bruce Ma --- .../host-local/backend/allocator/allocator.go | 39 +++++++++++++------ 1 file changed, 27 insertions(+), 12 deletions(-) diff --git a/plugins/ipam/host-local/backend/allocator/allocator.go b/plugins/ipam/host-local/backend/allocator/allocator.go index d1c2b1018..2d9f80ec2 100644 --- a/plugins/ipam/host-local/backend/allocator/allocator.go +++ b/plugins/ipam/host-local/backend/allocator/allocator.go @@ -40,7 +40,7 @@ func NewIPAllocator(s *RangeSet, store backend.Store, id int) *IPAllocator { } } -// Get alocates an IP +// Get allocates an IP func (a *IPAllocator) Get(id string, ifname string, requestedIP net.IP) (*current.IPConfig, error) { a.store.Lock() defer a.store.Unlock() @@ -73,24 +73,39 @@ func (a *IPAllocator) Get(id string, ifname string, requestedIP net.IP) (*curren gw = r.Gateway } else { - iter, err := a.GetIter() - if err != nil { - return nil, err - } - for { - reservedIP, gw = iter.Next() - if reservedIP == nil { + // try to get existing IPs which have been allocated to this id + existIPs := a.store.GetByID(id, ifname) + for _, existIP := range existIPs { + // check whether the existing IP belong to this range set + if r, err := a.rangeset.RangeFor(existIP); err == nil { + reservedIP = &net.IPNet{IP: existIP, Mask: r.Subnet.Mask} + gw = r.Gateway break } + } - reserved, err := a.store.Reserve(id, ifname, reservedIP.IP, a.rangeID) + // if no existing IP was found, try to reserve a new one + if reservedIP == nil { + iter, err := a.GetIter() if err != nil { return nil, err } - - if reserved { - break + for { + reservedIP, gw = iter.Next() + if reservedIP == nil { + break + } + + reserved, err := a.store.Reserve(id, ifname, reservedIP.IP, a.rangeID) + if err != nil { + return nil, err + } + + if reserved { + break + } } + } } From eb1ff18c4c87f2d57ee86c7a40712bd0a8d578af Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Sat, 1 Jun 2019 18:06:19 +0800 Subject: [PATCH 3/4] host-local: add some testcases for allocation idempotency Signed-off-by: Bruce Ma --- .../backend/allocator/allocator_test.go | 4 +- plugins/ipam/host-local/host_local_test.go | 100 ++++++++++++++++++ 2 files changed, 102 insertions(+), 2 deletions(-) diff --git a/plugins/ipam/host-local/backend/allocator/allocator_test.go b/plugins/ipam/host-local/backend/allocator/allocator_test.go index 5888c14b0..a7027c152 100644 --- a/plugins/ipam/host-local/backend/allocator/allocator_test.go +++ b/plugins/ipam/host-local/backend/allocator/allocator_test.go @@ -221,14 +221,14 @@ var _ = Describe("host-local ip allocator", func() { It("should not allocate the broadcast address", func() { alloc := mkalloc() for i := 2; i < 7; i++ { - res, err := alloc.Get("ID", "eth0", nil) + res, err := alloc.Get(fmt.Sprintf("ID%d", i), "eth0", nil) Expect(err).ToNot(HaveOccurred()) s := fmt.Sprintf("192.168.1.%d/29", i) Expect(s).To(Equal(res.Address.String())) fmt.Fprintln(GinkgoWriter, "got ip", res.Address.String()) } - x, err := alloc.Get("ID", "eth0", nil) + x, err := alloc.Get("ID8", "eth0", nil) fmt.Fprintln(GinkgoWriter, "got ip", x) Expect(err).To(HaveOccurred()) }) diff --git a/plugins/ipam/host-local/host_local_test.go b/plugins/ipam/host-local/host_local_test.go index 46ab0b56d..d3b487bad 100644 --- a/plugins/ipam/host-local/host_local_test.go +++ b/plugins/ipam/host-local/host_local_test.go @@ -250,6 +250,106 @@ var _ = Describe("host-local Operations", func() { Expect(err).To(HaveOccurred()) }) + It("repeat allocating addresses on specific interface for same container ID with ADD", func() { + const ifname string = "eth0" + const nspath string = "/some/where" + + tmpDir, err := getTmpDir() + Expect(err).NotTo(HaveOccurred()) + defer os.RemoveAll(tmpDir) + + conf := fmt.Sprintf(`{ + "cniVersion": "0.3.1", + "name": "mynet0", + "type": "ipvlan", + "master": "foo0", + "ipam": { + "type": "host-local", + "dataDir": "%s", + "ranges": [ + [{ "subnet": "10.1.2.0/24" }] + ] + } + }`, tmpDir) + + args := &skel.CmdArgs{ + ContainerID: "dummy", + Netns: nspath, + IfName: ifname, + StdinData: []byte(conf), + } + + args1 := &skel.CmdArgs{ + ContainerID: "dummy1", + Netns: nspath, + IfName: ifname, + StdinData: []byte(conf), + } + + // Allocate the IP + r0, raw, err := testutils.CmdAddWithArgs(args, func() error { + return cmdAdd(args) + }) + Expect(err).NotTo(HaveOccurred()) + Expect(strings.Index(string(raw), "\"version\":")).Should(BeNumerically(">", 0)) + + result0, err := current.GetResult(r0) + Expect(err).NotTo(HaveOccurred()) + Expect(len(result0.IPs)).Should(Equal(1)) + Expect(result0.IPs[0].Address.String()).Should(Equal("10.1.2.2/24")) + + // Allocate the IP with the same container ID + r1, raw, err := testutils.CmdAddWithArgs(args, func() error { + return cmdAdd(args) + }) + Expect(err).NotTo(HaveOccurred()) + Expect(strings.Index(string(raw), "\"version\":")).Should(BeNumerically(">", 0)) + + result1, err := current.GetResult(r1) + Expect(err).NotTo(HaveOccurred()) + Expect(len(result1.IPs)).Should(Equal(1)) + Expect(result1.IPs[0].Address.String()).Should(Equal("10.1.2.2/24")) + + // Allocate the IP with the another container ID + r2, raw, err := testutils.CmdAddWithArgs(args1, func() error { + return cmdAdd(args1) + }) + Expect(err).NotTo(HaveOccurred()) + Expect(strings.Index(string(raw), "\"version\":")).Should(BeNumerically(">", 0)) + + result2, err := current.GetResult(r2) + Expect(err).NotTo(HaveOccurred()) + Expect(len(result2.IPs)).Should(Equal(1)) + Expect(result2.IPs[0].Address.String()).Should(Equal("10.1.2.3/24")) + + // Allocate the IP with the same container ID again + r3, raw, err := testutils.CmdAddWithArgs(args, func() error { + return cmdAdd(args) + }) + Expect(err).NotTo(HaveOccurred()) + Expect(strings.Index(string(raw), "\"version\":")).Should(BeNumerically(">", 0)) + + result3, err := current.GetResult(r3) + Expect(err).NotTo(HaveOccurred()) + Expect(len(result3.IPs)).Should(Equal(1)) + Expect(result3.IPs[0].Address.String()).Should(Equal("10.1.2.2/24")) + + ipFilePath := filepath.Join(tmpDir, "mynet0", "10.1.2.2") + + // Release the IPs + err = testutils.CmdDelWithArgs(args, func() error { + return cmdDel(args) + }) + Expect(err).NotTo(HaveOccurred()) + _, err = os.Stat(ipFilePath) + Expect(err).To(HaveOccurred()) + + err = testutils.CmdDelWithArgs(args1, func() error { + return cmdDel(args1) + }) + Expect(err).NotTo(HaveOccurred()) + }) + It("Verify DEL works on backwards compatible allocate", func() { const nspath string = "/some/where" const ifname string = "eth0" From e2984e784011ad0f04fd9f1459d2d01254ee0bd3 Mon Sep 17 00:00:00 2001 From: Bruce Ma Date: Sat, 6 Jul 2019 10:05:18 +0800 Subject: [PATCH 4/4] host-local: return error if duplicate allocation is requested for a given ID Signed-off-by: Bruce Ma --- .../host-local/backend/allocator/allocator.go | 46 +++++++++---------- plugins/ipam/host-local/host_local_test.go | 28 ++++------- 2 files changed, 29 insertions(+), 45 deletions(-) diff --git a/plugins/ipam/host-local/backend/allocator/allocator.go b/plugins/ipam/host-local/backend/allocator/allocator.go index 2d9f80ec2..4cec1a74e 100644 --- a/plugins/ipam/host-local/backend/allocator/allocator.go +++ b/plugins/ipam/host-local/backend/allocator/allocator.go @@ -73,39 +73,35 @@ func (a *IPAllocator) Get(id string, ifname string, requestedIP net.IP) (*curren gw = r.Gateway } else { - // try to get existing IPs which have been allocated to this id - existIPs := a.store.GetByID(id, ifname) - for _, existIP := range existIPs { + // try to get allocated IPs for this given id, if exists, just return error + // because duplicate allocation is not allowed in SPEC + // https://github.com/containernetworking/cni/blob/master/SPEC.md + allocatedIPs := a.store.GetByID(id, ifname) + for _, allocatedIP := range allocatedIPs { // check whether the existing IP belong to this range set - if r, err := a.rangeset.RangeFor(existIP); err == nil { - reservedIP = &net.IPNet{IP: existIP, Mask: r.Subnet.Mask} - gw = r.Gateway - break + if _, err := a.rangeset.RangeFor(allocatedIP); err == nil { + return nil, fmt.Errorf("%s has been allocated to %s, duplicate allocation is not allowed", allocatedIP.String(), id) } } - // if no existing IP was found, try to reserve a new one - if reservedIP == nil { - iter, err := a.GetIter() + iter, err := a.GetIter() + if err != nil { + return nil, err + } + for { + reservedIP, gw = iter.Next() + if reservedIP == nil { + break + } + + reserved, err := a.store.Reserve(id, ifname, reservedIP.IP, a.rangeID) if err != nil { return nil, err } - for { - reservedIP, gw = iter.Next() - if reservedIP == nil { - break - } - - reserved, err := a.store.Reserve(id, ifname, reservedIP.IP, a.rangeID) - if err != nil { - return nil, err - } - - if reserved { - break - } - } + if reserved { + break + } } } diff --git a/plugins/ipam/host-local/host_local_test.go b/plugins/ipam/host-local/host_local_test.go index d3b487bad..6f32c0a36 100644 --- a/plugins/ipam/host-local/host_local_test.go +++ b/plugins/ipam/host-local/host_local_test.go @@ -299,40 +299,28 @@ var _ = Describe("host-local Operations", func() { Expect(result0.IPs[0].Address.String()).Should(Equal("10.1.2.2/24")) // Allocate the IP with the same container ID - r1, raw, err := testutils.CmdAddWithArgs(args, func() error { + _, _, err = testutils.CmdAddWithArgs(args, func() error { return cmdAdd(args) }) - Expect(err).NotTo(HaveOccurred()) - Expect(strings.Index(string(raw), "\"version\":")).Should(BeNumerically(">", 0)) - - result1, err := current.GetResult(r1) - Expect(err).NotTo(HaveOccurred()) - Expect(len(result1.IPs)).Should(Equal(1)) - Expect(result1.IPs[0].Address.String()).Should(Equal("10.1.2.2/24")) + Expect(err).To(HaveOccurred()) // Allocate the IP with the another container ID - r2, raw, err := testutils.CmdAddWithArgs(args1, func() error { + r1, raw, err := testutils.CmdAddWithArgs(args1, func() error { return cmdAdd(args1) }) Expect(err).NotTo(HaveOccurred()) Expect(strings.Index(string(raw), "\"version\":")).Should(BeNumerically(">", 0)) - result2, err := current.GetResult(r2) + result1, err := current.GetResult(r1) Expect(err).NotTo(HaveOccurred()) - Expect(len(result2.IPs)).Should(Equal(1)) - Expect(result2.IPs[0].Address.String()).Should(Equal("10.1.2.3/24")) + Expect(len(result1.IPs)).Should(Equal(1)) + Expect(result1.IPs[0].Address.String()).Should(Equal("10.1.2.3/24")) // Allocate the IP with the same container ID again - r3, raw, err := testutils.CmdAddWithArgs(args, func() error { + _, _, err = testutils.CmdAddWithArgs(args, func() error { return cmdAdd(args) }) - Expect(err).NotTo(HaveOccurred()) - Expect(strings.Index(string(raw), "\"version\":")).Should(BeNumerically(">", 0)) - - result3, err := current.GetResult(r3) - Expect(err).NotTo(HaveOccurred()) - Expect(len(result3.IPs)).Should(Equal(1)) - Expect(result3.IPs[0].Address.String()).Should(Equal("10.1.2.2/24")) + Expect(err).To(HaveOccurred()) ipFilePath := filepath.Join(tmpDir, "mynet0", "10.1.2.2")