bgp: T6189: L3VPN connectivity is broken after re-enabling VRF

data/configd-include.json

@@ -108,6 +108,5 @@
 "vpn_openconnect.py",
 "vpn_pptp.py",
 "vpn_sstp.py",
-"vrf.py",
-"vrf_vni.py"
+"vrf.py"
 ]

data/templates/frr/zebra.vrf.route-map.frr.j2

@@ -1,10 +1,6 @@
 !
 {% 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.nht.no_resolve_via_default is vyos_defined %}
  no ip nht resolve-via-default
@@ -25,7 +21,7 @@ vrf {{ vrf }}
  ipv6 protocol {{ protocol_name }} route-map {{ protocol_config.route_map }}
 {%             endfor %}
 {%         endif %}
-{%         if vrf_config.vni is vyos_defined and no_vni is not vyos_defined %}
+{%         if vrf_config.vni is vyos_defined %}
  vni {{ vrf_config.vni }}
 {%         endif %}
 exit-vrf

interface-definitions/vrf.xml.in

@@ -120,20 +120,7 @@
               <constraintErrorMessage>VRF routing table must be in range from 100 to 65535</constraintErrorMessage>
             </properties>
           </leafNode>
-          <leafNode name="vni" owner="${vyos_conf_scripts_dir}/vrf_vni.py $VAR(../@)">
-            <properties>
-              <help>Virtual Network Identifier</help>
-              <!-- must be after BGP to keep correct order when removing L3VNIs in FRR -->
-              <priority>822</priority>
-              <valueHelp>
-                <format>u32:0-16777214</format>
-                <description>VXLAN virtual network identifier</description>
-              </valueHelp>
-              <constraint>
-                <validator name="numeric" argument="--range 0-16777214"/>
-              </constraint>
-            </properties>
-          </leafNode>
+          #include <include/vni.xml.i>
         </children>
       </tagNode>
     </children>

smoketest/scripts/cli/test_vrf.py

@@ -18,7 +18,6 @@
 import os
 import unittest
 
-from netifaces import interfaces
 from base_vyostest_shim import VyOSUnitTestSHIM
 
 from vyos.configsession import ConfigSessionError
@@ -27,6 +26,7 @@
 from vyos.utils.file import read_file
 from vyos.utils.network import get_interface_config
 from vyos.utils.network import is_intf_addr_assigned
+from vyos.utils.network import interface_exists
 from vyos.utils.system import sysctl_read
 
 base_path = ['vrf']
@@ -60,7 +60,7 @@ def tearDown(self):
         self.cli_delete(base_path)
         self.cli_commit()
         for vrf in vrfs:
-            self.assertNotIn(vrf, interfaces())
+            self.assertFalse(interface_exists(vrf))
 
     def test_vrf_vni_and_table_id(self):
         base_table = '1000'
@@ -89,7 +89,7 @@ def test_vrf_vni_and_table_id(self):
         iproute2_config = read_file('/etc/iproute2/rt_tables.d/vyos-vrf.conf')
         for vrf in vrfs:
             description = f'VyOS-VRF-{vrf}'
-            self.assertTrue(vrf in interfaces())
+            self.assertTrue(interface_exists(vrf))
             vrf_if = Interface(vrf)
             # validate proper interface description
             self.assertEqual(vrf_if.get_alias(), description)
@@ -131,7 +131,7 @@ def test_vrf_loopbacks_ips(self):
         loopbacks = ['127.0.0.1', '::1']
         for vrf in vrfs:
             # Ensure VRF was created
-            self.assertIn(vrf, interfaces())
+            self.assertTrue(interface_exists(vrf))
             # Verify IP forwarding is 1 (enabled)
             self.assertEqual(sysctl_read(f'net.ipv4.conf.{vrf}.forwarding'), '1')
             self.assertEqual(sysctl_read(f'net.ipv6.conf.{vrf}.forwarding'), '1')
@@ -171,7 +171,7 @@ def test_vrf_table_id_is_unalterable(self):
         self.cli_commit()
 
         # Check if VRF has been created
-        self.assertTrue(vrf in interfaces())
+        self.assertTrue(interface_exists(vrf))
 
         table = str(int(table) + 1)
         self.cli_set(base + ['table', table])
@@ -228,7 +228,7 @@ def test_vrf_static_route(self):
             next_hop = f'192.0.{table}.1'
             prefix = f'10.0.{table}.0/24'
 
-            self.assertTrue(vrf in interfaces())
+            self.assertTrue(interface_exists(vrf))
 
             frrconfig = self.getFRRconfig(f'vrf {vrf}')
             self.assertIn(f' vni {table}', frrconfig)
@@ -261,7 +261,7 @@ def test_vrf_link_local_ip_addresses(self):
         # Apply VRF config
         self.cli_commit()
         # Ensure VRF got created
-        self.assertIn(vrf, interfaces())
+        self.assertTrue(interface_exists(vrf))
         # ... and IP addresses are still assigned
         for address in addresses:
             self.assertTrue(is_intf_addr_assigned(interface, address))
@@ -293,7 +293,7 @@ def test_vrf_disable_forwarding(self):
         loopbacks = ['127.0.0.1', '::1']
         for vrf in vrfs:
             # Ensure VRF was created
-            self.assertIn(vrf, interfaces())
+            self.assertTrue(interface_exists(vrf))
             # Verify IP forwarding is 0 (disabled)
             self.assertEqual(sysctl_read(f'net.ipv4.conf.{vrf}.forwarding'), '0')
             self.assertEqual(sysctl_read(f'net.ipv6.conf.{vrf}.forwarding'), '0')
@@ -425,7 +425,7 @@ def test_vrf_vni_duplicates(self):
         # Verify VRF configuration
         table = base_table
         for vrf in vrfs:
-            self.assertTrue(vrf in interfaces())
+            self.assertTrue(interface_exists(vrf))
 
             frrconfig = self.getFRRconfig(f'vrf {vrf}')
             self.assertIn(f' vni {table}', frrconfig)
@@ -447,7 +447,7 @@ def test_vrf_vni_add_change_remove(self):
         # Verify VRF configuration
         table = base_table
         for vrf in vrfs:
-            self.assertTrue(vrf in interfaces())
+            self.assertTrue(interface_exists(vrf))
 
             frrconfig = self.getFRRconfig(f'vrf {vrf}')
             self.assertIn(f' vni {table}', frrconfig)
@@ -470,13 +470,39 @@ def test_vrf_vni_add_change_remove(self):
         # Verify VRF configuration
         table = base_table
         for vrf in vrfs:
-            self.assertTrue(vrf in interfaces())
+            self.assertTrue(interface_exists(vrf))
 
             frrconfig = self.getFRRconfig(f'vrf {vrf}')
             self.assertIn(f' vni {table}', frrconfig)
             # Increment table ID for the next run
             table = str(int(table) + 2)
 
+
+        # add a new VRF with VNI - this must not delete any existing VRF/VNI
+        purple = 'purple'
+        table = str(int(table) + 10)
+        self.cli_set(base_path + ['name', purple, 'table', table])
+        self.cli_set(base_path + ['name', purple, 'vni', table])
+
+        # commit changes
+        self.cli_commit()
+
+        # Verify VRF configuration
+        table = base_table
+        for vrf in vrfs:
+            self.assertTrue(interface_exists(vrf))
+
+            frrconfig = self.getFRRconfig(f'vrf {vrf}')
+            self.assertIn(f' vni {table}', frrconfig)
+            # Increment table ID for the next run
+            table = str(int(table) + 2)
+
+        # Verify purple VRF/VNI
+        self.assertTrue(interface_exists(purple))
+        table = str(int(table) + 10)
+        frrconfig = self.getFRRconfig(f'vrf {purple}')
+        self.assertIn(f' vni {table}', frrconfig)
+
         # Now delete all the VNIs
         for vrf in vrfs:
             base = base_path + ['name', vrf]
@@ -487,11 +513,16 @@ def test_vrf_vni_add_change_remove(self):
 
         # Verify no VNI is defined
         for vrf in vrfs:
-            self.assertTrue(vrf in interfaces())
+            self.assertTrue(interface_exists(vrf))
 
             frrconfig = self.getFRRconfig(f'vrf {vrf}')
             self.assertNotIn('vni', frrconfig)
 
+        # Verify purple VNI remains
+        self.assertTrue(interface_exists(purple))
+        frrconfig = self.getFRRconfig(f'vrf {purple}')
+        self.assertIn(f' vni {table}', frrconfig)
+
     def test_vrf_ip_ipv6_nht(self):
         table = '6910'
 

src/conf_mode/protocols_bgp.py

@@ -31,6 +31,7 @@
 from vyos.utils.network import get_interface_vrf
 from vyos.utils.network import is_addr_assigned
 from vyos.utils.process import process_named_running
+from vyos.utils.process import call
 from vyos import ConfigError
 from vyos import frr
 from vyos import airbag
@@ -50,13 +51,8 @@ def get_config(config=None):
 
     # eqivalent of the C foo ? 'a' : 'b' statement
     base = vrf and ['vrf', 'name', vrf, 'protocols', 'bgp'] or base_path
-    bgp = conf.get_config_dict(
-        base,
-        key_mangling=('-', '_'),
-        get_first_key=True,
-        no_tag_node_value_mangle=True,
-        with_recursive_defaults=True,
-    )
+    bgp = conf.get_config_dict(base, key_mangling=('-', '_'),
+                               get_first_key=True, no_tag_node_value_mangle=True)
 
     bgp['dependent_vrfs'] = conf.get_config_dict(['vrf', 'name'],
                                                  key_mangling=('-', '_'),
@@ -75,22 +71,29 @@ def get_config(config=None):
     if vrf:
         bgp.update({'vrf' : vrf})
         # We can not delete the BGP VRF instance if there is a L3VNI configured
+        # FRR L3VNI must be deleted first otherwise we will see error:
+        # "FRR error: Please unconfigure l3vni 3000"
         tmp = ['vrf', 'name', vrf, 'vni']
-        if conf.exists(tmp):
-            bgp.update({'vni' : conf.return_value(tmp)})
+        if conf.exists_effective(tmp):
+            bgp.update({'vni' : conf.return_effective_value(tmp)})
         # We can safely delete ourself from the dependent vrf list
         if vrf in bgp['dependent_vrfs']:
             del bgp['dependent_vrfs'][vrf]
 
-    bgp['dependent_vrfs'].update({'default': {'protocols': {
-        'bgp': conf.get_config_dict(base_path, key_mangling=('-', '_'),
-                                    get_first_key=True,
-                                    no_tag_node_value_mangle=True)}}})
+        bgp['dependent_vrfs'].update({'default': {'protocols': {
+            'bgp': conf.get_config_dict(base_path, key_mangling=('-', '_'),
+                                        get_first_key=True,
+                                        no_tag_node_value_mangle=True)}}})
+
     if not conf.exists(base):
         # If bgp instance is deleted then mark it
         bgp.update({'deleted' : ''})
         return bgp
 
+    # We have gathered the dict representation of the CLI, but there are default
+    # options which we need to update into the dictionary retrived.
+    bgp = conf.merge_defaults(bgp, recursive=True)
+
     # We also need some additional information from the config, prefix-lists
     # and route-maps for instance. They will be used in verify().
     #
@@ -242,10 +245,6 @@ def verify(bgp):
                 if verify_vrf_as_import(bgp['vrf'], tmp_afi, bgp['dependent_vrfs']):
                     raise ConfigError(f'Cannot delete VRF instance "{bgp["vrf"]}", ' \
                                       'unconfigure "import vrf" commands!')
-            # We can not delete the BGP instance if a L3VNI instance exists
-            if 'vni' in bgp:
-                raise ConfigError(f'Cannot delete VRF instance "{bgp["vrf"]}", ' \
-                                  f'unconfigure VNI "{bgp["vni"]}" first!')
         else:
             # We are running in the default VRF context, thus we can not delete
             # our main BGP instance if there are dependent BGP VRF instances.
@@ -254,7 +253,11 @@ def verify(bgp):
                     if vrf != 'default':
                         if dict_search('protocols.bgp', vrf_options):
                             raise ConfigError('Cannot delete default BGP instance, ' \
-                                              'dependent VRF instance(s) exist!')
+                                              'dependent VRF instance(s) exist(s)!')
+                        if 'vni' in vrf_options:
+                            raise ConfigError('Cannot delete default BGP instance, ' \
+                                              'dependent L3VNI exists!')
+
         return None
 
     if 'system_as' not in bgp:
@@ -591,6 +594,13 @@ def generate(bgp):
     return None
 
 def apply(bgp):
+    if 'deleted' in bgp:
+        # We need to ensure that the L3VNI is deleted first.
+        # This is not possible with old config backend
+        # priority bug
+        if {'vrf', 'vni'} <= set(bgp):
+            call('vtysh -c "conf t" -c "vrf {vrf}" -c "no vni {vni}"'.format(**bgp))
+
     bgp_daemon = 'bgpd'
 
     # Save original configuration prior to starting any commit actions

src/conf_mode/vrf.py

@@ -130,11 +130,6 @@ def get_config(config=None):
     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