From 39507a51f9ec0b22796d1ddb591812fc4c6ac903 Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Thu, 29 Oct 2026 15:22:24 +0100 Subject: static: T9482: handle a blackhole next to a next-hop of the same distance FRR staticd refuses a blackhole or reject route next to a next-hop or interface route of the same prefix and distance. VyOS accepted the combination, the commit failed later in frr-reload and on some FRR versions the config manager stalled. Add a disable node to blackhole and reject. Verification refuses an enabled blackhole or reject route next to a next-hop, interface or dhcp-interface route of the same distance, and blackhole together with reject, in routes, tables and VRFs. The same applies to the default route of a PPPoE interface and of a DHCP interface that has a gateway. A commit that changes only such an interface does not run the check, frr-reload refuses it and the commit is rolled back. A config migration sets disable on the route FRR did not apply, so existing configurations keep loading and route as before. --- src/migration-scripts/quagga/12-to-13 | 92 +++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) create mode 100644 src/migration-scripts/quagga/12-to-13 (limited to 'src/migration-scripts') diff --git a/src/migration-scripts/quagga/12-to-13 b/src/migration-scripts/quagga/12-to-13 new file mode 100644 index 000000000..15025c899 --- /dev/null +++ b/src/migration-scripts/quagga/12-to-13 @@ -0,0 +1,92 @@ +# Copyright VyOS maintainers and contributors +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License +# along with this library. If not, see . + +# T9482: +# FRR staticd refuses a blackhole or reject route next to a next-hop or +# interface route of the same prefix with the same distance. The route +# configured first is kept, VyOS renders the blackhole and reject route last, +# so it is the one that is dropped. Make this visible in the CLI by setting +# "disable" on that blackhole or reject route. +# A prefix with both a blackhole and a reject route rendered only the +# blackhole route, the reject route gets "disable" the same way. + +from vyos.configtree import ConfigTree + + +def distance(config: ConfigTree, path: list) -> int: + if config.exists(path + ['distance']): + return int(config.return_value(path + ['distance'])) + return 1 + + +def migrate_static(config: ConfigTree, base: list) -> None: + if not config.exists(base): + return + + sources = [base] + if config.exists(base + ['table']): + sources += [base + ['table', table] for table in config.list_nodes(base + ['table'])] + + for source in sources: + for route in ['route', 'route6']: + route_base = source + [route] + if not config.exists(route_base): + continue + for prefix in config.list_nodes(route_base): + prefix_base = route_base + [prefix] + # Both at once used to render only the blackhole route, the + # reject route was ignored. Routes in a table were not checked, + # so such a configuration can exist. + blackhole_base = prefix_base + ['blackhole'] + reject_base = prefix_base + ['reject'] + if ( + config.exists(blackhole_base) + and config.exists(reject_base) + and not config.exists(blackhole_base + ['disable']) + and not config.exists(reject_base + ['disable']) + ): + config.set(reject_base + ['disable']) + + for discard in ['blackhole', 'reject']: + discard_base = prefix_base + [discard] + if not config.exists(discard_base): + continue + if config.exists(discard_base + ['disable']): + continue + discard_distance = distance(config, discard_base) + + conflict = False + # dhcp-interface routes use the default distance + if discard_distance == 1 and config.exists(prefix_base + ['dhcp-interface']): + conflict = True + for node in ['interface', 'next-hop']: + node_base = prefix_base + [node] + if not config.exists(node_base): + continue + for name in config.list_nodes(node_base): + if config.exists(node_base + [name, 'disable']): + continue + if distance(config, node_base + [name]) == discard_distance: + conflict = True + if conflict: + config.set(discard_base + ['disable']) + + +def migrate(config: ConfigTree) -> None: + migrate_static(config, ['protocols', 'static']) + + if config.exists(['vrf', 'name']): + for vrf in config.list_nodes(['vrf', 'name']): + migrate_static(config, ['vrf', 'name', vrf, 'protocols', 'static']) -- cgit v1.2.3 From 39f31b58157d89edc077392d436c12a42ab1d726 Mon Sep 17 00:00:00 2001 From: Christian Breunig Date: Sun, 1 Nov 2026 18:50:48 +0000 Subject: static: T9482: check static routes only Verification also refused a blackhole or reject default route next to the default route of a PPPoE interface, or of a DHCP interface once its lease had a router. Those routes change without a commit of the static routes, by an interface-only commit or a lease event. The check missed those cases, depended on the lease, and was not covered by the migration, so a PPPoE configuration with a plain blackhole default route failed to load after the upgrade. Only conflicts between static routes are refused now, the renderer handles the interface default routes. The default distance is looked up only for a route without a distance. The migration comment claimed FRR keeps the route configured first. A conflict makes frr-reload lose every static route of that reload. A unit test covers the migration, and the config test asserts which route was disabled. --- smoketest/configs/assert/static-route-basic | 6 ++ smoketest/scripts/cli/test_protocols_static.py | 114 ----------------------- src/conf_mode/protocols_static.py | 49 ++-------- src/migration-scripts/quagga/12-to-13 | 8 +- src/tests/test_static_migration.py | 122 +++++++++++++++++++++++++ 5 files changed, 140 insertions(+), 159 deletions(-) create mode 100644 src/tests/test_static_migration.py (limited to 'src/migration-scripts') diff --git a/smoketest/configs/assert/static-route-basic b/smoketest/configs/assert/static-route-basic index af92e7382..163586e74 100644 --- a/smoketest/configs/assert/static-route-basic +++ b/smoketest/configs/assert/static-route-basic @@ -13,6 +13,8 @@ set protocols static route 10.0.0.0/8 next-hop 192.0.2.140 distance '123' set protocols static route 10.0.0.0/8 next-hop 192.0.2.140 interface 'eth0' set protocols static route 172.16.0.0/16 next-hop 172.18.203.254 bfd multi-hop source-address '172.18.203.254' set protocols static route 172.16.0.0/16 next-hop 172.18.203.254 bfd profile 'foo' +set protocols static route 192.0.2.0/24 blackhole disable +set protocols static route 192.0.2.0/24 next-hop 198.51.100.1 set protocols static route6 2001:db8:1::/48 next-hop fe80::1 bfd multi-hop source-address 'fe80::1' set protocols static route6 2001:db8:1::/48 next-hop fe80::1 bfd profile 'bar' set protocols static route6 2001:db8:1::/48 next-hop fe80::1 interface 'eth0.203' @@ -21,6 +23,10 @@ set protocols static route6 2001:db8:2::/48 next-hop fe80::1 bfd profile 'bar' set protocols static route6 2001:db8:2::/48 next-hop fe80::1 interface 'eth0.203' set protocols static route6 2001:db8:3::/48 next-hop fe80::1 bfd set protocols static route6 2001:db8:3::/48 next-hop fe80::1 interface 'eth0.203' +set protocols static route6 2001:db8:4::/48 next-hop fe80::1 interface 'eth0.203' +set protocols static route6 2001:db8:4::/48 reject disable +set protocols static table 100 route 203.0.113.0/24 blackhole +set protocols static table 100 route 203.0.113.0/24 reject disable set service lldp interface all set service ntp allow-client address '0.0.0.0/0' set service ntp allow-client address '::/0' diff --git a/smoketest/scripts/cli/test_protocols_static.py b/smoketest/scripts/cli/test_protocols_static.py index 472437adf..1611a4757 100755 --- a/smoketest/scripts/cli/test_protocols_static.py +++ b/smoketest/scripts/cli/test_protocols_static.py @@ -192,8 +192,6 @@ class TestProtocolsStatic(VyOSUnitTestSHIM.TestCase): def tearDown(self): self.cli_delete(base_path) self.cli_delete(['vrf']) - # left over when test_10 fails halfway - self.cli_delete(['interfaces', 'pppoe', 'pppoe10']) self.cli_commit() v4route = self.getFRRconfig('ip route') @@ -919,118 +917,6 @@ class TestProtocolsStatic(VyOSUnitTestSHIM.TestCase): with self.assertRaisesRegex(ConfigSessionError, 'both blackhole and reject'): self.cli_commit() - def test_10_discard_default_route_next_to_pppoe_default_route(self): - if not get_interface_config('eth0'): - self.skipTest('eth0 is not available on this test harness') - - # The default route of a PPPoE interface is rendered with the distance - # of the interface, 1 unless configured (210 is its route tag) - interface_path = ['interfaces', 'pppoe', 'pppoe10'] - distance = default_value(interface_path + ['default-route-distance']) - self.cli_set(interface_path + ['source-interface', 'eth0']) - self.cli_set(interface_path + ['authentication', 'username', 'vyos']) - self.cli_set(interface_path + ['authentication', 'password', 'vyos']) - self.cli_commit() - - path = base_path + ['route', '0.0.0.0/0'] - for discard in ['blackhole', 'reject']: - self.cli_set(path + [discard, 'distance', distance]) - with self.assertRaisesRegex( - ConfigSessionError, 'PPPoE interface "pppoe10"' - ): - self.cli_commit() - - # another distance is fine, so is a disabled route - self.cli_set(path + [discard, 'distance', str(int(distance) + 1)]) - self.cli_commit() - self.cli_set(path + [discard, 'distance', distance]) - self.cli_set(path + [discard, 'disable']) - self.cli_commit() - self.cli_delete(path) - - # IPv6 is only checked when IPv6 is enabled on the interface - path = base_path + ['route6', '::/0'] - self.cli_set(path + ['blackhole', 'distance', distance]) - self.cli_commit() - self.cli_delete(path) - self.cli_set(interface_path + ['ipv6', 'address', 'autoconf']) - self.cli_commit() - self.cli_set(path + ['blackhole', 'distance', distance]) - with self.assertRaisesRegex(ConfigSessionError, 'PPPoE interface "pppoe10"'): - self.cli_commit() - self.cli_delete(path) - - # an interface without a default route does not conflict - path = base_path + ['route', '0.0.0.0/0'] - self.cli_set(path + ['blackhole', 'distance', distance]) - self.cli_set(interface_path + ['no-default-route']) - self.cli_commit() - - self.cli_delete(base_path) - self.cli_delete(interface_path) - self.cli_commit() - - def test_11_discard_default_route_next_to_dhcp_default_route(self): - # needs the DHCP server of the QEMU environment, see test_05 - if not self.running_in_smoketest_harness(): - self.skipTest('Not running under VyOS CI/CD QEMU environment!') - - interface = 'eth0' - interface_path = ['interfaces', 'ethernet', interface] - distance = default_value( - interface_path + ['dhcp-options', 'default-route-distance'] - ) - self.cli_set(interface_path + ['address', 'dhcp']) - self.cli_commit() - - # the conflict is only known once the lease has a router - self.wait_for_dhcp_router(interface) - - path = base_path + ['route', '0.0.0.0/0', 'blackhole'] - self.cli_set(path + ['distance', distance]) - with self.assertRaisesRegex( - ConfigSessionError, f'DHCP interface "{interface}"' - ): - self.cli_commit() - - self.cli_set(path + ['distance', str(int(distance) + 1)]) - self.cli_commit() - - self.cli_delete(base_path) - self.cli_delete(interface_path + ['address']) - self.cli_commit() - - while process_named_running('dhclient', cmdline=interface, timeout=10): - sleep(0.250) - - def test_12_pppoe_distance_changed_alone_conflicts_with_discard_route(self): - # A commit that only changes the interface does not run the check of - # the static routes. frr-reload refuses the conflict and the commit is - # rolled back, the configuration stays as it was. - if not get_interface_config('eth0'): - self.skipTest('eth0 is not available on this test harness') - - interface_path = ['interfaces', 'pppoe', 'pppoe10'] - self.cli_set(interface_path + ['source-interface', 'eth0']) - self.cli_set(interface_path + ['authentication', 'username', 'vyos']) - self.cli_set(interface_path + ['authentication', 'password', 'vyos']) - self.cli_set(interface_path + ['default-route-distance', '216']) - self.cli_set(base_path + ['route', '0.0.0.0/0', 'blackhole', 'distance', '215']) - self.cli_commit() - - self.cli_set(interface_path + ['default-route-distance', '215']) - with self.assertRaises(ConfigSessionError): - self.cli_commit() - self.cli_discard() - - frrconfig = self.getFRRconfig('ip route 0.0.0.0/0', end_marker='') - self.assertIn('ip route 0.0.0.0/0 pppoe10 tag 210 216', frrconfig) - self.assertIn('ip route 0.0.0.0/0 blackhole 215', frrconfig) - - self.cli_delete(base_path) - self.cli_delete(interface_path) - self.cli_commit() - if __name__ == '__main__': unittest.main(verbosity=2, failfast=VyOSUnitTestSHIM.TestCase.debug_on()) diff --git a/src/conf_mode/protocols_static.py b/src/conf_mode/protocols_static.py index 0c6826a1c..90dcab21a 100755 --- a/src/conf_mode/protocols_static.py +++ b/src/conf_mode/protocols_static.py @@ -28,7 +28,6 @@ from vyos.frrender import get_frrender_dict from vyos.utils.dict import dict_search from vyos.xml_ref import default_value from vyos.utils.file import write_file -from vyos.template import get_dhcp_router from vyos.template import render from vyos import ConfigError from vyos import airbag @@ -80,38 +79,19 @@ def verify(config_dict): # FRR staticd refuses a blackhole or reject route and a next-hop or # interface route for the same prefix with the same distance, which only # shows up when frr-reload fails after the commit. Routes in a table are - # checked as well, blackhole and reject at once is refused there too. + # checked as well, blackhole and reject at once is refused there too. The + # default routes of DHCP and PPPoE interfaces change without a commit of + # this node, FRRender drops a blackhole or reject route next to them. def route_distance(base, route, prefix, node, config, name=None): + # Compare the numbers, the validator accepts leading zeros + if 'distance' in config: + return int(config['distance']) # the default distance comes from the CLI definition, a tag node needs - # a value in the path. Compare the numbers, the validator accepts - # leading zeros. + # a value in the path path = base + [route, prefix, node] if name: path.append(name) - distance = config.get('distance', default_value(path + ['distance'])) - return int(distance) - - def generated_defaults(route, prefix): - # same conditions and distances as the default routes in staticd.frr.j2 - if route == 'route' and prefix == '0.0.0.0/0': - for name, config in static.get('dhcp', {}).items(): - # the template renders the route only when a gateway is known, - # an existing configuration that conflicts before that keeps - # loading - if get_dhcp_router(name) is None: - continue - if 'no_default_route' not in config.get('dhcp_options', {}): - distance = dict_search( - 'dhcp_options.default_route_distance', config - ) - yield 'DHCP', name, int(distance or 1) - if (route, prefix) in [('route', '0.0.0.0/0'), ('route6', '::/0')]: - for name, config in static.get('pppoe', {}).items(): - if 'no_default_route' in config: - continue - if route == 'route6' and 'ipv6' not in config: - continue - yield 'PPPoE', name, int(config.get('default_route_distance') or 1) + return int(default_value(path + ['distance'])) # the CLI path of the routes, a table or a VRF changes it static_base = ['protocols', 'static'] @@ -158,19 +138,6 @@ def verify(config_dict): f'for prefix "{prefix}"!' ) - # The default routes of DHCP and PPPoE interfaces are - # rendered into the same prefix. A commit that changes only - # such an interface does not run this check, frr-reload - # refuses it and the commit is rolled back. - if base == static_base: - for kind, name, distance in generated_defaults(route, prefix): - if distance == discard_distance: - raise ConfigError( - f'Cannot use {discard} and the default route ' - f'of {kind} interface "{name}" with the same ' - f'distance {distance} for prefix "{prefix}"!' - ) - # A dhcp-interface route has no distance option, it is # rendered like a next-hop route without a distance as soon # as the interface has a gateway diff --git a/src/migration-scripts/quagga/12-to-13 b/src/migration-scripts/quagga/12-to-13 index 15025c899..3d9a1df37 100644 --- a/src/migration-scripts/quagga/12-to-13 +++ b/src/migration-scripts/quagga/12-to-13 @@ -15,10 +15,10 @@ # T9482: # FRR staticd refuses a blackhole or reject route next to a next-hop or -# interface route of the same prefix with the same distance. The route -# configured first is kept, VyOS renders the blackhole and reject route last, -# so it is the one that is dropped. Make this visible in the CLI by setting -# "disable" on that blackhole or reject route. +# interface route of the same prefix with the same distance. frr-reload adds +# all new lines in one transaction, so on boot such a configuration lost +# every static route. Set "disable" on that blackhole or reject route, the +# next-hop or interface route is kept. # A prefix with both a blackhole and a reject route rendered only the # blackhole route, the reject route gets "disable" the same way. diff --git a/src/tests/test_static_migration.py b/src/tests/test_static_migration.py new file mode 100644 index 000000000..7b974394a --- /dev/null +++ b/src/tests/test_static_migration.py @@ -0,0 +1,122 @@ +# Copyright VyOS maintainers and contributors +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License +# along with this library. If not, see . + +import importlib.machinery +import importlib.util +from unittest import TestCase + +from vyos.configtree import ConfigTree + +migration_script = 'src/migration-scripts/quagga/12-to-13' + +static = ['protocols', 'static'] +vrf_static = ['vrf', 'name', 'red', 'protocols', 'static'] +default = static + ['route', '0.0.0.0/0'] +prefix = static + ['route', '192.0.2.0/24'] + + +def load_migrate(): + loader = importlib.machinery.SourceFileLoader('quagga_12_to_13', migration_script) + spec = importlib.util.spec_from_loader(loader.name, loader) + module = importlib.util.module_from_spec(spec) + loader.exec_module(module) + return module.migrate + + +class TestStaticDiscardMigration(TestCase): + def setUp(self): + self.migrate = load_migrate() + self.config = ConfigTree('system {\n host-name vyos\n}\n') + + def set(self, path, value=None): + self.config.set(path, value=value, replace=False) + + def disabled(self, path): + return self.config.exists(path + ['disable']) + + def test_no_static(self): + # must not raise when static routing is not configured + self.migrate(self.config) + self.assertFalse(self.config.exists(static)) + + def test_next_hop_same_distance(self): + self.set(prefix + ['blackhole']) + self.set(prefix + ['next-hop', '198.51.100.1']) + self.migrate(self.config) + self.assertTrue(self.disabled(prefix + ['blackhole'])) + self.assertFalse(self.disabled(prefix + ['next-hop', '198.51.100.1'])) + + def test_interface_explicit_distance(self): + # the validator accepts leading zeros + self.set(prefix + ['reject', 'distance'], '05') + self.set(prefix + ['interface', 'eth0', 'distance'], '5') + self.migrate(self.config) + self.assertTrue(self.disabled(prefix + ['reject'])) + + def test_different_distance(self): + self.set(prefix + ['blackhole', 'distance'], '200') + self.set(prefix + ['next-hop', '198.51.100.1']) + self.migrate(self.config) + self.assertFalse(self.disabled(prefix + ['blackhole'])) + + def test_disabled_next_hop(self): + self.set(prefix + ['blackhole']) + self.set(prefix + ['next-hop', '198.51.100.1', 'disable']) + self.migrate(self.config) + self.assertFalse(self.disabled(prefix + ['blackhole'])) + + def test_dhcp_interface(self): + # rendered without a distance, so only the default distance conflicts + self.set(prefix + ['blackhole']) + self.set(prefix + ['dhcp-interface'], 'eth0') + other = static + ['route', '198.51.100.0/24'] + self.set(other + ['blackhole', 'distance'], '2') + self.set(other + ['dhcp-interface'], 'eth0') + self.migrate(self.config) + self.assertTrue(self.disabled(prefix + ['blackhole'])) + self.assertFalse(self.disabled(other + ['blackhole'])) + + def test_blackhole_and_reject(self): + # only the blackhole route was rendered + path = static + ['table', '100', 'route', '192.0.2.0/24'] + self.set(path + ['blackhole']) + self.set(path + ['reject', 'distance'], '250') + self.migrate(self.config) + self.assertFalse(self.disabled(path + ['blackhole'])) + self.assertTrue(self.disabled(path + ['reject'])) + + def test_blackhole_and_reject_next_to_next_hop(self): + self.set(prefix + ['blackhole']) + self.set(prefix + ['reject']) + self.set(prefix + ['next-hop', '198.51.100.1']) + self.migrate(self.config) + self.assertTrue(self.disabled(prefix + ['blackhole'])) + self.assertTrue(self.disabled(prefix + ['reject'])) + + def test_vrf_next_hop(self): + path = vrf_static + ['route6', '2001:db8::/32'] + self.set(['vrf', 'name', 'red', 'table'], '1000') + self.set(path + ['reject']) + self.set(path + ['next-hop', '2001:db8::1']) + self.migrate(self.config) + self.assertTrue(self.disabled(path + ['reject'])) + + def test_interface_default_route_ignored(self): + # the default route of a PPPoE or DHCP interface is not a static + # route, FRRender drops a blackhole route next to it + self.set(['interfaces', 'pppoe', 'pppoe0', 'source-interface'], 'eth0') + self.set(default + ['blackhole']) + self.migrate(self.config) + self.assertFalse(self.disabled(default + ['blackhole'])) -- cgit v1.2.3