From da44c7f21b73969f993a4483b26e50f63948d13c Mon Sep 17 00:00:00 2001 From: Arthur de Kerhor Date: Mon, 29 Jul 2024 16:02:50 +0200 Subject: [PATCH] send-max: fix flag when path is not filtered anymore when a path that was previously filtered becomes advertised because another path is withdrawn, the sendMaxFiltered flag needs to be unset. Otherwise, this path would never be deleted from the peer unless the peer is removed. --- cmd/gobgp/neighbor.go | 5 ++- pkg/server/server.go | 5 ++- test/scenario_test/addpath_test.py | 64 +++++++++++++++--------------- 3 files changed, 39 insertions(+), 35 deletions(-) diff --git a/cmd/gobgp/neighbor.go b/cmd/gobgp/neighbor.go index 54547479d..14f0a47af 100644 --- a/cmd/gobgp/neighbor.go +++ b/cmd/gobgp/neighbor.go @@ -699,12 +699,13 @@ func showRoute(dsts []*api.Destination, showAge, showBest, showLabel, showMUP, s format += "%-10s " } headers = append(headers, "Attrs") - format += "%-s\n" + format += "%-s" if showSendMaxFiltered { headers = append(headers, "Filtered") - format += "%-s\n" + format += "%-s" } + format += "\n" fmt.Printf(format, headers...) for _, pathStr := range pathStrs { diff --git a/pkg/server/server.go b/pkg/server/server.go index 4664351b2..8888f39ab 100644 --- a/pkg/server/server.go +++ b/pkg/server/server.go @@ -1381,11 +1381,14 @@ func (s *BgpServer) propagateUpdateToNeighbors(rib *table.TableManager, source * 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 + // If the path is filtered by policies, there is no need to send the path + // Otherwise, we send only paths that were previously filtered because of the max path limit p := s.filterpath(targetPeer, p, nil) if p == nil || !targetPeer.isPathSendMaxFiltered(p) { continue } + // We unset the flag as the path is not filtered anymore + targetPeer.unsetPathSendMaxFiltered(p) toAdd = append(toAdd, p) if len(toAdd) == len(toActuallyDelete) { break diff --git a/test/scenario_test/addpath_test.py b/test/scenario_test/addpath_test.py index 41e4c8e04..d8757c1d9 100644 --- a/test/scenario_test/addpath_test.py +++ b/test/scenario_test/addpath_test.py @@ -52,20 +52,12 @@ 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) - 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, - ) + 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] @@ -82,10 +74,7 @@ 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, - ) + g4.add_peer(g5, addpath=cls.SEND_MAX) g5.add_peer(g4, addpath=cls.SEND_MAX) cls.g1 = g1 @@ -202,8 +191,19 @@ def f(): assert_several_times(f) + def test_10_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) + + 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.SEND_MAX) + # expect the last path to not be filtered + self.assertFalse(adj_out[0]["paths"][-1].get("send-max-filtered", False)) + # test the best path is replaced due to the removal from g1 rib - def test_10_check_g2_global_rib(self): + def test_11_check_g2_global_rib(self): def f(): rib = self.g2.get_global_rib() self.assertEqual(len(rib), 1) @@ -214,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_11_check_g3_global_rib(self): + def test_12_check_g3_global_rib(self): def f(): rib = self.g3.get_global_rib() self.assertEqual(len(rib), 1) @@ -225,12 +225,12 @@ def f(): assert_several_times(f) # install a route with path_id via GoBGP CLI (no error check) - def test_12_install_add_paths_route_via_cli(self): + def test_13_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_13_check_g1_global_rib(self): + def test_14_check_g1_global_rib(self): def f(): rib = self.g1.get_global_rib() self.assertEqual(len(rib), 1) @@ -243,7 +243,7 @@ def f(): assert_several_times(f) - def test_14_check_g1_adj_out(self): + def test_15_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 +257,7 @@ def test_14_check_g1_adj_out(self): 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_15_check_g2_global_rib(self): + def test_16_check_g2_global_rib(self): def f(): rib = self.g2.get_global_rib() self.assertEqual(len(rib), 1) @@ -268,7 +268,7 @@ def f(): assert_several_times(f) # test the route from CLI is advertised from g1 - def test_16_check_g3_global_rib(self): + def test_17_check_g3_global_rib(self): def f(): rib = self.g3.get_global_rib() self.assertEqual(len(rib), 1) @@ -279,13 +279,13 @@ def f(): assert_several_times(f) # remove non-existing route with path_id via GoBGP CLI (no error check) - def test_17_remove_non_existing_add_paths_route_via_cli(self): + def test_18_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_18_check_g1_global_rib(self): + def test_19_check_g1_global_rib(self): def f(): rib = self.g1.get_global_rib() self.assertEqual(len(rib), 1) @@ -299,10 +299,10 @@ def f(): assert_several_times(f) # remove route with path_id via GoBGP CLI (no error check) - def test_19_remove_add_paths_route_via_cli(self): + def test_20_remove_add_paths_route_via_cli(self): self.g1.del_route(route='192.168.100.0/24', identifier=10) - def test_20_check_g1_adj_out(self): + def test_21_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) @@ -312,7 +312,7 @@ def test_20_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_21_check_g1_global_rib(self): + def test_22_check_g1_global_rib(self): def f(): rib = self.g1.get_global_rib() self.assertEqual(len(rib), 1) @@ -324,7 +324,7 @@ def f(): assert_several_times(f) # test the best path is replaced the removal from g1 rib - def test_22_check_g2_global_rib(self): + def test_23_check_g2_global_rib(self): def f(): rib = self.g2.get_global_rib() self.assertEqual(len(rib), 1) @@ -334,7 +334,7 @@ def f(): assert_several_times(f) # test the removed route from CLI is withdrawn by g1 - def test_23_check_g3_global_rib(self): + def test_24_check_g3_global_rib(self): def f(): rib = self.g3.get_global_rib() self.assertEqual(len(rib), 1)