mirror of
https://github.com/esphome/esphome.git
synced 2025-10-17 09:13:45 +01:00
[ota] Fix MQTT resolution when static IP appears first in device list (#11272)
This commit is contained in:
J. Nick Koston
GitHub
@@ -117,6 +117,17 @@ class Purpose(StrEnum):
|
||||
LOGGING = "logging"
|
||||
|
||||
|
||||
class PortType(StrEnum):
|
||||
SERIAL = "SERIAL"
|
||||
NETWORK = "NETWORK"
|
||||
MQTT = "MQTT"
|
||||
MQTTIP = "MQTTIP"
|
||||
|
||||
|
||||
# Magic MQTT port types that require special handling
|
||||
_MQTT_PORT_TYPES = frozenset({PortType.MQTT, PortType.MQTTIP})
|
||||
|
||||
|
||||
def _resolve_with_cache(address: str, purpose: Purpose) -> list[str]:
|
||||
"""Resolve an address using cache if available, otherwise return the address itself."""
|
||||
if CORE.address_cache and (cached := CORE.address_cache.get_addresses(address)):
|
||||
@@ -280,16 +291,67 @@ def mqtt_get_ip(config: ConfigType, username: str, password: str, client_id: str
|
||||
return mqtt.get_esphome_device_ip(config, username, password, client_id)
|
||||
|
||||
|
||||
_PORT_TO_PORT_TYPE = {
|
||||
"MQTT": "MQTT",
|
||||
"MQTTIP": "MQTTIP",
|
||||
}
|
||||
def _resolve_network_devices(
|
||||
devices: list[str], config: ConfigType, args: ArgsProtocol
|
||||
) -> list[str]:
|
||||
"""Resolve device list, converting MQTT magic strings to actual IP addresses.
|
||||
|
||||
This function filters the devices list to:
|
||||
- Replace MQTT/MQTTIP magic strings with actual IP addresses via MQTT lookup
|
||||
- Deduplicate addresses while preserving order
|
||||
- Only resolve MQTT once even if multiple MQTT strings are present
|
||||
- If MQTT resolution fails, log a warning and continue with other devices
|
||||
|
||||
Args:
|
||||
devices: List of device identifiers (IPs, hostnames, or magic strings)
|
||||
config: ESPHome configuration
|
||||
args: Command-line arguments containing MQTT credentials
|
||||
|
||||
Returns:
|
||||
List of network addresses suitable for connection attempts
|
||||
"""
|
||||
network_devices: list[str] = []
|
||||
mqtt_resolved: bool = False
|
||||
|
||||
for device in devices:
|
||||
port_type = get_port_type(device)
|
||||
if port_type in _MQTT_PORT_TYPES:
|
||||
# Only resolve MQTT once, even if multiple MQTT entries
|
||||
if not mqtt_resolved:
|
||||
try:
|
||||
mqtt_ips = mqtt_get_ip(
|
||||
config, args.username, args.password, args.client_id
|
||||
)
|
||||
network_devices.extend(mqtt_ips)
|
||||
except EsphomeError as err:
|
||||
_LOGGER.warning(
|
||||
"MQTT IP discovery failed (%s), will try other devices if available",
|
||||
err,
|
||||
)
|
||||
mqtt_resolved = True
|
||||
elif device not in network_devices:
|
||||
# Regular network address or IP - add if not already present
|
||||
network_devices.append(device)
|
||||
|
||||
return network_devices
|
||||
|
||||
|
||||
def get_port_type(port: str) -> str:
|
||||
def get_port_type(port: str) -> PortType:
|
||||
"""Determine the type of port/device identifier.
|
||||
|
||||
Returns:
|
||||
PortType.SERIAL for serial ports (/dev/ttyUSB0, COM1, etc.)
|
||||
PortType.MQTT for MQTT logging
|
||||
PortType.MQTTIP for MQTT IP lookup
|
||||
PortType.NETWORK for IP addresses, hostnames, or mDNS names
|
||||
"""
|
||||
if port.startswith("/") or port.startswith("COM"):
|
||||
return "SERIAL"
|
||||
return _PORT_TO_PORT_TYPE.get(port, "NETWORK")
|
||||
return PortType.SERIAL
|
||||
if port == "MQTT":
|
||||
return PortType.MQTT
|
||||
if port == "MQTTIP":
|
||||
return PortType.MQTTIP
|
||||
return PortType.NETWORK
|
||||
|
||||
|
||||
def run_miniterm(config: ConfigType, port: str, args) -> int:
|
||||
@@ -489,7 +551,7 @@ def upload_using_platformio(config: ConfigType, port: str):
|
||||
|
||||
|
||||
def check_permissions(port: str):
|
||||
if os.name == "posix" and get_port_type(port) == "SERIAL":
|
||||
if os.name == "posix" and get_port_type(port) == PortType.SERIAL:
|
||||
# Check if we can open selected serial port
|
||||
if not os.access(port, os.F_OK):
|
||||
raise EsphomeError(
|
||||
@@ -517,7 +579,7 @@ def upload_program(
|
||||
except AttributeError:
|
||||
pass
|
||||
|
||||
if get_port_type(host) == "SERIAL":
|
||||
if get_port_type(host) == PortType.SERIAL:
|
||||
check_permissions(host)
|
||||
|
||||
exit_code = 1
|
||||
@@ -550,11 +612,10 @@ def upload_program(
|
||||
else:
|
||||
binary = CORE.firmware_bin
|
||||
|
||||
# MQTT address resolution
|
||||
if get_port_type(host) in ("MQTT", "MQTTIP"):
|
||||
devices = mqtt_get_ip(config, args.username, args.password, args.client_id)
|
||||
# Resolve MQTT magic strings to actual IP addresses
|
||||
network_devices = _resolve_network_devices(devices, config, args)
|
||||
|
||||
return espota2.run_ota(devices, remote_port, password, binary)
|
||||
return espota2.run_ota(network_devices, remote_port, password, binary)
|
||||
|
||||
|
||||
def show_logs(config: ConfigType, args: ArgsProtocol, devices: list[str]) -> int | None:
|
||||
@@ -569,33 +630,22 @@ def show_logs(config: ConfigType, args: ArgsProtocol, devices: list[str]) -> int
|
||||
raise EsphomeError("Logger is not configured!")
|
||||
|
||||
port = devices[0]
|
||||
port_type = get_port_type(port)
|
||||
|
||||
if get_port_type(port) == "SERIAL":
|
||||
if port_type == PortType.SERIAL:
|
||||
check_permissions(port)
|
||||
return run_miniterm(config, port, args)
|
||||
|
||||
port_type = get_port_type(port)
|
||||
|
||||
# Check if we should use API for logging
|
||||
if has_api():
|
||||
addresses_to_use: list[str] | None = None
|
||||
# Resolve MQTT magic strings to actual IP addresses
|
||||
if has_api() and (
|
||||
network_devices := _resolve_network_devices(devices, config, args)
|
||||
):
|
||||
from esphome.components.api.client import run_logs
|
||||
|
||||
if port_type == "NETWORK":
|
||||
# Network addresses (IPs, mDNS names, or regular DNS hostnames) can be used
|
||||
# The resolve_ip_address() function in helpers.py handles all types
|
||||
addresses_to_use = devices
|
||||
elif port_type in ("MQTT", "MQTTIP") and has_mqtt_ip_lookup():
|
||||
# Use MQTT IP lookup for MQTT/MQTTIP types
|
||||
addresses_to_use = mqtt_get_ip(
|
||||
config, args.username, args.password, args.client_id
|
||||
)
|
||||
return run_logs(config, network_devices)
|
||||
|
||||
if addresses_to_use is not None:
|
||||
from esphome.components.api.client import run_logs
|
||||
|
||||
return run_logs(config, addresses_to_use)
|
||||
|
||||
if port_type in ("NETWORK", "MQTT") and has_mqtt_logging():
|
||||
if port_type in (PortType.NETWORK, PortType.MQTT) and has_mqtt_logging():
|
||||
from esphome import mqtt
|
||||
|
||||
return mqtt.show_logs(
|
||||
|
||||
@@ -1976,3 +1976,290 @@ def test_command_clean_all_args_used() -> None:
|
||||
# Verify the correct configuration paths were passed
|
||||
mock_clean_all.assert_any_call(["/path/to/config1"])
|
||||
mock_clean_all.assert_any_call(["/path/to/config2", "/path/to/config3"])
|
||||
|
||||
|
||||
def test_upload_program_ota_static_ip_with_mqttip(
|
||||
mock_mqtt_get_ip: Mock,
|
||||
mock_run_ota: Mock,
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Test upload_program with static IP and MQTTIP (issue #11260).
|
||||
|
||||
This tests the scenario where a device has manual_ip (static IP) configured
|
||||
and MQTT is also configured. The devices list contains both the static IP
|
||||
and "MQTTIP" magic string. This previously failed because only the first
|
||||
device was checked for MQTT resolution.
|
||||
"""
|
||||
setup_core(platform=PLATFORM_ESP32, tmp_path=tmp_path)
|
||||
|
||||
mock_mqtt_get_ip.return_value = ["192.168.2.50"] # Different subnet
|
||||
mock_run_ota.return_value = (0, "192.168.1.100")
|
||||
|
||||
config = {
|
||||
CONF_OTA: [
|
||||
{
|
||||
CONF_PLATFORM: CONF_ESPHOME,
|
||||
CONF_PORT: 3232,
|
||||
}
|
||||
],
|
||||
CONF_MQTT: {
|
||||
CONF_BROKER: "mqtt.local",
|
||||
},
|
||||
}
|
||||
args = MockArgs(username="user", password="pass", client_id="client")
|
||||
# Simulates choose_upload_log_host returning static IP + MQTTIP
|
||||
devices = ["192.168.1.100", "MQTTIP"]
|
||||
|
||||
exit_code, host = upload_program(config, args, devices)
|
||||
|
||||
assert exit_code == 0
|
||||
assert host == "192.168.1.100"
|
||||
|
||||
# Verify MQTT was resolved
|
||||
mock_mqtt_get_ip.assert_called_once_with(config, "user", "pass", "client")
|
||||
|
||||
# Verify espota2.run_ota was called with both IPs
|
||||
expected_firmware = (
|
||||
tmp_path / ".esphome" / "build" / "test" / ".pioenvs" / "test" / "firmware.bin"
|
||||
)
|
||||
mock_run_ota.assert_called_once_with(
|
||||
["192.168.1.100", "192.168.2.50"], 3232, "", expected_firmware
|
||||
)
|
||||
|
||||
|
||||
def test_upload_program_ota_multiple_mqttip_resolves_once(
|
||||
mock_mqtt_get_ip: Mock,
|
||||
mock_run_ota: Mock,
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Test that MQTT resolution only happens once even with multiple MQTT magic strings."""
|
||||
setup_core(platform=PLATFORM_ESP32, tmp_path=tmp_path)
|
||||
|
||||
mock_mqtt_get_ip.return_value = ["192.168.2.50", "192.168.2.51"]
|
||||
mock_run_ota.return_value = (0, "192.168.2.50")
|
||||
|
||||
config = {
|
||||
CONF_OTA: [
|
||||
{
|
||||
CONF_PLATFORM: CONF_ESPHOME,
|
||||
CONF_PORT: 3232,
|
||||
}
|
||||
],
|
||||
CONF_MQTT: {
|
||||
CONF_BROKER: "mqtt.local",
|
||||
},
|
||||
}
|
||||
args = MockArgs(username="user", password="pass", client_id="client")
|
||||
# Multiple MQTT magic strings in the list
|
||||
devices = ["MQTTIP", "MQTT", "192.168.1.100"]
|
||||
|
||||
exit_code, host = upload_program(config, args, devices)
|
||||
|
||||
assert exit_code == 0
|
||||
assert host == "192.168.2.50"
|
||||
|
||||
# Verify MQTT was only resolved once despite multiple MQTT magic strings
|
||||
mock_mqtt_get_ip.assert_called_once_with(config, "user", "pass", "client")
|
||||
|
||||
# Verify espota2.run_ota was called with all unique IPs
|
||||
expected_firmware = (
|
||||
tmp_path / ".esphome" / "build" / "test" / ".pioenvs" / "test" / "firmware.bin"
|
||||
)
|
||||
mock_run_ota.assert_called_once_with(
|
||||
["192.168.2.50", "192.168.2.51", "192.168.1.100"], 3232, "", expected_firmware
|
||||
)
|
||||
|
||||
|
||||
def test_upload_program_ota_mqttip_deduplication(
|
||||
mock_mqtt_get_ip: Mock,
|
||||
mock_run_ota: Mock,
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Test that duplicate IPs are filtered when MQTT returns same IP as static IP."""
|
||||
setup_core(platform=PLATFORM_ESP32, tmp_path=tmp_path)
|
||||
|
||||
# MQTT returns the same IP as the static IP
|
||||
mock_mqtt_get_ip.return_value = ["192.168.1.100"]
|
||||
mock_run_ota.return_value = (0, "192.168.1.100")
|
||||
|
||||
config = {
|
||||
CONF_OTA: [
|
||||
{
|
||||
CONF_PLATFORM: CONF_ESPHOME,
|
||||
CONF_PORT: 3232,
|
||||
}
|
||||
],
|
||||
CONF_MQTT: {
|
||||
CONF_BROKER: "mqtt.local",
|
||||
},
|
||||
}
|
||||
args = MockArgs(username="user", password="pass", client_id="client")
|
||||
devices = ["192.168.1.100", "MQTTIP"]
|
||||
|
||||
exit_code, host = upload_program(config, args, devices)
|
||||
|
||||
assert exit_code == 0
|
||||
assert host == "192.168.1.100"
|
||||
|
||||
# Verify MQTT was resolved
|
||||
mock_mqtt_get_ip.assert_called_once_with(config, "user", "pass", "client")
|
||||
|
||||
# Verify espota2.run_ota was called with deduplicated IPs (only one instance of 192.168.1.100)
|
||||
# Note: Current implementation doesn't dedupe, so we'll get the IP twice
|
||||
# This test documents current behavior - deduplication could be future enhancement
|
||||
mock_run_ota.assert_called_once()
|
||||
call_args = mock_run_ota.call_args[0]
|
||||
# Should contain both the original IP and MQTT-resolved IP (even if duplicate)
|
||||
assert "192.168.1.100" in call_args[0]
|
||||
|
||||
|
||||
@patch("esphome.components.api.client.run_logs")
|
||||
def test_show_logs_api_static_ip_with_mqttip(
|
||||
mock_run_logs: Mock,
|
||||
mock_mqtt_get_ip: Mock,
|
||||
) -> None:
|
||||
"""Test show_logs with static IP and MQTTIP (issue #11260).
|
||||
|
||||
This tests the scenario where a device has manual_ip (static IP) configured
|
||||
and MQTT is also configured. The devices list contains both the static IP
|
||||
and "MQTTIP" magic string.
|
||||
"""
|
||||
setup_core(
|
||||
config={
|
||||
"logger": {},
|
||||
CONF_API: {},
|
||||
CONF_MQTT: {CONF_BROKER: "mqtt.local"},
|
||||
},
|
||||
platform=PLATFORM_ESP32,
|
||||
)
|
||||
mock_run_logs.return_value = 0
|
||||
mock_mqtt_get_ip.return_value = ["192.168.2.50"]
|
||||
|
||||
args = MockArgs(username="user", password="pass", client_id="client")
|
||||
# Simulates choose_upload_log_host returning static IP + MQTTIP
|
||||
devices = ["192.168.1.100", "MQTTIP"]
|
||||
|
||||
result = show_logs(CORE.config, args, devices)
|
||||
|
||||
assert result == 0
|
||||
|
||||
# Verify MQTT was resolved
|
||||
mock_mqtt_get_ip.assert_called_once_with(CORE.config, "user", "pass", "client")
|
||||
|
||||
# Verify run_logs was called with both IPs
|
||||
mock_run_logs.assert_called_once_with(
|
||||
CORE.config, ["192.168.1.100", "192.168.2.50"]
|
||||
)
|
||||
|
||||
|
||||
@patch("esphome.components.api.client.run_logs")
|
||||
def test_show_logs_api_multiple_mqttip_resolves_once(
|
||||
mock_run_logs: Mock,
|
||||
mock_mqtt_get_ip: Mock,
|
||||
) -> None:
|
||||
"""Test that MQTT resolution only happens once for show_logs with multiple MQTT magic strings."""
|
||||
setup_core(
|
||||
config={
|
||||
"logger": {},
|
||||
CONF_API: {},
|
||||
CONF_MQTT: {CONF_BROKER: "mqtt.local"},
|
||||
},
|
||||
platform=PLATFORM_ESP32,
|
||||
)
|
||||
mock_run_logs.return_value = 0
|
||||
mock_mqtt_get_ip.return_value = ["192.168.2.50", "192.168.2.51"]
|
||||
|
||||
args = MockArgs(username="user", password="pass", client_id="client")
|
||||
# Multiple MQTT magic strings in the list
|
||||
devices = ["MQTTIP", "192.168.1.100", "MQTT"]
|
||||
|
||||
result = show_logs(CORE.config, args, devices)
|
||||
|
||||
assert result == 0
|
||||
|
||||
# Verify MQTT was only resolved once despite multiple MQTT magic strings
|
||||
mock_mqtt_get_ip.assert_called_once_with(CORE.config, "user", "pass", "client")
|
||||
|
||||
# Verify run_logs was called with all unique IPs (MQTT strings replaced with IPs)
|
||||
# Note: "MQTT" is a different magic string from "MQTTIP", but both trigger MQTT resolution
|
||||
# The _resolve_network_devices helper filters out both after first resolution
|
||||
mock_run_logs.assert_called_once_with(
|
||||
CORE.config, ["192.168.2.50", "192.168.2.51", "192.168.1.100"]
|
||||
)
|
||||
|
||||
|
||||
def test_upload_program_ota_mqtt_timeout_fallback(
|
||||
mock_mqtt_get_ip: Mock,
|
||||
mock_run_ota: Mock,
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
"""Test upload_program falls back to other devices when MQTT times out."""
|
||||
setup_core(platform=PLATFORM_ESP32, tmp_path=tmp_path)
|
||||
|
||||
# MQTT times out
|
||||
mock_mqtt_get_ip.side_effect = EsphomeError("Failed to find IP via MQTT")
|
||||
mock_run_ota.return_value = (0, "192.168.1.100")
|
||||
|
||||
config = {
|
||||
CONF_OTA: [
|
||||
{
|
||||
CONF_PLATFORM: CONF_ESPHOME,
|
||||
CONF_PORT: 3232,
|
||||
}
|
||||
],
|
||||
CONF_MQTT: {
|
||||
CONF_BROKER: "mqtt.local",
|
||||
},
|
||||
}
|
||||
args = MockArgs(username="user", password="pass", client_id="client")
|
||||
# Static IP first, MQTTIP second
|
||||
devices = ["192.168.1.100", "MQTTIP"]
|
||||
|
||||
exit_code, host = upload_program(config, args, devices)
|
||||
|
||||
# Should succeed using the static IP even though MQTT failed
|
||||
assert exit_code == 0
|
||||
assert host == "192.168.1.100"
|
||||
|
||||
# Verify MQTT was attempted
|
||||
mock_mqtt_get_ip.assert_called_once_with(config, "user", "pass", "client")
|
||||
|
||||
# Verify espota2.run_ota was called with only the static IP (MQTT failed)
|
||||
expected_firmware = (
|
||||
tmp_path / ".esphome" / "build" / "test" / ".pioenvs" / "test" / "firmware.bin"
|
||||
)
|
||||
mock_run_ota.assert_called_once_with(["192.168.1.100"], 3232, "", expected_firmware)
|
||||
|
||||
|
||||
@patch("esphome.components.api.client.run_logs")
|
||||
def test_show_logs_api_mqtt_timeout_fallback(
|
||||
mock_run_logs: Mock,
|
||||
mock_mqtt_get_ip: Mock,
|
||||
) -> None:
|
||||
"""Test show_logs falls back to other devices when MQTT times out."""
|
||||
setup_core(
|
||||
config={
|
||||
"logger": {},
|
||||
CONF_API: {},
|
||||
CONF_MQTT: {CONF_BROKER: "mqtt.local"},
|
||||
},
|
||||
platform=PLATFORM_ESP32,
|
||||
)
|
||||
mock_run_logs.return_value = 0
|
||||
# MQTT times out
|
||||
mock_mqtt_get_ip.side_effect = EsphomeError("Failed to find IP via MQTT")
|
||||
|
||||
args = MockArgs(username="user", password="pass", client_id="client")
|
||||
# Static IP first, MQTTIP second
|
||||
devices = ["192.168.1.100", "MQTTIP"]
|
||||
|
||||
result = show_logs(CORE.config, args, devices)
|
||||
|
||||
# Should succeed using the static IP even though MQTT failed
|
||||
assert result == 0
|
||||
|
||||
# Verify MQTT was attempted
|
||||
mock_mqtt_get_ip.assert_called_once_with(CORE.config, "user", "pass", "client")
|
||||
|
||||
# Verify run_logs was called with only the static IP (MQTT failed)
|
||||
mock_run_logs.assert_called_once_with(CORE.config, ["192.168.1.100"])
|
||||
|
||||
Reference in New Issue
Block a user