-
Notifications
You must be signed in to change notification settings - Fork 802
Add WithCompactResourceNaming() to fix storage name collisions #14442
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release/13.2
Are you sure you want to change the base?
Changes from all commits
efd24cb
02c3c68
70d2d79
c1721f8
c853549
21a93de
6e8dec8
1451d0d
ec43de2
471f184
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,7 +75,7 @@ public static IResourceBuilder<AzureContainerAppEnvironmentResource> AddAzureCon | |
| infra.Add(tags); | ||
|
|
||
| ProvisioningVariable? resourceToken = null; | ||
| if (appEnvResource.UseAzdNamingConvention) | ||
| if (appEnvResource.UseAzdNamingConvention || appEnvResource.UseCompactResourceNaming) | ||
| { | ||
| resourceToken = new ProvisioningVariable("resourceToken", typeof(string)) | ||
| { | ||
|
|
@@ -256,6 +256,30 @@ public static IResourceBuilder<AzureContainerAppEnvironmentResource> AddAzureCon | |
| $"{BicepFunction.ToLower(output.resource.Name)}-{BicepFunction.ToLower(volumeName)}"), | ||
| 32); | ||
| } | ||
| else if (appEnvResource.UseCompactResourceNaming) | ||
| { | ||
| Debug.Assert(resourceToken is not null); | ||
|
|
||
| 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); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. says this is max length 63 -
The above is just copying exactly what azd was using. |
||
|
|
||
| containerAppStorage.Name = BicepFunction.Take( | ||
| BicepFunction.Interpolate( | ||
| $"{BicepFunction.ToLower(output.resource.Name)}-{BicepFunction.ToLower(volumeName)}-{resourceToken}"), | ||
| 32); | ||
|
Comment on lines
+278
to
+281
|
||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -292,6 +316,26 @@ public static IResourceBuilder<AzureContainerAppEnvironmentResource> AddAzureCon | |
| storageVolume.Name = BicepFunction.Interpolate($"vol{resourceToken}"); | ||
| } | ||
| } | ||
| else if (appEnvResource.UseCompactResourceNaming) | ||
| { | ||
| Debug.Assert(resourceToken is not null); | ||
|
|
||
| if (storageVolume is not null) | ||
| { | ||
| // Sanitize env name for storage accounts: lowercase alphanumeric only. | ||
| // Reserve 2 chars for "sv" prefix + 13 for uniqueString = 15, leaving 9 for the env name. | ||
| var sanitizedPrefix = new string(appEnvResource.Name.ToLowerInvariant() | ||
| .Where(c => char.IsLetterOrDigit(c)).ToArray()); | ||
| if (sanitizedPrefix.Length > 9) | ||
| { | ||
| sanitizedPrefix = sanitizedPrefix[..9]; | ||
| } | ||
|
|
||
| storageVolume.Name = BicepFunction.Take( | ||
| BicepFunction.Interpolate($"{sanitizedPrefix}sv{resourceToken}"), | ||
| 24); | ||
| } | ||
| } | ||
|
|
||
| // Exposed so that callers reference the LA workspace in other bicep modules | ||
| infra.Add(new ProvisioningOutput("AZURE_LOG_ANALYTICS_WORKSPACE_NAME", typeof(string)) | ||
|
|
@@ -370,6 +414,35 @@ public static IResourceBuilder<AzureContainerAppEnvironmentResource> WithAzdReso | |
| return builder; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Configures the container app environment to use compact resource naming that maximally preserves | ||
| /// the <c>uniqueString</c> suffix for length-constrained Azure resources such as storage accounts. | ||
| /// </summary> | ||
| /// <param name="builder">The <see cref="AzureContainerAppEnvironmentResource"/> to configure.</param> | ||
| /// <returns>A reference to the <see cref="IResourceBuilder{T}"/> for chaining.</returns> | ||
| /// <remarks> | ||
| /// <para> | ||
| /// By default, the generated Azure resource names use long static suffixes (e.g. <c>storageVolume</c>, | ||
| /// <c>managedStorage</c>) that can consume most of the 24-character storage account name limit, truncating | ||
| /// the <c>uniqueString(resourceGroup().id)</c> portion that provides cross-deployment uniqueness. | ||
| /// </para> | ||
| /// <para> | ||
| /// When enabled, this method shortens the static portions of generated names so the full 13-character | ||
| /// <c>uniqueString</c> is preserved. This prevents naming collisions when deploying multiple environments | ||
| /// to different resource groups. | ||
| /// </para> | ||
| /// <para> | ||
| /// This option only affects volume-related storage resources. It does not change the naming of the | ||
| /// container app environment, container registry, log analytics workspace, or managed identity. | ||
| /// Use <see cref="WithAzdResourceNaming"/> to change those names as well. | ||
| /// </para> | ||
| /// </remarks> | ||
| public static IResourceBuilder<AzureContainerAppEnvironmentResource> WithCompactResourceNaming(this IResourceBuilder<AzureContainerAppEnvironmentResource> builder) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What do you think about making this the default behavior? So people don't have to "opt in" to getting code that works? Yes it would be a breaking change, but the "opt in" would be to opt back the broken behavior. This method could be called
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The problem is that we could really mess up peoples deployments if we do that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's the point of adding the option to get the existing naming back. |
||
| { | ||
| builder.Resource.UseCompactResourceNaming = true; | ||
| return builder; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Configures whether the Aspire dashboard should be included in the container app environment. | ||
| /// </summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,239 @@ | ||
| // Licensed to the .NET Foundation under one or more agreements. | ||
| // The .NET Foundation licenses this file to you under the MIT license. | ||
|
|
||
| using Aspire.Cli.Tests.Utils; | ||
| using Aspire.Deployment.EndToEnd.Tests.Helpers; | ||
| using Hex1b; | ||
| using Hex1b.Automation; | ||
| using Xunit; | ||
|
|
||
| namespace Aspire.Deployment.EndToEnd.Tests; | ||
|
|
||
| /// <summary> | ||
| /// End-to-end tests for compact resource naming with Azure Container App Environments. | ||
| /// Validates that WithCompactResourceNaming() fixes storage account naming collisions | ||
| /// caused by long environment names, and that the default naming is unchanged on upgrade. | ||
| /// </summary> | ||
| public sealed class AcaCompactNamingDeploymentTests(ITestOutputHelper output) | ||
| { | ||
| private static readonly TimeSpan s_testTimeout = TimeSpan.FromMinutes(40); | ||
|
|
||
| /// <summary> | ||
| /// Verifies that deploying with a long ACA environment name and a volume | ||
| /// succeeds when WithCompactResourceNaming() is used. | ||
| /// The storage account name would otherwise exceed 24 chars and truncate the uniqueString. | ||
| /// </summary> | ||
| [Fact] | ||
| public async Task DeployWithCompactNamingFixesStorageCollision() | ||
| { | ||
| using var cts = new CancellationTokenSource(s_testTimeout); | ||
| using var linkedCts = CancellationTokenSource.CreateLinkedTokenSource( | ||
| cts.Token, TestContext.Current.CancellationToken); | ||
|
|
||
| await DeployWithCompactNamingFixesStorageCollisionCore(linkedCts.Token); | ||
| } | ||
|
|
||
| private async Task DeployWithCompactNamingFixesStorageCollisionCore(CancellationToken cancellationToken) | ||
| { | ||
| var subscriptionId = AzureAuthenticationHelpers.TryGetSubscriptionId(); | ||
| if (string.IsNullOrEmpty(subscriptionId)) | ||
| { | ||
| Assert.Skip("Azure subscription not configured. Set ASPIRE_DEPLOYMENT_TEST_SUBSCRIPTION."); | ||
| } | ||
|
|
||
| if (!AzureAuthenticationHelpers.IsAzureAuthAvailable()) | ||
| { | ||
| if (DeploymentE2ETestHelpers.IsRunningInCI) | ||
| { | ||
| Assert.Fail("Azure authentication not available in CI. Check OIDC configuration."); | ||
| } | ||
| else | ||
| { | ||
| Assert.Skip("Azure authentication not available. Run 'az login' to authenticate."); | ||
| } | ||
| } | ||
|
|
||
| var workspace = TemporaryWorkspace.Create(output); | ||
| var recordingPath = DeploymentE2ETestHelpers.GetTestResultsRecordingPath(nameof(DeployWithCompactNamingFixesStorageCollision)); | ||
| var startTime = DateTime.UtcNow; | ||
| var resourceGroupName = DeploymentE2ETestHelpers.GenerateResourceGroupName("compact"); | ||
|
|
||
| output.WriteLine($"Test: {nameof(DeployWithCompactNamingFixesStorageCollision)}"); | ||
| output.WriteLine($"Resource Group: {resourceGroupName}"); | ||
| output.WriteLine($"Subscription: {subscriptionId[..8]}..."); | ||
| output.WriteLine($"Workspace: {workspace.WorkspaceRoot.FullName}"); | ||
|
|
||
| try | ||
| { | ||
| var builder = Hex1bTerminal.CreateBuilder() | ||
| .WithHeadless() | ||
| .WithDimensions(160, 48) | ||
| .WithAsciinemaRecording(recordingPath) | ||
| .WithPtyProcess("/bin/bash", ["--norc"]); | ||
|
|
||
| using var terminal = builder.Build(); | ||
| var pendingRun = terminal.RunAsync(cancellationToken); | ||
|
|
||
| var waitingForInitComplete = new CellPatternSearcher() | ||
| .Find("Aspire initialization complete"); | ||
|
|
||
| var waitingForVersionSelectionPrompt = new CellPatternSearcher() | ||
| .Find("(based on NuGet.config)"); | ||
|
|
||
| var waitingForPipelineSucceeded = new CellPatternSearcher() | ||
| .Find("PIPELINE SUCCEEDED"); | ||
|
|
||
| var counter = new SequenceCounter(); | ||
| var sequenceBuilder = new Hex1bTerminalInputSequenceBuilder(); | ||
|
|
||
| // Step 1: Prepare environment | ||
| output.WriteLine("Step 1: Preparing environment..."); | ||
| sequenceBuilder.PrepareEnvironment(workspace, counter); | ||
|
|
||
| // Step 2: Set up CLI | ||
| if (DeploymentE2ETestHelpers.IsRunningInCI) | ||
| { | ||
| output.WriteLine("Step 2: Using pre-installed Aspire CLI..."); | ||
| sequenceBuilder.SourceAspireCliEnvironment(counter); | ||
| } | ||
|
|
||
| // Step 3: Create single-file AppHost | ||
| output.WriteLine("Step 3: Creating single-file AppHost..."); | ||
| sequenceBuilder.Type("aspire init") | ||
| .Enter() | ||
| .Wait(TimeSpan.FromSeconds(5)) | ||
| .Enter() | ||
| .WaitUntil(s => waitingForInitComplete.Search(s).Count > 0, TimeSpan.FromMinutes(2)) | ||
| .WaitForSuccessPrompt(counter, TimeSpan.FromMinutes(2)); | ||
|
|
||
| // Step 4: Add required packages | ||
| output.WriteLine("Step 4: Adding Azure Container Apps package..."); | ||
| sequenceBuilder.Type("aspire add Aspire.Hosting.Azure.AppContainers") | ||
| .Enter(); | ||
|
|
||
| if (DeploymentE2ETestHelpers.IsRunningInCI) | ||
| { | ||
| sequenceBuilder | ||
| .WaitUntil(s => waitingForVersionSelectionPrompt.Search(s).Count > 0, TimeSpan.FromSeconds(60)) | ||
| .Enter(); | ||
| } | ||
|
|
||
| sequenceBuilder.WaitForSuccessPrompt(counter, TimeSpan.FromSeconds(180)); | ||
|
|
||
| // Step 5: Modify apphost.cs with a long environment name and a container with volume. | ||
| // Use WithCompactResourceNaming() so the storage account name preserves the uniqueString. | ||
| sequenceBuilder.ExecuteCallback(() => | ||
| { | ||
| var appHostFilePath = Path.Combine(workspace.WorkspaceRoot.FullName, "apphost.cs"); | ||
| var content = File.ReadAllText(appHostFilePath); | ||
|
|
||
| var buildRunPattern = "builder.Build().Run();"; | ||
| var replacement = """ | ||
| // Long env name (16 chars) would truncate uniqueString without compact naming | ||
| builder.AddAzureContainerAppEnvironment("my-long-env-name") | ||
| .WithCompactResourceNaming(); | ||
|
|
||
| // Container with a volume triggers storage account creation | ||
| builder.AddContainer("worker", "mcr.microsoft.com/dotnet/samples", "aspnetapp") | ||
| .WithVolume("data", "/app/data"); | ||
|
|
||
| builder.Build().Run(); | ||
| """; | ||
|
|
||
| content = content.Replace(buildRunPattern, replacement); | ||
| File.WriteAllText(appHostFilePath, content); | ||
|
|
||
| output.WriteLine($"Modified apphost.cs with long env name + compact naming + volume"); | ||
| }); | ||
|
|
||
| // Step 6: Set environment variables for deployment | ||
| sequenceBuilder.Type($"unset ASPIRE_PLAYGROUND && export AZURE__LOCATION=westus3 && export AZURE__RESOURCEGROUP={resourceGroupName}") | ||
| .Enter() | ||
| .WaitForSuccessPrompt(counter); | ||
|
|
||
| // Step 7: Deploy | ||
| output.WriteLine("Step 7: Deploying with compact naming..."); | ||
| sequenceBuilder | ||
| .Type("aspire deploy --clear-cache") | ||
| .Enter() | ||
| .WaitUntil(s => waitingForPipelineSucceeded.Search(s).Count > 0, TimeSpan.FromMinutes(30)) | ||
| .WaitForSuccessPrompt(counter, TimeSpan.FromMinutes(2)); | ||
|
|
||
| // Step 8: Verify storage account was created and name contains uniqueString | ||
| output.WriteLine("Step 8: Verifying storage account naming..."); | ||
| sequenceBuilder | ||
| .Type($"STORAGE_NAMES=$(az storage account list -g \"{resourceGroupName}\" --query \"[].name\" -o tsv) && " + | ||
| "echo \"Storage accounts: $STORAGE_NAMES\" && " + | ||
| "STORAGE_COUNT=$(echo \"$STORAGE_NAMES\" | wc -l) && " + | ||
| "echo \"Count: $STORAGE_COUNT\" && " + | ||
| // Verify each storage name contains 'sv' (compact naming marker) | ||
| "for name in $STORAGE_NAMES; do " + | ||
| "if echo \"$name\" | grep -q 'sv'; then echo \"✅ $name uses compact naming\"; " + | ||
| "else echo \"⚠️ $name does not use compact naming (may be ACR storage)\"; fi; " + | ||
| "done") | ||
| .Enter() | ||
| .WaitForSuccessPrompt(counter, TimeSpan.FromSeconds(30)); | ||
|
|
||
| // Step 9: Exit | ||
| sequenceBuilder.Type("exit").Enter(); | ||
|
|
||
| var sequence = sequenceBuilder.Build(); | ||
| await sequence.ApplyAsync(terminal, cancellationToken); | ||
| await pendingRun; | ||
|
|
||
| var duration = DateTime.UtcNow - startTime; | ||
| output.WriteLine($"✅ Test completed in {duration}"); | ||
|
|
||
| DeploymentReporter.ReportDeploymentSuccess( | ||
| nameof(DeployWithCompactNamingFixesStorageCollision), | ||
| resourceGroupName, | ||
| new Dictionary<string, string>(), | ||
| duration); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| output.WriteLine($"❌ Test failed: {ex.Message}"); | ||
|
|
||
| DeploymentReporter.ReportDeploymentFailure( | ||
| nameof(DeployWithCompactNamingFixesStorageCollision), | ||
| resourceGroupName, | ||
| ex.Message, | ||
| ex.StackTrace); | ||
|
|
||
| throw; | ||
| } | ||
| finally | ||
| { | ||
| output.WriteLine($"Cleaning up resource group: {resourceGroupName}"); | ||
| await CleanupResourceGroupAsync(resourceGroupName); | ||
| } | ||
| } | ||
|
|
||
| private async Task CleanupResourceGroupAsync(string resourceGroupName) | ||
| { | ||
| try | ||
| { | ||
| var process = new System.Diagnostics.Process | ||
| { | ||
| StartInfo = new System.Diagnostics.ProcessStartInfo | ||
| { | ||
| FileName = "az", | ||
| Arguments = $"group delete --name {resourceGroupName} --yes --no-wait", | ||
| RedirectStandardOutput = true, | ||
| RedirectStandardError = true, | ||
| UseShellExecute = false | ||
| } | ||
| }; | ||
|
|
||
| process.Start(); | ||
| await process.WaitForExitAsync(); | ||
| output.WriteLine(process.ExitCode == 0 | ||
| ? $"Resource group deletion initiated: {resourceGroupName}" | ||
| : $"Resource group deletion may have failed (exit code {process.ExitCode})"); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| output.WriteLine($"Failed to cleanup resource group: {ex.Message}"); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compact naming removes
.and-fromvolumeNameand 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-bvsab,a.bvsab), and there’s nooutput.index(or other discriminator) included to prevent collisions within the same storage account. Consider incorporating an invariant discriminator (likeoutput.index) or using a collision-resistant normalization strategy.