Merge pull request #5543 from alexk37/ospf-opaque-canonical

ospf: T9441: drop "parameters opaque-lsa" (migrated to "capability opaque") and render "compatible rfc1583" as FRR prints it

3 files changed, 319 insertions, 0 deletions

diff --git a/src/conf_mode/protocols_ospf.py b/src/conf_mode/protocols_ospf.py
index 47e1596a6..0f73a6c8a 100755
--- a/src/conf_mode/protocols_ospf.py
+++ b/src/conf_mode/protocols_ospf.py
@@ -52,6 +52,13 @@ def verify(config_dict):
                                  config_dict) or config_dict['ospf']
     ospf['policy'] = config_dict['policy']
 
+    # FRR supports opaque LSAs only in the default VRF
+    if vrf and 'opaque' in ospf.get('capability', {}):
+        raise ConfigError(
+            'OSPF "capability opaque" is only supported in the '
+            f'default VRF, not in VRF "{vrf}"!'
+        )
+
     verify_common_route_maps(ospf)
 
     # As we can have a default-information route-map, we need to validate it!
diff --git a/src/migration-scripts/ospf/2-to-3 b/src/migration-scripts/ospf/2-to-3
new file mode 100644
index 000000000..271dbd58e
--- /dev/null
+++ b/src/migration-scripts/ospf/2-to-3
@@ -0,0 +1,54 @@
+# Copyright VyOS maintainers and contributors <maintainers@vyos.io>
+#
+# 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 <http://www.gnu.org/licenses/>.
+
+# T9441: "parameters opaque-lsa" and "capability opaque" are two CLI nodes for
+#        the same FRR setting. Keep "capability opaque" only. FRR supports
+#        opaque LSAs only in the default VRF and refuses the setting in a VRF,
+#        so in VRF instances it never took effect: drop it there.
+
+from vyos.configtree import ConfigTree
+
+
+def opaque_lsa_migration(config: ConfigTree, ospf_base: list, vrf: bool) -> None:
+    parameters = ospf_base + ['parameters']
+    if config.exists(parameters + ['opaque-lsa']):
+        config.delete(parameters + ['opaque-lsa'])
+
+        # do not leave an empty "parameters" node behind
+        if config.list_nodes(parameters) == []:
+            config.delete(parameters)
+
+        if not vrf:
+            config.set(ospf_base + ['capability', 'opaque'])
+
+    # FRR refuses "capability opaque" outside the default VRF; drop a value
+    # that would otherwise fail every commit (it never took effect anyway)
+    capability = ospf_base + ['capability']
+    if vrf and config.exists(capability + ['opaque']):
+        config.delete(capability + ['opaque'])
+        if config.list_nodes(capability) == []:
+            config.delete(capability)
+
+
+def migrate(config: ConfigTree) -> None:
+    # OSPF in the default VRF
+    opaque_lsa_migration(config, ['protocols', 'ospf'], vrf=False)
+
+    # OSPF in every VRF
+    if config.exists(['vrf', 'name']):
+        for name in config.list_nodes(['vrf', 'name']):
+            opaque_lsa_migration(
+                config, ['vrf', 'name', name, 'protocols', 'ospf'], vrf=True
+            )
diff --git a/src/tests/test_ospf_migration.py b/src/tests/test_ospf_migration.py
new file mode 100644
index 000000000..95a73cbe9
--- /dev/null
+++ b/src/tests/test_ospf_migration.py
@@ -0,0 +1,258 @@
+# Copyright VyOS maintainers and contributors <maintainers@vyos.io>
+#
+# 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 <http://www.gnu.org/licenses/>.
+
+import importlib.machinery
+import importlib.util
+from unittest import TestCase
+
+from vyos.configtree import ConfigTree
+
+migration_script = 'src/migration-scripts/ospf/2-to-3'
+
+ospf = ['protocols', 'ospf']
+vrf_ospf = ['vrf', 'name', 'red', 'protocols', 'ospf']
+
+
+def load_migrate():
+    loader = importlib.machinery.SourceFileLoader('ospf_2_to_3', 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
+
+
+def opaque_lsa(base):
+    return base + ['parameters', 'opaque-lsa']
+
+
+def capability_opaque(base):
+    return base + ['capability', 'opaque']
+
+
+class TestOspfOpaqueLsaMigration(TestCase):
+    def setUp(self):
+        self.migrate = load_migrate()
+
+    def test_no_ospf(self):
+        # must not raise when OSPF is not configured
+        config = ConfigTree('system {\n    host-name vyos\n}\n')
+        self.migrate(config)
+        self.assertFalse(config.exists(ospf))
+        self.assertFalse(config.exists(['vrf']))
+
+    def test_neither_node(self):
+        config = ConfigTree(
+            'protocols {\n'
+            '    ospf {\n'
+            '        parameters {\n'
+            '            router-id 192.0.2.1\n'
+            '        }\n'
+            '    }\n'
+            '}\n'
+        )
+        self.migrate(config)
+        self.assertFalse(config.exists(capability_opaque(ospf)))
+        self.assertEqual(
+            config.return_value(ospf + ['parameters', 'router-id']), '192.0.2.1'
+        )
+
+    def test_global_instance(self):
+        config = ConfigTree(
+            'protocols {\n'
+            '    ospf {\n'
+            '        parameters {\n'
+            '            opaque-lsa\n'
+            '            router-id 192.0.2.1\n'
+            '        }\n'
+            '    }\n'
+            '}\n'
+        )
+        self.migrate(config)
+        self.assertTrue(config.exists(capability_opaque(ospf)))
+        self.assertFalse(config.exists(opaque_lsa(ospf)))
+        # the other parameters stay
+        self.assertEqual(
+            config.return_value(ospf + ['parameters', 'router-id']), '192.0.2.1'
+        )
+
+    def test_empty_parameters_node_is_removed(self):
+        config = ConfigTree(
+            'protocols {\n'
+            '    ospf {\n'
+            '        parameters {\n'
+            '            opaque-lsa\n'
+            '        }\n'
+            '    }\n'
+            '}\n'
+        )
+        self.migrate(config)
+        self.assertTrue(config.exists(capability_opaque(ospf)))
+        self.assertFalse(config.exists(ospf + ['parameters']))
+
+    def test_vrf_instance(self):
+        # FRR refuses opaque LSAs outside the default VRF: the option never
+        # took effect there, so it is dropped, not turned into "capability
+        # opaque" (which FRR would refuse at commit time)
+        config = ConfigTree(
+            'vrf {\n'
+            '    name red {\n'
+            '        protocols {\n'
+            '            ospf {\n'
+            '                parameters {\n'
+            '                    opaque-lsa\n'
+            '                    router-id 192.0.2.2\n'
+            '                }\n'
+            '            }\n'
+            '        }\n'
+            '        table 1000\n'
+            '    }\n'
+            '}\n'
+        )
+        self.migrate(config)
+        self.assertFalse(config.exists(capability_opaque(vrf_ospf)))
+        self.assertFalse(config.exists(opaque_lsa(vrf_ospf)))
+        self.assertEqual(
+            config.return_value(vrf_ospf + ['parameters', 'router-id']), '192.0.2.2'
+        )
+        # nothing is created in the default VRF
+        self.assertFalse(config.exists(ospf))
+
+    def test_vrf_capability_opaque_dropped(self):
+        # a VRF instance that already has "capability opaque" set directly
+        # (without "parameters opaque-lsa") is just as refused by FRR; drop it
+        config = ConfigTree(
+            'vrf {\n'
+            '    name red {\n'
+            '        protocols {\n'
+            '            ospf {\n'
+            '                capability {\n'
+            '                    opaque\n'
+            '                }\n'
+            '                parameters {\n'
+            '                    router-id 192.0.2.2\n'
+            '                }\n'
+            '            }\n'
+            '        }\n'
+            '        table 1000\n'
+            '    }\n'
+            '}\n'
+        )
+        self.migrate(config)
+        self.assertFalse(config.exists(capability_opaque(vrf_ospf)))
+        self.assertFalse(config.exists(vrf_ospf + ['capability']))
+        self.assertEqual(
+            config.return_value(vrf_ospf + ['parameters', 'router-id']), '192.0.2.2'
+        )
+
+    def test_both_nodes_set_in_vrf(self):
+        # both the dead "parameters opaque-lsa" deletion and the "capability
+        # opaque" drop fire for the same VRF instance in one pass
+        config = ConfigTree(
+            'vrf {\n'
+            '    name red {\n'
+            '        protocols {\n'
+            '            ospf {\n'
+            '                capability {\n'
+            '                    opaque\n'
+            '                }\n'
+            '                parameters {\n'
+            '                    opaque-lsa\n'
+            '                }\n'
+            '            }\n'
+            '        }\n'
+            '        table 1000\n'
+            '    }\n'
+            '}\n'
+        )
+        self.migrate(config)
+        self.assertFalse(config.exists(capability_opaque(vrf_ospf)))
+        self.assertFalse(config.exists(opaque_lsa(vrf_ospf)))
+        self.assertFalse(config.exists(vrf_ospf + ['capability']))
+        self.assertFalse(config.exists(vrf_ospf + ['parameters']))
+
+    def test_global_and_vrf_instances_are_independent(self):
+        # the default instance keeps the move to "capability opaque"; VRF
+        # "red" only loses the dead option; VRF "blue" has neither and must
+        # stay as it is
+        config = ConfigTree(
+            'protocols {\n'
+            '    ospf {\n'
+            '        parameters {\n'
+            '            opaque-lsa\n'
+            '        }\n'
+            '    }\n'
+            '}\n'
+            'vrf {\n'
+            '    name blue {\n'
+            '        protocols {\n'
+            '            ospf {\n'
+            '                parameters {\n'
+            '                    router-id 192.0.2.3\n'
+            '                }\n'
+            '            }\n'
+            '        }\n'
+            '    }\n'
+            '    name red {\n'
+            '        protocols {\n'
+            '            ospf {\n'
+            '                parameters {\n'
+            '                    opaque-lsa\n'
+            '                }\n'
+            '            }\n'
+            '        }\n'
+            '    }\n'
+            '}\n'
+        )
+        self.migrate(config)
+        blue = ['vrf', 'name', 'blue', 'protocols', 'ospf']
+        # capability opaque exists only in the default instance
+        self.assertTrue(config.exists(capability_opaque(ospf)))
+        self.assertFalse(config.exists(capability_opaque(vrf_ospf)))
+        self.assertFalse(config.exists(opaque_lsa(vrf_ospf)))
+        self.assertFalse(config.exists(capability_opaque(blue)))
+        self.assertEqual(
+            config.return_value(blue + ['parameters', 'router-id']), '192.0.2.3'
+        )
+
+    def test_both_nodes_set(self):
+        config = ConfigTree(
+            'protocols {\n'
+            '    ospf {\n'
+            '        capability {\n'
+            '            opaque\n'
+            '        }\n'
+            '        parameters {\n'
+            '            opaque-lsa\n'
+            '        }\n'
+            '    }\n'
+            '}\n'
+        )
+        self.migrate(config)
+        self.assertTrue(config.exists(capability_opaque(ospf)))
+        self.assertFalse(config.exists(opaque_lsa(ospf)))
+
+    def test_only_capability_set(self):
+        config = ConfigTree(
+            'protocols {\n'
+            '    ospf {\n'
+            '        capability {\n'
+            '            opaque\n'
+            '        }\n'
+            '    }\n'
+            '}\n'
+        )
+        self.migrate(config)
+        self.assertTrue(config.exists(capability_opaque(ospf)))
+        self.assertFalse(config.exists(ospf + ['parameters']))