helper/schema: Do not collapse TypeSet hash values when configuration block contains only Computed attributes

[bflad]

Reference: hashicorp/terraform-provider-aws#7198
Reference: hashicorp/terraform-provider-aws#10045
Reference: hashicorp/terraform-provider-aws#10339
Reference: hashicorp/terraform#22719

When a resource schema contains the following:

"config_block_attribute": {
	Type:     schema.TypeSet,
	Computed: true,
	Elem: &schema.Resource{
		Schema: map[string]*schema.Schema{
			"attribute1": {
				Type:     schema.TypeBool,
				Computed: true,
			},
			"attribute2": {
				Type:     schema.TypeString,
				Computed: true,
			},
		},
	},
},

The TypeSet hash values were previously all collapsed to the zero-value, which meant that multiple set entries were lost. Here we check that all of the attributes are not just Computed: true. If they are all Computed: true attributes, ignore the check for user-defined attributes to compute the hash value.

[radeksimko]

Thanks for the bug fix - this is much cleaner than workarounds proposed in the linked PRs. 😄

The only thing I'd be slightly worried about is where the HashResource function is used and in what context. e.g. We would not want spurious diffs to appear when a hash of a set item changes because a computed value has changed.

For that reason I'd like to double check with @hashicorp/terraform-core 👇

Can we be reasonably sure that the serialization logic is only used to guarantee uniqueness and not for diffing? I looked briefly at Set.HashEqual and Set.Equal, but I could not really trace the code using this + I reckon schema.HashResource can be used by providers themselves, which leaves me unsure and a bit confused.

@bflad If we get the 2nd 👍 I'd like to run all (or majority) of AWS acceptance tests with this patched SDK and check for any anomalies, due to the (perceived) sensitivity of this part of the codebase.
If we see no anomalies caused by this patch then I'm happy to merge it.

[apparentlymart]

Unfortunately I think this is one of those things where the answer differs depending on whether we're talking protocol version 4 or 5.

In protocol version 4 (and thus Terraform 0.10/0.11), Terraform Core has no awareness of the idea of a set at all and just sees this as a flat map of key/value pairs. However, working in our favor is that Terraform 0.10 and 0.11 generally just trust entirely whatever diff is produced by the provider, because in protocol 4 the response is the diff. Therefore, as long as the provider (or rather, the SDK) is consistent with itself in how it interprets the prior state, this could be fine. I believe the legacy diffing logic uses the values for diffing and ignores the hash segments in the keys, but I'm not 100% sure so it'd be good to test it.

In protocol version 5, the result of PlanResourceChange is a planned new state, not a diff. (Terraform's CLI layer produces the visual diff at render time.) The hash values are an SDK implementation detail in this case, and are discarded by the shim layer before returning data to Terraform Core. Since the protocol 5 shim layer is required to re-constitute all of the flatmap hash keys for each new request, I believe it's essentially forced to be self-consistent: it constructs both the flatmap prior state and the flatmap proposed new state at the same time and then runs the legacy diffing logic between the two. I would expect both of them to get constructed using the current hash logic, because the hashes returned from the previous apply operation are not persisted in the state anyway.

There were some particularly messy cases when we were working through the shim behaviors during 0.12 development that I think we should be careful to test the behaviors of here:

• When the nested schema.Resource for a Computed schema is empty, both when the container is a list and when it's a set (which lead to slightly different shimming logic). This situation is often an edge case for decisions like this "is everything computed?" because there's no "everything" to decide based on.
• When the container is a set and everything inside the nested schema.Resource is computed. In that case, we'll need to make sure the set implementation doesn't end up collapsing everything into a single element for comparison even when there are values set.
• Situations where we're performing an update and the update changes the value of one of the nested computed attributes, either at plan time or at apply time. The shim layer has a few cases where it has to create a temporary legacy diff using the old logic in order to replicate old behavior, and changes to the prior state values for computed attributes had a tendency to throw it off.

[radeksimko]

@bflad Do you mind adding tests for the 3 cases Martin mentioned above? Alternatively you can leave it to me.

Assuming these tests are all passing and don't "uncover any skeletons" 😄 I would then continue by running AWS acceptance tests and ideally one more major provider, and if these pass then I'm happy to merge it.

[paultyng]

Its probably safest to just merge this to v2, not sure the impact of merging to v1, do either of you @bflad / @radeksimko have thoughts on that?

[bflad]

@paultyng I think the AWS Provider folks are fine with this being in v2, since we will need to run the full testing anyways just to be safe.

[radeksimko]

👍

[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.