Skip to content
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

Better Azure resource name scheme / versioning Azure bicep #5756

Open
eerhardt opened this issue Sep 17, 2024 · 5 comments
Open

Better Azure resource name scheme / versioning Azure bicep #5756

eerhardt opened this issue Sep 17, 2024 · 5 comments
Labels
area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure

Comments

@eerhardt
Copy link
Member

Problem

In 8.x, we used an early/alpha version of Azure.Provisioning. This version used a naming scheme for Azure resources that tried to be the least common denominator of all resources.

        protected string GetGloballyUniqueName(string resourceName)
            => $"toLower(take('{resourceName}${{uniqueString(resourceGroup().id)}}', 24))";

This causes problems like Invalid Cloud deployment of Azure Service Bus topics - name truncation (dotnet/aspire#5341).

The new version of Azure.Provisioning has a better scheme that has information about what names the resource type allows (max length, valid characters, etc.).

    public override BicepValue<string>? ResolveName(
        ProvisioningContext context,
        Resource resource,
        ResourceNameRequirements requirements)
    {
        string prefix = SanitizeText(resource.ResourceName, requirements.ValidCharacters);
        string separator =
            requirements.ValidCharacters.HasFlag(ResourceNameCharacters.Hyphen) ? "-" :
            requirements.ValidCharacters.HasFlag(ResourceNameCharacters.Underscore) ? "_" :
            requirements.ValidCharacters.HasFlag(ResourceNameCharacters.Period) ? "." :
            "";
        BicepValue<string> suffix = GetUniqueSuffix(context, resource);
        return BicepFunction.Take(BicepFunction.Interpolate($"{prefix}{separator}{suffix}"), requirements.MaxLength);
    }

However, we are not able to take advantage of this new scheme. It is a breaking change to take this new naming scheme. If someone used 8.x to deploy to Azure, then updated to 9.0 and deployed again they would get duplicate resources since the naming scheme changed. For example, there is a separator now and toLower is no longer called in bicep but instead the resource.ResourceName is sanitized first before putting it in the bicep. If the resource doesn't allow upper case characters, the ResourceName is lowered. For resources that support upper case characters, this means a different resource name.

The new naming scheme is better than the old, but maintaining compatibliity is not allowing us to use it.

Proposed Solution

We should add a compatibility switch to allow developers who upgrade to the new version of .NET Aspire to continue using the same "deployment version" as a previous version.

One way we could do this is to allow Annotations on the IDistributedApplicationBuilder. One of the annotations would be an AzureDeploymentVersion annotation that would have values like:

  • 8
  • 9
  • Current

Current would be the default if there is no annotation specified. We would add new versions as we needed. And new breaking changes to the bicep would be gated behind a check if the specified AzureDeploymentVersion was "new enough".

This allows people to use the 9.0 .NET Aspire, but continue to use the old naming scheme.

We would also use this in other behavior changes - like when we change Azure PostgreSQL and Redis to use managed identity by default. Other "better defaults, but breaking changes" would do the same.

cc @mitchdenny @davidfowl @tg-msft

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication label Sep 17, 2024
@davidfowl davidfowl added area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure and removed area-app-model Issues pertaining to the APIs in Aspire.Hosting, e.g. DistributedApplication labels Sep 17, 2024
@tg-msft
Copy link
Contributor

tg-msft commented Sep 18, 2024

Consider exposing this via a default value like AddAzureStorage(..., AzureDeploymentVersion version = Current)? This would let us compile in the constant when not specified to avoid binary breaks but otherwise float on latest. It's effectively how we handle service versions for Azure's data plane. You could also do AddAzureStorage(..., AzureDeploymentVersion? version = null) and translate null into whatever's in the Annotations if you want an easy global default.

@eerhardt
Copy link
Member Author

@tg-msft - so your suggestion is to have both a "global" AzureDeploymentVersion for the whole app, as well as one for each Resource. And if the value is provided on a specific Resource that overrides the "global" value?

@tg-msft
Copy link
Contributor

tg-msft commented Sep 18, 2024

Yes. I think you're going to want more granularity than "all 8" or "all 9." We could always start globally though and add per-resource overrides in the future.

@eerhardt
Copy link
Member Author

Linking with @mitchdenny's proposal: #3121 to make this class based instead of a version enum / number.

@mitchdenny
Copy link
Member

I'd actually like to obsolete the existing PublishAsXYZ/AzXYZ methods and just have WithAzure<T> with a WithAzure that has a default T that mimics current behavior. Then we obsolete the old methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure
Projects
None yet
Development

No branches or pull requests

4 participants