bicep icon indicating copy to clipboard operation
bicep copied to clipboard

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

Open anthony-c-martin opened this issue 4 years ago • 11 comments

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.'."
}

anthony-c-martin avatar Aug 11 '21 17:08 anthony-c-martin

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

StefanIvemo avatar Aug 18 '21 17:08 StefanIvemo

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

anthony-c-martin avatar Aug 18 '21 17:08 anthony-c-martin

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.

anthony-c-martin avatar Aug 25 '21 20:08 anthony-c-martin

this is how we tell users to workaround it currently

bmoore-msft avatar Aug 25 '21 20:08 bmoore-msft

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?

anthony-c-martin avatar Aug 25 '21 20:08 anthony-c-martin

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?

bmoore-msft avatar Aug 25 '21 23:08 bmoore-msft

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 : ''
  }
}

AlexanderSehr avatar Feb 20 '22 15:02 AlexanderSehr

Still an issue

eduards-vavere avatar Jan 18 '23 09:01 eduards-vavere

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.

jtomkiew-mng avatar Feb 26 '24 18:02 jtomkiew-mng

Still an issue.

mwillebrands avatar Mar 15 '24 07:03 mwillebrands

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.

alex-frankel avatar Apr 22 '24 19:04 alex-frankel