Fix deadlock issue and address comments

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

pkg/agent/controller/networkpolicy/cache.go

@@ -383,7 +383,7 @@ func toServicesIndexFunc(obj interface{}) ([]string, error) {
 func (r *ruleCache) appliedToServicesIndexFunc(obj interface{}) ([]string, error) {
 	rule := obj.(*rule)
 	appliedToSvcNamespacedName := sets.String{}
-	memberSet, exist := r.unionAppliedToGroups(rule.AppliedToGroups)
+	memberSet, exist := r.unionAppliedToGroupsLocked(rule.AppliedToGroups)
 	if !exist {
 		return []string{}, nil
 	}
@@ -477,8 +477,9 @@ func (c *ruleCache) processGroupIDUpdates() {
 			for _, toSvcRule := range toSvcRules {
 				c.dirtyRuleHandler(toSvcRule.(*rule).ID)
 			}
-
+			c.appliedToSetLock.RLock()
 			appliedToSvcRules, err := c.rules.ByIndex(appliedToServicesIndex, svcStr)
+			c.appliedToSetLock.RUnlock()
 			if err != nil {
 				continue
 			}
@@ -926,6 +927,13 @@ func (c *ruleCache) unionAppliedToGroups(groupNames []string) (v1beta.GroupMembe
 	c.appliedToSetLock.RLock()
 	defer c.appliedToSetLock.RUnlock()
 
+	return c.unionAppliedToGroupsLocked(groupNames)
+}
+
+// unionAppliedToGroupsLocked gets the union of pods of the provided appliedTo groups.
+// If any group is found, the union and true will be returned. Otherwise an empty set and false will be returned.
+// It's caller's responsibility to lock and unlock c.appliedToSetLock.
+func (c *ruleCache) unionAppliedToGroupsLocked(groupNames []string) (v1beta.GroupMemberSet, bool) {
 	anyExists := false
 	set := v1beta.NewGroupMemberSet()
 	for _, groupName := range groupNames {

pkg/apis/controlplane/types.go

@@ -88,7 +88,8 @@ type GroupMember struct {
 	IPs []IPAddress
 	// Ports is the list NamedPort of the GroupMember.
 	Ports []NamedPort
-	// Service maintains the reference to the Service.
+	// Service is the reference to the Service. It can only be used in an AppliedTo
+	// Group and only a NodePort type Service can be referred by this field.
 	Service *ServiceReference
 }
 

pkg/apis/controlplane/v1beta2/types.go

@@ -88,7 +88,8 @@ type GroupMember struct {
 	Ports []NamedPort `json:"ports,omitempty" protobuf:"bytes,4,rep,name=ports"`
 	// Node maintains the reference to the Node.
 	Node *NodeReference `json:"node,omitempty" protobuf:"bytes,5,opt,name=node"`
-	// Service maintains the reference to the Service.
+	// Service is the reference to the Service. It can only be used in an AppliedTo
+	// Group and only a NodePort type Service can be referred by this field.
 	Service *ServiceReference `json:"service,omitempty" protobuf:"bytes,6,opt,name=service"`
 }
 

pkg/apis/crd/v1alpha1/types.go

@@ -450,9 +450,10 @@ type NetworkPolicyPeer struct {
 	// A NodeSelector cannot be set in AppliedTo field or set with any other selector.
 	// +optional
 	NodeSelector *metav1.LabelSelector `json:"nodeSelector,omitempty"`
-	// Select a certain Service which match the NamespacedName.
+	// Select a certain Service which matches the NamespacedName.
 	// A Service can only be set in either policy level AppliedTo field in a policy
 	// that only has ingress rules or rule level AppliedTo field in an ingress rule.
+	// Only a NodePort Service can be referred by this field.
 	// Cannot be set with any other selector.
 	// +optional
 	Service *NamespacedName `json:"service,omitempty"`

pkg/controller/types/networkpolicy.go

@@ -50,7 +50,8 @@ type AppliedToGroup struct {
 	// Selector describes how the group selects pods.
 	// Selector can't be used with Service.
 	Selector *GroupSelector
-	// Service describes the Service this group selects.
+	// Service refers to the Service this group selects. Only a NodePort type Service
+	// can be referred by this field.
 	// Service can't be used with Selector.
 	Service *controlplane.ServiceReference
 	// GroupMemberByNode is a mapping from nodeName to a set of GroupMembers on the Node,