v0.12 - ImportStateVerify tests failing for attributes where ResourceData.Set isn't called

[alexng-canuck]

Terraform Version

v0.12.0-alpha4.0.20190410151648-50660032848d

Debug Output

Expected Behavior

Some OCI services may omit an attribute in its response to our provider and so the provider avoids calling ResourceData.Set on such computed attributes. (i.e. it shouldn't have a value in state)

ImportStateVerify tests on such resources would result in a pass in v0.11.

Actual Behavior

We now get this error in v0.12. In the expected state, it seems to be checking that some zero-value is present even though we never set one. While the actual state omits these values because we never called into Resource.SetData.

--- FAIL: TestBudgetBudgetResource_basic (916.96s)
    testing.go:568: Step 7 error: ImportStateVerify attributes not equivalent. Difference is shown below. Top is actual, bottom is expected.
        
        (map[string]string) {
        }
        
        
        (map[string]string) (len=2) {
         (string) (len=12) "actual_spend": (string) (len=1) "0",
         (string) (len=16) "forecasted_spend": (string) (len=1) "0"
        }
        
FAIL

Additional Context

References

Possibly related to: #20221

[rileykarson]

We're experiencing this in the Google provider with Removed fields, https://github.com/terraform-providers/terraform-provider-google-beta/blob/master/google-beta/resource_compute_instance.go#L288-L292 and https://github.com/terraform-providers/terraform-provider-google-beta/blob/master/google-beta/resource_cloudfunctions_function.go#L271-L276 so far, and likely more fields. We're partway through a test run.

This is causing every single test with import defined (most of our tests) for these resources to fail.

[rileykarson]

This is also affecting at least one non-Removed field for us: https://github.com/terraform-providers/terraform-provider-google-beta/blob/master/google-beta/resource_container_node_pool.go#L96-L101

[apparentlymart]

Hi all,

This is unfortunately another one of those cases where the emergent behavior of the SDK combined with Terraform didn't line up with the intent, due to the provider plugin architecture in v0.11 and earlier. Terraform v0.11 has no concept of things being "null" or "unset", so the intent of the contract here was that providers must ensure that all attributes in the schema have some sort of value assigned at all times, with default values (or zero values) being used in cases where there is no reasonable value to use instead.

In practice, because Terraform Core hasn't been aware of the resource type schema until now, failing to call d.Set for a computed attribute mostly worked unless the user tried to access the attribute in question via an interpolation expression: Terraform would just save what the SDK told it to save, and then the field reader logic in the SDK was resilient to things being missing in the state on re-read, so the worst symptom of not setting things before was a less-than-ideal error message on expression evaluation.

In v0.12 mode, the SDK shims are using the diff-building logic from the SDK to convert the old-style diff model into a new-style "planned new state". This round-trip is necessarily lossy because the old Terraform "diff" model was itself lossy: it mostly worked because the SDK would insert defaults and fixups on read, but not everything would pass through in full fidelity. The SDK shims are using that same fixup logic we used to rely on for filling in missing information from the diff to construct the "planned new state", and so zero values are now being inserted as part of generating the PlanResourceChange response, rather than being inserted "just in time" after the call to ApplyResourceChange.

The reason for the inconsistency in ImportResourceState here is that this operation doesn't go through the diff model and thus isn't subject to all of the same shimming behaviors that a normal Create operation would be subject to. I did investigate whether we could introduce those same shimming behaviors to the import case, but that would involve a lot of additional logic to construct synthetic diffs, merge them, etc, and I expect such a large change would just push the quirky behavior somewhere else.

---

All of that SDK arcana aside, I don't think there's a safe way for us to address this fully without regressing some other behavior of the shims that is then likely to affect main code rather than just test code. Instead, the safer solution is to change the test code to allow for this difference, thus ensuring that the change won't impact any behavior elsewhere.

I've seen two paths forward so far, in my time spent reviewing the relevant code yesterday:

• The safest possible option, which changes only test code, is to use the IgnoreStateVerifyIgnore field to ignore fields that are not used. This wouldn't be appropriate if the attribute in question is conditionally populated because you'd no longer be testing whether or not it is, but this is appropriate for situations where you know a particular attribute cannot be populated.

  A common reason for that to arise is when an attribute has been removed but is retained only to carry the Removed annotation, so to cater for that case automatically I've proposed helper/resource: Ignore Removed attributes for ImportStateVerify #21019 which treats anything that is Removed as if it were in IgnoreStateVerifyIgnore.

• Failing that, the next best option is to ensure that d.Set is always called for every computed attribute during Read. This is a change to non-test code, but since Terraform v0.11 and the existing SDK don't have formal support for attributes being "unset" anyway: any situation where that was apparent was an abstraction leak, almost always leading to strange bugs elsewhere.

  I don't expect it to have any real impact to behavior as perceived by end-users, except that they'll no longer see the strange error message when accessing something that wasn't set. If there's no "real" value to set, then we can set a suitable zero value for the type following the same principles that the SDK itself would use to apply zero values for diffs: 0 for integers, "" for strings, false for booleans, empty lists/maps/sets, etc.

If we had the luxury of going back to revisit SDK assumptions here we'd either seek to support unset/null values as first-class or ensure that everything always gets set regardless of codepath, but at this point we're in a mode of making pragmatic tradeoffs to minimize risk to existing (implied) assumptions, particularly when running attached to Terraform v0.11. I'm sorry we can't reach 100% compatibility with the existing SDK here; we've done our best to make the right tradeoffs to maximize compatibility with main code, but the test harness exposes a lot more internal details about Terraform's old model that make it harder to hide the situations where the shims have to produce a slightly different result in order to avoid the data loss implied by Terraform's legacy data structures. The follow-on project to improve the SDK to support the new features in Core v0.12 (which includes explicit null) should give us the opportunity to improve on this situation for future work.

If the above workarounds will lead to a significant amount of work for any provider team, please let me know more details about the specific situation and we can see if we can find another tactical solution similar to #21019 where we might be able to reduce some of that workload by test harness changes while avoiding risky changes to the main shim behaviors.

[apparentlymart]

I've heard from a few folks that they saw success employing the strategies I described in my last comment in some different provider situations, so I'm going to close this out for now under the assumption that this workaround is workable.

As before, please let me know if it seems like these workarounds will lead to a significant amount of rework for any particular provider, with some details about the specific situations that are problematic, and we'll see if we can find a similar tactical solution to reduce that impact without risking regressions to non-test code.

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.