Support referencing existing Service Bus resources in references #7249

captainsafia · 2025-01-25T07:07:04Z

Opening this PR to sanity check the approach before adding it to other resource types. Some things of note:

Support for referencing existing resources in other subscription IDs/resource groups isn't implemented due to a gap in the underlying provisioning APIs. See [FEATURE REQ] FromExisting methods should support setting scopes Azure/azure-sdk-for-net#47980. There are workarounds for the but the juice might not be worth the squeeze.
Separate APIs for publishing/running with an existing resource backed by an annotation and explicit checks in the ConfigureInfrastructure calls for each resource.

davidfowl · 2025-01-25T07:07:57Z

Opening this PR to sanity check the approach before adding it to other resource types. Some things of note:

This should be a draft right?

src/Aspire.Hosting.Azure.ServiceBus/AzureServiceBusExtensions.cs

src/Aspire.Hosting/ApplicationModel/ExistingResourceAnnotation.cs

src/Aspire.Hosting.Azure.ServiceBus/AzureServiceBusExtensions.cs

davidfowl · 2025-01-25T07:08:57Z

src/Aspire.Hosting/ApplicationModel/ExistingResourceAnnotation.cs

+/// Represents a resource that is not managed by Aspire's provisioning or
+/// container management layer.
+/// </summary>
+public class ExistingResourceAnnotation(string name, bool isPublishMode = false) : IResourceAnnotation


Is this supposed to be Azure specific?

Based on what we decided, yes. Are you indicating that the type name should indicate it's an ExistingAzureResourceAnnotation?

It's in the wrong assembly.

This should be ExistingAzureResourceAnnotation and it needs the subscription id, resource group and name as properties.

I remove subscription ID and resource group from this model because of:

Support for referencing existing resources in other subscription IDs/resource groups isn't implemented due to a gap in the underlying provisioning APIs. See Azure/azure-sdk-for-net#47980. There are workarounds for the but the juice might not be worth the squeeze.

We can't materialize these into scopes in the existing reference in the Bicep identifier at the moment so it doesn't make sense to expose an API for something that isn't supported at the moment.

We can add them later easily enough but if we have them now they'd be non-functional.

I think we should at a minimum add the resource group so that we can play with what it would look like in a custom resource even though the Azure.Provisioning libs can't directly do it yet (we can try it out with the bicep resource). The reason it's important is because in a normal aspire deployment, the resource group is created on the fly, so at a minimum, using exisiting resource would look like a name + a different resource group in the same subscription.

The reason it's important is because in a normal aspire deployment, the resource group is created on the fly, so at a minimum, using exisiting resource would look like a name + a different resource group in the same subscription.

Hmmm...I see. Are you cool with iterating on this in a separate PR?

No, I think this pr is where we iterate

davidfowl · 2025-01-25T07:39:19Z

src/Aspire.Hosting.Azure/Provisioning/Provisioners/AzureProvisioner.cs

@@ -291,13 +291,13 @@ private async Task ProcessResourceAsync(IConfiguration configuration, Lazy<Task<

            resourceLogger.LogWarning("No provisioner found for {resourceType} skipping.", resource.GetType().Name);
        }
-        else if (!provisioner.ShouldProvision(configuration, resource.AzureResource))
+        else if (!resource.AzureResource.IsExisting() && !provisioner.ShouldProvision(configuration, resource.AzureResource))


Why do you need these checks?

To avoid provisioning a resource or reading the config that has been cached in user-secrets when the same config name is stored. Without this, starting with:

var bus = builder.AddServiceBus("messages");

And moving to:

var bus = builder.AddServiceBus("messages").RunAsExisting("cached-messaging");

would still load the config for the Aspire-managed "messages" resource from user-secrets.

Shouldn't that just work? I'm confused why we need this.

src/Aspire.Hosting.Azure.ServiceBus/AzureServiceBusExtensions.cs

src/Aspire.Hosting.Azure/ExistingAzureResourceExtensions.cs

tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs

eerhardt · 2025-01-27T21:54:09Z

src/Aspire.Hosting.Azure/ExistingAzureResourceExtensions.cs

+    {
+        ArgumentNullException.ThrowIfNull(resource);
+
+        existingAzureResourceAnnotation = resource.Annotations.OfType<ExistingAzureResourceAnnotation>().FirstOrDefault();


For annotaitons, we usually have either:

respect multiple (not applicable here)

expect single

Last one wins

aspire/src/Aspire.Hosting/ApplicationModel/ResourceExtensions.cs

Lines 277 to 279 in 706bbbf

public static bool TryGetContainerImageName(this IResource resource, [NotNullWhen(true)] out string? imageName)

{

if (resource.Annotations.OfType<ContainerImageAnnotation>().LastOrDefault() is { } imageAnnotation)

I don't think using the "first" one is typical.

Good point -- the intent here was to follow the expect single pattern.

eerhardt · 2025-01-27T21:56:54Z

tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs

+    }
+
+    [Fact]
+    public async Task RequiresPublishAsExistingInPublishMode()


(nit) Is this test name correct? the test calls RunAsExisting

Yeah, the test name is correct although confusing. I was intending to say that you need to call PublishAsExisting in publish-mode otherwise the API effectively no-ops.

eerhardt · 2025-01-27T21:57:07Z

tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs

+    }
+
+    [Fact]
+    public async Task AddExistingAzureServiceBusInPublishModeMode()


(nit) InPublishModeMode?

eerhardt · 2025-01-27T21:58:21Z

src/Aspire.Hosting.Azure.ServiceBus/AzureServiceBusExtensions.cs

            {
-                Sku = new AzureProvisioning.ServiceBusSku()
+                var existingResourceName = existingAnnotation.NameParameter.AsProvisioningParameter(infrastructure, $"{infrastructure.AspireResource.GetBicepIdentifier()}ExistingResourceName");


nit - Is ExistingResourceName too verbose?

eerhardt · 2025-01-27T21:59:40Z

src/Aspire.Hosting.Azure.ServiceBus/AzureServiceBusExtensions.cs

@@ -86,7 +96,7 @@ public static IResourceBuilder<AzureServiceBusResource> AddAzureServiceBus(this
                        infrastructure.Add(cdkRule);


Does generating these Queues and Topics work with an existing resource?

davidfowl · 2025-01-27T22:09:49Z

src/Aspire.Hosting.Azure/ExistingAzureResourceAnnotation.cs

+public class ExistingAzureResourceAnnotation(IResourceBuilder<ParameterResource> nameParameter) : IResourceAnnotation
+{
+    /// <summary>
+    /// Gets the name of the existing resource.
+    /// </summary>
+    public IResourceBuilder<ParameterResource> NameParameter { get; } = nameParameter;
+}


Suggested change

public class ExistingAzureResourceAnnotation(IResourceBuilder<ParameterResource> nameParameter) : IResourceAnnotation

{

/// <summary>

/// Gets the name of the existing resource.

/// </summary>

public IResourceBuilder<ParameterResource> NameParameter { get; } = nameParameter;

}

public class ExistingAzureResourceAnnotation(ParameterResource nameParameter) : IResourceAnnotation

{

/// <summary>

/// Gets the name of the existing resource.

/// </summary>

public ParameterResource NameParameter { get; } = nameParameter;

}

We don't have an AsProvisioningParameter overload that takes the ParameterResource directly although that seems like an oversight given what the implementation currently does.

aspire/src/Aspire.Hosting.Azure/AzureProvisioningResourceExtensions.cs

Lines 67 to 78 in 5008bfb

public static ProvisioningParameter AsProvisioningParameter(this IResourceBuilder<ParameterResource> parameterResourceBuilder, AzureResourceInfrastructure infrastructure, string? parameterName = null)

{

ArgumentNullException.ThrowIfNull(parameterResourceBuilder);

ArgumentNullException.ThrowIfNull(infrastructure);

parameterName ??= Infrastructure.NormalizeBicepIdentifier(parameterResourceBuilder.Resource.Name);

infrastructure.AspireResource.Parameters[parameterName] = parameterResourceBuilder.Resource;

return GetOrAddParameter(infrastructure, parameterName, parameterResourceBuilder.Resource.Secret);

}

I think we can probably just add another overload that takes ParameterResource and have the existing one call into it.

Support referencing existing Service Bus resources in references

7873ae4