Fix policy not being cleaned if a policy with the same name exist (#4986

)

We need to check if the UID matches when checking if a policy exists,
because it's possible another policy is created with the same name after
the policy is deleted. Otherwise the deleted policy would still be
enforced.

Signed-off-by: Quan Tian <qtian@vmware.com>

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -1425,21 +1425,24 @@ func (n *NetworkPolicyController) syncInternalNetworkPolicy(key *controlplane.Ne
 	switch key.Type {
 	case controlplane.AntreaClusterNetworkPolicy:
 		cnp, err := n.cnpLister.Get(key.Name)
-		if err != nil {
+		// We need to check if the UID matches because it's possible another policy is created with the same name after
+		// the policy is deleted. It's safe to just delete the internal NetworkPolicy associated with the old policy as
+		// the two policies are different items in the workqueue and internalNetworkPolicyStore due to different UIDs.
+		if err != nil || cnp.UID != key.UID {
 			n.deleteInternalNetworkPolicy(internalNetworkPolicyName)
 			return nil
 		}
 		newInternalNetworkPolicy, newAppliedToGroups, newAddressGroups = n.processClusterNetworkPolicy(cnp)
 	case controlplane.AntreaNetworkPolicy:
 		anp, err := n.anpLister.NetworkPolicies(key.Namespace).Get(key.Name)
-		if err != nil {
+		if err != nil || anp.UID != key.UID {
 			n.deleteInternalNetworkPolicy(internalNetworkPolicyName)
 			return nil
 		}
 		newInternalNetworkPolicy, newAppliedToGroups, newAddressGroups = n.processAntreaNetworkPolicy(anp)
 	case controlplane.K8sNetworkPolicy:
 		knp, err := n.networkPolicyLister.NetworkPolicies(key.Namespace).Get(key.Name)
-		if err != nil {
+		if err != nil || knp.UID != key.UID {
 			n.deleteInternalNetworkPolicy(internalNetworkPolicyName)
 			return nil
 		}

pkg/controller/networkpolicy/networkpolicy_controller_test.go

@@ -3046,6 +3046,92 @@ func TestSyncInternalNetworkPolicy(t *testing.T) {
 	checkGroupItemExistence(t, c.appliedToGroupStore)
 }
 
+// TestSyncInternalNetworkPolicyWithSameName verifies SyncInternalNetworkPolicy can work correctly when processing
+// multiple NetworkPolicies that have the same name.
+func TestSyncInternalNetworkPolicyWithSameName(t *testing.T) {
+	// policyA and policyB have the same name but different UIDs.
+	policyA := &networkingv1.NetworkPolicy{
+		ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "foo", UID: "uidA"},
+		Spec: networkingv1.NetworkPolicySpec{
+			PodSelector: selectorA,
+		},
+	}
+	policyB := &networkingv1.NetworkPolicy{
+		ObjectMeta: metav1.ObjectMeta{Namespace: "default", Name: "foo", UID: "uidB"},
+		Spec: networkingv1.NetworkPolicySpec{
+			PodSelector: selectorB,
+		},
+	}
+
+	selectorAGroup := getNormalizedUID(antreatypes.NewGroupSelector("default", &selectorA, nil, nil, nil).NormalizedName)
+	selectorBGroup := getNormalizedUID(antreatypes.NewGroupSelector("default", &selectorB, nil, nil, nil).NormalizedName)
+	expectedPolicyA := &antreatypes.NetworkPolicy{
+		UID:      "uidA",
+		Name:     "uidA",
+		SpanMeta: antreatypes.SpanMeta{NodeNames: sets.NewString()},
+		SourceRef: &controlplane.NetworkPolicyReference{
+			Type:      controlplane.K8sNetworkPolicy,
+			Namespace: "default",
+			Name:      "foo",
+			UID:       "uidA",
+		},
+		AppliedToGroups: []string{selectorAGroup},
+		Rules:           []controlplane.NetworkPolicyRule{},
+	}
+	expectedPolicyB := &antreatypes.NetworkPolicy{
+		UID:      "uidB",
+		Name:     "uidB",
+		SpanMeta: antreatypes.SpanMeta{NodeNames: sets.NewString()},
+		SourceRef: &controlplane.NetworkPolicyReference{
+			Type:      controlplane.K8sNetworkPolicy,
+			Namespace: "default",
+			Name:      "foo",
+			UID:       "uidB",
+		},
+		AppliedToGroups: []string{selectorBGroup},
+		Rules:           []controlplane.NetworkPolicyRule{},
+	}
+
+	// Add and sync policyA first, it should create an AppliedToGroup.
+	_, c := newController()
+	c.networkPolicyStore.Add(policyA)
+	networkPolicyRefA := getKNPReference(policyA)
+	assert.NoError(t, c.syncInternalNetworkPolicy(networkPolicyRefA))
+	obj, exists, _ := c.internalNetworkPolicyStore.Get(expectedPolicyA.Name)
+	require.True(t, exists)
+	assert.Equal(t, expectedPolicyA, obj.(*antreatypes.NetworkPolicy))
+	checkGroupItemExistence(t, c.appliedToGroupStore, selectorAGroup)
+
+	// Delete policyA and add policyB, then sync them concurrently, the resources associated with policyA should be deleted.
+	c.networkPolicyStore.Delete(policyA)
+	c.networkPolicyStore.Add(policyB)
+	networkPolicyRefB := getKNPReference(policyB)
+	var wg sync.WaitGroup
+	wg.Add(2)
+	go func() {
+		defer wg.Done()
+		assert.NoError(t, c.syncInternalNetworkPolicy(networkPolicyRefA))
+	}()
+	go func() {
+		defer wg.Done()
+		assert.NoError(t, c.syncInternalNetworkPolicy(networkPolicyRefB))
+	}()
+	wg.Wait()
+	_, exists, _ = c.internalNetworkPolicyStore.Get(expectedPolicyA.Name)
+	require.False(t, exists)
+	obj, exists, _ = c.internalNetworkPolicyStore.Get(expectedPolicyB.Name)
+	require.True(t, exists)
+	assert.Equal(t, expectedPolicyB, obj.(*antreatypes.NetworkPolicy))
+	checkGroupItemExistence(t, c.appliedToGroupStore, selectorBGroup)
+
+	// Delete policyB and sync it, the resources associated with policyB should be deleted.
+	c.networkPolicyStore.Delete(policyB)
+	assert.NoError(t, c.syncInternalNetworkPolicy(networkPolicyRefB))
+	_, exists, _ = c.internalNetworkPolicyStore.Get(expectedPolicyB.Name)
+	require.False(t, exists)
+	checkGroupItemExistence(t, c.appliedToGroupStore)
+}
+
 // TestSyncInternalNetworkPolicyConcurrently verifies SyncInternalNetworkPolicy can create and delete AppliedToGroups
 // and AddressGroups correctly when concurrently processing multiple NetworkPolicies that refer to the same groups.
 func TestSyncInternalNetworkPolicyConcurrently(t *testing.T) {