diff --git a/changelogs/fragments/win_reboot-fixes.yml b/changelogs/fragments/win_reboot-fixes.yml new file mode 100644 index 0000000000..af469119c9 --- /dev/null +++ b/changelogs/fragments/win_reboot-fixes.yml @@ -0,0 +1,4 @@ +bugfixes: +- win_reboot - handle post reboots when running test_command - https://github.com/ansible/ansible/issues/41713 +- win_reboot - fix issue when overridding connection timeout hung the post reboot uptime check - https://github.com/ansible/ansible/issues/42185 https://github.com/ansible/ansible/issues/42294 +- win_reboot - fix for handling an already scheduled reboot and other minor log formatting issues diff --git a/lib/ansible/plugins/action/win_reboot.py b/lib/ansible/plugins/action/win_reboot.py index 015c15ef97..8b7893b635 100644 --- a/lib/ansible/plugins/action/win_reboot.py +++ b/lib/ansible/plugins/action/win_reboot.py @@ -56,7 +56,7 @@ class ActionModule(ActionBase): except Exception as e: exc = e if what_desc: - display.debug("win_reboot: %s fail (expected), retrying in %d seconds..." % (what_desc, fail_sleep)) + display.debug("win_reboot: %s fail '%s' (expected), retrying in %d seconds..." % (what_desc, to_native(e), fail_sleep)) time.sleep(fail_sleep) raise TimedOutException("timed out waiting for %s: %s" % (what_desc, exc)) @@ -124,7 +124,7 @@ class ActionModule(ActionBase): (rc, stdout, stderr) = self._connection.exec_command('shutdown /r /t %d /c "%s"' % (pre_reboot_delay, msg)) # Test for "A system shutdown has already been scheduled. (1190)" and handle it gracefully - if rc == 1190: + if rc == 1190 or (rc != 0 and b"(1190)" in stderr): display.warning('A scheduled reboot was pre-empted by Ansible.') # Try to abort (this may fail if it was already aborted) @@ -138,7 +138,7 @@ class ActionModule(ActionBase): if rc != 0: result['failed'] = True result['rebooted'] = False - result['msg'] = "Shutdown command failed, error text was %s" % stderr + result['msg'] = "Shutdown command failed, error text was '%s'" % to_native(stderr) return result start = datetime.now() @@ -156,10 +156,12 @@ class ActionModule(ActionBase): # override connection timeout from defaults to custom value try: - self._connection.set_options(direct={"connection_timeout": connect_timeout}) + self._connection.set_option("connection_timeout", + connect_timeout) self._connection._reset() except AttributeError: - display.warning("Connection plugin does not allow the connection timeout to be overridden") + display.warning("Connection plugin does not allow the " + "connection timeout to be overridden") # try and get uptime try: @@ -174,18 +176,30 @@ class ActionModule(ActionBase): # reset the connection to clear the custom connection timeout try: - self._connection.set_options(direct={"connection_timeout": connection_timeout_orig}) + self._connection.set_option("connection_timeout", + connection_timeout_orig) self._connection._reset() - except (AnsibleError, AttributeError): - display.debug("Failed to reset connection_timeout back to default") + except (AnsibleError, AttributeError) as e: + display.debug("Failed to reset connection_timeout back to default: %s" % to_native(e)) # finally run test command to ensure everything is working def run_test_command(): display.vvv("attempting post-reboot test command '%s'" % test_command) - (rc, stdout, stderr) = self._connection.exec_command(test_command) - - if rc != 0: - raise Exception('test command failed') + try: + (rc, stdout, stderr) = self._connection.exec_command(test_command) + except Exception as e: + # in case of a failure trying to execute the command + # (another reboot occurred) we need to reset the connection + # to make sure we are not re-using the same shell id + try: + self._connection._reset() + except AttributeError: + pass + raise + else: + if rc != 0: + raise Exception("test command failed, stdout: '%s', stderr: '%s', rc: %d" + % (stdout, stderr, rc)) # FUTURE: add a stability check (system must remain up for N seconds) to deal with self-multi-reboot updates diff --git a/lib/ansible/plugins/connection/winrm.py b/lib/ansible/plugins/connection/winrm.py index 28aa5a62d7..3daa06e0c4 100644 --- a/lib/ansible/plugins/connection/winrm.py +++ b/lib/ansible/plugins/connection/winrm.py @@ -181,12 +181,11 @@ class Connection(ConnectionBase): super(Connection, self).__init__(*args, **kwargs) - def set_options(self, task_keys=None, var_options=None, direct=None): - if not HAS_WINRM: - return - - super(Connection, self).set_options(task_keys=None, var_options=var_options, direct=direct) - + def _build_winrm_kwargs(self): + # this used to be in set_options, as win_reboot needs to be able to + # override the conn timeout, we need to be able to build the args + # after setting individual options. This is called by _connect before + # starting the WinRM connection self._winrm_host = self.get_option('remote_addr') self._winrm_user = self.get_option('remote_user') self._winrm_pass = self._play_context.password @@ -479,6 +478,7 @@ class Connection(ConnectionBase): super(Connection, self)._connect() if not self.protocol: + self._build_winrm_kwargs() # build the kwargs from the options set self.protocol = self._winrm_connect() self._connected = True return self diff --git a/test/integration/targets/win_reboot/aliases b/test/integration/targets/win_reboot/aliases new file mode 100644 index 0000000000..0589efba85 --- /dev/null +++ b/test/integration/targets/win_reboot/aliases @@ -0,0 +1,2 @@ +windows/ci/group1 +windows/ci/smoketest diff --git a/test/integration/targets/win_reboot/tasks/main.yml b/test/integration/targets/win_reboot/tasks/main.yml new file mode 100644 index 0000000000..68ce7db812 --- /dev/null +++ b/test/integration/targets/win_reboot/tasks/main.yml @@ -0,0 +1,73 @@ +--- +- name: reboot with defaults + win_reboot: + +- name: schedule a reboot for sometime in the future + win_command: shutdown.exe /r /t 599 + +- name: reboot with a shutdown already scheduled + win_reboot: + +# test a reboot that reboots again during the test_command phase +- name: create test file + win_file: + path: '{{win_output_dir}}\win_reboot_test' + state: touch + +- name: reboot with secondary reboot stage + win_reboot: + test_command: powershell.exe -NoProfile -EncodedCommand {{lookup('template', 'post_reboot.ps1')|b64encode(encoding='utf-16-le')}} + +# try and reboot the host with a non admin user, we expect an error here +# this requires a bit of setup to create the user and allow it to connect +# over WinRM +- name: create password fact + set_fact: + standard_user: ansible_user_test + standard_pass: password123! + {{ lookup('password', '/dev/null chars=ascii_letters,digits length=8') }} + +- name: get original SDDL for WinRM listener + win_shell: (Get-Item -Path WSMan:\localhost\Service\RootSDDL).Value + register: original_sddl + +- name: create standard user + win_user: + name: '{{standard_user}}' + password: '{{standard_pass}}' + update_password: always + groups: Users + state: present + register: user_res + +- name: add standard user to WinRM listener + win_shell: | + $sid = New-Object -TypeName System.Security.Principal.SecurityIdentifier -ArgumentList "{{user_res.sid}}" + $sd = New-Object -TypeName System.Security.AccessControl.CommonSecurityDescriptor -ArgumentList $false, $false, "{{original_sddl.stdout_lines[0]}}" + $sd.DiscretionaryAcl.AddAccess( + [System.Security.AccessControl.AccessControlType]::Allow, + $sid, + (0x80000000 -bor 0x20000000), + [System.Security.AccessControl.InheritanceFlags]::None, + [System.Security.AccessControl.PropagationFlags]::None + ) + $new_sddl = $sd.GetSddlForm([System.Security.AccessControl.AccessControlSections]::All) + Set-Item -Path WSMan:\localhost\Service\RootSDDL -Value $new_sddl -Force + +- block: + - name: fail to reboot with non admin user + win_reboot: + vars: + ansible_user: '{{standard_user}}' + ansible_password: '{{standard_pass}}' + ansible_winrm_transport: ntlm + register: fail_shutdown + failed_when: fail_shutdown.msg != "Shutdown command failed, error text was 'Access is denied.(5)\n'" + + always: + - name: set the original SDDL to the WinRM listener + win_shell: Set-Item -Path WSMan:\localhost\Service\RootSDDL -Value "{{original_sddl.stdout_lines[0]}}" -Force + + - name: remove standard user + win_user: + name: '{{standard_user}}' + state: absent diff --git a/test/integration/targets/win_reboot/templates/post_reboot.ps1 b/test/integration/targets/win_reboot/templates/post_reboot.ps1 new file mode 100644 index 0000000000..e4a99a721d --- /dev/null +++ b/test/integration/targets/win_reboot/templates/post_reboot.ps1 @@ -0,0 +1,8 @@ +if (Test-Path -Path '{{win_output_dir}}\win_reboot_test') { + New-ItemProperty -Path 'HKLM:\SYSTEM\CurrentControlSet\Control\Session Manager' ` + -Name PendingFileRenameOperations ` + -Value @("\??\{{win_output_dir}}\win_reboot_test`0") ` + -PropertyType MultiString + Restart-Computer -Force + exit 1 +} diff --git a/test/units/plugins/connection/test_winrm.py b/test/units/plugins/connection/test_winrm.py index bfd27743b8..e906a9d2b3 100644 --- a/test/units/plugins/connection/test_winrm.py +++ b/test/units/plugins/connection/test_winrm.py @@ -204,6 +204,7 @@ class TestConnectionWinRM(object): conn = connection_loader.get('winrm', pc, new_stdin) conn.set_options(var_options=options, direct=direct) + conn._build_winrm_kwargs() for attr, expected in expected.items(): actual = getattr(conn, attr) @@ -236,6 +237,7 @@ class TestWinRMKerbAuth(object): new_stdin = StringIO() conn = connection_loader.get('winrm', pc, new_stdin) conn.set_options(var_options=options) + conn._build_winrm_kwargs() conn._kerb_auth("user@domain", "pass") mock_calls = mock_popen.mock_calls @@ -264,6 +266,7 @@ class TestWinRMKerbAuth(object): new_stdin = StringIO() conn = connection_loader.get('winrm', pc, new_stdin) conn.set_options(var_options=options) + conn._build_winrm_kwargs() conn._kerb_auth("user@domain", "pass") mock_calls = mock_pexpect.mock_calls @@ -292,6 +295,7 @@ class TestWinRMKerbAuth(object): conn = connection_loader.get('winrm', pc, new_stdin) options = {"_extras": {}, "ansible_winrm_kinit_cmd": "/fake/kinit"} conn.set_options(var_options=options) + conn._build_winrm_kwargs() with pytest.raises(AnsibleConnectionFailure) as err: conn._kerb_auth("user@domain", "pass") @@ -314,6 +318,7 @@ class TestWinRMKerbAuth(object): conn = connection_loader.get('winrm', pc, new_stdin) options = {"_extras": {}, "ansible_winrm_kinit_cmd": "/fake/kinit"} conn.set_options(var_options=options) + conn._build_winrm_kwargs() with pytest.raises(AnsibleConnectionFailure) as err: conn._kerb_auth("user@domain", "pass") @@ -337,6 +342,7 @@ class TestWinRMKerbAuth(object): new_stdin = StringIO() conn = connection_loader.get('winrm', pc, new_stdin) conn.set_options(var_options={"_extras": {}}) + conn._build_winrm_kwargs() with pytest.raises(AnsibleConnectionFailure) as err: conn._kerb_auth("invaliduser", "pass") @@ -361,6 +367,7 @@ class TestWinRMKerbAuth(object): new_stdin = StringIO() conn = connection_loader.get('winrm', pc, new_stdin) conn.set_options(var_options={"_extras": {}}) + conn._build_winrm_kwargs() with pytest.raises(AnsibleConnectionFailure) as err: conn._kerb_auth("invaliduser", "pass") @@ -383,6 +390,7 @@ class TestWinRMKerbAuth(object): new_stdin = StringIO() conn = connection_loader.get('winrm', pc, new_stdin) conn.set_options(var_options={"_extras": {}}) + conn._build_winrm_kwargs() with pytest.raises(AnsibleConnectionFailure) as err: conn._kerb_auth("username", "password") @@ -407,6 +415,7 @@ class TestWinRMKerbAuth(object): new_stdin = StringIO() conn = connection_loader.get('winrm', pc, new_stdin) conn.set_options(var_options={"_extras": {}}) + conn._build_winrm_kwargs() with pytest.raises(AnsibleConnectionFailure) as err: conn._kerb_auth("username", "password")