Azure / terraform-azurerm-avm-res-network-virtualnetwork

Azure Verified Module for Virtual Network
https://registry.terraform.io/modules/Azure/avm-res-network-virtualnetwork
MIT License
18 stars 14 forks source link

approach to defining associations between subnets & related resources #17

Closed kewalaka closed 7 months ago

kewalaka commented 8 months ago

Currently, the code creates a list of subnets like this:

  subnet_prefixes     = ["10.0.1.0/24", "10.0.2.0/24", "10.0.3.0/24"]
  subnet_names        = ["subnet1", "subnet2", "subnet3"]

& associations are done like this (this example illustrates service delegations:

  subnet_service_endpoints = {
    subnet1 = ["Microsoft.Storage"]
    subnet2 = ["Microsoft.Sql", "Microsoft.AzureActiveDirectory"]
  }

A more "terraform-like" way to do this is illustrated in this repository:

https://github.com/Azure/terraform-azurerm-subnets

An example from this project:

  subnets = {
    subnet0 = {
      address_prefixes                          = ["10.0.0.0/24"]
      private_endpoint_network_policies_enabled = false
      service_endpoints = [
        "Microsoft.Storage", "Microsoft.Sql"
      ]
      delegations = [
        {
          name = "Microsoft.Sql.managedInstances"
          service_delegation = {
            name = "Microsoft.Sql/managedInstances"
            actions = [
              "Microsoft.Network/virtualNetworks/subnets/join/action",
              "Microsoft.Network/virtualNetworks/subnets/prepareNetworkPolicies/action",
              "Microsoft.Network/virtualNetworks/subnets/unprepareNetworkPolicies/action",
            ]
          }
        }
      ]
    }
    subnet1 = {
      address_prefixes                          = ["10.0.1.0/24"]
      private_endpoint_network_policies_enabled = false
      service_endpoints                         = ["Microsoft.AzureActiveDirectory"]
    }

This is better as it allows you to specify the subnet hierarchy as a typed variable, instead of the current method which is more prone to error.

It also improves readability as the number of subnets grows, since the information relating to a subnet is in the same object definition.

@herms14 - would you support a PR to align to this approach?

kewalaka commented 8 months ago

Just to add more weight to this suggestion, the current code in this module is based on a resource that has been deprecated (terraform-azurerm-vnet), according to the notice inside the readme:

For the new infrastructure, you should use this module (https://github.com/Azure/terraform-azurerm-subnets) instead of terraform-azurerm-vnet. For existing infrastructure, we'll maintain terraform-azurerm-vnet module, fix bugs and amend new features.

herms14 commented 8 months ago

Hi @kewalaka,

Thanks for the feedback. You are right, the subnet should be converted into an object to support all of those dependencies. I'm working on improving this and is scheduled for the next update.

Feel free to submit a PR as well @kewalaka if you have it already :)

Thanks!

kewalaka commented 8 months ago

@herms14 no worries glad to contribute help 🙂 I was just checking before I created something, but what I was going to propose was to swap out the main body of the code for the content here - https://github.com/Azure/terraform-azurerm-subnets

There will be some features we may need to fold back in, but I think that's a better starting point. I'll raise a PR to illustrate, hopefully in the next couple days.

herms14 commented 8 months ago

Hi @kewalaka , I already have the code ready and is pending review by the AVM core team. Will update once PR has been approved.

herms14 commented 7 months ago

Fixed in PR #20