hashicorp / terraform-plugin-sdk

Terraform Plugin SDK enables building plugins (providers) to manage any service providers or custom in-house solutions
https://developer.hashicorp.com/terraform/plugin
Mozilla Public License 2.0
432 stars 230 forks source link

GCP authorized_networks block shows as modified in plan even though no visible change is indicated #162

Open bentinata opened 5 years ago

bentinata commented 5 years ago

Current Terraform Version

Terraform v0.12.1

Use-cases

Ever since v0.12, terraform plan and terraform apply no longer output a compact execution plan. Now, even a small change would list all resource property. Example, changing GCP SQL whitelisted IP now output:

~ settings {
    activation_policy           = "ALWAYS"
    authorized_gae_applications = []
    crash_safe_replication      = false
    disk_autoresize             = true
    disk_size                   = 10
    disk_type                   = "PD_SSD"
    pricing_plan                = "PER_USE"
    replication_type            = "SYNCHRONOUS"
    tier                        = "db-n1-standard-1"
    user_labels                 = {}
    version                     = 25

    backup_configuration {
        binary_log_enabled = true
        enabled            = true
        start_time         = "12:00"
    }

  ~ ip_configuration {
        ipv4_enabled = true
        require_ssl  = false

      - authorized_networks {
          - name  = "other" -> null
          - value = "10.0.0.1" -> null
        }
      - authorized_networks {
          - name  = "change-a" -> null
          - value = "10.0.0.2" -> null
        }
      + authorized_networks {
          + name  = "change-b"
          + value = "10.0.0.2"
        }
      + authorized_networks {
          + name  = "other"
          + value = "10.0.0.1"
        }
    }

    location_preference {
        zone = "asia-southeast1-b"
    }
}

"other" authorized networks doesn't need to be recreated. Properties like location_preference is not even changed.

Proposal

Just show changed properties. If it's deletion then show it as deletion (with -).

~ settings {
  ~ ip_configuration {
      ~ authorized_networks {
          ~ name  = "change-a" -> "change-b"
        }
    }
}
apparentlymart commented 5 years ago

Hi @bentinata! Thanks for raising this.

It looks like there are three different parts to this feature request:

  1. Only show changed properties.
  2. Why did changing change-a to change-b show as a delete and a recreate rather than an edit?
  3. Why did the authorized_network block with name = "other" show as being removed and then recreated here?

The first of those is already being discussed in hashicorp/terraform#21639, so I'm going to use this issue to focus on the other two.


The second point is a consequence of how the authorized_networks block is defined in the GCP provider schema: it's declared as being represented as a set of objects, and so the identity of each block (used for correlation in the diff) is the block's content; there is no other identifier to use.

Therefore changing the contents of the block changes that identity in a similar way as renaming a resource would behave, so Terraform sees it as removing one block and creating another rather than an edit in-place.

The GCP provider is specifying the attribute this way because with the current Terraform SDK version it is using the only options are to use the contents (be a set) or to correlate by position in sequence (be a list), and being a list would not be appropriate here since (I assume) these objects do not have a specific order when stored in the upstream API.

Terraform 0.12 has added a solution to this, but the SDK hasn't yet been updated to support it. The solution calls for moving that name attribute to be a label on the block, which Terraform would then use as the block's identity for correlation:

# Not yet supported, but GCP provider might adopt this solution
# in future once the SDK is able to support it.

  authorized_network "change-b" {
    value = "10.0.0.2"
  }
  authorized_network "other" {
    value = "10.0.0.1"
  }

Notably though, in your example use changed the name, so even with the above model the change you made would appear as a remove and an add, because the identity of the object was changed rather than an argument within it.

Terraform Core chooses its presentation of changes to blocks based on the way the provider has elected to represent those blocks in its schema, so Terraform Core's behavior is correct here though I do expect that once the SDK is able to support blocks with labels like above the use of sets to represent blocks will become more rare, in favor of named blocks which have a distinct key that Terraform can correlate by, and thus produce better change renderings.


That then leaves the third question: why did the name = "other" block show as being changed (removed and added) here even though the attributes shown don't seem to have changed at all?

This one does seem like a bug: I suspect something is happening with the other expiration_time argument which the diff rendering isn't showing, such as it changing from being entirely unset (null) to being an empty string (""). If that is the case, that suggests a bug in the SDK itself because the SDK is responsible for handling default values for arguments during planning.

With that said, I'm going to label this as an SDK bug so that it'll be easy to find and track by the team that is about to start work on updating the SDK to support the new Terraform 0.12 type system features, which is likely to include also fixing some quirks that result from the SDK still primarily talking the Terraform 0.11 protocol for backward compatibility.

Thanks for reporting this, @bentinata!