diff --git a/changelog/40054.fixed b/changelog/40054.fixed new file mode 100644 index 00000000000..57641e5c186 --- /dev/null +++ b/changelog/40054.fixed @@ -0,0 +1 @@ +Fix `sysctl.present` converts spaces to tabs. diff --git a/salt/modules/linux_sysctl.py b/salt/modules/linux_sysctl.py index 3bc884161e7..fe63cb6c596 100644 --- a/salt/modules/linux_sysctl.py +++ b/salt/modules/linux_sysctl.py @@ -88,21 +88,16 @@ def show(config_file=False): try: with salt.utils.files.fopen(config_file) as fp_: for line in fp_: - line = salt.utils.stringutils.to_str(line) + line = salt.utils.stringutils.to_str(line).strip() if not line.startswith("#") and "=" in line: - # search if we have some '=' instead of ' = ' separators - SPLIT = " = " - if SPLIT not in line: - SPLIT = SPLIT.strip() - key, value = line.split(SPLIT, 1) - key = key.strip() - value = value.lstrip() - ret[key] = value + key, value = line.split("=", 1) + ret[key.rstrip()] = value.lstrip() except OSError: log.error("Could not open sysctl file") return None else: - cmd = "{} -a".format(_which("sysctl")) + _sysctl = "{}".format(_which("sysctl")) + cmd = [_sysctl, "-a"] out = __salt__["cmd.run_stdout"](cmd, output_loglevel="trace") for line in out.splitlines(): if not line or " = " not in line: @@ -122,7 +117,8 @@ def get(name): salt '*' sysctl.get net.ipv4.ip_forward """ - cmd = "{} -n {}".format(_which("sysctl"), name) + _sysctl = "{}".format(_which("sysctl")) + cmd = [_sysctl, "-n", name] out = __salt__["cmd.run"](cmd, python_shell=False) return out @@ -146,7 +142,8 @@ def assign(name, value): raise CommandExecutionError("sysctl {} does not exist".format(name)) ret = {} - cmd = '{} -w {}="{}"'.format(_which("sysctl"), name, value) + _sysctl = "{}".format(_which("sysctl")) + cmd = [_sysctl, "-w", "{}={}".format(name, value)] data = __salt__["cmd.run_all"](cmd, python_shell=False) out = data["stdout"] err = data["stderr"] @@ -167,6 +164,17 @@ def assign(name, value): return ret +def _sanitize_sysctl_value(value): + """Replace separating whitespaces by exactly one tab. + + On Linux procfs, files such as /proc/sys/net/ipv4/tcp_rmem or many + other sysctl with whitespace in it consistently use one tab. When + setting the value, spaces or tabs can be used and will be converted + to tabs by the kernel (when reading them again). + """ + return re.sub(r"\s+", "\t", str(value)) + + def persist(name, value, config=None): """ Assign and persist a simple sysctl parameter for this minion. If ``config`` @@ -207,9 +215,6 @@ def persist(name, value, config=None): raise CommandExecutionError(msg.format(config)) for line in config_data: - if line.startswith("#"): - nlines.append(line) - continue if "=" not in line: nlines.append(line) continue @@ -217,25 +222,17 @@ def persist(name, value, config=None): # Strip trailing whitespace and split the k,v comps = [i.strip() for i in line.split("=", 1)] - # On Linux procfs, files such as /proc/sys/net/ipv4/tcp_rmem or any - # other sysctl with whitespace in it consistently uses 1 tab. Lets - # allow our users to put a space or tab between multi-value sysctls - # and have salt not try to set it every single time. - if isinstance(comps[1], str) and " " in comps[1]: - comps[1] = re.sub(r"\s+", "\t", comps[1]) - - # Do the same thing for the value 'just in case' - if isinstance(value, str) and " " in value: - value = re.sub(r"\s+", "\t", value) - - if len(comps) < 2: + if comps[0].startswith("#"): + # Check for comment lines after stripping leading whitespaces. nlines.append(line) continue + if name == comps[0]: # This is the line to edit - if str(comps[1]) == str(value): + sanitized_value = _sanitize_sysctl_value(value) + if _sanitize_sysctl_value(comps[1]) == sanitized_value: # It is correct in the config, check if it is correct in /proc - if str(get(name)) != str(value): + if _sanitize_sysctl_value(get(name)) != sanitized_value: assign(name, value) return "Updated" else: diff --git a/tests/pytests/unit/modules/test_linux_sysctl.py b/tests/pytests/unit/modules/test_linux_sysctl.py index ab80eb9de9a..0bdd24039d7 100644 --- a/tests/pytests/unit/modules/test_linux_sysctl.py +++ b/tests/pytests/unit/modules/test_linux_sysctl.py @@ -4,11 +4,14 @@ Tests for salt.modules.linux_sysctl module :codeauthor: jmoney """ +import os + import pytest import salt.modules.linux_sysctl as linux_sysctl import salt.modules.systemd_service as systemd from salt.exceptions import CommandExecutionError +from salt.utils.files import fopen from tests.support.mock import MagicMock, mock_open, patch pytestmark = [ @@ -26,8 +29,64 @@ def test_get(): Tests the return of get function """ mock_cmd = MagicMock(return_value=1) - with patch.dict(linux_sysctl.__salt__, {"cmd.run": mock_cmd}): - assert linux_sysctl.get("net.ipv4.ip_forward") == 1 + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch.dict(linux_sysctl.__salt__, {"cmd.run": mock_cmd}): + assert linux_sysctl.get("net.ipv4.ip_forward") == 1 + mock_cmd.assert_called_once_with( + ["/usr/sbin/sysctl", "-n", "net.ipv4.ip_forward"], python_shell=False + ) + + +def test_show(): + """ + Tests the return of show function + """ + mock_cmd = MagicMock( + return_value="""\ +kernel.core_pattern = |/usr/share/kdump-tools/dump-core %p %s %t %e +kernel.printk = 3 4 1 3 +net.ipv4.ip_forward = 1 +net.ipv4.tcp_rmem = 4096 131072 6291456 +""" + ) + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch.dict(linux_sysctl.__salt__, {"cmd.run_stdout": mock_cmd}): + assert linux_sysctl.show() == { + "kernel.core_pattern": "|/usr/share/kdump-tools/dump-core %p %s %t %e", + "kernel.printk": "3 4 1 3", + "net.ipv4.ip_forward": "1", + "net.ipv4.tcp_rmem": "4096\t131072\t6291456", + } + mock_cmd.assert_called_once_with( + ["/usr/sbin/sysctl", "-a"], output_loglevel="trace" + ) + + +def test_show_config_file(tmp_path): + """ + Tests the return of show function for a given file + """ + config = str(tmp_path / "sysctl.conf") + with fopen(config, "w", encoding="utf-8") as config_file: + config_file.write( + """\ +# Use dump-core from kdump-tools Debian package. +kernel.core_pattern = |/usr/share/kdump-tools/dump-core %p %s %t %e + # Stop low-level messages on console = less logging + kernel.printk = 3 4 1 3 + +net.ipv4.ip_forward=1 +net.ipv4.tcp_rmem = 4096 131072 6291456 +""" + ) + assert linux_sysctl.show(config) == { + "kernel.core_pattern": "|/usr/share/kdump-tools/dump-core %p %s %t %e", + "kernel.printk": "3 4 1 3", + "net.ipv4.ip_forward": "1", + "net.ipv4.tcp_rmem": "4096\t131072\t6291456", + } def test_get_no_sysctl_binary(): @@ -54,6 +113,7 @@ def test_assign_proc_sys_failed(): with patch.dict(linux_sysctl.__salt__, {"cmd.run_all": mock_cmd}): with pytest.raises(CommandExecutionError): linux_sysctl.assign("net.ipv4.ip_forward", 1) + mock_cmd.assert_not_called() def test_assign_cmd_failed(): @@ -68,9 +128,15 @@ def test_assign_cmd_failed(): "stdout": "net.ipv4.ip_forward = backward", } mock_cmd = MagicMock(return_value=cmd) - with patch.dict(linux_sysctl.__salt__, {"cmd.run_all": mock_cmd}): - with pytest.raises(CommandExecutionError): - linux_sysctl.assign("net.ipv4.ip_forward", "backward") + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch.dict(linux_sysctl.__salt__, {"cmd.run_all": mock_cmd}): + with pytest.raises(CommandExecutionError): + linux_sysctl.assign("net.ipv4.ip_forward", "backward") + mock_cmd.assert_called_once_with( + ["/usr/sbin/sysctl", "-w", "net.ipv4.ip_forward=backward"], + python_shell=False, + ) def test_assign_success(): @@ -86,30 +152,70 @@ def test_assign_success(): } ret = {"net.ipv4.ip_forward": "1"} mock_cmd = MagicMock(return_value=cmd) - with patch.dict(linux_sysctl.__salt__, {"cmd.run_all": mock_cmd}): - assert linux_sysctl.assign("net.ipv4.ip_forward", 1) == ret + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch.dict(linux_sysctl.__salt__, {"cmd.run_all": mock_cmd}): + assert linux_sysctl.assign("net.ipv4.ip_forward", 1) == ret + mock_cmd.assert_called_once_with( + ["/usr/sbin/sysctl", "-w", "net.ipv4.ip_forward=1"], python_shell=False + ) + + +def test_sanitize_sysctl_value(): + assert ( + linux_sysctl._sanitize_sysctl_value("4096 131072 6291456") + == "4096\t131072\t6291456" + ) + + +def test_sanitize_sysctl_value_int(): + assert linux_sysctl._sanitize_sysctl_value(1337) == "1337" + + +def test_persist_int(tmp_path): + """ + Tests linux_sysctl.persist for an integer that is already set. + """ + config = str(tmp_path / "sysctl.conf") + config_file_content = "fs.suid_dumpable = 2\n" + with fopen(config, "w", encoding="utf-8") as config_file: + config_file.write(config_file_content) + mock_run = MagicMock(return_value="2") + mock_run_all = MagicMock() + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("os.path.exists", MagicMock(return_value=True)), patch.dict( + linux_sysctl.__salt__, {"cmd.run": mock_run, "cmd.run_all": mock_run_all} + ): + assert ( + linux_sysctl.persist("fs.suid_dumpable", 2, config=config) + == "Already set" + ) + mock_run.assert_called_once_with( + ["/usr/sbin/sysctl", "-n", "fs.suid_dumpable"], python_shell=False + ) + mock_run_all.assert_not_called() + assert os.path.isfile(config) + with fopen(config, encoding="utf-8") as config_file: + written = config_file.read() + assert written == config_file_content def test_persist_no_conf_failure(): """ Tests adding of config file failure """ - asn_cmd = { - "pid": 1337, - "retcode": 0, - "stderr": "sysctl: permission denied", - "stdout": "", - } - mock_asn_cmd = MagicMock(return_value=asn_cmd) - cmd = "sysctl -w net.ipv4.ip_forward=1" - mock_cmd = MagicMock(return_value=cmd) - with patch.dict( - linux_sysctl.__salt__, - {"cmd.run_stdout": mock_cmd, "cmd.run_all": mock_asn_cmd}, - ): - with patch("salt.utils.files.fopen", mock_open()) as m_open: + fopen_mock = MagicMock(side_effect=OSError()) + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("os.path.isfile", MagicMock(return_value=False)), patch( + "os.path.exists", MagicMock(return_value=False) + ), patch("os.makedirs", MagicMock()), patch( + "salt.utils.files.fopen", fopen_mock + ): with pytest.raises(CommandExecutionError): - linux_sysctl.persist("net.ipv4.ip_forward", 1, config=None) + linux_sysctl.persist("net.ipv4.ip_forward", 42, config=None) + fopen_mock.called_once() def test_persist_no_conf_success(): @@ -131,18 +237,23 @@ def test_persist_no_conf_success(): sys_cmd = "systemd 208\n+PAM +LIBWRAP" mock_sys_cmd = MagicMock(return_value=sys_cmd) - with patch("salt.utils.files.fopen", mock_open()) as m_open, patch.dict( - linux_sysctl.__context__, {"salt.utils.systemd.version": 232} - ), patch.dict( - linux_sysctl.__salt__, - {"cmd.run_stdout": mock_sys_cmd, "cmd.run_all": mock_asn_cmd}, - ), patch.dict( - systemd.__context__, - {"salt.utils.systemd.booted": True, "salt.utils.systemd.version": 232}, - ): - linux_sysctl.persist("net.ipv4.ip_forward", 1, config=config) - writes = m_open.write_calls() - assert writes == ["#\n# Kernel sysctl configuration\n#\n"], writes + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("salt.utils.files.fopen", mock_open()) as m_open, patch.dict( + linux_sysctl.__context__, {"salt.utils.systemd.version": 232} + ), patch.dict( + linux_sysctl.__salt__, + {"cmd.run_stdout": mock_sys_cmd, "cmd.run_all": mock_asn_cmd}, + ), patch.dict( + systemd.__context__, + {"salt.utils.systemd.booted": True, "salt.utils.systemd.version": 232}, + ): + linux_sysctl.persist("net.ipv4.ip_forward", 1, config=config) + writes = m_open.write_calls() + assert writes == ["#\n# Kernel sysctl configuration\n#\n"], writes + mock_asn_cmd.assert_called_once_with( + ["/usr/sbin/sysctl", "-w", "net.ipv4.ip_forward=1"], python_shell=False + ) def test_persist_read_conf_success(): @@ -163,17 +274,263 @@ def test_persist_read_conf_success(): sys_cmd = "systemd 208\n+PAM +LIBWRAP" mock_sys_cmd = MagicMock(return_value=sys_cmd) - with patch("salt.utils.files.fopen", mock_open()): - with patch.dict( + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("salt.utils.files.fopen", mock_open()), patch.dict( linux_sysctl.__context__, {"salt.utils.systemd.version": 232} + ), patch.dict( + linux_sysctl.__salt__, + {"cmd.run_stdout": mock_sys_cmd, "cmd.run_all": mock_asn_cmd}, + ), patch.dict( + systemd.__context__, {"salt.utils.systemd.booted": True} ): - with patch.dict( - linux_sysctl.__salt__, - {"cmd.run_stdout": mock_sys_cmd, "cmd.run_all": mock_asn_cmd}, - ): - with patch.dict( - systemd.__context__, {"salt.utils.systemd.booted": True} - ): - assert ( - linux_sysctl.persist("net.ipv4.ip_forward", 1) == "Updated" - ) + assert linux_sysctl.persist("net.ipv4.ip_forward", 1) == "Updated" + mock_asn_cmd.assert_called_once_with( + ["/usr/sbin/sysctl", "-w", "net.ipv4.ip_forward=1"], python_shell=False + ) + + +def test_persist_parsing_file(tmp_path): + """ + Tests linux_sysctl.persist to correctly parse the config file. + """ + config = str(tmp_path / "sysctl.conf") + with fopen(config, "w", encoding="utf-8") as config_file: + config_file.write( + """\ +# Use dump-core from kdump-tools Debian package. +kernel.core_pattern = |/usr/share/kdump-tools/dump-core %p %s %t %e + # Stop low-level messages on console = less logging + kernel.printk = 3 4 1 3 + + net.ipv4.ip_forward=1 +net.ipv4.tcp_rmem = 4096 131072 6291456 +""" + ) + mock_run = MagicMock() + mock_run_all = MagicMock( + return_value={ + "pid": 1337, + "retcode": 0, + "stderr": "", + "stdout": "net.ipv4.ip_forward = 0", + } + ) + + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("os.path.exists", MagicMock(return_value=True)), patch.dict( + linux_sysctl.__salt__, {"cmd.run": mock_run, "cmd.run_all": mock_run_all} + ): + assert ( + linux_sysctl.persist("net.ipv4.ip_forward", "0", config=config) + == "Updated" + ) + mock_run.assert_not_called() + mock_run_all.assert_called_once_with( + ["/usr/sbin/sysctl", "-w", "net.ipv4.ip_forward=0"], python_shell=False + ) + assert os.path.isfile(config) + with fopen(config, encoding="utf-8") as config_file: + written = config_file.read() + assert ( + written + == """\ +# Use dump-core from kdump-tools Debian package. +kernel.core_pattern = |/usr/share/kdump-tools/dump-core %p %s %t %e + # Stop low-level messages on console = less logging + kernel.printk = 3 4 1 3 + +net.ipv4.ip_forward = 0 +net.ipv4.tcp_rmem = 4096 131072 6291456 +""" + ) + + +def test_persist_value_with_spaces_already_set(tmp_path): + """ + Tests linux_sysctl.persist for a value with spaces that is already set. + """ + config = str(tmp_path / "existing_sysctl_with_spaces.conf") + value = "|/usr/share/kdump-tools/dump-core %p %s %t %e" + config_file_content = "kernel.core_pattern = {}\n".format(value) + with fopen(config, "w", encoding="utf-8") as config_file: + config_file.write(config_file_content) + mock_run = MagicMock(return_value=value) + mock_run_all = MagicMock() + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("os.path.exists", MagicMock(return_value=True)), patch.dict( + linux_sysctl.__salt__, {"cmd.run": mock_run, "cmd.run_all": mock_run_all} + ): + assert ( + linux_sysctl.persist("kernel.core_pattern", value, config=config) + == "Already set" + ) + mock_run.assert_called_once_with( + ["/usr/sbin/sysctl", "-n", "kernel.core_pattern"], python_shell=False + ) + mock_run_all.assert_not_called() + assert os.path.isfile(config) + with fopen(config, encoding="utf-8") as config_file: + written = config_file.read() + assert written == config_file_content + + +def test_persist_value_with_spaces_already_configured(tmp_path): + """ + Tests linux_sysctl.persist for a value with spaces that is only configured. + """ + config = str(tmp_path / "existing_sysctl_with_spaces.conf") + value = "|/usr/share/kdump-tools/dump-core %p %s %t %e" + config_file_content = "kernel.core_pattern = {}\n".format(value) + with fopen(config, "w", encoding="utf-8") as config_file: + config_file.write(config_file_content) + mock_run = MagicMock(return_value="") + mock_run_all = MagicMock( + return_value={ + "pid": 1337, + "retcode": 0, + "stderr": "", + "stdout": "kernel.core_pattern = " + value, + } + ) + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("os.path.exists", MagicMock(return_value=True)), patch.dict( + linux_sysctl.__salt__, {"cmd.run": mock_run, "cmd.run_all": mock_run_all} + ): + assert ( + linux_sysctl.persist("kernel.core_pattern", value, config=config) + == "Updated" + ) + mock_run.assert_called_once_with( + ["/usr/sbin/sysctl", "-n", "kernel.core_pattern"], python_shell=False + ) + mock_run_all.assert_called_once_with( + ["/usr/sbin/sysctl", "-w", "kernel.core_pattern=" + value], + python_shell=False, + ) + assert os.path.isfile(config) + with fopen(config, encoding="utf-8") as config_file: + written = config_file.read() + assert written == config_file_content + + +def test_persist_value_with_spaces_update_config(tmp_path): + """ + Tests linux_sysctl.persist for a value with spaces that differs from the config. + """ + config = str(tmp_path / "existing_sysctl_with_spaces.conf") + value = "|/usr/share/kdump-tools/dump-core %p %s %t %e" + with fopen(config, "w", encoding="utf-8") as config_file: + config_file.write("kernel.core_pattern =\n") + mock_run = MagicMock() + mock_run_all = MagicMock( + return_value={ + "pid": 1337, + "retcode": 0, + "stderr": "", + "stdout": "kernel.core_pattern = " + value, + } + ) + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("os.path.exists", MagicMock(return_value=True)), patch.dict( + linux_sysctl.__salt__, {"cmd.run": mock_run, "cmd.run_all": mock_run_all} + ): + assert ( + linux_sysctl.persist("kernel.core_pattern", value, config=config) + == "Updated" + ) + mock_run.assert_not_called() + mock_run_all.assert_called_once_with( + ["/usr/sbin/sysctl", "-w", "kernel.core_pattern=" + value], + python_shell=False, + ) + assert os.path.isfile(config) + with fopen(config, encoding="utf-8") as config_file: + written = config_file.read() + assert written == "kernel.core_pattern = {}\n".format(value) + + +def test_persist_value_with_spaces_new_file(tmp_path): + """ + Tests linux_sysctl.persist for a value that contains spaces. + """ + config = str(tmp_path / "sysctl_with_spaces.conf") + value = "|/usr/share/kdump-tools/dump-core %p %s %t %e" + mock_run_all = MagicMock( + return_value={ + "pid": 1337, + "retcode": 0, + "stderr": "", + "stdout": "kernel.core_pattern = " + value, + } + ) + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("os.path.exists", MagicMock(return_value=True)), patch.dict( + linux_sysctl.__salt__, {"cmd.run_all": mock_run_all} + ): + assert ( + linux_sysctl.persist("kernel.core_pattern", value, config=config) + == "Updated" + ) + mock_run_all.assert_called_once_with( + ["/usr/sbin/sysctl", "-w", "kernel.core_pattern=" + value], + python_shell=False, + ) + assert os.path.isfile(config) + with fopen(config, encoding="utf-8") as config_file: + written = config_file.read() + assert ( + written + == """\ +# +# Kernel sysctl configuration +# +kernel.core_pattern = |/usr/share/kdump-tools/dump-core %p %s %t %e +""" + ) + + +def test_persist_value_with_tabs_new_file(tmp_path): + """ + Tests linux_sysctl.persist for a value that contains tabs. + """ + config = str(tmp_path / "sysctl_with_tabs.conf") + value = "|/usr/share/kdump-tools/dump-core\t%p\t%s\t%t\t%e" + mock_run_all = MagicMock( + return_value={ + "pid": 1337, + "retcode": 0, + "stderr": "", + "stdout": "kernel.core_pattern = " + value, + } + ) + which_mock = MagicMock(return_value="/usr/sbin/sysctl") + with patch("salt.utils.path.which", which_mock): + with patch("os.path.exists", MagicMock(return_value=True)), patch.dict( + linux_sysctl.__salt__, {"cmd.run_all": mock_run_all} + ): + assert ( + linux_sysctl.persist("kernel.core_pattern", value, config=config) + == "Updated" + ) + mock_run_all.assert_called_once_with( + ["/usr/sbin/sysctl", "-w", "kernel.core_pattern=" + value], + python_shell=False, + ) + assert os.path.isfile(config) + with fopen(config, encoding="utf-8") as config_file: + written = config_file.read() + assert ( + written + == """\ +# +# Kernel sysctl configuration +# +kernel.core_pattern = |/usr/share/kdump-tools/dump-core %p %s %t %e +""" + )