Maintain unique vrf IDs in watchEventBestPath vrf map

Fixes missing route propagation to proper vrfs in zapi

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)