[NPM] 🐞 {fix} error creating IPsets when same string for ports and labels is used (#734)

* Adding port_ format for named ports

* Cleaning existing ipsets incase of a upgrade

* Changing the delimiter for port prefix

* Some basic formatting

* Adding fixes to testcases

* Addressing comments

* Adding mitigation for empty string in named port

* Changing port nil behavior. NPM does partial rule handling in port nil case

* removing exists check, as setmap is not accessible from this test

* Adding support to delete only azure-npm ipsets

* Adding support to delete only azure-npm ipsets

* Addressing comments

* Changing azure-npm to const flag and cleaning up un wanted error log

* Changing the make entries to 0

Author: vakalapa

npm/ipsm/ipsm.go

@@ -6,6 +6,7 @@ package ipsm
 import (
 	"os"
 	"os/exec"
+	"regexp"
 	"strings"
 	"syscall"
 
@@ -70,12 +71,12 @@ func (ipsMgr *IpsetManager) Exists(key string, val string, kind string) bool {
 
 // SetExists checks whehter an ipset exists.
 func (ipsMgr *IpsetManager) SetExists(setName, kind string) bool {
-    m := ipsMgr.setMap
-    if kind == util.IpsetSetListFlag {
-        m = ipsMgr.listMap
-    }
-    _, exists := m[setName]
-    return exists
+	m := ipsMgr.setMap
+	if kind == util.IpsetSetListFlag {
+		m = ipsMgr.listMap
+	}
+	_, exists := m[setName]
+	return exists
 }
 
 func isNsSet(setName string) bool {
@@ -459,4 +460,73 @@ func (ipsMgr *IpsetManager) Restore(configFile string) error {
 	//TODO based on the set name and number of entries in the config file, update IPSetInventory
 
 	return nil
-}
+}
+
+// DestroyNpmIpsets destroys only ipsets created by NPM
+func (ipsMgr *IpsetManager) DestroyNpmIpsets() error {
+
+	cmdName := util.Ipset
+	cmdArgs := util.IPsetCheckListFlag
+
+	reply, err := exec.Command(cmdName, cmdArgs).Output()
+	if msg, failed := err.(*exec.ExitError); failed {
+		errCode := msg.Sys().(syscall.WaitStatus).ExitStatus()
+		if errCode > 0 {
+			metrics.SendErrorMetric(util.IpsmID, "{DestroyNpmIpsets} Error: There was an error running command: [%s] Stderr: [%v, %s]", cmdName, err, strings.TrimSuffix(string(msg.Stderr), "\n"))
+		}
+
+		return err
+	}
+	if reply == nil {
+		metrics.SendErrorMetric(util.IpsmID, "{DestroyNpmIpsets} Received empty string from ipset list while destroying azure-npm ipsets")
+		return nil
+	}
+
+	log.Logf("{DestroyNpmIpsets} Reply from command %s executed is %s", cmdName+" "+cmdArgs, reply)
+	re := regexp.MustCompile("Name: (" + util.AzureNpmPrefix + "\\d+)")
+	ipsetRegexSlice := re.FindAllSubmatch(reply, -1)
+
+	if len(ipsetRegexSlice) == 0 {
+		log.Logf("No Azure-NPM IPsets are found in the Node.")
+		return nil
+	}
+
+	ipsetLists := make([]string, 0)
+	for _, matchedItem := range ipsetRegexSlice {
+		if len(matchedItem) == 2 {
+			itemString := string(matchedItem[1])
+			if strings.Contains(itemString, util.AzureNpmFlag) {
+				ipsetLists = append(ipsetLists, itemString)
+			}
+		}
+	}
+
+	if len(ipsetLists) == 0 {
+		return nil
+	}
+
+	entry := &ipsEntry{
+		operationFlag: util.IpsetFlushFlag,
+	}
+
+	for _, ipsetName := range ipsetLists {
+		entry := &ipsEntry{
+			operationFlag: util.IpsetFlushFlag,
+			set:           ipsetName,
+		}
+
+		if _, err := ipsMgr.Run(entry); err != nil {
+			metrics.SendErrorMetric(util.IpsmID, "{DestroyNpmIpsets} Error: failed to flush ipset %s", ipsetName)
+		}
+	}
+
+	for _, ipsetName := range ipsetLists {
+		entry.operationFlag = util.IpsetDestroyFlag
+		entry.set = ipsetName
+		if _, err := ipsMgr.Run(entry); err != nil {
+			metrics.SendErrorMetric(util.IpsmID, "{DestroyNpmIpsets} Error: failed to destroy ipset %s", ipsetName)
+		}
+	}
+
+	return nil
+}

npm/ipsm/ipsm_test.go

@@ -521,6 +521,28 @@ func TestRun(t *testing.T) {
 	}
 }
 
+func TestDestroyNpmIpsets(t *testing.T) {
+	ipsMgr := NewIpsetManager()
+
+	err := ipsMgr.CreateSet("azure-npm-123456", []string{"nethash"})
+	if err != nil {
+		t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.CreateSet")
+		t.Errorf(err.Error())
+	}
+
+	err = ipsMgr.CreateSet("azure-npm-56543", []string{"nethash"})
+	if err != nil {
+		t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.CreateSet")
+		t.Errorf(err.Error())
+	}
+
+	err = ipsMgr.DestroyNpmIpsets()
+	if err != nil {
+		t.Errorf("TestDestroyNpmIpsets failed @ ipsMgr.DestroyNpmIpsets")
+		t.Errorf(err.Error())
+	}
+}
+
 func TestMain(m *testing.M) {
 	metrics.InitializeAll()
 	ipsMgr := NewIpsetManager()

npm/npm.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/Azure/azure-container-networking/aitelemetry"
 	"github.com/Azure/azure-container-networking/log"
+	"github.com/Azure/azure-container-networking/npm/ipsm"
 	"github.com/Azure/azure-container-networking/npm/iptm"
 	"github.com/Azure/azure-container-networking/npm/metrics"
 	"github.com/Azure/azure-container-networking/npm/util"
@@ -188,6 +189,9 @@ func NewNetworkPolicyManager(clientset *kubernetes.Clientset, informerFactory in
 	iptMgr := iptm.NewIptablesManager()
 	iptMgr.UninitNpmChains()
 
+	log.Logf("Azure-NPM creating, cleaning existing Azure NPM IPSets")
+	ipsm.NewIpsetManager().DestroyNpmIpsets()
+
 	var (
 		podInformer   = informerFactory.Core().V1().Pods()
 		nsInformer    = informerFactory.Core().V1().Namespaces()

npm/pod.go

@@ -92,7 +92,14 @@ func (npMgr *NetworkPolicyManager) AddPod(podObj *corev1.Pod) error {
 				case v1.ProtocolSCTP:
 					protocol = util.IpsetSCTPFlag
 				}
-				ipsMgr.AddToSet(port.Name, fmt.Sprintf("%s,%s%d", podIP, protocol, port.ContainerPort), util.IpsetIPPortHashFlag, podUid)
+				namedPortname := util.NamedPortIPSetPrefix + port.Name
+				ipsMgr.AddToSet(
+					namedPortname,
+					fmt.Sprintf("%s,%s%d", podIP, protocol, port.ContainerPort),
+					util.IpsetIPPortHashFlag,
+					podUid,
+				)
+
 			}
 		}
 	}
@@ -209,7 +216,12 @@ func (npMgr *NetworkPolicyManager) DeletePod(podObj *corev1.Pod) error {
 				case v1.ProtocolSCTP:
 					protocol = util.IpsetSCTPFlag
 				}
-				ipsMgr.DeleteFromSet(port.Name, fmt.Sprintf("%s,%s%d", cachedPodIp, protocol, port.ContainerPort), podUid)
+				namedPortname := util.NamedPortIPSetPrefix + port.Name
+				ipsMgr.DeleteFromSet(
+					namedPortname,
+					fmt.Sprintf("%s,%s%d", cachedPodIp, protocol, port.ContainerPort),
+					podUid,
+				)
 			}
 		}
 	}

npm/pod_test.go

@@ -70,6 +70,18 @@ func TestAddPod(t *testing.T) {
 			Phase: "Running",
 			PodIP: "1.2.3.4",
 		},
+		Spec: corev1.PodSpec{
+			Containers: []corev1.Container{
+				corev1.Container{
+					Ports: []corev1.ContainerPort{
+						corev1.ContainerPort{
+							Name:          "app:test-pod",
+							ContainerPort: 8080,
+						},
+					},
+				},
+			},
+		},
 	}
 
 	npMgr.Lock()