Address more comments

Signed-off-by: Dyanngg <dingyang@vmware.com>

multicluster/test/e2e/antreapolicy_test.go

@@ -54,8 +54,7 @@ func failOnError(err error, t *testing.T) {
 func initializeForPolicyTest(t *testing.T, data *MCTestData) {
 	perNamespacePods = []string{"a", "b", "c"}
 	perClusterNamespaces = make(map[string]antreae2e.TestNamespaceMeta)
-	nss := []string{"x", "y", "z"}
-	for _, ns := range nss {
+	for _, ns := range []string{"x", "y", "z"} {
 		perClusterNamespaces[ns] = antreae2e.TestNamespaceMeta{Name: ns}
 	}
 

pkg/controller/networkpolicy/clusternetworkpolicy.go

@@ -16,6 +16,7 @@ package networkpolicy
 
 import (
 	"reflect"
+	"sort"
 	"strings"
 
 	v1 "k8s.io/api/core/v1"
@@ -631,7 +632,9 @@ func groupNamespacesByLabelValue(affectedNSAndLabels map[string]labels.Set, labe
 func getLabelValues(labels map[string]string, labelKeys []string) string {
 	key := ""
 	for _, k := range labelKeys {
-		if v, ok := labels[k]; ok {
+		if v, ok := labels[k]; !ok {
+			return ""
+		} else {
 			key += v + labelValueSeparator
 		}
 	}
@@ -682,6 +685,7 @@ func (n *NetworkPolicyController) toAntreaPeerForSameLabelNamespaces(peer crdv1b
 		LabelIdentities: labelIdentities,
 	}
 	var atgs []*antreatypes.AppliedToGroup
+	sort.Strings(namespacesByLabelValues)
 	for _, ns := range namespacesByLabelValues {
 		atgForNamespace, _ := atgPerAffectedNS[ns]
 		atgs = append(atgs, atgForNamespace)

pkg/controller/networkpolicy/clusternetworkpolicy_test.go

@@ -17,8 +17,6 @@ package networkpolicy
 import (
 	"fmt"
 	"net"
-	"reflect"
-	"sort"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -36,48 +34,6 @@ import (
 	"antrea.io/antrea/pkg/util/k8s"
 )
 
-// ruleSemanticallyEqual compares two NetworkPolicyRule objects. It disregards
-// the appliedToGroup slice element order as long as two rules' appliedToGroups
-// have same elements.
-func ruleSemanticallyEqual(a, b controlplane.NetworkPolicyRule) bool {
-	sort.Strings(a.AppliedToGroups)
-	sort.Strings(b.AppliedToGroups)
-	return reflect.DeepEqual(a, b)
-}
-
-// diffNetworkPolicyRuleList checks if elements in two controlplane.NetworkPolicyRule
-// slices are equal. If not, it returns the unmatched NetworkPolicyRules.
-func diffNetworkPolicyRuleList(a, b []controlplane.NetworkPolicyRule) (extraA, extraB []controlplane.NetworkPolicyRule) {
-	if len(a) != len(b) {
-		return nil, nil
-	}
-	// Mark indexes in b that has already matched
-	visited := make([]bool, len(b))
-	for i := 0; i < len(a); i++ {
-		found := false
-		for j := 0; j < len(b); j++ {
-			if visited[j] {
-				continue
-			}
-			if ruleSemanticallyEqual(a[i], b[j]) {
-				visited[j] = true
-				found = true
-				break
-			}
-		}
-		if !found {
-			extraA = append(extraA, a[i])
-		}
-	}
-	for j := 0; j < len(b); j++ {
-		if visited[j] {
-			continue
-		}
-		extraB = append(extraB, b[j])
-	}
-	return
-}
-
 func TestProcessClusterNetworkPolicy(t *testing.T) {
 	p10 := float64(10)
 	t10 := int32(10)
@@ -1052,8 +1008,8 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 					{
 						Direction: controlplane.DirectionIn,
 						AppliedToGroups: []string{
-							getNormalizedUID(antreatypes.NewGroupSelector("nsC", nil, nil, nil, nil).NormalizedName),
 							getNormalizedUID(antreatypes.NewGroupSelector("nsB", nil, nil, nil, nil).NormalizedName),
+							getNormalizedUID(antreatypes.NewGroupSelector("nsC", nil, nil, nil, nil).NormalizedName),
 						},
 						From: controlplane.NetworkPolicyPeer{
 							AddressGroups: []string{
@@ -1947,10 +1903,7 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 			assert.Equal(t, tt.expectedPolicy.Priority, actualPolicy.Priority)
 			assert.Equal(t, tt.expectedPolicy.TierPriority, actualPolicy.TierPriority)
 			assert.Equal(t, tt.expectedPolicy.AppliedToPerRule, actualPolicy.AppliedToPerRule)
-			missingExpectedRules, extraActualRules := diffNetworkPolicyRuleList(tt.expectedPolicy.Rules, actualPolicy.Rules)
-			if len(missingExpectedRules) > 0 || len(extraActualRules) > 0 {
-				t.Errorf("Unexpected rules in processed policy. Missing expected rules: %v. Extra actual rules: %v", missingExpectedRules, extraActualRules)
-			}
+			assert.ElementsMatch(t, tt.expectedPolicy.Rules, actualPolicy.Rules)
 			assert.ElementsMatch(t, tt.expectedPolicy.AppliedToGroups, actualPolicy.AppliedToGroups)
 			assert.Equal(t, tt.expectedAppliedToGroups, len(actualAppliedToGroups))
 			assert.Equal(t, tt.expectedAddressGroups, len(actualAddressGroups))

test/e2e/antreapolicy_test.go

@@ -49,7 +49,7 @@ var (
 	podsByNamespace                             map[string][]Pod
 	k8sUtils                                    *KubernetesUtils
 	allTestList                                 []*TestCase
-	pods                                        []string
+	podsPerNamespace                            []string
 	namespaces                                  map[string]TestNamespaceMeta
 	podIPs                                      map[string][]string
 	p80, p81, p8080, p8081, p8082, p8085, p6443 int32
@@ -66,11 +66,9 @@ const (
 	// Verification of deleting/creating resources timed out.
 	timeout = 10 * time.Second
 	// audit log directory on Antrea Agent
-	logDir           = "/var/log/antrea/networkpolicy/"
-	logfileName      = "np.log"
-	defaultTierName  = "application"
-	formFactorNormal = "3by3PodWorkloads"
-	formFactorLarge  = "extraNamespaces"
+	logDir          = "/var/log/antrea/networkpolicy/"
+	logfileName     = "np.log"
+	defaultTierName = "application"
 )
 
 func failOnError(err error, t *testing.T) {
@@ -105,56 +103,7 @@ func getPodName(ns, po string) string {
 	return namespaces[ns].Name + "/" + po
 }
 
-// initNamespaceMeta populates the test Namespaces metadata.
-// There are two form factors for test workload Namespaces:
-//
-//	Normal: three Namespaces x, y, z.
-//	Large: two "prod" Namespaces labeled purpose=test and tier=prod.
-//	       two "dev" Namespaces labeled purpose=test and tier=dev.
-//	       one "no-tier-label" Namespace labeled purpose=test.
-//
-// The large form factor workloads are used for testcases where advanced
-// Namespace matching in policies are required.
-func initNamespaceMeta(formFactor string) map[string]TestNamespaceMeta {
-	allNamespaceMeta := make(map[string]TestNamespaceMeta)
-	suffix := randName("")
-	if formFactor == formFactorLarge {
-		for i := 1; i < 3; i++ {
-			prodNS := TestNamespaceMeta{
-				Name: "prod" + strconv.Itoa(i) + "-" + suffix,
-				Labels: map[string]string{
-					"purpose": "test",
-					"tier":    "prod",
-				},
-			}
-			allNamespaceMeta["prod"+strconv.Itoa(i)] = prodNS
-			devNS := TestNamespaceMeta{
-				Name: "dev" + strconv.Itoa(i) + "-" + suffix,
-				Labels: map[string]string{
-					"purpose": "test",
-					"tier":    "dev",
-				},
-			}
-			allNamespaceMeta["dev"+strconv.Itoa(i)] = devNS
-		}
-		allNamespaceMeta["no-tier"] = TestNamespaceMeta{
-			Name: "no-tier-" + suffix,
-			Labels: map[string]string{
-				"purpose": "test-exclusion",
-			},
-		}
-	} else if formFactor == formFactorNormal {
-		nss := []string{"x", "y", "z"}
-		for _, ns := range nss {
-			allNamespaceMeta[ns] = TestNamespaceMeta{
-				Name: ns + "-" + suffix,
-			}
-		}
-	}
-	return allNamespaceMeta
-}
-
-func initialize(t *testing.T, data *TestData, formFactor string) {
+func initialize(t *testing.T, data *TestData, customNamespaces map[string]TestNamespaceMeta) {
 	p80 = 80
 	p81 = 81
 	p8080 = 8080
@@ -164,15 +113,24 @@ func initialize(t *testing.T, data *TestData, formFactor string) {
 	selfNamespace = &crdv1beta1.PeerNamespaces{
 		Match: crdv1beta1.NamespaceMatchSelf,
 	}
-	pods = []string{"a", "b", "c"}
-	namespaces = initNamespaceMeta(formFactor)
+	namespaces = make(map[string]TestNamespaceMeta)
+	if customNamespaces != nil {
+		namespaces = customNamespaces
+	} else {
+		suffix := randName("")
+		for _, ns := range []string{"x", "y", "z"} {
+			namespaces[ns] = TestNamespaceMeta{
+				Name: ns + "-" + suffix,
+			}
+		}
+	}
 	// This function "initialize" will be used more than once, and variable "allPods" is global.
 	// It should be empty every time when "initialize" is performed, otherwise there will be unexpected
 	// results.
 	allPods = []Pod{}
 	podsByNamespace = make(map[string][]Pod)
-
-	for _, podName := range pods {
+	podsPerNamespace = []string{"a", "b", "c"}
+	for _, podName := range podsPerNamespace {
 		for _, ns := range namespaces {
 			allPods = append(allPods, NewPod(ns.Name, podName))
 			podsByNamespace[ns.Name] = append(podsByNamespace[ns.Name], NewPod(ns.Name, podName))
@@ -184,7 +142,7 @@ func initialize(t *testing.T, data *TestData, formFactor string) {
 	// k8sUtils is a global var
 	k8sUtils, err = NewKubernetesUtils(data)
 	failOnError(err, t)
-	ips, err := k8sUtils.Bootstrap(namespaces, pods, true, nil, nil)
+	ips, err := k8sUtils.Bootstrap(namespaces, podsPerNamespace, true, nil, nil)
 	failOnError(err, t)
 	podIPs = ips
 }
@@ -4373,7 +4331,7 @@ func TestAntreaPolicy(t *testing.T) {
 	}
 	defer teardownTest(t, data)
 
-	initialize(t, data, formFactorNormal)
+	initialize(t, data, nil)
 
 	// This test group only provides one case for each CR, including ACNP, ANNP, Tier,
 	// ClusterGroup and Group to make sure the corresponding validation webhooks is
@@ -4514,7 +4472,36 @@ func TestAntreaPolicyExtendedNamespaces(t *testing.T) {
 	}
 	defer teardownTest(t, data)
 
-	initialize(t, data, formFactorLarge)
+	extendedNamespaces := make(map[string]TestNamespaceMeta)
+	suffix := randName("")
+	// two "prod" Namespaces labeled purpose=test and tier=prod.
+	// two "dev" Namespaces labeled purpose=test and tier=dev.
+	// one "no-tier-label" Namespace labeled purpose=test.
+	for i := 1; i <= 2; i++ {
+		prodNS := TestNamespaceMeta{
+			Name: "prod" + strconv.Itoa(i) + "-" + suffix,
+			Labels: map[string]string{
+				"purpose": "test",
+				"tier":    "prod",
+			},
+		}
+		extendedNamespaces["prod"+strconv.Itoa(i)] = prodNS
+		devNS := TestNamespaceMeta{
+			Name: "dev" + strconv.Itoa(i) + "-" + suffix,
+			Labels: map[string]string{
+				"purpose": "test",
+				"tier":    "dev",
+			},
+		}
+		extendedNamespaces["dev"+strconv.Itoa(i)] = devNS
+	}
+	extendedNamespaces["no-tier"] = TestNamespaceMeta{
+		Name: "no-tier-" + suffix,
+		Labels: map[string]string{
+			"purpose": "test-exclusion",
+		},
+	}
+	initialize(t, data, extendedNamespaces)
 
 	t.Run("TestGroupACNPNamespaceLabelSelections", func(t *testing.T) {
 		t.Run("Case=ACNPStrictNamespacesIsolationByLabels", func(t *testing.T) { testACNPStrictNamespacesIsolationByLabels(t) })
@@ -4658,7 +4645,7 @@ func TestAntreaPolicyStatusWithAppliedToUnsupportedGroup(t *testing.T) {
 	}
 	defer teardownTest(t, data)
 
-	initialize(t, data, formFactorNormal)
+	initialize(t, data, nil)
 
 	testNamespace := getNS("x")
 	// Build a Group with namespaceSelector selecting namespaces outside testNamespace.

test/e2e/clustergroup_test.go

@@ -320,7 +320,7 @@ func TestClusterGroup(t *testing.T) {
 	}
 	defer teardownTest(t, data)
 
-	initialize(t, data, formFactorNormal)
+	initialize(t, data, nil)
 
 	t.Run("TestGroupClusterGroupValidate", func(t *testing.T) {
 		t.Run("Case=IPBlockWithPodSelectorDenied", func(t *testing.T) { testInvalidCGIPBlockWithPodSelector(t) })

test/e2e/group_test.go

@@ -263,7 +263,7 @@ func TestGroup(t *testing.T) {
 		t.Fatalf("Error when setting up test: %v", err)
 	}
 	defer teardownTest(t, data)
-	initialize(t, data, formFactorNormal)
+	initialize(t, data, nil)
 
 	t.Run("TestGroupNamespacedGroupValidate", func(t *testing.T) {
 		t.Run("Case=IPBlockWithPodSelectorDenied", func(t *testing.T) { testInvalidGroupIPBlockWithPodSelector(t) })

test/e2e/k8s_util.go

@@ -1138,16 +1138,15 @@ func (k *KubernetesUtils) ValidateRemoteCluster(remoteCluster *KubernetesUtils,
 	}
 }
 
-func (k *KubernetesUtils) Bootstrap(namespaces map[string]TestNamespaceMeta, pods []string, createNamespaces bool, nodeNames map[string]string, hostNetworks map[string]bool) (map[string][]string, error) {
+func (k *KubernetesUtils) Bootstrap(namespaces map[string]TestNamespaceMeta, podsPerNamespace []string, createNamespaces bool, nodeNames map[string]string, hostNetworks map[string]bool) (map[string][]string, error) {
 	for key, ns := range namespaces {
 		if createNamespaces {
 			if ns.Labels == nil {
 				ns.Labels = make(map[string]string)
 			}
 			// convenience label for testing
 			ns.Labels["ns"] = ns.Name
-			_, err := k.CreateOrUpdateNamespace(ns.Name, ns.Labels)
-			if err != nil {
+			if _, err := k.CreateOrUpdateNamespace(ns.Name, ns.Labels); err != nil {
 				return nil, fmt.Errorf("unable to create/update ns %s: %w", ns, err)
 			}
 		}
@@ -1159,7 +1158,7 @@ func (k *KubernetesUtils) Bootstrap(namespaces map[string]TestNamespaceMeta, pod
 		if hostNetworks != nil {
 			hostNetwork = hostNetworks[key]
 		}
-		for _, pod := range pods {
+		for _, pod := range podsPerNamespace {
 			log.Infof("Creating/updating Pod '%s/%s'", ns, pod)
 			deployment := ns.Name + pod
 			_, err := k.CreateOrUpdateDeployment(ns.Name, deployment, 1, map[string]string{"pod": pod, "app": pod}, nodeName, hostNetwork)
@@ -1169,8 +1168,8 @@ func (k *KubernetesUtils) Bootstrap(namespaces map[string]TestNamespaceMeta, pod
 		}
 	}
 	var allPods []Pod
-	podIPs := make(map[string][]string, len(pods)*len(namespaces))
-	for _, podName := range pods {
+	podIPs := make(map[string][]string, len(podsPerNamespace)*len(namespaces))
+	for _, podName := range podsPerNamespace {
 		for _, ns := range namespaces {
 			allPods = append(allPods, NewPod(ns.Name, podName))
 		}

test/e2e/nodenetworkpolicy_test.go

@@ -37,7 +37,7 @@ func initializeAntreaNodeNetworkPolicy(t *testing.T, data *TestData, toHostNetwo
 	p8082 = 8082
 	p8085 = 8085
 	pods = []string{"a"}
-	namespaces = initNamespaceMeta(formFactorNormal)
+	namespaces = initNamespaceMeta()
 	nodes = make(map[string]string)
 	nodes["x"] = controlPlaneNodeName()
 	nodes["y"] = workerNodeName(1)