From e1c90e679e3c13e52370a068a68359d119785304 Mon Sep 17 00:00:00 2001 From: djlongy Date: Sun, 1 Mar 2026 20:56:39 +1100 Subject: [PATCH] Fix copy_dict_to_element() losing alias-to-host association When copy_dict_to_element() processes a list (e.g. DNS resolver hosts), it pairs input items to XML elements by position. If a host with aliases (child elements under ) swaps position with a host that has no aliases, the scalar assignment branch sets the element text but does not remove the stale child elements. The alias-less host inherits the previous occupant's aliases. Add a guard in the scalar assignment branch: before setting .text, check if the element has child elements left over from a previous dict/list value and remove them. ET.Element.clear() is intentionally avoided because it also resets .tail, which would corrupt pretty-print whitespace. Add a regression test with a dedicated fixture containing two hosts (one with aliases, one without). The test provides hosts in reversed order and asserts that aliases remain with the correct parent after the module rewrites the XML. --- plugins/module_utils/pfsense.py | 22 +- ...ense_dns_resolver_config_hosts_aliases.xml | 251 ++++++++++++++++++ .../modules/test_pfsense_dns_resolver.py | 50 ++++ 3 files changed, 318 insertions(+), 5 deletions(-) create mode 100644 tests/unit/plugins/modules/fixtures/pfsense_dns_resolver_config_hosts_aliases.xml diff --git a/plugins/module_utils/pfsense.py b/plugins/module_utils/pfsense.py index 339d7f4f..7dd159e0 100644 --- a/plugins/module_utils/pfsense.py +++ b/plugins/module_utils/pfsense.py @@ -320,11 +320,23 @@ def copy_dict_to_element(self, src, top_elt, sub=0, prev_elt=None): changed = True elif self.copy_dict_to_element(item, all_sub_elts[idx], sub=sub + 1, prev_elt=prev_elt): changed = True - elif this_elt.text is None and value == '': - pass - elif this_elt.text != value: - this_elt.text = value - changed = True + else: + # If this element previously held a dict/list (has children) + # but the new value is a scalar, remove stale children first. + # Without this, nested elements (e.g. inside ) + # persist even when the parent is set to an empty string, + # causing data from one list entry to bleed into another. + # Note: ET.Element.clear() is not used here because it also + # resets .tail, which would corrupt pretty-print whitespace. + if len(this_elt) > 0: + for child in list(this_elt): + this_elt.remove(child) + changed = True + if this_elt.text is None and value == '': + pass + elif this_elt.text != value: + this_elt.text = value + changed = True self.debug.write('changed=%s this_elt.text=%s value=%s\n' % (changed, this_elt.text, value)) prev_elt = this_elt diff --git a/tests/unit/plugins/modules/fixtures/pfsense_dns_resolver_config_hosts_aliases.xml b/tests/unit/plugins/modules/fixtures/pfsense_dns_resolver_config_hosts_aliases.xml new file mode 100644 index 00000000..d8db7ef6 --- /dev/null +++ b/tests/unit/plugins/modules/fixtures/pfsense_dns_resolver_config_hosts_aliases.xml @@ -0,0 +1,251 @@ + + 23.3 + + normal + pfSense + acme.com + on + + all + All Users + system + 1998 + + + admins + System Administrators + system + 1999 + 0 + page-all + + + + system + + test + + + + + system + + groupe1 + + + + + system + + groupe2 + + + + admin + System Administrator + system + admins + $2y$10$AMCpA.Z.RNaferLp1yzFq.BvaGgfqaJKtQug7OErbocyNagsEK6xW + 0 + user-shell-access + + 2 + + + pfSense.css + + 2000 + 2000 + 0.pfsense.pool.ntp.org + + http + + 5c00e5f9029df + 2 + + yes + + + + 400000 + hadp + hadp + hadp + + monthly + + + + Etc/UTC + + + + + em0 + + dhcp + dhcp6 + + + + + + + + + 0 + + + + em1 + 192.168.1.1 + 24 + + + + + wan + 0 + + + + + em2 + opt1 + 10.0.0.1 + 24 + + + em1.100 + VLAN 100 + 172.16.0.1 + 24 + + + + + em1 + 100 + + VLAN 100 on LAN + em1.100 + + + + + + + 192.168.1.100 + 192.168.1.199 + + + 86400 + 172800 + + + + + + + + hmac-md5 + + + + allow + + + + + + + + + + + + + 10.0.0.100 + 10.0.0.199 + + + 86400 + 172800 + + + opt1.example.com + + + + + hmac-md5 + + + + allow + + + + + + + + + enabled + + + + + + all + all + + + + + + transparent + 4 + 10 + 10 + auto + 512 + 200 + 86400 + 0 + 900 + 10000 + disabled + 1 + + server + example.com + 10.0.0.1 + + + + alias1 + example.com + Alias 1 + + + alias2 + example.com + Alias 2 + + + + + other + example.com + 10.0.0.2 + + + + + + + aggregated change + + + diff --git a/tests/unit/plugins/modules/test_pfsense_dns_resolver.py b/tests/unit/plugins/modules/test_pfsense_dns_resolver.py index a9a7e53e..18130354 100644 --- a/tests/unit/plugins/modules/test_pfsense_dns_resolver.py +++ b/tests/unit/plugins/modules/test_pfsense_dns_resolver.py @@ -9,6 +9,7 @@ from ansible_collections.pfsensible.core.plugins.modules.pfsense_dns_resolver import PFSenseDNSResolverModule from .pfsense_module import TestPFSenseModule from ansible_collections.community.internal_test_tools.tests.unit.compat.mock import patch +from ansible_collections.community.internal_test_tools.tests.unit.plugins.modules.utils import set_module_args class TestPFSenseDNSResolverModule(TestPFSenseModule): @@ -102,6 +103,55 @@ def test_dns_resolver_noop(self): obj = dict() self.do_module_test(obj, changed=False) + def test_dns_resolver_hosts_reorder_aliases_stay(self): + """ test that aliases stay with their parent host when hosts are reordered + + Regression test: copy_dict_to_element() pairs list items by position. + When hosts are provided in a different order than config.xml, a host + with no aliases could inherit stale children from a host that + previously occupied the same position. + """ + # Use a fixture that has two hosts: "server" (with aliases) at position 0, + # "other" (no aliases) at position 1. + self.config_file = 'pfsense_dns_resolver_config_hosts_aliases.xml' + self.xml_result = None + + # Provide hosts reversed: other first, server second. + obj = dict( + hosts=[ + dict(host="other", domain="example.com", ip="10.0.0.2", descr="", aliases=[]), + dict(host="server", domain="example.com", ip="10.0.0.1", descr="", + aliases=[dict(host="alias1", domain="example.com", description="Alias 1"), + dict(host="alias2", domain="example.com", description="Alias 2")]), + ] + ) + # Run the module directly and inspect XML — bypass check_target_elt + # which cannot handle complex nested hosts/aliases structures. + with set_module_args(self.args_from_var(obj, state='present')): + result = self.execute_module(changed=True) + self.assertTrue(self.load_xml_result()) + + # Verify aliases stayed with the correct host after reorder. + # "other" must have NO alias children; "server" must keep its 2. + unbound = self.xml_result.find('unbound') + hosts_elts = unbound.findall('hosts') + self.assertEqual(len(hosts_elts), 2) + + for host_elt in hosts_elts: + hostname = host_elt.find('host').text + aliases_elt = host_elt.find('aliases') + if hostname == 'other': + items = aliases_elt.findall('item') if aliases_elt is not None else [] + self.assertEqual(len(items), 0, + 'Host "other" should have no alias items but got %d — ' + 'aliases bled from "server" due to positional matching' % len(items)) + elif hostname == 'server': + items = aliases_elt.findall('item') if aliases_elt is not None else [] + self.assertEqual(len(items), 2, + 'Host "server" should have 2 alias items but got %d' % len(items)) + else: + self.fail('Unexpected host element with hostname %r in result XML' % hostname) + def test_dns_resolver_domainoverrides_forward_tls_upstream(self): """ test initialization of the DNS Resolver """ obj = dict(