Address comments

pkg/agent/controller/networkpolicy/cache.go

@@ -531,7 +531,7 @@ func (c *ruleCache) DeleteAppliedToGroup(group *v1beta.AppliedToGroup) error {
 // toRule converts v1beta.NetworkPolicyRule to *rule.
 func toRule(r *v1beta.NetworkPolicyRule, policy *v1beta.NetworkPolicy, maxPriority int32) *rule {
 	appliedToGroups := policy.AppliedToGroups
-	if policy.AppliedToPerRule {
+	if len(r.AppliedToGroups) != 0 {
 		appliedToGroups = r.AppliedToGroups
 	}
 	rule := &rule{

pkg/agent/controller/networkpolicy/cache_test.go

@@ -628,9 +628,8 @@ func TestRuleCacheAddNetworkPolicy(t *testing.T) {
 		},
 	}
 	networkPolicy3 := &v1beta2.NetworkPolicy{
-		ObjectMeta:      metav1.ObjectMeta{UID: "policy3", Namespace: "ns3", Name: "name3"},
-		Rules:           []v1beta2.NetworkPolicyRule{*networkPolicyRule3},
-		AppliedToGroups: nil,
+		ObjectMeta: metav1.ObjectMeta{UID: "policy3", Namespace: "ns3", Name: "name3"},
+		Rules:      []v1beta2.NetworkPolicyRule{*networkPolicyRule3},
 		SourceRef: &v1beta2.NetworkPolicyReference{
 			Type:      v1beta2.AntreaNetworkPolicy,
 			Namespace: "ns3",

pkg/apis/controlplane/types.go

@@ -159,9 +159,6 @@ type NetworkPolicy struct {
 	TierPriority *int32
 	// Reference to the original NetworkPolicy that the internal NetworkPolicy is created for.
 	SourceRef *NetworkPolicyReference
-	// AppliedToPerRule tracks if appliedTo is set per rule basis rather than in policy spec.
-	// Must be false for K8s NetworkPolicy.
-	AppliedToPerRule bool
 }
 
 // Direction defines traffic direction of NetworkPolicyRule.

pkg/apis/controlplane/v1beta1/conversion.go

@@ -384,8 +384,16 @@ func Convert_controlplane_AppliedToGroupPatch_To_v1beta1_AppliedToGroupPatch(in
 }
 
 func Convert_controlplane_NetworkPolicy_To_v1beta1_NetworkPolicy(in *controlplane.NetworkPolicy, out *NetworkPolicy, s conversion.Scope) error {
+	v1beta1Rules := make([]NetworkPolicyRule, len(in.Rules))
+	for i := range in.Rules {
+		var v1beta1Rule NetworkPolicyRule
+		if err := Convert_controlplane_NetworkPolicyRule_To_v1beta1_NetworkPolicyRule(&in.Rules[i], &v1beta1Rule, nil); err != nil {
+			return err
+		}
+		v1beta1Rules[i] = v1beta1Rule
+	}
 	out.ObjectMeta = in.ObjectMeta
-	out.Rules = *(*[]NetworkPolicyRule)(unsafe.Pointer(&in.Rules))
+	out.Rules = v1beta1Rules
 	out.AppliedToGroups = *(*[]string)(unsafe.Pointer(&in.AppliedToGroups))
 	out.Priority = (*float64)(unsafe.Pointer(in.Priority))
 	out.TierPriority = (*int32)(unsafe.Pointer(in.TierPriority))

pkg/apis/controlplane/v1beta2/types.go

@@ -168,9 +168,6 @@ type NetworkPolicy struct {
 	TierPriority *int32 `json:"tierPriority,omitempty" protobuf:"varint,5,opt,name=tierPriority"`
 	// Reference to the original NetworkPolicy that the internal NetworkPolicy is created for.
 	SourceRef *NetworkPolicyReference `json:"sourceRef,omitempty" protobuf:"bytes,6,opt,name=sourceRef"`
-	// AppliedToPerRule tracks if appliedTo is set per rule basis rather than in policy spec.
-	// Must be false for K8s NetworkPolicy.
-	AppliedToPerRule bool `json:"appliedToPerRule,omitempty" protobuf:"bytes,7,opt,name=appliedToPerRule"`
 }
 
 // Direction defines traffic direction of NetworkPolicyRule.

pkg/controller/networkpolicy/antreanetworkpolicy.go

@@ -127,6 +127,7 @@ func (n *NetworkPolicyController) processAntreaNetworkPolicy(np *secv1alpha1.Net
 	appliedToPerRule := np.Spec.AppliedTo == nil
 	// appliedToGroupNames tracks all distinct appliedToGroups referred to by the Antrea NetworkPolicy,
 	// either in the spec section or in ingress/egress rules.
+	// The span calculation and stale appliedToGroup cleanup logic would work seamlessly for both cases.
 	appliedToGroupNamesSet := sets.String{}
 	// Create AppliedToGroup for each AppliedTo present in AntreaNetworkPolicy spec.
 	for _, at := range np.Spec.AppliedTo {

pkg/controller/networkpolicy/clusternetworkpolicy.go

@@ -127,6 +127,7 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *secv1alpha1.C
 	appliedToPerRule := cnp.Spec.AppliedTo == nil
 	// appliedToGroupNames tracks all distinct appliedToGroups referred to by the ClusterNetworkPolicy,
 	// either in the spec section or in ingress/egress rules.
+	// The span calculation and stale appliedToGroup cleanup logic would work seamlessly for both cases.
 	appliedToGroupNamesSet := sets.String{}
 	// Create AppliedToGroup for each AppliedTo present in ClusterNetworkPolicy spec.
 	for _, at := range cnp.Spec.AppliedTo {

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -689,10 +689,9 @@ func (n *NetworkPolicyController) processNetworkPolicy(np *networkingv1.NetworkP
 			Name:      np.Name,
 			UID:       np.UID,
 		},
-		AppliedToGroups:  appliedToGroupNames,
-		Rules:            rules,
-		AppliedToPerRule: false,
-		Generation:       np.Generation,
+		AppliedToGroups: appliedToGroupNames,
+		Rules:           rules,
+		Generation:      np.Generation,
 	}
 	return internalNetworkPolicy
 }
@@ -1472,16 +1471,15 @@ func (n *NetworkPolicyController) syncInternalNetworkPolicy(key string) error {
 		nodeNames = nodeNames.Union(appGroup.SpanMeta.NodeNames)
 	}
 	updatedNetworkPolicy := &antreatypes.NetworkPolicy{
-		UID:              internalNP.UID,
-		Name:             internalNP.Name,
-		SourceRef:        internalNP.SourceRef,
-		Rules:            internalNP.Rules,
-		AppliedToGroups:  internalNP.AppliedToGroups,
-		Priority:         internalNP.Priority,
-		TierPriority:     internalNP.TierPriority,
-		AppliedToPerRule: internalNP.AppliedToPerRule,
-		SpanMeta:         antreatypes.SpanMeta{NodeNames: nodeNames},
-		Generation:       internalNP.Generation,
+		UID:             internalNP.UID,
+		Name:            internalNP.Name,
+		SourceRef:       internalNP.SourceRef,
+		Rules:           internalNP.Rules,
+		AppliedToGroups: internalNP.AppliedToGroups,
+		Priority:        internalNP.Priority,
+		TierPriority:    internalNP.TierPriority,
+		SpanMeta:        antreatypes.SpanMeta{NodeNames: nodeNames},
+		Generation:      internalNP.Generation,
 	}
 	klog.V(4).Infof("Updating internal NetworkPolicy %s with %d Nodes", key, nodeNames.Len())
 	n.internalNetworkPolicyStore.Update(updatedNetworkPolicy)

pkg/controller/networkpolicy/store/networkpolicy.go

@@ -111,10 +111,15 @@ func ToNetworkPolicyMsg(in *types.NetworkPolicy, out *controlplane.NetworkPolicy
 	}
 	// Since stored objects are immutable, we just reference the fields here.
 	out.Rules = in.Rules
-	out.AppliedToGroups = in.AppliedToGroups
+	// AppliedToGroups at policy level only need to be populated to controlplane msg if
+	// appliedTo is not set per rule. Otherwise, the agent will read appliedTo from each
+	// rule, and in that case, the policy level appliedTo only contains span information,
+	// which the Antrea agent does not care about.
+	if !in.AppliedToPerRule {
+		out.AppliedToGroups = in.AppliedToGroups
+	}
 	out.Priority = in.Priority
 	out.TierPriority = in.TierPriority
-	out.AppliedToPerRule = in.AppliedToPerRule
 }
 
 // NetworkPolicyKeyFunc knows how to get the key of a NetworkPolicy.

pkg/controller/networkpolicy/validate.go

@@ -317,17 +317,25 @@ func (v *antreaPolicyValidator) validateRuleName(ingress, egress []secv1alpha1.R
 }
 
 func (a *antreaPolicyValidator) validateAppliedTo(ingress, egress []secv1alpha1.Rule, specAppliedTo []secv1alpha1.NetworkPolicyPeer) (string, bool) {
-	appliedToInSpec, appliedToInRules := specAppliedTo != nil, false
-	for i, rule := range append(ingress, egress...) {
-		if i == 0 {
-			appliedToInRules = rule.AppliedTo != nil
-		}
-		if (rule.AppliedTo != nil) != appliedToInRules {
-			return fmt.Sprintf("appliedTo field does not appear consistently in all rules"), false
+	appliedToInSpec := len(specAppliedTo) != 0
+	countAppliedToInRules := func(rules []secv1alpha1.Rule) int {
+		num := 0
+		for _, rule := range rules {
+			if len(rule.AppliedTo) != 0 {
+				num++
+			}
 		}
+		return num
+	}
+	numAppliedToInRules := countAppliedToInRules(ingress) + countAppliedToInRules(egress)
+	if appliedToInSpec && (numAppliedToInRules > 0) {
+		return "appliedTo should not be set in both spec and rules", false
+	}
+	if !appliedToInSpec && (numAppliedToInRules == 0) {
+		return "appliedTo needs to be set in either spec or rules", false
 	}
-	if appliedToInSpec == appliedToInRules {
-		return fmt.Sprintf("appliedTo is set in both spec and rules"), false
+	if numAppliedToInRules > 0 && (numAppliedToInRules != len(ingress)+len(egress)) {
+		return "appliedTo field should either be set in all rules or in none of them", false
 	}
 	return "", true
 }