Add VlanId in the cmdAdd Result struct (#1705)

* Add VlanId in the cmdAdd Result struct
This VlanId will appear in the prevResult during cmdDel request

CleanUp Pod Network using vlanId from prevResult in CNI itself

Skip processing Delete request if prevResult is nil
Add Logging vlanId to ipamd

Add support to test with containerd nodegroup in pod-eni test

Add check for empty Netns() in cni

Updated test agent version and Image

* Temporary Commit - to be reviewed

* Disable CHECK for this network configuration list
ref: https://github.com/containernetworking/cni/blob/spec-v0.4.0/SPEC.md#cni-plugin

* disableCheck value should be boolean

* Rename vlanInterface as dummyVlanInterface
Updated Prefix for dummyVlanInterface name

* Add dummyVlan only for SGP pods

* Minor fix

* Schedule max allowed pods on given instance for pod_networking_suite_test

* Fixed logging in cni.go
Add relevant comments

* Addresses PR comments

* Return error for incorrect vlanId

* fixed comment

* Formatted code files

* Minor logging change

* minor nit change

cmd/routed-eni-cni-plugin/cni.go

@@ -22,6 +22,7 @@ import (
 	"net"
 	"os"
 	"runtime"
+	"strconv"
 	"strings"
 
 	"github.com/containernetworking/cni/pkg/skel"
@@ -44,6 +45,9 @@ import (
 
 const ipamdAddress = "127.0.0.1:50051"
 
+const vlanInterfacePrefix = "vlan"
+const dummyVlanInterfacePrefix = "dummy"
+
 var version string
 
 // NetConf stores the common network config for the CNI plugin
@@ -95,6 +99,12 @@ func LoadNetConf(bytes []byte) (*NetConf, logger.Logger, error) {
 		return nil, nil, errors.Wrap(err, "add cmd: error loading config from args")
 	}
 
+	if conf.RawPrevResult != nil {
+		if err := cniSpecVersion.ParsePrevResult(&conf.NetConf); err != nil {
+			return nil, nil, fmt.Errorf("could not parse prevResult: %v", err)
+		}
+	}
+
 	logConfig := logger.Configuration{
 		LogLevel:    conf.PluginLogLevel,
 		LogLocation: conf.PluginLogFile,
@@ -122,6 +132,8 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 	log.Infof("Received CNI add request: ContainerID(%s) Netns(%s) IfName(%s) Args(%s) Path(%s) argsStdinData(%s)",
 		args.ContainerID, args.Netns, args.IfName, args.Args, args.Path, args.StdinData)
 
+	log.Debugf("Prev Result: %v\n", conf.PrevResult)
+
 	var k8sArgs K8sArgs
 	if err := cniTypes.LoadArgs(args.Args, &k8sArgs); err != nil {
 		log.Errorf("Failed to load k8s config from arg: %v", err)
@@ -192,16 +204,28 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 	}
 
 	var hostVethName string
-	if r.PodVlanId != 0 {
-		hostVethName = generateHostVethName("vlan", string(k8sArgs.K8S_POD_NAMESPACE), string(k8sArgs.K8S_POD_NAME))
+	var dummyVlanInterface *current.Interface
 
+	// Non-zero value means pods are using branch ENI
+	if r.PodVlanId != 0 {
+		hostVethName = generateHostVethName(vlanInterfacePrefix, string(k8sArgs.K8S_POD_NAMESPACE), string(k8sArgs.K8S_POD_NAME))
 		err = driverClient.SetupPodENINetwork(hostVethName, args.IfName, args.Netns, v4Addr, v6Addr, int(r.PodVlanId), r.PodENIMAC,
 			r.PodENISubnetGW, int(r.ParentIfIndex), mtu, log)
+
+		// This is a dummyVlanInterfaceName generated to identify dummyVlanInterface
+		// which will be created for PPSG scenario to pass along the vlanId information
+		// as a part of the ADD cmd Result struct
+		// The podVlanId is used by DEL cmd, fetched from the prevResult struct to cleanup the pod network
+		dummyVlanInterfaceName := generateHostVethName(dummyVlanInterfacePrefix, string(k8sArgs.K8S_POD_NAMESPACE), string(k8sArgs.K8S_POD_NAME))
+
+		// The dummyVlanInterface is purely virtual and relevent only for ppsg, so we decided to keep it separate
+		// and not overload the already available hostVethInterface
+		dummyVlanInterface = &current.Interface{Name: dummyVlanInterfaceName, Mac: fmt.Sprint(r.PodVlanId)}
+		log.Debugf("Using dummy vlanInterface: %v", dummyVlanInterface)
 	} else {
 		// build hostVethName
 		// Note: the maximum length for linux interface name is 15
 		hostVethName = generateHostVethName(conf.VethPrefix, string(k8sArgs.K8S_POD_NAMESPACE), string(k8sArgs.K8S_POD_NAME))
-
 		err = driverClient.SetupNS(hostVethName, args.IfName, args.Netns, v4Addr, v6Addr, int(r.DeviceNumber), r.VPCv4CIDRs, r.UseExternalSNAT, mtu, log)
 	}
 
@@ -250,6 +274,11 @@ func add(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 		},
 	}
 
+	// We append dummyVlanInterface only for pods using branch ENI
+	if dummyVlanInterface != nil {
+		result.Interfaces = append(result.Interfaces, dummyVlanInterface)
+	}
+
 	return cniTypes.PrintResult(result, conf.CNIVersion)
 }
 
@@ -270,6 +299,8 @@ func del(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 	driverClient driver.NetworkAPIs) error {
 
 	conf, log, err := LoadNetConf(args.StdinData)
+	log.Debugf("Prev Result: %v\n", conf.PrevResult)
+
 	if err != nil {
 		return errors.Wrap(err, "add cmd: error loading config from args")
 	}
@@ -283,6 +314,47 @@ func del(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 		return errors.Wrap(err, "del cmd: failed to load k8s config from args")
 	}
 
+	// With containerd as the runtime, it was observed that sometimes spurious delete requests
+	// are triggered from kubelet with an empty Netns. This check safeguards against such
+	// scenarios and we just return
+	// ref: https://github.com/kubernetes/kubernetes/issues/44100#issuecomment-329780382
+	if args.Netns == "" {
+		log.Info("Netns() is empty, so network already cleanedup. Nothing to do")
+		return nil
+	}
+	prevResult, ok := conf.PrevResult.(*current.Result)
+
+	// Try to use prevResult if available
+	// prevResult might not be availabe, if we are still using older cni spec < 0.4.0.
+	// So we should fallback to the old clean up method
+	if ok {
+		dummyVlanInterfaceName := generateHostVethName(dummyVlanInterfacePrefix, string(k8sArgs.K8S_POD_NAMESPACE), string(k8sArgs.K8S_POD_NAME))
+		for _, iface := range prevResult.Interfaces {
+			if iface.Name == dummyVlanInterfaceName {
+				podVlanId, err := strconv.Atoi(iface.Mac)
+				if err != nil {
+					log.Errorf("Failed to parse vlanId from prevResult: %v", err)
+					return errors.Wrap(err, "del cmd: failed to parse vlanId from prevResult")
+				}
+
+				// podVlanID can not be 0 as we add dummyVlanInterface only for ppsg
+				// if it is 0 then we should return an error
+				if podVlanId == 0 {
+					log.Errorf("Found SG pod:%s namespace:%s with 0 vlanID", k8sArgs.K8S_POD_NAME, k8sArgs.K8S_POD_NAMESPACE)
+					return errors.Wrap(err, "del cmd: found Incorrect 0 vlandId for ppsg")
+				}
+
+				err = cleanUpPodENI(podVlanId, log, args.ContainerID, driverClient)
+				if err != nil {
+					return err
+				}
+				log.Infof("Received del network response for pod %s namespace %s sandbox %s with vlanId: %v", string(k8sArgs.K8S_POD_NAME),
+					string(k8sArgs.K8S_POD_NAMESPACE), string(k8sArgs.K8S_POD_INFRA_CONTAINER_ID), podVlanId)
+				return nil
+			}
+		}
+	}
+
 	// notify local IP address manager to free secondary IP
 	// Set up a connection to the server.
 	conn, err := grpcClient.Dial(ipamdAddress, grpc.WithInsecure())
@@ -362,6 +434,16 @@ func del(args *skel.CmdArgs, cniTypes typeswrapper.CNITYPES, grpcClient grpcwrap
 	return nil
 }
 
+func cleanUpPodENI(podVlanId int, log logger.Logger, containerId string, driverClient driver.NetworkAPIs) error {
+	err := driverClient.TeardownPodENINetwork(podVlanId, log)
+	if err != nil {
+		log.Errorf("Failed on TeardownPodNetwork for container ID %s: %v",
+			containerId, err)
+		return errors.Wrap(err, "del cmd: failed on tear down pod network")
+	}
+	return nil
+}
+
 func main() {
 	log := logger.DefaultLogger()
 	about := fmt.Sprintf("AWS CNI %s", version)