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

Tree-shake existing resources where possible #13674

Open
jeskew opened this issue Mar 21, 2024 · 3 comments
Open

Tree-shake existing resources where possible #13674

jeskew opened this issue Mar 21, 2024 · 3 comments
Labels
enhancement New feature or request
Milestone

Comments

@jeskew
Copy link
Contributor

jeskew commented Mar 21, 2024

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:

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

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

jeskew commented Apr 24, 2024

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

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: #13555 (comment)) in place, at the cost of duplicate GET calls to the same resource.

@slavizh
Copy link
Contributor

slavizh commented Apr 28, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Status: Todo
Development

No branches or pull requests

3 participants