From 85714bc6c8076e8f9d3706724b7027c710736f62 Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Fri, 8 Mar 2024 13:54:32 +0800 Subject: [PATCH] Fix nil pointer dereference when ClusterGroup/Group is used (#6077) 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 --- pkg/controller/networkpolicy/clustergroup.go | 12 +- .../networkpolicy/clusternetworkpolicy.go | 2 +- .../clusternetworkpolicy_test.go | 22 +- pkg/controller/networkpolicy/crd_utils.go | 2 +- .../networkpolicy/crd_utils_test.go | 16 +- .../networkpolicy/networkpolicy_controller.go | 63 +++--- .../networkpolicy_controller_test.go | 200 ++++++++++++++++-- .../networkpolicy/store/addressgroup.go | 11 +- .../networkpolicy/store/appliedtogroup.go | 12 +- pkg/controller/types/networkpolicy.go | 33 ++- 10 files changed, 296 insertions(+), 77 deletions(-) diff --git a/pkg/controller/networkpolicy/clustergroup.go b/pkg/controller/networkpolicy/clustergroup.go index 735b365c241..000759ff70d 100644 --- a/pkg/controller/networkpolicy/clustergroup.go +++ b/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) } } diff --git a/pkg/controller/networkpolicy/clusternetworkpolicy.go b/pkg/controller/networkpolicy/clusternetworkpolicy.go index 14d24e1417d..f711232bfa0 100644 --- a/pkg/controller/networkpolicy/clusternetworkpolicy.go +++ b/pkg/controller/networkpolicy/clusternetworkpolicy.go @@ -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 diff --git a/pkg/controller/networkpolicy/clusternetworkpolicy_test.go b/pkg/controller/networkpolicy/clusternetworkpolicy_test.go index 1a0d19e3921..3562c861196 100644 --- a/pkg/controller/networkpolicy/clusternetworkpolicy_test.go +++ b/pkg/controller/networkpolicy/clusternetworkpolicy_test.go @@ -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", @@ -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, @@ -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", diff --git a/pkg/controller/networkpolicy/crd_utils.go b/pkg/controller/networkpolicy/crd_utils.go index 801470383e4..f58c82ed51f 100644 --- a/pkg/controller/networkpolicy/crd_utils.go +++ b/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. diff --git a/pkg/controller/networkpolicy/crd_utils_test.go b/pkg/controller/networkpolicy/crd_utils_test.go index a572e347c28..40963a94596 100644 --- a/pkg/controller/networkpolicy/crd_utils_test.go +++ b/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 { diff --git a/pkg/controller/networkpolicy/networkpolicy_controller.go b/pkg/controller/networkpolicy/networkpolicy_controller.go index 3e83e2017f4..af1edbcf773 100644 --- a/pkg/controller/networkpolicy/networkpolicy_controller.go +++ b/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) } @@ -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 @@ -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}, } @@ -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 @@ -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) } } diff --git a/pkg/controller/networkpolicy/networkpolicy_controller_test.go b/pkg/controller/networkpolicy/networkpolicy_controller_test.go index 9459934d557..2ee35d67d12 100644 --- a/pkg/controller/networkpolicy/networkpolicy_controller_test.go +++ b/pkg/controller/networkpolicy/networkpolicy_controller_test.go @@ -41,6 +41,7 @@ import ( k8stesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/cache" "k8s.io/client-go/util/workqueue" + "k8s.io/utils/pointer" fakepolicyversioned "sigs.k8s.io/network-policy-api/pkg/client/clientset/versioned/fake" policyv1a1informers "sigs.k8s.io/network-policy-api/pkg/client/informers/externalversions" @@ -2602,8 +2603,9 @@ func TestGetAppliedToWorkloads(t *testing.T) { { name: "atg-for-cg", inATG: &antreatypes.AppliedToGroup{ - Name: cgA.Name, - UID: cgA.UID, + Name: cgA.Name, + UID: cgA.UID, + SourceGroup: cgA.Name, }, expPods: []*corev1.Pod{podA}, expEEs: emptyEEs, @@ -2612,8 +2614,9 @@ func TestGetAppliedToWorkloads(t *testing.T) { { name: "atg-for-cg-no-pod-match", inATG: &antreatypes.AppliedToGroup{ - Name: cgB.Name, - UID: cgB.UID, + Name: cgB.Name, + UID: cgB.UID, + SourceGroup: cgB.Name, }, expPods: emptyPods, expEEs: emptyEEs, @@ -2622,8 +2625,9 @@ func TestGetAppliedToWorkloads(t *testing.T) { { name: "atg-for-nested-cg-one-child-empty", inATG: &antreatypes.AppliedToGroup{ - Name: nestedCG1.Name, - UID: nestedCG1.UID, + Name: nestedCG1.Name, + UID: nestedCG1.UID, + SourceGroup: nestedCG1.Name, }, expPods: []*corev1.Pod{podA}, expEEs: emptyEEs, @@ -2632,8 +2636,9 @@ func TestGetAppliedToWorkloads(t *testing.T) { { name: "atg-for-nested-cg-both-children-match-pod", inATG: &antreatypes.AppliedToGroup{ - Name: nestedCG2.Name, - UID: nestedCG2.UID, + Name: nestedCG2.Name, + UID: nestedCG2.UID, + SourceGroup: nestedCG2.Name, }, expPods: []*corev1.Pod{podA, podB}, expEEs: emptyEEs, @@ -2642,8 +2647,9 @@ func TestGetAppliedToWorkloads(t *testing.T) { { name: "atg-for-nested-cg-children-overlap-pod", inATG: &antreatypes.AppliedToGroup{ - Name: nestedCG3.Name, - UID: nestedCG3.UID, + Name: nestedCG3.Name, + UID: nestedCG3.UID, + SourceGroup: nestedCG3.Name, }, expPods: []*corev1.Pod{podA, podB}, expEEs: emptyEEs, @@ -2747,40 +2753,45 @@ func TestGetAddressGroupMemberSet(t *testing.T) { { name: "addrgrp-for-cg", inAddrGrp: &antreatypes.AddressGroup{ - Name: cgA.Name, - UID: cgA.UID, + Name: cgA.Name, + UID: cgA.UID, + SourceGroup: cgA.Name, }, expMemberSet: podAMemberSet, }, { name: "addrgrp-for-cg-no-pod-match", inAddrGrp: &antreatypes.AddressGroup{ - Name: cgB.Name, - UID: cgB.UID, + Name: cgB.Name, + UID: cgB.UID, + SourceGroup: cgB.Name, }, expMemberSet: controlplane.GroupMemberSet{}, }, { name: "addrgrp-for-nested-cg-one-child-empty", inAddrGrp: &antreatypes.AddressGroup{ - Name: nestedCG1.Name, - UID: nestedCG1.UID, + Name: nestedCG1.Name, + UID: nestedCG1.UID, + SourceGroup: nestedCG1.Name, }, expMemberSet: podAMemberSet, }, { name: "addrgrp-for-nested-cg-both-children-match-pod", inAddrGrp: &antreatypes.AddressGroup{ - Name: nestedCG2.Name, - UID: nestedCG2.UID, + Name: nestedCG2.Name, + UID: nestedCG2.UID, + SourceGroup: nestedCG2.Name, }, expMemberSet: podABMemberSet, }, { name: "addrgrp-for-nested-cg-children-overlap-pod", inAddrGrp: &antreatypes.AddressGroup{ - Name: nestedCG3.Name, - UID: nestedCG3.UID, + Name: nestedCG3.Name, + UID: nestedCG3.UID, + SourceGroup: nestedCG3.Name, }, expMemberSet: podABMemberSet, }, @@ -3037,7 +3048,7 @@ func TestMultipleNetworkPoliciesWithSameAppliedTo(t *testing.T) { SpanMeta: antreatypes.SpanMeta{NodeNames: sets.New[string]("nodeA", "nodeB")}, // according to policyA and policyB UID: types.UID(selectorBGroupUID), Name: selectorBGroupUID, - Selector: *selectorBGroup, + Selector: selectorBGroup, GroupMembers: controlplane.NewGroupMemberSet(&controlplane.GroupMember{Pod: &controlplane.PodReference{ Name: podC.Name, Namespace: podC.Namespace, @@ -3166,7 +3177,6 @@ func checkAddressGroupNotExist(t *testing.T, c *networkPolicyController, address func TestSyncInternalNetworkPolicy(t *testing.T) { p10 := float64(10) - allowAction := v1beta1.RuleActionAllow inputPolicy := &v1beta1.ClusterNetworkPolicy{ ObjectMeta: metav1.ObjectMeta{Name: "cnpA", UID: "uidA"}, Spec: v1beta1.ClusterNetworkPolicySpec{ @@ -3490,7 +3500,6 @@ func TestSyncInternalNetworkPolicyConcurrently(t *testing.T) { func TestSyncInternalNetworkPolicyWithGroups(t *testing.T) { p10 := float64(10) - allowAction := v1beta1.RuleActionAllow podA := getPod("podA", "nsA", "nodeA", "10.0.0.1", false) podA.Labels = selectorA.MatchLabels podB := getPod("podB", "nsB", "nodeB", "10.0.0.2", false) @@ -3888,6 +3897,151 @@ func TestSyncAppliedToGroupWithNode(t *testing.T) { assert.Equal(t, expectedAppliedToGroup, gotAppliedToGroup) } +func TestClusterNetworkPolicyWithClusterGroup(t *testing.T) { + ctx := context.TODO() + podA := getPod("podA", "nsA", "nodeA", "10.0.0.1", false) + podA.Labels = map[string]string{"fooA": "barA"} + podB := getPod("podB", "nsB", "nodeB", "10.0.0.2", false) + podB.Labels = map[string]string{"fooB": "barB"} + + cgA := &v1beta1.ClusterGroup{ + ObjectMeta: metav1.ObjectMeta{Name: "cgA", UID: "cgA-uid"}, + Spec: v1beta1.GroupSpec{PodSelector: &metav1.LabelSelector{MatchLabels: podA.Labels}}, + } + cgB := &v1beta1.ClusterGroup{ + ObjectMeta: metav1.ObjectMeta{Name: "cgB", UID: "cgB-uid"}, + Spec: v1beta1.GroupSpec{PodSelector: &metav1.LabelSelector{MatchLabels: podB.Labels}}, + } + acnp := &v1beta1.ClusterNetworkPolicy{ + ObjectMeta: metav1.ObjectMeta{Name: "acnpA", UID: "acnpA-uid"}, + Spec: v1beta1.ClusterNetworkPolicySpec{ + AppliedTo: []v1beta1.AppliedTo{{Group: cgA.Name}}, + Priority: 10, + Ingress: []v1beta1.Rule{ + {From: []v1beta1.NetworkPolicyPeer{{Group: cgB.Name}}, Action: &allowAction}, + }, + }, + } + + _, npc := newController([]runtime.Object{podA, podB}, []runtime.Object{cgA, cgB, acnp}) + stopCh := make(chan struct{}) + defer close(stopCh) + npc.informerFactory.Start(stopCh) + npc.informerFactory.WaitForCacheSync(stopCh) + npc.crdInformerFactory.Start(stopCh) + npc.crdInformerFactory.WaitForCacheSync(stopCh) + go npc.Run(stopCh) + go npc.groupingController.Run(stopCh) + go npc.groupingInterface.Run(stopCh) + + expectedPolicy := &antreatypes.NetworkPolicy{ + SpanMeta: antreatypes.SpanMeta{NodeNames: sets.New(podA.Spec.NodeName)}, + UID: acnp.UID, + Name: string(acnp.UID), + SourceRef: &controlplane.NetworkPolicyReference{Type: controlplane.AntreaClusterNetworkPolicy, Name: acnp.Name, UID: acnp.UID}, + Priority: pointer.Float64(acnp.Spec.Priority), + Rules: []controlplane.NetworkPolicyRule{ + { + Direction: controlplane.DirectionIn, + From: controlplane.NetworkPolicyPeer{AddressGroups: []string{cgB.Name}}, + Action: &allowAction, + }, + }, + AppliedToGroups: []string{cgA.Name}, + TierPriority: &DefaultTierPriority, + } + expectedAppliedToGroup := &antreatypes.AppliedToGroup{ + SpanMeta: antreatypes.SpanMeta{NodeNames: sets.New(podA.Spec.NodeName)}, + UID: cgA.UID, + Name: cgA.Name, + SourceGroup: cgA.Name, + GroupMemberByNode: map[string]controlplane.GroupMemberSet{ + podA.Spec.NodeName: controlplane.NewGroupMemberSet(&controlplane.GroupMember{ + Pod: &controlplane.PodReference{Name: podA.Name, Namespace: podA.Namespace}, + }), + }, + } + expectedAddressGroup := &antreatypes.AddressGroup{ + SpanMeta: antreatypes.SpanMeta{NodeNames: sets.New(podA.Spec.NodeName)}, + UID: cgB.UID, + Name: cgB.Name, + SourceGroup: cgB.Name, + GroupMembers: controlplane.NewGroupMemberSet(&controlplane.GroupMember{ + Pod: &controlplane.PodReference{Name: podB.Name, Namespace: podB.Namespace}, + IPs: []controlplane.IPAddress{ipStrToIPAddress(podB.Status.PodIP)}, + }), + } + + checkResources := func(policy *antreatypes.NetworkPolicy, atg *antreatypes.AppliedToGroup, ag *antreatypes.AddressGroup) { + require.EventuallyWithT(t, func(c *assert.CollectT) { + policies := npc.internalNetworkPolicyStore.List() + if !assert.Len(c, policies, 1) { + return + } + assert.Equal(c, policy, policies[0].(*antreatypes.NetworkPolicy)) + + atgs := npc.appliedToGroupStore.List() + if atg != nil { + if !assert.Len(c, atgs, 1) { + return + } + assert.Equal(c, atg, atgs[0].(*antreatypes.AppliedToGroup)) + } else { + assert.Empty(c, atgs) + } + + ags := npc.addressGroupStore.List() + if ag != nil { + if !assert.Len(c, ags, 1) { + return + } + assert.Equal(c, ag, ags[0].(*antreatypes.AddressGroup)) + } else { + assert.Empty(c, ags) + } + }, 2*time.Second, 50*time.Millisecond) + } + checkResources(expectedPolicy, expectedAppliedToGroup, expectedAddressGroup) + + // Delete the ClusterGroup used by the AddressGroup, the AddressGroup should be deleted, the rule's peer should be empty. + npc.crdClient.CrdV1beta1().ClusterGroups().Delete(ctx, cgB.Name, metav1.DeleteOptions{}) + expectedPolicy2 := &antreatypes.NetworkPolicy{ + SpanMeta: antreatypes.SpanMeta{NodeNames: sets.New(podA.Spec.NodeName)}, + UID: acnp.UID, + Name: string(acnp.UID), + SourceRef: &controlplane.NetworkPolicyReference{Type: controlplane.AntreaClusterNetworkPolicy, Name: acnp.Name, UID: acnp.UID}, + Priority: pointer.Float64(acnp.Spec.Priority), + Rules: []controlplane.NetworkPolicyRule{ + {Direction: controlplane.DirectionIn, Action: &allowAction}, + }, + AppliedToGroups: []string{cgA.Name}, + TierPriority: &DefaultTierPriority, + } + checkResources(expectedPolicy2, expectedAppliedToGroup, nil) + + // Delete the ClusterGroup used by the AppliedToGroup, the AppliedToGroup should be deleted, the policy's span and + // appliedToGroup should be empty. + npc.crdClient.CrdV1beta1().ClusterGroups().Delete(ctx, cgA.Name, metav1.DeleteOptions{}) + expectedPolicy3 := &antreatypes.NetworkPolicy{ + SpanMeta: antreatypes.SpanMeta{NodeNames: sets.New[string]()}, + UID: acnp.UID, + Name: string(acnp.UID), + SourceRef: &controlplane.NetworkPolicyReference{Type: controlplane.AntreaClusterNetworkPolicy, Name: acnp.Name, UID: acnp.UID}, + Priority: pointer.Float64(acnp.Spec.Priority), + Rules: []controlplane.NetworkPolicyRule{ + {Direction: controlplane.DirectionIn, Action: &allowAction}, + }, + AppliedToGroups: []string{}, + TierPriority: &DefaultTierPriority, + } + checkResources(expectedPolicy3, nil, nil) + + // Recreate the ClusterGroups, everything should be restored. + npc.crdClient.CrdV1beta1().ClusterGroups().Create(ctx, cgA, metav1.CreateOptions{}) + npc.crdClient.CrdV1beta1().ClusterGroups().Create(ctx, cgB, metav1.CreateOptions{}) + checkResources(expectedPolicy, expectedAppliedToGroup, expectedAddressGroup) +} + func checkQueueItemExistence(t *testing.T, queue workqueue.RateLimitingInterface, items ...string) { require.Equal(t, len(items), queue.Len()) expectedItems := sets.New[string](items...) diff --git a/pkg/controller/networkpolicy/store/addressgroup.go b/pkg/controller/networkpolicy/store/addressgroup.go index aa0f8c4e2d4..cbb878f704f 100644 --- a/pkg/controller/networkpolicy/store/addressgroup.go +++ b/pkg/controller/networkpolicy/store/addressgroup.go @@ -157,7 +157,7 @@ func NewAddressGroupStore() storage.Interface { indexers := cache.Indexers{ cache.NamespaceIndex: func(obj interface{}) ([]string, error) { ag, ok := obj.(*types.AddressGroup) - if !ok { + if !ok || ag.Selector == nil { return []string{}, nil } // ag.Selector.Namespace == "" means it's a cluster scoped group, we index it as it is. @@ -165,11 +165,18 @@ func NewAddressGroupStore() storage.Interface { }, IsNodeAddressGroupIndex: func(obj interface{}) ([]string, error) { ag, ok := obj.(*types.AddressGroup) - if !ok || ag.Selector.NodeSelector == nil { + if !ok || ag.Selector == nil || ag.Selector.NodeSelector == nil { return []string{}, nil } return []string{"true"}, nil }, + SourceGroupIndex: func(obj interface{}) ([]string, error) { + atg, ok := obj.(*types.AddressGroup) + if !ok || atg.SourceGroup == "" { + return []string{}, nil + } + return []string{atg.SourceGroup}, nil + }, } return ram.NewStore(AddressGroupKeyFunc, indexers, genAddressGroupEvent, keyAndSpanSelectFunc, func() runtime.Object { return new(controlplane.AddressGroup) }) } diff --git a/pkg/controller/networkpolicy/store/appliedtogroup.go b/pkg/controller/networkpolicy/store/appliedtogroup.go index d4a9ae44f21..86c98861d42 100644 --- a/pkg/controller/networkpolicy/store/appliedtogroup.go +++ b/pkg/controller/networkpolicy/store/appliedtogroup.go @@ -28,7 +28,10 @@ import ( "antrea.io/antrea/pkg/controller/types" ) -const IsAppliedToServiceIndex = "isAppliedToService" +const ( + IsAppliedToServiceIndex = "isAppliedToService" + SourceGroupIndex = "sourceGroup" +) // appliedToGroupEvent implements storage.InternalEvent. type appliedToGroupEvent struct { @@ -185,6 +188,13 @@ func NewAppliedToGroupStore() storage.Interface { } return []string{"true"}, nil }, + SourceGroupIndex: func(obj interface{}) ([]string, error) { + atg, ok := obj.(*types.AppliedToGroup) + if !ok || atg.SourceGroup == "" { + return []string{}, nil + } + return []string{atg.SourceGroup}, nil + }, } return ram.NewStore(AppliedToGroupKeyFunc, indexers, genAppliedToGroupEvent, keyAndSpanSelectFunc, func() runtime.Object { return new(controlplane.AppliedToGroup) }) } diff --git a/pkg/controller/types/networkpolicy.go b/pkg/controller/types/networkpolicy.go index 81171b57dde..0db81e22d68 100644 --- a/pkg/controller/types/networkpolicy.go +++ b/pkg/controller/types/networkpolicy.go @@ -41,18 +41,24 @@ func (meta *SpanMeta) Has(nodeName string) bool { type AppliedToGroup struct { SpanMeta // If the AppliedToGroup is created from GroupSelector, UID is generated from the hash value of GroupSelector.NormalizedName. - // If the AppliedToGroup is created for a ClusterGroup, the UID is that of the corresponding ClusterGroup. + // If the AppliedToGroup is created for a ClusterGroup/Group, the UID is that of the corresponding ClusterGroup/Group. // If the AppliedToGroup is created for a Service, the UID is generated from the hash value of NamespacedName of the Service. UID types.UID - // Name of this group, currently it's same as UID. + // In case the AddressGroup is created for a ClusterGroup, it's the Name of the corresponding ClusterGroup. + // In case the AddressGroup is created for a Group, it's the Namespace/Name of the corresponding Group. + // Otherwise, it's same as UID. Name string - // Selector describes how the group selects pods. - // Selector can't be used with Service. + + // Selector, Service, and SourceGroup are mutually exclusive ways of selecting GroupMembers for the AppliedToGroup. + // For any AppliedToGroup, only one must be set. + // Selector describes how the group selects pods using selector. Selector *GroupSelector // 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 + // SourceGroup refers to the ClusterGroup or Group the AppliedToGroup is derived from. + SourceGroup string + // GroupMemberByNode is a mapping from nodeName to a set of GroupMembers on the Node, // either GroupMembers or ExternalEntity on the external node. // It will be converted to a slice of GroupMember for transferring according @@ -65,14 +71,21 @@ type AppliedToGroup struct { // AddressGroup describes a set of addresses used as source or destination of Network Policy rules. type AddressGroup struct { SpanMeta - // UID is generated from the hash value of GroupSelector.NormalizedName. - // In case the AddressGroup is created for a ClusterGroup, the UID is - // that of the corresponding ClusterGroup. + // If the AddressGroup is created from GroupSelector, UID is generated from the hash value of GroupSelector.NormalizedName. + // If the AddressGroup is created for a ClusterGroup/Group, the UID is that of the corresponding ClusterGroup/Group. UID types.UID - // Name of this group, currently it's same as UID. + // In case the AddressGroup is created for a ClusterGroup, it's the Name of the corresponding ClusterGroup. + // In case the AddressGroup is created for a Group, it's the Namespace/Name of the corresponding ClusterGroup. + // Otherwise, it's same as UID. Name string + + // Selector and SourceGroup are mutually exclusive ways of selecting GroupMembers for the AddressGroup. + // For any AddressGroup, only one must be set. // Selector describes how the group selects pods to get their addresses. - Selector GroupSelector + Selector *GroupSelector + // SourceGroup refers to the ClusterGroup or Group the AddressGroup is derived from. + SourceGroup string + // GroupMembers is a set of GroupMembers selected by this group. // It will be converted to a slice of GroupMember for transferring according // to client's selection.