Automated cherry pick of #3383: Fix ipBlock referenced in child ClusterGroup not processed

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go

@@ -62,7 +62,8 @@ func (r *REST) Get(ctx context.Context, name string, options *metav1.GetOptions)
 			effectiveIPBlocks = append(effectiveIPBlocks, ipb.CIDR)
 		}
 		memberList.EffectiveIPBlocks = effectiveIPBlocks
-	} else {
+	}
+	if len(groupMembers) > 0 {
 		effectiveMembers := make([]controlplane.GroupMember, 0, len(groupMembers))
 		for _, member := range groupMembers {
 			effectiveMembers = append(effectiveMembers, *member)

pkg/apiserver/registry/networkpolicy/clustergroupmember/rest_test.go

@@ -130,7 +130,7 @@ func TestRESTGet(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "cgC",
 				},
-				EffectiveMembers: []controlplane.GroupMember{},
+				EffectiveMembers: nil,
 			},
 			expectedErr: false,
 		},

pkg/controller/networkpolicy/clustergroup.go

@@ -71,7 +71,7 @@ func (c *NetworkPolicyController) updateClusterGroup(oldObj, curObj interface{})
 		for _, ipb := range newGroup.IPBlocks {
 			newIPBs.Insert(ipb.CIDR.String())
 		}
-		return oldIPBs.Equal(newIPBs)
+		return !oldIPBs.Equal(newIPBs)
 	}
 	childGroupsUpdated := func() bool {
 		oldChildGroups, newChildGroups := sets.String{}, sets.String{}
@@ -230,8 +230,8 @@ func (c *NetworkPolicyController) syncInternalGroup(key string) error {
 	//   2. All its child groups are created and realized.
 	if len(grp.ChildGroups) > 0 {
 		for _, cgName := range grp.ChildGroups {
-			cg, found, _ := c.internalGroupStore.Get(cgName)
-			if !found || cg.(*antreatypes.Group).MembersComputed != v1.ConditionTrue {
+			internalGroup, found, _ := c.internalGroupStore.Get(cgName)
+			if !found || internalGroup.(*antreatypes.Group).MembersComputed != v1.ConditionTrue {
 				membersComputed = false
 				break
 			}
@@ -419,10 +419,8 @@ func (c *NetworkPolicyController) GetGroupMembers(cgName string) (controlplane.G
 	groupObj, found, _ := c.internalGroupStore.Get(cgName)
 	if found {
 		group := groupObj.(*antreatypes.Group)
-		if len(group.IPBlocks) > 0 {
-			return nil, group.IPBlocks, nil
-		}
-		return c.getClusterGroupMemberSet(group), nil, nil
+		member, ipb := c.getClusterGroupMembers(group)
+		return member, ipb, nil
 	}
 	return nil, nil, fmt.Errorf("no internal Group with name %s is found", cgName)
 }

pkg/controller/networkpolicy/clusternetworkpolicy.go

@@ -278,7 +278,7 @@ func (n *NetworkPolicyController) processClusterNetworkPolicy(cnp *crdv1alpha1.C
 	// If appliedTo is set at spec level and the ACNP has per-namespace rules, then each appliedTo needs
 	// to be split into appliedToGroups for each of its affected Namespace.
 	var clusterAppliedToAffectedNS []string
-	// atgForNamespace is the appliedToGroups splitted by Namespaces.
+	// atgForNamespace is the appliedToGroups split by Namespaces.
 	var atgForNamespace []string
 	if hasPerNamespaceRule && len(cnp.Spec.AppliedTo) > 0 {
 		for _, at := range cnp.Spec.AppliedTo {
@@ -490,6 +490,31 @@ func getUniqueNSSelectors(selectors []labels.Selector) []labels.Selector {
 	return selectors[:i]
 }
 
+// processInternalGroupForRule examines the internal group (and its childGroups if applicable)
+// to determine whether an addressGroup needs to be created, and returns any ipBlocks contained
+// by the internal Group as well.
+func (n *NetworkPolicyController) processInternalGroupForRule(group *antreatypes.Group) (bool, []controlplane.IPBlock) {
+	if len(group.IPBlocks) > 0 {
+		return false, group.IPBlocks
+	} else if len(group.ChildGroups) == 0 {
+		return true, nil
+	}
+	var ipBlocks []controlplane.IPBlock
+	createAddrGroup := false
+	for _, childName := range group.ChildGroups {
+		childGroup, found, _ := n.internalGroupStore.Get(childName)
+		if found {
+			child := childGroup.(*antreatypes.Group)
+			createChildAG, ipb := n.processInternalGroupForRule(child)
+			if createChildAG {
+				createAddrGroup = true
+			}
+			ipBlocks = append(ipBlocks, ipb...)
+		}
+	}
+	return createAddrGroup, ipBlocks
+}
+
 // processRefCG processes the ClusterGroup reference present in the rule and returns the
 // NetworkPolicyPeer with the corresponding AddressGroup or IPBlock.
 func (n *NetworkPolicyController) processRefCG(g string) (string, []controlplane.IPBlock) {
@@ -509,12 +534,17 @@ func (n *NetworkPolicyController) processRefCG(g string) (string, []controlplane
 		return "", nil
 	}
 	intGrp := ig.(*antreatypes.Group)
-	if len(intGrp.IPBlocks) > 0 {
-		return "", intGrp.IPBlocks
-	}
-	agKey := n.createAddressGroupForClusterGroupCRD(intGrp)
-	// Return if addressGroup was created or found.
-	return agKey, nil
+	// The ClusterGroup referred in the rule might have childGroups defined using selectors
+	// or ipBlocks (or both). An addressGroup needs to be created as long as there is at least
+	// one childGroup defined by selectors, or the ClusterGroup itself is defined by selectors.
+	// In case of updates, the original addressGroup created will be de-referenced and cleaned
+	// up if the ClusterGroup becomes ipBlocks-only.
+	createAddrGroup, ipb := n.processInternalGroupForRule(intGrp)
+	if createAddrGroup {
+		agKey := n.createAddressGroupForClusterGroupCRD(intGrp)
+		return agKey, ipb
+	}
+	return "", ipb
 }
 
 func (n *NetworkPolicyController) processAppliedToGroupForCG(g string) string {

pkg/controller/networkpolicy/clusternetworkpolicy_test.go

@@ -1584,11 +1584,27 @@ func TestProcessRefCG(t *testing.T) {
 			NamespaceSelector: &selectorA,
 		},
 	}
+	cgNested1 := crdv1alpha3.ClusterGroup{
+		ObjectMeta: metav1.ObjectMeta{Name: "cgD", UID: "uidD"},
+		Spec: crdv1alpha3.GroupSpec{
+			ChildGroups: []crdv1alpha3.ClusterGroupReference{"cgB"},
+		},
+	}
+	cgNested2 := crdv1alpha3.ClusterGroup{
+		ObjectMeta: metav1.ObjectMeta{Name: "cgE", UID: "uidE"},
+		Spec: crdv1alpha3.GroupSpec{
+			ChildGroups: []crdv1alpha3.ClusterGroupReference{"cgA", "cgB"},
+		},
+	}
 	_, npc := newController()
 	npc.addClusterGroup(&cgA)
 	npc.addClusterGroup(&cgB)
+	npc.addClusterGroup(&cgNested1)
+	npc.addClusterGroup(&cgNested2)
 	npc.cgStore.Add(&cgA)
 	npc.cgStore.Add(&cgB)
+	npc.cgStore.Add(&cgNested1)
+	npc.cgStore.Add(&cgNested2)
 	tests := []struct {
 		name        string
 		inputCG     string
@@ -1624,6 +1640,28 @@ func TestProcessRefCG(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:       "nested-cg-with-ipblock",
+			inputCG:    cgNested1.Name,
+			expectedAG: "",
+			expectedIPB: []controlplane.IPBlock{
+				{
+					CIDR:   *cidrIPNet,
+					Except: []controlplane.IPNet{},
+				},
+			},
+		},
+		{
+			name:       "nested-cg-with-ipblock-and-selector",
+			inputCG:    cgNested2.Name,
+			expectedAG: cgNested2.Name,
+			expectedIPB: []controlplane.IPBlock{
+				{
+					CIDR:   *cidrIPNet,
+					Except: []controlplane.IPNet{},
+				},
+			},
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/controller/networkpolicy/crd_utils.go

@@ -109,9 +109,8 @@ func (n *NetworkPolicyController) toAntreaPeerForCRD(peers []v1alpha1.NetworkPol
 			normalizedUID, groupIPBlocks := n.processRefCG(peer.Group)
 			if normalizedUID != "" {
 				addressGroups = append(addressGroups, normalizedUID)
-			} else if len(groupIPBlocks) > 0 {
-				ipBlocks = append(ipBlocks, groupIPBlocks...)
 			}
+			ipBlocks = append(ipBlocks, groupIPBlocks...)
 		} else if peer.FQDN != "" {
 			fqdns = append(fqdns, peer.FQDN)
 		} else {

pkg/controller/networkpolicy/networkpolicy_controller.go

@@ -1110,28 +1110,37 @@ func (n *NetworkPolicyController) getAddressGroupMemberSet(g *antreatypes.Addres
 	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)
-		return n.getClusterGroupMemberSet(group)
+		members, _ := n.getClusterGroupMembers(group)
+		return members
 	}
 	return n.getMemberSetForGroupType(addressGroupType, g.Name)
 }
 
-// getClusterGroupMemberSet knows how to construct a GroupMemberSet that contains
+// getClusterGroupMembers knows how to construct a GroupMemberSet and ipBlocks that contains
 // all the entities selected by a ClusterGroup. For ClusterGroup that has childGroups,
 // the members are computed as the union of all its childGroup's members.
-func (n *NetworkPolicyController) getClusterGroupMemberSet(group *antreatypes.Group) controlplane.GroupMemberSet {
-	if len(group.ChildGroups) == 0 {
-		return n.getMemberSetForGroupType(clusterGroupType, group.Name)
+func (n *NetworkPolicyController) getClusterGroupMembers(group *antreatypes.Group) (controlplane.GroupMemberSet, []controlplane.IPBlock) {
+	if len(group.IPBlocks) > 0 {
+		return nil, group.IPBlocks
+	} else if len(group.ChildGroups) == 0 {
+		return n.getMemberSetForGroupType(clusterGroupType, group.Name), nil
 	}
+	var ipBlocks []controlplane.IPBlock
 	groupMemberSet := controlplane.GroupMemberSet{}
 	for _, childName := range group.ChildGroups {
 		childGroup, found, _ := n.internalGroupStore.Get(childName)
 		if found {
 			child := childGroup.(*antreatypes.Group)
-			groupMemberSet.Merge(n.getMemberSetForGroupType(clusterGroupType, child.Name))
+			members, ipb := n.getClusterGroupMembers(child)
+			ipBlocks = append(ipBlocks, ipb...)
+			groupMemberSet.Merge(members)
 		}
 	}
-	return groupMemberSet
+	return groupMemberSet, ipBlocks
 }
 
 // getMemberSetForGroupType knows how to construct a GroupMemberSet for the given

test/e2e/antreapolicy_test.go

@@ -1455,7 +1455,7 @@ func testACNPClusterGroupRefRuleIPBlocks(t *testing.T) {
 		SetIPBlocks(ipBlock2)
 
 	builder := &ClusterNetworkPolicySpecBuilder{}
-	builder = builder.SetName("acnp-deny-ya-to-x-ips-ingress").
+	builder = builder.SetName("acnp-deny-x-ips-ingress-for-ya").
 		SetPriority(1.0).
 		SetAppliedToGroup([]ACNPAppliedToSpec{
 			{
@@ -1485,7 +1485,7 @@ func testACNPClusterGroupRefRuleIPBlocks(t *testing.T) {
 		},
 	}
 	testCase := []*TestCase{
-		{"ACNP Drop Ingress From Pod: y/a to ClusterGroup with ipBlocks", testStep},
+		{"ACNP Drop Ingress From x to Pod y/a to ClusterGroup with ipBlocks", testStep},
 	}
 	executeTests(t, testCase)
 }
@@ -2494,6 +2494,80 @@ func testACNPNestedClusterGroupCreateAndUpdate(t *testing.T, data *TestData) {
 	executeTestsWithData(t, testCase, data)
 }
 
+func testACNPNestedIPBlockClusterGroupCreateAndUpdate(t *testing.T) {
+	podXAIP, _ := podIPs["x/a"]
+	podXBIP, _ := podIPs["x/b"]
+	genCIDR := func(ip string) string {
+		if strings.Contains(ip, ".") {
+			return ip + "/32"
+		}
+		return ip + "/128"
+	}
+	cg1Name, cg2Name, cg3Name := "cg-x-a-ipb", "cg-x-b-ipb", "cg-select-x-c"
+	cgParentName := "cg-parent"
+	var ipBlockXA, ipBlockXB []crdv1alpha1.IPBlock
+	for i := 0; i < len(podXAIP); i++ {
+		ipBlockXA = append(ipBlockXA, crdv1alpha1.IPBlock{CIDR: genCIDR(podXAIP[i])})
+		ipBlockXB = append(ipBlockXB, crdv1alpha1.IPBlock{CIDR: genCIDR(podXBIP[i])})
+	}
+	cgBuilder1 := &ClusterGroupV1Alpha3SpecBuilder{}
+	cgBuilder1 = cgBuilder1.SetName(cg1Name).SetIPBlocks(ipBlockXA)
+	cgBuilder2 := &ClusterGroupV1Alpha3SpecBuilder{}
+	cgBuilder2 = cgBuilder2.SetName(cg2Name).SetIPBlocks(ipBlockXB)
+	cgParent := &ClusterGroupV1Alpha3SpecBuilder{}
+	cgParent = cgParent.SetName(cgParentName).SetChildGroups([]string{cg1Name, cg2Name})
+
+	builder := &ClusterNetworkPolicySpecBuilder{}
+	builder = builder.SetName("acnp-deny-x-ips-ingress-for-ya").
+		SetPriority(1.0).
+		SetAppliedToGroup([]ACNPAppliedToSpec{
+			{
+				PodSelector: map[string]string{"pod": "a"},
+				NSSelector:  map[string]string{"ns": "y"},
+			},
+		})
+	builder.AddIngress(v1.ProtocolTCP, &p80, nil, nil, nil, nil, nil,
+		nil, nil, false, nil, crdv1alpha1.RuleActionDrop, cgParentName, "")
+
+	reachability := NewReachability(allPods, Connected)
+	reachability.Expect("x/a", "y/a", Dropped)
+	reachability.Expect("x/b", "y/a", Dropped)
+	testStep := &TestStep{
+		"Port 80",
+		reachability,
+		[]metav1.Object{builder.Get(), cgBuilder1.Get(), cgBuilder2.Get(), cgParent.Get()},
+		[]int32{80},
+		v1.ProtocolTCP,
+		0,
+		nil,
+	}
+
+	cgBuilder3 := &ClusterGroupV1Alpha3SpecBuilder{}
+	cgBuilder3 = cgBuilder3.SetName(cg3Name).
+		SetNamespaceSelector(map[string]string{"ns": "x"}, nil).
+		SetPodSelector(map[string]string{"pod": "c"}, nil)
+	updatedCGParent := &ClusterGroupV1Alpha3SpecBuilder{}
+	updatedCGParent = updatedCGParent.SetName(cgParentName).SetChildGroups([]string{cg1Name, cg3Name})
+
+	reachability2 := NewReachability(allPods, Connected)
+	reachability2.Expect("x/a", "y/a", Dropped)
+	reachability2.Expect("x/c", "y/a", Dropped)
+	testStep2 := &TestStep{
+		"Port 80, updated",
+		reachability2,
+		[]metav1.Object{cgBuilder3.Get(), updatedCGParent.Get()},
+		[]int32{80},
+		v1.ProtocolTCP,
+		0,
+		nil,
+	}
+
+	testCase := []*TestCase{
+		{"ACNP Drop Ingress From x to Pod y/a with nested ClusterGroup with ipBlocks", []*TestStep{testStep, testStep2}},
+	}
+	executeTests(t, testCase)
+}
+
 func testACNPNamespaceIsolation(t *testing.T) {
 	builder := &ClusterNetworkPolicySpecBuilder{}
 	builder = builder.SetName("test-acnp-ns-isolation").
@@ -3174,6 +3248,7 @@ func TestAntreaPolicy(t *testing.T) {
 		t.Run("Case=ACNPClusterGroupIngressRuleDenyCGWithXBtoYA", func(t *testing.T) { testACNPIngressRuleDenyCGWithXBtoYA(t) })
 		t.Run("Case=ACNPClusterGroupServiceRef", func(t *testing.T) { testACNPClusterGroupServiceRefCreateAndUpdate(t, data) })
 		t.Run("Case=ACNPNestedClusterGroup", func(t *testing.T) { testACNPNestedClusterGroupCreateAndUpdate(t, data) })
+		t.Run("Case=ACNPNestedIPBlockClusterGroup", func(t *testing.T) { testACNPNestedIPBlockClusterGroupCreateAndUpdate(t) })
 		t.Run("Case=ACNPFQDNPolicy", func(t *testing.T) { testFQDNPolicy(t) })
 		t.Run("Case=FQDNPolicyInCluster", func(t *testing.T) { testFQDNPolicyInClusterService(t) })
 		t.Run("Case=ACNPToServices", func(t *testing.T) { testToServices(t) })