From 53c20b0fc3bf3005ebfce92045227eb74d1c29a7 Mon Sep 17 00:00:00 2001 From: Bodong Yang <86948717+Bodong-Yang@users.noreply.github.com> Date: Thu, 26 Dec 2024 10:05:24 +0900 Subject: [PATCH] feat!: reject any OTA update/rollback request on ecu_info.yaml not properly loaded (#465) This PR implements the requirement of rejecting any OTA update/rollback request when ecu_info.yaml file is broken, and reports the failure_reason as due to ecu_info.yaml is broken. If ecu_info.yaml is broken and cannot be parsed at startup, otaclient will set the live_ota_status to FAILURE, failure_type to UNRECOVERABLE, with failure_message: `ecu_info.yaml file is broken, please check /boot/ota/ecu_info.yaml. reject any OTA request.` And for every incoming OTA request, otaclient will reject with failure_type.UNRECOVERABLE for every ECUs listed in the request(as otaclient with default ecu_info.yaml doesn't have contact for the sub ECUs). For multiple ECU environment, due to when falling down to the default ecu_info, we will only have one entry(which is the main ECU) in the available_ecu_ids list, so the situation will be the same as single ECU environment. Other changes: 1. status_monitor: now status_monitor will always push to shm when the incoming report is OTAStatusChangeReport, regardless the push interval. BREAKING CHANGE: now otaclient will set itself in FAILURE OTA status and reject any OTA requests when ecu_info.yaml is not properly loaded at startup. --- src/otaclient/_status_monitor.py | 10 +- src/otaclient/grpc/api_v2/servicer.py | 33 ++++++ src/otaclient/ota_core.py | 69 +++++++++--- .../test_configs/test_ecu_info.py | 104 ++++++++++-------- .../test_configs/test_proxy_info.py | 53 +++++---- 5 files changed, 189 insertions(+), 80 deletions(-) diff --git a/src/otaclient/_status_monitor.py b/src/otaclient/_status_monitor.py index 6aba1b1c8..146544cf1 100644 --- a/src/otaclient/_status_monitor.py +++ b/src/otaclient/_status_monitor.py @@ -310,7 +310,15 @@ def _status_collector_thread(self) -> None: break # ------ push status on load_report ------ # - if self.load_report(report) and self._status and _now > _next_shm_push: + # NOTE: always push OTAStatus change report + if ( + self.load_report(report) + and self._status + and ( + isinstance(report.payload, OTAStatusChangeReport) + or _now > _next_shm_push + ) + ): try: self._shm_status.write_msg(self._status) _next_shm_push = _now + self.shm_push_interval diff --git a/src/otaclient/grpc/api_v2/servicer.py b/src/otaclient/grpc/api_v2/servicer.py index a44ccb1a5..16bcd34d8 100644 --- a/src/otaclient/grpc/api_v2/servicer.py +++ b/src/otaclient/grpc/api_v2/servicer.py @@ -21,6 +21,7 @@ import multiprocessing.queues as mp_queue from concurrent.futures import ThreadPoolExecutor +import otaclient.configs.cfg as otaclient_cfg from otaclient._types import ( IPCRequest, IPCResEnum, @@ -111,6 +112,22 @@ async def update( update_acked_ecus = set() response = api_types.UpdateResponse() + # NOTE(20241220): due to the fact that OTA Service API doesn't have field + # in UpdateResponseEcu msg, the only way to pass the failure_msg + # to upper is by status API. + if not otaclient_cfg.ECU_INFO_LOADED_SUCCESSFULLY: + logger.error( + "ecu_info.yaml is not loaded properly, reject any update request" + ) + for _update_req in request.iter_ecu(): + response.add_ecu( + api_types.UpdateResponseEcu( + ecu_id=_update_req.ecu_id, + result=api_types.FailureType.UNRECOVERABLE, + ) + ) + return response + # first: dispatch update request to all directly connected subECUs tasks: dict[asyncio.Task, ECUContact] = {} for ecu_contact in self.sub_ecus: @@ -225,6 +242,22 @@ async def rollback( logger.info(f"receive rollback request: {request}") response = api_types.RollbackResponse() + # NOTE(20241220): due to the fact that OTA Service API doesn't have field + # in UpdateResponseEcu msg, the only way to pass the failure_msg + # to upper is by status API. + if not otaclient_cfg.ECU_INFO_LOADED_SUCCESSFULLY: + logger.error( + "ecu_info.yaml is not loaded properly, reject any rollback request" + ) + for _rollback_req in request.iter_ecu(): + response.add_ecu( + api_types.RollbackResponseEcu( + ecu_id=_rollback_req.ecu_id, + result=api_types.FailureType.UNRECOVERABLE, + ) + ) + return response + # first: dispatch rollback request to all directly connected subECUs tasks: dict[asyncio.Task, ECUContact] = {} for ecu_contact in self.sub_ecus: diff --git a/src/otaclient/ota_core.py b/src/otaclient/ota_core.py index 203f719c1..f42b4c665 100644 --- a/src/otaclient/ota_core.py +++ b/src/otaclient/ota_core.py @@ -36,6 +36,7 @@ import requests.exceptions as requests_exc from requests import Response +import otaclient.configs.cfg as otaclient_cfg from ota_metadata.legacy import parser as ota_metadata_parser from ota_metadata.legacy import types as ota_metadata_types from ota_metadata.utils.cert_store import ( @@ -66,7 +67,12 @@ ) from otaclient._utils import SharedOTAClientStatusWriter, get_traceback, wait_and_log from otaclient.boot_control import BootControllerProtocol, get_boot_controller -from otaclient.configs.cfg import cfg, ecu_info, proxy_info +from otaclient.configs.cfg import ( + ECU_INFO_LOADED_SUCCESSFULLY, + cfg, + ecu_info, + proxy_info, +) from otaclient.create_standby import ( StandbySlotCreatorProtocol, get_standby_slot_creator, @@ -672,17 +678,7 @@ def __init__( ) return - # load and report booted OTA status - _boot_ctrl_loaded_ota_status = self.boot_controller.get_booted_ota_status() - self._live_ota_status = _boot_ctrl_loaded_ota_status - status_report_queue.put_nowait( - StatusReport( - payload=OTAStatusChangeReport( - new_ota_status=_boot_ctrl_loaded_ota_status, - ), - ) - ) - + # ------ load firmware version ------ # self.current_version = self.boot_controller.load_version() status_report_queue.put_nowait( StatusReport( @@ -691,7 +687,9 @@ def __init__( ), ) ) + logger.info(f"firmware_version: {self.current_version}") + # ------ load CA store ------ # self.ca_chains_store = None try: self.ca_chains_store = load_ca_cert_chains(cfg.CERT_DPATH) @@ -701,9 +699,36 @@ def __init__( self.ca_chains_store = CAChainStore() - self.started = True - logger.info("otaclient started") - logger.info(f"firmware_version: {self.current_version}") + # load and report booted OTA status + _boot_ctrl_loaded_ota_status = self.boot_controller.get_booted_ota_status() + if not otaclient_cfg.ECU_INFO_LOADED_SUCCESSFULLY: + logger.error( + "ecu_info.yaml file is not loaded properly, will reject any OTA request." + ) + logger.error(f"set live_ota_status to {OTAStatus.FAILURE!r}") + self._live_ota_status = OTAStatus.FAILURE + status_report_queue.put_nowait( + StatusReport( + payload=OTAStatusChangeReport( + new_ota_status=OTAStatus.FAILURE, + failure_type=FailureType.UNRECOVERABLE, + failure_reason=f"ecu_info.yaml file is broken or missing, please check {cfg.ECU_INFO_FPATH}. " + "reject any OTA request.", + ), + ) + ) + else: + self._live_ota_status = _boot_ctrl_loaded_ota_status + status_report_queue.put_nowait( + StatusReport( + payload=OTAStatusChangeReport( + new_ota_status=_boot_ctrl_loaded_ota_status, + ), + ) + ) + + self.started = True + logger.info("otaclient started") def _on_failure( self, @@ -843,6 +868,20 @@ def main( ) ) + elif not self.started: + _err_msg = "reject OTA request due to otaclient is not (yet) started." + if not ECU_INFO_LOADED_SUCCESSFULLY: + _err_msg = f"reject OTA request due to {cfg.ECU_INFO_FPATH} missing or broken" + + logger.error(_err_msg) + resp_queue.put_nowait( + IPCResponse( + res=IPCResEnum.REJECT_OTHER, + msg=_err_msg, + session_id=request.session_id, + ) + ) + elif isinstance(request, UpdateRequestV2): _update_thread = threading.Thread( diff --git a/tests/test_otaclient/test_configs/test_ecu_info.py b/tests/test_otaclient/test_configs/test_ecu_info.py index b85c31e0b..d21522659 100644 --- a/tests/test_otaclient/test_configs/test_ecu_info.py +++ b/tests/test_otaclient/test_configs/test_ecu_info.py @@ -25,23 +25,23 @@ @pytest.mark.parametrize( - "ecu_info_yaml, expected_ecu_info", + "ecu_info_yaml, expected_res", ( # --- case 1: invalid ecu_info --- # # case 1.1: valid yaml(empty file), invalid ecu_info ( "# this is an empty file", - DEFAULT_ECU_INFO, + (False, DEFAULT_ECU_INFO), ), # case 1.2: valid yaml(array), invalid ecu_info ( ("- this is an\n- yaml file that\n- contains a array\n"), - DEFAULT_ECU_INFO, + (False, DEFAULT_ECU_INFO), ), # case 1.2: invalid yaml ( " - \n not a \n [ valid yaml", - DEFAULT_ECU_INFO, + (False, DEFAULT_ECU_INFO), ), # --- case 2: single ECU --- # # case 2.1: basic single ECU @@ -52,10 +52,13 @@ 'ip_addr: "192.168.1.1"\n' "bootloader: jetson-cboot\n" ), - ECUInfo( - ecu_id="autoware", - ip_addr=IPv4Address("192.168.1.1"), - bootloader=BootloaderType.JETSON_CBOOT, + ( + True, + ECUInfo( + ecu_id="autoware", + ip_addr=IPv4Address("192.168.1.1"), + bootloader=BootloaderType.JETSON_CBOOT, + ), ), ), # case 2.2: single ECU with bootloader type specified @@ -66,10 +69,13 @@ 'ip_addr: "192.168.1.1"\n' 'bootloader: "grub"\n' ), - ECUInfo( - ecu_id="autoware", - ip_addr=IPv4Address("192.168.1.1"), - bootloader=BootloaderType.GRUB, + ( + True, + ECUInfo( + ecu_id="autoware", + ip_addr=IPv4Address("192.168.1.1"), + bootloader=BootloaderType.GRUB, + ), ), ), # --- case 3: multiple ECUs --- # @@ -87,21 +93,24 @@ '- ecu_id: "p2"\n' ' ip_addr: "192.168.0.12"\n' ), - ECUInfo( - ecu_id="autoware", - ip_addr=IPv4Address("192.168.1.1"), - bootloader=BootloaderType.GRUB, - available_ecu_ids=["autoware", "p1", "p2"], - secondaries=[ - ECUContact( - ecu_id="p1", - ip_addr=IPv4Address("192.168.0.11"), - ), - ECUContact( - ecu_id="p2", - ip_addr=IPv4Address("192.168.0.12"), - ), - ], + ( + True, + ECUInfo( + ecu_id="autoware", + ip_addr=IPv4Address("192.168.1.1"), + bootloader=BootloaderType.GRUB, + available_ecu_ids=["autoware", "p1", "p2"], + secondaries=[ + ECUContact( + ecu_id="p1", + ip_addr=IPv4Address("192.168.0.11"), + ), + ECUContact( + ecu_id="p2", + ip_addr=IPv4Address("192.168.0.12"), + ), + ], + ), ), ), # case 3.2: multiple ECUs, with main ECU's bootloader specified @@ -118,32 +127,37 @@ '- ecu_id: "p2"\n' ' ip_addr: "192.168.0.12"\n' ), - ECUInfo( - ecu_id="autoware", - ip_addr=IPv4Address("192.168.1.1"), - bootloader=BootloaderType.JETSON_UEFI, - available_ecu_ids=["autoware", "p1", "p2"], - secondaries=[ - ECUContact( - ecu_id="p1", - ip_addr=IPv4Address("192.168.0.11"), - ), - ECUContact( - ecu_id="p2", - ip_addr=IPv4Address("192.168.0.12"), - ), - ], + ( + True, + ECUInfo( + ecu_id="autoware", + ip_addr=IPv4Address("192.168.1.1"), + bootloader=BootloaderType.JETSON_UEFI, + available_ecu_ids=["autoware", "p1", "p2"], + secondaries=[ + ECUContact( + ecu_id="p1", + ip_addr=IPv4Address("192.168.0.11"), + ), + ECUContact( + ecu_id="p2", + ip_addr=IPv4Address("192.168.0.12"), + ), + ], + ), ), ), ), ) -def test_ecu_info(tmp_path: Path, ecu_info_yaml: str, expected_ecu_info: ECUInfo): +def test_ecu_info( + tmp_path: Path, ecu_info_yaml: str, expected_res: tuple[bool, ECUInfo] +): # --- preparation --- # (ota_dir := tmp_path / "boot" / "ota").mkdir(parents=True, exist_ok=True) (ecu_info_file := ota_dir / "ecu_info.yaml").write_text(ecu_info_yaml) # --- execution --- # - _, loaded_ecu_info = parse_ecu_info(ecu_info_file) + res = parse_ecu_info(ecu_info_file) # --- assertion --- # - assert loaded_ecu_info == expected_ecu_info + assert res == expected_res diff --git a/tests/test_otaclient/test_configs/test_proxy_info.py b/tests/test_otaclient/test_configs/test_proxy_info.py index e2b2d40f3..38a639485 100644 --- a/tests/test_otaclient/test_configs/test_proxy_info.py +++ b/tests/test_otaclient/test_configs/test_proxy_info.py @@ -19,12 +19,16 @@ from pathlib import Path import pytest +import pytest_mock +import otaclient.configs._proxy_info as _proxy_info_module from otaclient.configs import ProxyInfo from otaclient.configs._proxy_info import DEFAULT_PROXY_INFO, parse_proxy_info logger = logging.getLogger(__name__) +MODULE_NAME = _proxy_info_module.__name__ + @pytest.mark.parametrize( "_input_yaml, _expected", @@ -35,7 +39,7 @@ # NOTE: this default value is for x1 backward compatibility. ( "# this is an empty file", - DEFAULT_PROXY_INFO, + (False, DEFAULT_PROXY_INFO), ), # ------ case 2: typical sub ECU's proxy_info.yaml ------ # ( @@ -45,16 +49,19 @@ "enable_local_ota_proxy_cache: true\n" 'logging_server: "http://10.0.0.1:8083"\n' ), - ProxyInfo( - **{ - "format_version": 1, - "upper_ota_proxy": "http://10.0.0.1:8082", - "enable_local_ota_proxy": True, - "enable_local_ota_proxy_cache": True, - "local_ota_proxy_listen_addr": "0.0.0.0", - "local_ota_proxy_listen_port": 8082, - "logging_server": "http://10.0.0.1:8083", - } + ( + True, + ProxyInfo( + **{ + "format_version": 1, + "upper_ota_proxy": "http://10.0.0.1:8082", + "enable_local_ota_proxy": True, + "enable_local_ota_proxy_cache": True, + "local_ota_proxy_listen_addr": "0.0.0.0", + "local_ota_proxy_listen_port": 8082, + "logging_server": "http://10.0.0.1:8083", + } + ), ), ), # ------ case 3: invalid/corrupted proxy_info.yaml ------ # @@ -63,7 +70,7 @@ # proxy_info.yaml will be used. ( "not a valid proxy_info.yaml", - DEFAULT_PROXY_INFO, + (False, DEFAULT_PROXY_INFO), ), # ------ case 4: proxy_info.yaml is valid yaml but contains invalid fields ------ # # in this case, default predefined default proxy_info.yaml will be loaded @@ -81,31 +88,39 @@ "local_ota_proxy_listen_addr: 123\n" 'local_ota_proxy_listen_port: "2808"\n' ), - DEFAULT_PROXY_INFO, + (False, DEFAULT_PROXY_INFO), ), # ------ case 5: corrupted/invalid yaml ------ # # in this case, default predefined default proxy_info.yaml will be loaded ( "/t/t/t/t/t/t/t/tyaml file should not contain tabs/t/t/t/", - DEFAULT_PROXY_INFO, + (False, DEFAULT_PROXY_INFO), ), # ------ case 6: backward compatibility test ------ # # for superseded field, the corresponding field should be assigned. # for removed field, it should not impact the config file loading. ( "enable_ota_proxy: true\ngateway: false\n", - DEFAULT_PROXY_INFO, + (True, DEFAULT_PROXY_INFO), ), # ------ case 7(20240626): NetworkPort allow str value ------ # ( 'local_ota_proxy_listen_port: "8082"', - ProxyInfo(local_ota_proxy_listen_port=8082), + (True, ProxyInfo(local_ota_proxy_listen_port=8082)), ), ), ) -def test_proxy_info(tmp_path: Path, _input_yaml: str, _expected: ProxyInfo): +def test_proxy_info( + tmp_path: Path, + _input_yaml: str, + _expected: tuple[bool, ProxyInfo], + mocker: pytest_mock.MockerFixture, +) -> None: + # disable deprecation warning on test + mocker.patch(f"{MODULE_NAME}._deprecation_check") + proxy_info_file = tmp_path / "proxy_info.yml" proxy_info_file.write_text(_input_yaml) - _, _proxy_info = parse_proxy_info(str(proxy_info_file)) + _res = parse_proxy_info(str(proxy_info_file)) - assert _proxy_info == _expected + assert _res == _expected