fortinetdev / terraform-provider-fortios

Terraform Fortios provider
https://www.terraform.io/docs/providers/fortios/
Mozilla Public License 2.0
67 stars 49 forks source link

Managing address and address group resources - order of operations #186

Closed zephyia closed 1 year ago

zephyia commented 2 years ago

I am attempting to use this module to manage address and addres group objects and am running into an issue with the order of operations.

I have reproduced a simple test case illustrating the issue below:

$ cat main.tf 

terraform {
  required_providers {
    fortios = {
      source = "fortinetdev/fortios"
    }
  }
}

provider "fortios" {
  hostname = "10.80.16.253"
  token    = "<<REDACTED>>"
  insecure = "true"
  vdom     = "root"
}

locals {
  addresses = {
    TF_TEST_ADDRESS_01 = "10.1.0.0/16"
    TF_TEST_ADDRESS_02 = "10.2.0.0/16"
    TF_TEST_ADDRESS_03 = "10.3.0.0/16"
    TF_TEST_ADDRESS_04 = "10.4.0.0/16"
  }
}

resource "fortios_firewall_address" "tf_addresses" {

  for_each = local.addresses

  name          = each.key
  subnet        = each.value
  type          = "ipmask"
  comment       = "Managed by terraform"
  allow_routing = "disable"
  color         = 3
}

resource "fortios_firewall_addrgrp" "tf_address_grp" {
  name          = "TF_TEST_ADDRESS_GRP"
  allow_routing = "disable"
  color         = 3
  exclude       = "disable"

  dynamic "member" {
    for_each = fortios_firewall_address.tf_addresses

    content {
      name = member.value.name
    }
  }

}
$ terraform1 -version
Terraform v1.0.8
on linux_amd64
+ provider registry.terraform.io/fortinetdev/fortios v1.13.2
$ terraform1 apply

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_01"] will be created
  + resource "fortios_firewall_address" "tf_addresses" {
      + allow_routing         = "disable"
      + associated_interface  = (known after apply)
      + cache_ttl             = (known after apply)
      + clearpass_spt         = (known after apply)
      + color                 = 3
      + comment               = "Managed by terraform"
      + country               = (known after apply)
      + dynamic_sort_subtable = "false"
      + end_ip                = (known after apply)
      + end_mac               = (known after apply)
      + epg_name              = (known after apply)
      + fqdn                  = (known after apply)
      + id                    = (known after apply)
      + interface             = (known after apply)
      + name                  = "TF_TEST_ADDRESS_01"
      + node_ip_only          = (known after apply)
      + obj_tag               = (known after apply)
      + obj_type              = (known after apply)
      + organization          = (known after apply)
      + policy_group          = (known after apply)
      + sdn                   = (known after apply)
      + sdn_addr_type         = (known after apply)
      + sdn_tag               = (known after apply)
      + start_ip              = (known after apply)
      + start_mac             = (known after apply)
      + sub_type              = (known after apply)
      + subnet                = "10.1.0.0/16"
      + subnet_name           = (known after apply)
      + tenant                = (known after apply)
      + type                  = "ipmask"
      + uuid                  = (known after apply)
      + visibility            = (known after apply)
      + wildcard              = (known after apply)
      + wildcard_fqdn         = (known after apply)
    }

  # fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_02"] will be created
  + resource "fortios_firewall_address" "tf_addresses" {
      + allow_routing         = "disable"
      + associated_interface  = (known after apply)
      + cache_ttl             = (known after apply)
      + clearpass_spt         = (known after apply)
      + color                 = 3
      + comment               = "Managed by terraform"
      + country               = (known after apply)
      + dynamic_sort_subtable = "false"
      + end_ip                = (known after apply)
      + end_mac               = (known after apply)
      + epg_name              = (known after apply)
      + fqdn                  = (known after apply)
      + id                    = (known after apply)
      + interface             = (known after apply)
      + name                  = "TF_TEST_ADDRESS_02"
      + node_ip_only          = (known after apply)
      + obj_tag               = (known after apply)
      + obj_type              = (known after apply)
      + organization          = (known after apply)
      + policy_group          = (known after apply)
      + sdn                   = (known after apply)
      + sdn_addr_type         = (known after apply)
      + sdn_tag               = (known after apply)
      + start_ip              = (known after apply)
      + start_mac             = (known after apply)
      + sub_type              = (known after apply)
      + subnet                = "10.2.0.0/16"
      + subnet_name           = (known after apply)
      + tenant                = (known after apply)
      + type                  = "ipmask"
      + uuid                  = (known after apply)
      + visibility            = (known after apply)
      + wildcard              = (known after apply)
      + wildcard_fqdn         = (known after apply)
    }

  # fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_03"] will be created
  + resource "fortios_firewall_address" "tf_addresses" {
      + allow_routing         = "disable"
      + associated_interface  = (known after apply)
      + cache_ttl             = (known after apply)
      + clearpass_spt         = (known after apply)
      + color                 = 3
      + comment               = "Managed by terraform"
      + country               = (known after apply)
      + dynamic_sort_subtable = "false"
      + end_ip                = (known after apply)
      + end_mac               = (known after apply)
      + epg_name              = (known after apply)
      + fqdn                  = (known after apply)
      + id                    = (known after apply)
      + interface             = (known after apply)
      + name                  = "TF_TEST_ADDRESS_03"
      + node_ip_only          = (known after apply)
      + obj_tag               = (known after apply)
      + obj_type              = (known after apply)
      + organization          = (known after apply)
      + policy_group          = (known after apply)
      + sdn                   = (known after apply)
      + sdn_addr_type         = (known after apply)
      + sdn_tag               = (known after apply)
      + start_ip              = (known after apply)
      + start_mac             = (known after apply)
      + sub_type              = (known after apply)
      + subnet                = "10.3.0.0/16"
      + subnet_name           = (known after apply)
      + tenant                = (known after apply)
      + type                  = "ipmask"
      + uuid                  = (known after apply)
      + visibility            = (known after apply)
      + wildcard              = (known after apply)
      + wildcard_fqdn         = (known after apply)
    }

  # fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_04"] will be created
  + resource "fortios_firewall_address" "tf_addresses" {
      + allow_routing         = "disable"
      + associated_interface  = (known after apply)
      + cache_ttl             = (known after apply)
      + clearpass_spt         = (known after apply)
      + color                 = 3
      + comment               = "Managed by terraform"
      + country               = (known after apply)
      + dynamic_sort_subtable = "false"
      + end_ip                = (known after apply)
      + end_mac               = (known after apply)
      + epg_name              = (known after apply)
      + fqdn                  = (known after apply)
      + id                    = (known after apply)
      + interface             = (known after apply)
      + name                  = "TF_TEST_ADDRESS_04"
      + node_ip_only          = (known after apply)
      + obj_tag               = (known after apply)
      + obj_type              = (known after apply)
      + organization          = (known after apply)
      + policy_group          = (known after apply)
      + sdn                   = (known after apply)
      + sdn_addr_type         = (known after apply)
      + sdn_tag               = (known after apply)
      + start_ip              = (known after apply)
      + start_mac             = (known after apply)
      + sub_type              = (known after apply)
      + subnet                = "10.4.0.0/16"
      + subnet_name           = (known after apply)
      + tenant                = (known after apply)
      + type                  = "ipmask"
      + uuid                  = (known after apply)
      + visibility            = (known after apply)
      + wildcard              = (known after apply)
      + wildcard_fqdn         = (known after apply)
    }

  # fortios_firewall_addrgrp.tf_address_grp will be created
  + resource "fortios_firewall_addrgrp" "tf_address_grp" {
      + allow_routing         = "disable"
      + color                 = 3
      + dynamic_sort_subtable = "false"
      + exclude               = "disable"
      + id                    = (known after apply)
      + name                  = "TF_TEST_ADDRESS_GRP"
      + type                  = (known after apply)
      + uuid                  = (known after apply)
      + visibility            = (known after apply)

      + member {
          + name = "TF_TEST_ADDRESS_01"
        }
      + member {
          + name = "TF_TEST_ADDRESS_02"
        }
      + member {
          + name = "TF_TEST_ADDRESS_03"
        }
      + member {
          + name = "TF_TEST_ADDRESS_04"
        }
    }

Plan: 5 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_01"]: Creating...
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_04"]: Creating...
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_02"]: Creating...
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_03"]: Creating...
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_01"]: Creation complete after 1s [id=TF_TEST_ADDRESS_01]
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_04"]: Creation complete after 1s [id=TF_TEST_ADDRESS_04]
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_02"]: Creation complete after 1s [id=TF_TEST_ADDRESS_02]
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_03"]: Creation complete after 1s [id=TF_TEST_ADDRESS_03]
fortios_firewall_addrgrp.tf_address_grp: Creating...
fortios_firewall_addrgrp.tf_address_grp: Creation complete after 0s [id=TF_TEST_ADDRESS_GRP]

Apply complete! Resources: 5 added, 0 changed, 0 destroyed.

Applying this terraform will work without issue as terraform will create the address objects and then the address group object. However the issue comes when one of the addresses needs to be removed. We can simulate this by commenting out one of the addressed as below:

$ cat main.tf

terraform {
  required_providers {
    fortios = {
      source = "fortinetdev/fortios"
    }
  }
}

provider "fortios" {
  hostname = "10.80.16.253"
  token    = "<<REDCATED>>"
  insecure = "true"
  vdom     = "root"
}

locals {
  addresses = {
    TF_TEST_ADDRESS_01 = "10.1.0.0/16"
    TF_TEST_ADDRESS_02 = "10.2.0.0/16"
#    TF_TEST_ADDRESS_03 = "10.3.0.0/16"
    TF_TEST_ADDRESS_04 = "10.4.0.0/16"
  }
}

resource "fortios_firewall_address" "tf_addresses" {

  for_each = local.addresses

  name          = each.key
  subnet        = each.value
  type          = "ipmask"
  comment       = "Managed by terraform"
  allow_routing = "disable"
  color         = 3
}

resource "fortios_firewall_addrgrp" "tf_address_grp" {
  name          = "TF_TEST_ADDRESS_GRP"
  allow_routing = "disable"
  color         = 3
  exclude       = "disable"

  dynamic "member" {
    for_each = fortios_firewall_address.tf_addresses

    content {
      name = member.value.name
    }
  }

}

When this runs you get the following error:

$ terraform1 apply
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_03"]: Refreshing state... [id=TF_TEST_ADDRESS_03]
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_04"]: Refreshing state... [id=TF_TEST_ADDRESS_04]
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_01"]: Refreshing state... [id=TF_TEST_ADDRESS_01]
fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_02"]: Refreshing state... [id=TF_TEST_ADDRESS_02]
fortios_firewall_addrgrp.tf_address_grp: Refreshing state... [id=TF_TEST_ADDRESS_GRP]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place
  - destroy

Terraform will perform the following actions:

  # fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_03"] will be destroyed
  - resource "fortios_firewall_address" "tf_addresses" {
      - allow_routing         = "disable" -> null
      - cache_ttl             = 0 -> null
      - clearpass_spt         = "unknown" -> null
      - color                 = 3 -> null
      - comment               = "Managed by terraform" -> null
      - dynamic_sort_subtable = "false" -> null
      - end_mac               = "00:00:00:00:00:00" -> null
      - id                    = "TF_TEST_ADDRESS_03" -> null
      - name                  = "TF_TEST_ADDRESS_03" -> null
      - obj_type              = "ip" -> null
      - sdn_addr_type         = "private" -> null
      - start_mac             = "00:00:00:00:00:00" -> null
      - sub_type              = "sdn" -> null
      - subnet                = "10.3.0.0/16" -> null
      - type                  = "ipmask" -> null
      - uuid                  = "bd22f5a4-2577-51ec-35eb-8aaf61975415" -> null
    }

  # fortios_firewall_addrgrp.tf_address_grp will be updated in-place
  ~ resource "fortios_firewall_addrgrp" "tf_address_grp" {
        id                    = "TF_TEST_ADDRESS_GRP"
        name                  = "TF_TEST_ADDRESS_GRP"
        # (6 unchanged attributes hidden)

      ~ member {
          ~ name = "TF_TEST_ADDRESS_03" -> "TF_TEST_ADDRESS_04"
        }
      - member {
          - name = "TF_TEST_ADDRESS_04" -> null
        }
        # (2 unchanged blocks hidden)
    }

Plan: 0 to add, 1 to change, 1 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

fortios_firewall_address.tf_addresses["TF_TEST_ADDRESS_03"]: Destroying... [id=TF_TEST_ADDRESS_03]
╷
│ Error: Error deleting FirewallAddress resource: Internal Server Error - Internal error when processing the request (500)
│
│
╵

This is because terraform is trying to delete the address object, prior to updating the address group to remove the reference to the address and the fortigate wont let you delete an address object that is referenced in policies, address groups etc. No combination of depends_on or lifecycle rules seems to be able to help here. The order of events for destroy needs to be different to create. I havent tested but imagine a similar issue exists if the address objects are refenced by a policy and you try to remove them.

When you are creating the objects the provider needs to create the referenced address objects first and then create the address group object - which it does without issue. But on removing one address object the provider needs to update the address group first then remove the address object. I am not 100% familar with the internals of terraform providers but I am hoping the provider has some control over the order of operations and is able to be updated to deal with this case?

Otherwise can you suggest other ways to make this work - a multipass method requiring multiple runs where the changes are made incrementally will not work in this case as the source of the IP addresses is programatic and this all part of an automated pipeline.

lix-fortinet commented 2 years ago

Hi @zephyia ,

Thank you for raising this issue, and sorry for the late response. This issue is a little complicated. Team is working on this issue. We will reply to you ASAP.

Thanks, Xing

lix-fortinet commented 2 years ago

Hi @zephyia ,

Terraform has a meta-argument named lifecycle. Set create_before_destroy of lifecycle to true in fortios_firewall_address could solve this issue(Reference: https://www.terraform.io/docs/language/meta-arguments/lifecycle.html). As you said, the reason for this issue is that fortios_firewall_address could not be deleted if it is referenced by other resources. Set create_before_destroy to true will destroy fortios_firewall_address after referenced resources been updated. Could you add lifecycle to fortios_firewall_address and try it again, for example:

resource "fortios_firewall_address" "tf_addresses" {

  for_each = local.addresses

  name          = each.key
  subnet        = each.value
  type          = "ipmask"
  comment       = "Managed by terraform"
  allow_routing = "disable"
  color         = 3
  lifecycle {
    create_before_destroy = true
  }
}

Please let me know if you have any questions.

Thanks, Xing

doumhfr commented 2 years ago

Hi,

we have the same problem

You create x addresses, member of a group. when you want to destroy them (group and addresses), it doesn't work, because the provider try to delete first the addresses, and then the group.

I'm not sure the workaround will work.

By default, when Terraform must change a resource argument that cannot be updated in-place due to remote API limitations, Terraform will instead destroy the existing object and then create a new replacement object with the new configured arguments.

we don"t have problem regarding recreating something, we have a problem due to the provider that doesn't respect the order of a delete. I think the problem is on the provider

doumhfr commented 2 years ago

Ok it's working :D

infradmin commented 2 years ago

I have the same issue: if one of address objects is going to be deleted and corresponding address groups object should be updated, it tries to destroy address first (obviously unsuccessful). BTW, interesting that if I destroy all address objects together with related address group, then the deletion order is correct — first a group and then addresses. Proposed work-around did not help.

lix-fortinet commented 2 years ago

Hi @infradmin,

Thank you for your comment. We could not handle the process sequence on the provider side. It is controlled by Terraform. The way we solve this issue is using lifecycle with argument create_before_destroy set to true. However, based on our test, it only works on dynamic resources using meta-argument count or for_each. If you are not using locals block and for_each, please try using count. For instance:

resource "fortios_firewall_addrgrp" "tf_address_grp" {
  name          = "TF_TEST_ADDRESS_GRP"
  allow_routing = "disable"
  color         = 3
  exclude       = "disable"

  member {
    name = fortios_firewall_address.tf_addresses1[0].name
  }
  member {
    name = fortios_firewall_address.tf_addresses2[0].name
  }

  depends_on = [
    fortios_firewall_address.tf_addresses1,
    fortios_firewall_address.tf_addresses2
  ]
}

resource "fortios_firewall_address" "tf_addresses1" {
  count = 1

  name          = TF_TEST_ADDRESS_01
  subnet        = "10.1.0.0/16"
  type          = "ipmask"
  color         = 3
  lifecycle {
    create_before_destroy = true
  }
}

resource "fortios_firewall_address" "tf_addresses2" {
  count = 1

  name          = TF_TEST_ADDRESS_01
  subnet        = "10.2.0.0/16"
  type          = "ipmask"
  color         = 3
  lifecycle {
    create_before_destroy = true
  }
}

When you want to delete resource fortios_firewall_address.tf_addresses2, you need to set the argument count to 0 and remove the related member in fortios_firewall_addrgrp:

resource "fortios_firewall_addrgrp" "tf_address_grp" {
  name          = "TF_TEST_ADDRESS_GRP"
  allow_routing = "disable"
  color         = 3
  exclude       = "disable"

  member {
    name = fortios_firewall_address.tf_addresses1[0].name
  }

  depends_on = [
    fortios_firewall_address.tf_addresses1,
    fortios_firewall_address.tf_addresses2
  ]
}

resource "fortios_firewall_address" "tf_addresses1" {
  count = 1

  name          = TF_TEST_ADDRESS_01
  subnet        = "10.1.0.0/16"
  type          = "ipmask"
  color         = 3
  lifecycle {
    create_before_destroy = true
  }
}

resource "fortios_firewall_address" "tf_addresses2" {
  count = 0

  name          = TF_TEST_ADDRESS_01
  subnet        = "10.2.0.0/16"
  type          = "ipmask"
  color         = 3
  lifecycle {
    create_before_destroy = true
  }
}

Please let me know if it still not work.

Thanks, Xing

infradmin commented 2 years ago

Thank you, @lix-fortinet for the fast reply, but actually I am using for_each... Here is my example:

locals {
  subnets = [
    {
      "name"   = "test1",
      "subnet" = "192.168.1.0/24",
      "group"  = "test_group"
    },
    {
      "name"   = "test2",
      "subnet" = "192.168.2.0/24",
      "group"  = "test_group"
    }
  ]
}

resource "fortios_firewall_address" "addr" {
  for_each      = { for i in local.subnets : i.name => i }
  name          = each.key
  subnet        = each.value.subnet
  type          = "ipmask"
  allow_routing = "disable"
  lifecycle {
    create_before_destroy = true
  }
}

resource "fortios_firewall_addrgrp" "addgrp" {
  for_each      = { for i in local.subnets : i.group => i... }
  name          = each.key
  allow_routing = "disable"
  dynamic "member" {
    for_each = each.value
    content {
      name = fortios_firewall_address.addr["${member.value["name"]}"].name
    }
  }
}

As you proposed I added lifecycle meta-argument but it did not help. If I remove "test2" subnet from local and apply again I get:

fortios_firewall_address.addr["test2"]: Destroying... [id=test2]
╷
│ Error: Error deleting FirewallAddress resource: Internal Server Error - Internal error when processing the request (500)

Using Terraform v1.1.3 and provider registry.terraform.io/fortinetdev/fortios v1.13.2.

zephyia commented 2 years ago

Hey all,

I have been mostly successful in getting this to work – here is what I am currently using:

terraform {

required_providers {

fortios = {

  source = "fortinetdev/fortios"

  #    version = "1.13.2"

}

}

}

provider "fortios" {

hostname = "10.80.16.253"

token = "xxxxx"

insecure = "true"

vdom = "root"

}

locals {

clients = {

client1 = {

  prod =  "10.1.1.1/32"

  office =  "10.1.1.2/32"

  home =  "10.1.1.3/32"

}

client2 = {

  main_street = "10.2.2.2/32"

}

client3 = {

  ip01 = "10.3.3.1/32",

  ip02 = "10.3.3.2/32",

  ip03 = "10.3.3.3/32",

  ip04 = "10.3.3.4/32"

}

}

clients_flat = flatten([ for k, v in local.clients : [ for k1, v1 in v : {

client = k

name = k1

ip = v1

} if length(keys(v)) > 0] ])

clients_with_ips = { for k, v in local.clients : k => v if length(keys(v)) > 0 }

clients_map = { for c in local.clientsflat : upper(format("%s%s",c.client,c.name)) => c.ip }

}

resource "fortios_firewall_address" "client_address" {

for_each = local.clients_map

name = each.key

subnet = each.value

type = "ipmask"

comment = "Managed by terraform"

allow_routing = "disable"

color = 3

lifecycle {

create_before_destroy = true

}

}

resource "fortios_firewall_addrgrp" "client_address_grp" {

for_each = local.clients_with_ips

name = upper(each.key)

allow_routing = "disable"

color = 3

exclude = "disable"

dynamic "member" {

for_each = sort(keys(each.value))

content {

  name = fortios_firewall_address.client_address[upper(format("%s_%s",each.key,member.value))].name

}

}

depends_on = [

fortios_firewall_address.client_address

]

}

The problem I am facing is that the provider seems to be incorrectly comparing the order of the members in an address group.

If you use the above to create the 3 address groups, then comment out “ip02” in client3 and apply, everything will work. Then uncomment it to add it back, that will also work – however form now on every time you run apply it the provider will try to update the address group as the order of the members in the terraform state doesn’t match the order of the members coming back from the fortigate api.

e.g.:

$ terraform1 apply

fortios_firewall_address.client_address["CLIENT3_IP03"]: Refreshing state... [id=CLIENT3_IP03]

fortios_firewall_address.client_address["CLIENT1_PROD"]: Refreshing state... [id=CLIENT1_PROD]

fortios_firewall_address.client_address["CLIENT3_IP02"]: Refreshing state... [id=CLIENT3_IP02]

fortios_firewall_address.client_address["CLIENT1_OFFICE"]: Refreshing state... [id=CLIENT1_OFFICE]

fortios_firewall_address.client_address["CLIENT1_HOME"]: Refreshing state... [id=CLIENT1_HOME]

fortios_firewall_address.client_address["CLIENT3_IP04"]: Refreshing state... [id=CLIENT3_IP04]

fortios_firewall_address.client_address["CLIENT3_IP01"]: Refreshing state... [id=CLIENT3_IP01]

fortios_firewall_address.client_address["CLIENT2_MAIN_STREET"]: Refreshing state... [id=CLIENT2_MAIN_STREET]

fortios_firewall_addrgrp.client_address_grp["client3"]: Refreshing state... [id=CLIENT3]

fortios_firewall_addrgrp.client_address_grp["client2"]: Refreshing state... [id=CLIENT2]

fortios_firewall_addrgrp.client_address_grp["client1"]: Refreshing state... [id=CLIENT1]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:

~ update in-place

Terraform will perform the following actions:

fortios_firewall_addrgrp.client_address_grp["client3"] will be updated in-place

~ resource "fortios_firewall_addrgrp" "client_address_grp" {

    id                    = "CLIENT3"

    name                  = "CLIENT3"

    # (6 unchanged attributes hidden)

  ~ member {

      ~ name = "CLIENT3_IP03" -> "CLIENT3_IP02"

    }

  ~ member {

      ~ name = "CLIENT3_IP04" -> "CLIENT3_IP03"

    }

  ~ member {

      ~ name = "CLIENT3_IP02" -> "CLIENT3_IP04"

    }

    # (1 unchanged block hidden)

}

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?

Terraform will perform the actions described above.

Only 'yes' will be accepted to approve.

The provider needs to be fixed to sort both lists before doing a comparison – can we get that addressed please?

Thanks

c.

From: infradmin @.> Sent: Tuesday, 11 January 2022 5:29 PM To: fortinetdev/terraform-provider-fortios @.> Cc: Chris Wright @.>; Mention @.> Subject: Re: [fortinetdev/terraform-provider-fortios] Managing address and address group resources - order of operations (#186)

Thank you, @lix-fortinet https://github.com/lix-fortinet for the fast reply, but actually I am using for_each... Here is my example:

locals {

subnets = [

{

  "name"   = "test1",

  "subnet" = "192.168.1.0/24",

  "group"  = "test_group"

},

{

  "name"   = "test2",

  "subnet" = "192.168.2.0/24",

  "group"  = "test_group"

}

]

}

resource "fortios_firewall_address" "addr" {

for_each = { for i in local.subnets : i.name => i }

name = each.key

subnet = each.value.subnet

type = "ipmask"

allow_routing = "disable"

lifecycle {

create_before_destroy = true

}

}

resource "fortios_firewall_addrgrp" "addgrp" {

for_each = { for i in local.subnets : i.group => i... }

name = each.key

allow_routing = "disable"

dynamic "member" {

for_each = each.value

content {

  name = fortios_firewall_address.addr["${member.value["name"]}"].name

}

}

}

As you proposed I added lifecycle meta-argument but it did not help. If I remove "test2" subnet from local and apply again I get:

fortios_firewall_address.addr["test2"]: Destroying... [id=test2]

│ Error: Error deleting FirewallAddress resource: Internal Server Error - Internal error when processing the request (500)

Using Terraform v1.1.3 and provider registry.terraform.io/fortinetdev/fortios v1.13.2.

— Reply to this email directly, view it on GitHub https://github.com/fortinetdev/terraform-provider-fortios/issues/186#issuecomment-1009638102 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6A5ITZEUOO5GAUC2DR2NLUVPE3HANCNFSM5FKTHTGQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub . You are receiving this because you were mentioned.Message ID: @.***>

lix-fortinet commented 2 years ago

Hi @zephyia,

Interesting, it works well in my end using exactly the same environment and configuration as yours.

$ terraform apply --auto-approve
fortios_firewall_address.addr["test2"]: Refreshing state... [id=test2]
fortios_firewall_address.addr["test1"]: Refreshing state... [id=test1]
fortios_firewall_addrgrp.addgrp["test_group"]: Refreshing state... [id=test_group]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place
  - destroy

Terraform will perform the following actions:

  # fortios_firewall_address.addr["test2"] will be destroyed
  # (because key ["test2"] is not in for_each map)
  - resource "fortios_firewall_address" "addr" {
      - allow_routing         = "disable" -> null
      - cache_ttl             = 0 -> null
      - clearpass_spt         = "unknown" -> null
      - color                 = 0 -> null
      - dynamic_sort_subtable = "false" -> null
      - sdn_addr_type         = "private" -> null
      - sub_type              = "sdn" -> null
      - subnet                = "192.168.2.0/24" -> null
      - type                  = "ipmask" -> null
      - uuid                  = "7bd2f5f6-7318-51ec-0465-bdd5b73ca82c" -> null
    }

  # fortios_firewall_addrgrp.addgrp["test_group"] will be updated in-place
  ~ resource "fortios_firewall_addrgrp" "addgrp" {
        id                    = "test_group"
        name                  = "test_group"
        # (6 unchanged attributes hidden)

      - member {
          - name = "test2" -> null
        }
        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 1 to destroy.
fortios_firewall_addrgrp.addgrp["test_group"]: Modifying... [id=test_group]
fortios_firewall_addrgrp.addgrp["test_group"]: Modifications complete after 0s [id=test_group]
fortios_firewall_address.addr["test2"]: Destroying... [id=test2]
fortios_firewall_address.addr["test2"]: Destruction complete after 0s

Apply complete! Resources: 0 added, 1 changed, 1 destroyed. 

Could you add argument depends_on in resource fortios_firewall_addrgrp.addgrp and have a try? Like this:

resource "fortios_firewall_addrgrp" "addgrp" {
  for_each = { for i in local.subnets : i.group => i... }
  name = each.key
  allow_routing = "disable"
  dynamic "member" {
    for_each = each.value
    content {
      name = fortios_firewall_address.addr["${member.value["name"]}"].name
    }
  }
  depends_on = [
    fortios_firewall_address.addr
  ]
}

If it still not work, please let me know the following info to help me reproduce in my end:

  1. Your OS, (Linux, Windows, MacOS, docker container, Linux APP under windows, etc.);
  2. FortiOS version;
  3. Does create_before_destroy exist under resource fortios_firewall_address in .tfstate file?
  4. Do you have any other configurations in your .tf file?

Thanks, Xing

zephyia commented 2 years ago

I think you have responded to the wrong person – if you look at the terraform I posted it already has the depends on you are suggesting to add.

Also, my issue is not with create / destroy order – my email clarified that is indeed working in the way my terraform is setup.

My issue is the comparison of members between what the terraform state thinks it should be and what is returned from the foritos API.

It is possible to get the state into a “state” where the order of the members is different to that returned from the API and even though the two lists contain the same members, because they are in different orders terraform keeps trying to update them.

I can reproduce this consistently with the terraform example from my previous email – which included the steps needed to reproduce the issue.

Thanks

c.

From: lix-fortinet @.> Sent: Wednesday, 12 January 2022 9:57 AM To: fortinetdev/terraform-provider-fortios @.> Cc: Chris Wright @.>; Mention @.> Subject: Re: [fortinetdev/terraform-provider-fortios] Managing address and address group resources - order of operations (#186)

Hi @zephyia https://github.com/zephyia ,

Interesting, it works well in my end using exactly the same environment and configuration as yours.

$ terraform apply --auto-approve fortios_firewall_address.addr["test2"]: Refreshing state... [id=test2] fortios_firewall_address.addr["test1"]: Refreshing state... [id=test1] fortios_firewall_addrgrp.addgrp["test_group"]: Refreshing state... [id=test_group]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols: ~ update in-place

Terraform will perform the following actions:

fortios_firewall_address.addr["test2"] will be destroyed

(because key ["test2"] is not in for_each map)

Plan: 0 to add, 1 to change, 1 to destroy. fortios_firewall_addrgrp.addgrp["test_group"]: Modifying... [id=test_group] fortios_firewall_addrgrp.addgrp["test_group"]: Modifications complete after 0s [id=test_group] fortios_firewall_address.addr["test2"]: Destroying... [id=test2] fortios_firewall_address.addr["test2"]: Destruction complete after 0s

Apply complete! Resources: 0 added, 1 changed, 1 destroyed.

Could you add argument depends_on in resource fortios_firewall_addrgrp.addgrp and have a try? Like this:

resource "fortios_firewall_addrgrp" "addgrp" { for_each = { for i in local.subnets : i.group => i... } name = each.key allow_routing = "disable" dynamic "member" { for_each = each.value content { name = fortios_firewall_address.addr["${member.value["name"]}"].name } } depends_on = [ fortios_firewall_address.addr ] }

If it still not work, please let me know the following info to help me reproduce in my end:

  1. Your OS, (Linux, Windows, MacOS, docker container, Linux APP under windows, etc.);
  2. FortiOS version;
  3. Does create_before_destroy exist under resource fortios_firewall_address in .tfstate file?
  4. Do you have any other configurations in your .tf file?

Thanks, Xing

— Reply to this email directly, view it on GitHub https://github.com/fortinetdev/terraform-provider-fortios/issues/186#issuecomment-1010441234 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AA6A5IV7J4ANAOCHXARP5V3UVSYU7ANCNFSM5FKTHTGQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub . You are receiving this because you were mentioned.Message ID: @.***>

infradmin commented 2 years ago

Hi @lix-fortinet I have realized that after adding create_before_destroy to resource block I need to run terraform apply first to add this to the state, and only then try to delete this resource. Thank you.

zephyia commented 2 years ago

Hi all,

Just following up on this issue:

With this terraform:

  required_providers {
    fortios = {
      source = "fortinetdev/fortios"
      #    version = "1.13.2"
    }
  }
}

provider "fortios" {
  hostname = "10.80.16.253"
  token    = "xxxxx"
  insecure = "true"
  vdom     = "root"
}

locals {
  clients = {
    client1 = {
      prod =  "10.1.1.1/32"
      office =  "10.1.1.2/32"
      home =  "10.1.1.3/32"
    }
    client2 = {
      main_street = "10.2.2.2/32"
    }
    client3 = {
      ip01 = "10.3.3.1/32",
      ip02 = "10.3.3.2/32",
      ip03 = "10.3.3.3/32",
      ip04 = "10.3.3.4/32"
    }
  }
  clients_flat = flatten([ for k, v in local.clients : [ for k1, v1 in v : {
    client = k
    name = k1
    ip = v1
  } if length(keys(v)) > 0] ])

  clients_with_ips = { for k, v in local.clients : k => v if length(keys(v)) > 0 }
  clients_map = { for c in local.clients_flat : upper(format("%s_%s",c.client,c.name)) => c.ip }
}

resource "fortios_firewall_address" "client_address" {
  for_each = local.clients_map

  name          = each.key
  subnet        = each.value
  type          = "ipmask"
  comment       = "Managed by terraform"
  allow_routing = "disable"
  color         = 3
  lifecycle {
    create_before_destroy = true
  }
}

resource "fortios_firewall_addrgrp" "client_address_grp" {
  for_each = local.clients_with_ips

  name          = upper(each.key)
  allow_routing = "disable"
  color         = 3
  exclude       = "disable"
  dynamic "member" {
    for_each = sort(keys(each.value))
    content {
      name = fortios_firewall_address.client_address[upper(format("%s_%s",each.key,member.value))].name
    }
  }
  depends_on = [
   fortios_firewall_address.client_address
  ]
}

The problem I am still facing is that the provider seems to be incorrectly comparing the order of the members in an address group.

If you use the above to create the 3 address groups, then comment out “ip02” in client3 and apply, everything will work. Then uncomment it to add it back, that will also work – however form now on every time you run an apply (without changing anything) the provider will try to update the address group to change the order of the members as the order of the members in the terraform state doesn’t match the order of the members coming back from the fortigate api.

e.g.:

fortios_firewall_address.client_address["CLIENT3_IP03"]: Refreshing state... [id=CLIENT3_IP03]
fortios_firewall_address.client_address["CLIENT1_PROD"]: Refreshing state... [id=CLIENT1_PROD]
fortios_firewall_address.client_address["CLIENT3_IP02"]: Refreshing state... [id=CLIENT3_IP02]
fortios_firewall_address.client_address["CLIENT1_OFFICE"]: Refreshing state... [id=CLIENT1_OFFICE]
fortios_firewall_address.client_address["CLIENT1_HOME"]: Refreshing state... [id=CLIENT1_HOME]
fortios_firewall_address.client_address["CLIENT3_IP04"]: Refreshing state... [id=CLIENT3_IP04]
fortios_firewall_address.client_address["CLIENT3_IP01"]: Refreshing state... [id=CLIENT3_IP01]
fortios_firewall_address.client_address["CLIENT2_MAIN_STREET"]: Refreshing state... [id=CLIENT2_MAIN_STREET]
fortios_firewall_addrgrp.client_address_grp["client3"]: Refreshing state... [id=CLIENT3]
fortios_firewall_addrgrp.client_address_grp["client2"]: Refreshing state... [id=CLIENT2]
fortios_firewall_addrgrp.client_address_grp["client1"]: Refreshing state... [id=CLIENT1]

Terraform used the selected providers to generate the following execution plan. Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  # fortios_firewall_addrgrp.client_address_grp["client3"] will be updated in-place
  ~ resource "fortios_firewall_addrgrp" "client_address_grp" {
        id                    = "CLIENT3"
        name                  = "CLIENT3"
        # (6 unchanged attributes hidden)

      ~ member {
          ~ name = "CLIENT3_IP03" -> "CLIENT3_IP02"
        }
      ~ member {
          ~ name = "CLIENT3_IP04" -> "CLIENT3_IP03"
        }
      ~ member {
          ~ name = "CLIENT3_IP02" -> "CLIENT3_IP04"
        }

        # (1 unchanged block hidden)
    }

Plan: 0 to add, 1 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

The provider needs to be fixed to sort both lists before doing a comparison – can we get that addressed please?

lix-fortinet commented 2 years ago

Hi @zephyia,

Could you try to set argument dynamic_sort_subtable to true and try it again? member will be sort by name when dynamic_sort_subtable set to true.

For your example:

resource "fortios_firewall_addrgrp" "client_address_grp" {
  for_each = local.clients_with_ips

  name          = upper(each.key)
  allow_routing = "disable"
  color         = 3
  exclude       = "disable"
  dynamic "member" {
    for_each = sort(keys(each.value))
    content {
      name = fortios_firewall_address.client_address[upper(format("%s_%s",each.key,member.value))].name
    }
  }
  depends_on = [
   fortios_firewall_address.client_address
  ]
  dynamic_sort_subtable = "true"
}

Please let me know if you have any questions.

Thanks, Xing

zephyia commented 2 years ago

Hi @lix-fortinet,

I have now tested with the property you suggested and can confirm that the issue is not present when this is set. I would suggest that it is probably worth explaining the non-standard use of a resource property like this somewhere prominent.

I think I can close out this issue now - do you agree?

Thanks

c.

lix-fortinet commented 2 years ago

Hi @zephyia,

Thank you for your advice. We will consider it and make improvement in the future release. And of course, you could close this issue if you do not have any other questions.

Thanks, Xing

lix-fortinet commented 1 year ago

Hi @zephyia,

We will close this issue since it has been fixed. Feel free to open a new issue if you have any other questions.

Thanks, Xing