Address comments

Signed-off-by: wgrayson <wgrayson@vmware.com>

docs/antrea-network-policy.md

@@ -403,7 +403,7 @@ The [second example](#acnp-with-clustergroup-reference) policy applies to all ne
 "test-cg-with-db-selector" ClusterGroup.
 The [third example](#acnp-for-complete-pod-isolation-in-selected-namespaces) policy applies to all Pods in the
 Namespaces that matches label "app=no-network-access-required".
-`appliedTo' also supports ServiceAccount based selection. This allows users using Service Account to select Pods. 
+`appliedTo' also supports ServiceAccount based selection. This allows users using Service Account to select Pods.
 More details can be found in the [ServiceAccountSelector](#serviceaccount-based-selection) section.
 
 **priority**: The `priority` field determines the relative priority of the
@@ -1116,15 +1116,16 @@ the Service referred in `ServiceReference`.
 ## ServiceAccount based selection
 
 Antrea ClusterNetworkPolicy accepts a `serviceAccounts` field to select all Pods that have been assigned one of
-ServiceAccounts selected by this field. This field could be used in `appliedTo`, ingress `from` and egress `to` section. 
+ServiceAccounts selected by this field. This field could be used in `appliedTo`, ingress `from` and egress `to` section.
 No matter `serviceAccounts` is used in which sections, it cannot be used with any other fields.
 
 `serviceAccounts` can be used in three ways:
+
 1. `name` + `namespace`: The ServiceAccount with a specific name under a specific namespace will be selected.
 2. `labelSelector`: All ServiceAccounts with specific labels in all namespace will be selected.
 3. `labelSelector` + `namespaceSelector`: All ServiceAccounts with specific labels in all namespaces that have
    specific labels will be selected.
-   
+
 An example policy using `serviceAccounts` could look like this:
 
 ```yaml
@@ -1137,22 +1138,22 @@ spec:
     tier: securityops
     appliedTo:
       - serviceAccounts:
-        - labelSelector:
+          labelSelector:
             matchLabels:
               level: user
     egress:
       - action: Drop
         to:
           - serviceAccounts:
-            - name: sa-1
+              name: sa-1
               namespace: ns-1
         name: ServiceAccountsEgressRule
         enableLogging: false
     ingress:
      - action: Reject
        from:
          - serviceAccounts:
-           - labelSelector:
+             labelSelector:
                matchLabels:
                  level: admin
              namespaceSelector:
@@ -1162,7 +1163,7 @@ spec:
        enableLogging: false
 ```
 
-In this example, the policy will apply to all Pods whose ServiceAccount has label `level: user`. 
+In this example, the policy will apply to all Pods whose ServiceAccount has label `level: user`.
 Let's call those Pods "appliedToPods".
 The egress `to` section will select all Pods whose ServiceAccount is in `ns-1` namespace and name as `sa-1`.
 Let's call those Pods "egressPods".
@@ -1175,15 +1176,14 @@ So the policy will act as:
 Traffic from "appliedToPods" to "egressPods" will be dropped.
 Traffic from "ingressPods" to "appliedToPods" will be rejected.
 
-
 There are some CAVEATS after introducing `serviceAccounts`:
 
 Antrea ClusterNetworkPolicy requires `appliedTo` should be set either in `spec.appliedTo` or in `appliedTo` of all rules.
 If `serviceAccounts` field doesn't select any ServiceAccounts or selected ServiceAccounts are not used by any Pods,
 it may cause the policy violates the requirement above. In this situation, the whole policy won't be processed.
 
 Antrea will assign a custom label to all Pods in Antrea group index to help Antrea to select Pods based on ServiceAccount.
-The custom label looks like: `antrea.io/reserved-label-service-account:[ServiceAccountName]`. Users should avoid using
+The custom label looks like: `internal.antrea.io/service-account:[ServiceAccountName]`. Users should avoid using
 this label in any entities whether a policy with `serviceAccounts` is applied in the cluster.
 
 ## RBAC

pkg/apis/crd/v1alpha1/types.go

@@ -438,7 +438,7 @@ type NetworkPolicyPeer struct {
 	// workloads in AppliedTo/To/From fields.
 	// Cannot be set with any other selector.
 	// +optional
-	ServiceAccounts []ServiceAccount `json:"serviceAccounts,omitempty"`
+	ServiceAccounts *PeerServiceAccounts `json:"serviceAccounts,omitempty"`
 }
 
 type PeerNamespaces struct {
@@ -597,7 +597,7 @@ type TierList struct {
 	Items []Tier `json:"items"`
 }
 
-type ServiceAccount struct {
+type PeerServiceAccounts struct {
 	// Name and Namespace must be used together to select a specific ServiceAccount.
 	// Name and Namespace cannot be set with any other fields.
 	Name      string `json:"name,omitempty"`

pkg/controller/grouping/custom_label.go

@@ -21,7 +21,7 @@ import (
 )
 
 const (
-	CustomLabelKeyPrefix   = "antrea.io/reserved-label-"
+	CustomLabelKeyPrefix   = "internal.antrea.io/"
 	ServiceAccountLabelKey = "service-account"
 )
 

pkg/controller/networkpolicy/clusternetworkpolicy.go

@@ -16,6 +16,7 @@ package networkpolicy
 
 import (
 	"reflect"
+	"strconv"
 
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -261,45 +262,43 @@ func (n *NetworkPolicyController) deleteNamespace(old interface{}) {
 // re-processed based on if they are affected by an added/updated/deleted
 // ServiceAccount.
 func (n *NetworkPolicyController) filterACNPsByServiceAccount(sa *v1.ServiceAccount) (acnps []*crdv1alpha1.ClusterNetworkPolicy) {
-	acnpObjs, err := n.cnpInformer.Informer().GetIndexer().ByIndex(ServiceAccountsPeerIndex, HasServiceAccountsPeer)
+	acnpObjs, err := n.cnpInformer.Informer().GetIndexer().ByIndex(HasServiceAccountsPeerIndex, strconv.FormatBool(true))
 	if err != nil {
 		klog.Errorf("Error fetching ClusterNetworkPolicies that have ServiceAccounts in either AppliedTo or Rules: %v", err)
 		return nil
 	}
 
 	peerSelectedServiceAccount := func(peers []crdv1alpha1.NetworkPolicyPeer) bool {
 		for _, peer := range peers {
-			if peer.ServiceAccounts != nil && len(peer.ServiceAccounts) > 0 {
-				for _, serviceAccount := range peer.ServiceAccounts {
-					if serviceAccount.Name == sa.Name && serviceAccount.Namespace == sa.Namespace {
-						return true
-					}
-					if serviceAccount.LabelSelector != nil {
-						if serviceAccount.NamespaceSelector != nil {
-							saNSSelector, err := metav1.LabelSelectorAsSelector(serviceAccount.NamespaceSelector)
-							if err != nil {
-								continue
-							}
-							selectedNSs, _ := n.namespaceLister.List(saNSSelector)
-							nsSelected := false
-							for _, selectedNS := range selectedNSs {
-								if sa.Namespace == selectedNS.Name {
-									nsSelected = true
-									break
-								}
-							}
-							if !nsSelected {
-								continue
-							}
-						}
-						saSelector, err := metav1.LabelSelectorAsSelector(serviceAccount.LabelSelector)
+			if peer.ServiceAccounts != nil {
+				if peer.ServiceAccounts.Name == sa.Name && peer.ServiceAccounts.Namespace == sa.Namespace {
+					return true
+				}
+				if peer.ServiceAccounts.LabelSelector != nil {
+					if peer.ServiceAccounts.NamespaceSelector != nil {
+						saNSSelector, err := metav1.LabelSelectorAsSelector(peer.ServiceAccounts.NamespaceSelector)
 						if err != nil {
 							continue
 						}
-						if saSelector.Matches(labels.Set(sa.Labels)) {
-							return true
+						selectedNSs, _ := n.namespaceLister.List(saNSSelector)
+						nsSelected := false
+						for _, selectedNS := range selectedNSs {
+							if sa.Namespace == selectedNS.Name {
+								nsSelected = true
+								break
+							}
+						}
+						if !nsSelected {
+							continue
 						}
 					}
+					saSelector, err := metav1.LabelSelectorAsSelector(peer.ServiceAccounts.LabelSelector)
+					if err != nil {
+						continue
+					}
+					if saSelector.Matches(labels.Set(sa.Labels)) {
+						return true
+					}
 				}
 			}
 		}
@@ -430,8 +429,8 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *crdv1alpha1.C
 	if hasPerNamespaceRule && len(cnp.Spec.AppliedTo) > 0 {
 		for _, at := range cnp.Spec.AppliedTo {
 			var newATs []crdv1alpha1.NetworkPolicyPeer
-			if len(at.ServiceAccounts) > 0 {
-				newATs = n.transServiceAccounts(at.ServiceAccounts)
+			if at.ServiceAccounts != nil {
+				newATs = n.translateServiceAccounts(at.ServiceAccounts)
 			} else {
 				newATs = append(newATs, at)
 			}
@@ -497,8 +496,8 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *crdv1alpha1.C
 					// Create a rule for each affected Namespace of appliedTo at rule level
 					for _, at := range cnpRule.AppliedTo {
 						var newATs []crdv1alpha1.NetworkPolicyPeer
-						if len(at.ServiceAccounts) > 0 {
-							newATs = n.transServiceAccounts(at.ServiceAccounts)
+						if at.ServiceAccounts != nil {
+							newATs = n.translateServiceAccounts(at.ServiceAccounts)
 						} else {
 							newATs = append(newATs, at)
 						}
@@ -550,10 +549,10 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *crdv1alpha1.C
 func (n *NetworkPolicyController) validateAppliedToWithSA(acnp *crdv1alpha1.ClusterNetworkPolicy) bool {
 	peerNotEmpty := func(peers []crdv1alpha1.NetworkPolicyPeer) bool {
 		for _, peer := range peers {
-			if len(peer.ServiceAccounts) == 0 {
+			if peer.ServiceAccounts == nil {
 				return true
 			}
-			if len(n.transServiceAccounts(peer.ServiceAccounts)) > 0 {
+			if len(n.translateServiceAccounts(peer.ServiceAccounts)) > 0 {
 				return true
 			}
 		}
@@ -574,9 +573,9 @@ func (n *NetworkPolicyController) validateAppliedToWithSA(acnp *crdv1alpha1.Clus
 	return allRulesHaveAT(acnp.Spec.Ingress) && allRulesHaveAT(acnp.Spec.Egress)
 }
 
-// transServiceAccounts translates a list of ServiceAccount to a list of
+// translateServiceAccounts translates a crdv1alpha1.PeerServiceAccounts to a list of
 // NetworkPolicyPeer with PodSelector+NamespaceSelector.
-func (n *NetworkPolicyController) transServiceAccounts(serviceAccounts []crdv1alpha1.ServiceAccount) (newPeers []crdv1alpha1.NetworkPolicyPeer) {
+func (n *NetworkPolicyController) translateServiceAccounts(sa *crdv1alpha1.PeerServiceAccounts) (newPeers []crdv1alpha1.NetworkPolicyPeer) {
 	// saToPSelNSel returns a crdv1alpha1.NetworkPolicyPeer with PodSelector +
 	// NamespaceSelector based on ServiceAccount Name + Namespace
 	saToPSelNSel := func(saNs, saName string) crdv1alpha1.NetworkPolicyPeer {
@@ -589,33 +588,35 @@ func (n *NetworkPolicyController) transServiceAccounts(serviceAccounts []crdv1al
 			},
 		}
 	}
-	for _, sa := range serviceAccounts {
-		if sa.Name != "" && sa.Namespace != "" {
-			newPeers = append(newPeers, saToPSelNSel(sa.Namespace, sa.Name))
+	if sa == nil {
+		 return
+	}
+	if sa.Name != "" && sa.Namespace != "" {
+		newPeers = append(newPeers, saToPSelNSel(sa.Namespace, sa.Name))
+		return
+	}
+	if sa.LabelSelector != nil {
+		var selectedSAs []*v1.ServiceAccount
+		saLabelSelector, err := metav1.LabelSelectorAsSelector(sa.LabelSelector)
+		if err != nil {
+			return
 		}
-		if sa.LabelSelector != nil {
-			var selectedSAs []*v1.ServiceAccount
-			saLabelSelector, err := metav1.LabelSelectorAsSelector(sa.LabelSelector)
+		if sa.NamespaceSelector == nil {
+			selectedSAs, _ = n.serviceAccountLister.List(saLabelSelector)
+		} else {
+			saNSSelector, err := metav1.LabelSelectorAsSelector(sa.NamespaceSelector)
 			if err != nil {
-				continue
-			}
-			if sa.NamespaceSelector == nil {
-				selectedSAs, _ = n.serviceAccountLister.List(saLabelSelector)
-			} else {
-				saNSSelector, err := metav1.LabelSelectorAsSelector(sa.NamespaceSelector)
-				if err != nil {
-					continue
-				}
-				selectedNSs, _ := n.namespaceLister.List(saNSSelector)
-				for _, selectedNS := range selectedNSs {
-					saInNS, _ := n.serviceAccountLister.ServiceAccounts(selectedNS.Name).List(saLabelSelector)
-					selectedSAs = append(selectedSAs, saInNS...)
-				}
+				return
 			}
-			for _, selectedSA := range selectedSAs {
-				newPeers = append(newPeers, saToPSelNSel(selectedSA.Namespace, selectedSA.Name))
+			selectedNSs, _ := n.namespaceLister.List(saNSSelector)
+			for _, selectedNS := range selectedNSs {
+				saInNS, _ := n.serviceAccountLister.ServiceAccounts(selectedNS.Name).List(saLabelSelector)
+				selectedSAs = append(selectedSAs, saInNS...)
 			}
 		}
+		for _, selectedSA := range selectedSAs {
+			newPeers = append(newPeers, saToPSelNSel(selectedSA.Namespace, selectedSA.Name))
+		}
 	}
 	return
 }
@@ -652,8 +653,8 @@ func (n *NetworkPolicyController) processClusterAppliedTo(appliedTo []crdv1alpha
 	for _, at := range appliedTo {
 		if at.Group != "" {
 			insertATG(n.processAppliedToGroupForCG(at.Group))
-		} else if len(at.ServiceAccounts) > 0 {
-			newPeers := n.transServiceAccounts(at.ServiceAccounts)
+		} else if at.ServiceAccounts != nil {
+			newPeers := n.translateServiceAccounts(at.ServiceAccounts)
 			for _, newPeer := range newPeers {
 				insertATG(n.createAppliedToGroup("", newPeer.PodSelector, newPeer.NamespaceSelector, nil))
 			}