Merge branch '3006.x' into actions_nightly_bld

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

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".

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

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()

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