fortinetdev / terraform-provider-fortios

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

Policy sorting doesn't work reliably with for_each loop #314

Open p-v-a opened 8 months ago

p-v-a commented 8 months ago

I'd like to draw some attention to way fortinet API and terraform interfere causing issues with terraform for_each. I'm pretty sure similar issue exists in other places, however I'm going to demonstrate an issue using policy fortios_firewall_security_policyseq resource.

in this particular scenario we have an array of objects, that defines policies. There are few helpers variables derived from it, which I'm going to omit here. Important point is that both fortios_firewall_policy and fortios_firewall_security_policyseq gets created using for_each loop.

resource "fortios_firewall_policy" "tr_policy" {
  for_each                    = { for item in local.flatten_policies : format(local.resource_name_format, item.name, var.vdom) => item }

  name                        = each.value.name

  global_label                = each.value.global_label
  comments                    = format("Terraform - %s", each.value.comment)

  action                      = each.value.action
  schedule                    = each.value.schedule
  ....
}

resource "fortios_firewall_security_policyseq" "tr_policy_ordering" {
  for_each              = { for item in local.policies_order : item.key => item.value if item.value.next_policy_id != null}
  vdomparam             = each.value.vdom
  policy_src_id         = each.value.policy_id
  policy_dst_id         = each.value.next_policy_id
  alter_position        = "before"
  enable_state_checking = true
}

The issue arise in fortios_firewall_security_policyseq, as terraform calling fortios_firewall_security_policyseq not sequentially, it least to final order mangled. Assume we have current policies order like this [16, 15, 14, 13, 12, 11, 10, 9, 8, 7, 6, 5, 4, 3, 2, 1], and desired policy order is [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16].

Terraform would update fortios_firewall_security_policyseq resource for policy 9, and place it before policy 10. resulting in [16, 15, 14, 13, 12, 11, 9, 10, 8, 7, 6, 5, 4, 3, 2, 1]. Next it tries to order policy 10, and place it before policy 11, resulting in [16, 15, 14, 13, 12, 10, 11, 9, 8, 7, 6, 5, 4, 3, 2, 1], etc. as you can see it causes incorrect order to be applied, even if original srd-id and dst-id are sorted in the right order.

As far as I can see there is no way out of this, unless sorting is treated as an atomic operation that acts upon whole policy list within one resource.

So instead of fortios_firewall_security_policyseq, which takes only srd-id and dst-id, we need something that can sort policies in one operatino:

resource "fortios_firewall_security_policy_order" "tr_policy_ordering" {
  dynamic "policy_order" {
    for_each   =  local.policies_order 
   }

    content {
      policyid               = policy_order.value.policyid
      anchor_policyid        = policy_order.value.anchor_policyid
      relation               = policy_order.value.relation
    }
  }
MaxxLiu22 commented 8 months ago

Hi @p-v-a ,

Thank you for raising this question, we have a resource called fortios_firewall_securitypolicy_sort to sort policies by policyid or name ascendingly, ascendingly or in a manual order. here is an example for your reference. let me know if that doesn't solve your question.

variable "name" {
  type        = list(any)
  default     = ["z", "b", "c", "d"]
  description = "description"
}

# variable name {
#   type        = list
#   default     = ["a", "f", "zgt", "za", "zzz", "334", "25", "1689"]
#   description = "description"
# }

resource "fortios_firewall_securitypolicy" "trname" {
  for_each   = toset(var.name)
  action     = "accept"
  logtraffic = "utm"
  name       = each.key
  schedule   = "always"

  dstaddr {
    name = "all"
  }

  dstintf {
    name = port2
  }

  srcaddr {
    name = "all"
  }

  srcintf {
    name = port3
  }
}

resource "fortios_firewall_securitypolicy_sort" "test" {
  sortby         = "name"   # ["name", "policyid"]
  sortdirection  = "manual" #  ["descending", "ascending", "manual"]
  force_recreate = join(" ", var.name)
  manual_order   = var.name #["z", "b", "c", "d"]

  depends_on = [
    fortios_firewall_securitypolicy.trname
  ]
}

Thanks, Maxx

p-v-a commented 8 months ago

I looked at this previously, but documentation is missing example, and from description of manual_order and sortby attributes I came to conclusion that it just sort by name, and you can't define an order.

Thank you for pointing this out, this simplifies things a lot!

MaxxLiu22 commented 8 months ago

Thank you for raising this flaw in our documents, sorry for the confusing, I will report that to the development team for improvement, Thank you again for pointing that out.

p-v-a commented 8 months ago

Just for the benefit of other who might be looking for a solution like i was.

andyburridge commented 1 week ago

@p-v-a sorry to hijack this thread, but I'm curious on usage of the fortios_firewall_policy_sortresource.

I have separate terraform files per interface pair, e.g.

internal-external.tf external-internal.tf ...

Ideally I would like to have a fortios_firewall_policy_sort resource per terraform file to order the policies whilst maintaining the same structure. Do you know:

MaxxLiu22 commented 1 week ago

Hi @andyburridge ,

Thank you for your question. I conducted some experiments, and here is what I found:

Setting sortdirection = "ascending" or "descending" applies to group policy sorting, which will sort all policies in either ascending or descending order, as defined for each group. For example, here is a case of ascending order:

image

Setting sortdirection = "manual" applies to group-specific sorting, meaning that policies within the same group will be sorted based on your own defined order. Example:

resource "fortios_firewall_policy_sort" "test2" {
  sortby        = "name"
  sortdirection = "manual"
  manual_order = ["d", "f", "e"]

  depends_on = [
    fortios_firewall_policy.trname2, fortios_firewall_policy_sort.test
  ]
}

image

Regarding your first question: Yes, you can have multiple fortios_firewall_policy_sort resources. However, as mentioned above, if you set sortdirection = "ascending" or "descending", it applies to group-wide policy sorting. The last fortios_firewall_policy_sort will overwrite the previous one. To define different group policy orders, you can use multiple fortios_firewall_policy_sort resources with sortdirection = "manual".

For your second question: If sortdirection is set to "manual", then yes, you can achieve this. Otherwise, I’m afraid not.

From my understanding, if you need a more complex sorting scenario, you might consider defining their order in their names and sorting accordingly, or manually define their order in each Terraform file by setting sortdirection = "manual"

Let me know if you still have questions.

Thanks, Maxx

andyburridge commented 1 week ago

Thanks @MaxxLiu22 that's perfect, exactly what I needed to understand. Really appreciate the support.