server: update path-attributes when peer is disconnected

close #807

also add a test to check this issue is fixed

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

3 files changed, 86 insertions, 113 deletions

diff --git a/server/peer.go b/server/peer.go
index a8b67b63..f5491b33 100644
--- a/server/peer.go
+++ b/server/peer.go
@@ -165,6 +165,38 @@ func (peer *Peer) getBestFromLocal(rfList []bgp.RouteFamily) ([]*table.Path, []*
 	return pathList, filtered
 }
 
+func (peer *Peer) processOutgoingPaths(paths []*table.Path) []*table.Path {
+	if peer.fsm.state != bgp.BGP_FSM_ESTABLISHED {
+		return nil
+	}
+	if peer.fsm.pConf.GracefulRestart.State.LocalRestarting {
+		log.WithFields(log.Fields{
+			"Topic": "Peer",
+			"Key":   peer.conf.Config.NeighborAddress,
+		}).Debug("now syncing, suppress sending updates")
+		return nil
+	}
+
+	outgoing := make([]*table.Path, 0, len(paths))
+	options := &table.PolicyOptions{
+		Neighbor: peer.fsm.peerInfo.Address,
+	}
+	for _, path := range paths {
+		path = peer.policy.ApplyPolicy(peer.TableID(), table.POLICY_DIRECTION_EXPORT, filterpath(peer, path), options)
+		if path == nil {
+			continue
+		}
+		if !peer.isRouteServerClient() && !peer.gConf.Collector.Enabled {
+			path = path.Clone(path.IsWithdraw)
+			path.UpdatePathAttrs(&peer.gConf, &peer.conf)
+		}
+		outgoing = append(outgoing, path)
+	}
+
+	peer.adjRibOut.Update(outgoing)
+	return outgoing
+}
+
 func (peer *Peer) handleBGPmessage(e *FsmMsg) ([]*table.Path, []*bgp.BGPMessage, []bgp.RouteFamily) {
 	m := e.MsgData.(*bgp.BGPMessage)
 	log.WithFields(log.Fields{
diff --git a/server/server.go b/server/server.go
index 532a7416..1102ab44 100644
--- a/server/server.go
+++ b/server/server.go
@@ -531,7 +531,6 @@ func filterpath(peer *Peer, path *table.Path) *table.Path {
 func (server *BgpServer) dropPeerAllRoutes(peer *Peer, families []bgp.RouteFamily) []*SenderMsg {
 	msgs := make([]*SenderMsg, 0)
 
-	options := &table.PolicyOptions{}
 	ids := make([]string, 0, len(server.neighborMap))
 	if peer.isRouteServerClient() {
 		for _, targetPeer := range server.neighborMap {
@@ -547,55 +546,12 @@ func (server *BgpServer) dropPeerAllRoutes(peer *Peer, families []bgp.RouteFamil
 		best, withdrawn := server.globalRib.DeletePathsByPeer(ids, peer.fsm.peerInfo, rf)
 		server.validatePaths(nil, withdrawn, true)
 
-		if peer.isRouteServerClient() {
-			for _, targetPeer := range server.neighborMap {
-				if !targetPeer.isRouteServerClient() || targetPeer == peer || targetPeer.fsm.state != bgp.BGP_FSM_ESTABLISHED {
-					continue
-				}
-				if _, ok := targetPeer.fsm.rfMap[rf]; !ok {
-					continue
-				}
-				pathList := make([]*table.Path, 0, len(best[targetPeer.TableID()]))
-				options.Neighbor = targetPeer.fsm.peerInfo.Address
-				for _, bestPath := range best[targetPeer.TableID()] {
-					if path := server.policy.ApplyPolicy(targetPeer.TableID(), table.POLICY_DIRECTION_EXPORT, filterpath(targetPeer, bestPath), options); path != nil {
-						pathList = append(pathList, path)
-					}
-				}
-				msgList := table.CreateUpdateMsgFromPaths(pathList)
-				msgs = append(msgs, newSenderMsg(targetPeer, msgList))
-				targetPeer.adjRibOut.Update(pathList)
-			}
-		} else {
-			sendPathList := make([]*table.Path, 0, len(best[table.GLOBAL_RIB_NAME]))
-			for _, path := range best[table.GLOBAL_RIB_NAME] {
-				if path != nil {
-					sendPathList = append(sendPathList, path)
-				}
-			}
-			if len(sendPathList) == 0 {
-				return msgs
+		for _, targetPeer := range server.neighborMap {
+			if peer.isRouteServerClient() != targetPeer.isRouteServerClient() || targetPeer == peer {
+				continue
 			}
-
-			server.broadcastBests(sendPathList)
-
-			for _, targetPeer := range server.neighborMap {
-				if targetPeer.isRouteServerClient() || targetPeer.fsm.state != bgp.BGP_FSM_ESTABLISHED {
-					continue
-				}
-				if _, ok := targetPeer.fsm.rfMap[rf]; !ok {
-					continue
-				}
-				pathList := make([]*table.Path, 0, len(sendPathList))
-				options.Neighbor = targetPeer.fsm.peerInfo.Address
-				for _, path := range sendPathList {
-					if path := server.policy.ApplyPolicy(table.GLOBAL_RIB_NAME, table.POLICY_DIRECTION_EXPORT, filterpath(targetPeer, path), options); path != nil {
-						pathList = append(pathList, path)
-					}
-				}
-				targetPeer.adjRibOut.Update(pathList)
-				msgList := table.CreateUpdateMsgFromPaths(pathList)
-
+			if paths := targetPeer.processOutgoingPaths(best[targetPeer.TableID()]); len(paths) > 0 {
+				msgList := table.CreateUpdateMsgFromPaths(paths)
 				msgs = append(msgs, newSenderMsg(targetPeer, msgList))
 			}
 		}
@@ -632,6 +588,9 @@ func (server *BgpServer) broadcastValidationResults(results []*api.ROAResult) {
 
 func (server *BgpServer) broadcastBests(bests []*table.Path) {
 	for _, path := range bests {
+		if path == nil {
+			continue
+		}
 		if !path.IsFromExternal() {
 			z := newBroadcastZapiBestMsg(server.zclient, path)
 			if z != nil {
@@ -813,10 +772,10 @@ func (server *BgpServer) validatePaths(newly, withdrawn []*table.Path, peerDown
 }
 
 func (server *BgpServer) propagateUpdate(peer *Peer, pathList []*table.Path) ([]*SenderMsg, []*table.Path) {
-	msgs := make([]*SenderMsg, 0)
 	rib := server.globalRib
-	var alteredPathList []*table.Path
-	options := &table.PolicyOptions{}
+	var alteredPathList, newly, withdrawn []*table.Path
+	var best map[string][]*table.Path
+
 	if peer != nil && peer.isRouteServerClient() {
 		for _, path := range pathList {
 			path.Filter(peer.ID(), table.POLICY_DIRECTION_IMPORT)
@@ -830,10 +789,7 @@ func (server *BgpServer) propagateUpdate(peer *Peer, pathList []*table.Path) ([]
 			moded = append(moded, server.RSimportPaths(targetPeer, pathList)...)
 		}
 		isTarget := func(p *Peer) bool {
-			if !p.isRouteServerClient() || p.fsm.state != bgp.BGP_FSM_ESTABLISHED || p.fsm.pConf.GracefulRestart.State.LocalRestarting {
-				return false
-			}
-			return true
+			return p.isRouteServerClient() && p.fsm.state == bgp.BGP_FSM_ESTABLISHED && !p.fsm.pConf.GracefulRestart.State.LocalRestarting
 		}
 
 		ids := make([]string, 0, len(server.neighborMap))
@@ -842,71 +798,30 @@ func (server *BgpServer) propagateUpdate(peer *Peer, pathList []*table.Path) ([]
 				ids = append(ids, targetPeer.TableID())
 			}
 		}
-		best, newly, withdrawn := rib.ProcessPaths(ids, append(pathList, moded...))
+		best, newly, withdrawn = rib.ProcessPaths(ids, append(pathList, moded...))
 		server.validatePaths(newly, withdrawn, false)
-
-		for _, targetPeer := range server.neighborMap {
-			if !isTarget(targetPeer) {
-				continue
-			}
-			sendPathList := make([]*table.Path, 0, len(best[targetPeer.TableID()]))
-			options.Neighbor = targetPeer.fsm.peerInfo.Address
-			for _, bestPath := range best[targetPeer.TableID()] {
-				path := server.policy.ApplyPolicy(targetPeer.TableID(), table.POLICY_DIRECTION_EXPORT, filterpath(targetPeer, bestPath), options)
-				if path != nil {
-					sendPathList = append(sendPathList, path)
-				}
-			}
-			msgList := table.CreateUpdateMsgFromPaths(sendPathList)
-			targetPeer.adjRibOut.Update(sendPathList)
-			msgs = append(msgs, newSenderMsg(targetPeer, msgList))
-		}
 	} else {
 		for idx, path := range pathList {
 			pathList[idx] = server.policy.ApplyPolicy(table.GLOBAL_RIB_NAME, table.POLICY_DIRECTION_IMPORT, path, nil)
 		}
 		alteredPathList = pathList
-		best, newly, withdrawn := rib.ProcessPaths([]string{table.GLOBAL_RIB_NAME}, pathList)
+		best, newly, withdrawn = rib.ProcessPaths([]string{table.GLOBAL_RIB_NAME}, pathList)
 		server.validatePaths(newly, withdrawn, false)
-		sendPathList := make([]*table.Path, 0, len(best[table.GLOBAL_RIB_NAME]))
-		if server.bgpConfig.Global.Collector.Enabled {
-			sendPathList = pathList
-		} else {
-			for _, path := range best[table.GLOBAL_RIB_NAME] {
-				if path != nil {
-					sendPathList = append(sendPathList, path)
-				}
-			}
-			if len(sendPathList) == 0 {
-				return msgs, alteredPathList
-			}
-			server.broadcastBests(sendPathList)
+		if len(best[table.GLOBAL_RIB_NAME]) == 0 {
+			return nil, alteredPathList
 		}
+		if !server.bgpConfig.Global.Collector.Enabled {
+			server.broadcastBests(best[table.GLOBAL_RIB_NAME])
+		}
+	}
 
-		for _, targetPeer := range server.neighborMap {
-			if targetPeer.isRouteServerClient() || targetPeer.fsm.state != bgp.BGP_FSM_ESTABLISHED || targetPeer.fsm.pConf.GracefulRestart.State.LocalRestarting {
-				if targetPeer.fsm.pConf.GracefulRestart.State.LocalRestarting {
-					log.WithFields(log.Fields{
-						"Topic": "Peer",
-						"Key":   targetPeer.conf.Config.NeighborAddress,
-					}).Debug("now syncing, suppress sending updates")
-				}
-				continue
-			}
-			pathList := make([]*table.Path, len(sendPathList))
-			copy(pathList, sendPathList)
-			options.Neighbor = targetPeer.fsm.peerInfo.Address
-			for idx, path := range pathList {
-				path = server.policy.ApplyPolicy(table.GLOBAL_RIB_NAME, table.POLICY_DIRECTION_EXPORT, filterpath(targetPeer, path), options)
-				if path != nil && !server.bgpConfig.Global.Collector.Enabled {
-					path = path.Clone(path.IsWithdraw)
-					path.UpdatePathAttrs(&server.bgpConfig.Global, &targetPeer.conf)
-				}
-				pathList[idx] = path
-			}
-			targetPeer.adjRibOut.Update(pathList)
-			msgList := table.CreateUpdateMsgFromPaths(pathList)
-
+	msgs := make([]*SenderMsg, 0)
+	for _, targetPeer := range server.neighborMap {
+		if (peer == nil && targetPeer.isRouteServerClient()) || (peer != nil && peer.isRouteServerClient() != targetPeer.isRouteServerClient()) {
+			continue
+		}
+		if paths := targetPeer.processOutgoingPaths(best[targetPeer.TableID()]); len(paths) > 0 {
+			msgList := table.CreateUpdateMsgFromPaths(paths)
 			msgs = append(msgs, newSenderMsg(targetPeer, msgList))
 		}
 	}
diff --git a/test/scenario_test/bgp_router_test.py b/test/scenario_test/bgp_router_test.py
index dd7c1edb..9486ac44 100644
--- a/test/scenario_test/bgp_router_test.py
+++ b/test/scenario_test/bgp_router_test.py
@@ -251,7 +251,7 @@ class GoBGPTestBase(unittest.TestCase):
 
     def test_15_check_active_connection(self):
         g1 = self.gobgp
-        g2 = GoBGPContainer(name='g2', asn=65000, router_id='192.168.0.5',
+        g2 = GoBGPContainer(name='g2', asn=65000, router_id='192.168.0.7',
                             ctn_image_name=self.gobgp.image,
                             log_level=parser_option.gobgp_log_level)
         time.sleep(g2.run())
@@ -285,6 +285,32 @@ class GoBGPTestBase(unittest.TestCase):
         med = extract_path_attribute(path, BGP_ATTR_TYPE_MULTI_EXIT_DISC)
         self.assertTrue(med['metric'] == 2000)
 
+    def test_17_check_shutdown(self):
+        q1 = self.quaggas['q1']
+        q2 = self.quaggas['q2']
+        q3 = self.quaggas['q3']
+
+        q2.add_route('20.0.0.0/24')
+        q3.add_route('20.0.0.0/24')
+
+        self.test_01_neighbor_established()
+
+        self.test_02_check_gobgp_global_rib()
+
+        paths = q1.get_global_rib('20.0.0.0/24')
+        self.assertTrue(len(paths) == 1)
+        n_addrs = [i[1].split('/')[0] for i in self.gobgp.ip_addrs]
+        self.assertTrue(paths[0]['nexthop'] in n_addrs)
+
+        q3.stop()
+
+        time.sleep(3)
+
+        paths = q1.get_global_rib('20.0.0.0/24')
+        self.assertTrue(len(paths) == 1)
+        self.assertTrue(paths[0]['nexthop'] in n_addrs)
+
+
 if __name__ == '__main__':
     if os.geteuid() is not 0:
         print "you are not root."