ImportStateVerify should apply DiffSuppressFuncs

[danawillow]

ImportStateVerify tests import a resource with a given id, and then check all fields in the imported state against an existing resource. However, it does these checks with strict equality, meaning that if a diff would have been suppressed, it still shows up in these tests as a diff. It would be great to be able to suppress those automatically so we don't have to complicate our test logic around checking those fields.

[bflad]

The next version of terraform-plugin-sdk/v2, unreleased at the moment, but slated to be v2.11.0 adds an additional DiffSuppressOnRefresh field to helper/schema.Schema. When enabled, this checks the DiffSuppressFunc during refresh. If the planned value matches the old state value via the DiffSuppressFunc, the old value will be preserved (or put another way, the planned value will be ignored). This should help mitigate cases like these where the DiffSuppressFunc implementations were likely put in place with the expectation that this would already happen in the SDK, however it previously did not.

We expect DiffSuppressOnRefresh to be generally safe to apply in most cases where DiffSuppressFunc is already being used, however it is not enabled by default for backwards compatibility.

The DiffSuppressOnRefresh functionality may also resolve this particular issue, but verification is required and I don't have a quick reproduction handy, so I'm hesitant to immediately close this out.

[apparentlymart]

I'm not entirely sure that DiffSuppressOnRefresh can help in this particular case, because when we're importing we don't start with a configuration and so we don't have a distinct "original value" to preserve. If the import implementation and the refresh implementation disagree about how a particular value is normalized then DiffSuppressOnRefresh would preserve the form the import produced, but since importing and refreshing are typically based on the same API calls anyway I would expect that they would be identically-normalized in most cases.

With that said, I may be misunderstanding the original problem statement here. I think I've never written tests for an import function in particular and so I'm not sure exactly how that test process behaves; perhaps there's something extra going on for that particular testing situation which changes the circumstances so that DiffSuppressOnRefresh could be more helpful. :thinking:

[danawillow]

I filed this FR a while back so I don't have all the same context I did back then, but from what I remember: the Google provider uses ImportStateVerify basically as a way to check that we were writing every field we read from the API back to state. If we didn't, then the test would fail because the "imported" resource wouldn't have the field (since import calls read), but the state would (since it was in the config).

As a contrived simple example, let's say we have a string field, and we don't care about case. In a real life situation, the config could have field = "foo", the API could return field = "FOO", and a DiffSuppressFunc could ignore the case and make this not diff. In a test though, this same situation would show a diff and fail the test because the DiffSuppressFunc doesn't get run. This could be worked around by putting field = "FOO" in the test, but in a more complex situation, it wouldn't match user behavior as well.

Also cc @rileykarson who can probably talk about whether this is still a problem and may even be able to give real examples of trying to work around this.