fixes saltstack/salt#62768 add atomic file operation for symlink changes

changelog/62768.added

@@ -0,0 +1 @@
+Add atomic file operation for symlink changes

salt/modules/file.py

@@ -3709,7 +3709,7 @@ def is_link(path):
     return os.path.islink(os.path.expanduser(path))
 
 
-def symlink(src, path, force=False):
+def symlink(src, path, force=False, atomic=False):
     """
     Create a symbolic link (symlink, soft link) to a file
 
@@ -3723,8 +3723,12 @@ def symlink(src, path, force=False):
             Overwrite an existing symlink with the same name
             .. versionadded:: 3005
 
+        atomic (bool):
+            Use atomic file operations to create the symlink
+            .. versionadded:: 3006.0
+
     Returns:
-        bool: True if successful, otherwise False
+        bool: ``True`` if successful, otherwise raises ``CommandExecutionError``
 
     CLI Example:
 
@@ -3734,6 +3738,9 @@ def symlink(src, path, force=False):
     """
     path = os.path.expanduser(path)
 
+    if not os.path.isabs(path):
+        raise SaltInvocationError("Link path must be absolute: {}".format(path))
+
     if os.path.islink(path):
         try:
             if os.path.normpath(salt.utils.path.readlink(path)) == os.path.normpath(
@@ -3744,18 +3751,32 @@ def symlink(src, path, force=False):
         except OSError:
             pass
 
-        if force:
+        if not force and not atomic:
-            os.unlink(path)
-        else:
             msg = "Found existing symlink: {}".format(path)
             raise CommandExecutionError(msg)
 
-    if os.path.exists(path):
+    if os.path.exists(path) and not force and not atomic:
         msg = "Existing path is not a symlink: {}".format(path)
         raise CommandExecutionError(msg)
 
-    if not os.path.isabs(path):
+    if (os.path.islink(path) or os.path.exists(path)) and force and not atomic:
-        raise SaltInvocationError("Link path must be absolute: {}".format(path))
+        os.unlink(path)
+    elif atomic:
+        link_dir = os.path.dirname(path)
+        retry = 0
+        while retry < 5:
+            temp_link = tempfile.mktemp(dir=link_dir)
+            try:
+                os.symlink(src, temp_link)
+                break
+            except FileExistsError:
+                retry += 1
+        try:
+            os.replace(temp_link, path)
+            return True
+        except OSError:
+            os.remove(temp_link)
+            raise CommandExecutionError("Could not create '{}'".format(path))
 
     try:
         os.symlink(src, path)

salt/states/file.py

@@ -1540,6 +1540,7 @@ def symlink(
     win_perms=None,
     win_deny_perms=None,
     win_inheritance=None,
+    atomic=False,
     **kwargs
 ):
     """
@@ -1618,17 +1619,22 @@ def symlink(
         True to inherit permissions from parent, otherwise False
 
         .. versionadded:: 2017.7.7
+
+    atomic
+        Use atomic file operation to create the symlink.
+
+        .. versionadded:: 3006.0
     """
     name = os.path.expanduser(name)
 
+    ret = {"name": name, "changes": {}, "result": True, "comment": ""}
+    if not name:
+        return _error(ret, "Must provide name to file.symlink")
+
     # Make sure that leading zeros stripped by YAML loader are added back
     mode = salt.utils.files.normalize_mode(mode)
 
     user = _test_owner(kwargs, user=user)
-    ret = {"name": name, "changes": {}, "result": True, "comment": ""}
-    if not name:
-        return _error(ret, "Must provide name to file.symlink")
-
     if user is None:
         user = __opts__["user"]
 
@@ -1752,12 +1758,9 @@ def symlink(
 
     if __salt__["file.is_link"](name):
         # The link exists, verify that it matches the target
-        if os.path.normpath(__salt__["file.readlink"](name)) != os.path.normpath(
+        if os.path.normpath(__salt__["file.readlink"](name)) == os.path.normpath(
             target
         ):
-            # The target is wrong, delete the link
-            os.remove(name)
-        else:
             if _check_symlink_ownership(name, user, group, win_owner):
                 # The link looks good!
                 if salt.utils.platform.is_windows():
@@ -1837,14 +1840,7 @@ def symlink(
                         name, backupname, exc
                     ),
                 )
-        elif force:
+        elif not force and not atomic:
-            # Remove whatever is in the way
-            if __salt__["file.is_link"](name):
-                __salt__["file.remove"](name)
-                ret["changes"]["forced"] = "Symlink was forcibly replaced"
-            else:
-                __salt__["file.remove"](name)
-        else:
             # Otherwise throw an error
             fs_entry_type = (
                 "File"
@@ -1858,26 +1854,24 @@ def symlink(
                 "{} exists where the symlink {} should be".format(fs_entry_type, name),
             )
 
-    if not os.path.exists(name):
+    try:
-        # The link is not present, make it
+        __salt__["file.symlink"](target, name, force=force, atomic=atomic)
-        try:
+    except (CommandExecutionError, OSError) as exc:
-            __salt__["file.symlink"](target, name)
+        ret["result"] = False
-        except OSError as exc:
+        ret["comment"] = "Unable to create new symlink {} -> {}: {}".format(
-            ret["result"] = False
+            name, target, exc
-            ret["comment"] = "Unable to create new symlink {} -> {}: {}".format(
+        )
-                name, target, exc
+        return ret
-            )
+    else:
-            return ret
+        ret["comment"] = "Created new symlink {} -> {}".format(name, target)
-        else:
+        ret["changes"]["new"] = name
-            ret["comment"] = "Created new symlink {} -> {}".format(name, target)
-            ret["changes"]["new"] = name
 
-        if not _check_symlink_ownership(name, user, group, win_owner):
+    if not _check_symlink_ownership(name, user, group, win_owner):
-            if not _set_symlink_ownership(name, user, group, win_owner):
+        if not _set_symlink_ownership(name, user, group, win_owner):
-                ret["result"] = False
+            ret["result"] = False
-                ret["comment"] += ", but was unable to set ownership to {}:{}".format(
+            ret["comment"] += ", but was unable to set ownership to {}:{}".format(
-                    user, group
+                user, group
-                )
+            )
     return ret
 
 

tests/pytests/functional/modules/file/test_symlink.py

@@ -120,3 +120,18 @@ def test_symlink_target_relative_path(file, source):
     with pytest.raises(SaltInvocationError) as exc:
         file.symlink(str(source), str(target))
     assert "Link path must be absolute" in exc.value.message
+
+
+def test_symlink_exists_different_atomic(file, source):
+    """
+    Test symlink with an existing symlink to a different file with atomic=True
+    Should replace the existing symlink with a new one to the correct location
+    """
+    dif_source = source.parent / "dif_source.txt"
+    target = source.parent / "symlink.lnk"
+    target.symlink_to(dif_source)
+    try:
+        file.symlink(str(source), str(target), atomic=True)
+        assert salt.utils.path.readlink(str(target)) == str(source)
+    finally:
+        target.unlink()