Skip to content

Commit 035e575

Browse files
djlongyopoplawski
djlongy
authored and
opoplawski
committed
Fix copy_dict_to_element() losing alias-to-host association
1 parent ae3145d commit 035e575

3 files changed

Lines changed: 319 additions & 6 deletions

File tree

‎plugins/module_utils/pfsense.py‎

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -320,12 +320,24 @@ def copy_dict_to_element(self, src, top_elt, sub=0, prev_elt=None):
320320
changed = True
321321
elif self.copy_dict_to_element(item, all_sub_elts[idx], sub=sub + 1, prev_elt=prev_elt):
322322
changed = True
323-
elif this_elt.text is None and value == '':
324-
pass
325-
elif this_elt.text != value:
326-
this_elt.text = value
327-
changed = True
328-
self.debug.write('changed=%s this_elt.text=%s != value=%s\n' % (changed, repr(this_elt.text), repr(value)))
323+
else:
324+
# If this element previously held a dict/list (has children)
325+
# but the new value is a scalar, remove stale children first.
326+
# Without this, nested elements (e.g. <item> inside <aliases>)
327+
# persist even when the parent is set to an empty string,
328+
# causing data from one list entry to bleed into another.
329+
# Note: ET.Element.clear() is not used here because it also
330+
# resets .tail, which would corrupt pretty-print whitespace.
331+
if len(this_elt) > 0:
332+
for child in list(this_elt):
333+
this_elt.remove(child)
334+
changed = True
335+
if this_elt.text is None and value == '':
336+
pass
337+
elif this_elt.text != value:
338+
this_elt.text = value
339+
changed = True
340+
self.debug.write('changed=%s this_elt.text=%s value=%s\n' % (changed, this_elt.text, value))
329341
prev_elt = this_elt
330342

331343
# Sub-elements must be completely described, so remove any missing elements

‎tests/unit/plugins/modules/fixtures/pfsense_dns_resolver_config_hosts_aliases.xml‎

Lines changed: 251 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,251 @@
1+
<pfsense>
2+
<version>23.3</version>
3+
<system>
4+
<optimization>normal</optimization>
5+
<hostname>pfSense</hostname>
6+
<domain>acme.com</domain>
7+
<dnsallowoverride>on</dnsallowoverride>
8+
<group>
9+
<name>all</name>
10+
<description>All Users</description>
11+
<scope>system</scope>
12+
<gid>1998</gid>
13+
</group>
14+
<group>
15+
<name>admins</name>
16+
<description>System Administrators</description>
17+
<scope>system</scope>
18+
<gid>1999</gid>
19+
<member>0</member>
20+
<priv>page-all</priv>
21+
</group>
22+
<group>
23+
<priv></priv>
24+
<scope>system</scope>
25+
<gid></gid>
26+
<name>test</name>
27+
<description></description>
28+
</group>
29+
<group>
30+
<priv></priv>
31+
<scope>system</scope>
32+
<gid></gid>
33+
<name>groupe1</name>
34+
<description></description>
35+
</group>
36+
<group>
37+
<priv></priv>
38+
<scope>system</scope>
39+
<gid></gid>
40+
<name>groupe2</name>
41+
<description></description>
42+
</group>
43+
<user>
44+
<name>admin</name>
45+
<descr>System Administrator</descr>
46+
<scope>system</scope>
47+
<groupname>admins</groupname>
48+
<bcrypt-hash>$2y$10$AMCpA.Z.RNaferLp1yzFq.BvaGgfqaJKtQug7OErbocyNagsEK6xW</bcrypt-hash>
49+
<uid>0</uid>
50+
<priv>user-shell-access</priv>
51+
<expires></expires>
52+
<dashboardcolumns>2</dashboardcolumns>
53+
<authorizedkeys></authorizedkeys>
54+
<ipsecpsk></ipsecpsk>
55+
<webguicss>pfSense.css</webguicss>
56+
</user>
57+
<nextuid>2000</nextuid>
58+
<nextgid>2000</nextgid>
59+
<timeservers>0.pfsense.pool.ntp.org</timeservers>
60+
<webgui>
61+
<protocol>http</protocol>
62+
<loginautocomplete></loginautocomplete>
63+
<ssl-certref>5c00e5f9029df</ssl-certref>
64+
<dashboardcolumns>2</dashboardcolumns>
65+
</webgui>
66+
<disablenatreflection>yes</disablenatreflection>
67+
<disablesegmentationoffloading></disablesegmentationoffloading>
68+
<disablelargereceiveoffloading></disablelargereceiveoffloading>
69+
<ipv6allow></ipv6allow>
70+
<maximumtableentries>400000</maximumtableentries>
71+
<powerd_ac_mode>hadp</powerd_ac_mode>
72+
<powerd_battery_mode>hadp</powerd_battery_mode>
73+
<powerd_normal_mode>hadp</powerd_normal_mode>
74+
<bogons>
75+
<interval>monthly</interval>
76+
</bogons>
77+
<already_run_config_upgrade></already_run_config_upgrade>
78+
<ssh></ssh>
79+
<timezone>Etc/UTC</timezone>
80+
</system>
81+
<interfaces>
82+
<wan>
83+
<enable></enable>
84+
<if>em0</if>
85+
<mtu></mtu>
86+
<ipaddr>dhcp</ipaddr>
87+
<ipaddrv6>dhcp6</ipaddrv6>
88+
<subnet></subnet>
89+
<gateway></gateway>
90+
<blockpriv></blockpriv>
91+
<blockbogons></blockbogons>
92+
<dhcphostname></dhcphostname>
93+
<media></media>
94+
<mediaopt></mediaopt>
95+
<dhcp6-duid></dhcp6-duid>
96+
<dhcp6-ia-pd-len>0</dhcp6-ia-pd-len>
97+
</wan>
98+
<lan>
99+
<enable></enable>
100+
<if>em1</if>
101+
<ipaddr>192.168.1.1</ipaddr>
102+
<subnet>24</subnet>
103+
<ipaddrv6></ipaddrv6>
104+
<subnetv6></subnetv6>
105+
<media></media>
106+
<mediaopt></mediaopt>
107+
<track6-interface>wan</track6-interface>
108+
<track6-prefix-id>0</track6-prefix-id>
109+
<gateway></gateway>
110+
<gatewayv6></gatewayv6>
111+
</lan>
112+
<opt1>
113+
<if>em2</if>
114+
<descr>opt1</descr>
115+
<ipaddr>10.0.0.1</ipaddr>
116+
<subnet>24</subnet>
117+
</opt1>
118+
<opt2>
119+
<if>em1.100</if>
120+
<descr>VLAN 100</descr>
121+
<ipaddr>172.16.0.1</ipaddr>
122+
<subnet>24</subnet>
123+
</opt2>
124+
</interfaces>
125+
<vlans>
126+
<vlan>
127+
<if>em1</if>
128+
<tag>100</tag>
129+
<pcp></pcp>
130+
<descr>VLAN 100 on LAN</descr>
131+
<vlanif>em1.100</vlanif>
132+
</vlan>
133+
</vlans>
134+
<dhcpd>
135+
<lan>
136+
<enable></enable>
137+
<range>
138+
<from>192.168.1.100</from>
139+
<to>192.168.1.199</to>
140+
</range>
141+
<failover_peerip></failover_peerip>
142+
<defaultleasetime>86400</defaultleasetime>
143+
<maxleasetime>172800</maxleasetime>
144+
<netmask></netmask>
145+
<gateway></gateway>
146+
<domain></domain>
147+
<domainsearchlist></domainsearchlist>
148+
<ddnsdomain></ddnsdomain>
149+
<ddnsdomainprimary></ddnsdomainprimary>
150+
<ddnsdomainkeyname></ddnsdomainkeyname>
151+
<ddnsdomainkeyalgorithm>hmac-md5</ddnsdomainkeyalgorithm>
152+
<ddnsdomainkey></ddnsdomainkey>
153+
<mac_allow></mac_allow>
154+
<mac_deny></mac_deny>
155+
<ddnsclientupdates>allow</ddnsclientupdates>
156+
<tftp></tftp>
157+
<ldap></ldap>
158+
<nextserver></nextserver>
159+
<filename></filename>
160+
<filename32></filename32>
161+
<filename64></filename64>
162+
<rootpath></rootpath>
163+
<numberoptions></numberoptions>
164+
</lan>
165+
<opt1>
166+
<enable></enable>
167+
<range>
168+
<from>10.0.0.100</from>
169+
<to>10.0.0.199</to>
170+
</range>
171+
<failover_peerip></failover_peerip>
172+
<defaultleasetime>86400</defaultleasetime>
173+
<maxleasetime>172800</maxleasetime>
174+
<netmask></netmask>
175+
<gateway></gateway>
176+
<domain>opt1.example.com</domain>
177+
<domainsearchlist></domainsearchlist>
178+
<ddnsdomain></ddnsdomain>
179+
<ddnsdomainprimary></ddnsdomainprimary>
180+
<ddnsdomainkeyname></ddnsdomainkeyname>
181+
<ddnsdomainkeyalgorithm>hmac-md5</ddnsdomainkeyalgorithm>
182+
<ddnsdomainkey></ddnsdomainkey>
183+
<mac_allow></mac_allow>
184+
<mac_deny></mac_deny>
185+
<ddnsclientupdates>allow</ddnsclientupdates>
186+
<tftp></tftp>
187+
<ldap></ldap>
188+
<nextserver></nextserver>
189+
<filename></filename>
190+
<filename32></filename32>
191+
<filename64></filename64>
192+
<rootpath></rootpath>
193+
<numberoptions></numberoptions>
194+
<denyunknown>enabled</denyunknown>
195+
</opt1>
196+
</dhcpd>
197+
<unbound>
198+
<enable></enable>
199+
<dnssec></dnssec>
200+
<active_interface>all</active_interface>
201+
<outgoing_interface>all</outgoing_interface>
202+
<custom_options></custom_options>
203+
<hideidentity></hideidentity>
204+
<hideversion></hideversion>
205+
<dnssecstripped></dnssecstripped>
206+
<qname-minimisation></qname-minimisation>
207+
<system_domain_local_zone_type>transparent</system_domain_local_zone_type>
208+
<msgcachesize>4</msgcachesize>
209+
<outgoing_num_tcp>10</outgoing_num_tcp>
210+
<incoming_num_tcp>10</incoming_num_tcp>
211+
<edns_buffer_size>auto</edns_buffer_size>
212+
<num_queries_per_thread>512</num_queries_per_thread>
213+
<jostle_timeout>200</jostle_timeout>
214+
<cache_max_ttl>86400</cache_max_ttl>
215+
<cache_min_ttl>0</cache_min_ttl>
216+
<infra_host_ttl>900</infra_host_ttl>
217+
<infra_cache_numhosts>10000</infra_cache_numhosts>
218+
<unwanted_reply_threshold>disabled</unwanted_reply_threshold>
219+
<log_verbosity>1</log_verbosity>
220+
<hosts>
221+
<host>server</host>
222+
<domain>example.com</domain>
223+
<ip>10.0.0.1</ip>
224+
<descr></descr>
225+
<aliases>
226+
<item>
227+
<host>alias1</host>
228+
<domain>example.com</domain>
229+
<description>Alias 1</description>
230+
</item>
231+
<item>
232+
<host>alias2</host>
233+
<domain>example.com</domain>
234+
<description>Alias 2</description>
235+
</item>
236+
</aliases>
237+
</hosts>
238+
<hosts>
239+
<host>other</host>
240+
<domain>example.com</domain>
241+
<ip>10.0.0.2</ip>
242+
<descr></descr>
243+
<aliases></aliases>
244+
</hosts>
245+
</unbound>
246+
<revision>
247+
<time>1545602758</time>
248+
<description>aggregated change</description>
249+
<username></username>
250+
</revision>
251+
</pfsense>

‎tests/unit/plugins/modules/test_pfsense_dns_resolver.py‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
from ansible_collections.pfsensible.core.plugins.modules.pfsense_dns_resolver import PFSenseDNSResolverModule
1010
from .pfsense_module import TestPFSenseModule
1111
from ansible_collections.community.internal_test_tools.tests.unit.compat.mock import patch
12+
from ansible_collections.community.internal_test_tools.tests.unit.plugins.modules.utils import set_module_args
1213

1314

1415
class TestPFSenseDNSResolverModule(TestPFSenseModule):
@@ -102,6 +103,55 @@ def test_dns_resolver_noop(self):
102103
obj = dict()
103104
self.do_module_test(obj, changed=False)
104105

106+
def test_dns_resolver_hosts_reorder_aliases_stay(self):
107+
""" test that aliases stay with their parent host when hosts are reordered
108+
109+
Regression test: copy_dict_to_element() pairs list items by position.
110+
When hosts are provided in a different order than config.xml, a host
111+
with no aliases could inherit stale <item> children from a host that
112+
previously occupied the same position.
113+
"""
114+
# Use a fixture that has two hosts: "server" (with aliases) at position 0,
115+
# "other" (no aliases) at position 1.
116+
self.config_file = 'pfsense_dns_resolver_config_hosts_aliases.xml'
117+
self.xml_result = None
118+
119+
# Provide hosts reversed: other first, server second.
120+
obj = dict(
121+
hosts=[
122+
dict(host="other", domain="example.com", ip="10.0.0.2", descr="", aliases=[]),
123+
dict(host="server", domain="example.com", ip="10.0.0.1", descr="",
124+
aliases=[dict(host="alias1", domain="example.com", description="Alias 1"),
125+
dict(host="alias2", domain="example.com", description="Alias 2")]),
126+
]
127+
)
128+
# Run the module directly and inspect XML — bypass check_target_elt
129+
# which cannot handle complex nested hosts/aliases structures.
130+
with set_module_args(self.args_from_var(obj, state='present')):
131+
result = self.execute_module(changed=True)
132+
self.assertTrue(self.load_xml_result())
133+
134+
# Verify aliases stayed with the correct host after reorder.
135+
# "other" must have NO alias <item> children; "server" must keep its 2.
136+
unbound = self.xml_result.find('unbound')
137+
hosts_elts = unbound.findall('hosts')
138+
self.assertEqual(len(hosts_elts), 2)
139+
140+
for host_elt in hosts_elts:
141+
hostname = host_elt.find('host').text
142+
aliases_elt = host_elt.find('aliases')
143+
if hostname == 'other':
144+
items = aliases_elt.findall('item') if aliases_elt is not None else []
145+
self.assertEqual(len(items), 0,
146+
'Host "other" should have no alias items but got %d — '
147+
'aliases bled from "server" due to positional matching' % len(items))
148+
elif hostname == 'server':
149+
items = aliases_elt.findall('item') if aliases_elt is not None else []
150+
self.assertEqual(len(items), 2,
151+
'Host "server" should have 2 alias items but got %d' % len(items))
152+
else:
153+
self.fail('Unexpected host element with hostname %r in result XML' % hostname)
154+
105155
def test_dns_resolver_domainoverrides_forward_tls_upstream(self):
106156
""" test initialization of the DNS Resolver """
107157
obj = dict(

0 commit comments

Comments
 (0)