Merge lp:~allenap/maas/fix-1384464 into lp:~maas-committers/maas/trunk

[Gavin Panella]

Use a configured HTTP proxy to import boot loaders to the cluster, but do not use a proxy when importing boot images from the region.

Previously the cluster used the proxy to download boot images. However, boot images must now be retrieved from the region and not the Internet, whereas boot loaders still must be retrieved via the Internet.

[Gavin Panella (allenap)]

Many many thanks to Narinder Gupta for exhaustively testing this.

[Andres Rodriguez (andreserl)]

So tests were successful? Have we ensured that there are no regressions?

[Andres Rodriguez (andreserl)]

No regressions when not having a proxy?

[Gavin Panella (allenap)]

Andres, I'll give it a go locally.

[Graham Binns (gmb)]

My usual critique: Commit message lacks a wherefore (mentioning the bug would be great). Otherwise, this looks good.

review: Approve

[Gavin Panella (allenap)]

FTR, most of the work in this branch was done by Blake, and first appeared in https://code.launchpad.net/~blake-rouse/maas/fix-1384464/+merge/239440.

[Gavin Panella (allenap)]

Andres, I've just imported Trusty i386 to my NUCs at home without proxy and it was fine.

[Christian Reis (kiko)]

I have a question about the first proxy piece, where it seems we're only using http_proxy, not https_proxy.

review: Needs Fixing

[Christian Reis (kiko)]

I think I understand now; filed bug 1387381. Can you land this for 1.7 as well?

review: Approve

=== modified file 'src/maasserver/api/tests/test_nodegroup.py'
--- src/maasserver/api/tests/test_nodegroup.py	2014-10-10 15:00:47 +0000
+++ src/maasserver/api/tests/test_nodegroup.py	2014-10-29 20:22:20 +0000
@@ -17,6 +17,7 @@
 import httplib
 import json
 from textwrap import dedent
+from urlparse import urlparse
 
 import bson
 from django.contrib.auth.models import AnonymousUser
@@ -27,6 +28,7 @@
     NODEGROUP_STATUS_CHOICES,
     )
 from maasserver.models import (
+    Config,
     DownloadProgress,
     NodeGroup,
     nodegroup as nodegroup_module,
@@ -472,6 +474,8 @@
         self.assertItemsEqual([node.system_id], parsed_result)
 
     def test_nodegroup_import_boot_images_calls_ImportBootImages(self):
+        proxy = 'http://%s.example.com' % factory.make_name('proxy')
+        Config.objects.set_config('http_proxy', proxy)
         sources = [get_simplestream_endpoint()]
         fake_client = Mock()
         mock_getClientFor = self.patch(nodegroup_module, 'getClientFor')
@@ -488,7 +492,9 @@
             explain_unexpected_response(httplib.OK, response))
         self.assertThat(
             fake_client,
-            MockCalledOnceWith(ImportBootImages, sources=sources))
+            MockCalledOnceWith(
+                ImportBootImages, sources=sources,
+                http_proxy=urlparse(proxy), https_proxy=urlparse(proxy)))
 
     def test_nodegroup_import_boot_images_denied_if_not_admin(self):
         nodegroup = factory.make_NodeGroup()

=== modified file 'src/maasserver/models/nodegroup.py'
--- src/maasserver/models/nodegroup.py	2014-10-10 15:00:47 +0000
+++ src/maasserver/models/nodegroup.py	2014-10-29 20:22:20 +0000
@@ -17,6 +17,7 @@
     'NODEGROUP_CLUSTER_NAME_TEMPLATE',
     ]
 
+from urlparse import urlparse
 
 from apiclient.creds import convert_tuple_to_string
 from crochet import TimeoutError
@@ -295,6 +296,7 @@
         the images that are exposed there.
         """
         # Avoid circular imports
+        from maasserver.models import Config
         from maasserver.bootresources import get_simplestream_endpoint
         try:
             client = getClientFor(self.uuid, timeout=1)
@@ -303,7 +305,12 @@
             # the cluster is down, it will do an import on start up.
             return
         sources = [get_simplestream_endpoint()]
-        return client(ImportBootImages, sources=sources)
+        http_proxy = Config.objects.get_config("http_proxy")
+        if http_proxy is not None:
+            http_proxy = urlparse(http_proxy)
+        return client(
+            ImportBootImages, sources=sources,
+            http_proxy=http_proxy, https_proxy=http_proxy)
 
     def add_seamicro15k(self, mac, username, password, power_control=None):
         """ Add all of the specified cards the Seamicro SM15000 chassis at the

=== modified file 'src/maasserver/models/tests/test_nodegroup.py'
--- src/maasserver/models/tests/test_nodegroup.py	2014-09-30 18:13:21 +0000
+++ src/maasserver/models/tests/test_nodegroup.py	2014-10-29 20:22:20 +0000
@@ -14,6 +14,7 @@
 __metaclass__ = type
 __all__ = []
 
+from urlparse import urlparse
 
 from apiclient.creds import convert_string_to_tuple
 from django.db.models.signals import post_save
@@ -26,6 +27,7 @@
     NODEGROUPINTERFACE_MANAGEMENT,
     )
 from maasserver.models import (
+    Config,
     NodeGroup,
     nodegroup as nodegroup_module,
     )
@@ -381,6 +383,8 @@
             mock_getClientFor, MockCalledOnceWith(nodegroup.uuid, timeout=1))
 
     def test_import_boot_images_calls_client_with_resource_endpoint(self):
+        proxy = 'http://%s.example.com' % factory.make_name('proxy')
+        Config.objects.set_config('http_proxy', proxy)
         sources = [get_simplestream_endpoint()]
         fake_client = Mock()
         mock_getClientFor = self.patch(nodegroup_module, 'getClientFor')
@@ -389,7 +393,9 @@
         nodegroup.import_boot_images()
         self.assertThat(
             fake_client,
-            MockCalledOnceWith(ImportBootImages, sources=sources))
+            MockCalledOnceWith(
+                ImportBootImages, sources=sources,
+                http_proxy=urlparse(proxy), https_proxy=urlparse(proxy)))
 
     def test_import_boot_images_does_nothing_if_no_connection_to_cluster(self):
         mock_getClientFor = self.patch(nodegroup_module, 'getClientFor')
@@ -401,6 +407,8 @@
         self.assertThat(mock_get_simplestreams_endpoint, MockNotCalled())
 
     def test_import_boot_images_end_to_end(self):
+        proxy = 'http://%s.example.com' % factory.make_name('proxy')
+        Config.objects.set_config('http_proxy', proxy)
         nodegroup = factory.make_NodeGroup(status=NODEGROUP_STATUS.ACCEPTED)
 
         self.useFixture(RegionEventLoopFixture("rpc"))
@@ -413,7 +421,9 @@
 
         self.assertThat(
             protocol.ImportBootImages,
-            MockCalledOnceWith(ANY, sources=[get_simplestream_endpoint()]))
+            MockCalledOnceWith(
+                ANY, sources=[get_simplestream_endpoint()],
+                http_proxy=urlparse(proxy), https_proxy=urlparse(proxy)))
 
     def test_is_connected_returns_true_if_connection(self):
         mock_getClientFor = self.patch(nodegroup_module, 'getClientFor')

=== modified file 'src/provisioningserver/boot/utils.py'
--- src/provisioningserver/boot/utils.py	2014-08-13 21:49:35 +0000
+++ src/provisioningserver/boot/utils.py	2014-10-29 20:22:20 +0000
@@ -46,7 +46,14 @@
     :param url: URL to download file
     :returns: File data, or None
     """
-    response = urllib2.urlopen(url)
+    from urllib import getproxies
+    from twisted.python import log
+    log.msg("Proxies: %s" % getproxies())
+
+    # Build a new opener so that the environment is checked for proxy
+    # URLs. Using urllib2.urlopen() means that we'd only be using the
+    # proxies as defined when urlopen() was called the first time.
+    response = urllib2.build_opener().open(url)
     return response.read()
 
 

=== modified file 'src/provisioningserver/pserv_services/image_download_service.py'
--- src/provisioningserver/pserv_services/image_download_service.py	2014-10-02 11:53:59 +0000
+++ src/provisioningserver/pserv_services/image_download_service.py	2014-10-29 20:22:20 +0000
@@ -26,6 +26,7 @@
 from provisioningserver.rpc.region import (
     GetBootSources,
     GetBootSourcesV2,
+    GetProxies,
     )
 from provisioningserver.utils.twisted import (
     pause,
@@ -108,7 +109,16 @@
 
         # Get sources from region
         sources = yield self._get_boot_sources(client)
-        yield import_boot_images(sources.get("sources"))
+        # Get http proxy from region
+        proxies = yield client(GetProxies)
+
+        def get_proxy_url(scheme):
+            url = proxies.get(scheme)  # url is a ParsedResult.
+            return None if url is None else url.geturl()
+
+        yield import_boot_images(
+            sources.get("sources"), get_proxy_url("http"),
+            get_proxy_url("https"))
 
     @inlineCallbacks
     def maybe_start_download(self):

=== modified file 'src/provisioningserver/pserv_services/tests/test_image_download_service.py'
--- src/provisioningserver/pserv_services/tests/test_image_download_service.py	2014-10-02 11:53:59 +0000
+++ src/provisioningserver/pserv_services/tests/test_image_download_service.py	2014-10-29 20:22:20 +0000
@@ -14,8 +14,8 @@
 __metaclass__ = type
 __all__ = []
 
-
 from datetime import timedelta
+from urlparse import urlparse
 
 from fixtures import FakeLogger
 from maastesting.factory import factory
@@ -139,10 +139,15 @@
             tftppath, 'maas_meta_last_modified')
         one_week = timedelta(minutes=15).total_seconds()
         maas_meta_last_modified.return_value = clock.seconds() - one_week
+        http_proxy = factory.make_simple_http_url()
+        https_proxy = factory.make_simple_http_url()
         rpc_client = Mock()
         client_call = Mock()
         client_call.side_effect = [
             defer.succeed(dict(sources=sentinel.sources)),
+            defer.succeed(dict(
+                http=urlparse(http_proxy),
+                https=urlparse(https_proxy))),
             ]
         rpc_client.getClient.return_value = client_call
 
@@ -158,7 +163,8 @@
         service.startService()
         self.assertThat(
             deferToThread, MockCalledOnceWith(
-                _run_import, sentinel.sources))
+                _run_import, sentinel.sources, http_proxy=http_proxy,
+                https_proxy=https_proxy))
 
     def test_no_download_if_no_rpc_connections(self):
         rpc_client = Mock()

=== modified file 'src/provisioningserver/rpc/boot_images.py'
--- src/provisioningserver/rpc/boot_images.py	2014-09-30 10:05:55 +0000
+++ src/provisioningserver/rpc/boot_images.py	2014-10-29 20:22:20 +0000
@@ -18,6 +18,7 @@
     "is_import_boot_images_running",
     ]
 
+from urlparse import urlparse
 
 from provisioningserver import concurrency
 from provisioningserver.auth import get_maas_user_gpghome
@@ -35,8 +36,18 @@
         Config.load_from_cache()['tftp']['resource_root'])
 
 
+def get_hosts_from_sources(sources):
+    """Return set of hosts that are contained in the given sources."""
+    hosts = set()
+    for source in sources:
+        url = urlparse(source['url'])
+        if url.hostname is not None:
+            hosts.add(url.hostname)
+    return hosts
+
+
 @synchronous
-def _run_import(sources):
+def _run_import(sources, http_proxy=None, https_proxy=None):
     """Run the import.
 
     This is function is synchronous so it must be called with deferToThread.
@@ -44,15 +55,25 @@
     variables = {
         'GNUPGHOME': get_maas_user_gpghome(),
         }
+    if http_proxy is not None:
+        variables['http_proxy'] = http_proxy
+    if https_proxy is not None:
+        variables['https_proxy'] = https_proxy
+    # Communication to the sources and loopback should not go through proxy.
+    no_proxy_hosts = ["localhost", "127.0.0.1", "::1"]
+    no_proxy_hosts += list(get_hosts_from_sources(sources))
+    variables['no_proxy'] = ','.join(no_proxy_hosts)
     with environment_variables(variables):
         boot_resources.import_images(sources)
 
 
-def import_boot_images(sources):
+def import_boot_images(sources, http_proxy=None, https_proxy=None):
     """Imports the boot images from the given sources."""
     lock = concurrency.boot_images
     if not lock.locked:
-        return lock.run(deferToThread, _run_import, sources)
+        return lock.run(
+            deferToThread, _run_import, sources,
+            http_proxy=http_proxy, https_proxy=https_proxy)
 
 
 def is_import_boot_images_running():

=== modified file 'src/provisioningserver/rpc/cluster.py'
--- src/provisioningserver/rpc/cluster.py	2014-10-02 16:21:43 +0000
+++ src/provisioningserver/rpc/cluster.py	2014-10-29 20:22:20 +0000
@@ -353,6 +353,8 @@
                   (b"arches", amp.ListOf(amp.Unicode())),
                   (b"subarches", amp.ListOf(amp.Unicode())),
                   (b"labels", amp.ListOf(amp.Unicode()))]))])),
+        (b"http_proxy", ParsedURL(optional=True)),
+        (b"https_proxy", ParsedURL(optional=True)),
     ]
     response = []
     errors = []

=== modified file 'src/provisioningserver/rpc/clusterservice.py'
--- src/provisioningserver/rpc/clusterservice.py	2014-10-23 21:11:15 +0000
+++ src/provisioningserver/rpc/clusterservice.py	2014-10-29 20:22:20 +0000
@@ -138,13 +138,16 @@
         return {"images": list_boot_images()}
 
     @cluster.ImportBootImages.responder
-    def import_boot_images(self, sources):
+    def import_boot_images(self, sources, http_proxy=None, https_proxy=None):
         """import_boot_images()
 
         Implementation of
         :py:class:`~provisioningserver.rpc.cluster.ImportBootImages`.
         """
-        import_boot_images(sources)
+        get_proxy_url = lambda url: None if url is None else url.geturl()
+        import_boot_images(
+            sources, http_proxy=get_proxy_url(http_proxy),
+            https_proxy=get_proxy_url(https_proxy))
         return {}
 
     @cluster.IsImportBootImagesRunning.responder

=== modified file 'src/provisioningserver/rpc/tests/test_boot_images.py'
--- src/provisioningserver/rpc/tests/test_boot_images.py	2014-09-30 10:05:55 +0000
+++ src/provisioningserver/rpc/tests/test_boot_images.py	2014-10-29 20:22:20 +0000
@@ -15,6 +15,7 @@
 __all__ = []
 
 import os
+from random import randint
 
 from maastesting.factory import factory
 from maastesting.matchers import (
@@ -30,6 +31,7 @@
 from provisioningserver.rpc import boot_images
 from provisioningserver.rpc.boot_images import (
     _run_import,
+    get_hosts_from_sources,
     import_boot_images,
     is_import_boot_images_running,
     list_boot_images,
@@ -37,10 +39,25 @@
 from provisioningserver.testing.config import BootSourcesFixture
 from provisioningserver.testing.testcase import PservTestCase
 from provisioningserver.utils.twisted import pause
+from testtools.matchers import Equals
 from twisted.internet import defer
 from twisted.internet.task import Clock
 
 
+def make_sources():
+    hosts = [factory.make_name('host').lower() for _ in range(3)]
+    urls = [
+        'http://%s:%s/images-stream/streams/v1/index.json' % (
+            host, randint(1, 1000))
+        for host in hosts
+        ]
+    sources = [
+        {'url': url, 'selections': []}
+        for url in urls
+        ]
+    return sources, hosts
+
+
 class TestListBootImages(PservTestCase):
 
     def test__calls_list_boot_images_with_resource_root(self):
@@ -56,6 +73,13 @@
             MockCalledOnceWith(sentinel.resource_root))
 
 
+class TestGetHostsFromSources(PservTestCase):
+
+    def test__returns_set_of_hosts_from_sources(self):
+        sources, hosts = make_sources()
+        self.assertItemsEqual(hosts, get_hosts_from_sources(sources))
+
+
 class TestRunImport(PservTestCase):
 
     def make_archive_url(self, name=None):
@@ -95,22 +119,29 @@
         _run_import(sources=[])
         self.assertEqual(home, fake.env['GNUPGHOME'])
 
+    def test__run_import_sets_proxy_if_given(self):
+        proxy = 'http://%s.example.com' % factory.make_name('proxy')
+        fake = self.patch_boot_resources_function()
+        _run_import(sources=[], http_proxy=proxy, https_proxy=proxy)
+        self.expectThat(fake.env['http_proxy'], Equals(proxy))
+        self.expectThat(fake.env['https_proxy'], Equals(proxy))
+
+    def test__run_import_sets_proxy_for_loopback(self):
+        fake = self.patch_boot_resources_function()
+        _run_import(sources=[])
+        self.assertEqual(fake.env['no_proxy'], "localhost,127.0.0.1,::1")
+
+    def test__run_import_sets_proxy_for_source_host(self):
+        sources, hosts = make_sources()
+        fake = self.patch_boot_resources_function()
+        _run_import(sources=sources)
+        self.assertItemsEqual(
+            fake.env['no_proxy'].split(','),
+            ["localhost", "127.0.0.1", "::1"] + hosts)
+
     def test__run_import_accepts_sources_parameter(self):
         fake = self.patch(boot_resources, 'import_images')
-        sources = [
-            {
-                'path': "http://example.com",
-                'selections': [
-                    {
-                        'os': "ubuntu",
-                        'release': "trusty",
-                        'arches': ["amd64"],
-                        'subarches': ["generic"],
-                        'labels': ["release"]
-                    },
-                ],
-            },
-        ]
+        sources, _ = make_sources()
         _run_import(sources=sources)
         self.assertThat(fake, MockCalledOnceWith(sources))
 
@@ -136,7 +167,8 @@
         yield import_boot_images(sentinel.sources)
         self.assertThat(
             deferToThread, MockCalledOnceWith(
-                _run_import, sentinel.sources))
+                _run_import, sentinel.sources,
+                http_proxy=None, https_proxy=None))
 
     def test__takes_lock_when_running(self):
         clock = Clock()

=== modified file 'src/provisioningserver/rpc/tests/test_clusterservice.py'
--- src/provisioningserver/rpc/tests/test_clusterservice.py	2014-10-14 16:30:50 +0000
+++ src/provisioningserver/rpc/tests/test_clusterservice.py	2014-10-29 20:22:20 +0000
@@ -323,7 +323,26 @@
 
         self.assertThat(
             import_boot_images,
-            MockCalledOnceWith(sources))
+            MockCalledOnceWith(sources, http_proxy=None, https_proxy=None))
+
+    @inlineCallbacks
+    def test_import_boot_images_calls_import_boot_images_with_proxies(self):
+        import_boot_images = self.patch(clusterservice, "import_boot_images")
+
+        proxy = 'http://%s.example.com' % factory.make_name('proxy')
+        parsed_proxy = urlparse(proxy)
+
+        yield call_responder(
+            Cluster(), cluster.ImportBootImages, {
+                'sources': [],
+                'http_proxy': parsed_proxy,
+                'https_proxy': parsed_proxy,
+                })
+
+        self.assertThat(
+            import_boot_images,
+            MockCalledOnceWith(
+                [], http_proxy=proxy, https_proxy=proxy))
 
 
 class TestClusterProtocol_IsImportBootImagesRunning(MAASTestCase):