From cb2bfc88570954cb9872ae0de6bdf4a9c69e721a Mon Sep 17 00:00:00 2001 From: nicholasmhughes Date: Thu, 22 Sep 2022 19:36:55 -0400 Subject: [PATCH] fixes saltstack/salt#39292 fix copying superopts to bind mounts --- changelog/39292.fixed | 1 + salt/states/mount.py | 129 ++++++++++++---------- tests/pytests/unit/states/test_mount.py | 140 ++++++++++++++++++++++++ 3 files changed, 214 insertions(+), 56 deletions(-) create mode 100644 changelog/39292.fixed diff --git a/changelog/39292.fixed b/changelog/39292.fixed new file mode 100644 index 00000000000..cbb472423aa --- /dev/null +++ b/changelog/39292.fixed @@ -0,0 +1 @@ +Fix mounted bind mounts getting active mount options added diff --git a/salt/states/mount.py b/salt/states/mount.py index db075363898..96efa753c32 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": ""} @@ -242,13 +249,14 @@ def mounted( 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") break _device = os.path.dirname(_device) @@ -256,13 +264,14 @@ def mounted( 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"] else: @@ -407,6 +416,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] @@ -460,58 +470,65 @@ def mounted( 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 + 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)) + ) ) - 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) - ) - 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: diff --git a/tests/pytests/unit/states/test_mount.py b/tests/pytests/unit/states/test_mount.py index f17541308ad..1ccf85f9ab4 100644 --- a/tests/pytests/unit/states/test_mount.py +++ b/tests/pytests/unit/states/test_mount.py @@ -1289,3 +1289,143 @@ 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") + + +def test_bind_mount_copy_active_opts(): + name = "/home/tmp" + device = "/home/tmp" + fstype = "none" + opts = [ + "bind", + "nodev", + "noexec", + "nosuid", + "rw", + ] + + ret = {"name": name, "result": None, "comment": "", "changes": {}} + + mock_active = MagicMock( + return_value={ + "/home/tmp": { + "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": "/home/tmp", + "superopts": ["rw", "discard", "errors=remount-ro"], + }, + } + ) + mock_read_mount_cache = MagicMock( + return_value={ + "device": "/home/tmp", + "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=[ + "/home/tmp", + "/dev/vda1", + "/home/tmp", + "/dev/vda1", + "/home/tmp", + "/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", + )