server: fix bug of withdrawal handling

Signed-off-by: Wataru Ishida <ishida.wataru@lab.ntt.co.jp>

2 files changed, 57 insertions, 12 deletions

diff --git a/server/server.go b/server/server.go
index 7b47b67b..e51ba977 100644
--- a/server/server.go
+++ b/server/server.go
@@ -360,23 +360,25 @@ func filterpath(peer *Peer, path *table.Path, withdrawals []*table.Path) *table.
 	}
 
 	if peer.ID() == path.GetSource().Address.String() {
-		// Say, gobgp was advertising prefix A and peer P also.
-		// When gobgp withdraws prefix A, best path calculation chooses
-		// the path from P as the best path for prefix A.
-		// For peers other than P, this path should be advertised
-		// (as implicit withdrawal). However for P, we should advertise
-		// the local withdraw path.
-
 		// Note: multiple paths having the same prefix could exist the
 		// withdrawals list in the case of Route Server setup with
 		// import policies modifying paths. In such case, gobgp sends
 		// duplicated update messages; withdraw messages for the same
 		// prefix.
-		// However, currently we don't support local path for Route
-		// Server setup so this is NOT the case.
-		for _, w := range withdrawals {
-			if w.IsLocal() && path.GetNlri().String() == w.GetNlri().String() {
-				return w
+		if !peer.isRouteServerClient() {
+			// Say, peer A and B advertized same prefix P, and
+			// best path calculation chose a path from B as best.
+			// When B withdraws prefix P, best path calculation chooses
+			// the path from A as best.
+			// For peers other than A, this path should be advertised
+			// (as implicit withdrawal). However for A, we should advertise
+			// the withdrawal path.
+			// Thing is same when peer A and we advertized prefix P (as local
+			// route), then, we withdraws the prefix.
+			for _, w := range withdrawals {
+				if path.GetNlri().String() == w.GetNlri().String() {
+					return w
+				}
 			}
 		}
 		log.WithFields(log.Fields{
diff --git a/test/scenario_test/bgp_router_test.py b/test/scenario_test/bgp_router_test.py
index 5f85c23a..1a72c3e5 100644
--- a/test/scenario_test/bgp_router_test.py
+++ b/test/scenario_test/bgp_router_test.py
@@ -414,6 +414,49 @@ class GoBGPTestBase(unittest.TestCase):
 
         self.assertTrue(cnt == cnt2)
 
+
+    def test_22_check_withdrawal3(self):
+        gobgp_ctn_image_name = parser_option.gobgp_image
+        g1 = self.gobgp
+        g2 = GoBGPContainer(name='g2', asn=65006, router_id='192.168.0.8',
+                            ctn_image_name=gobgp_ctn_image_name,
+                            log_level=parser_option.gobgp_log_level)
+        g3 = GoBGPContainer(name='g3', asn=65007, router_id='192.168.0.9',
+                            ctn_image_name=gobgp_ctn_image_name,
+                            log_level=parser_option.gobgp_log_level)
+
+        initial_wait_time = max(ctn.run() for ctn in [g2, g3])
+        time.sleep(initial_wait_time)
+
+        self.quaggas = {'g2': g2, 'g3': g3}
+
+        g2.local('gobgp global rib add 50.0.0.0/24')
+        g3.local('gobgp global rib add 50.0.0.0/24 med 10')
+
+        g1.add_peer(g2)
+        g2.add_peer(g1)
+        g1.add_peer(g3)
+        g3.add_peer(g1)
+
+        self.test_01_neighbor_established()
+
+        self.test_02_check_gobgp_global_rib()
+
+        paths = g1.get_adj_rib_out(g2, '50.0.0.0/24')
+        self.assertTrue(len(paths) == 0)
+        paths = g1.get_adj_rib_out(g3, '50.0.0.0/24')
+        self.assertTrue(len(paths) == 1)
+        self.assertTrue(paths[0]['source-id'] == '192.168.0.8')
+
+        g2.local('gobgp global rib del 50.0.0.0/24')
+
+        paths = g1.get_adj_rib_out(g2, '50.0.0.0/24')
+        self.assertTrue(len(paths) == 1)
+        self.assertTrue(paths[0]['source-id'] == '192.168.0.9')
+        paths = g1.get_adj_rib_out(g3, '50.0.0.0/24')
+        self.assertTrue(len(paths) == 0)
+
+
 if __name__ == '__main__':
     if os.geteuid() is not 0:
         print "you are not root."