diff --git a/internal/pkg/table/path.go b/internal/pkg/table/path.go index dc3139452..da4066cdc 100644 --- a/internal/pkg/table/path.go +++ b/internal/pkg/table/path.go @@ -142,6 +142,7 @@ type Path struct { IsNexthopInvalid bool IsWithdraw bool } + type FilteredType uint8 const ( @@ -150,7 +151,21 @@ const ( SendMaxFiltered ) -type PathLocalKey string +type PathDestLocalKey struct { + Family bgp.RouteFamily + Prefix string +} +type PathLocalKey struct { + PathDestLocalKey + Id uint32 +} + +func NewPathDestLocalKey(f bgp.RouteFamily, destPrefix string) *PathDestLocalKey { + return &PathDestLocalKey{ + Family: f, + Prefix: destPrefix, + } +} var localSource = &PeerInfo{} @@ -590,8 +605,18 @@ func (path *Path) String() string { // GetLocalKey identifies the path in the local BGP server. func (path *Path) GetLocalKey() PathLocalKey { - // return PathLocalKey(path.GetPrefix()) - return PathLocalKey(fmt.Sprintf("%s:%s:%d", path.GetRouteFamily(), path.GetNlri(), path.GetNlri().PathLocalIdentifier())) + return PathLocalKey{ + PathDestLocalKey: path.GetDestLocalKey(), + Id: path.GetNlri().PathLocalIdentifier(), + } +} + +// GetDestLocalKey identifies the path destination in the local BGP server. +func (path *Path) GetDestLocalKey() PathDestLocalKey { + return PathDestLocalKey{ + Family: path.GetRouteFamily(), + Prefix: path.GetNlri().String(), + } } func (path *Path) GetPrefix() string { diff --git a/pkg/server/peer.go b/pkg/server/peer.go index 06ab7cc28..3dac72b49 100644 --- a/pkg/server/peer.go +++ b/pkg/server/peer.go @@ -98,13 +98,14 @@ func newDynamicPeer(g *oc.Global, neighborAddress string, pg *oc.PeerGroup, loc } type peer struct { - tableId string - fsm *fsm - adjRibIn *table.AdjRib - policy *table.RoutingPolicy - localRib *table.TableManager - prefixLimitWarned map[bgp.RouteFamily]bool - dstRoutesCount map[bgp.RouteFamily]map[string]uint8 + tableId string + fsm *fsm + adjRibIn *table.AdjRib + policy *table.RoutingPolicy + localRib *table.TableManager + prefixLimitWarned map[bgp.RouteFamily]bool + // map of path local identifiers sent for that prefix + sentPaths map[table.PathDestLocalKey]map[uint32]struct{} sendMaxPathFiltered map[table.PathLocalKey]struct{} llgrEndChs []chan struct{} } @@ -115,7 +116,7 @@ func newPeer(g *oc.Global, conf *oc.Neighbor, loc *table.TableManager, policy *t policy: policy, fsm: newFSM(g, conf, logger), prefixLimitWarned: make(map[bgp.RouteFamily]bool), - dstRoutesCount: make(map[bgp.RouteFamily]map[string]uint8), + sentPaths: make(map[table.PathDestLocalKey]map[uint32]struct{}), sendMaxPathFiltered: make(map[table.PathLocalKey]struct{}), } if peer.isRouteServerClient() { @@ -215,56 +216,39 @@ func (peer *peer) getAddPathSendMax(family bgp.RouteFamily) uint8 { return 0 } -func (peer *peer) getRoutesCount(dstPrefix string, family bgp.RouteFamily) uint8 { - peer.fsm.lock.RLock() - defer peer.fsm.lock.RUnlock() - if _, ok := peer.dstRoutesCount[family]; ok { - return peer.dstRoutesCount[family][dstPrefix] +func (peer *peer) getRoutesCount(family bgp.RouteFamily, dstPrefix string) uint8 { + destLocalKey := table.NewPathDestLocalKey(family, dstPrefix) + if identifiers, ok := peer.sentPaths[*destLocalKey]; ok { + count := len(identifiers) + // the send-max config is uint8, so we need to check for overflow + if count > int(^uint8(0)) { + return ^uint8(0) + } + return uint8(count) } return 0 } -func (peer *peer) setRoutesCount(dstPrefix string, family bgp.RouteFamily, count uint8) { - peer.fsm.lock.Lock() - defer peer.fsm.lock.Unlock() - if _, ok := peer.dstRoutesCount[family]; !ok { - peer.dstRoutesCount[family] = make(map[string]uint8) - } - peer.dstRoutesCount[family][dstPrefix] = count -} - -func (peer *peer) incrementRoutesCount(dstPrefix string, family bgp.RouteFamily, inc uint8) { - if inc == 0 { - return - } - - peer.fsm.lock.Lock() - defer peer.fsm.lock.Unlock() - if _, ok := peer.dstRoutesCount[family]; !ok { - peer.dstRoutesCount[family] = make(map[string]uint8) - } - newCount := peer.dstRoutesCount[family][dstPrefix] + inc - if newCount < peer.dstRoutesCount[family][dstPrefix] { - newCount = 0xFF - } - peer.dstRoutesCount[family][dstPrefix] = newCount -} - -func (peer *peer) decrementRoutesCount(dstPrefix string, family bgp.RouteFamily, dec uint8) { - if dec == 0 { +func (peer *peer) updateRoutes(paths ...*table.Path) { + if len(paths) == 0 { return } - - peer.fsm.lock.Lock() - defer peer.fsm.lock.Unlock() - if _, ok := peer.dstRoutesCount[family]; !ok { - peer.dstRoutesCount[family] = make(map[string]uint8) - } - newCount := peer.dstRoutesCount[family][dstPrefix] - dec - if newCount > peer.dstRoutesCount[family][dstPrefix] { - newCount = 0 + for _, path := range paths { + localKey := path.GetLocalKey() + destLocalKey := localKey.PathDestLocalKey + identifiers, destExists := peer.sentPaths[destLocalKey] + if path.IsWithdraw && destExists { + delete(identifiers, path.GetNlri().PathLocalIdentifier()) + } else if !path.IsWithdraw { + if !destExists { + peer.sentPaths[destLocalKey] = make(map[uint32]struct{}) + } + identifiers := peer.sentPaths[destLocalKey] + if len(identifiers) < int(peer.getAddPathSendMax(destLocalKey.Family)) { + identifiers[localKey.Id] = struct{}{} + } + } } - peer.dstRoutesCount[family][dstPrefix] = newCount } func (peer *peer) isPathSendMaxFiltered(path *table.Path) bool { @@ -286,44 +270,16 @@ func (peer *peer) unsetPathSendMaxFiltered(path *table.Path) bool { return true } -func (peer *peer) getSendMaxFilteredPathList(dest *table.Destination, limit int) []*table.Path { - knownPathList := dest.GetKnownPathList(peer.TableID(), peer.AS()) - list := make([]*table.Path, 0, len(knownPathList)) - for _, p := range knownPathList { - if !peer.isPathSendMaxFiltered(p) { - continue - } - list = append(list, p) - if limit > 0 && len(list) == limit { - break - } - } - return list -} - -func (peer *peer) canSendPathWithinLimit(path *table.Path) bool { +func (peer *peer) hasPathAlreadyBeenSent(path *table.Path) bool { if path == nil { return false } - - family := path.GetRouteFamily() - dstPrefix := path.GetPrefix() - sendMax := peer.getAddPathSendMax(family) - dstRouteCount := peer.getRoutesCount(dstPrefix, family) - - if dstRouteCount >= sendMax { - peer.sendMaxPathFiltered[path.GetLocalKey()] = struct{}{} + destLocalKey := path.GetDestLocalKey() + if _, dstExist := peer.sentPaths[destLocalKey]; !dstExist { return false } - - if dstRouteCount > 0 && path.IsWithdraw { - peer.decrementRoutesCount(dstPrefix, family, 1) - } else if dstRouteCount < sendMax && !path.IsWithdraw { - peer.incrementRoutesCount(dstPrefix, family, 1) - } else { - return false - } - return true + _, pathExist := peer.sentPaths[destLocalKey][path.GetNlri().PathLocalIdentifier()] + return pathExist } func (peer *peer) isDynamicNeighbor() bool { diff --git a/pkg/server/server.go b/pkg/server/server.go index a43ebe928..e0e5220d0 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -1346,15 +1346,19 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * return family }() if targetPeer.isAddPathSendEnabled(f) { + // in case of multiple paths to the same destination, we need to + // filter the paths before counting the number of paths to be sent. if newPath.IsWithdraw { bestList = func() []*table.Path { - l := make([]*table.Path, 0, len(dsts)) + l := []*table.Path{} for _, d := range dsts { toDelete := d.GetWithdrawnPath() toActuallyDelete := make([]*table.Path, 0, len(toDelete)) for _, p := range toDelete { + // if the path is filtered, there is no need to send the withdrawal + p := s.filterpath(targetPeer, p, nil) // the path was never advertized to the peer - if targetPeer.unsetPathSendMaxFiltered(p) { + if p == nil || targetPeer.unsetPathSendMaxFiltered(p) { continue } toActuallyDelete = append(toActuallyDelete, p) @@ -1365,8 +1369,6 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * } destination := rib.GetDestination(toActuallyDelete[0]) - dstPrefix := toActuallyDelete[0].GetPrefix() - targetPeer.decrementRoutesCount(dstPrefix, f, uint8(len(toActuallyDelete))) l = append(l, toActuallyDelete...) // the destination has been removed from the table @@ -1375,54 +1377,78 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * continue } - toAdd := targetPeer.getSendMaxFilteredPathList(destination, len(toActuallyDelete)) - targetPeer.incrementRoutesCount(dstPrefix, f, uint8(len(toAdd))) - for _, p := range toAdd { - targetPeer.unsetPathSendMaxFiltered(p) + knownPathList := destination.GetKnownPathList(targetPeer.TableID(), targetPeer.AS()) + toAdd := make([]*table.Path, 0, len(knownPathList)) + for _, p := range knownPathList { + // if the path is filtered, there is no need to send the path + p := s.filterpath(targetPeer, p, nil) + if p == nil || !targetPeer.isPathSendMaxFiltered(p) { + continue + } + toAdd = append(toAdd, p) + if len(toAdd) == len(toActuallyDelete) { + break + } } l = append(l, toAdd...) } + targetPeer.updateRoutes(l...) return l }() - } else if targetPeer.canSendPathWithinLimit(newPath) { - bestList = []*table.Path{newPath} - if newPath.GetRouteFamily() == bgp.RF_RTC_UC { - // we assumes that new "path" nlri was already sent before. This assumption avoids the - // infinite UPDATE loop between Route Reflector and its clients. - for _, old := range dsts[0].OldKnownPathList { - if old.IsLocal() { - bestList = []*table.Path{} - break + } else { + alreadySent := targetPeer.hasPathAlreadyBeenSent(newPath) + newPath := s.filterpath(targetPeer, newPath, nil) + // if the path is not filtered and the path has already been sent or land in the limit, we can send it + if newPath == nil { + bestList = []*table.Path{} + } else if alreadySent || targetPeer.getRoutesCount(f, newPath.GetPrefix()) < targetPeer.getAddPathSendMax(f) { + bestList = []*table.Path{newPath} + if !alreadySent { + targetPeer.updateRoutes(newPath) + } + if newPath.GetRouteFamily() == bgp.RF_RTC_UC { + // we assumes that new "path" nlri was already sent before. This assumption avoids the + // infinite UPDATE loop between Route Reflector and its clients. + for _, old := range dsts[0].OldKnownPathList { + if old.IsLocal() { + bestList = []*table.Path{} + break + } } } + } else { + bestList = []*table.Path{} + targetPeer.sendMaxPathFiltered[newPath.GetLocalKey()] = struct{}{} + s.logger.Warn("exceeding max routes for prefix", + log.Fields{ + "Topic": "Peer", + "Key": targetPeer.ID(), + "Prefix": newPath.GetPrefix(), + }) } - } else { - bestList = []*table.Path{} - s.logger.Warn("exceeding max routes for prefix", - log.Fields{ - "Topic": "Peer", - "Key": targetPeer.ID(), - "Prefix": newPath.GetPrefix(), - }) } - oldList = nil - } else if targetPeer.isRouteServerClient() { - if targetPeer.isSecondaryRouteEnabled() { - if paths := s.sendSecondaryRoutes(targetPeer, newPath, dsts); len(paths) > 0 { - sendfsmOutgoingMsg(targetPeer, paths, nil, false) - } - continue + if needToAdvertise(targetPeer) && len(bestList) > 0 { + sendfsmOutgoingMsg(targetPeer, bestList, nil, false) } - bestList, oldList, _ = dstsToPaths(targetPeer.TableID(), targetPeer.AS(), dsts) } else { - bestList = gBestList - oldList = gOldList - } - if !needOld { - oldList = nil - } - if paths := s.processOutgoingPaths(targetPeer, bestList, oldList); len(paths) > 0 { - sendfsmOutgoingMsg(targetPeer, paths, nil, false) + if targetPeer.isRouteServerClient() { + if targetPeer.isSecondaryRouteEnabled() { + if paths := s.sendSecondaryRoutes(targetPeer, newPath, dsts); len(paths) > 0 { + sendfsmOutgoingMsg(targetPeer, paths, nil, false) + } + continue + } + bestList, oldList, _ = dstsToPaths(targetPeer.TableID(), targetPeer.AS(), dsts) + } else { + bestList = gBestList + oldList = gOldList + } + if !needOld { + oldList = nil + } + if paths := s.processOutgoingPaths(targetPeer, bestList, oldList); len(paths) > 0 { + sendfsmOutgoingMsg(targetPeer, paths, nil, false) + } } } } diff --git a/test/scenario_test/addpath_test.py b/test/scenario_test/addpath_test.py index 965b568eb..41e4c8e04 100644 --- a/test/scenario_test/addpath_test.py +++ b/test/scenario_test/addpath_test.py @@ -52,9 +52,23 @@ def setUpClass(cls): g3 = GoBGPContainer(name='g3', asn=65000, router_id='192.168.0.3', ctn_image_name=gobgp_ctn_image_name, log_level=parser_option.gobgp_log_level) - e1 = ExaBGPContainer(name='e1', asn=65000, router_id='192.168.0.4') - - ctns = [g1, g2, g3, e1] + g4 = GoBGPContainer( + name="g4", + asn=65000, + router_id="192.168.0.4", + ctn_image_name=gobgp_ctn_image_name, + log_level=parser_option.gobgp_log_level, + ) + g5 = GoBGPContainer( + name="g5", + asn=65000, + router_id="192.168.0.5", + ctn_image_name=gobgp_ctn_image_name, + log_level=parser_option.gobgp_log_level, + ) + e1 = ExaBGPContainer(name="e1", asn=65000, router_id="192.168.0.6") + + ctns = [g1, g2, g3, g4, g5, e1] initial_wait_time = max(ctn.run() for ctn in ctns) time.sleep(initial_wait_time) @@ -68,9 +82,17 @@ def setUpClass(cls): g1.add_peer(g3, addpath=cls.SEND_MAX, is_rr_client=True) g3.add_peer(g1, addpath=cls.SEND_MAX) + g4.add_peer( + g5, + addpath=cls.SEND_MAX, + ) + g5.add_peer(g4, addpath=cls.SEND_MAX) + cls.g1 = g1 cls.g2 = g2 cls.g3 = g3 + cls.g4 = g4 + cls.g5 = g5 cls.e1 = e1 # test each neighbor state is turned establish @@ -78,6 +100,7 @@ def test_00_neighbor_established(self): self.g1.wait_for(expected_state=BGP_FSM_ESTABLISHED, peer=self.g2) self.g1.wait_for(expected_state=BGP_FSM_ESTABLISHED, peer=self.g3) self.g1.wait_for(expected_state=BGP_FSM_ESTABLISHED, peer=self.e1) + self.g4.wait_for(expected_state=BGP_FSM_ESTABLISHED, peer=self.g5) # prepare routes with path_id (no error check) def test_01_prepare_add_paths_routes(self): @@ -90,8 +113,42 @@ def test_01_prepare_add_paths_routes(self): aspath=aspath, ) - # test three routes are installed to the rib due to add-path feature - def test_02_check_g1_global_rib(self): + # update multiple time the same route + # and expect the route counter to increment only once + for i in range(self.INSTALLED_PATHS): + self.g4.add_route( + route="192.168.100.0/24", + identifier=10, + local_pref=i + 100 + 1, + ) + + def test_02_check_g4_adj_out(self): + def f(): + # the last update should have been received by g5 + rib = self.g4.get_adj_rib_out(self.g5, add_path_enabled=True) + self.assertEqual(len(rib), 1) + self.assertEqual(len(rib[0]["paths"]), 1) + self.assertEqual( + rib[0]["paths"][0]["local-pref"], 100 + self.INSTALLED_PATHS + ) + self.assertFalse(rib[0]["paths"][0].get("send-max-filtered", False)) + + assert_several_times(f) + + def test_03_check_g5_global_rib(self): + def f(): + # the last update should have been received by g5 + rib = self.g5.get_global_rib() + self.assertEqual(len(rib), 1) + self.assertEqual(len(rib[0]["paths"]), 1) + self.assertEqual( + rib[0]["paths"][0]["local-pref"], 100 + self.INSTALLED_PATHS + ) + + assert_several_times(f) + + # test INSTALLED_PATHS routes are installed to the rib due to add-path feature + def test_04_check_g1_global_rib(self): def f(): rib = self.g1.get_global_rib() self.assertEqual(len(rib), 1) @@ -100,7 +157,7 @@ def f(): assert_several_times(f) # test only the best path is advertised to g2 - def test_03_check_g2_global_rib(self): + def test_05_check_g2_global_rib(self): def f(): rib = self.g2.get_global_rib() self.assertEqual(len(rib), 1) @@ -109,8 +166,8 @@ def f(): assert_several_times(f) - # test three routes are advertised to g3 - def test_04_check_g3_global_rib(self): + # test SEND_MAX routes are advertised to g3 + def test_06_check_g3_global_rib(self): def f(): rib = self.g3.get_global_rib() self.assertEqual(len(rib), 1) @@ -118,7 +175,7 @@ def f(): assert_several_times(f) - def test_05_check_g1_adj_out(self): + def test_07_check_g1_adj_out(self): adj_out = self.g1.get_adj_rib_out(self.g2, add_path_enabled=True) self.assertEqual(len(adj_out), 1) self.assertEqual(len(adj_out[0]["paths"]), 1) @@ -130,11 +187,11 @@ def test_05_check_g1_adj_out(self): self.assertTrue(adj_out[0]["paths"][-1].get("send-max-filtered", False)) # withdraw a route with path_id (no error check) - def test_06_withdraw_route_with_path_id(self): + def test_08_withdraw_route_with_path_id(self): self.e1.del_route(route="192.168.100.0/24", identifier=10) # test the withdrawn route is removed from the rib - def test_07_check_g1_global_rib(self): + def test_09_check_g1_global_rib(self): def f(): rib = self.g1.get_global_rib() self.assertEqual(len(rib), 1) @@ -146,7 +203,7 @@ def f(): assert_several_times(f) # test the best path is replaced due to the removal from g1 rib - def test_08_check_g2_global_rib(self): + def test_10_check_g2_global_rib(self): def f(): rib = self.g2.get_global_rib() self.assertEqual(len(rib), 1) @@ -157,7 +214,7 @@ def f(): # test the withdrawn route is removed from the rib of g3 # and the filtered route is advertised to g3 - def test_09_check_g3_global_rib(self): + def test_11_check_g3_global_rib(self): def f(): rib = self.g3.get_global_rib() self.assertEqual(len(rib), 1) @@ -168,12 +225,12 @@ def f(): assert_several_times(f) # install a route with path_id via GoBGP CLI (no error check) - def test_10_install_add_paths_route_via_cli(self): + def test_12_install_add_paths_route_via_cli(self): # identifier is duplicated with the identifier of the route from e1 self.g1.add_route(route='192.168.100.0/24', identifier=10, local_pref=500) # test the route from CLI is installed to the rib - def test_11_check_g1_global_rib(self): + def test_13_check_g1_global_rib(self): def f(): rib = self.g1.get_global_rib() self.assertEqual(len(rib), 1) @@ -186,7 +243,7 @@ def f(): assert_several_times(f) - def test_12_check_g1_adj_out(self): + def test_14_check_g1_adj_out(self): adj_out = self.g1.get_adj_rib_out(self.g2, add_path_enabled=True) self.assertEqual(len(adj_out), 1) self.assertEqual(len(adj_out[0]["paths"]), 1) @@ -194,14 +251,13 @@ def test_12_check_g1_adj_out(self): adj_out = self.g1.get_adj_rib_out(self.g3, add_path_enabled=True) self.assertEqual(len(adj_out), 1) self.assertEqual(len(adj_out[0]["paths"]), self.INSTALLED_PATHS) - print(json.dumps(adj_out, indent=2)) # the new best path shouldn't be advertised as it is added after # the limit is reached self.assertEqual(adj_out[0]["paths"][0]["local-pref"], 500) self.assertTrue(adj_out[0]["paths"][0].get("send-max-filtered", False)) # test the best path is replaced due to the CLI route from g1 rib - def test_13_check_g2_global_rib(self): + def test_15_check_g2_global_rib(self): def f(): rib = self.g2.get_global_rib() self.assertEqual(len(rib), 1) @@ -212,11 +268,10 @@ def f(): assert_several_times(f) # test the route from CLI is advertised from g1 - def test_14_check_g3_global_rib(self): + def test_16_check_g3_global_rib(self): def f(): rib = self.g3.get_global_rib() self.assertEqual(len(rib), 1) - print(json.dumps(rib, indent=2)) self.assertEqual(len(rib[0]["paths"]), self.SEND_MAX) for path in rib[0]['paths']: self.assertTrue(2 <= len(path["aspath"]) <= self.INSTALLED_PATHS) @@ -224,13 +279,13 @@ def f(): assert_several_times(f) # remove non-existing route with path_id via GoBGP CLI (no error check) - def test_15_remove_non_existing_add_paths_route_via_cli(self): + def test_17_remove_non_existing_add_paths_route_via_cli(self): # specify locally non-existing identifier which has the same value # with the identifier of the route from e1 self.g1.del_route(route='192.168.100.0/24', identifier=20) # test none of route is removed by non-existing path_id via CLI - def test_16_check_g1_global_rib(self): + def test_18_check_g1_global_rib(self): def f(): rib = self.g1.get_global_rib() self.assertEqual(len(rib), 1) @@ -244,10 +299,10 @@ def f(): assert_several_times(f) # remove route with path_id via GoBGP CLI (no error check) - def test_17_remove_add_paths_route_via_cli(self): + def test_19_remove_add_paths_route_via_cli(self): self.g1.del_route(route='192.168.100.0/24', identifier=10) - def test_18_check_g1_adj_out(self): + def test_20_check_g1_adj_out(self): adj_out = self.g1.get_adj_rib_out(self.g2, add_path_enabled=True) self.assertEqual(len(adj_out), 1) self.assertEqual(len(adj_out[0]["paths"]), 1) @@ -257,7 +312,7 @@ def test_18_check_g1_adj_out(self): self.assertEqual(len(adj_out[0]["paths"]), self.INSTALLED_PATHS - 1) # test the route is removed from the rib via CLI - def test_19_check_g1_global_rib(self): + def test_21_check_g1_global_rib(self): def f(): rib = self.g1.get_global_rib() self.assertEqual(len(rib), 1) @@ -269,7 +324,7 @@ def f(): assert_several_times(f) # test the best path is replaced the removal from g1 rib - def test_20_check_g2_global_rib(self): + def test_22_check_g2_global_rib(self): def f(): rib = self.g2.get_global_rib() self.assertEqual(len(rib), 1) @@ -279,7 +334,7 @@ def f(): assert_several_times(f) # test the removed route from CLI is withdrawn by g1 - def test_21_check_g3_global_rib(self): + def test_23_check_g3_global_rib(self): def f(): rib = self.g3.get_global_rib() self.assertEqual(len(rib), 1)