Virtual network module use of contains function

[maxime-roy]

Hi!

I have a quick question for you, is there a particular reason the contains() function is used in the Microsoft.Network/VirtualNetwork module? With the current implementation, the module doesn't allow us to feed an empty string for a subnet property (like a NSG or a RT id ). This scenario could be useful where we send an array of subnet configuration to the module where not all subnets have a NSG, a route table or a NAT gateway linked to them.

It's something that I could submit a fix for, but I want to know if there was a particular reason it was done this way before working on it :) !

Here's an example for what the module prevents us to do:

module virtualNetworkModule './modules/Microsoft.Network/virtualNetworks/deploy.bicep' = {
  name: 'deploy-virtualnetwork-${uniqueString(resourceGroup().name)}'
  scope: resourceGroup(networkResourceGroupName)
  params: {
    name: virtualNetworkName
    location: networkResourceGroupLocation
    lock: ''
    addressPrefixes: virtualNetworkAddressPrefixes
    subnets: [for subnet in virtualNetworkSubnets: {
      name: subnet.name
      addressPrefix: subnet.addressPrefix
      networkSecurityGroupId: (empty(subnet.networkSecurityGroupName) ?  '' : resourceId(resourceGroup().name, 'Microsoft.Network/networkSecurityGroups' , subnet.networkSecurityGroupName))
      routeTableId: (empty(subnet.routeTableName) ? '' : resourceId(resourceGroup().name, 'Microsoft.Network/routeTables' , subnet.routeTableName))
      serviceEndpoints: subnet.serviceEndpoints
      delegations: subnet.delegations
      privateEndpointNetworkPolicies: subnet.privateEndpointNetworkPolicies
      privateLinkServiceNetworkPolicies: subnet.privateLinkServiceNetworkPolicies
    }]
    tags: virtualNetworkTags
  }
  dependsOn: [
    networkSecurityGroupModule
    routeTableModule
  ]
}

Thank you!

[eriqua]

Hey @maxime-roy, thanks for your question.

contains() allows you to provide a subnets array of objects where only the non-empty values are listed. e.g., similarly to what used by the Vnet common test:

 subnets: [
      {
        addressPrefix: '10.0.255.0/24'
        name: 'GatewaySubnet'
      }
      {
        addressPrefix: '10.0.0.0/24'
        name: '<<namePrefix>>-az-subnet-x-001'
        networkSecurityGroupId: resourceGroupResources.outputs.networkSecurityGroupResourceId
        roleAssignments: [
          {
            principalIds: [
              resourceGroupResources.outputs.managedIdentityPrincipalId
            ]
            roleDefinitionIdOrName: 'Reader'
          }
        ]
        routeTableId: resourceGroupResources.outputs.routeTableResourceId
        serviceEndpoints: [
          {
            service: 'Microsoft.Storage'
          }
          {
            service: 'Microsoft.Sql'
          }
        ]
      }
]

By checking on empty() instead, you would need to specify all the subnet object parameters, e.g.

 subnets: [
      {
        addressPrefix: '10.0.255.0/24'
        name: 'GatewaySubnet'
        networkSecurityGroupId: ''
        roleAssignments: []
        routeTableId: ''
        serviceEndpoints: []
      }
      {
        addressPrefix: '10.0.0.0/24'
        name: '<<namePrefix>>-az-subnet-x-001'
        networkSecurityGroupId: resourceGroupResources.outputs.networkSecurityGroupResourceId
        roleAssignments: [
          {
            principalIds: [
              resourceGroupResources.outputs.managedIdentityPrincipalId
            ]
            roleDefinitionIdOrName: 'Reader'
          }
        ]
        routeTableId: resourceGroupResources.outputs.routeTableResourceId
        serviceEndpoints: [
          {
            service: 'Microsoft.Storage'
          }
          {
            service: 'Microsoft.Sql'
          }
        ]
      }
]

I wouldn't suggest to get rid of the contains() logic, at most I'd suggest to discuss if extending the module check with a combination of contains() and empty() to support both. Something like:

networkSecurityGroup: contains(subnet, 'networkSecurityGroupId') && !empty(subnet.networkSecurityGroupId) ? {
          'id': subnet.networkSecurityGroupId
        } : null

Nevertheless, I'd be interested in understanding your scenario a bit more. Is there anything that prevents you to simply not specify an empty subnet object parameter instead of providing an empty value for it?

[maxime-roy]

Hi @eriqua !

Thank you for your reply. In this scenario, I was thinking about using the combination of contains() and empty() .

The reason why I think it could have some value to manage an empty value is when you want to use loops to create subnets with different configurations. Like if some subnets need to have a NAT gateway and others not and I deploy them using a loop on an array of objects, I have to have the natgatewayid parameter in the loop but it could be empty in the parameter I send to it in the object.

That kind of scenario could be applicable on all modules in the repository, especially while using custom arrays of objects as parameters.

Kind regards,

[coolhome]

I agree with @maxime-roy here, I think a combination is best of both worlds.

I converted my CARML virtual network to use the following. Creating undefined properties from a loop is REALLY hard. A great use case is to pull existing subnets.

resource virtualNetwork 'Microsoft.Network/virtualNetworks@2021-08-01' = {
  name: name
  location: location
  tags: tags
  properties: {
    addressSpace: {
      addressPrefixes: addressPrefixes
    }
    ddosProtectionPlan: !empty(ddosProtectionPlanId) ? ddosProtectionPlan : null
    dhcpOptions: !empty(dnsServers) ? dnsServers_var : null
    enableDdosProtection: !empty(ddosProtectionPlanId)
    subnets: [for subnet in subnets: {
      name: subnet.name
      properties: {
        addressPrefix: subnet.addressPrefix
        addressPrefixes: contains(subnet, 'addressPrefixes') && !empty(subnet.addressPrefixes) ? subnet.addressPrefixes : []
        applicationGatewayIpConfigurations: contains(subnet, 'applicationGatewayIpConfigurations') && !empty(subnet.applicationGatewayIpConfigurations) ? subnet.applicationGatewayIpConfigurations : []
        delegations: contains(subnet, 'delegations') && !empty(subnet.delegations) ? subnet.delegations : []
        ipAllocations: contains(subnet, 'ipAllocations') && !empty(subnet.ipAllocations) ? subnet.ipAllocations : []
        natGateway: contains(subnet, 'natGatewayId') && !empty(subnet.natGatewayId) ? {
          'id': subnet.natGatewayId
        } : null
        networkSecurityGroup: contains(subnet, 'networkSecurityGroupId') && !empty(subnet.networkSecurityGroupId) ? {
          'id': subnet.networkSecurityGroupId
        } : null
        privateEndpointNetworkPolicies: contains(subnet, 'privateEndpointNetworkPolicies') && !empty(subnet.privateEndpointNetworkPolicies) ? subnet.privateEndpointNetworkPolicies : null
        privateLinkServiceNetworkPolicies: contains(subnet, 'privateLinkServiceNetworkPolicies') && !empty(subnet.privateLinkServiceNetworkPolicies) ? subnet.privateLinkServiceNetworkPolicies : null
        routeTable: contains(subnet, 'routeTableId') && !empty(subnet.routeTableId) ? {
          'id': subnet.routeTableId
        } : null
        serviceEndpoints: contains(subnet, 'serviceEndpoints') && !empty(subnet.serviceEndpoints) ? subnet.serviceEndpoints : []
        serviceEndpointPolicies: contains(subnet, 'serviceEndpointPolicies') && !empty(subnet.serviceEndpointPolicies) ? subnet.serviceEndpointPolicies : []
      }
    }]
  }
}

[coolhome]

I did not realize the history behind subnets - Azure/bicep#6755 (comment)

[AlexanderSehr]

yeah - this subnet story is a real treat :D @coolhome, just so that you're aware, the same is true for e.g. route tables & security rules.

Anyhow, to add my 5 cents to the story:
We actually used empty() in some areas in the past - but - removed it. I think the rationale was that it can easily happen that you lock yourself in a state where you cannot set an empty value anymore. For example, removing the assignment of an NSG after the fact by providing an empty string as a value could be ignored. I think to be on the safe side one should probably always have [] or {} as defaults instead of null. However, I have to think about this a bit more / test it out.
Fundamentally I support the idea - we just have to make sure to not accidently break something along the way.

[maxime-roy]

In this situation, I don't mind it being something else than null . It really was "only" for the purpose of the example. As long as the empty/null value to set is documented, we can always adjust to that.

Thanks again for looking into it!

[eriqua]

Thanks @maxime-roy and @coolhome for elaborating.
I created issue #2268 from this discussion to provide more visibility to the request and allow its triage.

Besides the discussion started as a feature request for the Vnet module, for consistency that would make sense to be implemented for all input parameter objects throughout the library.

[maxime-roy]

@eriqua , Thanks for creating the issue ! And yeah, I was providing the example with the VNet module but it's definitely something that would be applicable to the whole library :) !

[AlexanderSehr]

I'd suggest to close this discussion in favor of issue #2268. Otherwise it may become difficult to track the discussion :)