From 05ab8ceda47773d57e33d12d3ede7bab67ba01ac Mon Sep 17 00:00:00 2001 From: Alexander Chernavin Date: Tue, 4 Oct 2022 12:38:54 +0000 Subject: [PATCH] bgpd: fix "bgp max-med on-startup" Currently, if `bgp max-med on-startup` is configured, after BGP session is established for the first time, a timer for the specified time is started. When the timer is expired, an UPDATE message should be sent to reflect changes in the routes' MED value. The problem is that the routes are being suppressed because based on the attributes they look like they have not changed. However, in the case of max-med, the value is copied to the packet directly from `bgp->maxmed_value`, not from the attributes. Thus, changes in this case cannot be detected by comparing attributes. With this fix, avoid route suppressing when the `max-med on-startup` timer expires and initiates an UPDATE. Signed-off-by: Alexander Chernavin --- bgpd/bgp_updgrp_adv.c | 5 + .../bgp_max_med_on_startup/__init__.py | 0 .../bgp_max_med_on_startup/r1/bgpd.conf | 11 ++ .../bgp_max_med_on_startup/r1/zebra.conf | 9 ++ .../bgp_max_med_on_startup/r2/bgpd.conf | 7 ++ .../bgp_max_med_on_startup/r2/zebra.conf | 6 + .../test_bgp_max_med_on_startup.py | 114 ++++++++++++++++++ 7 files changed, 152 insertions(+) create mode 100644 tests/topotests/bgp_max_med_on_startup/__init__.py create mode 100644 tests/topotests/bgp_max_med_on_startup/r1/bgpd.conf create mode 100644 tests/topotests/bgp_max_med_on_startup/r1/zebra.conf create mode 100644 tests/topotests/bgp_max_med_on_startup/r2/bgpd.conf create mode 100644 tests/topotests/bgp_max_med_on_startup/r2/zebra.conf create mode 100644 tests/topotests/bgp_max_med_on_startup/test_bgp_max_med_on_startup.py diff --git a/bgpd/bgp_updgrp_adv.c b/bgpd/bgp_updgrp_adv.c index 27e3677702..72e70ebf9f 100644 --- a/bgpd/bgp_updgrp_adv.c +++ b/bgpd/bgp_updgrp_adv.c @@ -355,6 +355,11 @@ static int update_group_announce_walkcb(struct update_group *updgrp, void *arg) struct update_subgroup *subgrp; UPDGRP_FOREACH_SUBGRP (updgrp, subgrp) { + /* Avoid supressing duplicate routes later + * when processing in subgroup_announce_table(). + */ + SET_FLAG(subgrp->sflags, SUBGRP_STATUS_FORCE_UPDATES); + subgroup_announce_all(subgrp); } diff --git a/tests/topotests/bgp_max_med_on_startup/__init__.py b/tests/topotests/bgp_max_med_on_startup/__init__.py new file mode 100644 index 0000000000..e69de29bb2 diff --git a/tests/topotests/bgp_max_med_on_startup/r1/bgpd.conf b/tests/topotests/bgp_max_med_on_startup/r1/bgpd.conf new file mode 100644 index 0000000000..41bf96344a --- /dev/null +++ b/tests/topotests/bgp_max_med_on_startup/r1/bgpd.conf @@ -0,0 +1,11 @@ +! +router bgp 65001 + bgp max-med on-startup 5 777 + no bgp ebgp-requires-policy + neighbor 192.168.255.2 remote-as 65001 + neighbor 192.168.255.2 timers 3 10 + address-family ipv4 unicast + redistribute connected + exit-address-family + ! +! diff --git a/tests/topotests/bgp_max_med_on_startup/r1/zebra.conf b/tests/topotests/bgp_max_med_on_startup/r1/zebra.conf new file mode 100644 index 0000000000..7c2ed09b81 --- /dev/null +++ b/tests/topotests/bgp_max_med_on_startup/r1/zebra.conf @@ -0,0 +1,9 @@ +! +interface lo + ip address 172.16.255.254/32 +! +interface r1-eth0 + ip address 192.168.255.1/30 +! +ip forwarding +! diff --git a/tests/topotests/bgp_max_med_on_startup/r2/bgpd.conf b/tests/topotests/bgp_max_med_on_startup/r2/bgpd.conf new file mode 100644 index 0000000000..187713d089 --- /dev/null +++ b/tests/topotests/bgp_max_med_on_startup/r2/bgpd.conf @@ -0,0 +1,7 @@ +! +router bgp 65001 + no bgp ebgp-requires-policy + neighbor 192.168.255.1 remote-as 65001 + neighbor 192.168.255.1 timers 3 10 + ! +! diff --git a/tests/topotests/bgp_max_med_on_startup/r2/zebra.conf b/tests/topotests/bgp_max_med_on_startup/r2/zebra.conf new file mode 100644 index 0000000000..fd45c48d6d --- /dev/null +++ b/tests/topotests/bgp_max_med_on_startup/r2/zebra.conf @@ -0,0 +1,6 @@ +! +interface r2-eth0 + ip address 192.168.255.2/30 +! +ip forwarding +! diff --git a/tests/topotests/bgp_max_med_on_startup/test_bgp_max_med_on_startup.py b/tests/topotests/bgp_max_med_on_startup/test_bgp_max_med_on_startup.py new file mode 100644 index 0000000000..a83d43310b --- /dev/null +++ b/tests/topotests/bgp_max_med_on_startup/test_bgp_max_med_on_startup.py @@ -0,0 +1,114 @@ +#!/usr/bin/env python + +# +# test_bgp_max_med_on_startup.py +# +# Copyright (c) 2022 Rubicon Communications, LLC. +# +# Permission to use, copy, modify, and/or distribute this software +# for any purpose with or without fee is hereby granted, provided +# that the above copyright notice and this permission notice appear +# in all copies. +# +# THE SOFTWARE IS PROVIDED "AS IS" AND NETDEF DISCLAIMS ALL WARRANTIES +# WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF +# MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL NETDEF BE LIABLE FOR +# ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY +# DAMAGES WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, +# WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS +# ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE +# OF THIS SOFTWARE. +# + +""" +Test whether `bgp max-med on-startup (5-86400) [(0-4294967295)]` is working +correctly. +""" + +import os +import sys +import json +import pytest +import functools + +CWD = os.path.dirname(os.path.realpath(__file__)) +sys.path.append(os.path.join(CWD, "../")) + +# pylint: disable=C0413 +from lib import topotest +from lib.topogen import Topogen, TopoRouter, get_topogen + +pytestmark = [pytest.mark.bgpd] + + +def build_topo(tgen): + for routern in range(1, 3): + tgen.add_router("r{}".format(routern)) + + switch = tgen.add_switch("s1") + switch.add_link(tgen.gears["r1"]) + switch.add_link(tgen.gears["r2"]) + + +def setup_module(mod): + tgen = Topogen(build_topo, mod.__name__) + tgen.start_topology() + + router_list = tgen.routers() + + for i, (rname, router) in enumerate(router_list.items(), 1): + router.load_config( + TopoRouter.RD_ZEBRA, os.path.join(CWD, "{}/zebra.conf".format(rname)) + ) + router.load_config( + TopoRouter.RD_BGP, os.path.join(CWD, "{}/bgpd.conf".format(rname)) + ) + + tgen.start_router() + + +def teardown_module(mod): + tgen = get_topogen() + tgen.stop_topology() + + +def test_bgp_max_med_on_startup(): + tgen = get_topogen() + + if tgen.routers_have_failure(): + pytest.skip(tgen.errors) + + router1 = tgen.gears["r1"] + router2 = tgen.gears["r2"] + + def _bgp_converge(router): + output = json.loads(router.vtysh_cmd("show ip bgp neighbor 192.168.255.1 json")) + expected = {"192.168.255.1": {"bgpState": "Established"}} + return topotest.json_cmp(output, expected) + + def _bgp_has_routes(router, metric): + output = json.loads( + router.vtysh_cmd("show ip bgp neighbor 192.168.255.1 routes json") + ) + expected = {"routes": {"172.16.255.254/32": [{"metric": metric}]}} + return topotest.json_cmp(output, expected) + + # Check session is established + test_func = functools.partial(_bgp_converge, router2) + success, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + assert result is None, "Failed bgp convergence on r2" + + # Check metric has value of max-med + test_func = functools.partial(_bgp_has_routes, router2, 777) + success, result = topotest.run_and_expect(test_func, None, count=30, wait=0.5) + assert result is None, "r2 does not receive routes with metric 777" + + # Check that when the max-med timer expires, metric is updated + test_func = functools.partial(_bgp_has_routes, router2, 0) + success, result = topotest.run_and_expect(test_func, None, count=16, wait=0.5) + assert result is None, "r2 does not receive routes with metric 0" + + +if __name__ == "__main__": + args = ["-s"] + sys.argv[1:] + sys.exit(pytest.main(args))