From d6b339ba343aee91aa3ba2ce277e361958d34273 Mon Sep 17 00:00:00 2001 From: Julia Kreger Date: Thu, 9 Jan 2025 16:02:18 -0800 Subject: [PATCH] Automatic zstd detection and decompression... ... for conductor downloads. The issue is we don't have the underlying library for requests to do Zstandard decompression, but userspace tools are common in linux distributions, and opportunistically we will try to detect, and de-compresse artifacts. Zstandard is popular for compression of artifacts in container registries. Change-Id: I0f6b3b7a8685bb2724505836c770e080bc0e0632 --- bindep.txt | 2 + ironic/common/images.py | 35 ++++++++++- ironic/conf/conductor.py | 9 +++ ironic/tests/unit/common/test_images.py | 60 ++++++++++++++++--- ...c-zstd-decompression-bf30cb99ebbb07f3.yaml | 11 ++++ 5 files changed, 108 insertions(+), 9 deletions(-) create mode 100644 releasenotes/notes/automatic-zstd-decompression-bf30cb99ebbb07f3.yaml diff --git a/bindep.txt b/bindep.txt index 7f3c28f12a..2752e2dc82 100644 --- a/bindep.txt +++ b/bindep.txt @@ -94,3 +94,5 @@ unzip [imagebuild] sudo [imagebuild] gawk [imagebuild] mtools [imagebuild] +# For automatic artifact decompression +zstd [devstack] diff --git a/ironic/common/images.py b/ironic/common/images.py index df321219e8..f3193042c6 100644 --- a/ironic/common/images.py +++ b/ironic/common/images.py @@ -417,6 +417,34 @@ def fetch_into(context, image_href, image_file, return None +def _handle_zstd_compression(path): + zstd_comp = False + with open(path, 'rb') as comp_check: + # Check for zstd compression. Zstd has a variable window for streaming + # clients with transparent connections, and 128 byte blocks. + # Ultimately, requests can't support handling of such content without + # the zstandard library (bsd), but that is not available in global + # requirements. As such, and likely best complexity wise, if we find it + # we can handle it directly. + # https://github.com/facebook/zstd/blob/dev/doc/zstd_compression_format.md + # Ensure we're at the start of the file + comp_check.seek(0) + read = comp_check.read(4) + if read.startswith(b"\x28\xb5\x2f\xfd"): + zstd_comp = True + + if zstd_comp and not CONF.conductor.disable_zstandard_decompression: + temp_path = path + '.zstd' + shutil.move(path, temp_path) + try: + utils.execute('zstd', '-d', '--rm', temp_path) + except OSError as e: + LOG.error('Failed to decompress a zstd compressed file: %s', e) + # Restore the downloaded file... We might want to fail the + # entire process. + shutil.move(temp_path, path) + + def fetch(context, image_href, path, force_raw=False, checksum=None, checksum_algo=None, image_auth_data=None): @@ -428,8 +456,11 @@ def fetch(context, image_href, path, force_raw=False, and checksum): checksum_utils.validate_checksum(path, checksum, checksum_algo) - # FIXME(TheJulia): need to check if we need to extract the file - # i.e. zstd... before forcing raw. + # Check and decompress zstd files, since python-requests realistically + # can't do it for us as-is. Also, some OCI container registry artifacts + # may generally just be zstd compressed, regardless if it is a raw file + # or a qcow2 file. + _handle_zstd_compression(path) if force_raw: image_to_raw(image_href, path, "%s.part" % path) diff --git a/ironic/conf/conductor.py b/ironic/conf/conductor.py index 189cec6060..43aab0fa37 100644 --- a/ironic/conf/conductor.py +++ b/ironic/conf/conductor.py @@ -522,6 +522,15 @@ opts = [ 'functionality by setting this option to True will ' 'create a more secure environment, however it may ' 'break users in an unexpected fashion.')), + cfg.BoolOpt('disable_zstandard_decompression', + default=False, + mutable=False, + help=_('Option to enable disabling transparent decompression ' + 'of files which are compressed with Zstandard ' + 'compression. This option is provided should operators ' + 'wish to disable this functionality, otherwise it is ' + 'automaticly applied by the conductor should a ' + 'compressed artifact be detected.')), ] diff --git a/ironic/tests/unit/common/test_images.py b/ironic/tests/unit/common/test_images.py index 4361233fd9..50a82e72e3 100644 --- a/ironic/tests/unit/common/test_images.py +++ b/ironic/tests/unit/common/test_images.py @@ -42,9 +42,11 @@ class IronicImagesTestCase(base.TestCase): class FakeImgInfo(object): pass + @mock.patch.object(images, '_handle_zstd_compression', autospec=True) @mock.patch.object(image_service, 'get_image_service', autospec=True) @mock.patch.object(builtins, 'open', autospec=True) - def test_fetch_image_service(self, open_mock, image_service_mock): + def test_fetch_image_service(self, open_mock, image_service_mock, + mock_zstd): mock_file_handle = mock.MagicMock(spec=io.BytesIO) mock_file_handle.__enter__.return_value = 'file' open_mock.return_value = mock_file_handle @@ -56,12 +58,15 @@ class IronicImagesTestCase(base.TestCase): context='context') image_service_mock.return_value.download.assert_called_once_with( 'image_href', 'file') + mock_zstd.assert_called_once_with('path') + @mock.patch.object(images, '_handle_zstd_compression', autospec=True) @mock.patch.object(image_service, 'get_image_service', autospec=True) @mock.patch.object(images, 'image_to_raw', autospec=True) @mock.patch.object(builtins, 'open', autospec=True) def test_fetch_image_service_force_raw(self, open_mock, image_to_raw_mock, - image_service_mock): + image_service_mock, + mock_zstd): image_service_mock.return_value.transfer_verified_checksum = None mock_file_handle = mock.MagicMock(spec=io.BytesIO) mock_file_handle.__enter__.return_value = 'file' @@ -74,7 +79,9 @@ class IronicImagesTestCase(base.TestCase): 'image_href', 'file') image_to_raw_mock.assert_called_once_with( 'image_href', 'path', 'path.part') + mock_zstd.assert_called_once_with('path') + @mock.patch.object(images, '_handle_zstd_compression', autospec=True) @mock.patch.object(fileutils, 'compute_file_checksum', autospec=True) @mock.patch.object(image_service, 'get_image_service', autospec=True) @@ -82,7 +89,7 @@ class IronicImagesTestCase(base.TestCase): @mock.patch.object(builtins, 'open', autospec=True) def test_fetch_image_service_force_raw_with_checksum( self, open_mock, image_to_raw_mock, - image_service_mock, mock_checksum): + image_service_mock, mock_checksum, mock_zstd): image_service_mock.return_value.transfer_verified_checksum = None mock_file_handle = mock.MagicMock(spec=io.BytesIO) mock_file_handle.__enter__.return_value = 'file' @@ -98,7 +105,9 @@ class IronicImagesTestCase(base.TestCase): 'image_href', 'file') image_to_raw_mock.assert_called_once_with( 'image_href', 'path', 'path.part') + mock_zstd.assert_called_once_with('path') + @mock.patch.object(images, '_handle_zstd_compression', autospec=True) @mock.patch.object(fileutils, 'compute_file_checksum', autospec=True) @mock.patch.object(image_service, 'get_image_service', autospec=True) @@ -106,7 +115,8 @@ class IronicImagesTestCase(base.TestCase): @mock.patch.object(builtins, 'open', autospec=True) def test_fetch_image_service_with_checksum_mismatch( self, open_mock, image_to_raw_mock, - image_service_mock, mock_checksum): + image_service_mock, mock_checksum, + mock_zstd): image_service_mock.return_value.transfer_verified_checksum = None mock_file_handle = mock.MagicMock(spec=io.BytesIO) mock_file_handle.__enter__.return_value = 'file' @@ -124,7 +134,9 @@ class IronicImagesTestCase(base.TestCase): 'image_href', 'file') # If the checksum fails, then we don't attempt to convert the image. image_to_raw_mock.assert_not_called() + mock_zstd.assert_not_called() + @mock.patch.object(images, '_handle_zstd_compression', autospec=True) @mock.patch.object(fileutils, 'compute_file_checksum', autospec=True) @mock.patch.object(image_service, 'get_image_service', autospec=True) @@ -132,7 +144,8 @@ class IronicImagesTestCase(base.TestCase): @mock.patch.object(builtins, 'open', autospec=True) def test_fetch_image_service_force_raw_no_checksum_algo( self, open_mock, image_to_raw_mock, - image_service_mock, mock_checksum): + image_service_mock, mock_checksum, + mock_zstd): image_service_mock.return_value.transfer_verified_checksum = None mock_file_handle = mock.MagicMock(spec=io.BytesIO) mock_file_handle.__enter__.return_value = 'file' @@ -148,7 +161,9 @@ class IronicImagesTestCase(base.TestCase): 'image_href', 'file') image_to_raw_mock.assert_called_once_with( 'image_href', 'path', 'path.part') + mock_zstd.assert_called_once_with('path') + @mock.patch.object(images, '_handle_zstd_compression', autospec=True) @mock.patch.object(fileutils, 'compute_file_checksum', autospec=True) @mock.patch.object(image_service, 'get_image_service', autospec=True) @@ -156,7 +171,8 @@ class IronicImagesTestCase(base.TestCase): @mock.patch.object(builtins, 'open', autospec=True) def test_fetch_image_service_force_raw_combined_algo( self, open_mock, image_to_raw_mock, - image_service_mock, mock_checksum): + image_service_mock, mock_checksum, + mock_zstd): image_service_mock.return_value.transfer_verified_checksum = None mock_file_handle = mock.MagicMock(spec=io.BytesIO) mock_file_handle.__enter__.return_value = 'file' @@ -172,7 +188,9 @@ class IronicImagesTestCase(base.TestCase): 'image_href', 'file') image_to_raw_mock.assert_called_once_with( 'image_href', 'path', 'path.part') + mock_zstd.assert_called_once_with('path') + @mock.patch.object(images, '_handle_zstd_compression', autospec=True) @mock.patch.object(fileutils, 'compute_file_checksum', autospec=True) @mock.patch.object(image_service, 'get_image_service', autospec=True) @@ -180,7 +198,8 @@ class IronicImagesTestCase(base.TestCase): @mock.patch.object(builtins, 'open', autospec=True) def test_fetch_image_service_auth_data_checksum( self, open_mock, image_to_raw_mock, - svc_mock, mock_checksum): + svc_mock, mock_checksum, + mock_zstd): svc_mock.return_value.transfer_verified_checksum = 'f00' svc_mock.return_value.is_auth_set_needed = True mock_file_handle = mock.MagicMock(spec=io.BytesIO) @@ -201,6 +220,7 @@ class IronicImagesTestCase(base.TestCase): 'image_href', 'path', 'path.part') svc_mock.return_value.set_image_auth.assert_called_once_with( 'image_href', 'meow') + mock_zstd.assert_called_once_with('path') @mock.patch.object(image_format_inspector, 'detect_file_format', autospec=True) @@ -682,6 +702,32 @@ class IronicImagesTestCase(base.TestCase): validate_mock.side_effect = OSError self.assertIsNone(images.is_source_a_path('context', url)) + @mock.patch.object(utils, 'execute', autospec=True) + @mock.patch.object(shutil, 'move', autospec=True) + @mock.patch.object(builtins, 'open', autospec=True) + def test__hanlde_zstd_compression(self, mock_open, mock_move, + mock_exec): + mock_file_handle = mock.Mock() + mock_file_handle.read.return_value = b"\x28\xb5\x2f\xfd" + mock_open.return_value.__enter__.open = mock_file_handle + images._handle_zstd_compression('path') + mock_move.assert_called_once_with('path', 'path.zstd') + mock_exec.assert_called_once_with('zstd', '-d', '--rm', 'path.zstd') + + @mock.patch.object(utils, 'execute', autospec=True) + @mock.patch.object(shutil, 'move', autospec=True) + @mock.patch.object(builtins, 'open', autospec=True) + def test__hanlde_zstd_compression_disabled(self, mock_open, mock_move, + mock_exec): + mock_file_handle = mock.Mock() + mock_file_handle.read.return_value = b"\x28\xb5\x2f\xfd" + mock_open.return_value.__enter__.open = mock_file_handle + CONF.set_override('disable_zstandard_decompression', True, + group='conductor') + images._handle_zstd_compression('path') + mock_move.assert_not_called() + mock_exec.assert_not_called() + class FsImageTestCase(base.TestCase): diff --git a/releasenotes/notes/automatic-zstd-decompression-bf30cb99ebbb07f3.yaml b/releasenotes/notes/automatic-zstd-decompression-bf30cb99ebbb07f3.yaml new file mode 100644 index 0000000000..a393a4794e --- /dev/null +++ b/releasenotes/notes/automatic-zstd-decompression-bf30cb99ebbb07f3.yaml @@ -0,0 +1,11 @@ +--- +features: + - | + Adds the capability for Ironic's conductor to detect Zstandard + compressed content and to automatically decompress the files + to enable image format detection and conversion. + + This is due to use of Zstandard compression upon artifacts stored + in container registries is a popular practice, and can be disabled + using the ``[conductor]disable_zstandard_decompression`` + configuration option.