Output variable refresh bug

[bengesoff]

• Output variable refresh bug: I added a CustomizeDiff function to the service resource to mark the cloned_version and active_version attributes as recomputed when any of the other attributes change (with the exception of name, comment and version_comment). This solves the problem where an output variable referencing these attributes wouldn't be updated after an apply and would require another apply or refresh for this to propagate through.
• Removed StateFuncs from all of the logging blocks as these were causing unnecessary whitespace diffs for the CustomizeDiffs and forcing the tests to fail with non-empty plans (the version was being marked as recomputed). I couldn't find any adverse effects from doing this, but would appreciate some confirmation here as there were a lot of comments about a known issue and there might have been a good reason that they were added in.

[bengesoff]

Rebased onto master, just going to retest

[bengesoff]

Full acceptance test suite passing for me ✅

[Integralist]

LGTM but going to request another Terraform SME who has historical context to take a look as well.

[mccurdyc]

Hmm it's hard to remember why we did this, trying to find context:

• Armored PGP Key for Tests and SFTP, BigQuery use trimSpaceStateFunc #268

[Integralist]

This was discussed internally.

@bengesoff will attempt to replicate issue with logging blocks, and if is unable to replicate we'll be OK proceeding with the understanding that this PR will include extra documentation to explain to users how to trim whitespace for their keys if this indeed does bubble back up as an issue in the future.

[bengesoff]

Update from my logging whitespace investigations:

As feared, the API handling of whitespace in the PEM blobs doesn’t appear to be consistent. It appears that at least Elasticsearch and Kafka logging both strip the whitespace off, whereas BigQuery and others preserve whatever is given to them. This means that the trimSpaceStateFunc does have some effect, but only in a small proportion of the cases in which it’s used.

The result of this being that we essentially have three options:

• Stick with the fix of deleting the StateFuncs everywhere and document the need for trimspace() if the strings have whitespace
• Keep the StateFuncs in and close the PR, which will leave the output variable refresh bug at large (due to CustomiseDiff not being an option in this case)
• Pass on the inconsistency to the relevant team at Fastly responsible for these APIs and get the best of both worlds

[bengesoff]

In addition to the first bullet point of my previous comment: we could also augment the documentation with a ValidateFunc to ensure that trimspace() has been used, e.g.

func validateStringTrimmed(value interface{}, p cty.Path) diag.Diagnostics {
	if value != strings.TrimSpace(value.(string)) {
		return diag.FromErr(p.NewErrorf("string must not contain trailing whitespace"))
	}

	return nil
}

This might be a good alternative to removing the StateFunc altogether as it will lead to a good error message.

[Integralist]

Hi @bengesoff

I was discussing this in more detail with @phamann and we came to the conclusion that regardless of what we do in the Terraform provider, the backend APIs need to be consistent by trimming excess whitespace.

Additionally Patrick wondered if we could maybe utilise DiffSupressFunc (instead of ValidateFunc) and have the diff function check a trim of the relevant cert/key attribute, i.e. it's comparing a trimmed version of the HCL input against a trimmed version of the state stored value, so we would only show a diff if there was actually a difference.

But again, we'd still need the data to be stored Fastly's side trimmed.

Let me know you thoughts.

[bengesoff]

Hi @Integralist

Just to clarify, you guys reckon the direction is to always trim rather than never trim the whitespace?

I did try the DiffSuppressFunc actually, with the same idea but unfortunately it won't work. It comes back to the TypeSet problem where TF thinks the whole block is being created/deleted 😞 certainly an attractive idea though as otherwise we'll still have to push the burden onto the user

[Integralist]

@bengesoff OK no worries, thanks for that feedback.

re:

always trim rather than never trim the whitespace?

The thinking here is that only the BigQuery and GCS endpoints are different, and so adding trimming to those endpoints is a smaller blast radius than the reverse which would be removing trimming from everywhere. If the APIs do this then it would mean we'd use your ValidateFunc suggestion and have this change as part of a v1 release as this change would be 'breaking' in the sense that we're now forcing the requirement to trim the content back onto users where before it was handled internally by the provider.

[bengesoff]

That makes sense, fair enough! Are you waiting for a final decision on this still?

[Integralist]

That makes sense, fair enough! Are you waiting for a final decision on this still?

Yup! There's an internal ticket that I'm following to see how this progresses. I'll let you know whenever we have an update.

[Integralist]

@bengesoff FYI - might not affect this PR but the master branch is now named main.

[Integralist]

This PR should now be safe without having to wait for our internal API work (thanks to #439).