Skip to content

Commit

Permalink
Fix nil pointer dereference when ClusterGroup/Group is used (#6077)
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
tnqn authored Mar 8, 2024
1 parent ac8aa95 commit 17ff95a
Show file tree
Hide file tree
Showing 10 changed files with 296 additions and 77 deletions.
12 changes: 6 additions & 6 deletions pkg/controller/networkpolicy/clustergroup.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}

Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/networkpolicy/clusternetworkpolicy.go
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,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
Expand Down
22 changes: 17 additions & 5 deletions pkg/controller/networkpolicy/clusternetworkpolicy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1961,8 +1961,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",
Expand Down Expand Up @@ -1995,7 +1999,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,
Expand All @@ -2014,8 +2022,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",
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/networkpolicy/crd_utils.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
16 changes: 12 additions & 4 deletions pkg/controller/networkpolicy/crd_utils_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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 {
Expand Down
63 changes: 39 additions & 24 deletions pkg/controller/networkpolicy/networkpolicy_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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},
}
Expand All @@ -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)
}
Expand Down Expand Up @@ -1306,10 +1313,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
Expand Down Expand Up @@ -1357,6 +1365,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},
}
Expand All @@ -1373,14 +1382,20 @@ func (n *NetworkPolicyController) syncAppliedToGroup(key string) error {
// getAppliedToWorkloads returns a list of workloads (Pods, ExternalEntities or Nodes) selected by an AppliedToGroup
// for standalone selectors or Pods and ExternalEntities corresponding to a ClusterGroup.
func (n *NetworkPolicyController) getAppliedToWorkloads(g *antreatypes.AppliedToGroup) ([]*v1.Pod, []*v1alpha2.ExternalEntity, []*v1.Node, 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)
pods, ees, err := n.getInternalGroupWorkloads(grp)
return pods, ees, nil, err
// 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.SourceGroup)
if found {
grp := group.(*antreatypes.Group)
pods, ees, err := n.getInternalGroupWorkloads(grp)
return pods, ees, nil, err
}
// The internal Group doesn't exist yet or has been deleted. The AppliedToGroup selects nothing at the moment.
// Once the internalGroup is created, the AppliedToGroup will be resynced.
return nil, nil, nil, nil
}
// Selector can't be nil when it reaches here.
if g.Selector.NodeSelector != nil {
nodes, err := n.nodeLister.List(g.Selector.NodeSelector)
return nil, nil, nodes, err
Expand Down Expand Up @@ -1595,8 +1610,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)
}
}

Expand Down
Loading

0 comments on commit 17ff95a

Please sign in to comment.