search

Azure / Azure-Verified-Modules

Azure Verified Modules (AVM) is an initiative to consolidate and set the standards for what a good Infrastructure-as-Code module looks like. Modules will then align to these standards, across languages (Bicep, Terraform etc.) and will then be classified as AVMs and available from their respective language specific registries.
https://aka.ms/AVM
MIT License
321 stars 68 forks source link

[Module Proposal]: `avm/res/network/p2s-vpn-gateway` #1314

Open ericscheffler opened 3 weeks ago

ericscheffler commented 3 weeks ago

Check for previous/existing GitHub issues/module proposals

Check this module doesn't already exist in the module indexes

Bicep or Terraform?

Bicep

Module Classification?

Resource Module

Module Name

avm/res/network/p2s-vpn-gateway

Module Details

Proposing creation of a new module to provision P2S VPN gateways within a Virtual WAN (VWAN) hub, using the Microsoft.Network/p2svpnGateways resource provider. This module would be a prerequisite to publishing an AVM pattern for Azure VWAN, and is currently a gap. This module would also require and depend on another module (maybe a child?) for VWAN VPN Server configurations, and would use the Microsoft.Network/vpnServerConfigurations resource provider.

Do you want to be the owner of this module?

Yes

Module Owner's GitHub Username (handle)

ericscheffler

(Optional) Secondary Module Owner's GitHub Username (handle)

No response

matebarabas commented 3 weeks ago

Hi @ericscheffler,

Thanks for requesting/proposing to be an AVM module owner!

We just want to confirm you agree to the below pages that define what module ownership means:

Any questions or clarifications needed, let us know!

If you agree, please just reply to this issue with the exact sentence below (as this helps with our automation 👍):

"I CONFIRM I WISH TO OWN THIS AVM MODULE AND UNDERSTAND THE REQUIREMENTS AND DEFINITION OF A MODULE OWNER"

Thanks,

The AVM Core Team

RR

matebarabas commented 3 weeks ago

@AlexanderSehr, can you please help clarify the following question? In my understanding, due to some limitations we have/had in our CI environment, the naming convention for resource modules says, only start a new word, separated by a hyphen, when the RT's name starts with an upper-case character. This resource in question is called Microsoft.Network/p2svpnGateways see this for more details. Hence, the name must be avm/res/network/p2svpn-gateway (1) instead of avm/res/network/p2s-vpn-gateway (2), which would be easier to read. Can you please advise and let me know if we must go with (1) or is it ok to preceed with option (2)? Thanks!

AlexanderSehr commented 3 weeks ago

@AlexanderSehr, can you please help clarify the following question? In my understanding, due to some limitations we have/had in our CI environment, the naming convention for resource modules says, only start a new word, separated by a hyphen, when the RT's name starts with an upper-case character. This resource in question is called Microsoft.Network/p2svpnGateways see this for more details. Hence, the name must be avm/res/network/p2svpn-gateway (1) instead of avm/res/network/p2s-vpn-gateway (2), which would be easier to read. Can you please advise and let me know if we must go with (1) or is it ok to preceed with option (2)? Thanks!

Hey @matebarabas, I can't be 100% sure, but I think you can add as many - as you want to the provider namespace & resource type. The only effect I can think of additional - would have is that they'll be passed through to the name of the module in the registry itself (so not just the folder name). @eriqua would you have anything else in mind?

ericscheffler commented 3 weeks ago

I CONFIRM I WISH TO OWN THIS AVM MODULE AND UNDERSTAND THE REQUIREMENTS AND DEFINITION OF A MODULE OWNER

matebarabas commented 3 weeks ago

Hi @ericscheffler,

Thanks for confirming that you wish to own this AVM module and understand the related requirements and responsibilities!

Before starting development, please ensure ALL the following requirements are met.

Please use the following values explicitly as provided in the module index page:

Check if this module exists in the other IaC language. If so, collaborate with the other owner for consistency. 👍

You can now start the development of this module! ✅ Happy coding! 🎉

Please respond to this comment and request a review from the AVM core team once your module is ready to be published! Please include a link pointing to your PR, once available. 🙏

Any further questions or clarifications needed, let us know!

Thanks,

The AVM Core Team

matebarabas commented 3 weeks ago

@AlexanderSehr, can you please help clarify the following question? In my understanding, due to some limitations we have/had in our CI environment, the naming convention for resource modules says, only start a new word, separated by a hyphen, when the RT's name starts with an upper-case character. This resource in question is called Microsoft.Network/p2svpnGateways see this for more details. Hence, the name must be avm/res/network/p2svpn-gateway (1) instead of avm/res/network/p2s-vpn-gateway (2), which would be easier to read. Can you please advise and let me know if we must go with (1) or is it ok to preceed with option (2)? Thanks!

Hey @matebarabas, I can't be 100% sure, but I think you can add as many - as you want to the provider namespace & resource type. The only effect I can think of additional - would have is that they'll be passed through to the name of the module in the registry itself (so not just the folder name). @eriqua would you have anything else in mind?

@ericscheffler, please keep your eyes on this conversation as we haven't fully finalized the name of this module yet. Thanks!

ericscheffler commented 3 weeks ago

@AlexanderSehr or @matebarabas; I'm starting to build this out, but wanted to get a read on whether I should create the VPN Server Configuration (Microsoft.Network/vpnServerConfigurations) as a child module to this one (p2s-vpn-gateway), or as a standalone module of its own?

matebarabas commented 3 weeks ago

@ericscheffler, as that's a different RP/RT pair, by default, it should be a dedicated resource module. However, if its sole purpose of existent (and only use case) is to be used in the p2svpn-gateway module, then it's an exception, like the NICs or disks are for VMs in the VM module. As I'm not familiar with this resource type, can you please provide some more details on this? Thanks!

ericscheffler commented 2 weeks ago

@matebarabas I think it's fair to say that the sole purpose of the vpnServerConfigurations RP is to create an identity and routing configuration that is used by the p2svpnGateways RP (at least, that's the only reason I've ever used it...), so it sounds like it would be an exception like you described. That said, I'm happy to write it either way.

matebarabas commented 2 weeks ago

In that case, it sounds reasonable to me to continue this direction. However, I'd like to get a second opinion from someone on the @Azure/avm-core-team-technical-bicep team.

Please note that according to the two examples available in the Azure Resource Reference, the Microsoft.Network/vpnServerConfigurations resource type is indeed only used as part of the Microsoft.Network/p2sVpnGateways resource. The one other question I can think of that should be considered if a one-to-many relationsship is possible between these two, and how that would be managed. If it's not thing, having both resources under the "perceived parent" resource seems to be the best way forward, such as it is in case of the VM module.

AlexanderSehr commented 2 weeks ago

In that case, it sounds reasonable to me to continue this direction. However, I'd like to get a second opinion from someone on the @Azure/avm-core-team-technical-bicep team.

Please note that according to the two examples available in the Azure Resource Reference, the Microsoft.Network/vpnServerConfigurations resource type is indeed only used as part of the Microsoft.Network/p2sVpnGateways resource. The one other question I can think of that should be considered if a one-to-many relationsship is possible between these two, and how that would be managed. If it's not thing, having both resources under the "perceived parent" resource seems to be the best way forward, such as it is in case of the VM module.

I think we may be overcomplicating this a little bit. Both Microsoft.Network/vpnServerConfigurations & Microsoft.Network/p2sVpnGateways are dedicated resource types. As such, we can have dedicated modules for them that can be published independently. The calling solution can then deploy the vpnServerConfiguration into / as part of the p2sVpnGateways resource. Just as we do for VirtualWan & VirtualHub etc.

The 'exception', refering to Nic & Disc, does not really work here in my opinon as both NIC & Disc are also dedicated resource types that are published independently of the VM and are referenced by the same. What we could argue though is that just like in the VM's case, the p2sVpnGateways module could reference the published vpnServerConfigurations module and make one optionally deployable through the other.

ericscheffler commented 2 weeks ago

I'm fine with writing it as two separate resources. I know it will require more writing (additional tests, etc.), and this is probably my inexperience talking, but I feel like this is a simpler approach and makes calling it from a pattern more transparent and simpler. Are there any other pros/cons I might be missing?

matebarabas commented 2 weeks ago

Alright, so based on Alex's guidance, let's split this up to 2 dedicated resource modules. Can you please file another module proposal for avm/res/network/vpn-server-configuration? Thanks!

ericscheffler commented 2 weeks ago

Alright, so based on Alex's guidance, let's split this up to 2 dedicated resource modules. Can you please file another module proposal for avm/res/network/vpn-server-configuration? Thanks!

Added #1333 !

eriqua commented 2 weeks ago

@AlexanderSehr, can you please help clarify the following question? In my understanding, due to some limitations we have/had in our CI environment, the naming convention for resource modules says, only start a new word, separated by a hyphen, when the RT's name starts with an upper-case character. This resource in question is called Microsoft.Network/p2svpnGateways see this for more details. Hence, the name must be avm/res/network/p2svpn-gateway (1) instead of avm/res/network/p2s-vpn-gateway (2), which would be easier to read. Can you please advise and let me know if we must go with (1) or is it ok to preceed with option (2)? Thanks!

Hey @matebarabas, I can't be 100% sure, but I think you can add as many - as you want to the provider namespace & resource type. The only effect I can think of additional - would have is that they'll be passed through to the name of the module in the registry itself (so not just the folder name). @eriqua would you have anything else in mind?

Hey all, back to the above question about additional dashes for module names.

From a technical standpoint, I'd also expect inserting additional dashes to work. I'd also agree the avm/res/network/p2s-vpn-gateway alternative would improve readability.

However, our specs mention the convention of adding dashes only by API name capital letters as a MUST. The resource Microsoft.Network/trafficmanagerprofiles is even brought up as an example. Ref https://azure.github.io/Azure-Verified-Modules/specs/shared/#bicep-resource-module-naming

Hence, @matebarabas @AlexanderSehr @jtracey93 if we are fine with adding the dash for readability purposes, we should also relax the above spec accordingly.

matebarabas commented 1 week ago

Thanks, @eriqua, for your pov! I'll update the issue and the index, adding the extra dash - for the time being, let's do this as an exception. I recommend testing that everything works and if it does, I'll update the related specification.

@ericscheffler, please proceed with the development accordingly. Thank you!