launchpad-reviewers team mailing list archive

[Raphaël Badin <raphael.badin@xxxxxxxxxxxxx>]

Raphaël Badin has proposed merging lp:~rvb/maas/add-ns-server into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~rvb/maas/add-ns-server/+merge/116824

This branch adds a new method to retrieve the MAAS' DNS server IP.  That information will be used in the zone files.  We derive that information from settings.DEFAULT_MAAS_URL: we already have the code in place to guess the MAAS server's IP address and we still give the user the possibility to tweak that parameter if needed.

Note that I didn't test the log content.  Not sure this is useful as long as every code path is tested. 

= Pre-imp =

Talked about this with Jeroen.
-- 
https://code.launchpad.net/~rvb/maas/add-ns-server/+merge/116824
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/add-ns-server into lp:maas.

=== modified file 'src/maasserver/dns.py'
--- src/maasserver/dns.py	2012-07-25 16:15:24 +0000
+++ src/maasserver/dns.py	2012-07-26 10:31:22 +0000
@@ -19,6 +19,9 @@
 
 
 import collections
+import logging
+import socket
+from urlparse import urlparse
 
 from django.conf import settings
 from django.db.models.signals import (
@@ -26,6 +29,7 @@
     post_save,
     )
 from django.dispatch import receiver
+from maasserver.exceptions import MAASException
 from maasserver.models import (
     DHCPLease,
     Node,
@@ -36,6 +40,11 @@
     INT_MAX,
     Sequence,
     )
+from netaddr import (
+    AddrFormatError,
+    IPAddress,
+    IPNetwork,
+    )
 from provisioningserver import tasks
 from provisioningserver.dns.config import DNSZoneConfig
 
@@ -54,6 +63,54 @@
     return settings.DNS_CONNECT
 
 
+class DNSException(MAASException):
+    """An error occured when setting up MAAS' DNS server."""
+
+
+def _warn_loopback(ip):
+    """Warn if the given IP address is in the loopback network."""
+    if IPAddress(ip) in IPNetwork('127.0.0.1/8'):
+        logging.getLogger('maas').warn(
+            "The DNS server address used will be '%s'.  That address is "
+            "in the loopback network.  This might not be a problem "
+            "if you're not using MAAS' DNS features or if "
+            "you don't rely on this information.  Be sure to set the "
+            "setting DEFAULT_MAAS_URL." % ip)
+
+
+def get_dns_server_address():
+    """Return the DNS server's IP address.
+
+    That address is derived from DEFAULT_MAAS_URL in order to get a sensible
+    default and at the same time give a possibility to the user to change this.
+    """
+    hostname_or_ip = urlparse(settings.DEFAULT_MAAS_URL).netloc.split(':')[0]
+
+    # If 'hostname_or_ip' is a valid IP address, return it.
+    try:
+        IPAddress(hostname_or_ip)
+        _warn_loopback(hostname_or_ip)
+        return hostname_or_ip
+    except AddrFormatError:
+        pass
+
+    # Try to resolve the hostname.
+    try:
+        ip = socket.gethostbyname(hostname_or_ip)
+        _warn_loopback(ip)
+        return ip
+    except socket.error:
+        pass
+
+    # No suitable address has been found.
+    raise DNSException(
+        "Unable to find a suitable IP for the MAAS server.  Such an IP "
+        "is required for MAAS' DNS features to work.  Make sure that the "
+        "setting DEFAULT_MAAS_URL is defined properly.  The IP in "
+        "DEFAULT_MAAS_URL is the one which will be used for the NS record "
+        "in MAAS' zone files.")
+
+
 @receiver(post_save, sender=NodeGroup)
 def dns_post_save_NodeGroup(sender, instance, created, **kwargs):
     """Create or update DNS zones related to the new nodegroup."""

=== modified file 'src/maasserver/tests/test_dns.py'
--- src/maasserver/tests/test_dns.py	2012-07-25 15:25:42 +0000
+++ src/maasserver/tests/test_dns.py	2012-07-26 10:31:22 +0000
@@ -14,12 +14,15 @@
 
 
 from itertools import islice
+import socket
 
 from django.conf import settings
 from django.core.management import call_command
 from maasserver.dns import (
     add_zone,
     change_dns_zones,
+    DNSException,
+    get_dns_server_address,
     is_dns_enabled,
     next_zone_serial,
     write_full_dns_config,
@@ -33,6 +36,7 @@
 from maasserver.testing.testcase import TestCase
 from maastesting.bindfixture import BINDServer
 from maastesting.celery import CeleryFixture
+from maastesting.fakemethod import FakeMethod
 from maastesting.tests.test_bindfixture import dig_call
 from netaddr import (
     IPNetwork,
@@ -62,6 +66,53 @@
             ['%0.10d' % i for i in range(initial + 1, initial + 11)],
             [next_zone_serial() for i in range(initial, initial + 10)])
 
+    def test_get_dns_server_address_returns_IP(self):
+        ip = factory.getRandomIPAddress()
+        url = 'http://%s:%d/%s' % (
+            ip, factory.getRandomPort(), factory.getRandomString())
+        self.patch(settings, 'DEFAULT_MAAS_URL', url)
+        self.assertEqual(ip, get_dns_server_address())
+
+    def test_get_dns_server_address_returns_IP_if_IP_is_localhost(self):
+        ip = factory.getRandomIPInNetwork(IPNetwork('127.0.0.1/8'))
+        url = 'http://%s:%d/%s' % (
+            ip, factory.getRandomPort(), factory.getRandomString())
+        self.patch(settings, 'DEFAULT_MAAS_URL', url)
+        self.assertEqual(ip, get_dns_server_address())
+
+    def test_get_dns_server_address_resolves_hostname(self):
+        ip = factory.getRandomIPAddress()
+        resolver = FakeMethod(result=ip)
+        self.patch(socket, 'gethostbyname', resolver)
+        hostname = factory.getRandomString()
+        url = 'http://%s:%d/%s' % (
+            hostname, factory.getRandomPort(), factory.getRandomString())
+        self.patch(settings, 'DEFAULT_MAAS_URL', url)
+        self.assertEqual(
+            (ip, [(hostname, )]),
+            (get_dns_server_address(), resolver.extract_args()))
+
+    def test_get_dns_server_address_resolves_hostname_localhost(self):
+        ip = factory.getRandomIPInNetwork(IPNetwork('127.0.0.1/8'))
+        resolver = FakeMethod(result=ip)
+        self.patch(socket, 'gethostbyname', resolver)
+        hostname = factory.getRandomString()
+        url = 'http://%s:%d/%s' % (
+            hostname, factory.getRandomPort(), factory.getRandomString())
+        self.patch(settings, 'DEFAULT_MAAS_URL', url)
+        self.assertEqual(
+            (ip, [(hostname, )]),
+            (get_dns_server_address(), resolver.extract_args()))
+
+    def test_get_dns_server_address_raises_if_hostname_doesnt_resolve(self):
+        resolver = FakeMethod(failure=socket.error)
+        self.patch(socket, 'gethostbyname', resolver)
+        url = 'http://%s:%d/%s' % (
+            factory.getRandomString(), factory.getRandomPort(),
+            factory.getRandomString())
+        self.patch(settings, 'DEFAULT_MAAS_URL', url)
+        self.assertRaises(DNSException, get_dns_server_address)
+
 
 class TestDNSConfigModifications(TestCase):