From b20ee71f3264a76f86c5a38cf97517ee347fc48c Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Wed, 19 Feb 2025 19:15:50 -0800 Subject: [PATCH 1/5] Don't set properties on existing Azure SQL server resources --- .../AzureSqlExtensions.cs | 13 ++----- .../ExistingAzureResourceTests.cs | 37 +++++-------------- 2 files changed, 13 insertions(+), 37 deletions(-) diff --git a/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs b/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs index e840a5fb7ac..d73879b655d 100644 --- a/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs +++ b/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs @@ -205,14 +205,6 @@ private static void CreateSqlServer( { var resource = SqlServer.FromExisting(identifier); resource.Name = name; - resource.Administrators = new ServerExternalAdministrator() - { - AdministratorType = SqlAdministratorType.ActiveDirectory, - IsAzureADOnlyAuthenticationEnabled = true, - Sid = principalIdParameter, - Login = principalNameParameter, - TenantId = BicepFunction.GetSubscription().TenantId - }; return resource; }, (infrastructure) => @@ -248,7 +240,10 @@ private static void CreateSqlServer( // the principalType. var principalTypeParameter = new ProvisioningParameter(AzureBicepResource.KnownParameters.PrincipalType, typeof(string)); infrastructure.Add(principalTypeParameter); - sqlServer.Administrators.PrincipalType = principalTypeParameter; + if (!sqlServer.IsExistingResource) + { + sqlServer.Administrators.PrincipalType = principalTypeParameter; + } infrastructure.Add(new SqlFirewallRule("sqlFirewallRule_AllowAllIps") { diff --git a/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs b/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs index 1ed5ce92dee..8bff52f1718 100644 --- a/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs +++ b/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs @@ -1050,15 +1050,6 @@ param existingResourceName string resource sqlServer 'Microsoft.Sql/servers@2021-11-01' existing = { name: existingResourceName - properties: { - administrators: { - administratorType: 'ActiveDirectory' - login: principalName - sid: principalId - tenantId: subscription().tenantId - azureADOnlyAuthentication: true - } - } } resource sqlFirewallRule_AllowAllAzureIps 'Microsoft.Sql/servers/firewallRules@2021-11-01' = { @@ -1118,16 +1109,6 @@ param principalType string resource sqlServer 'Microsoft.Sql/servers@2021-11-01' existing = { name: existingResourceName - properties: { - administrators: { - administratorType: 'ActiveDirectory' - principalType: principalType - login: principalName - sid: principalId - tenantId: subscription().tenantId - azureADOnlyAuthentication: true - } - } } resource sqlFirewallRule_AllowAllAzureIps 'Microsoft.Sql/servers/firewallRules@2021-11-01' = { @@ -1300,13 +1281,13 @@ public async Task SupportsExistingAzureApplicationInsightsWithResourceGroup() var expectedBicep = """ @description('The location for the resource(s) to be deployed.') param location string = resourceGroup().location - + param existingResourceName string - + resource appInsights 'Microsoft.Insights/components@2020-02-02' existing = { name: existingResourceName } - + output appInsightsConnectionString string = appInsights.properties.ConnectionString """; @@ -1347,17 +1328,17 @@ public async Task SupportsExistingAzureOpenAIWithResourceGroup() var expectedBicep = """ @description('The location for the resource(s) to be deployed.') param location string = resourceGroup().location - + param existingResourceName string - + param principalType string - + param principalId string - + resource openAI 'Microsoft.CognitiveServices/accounts@2024-10-01' existing = { name: existingResourceName } - + resource openAI_CognitiveServicesOpenAIContributor 'Microsoft.Authorization/roleAssignments@2022-04-01' = { name: guid(openAI.id, principalId, subscriptionResourceId('Microsoft.Authorization/roleDefinitions', 'a001fd3d-188f-4b5d-821b-7da978bf7442')) properties: { @@ -1367,7 +1348,7 @@ param principalId string } scope: openAI } - + resource mymodel 'Microsoft.CognitiveServices/accounts/deployments@2024-10-01' = { name: 'mymodel' properties: { From 884f94cb3da6b1bf87d8ad32661164972449a920 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Wed, 19 Feb 2025 19:54:29 -0800 Subject: [PATCH 2/5] Set admin for existing resources correctly --- .../AzureSqlExtensions.cs | 91 +++++++++++++++++++ .../ExistingAzureResourceTests.cs | 18 ++++ 2 files changed, 109 insertions(+) diff --git a/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs b/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs index d73879b655d..106de61a28f 100644 --- a/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs +++ b/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs @@ -5,6 +5,7 @@ using Aspire.Hosting.Azure; using Azure.Provisioning; using Azure.Provisioning.Expressions; +using Azure.Provisioning.Primitives; using Azure.Provisioning.Sql; namespace Aspire.Hosting; @@ -226,6 +227,20 @@ private static void CreateSqlServer( }; }); + // If the resource is an existing resource, we model the administrator access + // for the managed identity as an "edge" between the parent SqlServer resource + // and a custom SqlServerAzureADAdministrator resource. + if (sqlServer.IsExistingResource) + { + var admin = new SqlServerAzureADAdministratorWorkaround($"{sqlServer.BicepIdentifier}_admin") + { + ParentOverride = sqlServer, + LoginOverride = principalNameParameter, + SidOverride = principalIdParameter + }; + infrastructure.Add(admin); + } + infrastructure.Add(new SqlFirewallRule("sqlFirewallRule_AllowAllAzureIps") { Parent = sqlServer, @@ -240,6 +255,7 @@ private static void CreateSqlServer( // the principalType. var principalTypeParameter = new ProvisioningParameter(AzureBicepResource.KnownParameters.PrincipalType, typeof(string)); infrastructure.Add(principalTypeParameter); + // Avoid mutating properties on existing resources. if (!sqlServer.IsExistingResource) { sqlServer.Administrators.PrincipalType = principalTypeParameter; @@ -268,4 +284,79 @@ private static void CreateSqlServer( infrastructure.Add(new ProvisioningOutput("sqlServerFqdn", typeof(string)) { Value = sqlServer.FullyQualifiedDomainName }); } + + /// + /// Workaround for immutable properties on SqlServerAzureADAdministrator. + /// + private sealed class SqlServerAzureADAdministratorWorkaround(string bicepIdentifier) : SqlServerAzureADAdministrator(bicepIdentifier) + { + private BicepValue? _name; + private BicepValue? _login; + private BicepValue? _sid; + private ResourceReference? _parent; + + /// + /// Login name of the server administrator. + /// + public BicepValue LoginOverride + { + get + { + Initialize(); + return _login!; + } + set + { + Initialize(); + _login!.Assign(value); + } + } + + /// + /// SID (object ID) of the server administrator. + /// + public BicepValue SidOverride + { + get + { + Initialize(); + return _sid!; + } + set + { + Initialize(); + _sid!.Assign(value); + } + } + + /// + /// Parent resource of the server administrator. + /// + public SqlServer? ParentOverride + { + get + { + Initialize(); + return _parent!.Value; + } + set + { + Initialize(); + _parent!.Value = value; + } + } + + private static BicepValue GetNameDefaultValue() + { + return new StringLiteralExpression("ActiveDirectory"); + } + + protected override void DefineProvisionableProperties() + { + _name = DefineProperty("Name", ["name"], defaultValue: GetNameDefaultValue()); + _login = DefineProperty("Login", ["properties", "login"]); + _sid = DefineProperty("Sid", ["properties", "sid"]); + _parent = DefineResource("Parent", ["parent"], isOutput: false, isRequired: true); + } + } } diff --git a/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs b/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs index 8bff52f1718..32623153c8f 100644 --- a/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs +++ b/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs @@ -1052,6 +1052,15 @@ param existingResourceName string name: existingResourceName } + resource sqlServer_admin 'Microsoft.Sql/servers/administrators@2021-11-01' = { + name: 'ActiveDirectory' + properties: { + login: principalName + sid: principalId + } + parent: sqlServer + } + resource sqlFirewallRule_AllowAllAzureIps 'Microsoft.Sql/servers/firewallRules@2021-11-01' = { name: 'AllowAllAzureIps' properties: { @@ -1111,6 +1120,15 @@ param principalType string name: existingResourceName } + resource sqlServer_admin 'Microsoft.Sql/servers/administrators@2021-11-01' = { + name: 'ActiveDirectory' + properties: { + login: principalName + sid: principalId + } + parent: sqlServer + } + resource sqlFirewallRule_AllowAllAzureIps 'Microsoft.Sql/servers/firewallRules@2021-11-01' = { name: 'AllowAllAzureIps' properties: { From ba06ed7ed886fd0e44e8f7393361e9142866dd86 Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Wed, 19 Feb 2025 20:06:04 -0800 Subject: [PATCH 3/5] Add workaround reference for Azure SDK issue --- src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs b/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs index 106de61a28f..925790f931f 100644 --- a/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs +++ b/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs @@ -287,6 +287,7 @@ private static void CreateSqlServer( /// /// Workaround for immutable properties on SqlServerAzureADAdministrator. + /// See https://github.com/Azure/azure-sdk-for-net/issues/48364. /// private sealed class SqlServerAzureADAdministratorWorkaround(string bicepIdentifier) : SqlServerAzureADAdministrator(bicepIdentifier) { From f4d23c7aff23c85f349b733f834b6f988f696607 Mon Sep 17 00:00:00 2001 From: Eric Erhardt Date: Wed, 19 Feb 2025 22:07:43 -0600 Subject: [PATCH 4/5] PR feedback. Add a link to Azure.Provisioning issue. --- src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs | 12 ++++++------ .../ExistingAzureResourceTests.cs | 2 -- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs b/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs index 925790f931f..94d315453e2 100644 --- a/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs +++ b/src/Aspire.Hosting.Azure.Sql/AzureSqlExtensions.cs @@ -251,13 +251,13 @@ private static void CreateSqlServer( if (distributedApplicationBuilder.ExecutionContext.IsRunMode) { - // When in run mode we inject the users identity and we need to specify - // the principalType. - var principalTypeParameter = new ProvisioningParameter(AzureBicepResource.KnownParameters.PrincipalType, typeof(string)); - infrastructure.Add(principalTypeParameter); // Avoid mutating properties on existing resources. if (!sqlServer.IsExistingResource) { + // When in run mode we inject the users identity and we need to specify + // the principalType. + var principalTypeParameter = new ProvisioningParameter(AzureBicepResource.KnownParameters.PrincipalType, typeof(string)); + infrastructure.Add(principalTypeParameter); sqlServer.Administrators.PrincipalType = principalTypeParameter; } @@ -286,8 +286,8 @@ private static void CreateSqlServer( } /// - /// Workaround for immutable properties on SqlServerAzureADAdministrator. - /// See https://github.com/Azure/azure-sdk-for-net/issues/48364. + /// Workaround for issue using SqlServerAzureADAdministrator. + /// See https://github.com/Azure/azure-sdk-for-net/issues/48364 for more information. /// private sealed class SqlServerAzureADAdministratorWorkaround(string bicepIdentifier) : SqlServerAzureADAdministrator(bicepIdentifier) { diff --git a/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs b/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs index 32623153c8f..339f73acc77 100644 --- a/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs +++ b/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs @@ -1114,8 +1114,6 @@ param principalName string param existingResourceName string - param principalType string - resource sqlServer 'Microsoft.Sql/servers@2021-11-01' existing = { name: existingResourceName } From 783d95ebdf35ff199c04d557d595507b1f5831fd Mon Sep 17 00:00:00 2001 From: Safia Abdalla Date: Wed, 19 Feb 2025 20:17:12 -0800 Subject: [PATCH 5/5] Remove `principalType` parameter from test assertion --- tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs b/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs index 339f73acc77..eb2a7c16a77 100644 --- a/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs +++ b/tests/Aspire.Hosting.Azure.Tests/ExistingAzureResourceTests.cs @@ -1096,8 +1096,7 @@ public async Task SupportsExistingAzureSqlServerInRunMode() "params": { "existingResourceName": "{existingResourceName.value}", "principalId": "", - "principalName": "", - "principalType": "" + "principalName": "" } } """;