Redesign airlock storage account architecture#4964
Conversation
|
Review from @marrobi which needs addressing: Review of remaining files — issues foundTerraform
API / config
Docs
mkdocs
E2E
Housekeeping
|
There was a problem hiding this comment.
Pull request overview
This PR implements the “airlock v2” consolidated storage design (2 shared storage accounts + metadata-based stage tracking), while keeping legacy v1 behavior available via per-workspace airlock_version and a core-level enable_legacy_airlock toggle/guardrails.
Changes:
- Adds
airlock_versionworkspace property and deploys either legacy (airlock) or consolidated (airlock_v2) workspace airlock Terraform. - Updates core Airlock Terraform to introduce consolidated storage accounts, unify EventGrid blob-created handling, and gate legacy resources behind
enable_legacy_airlock. - Extends API + airlock processor to carry
airlock_version/review_workspace_idthrough events and support v1/v2 execution paths; adds/updates tests and docs.
Reviewed changes
Copilot reviewed 65 out of 67 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| templates/workspaces/base/terraform/workspace.tf | Switches workspace airlock module selection based on airlock_version and adds v2 module. |
| templates/workspaces/base/terraform/variables.tf | Introduces airlock_version and marks legacy malware-scanning variables as unused in workspace TF. |
| templates/workspaces/base/terraform/airlock_v2/variables.tf | Declares inputs for the new consolidated workspace airlock module. |
| templates/workspaces/base/terraform/airlock_v2/storage_accounts.tf | Adds PE + ABAC-conditioned RBAC for workspace access to shared workspace-global storage. |
| templates/workspaces/base/terraform/airlock_v2/providers.tf | Defines provider requirements/aliases for the new module. |
| templates/workspaces/base/terraform/airlock_v2/locals.tf | Computes core RG + shared workspace-global storage account name. |
| templates/workspaces/base/terraform/airlock_v2/data.tf | Reads core identity + core private DNS zone for use by workspace module. |
| templates/workspaces/base/template_schema.json | Adds updateable airlock_version property to workspace schema. |
| templates/workspaces/base/porter.yaml | Bumps base workspace bundle version and wires airlock_version parameter into TF. |
| templates/workspaces/airlock-import-review/terraform/import_review_resources.terraform | Updates import-review workspace to support v1/v2 storage access patterns with conditional resources. |
| templates/workspaces/airlock-import-review/template_schema.json | Adds airlock_version parameter to the import-review workspace template schema. |
| templates/workspaces/airlock-import-review/porter.yaml | Bumps import-review bundle version and wires airlock_version into TF. |
| mkdocs.yml | Updates MkDocs navigation to include legacy airlock + guardrails docs. |
| e2e_tests/test_airlock.py | Removes the old “full flow” airlock test and keeps review-VM focused test. |
| e2e_tests/test_airlock_consolidated.py | Adds new E2E tests validating consolidated (v2) airlock flows and storage routing. |
| e2e_tests/resources/workspace.py | Minor string formatting normalization in identifier URI helper. |
| e2e_tests/pytest.ini | Adds a marker for consolidated airlock tests. |
| e2e_tests/conftest.py | Adjusts workspace setup fixture to support Manual/Automatic auth based on config. |
| docs/tre-admins/legacy-airlock-migration-guardrails.md | Adds an operator runbook for safely disabling legacy v1 airlock. |
| docs/azure-tre-overview/airlock-legacy.md | Adds documentation describing the legacy v1 per-stage storage architecture. |
| core/version.txt | Bumps core component version. |
| core/terraform/variables.tf | Adds enable_legacy_airlock + acknowledgement/blocking guardrail variables. |
| core/terraform/main.tf | Passes enable_legacy_airlock into airlock module and tweaks dependencies. |
| core/terraform/appgateway/appgateway.tf | Adjusts lifecycle ignores for App Gateway public IP tags. |
| core/terraform/api-webapp.tf | Plumbs legacy-airlock guardrail settings into API app settings. |
| core/terraform/airlock/variables.tf | Adds enable_legacy_airlock variable in airlock module. |
| core/terraform/airlock/storage_accounts.tf | Introduces consolidated storage accounts and updated PE/RBAC/EventGrid wiring. |
| core/terraform/airlock/storage_accounts_v1.tf | Adds legacy v1 storage accounts behind enable_legacy_airlock. |
| core/terraform/airlock/locals.tf | Adds consolidated names and reintroduces v1 names conditionally for legacy mode. |
| core/terraform/airlock/identity.tf | Removes legacy role assignments from this file (moved to v1-specific file). |
| core/terraform/airlock/eventgrid_topics.tf | Unifies blob-created subscriptions and adds workspace-global blob-created plumbing; keeps legacy conditionally. |
| core/terraform/airlock/eventgrid_topics_v1.tf | Adds legacy v1 EventGrid system topics/subscriptions behind enable_legacy_airlock. |
| core/terraform/airlock/data.tf | Points diagnostic categories lookup at the new consolidated system topic. |
| config.sample.yaml | Documents and adds new legacy-airlock config toggles. |
| config_schema.json | Adds schema entries for legacy-airlock toggles. |
| CHANGELOG.md | Adds unreleased changelog entry for airlock migration + related template fix. |
| api_app/tests_ma/test_services/test_legacy_airlock_guard.py | Adds unit tests for the new API startup guard. |
| api_app/tests_ma/test_services/test_airlock.py | Adds tests for public-stage detection and status-changed event payload additions. |
| api_app/tests_ma/test_services/test_airlock_storage_helper.py | Adds tests for API storage-account/stage mapping logic (v1/v2). |
| api_app/services/legacy_airlock_guard.py | Implements API startup guardrails for disabling legacy airlock safely. |
| api_app/services/airlock.py | Adds v2 storage resolution path and updates status-changed event sending. |
| api_app/services/airlock_storage_helper.py | Adds API helper to map request type/status/version to storage account + metadata stage. |
| api_app/resources/constants.py | Adds consolidated storage/stage constants alongside legacy constants. |
| api_app/models/domain/events.py | Extends StatusChangedData with review_workspace_id and airlock_version. |
| api_app/models/domain/airlock_request.py | Adds airlock_version field to AirlockRequest domain model. |
| api_app/main.py | Runs legacy-airlock migration guard during API startup lifespan. |
| api_app/event_grid/event_sender.py | Includes review_workspace_id + airlock_version in emitted StatusChanged events and adjusts workspace id semantics for v2. |
| api_app/db/repositories/workspaces.py | Adds query helper to detect active v1 workspaces. |
| api_app/db/repositories/airlock_requests.py | Adds “in-flight v1 requests” query for guardrails and stores airlock_version on request creation. |
| api_app/core/config.py | Adds env-driven config flags for legacy airlock enable/block behavior. |
| api_app/api/routes/api.py | Removes redundant global openapi_definitions lines. |
| api_app/api/routes/airlock.py | Stamps new airlock requests with workspace airlock_version. |
| api_app/_version.py | Bumps API version. |
| airlock_processor/tests/test_status_change_queue_trigger.py | Extends tests for v1/v2 branching, review workspace id handling, and v2 copy/metadata transitions. |
| airlock_processor/tests/test_blob_created_trigger.py | Adds tests for v2 BlobCreated processing based on container metadata stage. |
| airlock_processor/tests/shared_code/test_blob_operations.py | Adds tests for new blob-copy polling helper. |
| airlock_processor/tests/shared_code/test_blob_operations_metadata.py | Adds comprehensive tests for metadata-based container operations. |
| airlock_processor/tests/shared_code/test_airlock_storage_helper.py | Adds tests for processor storage-account/stage mapping logic (v1/v2). |
| airlock_processor/StatusChangedQueueTrigger/init.py | Implements v1/v2 branching: metadata stage updates for v2 and legacy copy behavior for v1. |
| airlock_processor/shared_code/constants.py | Adds consolidated storage/stage constants for processor side. |
| airlock_processor/shared_code/blob_operations.py | Adds copy result return + blob-copy completion polling helper. |
| airlock_processor/shared_code/blob_operations_metadata.py | Adds metadata-based container create/update/read/delete helpers for v2. |
| airlock_processor/shared_code/airlock_storage_helper.py | Adds processor helper to map request type/status/version to storage account + metadata stage. |
| airlock_processor/BlobCreatedTrigger/init.py | Adds v2 BlobCreated handling based on container metadata stage. |
| airlock_processor/_version.py | Bumps airlock processor version. |
| .gitignore | Ignores *_old.tf files. |
| # Return standard blob storage URL format | ||
| return "https://{}.blob.{}/{}?{}" \ | ||
| .format(account_name, STORAGE_ENDPOINT, airlock_request.id, token) |
Unit Test Results901 tests 901 ✅ 9s ⏱️ Results for commit 878ea01. ♻️ This comment has been updated with latest results. |
rudolphjacksonm
changed the title
…t#4964) - Add Terraform moved blocks for safe storage account state migration (sa_import_external → sa_airlock_core, sa_import_in_progress → sa_airlock_workspace_global) - Fix storage helper to raise ValueError for unmapped airlock request statuses instead of returning None - Change guard default to BLOCK_DISABLE_LEGACY_AIRLOCK_IF_V1_EXISTS=true to prevent data loss on misconfiguration - Register airlock_consolidated pytest marker and apply to consolidated test suite - Validate EventGrid diagnostic_setting for_each stability with flag toggles - Verify docs reference full PE resource IDs in ABAC conditions (not just names) - Confirm App Gateway routing references removed from documentation - All 66 storage helper tests passing with enhanced error coverage - Terraform validation passes Item #12 (unrelated whitespace/formatting churn) deferred to separate PR to keep this PR focused.
|
@rudolphjacksonm might be able to get an agent to test this from local machine, test the import and export paths using runcommamnd on the various VMs. Would eb good to extend the e2e airlock tests (albeit not run them on each PR!) |
|
original test plan: #4853 (comment) |
96b5dfa to
00f2184
Compare
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/29574605411 (with refid (in response to this comment from @rudolphjacksonm) |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 67 changed files in this pull request and generated 4 comments.
Files not reviewed (1)
- core/terraform/airlock/.terraform.lock.hcl: Generated file
Comments suppressed due to low confidence (1)
docs/azure-tre-overview/airlock-legacy.md:10
- This sentence states
airlock_version: 1is "the default", but the workspace base template defaultsairlock_versionto 2 (seetemplates/workspaces/base/terraform/variables.tf). This is misleading for new deployments and migration guidance.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 67 changed files in this pull request and generated 2 comments.
Files not reviewed (1)
- core/terraform/airlock/.terraform.lock.hcl: Generated file
Comments suppressed due to low confidence (4)
docs/azure-tre-overview/airlock-legacy.md:10
- This doc claims
airlock_version: 1is the default, but the base workspace template now defaultsairlock_versionto 2 (e.g., templates/workspaces/base/terraform/variables.tf sets default=2). This can mislead operators during migration/rollout.
core/terraform/airlock/storage_accounts.tf:13 - Same issue as the previous
movedblock: mapping legacysa_import_in_progressto consolidatedsa_airlock_workspace_globalwill churn the existing in-progress account (stalimip{tre_id}) into a different-named resource (stalairlockg{tre_id}) and then recreate the legacy account atsa_import_in_progress[0]. This is destructive for upgrades; move should target the new counted legacy address instead.
devops/scripts/set_docker_sock_permission.sh:47 groupaddrequires root privileges, but this script is sourced from the Makefile (not run under sudo). Without sudo here, the permission fix will fail in non-root environments.
devops/scripts/set_docker_sock_permission.sh:51usermod/chmodrequire elevated privileges; when this script is sourced from the Makefile without sudo, these commands will fail. Prefix with sudo to preserve the prior behavior.
- Add provider = azurerm.core to data source and role assignment in airlock_v2/storage_accounts.tf to fix split-subscription deployments - Fix @finalStatuses enum serialization: convert to string values for Cosmos SDK JSON compatibility in airlock_requests.py - Fix incorrect 'default' claim in airlock-legacy.md (default is v2) - Add missing PR reference to CHANGELOG.md bug fix entry - Remove inaccurate 'via App GW/SAS' comments in airlock.py and update SAS URL comment to accurately describe direct storage account URLs
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/30008586146 (with refid (in response to this comment from @rudolphjacksonm) |
| - name: airlock_version | ||
| type: integer | ||
| default: 2 | ||
| description: "Airlock storage version: 1 = legacy per-stage storage accounts (stalimip{tre_id}), 2 = consolidated metadata-based storage (stalairlock{tre_id})" |
…ithub.com/marrobi/AzureTRE into copilot/redesign-airlock-storage-accounts
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/30014711207 (with refid (in response to this comment from @rudolphjacksonm) |
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/30015039317 (with refid (in response to this comment from @rudolphjacksonm) |
|
/test-extended |
|
🤖 pr-bot 🤖 🏃 Running extended tests: https://github.com/microsoft/AzureTRE/actions/runs/30015552628 (with refid (in response to this comment from @rudolphjacksonm) |
…4964 # Conflicts: # CHANGELOG.md
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 68 changed files in this pull request and generated 3 comments.
Files not reviewed (1)
- core/terraform/airlock/.terraform.lock.hcl: Generated file
Comments suppressed due to low confidence (2)
templates/workspaces/base/terraform/variables.tf:191
- This description says the scan result topic is "no longer used" after consolidation, but it is still used for legacy v1 airlock deployments. Please clarify so operators understand when this input is required.
api_app/services/airlock.py:145 - SAS URL generation currently always returns the direct storage account URL (
https://{account}.blob.{suffix}/...). The PR description states that publicly accessible stages should use the App Gateway FQDN/path (e.g.,/airlock-storage/*) for SAS URLs. If that routing is still required, this function needs a way to switch the host for public stages (likely usingis_publicly_accessible_stage+ a configured gateway hostname).
# Return standard blob storage URL format
return "https://{}.blob.{}/{}?{}" \
.format(account_name, STORAGE_ENDPOINT, airlock_request.id, token)
def get_account_url(account_name: str) -> str:
return f"https://{account_name}.blob.{STORAGE_ENDPOINT}/"
| type = bool | ||
| default = false | ||
| description = "Enable Airlock malware scanning for the workspace" | ||
| description = "Enable Airlock malware scanning for the workspace. Passed by porter bundle but no longer used in workspace terraform after airlock consolidation." |
| @Environment[Microsoft.Network/privateEndpoints] StringEqualsIgnoreCase | ||
| '${azurerm_private_endpoint.sa_airlock_core_pe[0].id}' | ||
| AND | ||
| @Resource[Microsoft.Storage/storageAccounts/blobServices/containers/metadata:stage] | ||
| StringEquals 'import-in-progress' |
| def _handle_v2_blob_created(json_body, topic, request_id, stepResultEvent, dataDeletionEvent): | ||
| """Handle BlobCreated events from v2 consolidated storage accounts. | ||
|
|
||
| In v2, cross-account copies (e.g., import approval: core → workspace-global) | ||
| fire BlobCreated events. Container metadata determines the stage and appropriate | ||
| step result, matching the v1 pattern where BlobCreatedTrigger signals copy completion. |
Resolves #4358
Note: Original pull request and comments are #4853.
Consolidate airlock storage architecture and implement metadata-based management
Summary
Redesigns the airlock storage architecture from 56+ per-workspace storage accounts (5 core + ~5 per workspace) down to 2 shared storage accounts, using container metadata to track request stages instead of physically copying blobs between accounts.
A new per-workspace
airlock_versionproperty (1= legacy,2= consolidated) enables a gradual, backwards-compatible migration path.Problem
The existing airlock design creates 5–10 storage accounts per workspace, each with private endpoints, EventGrid topics, and role assignments. At scale (100 workspaces) this results in:
Changes
Core Terraform
sa_airlock_core(external-facing) andsa_airlock_workspace_global(internal)enable_legacy_airlocktoggle (default:true)/airlock-storage/*routes public SAS requests to the consolidated accountAPI (api_app)
AirlockRequestmodel gains anairlock_versionfield (default1)StatusChangedDataevent includesreview_workspace_idandairlock_versionAirlock Processor (airlock_processor)
StatusChangedQueueTriggerbased onairlock_versionblob_operations_metadata.pyfor in-place metadata updates (no blob copy)airlock_storage_helper.pyto resolve the correct storage account per version/stageDocumentation
Migration path
enable_legacy_airlock = true(default) — both v1 and v2 accounts exist side-by-sideairlock_version = 2on individual workspaces — new requests use consolidated storage; in-flight v1 requests complete normallyenable_legacy_airlock = falseto remove legacy accountsImpact
Version bumps
Testing
airlock_version = 1**