Azure / terraform-azurerm-hubnetworking

Terraform verified module for deploying multi-hub & spoke architectures
https://registry.terraform.io/modules/Azure/hubnetworking/azurerm/latest
MIT License
22 stars 17 forks source link

Support for multiple public IP addresses / public IP prefixes #56

Open rodmhgl opened 1 year ago

rodmhgl commented 1 year ago

Is there an existing issue for this?

Description

I have a need to add multiple public IPs to an Azure Firewall. Is there currently support in the module for multiple public IPs?

Ideally, these would be sourced from a Public IP Prefix.

New or Affected Resource(s)/Data Source(s)

azurerm_public_ip

Potential Terraform Configuration

resource "azurerm_public_ip" "fw_additional_pip" {
  for_each = local.fw_additional_ip_configurations

  allocation_method   = "Static"
  location            = each.value.location
  name                = each.value.name
  resource_group_name = each.value.resource_group_name
  ip_version          = each.value.ip_version
  public_ip_prefix_id = try(var.public_ip_prefix_id, null)
  sku                 = "Standard"
  sku_tier            = each.value.sku_tier
  tags                = {}
  zones               = each.value.zones
}

# Possible add a dynamic block into azurerm_firewall to configure the additional IPs?

References

No response

lonegunmanb commented 1 year ago

Hi @rodmhgl thanks for opening this feature request. I've reviewed the current implementation:

ip_configuration {
    name                 = each.value.default_ip_configuration.name
    public_ip_address_id = azurerm_public_ip.fw_default_ip_configuration_pip[each.key].id
    subnet_id            = azurerm_subnet.fw_subnet[each.key].id
  }

The public_ip_address_id and subnet_id are all exported from the resources created by this module, so my opinion on these additional public ips is, what about we let the module's caller inject the whole block like this?:

variable "hub_virtual_networks" {
  type = map(object({
    name                            = string
    address_space                   = list(string)
    location                        = string
...
  additional_ip_configurations = optional(list(object{
    name = string
    public_ip_address_id = optional(string)
    subnet_id = optional(string)
  }), [])
...
 }))
}

So we can delegate the creation of subnet / public_ip resources to the module's caller to simplify the module's complexity. @rodmhgl @matt-FFFFFF WDYT?

matt-FFFFFF commented 1 year ago

@lonegunmanb

I think we should consider adding g a new map to the firewall block to account for multiple IP addresses.

We keep the default_ip_configuration block but deprecate it. Recommending that users use the new ip_configurations map.

Behind the scenes we move the public ip resource into a for_each loop. If customer uses only default_ip_configuration we can create a map with a single entry.

This way we ensure the module can create all the required resources and do not make any breaking changes.

matt-FFFFFF commented 1 year ago

We can use the moved block internally to handle the change

rodmhgl commented 1 year ago

Hi all, apologies for creating this request and then disappearing. I wasn't getting the notifications for some reason.

I like the idea of delegating the creation of the public IPs to the caller and then passing them in via map; having the module create the PIPs would definitely over-complicate the module.