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

Only add an explicit dependency on an existing resource when the deployments engine will use the GET response #15447

Merged
merged 5 commits into from
Nov 1, 2024

Conversation

jeskew
Copy link
Member

@jeskew jeskew commented Oct 30, 2024

Resolves #13674

This PR updates Bicep's resource dependency inference to only record a direct dependency on an existing resource when the deployment will read data from the resource's remote state. In other cases, such as when a template author reads the id or name property or calls a function like listKeys() or getSecret() on the resource, the ResourceDependencyVisitor will instead take a direct dependency on the existing resource's dependencies.

This update will have the effect of only reading the state of existing resources when it is actually used. The ARM backend performs tree shaking on the template and will only create GET jobs for existing resources that are depended on by other resources (whether implicitly or explicitly). There are two observable behavioral differences that result from this:

  • A template using language version 2.0 will no longer require /read permissions on an existing resource unless it is actually read. This is particularly useful for KeyVault secret parameters, which require an existing resource and the .getSecret function, as it is not uncommon for a KeyVault to permit usage in ARM deployments but not grant /read permissions to the deploying principal. This causes templates compiled to languageVersion 2.0 to exhibit the same behavior as templates compiled to non-symbolic name templates.
  • A template using language version 2.0 can have multiple existing resources pointing to the same resource ID so long as at most one of them is read from. (ARM's tree shaking of existing resources happens before the duplicate check.) There are still some cases where languageVersion: 2.0 behavior will diverge from a non-symbolic name template, but this change will bring their behavior much closer in line with each other. The following is an example of a template that would cause an error in language version 2.0 but raise no issues with a non-symbolic name template:
param saName string

resource sa1 'Microsoft.Storage/storageAccounts@2023-05-01' = {
  name: saName
  location: resourceGroup().location
  sku: {
    name: 'Standard_GRS'
  }
  kind: 'StorageV2'
}

resource sa2 'Microsoft.Storage/storageAccounts@2023-05-01' = {
  name: saName
  location: resourceGroup().location
  sku: {
    name: 'Standard_GRS'
  }
  kind: 'StorageV2'
}

output out1 string = sa1.properties.primaryEndpoints.blob
output out2 string = sa2.properties.primaryEndpoints.blob
  • Symbolic name users can still force that an existing resource be read and that its existence be checked before a separate resource is deployed by using an explicit dependency. This is an affordance only available in langauge version 2.0:
resource sa1 'Microsoft.Storage/storageAccounts@2023-05-01' existing = {
  name: 'first'
}

resource sa2 'Microsoft.Storage/storageAccounts@2023-05-01' = {
  dependsOn: [
    sa1 // <-- The sa2 deployment won't start unless sa1 exists and is readable. This is a no-op in a non-symbolic name template
  ]
  name: 'second'
  location: resourceGroup().location
  sku: {
    name: 'Standard_GRS'
  }
  kind: 'StorageV2'
}
Microsoft Reviewers: Open in CodeFlow

Copy link
Contributor

github-actions bot commented Oct 30, 2024

Test this change out locally with the following install scripts (Action run 11634323158)

VSCode
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 11634323158
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 11634323158"
Azure CLI
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 11634323158
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 11634323158"

Copy link
Contributor

github-actions bot commented Oct 30, 2024

Dotnet Test Results

    72 files   -     36      72 suites   - 36   28m 58s ⏱️ - 15m 52s
11 391 tests  -     13  11 391 ✅  -     13  0 💤 ±0  0 ❌ ±0 
26 513 runs   - 13 216  26 513 ✅  - 13 216  0 💤 ±0  0 ❌ ±0 

Results for commit 909f351. ± Comparison against base commit 74733c6.

♻️ This comment has been updated with latest results.

@jeskew jeskew force-pushed the jeskew/existing-dependencies branch from f520af1 to 909f351 Compare November 1, 2024 17:57
Copy link
Contributor

@shenglol shenglol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jeskew jeskew merged commit 1dcdf46 into main Nov 1, 2024
47 checks passed
@jeskew jeskew deleted the jeskew/existing-dependencies branch November 1, 2024 18:22
jeskew added a commit that referenced this pull request Nov 11, 2024
…the deployments engine will use the GET response" (#15524)

Reverts #15447 to address #15513 

There is a subtle bug in the way `dependsOn` is generated when a
non-copy resource (_r_) depends on a copy resource (_parent_) which
depends on a copy resource (_grandparent_) when _parent_ is not included
in the calculated dependency graph. Note that even with this reversion,
this scenario is still broken for non-symbolic name templates, but this
is not unique to v0.31.
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/15524)
jeskew added a commit that referenced this pull request Dec 3, 2024
#15580)

Resolves #15513

###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/15580)

Bicep will normally generate an explicit dependency when one resource
refers to another. For example, if the body of `b` includes a symbolic
reference to `a`, then in the compiled JSON template, the declaration
for `b` will have a `dependsOn` property that includes `a`.

However, if `a` is an `existing` resource and the template is not being
compiled to language version 2.0, then the compiler will "skip over" `a`
and have `b` depend on whatever `a` depends on. For example, for the
following template:

```bicep
resource a 'type@version' existing = {
  name: 'a'
  dependsOn: [
    c
  ]
}

resource b 'type@version' = {
  name: 'b'
  properties: {
    foo: a.properties.bar
  }
}

resource c 'type@version' = {
  name: 'c'
}
```

the non-symbolic name output will have `b` depend on `c`.

#15447 added a couple of scenarios in which Bicep would skip over an
existing resource even if compiling with symbolic name support. This was
done because the ARM backend will perform a `GET` request on any
`existing` resource in the template _unless_ its body properties are
never read and no deployed resource explicitly depends on it. The extra
`GET` requests could sometimes cause template deployments to fail, for
example if the deploying principal had permission to use secrets from a
key vault as part of a deployment but did not have more generic /read
permissions on the vault.

#15447 reused some existing logic for skipping over an intermediate
existing dependency that unfortunately had an underlying bug that
manifested when the skipped over resource was looped and used its loop
iterator to index into the dependency once removed. For example, if we
modify the earlier example slightly:

```bicep
resource a 'type@version' existing = [for i in range(0, 10): {
  name: 'a${i}'
  dependsOn: [
    c[i]
  ]
}]

resource b 'type@version' = {
  name: 'b'
  properties: {
    foo: [for i in range(0, 10): a[i].properties.bar]
  }
}

resource c 'type@version' = [for i in range(0, 10): {
  name: 'c${i}'
}]
```

Then in the compiled output, `b` will have an explicit dependency on
`[resourceId('type', format('c[{0}]', copyIndex()))]`. Because `b` is
not looped, the deployment will fail. Related issues will occur if `b`
indexes into `a` with a more complex expression or if there is an
intervening variable.

This PR updates explicit dependency generation to take all steps between
a depending resource and its dependency into account when generating
index expressions. For example, in the following template:

```bicep
resource vnets 'Microsoft.Network/virtualNetworks@2024-03-01' = [for i in range(0, 2): {
  name: 'vnet${i}'
}]

resource subnets 'Microsoft.Network/virtualNetworks/subnets@2024-03-01' existing = [for j in range(0, 10): {
  parent: vnets[j % 2]
  name: 'subnet'
}]

resource vault 'Microsoft.KeyVault/vaults@2023-07-01' = [for k in range(11, 10): {
  name: 'vault${k}'
  location: resourceGroup().location
  properties: {
    sku: {
      name: 'standard'
      family: 'A'
    }
    tenantId: subscription().tenantId
    networkAcls: {
      virtualNetworkRules: [{
        id: subnetIds[k - 11]
      }]
    }
  }
}]

var subnetIds = [for l in range(20, 10): subnets[l - 20].id]
```

`vault` will depend on `vnets[(range(20, 10)[k - 11] - 20) % 2]`. Prior
to this PR, `vault` will instead depend on `vnets[k % 2]`, which is the
wrong vnet.

This PR does **not** reapply the change from #15447 but only addresses
the issue described above. #15447 is reapplied in #15693
jeskew added a commit that referenced this pull request Dec 5, 2024
…oyments engine will use the GET response (#15693)

Resolves #13674 
Resolves #15686 

This PR reapplies the change from #15447 now that the bug in indexing
expression traversal is fixed.
###### Microsoft Reviewers: [Open in
CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/15693)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tree-shake existing resources where possible
2 participants