Redesign airlock storage account architecture - #5048
Redesign airlock storage account architecture#5048Marcus Robinson (marrobi) wants to merge 77 commits into
Conversation
Squashed 61 commits into a single commit for a clean PR.
Unit Test Results973 tests 973 ✅ 11s ⏱️ Results for commit 2264ade. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Pull request overview
Introduces Airlock v2 consolidated storage while retaining legacy v1 support.
Changes:
- Adds metadata-based shared storage and ABAC controls.
- Updates API and processor routing for per-request Airlock versions.
- Adds migration configuration, tests, and architecture documentation.
Reviewed changes
Copilot reviewed 58 out of 59 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
templates/workspaces/base/terraform/workspace.tf |
Selects v1 or v2 Airlock module. |
templates/workspaces/base/terraform/variables.tf |
Adds Airlock version input. |
templates/workspaces/base/terraform/airlock_v2/variables.tf |
Defines v2 module inputs. |
templates/workspaces/base/terraform/airlock_v2/storage_accounts.tf |
Configures shared storage access and ABAC. |
templates/workspaces/base/terraform/airlock_v2/providers.tf |
Configures v2 providers. |
templates/workspaces/base/terraform/airlock_v2/locals.tf |
Defines shared storage names. |
templates/workspaces/base/terraform/airlock_v2/data.tf |
Reads core identities and DNS. |
templates/workspaces/base/template_schema.json |
Exposes Airlock version property. |
templates/workspaces/base/porter.yaml |
Passes Airlock version to Terraform. |
templates/workspaces/airlock-import-review/terraform/import_review_resources.terraform |
Reconfigures import-review storage access. |
templates/workspaces/airlock-import-review/porter.yaml |
Updates template version. |
mkdocs.yml |
Adds legacy documentation navigation. |
e2e_tests/test_airlock.py |
Consolidates existing Airlock flow coverage. |
e2e_tests/test_airlock_consolidated.py |
Adds v2 end-to-end tests. |
e2e_tests/pytest.ini |
Registers consolidated test marker. |
e2e_tests/conftest.py |
Supports automatic workspace authentication. |
docs/azure-tre-overview/airlock.md |
Documents consolidated architecture. |
docs/azure-tre-overview/airlock-legacy.md |
Documents legacy architecture and migration. |
core/version.txt |
Updates core version. |
core/terraform/variables.tf |
Adds legacy infrastructure toggle. |
core/terraform/main.tf |
Connects consolidated Airlock modules. |
core/terraform/appgateway/variables.tf |
Adds storage backend input. |
core/terraform/appgateway/locals.tf |
Defines storage routing names. |
core/terraform/appgateway/appgateway.tf |
Adds storage proxy routing. |
core/terraform/api-webapp.tf |
Exposes App Gateway FQDN. |
core/terraform/airlock/variables.tf |
Adds module legacy toggle. |
core/terraform/airlock/storage_accounts.tf |
Creates consolidated storage infrastructure. |
core/terraform/airlock/storage_accounts_v1.tf |
Preserves conditional legacy accounts. |
core/terraform/airlock/outputs.tf |
Exposes core storage FQDN. |
core/terraform/airlock/locals.tf |
Defines v1 and v2 resource names. |
core/terraform/airlock/identity.tf |
Relocates storage role assignments. |
core/terraform/airlock/eventgrid_topics.tf |
Adds consolidated event subscriptions. |
core/terraform/airlock/eventgrid_topics_v1.tf |
Preserves conditional legacy events. |
core/terraform/airlock/data.tf |
Updates diagnostic source topic. |
config.sample.yaml |
Documents legacy toggle. |
config_schema.json |
Validates legacy toggle. |
CHANGELOG.md |
Records Airlock migration support. |
api_app/tests_ma/test_services/test_airlock.py |
Tests v2 links and review workspace events. |
api_app/tests_ma/test_services/test_airlock_storage_helper.py |
Tests account and stage mapping. |
api_app/services/airlock.py |
Routes SAS links by Airlock version. |
api_app/services/airlock_storage_helper.py |
Implements API storage mapping. |
api_app/resources/constants.py |
Adds consolidated names and stages. |
api_app/models/domain/events.py |
Extends status event metadata. |
api_app/models/domain/airlock_request.py |
Persists request Airlock version. |
api_app/event_grid/event_sender.py |
Publishes versioned workspace metadata. |
api_app/db/repositories/airlock_requests.py |
Stamps request versions. |
api_app/core/config.py |
Reads App Gateway configuration. |
api_app/api/routes/airlock.py |
Selects workspace Airlock version. |
airlock_processor/tests/test_status_change_queue_trigger.py |
Tests versioned status transitions. |
airlock_processor/tests/test_blob_created_trigger.py |
Tests v2 blob completion events. |
airlock_processor/tests/shared_code/test_blob_operations_metadata.py |
Tests metadata storage operations. |
airlock_processor/tests/shared_code/test_airlock_storage_helper.py |
Tests processor storage mapping. |
airlock_processor/StatusChangedQueueTrigger/__init__.py |
Implements metadata transitions and copies. |
airlock_processor/shared_code/constants.py |
Adds v2 processor constants. |
airlock_processor/shared_code/blob_operations_metadata.py |
Implements metadata container operations. |
airlock_processor/shared_code/airlock_storage_helper.py |
Resolves processor accounts and stages. |
airlock_processor/BlobCreatedTrigger/__init__.py |
Handles v2 copy completion events. |
airlock_processor/_version.py |
Updates processor version. |
.gitignore |
Ignores old Terraform files. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
… cases (microsoft#5048) - Default airlock requests to v2 in the API; backfill pre-existing workspaces with airlock_version=1 via a DB migration (thread: porter.yaml default mismatch). - enable_legacy_airlock defaults to true in config schema; sample config sets false. - v1 import-in-progress account name no longer altered by review_workspace_id. - BlobCreatedTrigger re-raises container metadata read failures (no silent hang). - Persist the on-upload malware scan verdict and apply it on submission (v2). - airlock_v2 shared storage data source uses the core provider alias. - Correct the async Event Grid copy-completion architecture docs.
…w VMs (microsoft#5048) Add import-in-progress to the API identity ABAC condition on the consolidated core storage account so a user-delegation SAS for an in-review import can be read by the review VM (previously received 403).
…data Move the v2 Draft-time scan verdict handling from processor container metadata into an API-side pendingScanResult on the request. The status update consumer stores an early (Draft) scan verdict; the submit route applies it on submission. Removes the processor metadata persist/awaiting path. Follow-up multi-scan contract tracked in microsoft#5049.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 67 changed files in this pull request and generated 3 comments.
Suppressed comments (3)
templates/workspaces/base/terraform/airlock_v2/storage_accounts.tf:26
- Registering every workspace endpoint in the shared core blob DNS zone gives
stalairlockg...multiple A records: the processor endpoint already registers there, and each workspace adds another. A workspace can therefore resolve the processor or another workspace's endpoint; the role condition requires this workspace's exact private-endpoint ID, so those connections receive 403s intermittently. Use DNS scoped per workspace/VNet (or route all access through one shared endpoint) so this hostname resolves to only the endpoint allowed by the ABAC condition.
config_schema.json:91 - The new setting is not forwarded by the GitHub Actions deployment path.
devcontainer_run_commandruns withUSE_ENV_VARS_NOT_FILES=trueand injectsTF_VAR_enable_airlock_malware_scanning, but has no input orTF_VAR_enable_legacy_airlock; hosted deployments therefore always use Terraform's defaulttrueand cannot apply the advertised toggle. Add the action input, workflow variable plumbing, and container environment mapping.
docs/azure-tre-overview/airlock-legacy.md:10 - Version 1 is not the default: the workspace Terraform variable, Porter parameter, schema, and API request model all default to version 2. This instruction can cause operators to misunderstand migration behavior; describe version 1 as an explicit legacy selection.
…migration, scan/stage edge cases (microsoft#5048) - DNS: resolve the shared global airlock account to each workspace's own private endpoint via a workspace-scoped (more-qualified) private DNS zone + manual A record, instead of colliding in the shared core blob zone. - Migration: backfill in-flight airlock requests (not just workspaces) with airlock_version=1 so legacy requests keep routing to legacy storage. - ScanResultTrigger: ignore scan results for copied (post-approval) blobs so they don't dead-letter against an already-advanced request. - StatusChangedQueueTrigger: don't let a late/duplicate submitted event revert a container out of a terminal (blocked/rejected/approved) stage. - CI: thread enable_legacy_airlock through the devcontainer action. - Docs: airlock_version 1 is an explicit legacy opt-in, not the default.
|
Also addressed the three suppressed review comments in 6e7909e:
The two still-open threads are intentionally left for follow-up: the v1→v2 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 69 out of 70 changed files in this pull request and generated 4 comments.
Suppressed comments (1)
core/terraform/airlock/storage_accounts.tf:257
- This second system topic has the same unsupported argument: AzureRM 4.57.0 requires
source_resource_id. As written, Terraform fails validation before the workspace-global BlobCreated topic can be created.
microsoft#4964 (microsoft#5048) Recovers the multi-workspace fix lost when the PR was recreated: - Per-workspace airlock SAS signer app registration (airlock_v2/signer.tf) with a federated identity credential so the core API managed identity mints SAS as the signer (no secret). The shared global-account role assignment now uses the per-workspace signer principal, so multiple v2 workspaces no longer collide with RoleAssignmentExists 409, and a SAS leaked from one workspace cannot be replayed from another (per-workspace PE + ABAC enforced). - API: get_airlock_signer_credential (credentials.py) + airlock.py signs the global-account SAS as the per-workspace signer; falls back to the API identity for v1/core. - Guard: block changing a workspace airlock_version while it has in-flight requests (legacy_airlock_guard.ensure_airlock_version_change_allowed + get_in_flight_airlock_request_ids_for_workspace), wired into patch_workspace. - Persist airlock_signer_client_id as a workspace property (porter output). Deferred (needs enable_legacy_airlock exposed to the API): the create-time version-supported check and the startup legacy-airlock migration guard.
) - Expose enable_legacy_airlock (+ block_disable_legacy_airlock_if_v1_exists) to the API via app settings/config, and restore the two remaining microsoft#4964 guards: ensure_workspace_airlock_version_supported (block creating a v1 workspace when legacy airlock is disabled) and run_legacy_airlock_migration_guard (warn/block at startup if active v1 dependencies remain). Adds get_active_v1_workspace_ids and get_in_flight_v1_airlock_request_ids. - ScanResultTrigger: only suppress copied blobs in the consolidated stalairlock accounts, so the v1 submit copy into stalimip still emits its scan StepResult. - StatusChangedQueueTrigger: default missing airlock_version to 1 (legacy) so queued events from a pre-v2 API aren't routed to consolidated accounts. - core: switch eventgrid source_arm_resource_id -> source_resource_id; drop the unused APP_GATEWAY_FQDN api app setting.
…d enable_legacy_airlock (microsoft#5048) When enable_legacy_airlock is set, the import-review workspace now also provisions a private endpoint + private DNS to the legacy stalimip import-in- progress account (count-gated), so review VMs for airlock_version=1 requests can still reach their in-progress data. Reviewers access it via per-request SAS. Adds the enable_legacy_airlock bundle parameter (default true) threaded into the terraform steps. tre-workspace-airlock-import-review 1.6.0.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 77 out of 78 changed files in this pull request and generated 2 comments.
Suppressed comments (6)
templates/workspaces/airlock-import-review/terraform/import_review_resources.terraform:20
- This template now provisions connectivity only to
stalairlock${TRE_ID}. A legacy v1 import review still receives a SAS forstalimip${TRE_ID}, but this workspace no longer has private endpoint/DNS connectivity to that account, so review VMs cannot open v1 requests. Keep a count-gated legacy endpoint while v1 is enabled, or make the review workspace version-aware.
api_app/api/routes/airlock.py:106 - When a pending verdict exists, this immediately advances the database beyond
Submittedbefore the asynchronous submitted event enumerates files. That enumeration emitscompleted_step="submitted", butAirlockStatusUpdaternow rejects it because the request is alreadyInReview/BlockingInProgress, sorequest_filesis never persisted and the message retries/dead-letters. Apply file-only results independently of status, or serialize enumeration before this transition.
updated_request = await update_and_publish_event_airlock_request(
updated_request, airlock_request_repo, user, workspace,
new_status=AirlockRequestStatus(pending["new_status"]),
status_message=pending.get("status_message"),
pending_scan_result=None)
api_app/api/routes/workspaces.py:136
- The create path rejects v1 when legacy core resources are disabled, but the patch path only checks in-flight requests. An administrator can therefore change an airlock-enabled workspace to version 1 with
ENABLE_LEGACY_AIRLOCK=false; deployment then targets core accounts that do not exist. Validate the merged workspace properties here as well.
await ensure_airlock_version_change_allowed(workspace, resource_patch, airlock_request_repo)
airlock_processor/StatusChangedQueueTrigger/init.py:132
- This is the second
set()on the same single-event output binding during a v2 submit when scanning is disabled; the earlier file-enumeration event is overwritten rather than publishing two events. As a result the request reachesInReviewwithout persistingrequest_files. Accumulate both events and set anOut[List[EventGridOutputEvent]]once, or publish them separately.
stepResultEvent.set(
func.EventGridOutputEvent(
id=str(uuid.uuid4()),
data={"completed_step": constants.STAGE_SUBMITTED, "new_status": constants.STAGE_IN_REVIEW, "request_id": req_id},
subject=req_id,
event_type="Airlock.StepResult",
event_time=datetime.datetime.now(datetime.UTC),
data_version=constants.STEP_RESULT_EVENT_DATA_VERSION))
airlock_processor/StatusChangedQueueTrigger/init.py:104
- The v2 same-account submit path no longer calls
copy_data, which was also where the one-file invariant was enforced.get_request_filesonly enumerates, so a multi-file request now proceeds to review and fails only during the later approval copy (and independent scan verdicts can race). Reject zero/multiple files before changing the container toimport-in-progress.
source_account = airlock_storage_helper.get_storage_account_name_for_request(request_type, previous_status, ws_id, airlock_version=request_properties.airlock_version)
dest_account = airlock_storage_helper.get_storage_account_name_for_request(request_type, new_status, effective_ws_id, airlock_version=request_properties.airlock_version)
new_stage = airlock_storage_helper.get_stage_from_status(request_type, new_status)
if source_account == dest_account:
api_app/main.py:32
- On an existing pre-v2 deployment with legacy disabled, this guard runs before the
/migrationsendpoint is available and treats missing versions as v1. The deployment workflow waits for API health before callingdb-migrate, so the API never starts and the backfill that would unblock it cannot run. Run/backfill the migration before this blocking check, or require a staged deployment with legacy enabled.
await run_legacy_airlock_migration_guard()
…icrosoft#5048) Addresses the startup deadlock: run_legacy_airlock_migration_guard now backfills airlock_version on pre-v2 workspaces/requests in-process before evaluating v1 dependencies, so it no longer treats every missing version as v1 and no longer depends on the external db-migrate call (which needs the API healthy). Blocking behaviour is retained.
… core import-in-progress private-link (microsoft#5048) - Version-change guard now blocks on any data-retaining (non-cancelled) airlock request, not just in-flight, so switching v1->v2 can't destroy approved-import data/links; patch_workspace also validates merged properties against enable_legacy_airlock. - v2 submit: reject zero/>1 files (the metadata submit no longer copies via copy_data which enforced this); carry request_files on the scanning-disabled in_review event so files aren't lost to the single-value output binding; and the API status consumer persists file-enumeration results even when the request has already advanced (e.g. an early scan verdict), instead of dead-lettering. - Core consolidated account: require private link for import-in-progress in the API ABAC condition, so a leaked review SAS can't be replayed via the public endpoint (import-external/export-approved stay public).
|
Also addressed the suppressed comments from this review:
Commits: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated no new comments.
Suppressed comments (5)
airlock_processor/shared_code/blob_operations.py:144
- This new five-minute limit applies to every legacy and cross-account copy, not only to the v2 draft-sealing path that deletes its source immediately. A valid large airlock blob can remain
pendinglonger than 300 seconds; this code then aborts it and repeated deliveries can never complete the request. Keep the existing event-driven completion behavior for copies whose source is retained, and only synchronously wait where deletion truly requires it (with a timeout compatible with supported blob sizes/function limits).
# An async copy still reads from the source, so the caller must not delete it until this settles.
copy_status = copy.get("copy_status")
waited_seconds = 0
while copy_status == "pending" and waited_seconds < COPY_TIMEOUT_SECONDS:
time.sleep(COPY_POLL_INTERVAL_SECONDS)
waited_seconds += COPY_POLL_INTERVAL_SECONDS
copy_status = copied_blob.get_blob_properties().copy.status
if copy_status != "success":
if copy_status == "pending":
# Abort the copy so a late completion cannot recreate the destination after we fail,
# which would otherwise leave orphaned data once the source is deleted.
try:
copied_blob.abort_copy(copy["copy_id"])
logging.warning(f"Aborted still-pending copy of '{source_blob.blob_name}' after {waited_seconds}s")
except Exception as abort_error:
logging.error(f"Failed aborting pending copy of '{source_blob.blob_name}': {abort_error}")
raise Exception(f"Copy of '{source_blob.blob_name}' did not complete: status '{copy_status}' after {waited_seconds}s")
api_app/api/routes/workspaces.py:181
- Shared-container cleanup is started only after the uninstall has been queued. If the repository query or credential setup fails, this endpoint returns 500 even though workspace deletion is already progressing; if an individual deletion fails,
delete_workspace_airlock_containerssuppresses it and the workspace signer/role assignment can be destroyed while sensitive containers remain permanently in the shared accounts. Make cleanup a durable, retryable prerequisite/operation step and only complete workspace deletion after it succeeds.
# Shared storage outlives the workspace, so remove its containers once uninstall is queued.
await delete_workspace_airlock_containers(workspace, airlock_request_repo)
airlock_processor/StatusChangedQueueTrigger/init.py:334
- If neither the draft nor sealed container exists, this fallback selects the sealed name and
get_request_filesthen raisesResourceNotFoundError. That bypasses the newNoDataInRequestExceptionhandling, so the message is retried/dead-lettered instead of moving the request toFailedwith the intended diagnostic. Check the sealed container too and raiseNoDataInRequestExceptionwhen both are absent.
if not blob_operations.container_exists(storage_account_name, container_name):
container_name = request_properties.request_id
api_app/services/legacy_airlock_guard.py:38
- This comparison runs before template-schema validation. A PATCH such as
{"properties":{"airlock_version":"2"}}reaches"2" < 1, raises an uncaughtTypeError, and returns 500 instead of the normal 400 validation response. Validate that the value is an integer in the supported set before comparing versions (and reject booleans, which are Python integers).
airlock_processor/BlobCreatedTrigger/init.py:34 - The new v2 dispatch still uses the unchecked regex extraction immediately above it, so a malformed Event Grid subject raises an opaque
AttributeErrorbefore the hardened parser can produce the clearValueErrorpromised by this change. Parse the topic and subject throughget_blob_info_from_topic_and_subjectbefore dispatching.
if constants.STORAGE_ACCOUNT_NAME_AIRLOCK_CORE in topic or constants.STORAGE_ACCOUNT_NAME_AIRLOCK_WORKSPACE_GLOBAL in topic:
_handle_v2_blob_created(json_body, topic, request_id, stepResultEvent, dataDeletionEvent)
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated no new comments.
Suppressed comments (2)
airlock_processor/BlobCreatedTrigger/init.py:33
- The v2 branch still derives
request_idwith an unchecked regex before calling the new safe parser. A malformed Event Grid subject therefore raisesAttributeErrorfrom.group(1)instead of the clearValueErrorthis PR intends. Parse the topic and subject once withget_blob_info_from_topic_and_subjectbefore dispatching.
request_id = re.search(r'/blobServices/default/containers/(.*?)/blobs', json_body["subject"]).group(1)
if constants.STORAGE_ACCOUNT_NAME_AIRLOCK_CORE in topic or constants.STORAGE_ACCOUNT_NAME_AIRLOCK_WORKSPACE_GLOBAL in topic:
api_app/services/legacy_airlock_guard.py:43
- This comparison runs before template-schema validation. Because patch properties are untyped, a request such as
{"airlock_version": "2"}reaches"2" < 1, raisesTypeError, and returns a 500 rather than the expected validation 400. Validate that the value is an integer in{1, 2}before comparing it.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
airlock_processor/StatusChangedQueueTrigger/init.py:334
- When both the draft and sealed containers are missing, this falls through to
get_request_fileson the sealed name, which raisesResourceNotFoundError. The generic handler then retries/dead-letters the message, so the request remainsSubmittedand the newNoDataInRequestException/Failedpath is never reached. Check the sealed container here and raiseNoDataInRequestExceptionwhen neither exists.
if not blob_operations.container_exists(storage_account_name, container_name):
container_name = request_properties.request_id
airlock_processor/BlobCreatedTrigger/init.py:35
- The subject is still parsed on line 31 with
re.search(...).group(1)before this new v2 path runs. A malformed Event Grid subject therefore raises an opaqueAttributeErrorand bypasses the clearValueErroradded toget_blob_info_from_topic_and_subject. Parse the topic and subject through that helper before dispatching.
if constants.STORAGE_ACCOUNT_NAME_AIRLOCK_CORE in topic or constants.STORAGE_ACCOUNT_NAME_AIRLOCK_WORKSPACE_GLOBAL in topic:
_handle_v2_blob_created(json_body, topic, request_id, stepResultEvent, dataDeletionEvent)
return
api_app/_version.py:1
- The PR's component-version section lists API
0.27.27, while this change publishes0.27.28. Align the release version or the PR metadata so operators know which API artifact belongs to this change.
__version__ = "0.27.28"
- BlobCreatedTrigger: parse topic/subject via get_blob_info_from_topic_and_subject so a malformed Event Grid subject raises a clear ValueError, not an opaque AttributeError (drops now-unused import re). - get_request_files: raise NoDataInRequestException when neither the draft nor the sealed container exists, so a submission fails cleanly instead of ResourceNotFound retrying/dead-lettering while stuck in Submitted. - e2e airlock helpers: log only account/container path, omit the SAS query string (credential leak in test logs) for both upload and delete. - Bump airlock-processor 0.8.30 -> 0.8.31; CHANGELOG.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 88 out of 89 changed files in this pull request and generated no new comments.
Suppressed comments (1)
api_app/services/legacy_airlock_guard.py:36
ResourcePatch.propertiesis an untyped dictionary, so a request such as{"airlock_version":"2"}reaches the numeric comparison below and raisesTypeError, which is not caught by this route and becomes a 500 before template validation can return a client error. Validate that the supplied value is an integer in{1, 2}before comparing it with the current version.
There was a problem hiding this comment.
🟡 Changes recommended
Missing Terraform state moves and a sealed-container retry race can respectively disrupt upgrades and permit stale malware verdicts.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 88/89 changed files
- Comments generated: 3
- Review effort level: Balanced
…sign-airlock-storage-accounts # Conflicts: # CHANGELOG.md # api_app/_version.py # api_app/core/credentials.py
There was a problem hiding this comment.
🟡 Changes recommended
Copy timeout, migration concurrency, test coverage, and SAS logging issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
airlock_processor/shared_code/blob_operations.py:150
- The new five-minute polling limit is applied to every copy, including legacy stage copies and v2 cross-account approval copies whose completion is already signalled by BlobCreated. A valid large transfer that remains
pendingfor more than 300 seconds is now aborted and retried until dead-lettered, so large airlock requests can no longer complete. Limit synchronous waiting to the v2 draft-sealing path (where the source is deleted immediately), and leave event-driven boundary/legacy copies asynchronous.
while copy_status == "pending" and waited_seconds < COPY_TIMEOUT_SECONDS:
time.sleep(COPY_POLL_INTERVAL_SECONDS)
waited_seconds += COPY_POLL_INTERVAL_SECONDS
copy_status = copied_blob.get_blob_properties().copy.status
api_app/api/routes/workspaces.py:139
- This guard is a check-then-act race: a request can fetch the still-deployed v1 workspace and be created after this query returns empty but before
patch_workspacemarks the workspace as updating. The upgrade can then destroy that request's v1 storage despite the stated in-flight guard. Serialize request creation with the version transition (for example, atomically enter a migration state that request creation rejects, then recheck in-flight requests).
await ensure_airlock_version_change_allowed(workspace, resource_patch, airlock_request_repo)
ensure_workspace_airlock_version_supported({**workspace.properties, **(resource_patch.properties or {})}, default_version=1)
e2e_tests/test_airlock_consolidated.py:169
- This log includes the full SAS-bearing container URL. Redact the query string before logging it.
- Files reviewed: 89/90 changed files
- Comments generated: 4
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The legacy guard can be bypassed and failed submission copies cannot recover on retry.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
Previously missed (3) — in code that hasn't changed since the last review.
api_app/services/legacy_airlock_guard.py:19
- An empty properties object bypasses the legacy-airlock guard. The create-request route treats missing
enable_airlockas enabled and defaults a missing version to v1, so withENABLE_LEGACY_AIRLOCK=falsea workspace whose properties are{}can still create a v1 request that has no backing storage and will stall. Let the normal defaults below handle an empty dictionary instead of returning early.
airlock_processor/_version.py:1 - The PR description's component-version list still says airlock-processor
0.8.31, while this file and the changelog publish0.8.32. Update the PR description so release guidance identifies the actual artifact version.
api_app/_version.py:1 - The PR description's component-version list still says API
0.27.28, while this file and the changelog publish0.27.29. Update the PR description so the documented release version matches the artifact.
- Files reviewed: 89/90 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Airlock request creation can race with the version migration guard, allowing active v1 data to be destroyed during an upgrade.
Review details
Suppressed comments (1)
api_app/api/routes/workspaces.py:138
- The in-flight check is vulnerable to a TOCTOU race: after this query returns empty,
POST /requestscan still create and stamp a v1 request before the workspace patch is persisted/queued. The upgrade can then destroy the v1 workspace storage while that request is active, despite the migration guard. Please serialize request creation with the version change (for example, atomically mark the workspace as migrating so request creation is rejected, then recheck in-flight requests before committing the new version).
await ensure_airlock_version_change_allowed(workspace, resource_patch, airlock_request_repo)
- Files reviewed: 89/90 changed files
- Comments generated: 0 new
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
Submission redelivery can validate mutable draft metadata instead of the authoritative sealed data.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 89/90 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The processor currently has an import-blocking indentation error, and disabling v2 Airlock can orphan shared-account containers by destroying their signer.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 89/90 changed files
- Comments generated: 2
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The processor currently has a blocking syntax error, and null version patches can bypass the migration safety guard.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
airlock_processor/StatusChangedQueueTrigger/init.py:341
- This extra indentation leaves the function without an
if use_metadata:block, so importing the Function app fails immediately withIndentationErrorand no Airlock status messages can be processed. The legacy branch also needs to initializecontainer_namebefore the common call.
if use_metadata:
- Files reviewed: 90/91 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🟡 Changes recommended
The v2 Terraform module’s outdated environment dependency prevents deployment in Azure China.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 90/91 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
The security-sensitive storage, ABAC, identity, destructive migration, and asynchronous lifecycle changes require final human validation.
Review details
- Files reviewed: 90/91 changed files
- Comments generated: 1
- Review effort level: Balanced
| schemaVersion: 1.0.0 | ||
| name: tre-workspace-airlock-import-review | ||
| version: 0.16.1 | ||
| version: 0.17.1 |
There was a problem hiding this comment.
🟡 Changes recommended
Import-review workspace creation can still fail when legacy Airlock is disabled because its schema default is not persisted when omitted.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
templates/workspaces/airlock-import-review/template_schema.json:37
- This default is not applied by JSON Schema validation:
create_workspace_itempersists only request properties (with a special case forairlock_version), whileensure_workspace_airlock_version_supportedtreats an omittedenable_airlockastrue. Consequently, creating this review workspace through the API without explicitly sending the hidden field still fails when legacy Airlock is disabled, contrary to the intended fix. Resolve and persist the template'senable_airlockdefault before running the guard, and cover the omitted-property case.
- Files reviewed: 90/91 changed files
- Comments generated: 1
- Review effort level: Balanced
| schemaVersion: 1.0.0 | ||
| name: tre-workspace-base | ||
| version: 2.10.1 | ||
| version: 2.11.1 |
There was a problem hiding this comment.
🔵 Needs a closer look
The migration guard has a request-creation race, and unsupported Airlock versions are not consistently rejected.
Review details
Suppressed comments (3)
Previously missed (2) — in code that hasn't changed since the last review.
api_app/services/legacy_airlock_guard.py:23
- Despite the function's contract to reject unsupported versions, this value is only checked for the legacy-disabled case. Values such as
3,0,True, or"2"pass and can be stamped onto requests, where>= 2is silently treated as v2. Apply the same strict integer/1-or-2 validation used by the patch guard before checking legacy availability.
CHANGELOG.md:10 - This migration guidance still names bundle
2.11.0, but the manifest in this PR publishes2.11.1. Operators following the changelog could register the wrong bundle version and miss the fixes documented below; update this reference to2.11.1.
api_app/api/routes/workspaces.py:138
- This check is subject to a create-vs-upgrade race: a request can be created after the in-flight query but before the workspace patch/upgrade is established. That request is stamped with the old storage version while the upgrade can destroy that version's workspace storage. The version change needs an atomic workspace migration/lock state that request creation checks, rather than a standalone preflight query.
await ensure_airlock_version_change_allowed(workspace, resource_patch, airlock_request_repo)
- Files reviewed: 95/96 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Summary
Redesigns the Airlock storage architecture from per-stage storage accounts (v1) to a consolidated, metadata-based model (v2). In v2 each request lives in a single container whose stage is tracked by container metadata, so most transitions are metadata-only and workspaces share two storage accounts instead of ~10+ growing linearly per workspace.
The change is backwards compatible and opt-in per workspace: new workspaces default to v2, existing workspaces keep working on v1 and can migrate on their own schedule. Legacy v1 infrastructure is retained behind a core toggle so nothing is destroyed until an operator explicitly opts out.
Architecture
stalairlock{tre_id}(core stages: import-external/in-progress/rejected/blocked, export-approved) andstalairlockg{tre_id}(workspace stages: import-approved, export-internal/in-progress/rejected/blocked). Stage is held in containerstagemetadata.<request-id>-draft) is copied into a new immutable<request-id>container and the draft is deleted, which structurally revokes the researcher's upload SAS. The scan and review only ever see the sealed copy.BlobCreatedTrigger(V2_STAGE_COMPLETION_MAP).workspace_id+stage, so a workspace private endpoint (and any issued User-Delegation SAS) can only reach its own containers at the allowed stage.Versioning & migration
airlock_versionproperty (1= legacy per-stage,2= consolidated). New workspaces default to2.POST /migrationsstamps pre-v2 workspaces with an explicitairlock_version=1so a redeploy never silently migrates them (the bundle default is2).1 -> 2(patchairlock_version) is guarded: it is blocked while the workspace has in-flight requests (HTTP 400) and downgrades are rejected.tre-workspace-base2.11.0,tre-service-airlock-import-review0.17.0. Adopting v2 is then an explicitairlock_version=2patch.enable_legacy_airlocktoggle (defaulttrue; sample config setsfalse) keeps or removes the v1 core storage accounts. TheUSE_METADATA_STAGE_MANAGEMENTenv var is removed.Malware scanning
Submitted -> In-Reviewtransition; it is no longer turned into a status change directly (which previously stranded requests when the verdict arrived after sealing).ScanResultTriggerignores verdicts for the writable draft container (only the sealed copy's verdict is authoritative) and fails closed on a malformed verdict.Robustness / correctness
enable_legacy_airlock=false(clear 400 instead of a silent stall).Failed) from transient errors (re-raised for Service Bus retry).NoneTypecrashes).count+ state-preservingmovedblocks; spuriousmovedblocks on resources that were alreadycount-indexed onmainwere removed.Breaking changes / upgrade guidance
enable_legacy_airlock: trueexplicitly inconfig.yaml. It defaults totruetoday but will default tofalsein a future release. Setting it tofalsepermanently deletes the v1 core storage accounts and must only be done once noairlock_version=1workspaces or in-flight v1 requests remain.POST /migrations, then upgrade workspaces (in-place, minor) before opting any into v2. Upgrading a workspace1 -> 2deletes that workspace's v1 storage (completed-request files); request records/metadata are retained. See Legacy Airlock & migration.Testing
make test-e2e-airlockor the/test-airlockPR comment.enable_legacy_airlock=falsecore teardown, per-workspace DNS/signer isolation, ABAC boundary and exfil checks, malware block/allow, and sovereign issuer wiring.Component versions
api0.27.28 ·airlock-processor0.8.31 ·core0.18.9 ·tre-workspace-base2.11.0 ·tre-service-airlock-import-review0.17.0Known follow-ups
maxDeliveryCount) can strand a request inSubmitted/*InProgress. Planned: an outbox/reconciler that guarantees at-least-once delivery, re-drives stranded requests, and moves a genuinely poison message to a terminalFailedstate.Submittedrequest.Submitted -> Cancelledwas intentionally not enabled here because it races with the asynchronous submit pipeline; the reconciler above is the correct way to unstick such requests. If the transition is re-introduced later it must idempotently clean both the draft and sealed locations.module "airlock"/module "airlock_v2") would be cleaner, but is deferred to avoid additional state surgery on live storage.