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

9 files changed, 429 insertions, 12 deletions

diff --git a/data/templates/frr/ospfd.frr.j2 b/data/templates/frr/ospfd.frr.j2
index 2809d34a0..77bd901b3 100644
--- a/data/templates/frr/ospfd.frr.j2
+++ b/data/templates/frr/ospfd.frr.j2
@@ -211,11 +211,8 @@ router ospf {{ 'vrf ' ~ vrf if vrf is vyos_defined }}
 {% if parameters.abr_type is vyos_defined %}
  ospf abr-type {{ parameters.abr_type }}
 {% endif %}
-{% if parameters.opaque_lsa is vyos_defined %}
- ospf opaque-lsa
-{% endif %}
 {% if parameters.rfc1583_compatibility is vyos_defined %}
- ospf rfc1583compatibility
+ compatible rfc1583
 {% endif %}
 {% if parameters.router_id is vyos_defined %}
  ospf router-id {{ parameters.router_id }}
diff --git a/interface-definitions/include/ospf/protocol-common-config.xml.i b/interface-definitions/include/ospf/protocol-common-config.xml.i
index caef8abb4..3b8e5aedd 100644
--- a/interface-definitions/include/ospf/protocol-common-config.xml.i
+++ b/interface-definitions/include/ospf/protocol-common-config.xml.i
@@ -640,12 +640,6 @@
       </properties>
       <defaultValue>cisco</defaultValue>
     </leafNode>
-    <leafNode name="opaque-lsa">
-      <properties>
-        <help>Enable the Opaque-LSA capability (rfc2370)</help>
-        <valueless/>
-      </properties>
-    </leafNode>
     <leafNode name="rfc1583-compatibility">
       <properties>
         <help>Enable RFC1583 criteria for handling AS external routes</help>
diff --git a/interface-definitions/include/version/ospf-version.xml.i b/interface-definitions/include/version/ospf-version.xml.i
index df108837b..8ba074e81 100644
--- a/interface-definitions/include/version/ospf-version.xml.i
+++ b/interface-definitions/include/version/ospf-version.xml.i
@@ -1,3 +1,3 @@
 <!-- include start from include/version/ospf-version.xml.i -->
-<syntaxVersion component='ospf' version='2'></syntaxVersion>
+<syntaxVersion component='ospf' version='3'></syntaxVersion>
 <!-- include end -->
diff --git a/smoketest/configs/assert/vrf-ospf b/smoketest/configs/assert/vrf-ospf
index 1ee2a2bbc..d9c598654 100644
--- a/smoketest/configs/assert/vrf-ospf
+++ b/smoketest/configs/assert/vrf-ospf
@@ -5,6 +5,7 @@ set interfaces ethernet eth1 vrf 'red'
 set interfaces ethernet eth2 offload gro
 set interfaces ethernet eth2 vrf 'blue'
 set protocols ospf area 0 network '192.0.2.0/24'
+set protocols ospf capability opaque
 set protocols ospf interface eth0 authentication md5 key-id 10 md5-key 'ospfkey'
 set protocols ospf interface eth0 passive disable
 set protocols ospf log-adjacency-changes
diff --git a/smoketest/configs/vrf-ospf b/smoketest/configs/vrf-ospf
index 349b803d3..2363355c7 100644
--- a/smoketest/configs/vrf-ospf
+++ b/smoketest/configs/vrf-ospf
@@ -27,6 +27,7 @@ protocols {
         }
         parameters {
             abr-type cisco
+            opaque-lsa
             router-id 1.2.3.4
         }
         passive-interface default
@@ -128,6 +129,7 @@ vrf {
                 }
                 parameters {
                     abr-type cisco
+                    opaque-lsa
                     router-id 9.10.11.12
                 }
                 passive-interface default
diff --git a/smoketest/scripts/cli/test_protocols_ospf.py b/smoketest/scripts/cli/test_protocols_ospf.py
index 7879a7379..28568ecbb 100755
--- a/smoketest/scripts/cli/test_protocols_ospf.py
+++ b/smoketest/scripts/cli/test_protocols_ospf.py
@@ -85,7 +85,7 @@ class TestProtocolsOSPF(VyOSUnitTestSHIM.TestCase):
         self.cli_set(base_path + ['auto-cost', 'reference-bandwidth', bandwidth])
         self.cli_set(base_path + ['parameters', 'router-id', router_id])
         self.cli_set(base_path + ['parameters', 'abr-type', abr_type])
-        self.cli_set(base_path + ['parameters', 'opaque-lsa'])
+        self.cli_set(base_path + ['capability', 'opaque'])
         self.cli_set(base_path + ['parameters', 'rfc1583-compatibility'])
         self.cli_set(base_path + ['log-adjacency-changes', 'detail'])
         self.cli_set(base_path + ['default-metric', metric])
@@ -712,5 +712,109 @@ class TestProtocolsOSPF(VyOSUnitTestSHIM.TestCase):
         self.cli_commit()
         verify('')
 
+    def test_ospf_21_capability_opaque(self):
+        # "capability opaque" is the only CLI node for FRR's "capability opaque"
+        # (T9441: "parameters opaque-lsa" was migrated to it). The generated
+        # FRR config file must hold the line once, and no alias form of it.
+        #
+        # This reads the generated FRR config file: FRR accepts the alias
+        # "ospf opaque-lsa", so vtysh cannot tell which form was rendered.
+        frr_conf = '/run/frr/config/vyos.frr.conf'
+        capability = base_path + ['capability', 'opaque']
+
+        # the router-id keeps "router ospf" configured when the node is
+        # deleted below, so the delete is tested against a running instance
+        self.cli_set(base_path + ['parameters', 'router-id', '192.0.2.1'])
+        self.cli_set(capability)
+        self.cli_commit()
+
+        frrconfig = read_file(frr_conf)
+        self.assertEqual(frrconfig.count('\n capability opaque\n'), 1)
+        self.assertNotRegex(frrconfig, r'(?m)^ ospf opaque-lsa')
+        frrconfig = self.getFRRconfig('router ospf', stop_section='^exit')
+        self.assertIn(' capability opaque', frrconfig)
+
+        self.cli_delete(capability)
+        self.cli_commit()
+        self.assertNotIn('capability opaque', read_file(frr_conf))
+        frrconfig = self.getFRRconfig('router ospf', stop_section='^exit')
+        self.assertNotIn(' capability opaque', frrconfig)
+        self.assertIn(' ospf router-id 192.0.2.1', frrconfig)
+
+    def test_ospf_22_rfc1583_canonical_form(self):
+        # FRR prints "compatible rfc1583". The CLI option "parameters
+        # rfc1583-compatibility" must be rendered in this form: if the alias
+        # form "ospf rfc1583compatibility" is rendered, frr-reload finds the
+        # line missing on every reload, removes the setting and adds it again,
+        # which runs an extra SPF on each FRR reload, whatever it changed.
+        #
+        # This reads the generated FRR config file: FRR accepts both forms, so
+        # vtysh cannot tell which one we rendered.
+        frr_conf = '/run/frr/config/vyos.frr.conf'
+        alias_form = r'(?m)^ ospf rfc1583compatibility'
+        rfc1583 = base_path + ['parameters', 'rfc1583-compatibility']
+
+        # the router-id keeps "router ospf" configured when the node is
+        # deleted below, so the delete is tested against a running instance
+        self.cli_set(base_path + ['parameters', 'router-id', '192.0.2.1'])
+        self.cli_set(rfc1583)
+        self.cli_commit()
+
+        frrconfig = read_file(frr_conf)
+        self.assertEqual(frrconfig.count('\n compatible rfc1583\n'), 1)
+        self.assertNotRegex(frrconfig, alias_form)
+        frrconfig = self.getFRRconfig('router ospf', stop_section='^exit')
+        self.assertIn(' compatible rfc1583', frrconfig)
+
+        self.cli_delete(rfc1583)
+        self.cli_commit()
+        self.assertNotIn('rfc1583', read_file(frr_conf))
+        frrconfig = self.getFRRconfig('router ospf', stop_section='^exit')
+        self.assertNotIn(' compatible rfc1583', frrconfig)
+        self.assertIn(' ospf router-id 192.0.2.1', frrconfig)
+
+        # a VRF instance renders the same form, in the VRF block only
+        vrf = 'blue'
+        vrf_base = ['vrf', 'name', vrf]
+        vrf_header = f'router ospf vrf {vrf}'
+        self.cli_set(vrf_base + ['table', '1000'])
+        vrf_params = vrf_base + ['protocols', 'ospf', 'parameters']
+        self.cli_set(vrf_params + ['router-id', '1.1.1.1'])
+        self.cli_set(vrf_params + ['rfc1583-compatibility'])
+        self.cli_commit()
+
+        frrconfig = read_file(frr_conf)
+        self.assertNotRegex(frrconfig, alias_form)
+        _, _, vrf_block = frrconfig.partition(vrf_header)
+        self.assertIn(' compatible rfc1583', vrf_block)
+        frrconfig = self.getFRRconfig(vrf_header, stop_section='^exit')
+        self.assertIn(' compatible rfc1583', frrconfig)
+
+        self.cli_delete(vrf_base)
+
+    def test_ospf_23_vrf_capability_opaque(self):
+        # FRR refuses opaque LSAs outside the default VRF (T9441's migration
+        # drops a saved "capability opaque" there for the same reason); a new
+        # "set" must be rejected before FRR is reloaded, not left to FRR's
+        # own refusal
+        vrf = 'blue'
+        vrf_base = ['vrf', 'name', vrf]
+        vrf_ospf = vrf_base + ['protocols', 'ospf']
+        self.cli_set(vrf_base + ['table', '1000'])
+        self.cli_set(vrf_ospf + ['parameters', 'router-id', '1.1.1.1'])
+        self.cli_set(vrf_ospf + ['capability', 'opaque'])
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        self.cli_delete(vrf_ospf + ['capability'])
+        self.cli_commit()
+
+        vrf_header = f'router ospf vrf {vrf}'
+        frrconfig = self.getFRRconfig(vrf_header, stop_section='^exit')
+        self.assertIn(' ospf router-id 1.1.1.1', frrconfig)
+        self.assertNotIn('capability opaque', frrconfig)
+
+        self.cli_delete(vrf_base)
+
 if __name__ == '__main__':
     unittest.main(verbosity=2, failfast=VyOSUnitTestSHIM.TestCase.debug_on())
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']))