Azure / bicep-registry-modules

Bicep registry modules
MIT License
472 stars 327 forks source link

[AVM Module Issue]: Azure Firewall Basic deployment failed when Management Public IP is not given #2589

Closed pmalarme closed 2 months ago

pmalarme commented 3 months ago

Check for previous/existing GitHub issues

Issue Type?

Bug

Module Name

avm/res/network/azure-firewall

(Optional) Module Version

0.3.0

Description

Context

When deploying Azure Firewall with Basic SKU Tier and without giving a public IP for Firewall Management, I get the following error:

"details":[{"code":"AzureFirewallManagementIpConfigRequiresSubnetAndPublicIp","message":"AzureFirewall hub-dev-afw management IP configuration requires both a subnet and a public IP address.","details":[]}]

Bicep Template

module firewall 'br/public:avm/res/network/azure-firewall:0.3.0' = {
  name: take('${deployment().name}-firewall', 64)
  scope: resourceGroup
  params: {
    name: firewallName
    location: location
    tags: tags
    enableTelemetry: enableTelemetry
    diagnosticSettings: diagnosticsSettings
    virtualNetworkResourceId: virtualNetwork.outputs.resourceId
    threatIntelMode: 'Deny'
    azureSkuTier: 'Basic'
    networkRuleCollections: []
  }
}

Solution

When the management IP is not set, a default one is created. The user can specified a managementIPAddressObject that is optional. But in the code, in the context above, the IP address will never be set because of this condition: (!empty(managementIPResourceID) || !empty(managementIPAddressObject)). Therefore the deployment of Firewall basic will always fail.

// ----------------------------------------------------------------------------
// Prep managementIPConfiguration object for different uses cases:
// 1. Use existing Management Public IP
// 2. Use new Management Public IP created in this module

var managementIPConfiguration = {
  name: !empty(managementIPResourceID) ? last(split(managementIPResourceID, '/')) : managementIPAddress.outputs.name
  properties: union(
    {
      subnet: {
        id: '${virtualNetworkResourceId}/subnets/AzureFirewallManagementSubnet' // The subnet name must be AzureFirewallManagementSubnet for a 'Basic' SKU tier firewall
      }
    },
    (!empty(managementIPResourceID) || !empty(managementIPAddressObject))
      ? {
          // Use existing Management Public IP, new Management Public IP created in this module, or none if neither
          publicIPAddress: {
            id: !empty(managementIPResourceID) ? managementIPResourceID : managementIPAddress.outputs.resourceId
          }
        }
      : {}
  )
}

This should be:

// ----------------------------------------------------------------------------
// Prep managementIPConfiguration object for different uses cases:
// 1. Use existing Management Public IP
// 2. Use new Management Public IP created in this module

var managementIPConfiguration = {
  name: !empty(managementIPResourceID) ? last(split(managementIPResourceID, '/')) : managementIPAddress.outputs.name
  properties: {
    subnet: {
      id: '${virtualNetworkResourceId}/subnets/AzureFirewallManagementSubnet' // The subnet name must be AzureFirewallManagementSubnet for a 'Basic' SKU tier firewall
    }
    // Use existing Management Public IP, new Management Public IP created in this module, or none if neither
    publicIPAddress: {
      id: !empty(managementIPResourceID) ? managementIPResourceID : managementIPAddress.outputs.resourceId
    }
  }
}

Workaround

Create a public IP for Firewall management and set it explicitely

module firewall 'br/public:avm/res/network/azure-firewall:0.3.0' = {
  name: take('${deployment().name}-firewall', 64)
  scope: resourceGroup
  params: {
    name: firewallName
    location: location
    tags: tags
    enableTelemetry: enableTelemetry
    diagnosticSettings: diagnosticsSettings
    virtualNetworkResourceId: virtualNetwork.outputs.resourceId
    managementIPResourceID: firewallManagementPublicIp.outputs.resourceId
    threatIntelMode: 'Deny'
    azureSkuTier: 'Basic'
    networkRuleCollections: []
  }
}

(Optional) Correlation Id

No response

microsoft-github-policy-service[bot] commented 3 months ago

[!IMPORTANT] The "Needs: Triage :mag:" label must be removed once the triage process is complete!

[!TIP] For additional guidance on how to triage this issue/PR, see the BRM Issue Triage documentation.

avm-team-linter[bot] commented 3 months ago

@pmalarme, thanks for submitting this issue for the avm/res/network/azure-firewall module!

[!IMPORTANT] A member of the @Azure/avm-res-network-azurefirewall-module-owners-bicep or @Azure/avm-res-network-azurefirewall-module-contributors-bicep team will review it soon!

microsoft-github-policy-service[bot] commented 2 months ago

[!WARNING] Tagging the AVM Core Team (@Azure/avm-core-team-technical-bicep) due to a module owner or contributor having not responded to this issue within 3 business days. The AVM Core Team will attempt to contact the module owners/contributors directly.

[!TIP]

  • To prevent further actions to take effect, the "Status: Response Overdue 🚩" label must be removed, once this issue has been responded to.
  • To avoid this rule being (re)triggered, the ""Needs: Triage :mag:" label must be removed as part of the triage process (when the issue is first responded to)!
microsoft-github-policy-service[bot] commented 2 months ago

[!WARNING] Tagging the AVM Core Team (@Azure/avm-core-team-technical-bicep) due to a module owner or contributor having not responded to this issue within 3 business days. The AVM Core Team will attempt to contact the module owners/contributors directly.

[!TIP]

  • To prevent further actions to take effect, the "Status: Response Overdue 🚩" label must be removed, once this issue has been responded to.
  • To avoid this rule being (re)triggered, the ""Needs: Triage :mag:" label must be removed as part of the triage process (when the issue is first responded to)!
microsoft-github-policy-service[bot] commented 2 months ago

[!CAUTION] This issue requires the AVM Core Team's (@Azure/avm-core-team-technical-bicep) immediate attention as it hasn't been responded to within 6 business days.

[!TIP]

  • To avoid this rule being (re)triggered, the "Needs: Triage :mag:" and "Status: Response Overdue :triangular_flag_on_post:" labels must be removed when the issue is first responded to!
  • Remove the "Needs: Immediate Attention :bangbang:" label once the issue has been responded to.
hundredacres commented 2 months ago

Taking a look.

hundredacres commented 2 months ago

Fixed in https://github.com/Azure/bicep-registry-modules/pull/2706