Add WithCompactResourceNaming() to fix storage name collisions#14442
Add WithCompactResourceNaming() to fix storage name collisions#14442mitchdenny wants to merge 10 commits intorelease/13.2from
Conversation
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.sh | bash -s -- 14442Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/aspire/main/eng/scripts/get-aspire-cli-pr.ps1) } 14442" |
|
/deployment-test |
|
🚀 Deployment tests starting on PR #14442... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
There was a problem hiding this comment.
Pull request overview
This PR introduces an opt-in compact naming mode for Azure Container Apps environment–related storage resources to prevent storage account naming collisions when the environment name is long, while keeping the default naming behavior unchanged for upgrade safety.
Changes:
- Add
WithCompactResourceNaming()to generate shorter, collision-resistant names for storage account / managed storage / file shares. - Update Bicep generation to include a
resourceTokenwhen compact naming is enabled and apply new naming formulas. - Add unit snapshot tests and new E2E deployment/upgrade tests covering compact naming and upgrade safety.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppExtensions.cs | Implements compact naming logic and exposes WithCompactResourceNaming() API. |
| src/Aspire.Hosting.Azure.AppContainers/AzureContainerAppEnvironmentResource.cs | Adds internal flag to enable compact naming behavior. |
| tests/Aspire.Hosting.Azure.Tests/AzureContainerAppsTests.cs | Adds unit tests validating compact naming behavior via manifest/Bicep snapshots. |
| tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureContainerAppsTests.AddContainerAppEnvironmentWithCompactNamingPreservesUniqueString.verified.bicep | Snapshot covering compact storage account naming for long env names. |
| tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureContainerAppsTests.AddContainerAppEnvironmentWithCompactNamingPreservesUniqueString.verified.json | Snapshot metadata for the above test. |
| tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureContainerAppsTests.CompactNamingMultipleVolumesHaveUniqueNames.verified.bicep | Snapshot covering multiple volume/bind mount naming under compact mode. |
| tests/Aspire.Hosting.Azure.Tests/Snapshots/AzureContainerAppsTests.CompactNamingMultipleVolumesHaveUniqueNames.verified.json | Snapshot metadata for the above test. |
| tests/Aspire.Deployment.EndToEnd.Tests/Helpers/DeploymentE2ETestHelpers.cs | Adds helper to install GA/release-quality CLI for upgrade scenario testing. |
| tests/Aspire.Deployment.EndToEnd.Tests/AcaCompactNamingDeploymentTests.cs | Adds E2E deployment test validating compact naming resolves collision scenario. |
| tests/Aspire.Deployment.EndToEnd.Tests/AcaCompactNamingUpgradeDeploymentTests.cs | Adds E2E upgrade-safety test to ensure defaults don’t change across GA→dev upgrade. |
| var volumeName = output.volume.Type switch | ||
| { | ||
| ContainerMountType.BindMount => $"bm{output.index}", | ||
| ContainerMountType.Volume => output.volume.Source ?? $"v{output.index}", | ||
| _ => throw new NotSupportedException() | ||
| }; | ||
|
|
||
| // Remove '.' and '-' characters from volumeName | ||
| volumeName = volumeName.Replace(".", "").Replace("-", ""); | ||
|
|
||
| share.Name = BicepFunction.Take( | ||
| BicepFunction.Interpolate( | ||
| $"{BicepFunction.ToLower(output.resource.Name)}-{BicepFunction.ToLower(volumeName)}"), | ||
| 60); |
There was a problem hiding this comment.
Compact naming removes . and - from volumeName and then builds the file share name from only {resourceName}-{volumeName}. This can map distinct user-provided volume names to the same value (e.g., a-b vs ab, a.b vs ab), and there’s no output.index (or other discriminator) included to prevent collisions within the same storage account. Consider incorporating an invariant discriminator (like output.index) or using a collision-resistant normalization strategy.
| containerAppStorage.Name = BicepFunction.Take( | ||
| BicepFunction.Interpolate( | ||
| $"{BicepFunction.ToLower(output.resource.Name)}-{BicepFunction.ToLower(volumeName)}-{resourceToken}"), | ||
| 32); |
There was a problem hiding this comment.
containerAppStorage.Name is built with take(..., 32). If output.resource.Name and/or volumeName are long, different volumes can collapse to the same 32-char prefix after truncation (the resourceToken suffix might also be truncated). To guarantee uniqueness within an environment, consider including an invariant short discriminator (e.g., output.index or a short hash) in the constructed name before applying take.
| sequenceBuilder | ||
| .Type("cp -f /tmp/aspire-cli-dev/aspire ~/.aspire/bin/aspire 2>/dev/null || echo 'Dev CLI not at /tmp, checking env...'") | ||
| .Enter() | ||
| .WaitForSuccessPrompt(counter); | ||
|
|
||
| // Re-source environment to pick up the dev CLI | ||
| sequenceBuilder.SourceAspireCliEnvironment(counter); | ||
| } |
There was a problem hiding this comment.
In CI, restoring the dev CLI uses cp ... || echo ... and then proceeds without verifying the copy succeeded. If /tmp/aspire-cli-dev/aspire isn’t present (or the copy fails), the test may continue running the GA CLI and still pass, making the upgrade validation unreliable. Consider failing fast when the dev CLI artifact isn’t available and/or asserting after aspire --version that the expected (dev) build is running before redeploying.
🎬 CLI E2E Test RecordingsThe following terminal recordings are available for commit
📹 Recordings uploaded automatically from CI run #21940203098 |
|
/deployment-test |
…date aspire update shows two prompts when switching channels: 1. 'Perform updates? [y/n]' - package confirmation 2. 'Which directory for NuGet.config file?' - NuGet config placement Both need Enter to accept defaults.
aspire update has multiple sequential prompts (confirm, NuGet.config dir, NuGet.config apply, potential CLI self-update). Use Wait+Enter pattern to accept all defaults without needing to track each prompt individually.
Timed Enter presses were unreliable — if prompts appeared at different speeds, extra Enters would leak to the shell and corrupt subsequent commands. Now explicitly waits for each of the 3 prompts: 1. 'Perform updates? [y/n]' 2. 'Which directory for NuGet.config file?' 3. 'Apply these changes to NuGet.config? [y/n]'
mitchdenny
force-pushed
the
fix/compact-resource-naming-14427
branch
from
3d7d38b to
471f184
Compare
|
/deployment-test |
|
🚀 Deployment tests starting on PR #14442... This will deploy to real Azure infrastructure. Results will be posted here when complete. |
|
✅ Deployment E2E Tests passed Summary: 21 passed, 0 failed, 0 cancelled Passed Tests
🎬 Terminal Recordings
|
2 participants
Summary
Fixes #14427
When Azure Container App environment names are long (≥11 characters), the
uniqueStringsuffix gets truncated in storage account and managed storage names, causing naming collisions across deployments in different resource groups.Problem
Azure resource names are generated as
take('{bicepIdentifier}{uniqueString(resourceGroup().id)}', maxLength). TheuniqueStringis 13 chars, and storage accounts have a 24-char max. Aspire's static suffixes (storageVolume= 13 chars,managedStorage= 14 chars) inflate the prefix, causing the unique suffix to be truncated or eliminated entirely.myenvstoragevolume{uniq...}myenvnmstoragevolume{un...}mylongenvstoragevolume{...}Solution
WithCompactResourceNaming()is an opt-in method that shortens only the length-constrained resource names while preserving the full 13-charuniqueString:take('{prefix}sv{resourceToken}', 24)— prefix truncated to 9 chars, 2-char marker, full 13-char token = 24take('{name}-{volume}-{resourceToken}', 32)— includes both resource token (cross-deployment uniqueness) and volume name (within-deployment uniqueness)take('{name}-{volume}', 60)— 63-char limit, less criticalUsage
Design Decisions
WithAzdResourceNaming()andWithCompactResourceNaming()are used, Azd naming takes precedence (it already fixes storage).Nameproperties are shortenedTests