launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/enlist-nodegroup-bug-1081695 into lp:maas.

Commit message:
Auto-detect the nodegroup to which a node should be attached to based on the information on the originating available from the request.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1081695 in MAAS: "New nodes always get enlisted in the default nodegroup."
  https://bugs.launchpad.net/maas/+bug/1081695

For more details, see:
https://code.launchpad.net/~rvb/maas/enlist-nodegroup-bug-1081695/+merge/135690

Turns out the logic to actually fetch to right nodegroup from an IP address was already there… hooking it up in the API code was the only missing part.

= Pre-imp =

This was discussed with the whole team during this morning's stand up call.
-- 
https://code.launchpad.net/~rvb/maas/enlist-nodegroup-bug-1081695/+merge/135690
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/enlist-nodegroup-bug-1081695 into lp:maas.

=== modified file 'src/maasserver/api.py'
--- src/maasserver/api.py	2012-11-21 17:55:05 +0000
+++ src/maasserver/api.py	2012-11-22 13:48:21 +0000
@@ -166,6 +166,7 @@
 from maasserver.utils import (
     absolute_reverse,
     build_absolute_uri,
+    get_origin_ip,
     map_enum,
     strip_domain,
     )
@@ -645,6 +646,14 @@
             # assume 'generic'.
             altered_query_data['architecture'] += '/generic'
 
+    if 'nodegroup' not in altered_query_data:
+        # If 'nodegroup' is not explicitely specified, get the origin of the
+        # request to figure out which nodegroup the new node should be
+        # attached to.
+        origin_ip = get_origin_ip(request)
+        if origin_ip is not None:
+            altered_query_data['nodegroup'] = origin_ip
+
     Form = get_node_create_form(request.user)
     form = Form(altered_query_data)
     if form.is_valid():

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-21 10:24:27 +0000
+++ src/maasserver/tests/test_api.py	2012-11-22 13:48:21 +0000
@@ -126,6 +126,7 @@
     ANY,
     Mock,
     )
+from netaddr import IPNetwork
 from provisioningserver import (
     boot_images,
     kernel_opts,
@@ -740,6 +741,45 @@
         self.assertEqual(
             expected_hostname, parsed_result.get('hostname'))
 
+    def test_created_node_nodegroup_is_inferred_from_origin_network(self):
+        network = IPNetwork('192.168.0.3/24')
+        origin_ip = factory.getRandomIPInNetwork(network)
+        NodeGroup.objects.ensure_master()
+        nodegroup = factory.make_node_group(network=network)
+        response = self.client.post(
+            self.get_uri('nodes/'),
+            data={
+                'op': 'new',
+                'hostname': factory.make_name('hostname'),
+                'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+                'after_commissioning_action':
+                    NODE_AFTER_COMMISSIONING_ACTION.DEFAULT,
+                'mac_addresses': [factory.getRandomMACAddress()],
+            },
+            HTTP_HOST=origin_ip + ':90')
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        parsed_result = json.loads(response.content)
+        node = Node.objects.get(system_id=parsed_result.get('system_id'))
+        self.assertEqual(nodegroup, node.nodegroup)
+
+    def test_created_node_uses_default_nodegroup_if_origin_not_found(self):
+        unknown_host = factory.make_name('host')
+        response = self.client.post(
+            self.get_uri('nodes/'),
+            data={
+                'op': 'new',
+                'hostname': factory.make_name('hostname'),
+                'architecture': factory.getRandomChoice(ARCHITECTURE_CHOICES),
+                'after_commissioning_action':
+                    NODE_AFTER_COMMISSIONING_ACTION.DEFAULT,
+                'mac_addresses': [factory.getRandomMACAddress()],
+            },
+            HTTP_HOST=unknown_host)
+        self.assertEqual(httplib.OK, response.status_code, response.content)
+        parsed_result = json.loads(response.content)
+        node = Node.objects.get(system_id=parsed_result.get('system_id'))
+        self.assertEqual(NodeGroup.objects.ensure_master(), node.nodegroup)
+
 
 class NonAdminEnlistmentAPITest(APIv10TestMixin, MultipleUsersScenarios,
                                 TestCase):

=== modified file 'src/maasserver/utils/__init__.py'
--- src/maasserver/utils/__init__.py	2012-11-08 10:11:03 +0000
+++ src/maasserver/utils/__init__.py	2012-11-22 13:48:21 +0000
@@ -14,11 +14,13 @@
     'absolute_reverse',
     'build_absolute_uri',
     'get_db_state',
+    'get_origin_ip',
     'ignore_unused',
     'map_enum',
     'strip_domain',
     ]
 
+import socket
 from urllib import urlencode
 from urlparse import urljoin
 
@@ -103,3 +105,16 @@
 def strip_domain(hostname):
     """Return `hostname` with the domain part removed."""
     return hostname.split('.', 1)[0]
+
+
+def get_origin_ip(request):
+    """Return the IP address of the originating host of the request.
+
+    Return the IP address obtained by resolving the host given by
+    request.get_host().
+    """
+    host = request.get_host().split(':')[0]
+    try:
+        return socket.gethostbyname(host)
+    except socket.error:
+        return None

=== modified file 'src/maasserver/utils/tests/test_utils.py'
--- src/maasserver/utils/tests/test_utils.py	2012-11-08 10:11:03 +0000
+++ src/maasserver/utils/tests/test_utils.py	2012-11-22 13:48:21 +0000
@@ -12,11 +12,13 @@
 __metaclass__ = type
 __all__ = []
 
+import socket
 from urllib import urlencode
 
 from django.conf import settings
 from django.core.urlresolvers import reverse
 from django.http import HttpRequest
+from django.test.client import RequestFactory
 from maasserver.enum import NODE_STATUS_CHOICES
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase as DjangoTestCase
@@ -24,10 +26,15 @@
     absolute_reverse,
     build_absolute_uri,
     get_db_state,
+    get_origin_ip,
     map_enum,
     strip_domain,
     )
 from maastesting.testcase import TestCase
+from mock import (
+    call,
+    Mock,
+    )
 
 
 class TestEnum(TestCase):
@@ -175,3 +182,38 @@
         inputs = [input for input, _ in input_and_results]
         results = [result for _, result in input_and_results]
         self.assertEqual(results, map(strip_domain, inputs))
+
+
+class TestGetOriginIP(TestCase):
+
+    def get_request(self, server_name, server_port='80'):
+        return RequestFactory().post(
+            '/', SERVER_NAME=server_name, SERVER_PORT=server_port)
+
+    def test_get_origin_ip_returns_ip(self):
+        ip = factory.getRandomIPAddress()
+        request = self.get_request(ip)
+        self.assertEqual(ip, get_origin_ip(request))
+
+    def test_get_origin_ip_strips_port(self):
+        ip = factory.getRandomIPAddress()
+        request = self.get_request(ip, '8888')
+        self.assertEqual(ip, get_origin_ip(request))
+
+    def test_get_origin_ip_resolves_hostname(self):
+        ip = factory.getRandomIPAddress()
+        hostname = factory.make_name('hostname')
+        request = self.get_request(hostname)
+        resolver = self.patch(socket, 'gethostbyname', Mock(return_value=ip))
+        self.assertEqual(
+            (ip, call(hostname)),
+            (get_origin_ip(request), resolver.call_args))
+
+    def test_get_origin_ip_returns_None_if_hostname_cannot_get_resolved(self):
+        hostname = factory.make_name('hostname')
+        request = self.get_request(hostname)
+        resolver = self.patch(
+            socket, 'gethostbyname', Mock(side_effect=socket.error))
+        self.assertEqual(
+            (None, call(hostname)),
+            (get_origin_ip(request), resolver.call_args))