Repository navigation
Add a managed identity option to the Container Apps sample - #140
bryansan-local wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
There was a problem hiding this comment.
Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.
An organization owner can choose the repository organization as the billing source for reviews requested by members with a paid Copilot license. Learn about Copilot code review billing.
paolosalvatori
left a comment
paolosalvatori
left a comment
There was a problem hiding this comment.
Paths are under samples/container-apps-blob-storage/python/ unless shown in full.
1. Rules applied
All rule files below were read in full. They are path-scoped to localstack-pro-azure/**, and this repo has no CLAUDE.md or .claude/rules of its own, so they are applied here by analogy and only where they carry over to sample code; the emulator-only invariants (stores, fill(), parity markers) do not apply.
localstack-pro-azure/CLAUDE.md.claude/rules/azure/common/:coding-style.md,hooks.md,patterns.md,security.md,testing.md.claude/rules/azure/python/:cloud-pipeline.md,coding-style.md,hooks.md,patterns.md,security.md,testing.md.claude/rules/azure/bicep/:coding-style.md,hooks.md,patterns.md,security.md,testing.md.claude/rules/azure/terraform/:coding-style.md,hooks.md,patterns.md,security.md,testing.md.claude/rules/azure/scripts/:coding-style.md,hooks.md,patterns.md,security.md,testing.md.claude/rules/azure/emulator/: none applies; no file covers Container Apps, Storage or Managed Identity, and the PR changes no emulator code.
None missing or unreadable.
2. Existing comments
Copilot posted two quota notices (no review) and an overview with no inline issues. The overview flagged Bicep SystemAssigned and Terraform UserAssigned as not yet verified on real Azure, as the PR description says too. I traced both paths (identity wiring, principalType, the role-before-app ordering for user-assigned, the post-app grant for system-assigned) and found no defect; the CI side of the same two variants is the run-samples.sh finding. No human comments.
3. Findings
Six LOW findings, each inline with a fix:
scripts/validate.shL281, L299: Round-trip retry gives up before the documented role propagation timescripts/validate.shL169: Role check resolves the principal through Microsoft Graphrun-samples.shL84, L111: TerraformUserAssignedand BicepSystemAssignednever run in CIscripts/README.mdL42: "No secret" is not true in managed-identity modeterraform/main.tfL73, L211: Role name literal is repeated instead of kept inlocalssrc/blob_storage_client.pyL60:from_envdocstring still says the connection string is required
Out of scope (pre-existing): scripts/deploy.sh L151-160 tests $ENVIRONMENT, which neither the script nor the workflow sets, so the HTTPS-to-HTTP downgrade is dead code and the container already gets the HTTPS endpoint. Its comment ("containers don't have the LS TLS cert") now contradicts the managed-identity path at L356-359, which relies on HTTPS from inside the container and passes CI. Worth deleting in a follow-up, together with the same branch in samples/aci-blob-storage/python/scripts/deploy.sh:117.
4. Clean files
README.mdbicep/README.mdbicep/deploy.shbicep/main.bicep(builds and lints clean; the explicitdependsOnis justified, since nothing references the user-assigned role assignment by property)scripts/cleanup.shscripts/deploy.sh(no findings of its own; its L358-359 comment shares the README wording above)src/app.pysrc/requirements.txt(azure-identity==1.25.3was published 2026-03-13 and needs Python >= 3.9; the image is 3.12)terraform/README.mdterraform/deploy.shterraform/outputs.tfterraform/variables.tf
5. Verdict
Approve. The three identity modes are wired consistently across az CLI, Bicep and Terraform, and every new managed-identity CI job passed on its first attempt. Checked against the sources and correct: the Storage Blob Data Contributor id (ba92f5b4-2d11-453d-a403-e96b0029c9fe), the IDENTITY_ENDPOINT/IDENTITY_HEADER contract, DefaultAzureCredential picking the user-assigned identity from AZURE_CLIENT_ID, and the az CLI's idempotent role assignment create (so a re-run does not trip over the existing assignment). None of the six LOWs blocks; the two validate.sh ones matter most for runs against real Azure.
| "$ENTRY_MESSAGE" | ||
| echo -n " Checking entry is served back from Blob Storage... " | ||
| if [[ $MANAGED_IDENTITY_TYPE != "None" ]]; then | ||
| for i in $(seq 1 30); do |
There was a problem hiding this comment.
LOW | L281, L299 | Round-trip retry gives up before the documented role propagation time
Description: Microsoft Learn says role assignment changes "can take up to 10 minutes" to take effect, but on Azure each pass here costs about 11 s (the 10 s sleep plus a GET and a POST that Storage turns away quickly with 403), so the loop gives up after roughly 5.5 minutes. For SystemAssigned, deploy.sh grants the role only once the app exists and validate.sh starts right after, so a grant near the documented bound fails a correct deployment. LocalStack enforces RBAC only with LS_AZURE_ENFORCE_RBAC=1, so on CI the first GET still passes and the larger budget costs nothing.
Rules applied: Assign an Azure role for access to blob data: "it can take up to 10 minutes for changes to take effect"; same bound in Troubleshoot Azure RBAC.
Also: the comment at L281 should say "up to 10 minutes" rather than "a few minutes".
| for i in $(seq 1 30); do | |
| for i in $(seq 1 60); do |
| fi | ||
| STORAGE_ACCOUNT_ID=$(az storage account show --name $STORAGE_ACCOUNT_NAME --resource-group $RESOURCE_GROUP_NAME --query 'id' --output tsv 2>/dev/null) | ||
| check_output "identity has [$STORAGE_ROLE] on the storage account" \ | ||
| "az role assignment list --assignee ${PRINCIPAL_ID:-missing} --scope $STORAGE_ACCOUNT_ID --query '[].roleDefinitionName' --output tsv" \ |
There was a problem hiding this comment.
LOW | L169 | Role check resolves the principal through Microsoft Graph
Description: With --assignee <guid> the az CLI first resolves the principal in Microsoft Graph and fails with "Cannot find user or service principal in graph database" when Graph has no match yet; the raw-GUID fallback applies only when the Graph call itself errors. An identity created moments earlier can still be replicating, the case deploy.sh already handles by granting with --assignee-object-id (L45-55), so this check can fail a correct deployment. az role assignment list takes the same flag, which bypasses the Graph query.
Rules applied: az CLI 2.91.0 azure/cli/command_modules/role/custom.py (_resolve_object_id_and_type) and az role assignment list --help (--assignee-object-id: "bypass Microsoft Graph query").
| "az role assignment list --assignee ${PRINCIPAL_ID:-missing} --scope $STORAGE_ACCOUNT_ID --query '[].roleDefinitionName' --output tsv" \ | |
| "az role assignment list --assignee-object-id ${PRINCIPAL_ID:-missing} --scope $STORAGE_ACCOUNT_ID --query '[].roleDefinitionName' --output tsv" \ |
| "samples/web-app-app-configuration/dotnet/terraform|bash deploy.sh" | ||
| "samples/aci-blob-storage/python/terraform|bash deploy.sh" | ||
| "samples/container-apps-blob-storage/python/terraform|bash deploy.sh" | ||
| "samples/container-apps-blob-storage/python/terraform|MANAGED_IDENTITY_TYPE=SystemAssigned bash deploy.sh" |
There was a problem hiding this comment.
LOW | L84, L111 | Terraform UserAssigned and Bicep SystemAssigned never run in CI
Description: CI covers four of the six managed-identity deployments, and the two it skips are the ones the PR description lists as not yet verified on real Azure. Each has wiring of its own (the counted identity and AZURE_CLIENT_ID env in Terraform, the post-app role assignment in Bicep), so a provider or emulator change can break it while every job stays green. Registering both adds two entries (four jobs of about 4 minutes).
Rules applied: General best practice: changed behaviour needs coverage, and run-samples.sh is the only entry point CI drives.
Also add this entry after L111:
"samples/container-apps-blob-storage/python/bicep|MANAGED_IDENTITY_TYPE=SystemAssigned bash deploy.sh"| "samples/container-apps-blob-storage/python/terraform|MANAGED_IDENTITY_TYPE=SystemAssigned bash deploy.sh" | |
| "samples/container-apps-blob-storage/python/terraform|MANAGED_IDENTITY_TYPE=SystemAssigned bash deploy.sh" | |
| "samples/container-apps-blob-storage/python/terraform|MANAGED_IDENTITY_TYPE=UserAssigned bash deploy.sh" |
| - `SystemAssigned`: the app is created with a system-assigned identity, which is then granted the `Storage Blob Data Contributor` role on the storage account. | ||
| - `UserAssigned`: a user-assigned identity is created and granted the role first, then attached to the app; its client ID is passed as `AZURE_CLIENT_ID`. | ||
|
|
||
| In both cases the app receives only the blob endpoint (`AZURE_STORAGE_ACCOUNT_URL`) and no secret, and `DefaultAzureCredential` gets its token from the identity endpoint Container Apps injects into the container. |
There was a problem hiding this comment.
LOW | L42 | "No secret" is not true in managed-identity mode
Description: Every mode still passes --registry-password, which the az CLI stores as a Container Apps secret named {server}-{user}, and the Bicep and Terraform flavours declare registry-password explicitly. Managed identity removes only storage-conn, which is what validate.sh asserts, and for UserAssigned the app also gets AZURE_CLIENT_ID, so "only the blob endpoint" is off as well.
Rules applied: General best practice (docs must match behaviour); az CLI 2.91.0 containerapp/_utils.py (store_as_secret_and_return_secret_ref).
Also: the scripts/deploy.sh comment at L358-359 makes the same claim ("and no secret at all"); "instead of the storage secret" would fix it.
| In both cases the app receives only the blob endpoint (`AZURE_STORAGE_ACCOUNT_URL`) and no secret, and `DefaultAzureCredential` gets its token from the identity endpoint Container Apps injects into the container. | |
| In both cases the app gets the blob endpoint (`AZURE_STORAGE_ACCOUNT_URL`) instead of the `storage-conn` secret, and `DefaultAzureCredential` gets its token from the identity endpoint Container Apps injects into the container. The ACR admin password used to pull the image remains a Container Apps secret. |
| resource "azurerm_role_assignment" "user_assigned" { | ||
| count = var.managed_identity_type == "UserAssigned" ? 1 : 0 | ||
| scope = azurerm_storage_account.example.id | ||
| role_definition_name = "Storage Blob Data Contributor" |
There was a problem hiding this comment.
LOW | L73, L211 | Role name literal is repeated instead of kept in locals
Description: The role name is typed into both role assignments, while this file's locals already keeps the secret names that several blocks reference "in one place so the references never drift apart", and the Bicep and script flavours each define the role once (storageBlobDataContributorRoleId, STORAGE_ROLE). As written, changing the role means two edits that nothing ties together. The fix touches three spots, so there is no suggestion block.
Rules applied: .claude/rules/azure/terraform/coding-style.md: "Factor repeated literals into locals/variables with a single source of truth" (applied by analogy), plus this file's own locals comment at L10-13.
After:
locals {
# ...existing names...
storage_role_name = "Storage Blob Data Contributor"
}
role_definition_name = local.storage_role_name # L73 and L211| @@ -58,17 +60,27 @@ def from_env(cls) -> "BlobGuestbookClient | None": | |||
| BLOB_CONTAINER_NAME - Name of the blob container | |||
There was a problem hiding this comment.
LOW | L60 | from_env docstring still says the connection string is required
Description: from_env now takes AZURE_STORAGE_ACCOUNT_URL in place of the connection string, and DefaultAzureCredential reads AZURE_CLIENT_ID (azure-identity 1.25.3 defaults managed_identity_client_id to it), but the docstring still lists only the connection string as required. L58-59 are outside the diff, so the suggestion continues the list from L60.
Rules applied: General best practice (a docstring must not contradict the code); azure-identity 1.25.3 azure/identity/_credentials/default.py.
| BLOB_CONTAINER_NAME - Name of the blob container | |
| or AZURE_STORAGE_ACCOUNT_URL - Blob endpoint, reached with DefaultAzureCredential | |
| BLOB_CONTAINER_NAME - Name of the blob container | |
| Optional env vars: | |
| AZURE_CLIENT_ID - Client ID of a user-assigned managed identity |
…y and update related documentation
Motivation
The Container Apps sample only reached Blob Storage with a connection string secret. Container apps can now run with a managed identity on the emulator, which is the usual way for an app on ACA to reach Azure services without secrets.
Changes
MANAGED_IDENTITY_TYPE(Noneby default,SystemAssignedorUserAssigned) for az CLI, Bicep and Terraform. The app's identity getsStorage Blob Data Contributorand replaces thestorage-connsecret.DefaultAzureCredential.validate.shchecks the identity and the role, andcleanup.shdeletes the user-assigned identity.run-samples.shruns the new variants.Tests
validate.sh. WithLS_AZURE_ENFORCE_RBAC=1, removing the role makes the app's write fail.SystemAssignedandUserAssigned, TerraformSystemAssignedand BicepUserAssignedpass. The other variants are pending, since the subscription is at its limit of 50 Container App Environments.Part of SMF-936