diff --git a/include/multipass/network_interface_info.h b/include/multipass/network_interface_info.h index 8aec7dfebf..a51a6a681e 100644 --- a/include/multipass/network_interface_info.h +++ b/include/multipass/network_interface_info.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2020 Canonical, Ltd. + * Copyright (C) 2020-2021 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 @@ -21,13 +21,24 @@ #include #include +#include +#include +#include + namespace multipass { struct NetworkInterfaceInfo { + bool has_link(const std::string& net) const + { + auto end = links.cend(); + return std::find(links.cbegin(), end, net) != end; + } + std::string id; std::string type; std::string description; + std::vector links = {}; // default initializer allows aggregate init of the other 3 }; } // namespace multipass #endif // MULTIPASS_NETWORK_INTERFACE_INFO_H diff --git a/include/multipass/virtual_machine_factory.h b/include/multipass/virtual_machine_factory.h index 2ed9b1bd80..0511872975 100644 --- a/include/multipass/virtual_machine_factory.h +++ b/include/multipass/virtual_machine_factory.h @@ -36,6 +36,7 @@ class URLDownloader; class VirtualMachineDescription; class VMImageHost; class VMStatusMonitor; +struct NetworkInterface; struct NetworkInterfaceInfo; class VirtualMachineFactory @@ -53,6 +54,7 @@ class VirtualMachineFactory virtual void remove_resources_for(const std::string& name) = 0; virtual FetchType fetch_type() = 0; + virtual void prepare_networking(std::vector& extra_interfaces) = 0; // note the arg may be updated virtual VMImage prepare_source_image(const VMImage& source_image) = 0; virtual void prepare_instance_image(const VMImage& instance_image, const VirtualMachineDescription& desc) = 0; virtual void hypervisor_health_check() = 0; diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index b7a928b700..27f2a1dd8f 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -2141,6 +2141,8 @@ void mp::Daemon::create_vm(const CreateRequest* request, grpc::ServerWriterWrite(reply); + config->factory->prepare_networking(checked_args.extra_interfaces); + // This set stores the MAC's which need to be in the allocated_mac_addrs if everything goes well. auto new_macs = allocated_mac_addrs; diff --git a/src/platform/backends/lxd/lxd_virtual_machine_factory.cpp b/src/platform/backends/lxd/lxd_virtual_machine_factory.cpp index 56567b3a96..8afc7a918b 100644 --- a/src/platform/backends/lxd/lxd_virtual_machine_factory.cpp +++ b/src/platform/backends/lxd/lxd_virtual_machine_factory.cpp @@ -19,9 +19,12 @@ #include "lxd_virtual_machine.h" #include "lxd_vm_image_vault.h" +#include + #include #include #include +#include #include #include #include @@ -36,6 +39,7 @@ namespace { constexpr auto category = "lxd factory"; const QString multipass_bridge_name = "mpbr0"; + } // namespace mp::LXDVirtualMachineFactory::LXDVirtualMachineFactory(NetworkAccessManager::UPtr manager, const mp::Path& data_dir, @@ -173,7 +177,8 @@ auto mp::LXDVirtualMachineFactory::networks() const -> std::vectorsecond.description) : lxd_description.toStdString(); - ret.push_back({std::move(id), std::move(type), std::move(description)}); + ret.push_back({std::move(id), std::move(type), std::move(description), + std::move(platform_it->second.links)}); platform_networks.erase(platform_it); // prevent matching with this network again } @@ -184,3 +189,20 @@ auto mp::LXDVirtualMachineFactory::networks() const -> std::vector& extra_interfaces) +{ + auto host_nets = networks(); + for (auto& net : extra_interfaces) + { + auto it = std::find_if(host_nets.cbegin(), host_nets.cend(), + [&net](const mp::NetworkInterfaceInfo& info) { return info.id == net.id; }); + if (it != host_nets.end() && it->type == "ethernet") + { + it = std::find_if(host_nets.cbegin(), host_nets.cend(), [&net](const mp::NetworkInterfaceInfo& info) { + return info.type == "bridge" && info.has_link(net.id); + }); + net.id = it != host_nets.cend() ? it->id : mp::backend::create_bridge_with(net.id); + } + } +} diff --git a/src/platform/backends/lxd/lxd_virtual_machine_factory.h b/src/platform/backends/lxd/lxd_virtual_machine_factory.h index 0c3ef8c130..54521b2b39 100644 --- a/src/platform/backends/lxd/lxd_virtual_machine_factory.h +++ b/src/platform/backends/lxd/lxd_virtual_machine_factory.h @@ -34,6 +34,7 @@ class LXDVirtualMachineFactory final : public BaseVirtualMachineFactory explicit LXDVirtualMachineFactory(NetworkAccessManager::UPtr manager, const Path& data_dir, const QUrl& base_url = lxd_socket_url); + void prepare_networking(std::vector& extra_interfaces) override; VirtualMachine::UPtr create_virtual_machine(const VirtualMachineDescription& desc, VMStatusMonitor& monitor) override; void remove_resources_for(const std::string& name) override; diff --git a/src/platform/backends/shared/base_virtual_machine_factory.h b/src/platform/backends/shared/base_virtual_machine_factory.h index e635ea5f55..6fb2662fbf 100644 --- a/src/platform/backends/shared/base_virtual_machine_factory.h +++ b/src/platform/backends/shared/base_virtual_machine_factory.h @@ -44,6 +44,11 @@ class BaseVirtualMachineFactory : public VirtualMachineFactory return {}; }; + void prepare_networking(std::vector& /*extra_interfaces*/) override + { + // only certain backends need to do anything to prepare networking + } + VMImageVault::UPtr create_image_vault(std::vector image_hosts, URLDownloader* downloader, const Path& cache_dir_path, const Path& data_dir_path, const days& days_to_expire) override diff --git a/src/platform/backends/shared/linux/backend_utils.cpp b/src/platform/backends/shared/linux/backend_utils.cpp index 221f99375e..af1dd7edeb 100644 --- a/src/platform/backends/shared/linux/backend_utils.cpp +++ b/src/platform/backends/shared/linux/backend_utils.cpp @@ -322,7 +322,7 @@ void mp::backend::check_if_kvm_is_in_use() // @precondition no bridge exists for this interface // @precondition interface identifies an ethernet device -void mp::backend::create_bridge_with(const std::string& interface) +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"; @@ -359,7 +359,11 @@ void mp::backend::create_bridge_with(const std::string& interface) for '/' to signal null `device` and `specific-object` derived from nmcli and libnm. See https://bit.ly/3dMA3QB */ rollback_guard.dismiss(); // we succeeded! - mpl::log(mpl::Level::info, log_category_create, fmt::format("Created bridge: {}", parent_name)); + + auto ret = parent_name.toStdString(); + mpl::log(mpl::Level::info, log_category_create, fmt::format("Created bridge: {}", ret)); + + return ret; } mp::backend::CreateBridgeException::CreateBridgeException(const std::string& detail, const QDBusError& dbus_error, diff --git a/src/platform/backends/shared/linux/backend_utils.h b/src/platform/backends/shared/linux/backend_utils.h index 304d909335..5df114206d 100644 --- a/src/platform/backends/shared/linux/backend_utils.h +++ b/src/platform/backends/shared/linux/backend_utils.h @@ -40,7 +40,7 @@ Path convert_to_qcow_if_necessary(const Path& image_path); QString cpu_arch(); void check_for_kvm_support(); void check_if_kvm_is_in_use(); -void create_bridge_with(const std::string& interface); +std::string create_bridge_with(const std::string& interface); class CreateBridgeException : public std::runtime_error { diff --git a/src/platform/platform_linux.cpp b/src/platform/platform_linux.cpp index 7ec82d9c6f..ea69221055 100644 --- a/src/platform/platform_linux.cpp +++ b/src/platform/platform_linux.cpp @@ -110,25 +110,47 @@ bool is_ethernet(const QDir& net_dir) get_net_devtype(net_dir).isEmpty(); } -mp::NetworkInterfaceInfo get_network(const QDir& net_dir) +mp::optional get_network(const QDir& net_dir) { static const auto bridge_fname = QStringLiteral("brif"); + auto id = net_dir.dirName().toStdString(); - std::string type, description; if (auto bridge = "bridge"; net_dir.exists(bridge)) { - type = bridge; + std::vector links; QStringList bridge_members = QDir{net_dir.filePath(bridge_fname)}.entryList(QDir::NoDotAndDotDot | QDir::Dirs); - description = bridge_members.isEmpty() ? "Empty network bridge" - : fmt::format("Network bridge with {}", bridge_members.join(", ")); + + links.reserve(bridge_members.size()); + std::transform(bridge_members.cbegin(), bridge_members.cend(), std::back_inserter(links), + [](const QString& interface) { return interface.toStdString(); }); + + return {{std::move(id), bridge, /*description=*/"", std::move(links)}}; // description needs updating with links } else if (is_ethernet(net_dir)) + return {{std::move(id), "ethernet", "Ethernet device"}}; + + return mp::nullopt; +} + +void update_bridges(std::map& networks) +{ + for (auto& item : networks) { - type = "ethernet"; - description = "Ethernet device"; + if (auto& net = item.second; net.type == "bridge") + { // bridge descriptions and links depend on what other networks we recognized + auto& links = net.links; + auto is_unknown = [&networks](const std::string& id) { + auto same_as = [&id](const auto& other) { return other.first == id; }; + return std::find_if(networks.cbegin(), networks.cend(), same_as) == networks.cend(); + }; + links.erase(std::remove_if(links.begin(), links.end(), is_unknown), + links.end()); // filter links to networks we don't recognize + + net.description = + links.empty() ? "Network bridge" + : fmt::format("Network bridge with {}", fmt::join(links.cbegin(), links.cend(), ", ")); + } } - - return mp::NetworkInterfaceInfo{net_dir.dirName().toStdString(), std::move(type), std::move(description)}; } } // namespace @@ -160,11 +182,15 @@ auto mp::platform::detail::get_network_interfaces_from(const QDir& sys_dir) auto ifaces_info = std::map(); for (const auto& entry : sys_dir.entryList(QDir::NoDotAndDotDot | QDir::Dirs)) { - auto iface = get_network(QDir{sys_dir.filePath(entry)}); - auto name = iface.id; // (can't rely on param evaluation order) - ifaces_info.emplace(std::move(name), std::move(iface)); + if (auto iface = get_network(QDir{sys_dir.filePath(entry)}); iface) + { + auto name = iface->id; // (can't rely on param evaluation order) + ifaces_info.emplace(std::move(name), std::move(*iface)); + } } + update_bridges(ifaces_info); + return ifaces_info; } diff --git a/tests/linux/test_backend_utils.cpp b/tests/linux/test_backend_utils.cpp index 36cd5e421d..67739a5e8e 100644 --- a/tests/linux/test_backend_utils.cpp +++ b/tests/linux/test_backend_utils.cpp @@ -312,7 +312,7 @@ struct CreateBridgeTest : public Test QMap outer_map; QMap::const_iterator outer_it; QVariantMap::const_iterator inner_it; - QString parent_name = get_bridge_name(child).left(15); + QString parent_name = get_bridge_name(child); return (outer_map = arg.value>()).size() == 2 && (outer_it = outer_map.find("connection")) != outer_map.end() && outer_it->size() == 3 && @@ -330,7 +330,7 @@ struct CreateBridgeTest : public Test QMap outer_map; QMap::const_iterator outer_it; QVariantMap::const_iterator inner_it; - QString parent_name = get_bridge_name(child).left(15); + QString parent_name = get_bridge_name(child); QString child_name = parent_name + "-child"; return (outer_map = arg.value>()).size() == 1 && @@ -350,6 +350,11 @@ struct CreateBridgeTest : public Test Property(&QDBusObjectPath::path, Property(&QString::toStdString, Eq(path)))); } + static QString get_bridge_name(const char* child) + { + return (QString{"br-"} + child).left(15); + } + MockDBusProvider::GuardedMock mock_dbus_injection = MockDBusProvider::inject(); MockDBusProvider* mock_dbus_provider = mock_dbus_injection.first; MockDBusConnection mock_bus{}; @@ -358,17 +363,11 @@ struct CreateBridgeTest : public Test mpt::MockLogger::Scope logger_scope = mpt::MockLogger::inject(); const QVariant empty{}; - -private: - static QString get_bridge_name(const char* child) - { - return QString{"br-"} + child; - } }; TEST_F(CreateBridgeTest, creates_and_activates_connections) // success case { - static constexpr auto network = "wlan1234567890a"; + static constexpr auto network = "eth1234567890a"; static constexpr auto child_obj_path = "/an/obj/path/for/child"; static constexpr auto null_obj_path = "/"; @@ -390,7 +389,7 @@ TEST_F(CreateBridgeTest, creates_and_activates_connections) // success case } inject_dbus_interfaces(); - EXPECT_NO_THROW(mp::backend::create_bridge_with(network)); + EXPECT_EQ(mp::backend::create_bridge_with(network), get_bridge_name(network).toStdString()); } TEST_F(CreateBridgeTest, throws_if_bus_disconnected) diff --git a/tests/linux/test_platform_linux.cpp b/tests/linux/test_platform_linux.cpp index 5feb24df1d..43a341f701 100644 --- a/tests/linux/test_platform_linux.cpp +++ b/tests/linux/test_platform_linux.cpp @@ -384,27 +384,6 @@ TEST_P(TestUnsupportedDrivers, test_unsupported_driver) INSTANTIATE_TEST_SUITE_P(PlatformLinux, TestUnsupportedDrivers, Values(QStringLiteral("hyperkit"), QStringLiteral("hyper-v"), QStringLiteral("other"))); -TEST_F(PlatformLinux, retrieves_networks_from_the_system) -{ - const mpt::TempDir tmp_dir; - const auto fake_nets = {"eth0", "foo", "kkkkk"}; - - QDir fake_sys_class_net{tmp_dir.path()}; - for (const auto& net : fake_nets) - ASSERT_TRUE(fake_sys_class_net.mkpath(net)); - - auto net_map = mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()); - ASSERT_EQ(net_map.size(), fake_nets.size()); - - auto expect_it = std::cbegin(fake_nets); - for (const auto& [key, val] : net_map) - { - ASSERT_NE(expect_it, std::cend(fake_nets)); - EXPECT_EQ(key, *expect_it++); - EXPECT_EQ(val.id, key); - } -} - TEST_F(PlatformLinux, retrieves_empty_bridges) { const mpt::TempDir tmp_dir; @@ -419,10 +398,10 @@ TEST_F(PlatformLinux, retrieves_empty_bridges) using value_type = decltype(net_map)::value_type; using Net = mp::NetworkInterfaceInfo; - EXPECT_THAT(net_map, ElementsAre(AllOf(Field(&value_type::first, fake_bridge), - Field(&value_type::second, - AllOf(Field(&Net::id, fake_bridge), Field(&Net::type, "bridge"), - Field(&Net::description, HasSubstr("Empty network bridge"))))))); + EXPECT_THAT(net_map, ElementsAre(AllOf( + Field(&value_type::first, fake_bridge), + Field(&value_type::second, AllOf(Field(&Net::id, fake_bridge), Field(&Net::type, "bridge"), + Field(&Net::description, StrEq("Network bridge"))))))); } TEST_F(PlatformLinux, retrieves_ethernet_devices) @@ -444,7 +423,19 @@ TEST_F(PlatformLinux, retrieves_ethernet_devices) EXPECT_EQ(it->second.description, "Ethernet device"); } -TEST_F(PlatformLinux, does_not_identify_other_virtual) +TEST_F(PlatformLinux, does_not_retrieve_unknown_networks) +{ + const mpt::TempDir tmp_dir; + const auto fake_nets = {"eth0", "foo", "kkkkk"}; + + QDir fake_sys_class_net{tmp_dir.path()}; + for (const auto& net : fake_nets) + ASSERT_TRUE(fake_sys_class_net.mkpath(net)); + + EXPECT_THAT(mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()), IsEmpty()); +} + +TEST_F(PlatformLinux, does_not_retrieve_other_virtual) { const mpt::TempDir tmp_dir; const auto fake_virt = "somevirt"; @@ -452,18 +443,10 @@ TEST_F(PlatformLinux, does_not_identify_other_virtual) QDir fake_sys_class_net{tmp_dir.path() + "/virtual"}; ASSERT_EQ(mpt::make_file_with_content(fake_sys_class_net.filePath(fake_virt) + "/type", "1"), 1); - auto net_map = mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()); - - ASSERT_EQ(net_map.size(), 1u); - - auto it = net_map.cbegin(); - EXPECT_EQ(it->first, fake_virt); - EXPECT_EQ(it->second.id, fake_virt); - EXPECT_TRUE(it->second.type.empty()); - EXPECT_TRUE(it->second.description.empty()); + EXPECT_THAT(mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()), IsEmpty()); } -TEST_F(PlatformLinux, does_not_identify_wireless) +TEST_F(PlatformLinux, does_not_retrieve_wireless) { const mpt::TempDir tmp_dir; const auto fake_wifi = "somewifi"; @@ -473,18 +456,10 @@ TEST_F(PlatformLinux, does_not_identify_wireless) ASSERT_EQ(mpt::make_file_with_content(wifi_dir.filePath("type"), "1"), 1); ASSERT_TRUE(wifi_dir.mkpath("wireless")); - auto net_map = mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()); - - ASSERT_EQ(net_map.size(), 1u); - - auto it = net_map.cbegin(); - EXPECT_EQ(it->first, fake_wifi); - EXPECT_EQ(it->second.id, fake_wifi); - EXPECT_TRUE(it->second.type.empty()); - EXPECT_TRUE(it->second.description.empty()); + EXPECT_THAT(mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()), IsEmpty()); } -TEST_F(PlatformLinux, does_not_identify_protocols) +TEST_F(PlatformLinux, does_not_retrieve_protocols) { const mpt::TempDir tmp_dir; const auto fake_net = "somenet"; @@ -492,18 +467,10 @@ TEST_F(PlatformLinux, does_not_identify_protocols) QDir fake_sys_class_net{tmp_dir.path()}; ASSERT_EQ(mpt::make_file_with_content(fake_sys_class_net.filePath(fake_net) + "/type", "32"), 2); - auto net_map = mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()); - - ASSERT_EQ(net_map.size(), 1u); - - auto it = net_map.cbegin(); - EXPECT_EQ(it->first, fake_net); - EXPECT_EQ(it->second.id, fake_net); - EXPECT_TRUE(it->second.type.empty()); - EXPECT_TRUE(it->second.description.empty()); + EXPECT_THAT(mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()), IsEmpty()); } -TEST_F(PlatformLinux, does_not_identify_other_specified_device_types) +TEST_F(PlatformLinux, does_not_retrieve_other_specified_device_types) { const mpt::TempDir tmp_dir; const auto fake_net = "somenet"; @@ -517,16 +484,10 @@ TEST_F(PlatformLinux, does_not_identify_other_specified_device_types) auto net_map = mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()); - ASSERT_EQ(net_map.size(), 1u); - - auto it = net_map.cbegin(); - EXPECT_EQ(it->first, fake_net); - EXPECT_EQ(it->second.id, fake_net); - EXPECT_TRUE(it->second.type.empty()); - EXPECT_TRUE(it->second.description.empty()); + EXPECT_THAT(mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()), IsEmpty()); } -struct BridgeMemberTest : public PlatformLinux, WithParamInterface> +struct BridgeMemberTest : public PlatformLinux, WithParamInterface>> { }; @@ -543,26 +504,41 @@ TEST_P(BridgeMemberTest, retrieves_bridges_with_members) ASSERT_TRUE(interface_dir.mkpath("bridge")); ASSERT_TRUE(members_dir.mkpath(".")); - std::vector> substrs_expectations{}; - for (const auto& member : GetParam()) + using net_value_type = decltype(MP_PLATFORM.get_network_interfaces_info())::value_type; + std::vector> network_matchers{}; + std::vector> substrs_matchers{}; + for (const auto& [member, recognized] : GetParam()) { + QDir member_dir = fake_sys_class_net.filePath(QString::fromStdString(member)); + ASSERT_TRUE(member_dir.mkpath(".")); ASSERT_TRUE(members_dir.mkpath(QString::fromStdString(member))); - substrs_expectations.push_back(HasSubstr(member)); + + if (recognized) + { + ASSERT_EQ(mpt::make_file_with_content(member_dir.filePath("type"), "1"), 1); + + substrs_matchers.push_back(HasSubstr(member)); + network_matchers.push_back(Field(&net_value_type::first, member)); + } + else + substrs_matchers.push_back(Not(HasSubstr(member))); } auto net_map = mp::platform::detail::get_network_interfaces_from(fake_sys_class_net.path()); - using value_type = decltype(net_map)::value_type; using Net = mp::NetworkInterfaceInfo; + network_matchers.push_back( + AllOf(Field(&net_value_type::first, fake_bridge), + Field(&net_value_type::second, AllOf(Field(&Net::id, fake_bridge), Field(&Net::type, "bridge"), + Field(&Net::description, AllOfArray(substrs_matchers)))))); - EXPECT_THAT(net_map, ElementsAre(AllOf(Field(&value_type::first, fake_bridge), - Field(&value_type::second, - AllOf(Field(&Net::id, fake_bridge), Field(&Net::type, "bridge"), - Field(&Net::description, AllOfArray(substrs_expectations))))))); + EXPECT_THAT(net_map, UnorderedElementsAreArray(network_matchers)); } -INSTANTIATE_TEST_SUITE_P(PlatformLinux, BridgeMemberTest, - Values(std::vector{"en0"}, std::vector{"en0, en1"}, - std::vector{"asdf", "ggi", "a1", "fu"})); +using Param = std::vector>; +INSTANTIATE_TEST_SUITE_P( + PlatformLinux, BridgeMemberTest, + Values(Param{{"en0", true}}, Param{{"en0", false}}, Param{{"en0", false}, {"en1", true}}, + Param{{"asdf", true}, {"ggi", true}, {"a1", true}, {"fu", false}, {"ho", true}, {"ra", false}})); } // namespace diff --git a/tests/mock_virtual_machine_factory.h b/tests/mock_virtual_machine_factory.h index 1a68cb2911..dd7440d156 100644 --- a/tests/mock_virtual_machine_factory.h +++ b/tests/mock_virtual_machine_factory.h @@ -35,6 +35,7 @@ struct MockVirtualMachineFactory : public VirtualMachineFactory MOCK_METHOD1(remove_resources_for, void(const std::string&)); MOCK_METHOD0(fetch_type, FetchType()); + MOCK_METHOD1(prepare_networking, void(std::vector&)); MOCK_METHOD1(prepare_source_image, VMImage(const VMImage&)); MOCK_METHOD2(prepare_instance_image, void(const VMImage&, const VirtualMachineDescription&)); MOCK_METHOD0(hypervisor_health_check, void());