hashicorp / terraform-provider-google

Terraform Provider for Google Cloud Platform
https://registry.terraform.io/providers/hashicorp/google/latest/docs
Mozilla Public License 2.0
2.27k stars 1.72k forks source link

Fit our state-setting model to the protocol #4328

Open paddycarver opened 5 years ago

paddycarver commented 5 years ago

In learning more about the Terraform protocol, it came up that when a value exists in a Terraform config, only two possible values can be set in that field's state:

This applies even when a value is computed, and failure to adhere to this may break if the field changes and is interpolated into another resource, as the plan will not match what was applied. In our current SDK, we're mostly getting away with this. In the next SDK, we will not get away with this. So we should correct this for 3.0.0.

We mostly do this in areas where we use self_links or other values that have multiple, semantically equivalent forms. The proposed strategy we should be following is to do this canonicalization when setting the value in state. Basically, when we're about to update a value in state, check if it's semantically equivalent to the value in the config or in state, and not change it if it is. We should still DiffSuppress (to avoid two equivalent representations causing a spurious diff) but this should keep us in line with the Terraform protocol's expectations.

slevenick commented 4 years ago

This may cause issues around TypeSets during import that require specifying custom hash functions.

If we have a field that is either a self link or a name in a set, it is impossible to ensure it is set to the config value because we don't have access to the config during import. This means during import we will set the value to whatever comes from the API, causing a diff with the config. This could be fixed with a DiffSuppressFunc, but because we are in a TypeSet, the hashes will be different, still causing a diff. We can fix it with both a custom hash function and a DiffSuppressFunc

rileykarson commented 4 years ago

This fell out of scope of 3.0.0. Added to 4.0.0 to ensure it's tracked for that ~next year, although it's likely we can do it without a breaking change spread over normal releases.