address comments

Signed-off-by: Qiyue Yao <yaoq@vmware.com>

docs/antrea-network-policy.md

@@ -1626,11 +1626,11 @@ restrictions.
   Antrea NetworkPolicy is Namespace scoped. For example, the
   `test-grp-with-namespace` Group in the [sample](#group-crd) cannot be
   used by Antrea NetworkPolicy `appliedTo`.
-- Antrea will not validate the referenced Group resources; if the convention is
-  violated in the Antrea NetworkPolicy's `appliedTo` section or for any of the rules'
-  `appliedTo`, then Antrea will report a condition of type `Realizable` in the
-  NetworkPolicy status with `False` status, `NetworkPolicyAppliedToUnsupportedGroup`
-  reason and a detailed message.
+- Antrea will not validate the referenced Group resources for the `appliedTo` convention;
+  if the convention is violated in the Antrea NetworkPolicy's `appliedTo` section
+  or for any of the rules' `appliedTo`, then Antrea will report a condition
+  `Realizable=False` in the NetworkPolicy status, the condition includes
+  `NetworkPolicyAppliedToUnsupportedGroup` reason and a detailed message.
 - `childGroups` only accepts strings, and they will be considered as names of
   the Groups and will be looked up in the policy's own Namespace. For example, if
   child Group `child-0` exists in `ns-2`, it should not be added as a child Group for

pkg/controller/networkpolicy/antreanetworkpolicy.go

@@ -15,7 +15,7 @@
 package networkpolicy
 
 import (
-	"errors"
+	"fmt"
 
 	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/client-go/tools/cache"
@@ -26,9 +26,6 @@ import (
 	antreatypes "antrea.io/antrea/pkg/controller/types"
 )
 
-// ErrNetworkPolicyAppliedToUnsupportedGroup is one of the unrealizable conditions of an internal Network Policy.
-var ErrNetworkPolicyAppliedToUnsupportedGroup = errors.New("group with IPBlocks or NamespaceSelector can not be used as AppliedTo")
-
 // addANP receives AntreaNetworkPolicy ADD events and creates resources
 // which can be consumed by agents to configure corresponding rules on the Nodes.
 func (n *NetworkPolicyController) addANP(obj interface{}) {
@@ -143,10 +140,10 @@ func (n *NetworkPolicyController) processAntreaNetworkPolicy(np *crdv1alpha1.Net
 				Name:      np.Name,
 				UID:       np.UID,
 			},
-			Name:              internalNetworkPolicyKeyFunc(np),
-			UID:               np.UID,
-			Generation:        np.Generation,
-			RealizableMessage: err.Error(),
+			Name:             internalNetworkPolicyKeyFunc(np),
+			UID:              np.UID,
+			Generation:       np.Generation,
+			RealizationError: err,
 		}
 	}
 	// Create AppliedToGroup for each AppliedTo present in AntreaNetworkPolicy spec.
@@ -241,6 +238,17 @@ func (n *NetworkPolicyController) processAppliedTo(namespace string, appliedTo [
 	return appliedToGroupNames, nil
 }
 
+// ErrNetworkPolicyAppliedToUnsupportedGroup is an error response when
+// a Group with IPBlocks or NamespaceSelector is used as AppliedTo.
+type ErrNetworkPolicyAppliedToUnsupportedGroup struct {
+	namespace string
+	groupName string
+}
+
+func (e ErrNetworkPolicyAppliedToUnsupportedGroup) Error() string {
+	return fmt.Sprintf("group %s/%s with IPBlocks or NamespaceSelector can not be used as AppliedTo", e.namespace, e.groupName)
+}
+
 func (n *NetworkPolicyController) processAppliedToGroupForNamespacedGroup(namespace, groupName string) (string, error) {
 	// Retrieve Group for corresponding entry in the AppliedToGroup.
 	g, err := n.grpLister.Groups(namespace).Get(groupName)
@@ -260,7 +268,7 @@ func (n *NetworkPolicyController) processAppliedToGroupForNamespacedGroup(namesp
 	intGrp := ig.(*antreatypes.Group)
 	if len(intGrp.IPBlocks) > 0 || (intGrp.Selector != nil && intGrp.Selector.NamespaceSelector != nil) {
 		klog.V(2).InfoS("Group with IPBlocks or NamespaceSelector can not be used as AppliedTo", "Group", g)
-		return "", ErrNetworkPolicyAppliedToUnsupportedGroup
+		return "", ErrNetworkPolicyAppliedToUnsupportedGroup{namespace: namespace, groupName: groupName}
 	}
 	return n.createAppliedToGroupForInternalGroup(intGrp), nil
 }

pkg/controller/networkpolicy/clustergroup.go

@@ -295,7 +295,7 @@ func (c *NetworkPolicyController) updateClusterGroupStatus(cg *crdv1alpha3.Clust
 		Status: cStatus,
 		Type:   crdv1alpha3.GroupMembersComputed,
 	}
-	if compareGroupMembersComputedConditionEqual(cg.Status.Conditions, condStatus) {
+	if groupMembersComputedConditionEqual(cg.Status.Conditions, condStatus) {
 		// There is no change in conditions.
 		return nil
 	}

pkg/controller/networkpolicy/clustergroup_test.go

@@ -487,7 +487,7 @@ func TestClusterClusterGroupMembersComputedConditionEqual(t *testing.T) {
 				Type:   crdv1alpha3.GroupMembersComputed,
 				Status: tt.checkStatus,
 			}
-			actualValue := compareGroupMembersComputedConditionEqual(tt.existingConds, inCond)
+			actualValue := groupMembersComputedConditionEqual(tt.existingConds, inCond)
 			assert.Equal(t, tt.expValue, actualValue)
 		})
 	}

pkg/controller/networkpolicy/crd_utils.go

@@ -52,15 +52,15 @@ var semanticIgnoreLastTransitionTime = conversion.EqualitiesOrDie(
 	},
 )
 
-// CompareNetworkPolicyStatus compares two NetworkPolicyStatus objects, ignoring
-// LastTransitionTime equality in the status Conditions.
-func CompareNetworkPolicyStatus(oldStatus, newStatus v1alpha1.NetworkPolicyStatus) bool {
+// NetworkPolicyStatusEqual compares two NetworkPolicyStatus objects. It disregards
+// the LastTransitionTime field in the status Conditions.
+func NetworkPolicyStatusEqual(oldStatus, newStatus v1alpha1.NetworkPolicyStatus) bool {
 	return semanticIgnoreLastTransitionTime.DeepEqual(oldStatus, newStatus)
 }
 
-// compareGroupMembersComputedConditionEqual checks whether the condition status for GroupMembersComputed condition
+// groupMembersComputedConditionEqual checks whether the condition status for GroupMembersComputed condition
 // is same. Returns true if equal, otherwise returns false. It disregards the lastTransitionTime field.
-func compareGroupMembersComputedConditionEqual(conds []crdv1alpha3.GroupCondition, condition crdv1alpha3.GroupCondition) bool {
+func groupMembersComputedConditionEqual(conds []crdv1alpha3.GroupCondition, condition crdv1alpha3.GroupCondition) bool {
 	for _, c := range conds {
 		if c.Type == crdv1alpha3.GroupMembersComputed {
 			if c.Status == condition.Status {

pkg/controller/networkpolicy/group.go

@@ -266,7 +266,7 @@ func (n *NetworkPolicyController) updateGroupStatus(g *crdv1alpha3.Group, cStatu
 		Status: cStatus,
 		Type:   crdv1alpha3.GroupMembersComputed,
 	}
-	if compareGroupMembersComputedConditionEqual(g.Status.Conditions, condStatus) {
+	if groupMembersComputedConditionEqual(g.Status.Conditions, condStatus) {
 		// There is no change in conditions.
 		return nil
 	}

pkg/controller/networkpolicy/group_test.go

@@ -499,7 +499,7 @@ func TestGroupMembersComputedConditionEqual(t *testing.T) {
 				Type:   crdv1alpha3.GroupMembersComputed,
 				Status: tt.checkStatus,
 			}
-			actualValue := compareGroupMembersComputedConditionEqual(tt.existingConds, inCond)
+			actualValue := groupMembersComputedConditionEqual(tt.existingConds, inCond)
 			assert.Equal(t, tt.expValue, actualValue)
 		})
 	}

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -1492,7 +1492,7 @@ func (n *NetworkPolicyController) syncInternalNetworkPolicy(key string) error {
 		PerNamespaceSelectors: internalNP.PerNamespaceSelectors,
 		SpanMeta:              antreatypes.SpanMeta{NodeNames: nodeNames},
 		Generation:            internalNP.Generation,
-		RealizableMessage:     internalNP.RealizableMessage,
+		RealizationError:      internalNP.RealizationError,
 	}
 	klog.V(4).Infof("Updating internal NetworkPolicy %s with %d Nodes", key, nodeNames.Len())
 	n.internalNetworkPolicyStore.Update(updatedNetworkPolicy)

pkg/controller/networkpolicy/status_controller.go

@@ -111,7 +111,7 @@ func NewStatusController(antreaClient antreaclientset.Interface, internalNetwork
 func (c *StatusController) updateCNP(old, cur interface{}) {
 	curCNP := cur.(*crdv1alpha1.ClusterNetworkPolicy)
 	oldCNP := old.(*crdv1alpha1.ClusterNetworkPolicy)
-	if CompareNetworkPolicyStatus(oldCNP.Status, curCNP.Status) {
+	if NetworkPolicyStatusEqual(oldCNP.Status, curCNP.Status) {
 		return
 	}
 	key := internalNetworkPolicyKeyFunc(oldCNP)
@@ -121,7 +121,7 @@ func (c *StatusController) updateCNP(old, cur interface{}) {
 func (c *StatusController) updateANP(old, cur interface{}) {
 	curANP := cur.(*crdv1alpha1.NetworkPolicy)
 	oldANP := old.(*crdv1alpha1.NetworkPolicy)
-	if CompareNetworkPolicyStatus(oldANP.Status, curANP.Status) {
+	if NetworkPolicyStatusEqual(oldANP.Status, curANP.Status) {
 		return
 	}
 	key := internalNetworkPolicyKeyFunc(oldANP)
@@ -274,7 +274,7 @@ func (c *StatusController) syncHandler(key string) error {
 		status := &crdv1alpha1.NetworkPolicyStatus{
 			Phase:              crdv1alpha1.NetworkPolicyPending,
 			ObservedGeneration: internalNP.Generation,
-			Conditions:         GenerateNetworkPolicyCondition(""),
+			Conditions:         GenerateNetworkPolicyCondition(nil),
 		}
 		if internalNP.SourceRef.Type == controlplane.AntreaNetworkPolicy {
 			return c.npControlInterface.UpdateAntreaNetworkPolicyStatus(internalNP.SourceRef.Namespace, internalNP.SourceRef.Name, status)
@@ -284,13 +284,12 @@ func (c *StatusController) syncHandler(key string) error {
 
 	// It means the NetworkPolicy has been processed, and marked as unrealizable. It will enter unrealizable phase
 	// instead of being further realized. Antrea-agents will not process further.
-	if internalNP.RealizableMessage != "" {
+	if internalNP.RealizationError != nil {
 		status := &crdv1alpha1.NetworkPolicyStatus{
 			Phase:              crdv1alpha1.NetworkPolicyPending,
 			ObservedGeneration: internalNP.Generation,
-			Conditions:         GenerateNetworkPolicyCondition(internalNP.RealizableMessage),
+			Conditions:         GenerateNetworkPolicyCondition(internalNP.RealizationError),
 		}
-		internalNP.SpanMeta.NodeNames = nil
 		if internalNP.SourceRef.Type == controlplane.AntreaNetworkPolicy {
 			return c.npControlInterface.UpdateAntreaNetworkPolicyStatus(internalNP.SourceRef.Namespace, internalNP.SourceRef.Name, status)
 		}
@@ -321,7 +320,7 @@ func (c *StatusController) syncHandler(key string) error {
 		ObservedGeneration:   internalNP.Generation,
 		CurrentNodesRealized: int32(currentNodes),
 		DesiredNodesRealized: int32(desiredNodes),
-		Conditions:           GenerateNetworkPolicyCondition(""),
+		Conditions:           GenerateNetworkPolicyCondition(nil),
 	}
 	klog.V(2).Infof("Updating NetworkPolicy %s status: %v", internalNP.SourceRef.ToString(), status)
 	if internalNP.SourceRef.Type == controlplane.AntreaNetworkPolicy {
@@ -349,7 +348,7 @@ func (c *networkPolicyControl) UpdateAntreaNetworkPolicyStatus(namespace, name s
 		klog.Infof("Didn't find the original Antrea NetworkPolicy %s/%s, skip updating status", namespace, name)
 		return nil
 	}
-	if CompareNetworkPolicyStatus(anp.Status, *status) {
+	if NetworkPolicyStatusEqual(anp.Status, *status) {
 		return nil
 	}
 
@@ -382,7 +381,7 @@ func (c *networkPolicyControl) UpdateAntreaClusterNetworkPolicyStatus(name strin
 		return nil
 	}
 	// If the current status equals to the desired status, no need to update.
-	if CompareNetworkPolicyStatus(cnp.Status, *status) {
+	if NetworkPolicyStatusEqual(cnp.Status, *status) {
 		return nil
 	}
 
@@ -408,25 +407,25 @@ func (c *networkPolicyControl) UpdateAntreaClusterNetworkPolicyStatus(name strin
 	return updateErr
 }
 
-// GenerateNetworkPolicyCondition generates conditions based on the given error message.
-// Empty error message generates stale NetworkPolicyCondition of True status.
+// GenerateNetworkPolicyCondition generates conditions based on the given error type.
+// Error of nil type means the NetworkPolicyCondition status is True.
 // Supports ErrNetworkPolicyAppliedToUnsupportedGroup error.
-func GenerateNetworkPolicyCondition(message string) []crdv1alpha1.NetworkPolicyCondition {
+func GenerateNetworkPolicyCondition(err error) []crdv1alpha1.NetworkPolicyCondition {
 	var conditions []crdv1alpha1.NetworkPolicyCondition
-	switch message {
-	case "":
+	switch err.(type) {
+	case nil:
 		conditions = append(conditions, crdv1alpha1.NetworkPolicyCondition{
 			Type:               crdv1alpha1.NetworkPolicyConditionRealizable,
 			Status:             v1.ConditionTrue,
 			LastTransitionTime: v1.Now(),
 		})
-	case ErrNetworkPolicyAppliedToUnsupportedGroup.Error():
+	case ErrNetworkPolicyAppliedToUnsupportedGroup:
 		conditions = append(conditions, crdv1alpha1.NetworkPolicyCondition{
 			Type:               crdv1alpha1.NetworkPolicyConditionRealizable,
 			Status:             v1.ConditionFalse,
 			LastTransitionTime: v1.Now(),
 			Reason:             "NetworkPolicyAppliedToUnsupportedGroup",
-			Message:            message,
+			Message:            err.Error(),
 		})
 	}
 	return conditions

pkg/controller/networkpolicy/status_controller_test.go

@@ -163,14 +163,14 @@ func TestCreateAntreaNetworkPolicy(t *testing.T) {
 				ObservedGeneration:   1,
 				CurrentNodesRealized: 0,
 				DesiredNodesRealized: 2,
-				Conditions:           GenerateNetworkPolicyCondition(""),
+				Conditions:           GenerateNetworkPolicyCondition(nil),
 			},
 			expectedCNPStatus: &crdv1alpha1.NetworkPolicyStatus{
 				Phase:                crdv1alpha1.NetworkPolicyRealizing,
 				ObservedGeneration:   1,
 				CurrentNodesRealized: 0,
 				DesiredNodesRealized: 2,
-				Conditions:           GenerateNetworkPolicyCondition(""),
+				Conditions:           GenerateNetworkPolicyCondition(nil),
 			},
 		},
 		{
@@ -190,14 +190,14 @@ func TestCreateAntreaNetworkPolicy(t *testing.T) {
 				ObservedGeneration:   2,
 				CurrentNodesRealized: 1,
 				DesiredNodesRealized: 2,
-				Conditions:           GenerateNetworkPolicyCondition(""),
+				Conditions:           GenerateNetworkPolicyCondition(nil),
 			},
 			expectedCNPStatus: &crdv1alpha1.NetworkPolicyStatus{
 				Phase:                crdv1alpha1.NetworkPolicyRealizing,
 				ObservedGeneration:   3,
 				CurrentNodesRealized: 1,
 				DesiredNodesRealized: 2,
-				Conditions:           GenerateNetworkPolicyCondition(""),
+				Conditions:           GenerateNetworkPolicyCondition(nil),
 			},
 		},
 		{
@@ -217,14 +217,14 @@ func TestCreateAntreaNetworkPolicy(t *testing.T) {
 				ObservedGeneration:   3,
 				CurrentNodesRealized: 2,
 				DesiredNodesRealized: 2,
-				Conditions:           GenerateNetworkPolicyCondition(""),
+				Conditions:           GenerateNetworkPolicyCondition(nil),
 			},
 			expectedCNPStatus: &crdv1alpha1.NetworkPolicyStatus{
 				Phase:                crdv1alpha1.NetworkPolicyRealized,
 				ObservedGeneration:   4,
 				CurrentNodesRealized: 2,
 				DesiredNodesRealized: 2,
-				Conditions:           GenerateNetworkPolicyCondition(""),
+				Conditions:           GenerateNetworkPolicyCondition(nil),
 			},
 		},
 	}
@@ -249,8 +249,8 @@ func TestCreateAntreaNetworkPolicy(t *testing.T) {
 
 			// TODO: Use a determinate mechanism.
 			time.Sleep(500 * time.Millisecond)
-			assert.True(t, CompareNetworkPolicyStatus(*tt.expectedANPStatus, *networkPolicyControl.getAntreaNetworkPolicyStatus()))
-			assert.True(t, CompareNetworkPolicyStatus(*tt.expectedCNPStatus, *networkPolicyControl.getAntreaClusterNetworkPolicyStatus()))
+			assert.True(t, NetworkPolicyStatusEqual(*tt.expectedANPStatus, *networkPolicyControl.getAntreaNetworkPolicyStatus()))
+			assert.True(t, NetworkPolicyStatusEqual(*tt.expectedCNPStatus, *networkPolicyControl.getAntreaClusterNetworkPolicyStatus()))
 		})
 	}
 }
@@ -273,19 +273,19 @@ func TestUpdateAntreaNetworkPolicy(t *testing.T) {
 	statusController.UpdateStatus(newNetworkPolicyStatus("cnp1", "node5", 2))
 	// TODO: Use a determinate mechanism.
 	time.Sleep(500 * time.Millisecond)
-	assert.True(t, CompareNetworkPolicyStatus(crdv1alpha1.NetworkPolicyStatus{
+	assert.True(t, NetworkPolicyStatusEqual(crdv1alpha1.NetworkPolicyStatus{
 		Phase:                crdv1alpha1.NetworkPolicyRealized,
 		ObservedGeneration:   1,
 		CurrentNodesRealized: 2,
 		DesiredNodesRealized: 2,
-		Conditions:           GenerateNetworkPolicyCondition(""),
+		Conditions:           GenerateNetworkPolicyCondition(nil),
 	}, *networkPolicyControl.getAntreaNetworkPolicyStatus()))
-	assert.True(t, CompareNetworkPolicyStatus(crdv1alpha1.NetworkPolicyStatus{
+	assert.True(t, NetworkPolicyStatusEqual(crdv1alpha1.NetworkPolicyStatus{
 		Phase:                crdv1alpha1.NetworkPolicyRealized,
 		ObservedGeneration:   2,
 		CurrentNodesRealized: 3,
 		DesiredNodesRealized: 3,
-		Conditions:           GenerateNetworkPolicyCondition(""),
+		Conditions:           GenerateNetworkPolicyCondition(nil),
 	}, *networkPolicyControl.getAntreaClusterNetworkPolicyStatus()))
 
 	anp1Updated := newInternalNetworkPolicy("anp1", 2, []string{"node1", "node2", "node3"}, newAntreaNetworkPolicyReference("ns1", "anp1"))
@@ -294,19 +294,19 @@ func TestUpdateAntreaNetworkPolicy(t *testing.T) {
 	networkPolicyStore.Update(cnp1Updated)
 	// TODO: Use a determinate mechanism.
 	time.Sleep(500 * time.Millisecond)
-	assert.True(t, CompareNetworkPolicyStatus(crdv1alpha1.NetworkPolicyStatus{
+	assert.True(t, NetworkPolicyStatusEqual(crdv1alpha1.NetworkPolicyStatus{
 		Phase:                crdv1alpha1.NetworkPolicyRealizing,
 		ObservedGeneration:   2,
 		CurrentNodesRealized: 0,
 		DesiredNodesRealized: 3,
-		Conditions:           GenerateNetworkPolicyCondition(""),
+		Conditions:           GenerateNetworkPolicyCondition(nil),
 	}, *networkPolicyControl.getAntreaNetworkPolicyStatus()))
-	assert.True(t, CompareNetworkPolicyStatus(crdv1alpha1.NetworkPolicyStatus{
+	assert.True(t, NetworkPolicyStatusEqual(crdv1alpha1.NetworkPolicyStatus{
 		Phase:                crdv1alpha1.NetworkPolicyRealizing,
 		ObservedGeneration:   3,
 		CurrentNodesRealized: 0,
 		DesiredNodesRealized: 2,
-		Conditions:           GenerateNetworkPolicyCondition(""),
+		Conditions:           GenerateNetworkPolicyCondition(nil),
 	}, *networkPolicyControl.getAntreaClusterNetworkPolicyStatus()))
 }
 

pkg/controller/types/networkpolicy.go

@@ -108,7 +108,7 @@ type NetworkPolicy struct {
 	// to re-calculate affected Namespaces.
 	// It is set only for AntreaClusterNetworkPolicies with per-namespace rules.
 	PerNamespaceSelectors []labels.Selector
-	// RealizableMessage stores realizable information of the internal Network Policy.
+	// RealizationError stores realization error of the internal Network Policy.
 	// It is set when processing the original Network Policy.
-	RealizableMessage string
+	RealizationError error
 }