Merge pull request #3197 from canonical/add-lxd-interfaces

Add network interfaces on LXD backend

include/multipass/virtual_machine.h

@@ -20,6 +20,7 @@
 
 #include "disabled_copy_move.h"
 #include "ip_address.h"
+#include "network_interface.h"
 
 #include <chrono>
 #include <condition_variable>
@@ -77,6 +78,7 @@ class VirtualMachine : private DisabledCopyMove
     virtual void update_cpus(int num_cores) = 0;
     virtual void resize_memory(const MemorySize& new_size) = 0;
     virtual void resize_disk(const MemorySize& new_size) = 0;
+    virtual void add_network_interface(int index, const NetworkInterface& net) = 0;
     virtual std::unique_ptr<MountHandler> make_native_mount_handler(const SSHKeyProvider* ssh_key_provider,
                                                                     const std::string& target,
                                                                     const VMMount& mount) = 0;

src/daemon/daemon.cpp

@@ -251,7 +251,8 @@ std::vector<mp::NetworkInterface> read_extra_interfaces(const QJsonObject& recor
         {
             auto id = entry.toObject()["id"].toString().toStdString();
             auto mac_address = entry.toObject()["mac_address"].toString().toStdString();
-            if (!mpu::valid_mac_address(mac_address))
+            // Allow empty addresses (for nonconfigured interfaces).
+            if (!mac_address.empty() && !mpu::valid_mac_address(mac_address))
             {
                 throw std::runtime_error(fmt::format("Invalid MAC address {}", mac_address));
             }
@@ -508,12 +509,7 @@ std::vector<mp::NetworkInterface> validate_extra_interfaces(const mp::LaunchRequ
 
         if (net_id == mp::bridged_network_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 {}=<name>` to use the `--bridged` shortcut.",
-                                mp::bridged_interface_key));
-            net_id = bridged_id.toStdString();
+            net_id = get_bridged_interface_name();
         }
 
         if (!factory_networks)
@@ -1990,6 +1986,8 @@ try // clang-format on
                 mpl::log(mpl::Level::error, category, "Mounts have been disabled on this instance of Multipass");
             }
 
+            configure_new_interfaces(vm, vm_instance_specs[name]);
+
             vm.start();
         }
 
@@ -2874,6 +2872,33 @@ mp::MountHandler::UPtr mp::Daemon::make_mount(VirtualMachine* vm, const std::str
                : vm->make_native_mount_handler(config->ssh_key_provider.get(), target, mount);
 }
 
+void mp::Daemon::configure_new_interfaces(mp::VirtualMachine& vm, mp::VMSpecs& specs)
+{
+    std::vector<size_t> new_interfaces;
+
+    for (auto i = 0u; i < specs.extra_interfaces.size(); ++i)
+    {
+        auto& interface = specs.extra_interfaces[i];
+
+        if (interface.mac_address.empty()) // An empty MAC address means the interface needs to be configured.
+        {
+            new_interfaces.push_back(i);
+
+            interface.mac_address = generate_unused_mac_address(allocated_mac_addrs);
+        }
+    }
+
+    if (!new_interfaces.empty())
+    {
+        config->factory->prepare_networking(specs.extra_interfaces);
+
+        for (const auto i : new_interfaces)
+        {
+            vm.add_network_interface(i, specs.extra_interfaces[i]);
+        }
+    }
+}
+
 QFutureWatcher<mp::Daemon::AsyncOperationStatus>*
 mp::Daemon::create_future_watcher(std::function<void()> const& finished_op)
 {

src/daemon/daemon.h

@@ -146,6 +146,7 @@ public slots:
     void init_mounts(const std::string& name);
     void stop_mounts(const std::string& name);
     MountHandler::UPtr make_mount(VirtualMachine* vm, const std::string& target, const VMMount& mount);
+    void configure_new_interfaces(VirtualMachine& vm, VMSpecs& specs);
 
     struct AsyncOperationStatus
     {

src/daemon/instance_settings_handler.cpp

@@ -173,7 +173,9 @@ void update_bridged(const QString& key, const QString& val, mp::VirtualMachine&
 
     if (bridged)
     {
-        // TODO: add the interface
+        // TODO: in this first approach, we add an interface with manual configuration.
+        // The empty string in the MAC indicates the daemon that the interface must be configured.
+        spec.extra_interfaces.push_back({br_interface, "", false});
     }
 }
 
@@ -197,7 +199,6 @@ mp::InstanceSettingsHandler::InstanceSettingsHandler(
       preparing_instances{preparing_instances},
       instance_persister{std::move(instance_persister)},
       bridged_interface{std::move(bridged_interface)}
-
 {
 }
 

src/platform/backends/lxd/lxd_virtual_machine.cpp

@@ -471,6 +471,22 @@ void mp::LXDVirtualMachine::resize_disk(const MemorySize& new_size)
     lxd_request(manager, "PATCH", url(), patch_json);
 }
 
+void mp::LXDVirtualMachine::add_network_interface(int index, const mp::NetworkInterface& net)
+{
+    assert(manager);
+
+    auto net_name = QStringLiteral("eth%1").arg(index + 1);
+    QJsonObject net_config{{"name", net_name},
+                           {"nictype", "bridged"},
+                           {"parent", QString::fromStdString(net.id)},
+                           {"type", "nic"},
+                           {"hwaddr", QString::fromStdString(net.mac_address)}};
+
+    QJsonObject patch_json{{"devices", QJsonObject{{net_name, net_config}}}};
+
+    lxd_request(manager, "PATCH", url(), patch_json);
+}
+
 std::unique_ptr<multipass::MountHandler>
 mp::LXDVirtualMachine::make_native_mount_handler(const SSHKeyProvider* ssh_key_provider, const std::string& target,
                                                  const VMMount& mount)

src/platform/backends/lxd/lxd_virtual_machine.h

@@ -53,6 +53,7 @@ class LXDVirtualMachine : public BaseVirtualMachine
     void update_cpus(int num_cores) override;
     void resize_memory(const MemorySize& new_size) override;
     void resize_disk(const MemorySize& new_size) override;
+    void add_network_interface(int index, const NetworkInterface& net) override;
     std::unique_ptr<MountHandler> make_native_mount_handler(const SSHKeyProvider* ssh_key_provider,
                                                             const std::string& target, const VMMount& mount) override;
 

src/platform/backends/shared/base_virtual_machine.h

@@ -42,6 +42,10 @@ class BaseVirtualMachine : public VirtualMachine
     BaseVirtualMachine(const std::string& vm_name) : VirtualMachine(vm_name){};
 
     std::vector<std::string> get_all_ipv4(const SSHKeyProvider& key_provider) override;
+    void add_network_interface(int index, const NetworkInterface& net) override
+    {
+        throw NotImplementedOnThisBackendException("bridging");
+    }
     std::unique_ptr<MountHandler> make_native_mount_handler(const SSHKeyProvider* ssh_key_provider,
                                                             const std::string& target,
                                                             const multipass::VMMount& mount) override

tests/lxd/test_lxd_backend.cpp

@@ -2191,3 +2191,48 @@ TEST_F(LXDBackend, createsBridgesViaBackendUtils)
     EXPECT_CALL(*mock_backend, create_bridge_with(net.id)).WillOnce(Return(bridge));
     EXPECT_EQ(factory.create_bridge_with(net), bridge);
 }
+
+TEST_F(LXDBackend, addsNetworkInterface)
+{
+    mpt::StubVMStatusMonitor stub_monitor;
+    unsigned times_called = 0;
+
+    EXPECT_CALL(*mock_network_access_manager, createRequest(_, _, _))
+        .WillRepeatedly([&times_called](auto, auto request, auto outgoingData) {
+            outgoingData->open(QIODevice::ReadOnly);
+            auto data = outgoingData->readAll();
+            auto op = request.attribute(QNetworkRequest::CustomVerbAttribute).toString();
+            auto url = request.url().toString();
+
+            if (op == "GET" && url.contains("1.0/virtual-machines/pied-piper-valley/state"))
+            {
+                return new mpt::MockLocalSocketReply(mpt::vm_state_fully_running_data);
+            }
+            else if (op == "PUT" && url.contains("1.0/virtual-machines/pied-piper-valley/state") &&
+                     data.contains("stop"))
+            {
+                return new mpt::MockLocalSocketReply(mpt::stop_vm_data);
+            }
+            else if (op == "PATCH")
+            {
+                ++times_called;
+
+                EXPECT_EQ(data.toStdString(), "{\"devices\":{\"eth2\":{\"hwaddr\":\"52:54:00:56:78:90\",\"name\":"
+                                              "\"eth2\",\"nictype\":\"bridged\",\"parent\":\"id\",\"type\":\"nic\"}}}");
+
+                return new mpt::MockLocalSocketReply(mpt::stop_vm_data);
+            }
+
+            return new mpt::MockLocalSocketReply(mpt::not_found_data, QNetworkReply::ContentNotFoundError);
+        });
+
+    mp::LXDVirtualMachineFactory backend{std::move(mock_network_access_manager), data_dir.path(), base_url};
+
+    auto machine = backend.create_virtual_machine(default_description, stub_monitor);
+
+    machine->stop();
+
+    machine->add_network_interface(1, {"id", "52:54:00:56:78:90", true});
+
+    EXPECT_EQ(times_called, 1u);
+}

tests/mock_virtual_machine.h

@@ -64,6 +64,7 @@ struct MockVirtualMachineT : public T
     MOCK_METHOD(void, update_cpus, (int num_cores), (override));
     MOCK_METHOD(void, resize_memory, (const MemorySize& new_size), (override));
     MOCK_METHOD(void, resize_disk, (const MemorySize& new_size), (override));
+    MOCK_METHOD(void, add_network_interface, (int, const NetworkInterface&), (override));
     MOCK_METHOD(std::unique_ptr<MountHandler>, make_native_mount_handler,
                 (const SSHKeyProvider* ssh_key_provider, const std::string& target, const VMMount& mount), (override));
 };

tests/stub_virtual_machine.h

@@ -111,6 +111,10 @@ struct StubVirtualMachine final : public multipass::VirtualMachine
     {
     }
 
+    void add_network_interface(int, const NetworkInterface&) override
+    {
+    }
+
     std::unique_ptr<MountHandler> make_native_mount_handler(const SSHKeyProvider* ssh_key_provider,
                                                             const std::string& target, const VMMount& mount) override
     {

tests/test_base_virtual_machine.cpp

@@ -148,6 +148,14 @@ TEST_F(BaseVM, get_all_ipv4_works_when_instance_is_off)
     EXPECT_EQ(base_vm.get_all_ipv4(key_provider).size(), 0u);
 }
 
+TEST_F(BaseVM, add_network_interface_throws)
+{
+    StubBaseVirtualMachine base_vm(mp::VirtualMachine::State::off);
+
+    MP_EXPECT_THROW_THAT(base_vm.add_network_interface(1, {"eth1", "52:54:00:00:00:00", true}),
+                         mp::NotImplementedOnThisBackendException, mpt::match_what(HasSubstr("bridging")));
+}
+
 struct IpTestParams
 {
     int exit_status;

tests/test_daemon.cpp

@@ -1914,7 +1914,7 @@ TEST_F(Daemon, refuses_launch_bridged_without_setting)
     std::stringstream err_stream;
     send_command({"launch", "--network", "bridged"}, trash_stream, err_stream);
     EXPECT_THAT(err_stream.str(),
-                HasSubstr("You have to `multipass set local.bridged-network=<name>` to use the `--bridged` shortcut."));
+                HasSubstr("You have to `multipass set local.bridged-network=<name>` to use the \"bridged\" shortcut."));
 }
 
 TEST_F(Daemon, refuses_launch_with_invalid_bridged_interface)

tests/test_daemon_start.cpp

@@ -81,6 +81,35 @@ TEST_F(TestDaemonStart, successfulStartOkStatus)
     EXPECT_TRUE(status.ok());
 }
 
+TEST_F(TestDaemonStart, startConfiguresInterfaces)
+{
+    std::vector<mp::NetworkInterface> unconfigured{{"eth7", "", true}};
+    auto mock_factory = use_a_mock_vm_factory();
+    const auto [temp_dir, filename] = plant_instance_json(fake_json_contents(mac_addr, unconfigured));
+
+    auto instance_ptr = std::make_unique<NiceMock<mpt::MockVirtualMachine>>(mock_instance_name);
+    EXPECT_CALL(*mock_factory, create_virtual_machine(_, _)).WillOnce([&instance_ptr](const auto&, auto&) {
+        return std::move(instance_ptr);
+    });
+    EXPECT_CALL(*instance_ptr, wait_until_ssh_up(_)).WillRepeatedly(Return());
+    EXPECT_CALL(*instance_ptr, current_state()).WillRepeatedly(Return(mp::VirtualMachine::State::off));
+    EXPECT_CALL(*instance_ptr, start()).Times(1);
+    EXPECT_CALL(*instance_ptr, add_network_interface(_, _)).Times(1);
+
+    config_builder.data_directory = temp_dir->path();
+    config_builder.vault = std::make_unique<NiceMock<mpt::MockVMImageVault>>();
+
+    mp::Daemon daemon{config_builder.build()};
+
+    mp::StartRequest request;
+    request.mutable_instance_names()->add_instance_name(mock_instance_name);
+
+    auto status = call_daemon_slot(daemon, &mp::Daemon::start, request,
+                                   StrictMock<mpt::MockServerReaderWriter<mp::StartReply, mp::StartRequest>>{});
+
+    EXPECT_TRUE(status.ok());
+}
+
 TEST_F(TestDaemonStart, unknownStateDoesNotStart)
 {
     auto mock_factory = use_a_mock_vm_factory();

tests/test_instance_settings_handler.cpp

@@ -466,6 +466,20 @@ TEST_F(TestInstanceSettingsHandler, setRefusesToUnbridge)
                          mpt::match_what(HasSubstr("Bridged interface cannot be removed")));
 }
 
+TEST_F(TestInstanceSettingsHandler, setAddsInterface)
+{
+    constexpr auto target_instance_name = "pappo";
+    specs.insert({{"blues", {}}, {"local", {}}, {target_instance_name, {}}});
+    specs[target_instance_name].extra_interfaces = {{"id", "52:54:00:45:67:89", true}};
+
+    mock_vm(target_instance_name); // TODO: make this an expectation.
+
+    make_handler().set(make_key(target_instance_name, "bridged"), "true");
+
+    EXPECT_EQ(specs[target_instance_name].extra_interfaces.size(), 2u);
+    EXPECT_EQ(specs[target_instance_name].extra_interfaces[1].mac_address, "");
+}
+
 using VMSt = mp::VirtualMachine::State;
 using Property = const char*;
 using PropertyAndState = std::tuple<Property, VMSt>; // no subliminal political msg intended :)