From 0da37b02108d79fc902d1d6731c9245ff733dc84 Mon Sep 17 00:00:00 2001 From: mageshgv Date: Fri, 4 Oct 2019 10:24:19 -0700 Subject: Maintain unique vrf IDs in watchEventBestPath vrf map Fixes missing route propagation to proper vrfs in zapi --- pkg/server/server.go | 6 ++-- pkg/server/server_test.go | 61 ++++++++++++++++++++++++++++++++------ pkg/server/zclient.go | 4 +-- test/scenario_test/zapi_v3_test.py | 58 ++++++++++++++++++++++++++++++++++-- 4 files changed, 112 insertions(+), 17 deletions(-) diff --git a/pkg/server/server.go b/pkg/server/server.go index d6c54e10..c5dd18f3 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -708,13 +708,13 @@ func (s *BgpServer) notifyBestWatcher(best []*table.Path, multipath [][]*table.P clonedM[i] = clonePathList(pathList) } clonedB := clonePathList(best) - m := make(map[string]uint32) + m := make(map[uint32]bool) for _, p := range clonedB { switch p.GetRouteFamily() { case bgp.RF_IPv4_VPN, bgp.RF_IPv6_VPN: for _, vrf := range s.globalRib.Vrfs { if vrf.Id != 0 && table.CanImportToVrf(vrf, p) { - m[p.GetNlri().String()] = uint32(vrf.Id) + m[uint32(vrf.Id)] = true } } } @@ -3828,7 +3828,7 @@ type watchEventTable struct { type watchEventBestPath struct { PathList []*table.Path MultiPathList [][]*table.Path - Vrf map[string]uint32 + Vrf map[uint32]bool } type watchEventMessage struct { diff --git a/pkg/server/server_test.go b/pkg/server/server_test.go index 0c005efb..a390ce58 100644 --- a/pkg/server/server_test.go +++ b/pkg/server/server_test.go @@ -468,6 +468,10 @@ func TestMonitor(test *testing.T) { assert.Nil(err) defer s.StopBgp(context.Background(), &api.StopBgpRequest{}) + // Vrf1 111:111 and vrf2 import 111:111 and 222:222 + addVrf(test, s, "vrf1", "111:111", []string{"111:111"}, []string{"111:111"}, 1) + addVrf(test, s, "vrf2", "222:222", []string{"111:111", "222:222"}, []string{"222:222"}, 2) + p1 := &api.Peer{ Conf: &api.PeerConf{ NeighborAddress: "127.0.0.1", @@ -529,9 +533,10 @@ func TestMonitor(test *testing.T) { assert.Equal(1, len(b.PathList)) assert.Equal("10.0.0.0/24", b.PathList[0].GetNlri().String()) assert.False(b.PathList[0].IsWithdraw) + assert.Equal(0, len(b.Vrf)) // Withdraws the previous route. - // NOTE: Withdow should not require any path attribute. + // NOTE: Withdraw should not require any path attribute. if err := t.addPathList("", []*table.Path{table.NewPath(nil, bgp.NewIPAddrPrefix(24, "10.0.0.0"), true, nil, time.Now(), false)}); err != nil { log.Fatal(err) } @@ -540,6 +545,7 @@ func TestMonitor(test *testing.T) { assert.Equal(1, len(b.PathList)) assert.Equal("10.0.0.0/24", b.PathList[0].GetNlri().String()) assert.True(b.PathList[0].IsWithdraw) + assert.Equal(0, len(b.Vrf)) // Stops the watcher still having an item. w.Stop() @@ -594,6 +600,26 @@ func TestMonitor(test *testing.T) { assert.Equal("10.2.0.0/24", u.PathList[0].GetNlri().String()) assert.True(u.PathList[0].IsWithdraw) + // Test bestpath events with vrf and rt import + w.Stop() + w = s.watch(watchBestPath(false)) + attrs = []bgp.PathAttributeInterface{ + bgp.NewPathAttributeOrigin(0), + bgp.NewPathAttributeNextHop("10.0.0.1"), + } + + if err := s.addPathList("vrf1", []*table.Path{table.NewPath(nil, bgp.NewIPAddrPrefix(24, "10.0.0.0"), false, attrs, time.Now(), false)}); err != nil { + log.Fatal(err) + } + ev = <-w.Event() + b = ev.(*watchEventBestPath) + assert.Equal(1, len(b.PathList)) + assert.Equal("111:111:10.0.0.0/24", b.PathList[0].GetNlri().String()) + assert.False(b.PathList[0].IsWithdraw) + assert.Equal(2, len(b.Vrf)) + assert.True(b.Vrf[1]) + assert.True(b.Vrf[2]) + // Stops the watcher still having an item. w.Stop() } @@ -1161,17 +1187,34 @@ func parseRDRT(rdStr string) (bgp.RouteDistinguisherInterface, bgp.ExtendedCommu return rd, rt, nil } -func addVrf(t *testing.T, s *BgpServer, vrfName, rdStr string, id uint32) { - rd, rt, err := parseRDRT(rdStr) +func addVrf(t *testing.T, s *BgpServer, vrfName, rdStr string, importRtsStr []string, exportRtsStr []string, id uint32) { + rd, _, err := parseRDRT(rdStr) if err != nil { t.Fatal(err) } + importRts := make([]bgp.ExtendedCommunityInterface, 0, len(importRtsStr)) + for _, importRtStr := range importRtsStr { + _, rt, err := parseRDRT(importRtStr) + if err != nil { + t.Fatal(err) + } + importRts = append(importRts, rt) + } + + exportRts := make([]bgp.ExtendedCommunityInterface, 0, len(exportRtsStr)) + for _, exportRtStr := range exportRtsStr { + _, rt, err := parseRDRT(exportRtStr) + if err != nil { + t.Fatal(err) + } + exportRts = append(exportRts, rt) + } req := &api.AddVrfRequest{ Vrf: &api.Vrf{ Name: vrfName, - ImportRt: apiutil.MarshalRTs([]bgp.ExtendedCommunityInterface{rt}), - ExportRt: apiutil.MarshalRTs([]bgp.ExtendedCommunityInterface{rt}), + ImportRt: apiutil.MarshalRTs(importRts), + ExportRt: apiutil.MarshalRTs(exportRts), Rd: apiutil.MarshalRD(rd), Id: id, }, @@ -1188,8 +1231,8 @@ func TestDoNotReactToDuplicateRTCMemberships(t *testing.T) { s1 := runNewServer(1, "1.1.1.1", 10179) s2 := runNewServer(1, "2.2.2.2", 20179) - addVrf(t, s1, "vrf1", "111:111", 1) - addVrf(t, s2, "vrf1", "111:111", 1) + addVrf(t, s1, "vrf1", "111:111", []string{"111:111"}, []string{"111:111"}, 1) + addVrf(t, s2, "vrf1", "111:111", []string{"111:111"}, []string{"111:111"}, 1) if err := peerServers(t, ctx, []*BgpServer{s1, s2}, []config.AfiSafiType{config.AFI_SAFI_TYPE_L3VPN_IPV4_UNICAST, config.AFI_SAFI_TYPE_RTC}); err != nil { t.Fatal(err) @@ -1446,7 +1489,7 @@ func TestDeleteNonExistingVrf(t *testing.T) { log.SetLevel(log.DebugLevel) s := runNewServer(1, "1.1.1.1", 10179) - addVrf(t, s, "vrf1", "111:111", 1) + addVrf(t, s, "vrf1", "111:111", []string{"111:111"}, []string{"111:111"}, 1) req := &api.DeleteVrfRequest{Name: "Invalidvrf"} if err := s.DeleteVrf(context.Background(), req); err == nil { t.Fatal("Did not raise error for invalid vrf deletion.", err) @@ -1458,7 +1501,7 @@ func TestDeleteVrf(t *testing.T) { log.SetLevel(log.DebugLevel) s := runNewServer(1, "1.1.1.1", 10179) - addVrf(t, s, "vrf1", "111:111", 1) + addVrf(t, s, "vrf1", "111:111", []string{"111:111"}, []string{"111:111"}, 1) req := &api.DeleteVrfRequest{Name: "vrf1"} if err := s.DeleteVrf(context.Background(), req); err != nil { t.Fatal("Vrf delete failed", err) diff --git a/pkg/server/zclient.go b/pkg/server/zclient.go index 1c315a6c..390628f0 100644 --- a/pkg/server/zclient.go +++ b/pkg/server/zclient.go @@ -433,8 +433,8 @@ func (z *zebraClient) loop() { for _, path := range msg.PathList { vrfs := []uint32{0} if msg.Vrf != nil { - if v, ok := msg.Vrf[path.GetNlri().String()]; ok { - vrfs = append(vrfs, v) + for vrfId := range msg.Vrf { + vrfs = append(vrfs, vrfId) } } for _, i := range vrfs { diff --git a/test/scenario_test/zapi_v3_test.py b/test/scenario_test/zapi_v3_test.py index cd6348da..fe116488 100644 --- a/test/scenario_test/zapi_v3_test.py +++ b/test/scenario_test/zapi_v3_test.py @@ -13,8 +13,6 @@ # See the License for the specific language governing permissions and # limitations under the License. - - import sys import time import unittest @@ -58,7 +56,7 @@ class GoBGPTestBase(unittest.TestCase): def test_01_neighbor_established(self): self.g1.wait_for(expected_state=BGP_FSM_ESTABLISHED, peer=self.g2) - def test_02_create_vrf(self): + def test_02_vrf_routes_add(self): self.g1.local('ip netns add ns01') self.g1.local('ip netns add ns02') self.g2.local('ip netns add ns01') @@ -92,6 +90,60 @@ class GoBGPTestBase(unittest.TestCase): self.assertEqual(len(lines), 1) self.assertEqual(lines[0].split(' ')[0], '20.0.0.0/24') + def test_03_vrf_routes_del(self): + self.g1.local("gobgp vrf vrf01 rib del 10.0.0.0/24 nexthop 127.0.0.1") + self.g1.local("gobgp vrf vrf02 rib del 20.0.0.0/24 nexthop 127.0.0.1") + + time.sleep(2) + lines = self.g2.local("ip netns exec ns01 ip r", capture=True).split('\n') + self.assertEqual(len(lines), 1) + self.assertEqual(lines[0], "") + + lines = self.g2.local("ip netns exec ns02 ip r", capture=True).split('\n') + self.assertEqual(len(lines), 1) + self.assertEqual(lines[0], "") + + def test_04_vrf_import_routes(self): + self.g1.local("gobgp vrf del vrf01") + # Import vrf2 routes into vrf1 + self.g1.local("gobgp vrf add vrf01 id 1 rd 1:1 rt import 1:1 2:2 export 1:1") + + self.g1.local("gobgp vrf vrf01 rib add 10.0.0.0/24 nexthop 127.0.0.1") + self.g1.local("gobgp vrf vrf02 rib add 20.0.0.0/24 nexthop 127.0.0.1") + + time.sleep(2) + + # g1 has the vrf2 route imported to vrf1 and updated on zebra + lines = self.g1.local("ip netns exec ns01 ip r", capture=True).split('\n') + self.assertEqual(len(lines), 2) + route_destinations = set() + route_destinations.add(lines[0].split(' ')[0]) + route_destinations.add(lines[1].split(' ')[0]) + self.assertEqual(len(route_destinations.intersection(set(["10.0.0.0/24", "20.0.0.0/24"]))), 2) + + # Ensure other vrf and other neighbors are not impacted + lines = self.g1.local("ip netns exec ns02 ip r", capture=True).split('\n') + self.assertEqual(len(lines), 1) + self.assertEqual(lines[0].split(' ')[0], '20.0.0.0/24') + + lines = self.g2.local("ip netns exec ns01 ip r", capture=True).split('\n') + self.assertEqual(len(lines), 1) + self.assertEqual(lines[0].split(' ')[0], '10.0.0.0/24') + + lines = self.g2.local("ip netns exec ns02 ip r", capture=True).split('\n') + self.assertEqual(len(lines), 1) + self.assertEqual(lines[0].split(' ')[0], '20.0.0.0/24') + + # Routes imported from another vrf are cleaned up properly + self.g1.local("gobgp vrf vrf02 rib del 20.0.0.0/24 nexthop 127.0.0.1") + lines = self.g1.local("ip netns exec ns01 ip r", capture=True).split('\n') + self.assertEqual(len(lines), 1) + self.assertEqual(lines[0].split(' ')[0], '10.0.0.0/24') + + lines = self.g1.local("ip netns exec ns02 ip r", capture=True).split('\n') + self.assertEqual(len(lines), 1) + self.assertEqual(lines[0].split(' ')[0], '') + if __name__ == '__main__': output = local("which docker 2>&1 > /dev/null ; echo $?", capture=True) -- cgit v1.2.3