vrf: T5174: ensure no duplicate VNIs can be defined

5 files changed, 138 insertions, 15 deletions

diff --git a/data/templates/frr/zebra.vrf.route-map.frr.j2 b/data/templates/frr/zebra.vrf.route-map.frr.j2
index 3c5791c4c..4e1206374 100644
--- a/data/templates/frr/zebra.vrf.route-map.frr.j2
+++ b/data/templates/frr/zebra.vrf.route-map.frr.j2
@@ -1,6 +1,10 @@
 !
 {% if name is vyos_defined %}
 {%     for vrf, vrf_config in name.items() %}
+{#         code path required for vrf_vni.py as we will only render the required VR configuration and not all of them #}
+{%         if only_vrf is vyos_defined and vrf is not vyos_defined(only_vrf) %}
+{%             continue %}
+{%         endif %}
 vrf {{ vrf }}
 {%         if vrf_config.ip.protocol is vyos_defined %}
 {%             for protocol_name, protocol_config in vrf_config.ip.protocol.items() %}
@@ -15,7 +19,7 @@ vrf {{ vrf }}
  ipv6 protocol {{ protocol_name }} route-map {{ protocol_config.route_map }}
 {%             endfor %}
 {%         endif %}
-{%         if vrf_config.vni is vyos_defined %}
+{%         if vrf_config.vni is vyos_defined and no_vni is not vyos_defined %}
  vni {{ vrf_config.vni }}
 {%         endif %}
 exit-vrf
diff --git a/python/vyos/template.py b/python/vyos/template.py
index 06a292706..254a15e3a 100644
--- a/python/vyos/template.py
+++ b/python/vyos/template.py
@@ -44,6 +44,7 @@ def _get_environment(location=None):
         loader=loc_loader,
         trim_blocks=True,
         undefined=ChainableUndefined,
+        extensions=['jinja2.ext.loopcontrols']
     )
     env.filters.update(_FILTERS)
     env.tests.update(_TESTS)
diff --git a/smoketest/scripts/cli/test_vrf.py b/smoketest/scripts/cli/test_vrf.py
index 8016c0105..926616727 100755
--- a/smoketest/scripts/cli/test_vrf.py
+++ b/smoketest/scripts/cli/test_vrf.py
@@ -61,7 +61,8 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
             self.assertNotIn(vrf, interfaces())
 
     def test_vrf_vni_and_table_id(self):
-        table = '1000'
+        base_table = '1000'
+        table = base_table
         for vrf in vrfs:
             base = base_path + ['name', vrf]
             description = f'VyOS-VRF-{vrf}'
@@ -82,7 +83,7 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         self.cli_commit()
 
         # Verify VRF configuration
-        table = '1000'
+        table = base_table
         iproute2_config = read_file('/etc/iproute2/rt_tables.d/vyos-vrf.conf')
         for vrf in vrfs:
             description = f'VyOS-VRF-{vrf}'
@@ -196,7 +197,8 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
             self.cli_delete(['interfaces', section, interface, 'vrf'])
 
     def test_vrf_static_route(self):
-        table = '100'
+        base_table = '100'
+        table = base_table
         for vrf in vrfs:
             next_hop = f'192.0.{table}.1'
             prefix = f'10.0.{table}.0/24'
@@ -217,13 +219,12 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         self.cli_commit()
 
         # Verify VRF configuration
-        table = '100'
+        table = base_table
         for vrf in vrfs:
             next_hop = f'192.0.{table}.1'
             prefix = f'10.0.{table}.0/24'
 
             self.assertTrue(vrf in interfaces())
-            vrf_if = Interface(vrf)
 
             frrconfig = self.getFRRconfig(f'vrf {vrf}')
             self.assertIn(f' vni {table}', frrconfig)
@@ -369,5 +370,98 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
                 route_map = f'route-map-{vrf}-{protocol}'
                 self.assertIn(f' ipv6 protocol {protocol} route-map {route_map}', frrconfig)
 
+    def test_vrf_vni_duplicates(self):
+        base_table = '6300'
+        table = base_table
+        for vrf in vrfs:
+            base = base_path + ['name', vrf]
+            self.cli_set(base + ['table', str(table)])
+            self.cli_set(base + ['vni', '100'])
+            table = str(int(table) + 1)
+
+        # L3VNIs can only be used once
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        table = base_table
+        for vrf in vrfs:
+            base = base_path + ['name', vrf]
+            self.cli_set(base + ['vni', str(table)])
+            table = str(int(table) + 1)
+
+        # commit changes
+        self.cli_commit()
+
+        # Verify VRF configuration
+        table = base_table
+        for vrf in vrfs:
+            self.assertTrue(vrf in interfaces())
+
+            frrconfig = self.getFRRconfig(f'vrf {vrf}')
+            self.assertIn(f' vni {table}', frrconfig)
+            # Increment table ID for the next run
+            table = str(int(table) + 1)
+
+    def test_vrf_vni_add_change_remove(self):
+        base_table = '6300'
+        table = base_table
+        for vrf in vrfs:
+            base = base_path + ['name', vrf]
+            self.cli_set(base + ['table', str(table)])
+            self.cli_set(base + ['vni', str(table)])
+            table = str(int(table) + 1)
+
+        # commit changes
+        self.cli_commit()
+
+        # Verify VRF configuration
+        table = base_table
+        for vrf in vrfs:
+            self.assertTrue(vrf in interfaces())
+
+            frrconfig = self.getFRRconfig(f'vrf {vrf}')
+            self.assertIn(f' vni {table}', frrconfig)
+            # Increment table ID for the next run
+            table = str(int(table) + 1)
+
+        # Now change all L3VNIs (increment 2)
+        # We must also change the base_table number as we probably could get
+        # duplicate VNI's during the test as VNIs are applied 1:1 to FRR
+        base_table = '5000'
+        table = base_table
+        for vrf in vrfs:
+            base = base_path + ['name', vrf]
+            self.cli_set(base + ['vni', str(table)])
+            table = str(int(table) + 2)
+
+        # commit changes
+        self.cli_commit()
+
+        # Verify VRF configuration
+        table = base_table
+        for vrf in vrfs:
+            self.assertTrue(vrf in interfaces())
+
+            frrconfig = self.getFRRconfig(f'vrf {vrf}')
+            self.assertIn(f' vni {table}', frrconfig)
+            # Increment table ID for the next run
+            table = str(int(table) + 2)
+
+        # Now delete all the VNIs
+        for vrf in vrfs:
+            base = base_path + ['name', vrf]
+            self.cli_delete(base + ['vni'])
+
+        # commit changes
+        self.cli_commit()
+
+        # Verify no VNI is defined
+        for vrf in vrfs:
+            self.assertTrue(vrf in interfaces())
+
+            frrconfig = self.getFRRconfig(f'vrf {vrf}')
+            self.assertNotIn('vni', frrconfig)
+
+
 if __name__ == '__main__':
     unittest.main(verbosity=2)
diff --git a/src/conf_mode/vrf.py b/src/conf_mode/vrf.py
index a7ef4cb5c..0b983293e 100755
--- a/src/conf_mode/vrf.py
+++ b/src/conf_mode/vrf.py
@@ -108,6 +108,12 @@ def get_config(config=None):
     # vyos.configverify.verify_common_route_maps() for more information.
     tmp = {'policy' : {'route-map' : conf.get_config_dict(['policy', 'route-map'],
                                                           get_first_key=True)}}
+
+    # L3VNI setup is done via vrf_vni.py as it must be de-configured (on node
+    # deletetion prior to the BGP process. Tell the Jinja2 template no VNI
+    # setup is needed
+    vrf.update({'no_vni' : ''})
+
     # Merge policy dict into "regular" config dict
     vrf = dict_merge(tmp, vrf)
     return vrf
@@ -124,8 +130,8 @@ def verify(vrf):
                                   f'static routes installed!')
 
     if 'name' in vrf:
-        reserved_names = ["add", "all", "broadcast", "default", "delete", "dev", "get", "inet", "mtu", "link", "type",
-                          "vrf"]
+        reserved_names = ["add", "all", "broadcast", "default", "delete", "dev",
+                          "get", "inet", "mtu", "link", "type", "vrf"]
         table_ids = []
         for name, vrf_config in vrf['name'].items():
             # Reserved VRF names
@@ -142,8 +148,8 @@ def verify(vrf):
                 if tmp and tmp != vrf_config['table']:
                     raise ConfigError(f'VRF "{name}" table id modification not possible!')
 
-            # VRf routing table ID must be unique on the system
-            if vrf_config['table'] in table_ids:
+            # VRF routing table ID must be unique on the system
+            if 'table' in vrf_config and vrf_config['table'] in table_ids:
                 raise ConfigError(f'VRF "{name}" table id is not unique!')
             table_ids.append(vrf_config['table'])
 
diff --git a/src/conf_mode/vrf_vni.py b/src/conf_mode/vrf_vni.py
index 87d7798b8..a7424b517 100644
--- a/src/conf_mode/vrf_vni.py
+++ b/src/conf_mode/vrf_vni.py
@@ -19,6 +19,7 @@ from sys import exit
 
 from vyos.config import Config
 from vyos.template import render_to_string
+from vyos.util import dict_search
 from vyos import ConfigError
 from vyos import frr
 from vyos import airbag
@@ -36,11 +37,19 @@ def get_config(config=None):
     else:
         return None
 
-    base = ['vrf', 'name', vrf_name]
-    tmp = conf.get_config_dict(base, key_mangling=('-', '_'),
-                               no_tag_node_value_mangle=True, get_first_key=True)
+    # Using duplicate L3VNIs makes no sense - it's also forbidden in FRR,
+    # thus VyOS CLI must deny this, too. Instead of getting only the dict for
+    # the requested VRF and den comparing it with depenent VRfs to not have any
+    # duplicate we will just grad ALL VRFs by default but only render/apply
+    # the configuration for the requested VRF - that makes the code easier and
+    # hopefully less error prone
+    vrf = conf.get_config_dict(['vrf'], key_mangling=('-', '_'),
+                               no_tag_node_value_mangle=True,
+                               get_first_key=True)
+
+    # Store name of VRF we are interested in for FRR config rendering
+    vrf.update({'only_vrf' : vrf_name})
 
-    vrf = {'name': {vrf_name: tmp}}
     return vrf
 
 def verify(vrf):
@@ -50,6 +59,15 @@ def verify(vrf):
     if len(argv) < 2:
         raise ConfigError('VRF parameter not specified when valling vrf_vni.py')
 
+    if 'name' in vrf:
+        vni_ids = []
+        for name, vrf_config in vrf['name'].items():
+            # VRF VNI (Virtual Network Identifier) must be unique on the system
+            if 'vni' in vrf_config:
+                if vrf_config['vni'] in vni_ids:
+                    raise ConfigError(f'VRF "{name}" VNI is not unique!')
+                vni_ids.append(vrf_config['vni'])
+
     return None
 
 def generate(vrf):
@@ -67,7 +85,7 @@ def apply(vrf):
     frr_cfg.load_configuration(frr_daemon)
     # There is only one VRF inside the dict as we read only one in get_config()
     if vrf and 'name' in vrf:
-        vrf_name = next(iter(vrf['name']))
+        vrf_name = vrf['only_vrf']
         frr_cfg.modify_section(f'^vrf {vrf_name}', stop_pattern='^exit-vrf', remove_stop_mark=True)
     if vrf and 'new_frr_config' in vrf:
         frr_cfg.add_before(frr.default_add_before, vrf['new_frr_config'])