launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/default-maas-url into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)

For more details, see:
https://code.launchpad.net/~jtv/maas/default-maas-url/+merge/98186

We really need an effective setting for maas_url, or the nodes won't know where to pick up their metadata.  It's a horrible copout to make the user set that in a run-of-the-mill network setup.  As agreed per email: look for the default route, or if none is available, pick the first IP address.

This does not do IPv6 yet.  There simply wasn't time.  It may also miss other cases that I simply didn't think of yet.  But it should obviate a lot of configuration.

When it can find no answers, the guessing logic has a tendency to swallow errors and revert to the hostname.  This is necessary because even setting maas_url explicitly in the config won't stop the django settings from invoking the logic.  It needs to provide something, even when it can't figure things out.  That, more than hope of success, is the real reason why it returns socket.gethostname() as its last-ditch guess.
-- 
https://code.launchpad.net/~jtv/maas/default-maas-url/+merge/98186
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/default-maas-url into lp:maas.

=== modified file 'src/maas/development.py'
--- src/maas/development.py	2012-03-15 22:43:55 +0000
+++ src/maas/development.py	2012-03-19 11:22:29 +0000
@@ -11,13 +11,13 @@
 __metaclass__ = type
 
 import os
-from socket import gethostname
 
 from maas import (
     import_local_settings,
     import_settings,
     settings,
     )
+from metadataserver.address import guess_server_address
 
 # We expect the following settings to be overridden. They are mentioned here
 # to silence lint warnings.
@@ -27,7 +27,7 @@
 import_settings(settings)
 
 # In development, django can be accessed directly on port 5240.
-DEFAULT_MAAS_URL = "http://%s:5240/"; % gethostname()
+DEFAULT_MAAS_URL = "http://%s:5240/"; % guess_server_address()
 
 # Use our custom test runner, which makes sure that a local database
 # cluster is running in the branch.

=== modified file 'src/maas/settings.py'
--- src/maas/settings.py	2012-03-16 14:38:57 +0000
+++ src/maas/settings.py	2012-03-19 11:22:29 +0000
@@ -11,13 +11,13 @@
 __metaclass__ = type
 
 import os
-from socket import gethostname
 from urlparse import urljoin
 
 # Use new style url tag:
 # https://docs.djangoproject.com/en/dev/releases/1.3/#changes-to-url-and-ssi
 import django.template
 from maas import import_local_settings
+from metadataserver.address import guess_server_address
 
 
 django.template.add_to_builtins('django.templatetags.future')
@@ -58,7 +58,7 @@
 # Default URL specifying protocol, host, and (if necessary) port where
 # this MAAS can be found.  Configuration can, and probably should,
 # override this.
-DEFAULT_MAAS_URL = "http://%s/"; % gethostname()
+DEFAULT_MAAS_URL = "http://%s/"; % guess_server_address()
 
 if FORCE_SCRIPT_NAME is not None:
     LOGOUT_URL = FORCE_SCRIPT_NAME + LOGOUT_URL

=== added file 'src/metadataserver/address.py'
--- src/metadataserver/address.py	1970-01-01 00:00:00 +0000
+++ src/metadataserver/address.py	2012-03-19 11:22:29 +0000
@@ -0,0 +1,87 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Figure out server address for the maas_url setting."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = [
+    'guess_server_address',
+    ]
+
+from fcntl import ioctl
+from logging import getLogger
+import re
+import socket
+import struct
+from subprocess import check_output
+
+# fcntl operation as defined in <ioctls.h>.  This is GNU/Linux-specific!
+SIOCGIFADDR = 0x8915
+
+
+def get_command_output(*command_line):
+    """Execute a command line, and return its output.
+
+    Raises an exception if return value is nonzero.
+
+    :param *command_line: Words for the command line.  No shell expansions
+        are performed.
+    :type *command_line: Sequence of basestring.
+    :return: Output from the command.
+    :rtype: List of basestring, one per line.
+    """
+    return check_output(('env', 'LC_ALL=C') + command_line).splitlines()
+
+
+def find_default_interface(ip_route_output):
+    """Find the network interface used for the system's default route.
+
+    If no default is found, makes a guess.
+
+    :param ip_route_output: Output lines from "ip route show" output.
+    :type ip_route_output: Sequence of basestring.
+    :return: basestring, or None.
+    """
+    route_lines = list(ip_route_output)
+    for line in route_lines:
+        match = re.match('default\s+.*\sdev\s+(\w+)', line)
+        if match is not None:
+            return match.groups()[0]
+
+    # Still nothing?  Try the first recognizable interface in the list.
+    for line in route_lines:
+        match = re.match('\s*(?:\S+\s+)*dev\s+(\w+)', line)
+        if match is not None:
+            return match.groups()[0]
+    return None
+
+
+def get_ip_address(interface):
+    """Get the IP address for a given network interface."""
+    # Apparently the netifaces module would do this for us.
+    s = socket.socket(socket.AF_INET, socket.SOCK_DGRAM)
+    interface_name = struct.pack(b'256s', interface[:15])
+    try:
+        info = ioctl(s.fileno(), SIOCGIFADDR, interface_name)
+    except IOError as e:
+        getLogger('maasserver').warn(
+            "Could not determine address for apparent default interface %s "
+            "(%s)"
+            % (interface, e))
+        return None
+    return socket.inet_ntoa(info[20:24])
+
+
+def guess_server_address():
+    """Make a guess as to this server's IP address."""
+    ip_route_output = get_command_output('/bin/ip', 'route', 'show')
+    interface = find_default_interface(ip_route_output)
+    if interface is None:
+        return socket.gethostname()
+    else:
+        return get_ip_address(interface)

=== added file 'src/metadataserver/tests/test_address.py'
--- src/metadataserver/tests/test_address.py	1970-01-01 00:00:00 +0000
+++ src/metadataserver/tests/test_address.py	2012-03-19 11:22:29 +0000
@@ -0,0 +1,87 @@
+# Copyright 2012 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test server-address-guessing logic."""
+
+from __future__ import (
+    print_function,
+    unicode_literals,
+    )
+
+__metaclass__ = type
+__all__ = []
+
+from socket import gethostname
+
+from maastesting.testcase import TestCase
+from metadataserver import address
+from testtools.matchers import MatchesRegex
+
+
+def parse_locale_lines(output):
+    """Parse lines of output from /bin/locale into a dict."""
+    return {
+        key: value.strip('"')
+        for key, value in [line.split('=') for line in output]}
+
+
+class TestAddress(TestCase):
+
+    def test_get_command_output_executes_command(self):
+        self.assertEqual(
+            ["Hello"], address.get_command_output('echo', 'Hello'))
+
+    def test_get_command_output_does_not_expand_arguments(self):
+        self.assertEqual(["$*"], address.get_command_output('echo', '$*'))
+
+    def test_get_command_output_returns_sequence_of_lines(self):
+        self.assertEqual(
+            ['1', '2'], address.get_command_output('echo', '1\n2'))
+
+    def test_get_command_output_uses_C_locale(self):
+        locale = parse_locale_lines(address.get_command_output('locale'))
+        self.assertEqual('C', locale['LC_CTYPE'])
+        self.assertEqual('en_US.UTF-8', locale['LANG'])
+
+    def test_find_default_interface_finds_default_interface(self):
+        sample_ip_route = [
+            "default via 10.0.0.1 dev eth1  proto static",
+            "169.254.0.0/16 dev eth2  scope link  metric 1000",
+            "10.0.0.0/24 dev eth0  proto kernel  scope link  src 10.0.0.11  "
+                "metric 2",
+            "10.1.0.0/24 dev virbr0  proto kernel  scope link  src 10.1.0.1",
+            "10.1.1.0/24 dev virbr1  proto kernel  scope link  src 10.1.1.1",
+            ]
+        self.assertEqual(
+            'eth1', address.find_default_interface(sample_ip_route))
+
+    def test_find_default_interface_makes_a_guess_if_no_default(self):
+        sample_ip_route = [
+            "10.0.0.0/24 dev eth2  proto kernel  scope link  src 10.0.0.11  "
+                "metric 2",
+            "10.1.0.0/24 dev virbr0  proto kernel  scope link  src 10.1.0.1",
+            "10.1.1.0/24 dev virbr1  proto kernel  scope link  src 10.1.1.1",
+            ]
+        self.assertEqual(
+            'eth2', address.find_default_interface(sample_ip_route))
+
+    def test_find_default_interface_returns_None_on_failure(self):
+        self.assertIsNone(address.find_default_interface([]))
+
+    def test_get_ip_address_finds_IP_address_of_interface(self):
+        self.assertEqual('127.0.0.1', address.get_ip_address(b'lo'))
+
+    def test_get_ip_address_returns_None_on_failure(self):
+        self.assertIsNone(address.get_ip_address(b'ethturboveyronsuper9'))
+
+    def test_guess_server_address_finds_IP_address(self):
+        self.assertThat(
+            address.guess_server_address(),
+            MatchesRegex("^[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+$"))
+
+    def test_guess_server_address_returns_hostname_as_last_ditch_guess(self):
+        def return_empty_list(*args):
+            return []
+
+        self.patch(address, 'get_command_output', return_empty_list)
+        self.assertEqual(gethostname(), address.guess_server_address())