configd: T9368: reconcile FRR after a DHCP lease without the commit socket

python/vyos/frrender.py

@@ -21,8 +21,10 @@
 Will fail early if the rendered configuration has any errors.
 """
 
+import fcntl
 import os
 
+from contextlib import contextmanager
 from copy import deepcopy
 from time import sleep
 
@@ -33,6 +35,7 @@
 from vyos.defaults import frr_debug_enable
 from vyos.utils.dict import dict_search
 from vyos.utils.dict import dict_set_nested
+from vyos.utils.file import read_file
 from vyos.utils.file import write_file
 from vyos.utils.process import rc_cmd
 from vyos.template import get_dhcp_router
@@ -44,6 +47,21 @@ def debug(message):
         return
     print(message)
 
+frr_config_file: str = '/run/frr/config/vyos.frr.conf'
+# Configuration of the last successful reload, for consumers which have no
+# cached configuration of their own
+frr_applied_config_file: str = '/run/frr/config/vyos.frr.applied.conf'
+frr_render_lock_file: str = '/run/vyos-frr-render.lock'
+
+@contextmanager
+def frr_render_lock():
+    """Serialize everything which renders FRR. The rendered configuration is a
+    single file handed to frr-reload.py, so a second renderer would rewrite it
+    while FRR is being reloaded from it."""
+    with open(frr_render_lock_file, 'w') as lock_file:
+        fcntl.lockf(lock_file, fcntl.LOCK_EX)
+        yield
+
 ERROR_RELOAD_TEST: str = 'The system encountered an error while rendering the ' \
     'new routing daemon configuration. To ensure network stability and avoid ' \
     'potential connectivity disruptions, the configuration was not applied!'
@@ -739,7 +757,7 @@ class FRRender:
     cached_config_dict = {}
     cached_dhcp_gateways = {}
     def __init__(self):
-        self._frr_conf = '/run/frr/config/vyos.frr.conf'
+        self._frr_conf = frr_config_file
 
     def generate(self, config_dict) -> None:
         """
@@ -927,6 +945,8 @@ def apply(self, count_max=5):
         if count >= count_max:
             raise ConfigError(emsg)
 
+        write_file(frr_applied_config_file, read_file(self._frr_conf))
+
         # frr-reload.py --reload has already saved the configuration to
         # /etc/frr/frr.conf (bind-mounted from /run/frr/config/frr.conf): it
         # does so whenever it is not run with --daemon. T3217 added a second

smoketest/scripts/cli/test_interfaces_pppoe.py

@@ -37,6 +37,9 @@
 veth_path: list = ['interfaces', 'virtual-ethernet']
 pppoe_server_path = ['service', 'pppoe-server']
 connect_timeout: int = 20
+# A SLAAC address only shows up once the peer answers the Router Solicitation.
+# That is best effort and takes its own time after the link itself is up
+autoconf_timeout: int = 60
 name_servers: list = ['1.1.1.1', '2.2.2.2']
 ipv4_pool: str = '100.64.0.0/18'
 ipv6_pool: str = '2001:db8:8000::/48'
@@ -90,6 +93,11 @@ def has_global_ipv6_address(interface) -> bool:
     return any(not IPv6Address(addr).is_link_local
                for addr in get_interface_addresses(interface, 'inet6'))
 
+def has_link_local_address(interface) -> bool:
+    """ Check if the interface got a link-local IPv6 address assigned """
+    return any(IPv6Address(addr).is_link_local
+               for addr in get_interface_addresses(interface, 'inet6'))
+
 # add a classmethod to setup a temporaray PPPoE server for "proper" validation
 class PPPoEInterfaceTest(VyOSUnitTestSHIM.TestCase):
     @classmethod
@@ -373,13 +381,21 @@ def test_pppoe_ipv6_link_local(self):
             self.assertTrue(wait_for_interface(interface),
                             msg=f'Interface {interface} not found after {connect_timeout} seconds!')
 
-            # The link-local address is assigned before the router
-            # solicitation is sent out - once a global IPv6 address is present
-            # we know we are not testing too early
-            self.assertTrue(wait_for(has_global_ipv6_address, interface,
+            # pppd assigns the address once IPV6CP is done, which is not implied
+            # by the interface being there - that already happens for IPCP
+            self.assertTrue(wait_for(has_link_local_address, interface,
                                      interval=0.250, timeout=connect_timeout),
-                            msg=f'Interface {interface} got no global IPv6 address!')
+                            msg=f'Interface {interface} got no link-local address!')
+
+        # An option which does not require a reconnect is applied to the
+        # established session within the commit itself. Any address VyOS adds on
+        # its own is thus on the interface by the time the commit returns - we
+        # are no longer racing the hooks called when the link came up
+        for interface in self._interfaces:
+            self.cli_set(base_path + [interface, 'description', 'T9060'])
+        self.cli_commit()
 
+        for interface in self._interfaces:
             link_local = [addr for addr in get_interface_addresses(interface, 'inet6')
                           if IPv6Address(addr).is_link_local]
 
@@ -395,6 +411,32 @@ def test_pppoe_ipv6_link_local(self):
             # Validate and verify assigned IP addresses
             self._verify_interface_address(interface)
 
+    def test_pppoe_ipv6_autoconf_global_address(self):
+        # A link with IPv6 autoconf picks up a global address from the Router
+        # Advertisement of the BRAS. This is the only test depending on the
+        # peer answering, thus it carries its own, more generous timeout
+        for interface in self._interfaces:
+            (user, passwd) = self.u_p_dict[interface]
+
+            self.cli_set(base_path + [interface, 'authentication', 'username', user])
+            self.cli_set(base_path + [interface, 'authentication', 'password', passwd])
+            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'])
+
+        self.cli_commit()
+
+        for interface in self._interfaces:
+            self.assertTrue(wait_for_interface(interface),
+                            msg=f'Interface {interface} not found after {connect_timeout} seconds!')
+
+            self.assertTrue(wait_for(has_global_ipv6_address, interface,
+                                     interval=0.250, timeout=autoconf_timeout),
+                            msg=f'Interface {interface} got no global IPv6 address!')
+
+            # The address must come from the pool of the BRAS
+            self._verify_interface_address(interface)
+
     def test_pppoe_ipv6_autoconf_without_router_advertisement(self):
         # T9354: When the link comes up with IPv6 autoconf enabled a Router
         # Solicitation is sent out. This is best effort - if the BRAS never

src/etc/dhcp/dhclient-enter-hooks.d/98-vyos-static-routes-dhclient-hook

@@ -28,6 +28,7 @@ fi
 # - RELEASE: lease released, remove routes
 # - STOP: dhclient stopped, remove routes
 if [ "$reason" == "PREINIT" ] || [ "$reason" == "EXPIRE" ] || [ "$reason" == "FAIL" ] || [ "$reason" == "RELEASE" ] || [ "$reason" == "STOP" ]; then
-    # Re-generate static routes config to remove routes that depend on this interface
-    sudo /usr/libexec/vyos/vyos-request-configd-update.py
+    # Reconcile the rendered FRR configuration - routes depending on this
+    # interface must be removed
+    sudo /usr/libexec/vyos/vyos-frr-render.py
 fi

src/etc/dhcp/dhclient-exit-hooks.d/98-vyos-static-routes-dhclient-hook

@@ -34,5 +34,5 @@ elif [ "$reason" != "BOUND" ] && [ "$reason" != "EXPIRE" ] && [ "$reason" != "RE
     return 0
 fi
 
-# Re-generate the static routes config
-sudo /usr/libexec/vyos/vyos-request-configd-update.py
+# Reconcile the rendered FRR configuration with the current lease
+sudo /usr/libexec/vyos/vyos-frr-render.py

src/etc/ppp/ip-up.d/99-vyos-pppoe-callback

@@ -26,6 +26,7 @@ from sys import exit
 from vyos.configquery import ConfigTreeQuery
 from vyos.configdict import get_interface_dict
 from vyos.ifconfig import PPPoEIf
+from vyos.utils.commit import wait_for_commit_lock
 
 # When the ppp link comes up, this script is called with the following
 # parameters
@@ -41,6 +42,12 @@ if (len(argv) < 7):
 
 interface = argv[6]
 
+# The session is re-established from within a commit and can come back up
+# before that commit finished. The configuration read below is the running
+# one, which until then still is the configuration from before the commit -
+# applying it would undo what is being committed right now
+wait_for_commit_lock()
+
 conf = ConfigTreeQuery()
 _, pppoe = get_interface_dict(conf.config, ['interfaces', 'pppoe'], interface)
 

src/helpers/vyos-frr-render.py

@@ -0,0 +1,58 @@
+#!/usr/bin/env python3
+#
+# Copyright (C) VyOS Inc.
+#
+# 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/>.
+
+"""Render the FRR configuration and reload FRR without vyos-configd.
+
+Only vyos-configd renders FRR, and only at the end of a commit it handled
+itself. Two paths need a render without it: a DHCP lease event, which changes
+the rendered configuration without any CLI change, and a commit which fell back
+to running the conf-mode scripts directly."""
+
+import sys
+
+from vyos.config import Config
+from vyos.frrender import FRRender
+from vyos.frrender import frr_applied_config_file
+from vyos.frrender import frr_config_file
+from vyos.frrender import frr_render_lock
+from vyos.frrender import get_frrender_dict
+from vyos.utils.file import read_file
+from vyos import ConfigError
+
+
+def render() -> None:
+    """Reload FRR if the rendered configuration differs from the one FRR was
+    last reloaded with.
+
+    A fresh instance has no cached configuration, thus its own change detection
+    can not be used here. The rendered file is no substitute for it - a render
+    which failed before the reload leaves it unapplied."""
+    frr = FRRender()
+    frr.generate(get_frrender_dict(Config()))
+
+    if read_file(frr_config_file) == read_file(frr_applied_config_file, None):
+        return
+
+    frr.apply()
+
+
+if __name__ == '__main__':
+    try:
+        with frr_render_lock():
+            render()
+    except ConfigError as e:
+        print(e)
+        sys.exit(1)

src/services/vyos-commitd

@@ -45,6 +45,7 @@ from vyos.configsource import ConfigSourceError
 from vyos.configdiff import get_commit_scripts
 from vyos.config import Config
 from vyos.frrender import FRRender
+from vyos.frrender import frr_render_lock
 from vyos.frrender import get_frrender_dict
 from vyos import ConfigError
 
@@ -293,11 +294,12 @@ def call_frr_render(frr, config):
     def _call_frr_render(frr, config):
         # pylint: disable=broad-exception-caught
         try:
-            tmp = get_frrender_dict(config)
-            if frr.generate(tmp):
-                # only apply a new FRR configuration if anything changed
-                # in comparison to the previous applied configuration
-                frr.apply()
+            with frr_render_lock():
+                tmp = get_frrender_dict(config)
+                if frr.generate(tmp):
+                    # only apply a new FRR configuration if anything changed
+                    # in comparison to the previous applied configuration
+                    frr.apply()
 
         except ConfigError as e:
             logger.error(e)

src/services/vyos-configd

@@ -39,6 +39,7 @@ from vyos.configsource import ConfigSourceError
 from vyos.configdiff import get_commit_scripts
 from vyos.config import Config
 from vyos.frrender import FRRender
+from vyos.frrender import frr_render_lock
 from vyos.frrender import get_frrender_dict
 from vyos import ConfigError
 from vyos.configmanager import ConfigManager
@@ -250,11 +251,12 @@ def call_frr_render(frr, config):
     def _call_frr_render(frr, config):
         # pylint: disable=broad-exception-caught
         try:
-            tmp = get_frrender_dict(config)
-            if frr.generate(tmp):
-                # only apply a new FRR configuration if anything changed
-                # in comparison to the previous applied configuration
-                frr.apply()
+            with frr_render_lock():
+                tmp = get_frrender_dict(config)
+                if frr.generate(tmp):
+                    # only apply a new FRR configuration if anything changed
+                    # in comparison to the previous applied configuration
+                    frr.apply()
 
         except ConfigError as e:
             logger.error(e)