Merge lp:~lamont/maas/bug-1677741 into lp:~maas-committers/maas/trunk
- bug-1677741
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | LaMont Jones |
Approved revision: | no longer in the source branch. |
Merged at revision: | 5908 |
Proposed branch: | lp:~lamont/maas/bug-1677741 |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
432 lines (+215/-82) 2 files modified
src/maasserver/models/staticipaddress.py (+75/-43) src/maasserver/models/tests/test_staticipaddress.py (+140/-39) |
To merge this branch: | bzr merge lp:~lamont/maas/bug-1677741 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Mike Pontillo (community) | Approve | ||
Review via email: mp+321572@code.launchpad.net |
Commit message
In some cases, DNS Resource address records were ignored.
Description of the change
Refactor the special mappings in the static ipaddress model so that they
actually return the right things. Add greater coverage to the unit tests.
staticipaddress.py is best reviewed by simply reading the new version of the
file, since it's building an SQL query bit by bit and the diff doesn't help.
LaMont Jones (lamont) wrote : | # |
> This code is very difficult to review since its complexity has gone through
> the roof.
>
> I think I said this the last this was changed, but I really think we should
> break this down into multiple queries (via views, perhaps) so that individual
> pieces can be tested easier.
>
> For now, I attempted to review by comparing the added/changed tests to the
> added/changed code. I think I would like to see more specific tests to capture
> each edge case, such as:
>
> test__default_
> test__default_
These are both covered via:
test_
test_
test_
I have updated them to be more clear about what they are expecting to see.
> test__domains_
This is covered (among others) via:
test_
> test__attached_
This is covered via:
test_
> That way it's clearer to future maintainers how each edge case and branch
> called out in the SQL maps to a requirement for a specific behavior.
Mike Pontillo (mpontillo) wrote : | # |
Thanks for the cleanup; I tested this and it's working well!
Preview Diff
1 | === modified file 'src/maasserver/models/staticipaddress.py' |
2 | --- src/maasserver/models/staticipaddress.py 2017-03-25 22:29:09 +0000 |
3 | +++ src/maasserver/models/staticipaddress.py 2017-03-31 21:12:36 +0000 |
4 | @@ -252,14 +252,21 @@ |
5 | """Get the special mappings, possibly limited to a single Domain. |
6 | |
7 | This function is responsible for creating these mappings: |
8 | - - any USER_RESERVED IP, |
9 | + - any USER_RESERVED IP that has no name (dnsrr or node), |
10 | - any IP not associated with a Node, |
11 | - any IP associated with a DNSResource. |
12 | + |
13 | + Addresses that are associated with both a Node and a DNSResource behave |
14 | + thusly: |
15 | + - Both forward mappings include the address |
16 | + - The reverse mapping points only to the Node (and is the |
17 | + responsibility of the caller.) |
18 | + |
19 | The caller is responsible for addresses otherwise derived from nodes. |
20 | |
21 | Because of how the get hostname_ip_mapping code works, we actually need |
22 | - to fetch ALL of the entries for subnets, but forward mappings need to |
23 | - be domain-specific. |
24 | + to fetch ALL of the entries for subnets, but forward mappings are |
25 | + domain-specific. |
26 | |
27 | :param domain: limit return to just the given Domain. If anything |
28 | other than a Domain is passed in (e.g., a Subnet or None), we |
29 | @@ -270,32 +277,6 @@ |
30 | :return: a (default) dict of hostname: HostnameIPMapping entries. |
31 | """ |
32 | default_ttl = "%d" % Config.objects.get_config('default_dns_ttl') |
33 | - if isinstance(domain, Domain): |
34 | - # Domains are special in that we only want to have entries for the |
35 | - # domain that we were asked about. And they can possibly come from |
36 | - # either the child or the parent for glue. |
37 | - where_clause = """ |
38 | - AND ( |
39 | - dnsrr.dom2_id = %s OR |
40 | - node.dom2_id = %s OR |
41 | - dnsrr.domain_id = %s OR |
42 | - node.domain_id = %s |
43 | - """ |
44 | - query_parms = [domain.id, domain.id, domain.id, domain.id] |
45 | - # And the default domain is extra special, since it needs to have |
46 | - # A/AAAA RRs for any USER_RESERVED addresses that have no name |
47 | - # otherwise attached to them. |
48 | - if domain.is_default(): |
49 | - where_clause += """ OR ( |
50 | - dnsrr.fqdn IS NULL AND |
51 | - node.fqdn IS NULL) |
52 | - """ |
53 | - where_clause += ")" |
54 | - else: |
55 | - # There is nothing special about the query for subnets. |
56 | - domain = None |
57 | - where_clause = "" |
58 | - query_parms = [] |
59 | # raw_ttl says that we don't coalesce, but we need to pick one, so we |
60 | # go with DNSResource if it is involved. |
61 | if raw_ttl: |
62 | @@ -371,27 +352,78 @@ |
63 | ) AS node ON |
64 | node_sip_id = staticip.id |
65 | WHERE |
66 | - staticip.ip IS NOT NULL AND |
67 | - host(staticip.ip) != '' AND |
68 | - ( |
69 | - staticip.alloc_type = %s OR |
70 | - node.fqdn IS NULL OR |
71 | - dnsrr IS NOT NULL |
72 | - )""" + where_clause + """ |
73 | - """ |
74 | + (staticip.ip IS NOT NULL AND host(staticip.ip) != '') AND |
75 | + """ |
76 | + |
77 | + query_parms = [] |
78 | + if isinstance(domain, Domain): |
79 | + if domain.is_default(): |
80 | + # The default domain is extra special, since it needs to have |
81 | + # A/AAAA RRs for any USER_RESERVED addresses that have no name |
82 | + # otherwise attached to them. |
83 | + # We need to get all of the entries that are: |
84 | + # - in this domain and have a dnsrr associated, OR |
85 | + # - are USER_RESERVED and have NO fqdn associated at all. |
86 | + sql_query += """ (( |
87 | + staticip.alloc_type = %s AND |
88 | + dnsrr.fqdn IS NULL AND |
89 | + node.fqdn IS NULL |
90 | + ) OR ( |
91 | + dnsrr.fqdn IS NOT NULL AND |
92 | + ( |
93 | + dnsrr.dom2_id = %s OR |
94 | + node.dom2_id = %s OR |
95 | + dnsrr.domain_id = %s OR |
96 | + node.domain_id = %s)))""" |
97 | + query_parms += [IPADDRESS_TYPE.USER_RESERVED] |
98 | + else: |
99 | + # For domains, we only need answers for the domain we were |
100 | + # given. These can can possibly come from either the child or |
101 | + # the parent for glue. Anything with a node associated will be |
102 | + # found inside of get_hostname_ip_mapping() - we need any |
103 | + # entries that are: |
104 | + # - in this domain and have a dnsrr associated. |
105 | + sql_query += """ ( |
106 | + dnsrr.fqdn IS NOT NULL AND |
107 | + ( |
108 | + dnsrr.dom2_id = %s OR |
109 | + node.dom2_id = %s OR |
110 | + dnsrr.domain_id = %s OR |
111 | + node.domain_id = %s))""" |
112 | + query_parms += [domain.id, domain.id, domain.id, domain.id] |
113 | + else: |
114 | + # In the subnet map, addresses attached to nodes only map back to |
115 | + # the node, since some things don't like multiple PTR RRs in |
116 | + # answers from the DNS. |
117 | + # Since that is handled in get_hostname_ip_mapping, we exclude |
118 | + # anything where the node also has a link to the address. |
119 | + domain = None |
120 | + sql_query += """ (( |
121 | + node.fqdn IS NULL AND dnsrr.fqdn IS NOT NULL |
122 | + ) OR ( |
123 | + staticip.alloc_type = %s AND |
124 | + dnsrr.fqdn IS NULL AND |
125 | + node.fqdn IS NULL))""" |
126 | + query_parms += [IPADDRESS_TYPE.USER_RESERVED] |
127 | + |
128 | default_domain = Domain.objects.get_default_domain() |
129 | mapping = defaultdict(HostnameIPMapping) |
130 | cursor = connection.cursor() |
131 | - query_parms = [IPADDRESS_TYPE.USER_RESERVED] + query_parms |
132 | cursor.execute(sql_query, query_parms) |
133 | - for (fqdn, system_id, node_type, ttl, |
134 | - ip) in cursor.fetchall(): |
135 | + for (fqdn, system_id, node_type, ttl, ip) in cursor.fetchall(): |
136 | if fqdn is None or fqdn == '': |
137 | fqdn = "%s.%s" % ( |
138 | get_ip_based_hostname(ip), default_domain.name) |
139 | - mapping[fqdn].node_type = node_type |
140 | - mapping[fqdn].system_id = system_id |
141 | - mapping[fqdn].ttl = ttl |
142 | + # It is possible that there are both Node and DNSResource entries |
143 | + # for this fqdn. If we have any system_id, preserve it. Ditto for |
144 | + # TTL. It is left as an exercise for the admin to make sure that |
145 | + # the any non-default TTL applied to the Node and DNSResource are |
146 | + # equal. |
147 | + if system_id is not None: |
148 | + mapping[fqdn].node_type = node_type |
149 | + mapping[fqdn].system_id = system_id |
150 | + if ttl is not None: |
151 | + mapping[fqdn].ttl = ttl |
152 | mapping[fqdn].ips.add(ip) |
153 | return mapping |
154 | |
155 | |
156 | === modified file 'src/maasserver/models/tests/test_staticipaddress.py' |
157 | --- src/maasserver/models/tests/test_staticipaddress.py 2017-03-25 22:29:09 +0000 |
158 | +++ src/maasserver/models/tests/test_staticipaddress.py 2017-03-31 21:12:36 +0000 |
159 | @@ -10,7 +10,6 @@ |
160 | shuffle, |
161 | ) |
162 | import threading |
163 | -from unittest import skip |
164 | from unittest.mock import sentinel |
165 | |
166 | from django.core.exceptions import ValidationError |
167 | @@ -822,7 +821,6 @@ |
168 | node.system_id, 30, {vlanip.ip}, node.node_type)} |
169 | self.assertEqual(expected_mapping, mapping) |
170 | |
171 | - @skip("XXX: GavinPanella 2016-04-27 bug=1556188: Fails spuriously.") |
172 | def test_get_hostname_ip_mapping_returns_domain_head_ips(self): |
173 | parent = factory.make_Domain() |
174 | name = factory.make_name() |
175 | @@ -923,6 +921,67 @@ |
176 | } |
177 | self.assertEqual(expected_mapping, mapping) |
178 | |
179 | + def test_get_hostname_ip_mapping_handles_dnsrr_node_collision(self): |
180 | + hostname = factory.make_name('hostname') |
181 | + domainname = factory.make_name('domain') |
182 | + # Randomly make our domain either the default, or non-default. |
183 | + # This does not change what we expect the answer to be. |
184 | + if factory.pick_bool(): |
185 | + Domain.objects.get_default_domain() |
186 | + domain = factory.make_Domain(name=domainname) |
187 | + full_hostname = "%s.%s" % (hostname, domainname) |
188 | + dnsrrname = factory.make_name('dnsrrname') |
189 | + full_dnsrrname = "%s.%s" % (dnsrrname, domainname) |
190 | + |
191 | + subnet = factory.make_Subnet() |
192 | + node = factory.make_Node_with_Interface_on_Subnet( |
193 | + interface=True, hostname=full_hostname, interface_count=3, |
194 | + subnet=subnet) |
195 | + boot_interface = node.get_boot_interface() |
196 | + staticip = factory.make_StaticIPAddress( |
197 | + alloc_type=IPADDRESS_TYPE.STICKY, |
198 | + ip=factory.pick_ip_in_Subnet(subnet), |
199 | + subnet=subnet, interface=boot_interface) |
200 | + iface2 = node.interface_set.exclude(id=boot_interface.id).first() |
201 | + sip2 = factory.make_StaticIPAddress( |
202 | + alloc_type=IPADDRESS_TYPE.STICKY, |
203 | + ip=factory.pick_ip_in_Subnet(subnet), |
204 | + subnet=subnet, interface=iface2) |
205 | + sip3 = factory.make_StaticIPAddress( |
206 | + alloc_type=IPADDRESS_TYPE.STICKY, |
207 | + ip=factory.pick_ip_in_Subnet(subnet), |
208 | + subnet=subnet) |
209 | + # Create a DNSResource with one address on the node, and one not. |
210 | + # The forward mapping for the dnsrr points to both IPs. |
211 | + # The subnet mapping for the IP on the node only points to the node. |
212 | + # The other IP points to the DNSRR name. |
213 | + factory.make_DNSResource( |
214 | + name=dnsrrname, domain=domain, ip_addresses=[sip2, sip3]) |
215 | + mapping = StaticIPAddress.objects.get_hostname_ip_mapping(domain) |
216 | + expected = { |
217 | + full_hostname: |
218 | + HostnameIPMapping( |
219 | + node.system_id, 30, {staticip.ip}, node.node_type), |
220 | + "%s.%s" % (iface2.name, full_hostname): |
221 | + HostnameIPMapping( |
222 | + node.system_id, 30, {sip2.ip}, node.node_type), |
223 | + full_dnsrrname: |
224 | + HostnameIPMapping( |
225 | + node.system_id, 30, {sip2.ip, sip3.ip}, node.node_type), |
226 | + } |
227 | + self.assertEqual(expected, mapping) |
228 | + mapping = StaticIPAddress.objects.get_hostname_ip_mapping(subnet) |
229 | + expected = { |
230 | + full_hostname: |
231 | + HostnameIPMapping( |
232 | + node.system_id, 30, {staticip.ip}, node.node_type), |
233 | + "%s.%s" % (iface2.name, full_hostname): |
234 | + HostnameIPMapping( |
235 | + node.system_id, 30, {sip2.ip}, node.node_type), |
236 | + full_dnsrrname: |
237 | + HostnameIPMapping(None, 30, {sip3.ip}, None), |
238 | + } |
239 | + |
240 | |
241 | class TestStaticIPAddress(MAASServerTestCase): |
242 | |
243 | @@ -1012,23 +1071,34 @@ |
244 | alloc_type=IPADDRESS_TYPE.USER_RESERVED) |
245 | for _ in range(num_ips) |
246 | ] |
247 | + expected = { |
248 | + ip.dnsresource_set.first().fqdn: |
249 | + HostnameIPMapping(None, 30, {ip.ip}, None) |
250 | + for ip in ips |
251 | + } |
252 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0) |
253 | - self.expectThat(mappings, HasLength(len(ips))) |
254 | + self.assertEqual(expected, mappings) |
255 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain1) |
256 | self.expectThat(mappings, HasLength(0)) |
257 | |
258 | def test_user_reserved_addresses_included_in_correct_domains(self): |
259 | - # Generate addresses in two domains, and make sure that they wind up in |
260 | - # the right domains, and not in other domains. |
261 | + # Generate some addresses with no names attached, and some more in each |
262 | + # of two (non-default) domains. Make sure that they wind up in the |
263 | + # right domains, and not in other domains. |
264 | domain0 = Domain.objects.get_default_domain() |
265 | domain1 = factory.make_Domain() |
266 | domain2 = factory.make_Domain() |
267 | + ips0 = [ |
268 | + factory.make_StaticIPAddress( |
269 | + alloc_type=IPADDRESS_TYPE.USER_RESERVED) |
270 | + for _ in range(randint(3, 5)) |
271 | + ] |
272 | ips1 = [ |
273 | factory.make_StaticIPAddress( |
274 | hostname="%s.%s" % ( |
275 | factory.make_name('host'), domain1.name), |
276 | alloc_type=IPADDRESS_TYPE.USER_RESERVED) |
277 | - for _ in range(randint(3, 5)) |
278 | + for _ in range(randint(6, 9)) |
279 | ] |
280 | ips2 = [ |
281 | factory.make_StaticIPAddress( |
282 | @@ -1037,12 +1107,27 @@ |
283 | alloc_type=IPADDRESS_TYPE.USER_RESERVED) |
284 | for _ in range(randint(1, 2)) |
285 | ] |
286 | + expected = { |
287 | + "%s.%s" % (get_ip_based_hostname(ip.ip), domain0.name): |
288 | + HostnameIPMapping(None, 30, {ip.ip}, None) |
289 | + for ip in ips0 |
290 | + } |
291 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0) |
292 | - self.expectThat(mappings, HasLength(0)) |
293 | + self.assertEqual(expected, mappings) |
294 | + expected = { |
295 | + ip.dnsresource_set.first().fqdn: |
296 | + HostnameIPMapping(None, 30, {ip.ip}, None) |
297 | + for ip in ips1 |
298 | + } |
299 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain1) |
300 | - self.expectThat(mappings, HasLength(len(ips1))) |
301 | + self.assertEqual(expected, mappings) |
302 | + expected = { |
303 | + ip.dnsresource_set.first().fqdn: |
304 | + HostnameIPMapping(None, 30, {ip.ip}, None) |
305 | + for ip in ips2 |
306 | + } |
307 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain2) |
308 | - self.expectThat(mappings, HasLength(len(ips2))) |
309 | + self.assertEqual(expected, mappings) |
310 | |
311 | def test_dns_resources_only_have_correct_domain(self): |
312 | # Create two DNS resources pointing to the same IP, and make sure that |
313 | @@ -1080,35 +1165,54 @@ |
314 | def test_user_reserved_IP_on_node_or_default_domain_not_both(self): |
315 | # ip1 gets assigned to an interface on a node in domain1 |
316 | # ip2 is not assigned to anything. |
317 | + # ip3 gets assigned to a DNS resource in domain2 |
318 | # ip1 should show up for the node, in domain1, and the subnet. |
319 | # ip2 should show up in the default domain, and the subnet. |
320 | + # ip3 should show up in domain2, and the subnet. |
321 | domain0 = Domain.objects.get_default_domain() |
322 | domain1 = factory.make_Domain() |
323 | + domain2 = factory.make_Domain() |
324 | + rrname = factory.make_name('dnsrr') |
325 | subnet = factory.make_Subnet() |
326 | ip1 = factory.make_StaticIPAddress(subnet=subnet) |
327 | - ip2 = factory.make_StaticIPAddress(subnet=subnet) |
328 | + ip2 = factory.make_StaticIPAddress( |
329 | + subnet=subnet, alloc_type=IPADDRESS_TYPE.USER_RESERVED) |
330 | + ip3 = factory.make_StaticIPAddress( |
331 | + subnet=subnet, alloc_type=IPADDRESS_TYPE.USER_RESERVED) |
332 | + dnsrr = factory.make_DNSResource( |
333 | + name=rrname, domain=domain2, ip_addresses=[ip3]) |
334 | name2 = "%s.%s" % (get_ip_based_hostname(ip2.ip), domain0.name) |
335 | node = factory.make_Node( |
336 | interface=True, |
337 | domain=domain1, |
338 | vlan=subnet.vlan) |
339 | node.interface_set.first().ip_addresses.add(ip1) |
340 | + expected0 = { |
341 | + name2: HostnameIPMapping(None, 30, {ip2.ip}, None), |
342 | + } |
343 | + expected1 = { |
344 | + node.fqdn: |
345 | + HostnameIPMapping( |
346 | + node.system_id, 30, {ip1.ip}, node.node_type), |
347 | + } |
348 | + expected2 = { |
349 | + dnsrr.fqdn: HostnameIPMapping(None, 30, {ip3.ip}, None), |
350 | + } |
351 | + expected_subnet = { |
352 | + name2: HostnameIPMapping(None, 30, {ip2.ip}, None), |
353 | + node.fqdn: |
354 | + HostnameIPMapping( |
355 | + node.system_id, 30, {ip1.ip}, node.node_type), |
356 | + dnsrr.fqdn: HostnameIPMapping(None, 30, {ip3.ip}, None), |
357 | + } |
358 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0) |
359 | - self.expectThat(mappings, HasLength(1)) |
360 | - self.assertEqual( |
361 | - mappings, { |
362 | - name2: HostnameIPMapping(None, 30, {ip2.ip}, None)}) |
363 | + self.assertEqual(expected0, mappings) |
364 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain1) |
365 | - self.expectThat(mappings, HasLength(1)) |
366 | - self.assertEqual( |
367 | - mappings.get(node.fqdn), |
368 | - HostnameIPMapping(node.system_id, 30, {ip1.ip}, node.node_type)) |
369 | + self.assertEqual(expected1, mappings) |
370 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain2) |
371 | + self.assertEqual(expected2, mappings) |
372 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet) |
373 | - self.assertEqual( |
374 | - mappings, { |
375 | - node.fqdn: HostnameIPMapping( |
376 | - node.system_id, 30, {ip1.ip}, node.node_type), |
377 | - name2: HostnameIPMapping(None, 30, {ip2.ip}, None)}) |
378 | + self.assertEqual(expected_subnet, mappings) |
379 | |
380 | def test_node_glue_correctly_generated(self): |
381 | # Given node "foo.bar.com" and a "foo.bar.com" domain (in addition to |
382 | @@ -1131,18 +1235,17 @@ |
383 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(parent) |
384 | self.expectThat(mappings, HasLength(1)) |
385 | self.assertEqual( |
386 | - mappings.get(node.fqdn), |
387 | - HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type)) |
388 | + HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type), |
389 | + mappings.get(node.fqdn)) |
390 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain) |
391 | self.expectThat(mappings, HasLength(1)) |
392 | self.assertEqual( |
393 | - mappings.get(node.fqdn), |
394 | - HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type)) |
395 | + HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type), |
396 | + mappings.get(node.fqdn)) |
397 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet) |
398 | - self.assertEqual( |
399 | - mappings, { |
400 | - node.fqdn: HostnameIPMapping( |
401 | - node.system_id, 30, {ip.ip}, node.node_type)}) |
402 | + self.assertEqual({ |
403 | + node.fqdn: HostnameIPMapping( |
404 | + node.system_id, 30, {ip.ip}, node.node_type)}, mappings) |
405 | |
406 | def test_dnsrr_glue_correctly_generated(self): |
407 | # Given DNSResource "foo.bar.com" and a "foo.bar.com" domain (in |
408 | @@ -1163,18 +1266,16 @@ |
409 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(parent) |
410 | self.expectThat(mappings, HasLength(1)) |
411 | self.assertEqual( |
412 | - mappings.get(dnsrr.fqdn), |
413 | - HostnameIPMapping(None, 30, {ip.ip}, None)) |
414 | + HostnameIPMapping(None, 30, {ip.ip}, None), |
415 | + mappings.get(dnsrr.fqdn)) |
416 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain) |
417 | self.expectThat(mappings, HasLength(1)) |
418 | self.assertEqual( |
419 | - mappings.get(dnsrr.fqdn), |
420 | - HostnameIPMapping(None, 30, {ip.ip}, None)) |
421 | + HostnameIPMapping(None, 30, {ip.ip}, None), |
422 | + mappings.get(dnsrr.fqdn)) |
423 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet) |
424 | - self.assertEqual( |
425 | - mappings, { |
426 | - dnsrr.fqdn: HostnameIPMapping( |
427 | - None, 30, {ip.ip}, None)}) |
428 | + self.assertEqual({ |
429 | + dnsrr.fqdn: HostnameIPMapping(None, 30, {ip.ip}, None)}, mappings) |
430 | |
431 | |
432 | class TestRenderJSON(MAASServerTestCase): |
This code is very difficult to review since its complexity has gone through the roof.
I think I said this the last this was changed, but I really think we should break this down into multiple queries (via views, perhaps) so that individual pieces can be tested easier.
For now, I attempted to review by comparing the added/changed tests to the added/changed code. I think I would like to see more specific tests to capture each edge case, such as:
test_ _default_ domain_ is_extra_ special_ ipv4 _default_ domain_ is_extra_ special_ ipv6 _domains_ are_special _attached_ addresses_ only_map_ back_to_ node
test_
test_
test_
That way it's clearer to future maintainers how each edge case and branch called out in the SQL maps to a requirement for a specific behavior.