fix: [NPM] niche multi-value pod selector bug

npm/pkg/controlplane/translation/parseSelector.go

@@ -1,6 +1,8 @@
 package translation
 
 import (
+	"fmt"
+
 	"github.com/Azure/azure-container-networking/log"
 	"github.com/Azure/azure-container-networking/npm/pkg/dataplane/ipsets"
 	"github.com/Azure/azure-container-networking/npm/util"
@@ -230,7 +232,7 @@ func parseNSSelector(selector *metav1.LabelSelector) []labelSelector {
 // parsePodSelector parses podSelector and returns slice of labelSelector object
 // which includes operator, setType, ipset name and its members slice.
 // Members slice exists only if setType is only NestedLabelOfPod.
-func parsePodSelector(selector *metav1.LabelSelector) ([]labelSelector, error) {
+func parsePodSelector(policyKey string, selector *metav1.LabelSelector) ([]labelSelector, error) {
 	parsedSelectors := newParsedSelectors()
 
 	// #1. MatchLabels
@@ -257,7 +259,8 @@ func parsePodSelector(selector *metav1.LabelSelector) ([]labelSelector, error) {
 				setType = ipsets.KeyValueLabelOfPod
 			} else {
 				// "(!) + matchKey + : + multiple matchVals" case
-				setName = req.Key
+				// see caveat in definition of TranslatedIPSet for why the policy key must be included in the set name
+				setName = fmt.Sprintf("%s-%s", policyKey, req.Key)
 				for _, val := range req.Values {
 					setName = util.GetIpSetFromLabelKV(setName, val)
 					members = append(members, util.GetIpSetFromLabelKV(req.Key, val))

npm/pkg/controlplane/translation/translatePolicy.go

@@ -216,8 +216,8 @@ func ipBlockRule(policyName, ns string, direction policies.Direction, matchType
 
 // PodSelector translates podSelector of NetworkPolicyPeer field in networkpolicy object to translatedIPSet and SetInfo.
 // This function is called only when the NetworkPolicyPeer has namespaceSelector field.
-func podSelector(matchType policies.MatchType, selector *metav1.LabelSelector) ([]*ipsets.TranslatedIPSet, []policies.SetInfo, error) {
-	podSelectors, err := parsePodSelector(selector)
+func podSelector(policyKey string, matchType policies.MatchType, selector *metav1.LabelSelector) ([]*ipsets.TranslatedIPSet, []policies.SetInfo, error) {
+	podSelectors, err := parsePodSelector(policyKey, selector)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -241,8 +241,8 @@ func podSelector(matchType policies.MatchType, selector *metav1.LabelSelector) (
 
 // podSelectorWithNS translates podSelector of spec and NetworkPolicyPeer in networkpolicy object to translatedIPSet and SetInfo.
 // This function is called only when the NetworkPolicyPeer does not have namespaceSelector field.
-func podSelectorWithNS(ns string, matchType policies.MatchType, selector *metav1.LabelSelector) ([]*ipsets.TranslatedIPSet, []policies.SetInfo, error) {
-	podSelectorIPSets, podSelectorList, err := podSelector(matchType, selector)
+func podSelectorWithNS(policyKey, ns string, matchType policies.MatchType, selector *metav1.LabelSelector) ([]*ipsets.TranslatedIPSet, []policies.SetInfo, error) {
+	podSelectorIPSets, podSelectorList, err := podSelector(policyKey, matchType, selector)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -397,7 +397,7 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire
 
 		// #2.3 handle podSelector and port if exist
 		if peer.PodSelector != nil && peer.NamespaceSelector == nil {
-			podSelectorIPSets, podSelectorList, err := podSelectorWithNS(npmNetPol.Namespace, matchType, peer.PodSelector)
+			podSelectorIPSets, podSelectorList, err := podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, matchType, peer.PodSelector)
 			if err != nil {
 				return err
 			}
@@ -418,7 +418,7 @@ func translateRule(npmNetPol *policies.NPMNetworkPolicy, netPolName string, dire
 		}
 
 		// #2.4 handle namespaceSelector and podSelector and port if exist
-		podSelectorIPSets, podSelectorList, err := podSelector(matchType, peer.PodSelector)
+		podSelectorIPSets, podSelectorList, err := podSelector(npmNetPol.PolicyKey, matchType, peer.PodSelector)
 		if err != nil {
 			return err
 		}
@@ -542,7 +542,7 @@ func TranslatePolicy(npObj *networkingv1.NetworkPolicy) (*policies.NPMNetworkPol
 	// podSelector in spec.PodSelector is common for ingress and egress.
 	// Process this podSelector first.
 	var err error
-	npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.Namespace, policies.EitherMatch, &npObj.Spec.PodSelector)
+	npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, policies.EitherMatch, &npObj.Spec.PodSelector)
 	if err != nil {
 		return nil, err
 	}

npm/pkg/controlplane/translation/translatePolicy_test.go

@@ -536,6 +536,8 @@ func TestIPBlockRule(t *testing.T) {
 
 func TestPodSelector(t *testing.T) {
 	matchType := policies.DstMatch
+	policyKey := "test-ns/test-policy"
+	policyKeyWithDash := policyKey + "-"
 	tests := []struct {
 		name              string
 		namespace         string
@@ -687,14 +689,14 @@ func TestPodSelector(t *testing.T) {
 			},
 			podSelectorIPSets: []*ipsets.TranslatedIPSet{
 				ipsets.NewTranslatedIPSet("k0:v0", ipsets.KeyValueLabelOfPod),
-				ipsets.NewTranslatedIPSet("k1:v10:v11", ipsets.NestedLabelOfPod, []string{"k1:v10", "k1:v11"}...),
+				ipsets.NewTranslatedIPSet(policyKeyWithDash+"k1:v10:v11", ipsets.NestedLabelOfPod, []string{"k1:v10", "k1:v11"}...),
 				ipsets.NewTranslatedIPSet("k1:v10", ipsets.KeyValueLabelOfPod),
 				ipsets.NewTranslatedIPSet("k1:v11", ipsets.KeyValueLabelOfPod),
 				ipsets.NewTranslatedIPSet("k2", ipsets.KeyLabelOfPod),
 			},
 			podSelectorList: []policies.SetInfo{
 				policies.NewSetInfo("k0:v0", ipsets.KeyValueLabelOfPod, included, matchType),
-				policies.NewSetInfo("k1:v10:v11", ipsets.NestedLabelOfPod, included, matchType),
+				policies.NewSetInfo(policyKeyWithDash+"k1:v10:v11", ipsets.NestedLabelOfPod, included, matchType),
 				policies.NewSetInfo("k2", ipsets.KeyLabelOfPod, nonIncluded, matchType),
 			},
 		},
@@ -708,9 +710,10 @@ func TestPodSelector(t *testing.T) {
 			var podSelectorList []policies.SetInfo
 			var err error
 			if tt.namespace == "" {
-				podSelectorIPSets, podSelectorList, err = podSelector(tt.matchType, tt.labelSelector)
+				podSelectorIPSets, podSelectorList, err = podSelector(policyKey, tt.matchType, tt.labelSelector)
 			} else {
-				podSelectorIPSets, podSelectorList, err = podSelectorWithNS(tt.namespace, tt.matchType, tt.labelSelector)
+				// technically, the policyKey prefix would contain the namespace, but it might not for these tests
+				podSelectorIPSets, podSelectorList, err = podSelectorWithNS(policyKey, tt.namespace, tt.matchType, tt.labelSelector)
 			}
 			require.NoError(t, err)
 			require.Equal(t, tt.podSelectorIPSets, podSelectorIPSets)
@@ -1668,7 +1671,7 @@ func TestIngressPolicy(t *testing.T) {
 				ACLPolicyID: tt.npmNetPol.ACLPolicyID,
 			}
 			var err error
-			npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.Namespace, policies.EitherMatch, tt.targetSelector)
+			npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, policies.EitherMatch, tt.targetSelector)
 			require.NoError(t, err)
 			splitPolicyKey := strings.Split(npmNetPol.PolicyKey, "/")
 			require.Len(t, splitPolicyKey, 2, "policy key must include name")
@@ -2060,7 +2063,7 @@ func TestEgressPolicy(t *testing.T) {
 				ACLPolicyID: tt.npmNetPol.ACLPolicyID,
 			}
 			var err error
-			npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.Namespace, policies.EitherMatch, tt.targetSelector)
+			npmNetPol.PodSelectorIPSets, npmNetPol.PodSelectorList, err = podSelectorWithNS(npmNetPol.PolicyKey, npmNetPol.Namespace, policies.EitherMatch, tt.targetSelector)
 			require.NoError(t, err)
 			splitPolicyKey := strings.Split(npmNetPol.PolicyKey, "/")
 			require.Len(t, splitPolicyKey, 2, "policy key must include name")

npm/pkg/dataplane/dataplane.go

@@ -41,11 +41,12 @@ type DataPlane struct {
 	stopChannel    <-chan struct{}
 }
 
+// TODO this struct could be made unexported
 type NPMEndpoint struct {
-	Name string
-	ID   string
-	IP   string
-	// TODO: check it may use PolicyKey instead of Policy name
+	Name   string
+	ID     string
+	IP     string
+	PodKey string
 	// Map with Key as Network Policy name to to emulate set
 	// and value as struct{} for minimal memory consumption
 	NetPolReference map[string]struct{}
@@ -73,7 +74,6 @@ func NewDataPlane(nodeName string, ioShim *common.IOShim, cfg *Config, stopChann
 }
 
 // BootupDataplane cleans the NPM sets and policies in the dataplane and performs initialization.
-// TODO rename this function to BootupDataplane
 func (dp *DataPlane) BootupDataplane() error {
 	// NOTE: used to create an all-namespaces set, but there's no need since it will be created by the control plane
 	return dp.bootupDataPlane() //nolint:wrapcheck // unnecessary to wrap error
@@ -225,7 +225,6 @@ func (dp *DataPlane) AddPolicy(policy *policies.NPMNetworkPolicy) error {
 	if err != nil {
 		return fmt.Errorf("[DataPlane] error while applying dataplane: %w", err)
 	}
-	// TODO calculate endpoints to apply policy on
 	endpointList, err := dp.getEndpointsToApplyPolicy(policy)
 	if err != nil {
 		return err
@@ -321,8 +320,6 @@ func (dp *DataPlane) createIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n
 		}
 	}
 
-	// TODO is there a possibility for a list set of selector referencing rule ipset?
-	// if so this below addition would throw an error because rule ipsets are not created
 	// Check if any list sets are provided with members to add
 	for _, set := range sets {
 		// Check if any CIDR block IPSets needs to be applied
@@ -362,11 +359,9 @@ func (dp *DataPlane) deleteIPSetsAndReferences(sets []*ipsets.TranslatedIPSet, n
 		}
 	}
 
-	// Check if any list sets are provided with members to add
-	// TODO for nested IPsets check if we are safe to remove members
-	// if k1:v0:v1 is created by two network policies
-	// and both have same members
-	// then we should not delete k1:v0:v1 members ( special case for nested ipsets )
+	// Check if any list sets are provided with members to delete
+	// NOTE: every translated member will be deleted, even if the member is part of the same set in another policy
+	// see the definition of TranslatedIPSet for how to avoid this situation
 	for _, set := range sets {
 		// Check if any CIDR block IPSets needs to be applied
 		setType := set.Metadata.Type

npm/pkg/dataplane/ipsets/ipset.go

@@ -105,6 +105,10 @@ func (setType SetType) getSetKind() SetKind {
 // 2. NestedLabelOfPod IPSet from multi value labels
 // Members field holds member ipset names for NestedLabelOfPod and ip address ranges
 // for CIDRBlocks IPSet
+// Caveat: if a list set with translated members is referenced in multiple policies,
+// then it must have a different ipset name for each policy. Otherwise, deleting the policy
+// will result in removing the translated members from the set even if another policy requires
+// those members. See dataplane.go for more details.
 type TranslatedIPSet struct {
 	Metadata *IPSetMetadata
 	// Members holds member ipset names for NestedLabelOfPod and ip address ranges