Fixes duplicate resource sub-resources #2564 (#2565)

docs/CHANGELOG-v1.md

@@ -41,6 +41,8 @@ What's changed since v1.31.3:
 - Bug fixes:
   - Fixed additional false positives of `Azure.Deployment.SecureParameter` by @BernieWhite.
     [#2556](https://github.com/Azure/PSRule.Rules.Azure/issues/2556)
+  - Fixed expansion with sub-resource handling of deployments with duplicate resources by @BernieWhite.
+    [#2564](https://github.com/Azure/PSRule.Rules.Azure/issues/2564)
 
 ## v1.31.3
 

src/PSRule.Rules.Azure/Common/JsonExtensions.cs

@@ -127,6 +127,14 @@ internal static bool TryGetResources(this JObject resource, string type, out JOb
             return results.Count > 0;
         }
 
+        internal static bool TryGetResourcesArray(this JObject resource, out JArray resources)
+        {
+            if (resource.TryGetProperty(PROPERTY_RESOURCES, out resources))
+                return true;
+
+            return false;
+        }
+
         internal static bool PropertyEquals(this JObject o, string propertyName, string value)
         {
             return o.TryGetProperty(propertyName, out var s) && string.Equals(s, value, StringComparison.OrdinalIgnoreCase);
@@ -175,6 +183,18 @@ internal static void AddRange(this JArray o, IEnumerable<JToken> items)
                 o.Add(item);
         }
 
+        /// <summary>
+        /// Add items to the start of the array instead of the end.
+        /// </summary>
+        /// <param name="o">The <seealso cref="JArray"/> to add items to.</param>
+        /// <param name="items">A set of items to add.</param>
+        internal static void AddRangeFromStart(this JArray o, IEnumerable<JToken> items)
+        {
+            var counter = 0;
+            foreach (var item in items)
+                o.Insert(counter++, item);
+        }
+
         internal static IEnumerable<JObject> GetPeerConditionByField(this JObject o, string field)
         {
             return o.BeforeSelf().OfType<JObject>().Where(peer => peer.TryGetProperty(PROPERTY_FIELD, out var peerField) &&

src/PSRule.Rules.Azure/Common/ResourceHelper.cs

@@ -10,6 +10,7 @@ namespace PSRule.Rules.Azure
     internal static class ResourceHelper
     {
         private const string SLASH = "/";
+        private const string DOT = ".";
         private const string SUBSCRIPTIONS = "subscriptions";
         private const string RESOURCEGROUPS = "resourceGroups";
         private const string PROVIDERS = "providers";
@@ -39,6 +40,25 @@ internal static bool IsResourceType(string resourceId, string resourceType)
             return i == idParts.Length;
         }
 
+        /// <summary>
+        /// Determine if the resource type is a sub-resource of the parent resource Id.
+        /// </summary>
+        /// <param name="parentId">The resource Id of the parent resource.</param>
+        /// <param name="resourceType">The resource type of sub-resource.</param>
+        /// <returns>Returns <c>true</c> if the resource type is a sub-resource. Otherwise <c>false</c> is returned.</returns>
+        internal static bool IsSubResourceType(string parentId, string resourceType)
+        {
+            if (string.IsNullOrEmpty(parentId) || string.IsNullOrEmpty(parentId))
+                return false;
+
+            // Is the resource type has no provider namespace dot it is a sub type.
+            if (!resourceType.Contains(DOT)) return true;
+
+            // Compare full type names.
+            var parentType = GetResourceType(parentId);
+            return resourceType.StartsWith(parentType, StringComparison.OrdinalIgnoreCase);
+        }
+
         /// <summary>
         /// Get the resource type of the resource.
         /// </summary>
@@ -111,10 +131,15 @@ internal static string CombineResourceId(string subscriptionId, string resourceG
                 throw new TemplateFunctionException(nameof(resourceType), FunctionErrorType.MismatchingResourceSegments, PSRuleResources.MismatchingResourceSegments);
 
             var parts = resourceTypeLength + nameLength;
-            parts += parts > 0 ? 1 : 0;
             parts += subscriptionId != null ? 2 : 0;
             parts += resourceGroup != null ? 2 : 0;
 
+            for (var p = 0; resourceType != null && p < resourceType.Length; p++)
+            {
+                if (resourceType[p].Contains(DOT))
+                    parts++;
+            }
+
             var result = new string[parts * 2];
             var i = 0;
             var j = 0;
@@ -135,10 +160,15 @@ internal static string CombineResourceId(string subscriptionId, string resourceG
             }
             if (resourceTypeLength > 0 && depth >= 0)
             {
-                result[i++] = SLASH;
-                result[i++] = PROVIDERS;
                 while (i < result.Length && j <= depth)
                 {
+                    // If a resource provider is included prepend /providers.
+                    if (resourceType[j].Contains(DOT))
+                    {
+                        result[i++] = SLASH;
+                        result[i++] = PROVIDERS;
+                    }
+
                     result[i++] = SLASH;
                     result[i++] = resourceType[j];
                     result[i++] = SLASH;
@@ -148,12 +178,30 @@ internal static string CombineResourceId(string subscriptionId, string resourceG
             return string.Concat(result);
         }
 
-        internal static string CombineResourceId(string subscriptionId, string resourceGroup, string resourceType, string name, int depth = int.MaxValue)
+        internal static string CombineResourceId(string subscriptionId, string resourceGroup, string resourceType, string name, int depth = int.MaxValue, string scope = null)
         {
             TryResourceIdComponents(resourceType, name, out var typeComponents, out var nameComponents);
+
+            // Handle scoped resource IDs.
+            if (!string.IsNullOrEmpty(scope) && scope != SLASH && TryResourceIdComponents(scope, out subscriptionId, out resourceGroup, out var parentTypeComponents, nameComponents: out var parentNameComponents))
+            {
+                typeComponents = MergeComponents(parentTypeComponents, typeComponents);
+                nameComponents = MergeComponents(parentNameComponents, nameComponents);
+            }
             return CombineResourceId(subscriptionId, resourceGroup, typeComponents, nameComponents, depth);
         }
 
+        /// <summary>
+        /// Merge type or name components from parent and child.
+        /// </summary>
+        private static string[] MergeComponents(string[] parent, string[] child)
+        {
+            var combined = new string[parent.Length + child.Length];
+            Array.Copy(parent, combined, parent.Length);
+            Array.Copy(child, 0, combined, parent.Length, child.Length);
+            return combined;
+        }
+
         /// <summary>
         /// Combines Id fragments to form a resource Id at a management group scope.
         /// </summary>

src/PSRule.Rules.Azure/Data/Template/RuleDataExportVisitor.cs

@@ -322,27 +322,34 @@ private static bool ProjectWebApp(IResourceValue resource)
         /// </summary>
         private static void MergeResource(TemplateContext context, IResourceValue resource, List<IResourceValue> unprocessed, HashSet<string> processed)
         {
-            if (!ShouldMerge(resource.Type))
-                return;
+            _ = MergeResourceLeft(context, resource, unprocessed, processed) ||
+                MergeResourceRight(context, resource, unprocessed, processed);
+        }
+
+        private static bool MergeResourceLeft(TemplateContext context, IResourceValue resource, List<IResourceValue> unprocessed, HashSet<string> processed)
+        {
+            if (!ShouldMergeLeft(resource.Type))
+                return false;
 
             var duplicates = unprocessed.FindAll(x => x.Id == resource.Id);
             for (var i = 1; duplicates.Count > 1 && i < duplicates.Count; i++)
             {
-                MergeResource(duplicates[0].Value, duplicates[i].Value, processed);
+                MergeResourceLeft(duplicates[0].Value, duplicates[i].Value, processed);
                 unprocessed.Remove(duplicates[i]);
                 context.RemoveResource(duplicates[i]);
             }
+            return true;
         }
 
         /// <summary>
         /// Merge specific resources and thier sub-resources.
         /// </summary>
-        private static void MergeResource(JObject resourceA, JObject resourceB, HashSet<string> processed)
+        private static void MergeResourceLeft(JObject left, JObject right, HashSet<string> processed)
         {
-            resourceA.Merge(resourceB, _MergeSettings);
+            left.Merge(right, _MergeSettings);
 
             // Handle child resources
-            if (resourceA.TryGetResources(out var resources))
+            if (left.TryGetResources(out var resources))
             {
                 for (var i = 0; resources != null && i < resources.Length; i++)
                 {
@@ -352,14 +359,65 @@ private static void MergeResource(JObject resourceA, JObject resourceB, HashSet<
                     var duplicates = Array.FindAll(resources, x => x[PROPERTY_ID].ToString() == childResourceId);
                     for (var j = 1; duplicates.Length > 1 && j < duplicates.Length; j++)
                     {
-                        MergeResource(duplicates[0].Value<JObject>(), duplicates[j].Value<JObject>(), processed);
+                        MergeResourceLeft(duplicates[0].Value<JObject>(), duplicates[j].Value<JObject>(), processed);
                         duplicates[j].Remove();
                     }
                     processed.Add(childResourceId);
                 }
             }
         }
 
+        /// <summary>
+        /// Merge resources based on duplicates which could occur across modules.
+        /// Last instance wins but sub-resources are accumulated.
+        /// </summary>
+        private static bool MergeResourceRight(TemplateContext context, IResourceValue resource, List<IResourceValue> unprocessed, HashSet<string> processed)
+        {
+            var duplicates = unprocessed.FindAll(x => x.Id == resource.Id);
+            for (var i = 1; duplicates.Count > 1 && i < duplicates.Count; i++)
+            {
+                MergeResourceRight(duplicates[i - 1].Value, duplicates[i].Value, processed);
+                unprocessed.Remove(duplicates[i - 1]);
+                context.RemoveResource(duplicates[i - 1]);
+            }
+
+            var right = duplicates[duplicates.Count - 1];
+            if (duplicates.Count > 1 && right.Value.TryGetResources(out var subResources))
+            {
+                for (var i = subResources.Length - 1; i >= 0; i--)
+                {
+                    if (!subResources[i].TryGetProperty(PROPERTY_ID, out var childResourceId))
+                        continue;
+
+                    if (processed.Contains(childResourceId))
+                    {
+                        subResources[i].Remove();
+                    }
+                    else
+                    {
+                        processed.Add(childResourceId);
+                    }
+                }
+            }
+            return true;
+        }
+
+        /// <summary>
+        /// Merge resources sub-resources only.
+        /// </summary>
+        private static void MergeResourceRight(JObject left, JObject right, HashSet<string> processed)
+        {
+            // Get sub-resources.
+            if (left.TryGetResources(out var leftResources))
+            {
+                if (!right.TryGetResourcesArray(out var rightResources))
+                    rightResources = new JArray();
+
+                rightResources.AddRangeFromStart(leftResources);
+                right.ReplaceProperty(PROPERTY_RESOURCES, rightResources);
+            }
+        }
+
         /// <summary>
         /// Move sub-resources based on parent resource relationship.
         /// This process nests sub-resources so that relationship can be analyzed.
@@ -398,7 +456,7 @@ private static bool ShouldMove(string resourceType)
         /// <summary>
         /// Determines if the specific resource type should be merged.
         /// </summary>
-        private static bool ShouldMerge(string resourceType)
+        private static bool ShouldMergeLeft(string resourceType)
         {
             return string.Equals(resourceType, "Microsoft.Storage/storageAccounts", StringComparison.OrdinalIgnoreCase);
         }

src/PSRule.Rules.Azure/Data/Template/TemplateVisitor.cs

@@ -1256,16 +1256,19 @@ private static ResourceValue ResourceInstance(TemplateContext context, JObject r
             var subscriptionId = context.Subscription.SubscriptionId;
             var resourceGroupName = context.ResourceGroup.Name;
 
-            // Handle special case for cross-scope deployments which may have an alternative subscription or resource group set
+            // Handle special case for cross-scope deployments which may have an alternative subscription or resource group set.
             if (IsDeploymentResource(type))
             {
                 subscriptionId = ResolveDeploymentScopeProperty(context, resource, PROPERTY_SUBSCRIPTIONID, subscriptionId);
                 resourceGroupName = ResolveDeploymentScopeProperty(context, resource, PROPERTY_RESOURCEGROUP, resourceGroupName);
             }
 
+            // Get scope if specified.
+            var scope = context.TryParentResourceId(resource, out var parentIds) && parentIds != null && parentIds.Length > 0 ? parentIds[0] : null;
+
             string resourceId = null;
             if (context.Deployment.DeploymentScope == DeploymentScope.ResourceGroup)
-                resourceId = ResourceHelper.CombineResourceId(subscriptionId, resourceGroupName, type, name);
+                resourceId = ResourceHelper.CombineResourceId(subscriptionId, resourceGroupName, type, name, scope: scope);
 
             if (context.Deployment.DeploymentScope == DeploymentScope.Subscription)
                 resourceId = ResourceHelper.CombineResourceId(subscriptionId, null, type, name);

tests/PSRule.Rules.Azure.Tests/ResourceHelperTests.cs

@@ -85,13 +85,15 @@ public void CombineResourceId()
             {
                 "microsoft.operationalinsights/workspaces",
                 "Microsoft.Network/virtualNetworks/subnets",
-                "Microsoft.KeyVault/vaults/providers/diagnosticSettings"
+                "Microsoft.KeyVault/vaults/providers/diagnosticSettings",
+                "Microsoft.Insights/diagnosticSettings"
             };
             var resourceName = new string[]
             {
                 "workspace001",
                 "vnet-A/GatewaySubnet",
-                "kv-bicep-app-002/Microsoft.Insights/service"
+                "kv-bicep-app-002/Microsoft.Insights/service",
+                "service"
             };
             var id = new string[]
             {
@@ -100,11 +102,13 @@ public void CombineResourceId()
                 "/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/test-rg/providers/Microsoft.KeyVault/vaults/kv-bicep-app-002/providers/Microsoft.Insights/diagnosticSettings/service",
                 "/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/test-rg"
             };
+            var scope = "/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/test-rg/providers/Microsoft.KeyVault/vaults/kv-bicep-app-002";
 
             Assert.Equal(id[0], ResourceHelper.CombineResourceId("00000000-0000-0000-0000-000000000000", "rg-test", resourceType[0], resourceName[0]));
             Assert.Equal(id[1], ResourceHelper.CombineResourceId("ffffffff-ffff-ffff-ffff-ffffffffffff", "test-rg", resourceType[1], resourceName[1]));
             Assert.Equal(id[2], ResourceHelper.CombineResourceId("ffffffff-ffff-ffff-ffff-ffffffffffff", "test-rg", resourceType[2], resourceName[2]));
             Assert.Equal(id[3], ResourceHelper.CombineResourceId("ffffffff-ffff-ffff-ffff-ffffffffffff", "test-rg", (string[])null, null));
+            Assert.Equal(id[2], ResourceHelper.CombineResourceId("ffffffff-ffff-ffff-ffff-ffffffffffff", "test-rg", resourceType[3], resourceName[3], scope: scope));
         }
 
         [Fact]

tests/PSRule.Rules.Azure.Tests/Template.Bicep.2.json

@@ -158,7 +158,7 @@
             "value": "kv-bicep-app-002"
           },
           "workspaceId": {
-              "value": "notworkspace"
+            "value": "notworkspace"
           }
         },
         "template": {
@@ -346,7 +346,7 @@
     {
       "type": "Microsoft.Resources/deployments",
       "apiVersion": "2020-06-01",
-      "name": "keyvault-deployment",
+      "name": "keyvault-deployment-2",
       "properties": {
         "expressionEvaluationOptions": {
           "scope": "inner"
@@ -357,7 +357,7 @@
             "value": "kv-bicep-app-003"
           },
           "workspaceId": {
-              "value": "[parameters('workspaceId')]"
+            "value": "[parameters('workspaceId')]"
           }
         },
         "template": {
@@ -543,4 +543,4 @@
       }
     }
   ]
-}
+}

tests/PSRule.Rules.Azure.Tests/TemplateVisitorTests.cs

@@ -644,7 +644,7 @@ public void MaterializedView()
         {
             var resources = ProcessTemplate(GetSourcePath("Tests.Bicep.8.json"), null);
             Assert.NotNull(resources);
-            Assert.Equal(8, resources.Length);
+            Assert.Equal(11, resources.Length);
 
             var storage = resources.FirstOrDefault(r => r["type"].ToString() == "Microsoft.Storage/storageAccounts");
             Assert.NotNull(storage);
@@ -662,6 +662,12 @@ public void MaterializedView()
             Assert.Equal("abc", sqlServer["properties"]["administrators"]["login"].ToString());
             Assert.Equal("ffffffff-ffff-ffff-ffff-ffffffffffff", sqlServer["identity"]["principalId"]);
             Assert.Equal("ffffffff-ffff-ffff-ffff-ffffffffffff", sqlServer["identity"]["tenantId"]);
+
+            var serviceBus = resources.FirstOrDefault(r => r["type"].ToString() == "Microsoft.ServiceBus/namespaces");
+            subResources = serviceBus["resources"].Values<JObject>().ToArray();
+            Assert.Single(subResources);
+            Assert.Equal("d2", subResources[0]["properties"]["other"].Value<string>());
+            Assert.Equal("/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/ps-rule-test-rg/providers/Microsoft.ServiceBus/namespaces/servicebus/providers/Microsoft.Insights/diagnosticSettings/logs", subResources[0]["id"].Value<string>());
         }
 
         [Fact]

tests/PSRule.Rules.Azure.Tests/Tests.Bicep.7.child.bicep

@@ -10,7 +10,7 @@ resource vault 'Microsoft.KeyVault/vaults@2021-10-01' existing = {
 
 resource kvSecret 'Microsoft.KeyVault/vaults/secrets@2021-10-01' = {
   parent: vault
-  name: 'secret1'
+  name: 'secret2'
   properties: {
     value: secret
   }