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.

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

[AVM Module Issue]: Support templating for defaults #133

Open
1 task done
JWilkinsonMB opened this issue Oct 21, 2024 · 7 comments
Open
1 task done

[AVM Module Issue]: Support templating for defaults #133

JWilkinsonMB opened this issue Oct 21, 2024 · 7 comments
Assignees
Labels
Language: Terraform 🌐 This is related to the Terraform IaC language Type: Feature Request ➕ New feature or request

Comments

@JWilkinsonMB
Copy link

JWilkinsonMB commented Oct 21, 2024

Check for previous/existing GitHub issues

  • I have checked for previous/existing GitHub issues

Issue Type?

Feature Request

(Optional) Module Version

v0.9.0-beta2

(Optional) Correlation Id

No response

Description

The current implementation of defaults requires the full value of the parameter to be provided. Can this be enhanced so that templated values are an option?

For example, there are 67 parameters for different DNS zones in the assignment Deploy-Private-DNS-Zones, e.g.

"azureFilePrivateDnsZoneId": {
 "value": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/placeholder/providers/Microsoft.Network/privateDnsZones//providers/Microsoft.Network/privateDnsZones/privatelink.afs.azure.net"
},
"azureAutomationWebhookPrivateDnsZoneId": {
 "value": "/subscriptions/00000000-0000-0000-0000-000000000000/resourceGroups/placeholder/providers/Microsoft.Network/privateDnsZones//providers/Microsoft.Network/privateDnsZones/privatelink.azure-automation.net"
},

In the above examples, the zone names on the end of the resource ID i.e. privatelink.afs.azure.net & privatelink.azure-automation.net are constants, as that zone name will always be used with that parameter (reference).

As it stands, to use this policy requires manually setting the full resource IDs for all 67 policies, including the zone names, e.g.

private_dns_zone_file = jsonencode({
      value = "/subscriptions/${var.connectivity_subscription_id}/resourceGroups/${var.private_dns_resource_group_name}/providers/Microsoft.Network/privateDnsZones/privatelink.afs.azure.net"
    })
private_dns_zone_automation_webhook = jsonencode({
      value = "/subscriptions/${var.connectivity_subscription_id}/resourceGroups/${var.private_dns_resource_group_name}/providers/Microsoft.Network/privateDnsZones/privatelink.azure-automation.net"
    })

This leaves room for error if the resource IDs aren't built correctly, for example using the wrong zone name which templating could avoid by just providing the subscription ID and resource group name, and have the rest filled in from the template in the default.

Further, in the context of ALZ I think we can be more opinionated about this, as the guidance is to have all private DNS zones within a central connectivity subscription, therefore there's the opportunity to consolidate the setting of 67 defaults into just 1, which will make it much easier to adopt and keep the terraform code cleaner as the logic can be moved into the default definition.

Note that Government & China clouds have different zone names, so the above doesn't hold true for those clouds as the zone name would need updating for each resource, however I believe that could be better addressed by providing a different default file specific to each cloud.

@JWilkinsonMB JWilkinsonMB added Language: Terraform 🌐 This is related to the Terraform IaC language Needs: Triage 🔍 Maintainers need to triage still labels Oct 21, 2024
@matt-FFFFFF matt-FFFFFF self-assigned this Oct 24, 2024
@matt-FFFFFF matt-FFFFFF removed the Needs: Triage 🔍 Maintainers need to triage still label Oct 24, 2024
@matt-FFFFFF
Copy link
Member

Hey, thanks for the suggestion!

I have considered this feature and think that putting in the logic to create the full set of defaults (or policy assignments to modify) would mean that the module is tightly coupled to the library version. This is something we want to avoid as it makes maintenance more difficult.

I think this is actually an issue with the policy set definition. We could be more opinionated here and accept the two parameters you suggest - resource group name and subscription id.

I will take this to the team and let you know.

@JWilkinsonMB
Copy link
Author

Thanks, I agree it wouldn't make sense to create the full set of defaults here. What I was thinking was that instead of having the value replaced in full like this:

private_dns_zone_file = jsonencode({
      value = "/subscriptions/${var.private_dns_subscription_id}/resourceGroups/${var.private_dns_resource_group}/providers/Microsoft.Network/privateDnsZones/privatelink.afs.azure.net"
    })

You could have:

private_dns_zone = {
      resource_group = var.private_dns_resource_group
      subscription_id = var.private_dns_subscription_id
    }

In the library you could have a default that's something like this:

{
    "default_name": "private_dns_zone",
    "policy_assignments": [
        {
            "parameter_names": [
                "azureFilePrivateDnsZoneId",
                "azureAutomationWebhookPrivateDnsZoneId",
                "azureAutomationDSCHybridPrivateDnsZoneId"
            ],
            "temmplate_values": {
                "resource_group": "$resource_group$",
                "subscription_id": "$subscription_id$"
            },
            "policy_assignment_name": "Deploy-Private-DNS-Zones"
        }
    ]
}

Then you could drop those replacements in to the assignment within the library:

"azureFilePrivateDnsZoneId": {
        "value": "/subscriptions/$subscription_id$/resourceGroups/$resource_group$/providers/Microsoft.Network/privateDnsZones/privatelink.afs.azure.net"
    },
    "azureAutomationWebhookPrivateDnsZoneId": {
        "value": "/subscriptions/$subscription_id$/resourceGroups/$resource_group$/providers/Microsoft.Network/privateDnsZones/privatelink.azure-automation.net"
    },
    "azureAutomationDSCHybridPrivateDnsZoneId": {
        "value": "/subscriptions/$subscription_id$/resourceGroups/$resource_group$/providers/Microsoft.Network/privateDnsZones/privatelink.azure-automation.net"
    }

@matt-FFFFFF
Copy link
Member

I will have a think on how we could implement this - thanks for the suggestion

@matt-FFFFFF
Copy link
Member

Please see updated library: https://github.com/Azure/Azure-Landing-Zones-Library/releases/tag/platform%2Falz%2F2024.11.0

We have modified the policy set definition to accept simplified parameters. Does this solve this particular issue? @JWilkinsonMB

@matt-FFFFFF matt-FFFFFF added the Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author label Nov 6, 2024
@JWilkinsonMB
Copy link
Author

Conceptually it solves the Private DNS issue that originated the issue, I think templating still has merit as it's more flexible if upstream policies can't be changed, but I don't have another example of that.

Practically though it looks like it's broken things. alzlib looks to only support extracting a simple parameter assignment as the value when generating role assignments, so when it comes across the template function it fails.
https://github.com/Azure/alzlib/blob/bf74df7aa03c38840a8443eebb3417e44aa1439b/deployment/managementgroup.go#L400-L406

Planning failed. Terraform encountered an error while generating this plan.

╷
│ Error: architectureDataSource.Read() Error generating policy role assignments
│
│   with module.alz.module.alz_global.data.alz_architecture.this,
│   on .terraform\modules\alz.alz_global\main.tf line 1, in data "alz_architecture" "this":
│    1: data "alz_architecture" "this" {
│
│ Hierarchy.PolicyRoleAssignments: error generating additional role assignments for management group `global-landingzones-corp`: ManagementGroup.GeneratePolicyAssignmentAdditionalRoleAssignments: error
│ extracting parameter name from ARM function `[if(equals(parameters('dnsZoneSubscriptionId'), ''), parameters('azureFilePrivateDnsZoneId'), format('/subscriptions/{0}/resourceGroups/{1}/providers/{2}/{3}',    
│ parameters('dnsZoneSubscriptionId'), toLower(parameters('dnsZoneResourceGroupName')), parameters('dnsZoneResourceType'), replace(replace(parameters('dnsZoneNames').azureFilePrivateDnsZoneId, '{regionName}',  
│ parameters('dnsZoneRegion')), '{regionCode}', parameters('dnzZoneRegionShortNames')[parameters('dnsZoneRegion')])))]`: value is not a parameter reference

@microsoft-github-policy-service microsoft-github-policy-service bot added Needs: Attention 👋 Reply has been added to issue, maintainer to review and removed Needs: Author Feedback 👂 Awaiting feedback from the issue/PR author labels Nov 6, 2024
@matt-FFFFFF
Copy link
Member

Hi yes this slipped through the testing net... bear with us whilst we come up with something

@matt-FFFFFF
Copy link
Member

raised Azure/alzlib#187 to track

@matt-FFFFFF matt-FFFFFF removed the Needs: Attention 👋 Reply has been added to issue, maintainer to review label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language: Terraform 🌐 This is related to the Terraform IaC language Type: Feature Request ➕ New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants