From 22a1b99e57ba31197fcc4a9233803ba65a66ed49 Mon Sep 17 00:00:00 2001 From: Diogo Gomes Date: Thu, 12 Apr 2018 23:22:52 +0100 Subject: [PATCH] UPnP async (#13666) * moved from miniupnpc to pyupnp-async * update requirements * Tests added * hound * update requirements_test_all.txt * update gen_requirements_all.py * addresses @pvizeli requested changes * address review comments --- homeassistant/components/sensor/upnp.py | 52 ++++---- homeassistant/components/upnp.py | 71 +++++++---- requirements_all.txt | 6 +- requirements_test_all.txt | 3 + script/gen_requirements_all.py | 1 + tests/components/test_upnp.py | 160 +++++++++++++++--------- 6 files changed, 180 insertions(+), 113 deletions(-) diff --git a/homeassistant/components/sensor/upnp.py b/homeassistant/components/sensor/upnp.py index e5acae67916..e0c57ca9ac6 100644 --- a/homeassistant/components/sensor/upnp.py +++ b/homeassistant/components/sensor/upnp.py @@ -6,38 +6,44 @@ https://home-assistant.io/components/sensor.upnp/ """ import logging -from homeassistant.components.upnp import DATA_UPNP, UNITS +from homeassistant.components.upnp import DATA_UPNP, UNITS, CIC_SERVICE from homeassistant.helpers.entity import Entity _LOGGER = logging.getLogger(__name__) +BYTES_RECEIVED = 1 +BYTES_SENT = 2 +PACKETS_RECEIVED = 3 +PACKETS_SENT = 4 + # sensor_type: [friendly_name, convert_unit, icon] SENSOR_TYPES = { - 'byte_received': ['received bytes', True, 'mdi:server-network'], - 'byte_sent': ['sent bytes', True, 'mdi:server-network'], - 'packets_in': ['packets received', False, 'mdi:server-network'], - 'packets_out': ['packets sent', False, 'mdi:server-network'], + BYTES_RECEIVED: ['received bytes', True, 'mdi:server-network'], + BYTES_SENT: ['sent bytes', True, 'mdi:server-network'], + PACKETS_RECEIVED: ['packets received', False, 'mdi:server-network'], + PACKETS_SENT: ['packets sent', False, 'mdi:server-network'], } -def setup_platform(hass, config, add_devices, discovery_info=None): +async def async_setup_platform(hass, config, add_devices, discovery_info=None): """Set up the IGD sensors.""" - upnp = hass.data[DATA_UPNP] + device = hass.data[DATA_UPNP] + service = device.find_first_service(CIC_SERVICE) unit = discovery_info['unit'] add_devices([ - IGDSensor(upnp, t, unit if SENSOR_TYPES[t][1] else None) + IGDSensor(service, t, unit if SENSOR_TYPES[t][1] else '#') for t in SENSOR_TYPES], True) class IGDSensor(Entity): """Representation of a UPnP IGD sensor.""" - def __init__(self, upnp, sensor_type, unit=""): + def __init__(self, service, sensor_type, unit=None): """Initialize the IGD sensor.""" - self._upnp = upnp + self._service = service self.type = sensor_type self.unit = unit - self.unit_factor = UNITS[unit] if unit is not None else 1 + self.unit_factor = UNITS[unit] if unit in UNITS else 1 self._name = 'IGD {}'.format(SENSOR_TYPES[sensor_type][0]) self._state = None @@ -49,9 +55,9 @@ class IGDSensor(Entity): @property def state(self): """Return the state of the device.""" - if self._state is None: - return None - return format(self._state / self.unit_factor, '.1f') + if self._state: + return format(float(self._state) / self.unit_factor, '.1f') + return self._state @property def icon(self): @@ -63,13 +69,13 @@ class IGDSensor(Entity): """Return the unit of measurement of this entity, if any.""" return self.unit - def update(self): + async def async_update(self): """Get the latest information from the IGD.""" - if self.type == "byte_received": - self._state = self._upnp.totalbytereceived() - elif self.type == "byte_sent": - self._state = self._upnp.totalbytesent() - elif self.type == "packets_in": - self._state = self._upnp.totalpacketreceived() - elif self.type == "packets_out": - self._state = self._upnp.totalpacketsent() + if self.type == BYTES_RECEIVED: + self._state = await self._service.get_total_bytes_received() + elif self.type == BYTES_SENT: + self._state = await self._service.get_total_bytes_sent() + elif self.type == PACKETS_RECEIVED: + self._state = await self._service.get_total_packets_received() + elif self.type == PACKETS_SENT: + self._state = await self._service.get_total_packets_sent() diff --git a/homeassistant/components/upnp.py b/homeassistant/components/upnp.py index 960d8f3780e..dd611090c22 100644 --- a/homeassistant/components/upnp.py +++ b/homeassistant/components/upnp.py @@ -6,6 +6,7 @@ https://home-assistant.io/components/upnp/ """ from ipaddress import ip_address import logging +import asyncio import voluptuous as vol @@ -14,7 +15,7 @@ from homeassistant.helpers import config_validation as cv from homeassistant.helpers import discovery from homeassistant.util import get_local_ip -REQUIREMENTS = ['miniupnpc==2.0.2'] +REQUIREMENTS = ['pyupnp-async==0.1.0.1'] DEPENDENCIES = ['http'] _LOGGER = logging.getLogger(__name__) @@ -22,7 +23,7 @@ _LOGGER = logging.getLogger(__name__) DEPENDENCIES = ['api'] DOMAIN = 'upnp' -DATA_UPNP = 'UPNP' +DATA_UPNP = 'upnp_device' CONF_LOCAL_IP = 'local_ip' CONF_ENABLE_PORT_MAPPING = 'port_mapping' @@ -33,6 +34,11 @@ CONF_HASS = 'hass' NOTIFICATION_ID = 'upnp_notification' NOTIFICATION_TITLE = 'UPnP Setup' +IGD_DEVICE = 'urn:schemas-upnp-org:device:InternetGatewayDevice:1' +PPP_SERVICE = 'urn:schemas-upnp-org:service:WANPPPConnection:1' +IP_SERVICE = 'urn:schemas-upnp-org:service:WANIPConnection:1' +CIC_SERVICE = 'urn:schemas-upnp-org:service:WANCommonInterfaceConfig:1' + UNITS = { "Bytes": 1, "KBytes": 1024, @@ -51,8 +57,7 @@ CONFIG_SCHEMA = vol.Schema({ }, extra=vol.ALLOW_EXTRA) -# pylint: disable=import-error, no-member, broad-except, c-extension-no-member -def setup(hass, config): +async def async_setup(hass, config): """Register a port mapping for Home Assistant via UPnP.""" config = config[DOMAIN] host = config.get(CONF_LOCAL_IP) @@ -67,21 +72,35 @@ def setup(hass, config): 'Unable to determine local IP. Add it to your configuration.') return False - import miniupnpc + import pyupnp_async + from pyupnp_async.error import UpnpSoapError - upnp = miniupnpc.UPnP() - hass.data[DATA_UPNP] = upnp - - upnp.discoverdelay = 200 - upnp.discover() - try: - upnp.selectigd() - except Exception: - _LOGGER.exception("Error when attempting to discover an UPnP IGD") + service = None + resp = await pyupnp_async.msearch_first(search_target=IGD_DEVICE) + if not resp: return False - unit = config.get(CONF_UNITS) - discovery.load_platform(hass, 'sensor', DOMAIN, {'unit': unit}, config) + try: + device = await resp.get_device() + hass.data[DATA_UPNP] = device + for _service in device.services: + if _service['serviceType'] == PPP_SERVICE: + service = device.find_first_service(PPP_SERVICE) + if _service['serviceType'] == IP_SERVICE: + service = device.find_first_service(IP_SERVICE) + if _service['serviceType'] == CIC_SERVICE: + unit = config.get(CONF_UNITS) + discovery.load_platform(hass, 'sensor', + DOMAIN, + {'unit': unit}, + config) + except UpnpSoapError as error: + _LOGGER.error(error) + return False + + if not service: + _LOGGER.warning("Could not find any UPnP IGD") + return False port_mapping = config.get(CONF_ENABLE_PORT_MAPPING) if not port_mapping: @@ -98,12 +117,12 @@ def setup(hass, config): if internal == CONF_HASS: internal = internal_port try: - upnp.addportmapping( - external, 'TCP', host, internal, 'Home Assistant', '') + await service.add_port_mapping(internal, external, host, 'TCP', + desc='Home Assistant') registered.append(external) - except Exception: - _LOGGER.exception("UPnP failed to configure port mapping for %s", - external) + _LOGGER.debug("external %s -> %s @ %s", external, internal, host) + except UpnpSoapError as error: + _LOGGER.error(error) hass.components.persistent_notification.create( 'ERROR: tcp port {} is already mapped in your router.' '
Please disable port_mapping in the upnp ' @@ -113,11 +132,13 @@ def setup(hass, config): title=NOTIFICATION_TITLE, notification_id=NOTIFICATION_ID) - def deregister_port(event): + async def deregister_port(event): """De-register the UPnP port mapping.""" - for external in registered: - upnp.deleteportmapping(external, 'TCP') + tasks = [service.delete_port_mapping(external, 'TCP') + for external in registered] + if tasks: + await asyncio.wait(tasks) - hass.bus.listen_once(EVENT_HOMEASSISTANT_STOP, deregister_port) + hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, deregister_port) return True diff --git a/requirements_all.txt b/requirements_all.txt index 8fe360df8e8..d26f8717384 100644 --- a/requirements_all.txt +++ b/requirements_all.txt @@ -517,9 +517,6 @@ mficlient==0.3.0 # homeassistant.components.sensor.miflora miflora==0.3.0 -# homeassistant.components.upnp -miniupnpc==2.0.2 - # homeassistant.components.sensor.mopar motorparts==1.0.2 @@ -1055,6 +1052,9 @@ pytradfri[async]==5.4.2 # homeassistant.components.device_tracker.unifi pyunifi==2.13 +# homeassistant.components.upnp +pyupnp-async==0.1.0.1 + # homeassistant.components.keyboard # pyuserinput==0.1.11 diff --git a/requirements_test_all.txt b/requirements_test_all.txt index 484fd1c39f5..e17cbffe8d6 100644 --- a/requirements_test_all.txt +++ b/requirements_test_all.txt @@ -158,6 +158,9 @@ pythonwhois==2.4.3 # homeassistant.components.device_tracker.unifi pyunifi==2.13 +# homeassistant.components.upnp +pyupnp-async==0.1.0.1 + # homeassistant.components.notify.html5 pywebpush==1.6.0 diff --git a/script/gen_requirements_all.py b/script/gen_requirements_all.py index 708d9dbd30b..27b972dcefa 100755 --- a/script/gen_requirements_all.py +++ b/script/gen_requirements_all.py @@ -76,6 +76,7 @@ TEST_REQUIREMENTS = ( 'pyqwikswitch', 'python-forecastio', 'pyunifi', + 'pyupnp-async', 'pywebpush', 'restrictedpython', 'rflink', diff --git a/tests/components/test_upnp.py b/tests/components/test_upnp.py index e2096d28e58..4956b8a6278 100644 --- a/tests/components/test_upnp.py +++ b/tests/components/test_upnp.py @@ -1,5 +1,4 @@ """Test the UPNP component.""" -import asyncio from collections import OrderedDict from unittest.mock import patch, MagicMock @@ -7,15 +6,64 @@ import pytest from homeassistant.const import EVENT_HOMEASSISTANT_STOP from homeassistant.setup import async_setup_component +from homeassistant.components.upnp import IP_SERVICE, DATA_UPNP + + +class MockService(MagicMock): + """Mock upnp IP service.""" + + async def add_port_mapping(self, *args, **kwargs): + """Original function.""" + self.mock_add_port_mapping(*args, **kwargs) + + async def delete_port_mapping(self, *args, **kwargs): + """Original function.""" + self.mock_delete_port_mapping(*args, **kwargs) + + +class MockDevice(MagicMock): + """Mock upnp device.""" + + def find_first_service(self, *args, **kwargs): + """Original function.""" + self._service = MockService() + return self._service + + def peep_first_service(self): + """Access Mock first service.""" + return self._service + + +class MockResp(MagicMock): + """Mock upnp msearch response.""" + + async def get_device(self, *args, **kwargs): + """Original function.""" + device = MockDevice() + service = {'serviceType': IP_SERVICE} + device.services = [service] + return device @pytest.fixture -def mock_miniupnpc(): - """Mock miniupnpc.""" - mock = MagicMock() +def mock_msearch_first(*args, **kwargs): + """Wrapper to async mock function.""" + async def async_mock_msearch_first(*args, **kwargs): + """Mock msearch_first.""" + return MockResp(*args, **kwargs) - with patch.dict('sys.modules', {'miniupnpc': mock}): - yield mock.UPnP() + with patch('pyupnp_async.msearch_first', new=async_mock_msearch_first): + yield + + +@pytest.fixture +def mock_async_exception(*args, **kwargs): + """Wrapper to async mock function with exception.""" + async def async_mock_exception(*args, **kwargs): + return Exception + + with patch('pyupnp_async.msearch_first', new=async_mock_exception): + yield @pytest.fixture @@ -26,75 +74,66 @@ def mock_local_ip(): yield -@pytest.fixture(autouse=True) -def mock_discovery(): - """Mock discovery of upnp sensor.""" - with patch('homeassistant.components.upnp.discovery'): - yield - - -@asyncio.coroutine -def test_setup_fail_if_no_ip(hass): +async def test_setup_fail_if_no_ip(hass): """Test setup fails if we can't find a local IP.""" with patch('homeassistant.components.upnp.get_local_ip', return_value='127.0.0.1'): - result = yield from async_setup_component(hass, 'upnp', { + result = await async_setup_component(hass, 'upnp', { 'upnp': {} }) assert not result -@asyncio.coroutine -def test_setup_fail_if_cannot_select_igd(hass, mock_local_ip, mock_miniupnpc): +async def test_setup_fail_if_cannot_select_igd(hass, + mock_local_ip, + mock_async_exception): """Test setup fails if we can't find an UPnP IGD.""" - mock_miniupnpc.selectigd.side_effect = Exception - - result = yield from async_setup_component(hass, 'upnp', { + result = await async_setup_component(hass, 'upnp', { 'upnp': {} }) assert not result -@asyncio.coroutine -def test_setup_succeeds_if_specify_ip(hass, mock_miniupnpc): +async def test_setup_succeeds_if_specify_ip(hass, mock_msearch_first): """Test setup succeeds if we specify IP and can't find a local IP.""" with patch('homeassistant.components.upnp.get_local_ip', return_value='127.0.0.1'): - result = yield from async_setup_component(hass, 'upnp', { + result = await async_setup_component(hass, 'upnp', { 'upnp': { 'local_ip': '192.168.0.10' } }) assert result + mock_service = hass.data[DATA_UPNP].peep_first_service() + assert len(mock_service.mock_add_port_mapping.mock_calls) == 1 + mock_service.mock_add_port_mapping.assert_called_once_with( + 8123, 8123, '192.168.0.10', 'TCP', desc='Home Assistant') -@asyncio.coroutine -def test_no_config_maps_hass_local_to_remote_port(hass, mock_miniupnpc): +async def test_no_config_maps_hass_local_to_remote_port(hass, + mock_local_ip, + mock_msearch_first): """Test by default we map local to remote port.""" - result = yield from async_setup_component(hass, 'upnp', { - 'upnp': { - 'local_ip': '192.168.0.10' - } + result = await async_setup_component(hass, 'upnp', { + 'upnp': {} }) assert result - assert len(mock_miniupnpc.addportmapping.mock_calls) == 1 - external, _, host, internal, _, _ = \ - mock_miniupnpc.addportmapping.mock_calls[0][1] - assert host == '192.168.0.10' - assert external == 8123 - assert internal == 8123 + mock_service = hass.data[DATA_UPNP].peep_first_service() + assert len(mock_service.mock_add_port_mapping.mock_calls) == 1 + mock_service.mock_add_port_mapping.assert_called_once_with( + 8123, 8123, '192.168.0.10', 'TCP', desc='Home Assistant') -@asyncio.coroutine -def test_map_hass_to_remote_port(hass, mock_miniupnpc): +async def test_map_hass_to_remote_port(hass, + mock_local_ip, + mock_msearch_first): """Test mapping hass to remote port.""" - result = yield from async_setup_component(hass, 'upnp', { + result = await async_setup_component(hass, 'upnp', { 'upnp': { - 'local_ip': '192.168.0.10', 'ports': { 'hass': 1000 } @@ -102,41 +141,38 @@ def test_map_hass_to_remote_port(hass, mock_miniupnpc): }) assert result - assert len(mock_miniupnpc.addportmapping.mock_calls) == 1 - external, _, host, internal, _, _ = \ - mock_miniupnpc.addportmapping.mock_calls[0][1] - assert external == 1000 - assert internal == 8123 + mock_service = hass.data[DATA_UPNP].peep_first_service() + assert len(mock_service.mock_add_port_mapping.mock_calls) == 1 + mock_service.mock_add_port_mapping.assert_called_once_with( + 8123, 1000, '192.168.0.10', 'TCP', desc='Home Assistant') -@asyncio.coroutine -def test_map_internal_to_remote_ports(hass, mock_miniupnpc): +async def test_map_internal_to_remote_ports(hass, + mock_local_ip, + mock_msearch_first): """Test mapping local to remote ports.""" ports = OrderedDict() ports['hass'] = 1000 ports[1883] = 3883 - result = yield from async_setup_component(hass, 'upnp', { + result = await async_setup_component(hass, 'upnp', { 'upnp': { - 'local_ip': '192.168.0.10', 'ports': ports } }) assert result - assert len(mock_miniupnpc.addportmapping.mock_calls) == 2 - external, _, host, internal, _, _ = \ - mock_miniupnpc.addportmapping.mock_calls[0][1] - assert external == 1000 - assert internal == 8123 + mock_service = hass.data[DATA_UPNP].peep_first_service() + assert len(mock_service.mock_add_port_mapping.mock_calls) == 2 - external, _, host, internal, _, _ = \ - mock_miniupnpc.addportmapping.mock_calls[1][1] - assert external == 3883 - assert internal == 1883 + mock_service.mock_add_port_mapping.assert_any_call( + 8123, 1000, '192.168.0.10', 'TCP', desc='Home Assistant') + mock_service.mock_add_port_mapping.assert_any_call( + 1883, 3883, '192.168.0.10', 'TCP', desc='Home Assistant') hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP) - yield from hass.async_block_till_done() - assert len(mock_miniupnpc.deleteportmapping.mock_calls) == 2 - assert mock_miniupnpc.deleteportmapping.mock_calls[0][1][0] == 1000 - assert mock_miniupnpc.deleteportmapping.mock_calls[1][1][0] == 3883 + await hass.async_block_till_done() + assert len(mock_service.mock_delete_port_mapping.mock_calls) == 2 + + mock_service.mock_delete_port_mapping.assert_any_call(1000, 'TCP') + mock_service.mock_delete_port_mapping.assert_any_call(3883, 'TCP')