server: fix advertising multiple local withdrawals with same prefix

a bug introduced by 332766189685028c4f9852e4285fb1a9025223cc

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

2 files changed, 24 insertions, 19 deletions

diff --git a/server/peer.go b/server/peer.go
index 030044d0..d27f96e6 100644
--- a/server/peer.go
+++ b/server/peer.go
@@ -128,7 +128,7 @@ func (peer *Peer) getAccepted(rfList []bgp.RouteFamily) []*table.Path {
 	return peer.adjRibIn.PathList(rfList, true)
 }
 
-func (peer *Peer) filterpath(path *table.Path) *table.Path {
+func (peer *Peer) filterpath(path *table.Path, withdrawals []*table.Path) *table.Path {
 	// special handling for RTC nlri
 	// see comments in (*Destination).Calculate()
 	if path != nil && path.GetRouteFamily() == bgp.RF_RTC_UC && !path.IsWithdraw {
@@ -147,7 +147,7 @@ func (peer *Peer) filterpath(path *table.Path) *table.Path {
 			}
 		}
 	}
-	if filterpath(peer, path) == nil {
+	if path = filterpath(peer, path, withdrawals); path == nil {
 		return nil
 	}
 
@@ -172,7 +172,7 @@ func (peer *Peer) getBestFromLocal(rfList []bgp.RouteFamily) ([]*table.Path, []*
 	pathList := []*table.Path{}
 	filtered := []*table.Path{}
 	for _, path := range peer.localRib.GetBestPathList(peer.TableID(), rfList) {
-		if p := peer.filterpath(path); p != nil {
+		if p := peer.filterpath(path, nil); p != nil {
 			pathList = append(pathList, p)
 		} else {
 			filtered = append(filtered, path)
@@ -200,23 +200,9 @@ func (peer *Peer) processOutgoingPaths(paths, withdrawals []*table.Path) []*tabl
 	}
 
 	outgoing := make([]*table.Path, 0, len(paths))
-	// 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 _, path := range withdrawals {
-		if path.IsLocal() {
-			if _, ok := peer.fsm.rfMap[path.GetRouteFamily()]; ok {
-				outgoing = append(outgoing, path)
-			}
-		}
-	}
 
 	for _, path := range paths {
-		if p := peer.filterpath(path); p != nil {
+		if p := peer.filterpath(path, withdrawals); p != nil {
 			outgoing = append(outgoing, p)
 		}
 	}
diff --git a/server/server.go b/server/server.go
index 1954d124..c2386f94 100644
--- a/server/server.go
+++ b/server/server.go
@@ -267,7 +267,7 @@ func isASLoop(peer *Peer, path *table.Path) bool {
 	return false
 }
 
-func filterpath(peer *Peer, path *table.Path) *table.Path {
+func filterpath(peer *Peer, path *table.Path, withdrawals []*table.Path) *table.Path {
 	if path == nil {
 		return nil
 	}
@@ -349,6 +349,25 @@ func filterpath(peer *Peer, path *table.Path) *table.Path {
 	}
 
 	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
+			}
+		}
 		log.WithFields(log.Fields{
 			"Topic": "Peer",
 			"Key":   peer.ID(),