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.

Sign up for GitHub

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

Fixes ordering of symbolic copy loop dependencies #3257 #3259

Merged
merged 1 commit into from
Feb 18, 2025
Merged

Fixes ordering of symbolic copy loop dependencies #3257 #3259

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/CHANGELOG-v1.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,12 @@ See [upgrade notes][1] for helpful information when upgrading from previous vers

## Unreleased

What's changed since v1.41.2:

- Bug fixes:
- Fixed ordering of symbolic copy loop dependencies by @BernieWhite.
[#3257](https://github.com/Azure/PSRule.Rules.Azure/issues/3257)

## v1.41.2

What's changed since v1.41.1:
Expand Down
165 changes: 80 additions & 85 deletions src/PSRule.Rules.Azure/Data/Template/ResourceDependencyGraph.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,116 +5,111 @@
using System.Collections.Generic;
using System.Diagnostics;

namespace PSRule.Rules.Azure.Data.Template
namespace PSRule.Rules.Azure.Data.Template;

/// <summary>
/// A graph that tracks resource dependencies in scope of a deployment.
/// </summary>
internal sealed class ResourceDependencyGraph
{
private readonly Dictionary<string, Node> _ById = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, Node> _ByName = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, Node> _BySymbolicName = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, List<Node>> _ByCopyName = new(StringComparer.OrdinalIgnoreCase);

[DebuggerDisplay("{Resource.Id}")]
private sealed class Node(IResourceValue resource, string[] dependencies)
{
internal readonly IResourceValue Resource = resource;
internal readonly string[] Dependencies = dependencies;
}

/// <summary>
/// A graph that tracks resource dependencies in scope of a deployment.
/// Sort the provided resources based on dependency graph.
/// </summary>
internal sealed class ResourceDependencyGraph
/// <param name="resources">The resources to sort.</param>
/// <returns>An ordered set of resources.</returns>
internal IResourceValue[] Sort(IResourceValue[] resources)
{
private readonly Dictionary<string, Node> _ById = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, Node> _ByName = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, Node> _BySymbolicName = new(StringComparer.OrdinalIgnoreCase);
private readonly Dictionary<string, List<Node>> _ByCopyName = new(StringComparer.OrdinalIgnoreCase);
if (resources == null || resources.Length <= 1)
return resources;

[DebuggerDisplay("{Resource.Id}")]
private sealed class Node
{
internal readonly IResourceValue Resource;
internal readonly string[] Dependencies;
var stack = new List<IResourceValue>(resources.Length);
var visited = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

public Node(IResourceValue resource, string[] dependencies)
foreach (var resource in resources)
{
if (TryGet(resource.Id, out var item))
{
Resource = resource;
Dependencies = dependencies;
Visit(item, visited, stack);
}
}

/// <summary>
/// Sort the provided resources based on dependency graph.
/// </summary>
/// <param name="resources">The resources to sort.</param>
/// <returns>An ordered set of resources.</returns>
internal IResourceValue[] Sort(IResourceValue[] resources)
{
if (resources == null || resources.Length <= 1)
return resources;

var stack = new List<IResourceValue>(resources.Length);
var visited = new HashSet<string>(StringComparer.OrdinalIgnoreCase);

foreach (var resource in resources)
else
{
if (TryGet(resource.Id, out var item))
{
Visit(item, visited, stack);
}
else
{
stack.Add(resource);
visited.Add(resource.Id);
}
stack.Add(resource);
visited.Add(resource.Id);
}
return stack.ToArray();
}
return [.. stack];
}

/// <summary>
/// Add a resource to the graph.
/// </summary>
/// <param name="resource">The resource node to add to the graph.</param>
/// <param name="dependencies">Any dependencies for the node.</param>
internal void Track(IResourceValue resource, string[] dependencies)
{
if (resource == null)
return;
/// <summary>
/// Add a resource to the graph.
/// </summary>
/// <param name="resource">The resource node to add to the graph.</param>
/// <param name="dependencies">Any dependencies for the node.</param>
internal void Track(IResourceValue resource, string[] dependencies)
{
if (resource == null)
return;

var item = new Node(resource, dependencies);
_ById[resource.Id] = item;
_ByName[resource.Name] = item;
var item = new Node(resource, dependencies);
_ById[resource.Id] = item;
_ByName[resource.Name] = item;

if (!string.IsNullOrEmpty(resource.SymbolicName))
_BySymbolicName[resource.SymbolicName] = item;
if (!string.IsNullOrEmpty(resource.SymbolicName))
_BySymbolicName[resource.SymbolicName] = item;

if (resource.Copy != null && !string.IsNullOrEmpty(resource.Copy.Name))
if (resource.Copy != null && !string.IsNullOrEmpty(resource.Copy.Name))
{
if (!_ByCopyName.TryGetValue(resource.Copy.Name, out var copyItems))
{
if (!_ByCopyName.TryGetValue(resource.Copy.Name, out var copyItems))
{
copyItems = new List<Node>();
_ByCopyName[resource.Copy.Name] = copyItems;
}
copyItems.Add(item);
copyItems = [];
_ByCopyName[resource.Copy.Name] = copyItems;
}
copyItems.Add(item);
}
}

private bool TryGet(string key, out Node item)
{
return _ById.TryGetValue(key, out item) ||
_ByName.TryGetValue(key, out item) ||
_BySymbolicName.TryGetValue(key, out item);
}
private bool TryGet(string key, out Node item)
{
return _ById.TryGetValue(key, out item) ||
_ByName.TryGetValue(key, out item) ||
_BySymbolicName.TryGetValue(key, out item);
}

/// <summary>
/// Traverse a node and dependencies.
/// </summary>
private void Visit(Node source, HashSet<string> visited, List<IResourceValue> stack)
{
if (visited.Contains(source.Resource.Id))
return;
/// <summary>
/// Traverse a node and dependencies.
/// </summary>
private void Visit(Node source, HashSet<string> visited, List<IResourceValue> stack)
{
if (visited.Contains(source.Resource.Id))
return;

visited.Add(source.Resource.Id);
for (var i = 0; source.Dependencies != null && i < source.Dependencies.Length; i++)
visited.Add(source.Resource.Id);
for (var i = 0; source.Dependencies != null && i < source.Dependencies.Length; i++)
{
if (_ByCopyName.TryGetValue(source.Dependencies[i], out var countItems))
{
if (TryGet(source.Dependencies[i], out var item))
foreach (var countItem in countItems)
{
Visit(item, visited, stack);
}
else if (_ByCopyName.TryGetValue(source.Dependencies[i], out var countItems))
{
foreach (var countItem in countItems)
Visit(countItem, visited, stack);
Visit(countItem, visited, stack);
}
}
stack.Add(source.Resource);
else if (TryGet(source.Dependencies[i], out var item))
{
Visit(item, visited, stack);
}
}
stack.Add(source.Resource);
}
}
70 changes: 53 additions & 17 deletions tests/PSRule.Rules.Azure.Tests/DependencyMapTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
// Copyright (c) Microsoft Corporation.
// Licensed under the MIT License.

using System.Linq;
using Newtonsoft.Json.Linq;
using PSRule.Rules.Azure.Data.Template;
using static PSRule.Rules.Azure.Data.Template.TemplateVisitor;
Expand All @@ -10,7 +11,7 @@ namespace PSRule.Rules.Azure
public sealed class DependencyMapTests
{
[Fact]
public void SortWithComparer()
public void SortDependencies_WithoutSymbolicName_ShouldReorderResources()
{
var context = new TemplateContext();
var resources = new IResourceValue[]
Expand All @@ -36,13 +37,13 @@ public void SortWithComparer()

// https://github.com/Azure/PSRule.Rules.Azure/issues/2255
context = new TemplateContext();
resources = new IResourceValue[]
{
resources =
[
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\" ] }")),
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-001\", \"dependsOn\": [ ] }")),
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-002\" }")),
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\" ] }")),
};
];
resources = context.SortDependencies(resources);

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

[Fact]
public void SortSymbolicNameWithComparer()
public void SortDependencies_WithSymbolicName_ShouldReorderResources()
{
var context = new TemplateContext();
var resources = new IResourceValue[]
Expand All @@ -69,30 +70,65 @@ public void SortSymbolicNameWithComparer()
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-002\" }"), symbolicName: "rt-002"),
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/virtualNetworks/subnets\", \"name\": \"vnet-001/subnet-001\", \"dependsOn\": [ \"vnet-001\" ] }"), symbolicName: "vnet-001/subnet-001"),
};
resources = context.SortDependencies(resources);

var actual = resources[0];
Assert.Equal("rt-002", actual.Value["name"].Value<string>());

actual = resources[1];
Assert.Equal("vnet-001", actual.Value["name"].Value<string>());
var actual = context.SortDependencies(resources).Select(r =>
{
return r.Value["name"].Value<string>();
}).ToArray();

Assert.Equal(
[
"rt-002",
"vnet-001",
"rt-001",
"vnet-001/subnet-001"
], actual);
}

actual = resources[2];
Assert.Equal("rt-001", actual.Value["name"].Value<string>());
/// <summary>
/// If a dependency is a copy with a symbolic name all instances of the dependency should be reordered above the dependant resource.
/// </summary>
[Fact]
public void SortDependencies_WithMultipleResourceCopies_ShouldReorderAllResources()
{
var context = new TemplateContext();
var resources = new IResourceValue[]
{
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/virtualNetworks\", \"name\": \"vnet-001\", \"dependsOn\": [ \"routeTables\" ] }"), symbolicName: "vnet-001"),
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-001\", \"dependsOn\": [ ] }"), symbolicName: "routeTables", copyInstance: 0),
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/routeTables\", \"name\": \"rt-002\" }"), symbolicName: "routeTables", copyInstance: 1),
GetResourceValue(context, JObject.Parse("{ \"type\": \"Microsoft.Network/virtualNetworks/subnets\", \"name\": \"vnet-001/subnet-001\", \"dependsOn\": [ \"vnet-001\" ] }"), symbolicName: "vnet-001/subnet-001"),
};

actual = resources[3];
Assert.Equal("vnet-001/subnet-001", actual.Value["name"].Value<string>());
var actual = context.SortDependencies(resources).Select(r =>
{
return r.Value["name"].Value<string>();
}).ToArray();

Assert.Equal(
[
"rt-001",
"rt-002",
"vnet-001",
"vnet-001/subnet-001"
], actual);
}

#region Helper methods

private static IResourceValue GetResourceValue(TemplateContext context, JObject resource, string symbolicName = null)
private static IResourceValue GetResourceValue(TemplateContext context, JObject resource, string symbolicName = null, int? copyInstance = null)
{
var copy = copyInstance == null || symbolicName == null ? null : new TemplateContext.CopyIndexState
{
Name = symbolicName,
Index = copyInstance.Value
};

resource.TryGetProperty("name", out var name);
resource.TryGetProperty("type", out var type);
resource.TryGetDependencies(out var dependencies);
var resourceId = ResourceHelper.CombineResourceId(context.Subscription.SubscriptionId, context.ResourceGroup.Name, type, name);
var result = new ResourceValue(resourceId, name, type, symbolicName, resource, null);
var result = new ResourceValue(resourceId, name, type, symbolicName, resource, copy);
context.TrackDependencies(result, dependencies);
return result;
}
Expand Down
Loading