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

Two mutually-exclusive conditional resources with the same type and name will throw name validation error #1410

Open
alex-frankel opened this issue Jan 29, 2021 · 24 comments · May be fixed by #15909
Assignees
Labels
intermediate language Related to the intermediate language Quality Sprint: Yes

Comments

@alex-frankel
Copy link
Collaborator

Bicep version
v0.2.348-g3c12faa1d2

Describe the bug
Right now, if I have two resources that are conditionally deployed such that they will never be deployed at the same time, they will validate as if they would be deployed together. So if if these two resources have the same type and name, the name duplication validation kicks in and throws an error.

Repro:

param createMode string {
  default: 'GeoRestore'
  allowed: [
    'GeoRestore'
    'Replica'
  ]  
}

resource postgres 'microsoft.dbforpostgresql/servers@2017-12-01' = if (createMode == 'GeoRestore') {
  name: 'foo'
  location: 'eastus'
  properties: {
    createMode: 'GeoRestore'
    sourceServerId: '<ID>'

  }
}

resource postgres2 'microsoft.dbforpostgresql/servers@2017-12-01' = if (createMode == 'Replica') {
  name: 'foo'
  location: 'eastus'
  properties: {
    createMode: 'Replica'
    sourceServerId: '<ID>'
  }
}

I'm not sure if this is really a bug, or more a reflection of a gap in bicep. If we had if/else (#1171), I don't think this would be an issue and actually a good thing as it would drive you towards a pit of success.

cc @miqm

@miqm
Copy link
Collaborator

miqm commented Jan 29, 2021

Ugh, I wrote that when if's weren't yet implemented...

Question is now how to mitigate it.

My proposal is to warn (or even just notify?) and not throw error when resource have this same name and a condition is applied on the resource.

@alex-frankel
Copy link
Collaborator Author

Warning is probably the right answer as long as we only do that when a condition is present and still throw an error when there are no conditions.

@miqm
Copy link
Collaborator

miqm commented Feb 1, 2021

ok, I'll handle this shortly, assign this to me please.

@alex-frankel alex-frankel added bug Something isn't working and removed Needs: Triage 🔍 labels Feb 1, 2021
@anthony-c-martin
Copy link
Member

At some point in the future, perhaps when we've got more advanced constant folding support, it would be awesome to be able to perform some flow analysis - in the above case, it should be possible to understand programatically that blocks A & B are mutually exclusive.

I think we're a long way off from this currently however, and the current proposal sounds great!

@majastrz
Copy link
Member

majastrz commented Feb 1, 2021

On the other hand, supporting if...elseif...elseif....elseif..........else would allow the user to express their intent regardless of us detecting the expressions are mutually exclusive.

@miqm
Copy link
Collaborator

miqm commented Feb 1, 2021

On the other hand, supporting if...elseif...elseif....elseif..........else would allow the user to express their intent regardless of us detecting the expressions are mutually exclusive.

switch perhaps?

@majastrz
Copy link
Member

majastrz commented Feb 1, 2021

yup!

@miqm
Copy link
Collaborator

miqm commented Feb 1, 2021

Hmm, we've a problem...
I implemented warning but when I tried to deploy generated template:

param createMode string {
  default: 'v1'
  allowed: [
    'v1'
    'v2'
  ]
}

resource vnet1 'Microsoft.Network/virtualNetworks@2020-06-01' = if (createMode == 'v1') {
  name: 'foo'
  location: 'northeurope'
  properties: {
    addressSpace: {
      addressPrefixes: [
        '10.0.100.0/24'
      ]
    }
  }
}

resource vnet2 'microsoft.Network/virtualNetworks@2020-06-01' = if (createMode == 'v2') {
  name: 'foo'
  location: 'northeurope'
  properties: {
    addressSpace: {
      addressPrefixes: [
        '10.0.200.0/24'
      ]
    }
  }
}
{
  "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
  "contentVersion": "1.0.0.0",
  "parameters": {
    "createMode": {
      "type": "string",
      "defaultValue": "v1",
      "allowedValues": [
        "v1",
        "v2"
      ]
    }
  },
  "functions": [],
  "resources": [
    {
      "condition": "[equals(parameters('createMode'), 'v1')]",
      "type": "Microsoft.Network/virtualNetworks",
      "apiVersion": "2020-06-01",
      "name": "foo",
      "location": "northeurope",
      "properties": {
        "addressSpace": {
          "addressPrefixes": [
            "10.0.100.0/24"
          ]
        }
      }
    },
    {
      "condition": "[equals(parameters('createMode'), 'v2')]",
      "type": "Microsoft.Network/virtualNetworks",
      "apiVersion": "2020-06-01",
      "name": "foo",
      "location": "northeurope",
      "properties": {
        "addressSpace": {
          "addressPrefixes": [
            "10.0.200.0/24"
          ]
        }
      }
    }
  ]
}

it failed with error: {"error":{"code":"InvalidTemplate","message":"Deployment template validation failed: 'The resource 'Microsoft.Network/virtualNetworks/foo' at line '16' and column '5' is defined multiple times in a template. Please see https://aka.ms/arm-template/#resources for usage details.'.","additionalInfo":[{"type":"TemplateViolation","info":{"lineNumber":16,"linePosition":5,"path":"properties.template.resources[0]"}}]}}

Seems that conditions are evaluated at deployment stage, while the name checking is on validation stage.

I recently had similar issue that validation was not passing because of missing property value - but I wanted exclude deployment entire resource if property was not set - so I needed to add in the property some dummy value for the case when condition was not met.

It seems like here would be similar case.
To solve it, first option is that developer would need to use interpolation and add some 'inactive' suffix to the name of the resources for the case that is opposite to the resource condition - resource with that name would never be deployed but will pass validation.
Second option is as error message says - use modules.

Either way - it's not a bug. Or more precisely - won't fix (unfortunately).

@alex-frankel
Copy link
Collaborator Author

ah, of course. The deployments engine (quite frustratingly) evaluates the resource even if the condition for deployment is false. We have a work item to stop this from happening. So technically, this was not a bug -- it correctly validated. We will need to revisit this once we have done our side of the work in the Deployments Service. Thanks for doing the investigation and sorry about the goose chase.

@alex-frankel alex-frankel added revisit and removed bug Something isn't working labels Feb 1, 2021
@anthony-c-martin anthony-c-martin added the intermediate language Related to the intermediate language label Feb 1, 2021
@miqm
Copy link
Collaborator

miqm commented Feb 2, 2021

For the record, implementation of warning on conditional is on my fork: https://github.com/miqm/bicep/tree/bugfix/duplicate-names-with-conditions-1410 for future pickup.

@miqm
Copy link
Collaborator

miqm commented Mar 15, 2021

@alex-frankel - do you have some ETA on the Deployments Service to improve conditionals on ARM side? basically whenever I want to have several resources behind same conditional (i.e. featureEnabled) and they use each other, in each property usage I need to add conditional (even when entire resource have if), and it's frustrating.

@alex-frankel
Copy link
Collaborator Author

The PR is out, but we have some prerequisite work that still needs to be completed. I'd guess ~3 months.

@brwilkinson
Copy link
Collaborator

seems like an old thread, however ... just fyi, since I don't think it was mentioned above, current workaround is giving them conditional names of the resources, so they are unique, however when it gets deployed, either way it has the name that you wanted.

resource LBalancerSS 'Microsoft.Network/loadBalancers@2020-07-01' = if ((length(NATRules) != 0)) {
  name: '${Deployment}${((length(NATRules) != 0) ? '-lb' : 'na')}${LB.LBName}'
resource LBalancer 'Microsoft.Network/loadBalancers@2020-07-01' = if (length(NATRules) == 0) {
  name: '${Deployment}${((length(NATRules) == 0) ? '-lb' : 'na')}${LB.LBName}'

@miqm miqm removed their assignment Sep 15, 2021
@farroar
Copy link

farroar commented Dec 10, 2021

Checking in on this one too. As I understand, the conditional statements are ignored until deployment. This make it not possible to use conditional logic to deploy X or Y when X and Y have common elements since both blocks of code are evaluated as if the conditional doesn't exist at all, correct?

I'm running into this with where I want to deploy a series of subnets with optional additional elements based on Booleans.

For example:

@batchSize(1)
resource vnet_subnets_rt 'Microsoft.Network/virtualNetworks/subnets@2021-02-01' = [for subnet in subnets: if (subnet.route_table) {
  name: '${vnet.name}/${subnet.name}'
  properties: {
    addressPrefix: subnet.prefix

    routeTable: {
      id: '${routetable_id_substring}/${subnet.name}-routetable'
    }
  }
}]


@batchSize(1)
resource vnet_subnets 'Microsoft.Network/virtualNetworks/subnets@2021-02-01' = [for subnet in subnets: if (!subnet.route_table) {
  name: '${vnet.name}/${subnet.name}'
  properties: {
    addressPrefix: subnet.prefix
  }
}]

will be evaluated as if the conditionals weren't there, BUT if it were to get to deployment then those same conditionals would do their job, correct?

I know there are different ways to achieve this, but I need this to be able to deal with a variable length array of elements.

Basically, trying to use logic to deploy subnets differently depending on if a route table needs to be associated.

Any updates on when this might be addressed? The workaround will force me to break up my array.

@farroar
Copy link

farroar commented Dec 13, 2021

@brwilkinson

Thanks for the example, very useful. However, I'm running into an issue because I'd like to have a route table for each subnet. Your example has one global route table that is assigned as needed. The line:

contains(sn, 'Route') && bool(sn.Route) ? RouteTableGlobal : null

I need to find a way to make 'RouteTableGobal' variable based on the subnet. Since the actual variable you define:

var RouteTableGlobal = {
  id: resourceId(Global.HubRGName, 'Microsoft.Network/routeTables/', '${replace(Global.hubVNetName, 'vn', 'rt')}${Domain}${Global.RTName}')
}

is in a dictionary, I don't know how I'd make that variable.

Really appreciate any thoughts you may have, I'll keep on with it.

@brwilkinson
Copy link
Collaborator

brwilkinson commented Dec 13, 2021

yes, that makes sense.

In the same file. I do that for the NSG's.

https://github.com/brwilkinson/AzureDeploymentFramework/blob/main/ADF/bicep/VNET.bicep#L102

Here is the separate NSG template:

https://github.com/brwilkinson/AzureDeploymentFramework/blob/main/ADF/bicep/NSG.bicep

@farroar
Copy link

farroar commented Dec 13, 2021

Very helpful! WiFi High Five :)

Bicep is a funky beast, interesting how you had to deal with the NSG logic.

@brwilkinson
Copy link
Collaborator

@farroar

Right, 😊

did you mean this part? In this part i always have a 1 to 1 map between NSG and subnet, so loop over the same object array, then just attach it by the id, I use a comment to allow multiline, just for formatting.

        "SubnetInfo": [
          {
            "name": "snMT01",
            "prefix": "0/27",
            "NSG": 1,

I like to be able to enable/disable attaching the NSG via the true/false or 0/1 above on each subnet.

        networkSecurityGroup: ! (contains(sn, 'NSG') && bool(sn.NSG)) ? null : /*
        */  {
              id: NSG[index].id
            }

or this part? In this part I merge user rules with default rules.. :)

var subnetInfo = contains(DeploymentInfo, 'subnetInfo') ? DeploymentInfo.subnetInfo : []
  
var NSGInfo = [for (subnet, index) in subnetInfo: {
  match: ((Global.CN == '.') || contains(Global.CN, subnet.name))
  subnetNSGParam: contains(subnet, 'securityRules') ? subnet.securityRules : []
  subnetNSGDefault: contains(NSGDefault, subnet.name) ? NSGDefault[subnet.name] : []
}]

var NSGDefault = {
  AzureBastionSubnet: [
...

@farroar
Copy link

farroar commented Dec 13, 2021

The first part, however VS Code gave me a hard time on the comment line splitting.

I found it interesting that you used the bang (!) to negate the logic so you could place the null in front. Fun code gymnastics to make formatting work.

A lot of what you have working has to do with how you are presenting the variables. I'm finding that I need to spend a lot more time thinking about that with Bicep. I use Terraform and Python a lot and I'm used to that flexibility.

If the API could just be updated to allow additive subnets
Azure/azure-quickstart-templates#2786

@brwilkinson
Copy link
Collaborator

@farroar oh yeah, agree on all of those ✅

@mleneveut
Copy link

any updates on this ?

{
              "type": "Microsoft.KeyVault/vaults",
              "apiVersion": "2016-10-01",
              "condition": "[or(equals(parameters('env'), 'dev'), equals(parameters('env'), 'rec'))]",
              "name": "[variables('billingKeyVaultName')]",
              "location": "[parameters('location')]",
              "properties": {
                "accessPolicies": [
                  {...a...}
              }
}
{
              "type": "Microsoft.KeyVault/vaults",
              "apiVersion": "2016-10-01",
              "condition": "[equals(parameters('env'), 'prd')]",
              "name": "[variables('billingKeyVaultName')]",
              "location": "[parameters('location')]",
              "properties": {
                "accessPolicies": [
                  {...a...}, {...b...}
              }
}

'The resource 'Microsoft.KeyVault/vaults/energy-billing-dev-kv' at line '1' and column '36421' is defined multiple times in a template

@jeskew
Copy link
Contributor

jeskew commented Feb 2, 2024

Stumbled across this while looking into a linked issue. The ARM engine has been updated to support mutually exclusive resource declarations (the "The resource 'X' ... is defined multiple times in a template" validation error will only be returned if multiple resource declarations with a true condition target the same resource ID), so I think we just need to update the check in Bicep.

Retagging for triage.

@anthony-c-martin
Copy link
Member

Related to #3750

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intermediate language Related to the intermediate language Quality Sprint: Yes
Projects
Status: In Review
Development

Successfully merging a pull request may close this issue.

9 participants