Merge #3357

Fix `get local.<instance>.bridged` command
canonical · Jan 18, 2024 · 445ae7c · 445ae7c
2 parents 0b7e88a + 04ae2b9
commit 445ae7c
Show file tree

Hide file tree

Showing 13 changed files with 88 additions and 10 deletions.
diff --git 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<NetworkInterfaceInfo> 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
@@ -1239,6 +1239,7 @@ mp::SettingsHandler* register_instance_mod(std::unordered_map<std::string, mp::V
                                            const std::unordered_set<std::string>& preparing_instances,
                                            std::function<void()> instance_persister,
                                            std::function<std::string()> bridged_interface,
+                                           std::function<std::string()> bridge_name,
                                            std::function<std::vector<mp::NetworkInterfaceInfo>()> host_networks)
 {
     return MP_SETTINGS.register_handler(std::make_unique<mp::InstanceSettingsHandler>(vm_instance_specs,
@@ -1247,6 +1248,7 @@ mp::SettingsHandler* register_instance_mod(std::unordered_map<std::string, mp::V
                                                                                       preparing_instances,
                                                                                       std::move(instance_persister),
                                                                                       std::move(bridged_interface),
+                                                                                      std::move(bridge_name),
                                                                                       host_networks));
 }
 
@@ -1405,6 +1407,9 @@ mp::Daemon::Daemon(std::unique_ptr<const DaemonConfig> 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
@@ -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<std::string()> bridged_interface,
+                    std::function<std::string()> bridge_name,
                     std::function<std::vector<mp::NetworkInterfaceInfo>()> 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,13 +226,15 @@ mp::InstanceSettingsHandler::InstanceSettingsHandler(
     const std::unordered_set<std::string>& preparing_instances,
     std::function<void()> instance_persister,
     std::function<std::string()> bridged_interface,
+    std::function<std::string()> bridge_name,
     std::function<std::vector<NetworkInterfaceInfo>()> host_networks)
     : vm_instance_specs{vm_instance_specs},
       operative_instances{operative_instances},
       deleted_instances{deleted_instances},
       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
@@ -44,6 +44,7 @@ class InstanceSettingsHandler : public SettingsHandler
                             const std::unordered_set<std::string>& preparing_instances,
                             std::function<void()> instance_persister,
                             std::function<std::string()> bridged_interface,
+                            std::function<std::string()> bridge_name,
                             std::function<std::vector<NetworkInterfaceInfo>()> host_networks);
 
     std::set<QString> keys() const override;
@@ -63,6 +64,7 @@ class InstanceSettingsHandler : public SettingsHandler
     const std::unordered_set<std::string>& preparing_instances;
     std::function<void()> instance_persister;
     std::function<std::string()> bridged_interface;
+    std::function<std::string()> bridge_name;
     std::function<std::vector<NetworkInterfaceInfo>()> host_networks;
 };
 

diff --git 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<NetworkInterf
     return ret;
 }
 
+std::string mp::LXDVirtualMachineFactory::bridge_name_for(const std::string& iface_name) const
+{
+    return MP_BACKEND.bridge_name(iface_name);
+};
+
 void mp::LXDVirtualMachineFactory::prepare_networking(std::vector<NetworkInterface>& 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
@@ -52,6 +52,8 @@ class LXDVirtualMachineFactory : public BaseVirtualMachineFactory
 
     std::vector<NetworkInterfaceInfo> networks() const override;
 
+    std::string bridge_name_for(const std::string& iface_name) const override;
+
 protected:
     void remove_resources_for_impl(const std::string& name) override;
     std::string create_bridge_with(const NetworkInterfaceInfo& interface) override;

diff --git 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;
 

diff --git a/src/platform/backends/shared/linux/backend_utils.cpp b/src/platform/backends/shared/linux/backend_utils.cpp
@@ -196,14 +196,22 @@ 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)
 {
     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<VariantMapMap>(); });
@@ -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
@@ -51,6 +51,7 @@ class Backend : public Singleton<Backend>
 public:
     using Singleton<Backend>::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;
 

diff --git a/tests/lxd/test_lxd_backend.cpp b/tests/lxd/test_lxd_backend.cpp
@@ -2355,3 +2355,21 @@ TEST_F(LXDBackend, addsNetworkInterface)
 
     EXPECT_EQ(times_called, 1u);
 }
+
+struct LXDNetworkNameTestSuite : LXDBackend, WithParamInterface<std::pair<std::string, std::string>>
+{
+};
+
+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(name), ret);
+}
+
+INSTANTIATE_TEST_SUITE_P(LXDBackend,
+                         LXDNetworkNameTestSuite,
+                         Values(std::make_pair("enp4s0", "br-enp4s0"),
+                                std::make_pair("enx586d8fd35b6c", "br-enx586d8fd35")));
diff --git 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<NetworkInterfaceInfo>, 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_base_virtual_machine_factory.cpp b/tests/test_base_virtual_machine_factory.cpp
@@ -54,13 +54,19 @@ struct MockBaseFactory : mp::BaseVirtualMachineFactory
     MOCK_METHOD(QString, get_backend_version_string, (), (const, override));
     MOCK_METHOD(void, prepare_networking, (std::vector<mp::NetworkInterface>&), (override));
     MOCK_METHOD(std::vector<mp::NetworkInterfaceInfo>, 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<mp::NetworkInterfaceInfo>& host_nets,
                  const std::string& bridge_type),
                 (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<MockBaseFactory> factory;
+
+    ASSERT_EQ(factory.base_bridge_name_for("any_interface"), "");
+}
+
 TEST_F(BaseFactory, create_bridge_not_implemented)
 {
     StrictMock<MockBaseFactory> factory;

diff --git 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<std::string()> make_fake_bridge_name()
+    {
+        return [] { return "br-eth8"; };
+    }
+
     std::function<std::vector<mp::NetworkInterfaceInfo>()> make_fake_host_networks()
     {
         std::vector<mp::NetworkInterfaceInfo> 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)
 {