Azure / bicep

Bicep is a declarative language for describing and deploying Azure resources
MIT License
3.25k stars 754 forks source link

`reference()` with api version argument returns different results from `reference()` without api version #5328

Closed jahead closed 2 years ago

jahead commented 2 years ago

Bicep version v0.4.1008

Describe the bug Unable to get the fullyQualifiedDomainName from provider Microsoft.DBforMySQL/flexibleServers@2021-05-01 properties. Reported error is The template output 'dbHostUrl' is not valid: The language expression property 'fullyQualifiedDomainName' doesn't exist, available properties are 'administratorLogin, storage, state, createMode, network, backup, highAvailability'.."

Maybe the bicep type is wrong? bit fullyQualifiedDomainName properties is in all sorts of documentation not in the bicep

To Reproduce Depoy a mySql flexible server and try and use the fullyQualifiedDomainName as an output

Additional context create-mysql-server.bicep

param serviceName string

param administratorLogin string = 'dbadmin'
param passwordSeed string
param administratorLoginPassword string

@allowed([
  'production'
  'nonProduction'
])
param evironmentType string = 'nonProduction'

@allowed([
  'australiaeast'
])
param location string = 'australiaeast'

param dbName string

var metadata = json(loadTextContent('./metadata.json'))

var locationShortMap = metadata.locationShortMap

var evironmentTypeShort = evironmentType == 'production' ? 'prod' : 'nonProd'

var unquieTag = '${substring(uniqueString(resourceGroup().id), 4)}'

var suffix = '${locationShortMap[location]}-${evironmentTypeShort}-${unquieTag}'

var mySqlServerName = toLower('mc-${serviceName}-mysql-${suffix}')

resource mySqlServer 'Microsoft.DBforMySQL/flexibleServers@2021-05-01' = {
  name: mySqlServerName
  location: location
  sku: {
    name: 'Standard_B1s'
    tier: 'Burstable'
  }
  properties: {
    administratorLogin: administratorLogin
    administratorLoginPassword: administratorLoginPassword
    backup: {
      backupRetentionDays: 30
      geoRedundantBackup: 'Disabled'
    }
  }
  resource firewall 'firewallRules' = {
    name: mySqlServerName
    properties: {
      startIpAddress: '0.0.0.0'
      endIpAddress: '0.0.0.0'
    }
  }
}

resource bookingDb 'Microsoft.DBforMySQL/flexibleServers/databases@2021-05-01' = {
  name: dbName
  parent: mySqlServer
}

output administratorLogin string = administratorLogin
output administratorLoginPassword string = administratorLoginPassword
output dbUsername string = administratorLogin
output dbPassword string = administratorLoginPassword
output dbHostUrl string = mySqlServer.properties.fullyQualifiedDomainName
output dbName string = bookingDb.name
output serverId string = mySqlServer.id
{
    "status": "Failed",
    "error": {
        "code": "DeploymentOutputEvaluationFailed",
        "message": "Unable to evaluate template outputs: 'dbHostUrl'. Please see error details and deployment operations. Please see https://aka.ms/arm-debug for usage details.",
        "details": [
            {
                "code": "DeploymentOutputEvaluationFailed",
                "target": "dbHostUrl",
                "message": "The template output 'dbHostUrl' is not valid: The language expression property 'fullyQualifiedDomainName' doesn't exist, available properties are 'administratorLogin, storage, state, createMode, network, backup, highAvailability'.."
            }
        ]
    }
}

workaround just to build my own FQDN from the name service :(

alex-frankel commented 2 years ago

I agree the type information we have is wrong. I created https://github.com/Azure/azure-rest-api-specs/issues/16991 to track this.

alex-frankel commented 2 years ago

So this one I am confused about. If I emit the entire resource, I don't get an error and I can see that property in the returned object, but if I attempt to emit the property fullyQualifiedDomainName I am told the property doesn't exist. The issue seems to be in the emitted JSON, which does not use the "full" argument for the reference function.

This works:

output fqdn string = reference(mySqlServer.id, mySqlServer.apiVersion, 'full').properties.fullyQualifiedDomainName

This does not:

output fqdn string = mySqlServer.properties.fullyQualifiedDomainName

EDIT -- 3/9/22: We need to take the time to identify the root cause on this one. I am going to schedule some time internally for us to discuss.

bmoore-msft commented 2 years ago

re: root cause - this is intentional (right or wrong aside) - the full properties object of the GET is not returned by default to reference(), it's a subset. That's where 'full' comes in... So if what you want is not there, adding the 'full' param will make sure everything in the GET response is returned.

I don't remember off the top how the deployment engine decides what to filter though...

alex-frankel commented 2 years ago

the full properties object of the GET is not returned by default to reference(), it's a subset

I thought the only properties not included when not specifying 'full' are top-level properties like kind. Since this is part of the properties object, it should be returned. And in either case, Bicep has the logic to determine when it needs to specify 'full' in the generated JSON, so I think something else might be happening.

bmoore-msft commented 2 years ago

Maybe the logic bicep has is related to the logic the deployment engine uses... here's a quick test you can do to see the diff - from JSON

    "dbHostObj": {
      "type": "object",
      "value": "[reference(resourceId('Microsoft.DBforMySQL/flexibleServers', variables('mySqlServerName')))]"
    },
    "dbHostObjFull": {
      "type": "object",
      "value": "[reference(resourceId('Microsoft.DBforMySQL/flexibleServers', variables('mySqlServerName')), '2021-05-01', 'Full')]"
    },

Just looking at the properties object for those, we see a subset of properties returned on the first one... I believe that ARM only issues one GET in that case - a handy optimization that suggests we process something server side.

Anyway - so we have a few places to look for the problem here...

alex-frankel commented 2 years ago

This may be a timing thing. If I use an existing reference, both the "raw" reference() call and the idiomatic bicep code work:

resource mySqlServer 'Microsoft.DBforMySQL/flexibleServers@2021-05-01' existing = {
  name: 'foobar${uniqueString(resourceGroup().id, 'alfran')}'
}

output fqdn string = reference(mySqlServer.id, mySqlServer.apiVersion, 'full').properties.fullyQualifiedDomainName

output fqdn2 string = mySqlServer.properties.fullyQualifiedDomainName

I'm wondering if right after the PUT completes, the fullyQualifiedDomainName is not available..

bmoore-msft commented 2 years ago

Ah... maybe... though it doesn't explain why 'full' also works in the case of a new resource or a PUT on an existing resource (i.e. the resource is in the template) and "not full" doesn't - timing is the same and it's not just that property that's different between the two there are a lot of differences.

We may be cheating in the deployment engine and relying on the PUT response and not actually doing a GET (since the resource is in the template). In the existing case we have no choice but to issue the GET. publicIpAddresses exhibit a similar behavior when dynamic alloc is used. If that's the case the simple un-optimization is probably worth the user comfort in these scenarios.

alex-frankel commented 2 years ago

Alex to include minimal repros and correlation IDs for both the success and failure cases.

alex-frankel commented 2 years ago

I'm very confident at this point as to the root cause of the issue which is whether or not the API version is included and therefore which REST verb you will use when you make the request (PUT or GET).

From the bicep perspective, is there anything we can do about this @anthony-c-martin / @majastrz? Can we do something like "always include the api version for all usage of reference()" so that we always do a GET request? I guess that could be a breaking behavior change..

Otherwise, seems the right thing to do is update the ARM RPC and enforce that PUT response == GET response.

shenglol commented 2 years ago

Btw, resolving reference() without API version doesn't always use PUT responses. If the referenced resource supports asynchronous PUT, ARM will use the final successful GET response to evaluate the reference function.

majastrz commented 2 years ago

We discussed this in triage today and the plan is to make changes in the what-if engine to deal with API versions and will restore the original bicep codegen behavior to always emit the API version for reference() calls.

bmoore-msft commented 2 years ago

@majastrz - just need to make sure we have enough of the scheduling fixed that we don't trigger the reference() too early - we've knocked out a ton of these recently, not sure if we've fixed all of them.

It's a breaking change too, isn't it?