Merge lp:~lamont/maas/bug-1630636b into lp:~maas-committers/maas/trunk

[LaMont Jones]

Only escape single-quotes in the "turn off pxe boot" url given to the deploying node.

Instead of shell escaping the URL for "turning off pxe boot", just url-escape any single-quotes, since the url is (1) enclosed in single-quotes by the template, and (2) under the control of the MAAS admin, and running (as root) on the deploying node, which he already has full control over.

Because of the [] surrounding an ipv6 literal, pipes.quote() decides to wrap the entire thing in single quotes, which was breaking the template by causing the url to be outside of the quotes.

[Andres Rodriguez (andreserl)]

Question inline:

review: Needs Information

[Blake Rouse (blake-rouse)]

Code looks good, but I have the same question as Andres.

review: Needs Information

[Gavin Panella (allenap)]

I think this bug can be fixed imperfectly and more simply with http://paste.ubuntu.com/23314598/. The fix in this branch works but is also imperfect and a bit misleading. The test_get_curtin_config* tests also do not fail if I change quote_url to return the input unaltered.

A better fix might be http://paste.ubuntu.com/23314621/ (which works because YAML is a superset of JSON). Could you incorporate this approach into your branch, and change those tests so that they fail when the fix is absent? The preseeds are meant to be YAML, right? You could try loading them using yaml.safe_load to check that they're well formed.

review: Needs Fixing

[LaMont Jones (lamont)]

replies added. Will be pushing a branch with feedback incorporated.

[Gavin Panella (allenap)]

I think you've misunderstood what I suggested.

The point in the template where escape.shell was being used is a YAML array element. To correctly escape a value into that spot you can use json.dumps to produce something that conforms to YAML's escaping and encoding rules. Replacing single quotes with %27 is not the same.

I've put what I mean into lp:~allenap/maas/lamont--bug-1630636b which you can pull/merge into this branch. I've updated code and tests.

review: Needs Fixing

[LaMont Jones (lamont)]

Pulled Gavin's branch, made one small tweak to the test scenario generation.

[Gavin Panella (allenap)]

Looks good!

review: Approve

=== modified file 'contrib/preseeds_v2/curtin_userdata'
--- contrib/preseeds_v2/curtin_userdata	2016-10-06 01:36:10 +0000
+++ contrib/preseeds_v2/curtin_userdata	2016-10-13 16:33:14 +0000
@@ -14,7 +14,7 @@
   driver_04_load: ["sh", "-c", "depmod && modprobe {{driver['module']}}"]
 {{endif}}
 late_commands:
-  maas: [wget, '--no-proxy', '{{node_disable_pxe_url|escape.shell}}', '--post-data', '{{node_disable_pxe_data|escape.shell}}', '-O', '/dev/null']
+  maas: [wget, '--no-proxy', {{node_disable_pxe_url|escape.json}}, '--post-data', {{node_disable_pxe_data|escape.json}}, '-O', '/dev/null']
 {{if third_party_drivers and driver}}
   driver_00_key_get: curtin in-target -- sh -c "/bin/echo -en '{{key_string}}' > /tmp/maas-{{driver['package']}}.gpg"
   driver_02_key_add: ["curtin", "in-target", "--", "apt-key", "add", "/tmp/maas-{{driver['package']}}.gpg"]

=== modified file 'src/maasserver/preseed.py'
--- src/maasserver/preseed.py	2016-09-02 10:17:35 +0000
+++ src/maasserver/preseed.py	2016-10-13 16:33:14 +0000
@@ -15,8 +15,8 @@
     ]
 
 from collections import namedtuple
+import json
 import os.path
-from pipes import quote
 from urllib.parse import (
     urlencode,
     urlparse,
@@ -590,8 +590,8 @@
     """Return a singleton containing methods to escape various formats used in
     the preseed templates.
     """
-    Escape = namedtuple('Escape', 'shell')
-    return Escape(shell=quote)
+    Escape = namedtuple('Escape', 'json')
+    return Escape(json=json.dumps)
 
 
 class PreseedTemplate(tempita.Template):

=== modified file 'src/maasserver/tests/test_preseed.py'
--- src/maasserver/tests/test_preseed.py	2016-09-27 20:58:10 +0000
+++ src/maasserver/tests/test_preseed.py	2016-10-13 16:33:14 +0000
@@ -6,8 +6,8 @@
 __all__ = []
 
 import http.client
+import json
 import os
-from pipes import quote
 from unittest.mock import sentinel
 from urllib.parse import urlparse
 
@@ -80,6 +80,7 @@
     MockCalledOnceWith,
     MockNotCalled,
 )
+from maastesting.testcase import MAASTestCase
 from metadataserver.models import NodeKey
 from provisioningserver.drivers.osystem.ubuntu import UbuntuOS
 from provisioningserver.rpc.exceptions import NoConnectionsAvailable
@@ -488,14 +489,21 @@
             context['third_party_drivers'])
 
 
-class TestPreseedTemplate(MAASServerTestCase):
+class TestPreseedTemplate(MAASTestCase):
     """Tests for class:`PreseedTemplate`."""
 
-    def test_escape_shell(self):
-        template = PreseedTemplate("{{var|escape.shell}}")
-        var = "$ ! ()"
-        observed = template.substitute(var=var)
-        self.assertEqual(quote(var), observed)
+    scenarios = [
+        (name, dict(var=var, expected=json.dumps(var))) for name, var in (
+            ('plain', '$ ! ()'),
+            ('quote', "$ ' ()"),
+            ('double', '$ " ()'),
+        )
+    ]
+
+    def test_escape_json(self):
+        template = PreseedTemplate("{{var|escape.json}}")
+        observed = template.substitute(var=self.var)
+        self.assertEqual(self.expected, observed)
 
 
 class TestRenderPreseed(
@@ -851,6 +859,69 @@
                 ]
             ))
 
+    def test_get_curtin_config_with_ipv4_rack_url(self):
+        primary_rack = self.rpc_rack_controller
+        primary_rack.url = 'http://192.168.1.1:5240/'
+        primary_rack.save()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            primary_rack=primary_rack)
+        self.configure_get_boot_images_for_node(node, 'xinstall')
+        config = get_curtin_config(node)
+        yaml_conf = yaml.safe_load(config)
+        self.assertEqual('reboot', yaml_conf['power_state']['mode'])
+        self.assertEqual(
+            "%smetadata/latest/by-id/%s/" % (
+                primary_rack.url, node.system_id),
+            yaml_conf['late_commands']['maas'][2])
+        self.assertTrue('debconf_selections' in yaml_conf)
+
+    def test_get_curtin_config_with_ipv6_rack_url(self):
+        primary_rack = self.rpc_rack_controller
+        primary_rack.url = 'http://[2001:db8::1]:5240/'
+        primary_rack.save()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            primary_rack=primary_rack)
+        self.configure_get_boot_images_for_node(node, 'xinstall')
+        config = get_curtin_config(node)
+        yaml_conf = yaml.safe_load(config)
+        self.assertEqual('reboot', yaml_conf['power_state']['mode'])
+        self.assertEqual(
+            "%smetadata/latest/by-id/%s/" % (
+                primary_rack.url, node.system_id),
+            yaml_conf['late_commands']['maas'][2])
+        self.assertTrue('debconf_selections' in yaml_conf)
+
+    def test_get_curtin_config_with_name_rack_url(self):
+        primary_rack = self.rpc_rack_controller
+        primary_rack.url = 'http://%s:5240/' % factory.make_name('host')
+        primary_rack.save()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            primary_rack=primary_rack)
+        self.configure_get_boot_images_for_node(node, 'xinstall')
+        config = get_curtin_config(node)
+        yaml_conf = yaml.safe_load(config)
+        self.assertEqual('reboot', yaml_conf['power_state']['mode'])
+        self.assertEqual(
+            "%smetadata/latest/by-id/%s/" % (
+                primary_rack.url, node.system_id),
+            yaml_conf['late_commands']['maas'][2])
+        self.assertTrue('debconf_selections' in yaml_conf)
+
+    def test_get_curtin_config_with_quote_rack_url(self):
+        primary_rack = self.rpc_rack_controller
+        primary_rack.url = 'http://%s:5240/' % factory.make_name("ho'st")
+        primary_rack.save()
+        node = factory.make_Node_with_Interface_on_Subnet(
+            primary_rack=primary_rack)
+        self.configure_get_boot_images_for_node(node, 'xinstall')
+        config = get_curtin_config(node)
+        yaml_conf = yaml.safe_load(config)
+        self.assertEqual('reboot', yaml_conf['power_state']['mode'])
+        self.assertEqual(
+            "%smetadata/latest/by-id/%s/" % (primary_rack.url, node.system_id),
+            yaml_conf['late_commands']['maas'][2])
+        self.assertTrue('debconf_selections' in yaml_conf)
+
     def make_fastpath_node(self, main_arch=None):
         """Return a `Node`, with FPI enabled, and the given main architecture.