[NPM] [BUG] Supporting multiple values under label selector MatchExpr

.github/workflows/cyclonus-netpol-test.yaml

@@ -40,7 +40,7 @@ jobs:
 
       - name: Install Azure NPM
         run: |
-          sed -i 's/mcr.microsoft.com\/containernetworking\/azure-npm:v1.3.1/acnpublic.azurecr.io\/azure-npm:cyclonus/' ./npm/azure-npm.yaml
+          sed -i 's/mcr.microsoft.com\/containernetworking\/azure-npm:v1.4.1/acnpublic.azurecr.io\/azure-npm:cyclonus/' ./npm/azure-npm.yaml
           kind load docker-image acnpublic.azurecr.io/azure-npm:cyclonus --name npm-kind
           kubectl apply -f ./npm/azure-npm.yaml
 

npm/ipsm/ipsm.go

@@ -17,6 +17,14 @@ import (
 	utilexec "k8s.io/utils/exec"
 )
 
+// ReferCountOperation is used to indicate whether ipset refer count should be increased or decreased.
+type ReferCountOperation bool
+
+const (
+	IncrementOp ReferCountOperation = true
+	DecrementOp ReferCountOperation = false
+)
+
 type ipsEntry struct {
 	operationFlag string
 	name          string
@@ -38,11 +46,20 @@ type Ipset struct {
 	referCount int
 }
 
+func (ipset *Ipset) incReferCount() {
+	ipset.referCount++
+}
+
+func (ipset *Ipset) decReferCount() {
+	ipset.referCount--
+}
+
 // NewIpset creates a new instance for Ipset object.
 func NewIpset(setName string) *Ipset {
 	return &Ipset{
-		name:     setName,
-		elements: make(map[string]string),
+		name:       setName,
+		elements:   make(map[string]string),
+		referCount: 0,
 	}
 }
 
@@ -73,6 +90,21 @@ func (ipsMgr *IpsetManager) Exists(listName string, setName string, kind string)
 	return true
 }
 
+// IpSetReferIncOrDec checks if an element exists in setMap/listMap and then increases or decreases this referCount.
+func (ipsMgr *IpsetManager) IpSetReferIncOrDec(ipsetName string, kind string, countOperation ReferCountOperation) {
+	m := ipsMgr.SetMap
+	if kind == util.IpsetSetListFlag {
+		m = ipsMgr.ListMap
+	}
+
+	switch countOperation {
+	case IncrementOp:
+		m[ipsetName].incReferCount()
+	case DecrementOp:
+		m[ipsetName].decReferCount()
+	}
+}
+
 // SetExists checks if an ipset exists, and returns the type
 func (ipsMgr *IpsetManager) SetExists(setName string) (bool, string) {
 	_, exists := ipsMgr.SetMap[setName]
@@ -122,6 +154,11 @@ func (ipsMgr *IpsetManager) DeleteList(listName string) error {
 		set:           util.GetHashedName(listName),
 	}
 
+	if ipsMgr.ListMap[listName].referCount > 0 {
+		ipsMgr.IpSetReferIncOrDec(listName, util.IpsetSetListFlag, DecrementOp)
+		return nil
+	}
+
 	if errCode, err := ipsMgr.Run(entry); err != nil {
 		if errCode == 1 {
 			return nil
@@ -428,6 +465,8 @@ func (ipsMgr *IpsetManager) DeleteFromSet(setName, ip, podKey string) error {
 	return nil
 }
 
+// TODO this below function is to be extended while improving ipset refer count
+// support, if not used, please remove this stale function.
 // Clean removes all the empty sets & lists under the namespace.
 func (ipsMgr *IpsetManager) Clean() error {
 	for setName, set := range ipsMgr.SetMap {

npm/networkPolicyController.go

@@ -351,10 +351,24 @@ func (c *networkPolicyController) syncAddAndUpdateNetPol(netPolObj *networkingv1
 			return fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset named port %s with err: %v", set, err)
 		}
 	}
-	for _, list := range lists {
-		if err = ipsMgr.CreateList(list); err != nil {
-			return fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset list %s with err: %v", list, err)
+
+	// lists is a map with list name and members as value
+	// NPM will create the list first and increments the refer count
+	for listKey := range lists {
+		if err = ipsMgr.CreateList(listKey); err != nil {
+			return fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset list %s with err: %v", listKey, err)
+		}
+		ipsMgr.IpSetReferIncOrDec(listKey, util.IpsetSetListFlag, ipsm.IncrementOp)
+	}
+	// Then NPM will add members to the above list, this is to avoid members being added
+	// to lists before they are created.
+	for listKey, listLabelsMembers := range lists {
+		for _, listMember := range listLabelsMembers {
+			if err = ipsMgr.AddToList(listKey, listMember); err != nil {
+				return fmt.Errorf("[syncAddAndUpdateNetPol] Error: Adding ipset member %s to ipset list %s with err: %v", listMember, listKey, err)
+			}
 		}
+		ipsMgr.IpSetReferIncOrDec(listKey, util.IpsetSetListFlag, ipsm.IncrementOp)
 	}
 
 	if err = c.createCidrsRule("in", netPolObj.ObjectMeta.Name, netPolObj.ObjectMeta.Namespace, ingressIPCidrs, ipsMgr); err != nil {
@@ -385,7 +399,7 @@ func (c *networkPolicyController) cleanUpNetworkPolicy(netPolKey string, isSafeC
 	ipsMgr := c.npMgr.NsMap[util.KubeAllNamespacesFlag].IpsMgr
 	iptMgr := c.npMgr.NsMap[util.KubeAllNamespacesFlag].iptMgr
 	// translate policy from "cachedNetPolObj"
-	_, _, _, ingressIPCidrs, egressIPCidrs, iptEntries := translatePolicy(cachedNetPolObj)
+	_, _, lists, ingressIPCidrs, egressIPCidrs, iptEntries := translatePolicy(cachedNetPolObj)
 
 	var err error
 	// delete iptables entries
@@ -395,6 +409,17 @@ func (c *networkPolicyController) cleanUpNetworkPolicy(netPolKey string, isSafeC
 		}
 	}
 
+	// lists is a map with list name and members as value
+	for listKey := range lists {
+		// We do not have delete the members before deleting set as,
+		// 1. ipset allows deleting a ipset list with members
+		// 2. if the refer count is more than one we should not remove members
+		// 3. for reduced datapath operations
+		if err = ipsMgr.DeleteList(listKey); err != nil {
+			return fmt.Errorf("[syncAddAndUpdateNetPol] Error: creating ipset list %s with err: %v", listKey, err)
+		}
+	}
+
 	// delete ipset list related to ingress CIDRs
 	if err = c.removeCidrsRule("in", cachedNetPolObj.Name, cachedNetPolObj.Namespace, ingressIPCidrs, ipsMgr); err != nil {
 		return fmt.Errorf("[cleanUpNetworkPolicy] Error: removeCidrsRule in due to %v", err)

npm/npm.go

@@ -164,7 +164,7 @@ func (npMgr *NetworkPolicyManager) backup() {
 		time.Sleep(backupWaitTimeInSeconds * time.Second)
 
 		if err = iptMgr.Save(util.IptablesConfigFile); err != nil {
-			metrics.SendErrorLogAndMetric(util.NpmID, "Error: failed to back up Azure-NPM states")
+			metrics.SendErrorLogAndMetric(util.NpmID, "Error: failed to back up Azure-NPM states %s", err.Error())
 		}
 	}
 }

npm/parseSelector.go

@@ -121,73 +121,201 @@ func sortSelector(selector *metav1.LabelSelector) {
 	selector.MatchExpressions = sortedReqs
 }
 
+// getSetNameForMultiValueSelector takes in label with multiple values without operator
+// and returns a new 2nd level ipset name
+func getSetNameForMultiValueSelector(key string, vals []string) string {
+	newIpSet := key
+	for _, val := range vals {
+		newIpSet = util.GetIpSetFromLabelKV(newIpSet, val)
+	}
+	return newIpSet
+}
+
 // HashSelector returns the hash value of the selector.
 func HashSelector(selector *metav1.LabelSelector) string {
 	sortSelector(selector)
 	return util.Hash(fmt.Sprintf("%v", selector))
 }
 
-// parseSelector takes a LabelSelector and returns a slice of processed labels, keys and values.
-func parseSelector(selector *metav1.LabelSelector) ([]string, []string, []string) {
+// flattenNameSpaceSelector will help flatten multiple NameSpace selector match Expressions values
+// into multiple label selectors helping with the OR condition.
+func FlattenNameSpaceSelector(nsSelector *metav1.LabelSelector) []metav1.LabelSelector {
+	/*
+			This function helps to create multiple labelSelectors when given a single multivalue nsSelector
+			Take below exmaple: this nsSelector has 2 values in a matchSelector.
+			- namespaceSelector:
+		        matchExpressions:
+		        - key: ns
+		          operator: NotIn
+		          values:
+		          - netpol-x
+		          - netpol-y
+
+			goal is to convert this single nsSelector into multiple nsSelectors to preserve OR condition
+			between multiple values of the matchExpr i.e. this function will return
+
+			- namespaceSelector:
+		        matchExpressions:
+		        - key: ns
+		          operator: NotIn
+		          values:
+		          - netpol-x
+			- namespaceSelector:
+		        matchExpressions:
+		        - key: ns
+		          operator: NotIn
+		          values:
+		          - netpol-y
+
+			then, translate policy will replicate each of these nsSelectors to add two different rules in iptables,
+			resulting in OR condition between the values.
+
+			Check TestFlattenNameSpaceSelector 2nd subcase for complex scenario
+	*/
+
+	// To avoid any additional length checks, just return a slice of labelSelectors
+	// with original nsSelector
+	if nsSelector == nil {
+		return []metav1.LabelSelector{}
+	}
+
+	if len(nsSelector.MatchExpressions) == 0 {
+		return []metav1.LabelSelector{*nsSelector}
+	}
+
+	// create a baseSelector which needs to be same across all
+	// new labelSelectors
+	baseSelector := &metav1.LabelSelector{
+		MatchLabels:      nsSelector.MatchLabels,
+		MatchExpressions: []metav1.LabelSelectorRequirement{},
+	}
+
+	multiValuePresent := false
+	multiValueMatchExprs := []metav1.LabelSelectorRequirement{}
+	for _, req := range nsSelector.MatchExpressions {
+
+		// Only In and NotIn operators of matchExprs have multiple values
+		// NPM will ignore single value matchExprs of these operators.
+		// for multiple values, it will create a slice of them to be used for Zipping with baseSelector
+		// to create multiple nsSelectors to preserve OR condition across all labels and expressions
+		if (req.Operator == metav1.LabelSelectorOpIn) || (req.Operator == metav1.LabelSelectorOpNotIn) {
+			if len(req.Values) == 1 {
+				// for length 1, add the matchExpr to baseSelector
+				baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req)
+			} else {
+				multiValuePresent = true
+				multiValueMatchExprs = append(multiValueMatchExprs, req)
+			}
+		} else if (req.Operator == metav1.LabelSelectorOpExists) || (req.Operator == metav1.LabelSelectorOpDoesNotExist) {
+			// since Exists and NotExists do not contain any values, NPM can safely add them to the baseSelector
+			baseSelector.MatchExpressions = append(baseSelector.MatchExpressions, req)
+		} else {
+			log.Errorf("Invalid operator [%s] for selector [%v] requirement", req.Operator, *nsSelector)
+		}
+	}
+
+	// If there are no multiValue NS selector match expressions
+	// return the original NsSelector
+	if !multiValuePresent {
+		return []metav1.LabelSelector{*nsSelector}
+	}
+
+	// Now use the baseSelector and loop over multiValueMatchExprs to create all
+	// combinations of values
+	flatNsSelectors := []metav1.LabelSelector{
+		*baseSelector.DeepCopy(),
+	}
+	for _, req := range multiValueMatchExprs {
+		flatNsSelectors = zipMatchExprs(flatNsSelectors, req)
+	}
+
+	return flatNsSelectors
+}
+
+// zipMatchExprs helps with zipping a given matchExpr with given baseLabelSelectors
+// this func will loop over each baseSelector in the slice,
+// deepCopies each baseSelector, combines with given matchExpr by looping over each value
+// and creating a new LabelSelector with given baseSelector and value matchExpr
+// then returns a new slice of these zipped LabelSelectors
+func zipMatchExprs(baseSelectors []metav1.LabelSelector, matchExpr metav1.LabelSelectorRequirement) []metav1.LabelSelector {
+	zippedLabelSelectors := []metav1.LabelSelector{}
+	for _, selector := range baseSelectors {
+		for _, value := range matchExpr.Values {
+			tempBaseSelector := selector.DeepCopy()
+			tempBaseSelector.MatchExpressions = append(
+				tempBaseSelector.MatchExpressions,
+				metav1.LabelSelectorRequirement{
+					Key:      matchExpr.Key,
+					Operator: matchExpr.Operator,
+					Values:   []string{value},
+				},
+			)
+			zippedLabelSelectors = append(zippedLabelSelectors, *tempBaseSelector)
+
+		}
+	}
+	return zippedLabelSelectors
+}
+
+// parseSelector takes a LabelSelector and returns a slice of processed labels, Lists with members as values.
+// this function returns a slice of all the label ipsets excluding multivalue matchExprs
+// and a map of labelKeys and labelIpsetname for multivalue match exprs
+// higher level functions will need to compute what sets or ipsets should be
+// used from this map
+func parseSelector(selector *metav1.LabelSelector) ([]string, map[string][]string) {
 	var (
 		labels []string
-		keys   []string
-		vals   []string
+		vals   map[string][]string
 	)
 
+	vals = make(map[string][]string)
 	if selector == nil {
-		return labels, keys, vals
+		return labels, vals
 	}
 
 	if len(selector.MatchLabels) == 0 && len(selector.MatchExpressions) == 0 {
 		labels = append(labels, "")
-		keys = append(keys, "")
-		vals = append(vals, "")
-
-		return labels, keys, vals
+		return labels, vals
 	}
 
 	sortedKeys, sortedVals := util.SortMap(&selector.MatchLabels)
 
 	for i := range sortedKeys {
 		labels = append(labels, sortedKeys[i]+":"+sortedVals[i])
 	}
-	keys = append(keys, sortedKeys...)
-	vals = append(vals, sortedVals...)
 
 	for _, req := range selector.MatchExpressions {
 		var k string
 		switch op := req.Operator; op {
 		case metav1.LabelSelectorOpIn:
-			for _, v := range req.Values {
-				k = req.Key
-				keys = append(keys, k)
-				vals = append(vals, v)
-				labels = append(labels, k+":"+v)
+			k = req.Key
+			if len(req.Values) == 1 {
+				labels = append(labels, k+":"+req.Values[0])
+			} else {
+				// We are not adding the k:v to labels for multiple values, because, labels are used
+				// to contruct partial IptEntries and if these below labels are added then we are inducing
+				// AND condition on values of a match expression instead of OR
+				vals[k] = append(vals[k], req.Values...)
 			}
 		case metav1.LabelSelectorOpNotIn:
-			for _, v := range req.Values {
-				k = util.IptablesNotFlag + req.Key
-				keys = append(keys, k)
-				vals = append(vals, v)
-				labels = append(labels, k+":"+v)
+			k = util.IptablesNotFlag + req.Key
+			if len(req.Values) == 1 {
+				labels = append(labels, k+":"+req.Values[0])
+			} else {
+				vals[k] = append(vals[k], req.Values...)
 			}
 		// Exists matches pods with req.Key as key
 		case metav1.LabelSelectorOpExists:
 			k = req.Key
-			keys = append(keys, req.Key)
-			vals = append(vals, "")
 			labels = append(labels, k)
 		// DoesNotExist matches pods without req.Key as key
 		case metav1.LabelSelectorOpDoesNotExist:
 			k = util.IptablesNotFlag + req.Key
-			keys = append(keys, k)
-			vals = append(vals, "")
 			labels = append(labels, k)
 		default:
 			log.Errorf("Invalid operator [%s] for selector [%v] requirement", op, *selector)
 		}
 	}
 
-	return labels, keys, vals
+	return labels, vals
 }