Skip to content

Commit

Permalink
Move all indexing expressions when calculating dependsOn expressions (
Browse files Browse the repository at this point in the history
#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
  • Loading branch information
jeskew authored Dec 3, 2024
1 parent 2b8c6f9 commit 0602a35
Show file tree
Hide file tree
Showing 17 changed files with 1,239 additions and 188 deletions.
4 changes: 2 additions & 2 deletions src/Bicep.Cli/Rpc/CliJsonRpcServer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ public async Task<GetDeploymentGraphResponse> GetDeploymentGraph(GetDeploymentGr
{
var compilation = await GetCompilation(compiler, request.Path);
var model = compilation.GetEntrypointSemanticModel();
var dependenciesBySymbol = ResourceDependencyVisitor.GetResourceDependencies(model, new() { IncludeExisting = true })
var dependenciesBySymbol = ResourceDependencyVisitor.GetResourceDependencies(model)
.Where(x => !x.Key.Type.IsError())
.ToImmutableDictionary(x => x.Key, x => x.Value);

Expand All @@ -200,7 +200,7 @@ public async Task<GetDeploymentGraphResponse> GetDeploymentGraph(GetDeploymentGr
foreach (var (symbol, dependencies) in dependenciesBySymbol)
{
var source = nodesBySymbol.TryGetValue(symbol);
foreach (var dependency in dependencies.Where(d => d.Kind == ResourceDependencyKind.Primary))
foreach (var dependency in dependencies)
{
var target = nodesBySymbol.TryGetValue(dependency.Resource);
if (source is { } && target is { })
Expand Down
Loading

0 comments on commit 0602a35

Please sign in to comment.