From 17d61393a942914f8f64b34c16ae23a8a4ae65c4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Wed, 13 Sep 2023 08:58:24 -0300 Subject: [PATCH 1/4] [daemon] Add local.instance.bridged setting. --- src/daemon/instance_settings_handler.cpp | 33 ++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/src/daemon/instance_settings_handler.cpp b/src/daemon/instance_settings_handler.cpp index 8c8f54be3e..cd1b13696b 100644 --- a/src/daemon/instance_settings_handler.cpp +++ b/src/daemon/instance_settings_handler.cpp @@ -19,6 +19,7 @@ #include #include +#include #include #include @@ -30,6 +31,7 @@ namespace constexpr auto cpus_suffix = "cpus"; constexpr auto mem_suffix = "memory"; constexpr auto disk_suffix = "disk"; +constexpr auto bridged_suffix = "bridged"; enum class Operation { @@ -46,7 +48,7 @@ QRegularExpression make_key_regex() { const auto instance_pattern = QStringLiteral("(?.+)"); const auto prop_template = QStringLiteral("(?%1)"); - const auto either_prop = QStringList{cpus_suffix, mem_suffix, disk_suffix}.join("|"); + const auto either_prop = QStringList{cpus_suffix, mem_suffix, disk_suffix, bridged_suffix}.join("|"); const auto prop_pattern = prop_template.arg(either_prop); const auto key_template = QStringLiteral(R"(%1\.%2\.%3)"); @@ -153,6 +155,27 @@ void update_disk(const QString& key, const QString& val, mp::VirtualMachine& ins } } +bool is_bridged(const mp::VMSpecs& spec) +{ + return std::any_of(spec.extra_interfaces.cbegin(), spec.extra_interfaces.cend(), + [](const auto& network) -> bool { return network.auto_mode; }); +} + +void update_bridged(const QString& key, const QString& val, mp::VirtualMachine& instance, mp::VMSpecs& spec) +{ + auto bridged = mp::BoolSettingSpec{key, "false"}.interpret(val) == "true"; + + if (!bridged && is_bridged(spec)) + { + throw mp::InvalidSettingException{key, val, "Bridged interface cannot be removed"}; + } + + if (bridged) + { + // TODO: add the interface + } +} + } // namespace mp::InstanceSettingsException::InstanceSettingsException(const std::string& reason, const std::string& instance, @@ -180,7 +203,7 @@ std::set mp::InstanceSettingsHandler::keys() const std::set ret; for (const auto& item : vm_instance_specs) - for (const auto& suffix : {cpus_suffix, mem_suffix, disk_suffix}) + for (const auto& suffix : {cpus_suffix, mem_suffix, disk_suffix, bridged_suffix}) ret.insert(key_template.arg(item.first.c_str()).arg(suffix)); return ret; @@ -191,6 +214,8 @@ QString mp::InstanceSettingsHandler::get(const QString& key) const auto [instance_name, property] = parse_key(key); const auto& spec = find_spec(instance_name); + if (property == bridged_suffix) + return is_bridged(spec) ? "true" : "false"; if (property == cpus_suffix) return QString::number(spec.num_cores); if (property == mem_suffix) @@ -214,6 +239,10 @@ void mp::InstanceSettingsHandler::set(const QString& key, const QString& val) if (property == cpus_suffix) update_cpus(key, val, instance, spec); + else if (property == bridged_suffix) + { + update_bridged(key, val, instance, spec); + } else { auto size = get_memory_size(key, val); From ac542954bfa5a9fca315d9f963c164782630caf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Wed, 13 Sep 2023 08:58:43 -0300 Subject: [PATCH 2/4] [tests] Check set local.instance.bridged. --- tests/test_instance_settings_handler.cpp | 76 +++++++++++++++++++++--- 1 file changed, 68 insertions(+), 8 deletions(-) diff --git a/tests/test_instance_settings_handler.cpp b/tests/test_instance_settings_handler.cpp index d5954bbe39..2f89562bfd 100644 --- a/tests/test_instance_settings_handler.cpp +++ b/tests/test_instance_settings_handler.cpp @@ -82,7 +82,9 @@ struct TestInstanceSettingsHandler : public Test std::unordered_map deleted_vms; std::unordered_set preparing_vms; bool fake_persister_called = false; - inline static constexpr auto properties = std::array{"cpus", "disk", "memory"}; + inline static constexpr std::array numeric_properties{"cpus", "disk", "memory"}; + inline static constexpr std::array boolean_properties{"bridged"}; + inline static constexpr std::array properties{"cpus", "disk", "memory", "bridged"}; }; QString make_key(const QString& instance_name, const QString& property) @@ -193,6 +195,26 @@ TEST_F(TestInstanceSettingsHandler, getReturnsMemorySizesInHumanReadableFormat) EXPECT_EQ(handler.get(make_key(target_instance_name, "memory")), "337.6KiB"); } +struct TestBridgedInstanceSettings : public TestInstanceSettingsHandler, public WithParamInterface +{ +}; + +TEST_P(TestBridgedInstanceSettings, getFetchesBridged) +{ + const auto bridged = GetParam(); + + constexpr auto target_instance_name = "lemmy"; + specs.insert({{"mikkey", {}}, {"phil", {}}, {target_instance_name, {}}}); + + // An extra interface in auto mode will result in bridging. + specs[target_instance_name].extra_interfaces = {{"id", "52:54:00:12:34:56", bridged}}; + + const auto got = make_handler().get(make_key(target_instance_name, "bridged")); + EXPECT_EQ(got, bridged ? "true" : "false"); +} + +INSTANTIATE_TEST_SUITE_P(getFetchesBridged, TestBridgedInstanceSettings, Values(true, false)); + TEST_F(TestInstanceSettingsHandler, getFetchesPropertiesOfInstanceInSpecialState) { constexpr auto preparing_instance = "nouvelle", deleted_instance = "vague"; @@ -424,6 +446,19 @@ TEST_F(TestInstanceSettingsHandler, setRefusesWrongProperty) EXPECT_EQ(original_specs, specs[target_instance_name]); } +TEST_F(TestInstanceSettingsHandler, setRefusesToUnbridge) +{ + constexpr auto target_instance_name = "hendrix"; + specs.insert({{"voodoo", {}}, {"chile", {}}, {target_instance_name, {}}}); + specs[target_instance_name].extra_interfaces = {{"id", "52:54:00:78:90:12", true}}; + + mock_vm(target_instance_name); // TODO: make this an expectation. + + MP_EXPECT_THROW_THAT(make_handler().set(make_key(target_instance_name, "bridged"), "false"), + mp::InvalidSettingException, + mpt::match_what(HasSubstr("Bridged interface cannot be removed"))); +} + using VMSt = mp::VirtualMachine::State; using Property = const char*; using PropertyAndState = std::tuple; // no subliminal political msg intended :) @@ -476,7 +511,8 @@ TEST_P(TestInstanceModOnStoppedInstance, setWorksOnOtherStates) } INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceModOnStoppedInstance, - Combine(ValuesIn(TestInstanceSettingsHandler::properties), Values(VMSt::off, VMSt::stopped))); + Combine(ValuesIn(TestInstanceSettingsHandler::numeric_properties), + Values(VMSt::off, VMSt::stopped))); struct TestInstanceModPersists : public TestInstanceSettingsHandler, public WithParamInterface { @@ -496,7 +532,7 @@ TEST_P(TestInstanceModPersists, setPersistsInstances) } INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceModPersists, - ValuesIn(TestInstanceSettingsHandler::properties)); + ValuesIn(TestInstanceSettingsHandler::numeric_properties)); TEST_F(TestInstanceSettingsHandler, setRefusesToModifyInstancesInSpecialState) { @@ -554,12 +590,12 @@ TEST_F(TestInstanceSettingsHandler, getAndSetThrowOnBadKey) using PlainValue = const char*; using PropertyValue = std::tuple; -struct TestInstanceSettingsHandlerBadValues : public TestInstanceSettingsHandler, - public WithParamInterface +struct TestInstanceSettingsHandlerBadNumericValues : public TestInstanceSettingsHandler, + public WithParamInterface { }; -TEST_P(TestInstanceSettingsHandlerBadValues, setRefusesBadValues) +TEST_P(TestInstanceSettingsHandlerBadNumericValues, setRefusesBadNumericValues) { constexpr auto target_instance_name = "xanana"; const auto& [property, bad_val] = GetParam(); @@ -574,8 +610,32 @@ TEST_P(TestInstanceSettingsHandlerBadValues, setRefusesBadValues) EXPECT_EQ(original_specs, specs[target_instance_name]); } -INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceSettingsHandlerBadValues, - Combine(ValuesIn(TestInstanceSettingsHandler::properties), +INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceSettingsHandlerBadNumericValues, + Combine(ValuesIn(TestInstanceSettingsHandler::numeric_properties), Values("0", "2u", "1.5f", "2.0", "0xa", "0x8", "-4", "-1", "rubbish", " 123nonsense ", "ยค9", "\n", "\t", "^", ""))); + +struct TestInstanceSettingsHandlerBadBooleanValues : public TestInstanceSettingsHandler, + public WithParamInterface +{ +}; + +TEST_P(TestInstanceSettingsHandlerBadBooleanValues, setRefusesBadBooleanValues) +{ + constexpr auto target_instance_name = "zappa"; + const auto& [property, bad_val] = GetParam(); + + const auto original_specs = specs[target_instance_name]; + mock_vm(target_instance_name); // TODO: make this an expectation. + + MP_EXPECT_THROW_THAT(make_handler().set(make_key(target_instance_name, property), bad_val), + mp::InvalidSettingException, + mpt::match_what(AllOf(HasSubstr(bad_val), HasSubstr("try \"true\" or \"false\"")))); + + EXPECT_EQ(original_specs, specs[target_instance_name]); +} + +INSTANTIATE_TEST_SUITE_P(TestInstanceSettingsHandler, TestInstanceSettingsHandlerBadBooleanValues, + Combine(ValuesIn(TestInstanceSettingsHandler::boolean_properties), + Values("apostrophe", "(')", "1974"))); } // namespace From 24a40629691811beda576f52c709009028bd80f6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Fri, 22 Sep 2023 08:27:49 -0300 Subject: [PATCH 3/4] [settings] Fix `get local..bridged`. It must return true if and only if there exists in the instance an interface bridged to the current interface set in local.bridged-interface. --- src/daemon/daemon.cpp | 23 +++++++++++++++++++---- src/daemon/instance_settings_handler.cpp | 20 ++++++++++++-------- src/daemon/instance_settings_handler.h | 3 ++- 3 files changed, 33 insertions(+), 13 deletions(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index b90563599c..80ac66bcda 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -457,6 +457,19 @@ auto try_mem_size(const std::string& val) -> std::optional } } +std::string get_bridged_interface_name() +{ + const auto bridged_id = MP_SETTINGS.get(mp::bridged_interface_key); + + if (bridged_id == "") + { + throw std::runtime_error(fmt::format("You have to `multipass set {}=` to use the \"bridged\" shortcut.", + mp::bridged_interface_key)); + } + + return bridged_id.toStdString(); +} + std::vector validate_extra_interfaces(const mp::LaunchRequest* request, const mp::VirtualMachineFactory& factory, std::vector& nets_need_bridging, @@ -1170,10 +1183,11 @@ register_instance_mod(std::unordered_map& vm_instance_ std::unordered_map& vm_instances, const std::unordered_map& deleted_instances, const std::unordered_set& preparing_instances, - std::function instance_persister) + std::function instance_persister, std::function bridged_interface) { return MP_SETTINGS.register_handler(std::make_unique( - vm_instance_specs, vm_instances, deleted_instances, preparing_instances, std::move(instance_persister))); + vm_instance_specs, vm_instances, deleted_instances, preparing_instances, std::move(instance_persister), + std::move(bridged_interface))); } } // namespace @@ -1184,8 +1198,9 @@ mp::Daemon::Daemon(std::unique_ptr the_config) mp::utils::backend_directory_path(config->data_directory, config->factory->get_backend_directory_name()), mp::utils::backend_directory_path(config->cache_directory, config->factory->get_backend_directory_name()))}, daemon_rpc{config->server_address, *config->cert_provider, config->client_cert_store.get()}, - instance_mod_handler{register_instance_mod(vm_instance_specs, operative_instances, deleted_instances, - preparing_instances, [this] { persist_instances(); })} + instance_mod_handler{register_instance_mod( + vm_instance_specs, operative_instances, deleted_instances, preparing_instances, + [this] { persist_instances(); }, get_bridged_interface_name)} { connect_rpc(daemon_rpc, *this); std::vector invalid_specs; diff --git a/src/daemon/instance_settings_handler.cpp b/src/daemon/instance_settings_handler.cpp index cd1b13696b..b503e18de4 100644 --- a/src/daemon/instance_settings_handler.cpp +++ b/src/daemon/instance_settings_handler.cpp @@ -155,17 +155,18 @@ void update_disk(const QString& key, const QString& val, mp::VirtualMachine& ins } } -bool is_bridged(const mp::VMSpecs& spec) +bool is_bridged(const mp::VMSpecs& spec, const std::string& br_interface) { return std::any_of(spec.extra_interfaces.cbegin(), spec.extra_interfaces.cend(), - [](const auto& network) -> bool { return network.auto_mode; }); + [&br_interface](const auto& network) -> bool { return network.id == br_interface; }); } -void update_bridged(const QString& key, const QString& val, mp::VirtualMachine& instance, mp::VMSpecs& spec) +void update_bridged(const QString& key, const QString& val, mp::VirtualMachine& instance, mp::VMSpecs& spec, + const std::string& br_interface) { auto bridged = mp::BoolSettingSpec{key, "false"}.interpret(val) == "true"; - if (!bridged && is_bridged(spec)) + if (!bridged && is_bridged(spec, br_interface)) { throw mp::InvalidSettingException{key, val, "Bridged interface cannot be removed"}; } @@ -188,12 +189,15 @@ mp::InstanceSettingsHandler::InstanceSettingsHandler( std::unordered_map& vm_instance_specs, std::unordered_map& vm_instances, const std::unordered_map& deleted_instances, - const std::unordered_set& preparing_instances, std::function instance_persister) + const std::unordered_set& preparing_instances, std::function instance_persister, + std::function bridged_interface) : vm_instance_specs{vm_instance_specs}, vm_instances{vm_instances}, deleted_instances{deleted_instances}, preparing_instances{preparing_instances}, - instance_persister{std::move(instance_persister)} + instance_persister{std::move(instance_persister)}, + bridged_interface{std::move(bridged_interface)} + { } @@ -215,7 +219,7 @@ QString mp::InstanceSettingsHandler::get(const QString& key) const const auto& spec = find_spec(instance_name); if (property == bridged_suffix) - return is_bridged(spec) ? "true" : "false"; + return is_bridged(spec, bridged_interface()) ? "true" : "false"; if (property == cpus_suffix) return QString::number(spec.num_cores); if (property == mem_suffix) @@ -241,7 +245,7 @@ void mp::InstanceSettingsHandler::set(const QString& key, const QString& val) update_cpus(key, val, instance, spec); else if (property == bridged_suffix) { - update_bridged(key, val, instance, spec); + update_bridged(key, val, instance, spec, bridged_interface()); } else { diff --git a/src/daemon/instance_settings_handler.h b/src/daemon/instance_settings_handler.h index 10f8cc43b7..98e84d2428 100644 --- a/src/daemon/instance_settings_handler.h +++ b/src/daemon/instance_settings_handler.h @@ -41,7 +41,7 @@ class InstanceSettingsHandler : public SettingsHandler std::unordered_map& vm_instances, const std::unordered_map& deleted_instances, const std::unordered_set& preparing_instances, - std::function instance_persister); + std::function instance_persister, std::function bridged_interface); std::set keys() const override; QString get(const QString& key) const override; @@ -59,6 +59,7 @@ class InstanceSettingsHandler : public SettingsHandler const std::unordered_map& deleted_instances; const std::unordered_set& preparing_instances; std::function instance_persister; + std::function bridged_interface; }; class InstanceSettingsException : public SettingsException From 1a28788031692fc20d03c63bf6539305493738da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Fri, 22 Sep 2023 08:29:27 -0300 Subject: [PATCH 4/4] [tests] Check `get local..bridged`. --- tests/test_instance_settings_handler.cpp | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/tests/test_instance_settings_handler.cpp b/tests/test_instance_settings_handler.cpp index 2f89562bfd..59ca54b986 100644 --- a/tests/test_instance_settings_handler.cpp +++ b/tests/test_instance_settings_handler.cpp @@ -49,7 +49,8 @@ struct TestInstanceSettingsHandler : public Test { mp::InstanceSettingsHandler make_handler() { - return mp::InstanceSettingsHandler{specs, vms, deleted_vms, preparing_vms, make_fake_persister()}; + return mp::InstanceSettingsHandler{ + specs, vms, deleted_vms, preparing_vms, make_fake_persister(), make_fake_bridged_interface()}; } void fake_instance_state(const char* name, SpecialInstanceState special_state) @@ -65,6 +66,11 @@ struct TestInstanceSettingsHandler : public Test return [this] { fake_persister_called = true; }; } + std::function make_fake_bridged_interface() + { + return [] { return "eth8"; }; + } + template