Fix handling of ignore_changes

[jbardin]

Ignore changes was intended to only ignore changes happening within the configuration, and should not be applied to values set by the provider. We were previously re-applying this to the planned value for compatibility, but intended to remove the patch in a major release version.

It also turned out that we should have been applying ignore_changes directly to the configuration, rather than the proposed value created for the plan. Doing this ensures that the configuration used for comparison internally, the proposed value, and the configuration passed to the provider all show the same starting data minus the ignored changes. Without this change, because providers are required to create their plan based on the proposed value, AssertPlanValid would always show errors when comparing the planned value against the config, though they are currently only reported as warnings for legacy providers.

While there still are many documented cases in the providers' docs, ignore_changes is not something that providers can use to prevent changes to their configured values. In future versions of providers that are no longer in the legacy protocol exception, altering configuration values will be a hard error, so future providers cannot intentionally cause "drift" to configured values, and ignore_changes would not be useful in that configuration in the first place.

This PR also adds some basic validation to the ignore_changes argument, preventing users from setting references to arguments not set in the configuration, which prevents the case of mistakingly trying to ignore changes to computed values.

The following configuration for example will result in the error below:

resource "null_resource" "a" {
  lifecycle {
    ignore_changes = [id]
  }
}

Error: Cannot ignore argument not set in the configuration

  on main.tf line 3, in resource "null_resource" "a":
   3:     ignore_changes = [id]

The ignore_changes argument is not set in the configuration.
The ignore_changes mechanism only applies to changes within the configuration,
and must be used with arguments set in the configuration and not computed by
the provider.

[codecov]

Codecov Report

Merging #26387 into master will increase coverage by 0.00%.
The diff coverage is 78.94%.

Impacted Files	Coverage Δ
terraform/eval_diff.go	68.75% <68.42%> (+0.17%)	⬆️
terraform/eval_validate.go	56.62% <88.88%> (+2.88%)	⬆️
terraform/provider_mock.go	80.25% <100.00%> (ø)
internal/providercache/dir.go	66.66% <0.00%> (-6.25%)	⬇️

[mildwonkey]

👍 Change looks great! I'll approve this now while you decide if it's going to cause any problems for current providers 😉

[mildwonkey]

I extra-approve this part, thank you ❤️

[jbardin]

I've confirmed that the primary use case for providers using ignore_changes is to prevent correcting drift in the resources. There are a number of resources that have fields requiring "initial values", which may change during the lifetime of the resource. In some cases the user may want to enforce the configuration value, and other they may want to allow the external updates to take precedent.

There are 2 cases in the normal plan+apply cycle which need to be covered to allow this type of drift:

• The resource is refreshed, saving a new value to the ignored field -- In this case everything proceeds normally, as terraform only see it as a change in configuration against state which is correctly suppressed by ignore_changes.
• The resource is applied with one value, and returns another (i.e. "drift" happens between plan and apply) -- This is an error from the provider, however providers using the legacy SDK are exempt from this and will only log warnings.

One hypothetical case where this would cause additional errors is when the provider is altering the plan itself (which would be done through CustomizeDiff in the legacy SDK), assigning a value that does not match the configuration or the refreshed state. The chances of this happening appear quite low, and seems like we could update the resource behavior to preserve user experience if required.

Since the errors in the typical cases above could only be generated during apply, and existing providers are exempt from these errors, we should be able to update the internal behavior while also giving providers time find solutions to handling fields.

[ghost]

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.