Clean up review VMs for cancelled Airlock requests

CHANGELOG.md

@@ -30,6 +30,7 @@ ENHANCEMENTS:
 * Support for _Azure Firewall Basic_ SKU [#3107](https://github.com/microsoft/AzureTRE/pull/3107). This SKU doesn't support deallocation and for most non 24/7 scenarios will be more expensive than the Standard SKU.
 * Update Azure Machine Learning Workspace Service to support "no public IP" compute. This is a full rework so upgrades of existing Azure ML Workspace Service deployments are not supported. Requires `v0.8.0` or later of the TRE project.  [#3052](https://github.com/microsoft/AzureTRE/pull/3052)
 * Move non-core DNS zones out of the network module to reduce dependencies [#3119](https://github.com/microsoft/AzureTRE/pull/3119)
+* Review VMs are being cleaned up when an Airlock request is canceled ([#3130](https://github.com/microsoft/AzureTRE/pull/3130))
 
 BUG FIXES:
 * Reauth CLI if TRE endpoint has changed [#3137](https://github.com/microsoft/AzureTRE/pull/3137)

api_app/_version.py

@@ -1 +1 @@
-__version__ = "0.10.0"
+__version__ = "0.11.0"

api_app/api/routes/airlock.py

@@ -26,7 +26,7 @@
 from .resource_helpers import construct_location_header
 
 from services.airlock import create_review_vm, review_airlock_request, get_airlock_container_link, get_allowed_actions, save_and_publish_event_airlock_request, update_and_publish_event_airlock_request, \
-    enrich_requests_with_allowed_actions, get_airlock_requests_by_user_and_workspace
+    enrich_requests_with_allowed_actions, get_airlock_requests_by_user_and_workspace, cancel_request
 
 airlock_workspace_router = APIRouter(dependencies=[Depends(get_current_workspace_owner_or_researcher_user_or_airlock_manager)])
 
@@ -101,10 +101,14 @@ async def create_submit_request(airlock_request=Depends(get_airlock_request_by_i
                                dependencies=[Depends(get_current_workspace_owner_or_researcher_user), Depends(get_workspace_by_id_from_path)])
 async def create_cancel_request(airlock_request=Depends(get_airlock_request_by_id_from_path),
                                 user=Depends(get_current_workspace_owner_or_researcher_user),
+                                workspace=Depends(get_workspace_by_id_from_path),
                                 airlock_request_repo=Depends(get_repository(AirlockRequestRepository)),
-                                workspace=Depends(get_workspace_by_id_from_path)) -> AirlockRequestWithAllowedUserActions:
-    updated_request = await update_and_publish_event_airlock_request(airlock_request, airlock_request_repo, user, workspace,
-                                                                     new_status=AirlockRequestStatus.Cancelled)
+                                user_resource_repo=Depends(get_repository(UserResourceRepository)),
+                                workspace_service_repo=Depends(get_repository(WorkspaceServiceRepository)),
+                                resource_history_repo=Depends(get_repository(ResourceHistoryRepository)),
+                                operation_repo=Depends(get_repository(OperationRepository)),
+                                resource_template_repo=Depends(get_repository(ResourceTemplateRepository)),) -> AirlockRequestWithAllowedUserActions:
+    updated_request = await cancel_request(airlock_request, user, workspace, airlock_request_repo, user_resource_repo, workspace_service_repo, resource_template_repo, operation_repo, resource_history_repo)
     allowed_actions = get_allowed_actions(updated_request, user, airlock_request_repo)
     return AirlockRequestWithAllowedUserActions(airlockRequest=updated_request, allowedUserActions=allowed_actions)
 

api_app/api/routes/resource_helpers.py

@@ -4,6 +4,10 @@
 from typing import Dict, Any, Optional
 
 from fastapi import HTTPException, status
+from db.repositories.user_resources import UserResourceRepository
+from models.domain.user_resource import UserResource
+from models.domain.workspace_service import WorkspaceService
+from models.schemas.resource import ResourcePatch
 from db.repositories.resources import ResourceRepository
 from db.repositories.resources_history import ResourceHistoryRepository
 from models.domain.resource_template import ResourceTemplate
@@ -256,3 +260,29 @@ async def get_template(
 
 def get_timestamp() -> float:
     return datetime.utcnow().timestamp()
+
+
+async def update_user_resource(
+        user_resource: UserResource,
+        resource_patch: ResourcePatch,
+        force_version_update: bool,
+        user: User,
+        etag: str,
+        workspace_service: WorkspaceService,
+        user_resource_repo: UserResourceRepository,
+        resource_template_repo: ResourceTemplateRepository,
+        operations_repo: OperationRepository,
+        resource_history_repo: ResourceHistoryRepository) -> Operation:
+
+    patched_user_resource, resource_template = await user_resource_repo.patch_user_resource(user_resource, resource_patch, etag, resource_template_repo, resource_history_repo, workspace_service.templateName, user, force_version_update)
+    operation = await send_resource_request_message(
+        resource=patched_user_resource,
+        operations_repo=operations_repo,
+        resource_repo=user_resource_repo,
+        user=user,
+        resource_template=resource_template,
+        resource_template_repo=resource_template_repo,
+        resource_history_repo=resource_history_repo,
+        action=RequestAction.Upgrade)
+
+    return operation

api_app/api/routes/workspaces.py

@@ -34,7 +34,7 @@
 from services.azure_resource_status import get_azure_resource_status
 from azure.cosmos.exceptions import CosmosAccessConditionFailedError
 from .resource_helpers import get_identity_role_assignments, save_and_deploy_resource, construct_location_header, send_uninstall_message, \
-    send_custom_action_message, send_resource_request_message
+    send_custom_action_message, send_resource_request_message, update_user_resource
 from models.domain.request_action import RequestAction
 
 workspaces_core_router = APIRouter(dependencies=[Depends(get_current_tre_user_or_tre_admin)])
@@ -466,17 +466,7 @@ async def patch_user_resource(
     validate_user_has_valid_role_for_user_resource(user, user_resource)
 
     try:
-        patched_user_resource, resource_template = await user_resource_repo.patch_user_resource(user_resource, user_resource_patch, etag, resource_template_repo, resource_history_repo, workspace_service.templateName, user, force_version_update)
-        operation = await send_resource_request_message(
-            resource=patched_user_resource,
-            operations_repo=operations_repo,
-            resource_repo=user_resource_repo,
-            user=user,
-            resource_template=resource_template,
-            resource_template_repo=resource_template_repo,
-            resource_history_repo=resource_history_repo,
-            action=RequestAction.Upgrade)
-
+        operation = await update_user_resource(user_resource, user_resource_patch, force_version_update, user, etag, workspace_service, user_resource_repo, resource_template_repo, operations_repo, resource_history_repo)
         response.headers["Location"] = construct_location_header(operation)
         return OperationInResponse(operation=operation)
     except CosmosAccessConditionFailedError:

api_app/services/airlock.py

@@ -10,16 +10,18 @@
 from models.domain.user_resource import UserResource
 from models.domain.operation import Operation
 from models.domain.resource import ResourceType
+from models.domain.workspace_service import WorkspaceService
 from models.schemas.airlock_request import AirlockReviewInCreate
 from models.schemas.airlock_request import AirlockRequestWithAllowedUserActions
+from models.schemas.resource import ResourcePatch
 from typing import Tuple, List, Optional
 from models.schemas.user_resource import UserResourceInCreate
 from services.azure_resource_status import get_azure_resource_status
 from services.authentication import get_access_service
 
 from resources import strings, constants
 
-from api.routes.resource_helpers import save_and_deploy_resource, send_uninstall_message
+from api.routes.resource_helpers import save_and_deploy_resource, send_uninstall_message, update_user_resource
 
 from db.repositories.user_resources import UserResourceRepository
 from db.repositories.workspace_services import WorkspaceServiceRepository
@@ -381,6 +383,9 @@ async def delete_review_user_resource(
     workspace_service = await workspace_service_repo.get_workspace_service_by_id(workspace_id=user_resource.workspaceId,
                                                                                  service_id=user_resource.parentWorkspaceServiceId)
 
+    # disable might contain logic that we need to execute before the deletion of the resource
+    _ = await disable_user_resource(user_resource, user, workspace_service, user_resource_repo, resource_template_repo, operations_repo, resource_history_repo)
+
     resource_template = await resource_template_repo.get_template_by_name_and_version(
         user_resource.templateName,
         user_resource.templateVersion,
@@ -401,6 +406,23 @@ async def delete_review_user_resource(
     return operation
 
 
+async def disable_user_resource(
+        user_resource: UserResource,
+        user: User,
+        workspace_service: WorkspaceService,
+        user_resource_repo: UserResourceRepository,
+        resource_template_repo: ResourceTemplateRepository,
+        operations_repo: OperationRepository,
+        resource_history_repo: ResourceHistoryRepository) -> Operation:
+
+    resource_patch = ResourcePatch(isEnabled=False)
+    operation = await update_user_resource(user_resource=user_resource, resource_patch=resource_patch, force_version_update=False,
+                                           user=user, etag=user_resource.etag, workspace_service=workspace_service, user_resource_repo=user_resource_repo,
+                                           resource_template_repo=resource_template_repo, operations_repo=operations_repo, resource_history_repo=resource_history_repo)
+
+    return operation
+
+
 async def delete_all_review_user_resources(
         airlock_request: AirlockRequest,
         user_resource_repo: UserResourceRepository,
@@ -430,3 +452,11 @@ async def delete_all_review_user_resources(
 
     logging.info(f"Started {len(operations)} operations on deleting user resources")
     return operations
+
+
+async def cancel_request(airlock_request: AirlockRequest, user: User, workspace: Workspace,
+                         airlock_request_repo: AirlockRequestRepository, user_resource_repo: UserResourceRepository, workspace_service_repo: WorkspaceServiceRepository,
+                         resource_template_repo: ResourceTemplateRepository, operations_repo: OperationRepository, resource_history_repo: ResourceHistoryRepository) -> AirlockRequest:
+    updated_request = await update_and_publish_event_airlock_request(airlock_request=airlock_request, airlock_request_repo=airlock_request_repo, updated_by=user, workspace=workspace, new_status=AirlockRequestStatus.Cancelled)
+    await delete_all_review_user_resources(airlock_request, user_resource_repo, workspace_service_repo, resource_template_repo, operations_repo, resource_history_repo, user)
+    return updated_request

api_app/tests_ma/test_api/test_routes/test_airlock.py

@@ -232,8 +232,8 @@ async def test_post_submit_airlock_request_with_illegal_status_change_returns_40
 
     # [POST] /workspaces/{workspace_id}/requests/{airlock_request_id}/cancel
     @patch("api.routes.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object())
-    @patch("api.routes.airlock.update_and_publish_event_airlock_request", return_value=sample_airlock_request_object(status=AirlockRequestStatus.Cancelled))
-    async def test_post_cancel_airlock_request_canceles_request_returns_200(self, _, __, app, client):
+    @patch("services.airlock.update_and_publish_event_airlock_request", return_value=sample_airlock_request_object(status=AirlockRequestStatus.Cancelled))
+    async def test_post_cancel_airlock_request_cancels_request_returns_200(self, _, __, app, client):
         response = await client.post(app.url_path_for(strings.API_CANCEL_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID))
         assert response.status_code == status.HTTP_200_OK
         assert response.json()["airlockRequest"]["id"] == AIRLOCK_REQUEST_ID
@@ -332,14 +332,17 @@ async def test_post_create_airlock_review_with_illegal_status_change_returns_400
         assert response.status_code == status.HTTP_400_BAD_REQUEST
 
     @patch("services.airlock.send_uninstall_message")
+    @patch("services.airlock.ResourceHistoryRepository.save_item")
+    @patch("services.airlock.UserResourceRepository.update_item_with_etag")
+    @patch("services.airlock.update_user_resource")
     @patch("services.airlock.ResourceTemplateRepository.get_template_by_name_and_version", return_value=ResourceTemplate(name="test_template", id="123", description="test", version="0.0.1", resourceType="user-resource", current=True, required=[], properties={}))
     @patch("services.airlock.WorkspaceServiceRepository.get_workspace_service_by_id", return_value=WorkspaceService(id=WORKSPACE_SERVICE_ID, templateName="test", templateVersion="0.0.1", _etag="123"))
     @patch("services.airlock.UserResourceRepository.get_user_resource_by_id", return_value=UserResource(id=USER_RESOURCE_ID, templateName="test", templateVersion="0.0.1", _etag="123"))
     @patch("services.airlock.AirlockRequestRepository.read_item_by_id", return_value=sample_airlock_request_object(status=AirlockRequestStatus.InReview, review_user_resource=True))
     @patch("services.airlock.AirlockRequestRepository.create_airlock_review_item", return_value=sample_airlock_review_object())
     @patch("services.airlock.update_and_publish_event_airlock_request", return_value=sample_airlock_request_object(status=AirlockRequestStatus.Approved, review_user_resource=True))
     @patch("services.airlock.AirlockRequestRepository.save_item")
-    async def test_post_create_airlock_review_cleans_up_review_user_resources(self, _, __, ___, ____, _____, ______, _______, send_uninstall_message_mock, app, client, sample_airlock_review_input_data):
+    async def test_post_create_airlock_review_cleans_up_review_user_resources(self, _, __, ___, ____, _____, ______, _______, ________, _________, __________, send_uninstall_message_mock, app, client, sample_airlock_review_input_data):
         response = await client.post(app.url_path_for(strings.API_REVIEW_AIRLOCK_REQUEST, workspace_id=WORKSPACE_ID, airlock_request_id=AIRLOCK_REQUEST_ID), json=sample_airlock_review_input_data)
         assert response.status_code == status.HTTP_200_OK
         assert send_uninstall_message_mock.call_count == 1

api_app/tests_ma/test_api/test_routes/test_workspaces.py

@@ -988,7 +988,7 @@ async def test_patch_user_resource_returns_422_if_invalid_id(self, get_workspace
 
     # [PATCH] /workspaces/{workspace_id}/workspace-services/{service_id}/user-resources/{resource_id}
     @ patch("api.routes.workspaces.ResourceHistoryRepository.save_item", return_value=AsyncMock())
-    @ patch("api.routes.workspaces.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
+    @ patch("api.routes.resource_helpers.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
     @ patch("api.routes.workspaces.ResourceTemplateRepository.get_template_by_name_and_version", return_value=None)
     @ patch("api.routes.workspaces.validate_user_has_valid_role_for_user_resource")
     @ patch("api.dependencies.workspaces.WorkspaceServiceRepository.get_workspace_service_by_id", return_value=sample_workspace_service())
@@ -1038,7 +1038,7 @@ async def test_patch_user_resource_with_upgrade_major_version_returns_bad_reques
 
     # [PATCH] /workspaces/{workspace_id}/workspace-services/{service_id}/user-resources/{resource_id}
     @ patch("api.routes.workspaces.ResourceHistoryRepository.save_item", return_value=AsyncMock())
-    @ patch("api.routes.workspaces.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
+    @ patch("api.routes.resource_helpers.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
     @ patch("api.routes.workspaces.ResourceTemplateRepository.get_template_by_name_and_version", return_value=sample_workspace_service())
     @ patch("api.routes.workspaces.validate_user_has_valid_role_for_user_resource")
     @ patch("api.dependencies.workspaces.WorkspaceServiceRepository.get_workspace_service_by_id", return_value=sample_workspace_service())
@@ -1089,7 +1089,7 @@ async def test_patch_user_resource_with_downgrade_version_returns_bad_request(se
 
     # [PATCH] /workspaces/{workspace_id}/workspace-services/{service_id}/user-resources/{resource_id}
     @ patch("api.routes.workspaces.ResourceHistoryRepository.save_item", return_value=AsyncMock())
-    @ patch("api.routes.workspaces.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
+    @ patch("api.routes.resource_helpers.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
     @ patch("api.routes.workspaces.ResourceTemplateRepository.get_template_by_name_and_version", return_value=sample_workspace_service())
     @ patch("api.routes.workspaces.validate_user_has_valid_role_for_user_resource")
     @ patch("api.dependencies.workspaces.WorkspaceServiceRepository.get_workspace_service_by_id", return_value=sample_workspace_service())
@@ -1115,7 +1115,7 @@ async def test_patch_user_resource_with_upgrade_minor_version_patches_user_resou
 
     # [PATCH] /workspaces/{workspace_id}/workspace-services/{service_id}/user-resources/{resource_id}
     @ patch("api.routes.workspaces.ResourceHistoryRepository.save_item", return_value=AsyncMock())
-    @ patch("api.routes.workspaces.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
+    @ patch("api.routes.resource_helpers.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
     @ patch("api.routes.workspaces.UserResourceRepository.update_item_with_etag", return_value=sample_user_resource_object())
     @ patch("api.routes.workspaces.ResourceTemplateRepository.get_template_by_name_and_version", return_value=sample_resource_template())
     @ patch("api.dependencies.workspaces.WorkspaceServiceRepository.get_workspace_service_by_id", return_value=sample_workspace_service())
@@ -1551,7 +1551,7 @@ async def test_patch_user_resource_returns_422_if_invalid_id(self, get_workspace
 
     # [PATCH] /workspaces/{workspace_id}/workspace-services/{service_id}/user-resources/{resource_id}
     @ patch("api.routes.workspaces.ResourceHistoryRepository.save_item", return_value=AsyncMock())
-    @ patch("api.routes.workspaces.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
+    @ patch("api.routes.resource_helpers.send_resource_request_message", return_value=sample_resource_operation(resource_id=USER_RESOURCE_ID, operation_id=OPERATION_ID))
     @ patch("api.routes.workspaces.ResourceTemplateRepository.get_template_by_name_and_version", return_value=None)
     @ patch("api.routes.workspaces.validate_user_has_valid_role_for_user_resource")
     @ patch("api.dependencies.workspaces.WorkspaceServiceRepository.get_workspace_service_by_id", return_value=sample_workspace_service())