dotnet / aspire

Tools, templates, and packages to accelerate building observable, production-ready apps
https://learn.microsoft.com/dotnet/aspire
MIT License
3.94k stars 482 forks source link

Subsequent provisioning of Azure Sql Server fails #3101

Open miceiken opened 8 months ago

miceiken commented 8 months ago

Seemingly out of the blue, our pipeline provisioning started failing:

  (✓) Done: Resource group: rg-snip
  (✓) Done: Log Analytics workspace: law-snip
  (✓) Done: Container Apps Environment: cae-snip
  (✓) Done: Container Registry: acrsnip
  (✓) Done: Container App: snip
  (✓) Done: Key Vault: secretssnip
  (✓) Done: Application Insights: appinsights-snip
  (x) Failed: Azure SQL Server: sqlserver-snip

ERROR: deployment failed: failing invoking action 'provision', error deploying infrastructure: deploying to subscription:

Deployment Error Details:
InvalidParameterValue: Invalid value given for parameter ExternalAdministratorLoginSid. Specify a valid parameter value.

We did try changing the AD admin for the SQL server to a group (with the managed identity as a member) in Azure Portal, but even when changing it back to the managed identity it still fails.

I think it is related to: https://github.com/Azure/bicep-types-az/issues/2320

https://learn.microsoft.com/en-us/azure/templates/microsoft.sql/2022-05-01-preview/servers?pivots=deployment-language-bicep

The Azure Active Directory administrator of the server. This can only be used at server create time. If used for server update, it will be ignored or it will result in an error. For updates individual APIs will need to be used

davidfowl commented 8 months ago

What version of aspire is this?

miceiken commented 8 months ago

The Aspire Preview 4 release

 Workload version: 8.0.200-manifests.a7f084b6
 [aspire]
   Installation Source: SDK 8.0.200
   Manifest Version:    8.0.0-preview.4.24156.9/8.0.100
   Manifest Path:       /usr/local/share/dotnet/sdk-manifests/8.0.100/microsoft.net.sdk.aspire/8.0.0-preview.4.24156.9/WorkloadManifest.json
   Install Type:        FileBased
azd version 1.7.0 (commit 49d6adc2efb178083f61822e6b4715258560803d)
# azd config
{
  "alpha": {
    "infraSynth": "on",
    "resourceGroupDeployments": "on"
  },
  "platform": {
    "type": "default"
  },
}

Running

azd infra synth
azd provision
davidfowl commented 8 months ago

Can you file the issue here https://github.com/Azure/azure-dev/issues

miceiken commented 8 months ago

Done. It seems to me from my rudimentary understanding that the Bicep file for Azure Sql Resource should be amended to use a Microsoft.Sql/servers/administrators (and AD) resource, similiar to this comment: https://github.com/Azure/bicep-types-az/issues/2320

Looking at the Aspire codebase now however, it seems like provisioning in P5 and on will use CDK instead of Bicep template, so maybe it's not so relevant?

davidfowl commented 8 months ago

cc @mitchdenny

mitchdenny commented 8 months ago

Hrm ... looks like we will need to get the child resource added to the CDK in order to use this.

mitchdenny commented 8 months ago

cc @JoshLove-msft

joperezr commented 8 months ago

This is waiting on a change from CDK.

mitchdenny commented 8 months ago

More detail - this is the resource type we need implemented: https://learn.microsoft.com/en-us/azure/templates/microsoft.sql/servers/administrators?pivots=deployment-language-bicep

miceiken commented 8 months ago

Just posting my temporary workaround until the issue is resolved. Updating the synthesized infra/sqlserver/aspire.hosting.azure.bicep.sql.bicep to:

@description('User name')
param principalName string

@description('User id')
param principalId string

@description('Tags that will be applied to all resources')
param tags object = {}

@description('The location used for all deployed resources')
param location string = resourceGroup().location

@description('The name of the sql server resource')
param serverName string

param databases array = []

var resourceToken = uniqueString(resourceGroup().id)

resource sql 'Microsoft.Sql/servers@2022-05-01-preview' = {
  name: '${serverName}-${resourceToken}'
  location: location
  tags: tags
  properties: {
    minimalTlsVersion: '1.2'
    publicNetworkAccess: 'Enabled'
    administrators: {}
  }

  resource sqlAdminsResource 'administrators@2022-05-01-preview' = {
    name: 'ActiveDirectory'
    properties: {
      administratorType: 'ActiveDirectory'
      login: principalName
      sid: principalId
      tenantId: subscription().tenantId
    }
  }

  resource sqlAzureAdOnly 'azureADOnlyAuthentications@2022-05-01-preview' = {
    name: 'Default'
    properties: {
      azureADOnlyAuthentication: true
    }
    dependsOn: [sqlAdminsResource]
  }

  resource sqlFirewall 'firewallRules@2022-05-01-preview' = {
    name: 'fw-sql-localdev'
    properties: {
      startIpAddress: '0.0.0.0'
      endIpAddress: '255.255.255.255'
    }
  }

  resource db 'databases@2022-05-01-preview' = [
    for name in databases: {
      name: name
      location: location
      sku: {
        name: 'S0'
      }
      tags: tags
    }
  ]
}

output sqlServerFqdn string = sql.properties.fullyQualifiedDomainName

I'm not sure this would work on initial provisioning as it does not contain any administrators (neither AD or credentials) in the Microsoft.Sql/servers@2022-05-01-preview resource, but it works currently when already provisioned.

mitchdenny commented 8 months ago

Thanks for sharing this

mitchdenny commented 7 months ago

@JoshLove-msft @tg-msft I think to solve this issue we are going to need a resource to represent the azureADOnlyAuthentications@2022-05-01-preview child resource.

joperezr commented 5 months ago

Per @tg-msft This is blocked on next CDK update.

joperezr commented 5 months ago

Marking as 8.2 as we are not planning to take the new CDK until then. @mitchdenny do we need it for 8.1? If so, @tg-msft mentioned we could probably also get this a servicing.

mitchdenny commented 5 months ago

We can let this go to 8.2 I think.

mitchdenny commented 3 months ago

Backlogging this until we get CDK updates.