Support peering existing VNets

[gbeaud]

Describe the solution you'd like

• Deploying the module on existing VNets overwrites existing subnets, which is not idempotent despite ARM's theoretical behavior
• This module can only be used once per VNet, as the next time it is run, it will overwrite subnets. This is a limitation since we may like to deploy it again to apply modifications later (e.g. changing the range). The way it is built only supports a one-off deployment for new VNets and then forces users to use another method for further modifications and management.
• I wish I could use this module to capture and manage my entire network:

1. A parameter file would have a list/dictionary to capture all Vnets with their name, range, some settings etc...
2. The code would iterate over the Vnets, and if the Vnet already exists, it doesn't recreate the VNet (which would overwrite the subnets), but simply applies the settings and peering to hub
3. If the VNet described doesn't exist, then it creates it like it's already doing today

Describe alternatives you've considered

• I wish I could code this logic myself in a main.bicep that is calling your modules but the problem is that your module overwrites subnets in any case so we need to modify the module to make it work as I can't just modify this behavior from a main.bicep
• I'm happy to propose modifications and make a pull request to implement my suggestions but let me know beforehand if you validate the approach
• Key challenge: how to test if a resource exists or not in Bicep? (We don't want the user to give a bool parameter for existence)

Additional context

[jtracey93]

Hey @gbeaud,

Thanks for the issue.

Unfortunately this is actually not something we can fix easily with the module, and also suffer from this in the bicep-lz-vending module as well where we have documented this more here: https://github.com/azure/bicep-lz-vending/wiki/knownissues#blank-or-empty-subnets-are-removed-when-re-deploying-this-module-on-a-subscription-and-associated-virtual-network-that-it-previously-provisioned

The root issue tracking this behaviour is https://github.com/Azure/azure-quickstart-templates/issues/2786 and I highly advise to add your input there also to help amplify this.

The approach you mention around checking if the VNet exists, and if it does, then dont overwrite it, is not possible in Bicep as if you do a lookup on an existing resource and its not found, the deployment will fail.

We have considered introducing a deployment script to help here and effectively do a GET on the VNet first, if it exists, and then if it does union the VNet with the desired config in the bicep module and therefore it shouldn't overwrite. But this will make this modules deployment time, very slow, as a deployment script on average takes 5/10 minutes and also requires some Managed Identities and RBAC work also - so not super clean.

Let me know your thoughts here on what you'd like to see, with whats technically possible today 👍

Thanks

Jack

[gbeaud]

Thanks @jtracey93 for the quick feedback! I posted a comment in the issue you shared to emphasize its importance 👍

Well my best guess, given the limitations, would be to add a "newVNet" bool parameter, which the user would set to true when deploying a new VNet and to false for further management. In the module we would check for this parameter: if true it creates the VNet, if false it uses a "existing" resource type to perform modifications like peering etc...

Limitations:

• Not dynamic (doesn't update automatically)
• Puts the burden on the user
• We lose some functionality by using an existing resource, right? (I guess we can still do the peering but not change the range and other settings, right?)

On the other hand it would be a one-off setting so not very cumbersome for the user: each new deployment starts with a "true" and then ongoing operations continue with "false" - just one parameter to change once.

Let me know your thought 😊 Cheers Guillaume

[jtracey93]

Thanks @gbeaud for coming back here.

What about the idea of a deployment script that takes away the need for a new param as we can handle the logic there?

Let me know

[gbeaud]

@jtracey93 Yes that sounds like a good approach 😊👍

[csaba-almasi]

Thanks @gbeaud for coming back here.

What about the idea of a deployment script that takes away the need for a new param as we can handle the logic there?

Let me know

This would be very welcome for our setup as well, currently implementing the workaround using this approach

[jtracey93]

We are likely to hold off on this for a few months as this may be on the road to being fixed properly in the network RP.

[csaba-almasi]

We are likely to hold off on this for a few months as this may be on the road to being fixed properly in the network RP.

No offense @jtracey93 since it's not this team's responsibility but that issue has been known for almost 7 years now and still no resolution to it.

[jtracey93]

I hear you, and no offense taken. It's hurt me too for that long.

But I have faith as have been pushing for this internally and its looking positive

[JimmyKarlsson112]

Any updates on this from Network RP team?

[jtracey93]

Checkout https://github.com/Azure/azure-quickstart-templates/issues/2786 for the latest. It close