feat: don't use CNS for CNI DEL command in windows multitenancy (#1216)

* feat: don't use CNS for CNI DEL command in windows multitenancy

* go fmt

* go fmt take 2

* fix: don't fallback to CNS for getNetwork or deleteHostNCApipaEndpoint, handle errNetworkNotFound

* test: add test for FindNetworkIDFromNetNs

* fix: getNetworkName needs to fallback to CNS when not found in state file for ADD

* fix: simplify the deleteHostNCApipaEndpoint function

* fix: linter

* fix: cnm should compile

* fix: always return retriable error for endpoint deletion failure

* fix: handle npe in cns/hnsclient by not using that package

* fix: logging

* fix: don't try cns if there is no multitenancy client

* fix: don't call CNS twice during ADD cmd

* fix: use hns wrapper, add some logging, don't return error when endpoint is already deleted

Author: thatmattlong

cni/client/client_common_test.go

@@ -1,3 +1,4 @@
+//go:build unit || integration
 // +build unit integration
 
 package client

cni/client/client_integration_test.go

@@ -1,5 +1,5 @@
-// +build linux
-// +build integration
+//go:build linux && integration
+// +build linux,integration
 
 package client
 

cni/client/client_unit_test.go

@@ -1,3 +1,4 @@
+//go:build unit
 // +build unit
 
 package client

cni/network/network.go

@@ -453,7 +453,7 @@ func (plugin *NetPlugin) Add(args *cniSkel.CmdArgs) error {
 	}
 
 	// Initialize values from network config.
-	networkID, err := plugin.getNetworkName(k8sPodName, k8sNamespace, args.IfName, nwCfg)
+	networkID, err := plugin.getNetworkName(k8sPodName, k8sNamespace, args.IfName, args.Netns, cnsNetworkConfig, nwCfg)
 	if err != nil {
 		log.Printf("[cni-net] Failed to extract network name from network config. error: %v", err)
 		return err
@@ -843,7 +843,7 @@ func (plugin *NetPlugin) Get(args *cniSkel.CmdArgs) error {
 	}
 
 	// Initialize values from network config.
-	if networkId, err = plugin.getNetworkName(k8sPodName, k8sNamespace, args.IfName, nwCfg); err != nil {
+	if networkId, err = plugin.getNetworkName(k8sPodName, k8sNamespace, args.IfName, args.Netns, nil, nwCfg); err != nil {
 		// TODO: Ideally we should return from here only.
 		log.Printf("[cni-net] Failed to extract network name from network config. error: %v", err)
 	}
@@ -962,14 +962,13 @@ func (plugin *NetPlugin) Delete(args *cniSkel.CmdArgs) error {
 			plugin.ipamInvoker = NewAzureIpamInvoker(plugin, &nwInfo)
 		}
 	}
-	// Initialize values from network config.
-	networkID, err = plugin.getNetworkName(k8sPodName, k8sNamespace, args.IfName, nwCfg)
 
-	// If error is not found error, then we ignore it, to comply with CNI SPEC.
+	// Initialize values from network config.
+	networkID, err = plugin.getNetworkName(k8sPodName, k8sNamespace, args.IfName, args.Netns, nil, nwCfg)
 	if err != nil {
 		log.Printf("[cni-net] Failed to extract network name from network config. error: %v", err)
-
-		if !cnscli.IsNotFound(err) {
+		// If error is not found error, then we ignore it, to comply with CNI SPEC.
+		if !network.IsNetworkNotFoundError(err) {
 			err = plugin.Errorf("Failed to extract network name from network config. error: %v", err)
 			return err
 		}
@@ -1013,19 +1012,15 @@ func (plugin *NetPlugin) Delete(args *cniSkel.CmdArgs) error {
 		return err
 	}
 
-	cnsclient, err := cnscli.New(nwCfg.CNSUrl, defaultRequestTimeout)
-	if err != nil {
-		log.Printf("failed to initialized cns client with URL %s: %v", nwCfg.CNSUrl, err.Error())
-		return plugin.Errorf(err.Error())
-	}
-
 	// schedule send metric before attempting delete
 	defer sendMetricFunc()
 	telemetry.LogAndSendEvent(plugin.tb, fmt.Sprintf("Deleting endpoint:%v", endpointID))
 	// Delete the endpoint.
-	if err = plugin.nm.DeleteEndpoint(cnsclient, networkID, endpointID); err != nil {
-		err = plugin.Errorf("Failed to delete endpoint: %v", err)
-		return err
+	if err = plugin.nm.DeleteEndpoint(networkID, endpointID); err != nil {
+		// return a retriable error so the container runtime will retry this DEL later
+		// the implementation of this function returns nil if the endpoint doens't exist, so
+		// we don't have to check that here
+		return plugin.RetriableError(fmt.Errorf("failed to delete endpoint: %w", err))
 	}
 
 	if !nwCfg.MultiTenancy {

cni/network/network_linux.go

@@ -135,7 +135,7 @@ func updateSubnetPrefix(cnsNetworkConfig *cns.GetNetworkContainerResponse, subne
 	return nil
 }
 
-func (plugin *NetPlugin) getNetworkName(podName, podNs, ifName string, nwCfg *cni.NetworkConfig) (string, error) {
+func (plugin *NetPlugin) getNetworkName(_, _, _, _ string, _ *cns.GetNetworkContainerResponse, nwCfg *cni.NetworkConfig) (string, error) {
 	return nwCfg.Name, nil
 }
 

cni/network/network_linux_test.go

@@ -1,4 +1,5 @@
-//+build linux
+//go:build linux
+// +build linux
 
 package network
 

cni/network/network_test.go

@@ -1060,7 +1060,7 @@ func TestGetNetworkName(t *testing.T) {
 	for _, tt := range tests {
 		tt := tt
 		t.Run(tt.name, func(t *testing.T) {
-			nwName, _ := plugin.getNetworkName("", "", "", &tt.nwCfg)
+			nwName, _ := plugin.getNetworkName("", "", "", "", nil, &tt.nwCfg)
 			require.Equal(t, tt.nwCfg.Name, nwName)
 		})
 	}

cni/network/network_windows.go

@@ -1,7 +1,6 @@
 package network
 
 import (
-	"context"
 	"encoding/json"
 	"fmt"
 	"net"
@@ -160,42 +159,45 @@ func updateSubnetPrefix(cnsNwConfig *cns.GetNetworkContainerResponse, subnetPref
 	return nil
 }
 
-func (plugin *NetPlugin) getNetworkName(podName, podNs, ifName string, nwCfg *cni.NetworkConfig) (string, error) {
-	var (
-		networkName      string
-		err              error
-		cnsNetworkConfig *cns.GetNetworkContainerResponse
-	)
-
-	networkName = nwCfg.Name
-	err = nil
-
-	if nwCfg.MultiTenancy {
-		determineWinVer()
-		if len(strings.TrimSpace(podName)) == 0 || len(strings.TrimSpace(podNs)) == 0 {
-			err = fmt.Errorf("POD info cannot be empty. PodName: %s, PodNamespace: %s", podName, podNs)
-			return networkName, err
-		}
+func (plugin *NetPlugin) getNetworkName(podName, podNs, ifName, netNs string, cnsResponse *cns.GetNetworkContainerResponse, nwCfg *cni.NetworkConfig) (string, error) {
+	// For singletenancy, the network name is simply the nwCfg.Name
+	if !nwCfg.MultiTenancy {
+		return nwCfg.Name, nil
+	}
 
-		_, cnsNetworkConfig, _, err = plugin.multitenancyClient.GetContainerNetworkConfiguration(context.TODO(), nwCfg, podName, podNs, ifName)
-		if err != nil {
-			log.Printf(
-				"GetContainerNetworkConfiguration failed for podname %v namespace %v with error %v",
-				podName,
-				podNs,
-				err)
-		} else {
-			var subnet net.IPNet
-			if err = updateSubnetPrefix(cnsNetworkConfig, &subnet); err == nil {
-				// networkName will look like ~ azure-vlan1-172-28-1-0_24
-				networkName = strings.Replace(subnet.String(), ".", "-", -1)
-				networkName = strings.Replace(networkName, "/", "_", -1)
-				networkName = fmt.Sprintf("%s-vlan%v-%v", nwCfg.Name, cnsNetworkConfig.MultiTenancyInfo.ID, networkName)
-			}
+	// in multitenancy case, the network name will be in the state file or can be built from cnsResponse
+	determineWinVer()
+	if len(strings.TrimSpace(netNs)) == 0 || len(strings.TrimSpace(podName)) == 0 || len(strings.TrimSpace(podNs)) == 0 {
+		return "", fmt.Errorf("POD info cannot be empty. PodName: %s, PodNamespace: %s, NetNs: %s", podName, podNs, netNs)
+	}
+
+	// First try to build the network name from the cnsResponse if present
+	// This will happen during ADD call
+	if cnsResponse != nil {
+		var subnet net.IPNet
+		if err := updateSubnetPrefix(cnsResponse, &subnet); err != nil {
+			log.Printf("Error updating subnet prefix: %v", err)
+			return "", err
 		}
+
+		// networkName will look like ~ azure-vlan1-172-28-1-0_24
+		networkSuffix := strings.Replace(subnet.String(), ".", "-", -1)
+		networkSuffix = strings.Replace(networkSuffix, "/", "_", -1)
+		networkName := fmt.Sprintf("%s-vlan%v-%v", nwCfg.Name, cnsResponse.MultiTenancyInfo.ID, networkSuffix)
+
+		return networkName, nil
+	}
+
+	// If no cnsResponse was present, try to get the network name from the state file
+	// This will happen during DEL call
+	networkName, err := plugin.nm.FindNetworkIDFromNetNs(netNs)
+	if err != nil {
+		log.Printf("Error getting network name from state: %v.", err)
+		return "", fmt.Errorf("error getting network name from state: %w", err)
+
 	}
 
-	return networkName, err
+	return networkName, nil
 }
 
 func setupInfraVnetRoutingForMultitenancy(

cni/network/network_windows_test.go

@@ -183,7 +183,7 @@ func TestSetEndpointOptions(t *testing.T) {
 					ID: 1,
 				},
 				CnetAddressSpace: []cns.IPSubnet{
-					cns.IPSubnet{
+					{
 						IPAddress:    "192.168.0.4",
 						PrefixLength: 24,
 					},
@@ -221,7 +221,7 @@ func TestSetPoliciesFromNwCfg(t *testing.T) {
 			nwCfg: cni.NetworkConfig{
 				RuntimeConfig: cni.RuntimeConfig{
 					PortMappings: []cni.PortMapping{
-						cni.PortMapping{
+						{
 							Protocol:      "tcp",
 							HostIp:        "19.268.0.4",
 							HostPort:      8000,

cni/plugin.go

@@ -150,6 +150,13 @@ func (plugin *Plugin) Errorf(format string, args ...interface{}) *cniTypes.Error
 	return plugin.Error(fmt.Errorf(format, args...))
 }
 
+// RetriableError logs and returns a CNI error with the TryAgainLater error code
+func (plugin *Plugin) RetriableError(err error) *cniTypes.Error {
+	tryAgainErr := cniTypes.NewError(cniTypes.ErrTryAgainLater, err.Error(), "")
+	log.Printf("[%v] %+v.", plugin.Name, tryAgainErr.Error())
+	return tryAgainErr
+}
+
 // Initialize key-value store
 func (plugin *Plugin) InitializeKeyValueStore(config *common.PluginConfig) error {
 	// Create the key value store.