Various CNI delegation corrections related to framework bump, and central IPAM support

cmd/danm/danm.go

@@ -10,7 +10,7 @@ import (
 
 func main() {
   var err error
-  f, err := os.OpenFile("/var/log/plugin.log", os.O_RDWR | os.O_CREATE | os.O_APPEND, 0640)
+  f, err := os.OpenFile("/var/log/danm.log", os.O_RDWR | os.O_CREATE | os.O_APPEND, 0640)
   if err == nil {
     log.SetOutput(f)
     defer f.Close()

cmd/fakeipam/fakeipam.go

@@ -28,7 +28,7 @@ func reserveIp(args *skel.CmdArgs) error {
   cniRes,err := createCniResult(ipamConf)
   if err != nil {
     return err
-  }  
+  }
   return cniRes.Print()
 }
 

integration/cni_config/00-danm.conf

@@ -1,4 +1,5 @@
 {
+  "cniVersion": "0.3.1",
   "name": "meta_cni",
   "name_comment": "Mandatory parameter, but can be anything",
   "type": "danm",

pkg/cnidel/cnidel.go

@@ -16,6 +16,7 @@ import (
   "github.com/nokia/danm/pkg/danmep"
   "github.com/nokia/danm/pkg/datastructs"
   "github.com/nokia/danm/pkg/ipam"
+  "github.com/nokia/danm/pkg/netcontrol"
   danmtypes "github.com/nokia/danm/crd/apis/danm/v1"
   danmclientset "github.com/nokia/danm/crd/client/clientset/versioned"
 )
@@ -50,14 +51,14 @@ func DelegateInterfaceSetup(netConf *datastructs.NetConf, danmClient danmclients
     ipamOptions datastructs.IpamConfig
   )
   if isIpamNeeded(netInfo, ep) {
-   ep.Spec.Iface.Address, ep.Spec.Iface.AddressIPv6, _, err = ipam.Reserve(danmClient, *netInfo, ep.Spec.Iface.Address, ep.Spec.Iface.AddressIPv6)
+    ep.Spec.Iface.Address, ep.Spec.Iface.AddressIPv6, _, err = ipam.Reserve(danmClient, *netInfo, ep.Spec.Iface.Address, ep.Spec.Iface.AddressIPv6)
     if err != nil {
       return nil, errors.New("IP address reservation failed for network:" + netInfo.ObjectMeta.Name + " with error:" + err.Error())
     }
-   //TODO: as netInfo is only copied to IPAM above, the IP allocation is not refreshed in the original copy.
-   //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.
+    //As netInfo is only copied to IPAM above, the IP allocation is not refreshed in the original copy.
+    //Without re-reading the network body we risk leaking IPs if error happens later on within the same thread!
+    netInfo,err = netcontrol.GetNetworkFromEp(danmClient, *ep)
+    if err != nil {log.Println("lofasz:" + err.Error())}
     ipamOptions = getCniIpamConfig(netInfo, ep.Spec.Iface.Address, ep.Spec.Iface.AddressIPv6)
   }
   rawConfig, err := getCniPluginConfig(netConf, netInfo, ipamOptions, ep)
@@ -185,7 +186,15 @@ func setEpIfaceAddress(cniResult *current.Result, epIface *danmtypes.DanmEpIface
 // DelegateInterfaceDelete delegates Ks8 Pod network interface delete task to the input 3rd party CNI plugin
 // Returns an error if interface creation was unsuccessful, or if the 3rd party CNI config could not be loaded
 func DelegateInterfaceDelete(netConf *datastructs.NetConf, danmClient danmclientset.Interface, netInfo *danmtypes.DanmNet, ep *danmtypes.DanmEp) error {
-  rawConfig, err := getCniPluginConfig(netConf, netInfo, datastructs.IpamConfig{Type: ipamType}, ep)
+  var ip4, ip6 string
+  if wasIpAllocatedByDanm(ep.Spec.Iface.Address, netInfo.Spec.Options.Cidr) {
+    ip4 = ep.Spec.Iface.Address
+  }
+  if wasIpAllocatedByDanm(ep.Spec.Iface.AddressIPv6, netInfo.Spec.Options.Net6) {
+    ip6 = ep.Spec.Iface.AddressIPv6
+  }
+  ipamForDelete := getCniIpamConfig(netInfo, ip4, ip6)
+  rawConfig, err := getCniPluginConfig(netConf, netInfo, ipamForDelete, ep)
   if err != nil {
     FreeDelegatedIps(danmClient, netInfo, ep.Spec.Iface.Address, ep.Spec.Iface.AddressIPv6)
     return err
@@ -212,14 +221,8 @@ func freeDelegatedIp(danmClient danmclientset.Interface, netInfo *danmtypes.Danm
   if netInfo.Spec.NetworkType == "flannel" && ip != ""{
     flannelIpExhaustionWorkaround(ip)
   }
-  _, 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))) {
+  if wasIpAllocatedByDanm(ip, netInfo.Spec.Options.Cidr) || wasIpAllocatedByDanm(ip, netInfo.Spec.Options.Net6) {
     err := ipam.Free(danmClient, *netInfo, ip)
     if err != nil {
       return errors.New("cannot give back ip address: " + ip + " for network:" + netInfo.ObjectMeta.Name +
@@ -229,6 +232,18 @@ func freeDelegatedIp(danmClient danmclientset.Interface, netInfo *danmtypes.Danm
   return nil
 }
 
+func wasIpAllocatedByDanm(ip, cidr string) bool {
+  _, subnet, _ := net.ParseCIDR(cidr)
+  parsedIp := net.ParseIP(ip)
+  if parsedIp == nil {
+    parsedIp,_,_ = net.ParseCIDR(ip)
+  }
+  if parsedIp != nil && (subnet != nil && subnet.Contains(parsedIp)) {
+    return true
+  }
+  return false
+}
+
 // Host-local IPAM management plugin uses disk-local Store by default.
 // Right now it is buggy in a sense that it does not try to free IPs if the container being deleted does not exist already.
 // But it should!

pkg/datastructs/datastructs.go

@@ -10,7 +10,7 @@ const (
 )
 
 var (
-  SupportedCniVersions = version.PluginSupports("0.1.0", "0.2.0", "0.3.0", "0.3.1")
+  SupportedCniVersions = version.PluginSupports("0.3.1")
 )
 
 type NetConf struct {

pkg/metacni/metacni.go

@@ -393,6 +393,9 @@ func createDanmInterface(syncher *syncher.Syncher, danmClient danmclientset.Inte
     syncher.PushResult(netInfo.ObjectMeta.Name, errors.New("IP address reservation failed for network:" + netInfo.ObjectMeta.Name + " with error:" + err.Error()), nil)
     return
   }
+  //As netInfo is only copied to IPAM above, the IP allocation is not refreshed in the original copy.
+  //Without re-reading the network body we risk leaking IPs if an error happens later on within the same thread!
+  netInfo,_ = netcontrol.GetNetworkFromInterface(danmClient, iface, netInfo.ObjectMeta.Namespace)
   epSpec := danmtypes.DanmEpIface {
     Name: cnidel.CalculateIfaceName(DanmConfig.NamingScheme, netInfo.Spec.Options.Prefix, iface.DefaultIfaceName, iface.SequenceId),
     Address:     ip4,

test/stubs/danm/netclient_stub.go

@@ -57,17 +57,20 @@ func (netClient *NetClientStub) Update(obj *danmtypes.DanmNet) (*danmtypes.DanmN
       }
     }
   }
-  if strings.Contains(obj.Spec.NetworkID, "conflict") && obj.ObjectMeta.ResourceVersion != magicVersion {
-    for index, net := range netClient.TestNets {
-      if net.Spec.NetworkID == obj.Spec.NetworkID {
-        netClient.TestNets[index].ObjectMeta.ResourceVersion = magicVersion
-      }
+  var netIndex int
+  for index, net := range netClient.TestNets {
+    if net.ObjectMeta.Name == obj.ObjectMeta.Name {
+      netIndex = index
     }
+  }
+  if strings.Contains(obj.Spec.NetworkID, "conflict") && obj.ObjectMeta.ResourceVersion != magicVersion {
+    netClient.TestNets[netIndex].ObjectMeta.ResourceVersion = magicVersion
     return nil, errors.New(datastructs.OptimisticLockErrorMsg)
   }
   if strings.Contains(obj.Spec.NetworkID, "error") {
     return nil, errors.New("fatal error, don't retry")
   }
+  netClient.TestNets[netIndex] = *obj
   return obj, nil
 }
 
@@ -84,7 +87,7 @@ func (netClient *NetClientStub) Get(netName string, options meta_v1.GetOptions)
     return nil, errors.New("fatal error, don't retry")
   }
   for _, testNet := range netClient.TestNets {
-    if testNet.Spec.NetworkID == netName {
+    if testNet.ObjectMeta.Name == netName {
       return &testNet, nil
     }
   }