Open bflad opened 5 years ago
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.
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.
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.
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
? 😄
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.
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.
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.
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.
Terraform Version
Terraform Resource Files
In the Terraform AWS Provider:
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
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:
References