launchpad-reviewers team mailing list archive

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

Raphaël Badin has proposed merging lp:~rvb/maas/fix-origin-adr into lp:maas.

Commit message:
Fix the detection of the origin ip for a request.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1081701 in MAAS: "The metadata address mentioned in the preseed is wrong."
  https://bugs.launchpad.net/maas/+bug/1081701

For more details, see:
https://code.launchpad.net/~rvb/maas/fix-origin-adr/+merge/136691

Fix the detection of the origin ip for a request: the documentation for request.get_host is misleading, the origin IP is available at request.META['REMOTE_ADDR']

= Notes =

This has been pre-imp'ed with Gavin.
-- 
https://code.launchpad.net/~rvb/maas/fix-origin-adr/+merge/136691
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~rvb/maas/fix-origin-adr into lp:maas.

=== modified file 'src/maasserver/tests/test_api.py'
--- src/maasserver/tests/test_api.py	2012-11-27 16:01:37 +0000
+++ src/maasserver/tests/test_api.py	2012-11-28 15:56:20 +0000
@@ -694,7 +694,7 @@
                     NODE_AFTER_COMMISSIONING_ACTION.DEFAULT,
                 'mac_addresses': [factory.getRandomMACAddress()],
             },
-            HTTP_HOST=origin_ip + ':90')
+            REMOTE_ADDR=origin_ip)
         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'))
@@ -3449,9 +3449,10 @@
         factory.make_node_group(maas_url=ng_url, network=network)
         params = self.get_default_params()
 
-        # Simulate that the request targets ip by setting 'SERVER_NAME'.
+        # Simulate that the request originates from ip by setting
+        # 'REMOTE_ADDR'.
         response = self.client.get(
-            reverse('pxeconfig'), params, SERVER_NAME=ip)
+            reverse('pxeconfig'), params, REMOTE_ADDR=ip)
         self.assertThat(
             json.loads(response.content)["preseed_url"],
             StartsWith(ng_url))
@@ -3467,9 +3468,10 @@
         factory.make_node_group(maas_url=ng_url, network=network)
         params = self.get_default_params()
 
-        # Simulate that the request targets ip by setting 'SERVER_NAME'.
+        # Simulate that the request originates from ip by setting
+        # 'REMOTE_ADDR'.
         response = self.client.get(
-            reverse('pxeconfig'), params, SERVER_NAME=ip)
+            reverse('pxeconfig'), params, REMOTE_ADDR=ip)
         self.assertEqual(
             (ip, hostname),
             (json.loads(response.content)["log_host"], mock.call_args[0][0]))
@@ -3493,9 +3495,10 @@
         node.nodegroup = nodegroup
         node.save()
 
-        # Simulate that the request targets ip by setting 'SERVER_NAME'.
+        # Simulate that the request originates from ip by setting
+        # 'REMOTE_ADDR'.
         response = self.client.get(
-            reverse('pxeconfig'), params, SERVER_NAME=ip)
+            reverse('pxeconfig'), params, REMOTE_ADDR=ip)
         self.assertThat(
             json.loads(response.content)["preseed_url"],
             StartsWith(ng_url))

=== modified file 'src/maasserver/utils/__init__.py'
--- src/maasserver/utils/__init__.py	2012-11-23 10:51:43 +0000
+++ src/maasserver/utils/__init__.py	2012-11-28 15:56:20 +0000
@@ -15,13 +15,11 @@
     'build_absolute_uri',
     'find_nodegroup',
     'get_db_state',
-    'get_origin_ip',
     'ignore_unused',
     'map_enum',
     'strip_domain',
     ]
 
-import socket
 from urllib import urlencode
 from urlparse import urljoin
 
@@ -111,19 +109,6 @@
     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
-
-
 def find_nodegroup(request):
     """Find the nodegroup whose subnet contains the IP Address of the
     originating host of the request..
@@ -133,9 +118,9 @@
     """
     # Circular imports.
     from maasserver.models import NodeGroup
-    ip_address = get_origin_ip(request)
+    ip_address = request.META['REMOTE_ADDR']
     if ip_address is not None:
-        return get_one(NodeGroup.objects.raw("""
+        query = NodeGroup.objects.raw("""
             SELECT *
             FROM maasserver_nodegroup
             WHERE id IN (
@@ -143,5 +128,6 @@
                 FROM maasserver_nodegroupinterface
                 WHERE (inet %s & subnet_mask) = (ip & subnet_mask)
             )
-            """, [ip_address]))
+            """, [ip_address])
+        return get_one(query)
     return None

=== modified file 'src/maasserver/utils/tests/test_utils.py'
--- src/maasserver/utils/tests/test_utils.py	2012-11-23 12:51:00 +0000
+++ src/maasserver/utils/tests/test_utils.py	2012-11-28 15:56:20 +0000
@@ -12,7 +12,6 @@
 __metaclass__ = type
 __all__ = []
 
-import socket
 from urllib import urlencode
 
 from django.conf import settings
@@ -32,15 +31,10 @@
     build_absolute_uri,
     find_nodegroup,
     get_db_state,
-    get_origin_ip,
     map_enum,
     strip_domain,
     )
 from maastesting.testcase import TestCase
-from mock import (
-    call,
-    Mock,
-    )
 from netaddr import IPNetwork
 
 
@@ -197,40 +191,8 @@
         self.assertEqual(results, map(strip_domain, inputs))
 
 
-def get_request(server_name, server_port='80'):
-    return RequestFactory().post(
-        '/', SERVER_NAME=server_name, SERVER_PORT=server_port)
-
-
-class TestGetOriginIP(TestCase):
-
-    def test_get_origin_ip_returns_ip(self):
-        ip = factory.getRandomIPAddress()
-        request = get_request(ip)
-        self.assertEqual(ip, get_origin_ip(request))
-
-    def test_get_origin_ip_strips_port(self):
-        ip = factory.getRandomIPAddress()
-        request = 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 = 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 = 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))
+def get_request(origin_ip):
+    return RequestFactory().post('/', REMOTE_ADDR=origin_ip)
 
 
 class TestFindNodegroup(DjangoTestCase):

=== modified file 'src/metadataserver/tests/test_api.py'
--- src/metadataserver/tests/test_api.py	2012-11-28 15:12:18 +0000
+++ src/metadataserver/tests/test_api.py	2012-11-28 15:56:20 +0000
@@ -657,7 +657,7 @@
             'metadata-enlist-preseed', args=['latest'])
         response = self.client.get(
             anon_enlist_preseed_url, {'op': 'get_enlist_preseed'},
-            SERVER_NAME=ip)
+            REMOTE_ADDR=ip)
         self.assertThat(response.content, Contains(ng_url))
 
     def test_anonymous_get_preseed(self):
@@ -742,7 +742,7 @@
         ip = factory.getRandomIPInNetwork(network)
         factory.make_node_group(maas_url=nodegroup_url, network=network)
         url = reverse('enlist-metadata-user-data', args=['latest'])
-        response = self.client.get(url, SERVER_NAME=ip)
+        response = self.client.get(url, REMOTE_ADDR=ip)
         self.assertThat(
             response.content,
             MatchesAll(Contains(nodegroup_url), Not(Contains(maas_url))))