Azure / bicep

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

`existing` resource names and scopes must be valid expressions (regardless of condition) after introducing user-defined-types to template #12204

Open AlexanderSehr opened 1 year ago

AlexanderSehr commented 1 year ago

Bicep version Bicep CLI version 0.22.6 (d62b94db31)

Describe the bug In CARML, for Customer-Managed-Keys we used to reference existing KeyVault resources like in the following example:

param resourceId string = ''

resource vault 'Microsoft.KeyVault/vaults@2021-10-01' existing = if (!empty(resourceId)) {
  name: last(split(resourceId, '/'))!
  scope: resourceGroup(split(resourceId, '/')[2], split(resourceId, '/')[4])
}

which worked like a charm. What I now noticed is, that as soon as I introduce a completely unrelated User-Defined-Type, the above scope statement starts breaking using the default value of ''.

The full example may look like

targetScope = 'subscription'

param resourceId string = ''

resource vault 'Microsoft.KeyVault/vaults@2021-10-01' existing = if (!empty(resourceId)) {
  name: last(split(resourceId, '/'))!
  scope: resourceGroup(split(resourceId, '/')[2], split(resourceId, '/')[4])
}

type lockType = {
  name: string?
  kind: ('CanNotDelete' | 'ReadOnly' | 'None')?
}?

So here is the error that appears if I deploy the above: The language expression property array index '2' is out of bounds...

The workaround is to change the resource to

resource vault 'Microsoft.KeyVault/vaults@2021-10-01' existing = if (!empty(resourceId)) {
  name: last(split((!empty(resourceId) ? resourceId : 'dummyVault'), '/'))!
  scope: resourceGroup(split((!empty(resourceId) ? resourceId : '//'), '/')[2], split(((!empty(resourceId) ? resourceId : '////')), '/')[4])
}

Originally I thought the reason is that user defined types work differently (hence user UDT implementation in AVM looks a bit crazy), but by now I believe it's a bug in the way the expression is handled.

To Reproduce Steps to reproduce the behavior:

Simply deploy the 'full example' shown above. In the same you can also test the workaround.

Additional context

A full test run is shown below. In between the two runs, I enabled the previously outcommented user defined type.

New-AzDeployment -TemplateFile 'C:\testScope.bicep' -Location 'WestEurope' -VErbose
VERBOSE: Using Bicep v0.22.6
WARNING: C:\testScope.bicep(5,10) : Warning no-unused-existing-resources: Existing resource "vault" is declared but never used. [https://aka.ms/bicep/linter/no-unused-existing-resources]
VERBOSE: 
VERBOSE: 21:05:41 - Template is valid.
VERBOSE: 21:05:42 - Create template deployment 'testScope'
VERBOSE: 21:05:42 - Checking deployment status in 5 seconds

Id                      : /subscriptions/<subId>/providers/Microsoft.Resources/deployments/testScope
DeploymentName          : testScope
Location                : westeurope
ProvisioningState       : Succeeded
Timestamp               : 19/10/2023 19:05:43
Mode                    : Incremental
TemplateLink            : 
Parameters              : 
                          Name             Type                       Value
                          ===============  =========================  ==========
                          resourceId       String                     ""

Outputs                 : 
DeploymentDebugLogLevel : 

> New-AzDeployment -TemplateFile 'C:\testScope.bicep' -Location 'WestEurope' -Verbose
VERBOSE: Using Bicep v0.22.6
WARNING: C:\testScope.bicep(5,10) : Warning no-unused-existing-resources: Existing resource "vault" is declared but never used. [https://aka.ms/bicep/linter/no-unused-existing-resources]
VERBOSE: 
New-AzDeployment: 21:06:03 - Error: Code=InvalidTemplate; Message=Deployment template validation failed: 'The template resource '' at line '43' and column '18' is not valid: The language expression property array index '2' is out of bounds.. Please see https://aka.ms/arm-functions for usage details.'.
New-AzDeployment: The deployment validation failed
jeskew commented 1 year ago

This isn't directly related to user-defined types, though you're right that using a type statement can trigger the behavior you've encountered. This is something that would need to be addressed in the ARM backend service, most likely by not evaluating the names or scoping properties of resource with a false condition.

I would recommend using the following resource declaration instead to work around current ARM limitations:

param resourceId string = ''

var idParts = split(resourceId, '/')

resource vault 'Microsoft.KeyVault/vaults@2021-10-01' existing = if (!empty(resourceId)) {
  name: !empty(resourceId) ? last(idParts) : 'dummyVault'
  scope: resourceGroup(idParts[?2] ?? 'dummySubscriptionId', idParts[?4] ?? 'dummyRgName')
}

Some background on this: There are two ways an existing resource may get represented in a JSON template:

The second option is used when Bicep is targeting what is known as "language version 2.0." There are a couple of things you can do that will cause Bicep to use this compilation mode, one of which is using user-defined types. You can also enable this compilation mode via a bicepconfig.json file.

The distinction between how existing resources get represented will only make a difference in a deployment when the existing resource is never used. Let's use your first snippet as an example:

param resourceId string = ''

resource vault 'Microsoft.KeyVault/vaults@2021-10-01' existing = if (!empty(resourceId)) {
  name: last(split(resourceId, '/'))!
  scope: resourceGroup(split(resourceId, '/')[2], split(resourceId, '/')[4])
}

When compiled, the resulting ARM JSON file will look like this:

{
  "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
  "contentVersion": "1.0.0.0",
  "metadata": {
    "_generator": {
      "name": "bicep",
      "version": "0.22.6.54827",
      "templateHash": "13410433678536827458"
    }
  },
  "parameters": {
    "resourceId": {
      "type": "string",
      "defaultValue": ""
    }
  },
  "resources": []
}

If we add an unused user-defined type:

type unused = string

param resourceId string = ''

resource vault 'Microsoft.KeyVault/vaults@2021-10-01' existing = if (!empty(resourceId)) {
  name: last(split(resourceId, '/'))!
  scope: resourceGroup(split(resourceId, '/')[2], split(resourceId, '/')[4])
}

The resulting JSON file will use an existing resource declaration:

{
  "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
  "languageVersion": "2.0",
  "contentVersion": "1.0.0.0",
  "metadata": {
    "_generator": {
      "name": "bicep",
      "version": "0.22.6.54827",
      "templateHash": "8872817320170724981"
    }
  },
  "definitions": {
    "unused": {
      "type": "string"
    }
  },
  "parameters": {
    "resourceId": {
      "type": "string",
      "defaultValue": ""
    }
  },
  "resources": {
    "vault": {
      "condition": "[not(empty(parameters('resourceId')))]",
      "existing": true,
      "type": "Microsoft.KeyVault/vaults",
      "apiVersion": "2021-10-01",
      "subscriptionId": "[split(parameters('resourceId'), '/')[2]]",
      "resourceGroup": "[split(parameters('resourceId'), '/')[4]]",
      "name": "[last(split(parameters('resourceId'), '/'))]"
    }
  }
}

Trying to get the item at index 2 from the return value of split('', '/') will always throw an error in the deployment engine, but as you can see, the first JSON template doesn't have a split expression anywhere because nothing refers to the vault resource.

You can force the version without a user-defined type to throw the same error by referring to a property of vault that requires access to the full resource ID:

param resourceId string = ''

resource vault 'Microsoft.KeyVault/vaults@2021-10-01' existing = if (!empty(resourceId)) {
  name: last(split(resourceId, '/'))!
  scope: resourceGroup(split(resourceId, '/')[2], split(resourceId, '/')[4])
}

output vaultId string = vault.id

which compiles to:

{
  "$schema": "https://schema.management.azure.com/schemas/2019-04-01/deploymentTemplate.json#",
  "contentVersion": "1.0.0.0",
  "metadata": {
    "_generator": {
      "name": "bicep",
      "version": "0.22.6.54827",
      "templateHash": "12140109393935145231"
    }
  },
  "parameters": {
    "resourceId": {
      "type": "string",
      "defaultValue": ""
    }
  },
  "resources": [],
  "outputs": {
    "vaultId": {
      "type": "string",
      "value": "[extensionResourceId(format('/subscriptions/{0}/resourceGroups/{1}', split(parameters('resourceId'), '/')[2], split(parameters('resourceId'), '/')[4]), 'Microsoft.KeyVault/vaults', last(split(parameters('resourceId'), '/')))]"
    }
  }
}
jeskew commented 1 year ago

Related to #10731

AlexanderSehr commented 1 year ago

Hey @jeskew, thank you for the extensive answer. I guess there is no chance that the UDT version would not trigger that infamous language version 2.0 to be picked up? Or is that language version even the latest one and is required for UDT?

Also thanks for providing another workaround. It certainly is a bit smaller than the one I came up with. It may still be a struggle for some of our modules as they reference sometimes several existing resources and or nested ones, but I'll give it a shot :)

In the meantime I keep my fingers crossed for version 3.0 🤞

alex-frankel commented 1 year ago

That language version is required for UDTs.

@jeskew would this be resolved with #2371? Seems the code @AlexanderSehr is safe in that the split() function should only be evaluated if resourceId is not an empty string?

jeskew commented 1 year ago

@alex-frankel Potentially, though I think for backwards compatibility reasons that behavior would require a deployment with an API version >= 2019-05-10. (Earlier API versions need to be able to calculate a valid resource ID for all resources in complete mode, not just those with a true condition.)

jeskew commented 1 week ago

The fix for this should be rolled out everywhere now. One behavioral difference you will still see with language version 2.0 is that an existing resource with a true or omitted condition will need to use valid expressions for their names and scope, whereas this isn't necessarily true in non-symbolic name templates. If the condition is false, though, invalid expressions should be fine.

AlexanderSehr commented 5 days ago

Hey @jeskew, I just tested if I can simplify our usage of existing resources in AVM now, and at least as of today I still face the same issues as before (using Test-AzDeployment):

[
  {
    "Code": "InvalidTemplate",
    "Message": "Deployment template validation failed: 'The template reference 'cMKKeyVault' is not valid: could not find template resource or resource copy with this name. Please see https://aka.ms/arm-function-reference for usage details.'.",
    "Target": "/subscriptions/<subId>/resourceGroups/dep-appconfiguration.configurationstores-accmin-rg/providers/Microsoft.Resources/deployments/6rzp6xjndwicw-test-accmin-init",
    "Details": null
  },
  {
    "Code": "InvalidTemplate",
    "Message": "Deployment template validation failed: 'The template reference 'cMKKeyVault' is not valid: could not find template resource or resource copy with this name. Please see https://aka.ms/arm-function-reference for usage details.'.",
    "Target": "/subscriptions/<subId>/resourceGroups/dep-appconfiguration.configurationstores-accmin-rg/providers/Microsoft.Resources/deployments/6rzp6xjndwicw-test-accmin-idem",
    "Details": null
  }
]

My attempt to 'simplify': Image

Source code: ref The file used for testing: ref (does not configure CMK, i.e., the object should be empty and not being evauated) Bicep version: v0.30.23

So either I'm missing something / doing something terribly wrong, or the issue is maybe not yet closed. As this is relevant for 30+ of the AVM modules, any advice would be welcome :)

jeskew commented 3 days ago

@AlexanderSehr thanks for the detailed report! There is a fix merged but it'll take a bit to roll out. I don't think we'll be able to do a hotfix since the affected templates have never been deployable.

Related to #15443

alex-frankel commented 3 days ago

@AlexanderSehr - if you are eager to try it out, you can install the nightly build: https://github.com/Azure/bicep/blob/main/docs/installing-nightly.md

AlexanderSehr commented 3 days ago

@AlexanderSehr thanks for the detailed report! There is a fix merged but it'll take a bit to roll out. I don't think we'll be able to do a hotfix since the affected templates have never been deployable.

Related to #15443

ah no rush, thanks @jeskew :) Given the issue was closed and a rollout was mentioned, my brain somehow did not connect the dots that this will require a Bicep version update rather than being something that magically works differently in the back 😄

slavizh commented 18 hours ago

@AlexanderSehr let me know how your tests went. Mine ended up pretty bad https://github.com/Azure/bicep/issues/15513

slavizh commented 15 hours ago

@AlexanderSehr the issues I have found were related to using extensibility feature.