Option to remove region suffix

[amp-pds]

Describe the feature end to end, including deployment scenario details under which the feature would occur.

The ALZ template currently deploys an Azure firewall and secure virtual hub with the region name as a suffix.

Our organisation's standard naming convention already has the region name included, so for us the region is duplicated in the resource name.

Why is this feature important. Describe why this would be important for your organization and others. Would this impact similar orgs in the same way?

Only a cosmetic change to naming convention.

Please provide the correlation id associated with your error or bug.

xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx

Can you describe any alternatives that you have taken since this feature does not exist?

No response

Feature Implementation

No response

Check previous GitHub issues

• I have searched the issues for this item and found no duplicate

Code of Conduct

• I agree to follow this project's Code of Conduct

[jtracey93]

Great ask and something we will put on our backlog under feature AB#30599

[johnlokerse]

@jtracey93 I would like to pick this one up if it's still open.

I assume it's about the resources in this file?

ALZ-Bicep/infra-as-code/bicep/modules/vwanConnectivity/vwanConnectivity.bicep

Line 320 in e9cfbcb

resource resAzureFirewall 'Microsoft.Network/azureFirewalls@2023-02-01' = [for (hub, i) in parVirtualWanHubs: if ((parVirtualHubEnabled) && (hub.parAzFirewallEnabled)) {

I am going to make the name customizable, so the user can give it their own name. If the user does not supply a name then it will be the default value like it is now. I am also seeing some inconsistencies in the descriptions so I will change this too.

[jtracey93]

Yes please @johnlokerse

[johnlokerse]

I would like to do a braindump, mainly because I am a bit stuck and need another point of view ;-)

In vwanConnectivity.bicep we have parameter parVirtualWanHubs which looks like this:

param parVirtualWanHubs array = [ {
    parVpnGatewayEnabled: true
    parExpressRouteGatewayEnabled: true
    parAzFirewallEnabled: true
    parVirtualHubAddressPrefix: '10.100.0.0/23'
    parHubLocation: parLocation
    parHubRoutingPreference: 'ExpressRoute' //allowed values are 'ASN','VpnGateway','ExpressRoute'.
    parVirtualRouterAutoScaleConfiguration: 2 //minimum capacity should be between 2 to 50
    parVirtualHubRoutingIntentDestinations: []
  }
]

Also, there are three parameters for resource names: parVpnGatewayName, parExpressRouteGatewayName, and parAzFirewallName. These parameters are separate from the parVirtualWanHubs array parameter. This being separate from the name parameters is a problem, because the names for each iteration need to be unique, or else the validation of Bicep breaks. In the current setup, the suffix parHubLocation avoids this problem.

What I am thinking is to move the parVpnGatewayName, parExpressRouteGatewayName, and parAzFirewallName (and probably more parameters) parameters to the parVirtualWanHubs. So each iteration of the objects in the array is complete with all the information it needs to bundle the services to the hub.

So in short; bundle all parameters in parVirtualWanHubs (just like parHubLocation) that are needed for the iteration on each hub.

This has some impact when ALZ-Bicep users are upgrading their codebase. What are your thoughts @jtracey93 and @amp-pds?

[jtracey93]

Hey @johnlokerse,

Thanks for looking into this and I can see the complexity that this array of objects presents.

I think I agree with your suggestion of making this part of the array. It might be an interesting time to look at declaring this complex object as a user defined type If you are up for looking at that as well, that would be amazing.

If not, I think making it part of the array still make sense and we can almost keep the default values of the names. as they are today and we can then provide in. changes guidance for this for those who have more than just a single hub.

Thinking aloud I think you may have also discovered another bug which would not have allowed multiple hubs per region in the current implementation approach. So you may actually fix that as part of this change also 👍

Finally, as just putting it out there, do we think we could implement some logic in the module itself to say using new or old naming schema and therefore? we could actually make this a non breaking change as we would set the default to be the old schema but somebody could this to the new schema and therefore. access the new naming flexibility. Just an idea.

[johnlokerse]

@jtracey93

I think I agree with your suggestion of making this part of the array. It might be an interesting time to look at declaring this complex object as a user defined type If you are up for looking at that as well, that would be amazing.

Yes, I will take a look at user defined types. 👍 It was something I wanted to suggest to do 😄. I will follow the AVM way of writing.

Thinking aloud I think you may have also discovered another bug which would not have allowed multiple hubs per region in the current implementation approach. So you may actually fix that as part of this change also 👍

Finally, as just putting it out there, do we think we could implement some logic in the module itself to say using new or old naming schema and therefore? we could actually make this a non breaking change as we would set the default to be the old schema but somebody could this to the new schema and therefore. access the new naming flexibility. Just an idea.

Yes, having the option to use the new or old naming schema is a great idea. I think it is the only way to make it non breaking. I will be on it, thanks for sharing your thoughts @jtracey93 👍

[johnlokerse]

@jtracey93 I created a pull request and currently testing with Azure deployments. Deploying these resources takes a while and need to update the documentation... ;-)