Merge pull request #3393 from vyos/mergify/bp/sagitta/pr-3392

bgp: T6189: L3VPN connectivity is broken after re-enabling VRF (backport #3392)

7 files changed, 74 insertions, 159 deletions

diff --git a/data/configd-include.json b/data/configd-include.json
index fe27ae2b7..212b260e1 100644
--- a/data/configd-include.json
+++ b/data/configd-include.json
@@ -107,6 +107,5 @@
 "vpn_openconnect.py",
 "vpn_pptp.py",
 "vpn_sstp.py",
-"vrf.py",
-"vrf_vni.py"
+"vrf.py"
 ]
diff --git a/data/templates/frr/zebra.vrf.route-map.frr.j2 b/data/templates/frr/zebra.vrf.route-map.frr.j2
index f1cc6fe66..8ebb82511 100644
--- a/data/templates/frr/zebra.vrf.route-map.frr.j2
+++ b/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
diff --git a/interface-definitions/vrf.xml.in b/interface-definitions/vrf.xml.in
index 94ed96e4b..a20be995a 100644
--- a/interface-definitions/vrf.xml.in
+++ b/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>
diff --git a/smoketest/scripts/cli/test_vrf.py b/smoketest/scripts/cli/test_vrf.py
index f6e4181c0..243397dc2 100755
--- a/smoketest/scripts/cli/test_vrf.py
+++ b/smoketest/scripts/cli/test_vrf.py
@@ -18,7 +18,6 @@ import re
 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.ifconfig import Section
 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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
             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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         # 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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         # 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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         # 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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
         # 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 @@ class VRFTest(VyOSUnitTestSHIM.TestCase):
 
         # 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'
 
diff --git a/src/conf_mode/protocols_bgp.py b/src/conf_mode/protocols_bgp.py
index 4df97d133..44409c0e3 100755
--- a/src/conf_mode/protocols_bgp.py
+++ b/src/conf_mode/protocols_bgp.py
@@ -31,6 +31,7 @@ from vyos.utils.dict import dict_search
 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:
@@ -607,6 +610,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
diff --git a/src/conf_mode/vrf.py b/src/conf_mode/vrf.py
index 587309005..8d8c234c0 100755
--- a/src/conf_mode/vrf.py
+++ b/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
diff --git a/src/conf_mode/vrf_vni.py b/src/conf_mode/vrf_vni.py
deleted file mode 100644
index 8dab164d7..000000000
--- a/src/conf_mode/vrf_vni.py
+++ /dev/null
@@ -1,103 +0,0 @@
-#!/usr/bin/env python3
-#
-# Copyright (C) 2023-2024 VyOS maintainers and contributors
-#
-# This program is free software; you can redistribute it and/or modify
-# it under the terms of the GNU General Public License version 2 or later as
-# published by the Free Software Foundation.
-#
-# This program 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 General Public License for more details.
-#
-# You should have received a copy of the GNU General Public License
-# along with this program.  If not, see <http://www.gnu.org/licenses/>.
-
-from sys import argv
-from sys import exit
-
-from vyos.config import Config
-from vyos.template import render_to_string
-from vyos import ConfigError
-from vyos import frr
-from vyos import airbag
-airbag.enable()
-
-def get_config(config=None):
-    if config:
-        conf = config
-    else:
-        conf = Config()
-
-    vrf_name = None
-    if len(argv) > 1:
-        vrf_name = argv[1]
-    else:
-        return None
-
-    # 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})
-
-    return vrf
-
-def verify(vrf):
-    if not vrf:
-        return
-
-    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):
-    if not vrf:
-        return
-
-    vrf['new_frr_config'] = render_to_string('frr/zebra.vrf.route-map.frr.j2', vrf)
-    return None
-
-def apply(vrf):
-    frr_daemon = 'zebra'
-
-    # add configuration to FRR
-    frr_cfg = frr.FRRConfig()
-    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 'only_vrf' in vrf:
-        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'])
-    frr_cfg.commit_configuration(frr_daemon)
-
-    return None
-
-if __name__ == '__main__':
-    try:
-        c = get_config()
-        verify(c)
-        generate(c)
-        apply(c)
-    except ConfigError as e:
-        print(e)
-        exit(1)