pppoe: T9395: render default routes via FRR only

Deleting a PPPoE interface shortly after its session came up could fail the
commit with "% could not lock candidate DS". The default routes were installed
via vtysh both from the commit and from the asynchronous ppp ip-up hook, so the
two FRR sessions raced for the mgmtd lock. The IPv6 default route was also
missing from the rendered FRR configuration, so an FRR reload could remove it.

Both default routes are now part of the rendered FRR configuration, which
vyos-configd applies at the end of every commit. PPPoE no longer calls vtysh.

4 files changed, 28 insertions, 40 deletions

diff --git a/data/templates/frr/staticd.frr.j2 b/data/templates/frr/staticd.frr.j2
index 18d300dae..0530b274b 100644
--- a/data/templates/frr/staticd.frr.j2
+++ b/data/templates/frr/staticd.frr.j2
@@ -90,10 +90,14 @@ vrf {{ vrf }}
 {%         endif %}
 {%     endfor %}
 {% endif %}
-{# IPv4 default routes from PPPoE interfaces #}
+{# IPv4 and IPv6 default routes from PPPoE interfaces #}
 {% if pppoe is vyos_defined %}
 {%     for interface, interface_config in pppoe.items() if interface_config.no_default_route is not vyos_defined %}
-{{ 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 }}
+{%         set distance = interface_config.default_route_distance if interface_config.default_route_distance is vyos_defined else '' %}
+{{ ip_prefix }} route 0.0.0.0/0 {{ interface }} tag 210 {{ distance }}
+{%         if interface_config.ipv6 is vyos_defined %}
+{{ ipv6_prefix }} route ::/0 {{ interface }} tag 210 {{ distance }}
+{%         endif %}
 {%     endfor %}
 {% endif %}
 {# IPv6 routing #}
diff --git a/python/vyos/configdict.py b/python/vyos/configdict.py
index da2e9792c..d316ece3b 100644
--- a/python/vyos/configdict.py
+++ b/python/vyos/configdict.py
@@ -453,6 +453,9 @@ def get_pppoe_interfaces(conf, vrf=None):
             options.update({'default_route_distance' : ifconfig['default_route_distance']})
         if 'no_default_route' in ifconfig:
             options.update({'no_default_route' : {}})
+        # IPv6 default route is only installed when IPv6 is enabled on the link
+        if 'ipv6' in ifconfig:
+            options.update({'ipv6' : {}})
         if 'vrf' in ifconfig:
             if vrf == ifconfig['vrf']: pppoe_interfaces.update({ifname : options})
         else:
diff --git a/python/vyos/ifconfig/pppoe.py b/python/vyos/ifconfig/pppoe.py
index 413c75023..cbd77b178 100644
--- a/python/vyos/ifconfig/pppoe.py
+++ b/python/vyos/ifconfig/pppoe.py
@@ -19,7 +19,6 @@ from vyos.utils.assertion import assert_range
 from vyos.utils.dict import dict_search
 from vyos.utils.process import cmdl
 from vyos.utils.process import get_wrapper
-from vyos.utils.network import get_interface_config
 from vyos.utils.network import mac2eui64
 
 @Interface.register
@@ -51,31 +50,6 @@ class PPPoEIf(Interface):
         },
     }}
 
-    def _remove_routes(self, vrf=None):
-        # Always delete default routes when interface is removed
-        vrf_cmd = ['-c', f'vrf {vrf}'] if vrf else []
-        self._cmdl(['vtysh', '-c', 'conf t'] + vrf_cmd + ['-c', f'no ip route 0.0.0.0/0 {self.ifname} tag 210'])
-        self._cmdl(['vtysh', '-c', 'conf t'] + vrf_cmd + ['-c', f'no ipv6 route ::/0 {self.ifname} tag 210'])
-
-    def remove(self):
-        """
-        Remove interface from operating system. Removing the interface
-        deconfigures all assigned IP addresses and clear possible DHCP(v6)
-        client processes.
-        Example:
-        >>> from vyos.ifconfig import Interface
-        >>> i = Interface('pppoe0')
-        >>> i.remove()
-        """
-        vrf = None
-        tmp = get_interface_config(self.ifname)
-        if 'master' in tmp:
-            vrf = tmp['master']
-        self._remove_routes(vrf)
-
-        # remove bond master which places members in disabled state
-        super().remove()
-
     def _create(self):
         # we cannot create this interface as it is managed outside
         pass
@@ -141,22 +115,14 @@ class PPPoEIf(Interface):
 
         super().update(config)
 
-        # generate proper configuration string when VRFs are in use
-        vrf = []
-        if 'vrf' in config:
-            tmp = config['vrf']
-            vrf = ['-c', f'vrf {tmp}']
+        vrf = config.get('vrf')
 
         # learn default router in Router Advertisement.
         tmp = '0' if 'no_default_route' in config else '1'
         self.set_accept_ra_defrtr(tmp)
 
-        if 'no_default_route' not in config:
-            # Set default route(s) pointing to PPPoE interface
-            distance = config['default_route_distance']
-            self._cmdl(['vtysh', '-c', 'conf t'] + vrf + ['-c', f'ip route 0.0.0.0/0 {self.ifname} tag 210 {distance}'])
-            if 'ipv6' in config:
-                self._cmdl(['vtysh', '-c', 'conf t'] + vrf + ['-c', f'ipv6 route ::/0 {self.ifname} tag 210 {distance}'])
+        # Default route(s) pointing to the PPPoE interface are rendered by
+        # vyos.frrender.get_pppoe_interfaces() at the end of every commit
 
         # Kick a Router Solicitation when IPv6 is up. This is best effort -
         # the peer may answer late or not at all
@@ -164,7 +130,7 @@ class PPPoEIf(Interface):
             # systemd-run(1) only asks PID 1 to start the transient unit, so
             # rdisc6(8) does not inherit our VRF context - it has to be entered
             # inside the unit itself
-            wrapper = get_wrapper(config['vrf'] if 'vrf' in config else None, None)
+            wrapper = get_wrapper(vrf, None)
             description = f'VyOS IPv6 Router Solicitation on {self.ifname}'
             cmdl(['systemd-run', '--quiet', '--collect',
                   f'--description={description}'] + wrapper +
diff --git a/smoketest/scripts/cli/test_interfaces_pppoe.py b/smoketest/scripts/cli/test_interfaces_pppoe.py
index 67608ced2..f9a764b7a 100755
--- a/smoketest/scripts/cli/test_interfaces_pppoe.py
+++ b/smoketest/scripts/cli/test_interfaces_pppoe.py
@@ -537,6 +537,8 @@ class PPPoEInterfaceTest(VyOSUnitTestSHIM.TestCase):
         # T6991/T9054: The PPPoE default route must not be withdrawn from FRR
         # when "protocols static" is deleted - only the statically configured
         # routes must disappear, the PPPoE-sourced default route must stay.
+        # Both IPv4 and IPv6 default routes are rendered by vyos.frrender, so
+        # they also survive any other FRR re-render.
         interface = self._interfaces[0]
         (user, passwd) = self.u_p_dict[interface]
         static_base_path = ['protocols', 'static']
@@ -544,14 +546,17 @@ class PPPoEInterfaceTest(VyOSUnitTestSHIM.TestCase):
         self.cli_set(base_path + [interface, 'authentication', 'username', user])
         self.cli_set(base_path + [interface, 'authentication', 'password', passwd])
         self.cli_set(base_path + [interface, 'source-interface', self._source_interface])
+        self.cli_set(base_path + [interface, 'ipv6', 'address', 'autoconf'])
         self.cli_commit()
 
         self.assertTrue(wait_for_interface(interface),
                         msg=f'Interface {interface} not found after {connect_timeout} seconds!')
 
         default_route = rf'ip route 0.0.0.0/0 {interface} tag 210'
+        default_route6 = rf'ipv6 route ::/0 {interface} tag 210'
         frrconfig = self.getFRRconfig('')
         self.assertIn(default_route, frrconfig)
+        self.assertIn(default_route6, frrconfig)
 
         # Add an unrelated static route - this is what triggers "protocols
         # static" to exist on the CLI in the first place
@@ -560,6 +565,7 @@ class PPPoEInterfaceTest(VyOSUnitTestSHIM.TestCase):
 
         frrconfig = self.getFRRconfig('')
         self.assertIn(default_route, frrconfig)
+        self.assertIn(default_route6, frrconfig)
         self.assertIn(r'ip route 10.0.0.0/8 blackhole', frrconfig)
 
         # Now delete "protocols static" entirely - the PPPoE default route
@@ -570,6 +576,15 @@ class PPPoEInterfaceTest(VyOSUnitTestSHIM.TestCase):
         frrconfig = self.getFRRconfig('')
         self.assertNotIn(r'ip route 10.0.0.0/8 blackhole', frrconfig)
         self.assertIn(default_route, frrconfig)
+        self.assertIn(default_route6, frrconfig)
+
+        # Deleting the PPPoE interface withdraws both default routes
+        self.cli_delete(base_path + [interface])
+        self.cli_commit()
+
+        frrconfig = self.getFRRconfig('')
+        self.assertNotIn(default_route, frrconfig)
+        self.assertNotIn(default_route6, frrconfig)
 
 if __name__ == '__main__':
     unittest.main(verbosity=2, failfast=VyOSUnitTestSHIM.TestCase.debug_on())