diff --git a/python/vyos/frrender.py b/python/vyos/frrender.py index eac03e4d37..371ea95b51 100644 --- a/python/vyos/frrender.py +++ b/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!' @@ -747,7 +765,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: """ @@ -935,6 +953,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 diff --git a/smoketest/scripts/cli/test_interfaces_pppoe.py b/smoketest/scripts/cli/test_interfaces_pppoe.py index cadea554dd..6514e52d24 100755 --- a/smoketest/scripts/cli/test_interfaces_pppoe.py +++ b/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 diff --git a/src/etc/dhcp/dhclient-enter-hooks.d/98-vyos-static-routes-dhclient-hook b/src/etc/dhcp/dhclient-enter-hooks.d/98-vyos-static-routes-dhclient-hook index 439a8dd871..f3b1b35f17 100755 --- a/src/etc/dhcp/dhclient-enter-hooks.d/98-vyos-static-routes-dhclient-hook +++ b/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 diff --git a/src/etc/dhcp/dhclient-exit-hooks.d/98-vyos-static-routes-dhclient-hook b/src/etc/dhcp/dhclient-exit-hooks.d/98-vyos-static-routes-dhclient-hook index 4f207411e3..2c895ffd6f 100755 --- a/src/etc/dhcp/dhclient-exit-hooks.d/98-vyos-static-routes-dhclient-hook +++ b/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 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 f3efb9ac06..9decfcbddd 100755 --- a/src/etc/ppp/ip-up.d/99-vyos-pppoe-callback +++ b/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) diff --git a/src/helpers/vyos-frr-render.py b/src/helpers/vyos-frr-render.py new file mode 100755 index 0000000000..739b56410a --- /dev/null +++ b/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 . + +"""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) diff --git a/src/helpers/vyos-request-configd-update.py b/src/helpers/vyos-request-configd-update.py deleted file mode 100755 index 55ed0eef9c..0000000000 --- a/src/helpers/vyos-request-configd-update.py +++ /dev/null @@ -1,31 +0,0 @@ -#!/usr/bin/env python3 - -import json -import zmq - -from vyos.utils.commit import wait_for_commit_lock -from vyos.defaults import vyos_configd_socket_path - -context = zmq.Context() - -request = { - 'type': 'node', - 'last': True, - 'data': '/usr/libexec/vyos/conf_mode/protocols_static.py', -} -request = json.dumps(request) - -print("Waiting for commit lock...") -wait_for_commit_lock() - -print("Connecting to vyos-configd server...") -socket = context.socket(zmq.REQ) -socket.connect(vyos_configd_socket_path) - -print(f"Sending request {request}...") -socket.send_string(request) - -message = socket.recv() -print(f"Received reply {request} [ {message} ]") - -print("All done") diff --git a/src/services/vyos-commitd b/src/services/vyos-commitd index b8e430b930..8a515446bd 100755 --- a/src/services/vyos-commitd +++ b/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) diff --git a/src/services/vyos-configd b/src/services/vyos-configd index 32d773f782..8e0b1adecf 100755 --- a/src/services/vyos-configd +++ b/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) diff --git a/src/shim/vyshim.c b/src/shim/vyshim.c index 7b516cd46a..81a1f8b49a 100644 --- a/src/shim/vyshim.c +++ b/src/shim/vyshim.c @@ -53,6 +53,8 @@ #define COMMIT_MARKER "/var/tmp/initial_in_commit" #define QUEUE_MARKER "/var/tmp/last_in_queue" +#define FRR_RENDER "/usr/libexec/vyos/vyos-frr-render.py" + enum { SUCCESS = 1 << 0, ERROR_COMMIT = 1 << 1, @@ -66,6 +68,7 @@ volatile int timeout = 0; int initialization(void *, char *); int pass_through(char **, int); +int frr_render(char *); void timer_handler(int); void leave_hint(char *); @@ -107,6 +110,13 @@ int main(int argc, char* argv[]) strsep(&pid_str, "_"); debug_print("config session pid: %s\n", pid_str); + // Consume the marker before talking to the daemon, else it is left behind + // for the next commit when the daemon can not be used here + if (access(QUEUE_MARKER, F_OK) != -1) { + last = 1; + remove(QUEUE_MARKER); + } + if (access(COMMIT_MARKER, F_OK) != -1) { init_timeout = initialization(requester, pid_str); if (!init_timeout) remove(COMMIT_MARKER); @@ -115,14 +125,12 @@ int main(int argc, char* argv[]) // if initial communication failed, pass through execution of script if (init_timeout) { int ret = pass_through(argv, ex_index); + // The conf-mode scripts do not render FRR, only the daemon does - + // without this the FRR reload of this commit is silently skipped + if (last && frr_render(pid_str) != 0) ret = -1; return ret; } - if (access(QUEUE_MARKER, F_OK) != -1) { - last = 1; - remove(QUEUE_MARKER); - } - char error_code[1]; debug_print("Sending node data ...\n"); char *string_node_data_msg = mkjson(MKJSON_OBJ, 3, @@ -159,6 +167,11 @@ int main(int argc, char* argv[]) ret = pass_through(argv, ex_index); } + // The daemon renders FRR only for a last script it handled itself + if (last && (err & (PASS | ERROR_DAEMON))) { + if (frr_render(pid_str) != 0) ret = -1; + } + if (err & ERROR_COMMIT) { debug_print("Received ERROR_COMMIT\n"); ret = -1; @@ -344,6 +357,46 @@ int pass_through(char **argv, int ex_index) return 0; } +int frr_render(char *pid_val) +{ + pid_t child_pid; + int status; + + // Nothing to fall back to, e.g. during a partial package upgrade + if (access(FRR_RENDER, X_OK) == -1) { + debug_print("%s is not available\n", FRR_RENDER); + return 0; + } + + debug_print("rendering FRR configuration without the daemon\n"); + + if ((child_pid=fork()) < 0) { + debug_print("frr_render fork() failed\n"); + return -1; + } else if (child_pid == 0) { + char *newargv[] = { FRR_RENDER, NULL }; + if (-1 == execv(FRR_RENDER, newargv)) { + fprintf(stderr, "frr_render execv failed %s: %s\n", + FRR_RENDER, strerror(errno)); + exit(EXIT_FAILURE); + } + } + + if (waitpid(child_pid, &status, 0) != child_pid) { + debug_print("frr_render waitpid() failed\n"); + return -1; + } + + if (WIFEXITED(status) && WEXITSTATUS(status) == 0) { + return 0; + } + + // Same treatment as an apply error reported by the daemon + leave_hint(pid_val); + + return -1; +} + void timer_handler(int signum) { debug_print("timer_handler invoked\n");