pppoe-server: T8143: Set 'vpp-cp' option automatically if interface is in VPP

7 files changed, 93 insertions, 83 deletions

diff --git a/interface-definitions/include/version/pppoe-server-version.xml.i b/interface-definitions/include/version/pppoe-server-version.xml.i
index 2e020faa3..1b2718d61 100644
--- a/interface-definitions/include/version/pppoe-server-version.xml.i
+++ b/interface-definitions/include/version/pppoe-server-version.xml.i
@@ -1,3 +1,3 @@
 <!-- include start from include/version/pppoe-server-version.xml.i -->
-<syntaxVersion component='pppoe-server' version='11'></syntaxVersion>
+<syntaxVersion component='pppoe-server' version='12'></syntaxVersion>
 <!-- include end -->
diff --git a/interface-definitions/service_pppoe-server.xml.in b/interface-definitions/service_pppoe-server.xml.in
index 955c774e2..81a4a95e3 100644
--- a/interface-definitions/service_pppoe-server.xml.in
+++ b/interface-definitions/service_pppoe-server.xml.in
@@ -71,12 +71,6 @@
               </leafNode>
               #include <include/accel-ppp/vlan.xml.i>
               #include <include/accel-ppp/vlan-mon.xml.i>
-              <leafNode name="vpp-cp">
-                <properties>
-                  <help>Enable PPPoE control-plane integration with VPP</help>
-                  <valueless/>
-                </properties>
-              </leafNode>
             </children>
           </tagNode>
           <leafNode name="service-name">
diff --git a/smoketest/scripts/cli/test_service_pppoe-server.py b/smoketest/scripts/cli/test_service_pppoe-server.py
index 5e1cbdd88..1a06e050f 100755
--- a/smoketest/scripts/cli/test_service_pppoe-server.py
+++ b/smoketest/scripts/cli/test_service_pppoe-server.py
@@ -213,19 +213,6 @@ class TestServicePPPoEServer(BasicAccelPPPTest.TestCase):
         self.assertIn('accept-any-service=1', config)
         self.assertIn('accept-blank-service=1', config)
 
-    def test_accel_vpp_cp(self):
-        self.basic_config()
-        self.cli_commit()
-
-        self.set(['interface', interface, 'vpp-cp'])
-
-        # vpp-cp require VPP service to be started and interface is configured in VPP
-        with self.assertRaises(ConfigSessionError):
-            self.cli_commit()
-
-        # All other checks for PPPoE control-plane integration
-        # with VPP are verified in test_vpp.py
-
 
 if __name__ == '__main__':
     unittest.main(verbosity=2, failfast=VyOSUnitTestSHIM.TestCase.debug_on())
diff --git a/smoketest/scripts/cli/test_vpp.py b/smoketest/scripts/cli/test_vpp.py
index 1886ac615..b1c57dd8c 100755
--- a/smoketest/scripts/cli/test_vpp.py
+++ b/smoketest/scripts/cli/test_vpp.py
@@ -1562,9 +1562,8 @@ class TestVPP(VyOSUnitTestSHIM.TestCase):
         self.cli_set(pppoe_base + ['client-ip-pool', pool, 'range', '192.0.2.0/24'])
         self.cli_set(pppoe_base + ['default-pool', pool])
 
-        # Enable PPPoE control-plane integration with VPP
-        self.cli_set(pppoe_base + ['interface', interface, 'vpp-cp'])
-        self.cli_set(pppoe_base + ['interface', f'{interface}.{vni}', 'vpp-cp'])
+        self.cli_set(pppoe_base + ['interface', interface])
+        self.cli_set(pppoe_base + ['interface', f'{interface}.{vni}'])
 
         self.cli_commit()
 
@@ -1572,6 +1571,7 @@ class TestVPP(VyOSUnitTestSHIM.TestCase):
         config = read_file(config_file)
 
         # Validate configuration
+        # PPPoE on VPP-managed interfaces automatically get control-plane integration
         self.assertIn(f'interface={interface},vpp-cp=true', config)
         self.assertIn(f'interface={interface}.{vni},vpp-cp=true', config)
 
diff --git a/src/conf_mode/service_pppoe-server.py b/src/conf_mode/service_pppoe-server.py
index bfa475b95..3d1fcab98 100755
--- a/src/conf_mode/service_pppoe-server.py
+++ b/src/conf_mode/service_pppoe-server.py
@@ -20,11 +20,12 @@ from sys import exit
 
 from vyos.config import Config
 from vyos.configdict import get_accel_dict
-from vyos.configdict import is_node_changed
-from vyos.configdiff import get_config_diff, Diff
+from vyos.configdict import is_node_changed, node_changed
+from vyos.configdiff import Diff
 from vyos.configverify import verify_interface_exists
 from vyos.template import render
 from vyos.utils.process import call
+from vyos.utils.process import is_systemd_service_active
 from vyos.utils.dict import dict_search
 from vyos.accel_ppp_util import verify_accel_ppp_name_servers
 from vyos.accel_ppp_util import verify_accel_ppp_wins_servers
@@ -40,6 +41,12 @@ airbag.enable()
 pppoe_conf = r'/run/accel-pppd/pppoe.conf'
 pppoe_chap_secrets = r'/run/accel-pppd/pppoe.chap-secrets'
 
+
+def base_ifname(ifname):
+    # Get the base interface name without VLAN
+    return ifname.split('.')[0]
+
+
 def convert_pado_delay(pado_delay):
     new_pado_delay = {'delays_without_sessions': [],
                       'delays_with_sessions': []}
@@ -60,18 +67,18 @@ def get_config(config=None):
     # retrieve common dictionary keys
     pppoe = get_accel_dict(conf, base, pppoe_chap_secrets)
 
-    if conf.exists(['vpp']):
-        pppoe['vpp_config'] = conf.get_config_dict(
-            ['vpp'],
+    vpp_interface_base = ['vpp', 'settings', 'interface']
+    if conf.exists(vpp_interface_base) and is_systemd_service_active('vpp.service'):
+        vpp_ifaces = conf.get_config_dict(
+            vpp_interface_base,
             key_mangling=('-', '_'),
             get_first_key=True,
             no_tag_node_value_mangle=True,
         )
-
-    diff = get_config_diff(conf)
-    node_diff = diff.get_child_nodes_diff(
-        base + ['interface'], expand_nodes=Diff.DELETE, recursive=True
-    )
+        pppoe['vpp_ifaces'] = vpp_ifaces
+        for interface in pppoe.get('interface', {}):
+            if base_ifname(interface) in vpp_ifaces:
+                pppoe['interface'][interface]['vpp_cp'] = {}
 
     pppoe['vpp_cp_interfaces'] = {
         'add': [
@@ -81,8 +88,8 @@ def get_config(config=None):
         ],
         'delete': [
             iface
-            for iface, iface_config in node_diff.get('delete').items()
-            if 'vpp-cp' in iface_config
+            for iface in node_changed(conf, base + ['interface'])
+            if base_ifname(iface) in pppoe.get('vpp_ifaces', {})
         ],
     }
 
@@ -98,14 +105,23 @@ def get_config(config=None):
         pado_delay = dict_search('pado_delay', pppoe)
         pppoe['pado_delay'] = convert_pado_delay(pado_delay)
 
-    # reload-or-restart does not implemented in accel-ppp
+    # reload-or-restart is not implemented in accel-ppp
     # use this workaround until it will be implemented
     # https://phabricator.accel-ppp.org/T3
-    conditions = [is_node_changed(conf, base + ['client-ip-pool']),
-                  is_node_changed(conf, base + ['client-ipv6-pool']),
-                  is_node_changed(conf, base + ['interface']),
-                  is_node_changed(conf, base + ['authentication','radius','dynamic-author']),
-                  is_node_changed(conf, base + ['authentication','mode'])]
+    changed_vpp_ifaces = node_changed(
+        conf, vpp_interface_base, expand_nodes=Diff.DELETE | Diff.ADD
+    )
+    conditions = [
+        is_node_changed(conf, base + ['client-ip-pool']),
+        is_node_changed(conf, base + ['client-ipv6-pool']),
+        is_node_changed(conf, base + ['interface']),
+        is_node_changed(conf, base + ['authentication', 'radius', 'dynamic-author']),
+        is_node_changed(conf, base + ['authentication', 'mode']),
+        any(
+            base_ifname(iface) in changed_vpp_ifaces
+            for iface in pppoe.get('interface', {})
+        ),
+    ]
     if any(conditions):
         pppoe.update({'restart_required': {}})
     pppoe['server_type'] = 'pppoe'
@@ -154,27 +170,19 @@ def verify(pppoe):
 
     # Check is interface exists in the system
     for interface, interface_config in pppoe['interface'].items():
-        verify_interface_exists(pppoe, interface, warning_only=True)
+        # 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 'vlan_mon' in interface_config and not 'vlan' in interface_config:
-            raise ConfigError('Option "vlan-mon" requires "vlan" to be set!')
-
-    vpp_cp_interfaces = pppoe.get('vpp_cp_interfaces', {}).get('add', [])
-    if vpp_cp_interfaces:
-        try:
-            vpp = VPPControl()
-        except Exception:
+        if 'vlan_mon' in interface_config and base_ifname(interface) in pppoe.get(
+            'vpp_ifaces', {}
+        ):
             raise ConfigError(
-                'PPPoE control-plane integration with VPP is enabled on '
-                f'interface(s) {", ".join(vpp_cp_interfaces)} '
-                'but VPP service was not started'
+                f'Cannot set option "vlan-mon": interface {interface} is integrated with control-plane!'
             )
 
-        for iface in vpp_cp_interfaces:
-            if vpp.get_sw_if_index(iface) is None:
-                raise ConfigError(
-                    f'{iface} should be a VPP interface for control-plane integration'
-                )
+        if 'vlan_mon' in interface_config and not 'vlan' in interface_config:
+            raise ConfigError('Option "vlan-mon" requires "vlan" to be set!')
 
     return None
 
@@ -196,7 +204,7 @@ def apply(pppoe):
 
     # delete pppoe mapping in vpp
     vpp_cp_ifaces_delete = pppoe.get('vpp_cp_interfaces', {}).get('delete', [])
-    if 'vpp_config' in pppoe and vpp_cp_ifaces_delete:
+    if 'vpp_ifaces' in pppoe and vpp_cp_ifaces_delete:
         vpp = VPPControl()
         for iface in vpp_cp_ifaces_delete:
             vpp.map_pppoe_interface(iface, is_add=False)
diff --git a/src/conf_mode/vpp.py b/src/conf_mode/vpp.py
index 4cbfab6bc..274cd1da5 100755
--- a/src/conf_mode/vpp.py
+++ b/src/conf_mode/vpp.py
@@ -265,25 +265,24 @@ def get_config(config=None):
             # to be reinitialized after the commit
             set_dependents('ethernet', conf, removed_iface)
 
-    # Get interfaces that are used in PPPoe for control-plane integration
-    pppoe_conf = conf.get_config_dict(
-        ['service', 'pppoe-server'],
+    # Get interfaces that should be used in PPPoE for control-plane integration
+    pppoe_ifaces = conf.get_config_dict(
+        ['service', 'pppoe-server', 'interface'],
         key_mangling=('-', '_'),
         get_first_key=True,
         no_tag_node_value_mangle=True,
     )
-    pppoe_map_ifaces = [
-        ifname
-        for ifname, iface_conf in pppoe_conf.get('interface', {}).items()
-        if 'vpp_cp' in iface_conf
+    changed_pppoe_ifaces = [
+        iface for iface in pppoe_ifaces if iface.split('.')[0] in tmp
     ]
 
     if not conf.exists(base):
+        if changed_pppoe_ifaces:
+            set_dependents('pppoe_server', conf)
         return {
             'removed_ifaces': removed_ifaces,
             'xconn_members': xconn_members,
             'persist_config': eth_ifaces_persist,
-            **({'pppoe_ifaces': pppoe_map_ifaces} if pppoe_map_ifaces else {}),
         }
 
     config = conf.get_config_dict(
@@ -455,28 +454,19 @@ def get_config(config=None):
         set_dependents('vpp_ipfix', conf)
 
     # PPPoE dependency
-    if pppoe_map_ifaces:
-        config['pppoe_ifaces'] = pppoe_map_ifaces
+    added_pppoe_ifaces = [
+        iface
+        for iface in pppoe_ifaces
+        if iface.split('.')[0] in config.get('settings', {}).get('interface', {})
+    ]
+    changed_pppoe_ifaces.extend(added_pppoe_ifaces)
+    if changed_pppoe_ifaces:
         set_dependents('pppoe_server', conf)
 
     return config
 
 
 def verify(config):
-    # Cannot remove interface if PPPoE control-plane is still enabled
-    removed_ifaces = [iface['iface_name'] for iface in config.get('removed_ifaces', [])]
-    pppoe_removed_ifaces = [
-        p
-        for p in config.get('pppoe_ifaces', [])
-        for r in removed_ifaces
-        if p == r or p.startswith(f'{r}.')
-    ]
-    if pppoe_removed_ifaces:
-        raise ConfigError(
-            f'{", ".join(pppoe_removed_ifaces)} still in use by the PPPoE server. '
-            'Disable PPPoE control-plane integration with VPP before proceeding.'
-        )
-
     # Check remove VPP interface that used in IPFIX
     _check_removed_interfaces(
         config, 'IPFIX monitoring', config.get('ipfix', {}).get('interface', {})
diff --git a/src/migration-scripts/pppoe-server/11-to-12 b/src/migration-scripts/pppoe-server/11-to-12
new file mode 100644
index 000000000..d38ba0367
--- /dev/null
+++ b/src/migration-scripts/pppoe-server/11-to-12
@@ -0,0 +1,31 @@
+# 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/>.
+
+# Delete 'vpp-cp' option from interface settings
+# because it will be set automatically (T8143)
+
+from vyos.configtree import ConfigTree
+
+base = ['service', 'pppoe-server']
+
+def migrate(config: ConfigTree) -> None:
+    if not config.exists(base):
+        return
+
+    for interface in config.list_nodes(base + ['interface']):
+        base_path = base + ['interface', interface]
+        # Delete vpp-cp option from PPPoE interface settings
+        if config.exists(base_path + ['vpp-cp']):
+            config.delete(base_path + ['vpp-cp'])