Address comments and add UT

Signed-off-by: Yang Ding <dingyang@vmware.com>

pkg/apis/controlplane/types.go

@@ -15,11 +15,12 @@
 package controlplane
 
 import (
+	"net"
+	"strconv"
+
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/intstr"
-	"net"
-	"strconv"
 
 	crdv1alpha1 "antrea.io/antrea/pkg/apis/crd/v1alpha1"
 	statsv1alpha1 "antrea.io/antrea/pkg/apis/stats/v1alpha1"

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

@@ -41,33 +41,32 @@ func NewREST(querier groupMembershipQuerier) *REST {
 }
 
 type groupMembershipQuerier interface {
-	GetGroupMembers(name string) (interface{}, error)
+	GetGroupMembers(name string) (controlplane.GroupMemberSet, []controlplane.IPBlock, error)
 }
 
 func (r *REST) New() runtime.Object {
 	return &controlplane.ClusterGroupMembers{}
 }
 
 func (r *REST) Get(ctx context.Context, name string, options *metav1.GetOptions) (runtime.Object, error) {
-	m, err := r.querier.GetGroupMembers(name)
+	groupMembers, ipBlocks, err := r.querier.GetGroupMembers(name)
 	if err != nil {
 		return nil, errors.NewInternalError(err)
 	}
 	memberList := &controlplane.ClusterGroupMembers{}
-	switch members := m.(type) {
-	case controlplane.GroupMemberSet:
-		effectiveMembers := make([]controlplane.GroupMember, 0, len(members))
-		for _, member := range members {
-			effectiveMembers = append(effectiveMembers, *member)
-		}
-		memberList.EffectiveMembers = effectiveMembers
-	case []controlplane.IPBlock:
-		effectiveIPBlocks := make([]controlplane.IPNet, 0, len(members))
-		for _, member := range members {
+	if len(ipBlocks) > 0 {
+		effectiveIPBlocks := make([]controlplane.IPNet, 0, len(ipBlocks))
+		for _, ipb:= range ipBlocks {
 			// ClusterGroup ipBlock cannot have except slices
-			effectiveIPBlocks = append(effectiveIPBlocks, member.CIDR)
+			effectiveIPBlocks = append(effectiveIPBlocks, ipb.CIDR)
 		}
 		memberList.EffectiveIPBlocks = effectiveIPBlocks
+	} else {
+		effectiveMembers := make([]controlplane.GroupMember, 0, len(groupMembers))
+		for _, member := range groupMembers {
+			effectiveMembers = append(effectiveMembers, *member)
+		}
+		memberList.EffectiveMembers = effectiveMembers
 	}
 	memberList.Name = name
 	return memberList, nil

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

@@ -15,6 +15,7 @@
 package clustergroupmember
 
 import (
+	"net"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -30,11 +31,17 @@ type fakeQuerier struct {
 	members map[string]controlplane.GroupMemberSet
 }
 
-func (q fakeQuerier) GetGroupMembers(uid string) (interface{}, error) {
+func (q fakeQuerier) GetGroupMembers(uid string) (controlplane.GroupMemberSet, []controlplane.IPBlock, error) {
 	if memberList, ok := q.members[uid]; ok {
-		return memberList, nil
+		return memberList, nil, nil
+	} else if uid == "cgIPBlock" {
+		testCIDR := controlplane.IPNet{
+			IP:           controlplane.IPAddress(net.ParseIP("10.0.0.1")),
+			PrefixLength: int32(24),
+		}
+		return nil, []controlplane.IPBlock{{CIDR: testCIDR}}, nil
 	}
-	return controlplane.GroupMemberSet{}, nil
+	return nil, nil, nil
 }
 
 func TestRESTGet(t *testing.T) {
@@ -121,6 +128,22 @@ func TestRESTGet(t *testing.T) {
 			},
 			expectedErr: false,
 		},
+		{
+			name:      "ipBlock-cg",
+			groupName: "cgIPBlock",
+			expectedObj: &controlplane.ClusterGroupMembers{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "cgIPBlock",
+				},
+				EffectiveIPBlocks: []controlplane.IPNet{
+					{
+						IP:           controlplane.IPAddress(net.ParseIP("10.0.0.1")),
+						PrefixLength: int32(24),
+					},
+				},
+			},
+			expectedErr: false,
+		},
 	}
 	rest := NewREST(fakeQuerier{members: members})
 	for _, tt := range tests {

pkg/controller/networkpolicy/clustergroup.go

@@ -429,14 +429,14 @@ func (n *NetworkPolicyController) getAssociatedGroupsByName(grpName string) []an
 // GetGroupMembers returns the current members of a ClusterGroup.
 // If the ClusterGroup is defined by IPBlocks, the returned members will be []controlplane.IPBlock.
 // Otherwise, the returned members will be of type controlplane.GroupMemberSet.
-func (n *NetworkPolicyController) GetGroupMembers(cgName string) (interface{}, error) {
+func (n *NetworkPolicyController) GetGroupMembers(cgName string) (controlplane.GroupMemberSet, []controlplane.IPBlock, error) {
 	groupObj, found, _ := n.internalGroupStore.Get(cgName)
 	if found {
 		group := groupObj.(*antreatypes.Group)
 		if len(group.IPBlocks) > 0 {
-			return group.IPBlocks, nil
+			return nil, group.IPBlocks, nil
 		}
-		return n.getClusterGroupMemberSet(group), nil
+		return n.getClusterGroupMemberSet(group), nil, nil
 	}
-	return nil, fmt.Errorf("no internal Group with name %s is found", cgName)
+	return nil, nil, fmt.Errorf("no internal Group with name %s is found", cgName)
 }

pkg/controller/networkpolicy/clustergroup_test.go

@@ -822,7 +822,7 @@ func TestGetGroupMembers(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			npc.internalGroupStore.Create(&tt.group)
 			npc.groupingInterface.AddGroup(clusterGroupType, tt.group.Name, tt.group.Selector)
-			members, err := npc.GetGroupMembers(tt.group.Name)
+			members, _, err := npc.GetGroupMembers(tt.group.Name)
 			assert.Equal(t, nil, err)
 			assert.Equal(t, tt.expectedMembers, members)
 		})

pkg/controller/networkpolicy/networkpolicy_controller_test.go

@@ -1434,8 +1434,7 @@ func TestAddPod(t *testing.T) {
 			updatedInAddrGroup := updatedInAddrGroupObj.(*antreatypes.AddressGroup)
 			updatedOutAddrGroupObj, _, _ := npc.addressGroupStore.Get(outGroupID)
 			updatedOutAddrGroup := updatedOutAddrGroupObj.(*antreatypes.AddressGroup)
-			gm, _ := npc.GetGroupMembers(groupKey)
-			groupMembers, _ := gm.(controlplane.GroupMemberSet)
+			groupMembers, _, _ := npc.GetGroupMembers(groupKey)
 			if tt.appGroupMatch {
 				assert.Len(t, podsAdded, 1, "expected Pod to match AppliedToGroup")
 			} else {
@@ -1533,8 +1532,7 @@ func TestDeletePod(t *testing.T) {
 	updatedAddrGroup := updatedAddrGroupObj.(*antreatypes.AddressGroup)
 	// Ensure Pod2 IP is removed from AddressGroup.
 	memberPod2 := &controlplane.GroupMember{IPs: []controlplane.IPAddress{ipStrToIPAddress(p2IP)}}
-	gm, _ := npc.GetGroupMembers(groupKey)
-	groupMembers, _ := gm.(controlplane.GroupMemberSet)
+	groupMembers, _, _ := npc.GetGroupMembers(groupKey)
 	assert.False(t, updatedAddrGroup.GroupMembers.Has(memberPod2))
 	assert.False(t, groupMembers.Has(memberPod2))
 }
@@ -1678,8 +1676,7 @@ func TestAddNamespace(t *testing.T) {
 			updatedInAddrGroup := updatedInAddrGroupObj.(*antreatypes.AddressGroup)
 			updatedOutAddrGroupObj, _, _ := npc.addressGroupStore.Get(outGroupID)
 			updatedOutAddrGroup := updatedOutAddrGroupObj.(*antreatypes.AddressGroup)
-			gm, _ := npc.GetGroupMembers(groupKey)
-			groupMembers, _ := gm.(controlplane.GroupMemberSet)
+			groupMembers, _, _ := npc.GetGroupMembers(groupKey)
 			memberPod1 := &controlplane.GroupMember{
 				Pod: &controlplane.PodReference{Name: "p1", Namespace: "nsA"},
 				IPs: []controlplane.IPAddress{ipStrToIPAddress("1.2.3.4")},
@@ -1842,8 +1839,7 @@ func TestDeleteNamespace(t *testing.T) {
 			updatedInAddrGroup := updatedInAddrGroupObj.(*antreatypes.AddressGroup)
 			updatedOutAddrGroupObj, _, _ := npc.addressGroupStore.Get(outGroupID)
 			updatedOutAddrGroup := updatedOutAddrGroupObj.(*antreatypes.AddressGroup)
-			gm, _ := npc.GetGroupMembers(groupKey)
-			groupMembers, _ := gm.(controlplane.GroupMemberSet)
+			groupMembers, _, _ := npc.GetGroupMembers(groupKey)
 			memberPod1 := &controlplane.GroupMember{IPs: []controlplane.IPAddress{ipStrToIPAddress("1.1.1.1")}}
 			memberPod2 := &controlplane.GroupMember{IPs: []controlplane.IPAddress{ipStrToIPAddress("1.1.1.2")}}
 			if tt.inAddressGroupMatch {
@@ -1973,19 +1969,16 @@ func TestAddAndUpdateService(t *testing.T) {
 		},
 		IPs: []controlplane.IPAddress{ipStrToIPAddress("4.3.2.1")},
 	}
-	gm1, _ := npc.GetGroupMembers(testCG1.Name)
-	groupMembers1, _ := gm1.(controlplane.GroupMemberSet)
+	groupMembers1, _, _ := npc.GetGroupMembers(testCG1.Name)
 	assert.True(t, groupMembers1.Has(memberPod1))
 	assert.False(t, groupMembers1.Has(memberPod2))
-	gm2, _ := npc.GetGroupMembers(testCG2.Name)
-	groupMembers2, _ := gm2.(controlplane.GroupMemberSet)
+	groupMembers2, _, _ := npc.GetGroupMembers(testCG2.Name)
 	assert.False(t, groupMembers2.Has(memberPod1))
 	assert.False(t, groupMembers2.Has(memberPod2))
 	// Update svc-1 to select app test-2 instead
 	npc.serviceStore.Update(testSvc1Updated)
 	npc.syncInternalGroup(testCG1.Name)
-	gmu, _ := npc.GetGroupMembers(testCG1.Name)
-	groupMembers1Updated, _ := gmu.(controlplane.GroupMemberSet)
+	groupMembers1Updated, _, _ := npc.GetGroupMembers(testCG1.Name)
 	assert.False(t, groupMembers1Updated.Has(memberPod1))
 	assert.True(t, groupMembers1Updated.Has(memberPod2))
 }
@@ -2043,14 +2036,12 @@ func TestDeleteService(t *testing.T) {
 		},
 		IPs: []controlplane.IPAddress{ipStrToIPAddress("1.2.3.4")},
 	}
-	gm, _ := npc.GetGroupMembers(testCG.Name)
-	groupMembers, _ := gm.(controlplane.GroupMemberSet)
+	groupMembers, _, _ := npc.GetGroupMembers(testCG.Name)
 	assert.True(t, groupMembers.Has(memberPod))
 	// Make sure that after Service deletion, the Pod member is removed from Group.
 	npc.serviceStore.Delete(testSvc)
 	npc.syncInternalGroup(testCG.Name)
-	gmu, _ := npc.GetGroupMembers(testCG.Name)
-	groupMembersUpdated, _ := gmu.(controlplane.GroupMemberSet)
+	groupMembersUpdated, _, _ := npc.GetGroupMembers(testCG.Name)
 	assert.False(t, groupMembersUpdated.Has(memberPod))
 }