Merge 3006.x into master

tests/pytests/functional/states/test_pkgrepo.py

@@ -5,37 +5,33 @@ import pytest
 import salt.utils.files
 
 
+@pytest.mark.parametrize(
+    "options",
+    [
+        "",
+        "  signed-by=/foo/bar  ",
+        "  trusted=yes",
+        "signed-by=/foo/bar arch=amd64,i386",
+        "signed-by=foo/bar trusted=yes arch=amd64",
+    ],
+)
 @pytest.mark.skipif(
     not any([x for x in ["ubuntu", "debian"] if x in platform.platform()]),
     reason="Test only for debian based platforms",
 )
-def test_adding_repo_file(states, tmp_path):
+def test_adding_repo_file_options(states, tmp_path, options):
     """
     test adding a repo file using pkgrepo.managed
+    and maintaining the user-supplied options
     """
     repo_file = str(tmp_path / "stable-binary.list")
-    repo_content = "deb http://www.deb-multimedia.org stable main"
-    ret = states.pkgrepo.managed(name=repo_content, file=repo_file, clean_file=True)
-    with salt.utils.files.fopen(repo_file, "r") as fp:
-        file_content = fp.read()
-    assert file_content.strip() == repo_content
-
-
-@pytest.mark.skipif(
-    not any([x for x in ["ubuntu", "debian"] if x in platform.platform()]),
-    reason="Test only for debian based platforms",
-)
-def test_adding_repo_file_arch(states, tmp_path):
-    """
-    test adding a repo file using pkgrepo.managed
-    and setting architecture
-    """
-    repo_file = str(tmp_path / "stable-binary.list")
-    repo_content = "deb [arch=amd64  ] http://www.deb-multimedia.org stable main"
+    option = f"[{options}] " if options != "" else ""
+    expected_option = f"[{options.strip()}] " if options != "" else ""
+    repo_content = f"deb {option}http://www.deb-multimedia.org stable main"
     ret = states.pkgrepo.managed(name=repo_content, file=repo_file, clean_file=True)
     with salt.utils.files.fopen(repo_file, "r") as fp:
         file_content = fp.read()
         assert (
             file_content.strip()
-            == "deb [arch=amd64] http://www.deb-multimedia.org stable main"
+            == f"deb {expected_option}http://www.deb-multimedia.org stable main"
         )

tests/pytests/integration/minion/test_return_retries.py

@@ -5,20 +5,20 @@ from saltfactories.utils import random_string
 
 
 @pytest.fixture(scope="function")
-def salt_minion_retry(salt_master_factory, salt_minion_id):
+def salt_minion_retry(salt_master, salt_minion_id):
     # override the defaults for this test
     config_overrides = {
         "return_retry_timer_max": 0,
         "return_retry_timer": 5,
         "return_retry_tries": 30,
     }
-    factory = salt_master_factory.salt_minion_daemon(
+    factory = salt_master.salt_minion_daemon(
         random_string("retry-minion-"),
         overrides=config_overrides,
         extra_cli_arguments_after_first_start_failure=["--log-level=info"],
     )
     factory.after_terminate(
-        pytest.helpers.remove_stale_minion_key, salt_master_factory, factory.id
+        pytest.helpers.remove_stale_minion_key, salt_master, factory.id
     )
 
     with factory.started():
@@ -37,7 +37,7 @@ def test_publish_retry(salt_master, salt_minion_retry, salt_cli, salt_run_cli):
         # verify we don't yet have the result and sleep
         assert salt_run_cli.run("jobs.lookup_jid", jid, _timeout=10).data == {}
 
-        # the 70s sleep (and 60s timer value) is to reduce flakiness due to slower test runs
+        # the 5s sleep (and 60s timeout value) is to reduce flakiness due to slower test runs
         # and should be addresses when number of tries is configurable through minion opts
         time.sleep(5)
 
@@ -62,7 +62,7 @@ def test_pillar_timeout(salt_master_factory):
             {"cmd_json": cmd},
         ],
         "auto_accept": True,
-        "worker_threads": 3,
+        "worker_threads": 2,
         "peer": True,
     }
     minion_overrides = {
@@ -77,7 +77,7 @@ def test_pillar_timeout(salt_master_factory):
         - name: example
         - changes: True
         - result: True
-        - comment: "Nothing has acutally been changed"
+        - comment: "Nothing has actually been changed"
     """
     master = salt_master_factory.salt_master_daemon(
         "pillar-timeout-master",

tests/pytests/unit/client/ssh/test_shell.py

@@ -4,7 +4,7 @@ import types
 import pytest
 
 import salt.client.ssh.shell as shell
-from tests.support.mock import patch
+from tests.support.mock import MagicMock, PropertyMock, patch
 
 
 @pytest.fixture
@@ -52,3 +52,49 @@ def test_ssh_shell_exec_cmd(caplog):
         ret = _shell.exec_cmd("ls {}".format(passwd))
         assert not any([x for x in ret if passwd in str(x)])
         assert passwd not in caplog.text
+
+
+def test_ssh_shell_exec_cmd_waits_for_term_close_before_reading_exit_status():
+    """
+    Ensure that the terminal is always closed before accessing its exitstatus.
+    """
+    term = MagicMock()
+    has_unread_data = PropertyMock(side_effect=(True, True, False))
+    exitstatus = PropertyMock(
+        side_effect=lambda *args: 0 if term._closed is True else None
+    )
+    term.close.side_effect = lambda *args, **kwargs: setattr(term, "_closed", True)
+    type(term).has_unread_data = has_unread_data
+    type(term).exitstatus = exitstatus
+    term.recv.side_effect = (("hi ", ""), ("there", ""), (None, None), (None, None))
+    shl = shell.Shell({}, "localhost")
+    with patch("salt.utils.vt.Terminal", autospec=True, return_value=term):
+        stdout, stderr, retcode = shl.exec_cmd("do something")
+    assert stdout == "hi there"
+    assert stderr == ""
+    assert retcode == 0
+
+
+def test_ssh_shell_exec_cmd_returns_status_code_with_highest_bit_set_if_process_dies():
+    """
+    Ensure that if a child process dies as the result of a signal instead of exiting
+    regularly, the shell returns the signal code encoded in the lowest seven bits with
+    the highest one set, not None.
+    """
+    term = MagicMock()
+    term.exitstatus = None
+    term.signalstatus = 9
+    has_unread_data = PropertyMock(side_effect=(True, True, False))
+    type(term).has_unread_data = has_unread_data
+    term.recv.side_effect = (
+        ("", "leave me alone"),
+        ("", " please"),
+        (None, None),
+        (None, None),
+    )
+    shl = shell.Shell({}, "localhost")
+    with patch("salt.utils.vt.Terminal", autospec=True, return_value=term):
+        stdout, stderr, retcode = shl.exec_cmd("do something")
+    assert stdout == ""
+    assert stderr == "leave me alone please"
+    assert retcode == 137

tests/pytests/unit/client/ssh/test_ssh.py

@@ -3,7 +3,7 @@ import pytest
 import salt.client.ssh.client
 import salt.utils.msgpack
 from salt.client import ssh
-from tests.support.mock import MagicMock, patch
+from tests.support.mock import MagicMock, Mock, patch
 
 pytestmark = [
     pytest.mark.skip_if_binaries_missing("ssh", "ssh-keygen", check_all=True),
@@ -449,3 +449,60 @@ def test_key_deploy_no_permission_denied(tmp_path, opts):
     ret = client.key_deploy(host, ssh_ret)
     assert ret == ssh_ret
     assert mock_key_run.call_count == 0
+
+
+@pytest.mark.parametrize("retcode,expected", [("null", None), ('"foo"', "foo")])
+def test_handle_routine_remote_invalid_retcode(opts, target, retcode, expected, caplog):
+    """
+    Ensure that if a remote returns an invalid retcode as part of the return dict,
+    the final exit code is still an integer and set to 1 at least.
+    """
+    single_ret = (f'{{"local": {{"retcode": {retcode}, "return": "foo"}}}}', "", 0)
+    opts["tgt"] = "localhost"
+    single = MagicMock(spec=ssh.Single)
+    single.id = "localhost"
+    single.run.return_value = single_ret
+    que = Mock()
+
+    with patch("salt.roster.get_roster_file", MagicMock(return_value="")), patch(
+        "salt.client.ssh.Single", autospec=True, return_value=single
+    ):
+        client = ssh.SSH(opts)
+        client.handle_routine(que, opts, "localhost", target)
+    que.put.assert_called_once_with(
+        ({"id": "localhost", "ret": {"retcode": expected, "return": "foo"}}, 1)
+    )
+    assert f"Host 'localhost' reported an invalid retcode: '{expected}'" in caplog.text
+
+
+def test_handle_routine_single_run_invalid_retcode(opts, target, caplog):
+    """
+    Ensure that if Single.run() call returns an invalid retcode,
+    the final exit code is still an integer and set to 1 at least.
+    """
+    single_ret = ("", "Something went seriously wrong", None)
+    opts["tgt"] = "localhost"
+    single = MagicMock(spec=ssh.Single)
+    single.id = "localhost"
+    single.run.return_value = single_ret
+    que = Mock()
+
+    with patch("salt.roster.get_roster_file", MagicMock(return_value="")), patch(
+        "salt.client.ssh.Single", autospec=True, return_value=single
+    ):
+        client = ssh.SSH(opts)
+        client.handle_routine(que, opts, "localhost", target)
+    que.put.assert_called_once_with(
+        (
+            {
+                "id": "localhost",
+                "ret": {
+                    "stdout": "",
+                    "stderr": "Something went seriously wrong",
+                    "retcode": 1,
+                },
+            },
+            1,
+        )
+    )
+    assert "Got an invalid retcode for host 'localhost': 'None'" in caplog.text