diff --git a/CHANGELOG.md b/CHANGELOG.md index 1284b51429..4a88fb589e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,9 @@ ENHANCEMENTS: * Specify default_outbound_access_enabled = false setting for all subnets ([#4757](https://github.com/microsoft/AzureTRE/pull/4757)) * Pin all GitHub Actions workflow steps to full commit SHAs to prevent supply chain attacks plus update to latest releases ([#4886](https://github.com/microsoft/AzureTRE/pull/4886)) +BUG FIXES: +* Fix `OSError: [Errno 7] Argument list too long` when deploying many workspaces by replacing `--param` CLI arguments with a temporary Porter parameter set file ([#4903](https://github.com/microsoft/AzureTRE/issues/4903)) + ## (0.28.0) (March 2, 2026) **BREAKING CHANGES** * Sonatype Nexus shared service now requires explicit EULA acceptance (`accept_nexus_eula: true`) when deploying. This ensures compliance with Sonatype Nexus Community Edition licensing. ([#4842](https://github.com/microsoft/AzureTRE/issues/4842)) diff --git a/resource_processor/_version.py b/resource_processor/_version.py index 26c36ca79a..f2b93b13de 100644 --- a/resource_processor/_version.py +++ b/resource_processor/_version.py @@ -1 +1 @@ -__version__ = "0.13.3" +__version__ = "0.13.4" diff --git a/resource_processor/helpers/commands.py b/resource_processor/helpers/commands.py index 35047d309a..20c67c3160 100644 --- a/resource_processor/helpers/commands.py +++ b/resource_processor/helpers/commands.py @@ -2,6 +2,8 @@ import json import base64 import logging +import tempfile +import uuid from urllib.parse import urlparse from shared.logging import logger, shell_output_logger @@ -78,7 +80,7 @@ def azure_acr_login_command(config): async def build_porter_command(config, msg_body, custom_action=False): porter_parameter_keys = await get_porter_parameter_keys(config, msg_body) - porter_parameters = [] + param_set_entries = [] if porter_parameter_keys is None: logger.warning("Unknown porter parameters - explain probably failed.") @@ -117,9 +119,30 @@ async def build_porter_command(config, msg_body, custom_action=False): val_base64_bytes = base64.b64encode(val_bytes) parameter_value = val_base64_bytes.decode("ascii") - porter_parameters.extend(["--param", f"{parameter_name}={parameter_value}"]) + param_set_entries.append({ + "name": parameter_name, + "source": {"value": str(parameter_value)} + }) installation_id = msg_body['id'] + param_set_name = f"tre-params-{installation_id}-{uuid.uuid4().hex[:8]}" + + param_set_file = None + if param_set_entries: + param_set = { + "schemaType": "ParameterSet", + "schemaVersion": "1.0.1", + "name": param_set_name, + "namespace": "", + "parameters": param_set_entries + } + with tempfile.NamedTemporaryFile(mode='w', suffix='.json', delete=False) as f: + json.dump(param_set, f) + param_set_file = f.name + + commands = [] + if param_set_file: + commands.append(["porter", "parameters", "apply", param_set_file]) command = ["porter"] if custom_action: @@ -131,7 +154,8 @@ async def build_porter_command(config, msg_body, custom_action=False): "--reference", f"{config['registry_server']}/{msg_body['name']}:v{msg_body['version']}" ]) - command.extend(porter_parameters) + if param_set_file: + command.extend(["--parameter-set", param_set_name]) command.append("--force") command.extend(["--credential-set", "arm_auth"]) command.extend(["--credential-set", "aad_auth"]) @@ -139,7 +163,9 @@ async def build_porter_command(config, msg_body, custom_action=False): if msg_body['action'] == 'upgrade': command.append("--force-upgrade") - return [command] + commands.append(command) + + return (commands, param_set_file, param_set_name) async def build_porter_command_for_outputs(msg_body): diff --git a/resource_processor/tests_rp/test_commands.py b/resource_processor/tests_rp/test_commands.py index 2caab5c0bd..aa88f910d4 100644 --- a/resource_processor/tests_rp/test_commands.py +++ b/resource_processor/tests_rp/test_commands.py @@ -1,5 +1,6 @@ import json import asyncio +import os import pytest from unittest.mock import patch, AsyncMock from helpers.commands import azure_login_command, apply_porter_credentials_sets_command, azure_acr_login_command, build_porter_command, build_porter_command_for_outputs, get_porter_parameter_keys, run_command_helper, get_special_porter_param_value @@ -58,17 +59,36 @@ async def test_build_porter_command(mock_get_porter_parameter_keys): msg_body = {"id": "guid", "action": "install", "name": "mybundle", "version": "1.0.0", "parameters": {"param1": "value1"}} mock_get_porter_parameter_keys.return_value = ["param1"] - expected_command = [[ - "porter", "install", "guid", - "--reference", "myregistry.azurecr.io/mybundle:v1.0.0", - "--param", "param1=value1", - "--force", - "--credential-set", "arm_auth", - "--credential-set", "aad_auth" - ]] - - command = await build_porter_command(config, msg_body) - assert command == expected_command + commands, param_set_file, param_set_name = await build_porter_command(config, msg_body) + try: + assert param_set_file is not None + assert param_set_name.startswith("tre-params-guid-") + assert len(param_set_name) == len("tre-params-guid-") + 8 + assert os.path.exists(param_set_file) + + # First command applies the parameter set to Porter's store + assert commands[0] == ["porter", "parameters", "apply", param_set_file] + + # Second command is the main porter install using the parameter set by name + assert commands[1] == [ + "porter", "install", "guid", + "--reference", "myregistry.azurecr.io/mybundle:v1.0.0", + "--parameter-set", param_set_name, + "--force", + "--credential-set", "arm_auth", + "--credential-set", "aad_auth" + ] + + with open(param_set_file) as f: + param_set = json.load(f) + + assert param_set["schemaType"] == "ParameterSet" + assert param_set["name"] == param_set_name + assert len(param_set["parameters"]) == 1 + assert param_set["parameters"][0] == {"name": "param1", "source": {"value": "value1"}} + finally: + if param_set_file and os.path.exists(param_set_file): + os.unlink(param_set_file) @pytest.mark.asyncio @@ -78,18 +98,28 @@ async def test_build_porter_command_for_upgrade(mock_get_porter_parameter_keys): msg_body = {"id": "guid", "action": "upgrade", "name": "mybundle", "version": "1.0.0", "parameters": {"param1": "value1"}} mock_get_porter_parameter_keys.return_value = ["param1"] - expected_command = [[ - "porter", "upgrade", "guid", - "--reference", "myregistry.azurecr.io/mybundle:v1.0.0", - "--param", "param1=value1", - "--force", - "--credential-set", "arm_auth", - "--credential-set", "aad_auth", - "--force-upgrade" - ]] - - command = await build_porter_command(config, msg_body) - assert command == expected_command + commands, param_set_file, param_set_name = await build_porter_command(config, msg_body) + try: + assert param_set_file is not None + assert param_set_name.startswith("tre-params-guid-") + assert os.path.exists(param_set_file) + + # First command applies the parameter set to Porter's store + assert commands[0] == ["porter", "parameters", "apply", param_set_file] + + # Second command is the main porter upgrade using the parameter set by name + assert commands[1] == [ + "porter", "upgrade", "guid", + "--reference", "myregistry.azurecr.io/mybundle:v1.0.0", + "--parameter-set", param_set_name, + "--force", + "--credential-set", "arm_auth", + "--credential-set", "aad_auth", + "--force-upgrade" + ] + finally: + if param_set_file and os.path.exists(param_set_file): + os.unlink(param_set_file) @pytest.mark.asyncio @@ -106,6 +136,26 @@ async def test_build_porter_command_for_outputs(): assert command == expected_command +@pytest.mark.asyncio +async def test_build_porter_command_no_parameters(mock_get_porter_parameter_keys): + """Test build_porter_command returns no --parameter-set when there are no parameters.""" + config = {"registry_server": "myregistry.azurecr.io"} + msg_body = {"id": "guid", "action": "install", "name": "mybundle", "version": "1.0.0", "parameters": {}} + mock_get_porter_parameter_keys.return_value = [] + + commands, param_set_file, param_set_name = await build_porter_command(config, msg_body) + + assert param_set_file is None + assert param_set_name.startswith("tre-params-guid-") + assert commands == [[ + "porter", "install", "guid", + "--reference", "myregistry.azurecr.io/mybundle:v1.0.0", + "--force", + "--credential-set", "arm_auth", + "--credential-set", "aad_auth" + ]] + + @pytest.mark.asyncio async def test_build_porter_command_with_complex_parameters(mock_get_porter_parameter_keys): """Test build_porter_command function with complex parameter types (dict, list).""" @@ -127,33 +177,42 @@ async def test_build_porter_command_with_complex_parameters(mock_get_porter_para mock_get_porter_parameter_keys.return_value = ["dict_param", "list_param", "string_param"] - command = await build_porter_command(config, msg_body) + commands, param_set_file, param_set_name = await build_porter_command(config, msg_body) + + try: + # First command is the apply command + assert commands[0] == ["porter", "parameters", "apply", param_set_file] - # Verify the command contains properly encoded complex parameters - command_args = command[0] + # Main porter command should reference the parameter set by name + main_command = commands[1] + assert "--parameter-set" in main_command + assert param_set_name in main_command + assert param_set_name.startswith("tre-params-guid-") + assert "--param" not in main_command - # Find the indices of parameters - param_indices = [i for i, arg in enumerate(command_args) if arg == "--param"] - param_values = [command_args[i + 1] for i in param_indices] + # Verify the param set file contains the correct parameters + assert param_set_file is not None + with open(param_set_file) as f: + param_set = json.load(f) - # Check for all parameters - dict_param = next((p for p in param_values if p.startswith("dict_param=")), None) - list_param = next((p for p in param_values if p.startswith("list_param=")), None) - string_param = next((p for p in param_values if p.startswith("string_param=")), None) + params_by_name = {p["name"]: p["source"]["value"] for p in param_set["parameters"]} - assert dict_param is not None - assert list_param is not None - assert string_param is not None - assert string_param == "string_param=simple_value" + assert "dict_param" in params_by_name + assert "list_param" in params_by_name + assert "string_param" in params_by_name + assert params_by_name["string_param"] == "simple_value" - # Verify the dict and list are base64 encoded - import base64 + # Verify the dict and list are base64 encoded + import base64 - dict_encoded = base64.b64encode(json.dumps(dict_value).encode("ascii")).decode("ascii") - list_encoded = base64.b64encode(json.dumps(list_value).encode("ascii")).decode("ascii") + dict_encoded = base64.b64encode(json.dumps(dict_value).encode("ascii")).decode("ascii") + list_encoded = base64.b64encode(json.dumps(list_value).encode("ascii")).decode("ascii") - assert dict_param == f"dict_param={dict_encoded}" - assert list_param == f"list_param={list_encoded}" + assert params_by_name["dict_param"] == dict_encoded + assert params_by_name["list_param"] == list_encoded + finally: + if param_set_file and os.path.exists(param_set_file): + os.unlink(param_set_file) @pytest.mark.asyncio diff --git a/resource_processor/vmss_porter/runner.py b/resource_processor/vmss_porter/runner.py index 120ececba0..3c8b9e4507 100644 --- a/resource_processor/vmss_porter/runner.py +++ b/resource_processor/vmss_porter/runner.py @@ -3,6 +3,7 @@ from multiprocessing import Process import json import asyncio +import os import sys from helpers.commands import azure_acr_login_command, azure_login_command, build_porter_command, build_porter_command_for_outputs, apply_porter_credentials_sets_command, run_command_helper from shared.config import get_config @@ -161,6 +162,15 @@ def service_bus_message_generator(sb_message: dict, status: str, deployment_mess return resource_request_message +async def _cleanup_param_set(param_set_name: str, param_set_file: str, config: dict): + """Remove a Porter parameter set from its local store and delete the temp file.""" + await run_command_helper(["porter", "parameters", "delete", param_set_name], config, "Delete parameter set") + try: + os.unlink(param_set_file) + except OSError as e: + logger.debug(f"Best-effort cleanup: could not delete temp parameter set file '{param_set_file}': {e}") + + async def invoke_porter_action(msg_body: dict, sb_client: ServiceBusClient, config: dict) -> bool: """ Handle resource message by invoking specified porter action (i.e. install, uninstall) @@ -178,10 +188,14 @@ async def invoke_porter_action(msg_body: dict, sb_client: ServiceBusClient, conf # Build and run porter command (flagging if its a built-in action or custom so we can adapt porter command appropriately) is_custom_action = action not in ["install", "upgrade", "uninstall"] - porter_command = await build_porter_command(config, msg_body, is_custom_action) + porter_command, param_set_file, param_set_name = await build_porter_command(config, msg_body, is_custom_action) logger.debug("Starting to run porter execution command...") - returncode, _, err = await run_porter(porter_command, config) + try: + returncode, _, err = await run_porter(porter_command, config) + finally: + if param_set_file: + await _cleanup_param_set(param_set_name, param_set_file, config) logger.debug("Finished running porter execution command.") action_completed_without_error = False @@ -203,8 +217,12 @@ async def invoke_porter_action(msg_body: dict, sb_client: ServiceBusClient, conf if "upgrade" == action and ("could not find installation" in err or "The installation cannot be upgraded, because it is not installed." in err): logger.warning("Upgrade failed, attempting install...") msg_body['action'] = "install" - porter_command = await build_porter_command(config, msg_body, False) - returncode, _, err = await run_porter(porter_command, config) + porter_command, param_set_file, param_set_name = await build_porter_command(config, msg_body, False) + try: + returncode, _, err = await run_porter(porter_command, config) + finally: + if param_set_file: + await _cleanup_param_set(param_set_name, param_set_file, config) if returncode == 0: action_completed_without_error = True