❓👂 Question/Feedback - Add subnets for vNet creation in modules

[oxbo-andre]

Question/Feedback

Subnets not created in VNet? It's not best practice to created afterward lose the subnet, but create the subnet in the VNet with a array and redeploy the Vnet.

Possible Answers/Solutions?

Configure the subnets also in the Vnet at least one.

[DaFitRobsta]

Hello @oxbo-andre,
Could you please elaborate on your use case(s)? I understand organizations have different philosophies on who's allowed to manage the subnets within a virtual network. Our opinionated approach assumes the application team (or whomever manages the subscription/application) would be responsible for defining their subnets within the vnet.
We would like to better understand your use cases where this would be beneficial.

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

[oxbo-andre]

Hello @oxbo-andre, Could you please elaborate on your use case(s)? I understand organizations have different philosophies on who's allowed to manage the subnets within a virtual network. Our opinionated approach assumes the application team (or whomever manages the subscription/application) would be responsible for defining their subnets within the VNet. We would like to better understand your use cases where this would be beneficial.

Hi @DaFitRobsta when you deploy a VNet with code and then deploy later the subnet the VNet will be deleted and deployed with the subnet. But in a existing VNet you don't want this behavior.

So when you deploy resources with IAC you edit your deployment and deploy it and only the changes will be done, no VNet removed and created new.

Or maybe it don't understand it correctly, like to hear when I am wrong.

[DaFitRobsta]

Hello @oxbo-andre,
In the case of updating a VNet via IaC, when "redeploying" or updating properties of a VNet, the VNet is not deleted. Properties that are defined in IaC that differ from the current state will be reconciled.
For subnet declarations, you are correct. Subnets will be overwritten (deleted, created) with what is defined in IaC. This includes NSG, UDR, Service Endpoints, etc. that are tied to each individual subnet.

With this understanding, are you still looking for us to include subnet declarations for the spokeNetworking.bicep module? If so, I will need chat with my team about the implications of adding subnet declarations within the current spokeNewtorking.bicep module.

[johnlokerse]

@oxbo-andre @DaFitRobsta My team also adopted the ALZ-Bicep repository and we modified spokeNetworking.bicep to our liking. Although we mainly want the application teams to decide on their own subnets we added an optional parSpokeSubnet array. The default value of this parameter is empty array, so if an application team wants us to handle their subnetting we can help, if not we leave it empty.

Is this something that can help you @oxbo-andre?

Example:

@description('Specifies the location.')
param location string = 'westeurope'

param parSubnets array = []

resource resSpokeVirtualNetwork 'Microsoft.Network/virtualNetworks@2021-02-01' = {
  name: 'spokeVirtualNetwork'
  location: location
  properties: {
    addressSpace: {
      addressPrefixes: [
        '192.168.1.0/24'
      ]
    }
    subnets: [for subnetProperties in parSubnets: {
      name: subnetProperties.name
      properties: {
        addressPrefix: subnetProperties.addressPrefix
      }
    }]
  }
}

[DaFitRobsta]

@johnlokerse, Thank you for your input and example. There might be times where you want to deploy a blank virtual network or a virtual network with subnets. To address this situation, consider the following modifications:

spokeNetworking.bicep additional parameter and variable:

@description('Subnet declaration')
param parSpokeNetworkSubnets array = []

// If subnets are defined, wrap them into a variable to be evaluated later
var varSpokeNetworkSubnets = [for subnet in parSpokeNetworkSubnets: {
  name: subnet.name
  properties: {
    addressPrefix: subnet.addressPrefix
  }
}]

spokeNetworking.bicep virtual network resource declaration using the Ternary Operator:

resource resSpokeVirtualNetwork 'Microsoft.Network/virtualNetworks@2021-08-01' = {
  name: parSpokeNetworkName
  location: parLocation
  tags: parTags
  properties: {
    addressSpace: {
      addressPrefixes: [
        parSpokeNetworkAddressPrefix
      ]
    }
    subnets: !empty(varSpokeNetworkSubnets) ? varSpokeNetworkSubnets : []
    enableDdosProtection: (!empty(parDdosProtectionPlanId) ? true : false)
    ddosProtectionPlan: (!empty(parDdosProtectionPlanId) ? true : false) ? {
      id: parDdosProtectionPlanId
    } : null
    dhcpOptions: (!empty(parDnsServerIps) ? true : false) ? {
      dnsServers: parDnsServerIps
    } : null
  }
}

spokeNetworking.parameters.all.json additional parameter

"parSpokeNetworkSubnets": {
    "value": [
        {
            "name": "snet-web",
            "addressPrefix": "10.11.0.0/24"
        },
        {
            "name": "snet-app",
            "addressPrefix": "10.11.1.0/24"
        },
        {
            "name": "snet-data",
            "addressPrefix": "10.11.2.0/24"
        }
    ]
}

[jtracey93]

@oxbo-andre @johnlokerse

Do you want to see this implemented for the spoke networking module in ALZ bicep?

The issue we have long battled with subnets, for context, is the long list of properties that may need to be supported and therefore validated and input. Also, the fact you can declare subnets in 2 ways, child resources of vNets or standalone resources; but we'll ignore that for now.

We have also tried to avoid taking too many, if any, complex array/objects in as parameter input values in the ALZ Bicep modules to make them as user friendly as possible. Especially as today there is no intellisense support for complex arrays/objects; however, this is something the Bicep engineering team are working on with "Custom Types".

If we did introduce subnets, this would need to be an ALZ all up discussion across all implementation options. As it not only brings subnets in but NSGs and UDRs at a minimum to satisfy policies that are assigned by default.

Hence why in all ALZ implementations we opted to not create subnets in spokes and instead create blank vNets and leave it at that for simplicity.

Let us know your thoughts

cc: @DaFitRobsta

[oxbo-andre]

Hello @oxbo-andre, In the case of updating a VNet via IaC, when "redeploying" or updating properties of a VNet, the VNet is not deleted. Properties that are defined in IaC that differ from the current state will be reconciled. For subnet declarations, you are correct. Subnets will be overwritten (deleted, created) with what is defined in IaC. This includes NSG, UDR, Service Endpoints, etc. that are tied to each individual subnet.

With this understanding, are you still looking for us to include subnet declarations for the spokeNetworking.bicep module? If so, I will need chat with my team about the implications of adding subnet declarations within the current spokeNewtorking.bicep module.

Yes we would like to see subnets in the Vnet.

@oxbo-andre @johnlokerse

Do you want to see this implemented for the spoke networking module in ALZ bicep?

The issue we have long battled with subnets, for context, is the long list of properties that may need to be supported and therefore validated and input. Also, the fact you can declare subnets in 2 ways, child resources of vNets or standalone resources; but we'll ignore that for now.

We have also tried to avoid taking too many, if any, complex array/objects in as parameter input values in the ALZ Bicep modules to make them as user friendly as possible. Especially as today there is no intellisense support for complex arrays/objects; however, this is something the Bicep engineering team are working on with "Custom Types".

If we did introduce subnets, this would need to be an ALZ all up discussion across all implementation options. As it not only brings subnets in but NSGs and UDRs at a minimum to satisfy policies that are assigned by default.

Hence why in all ALZ implementations we opted to not create subnets in spokes and instead create blank vNets and leave it at that for simplicity.

Let us know your thoughts

cc: @DaFitRobsta

Yes we want to see this implemented in the ALZ bicep.

[johnlokerse]

@jtracey93 @DaFitRobsta

In my opinion, the workload should be in control of subnetting, but I can imagine that organizations want to control the subnetting or use the Bicep module in a different way than its purpose. So, if organizations want to see this implemented, it should be optional and should be framed to:

• subnet name
• addressPrefix
• networkSecurityGroup reference (no creation, just referring to an id) - optional
• routeTable reference (no creation, just referring to an id) - optional

A design choice could be to create a standalone module (just like spokeNetworking.bicep) for subnets to reduce the complex input parameters.

Custom types with IntelliSense would help a-lot!

Do my thoughts make sense?

[jtracey93]

Thanks both @oxbo-andre & @johnlokerse for your replies and inputs.

@johnlokerse your suggestions make perfect sense and thanks for the taking the time to propose something, greatly appreciated. Firmly agree subnet creation is for the application/workload team to best make the decisions on and create 👍

I'm thinking I want to avoid a separate module however as ideally subnets should be defined as children of the vNet resource and not a standalone resource to avoid the long-discussed issue: Azure/azure-quickstart-templates#2786 (which we are discussing with the ARM PG at length currently JFYI)

So if we were to add an parameter to spokeNetworking.bicep and probably worth adding to hubNetworking.bicep also if we do it, that looked like this:

param parSubnets array = [
  {
    name: 'snet1'
    cidr: '10.95.10.0/24'
    nsg: '/subscriptions/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/xxxxxxx/providers/Microsoft.Network/networkSecurityGroups/xxxxxxx'
    udr: '/subscriptions/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/xxxxxxx/providers/Microsoft.Network/routeTables/xxxxxxx'
  }
  {
    name: 'snet2'
    cidr: '10.95.20.0/24'
    nsg: '/subscriptions/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/xxxxxxx/providers/Microsoft.Network/networkSecurityGroups/xxxxxxx'
    udr: ''
  }
  {
    name: 'snet3'
    cidr: '10.95.30.0/24'
    nsg: ''
    udr: ''
  }
]

Would this work?

Questions

@oxbo-andre & @johnlokerse please share your insights on these 👍

1. Does the above schema work?
2. Logic wise, if no NSG or UDR resource IDs are provided we would try to create the subnet with no NSG or UDR associated. is this what you'd expect/want?
   1. Would you instead prefer if you specified say just a name in the nsg or udr keys then we create a blank NSG/UDR with that name in the same Resource Group as the vNet?
      • This clearly adds a lot of complexity and also then the door opens to how do we configure NSG/UDR rules and routes respectively. So would be interested if you just think leaving the logic as we dont create NSG/UDRs at all and you only provide an existing is the best approach and you both agree?
3. By default this parameter would be an empty array [] and therefore would create no subnets at all, is that okay?
4. Would you want/need the intellisense support to be available in Bicep before we implemented this, to avoid the complexity of defining the array of objects alone in your IDE?
5. Anything else we've missed?

ALZ Team Stuff

@DaFitRobsta, @krowlandson, @matt-FFFFFF, @jfaurskov, @mblant - JFYI. No decision been made here but just interested in getting the thoughts captured in this issue from everyone to help us make a call in the near future.

@krowlandson are there any similar issues on ALZ Terraform for this ask that we can link up to this issue?

[krowlandson]

@jtracey93 I don't think we have any open issues relating to this but the following is what we've done on the Terraform side:

• Virtual networks and subnets are created using the dedicated resources in Terraform, namely:
  • azurerm_virtual_network
  • azurerm_subnet
• This ensures that we are able to create subnets within the module (e.g. GatewaySubnet and AzureFirewallSubnet) whilst still allowing application support teams / Subscription owners to create their own subnets without any implication for the platform team.
• As Terraform tracks state outside of Azure, it will only try to make changes to the Virtual Network if absolutely needed. This largely mitigates any of the platform issues we face when a Virtual Network deployment fails due to a mismatch between the virtual network definition and the deployed subnets.
• However, we can still run into issues when a change is legitimately made to the Virtual Network which requires it to be updated. As such, operational updates to Virtual Networks (whilst rare) can still be problematic but I believe this may be more related to a provider bug, as we've managed to work around this with the patch method when using the azapi_update_resource resource.

@matt-FFFFFF did some testing on this and can probably provide more detail over which updates worked, and which caused issues when subnets were not declared as part of the request.

[krowlandson]

Also, this is the schema we use for custom subnets managed by the Terraform module:

            subnets = list(
              object({
                name                      = string
                address_prefixes          = list(string)
                network_security_group_id = string
                route_table_id            = string
              })
            )

https://github.com/Azure/terraform-azurerm-caf-enterprise-scale/blob/6879e9b9a74722db431d1cfed0ee30fefcdd1e6c/variables.tf#L236-L243

[jtracey93]

Thanks @krowlandson, this is only for the hub vNet's though right? Not spokes as we dont support them in the TF module today?

Do you also follow the same logic of the NSG and UDR needs to be pre-existing? And you wont create them if they dont?

[krowlandson]

Correct @jtracey93, but a virtual network is a virtual network so still relevant 😄

And yes, assumption is that the NSG/UDR pre-exist and are just being attached.

Spoke networks are part of the lz-vending module, which is where we use the azapi_update_resource resource.

[aavdberg]

Thanks both @oxbo-andre & @johnlokerse for your replies and inputs.

@johnlokerse your suggestions make perfect sense and thanks for the taking the time to propose something, greatly appreciated. Firmly agree subnet creation is for the application/workload team to best make the decisions on and create 👍

I'm thinking I want to avoid a separate module however as ideally subnets should be defined as children of the vNet resource and not a standalone resource to avoid the long-discussed issue: Azure/azure-quickstart-templates#2786 (which we are discussing with the ARM PG at length currently JFYI)

So if we were to add an parameter to spokeNetworking.bicep and probably worth adding to hubNetworking.bicep also if we do it, that looked like this:

param parSubnets array = [
  {
    name: 'snet1'
    cidr: '10.95.10.0/24'
    nsg: '/subscriptions/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/xxxxxxx/providers/Microsoft.Network/networkSecurityGroups/xxxxxxx'
    udr: '/subscriptions/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/xxxxxxx/providers/Microsoft.Network/routeTables/xxxxxxx'
  }
  {
    name: 'snet2'
    cidr: '10.95.20.0/24'
    nsg: '/subscriptions/xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx/resourceGroups/xxxxxxx/providers/Microsoft.Network/networkSecurityGroups/xxxxxxx'
    udr: ''
  }
  {
    name: 'snet3'
    cidr: '10.95.30.0/24'
    nsg: ''
    udr: ''
  }
]

Would this work?

Questions

@oxbo-andre & @johnlokerse please share your insights on these 👍

1. Does the above schema work?

Yes

2. Logic wise, if no NSG or UDR resource IDs are provided we would try to create the subnet with no NSG or UDR associated. is this what you'd expect/want?

Yes

1. Would you instead prefer if you specified say just a name in the nsg or udr keys then we create a blank NSG/UDR with that name in the same Resource Group as the vNet?

No

  * This clearly adds a lot of complexity and also then the door opens to how do we configure NSG/UDR rules and routes respectively. So would be interested if you just think leaving the logic as we dont create NSG/UDRs at all and you only provide an existing is the best approach and you both agree?

Agree

3. By default this parameter would be an empty array [] and therefore would create no subnets at all, is that okay?
   Yes
4. Would you want/need the intellisense support to be available in Bicep before we implemented this, to avoid the complexity of defining the array of objects alone in your IDE?
   Yes
5. Anything else we've missed?

ALZ Team Stuff

@DaFitRobsta, @krowlandson, @matt-FFFFFF, @jfaurskov, @mblant - JFYI. No decision been made here but just interested in getting the thoughts captured in this issue from everyone to help us make a call in the near future.

@krowlandson are there any similar issues on ALZ Terraform for this ask that we can link up to this issue?

[johnlokerse]

@jtracey93

1. Does the above schema work?

Yes 😄

2. Logic wise, if no NSG or UDR resource IDs are provided we would try to create the subnet with no NSG or UDR associated. is this what you'd expect/want?

Yes, make it optional

1. Would you instead prefer if you specified say just a name in the nsg or udr keys then we create a blank NSG/UDR with that name in the same Resource Group as the vNet?
   • This clearly adds a lot of complexity and also then the door opens to how do we configure NSG/UDR rules and routes respectively. So would be interested if you just think leaving the logic as we dont create NSG/UDRs at all and you only provide an existing is the best approach and you both agree?

Let it be optional, because of the complex parameters. Provide an existing NSG or UDR by resourceId.

3. By default this parameter would be an empty array [] and therefore would create no subnets at all, is that okay?

Yes

5. Would you want/need the intellisense support to be available in Bicep before we implemented this, to avoid the complexity of defining the array of objects alone in your IDE?

In my opinion we should wait for intellisense if the implementation becomes "big and complex". If its only name, cidr, udr and nsg it is fine to implemented the way you featured.

7. Anything else we've missed?

No, I like the solution to make it optional 😄

[jtracey93]

Thanks @aavdberg for your answers.

And also to @johnlokerse however, I do have a clarifying question for you, if that's okay?

• To clarify, you do not want us to create a UDR or NSG if an existing one isn't provided? (crosses fingers as if so we can likely add with the Bicep current feature set and improve once Complex Types exist 🤞)

Let us know

[DaFitRobsta]

Hello @johnlokerse and @oxbo-andre,
During our ALZ Bicep team scrum call today, we discussed your request of adding virtual network subnet declarations within the spokeNetworking.bicep module. At this time, we will not be updating the module to support this feature request. The reasons include, but not limited to:

• Complexity of managing all of the subnet properties, including NSG, UDR, service endpoints, subnet delegations, etc.
• Intellisense will be a challenge as we'll be using an array of objects to define the subnet properties
• Feature parity with ALZ Azure Portal and Terraform experience
• Future updates to the virtual network resource provider could change the way we manage subnets as a child and/or a separate resource object. All to say this future change could make it easier to manage a subnet declaration within this module

Currently, the ALZ Bicep modules provides a base skeleton/scaffolding to deploy workloads into. To further customize the workload spoke network design, we highly encourage you to use the following ordered methods:

1. CARML - Utilize this mature Bicep repo for resource deployments
2. Fork this repo and customize the modules accordingly
3. Write your own custom modules

cc: @jtracey93

[joshuadmatthews]

The issue is that once I define subnets in my spoke workload templates, I can no longer deploy spokeNetworking.bicep as it doesn't know about the subnets. The change we need is a method of deploying the vnet without including the subnets and Azure should ignore the subnets that are already there instead of trying to delete them. I feel there is not clarity in this thread regarding the true nature of the issue

"I'm thinking I want to avoid a separate module however as ideally subnets should be defined as children of the vNet resource and not a standalone resource"

This is backwards. Ideally subnets should be standalone resources, NOT children properties of vNets. Seeing as this is a breaking change though, it would be acceptable to add a flag to instruct the API to ignore subnets when deploying a vNet that has them defined in Azure but not in the template being deployed.

I would recommend reading Azure/azure-quickstart-templates#2786 thoroughly to understand the issue. We have been waiting 6+ years for a fix to this, and this thread is not inspiring confidence that it will be fixed in a satisfactory manner. Managing all of the properties of subnets in spokeNetworking.bicep is not at all what we want @DaFitRobsta

[jtracey93]

Hey @joshuadmatthews,

Thanks for your comments and understand your frustrations, i join your with them. As you can see in Azure/azure-quickstart-templates#2786 this has been ongoing for sometime and is not something the ALZ-Bicep team are responsible for (the focus of this issue).

I'd really welcome you to add your above comment to that long issue chain above so we keep the conversation in the correct place, as this is owned by the ARM Product Group 👍

However, we are discussing the above-mentioned issue with the ARM Product Group & Networking Product Groups to try to make traction on it not deleting subnets not defined.

Back to this issue for ALZ-Bicep

We discussed last night, as per @DaFitRobsta comment, in the ALZ-Bicep core team and decided not to add subnet support due to the complexity of the resulting array of objects and losing intellisense support. Plus, we would need to do it in ALZ Portal Accelerator & Terraform and we cannot in Terraform either due to the requirement of provider block declarations for spoke subscriptions and these cannot be dynamically calculated or set due to the way terraform works today.

Therefore, we are suggesting updating our documentation on the spokeNetworking.bicep file to say if you want to define more than just a blank vNet and hand it over to the app/workload team to create their subnets etc., then you should consider using the CARML vNets module which handles all properties or create your own vNet module for your requirements based of the API spec (https://aka.ms/armtemplatereference).

Hopefully that all makes sense and you see where we are coming from in ALZ-Bicep terms.

Happy to discuss further if you wish 👍

[joshuadmatthews]

Thanks for quick response. I do agree this isn't an issue for ALZ-Bicep team to handle, just pointing out that if the issue from the other thread could get a proper fix then ALZ-Bicep wouldn't have to do anything. It would be clear that ALZ-Bicep defines the vnet and the peering, and spoke templates define the subnets. Two birds with one stone.

[jtracey93]

Thanks for quick response. I do agree this isn't an issue for ALZ-Bicep team to handle, just pointing out that if the issue from the other thread could get a proper fix then ALZ-Bicep wouldn't have to do anything. It would be clear that ALZ-Bicep defines the vnet and the peering, and spoke templates define the subnets. Two birds with one stone.

Completely agree and we are pushing for change fear not 😎

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.

[jtracey93]

Re-opening.

@DaFitRobsta can we get the spokeNetownring read me updated with some links to alternative modules if subnets required like CARML and details on as to why we do not include them in ALZ RIs

[ghost]

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.