Azure / terraform-azurerm-avm-ptn-virtualwan

MIT License
5 stars 11 forks source link

Bug: Creating azurerm_virtual_hub_connection resources using the module results in type errors #6

Closed OmnipotentOwl closed 7 months ago

OmnipotentOwl commented 11 months ago

Summary

When attempting to provision virtual hub connections to virtual networks the following exception occurs because the types used are inconsistent between the results given by the boolean evaluation.

╷
│ Error: Inconsistent conditional result types
│
│   on .terraform\modules\connectivity_vwan.avm_ptn_virtualwan\network.tf line 11, in resource "azurerm_virtual_hub_connection" "hub_connection":
│   11:     for_each = each.value.routing != null && length(each.value.routing) > 0 ? each.value.routing : []
│     ├────────────────
│     │ each.value.routing is null
│
│ The true and false result expressions must have consistent types. The 'true' value is object, but the 'false' value is tuple.

Reproduction

Create a hub connection without specifying the routing block.

Remediation

Below I have proposed a solution that fixes the issue with the routing block. Reviewing the azurerm provider internals it looks like the routing block can be specified once, and the propagated_route_table block can be specified once, but the static_vnet_route block doesn't indicate that it has a restriction from the provider side on how many times it can be specified.

resource "azurerm_virtual_hub_connection" "hub_connection" {
  for_each = local.virtual_network_connections != null && length(local.virtual_network_connections) > 0 ? local.virtual_network_connections : {}

  name                      = each.value.name
  virtual_hub_id            = azurerm_virtual_hub.virtual_hub[each.value.virtual_hub_name].id
  remote_virtual_network_id = each.value.remote_virtual_network_id
  internet_security_enabled = try(each.value.internet_security_enabled, null)
  dynamic "routing" {
    for_each = each.value.routing != null ? [each.value.routing] : [] <-- Change to this for objects

    content {
      associated_route_table_id = try(routing.value.associated_route_table_id, null)

      dynamic "propagated_route_table" {
        for_each = routing.value.propagated_route_table != null ? [routing.value.propagated_route_table] : [] <-- Change to this for objects

        content {
          route_table_ids = try(propagated_route_tables.value.route_table_ids, [])
          labels          = try(propagated_route_tables.value.labels, [])
        }
      }

      dynamic "static_vnet_route" {
        for_each = routing.value.static_vnet_route != null ? routing.value.static_vnet_route : {} <-- this can be more then one item per the provider but for your object this also works.

        content {
          name                = try(static_vnet_route.value.name, null)
          address_prefixes    = try(static_vnet_route.value.address_prefixes, [])
          next_hop_ip_address = try(static_vnet_route.value.next_hop_ip_address, null)
        }
      }
    }
  }
}
khushal08 commented 9 months ago

@OmnipotentOwl Thanks for the feedback. Started looking.

khushal08 commented 9 months ago

Hi @OmnipotentOwl, I can see the changes in your forked repo. Do you want to create a pull request? Have you tested these changes?

OmnipotentOwl commented 9 months ago

Let me see if I can bring the changes from that branch into a PR. The branch is actively used in production deployments so its fully working for the areas being used (vWAN, VPN Site-to-Site Gateways, Azure Firewalls, Firewall Policies, VPN Sites, VPN Site-to-Site Connections, Virtual Network Peering, Routing intents). My current design fits the module into a larger module for building out an Azure Platform Landing Zone and uses some of the other avm modules as well.

khushal08 commented 7 months ago

Waiting on PR - https://github.com/Azure/terraform-azurerm-avm-ptn-virtualwan/pull/17

khushal08 commented 7 months ago

This issue is fixed and code changes have been pushed to the main.