From faa0f584b3d1c87195fe327f3eeed39bde0445b5 Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Tue, 4 Aug 2020 21:58:45 -0700 Subject: [PATCH 01/14] ipam leak fix --- cni/ipam/ipam.go | 121 ++++++++++------- cni/ipam/ipam_test.go | 44 +++--- cni/netconfig.go | 1 + cni/network/network.go | 19 +++ ipam/api.go | 32 ++--- ipam/manager_old_test.go | 283 +++++++++++++++++++++++++++++++++++++++ ipam/manager_test.go | 49 +++---- ipam/pool.go | 41 ++++-- ipam/pool_test.go | 40 +++++- 9 files changed, 519 insertions(+), 111 deletions(-) create mode 100644 ipam/manager_old_test.go diff --git a/cni/ipam/ipam.go b/cni/ipam/ipam.go index 785e05616e..a37a3ba950 100644 --- a/cni/ipam/ipam.go +++ b/cni/ipam/ipam.go @@ -149,15 +149,37 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { return err } - // Check if an address pool is specified. - if nwCfg.Ipam.Subnet == "" { + options := make(map[string]string) + options[ipam.OptAddressID] = nwCfg.Ipam.EndpointID + options[ipam.OptInterfaceName] = nwCfg.Master + + var requestPool bool + + // Allocate an address for the endpoint. + err, apInfo, ipAddress := plugin.RequestAddress(options, nwCfg) + + defer func() { + if err != nil && ipAddress != nil { + log.Printf("[cni-ipam] Releasing address %v.", ipAddress) + plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, ipAddress.IP.String(), nil) + } + }() + + if err != nil { + if err == ipam.ErrAddressPoolNotFound { + requestPool = true + } else { + err = plugin.Errorf("Failed to allocate address: %v", err) + return err + } + } + + // Check if an address pool is needed to be created + // if so attempt to create to address pool + if requestPool { var poolID string var subnet string - // Select the requested interface. - options := make(map[string]string) - options[ipam.OptInterfaceName] = nwCfg.Master - isIpv6 := false if nwCfg.Ipam.Type == ipamV6 { isIpv6 = true @@ -180,37 +202,9 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { nwCfg.Ipam.Subnet = subnet log.Printf("[cni-ipam] Allocated address poolID %v with subnet %v.", poolID, subnet) - } - // Allocate an address for the endpoint. - address, err := plugin.am.RequestAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, nil) - if err != nil { - err = plugin.Errorf("Failed to allocate address: %v", err) - return err - } - - // On failure, release the address. - defer func() { - if err != nil && address != "" { - log.Printf("[cni-ipam] Releasing address %v.", address) - plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, address, nil) - } - }() - - log.Printf("[cni-ipam] Allocated address %v.", address) - - // Parse IP address. - ipAddress, err := platform.ConvertStringToIPNet(address) - if err != nil { - err = plugin.Errorf("Failed to parse address: %v", err) - return err - } - - // Query pool information for gateways and DNS servers. - apInfo, err := plugin.am.GetPoolInfo(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) - if err != nil { - err = plugin.Errorf("Failed to get pool information: %v", err) - return err + // Allocate the address as we just got a pool for the address + err, apInfo, ipAddress = plugin.RequestAddress(options, nwCfg) } version := "4" @@ -280,21 +274,25 @@ func (plugin *ipamPlugin) Delete(args *cniSkel.CmdArgs) error { return err } - // If an address is specified, release that address. Otherwise, release the pool. - if nwCfg.Ipam.Address != "" { + // If an address or endpoint id is specified, release that record. Otherwise, release the pool. + if nwCfg.Ipam.Address != "" || nwCfg.Ipam.EndpointID != "" { // Release the address. - err := plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, nil) + + options := make(map[string]string) + options[ipam.OptAddressID] = nwCfg.Ipam.EndpointID + + err := plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, options) if err != nil { err = plugin.Errorf("Failed to release address: %v", err) return err } - } else { - // Release the pool. - err := plugin.am.ReleasePool(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) - if err != nil { - err = plugin.Errorf("Failed to release pool: %v", err) - return err - } + } + + // Release the pool. + err = plugin.am.ReleasePool(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) + if err != nil { + err = plugin.Errorf("Failed to release pool: %v", err) + return err } return nil @@ -304,3 +302,34 @@ func (plugin *ipamPlugin) Delete(args *cniSkel.CmdArgs) error { func (plugin *ipamPlugin) Update(args *cniSkel.CmdArgs) error { return nil } + +// Request Address for CNI IPAM +func (plugin *ipamPlugin) RequestAddress(options map[string]string, nwCfg *cni.NetworkConfig) (error, *ipam.AddressPoolInfo, *net.IPNet) { + var err error + var address string + var ipAddress *net.IPNet + + address, err = plugin.am.RequestAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, options) + + if err != nil { + return err, nil, nil + } + + log.Printf("[cni-ipam] Allocated address %v.", address) + + // Parse IP address. + ipAddress, err = platform.ConvertStringToIPNet(address) + if err != nil { + err = plugin.Errorf("Failed to parse address: %v", err) + return err, nil, nil + } + + // Query pool information for gateways and DNS servers. + apInfo, err := plugin.am.GetPoolInfo(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) + if err != nil { + err = plugin.Errorf("Failed to get pool information: %v", err) + return err, nil, nil + } + + return err, apInfo, ipAddress +} diff --git a/cni/ipam/ipam_test.go b/cni/ipam/ipam_test.go index 5a36ec15b7..600372628b 100644 --- a/cni/ipam/ipam_test.go +++ b/cni/ipam/ipam_test.go @@ -8,6 +8,7 @@ import ( "fmt" cniSkel "github.com/containernetworking/cni/pkg/skel" cniTypesCurr "github.com/containernetworking/cni/pkg/types/current" + "github.com/google/uuid" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" "net/http" @@ -49,25 +50,27 @@ func parseResult(stdinData []byte) (*cniTypesCurr.Result, error) { return result, nil } -func getStdinData(cniversion, subnet, ipAddress string) []byte { +func getStdinData(cniversion, subnet, ipAddress, endPointId string) []byte { stdinData := fmt.Sprintf( `{ "cniversion": "%s", "ipam": { "type": "internal", "subnet": "%s", - "ipAddress": "%s" + "ipAddress": "%s", + "EndpointID": "%s" } - }`, cniversion, subnet, ipAddress) + }`, cniversion, subnet, ipAddress, endPointId) + return []byte(stdinData) } var ( - - plugin *ipamPlugin - testAgent *common.Listener - arg *cniSkel.CmdArgs - err error + plugin *ipamPlugin + testAgent *common.Listener + arg *cniSkel.CmdArgs + err error + endpointID1 = uuid.New().String() _ = BeforeSuite(func() { // TODO: Ensure that the other testAgent has bees released. @@ -120,7 +123,7 @@ var ( Context("When ADD with nothing, call for ipam triggering request pool and address", func() { It("Request pool and ADD successfully", func() { - arg.StdinData = getStdinData("0.4.0", "", "") + arg.StdinData = getStdinData("0.4.0", "", "", endpointID1) err = plugin.Add(arg) Expect(err).ShouldNot(HaveOccurred()) result, err = parseResult(arg.StdinData) @@ -134,7 +137,7 @@ var ( Context("When DELETE with subnet and address, call for ipam triggering release address", func() { It("DELETE address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", result.IPs[0].Address.IP.String()) + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", result.IPs[0].Address.IP.String(), endpointID1) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) }) @@ -142,7 +145,7 @@ var ( Context("When DELETE with subnet, call for ipam triggering releasing pool", func() { It("DELETE pool successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "", endpointID1) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) }) @@ -153,7 +156,7 @@ var ( Context("When address is given", func() { It("Request pool and address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "", "10.0.0.6") + arg.StdinData = getStdinData("0.4.0", "", "10.0.0.6", "") err = plugin.Add(arg) Expect(err).ShouldNot(HaveOccurred()) result, err := parseResult(arg.StdinData) @@ -166,8 +169,9 @@ var ( Context("When subnet is given", func() { It("Request a usable address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "", endpointID1) err = plugin.Add(arg) + Expect(err).ShouldNot(HaveOccurred()) result, err := parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) @@ -182,7 +186,7 @@ var ( Context("When address and subnet is given", func() { It("Release address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.5") + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.5", endpointID1) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) }) @@ -190,7 +194,7 @@ var ( Context("When address and subnet is given", func() { It("Release address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.6") + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.6", endpointID1) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) }) @@ -198,7 +202,15 @@ var ( Context("When subnet is given", func() { It("Release pool successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "", endpointID1) + err = plugin.Delete(arg) + Expect(err).ShouldNot(HaveOccurred()) + }) + }) + + Context("When subnet is given and no Id", func() { + It("Release pool successfully", func() { + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "", "") err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) }) diff --git a/cni/netconfig.go b/cni/netconfig.go index 3faeef8451..6a9e57e292 100644 --- a/cni/netconfig.go +++ b/cni/netconfig.go @@ -69,6 +69,7 @@ type NetworkConfig struct { Subnet string `json:"subnet,omitempty"` Address string `json:"ipAddress,omitempty"` QueryInterval string `json:"queryInterval,omitempty"` + EndpointID string } DNS cniTypes.DNS `json:"dns"` RuntimeConfig RuntimeConfig `json:"runtimeConfig"` diff --git a/cni/network/network.go b/cni/network/network.go index e576b004bd..49ca0302bc 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -462,6 +462,9 @@ func (plugin *netPlugin) Add(args *cniSkel.CmdArgs) error { endpointId := GetEndpointID(args) policies := cni.GetPoliciesFromNwCfg(nwCfg.AdditionalArgs) + // add endpoint ID to leverage in failure cases + nwCfg.Ipam.EndpointID = endpointId + // Check whether the network already exists. nwInfo, nwInfoErr := plugin.nm.GetNetworkInfo(networkId) if nwInfoErr == nil { @@ -834,9 +837,18 @@ func (plugin *netPlugin) Delete(args *cniSkel.CmdArgs) error { } endpointId := GetEndpointID(args) + // add endpoint ID to leverage in failure cases + nwCfg.Ipam.EndpointID = endpointId // Query the network. if nwInfo, err = plugin.nm.GetNetworkInfo(networkId); err != nil { + + // attempt to release address associated with this Endpoint id + // This is to ensure clean up is done even in failure cases + if err = plugin.DelegateDel(nwCfg.Ipam.Type, nwCfg); err != nil { + log.Printf("Network not found, attempted to release address with error: %v", err) + } + // Log the error but return success if the endpoint being deleted is not found. plugin.Errorf("[cni-net] Failed to query network: %v", err) err = nil @@ -845,6 +857,13 @@ func (plugin *netPlugin) Delete(args *cniSkel.CmdArgs) error { // Query the endpoint. if epInfo, err = plugin.nm.GetEndpointInfo(networkId, endpointId); err != nil { + + // attempt to release address associated with this Endpoint id + // This is to ensure clean up is done even in failure cases + if err = plugin.DelegateDel(nwCfg.Ipam.Type, nwCfg); err != nil { + log.Printf("Endpoint not found, attempted to release address with error: %v", err) + } + // Log the error but return success if the endpoint being deleted is not found. plugin.Errorf("[cni-net] Failed to query endpoint: %v", err) err = nil diff --git a/ipam/api.go b/ipam/api.go index dc3be1d725..f973aa603f 100644 --- a/ipam/api.go +++ b/ipam/api.go @@ -9,21 +9,23 @@ import ( var ( // Error responses returned by AddressManager. - errInvalidAddressSpace = fmt.Errorf("Invalid address space") - errInvalidPoolId = fmt.Errorf("Invalid address pool") - errInvalidAddress = fmt.Errorf("Invalid address") - errInvalidScope = fmt.Errorf("Invalid scope") - errInvalidConfiguration = fmt.Errorf("Invalid configuration") - errAddressPoolExists = fmt.Errorf("Address pool already exists") - errAddressPoolNotFound = fmt.Errorf("Address pool not found") - errAddressPoolInUse = fmt.Errorf("Address pool already in use") - errAddressPoolNotInUse = fmt.Errorf("Address pool not in use") - errNoAvailableAddressPools = fmt.Errorf("No available address pools") - errAddressExists = fmt.Errorf("Address already exists") - errAddressNotFound = fmt.Errorf("Address not found") - errAddressInUse = fmt.Errorf("Address already in use") - errAddressNotInUse = fmt.Errorf("Address not in use") - errNoAvailableAddresses = fmt.Errorf("No available addresses") + errInvalidAddressSpace = fmt.Errorf("Invalid address space") + errInvalidPoolId = fmt.Errorf("Invalid address pool") + errInvalidAddress = fmt.Errorf("Invalid address") + errInvalidScope = fmt.Errorf("Invalid scope") + errInvalidConfiguration = fmt.Errorf("Invalid configuration") + errAddressPoolAddressesInUse = fmt.Errorf("Pool has addresses in use") + errAddressPoolExists = fmt.Errorf("Address pool already exists") + errAddressPoolNotFound = fmt.Errorf("Address pool not found") + ErrAddressPoolNotFound = fmt.Errorf("Address pool not found") + errAddressPoolInUse = fmt.Errorf("Address pool already in use") + errAddressPoolNotInUse = fmt.Errorf("Address pool not in use") + errNoAvailableAddressPools = fmt.Errorf("No available address pools") + errAddressExists = fmt.Errorf("Address already exists") + errAddressNotFound = fmt.Errorf("Address not found") + errAddressInUse = fmt.Errorf("Address already in use") + errAddressNotInUse = fmt.Errorf("Address not in use") + errNoAvailableAddresses = fmt.Errorf("No available addresses") // Options used by AddressManager. OptInterfaceName = "azure.interface.name" diff --git a/ipam/manager_old_test.go b/ipam/manager_old_test.go new file mode 100644 index 0000000000..d33098a837 --- /dev/null +++ b/ipam/manager_old_test.go @@ -0,0 +1,283 @@ +// Copyright 2017 Microsoft. All rights reserved. +// MIT License + +package ipam + +import ( + "github.com/google/uuid" + "net" + "testing" +) + +// +// Address manager tests. +// + +// Tests address spaces are created and queried correctly. +func TestAddressSpaceCreateAndGet(t *testing.T) { + // Start with the test address space. + var options map[string]interface{} + + am, err := createAddressManager(options) + if err != nil { + t.Fatalf("createAddressManager failed, err:%+v.", err) + } + + // Test if the address spaces are returned correctly. + local, global := am.GetDefaultAddressSpaces() + + if local != LocalDefaultAddressSpaceId { + t.Errorf("GetDefaultAddressSpaces returned invalid local address space.") + } + + if global != GlobalDefaultAddressSpaceId { + t.Errorf("GetDefaultAddressSpaces returned invalid global address space.") + } +} + +// Tests updating an existing address space adds new resources and removes stale ones. +func TestAddressSpaceUpdate(t *testing.T) { + // Start with the test address space. + var options map[string]interface{} + + am, err := createAddressManager(options) + if err != nil { + t.Fatalf("createAddressManager failed, err:%+v.", err) + } + amImpl := am.(*addressManager) + + // Create a new local address space to update the existing one. + localAs, err := amImpl.newAddressSpace(LocalDefaultAddressSpaceId, LocalScope) + if err != nil { + t.Errorf("newAddressSpace failed, err:%+v.", err) + } + + // Remove addr12 and add addr13 in subnet1. + ap, err := localAs.newAddressPool(anyInterface, anyPriority, &subnet1) + ap.newAddressRecord(&addr11) + ap.newAddressRecord(&addr13) + + // Remove subnet2. + // Add subnet3 with addr31. + ap, err = localAs.newAddressPool(anyInterface, anyPriority, &subnet3) + ap.newAddressRecord(&addr31) + + err = amImpl.setAddressSpace(localAs) + if err != nil { + t.Errorf("setAddressSpace failed, err:%+v.", err) + } + + // Test that the address space was updated correctly. + localAs, err = amImpl.getAddressSpace(LocalDefaultAddressSpaceId) + if err != nil { + t.Errorf("getAddressSpace failed, err:%+v.", err) + } + + // Subnet1 should have addr11 and addr13, but not addr12. + ap, err = localAs.getAddressPool(subnet1.String()) + if err != nil { + t.Errorf("Cannot find subnet1, err:%+v.", err) + } + + _, err = ap.requestAddress(addr11.String(), nil) + if err != nil { + t.Errorf("Cannot find addr11, err:%+v.", err) + } + + _, err = ap.requestAddress(addr12.String(), nil) + if err == nil { + t.Errorf("Found addr12.") + } + + _, err = ap.requestAddress(addr13.String(), nil) + if err != nil { + t.Errorf("Cannot find addr13, err:%+v.", err) + } + + // Subnet2 should not exist. + ap, err = localAs.getAddressPool(subnet2.String()) + if err == nil { + t.Errorf("Found subnet2.") + } + + // Subnet3 should have addr31 only. + ap, err = localAs.getAddressPool(subnet3.String()) + if err != nil { + t.Errorf("Cannot find subnet3, err:%+v.", err) + } + + _, err = ap.requestAddress(addr31.String(), nil) + if err != nil { + t.Errorf("Cannot find addr31, err:%+v.", err) + } + + _, err = ap.requestAddress(addr32.String(), nil) + if err == nil { + t.Errorf("Found addr32.") + } +} + +// Tests multiple wildcard address pool requests return separate pools. +func TestAddressPoolRequestsForSeparatePools(t *testing.T) { + // Start with the test address space. + var options map[string]interface{} + am, err := createAddressManager(options) + if err != nil { + t.Fatalf("createAddressManager failed, err:%+v.", err) + } + + // Request two separate address pools. + poolId1, subnet1, err := am.RequestPool(LocalDefaultAddressSpaceId, "", "", nil, false) + if err != nil { + t.Errorf("RequestPool failed, err:%v", err) + } + + poolId2, subnet2, err := am.RequestPool(LocalDefaultAddressSpaceId, "", "", nil, false) + if err != nil { + t.Errorf("RequestPool failed, err:%v", err) + } + + // Test the poolIds and subnets do not match. + if poolId1 == poolId2 || subnet1 == subnet2 { + t.Errorf("Pool requests returned the same pool.") + } + + // Release the address pools. + err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId1) + if err != nil { + t.Errorf("ReleasePool failed, err:%v", err) + } + + err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId2) + if err != nil { + t.Errorf("ReleasePool failed, err:%v", err) + } +} + +// Tests multiple identical address pool requests return the same pool and pools are referenced correctly. +func TestAddressPoolRequestsForSamePool(t *testing.T) { + // Start with the test address space. + var options map[string]interface{} + + am, err := createAddressManager(options) + if err != nil { + t.Fatalf("createAddressManager failed, err:%+v.", err) + } + + // Request the same address pool twice. + poolId1, subnet1, err := am.RequestPool(LocalDefaultAddressSpaceId, "", "", nil, false) + if err != nil { + t.Errorf("RequestPool failed, err:%v", err) + } + + poolId2, subnet2, err := am.RequestPool(LocalDefaultAddressSpaceId, poolId1, "", nil, false) + if err != nil { + t.Errorf("RequestPool failed, err:%v", err) + } + + // Test the subnets do not match. + if poolId1 != poolId2 || subnet1 != subnet2 { + t.Errorf("Pool requests returned different pools.") + } + + // Release the address pools. + err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId1) + if err != nil { + t.Errorf("ReleasePool failed, err:%v", err) + } + + err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId2) + if err != nil { + t.Errorf("ReleasePool failed, err:%v", err) + } + + // Third release should fail. + err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId1) + if err == nil { + t.Errorf("ReleasePool succeeded extra, err:%v", err) + } +} + +// Tests address requests from the same pool return separate addresses and releases work correctly. +func TestAddressRequestsFromTheSamePool(t *testing.T) { + // Start with the test address space. + var options map[string]interface{} + + am, err := createAddressManager(options) + if err != nil { + t.Fatalf("createAddressManager failed, err:%+v.", err) + } + + // Request a pool. + poolId, _, err := am.RequestPool(LocalDefaultAddressSpaceId, "", "", nil, false) + if err != nil { + t.Errorf("RequestPool failed, err:%v", err) + } + + options1 := make(map[string]string) + options1[OptAddressID] = uuid.New().String() + + options2 := make(map[string]string) + options2[OptAddressID] = uuid.New().String() + + options3 := make(map[string]string) + options3[OptAddressID] = uuid.New().String() + + // Request two addresses from the pool. + address1, err := am.RequestAddress(LocalDefaultAddressSpaceId, poolId, "", options1) + if err != nil { + t.Errorf("RequestAddress failed, err:%v", err) + } + + addr, _, _ := net.ParseCIDR(address1) + address1 = addr.String() + + address2, err := am.RequestAddress(LocalDefaultAddressSpaceId, poolId, "", options2) + if err != nil { + t.Errorf("RequestAddress failed, err:%v", err) + } + + addr, _, _ = net.ParseCIDR(address2) + address2 = addr.String() + + // Request four addresses from the pool. + address3, err := am.RequestAddress(LocalDefaultAddressSpaceId, poolId, "", options3) + if err != nil { + t.Errorf("RequestAddress failed, err:%v", err) + } + + addr, _, _ = net.ParseCIDR(address3) + address3 = addr.String() + + var m map[string]string + _, exists := m[address1] + _, exists = m[address2] + _, exists = m[address3] + + // Test the addresses do not match. + if exists { + t.Errorf("Address requests returned the same address %v.", address1) + } + + // Release addresses and the pool. + err = am.ReleaseAddress(LocalDefaultAddressSpaceId, poolId, address1, options1) + if err != nil { + t.Errorf("ReleaseAddress failed, err:%v", err) + } + + err = am.ReleaseAddress(LocalDefaultAddressSpaceId, poolId, address2, options2) + if err != nil { + t.Errorf("ReleaseAddress failed, err:%v", err) + } + + // Release addresses and the pool. + err = am.ReleaseAddress(LocalDefaultAddressSpaceId, poolId, "", options3) + if err != nil { + t.Errorf("ReleaseAddress failed, err:%v", err) + } + + err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId) + if err != nil { + t.Errorf("ReleasePool failed, err:%v", err) + } +} diff --git a/ipam/manager_test.go b/ipam/manager_test.go index 30e0e05018..0c1f0ec251 100644 --- a/ipam/manager_test.go +++ b/ipam/manager_test.go @@ -102,6 +102,9 @@ func setupTestAddressSpace(am AddressManager) error { ap, err := localAs.newAddressPool(anyInterface, anyPriority, &subnet1) ap.newAddressRecord(&addr11) ap.newAddressRecord(&addr12) + ap.newAddressRecord(&addr13) + ap.newAddressRecord(&addr22) + ap.newAddressRecord(&addr32) // Add subnet2 with addr21. ap, err = localAs.newAddressPool(anyInterface, anyPriority, &subnet2) @@ -233,17 +236,17 @@ var ( ModificationTime: timeReboot.Add(time.Hour), } ap := &addressPool{ - Id: "ap-test", - RefCount: 1, - Addresses: make(map[string]*addressRecord), + Id: "ap-test", + RefCount: 1, + Addresses: make(map[string]*addressRecord), } ap.Addresses["ar-test"] = &addressRecord{ - ID: "ar-test", - InUse: true, + ID: "ar-test", + InUse: true, } as := &addressSpace{ - Id: "as-test", - Pools: make(map[string]*addressPool), + Id: "as-test", + Pools: make(map[string]*addressPool), } as.Pools["ap-test"] = ap am.AddrSpaces["as-test"] = as @@ -267,17 +270,17 @@ var ( GetModificationTimeError: errors.New("Error"), } ap := &addressPool{ - Id: "ap-test", - RefCount: 1, - Addresses: make(map[string]*addressRecord), + Id: "ap-test", + RefCount: 1, + Addresses: make(map[string]*addressRecord), } ap.Addresses["ar-test"] = &addressRecord{ - ID: "ar-test", - InUse: true, + ID: "ar-test", + InUse: true, } as := &addressSpace{ - Id: "as-test", - Pools: make(map[string]*addressPool), + Id: "as-test", + Pools: make(map[string]*addressPool), } as.Pools["ap-test"] = ap am.AddrSpaces["as-test"] = as @@ -299,17 +302,17 @@ var ( } am.store = &testutils.KeyValueStoreMock{} ap := &addressPool{ - Id: "ap-test", - RefCount: 1, - Addresses: make(map[string]*addressRecord), + Id: "ap-test", + RefCount: 1, + Addresses: make(map[string]*addressRecord), } ap.Addresses["ar-test"] = &addressRecord{ - ID: "ar-test", - InUse: true, + ID: "ar-test", + InUse: true, } as := &addressSpace{ - Id: "as-test", - Pools: make(map[string]*addressPool), + Id: "as-test", + Pools: make(map[string]*addressPool), } as.Pools["ap-test"] = ap am.AddrSpaces["as-test"] = as @@ -409,8 +412,8 @@ var ( am := &addressManager{ AddrSpaces: make(map[string]*addressSpace), } - am.AddrSpaces[LocalDefaultAddressSpaceId] = &addressSpace{Id:"localId"} - am.AddrSpaces[GlobalDefaultAddressSpaceId] = &addressSpace{Id:"globalId"} + am.AddrSpaces[LocalDefaultAddressSpaceId] = &addressSpace{Id: "localId"} + am.AddrSpaces[GlobalDefaultAddressSpaceId] = &addressSpace{Id: "globalId"} localId, globalId := am.GetDefaultAddressSpaces() Expect(localId).To(Equal("localId")) Expect(globalId).To(Equal("globalId")) diff --git a/ipam/pool.go b/ipam/pool.go index f7ebbaff01..8b012e34ed 100644 --- a/ipam/pool.go +++ b/ipam/pool.go @@ -159,6 +159,8 @@ func (am *addressManager) setAddressSpace(as *addressSpace) error { if !ok { am.AddrSpaces[as.Id] = as } else { + //merges the address set refreshed from the source + //and the ones we have currently this address space as1.merge(as) } @@ -177,6 +179,9 @@ func (am *addressManager) setAddressSpace(as *addressSpace) error { // Merges a new address space to an existing one. func (as *addressSpace) merge(newas *addressSpace) { // The new epoch after the merge. + //epoch is essentially the count of invocations + // used to ensure if certain addresses refreshed from the source + // are still relevant as.epoch++ // Add new pools and addresses. @@ -278,7 +283,7 @@ func (as *addressSpace) newAddressPool(ifName string, priority int, subnet *net. func (as *addressSpace) getAddressPool(poolId string) (*addressPool, error) { ap := as.Pools[poolId] if ap == nil { - return nil, errInvalidPoolId + return nil, ErrAddressPoolNotFound } return ap, nil @@ -308,6 +313,11 @@ func (as *addressSpace) requestPool(poolId string, subPoolId string, options map // Skip if pool is already in use. if pool.isInUse() { log.Printf("[ipam] Pool is in use.") + + // in the case we + if !pool.IsAnyRecordInUse() { + as.releasePool(poolId) + } continue } @@ -361,18 +371,18 @@ func (as *addressSpace) requestPool(poolId string, subPoolId string, options map func (as *addressSpace) releasePool(poolId string) error { var err error - log.Printf("[ipam] Releasing pool with poolId:%v.", poolId) + log.Printf("[ipam] Attempting to releasing pool with poolId:%v.", poolId) ap, ok := as.Pools[poolId] if !ok { err = errAddressPoolNotFound - } else { - if !ap.isInUse() { - err = errAddressPoolNotInUse - } + } else if ap.IsAnyRecordInUse() { + err = errAddressPoolAddressesInUse + log.Printf("[ipam] Skip releasing pool with poolId:%v. due to: %v", + poolId, errAddressPoolAddressesInUse) } - if err != nil { + if err != nil && err != errAddressPoolAddressesInUse { log.Printf("[ipam] Failed to release pool, err:%v.", err) return err } @@ -380,7 +390,7 @@ func (as *addressSpace) releasePool(poolId string) error { ap.RefCount-- // Delete address pool if it is no longer available. - if ap.epoch < as.epoch && !ap.isInUse() { + if !ap.isInUse() { log.Printf("[ipam] Deleting stale pool with poolId:%v.", poolId) delete(as.Pools, poolId) } @@ -423,6 +433,16 @@ func (ap *addressPool) isInUse() bool { return ap.RefCount > 0 } +// Returns if any address in the pool is currently in use. +func (ap *addressPool) IsAnyRecordInUse() bool { + for _, address := range ap.Addresses { + if address.InUse || address.ID != "" { + return true + } + } + return false +} + // Creates a new addressRecord object. func (ap *addressPool) newAddressRecord(addr *net.IP) (*addressRecord, error) { id := addr.String() @@ -484,7 +504,7 @@ func (ap *addressPool) requestAddress(address string, options map[string]string) // If no address was found, return any available address. if ar == nil { for _, ar = range ap.Addresses { - if !ar.InUse && ar.ID == "" { + if !ar.InUse { break } ar = nil @@ -498,6 +518,7 @@ func (ap *addressPool) requestAddress(address string, options map[string]string) if id != "" { ap.addrsByID[id] = ar ar.ID = id + ar.InUse = true } else { ar.InUse = true } @@ -546,7 +567,7 @@ func (ap *addressPool) releaseAddress(address string, options map[string]string) return nil } - if !ar.InUse { + if !ar.InUse && ar.ID == "" { log.Printf("Address not in use. Not Returning error") return nil } diff --git a/ipam/pool_test.go b/ipam/pool_test.go index 99eb1e1018..a16157634f 100644 --- a/ipam/pool_test.go +++ b/ipam/pool_test.go @@ -1,6 +1,7 @@ package ipam import ( + "github.com/google/uuid" "net" "testing" @@ -654,7 +655,7 @@ var ( Addresses: map[string]*addressRecord{}, } as.Pools["10.1.0.0/16"] = &addressPool{ - Id: "10.1.0.0/16", + Id: "10.1.0.0/16", Addresses: map[string]*addressRecord{ "10.1.0.1/16": &addressRecord{}, }, @@ -829,6 +830,43 @@ var ( }) }) + Context("When id is not found and a address is available", func() { + It("Should return a new address", func() { + ap := &addressPool{ + Addresses: map[string]*addressRecord{}, + addrsByID: map[string]*addressRecord{}, + Subnet: subnet1, + } + arId := uuid.New().String() + + ap.Addresses["0"] = &addressRecord{ + ID: "", + Addr: addr11, + InUse: false, + } + + ap.Addresses["1"] = &addressRecord{ + ID: "", + Addr: addr12, + InUse: false, + } + + ap.Addresses["3"] = &addressRecord{ + ID: "", + Addr: addr13, + InUse: false, + } + + options := map[string]string{} + options[OptAddressID] = arId + + addr, err := ap.requestAddress("", options) + Expect(err).NotTo(HaveOccurred()) + Expect(addr).NotTo(BeEmpty()) + Expect(ap.addrsByID[arId].ID).To(Equal(arId)) + }) + }) + Context("When addressRecord is in use and id is empty", func() { It("Should raise errAddressInUse", func() { ap := &addressPool{ From 9a05e5c73e7da41f38a9c6acbee791f1b2b8111f Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Tue, 4 Aug 2020 22:12:53 -0700 Subject: [PATCH 02/14] don't decrement refCount if addresses are in use --- ipam/api.go | 33 ++++++++++++++++----------------- ipam/pool.go | 22 ++++++++++++---------- 2 files changed, 28 insertions(+), 27 deletions(-) diff --git a/ipam/api.go b/ipam/api.go index f973aa603f..c6670e399c 100644 --- a/ipam/api.go +++ b/ipam/api.go @@ -9,23 +9,22 @@ import ( var ( // Error responses returned by AddressManager. - errInvalidAddressSpace = fmt.Errorf("Invalid address space") - errInvalidPoolId = fmt.Errorf("Invalid address pool") - errInvalidAddress = fmt.Errorf("Invalid address") - errInvalidScope = fmt.Errorf("Invalid scope") - errInvalidConfiguration = fmt.Errorf("Invalid configuration") - errAddressPoolAddressesInUse = fmt.Errorf("Pool has addresses in use") - errAddressPoolExists = fmt.Errorf("Address pool already exists") - errAddressPoolNotFound = fmt.Errorf("Address pool not found") - ErrAddressPoolNotFound = fmt.Errorf("Address pool not found") - errAddressPoolInUse = fmt.Errorf("Address pool already in use") - errAddressPoolNotInUse = fmt.Errorf("Address pool not in use") - errNoAvailableAddressPools = fmt.Errorf("No available address pools") - errAddressExists = fmt.Errorf("Address already exists") - errAddressNotFound = fmt.Errorf("Address not found") - errAddressInUse = fmt.Errorf("Address already in use") - errAddressNotInUse = fmt.Errorf("Address not in use") - errNoAvailableAddresses = fmt.Errorf("No available addresses") + errInvalidAddressSpace = fmt.Errorf("Invalid address space") + errInvalidPoolId = fmt.Errorf("Invalid address pool") + errInvalidAddress = fmt.Errorf("Invalid address") + errInvalidScope = fmt.Errorf("Invalid scope") + errInvalidConfiguration = fmt.Errorf("Invalid configuration") + errAddressPoolExists = fmt.Errorf("Address pool already exists") + errAddressPoolNotFound = fmt.Errorf("Address pool not found") + ErrAddressPoolNotFound = fmt.Errorf("Address pool not found") + errAddressPoolInUse = fmt.Errorf("Address pool already in use") + errAddressPoolNotInUse = fmt.Errorf("Address pool not in use") + errNoAvailableAddressPools = fmt.Errorf("No available address pools") + errAddressExists = fmt.Errorf("Address already exists") + errAddressNotFound = fmt.Errorf("Address not found") + errAddressInUse = fmt.Errorf("Address already in use") + errAddressNotInUse = fmt.Errorf("Address not in use") + errNoAvailableAddresses = fmt.Errorf("No available addresses") // Options used by AddressManager. OptInterfaceName = "azure.interface.name" diff --git a/ipam/pool.go b/ipam/pool.go index 8b012e34ed..03589b4369 100644 --- a/ipam/pool.go +++ b/ipam/pool.go @@ -370,29 +370,31 @@ func (as *addressSpace) requestPool(poolId string, subPoolId string, options map // Releases a previously requested address pool back to its address space. func (as *addressSpace) releasePool(poolId string) error { var err error + var addressesInUse bool log.Printf("[ipam] Attempting to releasing pool with poolId:%v.", poolId) ap, ok := as.Pools[poolId] if !ok { err = errAddressPoolNotFound - } else if ap.IsAnyRecordInUse() { - err = errAddressPoolAddressesInUse - log.Printf("[ipam] Skip releasing pool with poolId:%v. due to: %v", - poolId, errAddressPoolAddressesInUse) + } else if addressesInUse = ap.IsAnyRecordInUse(); addressesInUse { + log.Printf("[ipam] Skip releasing pool with poolId:%v. due to address being in use", + poolId) } - if err != nil && err != errAddressPoolAddressesInUse { + if err != nil { log.Printf("[ipam] Failed to release pool, err:%v.", err) return err } - ap.RefCount-- + if !addressesInUse { + ap.RefCount-- - // Delete address pool if it is no longer available. - if !ap.isInUse() { - log.Printf("[ipam] Deleting stale pool with poolId:%v.", poolId) - delete(as.Pools, poolId) + // Delete address pool if it is no longer available. + if !ap.isInUse() { + log.Printf("[ipam] Deleting stale pool with poolId:%v.", poolId) + delete(as.Pools, poolId) + } } return nil From fc0a15f0330b3ae68248c81431cb6c059107824f Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Tue, 4 Aug 2020 22:32:18 -0700 Subject: [PATCH 03/14] capture error on rerequesting address --- cni/ipam/ipam.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/cni/ipam/ipam.go b/cni/ipam/ipam.go index a37a3ba950..7b3f15af57 100644 --- a/cni/ipam/ipam.go +++ b/cni/ipam/ipam.go @@ -205,6 +205,11 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { // Allocate the address as we just got a pool for the address err, apInfo, ipAddress = plugin.RequestAddress(options, nwCfg) + + if err != nil { + err = plugin.Errorf("Failed to allocate address: %v", err) + return err + } } version := "4" From 44607247d362a3218c83ef4ab020dc2c90601d7f Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Thu, 6 Aug 2020 16:54:09 -0700 Subject: [PATCH 04/14] fixing up some tests --- ipam/azure_test.go | 26 ++++++++++++++++----- ipam/pool_test.go | 57 +++++++++++++++++++++++++++++----------------- 2 files changed, 56 insertions(+), 27 deletions(-) diff --git a/ipam/azure_test.go b/ipam/azure_test.go index b754e23eac..0553dedca7 100644 --- a/ipam/azure_test.go +++ b/ipam/azure_test.go @@ -4,11 +4,11 @@ package ipam import ( + "encoding/json" "net/http" "net/url" "testing" "time" - "encoding/json" cniTypesCurr "github.com/containernetworking/cni/pkg/types/current" . "github.com/onsi/ginkgo" @@ -54,8 +54,8 @@ var ( var ( testAgent *common.Listener - source *azureSource - err error + source *azureSource + err error ) BeforeSuite(func() { @@ -111,11 +111,11 @@ var ( options := make(map[string]interface{}) options[common.OptEnvironment] = common.OptEnvironmentAzure options[common.OptAPIServerURL] = "null" - options[common.OptIpamQueryUrl] = "http://"+ipamQueryUrl + options[common.OptIpamQueryUrl] = "http://" + ipamQueryUrl source, err = newAzureSource(options) Expect(err).ShouldNot(HaveOccurred()) Expect(source.name).Should(Equal("Azure")) - Expect(source.queryUrl).Should(Equal("http://"+ipamQueryUrl)) + Expect(source.queryUrl).Should(Equal("http://" + ipamQueryUrl)) Expect(source.queryInterval).Should(Equal(azureQueryInterval)) }) }) @@ -139,6 +139,12 @@ var ( err = source.start(sink) Expect(err).NotTo(HaveOccurred()) Expect(source.sink).NotTo(BeNil()) + // this is to avoid a race condition that fails this test + //source.queryInterval defaults to 1 nanosecond + // if this test moves fast enough, it will have the refresh method + // return on this check if time.Since(s.lastRefresh) < s.queryInterval + + source.lastRefresh = time.Now().Add(-1 * time.Second) err = source.refresh() Expect(err).To(HaveOccurred()) }) @@ -153,6 +159,14 @@ var ( err = source.start(sink) Expect(err).NotTo(HaveOccurred()) Expect(source.sink).NotTo(BeNil()) + + // this is to avoid a race condition that fails this test + //source.queryInterval defaults to 1 nanosecond + // if this test moves fast enough, it will have the refresh method + // return on this check if time.Since(s.lastRefresh) < s.queryInterval + + source.lastRefresh = time.Now().Add(-1 * time.Second) + err = source.refresh() Expect(err).To(HaveOccurred()) }) @@ -163,7 +177,7 @@ var ( options := make(map[string]interface{}) options[common.OptEnvironment] = common.OptEnvironmentAzure options[common.OptAPIServerURL] = "null" - options[common.OptIpamQueryUrl] = "http://"+ipamQueryUrl + options[common.OptIpamQueryUrl] = "http://" + ipamQueryUrl am, err := createAddressManager(options) Expect(err).ToNot(HaveOccurred()) diff --git a/ipam/pool_test.go b/ipam/pool_test.go index a16157634f..c109576ded 100644 --- a/ipam/pool_test.go +++ b/ipam/pool_test.go @@ -510,7 +510,7 @@ var ( Pools: map[string]*addressPool{}, } pool, err := as.getAddressPool("10.0.0.0/16") - Expect(err).To(Equal(errInvalidPoolId)) + Expect(err).To(Equal(errAddressPoolNotFound)) Expect(pool).To(BeNil()) }) }) @@ -679,39 +679,39 @@ var ( }) }) - Context("When pool is not in use", func() { - It("Should raise an error", func() { + Context("When pool's addresses are all not in use", func() { + It("Should release the pool ", func() { poolId := "10.0.0.0/16" as := &addressSpace{ + epoch: 2, Pools: map[string]*addressPool{}, } as.Pools[poolId] = &addressPool{ - RefCount: 0, + epoch: 1, + RefCount: 1, + Addresses: map[string]*addressRecord{}, } - err := as.releasePool("10.0.0.0/16") - Expect(err).To(Equal(errAddressPoolNotInUse)) - Expect(as.Pools[poolId]).NotTo(BeNil()) - }) - }) - Context("When pool's epoch is less than the space's epoch and pool is never in use", func() { - It("Should release the pool ", func() { - poolId := "10.0.0.0/16" - as := &addressSpace{ - epoch: 2, - Pools: map[string]*addressPool{}, + as.Pools[poolId].Addresses["10.0.0.1"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, } - as.Pools[poolId] = &addressPool{ - epoch: 1, - RefCount: 1, + as.Pools[poolId].Addresses["10.0.0.2"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, + } + as.Pools[poolId].Addresses["10.0.0.3"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, } + err := as.releasePool("10.0.0.0/16") Expect(err).NotTo(HaveOccurred()) Expect(as.Pools[poolId]).To(BeNil()) }) }) - Context("When the epoch of pool is equal to the epoch of addressSpace", func() { + Context("When the pool has in use addresses", func() { It("Should not delete the pool", func() { poolId := "10.0.0.0/16" as := &addressSpace{ @@ -719,9 +719,24 @@ var ( Pools: map[string]*addressPool{}, } as.Pools[poolId] = &addressPool{ - epoch: 1, - RefCount: 1, + epoch: 1, + RefCount: 1, + Addresses: map[string]*addressRecord{}, } + + as.Pools[poolId].Addresses["10.0.0.1"] = &addressRecord{ + InUse: true, + Addr: net.IPv4zero, + } + as.Pools[poolId].Addresses["10.0.0.2"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, + } + as.Pools[poolId].Addresses["10.0.0.3"] = &addressRecord{ + InUse: false, + Addr: net.IPv4zero, + } + err := as.releasePool("10.0.0.0/16") Expect(err).NotTo(HaveOccurred()) Expect(as.Pools[poolId]).NotTo(BeNil()) From 44706cc6a73d95c8ef9bb5db2e27ebcb118a1527 Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Mon, 10 Aug 2020 14:31:25 -0700 Subject: [PATCH 05/14] reorder a test --- cnm/ipam/ipam_test.go | 42 +++++++++++++++++++++--------------------- 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/cnm/ipam/ipam_test.go b/cnm/ipam/ipam_test.go index c83231fe5b..018296cc44 100644 --- a/cnm/ipam/ipam_test.go +++ b/cnm/ipam/ipam_test.go @@ -234,19 +234,18 @@ func TestRequestAddress(t *testing.T) { address1 = address.String() } -// Tests IpamDriver.ReleaseAddress functionality. -func TestReleaseAddress(t *testing.T) { +// Tests IpamDriver.GetPoolInfo functionality. +func TestGetPoolInfo(t *testing.T) { var body bytes.Buffer - var resp ReleaseAddressResponse + var resp GetPoolInfoResponse - payload := &ReleaseAddressRequest{ - PoolID: poolId1, - Address: address1, + payload := &GetPoolInfoRequest{ + PoolID: poolId1, } json.NewEncoder(&body).Encode(payload) - req, err := http.NewRequest(http.MethodGet, ReleaseAddressPath, &body) + req, err := http.NewRequest(http.MethodGet, GetPoolInfoPath, &body) if err != nil { t.Fatal(err) } @@ -257,22 +256,23 @@ func TestReleaseAddress(t *testing.T) { err = decodeResponse(w, &resp) if err != nil || resp.Err != "" { - t.Errorf("ReleaseAddress response is invalid %+v", resp) + t.Errorf("GetPoolInfo response is invalid %+v", resp) } } -// Tests IpamDriver.ReleasePool functionality. -func TestReleasePool(t *testing.T) { +// Tests IpamDriver.ReleaseAddress functionality. +func TestReleaseAddress(t *testing.T) { var body bytes.Buffer - var resp ReleasePoolResponse + var resp ReleaseAddressResponse - payload := &ReleasePoolRequest{ - PoolID: poolId1, + payload := &ReleaseAddressRequest{ + PoolID: poolId1, + Address: address1, } json.NewEncoder(&body).Encode(payload) - req, err := http.NewRequest(http.MethodGet, ReleasePoolPath, &body) + req, err := http.NewRequest(http.MethodGet, ReleaseAddressPath, &body) if err != nil { t.Fatal(err) } @@ -283,22 +283,22 @@ func TestReleasePool(t *testing.T) { err = decodeResponse(w, &resp) if err != nil || resp.Err != "" { - t.Errorf("ReleasePool response is invalid %+v", resp) + t.Errorf("ReleaseAddress response is invalid %+v", resp) } } -// Tests IpamDriver.GetPoolInfo functionality. -func TestGetPoolInfo(t *testing.T) { +// Tests IpamDriver.ReleasePool functionality. +func TestReleasePool(t *testing.T) { var body bytes.Buffer - var resp GetPoolInfoResponse + var resp ReleasePoolResponse - payload := &GetPoolInfoRequest{ + payload := &ReleasePoolRequest{ PoolID: poolId1, } json.NewEncoder(&body).Encode(payload) - req, err := http.NewRequest(http.MethodGet, GetPoolInfoPath, &body) + req, err := http.NewRequest(http.MethodGet, ReleasePoolPath, &body) if err != nil { t.Fatal(err) } @@ -309,7 +309,7 @@ func TestGetPoolInfo(t *testing.T) { err = decodeResponse(w, &resp) if err != nil || resp.Err != "" { - t.Errorf("GetPoolInfo response is invalid %+v", resp) + t.Errorf("ReleasePool response is invalid %+v", resp) } } From 7448bac52d7cad1d34713ebae47be0d3a749f5fe Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Mon, 10 Aug 2020 17:10:42 -0700 Subject: [PATCH 06/14] revert IPAM.go file --- cni/ipam/ipam.go | 122 +++++++++++++++++------------------------------ ipam/pool.go | 3 +- 2 files changed, 46 insertions(+), 79 deletions(-) diff --git a/cni/ipam/ipam.go b/cni/ipam/ipam.go index 7b3f15af57..785e05616e 100644 --- a/cni/ipam/ipam.go +++ b/cni/ipam/ipam.go @@ -149,37 +149,15 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { return err } - options := make(map[string]string) - options[ipam.OptAddressID] = nwCfg.Ipam.EndpointID - options[ipam.OptInterfaceName] = nwCfg.Master - - var requestPool bool - - // Allocate an address for the endpoint. - err, apInfo, ipAddress := plugin.RequestAddress(options, nwCfg) - - defer func() { - if err != nil && ipAddress != nil { - log.Printf("[cni-ipam] Releasing address %v.", ipAddress) - plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, ipAddress.IP.String(), nil) - } - }() - - if err != nil { - if err == ipam.ErrAddressPoolNotFound { - requestPool = true - } else { - err = plugin.Errorf("Failed to allocate address: %v", err) - return err - } - } - - // Check if an address pool is needed to be created - // if so attempt to create to address pool - if requestPool { + // Check if an address pool is specified. + if nwCfg.Ipam.Subnet == "" { var poolID string var subnet string + // Select the requested interface. + options := make(map[string]string) + options[ipam.OptInterfaceName] = nwCfg.Master + isIpv6 := false if nwCfg.Ipam.Type == ipamV6 { isIpv6 = true @@ -202,14 +180,37 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { nwCfg.Ipam.Subnet = subnet log.Printf("[cni-ipam] Allocated address poolID %v with subnet %v.", poolID, subnet) + } - // Allocate the address as we just got a pool for the address - err, apInfo, ipAddress = plugin.RequestAddress(options, nwCfg) + // Allocate an address for the endpoint. + address, err := plugin.am.RequestAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, nil) + if err != nil { + err = plugin.Errorf("Failed to allocate address: %v", err) + return err + } - if err != nil { - err = plugin.Errorf("Failed to allocate address: %v", err) - return err + // On failure, release the address. + defer func() { + if err != nil && address != "" { + log.Printf("[cni-ipam] Releasing address %v.", address) + plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, address, nil) } + }() + + log.Printf("[cni-ipam] Allocated address %v.", address) + + // Parse IP address. + ipAddress, err := platform.ConvertStringToIPNet(address) + if err != nil { + err = plugin.Errorf("Failed to parse address: %v", err) + return err + } + + // Query pool information for gateways and DNS servers. + apInfo, err := plugin.am.GetPoolInfo(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) + if err != nil { + err = plugin.Errorf("Failed to get pool information: %v", err) + return err } version := "4" @@ -279,25 +280,21 @@ func (plugin *ipamPlugin) Delete(args *cniSkel.CmdArgs) error { return err } - // If an address or endpoint id is specified, release that record. Otherwise, release the pool. - if nwCfg.Ipam.Address != "" || nwCfg.Ipam.EndpointID != "" { + // If an address is specified, release that address. Otherwise, release the pool. + if nwCfg.Ipam.Address != "" { // Release the address. - - options := make(map[string]string) - options[ipam.OptAddressID] = nwCfg.Ipam.EndpointID - - err := plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, options) + err := plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, nil) if err != nil { err = plugin.Errorf("Failed to release address: %v", err) return err } - } - - // Release the pool. - err = plugin.am.ReleasePool(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) - if err != nil { - err = plugin.Errorf("Failed to release pool: %v", err) - return err + } else { + // Release the pool. + err := plugin.am.ReleasePool(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) + if err != nil { + err = plugin.Errorf("Failed to release pool: %v", err) + return err + } } return nil @@ -307,34 +304,3 @@ func (plugin *ipamPlugin) Delete(args *cniSkel.CmdArgs) error { func (plugin *ipamPlugin) Update(args *cniSkel.CmdArgs) error { return nil } - -// Request Address for CNI IPAM -func (plugin *ipamPlugin) RequestAddress(options map[string]string, nwCfg *cni.NetworkConfig) (error, *ipam.AddressPoolInfo, *net.IPNet) { - var err error - var address string - var ipAddress *net.IPNet - - address, err = plugin.am.RequestAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, options) - - if err != nil { - return err, nil, nil - } - - log.Printf("[cni-ipam] Allocated address %v.", address) - - // Parse IP address. - ipAddress, err = platform.ConvertStringToIPNet(address) - if err != nil { - err = plugin.Errorf("Failed to parse address: %v", err) - return err, nil, nil - } - - // Query pool information for gateways and DNS servers. - apInfo, err := plugin.am.GetPoolInfo(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) - if err != nil { - err = plugin.Errorf("Failed to get pool information: %v", err) - return err, nil, nil - } - - return err, apInfo, ipAddress -} diff --git a/ipam/pool.go b/ipam/pool.go index 03589b4369..0d776c2a13 100644 --- a/ipam/pool.go +++ b/ipam/pool.go @@ -314,7 +314,8 @@ func (as *addressSpace) requestPool(poolId string, subPoolId string, options map if pool.isInUse() { log.Printf("[ipam] Pool is in use.") - // in the case we + // in case the pool is actually not in use, + // attempt to release it if !pool.IsAnyRecordInUse() { as.releasePool(poolId) } From 72b6f4edbb08de0c1808e66772da919fb5ac95fd Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Sat, 15 Aug 2020 14:54:41 -0700 Subject: [PATCH 07/14] adding couple more changes --- cni/ipam/ipam.go | 41 ++++++----- cni/ipam/ipam_test.go | 150 +++++++++++++++++++++++++++++++++-------- cni/netconfig.go | 1 - cni/network/network.go | 5 -- ipam/pool.go | 6 +- 5 files changed, 146 insertions(+), 57 deletions(-) diff --git a/cni/ipam/ipam.go b/cni/ipam/ipam.go index 785e05616e..85d76e9409 100644 --- a/cni/ipam/ipam.go +++ b/cni/ipam/ipam.go @@ -149,15 +149,17 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { return err } + // Select the requested interface. + // assign the container id + options := make(map[string]string) + options[ipam.OptInterfaceName] = nwCfg.Master + options[ipam.OptAddressID] = args.ContainerID + // Check if an address pool is specified. if nwCfg.Ipam.Subnet == "" { var poolID string var subnet string - // Select the requested interface. - options := make(map[string]string) - options[ipam.OptInterfaceName] = nwCfg.Master - isIpv6 := false if nwCfg.Ipam.Type == ipamV6 { isIpv6 = true @@ -183,7 +185,7 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { } // Allocate an address for the endpoint. - address, err := plugin.am.RequestAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, nil) + address, err := plugin.am.RequestAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, options) if err != nil { err = plugin.Errorf("Failed to allocate address: %v", err) return err @@ -193,7 +195,7 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { defer func() { if err != nil && address != "" { log.Printf("[cni-ipam] Releasing address %v.", address) - plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, address, nil) + plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, address, options) } }() @@ -280,23 +282,20 @@ func (plugin *ipamPlugin) Delete(args *cniSkel.CmdArgs) error { return err } - // If an address is specified, release that address. Otherwise, release the pool. - if nwCfg.Ipam.Address != "" { - // Release the address. - err := plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, nil) - if err != nil { - err = plugin.Errorf("Failed to release address: %v", err) - return err - } - } else { - // Release the pool. - err := plugin.am.ReleasePool(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) - if err != nil { - err = plugin.Errorf("Failed to release pool: %v", err) - return err - } + // Select the requested interface. + options := make(map[string]string) + options[ipam.OptAddressID] = args.ContainerID + + err = plugin.am.ReleaseAddress(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet, nwCfg.Ipam.Address, options) + + if err != nil { + err = plugin.Errorf("Failed to release address: %v", err) + return err } + // Release the pool. + plugin.am.ReleasePool(nwCfg.Ipam.AddrSpace, nwCfg.Ipam.Subnet) + return nil } diff --git a/cni/ipam/ipam_test.go b/cni/ipam/ipam_test.go index 600372628b..8c76e38ef0 100644 --- a/cni/ipam/ipam_test.go +++ b/cni/ipam/ipam_test.go @@ -11,13 +11,13 @@ import ( "github.com/google/uuid" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" + "net" "net/http" "net/url" "testing" "time" "github.com/Azure/azure-container-networking/common" - "github.com/Azure/azure-container-networking/platform" ) var ipamQueryUrl = "localhost:42424" @@ -28,6 +28,7 @@ var ipamQueryResponse = "" + " " + " " + " " + + " " + " " + " " + "" @@ -50,17 +51,16 @@ func parseResult(stdinData []byte) (*cniTypesCurr.Result, error) { return result, nil } -func getStdinData(cniversion, subnet, ipAddress, endPointId string) []byte { +func getStdinData(cniversion, subnet, ipAddress string) []byte { stdinData := fmt.Sprintf( `{ "cniversion": "%s", "ipam": { "type": "internal", "subnet": "%s", - "ipAddress": "%s", - "EndpointID": "%s" + "ipAddress": "%s" } - }`, cniversion, subnet, ipAddress, endPointId) + }`, cniversion, subnet, ipAddress) return []byte(stdinData) } @@ -72,6 +72,14 @@ var ( err error endpointID1 = uuid.New().String() + //this usedAddresses map is to test not duplicate IP's + // have been provided throughout this test execution + UsedAddresses = map[string]string{} + + //below is the network,used to test if the IP's provided by IPAM + // is in the the space requested. + network = net.IPNet{IP: net.ParseIP("10.0.0.0"), Mask: net.CIDRMask(16, 32)} + _ = BeforeSuite(func() { // TODO: Ensure that the other testAgent has bees released. time.Sleep(1 * time.Second) @@ -123,29 +131,40 @@ var ( Context("When ADD with nothing, call for ipam triggering request pool and address", func() { It("Request pool and ADD successfully", func() { - arg.StdinData = getStdinData("0.4.0", "", "", endpointID1) + arg.StdinData = getStdinData("0.4.0", "", "") err = plugin.Add(arg) Expect(err).ShouldNot(HaveOccurred()) result, err = parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - address1, _ := platform.ConvertStringToIPNet("10.0.0.5/16") - address2, _ := platform.ConvertStringToIPNet("10.0.0.6/16") - Expect(result.IPs[0].Address.IP).Should(Or(Equal(address1.IP), Equal(address2.IP))) - Expect(result.IPs[0].Address.Mask).Should(Equal(address1.Mask)) + + //confirm if IP is in use by other invocation + _, exists := UsedAddresses[result.IPs[0].Address.IP.String()] + + Expect(exists).Should(BeFalse()) + + // set the IP as in use + UsedAddresses[result.IPs[0].Address.IP.String()] = result.IPs[0].Address.IP.String() + + //validate the IP is part of this network IP space + Expect(network.Contains(result.IPs[0].Address.IP)).Should(Equal(true)) + Expect(result.IPs[0].Address.Mask).Should(Equal(network.Mask)) }) }) Context("When DELETE with subnet and address, call for ipam triggering release address", func() { It("DELETE address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", result.IPs[0].Address.IP.String(), endpointID1) + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", result.IPs[0].Address.IP.String()) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) + + delete(UsedAddresses, result.IPs[0].Address.IP.String()) + }) }) Context("When DELETE with subnet, call for ipam triggering releasing pool", func() { It("DELETE pool successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "", endpointID1) + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) }) @@ -156,62 +175,139 @@ var ( Context("When address is given", func() { It("Request pool and address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "", "10.0.0.6", "") + arg.StdinData = getStdinData("0.4.0", "", "10.0.0.6") err = plugin.Add(arg) Expect(err).ShouldNot(HaveOccurred()) result, err := parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - address, _ := platform.ConvertStringToIPNet("10.0.0.6/16") - Expect(result.IPs[0].Address.IP).Should(Equal(address.IP)) - Expect(result.IPs[0].Address.Mask).Should(Equal(address.Mask)) + + //confirm if IP is in use by other invocation + _, exists := UsedAddresses[result.IPs[0].Address.IP.String()] + + Expect(exists).Should(BeFalse()) + + // set the IP as in use + UsedAddresses[result.IPs[0].Address.IP.String()] = result.IPs[0].Address.IP.String() + + //validate the IP is part of this network IP space + Expect(network.Contains(result.IPs[0].Address.IP)).Should(Equal(true)) + Expect(result.IPs[0].Address.Mask).Should(Equal(network.Mask)) }) }) Context("When subnet is given", func() { It("Request a usable address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "", endpointID1) + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") err = plugin.Add(arg) Expect(err).ShouldNot(HaveOccurred()) result, err := parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - address, _ := platform.ConvertStringToIPNet("10.0.0.5/16") - Expect(result.IPs[0].Address.IP).Should(Equal(address.IP)) - Expect(result.IPs[0].Address.Mask).Should(Equal(address.Mask)) + + //confirm if IP is in use by other invocation + _, exists := UsedAddresses[result.IPs[0].Address.IP.String()] + + Expect(exists).Should(BeFalse()) + + // set the IP as in use + UsedAddresses[result.IPs[0].Address.IP.String()] = result.IPs[0].Address.IP.String() + + //validate the IP is part of this network IP space + Expect(network.Contains(result.IPs[0].Address.IP)).Should(Equal(true)) + Expect(result.IPs[0].Address.Mask).Should(Equal(network.Mask)) + }) + }) + + Context("When container id is given with subnet", func() { + It("Request a usable address successfully", func() { + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") + arg.ContainerID = endpointID1 + err = plugin.Add(arg) + + Expect(err).ShouldNot(HaveOccurred()) + result, err := parseResult(arg.StdinData) + Expect(err).ShouldNot(HaveOccurred()) + + //confirm if IP is in use by other invocation + _, exists := UsedAddresses[result.IPs[0].Address.IP.String()] + + Expect(exists).Should(BeFalse()) + + // set the IP as in use and the container ID is using it + UsedAddresses[result.IPs[0].Address.IP.String()] = result.IPs[0].Address.IP.String() + UsedAddresses[arg.ContainerID] = result.IPs[0].Address.IP.String() + + //validate the IP is part of this network IP space + Expect(network.Contains(result.IPs[0].Address.IP)).Should(Equal(true)) + Expect(result.IPs[0].Address.Mask).Should(Equal(network.Mask)) + + //release the container ID for not test + arg.ContainerID = "" }) }) }) Describe("Test IPAM DELETE", func() { + Context("Delete when only container id is given", func() { + It("Deleted", func() { + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") + arg.ContainerID = endpointID1 + + err = plugin.Delete(arg) + + addressKey := UsedAddresses[arg.ContainerID] + + // set IP and container ID as not in use + delete(UsedAddresses, addressKey) + delete(UsedAddresses, arg.ContainerID) + + Expect(err).ShouldNot(HaveOccurred()) + arg.ContainerID = "" + }) + }) + Context("When address and subnet is given", func() { It("Release address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.5", endpointID1) + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.5") err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) + + // set IP as not in use + delete(UsedAddresses, "10.0.0.5") + }) + }) + + Context("When pool is in use", func() { + It("Fail to request pool", func() { + arg.StdinData = getStdinData("0.4.0", "", "") + err = plugin.Add(arg) + Expect(err).Should(HaveOccurred()) }) }) Context("When address and subnet is given", func() { It("Release address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.6", endpointID1) + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.6") err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) + // set IP as not in use + delete(UsedAddresses, "10.0.0.6") }) }) Context("When subnet is given", func() { It("Release pool successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "", endpointID1) + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "") err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) }) }) - Context("When subnet is given and no Id", func() { - It("Release pool successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "", "") - err = plugin.Delete(arg) + Context("When pool is not use", func() { + It("Confirm pool was released by succesfully requesting pool", func() { + arg.StdinData = getStdinData("0.4.0", "", "") + err = plugin.Add(arg) Expect(err).ShouldNot(HaveOccurred()) }) }) diff --git a/cni/netconfig.go b/cni/netconfig.go index 6a9e57e292..3faeef8451 100644 --- a/cni/netconfig.go +++ b/cni/netconfig.go @@ -69,7 +69,6 @@ type NetworkConfig struct { Subnet string `json:"subnet,omitempty"` Address string `json:"ipAddress,omitempty"` QueryInterval string `json:"queryInterval,omitempty"` - EndpointID string } DNS cniTypes.DNS `json:"dns"` RuntimeConfig RuntimeConfig `json:"runtimeConfig"` diff --git a/cni/network/network.go b/cni/network/network.go index 49ca0302bc..dbf2691dd2 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -462,9 +462,6 @@ func (plugin *netPlugin) Add(args *cniSkel.CmdArgs) error { endpointId := GetEndpointID(args) policies := cni.GetPoliciesFromNwCfg(nwCfg.AdditionalArgs) - // add endpoint ID to leverage in failure cases - nwCfg.Ipam.EndpointID = endpointId - // Check whether the network already exists. nwInfo, nwInfoErr := plugin.nm.GetNetworkInfo(networkId) if nwInfoErr == nil { @@ -837,8 +834,6 @@ func (plugin *netPlugin) Delete(args *cniSkel.CmdArgs) error { } endpointId := GetEndpointID(args) - // add endpoint ID to leverage in failure cases - nwCfg.Ipam.EndpointID = endpointId // Query the network. if nwInfo, err = plugin.nm.GetNetworkInfo(networkId); err != nil { diff --git a/ipam/pool.go b/ipam/pool.go index 0d776c2a13..fac6aec514 100644 --- a/ipam/pool.go +++ b/ipam/pool.go @@ -521,11 +521,10 @@ func (ap *addressPool) requestAddress(address string, options map[string]string) if id != "" { ap.addrsByID[id] = ar ar.ID = id - ar.InUse = true - } else { - ar.InUse = true } + ar.InUse = true + // Return address in CIDR notation. addr = &net.IPNet{ IP: ar.Addr, @@ -576,6 +575,7 @@ func (ap *addressPool) releaseAddress(address string, options map[string]string) } ar.InUse = false + ar.ID = "" if id != "" && ar.ID == id { delete(ap.addrsByID, ar.ID) From 14dbf14c041def88089bf66cd4613da4e51f0acb Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Mon, 17 Aug 2020 09:30:07 -0700 Subject: [PATCH 08/14] removing test file I added --- ipam/manager_old_test.go | 283 --------------------------------------- 1 file changed, 283 deletions(-) delete mode 100644 ipam/manager_old_test.go diff --git a/ipam/manager_old_test.go b/ipam/manager_old_test.go deleted file mode 100644 index d33098a837..0000000000 --- a/ipam/manager_old_test.go +++ /dev/null @@ -1,283 +0,0 @@ -// Copyright 2017 Microsoft. All rights reserved. -// MIT License - -package ipam - -import ( - "github.com/google/uuid" - "net" - "testing" -) - -// -// Address manager tests. -// - -// Tests address spaces are created and queried correctly. -func TestAddressSpaceCreateAndGet(t *testing.T) { - // Start with the test address space. - var options map[string]interface{} - - am, err := createAddressManager(options) - if err != nil { - t.Fatalf("createAddressManager failed, err:%+v.", err) - } - - // Test if the address spaces are returned correctly. - local, global := am.GetDefaultAddressSpaces() - - if local != LocalDefaultAddressSpaceId { - t.Errorf("GetDefaultAddressSpaces returned invalid local address space.") - } - - if global != GlobalDefaultAddressSpaceId { - t.Errorf("GetDefaultAddressSpaces returned invalid global address space.") - } -} - -// Tests updating an existing address space adds new resources and removes stale ones. -func TestAddressSpaceUpdate(t *testing.T) { - // Start with the test address space. - var options map[string]interface{} - - am, err := createAddressManager(options) - if err != nil { - t.Fatalf("createAddressManager failed, err:%+v.", err) - } - amImpl := am.(*addressManager) - - // Create a new local address space to update the existing one. - localAs, err := amImpl.newAddressSpace(LocalDefaultAddressSpaceId, LocalScope) - if err != nil { - t.Errorf("newAddressSpace failed, err:%+v.", err) - } - - // Remove addr12 and add addr13 in subnet1. - ap, err := localAs.newAddressPool(anyInterface, anyPriority, &subnet1) - ap.newAddressRecord(&addr11) - ap.newAddressRecord(&addr13) - - // Remove subnet2. - // Add subnet3 with addr31. - ap, err = localAs.newAddressPool(anyInterface, anyPriority, &subnet3) - ap.newAddressRecord(&addr31) - - err = amImpl.setAddressSpace(localAs) - if err != nil { - t.Errorf("setAddressSpace failed, err:%+v.", err) - } - - // Test that the address space was updated correctly. - localAs, err = amImpl.getAddressSpace(LocalDefaultAddressSpaceId) - if err != nil { - t.Errorf("getAddressSpace failed, err:%+v.", err) - } - - // Subnet1 should have addr11 and addr13, but not addr12. - ap, err = localAs.getAddressPool(subnet1.String()) - if err != nil { - t.Errorf("Cannot find subnet1, err:%+v.", err) - } - - _, err = ap.requestAddress(addr11.String(), nil) - if err != nil { - t.Errorf("Cannot find addr11, err:%+v.", err) - } - - _, err = ap.requestAddress(addr12.String(), nil) - if err == nil { - t.Errorf("Found addr12.") - } - - _, err = ap.requestAddress(addr13.String(), nil) - if err != nil { - t.Errorf("Cannot find addr13, err:%+v.", err) - } - - // Subnet2 should not exist. - ap, err = localAs.getAddressPool(subnet2.String()) - if err == nil { - t.Errorf("Found subnet2.") - } - - // Subnet3 should have addr31 only. - ap, err = localAs.getAddressPool(subnet3.String()) - if err != nil { - t.Errorf("Cannot find subnet3, err:%+v.", err) - } - - _, err = ap.requestAddress(addr31.String(), nil) - if err != nil { - t.Errorf("Cannot find addr31, err:%+v.", err) - } - - _, err = ap.requestAddress(addr32.String(), nil) - if err == nil { - t.Errorf("Found addr32.") - } -} - -// Tests multiple wildcard address pool requests return separate pools. -func TestAddressPoolRequestsForSeparatePools(t *testing.T) { - // Start with the test address space. - var options map[string]interface{} - am, err := createAddressManager(options) - if err != nil { - t.Fatalf("createAddressManager failed, err:%+v.", err) - } - - // Request two separate address pools. - poolId1, subnet1, err := am.RequestPool(LocalDefaultAddressSpaceId, "", "", nil, false) - if err != nil { - t.Errorf("RequestPool failed, err:%v", err) - } - - poolId2, subnet2, err := am.RequestPool(LocalDefaultAddressSpaceId, "", "", nil, false) - if err != nil { - t.Errorf("RequestPool failed, err:%v", err) - } - - // Test the poolIds and subnets do not match. - if poolId1 == poolId2 || subnet1 == subnet2 { - t.Errorf("Pool requests returned the same pool.") - } - - // Release the address pools. - err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId1) - if err != nil { - t.Errorf("ReleasePool failed, err:%v", err) - } - - err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId2) - if err != nil { - t.Errorf("ReleasePool failed, err:%v", err) - } -} - -// Tests multiple identical address pool requests return the same pool and pools are referenced correctly. -func TestAddressPoolRequestsForSamePool(t *testing.T) { - // Start with the test address space. - var options map[string]interface{} - - am, err := createAddressManager(options) - if err != nil { - t.Fatalf("createAddressManager failed, err:%+v.", err) - } - - // Request the same address pool twice. - poolId1, subnet1, err := am.RequestPool(LocalDefaultAddressSpaceId, "", "", nil, false) - if err != nil { - t.Errorf("RequestPool failed, err:%v", err) - } - - poolId2, subnet2, err := am.RequestPool(LocalDefaultAddressSpaceId, poolId1, "", nil, false) - if err != nil { - t.Errorf("RequestPool failed, err:%v", err) - } - - // Test the subnets do not match. - if poolId1 != poolId2 || subnet1 != subnet2 { - t.Errorf("Pool requests returned different pools.") - } - - // Release the address pools. - err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId1) - if err != nil { - t.Errorf("ReleasePool failed, err:%v", err) - } - - err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId2) - if err != nil { - t.Errorf("ReleasePool failed, err:%v", err) - } - - // Third release should fail. - err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId1) - if err == nil { - t.Errorf("ReleasePool succeeded extra, err:%v", err) - } -} - -// Tests address requests from the same pool return separate addresses and releases work correctly. -func TestAddressRequestsFromTheSamePool(t *testing.T) { - // Start with the test address space. - var options map[string]interface{} - - am, err := createAddressManager(options) - if err != nil { - t.Fatalf("createAddressManager failed, err:%+v.", err) - } - - // Request a pool. - poolId, _, err := am.RequestPool(LocalDefaultAddressSpaceId, "", "", nil, false) - if err != nil { - t.Errorf("RequestPool failed, err:%v", err) - } - - options1 := make(map[string]string) - options1[OptAddressID] = uuid.New().String() - - options2 := make(map[string]string) - options2[OptAddressID] = uuid.New().String() - - options3 := make(map[string]string) - options3[OptAddressID] = uuid.New().String() - - // Request two addresses from the pool. - address1, err := am.RequestAddress(LocalDefaultAddressSpaceId, poolId, "", options1) - if err != nil { - t.Errorf("RequestAddress failed, err:%v", err) - } - - addr, _, _ := net.ParseCIDR(address1) - address1 = addr.String() - - address2, err := am.RequestAddress(LocalDefaultAddressSpaceId, poolId, "", options2) - if err != nil { - t.Errorf("RequestAddress failed, err:%v", err) - } - - addr, _, _ = net.ParseCIDR(address2) - address2 = addr.String() - - // Request four addresses from the pool. - address3, err := am.RequestAddress(LocalDefaultAddressSpaceId, poolId, "", options3) - if err != nil { - t.Errorf("RequestAddress failed, err:%v", err) - } - - addr, _, _ = net.ParseCIDR(address3) - address3 = addr.String() - - var m map[string]string - _, exists := m[address1] - _, exists = m[address2] - _, exists = m[address3] - - // Test the addresses do not match. - if exists { - t.Errorf("Address requests returned the same address %v.", address1) - } - - // Release addresses and the pool. - err = am.ReleaseAddress(LocalDefaultAddressSpaceId, poolId, address1, options1) - if err != nil { - t.Errorf("ReleaseAddress failed, err:%v", err) - } - - err = am.ReleaseAddress(LocalDefaultAddressSpaceId, poolId, address2, options2) - if err != nil { - t.Errorf("ReleaseAddress failed, err:%v", err) - } - - // Release addresses and the pool. - err = am.ReleaseAddress(LocalDefaultAddressSpaceId, poolId, "", options3) - if err != nil { - t.Errorf("ReleaseAddress failed, err:%v", err) - } - - err = am.ReleasePool(LocalDefaultAddressSpaceId, poolId) - if err != nil { - t.Errorf("ReleasePool failed, err:%v", err) - } -} From 2de8c2c6c8004838ac86b792c90479d99d674ee4 Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Mon, 17 Aug 2020 10:19:30 -0700 Subject: [PATCH 09/14] remove a typo --- ipam/pool.go | 1 - 1 file changed, 1 deletion(-) diff --git a/ipam/pool.go b/ipam/pool.go index fac6aec514..378aa648a0 100644 --- a/ipam/pool.go +++ b/ipam/pool.go @@ -575,7 +575,6 @@ func (ap *addressPool) releaseAddress(address string, options map[string]string) } ar.InUse = false - ar.ID = "" if id != "" && ar.ID == id { delete(ap.addrsByID, ar.ID) From b236797bc012ad9f7eedad1ffebf726d42d3c3b1 Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Tue, 18 Aug 2020 09:04:19 -0700 Subject: [PATCH 10/14] remove couple things --- ipam/api.go | 1 - ipam/pool.go | 6 +++--- ipam/pool_test.go | 2 +- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/ipam/api.go b/ipam/api.go index c6670e399c..dc3be1d725 100644 --- a/ipam/api.go +++ b/ipam/api.go @@ -16,7 +16,6 @@ var ( errInvalidConfiguration = fmt.Errorf("Invalid configuration") errAddressPoolExists = fmt.Errorf("Address pool already exists") errAddressPoolNotFound = fmt.Errorf("Address pool not found") - ErrAddressPoolNotFound = fmt.Errorf("Address pool not found") errAddressPoolInUse = fmt.Errorf("Address pool already in use") errAddressPoolNotInUse = fmt.Errorf("Address pool not in use") errNoAvailableAddressPools = fmt.Errorf("No available address pools") diff --git a/ipam/pool.go b/ipam/pool.go index 378aa648a0..8e88098537 100644 --- a/ipam/pool.go +++ b/ipam/pool.go @@ -283,7 +283,7 @@ func (as *addressSpace) newAddressPool(ifName string, priority int, subnet *net. func (as *addressSpace) getAddressPool(poolId string) (*addressPool, error) { ap := as.Pools[poolId] if ap == nil { - return nil, ErrAddressPoolNotFound + return nil, errInvalidPoolId } return ap, nil @@ -507,7 +507,7 @@ func (ap *addressPool) requestAddress(address string, options map[string]string) // If no address was found, return any available address. if ar == nil { for _, ar = range ap.Addresses { - if !ar.InUse { + if !ar.InUse && ar.ID == "" { break } ar = nil @@ -569,7 +569,7 @@ func (ap *addressPool) releaseAddress(address string, options map[string]string) return nil } - if !ar.InUse && ar.ID == "" { + if !ar.InUse { log.Printf("Address not in use. Not Returning error") return nil } diff --git a/ipam/pool_test.go b/ipam/pool_test.go index c109576ded..99b89f8c38 100644 --- a/ipam/pool_test.go +++ b/ipam/pool_test.go @@ -510,7 +510,7 @@ var ( Pools: map[string]*addressPool{}, } pool, err := as.getAddressPool("10.0.0.0/16") - Expect(err).To(Equal(errAddressPoolNotFound)) + Expect(err).To(Equal(errInvalidPoolId)) Expect(pool).To(BeNil()) }) }) From 0b8fd291543d223051a5abba88b9464c7696ad56 Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Thu, 20 Aug 2020 12:07:52 -0700 Subject: [PATCH 11/14] clean up test file --- cni/ipam/ipam_test.go | 114 ++++++++++++++++++++++++------------------ 1 file changed, 64 insertions(+), 50 deletions(-) diff --git a/cni/ipam/ipam_test.go b/cni/ipam/ipam_test.go index 8c76e38ef0..d69e39d8ae 100644 --- a/cni/ipam/ipam_test.go +++ b/cni/ipam/ipam_test.go @@ -137,17 +137,11 @@ var ( result, err = parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - //confirm if IP is in use by other invocation - _, exists := UsedAddresses[result.IPs[0].Address.IP.String()] + AssertAddressNotInUse(result.IPs[0].Address.IP.String()) - Expect(exists).Should(BeFalse()) + TrackAddressUsage(result.IPs[0].Address.IP.String(), "") - // set the IP as in use - UsedAddresses[result.IPs[0].Address.IP.String()] = result.IPs[0].Address.IP.String() - - //validate the IP is part of this network IP space - Expect(network.Contains(result.IPs[0].Address.IP)).Should(Equal(true)) - Expect(result.IPs[0].Address.Mask).Should(Equal(network.Mask)) + AssertProperAddressSpace(result.IPs[0].Address) }) }) @@ -181,17 +175,11 @@ var ( result, err := parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - //confirm if IP is in use by other invocation - _, exists := UsedAddresses[result.IPs[0].Address.IP.String()] - - Expect(exists).Should(BeFalse()) + AssertAddressNotInUse(result.IPs[0].Address.IP.String()) - // set the IP as in use - UsedAddresses[result.IPs[0].Address.IP.String()] = result.IPs[0].Address.IP.String() + TrackAddressUsage(result.IPs[0].Address.IP.String(), "") - //validate the IP is part of this network IP space - Expect(network.Contains(result.IPs[0].Address.IP)).Should(Equal(true)) - Expect(result.IPs[0].Address.Mask).Should(Equal(network.Mask)) + AssertProperAddressSpace(result.IPs[0].Address) }) }) @@ -204,17 +192,11 @@ var ( result, err := parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - //confirm if IP is in use by other invocation - _, exists := UsedAddresses[result.IPs[0].Address.IP.String()] - - Expect(exists).Should(BeFalse()) + AssertAddressNotInUse(result.IPs[0].Address.IP.String()) - // set the IP as in use - UsedAddresses[result.IPs[0].Address.IP.String()] = result.IPs[0].Address.IP.String() + TrackAddressUsage(result.IPs[0].Address.IP.String(), "") - //validate the IP is part of this network IP space - Expect(network.Contains(result.IPs[0].Address.IP)).Should(Equal(true)) - Expect(result.IPs[0].Address.Mask).Should(Equal(network.Mask)) + AssertProperAddressSpace(result.IPs[0].Address) }) }) @@ -228,20 +210,13 @@ var ( result, err := parseResult(arg.StdinData) Expect(err).ShouldNot(HaveOccurred()) - //confirm if IP is in use by other invocation - _, exists := UsedAddresses[result.IPs[0].Address.IP.String()] + AssertAddressNotInUse(result.IPs[0].Address.IP.String()) - Expect(exists).Should(BeFalse()) + TrackAddressUsage(result.IPs[0].Address.IP.String(), arg.ContainerID) - // set the IP as in use and the container ID is using it - UsedAddresses[result.IPs[0].Address.IP.String()] = result.IPs[0].Address.IP.String() - UsedAddresses[arg.ContainerID] = result.IPs[0].Address.IP.String() + AssertProperAddressSpace(result.IPs[0].Address) - //validate the IP is part of this network IP space - Expect(network.Contains(result.IPs[0].Address.IP)).Should(Equal(true)) - Expect(result.IPs[0].Address.Mask).Should(Equal(network.Mask)) - - //release the container ID for not test + //release the container ID for next test arg.ContainerID = "" }) }) @@ -255,26 +230,24 @@ var ( arg.ContainerID = endpointID1 err = plugin.Delete(arg) + Expect(err).ShouldNot(HaveOccurred()) - addressKey := UsedAddresses[arg.ContainerID] + address := UsedAddresses[arg.ContainerID] - // set IP and container ID as not in use - delete(UsedAddresses, addressKey) - delete(UsedAddresses, arg.ContainerID) + RemoveAddressUsage(address, arg.ContainerID) - Expect(err).ShouldNot(HaveOccurred()) - arg.ContainerID = "" }) }) Context("When address and subnet is given", func() { It("Release address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.5") + + nextAddress := GetFirstAddress() + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", nextAddress) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) - // set IP as not in use - delete(UsedAddresses, "10.0.0.5") + RemoveAddressUsage(nextAddress, "") }) }) @@ -288,11 +261,12 @@ var ( Context("When address and subnet is given", func() { It("Release address successfully", func() { - arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", "10.0.0.6") + nextAddress := GetFirstAddress() + arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", nextAddress) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) - // set IP as not in use - delete(UsedAddresses, "10.0.0.6") + + RemoveAddressUsage(nextAddress, "") }) }) @@ -314,3 +288,43 @@ var ( }) }) ) + +func GetFirstAddress() string { + //return first value + for a := range UsedAddresses { + return a + } + return "" +} + +func AssertAddressNotInUse(address string) { + //confirm if IP is in use by other invocation + _, exists := UsedAddresses[address] + + Expect(exists).Should(BeFalse()) +} + +func TrackAddressUsage(address, containerId string) { + // set the IP as in use + // this is just for tracking in this test + UsedAddresses[address] = address + + if containerId != "" { + // set the container as in use + UsedAddresses[containerId] = address + } +} + +func RemoveAddressUsage(address, containerId string) { + delete(UsedAddresses, address) + if containerId != "" { + delete(UsedAddresses, arg.ContainerID) + arg.ContainerID = "" + } +} + +func AssertProperAddressSpace(address net.IPNet) { + //validate the IP is part of this network IP space + Expect(network.Contains(address.IP)).Should(Equal(true)) + Expect(address.Mask).Should(Equal(network.Mask)) +} From 9049eaa10ba68e4fbf1124366ee322d8b1629978 Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Mon, 31 Aug 2020 10:18:25 -0700 Subject: [PATCH 12/14] back from vacation, addressing some comments --- cni/ipam/ipam.go | 5 +++-- ipam/manager.go | 1 + ipam/pool.go | 15 ++++++++------- 3 files changed, 12 insertions(+), 9 deletions(-) diff --git a/cni/ipam/ipam.go b/cni/ipam/ipam.go index 85d76e9409..2590e8f6bb 100644 --- a/cni/ipam/ipam.go +++ b/cni/ipam/ipam.go @@ -149,10 +149,8 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { return err } - // Select the requested interface. // assign the container id options := make(map[string]string) - options[ipam.OptInterfaceName] = nwCfg.Master options[ipam.OptAddressID] = args.ContainerID // Check if an address pool is specified. @@ -165,6 +163,9 @@ func (plugin *ipamPlugin) Add(args *cniSkel.CmdArgs) error { isIpv6 = true } + // Select the requested interface. + options[ipam.OptInterfaceName] = nwCfg.Master + // Allocate an address pool. poolID, subnet, err = plugin.am.RequestPool(nwCfg.Ipam.AddrSpace, "", "", options, isIpv6) if err != nil { diff --git a/ipam/manager.go b/ipam/manager.go index 06c697699d..157e496d7a 100644 --- a/ipam/manager.go +++ b/ipam/manager.go @@ -170,6 +170,7 @@ func (am *addressManager) save() error { // Update time stamp. am.TimeStamp = time.Now() + log.Printf("[ipam] saving ipam state.\n") err := am.store.Write(storeKey, am) if err == nil { log.Printf("[ipam] Save succeeded.\n") diff --git a/ipam/pool.go b/ipam/pool.go index 8e88098537..31edf7c324 100644 --- a/ipam/pool.go +++ b/ipam/pool.go @@ -159,8 +159,9 @@ func (am *addressManager) setAddressSpace(as *addressSpace) error { if !ok { am.AddrSpaces[as.Id] = as } else { - //merges the address set refreshed from the source - //and the ones we have currently this address space + // merges the address set refreshed from the source + // and the ones we have currently in this address space + log.Printf("[ipam] merging address space") as1.merge(as) } @@ -179,7 +180,7 @@ func (am *addressManager) setAddressSpace(as *addressSpace) error { // Merges a new address space to an existing one. func (as *addressSpace) merge(newas *addressSpace) { // The new epoch after the merge. - //epoch is essentially the count of invocations + // epoch is essentially the count of invocations // used to ensure if certain addresses refreshed from the source // are still relevant as.epoch++ @@ -373,13 +374,13 @@ func (as *addressSpace) releasePool(poolId string) error { var err error var addressesInUse bool - log.Printf("[ipam] Attempting to releasing pool with poolId:%v.", poolId) + //merges the address set refreshed from the source log.Printf("[ipam] Attempting to release pool with poolId:%v.", poolId) ap, ok := as.Pools[poolId] if !ok { err = errAddressPoolNotFound } else if addressesInUse = ap.IsAnyRecordInUse(); addressesInUse { - log.Printf("[ipam] Skip releasing pool with poolId:%v. due to address being in use", + log.Printf("[ipam] Skip releasing pool with poolId:%s. due to address being in use", poolId) } @@ -389,11 +390,11 @@ func (as *addressSpace) releasePool(poolId string) error { } if !addressesInUse { - ap.RefCount-- + ap.RefCount = 0 // Delete address pool if it is no longer available. if !ap.isInUse() { - log.Printf("[ipam] Deleting stale pool with poolId:%v.", poolId) + log.Printf("[ipam] Deleting stale pool with poolId:%s.", poolId) delete(as.Pools, poolId) } } From 7006932d52d77a1ff94906c335d651409d0412e1 Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Mon, 31 Aug 2020 10:34:06 -0700 Subject: [PATCH 13/14] removing setting pool count to zero --- ipam/pool.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipam/pool.go b/ipam/pool.go index 31edf7c324..45e664a585 100644 --- a/ipam/pool.go +++ b/ipam/pool.go @@ -390,7 +390,7 @@ func (as *addressSpace) releasePool(poolId string) error { } if !addressesInUse { - ap.RefCount = 0 + ap.RefCount-- // Delete address pool if it is no longer available. if !ap.isInUse() { From 90401b7a4dd39a520ffdacf3a3e7e99fa4e37b23 Mon Sep 17 00:00:00 2001 From: Ali Egal Date: Mon, 31 Aug 2020 13:18:27 -0700 Subject: [PATCH 14/14] addressing a couple other comments --- cni/ipam/ipam_test.go | 6 +++--- ipam/manager.go | 1 + 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/cni/ipam/ipam_test.go b/cni/ipam/ipam_test.go index d69e39d8ae..d480e31b1e 100644 --- a/cni/ipam/ipam_test.go +++ b/cni/ipam/ipam_test.go @@ -242,7 +242,7 @@ var ( Context("When address and subnet is given", func() { It("Release address successfully", func() { - nextAddress := GetFirstAddress() + nextAddress := GetNextAddress() arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", nextAddress) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) @@ -261,7 +261,7 @@ var ( Context("When address and subnet is given", func() { It("Release address successfully", func() { - nextAddress := GetFirstAddress() + nextAddress := GetNextAddress() arg.StdinData = getStdinData("0.4.0", "10.0.0.0/16", nextAddress) err = plugin.Delete(arg) Expect(err).ShouldNot(HaveOccurred()) @@ -289,7 +289,7 @@ var ( }) ) -func GetFirstAddress() string { +func GetNextAddress() string { //return first value for a := range UsedAddresses { return a diff --git a/ipam/manager.go b/ipam/manager.go index 157e496d7a..af99fbb242 100644 --- a/ipam/manager.go +++ b/ipam/manager.go @@ -164,6 +164,7 @@ func (am *addressManager) restore() error { func (am *addressManager) save() error { // Skip if a store is not provided. if am.store == nil { + log.Printf("[ipam] ipam store is nil.\n") return nil }