Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Automated cherry pick of #6077: Fix nil pointer dereference when ClusterGroup/Group is used #6086

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading