Merge pull request #4753 from jestabro/config-save-sync-atomic

T7709: Add file sync and atomic write to config save script

7 files changed, 245 insertions, 17 deletions

diff --git a/debian/control b/debian/control
index c2128c819..c5db8f0e9 100644
--- a/debian/control
+++ b/debian/control
@@ -419,7 +419,8 @@ Architecture: all
 Depends:
  skopeo,
  snmp,
- vyos-1x
+ vyos-1x,
+ vmtouch
 Description: VyOS build sanity checking toolkit
 
 Package: vyos-1x-vmware
diff --git a/python/vyos/component_version.py b/python/vyos/component_version.py
index 136bd36e8..13fb8333f 100644
--- a/python/vyos/component_version.py
+++ b/python/vyos/component_version.py
@@ -209,6 +209,16 @@ def version_info_prune_component(x: VersionInfo, y: VersionInfo) -> VersionInfo:
     x.component = {k: v for k, v in x.component.items() if k in y.component}
 
 
+def add_system_version_string(config_str: str = None) -> str:
+    """Wrap config string with system version and return string."""
+    version_info = version_info_from_system()
+    if config_str is not None:
+        version_info.update_config_body(config_str)
+    version_info.update_footer()
+
+    return version_info.write_string()
+
+
 def add_system_version(config_str: str = None, out_file: str = None):
     """Wrap config string with system version and write to out_file.
 
diff --git a/python/vyos/configsession.py b/python/vyos/configsession.py
index 462a028bb..f0a22d930 100644
--- a/python/vyos/configsession.py
+++ b/python/vyos/configsession.py
@@ -392,13 +392,7 @@ class ConfigSession(object):
         return out
 
     def save_config(self, file_path):
-        if self._vyconf_session is None:
-            out = self.__run_command(SAVE_CONFIG + [file_path])
-        else:
-            out, _ = self._vyconf_session.save_config(
-                file=file_path, append_version=True
-            )
-
+        out = self.__run_command(SAVE_CONFIG + [file_path])
         return out
 
     def install_image(self, url):
diff --git a/python/vyos/utils/file.py b/python/vyos/utils/file.py
index 1e2de2b39..491cc6547 100644
--- a/python/vyos/utils/file.py
+++ b/python/vyos/utils/file.py
@@ -14,6 +14,8 @@
 # License along with this library.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
+import tempfile
+
 from vyos.utils.permission import chown
 
 def makedir(path, user=None, group=None):
@@ -185,3 +187,84 @@ def wait_for_file_write_complete(file_path, pre_hook=None, timeout=None, sleep_i
     """ Waits for a process to close a file after opening it in write mode. """
     wait_for_inotify(file_path,
       event_type='IN_CLOSE_WRITE', pre_hook=pre_hook, timeout=timeout, sleep_interval=sleep_interval)
+
+
+def copy_chown(source, target):
+    # pylint: disable=import-outside-toplevel
+    import shutil
+    import stat
+
+    shutil.copy2(source, target)
+    st = os.stat(source)
+    os.chown(target, st[stat.ST_UID], st[stat.ST_GID])
+
+
+def write_file_sync(file_path, data: str, mode='w'):
+    """Write file with explicit sync of file and directory"""
+    # pylint: disable=consider-using-with
+    file_dir = os.path.dirname(file_path)
+
+    # write and sync file
+    try:
+        file = open(file_path, mode)
+        file.write(data)
+        file.flush()
+        os.fsync(file.fileno())
+        file.close()
+    except OSError as e:
+        try:
+            file.close()
+        except OSError:
+            pass
+        raise e
+
+    # sync directory entry
+    try:
+        fd = os.open(file_dir, os.O_DIRECTORY | os.O_RDONLY)
+        os.fsync(fd)
+        os.close(fd)
+    except OSError as e:
+        try:
+            os.close(fd)
+        except OSError:
+            pass
+        raise e
+
+
+def write_file_atomic(file_path, data: str, mode='w'):
+    """Use os.rename for 'atomic' write.
+
+    Note that this requires an euid/egid of that of the source file for the
+    chown operation.
+
+    Note that this calls write_file_sync, above.
+    """
+    # pylint: disable=consider-using-with,raise-missing-from
+    file_dir = os.path.dirname(file_path)
+    temp_file = tempfile.NamedTemporaryFile(delete=False, dir=file_dir).name
+
+    def cleanup():
+        if os.path.exists(temp_file):
+            try:
+                os.unlink(temp_file)
+            except OSError:
+                pass
+
+    if os.path.exists(file_path):
+        try:
+            copy_chown(file_path, temp_file)
+        except OSError as e:
+            cleanup()
+            raise OSError(f'copy_chown {e}')
+
+    try:
+        write_file_sync(temp_file, data, mode=mode)
+    except OSError as e:
+        cleanup()
+        raise OSError(f'write_file_sync {e}')
+
+    try:
+        os.rename(temp_file, file_path)
+    except OSError as e:
+        cleanup()
+        raise OSError(f'rename {e}')
diff --git a/smoketest/scripts/cli/base_vyostest_shim.py b/smoketest/scripts/cli/base_vyostest_shim.py
index 526b24c46..56905c92a 100644
--- a/smoketest/scripts/cli/base_vyostest_shim.py
+++ b/smoketest/scripts/cli/base_vyostest_shim.py
@@ -106,6 +106,11 @@ class VyOSUnitTestSHIM:
 
             return out
 
+        def cli_save(self, file):
+            if self.debug:
+                print('save')
+            self._session.save_config(file)
+
         def op_mode(self, path : list) -> None:
             """
             Execute OP-mode command and return stdout
diff --git a/smoketest/scripts/cli/test_config_save.py b/smoketest/scripts/cli/test_config_save.py
new file mode 100755
index 000000000..f9f403a6e
--- /dev/null
+++ b/smoketest/scripts/cli/test_config_save.py
@@ -0,0 +1,101 @@
+#!/usr/bin/env python3
+#
+# Copyright VyOS maintainers and contributors <maintainers@vyos.io>
+#
+# 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/>.
+
+
+import os
+import unittest
+
+from vyos.utils.process import cmd
+from vyos.utils.config import read_saved_value
+from vyos.defaults import directories
+
+from base_vyostest_shim import VyOSUnitTestSHIM
+
+
+class TestConfigDep(VyOSUnitTestSHIM.TestCase):
+    def test_disk_resident(self):
+        config_file = os.path.join(directories['config'], 'config.boot')
+
+        evict_cmd = f'vmtouch -e {config_file}'
+        page_count_cmd = f'fincore -o PAGES -n {config_file}'
+
+        test_value = 'test_disk_resident'
+        test_path = ['interfaces', 'ethernet', 'eth3', 'description']
+
+        self.cli_set(test_path, value=test_value)
+        self.cli_commit()
+        self.cli_save(config_file)
+
+        cmd(evict_cmd)
+        # pages may be paged back into memory by the time the above
+        # completes (man vmtouch); either way, we read what is resident on
+        # disk. The following is just for curiosity:
+        pages = cmd(page_count_cmd)
+
+        saved_value = read_saved_value(test_path)
+
+        if self.debug:
+            print(f'vm pages on read config: {int(pages)}')
+
+        self.assertEqual(test_value, saved_value)
+
+        # clean up remaining
+        self.cli_delete(test_path)
+        self.cli_commit()
+        self.cli_save(config_file)
+
+    def test_disk_resident_atomic(self):
+        config_file = os.path.join(directories['config'], 'config.boot')
+
+        # save config will only call write_file_atomic if euid == 0:
+        # below is the command as invoked by CLI 'save'
+        save_cmd = (
+            'sudo sg vyattacfg "umask 0002; /usr/libexec/vyos/vyos-save-config.py"'
+        )
+
+        evict_cmd = f'vmtouch -e {config_file}'
+        page_count_cmd = f'fincore -o PAGES -n {config_file}'
+
+        test_value = 'test_disk_resident'
+        test_path = ['interfaces', 'ethernet', 'eth3', 'description']
+
+        self.cli_set(test_path, value=test_value)
+        self.cli_commit()
+        cmd(save_cmd)
+
+        cmd(evict_cmd)
+        # pages may be paged back into memory by the time the above
+        # completes (man vmtouch); either way, we read what is resident on
+        # disk. The following is just for curiosity:
+        pages = cmd(page_count_cmd)
+
+        saved_value = read_saved_value(test_path)
+
+        if self.debug:
+            print(f'vm pages on read config: {int(pages)}')
+
+        # check that we have at the least sync'd config;
+        # checking actual atomicity is a different matter ...
+        self.assertEqual(test_value, saved_value)
+
+        # clean up remaining
+        self.cli_delete(test_path)
+        self.cli_commit()
+        cmd(save_cmd)
+
+
+if __name__ == '__main__':
+    unittest.main(verbosity=2)
diff --git a/src/helpers/vyos-save-config.py b/src/helpers/vyos-save-config.py
index adf62b71d..208fb8ae8 100755
--- a/src/helpers/vyos-save-config.py
+++ b/src/helpers/vyos-save-config.py
@@ -23,15 +23,22 @@ from argparse import ArgumentParser
 
 from vyos.config import Config
 from vyos.remote import urlc
-from vyos.component_version import add_system_version
+from vyos.component_version import add_system_version_string
 from vyos.defaults import directories
+from vyos.utils.file import write_file
+from vyos.utils.file import write_file_sync
+from vyos.utils.file import write_file_atomic
+from vyos.utils.file import file_is_persistent
 
 DEFAULT_CONFIG_PATH = os.path.join(directories['config'], 'config.boot')
 remote_save = None
 
 parser = ArgumentParser(description='Save configuration')
 parser.add_argument('file', type=str, nargs='?', help='Save configuration to file')
-parser.add_argument('--write-json-file', type=str, help='Save JSON of configuration to file')
+parser.add_argument(
+    '--write-json-file', type=str, help='Save JSON of configuration to file'
+)
+
 args = parser.parse_args()
 file = args.file
 json_file = args.write_json_file
@@ -47,16 +54,43 @@ if re.match(r'\w+:/', save_file):
     except ValueError as e:
         sys.exit(e)
 
+
 config = Config()
 ct = config.get_config_tree(effective=True)
 
+# The effective config is None before boot configuration is complete.
+# Nothing to write, nor do we want to invite saving an empty string:
+# exit gracefully.
+if ct is None:
+    sys.exit()
+
+config_str = ct.to_string()
+versioned_config_str = add_system_version_string(config_str)
+
 # pylint: disable=consider-using-with
-write_file = save_file if remote_save is None else NamedTemporaryFile(delete=False).name
+file_to_write = (
+    save_file if remote_save is None else NamedTemporaryFile(delete=False).name
+)
 
-# config_tree is None before boot configuration is complete;
-# automated saves should check boot_configuration_complete
-config_str = None if ct is None else ct.to_string()
-add_system_version(config_str, write_file)
+if file_is_persistent(file_to_write):
+    if os.geteuid() == 0:
+        try:
+            write_file_atomic(file_to_write, versioned_config_str)
+        except OSError as e:
+            print(f'failed to write config file, write_file_atomic: {e}')
+            sys.exit(1)
+    else:
+        try:
+            write_file_sync(file_to_write, versioned_config_str)
+        except OSError as e:
+            print(f'failed to write config file, write_file_sync: {e}')
+            sys.exit(1)
+else:
+    try:
+        write_file(file_to_write, versioned_config_str)
+    except Exception as e:  # pylint: disable=broad-exception-caught
+        print(f'failed to write config file, write_file: {e}')
+        sys.exit(1)
 
 if json_file is not None and ct is not None:
     try:
@@ -67,6 +101,6 @@ if json_file is not None and ct is not None:
 
 if remote_save is not None:
     try:
-        remote_save.upload(write_file)
+        remote_save.upload(file_to_write)
     finally:
-        os.remove(write_file)
+        os.remove(file_to_write)