Improve VLAN configuration (#6094)

* Fix NetworkManager connection name for VLANs

The connection name for VLANs should include the parent interface name
for better identification. This was originally the intention, but the
interface object's name property was used which appears empty at that
point.

* Disallow creating multiple connections for the same VLAN id

Only allow a single connection per interface and VLAN id. The regular
network commands can be used to alter the configuration.

* Fix pytest

* Simply connection id name generation

Always rely on the Supervisor interface representation's name attribute
to generate the NetworkManager connection id. Make sure that the name
is correctly set when creating VLAN interfaces as well.

* Special case VLAN configuration

We can't use the match information when comparing Supervisor interface
representation with D-Bus representations. Special case VLAN and
compare using VLAN ID and parent interface.

Note that this currently compares connection UUID of the parent
interface.

* Fix pytest

* Separate VLAN creation logic from apply_changes

Apply changes is really all about updating the NetworkManager settings
of a particular network interface. The base in apply_changes() is
NetworkInterface class, which is the NetworkManager Device abstraction.
All physical interfaces have such a Device hence it is always present.

The only exception is when creating a VLAN: Since it is a virtual
device, there is no device when creating a VLAN.

This separate the two cases. This makes it much easier to reason if
a VLAN already exists or not, and to handle the case where a VLAN
needs to be created.

For all other network interfaces, the apply_changes() method can
now rely on the presence of the NetworkInterface Device abstraction.

* Add VLAN test interface and VLAN exists test

Add a test which checks that an error gets raised when a VLAN for a
particular interface/id combination already exists.

* Address pylint

* Fix test_ignore_veth_only_changes pytest

* Make VLAN interface disabled to avoid test issues

* Reference setting 38 in mocked connection

* Make sure interface type matches

Require a interface type match before doing any comparision.

* Add Supervisor host network configuration tests

* Fix device type checking

* Fix pytest

* Fix tests by taking VLAN interface into account

* Fix test_load_with_network_connection_issues

This seems like a hack, but it turns out that the additional active
connection caused coresys.host.network.update() to be called, which
implicitly "fake" activated the connection. Now it seems that our
mocking causes IPv4 gateway to be set.

So in a way, the test checked a particular mock behavior instead of
actual intention.

The crucial part of this test is that we make sure the settings remain
unchanged. This is done by ensuring that the the method is still auto.

* Fix test_check_network_interface_ipv4.py

Now that we have the VLAN interface active too it will raise an issue
as well.

* Apply suggestions from code review

Co-authored-by: Mike Degatano <[email protected]>

* Fix ruff check issue

---------

Co-authored-by: Mike Degatano <[email protected]>

Author: agners

supervisor/api/network.py

@@ -325,7 +325,7 @@ async def create_vlan(self, request: web.Request) -> None:
             )
 
         vlan_interface = Interface(
-            "",
+            f"{interface.name}.{vlan}",
             "",
             "",
             True,
@@ -339,4 +339,4 @@ async def create_vlan(self, request: web.Request) -> None:
             None,
             vlan_config,
         )
-        await asyncio.shield(self.sys_host.network.apply_changes(vlan_interface))
+        await asyncio.shield(self.sys_host.network.create_vlan(vlan_interface))

supervisor/dbus/network/setting/__init__.py

@@ -273,8 +273,8 @@ async def reload(self):
         if CONF_ATTR_VLAN in data:
             if CONF_ATTR_VLAN_ID in data[CONF_ATTR_VLAN]:
                 self._vlan = VlanProperties(
-                    data[CONF_ATTR_VLAN][CONF_ATTR_VLAN_ID],
-                    data[CONF_ATTR_VLAN].get(CONF_ATTR_VLAN_PARENT),
+                    id=data[CONF_ATTR_VLAN][CONF_ATTR_VLAN_ID],
+                    parent=data[CONF_ATTR_VLAN].get(CONF_ATTR_VLAN_PARENT),
                 )
             else:
                 self._vlan = None

supervisor/dbus/network/setting/generate.py

@@ -177,8 +177,6 @@ def get_connection_from_interface(
     # Generate/Update ID/name
     if not name or not name.startswith("Supervisor"):
         name = f"Supervisor {interface.name}"
-        if interface.type == InterfaceType.VLAN:
-            name = f"{name}.{cast(VlanConfig, interface.vlan).id}"
 
     if interface.type == InterfaceType.ETHERNET:
         iftype = "802-3-ethernet"
@@ -220,7 +218,7 @@ def get_connection_from_interface(
         conn[CONF_ATTR_802_ETHERNET] = {
             CONF_ATTR_802_ETHERNET_ASSIGNED_MAC: Variant("s", "preserve")
         }
-    elif interface.type == "vlan":
+    elif interface.type == InterfaceType.VLAN:
         parent = cast(VlanConfig, interface.vlan).interface
         if (
             parent

supervisor/host/configuration.py

@@ -82,6 +82,11 @@ class VlanConfig:
     """Represent a vlan configuration."""
 
     id: int
+    # Note: On VLAN creation, parent is the interface name, but in the NetworkManager
+    # config the parent is set to the connection UUID in get_connection_from_interface().
+    # On network update (which we call in apply_changes() on VLAN creation), the
+    # parent is then set to that connection UUID in _map_nm_vlan(), hence we always
+    # operate with a connection UUID as interface!
     interface: str | None
 
 
@@ -108,6 +113,25 @@ def equals_dbus_interface(self, inet: NetworkInterface) -> bool:
         if not inet.settings:
             return False
 
+        # Special handling for VLAN interfaces
+        if self.type == InterfaceType.VLAN and inet.type == DeviceType.VLAN:
+            if not self.vlan:
+                raise RuntimeError("VLAN information missing")
+
+            if inet.settings.vlan:
+                # For VLANs, compare by VLAN id and parent interface
+                return (
+                    inet.settings.vlan.id == self.vlan.id
+                    and inet.settings.vlan.parent == self.vlan.interface
+                )
+            return False
+
+        if (self.type, inet.type) not in [
+            (InterfaceType.ETHERNET, DeviceType.ETHERNET),
+            (InterfaceType.WIRELESS, DeviceType.WIRELESS),
+        ]:
+            return False
+
         if inet.settings.match and inet.settings.match.path:
             return inet.settings.match.path == [self.path]
 

supervisor/host/network.py

@@ -5,6 +5,8 @@
 import logging
 from typing import Any
 
+from supervisor.utils.sentry import async_capture_exception
+
 from ..const import ATTR_HOST_INTERNET
 from ..coresys import CoreSys, CoreSysAttributes
 from ..dbus.const import (
@@ -20,7 +22,6 @@
     WirelessMethodType,
 )
 from ..dbus.network.connection import NetworkConnection
-from ..dbus.network.interface import NetworkInterface
 from ..dbus.network.setting.generate import get_connection_from_interface
 from ..exceptions import (
     DBusError,
@@ -209,20 +210,53 @@ async def update(self, *, force_connectivity_check: bool = False):
 
         await self.check_connectivity(force=force_connectivity_check)
 
+    async def create_vlan(self, interface: Interface) -> None:
+        """Create a VLAN interface."""
+        if interface.vlan is None:
+            raise RuntimeError("VLAN information is missing")
+        # For VLAN interfaces, check if one already exists with same ID on same parent
+        try:
+            self.sys_dbus.network.get(interface.name)
+        except NetworkInterfaceNotFound:
+            _LOGGER.debug(
+                "VLAN interface %s does not exist, creating it", interface.name
+            )
+        else:
+            raise HostNetworkError(
+                f"VLAN {interface.vlan.id} already exists on interface {interface.vlan.interface}",
+                _LOGGER.error,
+            )
+
+        settings = get_connection_from_interface(interface, self.sys_dbus.network)
+
+        try:
+            await self.sys_dbus.network.settings.add_connection(settings)
+        except DBusError as err:
+            raise HostNetworkError(
+                f"Can't create new interface: {err}", _LOGGER.error
+            ) from err
+
+        await self.update(force_connectivity_check=True)
+
     async def apply_changes(
         self, interface: Interface, *, update_only: bool = False
     ) -> None:
         """Apply Interface changes to host."""
-        inet: NetworkInterface | None = None
-        with suppress(NetworkInterfaceNotFound):
+        try:
             inet = self.sys_dbus.network.get(interface.name)
+        except NetworkInterfaceNotFound as err:
+            # The API layer (or anybody else) should not pass any updates for
+            # non-existing interfaces.
+            await async_capture_exception(err)
+            raise HostNetworkError(
+                "Requested Network interface update is not possible", _LOGGER.warning
+            ) from err
 
         con: NetworkConnection | None = None
 
         # Update exist configuration
         if (
-            inet
-            and inet.settings
+            inet.settings
             and inet.settings.connection
             and interface.equals_dbus_interface(inet)
             and interface.enabled
@@ -257,7 +291,7 @@ async def apply_changes(
             )
 
         # Create new configuration and activate interface
-        elif inet and interface.enabled:
+        elif interface.enabled:
             _LOGGER.debug("Create new configuration for %s", interface.name)
             settings = get_connection_from_interface(interface, self.sys_dbus.network)
 
@@ -280,7 +314,7 @@ async def apply_changes(
                 ) from err
 
         # Remove config from interface
-        elif inet and not interface.enabled:
+        elif not interface.enabled:
             if not inet.settings:
                 _LOGGER.debug("Interface %s is already disabled.", interface.name)
                 return
@@ -291,16 +325,6 @@ async def apply_changes(
                     f"Can't disable interface {interface.name}: {err}", _LOGGER.error
                 ) from err
 
-        # Create new interface (like vlan)
-        elif not inet:
-            settings = get_connection_from_interface(interface, self.sys_dbus.network)
-
-            try:
-                await self.sys_dbus.network.settings.add_connection(settings)
-            except DBusError as err:
-                raise HostNetworkError(
-                    f"Can't create new interface: {err}", _LOGGER.error
-                ) from err
         else:
             raise HostNetworkError(
                 "Requested Network interface update is not possible", _LOGGER.warning

tests/api/test_network.py

@@ -389,7 +389,7 @@ async def test_api_network_vlan(
     connection = settings_service.AddConnection.calls[0][0]
     assert "uuid" in connection["connection"]
     assert connection["connection"] == {
-        "id": Variant("s", "Supervisor .1"),
+        "id": Variant("s", "Supervisor eth0.1"),
         "type": Variant("s", "vlan"),
         "llmnr": Variant("i", 2),
         "mdns": Variant("i", 2),
@@ -403,6 +403,16 @@ async def test_api_network_vlan(
         "parent": Variant("s", "0c23631e-2118-355c-bbb0-8943229cb0d6"),
     }
 
+    # Check if trying to recreate an existing VLAN raises an exception
+    result = await resp.json()
+    resp = await api_client.post(
+        f"/network/interface/{TEST_INTERFACE_ETH_NAME}/vlan/10",
+        json={"ipv4": {"method": "auto"}},
+    )
+    result = await resp.json()
+    assert result["result"] == "error"
+    assert len(settings_service.AddConnection.calls) == 1
+
 
 @pytest.mark.parametrize(
     ("method", "url"),

tests/conftest.py

@@ -64,11 +64,17 @@
 from .const import TEST_ADDON_SLUG
 from .dbus_service_mocks.base import DBusServiceMock
 from .dbus_service_mocks.network_connection_settings import (
+    DEFAULT_OBJECT_PATH as DEFAULT_CONNECTION_SETTINGS_OBJECT_PATH,
     ConnectionSettings as ConnectionSettingsService,
 )
 from .dbus_service_mocks.network_dns_manager import DnsManager as DnsManagerService
 from .dbus_service_mocks.network_manager import NetworkManager as NetworkManagerService
 
+from tests.dbus_service_mocks.network_active_connection import (
+    DEFAULT_OBJECT_PATH as DEFAULT_ACTIVE_CONNECTION_OBJECT_PATH,
+    ActiveConnection as ActiveConnectionService,
+)
+
 # pylint: disable=redefined-outer-name, protected-access
 
 
@@ -191,13 +197,21 @@ async def fixture_network_manager_services(
                 "/org/freedesktop/NetworkManager/AccessPoint/43099",
                 "/org/freedesktop/NetworkManager/AccessPoint/43100",
             ],
-            "network_active_connection": None,
-            "network_connection_settings": None,
+            "network_active_connection": [
+                "/org/freedesktop/NetworkManager/ActiveConnection/1",
+                "/org/freedesktop/NetworkManager/ActiveConnection/38",
+            ],
+            "network_connection_settings": [
+                "/org/freedesktop/NetworkManager/Settings/1",
+                "/org/freedesktop/NetworkManager/Settings/38",
+            ],
             "network_device_wireless": None,
             "network_device": [
                 "/org/freedesktop/NetworkManager/Devices/1",
                 "/org/freedesktop/NetworkManager/Devices/3",
+                "/org/freedesktop/NetworkManager/Devices/38",
             ],
+            "network_device_vlan": None,
             "network_dns_manager": None,
             "network_ip4config": None,
             "network_ip6config": None,
@@ -235,12 +249,24 @@ async def dns_manager_service(
     yield network_manager_services["network_dns_manager"]
 
 
+@pytest.fixture(name="active_connection_service")
+async def fixture_active_connection_service(
+    network_manager_services: dict[str, DBusServiceMock | dict[str, DBusServiceMock]],
+) -> ActiveConnectionService:
+    """Return mock active connection service."""
+    yield network_manager_services["network_active_connection"][
+        DEFAULT_ACTIVE_CONNECTION_OBJECT_PATH
+    ]
+
+
 @pytest.fixture(name="connection_settings_service")
 async def fixture_connection_settings_service(
     network_manager_services: dict[str, DBusServiceMock | dict[str, DBusServiceMock]],
 ) -> ConnectionSettingsService:
     """Return mock connection settings service."""
-    yield network_manager_services["network_connection_settings"]
+    yield network_manager_services["network_connection_settings"][
+        DEFAULT_CONNECTION_SETTINGS_OBJECT_PATH
+    ]
 
 
 @pytest.fixture(name="udisks2_services")

tests/dbus/network/setting/test_generate.py

@@ -48,7 +48,7 @@ async def test_get_connection_no_path(network_manager: NetworkManager):
 async def test_generate_from_vlan(network_manager: NetworkManager):
     """Test generate from a vlan interface."""
     vlan_interface = Interface(
-        name="",
+        name="eth0.1",
         mac="",
         path="",
         enabled=True,
@@ -64,7 +64,7 @@ async def test_generate_from_vlan(network_manager: NetworkManager):
     )
 
     connection_payload = get_connection_from_interface(vlan_interface, network_manager)
-    assert connection_payload["connection"]["id"].value == "Supervisor .1"
+    assert connection_payload["connection"]["id"].value == "Supervisor eth0.1"
     assert connection_payload["connection"]["type"].value == "vlan"
     assert "uuid" in connection_payload["connection"]
     assert "match" not in connection_payload["connection"]

tests/dbus/network/setting/test_init.py

@@ -15,7 +15,6 @@
 from supervisor.host.const import InterfaceMethod
 from supervisor.host.network import Interface
 
-from tests.dbus_service_mocks.base import DBusServiceMock
 from tests.dbus_service_mocks.network_connection_settings import (
     ConnectionSettings as ConnectionSettingsService,
 )
@@ -24,13 +23,7 @@
     WIRELESS_DEVICE_OBJECT_PATH,
 )
 
-
-@pytest.fixture(name="connection_settings_service", autouse=True)
-async def fixture_connection_settings_service(
-    network_manager_services: dict[str, DBusServiceMock | dict[str, DBusServiceMock]],
-) -> ConnectionSettingsService:
-    """Mock Connection Settings service."""
-    yield network_manager_services["network_connection_settings"]
+pytestmark = pytest.mark.usefixtures("connection_settings_service")
 
 
 @pytest.fixture(name="dbus_interface")

tests/dbus/network/test_connection.py

@@ -8,18 +8,11 @@
 from supervisor.dbus.network.connection import NetworkConnection
 
 from tests.const import TEST_INTERFACE_ETH_NAME
-from tests.dbus_service_mocks.base import DBusServiceMock
 from tests.dbus_service_mocks.network_active_connection import (
     ActiveConnection as ActiveConnectionService,
 )
 
-
-@pytest.fixture(name="active_connection_service", autouse=True)
-async def fixture_active_connection_service(
-    network_manager_services: dict[str, DBusServiceMock | dict[str, DBusServiceMock]],
-) -> ActiveConnectionService:
-    """Mock Active Connection service."""
-    yield network_manager_services["network_active_connection"]
+pytestmark = pytest.mark.usefixtures("active_connection_service")
 
 
 async def test_active_connection(