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:
 

api_app/_version.py

@@ -1 +1 @@
-__version__ = "0.9.2"
+__version__ = "0.10.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/services/airlock.py

@@ -10,16 +10,19 @@
 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.domain.request_action import RequestAction
 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, send_resource_request_message
 
 from db.repositories.user_resources import UserResourceRepository
 from db.repositories.workspace_services import WorkspaceServiceRepository
@@ -381,6 +384,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 +407,31 @@ 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)
+    patched_user_resource, resource_template = await user_resource_repo.patch_user_resource(user_resource, resource_patch, user_resource.etag, resource_template_repo, resource_history_repo, workspace_service.templateName, user, force_version_update=False)
+
+    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
+
+
 async def delete_all_review_user_resources(
         airlock_request: AirlockRequest,
         user_resource_repo: UserResourceRepository,
@@ -430,3 +461,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.send_resource_request_message")
     @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_services/test_airlock.py

@@ -4,8 +4,8 @@
 import time
 from resources import strings
 from services.airlock import validate_user_allowed_to_access_storage_account, get_required_permission, \
-    validate_request_status
-from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType, AirlockReview, AirlockReviewDecision, AirlockActions
+    validate_request_status, cancel_request, delete_review_user_resource
+from models.domain.airlock_request import AirlockRequest, AirlockRequestStatus, AirlockRequestType, AirlockReview, AirlockReviewDecision, AirlockActions, AirlockReviewUserResource
 from tests_ma.test_api.conftest import create_workspace_owner_user, create_workspace_researcher_user
 from mock import AsyncMock, patch, MagicMock
 from models.domain.events import AirlockNotificationData, AirlockNotificationUserData, StatusChangedData, \
@@ -22,6 +22,8 @@
 WORKSPACE_ID = "abc000d3-82da-4bfc-b6e9-9a7853ef753e"
 AIRLOCK_REQUEST_ID = "5dbc15ae-40e1-49a5-834b-595f59d626b7"
 AIRLOCK_REVIEW_ID = "96d909c5-e913-4c05-ae53-668a702ba2e5"
+USER_RESOURCE_ID = "cce59042-1dee-42dc-9388-6db846feeb3b"
+WORKSPACE_SERVICE_ID = "30f2fefa-e7bb-4e5b-93aa-e50bb037502a"
 CURRENT_TIME = time.time()
 
 
@@ -51,6 +53,7 @@ def sample_airlock_request(status=AirlockRequestStatus.Draft):
         id=AIRLOCK_REQUEST_ID,
         workspaceId=WORKSPACE_ID,
         type=AirlockRequestType.Import,
+        reviewUserResources={"user-guid-here": sample_airlock_user_resource_object()},
         files=[AirlockFile(
             name="data.txt",
             size=5
@@ -71,6 +74,14 @@ def sample_airlock_request(status=AirlockRequestStatus.Draft):
     return airlock_request
 
 
+def sample_airlock_user_resource_object():
+    return AirlockReviewUserResource(
+        workspaceId=WORKSPACE_ID,
+        workspaceServiceId=WORKSPACE_SERVICE_ID,
+        userResourceId=USER_RESOURCE_ID
+    )
+
+
 def sample_status_changed_event(new_status="draft", previous_status=None):
     status_changed_event = EventGridEvent(
         event_type="statusChanged",
@@ -453,3 +464,36 @@ async def test_get_allowed_actions_requires_same_roles_as_endpoint(action, requi
         user.roles = [role]
         allowed_actions = get_allowed_actions(request=sample_airlock_request(), user=user, airlock_request_repo=airlock_request_repo_mock)
         assert action in allowed_actions
+
+
+@pytest.mark.asyncio
+@patch("services.airlock.delete_review_user_resource")
+@patch("services.airlock.update_and_publish_event_airlock_request")
+async def test_cancel_request_deletes_review_resource(_, delete_review_user_resource, airlock_request_repo_mock):
+    await cancel_request(
+        airlock_request=sample_airlock_request(),
+        user=create_test_user(),
+        airlock_request_repo=airlock_request_repo_mock,
+        workspace=sample_workspace(),
+        user_resource_repo=AsyncMock(),
+        workspace_service_repo=AsyncMock(),
+        resource_template_repo=AsyncMock(),
+        operations_repo=AsyncMock(),
+        resource_history_repo=AsyncMock())
+
+    delete_review_user_resource.assert_called_once()
+
+
+@pytest.mark.asyncio
+@patch("services.airlock.disable_user_resource")
+@patch("services.airlock.send_uninstall_message")
+@patch("services.airlock.update_and_publish_event_airlock_request")
+async def test_delete_review_user_resource_disables_the_resource_before_deletion(_, __, disable_user_resource):
+    await delete_review_user_resource(user_resource=AsyncMock(),
+                                      user_resource_repo=AsyncMock(),
+                                      workspace_service_repo=AsyncMock(),
+                                      resource_template_repo=AsyncMock(),
+                                      operations_repo=AsyncMock(),
+                                      resource_history_repo=AsyncMock(),
+                                      user=create_test_user())
+    disable_user_resource.assert_called_once()

e2e_tests/test_airlock.py

@@ -145,7 +145,7 @@ async def test_airlock_review_vm_flow(setup_test_workspace_and_workspace_service
 
     # Check that deletion for user resource has started
     user_resource = await get_resource(f"/api{user_resource_path}", import_workspace_owner_token, verify)
-    assert user_resource["userResource"]["deploymentStatus"] == "deleting"
+    assert user_resource["userResource"]["deploymentStatus"] == "updating"
     LOGGER.info("Review VM has started deletion successfully")
 
     # EXPORT FLOW