Merge lp:~lamont/maas/bug-1642033 into lp:~maas-committers/maas/trunk
- bug-1642033
- Merge into trunk
Status: | Merged |
---|---|
Approved by: | LaMont Jones |
Approved revision: | no longer in the source branch. |
Merged at revision: | 5560 |
Proposed branch: | lp:~lamont/maas/bug-1642033 |
Merge into: | lp:~maas-committers/maas/trunk |
Diff against target: |
499 lines (+297/-65) 3 files modified
src/maasserver/models/staticipaddress.py (+157/-63) src/maasserver/models/tests/test_staticipaddress.py (+139/-2) src/maasserver/websockets/handlers/tests/test_domain.py (+1/-0) |
To merge this branch: | bzr merge lp:~lamont/maas/bug-1642033 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Mike Pontillo (community) | Approve | ||
Gavin Panella (community) | Approve | ||
Review via email: mp+310920@code.launchpad.net |
Commit message
When generating DNS for IP addresses attached to DNSResource objects, only do so if we are generating an appropriate Domain's DNS.
Description of the change
When generating DNS for IP addresses attached to DNSResource objects, only do so if we are generating an appropriate Domain's DNS.
LaMont Jones (lamont) wrote : | # |
> Looks good. One thing does need fixing but I trust you'll do it before
> landing. I can't say that I follow the query changes fully, but they don't
> appear to be insane, and the tests do kind of make sense to me.
Given the major refactor in 5549, I'm going to request a new review.
Mike Pontillo (mpontillo) wrote : | # |
So this function is just scary. I think it's difficult to know whether our current test cases are sufficient. I have a lot of comments below. It was hard to make sense of this for review purposes; I had to go through line by line. Long story short:
- Needs better comments, docstrings, and general code readability. See comments below.
- It would probably be cleaner (and, more importantly, easier to support this code in the field) if we separate the logic encapsulated in this function to a set of a few cleanly written SQL views (maybe one to three).
- I would remove the parameter that special-cases the TTL and just have the query return both versions. Again, this approach would be easier to support if/when we move to a view-based approach. (And, worst case, if you want to keep it for performance reasons, you just don't select that column from the view.)
In a follow-on commit, we might look at exposing the output of this function via the API (again, for easier supportability).
Mike Pontillo (mpontillo) wrote : | # |
Oh yeah, review result. I'm on the fence. I guess this is a Needs Fixing. But I almost want to say "Approve", because assuming the code basically works, I don't want to look at it again. ;-)
So I'll settle on Needs Information. Since you pretty much ripped and replaced this entire function, what's your confidence level in the previous unit tests? Do they cover everything this [monster of a] query could do?
Mike Pontillo (mpontillo) wrote : | # |
Looking better, thanks for the fixes. I'm going to approve this now since they should be easy to address.
We'll think about moving this to a view-based approach in a future branch.
Things I'd still like to see fixed: we could use a properly-renamed helper function (so we can drop the comment about the awfulness of the name) and proper docstrings to explain the parameters.
Also, it's weird that we use "domain_or_subnet" after we know it's actually a Domain. See my suggested fix below.
Mike Pontillo (mpontillo) wrote : | # |
Looks like this needs a little more cleanup. See comments below on the function parameters. Also, we have a raw_ttl parameter to the function that isn't used by any caller. Coincidentally, there is a skipped test case in test_staticipad
@skip("XXX: GavinPanella 2016-02-24 bug=1549397: Fails spuriously.")
def test_get_
# ...
Let's take this opportunity to either remove raw_ttl or fix this test case.
Mike Pontillo (mpontillo) wrote : | # |
Looks good. Let's move on.
Preview Diff
1 | === modified file 'src/maasserver/models/staticipaddress.py' |
2 | --- src/maasserver/models/staticipaddress.py 2016-11-09 08:20:11 +0000 |
3 | +++ src/maasserver/models/staticipaddress.py 2016-11-17 18:03:27 +0000 |
4 | @@ -28,7 +28,6 @@ |
5 | IntegerField, |
6 | Manager, |
7 | PROTECT, |
8 | - Q, |
9 | ) |
10 | from maasserver import DefaultMeta |
11 | from maasserver.enum import ( |
12 | @@ -235,56 +234,152 @@ |
13 | return self._attempt_allocation( |
14 | requested_address, alloc_type, user=user, subnet=subnet) |
15 | |
16 | - def _get_user_reserved_mappings(self, domain_or_subnet, raw_ttl=False): |
17 | - # A poorly named routine these days, since it actually returns |
18 | - # addresses for anything with any DNSResource records as well. |
19 | - default_ttl = Config.objects.get_config('default_dns_ttl') |
20 | - qs = self.filter( |
21 | - Q(alloc_type=IPADDRESS_TYPE.USER_RESERVED) | |
22 | - Q(dnsresource__isnull=False)) |
23 | - # If this is a subnet, we need to ignore subnet.id, as per |
24 | - # get_hostname_ip_mapping(). LP#1600259 |
25 | - if isinstance(domain_or_subnet, Subnet): |
26 | - pass |
27 | - elif isinstance(domain_or_subnet, Domain): |
28 | - qs = qs.filter(dnsresource__domain_id=domain_or_subnet.id) |
29 | - qs = qs.prefetch_related("dnsresource_set") |
30 | - mappings = defaultdict(HostnameIPMapping) |
31 | - for instance in qs: |
32 | - ip = instance.ip |
33 | - rrset = instance.dnsresource_set.all() |
34 | - # 2016-01-20 LaMontJones N.B.: |
35 | - # Empirically, for dnsrr in instance.dnsresource_set.all(): ... |
36 | - # else: with a non-empty rrset yields both the for loop AND the |
37 | - # else clause. Wrapping it all in a if/else: avoids that issue. |
38 | - if rrset.count() > 0: |
39 | - for dnsrr in rrset: |
40 | - if dnsrr.name is None or dnsrr.name == '': |
41 | - hostname = get_ip_based_hostname(ip) |
42 | - hostname = "%s.%s" % ( |
43 | - get_ip_based_hostname(ip), |
44 | - Domain.objects.get_default_domain().name) |
45 | - else: |
46 | - hostname = '%s.%s' % (dnsrr.name, dnsrr.domain.name) |
47 | - if raw_ttl or dnsrr.address_ttl is not None: |
48 | - ttl = dnsrr.address_ttl |
49 | - elif dnsrr.domain.ttl is not None: |
50 | - ttl = dnsrr.domain.ttl |
51 | - else: |
52 | - ttl = default_ttl |
53 | - mappings[hostname].ttl = ttl |
54 | - mappings[hostname].ips.add(ip) |
55 | - else: |
56 | - # No DNSResource, but it's USER_RESERVED. |
57 | - domain = Domain.objects.get_default_domain() |
58 | - hostname = "%s.%s" % (get_ip_based_hostname(ip), domain.name) |
59 | - if raw_ttl or domain.ttl is not None: |
60 | - ttl = domain.ttl |
61 | - else: |
62 | - ttl = default_ttl |
63 | - mappings[hostname].ttl = ttl |
64 | - mappings[hostname].ips.add(ip) |
65 | - return mappings |
66 | + def _get_special_mappings(self, domain, raw_ttl=False): |
67 | + """Get the special mappings, possibly limited to a single Domain. |
68 | + |
69 | + This function is responsible for creating these mappings: |
70 | + - any USER_RESERVED IP, |
71 | + - any IP not associated with a Node, |
72 | + - any IP associated with a DNSResource. |
73 | + The caller is responsible for addresses otherwise derived from nodes. |
74 | + |
75 | + Because of how the get hostname_ip_mapping code works, we actually need |
76 | + to fetch ALL of the entries for subnets, but forward mappings need to |
77 | + be domain-specific. |
78 | + |
79 | + :param domain: limit return to just the given Domain. If anything |
80 | + other than a Domain is passed in (e.g., a Subnet or None), we |
81 | + return all of the reverse mappings. |
82 | + :param raw_ttl: Boolean, if True then just return the address_ttl, |
83 | + otherwise, coalesce the address_ttl to be the correct answer for |
84 | + zone generation. |
85 | + :return: a (default) dict of hostname: HostnameIPMapping entries. |
86 | + """ |
87 | + default_ttl = "%d" % Config.objects.get_config('default_dns_ttl') |
88 | + if isinstance(domain, Domain): |
89 | + # Domains are special in that we only want to have entries for the |
90 | + # domain that we were asked about. And they can possibly come from |
91 | + # either the child or the parent for glue. |
92 | + where_clause = """ |
93 | + AND ( |
94 | + dnsrr.dom2_id = %s OR |
95 | + node.dom2_id = %s OR |
96 | + dnsrr.domain_id = %s OR |
97 | + node.domain_id = %s |
98 | + """ |
99 | + query_parms = [domain.id, domain.id, domain.id, domain.id] |
100 | + # And the default domain is extra special, since it needs to have |
101 | + # A/AAAA RRs for any USER_RESERVED addresses that have no name |
102 | + # otherwise attached to them. |
103 | + if domain.is_default(): |
104 | + where_clause += """ OR ( |
105 | + dnsrr.fqdn IS NULL AND |
106 | + node.fqdn IS NULL) |
107 | + """ |
108 | + where_clause += ")" |
109 | + else: |
110 | + # There is nothing special about the query for subnets. |
111 | + domain = None |
112 | + where_clause = "" |
113 | + query_parms = [] |
114 | + # raw_ttl says that we don't coalesce, but we need to pick one, so we |
115 | + # go with DNSResource if it is involved. |
116 | + if raw_ttl: |
117 | + ttl_clause = """COALESCE(dnsrr.address_ttl, node.address_ttl)""" |
118 | + else: |
119 | + ttl_clause = """ |
120 | + COALESCE( |
121 | + dnsrr.address_ttl, |
122 | + dnsrr.ttl, |
123 | + node.address_ttl, |
124 | + node.ttl, |
125 | + %s)""" % default_ttl |
126 | + # And here is the SQL query of doom. Build up inner selects to get the |
127 | + # view of a DNSResource (and Node) that we need, and finally use |
128 | + # domain2 to handle the case where an FQDN is also the name of a domain |
129 | + # that we know. |
130 | + sql_query = """ |
131 | + SELECT |
132 | + COALESCE(dnsrr.fqdn, node.fqdn) AS fqdn, |
133 | + node.system_id, |
134 | + node.node_type, |
135 | + """ + ttl_clause + """ AS ttl, |
136 | + staticip.ip |
137 | + FROM |
138 | + maasserver_staticipaddress AS staticip |
139 | + LEFT JOIN ( |
140 | + /* Create a dnsrr that has what we need. */ |
141 | + SELECT |
142 | + CASE WHEN dnsrr.name = '@' THEN |
143 | + dom.name |
144 | + ELSE |
145 | + CONCAT(dnsrr.name, '.', dom.name) |
146 | + END AS fqdn, |
147 | + dom.name as dom_name, |
148 | + dnsrr.domain_id, |
149 | + dnsrr.address_ttl, |
150 | + dom.ttl, |
151 | + dia.staticipaddress_id AS dnsrr_sip_id, |
152 | + dom2.id AS dom2_id |
153 | + FROM maasserver_dnsresource_ip_addresses AS dia |
154 | + JOIN maasserver_dnsresource AS dnsrr ON |
155 | + dia.dnsresource_id = dnsrr.id |
156 | + JOIN maasserver_domain AS dom ON |
157 | + dnsrr.domain_id = dom.id |
158 | + LEFT JOIN maasserver_domain AS dom2 ON |
159 | + CONCAT(dnsrr.name, '.', dom.name) = dom2.name OR ( |
160 | + dnsrr.name = '@' AND |
161 | + dom.name SIMILAR TO CONCAT('[-A-Za-z0-9]*.', dom2.name) |
162 | + ) |
163 | + ) AS dnsrr ON |
164 | + dnsrr_sip_id = staticip.id |
165 | + LEFT JOIN ( |
166 | + /* Create a node that has what we need. */ |
167 | + SELECT |
168 | + CONCAT(nd.hostname, '.', dom.name) AS fqdn, |
169 | + dom.name as dom_name, |
170 | + nd.system_id, |
171 | + nd.node_type, |
172 | + nd.domain_id, |
173 | + nd.address_ttl, |
174 | + dom.ttl, |
175 | + iia.staticipaddress_id AS node_sip_id, |
176 | + dom2.id AS dom2_id |
177 | + FROM maasserver_interface_ip_addresses AS iia |
178 | + JOIN maasserver_interface AS iface ON |
179 | + iia.interface_id = iface.id |
180 | + JOIN maasserver_node AS nd ON |
181 | + iface.node_id = nd.id |
182 | + JOIN maasserver_domain AS dom ON |
183 | + nd.domain_id = dom.id |
184 | + LEFT JOIN maasserver_domain AS dom2 ON |
185 | + CONCAT(nd.hostname, '.', dom.name) = dom2.name |
186 | + ) AS node ON |
187 | + node_sip_id = staticip.id |
188 | + WHERE |
189 | + staticip.ip IS NOT NULL AND |
190 | + host(staticip.ip) != '' AND |
191 | + ( |
192 | + staticip.alloc_type = %s OR |
193 | + node.fqdn IS NULL OR |
194 | + dnsrr IS NOT NULL |
195 | + )""" + where_clause + """ |
196 | + """ |
197 | + default_domain = Domain.objects.get_default_domain() |
198 | + mapping = defaultdict(HostnameIPMapping) |
199 | + cursor = connection.cursor() |
200 | + query_parms = [IPADDRESS_TYPE.USER_RESERVED] + query_parms |
201 | + cursor.execute(sql_query, query_parms) |
202 | + for (fqdn, system_id, node_type, ttl, |
203 | + ip) in cursor.fetchall(): |
204 | + if fqdn is None or fqdn == '': |
205 | + fqdn = "%s.%s" % ( |
206 | + get_ip_based_hostname(ip), default_domain.name) |
207 | + mapping[fqdn].node_type = node_type |
208 | + mapping[fqdn].system_id = system_id |
209 | + mapping[fqdn].ttl = ttl |
210 | + mapping[fqdn].ips.add(ip) |
211 | + return mapping |
212 | |
213 | def get_hostname_ip_mapping(self, domain_or_subnet, raw_ttl=False): |
214 | """Return hostname mappings for `StaticIPAddress` entries. |
215 | @@ -326,7 +421,7 @@ |
216 | node.boot_interface_id = parent.id |
217 | ), |
218 | False |
219 | - ) as is_boot |
220 | + ) AS is_boot |
221 | FROM |
222 | maasserver_interface AS interface |
223 | LEFT OUTER JOIN maasserver_interfacerelationship AS rel ON |
224 | @@ -335,7 +430,7 @@ |
225 | rel.parent_id = parent.id |
226 | JOIN maasserver_node AS node ON |
227 | node.id = interface.node_id |
228 | - JOIN maasserver_domain as domain ON |
229 | + JOIN maasserver_domain AS domain ON |
230 | domain.id = node.domain_id |
231 | JOIN maasserver_interface_ip_addresses AS link ON |
232 | link.interface_id = interface.id |
233 | @@ -349,15 +444,14 @@ |
234 | # domains. domain2.name will be non-null if this host's fqdn is the |
235 | # name of a domain in MAAS. |
236 | sql_query += """ |
237 | - LEFT JOIN maasserver_domain as domain2 ON |
238 | + LEFT JOIN maasserver_domain AS domain2 ON |
239 | /* Pick up another copy of domain looking for instances of |
240 | * nodes a the top of a domain. |
241 | - */ |
242 | - domain2.name = CONCAT(node.hostname, '.', domain.name) |
243 | + */ domain2.name = CONCAT(node.hostname, '.', domain.name) |
244 | WHERE |
245 | - (domain2.name IS NOT NULL OR node.domain_id = %s) AND |
246 | + (domain2.id = %s OR node.domain_id = %s) AND |
247 | """ |
248 | - query_parms = [domain_or_subnet.id, ] |
249 | + query_parms = [domain_or_subnet.id, domain_or_subnet.id, ] |
250 | else: |
251 | # For subnets, we need ALL the names, so that we can correctly |
252 | # identify which ones should have the FQDN. dns/zonegenerator.py |
253 | @@ -418,7 +512,7 @@ |
254 | maasserver_interface AS interface |
255 | JOIN maasserver_node AS node ON |
256 | node.id = interface.node_id |
257 | - JOIN maasserver_domain as domain ON |
258 | + JOIN maasserver_domain AS domain ON |
259 | domain.id = node.domain_id |
260 | JOIN maasserver_interface_ip_addresses AS link ON |
261 | link.interface_id = interface.id |
262 | @@ -428,14 +522,14 @@ |
263 | if isinstance(domain_or_subnet, Domain): |
264 | # This logic is similar to the logic in sql_query above. |
265 | iface_sql_query += """ |
266 | - LEFT JOIN maasserver_domain as domain2 ON |
267 | + LEFT JOIN maasserver_domain AS domain2 ON |
268 | /* Pick up another copy of domain looking for instances of |
269 | * the name as the top of a domain. |
270 | */ |
271 | domain2.name = CONCAT( |
272 | interface.name, '.', node.hostname, '.', domain.name) |
273 | WHERE |
274 | - (domain2.name IS NOT NULL OR node.domain_id = %s) AND |
275 | + (domain2.id = %s OR node.domain_id = %s) AND |
276 | """ |
277 | else: |
278 | # For subnets, we need ALL the names, so that we can correctly |
279 | @@ -455,7 +549,7 @@ |
280 | """ |
281 | # We get user reserved et al mappings first, so that we can overwrite |
282 | # TTL as we process the return from the SQL horror above. |
283 | - mapping = self._get_user_reserved_mappings(domain_or_subnet) |
284 | + mapping = self._get_special_mappings(domain_or_subnet, raw_ttl) |
285 | # All of the mappings that we got mean that we will only want to add |
286 | # addresses for the boot interface (is_boot == True). |
287 | iface_is_boot = defaultdict(bool, { |
288 | @@ -482,7 +576,7 @@ |
289 | # Next, get all the addresses, on all the interfaces, and add the ones |
290 | # that are not already present on the FQDN as $IFACE.$FQDN. Exclude |
291 | # any discovered addresses once there are any non-discovered addresses. |
292 | - cursor.execute(iface_sql_query, (domain_or_subnet.id,)) |
293 | + cursor.execute(iface_sql_query, query_parms) |
294 | for (fqdn, system_id, node_type, ttl, |
295 | ip, iface_name, assigned) in cursor.fetchall(): |
296 | if assigned: |
297 | |
298 | === modified file 'src/maasserver/models/tests/test_staticipaddress.py' |
299 | --- src/maasserver/models/tests/test_staticipaddress.py 2016-11-09 08:14:00 +0000 |
300 | +++ src/maasserver/models/tests/test_staticipaddress.py 2016-11-17 18:03:27 +0000 |
301 | @@ -40,6 +40,7 @@ |
302 | MAASServerTestCase, |
303 | MAASTransactionServerTestCase, |
304 | ) |
305 | +from maasserver.utils.dns import get_ip_based_hostname |
306 | from maasserver.utils.orm import ( |
307 | reload_object, |
308 | transactional, |
309 | @@ -447,7 +448,6 @@ |
310 | actual = StaticIPAddress.objects.get_hostname_ip_mapping(dom) |
311 | self.assertItemsEqual(expected_mapping, actual) |
312 | |
313 | - @skip("XXX: GavinPanella 2016-02-24 bug=1549397: Fails spuriously.") |
314 | def test_get_hostname_ip_mapping_returns_raw_ttl(self): |
315 | # We create 2 domains, one with a ttl, one withoout. |
316 | # Within each domain, create a node with an address_ttl, and one |
317 | @@ -941,6 +941,7 @@ |
318 | class TestUserReservedStaticIPAddress(MAASServerTestCase): |
319 | |
320 | def test_user_reserved_addresses_have_default_hostnames(self): |
321 | + # Reserved IPs get default hostnames when none are given. |
322 | subnet = factory.make_Subnet() |
323 | num_ips = randint(3, 5) |
324 | ips = [ |
325 | @@ -948,11 +949,13 @@ |
326 | subnet=subnet, alloc_type=IPADDRESS_TYPE.USER_RESERVED) |
327 | for _ in range(num_ips) |
328 | ] |
329 | - mappings = StaticIPAddress.objects._get_user_reserved_mappings( |
330 | + mappings = StaticIPAddress.objects._get_special_mappings( |
331 | subnet) |
332 | self.expectThat(mappings, HasLength(len(ips))) |
333 | |
334 | def test_user_reserved_addresses_included_in_get_hostname_ip_mapping(self): |
335 | + # Generate several IPs, with names in domain0, and make sure they don't |
336 | + # show up in domain1. |
337 | num_ips = randint(3, 5) |
338 | domain0 = Domain.objects.get_default_domain() |
339 | domain1 = factory.make_Domain() |
340 | @@ -968,6 +971,8 @@ |
341 | self.expectThat(mappings, HasLength(0)) |
342 | |
343 | def test_user_reserved_addresses_included_in_correct_domains(self): |
344 | + # Generate addresses in two domains, and make sure that they wind up in |
345 | + # the right domains, and not in other domains. |
346 | domain0 = Domain.objects.get_default_domain() |
347 | domain1 = factory.make_Domain() |
348 | domain2 = factory.make_Domain() |
349 | @@ -992,6 +997,138 @@ |
350 | mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain2) |
351 | self.expectThat(mappings, HasLength(len(ips2))) |
352 | |
353 | + def test_dns_resources_only_have_correct_domain(self): |
354 | + # Create two DNS resources pointing to the same IP, and make sure that |
355 | + # they only get the forward mapping that they should have, but get both |
356 | + # reverse mappings, as is proper. |
357 | + domain0 = Domain.objects.get_default_domain() |
358 | + domain1 = factory.make_Domain() |
359 | + domain2 = factory.make_Domain() |
360 | + subnet = factory.make_Subnet() |
361 | + ip = factory.make_StaticIPAddress(subnet=subnet) |
362 | + name1 = factory.make_name('label') |
363 | + name2 = factory.make_name('label') |
364 | + factory.make_DNSResource(name=name1, ip_addresses=[ip], domain=domain1) |
365 | + factory.make_DNSResource(name=name2, ip_addresses=[ip], domain=domain2) |
366 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0) |
367 | + self.expectThat(mappings, HasLength(0)) |
368 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain1) |
369 | + self.expectThat(mappings, HasLength(1)) |
370 | + self.assertEqual( |
371 | + mappings.get('%s.%s' % (name1, domain1.name)), |
372 | + HostnameIPMapping(None, 30, {ip.ip}, None)) |
373 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain2) |
374 | + self.expectThat(mappings, HasLength(1)) |
375 | + self.assertEqual( |
376 | + mappings.get('%s.%s' % (name2, domain2.name)), |
377 | + HostnameIPMapping(None, 30, {ip.ip}, None)) |
378 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet) |
379 | + self.assertEqual( |
380 | + mappings, { |
381 | + "%s.%s" % (name1, domain1.name): HostnameIPMapping( |
382 | + None, 30, {ip.ip}, None), |
383 | + "%s.%s" % (name2, domain2.name): HostnameIPMapping( |
384 | + None, 30, {ip.ip}, None)}) |
385 | + |
386 | + def test_user_reserved_IP_on_node_or_default_domain_not_both(self): |
387 | + # ip1 gets assigned to an interface on a node in domain1 |
388 | + # ip2 is not assigned to anything. |
389 | + # ip1 should show up for the node, in domain1, and the subnet. |
390 | + # ip2 should show up in the default domain, and the subnet. |
391 | + domain0 = Domain.objects.get_default_domain() |
392 | + domain1 = factory.make_Domain() |
393 | + subnet = factory.make_Subnet() |
394 | + ip1 = factory.make_StaticIPAddress(subnet=subnet) |
395 | + ip2 = factory.make_StaticIPAddress(subnet=subnet) |
396 | + name2 = "%s.%s" % (get_ip_based_hostname(ip2.ip), domain0.name) |
397 | + node = factory.make_Node( |
398 | + interface=True, |
399 | + domain=domain1, |
400 | + vlan=subnet.vlan) |
401 | + node.interface_set.first().ip_addresses.add(ip1) |
402 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0) |
403 | + self.expectThat(mappings, HasLength(1)) |
404 | + self.assertEqual( |
405 | + mappings, { |
406 | + name2: HostnameIPMapping(None, 30, {ip2.ip}, None)}) |
407 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain1) |
408 | + self.expectThat(mappings, HasLength(1)) |
409 | + self.assertEqual( |
410 | + mappings.get(node.fqdn), |
411 | + HostnameIPMapping(node.system_id, 30, {ip1.ip}, node.node_type)) |
412 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet) |
413 | + self.assertEqual( |
414 | + mappings, { |
415 | + node.fqdn: HostnameIPMapping( |
416 | + node.system_id, 30, {ip1.ip}, node.node_type), |
417 | + name2: HostnameIPMapping(None, 30, {ip2.ip}, None)}) |
418 | + |
419 | + def test_node_glue_correctly_generated(self): |
420 | + # Given node "foo.bar.com" and a "foo.bar.com" domain (in addition to |
421 | + # "bar.com"), with an IP associated with it, make sure that both |
422 | + # domains are correctly populated. |
423 | + domain0 = Domain.objects.get_default_domain() |
424 | + parent = factory.make_Domain(name=factory.make_name('parent')) |
425 | + name = factory.make_name('child') |
426 | + domain = factory.make_Domain(name="%s.%s" % (name, parent.name)) |
427 | + subnet = factory.make_Subnet() |
428 | + ip = factory.make_StaticIPAddress(subnet=subnet) |
429 | + node = factory.make_Node( |
430 | + interface=True, |
431 | + domain=parent, |
432 | + hostname=name, |
433 | + vlan=subnet.vlan) |
434 | + node.interface_set.first().ip_addresses.add(ip) |
435 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0) |
436 | + self.expectThat(mappings, HasLength(0)) |
437 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(parent) |
438 | + self.expectThat(mappings, HasLength(1)) |
439 | + self.assertEqual( |
440 | + mappings.get(node.fqdn), |
441 | + HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type)) |
442 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain) |
443 | + self.expectThat(mappings, HasLength(1)) |
444 | + self.assertEqual( |
445 | + mappings.get(node.fqdn), |
446 | + HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type)) |
447 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet) |
448 | + self.assertEqual( |
449 | + mappings, { |
450 | + node.fqdn: HostnameIPMapping( |
451 | + node.system_id, 30, {ip.ip}, node.node_type)}) |
452 | + |
453 | + def test_dnsrr_glue_correctly_generated(self): |
454 | + # Given DNSResource "foo.bar.com" and a "foo.bar.com" domain (in |
455 | + # addition to "bar.com"), with an IP associated with it, make sure that |
456 | + # both domains are correctly populated. |
457 | + domain0 = Domain.objects.get_default_domain() |
458 | + parent = factory.make_Domain(name=factory.make_name('parent')) |
459 | + name = factory.make_name('child') |
460 | + domain = factory.make_Domain(name="%s.%s" % (name, parent.name)) |
461 | + subnet = factory.make_Subnet() |
462 | + ip = factory.make_StaticIPAddress(subnet=subnet) |
463 | + dnsrr = factory.make_DNSResource( |
464 | + domain=domain, |
465 | + name='@', |
466 | + ip_addresses=[ip]) |
467 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0) |
468 | + self.expectThat(mappings, HasLength(0)) |
469 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(parent) |
470 | + self.expectThat(mappings, HasLength(1)) |
471 | + self.assertEqual( |
472 | + mappings.get(dnsrr.fqdn), |
473 | + HostnameIPMapping(None, 30, {ip.ip}, None)) |
474 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain) |
475 | + self.expectThat(mappings, HasLength(1)) |
476 | + self.assertEqual( |
477 | + mappings.get(dnsrr.fqdn), |
478 | + HostnameIPMapping(None, 30, {ip.ip}, None)) |
479 | + mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet) |
480 | + self.assertEqual( |
481 | + mappings, { |
482 | + dnsrr.fqdn: HostnameIPMapping( |
483 | + None, 30, {ip.ip}, None)}) |
484 | + |
485 | |
486 | class TestRenderJSON(MAASServerTestCase): |
487 | |
488 | |
489 | === modified file 'src/maasserver/websockets/handlers/tests/test_domain.py' |
490 | --- src/maasserver/websockets/handlers/tests/test_domain.py 2016-09-06 17:02:03 +0000 |
491 | +++ src/maasserver/websockets/handlers/tests/test_domain.py 2016-11-17 18:03:27 +0000 |
492 | @@ -86,6 +86,7 @@ |
493 | def test_list(self): |
494 | user = factory.make_User() |
495 | handler = DomainHandler(user, {}) |
496 | + Domain.objects.get_default_domain() |
497 | factory.make_Domain() |
498 | expected_domains = [ |
499 | self.dehydrate_domain(domain, for_list=True) |
Looks good. One thing does need fixing but I trust you'll do it before landing. I can't say that I follow the query changes fully, but they don't appear to be insane, and the tests do kind of make sense to me.