diff --git a/changelog/64630.fixed.md b/changelog/64630.fixed.md new file mode 100644 index 000000000000..f49c58d4c2e0 --- /dev/null +++ b/changelog/64630.fixed.md @@ -0,0 +1,3 @@ +Fixed an intermittent issue with file.recurse where the state would +report failure even on success. Makes sure symlinks are created +after the target file is created diff --git a/salt/fileserver/roots.py b/salt/fileserver/roots.py index e81f37dcf029..cb27396b9790 100644 --- a/salt/fileserver/roots.py +++ b/salt/fileserver/roots.py @@ -325,7 +325,7 @@ def file_hash(load, fnd): def _file_lists(load, form): """ - Return a dict containing the file lists for files, dirs, emtydirs and symlinks + Return a dict containing the file lists for files, dirs, empty dirs and symlinks """ if "env" in load: # "env" is not supported; Use "saltenv". diff --git a/salt/states/file.py b/salt/states/file.py index 4adf6e12529b..6d8c43b02b08 100644 --- a/salt/states/file.py +++ b/salt/states/file.py @@ -282,6 +282,7 @@ def run(): import itertools import logging import os +import pathlib import posixpath import re import shutil @@ -557,7 +558,26 @@ def process_symlinks(filenames, symlinks): managed_directories.add(mdest) keep.add(mdest) - return managed_files, managed_directories, managed_symlinks, keep + # Sets are randomly ordered. We need to use a list so we can make sure + # symlinks are always at the end. This is necessary because the file must + # exist before we can create a symlink to it. See issue: + # https://github.com/saltstack/salt/issues/64630 + new_managed_files = list(managed_files) + # Now let's move all the symlinks to the end + for link_src_relpath, _ in managed_symlinks: + for file_dest, file_src in managed_files: + # We need to convert relpath to fullpath. We're using pathlib to + # be platform-agnostic + symlink_full_path = pathlib.Path(f"{name}{os.sep}{link_src_relpath}") + file_dest_full_path = pathlib.Path(file_dest) + if symlink_full_path == file_dest_full_path: + new_managed_files.append( + new_managed_files.pop( + new_managed_files.index((file_dest, file_src)) + ) + ) + + return new_managed_files, managed_directories, managed_symlinks, keep def _gen_keep_files(name, require, walk_d=None): @@ -4432,18 +4452,26 @@ def recurse( or immediate subdirectories keep_symlinks - Keep symlinks when copying from the source. This option will cause - the copy operation to terminate at the symlink. If desire behavior - similar to rsync, then set this to True. This option is not taken - in account if ``fileserver_followsymlinks`` is set to False. + + Determines how symbolic links (symlinks) are handled during the copying + process. When set to ``True``, the copy operation will copy the symlink + itself, rather than the file or directory it points to. When set to + ``False``, the operation will follow the symlink and copy the target + file or directory. If you want behavior similar to rsync, set this + option to ``True``. + + However, if the ``fileserver_followsymlinks`` option is set to ``False``, + the ``keep_symlinks`` setting will be ignored, and symlinks will not be + copied at all. force_symlinks - Force symlink creation. This option will force the symlink creation. - If a file or directory is obstructing symlink creation it will be - recursively removed so that symlink creation can proceed. This - option is usually not needed except in special circumstances. This - option is not taken in account if ``fileserver_followsymlinks`` is - set to False. + + Controls the creation of symlinks when using ``keep_symlinks``. When set + to ``True``, it forces the creation of symlinks by removing any existing + files or directories that might be obstructing their creation. This + removal is done recursively if a directory is blocking the symlink. This + option is only used when ``keep_symlinks`` is passed and is ignored if + ``fileserver_followsymlinks`` is set to ``False``. win_owner The owner of the symlink and directories if ``makedirs`` is True. If diff --git a/tests/pytests/functional/states/file/test_recurse.py b/tests/pytests/functional/states/file/test_recurse.py index c735d5128dac..9b69bbf5fffd 100644 --- a/tests/pytests/functional/states/file/test_recurse.py +++ b/tests/pytests/functional/states/file/test_recurse.py @@ -7,6 +7,60 @@ ] +@pytest.fixture(scope="module") +def symlink_scenario_1(state_tree): + # Create directory structure + dir_name = "symlink_scenario_1" + source_dir = state_tree / dir_name + if not source_dir.is_dir(): + source_dir.mkdir() + source_file = source_dir / "source_file.txt" + source_file.write_text("This is the source file...") + symlink_file = source_dir / "symlink" + symlink_file.symlink_to(source_file) + yield dir_name + + +@pytest.fixture(scope="module") +def symlink_scenario_2(state_tree): + # Create directory structure + dir_name = "symlink_scenario_2" + source_dir = state_tree / dir_name / "test" + if not source_dir.is_dir(): + source_dir.mkdir(parents=True) + test1 = source_dir / "test1" + test2 = source_dir / "test2" + test3 = source_dir / "test3" + test_link = source_dir / "test" + test1.touch() + test2.touch() + test3.touch() + test_link.symlink_to(test3) + yield dir_name + + +@pytest.fixture(scope="module") +def symlink_scenario_3(state_tree): + # Create directory structure + dir_name = "symlink_scenario_3" + source_dir = state_tree / dir_name + if not source_dir.is_dir(): + source_dir.mkdir(parents=True) + # Create a file with the same name but is not a symlink + source_file = source_dir / "not_a_symlink" / "symlink" + source_file.parent.mkdir(parents=True) + source_file.write_text("This is the source file...") + # Create other fluff files + just_a_file = source_dir / "just_a_file.txt" + just_a_file.touch() + dummy_file = source_dir / "notasymlink" + dummy_file.touch() + # Create symlink to source with the same name + symlink_file = source_dir / "symlink" + symlink_file.symlink_to(source_file) + yield dir_name + + @pytest.mark.parametrize("test", (False, True)) def test_recurse(file, tmp_path, grail, test): """ @@ -249,3 +303,148 @@ def test_issue_2726_mode_kwarg(modules, tmp_path, state_tree): ret = modules.state.template_str("\n".join(good_template)) for state_run in ret: assert state_run.result is True + + +def test_issue_64630_keep_symlinks_true(file, symlink_scenario_1, tmp_path): + """ + Make sure that symlinks are created and that there isn't an error when there + are no conflicting target files + """ + target_dir = tmp_path / symlink_scenario_1 # Target for the file.recurse state + target_file = target_dir / "source_file.txt" + target_symlink = target_dir / "symlink" + + ret = file.recurse( + name=str(target_dir), source=f"salt://{target_dir.name}", keep_symlinks=True + ) + assert ret.result is True + + assert target_dir.exists() + assert target_file.is_file() + assert target_symlink.is_symlink() + + +def test_issue_64630_keep_symlinks_false(file, symlink_scenario_1, tmp_path): + """ + Make sure that symlinks are created as files and that there isn't an error + """ + target_dir = tmp_path / symlink_scenario_1 # Target for the file.recurse state + target_file = target_dir / "source_file.txt" + target_symlink = target_dir / "symlink" + + ret = file.recurse( + name=str(target_dir), source=f"salt://{target_dir.name}", keep_symlinks=False + ) + assert ret.result is True + + assert target_dir.exists() + assert target_file.is_file() + assert target_symlink.is_file() + assert target_file.read_text() == target_symlink.read_text() + + +def test_issue_64630_keep_symlinks_conflicting_force_symlinks_false( + file, symlink_scenario_1, tmp_path +): + """ + Make sure that symlinks are not created when there is a conflict. The state + should return False + """ + target_dir = tmp_path / symlink_scenario_1 # Target for the file.recurse state + target_file = target_dir / "source_file.txt" + target_symlink = target_dir / "symlink" + + # Create the conflicting file + target_symlink.parent.mkdir(parents=True) + target_symlink.touch() + assert target_symlink.is_file() + + ret = file.recurse( + name=str(target_dir), + source=f"salt://{target_dir.name}", + keep_symlinks=True, + force_symlinks=False, + ) + # We expect it to fail + assert ret.result is False + + # And files not to be created properly + assert target_dir.exists() + assert target_file.is_file() + assert target_symlink.is_file() + + +def test_issue_64630_keep_symlinks_conflicting_force_symlinks_true( + file, symlink_scenario_1, tmp_path +): + """ + Make sure that symlinks are created when there is a conflict with an + existing file. + """ + target_dir = tmp_path / symlink_scenario_1 # Target for the file.recurse state + target_file = target_dir / "source_file.txt" + target_symlink = target_dir / "symlink" + + # Create the conflicting file + target_symlink.parent.mkdir(parents=True) + target_symlink.touch() + assert target_symlink.is_file() + + ret = file.recurse( + name=str(target_dir), + source=f"salt://{target_dir.name}", + force_symlinks=True, + keep_symlinks=True, + ) + assert ret.result is True + + assert target_dir.exists() + assert target_file.is_file() + assert target_symlink.is_symlink() + + +def test_issue_64630_keep_symlinks_similar_names(file, symlink_scenario_3, tmp_path): + """ + Make sure that symlinks are created when there is a file that shares part + of the name of the actual symlink file. I'm not sure what I'm testing here + as I couldn't really get this to fail either way + """ + target_dir = tmp_path / symlink_scenario_3 # Target for the file.recurse state + # symlink target, but has the same name as the symlink itself + target_source = target_dir / "not_a_symlink" / "symlink" + target_symlink = target_dir / "symlink" + decoy_file = target_dir / "notasymlink" + just_a_file = target_dir / "just_a_file.txt" + + ret = file.recurse( + name=str(target_dir), source=f"salt://{target_dir.name}", keep_symlinks=True + ) + assert ret.result is True + + assert target_dir.exists() + assert target_source.is_file() + assert decoy_file.is_file() + assert just_a_file.is_file() + assert target_symlink.is_symlink() + + +def test_issue_62117(file, symlink_scenario_2, tmp_path): + target_dir = tmp_path / symlink_scenario_2 / "test" + target_file_1 = target_dir / "test1" + target_file_2 = target_dir / "test2" + target_file_3 = target_dir / "test3" + target_symlink = target_dir / "test" + + ret = file.recurse( + name=str(target_dir), + source=f"salt://{target_dir.parent.name}/test", + clean=True, + keep_symlinks=True, + ) + assert ret.result is True + + assert target_dir.exists() + assert target_file_1.is_file() + assert target_file_2.is_file() + assert target_file_3.is_file() + assert target_symlink.is_symlink() diff --git a/tests/pytests/unit/states/file/test_recurse.py b/tests/pytests/unit/states/file/test_recurse.py new file mode 100644 index 000000000000..53e6e0fd22f1 --- /dev/null +++ b/tests/pytests/unit/states/file/test_recurse.py @@ -0,0 +1,48 @@ +import logging +import os +import pathlib + +import pytest + +import salt.states.file as filestate +from tests.support.mock import MagicMock, patch + +log = logging.getLogger(__name__) + + +@pytest.fixture +def configure_loader_modules(): + return {filestate: {"__salt__": {}, "__opts__": {}, "__env__": "base"}} + + +def test__gen_recurse_managed_files(): + """ + Test _gen_recurse_managed_files to make sure it puts symlinks at the end of the list of files. + """ + target_dir = pathlib.Path(f"{os.sep}some{os.sep}path{os.sep}target") + cp_list_master = MagicMock( + return_value=[ + "target/symlink", + "target/just_a_file.txt", + "target/not_a_symlink/symlink", + "target/notasymlink", + ], + ) + cp_list_master_symlinks = MagicMock( + return_value={ + "target/symlink": f"{target_dir}{os.sep}not_a_symlink{os.sep}symlink" + } + ) + patch_salt = { + "cp.list_master": cp_list_master, + "cp.list_master_symlinks": cp_list_master_symlinks, + } + with patch.dict(filestate.__salt__, patch_salt): + files, dirs, links, keep = filestate._gen_recurse_managed_files( + name=str(target_dir), source=f"salt://{target_dir.name}", keep_symlinks=True + ) + expected = ( + f"{os.sep}some{os.sep}path{os.sep}target{os.sep}symlink", + "salt://target/symlink?saltenv=base", + ) + assert files[-1] == expected