Extra feedback for a variety of "move" consequences and edge-cases

[apparentlymart]

Although in most cases Terraform's handling of the new moved blocks should be straightforward and intuitive, there are inevitably some less common cases where the behavior may be surprising enough to warrant further explanation.

This set of changes aims to add a first round of explanations of that sort, focused on the following situations:

• Colliding moves, where two objects try to move to the same address in spite of our basic static validation rules.

  This can typically apply only when someone's previously used terraform state mv or terraform apply -target=... to create a situation that wouldn't be reachable through the standard workflow.

  However, there is at least one situation where it's reachable even in the standard workflow: if we need to resolve both module.foo moves to module.bar and aws_instance.a moves to module.bar.aws_instance.a then both of those moves would be considered structurally valid, but if our prior state already had an object at module.foo.aws_instance.a then the two aws_instance.a objects would conflict with one another to try to occupy the address module.bar.aws_instance.a.

  This is not a situation we expect folks to reach regularly, and so one of the changes here adds an additional warning message to the plan result in that unusual case, which is intended to help explain why Terraform is planning to destroy an object that the user might have expected to be moved to a new address which is still declared in configuration. We don't have enough information to derive a good sense of user intent in that situation, and so the warning just makes a general observation that you can use terraform state subcommands to make things less ambiguous if you're not satisfied with the resolution Terraform proposed.

• Moving to an address that isn't declared in the configuration.

  The main expected use of moved blocks is to set the to argument to refer to an address which has a corresponding declaration in the configuration. In that case, the happy path is for Terraform to simply pretend that an existing object had originally been created at the new address and not propose any other changes at all.

  However, because moved blocks are a historical record that will (in most cases) remain in modules indefinitely, we do need to allow for a later version of the module possibly removing the declaration of that object. Our rules for moves are that we should always generate the same effect for two actions handled together in the same plan as we would for those actions separated into two runs, and so therefore we must tolerate moving into an undeclared address and plan to delete any object that moved into that undeclared address.

  An unfortunate consequence of this rule is that if an author makes a mistake when writing the to address then Terraform will typically propose to destroy the object. As a compromise then, the changes here add some additional context to a planned destroy to help draw attention to the mismatch between configuration and state.

  Although the motivation to do this now was to give better feedback about incorrect moved statements, adding this additional feedback for planned destroy has been on my wishlist for a while now, and so I implemented it in a generic way which works even in the normal case where there's no moving going on. I hope that this will be useful to help users disambiguate the several different reasons that can cause Terraform to declare a particular resource instance as an "orphan" (no longer declared in configuration) and thus figure out what they need to change in the configuration if they don't want to delete the object.

While I was in the area anyway, I also made some minor adjustments to how we present the extra note about a resource instance having moved, which we'd planned to do in an earlier PR but accidentally overlooked it.

The following example output shows a rather gnarly combination of these situations all together, just for illustrative purposes. In practice I would expect it to be extremely rare to see all of these appear together in the same proposed plan. (I created the conflict between null_resource.a[0] and null_resource.a through some malicious use of terraform state mv prior to requesting this plan.)

Terraform will perform the following actions:

  # null_resource.a[0] will be destroyed
  # (because resource does not use count)
  - resource "null_resource" "a" {
      - id = "6362033012182345903" -> null
    }

  # null_resource.d will be destroyed
  # (because null_resource.d is not in configuration)
  # (moved from null_resource.c)
  - resource "null_resource" "d" {
      - id = "7027155152011416384" -> null
    }

Plan: 0 to add, 0 to change, 2 to destroy.
╷
│ Warning: Unresolved resource instance address changes
│ 
│ Terraform tried to adjust resource instance addresses in the prior
│ state based on change information recorded in the configuration,
│ but some adjustments did not succeed due to existing objects
│ already at the intended addresses:
│   - null_resource.a[0] could not move to null_resource.a
│ 
│ Terraform has planned to destroy these objects. If Terraform's
│ proposed changes aren't appropriate, you must first resolve the
│ conflicts using the "terraform state" subcommands and then create a
│ new plan.
╵

---

Making the implicit zero-to-none and none-to-zero key moves more visible in the UI here drew attention to a quirk of my earlier work in #29605, in the situation of moving from for_each to count rather than for_each to no repetition. I also fixed that while here, because otherwise the reason given for proposing to delete the old object didn't actually make sense.

[alisdair]

When adding new reasons here, I think we should also update the streaming JSON UI output to match:

terraform/internal/command/views/json/change.go

Lines 70 to 94 in c8cd0b1

	const (
	ReasonNone ChangeReason = ""
	ReasonTainted ChangeReason = "tainted"
	ReasonRequested ChangeReason = "requested"
	ReasonCannotUpdate ChangeReason = "cannot_update"
	ReasonUnknown ChangeReason = "unknown"
	)
	func changeReason(reason plans.ResourceInstanceChangeActionReason) ChangeReason {
	switch reason {
	case plans.ResourceInstanceChangeNoReason:
	return ReasonNone
	case plans.ResourceInstanceReplaceBecauseTainted:
	return ReasonTainted
	case plans.ResourceInstanceReplaceByRequest:
	return ReasonRequested
	case plans.ResourceInstanceReplaceBecauseCannotUpdate:
	return ReasonCannotUpdate
	default:
	// This should never happen, but there's no good way to guarantee
	// exhaustive handling of the enum, so a generic fall back is better
	// than a misleading result or a panic
	return ReasonUnknown
	}
	}

I'll submit a PR for these changes shortly. Can you think of any automated way to keep these two enums in sync for future changes? Maybe adding nishanths/exhaustive as a CI check?

[alisdair]

#29649

[github-actions]

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.