IGD review fixes (#17400)
* Fix discovery-dependency for upnp * Only delete port mappings on EVENT_HOMEASSISTANT_STOP + refactoring MockDevice * Call and store local_ip from async_setup * Don't depend on http-component * Remove discovery dependency
This commit is contained in:
Steven Looman
Paulus Schoutsen
parent
0f69be117f
commit
fc8af22191
3 changed files with 118 additions and 67 deletions
|
|
@ -16,7 +16,7 @@ from homeassistant.helpers import config_validation as cv
|
|||
from homeassistant.helpers import dispatcher
|
||||
from homeassistant.helpers.typing import ConfigType
|
||||
from homeassistant.helpers.typing import HomeAssistantType
|
||||
from homeassistant.components.discovery import DOMAIN as DISCOVERY_DOMAIN
|
||||
from homeassistant.util import get_local_ip
|
||||
|
||||
from .const import (
|
||||
CONF_ENABLE_PORT_MAPPING, CONF_ENABLE_SENSORS,
|
||||
|
|
@ -31,7 +31,6 @@ from .device import Device
|
|||
|
||||
|
||||
REQUIREMENTS = ['async-upnp-client==0.12.7']
|
||||
DEPENDENCIES = ['http']
|
||||
|
||||
NOTIFICATION_ID = 'upnp_notification'
|
||||
NOTIFICATION_TITLE = 'UPnP/IGD Setup'
|
||||
|
|
@ -50,18 +49,37 @@ CONFIG_SCHEMA = vol.Schema({
|
|||
}, extra=vol.ALLOW_EXTRA)
|
||||
|
||||
|
||||
def _substitute_hass_ports(ports, hass_port):
|
||||
"""Substitute 'hass' for the hass_port."""
|
||||
def _substitute_hass_ports(ports, hass_port=None):
|
||||
"""
|
||||
Substitute 'hass' for the hass_port.
|
||||
|
||||
This triggers a warning when hass_port is None.
|
||||
"""
|
||||
ports = ports.copy()
|
||||
|
||||
# substitute 'hass' for hass_port, both keys and values
|
||||
if CONF_HASS in ports:
|
||||
ports[hass_port] = ports[CONF_HASS]
|
||||
if hass_port is None:
|
||||
_LOGGER.warning(
|
||||
'Could not determine Home Assistant http port, '
|
||||
'not setting up port mapping from %s to %s. '
|
||||
'Enable the http-component.',
|
||||
CONF_HASS, ports[CONF_HASS])
|
||||
else:
|
||||
ports[hass_port] = ports[CONF_HASS]
|
||||
del ports[CONF_HASS]
|
||||
|
||||
for port in ports:
|
||||
if ports[port] == CONF_HASS:
|
||||
ports[port] = hass_port
|
||||
if hass_port is None:
|
||||
_LOGGER.warning(
|
||||
'Could not determine Home Assistant http port, '
|
||||
'not setting up port mapping from %s to %s. '
|
||||
'Enable the http-component.',
|
||||
port, ports[port])
|
||||
del ports[port]
|
||||
else:
|
||||
ports[port] = hass_port
|
||||
|
||||
return ports
|
||||
|
||||
|
|
@ -74,15 +92,14 @@ async def async_setup(hass: HomeAssistantType, config: ConfigType):
|
|||
# ensure sane config
|
||||
if DOMAIN not in config:
|
||||
return True
|
||||
|
||||
if DISCOVERY_DOMAIN not in config:
|
||||
_LOGGER.warning('UPNP needs discovery, please enable it')
|
||||
return False
|
||||
upnp_config = config[DOMAIN]
|
||||
|
||||
# overridden local ip
|
||||
upnp_config = config[DOMAIN]
|
||||
if CONF_LOCAL_IP in upnp_config:
|
||||
hass.data[DOMAIN]['local_ip'] = upnp_config[CONF_LOCAL_IP]
|
||||
else:
|
||||
hass.data[DOMAIN]['local_ip'] = \
|
||||
await hass.async_add_executor_job(get_local_ip)
|
||||
|
||||
# determine ports
|
||||
ports = {CONF_HASS: CONF_HASS} # default, port_mapping disabled by default
|
||||
|
|
@ -119,13 +136,15 @@ async def async_setup_entry(hass: HomeAssistantType,
|
|||
|
||||
# port mapping
|
||||
if data.get(CONF_ENABLE_PORT_MAPPING):
|
||||
local_ip = hass.data[DOMAIN].get('local_ip')
|
||||
local_ip = hass.data[DOMAIN]['local_ip']
|
||||
ports = hass.data[DOMAIN]['auto_config']['ports']
|
||||
_LOGGER.debug('Enabling port mappings: %s', ports)
|
||||
|
||||
hass_port = hass.http.server_port
|
||||
ports = _substitute_hass_ports(ports, hass_port)
|
||||
await device.async_add_port_mappings(ports, local_ip=local_ip)
|
||||
hass_port = None
|
||||
if hasattr(hass, 'http'):
|
||||
hass_port = hass.http.server_port
|
||||
ports = _substitute_hass_ports(ports, hass_port=hass_port)
|
||||
await device.async_add_port_mappings(ports, local_ip)
|
||||
|
||||
# sensors
|
||||
if data.get(CONF_ENABLE_SENSORS):
|
||||
|
|
@ -135,10 +154,12 @@ async def async_setup_entry(hass: HomeAssistantType,
|
|||
hass.async_create_task(hass.config_entries.async_forward_entry_setup(
|
||||
config_entry, 'sensor'))
|
||||
|
||||
async def unload_entry(event):
|
||||
"""Unload entry on quit."""
|
||||
await async_unload_entry(hass, config_entry)
|
||||
hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, unload_entry)
|
||||
async def delete_port_mapping(event):
|
||||
"""Delete port mapping on quit."""
|
||||
if data.get(CONF_ENABLE_PORT_MAPPING):
|
||||
_LOGGER.debug('Deleting port mappings')
|
||||
await device.async_delete_port_mappings()
|
||||
hass.bus.async_listen_once(EVENT_HOMEASSISTANT_STOP, delete_port_mapping)
|
||||
|
||||
return True
|
||||
|
||||
|
|
|
|||
|
|
@ -6,7 +6,6 @@ import aiohttp
|
|||
|
||||
from homeassistant.helpers.aiohttp_client import async_get_clientsession
|
||||
from homeassistant.helpers.typing import HomeAssistantType
|
||||
from homeassistant.util import get_local_ip
|
||||
|
||||
from .const import LOGGER as _LOGGER
|
||||
|
||||
|
|
@ -51,15 +50,13 @@ class Device:
|
|||
"""Get the name."""
|
||||
return self._igd_device.name
|
||||
|
||||
async def async_add_port_mappings(self, ports, local_ip=None):
|
||||
async def async_add_port_mappings(self, ports, local_ip):
|
||||
"""Add port mappings."""
|
||||
# determine local ip, ensure sane IP
|
||||
if local_ip is None:
|
||||
local_ip = get_local_ip()
|
||||
|
||||
if local_ip == '127.0.0.1':
|
||||
_LOGGER.error(
|
||||
'Could not create port mapping, our IP is 127.0.0.1')
|
||||
|
||||
# determine local ip, ensure sane IP
|
||||
local_ip = IPv4Address(local_ip)
|
||||
|
||||
# create port mappings
|
||||
|
|
|
|||
|
|
@ -9,6 +9,7 @@ from homeassistant.components.upnp.device import Device
|
|||
from homeassistant.const import EVENT_HOMEASSISTANT_STOP
|
||||
|
||||
from tests.common import MockConfigEntry
|
||||
from tests.common import MockDependency
|
||||
from tests.common import mock_coro
|
||||
|
||||
|
||||
|
|
@ -17,7 +18,7 @@ class MockDevice(Device):
|
|||
|
||||
def __init__(self, udn):
|
||||
"""Initializer."""
|
||||
super().__init__(None)
|
||||
super().__init__(MagicMock())
|
||||
self._udn = udn
|
||||
self.added_port_mappings = []
|
||||
self.removed_port_mappings = []
|
||||
|
|
@ -49,7 +50,15 @@ class MockDevice(Device):
|
|||
async def test_async_setup_no_auto_config(hass):
|
||||
"""Test async_setup."""
|
||||
# setup component, enable auto_config
|
||||
await async_setup_component(hass, 'upnp')
|
||||
config = {
|
||||
'discovery': {},
|
||||
# no upnp
|
||||
}
|
||||
with MockDependency('netdisco.discovery'), \
|
||||
patch('homeassistant.components.upnp.get_local_ip',
|
||||
return_value='192.168.1.10'):
|
||||
await async_setup_component(hass, 'upnp', config)
|
||||
await hass.async_block_till_done()
|
||||
|
||||
assert hass.data[upnp.DOMAIN]['auto_config'] == {
|
||||
'active': False,
|
||||
|
|
@ -62,7 +71,15 @@ async def test_async_setup_no_auto_config(hass):
|
|||
async def test_async_setup_auto_config(hass):
|
||||
"""Test async_setup."""
|
||||
# setup component, enable auto_config
|
||||
await async_setup_component(hass, 'upnp', {'upnp': {}, 'discovery': {}})
|
||||
config = {
|
||||
'discovery': {},
|
||||
'upnp': {},
|
||||
}
|
||||
with MockDependency('netdisco.discovery'), \
|
||||
patch('homeassistant.components.upnp.get_local_ip',
|
||||
return_value='192.168.1.10'):
|
||||
await async_setup_component(hass, 'upnp', config)
|
||||
await hass.async_block_till_done()
|
||||
|
||||
assert hass.data[upnp.DOMAIN]['auto_config'] == {
|
||||
'active': True,
|
||||
|
|
@ -75,12 +92,18 @@ async def test_async_setup_auto_config(hass):
|
|||
async def test_async_setup_auto_config_port_mapping(hass):
|
||||
"""Test async_setup."""
|
||||
# setup component, enable auto_config
|
||||
await async_setup_component(hass, 'upnp', {
|
||||
config = {
|
||||
'discovery': {},
|
||||
'upnp': {
|
||||
'port_mapping': True,
|
||||
'ports': {'hass': 'hass'},
|
||||
},
|
||||
'discovery': {}})
|
||||
}
|
||||
with MockDependency('netdisco.discovery'), \
|
||||
patch('homeassistant.components.upnp.get_local_ip',
|
||||
return_value='192.168.1.10'):
|
||||
await async_setup_component(hass, 'upnp', config)
|
||||
await hass.async_block_till_done()
|
||||
|
||||
assert hass.data[upnp.DOMAIN]['auto_config'] == {
|
||||
'active': True,
|
||||
|
|
@ -93,9 +116,15 @@ async def test_async_setup_auto_config_port_mapping(hass):
|
|||
async def test_async_setup_auto_config_no_sensors(hass):
|
||||
"""Test async_setup."""
|
||||
# setup component, enable auto_config
|
||||
await async_setup_component(hass, 'upnp', {
|
||||
config = {
|
||||
'discovery': {},
|
||||
'upnp': {'sensors': False},
|
||||
'discovery': {}})
|
||||
}
|
||||
with MockDependency('netdisco.discovery'), \
|
||||
patch('homeassistant.components.upnp.get_local_ip',
|
||||
return_value='192.168.1.10'):
|
||||
await async_setup_component(hass, 'upnp', config)
|
||||
await hass.async_block_till_done()
|
||||
|
||||
assert hass.data[upnp.DOMAIN]['auto_config'] == {
|
||||
'active': True,
|
||||
|
|
@ -115,18 +144,23 @@ async def test_async_setup_entry_default(hass):
|
|||
'port_mapping': False,
|
||||
})
|
||||
|
||||
# ensure hass.http is available
|
||||
await async_setup_component(hass, 'upnp')
|
||||
config = {
|
||||
'http': {},
|
||||
'discovery': {},
|
||||
# no upnp
|
||||
}
|
||||
with MockDependency('netdisco.discovery'), \
|
||||
patch('homeassistant.components.upnp.get_local_ip',
|
||||
return_value='192.168.1.10'):
|
||||
await async_setup_component(hass, 'http', config)
|
||||
await async_setup_component(hass, 'upnp', config)
|
||||
await hass.async_block_till_done()
|
||||
|
||||
# mock homeassistant.components.upnp.device.Device
|
||||
mock_device = MagicMock()
|
||||
mock_device.udn = udn
|
||||
mock_device.async_add_port_mappings.return_value = mock_coro()
|
||||
mock_device.async_delete_port_mappings.return_value = mock_coro()
|
||||
with patch.object(Device, 'async_create_device') as mock_create_device:
|
||||
mock_create_device.return_value = mock_coro(
|
||||
return_value=mock_device)
|
||||
with patch('homeassistant.components.upnp.device.get_local_ip',
|
||||
mock_device = MockDevice(udn)
|
||||
with patch.object(Device, 'async_create_device') as create_device:
|
||||
create_device.return_value = mock_coro(return_value=mock_device)
|
||||
with patch('homeassistant.components.upnp.get_local_ip',
|
||||
return_value='192.168.1.10'):
|
||||
assert await upnp.async_setup_entry(hass, entry) is True
|
||||
|
||||
|
|
@ -136,12 +170,9 @@ async def test_async_setup_entry_default(hass):
|
|||
hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP)
|
||||
await hass.async_block_till_done()
|
||||
|
||||
# ensure cleaned up
|
||||
assert udn not in hass.data[upnp.DOMAIN]['devices']
|
||||
|
||||
# ensure no port-mapping-methods called
|
||||
assert len(mock_device.async_add_port_mappings.mock_calls) == 0
|
||||
assert len(mock_device.async_delete_port_mappings.mock_calls) == 0
|
||||
# ensure no port-mappings created or removed
|
||||
assert not mock_device.added_port_mappings
|
||||
assert not mock_device.removed_port_mappings
|
||||
|
||||
|
||||
async def test_async_setup_entry_port_mapping(hass):
|
||||
|
|
@ -154,35 +185,37 @@ async def test_async_setup_entry_port_mapping(hass):
|
|||
'port_mapping': True,
|
||||
})
|
||||
|
||||
# ensure hass.http is available
|
||||
await async_setup_component(hass, 'upnp', {
|
||||
config = {
|
||||
'http': {},
|
||||
'discovery': {},
|
||||
'upnp': {
|
||||
'port_mapping': True,
|
||||
'ports': {'hass': 'hass'},
|
||||
},
|
||||
'discovery': {},
|
||||
})
|
||||
}
|
||||
with MockDependency('netdisco.discovery'), \
|
||||
patch('homeassistant.components.upnp.get_local_ip',
|
||||
return_value='192.168.1.10'):
|
||||
await async_setup_component(hass, 'http', config)
|
||||
await async_setup_component(hass, 'upnp', config)
|
||||
await hass.async_block_till_done()
|
||||
|
||||
mock_device = MockDevice(udn)
|
||||
with patch.object(Device, 'async_create_device') as mock_create_device:
|
||||
mock_create_device.return_value = mock_coro(return_value=mock_device)
|
||||
with patch('homeassistant.components.upnp.device.get_local_ip',
|
||||
return_value='192.168.1.10'):
|
||||
assert await upnp.async_setup_entry(hass, entry) is True
|
||||
with patch.object(Device, 'async_create_device') as create_device:
|
||||
create_device.return_value = mock_coro(return_value=mock_device)
|
||||
|
||||
# ensure device is stored/used
|
||||
assert hass.data[upnp.DOMAIN]['devices'][udn] == mock_device
|
||||
assert await upnp.async_setup_entry(hass, entry) is True
|
||||
|
||||
# ensure add-port-mapping-methods called
|
||||
assert mock_device.added_port_mappings == [
|
||||
[8123, ip_address('192.168.1.10'), 8123]
|
||||
]
|
||||
# ensure device is stored/used
|
||||
assert hass.data[upnp.DOMAIN]['devices'][udn] == mock_device
|
||||
|
||||
hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP)
|
||||
await hass.async_block_till_done()
|
||||
# ensure add-port-mapping-methods called
|
||||
assert mock_device.added_port_mappings == [
|
||||
[8123, ip_address('192.168.1.10'), 8123]
|
||||
]
|
||||
|
||||
# ensure cleaned up
|
||||
assert udn not in hass.data[upnp.DOMAIN]['devices']
|
||||
hass.bus.async_fire(EVENT_HOMEASSISTANT_STOP)
|
||||
await hass.async_block_till_done()
|
||||
|
||||
# ensure delete-port-mapping-methods called
|
||||
assert mock_device.removed_port_mappings == [8123]
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue