fix error when writing file with empty content #26

kmoe · 2019-05-03T10:24:22Z

Fixes #15

Previously, attempting to write a local file with content of "" would set the resource Id to "", since the Id is based on a SHA1 of the content. This caused the file to be deleted during the apply step.

The resource Id of an empty file is now the SHA1 of the filename concatenated with the SHA1 of the content, so it will never be "".

radeksimko

Do you mind attaching a test case which reproduces this behaviour (i.e. fails without a patch)?

Side note: We have @terraform-providers/enablement alias that can be used when requesting reviews from the whole team 😉

kmoe · 2019-05-03T10:53:08Z

Thanks @radeksimko! Apologies for the premature review request - I'll fix the tests and add a new case for this.

apparentlymart · 2019-05-03T17:22:02Z

local/resource_local_file.go

-	d.SetId(hex.EncodeToString(checksum[:]))
+	filenameChecksum := sha1.Sum([]byte(destination))
+	contentChecksum := sha1.Sum([]byte(content))
+	d.SetId(hex.EncodeToString(append(filenameChecksum[:], contentChecksum[:]...)))


The id isn't really meaningful for this resource type (it gets all of its required information from the other attributes) so if we're going to change it anyway, maybe more straightforward to just set it to some constant value like "-" to make it clearer that it's not significant, and then we can remove it altogether once a later SDK version removes the requirement for having an id attribute. (This requirement is now an SDK-only thing; Terraform Core just treats it the same as any other attribute.)

I'm not sure if this is important, but note that anyone that was referencing local_file.example.id and expecting to get a SHA1 checksum will see this as a breaking change. I think we never documented this as possible, so probably not a big deal, but probably worth mentioning in the changelog once this is merged just in case.

kmoe · 2019-05-15T12:29:29Z

It turns out the issue in #15 no longer causes a problem for the user as of Terraform 0.12: #15 (comment)

Instead, a warning is logged because the plan ends up with a null value for the content attribute where it should have "". The state file also ends up with "content": null. However, since these are considered equivalent in 0.12 for the purposes of SDK diffing (https://github.com/hashicorp/terraform/blob/master/config/hcl2shim/values_equiv_test.go#L39,L41), the user does get an empty file written with no errors.

Fix

I've fixed this issue in c507fd8, and manual tests pass for Terraform 0.11.13 and 0.12 latest.

With this patch, no warning is logged, and the contents of the final state file are valid (content: "") in both 0.11.13 and 0.12.

Tests

I haven't managed to make an acceptance test case that fails before this patch.

Since this provider is already using the 0.12 SDK, the code path followed when running terraform apply under 0.11 is significantly different from that when running make testacc. I don't think any action is required here.

However, the code path followed when running terraform apply under 0.12 involves EvalDiff.Eval, which is what produces the warning (https://github.com/hashicorp/terraform/blob/master/terraform/eval_diff.go#L210). It would be helpful to expose this test in the acceptance test framework for providers. I don't see a way to do this currently but would appreciate feedback from @apparentlymart to check my assumptions here.

In the meantime I've added an acceptance test inhttps://github.com//pull/26/commits/c507fd81316fa8ea99ec9d4498522e3b004c6853 which doesn't currently fail before the patch. I'll update this or at least add a comment after discussion.

apparentlymart · 2019-05-15T18:57:09Z

The test framework calls in to Terraform through terraform.NewContext and should be running all of the same steps as normal CLI usage from that point onwards, so I'm surprised that EvalDiff codepath isn't being exercised. 🤔

I wonder if some other detail about the test framework is changing the outcome here. terraform apply internally runs a validate, a refresh, a plan, and then an apply walk. The test harness follows a similar pattern but does some of the steps multiple times to ensure the provider is being consistent. Off the top of my head I can't think of a reason why that would change the outcome of the particular test cases you added there, but we have seen that be a cause occasionally before, and I don't have any other immediate ideas.

The testing process is implemented in testStep; you can see there it calling a few different operation methods on a terraform.Context. The corresponding main code is in the local backend's terraform apply implementation. You can see there that both of them are calling various methods on the terraform.Context object, but the test harness does a few extra calls.

BrendanThompson · 2019-05-28T06:03:23Z

A point here, there are scenarios wherein one may want to create a file but not care if its edited in the future. With the current implementation it is not possible to do this even with ignore_changes due to the content checksum for the Id.
This change, however, looks as though it will partially solve this issue; however, the resourceLocalFileRead will still nuke the Id if the content doesn't match.
What would you ( @apparentlymart ) recommend here, it is unclear to me as of yet if simply using an ignore_changes on the content attribute will fix this particular scenario now that the Id is no longer reliant on the content hash.

Previously, attempting to write a local file with content of "" would set the resource Id to "", since the Id is based on a SHA1 of the content. This caused the file to be deleted during the apply step. The Id is now set to a constant "-" to make it clear that it is not significant. The ForceNew behaviour for content has been replaced with an Update function for more appropriate semantics.

hashicorp-cla · 2022-03-12T17:05:16Z

All committers have signed the CLA.

kmoe requested review from radeksimko and appilon May 3, 2019 10:24

ghost added the size/XS label May 3, 2019

radeksimko reviewed May 3, 2019

View reviewed changes

kmoe removed the request for review from appilon May 3, 2019 10:53

apparentlymart reviewed May 3, 2019

View reviewed changes

kmoe force-pushed the kmoe/15-replace-file-empty-content branch from fdefcf1 to c507fd8 Compare May 10, 2019 10:25

ghost added size/M and removed size/XS labels May 15, 2019

kmoe requested a review from apparentlymart May 15, 2019 13:29

kmoe added 2 commits February 20, 2020 20:24

add test for empty file content case

6a50f78

kmoe force-pushed the kmoe/15-replace-file-empty-content branch from 202d605 to 6a50f78 Compare February 20, 2020 20:25

kmoe mentioned this pull request Feb 20, 2020

add support for lifecycle ignore_changes #35

Open

Base automatically changed from master to main February 1, 2021 17:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix error when writing file with empty content #26

fix error when writing file with empty content #26

kmoe commented May 3, 2019

radeksimko left a comment

kmoe commented May 3, 2019

apparentlymart May 3, 2019

kmoe commented May 15, 2019

apparentlymart commented May 15, 2019

BrendanThompson commented May 28, 2019

hashicorp-cla commented Mar 12, 2022 •

edited

fix error when writing file with empty content #26

Are you sure you want to change the base?

fix error when writing file with empty content #26

Conversation

kmoe commented May 3, 2019

radeksimko left a comment

Choose a reason for hiding this comment

kmoe commented May 3, 2019

apparentlymart May 3, 2019

Choose a reason for hiding this comment

kmoe commented May 15, 2019

Fix

Tests

apparentlymart commented May 15, 2019

BrendanThompson commented May 28, 2019

hashicorp-cla commented Mar 12, 2022 • edited

hashicorp-cla commented Mar 12, 2022 •

edited