Do not close returned image-chunk iterator & get_verifier early
The GlanceClientWrapper._get_verifier method may fail already on the metadata, so we better call it early before we open files and start downloads, which we then abort uncleanly. This also likely how the bug #1948706 was triggered in the first place: - The file gets opened - _get_verifier fails *before* we even iterate over the data - glance_utils.IterableWithLength won't close the underlying iterator. The added close statement, now guarded with `may_close_iterator` is likely superfluous. If we return the image chunk iterator, then we should rather not close the underlying iterable, as it will kill the transfer. Closes-Bug: #2053027 Change-Id: Ia247af39a96fbed90b027ad30158e66dd2f0bd5e
This commit is contained in:
Fabian Wiesel
parent
ca1db54f1b
commit
198805c7c5
|
|
@ -341,34 +341,49 @@ class GlanceImageServiceV2(object):
|
|||
if not any(check(mode) for check in (stat.S_ISFIFO, stat.S_ISSOCK)):
|
||||
os.fsync(fileno)
|
||||
|
||||
def _try_special_handlers(self, context, image_id, dst_path, verifier):
|
||||
image = self.show(context, image_id, include_locations=True)
|
||||
for entry in image.get('locations', []):
|
||||
loc_url = entry['url']
|
||||
loc_meta = entry['metadata']
|
||||
o = urlparse.urlparse(loc_url)
|
||||
xfer_method = self._get_transfer_method(o.scheme)
|
||||
if not xfer_method:
|
||||
continue
|
||||
|
||||
try:
|
||||
xfer_method(context, o, dst_path, loc_meta)
|
||||
LOG.info("Successfully transferred using %s", o.scheme)
|
||||
|
||||
if not verifier:
|
||||
return True
|
||||
|
||||
# Load chunks from the downloaded image file
|
||||
# for verification
|
||||
with open(dst_path, 'rb') as fh:
|
||||
downloaded_length = os.path.getsize(dst_path)
|
||||
image_chunks = glance_utils.IterableWithLength(fh,
|
||||
downloaded_length)
|
||||
self._verify_and_write(context, image_id, verifier,
|
||||
image_chunks, None, None)
|
||||
return True
|
||||
except Exception:
|
||||
LOG.exception("Download image error")
|
||||
|
||||
return False
|
||||
|
||||
def download(self, context, image_id, data=None, dst_path=None,
|
||||
trusted_certs=None):
|
||||
"""Calls out to Glance for data and writes data."""
|
||||
# First, try to get the verifier, so we do not even start to download
|
||||
# the image and then fail on the metadata
|
||||
verifier = self._get_verifier(context, image_id, trusted_certs)
|
||||
|
||||
# First, check if image could be directly downloaded by special handler
|
||||
# Second, try to delegate image download to a special handler
|
||||
if (self._download_handlers and dst_path is not None):
|
||||
image = self.show(context, image_id, include_locations=True)
|
||||
for entry in image.get('locations', []):
|
||||
loc_url = entry['url']
|
||||
loc_meta = entry['metadata']
|
||||
o = urlparse.urlparse(loc_url)
|
||||
xfer_method = self._get_transfer_method(o.scheme)
|
||||
if xfer_method:
|
||||
try:
|
||||
xfer_method(context, o, dst_path, loc_meta)
|
||||
LOG.info("Successfully transferred using %s", o.scheme)
|
||||
|
||||
# Load chunks from the downloaded image file
|
||||
# for verification (if required)
|
||||
with open(dst_path, 'rb') as fh:
|
||||
downloaded_length = os.path.getsize(dst_path)
|
||||
image_chunks = glance_utils.IterableWithLength(fh,
|
||||
downloaded_length)
|
||||
self._verify_and_write(context, image_id,
|
||||
trusted_certs, image_chunks, None, None)
|
||||
return
|
||||
except Exception:
|
||||
LOG.exception("Download image error")
|
||||
if self._try_special_handlers(context, image_id, dst_path,
|
||||
verifier):
|
||||
return
|
||||
|
||||
# By default (or if direct download has failed), use glance client call
|
||||
# to fetch the image and fill image_chunks
|
||||
|
|
@ -384,10 +399,10 @@ class GlanceImageServiceV2(object):
|
|||
raise exception.ImageUnacceptable(image_id=image_id,
|
||||
reason='Image has no associated data')
|
||||
|
||||
return self._verify_and_write(context, image_id, trusted_certs,
|
||||
return self._verify_and_write(context, image_id, verifier,
|
||||
image_chunks, data, dst_path)
|
||||
|
||||
def _verify_and_write(self, context, image_id, trusted_certs,
|
||||
def _verify_and_write(self, context, image_id, verifier,
|
||||
image_chunks, data, dst_path):
|
||||
"""Perform image signature verification and save the image file if
|
||||
needed.
|
||||
|
|
@ -398,9 +413,7 @@ class GlanceImageServiceV2(object):
|
|||
be written out but instead image_chunks iterator is returned.
|
||||
|
||||
:param image_id: The UUID of the image
|
||||
:param trusted_certs: A 'nova.objects.trusted_certs.TrustedCerts'
|
||||
object with a list of trusted image certificate
|
||||
IDs.
|
||||
:param verifier: An instance of a 'cursive.verifier'
|
||||
:param image_chunks An iterator pointing to the image data
|
||||
:param data: File object to use when writing the image.
|
||||
If passed as None and dst_path is provided, new file is opened.
|
||||
|
|
@ -421,15 +434,9 @@ class GlanceImageServiceV2(object):
|
|||
write_image = False
|
||||
|
||||
try:
|
||||
# Retrieve properties for verification of Glance image signature
|
||||
verifier = self._get_verifier(context, image_id, trusted_certs)
|
||||
|
||||
# Exit early if we do not need write nor verify
|
||||
if verifier is None and write_image is False:
|
||||
if data is None:
|
||||
return image_chunks
|
||||
else:
|
||||
return
|
||||
if verifier is None and not write_image:
|
||||
return image_chunks
|
||||
|
||||
for chunk in image_chunks:
|
||||
if verifier:
|
||||
|
|
@ -464,8 +471,6 @@ class GlanceImageServiceV2(object):
|
|||
data.flush()
|
||||
self._safe_fsync(data)
|
||||
data.close()
|
||||
if isinstance(image_chunks, glance_utils.IterableWithLength):
|
||||
image_chunks.iterable.close()
|
||||
|
||||
if data is None:
|
||||
return image_chunks
|
||||
|
|
|
|||
|
|
@ -587,6 +587,25 @@ class TestDownloadNoDirectUri(test.NoDBTestCase):
|
|||
ctx, 2, 'data', args=(mock.sentinel.image_id,))
|
||||
self.assertEqual(mock.sentinel.image_chunks, res)
|
||||
|
||||
@mock.patch('builtins.open')
|
||||
@mock.patch('nova.image.glance.GlanceImageServiceV2.show')
|
||||
def test_download_no_data_no_dest_path_iterator_v2(
|
||||
self, show_mock, open_mock):
|
||||
glance_iterable = mock.MagicMock(spec=io.BytesIO)
|
||||
fake_img_data = ['A', 'B', 'C']
|
||||
glance_iterable.__iter__.return_value = fake_img_data
|
||||
iterable = glanceclient.common.utils.IterableWithLength(
|
||||
iterable=glance_iterable, length=len(fake_img_data))
|
||||
client = mock.MagicMock()
|
||||
client.call.return_value = fake_glance_response(iterable)
|
||||
ctx = mock.sentinel.ctx
|
||||
service = glance.GlanceImageServiceV2(client)
|
||||
res = service.download(ctx, mock.sentinel.image_id)
|
||||
self.assertFalse(show_mock.called)
|
||||
self.assertFalse(open_mock.called)
|
||||
self.assertEqual(iterable, res)
|
||||
self.assertFalse(glance_iterable.close.called)
|
||||
|
||||
@mock.patch('builtins.open')
|
||||
@mock.patch('nova.image.glance.GlanceImageServiceV2.show')
|
||||
def test_download_data_no_dest_path_v2(self, show_mock, open_mock):
|
||||
|
|
@ -709,9 +728,9 @@ class TestDownloadNoDirectUri(test.NoDBTestCase):
|
|||
@mock.patch('nova.image.glance.GlanceImageServiceV2._get_verifier')
|
||||
@mock.patch('nova.image.glance.GlanceImageServiceV2._get_transfer_method')
|
||||
@mock.patch('nova.image.glance.GlanceImageServiceV2.show')
|
||||
def test_download_direct_rbd_uri_v2(
|
||||
def _test_download_direct_rbd_uri_v2(
|
||||
self, show_mock, get_tran_mock, get_verifier_mock, log_mock,
|
||||
open_mock, getsize_mock):
|
||||
open_mock, getsize_mock, with_verifier=True):
|
||||
self.flags(enable_rbd_download=True, group='glance')
|
||||
show_mock.return_value = {
|
||||
'locations': [
|
||||
|
|
@ -734,7 +753,7 @@ class TestDownloadNoDirectUri(test.NoDBTestCase):
|
|||
open_mock.return_value = writer
|
||||
service = glance.GlanceImageServiceV2(client)
|
||||
|
||||
verifier = mock.MagicMock()
|
||||
verifier = mock.MagicMock() if with_verifier else None
|
||||
get_verifier_mock.return_value = verifier
|
||||
|
||||
res = service.download(ctx, mock.sentinel.image_id,
|
||||
|
|
@ -748,34 +767,41 @@ class TestDownloadNoDirectUri(test.NoDBTestCase):
|
|||
tran_mod.assert_called_once_with(ctx, mock.ANY,
|
||||
mock.sentinel.dst_path,
|
||||
mock.sentinel.loc_meta)
|
||||
open_mock.assert_called_once_with(mock.sentinel.dst_path, 'rb')
|
||||
get_tran_mock.assert_called_once_with('rbd')
|
||||
|
||||
# no client call, chunks were read right after xfer_mod.download:
|
||||
client.call.assert_not_called()
|
||||
|
||||
# verifier called with the value we got from rbd download
|
||||
verifier.update.assert_has_calls(
|
||||
if verifier:
|
||||
open_mock.assert_called_once_with(mock.sentinel.dst_path, 'rb')
|
||||
|
||||
# verifier called with the value we got from rbd download
|
||||
verifier.update.assert_has_calls(
|
||||
[
|
||||
mock.call("rbd1"),
|
||||
mock.call("rbd2")
|
||||
]
|
||||
)
|
||||
verifier.verify.assert_called()
|
||||
log_mock.info.assert_has_calls(
|
||||
[
|
||||
mock.call("rbd1"),
|
||||
mock.call("rbd2")
|
||||
mock.call('Successfully transferred using %s', 'rbd'),
|
||||
mock.call(
|
||||
'Image signature verification succeeded for image %s',
|
||||
mock.sentinel.image_id)
|
||||
]
|
||||
)
|
||||
verifier.verify.assert_called()
|
||||
log_mock.info.assert_has_calls(
|
||||
[
|
||||
mock.call('Successfully transferred using %s', 'rbd'),
|
||||
mock.call(
|
||||
'Image signature verification succeeded for image %s',
|
||||
mock.sentinel.image_id)
|
||||
]
|
||||
)
|
||||
)
|
||||
|
||||
# not opened for writing (already written)
|
||||
self.assertFalse(open_mock(mock.sentinel.dst_path, 'rw').called)
|
||||
# write not called (written by rbd download)
|
||||
writer.write.assert_not_called()
|
||||
|
||||
def test_download_direct_rbd_uri_v2(self):
|
||||
self._test_download_direct_rbd_uri_v2()
|
||||
|
||||
def test_download_direct_rbd_uri_without_verifier_v2(self):
|
||||
self._test_download_direct_rbd_uri_v2(with_verifier=False)
|
||||
|
||||
@mock.patch('nova.image.glance.GlanceImageServiceV2._get_transfer_method')
|
||||
@mock.patch('nova.image.glance.GlanceImageServiceV2.show')
|
||||
@mock.patch('nova.image.glance.GlanceImageServiceV2._safe_fsync')
|
||||
|
|
@ -999,7 +1025,7 @@ class TestDownloadSignatureVerification(test.NoDBTestCase):
|
|||
service.download,
|
||||
context=None, image_id=None,
|
||||
data=None, dst_path=None)
|
||||
self.assertEqual(mock_log.error.call_count, 2)
|
||||
self.assertEqual(mock_log.error.call_count, 1)
|
||||
|
||||
@mock.patch('nova.image.glance.LOG')
|
||||
@mock.patch('nova.image.glance.GlanceImageServiceV2.show')
|
||||
|
|
@ -1069,13 +1095,12 @@ class TestDownloadSignatureVerification(test.NoDBTestCase):
|
|||
glanceclient.common.utils.IterableWithLength(
|
||||
iterable=glance_iterable, length=len(self.fake_img_data)))
|
||||
service = glance.GlanceImageServiceV2(self.client)
|
||||
mock_get_verifier.side_effect = \
|
||||
cursive_exception.SignatureVerificationError
|
||||
mock_get_verifier.return_value = self.BadVerifier()
|
||||
mock_dest = mock.MagicMock()
|
||||
mock_open.return_value = mock_dest
|
||||
mock_show.return_value = self.fake_img_props
|
||||
fake_path = 'FAKE_PATH'
|
||||
self.assertRaises(cursive_exception.SignatureVerificationError,
|
||||
self.assertRaises(cryptography.exceptions.InvalidSignature,
|
||||
service.download,
|
||||
context=None, image_id=None,
|
||||
data=None, dst_path=fake_path)
|
||||
|
|
|
|||
Loading…
Reference in New Issue