Azure / bicep

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

Missing validation for condition-false `reference()` statements #3990

Open anthony-c-martin opened 3 years ago

anthony-c-martin commented 3 years ago

The following Bicep file compiles just fine (with warnings, because of the property I made up), but fails ARM validation as the reference(...) function is evaluated regardless of the condition on the resource:

var condition = false

resource test 'Microsoft.Storage/storageAccounts@2021-04-01' = if (condition) {
  name: 'uniquename12982'
  kind: 'StorageV2'
  sku: {
    name: 'Standard_LRS'
  }
  location: 'West US'
}

resource test2 'Microsoft.Storage/storageAccounts@2021-04-01' = if (condition) {
  name: 'uniquename11298'
  kind: 'StorageV2'
  sku: {
    name: 'Standard_LRS'
  }
  location: 'West US'
  properties: {
    someProp: test.properties.someProp
  }
}

To fix it, change:

    someProp: test.properties.someProp

To:

    someProp: condition ? test.properties.someProp : null

This isn't made at all clear however. The template validation fails with:

{
  "code": "InvalidTemplate",
  "message": "Deployment template validation failed: 'The template resource 'Microsoft.Storage/storageAccounts/uniquename11298' reference to 'Microsoft.Storage/storageAccounts/uniquename12982' requires an API version. Please see https://aka.ms/arm-template for usage details.'."
}
StefanIvemo commented 3 years ago

When using modules this issue becomes even more confusing for the users. This template will fail with no indication for the user on what is actually wrong.

targetScope = 'subscription'

param deploy bool = false
param location string = 'eastus'

resource rg 'Microsoft.Resources/resourceGroups@2021-04-01' = if (deploy) {
  name: 'rg2'
  location: location
}

module nsg './nsg.bicep' = if (deploy) {
  name: 'nsg-deploy'
  params: {
    name: 'nsg2'
    location: location
  }
  scope: rg
}

module vnet './vnet.bicep' = if (deploy) {
  name: 'vnet-deploy'
  params: {
    vnetName: 'vnet2'
    location: location    
    nsgId: nsg.outputs.resourceId
  }
  scope: rg
}

Deployment error:

"error": {
        "code": "ResourceGroupNotFound",
        "message": "Resource group 'rg2' could not be found."
    }

And since it's a nested deployment it will look like there is an error with the NSG deployment when the reference that is causing the error is in the VNet deployment.

image

anthony-c-martin commented 3 years ago

Feels like this should potentially be a top-10-bug. We'll discuss in the next triage.

anthony-c-martin commented 3 years ago

A cheap solution which we could use to workaround the limitation in the Deployment engine would be to codegen an if() statement inside any conditional resource blocks:

E.g. for:

resource test2 'Microsoft.Storage/storageAccounts@2021-04-01' = if (condition) {
  name: 'uniquename11298'
  kind: 'StorageV2'
  sku: {
    name: 'Standard_LRS'
  }
  location: 'West US'
  properties: {
    someProp: test.properties.someProp
  }
}

Instead of generating the following expression: [reference('...').someProp]

We could potentially generate: [if(condition, reference('...').someProp, null())]

This should be safe because the condition would match that of the resource, e.g. in the template JSON:

{
  "name": "...",
  "type": "...",
  "apiVersion": "...",
  "condition": "<condition_here>",
  "properties": {
    "someProp": "[if(<condition_here>, reference('...').someProp, null())]"
  }
  ...
}

So that in practice, if the resource condition is true, the inner if() will never actually evaluate to null(), but it blocks preflight from trying to evaluate it.

bmoore-msft commented 3 years ago

this is how we tell users to workaround it currently

anthony-c-martin commented 3 years ago

this is how we tell users to workaround it currently

Good to know! Can you think of any reasons not to do the above for all reference() statements inside conditional resources, other than making the JSON expressions harder to read?

bmoore-msft commented 3 years ago

no reason currently - we'd want to keep an eye on the path where null would be used and something sneaks past the point in deployment where null matters. I don't think that's likely but realize I'm making a statement like "no one will ever need more than 640K".

we should also be careful about letting this fix make us lazy about fixing the root cause in the platform... so another conversation to provoke is should we do it in the deployment engine instead of bicep?

AlexanderSehr commented 2 years ago

Seems to be related to #2371

In any case, I can confirm this is still an issue and it would be awesome if it would eventually be fixed.

My current case: MSI deployment + Role Assignment. As per Alex's suggestion, I duplicated the condition now in the msi_rbac block.

@description('Optional. A parameter to control which deployments should be executed')
@allowed([
  'All'
  'Only infrastructure'
  'Only Storage & Image'
  'Only image'
])
param deploymentsToPerform string = 'Only Storage & Image'

// User Assigned Identity (MSI)
module msi 'br/modules:microsoft.managedidentity.userassignedidentities:0.4.735' =  if(deploymentsToPerform == 'All' || deploymentsToPerform == 'Only infrastructure') {
  name: '${deployment().name}-msi'
  scope: resourceGroup(rgParam.name)
  params: {
    name: msiParam.name
  }
  dependsOn: [
    rg
  ]
}

// MSI Subscription contributor assignment
module msi_rbac '../CARML/Microsoft.Authorization/roleAssignments/subscriptions/deploy.bicep' =  if(deploymentsToPerform == 'All' || deploymentsToPerform == 'Only infrastructure') {
  name: '${deployment().name}-ra' 
  params: {
     roleDefinitionIdOrName: msiRoleAssignmentParam.roleDefinitionIdOrName

    // Ideal solution
    principalId: msi.outputs.principalId 
    // Results in: Deployment template validation failed: 'The template resource 'Microsoft.Resources/deployments/imageInfra.deploy-ra' reference to 'Microsoft.Resources/deployments/imageInfra.deploy-msi' requires an API version. Please see https://aka.ms/arm-template for usage details.'.
    // Compiles to: reference(extensionResourceId(format('/subscriptions/{0}/resourceGroups/{1}', subscription().subscriptionId, parameters('rgParam').name), 'Microsoft.Resources/deployments', format('{0}-msi', deployment().name))).outputs.principalId.value

    // Workaround via ARM reference
    principalId: reference(extensionResourceId(format('/subscriptions/{0}/resourceGroups/{1}', subscription().subscriptionId, rgParam.name), 'Microsoft.Resources/deployments', format('{0}-msi', deployment().name)),'2021-04-01').outputs.principalId.value

    // Workaround vial Alex's suggestion
    principalId: (deploymentsToPerform == 'All' || deploymentsToPerform == 'Only infrastructure') ? msi.outputs.principalId : ''
  }
}
eduards-vavere commented 1 year ago

Still an issue

jtomkiew-mng commented 6 months ago

Still an issue - encountered this today in the following case (simplified for brevity):

param enableLoadBalancer bool = false
// ...
module loadBalancer './modules/load-balancer.bicep' = if (enableLoadBalancer) {
  name: 'deploy-lb'
  params: {
    // ...
  }
}
module loadBalancerDnsRecord './modules/dns-record.bicep' = if (enableLoadBalancer) {
  name: 'deploy-dns'
  params: {
    // ...
    dnsRecordIpAddress: loadBalancer.outputs.frontendPrivateIPAddress // fails with "Deployment 'deploy-lb' could not be found."
  }
}

Needed to explicitly make loadBalancer.outputs.frontendPrivateIPAddress access conditional inside second module parameters:

module loadBalancerDnsRecord './modules/dns-record.bicep' = if (enableLoadBalancer) {
  name: 'deploy-dns'
  params: {
    // ...
    dnsRecordIpAddress: enableLoadBalancer ? loadBalancer.outputs.frontendPrivateIPAddress : '' // works
  }
}

Feels redundant and not intuitive, as the module using it was already conditional with the same condition.

mwillebrands commented 5 months ago

Still an issue.

alex-frankel commented 4 months ago

To anyone still facing the issue, you can workaround this by opting into the symbolicNameCodegen experimental feature. With this new ARM Template schema, the issue has been resolved.

We are going to discuss a plan for migrating all bicep-generated templates to this new format, at which point we will call this issue resolved.