launchpad-reviewers team mailing list archive

[Julian Edwards <julian.edwards@xxxxxxxxxxxxx>]

Julian Edwards has proposed merging lp:~julian-edwards/maas/ephemeral_proxying_bug-1116331 into lp:maas.

Commit message:
Ensure that proxy settings are passed into enlistment and commissioning environments.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1116331 in MAAS: "proxy not used in ephemeral or commissioning"
  https://bugs.launchpad.net/maas/+bug/1116331

For more details, see:
https://code.launchpad.net/~julian-edwards/maas/ephemeral_proxying_bug-1116331/+merge/147822

The proxy settings were not being passed into enlistment and commissioning environments.  This change rectifies that.

There's two lots of user data; one for enlistment and one for commissioning.  The latter is now composed in a nicer programmatic way using MIME multipart so that the cloud-config part can be set (which sets the proxy) as well as providing the script to run.  At some point we ought to do this for the enlistment user data too.

I made the cloud-config part of the commissioning user data a separate template for maximum flexibility.
-- 
https://code.launchpad.net/~julian-edwards/maas/ephemeral_proxying_bug-1116331/+merge/147822
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~julian-edwards/maas/ephemeral_proxying_bug-1116331 into lp:maas.

=== modified file 'contrib/preseeds_v2/enlist_userdata'
--- contrib/preseeds_v2/enlist_userdata	2012-11-30 17:35:04 +0000
+++ contrib/preseeds_v2/enlist_userdata	2013-02-12 02:14:20 +0000
@@ -1,7 +1,10 @@
 #cloud-config
 
-# could/should set local mirror here or proxy here
-# apt_proxy: http://{{server_host}}:8000/
+{{if http_proxy}}
+apt_proxy: http_proxy
+{{elif server_host}}
+apt_proxy: http://{{server_host}}:8000/
+{{endif}}
 
 misc_bucket:
  - &maas_enlist |

=== modified file 'src/maasserver/models/node.py'
--- src/maasserver/models/node.py	2013-01-14 22:06:12 +0000
+++ src/maasserver/models/node.py	2013-02-12 02:14:20 +0000
@@ -635,7 +635,7 @@
         from metadataserver.commissioning.user_data import generate_user_data
         from metadataserver.models import NodeCommissionResult
 
-        commissioning_user_data = generate_user_data()
+        commissioning_user_data = generate_user_data(nodegroup=self.nodegroup)
         NodeCommissionResult.objects.clear_results(self)
         self.status = NODE_STATUS.COMMISSIONING
         self.owner = user

=== modified file 'src/maasserver/preseed.py'
--- src/maasserver/preseed.py	2012-11-27 10:39:51 +0000
+++ src/maasserver/preseed.py	2013-02-12 02:14:20 +0000
@@ -15,6 +15,7 @@
     'compose_preseed_url',
     'get_enlist_preseed',
     'get_preseed',
+    'get_preseed_context',
     ]
 
 from collections import namedtuple

=== modified file 'src/maasserver/tests/test_node.py'
--- src/maasserver/tests/test_node.py	2012-12-19 12:23:48 +0000
+++ src/maasserver/tests/test_node.py	2013-02-12 02:14:20 +0000
@@ -489,6 +489,8 @@
             ).return_value = user_data
         node.start_commissioning(factory.make_admin())
         self.assertEqual(user_data, NodeUserData.objects.get_user_data(node))
+        commissioning.user_data.generate_user_data.assert_called_with(
+            nodegroup=node.nodegroup)
 
     def test_start_commissioning_clears_node_commissioning_results(self):
         node = factory.make_node(status=NODE_STATUS.DECLARED)

=== modified file 'src/metadataserver/commissioning/tests/test_user_data.py'
--- src/metadataserver/commissioning/tests/test_user_data.py	2012-12-18 12:31:47 +0000
+++ src/metadataserver/commissioning/tests/test_user_data.py	2013-02-12 02:14:20 +0000
@@ -17,6 +17,7 @@
 from maasserver.testing.factory import factory
 from maasserver.testing.testcase import TestCase
 from maastesting.matchers import ContainsAll
+from metadataserver.commissioning import user_data
 from metadataserver.commissioning.user_data import (
     generate_user_data,
     is_snippet,
@@ -24,6 +25,10 @@
     read_snippet,
     strip_name,
     )
+from mock import (
+    Mock,
+    sentinel,
+    )
 
 
 class TestUserData(TestCase):
@@ -72,3 +77,24 @@
                 'def authenticate_headers',
                 'def encode_multipart_data',
             }))
+
+    def test_nodegroup_passed_to_get_preseed_context(self):
+        # I don't care about what effect it has, I just want to know
+        # that it was passed as it can affect the contents of
+        # `server_host` in the context.
+        fake_context = dict(http_proxy=factory.getRandomString())
+        user_data.get_preseed_context = Mock(return_value=fake_context)
+        nodegroup = sentinel.nodegroup
+        generate_user_data(nodegroup)
+        user_data.get_preseed_context.assert_called_with(nodegroup=nodegroup)
+
+    def test_generate_user_data_generates_mime_multipart(self):
+        # The generate_user_data func should create a MIME multipart
+        # message consisting of cloud-config and x-shellscript
+        # attachments.
+        self.assertThat(
+            generate_user_data(), ContainsAll({
+                'multipart',
+                'Content-Type: text/cloud-config',
+                'Content-Type: text/x-shellscript',
+            }))

=== modified file 'src/metadataserver/commissioning/user_data.py'
--- src/metadataserver/commissioning/user_data.py	2012-12-18 12:31:47 +0000
+++ src/metadataserver/commissioning/user_data.py	2013-02-12 02:14:20 +0000
@@ -21,9 +21,12 @@
     'generate_user_data',
     ]
 
+from email.mime.multipart import MIMEMultipart
+from email.mime.text import MIMEText
 from os import listdir
 import os.path
 
+from maasserver.preseed import get_preseed_context
 import tempita
 
 
@@ -58,24 +61,55 @@
     return snippet_name.replace('.', '_')
 
 
-def generate_user_data():
+def generate_user_data(nodegroup=None):
     """Produce the main commissioning script.
 
-    The script was templated so that code snippets become easier to
-    maintain, check for lint, and ideally, unit-test.  However its
-    contents are static: there are no variables.  It's perfectly
-    cacheable.
+    The main template file contains references to so-called ``snippets''
+    which are read in here, and substituted.  In addition, the regular
+    preseed context variables are available (such as 'http_proxy').
+
+    The final result is a MIME multipart message that consists of a
+    'cloud-config' part and an 'x-shellscript' part.  This allows maximum
+    flexibility with cloud-init as we read in a template
+    'user_data_config.template' to set cloud-init configs before the script
+    is run.
 
     :rtype: `bytes`
     """
-    encoding = 'utf-8'
+    ENCODING = 'utf-8'
     commissioning_dir = os.path.dirname(__file__)
-    template_file = os.path.join(commissioning_dir, 'user_data.template')
+    userdata_template_file = os.path.join(
+        commissioning_dir, 'user_data.template')
+    config_template_file = os.path.join(
+        commissioning_dir, 'user_data_config.template')
     snippets_dir = os.path.join(commissioning_dir, 'snippets')
-    template = tempita.Template.from_filename(template_file, encoding=encoding)
+    userdata_template = tempita.Template.from_filename(
+        userdata_template_file, encoding=ENCODING)
+    config_template = tempita.Template.from_filename(
+        config_template_file, encoding=ENCODING)
+    # The preseed context is a dict containing various configs that the
+    # templates can use.
+    preseed_context = get_preseed_context(nodegroup=nodegroup)
 
+    # Render the snippets in the main template.
     snippets = {
-        strip_name(name): read_snippet(snippets_dir, name, encoding=encoding)
+        strip_name(name): read_snippet(snippets_dir, name, encoding=ENCODING)
         for name in list_snippets(snippets_dir)
     }
-    return template.substitute(snippets).encode('utf-8')
+    snippets.update(preseed_context)
+    userdata = userdata_template.substitute(snippets).encode(ENCODING)
+
+    # Render the config.
+    config = config_template.substitute(preseed_context)
+
+    # Create a MIME multipart message from the config and the userdata.
+    config_part = MIMEText(config, 'cloud-config', ENCODING)
+    config_part.add_header(
+        'Content-Disposition', 'attachment; filename="config"')
+    data_part = MIMEText(userdata, 'x-shellscript', ENCODING)
+    data_part.add_header(
+        'Content-Disposition', 'attachment; filename="user_data.sh"')
+    combined = MIMEMultipart()
+    combined.attach(config_part)
+    combined.attach(data_part)
+    return combined.as_string()

=== added file 'src/metadataserver/commissioning/user_data_config.template'
--- src/metadataserver/commissioning/user_data_config.template	1970-01-01 00:00:00 +0000
+++ src/metadataserver/commissioning/user_data_config.template	2013-02-12 02:14:20 +0000
@@ -0,0 +1,7 @@
+#cloud-config
+{{if http_proxy}}
+apt_proxy: http_proxy
+{{elif server_host}}
+apt_proxy: http://{{server_host}}:8000/
+{{endif}}
+