hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.12k stars 9.47k forks source link

Incorrect order of operations with fortinetdev/fortios provider #30336

Closed infradmin closed 2 years ago

infradmin commented 2 years ago

Hello.

I got the issue with fortios provider but its developers say that it is terraform bug not a provider's one (link to the message).

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

Here is my code example (sorry, masking access info):

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

provider "fortios" {
  hostname = "my.fgt.domain.name"
  token    = "myfgtaccesstoken"
}

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"
}

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
    }
  }
}

The problem is order of operations. I have few address objects and one address group object, addresses are members of address group. Creation of these objects goes successfully.

But if I need to remove one of address objects, there is an issue. Terraform plan shows correctly that one item should be changed (address group — remove a member) and one item destroyed (address).

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
      - id                    = "test2" -> null
      - name                  = "test2" -> null
      - node_ip_only          = "disable" -> null
      - obj_type              = "ip" -> null
      - sdn_addr_type         = "private" -> null
      - sub_type              = "sdn" -> null
      - subnet                = "192.168.2.0/24" -> null
      - type                  = "ipmask" -> null
      - uuid                  = "46a9f3b6-72a5-51ec-f642-85336284b164" -> 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.

Expected Behavior

The proper order should be: first to change address group and then destroy address.

Actual Behavior

But terraform apply tries to destroy address first, resulting in obvious error because this address object is still used by address group.

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

Steps to Reproduce

  1. copy code example to .tf file (you will need to substitute provider login info)
  2. terraform init
  3. terraform apply
  4. remove "test2" subnet from local.subnets
  5. terraform apply
infradmin commented 2 years ago

A work-around is proposed to add

  lifecycle {
    create_before_destroy = true
  }

to the address resource, and in some cases it works. But is there a solution?

jbardin commented 2 years ago

@infradmin,

Providers cannot change the order of operations, nor does Terraform ever change ordering based on the provider. The order of operations is always done exactly as specified in the configuration, based on the relationships between resources. If the ordering you require is that created by create_before_destroy=true, then that is the configuration you need. The create_before_destroy option isn't a workaround, is how you declare when resources must be created, updated, and destroyed in the inverse order from the default.

We use GitHub issues for tracking bugs and enhancements, rather than for questions. While we can sometimes help with certain simple problems here, it's better to use the community forum where there are more people ready to help.

Thanks!

infradmin commented 2 years ago

@jbardin Thank you, surely you are right

Providers cannot change the order of operations, nor does Terraform ever change ordering based on the provider. The order of operations is always done exactly as specified in the configuration, based on the relationships between resources. If the ordering you require is that created by create_before_destroy=true, then that is the configuration you need. The create_before_destroy option isn't a workaround, is how you declare when resources must be created, updated, and destroyed in the inverse order from the default.

But in my case I expect relationships to be created when I reference address resources creating members of the address group:

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

I think in this case terraform should understand that address group must be changed before any of its members will be destroyed.

I work mostly with azurerm provider and there it works this way. Also it very logical and I believe this is the way terraform creators want us to use.

jbardin commented 2 years ago

I think in this case terraform should understand that address group must be changed before any of its members will be destroyed.

This unfortunately is not possible without some extra information. The default ordering is to destroy first, then create or update -- if you need something different from the default that extra information must be added to the configuration. It's possible a future version of terraform may allow providers to communicate some lifecycle options, but that can have major ramifications throughout a configuration as the order of all descendent resources needs to take this into account, many of which may not properly accommodate it. It's definitely something we are going to reevaluate once the time comes, but for now the explicit option in the config makes for a simple and robust solution which is well understood.

github-actions[bot] commented 2 years ago

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.