diff --git a/ironic_python_agent/partition_utils.py b/ironic_python_agent/partition_utils.py index c32a994d0..498fb2843 100644 --- a/ironic_python_agent/partition_utils.py +++ b/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 diff --git a/ironic_python_agent/tests/unit/test_partition_utils.py b/ironic_python_agent/tests/unit/test_partition_utils.py index 1eb2cbbfd..64316dc00 100644 --- a/ironic_python_agent/tests/unit/test_partition_utils.py +++ b/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) diff --git a/releasenotes/notes/configdrive-ssl-02b069948dfef814.yaml b/releasenotes/notes/configdrive-ssl-02b069948dfef814.yaml new file mode 100644 index 000000000..61dc4cbcd --- /dev/null +++ b/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.