Incorrect plan when using `replace_triggered_by` with `-replace`

[g7r]

Terraform Version

Terraform v1.13.4
on linux_amd64

Terraform Configuration Files

variable "t" {
  type = string
}

resource "terraform_data" "t" {
  count = 100
  input = var.t
}

resource "terraform_data" "r" {
  count = 100
  input = count.index
  lifecycle {
    replace_triggered_by = [terraform_data.t[count.index]]
  }
}

Debug Output

https://gist.githubusercontent.com/g7r/759be7835d33f6fbc38828f61fdff4f8/raw/03ab3123308cd4ef6f50ef65863923b374e2cc94/tftrace.log

Expected Behavior

Plan: 100 to add, 100 to change, 100 to destroy.

Actual Behavior

Plan: 16 to add, 97 to change, 16 to destroy.

The numbers vary every time.

Steps to Reproduce

1. terraform init
2. terraform apply -var t=a
3. terraform plan -var t=b -replace 'terraform_data.r[0]' -replace 'terraform_data.r[1]' -replace 'terraform_data.r[2]'

Additional Context

b.ForceReplace slice obtained from command line arguments is being passed by reference to graph nodes: https://github.com/hashicorp/terraform/blob/v1.13.4/internal/terraform/graph_builder_plan.go#L308

The number of -replace arguments matters. If len(b.ForceReplace) == cap(b.ForceReplace), the bug doesn't reproduce.

It doesn't matter which addresses to use for -replace. E.g. you can replace 3rd action with:

• terraform plan -var t=b -replace a.a -replace a.b -replace a.c
• terraform plan -var t=b -replace a.a -replace a.a -replace a.a

The same slice is being concurrently modified: https://github.com/hashicorp/terraform/blob/v1.13.4/internal/terraform/node_resource_plan_instance.go#L572

I suggest making a copy of the slice at graph_builder_plan.go.

References

No response

Generative AI / LLM assisted development?

No response

[jbardin]

Hi @g7r,

Thanks for filing the issue!
Juts to make sure, the expected outcome here would not be 100 to add, 100 to change, 100 to destroy, but rather 103 to add, 97 to change, 103 to destroy. All 100 of terraform_data.r will be replaced because of the changes, but 3 of terraform_data.t are also being replaced.

While it is a shared slice problem here, I think we can just skip the slice modification altogether. A resource instance can only replace itself, and it can only do it once, so there's no reason to track the other addresses at that point. Carrying all the replace addresses along was just a consequence of the old implementation but not actually useful for anything.

[g7r]

Juts to make sure, the expected outcome here would not be 100 to add, 100 to change, 100 to destroy, but rather 103 to add, 97 to change, 103 to destroy. All 100 of terraform_data.r will be replaced because of the changes, but 3 of terraform_data.t are also being replaced.

You are correct. Depending on the addresses in -replace, different results are expected:

• if we use fictional addresses, 100 to add, 100 to change, 100 to destroy is expected
• if we reference only terraform_data.r[???] resources, 100 to add, 100 to change, 100 to destroy is expected
• if we reference any of terraform_data.t[???] resources, other numbers are expected, depending on the number of replaced t resources

[g7r]

While it is a shared slice problem here, I think we can just skip the slice modification altogether. A resource instance can only replace itself, and it can only do it once, so there's no reason to track the other addresses at that point. Carrying all the replace addresses along was just a consequence of the old implementation but not actually useful for anything.

I've submitted a PR that adds a slices.Clone(b.ForceReplace), the simplest fix possible I could think of. I'm not fluent enough with the codebase, so I don't quite get what fix you propose 🙂

[jbardin]

Thanks @g7r, that does solve the shared slice problem between nodes (there's technically no race), and is probably a good idea in general, I'd like to make sure we fix the shared slice issue for instances as well. The instances only need to know if they are being force-replaced or not, and don't need to carry along the extra addresses at that point, so it could just as easily be a bool value.

I made a quick POC here (#37833). That may still warrant further refactoring past my initial find+replace, but the tests passing indicate it's probably equivalently functional. It also needs a test case that fails for the current release, so I'll need to continue that later.

[g7r]

@jbardin, thanks for your effort!