Merge "Add filename parameter to Redfish virtual media boot URL"

ironic/drivers/modules/redfish/boot.py

@@ -16,6 +16,7 @@ import tempfile
 
 from oslo_log import log
 from oslo_utils import importutils
+from six.moves.urllib import parse as urlparse
 
 from ironic.common import boot_devices
 from ironic.common import exception
@@ -218,6 +219,36 @@ class RedfishVirtualMediaBoot(base.BootInterface):
                         {'node': task.node.uuid, 'image': object_name,
                          'error': e})
 
+    @staticmethod
+    def _append_filename_param(url, filename):
+        """Append 'filename=<file>' parameter to given URL.
+
+        Some BMCs seem to validate boot image URL requiring the URL to end
+        with something resembling ISO image file name.
+
+        This function tries to add, hopefully, meaningless 'filename'
+        parameter to URL's query string in hope to make the entire boot image
+        URL looking more convincing to the BMC.
+
+        However, `url` with fragments might not get cured by this hack.
+
+        :param url: a URL to work on
+        :param filename: name of the file to append to the URL
+        :returns: original URL with 'filename' parameter appended
+        """
+        parsed_url = urlparse.urlparse(url)
+        parsed_qs = urlparse.parse_qsl(parsed_url.query)
+
+        has_filename = [x for x in parsed_qs if x[0].lower() == 'filename']
+        if has_filename:
+            return url
+
+        parsed_qs.append(('filename', filename))
+        parsed_url = list(parsed_url)
+        parsed_url[4] = urlparse.urlencode(parsed_qs)
+
+        return urlparse.urlunparse(parsed_url)
+
     @staticmethod
     def _get_floppy_image_name(node):
         """Returns the floppy image name for a given node.
@@ -279,6 +310,8 @@ class RedfishVirtualMediaBoot(base.BootInterface):
 
         image_url = swift_api.get_temp_url(container, object_name, timeout)
 
+        image_url = cls._append_filename_param(image_url, 'bootme.img')
+
         LOG.debug("Created floppy image %(name)s in Swift for node %(node)s, "
                   "exposed as temporary URL "
                   "%(url)s", {'node': task.node.uuid,
@@ -388,6 +421,9 @@ class RedfishVirtualMediaBoot(base.BootInterface):
             boot_iso_url = swift_api.get_temp_url(
                 container, iso_object_name, timeout)
 
+            boot_iso_url = cls._append_filename_param(
+                boot_iso_url, 'bootme.iso')
+
         LOG.debug("Created ISO %(name)s in Swift for node %(node)s, exposed "
                   "as temporary URL %(url)s", {'node': task.node.uuid,
                                                'name': iso_object_name,

ironic/tests/unit/drivers/modules/redfish/test_boot.py

@@ -175,6 +175,30 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                               task.driver.boot._parse_deploy_info,
                               task.node)
 
+    def test__append_filename_param_without_qs(self):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            res = task.driver.boot._append_filename_param(
+                'http://a.b/c', 'b.img')
+            expected = 'http://a.b/c?filename=b.img'
+            self.assertEqual(expected, res)
+
+    def test__append_filename_param_with_qs(self):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            res = task.driver.boot._append_filename_param(
+                'http://a.b/c?d=e&f=g', 'b.img')
+            expected = 'http://a.b/c?d=e&f=g&filename=b.img'
+            self.assertEqual(expected, res)
+
+    def test__append_filename_param_with_filename(self):
+        with task_manager.acquire(self.context, self.node.uuid,
+                                  shared=True) as task:
+            res = task.driver.boot._append_filename_param(
+                'http://a.b/c?filename=bootme.img', 'b.img')
+            expected = 'http://a.b/c?filename=bootme.img'
+            self.assertEqual(expected, res)
+
     @mock.patch.object(redfish_boot, 'swift', autospec=True)
     def test__cleanup_floppy_image(self, mock_swift):
         with task_manager.acquire(self.context, self.node.uuid,
@@ -193,14 +217,18 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
     def test__prepare_floppy_image(self, mock_create_vfat_image, mock_swift):
         with task_manager.acquire(self.context, self.node.uuid,
                                   shared=True) as task:
-            task.driver.boot._prepare_floppy_image(task)
+            mock_swift_api = mock_swift.SwiftAPI.return_value
+            mock_swift_api.get_temp_url.return_value = 'https://a.b/c.f?e=f'
+
+            url = task.driver.boot._prepare_floppy_image(task)
+
+            self.assertIn('filename=bootme.img', url)
+
+            mock_swift.SwiftAPI.assert_called_once_with()
 
             mock_create_vfat_image.assert_called_once_with(
                 mock.ANY, parameters=mock.ANY)
 
-            mock_swift.SwiftAPI.assert_called_once_with()
-            mock_swift_api = mock_swift.SwiftAPI.return_value
-
             mock_swift_api.create_object.assert_called_once_with(
                 mock.ANY, mock.ANY, mock.ANY, mock.ANY)
 
@@ -234,7 +262,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                 task, 'http://kernel/img', 'http://ramdisk/img',
                 'http://bootloader/img', root_uuid=task.node.uuid)
 
-            self.assertTrue(url)
+            self.assertIn('filename=bootme.iso', url)
 
             mock_create_boot_iso.assert_called_once_with(
                 mock.ANY, mock.ANY, 'http://kernel/img', 'http://ramdisk/img',
@@ -263,7 +291,7 @@ class RedfishVirtualMediaBootTestCase(db_base.DbTestCase):
                 task, 'http://kernel/img', 'http://ramdisk/img',
                 bootloader_href=None, root_uuid=task.node.uuid)
 
-            self.assertTrue(url)
+            self.assertIn('filename=bootme.iso', url)
 
             mock_create_boot_iso.assert_called_once_with(
                 mock.ANY, mock.ANY, 'http://kernel/img', 'http://ramdisk/img',