bicep icon indicating copy to clipboard operation
bicep copied to clipboard

In deployments runtime, do not check properties of resources with a `false` condition

Open majastrz opened this issue 3 years ago • 28 comments

Bicep version v0.3.255

Describe the bug In certain cases, we generate invalid reference() calls in the JSON.

To Reproduce

  1. Start with this bicep file:
resource one 'Microsoft.Storage/storageAccounts@2019-06-01' = if(false) {
  kind: 'StorageV2'
  name: 'majastrzfalse'
  location: resourceGroup().location
  sku: {
    name: 'Standard_LRS'
  }
}

resource two 'Microsoft.Storage/storageAccounts@2019-06-01' = {
  kind: 'StorageV2'
  name: 'majastrzfalse2'
  location: resourceGroup().location
  sku: {
    name: 'Standard_LRS'
  }
  properties: {
    accessTier: one.properties.accessTier
  }
}
  1. Run what-if
  2. See error:
New-AzResourceGroupDeployment :
InvalidTemplate - Long running operation failed with status 'Failed'. Additional Info:'Deployment template validation
failed: 'The template resource 'Microsoft.Storage/storageAccounts/majastrzfalse2' reference to
'Microsoft.Storage/storageAccounts/majastrzfalse' requires an API version. Please see https://aka.ms/arm-template for
usage details.'.'
At line:1 char:1
+ New-AzResourceGroupDeployment -Name empty -ResourceGroupName majastrz ...
+ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
    + CategoryInfo          : CloseError: (:) [New-AzResourceGroupDeployment], ResourceManagerCloudException
    + FullyQualifiedErrorId : Microsoft.Azure.Commands.ResourceManager.Cmdlets.Implementation.NewAzureResourceGroupDep
   loymentCmdlet

Additional context Compiled JSON:

{
  "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
  "contentVersion": "1.0.0.0",
  "metadata": {
    "_generator": {
      "name": "bicep",
      "version": "0.3.255.40792",
      "templateHash": "13339784985949539225"
    }
  },
  "functions": [],
  "resources": [
    {
      "condition": "[false()]",
      "type": "Microsoft.Storage/storageAccounts",
      "apiVersion": "2019-06-01",
      "name": "majastrzfalse",
      "kind": "StorageV2",
      "location": "[resourceGroup().location]",
      "sku": {
        "name": "Standard_LRS"
      }
    },
    {
      "type": "Microsoft.Storage/storageAccounts",
      "apiVersion": "2019-06-01",
      "name": "majastrzfalse2",
      "kind": "StorageV2",
      "location": "[resourceGroup().location]",
      "sku": {
        "name": "Standard_LRS"
      },
      "properties": {
        "accessTier": "[reference(resourceId('Microsoft.Storage/storageAccounts', 'majastrzfalse')).accessTier]"
      },
      "dependsOn": [
        "[resourceId('Microsoft.Storage/storageAccounts', 'majastrzfalse')]"
      ]
    }
  ]
}

majastrz avatar Apr 23 '21 01:04 majastrz

Yes, this is the second example with if:s in the ARM docs https://docs.microsoft.com/sv-se/azure/azure-resource-manager/templates/template-functions-logical?tabs=json#if.

The bicep tab in the documentation still states that conditions are not supported at all.

And if I put this (working with an optional KV in my case) value: '[if(equals(parameters(\'environment\'), \'dev\'), reference(resourceId(\'Microsoft.KeyVault/vaults\', variables(\'keyVaultName\'))).vaultUri, json(\'null\'))]'

bicep build turns that initial [ into [[ in the json file, the rest of it is kept as is though...

ulfaxelssoncab avatar Apr 29 '21 14:04 ulfaxelssoncab

The bicep tab in the documentation still states that conditions are not supported at all. @tfitzmac / @mumian - can we get this updated?

@ulfaxelssoncab - you shouldn't need to use ARM expressions. This is at least partially a bug in our ARM Template codegen, as we are not using the 'Full' argument on the reference call. In the meantime, does this work as a workaround? This should be the equivalent of the ARM-style expression you wrote:

var falseThing = false

resource one 'Microsoft.Storage/storageAccounts@2019-06-01' = if(falseThing) {
  kind: 'StorageV2'
  name: 'majastrzfalse'
  location: resourceGroup().location
  sku: {
    name: 'Standard_LRS'
  }
}

resource two 'Microsoft.Storage/storageAccounts@2019-06-01' = {
  kind: 'StorageV2'
  name: 'majastrzfalse2'
  location: resourceGroup().location
  sku: {
    name: 'Standard_LRS'
  }
  properties: {
    accessTier: falseThing ? one.properties.accessTier : null
  }
}

alex-frankel avatar May 03 '21 16:05 alex-frankel

Yes, thank you, that works just like the ARM-style expression, without the double [ problem.

Although it does yield a warning: The property "value" expected a value of type "string" but the provided value is of type "null | string".

Possibly null should be considered equivalent of all other types? Or at least in this case.

Since all the examples for the ternary operator in the documentation/tutorial use a "static" bool I did not even consider trying to use an expression as the argument there...

ulfaxelssoncab avatar May 03 '21 17:05 ulfaxelssoncab

Although it does yield a warning: The property "value" expected a value of type "string" but the provided value is of type "null | string".

Possibly null should be considered equivalent of all other types? Or at least in this case.

Right, this is a separate issue with our type system that we are looking to resolve. @majastrz do you know if we have a good issue tracking this?

alex-frankel avatar May 04 '21 15:05 alex-frankel

@alex-frankel - you can assign this one to me.

mumian avatar May 04 '21 18:05 mumian

Add the Bicep conditions example - https://github.com/MicrosoftDocs/azure-docs-pr/pull/157356

mumian avatar May 05 '21 15:05 mumian

Although it does yield a warning: The property "value" expected a value of type "string" but the provided value is of type "null | string". Possibly null should be considered equivalent of all other types? Or at least in this case.

Right, this is a separate issue with our type system that we are looking to resolve. @majastrz do you know if we have a good issue tracking this?

This part has been fixed with #2664.

majastrz avatar May 26 '21 00:05 majastrz

If I understand correctly, the PR fix returns a null value in cases where the resource is not deployed. But this may not always be the desired behavior. For example. there are resources that need to be deployed just once (e.g. vnets, MIs), but still need to be referenced in future deployments.

aelij avatar Jul 21 '21 09:07 aelij

Could this fix please be prioritized? It's causing hard-to-detect bugs in our deployments.

aelij avatar Feb 21 '22 09:02 aelij

@aelij - can you clarify your expectations for a fix here? It sounds like you are looking for one of the proposals in #3750 to be implemented. Does that sound right?

The "fix" is to make sure you are handling the potentially null value for the resource reference, but the issue is that we don't flag those cases for you while you are editing.

alex-frankel avatar Mar 09 '22 23:03 alex-frankel

@alex-frankel I'm not sure #3750 is the right solution here. Even if the resource is conditionally deployed, we may still want to reference its properties (for deploy only once resources). I would expect Bicep to always emit the API version with reference() to a resource.

aelij avatar Mar 10 '22 06:03 aelij

Can you clarify more what a "deploy once" resource is? Is that due to some RP idempotency/API design issues? Some of that is discussed in #4023. Is that more of what you are talking about?

The intention in Bicep/ARM is for you to say "deploy this resource or do not", which is slightly different than "deploy once". The expectation is all the APIs are idempotent and it shouldn't matter if you are deploying it once or one hundred times. If deploying a second time causes issues, then we need to fix the relevant API.

alex-frankel avatar Mar 10 '22 16:03 alex-frankel

There are 3 types of cases I can think (possibly more exist):

  1. Non-idempotent resources (e.g. VNet)
  2. Connected resources that you can BYO and afterwards managed by another resource (e.g. App Gateway for AKS)
  3. Resources that are costly to redeploy each time and so are only deployed conditionally during specialized deployments, such as region buildouts (e.g. deployment scripts)

Unfortunately I think we are quite far from the ideal of ARM's intention, and it might take years to fix as it involves many RPs. Providing us with proper tooling to work around those issues in the interim is most valuable.

aelij avatar Mar 10 '22 17:03 aelij

I think scenarios 2 and 3 are more valid, though I still think those are the result of poor API design and/or implementation. Either way, I think at a certain point we may need to give up on fixing APIs and add some "escape-hatch" type features to deal with it, but we need to be really careful in designing those, so they are not abused.

With #6163, we should no longer be emitting invalid reference calls, which means the bicep code should not fail during preflight validation. However, it is not going to fix #3750, for cases where we need the user to handle the null-ness. Those could still fail at runtime if the resource does not exist.

alex-frankel avatar Mar 11 '22 19:03 alex-frankel

@alex-frankel if it helps when we have this scenario the condition we have on the resource we put the same condition on the property/parameter. Of course on the property/parameter besides the condition we need to provide true/false values. On of those is the reference and the other is usually - null, '', [], or {} depending on the type of the property parameter.

slavizh avatar Mar 14 '22 13:03 slavizh

Changed title and linking to internal issue tracking this fix: https://msazure.visualstudio.com/One/_workitems/edit/4830897

As of #6163, we are no longer emitting invalid reference() syntax

alex-frankel avatar Mar 21 '22 21:03 alex-frankel

I have the same issue. Conditional resource creation is ignored and validation fails with the message . Is there a fix for this?

{"code":"InvalidTemplate","message":"Deployment template validation failed: 'The resource 'Microsoft.ContainerService/managedClusters/dataplane-eks' at line '292' and column '9' is defined multiple times in a template. Please see https://aka.ms/arm-template/#resources for usage details.'."}

vishnu-anil avatar Aug 24 '22 13:08 vishnu-anil

that would be my workaround which helped

output paramstate bool = deployGLBwithCert

module appGwPrivate 'appw-private-endpoint.bicep' =  if (deployGLBwithCert) {
  name: 'appGwPrivate'
  params: {
    appGwId: appgw.id
    location: location
    pepName: pepName
    subnetJumpSpokeId: subnetJumpSpokeId
  }
}
var varpepnicid = (deployGLBwithCert) ? appGwPrivate.outputs.pepNicId : 'nothing to add'
module appGwNicToIp 'appgw-private-nic-to-ip.bicep' =  if (deployGLBwithCert) {
  name: 'appgw-private-nic-to-ip'
  params: {
    pepNicId: varpepnicid
  }
}
var varappGwNicToIp = (deployGLBwithCert) ? appGwNicToIp.outputs.nicPrivateIp : 'nothing to add'
module appGwPrivateDns 'appw-private-dns.bicep' =    if (deployGLBwithCert) {
  name: 'appGwPrivateDns'
  scope: resourceGroup(vnetResourceGroupName)
  params: {
    apimName: apimName
    vnetSpokeId: vnetSpokeId
    pepIp: varappGwNicToIp
  }

Fobermeyer avatar Sep 15 '22 09:09 Fobermeyer

that would be my workaround which helped

output paramstate bool = deployGLBwithCert

module appGwPrivate 'appw-private-endpoint.bicep' =  if (deployGLBwithCert) {
  name: 'appGwPrivate'
  params: {
    appGwId: appgw.id
    location: location
    pepName: pepName
    subnetJumpSpokeId: subnetJumpSpokeId
  }
}
var varpepnicid = (deployGLBwithCert) ? appGwPrivate.outputs.pepNicId : 'nothing to add'
module appGwNicToIp 'appgw-private-nic-to-ip.bicep' =  if (deployGLBwithCert) {
  name: 'appgw-private-nic-to-ip'
  params: {
    pepNicId: varpepnicid
  }
}
var varappGwNicToIp = (deployGLBwithCert) ? appGwNicToIp.outputs.nicPrivateIp : 'nothing to add'
module appGwPrivateDns 'appw-private-dns.bicep' =    if (deployGLBwithCert) {
  name: 'appGwPrivateDns'
  scope: resourceGroup(vnetResourceGroupName)
  params: {
    apimName: apimName
    vnetSpokeId: vnetSpokeId
    pepIp: varappGwNicToIp
  }

This does not work. I get an error complaining about the deployment for the parent module is not defined if the condition is set to false & the parent module is not deployed

{"code": "InvalidTemplate", "message": "Deployment template validation failed: 'The resource 'subscriptions/xxxxxxx-xxxxxxxxx/resourceGroups/providers/Microsoft.Resources/deployments/appGwPrivate' is not defined in the template. Please see https://aka.ms/arm-syntax for usage details.'.", "additionalInfo": [{"type": "TemplateViolation", "info": {"lineNumber": 0, "linePosition": 0, "path": ""}}]}

guri-s avatar Apr 12 '23 02:04 guri-s

Does this mean that for-if statements do not work? I think it should be removed from the documentation then, until it is fixed - and this issue is already 3 years old?

https://learn.microsoft.com/en-us/azure/azure-resource-manager/bicep/loops#loop-with-condition

jepperaskdk avatar Mar 21 '24 20:03 jepperaskdk

Did anything change in that context recently?

We had a Bicep template deploying correctly for a few months but it recently suddenly started throwing errors during validation saying that a resource cannot reference itself.

Looks like the issue is that we use a resource definition with a condition which with a specific set of parameters evaluates to false, however validation engine ignores that and thinks we have self-referencing resources.

kamilzzz avatar Mar 25 '24 20:03 kamilzzz

@kamilzzz - can you share the repro of this problem in a separate issue? If it only started failing recently, it is a separate issue from this one.

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

I experienced the same this morning using this in my BICEP file:

module connectivityHubPrivateDNSZones './.../privatednszones.bicep' = if(initialSetup == false) {

Resulting in:

image

While my parameter is set to:

param initialSetup = true

buysoft avatar Apr 03 '24 09:04 buysoft

I am having the same Issue in this Case:

module subnet_NSGs 'br/public:avm/res/network/network-security-group:0.1.3' = [ for subnet in subnets: if (!contains(excludedSubnetNames, subnet.name)) {
    name: '${uniqueString(deployment().name)}-nsg_endpoint-${locationShortname}-${subnet.name}'
    params: {
      enableTelemetry: false
      name: 'nsg-${subnet.name}-${subscriptionShortname}-${environment}-${locationShortname}-01'
      securityRules: [] // endpoint rules
    }
  }
]

psinnathurai avatar Jun 18 '24 15:06 psinnathurai

Facing the same issue, It's not happening with all the resources, only with few of the resources

For example:-

module primaryAppGateway 'create_application_gateway.bicep' = if (environmentConfig.enableVcurrent) {
  scope: resourceGroup(environmentConfig.primaryResourceGroup.name)
  name: 'create-${primaryAppGatewayName}'
  params: {
    name: primaryAppGatewayName
    nsgName: environmentConfig.primaryResourceGroup.nsg
    region: environmentConfig.primaryRegion
    sizingConfig: sizingConfig.applicationGateway
    tags: union(tags, defaultTags)
    subnetName: '${environmentConfig.primaryResourceGroup.vnet}/${agwSubnet}'
  }
}

module vnextAppGateway 'create_application_gateway.bicep' = if (environmentConfig.enableVnext) {
  scope: resourceGroup(environmentConfig.vnextResourceGroup.name)
  name: 'create-${vnextAppGatewayName}'
  params: {
    name: vnextAppGatewayName
    nsgName: environmentConfig.vnextResourceGroup.nsg
    region: environmentConfig.primaryRegion
    sizingConfig: sizingConfig.applicationGateway
    tags: union(tags, defaultTags)
    subnetName: '${environmentConfig.vnextResourceGroup.vnet}/${agwSubnet}'
  }
}

In the above example enableVcurrent is set to false but still bicep is trying to create this and resulting into error Resource group 'rg-demo43-stag-primary-eastus' could not be found. (Code: ResourceGroupNotFound)

avivansh avatar Jun 25 '24 07:06 avivansh

@psinnathurai / @avivansh - can you try enabling symbolic names in bicepconfig.json and see if it resolves the issue?

"experimentalFeaturesEnabled": {
        "symbolicNameCodegen": true
    }

We are moving closer to turning on symbolic names by default for all bicep users and that is where we expect to resolve this particular issue. We believe we have fixed this issue when symbolic names are enabled, but want to make sure we are not missing any use cases.

alex-frankel avatar Jul 01 '24 14:07 alex-frankel

@alex-frankel, thanks for sharing this. It worked for me.

avivansh avatar Jul 01 '24 17:07 avivansh