diff --git a/app/pyproject.toml b/app/pyproject.toml index 9499d32f..601077cd 100644 --- a/app/pyproject.toml +++ b/app/pyproject.toml @@ -3,7 +3,7 @@ [project] name = "github-runner-image-builder" -version = "0.14.1" +version = "0.15.0" authors = [ { name = "Canonical IS DevOps", email = "is-devops-team@canonical.com" }, ] diff --git a/app/src/github_runner_image_builder/errors.py b/app/src/github_runner_image_builder/errors.py index 3edcd0f5..96043e01 100644 --- a/app/src/github_runner_image_builder/errors.py +++ b/app/src/github_runner_image_builder/errors.py @@ -97,6 +97,10 @@ class UploadImageError(OpenstackBaseError): """Represents an error when uploading image to Openstack.""" +class DownloadImageError(OpenstackBaseError): + """Represents an error when downloading image from Openstack.""" + + class OpenstackError(OpenstackBaseError): """Represents an error while communicating with Openstack.""" diff --git a/app/src/github_runner_image_builder/openstack_builder.py b/app/src/github_runner_image_builder/openstack_builder.py index a1fd96b1..4d01a5bb 100644 --- a/app/src/github_runner_image_builder/openstack_builder.py +++ b/app/src/github_runner_image_builder/openstack_builder.py @@ -85,6 +85,9 @@ MIN_RAM = 1024 # M MIN_DISK = 20 # G +TMP_SNAPSHOT_SUFFIX = "-tmp" +HASH_BLOCK_SIZE = 4 * 1024 * 1024 # 4MiB + # We saw an issue with arm noble images with the latest release date, so we are using a fixed date. NOBLE_ARM64_RELEASE_DATE = date(2025, 11, 13) @@ -837,6 +840,68 @@ class _UploadCloudConfig: keep_revisions: int +def _validate_downloaded_snapshot( + conn: openstack.connection.Connection, image_id: str, file_path: pathlib.Path +) -> None: + """Check that the snapshot was downloaded in full and without corruption. + + The snapshot is streamed from Glance and openstacksdk does not verify what it wrote. Without + this check a truncated or corrupted download would be uploaded to the other clouds and + published as a seemingly valid image. + + Args: + conn: The OpenStack connection instance. + image_id: The ID of the snapshot image that was downloaded. + file_path: The path the snapshot was downloaded to. + + Raises: + DownloadImageError: if the downloaded snapshot does not match the snapshot image. + """ + # The image is refetched since the size and the hashes are not populated on the image returned + # when the snapshot is requested. + snapshot = conn.get_image(name_or_id=image_id) + downloaded_size = file_path.stat().st_size + if snapshot.size and downloaded_size != snapshot.size: + raise github_runner_image_builder.errors.DownloadImageError( + f"Incomplete snapshot download, expected {snapshot.size} bytes, " + f"got {downloaded_size} bytes." + ) + algorithm, expected_hash = ( + (snapshot.hash_algo, snapshot.hash_value) + # The algorithm is chosen by the cloud, fall back to the md5 checksum if it is one this + # runtime cannot compute. + if snapshot.hash_value and snapshot.hash_algo in hashlib.algorithms_available + else ("md5", snapshot.checksum) + ) + if not expected_hash: + logger.warning("Snapshot %s exposes no hash, skipping download hash check.", image_id) + return + downloaded_hash = _hash_file(file_path=file_path, algorithm=algorithm) + if downloaded_hash != expected_hash: + raise github_runner_image_builder.errors.DownloadImageError( + f"Corrupt snapshot download, expected {algorithm}: {expected_hash}, " + f"got {downloaded_hash}." + ) + + +def _hash_file(file_path: pathlib.Path, algorithm: str) -> str: + """Compute the hash of a file. + + Args: + file_path: The path of the file to hash. + algorithm: The name of the hash algorithm to use. + + Returns: + The hexadecimal digest of the file. + """ + # The hash is only used to detect a corrupt transfer, not for security purposes. + digest = hashlib.new(algorithm, usedforsecurity=False) + with open(file_path, "rb") as file: + for block in iter(lambda: file.read(HASH_BLOCK_SIZE), b""): + digest.update(block) + return digest.hexdigest() + + def _upload_to_clouds( conn: openstack.connection.Connection, image: openstack.image.v2.image.Image, @@ -857,8 +922,16 @@ def _upload_to_clouds( if not upload_cloud_names: return (image,) file_path = pathlib.Path(f"{image.name}.snapshot") - logger.info("Downloading snapshot to %s.", file_path) - conn.download_image(name_or_id=image.id, output_file=file_path, stream=True) + # The snapshot is downloaded under a temporary name so that a partial download is never + # mistaken for a complete one, mirroring how the images themselves are uploaded. + tmp_file_path = file_path.with_name(f"{file_path.name}{TMP_SNAPSHOT_SUFFIX}") + try: + logger.info("Downloading snapshot to %s.", tmp_file_path) + conn.download_image(name_or_id=image.id, output_file=tmp_file_path, stream=True) + _validate_downloaded_snapshot(conn=conn, image_id=image.id, file_path=tmp_file_path) + tmp_file_path.replace(file_path) + finally: + tmp_file_path.unlink(missing_ok=True) images: list[openstack.image.v2.image.Image] = [] for cloud_name in upload_cloud_names: logger.info("Uploading downloaded snapshot to %s.", cloud_name) diff --git a/app/src/github_runner_image_builder/store.py b/app/src/github_runner_image_builder/store.py index 2674b54b..e8b6e4d4 100644 --- a/app/src/github_runner_image_builder/store.py +++ b/app/src/github_runner_image_builder/store.py @@ -20,6 +20,11 @@ # Timeout constants (in seconds) SNAPSHOT_CREATION_TIMEOUT = 60 * 30 # 30 minutes +TMP_IMAGE_NAME_SUFFIX = "-tmp" +FILE_MD5_PROPERTY = "owner_specified.openstack.md5" +FILE_SHA256_PROPERTY = "owner_specified.openstack.sha256" +ACTIVE_IMAGE_STATUS = "active" +SHA256_ALGORITHM = "sha256" def create_snapshot( @@ -74,19 +79,27 @@ def upload_image( Returns: The created image. """ + tmp_image_name = f"{image_name}{TMP_IMAGE_NAME_SUFFIX}" with openstack.connect(cloud=cloud_name) as connection: try: - logger.info("Uploading image %s.", image_name) + _delete_images_by_name(connection=connection, image_name=tmp_image_name) + + logger.info("Uploading image %s.", tmp_image_name) image_properties = {"architecture": arch.to_openstack()} # ignore type since the library does not provide correct type hinting but the docstring # does define the return type. image: Image = connection.create_image( - name=image_name, + name=tmp_image_name, filename=str(image_path), properties=image_properties, allow_duplicates=True, wait=True, + # Required for the locally computed hashes to be recorded on the image. + validate_checksum=True, ) # type: ignore + _validate_image_checksums(image=image) + logger.info("Renaming image %s to %s.", tmp_image_name, image_name) + image = cast(Image, connection.image.update_image(image, name=image_name)) logger.info("Pruning older images %s, keeping %s.", image_name, keep_revisions) _prune_old_images( connection=connection, image_name=image_name, num_revisions=keep_revisions @@ -96,6 +109,66 @@ def upload_image( except openstack.exceptions.OpenStackCloudException as exc: logger.exception("Error while uploading image.") raise UploadImageError from exc + finally: + # The temporary image is renamed on success, meaning this only has an effect if the + # upload did not complete. + _delete_images_by_name_quietly(connection=connection, image_name=tmp_image_name) + + +def _delete_images_by_name(connection: openstack.connection.Connection, image_name: str) -> None: + """Delete every image matching the given name. + + Args: + connection: The connected openstack cloud instance. + image_name: The exact image name to delete. + """ + for image in connection.image.images(name=image_name): + logger.info("Deleting image %s %s.", image_name, image.id) + connection.delete_image(image.id, wait=True) + + +def _delete_images_by_name_quietly( + connection: openstack.connection.Connection, image_name: str +) -> None: + """Delete every image matching the given name, logging instead of raising on failure. + + Args: + connection: The connected openstack cloud instance. + image_name: The exact image name to delete. + """ + try: + _delete_images_by_name(connection=connection, image_name=image_name) + except openstack.exceptions.OpenStackCloudException: + # Raising here would mask the original error, the leftover image is deleted on the next + # upload instead. + logger.exception("Failed to clean up image %s.", image_name) + + +def _validate_image_checksums(image: Image) -> None: + """Compare the hashes Glance computed against the hashes computed locally during upload. + + Args: + image: The uploaded image. + + Raises: + UploadImageError: If the hashes are missing or do not match. + """ + properties = image.properties or {} + local_md5 = properties.get(FILE_MD5_PROPERTY) + local_sha256 = properties.get(FILE_SHA256_PROPERTY) + if not local_md5 or not local_sha256: + raise UploadImageError(f"Image {image.name} is missing the locally computed hashes.") + if image.checksum != local_md5: + raise UploadImageError( + f"Checksum mismatch for image {image.name}, md5: {image.checksum} != {local_md5}." + ) + # Glance computes the multihash with the algorithm it is configured with, which is not + # necessarily the sha256 that openstacksdk computes locally. + if image.hash_algo == SHA256_ALGORITHM and image.hash_value != local_sha256: + raise UploadImageError( + f"Checksum mismatch for image {image.name}, " + f"sha256: {image.hash_value} != {local_sha256}." + ) def _prune_old_images( @@ -111,7 +184,11 @@ def _prune_old_images( Raises: OpenstackError: if there was an error deleting the images. """ - images = _get_sorted_images_by_created_at(connection=connection, image_name=image_name) + # Images left behind in a non-active status by a failed upload have to be pruned as well, + # otherwise they accumulate forever. + images = _get_sorted_images_by_created_at( + connection=connection, image_name=image_name, active_only=False + ) if not images: return images_to_prune = images[num_revisions:] @@ -140,6 +217,16 @@ def get_latest_build_id(cloud_name: str, image_name: str, active_only: bool = Tr images = _get_sorted_images_by_created_at( connection=connection, image_name=image_name, active_only=active_only ) + if not active_only: + # An upload in progress is still under its temporary name. + images = _sort_images_by_created_at( + images + + _get_sorted_images_by_created_at( + connection=connection, + image_name=f"{image_name}{TMP_IMAGE_NAME_SUFFIX}", + active_only=False, + ) + ) if not images: return "" # The type of ID is in string but the library does not provide correct type hints for it. @@ -155,9 +242,8 @@ def _get_sorted_images_by_created_at( Args: connection: The connected openstack cloud instance. - image_name: The image name to search for. - active_only: If True (default), query only active images via search_images. - If False, query all images regardless of status via the image proxy API. + image_name: The exact image name to search for. + active_only: If True (default), only images that finished uploading are returned. Raises: OpenstackError: if there was an error fetching the images. @@ -166,14 +252,24 @@ def _get_sorted_images_by_created_at( The images sorted by created_at date with latest first. """ try: - if active_only: - images = cast(list[Image], connection.search_images(image_name)) - else: - images = list(connection.image.images(name=image_name)) + images = list(connection.image.images(name=image_name)) except openstack.exceptions.OpenStackCloudException as exc: logger.exception("Failed to search images with name %s.", image_name) raise OpenstackError from exc + if active_only: + images = [image for image in images if image.status == ACTIVE_IMAGE_STATUS] + return _sort_images_by_created_at(images) + + +def _sort_images_by_created_at(images: list[Image]) -> list[Image]: + """Sort images by their creation date, latest first. + + Args: + images: The images to sort. + Returns: + The images sorted by created_at date with latest first. + """ # The type of images are list[Image] but the library does not provide correct type hints for # it. return sorted(images, key=lambda image: image.created_at, reverse=True) # type: ignore diff --git a/app/tests/unit/factories.py b/app/tests/unit/factories.py index a05f22d8..3c2f5cb0 100644 --- a/app/tests/unit/factories.py +++ b/app/tests/unit/factories.py @@ -35,6 +35,14 @@ class Meta: # pylint: disable=too-few-public-methods id: str # UUID created_at = Faker("date") # Example format: 2024-04-16T04:31:12Z + status = "active" + checksum = "test-md5" + hash_algo = "sha256" + hash_value = "test-sha256" + properties = { + "owner_specified.openstack.md5": "test-md5", + "owner_specified.openstack.sha256": "test-sha256", + } class MockRequestsReponseFactory(factory.Factory): diff --git a/app/tests/unit/test_openstack_builder.py b/app/tests/unit/test_openstack_builder.py index 4812a886..6ba3d7f8 100644 --- a/app/tests/unit/test_openstack_builder.py +++ b/app/tests/unit/test_openstack_builder.py @@ -7,6 +7,7 @@ # module. # pylint:disable=protected-access,too-many-lines +import hashlib import pathlib import secrets import typing @@ -23,6 +24,7 @@ from github_runner_image_builder.config import Arch from github_runner_image_builder.errors import ExternalScriptError from github_runner_image_builder.openstack_builder import EXTERNAL_SCRIPT_PATH +from tests.unit.factories import MockOpenstackImageFactory def test_determine_cloud_no_clouds_yaml_error(monkeypatch: pytest.MonkeyPatch): @@ -265,8 +267,9 @@ def test__create_security_group(): ), ], ) -def test_run( +def test_run( # pylint: disable=too-many-locals monkeypatch: pytest.MonkeyPatch, + tmp_path: pathlib.Path, cloud_config: openstack_builder.CloudConfig, with_external_script: bool, ): @@ -275,6 +278,7 @@ def test_run( act: when run is called. assert: all subfunctions are called. """ + monkeypatch.chdir(tmp_path) image_config = openstack_builder.config.ImageConfig( arch=openstack_builder.Arch.X64, base=openstack_builder.BaseImage.JAMMY, @@ -307,6 +311,11 @@ def test_run( monkeypatch.setattr(store, "create_snapshot", create_image_snapshot := MagicMock()) connection_enter_mock = MagicMock() connection_mock = MagicMock() + connection_mock.image.images.return_value = [] + connection_mock.create_image.return_value = MockOpenstackImageFactory(id="test-image-id") + connection_mock.download_image.side_effect = lambda name_or_id, output_file, stream: ( + pathlib.Path(output_file).write_bytes(b"snapshot") + ) connection_enter_mock.__enter__.return_value = connection_mock monkeypatch.setattr( openstack_builder.openstack, @@ -329,6 +338,7 @@ def test_run( monkeypatch.setattr( openstack_builder, "_wait_for_snapshot_complete", (wait_snapshot_mock := MagicMock()) ) + monkeypatch.setattr(openstack_builder, "_validate_downloaded_snapshot", MagicMock()) openstack_builder.run( cloud_config=cloud_config, @@ -1348,3 +1358,92 @@ def test__wait_for_snapshot_complete(monkeypatch: pytest.MonkeyPatch, num_not_ac openstack_builder._wait_for_snapshot_complete(conn=connection_mock, image=MagicMock()) is None ) + + +SNAPSHOT_CONTENT = b"\x00" * 1024 +SNAPSHOT_MD5 = hashlib.md5(SNAPSHOT_CONTENT, usedforsecurity=False).hexdigest() +SNAPSHOT_SHA256 = hashlib.sha256(SNAPSHOT_CONTENT).hexdigest() + + +def _snapshot_file(tmp_path: pathlib.Path, content: bytes = SNAPSHOT_CONTENT) -> pathlib.Path: + """Write a snapshot file. + + Args: + tmp_path: The temporary directory to write to. + content: The contents of the snapshot file. + + Returns: + The path of the written snapshot file. + """ + file_path = tmp_path / "test.snapshot" + file_path.write_bytes(content) + return file_path + + +@pytest.mark.parametrize( + "hash_algo, hash_value, checksum", + [ + pytest.param("sha256", SNAPSHOT_SHA256, "", id="sha256 hash"), + pytest.param("", "", SNAPSHOT_MD5, id="md5 checksum only"), + pytest.param("unknown-algo", "unusable", SNAPSHOT_MD5, id="unsupported hash algorithm"), + pytest.param("", "", "", id="no hash exposed"), + ], +) +def test__validate_downloaded_snapshot( + tmp_path: pathlib.Path, hash_algo: str, hash_value: str, checksum: str +): + """ + arrange: given a downloaded snapshot matching the snapshot image size and hash. + act: when _validate_downloaded_snapshot is called. + assert: no errors are raised. + """ + file_path = _snapshot_file(tmp_path) + connection_mock = MagicMock() + connection_mock.get_image.return_value = MagicMock( + size=len(SNAPSHOT_CONTENT), hash_algo=hash_algo, hash_value=hash_value, checksum=checksum + ) + + assert ( + openstack_builder._validate_downloaded_snapshot( + conn=connection_mock, image_id="test-id", file_path=file_path + ) + is None + ) + + +def test__validate_downloaded_snapshot_truncated(tmp_path: pathlib.Path): + """ + arrange: given a downloaded snapshot that is smaller than the snapshot image. + act: when _validate_downloaded_snapshot is called. + assert: DownloadImageError is raised. + """ + file_path = _snapshot_file(tmp_path, content=b"\x00" * 512) + connection_mock = MagicMock() + connection_mock.get_image.return_value = MagicMock(size=1024) + + with pytest.raises(errors.DownloadImageError) as exc: + openstack_builder._validate_downloaded_snapshot( + conn=connection_mock, image_id="test-id", file_path=file_path + ) + + assert "expected 1024 bytes, got 512 bytes" in str(exc.getrepr()) + + +def test__validate_downloaded_snapshot_corrupt(tmp_path: pathlib.Path): + """ + arrange: given a downloaded snapshot whose hash differs from the snapshot image hash. + act: when _validate_downloaded_snapshot is called. + assert: DownloadImageError is raised. + """ + file_path = _snapshot_file(tmp_path, content=b"\x01" * 1024) + connection_mock = MagicMock() + connection_mock.get_image.return_value = MagicMock( + size=1024, hash_algo="sha256", hash_value=SNAPSHOT_SHA256 + ) + + with pytest.raises(errors.DownloadImageError) as exc: + openstack_builder._validate_downloaded_snapshot( + conn=connection_mock, image_id="test-id", file_path=file_path + ) + + assert "Corrupt snapshot download" in str(exc.getrepr()) diff --git a/app/tests/unit/test_store.py b/app/tests/unit/test_store.py index 66c4c6b3..8120010d 100644 --- a/app/tests/unit/test_store.py +++ b/app/tests/unit/test_store.py @@ -6,7 +6,7 @@ # Need access to protected functions for testing # pylint:disable=protected-access -from unittest.mock import MagicMock +from unittest.mock import MagicMock, call import pytest from openstack.connection import Connection @@ -71,7 +71,7 @@ def test__get_sorted_images_by_created_at_error(mock_connection: MagicMock): act: when _get_sorted_images_by_created_at is called. assert: the images are returned in sorted order by creation date. """ - mock_connection.search_images.side_effect = openstack.exceptions.OpenStackCloudException( + mock_connection.image.images.side_effect = openstack.exceptions.OpenStackCloudException( "Network error" ) @@ -87,7 +87,7 @@ def test__get_sorted_images_by_created_at(mock_connection: MagicMock): act: when _get_sorted_images_by_created_at is called. assert: the images are returned in sorted order by creation date. """ - mock_connection.search_images.return_value = [ + mock_connection.image.images.return_value = [ (first := MockOpenstackImageFactory(id="1", created_at="2024-01-01T00:00:00Z")), (third := MockOpenstackImageFactory(id="3", created_at="2024-03-03T00:00:00Z")), (second := MockOpenstackImageFactory(id="2", created_at="2024-02-02T00:00:00Z")), @@ -98,28 +98,50 @@ def test__get_sorted_images_by_created_at(mock_connection: MagicMock): ) == [third, second, first] -def test__get_sorted_images_by_created_at_any_status(mock_connection: MagicMock): +def test_get_latest_build_id_any_status(mock_connection: MagicMock): """ arrange: given a mocked openstack connection returning images via image proxy. - act: when _get_sorted_images_by_created_at is called with active_only=False. - assert: connection.image.images is used (not search_images) and result is sorted. + act: when get_latest_build_id is called with active_only=False. + assert: images under both the final and the temporary name are considered. """ mock_connection.image = MagicMock() - mock_connection.image.images.return_value = iter( - [ - (first := MockOpenstackImageFactory(id="1", created_at="2024-01-01T00:00:00Z")), - (third := MockOpenstackImageFactory(id="3", created_at="2024-03-03T00:00:00Z")), - (second := MockOpenstackImageFactory(id="2", created_at="2024-02-02T00:00:00Z")), - ] + first = MockOpenstackImageFactory(id="1", created_at="2024-01-01T00:00:00Z") + third = MockOpenstackImageFactory(id="3", created_at="2024-03-03T00:00:00Z") + second = MockOpenstackImageFactory(id="2", created_at="2024-02-02T00:00:00Z") + in_progress = MockOpenstackImageFactory( + id="4", created_at="2024-04-04T00:00:00Z", status="saving" ) + mock_connection.image.images.side_effect = [ + iter([first, third, second]), + iter([in_progress]), + ] - result = store._get_sorted_images_by_created_at( - connection=mock_connection, image_name="test-image", active_only=False + result = store.get_latest_build_id( + cloud_name=MagicMock(), image_name="test-image", active_only=False ) - mock_connection.image.images.assert_called_once_with(name="test-image") - mock_connection.search_images.assert_not_called() - assert result == [third, second, first] + assert mock_connection.image.images.call_args_list == [ + call(name="test-image"), + call(name=f"test-image{store.TMP_IMAGE_NAME_SUFFIX}"), + ] + assert result == "4" + + +def test_get_latest_build_id_active_only_ignores_tmp(mock_connection: MagicMock): + """ + arrange: given an in-progress upload that is newer than the latest active image. + act: when get_latest_build_id is called with the default active_only. + assert: only the active image is returned and the temporary name is not queried. + """ + mock_connection.image = MagicMock() + active = MockOpenstackImageFactory(id="1", created_at="2024-01-01T00:00:00Z") + saving = MockOpenstackImageFactory(id="2", created_at="2024-04-04T00:00:00Z", status="saving") + mock_connection.image.images.side_effect = [iter([active, saving])] + + result = store.get_latest_build_id(cloud_name=MagicMock(), image_name="test-image") + + assert mock_connection.image.images.call_args_list == [call(name="test-image")] + assert result == "1" def test__get_sorted_images_by_created_at_any_status_error(mock_connection: MagicMock): @@ -145,7 +167,7 @@ def test__prune_old_images_error(mock_connection: MagicMock): act: when _prune_old_images is called. assert: failure to delete is logged. """ - mock_connection.search_images.return_value = [ + mock_connection.image.images.return_value = [ MockOpenstackImageFactory(id="1", created_at="2024-01-01T00:00:00Z"), MockOpenstackImageFactory(id="2", created_at="2024-02-02T00:00:00Z"), ] @@ -165,7 +187,7 @@ def test__prune_old_images_fail(mock_connection: MagicMock): act: when _prune_old_images is called. assert: failure to delete is logged. """ - mock_connection.search_images.return_value = [ + mock_connection.image.images.return_value = [ MockOpenstackImageFactory(id="1", created_at="2024-01-01T00:00:00Z"), MockOpenstackImageFactory(id="2", created_at="2024-02-02T00:00:00Z"), ] @@ -183,7 +205,7 @@ def test__prune_old_images(mock_connection: MagicMock): act: when _prune_old_images is called. assert: delete mock is called. """ - mock_connection.search_images.return_value = [ + mock_connection.image.images.return_value = [ MockOpenstackImageFactory(id="1", created_at="2024-01-01T00:00:00Z"), MockOpenstackImageFactory(id="2", created_at="2024-02-02T00:00:00Z"), ] @@ -218,22 +240,199 @@ def test_upload_image_error(mock_connection: MagicMock): def test_upload_image(mock_connection: MagicMock): """ - arrange: given a mocked openstack create_image function that raises an exception. + arrange: given a mocked openstack create_image function that uploads successfully. act: when upload_image is called. - assert: UploadImageError is raised. + assert: the image is uploaded under a temporary name and the renamed image is returned. """ - mock_connection.create_image.return_value = (test_image := MockOpenstackImageFactory(id="1")) + mock_connection.image.images.return_value = [] + mock_connection.create_image.return_value = MockOpenstackImageFactory(id="1") + mock_connection.image.update_image.return_value = ( + renamed_image := MockOpenstackImageFactory(id="1") + ) assert ( store.upload_image( arch=MagicMock(), cloud_name=MagicMock(), - image_name=MagicMock(), + image_name="test-image", image_path=MagicMock(), keep_revisions=MagicMock(), ) - == test_image + == renamed_image + ) + assert ( + mock_connection.create_image.call_args.kwargs["name"] + == f"test-image{store.TMP_IMAGE_NAME_SUFFIX}" + ) + assert mock_connection.create_image.call_args.kwargs["validate_checksum"] is True + assert mock_connection.image.update_image.call_args.kwargs["name"] == "test-image" + + +def test_upload_image_deletes_leftover_tmp_image(mock_connection: MagicMock): + """ + arrange: given a leftover temporary image from a previously interrupted upload. + act: when upload_image is called. + assert: the leftover image is deleted before the upload starts. + """ + mock_connection.image.images.side_effect = [[MockOpenstackImageFactory(id="stale")], [], []] + mock_connection.create_image.return_value = MockOpenstackImageFactory(id="1") + mock_connection.image.update_image.return_value = MockOpenstackImageFactory(id="1") + + store.upload_image( + arch=MagicMock(), + cloud_name=MagicMock(), + image_name="test-image", + image_path=MagicMock(), + keep_revisions=MagicMock(), + ) + + assert ( + mock_connection.image.images.call_args_list[0].kwargs["name"] + == f"test-image{store.TMP_IMAGE_NAME_SUFFIX}" ) + mock_connection.delete_image.assert_any_call("stale", wait=True) + + +def test_upload_image_non_sha256_multihash(mock_connection: MagicMock): + """ + arrange: given a cloud whose Glance computes the multihash with an algorithm other than sha256. + act: when upload_image is called. + assert: the image is accepted based on the md5 checksum alone. + """ + mock_connection.image.images.return_value = [] + mock_connection.create_image.return_value = MockOpenstackImageFactory( + id="1", hash_algo="sha512", hash_value="a-sha512-digest" + ) + mock_connection.image.update_image.return_value = MockOpenstackImageFactory(id="1") + + store.upload_image( + arch=MagicMock(), + cloud_name=MagicMock(), + image_name="test-image", + image_path=MagicMock(), + keep_revisions=MagicMock(), + ) + + mock_connection.image.update_image.assert_called_once() + + +def test_upload_image_checksum_mismatch_error(mock_connection: MagicMock): + """ + arrange: given an uploaded image whose Glance hashes differ from the local ones. + act: when upload_image is called. + assert: UploadImageError is raised and the image is not renamed. + """ + mock_connection.image.images.return_value = [] + mock_connection.create_image.return_value = MockOpenstackImageFactory( + id="1", checksum="corrupted-md5" + ) + + with pytest.raises(UploadImageError) as exc: + store.upload_image( + arch=MagicMock(), + cloud_name=MagicMock(), + image_name="test-image", + image_path=MagicMock(), + keep_revisions=MagicMock(), + ) + + assert "Checksum mismatch" in str(exc.getrepr()) + mock_connection.image.update_image.assert_not_called() + + +def test_upload_image_sha256_mismatch_error(mock_connection: MagicMock): + """ + arrange: given an uploaded image whose Glance sha256 differs from the local one. + act: when upload_image is called. + assert: UploadImageError is raised and the image is not renamed. + """ + mock_connection.image.images.return_value = [] + mock_connection.create_image.return_value = MockOpenstackImageFactory( + id="1", hash_algo="sha256", hash_value="corrupted-sha256" + ) + + with pytest.raises(UploadImageError) as exc: + store.upload_image( + arch=MagicMock(), + cloud_name=MagicMock(), + image_name="test-image", + image_path=MagicMock(), + keep_revisions=MagicMock(), + ) + + assert "sha256: corrupted-sha256 != test-sha256" in str(exc.getrepr()) + mock_connection.image.update_image.assert_not_called() + + +def test_upload_image_missing_checksum_error(mock_connection: MagicMock): + """ + arrange: given an uploaded image without the locally computed hashes. + act: when upload_image is called. + assert: UploadImageError is raised and the image is not renamed. + """ + mock_connection.image.images.return_value = [] + mock_connection.create_image.return_value = MockOpenstackImageFactory(id="1", properties={}) + + with pytest.raises(UploadImageError) as exc: + store.upload_image( + arch=MagicMock(), + cloud_name=MagicMock(), + image_name="test-image", + image_path=MagicMock(), + keep_revisions=MagicMock(), + ) + + assert "missing the locally computed hashes" in str(exc.getrepr()) + mock_connection.image.update_image.assert_not_called() + + +def test_upload_image_deletes_tmp_image_on_error(mock_connection: MagicMock): + """ + arrange: given an uploaded image that fails the checksum validation. + act: when upload_image is called. + assert: the temporary image is deleted. + """ + mock_connection.image.images.side_effect = [ + [], + [MockOpenstackImageFactory(id="1", name="test-image-tmp")], + ] + mock_connection.create_image.return_value = MockOpenstackImageFactory(id="1", properties={}) + + with pytest.raises(UploadImageError): + store.upload_image( + arch=MagicMock(), + cloud_name=MagicMock(), + image_name="test-image", + image_path=MagicMock(), + keep_revisions=MagicMock(), + ) + + mock_connection.delete_image.assert_called_once_with("1", wait=True) + + +def test_upload_image_tmp_image_cleanup_error(mock_connection: MagicMock): + """ + arrange: given a failed upload whose temporary image cannot be deleted. + act: when upload_image is called. + assert: the original error is raised instead of the cleanup error. + """ + mock_connection.image.images.side_effect = [ + [], + [MockOpenstackImageFactory(id="1", name="test-image-tmp")], + ] + mock_connection.create_image.return_value = MockOpenstackImageFactory(id="1", properties={}) + mock_connection.delete_image.side_effect = openstack.exceptions.SDKException("delete failed") + + with pytest.raises(UploadImageError) as exc: + store.upload_image( + arch=MagicMock(), + cloud_name=MagicMock(), + image_name="test-image", + image_path=MagicMock(), + keep_revisions=MagicMock(), + ) + + assert "missing the locally computed hashes" in str(exc.getrepr()) @pytest.mark.usefixtures("mock_connection") diff --git a/docs/changelog.md b/docs/changelog.md index 4cb5f607..c8c55a66 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -1,5 +1,12 @@ +## Verify image checksums when uploading and downloading images + +- Verify the integrity of an image after uploading it to OpenStack Glance, so that a failed or truncated upload can no longer be published as a usable image. +- Upload images under a temporary name and rename them only once verified, so consumers never see an image that is still uploading. +- Verify the size and hash of a snapshot downloaded from the build cloud before uploading it to the other clouds. +- Only report images that finished uploading when querying the latest image. + ## [#223 Fix GARM image incompatibility](https://github.com/canonical/github-runner-image-builder-operator/pull/223) (2026-05-27) - Add `runner` user as an alias to the `ubuntu` user (same UID/GID, same home directory) so GARM can boot runners from images produced by this charm.