diff --git a/changelog/39292.fixed b/changelog/39292.fixed new file mode 100644 index 000000000000..cbb472423aa8 --- /dev/null +++ b/changelog/39292.fixed @@ -0,0 +1 @@ +Fix mounted bind mounts getting active mount options added diff --git a/changelog/54508.fixed b/changelog/54508.fixed new file mode 100644 index 000000000000..0292039705a1 --- /dev/null +++ b/changelog/54508.fixed @@ -0,0 +1 @@ +Fix mount.mounted does not handle blanks properly diff --git a/salt/modules/mount.py b/salt/modules/mount.py index 76a52b89ea61..3f60af9eb311 100644 --- a/salt/modules/mount.py +++ b/salt/modules/mount.py @@ -1312,9 +1312,9 @@ def mount( cmd = "mount " if device: - cmd += "{} {} {} ".format(args, device, name) + cmd += "{} '{}' '{}' ".format(args, device, name) else: - cmd += "{} ".format(name) + cmd += "'{}' ".format(name) out = __salt__["cmd.run_all"](cmd, runas=user, python_shell=False) if out["retcode"]: return out["stderr"] @@ -1374,9 +1374,9 @@ def remount(name, device, mkmnt=False, fstype="", opts="defaults", user=None): args += " -t {}".format(fstype) if __grains__["os"] not in ["OpenBSD", "MacOS", "Darwin"] or force_mount: - cmd = "mount {} {} {} ".format(args, device, name) + cmd = "mount {} '{}' '{}' ".format(args, device, name) else: - cmd = "mount -u {} {} {} ".format(args, device, name) + cmd = "mount -u {} '{}' '{}' ".format(args, device, name) out = __salt__["cmd.run_all"](cmd, runas=user, python_shell=False) if out["retcode"]: return out["stderr"] @@ -1416,9 +1416,9 @@ def umount(name, device=None, user=None, util="mount"): return "{} does not have anything mounted".format(name) if not device: - cmd = "umount {}".format(name) + cmd = "umount '{}'".format(name) else: - cmd = "umount {}".format(device) + cmd = "umount '{}'".format(device) out = __salt__["cmd.run_all"](cmd, runas=user, python_shell=False) if out["retcode"]: return out["stderr"] @@ -1535,11 +1535,11 @@ def swapon(name, priority=None): if __grains__["kernel"] == "SunOS": if __grains__["virtual"] != "zone": - __salt__["cmd.run"]("swap -a {}".format(name), python_shell=False) + __salt__["cmd.run"]("swap -a '{}'".format(name), python_shell=False) else: return False else: - cmd = "swapon {}".format(name) + cmd = "swapon '{}'".format(name) if priority and "AIX" not in __grains__["kernel"]: cmd += " -p {}".format(priority) __salt__["cmd.run"](cmd, python_shell=False) @@ -1569,13 +1569,13 @@ def swapoff(name): if name in on_: if __grains__["kernel"] == "SunOS": if __grains__["virtual"] != "zone": - __salt__["cmd.run"]("swap -a {}".format(name), python_shell=False) + __salt__["cmd.run"]("swap -a '{}'".format(name), python_shell=False) else: return False elif __grains__["os"] != "OpenBSD": - __salt__["cmd.run"]("swapoff {}".format(name), python_shell=False) + __salt__["cmd.run"]("swapoff '{}'".format(name), python_shell=False) else: - __salt__["cmd.run"]("swapctl -d {}".format(name), python_shell=False) + __salt__["cmd.run"]("swapctl -d '{}'".format(name), python_shell=False) on_ = swaps() if name in on_: return False diff --git a/salt/states/mount.py b/salt/states/mount.py index db0753638986..36b9a16b5d04 100644 --- a/salt/states/mount.py +++ b/salt/states/mount.py @@ -70,6 +70,7 @@ def mounted( extra_mount_ignore_fs_keys=None, extra_mount_translate_options=None, hidden_opts=None, + bind_mount_copy_active_opts=True, **kwargs ): """ @@ -186,6 +187,12 @@ def mounted( as part of the state application .. versionadded:: 2015.8.2 + + bind_mount_copy_active_opts + If set to ``False``, this option disables the default behavior of + copying the options from the bind mount if it was found to be active. + + .. versionadded:: 3006.0 """ ret = {"name": name, "changes": {}, "result": True, "comment": ""} @@ -234,37 +241,41 @@ def mounted( # Get the active data active = __salt__["mount.active"](extended=True) real_name = os.path.realpath(name) + # real_name for comparisons to the active mount list + comp_real_name = real_name.replace(" ", "\\040") if device.startswith("/"): - if "bind" in opts and real_name in active: - _device = device - if active[real_name]["device"].startswith("/"): + if "bind" in opts and comp_real_name in active: + _device = device.replace(" ", "\\040") + if active[comp_real_name]["device"].startswith("/"): # Find the device that the bind really points at. while True: if _device in active: _real_device = active[_device]["device"] - opts = list( - set( - opts - + active[_device]["opts"] - + active[_device]["superopts"] + if bind_mount_copy_active_opts: + opts = sorted( + set( + opts + + active[_device]["opts"] + + active[_device]["superopts"] + ) ) - ) - active[real_name]["opts"].append("bind") + active[comp_real_name]["opts"].append("bind") break - _device = os.path.dirname(_device) + _device = os.path.dirname(_device.replace("\\040", " ")) real_device = _real_device else: # Remote file systems act differently. if _device in active: - opts = list( - set( - opts - + active[_device]["opts"] - + active[_device]["superopts"] + if bind_mount_copy_active_opts: + opts = sorted( + set( + opts + + active[_device]["opts"] + + active[_device]["superopts"] + ) ) - ) - active[real_name]["opts"].append("bind") - real_device = active[real_name]["device"] + active[comp_real_name]["opts"].append("bind") + real_device = active[comp_real_name]["device"] else: real_device = os.path.realpath(device) elif device.upper().startswith("UUID="): @@ -315,25 +326,25 @@ def mounted( if "device_name" in fuse_match.groupdict(): real_device = fuse_match.group("device_name") - if real_name in active: - if "superopts" not in active[real_name]: - active[real_name]["superopts"] = [] + if comp_real_name in active: + if "superopts" not in active[comp_real_name]: + active[comp_real_name]["superopts"] = [] if mount: - device_list.append(active[real_name]["device"]) + device_list.append(active[comp_real_name]["device"]) device_list.append(os.path.realpath(device_list[0])) alt_device = ( - active[real_name]["alt_device"] - if "alt_device" in active[real_name] + active[comp_real_name]["alt_device"] + if "alt_device" in active[comp_real_name] else None ) uuid_device = ( - active[real_name]["device_uuid"] - if "device_uuid" in active[real_name] + active[comp_real_name]["device_uuid"] + if "device_uuid" in active[comp_real_name] else None ) label_device = ( - active[real_name]["device_label"] - if "device_label" in active[real_name] + active[comp_real_name]["device_label"] + if "device_label" in active[comp_real_name] else None ) if alt_device and alt_device not in device_list: @@ -407,6 +418,7 @@ def mounted( if extra_mount_translate_options: mount_translate_options.update(extra_mount_translate_options) + trigger_remount = [] for opt in opts: if opt in mount_translate_options: opt = mount_translate_options[opt] @@ -440,7 +452,7 @@ def mounted( _id = _info[_param] opt = _param + "=" + str(_id) - _active_superopts = active[real_name].get("superopts", []) + _active_superopts = active[comp_real_name].get("superopts", []) for _active_opt in _active_superopts: size_match = re.match( r"size=(?P[0-9]+)(?Pk|m|g)", @@ -454,64 +466,71 @@ def mounted( _active_superopts.append(_active_opt) if ( - opt not in active[real_name]["opts"] + opt not in active[comp_real_name]["opts"] and opt not in _active_superopts and opt not in mount_invisible_options and opt not in mount_ignore_fs_keys.get(fstype, []) and opt not in mount_invisible_keys ): - if __opts__["test"]: - ret["result"] = None - ret[ - "comment" - ] = "Remount would be forced because options ({}) changed".format( - opt - ) - return ret - else: - # Some file systems require umounting and mounting if options change - # add others to list that require similiar functionality - if fstype in ["nfs", "cvfs"] or fstype.startswith("fuse"): - ret["changes"]["umount"] = ( - "Forced unmount and mount because " - + "options ({}) changed".format(opt) - ) - unmount_result = __salt__["mount.umount"](real_name) - if unmount_result is True: - mount_result = __salt__["mount.mount"]( - real_name, - device, - mkmnt=mkmnt, - fstype=fstype, - opts=opts, - ) - ret["result"] = mount_result - else: - ret["result"] = False - ret["comment"] = "Unable to unmount {}: {}.".format( - real_name, unmount_result - ) - return ret - else: - ret["changes"]["umount"] = ( - "Forced remount because " - + "options ({}) changed".format(opt) + trigger_remount.append(opt) + + if trigger_remount: + if __opts__["test"]: + ret["result"] = None + ret[ + "comment" + ] = "Remount would be forced because options ({}) changed".format( + ",".join(sorted(trigger_remount)) + ) + return ret + else: + # Some file systems require umounting and mounting if options change + # add others to list that require similiar functionality + if fstype in ["nfs", "cvfs"] or fstype.startswith("fuse"): + ret["changes"]["umount"] = ( + "Forced unmount and mount because " + + "options ({}) changed".format( + ",".join(sorted(trigger_remount)) ) - remount_result = __salt__["mount.remount"]( + ) + unmount_result = __salt__["mount.umount"](real_name) + if unmount_result is True: + mount_result = __salt__["mount.mount"]( real_name, device, mkmnt=mkmnt, fstype=fstype, opts=opts, ) - ret["result"] = remount_result - # Cleanup after the remount, so we - # don't write remount into fstab - if "remount" in opts: - opts.remove("remount") + ret["result"] = mount_result + else: + ret["result"] = False + ret["comment"] = "Unable to unmount {}: {}.".format( + real_name, unmount_result + ) + return ret + else: + ret["changes"]["umount"] = ( + "Forced remount because " + + "options ({}) changed".format( + ",".join(sorted(trigger_remount)) + ) + ) + remount_result = __salt__["mount.remount"]( + real_name, + device, + mkmnt=mkmnt, + fstype=fstype, + opts=opts, + ) + ret["result"] = remount_result + # Cleanup after the remount, so we + # don't write remount into fstab + if "remount" in opts: + opts.remove("remount") - # Update the cache - update_mount_cache = True + # Update the cache + update_mount_cache = True mount_cache = __salt__["mount.read_mount_cache"](real_name) if "opts" in mount_cache: @@ -605,7 +624,7 @@ def mounted( ret["changes"]["umount"] += ", current: " + ", ".join(device_list) out = __salt__["mount.umount"](real_name, user=user) active = __salt__["mount.active"](extended=True) - if real_name in active: + if comp_real_name in active: ret["comment"] = "Unable to unmount" ret["result"] = None return ret @@ -613,7 +632,7 @@ def mounted( else: ret["comment"] = "Target was already mounted" # using a duplicate check so I can catch the results of a umount - if real_name not in active: + if comp_real_name not in active: if mount: # The mount is not present! Mount it if __opts__["test"]: @@ -641,7 +660,7 @@ def mounted( ret["comment"] = out ret["result"] = False return ret - elif real_name in active: + elif comp_real_name in active: # (Re)mount worked! ret["comment"] = "Target was successfully mounted" ret["changes"]["mount"] = True @@ -922,10 +941,11 @@ def unmounted( # Get the active data active = __salt__["mount.active"](extended=True) - if name not in active: + comp_name = name.replace(" ", "\\040") + if comp_name not in active: # Nothing to unmount ret["comment"] = "Target was already unmounted" - if name in active: + if comp_name in active: # The mount is present! Unmount it if __opts__["test"]: ret["result"] = None diff --git a/tests/pytests/unit/modules/test_mount.py b/tests/pytests/unit/modules/test_mount.py index c48886c24b7b..7e0b055db254 100644 --- a/tests/pytests/unit/modules/test_mount.py +++ b/tests/pytests/unit/modules/test_mount.py @@ -473,13 +473,13 @@ def test_mount(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.mount("name", "device") mock.assert_called_with( - "mount device name ", python_shell=False, runas=None + "mount 'device' 'name' ", python_shell=False, runas=None ) with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.mount("name", "device", fstype="fstype") mock.assert_called_with( - "mount -t fstype device name ", + "mount -t fstype 'device' 'name' ", python_shell=False, runas=None, ) @@ -497,13 +497,13 @@ def test_mount(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.mount("name", "device") mock.assert_called_with( - "mount device name ", python_shell=False, runas=None + "mount 'device' 'name' ", python_shell=False, runas=None ) with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.mount("name", "device", fstype="fstype") mock.assert_called_with( - "mount -v fstype device name ", + "mount -v fstype 'device' 'name' ", python_shell=False, runas=None, ) @@ -521,7 +521,7 @@ def test_mount(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.mount("name", "device") mock.assert_called_with( - "mount -o defaults device name ", + "mount -o defaults 'device' 'name' ", python_shell=False, runas=None, ) @@ -529,7 +529,7 @@ def test_mount(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.mount("name", "device", fstype="fstype") mock.assert_called_with( - "mount -o defaults -t fstype device name ", + "mount -o defaults -t fstype 'device' 'name' ", python_shell=False, runas=None, ) @@ -578,7 +578,7 @@ def test_remount_already_mounted_no_fstype(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.remount("name", "device") mock.assert_called_with( - "mount -u -o noowners device name ", + "mount -u -o noowners 'device' 'name' ", python_shell=False, runas=None, ) @@ -590,7 +590,7 @@ def test_remount_already_mounted_no_fstype(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.remount("name", "device") mock.assert_called_with( - "mount -o remount device name ", python_shell=False, runas=None + "mount -o remount 'device' 'name' ", python_shell=False, runas=None ) with patch.dict(mount.__grains__, {"os": "Linux"}): @@ -600,7 +600,7 @@ def test_remount_already_mounted_no_fstype(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.remount("name", "device") mock.assert_called_with( - "mount -o defaults,remount device name ", + "mount -o defaults,remount 'device' 'name' ", python_shell=False, runas=None, ) @@ -618,7 +618,7 @@ def test_remount_already_mounted_with_fstype(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.remount("name", "device", fstype="type") mock.assert_called_with( - "mount -u -o noowners -t type device name ", + "mount -u -o noowners -t type 'device' 'name' ", python_shell=False, runas=None, ) @@ -630,7 +630,7 @@ def test_remount_already_mounted_with_fstype(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.remount("name", "device", fstype="type") mock.assert_called_with( - "mount -o remount -v type device name ", + "mount -o remount -v type 'device' 'name' ", python_shell=False, runas=None, ) @@ -642,7 +642,7 @@ def test_remount_already_mounted_with_fstype(): with patch.dict(mount.__salt__, {"cmd.run_all": mock}): assert mount.remount("name", "device", fstype="type") mock.assert_called_with( - "mount -o defaults,remount -t type device name ", + "mount -o defaults,remount -t type 'device' 'name' ", python_shell=False, runas=None, ) @@ -681,30 +681,24 @@ def test_is_fuse_exec(): with patch.object(salt.utils.path, "which", return_value=None): assert not mount.is_fuse_exec("cmd") - def _ldd_side_effect(cmd, *args, **kwargs): - """ - Neither of these are full ldd output, but what is_fuse_exec is - looking for is 'libfuse' in the ldd output, so these examples - should be sufficient enough to test both the True and False cases. - """ - return { - "ldd cmd1": textwrap.dedent( + which_mock = MagicMock(side_effect=lambda x: x) + ldd_mock = MagicMock( + side_effect=[ + textwrap.dedent( """\ linux-vdso.so.1 (0x00007ffeaf5fb000) libfuse3.so.3 => /usr/lib/libfuse3.so.3 (0x00007f91e66ac000) """ ), - "ldd cmd2": textwrap.dedent( + textwrap.dedent( """\ linux-vdso.so.1 (0x00007ffeaf5fb000) """ ), - }[cmd] - - which_mock = MagicMock(side_effect=lambda x: x) - ldd_mock = MagicMock(side_effect=_ldd_side_effect) + ] + ) with patch.object(salt.utils.path, "which", which_mock): - with patch.dict(mount.__salt__, {"cmd.run": _ldd_side_effect}): + with patch.dict(mount.__salt__, {"cmd.run": ldd_mock}): assert mount.is_fuse_exec("cmd1") assert not mount.is_fuse_exec("cmd2") diff --git a/tests/pytests/unit/states/test_mount.py b/tests/pytests/unit/states/test_mount.py index f17541308add..5e4d5274e858 100644 --- a/tests/pytests/unit/states/test_mount.py +++ b/tests/pytests/unit/states/test_mount.py @@ -1289,3 +1289,145 @@ def test_fstab_absent_absent(): ), patch.dict(mount.__salt__, salt_mock): assert mount.fstab_absent("/dev/sda1", "/home") == ret salt_mock["mount.fstab"].assert_called_with("/etc/fstab") + + +@pytest.mark.parametrize("mount_name", ["/home/tmp", "/home/tmp with spaces"]) +def test_bind_mount_copy_active_opts(mount_name): + name = mount_name + device = name + active_name = name.replace(" ", "\\040") + fstype = "none" + opts = [ + "bind", + "nodev", + "noexec", + "nosuid", + "rw", + ] + + ret = {"name": name, "result": None, "comment": "", "changes": {}} + + mock_active = MagicMock( + return_value={ + active_name: { + "alt_device": "/dev/vda1", + "device": "/dev/vda1", + "device_label": None, + "device_uuid": "b4e712d1-cd94-4b7c-97cd-294d3db80ec6", + "fstype": "ext4", + "major": "254", + "minor": "1", + "mountid": "105", + "opts": ["rw", "relatime"], + "parentid": "25", + "root": active_name, + "superopts": ["rw", "discard", "errors=remount-ro"], + }, + } + ) + mock_read_mount_cache = MagicMock( + return_value={ + "device": device, + "fstype": "none", + "mkmnt": False, + "opts": ["bind", "nodev", "noexec", "nosuid", "rw"], + } + ) + mock_set_fstab = MagicMock(return_value="new") + + with patch.dict(mount.__grains__, {"os": "CentOS"}), patch.dict( + mount.__salt__, + { + "mount.active": mock_active, + "mount.read_mount_cache": mock_read_mount_cache, + "mount.remount": MagicMock(return_value=True), + "mount.set_fstab": mock_set_fstab, + "mount.write_mount_cache": MagicMock(return_value=True), + }, + ), patch.object( + os.path, + "realpath", + MagicMock( + side_effect=[ + name, + "/dev/vda1", + name, + "/dev/vda1", + name, + "/dev/vda1", + ] + ), + ): + with patch.dict(mount.__opts__, {"test": True}): + ret[ + "comment" + ] = "Remount would be forced because options (nodev,noexec,nosuid) changed" + result = mount.mounted( + name=name, + device=device, + fstype=fstype, + opts=opts, + persist=True, + bind_mount_copy_active_opts=False, + ) + assert result == ret + + with patch.dict(mount.__opts__, {"test": False}): + ret["comment"] = "Target was already mounted. Added new entry to the fstab." + ret["changes"] = { + "persist": "new", + "umount": "Forced remount because options (nodev,noexec,nosuid) changed", + } + ret["result"] = True + + # bind_mount_copy_active_opts is off + result = mount.mounted( + name=name, + device=device, + fstype=fstype, + opts=opts, + persist=True, + bind_mount_copy_active_opts=False, + ) + assert result == ret + + mock_set_fstab.assert_called_with( + name, + device, + fstype, + ["bind", "nodev", "noexec", "nosuid", "rw"], + 0, + 0, + "/etc/fstab", + match_on="auto", + ) + + # bind_mount_copy_active_opts is on (default) + result = mount.mounted( + name=name, + device=device, + fstype=fstype, + opts=opts, + persist=True, + ) + assert result == ret + + mock_set_fstab.assert_called_with( + name, + device, + fstype, + [ + "bind", + "discard", + "errors=remount-ro", + "nodev", + "noexec", + "nosuid", + "relatime", + "rw", + ], + 0, + 0, + "/etc/fstab", + match_on="auto", + )