Fixed Resource group ID is incorrect under subscription scope #3198 (#…

…3199) * Fixed Resource group ID is incorrect under subscription scope #3198 * Add contribution
Azure · Dec 7, 2024 · fa40383 · fa40383
1 parent 375e4e5
commit fa40383
Show file tree

Hide file tree

Showing 9 changed files with 351 additions and 11 deletions.
diff --git a/docs/CHANGELOG-v1.md b/docs/CHANGELOG-v1.md
@@ -31,12 +31,17 @@ See [upgrade notes][1] for helpful information when upgrading from previous vers
 
 What's changed since pre-release v1.40.0-B0147:
 
+- General improvements:
+  - Added first time contributor guide in docs by @that-ar-guy.
+    [#3097](https://github.com/Azure/PSRule.Rules.Azure/issues/3097)
 - Engineering:
   - Quality updates to rule documentation by @BernieWhite.
     [#3102](https://github.com/Azure/PSRule.Rules.Azure/issues/3102)
 - Bug fixes:
   - Fixed evaluation of APIM policies when using embedded C# with quotes by @BernieWhite.
     [#3184](https://github.com/Azure/PSRule.Rules.Azure/issues/3184)
+  - Fixed Resource group ID is incorrect under subscription scope by @BernieWhite.
+    [#3198](https://github.com/Azure/PSRule.Rules.Azure/issues/3198)
 
 ## v1.40.0-B0147 (pre-release)
 

diff --git a/src/PSRule.Rules.Azure/Common/ResourceHelper.cs b/src/PSRule.Rules.Azure/Common/ResourceHelper.cs
@@ -215,22 +215,27 @@ internal static string ResourceId(string? tenant, string? managementGroup, strin
         return ResourceId(tenant, managementGroup, subscriptionId, resourceGroup, typeParts, nameParts, scopeId, depth);
     }
 
-    internal static string ResourceId(string? tenant, string? managementGroup, string? subscriptionId, string? resourceGroup, string[]? resourceType, string[]? resourceName, string? scopeId, int depth = int.MaxValue)
+    internal static string ResourceId(string? tenant, string? managementGroup, string? subscriptionId, string? resourceGroup, string[]? resourceTypeParts, string[]? resourceNameParts, string? scopeId, int depth = int.MaxValue)
     {
-        var resourceTypeLength = resourceType?.Length ?? 0;
-        var nameLength = resourceName?.Length ?? 0;
+        var resourceTypeLength = resourceTypeParts?.Length ?? 0;
+        var nameLength = resourceNameParts?.Length ?? 0;
         if (resourceTypeLength != nameLength)
-            throw new TemplateFunctionException(nameof(resourceType), FunctionErrorType.MismatchingResourceSegments, PSRuleResources.MismatchingResourceSegments);
+            throw new TemplateFunctionException(nameof(resourceTypeParts), FunctionErrorType.MismatchingResourceSegments, PSRuleResources.MismatchingResourceSegments);
 
         if (!ResourceIdComponents(scopeId, out var scopeTenant, out var scopeManagementGroup, out var scopeSubscriptionId, out var scopeResourceGroup, out var scopeResourceType, out var scopeResourceName))
         {
             scopeResourceType = null;
             scopeResourceName = null;
         }
 
-        resourceType = MergeResourceNameOrType(scopeResourceType, resourceType);
-        resourceName = MergeResourceNameOrType(scopeResourceName, resourceName);
-        return ResourceId(scopeTenant ?? tenant, scopeManagementGroup ?? managementGroup, scopeSubscriptionId ?? subscriptionId, scopeResourceGroup ?? resourceGroup, resourceType, resourceName, depth);
+        resourceTypeParts = MergeResourceNameOrType(scopeResourceType, resourceTypeParts);
+        resourceNameParts = MergeResourceNameOrType(scopeResourceName, resourceNameParts);
+        return ResourceId(scopeTenant ?? tenant, scopeManagementGroup ?? managementGroup, scopeSubscriptionId ?? subscriptionId, scopeResourceGroup ?? resourceGroup, resourceTypeParts, resourceNameParts, depth);
+    }
+
+    internal static string ResourceGroupId(string subscriptionId, string resourceGroup)
+    {
+        return string.Format("/subscriptions/{0}/resourceGroups/{1}", subscriptionId, resourceGroup);
     }
 
     private static string[]? MergeResourceNameOrType(string[]? parentNameOrType, string[]? nameOrType)

diff --git a/src/PSRule.Rules.Azure/Data/Template/TemplateVisitor.cs b/src/PSRule.Rules.Azure/Data/Template/TemplateVisitor.cs
@@ -70,6 +70,8 @@ internal abstract class TemplateVisitor : ResourceManagerVisitor
         private const string PROPERTY_ROOTDEPLOYMENT = "rootDeployment";
         private const string PROPERTY_NULLABLE = "nullable";
 
+        private const string TYPE_RESOURCE_GROUPS = "Microsoft.Resources/resourceGroups";
+
         internal sealed class TemplateContext : ResourceManagerVisitorContext, ITemplateContext
         {
             private const string CLOUD_PUBLIC = "AzureCloud";
@@ -1341,17 +1343,28 @@ private static ResourceValue ResourceInstance(TemplateContext context, JObject r
             var scope = context.TryParentResourceId(resource, out var parentIds) && parentIds != null && parentIds.Length > 0 ? parentIds[0] : null;
 
             string resourceId = null;
-            if (deploymentScope == DeploymentScope.ResourceGroup)
-                resourceId = ResourceHelper.ResourceId(tenant: null, managementGroup: null, subscriptionId: subscriptionId, resourceGroup: resourceGroupName, resourceType: type, resourceName: name, scopeId: scope);
 
+            // Handle special case when resource type is a resource group.
+            if (StringComparer.OrdinalIgnoreCase.Equals(type, TYPE_RESOURCE_GROUPS))
+            {
+                resourceId = ResourceHelper.ResourceGroupId(subscriptionId: subscriptionId, resourceGroup: name);
+            }
+            else if (deploymentScope == DeploymentScope.ResourceGroup)
+            {
+                resourceId = ResourceHelper.ResourceId(tenant: null, managementGroup: null, subscriptionId: subscriptionId, resourceGroup: resourceGroupName, resourceType: type, resourceName: name, scopeId: scope);
+            }
             else if (deploymentScope == DeploymentScope.Subscription)
+            {
                 resourceId = ResourceHelper.ResourceId(tenant: null, managementGroup: null, subscriptionId: subscriptionId, resourceGroup: null, resourceType: type, resourceName: name, scopeId: null);
-
+            }
             else if (deploymentScope == DeploymentScope.ManagementGroup)
+            {
                 resourceId = ResourceHelper.ResourceId(tenant: null, managementGroup: managementGroup, subscriptionId: null, resourceGroup: null, resourceType: type, resourceName: name, scopeId: scope);
-
+            }
             else if (deploymentScope == DeploymentScope.Tenant)
+            {
                 resourceId = ResourceHelper.ResourceId(tenant: "/", managementGroup: null, subscriptionId: null, resourceGroup: null, resourceType: type, resourceName: name, scopeId: "/");
+            }
 
             context.UpdateResourceScope(resource);
             resource[PROPERTY_ID] = resourceId;

diff --git a/tests/PSRule.Rules.Azure.Tests/Bicep/ScopeTestCases/BicepScopeTests.cs b/tests/PSRule.Rules.Azure.Tests/Bicep/ScopeTestCases/BicepScopeTests.cs
@@ -33,4 +33,28 @@ public void ProcessTemplate_WhenManagementGroupAtTenant_ShouldReturnCompleteProp
         Assert.Equal("ffffffff-ffff-ffff-ffff-ffffffffffff", actual["properties"]["tenantId"].Value<string>());
         Assert.Equal("/providers/Microsoft.Management/managementGroups/mg-01", actual["properties"]["details"]["parent"]["id"].Value<string>());
     }
+
+    [Fact]
+    public void ProcessTemplate_WhenResourceGroupFromSubscriptionScope_ShouldReturnCorrectIdentifiers()
+    {
+        var resources = ProcessTemplate(GetSourcePath("Bicep/ScopeTestCases/Tests.Bicep.2.json"), null, out _);
+
+        Assert.NotNull(resources);
+
+        var actual = resources.Where(r => r["name"].Value<string>() == "rg-1").FirstOrDefault();
+        Assert.Equal("Microsoft.Resources/resourceGroups", actual["type"].Value<string>());
+        Assert.Equal("rg-1", actual["name"].Value<string>());
+        Assert.Equal("/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/rg-1", actual["id"].Value<string>());
+
+        actual = resources.Where(r => r["name"].Value<string>() == "id-1").FirstOrDefault();
+        Assert.Equal("id-1", actual["name"].Value<string>());
+        Assert.Equal("/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/rg-1/providers/Microsoft.ManagedIdentity/userAssignedIdentities/id-1", actual["id"].Value<string>());
+
+        actual = resources.Where(r => r["name"].Value<string>() == "registry-1").FirstOrDefault();
+        Assert.Equal("registry-1", actual["name"].Value<string>());
+        Assert.Equal("/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/rg-1/providers/Microsoft.ContainerRegistry/registries/registry-1", actual["id"].Value<string>());
+
+        var property = actual["identity"]["userAssignedIdentities"].Value<JObject>().Properties().FirstOrDefault();
+        Assert.Equal("/subscriptions/ffffffff-ffff-ffff-ffff-ffffffffffff/resourceGroups/rg-1/providers/Microsoft.ManagedIdentity/userAssignedIdentities/id-1", property.Name);
+    }
 }
diff --git a/tests/PSRule.Rules.Azure.Tests/Bicep/ScopeTestCases/Tests.Bicep.2.bicep b/tests/PSRule.Rules.Azure.Tests/Bicep/ScopeTestCases/Tests.Bicep.2.bicep
@@ -0,0 +1,39 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+targetScope = 'subscription'
+
+param location string = deployment().location
+
+@description('')
+param tags object = {}
+
+resource rg 'Microsoft.Resources/resourceGroups@2024-07-01' = {
+  name: 'rg-1'
+  location: location
+  tags: tags
+}
+
+module id './Tests.Bicep.2.child2.bicep' = {
+  scope: rg
+  name: 'id-deploy'
+  params: {
+    name: 'id-1'
+    location: location
+    tags: tags
+  }
+}
+
+module registry './Tests.Bicep.2.child1.bicep' = {
+  scope: rg
+  name: 'registry-deploy'
+  params: {
+    location: location
+    name: 'registry-1'
+    tags: tags
+    identityId: id.outputs.id
+  }
+}
+
+output userAssignedIdentityId string = id.outputs.id
+output registryId string = registry.outputs.id
diff --git a/tests/PSRule.Rules.Azure.Tests/Bicep/ScopeTestCases/Tests.Bicep.2.child1.bicep b/tests/PSRule.Rules.Azure.Tests/Bicep/ScopeTestCases/Tests.Bicep.2.child1.bicep
@@ -0,0 +1,36 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+targetScope = 'resourceGroup'
+
+param name string
+param location string
+param identityId string
+param tags object
+
+resource containerRegistry 'Microsoft.ContainerRegistry/registries@2023-11-01-preview' = {
+  name: name
+  location: location
+  identity: {
+    type: 'UserAssigned'
+    userAssignedIdentities: {
+      '${identityId}': {}
+    }
+  }
+  sku: {
+    name: 'Premium'
+  }
+  tags: tags
+  properties: {
+    adminUserEnabled: false
+    policies: {
+      azureADAuthenticationAsArmPolicy: {
+        status: 'enabled'
+      }
+    }
+  }
+}
+
+output id string = containerRegistry.id
+output url string = containerRegistry.properties.loginServer
+output identity object = containerRegistry.identity.userAssignedIdentities
diff --git a/tests/PSRule.Rules.Azure.Tests/Bicep/ScopeTestCases/Tests.Bicep.2.child2.bicep b/tests/PSRule.Rules.Azure.Tests/Bicep/ScopeTestCases/Tests.Bicep.2.child2.bicep
@@ -0,0 +1,14 @@
+// Copyright (c) Microsoft Corporation.
+// Licensed under the MIT License.
+
+param name string
+param location string
+param tags object
+
+resource id 'Microsoft.ManagedIdentity/userAssignedIdentities@2023-01-31' = {
+  name: name
+  location: location
+  tags: tags
+}
+
+output id string = id.id