From 0a46a762fd4a5931e9d61b3fc314b2a575522ee2 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Thu, 26 Jan 2023 16:01:19 +0000 Subject: [PATCH 01/12] clean up review VMS when an airlock request is cancelled --- CHANGELOG.md | 1 + api_app/api/routes/airlock.py | 12 +++-- api_app/services/airlock.py | 41 +++++++++++++++- .../test_api/test_routes/test_airlock.py | 9 ++-- .../tests_ma/test_services/test_airlock.py | 48 ++++++++++++++++++- 5 files changed, 101 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3febea9645..110eb1e4ce 100644 --- a/CHANGELOG.md +++ b/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 ([#3105](https://github.com/microsoft/AzureTRE/pull/3105)) BUG FIXES: diff --git a/api_app/api/routes/airlock.py b/api_app/api/routes/airlock.py index a0f9633b4f..a72a071dd8 100644 --- a/api_app/api/routes/airlock.py +++ b/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) diff --git a/api_app/services/airlock.py b/api_app/services/airlock.py index 8027ea1cba..13563c16f2 100644 --- a/api_app/services/airlock.py +++ b/api_app/services/airlock.py @@ -10,8 +10,11 @@ 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 @@ -19,7 +22,7 @@ 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 diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock.py b/api_app/tests_ma/test_api/test_routes/test_airlock.py index 6ad2e65cab..d7c202b2ec 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock.py +++ b/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,6 +332,9 @@ 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")) @@ -339,7 +342,7 @@ async def test_post_create_airlock_review_with_illegal_status_change_returns_400 @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 diff --git a/api_app/tests_ma/test_services/test_airlock.py b/api_app/tests_ma/test_services/test_airlock.py index 8c07b329c0..39b309d4b7 100644 --- a/api_app/tests_ma/test_services/test_airlock.py +++ b/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() From 22fb243abfb0d5d24ec844b33f2de3f1f334dcc3 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Thu, 26 Jan 2023 16:03:29 +0000 Subject: [PATCH 02/12] update changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 110eb1e4ce..5b07b30481 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,7 +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 ([#3105](https://github.com/microsoft/AzureTRE/pull/3105)) +* Review VMs are being cleaned up when an Airlock request is canceled ([#3130](https://github.com/microsoft/AzureTRE/pull/3130)) BUG FIXES: From f71777d7fc98e9c49cf7adb3f8818d9563f79c1b Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Thu, 26 Jan 2023 16:15:07 +0000 Subject: [PATCH 03/12] 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 a2fecb4576..c5981731c5 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.9.2" +__version__ = "0.9.3" From 14cca3a99490764b8642af2e5c77c7a6d6b4c7ee Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Thu, 26 Jan 2023 21:12:00 +0000 Subject: [PATCH 04/12] fix e2e --- e2e_tests/test_airlock.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/e2e_tests/test_airlock.py b/e2e_tests/test_airlock.py index 520a024336..030c8a1f1a 100644 --- a/e2e_tests/test_airlock.py +++ b/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 From 07827f5c51639c184beb50e7eb2974278a172e62 Mon Sep 17 00:00:00 2001 From: Yuval Yaron <43217306+yuvalyaron@users.noreply.github.com> Date: Sun, 29 Jan 2023 10:19:25 +0200 Subject: [PATCH 05/12] Update api_app/_version.py Co-authored-by: Tamir Kamara <26870601+tamirkamara@users.noreply.github.com> --- 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 c5981731c5..61fb31cae0 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.9.3" +__version__ = "0.10.0" From 37fa80ec55f46cb6ffc1f4f2ef12c64488ae483e Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 29 Jan 2023 13:23:48 +0000 Subject: [PATCH 06/12] add update_user_resource method in resource_helpers --- api_app/api/routes/resource_helpers.py | 29 ++++++++++++++++++++++++++ api_app/api/routes/workspaces.py | 14 ++----------- api_app/services/airlock.py | 17 ++++----------- 3 files changed, 35 insertions(+), 25 deletions(-) diff --git a/api_app/api/routes/resource_helpers.py b/api_app/api/routes/resource_helpers.py index 9e5626d196..49b573d421 100644 --- a/api_app/api/routes/resource_helpers.py +++ b/api_app/api/routes/resource_helpers.py @@ -4,6 +4,10 @@ from typing import Dict, Any, Optional from fastapi import HTTPException, status +from api_app.db.repositories.user_resources import UserResourceRepository +from api_app.models.domain.user_resource import UserResource +from api_app.models.domain.workspace_service import WorkspaceService +from api_app.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,28 @@ 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, + 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, user_resource.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 diff --git a/api_app/api/routes/workspaces.py b/api_app/api/routes/workspaces.py index 9a728cfcf2..155abcdedb 100644 --- a/api_app/api/routes/workspaces.py +++ b/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 = update_user_resource(user_resource, user_resource_patch, force_version_update, user, 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: diff --git a/api_app/services/airlock.py b/api_app/services/airlock.py index 13563c16f2..24e99b60f3 100644 --- a/api_app/services/airlock.py +++ b/api_app/services/airlock.py @@ -11,7 +11,6 @@ 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 @@ -22,7 +21,7 @@ from resources import strings, constants -from api.routes.resource_helpers import save_and_deploy_resource, send_uninstall_message, send_resource_request_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 @@ -417,17 +416,9 @@ async def disable_user_resource( 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) + operation = update_user_resource(user_resource=user_resource, resource_patch=resource_patch, force_version_update=False, + user=user, 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 From 1758ed7f7782003e201bbb3f16b8f924451d7665 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 29 Jan 2023 15:00:36 +0000 Subject: [PATCH 07/12] fix imports --- api_app/api/routes/resource_helpers.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/api_app/api/routes/resource_helpers.py b/api_app/api/routes/resource_helpers.py index 49b573d421..ace6618714 100644 --- a/api_app/api/routes/resource_helpers.py +++ b/api_app/api/routes/resource_helpers.py @@ -4,10 +4,10 @@ from typing import Dict, Any, Optional from fastapi import HTTPException, status -from api_app.db.repositories.user_resources import UserResourceRepository -from api_app.models.domain.user_resource import UserResource -from api_app.models.domain.workspace_service import WorkspaceService -from api_app.models.schemas.resource import ResourcePatch +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 From 9b60e6f195a6c84077e52e331d4b9738fe70e130 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 29 Jan 2023 15:56:06 +0000 Subject: [PATCH 08/12] add await to coroutines --- api_app/api/routes/workspaces.py | 2 +- api_app/services/airlock.py | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/api_app/api/routes/workspaces.py b/api_app/api/routes/workspaces.py index 155abcdedb..880d47b50d 100644 --- a/api_app/api/routes/workspaces.py +++ b/api_app/api/routes/workspaces.py @@ -466,7 +466,7 @@ async def patch_user_resource( validate_user_has_valid_role_for_user_resource(user, user_resource) try: - operation = update_user_resource(user_resource, user_resource_patch, force_version_update, user, workspace_service, user_resource_repo, resource_template_repo, operations_repo, resource_history_repo) + operation = await update_user_resource(user_resource, user_resource_patch, force_version_update, user, 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: diff --git a/api_app/services/airlock.py b/api_app/services/airlock.py index 24e99b60f3..f8cd7e1fa5 100644 --- a/api_app/services/airlock.py +++ b/api_app/services/airlock.py @@ -416,9 +416,9 @@ async def disable_user_resource( resource_history_repo: ResourceHistoryRepository) -> Operation: resource_patch = ResourcePatch(isEnabled=False) - operation = update_user_resource(user_resource=user_resource, resource_patch=resource_patch, force_version_update=False, - user=user, 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) + operation = await update_user_resource(user_resource=user_resource, resource_patch=resource_patch, force_version_update=False, + user=user, 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 From 351da94fd0f0f4b0cfd834670570a616a0cc9b73 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 29 Jan 2023 18:15:33 +0000 Subject: [PATCH 09/12] remove guacamole creation from review VM test --- e2e_tests/conftest.py | 4 ++-- e2e_tests/test_airlock.py | 10 +++++----- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/e2e_tests/conftest.py b/e2e_tests/conftest.py index d429d4ba00..9a7a60bc28 100644 --- a/e2e_tests/conftest.py +++ b/e2e_tests/conftest.py @@ -127,7 +127,7 @@ async def setup_test_workspace(verify) -> Tuple[str, str, str]: # Session scope isn't in effect with python-xdist: https://github.com/microsoft/AzureTRE/issues/2868 @pytest.fixture(scope="session") -async def setup_test_workspace_and_workspace_service(verify, setup_test_workspace): +async def setup_test_workspace_and_guacamole_service(verify, setup_test_workspace): # Set up workspace_path, workspace_id, workspace_owner_token = setup_test_workspace @@ -160,7 +160,7 @@ async def setup_test_aad_workspace(verify) -> Tuple[str, str, str]: # Session scope isn't in effect with python-xdist: https://github.com/microsoft/AzureTRE/issues/2868 @pytest.fixture(scope="session") -async def setup_test_airlock_import_review_workspace_and_workspace_service(verify) -> Tuple[str, str, str, str, str]: +async def setup_test_airlock_import_review_workspace_and_guacamole_service(verify) -> Tuple[str, str, str, str, str]: pre_created_workspace_id = config.TEST_AIRLOCK_IMPORT_REVIEW_WORKSPACE_ID # Set up workspace_path, workspace_id = await create_or_get_test_workspace(auth_type="Automatic", verify=verify, template_name=resource_strings.AIRLOCK_IMPORT_REVIEW_WORKSPACE, pre_created_workspace_id=pre_created_workspace_id) diff --git a/e2e_tests/test_airlock.py b/e2e_tests/test_airlock.py index 030c8a1f1a..baffcdc3d2 100644 --- a/e2e_tests/test_airlock.py +++ b/e2e_tests/test_airlock.py @@ -73,10 +73,10 @@ async def submit_airlock_import_request(workspace_path: str, workspace_owner_tok @pytest.mark.timeout(50 * 60) @pytest.mark.airlock -async def test_airlock_review_vm_flow(setup_test_workspace_and_workspace_service, setup_test_airlock_import_review_workspace_and_workspace_service, verify): - workspace_path, workspace_id, workspace_service_path, workspace_service_id, workspace_owner_token = setup_test_workspace_and_workspace_service - _, import_review_workspace_id, _, import_review_workspace_service_id, _ = setup_test_airlock_import_review_workspace_and_workspace_service - LOGGER.info("Workspace and workspace service set up") +async def test_airlock_review_vm_flow(setup_test_workspace, setup_test_airlock_import_review_workspace_and_guacamole_service, verify): + workspace_path, workspace_id, workspace_owner_token = setup_test_workspace + _, import_review_workspace_id, _, import_review_workspace_service_id, _ = setup_test_airlock_import_review_workspace_and_guacamole_service + LOGGER.info("Base workspace set up") # Preparation: Update the research workspace so that it has the import review details patch_payload = { @@ -90,7 +90,7 @@ async def test_airlock_review_vm_flow(setup_test_workspace_and_workspace_service "import_vm_user_resource_template_name": "tre-service-guacamole-import-reviewvm" }, "export": { - "export_vm_workspace_service_id": workspace_service_id, + "export_vm_workspace_service_id": "", "export_vm_user_resource_template_name": "tre-service-guacamole-export-reviewvm" } } From 33daaf15b404c1e1df80ec647f9a6a61fa430254 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 29 Jan 2023 19:39:14 +0000 Subject: [PATCH 10/12] fix tests - use the etag from the route instead of from the resource --- api_app/api/routes/resource_helpers.py | 3 ++- api_app/api/routes/workspaces.py | 2 +- api_app/services/airlock.py | 2 +- api_app/tests_ma/test_api/test_routes/test_airlock.py | 2 +- .../tests_ma/test_api/test_routes/test_workspaces.py | 10 +++++----- 5 files changed, 10 insertions(+), 9 deletions(-) diff --git a/api_app/api/routes/resource_helpers.py b/api_app/api/routes/resource_helpers.py index ace6618714..4ec28bc6f6 100644 --- a/api_app/api/routes/resource_helpers.py +++ b/api_app/api/routes/resource_helpers.py @@ -267,13 +267,14 @@ async def update_user_resource( 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, user_resource.etag, resource_template_repo, resource_history_repo, workspace_service.templateName, user, force_version_update) + 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, diff --git a/api_app/api/routes/workspaces.py b/api_app/api/routes/workspaces.py index 880d47b50d..f6f9ffe84d 100644 --- a/api_app/api/routes/workspaces.py +++ b/api_app/api/routes/workspaces.py @@ -466,7 +466,7 @@ async def patch_user_resource( validate_user_has_valid_role_for_user_resource(user, user_resource) try: - operation = await update_user_resource(user_resource, user_resource_patch, force_version_update, user, workspace_service, user_resource_repo, resource_template_repo, operations_repo, resource_history_repo) + 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: diff --git a/api_app/services/airlock.py b/api_app/services/airlock.py index f8cd7e1fa5..4023eea315 100644 --- a/api_app/services/airlock.py +++ b/api_app/services/airlock.py @@ -417,7 +417,7 @@ async def disable_user_resource( resource_patch = ResourcePatch(isEnabled=False) operation = await update_user_resource(user_resource=user_resource, resource_patch=resource_patch, force_version_update=False, - user=user, workspace_service=workspace_service, user_resource_repo=user_resource_repo, + 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 diff --git a/api_app/tests_ma/test_api/test_routes/test_airlock.py b/api_app/tests_ma/test_api/test_routes/test_airlock.py index d7c202b2ec..f75a287347 100644 --- a/api_app/tests_ma/test_api/test_routes/test_airlock.py +++ b/api_app/tests_ma/test_api/test_routes/test_airlock.py @@ -334,7 +334,7 @@ async def test_post_create_airlock_review_with_illegal_status_change_returns_400 @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.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")) diff --git a/api_app/tests_ma/test_api/test_routes/test_workspaces.py b/api_app/tests_ma/test_api/test_routes/test_workspaces.py index b7b1aae140..c211f638da 100644 --- a/api_app/tests_ma/test_api/test_routes/test_workspaces.py +++ b/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()) From f3fe7d9859fb7a21b1d0c313f7b4b2bf019a2416 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Sun, 29 Jan 2023 19:53:18 +0000 Subject: [PATCH 11/12] update 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 61fb31cae0..ae6db5f176 100644 --- a/api_app/_version.py +++ b/api_app/_version.py @@ -1 +1 @@ -__version__ = "0.10.0" +__version__ = "0.11.0" From 199d17c2270581bcbd27f1b6341727a21ddbd189 Mon Sep 17 00:00:00 2001 From: Yuval Yaron Date: Mon, 30 Jan 2023 12:51:45 +0000 Subject: [PATCH 12/12] remove log about fixtures --- e2e_tests/test_airlock.py | 1 - 1 file changed, 1 deletion(-) diff --git a/e2e_tests/test_airlock.py b/e2e_tests/test_airlock.py index baffcdc3d2..f9c395a32f 100644 --- a/e2e_tests/test_airlock.py +++ b/e2e_tests/test_airlock.py @@ -76,7 +76,6 @@ async def submit_airlock_import_request(workspace_path: str, workspace_owner_tok async def test_airlock_review_vm_flow(setup_test_workspace, setup_test_airlock_import_review_workspace_and_guacamole_service, verify): workspace_path, workspace_id, workspace_owner_token = setup_test_workspace _, import_review_workspace_id, _, import_review_workspace_service_id, _ = setup_test_airlock_import_review_workspace_and_guacamole_service - LOGGER.info("Base workspace set up") # Preparation: Update the research workspace so that it has the import review details patch_payload = {