adopt vnet approach from subvending LZ module & modify subnets to work with Azure Policy

[kewalaka]

Description

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

• functionally, this allows a second team to create subnets within a vnet, without terraform overwriting the subnets

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;

• The 'complete' example is now much more complete. It deploys all the various resources such as routes, NSGs, DDOS (but disabled, for cost saving), etc.
• The 'default' example just deploys the minimal vnet
• There is an additional example to test (3)

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

• vnet_location is now location
• virtual_network_dns_servers is dns_servers
• virtual_network_ddos_protection_plan is ddos_protection_plan
• virtual_network_address_space is address_space
• vnet_peering_config is peerings (?)

... 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

• [ ] Non-module change (e.g. CI/CD, documentation, etc.)
• [X] Azure Verified Module updates:
  • [ ] Bugfix containing backwards compatible bug fixes, and I have NOT bumped the MAJOR or MINOR version in locals.version.tf.json:
  • [ ] Someone has opened a bug report issue, and I have included "Closes #{bug_report_issue_number}" in the PR description.
  • [ ] The bug was found by the module author, and no one has opened an issue to report it yet.
  • [ ] Feature update backwards compatible feature updates, and I have bumped the MINOR version in locals.version.tf.json.
  • [X] Breaking changes and I have bumped the MAJOR version in locals.version.tf.json. edit: bumped the minor version as we are still < v1.0
  • [X] Update to documentation

Checklist

• [X] I'm sure there are no other open Pull Requests for the same update/change (except those noted that this supersedes)
• [X] My corresponding pipelines / checks run clean and green without any errors or warnings
• [X] I did run all pre-commit checks

[kewalaka]

@matt-FFFFFF I had to use a 'wait for' due to occasional conflicts when testing, is there a better way?

I don't know if this is something that should be noted upstream, it seems like APIs are releasing op locks too early.

[kewalaka]

as an aside, the current approach uses a data source to grab the subscription details

data "azurerm_subscription" "this" {}

I'm wondering if :

data "azurerm_client_config" "this" {}

... might be a better choice

[kewalaka]

I've raised this on the AzAPI repo noting the issues with the 409 conflict errors.

I'm thinking also it would be smart to introduce a 'wait for subnet' operations block too , then as a workaround if necessary a caller could call this resource module twice to work around this issue. (unless there's a better way)

[haflidif]

I've raised this on the AzAPI repo noting the issues with the 409 conflict errors.

I'm thinking also it would be smart to introduce a 'wait for subnet' operations block too , then as a workaround if necessary a caller could call this resource module twice to work around this issue. (unless there's a better way)

Yeah It's the same issue I stumbled up on during the development of my module, when I was testing the different scenarios on the same vnet but calling my module multiple times in the same vnet, I suspect it's only few seconds that it takes to be ready for each subnet, but enough to hit the operation still in progress error - I would recommend to introduce some time wait block, but not sure how to do that when using a for each loop inside the module to create multiple subnets.

[kewalaka]

@haflidif hopefully it can be fixed, in the meantime I have added a time wait and an example to the tests that illustrates how to work around it. Unaware of any sensible ways you can put a pause between 'for_each' objects. Just about to test it against my own runners.

[matt-FFFFFF]

See this.

https://github.com/Azure/terraform-provider-azapi/issues/503#issuecomment-2106030842

[kewalaka]

See this.

Azure/terraform-provider-azapi#503 (comment)

onto it.

[kewalaka]

fixed with no time waits required, pulled them out. I've converted the vnet peering to azapi too so i can use the same there.

thanks @matt-FFFFFF !

i believe this is ready for review again.

[kewalaka]

LGTM 🚀 we should include some information on how to migrate from the last version to the new version or are we still just in the development phase so we don't need to include any migration guides between versions ?

That's a question for the AVM core team - I've started with some notes above to call it out - but it is incomplete.

This is quite a change between versions, as the module is switching from using azurerm provider to use AzAPI provider.

I don't think he change from AzureRM to AzAPI is so important as that is internal to the module - what's more important is the naming of variables (inputs) and outputs have changed (I believe the changes are all necessary, to align with the spec, or for consistency). The internal implementation should be changeable without notification as long as the usage is the same - but in this case the usage has changed as well.

[haflidif]

LGTM 🚀 we should include some information on how to migrate from the last version to the new version or are we still just in the development phase so we don't need to include any migration guides between versions ?

That's a question for the AVM core team - I've started with some notes above to call it out - but it is incomplete.

This is quite a change between versions, as the module is switching from using azurerm provider to use AzAPI provider.

I don't think he change from AzureRM to AzAPI is so important as that is internal to the module - what's more important is the naming of variables (inputs) and outputs have changed (I believe the changes are all necessary, to align with the spec, or for consistency). The internal implementation should be changeable without notification as long as the usage is the same - but in this case the usage has changed as well.

Absolutely, I totally agree that normally internal module changes are not required to be notified specially, but in this case here, if you just go from version 0.1.4 to 0.2.0 even though you have not change any inputs required for the module, you will get notification on "Destroy" because the azurerm provider is no longer being used to create the virtual networks / subnets hence the state file doesn't match the references. - and then you need to import all existing resources, that alone I think is worth mentioning in breaking changes at least.

[kewalaka]

[resources will be destroyed and recreated because] the azurerm provider is no longer being used to create the virtual networks / subnets hence the state file doesn't match the references

edit I had thought that perhaps moved blocks would work but of course because the resource types are different it won't. hmm.. I wonder if 'moved' was done from outside the module it would work?

edit2 can't see how moved would help even if called outside the module, looking at the behaviour it just seems to rename the actual resource in the state & then re-run the plan - whereas when look at the state the representation of azapi & azurerm resource is different - so a simple rename won't cut it.

Don't have a good answer here - any thoughts @matt-FFFFFF ?

[haflidif]

[resources will be destroyed and recreated because] the azurerm provider is no longer being used to create the virtual networks / subnets hence the state file doesn't match the references

edit I had thought that perhaps moved blocks would work but of course because the resource types are different it won't. hmm.. I wonder if 'moved' was done from outside the module it would work?

edit2 can't see how moved would help even if called outside the module, looking at the behaviour it just seems to rename the actual resource in the state & then re-run the plan - whereas when look at the state the representation of azapi & azurerm resource is different - so a simple rename won't cut it.

Don't have a good answer here - any thoughts @matt-FFFFFF ?

Yeah I think moved block would do the job, nice solution! 👏

[kewalaka]

Yeah I think moved block would do the job, nice solution! 👏

Sorry as per my edits I don't think the moved block will help 😕. Open to suggestions...

[matt-FFFFFF]

Can we use import & removed blocks?

[kewalaka]

Can we use import & removed blocks?

sounds painful 😅, but ok - i might need some help testing this. My MSDN subscription has ran out of money until +2 weeks 😿

[kewalaka]

I am unsure why we need to supply the subscription_id as a variable since we are supplying the resource id of the vnet if using an existing one?

explained sub requirement in the code feedback, a copy:

Optionally this allows vnets to be made in a different subscription.

Can't always be inferred from existing vnet as this is optional.

If not there this will mean resources are made in the current sub.

Is this trying to be too clever, maybe. In theory this module as it stands could drop into to LZ module too.

[haflidif]

Can we use import & removed blocks?

sounds painful 😅, but ok - i might need some help testing this. My MSDN subscription has ran out of money until +2 weeks 😿

I can help with that! I've enough credits on my subscriptions 😊 just tell me what you need ? 💪

[kewalaka]

Can we use import & removed blocks?

sounds painful 😅, but ok - i might need some help testing this. My MSDN subscription has ran out of money until +2 weeks 😿

I can help with that! I've enough credits on my subscriptions 😊 just tell me what you need ? 💪

Matt is suggesting using recently added TF capability to update the state - using removed {} and import {}. These take a source & destination parameter:

They work the same as terraform state rm and terraform import, except they can be defined in the HCL, which helps in situations like this where we need to shift resources between different resource types.

See: https://developer.hashicorp.com/terraform/language/modules/syntax#removing-modules as an example for "removed".

If it was just refactoring, and the same resource type, then moved {} would do the trick.

In terms of approach, what I'd suggest is use the new "complete" example from this new PR, because it has more coverage, against the current version of the module, and then the challenge is to make it migrate to this version using the import & removed blocks.

[kewalaka]

Update; just realised the subnet outputs are in need of love too - I've posted c653c98

[jaredfholgate]

@kewalaka Given the scale of this change and the likelihood that I am going to add code to it as I test it, I have migrated your branch over to the main repo. Unfortunately, there is no way to change the source branch for a PR, so I'll need to close this PR and open a new one. All your commits are still there with your name on them, so you'll still get the credit for this.

[kewalaka]

@kewalaka Given the scale of this change and the likelihood that I am going to add code to it as I test it, I have migrated your branch over to the main repo. Unfortunately, there is no way to change the source branch for a PR, so I'll need to close this PR and open a new one. All your commits are still there with your name on them, so you'll still get the credit for this.

I won't be credited because the squash commit will show it coming from the person who merges the PR (unless I'm mistaken), but I'm not that fussed about it 😂

[matt-FFFFFF]

You'll get credit for the lines of code!