Skip to content

Fix copy_dict_to_element() stale children causing alias misassignment #237

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
djlongy wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix copy_dict_to_element() stale children causing alias misassignment #237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 17 additions & 5 deletions plugins/module_utils/pfsense.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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. <item> inside <aliases>)
# 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

Expand Down
251 changes: 251 additions & 0 deletions tests/unit/plugins/modules/fixtures/pfsense_dns_resolver_config_hosts_aliases.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,251 @@
<pfsense>
<version>23.3</version>
<system>
<optimization>normal</optimization>
<hostname>pfSense</hostname>
<domain>acme.com</domain>
<dnsallowoverride>on</dnsallowoverride>
<group>
<name>all</name>
<description>All Users</description>
<scope>system</scope>
<gid>1998</gid>
</group>
<group>
<name>admins</name>
<description>System Administrators</description>
<scope>system</scope>
<gid>1999</gid>
<member>0</member>
<priv>page-all</priv>
</group>
<group>
<priv></priv>
<scope>system</scope>
<gid></gid>
<name>test</name>
<description></description>
</group>
<group>
<priv></priv>
<scope>system</scope>
<gid></gid>
<name>groupe1</name>
<description></description>
</group>
<group>
<priv></priv>
<scope>system</scope>
<gid></gid>
<name>groupe2</name>
<description></description>
</group>
<user>
<name>admin</name>
<descr>System Administrator</descr>
<scope>system</scope>
<groupname>admins</groupname>
<bcrypt-hash>$2y$10$AMCpA.Z.RNaferLp1yzFq.BvaGgfqaJKtQug7OErbocyNagsEK6xW</bcrypt-hash>
<uid>0</uid>
<priv>user-shell-access</priv>
<expires></expires>
<dashboardcolumns>2</dashboardcolumns>
<authorizedkeys></authorizedkeys>
<ipsecpsk></ipsecpsk>
<webguicss>pfSense.css</webguicss>
</user>
<nextuid>2000</nextuid>
<nextgid>2000</nextgid>
<timeservers>0.pfsense.pool.ntp.org</timeservers>
<webgui>
<protocol>http</protocol>
<loginautocomplete></loginautocomplete>
<ssl-certref>5c00e5f9029df</ssl-certref>
<dashboardcolumns>2</dashboardcolumns>
</webgui>
<disablenatreflection>yes</disablenatreflection>
<disablesegmentationoffloading></disablesegmentationoffloading>
<disablelargereceiveoffloading></disablelargereceiveoffloading>
<ipv6allow></ipv6allow>
<maximumtableentries>400000</maximumtableentries>
<powerd_ac_mode>hadp</powerd_ac_mode>
<powerd_battery_mode>hadp</powerd_battery_mode>
<powerd_normal_mode>hadp</powerd_normal_mode>
<bogons>
<interval>monthly</interval>
</bogons>
<already_run_config_upgrade></already_run_config_upgrade>
<ssh></ssh>
<timezone>Etc/UTC</timezone>
</system>
<interfaces>
<wan>
<enable></enable>
<if>em0</if>
<mtu></mtu>
<ipaddr>dhcp</ipaddr>
<ipaddrv6>dhcp6</ipaddrv6>
<subnet></subnet>
<gateway></gateway>
<blockpriv></blockpriv>
<blockbogons></blockbogons>
<dhcphostname></dhcphostname>
<media></media>
<mediaopt></mediaopt>
<dhcp6-duid></dhcp6-duid>
<dhcp6-ia-pd-len>0</dhcp6-ia-pd-len>
</wan>
<lan>
<enable></enable>
<if>em1</if>
<ipaddr>192.168.1.1</ipaddr>
<subnet>24</subnet>
<ipaddrv6></ipaddrv6>
<subnetv6></subnetv6>
<media></media>
<mediaopt></mediaopt>
<track6-interface>wan</track6-interface>
<track6-prefix-id>0</track6-prefix-id>
<gateway></gateway>
<gatewayv6></gatewayv6>
</lan>
<opt1>
<if>em2</if>
<descr>opt1</descr>
<ipaddr>10.0.0.1</ipaddr>
<subnet>24</subnet>
</opt1>
<opt2>
<if>em1.100</if>
<descr>VLAN 100</descr>
<ipaddr>172.16.0.1</ipaddr>
<subnet>24</subnet>
</opt2>
</interfaces>
<vlans>
<vlan>
<if>em1</if>
<tag>100</tag>
<pcp></pcp>
<descr>VLAN 100 on LAN</descr>
<vlanif>em1.100</vlanif>
</vlan>
</vlans>
<dhcpd>
<lan>
<enable></enable>
<range>
<from>192.168.1.100</from>
<to>192.168.1.199</to>
</range>
<failover_peerip></failover_peerip>
<defaultleasetime>86400</defaultleasetime>
<maxleasetime>172800</maxleasetime>
<netmask></netmask>
<gateway></gateway>
<domain></domain>
<domainsearchlist></domainsearchlist>
<ddnsdomain></ddnsdomain>
<ddnsdomainprimary></ddnsdomainprimary>
<ddnsdomainkeyname></ddnsdomainkeyname>
<ddnsdomainkeyalgorithm>hmac-md5</ddnsdomainkeyalgorithm>
<ddnsdomainkey></ddnsdomainkey>
<mac_allow></mac_allow>
<mac_deny></mac_deny>
<ddnsclientupdates>allow</ddnsclientupdates>
<tftp></tftp>
<ldap></ldap>
<nextserver></nextserver>
<filename></filename>
<filename32></filename32>
<filename64></filename64>
<rootpath></rootpath>
<numberoptions></numberoptions>
</lan>
<opt1>
<enable></enable>
<range>
<from>10.0.0.100</from>
<to>10.0.0.199</to>
</range>
<failover_peerip></failover_peerip>
<defaultleasetime>86400</defaultleasetime>
<maxleasetime>172800</maxleasetime>
<netmask></netmask>
<gateway></gateway>
<domain>opt1.example.com</domain>
<domainsearchlist></domainsearchlist>
<ddnsdomain></ddnsdomain>
<ddnsdomainprimary></ddnsdomainprimary>
<ddnsdomainkeyname></ddnsdomainkeyname>
<ddnsdomainkeyalgorithm>hmac-md5</ddnsdomainkeyalgorithm>
<ddnsdomainkey></ddnsdomainkey>
<mac_allow></mac_allow>
<mac_deny></mac_deny>
<ddnsclientupdates>allow</ddnsclientupdates>
<tftp></tftp>
<ldap></ldap>
<nextserver></nextserver>
<filename></filename>
<filename32></filename32>
<filename64></filename64>
<rootpath></rootpath>
<numberoptions></numberoptions>
<denyunknown>enabled</denyunknown>
</opt1>
</dhcpd>
<unbound>
<enable></enable>
<dnssec></dnssec>
<active_interface>all</active_interface>
<outgoing_interface>all</outgoing_interface>
<custom_options></custom_options>
<hideidentity></hideidentity>
<hideversion></hideversion>
<dnssecstripped></dnssecstripped>
<qname-minimisation></qname-minimisation>
<system_domain_local_zone_type>transparent</system_domain_local_zone_type>
<msgcachesize>4</msgcachesize>
<outgoing_num_tcp>10</outgoing_num_tcp>
<incoming_num_tcp>10</incoming_num_tcp>
<edns_buffer_size>auto</edns_buffer_size>
<num_queries_per_thread>512</num_queries_per_thread>
<jostle_timeout>200</jostle_timeout>
<cache_max_ttl>86400</cache_max_ttl>
<cache_min_ttl>0</cache_min_ttl>
<infra_host_ttl>900</infra_host_ttl>
<infra_cache_numhosts>10000</infra_cache_numhosts>
<unwanted_reply_threshold>disabled</unwanted_reply_threshold>
<log_verbosity>1</log_verbosity>
<hosts>
<host>server</host>
<domain>example.com</domain>
<ip>10.0.0.1</ip>
<descr></descr>
<aliases>
<item>
<host>alias1</host>
<domain>example.com</domain>
<description>Alias 1</description>
</item>
<item>
<host>alias2</host>
<domain>example.com</domain>
<description>Alias 2</description>
</item>
</aliases>
</hosts>
<hosts>
<host>other</host>
<domain>example.com</domain>
<ip>10.0.0.2</ip>
<descr></descr>
<aliases></aliases>
</hosts>
</unbound>
<revision>
<time>1545602758</time>
<description>aggregated change</description>
<username></username>
</revision>
</pfsense>
50 changes: 50 additions & 0 deletions tests/unit/plugins/modules/test_pfsense_dns_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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 <item> 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 <item> 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(
Expand Down