Address comments

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

docs/antrea-network-policy.md

@@ -1115,7 +1115,7 @@ the Service referred in `ServiceReference`.
 
 ## ServiceAccount based selection
 
-Antrea ClusterNetworkPolicy accepts a `serviceAccount` field to select all Pods that have been assigned the 
+Antrea ClusterNetworkPolicy accepts a `serviceAccount` field to select all Pods that have been assigned the
 ServiceAccount selected by this field. This field could be used in `appliedTo`, ingress `from` and egress `to` section.
 No matter `serviceAccount` is used in which sections, it cannot be used with any other fields.
 

pkg/apis/crd/v1alpha1/types.go

@@ -597,7 +597,7 @@ type TierList struct {
 	Items []Tier `json:"items"`
 }
 
-// NamespacedName selects a specific Object by using Name + Namespace.
+// NamespacedName refers to a Namespace scoped resource.
 // All fields must be used together.
 type NamespacedName struct {
 	Name      string `json:"name,omitempty"`

pkg/controller/grouping/group_entity_index.go

@@ -35,6 +35,10 @@ import (
 const (
 	// Cluster scoped selectors are stored under empty Namespace in the selectorItemIndex.
 	emptyNamespace = ""
+	// Antrea could add custom labels using CustomLabelKeyPrefix+CustomLabelKeyXXX as
+	// the label key to entities for internal process.
+	CustomLabelKeyPrefix         = "internal.antrea.io/"
+	CustomLabelKeyServiceAccount = "service-account"
 )
 
 var (
@@ -358,22 +362,14 @@ func (i *GroupEntityIndex) deleteEntityFromLabelItem(label, entity string) *labe
 
 // createLabelItem creates a labelItem based on the provided entityItem.
 // It's called when there is no existing labelItem for a label set.
-func (i *GroupEntityIndex) createLabelItem(entityType entityType, eItem *entityItem) *labelItem {
-	// In addCustomLabels func, we write custom labels into labelItem.labels. To avoid
-	// change the original labels and data race, use copy of
-	// eItem.entity.GetLabels() in labelItem.labels instead of the pointer.
-	entityLabelsCopy := map[string]string{}
-	for k, v := range eItem.entity.GetLabels() {
-		entityLabelsCopy[k] = v
-	}
+func (i *GroupEntityIndex) createLabelItem(entityType entityType, eItem *entityItem, labels map[string]string) *labelItem {
 	lItem := &labelItem{
-		labels:           entityLabelsCopy,
+		labels:           labels,
 		namespace:        eItem.entity.GetNamespace(),
 		entityType:       entityType,
 		entityItemKeys:   sets.NewString(),
 		selectorItemKeys: sets.NewString(),
 	}
-	addCustomLabels(lItem, entityType, eItem.entity)
 	// Create the labelItem.
 	i.labelItems[eItem.labelItemKey] = lItem
 	// Add it to the labelItemIndex.
@@ -405,16 +401,23 @@ func (i *GroupEntityIndex) createLabelItem(entityType entityType, eItem *entityI
 }
 
 func (i *GroupEntityIndex) AddPod(pod *v1.Pod) {
-	i.addEntity(podEntityType, pod)
+	// Create a new map to add custom labels to avoid changing the original labels and
+	// introducing data race.
+	labels := map[string]string{}
+	for k, v := range pod.GetLabels() {
+		labels[k] = v
+	}
+	labels[CustomLabelKeyPrefix+CustomLabelKeyServiceAccount] = pod.Spec.ServiceAccountName
+	i.addEntity(podEntityType, pod, labels)
 }
 
 func (i *GroupEntityIndex) AddExternalEntity(ee *v1alpha2.ExternalEntity) {
-	i.addEntity(externalEntityType, ee)
+	i.addEntity(externalEntityType, ee, ee.Labels)
 }
 
-func (i *GroupEntityIndex) addEntity(entityType entityType, entity metav1.Object) {
+func (i *GroupEntityIndex) addEntity(entityType entityType, entity metav1.Object, labels map[string]string) {
 	eKey := getEntityItemKey(entityType, entity)
-	lKey := getLabelItemKey(entityType, entity)
+	lKey := getLabelItemKey(entityType, entity, labels)
 	var oldLabelItem *labelItem
 	var entityUpdated bool
 
@@ -451,7 +454,7 @@ func (i *GroupEntityIndex) addEntity(entityType entityType, entity metav1.Object
 	// Create a labelItem if it doesn't exist.
 	lItem, exists := i.labelItems[lKey]
 	if !exists {
-		lItem = i.createLabelItem(entityType, eItem)
+		lItem = i.createLabelItem(entityType, eItem, labels)
 	}
 	lItem.entityItemKeys.Insert(eKey)
 
@@ -771,8 +774,8 @@ func getEntityItemKeyByName(entityType entityType, namespace, name string) strin
 }
 
 // getLabelItemKey returns the label key used in labelItems.
-func getLabelItemKey(entityType entityType, obj metav1.Object) string {
-	return fmt.Sprint(entityType) + "/" + obj.GetNamespace() + "/" + getAllLabelsKey(entityType, obj)
+func getLabelItemKey(entityType entityType, obj metav1.Object, allLabels map[string]string) string {
+	return fmt.Sprint(entityType) + "/" + obj.GetNamespace() + "/" + labels.Set(allLabels).String()
 }
 
 // getGroupItemKey returns the group key used in groupItems.
@@ -784,14 +787,3 @@ func getGroupItemKey(groupType GroupType, name string) string {
 func getSelectorItemKey(selector *types.GroupSelector) string {
 	return selector.NormalizedName
 }
-
-// addCustomLabels adds custom labels to labelItem.labels.
-// Custom labels include: ServiceAccountLabel.
-func addCustomLabels(labelItem *labelItem, entityType entityType, obj metav1.Object) {
-	if key, value := getServiceAccountLabel(entityType, obj); key != "" {
-		if labelItem.labels == nil {
-			labelItem.labels = make(map[string]string)
-		}
-		labelItem.labels[key] = value
-	}
-}

pkg/controller/networkpolicy/clusternetworkpolicy.go

@@ -283,14 +283,20 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *crdv1alpha1.C
 	var atgForNamespace []string
 	if hasPerNamespaceRule && len(cnp.Spec.AppliedTo) > 0 {
 		for _, at := range cnp.Spec.AppliedTo {
-			translatedAT := n.translateServiceAccountInPeer(at)
-			affectedNS, selectors := n.getAffectedNamespacesForAppliedTo(translatedAT)
-			affectedNamespaceSelectors = append(affectedNamespaceSelectors, selectors...)
-			for _, ns := range affectedNS {
-				atg := n.createAppliedToGroup(ns, translatedAT.PodSelector, nil, translatedAT.ExternalEntitySelector)
+			if at.ServiceAccount != nil {
+				atg := n.createAppliedToGroup(at.ServiceAccount.Namespace, serviceAccountNameToPodSelector(at.ServiceAccount.Name), nil, nil)
 				atgNamesSet.Insert(atg)
-				clusterAppliedToAffectedNS = append(clusterAppliedToAffectedNS, ns)
+				clusterAppliedToAffectedNS = append(clusterAppliedToAffectedNS, at.ServiceAccount.Namespace)
 				atgForNamespace = append(atgForNamespace, atg)
+			} else {
+				affectedNS, selectors := n.getAffectedNamespacesForAppliedTo(at)
+				affectedNamespaceSelectors = append(affectedNamespaceSelectors, selectors...)
+				for _, ns := range affectedNS {
+					atg := n.createAppliedToGroup(ns, at.PodSelector, nil, at.ExternalEntitySelector)
+					atgNamesSet.Insert(atg)
+					clusterAppliedToAffectedNS = append(clusterAppliedToAffectedNS, ns)
+					atgForNamespace = append(atgForNamespace, atg)
+				}
 			}
 		}
 	}
@@ -343,14 +349,20 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *crdv1alpha1.C
 				} else {
 					// Create a rule for each affected Namespace of appliedTo at rule level
 					for _, at := range cnpRule.AppliedTo {
-						translatedAT := n.translateServiceAccountInPeer(at)
-						affectedNS, selectors := n.getAffectedNamespacesForAppliedTo(translatedAT)
-						affectedNamespaceSelectors = append(affectedNamespaceSelectors, selectors...)
-						for _, ns := range affectedNS {
-							atg := n.createAppliedToGroup(ns, translatedAT.PodSelector, nil, translatedAT.ExternalEntitySelector)
+						if at.ServiceAccount != nil {
+							atg := n.createAppliedToGroup(at.ServiceAccount.Namespace, serviceAccountNameToPodSelector(at.ServiceAccount.Name), nil, nil)
 							atgNamesSet.Insert(atg)
 							klog.V(4).Infof("Adding a new per-namespace rule with appliedTo %v for rule %d of %s", atg, idx, cnp.Name)
-							addRule(n.toNamespacedPeerForCRD(perNSPeers, ns), direction, []string{atg})
+							addRule(n.toNamespacedPeerForCRD(perNSPeers, at.ServiceAccount.Namespace), direction, []string{atg})
+						} else {
+							affectedNS, selectors := n.getAffectedNamespacesForAppliedTo(at)
+							affectedNamespaceSelectors = append(affectedNamespaceSelectors, selectors...)
+							for _, ns := range affectedNS {
+								atg := n.createAppliedToGroup(ns, at.PodSelector, nil, at.ExternalEntitySelector)
+								atgNamesSet.Insert(atg)
+								klog.V(4).Infof("Adding a new per-namespace rule with appliedTo %v for rule %d of %s", atg, idx, cnp.Name)
+								addRule(n.toNamespacedPeerForCRD(perNSPeers, ns), direction, []string{atg})
+							}
 						}
 					}
 				}
@@ -385,21 +397,11 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *crdv1alpha1.C
 	return internalNetworkPolicy
 }
 
-// translateServiceAccountInPeer translates a crdv1alpha1.NetworkPolicyPeer. If the
-// original NetworkPolicyPeer has ServiceAccount, it will be translated to a
-// NetworkPolicyPeer with PodSelector+NamespaceSelector. Otherwise, the original
-// NetworkPolicyPeer will be returned.
-func (n *NetworkPolicyController) translateServiceAccountInPeer(originalPeer crdv1alpha1.NetworkPolicyPeer) crdv1alpha1.NetworkPolicyPeer {
-	if originalPeer.ServiceAccount == nil {
-		return originalPeer
-	}
-	return crdv1alpha1.NetworkPolicyPeer{
-		PodSelector: &metav1.LabelSelector{
-			MatchLabels: map[string]string{grouping.CustomLabelKeyPrefix + grouping.ServiceAccountLabelKey: originalPeer.ServiceAccount.Name},
-		},
-		NamespaceSelector: &metav1.LabelSelector{
-			MatchLabels: map[string]string{"kubernetes.io/metadata.name": originalPeer.ServiceAccount.Namespace},
-		},
+// serviceAccountNameToPodSelector returns a PodSelector which could be used to
+// select Pods based on their ServiceAccountName.
+func serviceAccountNameToPodSelector(saName string) *metav1.LabelSelector {
+	return &metav1.LabelSelector{
+		MatchLabels: map[string]string{grouping.CustomLabelKeyPrefix + grouping.CustomLabelKeyServiceAccount: saName},
 	}
 }
 
@@ -430,9 +432,10 @@ func (n *NetworkPolicyController) processClusterAppliedTo(appliedTo []crdv1alpha
 		var atg string
 		if at.Group != "" {
 			atg = n.processAppliedToGroupForCG(at.Group)
+		} else if at.ServiceAccount != nil {
+			atg = n.createAppliedToGroup(at.ServiceAccount.Namespace, serviceAccountNameToPodSelector(at.ServiceAccount.Name), nil, nil)
 		} else {
-			translatedAT := n.translateServiceAccountInPeer(at)
-			atg = n.createAppliedToGroup("", translatedAT.PodSelector, translatedAT.NamespaceSelector, translatedAT.ExternalEntitySelector)
+			atg = n.createAppliedToGroup("", at.PodSelector, at.NamespaceSelector, at.ExternalEntitySelector)
 		}
 		if atg != "" {
 			appliedToGroupNames = append(appliedToGroupNames, atg)

pkg/controller/networkpolicy/clusternetworkpolicy_test.go

@@ -41,7 +41,7 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 	nsA := v1.Namespace{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:   "nsA",
-			Labels: map[string]string{"foo1": "bar1", "kubernetes.io/metadata.name": "nsA"},
+			Labels: map[string]string{"foo1": "bar1"},
 		},
 	}
 	nsB := v1.Namespace{
@@ -79,13 +79,10 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 	selectorA := metav1.LabelSelector{MatchLabels: map[string]string{"foo1": "bar1"}}
 	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{"internal.antrea.io/service-account": saA.Name}}
-	selectorE := metav1.LabelSelector{MatchLabels: map[string]string{"kubernetes.io/metadata.name": saA.Namespace}}
 
 	labelSelectorA, _ := metav1.LabelSelectorAsSelector(&selectorA)
 	labelSelectorB, _ := metav1.LabelSelectorAsSelector(&selectorB)
-	labelSelectorE, _ := metav1.LabelSelectorAsSelector(&selectorE)
 	cgA := crdv1alpha3.ClusterGroup{
 		ObjectMeta: metav1.ObjectMeta{Name: "cgA", UID: "uidA"},
 		Spec: crdv1alpha3.GroupSpec{
@@ -1016,7 +1013,7 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 						Action:   &dropAction,
 					},
 				},
-				AppliedToGroups: []string{getNormalizedUID(toGroupSelector("", &selectorD, &selectorE, nil).NormalizedName)},
+				AppliedToGroups: []string{getNormalizedUID(toGroupSelector("nsA", &selectorD, nil, nil).NormalizedName)},
 			},
 			expectedAppliedToGroups: 1,
 			expectedAddressGroups:   0,
@@ -1061,7 +1058,7 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 					{
 						Direction: controlplane.DirectionOut,
 						To: controlplane.NetworkPolicyPeer{
-							AddressGroups: []string{getNormalizedUID(toGroupSelector("", &selectorD, &selectorE, nil).NormalizedName)},
+							AddressGroups: []string{getNormalizedUID(toGroupSelector("nsA", &selectorD, nil, nil).NormalizedName)},
 						},
 						Priority: 0,
 						Action:   &dropAction,
@@ -1116,10 +1113,10 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 						},
 						Priority:        0,
 						Action:          &dropAction,
-						AppliedToGroups: []string{getNormalizedUID(toGroupSelector("", &selectorD, &selectorE, nil).NormalizedName)},
+						AppliedToGroups: []string{getNormalizedUID(toGroupSelector("nsA", &selectorD, nil, nil).NormalizedName)},
 					},
 				},
-				AppliedToGroups:  []string{getNormalizedUID(toGroupSelector("", &selectorD, &selectorE, nil).NormalizedName)},
+				AppliedToGroups:  []string{getNormalizedUID(toGroupSelector("nsA", &selectorD, nil, nil).NormalizedName)},
 				AppliedToPerRule: true,
 			},
 			expectedAppliedToGroups: 1,
@@ -1188,17 +1185,16 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 						},
 						Priority:        1,
 						Action:          &allowAction,
-						AppliedToGroups: []string{getNormalizedUID(toGroupSelector("", &selectorD, &selectorE, nil).NormalizedName)},
+						AppliedToGroups: []string{getNormalizedUID(toGroupSelector("nsA", &selectorD, nil, nil).NormalizedName)},
 					},
 				},
 				AppliedToGroups: []string{
 					getNormalizedUID(toGroupSelector("nsA", &selectorD, nil, nil).NormalizedName),
-					getNormalizedUID(toGroupSelector("", &selectorD, &selectorE, nil).NormalizedName),
 				},
 				AppliedToPerRule:      true,
-				PerNamespaceSelectors: []labels.Selector{labelSelectorE},
+				PerNamespaceSelectors: []labels.Selector{},
 			},
-			expectedAppliedToGroups: 2,
+			expectedAppliedToGroups: 1,
 			expectedAddressGroups:   2,
 		},
 	}
@@ -1210,7 +1206,6 @@ func TestProcessClusterNetworkPolicy(t *testing.T) {
 			c.namespaceStore.Add(&nsA)
 			c.namespaceStore.Add(&nsB)
 			c.serviceStore.Add(&svcA)
-			c.serviceAccountStore.Add(&saA)
 			if tt.inputPolicy.Spec.Tier != "" {
 				c.tierStore.Add(&tierA)
 			}

pkg/controller/networkpolicy/crd_utils.go

@@ -114,9 +114,11 @@ func (n *NetworkPolicyController) toAntreaPeerForCRD(peers []v1alpha1.NetworkPol
 			}
 		} else if peer.FQDN != "" {
 			fqdns = append(fqdns, peer.FQDN)
+		} else if peer.ServiceAccount != nil {
+			normalizedUID := n.createAddressGroup(peer.ServiceAccount.Namespace, serviceAccountNameToPodSelector(peer.ServiceAccount.Name), nil, nil)
+			addressGroups = append(addressGroups, normalizedUID)
 		} else {
-			translatedPeer := n.translateServiceAccountInPeer(peer)
-			normalizedUID := n.createAddressGroup(np.GetNamespace(), translatedPeer.PodSelector, translatedPeer.NamespaceSelector, translatedPeer.ExternalEntitySelector)
+			normalizedUID := n.createAddressGroup(np.GetNamespace(), peer.PodSelector, peer.NamespaceSelector, peer.ExternalEntitySelector)
 			addressGroups = append(addressGroups, normalizedUID)
 		}
 	}

pkg/controller/networkpolicy/networkpolicy_controller_test.go

@@ -71,7 +71,6 @@ type networkPolicyController struct {
 	*NetworkPolicyController
 	namespaceStore             cache.Store
 	serviceStore               cache.Store
-	serviceAccountStore        cache.Store
 	networkPolicyStore         cache.Store
 	cnpStore                   cache.Store
 	tierStore                  cache.Store
@@ -129,7 +128,6 @@ func newController(objects ...runtime.Object) (*fake.Clientset, *networkPolicyCo
 		npController,
 		informerFactory.Core().V1().Namespaces().Informer().GetStore(),
 		informerFactory.Core().V1().Services().Informer().GetStore(),
-		informerFactory.Core().V1().ServiceAccounts().Informer().GetStore(),
 		informerFactory.Networking().V1().NetworkPolicies().Informer().GetStore(),
 		crdInformerFactory.Crd().V1alpha1().ClusterNetworkPolicies().Informer().GetStore(),
 		crdInformerFactory.Crd().V1alpha1().Tiers().Informer().GetStore(),
@@ -195,7 +193,6 @@ func newControllerWithoutEventHandler(k8sObjects, crdObjects []runtime.Object) (
 		npController,
 		informerFactory.Core().V1().Namespaces().Informer().GetStore(),
 		informerFactory.Core().V1().Services().Informer().GetStore(),
-		informerFactory.Core().V1().ServiceAccounts().Informer().GetStore(),
 		informerFactory.Networking().V1().NetworkPolicies().Informer().GetStore(),
 		cnpInformer.Informer().GetStore(),
 		tierInformer.Informer().GetStore(),

pkg/controller/networkpolicy/validate.go

@@ -470,7 +470,7 @@ func (v *antreaPolicyValidator) validateAppliedTo(ingress, egress []crdv1alpha1.
 				return "group cannot be set with other peers in appliedTo", false
 			}
 			if eachAppliedTo.ServiceAccount != nil && appliedToFieldsNum > 1 {
-				return "serviceAccounts cannot be set with other peers in appliedTo", false
+				return "serviceAccount cannot be set with other peers in appliedTo", false
 			}
 		}
 		return "", true
@@ -509,7 +509,7 @@ func (v *antreaPolicyValidator) validatePeers(ingress, egress []crdv1alpha1.Rule
 				return "group cannot be set with other peers in rules", false
 			}
 			if peer.ServiceAccount != nil && peerFieldsNum > 1 {
-				return "serviceAccounts cannot be set with other peers in rules", false
+				return "serviceAccount cannot be set with other peers in rules", false
 			}
 		}
 		return "", true