Skip to content

Redesign airlock storage account architecture - #5048

Open
Marcus Robinson (marrobi) wants to merge 67 commits into
microsoft:mainfrom
marrobi:copilot/copilotredesign-airlock-storage-accounts
Open

Redesign airlock storage account architecture#5048
Marcus Robinson (marrobi) wants to merge 67 commits into
microsoft:mainfrom
marrobi:copilot/copilotredesign-airlock-storage-accounts

Conversation

@marrobi

@marrobi Marcus Robinson (marrobi) commented Aug 17, 2026

Copy link
Copy Markdown
Member

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

  • Consolidated storage — two core-managed accounts: stalairlock{tre_id} (core stages: import-external/in-progress/rejected/blocked, export-approved) and stalairlockg{tre_id} (workspace stages: import-approved, export-internal/in-progress/rejected/blocked). Stage is held in container stage metadata.
  • Immutability by sealing — on submit, the writable draft container (<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.
  • Single boundary copy — the only cross-account data movement is the core↔workspace copy on approval; its completion is signalled by BlobCreatedTrigger (V2_STAGE_COMPLETION_MAP).
  • ABAC — storage access is gated by Azure Attribute-Based Access Control on workspace_id + stage, so a workspace private endpoint (and any issued User-Delegation SAS) can only reach its own containers at the allowed stage.
  • Per-workspace SAS signer — each workspace gets its own Entra app registration used to mint scoped SAS. It is created and owned by TRE for every workspace, so both automatic- and manual-auth workspaces fully support v2.

Versioning & migration

  • New per-workspace airlock_version property (1 = legacy per-stage, 2 = consolidated). New workspaces default to 2.
  • POST /migrations stamps pre-v2 workspaces with an explicit airlock_version=1 so a redeploy never silently migrates them (the bundle default is 2).
  • Migrating a workspace 1 -> 2 (patch airlock_version) is guarded: it is blocked while the workspace has in-flight requests (HTTP 400) and downgrades are rejected.
  • Bundles are versioned as minor bumps so existing workspaces upgrade in place with no v2 infrastructure and no data movement: tre-workspace-base 2.11.0, tre-service-airlock-import-review 0.17.0. Adopting v2 is then an explicit airlock_version=2 patch.
  • Core enable_legacy_airlock toggle (default true; sample config sets false) keeps or removes the v1 core storage accounts. The USE_METADATA_STAGE_MANAGEMENT env var is removed.

Malware scanning

  • Uses Microsoft Defender for Storage on-upload scanning. The verdict is recorded as a fact on the request and gates the Submitted -> In-Review transition; it is no longer turned into a status change directly (which previously stranded requests when the verdict arrived after sealing).
  • ScanResultTrigger ignores verdicts for the writable draft container (only the sealed copy's verdict is authoritative) and fails closed on a malformed verdict.

Robustness / correctness

  • Sovereign-cloud support: workload-identity token-exchange audience and the signer issuer are derived from the AAD environment.
  • Request creation is rejected on a v1 workspace when enable_legacy_airlock=false (clear 400 instead of a silent stall).
  • Processor distinguishes deterministic validation failures (no/too-many/missing files -> Failed) from transient errors (re-raised for Service Bus retry).
  • Clear errors when an Event Grid topic/subject or blob URL can't be parsed (no more opaque NoneType crashes).
  • Terraform: v1 resources are made conditional with count + state-preserving moved blocks; spurious moved blocks on resources that were already count-indexed on main were removed.

Breaking changes / upgrade guidance

  • Set enable_legacy_airlock: true explicitly in config.yaml. It defaults to true today but will default to false in a future release. Setting it to false permanently deletes the v1 core storage accounts and must only be done once no airlock_version=1 workspaces or in-flight v1 requests remain.
  • After upgrading the API, run POST /migrations, then upgrade workspaces (in-place, minor) before opting any into v2. Upgrading a workspace 1 -> 2 deletes that workspace's v1 storage (completed-request files); request records/metadata are retained. See Legacy Airlock & migration.

Testing

  • ~849 API + 110 airlock-processor unit tests; new E2E airlock coverage (draft seal, file-count validation, rejected/cancelled lifecycles, cross-workspace access) runnable via make test-e2e-airlock or the /test-airlock PR comment.
  • Extensive live validation on a TRE: v1 and v2 happy paths (import/export approve/reject/block/cancel), v1->v2 migration (v1 storage destroyed, request metadata retained), in-flight migration guard, enable_legacy_airlock=false core teardown, per-workspace DNS/signer isolation, ABAC boundary and exfil checks, malware block/allow, and sovereign issuer wiring.

Component versions

api 0.27.28 · airlock-processor 0.8.31 · core 0.18.9 · tre-workspace-base 2.11.0 · tre-service-airlock-import-review 0.17.0

Known follow-ups

  • Transactional outbox / reconciler. The status update writes to Cosmos and publishes the Event Grid event non-atomically, so a crash between the two (or a poison message that dead-letters after maxDeliveryCount) can strand a request in Submitted/*InProgress. Planned: an outbox/reconciler that guarantees at-least-once delivery, re-drives stranded requests, and moves a genuinely poison message to a terminal Failed state.
  • Cancel of a stranded Submitted request. Submitted -> Cancelled was 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.
  • Cleanup durability & data lifecycle. Workspace-deletion container cleanup runs best-effort (now including cancelled requests); making it run after a successful uninstall and retryable, plus a retention/lifecycle policy and a recovery path for rejected/blocked data, are deferred.
  • Core v1 module extraction (tidy-up). The core v1 resources are conditioned per-resource; extracting them into a nested module (as the workspace bundle already does with module "airlock" / module "airlock_v2") would be cleaner, but is deferred to avoid additional state surgery on live storage.

Squashed 61 commits into a single commit for a clean PR.
@marrobi
Marcus Robinson (marrobi) requested a balanced review from Copilot August 17, 2026 23:08
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown

Unit Test Results

950 tests   950 ✅  12s ⏱️
  2 suites    0 💤
  2 files      0 ❌

Results for commit 252486a.

♻️ This comment has been updated with latest results.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread templates/workspaces/base/terraform/airlock_v2/storage_accounts.tf
Comment thread templates/workspaces/base/terraform/workspace.tf
Comment thread templates/workspaces/base/porter.yaml
Comment thread airlock_processor/StatusChangedQueueTrigger/__init__.py Outdated
Comment thread airlock_processor/StatusChangedQueueTrigger/__init__.py Outdated
Comment thread airlock_processor/BlobCreatedTrigger/__init__.py Outdated
Comment thread core/terraform/airlock/storage_accounts.tf Outdated
Comment thread docs/azure-tre-overview/airlock.md Outdated
… 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_command runs with USE_ENV_VARS_NOT_FILES=true and injects TF_VAR_enable_airlock_malware_scanning, but has no input or TF_VAR_enable_legacy_airlock; hosted deployments therefore always use Terraform's default true and 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.

Comment thread api_app/api/routes/migrations.py
Comment thread api_app/api/routes/airlock.py Outdated
Comment thread airlock_processor/ScanResultTrigger/__init__.py Outdated
…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.
@marrobi

Copy link
Copy Markdown
Member Author

Also addressed the three suppressed review comments in 6e7909e:

  • DNS (templates/workspaces/base/terraform/airlock_v2/storage_accounts.tf): each workspace now gets a more-qualified private DNS zone (stalairlockg<tre_id>.privatelink.blob.core.windows.net) linked only to its own VNet, with a manual apex A record pointing at that workspace's private endpoint IP. Azure resolves via the most-specific linked zone, so the shared global account resolves to each workspace's own endpoint (matching the ABAC private-endpoint condition) instead of colliding as multiple/last-writer-wins A records in the shared core blob zone. Verified on a live env that the shared zone previously held a single record for the account shared by the core processor and workspace endpoints. This mirrors the existing manual-A-record pattern already used by the import-review workspace.

  • config_schema.json / GitHub Actions: added an ENABLE_LEGACY_AIRLOCK input and -e TF_VAR_enable_legacy_airlock mapping to the devcontainer_run_command action so the toggle is honoured on the hosted deployment path (previously only TF_VAR_enable_airlock_malware_scanning was threaded, so hosted deploys always used Terraform's default).

  • docs/azure-tre-overview/airlock-legacy.md: corrected — airlock_version: 1 is an explicit legacy opt-in; new workspaces default to 2.

The two still-open threads are intentionally left for follow-up: the v1→v2 count = 0 data-loss concern on workspace.tf, and the version-aware review-workspace connectivity on import_review_resources.terraform.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread core/terraform/airlock/storage_accounts.tf Outdated
Comment thread core/terraform/api-webapp.tf Outdated
Comment thread airlock_processor/ScanResultTrigger/__init__.py Outdated
Comment thread airlock_processor/StatusChangedQueueTrigger/__init__.py Outdated
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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 for stalimip${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 Submitted before the asynchronous submitted event enumerates files. That enumeration emits completed_step="submitted", but AirlockStatusUpdater now rejects it because the request is already InReview/BlockingInProgress, so request_files is 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 reaches InReview without persisting request_files. Accumulate both events and set an Out[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_files only 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 to import-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 /migrations endpoint is available and treats missing versions as v1. The deployment workflow waits for API health before calling db-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()

Comment thread api_app/services/legacy_airlock_guard.py Outdated
Comment thread core/terraform/airlock/storage_accounts.tf Outdated
…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).
@marrobi

Copy link
Copy Markdown
Member Author

Also addressed the suppressed comments from this review:

  • airlock.py:106 / StatusChangedQueueTrigger:132 (file persistence)request_files is no longer lost when a request advances to in_review on submission: the scanning-disabled path carries the enumerated files on the single in_review StepResult, and the API status consumer now persists a file-enumeration result even when the request has already advanced (rather than dead-lettering it).
  • StatusChangedQueueTrigger:104 (one-file invariant) — v2 submit now rejects zero or multiple files before moving the container to import-in-progress (the metadata-only submit no longer copies via copy_data, which used to enforce this).
  • workspaces.py:136 (patch validation)patch_workspace now validates the merged workspace properties against ensure_workspace_airlock_version_supported, so a workspace can't be switched to v1 while enable_legacy_airlock=false.
  • main.py:32 (startup guard deadlock) — the startup guard now runs the airlock_version backfill in-process before the block check, so it evaluates real versions and no longer depends on the external db-migrate call (blocking behaviour retained).
  • import_review_resources.terraform:20 (v1 review connectivity) — already implemented earlier (19609d4f4): count-gated private endpoint + DNS to the legacy stalimip account behind enable_legacy_airlock.

Commits: 8d5c8f7aa (startup-guard backfill) and d1751f5ee (the rest).

@marrobi
Marcus Robinson (marrobi) requested a balanced review from Copilot August 18, 2026 15:51

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 88 out of 89 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

api_app/api/routes/workspaces.py:181

  • Queueing the uninstall before cleanup creates a race with the resource processor: it can destroy the workspace signer and its conditioned role assignment while these shared-account deletions are still running. The cleanup helper also catches every deletion failure and returns successfully, so a transient failure leaves containers in the shared account with no retry path after the signer is removed. Cleanup must be a successful precondition of queueing uninstall, or be moved to a durable worker that retries and retains credentials until all containers are removed.
        # Shared storage outlives the workspace, so remove its containers once uninstall is queued.
        await delete_workspace_airlock_containers(workspace, airlock_request_repo)

Comment thread airlock_processor/StatusChangedQueueTrigger/__init__.py
Copilot AI review requested due to automatic review settings August 20, 2026 17:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 88 out of 89 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

core/terraform/airlock/storage_accounts_v1.tf:434

  • api_sa_data_contributor also had count before this change, so its state is already at indexed addresses. Moving an unindexed resource to [0] is invalid/loses the remaining instances; no move is required when only relocating the resource block between files.
    api_app/api/routes/workspaces.py:181
  • Workspace deletion now waits for every request to be deleted from two accounts under both possible container names. This is an unbounded serial sequence of network calls (four per historical request), so a workspace with enough Airlock history can exceed the API request timeout after uninstall has already been queued, leaving the caller without the operation response. Move cleanup into the asynchronous uninstall/resource-processing path, or enqueue a separate cleanup operation rather than awaiting it in the HTTP handler.
        # 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

  • When both the draft and sealed containers are absent, this fallback calls list_blobs on the nonexistent sealed container before handle_status_changed can raise NoDataInRequestException. The generic exception path then retries/dead-letters the message and leaves the request stuck in Submitted instead of transitioning it to Failed. Check the sealed container here and raise NoDataInRequestException when neither exists.
        if not blob_operations.container_exists(storage_account_name, container_name):
            container_name = request_properties.request_id

airlock_processor/StatusChangedQueueTrigger/init.py:94

  • This comment contradicts the implementation below: v2 submission copies the mutable draft container into the sealed request container before deleting the draft. Describe the sealing copy so future changes do not incorrectly assume submission is metadata-only.
        # v2 submit does not copy, so enforce the single-file rule explicitly.

docs/azure-tre-overview/airlock-legacy.md:104

  • The v2 implementation performs a same-account draft-to-sealed copy on submission and another cross-account copy on approval, so an approved request can perform two data copies. Reporting only one copy understates both transfer time and storage operations; update the comparison accordingly.

Comment thread core/terraform/airlock/storage_accounts_v1.tf Outdated
Copilot AI review requested due to automatic review settings August 20, 2026 18:31

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 88 out of 89 changed files in this pull request and generated 1 comment.

Comment thread api_app/db/repositories/airlock_requests.py Outdated
Copilot AI review requested due to automatic review settings August 20, 2026 18:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 88 out of 89 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

api_app/db/repositories/airlock_requests.py:109

  • Allowing Submitted -> Cancelled introduces an ordering race in the storage workflow. Status-change events are delivered through a non-session Service Bus queue, so the cancellation handler can run before the submitted handler has sealed the draft; it then deletes a not-yet-created sealed container, after which the delayed submit event creates it while the API discards its result because the request is already final. This leaves cancelled request data orphaned. The transition needs ordered/per-request processing or cancellation handling that remains effective against a later submit.
    api_app/api/routes/workspaces.py:181
  • Cleanup failures are swallowed, but the uninstall has already been queued and will destroy the workspace signer and its conditioned role assignment. Any global-account container that failed deletion can therefore remain indefinitely with no later retry path tied to the workspace. Complete/retry cleanup before queuing uninstall, or move it to a durable operation that can fail the deletion instead of proceeding after partial cleanup.
        # Shared storage outlives the workspace, so remove its containers once uninstall is queued.
        await delete_workspace_airlock_containers(workspace, airlock_request_repo)

Comment thread templates/workspaces/airlock-import-review/porter.yaml Outdated
Copilot AI review requested due to automatic review settings August 20, 2026 18:58
…-from-submitted, v2 approval completion via BlobCreatedTrigger, in-place minor upgrade

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 88 out of 89 changed files in this pull request and generated no new comments.

Suppressed comments (6)

core/terraform/airlock/storage_accounts_v1.tf:430

  • These two role-assignment resources already used count before this change, so their state addresses were already indexed (...[0], ...[1], etc.). Moving the whole resource address to [0] is therefore not a valid singleton-to-count migration and can make Terraform reject the move or collapse multiple existing assignments onto one address. Keep the existing indexed state addresses by removing both role-assignment moved blocks; the singleton resources above still need their moves.
    templates/workspaces/airlock-import-review/porter.yaml:4
  • This changes the template from 0.16.1 to 1.6.2, which is a breaking major-version jump and will require forced upgrades for existing review workspaces. The added backwards-compatible connectivity is new functionality, so the repository's semantic-versioning rules call for a minor bump (for example, 0.17.0); 1.6.2 also appears to have been copied from the firewall bundle.
    airlock_processor/shared_code/blob_operations.py:130
  • The new five-minute deadline is now applied to every Airlock copy, including legacy transitions and cross-account approval copies that previously completed asynchronously via BlobCreated. A legitimate large research blob that takes longer than 300 seconds will now be aborted and retried indefinitely, preventing the request from progressing. Limit synchronous polling to the v2 draft-sealing path where the source is deleted immediately, and retain event-driven completion for other copies (or use a service-supported long-running copy timeout).
    # 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:

api_app/api/routes/workspaces.py:181

  • The shared containers are deleted immediately after merely queuing the uninstall. If the resource processor later fails, dead-letters, or never receives that uninstall, the workspace remains deployed but its Airlock data has already been irreversibly removed. Defer this cleanup until the workspace uninstall succeeds, with retryable cleanup handling, rather than performing it in the request path.
        # Shared storage outlives the workspace, so remove its containers once uninstall is queued.
        await delete_workspace_airlock_containers(workspace, airlock_request_repo)

.github/dependabot.yml:62

  • The PR description says this adds granular Dockerfile globbing, but the configuration still uses the unchanged repository-wide "**/*" directory. This hunk only edits the comment and does not implement the described Dependabot behavior; either add the intended granular directories or correct the PR description.
  # The Docker manager includes Dockerfile.tmpl files in this repository-wide glob.
  - package-ecosystem: "docker"
    directories:
      - "**/*"

airlock_processor/_version.py:1

  • The PR description says the Airlock processor is updated to 0.8.13, while this file publishes 0.8.30. Align the description and release/changelog metadata with the version that is actually intended so operators do not register or diagnose the wrong component version.
__version__ = "0.8.30"

Copilot AI review requested due to automatic review settings August 20, 2026 19:06

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 88 out of 89 changed files in this pull request and generated no new comments.

Suppressed comments (4)

templates/workspaces/airlock-import-review/porter.yaml:4

  • This jumps the bundle from 0.16.1 to 1.6.2, which is also the unrelated firewall bundle's current version. The added backward-compatible connectivity is a minor feature, so this appears to be a copied version and would publish the review workspace under an unintended major version.
    api_app/api/routes/workspaces.py:181
  • The uninstall message is dispatched before shared-container cleanup starts. The workspace uninstall destroys the per-workspace signer application and its conditioned role assignment, but cleanup needs that signer to delete containers from the global account; processing can therefore race cleanup and leave containers orphaned, especially for workspaces with many requests. Perform cleanup through an identity that outlives the workspace or make it an ordered uninstall step before destroying the signer.
        # 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

  • When neither the draft nor sealed container exists, this fallback selects the sealed name without checking it. The following list_blobs() then raises ResourceNotFoundError before handle_status_changed() can raise NoDataInRequestException, so the message retries/dead-letters and the request remains Submitted instead of becoming Failed. Check the sealed container here and raise the deterministic no-data exception when both are absent.
        if not blob_operations.container_exists(storage_account_name, container_name):
            container_name = request_properties.request_id

docs/azure-tre-overview/airlock-legacy.md:14

  • This says the signer needs no role assignments, but airlock_v2/storage_accounts.tf assigns its service principal Storage Blob Data Contributor with the workspace ABAC condition. Clarify that no directory-role assignment or consent is needed; Azure RBAC is still required and provisioned by Terraform.

Copilot AI review requested due to automatic review settings August 20, 2026 19:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 88 out of 89 changed files in this pull request and generated no new comments.

Suppressed comments (5)

api_app/api/routes/workspaces.py:138

  • This guard and the workspace patch are not atomic. After the query reports no in-flight requests, a concurrent create-request call can still read the v1 workspace and create a v1 request before the patch switches it to v2; the deployment can then destroy the request's legacy storage. Introduce a persisted migration/lock state that request creation checks, or otherwise serialize request creation with the version transition.
        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)

api_app/services/airlock.py:190

  • Deletion failures are only logged, but the uninstall remains queued and there is no durable retry. Because these containers live in shared accounts that outlive the workspace, any transient Azure/DNS/credential failure leaves sensitive workspace data orphaned indefinitely. Make cleanup a durable operation with retries and prevent deletion from being finalized until cleanup succeeds.
    templates/workspaces/airlock-import-review/porter.yaml:4
  • This jumps the bundle from 0.16.1 to 1.6.2, even though the change adds backward-compatible functionality. That is an unintended major-version jump and conflicts with this repository's semantic-versioning rule; use the next minor version for this enhancement.
    airlock_processor/BlobCreatedTrigger/init.py:35
  • This check searches the entire Event Grid topic, including the resource group and TRE-ID suffix. A legacy account is therefore misclassified as v2 whenever the configured tre_id contains stalairlock; it then enters the metadata path instead of legacy processing. Parse the storage account once and match its prefix, which also uses the new parser's explicit malformed-subject error.
    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

airlock_processor/StatusChangedQueueTrigger/init.py:334

  • The fallback assumes the sealed container exists. If both draft and sealed containers are absent, list_blobs() raises before handle_status_changed() can raise the new NoDataInRequestException, so the message retries/dead-letters and the request remains Submitted instead of receiving the intended failure status. Check the sealed container and raise NoDataInRequestException here.
        # On a redelivery the draft is already sealed away, so enumerate the submitted copy instead.
        if not blob_operations.container_exists(storage_account_name, container_name):
            container_name = request_properties.request_id

Copilot AI review requested due to automatic review settings August 20, 2026 19:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pending longer 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_containers suppresses 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_files then raises ResourceNotFoundError. That bypasses the new NoDataInRequestException handling, so the message is retried/dead-lettered instead of moving the request to Failed with the intended diagnostic. Check the sealed container too and raise NoDataInRequestException when 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 uncaught TypeError, 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 AttributeError before the hardened parser can produce the clear ValueError promised by this change. Parse the topic and subject through get_blob_info_from_topic_and_subject 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)

Copilot AI review requested due to automatic review settings August 20, 2026 19:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_id with an unchecked regex before calling the new safe parser. A malformed Event Grid subject therefore raises AttributeError from .group(1) instead of the clear ValueError this PR intends. Parse the topic and subject once with get_blob_info_from_topic_and_subject before 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, raises TypeError, and returns a 500 rather than the expected validation 400. Validate that the value is an integer in {1, 2} before comparing it.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_files on the sealed name, which raises ResourceNotFoundError. The generic handler then retries/dead-letters the message, so the request remains Submitted and the new NoDataInRequestException/Failed path is never reached. Check the sealed container here and raise NoDataInRequestException when 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 opaque AttributeError and bypasses the clear ValueError added to get_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 publishes 0.27.28. Align the release version or the PR metadata so operators know which API artifact belongs to this change.
__version__ = "0.27.28"

Comment thread e2e_tests/airlock/request.py Outdated
- 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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.properties is an untyped dictionary, so a request such as {"airlock_version":"2"} reaches the numeric comparison below and raises TypeError, 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Redesign airlock to reduce number of storage accounts used

2 participants