Skip to content

Commit 7d53624

Browse files
BernieWhiteBernieWhite
authored
Fixes ordering of symbolic copy loop dependencies #3257 (#3259)
1 parent 32e5884 commit 7d53624

File tree

3 files changed

+139
-102
lines changed

3 files changed

+139
-102
lines changed

‎docs/CHANGELOG-v1.md

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ See [upgrade notes][1] for helpful information when upgrading from previous vers
2929

3030
## Unreleased
3131

32+
What's changed since v1.41.2:
33+
34+
- Bug fixes:
35+
- Fixed ordering of symbolic copy loop dependencies by @BernieWhite.
36+
[#3257](https://github.com/Azure/PSRule.Rules.Azure/issues/3257)
37+
3238
## v1.41.2
3339

3440
What's changed since v1.41.1:

‎src/PSRule.Rules.Azure/Data/Template/ResourceDependencyGraph.cs

Lines changed: 80 additions & 85 deletions
Original file line numberDiff line numberDiff line change
@@ -5,116 +5,111 @@
55
using System.Collections.Generic;
66
using System.Diagnostics;
77

8-
namespace PSRule.Rules.Azure.Data.Template
8+
namespace PSRule.Rules.Azure.Data.Template;
9+
10+
/// <summary>
11+
/// A graph that tracks resource dependencies in scope of a deployment.
12+
/// </summary>
13+
internal sealed class ResourceDependencyGraph
914
{
15+
private readonly Dictionary<string, Node> _ById = new(StringComparer.OrdinalIgnoreCase);
16+
private readonly Dictionary<string, Node> _ByName = new(StringComparer.OrdinalIgnoreCase);
17+
private readonly Dictionary<string, Node> _BySymbolicName = new(StringComparer.OrdinalIgnoreCase);
18+
private readonly Dictionary<string, List<Node>> _ByCopyName = new(StringComparer.OrdinalIgnoreCase);
19+
20+
[DebuggerDisplay("{Resource.Id}")]
21+
private sealed class Node(IResourceValue resource, string[] dependencies)
22+
{
23+
internal readonly IResourceValue Resource = resource;
24+
internal readonly string[] Dependencies = dependencies;
25+
}
26+
1027
/// <summary>
11-
/// A graph that tracks resource dependencies in scope of a deployment.
28+
/// Sort the provided resources based on dependency graph.
1229
/// </summary>
13-
internal sealed class ResourceDependencyGraph
30+
/// <param name="resources">The resources to sort.</param>
31+
/// <returns>An ordered set of resources.</returns>
32+
internal IResourceValue[] Sort(IResourceValue[] resources)
1433
{
15-
private readonly Dictionary<string, Node> _ById = new(StringComparer.OrdinalIgnoreCase);
16-
private readonly Dictionary<string, Node> _ByName = new(StringComparer.OrdinalIgnoreCase);
17-
private readonly Dictionary<string, Node> _BySymbolicName = new(StringComparer.OrdinalIgnoreCase);
18-
private readonly Dictionary<string, List<Node>> _ByCopyName = new(StringComparer.OrdinalIgnoreCase);
34+
if (resources == null || resources.Length <= 1)
35+
return resources;
1936

20-
[DebuggerDisplay("{Resource.Id}")]
21-
private sealed class Node
22-
{
23-
internal readonly IResourceValue Resource;
24-
internal readonly string[] Dependencies;
37+
var stack = new List<IResourceValue>(resources.Length);
38+
var visited = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
2539

26-
public Node(IResourceValue resource, string[] dependencies)
40+
foreach (var resource in resources)
41+
{
42+
if (TryGet(resource.Id, out var item))
2743
{
28-
Resource = resource;
29-
Dependencies = dependencies;
44+
Visit(item, visited, stack);
3045
}
31-
}
32-
33-
/// <summary>
34-
/// Sort the provided resources based on dependency graph.
35-
/// </summary>
36-
/// <param name="resources">The resources to sort.</param>
37-
/// <returns>An ordered set of resources.</returns>
38-
internal IResourceValue[] Sort(IResourceValue[] resources)
39-
{
40-
if (resources == null || resources.Length <= 1)
41-
return resources;
42-
43-
var stack = new List<IResourceValue>(resources.Length);
44-
var visited = new HashSet<string>(StringComparer.OrdinalIgnoreCase);
45-
46-
foreach (var resource in resources)
46+
else
4747
{
48-
if (TryGet(resource.Id, out var item))
49-
{
50-
Visit(item, visited, stack);
51-
}
52-
else
53-
{
54-
stack.Add(resource);
55-
visited.Add(resource.Id);
56-
}
48+
stack.Add(resource);
49+
visited.Add(resource.Id);
5750
}
58-
return stack.ToArray();
5951
}
52+
return [.. stack];
53+
}
6054

61-
/// <summary>
62-
/// Add a resource to the graph.
63-
/// </summary>
64-
/// <param name="resource">The resource node to add to the graph.</param>
65-
/// <param name="dependencies">Any dependencies for the node.</param>
66-
internal void Track(IResourceValue resource, string[] dependencies)
67-
{
68-
if (resource == null)
69-
return;
55+
/// <summary>
56+
/// Add a resource to the graph.
57+
/// </summary>
58+
/// <param name="resource">The resource node to add to the graph.</param>
59+
/// <param name="dependencies">Any dependencies for the node.</param>
60+
internal void Track(IResourceValue resource, string[] dependencies)
61+
{
62+
if (resource == null)
63+
return;
7064

71-
var item = new Node(resource, dependencies);
72-
_ById[resource.Id] = item;
73-
_ByName[resource.Name] = item;
65+
var item = new Node(resource, dependencies);
66+
_ById[resource.Id] = item;
67+
_ByName[resource.Name] = item;
7468

75-
if (!string.IsNullOrEmpty(resource.SymbolicName))
76-
_BySymbolicName[resource.SymbolicName] = item;
69+
if (!string.IsNullOrEmpty(resource.SymbolicName))
70+
_BySymbolicName[resource.SymbolicName] = item;
7771

78-
if (resource.Copy != null && !string.IsNullOrEmpty(resource.Copy.Name))
72+
if (resource.Copy != null && !string.IsNullOrEmpty(resource.Copy.Name))
73+
{
74+
if (!_ByCopyName.TryGetValue(resource.Copy.Name, out var copyItems))
7975
{
80-
if (!_ByCopyName.TryGetValue(resource.Copy.Name, out var copyItems))
81-
{
82-
copyItems = new List<Node>();
83-
_ByCopyName[resource.Copy.Name] = copyItems;
84-
}
85-
copyItems.Add(item);
76+
copyItems = [];
77+
_ByCopyName[resource.Copy.Name] = copyItems;
8678
}
79+
copyItems.Add(item);
8780
}
81+
}
8882

89-
private bool TryGet(string key, out Node item)
90-
{
91-
return _ById.TryGetValue(key, out item) ||
92-
_ByName.TryGetValue(key, out item) ||
93-
_BySymbolicName.TryGetValue(key, out item);
94-
}
83+
private bool TryGet(string key, out Node item)
84+
{
85+
return _ById.TryGetValue(key, out item) ||
86+
_ByName.TryGetValue(key, out item) ||
87+
_BySymbolicName.TryGetValue(key, out item);
88+
}
9589

96-
/// <summary>
97-
/// Traverse a node and dependencies.
98-
/// </summary>
99-
private void Visit(Node source, HashSet<string> visited, List<IResourceValue> stack)
100-
{
101-
if (visited.Contains(source.Resource.Id))
102-
return;
90+
/// <summary>
91+
/// Traverse a node and dependencies.
92+
/// </summary>
93+
private void Visit(Node source, HashSet<string> visited, List<IResourceValue> stack)
94+
{
95+
if (visited.Contains(source.Resource.Id))
96+
return;
10397

104-
visited.Add(source.Resource.Id);
105-
for (var i = 0; source.Dependencies != null && i < source.Dependencies.Length; i++)
98+
visited.Add(source.Resource.Id);
99+
for (var i = 0; source.Dependencies != null && i < source.Dependencies.Length; i++)
100+
{
101+
if (_ByCopyName.TryGetValue(source.Dependencies[i], out var countItems))
106102
{
107-
if (TryGet(source.Dependencies[i], out var item))
103+
foreach (var countItem in countItems)
108104
{
109-
Visit(item, visited, stack);
110-
}
111-
else if (_ByCopyName.TryGetValue(source.Dependencies[i], out var countItems))
112-
{
113-
foreach (var countItem in countItems)
114-
Visit(countItem, visited, stack);
105+
Visit(countItem, visited, stack);
115106
}
116107
}
117-
stack.Add(source.Resource);
108+
else if (TryGet(source.Dependencies[i], out var item))
109+
{
110+
Visit(item, visited, stack);
111+
}
118112
}
113+
stack.Add(source.Resource);
119114
}
120115
}

‎tests/PSRule.Rules.Azure.Tests/DependencyMapTests.cs

Lines changed: 53 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
// Copyright (c) Microsoft Corporation.
22
// Licensed under the MIT License.
33

4+
using System.Linq;
45
using Newtonsoft.Json.Linq;
56
using PSRule.Rules.Azure.Data.Template;
67
using static PSRule.Rules.Azure.Data.Template.TemplateVisitor;
@@ -10,7 +11,7 @@ namespace PSRule.Rules.Azure
1011
public sealed class DependencyMapTests
1112
{
1213
[Fact]
13-
public void SortWithComparer()
14+
public void SortDependencies_WithoutSymbolicName_ShouldReorderResources()
1415
{
1516
var context = new TemplateContext();
1617
var resources = new IResourceValue[]
@@ -36,13 +37,13 @@ public void SortWithComparer()
3637

3738
// https://github.com/Azure/PSRule.Rules.Azure/issues/2255
3839
context = new TemplateContext();
39-
resources = new IResourceValue[]
40-
{
40+
resources =
41+
[
4142
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/virtualNetworks\", \"name\": \"vnet-001\", \"dependsOn\": [ \"/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/ps-rule-test-rg/providers/Microsoft.Network/routeTables/rt-002\" ] }")),
4243
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-001\", \"dependsOn\": [ ] }")),
4344
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-002\" }")),
4445
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/virtualNetworks/subnets\", \"name\": \"vnet-001/subnet-001\", \"dependsOn\": [ \"/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/ps-rule-test-rg/providers/Microsoft.Network/virtualNetworks/vnet-001\" ] }")),
45-
};
46+
];
4647
resources = context.SortDependencies(resources);
4748

4849
actual = resources[0];
@@ -59,7 +60,7 @@ public void SortWithComparer()
5960
}
6061

6162
[Fact]
62-
public void SortSymbolicNameWithComparer()
63+
public void SortDependencies_WithSymbolicName_ShouldReorderResources()
6364
{
6465
var context = new TemplateContext();
6566
var resources = new IResourceValue[]
@@ -69,30 +70,65 @@ public void SortSymbolicNameWithComparer()
6970
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-002\" }"), symbolicName: "rt-002"),
7071
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/virtualNetworks/subnets\", \"name\": \"vnet-001/subnet-001\", \"dependsOn\": [ \"vnet-001\" ] }"), symbolicName: "vnet-001/subnet-001"),
7172
};
72-
resources = context.SortDependencies(resources);
7373

74-
var actual = resources[0];
75-
Assert.Equal("rt-002", actual.Value["name"].Value<string>());
76-
77-
actual = resources[1];
78-
Assert.Equal("vnet-001", actual.Value["name"].Value<string>());
74+
var actual = context.SortDependencies(resources).Select(r =>
75+
{
76+
return r.Value["name"].Value<string>();
77+
}).ToArray();
78+
79+
Assert.Equal(
80+
[
81+
"rt-002",
82+
"vnet-001",
83+
"rt-001",
84+
"vnet-001/subnet-001"
85+
], actual);
86+
}
7987

80-
actual = resources[2];
81-
Assert.Equal("rt-001", actual.Value["name"].Value<string>());
88+
/// <summary>
89+
/// If a dependency is a copy with a symbolic name all instances of the dependency should be reordered above the dependant resource.
90+
/// </summary>
91+
[Fact]
92+
public void SortDependencies_WithMultipleResourceCopies_ShouldReorderAllResources()
93+
{
94+
var context = new TemplateContext();
95+
var resources = new IResourceValue[]
96+
{
97+
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/virtualNetworks\", \"name\": \"vnet-001\", \"dependsOn\": [ \"routeTables\" ] }"), symbolicName: "vnet-001"),
98+
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-001\", \"dependsOn\": [ ] }"), symbolicName: "routeTables", copyInstance: 0),
99+
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-002\" }"), symbolicName: "routeTables", copyInstance: 1),
100+
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/virtualNetworks/subnets\", \"name\": \"vnet-001/subnet-001\", \"dependsOn\": [ \"vnet-001\" ] }"), symbolicName: "vnet-001/subnet-001"),
101+
};
82102

83-
actual = resources[3];
84-
Assert.Equal("vnet-001/subnet-001", actual.Value["name"].Value<string>());
103+
var actual = context.SortDependencies(resources).Select(r =>
104+
{
105+
return r.Value["name"].Value<string>();
106+
}).ToArray();
107+
108+
Assert.Equal(
109+
[
110+
"rt-001",
111+
"rt-002",
112+
"vnet-001",
113+
"vnet-001/subnet-001"
114+
], actual);
85115
}
86116

87117
#region Helper methods
88118

89-
private static IResourceValue GetResourceValue(TemplateContext context, JObject resource, string symbolicName = null)
119+
private static IResourceValue GetResourceValue(TemplateContext context, JObject resource, string symbolicName = null, int? copyInstance = null)
90120
{
121+
var copy = copyInstance == null || symbolicName == null ? null : new TemplateContext.CopyIndexState
122+
{
123+
Name = symbolicName,
124+
Index = copyInstance.Value
125+
};
126+
91127
resource.TryGetProperty("name", out var name);
92128
resource.TryGetProperty("type", out var type);
93129
resource.TryGetDependencies(out var dependencies);
94130
var resourceId = ResourceHelper.CombineResourceId(context.Subscription.SubscriptionId, context.ResourceGroup.Name, type, name);
95-
var result = new ResourceValue(resourceId, name, type, symbolicName, resource, null);
131+
var result = new ResourceValue(resourceId, name, type, symbolicName, resource, copy);
96132
context.TrackDependencies(result, dependencies);
97133
return result;
98134
}

0 commit comments

Comments
 (0)