terraform-plugin-sdk CustomizeDiff using ForceNewIfChange on TypeSet Configuration Block Reporting Inconsistent Plan

Terraform Version

# go.mod
github.com/hashicorp/terraform v0.12.0-beta2

Terraform Resource Files

In the Terraform AWS Provider:

# aws/resource_aws_route53_zone.go (inside &schema.Resource)

		CustomizeDiff: customdiff.All(
			customdiff.ForceNewIfChange("vpc", func(old, new, meta interface{}) bool {
				// "vpc" can only be in-place updated if already specified
				return old.(*schema.Set).Len() == 0 || new.(*schema.Set).Len() == 0
			}),
		),

# aws/resource_aws_route53_zone_test.go

// Reference: https://github.com/terraform-providers/terraform-provider-aws/issues/7614
func TestAccAWSRoute53Zone_VPC_ForceNewIfChange(t *testing.T) {
	var zone1, zone2, zone3 route53.GetHostedZoneOutput

	rName := acctest.RandomWithPrefix("tf-acc-test")
	resourceName := "aws_route53_zone.test"
	vpcResourceName := "aws_vpc.test1"
	zoneName := fmt.Sprintf("%s.terraformtest.com", rName)

	resource.ParallelTest(t, resource.TestCase{
		PreCheck:     func() { testAccPreCheck(t) },
		Providers:    testAccProviders,
		CheckDestroy: testAccCheckRoute53ZoneDestroy,
		Steps: []resource.TestStep{
			{
				Config: testAccRoute53ZoneConfig(zoneName),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckRoute53ZoneExists(resourceName, &zone1),
					resource.TestCheckResourceAttr(resourceName, "vpc.#", "0"),
				),
			},
			{
				Config: testAccRoute53ZoneConfigVPCSingle(rName, zoneName),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckRoute53ZoneExists(resourceName, &zone2),
					resource.TestCheckResourceAttr(resourceName, "vpc.#", "1"),
					testAccCheckRoute53ZoneAssociatesWithVpc(vpcResourceName, &zone2),
				),
			},
			{
				Config: testAccRoute53ZoneConfig(zoneName),
				Check: resource.ComposeTestCheckFunc(
					testAccCheckRoute53ZoneExists(resourceName, &zone3),
					resource.TestCheckResourceAttr(resourceName, "vpc.#", "0"),
				),
			},
		},
	})
}

Expected Behavior

Terraform Provider SDK marks resource for recreation when going from 0 vpc configuration blocks to greater than 0 configuration blocks or vice versa.

Actual Behavior

--- FAIL: TestAccAWSRoute53Zone_VPC_ForceNewIfChange (55.58s)
    testing.go:568: Step 1 error: errors during apply:

        Error: Provider produced inconsistent final plan

        When expanding the plan for aws_route53_zone.test to include new values
        learned so far during apply, provider "aws" produced an invalid new value for
        .vpc: planned set element cty.Value{ty:
        cty.Object(map[string]cty.Type{"vpc_id":cty.String, "vpc_region":cty.String}),
        v: map[string]interface {}{"vpc_id":"", "vpc_region":cty.unknown}} does not
        correlate with any element in actual.

        This is a bug in the provider, which should be reported in the provider's own
        issue tracker.

Steps to Reproduce

TF_ACC=1 go test ./aws -v -timeout 120m -run='TestAccAWSRoute53Zone_VPC_ForceNewIfChange'

Additional Context

This was originally tried with the Terraform 0.11 Provider SDK a few months back, which yielded the previous iteration of this error:

--- FAIL: TestAccAWSRoute53Zone_VPC_ForceNewIfChange (52.34s)
    testing.go:538: Step 1 error: Error applying: 1 error occurred:
        	* aws_route53_zone.test: aws_route53_zone.test: diffs didn't match during apply. This is a bug with Terraform and should be reported as a GitHub Issue.

        Please include the following information in your report:

            Terraform Version: 0.11.9
            Resource ID: aws_route53_zone.test
            Mismatch reason: attribute mismatch: vpc.2118463170.vpc_id
            Diff One (usually from plan): *terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{"name_servers.#":*terraform.ResourceAttrDiff{Old:"4", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "vpc.~2118463170.vpc_region":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "vpc.2118463170.vpc_id":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "vpc.2118463170.vpc_region":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "force_destroy":*terraform.ResourceAttrDiff{Old:"false", New:"false", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "name":*terraform.ResourceAttrDiff{Old:"tf-acc-test-6356618791358131134.terraformtest.com.", New:"tf-acc-test-6356618791358131134.terraformtest.com.", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "comment":*terraform.ResourceAttrDiff{Old:"Managed by Terraform", New:"Managed by Terraform", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "vpc.#":*terraform.ResourceAttrDiff{Old:"0", New:"1", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:true, Sensitive:false, Type:0x0}, "vpc.~2118463170.vpc_id":*terraform.ResourceAttrDiff{Old:"", New:"${aws_vpc.test1.id}", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "zone_id":*terraform.ResourceAttrDiff{Old:"Z1AT08GB5RV06G", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, Meta:map[string]interface {}(nil)}
            Diff Two (usually from apply): *terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{"vpc.#":*terraform.ResourceAttrDiff{Old:"", New:"1", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:true, Sensitive:false, Type:0x0}, "zone_id":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "name":*terraform.ResourceAttrDiff{Old:"", New:"tf-acc-test-6356618791358131134.terraformtest.com.", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:true, Sensitive:false, Type:0x0}, "comment":*terraform.ResourceAttrDiff{Old:"", New:"Managed by Terraform", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "vpc.3803688031.vpc_region":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "vpc.3803688031.vpc_id":*terraform.ResourceAttrDiff{Old:"", New:"vpc-0406819a6dadcf2a0", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "name_servers.#":*terraform.ResourceAttrDiff{Old:"", New:"", NewComputed:true, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}, "force_destroy":*terraform.ResourceAttrDiff{Old:"", New:"false", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, Meta:map[string]interface {}(nil)}

        Also include as much context as you can about your config, state, and the steps you performed to trigger this error.

References

https://github.com/terraform-providers/terraform-provider-aws/issues/7614

Apr 23 '19 10:04 bflad

From the Additional Context here it seems like this issue was present in v0.11 too and thus it's probably a bug either in the SDK code itself (not the v0.12 shims) or possibly a design flaw in CustomizeDiff where it's not properly preserving the invariants Terraform Core is relying on.

With that said, it does seem like something we should fix but isn't a blocker for v0.12.0 release, so I'm going to remove it from the release milestone. The provider-sdk label will prompt us to look at this again as part of subsequent work to improve the SDK.

Apr 23 '19 20:04 apparentlymart

After some serious diving, the root cause here is how helper/schema handles a ForceNew triggered by a CustomizeDiff on a set. When a ForceNew is encountered it causes the a new diff to be generated, but that seems to be missing New values (probably because it needs to be run without state), and it can generate different hashes which will add duplicate entries in the final diff (which is what is causing both the "diffs didn't match" and "inconsistent final plan" error ).

We may be able to fix why the ~ is or is not being added to the keys in this particular case, but CustomizeDiff on a set needs some more research in general because it's not clear how that could work with a set since the recalculated diff keys can't be correlated with the original. We may want to consider recommending not using CustomizeDiff on sets at all.

Apr 24 '19 14:04 jbardin

In this particular case it doesn't seem like there's a strong reason why this vpc block needs to be a set... it has MaxItems: 1 anyway, so it being a list or a set wouldn't really make any significant difference here; the diff UX would even be marginally better using TypeList because it would be able to show in-place updates in the block rather than adding/removing a block.

Thinking though what you said here, this seems to me just like a generalization of our advice so far that Computed: true should be avoided inside sets. The CustomizeDiff contract was already intentionally constrained to work within a set of rules that I think would avoid this problem in that case: it allows value changes only to Computed attributes that are not set in the config -- and thus, if you don't use Computed in a set, effectively bars you from using it against values in that set.

It will still allow ForceNew to be activated on set elements, but I believe if they don't have any computed attributes inside then that should be harmless... the set hashes will match because the configuration alone is driving those values.

With all of that said: @bflad, could we address this specific case by switching this vpc block to be TypeList instead? I believe that would clear up all of the confusion here, because the single vpc block will be represented by list index zero in all cases.

Apr 24 '19 16:04 apparentlymart

Apologies for not specifically calling this out, but this is totally fine to be outside the scope of v0.12.0. 👍

@apparentlymart did you maybe misread MinItems: 1 for MaxItems: 1? 😄

https://github.com/terraform-providers/terraform-provider-aws/blob/9aebcd64195d97c7c8e2f629d2004c65858138ae/aws/resource_aws_route53_zone.go#L43-L63

I wish this was something you can have just one item and I could fix in that manner, but alas, Route53 supports 100+ VPC associations for private zones.

May 07 '19 01:05 bflad

Ahh indeed, I apparently did (optimistically) misread.

In that case, I think the other advice here is probably more relevant: can we design this in a different way so that there are no Computed attributes in the set? The behavior of those will always be troublesome, since in some ways the problem is essentially undecidable (in the worst case, all of a block's attributes could be computed and so we'd have nothing at all to use to correlate what ForceNew is applying to).

In the shorter term, perhaps it would work to set the whole set as forcing replacement, rather than a specific attribute of one set element, which should then avoid the need to correlate with a particular element, at the expense of a less precise plan rendering.

May 07 '19 01:05 apparentlymart

I'm not sure if this is directly related, but I hit Mismatch reason: attribute mismatch issue with Terraform v0.11.13, when I used interpolation when passing property from one resource to another resource. My example:

resource "helm_release" "ceph-cluster" {
  <omitted unrelated>
  values     = "nodes:\n${join("", formatlist("- %s\n", hcloud_server.servers.*.name))}"
}

I was getting following error:

    Mismatch reason: attribute mismatch: values
    Diff One (usually from plan): *terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff{"values":*terraform.ResourceAttrDiff{Old:"nodes:\n- testing-1.xxx\n- testing-2.xxx\n- testing-3.xxx\n", New:"nodes:\n- testing-1.xxx\n- testing-2.xxx\n", NewComputed:false, NewRemoved:false, NewExtra:interface {}(nil), RequiresNew:false, Sensitive:false, Type:0x0}}, Destroy:false, DestroyDeposed:false, DestroyTainted:false, Meta:map[string]interface {}(nil)}
    Diff Two (usually from apply): *terraform.InstanceDiff{mu:sync.Mutex{state:0, sema:0x0}, Attributes:map[string]*terraform.ResourceAttrDiff(nil), Destroy:false, DestroyDeposed:false, DestroyTainted:false, Meta:map[string]interface {}(nil)}

If found an workaround for that though, which is null_resource:

resource "null_resource" "ceph_cluster_nodes" {
  triggers {
    nodes_list = "nodes:\n${join("", formatlist("- %s\n", hcloud_server.servers.*.name))}"
  }
}
resource "helm_release" "ceph-cluster" {
  values = "${null_resource.ceph_cluster_nodes.triggers.nodes_list}"
}

I hope people who hit similar issues will find this comment.

May 21 '19 20:05 invidian

I ran into this issue as well in 0.12, only with aws_route_table route block entries. It was seeing a diff from "" -> null which I think threw it for a loop. I worked around it by splitting out those route blocks into separate aws_route resources and imported them, and that fixed the state. I suppose it would have also worked the same by manually removing the aws_route_table state and re-importing it.

May 24 '19 17:05 jnicholls