From 52f02bc6233c1b9ceb7b36f126ccc84a20c00c3c Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 12 Dec 2024 11:34:00 -0500 Subject: [PATCH 1/6] Initial refactor of image vault utils --- include/multipass/file_ops.h | 3 + include/multipass/vm_image_vault.h | 8 - include/multipass/vm_image_vault_utils.h | 87 +++++++++ include/multipass/xz_image_decoder.h | 5 +- src/daemon/default_vm_image_vault.cpp | 6 +- .../backends/lxd/lxd_vm_image_vault.cpp | 4 +- src/utils/file_ops.cpp | 11 ++ src/utils/utils.cpp | 10 - src/utils/vm_image_vault_utils.cpp | 73 ++------ src/xz_decoder/xz_image_decoder.cpp | 8 +- tests/CMakeLists.txt | 1 + tests/mock_file_ops.h | 3 + tests/mock_image_decoder.h | 37 ++++ tests/mock_image_vault_utils.h | 42 +++++ tests/test_file_ops.cpp | 7 + tests/test_image_vault_utils.cpp | 173 ++++++++++++++++++ tests/test_utils.cpp | 54 ------ 17 files changed, 390 insertions(+), 142 deletions(-) create mode 100644 include/multipass/vm_image_vault_utils.h create mode 100644 tests/mock_image_decoder.h create mode 100644 tests/mock_image_vault_utils.h create mode 100644 tests/test_image_vault_utils.cpp diff --git a/include/multipass/file_ops.h b/include/multipass/file_ops.h index fef2aeb5bb..94661c75ab 100644 --- a/include/multipass/file_ops.h +++ b/include/multipass/file_ops.h @@ -87,6 +87,9 @@ class FileOps : public Singleton virtual qint64 write(QFileDevice& file, const QByteArray& data) const; virtual bool flush(QFile& file) const; + virtual QString remove_extension(const QString& path) const; + virtual bool copy(const QString& from, const QString& to) const; + // QSaveFile operations virtual bool commit(QSaveFile& file) const; diff --git a/include/multipass/vm_image_vault.h b/include/multipass/vm_image_vault.h index d458880b61..9e64d6abaf 100644 --- a/include/multipass/vm_image_vault.h +++ b/include/multipass/vm_image_vault.h @@ -42,14 +42,6 @@ namespace multipass class VMImageHost; namespace vault { -// Helper functions and classes for all image vault types -QString filename_for(const Path& path); -QString copy(const QString& file_name, const QDir& output_dir); -void delete_file(const Path& path); -QString compute_image_hash(const Path& image_path); -void verify_image_download(const Path& image_path, const QString& image_hash); -QString extract_image(const Path& image_path, const ProgressMonitor& monitor, const bool delete_file = false); -std::unordered_map configure_image_host_map(const std::vector& image_hosts); class DeleteOnException { diff --git a/include/multipass/vm_image_vault_utils.h b/include/multipass/vm_image_vault_utils.h new file mode 100644 index 0000000000..5da2f77927 --- /dev/null +++ b/include/multipass/vm_image_vault_utils.h @@ -0,0 +1,87 @@ +/* + * Copyright (C) Canonical, Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +#ifndef MULTIPASS_VM_IMAGE_VAULT_UTILS_H +#define MULTIPASS_VM_IMAGE_VAULT_UTILS_H + +#include "file_ops.h" +#include "xz_image_decoder.h" + +#include +#include +#include + +#define MP_IMAGE_VAULT_UTILS \ + multipass::ImageVaultUtils \ + { \ + } + +namespace multipass +{ +class VMImageHost; + +class ImageVaultUtils +{ +public: + using DefaultDecoder = XzImageDecoder; + + static QString copy_to_dir(const QString& file, const QDir& output_dir); + static QString compute_hash(QIODevice& device); + static QString compute_file_hash(const QString& file); + + template + static void verify_file_hash(const QString& file, const QString& hash, const Hasher& hasher = ImageVaultUtils{}); + + template + static QString extract_file(const QString& file, + const ProgressMonitor& monitor, + bool delete_original = false, + const Decoder& decoder = DefaultDecoder{}); + + using HostMap = std::unordered_map; + using Hosts = std::vector; + + static HostMap configure_image_host_map(const Hosts& image_hosts); +}; + +template +void ImageVaultUtils::verify_file_hash(const QString& file, const QString& hash, const Hasher& hasher) +{ + auto file_hash = hasher.compute_file_hash(file); +} + +template +QString ImageVaultUtils::extract_file(const QString& file, + const ProgressMonitor& monitor, + bool delete_original, + const Decoder& decoder) +{ + auto new_path = MP_FILEOPS.remove_extension(file); + decoder.decode_to(file, new_path, monitor); + + if (delete_original) + { + QFile qfile{file}; + MP_FILEOPS.remove(qfile); + } + + return new_path; +} + +} // namespace multipass + +#endif // MULTIPASS_VM_IMAGE_VAULT_UTILS_H diff --git a/include/multipass/xz_image_decoder.h b/include/multipass/xz_image_decoder.h index 598159f9be..92de3a8250 100644 --- a/include/multipass/xz_image_decoder.h +++ b/include/multipass/xz_image_decoder.h @@ -32,14 +32,13 @@ namespace multipass class XzImageDecoder { public: - XzImageDecoder(const Path& xz_file_path); + XzImageDecoder(); - void decode_to(const Path& decoded_file_path, const ProgressMonitor& monitor); + void decode_to(const Path& xz_file_path, const Path& decoded_file_path, const ProgressMonitor& monitor) const; using XzDecoderUPtr = std::unique_ptr; private: - QFile xz_file; XzDecoderUPtr xz_decoder; }; } // namespace multipass diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index 42f1d00915..3236a4c23b 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -41,6 +41,7 @@ #include #include +#include namespace mp = multipass; namespace mpl = multipass::logging; @@ -170,7 +171,10 @@ void remove_source_images(const mp::VMImage& source_image, const mp::VMImage& pr { // The prepare phase may have been a no-op, check and only remove source images if (source_image.image_path != prepared_image.image_path) - mp::vault::delete_file(source_image.image_path); + { + QFile source_file{source_image.image_path}; + MP_FILEOPS.remove(source_file); + } } void delete_image_dir(const mp::Path& image_path) diff --git a/src/platform/backends/lxd/lxd_vm_image_vault.cpp b/src/platform/backends/lxd/lxd_vm_image_vault.cpp index b2b7cd05af..72d4e23479 100644 --- a/src/platform/backends/lxd/lxd_vm_image_vault.cpp +++ b/src/platform/backends/lxd/lxd_vm_image_vault.cpp @@ -47,6 +47,7 @@ #include #include +#include #include namespace mp = multipass; @@ -90,7 +91,8 @@ QString post_process_downloaded_image(const QString& image_path, const mp::Progr if (original_image_path != new_image_path) { - mp::vault::delete_file(original_image_path); + QFile original_file{original_image_path}; + MP_FILEOPS.remove(original_file); } return new_image_path; diff --git a/src/utils/file_ops.cpp b/src/utils/file_ops.cpp index 1240ab75ff..7e403f52c3 100644 --- a/src/utils/file_ops.cpp +++ b/src/utils/file_ops.cpp @@ -170,6 +170,17 @@ bool mp::FileOps::flush(QFile& file) const return file.flush(); } +QString mp::FileOps::remove_extension(const QString& path) const +{ + QFileInfo info{path}; + return info.dir().path() + info.completeBaseName(); +} + +bool mp::FileOps::copy(const QString& from, const QString& to) const +{ + return QFile::copy(from, to); +} + bool mp::FileOps::commit(QSaveFile& file) const { return file.commit(); diff --git a/src/utils/utils.cpp b/src/utils/utils.cpp index 5dacfd8dc7..8198543d14 100644 --- a/src/utils/utils.cpp +++ b/src/utils/utils.cpp @@ -406,16 +406,6 @@ void mp::utils::validate_server_address(const std::string& address) throw std::runtime_error(fmt::format("invalid port number in address '{}'", address)); } -std::string mp::utils::filename_for(const std::string& path) -{ - return QFileInfo(QString::fromStdString(path)).fileName().toStdString(); -} - -bool mp::utils::is_dir(const std::string& path) -{ - return QFileInfo(QString::fromStdString(path)).isDir(); -} - std::string mp::utils::match_line_for(const std::string& output, const std::string& matcher) { std::istringstream ss{output}; diff --git a/src/utils/vm_image_vault_utils.cpp b/src/utils/vm_image_vault_utils.cpp index ce5f09deec..e24c214398 100644 --- a/src/utils/vm_image_vault_utils.cpp +++ b/src/utils/vm_image_vault_utils.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -27,77 +28,25 @@ namespace mp = multipass; -QString mp::vault::filename_for(const mp::Path& path) +QString mp::ImageVaultUtils::copy_to_dir(const QString& file, const QDir& output_dir) { - QFileInfo file_info(path); - return file_info.fileName(); + return ""; } -QString mp::vault::copy(const QString& file_name, const QDir& output_dir) +QString mp::ImageVaultUtils::compute_hash(QIODevice& device) { - if (file_name.isEmpty()) - return {}; - - if (!QFileInfo::exists(file_name)) - throw std::runtime_error(fmt::format("{} missing", file_name)); - - QFileInfo info{file_name}; - const auto source_name = info.fileName(); - auto new_path = output_dir.filePath(source_name); - QFile::copy(file_name, new_path); - return new_path; -} - -void mp::vault::delete_file(const mp::Path& path) -{ - QFile file{path}; - file.remove(); -} - -QString mp::vault::compute_image_hash(const mp::Path& image_path) -{ - QFile image_file(image_path); - if (!image_file.open(QFile::ReadOnly)) - { - throw std::runtime_error("Cannot open image file for computing hash"); - } - - QCryptographicHash hash(QCryptographicHash::Sha256); - if (!hash.addData(&image_file)) - { - throw std::runtime_error("Cannot read image file to compute hash"); - } - - return hash.result().toHex(); -} - -void mp::vault::verify_image_download(const mp::Path& image_path, const QString& image_hash) -{ - auto computed_hash = compute_image_hash(image_path); - - if (computed_hash != image_hash) - { - throw std::runtime_error("Downloaded image hash does not match"); - } + return ""; } -QString mp::vault::extract_image(const mp::Path& image_path, const mp::ProgressMonitor& monitor, const bool delete_file) +QString mp::ImageVaultUtils::compute_file_hash(const QString& path) { - mp::XzImageDecoder xz_decoder(image_path); - QString new_image_path{image_path}; - - new_image_path.remove(".xz"); - - xz_decoder.decode_to(new_image_path, monitor); - - mp::vault::delete_file(image_path); - - return new_image_path; + return ""; } -std::unordered_map -mp::vault::configure_image_host_map(const std::vector& image_hosts) +mp::ImageVaultUtils::HostMap mp::ImageVaultUtils::configure_image_host_map(const Hosts& image_hosts) { + return {}; + /* std::unordered_map remote_image_host_map; for (const auto& image_host : image_hosts) @@ -108,5 +57,5 @@ mp::vault::configure_image_host_map(const std::vector& image_h } } - return remote_image_host_map; + return remote_image_host_map;*/ } diff --git a/src/xz_decoder/xz_image_decoder.cpp b/src/xz_decoder/xz_image_decoder.cpp index d6da42abef..1785f116ab 100644 --- a/src/xz_decoder/xz_image_decoder.cpp +++ b/src/xz_decoder/xz_image_decoder.cpp @@ -54,15 +54,17 @@ bool verify_decode(const xz_ret& ret) } } // namespace -mp::XzImageDecoder::XzImageDecoder(const Path& xz_file_path) - : xz_file{xz_file_path}, xz_decoder{xz_dec_init(XZ_DYNALLOC, 1u << 26), xz_dec_end} +mp::XzImageDecoder::XzImageDecoder() : xz_decoder{xz_dec_init(XZ_DYNALLOC, 1u << 26), xz_dec_end} { xz_crc32_init(); xz_crc64_init(); } -void mp::XzImageDecoder::decode_to(const Path& decoded_image_path, const ProgressMonitor& monitor) +void mp::XzImageDecoder::decode_to(const Path& xz_file_path, + const Path& decoded_image_path, + const ProgressMonitor& monitor) const { + QFile xz_file{xz_file_path}; if (!xz_file.open(QIODevice::ReadOnly)) throw std::runtime_error(fmt::format("failed to open {} for reading", xz_file.fileName())); diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index b201a931ae..1d5a116bf9 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -124,6 +124,7 @@ add_executable(multipass_tests test_sftp_utils.cpp test_file_ops.cpp test_recursive_dir_iter.cpp + test_image_vault_utils.cpp ) target_include_directories(multipass_tests diff --git a/tests/mock_file_ops.h b/tests/mock_file_ops.h index cf257f23ea..3afba6601e 100644 --- a/tests/mock_file_ops.h +++ b/tests/mock_file_ops.h @@ -62,6 +62,9 @@ class MockFileOps : public FileOps MOCK_METHOD(qint64, write, (QFileDevice&, const QByteArray&), (const, override)); MOCK_METHOD(bool, flush, (QFile & file), (const, override)); + MOCK_METHOD(QString, remove_extension, (const QString& path), (const, override)); + MOCK_METHOD(bool, copy, (const QString&, const QString&), (const, override)); + // QSaveFile mock methods MOCK_METHOD(bool, commit, (QSaveFile&), (const, override)); diff --git a/tests/mock_image_decoder.h b/tests/mock_image_decoder.h new file mode 100644 index 0000000000..f7083ebe84 --- /dev/null +++ b/tests/mock_image_decoder.h @@ -0,0 +1,37 @@ +/* + * Copyright (C) Canonical, Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +#ifndef MULTIPASS_MOCK_IMAGE_DECODER_H +#define MULTIPASS_MOCK_IMAGE_DECODER_H + +#include "common.h" + +#include +#include + +namespace multipass::test +{ + +class MockImageDecoder +{ +public: + MOCK_METHOD(void, decode_to, (const Path&, const Path&, const ProgressMonitor&), (const)); +}; + +} // namespace multipass::test + +#endif // MULTIPASS_MOCK_IMAGE_DECODER_H diff --git a/tests/mock_image_vault_utils.h b/tests/mock_image_vault_utils.h new file mode 100644 index 0000000000..331ae51d5e --- /dev/null +++ b/tests/mock_image_vault_utils.h @@ -0,0 +1,42 @@ +/* + * Copyright (C) Canonical, Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +#ifndef MULTIPASS_MOCK_IMAGE_VALUE_UTILS_H +#define MULTIPASS_MOCK_IMAGE_VALUE_UTILS_H + +#include "common.h" +#include "mock_image_decoder.h" + +#include + +namespace multipass::test +{ + +class MockImageVaultUtils +{ +public: + MOCK_METHOD(QString, copy_to_dir, (const QString&, const QDir&), (const)); + MOCK_METHOD(QString, compute_hash, (QIODevice&), (const)); + MOCK_METHOD(QString, compute_file_hash, (const QString&), (const)); + MOCK_METHOD(void, verify_file_hash, (const QString&, const QString&), (const)); + + MOCK_METHOD(void, extract_file, (const QString&, const ProgressMonitor&, bool, const MockImageDecoder&), (const)); +}; + +} // namespace multipass::test + +#endif // MULTIPASS_MOCK_IMAGE_VALUE_UTILS_H diff --git a/tests/test_file_ops.cpp b/tests/test_file_ops.cpp index f8a54caac2..fcc6b84bb0 100644 --- a/tests/test_file_ops.cpp +++ b/tests/test_file_ops.cpp @@ -192,3 +192,10 @@ TEST_F(FileOps, posix_lseek) EXPECT_EQ(r, file_content.size() - seek); EXPECT_STREQ(buffer.data(), file_content.c_str() + seek); } + +TEST_F(FileOps, remove_extension) +{ + EXPECT_EQ(MP_FILEOPS.remove_extension("test.txt"), "test"); + EXPECT_EQ(MP_FILEOPS.remove_extension("tests/test.test.txt"), "tests/test.test"); + EXPECT_EQ(MP_FILEOPS.remove_extension("/sets/test.png"), "/sets/test.png"); +} diff --git a/tests/test_image_vault_utils.cpp b/tests/test_image_vault_utils.cpp new file mode 100644 index 0000000000..5062358e10 --- /dev/null +++ b/tests/test_image_vault_utils.cpp @@ -0,0 +1,173 @@ +/* + * Copyright (C) Canonical, Ltd. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; version 3. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + * + */ + +#include "common.h" +#include "mock_file_ops.h" +#include "mock_image_decoder.h" +#include "mock_image_vault_utils.h" + +#include +#include +#include + +namespace mp = multipass; +namespace mpt = multipass::test; + +using namespace testing; + +namespace +{ + +struct TestImageVaultUtils : public ::testing::Test +{ + const mpt::MockFileOps::GuardedMock mock_file_ops_guard{StrictMock::inject()}; + mpt::MockFileOps& mock_file_ops{*mock_file_ops_guard.first}; + + const QDir test_dir{"secrets/secret_filled_folder"}; + const QString test_path{"not_secrets/a_secret.txt"}; + QFile test_file{test_path}; + const QFileInfo test_info{test_path}; + + const QString test_output{"secrets/secret_filled_folder/a_secret.txt"}; +}; + +TEST_F(TestImageVaultUtils, copy_to_dir_handles_empty_file) +{ + EXPECT_EQ(MP_IMAGE_VAULT_UTILS.copy_to_dir("", test_dir), ""); +} + +TEST_F(TestImageVaultUtils, copy_to_dir_throws_on_nonexistant_file) +{ + EXPECT_CALL(mock_file_ops, exists(test_info)).WillOnce(Return(false)); + + MP_EXPECT_THROW_THAT(MP_IMAGE_VAULT_UTILS.copy_to_dir(test_path, test_dir), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr(test_path.toStdString()), HasSubstr("not found")))); +} + +TEST_F(TestImageVaultUtils, copy_to_dir_throws_on_fail_to_copy) +{ + EXPECT_CALL(mock_file_ops, exists(test_info)).WillOnce(Return(true)); + EXPECT_CALL(mock_file_ops, copy(test_path, test_output)).WillOnce(Return(false)); + + MP_EXPECT_THROW_THAT(MP_IMAGE_VAULT_UTILS.copy_to_dir(test_path, test_dir), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr(test_path.toStdString()), + HasSubstr("Failed to copy"), + HasSubstr(test_output.toStdString())))); +} + +TEST_F(TestImageVaultUtils, copy_to_dir_copys_to_dir) +{ + EXPECT_CALL(mock_file_ops, exists(test_info)).WillOnce(Return(true)); + EXPECT_CALL(mock_file_ops, copy(test_path, test_output)).WillOnce(Return(true)); + + auto result = MP_IMAGE_VAULT_UTILS.copy_to_dir(test_path, test_dir); + EXPECT_EQ(result, test_output); +} + +TEST_F(TestImageVaultUtils, compute_hash_throws_when_cant_read) +{ + QBuffer buffer{}; // note: buffer is not opened + MP_EXPECT_THROW_THAT(MP_IMAGE_VAULT_UTILS.compute_hash(buffer), + std::runtime_error, + mpt::match_what(HasSubstr("Failed to read"))); +} + +TEST_F(TestImageVaultUtils, compute_hash_computes_sha256) +{ + QByteArray data = ":)"; + QBuffer buffer{&data}; + + buffer.open(QIODevice::ReadOnly); + + auto hash = MP_IMAGE_VAULT_UTILS.compute_hash(buffer); + EXPECT_EQ(hash, "54d626e08c1c802b305dad30b7e54a82f102390cc92c7d4db112048935236e9c"); +} + +TEST_F(TestImageVaultUtils, compute_file_hash_throws_when_cant_open) +{ + EXPECT_CALL(mock_file_ops, open(Property(&QFileDevice::fileName, test_path), Truly([](const auto& mode) { + return (mode & QFile::ReadOnly) > 0; + }))) + .WillOnce(Return(false)); + + MP_EXPECT_THROW_THAT(MP_IMAGE_VAULT_UTILS.compute_file_hash(test_path), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr(test_path.toStdString()), HasSubstr("Failed to open")))); +} + +TEST_F(TestImageVaultUtils, verify_file_hash_throws_on_bad_hash) +{ + mpt::MockImageVaultUtils mock_utils; + EXPECT_CALL(mock_utils, compute_file_hash(test_path)).WillOnce(Return(":(")); + + MP_EXPECT_THROW_THAT(MP_IMAGE_VAULT_UTILS.verify_file_hash(test_path, ":)", mock_utils), + std::runtime_error, + mpt::match_what(HasSubstr("hash does not match"))); +} + +TEST_F(TestImageVaultUtils, verify_file_hash_doesnt_throw_on_good_hash) +{ + mpt::MockImageVaultUtils mock_utils; + EXPECT_CALL(mock_utils, compute_file_hash(test_path)).WillOnce(Return(":)")); + + EXPECT_NO_THROW(MP_IMAGE_VAULT_UTILS.verify_file_hash(test_path, ":)", mock_utils)); +} + +TEST_F(TestImageVaultUtils, extract_image_will_delete_file) +{ + mpt::MockImageDecoder decoder{}; + EXPECT_CALL(decoder, decode_to(_, _, _)); + + mp::ProgressMonitor monitor = [](int, int) { return true; }; + + EXPECT_CALL(mock_file_ops, remove(Property(&QFile::fileName, test_path))); + + MP_IMAGE_VAULT_UTILS.extract_file(test_path, monitor, true, decoder); +} + +TEST_F(TestImageVaultUtils, extract_image_wont_delete_file) +{ + const QString dest{"not_secrets/a_secret"}; + EXPECT_CALL(mock_file_ops, remove_extension(test_path)).WillOnce(Return(dest)); + + mpt::MockImageDecoder decoder{}; + EXPECT_CALL(decoder, decode_to(_, _, _)); + + mp::ProgressMonitor monitor = [](int, int) { return true; }; + + EXPECT_CALL(mock_file_ops, remove(_)).Times(0); + + MP_IMAGE_VAULT_UTILS.extract_file(test_path, monitor, false, decoder); +} + +TEST_F(TestImageVaultUtils, extract_image_extracts_image) +{ + const QString dest{"not_secrets/a_secret"}; + EXPECT_CALL(mock_file_ops, remove_extension(test_path)).WillOnce(Return(dest)); + + mp::ProgressMonitor monitor = [](int, int) { return true; }; + + mpt::MockImageDecoder decoder{}; + EXPECT_CALL(decoder, decode_to(test_path, dest, _)); + + auto res = MP_IMAGE_VAULT_UTILS.extract_file(test_path, monitor, false, decoder); + EXPECT_EQ(res, dest); +} + +} // namespace diff --git a/tests/test_utils.cpp b/tests/test_utils.cpp index 4732add8a6..ff4222a117 100644 --- a/tests/test_utils.cpp +++ b/tests/test_utils.cpp @@ -613,29 +613,6 @@ TEST(Utils, validate_server_address_does_not_throw_on_good_address) EXPECT_NO_THROW(mp::utils::validate_server_address("test-server.net:123")); } -TEST(Utils, dir_is_a_dir) -{ - mpt::TempDir temp_dir; - EXPECT_TRUE(mp::utils::is_dir(temp_dir.path().toStdString())); -} - -TEST(Utils, file_is_not_a_dir) -{ - mpt::TempDir temp_dir; - auto file_name = temp_dir.path() + "/empty_test_file"; - mpt::make_file_with_content(file_name, ""); - - EXPECT_FALSE(mp::utils::is_dir(file_name.toStdString())); -} - -TEST(Utils, filename_only_is_returned) -{ - std::string file_name{"my_file"}; - std::string full_path{"/tmp/foo/" + file_name}; - - EXPECT_THAT(mp::utils::filename_for(full_path), Eq(file_name)); -} - TEST(Utils, no_subdirectory_returns_same_path) { mp::Path original_path{"/tmp/foo"}; @@ -746,34 +723,3 @@ TEST(Utils, check_filesystem_bytes_available_returns_non_negative) EXPECT_GE(bytes_available, 0); } - -TEST(VaultUtils, copy_creates_new_file_and_returned_path_exists) -{ - mpt::TempDir temp_dir1, temp_dir2; - auto orig_file_path = QDir(temp_dir1.path()).filePath("test_file"); - - mpt::make_file_with_content(orig_file_path); - - auto new_file_path = mp::vault::copy(orig_file_path, temp_dir2.path()); - - EXPECT_TRUE(QFile::exists(new_file_path)); -} - -TEST(VaultUtils, copy_returns_empty_path_when_file_name_is_empty) -{ - mpt::TempDir temp_dir; - - auto path = mp::vault::copy("", temp_dir.path()); - - EXPECT_TRUE(path.isEmpty()); -} - -TEST(VaultUtils, copy_throws_when_file_does_not_exist) -{ - mpt::TempDir temp_dir; - - const QString file_name{"/foo/bar"}; - - MP_EXPECT_THROW_THAT(mp::vault::copy(file_name, temp_dir.path()), std::runtime_error, - mpt::match_what(StrEq(fmt::format("{} missing", file_name)))); -} From 04a46f3606c3519fb7d4745c0c9f7c13ee675c80 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 12 Dec 2024 15:27:50 -0500 Subject: [PATCH 2/6] Add implementation of image vault utils --- include/multipass/vm_image_vault_utils.h | 8 ++-- src/daemon/default_vm_image_vault.cpp | 19 +++++--- .../backends/lxd/lxd_vm_image_vault.cpp | 11 ++--- .../backends/shared/base_vm_image_vault.h | 4 +- src/utils/file_ops.cpp | 5 ++- src/utils/vm_image_vault_utils.cpp | 33 ++++++++++---- tests/test_file_ops.cpp | 3 +- tests/test_image_vault_utils.cpp | 45 +++++++++++++++++-- 8 files changed, 97 insertions(+), 31 deletions(-) diff --git a/include/multipass/vm_image_vault_utils.h b/include/multipass/vm_image_vault_utils.h index 5da2f77927..97445d11c2 100644 --- a/include/multipass/vm_image_vault_utils.h +++ b/include/multipass/vm_image_vault_utils.h @@ -25,10 +25,7 @@ #include #include -#define MP_IMAGE_VAULT_UTILS \ - multipass::ImageVaultUtils \ - { \ - } +#define MP_IMAGE_VAULT_UTILS multipass::ImageVaultUtils() namespace multipass { @@ -62,6 +59,9 @@ template void ImageVaultUtils::verify_file_hash(const QString& file, const QString& hash, const Hasher& hasher) { auto file_hash = hasher.compute_file_hash(file); + + if (file_hash != hash) + throw std::runtime_error(fmt::format("Hash of {} does not match {}", file, hash)); } template diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index 3236a4c23b..7eacc78d86 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -299,7 +299,7 @@ mp::VMImage mp::DefaultVMImageVault::fetch_image(const FetchType& fetch_type, } vm_image = prepare(source_image); - vm_image.id = mp::vault::compute_image_hash(vm_image.image_path).toStdString(); + vm_image.id = MP_IMAGE_VAULT_UTILS.compute_file_hash(vm_image.image_path).toStdString(); remove_source_images(source_image, vm_image); @@ -360,7 +360,9 @@ mp::VMImage mp::DefaultVMImageVault::fetch_image(const FetchType& fetch_type, last_modified.toString(), 0, checksum.has_value()}; - const auto image_filename = mp::vault::filename_for(image_url.path()); + + QFileInfo file_info{image_url.path()}; + const auto image_filename = file_info.fileName(); // Attempt to make a sane directory name based on the filename of the image const auto image_dir_name = @@ -624,8 +626,10 @@ mp::VMImage mp::DefaultVMImageVault::download_and_prepare_source_image( } else { + QFileInfo file_info{info.image_location}; + source_image.id = id.toStdString(); - source_image.image_path = image_dir.filePath(mp::vault::filename_for(info.image_location)); + source_image.image_path = image_dir.filePath(file_info.fileName()); source_image.original_release = info.release_title.toStdString(); source_image.release_date = info.version.toStdString(); @@ -646,12 +650,13 @@ mp::VMImage mp::DefaultVMImageVault::download_and_prepare_source_image( { mpl::log(mpl::Level::debug, category, fmt::format("Verifying hash \"{}\"", id)); monitor(LaunchProgress::VERIFY, -1); - mp::vault::verify_image_download(source_image.image_path, id); + MP_IMAGE_VAULT_UTILS.verify_file_hash(source_image.image_path, id); } if (source_image.image_path.endsWith(".xz")) { - source_image.image_path = mp::vault::extract_image(source_image.image_path, monitor, true); + source_image.image_path = + MP_IMAGE_VAULT_UTILS.extract_file(source_image.image_path, monitor, true, XzImageDecoder{}); } auto prepared_image = prepare(source_image); @@ -678,14 +683,14 @@ QString mp::DefaultVMImageVault::extract_image_from(const VMImage& source_image, const auto image_name = file_info.fileName().remove(".xz"); const auto image_path = QDir(dest_dir).filePath(image_name); - return mp::vault::extract_image(image_path, monitor); + return MP_IMAGE_VAULT_UTILS.extract_file(image_path, monitor); } mp::VMImage mp::DefaultVMImageVault::image_instance_from(const VMImage& prepared_image, const mp::Path& dest_dir) { MP_UTILS.make_dir(dest_dir); - return {mp::vault::copy(prepared_image.image_path, dest_dir), + return {MP_IMAGE_VAULT_UTILS.copy_to_dir(prepared_image.image_path, dest_dir), prepared_image.id, prepared_image.original_release, prepared_image.current_release, diff --git a/src/platform/backends/lxd/lxd_vm_image_vault.cpp b/src/platform/backends/lxd/lxd_vm_image_vault.cpp index 72d4e23479..435e51b6a7 100644 --- a/src/platform/backends/lxd/lxd_vm_image_vault.cpp +++ b/src/platform/backends/lxd/lxd_vm_image_vault.cpp @@ -83,7 +83,7 @@ QString post_process_downloaded_image(const QString& image_path, const mp::Progr if (image_path.endsWith(".xz")) { - new_image_path = mp::vault::extract_image(image_path, monitor, true); + new_image_path = MP_IMAGE_VAULT_UTILS.extract_file(image_path, monitor, true, mp::XzImageDecoder{}); } QString original_image_path{new_image_path}; @@ -263,7 +263,7 @@ mp::VMImage mp::LXDVMImageVault::fetch_image(const FetchType& fetch_type, throw std::runtime_error(fmt::format("Custom image `{}` does not exist.", image_url.path())); source_image.image_path = image_url.path(); - id = mp::vault::compute_image_hash(source_image.image_path); + id = MP_IMAGE_VAULT_UTILS.compute_file_hash(source_image.image_path); last_modified = QDateTime::currentDateTime(); } @@ -307,13 +307,14 @@ mp::VMImage mp::LXDVMImageVault::fetch_image(const FetchType& fetch_type, if (query.query_type != Query::Type::LocalFile) { // TODO: Need to make this async like in DefaultVMImageVault - image_path = lxd_import_dir.filePath(mp::vault::filename_for(info.image_location)); + QFileInfo file_info{info.image_location}; + image_path = lxd_import_dir.filePath(file_info.fileName()); url_download_image(info, image_path, monitor); } else { - image_path = mp::vault::copy(source_image.image_path, lxd_import_dir.path()); + image_path = MP_IMAGE_VAULT_UTILS.copy_to_dir(source_image.image_path, lxd_import_dir.path()); } image_path = post_process_downloaded_image(image_path, monitor); @@ -524,7 +525,7 @@ void mp::LXDVMImageVault::url_download_image(const VMImageInfo& info, const QStr if (info.verify) { monitor(LaunchProgress::VERIFY, -1); - mp::vault::verify_image_download(image_path, info.id); + MP_IMAGE_VAULT_UTILS.verify_file_hash(image_path, info.id); } } diff --git a/src/platform/backends/shared/base_vm_image_vault.h b/src/platform/backends/shared/base_vm_image_vault.h index 09fda0da1b..d636c83c4d 100644 --- a/src/platform/backends/shared/base_vm_image_vault.h +++ b/src/platform/backends/shared/base_vm_image_vault.h @@ -23,6 +23,7 @@ #include #include #include +#include #include #include @@ -34,7 +35,8 @@ class BaseVMImageVault : public VMImageVault { public: explicit BaseVMImageVault(const std::vector& image_hosts) - : image_hosts{image_hosts}, remote_image_host_map{vault::configure_image_host_map(image_hosts)} {}; + : image_hosts{image_hosts}, + remote_image_host_map{MP_IMAGE_VAULT_UTILS.configure_image_host_map(image_hosts)} {}; VMImageHost* image_host_for(const std::string& remote_name) const override { diff --git a/src/utils/file_ops.cpp b/src/utils/file_ops.cpp index 7e403f52c3..ca8f608394 100644 --- a/src/utils/file_ops.cpp +++ b/src/utils/file_ops.cpp @@ -172,8 +172,9 @@ bool mp::FileOps::flush(QFile& file) const QString mp::FileOps::remove_extension(const QString& path) const { - QFileInfo info{path}; - return info.dir().path() + info.completeBaseName(); + const QFileInfo info{path}; + auto extension_len = info.suffix().size(); + return path.chopped(extension_len != 0 ? extension_len + 1 : 0); } bool mp::FileOps::copy(const QString& from, const QString& to) const diff --git a/src/utils/vm_image_vault_utils.cpp b/src/utils/vm_image_vault_utils.cpp index e24c214398..f839b908f0 100644 --- a/src/utils/vm_image_vault_utils.cpp +++ b/src/utils/vm_image_vault_utils.cpp @@ -30,25 +30,42 @@ namespace mp = multipass; QString mp::ImageVaultUtils::copy_to_dir(const QString& file, const QDir& output_dir) { - return ""; + if (file.isEmpty()) + return ""; + + const QFileInfo info{file}; + if (!MP_FILEOPS.exists(info)) + throw std::runtime_error(fmt::format("File {} not found", file)); + + auto new_location = output_dir.filePath(info.fileName()); + + if (!MP_FILEOPS.copy(file, new_location)) + throw std::runtime_error(fmt::format("Failed to copy {} to {}", file, new_location)); + + return new_location; } QString mp::ImageVaultUtils::compute_hash(QIODevice& device) { - return ""; + QCryptographicHash hash{QCryptographicHash::Sha256}; + if (!hash.addData(std::addressof(device))) + throw std::runtime_error("Failed to read data from device to hash"); + + return hash.result().toHex(); } QString mp::ImageVaultUtils::compute_file_hash(const QString& path) { - return ""; + QFile file{path}; + if (!MP_FILEOPS.open(file, QFile::ReadOnly)) + throw std::runtime_error(fmt::format("Failed to open {}", path)); + + return compute_hash(file); } mp::ImageVaultUtils::HostMap mp::ImageVaultUtils::configure_image_host_map(const Hosts& image_hosts) { - return {}; - /* - std::unordered_map remote_image_host_map; - + HostMap remote_image_host_map{}; for (const auto& image_host : image_hosts) { for (const auto& remote : image_host->supported_remotes()) @@ -57,5 +74,5 @@ mp::ImageVaultUtils::HostMap mp::ImageVaultUtils::configure_image_host_map(const } } - return remote_image_host_map;*/ + return remote_image_host_map; } diff --git a/tests/test_file_ops.cpp b/tests/test_file_ops.cpp index fcc6b84bb0..5a1cd1ddef 100644 --- a/tests/test_file_ops.cpp +++ b/tests/test_file_ops.cpp @@ -195,7 +195,8 @@ TEST_F(FileOps, posix_lseek) TEST_F(FileOps, remove_extension) { + EXPECT_EQ(MP_FILEOPS.remove_extension(""), ""); EXPECT_EQ(MP_FILEOPS.remove_extension("test.txt"), "test"); EXPECT_EQ(MP_FILEOPS.remove_extension("tests/test.test.txt"), "tests/test.test"); - EXPECT_EQ(MP_FILEOPS.remove_extension("/sets/test.png"), "/sets/test.png"); + EXPECT_EQ(MP_FILEOPS.remove_extension("/sets/test.png"), "/sets/test"); } diff --git a/tests/test_image_vault_utils.cpp b/tests/test_image_vault_utils.cpp index 5062358e10..35ccf3157e 100644 --- a/tests/test_image_vault_utils.cpp +++ b/tests/test_image_vault_utils.cpp @@ -18,6 +18,7 @@ #include "common.h" #include "mock_file_ops.h" #include "mock_image_decoder.h" +#include "mock_image_host.h" #include "mock_image_vault_utils.h" #include @@ -116,9 +117,10 @@ TEST_F(TestImageVaultUtils, verify_file_hash_throws_on_bad_hash) mpt::MockImageVaultUtils mock_utils; EXPECT_CALL(mock_utils, compute_file_hash(test_path)).WillOnce(Return(":(")); - MP_EXPECT_THROW_THAT(MP_IMAGE_VAULT_UTILS.verify_file_hash(test_path, ":)", mock_utils), - std::runtime_error, - mpt::match_what(HasSubstr("hash does not match"))); + MP_EXPECT_THROW_THAT( + MP_IMAGE_VAULT_UTILS.verify_file_hash(test_path, ":)", mock_utils), + std::runtime_error, + mpt::match_what(AllOf(HasSubstr(test_path.toStdString()), HasSubstr(":)"), HasSubstr("does not match")))); } TEST_F(TestImageVaultUtils, verify_file_hash_doesnt_throw_on_good_hash) @@ -170,4 +172,41 @@ TEST_F(TestImageVaultUtils, extract_image_extracts_image) EXPECT_EQ(res, dest); } +TEST_F(TestImageVaultUtils, empty_hosts_produces_empty_map) +{ + auto map = MP_IMAGE_VAULT_UTILS.configure_image_host_map({}); + EXPECT_TRUE(map.empty()); +} + +TEST_F(TestImageVaultUtils, configure_image_host_map_maps_hosts) +{ + mpt::MockImageHost mock1{}; + std::vector hosts1{"this", "is", "a", "remotes"}; + EXPECT_CALL(mock1, supported_remotes()).WillOnce(Return(hosts1)); + + mpt::MockImageHost mock2{}; + std::vector hosts2{"hi"}; + EXPECT_CALL(mock2, supported_remotes()).WillOnce(Return(hosts2)); + + auto map = MP_IMAGE_VAULT_UTILS.configure_image_host_map({&mock1, &mock2}); + + EXPECT_EQ(map.size(), hosts1.size() + hosts2.size()); + + for (const auto& host : hosts1) + { + if (auto it = map.find(host); it != map.end()) + EXPECT_EQ(it->second, &mock1); + else + ADD_FAILURE() << fmt::format("{} was not mapped", host); + } + + for (const auto& host : hosts2) + { + if (auto it = map.find(host); it != map.end()) + EXPECT_EQ(it->second, &mock2); + else + ADD_FAILURE() << fmt::format("{} was not mapped", host); + } +} + } // namespace From 96fbd1fb9fe88f2b3c454a479d5aa87e3f381ad0 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Thu, 12 Dec 2024 18:00:11 -0500 Subject: [PATCH 3/6] Move vault utils to singleton --- include/multipass/vm_image_vault_utils.h | 59 ++++++-------- src/daemon/default_vm_image_vault.cpp | 3 +- .../backends/lxd/lxd_vm_image_vault.cpp | 2 +- src/utils/vm_image_vault_utils.cpp | 35 ++++++++- tests/mock_image_vault_utils.h | 20 +++-- tests/test_image_vault_utils.cpp | 78 ++++++++++++------- 6 files changed, 121 insertions(+), 76 deletions(-) diff --git a/include/multipass/vm_image_vault_utils.h b/include/multipass/vm_image_vault_utils.h index 97445d11c2..7b31692149 100644 --- a/include/multipass/vm_image_vault_utils.h +++ b/include/multipass/vm_image_vault_utils.h @@ -21,65 +21,56 @@ #include "file_ops.h" #include "xz_image_decoder.h" +#include #include #include #include -#define MP_IMAGE_VAULT_UTILS multipass::ImageVaultUtils() +#define MP_IMAGE_VAULT_UTILS multipass::ImageVaultUtils::instance() namespace multipass { class VMImageHost; -class ImageVaultUtils +class ImageVaultUtils : public Singleton { public: - using DefaultDecoder = XzImageDecoder; + ImageVaultUtils(const PrivatePass&) noexcept; - static QString copy_to_dir(const QString& file, const QDir& output_dir); - static QString compute_hash(QIODevice& device); - static QString compute_file_hash(const QString& file); + using Decoder = std::function; + using DefaultDecoderT = XzImageDecoder; - template - static void verify_file_hash(const QString& file, const QString& hash, const Hasher& hasher = ImageVaultUtils{}); + virtual QString copy_to_dir(const QString& file, const QDir& output_dir) const; + [[nodiscard]] virtual QString compute_hash(QIODevice& device) const; + [[nodiscard]] virtual QString compute_file_hash(const QString& file) const; - template - static QString extract_file(const QString& file, - const ProgressMonitor& monitor, - bool delete_original = false, - const Decoder& decoder = DefaultDecoder{}); + virtual void verify_file_hash(const QString& file, const QString& hash) const; + + virtual QString extract_file(const QString& file, const Decoder& decoder, bool delete_original = false) const; + + template + QString extract_file(const QString& file, + const ProgressMonitor& monitor, + bool delete_original = false, + const DecoderT& = DefaultDecoderT{}) const; using HostMap = std::unordered_map; using Hosts = std::vector; - static HostMap configure_image_host_map(const Hosts& image_hosts); + [[nodiscard]] virtual HostMap configure_image_host_map(const Hosts& image_hosts) const; }; -template -void ImageVaultUtils::verify_file_hash(const QString& file, const QString& hash, const Hasher& hasher) -{ - auto file_hash = hasher.compute_file_hash(file); - - if (file_hash != hash) - throw std::runtime_error(fmt::format("Hash of {} does not match {}", file, hash)); -} - -template +template QString ImageVaultUtils::extract_file(const QString& file, const ProgressMonitor& monitor, bool delete_original, - const Decoder& decoder) + const DecoderT& decoder) const { - auto new_path = MP_FILEOPS.remove_extension(file); - decoder.decode_to(file, new_path, monitor); - - if (delete_original) - { - QFile qfile{file}; - MP_FILEOPS.remove(qfile); - } + auto decoder_fn = [&monitor, &decoder](const QString& encoded_file, const QString& destination) { + return decoder.decode_to(encoded_file, destination, monitor); + }; - return new_path; + return extract_file(file, decoder_fn, delete_original); } } // namespace multipass diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index 7eacc78d86..dc566cd6c3 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -655,8 +655,7 @@ mp::VMImage mp::DefaultVMImageVault::download_and_prepare_source_image( if (source_image.image_path.endsWith(".xz")) { - source_image.image_path = - MP_IMAGE_VAULT_UTILS.extract_file(source_image.image_path, monitor, true, XzImageDecoder{}); + source_image.image_path = MP_IMAGE_VAULT_UTILS.extract_file(source_image.image_path, monitor, true); } auto prepared_image = prepare(source_image); diff --git a/src/platform/backends/lxd/lxd_vm_image_vault.cpp b/src/platform/backends/lxd/lxd_vm_image_vault.cpp index 435e51b6a7..6a62696324 100644 --- a/src/platform/backends/lxd/lxd_vm_image_vault.cpp +++ b/src/platform/backends/lxd/lxd_vm_image_vault.cpp @@ -83,7 +83,7 @@ QString post_process_downloaded_image(const QString& image_path, const mp::Progr if (image_path.endsWith(".xz")) { - new_image_path = MP_IMAGE_VAULT_UTILS.extract_file(image_path, monitor, true, mp::XzImageDecoder{}); + new_image_path = MP_IMAGE_VAULT_UTILS.extract_file(image_path, monitor, true); } QString original_image_path{new_image_path}; diff --git a/src/utils/vm_image_vault_utils.cpp b/src/utils/vm_image_vault_utils.cpp index f839b908f0..01d36207dd 100644 --- a/src/utils/vm_image_vault_utils.cpp +++ b/src/utils/vm_image_vault_utils.cpp @@ -28,7 +28,11 @@ namespace mp = multipass; -QString mp::ImageVaultUtils::copy_to_dir(const QString& file, const QDir& output_dir) +mp::ImageVaultUtils::ImageVaultUtils(const PrivatePass& pass) noexcept : Singleton{pass} +{ +} + +QString mp::ImageVaultUtils::copy_to_dir(const QString& file, const QDir& output_dir) const { if (file.isEmpty()) return ""; @@ -45,7 +49,7 @@ QString mp::ImageVaultUtils::copy_to_dir(const QString& file, const QDir& output return new_location; } -QString mp::ImageVaultUtils::compute_hash(QIODevice& device) +QString mp::ImageVaultUtils::compute_hash(QIODevice& device) const { QCryptographicHash hash{QCryptographicHash::Sha256}; if (!hash.addData(std::addressof(device))) @@ -54,7 +58,7 @@ QString mp::ImageVaultUtils::compute_hash(QIODevice& device) return hash.result().toHex(); } -QString mp::ImageVaultUtils::compute_file_hash(const QString& path) +QString mp::ImageVaultUtils::compute_file_hash(const QString& path) const { QFile file{path}; if (!MP_FILEOPS.open(file, QFile::ReadOnly)) @@ -63,7 +67,30 @@ QString mp::ImageVaultUtils::compute_file_hash(const QString& path) return compute_hash(file); } -mp::ImageVaultUtils::HostMap mp::ImageVaultUtils::configure_image_host_map(const Hosts& image_hosts) +void mp::ImageVaultUtils::verify_file_hash(const QString& file, const QString& hash) const +{ + const auto file_hash = compute_file_hash(file); + + if (file_hash != hash) + throw std::runtime_error(fmt::format("Hash of {} does not match {}", file, hash)); +} + +QString mp::ImageVaultUtils::extract_file(const QString& file, const Decoder& decoder, bool delete_original) const +{ + auto new_path = MP_FILEOPS.remove_extension(file); + + decoder(file, new_path); + + if (delete_original) + { + QFile qfile{file}; + MP_FILEOPS.remove(qfile); + } + + return new_path; +} + +mp::ImageVaultUtils::HostMap mp::ImageVaultUtils::configure_image_host_map(const Hosts& image_hosts) const { HostMap remote_image_host_map{}; for (const auto& image_host : image_hosts) diff --git a/tests/mock_image_vault_utils.h b/tests/mock_image_vault_utils.h index 331ae51d5e..052f862211 100644 --- a/tests/mock_image_vault_utils.h +++ b/tests/mock_image_vault_utils.h @@ -19,22 +19,28 @@ #define MULTIPASS_MOCK_IMAGE_VALUE_UTILS_H #include "common.h" -#include "mock_image_decoder.h" #include namespace multipass::test { -class MockImageVaultUtils +class MockImageVaultUtils : public ImageVaultUtils { public: - MOCK_METHOD(QString, copy_to_dir, (const QString&, const QDir&), (const)); - MOCK_METHOD(QString, compute_hash, (QIODevice&), (const)); - MOCK_METHOD(QString, compute_file_hash, (const QString&), (const)); - MOCK_METHOD(void, verify_file_hash, (const QString&, const QString&), (const)); + using ImageVaultUtils::ImageVaultUtils; - MOCK_METHOD(void, extract_file, (const QString&, const ProgressMonitor&, bool, const MockImageDecoder&), (const)); + MOCK_METHOD(QString, copy_to_dir, (const QString&, const QDir&), (const, override)); + + MOCK_METHOD(QString, compute_hash, (QIODevice&), (const, override)); + MOCK_METHOD(QString, compute_file_hash, (const QString&), (const, override)); + MOCK_METHOD(void, verify_file_hash, (const QString&, const QString&), (const, override)); + + MOCK_METHOD(QString, extract_file, (const QString&, const Decoder&, bool), (const, override)); + + MOCK_METHOD(HostMap, configure_image_host_map, (const Hosts&), (const, override)); + + MP_MOCK_SINGLETON_BOILERPLATE(MockImageVaultUtils, ImageVaultUtils); }; } // namespace multipass::test diff --git a/tests/test_image_vault_utils.cpp b/tests/test_image_vault_utils.cpp index 35ccf3157e..69f558fd8a 100644 --- a/tests/test_image_vault_utils.cpp +++ b/tests/test_image_vault_utils.cpp @@ -40,7 +40,6 @@ struct TestImageVaultUtils : public ::testing::Test const QDir test_dir{"secrets/secret_filled_folder"}; const QString test_path{"not_secrets/a_secret.txt"}; - QFile test_file{test_path}; const QFileInfo test_info{test_path}; const QString test_output{"secrets/secret_filled_folder/a_secret.txt"}; @@ -114,62 +113,85 @@ TEST_F(TestImageVaultUtils, compute_file_hash_throws_when_cant_open) TEST_F(TestImageVaultUtils, verify_file_hash_throws_on_bad_hash) { - mpt::MockImageVaultUtils mock_utils; - EXPECT_CALL(mock_utils, compute_file_hash(test_path)).WillOnce(Return(":(")); + auto [mock_utils, _] = mpt::MockImageVaultUtils::inject(); + EXPECT_CALL(*mock_utils, compute_file_hash(test_path)).WillOnce(Return(":(")); MP_EXPECT_THROW_THAT( - MP_IMAGE_VAULT_UTILS.verify_file_hash(test_path, ":)", mock_utils), + mock_utils->ImageVaultUtils::verify_file_hash(test_path, ":)"), std::runtime_error, mpt::match_what(AllOf(HasSubstr(test_path.toStdString()), HasSubstr(":)"), HasSubstr("does not match")))); } TEST_F(TestImageVaultUtils, verify_file_hash_doesnt_throw_on_good_hash) { - mpt::MockImageVaultUtils mock_utils; - EXPECT_CALL(mock_utils, compute_file_hash(test_path)).WillOnce(Return(":)")); + auto [mock_utils, _] = mpt::MockImageVaultUtils::inject(); + EXPECT_CALL(*mock_utils, compute_file_hash(test_path)).WillOnce(Return(":)")); - EXPECT_NO_THROW(MP_IMAGE_VAULT_UTILS.verify_file_hash(test_path, ":)", mock_utils)); + EXPECT_NO_THROW(mock_utils->ImageVaultUtils::verify_file_hash(test_path, ":)")); } -TEST_F(TestImageVaultUtils, extract_image_will_delete_file) +TEST_F(TestImageVaultUtils, extract_file_will_delete_file) { - mpt::MockImageDecoder decoder{}; - EXPECT_CALL(decoder, decode_to(_, _, _)); - - mp::ProgressMonitor monitor = [](int, int) { return true; }; + auto decoder = [](const QString&, const QString&) {}; EXPECT_CALL(mock_file_ops, remove(Property(&QFile::fileName, test_path))); - MP_IMAGE_VAULT_UTILS.extract_file(test_path, monitor, true, decoder); + MP_IMAGE_VAULT_UTILS.extract_file(test_path, decoder, true); } -TEST_F(TestImageVaultUtils, extract_image_wont_delete_file) +TEST_F(TestImageVaultUtils, extract_file_wont_delete_file) { - const QString dest{"not_secrets/a_secret"}; - EXPECT_CALL(mock_file_ops, remove_extension(test_path)).WillOnce(Return(dest)); - - mpt::MockImageDecoder decoder{}; - EXPECT_CALL(decoder, decode_to(_, _, _)); + EXPECT_CALL(mock_file_ops, remove_extension(test_path)).WillOnce(Return(test_output)); - mp::ProgressMonitor monitor = [](int, int) { return true; }; + int calls = 0; + auto decoder = [&](const QString& path, const QString& target) { + EXPECT_EQ(test_path, path); + EXPECT_EQ(test_output, target); + ++calls; + }; EXPECT_CALL(mock_file_ops, remove(_)).Times(0); - MP_IMAGE_VAULT_UTILS.extract_file(test_path, monitor, false, decoder); + MP_IMAGE_VAULT_UTILS.extract_file(test_path, decoder, false); + EXPECT_EQ(calls, 1); +} + +TEST_F(TestImageVaultUtils, extract_file_extracts_file) +{ + EXPECT_CALL(mock_file_ops, remove_extension(test_path)).WillOnce(Return(test_output)); + + int calls = 0; + auto decoder = [&](const QString& path, const QString& target) { + EXPECT_EQ(test_path, path); + EXPECT_EQ(test_output, target); + ++calls; + }; + + auto res = MP_IMAGE_VAULT_UTILS.extract_file(test_path, decoder, false); + EXPECT_EQ(res, test_output); + EXPECT_EQ(calls, 1); } -TEST_F(TestImageVaultUtils, extract_image_extracts_image) +TEST_F(TestImageVaultUtils, extract_file_with_decoder_binds_monitor) { - const QString dest{"not_secrets/a_secret"}; - EXPECT_CALL(mock_file_ops, remove_extension(test_path)).WillOnce(Return(dest)); + EXPECT_CALL(mock_file_ops, remove_extension(test_path)).WillOnce(Return(test_output)); - mp::ProgressMonitor monitor = [](int, int) { return true; }; + int type = 1337; + int progress = 42; + int calls = 0; + auto monitor = [&calls, &type, &progress](int in_type, int in_progress) { + EXPECT_EQ(in_type, type); + EXPECT_EQ(in_progress, progress); + ++calls; + return true; + }; mpt::MockImageDecoder decoder{}; - EXPECT_CALL(decoder, decode_to(test_path, dest, _)); + EXPECT_CALL(decoder, decode_to(test_path, test_output, Truly([&](const auto& m) { return m(type, progress); }))); + + MP_IMAGE_VAULT_UTILS.extract_file(test_path, monitor, false, decoder); - auto res = MP_IMAGE_VAULT_UTILS.extract_file(test_path, monitor, false, decoder); - EXPECT_EQ(res, dest); + EXPECT_EQ(calls, 1); } TEST_F(TestImageVaultUtils, empty_hosts_produces_empty_map) From 35a2e676fee30720dd4110571677721e3a12581b Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Mon, 16 Dec 2024 12:49:08 -0500 Subject: [PATCH 4/6] Polish image vault utils refactor --- src/daemon/default_vm_image_vault.cpp | 3 +-- tests/CMakeLists.txt | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index dc566cd6c3..d01b06f1a4 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -30,7 +31,6 @@ #include #include #include -#include #include @@ -41,7 +41,6 @@ #include #include -#include namespace mp = multipass; namespace mpl = multipass::logging; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 1d5a116bf9..efef0f2b9c 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -82,6 +82,7 @@ add_executable(multipass_tests test_global_settings_handlers.cpp test_id_mappings.cpp test_image_vault.cpp + test_image_vault_utils.cpp test_instance_settings_handler.cpp test_ip_address.cpp test_json_utils.cpp @@ -124,7 +125,6 @@ add_executable(multipass_tests test_sftp_utils.cpp test_file_ops.cpp test_recursive_dir_iter.cpp - test_image_vault_utils.cpp ) target_include_directories(multipass_tests From eaf1f9ebe1fd22f271cbbf85751c75833aa550f4 Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Mon, 16 Dec 2024 17:12:31 -0500 Subject: [PATCH 5/6] Add dummy var to appease nodiscard --- tests/test_image_vault_utils.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_image_vault_utils.cpp b/tests/test_image_vault_utils.cpp index 69f558fd8a..b99ec3e58f 100644 --- a/tests/test_image_vault_utils.cpp +++ b/tests/test_image_vault_utils.cpp @@ -83,7 +83,7 @@ TEST_F(TestImageVaultUtils, copy_to_dir_copys_to_dir) TEST_F(TestImageVaultUtils, compute_hash_throws_when_cant_read) { QBuffer buffer{}; // note: buffer is not opened - MP_EXPECT_THROW_THAT(MP_IMAGE_VAULT_UTILS.compute_hash(buffer), + MP_EXPECT_THROW_THAT(auto _ = MP_IMAGE_VAULT_UTILS.compute_hash(buffer), std::runtime_error, mpt::match_what(HasSubstr("Failed to read"))); } @@ -106,7 +106,7 @@ TEST_F(TestImageVaultUtils, compute_file_hash_throws_when_cant_open) }))) .WillOnce(Return(false)); - MP_EXPECT_THROW_THAT(MP_IMAGE_VAULT_UTILS.compute_file_hash(test_path), + MP_EXPECT_THROW_THAT(auto _ = MP_IMAGE_VAULT_UTILS.compute_file_hash(test_path), std::runtime_error, mpt::match_what(AllOf(HasSubstr(test_path.toStdString()), HasSubstr("Failed to open")))); } From f128abb31461e53463585cd02022fb0e603c9a6e Mon Sep 17 00:00:00 2001 From: Trevor Shoe Date: Tue, 17 Dec 2024 09:02:30 -0500 Subject: [PATCH 6/6] Replace `auto _` with `std::ignore` --- tests/test_image_vault_utils.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_image_vault_utils.cpp b/tests/test_image_vault_utils.cpp index b99ec3e58f..9e971bf04d 100644 --- a/tests/test_image_vault_utils.cpp +++ b/tests/test_image_vault_utils.cpp @@ -83,7 +83,7 @@ TEST_F(TestImageVaultUtils, copy_to_dir_copys_to_dir) TEST_F(TestImageVaultUtils, compute_hash_throws_when_cant_read) { QBuffer buffer{}; // note: buffer is not opened - MP_EXPECT_THROW_THAT(auto _ = MP_IMAGE_VAULT_UTILS.compute_hash(buffer), + MP_EXPECT_THROW_THAT(std::ignore = MP_IMAGE_VAULT_UTILS.compute_hash(buffer), std::runtime_error, mpt::match_what(HasSubstr("Failed to read"))); } @@ -106,7 +106,7 @@ TEST_F(TestImageVaultUtils, compute_file_hash_throws_when_cant_open) }))) .WillOnce(Return(false)); - MP_EXPECT_THROW_THAT(auto _ = MP_IMAGE_VAULT_UTILS.compute_file_hash(test_path), + MP_EXPECT_THROW_THAT(std::ignore = MP_IMAGE_VAULT_UTILS.compute_file_hash(test_path), std::runtime_error, mpt::match_what(AllOf(HasSubstr(test_path.toStdString()), HasSubstr("Failed to open")))); }