pppoe: T4384: replace default-route CLI option with common CLI nodes already present for DHCP

VyOS 1.4 still leverages PPPd internals on the CLI.

pppd supports three options for a default route, none, auto, force.
* none: No default route is installed on interface up
* auto: Default route is only installed if there is yet no default route
* force: overwrite any default route

There are several drawbacks in this design for VyOS and the users. If auto is
specified, this only counted for static default routes - but what about dynamic
ones? Same for force, only a static default route got replaced but dynamic ones
did not got taken into account.

The CLI is changed and we now re-use already existing nodes from the DHCP
interface configuration:
* no-default-route:
  On link up no default route is installed, same as the previous
  default-route none

* default-route-distance:
  We can now specify the distance of this route for the routing table on the
  system. This defaults to 210 as we have for DHCP interfaces. All this will be
  migrated using a CLI migration script.

10 files changed, 158 insertions, 137 deletions

diff --git a/data/templates/frr/staticd.frr.j2 b/data/templates/frr/staticd.frr.j2
index cf8448f7f..589f03c2c 100644
--- a/data/templates/frr/staticd.frr.j2
+++ b/data/templates/frr/staticd.frr.j2
@@ -18,17 +18,18 @@ vrf {{ vrf }}
 {# IPv4 default routes from DHCP interfaces #}
 {% if dhcp is vyos_defined %}
 {%     for interface, interface_config in dhcp.items() %}
-{#         PPPoE routes behave a bit different ... #}
-{%         if interface.startswith('pppoe') and interface_config.default_route is vyos_defined and interface_config.default_route is not vyos_defined('none') %}
-{{ ip_prefix }} route 0.0.0.0/0 {{ interface }} tag 210
-{%         else %}
-{%             set next_hop = interface | get_dhcp_router %}
-{%             if next_hop is vyos_defined %}
-{{ ip_prefix }} route 0.0.0.0/0 {{ next_hop }} {{ interface }} tag 210 {{ interface_config.distance if interface_config.distance is vyos_defined }}
-{%             endif %}
+{%         set next_hop = interface | get_dhcp_router %}
+{%         if next_hop is vyos_defined %}
+{{ ip_prefix }} route 0.0.0.0/0 {{ next_hop }} {{ interface }} tag 210 {{ interface_config.dhcp_options.default_route_distance if interface_config.dhcp_options.default_route_distance is vyos_defined }}
 {%         endif %}
 {%     endfor %}
 {% endif %}
+{# IPv4 default routes from PPPoE interfaces #}
+{% if pppoe is vyos_defined %}
+{%     for interface, interface_config in pppoe.items() %}
+{{ ip_prefix }} route 0.0.0.0/0 {{ interface }} tag 210 {{ interface_config.default_route_distance if interface_config.default_route_distance is vyos_defined }}
+{%     endfor %}
+{% endif %}
 {# IPv6 routing #}
 {% if route6 is vyos_defined %}
 {%     for prefix, prefix_config in route6.items() %}
diff --git a/interface-definitions/include/version/interfaces-version.xml.i b/interface-definitions/include/version/interfaces-version.xml.i
index b97971531..0a209bc3a 100644
--- a/interface-definitions/include/version/interfaces-version.xml.i
+++ b/interface-definitions/include/version/interfaces-version.xml.i
@@ -1,3 +1,3 @@
 <!-- include start from include/version/interfaces-version.xml.i -->
-<syntaxVersion component='interfaces' version='25'></syntaxVersion>
+<syntaxVersion component='interfaces' version='26'></syntaxVersion>
 <!-- include end -->
diff --git a/interface-definitions/interfaces-pppoe.xml.in b/interface-definitions/interfaces-pppoe.xml.in
index 3a0b7a40c..8cd8f8c86 100644
--- a/interface-definitions/interfaces-pppoe.xml.in
+++ b/interface-definitions/interfaces-pppoe.xml.in
@@ -21,31 +21,8 @@
           #include <include/interface/dial-on-demand.xml.i>
           #include <include/interface/interface-firewall.xml.i>
           #include <include/interface/interface-policy.xml.i>
-          <leafNode name="default-route">
-            <properties>
-              <help>Default route insertion behaviour</help>
-              <completionHelp>
-                <list>auto none force</list>
-              </completionHelp>
-              <constraint>
-                <regex>^(auto|none|force)$</regex>
-              </constraint>
-              <constraintErrorMessage>PPPoE default-route option must be 'auto', 'none', or 'force'</constraintErrorMessage>
-              <valueHelp>
-                <format>auto</format>
-                <description>Automatically install a default route</description>
-              </valueHelp>
-              <valueHelp>
-                <format>none</format>
-                <description>Do not install a default route</description>
-              </valueHelp>
-              <valueHelp>
-                <format>force</format>
-                <description>Replace existing default route</description>
-              </valueHelp>
-            </properties>
-            <defaultValue>auto</defaultValue>
-          </leafNode>
+          #include <include/interface/no-default-route.xml.i>
+          #include <include/interface/default-route-distance.xml.i>
           #include <include/interface/dhcpv6-options.xml.i>
           #include <include/interface/description.xml.i>
           #include <include/interface/disable.xml.i>
diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py
index df4c80f23..399ac6feb 100644
--- a/python/vyos/configdict.py
+++ b/python/vyos/configdict.py
@@ -331,49 +331,83 @@ def is_source_interface(conf, interface, intftype=None):
 def get_dhcp_interfaces(conf, vrf=None):
     """ Common helper functions to retrieve all interfaces from current CLI
     sessions that have DHCP configured. """
+    # Cache and reset config level
+    old_level = conf.get_level()
+    conf.set_level([])
+
     dhcp_interfaces = {}
     dict = conf.get_config_dict(['interfaces'], get_first_key=True)
     if not dict:
         return dhcp_interfaces
 
-    def check_dhcp(config, ifname):
+    def check_dhcp(config):
+        ifname = config['ifname']
         tmp = {}
-        if dict_search('address', config) == 'dhcp' or dict_search('default_route', config) != None:
+        if 'address' in config and 'dhcp' in config['address']:
             options = {}
             if dict_search('dhcp_options.default_route_distance', config) != None:
-                options.update({'distance' : config['dhcp_options']['default_route_distance']})
-            if dict_search('default_route', config) != None:
-                options.update({'distance' : config['default_route']})
+                options.update({'dhcp_options' : config['dhcp_options']})
             if 'vrf' in config:
                 if vrf is config['vrf']: tmp.update({ifname : options})
             else: tmp.update({ifname : options})
+
         return tmp
 
     for section, interface in dict.items():
         for ifname in interface:
-            # always reset config level
+            # always reset config level, as get_interface_dict() will alter it
             conf.set_level([])
             # we already have a dict representation of the config from get_config_dict(),
             # but with the extended information from get_interface_dict() we also
             # get the DHCP client default-route-distance default option if not specified.
             ifconfig = get_interface_dict(conf, ['interfaces', section], ifname)
 
-            tmp = check_dhcp(ifconfig, ifname)
+            tmp = check_dhcp(ifconfig)
             dhcp_interfaces.update(tmp)
             # check per VLAN interfaces
             for vif, vif_config in ifconfig.get('vif', {}).items():
-                tmp = check_dhcp(vif_config, f'{ifname}.{vif}')
+                tmp = check_dhcp(vif_config)
                 dhcp_interfaces.update(tmp)
             # check QinQ VLAN interfaces
-            for vif_s, vif_s_config in ifconfig.get('vif-s', {}).items():
-                tmp = check_dhcp(vif_s_config, f'{ifname}.{vif_s}')
+            for vif_s, vif_s_config in ifconfig.get('vif_s', {}).items():
+                tmp = check_dhcp(vif_s_config)
                 dhcp_interfaces.update(tmp)
-                for vif_c, vif_c_config in vif_s_config.get('vif-c', {}).items():
-                    tmp = check_dhcp(vif_c_config, f'{ifname}.{vif_s}.{vif_c}')
+                for vif_c, vif_c_config in vif_s_config.get('vif_c', {}).items():
+                    tmp = check_dhcp(vif_c_config)
                     dhcp_interfaces.update(tmp)
 
+    # reset old config level
     return dhcp_interfaces
 
+def get_pppoe_interfaces(conf, vrf=None):
+    """ Common helper functions to retrieve all interfaces from current CLI
+    sessions that have DHCP configured. """
+    # Cache and reset config level
+    old_level = conf.get_level()
+    conf.set_level([])
+
+    pppoe_interfaces = {}
+    for ifname in conf.list_nodes(['interfaces', 'pppoe']):
+        # always reset config level, as get_interface_dict() will alter it
+        conf.set_level([])
+        # we already have a dict representation of the config from get_config_dict(),
+        # but with the extended information from get_interface_dict() we also
+        # get the DHCP client default-route-distance default option if not specified.
+        ifconfig = get_interface_dict(conf, ['interfaces', 'pppoe'], ifname)
+
+        options = {}
+        if 'default_route_distance' in ifconfig:
+            options.update({'default_route_distance' : ifconfig['default_route_distance']})
+        if 'no_default_route' in ifconfig:
+            options.update({'no_default_route' : {}})
+        if 'vrf' in ifconfig:
+            if vrf is ifconfig['vrf']: pppoe_interfaces.update({ifname : options})
+        else: pppoe_interfaces.update({ifname : options})
+
+    # reset old config level
+    conf.set_level(old_level)
+    return pppoe_interfaces
+
 def get_interface_dict(config, base, ifname=''):
     """
     Common utility function to retrieve and mangle the interfaces configuration
diff --git a/python/vyos/ifconfig/pppoe.py b/python/vyos/ifconfig/pppoe.py
index 1d13264bf..63ffc8069 100644
--- a/python/vyos/ifconfig/pppoe.py
+++ b/python/vyos/ifconfig/pppoe.py
@@ -27,12 +27,13 @@ class PPPoEIf(Interface):
         },
     }
 
-    def _remove_routes(self, vrf=''):
+    def _remove_routes(self, vrf=None):
         # Always delete default routes when interface is removed
+        vrf_cmd = ''
         if vrf:
-            vrf = f'-c "vrf {vrf}"'
-        self._cmd(f'vtysh -c "conf t" {vrf} -c "no ip route 0.0.0.0/0 {self.ifname} tag 210"')
-        self._cmd(f'vtysh -c "conf t" {vrf} -c "no ipv6 route ::/0 {self.ifname} tag 210"')
+            vrf_cmd = f'-c "vrf {vrf}"'
+        self._cmd(f'vtysh -c "conf t" {vrf_cmd} -c "no ip route 0.0.0.0/0 {self.ifname} tag 210"')
+        self._cmd(f'vtysh -c "conf t" {vrf_cmd} -c "no ipv6 route ::/0 {self.ifname} tag 210"')
 
     def remove(self):
         """
@@ -44,11 +45,11 @@ class PPPoEIf(Interface):
         >>> i = Interface('pppoe0')
         >>> i.remove()
         """
-
+        vrf = None
         tmp = get_interface_config(self.ifname)
-        vrf = ''
         if 'master' in tmp:
-            self._remove_routes(tmp['master'])
+            vrf = tmp['master']
+        self._remove_routes(vrf)
 
         # remove bond master which places members in disabled state
         super().remove()
@@ -84,10 +85,12 @@ class PPPoEIf(Interface):
         self._config = config
 
         # remove old routes from an e.g. old VRF assignment
-        vrf = ''
-        if 'vrf_old' in config:
-            vrf = config['vrf_old']
-        self._remove_routes(vrf)
+        if 'shutdown_required':
+            vrf = None
+            tmp = get_interface_config(self.ifname)
+            if 'master' in tmp:
+                vrf = tmp['master']
+            self._remove_routes(vrf)
 
         # DHCPv6 PD handling is a bit different on PPPoE interfaces, as we do
         # not require an 'address dhcpv6' CLI option as with other interfaces
@@ -98,54 +101,15 @@ class PPPoEIf(Interface):
 
         super().update(config)
 
-        if 'default_route' not in config or config['default_route'] == 'none':
-            return
-
-        #
-        # Set default routes pointing to pppoe interface
-        #
-        vrf = ''
-        sed_opt = '^ip route'
-
-        install_v4 = True
-        install_v6 = True
-
         # generate proper configuration string when VRFs are in use
+        vrf = ''
         if 'vrf' in config:
             tmp = config['vrf']
             vrf = f'-c "vrf {tmp}"'
-            sed_opt = f'vrf {tmp}'
-
-        if config['default_route'] == 'auto':
-            # only add route if there is no default route present
-            tmp = self._cmd(f'vtysh -c "show running-config staticd no-header" | sed -n "/{sed_opt}/,/!/p"')
-            for line in tmp.splitlines():
-                line = line.lstrip()
-                if line.startswith('ip route 0.0.0.0/0'):
-                    install_v4 = False
-                    continue
-
-                if 'ipv6' in config and line.startswith('ipv6 route ::/0'):
-                    install_v6 = False
-                    continue
-
-        elif config['default_route'] == 'force':
-            # Force means that all static routes are replaced with the ones from this interface
-            tmp = self._cmd(f'vtysh -c "show running-config staticd no-header" | sed -n "/{sed_opt}/,/!/p"')
-            for line in tmp.splitlines():
-                if self.ifname in line:
-                    # It makes no sense to remove a route with our interface and the later re-add it.
-                    # This will only make traffic disappear - which is a no-no!
-                    continue
-
-                line = line.lstrip()
-                if line.startswith('ip route 0.0.0.0/0'):
-                    self._cmd(f'vtysh -c "conf t" {vrf} -c "no {line}"')
-
-                if 'ipv6' in config and line.startswith('ipv6 route ::/0'):
-                    self._cmd(f'vtysh -c "conf t" {vrf} -c "no {line}"')
-
-        if install_v4:
-            self._cmd(f'vtysh -c "conf t" {vrf} -c "ip route 0.0.0.0/0 {self.ifname} tag 210"')
-        if install_v6 and 'ipv6' in config:
-            self._cmd(f'vtysh -c "conf t" {vrf} -c "ipv6 route ::/0 {self.ifname} tag 210"')
+
+        if 'no_default_route' not in config:
+            # Set default route(s) pointing to PPPoE interface
+            distance = config['default_route_distance']
+            self._cmd(f'vtysh -c "conf t" {vrf} -c "ip route 0.0.0.0/0 {self.ifname} tag 210 {distance}"')
+            if 'ipv6' in config:
+                self._cmd(f'vtysh -c "conf t" {vrf} -c "ipv6 route ::/0 {self.ifname} tag 210 {distance}"')
diff --git a/smoketest/scripts/cli/test_interfaces_pppoe.py b/smoketest/scripts/cli/test_interfaces_pppoe.py
index 4f1e1ee99..16f6e542b 100755
--- a/smoketest/scripts/cli/test_interfaces_pppoe.py
+++ b/smoketest/scripts/cli/test_interfaces_pppoe.py
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2019-2021 VyOS maintainers and contributors
+# Copyright (C) 2019-2022 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
@@ -60,7 +60,6 @@ class PPPoEInterfaceTest(VyOSUnitTestSHIM.TestCase):
 
             self.cli_set(base_path + [interface, 'authentication', 'user', user])
             self.cli_set(base_path + [interface, 'authentication', 'password', passwd])
-            self.cli_set(base_path + [interface, 'default-route', 'auto'])
             self.cli_set(base_path + [interface, 'mtu', mtu])
             self.cli_set(base_path + [interface, 'no-peer-dns'])
 
@@ -136,7 +135,7 @@ class PPPoEInterfaceTest(VyOSUnitTestSHIM.TestCase):
         for interface in self._interfaces:
             self.cli_set(base_path + [interface, 'authentication', 'user', 'vyos'])
             self.cli_set(base_path + [interface, 'authentication', 'password', 'vyos'])
-            self.cli_set(base_path + [interface, 'default-route', 'none'])
+            self.cli_set(base_path + [interface, 'no-default-route'])
             self.cli_set(base_path + [interface, 'no-peer-dns'])
             self.cli_set(base_path + [interface, 'source-interface', self._source_interface])
             self.cli_set(base_path + [interface, 'ipv6', 'address', 'autoconf'])
diff --git a/src/conf_mode/interfaces-pppoe.py b/src/conf_mode/interfaces-pppoe.py
index bfb1fadd5..279369a32 100755
--- a/src/conf_mode/interfaces-pppoe.py
+++ b/src/conf_mode/interfaces-pppoe.py
@@ -22,7 +22,9 @@ from netifaces import interfaces
 
 from vyos.config import Config
 from vyos.configdict import get_interface_dict
+from vyos.configdict import is_node_changed
 from vyos.configdict import leaf_node_changed
+from vyos.configdict import get_pppoe_interfaces
 from vyos.configverify import verify_authentication
 from vyos.configverify import verify_source_interface
 from vyos.configverify import verify_interface_exists
@@ -52,28 +54,12 @@ def get_config(config=None):
     # We should only terminate the PPPoE session if critical parameters change.
     # All parameters that can be changed on-the-fly (like interface description)
     # should not lead to a reconnect!
-    tmp = leaf_node_changed(conf, ['access-concentrator'])
-    if tmp: pppoe.update({'shutdown_required': {}})
-
-    tmp = leaf_node_changed(conf, ['connect-on-demand'])
-    if tmp: pppoe.update({'shutdown_required': {}})
-
-    tmp = leaf_node_changed(conf, ['service-name'])
-    if tmp: pppoe.update({'shutdown_required': {}})
-
-    tmp = leaf_node_changed(conf, ['source-interface'])
-    if tmp: pppoe.update({'shutdown_required': {}})
-
-    tmp = leaf_node_changed(conf, ['vrf'])
-    # leaf_node_changed() returns a list, as VRF is a non-multi node, there
-    # will be only one list element
-    if tmp: pppoe.update({'vrf_old': tmp[0]})
-
-    tmp = leaf_node_changed(conf, ['authentication', 'user'])
-    if tmp: pppoe.update({'shutdown_required': {}})
-
-    tmp = leaf_node_changed(conf, ['authentication', 'password'])
-    if tmp: pppoe.update({'shutdown_required': {}})
+    for options in ['access-concentrator', 'connect-on-demand', 'service-name',
+                    'source-interface', 'vrf', 'no-default-route', 'authentication']:
+        if is_node_changed(conf, options):
+            pppoe.update({'shutdown_required': {}})
+            # bail out early - no need to further process other nodes
+            break
 
     return pppoe
 
@@ -120,7 +106,7 @@ def apply(pppoe):
         return None
 
     # reconnect should only be necessary when certain config options change,
-    # like ACS name, authentication, no-peer-dns, source-interface
+    # like ACS name, authentication ... (see get_config() for details)
     if ((not is_systemd_service_running(f'ppp@{ifname}.service')) or
         'shutdown_required' in pppoe):
 
@@ -130,6 +116,9 @@ def apply(pppoe):
             p.remove()
 
         call(f'systemctl restart ppp@{ifname}.service')
+        # When interface comes "live" a hook is called:
+        # /etc/ppp/ip-up.d/99-vyos-pppoe-callback
+        # which triggers PPPoEIf.update()
     else:
         if os.path.isdir(f'/sys/class/net/{ifname}'):
             p = PPPoEIf(ifname)
diff --git a/src/conf_mode/protocols_static.py b/src/conf_mode/protocols_static.py
index 87432bc1c..58e202928 100755
--- a/src/conf_mode/protocols_static.py
+++ b/src/conf_mode/protocols_static.py
@@ -22,6 +22,7 @@ from sys import argv
 from vyos.config import Config
 from vyos.configdict import dict_merge
 from vyos.configdict import get_dhcp_interfaces
+from vyos.configdict import get_pppoe_interfaces
 from vyos.configverify import verify_common_route_maps
 from vyos.configverify import verify_vrf
 from vyos.template import render_to_string
@@ -59,7 +60,9 @@ def get_config(config=None):
 
     # T3680 - get a list of all interfaces currently configured to use DHCP
     tmp = get_dhcp_interfaces(conf, vrf)
-    if tmp: static['dhcp'] = tmp
+    if tmp: static.update({'dhcp' : tmp})
+    tmp = get_pppoe_interfaces(conf, vrf)
+    if tmp: static.update({'pppoe' : tmp})
 
     return static
 
diff --git a/src/etc/ppp/ip-up.d/99-vyos-pppoe-callback b/src/etc/ppp/ip-up.d/99-vyos-pppoe-callback
index bb918a468..78ca09010 100755
--- a/src/etc/ppp/ip-up.d/99-vyos-pppoe-callback
+++ b/src/etc/ppp/ip-up.d/99-vyos-pppoe-callback
@@ -1,6 +1,6 @@
 #!/usr/bin/env python3
 #
-# Copyright (C) 2021 VyOS maintainers and contributors
+# Copyright (C) 2021-2022 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
@@ -28,7 +28,8 @@ from syslog import openlog
 from syslog import LOG_PID
 from syslog import LOG_INFO
 
-from vyos.configquery import ConfigTreeQuery
+from vyos.configquery import Config
+from vyos.configdict import get_interface_dict
 from vyos.ifconfig import PPPoEIf
 from vyos.util import read_file
 
@@ -50,10 +51,9 @@ dialer_pid = read_file(f'/var/run/{interface}.pid')
 openlog(ident=f'pppd[{dialer_pid}]', facility=LOG_INFO)
 syslog('executing ' + argv[0])
 
-conf = ConfigTreeQuery()
-pppoe = conf.get_config_dict(['interfaces', 'pppoe', argv[6]],
-                             get_first_key=True, key_mangling=('-', '_'))
-pppoe['ifname'] = argv[6]
+conf = Config()
+pppoe = get_interface_dict(conf, ['interfaces', 'pppoe'], argv[6])
 
+# Update the config
 p = PPPoEIf(pppoe['ifname'])
 p.update(pppoe)
diff --git a/src/migration-scripts/interfaces/25-to-26 b/src/migration-scripts/interfaces/25-to-26
new file mode 100755
index 000000000..a8936235e
--- /dev/null
+++ b/src/migration-scripts/interfaces/25-to-26
@@ -0,0 +1,54 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) 2022 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/>.
+
+# T4384: pppoe: replace default-route CLI option with common CLI nodes already
+#        present for DHCP
+
+from sys import argv
+
+from vyos.ethtool import Ethtool
+from vyos.configtree import ConfigTree
+
+if (len(argv) < 1):
+    print("Must specify file name!")
+    exit(1)
+
+file_name = argv[1]
+with open(file_name, 'r') as f:
+    config_file = f.read()
+
+base = ['interfaces', 'pppoe']
+config = ConfigTree(config_file)
+
+if not config.exists(base):
+    exit(0)
+
+for ifname in config.list_nodes(base):
+    tmp_config = base + [ifname, 'default-route']
+    if config.exists(tmp_config):
+        # Retrieve current config value
+        value = config.return_value(tmp_config)
+        # Delete old Config node
+        config.delete(tmp_config)
+        if value == 'none':
+            config.set(base + [ifname, 'no-default-route'])
+
+try:
+    with open(file_name, 'w') as f:
+        f.write(config.to_string())
+except OSError as e:
+    print(f'Failed to save the modified config: {e}')
+    exit(1)