From 6e30be092478c8592e77fa79a00431fea4c5a625 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Fri, 5 Jan 2024 13:55:26 -0300 Subject: [PATCH 1/8] [factory] Add bridge_name_for(). This is a platform function which returns the name of the bridge created to bridge a given network interface. --- include/multipass/virtual_machine_factory.h | 1 + src/daemon/daemon.cpp | 5 +++++ src/daemon/instance_settings_handler.cpp | 22 +++++++++++++------ src/daemon/instance_settings_handler.h | 2 ++ .../shared/base_virtual_machine_factory.h | 5 +++++ 5 files changed, 28 insertions(+), 7 deletions(-) diff --git a/include/multipass/virtual_machine_factory.h b/include/multipass/virtual_machine_factory.h index 80d711a016..553331921b 100644 --- a/include/multipass/virtual_machine_factory.h +++ b/include/multipass/virtual_machine_factory.h @@ -70,6 +70,7 @@ class VirtualMachineFactory : private DisabledCopyMove // List all the network interfaces seen by the backend. virtual std::vector networks() const = 0; virtual void require_snapshots_support() const = 0; + virtual std::string bridge_name_for(const std::string& iface_name) const = 0; protected: VirtualMachineFactory() = default; diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index ea424f9f0b..d5e2815485 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -1239,6 +1239,7 @@ mp::SettingsHandler* register_instance_mod(std::unordered_map& preparing_instances, std::function instance_persister, std::function bridged_interface, + std::function bridge_name, std::function()> host_networks) { return MP_SETTINGS.register_handler(std::make_unique(vm_instance_specs, @@ -1247,6 +1248,7 @@ mp::SettingsHandler* register_instance_mod(std::unordered_map the_config) preparing_instances, [this] { persist_instances(); }, get_bridged_interface_name, + [this]() { + return config->factory->bridge_name_for(MP_SETTINGS.get(mp::bridged_interface_key).toStdString()); + }, [this]() { return config->factory->networks(); })}, snapshot_mod_handler{ register_snapshot_mod(operative_instances, deleted_instances, preparing_instances, *config->factory)} diff --git a/src/daemon/instance_settings_handler.cpp b/src/daemon/instance_settings_handler.cpp index e5ff0e9eb3..bcbed8e6fe 100644 --- a/src/daemon/instance_settings_handler.cpp +++ b/src/daemon/instance_settings_handler.cpp @@ -157,22 +157,25 @@ void update_disk(const QString& key, const QString& val, mp::VirtualMachine& ins } } -bool is_bridged(const mp::VMSpecs& spec, const std::string& br_interface) +bool is_bridged(const mp::VMSpecs& spec, const std::string& br_interface, const std::string& br_name) { return std::any_of(spec.extra_interfaces.cbegin(), spec.extra_interfaces.cend(), - [&br_interface](const auto& network) -> bool { return network.id == br_interface; }); + [&br_interface, &br_name](const auto& network) -> bool { + return network.id == br_interface || network.id == br_name; + }); } void checked_update_bridged(const QString& key, const QString& val, mp::VirtualMachine& instance, mp::VMSpecs& spec, - const std::string& br_interface) + const std::string& br_interface, + const std::string& br_name) { auto bridged = mp::BoolSettingSpec{key, "false"}.interpret(val) == "true"; - if (!bridged && is_bridged(spec, br_interface)) + if (!bridged && is_bridged(spec, br_interface, br_name)) { throw mp::InvalidSettingException{key, val, "Bridged interface cannot be removed"}; } @@ -189,6 +192,7 @@ void update_bridged(const QString& key, mp::VirtualMachine& instance, mp::VMSpecs& spec, std::function bridged_interface, + std::function bridge_name, std::function()> host_networks) { const auto& host_nets = host_networks(); // This will throw if not implemented on this backend. @@ -204,7 +208,7 @@ void update_bridged(const QString& key, mp::bridged_interface_key)); } - checked_update_bridged(key, val, instance, spec, br); + checked_update_bridged(key, val, instance, spec, br, bridge_name()); } } // namespace @@ -222,6 +226,7 @@ mp::InstanceSettingsHandler::InstanceSettingsHandler( const std::unordered_set& preparing_instances, std::function instance_persister, std::function bridged_interface, + std::function bridge_name, std::function()> host_networks) : vm_instance_specs{vm_instance_specs}, operative_instances{operative_instances}, @@ -229,6 +234,7 @@ mp::InstanceSettingsHandler::InstanceSettingsHandler( preparing_instances{preparing_instances}, instance_persister{std::move(instance_persister)}, bridged_interface{std::move(bridged_interface)}, + bridge_name{std::move(bridge_name)}, host_networks{std::move(host_networks)} { } @@ -251,7 +257,9 @@ QString mp::InstanceSettingsHandler::get(const QString& key) const const auto& spec = find_spec(instance_name); if (property == bridged_suffix) - return is_bridged(spec, bridged_interface()) ? "true" : "false"; + { + return is_bridged(spec, bridged_interface(), bridge_name()) ? "true" : "false"; + } if (property == cpus_suffix) return QString::number(spec.num_cores); if (property == mem_suffix) @@ -277,7 +285,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, bridged_interface, host_networks); + update_bridged(key, val, instance, spec, bridged_interface, bridge_name, host_networks); } else { diff --git a/src/daemon/instance_settings_handler.h b/src/daemon/instance_settings_handler.h index 48a1cdfb63..91b7f32e43 100644 --- a/src/daemon/instance_settings_handler.h +++ b/src/daemon/instance_settings_handler.h @@ -44,6 +44,7 @@ class InstanceSettingsHandler : public SettingsHandler const std::unordered_set& preparing_instances, std::function instance_persister, std::function bridged_interface, + std::function bridge_name, std::function()> host_networks); std::set keys() const override; @@ -63,6 +64,7 @@ class InstanceSettingsHandler : public SettingsHandler const std::unordered_set& preparing_instances; std::function instance_persister; std::function bridged_interface; + std::function bridge_name; std::function()> host_networks; }; diff --git a/src/platform/backends/shared/base_virtual_machine_factory.h b/src/platform/backends/shared/base_virtual_machine_factory.h index ff7e53e25d..24b3e3fd5c 100644 --- a/src/platform/backends/shared/base_virtual_machine_factory.h +++ b/src/platform/backends/shared/base_virtual_machine_factory.h @@ -75,6 +75,11 @@ class BaseVirtualMachineFactory : public VirtualMachineFactory void require_snapshots_support() const override; + std::string bridge_name_for(const std::string& iface_name) const override + { + return ""; + }; + protected: static const Path instances_subdir; From b9351194353b1a97042a5e7797725d5406b300c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Fri, 5 Jan 2024 13:55:50 -0300 Subject: [PATCH 2/8] [lxd] Add bridge_name_for(). --- src/platform/backends/lxd/lxd_virtual_machine_factory.h | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/platform/backends/lxd/lxd_virtual_machine_factory.h b/src/platform/backends/lxd/lxd_virtual_machine_factory.h index e2259de958..3d307cc757 100644 --- a/src/platform/backends/lxd/lxd_virtual_machine_factory.h +++ b/src/platform/backends/lxd/lxd_virtual_machine_factory.h @@ -52,6 +52,11 @@ class LXDVirtualMachineFactory : public BaseVirtualMachineFactory std::vector networks() const override; + std::string bridge_name_for(const std::string& iface_name) const override + { + return "br-" + iface_name; + }; + protected: void remove_resources_for_impl(const std::string& name) override; std::string create_bridge_with(const NetworkInterfaceInfo& interface) override; From 25be143d0a39483c8550735b24df147904f6bf57 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Fri, 5 Jan 2024 13:56:31 -0300 Subject: [PATCH 3/8] [tests] Check `local..bridged` works. --- tests/mock_virtual_machine_factory.h | 1 + tests/test_instance_settings_handler.cpp | 11 ++++++++++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/tests/mock_virtual_machine_factory.h b/tests/mock_virtual_machine_factory.h index 3413fd4e19..500812c2ee 100644 --- a/tests/mock_virtual_machine_factory.h +++ b/tests/mock_virtual_machine_factory.h @@ -48,6 +48,7 @@ struct MockVirtualMachineFactory : public VirtualMachineFactory MOCK_METHOD(void, configure, (VirtualMachineDescription&), (override)); MOCK_METHOD(std::vector, networks, (), (const, override)); MOCK_METHOD(void, require_snapshots_support, (), (const, override)); + MOCK_METHOD(std::string, bridge_name_for, (const std::string&), (const, override)); // originally protected: MOCK_METHOD(std::string, create_bridge_with, (const NetworkInterfaceInfo&), (override)); diff --git a/tests/test_instance_settings_handler.cpp b/tests/test_instance_settings_handler.cpp index af84f4406b..ef5a75a93f 100644 --- a/tests/test_instance_settings_handler.cpp +++ b/tests/test_instance_settings_handler.cpp @@ -55,6 +55,7 @@ struct TestInstanceSettingsHandler : public Test preparing_vms, make_fake_persister(), make_fake_bridged_interface(), + make_fake_bridge_name(), make_fake_host_networks()}; } @@ -76,6 +77,11 @@ struct TestInstanceSettingsHandler : public Test return [] { return "eth8"; }; } + std::function make_fake_bridge_name() + { + return [] { return "br-eth8"; }; + } + std::function()> make_fake_host_networks() { std::vector ret; @@ -235,7 +241,10 @@ TEST_P(TestBridgedInstanceSettings, getFetchesBridged) INSTANTIATE_TEST_SUITE_P(getFetchesBridged, TestBridgedInstanceSettings, - Values(std::make_pair("eth8", true), std::make_pair("eth9", false))); + Values(std::make_pair("eth8", true), + std::make_pair("br-eth8", true), + std::make_pair("eth9", false), + std::make_pair("br-eth9", false))); TEST_F(TestInstanceSettingsHandler, getFetchesPropertiesOfInstanceInSpecialState) { From 7d89c5d7dcdcba3f5489328507a0803d653ab5ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Tue, 9 Jan 2024 17:59:46 -0300 Subject: [PATCH 4/8] [tests] Check base factory's bridge_name_for works --- tests/test_base_virtual_machine_factory.cpp | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/test_base_virtual_machine_factory.cpp b/tests/test_base_virtual_machine_factory.cpp index 038ab11928..2d06daa0d3 100644 --- a/tests/test_base_virtual_machine_factory.cpp +++ b/tests/test_base_virtual_machine_factory.cpp @@ -54,6 +54,7 @@ struct MockBaseFactory : mp::BaseVirtualMachineFactory MOCK_METHOD(QString, get_backend_version_string, (), (const, override)); MOCK_METHOD(void, prepare_networking, (std::vector&), (override)); MOCK_METHOD(std::vector, networks, (), (const, override)); + MOCK_METHOD(std::string, bridge_name_for, (const std::string&), (const, override)); MOCK_METHOD(std::string, create_bridge_with, (const mp::NetworkInterfaceInfo&), (override)); MOCK_METHOD(void, prepare_interface, (mp::NetworkInterface & net, std::vector& host_nets, @@ -61,6 +62,11 @@ struct MockBaseFactory : mp::BaseVirtualMachineFactory (override)); MOCK_METHOD(void, remove_resources_for_impl, (const std::string&), (override)); + std::string base_bridge_name_for(const std::string& iface_name) const + { + return mp::BaseVirtualMachineFactory::bridge_name_for(iface_name); + } + std::string base_create_bridge_with(const mp::NetworkInterfaceInfo& interface) { return mp::BaseVirtualMachineFactory::create_bridge_with(interface); // protected @@ -155,6 +161,13 @@ TEST_F(BaseFactory, creates_cloud_init_iso_image) EXPECT_TRUE(QFile::exists(vm_desc.cloud_init_iso)); } +TEST_F(BaseFactory, baseBridgeNameForReturnsEmpty) +{ + StrictMock factory; + + ASSERT_EQ(factory.base_bridge_name_for("any_interface"), ""); +} + TEST_F(BaseFactory, create_bridge_not_implemented) { StrictMock factory; From 7f10f711c4b15361cd1885a1847c4fffe5f6d03c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Tue, 9 Jan 2024 18:00:30 -0300 Subject: [PATCH 5/8] [tests] Check LXD factory's bridge_name_for works --- tests/lxd/test_lxd_backend.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/tests/lxd/test_lxd_backend.cpp b/tests/lxd/test_lxd_backend.cpp index ecae440869..d93dc3a822 100644 --- a/tests/lxd/test_lxd_backend.cpp +++ b/tests/lxd/test_lxd_backend.cpp @@ -2355,3 +2355,10 @@ TEST_F(LXDBackend, addsNetworkInterface) EXPECT_EQ(times_called, 1u); } + +TEST_F(LXDBackend, backendReturnsCorrectBridgeName) +{ + CustomLXDFactory factory{std::move(mock_network_access_manager), data_dir.path(), base_url}; + + EXPECT_EQ(factory.bridge_name_for("eth7"), "br-eth7"); +} From 94825a7da1db5d28ff62c80c6dab9d64563baed2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Mon, 15 Jan 2024 14:48:09 -0300 Subject: [PATCH 6/8] [linux] Add function returning bridge name. --- src/platform/backends/shared/linux/backend_utils.cpp | 12 ++++++++++-- src/platform/backends/shared/linux/backend_utils.h | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/src/platform/backends/shared/linux/backend_utils.cpp b/src/platform/backends/shared/linux/backend_utils.cpp index 67b2c6058a..9367fdec35 100644 --- a/src/platform/backends/shared/linux/backend_utils.cpp +++ b/src/platform/backends/shared/linux/backend_utils.cpp @@ -196,6 +196,15 @@ std::string mp::backend::generate_random_subnet() throw std::runtime_error("Could not determine a subnet for networking."); } +std::string mp::Backend::bridge_name(const std::string& interface) const +{ + static const std::string base_name{"br-"}; + + static const auto max_iface_length{max_bridge_name_len - base_name.size()}; + + return fmt::format("{}{:.{}}", base_name, interface, max_iface_length); +} + // @precondition no bridge exists for this interface // @precondition interface identifies an ethernet device std::string mp::Backend::create_bridge_with(const std::string& interface) @@ -203,7 +212,6 @@ std::string mp::Backend::create_bridge_with(const std::string& interface) static constexpr auto log_category_create = "create bridge"; static constexpr auto log_category_rollback = "rollback bridge"; static const auto root_path = QDBusObjectPath{"/"}; - static const auto base_name = QStringLiteral("br-"); static std::once_flag once; std::call_once(once, [] { qDBusRegisterMetaType(); }); @@ -212,7 +220,7 @@ std::string mp::Backend::create_bridge_with(const std::string& interface) auto nm_root = get_checked_interface(system_bus, nm_bus_name, nm_root_obj, nm_root_ifc); auto nm_settings = get_checked_interface(system_bus, nm_bus_name, nm_settings_obj, nm_settings_ifc); - auto parent_name = (base_name + interface.c_str()).left(max_bridge_name_len); + auto parent_name = QString::fromStdString(bridge_name(interface)); auto child_name = parent_name + "-child"; mpl::log(mpl::Level::debug, log_category_create, fmt::format("Creating bridge: {}", parent_name)); diff --git a/src/platform/backends/shared/linux/backend_utils.h b/src/platform/backends/shared/linux/backend_utils.h index 6d2992c1a7..6ae56d5c20 100644 --- a/src/platform/backends/shared/linux/backend_utils.h +++ b/src/platform/backends/shared/linux/backend_utils.h @@ -51,6 +51,7 @@ class Backend : public Singleton public: using Singleton::Singleton; + virtual std::string bridge_name(const std::string& interface) const; virtual std::string create_bridge_with(const std::string& interface); virtual std::string get_subnet(const Path& network_dir, const QString& bridge_name) const; From 6a752ba9606ab684c36283f9bd0ee8d2dd18d2ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Mon, 15 Jan 2024 14:49:12 -0300 Subject: [PATCH 7/8] [lxd] Get interface name from backend. --- src/platform/backends/lxd/lxd_virtual_machine_factory.cpp | 5 +++++ src/platform/backends/lxd/lxd_virtual_machine_factory.h | 5 +---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/src/platform/backends/lxd/lxd_virtual_machine_factory.cpp b/src/platform/backends/lxd/lxd_virtual_machine_factory.cpp index 8c718cf557..77d011d079 100644 --- a/src/platform/backends/lxd/lxd_virtual_machine_factory.cpp +++ b/src/platform/backends/lxd/lxd_virtual_machine_factory.cpp @@ -252,6 +252,11 @@ auto mp::LXDVirtualMachineFactory::networks() const -> std::vector& extra_interfaces) { prepare_networking_guts(extra_interfaces, "bridge"); diff --git a/src/platform/backends/lxd/lxd_virtual_machine_factory.h b/src/platform/backends/lxd/lxd_virtual_machine_factory.h index 3d307cc757..7b6ba6fcd1 100644 --- a/src/platform/backends/lxd/lxd_virtual_machine_factory.h +++ b/src/platform/backends/lxd/lxd_virtual_machine_factory.h @@ -52,10 +52,7 @@ class LXDVirtualMachineFactory : public BaseVirtualMachineFactory std::vector networks() const override; - std::string bridge_name_for(const std::string& iface_name) const override - { - return "br-" + iface_name; - }; + std::string bridge_name_for(const std::string& iface_name) const override; protected: void remove_resources_for_impl(const std::string& name) override; From 04ae2b987032b8f636b091c1eb51476ddd0399e6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Luis=20Pe=C3=B1aranda?= Date: Mon, 15 Jan 2024 14:50:17 -0300 Subject: [PATCH 8/8] [tests][lxd] Check returned interface names. --- tests/lxd/test_lxd_backend.cpp | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/tests/lxd/test_lxd_backend.cpp b/tests/lxd/test_lxd_backend.cpp index d93dc3a822..da972ce414 100644 --- a/tests/lxd/test_lxd_backend.cpp +++ b/tests/lxd/test_lxd_backend.cpp @@ -2356,9 +2356,20 @@ TEST_F(LXDBackend, addsNetworkInterface) EXPECT_EQ(times_called, 1u); } -TEST_F(LXDBackend, backendReturnsCorrectBridgeName) +struct LXDNetworkNameTestSuite : LXDBackend, WithParamInterface> { +}; + +TEST_P(LXDNetworkNameTestSuite, backendReturnsCorrectBridgeName) +{ + const auto [name, ret] = GetParam(); + CustomLXDFactory factory{std::move(mock_network_access_manager), data_dir.path(), base_url}; - EXPECT_EQ(factory.bridge_name_for("eth7"), "br-eth7"); + EXPECT_EQ(factory.bridge_name_for(name), ret); } + +INSTANTIATE_TEST_SUITE_P(LXDBackend, + LXDNetworkNameTestSuite, + Values(std::make_pair("enp4s0", "br-enp4s0"), + std::make_pair("enx586d8fd35b6c", "br-enx586d8fd35")));