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

feat: allow subnets to be created against an existing vnet #43

Closed kewalaka closed 1 month ago

kewalaka commented 4 months ago

In larger orgs, vnet creation & subnet creation is often done by different teams. A typical pattern is for the platform team to create the vnet & peering via a subscription vending approach, and then the app workload team manage the subnets.

I have an approach based on this in this event hub namespace, and I've discussed the approach with @matt-FFFFFF previously, though it hasn't been formally accepted.

e.g. check out the way the namespace is made optional here: https://github.com/kewalaka/terraform-azurerm-avm-res-eventhub-namespace/

The suggested approach is based on this pattern:

https://azure.github.io/Azure-Verified-Modules/specs/terraform/#id-tfnfr11---category-code-style---null-comparison-as-creation-toogle

The ask is to be able to deploy a subnet within an existing vnet (for instance, one created by subscription vending).

### Tasks
matt-FFFFFF commented 4 months ago

Hi @kewalaka

We like the implementation you have selected in that module and plan to issue guidance to the same effect for all resource modules that have child resources.

herms14 commented 4 months ago

Hi @kewalaka ,

Will look into this feature request. Thanks!

matt-FFFFFF commented 4 months ago

@herms14 after investigation it's best to not use a data source here. It can cause issues with the dependency graph.

Instead parse the supplied resource id as a string. Make sure to use a variable validation rule for the regex.

kewalaka commented 4 months ago

for the curious, the reason to avoid the data block is the same as this:

https://github.com/Azure/Azure-Verified-Modules/issues/313#issuecomment-2010359818

(in summary, any change to the properties of the data block, even if the resource does not depend on it, will trigger a delete & recreate).

thanks to Matt for the explanation!

herms14 commented 3 months ago

@kewalaka @matt-FFFFFF I have submitted PR #62 for this feature request. For your review

kewalaka commented 3 months ago

haha so funny - i was writing the same thing around the same time :)

https://github.com/kewalaka/terraform-azurerm-avm-res-network-virtualnetwork/tree/feat/snet_with_existing_vnet

I'll take a look at @herms14 version - probably tomorrow as I have things to do this evening.

edit i like @herms14 version more already - now i see why the resource id, of course, i was wondering how to avoid a data lookup for the sub id (noting the concern about data sources in general).

great timing too - just on the verge of wanting to use this for a customer, which is why i was just messing around with it myself too.

many thanks @herms14 - will look properly tmw, but on initial glance it looks good!

kewalaka commented 3 months ago

@kewalaka @matt-FFFFFF I have submitted PR #62 for this feature request. For your review

posted a few comments - a bit of crossover with Matt - I didn't see that was already there.

kewalaka commented 2 months ago

Looping back to this issue @jaredfholgate's approach to use a submodule feels more elegant, leaving the exisiting parent to be set via a flag rather than passed in by an id.

Where it makes sense, sub resources to be made by calling the sub module directly.

I'm thinking of refactoring my SQL mi and SQL database modules to do the same for the instance vs database split.

Again it is role seperation driving this requirement.

Thoughts? Is this the way now? @matt-FFFFFF