Move all indexing expressions when calculating `dependsOn` expressions #15580

jeskew · 2024-11-14T22:52:46Z

Resolves #15513

Microsoft Reviewers: Open in CodeFlow

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:

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:

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:

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

… the deployments engine will use the GET response" (#15524) This reverts commit 1435468.

…distance

… ResourceDependencyVisitor

github-actions · 2024-11-14T22:55:59Z

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

VSCode

Mac/Linux

bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 12058796864

Windows

iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 12058796864"

Azure CLI

Mac/Linux

bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 12058796864

Windows

iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 12058796864"

github-actions · 2024-11-14T23:00:11Z

Dotnet Test Results

78 files - 39 78 suites - 39 30m 19s ⏱️ - 15m 14s
11 506 tests + 3 11 506 ✅ + 4 0 💤 ±0 0 ❌ - 1
26 743 runs - 13 283 26 743 ✅ - 13 282 0 💤 ±0 0 ❌ - 1

Results for commit 851f527. ± Comparison against base commit 95ede42.

This pull request removes 1840 and adds 658 tests. Note that renamed tests count towards both.


		nestedProp1: 1
		nestedProp2: 2
		prop1: true
		prop2: false
	1
	2
	\$'")
	prop1: true
	prop2: false
…

Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Bicep_compiler_handles_corrupted_extension_package_gracefully (\u001f�\u0008\u0000\u0000\u0000\u0000\u0000\u0000
�Խ
�0\u0010\u0000��>E�\u0003���jT��"��\u0000�=Q�U�\u0008\u0005��M\u0007qiq�`�1w$�\u000b\u0017\u0011nM�&�QY	
\u0000�`}s��\u001c�u�!Q3�߀��1f�׽W��^YS�R�8�\u0007�\u00057��S"u�4��P�Z*T3\u0017K�br�o�\u001e�S�Q-�յh������u���p-�6�\u0010!s���ZF\u0012\u0014�y����(3���������F��lC���`�\u000f�R�s��<���\u0000�K�\u0012\u0000\u000c\u0000\u0000,"Value cannot be null. (Parameter 'source')")
Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Bicep_compiler_handles_corrupted_extension_package_gracefully (\u001f�\u0008\u0000\u0000\u0000\u0000\u0000\u0000
��K
�0\u0010\u0006�=EO��$i�]t��+\u0004-����\u0005��\u000b�E���
�[f�d���xg�mn\u000fyUK\u0010Q,�����\u001a3���\u0005�\u0004!1$D�M>ɀ�nl�G��\u001f�֑mNE�1��1H!5mHk�|m?Z��j�cֺ�+oN����
�X��\u0017
\u0006��߷��S���`\u0003�`M��?k��ϒ�?�����\u0004A\u0010\u0004�\u0012�ق��\u0000\u000c\u0000\u0000,"The path: index.json was not found in artifact contents")
Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Bicep_compiler_handles_corrupted_extension_package_gracefully (\u001f�\u0008\u0000\u0000\u0000\u0000\u0000\u0000
���\u000b�0\u0014\u0007��+�?`��7g���CFX\u0010t��B\u0006Z���?�<D\u0017��?��9n��m�;��U�NT�\u0014%�\u0000�P�7�*x���
��0�\u0006\u0004�@A�U��I�GY�B�2�^?�����,	�t�D��O9� |��s�\u001c�[wU]�4���^�[ޔg]�S\u001f���y���[��#��\u0000g��L'\u0011<�z:��d�������܆��氋셥��{2\u000c�0��\u0002�`�\u0016\u0000\u000c\u0000\u0000,"'7' is an invalid end of a number. Expected a delimiter. Path: $.INVALID_JSON | LineNumber: 0 | BytePositionInLine: 20.")
Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Bicep_compiler_handles_corrupted_extension_package_gracefully (\u001f�\u0008\u0000\u0000\u0000\u0000\u0000\u0000\u0003�Խ
�0\u0010\u0007��>E�\u0003�$�&���Ep�\u0001b{b���F(��n;�K�K?\u0004�\u001bs\u0007w!���{�lѤX�L��׌���\u0014@�yG�&B�q�u�|Bh3�&=�5U��\u001c�~�\u000c��Y��о�\u0000J*�\u0001"\u0019����\u000c�DDKc�qV�ذK}+��|�}�k:\u0003>�7݌o��>��\u000f�B\u000b��$\\u0005A��Y2����P�P`Sbb1�a~���Г���\z7�q\u001cg:/���'\u0000\u000c\u0000\u0000,"Value cannot be null. (Parameter 'source')")
Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Bicep_compiler_handles_corrupted_extension_package_gracefully (\u001f�\u0008\u0000\u0000\u0000\u0000\u0000\u0000\u0003�Խ
�0\u0010\u0007��>E�\u0003�ə�T� 8X�*\u0008�\u0012l�
��V(�\uda58\udcf8���C0�1wp\u0017�?���j\u001d�(�\u000b
3�\u001e%]c�\u0012���M��\u001b#\u000c�b�8U�4x\u0014���*C��A��.�4�9N\u0001�P�(\u0003�q91�sK�ϝ�./~�EqE��-��Ӗ��h���`�3�埁ɿ@��A) LI	&��d����t���\u0006���\u000b݅c~�w�,˲��\u0002���$\u0000\u000c\u0000\u0000,"'7' is an invalid end of a number. Expected a delimiter. Path: $.INVALID_JSON | LineNumber: 0 | BytePositionInLine: 20.")
Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Bicep_compiler_handles_corrupted_extension_package_gracefully (\u001f�\u0008\u0000\u0000\u0000\u0000\u0000\u0000\u0003���
�0\u000c\u0007��\u0014>AMb�z��^�l�>�\u000e�L\u0018{���`\u0007e\u0017���wl\u0008I	���l�-��lZI�2�\u0014s���\u001a}\u001f�F��\u0005\u0004\u0018�Y���}�\u0011]�m\u0013VYb�\u000f�<��T�\u0005���RL,\u0001	4�P�O�0�Z�,:wq���s[����jX��Ѩ��}kƧ�\u0003�@e\u0008
\u00123��c6���L�y�7��(���\u0004Q��\u0000\u000c\u0000\u0000,"The path: index.json was not found in artifact contents")
Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Repository_not_found_in_registry (ArtifactRegistryAddress { RegistryAddress = mcr.microsoft.com, RepositoryPath = unknown/path/az, ExtensionVersion = 0.0.0-placeholder },Azure.RequestFailedException: The artifact does not exist in the registry.
   at Bicep.Core.Registry.AzureContainerRegistryManager.DownloadManifestAndLayersAsync(IOciArtifactReference artifactReference, ContainerRegistryContentClient client) in /home/runner/work/bicep/bicep/src/Bicep.Core/Registry/AzureContainerRegistryManager.cs:line 138
   at Bicep.Core.Registry.AzureContainerRegistryManager.DownloadManifestAndLayersAsync(IOciArtifactReference artifactReference, ContainerRegistryContentClient client) in /home/runner/work/bicep/bicep/src/Bicep.Core/Registry/AzureContainerRegistryManager.cs:line 138,[(BCP192, Error, Unable to restore the artifact with reference "br:mcr.microsoft.com/unknown/path/az:0.0.0-placeholder": The artifact does not exist in the registry.)])
Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Repository_not_found_in_registry (ArtifactRegistryAddress { RegistryAddress = mcr.microsoft.com, RepositoryPath = unknown/path/az, ExtensionVersion = 0.0.0-placeholder },Azure.RequestFailedException: The artifact does not exist in the registry.
   at Bicep.Core.Registry.AzureContainerRegistryManager.DownloadManifestAndLayersAsync(IOciArtifactReference artifactReference, ContainerRegistryContentClient client) in D:\a\bicep\bicep\src\Bicep.Core\Registry\AzureContainerRegistryManager.cs:line 138
   at Bicep.Core.Registry.AzureContainerRegistryManager.DownloadManifestAndLayersAsync(IOciArtifactReference artifactReference, ContainerRegistryContentClient client) in D:\a\bicep\bicep\src\Bicep.Core\Registry\AzureContainerRegistryManager.cs:line 138,[(BCP192, Error, Unable to restore the artifact with reference "br:mcr.microsoft.com/unknown/path/az:0.0.0-placeholder": The artifact does not exist in the registry.)])
Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Repository_not_found_in_registry (ArtifactRegistryAddress { RegistryAddress = unknown.registry.azurecr.io, RepositoryPath = bicep/extensions/az, ExtensionVersion = 0.0.0-placeholder },System.AggregateException: Retry failed after 4 tries. Retry settings can be adjusted in ClientOptions.Retry or by configuring a custom retry policy in ClientOptions.RetryPolicy. (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443))
   at Bicep.Core.Registry.AzureContainerRegistryManager.DownloadManifestAndLayersAsync(IOciArtifactReference artifactReference, ContainerRegistryContentClient client) in /home/runner/work/bicep/bicep/src/Bicep.Core/Registry/AzureContainerRegistryManager.cs:line 138
   at Bicep.Core.Registry.AzureContainerRegistryManager.<>c__DisplayClass4_0.<<PullArtifactAsync>g__DownloadManifestInternalAsync|0>d.MoveNext() in /home/runner/work/bicep/bicep/src/Bicep.Core/Registry/AzureContainerRegistryManager.cs:line 44
--- End of stack trace from previous location ---
   at Bicep.Core.Registry.AzureContainerRegistryManager.PullArtifactAsync(RootConfiguration configuration, IOciArtifactReference artifactReference) in /home/runner/work/bicep/bicep/src/Bicep.Core/Registry/AzureContainerRegistryManager.cs:line 51
   at Bicep.Core.Registry.AzureContainerRegistryManager.DownloadManifestAndLayersAsync(IOciArtifactReference artifactReference, ContainerRegistryContentClient client) in /home/runner/work/bicep/bicep/src/Bicep.Core/Registry/AzureContainerRegistryManager.cs:line 138
   at Bicep.Core.Registry.AzureContainerRegistryManager.<>c__DisplayClass4_0.<<PullArtifactAsync>g__DownloadManifestInternalAsync|0>d.MoveNext() in /home/runner/work/bicep/bicep/src/Bicep.Core/Registry/AzureContainerRegistryManager.cs:line 44
--- End of stack trace from previous location ---
   at Bicep.Core.Registry.AzureContainerRegistryManager.PullArtifactAsync(RootConfiguration configuration, IOciArtifactReference artifactReference) in /home/runner/work/bicep/bicep/src/Bicep.Core/Registry/AzureContainerRegistryManager.cs:line 63
   at Bicep.Core.Registry.OciArtifactRegistry.TryRestoreArtifactAsync(RootConfiguration configuration, OciArtifactReference reference) in /home/runner/work/bicep/bicep/src/Bicep.Core/Registry/OciArtifactRegistry.cs:line 499,[(BCP192, Error, Unable to restore the artifact with reference "br:unknown.registry.azurecr.io/bicep/extensions/az:0.0.0-placeholder": Retry failed after 4 tries. Retry settings can be adjusted in ClientOptions.Retry or by configuring a custom retry policy in ClientOptions.RetryPolicy. (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)))])
Bicep.Core.IntegrationTests.AzTypesViaRegistryTests ‑ Repository_not_found_in_registry (ArtifactRegistryAddress { RegistryAddress = unknown.registry.azurecr.io, RepositoryPath = bicep/extensions/az, ExtensionVersion = 0.0.0-placeholder },System.AggregateException: Retry failed after 4 tries. Retry settings can be adjusted in ClientOptions.Retry or by configuring a custom retry policy in ClientOptions.RetryPolicy. (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443))
   at Bicep.Core.Registry.AzureContainerRegistryManager.DownloadManifestAndLayersAsync(IOciArtifactReference artifactReference, ContainerRegistryContentClient client) in D:\a\bicep\bicep\src\Bicep.Core\Registry\AzureContainerRegistryManager.cs:line 138
   at Bicep.Core.Registry.AzureContainerRegistryManager.<>c__DisplayClass4_0.<<PullArtifactAsync>g__DownloadManifestInternalAsync|0>d.MoveNext() in D:\a\bicep\bicep\src\Bicep.Core\Registry\AzureContainerRegistryManager.cs:line 44
--- End of stack trace from previous location ---
   at Bicep.Core.Registry.AzureContainerRegistryManager.PullArtifactAsync(RootConfiguration configuration, IOciArtifactReference artifactReference) in D:\a\bicep\bicep\src\Bicep.Core\Registry\AzureContainerRegistryManager.cs:line 51
   at Bicep.Core.Registry.AzureContainerRegistryManager.DownloadManifestAndLayersAsync(IOciArtifactReference artifactReference, ContainerRegistryContentClient client) in D:\a\bicep\bicep\src\Bicep.Core\Registry\AzureContainerRegistryManager.cs:line 138
   at Bicep.Core.Registry.AzureContainerRegistryManager.<>c__DisplayClass4_0.<<PullArtifactAsync>g__DownloadManifestInternalAsync|0>d.MoveNext() in D:\a\bicep\bicep\src\Bicep.Core\Registry\AzureContainerRegistryManager.cs:line 44
--- End of stack trace from previous location ---
   at Bicep.Core.Registry.AzureContainerRegistryManager.PullArtifactAsync(RootConfiguration configuration, IOciArtifactReference artifactReference) in D:\a\bicep\bicep\src\Bicep.Core\Registry\AzureContainerRegistryManager.cs:line 63
   at Bicep.Core.Registry.OciArtifactRegistry.TryRestoreArtifactAsync(RootConfiguration configuration, OciArtifactReference reference) in D:\a\bicep\bicep\src\Bicep.Core\Registry\OciArtifactRegistry.cs:line 499,[(BCP192, Error, Unable to restore the artifact with reference "br:unknown.registry.azurecr.io/bicep/extensions/az:0.0.0-placeholder": Retry failed after 4 tries. Retry settings can be adjusted in ClientOptions.Retry or by configuring a custom retry policy in ClientOptions.RetryPolicy. (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)) (No such host is known. (unknown.registry.azurecr.io:443)))])
…

♻️ This comment has been updated with latest results.

# Conflicts: # src/Bicep.Core.IntegrationTests/ScenarioTests.cs

…t does `parent`

anthony-c-martin · 2024-11-26T14:59:31Z

@jeskew - are there any opportunities to incorporate the real Deployment engine's expansion/reference evaluation capabilities into our testing framework? Given there are optimizations in both Bicep and backend which need to work correctly together, it's pretty hard to look at the codegened output and say "this looks correct". It would be really useful to be able to write tests or baselines that assert the exact deployment graph which the engine would follow, and I think probably help us avoid any regressions in this logic in the longer run.

jeskew · 2024-11-26T15:11:21Z

@anthony-c-martin I'll see if I can add something similar to what we have in the backend's baseline tests. We have the one artifact type that records the deployment plan (job count, job sequence, dependency graph), which would be useful here. The one thing that would make it complex is that the engine can't determine the plan without knowing all deployment parameters, and I don't think that's spelled out for our baselines today.

This is turning out to be much higher labor than I thought it would be. The parameters need to be written by hand, and the baselines that I have tried to convert to a deployment plan have failed validation because they're meant to exercise language features rather than be deployable.

I'm going to scope this PR down to just fixing the bug in #15513 and move the fix for #13674 and #15686 to a follow-up PR. The former shouldn't affect existing baselines, and the latter is a pretty small change.

jeskew added 5 commits November 14, 2024 17:14

Reapply "Only add an explicit dependency on an existing resource when…

040c5c5

… the deployments engine will use the GET response" (#15524) This reverts commit 1435468.

Update dependency expression generation to account for indexing at a …

b56dd0c

…distance

Update baselines

1fefdb0

Update NoUnnecessaryDependsOnRule to account for new output format of…

a61a5fe

… ResourceDependencyVisitor

Add repro test

a8e19d0

jeskew requested review from majastrz and a team November 14, 2024 23:33

jeskew marked this pull request as ready for review November 14, 2024 23:33

jeskew added 3 commits November 15, 2024 12:59

Consolidate added dependency inference tests

9445fbb

Merge branch 'main' into jeskew/15513-bis

2facf4f

# Conflicts: # src/Bicep.Core.IntegrationTests/ScenarioTests.cs

Mark an accidentally public method as private static

56d041c

jeskew force-pushed the jeskew/15513-bis branch from 74bd307 to 56d041c Compare November 15, 2024 21:28

jeskew mentioned this pull request Nov 18, 2024

Empty string in resource name fails, even when the resourse will not be used. #15632

Open

jeskew added 3 commits November 25, 2024 10:54

Merge branch 'main' into jeskew/15513-bis

f2a10b3

Update dependency inference to special-case scope in the same way i…

a708a01

…t does `parent`

Regenerate baselines

7f65b4d

Back out change to symbolic name dependency inference

851f527

jeskew force-pushed the jeskew/15513-bis branch from 7aceac6 to 851f527 Compare November 27, 2024 21:54

jeskew changed the title ~~Only add an explicit dependency on an existing resource when the deployments engine will use the GET response~~ Move all indexing expressions when calculating dependsOn expressions Nov 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all indexing expressions when calculating `dependsOn` expressions #15580

Move all indexing expressions when calculating `dependsOn` expressions #15580

jeskew commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

anthony-c-martin commented Nov 26, 2024 •

edited

Loading

jeskew commented Nov 26, 2024 •

edited

Loading

Move all indexing expressions when calculating dependsOn expressions #15580

Are you sure you want to change the base?

Move all indexing expressions when calculating dependsOn expressions #15580

Conversation

jeskew commented Nov 14, 2024 • edited Loading

Microsoft Reviewers: Open in CodeFlow

github-actions bot commented Nov 14, 2024 • edited Loading

github-actions bot commented Nov 14, 2024 • edited Loading

Dotnet Test Results

anthony-c-martin commented Nov 26, 2024 • edited Loading

jeskew commented Nov 26, 2024 • edited Loading

Move all indexing expressions when calculating `dependsOn` expressions #15580

Move all indexing expressions when calculating `dependsOn` expressions #15580

jeskew commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

github-actions bot commented Nov 14, 2024 •

edited

Loading

anthony-c-martin commented Nov 26, 2024 •

edited

Loading

jeskew commented Nov 26, 2024 •

edited

Loading