Address comments

Signed-off-by: wgrayson <[email protected]>

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
@@ -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,7 +1176,6 @@ 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.

pkg/apis/crd/v1alpha2/types.go

@@ -186,6 +186,10 @@ type AppliedTo struct {
 	// Groups is the set of ClusterGroup names.
 	// +optional
 	Groups []string `json:"groups,omitempty"`
+	// Select all Pods with the ServiceAccount matched by this field.
+	// Cannot be set with any other selector.
+	// +optional
+	ServiceAccounts []v1alpha1.ServiceAccount `json:"serviceAccounts,omitempty"`
 }
 
 // +genclient

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,7 +262,7 @@ 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
@@ -431,7 +432,7 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *crdv1alpha1.C
 		for _, at := range cnp.Spec.AppliedTo {
 			var newATs []crdv1alpha1.NetworkPolicyPeer
 			if len(at.ServiceAccounts) > 0 {
-				newATs = n.transServiceAccounts(at.ServiceAccounts)
+				newATs = n.translateServiceAccounts(at.ServiceAccounts)
 			} else {
 				newATs = append(newATs, at)
 			}
@@ -498,7 +499,7 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *crdv1alpha1.C
 					for _, at := range cnpRule.AppliedTo {
 						var newATs []crdv1alpha1.NetworkPolicyPeer
 						if len(at.ServiceAccounts) > 0 {
-							newATs = n.transServiceAccounts(at.ServiceAccounts)
+							newATs = n.translateServiceAccounts(at.ServiceAccounts)
 						} else {
 							newATs = append(newATs, at)
 						}
@@ -553,7 +554,7 @@ func (n *NetworkPolicyController) validateAppliedToWithSA(acnp *crdv1alpha1.Clus
 			if len(peer.ServiceAccounts) == 0 {
 				return true
 			}
-			if len(n.transServiceAccounts(peer.ServiceAccounts)) > 0 {
+			if len(n.translateServiceAccounts(peer.ServiceAccounts)) > 0 {
 				return true
 			}
 		}
@@ -574,9 +575,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 list of ServiceAccount to a list of
 // NetworkPolicyPeer with PodSelector+NamespaceSelector.
-func (n *NetworkPolicyController) transServiceAccounts(serviceAccounts []crdv1alpha1.ServiceAccount) (newPeers []crdv1alpha1.NetworkPolicyPeer) {
+func (n *NetworkPolicyController) translateServiceAccounts(serviceAccounts []crdv1alpha1.ServiceAccount) (newPeers []crdv1alpha1.NetworkPolicyPeer) {
 	// saToPSelNSel returns a crdv1alpha1.NetworkPolicyPeer with PodSelector +
 	// NamespaceSelector based on ServiceAccount Name + Namespace
 	saToPSelNSel := func(saNs, saName string) crdv1alpha1.NetworkPolicyPeer {
@@ -653,7 +654,7 @@ func (n *NetworkPolicyController) processClusterAppliedTo(appliedTo []crdv1alpha
 		if at.Group != "" {
 			insertATG(n.processAppliedToGroupForCG(at.Group))
 		} else if len(at.ServiceAccounts) > 0 {
-			newPeers := n.transServiceAccounts(at.ServiceAccounts)
+			newPeers := n.translateServiceAccounts(at.ServiceAccounts)
 			for _, newPeer := range newPeers {
 				insertATG(n.createAppliedToGroup("", newPeer.PodSelector, newPeer.NamespaceSelector, nil))
 			}

pkg/controller/networkpolicy/clusternetworkpolicy_test.go

@@ -81,7 +81,7 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 	selectorB := metav1.LabelSelector{MatchLabels: map[string]string{"foo2": "bar2"}}
 	selectorC := metav1.LabelSelector{MatchLabels: map[string]string{"foo3": "bar3"}}
 
-	selectorD := metav1.LabelSelector{MatchLabels: map[string]string{"antrea.io/reserved-label-service-account": saA.Name}}
+	selectorD := metav1.LabelSelector{MatchLabels: map[string]string{"internal.antrea.io/service-account": saA.Name}}
 	selectorE := metav1.LabelSelector{MatchLabels: map[string]string{"kubernetes.io/metadata.name": saA.Namespace}}
 
 	labelSelectorA, _ := metav1.LabelSelectorAsSelector(&selectorA)
@@ -1403,7 +1403,6 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 				c.tierStore.Add(&tierA)
 			}
 			actualPolicy := c.processClusterNetworkPolicy(tt.inputPolicy)
-			t.Logf("JB： %v", actualPolicy)
 			assert.Equal(t, tt.expectedPolicy.UID, actualPolicy.UID)
 			assert.Equal(t, tt.expectedPolicy.Name, actualPolicy.Name)
 			assert.Equal(t, tt.expectedPolicy.SourceRef, actualPolicy.SourceRef)

pkg/controller/networkpolicy/crd_utils.go

@@ -115,7 +115,7 @@ func (n *NetworkPolicyController) toAntreaPeerForCRD(peers []v1alpha1.NetworkPol
 		} else if peer.FQDN != "" {
 			fqdns = append(fqdns, peer.FQDN)
 		} else if len(peer.ServiceAccounts) > 0 {
-			newPeers := n.transServiceAccounts(peer.ServiceAccounts)
+			newPeers := n.translateServiceAccounts(peer.ServiceAccounts)
 			for _, newPeer := range newPeers {
 				normalizedUID := n.createAddressGroup(np.GetNamespace(), newPeer.PodSelector, newPeer.NamespaceSelector, nil)
 				addressGroups = append(addressGroups, normalizedUID)

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -82,9 +82,8 @@ const (
 	PriorityIndex = "priority"
 	// ClusterGroupIndex is used to index ClusterNetworkPolicies by ClusterGroup names.
 	ClusterGroupIndex = "clustergroup"
-	// ServiceAccountsPeerIndex is used to index ClusterNetworkPolicies by ServiceAccount labels.
-	ServiceAccountsPeerIndex = "hasServiceAccountsPeer"
-	HasServiceAccountsPeer   = "true"
+	// HasServiceAccountsPeerIndex is used to index ClusterNetworkPolicies by ServiceAccount labels.
+	HasServiceAccountsPeerIndex = "hasServiceAccountsPeer"
 
 	appliedToGroupType grouping.GroupType = "appliedToGroup"
 	addressGroupType   grouping.GroupType = "addressGroup"
@@ -279,7 +278,7 @@ var cnpIndexers = cache.Indexers{
 		}
 		return groupNames.List(), nil
 	},
-	ServiceAccountsPeerIndex: func(obj interface{}) ([]string, error) {
+	HasServiceAccountsPeerIndex: func(obj interface{}) ([]string, error) {
 		cnp, ok := obj.(*secv1alpha1.ClusterNetworkPolicy)
 		if !ok {
 			return []string{}, nil
@@ -293,25 +292,25 @@ var cnpIndexers = cache.Indexers{
 			return false
 		}
 		if peerHasServiceAccounts(cnp.Spec.AppliedTo) {
-			return []string{HasServiceAccountsPeer}, nil
+			return []string{strconv.FormatBool(true)}, nil
 		}
 		for _, ingressRule := range cnp.Spec.Ingress {
 			if peerHasServiceAccounts(ingressRule.From) {
-				return []string{HasServiceAccountsPeer}, nil
+				return []string{strconv.FormatBool(true)}, nil
 			}
 			if peerHasServiceAccounts(ingressRule.AppliedTo) {
-				return []string{HasServiceAccountsPeer}, nil
+				return []string{strconv.FormatBool(true)}, nil
 			}
 		}
 		for _, egressRule := range cnp.Spec.Egress {
 			if peerHasServiceAccounts(egressRule.To) {
-				return []string{HasServiceAccountsPeer}, nil
+				return []string{strconv.FormatBool(true)}, nil
 			}
 			if peerHasServiceAccounts(egressRule.AppliedTo) {
-				return []string{HasServiceAccountsPeer}, nil
+				return []string{strconv.FormatBool(true)}, nil
 			}
 		}
-		return []string{}, nil
+		return []string{strconv.FormatBool(false)}, nil
 	},
 }