Azure / bicep

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

Tree-shake `existing` resources where possible #13674

Open jeskew opened 5 months ago

jeskew commented 5 months ago

The semantics of reference() expressions vs "existing": true resources do not have a 1:1 correspondence in ARM, but the Bicep compiler uses them interchangeably. While ARM's existing resources offer additional functionality and IMHO enhanced readability, templates may not declare multiple existing resources that point to the same resource ID (while duplicative reference() calls are permitted), nor may templates use runtime expressions in resource top-level properties (which is permitted in some cases for expressions like resourceId() that overlap with the functionality offered by existing resources).

In order to minimize the number of breaking changes template authors might encounter when switching their compilation target to symbolic name templates, Bicep should try to "tree-shake" existing resources by not emitting "existing": true resources in the compiled JSON template unless the template:

We may eventually need a mechanism that forces an existing resource to be present in the compiled JSON, but there is no need for one now. This might look like:

@noinline()
resource foo '<type>@<version>' existing = {
jeskew commented 4 months ago

One issue encountered while working on this is that of the "non-inlinable" qualifiers listed above, i.e.

  • accesses a property of the resource other than apiVersion, type, id, or name,
  • calls a resource function other than getSecret(),
  • includes the resource in another resource's dependsOn property, or
  • includes a dependsOn property on the existing resource.

is that this list solves the problem in #13555 only because the property accessed in the provided example is .id. If the template were instead accessing a property only available via a GET request, then a user transitioning from non-symbolic name templates to language version 2.0 would experience a breaking change.

If the criteria for which resources must be represented in the compiled JSON as "existing": true resource declarations were restricted to the last two items, i.e.,:

  • includes the resource in another resource's dependsOn property, or
  • includes a dependsOn property on the existing resource.

then users would only experience a breaking change if they were previously relying on broken behavior (having a non-existing resource depend on an existing resource in a non-symbolic name template).

jurjenoskam commented 4 months ago

We've just ran into this problem as well. This example template builds and deploys just fine:

resource uami1 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-01-31' existing = {
  scope: resourceGroup('rg-subscription-default-resources')
  name: 'deployment-uami-${subscription().subscriptionId}'
}

resource uami2 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-01-31' existing = {
  scope: resourceGroup('rg-subscription-default-resources')
  name: 'deployment-uami-${subscription().subscriptionId}'
}

output uami1 object = uami1
output uami2 object = uami2

... but the moment you add a languageVersion 2.0 feature in there, template deployment will fail with an error stating that the userAssignedIdentity resource "is defined multiple times in a template". Simply adding a line like param foo string? is enough to trigger this behavior.

We ran into this problem because we're autogenerating our templates, and in some templates this results in referring to the same existing userAssignedIdentity using multiple symbolic names. This has worked just fine until now, as the Bicep existing resources did not result in ARM resources at all. Now that languageVersion 2.0 features are coming into play, things start to break.

While in our use case this problem would be fixed by the proposal made by this issue (as we're not querying any runtime properties of the resource), wouldn't it be possible to change ARM's behavior such that it would allow deploying duplicate resources, if and only if all of the duplicates have existing: true? That would keep the order of GET calls in the deployment graph (as described here: https://github.com/Azure/bicep/issues/13555#issuecomment-1994766688) in place, at the cost of duplicate GET calls to the same resource.

slavizh commented 4 months ago

This is bad that this issue exists even when the resource symbolic name (uami1 and uami2) is different. We often provide have scenarios where you define the user assigned identity so you can put it on the resource but also you have encryption and there you have to provide the user assigned identity as well if you choose to use it for encryption. That requires defining the same identity in two different existing statements.

maartengo commented 3 months ago

We just encountered the scenario of a broken deployment after adding a type import to our module, which converts it to languageVersion 2.0 automatically. Our deployment scenario:


param deployContainerApps bool

resource serviceBus 'Microsoft.ServiceBus/namespaces@2021-06-01-preview' = {
  name: resourceName
  location: location
  tags: tags
  sku: 'someSku'
  identity: {
    type: 'SystemAssigned'
  }
}

resource uami1 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-01-31' existing = {
  scope: resourceGroup('rg-subscription-default-resources')
  name: 'deployment-uami-${subscription().subscriptionId}'
}

module roleAssignment 'rbac.bicep' = if (deployContainerApps) {
  scope: resourceGroup('rg-subscription-default-resources')
  name: 'deployment-uami-${subscription().subscriptionId}'
  params: {
    roleGuid: '000-000-00-00-000-000'
    identity: deployContainerApps  ? uami1 : ''
    scope: serviceBus.id
  }
}

When we added a type import to this template we suddenly got the following error The resource 'Microsoft.ManagedIdentity/userAssignedIdentities/deployment-uami-...' under resource group '...' was not found. For more details please go to https://aka.ms/ARMResourceNotFoundFix. It took us quite some searching to figure out that this was caused by the added type import, and not an update to Azure/Bicep or something else.