Merge pull request #5021 from alexandr-san4ez/T8276-current

vpp: T8276: Add verification for virtual interfaces in PPPoE server configuration

5 files changed, 187 insertions, 11 deletions

diff --git a/python/vyos/configverify.py b/python/vyos/configverify.py
index 177c4f9b0..5ff8dda2c 100644
--- a/python/vyos/configverify.py
+++ b/python/vyos/configverify.py
@@ -22,7 +22,11 @@
 # makes use of it!
 
 from vyos import ConfigError
+from vyos.base import Warning
 from vyos.utils.dict import dict_search
+from vyos.utils.dict import dict_search_recursive
+from vyos.utils.network import interface_exists
+
 # pattern re-used in ipsec migration script
 dynamic_interface_pattern = r'(ppp|pppoe|sstpc|l2tp|ipoe)[0-9]+'
 
@@ -100,7 +104,6 @@ def verify_vrf(config):
     Common helper function used by interface implementations to perform
     recurring validation of VRF configuration.
     """
-    from vyos.utils.network import interface_exists
     if 'vrf' in config:
         vrfs = config['vrf']
         if isinstance(vrfs, str):
@@ -177,7 +180,6 @@ def verify_mirror_redirect(config):
 
     It makes no sense to mirror traffic back at yourself!
     """
-    from vyos.utils.network import interface_exists
     if {'mirror', 'redirect'} <= set(config):
         raise ConfigError('Mirror and redirect can not be enabled at the same time!')
 
@@ -247,10 +249,6 @@ def verify_interface_exists(config, ifname, state_required=False, warning_only=F
     if the interface is defined on the CLI, if it's not found we try if
     it exists at the OS level.
     """
-    from vyos.base import Warning
-    from vyos.utils.dict import dict_search_recursive
-    from vyos.utils.network import interface_exists
-
     if not state_required:
         # Check if interface is present in CLI config
         tmp = getattr(config, 'interfaces_root', {})
@@ -267,6 +265,47 @@ def verify_interface_exists(config, ifname, state_required=False, warning_only=F
         return False
     raise ConfigError(message)
 
+def verify_virtual_interface_exists(
+    config, ifname, state_required=False, warning_only=False
+):
+    """
+    Verify the existence of a virtual network interface in the configuration or the Linux kernel.
+
+    This function checks whether a specified virtual interface exists in the provided configuration
+    or in the Linux kernel. It can return a warning or raise an error based on the parameters provided.
+    """
+    physical_ifname, vif_id = ifname.split('.', maxsplit=1)
+
+    if vif_id and '.' in vif_id:
+        vif_s, vif_c = vif_id.split('.', maxsplit=1)
+        vif_id = None
+    else:
+        vif_s = vif_c = None
+
+    if not state_required:
+        # Check if sub-interface is present in CLI config
+        interfaces_root = getattr(config, 'interfaces_root', {})
+
+        if vif_s and vif_c:
+            path = ['ethernet', physical_ifname, 'vif-s', vif_s, 'vif-c']
+            key = vif_c
+        else:
+            path = ['ethernet', physical_ifname, 'vif']
+            key = vif_id
+
+        if bool(list(dict_search_recursive(interfaces_root, key, path=path))):
+            return True
+
+    # Interface not found on CLI, try Linux Kernel
+    if interface_exists(ifname):
+        return True
+
+    message = f'Virtual Interface "{ifname}" does not exist!'
+    if warning_only:
+        Warning(message)
+        return False
+    raise ConfigError(message)
+
 def verify_source_interface(config):
     """
     Common helper function used by interface implementations to
@@ -274,7 +313,6 @@ def verify_source_interface(config):
     required by e.g. peth/MACvlan, MACsec ...
     """
     import re
-    from vyos.utils.network import interface_exists
 
     ifname = config['ifname']
     if 'source_interface' not in config:
diff --git a/python/vyos/vpp/control_vpp.py b/python/vyos/vpp/control_vpp.py
index 3fab4039a..b37bb865f 100644
--- a/python/vyos/vpp/control_vpp.py
+++ b/python/vyos/vpp/control_vpp.py
@@ -477,6 +477,30 @@ class VPPControl:
             )
 
     @_Decorators.api_call
+    def get_pppoe_interface_mapping(self) -> dict:
+        """
+        Get PPPoE mapping between data-plain and control-plane interfaces
+
+        Returns:
+            dict: Dict where key is the dataplane interface and value is the control interface
+        """
+
+        reply = self.cli_cmd('show pppoe control-plane binding').reply
+
+        if 'No PPPoE control plane interface configured' in reply:
+            return {}
+
+        lines = reply.splitlines()
+        lines = lines[2:]  # Ignore two lines with headers
+
+        result = {}
+        for line in lines:
+            key, value = line.split()
+            result[key] = value
+
+        return result
+
+    @_Decorators.api_call
     def enable_dhcp_client(self, ifname: str) -> None:
         """Enable DHCP client detection on a given interface
 
diff --git a/smoketest/scripts/cli/test_vpp.py b/smoketest/scripts/cli/test_vpp.py
index 2c17345ae..a98b5ff0b 100755
--- a/smoketest/scripts/cli/test_vpp.py
+++ b/smoketest/scripts/cli/test_vpp.py
@@ -31,6 +31,7 @@ from vyos.utils.process import process_named_running
 from vyos.utils.file import read_file
 from vyos.utils.process import rc_cmd
 from vyos.utils.system import sysctl_read
+from vyos.utils.network import interface_exists
 from vyos.system import image
 from vyos.vpp import VPPControl
 from vyos.vpp.utils import vpp_iface_name_transform
@@ -1034,7 +1035,7 @@ class TestVPP(VyOSUnitTestSHIM.TestCase):
         self.assertEqual(sysctl_read('vm.max_map_count'), '65530')
         self.assertEqual(sysctl_read('kernel.shmmax'), '8589934592')
 
-    def test_17_vpp_pppoe_mapping(self):
+    def test_17_1_vpp_pppoe_mapping(self):
         config_file = '/run/accel-pppd/pppoe.conf'
         pool = "TEST-POOL"
         vni = '23'
@@ -1084,6 +1085,102 @@ class TestVPP(VyOSUnitTestSHIM.TestCase):
         self.cli_delete(['interfaces', 'ethernet', interface, 'vif'])
         self.cli_commit()
 
+    def test_17_2_vpp_pppoe_invalid_vif(self):
+        # Test verify step behavior when referenced PPPoE interface does not actually exist
+        pool = "TEST-POOL-2"
+        vni = '24'
+        pppoe_base = ['service', 'pppoe-server']
+
+        # Basic pppoe-server config
+        self.cli_set(pppoe_base + ['authentication', 'mode', 'noauth'])
+        self.cli_set(pppoe_base + ['gateway-address', '192.0.3.1'])
+        self.cli_set(pppoe_base + ['client-ip-pool', pool, 'range', '192.0.3.0/24'])
+        self.cli_set(pppoe_base + ['default-pool', pool])
+
+        self.cli_set(pppoe_base + ['interface', interface, 'combined'])
+        self.cli_set(pppoe_base + ['interface', f'{interface}.{vni}'])
+
+        err_msg = f'Virtual Interface "{interface}.{vni}" does not exist'
+        with self.assertRaisesRegex(ConfigSessionError, err_msg):
+            self.cli_commit()
+
+        # The second commit can throw exception instead of verify error:
+        #   - `FileNotFoundError: PCI device tap does not exist`
+        # More details here: https://vyos.dev/T8276
+        with self.assertRaisesRegex(ConfigSessionError, err_msg):
+            self.cli_commit()
+        self.assertTrue(interface_exists(interface))
+
+        self.cli_set(['interfaces', 'ethernet', interface, 'vif', vni])
+        self.cli_commit()
+
+        # Cleanup PPPoE server configuration and created VIF
+        self.cli_delete(pppoe_base)
+        self.cli_delete(['interfaces', 'ethernet', interface, 'vif', vni])
+        self.cli_commit()
+
+    def test_17_3_vpp_pppoe_delete_invalid_vif(self):
+        # Test verify step behavior when referenced PPPoE virtual interface was deleted
+        pool = "TEST-POOL-3"
+        vni = '25'
+        pppoe_base = ['service', 'pppoe-server']
+
+        # Basic pppoe-server config
+        self.cli_set(pppoe_base + ['authentication', 'mode', 'noauth'])
+        self.cli_set(pppoe_base + ['gateway-address', '192.0.4.1'])
+        self.cli_set(pppoe_base + ['client-ip-pool', pool, 'range', '192.0.4.0/24'])
+        self.cli_set(pppoe_base + ['default-pool', pool])
+        self.cli_set(pppoe_base + ['interface', interface, 'combined'])
+        self.cli_set(pppoe_base + ['interface', f'{interface}.{vni}'])
+
+        err_msg = f'Virtual Interface "{interface}.{vni}" does not exist'
+        with self.assertRaisesRegex(ConfigSessionError, err_msg):
+            self.cli_commit()
+
+        self.cli_delete(pppoe_base + ['interface', f'{interface}.{vni}'])
+        self.cli_commit()
+
+        # Cleanup PPPoE server configuration and created VIF
+        self.cli_delete(pppoe_base)
+        self.cli_commit()
+
+    def test_17_4_vpp_pppoe_invalid_sub_vif(self):
+        # Test verify step behavior when referenced PPPoE
+        # sub-interface which have several tags does not exist
+        pool = "TEST-POOL-4"
+        vif_s, vif_c = '26', '10'
+        pppoe_base = ['service', 'pppoe-server']
+
+        # Basic pppoe-server config
+        self.cli_set(pppoe_base + ['authentication', 'mode', 'noauth'])
+        self.cli_set(pppoe_base + ['gateway-address', '192.0.5.1'])
+        self.cli_set(pppoe_base + ['client-ip-pool', pool, 'range', '192.0.5.0/24'])
+        self.cli_set(pppoe_base + ['default-pool', pool])
+
+        self.cli_set(pppoe_base + ['interface', interface, 'combined'])
+        self.cli_set(pppoe_base + ['interface', f'{interface}.{vif_s}.{vif_c}'])
+
+        err_msg = f'Virtual Interface "{interface}.{vif_s}.{vif_c}" does not exist'
+        with self.assertRaisesRegex(ConfigSessionError, err_msg):
+            self.cli_commit()
+
+        # The second commit can throw exception instead of verify error:
+        #   - `FileNotFoundError: PCI device tap does not exist`
+        # More details here: https://vyos.dev/T8276
+        with self.assertRaisesRegex(ConfigSessionError, err_msg):
+            self.cli_commit()
+        self.assertTrue(interface_exists(interface))
+
+        self.cli_set(
+            ['interfaces', 'ethernet', interface, 'vif-s', vif_s, 'vif-c', vif_c]
+        )
+        self.cli_commit()
+
+        # Cleanup PPPoE server configuration and created VIF
+        self.cli_delete(pppoe_base)
+        self.cli_delete(['interfaces', 'ethernet', interface, 'vif-s', vif_s])
+        self.cli_commit()
+
     def test_18_kernel_options_hugepages(self):
         default_hp_size = '2M'
         hp_size_1g = '1G'
diff --git a/src/conf_mode/service_pppoe-server.py b/src/conf_mode/service_pppoe-server.py
index 3d1fcab98..dbcbbabf2 100755
--- a/src/conf_mode/service_pppoe-server.py
+++ b/src/conf_mode/service_pppoe-server.py
@@ -23,6 +23,7 @@ from vyos.configdict import get_accel_dict
 from vyos.configdict import is_node_changed, node_changed
 from vyos.configdiff import Diff
 from vyos.configverify import verify_interface_exists
+from vyos.configverify import verify_virtual_interface_exists
 from vyos.template import render
 from vyos.utils.process import call
 from vyos.utils.process import is_systemd_service_active
@@ -172,7 +173,11 @@ def verify(pppoe):
     for interface, interface_config in pppoe['interface'].items():
         # Interfaces integrated with the control-plane in VPP must exist in the system
         warning_only = 'vpp_cp' not in interface_config
-        verify_interface_exists(pppoe, interface, warning_only=warning_only)
+        if '.' in interface:
+            verify_interface_func = verify_virtual_interface_exists
+        else:
+            verify_interface_func = verify_interface_exists
+        verify_interface_func(pppoe, interface, warning_only=warning_only)
 
         if 'vlan_mon' in interface_config and base_ifname(interface) in pppoe.get(
             'vpp_ifaces', {}
@@ -206,8 +211,11 @@ def apply(pppoe):
     vpp_cp_ifaces_delete = pppoe.get('vpp_cp_interfaces', {}).get('delete', [])
     if 'vpp_ifaces' in pppoe and vpp_cp_ifaces_delete:
         vpp = VPPControl()
+        # Make sure that the mapping really exists
+        mapping = vpp.get_pppoe_interface_mapping()
         for iface in vpp_cp_ifaces_delete:
-            vpp.map_pppoe_interface(iface, is_add=False)
+            if iface in mapping:
+                vpp.map_pppoe_interface(iface, is_add=False)
 
     if 'remove' in pppoe:
         call(f'systemctl stop {systemd_service}')
diff --git a/src/conf_mode/vpp.py b/src/conf_mode/vpp.py
index d39eadac1..e6b739179 100755
--- a/src/conf_mode/vpp.py
+++ b/src/conf_mode/vpp.py
@@ -29,8 +29,11 @@ from vyos import ConfigError
 from vyos import airbag
 from vyos.base import Warning
 from vyos.config import Config, config_dict_merge
-from vyos.configdep import set_dependents, call_dependents
+from vyos.configdep import set_dependents
+from vyos.configdep import call_dependents
 from vyos.configdict import node_changed
+from vyos.configverify import verify_interface_exists
+from vyos.configverify import verify_virtual_interface_exists
 from vyos.ifconfig import Section
 from vyos.logger import getLogger
 from vyos.template import render
@@ -480,6 +483,7 @@ def get_config(config=None):
     changed_pppoe_ifaces.extend(added_pppoe_ifaces)
     if changed_pppoe_ifaces:
         set_dependents('pppoe_server', conf)
+    config['changed_pppoe_ifaces'] = changed_pppoe_ifaces
 
     return config
 
@@ -610,6 +614,11 @@ def verify(config):
 
     verify_routes_count(config['settings'])
 
+    for pppoe_iface in config.get('changed_pppoe_ifaces', []):
+        if '.' in pppoe_iface:
+            verify_virtual_interface_exists(config, pppoe_iface)
+        else:
+            verify_interface_exists(config, pppoe_iface)
 
 def generate(config):
     if not config or 'remove' in config: