Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
majastrz opened this issue Apr 23, 2021 · 27 comments
Assignees
Labels
bug Something isn't working intermediate language Related to the intermediate language Quality sprint: No top 10 committed
Milestone

Comments

@majastrz
Copy link
Member

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')]"
      ]
    }
  ]
}
@ghost ghost added the Needs: Triage 🔍 label Apr 23, 2021
@ulfaxelssoncab
Copy link

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 alex-frankel added bug Something isn't working investigate and removed Needs: Triage 🔍 labels Apr 29, 2021
@alex-frankel alex-frankel self-assigned this Apr 29, 2021
@alex-frankel
Copy link
Collaborator

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 alex-frankel added awaiting response documentation Improvements or additions to documentation labels May 3, 2021
@ulfaxelssoncab
Copy link

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
Copy link
Collaborator

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
Copy link
Contributor

mumian commented May 4, 2021

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

@alex-frankel alex-frankel assigned mumian and unassigned alex-frankel May 4, 2021
@mumian
Copy link
Contributor

mumian commented May 5, 2021

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

@majastrz
Copy link
Member Author

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.

@alex-frankel alex-frankel added this to the v0.5 milestone Jun 3, 2021
@aelij
Copy link
Member

aelij commented Jul 21, 2021

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
Copy link
Member

aelij commented Feb 21, 2022

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

@alex-frankel
Copy link
Collaborator

@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
Copy link
Member

aelij commented Mar 10, 2022

@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
Copy link
Collaborator

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
Copy link
Member

aelij commented Mar 10, 2022

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
Copy link
Collaborator

alex-frankel commented Mar 11, 2022

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.

@vishnu-anil
Copy link

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
Copy link

Fobermeyer commented Sep 15, 2022

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
Copy link

guri-s commented Apr 12, 2023

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
Copy link

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
Copy link

kamilzzz commented Mar 25, 2024

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.

@stephaniezyen stephaniezyen modified the milestones: v1.0, v1.1 Mar 28, 2024
@alex-frankel
Copy link
Collaborator

@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
Copy link

buysoft commented Apr 3, 2024

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
Copy link

psinnathurai commented Jun 18, 2024

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
Copy link

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
Copy link
Collaborator

@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
Copy link

avivansh commented Jul 1, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working intermediate language Related to the intermediate language Quality sprint: No top 10 committed
Projects
Status: Todo
Development

No branches or pull requests