d.HasChange() is returning true for elements that have not actually changed

[danawillow]

Terraform Version

terraform -v
Terraform v0.11.3
+ provider.google (unversioned)

Terraform Configuration Files

resource "google_service_account" "is-1108" {
  account_id   = "is-1108"
  display_name = "is-1108"
}

resource "google_project_iam_member" "is-1108" {
  role   = "roles/editor"
  member = "serviceAccount:${google_service_account.is-1108.email}"
}

resource "google_compute_instance" "is-1108" {
  name         = "is-1108-3"
  machine_type = "n1-standard-1"
  zone         = "us-central1-f"
  tags         = ["foo", "bar"]

  boot_disk {
    initialize_params {
      image = "debian-cloud/debian-9"
    }
  }

  network_interface {
    network = "default"
    access_config {
      # Ephemeral IP
    }
  }

  service_account {
    email = "${google_service_account.is-1108.email}"
    scopes = []
  }

  depends_on = ["google_project_iam_member.is-1108"]
}

Debug Output

$ terraform plan
Refreshing Terraform state in-memory prior to plan...
The refreshed state will be used to calculate this plan, but will not be
persisted to local or remote state storage.

google_service_account.is-1108: Refreshing state... (ID: projects/graphite-test-danahoffman-tf/s...danahoffman-tf.iam.gserviceaccount.com)
google_project_iam_member.is-1108: Refreshing state... (ID: graphite-test-danahoffman-tf/roles/edit...danahoffman-tf.iam.gserviceaccount.com)
google_compute_instance.is-1108: Refreshing state... (ID: is-1108-3)

------------------------------------------------------------------------

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  ~ update in-place

Terraform will perform the following actions:

  ~ google_compute_instance.is-1108
      tags.1996459178: "" => "bar"
      tags.2015626392: "baz" => ""
      tags.2356372769: "foo" => "foo"


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

------------------------------------------------------------------------

Note: You didn't specify an "-out" parameter to save this plan, so Terraform
can't guarantee that exactly these actions will be performed if
"terraform apply" is subsequently run.

https://gist.github.com/danawillow/fac76b10939fab4a31112108e70bbf37
Note the extra statements I added at the end:

2018-02-21T13:26:39.443-0800 [DEBUG] plugin.terraform-provider-google: 2018/02/21 13:26:39 [INFO] machine_type: HasChange false, Old n1-standard-1, New n1-standard-1
2018-02-21T13:26:39.443-0800 [DEBUG] plugin.terraform-provider-google: 2018/02/21 13:26:39 [INFO] min_cpu_platform: HasChange false, Old , New
2018-02-21T13:26:39.443-0800 [DEBUG] plugin.terraform-provider-google: 2018/02/21 13:26:39 [INFO] service_account: HasChange true, Old [map[scopes:0xc42024f280 email:[email protected]]], New [map[email:[email protected] scopes:0xc42024f1a0]]
2018-02-21T13:26:39.443-0800 [DEBUG] plugin.terraform-provider-google: 2018/02/21 13:26:39 [INFO] oScopes: []string{}
2018-02-21T13:26:39.443-0800 [DEBUG] plugin.terraform-provider-google: 2018/02/21 13:26:39 [INFO] nScopes: []string{}

Expected Behavior

Only the tags should have updated

Actual Behavior

The tags did update, but d.HasChange("service_account") returned true, causing Terraform to try to update the service account as well.

Steps to Reproduce

Use config above

1. terraform init
2. terraform apply
3. Change the tags in some way
4. terraform apply

Additional Context

None.

References

Originally reported at hashicorp/terraform-provider-google#1108, though the real bug appears to be in core and not the provider.

[apparentlymart]

Hi @danawillow! Sorry for this odd behavior...

I have a suspicion that the scopes argument is confusing the diff detector here; perhaps there is a bug in its handling of nested lists.

If you print that out using spew.Sdump (so we can see the full deep data structure) are there any differences evident in the nested list? (perhaps even something that shouldn't cause a difference, like a different slice cap.)

This code is going to get an overhaul soon as part of introducing the type system associated with the improved configuration language, and that ought to help with some of the trickier parts of helper/schema by being a (hopefully) less-leaky abstraction. If we can find a more tactical fix in the immediate term I'm happy to put it in, though.

[danawillow]

Thanks @apparentlymart! This is a fun one :)

oSpew: ([]interface {}) (len=1 cap=1) {
 (map[string]interface {}) (len=2) {
  (string) (len=5) "email": (string) (len=60) "[email protected]",
  (string) (len=6) "scopes": (*schema.Set)(0xc420456c40)({
   F: (schema.SchemaSetFunc) 0x1f19420,
   m: (map[string]interface {}) <nil>,
   once: (sync.Once) {
    m: (sync.Mutex) {
     state: (int32) 0,
     sema: (uint32) 0
    },
    done: (uint32) 0
   }
  })
 }
}

nSpew: ([]interface {}) (len=1 cap=1) {
 (map[string]interface {}) (len=2) {
  (string) (len=5) "email": (string) (len=60) "[email protected]",
  (string) (len=6) "scopes": (*schema.Set)(0xc420456b80)({
   F: (schema.SchemaSetFunc) 0x1f19420,
   m: (map[string]interface {}) <nil>,
   once: (sync.Once) {
    m: (sync.Mutex) {
     state: (int32) 0,
     sema: (uint32) 0
    },
    done: (uint32) 0
   }
  })
 }
}

[apparentlymart]

Ahh, so literally the only change is the address of the schema.Set in memory. 🤦‍♂️

Fortunately, this was enough to give me a lead which led to an explanation:

https://github.com/hashicorp/terraform/blob/9cf5350590ab25e8f2b51a69f9136f11d97369a0/helper/schema/resource_data.go#L138-L145

We have this Equal interface to allow *schema.Set to override its equality check because reflect.DeepEqual doesn't work for sets: the function pointer inside can never compare equal (see golang/go#8554), so it always returns false.

Unfortunately in this case the set is in a nested structure, so the top-level []interface{} does not implement Equal, and we end up in the DeepEqual codepath, where we return true because of that nested SchemaSetFunc.

It seems like the answer here would be to rewrite the equality checker to support all types and not use DeepEqual at all, and that is exactly the work we have ahead of us for the new type system anyway. With that said, it seems like there isn't really a quick tactical fix and so we'll just need to wait for this to get resolved naturally as part of switching to the new type system, which has its own "deep equal" function that behaves properly for all types within the system.

Fortunately we'll be embarking on this work soon (design/planning for it is in progress as I write this) so it shouldn't be too long before we can get a fix here. In the mean time, I suspect it could be worked around by retrieving the values and doing the deep-compare yourself in the provider code; as long as you are careful to use set.Equal rather than reflect.DeepEqual to compare the scopes set then things should work as expected.

[danawillow]

Thanks for the explanation! Looking forward to the new type system :)

[lawliet89]

Hey @apparentlymart, this still seem to be happening with Terraform 0.12.

I was trying to fix hashicorp/terraform-provider-vault#469 by adding CustomizeDiff to detect if the computed value requires updating. It seems like the binding set is always detected as having a diff despite being unchanged:

--- FAIL: TestGCPSecretRoleset (13.90s)
    testing.go:568: Step 0 error: After applying this step, the plan was not empty:
        
        DIFF:
        
        UPDATE: vault_gcp_secret_roleset.test
          backend:               "tf-test-gcp-5770619986517313269" => "tf-test-gcp-5770619986517313269"
          binding.#:             "1" => "1"
          binding.0.resource:    "//cloudresourcemanager.googleapis.com/projects/<redacted>" => "//cloudresourcemanager.googleapis.com/projects/<redacted>"
          binding.0.roles.#:     "1" => "1"
          binding.0.roles.0:     "roles/viewer" => "roles/viewer"
          id:                    "tf-test-gcp-5770619986517313269/roleset/tf-test-7606913612121587682" => "tf-test-gcp-5770619986517313269/roleset/tf-test-7606913612121587682"
          project:               "<redacted>" => "<redacted>"
          roleset:               "tf-test-7606913612121587682" => "tf-test-7606913612121587682"
          secret_type:           "access_token" => "access_token"
          service_account_email: "vaulttf-test-760691-1564118493@<redacted>.iam.gserviceaccount.com" => "<computed>"
          token_scopes.#:        "1" => "1"
          token_scopes.0:        "https://www.googleapis.com/auth/cloud-platform" => "https://www.googleapis.com/auth/cloud-platform"
        
        
        
        STATE:
        
        vault_gcp_secret_backend.test:
          ID = tf-test-gcp-5770619986517313269
          provider = provider.vault
          credentials =<redacted>
          default_lease_ttl_seconds = 0
          description = 
          max_lease_ttl_seconds = 0
          path = tf-test-gcp-5770619986517313269
        vault_gcp_secret_roleset.test:
          ID = tf-test-gcp-5770619986517313269/roleset/tf-test-7606913612121587682
          provider = provider.vault
          backend = tf-test-gcp-5770619986517313269
          binding.# = 1
          binding.0.resource = //cloudresourcemanager.googleapis.com/projects/<redacted>
          binding.0.roles.# = 1
          binding.0.roles.0 = roles/viewer
          project = <redacted>
          roleset = tf-test-7606913612121587682
          secret_type = access_token
          service_account_email = vaulttf-test-760691-1564118493@<redacted>.iam.gserviceaccount.com
          token_scopes.# = 1
          token_scopes.0 = https://www.googleapis.com/auth/cloud-platform
        
          Dependencies:
            vault_gcp_secret_backend.test
FAIL

Not sure what I can do about this.

Edit: Worked around it by doing the comparison manually.

[rileykarson]

I've encountered this as well, causing hashicorp/terraform-provider-google#4411. Diff from spew is at https://www.diffchecker.com/sMzxWDn3 (caveat: I rearranged the fields + modified some timestamps so they diffed cleanly).

Interestingly, the diff seems to persist when the scheduling block is no longer defined in config and should be Computed/unknown, as well as when lifecycle.ignore_chages is set on the block.

Edit: Also fwiw, my experience was with 0.12.6 and 0.12.8 with the Google provider @ HEAD.

[radeksimko]

0.12 did introduce new type system which is mentioned here, but the way it's currently exposed to providers is through shim layers, which effectively means that the new "enriched" cty type is received from core and almost immediately converted into the "old" type (TypeSet in this case), which still has the same (presumably broken) implementation of comparison/equality.

This is the likely reason it's still reproducible in 0.12 - correct me if I'm wrong @apparentlymart

We are in active talks currently about how we could expose these new types and what impact would it have on existing (arguably limited) interface which is exposed to providers today (e.g. schema.ResourceData). These talks haven't produced any outcomes nor deadlines yet.

If you are interested in more technical details I believe this is the main place where all the data gets converted back and forth:
https://github.com/hashicorp/terraform-plugin-sdk/blob/7d2b5732beb0f5a5be6ceec1ceb522f9b53fe1a2/internal/helper/plugin/grpc_provider.go

[apparentlymart]

Yes, I'd expect that this bug is still present, for the reason @radeksimko stated.

FWIW, my earlier comment about the potential to fix this with the new type system was made before we'd done significant work on what turned out to be quite a substantial amount of shim code to preserve (as closely as possible) old behaviors. While what I said is still broadly true -- comparing sets with cty should produce the correct result -- the story for exposing that capability inside the SDK API got a lot more complicated than we had expected at the time I wrote that comment.

[paddycarver]

#362 may mitigate and resolve this in certain scenarios.

[bflad]

I believe this should be resolved by #711, which was recently merged and will be released with version 2.9.0. If there are still issues after that, please file a new bug report and we can take a fresh look. Thank you!

[github-actions]

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.