win_reboot: fix 2.6 issues and better handle post reboot reboot (#42330)

* win_reboot: fix 2.6 issues and better handle post reboot reboot

* changed winrm _reset to reset

* Add handler to reset calls when .reset() throws an AnsibleError on older hosts

* Moving back to _reset to get the issue fixed

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

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):
+            except (AnsibleError, AttributeError) as e:
-                display.debug("Failed to reset connection_timeout back to default")
+                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)
+                try:
-
+                    (rc, stdout, stderr) = self._connection.exec_command(test_command)
-                if rc != 0:
+                except Exception as e:
-                    raise Exception('test command failed')
+                    # 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
 

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):
+    def _build_winrm_kwargs(self):
-        if not HAS_WINRM:
+        # this used to be in set_options, as win_reboot needs to be able to
-            return
+        # override the conn timeout, we need to be able to build the args
-
+        # after setting individual options. This is called by _connect before
-        super(Connection, self).set_options(task_keys=None, var_options=var_options, direct=direct)
+        # 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

test/integration/targets/win_reboot/aliases

@@ -0,0 +1,2 @@
+windows/ci/group1
+windows/ci/smoketest

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

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
+}

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")