From 1b011efa6d8313d2214cd61ea75c3763a6c953a3 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 11 Sep 2022 17:32:33 +0300 Subject: [PATCH 01/16] add 'previous status' field to 'status changed' message --- api_app/api/routes/airlock_resource_helpers.py | 4 ++-- api_app/event_grid/event_sender.py | 13 +++++++------ api_app/models/domain/events.py | 5 +++-- .../test_routes/test_airlock_resource_helpers.py | 2 +- 4 files changed, 13 insertions(+), 11 deletions(-) diff --git a/api_app/api/routes/airlock_resource_helpers.py b/api_app/api/routes/airlock_resource_helpers.py index 79d0db8568..59b81f047c 100644 --- a/api_app/api/routes/airlock_resource_helpers.py +++ b/api_app/api/routes/airlock_resource_helpers.py @@ -34,7 +34,7 @@ async def save_and_publish_event_airlock_request(airlock_request: AirlockRequest try: logging.debug(f"Sending status changed event for airlock request item: {airlock_request.id}") - await send_status_changed_event(airlock_request) + await send_status_changed_event(airlock_request=airlock_request, previous_status=None) await send_airlock_notification_event(airlock_request, role_assignment_details) except Exception as e: airlock_request_repo.delete_item(airlock_request.id) @@ -55,7 +55,7 @@ async def update_and_publish_event_airlock_request(airlock_request: AirlockReque try: logging.debug(f"Sending status changed event for airlock request item: {airlock_request.id}") - await send_status_changed_event(updated_airlock_request) + await send_status_changed_event(airlock_request=updated_airlock_request, previous_status=airlock_request.status) access_service = get_access_service() role_assignment_details = access_service.get_workspace_role_assignment_details(workspace) await send_airlock_notification_event(updated_airlock_request, role_assignment_details) diff --git a/api_app/event_grid/event_sender.py b/api_app/event_grid/event_sender.py index 7fba8bcd0c..b52de58bb7 100644 --- a/api_app/event_grid/event_sender.py +++ b/api_app/event_grid/event_sender.py @@ -1,26 +1,27 @@ import logging import re -from typing import Dict +from typing import Dict, Optional from azure.eventgrid import EventGridEvent from models.domain.events import StatusChangedData, AirlockNotificationData from event_grid.helpers import publish_event from core import config -from models.domain.airlock_request import AirlockRequest +from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus -async def send_status_changed_event(airlock_request: AirlockRequest): +async def send_status_changed_event(airlock_request: AirlockRequest, previous_status: Optional[AirlockRequestStatus]): request_id = airlock_request.id - status = airlock_request.status.value + new_status = airlock_request.status.value + previous_status = previous_status.value if previous_status else None request_type = airlock_request.requestType.value short_workspace_id = airlock_request.workspaceId[-4:] status_changed_event = EventGridEvent( event_type="statusChanged", - data=StatusChangedData(request_id=request_id, status=status, type=request_type, workspace_id=short_workspace_id).__dict__, + data=StatusChangedData(request_id=request_id, new_status=new_status, previous_status=previous_status, type=request_type, workspace_id=short_workspace_id).__dict__, subject=f"{request_id}/statusChanged", data_version="2.0" ) - logging.info(f"Sending status changed event with request ID {request_id}, status: {status}") + logging.info(f"Sending status changed event with request ID {request_id}, new status: {new_status}, previous status: {previous_status}") await publish_event(status_changed_event, config.EVENT_GRID_STATUS_CHANGED_TOPIC_ENDPOINT) diff --git a/api_app/models/domain/events.py b/api_app/models/domain/events.py index 9e8a34d2fd..4117a0c1d2 100644 --- a/api_app/models/domain/events.py +++ b/api_app/models/domain/events.py @@ -1,4 +1,4 @@ -from typing import Dict +from typing import Dict, Optional from models.domain.azuretremodel import AzureTREModel @@ -12,6 +12,7 @@ class AirlockNotificationData(AzureTREModel): class StatusChangedData(AzureTREModel): request_id: str - status: str + new_status: str + previous_status: Optional[str] type: str workspace_id: str diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py index 32152c4224..4fdbc930c4 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py +++ b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py @@ -44,7 +44,7 @@ def sample_airlock_request(status=AirlockRequestStatus.Draft): def sample_status_changed_event(status="draft"): status_changed_event = EventGridEvent( event_type="statusChanged", - data=StatusChangedData(request_id=AIRLOCK_REQUEST_ID, status=status, type=AirlockRequestType.Import, workspace_id=WORKSPACE_ID[-4:]).__dict__, + data=StatusChangedData(request_id=AIRLOCK_REQUEST_ID, new_status=status, type=AirlockRequestType.Import, workspace_id=WORKSPACE_ID[-4:]).__dict__, subject=f"{AIRLOCK_REQUEST_ID}/statusChanged", data_version="2.0" ) From a64bd87689a30f5d0f430a2fed460cb4afeaf562 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 11 Sep 2022 17:35:12 +0300 Subject: [PATCH 02/16] add support for container deletion in azure function 'toDeleteTrigger' --- airlock_processor/BlobCreatedTrigger/__init__.py | 2 +- airlock_processor/ToDeleteTrigger/__init__.py | 7 ++++++- airlock_processor/shared_code/blob_operations.py | 4 ++++ airlock_processor/shared_code/constants.py | 1 + 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/airlock_processor/BlobCreatedTrigger/__init__.py b/airlock_processor/BlobCreatedTrigger/__init__.py index f39b34e8d1..24759da58d 100644 --- a/airlock_processor/BlobCreatedTrigger/__init__.py +++ b/airlock_processor/BlobCreatedTrigger/__init__.py @@ -81,6 +81,6 @@ def main(msg: func.ServiceBusMessage, subject=request_id, event_type="Airlock.ToDelete", event_time=datetime.datetime.utcnow(), - data_version="1.0" + data_version=constants.TO_DELETE_EVENT_DATA_VERSION ) ) diff --git a/airlock_processor/ToDeleteTrigger/__init__.py b/airlock_processor/ToDeleteTrigger/__init__.py index 9251eca217..a82a83973c 100644 --- a/airlock_processor/ToDeleteTrigger/__init__.py +++ b/airlock_processor/ToDeleteTrigger/__init__.py @@ -15,6 +15,11 @@ def delete_blob_and_container_if_last_blob(blob_url: str): credential=credential) container_client = blob_service_client.get_container_client(container_name) + if not blob_name: + logging.info(f'No specific blob specified, deleting the entire container: {container_name}') + container_client.delete_container() + return + # If it's the only blob in the container, we need to delete the container too # Check how many blobs are in the container (note: this exausts the generator) blobs_num = sum(1 for _ in container_client.list_blobs()) @@ -33,7 +38,7 @@ def delete_blob_and_container_if_last_blob(blob_url: str): def main(msg: func.ServiceBusMessage): body = msg.get_body().decode('utf-8') - logging.info(f'Python ServiceBus queue trigger processed mesage: {body}') + logging.info(f'Python ServiceBus queue trigger processed message: {body}') json_body = json.loads(body) blob_url = json_body["data"]["blob_to_delete"] diff --git a/airlock_processor/shared_code/blob_operations.py b/airlock_processor/shared_code/blob_operations.py index 42459ea2d6..231a42ccc4 100644 --- a/airlock_processor/shared_code/blob_operations.py +++ b/airlock_processor/shared_code/blob_operations.py @@ -121,3 +121,7 @@ def get_blob_info_from_topic_and_subject(topic: str, subject: str): def get_blob_info_from_blob_url(blob_url: str) -> Tuple[str, str, str]: # Example of blob url: https://stalimappws663d.blob.core.windows.net/50866a82-d13a-4fd5-936f-deafdf1022ce/test_blob.txt return re.search(r'https://(.*?).blob.core.windows.net/(.*?)/(.*?)$', blob_url).groups() + + +def get_blob_url(account_name: str, container_name: str, blob_name='') -> str: + return f'{get_account_url(account_name)}{container_name}/{blob_name}' diff --git a/airlock_processor/shared_code/constants.py b/airlock_processor/shared_code/constants.py index 282e24d981..bab0247cc8 100644 --- a/airlock_processor/shared_code/constants.py +++ b/airlock_processor/shared_code/constants.py @@ -38,5 +38,6 @@ # Event Grid STEP_RESULT_EVENT_DATA_VERSION = "1.0" +TO_DELETE_EVENT_DATA_VERSION = "1.0" NO_THREATS = "No threats found" From c23976b38b548e491ba1e5b0b8e7d61b9baceac0 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 11 Sep 2022 17:44:40 +0300 Subject: [PATCH 03/16] handle request cancellation in StatusChangedQueueTrigger --- .../StatusChangedQueueTrigger/__init__.py | 151 ++++++++++++------ .../StatusChangedQueueTrigger/function.json | 9 +- .../tests/test_status_change_queue_trigger.py | 6 +- 3 files changed, 117 insertions(+), 49 deletions(-) diff --git a/airlock_processor/StatusChangedQueueTrigger/__init__.py b/airlock_processor/StatusChangedQueueTrigger/__init__.py index 9af33a8792..cc81175446 100644 --- a/airlock_processor/StatusChangedQueueTrigger/__init__.py +++ b/airlock_processor/StatusChangedQueueTrigger/__init__.py @@ -1,4 +1,5 @@ import logging +from typing import Optional import azure.functions as func import datetime @@ -14,7 +15,8 @@ class RequestProperties(BaseModel): request_id: str - status: str + new_status: str + previous_status: Optional[str] type: str workspace_id: str @@ -28,22 +30,23 @@ def __init__(self, source_account_name: str, dest_account_name: str): self.dest_account_name = dest_account_name -def main(msg: func.ServiceBusMessage, outputEvent: func.Out[func.EventGridOutputEvent]): +def main(msg: func.ServiceBusMessage, step_result_event: func.Out[func.EventGridOutputEvent], to_delete_event: func.Out[func.EventGridOutputEvent]): try: request_properties = extract_properties(msg) - request_files = get_request_files(request_properties) if request_properties.status == constants.STAGE_SUBMITTED else None - handle_status_changed(request_properties, outputEvent, request_files) + request_files = get_request_files(request_properties) if request_properties.new_status == constants.STAGE_SUBMITTED else None + handle_status_changed(request_properties, step_result_event, to_delete_event, request_files) except NoFilesInRequestException: - set_output_event_to_report_failure(outputEvent, request_properties, failure_reason=constants.NO_FILES_IN_REQUEST_MESSAGE, request_files=request_files) + set_output_event_to_report_failure(step_result_event, request_properties, failure_reason=constants.NO_FILES_IN_REQUEST_MESSAGE, request_files=request_files) except TooManyFilesInRequestException: - set_output_event_to_report_failure(outputEvent, request_properties, failure_reason=constants.TOO_MANY_FILES_IN_REQUEST_MESSAGE, request_files=request_files) + set_output_event_to_report_failure(step_result_event, request_properties, failure_reason=constants.TOO_MANY_FILES_IN_REQUEST_MESSAGE, request_files=request_files) except Exception: - set_output_event_to_report_failure(outputEvent, request_properties, failure_reason=constants.UNKNOWN_REASON_MESSAGE, request_files=request_files) + set_output_event_to_report_failure(step_result_event, request_properties, failure_reason=constants.UNKNOWN_REASON_MESSAGE, request_files=request_files) -def handle_status_changed(request_properties: RequestProperties, outputEvent: func.Out[func.EventGridOutputEvent], request_files): - new_status = request_properties.status +def handle_status_changed(request_properties: RequestProperties, step_result_event: func.Out[func.EventGridOutputEvent], to_delete_event: func.Out[func.EventGridOutputEvent], request_files): + new_status = request_properties.new_status + previous_status = request_properties.previous_status req_id = request_properties.request_id ws_id = request_properties.workspace_id request_type = request_properties.type @@ -66,12 +69,18 @@ def handle_status_changed(request_properties: RequestProperties, outputEvent: fu blob_operations.create_container(account_name, req_id) return + if new_status == constants.STAGE_CANCELLED: + storage_account_name = get_storage_account(previous_status, request_type, ws_id) + container_url = blob_operations.get_blob_url(account_name=storage_account_name, container_name=req_id) + set_output_event_to_trigger_blob_deletion(to_delete_event, request_properties, blob_url=container_url) + return + if new_status == constants.STAGE_SUBMITTED: - set_output_event_to_report_request_files(outputEvent, request_properties, request_files) + set_output_event_to_report_request_files(step_result_event, request_properties, request_files) if (is_require_data_copy(new_status)): logging.info('Request with id %s. requires data copy between storage accounts', req_id) - containers_metadata = get_source_dest_for_copy(new_status, request_type, ws_id) + containers_metadata = get_source_dest_for_copy(new_status=new_status, previous_status=previous_status, request_type=request_type, short_workspace_id=ws_id) blob_operations.create_container(containers_metadata.dest_account_name, req_id) blob_operations.copy_data(containers_metadata.source_account_name, containers_metadata.dest_account_name, req_id) @@ -104,17 +113,11 @@ def is_require_data_copy(new_status: str): return False -def get_source_dest_for_copy(new_status: str, request_type: str, short_workspace_id: str) -> ContainersCopyMetadata: +def get_source_dest_for_copy(new_status: str, previous_status: str, request_type: str, short_workspace_id: str) -> ContainersCopyMetadata: # sanity if is_require_data_copy(new_status) is False: raise Exception("Given new status is not supported") - try: - tre_id = os.environ["TRE_ID"] - except KeyError as e: - logging.error(f'Missing environment variable: {e}') - raise - request_type = request_type.lower() if request_type != constants.IMPORT_TYPE and request_type != constants.EXPORT_TYPE: msg = "Airlock request type must be either '{}' or '{}".format(str(constants.IMPORT_TYPE), @@ -122,60 +125,118 @@ def get_source_dest_for_copy(new_status: str, request_type: str, short_workspace logging.error(msg) raise Exception(msg) + source_account_name = get_storage_account(previous_status, request_type, short_workspace_id) + dest_account_name = get_storage_account_destination_for_copy(new_status, request_type, short_workspace_id) + return ContainersCopyMetadata(source_account_name, dest_account_name) + + +def get_storage_account(status: str, request_type: str, short_workspace_id: str) -> str: + tre_id = _get_tre_id() + + if request_type == constants.IMPORT_TYPE: + if status == constants.STAGE_DRAFT: + return constants.STORAGE_ACCOUNT_NAME_IMPORT_EXTERNAL + tre_id + elif status == constants.STAGE_APPROVED: + return constants.STORAGE_ACCOUNT_NAME_IMPORT_APPROVED + short_workspace_id + elif status == constants.STAGE_REJECTED: + return constants.STORAGE_ACCOUNT_NAME_IMPORT_REJECTED + tre_id + elif status == constants.STAGE_BLOCKED_BY_SCAN: + return constants.STORAGE_ACCOUNT_NAME_IMPORT_BLOCKED + tre_id + elif status in [constants.STAGE_IN_REVIEW, constants.STAGE_SUBMITTED, constants.STAGE_APPROVAL_INPROGRESS, constants.STAGE_REJECTION_INPROGRESS, constants.STAGE_BLOCKING_INPROGRESS]: + return constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS + tre_id + + if request_type == constants.EXPORT_TYPE: + if status == constants.STAGE_DRAFT: + return constants.STORAGE_ACCOUNT_NAME_EXPORT_INTERNAL + short_workspace_id + elif status == constants.STAGE_APPROVED: + return constants.STORAGE_ACCOUNT_NAME_EXPORT_APPROVED + tre_id + elif status == constants.STAGE_REJECTED: + return constants.STORAGE_ACCOUNT_NAME_EXPORT_REJECTED + short_workspace_id + elif status == constants.STAGE_BLOCKED_BY_SCAN: + return constants.STORAGE_ACCOUNT_NAME_EXPORT_BLOCKED + short_workspace_id + elif status in [constants.STAGE_IN_REVIEW, constants.STAGE_SUBMITTED, constants.STAGE_APPROVAL_INPROGRESS, constants.STAGE_REJECTION_INPROGRESS, constants.STAGE_BLOCKING_INPROGRESS]: + return constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS + short_workspace_id + + error_message = f"Missing current storage account definition for status '{status}' and request type '{request_type}'." + logging.error(error_message) + raise Exception(error_message) + + +def get_storage_account_destination_for_copy(new_status: str, request_type: str, short_workspace_id: str) -> str: + tre_id = _get_tre_id() + if request_type == constants.IMPORT_TYPE: if new_status == constants.STAGE_SUBMITTED: - source_account_name = constants.STORAGE_ACCOUNT_NAME_IMPORT_EXTERNAL + tre_id - dest_account_name = constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS + tre_id + return constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS + tre_id elif new_status == constants.STAGE_APPROVAL_INPROGRESS: - source_account_name = constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS + tre_id - dest_account_name = constants.STORAGE_ACCOUNT_NAME_IMPORT_APPROVED + short_workspace_id + return constants.STORAGE_ACCOUNT_NAME_IMPORT_APPROVED + short_workspace_id elif new_status == constants.STAGE_REJECTION_INPROGRESS: - source_account_name = constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS + tre_id - dest_account_name = constants.STORAGE_ACCOUNT_NAME_IMPORT_REJECTED + tre_id + return constants.STORAGE_ACCOUNT_NAME_IMPORT_REJECTED + tre_id elif new_status == constants.STAGE_BLOCKING_INPROGRESS: - source_account_name = constants.STORAGE_ACCOUNT_NAME_IMPORT_INPROGRESS + tre_id - dest_account_name = constants.STORAGE_ACCOUNT_NAME_IMPORT_BLOCKED + tre_id - else: + return constants.STORAGE_ACCOUNT_NAME_IMPORT_BLOCKED + tre_id + + if request_type == constants.EXPORT_TYPE: if new_status == constants.STAGE_SUBMITTED: - source_account_name = constants.STORAGE_ACCOUNT_NAME_EXPORT_INTERNAL + short_workspace_id - dest_account_name = constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS + short_workspace_id + return constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS + short_workspace_id elif new_status == constants.STAGE_APPROVAL_INPROGRESS: - source_account_name = constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS + short_workspace_id - dest_account_name = constants.STORAGE_ACCOUNT_NAME_EXPORT_APPROVED + tre_id + return constants.STORAGE_ACCOUNT_NAME_EXPORT_APPROVED + tre_id elif new_status == constants.STAGE_REJECTION_INPROGRESS: - source_account_name = constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS + short_workspace_id - dest_account_name = constants.STORAGE_ACCOUNT_NAME_EXPORT_REJECTED + short_workspace_id + return constants.STORAGE_ACCOUNT_NAME_EXPORT_REJECTED + short_workspace_id elif new_status == constants.STAGE_BLOCKING_INPROGRESS: - source_account_name = constants.STORAGE_ACCOUNT_NAME_EXPORT_INPROGRESS + short_workspace_id - dest_account_name = constants.STORAGE_ACCOUNT_NAME_EXPORT_BLOCKED + short_workspace_id + return constants.STORAGE_ACCOUNT_NAME_EXPORT_BLOCKED + short_workspace_id - return ContainersCopyMetadata(source_account_name, dest_account_name) + error_message = f"Missing copy destination storage account definition for status '{new_status}' and request type '{request_type}'." + logging.error(error_message) + raise Exception(error_message) -def set_output_event_to_report_failure(outputEvent, request_properties, failure_reason, request_files): +def set_output_event_to_report_failure(step_result_event, request_properties, failure_reason, request_files): logging.exception(f"Failed processing Airlock request with ID: '{request_properties.request_id}', changing request status to '{constants.STAGE_FAILED}'.") - outputEvent.set( + step_result_event.set( func.EventGridOutputEvent( id=str(uuid.uuid4()), - data={"completed_step": request_properties.status, "new_status": constants.STAGE_FAILED, "request_id": request_properties.request_id, "request_files": request_files, "error_message": failure_reason}, + data={"completed_step": request_properties.new_status, "new_status": constants.STAGE_FAILED, "request_id": request_properties.request_id, "request_files": request_files, "error_message": failure_reason}, subject=request_properties.request_id, event_type="Airlock.StepResult", event_time=datetime.datetime.utcnow(), data_version=constants.STEP_RESULT_EVENT_DATA_VERSION)) -def set_output_event_to_report_request_files(outputEvent, request_properties, request_files): - outputEvent.set( +def set_output_event_to_report_request_files(step_result_event, request_properties, request_files): + logging.info(f'Sending file enumeration result for request with ID: {request_properties.request_id} result: {request_files}') + step_result_event.set( func.EventGridOutputEvent( id=str(uuid.uuid4()), - data={"completed_step": request_properties.status, "request_id": request_properties.request_id, "request_files": request_files}, + data={"completed_step": request_properties.new_status, "request_id": request_properties.request_id, "request_files": request_files}, subject=request_properties.request_id, event_type="Airlock.StepResult", event_time=datetime.datetime.utcnow(), data_version=constants.STEP_RESULT_EVENT_DATA_VERSION)) -def get_request_files(request_properties): - containers_metadata = get_source_dest_for_copy(request_properties.status, request_properties.type, request_properties.workspace_id) - storage_account_name = containers_metadata.source_account_name +def set_output_event_to_trigger_blob_deletion(to_delete_event, request_properties, blob_url): + logging.info(f'Sending deletion event to delete container of request with ID: {request_properties.request_id}. container URL: {blob_url}') + to_delete_event.set( + func.EventGridOutputEvent( + id=str(uuid.uuid4()), + data={"blob_to_delete": blob_url}, + subject=request_properties.request_id, + event_type="Airlock.ToDelete", + event_time=datetime.datetime.utcnow(), + data_version=constants.TO_DELETE_EVENT_DATA_VERSION + ) + ) + + +def get_request_files(request_properties: RequestProperties): + storage_account_name = get_storage_account(request_properties.previous_status, request_properties.type, request_properties.workspace_id) return blob_operations.get_request_files(account_name=storage_account_name, request_id=request_properties.request_id) + + +def _get_tre_id(): + try: + tre_id = os.environ["TRE_ID"] + except KeyError as e: + logging.error(f'Missing environment variable: {e}') + raise + return tre_id diff --git a/airlock_processor/StatusChangedQueueTrigger/function.json b/airlock_processor/StatusChangedQueueTrigger/function.json index 73e532cab4..ff8656643d 100644 --- a/airlock_processor/StatusChangedQueueTrigger/function.json +++ b/airlock_processor/StatusChangedQueueTrigger/function.json @@ -10,10 +10,17 @@ }, { "type": "eventGrid", - "name": "outputEvent", + "name": "stepResultEvent", "topicEndpointUri": "EVENT_GRID_STEP_RESULT_TOPIC_URI_SETTING", "topicKeySetting": "EVENT_GRID_STEP_RESULT_TOPIC_KEY_SETTING", "direction": "out" + }, + { + "type": "eventGrid", + "name": "toDeleteEvent", + "topicEndpointUri": "EVENT_GRID_TO_DELETE_TOPIC_URI_SETTING", + "topicKeySetting": "EVENT_GRID_TO_DELETE_TOPIC_KEY_SETTING", + "direction": "out" } ] } diff --git a/airlock_processor/tests/test_status_change_queue_trigger.py b/airlock_processor/tests/test_status_change_queue_trigger.py index bb198931d2..6efd4a59f9 100644 --- a/airlock_processor/tests/test_status_change_queue_trigger.py +++ b/airlock_processor/tests/test_status_change_queue_trigger.py @@ -74,7 +74,7 @@ class TestFileEnumeration(unittest.TestCase): def test_get_request_files_should_be_called_on_submit_stage(self, _, mock_get_request_files, mock_set_output_event_to_report_request_files): message_body = "{ \"data\": { \"request_id\":\"123\",\"status\":\"submitted\" , \"type\":\"import\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, outputEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock()) self.assertTrue(mock_get_request_files.called) self.assertTrue(mock_set_output_event_to_report_request_files.called) @@ -84,7 +84,7 @@ def test_get_request_files_should_be_called_on_submit_stage(self, _, mock_get_re def test_get_request_files_should_not_be_called_if_new_status_is_not_submit(self, _, mock_get_request_files, mock_set_output_event_to_report_failure): message_body = "{ \"data\": { \"request_id\":\"123\",\"status\":\"fake-status\" , \"type\":\"import\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, outputEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock()) self.assertFalse(mock_get_request_files.called) self.assertFalse(mock_set_output_event_to_report_failure.called) @@ -94,7 +94,7 @@ def test_get_request_files_should_not_be_called_if_new_status_is_not_submit(self def test_get_request_files_should_be_called_when_failing_during_submit_stage(self, _, mock_get_request_files, mock_set_output_event_to_report_failure): message_body = "{ \"data\": { \"request_id\":\"123\",\"status\":\"submitted\" , \"type\":\"import\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, outputEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock()) self.assertTrue(mock_get_request_files.called) self.assertTrue(mock_set_output_event_to_report_failure.called) From fb0266bf185271b92861f200cb2ff05de9498e1f Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 11 Sep 2022 15:38:41 +0000 Subject: [PATCH 04/16] fix output event names --- .../StatusChangedQueueTrigger/__init__.py | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/airlock_processor/StatusChangedQueueTrigger/__init__.py b/airlock_processor/StatusChangedQueueTrigger/__init__.py index cc81175446..042d944f68 100644 --- a/airlock_processor/StatusChangedQueueTrigger/__init__.py +++ b/airlock_processor/StatusChangedQueueTrigger/__init__.py @@ -30,21 +30,21 @@ def __init__(self, source_account_name: str, dest_account_name: str): self.dest_account_name = dest_account_name -def main(msg: func.ServiceBusMessage, step_result_event: func.Out[func.EventGridOutputEvent], to_delete_event: func.Out[func.EventGridOutputEvent]): +def main(msg: func.ServiceBusMessage, stepResultEvent: func.Out[func.EventGridOutputEvent], toDeleteEvent: func.Out[func.EventGridOutputEvent]): try: request_properties = extract_properties(msg) request_files = get_request_files(request_properties) if request_properties.new_status == constants.STAGE_SUBMITTED else None - handle_status_changed(request_properties, step_result_event, to_delete_event, request_files) + handle_status_changed(request_properties, stepResultEvent, toDeleteEvent, request_files) except NoFilesInRequestException: - set_output_event_to_report_failure(step_result_event, request_properties, failure_reason=constants.NO_FILES_IN_REQUEST_MESSAGE, request_files=request_files) + set_output_event_to_report_failure(stepResultEvent, request_properties, failure_reason=constants.NO_FILES_IN_REQUEST_MESSAGE, request_files=request_files) except TooManyFilesInRequestException: - set_output_event_to_report_failure(step_result_event, request_properties, failure_reason=constants.TOO_MANY_FILES_IN_REQUEST_MESSAGE, request_files=request_files) + set_output_event_to_report_failure(stepResultEvent, request_properties, failure_reason=constants.TOO_MANY_FILES_IN_REQUEST_MESSAGE, request_files=request_files) except Exception: - set_output_event_to_report_failure(step_result_event, request_properties, failure_reason=constants.UNKNOWN_REASON_MESSAGE, request_files=request_files) + set_output_event_to_report_failure(stepResultEvent, request_properties, failure_reason=constants.UNKNOWN_REASON_MESSAGE, request_files=request_files) -def handle_status_changed(request_properties: RequestProperties, step_result_event: func.Out[func.EventGridOutputEvent], to_delete_event: func.Out[func.EventGridOutputEvent], request_files): +def handle_status_changed(request_properties: RequestProperties, stepResultEvent: func.Out[func.EventGridOutputEvent], toDeleteEvent: func.Out[func.EventGridOutputEvent], request_files): new_status = request_properties.new_status previous_status = request_properties.previous_status req_id = request_properties.request_id @@ -72,11 +72,11 @@ def handle_status_changed(request_properties: RequestProperties, step_result_eve if new_status == constants.STAGE_CANCELLED: storage_account_name = get_storage_account(previous_status, request_type, ws_id) container_url = blob_operations.get_blob_url(account_name=storage_account_name, container_name=req_id) - set_output_event_to_trigger_blob_deletion(to_delete_event, request_properties, blob_url=container_url) + set_output_event_to_trigger_blob_deletion(toDeleteEvent, request_properties, blob_url=container_url) return if new_status == constants.STAGE_SUBMITTED: - set_output_event_to_report_request_files(step_result_event, request_properties, request_files) + set_output_event_to_report_request_files(stepResultEvent, request_properties, request_files) if (is_require_data_copy(new_status)): logging.info('Request with id %s. requires data copy between storage accounts', req_id) @@ -190,9 +190,9 @@ def get_storage_account_destination_for_copy(new_status: str, request_type: str, raise Exception(error_message) -def set_output_event_to_report_failure(step_result_event, request_properties, failure_reason, request_files): +def set_output_event_to_report_failure(stepResultEvent, request_properties, failure_reason, request_files): logging.exception(f"Failed processing Airlock request with ID: '{request_properties.request_id}', changing request status to '{constants.STAGE_FAILED}'.") - step_result_event.set( + stepResultEvent.set( func.EventGridOutputEvent( id=str(uuid.uuid4()), data={"completed_step": request_properties.new_status, "new_status": constants.STAGE_FAILED, "request_id": request_properties.request_id, "request_files": request_files, "error_message": failure_reason}, @@ -202,9 +202,9 @@ def set_output_event_to_report_failure(step_result_event, request_properties, fa data_version=constants.STEP_RESULT_EVENT_DATA_VERSION)) -def set_output_event_to_report_request_files(step_result_event, request_properties, request_files): +def set_output_event_to_report_request_files(stepResultEvent, request_properties, request_files): logging.info(f'Sending file enumeration result for request with ID: {request_properties.request_id} result: {request_files}') - step_result_event.set( + stepResultEvent.set( func.EventGridOutputEvent( id=str(uuid.uuid4()), data={"completed_step": request_properties.new_status, "request_id": request_properties.request_id, "request_files": request_files}, @@ -214,9 +214,9 @@ def set_output_event_to_report_request_files(step_result_event, request_properti data_version=constants.STEP_RESULT_EVENT_DATA_VERSION)) -def set_output_event_to_trigger_blob_deletion(to_delete_event, request_properties, blob_url): +def set_output_event_to_trigger_blob_deletion(toDeleteEvent, request_properties, blob_url): logging.info(f'Sending deletion event to delete container of request with ID: {request_properties.request_id}. container URL: {blob_url}') - to_delete_event.set( + toDeleteEvent.set( func.EventGridOutputEvent( id=str(uuid.uuid4()), data={"blob_to_delete": blob_url}, From e9c884e44d37e7e12ba0030c80d9ca1c024a0680 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 11 Sep 2022 15:57:11 +0000 Subject: [PATCH 05/16] update versions --- airlock_processor/_version.py | 2 +- api_app/_version.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/airlock_processor/_version.py b/airlock_processor/_version.py index 3dd3d2d51b..a34b2f6b04 100644 --- a/airlock_processor/_version.py +++ b/airlock_processor/_version.py @@ -1 +1 @@ -__version__ = "0.4.6" +__version__ = "0.4.7" diff --git a/api_app/_version.py b/api_app/_version.py index 7fe0489074..b6f65f35da 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.29" +__version__ = "0.4.30" From bfff96ed9207d1c2164b46f3dafd655281273b77 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 11 Sep 2022 19:31:05 +0300 Subject: [PATCH 06/16] clean code by extracting to methods --- .../StatusChangedQueueTrigger/__init__.py | 25 +++++++------------ 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/airlock_processor/StatusChangedQueueTrigger/__init__.py b/airlock_processor/StatusChangedQueueTrigger/__init__.py index 042d944f68..f299d89819 100644 --- a/airlock_processor/StatusChangedQueueTrigger/__init__.py +++ b/airlock_processor/StatusChangedQueueTrigger/__init__.py @@ -53,26 +53,13 @@ def handle_status_changed(request_properties: RequestProperties, stepResultEvent logging.info('Processing request with id %s. new status is "%s", type is "%s"', req_id, new_status, request_type) - try: - tre_id = os.environ["TRE_ID"] - except KeyError as e: - logging.error(f'Missing environment variable: {e}') - raise - - if new_status == constants.STAGE_DRAFT and request_type == constants.IMPORT_TYPE: - account_name = constants.STORAGE_ACCOUNT_NAME_IMPORT_EXTERNAL + tre_id - blob_operations.create_container(account_name, req_id) - return - - if new_status == constants.STAGE_DRAFT and request_type == constants.EXPORT_TYPE: - account_name = constants.STORAGE_ACCOUNT_NAME_EXPORT_INTERNAL + ws_id + if new_status == constants.STAGE_DRAFT: + account_name = get_storage_account(status=constants.STAGE_DRAFT, request_type=request_type, short_workspace_id=ws_id) blob_operations.create_container(account_name, req_id) return if new_status == constants.STAGE_CANCELLED: - storage_account_name = get_storage_account(previous_status, request_type, ws_id) - container_url = blob_operations.get_blob_url(account_name=storage_account_name, container_name=req_id) - set_output_event_to_trigger_blob_deletion(toDeleteEvent, request_properties, blob_url=container_url) + delete_request_files(request_properties, toDeleteEvent) return if new_status == constants.STAGE_SUBMITTED: @@ -233,6 +220,12 @@ def get_request_files(request_properties: RequestProperties): return blob_operations.get_request_files(account_name=storage_account_name, request_id=request_properties.request_id) +def delete_request_files(request_properties: RequestProperties, toDeleteEvent: func.Out[func.EventGridOutputEvent]): + storage_account_name = get_storage_account(request_properties.previous_status, request_properties.type, request_properties.workspace_id) + container_url = blob_operations.get_blob_url(account_name=storage_account_name, container_name=request_properties.request_id) + set_output_event_to_trigger_blob_deletion(toDeleteEvent, request_properties, blob_url=container_url) + + def _get_tre_id(): try: tre_id = os.environ["TRE_ID"] From 15bfa87ce5598ded8f561c5065f3e5bef619379f Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Mon, 12 Sep 2022 08:06:11 +0000 Subject: [PATCH 07/16] update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f8db140af..0b0492b253 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ ENHANCEMENTS: * Gitea shared service support app-service standard SKUs ([#2523](https://github.com/microsoft/AzureTRE/pull/2523)) * Keyvault diagnostic settings in base workspace ([#2521](https://github.com/microsoft/AzureTRE/pull/2521)) * Airlock requests contain a field with information about the files that were submitted ([#2504](https://github.com/microsoft/AzureTRE/pull/2504)) +* Cancelling an Airlock request triggers deletion of the request files ([#2584](https://github.com/microsoft/AzureTRE/pull/2584)) * UI - Operations and notifications stability improvements ([[#2530](https://github.com/microsoft/AzureTRE/pull/2530)) * UI - Initial implemetation of Workspace Airlock Request View ([#2512](https://github.com/microsoft/AzureTRE/pull/2512)) * Add `is_expsed_externally` option to Azure ML Workspace Service ([#2548](https://github.com/microsoft/AzureTRE/pull2548)) From fdbe3b41ff937b804a9735a22ebd3f43b9376bf6 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Mon, 12 Sep 2022 09:22:52 +0000 Subject: [PATCH 08/16] fix unit tests --- .../tests/test_status_change_queue_trigger.py | 21 ++++++++++--------- .../test_airlock_resource_helpers.py | 6 +++--- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/airlock_processor/tests/test_status_change_queue_trigger.py b/airlock_processor/tests/test_status_change_queue_trigger.py index 6efd4a59f9..d05dc2ecd7 100644 --- a/airlock_processor/tests/test_status_change_queue_trigger.py +++ b/airlock_processor/tests/test_status_change_queue_trigger.py @@ -12,12 +12,13 @@ class TestPropertiesExtraction(unittest.TestCase): def test_extract_prop_valid_body_return_all_values(self): - message_body = "{ \"data\": { \"request_id\":\"123\",\"status\":\"456\" , \"type\":\"789\", \"workspace_id\":\"ws1\" }}" + message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"456\" ,\"previous_status\":\"789\" , \"type\":\"101112\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) req_prop = extract_properties(message) self.assertEqual(req_prop.request_id, "123") - self.assertEqual(req_prop.status, "456") - self.assertEqual(req_prop.type, "789") + self.assertEqual(req_prop.new_status, "456") + self.assertEqual(req_prop.previous_status, "789") + self.assertEqual(req_prop.type, "101112") self.assertEqual(req_prop.workspace_id, "ws1") def test_extract_prop_missing_arg_throws(self): @@ -72,9 +73,9 @@ class TestFileEnumeration(unittest.TestCase): @patch("StatusChangedQueueTrigger.is_require_data_copy", return_value=False) @mock.patch.dict(os.environ, {"TRE_ID": "tre-id"}, clear=True) def test_get_request_files_should_be_called_on_submit_stage(self, _, mock_get_request_files, mock_set_output_event_to_report_request_files): - message_body = "{ \"data\": { \"request_id\":\"123\",\"status\":\"submitted\" , \"type\":\"import\", \"workspace_id\":\"ws1\" }}" + message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"submitted\" ,\"previous_status\":\"draft\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, stepResultEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock(), toDeleteEvent=MagicMock()) self.assertTrue(mock_get_request_files.called) self.assertTrue(mock_set_output_event_to_report_request_files.called) @@ -82,9 +83,9 @@ def test_get_request_files_should_be_called_on_submit_stage(self, _, mock_get_re @patch("StatusChangedQueueTrigger.get_request_files") @patch("StatusChangedQueueTrigger.handle_status_changed") def test_get_request_files_should_not_be_called_if_new_status_is_not_submit(self, _, mock_get_request_files, mock_set_output_event_to_report_failure): - message_body = "{ \"data\": { \"request_id\":\"123\",\"status\":\"fake-status\" , \"type\":\"import\", \"workspace_id\":\"ws1\" }}" + message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"fake-status\" ,\"previous_status\":\"None\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, stepResultEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock(), toDeleteEvent=MagicMock()) self.assertFalse(mock_get_request_files.called) self.assertFalse(mock_set_output_event_to_report_failure.called) @@ -92,9 +93,9 @@ def test_get_request_files_should_not_be_called_if_new_status_is_not_submit(self @patch("StatusChangedQueueTrigger.get_request_files") @patch("StatusChangedQueueTrigger.handle_status_changed", side_effect=Exception) def test_get_request_files_should_be_called_when_failing_during_submit_stage(self, _, mock_get_request_files, mock_set_output_event_to_report_failure): - message_body = "{ \"data\": { \"request_id\":\"123\",\"status\":\"submitted\" , \"type\":\"import\", \"workspace_id\":\"ws1\" }}" + message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"submitted\" ,\"previous_status\":\"draft\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, stepResultEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock(), toDeleteEvent=MagicMock()) self.assertTrue(mock_get_request_files.called) self.assertTrue(mock_set_output_event_to_report_failure.called) @@ -102,7 +103,7 @@ def test_get_request_files_should_be_called_when_failing_during_submit_stage(sel @mock.patch.dict(os.environ, {"TRE_ID": "tre-id"}, clear=True) def test_get_request_files_called_with_correct_storage_account(self, mock_get_request_files): source_storage_account_for_submitted_stage = constants.STORAGE_ACCOUNT_NAME_EXPORT_INTERNAL + 'ws1' - message_body = "{ \"data\": { \"request_id\":\"123\",\"status\":\"submitted\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" + message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"submitted\" ,\"previous_status\":\"draft\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) request_properties = extract_properties(message) get_request_files(request_properties) diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py index d4590edf38..ec69646c92 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py +++ b/api_app/tests_ma/test_api/test_routes/test_airlock_resource_helpers.py @@ -41,10 +41,10 @@ def sample_airlock_request(status=AirlockRequestStatus.Draft): return airlock_request -def sample_status_changed_event(status="draft"): +def sample_status_changed_event(new_status="draft", previous_status=None): status_changed_event = EventGridEvent( event_type="statusChanged", - data=StatusChangedData(request_id=AIRLOCK_REQUEST_ID, new_status=status, type=AirlockRequestType.Import, workspace_id=WORKSPACE_ID[-4:]).__dict__, + data=StatusChangedData(request_id=AIRLOCK_REQUEST_ID, new_status=new_status, previous_status=previous_status, type=AirlockRequestType.Import, workspace_id=WORKSPACE_ID[-4:]).__dict__, subject=f"{AIRLOCK_REQUEST_ID}/statusChanged", data_version="2.0" ) @@ -163,7 +163,7 @@ async def test_update_and_publish_event_airlock_request_updates_item(_, event_gr airlock_request_repo_mock): airlock_request_mock = sample_airlock_request() updated_airlock_request_mock = sample_airlock_request(status=AirlockRequestStatus.Submitted) - status_changed_event_mock = sample_status_changed_event(status="submitted") + status_changed_event_mock = sample_status_changed_event(new_status="submitted", previous_status="draft") airlock_notification_event_mock = sample_airlock_notification_event(status="submitted") airlock_request_repo_mock.update_airlock_request = MagicMock(return_value=updated_airlock_request_mock) event_grid_sender_client_mock = event_grid_publisher_client_mock.return_value From 881dd42e6f24dc7f80112defd853c307fa163f46 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Mon, 12 Sep 2022 12:29:40 +0000 Subject: [PATCH 09/16] add unit tests --- .../StatusChangedQueueTrigger/__init__.py | 16 ++++++---------- .../tests/shared_code/test_blob_operations.py | 17 ++++++++++++++++- .../tests/test_status_change_queue_trigger.py | 10 ++++++++++ .../tests/test_to_delete_trigger.py | 6 ++++++ 4 files changed, 38 insertions(+), 11 deletions(-) diff --git a/airlock_processor/StatusChangedQueueTrigger/__init__.py b/airlock_processor/StatusChangedQueueTrigger/__init__.py index f299d89819..5dc5b49a86 100644 --- a/airlock_processor/StatusChangedQueueTrigger/__init__.py +++ b/airlock_processor/StatusChangedQueueTrigger/__init__.py @@ -59,7 +59,9 @@ def handle_status_changed(request_properties: RequestProperties, stepResultEvent return if new_status == constants.STAGE_CANCELLED: - delete_request_files(request_properties, toDeleteEvent) + storage_account_name = get_storage_account(request_properties.previous_status, request_properties.type, request_properties.workspace_id) + container_to_delete_url = blob_operations.get_blob_url(account_name=storage_account_name, container_name=request_properties.request_id) + set_output_event_to_trigger_container_deletion(toDeleteEvent, request_properties, container_url=container_to_delete_url) return if new_status == constants.STAGE_SUBMITTED: @@ -201,12 +203,12 @@ def set_output_event_to_report_request_files(stepResultEvent, request_properties data_version=constants.STEP_RESULT_EVENT_DATA_VERSION)) -def set_output_event_to_trigger_blob_deletion(toDeleteEvent, request_properties, blob_url): - logging.info(f'Sending deletion event to delete container of request with ID: {request_properties.request_id}. container URL: {blob_url}') +def set_output_event_to_trigger_container_deletion(toDeleteEvent, request_properties, container_url): + logging.info(f'Sending deletion event to delete container of request with ID: {request_properties.request_id}. container URL: {container_url}') toDeleteEvent.set( func.EventGridOutputEvent( id=str(uuid.uuid4()), - data={"blob_to_delete": blob_url}, + data={"blob_to_delete": container_url}, subject=request_properties.request_id, event_type="Airlock.ToDelete", event_time=datetime.datetime.utcnow(), @@ -220,12 +222,6 @@ def get_request_files(request_properties: RequestProperties): return blob_operations.get_request_files(account_name=storage_account_name, request_id=request_properties.request_id) -def delete_request_files(request_properties: RequestProperties, toDeleteEvent: func.Out[func.EventGridOutputEvent]): - storage_account_name = get_storage_account(request_properties.previous_status, request_properties.type, request_properties.workspace_id) - container_url = blob_operations.get_blob_url(account_name=storage_account_name, container_name=request_properties.request_id) - set_output_event_to_trigger_blob_deletion(toDeleteEvent, request_properties, blob_url=container_url) - - def _get_tre_id(): try: tre_id = os.environ["TRE_ID"] diff --git a/airlock_processor/tests/shared_code/test_blob_operations.py b/airlock_processor/tests/shared_code/test_blob_operations.py index 59ce7e481a..e32f805973 100644 --- a/airlock_processor/tests/shared_code/test_blob_operations.py +++ b/airlock_processor/tests/shared_code/test_blob_operations.py @@ -3,7 +3,7 @@ from unittest import TestCase from unittest.mock import MagicMock, patch -from shared_code.blob_operations import get_blob_info_from_topic_and_subject, get_blob_info_from_blob_url, copy_data +from shared_code.blob_operations import get_blob_info_from_topic_and_subject, get_blob_info_from_blob_url, copy_data, get_blob_url from exceptions import TooManyFilesInRequestException, NoFilesInRequestException @@ -75,3 +75,18 @@ def test_copy_data_adds_copied_from_metadata(self, _, mock_blob_service_client): # Check that copied_from field was set correctly in the metadata dest_blob_client_mock.start_copy_from_url.assert_called_with(f"{source_url}?sas", metadata=dest_metadata) + + def test_get_blob_url_should_return_blob_url(self): + account_name = "account" + container_name = "container" + blob_name = "blob" + + blob_url = get_blob_url(account_name, container_name, blob_name) + self.assertEqual(blob_url, f"https://{account_name}.blob.core.windows.net/{container_name}/{blob_name}") + + def test_get_blob_url_without_blob_name_should_return_container_url(self): + account_name = "account" + container_name = "container" + + blob_url = get_blob_url(account_name, container_name) + self.assertEqual(blob_url, f"https://{account_name}.blob.core.windows.net/{container_name}/") diff --git a/airlock_processor/tests/test_status_change_queue_trigger.py b/airlock_processor/tests/test_status_change_queue_trigger.py index d05dc2ecd7..a5a73c0b2d 100644 --- a/airlock_processor/tests/test_status_change_queue_trigger.py +++ b/airlock_processor/tests/test_status_change_queue_trigger.py @@ -110,6 +110,16 @@ def test_get_request_files_called_with_correct_storage_account(self, mock_get_re mock_get_request_files.assert_called_with(account_name=source_storage_account_for_submitted_stage, request_id=request_properties.request_id) +class TestFilesDeletion(unittest.TestCase): + @patch("StatusChangedQueueTrigger.set_output_event_to_trigger_container_deletion") + @mock.patch.dict(os.environ, {"TRE_ID": "tre-id"}, clear=True) + def test_delete_request_files_should_be_called_on_cancel_stage(self, mock_set_output_event_to_trigger_container_deletion): + message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"cancelled\" ,\"previous_status\":\"draft\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" + message = _mock_service_bus_message(body=message_body) + main(msg=message, stepResultEvent=MagicMock(), toDeleteEvent=MagicMock()) + self.assertTrue(mock_set_output_event_to_trigger_container_deletion.called) + + def _mock_service_bus_message(body: str): encoded_body = str.encode(body, "utf-8") message = ServiceBusMessage(body=encoded_body, message_id="123", user_properties={}) diff --git a/airlock_processor/tests/test_to_delete_trigger.py b/airlock_processor/tests/test_to_delete_trigger.py index bbe19b3e4f..4a812b952f 100644 --- a/airlock_processor/tests/test_to_delete_trigger.py +++ b/airlock_processor/tests/test_to_delete_trigger.py @@ -24,3 +24,9 @@ def test_delete_blob_and_container_if_last_blob_doesnt_delete_container(self, mo delete_blob_and_container_if_last_blob(blob_url) mock_blob_service_client().get_container_client().delete_container.assert_not_called() + + @patch("ToDeleteTrigger.BlobServiceClient") + def test_delete_blob_and_container_if_last_blob_deletes_container_if_no_blob_specified(self, mock_blob_service_client): + blob_url = "https://stalimextest.blob.core.windows.net/c144728c-3c69-4a58-afec-48c2ec8bfd45/" + delete_blob_and_container_if_last_blob(blob_url) + mock_blob_service_client().get_container_client().delete_container.assert_called_once() From b527e13af1a7886ac3b1b3c8db5e67e54e0920b8 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Mon, 12 Sep 2022 12:58:51 +0000 Subject: [PATCH 10/16] use already declared variables instead of request_properties --- airlock_processor/StatusChangedQueueTrigger/__init__.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/airlock_processor/StatusChangedQueueTrigger/__init__.py b/airlock_processor/StatusChangedQueueTrigger/__init__.py index 5dc5b49a86..cfd4616a3d 100644 --- a/airlock_processor/StatusChangedQueueTrigger/__init__.py +++ b/airlock_processor/StatusChangedQueueTrigger/__init__.py @@ -59,8 +59,8 @@ def handle_status_changed(request_properties: RequestProperties, stepResultEvent return if new_status == constants.STAGE_CANCELLED: - storage_account_name = get_storage_account(request_properties.previous_status, request_properties.type, request_properties.workspace_id) - container_to_delete_url = blob_operations.get_blob_url(account_name=storage_account_name, container_name=request_properties.request_id) + storage_account_name = get_storage_account(previous_status, request_type, ws_id) + container_to_delete_url = blob_operations.get_blob_url(account_name=storage_account_name, container_name=req_id) set_output_event_to_trigger_container_deletion(toDeleteEvent, request_properties, container_url=container_to_delete_url) return From 5e55108f1b984e9e7f7fb390bf042e04bbf2003d Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Mon, 12 Sep 2022 16:27:05 +0300 Subject: [PATCH 11/16] update api version --- api_app/_version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api_app/_version.py b/api_app/_version.py index b6f65f35da..e2b01a98c0 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.30" +__version__ = "0.4.31" From 0a1daa98eb38c9c113f20b96a2da061b52c7e857 Mon Sep 17 00:00:00 2001 From: Yuval Yaron <43217306+yuvalyaron@users.noreply.github.com> Date: Tue, 13 Sep 2022 10:42:31 +0300 Subject: [PATCH 12/16] update changelog Co-authored-by: Elad Iwanir <13205761+eladiw@users.noreply.github.com> --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0b0492b253..b03d318b0e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,7 +18,7 @@ ENHANCEMENTS: * Gitea shared service support app-service standard SKUs ([#2523](https://github.com/microsoft/AzureTRE/pull/2523)) * Keyvault diagnostic settings in base workspace ([#2521](https://github.com/microsoft/AzureTRE/pull/2521)) * Airlock requests contain a field with information about the files that were submitted ([#2504](https://github.com/microsoft/AzureTRE/pull/2504)) -* Cancelling an Airlock request triggers deletion of the request files ([#2584](https://github.com/microsoft/AzureTRE/pull/2584)) +* Cancelling an Airlock request triggers deletion of the request container and files ([#2584](https://github.com/microsoft/AzureTRE/pull/2584)) * UI - Operations and notifications stability improvements ([[#2530](https://github.com/microsoft/AzureTRE/pull/2530)) * UI - Initial implemetation of Workspace Airlock Request View ([#2512](https://github.com/microsoft/AzureTRE/pull/2512)) * Add `is_expsed_externally` option to Azure ML Workspace Service ([#2548](https://github.com/microsoft/AzureTRE/pull2548)) From ceebec4c17d64bdbbef8f1f4a78760494469d105 Mon Sep 17 00:00:00 2001 From: Yuval Yaron <43217306+yuvalyaron@users.noreply.github.com> Date: Tue, 13 Sep 2022 10:43:16 +0300 Subject: [PATCH 13/16] update log message Co-authored-by: Elad Iwanir <13205761+eladiw@users.noreply.github.com> --- airlock_processor/StatusChangedQueueTrigger/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/airlock_processor/StatusChangedQueueTrigger/__init__.py b/airlock_processor/StatusChangedQueueTrigger/__init__.py index cfd4616a3d..fad2464868 100644 --- a/airlock_processor/StatusChangedQueueTrigger/__init__.py +++ b/airlock_processor/StatusChangedQueueTrigger/__init__.py @@ -204,7 +204,7 @@ def set_output_event_to_report_request_files(stepResultEvent, request_properties def set_output_event_to_trigger_container_deletion(toDeleteEvent, request_properties, container_url): - logging.info(f'Sending deletion event to delete container of request with ID: {request_properties.request_id}. container URL: {container_url}') + logging.info(f'Sending container deletion event for request ID: {request_properties.request_id}. container URL: {container_url}') toDeleteEvent.set( func.EventGridOutputEvent( id=str(uuid.uuid4()), From 24f6936ee0e74334fb706febb824d83f4e926479 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Tue, 13 Sep 2022 11:39:50 +0000 Subject: [PATCH 14/16] rename references of toDelete event to dataDeletion event in statusChanged function --- airlock_processor/BlobCreatedTrigger/__init__.py | 4 ++-- .../StatusChangedQueueTrigger/__init__.py | 14 +++++++------- .../StatusChangedQueueTrigger/function.json | 2 +- airlock_processor/shared_code/constants.py | 2 +- .../tests/test_status_change_queue_trigger.py | 8 ++++---- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/airlock_processor/BlobCreatedTrigger/__init__.py b/airlock_processor/BlobCreatedTrigger/__init__.py index 24759da58d..680488aaa0 100644 --- a/airlock_processor/BlobCreatedTrigger/__init__.py +++ b/airlock_processor/BlobCreatedTrigger/__init__.py @@ -79,8 +79,8 @@ def main(msg: func.ServiceBusMessage, id=str(uuid.uuid4()), data={"blob_to_delete": copied_from[-1]}, # last container in copied_from is the one we just copied from subject=request_id, - event_type="Airlock.ToDelete", + event_type="Airlock.DataDeletion", event_time=datetime.datetime.utcnow(), - data_version=constants.TO_DELETE_EVENT_DATA_VERSION + data_version=constants.DATA_DELETION_EVENT_DATA_VERSION ) ) diff --git a/airlock_processor/StatusChangedQueueTrigger/__init__.py b/airlock_processor/StatusChangedQueueTrigger/__init__.py index fad2464868..80383a5dd7 100644 --- a/airlock_processor/StatusChangedQueueTrigger/__init__.py +++ b/airlock_processor/StatusChangedQueueTrigger/__init__.py @@ -30,11 +30,11 @@ def __init__(self, source_account_name: str, dest_account_name: str): self.dest_account_name = dest_account_name -def main(msg: func.ServiceBusMessage, stepResultEvent: func.Out[func.EventGridOutputEvent], toDeleteEvent: func.Out[func.EventGridOutputEvent]): +def main(msg: func.ServiceBusMessage, stepResultEvent: func.Out[func.EventGridOutputEvent], dataDeletionEvent: func.Out[func.EventGridOutputEvent]): try: request_properties = extract_properties(msg) request_files = get_request_files(request_properties) if request_properties.new_status == constants.STAGE_SUBMITTED else None - handle_status_changed(request_properties, stepResultEvent, toDeleteEvent, request_files) + handle_status_changed(request_properties, stepResultEvent, dataDeletionEvent, request_files) except NoFilesInRequestException: set_output_event_to_report_failure(stepResultEvent, request_properties, failure_reason=constants.NO_FILES_IN_REQUEST_MESSAGE, request_files=request_files) @@ -44,7 +44,7 @@ def main(msg: func.ServiceBusMessage, stepResultEvent: func.Out[func.EventGridOu set_output_event_to_report_failure(stepResultEvent, request_properties, failure_reason=constants.UNKNOWN_REASON_MESSAGE, request_files=request_files) -def handle_status_changed(request_properties: RequestProperties, stepResultEvent: func.Out[func.EventGridOutputEvent], toDeleteEvent: func.Out[func.EventGridOutputEvent], request_files): +def handle_status_changed(request_properties: RequestProperties, stepResultEvent: func.Out[func.EventGridOutputEvent], dataDeletionEvent: func.Out[func.EventGridOutputEvent], request_files): new_status = request_properties.new_status previous_status = request_properties.previous_status req_id = request_properties.request_id @@ -61,7 +61,7 @@ def handle_status_changed(request_properties: RequestProperties, stepResultEvent if new_status == constants.STAGE_CANCELLED: storage_account_name = get_storage_account(previous_status, request_type, ws_id) container_to_delete_url = blob_operations.get_blob_url(account_name=storage_account_name, container_name=req_id) - set_output_event_to_trigger_container_deletion(toDeleteEvent, request_properties, container_url=container_to_delete_url) + set_output_event_to_trigger_container_deletion(dataDeletionEvent, request_properties, container_url=container_to_delete_url) return if new_status == constants.STAGE_SUBMITTED: @@ -203,16 +203,16 @@ def set_output_event_to_report_request_files(stepResultEvent, request_properties data_version=constants.STEP_RESULT_EVENT_DATA_VERSION)) -def set_output_event_to_trigger_container_deletion(toDeleteEvent, request_properties, container_url): +def set_output_event_to_trigger_container_deletion(dataDeletionEvent, request_properties, container_url): logging.info(f'Sending container deletion event for request ID: {request_properties.request_id}. container URL: {container_url}') - toDeleteEvent.set( + dataDeletionEvent.set( func.EventGridOutputEvent( id=str(uuid.uuid4()), data={"blob_to_delete": container_url}, subject=request_properties.request_id, event_type="Airlock.ToDelete", event_time=datetime.datetime.utcnow(), - data_version=constants.TO_DELETE_EVENT_DATA_VERSION + data_version=constants.DATA_DELETION_EVENT_DATA_VERSION ) ) diff --git a/airlock_processor/StatusChangedQueueTrigger/function.json b/airlock_processor/StatusChangedQueueTrigger/function.json index ff8656643d..977644ef6f 100644 --- a/airlock_processor/StatusChangedQueueTrigger/function.json +++ b/airlock_processor/StatusChangedQueueTrigger/function.json @@ -17,7 +17,7 @@ }, { "type": "eventGrid", - "name": "toDeleteEvent", + "name": "dataDeletionEvent", "topicEndpointUri": "EVENT_GRID_TO_DELETE_TOPIC_URI_SETTING", "topicKeySetting": "EVENT_GRID_TO_DELETE_TOPIC_KEY_SETTING", "direction": "out" diff --git a/airlock_processor/shared_code/constants.py b/airlock_processor/shared_code/constants.py index bab0247cc8..b1c53cbf9a 100644 --- a/airlock_processor/shared_code/constants.py +++ b/airlock_processor/shared_code/constants.py @@ -38,6 +38,6 @@ # Event Grid STEP_RESULT_EVENT_DATA_VERSION = "1.0" -TO_DELETE_EVENT_DATA_VERSION = "1.0" +DATA_DELETION_EVENT_DATA_VERSION = "1.0" NO_THREATS = "No threats found" diff --git a/airlock_processor/tests/test_status_change_queue_trigger.py b/airlock_processor/tests/test_status_change_queue_trigger.py index a5a73c0b2d..221acb7391 100644 --- a/airlock_processor/tests/test_status_change_queue_trigger.py +++ b/airlock_processor/tests/test_status_change_queue_trigger.py @@ -75,7 +75,7 @@ class TestFileEnumeration(unittest.TestCase): def test_get_request_files_should_be_called_on_submit_stage(self, _, mock_get_request_files, mock_set_output_event_to_report_request_files): message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"submitted\" ,\"previous_status\":\"draft\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, stepResultEvent=MagicMock(), toDeleteEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock(), dataDeletionEvent=MagicMock()) self.assertTrue(mock_get_request_files.called) self.assertTrue(mock_set_output_event_to_report_request_files.called) @@ -85,7 +85,7 @@ def test_get_request_files_should_be_called_on_submit_stage(self, _, mock_get_re def test_get_request_files_should_not_be_called_if_new_status_is_not_submit(self, _, mock_get_request_files, mock_set_output_event_to_report_failure): message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"fake-status\" ,\"previous_status\":\"None\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, stepResultEvent=MagicMock(), toDeleteEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock(), dataDeletionEvent=MagicMock()) self.assertFalse(mock_get_request_files.called) self.assertFalse(mock_set_output_event_to_report_failure.called) @@ -95,7 +95,7 @@ def test_get_request_files_should_not_be_called_if_new_status_is_not_submit(self def test_get_request_files_should_be_called_when_failing_during_submit_stage(self, _, mock_get_request_files, mock_set_output_event_to_report_failure): message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"submitted\" ,\"previous_status\":\"draft\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, stepResultEvent=MagicMock(), toDeleteEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock(), dataDeletionEvent=MagicMock()) self.assertTrue(mock_get_request_files.called) self.assertTrue(mock_set_output_event_to_report_failure.called) @@ -116,7 +116,7 @@ class TestFilesDeletion(unittest.TestCase): def test_delete_request_files_should_be_called_on_cancel_stage(self, mock_set_output_event_to_trigger_container_deletion): message_body = "{ \"data\": { \"request_id\":\"123\",\"new_status\":\"cancelled\" ,\"previous_status\":\"draft\" , \"type\":\"export\", \"workspace_id\":\"ws1\" }}" message = _mock_service_bus_message(body=message_body) - main(msg=message, stepResultEvent=MagicMock(), toDeleteEvent=MagicMock()) + main(msg=message, stepResultEvent=MagicMock(), dataDeletionEvent=MagicMock()) self.assertTrue(mock_set_output_event_to_trigger_container_deletion.called) From 4528e60a4d3f09a6692e386a48a1c4e8bbde5a82 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Tue, 13 Sep 2022 11:45:08 +0000 Subject: [PATCH 15/16] change toDelete to DataDeletion --- airlock_processor/StatusChangedQueueTrigger/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/airlock_processor/StatusChangedQueueTrigger/__init__.py b/airlock_processor/StatusChangedQueueTrigger/__init__.py index 80383a5dd7..f815ef2f97 100644 --- a/airlock_processor/StatusChangedQueueTrigger/__init__.py +++ b/airlock_processor/StatusChangedQueueTrigger/__init__.py @@ -210,7 +210,7 @@ def set_output_event_to_trigger_container_deletion(dataDeletionEvent, request_pr id=str(uuid.uuid4()), data={"blob_to_delete": container_url}, subject=request_properties.request_id, - event_type="Airlock.ToDelete", + event_type="Airlock.DataDeletion", event_time=datetime.datetime.utcnow(), data_version=constants.DATA_DELETION_EVENT_DATA_VERSION ) From a44e3bd395fc20dd8b66acb464a9de03be241528 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Tue, 13 Sep 2022 14:50:13 +0300 Subject: [PATCH 16/16] update version and changelog --- CHANGELOG.md | 3 +-- api_app/_version.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b7078b6a7..c6dba6791e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ FEATURES: ENHANCEMENTS: -* +* Cancelling an Airlock request triggers deletion of the request container and files ([#2584](https://github.com/microsoft/AzureTRE/pull/2584)) BUG FIXES: @@ -36,7 +36,6 @@ ENHANCEMENTS: * Gitea shared service support app-service standard SKUs ([#2523](https://github.com/microsoft/AzureTRE/pull/2523)) * Keyvault diagnostic settings in base workspace ([#2521](https://github.com/microsoft/AzureTRE/pull/2521)) * Airlock requests contain a field with information about the files that were submitted ([#2504](https://github.com/microsoft/AzureTRE/pull/2504)) -* Cancelling an Airlock request triggers deletion of the request container and files ([#2584](https://github.com/microsoft/AzureTRE/pull/2584)) * UI - Operations and notifications stability improvements ([[#2530](https://github.com/microsoft/AzureTRE/pull/2530)) * UI - Initial implemetation of Workspace Airlock Request View ([#2512](https://github.com/microsoft/AzureTRE/pull/2512)) * Add `is_expsed_externally` option to Azure ML Workspace Service ([#2548](https://github.com/microsoft/AzureTRE/pull2548)) diff --git a/api_app/_version.py b/api_app/_version.py index e2b01a98c0..905119c354 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.4.31" +__version__ = "0.4.32"