fix: [WIN-NPM] don't apply policy to new pod with same IP as an old pod (#1569)

* don't apply policy to IP's old pod owner

* fix podkey/ip check

* test case for pod key check

* clarify error message

* fix windows lint and have complete UT coverage of isIPAffiliated

* comment out flaky UT for now

Author: huntergregory

npm/pkg/dataplane/dataplane-test-cases_windows_test.go

@@ -370,6 +370,40 @@ func getAllSerialTests() []*SerialTestCase {
 				},
 			},
 		},
+		{
+			Description: "Pod B replaces Pod A with same IP",
+			Actions: []*Action{
+				CreateEndpoint(endpoint1, ip1),
+				CreatePod("x", "a", ip1, thisNode, map[string]string{"k1": "v1"}),
+				ApplyDP(),
+				DeleteEndpoint(endpoint1),
+				CreateEndpoint(endpoint2, ip1),
+				// in case CreatePod("x", "b", ...) is somehow processed before DeletePod("x", "a", ...)
+				CreatePod("x", "b", ip1, thisNode, map[string]string{"k2": "v2"}),
+				// policy should not be applied to x/b since ipset manager should not consider pod x/b part of k1:v1 selector ipsets
+				UpdatePolicy(policyXBaseOnK1V1()),
+			},
+			TestCaseMetadata: &TestCaseMetadata{
+				Tags: []Tag{
+					podCrudTag,
+					netpolCrudTag,
+				},
+				DpCfg:            defaultWindowsDPCfg,
+				InitialEndpoints: nil,
+				ExpectedSetPolicies: []*hcn.SetPolicySetting{
+					dptestutils.SetPolicy(emptySet),
+					dptestutils.SetPolicy(allNamespaces, emptySet.GetHashedName(), nsXSet.GetHashedName()),
+					dptestutils.SetPolicy(nsXSet, ip1),
+					dptestutils.SetPolicy(podK1Set, ip1),
+					dptestutils.SetPolicy(podK1V1Set, ip1),
+					dptestutils.SetPolicy(podK2Set, ip1),
+					dptestutils.SetPolicy(podK2V2Set, ip1),
+				},
+				ExpectedEnpdointACLs: map[string][]*hnswrapper.FakeEndpointPolicy{
+					endpoint2: {},
+				},
+			},
+		},
 	}
 }
 

npm/pkg/dataplane/dataplane_windows.go

@@ -219,7 +219,7 @@ func (dp *DataPlane) updatePod(pod *updateNPMPod) error {
 		}
 
 		selectorIPSets := dp.getSelectorIPSets(policy)
-		ok, err := dp.ipsetMgr.DoesIPSatisfySelectorIPSets(pod.PodIP, selectorIPSets)
+		ok, err := dp.ipsetMgr.DoesIPSatisfySelectorIPSets(pod.PodIP, pod.PodKey, selectorIPSets)
 		if err != nil {
 			return err
 		}
@@ -263,12 +263,21 @@ func (dp *DataPlane) getEndpointsToApplyPolicy(policy *policies.NPMNetworkPolicy
 	defer dp.endpointCache.Unlock()
 
 	endpointList := make(map[string]string)
-	for ip := range netpolSelectorIPs {
+	for ip, podKey := range netpolSelectorIPs {
 		endpoint, ok := dp.endpointCache.cache[ip]
 		if !ok {
 			klog.Infof("[DataPlane] Ignoring endpoint with IP %s since it was not found in the endpoint cache. This IP might not be in the HNS network", ip)
 			continue
 		}
+
+		if endpoint.podKey != podKey {
+			// in case the pod controller hasn't updated the dp yet that the IP's pod owner has changed
+			klog.Infof(
+				"[DataPlane] ignoring endpoint with IP %s since the pod keys are different. podKey: [%s], endpoint: [%+v], endpoint stale pod key: [%+v]",
+				ip, podKey, endpoint, endpoint.stalePodKey)
+			continue
+		}
+
 		endpointList[ip] = endpoint.id
 		endpoint.netPolReference[policy.PolicyKey] = struct{}{}
 	}

npm/pkg/dataplane/ipsets/ipset.go

@@ -388,24 +388,6 @@ func (set *IPSet) hasMember(memberName string) bool {
 	return isMember
 }
 
-// isIPAffiliated determines whether an IP belongs to the set or its member sets in the case of a list set.
-// This method and GetSetContents are good examples of how the ipset struct may have been better designed
-// as an interface with hash and list implementations. Not worth it to redesign though.
-func (set *IPSet) isIPAffiliated(ip string) bool {
-	if set.Kind == HashSet {
-		if _, ok := set.IPPodKey[ip]; ok {
-			return true
-		}
-	}
-	for _, memberSet := range set.MemberIPSets {
-		_, ok := memberSet.IPPodKey[ip]
-		if ok {
-			return true
-		}
-	}
-	return false
-}
-
 func (set *IPSet) canSetBeSelectorIPSet() bool {
 	return (set.Type == KeyLabelOfPod ||
 		set.Type == KeyValueLabelOfPod ||

npm/pkg/dataplane/ipsets/ipset_windows.go

@@ -0,0 +1,18 @@
+package ipsets
+
+// isIPAffiliated determines whether an PodIP belongs to the set or its member sets in the case of a list set.
+// This method and GetSetContents are good examples of how the ipset struct may have been better designed
+// as an interface with hash and list implementations. Not worth it to redesign though.
+func (set *IPSet) isIPAffiliated(ip, podKey string) bool {
+	if set.Kind == HashSet {
+		if key, ok := set.IPPodKey[ip]; ok && key == podKey {
+			return true
+		}
+	}
+	for _, memberSet := range set.MemberIPSets {
+		if key, ok := memberSet.IPPodKey[ip]; ok && key == podKey {
+			return true
+		}
+	}
+	return false
+}

npm/pkg/dataplane/ipsets/ipsetmanager_windows.go

@@ -28,7 +28,7 @@ type networkPolicyBuilder struct {
 	toDeleteSets map[string]*hcn.SetPolicySetting
 }
 
-func (iMgr *IPSetManager) DoesIPSatisfySelectorIPSets(ip string, setList map[string]struct{}) (bool, error) {
+func (iMgr *IPSetManager) DoesIPSatisfySelectorIPSets(ip, podKey string, setList map[string]struct{}) (bool, error) {
 	if len(setList) == 0 {
 		klog.Infof("[ipset manager] unexpectedly encountered empty selector list")
 		return true, nil
@@ -42,18 +42,19 @@ func (iMgr *IPSetManager) DoesIPSatisfySelectorIPSets(ip string, setList map[str
 
 	for setName := range setList {
 		set := iMgr.setMap[setName]
-		if !set.isIPAffiliated(ip) {
+		if !set.isIPAffiliated(ip, podKey) {
 			return false, nil
 		}
 	}
 
 	return true, nil
 }
 
-// GetIPsFromSelectorIPSets will take in a map of prefixedSetNames and return an intersection of IPs
-func (iMgr *IPSetManager) GetIPsFromSelectorIPSets(setList map[string]struct{}) (map[string]struct{}, error) {
+// GetIPsFromSelectorIPSets will take in a map of prefixedSetNames and return an intersection of IPs mapped to pod key
+func (iMgr *IPSetManager) GetIPsFromSelectorIPSets(setList map[string]struct{}) (map[string]string, error) {
+	ips := make(map[string]string)
 	if len(setList) == 0 {
-		return map[string]struct{}{}, nil
+		return ips, nil
 	}
 	iMgr.Lock()
 	defer iMgr.Unlock()
@@ -67,30 +68,29 @@ func (iMgr *IPSetManager) GetIPsFromSelectorIPSets(setList map[string]struct{})
 	// which is a hash set, and we favor hash sets for firstSet
 	var firstSet *IPSet
 	for setName := range setList {
-		set := iMgr.setMap[setName]
-		if set.Kind == HashSet || firstSet == nil {
+		firstSet = iMgr.setMap[setName]
+		if firstSet.Kind == HashSet {
 			// firstSet can be any set, but ideally is a hash set for efficiency (compare the branch for hash sets to the one for lists below)
-			firstSet = set
+			break
 		}
 	}
-	ips := make(map[string]struct{})
 	if firstSet.Kind == HashSet {
 		// include every IP in firstSet that is also affiliated with every other selector set
-		for ip := range firstSet.IPPodKey {
+		for ip, podKey := range firstSet.IPPodKey {
 			isAffiliated := true
 			for otherSetName := range setList {
 				if otherSetName == firstSet.Name {
 					continue
 				}
 				otherSet := iMgr.setMap[otherSetName]
-				if !otherSet.isIPAffiliated(ip) {
+				if !otherSet.isIPAffiliated(ip, podKey) {
 					isAffiliated = false
 					break
 				}
 			}
 
 			if isAffiliated {
-				ips[ip] = struct{}{}
+				ips[ip] = podKey
 			}
 		}
 	} else {
@@ -100,19 +100,27 @@ func (iMgr *IPSetManager) GetIPsFromSelectorIPSets(setList map[string]struct{})
 
 		// only loop over the unique affiliated IPs
 		for _, memberSet := range firstSet.MemberIPSets {
-			for ip := range memberSet.IPPodKey {
-				ips[ip] = struct{}{}
+			for ip, podKey := range memberSet.IPPodKey {
+				if oldKey, ok := ips[ip]; ok && oldKey != podKey {
+					// this could lead to unintentionally considering this Pod (Pod B) to be part of the selector set if:
+					// 1. Pod B has the same IP as a previous Pod A
+					// 2. Pod B create is somehow processed before Pod A delete
+					// 3. This method is called before Pod A delete
+					// again, this
+					klog.Warningf("[GetIPsFromSelectorIPSets] IP currently associated with two different pod keys. to ensure no issues occur with network policies, restart this ip: %s", ip)
+				}
+				ips[ip] = podKey
 			}
 		}
-		for ip := range ips {
+		for ip, podKey := range ips {
 			// identical to the hash set case
 			isAffiliated := true
 			for otherSetName := range setList {
 				if otherSetName == firstSet.Name {
 					continue
 				}
 				otherSet := iMgr.setMap[otherSetName]
-				if !otherSet.isIPAffiliated(ip) {
+				if !otherSet.isIPAffiliated(ip, podKey) {
 					isAffiliated = false
 					break
 				}