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
17 stars 14 forks source link

Feat/use azapi for subnets #83

Closed jaredfholgate closed 1 month ago

jaredfholgate commented 1 month ago

Description

NOTE: This pull request is a clone of https://github.com/Azure/terraform-azurerm-avm-res-network-virtualnetwork/pull/74 submitted by @kewalaka. We just had to migrate to an un forked branch on this occasion for ease of testing, etc.

1) This ports the approach used in the sub vending LZ module

2) This also ports the example from @haflidif where subnets are also created using azapi, to allow an immediate association with an NSG & route table, along with aligning it to the AVM way. As noted, this fixes deployments when faced with typical Azure Policy restrictions.

3) This includes the logic to be able to deploy subnets into an existing vnet (fixes #43)

4) On the examples;

5) This fixes a number of variables that do not follow AVM spec, noting;

... and the outputs have been updated to align more closely with AVM too.

6) Change works with recent changes in azapi 1.13

The code passes lint, docs & grept apply checks.

Fixes #43, #65, #71, #73 Supersedes #62

-->

Type of Change

Checklist

jaredfholgate commented 1 month ago

@matt-FFFFFF How important is it that we provide an automated migration path here? There is a lot of change happening here. Adding the removed and import blocks to the examples is certainly feasible, but given we are pre-v1.0 I'm wondering how much time we should spend on it. I appreciate that users have probably already adopted the module, but in order to fully automate this we would need to do it in an external script. We'd need to find the resource ids from their state and then create remove and import blocks or commands. Interested in your thoughts as to how far we should go with this.

jaredfholgate commented 1 month ago

@matt-FFFFFF Refactored this a bit more following our discussion yesterday. Not specifically supporting the sub module being used independently, but it is there if we want to take that approach. Keen to get your thoughts. Also moved away from supplying the resource_id for existing VNETs and instead re-using the existing variables to reduce clunkiness. Final change is to make it so that subnet role assignments actually work. I will consider splitting out peerings into a sub module too, but can wait until later.

I did add support for reverse peerings too, so that the module supports an end to end solution. Otherwise there was a circular dependency and you could only ever do half a peering.

kewalaka commented 1 month ago

thanks for helping finish this off @jaredfholgate - LGTM - what do we need to do before pressing the Big Green Button & making a release?

matt-FFFFFF commented 1 month ago

I think we probably need it to be Monday 😂

Been a long week but glad to get there. I'm sure we will get this released soon.

kewalaka commented 1 month ago

just doing some quick usage testing, spotted shouldn't these be nullable = false :

private_link_service_network_policies_enabled & private_endpoint_network_policies

in https://github.com/kewalaka/terraform-azurerm-avm-res-network-virtualnetwork/blob/feat/use-azapi-for-subnets/modules/subnet/variables.tf

jaredfholgate commented 1 month ago

@kewalaka, @haflidif and @matt-FFFFFF. I'll get this release out today.