Skip to content

Commit

Permalink
fixes saltstack#39292 fix copying superopts to bind mounts
Browse files Browse the repository at this point in the history
  • Loading branch information
@nicholasmhughes
nicholasmhughes committed Sep 22, 2022
1 parent 6558946 commit f6a43ae
Show file tree
Hide file tree
Showing 3 changed files with 214 additions and 56 deletions.
1 change: 1 addition & 0 deletions changelog/39292.fixed
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix mounted bind mounts getting active mount options added
129 changes: 73 additions & 56 deletions salt/states/mount.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
):
"""
Expand Down Expand Up @@ -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": ""}

Expand Down Expand Up @@ -242,27 +249,29 @@ 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)
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"]
else:
Expand Down Expand Up @@ -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]
Expand Down Expand Up @@ -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
)
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:
Expand Down
140 changes: 140 additions & 0 deletions tests/pytests/unit/states/test_mount.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
)

0 comments on commit f6a43ae

Please sign in to comment.