Update Azure.Provisioning to latest version #5592

eerhardt · 2024-09-09T01:00:28Z

Update to the new Azure.Provisioning libraries.

Fix #5583

cc @mitchdenny @davidfowl @tg-msft @JoshLove-msft

Checklist

Is this feature complete?
- Yes. Ready to ship.
- No. Follow-up changes expected.
Are you including unit tests for the changes and scenario tests if relevant?
- Yes
- No
Did you add public API?
- Yes
  - If yes, did you have an API Review for it?
    - Yes
    - No
  - Did you add <remarks /> and <code /> elements on your triple slash comments?
    - Yes
    - No
- No
Does the change make any security assumptions or guarantees?
- Yes
  - If yes, have you done a threat model and had a security review?
    - Yes
    - No
- No
Does the change require an update in our Aspire docs?
- Yes
  - Link to aspire-docs issue: Update Aspire docs for new Azure.Provisioning version in 9.0 docs-aspire#1652
- No

Microsoft Reviewers: Open in CodeFlow

Fix dotnet#5583 WIP: This first stab converts KeyVault to the new Provisioning version.

src/Aspire.Hosting.Azure.KeyVault/AzureKeyVaultResourceExtensions.cs

playground/cdk/CdkSample.AppHost/mykv.module.bicep

src/Aspire.Hosting.Azure.Storage/AzureStorageExtensions.cs

davidfowl · 2024-09-11T03:02:11Z

src/Aspire.Hosting.Azure.Storage/AzureStorageExtensions.cs

+            AssignRole(StorageBuiltInRole.StorageTableDataContributor);
+            AssignRole(StorageBuiltInRole.StorageQueueDataContributor);
+
+            construct.Add(new BicepOutput("blobEndpoint", typeof(string)) { Value = storageAccount.PrimaryEndpoints.Value!.BlobUri });


@tg-msft why is .Value nullable?

BicepValue<T> is either a T? Value or an Expression? Expression. We have plans to clean this up so folks won't be using .Value!s and .Expression!s anywhere.

src/Aspire.Hosting.Azure.KeyVault/AzureKeyVaultResourceExtensions.cs

davidfowl · 2024-09-11T03:04:34Z

src/Aspire.Hosting.Azure.KeyVault/AzureKeyVaultResourceExtensions.cs

+                    new MemberExpression(
+                        new MemberExpression(
+                            new IdentifierExpression(keyVault.ResourceName),
+                            "properties"),
+                        "vaultUri")


Why doesn't storage look like this?

My understanding of @tg-msft's explanation is because the VaultUri property isn't parented directly under the KeyVaultService, but instead under the .Properties property. And that causes issues with the expressions. He is aware and is fixing this.

Known issue related to an open design question around BicepValue<T> vs "T with internal expression" for models. I think folks are leaning toward the latter. Eventually this will be able to replaced with keyVault.Properties.VaultUri. (This is the same reason we had the .Value! in one of the other comments.)

davidfowl · 2024-09-11T03:05:13Z

src/Aspire.Hosting.Azure.KeyVault/AzureKeyVaultResourceExtensions.cs

+            keyVault.Tags["aspire-resource-name"] = construct.Resource.Name;
+
+            var role = KeyVaultBuiltInRole.KeyVaultAdministrator;
+            var roleAssignment = new RoleAssignment($"{KeyVaultBuiltInRole.GetBuiltInRoleName(role)}_{keyVault.ResourceName}")


Does GetBuiltInRole have an interface (static interface method). We should be able to make this AssignRole method generic.

You should be able to punt on this with the new generated AssignRole overloads.

You should be able to punt on this with the new generated AssignRole overloads.

Yep, for all except WebPubSub, which doesn't have a BuiltInRole.

cc @vicancy

I just need to add some metadata to the spec (i.e., like this example) but didn't see anything documented at https://learn.microsoft.com/en-us/azure/role-based-access-control/built-in-roles other than SignalR/Web PubSub Contributor. I was going to follow-up with Liangying, but David beat me to it here. 😆 Are https://github.com/microsoft/azure-roles/blob/main/azure_roles.json#L585-L586 sufficient to add?

How about Azure/azure-sdk-for-net#45977 ? For some reason that the team is now investigating, the builtin roles for Web PubSub are listed in Preview category somehow, so that they are not seen in the documentation.

playground/cdk/CdkSample.AppHost/storage.module.bicep

…tputs at the bottom

eerhardt · 2024-09-11T19:40:25Z

src/Aspire.Hosting.Azure.PostgreSQL/AzurePostgresExtensions.cs

@@ -17,6 +18,10 @@ namespace Aspire.Hosting;
 /// </summary>
 public static class AzurePostgresExtensions
 {
+    private const string ServerResourceVersion = "2023-03-01-preview";


@davidfowl @mitchdenny @tg-msft - how should we think about resource versions? Should we always hard-code the version we expect, and then periodically update to new versions on some sort of cadence? Similar to our other dependency versions.

At the moment I have everything default to "latest stable" in Azure.Provisioning.* (though I'm pulling the info from ARM so it's not perfect yet). The long term plan is to have Azure.Provisioning.* automatically release whenever the management plane libraries are updated so the "latest stable" will change seemingly at random.

As a general rule, moving to newer service versions is goodness for customers but there are occasionally breaking changes. We could float on latest by default and allow conservative customers to pin, or we could pin and have to manually test/bump the world a couple times a year. We might want to talk offline with Jeff Richter and Mike Kistler from Azure's Breaking Change Review Board for their advice on how dangerous it would be for customers to float on latest.

I've decided to define these for all resources and put them in a shared file. Some things, like KeyVault, need to be used across many libraries. Thoughts on this approach?

Note I still need to ensure all resources use this pattern. Right now EventHubs doesn't.

I prefer updating the library and letting that determine the version of the default azure resource. If we don't do that then this will be just like container image tags except it'll be managed in both the Azure.Provisioning libraries and in aspire.

I would prefer that too, but there are problems with that approach.

Not all resources have a default version in the CDK right now. Especially "child" resources. For those we need to provide a version, or else we get errors that it is unset.

The default version for some things don't actually work. For example, the CDK doesn't emit an empty "properties" objects, but the default version for ServiceBus topics (I think it was this one) says that you must have a properties object. I can look up the exact error, if someone cares.

The concern that updating to a new version will break something. @tg-msft assures me that there are breaking changes that happen from time to time. So if we just take the version that the CDK uses, and someone updates their CDK to a new version, with new default resource versions, there is a potential for our bicep to be broken.

1 is a bug that will be fixed by either generating the same default as parent resources or using a resolver to copy those. There's still a bit to design around the versioning story. I'd like to to have one list of versions per library, but there are some challenges there with ARM.

Let's follow up on 2. I currently try to omit as many empty objects as possible (i.e., no reason to serialize the entire object graph with empty braces everywhere) but we can design a way to include specific empty objects where needed if using a new ObjectExpression() isn't already a sufficient workaround.

davidfowl · 2024-09-13T08:03:02Z

playground/cdk/CdkSample.AppHost/Program.cs

+            queue.MaxDeliveryCount = 5;
+            queue.LockDuration = new StringLiteral("PT5M");
+            // TODO: this should be
+            // queue.LockDuration = TimeSpan.FromMinutes(5);


It's a known issue - SB uses ISO8601 durations (that are no longer allowed in Azure, save for recurrences or human-centric time intervals, because no languages support them well) and we'll need to carry along a format to serialize TimeSpans.

eerhardt · 2024-09-13T13:37:51Z

Tests are broken becuase I need to update the test nuget.config file for the new feed.

@radical - is it possible to not have the duplicate test nuget.config?

src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs

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

radical · 2024-09-16T18:56:12Z

Tests are broken becuase I need to update the test nuget.config file for the new feed.

@radical - is it possible to not have the duplicate test nuget.config?

Yes, I will fix that.

The ones that still need to be specified are: Top-level - RedisResource - SignalRService - WebPubSubService Child resources - CognitiveServicesAccountDeployment - CosmosDBSqlDatabase - PostgreSqlFlexibleServerFirewallRule - PostgreSqlFlexibleServerDatabase - SqlFirewallRule - SqlDatabase

eerhardt · 2024-09-16T22:12:08Z

OK, I think this PR is ready for final review and can be merged to unblock other work.

There are follow up issues on the CDK we can make later.

Default ResourceVersions for all resources.
TimeSpan formatting for ServiceBus
WebPubSub roles
.Value! fix ups and using Expressions as workarounds.

Directory.Packages.props

playground/AspireEventHub/EventHubs.AppHost/ehstorage.module.bicep

playground/AspireEventHub/EventHubs.AppHost/eventhubns.module.bicep

playground/SqlServerEndToEnd/SqlServerEndToEnd.AppHost/sql1.module.bicep

mitchdenny · 2024-09-17T01:05:30Z

playground/bicep/BicepSample.AppHost/redis.module.bicep

-
-resource keyVault_IeF8jZvXV 'Microsoft.KeyVault/vaults@2022-07-01' existing = {
-  name: keyVaultName
+resource keyVault 'Microsoft.KeyVault/vaults@2019-09-01' existing = {


Why the downgrade in API version (surprising) /cc @tg-msft

Let me look into it.

mitchdenny · 2024-09-17T01:52:13Z

OK I've done a full pass over this now. Its looking really good. I've left a few comments but the themes are:

Resource name consistency between Aspire 8.0 and Aspire 9.0 (really important unless we've got a good reason to break it.
Resource API version regressions in some areas (just need to understand why, and whether its going to cause problems. Would probably pay to deploy playground/cdk sample using 8.0 and 9.0 to the same target resource group and make sure it deploys cleanly (if the resource names line up and the config settings that change are mutable it should work fine.

Not specifically called out in comments above, but some of this has got a lot more verbose. That isn't a problem provided we provide really good documentation.

- Default Storage AccessTier = Hot, to keep existing behavior - Use IPAddress when setting PostgreSQL firewall rules

davidfowl · 2024-09-18T02:08:31Z

/azp run

azure-pipelines · 2024-09-18T02:08:44Z

Azure Pipelines successfully started running 1 pipeline(s).

Update Azure.Provisioning to latest version

bbfea9d

Fix dotnet#5583 WIP: This first stab converts KeyVault to the new Provisioning version.

dotnet-issue-labeler bot added the area-components label Sep 9, 2024

davidfowl reviewed Sep 9, 2024

View reviewed changes

src/Aspire.Hosting.Azure.KeyVault/AzureKeyVaultResourceExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Sep 9, 2024

View reviewed changes

playground/cdk/CdkSample.AppHost/mykv.module.bicep Outdated Show resolved Hide resolved

davidfowl reviewed Sep 9, 2024

View reviewed changes

playground/cdk/CdkSample.AppHost/mykv.module.bicep Outdated Show resolved Hide resolved

eerhardt added 2 commits September 9, 2024 19:25

Add AsBicepParameter

929fcac

Update more stuff to make it deployable.

2a364cf

davidfowl added area-integrations Issues pertaining to Aspire Integrations packages azure Issues associated specifically with scenarios tied to using Azure and removed area-components labels Sep 10, 2024

eerhardt added 2 commits September 10, 2024 11:56

Add a location parameter so azd up works.

d4c4f8c

Update Storage to use the new CDK

1e23335

davidfowl reviewed Sep 11, 2024

View reviewed changes

src/Aspire.Hosting.Azure.Storage/AzureStorageExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Sep 11, 2024

View reviewed changes

src/Aspire.Hosting.Azure.KeyVault/AzureKeyVaultResourceExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Sep 11, 2024

View reviewed changes

playground/cdk/CdkSample.AppHost/storage.module.bicep Outdated Show resolved Hide resolved

davidfowl reviewed Sep 11, 2024

View reviewed changes

playground/cdk/CdkSample.AppHost/storage.module.bicep Outdated Show resolved Hide resolved

eerhardt added 2 commits September 11, 2024 13:44

Update PostgreSQL to use the new CDK

6b4faf1

Sort the bicep statements so that parameters appear at the top and ou…

906e1c1

…tputs at the bottom

eerhardt commented Sep 11, 2024

View reviewed changes

eerhardt added 8 commits September 11, 2024 18:14

Update Redis to use the new CDK

3e1a0c1

Update CosmosDB to use the new CDK

d63bcf4

Update Sql to use the new CDK

9147277

Update ServiceBus to use the new CDK

2e34551

Update SignalR to use the new CDK

0acf431

Update Search to use the new CDK

eb7fb7a

Update ApplicationInsights and OperationalInsights to use the new CDK

7a1bfa9

Update AppConfiguration to use the new CDK

bb41af8

davidfowl reviewed Sep 13, 2024

View reviewed changes

Update test nuget.configs

18cbd0a

eerhardt requested a review from radical as a code owner September 13, 2024 13:37

davidfowl reviewed Sep 13, 2024

View reviewed changes

src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs Outdated Show resolved Hide resolved

davidfowl reviewed Sep 14, 2024

View reviewed changes

tests/Aspire.Hosting.Azure.Tests/AzureBicepResourceTests.cs Outdated Show resolved Hide resolved

eerhardt added 3 commits September 16, 2024 12:06

Update the rest of the bicep in the playground apps.

2bb632d

Use simpler expression to set SqlFirewallRule addresses

9c1112b

Merge remote-tracking branch 'upstream/main' into UpdateCDK

dc2ef1e

eerhardt added 2 commits September 16, 2024 14:06

Add XML docs to the new APIs.

15f7258

eerhardt mentioned this pull request Sep 16, 2024

Update Aspire docs for new Azure.Provisioning version in 9.0 dotnet/docs-aspire#1652

Open

mitchdenny reviewed Sep 16, 2024

View reviewed changes

Directory.Packages.props Show resolved Hide resolved

mitchdenny reviewed Sep 16, 2024

View reviewed changes

playground/AspireEventHub/EventHubs.AppHost/ehstorage.module.bicep Outdated Show resolved Hide resolved

mitchdenny reviewed Sep 16, 2024

View reviewed changes

playground/AspireEventHub/EventHubs.AppHost/eventhubns.module.bicep Outdated Show resolved Hide resolved

mitchdenny reviewed Sep 17, 2024

View reviewed changes

playground/SqlServerEndToEnd/SqlServerEndToEnd.AppHost/sql1.module.bicep Show resolved Hide resolved

mitchdenny reviewed Sep 17, 2024

View reviewed changes

eerhardt added 6 commits September 17, 2024 11:03

Merge remote-tracking branch 'upstream/main' into UpdateCDK

35dd244

Fix up whitespace

fa37195

Use 8.x naming scheme.

16ea988

Fix deployment name in OpenAI

369e72f

Merge remote-tracking branch 'upstream/main' into UpdateCDK

da65e75

Respond to PR feedback

422d838

- Default Storage AccessTier = Hot, to keep existing behavior - Use IPAddress when setting PostgreSQL firewall rules

davidfowl approved these changes Sep 18, 2024

View reviewed changes

davidfowl merged commit fd0e633 into dotnet:main Sep 18, 2024
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Azure.Provisioning to latest version #5592

Update Azure.Provisioning to latest version #5592

eerhardt commented Sep 9, 2024 •

edited

Loading

davidfowl Sep 11, 2024

tg-msft Sep 11, 2024

davidfowl Sep 11, 2024

eerhardt Sep 11, 2024

tg-msft Sep 11, 2024

davidfowl Sep 11, 2024 •

edited

Loading

tg-msft Sep 11, 2024

eerhardt Sep 13, 2024

davidfowl Sep 13, 2024

tg-msft Sep 13, 2024

vicancy Sep 14, 2024

eerhardt Sep 11, 2024

tg-msft Sep 11, 2024

eerhardt Sep 13, 2024

davidfowl Sep 13, 2024

eerhardt Sep 13, 2024

tg-msft Sep 13, 2024

davidfowl Sep 13, 2024

tg-msft Sep 13, 2024

eerhardt commented Sep 13, 2024

radical commented Sep 16, 2024

eerhardt commented Sep 16, 2024

mitchdenny Sep 17, 2024

tg-msft Sep 17, 2024

mitchdenny commented Sep 17, 2024

davidfowl commented Sep 18, 2024

azure-pipelines bot commented Sep 18, 2024

Update Azure.Provisioning to latest version #5592

Update Azure.Provisioning to latest version #5592

Conversation

eerhardt commented Sep 9, 2024 • edited Loading

Checklist

Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidfowl Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eerhardt commented Sep 13, 2024

radical commented Sep 16, 2024

eerhardt commented Sep 16, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchdenny commented Sep 17, 2024

davidfowl commented Sep 18, 2024

azure-pipelines bot commented Sep 18, 2024

eerhardt commented Sep 9, 2024 •

edited

Loading

davidfowl Sep 11, 2024 •

edited

Loading