search

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

resource alignment with CARML & AVM standards #22

Closed kewalaka closed 7 months ago

kewalaka commented 8 months ago

Quoting module guidance

Resource modules: resource modules .... MUST mapped 1:1 to RPs (resource providers) and top level resources

for the Microsoft.Network/virtualNetworks resource provider, this means supporting the virtual network resource and the following child resources:

The ddos network protection plan doesn't fit in this hierarchy, should it be in this module? (I'm aware it is conditionally created and you can just supply the DDOS plan id).

All said, however separating it out is likely to break the following guidance:

Resource modules: resource modules have bring extra value to the end user (can’t just be simple wrappers)

.. so I"m interested to hear AVM core team feedback on how best to approach that.

Currently, Bicep in CARML aligns to the resource provider mapping and treats the ddos protection plan as a separate resource, and alignment to Bicep is desirable,

ref: https://azure.github.io/Azure-Verified-Modules/specs/shared/#id-snfr21---category-publishing---cross-language-collaboration

herms14 commented 8 months ago

happy to align with what the CARML implementation.

@Azure/avm-core-team would like to hear your thoughts on this feedback?

AlexanderSehr commented 8 months ago

Regarding subsets and peering yes, that would be ideal (including the optional reverse peering). I just brought it up with the team and while strongly correlated services are usually also ok (like A NIC for a VM, or public IP for a firewall), it was argued that DDOS should be treated as a seperate deployment with the VNET module accepting it as an optional input. The module also seems to already be in development.

herms14 commented 8 months ago

got it. Thanks! will remove the DDOS capability then.

tanvp112 commented 7 months ago

...accepting it as an optional input

Please don't remove but let us have the option to enable/disable it.

herms14 commented 7 months ago

This has been fixed in PR #20

herms14 commented 7 months ago

This has been fixed in PR https://github.com/Azure/terraform-azurerm-avm-res-network-virtualnetwork/pull/20