diff --git a/ironic/common/image_service.py b/ironic/common/image_service.py index aec53c8aef..7e76829cfd 100644 --- a/ironic/common/image_service.py +++ b/ironic/common/image_service.py @@ -98,7 +98,8 @@ class HttpImageService(BaseImageService): verify = CONF.webserver_verify_ca try: - response = requests.head(image_href, verify=verify) + response = requests.head(image_href, verify=verify, + timeout=CONF.webserver_connection_timeout) if response.status_code != http_client.OK: raise exception.ImageRefValidationFailed( image_href=output_url, @@ -130,8 +131,8 @@ class HttpImageService(BaseImageService): verify = CONF.webserver_verify_ca try: - response = requests.get(image_href, stream=True, - verify=verify) + response = requests.get(image_href, stream=True, verify=verify, + timeout=CONF.webserver_connection_timeout) if response.status_code != http_client.OK: raise exception.ImageRefValidationFailed( image_href=image_href, diff --git a/ironic/conf/default.py b/ironic/conf/default.py index 5cee6be310..c8359b20f8 100644 --- a/ironic/conf/default.py +++ b/ironic/conf/default.py @@ -362,7 +362,7 @@ utils_opts = [ 'dir.')), ] -cert_verify_opts = [ +webserver_opts = [ cfg.StrOpt('webserver_verify_ca', default='True', mutable=True, @@ -380,6 +380,10 @@ cert_verify_opts = [ 'is set to True i.e the certificates present in the ' 'standard path are used for SSL verification.' 'Defaults to True.')), + cfg.IntOpt('webserver_connection_timeout', + default=60, + help=_('Connection timeout when accessing remote web servers ' + 'with images.')), ] @@ -396,4 +400,4 @@ def register_opts(conf): conf.register_opts(portgroup_opts) conf.register_opts(service_opts) conf.register_opts(utils_opts) - conf.register_opts(cert_verify_opts) + conf.register_opts(webserver_opts) diff --git a/ironic/tests/unit/common/test_image_service.py b/ironic/tests/unit/common/test_image_service.py index aff875a08f..c04ae3801d 100644 --- a/ironic/tests/unit/common/test_image_service.py +++ b/ironic/tests/unit/common/test_image_service.py @@ -43,7 +43,8 @@ class HttpImageServiceTestCase(base.TestCase): response.status_code = http_client.OK self.service.validate_href(self.href) path_mock.assert_not_called() - head_mock.assert_called_once_with(self.href, verify=True) + head_mock.assert_called_once_with(self.href, verify=True, + timeout=60) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -60,7 +61,8 @@ class HttpImageServiceTestCase(base.TestCase): response = head_mock.return_value response.status_code = http_client.OK self.service.validate_href(self.href) - head_mock.assert_called_once_with(self.href, verify=False) + head_mock.assert_called_once_with(self.href, verify=False, + timeout=60) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -76,7 +78,8 @@ class HttpImageServiceTestCase(base.TestCase): head_mock.side_effect = requests.ConnectionError() self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) - head_mock.assert_called_once_with(self.href, verify=False) + head_mock.assert_called_once_with(self.href, verify=False, + timeout=60) head_mock.side_effect = requests.RequestException() self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) @@ -88,7 +91,8 @@ class HttpImageServiceTestCase(base.TestCase): response = head_mock.return_value response.status_code = http_client.OK self.service.validate_href(self.href) - head_mock.assert_called_once_with(self.href, verify=True) + head_mock.assert_called_once_with(self.href, verify=True, + timeout=60) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -105,7 +109,8 @@ class HttpImageServiceTestCase(base.TestCase): head_mock.side_effect = requests.ConnectionError() self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) - head_mock.assert_called_once_with(self.href, verify=True) + head_mock.assert_called_once_with(self.href, verify=True, + timeout=60) head_mock.side_effect = requests.RequestException() self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) @@ -118,7 +123,26 @@ class HttpImageServiceTestCase(base.TestCase): response.status_code = http_client.OK self.service.validate_href(self.href) - head_mock.assert_called_once_with(self.href, verify='/some/path') + head_mock.assert_called_once_with(self.href, verify='/some/path', + timeout=60) + response.status_code = http_client.NO_CONTENT + self.assertRaises(exception.ImageRefValidationFailed, + self.service.validate_href, + self.href) + response.status_code = http_client.BAD_REQUEST + self.assertRaises(exception.ImageRefValidationFailed, + self.service.validate_href, + self.href) + + @mock.patch.object(requests, 'head', autospec=True) + def test_validate_href_custom_timeout(self, head_mock): + cfg.CONF.set_override('webserver_connection_timeout', 15) + + response = head_mock.return_value + response.status_code = http_client.OK + self.service.validate_href(self.href) + head_mock.assert_called_once_with(self.href, verify=True, + timeout=15) response.status_code = http_client.NO_CONTENT self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, @@ -137,7 +161,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) - head_mock.assert_called_once_with(self.href, verify='/some/path') + head_mock.assert_called_once_with(self.href, verify='/some/path', + timeout=60) @mock.patch.object(requests, 'head', autospec=True) def test_validate_href_verify_error(self, head_mock): @@ -145,7 +170,8 @@ class HttpImageServiceTestCase(base.TestCase): head_mock.side_effect = requests.RequestException() self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) - head_mock.assert_called_once_with(self.href, verify='/some/path') + head_mock.assert_called_once_with(self.href, verify='/some/path', + timeout=60) @mock.patch.object(requests, 'head', autospec=True) def test_validate_href_verify_os_error(self, head_mock): @@ -153,7 +179,8 @@ class HttpImageServiceTestCase(base.TestCase): head_mock.side_effect = OSError() self.assertRaises(exception.ImageRefValidationFailed, self.service.validate_href, self.href) - head_mock.assert_called_once_with(self.href, verify='/some/path') + head_mock.assert_called_once_with(self.href, verify='/some/path', + timeout=60) @mock.patch.object(requests, 'head', autospec=True) def test_validate_href_error_with_secret_parameter(self, head_mock): @@ -165,7 +192,8 @@ class HttpImageServiceTestCase(base.TestCase): True) self.assertIn('secreturl', str(e)) self.assertNotIn(self.href, str(e)) - head_mock.assert_called_once_with(self.href, verify=False) + head_mock.assert_called_once_with(self.href, verify=False, + timeout=60) @mock.patch.object(requests, 'head', autospec=True) def _test_show(self, head_mock, mtime, mtime_date): @@ -175,7 +203,8 @@ class HttpImageServiceTestCase(base.TestCase): 'Last-Modified': mtime } result = self.service.show(self.href) - head_mock.assert_called_once_with(self.href, verify=True) + head_mock.assert_called_once_with(self.href, verify=True, + timeout=60) self.assertEqual({'size': 100, 'updated_at': mtime_date, 'properties': {}}, result) @@ -197,7 +226,8 @@ class HttpImageServiceTestCase(base.TestCase): head_mock.return_value.headers = {} self.assertRaises(exception.ImageRefValidationFailed, self.service.show, self.href) - head_mock.assert_called_with(self.href, verify=True) + head_mock.assert_called_with(self.href, verify=True, + timeout=60) @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) @@ -213,7 +243,8 @@ class HttpImageServiceTestCase(base.TestCase): image_service.IMAGE_CHUNK_SIZE ) req_get_mock.assert_called_once_with(self.href, stream=True, - verify=True) + verify=True, + timeout=60) @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) @@ -230,7 +261,8 @@ class HttpImageServiceTestCase(base.TestCase): image_service.IMAGE_CHUNK_SIZE ) req_get_mock.assert_called_once_with(self.href, stream=True, - verify=False) + verify=False, + timeout=60) @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) @@ -247,7 +279,8 @@ class HttpImageServiceTestCase(base.TestCase): image_service.IMAGE_CHUNK_SIZE ) req_get_mock.assert_called_once_with(self.href, stream=True, - verify=True) + verify=True, + timeout=60) @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) @@ -264,7 +297,8 @@ class HttpImageServiceTestCase(base.TestCase): image_service.IMAGE_CHUNK_SIZE ) req_get_mock.assert_called_once_with(self.href, stream=True, - verify='/some/path') + verify='/some/path', + timeout=60) @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) @@ -289,7 +323,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageDownloadFailed, self.service.download, self.href, file_mock) req_get_mock.assert_called_once_with(self.href, stream=True, - verify=False) + verify=False, + timeout=60) @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) @@ -305,7 +340,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageDownloadFailed, self.service.download, self.href, file_mock) req_get_mock.assert_called_once_with(self.href, stream=True, - verify='/some/path') + verify='/some/path', + timeout=60) @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) @@ -320,7 +356,8 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageDownloadFailed, self.service.download, self.href, file_mock) req_get_mock.assert_called_once_with(self.href, stream=True, - verify='/some/path') + verify='/some/path', + timeout=60) @mock.patch.object(shutil, 'copyfileobj', autospec=True) @mock.patch.object(requests, 'get', autospec=True) @@ -335,7 +372,26 @@ class HttpImageServiceTestCase(base.TestCase): self.assertRaises(exception.ImageDownloadFailed, self.service.download, self.href, file_mock) req_get_mock.assert_called_once_with(self.href, stream=True, - verify='/some/path') + verify='/some/path', + timeout=60) + + @mock.patch.object(shutil, 'copyfileobj', autospec=True) + @mock.patch.object(requests, 'get', autospec=True) + def test_download_success_custom_timeout( + self, req_get_mock, shutil_mock): + cfg.CONF.set_override('webserver_connection_timeout', 15) + response_mock = req_get_mock.return_value + response_mock.status_code = http_client.OK + response_mock.raw = mock.MagicMock(spec=io.BytesIO) + file_mock = mock.Mock(spec=io.BytesIO) + self.service.download(self.href, file_mock) + shutil_mock.assert_called_once_with( + response_mock.raw.__enter__(), file_mock, + image_service.IMAGE_CHUNK_SIZE + ) + req_get_mock.assert_called_once_with(self.href, stream=True, + verify=True, + timeout=15) class FileImageServiceTestCase(base.TestCase): diff --git a/releasenotes/notes/webserver-timeout-d85781bf634cef39.yaml b/releasenotes/notes/webserver-timeout-d85781bf634cef39.yaml new file mode 100644 index 0000000000..085d48565c --- /dev/null +++ b/releasenotes/notes/webserver-timeout-d85781bf634cef39.yaml @@ -0,0 +1,7 @@ +--- +fixes: + - | + Adds timeout to HTTP image validation and downloading operations, so that + the ``direct`` deploy does not hang when the remote server is not + responsive. The default timeout is 60 seconds and can be changed via the + new ``webserver_connection_timeout`` option.