From 69dfde6181017e87a742e0fc4e9016a9b0fc6bda Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Mon, 11 Mar 2024 10:50:52 +0800 Subject: [PATCH] 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 --- 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 | 57 +++-- .../networkpolicy_controller_test.go | 200 ++++++++++++++++-- .../networkpolicy/store/addressgroup.go | 11 +- .../networkpolicy/store/appliedtogroup.go | 12 +- pkg/controller/types/networkpolicy.go | 33 ++- 10 files changed, 291 insertions(+), 76 deletions(-) diff --git a/pkg/controller/networkpolicy/clustergroup.go b/pkg/controller/networkpolicy/clustergroup.go index 079065114c2..1306c59fca9 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 ae595fd9135..b02d27d63a7 100644 --- a/pkg/controller/networkpolicy/clusternetworkpolicy.go +++ b/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 diff --git a/pkg/controller/networkpolicy/clusternetworkpolicy_test.go b/pkg/controller/networkpolicy/clusternetworkpolicy_test.go index 08c40914817..db231bbac06 100644 --- a/pkg/controller/networkpolicy/clusternetworkpolicy_test.go +++ b/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", 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 40796745c71..1a471fea89e 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) } @@ -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) } } diff --git a/pkg/controller/networkpolicy/networkpolicy_controller_test.go b/pkg/controller/networkpolicy/networkpolicy_controller_test.go index 622f85c65bb..c8ca5f139b9 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" @@ -2578,8 +2579,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, @@ -2587,8 +2589,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, @@ -2596,8 +2599,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, @@ -2605,8 +2609,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, @@ -2614,8 +2619,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, @@ -2704,40 +2710,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, }, @@ -2994,7 +3005,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, @@ -3123,7 +3134,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{ @@ -3447,7 +3457,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) @@ -3782,6 +3791,151 @@ func TestSyncAppliedToGroupWithExternalEntity(t *testing.T) { } } +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.