From 463a32819c0ae8f5c84e75a16fdc99aef0a11cae Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 2 Feb 2021 03:30:38 -1000 Subject: [PATCH] Ensure homekit never picks a port that another config entry uses (#45433) --- homeassistant/components/homekit/__init__.py | 17 ++++++----- .../components/homekit/config_flow.py | 12 +++----- homeassistant/components/homekit/util.py | 21 ++++++++++++-- tests/components/homekit/test_config_flow.py | 6 ++-- tests/components/homekit/test_homekit.py | 5 +--- tests/components/homekit/test_util.py | 28 +++++++++++++++---- 6 files changed, 59 insertions(+), 30 deletions(-) diff --git a/homeassistant/components/homekit/__init__.py b/homeassistant/components/homekit/__init__.py index e92c35ffac8..568804ef081 100644 --- a/homeassistant/components/homekit/__init__.py +++ b/homeassistant/components/homekit/__init__.py @@ -238,12 +238,6 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry): port = conf[CONF_PORT] _LOGGER.debug("Begin setup HomeKit for %s", name) - # If the previous instance hasn't cleaned up yet - # we need to wait a bit - if not await hass.async_add_executor_job(port_is_available, port): - _LOGGER.warning("The local port %s is in use", port) - raise ConfigEntryNotReady - if CONF_ENTRY_INDEX in conf and conf[CONF_ENTRY_INDEX] == 0: _LOGGER.debug("Migrating legacy HomeKit data for %s", name) hass.async_add_executor_job( @@ -275,7 +269,16 @@ async def async_setup_entry(hass: HomeAssistant, entry: ConfigEntry): entry.entry_id, ) zeroconf_instance = await zeroconf.async_get_instance(hass) - await hass.async_add_executor_job(homekit.setup, zeroconf_instance) + + # If the previous instance hasn't cleaned up yet + # we need to wait a bit + try: + await hass.async_add_executor_job(homekit.setup, zeroconf_instance) + except (OSError, AttributeError) as ex: + _LOGGER.warning( + "%s could not be setup because the local port %s is in use", name, port + ) + raise ConfigEntryNotReady from ex undo_listener = entry.add_update_listener(_async_update_listener) diff --git a/homeassistant/components/homekit/config_flow.py b/homeassistant/components/homekit/config_flow.py index d8708168e12..a20d9a5b843 100644 --- a/homeassistant/components/homekit/config_flow.py +++ b/homeassistant/components/homekit/config_flow.py @@ -40,7 +40,7 @@ from .const import ( VIDEO_CODEC_COPY, ) from .const import DOMAIN # pylint:disable=unused-import -from .util import find_next_available_port +from .util import async_find_next_available_port CONF_CAMERA_COPY = "camera_copy" CONF_INCLUDE_EXCLUDE_MODE = "include_exclude_mode" @@ -162,7 +162,9 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): if user_input is not None: return self.async_create_entry(title=self.entry_title, data=self.hk_data) - self.hk_data[CONF_PORT] = await self._async_available_port() + self.hk_data[CONF_PORT] = await async_find_next_available_port( + self.hass, DEFAULT_CONFIG_FLOW_PORT + ) self.hk_data[CONF_NAME] = self._async_available_name( self.hk_data[CONF_HOMEKIT_MODE] ) @@ -205,12 +207,6 @@ class ConfigFlow(config_entries.ConfigFlow, domain=DOMAIN): title=f"{user_input[CONF_NAME]}:{user_input[CONF_PORT]}", data=user_input ) - async def _async_available_port(self): - """Return an available port the bridge.""" - return await self.hass.async_add_executor_job( - find_next_available_port, DEFAULT_CONFIG_FLOW_PORT - ) - @callback def _async_current_names(self): """Return a set of bridge names.""" diff --git a/homeassistant/components/homekit/util.py b/homeassistant/components/homekit/util.py index 98374b73f40..83a9a0a0353 100644 --- a/homeassistant/components/homekit/util.py +++ b/homeassistant/components/homekit/util.py @@ -15,6 +15,7 @@ from homeassistant.const import ( ATTR_CODE, ATTR_SUPPORTED_FEATURES, CONF_NAME, + CONF_PORT, CONF_TYPE, TEMP_CELSIUS, ) @@ -445,7 +446,7 @@ def _get_test_socket(): return test_socket -def port_is_available(port: int): +def port_is_available(port: int) -> bool: """Check to see if a port is available.""" test_socket = _get_test_socket() try: @@ -456,10 +457,24 @@ def port_is_available(port: int): return True -def find_next_available_port(start_port: int): +async def async_find_next_available_port(hass: HomeAssistant, start_port: int) -> int: + """Find the next available port not assigned to a config entry.""" + exclude_ports = set() + for entry in hass.config_entries.async_entries(DOMAIN): + if CONF_PORT in entry.data: + exclude_ports.add(entry.data[CONF_PORT]) + + return await hass.async_add_executor_job( + _find_next_available_port, start_port, exclude_ports + ) + + +def _find_next_available_port(start_port: int, exclude_ports: set) -> int: """Find the next available port starting with the given port.""" test_socket = _get_test_socket() for port in range(start_port, MAX_PORT): + if port in exclude_ports: + continue try: test_socket.bind(("", port)) return port @@ -469,7 +484,7 @@ def find_next_available_port(start_port: int): continue -def pid_is_alive(pid): +def pid_is_alive(pid) -> bool: """Check to see if a process is alive.""" try: os.kill(pid, 0) diff --git a/tests/components/homekit/test_config_flow.py b/tests/components/homekit/test_config_flow.py index 1dd628af18d..0e4114d566d 100644 --- a/tests/components/homekit/test_config_flow.py +++ b/tests/components/homekit/test_config_flow.py @@ -49,7 +49,7 @@ async def test_setup_in_bridge_mode(hass): assert result2["step_id"] == "bridge_mode" with patch( - "homeassistant.components.homekit.config_flow.find_next_available_port", + "homeassistant.components.homekit.config_flow.async_find_next_available_port", return_value=12345, ): result3 = await hass.config_entries.flow.async_configure( @@ -108,7 +108,7 @@ async def test_setup_in_accessory_mode(hass): assert result2["step_id"] == "accessory_mode" with patch( - "homeassistant.components.homekit.config_flow.find_next_available_port", + "homeassistant.components.homekit.config_flow.async_find_next_available_port", return_value=12345, ): result3 = await hass.config_entries.flow.async_configure( @@ -629,7 +629,7 @@ async def test_converting_bridge_to_accessory_mode(hass): assert result2["step_id"] == "bridge_mode" with patch( - "homeassistant.components.homekit.config_flow.find_next_available_port", + "homeassistant.components.homekit.config_flow.async_find_next_available_port", return_value=12345, ): result3 = await hass.config_entries.flow.async_configure( diff --git a/tests/components/homekit/test_homekit.py b/tests/components/homekit/test_homekit.py index a8c3c81595e..f0d6a8b365f 100644 --- a/tests/components/homekit/test_homekit.py +++ b/tests/components/homekit/test_homekit.py @@ -1020,10 +1020,7 @@ async def test_raise_config_entry_not_ready(hass, mock_zeroconf): ) entry.add_to_hass(hass) - with patch( - "homeassistant.components.homekit.port_is_available", - return_value=False, - ): + with patch(f"{PATH_HOMEKIT}.HomeKit.setup", side_effect=OSError): assert not await hass.config_entries.async_setup(entry.entry_id) await hass.async_block_till_done() diff --git a/tests/components/homekit/test_util.py b/tests/components/homekit/test_util.py index e0f10a94d69..afa1408a06b 100644 --- a/tests/components/homekit/test_util.py +++ b/tests/components/homekit/test_util.py @@ -3,6 +3,7 @@ import pytest import voluptuous as vol from homeassistant.components.homekit.const import ( + BRIDGE_NAME, CONF_FEATURE, CONF_FEATURE_LIST, CONF_LINKED_BATTERY_SENSOR, @@ -21,11 +22,11 @@ from homeassistant.components.homekit.const import ( TYPE_VALVE, ) from homeassistant.components.homekit.util import ( + async_find_next_available_port, cleanup_name_for_homekit, convert_to_float, density_to_air_quality, dismiss_setup_message, - find_next_available_port, format_sw_version, port_is_available, show_setup_message, @@ -43,6 +44,7 @@ from homeassistant.const import ( ATTR_CODE, ATTR_SUPPORTED_FEATURES, CONF_NAME, + CONF_PORT, CONF_TYPE, STATE_UNKNOWN, TEMP_CELSIUS, @@ -52,7 +54,7 @@ from homeassistant.core import State from .util import async_init_integration -from tests.common import async_mock_service +from tests.common import MockConfigEntry, async_mock_service def test_validate_entity_config(): @@ -251,14 +253,30 @@ async def test_dismiss_setup_msg(hass): async def test_port_is_available(hass): """Test we can get an available port and it is actually available.""" - next_port = await hass.async_add_executor_job( - find_next_available_port, DEFAULT_CONFIG_FLOW_PORT - ) + next_port = await async_find_next_available_port(hass, DEFAULT_CONFIG_FLOW_PORT) + assert next_port assert await hass.async_add_executor_job(port_is_available, next_port) +async def test_port_is_available_skips_existing_entries(hass): + """Test we can get an available port and it is actually available.""" + entry = MockConfigEntry( + domain=DOMAIN, + data={CONF_NAME: BRIDGE_NAME, CONF_PORT: DEFAULT_CONFIG_FLOW_PORT}, + options={}, + ) + entry.add_to_hass(hass) + + next_port = await async_find_next_available_port(hass, DEFAULT_CONFIG_FLOW_PORT) + + assert next_port + assert next_port != DEFAULT_CONFIG_FLOW_PORT + + assert await hass.async_add_executor_job(port_is_available, next_port) + + async def test_format_sw_version(): """Test format_sw_version method.""" assert format_sw_version("soho+3.6.8+soho-release-rt120+10") == "3.6.8"