diff --git a/changelog/62768.added b/changelog/62768.added new file mode 100644 index 00000000000..85cf2b6b345 --- /dev/null +++ b/changelog/62768.added @@ -0,0 +1 @@ +Add atomic file operation for symlink changes diff --git a/salt/modules/file.py b/salt/modules/file.py index 13edc0dc5c1..f19e30c2e0a 100644 --- a/salt/modules/file.py +++ b/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: - os.unlink(path) - else: + if not force and not atomic: 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): - raise SaltInvocationError("Link path must be absolute: {}".format(path)) + if (os.path.islink(path) or os.path.exists(path)) and force and not atomic: + 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) diff --git a/salt/states/file.py b/salt/states/file.py index f68b87b6e1a..5dc8940d805 100644 --- a/salt/states/file.py +++ b/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: - # 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: + elif not force and not atomic: # 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): - # The link is not present, make it - try: - __salt__["file.symlink"](target, name) - except OSError as exc: - ret["result"] = False - ret["comment"] = "Unable to create new symlink {} -> {}: {}".format( - name, target, exc - ) - return ret - else: - ret["comment"] = "Created new symlink {} -> {}".format(name, target) - ret["changes"]["new"] = name + try: + __salt__["file.symlink"](target, name, force=force, atomic=atomic) + except (CommandExecutionError, OSError) as exc: + ret["result"] = False + ret["comment"] = "Unable to create new symlink {} -> {}: {}".format( + name, target, exc + ) + return ret + else: + ret["comment"] = "Created new symlink {} -> {}".format(name, target) + ret["changes"]["new"] = name - if not _check_symlink_ownership(name, user, group, win_owner): - if not _set_symlink_ownership(name, user, group, win_owner): - ret["result"] = False - ret["comment"] += ", but was unable to set ownership to {}:{}".format( - user, group - ) + if not _check_symlink_ownership(name, user, group, win_owner): + if not _set_symlink_ownership(name, user, group, win_owner): + ret["result"] = False + ret["comment"] += ", but was unable to set ownership to {}:{}".format( + user, group + ) return ret diff --git a/tests/pytests/functional/modules/file/test_symlink.py b/tests/pytests/functional/modules/file/test_symlink.py index 68920fd9d7d..24e110dc41f 100644 --- a/tests/pytests/functional/modules/file/test_symlink.py +++ b/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()