Fix nil pointer dereference when ClusterGroup/Group is used (#6088)

When an AppliedToGroup or AddressGroup is derived from a ClusterGroup or
Group, we used the existence of the source Group in the internal group
storage as the indicator of the type of AppliedToGroup or AddressGroup.
After the source Group is deleted, the AppliedToGroup or AddressGroup
was considered as a Group with its own selector mistakenly. Accessing
its selector would panic due to nil pointer dereference.

This patch makes the type of AppliedToGroup and AddressGroup more
explicit by adding a field "SourceGroup" to indicate it. If the source
Group can't be found in the storage, we just return nil to indicate the
Group selects nothing at the moment.

Signed-off-by: Quan Tian <qtian@vmware.com>

pkg/controller/networkpolicy/clustergroup.go

@@ -281,17 +281,17 @@ func (c *NetworkPolicyController) triggerParentGroupUpdates(grp string) {
 
 // triggerDerivedGroupUpdates triggers processing of AppliedToGroup and AddressGroup derived from the provided group.
 func (c *NetworkPolicyController) triggerDerivedGroupUpdates(grp string) {
-	_, exists, _ := c.appliedToGroupStore.Get(grp)
-	if exists {
+	groups, _ := c.appliedToGroupStore.GetByIndex(store.SourceGroupIndex, grp)
+	for _, group := range groups {
 		// It's fine if the group is deleted after checking its existence as syncAppliedToGroup will do nothing when it
 		// doesn't find the group.
-		c.enqueueAppliedToGroup(grp)
+		c.enqueueAppliedToGroup(group.(*antreatypes.AppliedToGroup).Name)
 	}
-	_, exists, _ = c.addressGroupStore.Get(grp)
-	if exists {
+	groups, _ = c.addressGroupStore.GetByIndex(store.SourceGroupIndex, grp)
+	for _, group := range groups {
 		// It's fine if the group is deleted after checking its existence as syncAddressGroup will do nothing when it
 		// doesn't find the group.
-		c.enqueueAddressGroup(grp)
+		c.enqueueAddressGroup(group.(*antreatypes.AddressGroup).Name)
 	}
 }
 

pkg/controller/networkpolicy/clusternetworkpolicy.go

@@ -613,7 +613,7 @@ func (n *NetworkPolicyController) processRefGroupOrClusterGroup(g, namespace str
 	// up if the Group/ClusterGroup becomes ipBlocks-only.
 	createAddrGroup, ipb := n.processInternalGroupForRule(intGrp)
 	if createAddrGroup {
-		ag := &antreatypes.AddressGroup{UID: intGrp.UID, Name: key}
+		ag := &antreatypes.AddressGroup{UID: intGrp.UID, Name: key, SourceGroup: key}
 		return ag, ipb
 	}
 	return nil, ipb

pkg/controller/networkpolicy/clusternetworkpolicy_test.go

@@ -1873,8 +1873,12 @@ func TestProcessRefGroupOrClusterGroup(t *testing.T) {
 		{
 			name:           "cg-with-selector",
 			inputGroupName: cgA.Name,
-			expectedAG:     &antreatypes.AddressGroup{UID: cgA.UID, Name: cgA.Name},
-			expectedIPB:    nil,
+			expectedAG: &antreatypes.AddressGroup{
+				UID:         cgA.UID,
+				Name:        cgA.Name,
+				SourceGroup: cgA.Name,
+			},
+			expectedIPB: nil,
 		},
 		{
 			name:           "cg-with-selector-not-found",
@@ -1907,7 +1911,11 @@ func TestProcessRefGroupOrClusterGroup(t *testing.T) {
 		{
 			name:           "nested-cg-with-ipblock-and-selector",
 			inputGroupName: cgNested2.Name,
-			expectedAG:     &antreatypes.AddressGroup{UID: cgNested2.UID, Name: cgNested2.Name},
+			expectedAG: &antreatypes.AddressGroup{
+				UID:         cgNested2.UID,
+				Name:        cgNested2.Name,
+				SourceGroup: cgNested2.Name,
+			},
 			expectedIPB: []controlplane.IPBlock{
 				{
 					CIDR:   *cidrIPNet,
@@ -1926,8 +1934,12 @@ func TestProcessRefGroupOrClusterGroup(t *testing.T) {
 			name:           "g-with-selector",
 			inputNamespace: gA.Namespace,
 			inputGroupName: gA.Name,
-			expectedAG:     &antreatypes.AddressGroup{UID: gA.UID, Name: fmt.Sprintf("%s/%s", gA.Namespace, gA.Name)},
-			expectedIPB:    nil,
+			expectedAG: &antreatypes.AddressGroup{
+				UID:         gA.UID,
+				Name:        fmt.Sprintf("%s/%s", gA.Namespace, gA.Name),
+				SourceGroup: fmt.Sprintf("%s/%s", gA.Namespace, gA.Name),
+			},
+			expectedIPB: nil,
 		},
 		{
 			name:           "non-existing-group",

pkg/controller/networkpolicy/crd_utils.go

@@ -322,7 +322,7 @@ func (n *NetworkPolicyController) createAppliedToGroupForGroup(namespace, group
 		klog.V(2).InfoS("Group with IPBlocks can not be used as AppliedTo", "Group", key)
 		return nil
 	}
-	return &antreatypes.AppliedToGroup{UID: intGrp.UID, Name: key}
+	return &antreatypes.AppliedToGroup{UID: intGrp.UID, Name: key, SourceGroup: key}
 }
 
 // getTierPriority retrieves the priority associated with the input Tier name.

pkg/controller/networkpolicy/crd_utils_test.go

@@ -550,9 +550,13 @@ func TestCreateAppliedToGroupsForGroup(t *testing.T) {
 			expectedATG: nil,
 		},
 		{
-			name:        "cluster group with selectors",
-			inputGroup:  clusterGroupWithSelector.Name,
-			expectedATG: &antreatypes.AppliedToGroup{UID: clusterGroupWithSelector.UID, Name: clusterGroupWithSelector.Name},
+			name:       "cluster group with selectors",
+			inputGroup: clusterGroupWithSelector.Name,
+			expectedATG: &antreatypes.AppliedToGroup{
+				UID:         clusterGroupWithSelector.UID,
+				Name:        clusterGroupWithSelector.Name,
+				SourceGroup: clusterGroupWithSelector.Name,
+			},
 		},
 		{
 			name:           "empty group name",
@@ -576,7 +580,11 @@ func TestCreateAppliedToGroupsForGroup(t *testing.T) {
 			name:           "group with selectors",
 			inputNamespace: groupWithSelector.Namespace,
 			inputGroup:     groupWithSelector.Name,
-			expectedATG:    &antreatypes.AppliedToGroup{UID: groupWithSelector.UID, Name: fmt.Sprintf("%s/%s", groupWithSelector.Namespace, groupWithSelector.Name)},
+			expectedATG: &antreatypes.AppliedToGroup{
+				UID:         groupWithSelector.UID,
+				Name:        fmt.Sprintf("%s/%s", groupWithSelector.Namespace, groupWithSelector.Name),
+				SourceGroup: fmt.Sprintf("%s/%s", groupWithSelector.Namespace, groupWithSelector.Name),
+			},
 		},
 	}
 	for _, tt := range tests {

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -623,7 +623,7 @@ func (n *NetworkPolicyController) createAddressGroup(namespace string, podSelect
 	addressGroup := &antreatypes.AddressGroup{
 		UID:      types.UID(normalizedUID),
 		Name:     normalizedUID,
-		Selector: *groupSelector,
+		Selector: groupSelector,
 	}
 	return addressGroup
 }
@@ -1104,6 +1104,7 @@ func (n *NetworkPolicyController) syncAddressGroup(key string) error {
 		Name:         addressGroup.Name,
 		UID:          addressGroup.UID,
 		Selector:     addressGroup.Selector,
+		SourceGroup:  addressGroup.SourceGroup,
 		GroupMembers: memberSet,
 		SpanMeta:     antreatypes.SpanMeta{NodeNames: addrGroupNodeNames},
 	}
@@ -1123,17 +1124,23 @@ func (c *NetworkPolicyController) getNodeMemberSet(selector labels.Selector) con
 // getAddressGroupMemberSet knows how to construct a GroupMemberSet that contains
 // all the entities selected by an AddressGroup.
 func (n *NetworkPolicyController) getAddressGroupMemberSet(g *antreatypes.AddressGroup) controlplane.GroupMemberSet {
-	// Check if an internal Group object exists corresponding to this AddressGroup.
-	groupObj, found, _ := n.internalGroupStore.Get(g.Name)
-	if found {
-		// This AddressGroup is derived from a ClusterGroup.
-		// In case the ClusterGroup is defined by a mix of childGroup with selectors and
-		// childGroup with ipBlocks, this function only returns the aggregated GroupMemberSet
-		// computed from childGroup with selectors, as ipBlocks will be processed differently.
-		group := groupObj.(*antreatypes.Group)
-		members, _ := n.getInternalGroupMembers(group)
-		return members
+	// This AddressGroup is derived from a ClusterGroup/Group.
+	if g.SourceGroup != "" {
+		// Check if an internal Group object exists corresponding to this AddressGroup.
+		groupObj, found, _ := n.internalGroupStore.Get(g.SourceGroup)
+		if found {
+			// In case the ClusterGroup/Group is defined by a mix of childGroup with selectors and
+			// childGroup with ipBlocks, this function only returns the aggregated GroupMemberSet
+			// computed from childGroup with selectors, as ipBlocks will be processed differently.
+			group := groupObj.(*antreatypes.Group)
+			members, _ := n.getInternalGroupMembers(group)
+			return members
+		}
+		// The internal Group doesn't exist yet or has been deleted. The AddressGroup selects nothing at the moment.
+		// Once the internalGroup is created, the AddressGroup will be resynced.
+		return nil
 	}
+	// Selector can't be nil when it reaches here.
 	if g.Selector.NodeSelector != nil {
 		return n.getNodeMemberSet(g.Selector.NodeSelector)
 	}
@@ -1304,10 +1311,11 @@ func (n *NetworkPolicyController) syncAppliedToGroup(key string) error {
 		if err != nil {
 			klog.ErrorS(err, "Error when getting AppliedTo workloads for AppliedToGroup", "AppliedToGroup", appliedToGroup.Name)
 			updatedAppliedToGroup = &antreatypes.AppliedToGroup{
-				UID:       appliedToGroup.UID,
-				Name:      appliedToGroup.Name,
-				Selector:  appliedToGroup.Selector,
-				SyncError: err,
+				UID:         appliedToGroup.UID,
+				Name:        appliedToGroup.Name,
+				Selector:    appliedToGroup.Selector,
+				SourceGroup: appliedToGroup.SourceGroup,
+				SyncError:   err,
 			}
 		} else {
 			scheduledPodNum, scheduledExtEntityNum := 0, 0
@@ -1346,6 +1354,7 @@ func (n *NetworkPolicyController) syncAppliedToGroup(key string) error {
 				UID:               appliedToGroup.UID,
 				Name:              appliedToGroup.Name,
 				Selector:          appliedToGroup.Selector,
+				SourceGroup:       appliedToGroup.SourceGroup,
 				GroupMemberByNode: memberSetByNode,
 				SpanMeta:          antreatypes.SpanMeta{NodeNames: appGroupNodeNames},
 			}
@@ -1362,12 +1371,14 @@ func (n *NetworkPolicyController) syncAppliedToGroup(key string) error {
 // getAppliedToWorkloads returns a list of workloads (Pods and ExternalEntities) selected by an AppliedToGroup
 // for standalone selectors or corresponding to a ClusterGroup.
 func (n *NetworkPolicyController) getAppliedToWorkloads(g *antreatypes.AppliedToGroup) ([]*v1.Pod, []*v1alpha2.ExternalEntity, error) {
-	// Check if an internal Group object exists corresponding to this AppliedToGroup
-	group, found, _ := n.internalGroupStore.Get(g.Name)
-	if found {
-		// This AppliedToGroup is derived from a ClusterGroup.
-		grp := group.(*antreatypes.Group)
-		return n.getInternalGroupWorkloads(grp)
+	// This AppliedToGroup is derived from a ClusterGroup/Group.
+	if g.SourceGroup != "" {
+		// Check if an internal Group object exists corresponding to this AppliedToGroup
+		group, found, _ := n.internalGroupStore.Get(g.Name)
+		if found {
+			grp := group.(*antreatypes.Group)
+			return n.getInternalGroupWorkloads(grp)
+		}
 	}
 	pods, ees := n.groupingInterface.GetEntities(appliedToGroupType, g.Name)
 	return pods, ees, nil
@@ -1579,8 +1590,8 @@ func (n *NetworkPolicyController) syncInternalNetworkPolicy(key *controlplane.Ne
 			n.addressGroupStore.Create(addressGroup)
 			// For an AddressGroup that selects Nodes via nodeSelector, we calculate its members via NodeLister
 			// directly, instead of groupingInterface which handles Pod and ExternalEntity currently.
-			if addressGroup.Selector.NodeSelector == nil {
-				n.groupingInterface.AddGroup(addressGroupType, addressGroup.Name, &addressGroup.Selector)
+			if addressGroup.Selector != nil && addressGroup.Selector.NodeSelector == nil {
+				n.groupingInterface.AddGroup(addressGroupType, addressGroup.Name, addressGroup.Selector)
 			}
 		}