Azure / bicep

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

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

Open majastrz opened 3 years ago

majastrz commented 3 years ago

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

2. Run what-if
3. 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

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')]"
      ]
    }
  ]
}
ulfaxelssoncab commented 3 years ago

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...

alex-frankel commented 3 years ago

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
  }
}
ulfaxelssoncab commented 3 years ago

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...

alex-frankel commented 3 years ago

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?

mumian commented 3 years ago

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

mumian commented 3 years ago

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

majastrz commented 3 years ago

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.

aelij commented 3 years ago

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 commented 2 years ago

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

alex-frankel commented 2 years ago

@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.

aelij commented 2 years ago

@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.

alex-frankel commented 2 years ago

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.

aelij commented 2 years ago

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.

alex-frankel commented 2 years ago

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.

slavizh commented 2 years ago

@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.

alex-frankel commented 2 years ago

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

vishnu-anil commented 2 years ago

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.'."}
Fobermeyer commented 2 years ago

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
  }
guri-s commented 1 year ago

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": ""}}]}

jepperaskdk commented 6 months ago

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

kamilzzz commented 6 months ago

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.

alex-frankel commented 6 months ago

@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.

buysoft commented 6 months ago

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

psinnathurai commented 3 months ago

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
    }
  }
]
avivansh commented 3 months ago

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)

alex-frankel commented 3 months ago

@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.

avivansh commented 3 months ago

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