Merge pull request #3824 from vyos/mergify/bp/sagitta/pr-3813

configdep: T6559: fix regression in dependent script error under configd (backport #3813)

4 files changed, 82 insertions, 36 deletions

diff --git a/python/vyos/config.py b/python/vyos/config.py
index cca65f0eb..b7ee606a9 100644
--- a/python/vyos/config.py
+++ b/python/vyos/config.py
@@ -140,6 +140,7 @@ class Config(object):
 
         self._level = []
         self._dict_cache = {}
+        self.dependency_list = []
         (self._running_config,
          self._session_config) = self._config_source.get_configtree_tuple()
 
diff --git a/python/vyos/configdep.py b/python/vyos/configdep.py
index 73bd9ea96..e0fe1ddac 100644
--- a/python/vyos/configdep.py
+++ b/python/vyos/configdep.py
@@ -33,10 +33,9 @@ if typing.TYPE_CHECKING:
 dependency_dir = os.path.join(directories['data'],
                               'config-mode-dependencies')
 
-local_dependent_func: dict[str, list[typing.Callable]] = {}
+dependency_list: list[typing.Callable] = []
 
 DEBUG = False
-FORCE_LOCAL = False
 
 def debug_print(s: str):
     if DEBUG:
@@ -50,7 +49,8 @@ def canon_name_of_path(path: str) -> str:
     return canon_name(script)
 
 def caller_name() -> str:
-    return stack()[2].filename
+    filename = stack()[2].filename
+    return canon_name_of_path(filename)
 
 def name_of(f: typing.Callable) -> str:
     return f.__name__
@@ -107,46 +107,47 @@ def run_config_mode_script(script: str, config: 'Config'):
         mod.generate(c)
         mod.apply(c)
     except (VyOSError, ConfigError) as e:
-        raise ConfigError(repr(e))
+        raise ConfigError(str(e)) from e
 
 def def_closure(target: str, config: 'Config',
                 tagnode: typing.Optional[str] = None) -> typing.Callable:
     script = target + '.py'
     def func_impl():
-        if tagnode:
+        if tagnode is not None:
             os.environ['VYOS_TAGNODE_VALUE'] = tagnode
         run_config_mode_script(script, config)
+    tag_ext = f'_{tagnode}' if tagnode is not None else ''
+    func_impl.__name__ = f'{target}{tag_ext}'
     return func_impl
 
 def set_dependents(case: str, config: 'Config',
                    tagnode: typing.Optional[str] = None):
+    global dependency_list
+
+    dependency_list = config.dependency_list
+
     d = get_dependency_dict(config)
-    k = canon_name_of_path(caller_name())
-    tag_ext = f'_{tagnode}' if tagnode is not None else ''
-    if hasattr(config, 'dependent_func') and not FORCE_LOCAL:
-        dependent_func = getattr(config, 'dependent_func')
-        l = dependent_func.setdefault('vyos_configd', [])
-    else:
-        dependent_func = local_dependent_func
-        l = dependent_func.setdefault(k, [])
+    k = caller_name()
+    l = dependency_list
+
     for target in d[k][case]:
         func = def_closure(target, config, tagnode)
-        func.__name__ = f'{target}{tag_ext}'
         append_uniq(l, func)
-    debug_print(f'set_dependents: caller {k}, dependents {names_of(l)}')
 
-def call_dependents(dependent_func: dict = None):
-    k = canon_name_of_path(caller_name())
-    if dependent_func is None or FORCE_LOCAL:
-        dependent_func = local_dependent_func
-        l = dependent_func.get(k, [])
-    else:
-        l = dependent_func.get('vyos_configd', [])
-    debug_print(f'call_dependents: caller {k}, dependents {names_of(l)}')
+    debug_print(f'set_dependents: caller {k}, current dependents {names_of(l)}')
+
+def call_dependents():
+    k = caller_name()
+    l = dependency_list
+    debug_print(f'call_dependents: caller {k}, remaining dependents {names_of(l)}')
     while l:
         f = l.pop(0)
         debug_print(f'calling: {f.__name__}')
-        f()
+        try:
+            f()
+        except ConfigError as e:
+            s = f'dependent {f.__name__}: {str(e)}'
+            raise ConfigError(s) from e
 
 def called_as_dependent() -> bool:
     st = stack()[1:]
diff --git a/smoketest/scripts/cli/test_config_dependency.py b/smoketest/scripts/cli/test_config_dependency.py
new file mode 100755
index 000000000..14e88321a
--- /dev/null
+++ b/smoketest/scripts/cli/test_config_dependency.py
@@ -0,0 +1,49 @@
+#!/usr/bin/env python3
+# Copyright 2024 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/>.
+
+
+import unittest
+
+from base_vyostest_shim import VyOSUnitTestSHIM
+
+from vyos.configsession import ConfigSessionError
+
+
+class TestConfigDep(VyOSUnitTestSHIM.TestCase):
+    def test_configdep_error(self):
+        address_group = 'AG'
+        address = '192.168.137.5'
+        nat_base = ['nat', 'source', 'rule', '10']
+        interface = 'eth1'
+
+        self.cli_set(['firewall', 'group', 'address-group', address_group,
+                      'address', address])
+        self.cli_set(nat_base + ['outbound-interface', 'name', interface])
+        self.cli_set(nat_base + ['source', 'group', 'address-group', address_group])
+        self.cli_set(nat_base + ['translation', 'address', 'masquerade'])
+        self.cli_commit()
+
+        self.cli_delete(['firewall'])
+        # check error in call to dependent script (nat)
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        # clean up remaining
+        self.cli_delete(['nat'])
+        self.cli_commit()
+
+if __name__ == '__main__':
+    unittest.main(verbosity=2)
diff --git a/src/services/vyos-configd b/src/services/vyos-configd
index c89c486e5..a7306afd1 100755
--- a/src/services/vyos-configd
+++ b/src/services/vyos-configd
@@ -30,7 +30,6 @@ from vyos.defaults import directories
 from vyos.utils.boot import boot_configuration_complete
 from vyos.configsource import ConfigSourceString
 from vyos.configsource import ConfigSourceError
-from vyos.configdep import call_dependents
 from vyos.config import Config
 from vyos import ConfigError
 
@@ -134,7 +133,8 @@ def explicit_print(path, mode, msg):
     except OSError:
         logger.critical("error explicit_print")
 
-def run_script(script, config, args) -> int:
+def run_script(script_name, config, args) -> int:
+    script = conf_mode_scripts[script_name]
     script.argv = args
     config.set_level([])
     try:
@@ -143,8 +143,9 @@ def run_script(script, config, args) -> int:
         script.generate(c)
         script.apply(c)
     except ConfigError as e:
-        logger.critical(e)
-        explicit_print(session_out, session_mode, str(e))
+        s = f'{script_name}: {repr(e)}'
+        logger.error(s)
+        explicit_print(session_out, session_mode, s)
         return R_ERROR_COMMIT
     except Exception as e:
         logger.critical(e)
@@ -212,6 +213,7 @@ def process_node_data(config, data, last: bool = False) -> int:
 
     script_name = None
     args = []
+    config.dependency_list.clear()
 
     res = re.match(r'^(VYOS_TAGNODE_VALUE=[^/]+)?.*\/([^/]+).py(.*)', data)
     if res.group(1):
@@ -227,17 +229,10 @@ def process_node_data(config, data, last: bool = False) -> int:
     args.insert(0, f'{script_name}.py')
 
     if script_name not in include_set:
-        # call dependents now if last element of prio queue is run
-        # independent of configd
-        if last:
-            call_dependents(dependent_func=config.dependent_func)
         return R_PASS
 
     with stdout_redirected(session_out, session_mode):
-        result = run_script(conf_mode_scripts[script_name], config, args)
-
-    if last and result == R_SUCCESS:
-        call_dependents(dependent_func=config.dependent_func)
+        result = run_script(script_name, config, args)
 
     return result