From 1ab6bbd538b338052e8ffc64877e0e751d93f523 Mon Sep 17 00:00:00 2001 From: Yang Ding Date: Thu, 12 Aug 2021 14:44:47 -0700 Subject: [PATCH] Address comments and add UT Signed-off-by: Yang Ding --- pkg/apis/controlplane/types.go | 5 ++-- .../networkpolicy/clustergroupmember/rest.go | 25 ++++++++-------- .../clustergroupmember/rest_test.go | 29 +++++++++++++++++-- pkg/controller/networkpolicy/clustergroup.go | 8 ++--- .../networkpolicy/clustergroup_test.go | 2 +- .../networkpolicy_controller_test.go | 27 ++++++----------- 6 files changed, 55 insertions(+), 41 deletions(-) diff --git a/pkg/apis/controlplane/types.go b/pkg/apis/controlplane/types.go index 5adfcb6b264..96667e5f911 100644 --- a/pkg/apis/controlplane/types.go +++ b/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" diff --git a/pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go b/pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go index 6fe875ffe0e..f7dcd9fded4 100644 --- a/pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go +++ b/pkg/apiserver/registry/networkpolicy/clustergroupmember/rest.go @@ -41,7 +41,7 @@ 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 { @@ -49,25 +49,24 @@ func (r *REST) New() runtime.Object { } 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 diff --git a/pkg/apiserver/registry/networkpolicy/clustergroupmember/rest_test.go b/pkg/apiserver/registry/networkpolicy/clustergroupmember/rest_test.go index 96559b3d3da..594e3c53f00 100644 --- a/pkg/apiserver/registry/networkpolicy/clustergroupmember/rest_test.go +++ b/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 { diff --git a/pkg/controller/networkpolicy/clustergroup.go b/pkg/controller/networkpolicy/clustergroup.go index 0bbe46b6e04..f4f6a6da1d6 100644 --- a/pkg/controller/networkpolicy/clustergroup.go +++ b/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) } diff --git a/pkg/controller/networkpolicy/clustergroup_test.go b/pkg/controller/networkpolicy/clustergroup_test.go index db55e3b9ea5..4139890c46d 100644 --- a/pkg/controller/networkpolicy/clustergroup_test.go +++ b/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) }) diff --git a/pkg/controller/networkpolicy/networkpolicy_controller_test.go b/pkg/controller/networkpolicy/networkpolicy_controller_test.go index a3c7433512b..4f23759607d 100644 --- a/pkg/controller/networkpolicy/networkpolicy_controller_test.go +++ b/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)) }