From b8f1add9830578d94a052abe48931e976b292793 Mon Sep 17 00:00:00 2001 From: Jordan Borean Date: Mon, 3 Jun 2024 13:17:36 +1000 Subject: [PATCH] powershell - Improve Add-Type tempdir handler (#83080) Improves the Add-Type temporary directory handler to include a retry mechanism and not fail on an error. Deleting a temporary file used in compilation is not a critical error and should improve the reliability of Ansible on Windows hosts. --- .../fragments/PowerShell-AddType-temp.yml | 7 +++ .../executor/powershell/exec_wrapper.ps1 | 1 + .../powershell/module_powershell_wrapper.ps1 | 13 ++++- .../module_utils/csharp/Ansible.Basic.cs | 11 +++- .../Ansible.ModuleUtils.AddType.psm1 | 35 ++++++++++-- .../library/ansible_basic_tests.ps1 | 31 +++++++++++ .../targets/win_exec_wrapper/tasks/main.yml | 55 ++++++++++++++++--- 7 files changed, 137 insertions(+), 16 deletions(-) create mode 100644 changelogs/fragments/PowerShell-AddType-temp.yml diff --git a/changelogs/fragments/PowerShell-AddType-temp.yml b/changelogs/fragments/PowerShell-AddType-temp.yml new file mode 100644 index 00000000000..6019f8058ed --- /dev/null +++ b/changelogs/fragments/PowerShell-AddType-temp.yml @@ -0,0 +1,7 @@ +bugfixes: +- >- + powershell - Implement more robust deletion mechanism for C# code compilation + temporary files. This should avoid scenarios where the underlying temporary + directory may be temporarily locked by antivirus tools or other IO problems. + A failure to delete one of these temporary directories will result in a + warning rather than an outright failure. diff --git a/lib/ansible/executor/powershell/exec_wrapper.ps1 b/lib/ansible/executor/powershell/exec_wrapper.ps1 index 0f97bdfb8a5..cce99abc77f 100644 --- a/lib/ansible/executor/powershell/exec_wrapper.ps1 +++ b/lib/ansible/executor/powershell/exec_wrapper.ps1 @@ -178,6 +178,7 @@ $($ErrorRecord.InvocationInfo.PositionMessage) Write-AnsibleLog "INFO - converting json raw to a payload" "exec_wrapper" $payload = ConvertFrom-AnsibleJson -InputObject $json_raw + $payload.module_args._ansible_exec_wrapper_warnings = [System.Collections.Generic.List[string]]@() # TODO: handle binary modules # TODO: handle persistence diff --git a/lib/ansible/executor/powershell/module_powershell_wrapper.ps1 b/lib/ansible/executor/powershell/module_powershell_wrapper.ps1 index c35c84cfc86..f79dd6fbc86 100644 --- a/lib/ansible/executor/powershell/module_powershell_wrapper.ps1 +++ b/lib/ansible/executor/powershell/module_powershell_wrapper.ps1 @@ -29,7 +29,18 @@ if ($csharp_utils.Count -gt 0) { # add any C# references so the module does not have to do so $new_tmp = [System.Environment]::ExpandEnvironmentVariables($Payload.module_args["_ansible_remote_tmp"]) - Add-CSharpType -References $csharp_utils -TempPath $new_tmp -IncludeDebugInfo + + # We use a fake module object to capture warnings + $fake_module = [PSCustomObject]@{ + Tmpdir = $new_tmp + Verbosity = 3 + } + $warning_func = New-Object -TypeName System.Management.Automation.PSScriptMethod -ArgumentList Warn, { + param($message) + $Payload.module_args._ansible_exec_wrapper_warnings.Add($message) + } + $fake_module.PSObject.Members.Add($warning_func) + Add-CSharpType -References $csharp_utils -AnsibleModule $fake_module } if ($Payload.ContainsKey("coverage") -and $null -ne $host.Runspace -and $null -ne $host.Runspace.Debugger) { diff --git a/lib/ansible/module_utils/csharp/Ansible.Basic.cs b/lib/ansible/module_utils/csharp/Ansible.Basic.cs index a042af8cecc..085958270d7 100644 --- a/lib/ansible/module_utils/csharp/Ansible.Basic.cs +++ b/lib/ansible/module_utils/csharp/Ansible.Basic.cs @@ -1025,7 +1025,16 @@ namespace Ansible.Basic foreach (DictionaryEntry entry in param) { string paramKey = (string)entry.Key; - if (!legalInputs.Contains(paramKey, StringComparer.OrdinalIgnoreCase)) + if (paramKey == "_ansible_exec_wrapper_warnings") + { + // Special key used in module_powershell_wrapper to pass + // along any warnings that should be returned back to + // Ansible. + removedParameters.Add(paramKey); + foreach (string warning in (IList)entry.Value) + Warn(warning); + } + else if (!legalInputs.Contains(paramKey, StringComparer.OrdinalIgnoreCase)) unsupportedParameters.Add(paramKey); else if (!legalInputs.Contains(paramKey)) // For backwards compatibility we do not care about the case but we need to warn the users as this will diff --git a/lib/ansible/module_utils/powershell/Ansible.ModuleUtils.AddType.psm1 b/lib/ansible/module_utils/powershell/Ansible.ModuleUtils.AddType.psm1 index f40c3384cbc..b18a9a1729b 100644 --- a/lib/ansible/module_utils/powershell/Ansible.ModuleUtils.AddType.psm1 +++ b/lib/ansible/module_utils/powershell/Ansible.ModuleUtils.AddType.psm1 @@ -75,7 +75,7 @@ Function Add-CSharpType { [Switch]$IgnoreWarnings, [Switch]$PassThru, [Parameter(Mandatory = $true, ParameterSetName = "Module")][Object]$AnsibleModule, - [Parameter(ParameterSetName = "Manual")][String]$TempPath = $env:TMP, + [Parameter(ParameterSetName = "Manual")][String]$TempPath, [Parameter(ParameterSetName = "Manual")][Switch]$IncludeDebugInfo, [String[]]$CompileSymbols = @() ) @@ -280,9 +280,11 @@ Function Add-CSharpType { $include_debug = $AnsibleModule.Verbosity -ge 3 } else { - $temp_path = $TempPath + $temp_path = [System.IO.Path]::GetTempPath() $include_debug = $IncludeDebugInfo.IsPresent } + $temp_path = Join-Path -Path $temp_path -ChildPath ([Guid]::NewGuid().Guid) + $compiler_options = [System.Collections.ArrayList]@("/optimize") if ($defined_symbols.Count -gt 0) { $compiler_options.Add("/define:" + ([String]::Join(";", $defined_symbols.ToArray()))) > $null @@ -304,8 +306,12 @@ Function Add-CSharpType { ) # create a code snippet for each reference and check if we need - # to reference any extra assemblies - $ignore_warnings = [System.Collections.ArrayList]@() + # to reference any extra assemblies. + # CS1610 is a warning when csc.exe failed to delete temporary files. + # We use our own temp dir deletion mechanism so this doesn't become a + # fatal error. + # https://github.com/ansible-collections/ansible.windows/issues/598 + $ignore_warnings = [System.Collections.ArrayList]@('1610') $compile_units = [System.Collections.Generic.List`1[System.CodeDom.CodeSnippetCompileUnit]]@() foreach ($reference in $References) { # scan through code and add any assemblies that match @@ -373,7 +379,26 @@ Function Add-CSharpType { } } - $compile = $provider.CompileAssemblyFromDom($compile_parameters, $compile_units) + $null = New-Item -Path $temp_path -ItemType Directory -Force + try { + $compile = $provider.CompileAssemblyFromDom($compile_parameters, $compile_units) + } + finally { + # Try to delete the temp path, if this fails and we are running + # with a module object write a warning instead of failing. + try { + [System.IO.Directory]::Delete($temp_path, $true) + } + catch { + $msg = "Failed to cleanup temporary directory '$temp_path' used for compiling C# code." + if ($AnsibleModule) { + $AnsibleModule.Warn("$msg Files may still be present after the task is complete. Error: $_") + } + else { + throw "$msg Error: $_" + } + } + } } finally { foreach ($kvp in $originalEnv.GetEnumerator()) { diff --git a/test/integration/targets/module_utils_Ansible.Basic/library/ansible_basic_tests.ps1 b/test/integration/targets/module_utils_Ansible.Basic/library/ansible_basic_tests.ps1 index 625cb210874..27b0d107d77 100644 --- a/test/integration/targets/module_utils_Ansible.Basic/library/ansible_basic_tests.ps1 +++ b/test/integration/targets/module_utils_Ansible.Basic/library/ansible_basic_tests.ps1 @@ -1323,6 +1323,37 @@ test_no_log - Invoked with: $actual | Assert-DictionaryEqual -Expected $expected } + "Run with exec wrapper warnings" = { + Set-Variable -Name complex_args -Scope Global -Value @{ + _ansible_exec_wrapper_warnings = [System.Collections.Generic.List[string]]@( + 'Warning 1', + 'Warning 2' + ) + } + $m = [Ansible.Basic.AnsibleModule]::Create(@(), @{}) + $m.Warn("Warning 3") + + $failed = $false + try { + $m.ExitJson() + } + catch [System.Management.Automation.RuntimeException] { + $failed = $true + $_.Exception.Message | Assert-Equal -Expected "exit: 0" + $actual = [Ansible.Basic.AnsibleModule]::FromJson($_.Exception.InnerException.Output) + } + $failed | Assert-Equal -Expected $true + + $expected = @{ + changed = $false + invocation = @{ + module_args = @{} + } + warnings = @("Warning 1", "Warning 2", "Warning 3") + } + $actual | Assert-DictionaryEqual -Expected $expected + } + "FailJson with message" = { $m = [Ansible.Basic.AnsibleModule]::Create(@(), @{}) diff --git a/test/integration/targets/win_exec_wrapper/tasks/main.yml b/test/integration/targets/win_exec_wrapper/tasks/main.yml index f1342c480b1..75da3d62a64 100644 --- a/test/integration/targets/win_exec_wrapper/tasks/main.yml +++ b/test/integration/targets/win_exec_wrapper/tasks/main.yml @@ -194,16 +194,33 @@ become_test_username: ansible_become_test gen_pw: "{{ 'password123!' + lookup('password', '/dev/null chars=ascii_letters,digits length=8') }}" -- name: create unprivileged user - win_user: - name: "{{ become_test_username }}" - password: "{{ gen_pw }}" - update_password: always - groups: Users - register: become_test_user_result - - name: execute tests and ensure that test user is deleted regardless of success/failure block: + - name: create unprivileged user + win_user: + name: "{{ become_test_username }}" + password: "{{ gen_pw }}" + update_password: always + groups: Users + register: become_test_user_result + + - name: create tempdir for test user + win_file: + path: C:\Windows\TEMP\test-dir + state: directory + + - name: deny delete permissions on new temp dir for test user + win_acl: + path: C:\Windows\TEMP\test-dir + user: '{{ become_test_user_result.sid }}' + type: '{{ item.type }}' + rights: '{{ item.rights }}' + loop: + - type: allow + rights: ListDirectory, CreateFiles, CreateDirectories, ReadAttributes, ReadExtendedAttributes, WriteData, WriteAttributes, WriteExtendedAttributes, Synchronize + - type: deny + rights: DeleteSubdirectoriesAndFiles, Delete + - name: ensure current user is not the become user win_shell: whoami register: whoami_out @@ -238,6 +255,21 @@ - become_system is successful - become_system.output == become_test_user_result.sid + - name: run module with tempdir with no delete access + win_ping: + register: temp_deletion_warning + vars: + <<: *become_vars + ansible_remote_tmp: C:\Windows\TEMP\test-dir + + - name: assert warning about tmpdir deletion is present + assert: + that: + - temp_deletion_warning.warnings | count == 1 + - >- + temp_deletion_warning.warnings[0] is + regex("(?i).*Failed to cleanup temporary directory 'C:\\\\Windows\\\\TEMP\\\\test-dir\\\\.*' used for compiling C# code\\. Files may still be present after the task is complete\\..*") + always: - name: ensure test user is deleted win_user: @@ -249,7 +281,12 @@ win_shell: rmdir /S /Q {{ profile_dir_out.stdout_lines[0] }} args: executable: cmd.exe - when: become_test_username in profile_dir_out.stdout_lines[0] + when: become_test_username in profile_dir_out.stdout_lines[0] | default("") + + - name: remove test tempdir + win_file: + path: C:\Windows\TEMP\test-dir + state: absent - name: test common functions in exec test_common_functions: