Merge lp:~lamont/maas/bug-1677741 into lp:~maas-committers/maas/trunk

[LaMont Jones]

In some cases, DNS Resource address records were ignored.

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.

[Mike Pontillo (mpontillo)]

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
    test__default_domain_is_extra_special_ipv6
    test__domains_are_special
    test__attached_addresses_only_map_back_to_node

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.

review: Needs Fixing

[LaMont Jones (lamont)]

> 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
> test__default_domain_is_extra_special_ipv6
These are both covered via:
    test_user_reserved_addresses_included_in_get_hostname_ip_mapping
    test_user_reserved_addresses_included_in_correct_domains
    test_user_reserved_IP_on_node_or_default_domain_not_both
I have updated them to be more clear about what they are expecting to see.

> test__domains_are_special
This is covered (among others) via:
    test_user_reserved_addresses_included_in_correct_domains

> test__attached_addresses_only_map_back_to_node
This is covered via:
    test_get_hostname_ip_mapping_handles_dnsrr_node_collision

> 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)]

Thanks for the cleanup; I tested this and it's working well!

review: Approve

=== modified file 'src/maasserver/models/staticipaddress.py'
--- src/maasserver/models/staticipaddress.py	2017-03-25 22:29:09 +0000
+++ src/maasserver/models/staticipaddress.py	2017-03-31 21:12:36 +0000
@@ -252,14 +252,21 @@
         """Get the special mappings, possibly limited to a single Domain.
 
         This function is responsible for creating these mappings:
-        - any USER_RESERVED IP,
+        - any USER_RESERVED IP that has no name (dnsrr or node),
         - any IP not associated with a Node,
         - any IP associated with a DNSResource.
+
+        Addresses that are associated with both a Node and a DNSResource behave
+        thusly:
+        - Both forward mappings include the address
+        - The reverse mapping points only to the Node (and is the
+          responsibility of the caller.)
+
         The caller is responsible for addresses otherwise derived from nodes.
 
         Because of how the get hostname_ip_mapping code works, we actually need
-        to fetch ALL of the entries for subnets, but forward mappings need to
-        be domain-specific.
+        to fetch ALL of the entries for subnets, but forward mappings are
+        domain-specific.
 
         :param domain: limit return to just the given Domain.  If anything
             other than a Domain is passed in (e.g., a Subnet or None), we
@@ -270,32 +277,6 @@
         :return: a (default) dict of hostname: HostnameIPMapping entries.
         """
         default_ttl = "%d" % Config.objects.get_config('default_dns_ttl')
-        if isinstance(domain, Domain):
-            # Domains are special in that we only want to have entries for the
-            # domain that we were asked about.  And they can possibly come from
-            # either the child or the parent for glue.
-            where_clause = """
-                AND (
-                    dnsrr.dom2_id = %s OR
-                    node.dom2_id = %s OR
-                    dnsrr.domain_id = %s OR
-                    node.domain_id = %s
-            """
-            query_parms = [domain.id, domain.id, domain.id, domain.id]
-            # And the default domain is extra special, since it needs to have
-            # A/AAAA RRs for any USER_RESERVED addresses that have no name
-            # otherwise attached to them.
-            if domain.is_default():
-                where_clause += """ OR (
-                    dnsrr.fqdn IS NULL AND
-                    node.fqdn IS NULL)
-                """
-            where_clause += ")"
-        else:
-            # There is nothing special about the query for subnets.
-            domain = None
-            where_clause = ""
-            query_parms = []
         # raw_ttl says that we don't coalesce, but we need to pick one, so we
         # go with DNSResource if it is involved.
         if raw_ttl:
@@ -371,27 +352,78 @@
                 ) AS node ON
                     node_sip_id = staticip.id
             WHERE
-                staticip.ip IS NOT NULL AND
-                host(staticip.ip) != '' AND
-                (
-                    staticip.alloc_type = %s OR
-                    node.fqdn IS NULL OR
-                    dnsrr IS NOT NULL
-                )""" + where_clause + """
-        """
+                (staticip.ip IS NOT NULL AND host(staticip.ip) != '') AND
+                """
+
+        query_parms = []
+        if isinstance(domain, Domain):
+            if domain.is_default():
+                # The default domain is extra special, since it needs to have
+                # A/AAAA RRs for any USER_RESERVED addresses that have no name
+                # otherwise attached to them.
+                # We need to get all of the entries that are:
+                # - in this domain and have a dnsrr associated, OR
+                # - are USER_RESERVED and have NO fqdn associated at all.
+                sql_query += """ ((
+                        staticip.alloc_type = %s AND
+                        dnsrr.fqdn IS NULL AND
+                        node.fqdn IS NULL
+                    ) OR (
+                        dnsrr.fqdn IS NOT NULL AND
+                        (
+                            dnsrr.dom2_id = %s OR
+                            node.dom2_id = %s OR
+                            dnsrr.domain_id = %s OR
+                            node.domain_id = %s)))"""
+                query_parms += [IPADDRESS_TYPE.USER_RESERVED]
+            else:
+                # For domains, we only need answers for the domain we were
+                # given.  These can can possibly come from either the child or
+                # the parent for glue.  Anything with a node associated will be
+                # found inside of get_hostname_ip_mapping() - we need any
+                # entries that are:
+                # - in this domain and have a dnsrr associated.
+                sql_query += """ (
+                    dnsrr.fqdn IS NOT NULL AND
+                    (
+                        dnsrr.dom2_id = %s OR
+                        node.dom2_id = %s OR
+                        dnsrr.domain_id = %s OR
+                        node.domain_id = %s))"""
+            query_parms += [domain.id, domain.id, domain.id, domain.id]
+        else:
+            # In the subnet map, addresses attached to nodes only map back to
+            # the node, since some things don't like multiple PTR RRs in
+            # answers from the DNS.
+            # Since that is handled in get_hostname_ip_mapping, we exclude
+            # anything where the node also has a link to the address.
+            domain = None
+            sql_query += """ ((
+                    node.fqdn IS NULL AND dnsrr.fqdn IS NOT NULL
+                ) OR (
+                    staticip.alloc_type = %s AND
+                    dnsrr.fqdn IS NULL AND
+                    node.fqdn IS NULL))"""
+            query_parms += [IPADDRESS_TYPE.USER_RESERVED]
+
         default_domain = Domain.objects.get_default_domain()
         mapping = defaultdict(HostnameIPMapping)
         cursor = connection.cursor()
-        query_parms = [IPADDRESS_TYPE.USER_RESERVED] + query_parms
         cursor.execute(sql_query, query_parms)
-        for (fqdn, system_id, node_type, ttl,
-                ip) in cursor.fetchall():
+        for (fqdn, system_id, node_type, ttl, ip) in cursor.fetchall():
             if fqdn is None or fqdn == '':
                 fqdn = "%s.%s" % (
                     get_ip_based_hostname(ip), default_domain.name)
-            mapping[fqdn].node_type = node_type
-            mapping[fqdn].system_id = system_id
-            mapping[fqdn].ttl = ttl
+            # It is possible that there are both Node and DNSResource entries
+            # for this fqdn.  If we have any system_id, preserve it.  Ditto for
+            # TTL.  It is left as an exercise for the admin to make sure that
+            # the any non-default TTL applied to the Node and DNSResource are
+            # equal.
+            if system_id is not None:
+                mapping[fqdn].node_type = node_type
+                mapping[fqdn].system_id = system_id
+            if ttl is not None:
+                mapping[fqdn].ttl = ttl
             mapping[fqdn].ips.add(ip)
         return mapping
 
 
=== modified file 'src/maasserver/models/tests/test_staticipaddress.py'
--- src/maasserver/models/tests/test_staticipaddress.py	2017-03-25 22:29:09 +0000
+++ src/maasserver/models/tests/test_staticipaddress.py	2017-03-31 21:12:36 +0000
@@ -10,7 +10,6 @@
     shuffle,
 )
 import threading
-from unittest import skip
 from unittest.mock import sentinel
 
 from django.core.exceptions import ValidationError
@@ -822,7 +821,6 @@
                 node.system_id, 30, {vlanip.ip}, node.node_type)}
         self.assertEqual(expected_mapping, mapping)
 
-    @skip("XXX: GavinPanella 2016-04-27 bug=1556188: Fails spuriously.")
     def test_get_hostname_ip_mapping_returns_domain_head_ips(self):
         parent = factory.make_Domain()
         name = factory.make_name()
@@ -923,6 +921,67 @@
             }
         self.assertEqual(expected_mapping, mapping)
 
+    def test_get_hostname_ip_mapping_handles_dnsrr_node_collision(self):
+        hostname = factory.make_name('hostname')
+        domainname = factory.make_name('domain')
+        # Randomly make our domain either the default, or non-default.
+        # This does not change what we expect the answer to be.
+        if factory.pick_bool():
+            Domain.objects.get_default_domain()
+        domain = factory.make_Domain(name=domainname)
+        full_hostname = "%s.%s" % (hostname, domainname)
+        dnsrrname = factory.make_name('dnsrrname')
+        full_dnsrrname = "%s.%s" % (dnsrrname, domainname)
+
+        subnet = factory.make_Subnet()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            interface=True, hostname=full_hostname, interface_count=3,
+            subnet=subnet)
+        boot_interface = node.get_boot_interface()
+        staticip = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY,
+            ip=factory.pick_ip_in_Subnet(subnet),
+            subnet=subnet, interface=boot_interface)
+        iface2 = node.interface_set.exclude(id=boot_interface.id).first()
+        sip2 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY,
+            ip=factory.pick_ip_in_Subnet(subnet),
+            subnet=subnet, interface=iface2)
+        sip3 = factory.make_StaticIPAddress(
+            alloc_type=IPADDRESS_TYPE.STICKY,
+            ip=factory.pick_ip_in_Subnet(subnet),
+            subnet=subnet)
+        # Create a DNSResource with one address on the node, and one not.
+        # The forward mapping for the dnsrr points to both IPs.
+        # The subnet mapping for the IP on the node only points to the node.
+        # The other IP points to the DNSRR name.
+        factory.make_DNSResource(
+            name=dnsrrname, domain=domain, ip_addresses=[sip2, sip3])
+        mapping = StaticIPAddress.objects.get_hostname_ip_mapping(domain)
+        expected = {
+            full_hostname:
+                HostnameIPMapping(
+                    node.system_id, 30, {staticip.ip}, node.node_type),
+            "%s.%s" % (iface2.name, full_hostname):
+                HostnameIPMapping(
+                    node.system_id, 30, {sip2.ip}, node.node_type),
+            full_dnsrrname:
+                HostnameIPMapping(
+                    node.system_id, 30, {sip2.ip, sip3.ip}, node.node_type),
+        }
+        self.assertEqual(expected, mapping)
+        mapping = StaticIPAddress.objects.get_hostname_ip_mapping(subnet)
+        expected = {
+            full_hostname:
+                HostnameIPMapping(
+                    node.system_id, 30, {staticip.ip}, node.node_type),
+            "%s.%s" % (iface2.name, full_hostname):
+                HostnameIPMapping(
+                    node.system_id, 30, {sip2.ip}, node.node_type),
+            full_dnsrrname:
+                HostnameIPMapping(None, 30, {sip3.ip}, None),
+        }
+
 
 class TestStaticIPAddress(MAASServerTestCase):
 
@@ -1012,23 +1071,34 @@
                 alloc_type=IPADDRESS_TYPE.USER_RESERVED)
             for _ in range(num_ips)
         ]
+        expected = {
+            ip.dnsresource_set.first().fqdn:
+                HostnameIPMapping(None, 30, {ip.ip}, None)
+            for ip in ips
+        }
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0)
-        self.expectThat(mappings, HasLength(len(ips)))
+        self.assertEqual(expected, mappings)
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain1)
         self.expectThat(mappings, HasLength(0))
 
     def test_user_reserved_addresses_included_in_correct_domains(self):
-        # Generate addresses in two domains, and make sure that they wind up in
-        # the right domains, and not in other domains.
+        # Generate some addresses with no names attached, and some more in each
+        # of two (non-default) domains.  Make sure that they wind up in the
+        # right domains, and not in other domains.
         domain0 = Domain.objects.get_default_domain()
         domain1 = factory.make_Domain()
         domain2 = factory.make_Domain()
+        ips0 = [
+            factory.make_StaticIPAddress(
+                alloc_type=IPADDRESS_TYPE.USER_RESERVED)
+            for _ in range(randint(3, 5))
+        ]
         ips1 = [
             factory.make_StaticIPAddress(
                 hostname="%s.%s" % (
                     factory.make_name('host'), domain1.name),
                 alloc_type=IPADDRESS_TYPE.USER_RESERVED)
-            for _ in range(randint(3, 5))
+            for _ in range(randint(6, 9))
         ]
         ips2 = [
             factory.make_StaticIPAddress(
@@ -1037,12 +1107,27 @@
                 alloc_type=IPADDRESS_TYPE.USER_RESERVED)
             for _ in range(randint(1, 2))
         ]
+        expected = {
+            "%s.%s" % (get_ip_based_hostname(ip.ip), domain0.name):
+                HostnameIPMapping(None, 30, {ip.ip}, None)
+            for ip in ips0
+        }
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0)
-        self.expectThat(mappings, HasLength(0))
+        self.assertEqual(expected, mappings)
+        expected = {
+            ip.dnsresource_set.first().fqdn:
+                HostnameIPMapping(None, 30, {ip.ip}, None)
+            for ip in ips1
+        }
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain1)
-        self.expectThat(mappings, HasLength(len(ips1)))
+        self.assertEqual(expected, mappings)
+        expected = {
+            ip.dnsresource_set.first().fqdn:
+                HostnameIPMapping(None, 30, {ip.ip}, None)
+            for ip in ips2
+        }
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain2)
-        self.expectThat(mappings, HasLength(len(ips2)))
+        self.assertEqual(expected, mappings)
 
     def test_dns_resources_only_have_correct_domain(self):
         # Create two DNS resources pointing to the same IP, and make sure that
@@ -1080,35 +1165,54 @@
     def test_user_reserved_IP_on_node_or_default_domain_not_both(self):
         # ip1 gets assigned to an interface on a node in domain1
         # ip2 is not assigned to anything.
+        # ip3 gets assigned to a DNS resource in domain2
         # ip1 should show up for the node, in domain1, and the subnet.
         # ip2 should show up in the default domain, and the subnet.
+        # ip3 should show up in domain2, and the subnet.
         domain0 = Domain.objects.get_default_domain()
         domain1 = factory.make_Domain()
+        domain2 = factory.make_Domain()
+        rrname = factory.make_name('dnsrr')
         subnet = factory.make_Subnet()
         ip1 = factory.make_StaticIPAddress(subnet=subnet)
-        ip2 = factory.make_StaticIPAddress(subnet=subnet)
+        ip2 = factory.make_StaticIPAddress(
+            subnet=subnet, alloc_type=IPADDRESS_TYPE.USER_RESERVED)
+        ip3 = factory.make_StaticIPAddress(
+            subnet=subnet, alloc_type=IPADDRESS_TYPE.USER_RESERVED)
+        dnsrr = factory.make_DNSResource(
+            name=rrname, domain=domain2, ip_addresses=[ip3])
         name2 = "%s.%s" % (get_ip_based_hostname(ip2.ip), domain0.name)
         node = factory.make_Node(
             interface=True,
             domain=domain1,
             vlan=subnet.vlan)
         node.interface_set.first().ip_addresses.add(ip1)
+        expected0 = {
+            name2: HostnameIPMapping(None, 30, {ip2.ip}, None),
+        }
+        expected1 = {
+            node.fqdn:
+                HostnameIPMapping(
+                    node.system_id, 30, {ip1.ip}, node.node_type),
+        }
+        expected2 = {
+            dnsrr.fqdn: HostnameIPMapping(None, 30, {ip3.ip}, None),
+        }
+        expected_subnet = {
+            name2: HostnameIPMapping(None, 30, {ip2.ip}, None),
+            node.fqdn:
+                HostnameIPMapping(
+                    node.system_id, 30, {ip1.ip}, node.node_type),
+            dnsrr.fqdn: HostnameIPMapping(None, 30, {ip3.ip}, None),
+        }
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain0)
-        self.expectThat(mappings, HasLength(1))
-        self.assertEqual(
-            mappings, {
-                name2: HostnameIPMapping(None, 30, {ip2.ip}, None)})
+        self.assertEqual(expected0, mappings)
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain1)
-        self.expectThat(mappings, HasLength(1))
-        self.assertEqual(
-            mappings.get(node.fqdn),
-            HostnameIPMapping(node.system_id, 30, {ip1.ip}, node.node_type))
+        self.assertEqual(expected1, mappings)
+        mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain2)
+        self.assertEqual(expected2, mappings)
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet)
-        self.assertEqual(
-            mappings, {
-                node.fqdn: HostnameIPMapping(
-                    node.system_id, 30, {ip1.ip}, node.node_type),
-                name2: HostnameIPMapping(None, 30, {ip2.ip}, None)})
+        self.assertEqual(expected_subnet, mappings)
 
     def test_node_glue_correctly_generated(self):
         # Given node "foo.bar.com" and a "foo.bar.com" domain (in addition to
@@ -1131,18 +1235,17 @@
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(parent)
         self.expectThat(mappings, HasLength(1))
         self.assertEqual(
-            mappings.get(node.fqdn),
-            HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type))
+            HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type),
+            mappings.get(node.fqdn))
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain)
         self.expectThat(mappings, HasLength(1))
         self.assertEqual(
-            mappings.get(node.fqdn),
-            HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type))
+            HostnameIPMapping(node.system_id, 30, {ip.ip}, node.node_type),
+            mappings.get(node.fqdn))
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet)
-        self.assertEqual(
-            mappings, {
-                node.fqdn: HostnameIPMapping(
-                    node.system_id, 30, {ip.ip}, node.node_type)})
+        self.assertEqual({
+            node.fqdn: HostnameIPMapping(
+                node.system_id, 30, {ip.ip}, node.node_type)}, mappings)
 
     def test_dnsrr_glue_correctly_generated(self):
         # Given DNSResource "foo.bar.com" and a "foo.bar.com" domain (in
@@ -1163,18 +1266,16 @@
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(parent)
         self.expectThat(mappings, HasLength(1))
         self.assertEqual(
-            mappings.get(dnsrr.fqdn),
-            HostnameIPMapping(None, 30, {ip.ip}, None))
+            HostnameIPMapping(None, 30, {ip.ip}, None),
+            mappings.get(dnsrr.fqdn))
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(domain)
         self.expectThat(mappings, HasLength(1))
         self.assertEqual(
-            mappings.get(dnsrr.fqdn),
-            HostnameIPMapping(None, 30, {ip.ip}, None))
+            HostnameIPMapping(None, 30, {ip.ip}, None),
+            mappings.get(dnsrr.fqdn))
         mappings = StaticIPAddress.objects.get_hostname_ip_mapping(subnet)
-        self.assertEqual(
-            mappings, {
-                dnsrr.fqdn: HostnameIPMapping(
-                    None, 30, {ip.ip}, None)})
+        self.assertEqual({
+            dnsrr.fqdn: HostnameIPMapping(None, 30, {ip.ip}, None)}, mappings)
 
 
 class TestRenderJSON(MAASServerTestCase):