Skip to content

Commit

Permalink
Fix missing call of removing groupDb cache when deleting OVS group (#…
Browse files Browse the repository at this point in the history 
…4592)

The old group will be reused unexpectedly if groupDb cache managed
by OFBridge is not cleared for that group, which causes a new group
claims to have a different group type acquiring the old group type.

Fixes #4575

Signed-off-by: ceclinux <src655@gmail.com>
  • Loading branch information
@ceclinux
ceclinux committed Mar 10, 2023
1 parent cc2f40f commit 9b6319d
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 23 deletions.
10 changes: 8 additions & 2 deletions pkg/agent/openflow/client.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -644,7 +644,10 @@ func (c *client) UninstallServiceGroup(groupID binding.GroupIDType) error {
gCache, ok := c.featureService.groupCache.Load(groupID)
if ok {
if err := c.ofEntryOperations.DeleteOFEntries([]binding.OFEntry{gCache.(binding.Group)}); err != nil {
return fmt.Errorf("error when deleting Service Endpoints Group %d: %w", groupID, err)
return fmt.Errorf("error when deleting Openflow entries for Service Endpoints Group %d: %w", groupID, err)
}
if err := c.bridge.DeleteGroup(groupID); err != nil {
return fmt.Errorf("error when deleting OFSwitch groupDb Cache for Service Endpoints Group %d: %w", groupID, err)
}
c.featureService.groupCache.Delete(groupID)
}
Expand Down Expand Up @@ -1346,7 +1349,10 @@ func (c *client) UninstallMulticastGroup(groupID binding.GroupIDType) error {
gCache, ok := c.featureMulticast.groupCache.Load(groupID)
if ok {
if err := c.ofEntryOperations.DeleteOFEntries([]binding.OFEntry{gCache.(binding.Group)}); err != nil {
return fmt.Errorf("error when deleting Multicast receiver Group %d: %w", groupID, err)
return fmt.Errorf("error when deleting Openflow entries for Multicast receiver Group %d: %w", groupID, err)
}
if err := c.bridge.DeleteGroup(groupID); err != nil {
return fmt.Errorf("error when deleting OFSwitch groupDb Cache for Multicast receiver Group %d: %w", groupID, err)
}
c.featureMulticast.groupCache.Delete(groupID)
}
Expand Down
67 changes: 50 additions & 17 deletions pkg/agent/openflow/client_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -851,11 +851,12 @@ func Test_client_InstallServiceGroup(t *testing.T) {
}

testCases := []struct {
name string
withSessionAffinity bool
endpoints []proxy.Endpoint
expectedGroup string
mcsLocalService *types.ServiceGroupInfo
name string
withSessionAffinity bool
endpoints []proxy.Endpoint
expectedGroup string
mcsLocalService *types.ServiceGroupInfo
deleteOFEntriesError error
}{
{
name: "IPv4 Endpoints",
Expand Down Expand Up @@ -909,6 +910,17 @@ func Test_client_InstallServiceGroup(t *testing.T) {
"bucket=bucket_id:0,weight:100,actions=set_field:0xfec00010001000000000000000000100->xxreg3,set_field:0x50/0xffff->reg4,resubmit:ServiceLB," +
"bucket=bucket_id:1,weight:100,actions=set_field:0xfec00010001000000000000000000101->xxreg3,set_field:0x50/0xffff->reg4,resubmit:ServiceLB",
},
{
name: "delete group failed for IPv4 Endpoints",
endpoints: []proxy.Endpoint{
proxy.NewBaseEndpointInfo("10.10.0.100", "", "", 80, false, true, false, false, nil),
proxy.NewBaseEndpointInfo("10.10.0.101", "", "", 80, false, true, false, false, nil),
},
expectedGroup: "group_id=100,type=select," +
"bucket=bucket_id:0,weight:100,actions=set_field:0xa0a0064->reg3,set_field:0x50/0xffff->reg4,resubmit:EndpointDNAT," +
"bucket=bucket_id:1,weight:100,actions=set_field:0xa0a0065->reg3,set_field:0x50/0xffff->reg4,resubmit:EndpointDNAT",
deleteOFEntriesError: fmt.Errorf("error when deleting Openflow entries for Service Endpoints Group 100"),
},
}

for _, tc := range testCases {
Expand All @@ -921,16 +933,22 @@ func Test_client_InstallServiceGroup(t *testing.T) {
defer resetPipelines()

m.EXPECT().AddOFEntries(gomock.Any()).Return(nil).Times(1)
m.EXPECT().DeleteOFEntries(gomock.Any()).Return(nil).Times(1)
m.EXPECT().DeleteOFEntries(gomock.Any()).Return(tc.deleteOFEntriesError).Times(1)
assert.NoError(t, fc.InstallServiceGroup(groupID, tc.withSessionAffinity, tc.mcsLocalService, tc.endpoints))
gCacheI, ok := fc.featureService.groupCache.Load(groupID)
require.True(t, ok)
group := getGroupFromCache(gCacheI.(binding.Group))
assert.Equal(t, tc.expectedGroup, group)

assert.NoError(t, fc.UninstallServiceGroup(groupID))
_, ok = fc.featureService.groupCache.Load(groupID)
require.False(t, ok)
if tc.deleteOFEntriesError == nil {
assert.NoError(t, fc.UninstallServiceGroup(groupID))
_, ok = fc.featureService.groupCache.Load(groupID)
require.False(t, ok)
} else {
assert.Error(t, fc.UninstallServiceGroup(groupID))
_, ok = fc.featureService.groupCache.Load(groupID)
require.True(t, ok)
}
})
}
}
Expand Down Expand Up @@ -1992,10 +2010,11 @@ func Test_client_InstallMulticastGroup(t *testing.T) {
localReceivers := []uint32{50, 100}
remoteNodeReceivers := []net.IP{net.ParseIP("192.168.77.101"), net.ParseIP("192.168.77.102")}
testCases := []struct {
name string
localReceivers []uint32
remoteNodeReceivers []net.IP
expectedGroup string
name string
localReceivers []uint32
remoteNodeReceivers []net.IP
expectedGroup string
deleteOFEntriesError error
}{
{
name: "Local Receivers",
Expand All @@ -2021,6 +2040,14 @@ func Test_client_InstallMulticastGroup(t *testing.T) {
"bucket=bucket_id:2,actions=set_field:0x100/0x100->reg0,set_field:0x1->reg1,set_field:192.168.77.101->tun_dst,resubmit:MulticastOutput," +
"bucket=bucket_id:3,actions=set_field:0x100/0x100->reg0,set_field:0x1->reg1,set_field:192.168.77.102->tun_dst,resubmit:MulticastOutput",
},
{
name: "DeleteOFEntries Failed",
localReceivers: localReceivers,
expectedGroup: "group_id=101,type=all," +
"bucket=bucket_id:0,actions=set_field:0x100/0x100->reg0,set_field:0x32->reg1,resubmit:MulticastIngressRule," +
"bucket=bucket_id:1,actions=set_field:0x100/0x100->reg0,set_field:0x64->reg1,resubmit:MulticastIngressRule",
deleteOFEntriesError: fmt.Errorf("error when deleting Openflow entries for Multicast receiver Group 101"),
},
}
for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
Expand All @@ -2032,17 +2059,23 @@ func Test_client_InstallMulticastGroup(t *testing.T) {
defer resetPipelines()

m.EXPECT().AddOFEntries(gomock.Any()).Return(nil).Times(1)
m.EXPECT().DeleteOFEntries(gomock.Any()).Return(nil).Times(1)
m.EXPECT().DeleteOFEntries(gomock.Any()).Return(tc.deleteOFEntriesError).Times(1)

assert.NoError(t, fc.InstallMulticastGroup(groupID, tc.localReceivers, tc.remoteNodeReceivers))
gCacheI, ok := fc.featureMulticast.groupCache.Load(groupID)
require.True(t, ok)
group := getGroupFromCache(gCacheI.(binding.Group))
assert.Equal(t, tc.expectedGroup, group)

assert.NoError(t, fc.UninstallMulticastGroup(groupID))
_, ok = fc.featureMulticast.groupCache.Load(groupID)
require.False(t, ok)
if tc.deleteOFEntriesError == nil {
assert.NoError(t, fc.UninstallMulticastGroup(groupID))
_, ok = fc.featureMulticast.groupCache.Load(groupID)
require.False(t, ok)
} else {
assert.Error(t, fc.UninstallMulticastGroup(groupID))
_, ok = fc.featureMulticast.groupCache.Load(groupID)
require.True(t, ok)
}
})
}
}
Expand Down
5 changes: 1 addition & 4 deletions pkg/ovs/openflow/ofctrl_bridge.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -224,15 +224,12 @@ func (b *OFBridge) createGroupWithType(id GroupIDType, groupType ofctrl.GroupTyp
return g
}

// DeleteGroup deletes a specified group in groupDb.
func (b *OFBridge) DeleteGroup(id GroupIDType) error {
ofctrlGroup := b.ofSwitch.GetGroup(uint32(id))
if ofctrlGroup == nil {
return nil
}
g := &ofGroup{bridge: b, ofctrl: ofctrlGroup}
if err := g.Delete(); err != nil {
return fmt.Errorf("failed to delete the group: %w", err)
}
return b.ofSwitch.DeleteGroup(uint32(id))
}

Expand Down
32 changes: 32 additions & 0 deletions pkg/ovs/openflow/ofctrl_bridge_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (

"antrea.io/libOpenflow/util"
"antrea.io/ofnet/ofctrl"
"github.com/stretchr/testify/assert"

"antrea.io/antrea/pkg/ovs/ovsconfig"
)
Expand Down Expand Up @@ -85,3 +86,34 @@ func TestOFBridgeIsConnected(t *testing.T) {
}()
wg.Wait()
}

func TestDeleteGroup(t *testing.T) {
b := NewOFBridge("test-br", GetMgmtAddress(ovsconfig.DefaultOVSRunDir, "test-br"))

for _, m := range []struct {
name string
existingGroupID GroupIDType
deleteGroupID GroupIDType
err error
}{
{
name: "delete existing group without flow",
existingGroupID: 20,
deleteGroupID: 20,
err: nil,
},
{
name: "delete non-existing group",
existingGroupID: 20,
deleteGroupID: 30,
err: nil,
},
} {
t.Run(m.name, func(t *testing.T) {
b.ofSwitch = newFakeOFSwitch(b)
b.CreateGroup(m.existingGroupID)
err := b.DeleteGroup(m.deleteGroupID)
assert.Equal(t, m.err, err)
})
}
}

0 comments on commit 9b6319d

Please sign in to comment.