From 0e8a77520c904b320c55737750b4d9ffbb987ecb Mon Sep 17 00:00:00 2001 From: jhawkesworth Date: Tue, 30 Apr 2019 21:19:56 +0100 Subject: [PATCH] Refactor of win_xml (2nd attempt) to add support for processing multiple nodes and counting nodes matched by xpath (#53362) * add multi-node manipulation, delete on xpath match only and count capability to win_xml * fix pep8 and yamllint errors identified by ci tests * fixed bugs when handling multiple elements, multiple attribute nodes and handling for attribute nodes when using xpaths that only select attributes like //@lang. Added more tests and tweaked documentation. * fixed line-too-long error * fixed trailing space errors * trailing whitespace expunged * bump version_added to 2.9 for new changes * fix PSAvoidUsingPositionalParameters sanity check failure * refix sanity check as it broke the msg return value --- changelogs/fragments/win_xml_refactor.yaml | 5 + lib/ansible/modules/windows/win_xml.ps1 | 232 ++++++++++-------- lib/ansible/modules/windows/win_xml.py | 95 +++++-- .../targets/win_xml/files/books.xml | 10 + .../targets/win_xml/tasks/main.yml | 163 ++++++++++++ 5 files changed, 383 insertions(+), 122 deletions(-) create mode 100644 changelogs/fragments/win_xml_refactor.yaml create mode 100644 test/integration/targets/win_xml/files/books.xml diff --git a/changelogs/fragments/win_xml_refactor.yaml b/changelogs/fragments/win_xml_refactor.yaml new file mode 100644 index 0000000000..6cbab6e9c0 --- /dev/null +++ b/changelogs/fragments/win_xml_refactor.yaml @@ -0,0 +1,5 @@ +minor_changes: + - win_xml - Behaviour change, module now processes all nodes specified by xpath, not just first encountered. + - win_xml - Behaviour change, fragment no longer required when processing element type nodes and state=absent. + - win_xml - Some output messages worded differently now the module uses a generic method to save changes. + - win_xml - Added 'count' module parameter which will return number of nodes matched by xpath if set to yes/true diff --git a/lib/ansible/modules/windows/win_xml.ps1 b/lib/ansible/modules/windows/win_xml.ps1 index 47ea6e2703..d051a3f67d 100644 --- a/lib/ansible/modules/windows/win_xml.ps1 +++ b/lib/ansible/modules/windows/win_xml.ps1 @@ -80,6 +80,22 @@ function Compare-XmlDocs($actual, $expected) { } } + +function Save-ChangedXml($xmlorig, $result, $message, $check_mode, $backup) { + $result.changed = $true + if (-Not $check_mode) { + if ($backup) { + $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode + # Ensure backward compatibility (deprecate in future) + $result.backup = $result.backup_file + } + $xmlorig.Save($dest) + $result.msg = $message + } else { + $result.msg += " check mode" + } +} + $params = Parse-Args $args -supports_check_mode $true $check_mode = Get-AnsibleParam -obj $params -name "_ansible_check_mode" -type "bool" -default $false @@ -87,12 +103,13 @@ $debug_level = Get-AnsibleParam -obj $params -name "_ansible_verbosity" -type "i $debug = $debug_level -gt 2 $dest = Get-AnsibleParam $params "path" -type "path" -FailIfEmpty $true -aliases "dest", "file" -$fragment = Get-AnsibleParam $params "fragment" -type "str" -FailIfEmpty $true -aliases "xmlstring" +$fragment = Get-AnsibleParam $params "fragment" -type "str" -aliases "xmlstring" $xpath = Get-AnsibleParam $params "xpath" -type "str" -FailIfEmpty $true -$backup = Get-AnsibleParam $params "backup" -type "bool" -default $false +$backup = Get-AnsibleParam $params "backup" -type "bool" -Default $false $type = Get-AnsibleParam $params "type" -type "str" -Default "element" -ValidateSet "element", "attribute", "text" $attribute = Get-AnsibleParam $params "attribute" -type "str" -FailIfEmpty ($type -eq "attribute") $state = Get-AnsibleParam $params "state" -type "str" -Default "present" +$count = Get-AnsibleParam $params "count" -type "bool" -Default $false $result = @{ changed = $false @@ -117,121 +134,132 @@ $localname = $xmlorig.DocumentElement.LocalName $namespaceMgr.AddNamespace($xmlorig.$localname.SchemaInfo.Prefix, $namespace) +$nodeList = $xmlorig.SelectNodes($xpath, $namespaceMgr) +$nodeListCount = $nodeList.get_Count() +if ($count) { + $result.count = $nodeListCount + if (-not $fragment) { + Exit-Json $result + } +} +## Exit early if xpath did not match any nodes +if ($nodeListCount -eq 0) { + $result.msg = "The supplied xpath did not match any nodes. If this is unexpected, check your xpath is valid for the xml file at supplied path." + Exit-Json $result +} + +$changed = $false +$result.msg = "not changed" + if ($type -eq "element") { - $xmlchild = $null - Try { - $xmlchild = [xml]$fragment - } Catch { - Fail-Json $result "Failed to parse fragment as XML: $($_.Exception.Message)" - } - - $child = $xmlorig.CreateElement($xmlchild.get_DocumentElement().get_Name(), $xmlorig.get_DocumentElement().get_NamespaceURI()) - Copy-Xml -dest $child -src $xmlchild.DocumentElement -xmlorig $xmlorig - - $node = $xmlorig.SelectSingleNode($xpath, $namespaceMgr) - if ($node.get_NodeType() -eq "Document") { - $node = $node.get_DocumentElement() - } - $elements = $node.get_ChildNodes() - [bool]$present = $false - [bool]$changed = $false - if ($elements.get_Count()) { - if ($debug) { - $err = @() - $result.err = {$err}.Invoke() - } - foreach ($element in $elements) { - try { - Compare-XmlDocs $child $element - $present = $true - break - } catch { + if ($state -eq "absent") { + foreach ($node in $nodeList) { + # there are some nodes that match xpath, delete without comparing them to fragment + if (-Not $check_mode) { + $removedNode = $node.get_ParentNode().RemoveChild($node) + $changed = $true if ($debug) { - $result.err.Add($_.Exception.ToString()) + $result.removed += $result.removed + $removedNode.get_OuterXml() } } } - if (!$present -and ($state -eq "present")) { - [void]$node.AppendChild($child) - $result.msg = "xml added" - $changed = $true - } elseif ($present -and ($state -eq "absent")) { - [void]$node.RemoveChild($element) - $result.msg = "xml removed" - $changed = $true + } else { # state -eq 'present' + $xmlfragment = $null + Try { + $xmlfragment = [xml]$fragment + } Catch { + Fail-Json $result "Failed to parse fragment as XML: $($_.Exception.Message)" } - } else { - if ($state -eq "present") { - [void]$node.AppendChild($child) - $result.msg = "xml added" - $changed = $true - } - } - if ($changed) { - if ($backup) { - $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode - # Ensure backward compatibility (deprecate in future) - $result.backup = $result.backup_file + foreach ($node in $nodeList) { + $candidate = $xmlorig.CreateElement($xmlfragment.get_DocumentElement().get_Name(), $xmlorig.get_DocumentElement().get_NamespaceURI()) + Copy-Xml -dest $candidate -src $xmlfragment.DocumentElement -xmlorig $xmlorig + + if ($node.get_NodeType() -eq "Document") { + $node = $node.get_DocumentElement() + } + $elements = $node.get_ChildNodes() + [bool]$present = $false + [bool]$changed = $false + $element_count = $elements.get_Count() + $nstatus = "node: " + $node.get_Value() + " element: " + $elements.get_OuterXml() + " Element count is $element_count" + Add-Warning $result $nstatus + if ($elements.get_Count()) { + if ($debug) { + $err = @() + $result.err = {$err}.Invoke() + } + foreach ($element in $elements) { + $estatus = "element is " + $element.get_OuterXml() + Add-Warning $result $estatus + try { + Compare-XmlDocs $candidate $element + $present = $true + break + } catch { + if ($debug) { + $result.err.Add($_.Exception.ToString()) + } + } + } + if (-Not $present -and ($state -eq "present")) { + [void]$node.AppendChild($candidate) + $result.msg = $result.msg + "xml added " + $changed = $true + } + } } - if (-not $check_mode) { - $xmlorig.Save($dest) - } - $result.changed = $true - } else { - $result.msg = "not changed" } } elseif ($type -eq "text") { - $node = $xmlorig.SelectSingleNode($xpath, $namespaceMgr) - [bool]$add = ($node.get_InnerText() -ne $fragment) - if ($add) { - if ($backup) { - $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode - # Ensure backward compatibility (deprecate in future) - $result.backup = $result.backup_file + foreach ($node in $nodeList) { + if ($node.get_InnerText() -ne $fragment) { + $node.set_InnerText($fragment) + $changed = $true } - $node.set_InnerText($fragment) - if (-not $check_mode) { - $xmlorig.Save($dest) - } - $result.changed = $true - $result.msg = "text changed" - } else { - $result.msg = "not changed" } } elseif ($type -eq "attribute") { - $node = $xmlorig.SelectSingleNode($xpath, $namespaceMgr) - [bool]$add = !$node.HasAttribute($attribute) -Or ($node.$attribute -ne $fragment) - if ($add -And ($state -eq "present")) { - if ($backup) { - $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode - # Ensure backward compatibility (deprecate in future) - $result.backup = $result.backup_file + foreach ($node in $nodeList) { + if ($state -eq 'present') { + if ($node.NodeType -eq 'Attribute') { + if ($node.Name -eq $attribute -and $node.Value -ne $fragment ) { + # this is already the attribute with the right name, so just set its value to match fragment + $node.Value = $fragment + $changed = $true + } + } else { # assume NodeType is Element + if ($node.$attribute -ne $fragment) { + if (!$node.HasAttribute($attribute)) { # add attribute to Element if missing + $node.SetAttributeNode($attribute, $xmlorig.get_DocumentElement().get_NamespaceURI()) + } + #set the attribute into the element + $node.SetAttribute($attribute, $fragment) + $changed = $true + } + } + } elseif ($state -eq 'absent') { + if ($node.NodeType -eq 'Attribute') { + $attrNode = [System.Xml.XmlAttribute]$node + $parent = $attrNode.OwnerElement + $parent.RemoveAttribute($attribute) + $changed = $true + } else { # element node processing + if ($node.Name -eq $attribute ) { # note not caring about the state of 'fragment' at this point + $node.RemoveAttribute($attribute) + $changed = $true + } + } + } else { + Add-Warning $result "Unexpected state when processing attribute $($attribute), add was $add, state was $state" } - if (!$node.HasAttribute($attribute)) { - $node.SetAttributeNode($attribute, $xmlorig.get_DocumentElement().get_NamespaceURI()) - } - $node.SetAttribute($attribute, $fragment) - if (-not $check_mode) { - $xmlorig.Save($dest) - } - $result.changed = $true - $result.msg = "text changed" - } elseif (!$add -And ($state -eq "absent")) { - if ($backup) { - $result.backup_file = Backup-File -path $dest -WhatIf:$check_mode - # Ensure backward compatibility (deprecate in future) - $result.backup = $result.backup_file - } - $node.RemoveAttribute($attribute) - if (-not $check_mode) { - $xmlorig.Save($dest) - } - $result.changed = $true - $result.msg = "text changed" - } else { - $result.msg = "not changed" } } +if ($changed) { + if ($state -eq "absent") { + $summary = "$type removed" + } else { + $summary = "$type changed" + } + Save-ChangedXml -xmlorig $xmlorig -result $result -message $summary -check_mode $check_mode -backup $backup +} Exit-Json $result diff --git a/lib/ansible/modules/windows/win_xml.py b/lib/ansible/modules/windows/win_xml.py index 3f24c09a58..a503df81d8 100644 --- a/lib/ansible/modules/windows/win_xml.py +++ b/lib/ansible/modules/windows/win_xml.py @@ -15,28 +15,23 @@ DOCUMENTATION = r''' --- module: win_xml version_added: "2.7" -short_description: Add XML fragment to an XML parent +short_description: Manages XML file content on Windows hosts description: - - Adds XML fragments formatted as strings to existing XML on remote servers. + - Manages XML nodes, attributes and text, using xpath to select which xml nodes need to be managed. + - XML fragments, formatted as strings, are used to specify the desired state of a part or parts of XML files on remote Windows servers. - For non-Windows targets, use the M(xml) module instead. options: - path: + attribute: description: - - The path of remote servers XML. - type: path - required: true - aliases: [ dest, file ] - fragment: - description: - - The string representation of the XML fragment to be added. + - The attribute name if the type is 'attribute'. + - Required if C(type=attribute). type: str - required: true - aliases: [ xmlstring ] - xpath: + count: description: - - The node of the remote server XML where the fragment will go. - type: str - required: true + - When set to C(yes), return the number of nodes matched by I(xpath). + type: bool + default: false + version_added: 2.9 backup: description: - Determine whether a backup should be created. @@ -44,20 +39,49 @@ options: so you can get the original file back if you somehow clobbered it incorrectly. type: bool default: no + fragment: + description: + - The string representation of the XML fragment expected at xpath. Since ansible 2.9 not required when I(state=absent), or when I(count=yes). + type: str + required: false + aliases: [ xmlstring ] + path: + description: + - Path to the file to operate on. + type: path + required: true + aliases: [ dest, file ] + state: + description: + - Set or remove the nodes (or attributes) matched by I(xpath). + type: str + default: present + choices: [ present, absent ] + version_added: 2.9 type: description: - - The type of XML you are working with. + - The type of XML node you are working with. type: str required: yes default: element choices: [ attribute, element, text ] - attribute: + xpath: description: - - The attribute name if the type is 'attribute'. - - Required if C(type=attribute). + - Xpath to select the node or nodes to operate on. type: str + required: true author: - Richard Levenberg (@richardcs) + - Jon Hawkesworth (@jhawkesworth) +notes: + - Only supports operating on xml elements, attributes and text. + - Namespace, processing-instruction, command and document node types cannot be modified with this module. +seealso: + - module: xml + description: XML manipulation for Posix hosts. + - name: w3shools XPath tutorial + description: A useful tutorial on XPath + link: https://www.w3schools.com/xml/xpath_intro.asp ''' EXAMPLES = r''' @@ -74,6 +98,32 @@ EXAMPLES = r''' attribute: 'sslEnabledProtocols' fragment: 'TLSv1,TLSv1.1,TLSv1.2' type: attribute + +- name: remove debug configuration nodes from nlog.conf + win_xml: + path: C:\IISApplication\nlog.conf + xpath: /nlog/rules/logger[@name="debug"]/descendant::* + state: absent + +- name: count configured connectors in Tomcat's server.xml + win_xml: + path: C:\Tomcat\conf\server.xml + xpath: //Server/Service/Connector + count: yes + register: connector_count + +- name: show connector count + debug: + msg="Connector count is {{connector_count.count}}" + +- name: ensure all lang=en attributes to lang=nl + win_xml: + path: C:\Data\Books.xml + xpath: //@[lang="en"] + attribute: lang + fragment: nl + type: attribute + ''' RETURN = r''' @@ -82,6 +132,11 @@ backup_file: returned: if backup=yes type: str sample: C:\Path\To\File.txt.11540.20150212-220915.bak +count: + description: Number of nodes matched by xpath. + returned: if count=yes + type: int + sample: 33 msg: description: What was done. returned: always diff --git a/test/integration/targets/win_xml/files/books.xml b/test/integration/targets/win_xml/files/books.xml new file mode 100644 index 0000000000..e38ee15d4e --- /dev/null +++ b/test/integration/targets/win_xml/files/books.xml @@ -0,0 +1,10 @@ + + + + A Great Book + Best Book Ever + Worst Book Ever + Another Book + Worst Book Ever Two + + diff --git a/test/integration/targets/win_xml/tasks/main.yml b/test/integration/targets/win_xml/tasks/main.yml index 77a9b9aa63..d3d98693ff 100644 --- a/test/integration/targets/win_xml/tasks/main.yml +++ b/test/integration/targets/win_xml/tasks/main.yml @@ -142,3 +142,166 @@ assert: that: - sha1_checksum.stat.checksum == 'de86f79b409383447cf4cf112b20af8ffffcfdbf' + +# features added ansible 2.8 +# count + +- name: count logger nodes in log4j.xml + win_xml: + path: "{{ win_output_dir }}\\log4j.xml" + xpath: //logger + count: yes + register: logger_node_count + +- name: verify node count + assert: + that: + - logger_node_count.count == 5 + +# multiple attribute change +- name: ensure //logger/level value attributes are set to debug + win_xml: + path: "{{ win_output_dir }}\\log4j.xml" + xpath: '//logger/level[@value="error"]' + type: attribute + attribute: value + fragment: debug + count: yes + register: logger_level_value_attrs + +- name: verify //logger/level value attributes + assert: + that: + - logger_level_value_attrs.count == 4 + - logger_level_value_attrs.changed == true + - logger_level_value_attrs.msg == 'attribute changed' + +- name: ensure //logger/level value attributes are set to debug (idempotency) + win_xml: + path: "{{ win_output_dir }}\\log4j.xml" + xpath: '//logger/level[@value="error"]' + type: attribute + attribute: value + fragment: debug + count: yes + register: logger_level_value_attrs_again + +- name: verify //logger/level value attributes again (idempotency) + assert: + that: + - logger_level_value_attrs_again.count == 0 + - logger_level_value_attrs_again.changed == false + - logger_level_value_attrs_again.msg == 'The supplied xpath did not match any nodes. If this is unexpected, check your xpath is valid for the xml file at supplied path.' + +# multiple text nodes +- name: ensure test books.xml is present + win_copy: + src: books.xml + dest: '{{ win_output_dir }}\books.xml' + +- name: demonstrate multi text replace by replacing all title text elements + win_xml: + path: '{{ win_output_dir }}\books.xml' + xpath: //works/title + type: text + fragment: _TITLE_TEXT_REMOVED_BY_WIN_XML_MODULE_ + count: yes + register: multi_text + +- name: verify multi text change + assert: + that: + - multi_text.changed == true + - multi_text.count == 5 + - multi_text.msg == 'text changed' + +- name: demonstrate multi text replace by replacing all title text elements again (idempotency) + win_xml: + path: '{{ win_output_dir }}\books.xml' + xpath: //works/title + type: text + fragment: _TITLE_TEXT_REMOVED_BY_WIN_XML_MODULE_ + count: yes + register: multi_text_again + +- name: verify multi text again change (idempotency) + assert: + that: + - multi_text_again.changed == false + - multi_text_again.count == 5 + - multi_text_again.msg == 'not changed' + +# multiple element + +#- name: ensure a fresh test books.xml is present +# win_copy: +# src: books.xml +# dest: '{{ win_output_dir }}\books.xml' + +- name: demonstrate multi element should append new information element from fragment + win_xml: + path: '{{ win_output_dir }}\books.xml' + xpath: //works/title + type: element + fragment: This element added by ansible + count: yes + register: multi_element + +- name: verify multi element + assert: + that: + - multi_element.changed == true + - multi_element.count == 5 + - multi_element.msg == 'element changed' + +- name: demonstrate multi element unchanged (idempotency) + win_xml: + path: '{{ win_output_dir }}\books.xml' + xpath: //works/title + type: element + fragment: This element added by ansible + count: yes + register: multi_element_again + +- name: verify multi element again (idempotency) + assert: + that: + - multi_element_again.changed == false + - multi_element_again.count == 5 + - multi_element_again.msg == 'not changed' + +# multiple attributes on differing parent nodes + +- name: ensure all attribute lang=nl + win_xml: + path: '{{ win_output_dir }}\books.xml' + xpath: //@lang + type: attribute + attribute: lang + fragment: nl + count: yes + register: multi_attr + +- name: verify multi attribute + assert: + that: + - multi_attr.changed == true + - multi_attr.count == 6 + - multi_attr.msg == 'attribute changed' + +- name: ensure all attribute lang=nl (idempotency) + win_xml: + path: '{{ win_output_dir }}\books.xml' + xpath: //@lang + type: attribute + attribute: lang + fragment: nl + count: yes + register: multi_attr_again + +- name: verify multi attribute (idempotency) + assert: + that: + - multi_attr_again.changed == false + - multi_attr_again.count == 6 + - multi_attr_again.msg == 'not changed'