Respect global parameters when downloading a configdrive

* Use the same TLS parameters as everything else
* Respect image_download_connection_timeout
* Do not ignore HTTP errors

Change-Id: I84f8021f731186d82e44ac3d4ef2d12df13f830a

ironic_python_agent/partition_utils.py

@@ -29,13 +29,17 @@ from ironic_lib import disk_utils
 from ironic_lib import exception
 from ironic_lib import utils
 from oslo_concurrency import processutils
+from oslo_config import cfg
 from oslo_log import log
 from oslo_utils import excutils
 from oslo_utils import units
 import requests
 
+from ironic_python_agent import utils as ipa_utils
+
 
 LOG = log.getLogger()
+CONF = cfg.CONF
 
 MAX_CONFIG_DRIVE_SIZE_MB = 64
 
@@ -59,13 +63,27 @@ def get_configdrive(configdrive, node_uuid, tempdir=None):
     # Check if the configdrive option is a HTTP URL or the content directly
     is_url = utils.is_http_url(configdrive)
     if is_url:
+        verify, cert = ipa_utils.get_ssl_client_options(CONF)
+        timeout = CONF.image_download_connection_timeout
+        # TODO(dtantsur): support proxy parameters from instance_info
         try:
-            data = requests.get(configdrive).content
+            resp = requests.get(configdrive, verify=verify, cert=cert,
+                                timeout=timeout)
         except requests.exceptions.RequestException as e:
             raise exception.InstanceDeployFailure(
                 "Can't download the configdrive content for node %(node)s "
                 "from '%(url)s'. Reason: %(reason)s" %
                 {'node': node_uuid, 'url': configdrive, 'reason': e})
+
+        if resp.status_code >= 400:
+            raise exception.InstanceDeployFailure(
+                "Can't download the configdrive content for node %(node)s "
+                "from '%(url)s'. Got status code %(code)s, response "
+                "body %(body)s" %
+                {'node': node_uuid, 'url': configdrive,
+                 'code': resp.status_code, 'body': resp.text})
+
+        data = resp.content
     else:
         data = configdrive
 

ironic_python_agent/tests/unit/test_partition_utils.py

@@ -32,26 +32,68 @@ class GetConfigdriveTestCase(base.IronicAgentTest):
 
     @mock.patch.object(gzip, 'GzipFile', autospec=True)
     def test_get_configdrive(self, mock_gzip, mock_requests, mock_copy):
-        mock_requests.return_value = mock.MagicMock(content='Zm9vYmFy')
+        mock_requests.return_value = mock.MagicMock(content='Zm9vYmFy',
+                                                    status_code=200)
         tempdir = tempfile.mkdtemp()
         (size, path) = partition_utils.get_configdrive('http://1.2.3.4/cd',
                                                        'fake-node-uuid',
                                                        tempdir=tempdir)
         self.assertTrue(path.startswith(tempdir))
-        mock_requests.assert_called_once_with('http://1.2.3.4/cd')
+        mock_requests.assert_called_once_with('http://1.2.3.4/cd',
+                                              verify=True, cert=None,
+                                              timeout=60)
+        mock_gzip.assert_called_once_with('configdrive', 'rb',
+                                          fileobj=mock.ANY)
+        mock_copy.assert_called_once_with(mock.ANY, mock.ANY)
+
+    @mock.patch.object(gzip, 'GzipFile', autospec=True)
+    def test_get_configdrive_insecure(self, mock_gzip, mock_requests,
+                                      mock_copy):
+        self.config(insecure=True)
+        mock_requests.return_value = mock.MagicMock(content='Zm9vYmFy',
+                                                    status_code=200)
+        tempdir = tempfile.mkdtemp()
+        (size, path) = partition_utils.get_configdrive('http://1.2.3.4/cd',
+                                                       'fake-node-uuid',
+                                                       tempdir=tempdir)
+        self.assertTrue(path.startswith(tempdir))
+        mock_requests.assert_called_once_with('http://1.2.3.4/cd',
+                                              verify=False, cert=None,
+                                              timeout=60)
+        mock_gzip.assert_called_once_with('configdrive', 'rb',
+                                          fileobj=mock.ANY)
+        mock_copy.assert_called_once_with(mock.ANY, mock.ANY)
+
+    @mock.patch.object(gzip, 'GzipFile', autospec=True)
+    def test_get_configdrive_ssl(self, mock_gzip, mock_requests, mock_copy):
+        self.config(cafile='cafile', keyfile='keyfile', certfile='certfile')
+        mock_requests.return_value = mock.MagicMock(content='Zm9vYmFy',
+                                                    status_code=200)
+        tempdir = tempfile.mkdtemp()
+        (size, path) = partition_utils.get_configdrive('http://1.2.3.4/cd',
+                                                       'fake-node-uuid',
+                                                       tempdir=tempdir)
+        self.assertTrue(path.startswith(tempdir))
+        mock_requests.assert_called_once_with('http://1.2.3.4/cd',
+                                              verify='cafile',
+                                              cert=('certfile', 'keyfile'),
+                                              timeout=60)
         mock_gzip.assert_called_once_with('configdrive', 'rb',
                                           fileobj=mock.ANY)
         mock_copy.assert_called_once_with(mock.ANY, mock.ANY)
 
     def test_get_configdrive_binary(self, mock_requests, mock_copy):
-        mock_requests.return_value = mock.MagicMock(content=b'content')
+        mock_requests.return_value = mock.MagicMock(content=b'content',
+                                                    status_code=200)
         tempdir = tempfile.mkdtemp()
         (size, path) = partition_utils.get_configdrive('http://1.2.3.4/cd',
                                                        'fake-node-uuid',
                                                        tempdir=tempdir)
         self.assertTrue(path.startswith(tempdir))
         self.assertEqual(b'content', open(path, 'rb').read())
-        mock_requests.assert_called_once_with('http://1.2.3.4/cd')
+        mock_requests.assert_called_once_with('http://1.2.3.4/cd',
+                                              verify=True, cert=None,
+                                              timeout=60)
         self.assertFalse(mock_copy.called)
 
     @mock.patch.object(gzip, 'GzipFile', autospec=True)
@@ -70,6 +112,14 @@ class GetConfigdriveTestCase(base.IronicAgentTest):
                           'http://1.2.3.4/cd', 'fake-node-uuid')
         self.assertFalse(mock_copy.called)
 
+    def test_get_configdrive_bad_status_code(self, mock_requests, mock_copy):
+        mock_requests.return_value = mock.MagicMock(text='Not found',
+                                                    status_code=404)
+        self.assertRaises(exception.InstanceDeployFailure,
+                          partition_utils.get_configdrive,
+                          'http://1.2.3.4/cd', 'fake-node-uuid')
+        self.assertFalse(mock_copy.called)
+
     def test_get_configdrive_base64_error(self, mock_requests, mock_copy):
         self.assertRaises(exception.InstanceDeployFailure,
                           partition_utils.get_configdrive,
@@ -79,12 +129,15 @@ class GetConfigdriveTestCase(base.IronicAgentTest):
     @mock.patch.object(gzip, 'GzipFile', autospec=True)
     def test_get_configdrive_gzip_error(self, mock_gzip, mock_requests,
                                         mock_copy):
-        mock_requests.return_value = mock.MagicMock(content='Zm9vYmFy')
+        mock_requests.return_value = mock.MagicMock(content='Zm9vYmFy',
+                                                    status_code=200)
         mock_copy.side_effect = IOError
         self.assertRaises(exception.InstanceDeployFailure,
                           partition_utils.get_configdrive,
                           'http://1.2.3.4/cd', 'fake-node-uuid')
-        mock_requests.assert_called_once_with('http://1.2.3.4/cd')
+        mock_requests.assert_called_once_with('http://1.2.3.4/cd',
+                                              verify=True, cert=None,
+                                              timeout=60)
         mock_gzip.assert_called_once_with('configdrive', 'rb',
                                           fileobj=mock.ANY)
         mock_copy.assert_called_once_with(mock.ANY, mock.ANY)

releasenotes/notes/configdrive-ssl-02b069948dfef814.yaml

@@ -0,0 +1,12 @@
+---
+fixes:
+  - |
+    No longer ignores global TLS configuration options (``ipa-insecure``, etc)
+    when downloading a configdrive via a URL.
+  - |
+    No longer ignores error status codes from the server when downloading
+    a configdrive via a URL.
+  - |
+    The configdrive downloading code now respects the
+    ``ipa-image-download-connection-timeout`` option and will no longer hang
+    for a long time if the server does not respond.