Added new UTs to thoroughlly condition-cover the new scenarios.

Found a bug: during CNI DEL the IP coming from the DanmEp is in CIDR format, and shall be decoded accordingly.
Error found by introducing reserve checks to cnidel UTs too, and also checking how many times DanmNet Update was called.
The latter check is introduced to ipam UTs as well, which made me realize we can optimize API write both during Reserve and Free.
We only need to push back changes if anything was changed at all.
Write need not be called for pur IPv6, and erroneous cases!

cmd/cnitest/cnitest.go

@@ -123,17 +123,17 @@ func validateMacvlanConfig(receivedCniConfig, expectedCniConfig []byte, tcConf T
   var recMacvlanConf cnidel.MacvlanNet
   err := json.Unmarshal(receivedCniConfig, &recMacvlanConf)
   if err != nil {
-    return errors.New("Received MACVLAN config could not be unmarshalled, because:" + err.Error())
+    return errors.New("Received CNI config could not be unmarshalled, because:" + err.Error())
   }
-  log.Printf("Received MACVLAN config:%v",recMacvlanConf)
+  log.Printf("Received CNI config:%v",recMacvlanConf)
   var expMacvlanConf MacvlanCniTestConfig
   err = json.Unmarshal(expectedCniConfig, &expMacvlanConf)
   if err != nil {
-    return errors.New("Expected MACVLAN config could not be unmarshalled, because:" + err.Error())
+    return errors.New("Expected CNI config could not be unmarshalled, because:" + err.Error())
   }
   if tcConf.CniExpectations.Ip6 != "" {
     if recMacvlanConf.Ipam.Ips == nil {
-      return errors.New("Received MACVLAN CNI config does not contain IPv6 address under ipam section, but it shall!")
+      return errors.New("Received CNI config does not contain IPv6 address under ipam section, but it shall!")
     }
     newIpamConfig := datastructs.IpamConfig{Type: "fakeipam"}
     for _,ip := range recMacvlanConf.Ipam.Ips {
@@ -142,11 +142,11 @@ func validateMacvlanConfig(receivedCniConfig, expectedCniConfig []byte, tcConf T
       }
     }
     recMacvlanConf.Ipam = newIpamConfig
-    log.Printf("Received MACVLAN config after IPv6 adjustment:%v",recMacvlanConf)
+    log.Printf("Received config after IPv6 adjustment:%v",recMacvlanConf)
   }
-  log.Printf("Expected MACVLAN config:%v",expMacvlanConf.CniConf)
+  log.Printf("Expected config:%v",expMacvlanConf.CniConf)
   if !reflect.DeepEqual(recMacvlanConf, expMacvlanConf.CniConf) {
-    return errors.New("Received MACVLAN delegate configuration does not match with expected!")
+    return errors.New("Received delegate configuration does not match with expected!")
   }
   return nil
 }

pkg/cnidel/cnidel.go

@@ -57,10 +57,7 @@ func DelegateInterfaceSetup(netConf *datastructs.NetConf, danmClient danmclients
    //Therefore, anyone wishing to further update the same network object later on will use an outdated representation as the input.
    //IPAM should be refactored to always pass back the up-to-date object.
    //I guess it is okay now because we only want to free IPs, and RV differences are resolved by the generated client code.
-    ipamOptions, err = getCniIpamConfig(netInfo, ep.Spec.Iface.Address, ep.Spec.Iface.AddressIPv6)
-    if err != nil {
-      return nil, errors.New("IPAM config creation failed for network:" + netInfo.ObjectMeta.Name + " with error:" + err.Error())
-    }
+    ipamOptions = getCniIpamConfig(netInfo, ep.Spec.Iface.Address, ep.Spec.Iface.AddressIPv6)
   }
   rawConfig, err := getCniPluginConfig(netConf, netInfo, ipamOptions, ep)
   if err != nil {
@@ -104,7 +101,7 @@ func IsDeviceNeeded(cniType string) bool {
   }
 }
 
-func getCniIpamConfig(netinfo *danmtypes.DanmNet, ip4, ip6 string) (datastructs.IpamConfig, error) {
+func getCniIpamConfig(netinfo *danmtypes.DanmNet, ip4, ip6 string) datastructs.IpamConfig {
   var ipSlice = []datastructs.IpamIp{}
   if ip4 != "" {
     ipSlice = append(ipSlice, datastructs.IpamIp{
@@ -118,10 +115,10 @@ func getCniIpamConfig(netinfo *danmtypes.DanmNet, ip4, ip6 string) (datastructs.
                                 Version: 6,
                               })
   }
-  return  datastructs.IpamConfig{
+  return  datastructs.IpamConfig {
             Type: ipamType,
             Ips: ipSlice,
-          }, nil
+          }
 }
 
 func getCniPluginConfig(netConf *datastructs.NetConf, netInfo *danmtypes.DanmNet, ipamOptions datastructs.IpamConfig, ep *danmtypes.DanmEp) ([]byte, error) {
@@ -217,6 +214,9 @@ func freeDelegatedIp(danmClient danmclientset.Interface, netInfo *danmtypes.Danm
   _, v4Subnet, _ := net.ParseCIDR(netInfo.Spec.Options.Cidr)
   _, v6Subnet, _ := net.ParseCIDR(netInfo.Spec.Options.Net6)
   parsedIp := net.ParseIP(ip)
+  if parsedIp == nil {
+    parsedIp,_,_ = net.ParseCIDR(ip)
+  }
   //We only need to Free an IP if it was allocated by DANM IPAM, and it was allocated by DANM only if it falls into any of the defined subnets
   if parsedIp != nil && ((v4Subnet != nil && v4Subnet.Contains(parsedIp)) || (v6Subnet != nil && v6Subnet.Contains(parsedIp))) {
     err := ipam.Free(danmClient, *netInfo, ip)

pkg/ipam/ipam.go

@@ -21,12 +21,17 @@ import (
 // The reserved IP address is represented by setting a bit in the network's BitArray type allocation matrix
 // The refreshed network object is modified in the K8s API server at the end
 func Reserve(danmClient danmclientset.Interface, netInfo danmtypes.DanmNet, req4, req6 string) (string, string, string, error) {
+  origAlloc := netInfo.Spec.Options.Alloc
   tempNetSpec := netInfo
   for {
     ip4, ip6, macAddr, err := allocateIP(&tempNetSpec, req4, req6)
     if err != nil {
       return "", "", "", errors.New("failed to allocate IP address for network:" + netInfo.ObjectMeta.Name + " with error:" + err.Error())
     }
+    //Right now we only store IPv4 allocations in the API. If this bitmask is unchanged, there is nothing to update in the API server
+    if tempNetSpec.Spec.Options.Alloc == origAlloc {
+      return ip4, ip6, macAddr, nil
+    }
     retryNeeded, err, newNetSpec := updateIpAllocation(danmClient, tempNetSpec)
     if err != nil {
       return "", "", "", err
@@ -47,9 +52,14 @@ func Free(danmClient danmclientset.Interface, netInfo danmtypes.DanmNet, ip stri
     // Nothing to return here: either network, or the interface is an L2
     return nil
   }
+  origAlloc := netInfo.Spec.Options.Alloc
   tempNetSpec := netInfo
   for {
     resetIP(&tempNetSpec, ip)
+    //Right now we only store IPv4 allocations in the API. If this bitmask is unchanged, there is nothing to update in the API server
+    if tempNetSpec.Spec.Options.Alloc == origAlloc {
+      return nil
+    }
     retryNeeded, err, newNetSpec := updateIpAllocation(danmClient, tempNetSpec)
     if err != nil {
       return err

test/stubs/danm/client_stub.go

@@ -25,10 +25,14 @@ type Reservation struct {
 
 type ClientStub struct {
   Objects TestArtifacts
+  NetClient *NetClientStub
 }
 
 func (client *ClientStub) DanmNets(namespace string) client.DanmNetInterface {
-  return newNetClientStub(client.Objects.TestNets, client.Objects.ReservedIps)
+  if client.NetClient == nil {
+    client.NetClient = newNetClientStub(client.Objects.TestNets, client.Objects.ReservedIps)
+  }
+  return client.NetClient 
 }
 
 func (client *ClientStub) DanmEps(namespace string) client.DanmEpInterface {

test/stubs/danm/clientset_stub.go

@@ -6,15 +6,15 @@ import (
 )
 
 type ClientSetStub struct {
-  danmClient *ClientStub
+  DanmClient *ClientStub
 }
 
 func (c *ClientSetStub) DanmV1() danmv1.DanmV1Interface {
-  return c.danmClient
+  return c.DanmClient
 }
 
 func (c *ClientSetStub) Danm() danmv1.DanmV1Interface {
-  return c.danmClient
+  return c.DanmClient
 }
 
 func (c *ClientSetStub) Discovery() discovery.DiscoveryInterface {
@@ -23,6 +23,6 @@ func (c *ClientSetStub) Discovery() discovery.DiscoveryInterface {
 
 func NewClientSetStub(objects TestArtifacts) *ClientSetStub {
   var clientSet ClientSetStub
-  clientSet.danmClient = newClientStub(objects)
+  clientSet.DanmClient = newClientStub(objects)
   return &clientSet
 }

test/stubs/danm/netclient_stub.go

@@ -17,20 +17,22 @@ const (
 )
 
 type NetClientStub struct{
-  testNets []danmtypes.DanmNet
-  reservedIpsList []ReservedIpsList
+  TestNets []danmtypes.DanmNet
+  ReservedIpsList []ReservedIpsList
+  TimesUpdateWasCalled int
 }
 
 func newNetClientStub(nets []danmtypes.DanmNet, ips []ReservedIpsList) *NetClientStub {
-  return &NetClientStub{testNets: nets, reservedIpsList: ips}
+  return &NetClientStub{TestNets: nets, ReservedIpsList: ips}
 }
 
 func (netClient *NetClientStub) Create(obj *danmtypes.DanmNet) (*danmtypes.DanmNet, error) {
   return nil, nil
 }
 
 func (netClient *NetClientStub) Update(obj *danmtypes.DanmNet) (*danmtypes.DanmNet, error) {
-  for _, netReservation := range netClient.reservedIpsList {
+  netClient.TimesUpdateWasCalled++
+  for _, netReservation := range netClient.ReservedIpsList {
     if obj.Spec.NetworkID == netReservation.NetworkId {
       ba := bitarray.NewBitArrayFromBase64(obj.Spec.Options.Alloc)
       _, ipnet, _ := net.ParseCIDR(obj.Spec.Options.Cidr)
@@ -45,18 +47,18 @@ func (netClient *NetClientStub) Update(obj *danmtypes.DanmNet) (*danmtypes.DanmN
           continue
         }
         if !ba.Get(uint32(ipInInt)) && reservation.Set {
-          return nil, errors.New("Reservation failure, IP:" + reservation.Ip + " must be reserved in DanmNet:" + obj.Spec.NetworkID)
+          return nil, errors.New("Reservation failure, IP:" + reservation.Ip + " should have been reserved in DanmNet:" + obj.Spec.NetworkID)
         }
         if ba.Get(uint32(ipInInt)) && !reservation.Set {
-          return nil, errors.New("Reservation failure, IP:" + reservation.Ip + " must be free in DanmNet:" + obj.Spec.NetworkID)
+          return nil, errors.New("Reservation failure, IP:" + reservation.Ip + " should have been free in DanmNet:" + obj.Spec.NetworkID)
         }
       }
     }
   }
   if strings.Contains(obj.Spec.NetworkID, "conflict") && obj.ObjectMeta.ResourceVersion != magicVersion {
-    for index, net := range netClient.testNets {
+    for index, net := range netClient.TestNets {
       if net.Spec.NetworkID == obj.Spec.NetworkID {
-        netClient.testNets[index].ObjectMeta.ResourceVersion = magicVersion
+        netClient.TestNets[index].ObjectMeta.ResourceVersion = magicVersion
       }
     }
     return nil, errors.New(datastructs.OptimisticLockErrorMsg)
@@ -79,7 +81,7 @@ func (netClient *NetClientStub) Get(netName string, options meta_v1.GetOptions)
   if strings.Contains(netName, "error") {
     return nil, errors.New("fatal error, don't retry")
   }
-  for _, testNet := range netClient.testNets {
+  for _, testNet := range netClient.TestNets {
     if testNet.Spec.NetworkID == netName {
       return &testNet, nil
     }
@@ -101,5 +103,5 @@ func (netClient *NetClientStub) Patch(name string, pt types.PatchType, data []by
 }
 
 func (netClient *NetClientStub) AddReservedIpsList(reservedIps []ReservedIpsList) {
-  netClient.reservedIpsList = reservedIps
+  netClient.ReservedIpsList = reservedIps
 }

test/utils/utils.go

@@ -7,6 +7,7 @@ import (
   "github.com/nokia/danm/pkg/bitarray"
   "github.com/nokia/danm/pkg/ipam"
   "github.com/nokia/danm/pkg/admit"
+  stubs "github.com/nokia/danm/test/stubs/danm"
 )
 
 const (
@@ -37,7 +38,7 @@ func SetupAllocationPools(nets []danmtypes.DanmNet) error {
       if dnet.Spec.Options.Pool.End == "" {
         dnet.Spec.Options.Pool.End = (ipam.Int2ip(ipam.Ip2int(admit.GetBroadcastAddress(ipnet)) - 1)).String()
       }
-      if strings.HasPrefix(dnet.Spec.NetworkID, "full") {
+      if strings.HasPrefix(dnet.ObjectMeta.Name, "full") {
         exhaustNetwork(&dnet)
       }
       nets[index].Spec = dnet.Spec
@@ -55,6 +56,16 @@ func GetTestNet(netId string, testNets []danmtypes.DanmNet) *danmtypes.DanmNet {
   return nil
 }
 
+func CreateExpectedAllocationsList(ip string, isExpectedToBeSet bool, networkId string) []stubs.ReservedIpsList {
+  var ips []stubs.ReservedIpsList
+  if ip != "" {
+    reservation := stubs.Reservation {Ip: ip, Set: isExpectedToBeSet,}
+    expectedAllocation := stubs.ReservedIpsList{NetworkId: networkId, Reservations: []stubs.Reservation {reservation,},}
+    ips = append(ips, expectedAllocation)
+  }
+  return ips
+}
+
 func exhaustNetwork(netInfo *danmtypes.DanmNet) {
     ba := bitarray.NewBitArrayFromBase64(netInfo.Spec.Options.Alloc)
     _, ipnet, _ := net.ParseCIDR(netInfo.Spec.Options.Cidr)