Skip to content

Commit

Permalink
send-max: fix flag when path is not filtered anymore
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@hardeker
hardeker committed Jul 29, 2024
1 parent 86cb2b5 commit da44c7f
Show file tree
Hide file tree
Showing 3 changed files with 39 additions and 35 deletions.
5 changes: 3 additions & 2 deletions cmd/gobgp/neighbor.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
5 changes: 4 additions & 1 deletion pkg/server/server.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
64 changes: 32 additions & 32 deletions test/scenario_test/addpath_test.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand All @@ -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)
Expand Down

0 comments on commit da44c7f

Please sign in to comment.