Update terragrunt to generate tfvars.json file instead of using TF_VAR env var #1267

yorinasub17 · 2020-07-26T20:15:30Z

Fixes #752

Extends #1263 to always generate tfvars json file instead of relying on a debug mode.

NOTE: I decided to open a new PR on a new branch so that we have the other branch in case we decide the debug mode is better.

brikis98 · 2020-07-27T08:25:46Z

cli/args.go


-	for varName, varValue := range vars {
-		envVarName := fmt.Sprintf("TF_VAR_%s", varName)
+	variables, err := terraformModuleVariables(terragruntOptions)


Sanity check: is it possible that we're somehow concealing a bug by only generating the .tfvars.json file with the input vars we find?

I guess one possibility is that in terragrunt.hcl, you have a typo: e.g., you set instance_typ = "t3.micro" instead of instance_type = "t3.micro". Terragrunt wouldn't find an instance_typ variable in your module's inputs and therefore wouldn't include it in the .tfvars.json file. I guess this would be a bit confusing during debugging. That said, today, with env vars, you have the exact same problem, but it's less visible.

If we were to output a .tfvars file instead of tfvars.json, we could include a commented out section of variables that were in your inputs, but for which there were no matching input vars. That would make things a lot clearer...

This is valid, but as you mentioned, we already have this problem, and unlike with env vars, you would spot that the instance_type field is missing from the json when you load it. I think the benefit is marginal (e.g., it is just as easy to gloss over comments).

I agree that we should generate tfvars, but in the interest of having something ship sooner, I think we should punt on this to the next iteration. Generating hcl with comments in a robust manner will be complex since AFAIK, you can't just output the cty; you need to manipulate the AST to inject comment tokens.

Roger. Could file issue to track generating a .tfvars in future.

brikis98 · 2020-07-27T08:26:33Z

cli/args.go


-		out[envVarName] = string(envVarValue)
+	fileName := filepath.Join(terragruntOptions.WorkingDir, TerragruntTFVarsFileName)
+	if err := ioutil.WriteFile(fileName, fileContents, os.FileMode(int(0600))); err != nil {


Do we always override the previous file? Is it ever worth checking if the user has a file with such a name there already?

I think we should always override it to avoid the element of surprise for now. I added a warning log when terragrunt detects the file: 6814a3a

When we convert to hcl, I think we should switch to the same behavior as overwrite_terragrunt mode of generate blocks, which is:

Inject a signature as a comment to the generated file.

If file does not exist, generate it.

If file exists and has signature, overwrite it.

If file exists but no signature, error out.

brikis98 · 2020-07-27T08:27:42Z

cli/args.go

-	return out, nil
+	terragruntOptions.Logger.Printf("Variables passed to terraform are located in \"%s\"", fileName)
+	terragruntOptions.Logger.Printf("Run this command to replicate how terraform was invoked:")
+	terragruntOptions.Logger.Printf("\tterraform apply \"%s\"", terragruntOptions.WorkingDir)


It's not always going to be terraform apply, right? E.g., It could be plan, destroy, etc.

Moreover, due to extra_arguments, we pass other args along too... Perhaps we should log the full, exact command we're executing more clearly for users?

Good point. Since we already log the terraform command with the args as part of RunCommandWithOutput, and since I don't have access to the full args passed in at this stage, I decided to remove these logs and update the runcommand args to include the working dir.

6814a3a

brikis98 · 2020-07-27T08:29:41Z

cli/args.go

+		} else if varIsInEnv {
+			util.Debugf(
+				terragruntOptions.Logger,
+				"WARN: The variable %s was omitted from the debug file because the env var %s is already set.",
+				varName, nameAsEnvVar,
+			)
+		} else if !varIsDefined {
+			util.Debugf(
+				terragruntOptions.Logger,
+				"WARN: The variable %s was omitted because it is not defined in the terraform module.",
+				varName,
+			)


If we generated a .tfvars instead of .tfvars.json, we could include a commented-out section that has this exact info.

See above re:my thoughts on tfvars hcl.

docs/_docs/02_features/inputs.md

brikis98

Great work Yori 👍

brikis98 · 2020-07-27T17:32:48Z

cli/args.go

+// writeTFVarsFile will create a tfvars file that can be used to invoke the terraform module with the inputs generated
+// in terragrunt.
+func writeTFVarsFile(terragruntOptions *options.TerragruntOptions, terragruntConfig *config.TerragruntConfig) error {
+	terragruntOptions.Logger.Printf("Generating tfvars file")


NIT: specify what file is being generated (TerragruntTFVarsFileName).

Done: f599a09

brikis98 · 2020-07-27T17:33:31Z

cli/args.go


-	for varName, varValue := range vars {
-		envVarName := fmt.Sprintf("TF_VAR_%s", varName)
+	variables, err := terraformModuleVariables(terragruntOptions)


Roger. Could file issue to track generating a .tfvars in future.

brikis98 · 2020-07-27T17:34:02Z

cli/args.go

+	fileName := filepath.Join(terragruntOptions.WorkingDir, TerragruntTFVarsFileName)
+
+	// If the file already exists, log a warning indicating that we will overwrite it.
+	if util.FileExists(fileName) {


Will this warning appear if you re-run a Terragrunt command and there's a previously generated file in there?

Unfortunately yes, as currently there is no way to distinguish between a file that is user created and a file that is terragrunt generated (since we don't have comments to inject a signature).

To reduce noise, should we only log it as a debug?

Good point: done in c7c4b71

brikis98 · 2020-07-27T17:34:08Z

cli/args.go


-		out[envVarName] = string(envVarValue)
+	fileName := filepath.Join(terragruntOptions.WorkingDir, TerragruntTFVarsFileName)
+	if err := ioutil.WriteFile(fileName, fileContents, os.FileMode(int(0600))); err != nil {


yorinasub17 · 2020-07-30T16:35:32Z

@brikis98

I believe this is ready to merge now, but quick sanity check. When we first visited this idea, you had some reservations in #752 (comment). I have an answer to all but one of your questions (about secrets), which is worth thinking through prior to merging this in:

What file name to use? What if the file exists already? Do we generate a randomly named temp file? What about on subsequent re-runs of terragrunt commands?

The file is named terragrunt-generated.auto.tfvars.json which hopefully is directed enough that a user won't already be using this. Since the name is static, this file will always be regenerated and replaced on each terragrunt run, given that the contents could dynamically change by env var (e.g., TF_VAR_varname would cause terragrunt to omit that var from the json if it is defined in inputs).

In future iterations we can better handle a user provided terragrunt-generated.auto.tfvars.json file by replacing with .tfvars and using the terragrunt_overwrite behavior of code generation.

Do we delete the file after?

No, so that it can be debugged. However, we should consider if that is the right approach given the next bullet:

What if the values in the inputs block are secrets? Writing them to disk may be a bad idea.

**** I don't have a solution for this. Perhaps this is what forces us to delete this file after each run?

What if the user has -var-file arguments already in extra_arguments? Does the generated file go before or after those?

Based on the terraform resolution order, these args will merge into the generated inputs. That is, any var defined in -var-file will override vars defined in terragrunt-generated.auto.tfvars.json. This is validated with the tests.

I believe this was originally asked in the ticket because we were considering passing in the generated tfvars file with -var-file, but since we are using the automatic injection, this point is moot.

What if the user has -var-file arguments already when running terragrunt (e.g., terragrunt apply -var-file=foo? Does the generated file go before or after those?

Same as above bullet.

brikis98 · 2020-07-31T08:22:00Z

Do we delete the file after?

No, so that it can be debugged. However, we should consider if that is the right approach given the next bullet:

What if the values in the inputs block are secrets? Writing them to disk may be a bad idea.

**** I don't have a solution for this. Perhaps this is what forces us to delete this file after each run?

Hm, yea, this is a tough one. Some options to think through:

Could we use an in-memory-only file? This would be by top choice... I'm pretty sure we could make that work on *nix, but I don't know Windows well enough to see if there's a way to do it there. Example approaches:
We could delete the file right after each run. Of course, this makes debugging harder, so we'd need some --debug flag to force the file to stick around. We'd have to be extra careful to ensure that Terragrunt deletes the file even if there's an error or panic. Not much we could do about a forced kill (e.g., via CTRL+C).
If neither of the above works well enough, we go back to env vars, and only write the file when a --debug flag is passed.

yorinasub17 · 2020-08-01T03:34:24Z

I decided to go with option (2). Now the tfvars file is deleted with a defer call, which can be skipped with --terragrunt-debug. 150b34b

We could delete the file right after each run. Of course, this makes debugging harder, so we'd need some --debug flag to force the file to stick around. We'd have to be extra careful to ensure that Terragrunt deletes the file even if there's an error or panic. Not much we could do about a forced kill (e.g., via CTRL+C).

Looking at the docs for panic, it looks like all defer calls are executed normally (emphasis mine):

anic is a built-in function that stops the ordinary flow of control and begins panicking. When the function F calls panic, execution of F stops, any deferred functions in F are executed normally, and then F returns to its caller. To the caller, F then behaves like a call to panic....

So I think as long as the defer is done before the call to writeTFVarsFile (which I am doing), we are guaranteed to run this cleanup routine, which should mean that we will properly clean up the file in the face of error and panic.

brikis98 · 2020-08-01T13:17:49Z

Hm, I still a bit nervous about potentially writing secrets to disk:

terragrunt apply could run for an hour... That's a long time to have plaintext secrets sitting on disk.
For a variety of reasons, the Terragrunt process could be killed before the file is cleaned up. We should prob have tests that validate we clean up the file correctly, including in the face of a panic, but CTRL+C, kill, and a dozen other things would leave behind files on disk even if our code works perfectly.

Is it worth spending 30 min looking for a way on Windows to pass a file purely in memory? I know it can be done on *nix...

yorinasub17 · 2020-08-01T13:30:28Z

We should prob have tests that validate we clean up the file correctly, including in the face of a panic

I don't think this kind of testing is going to be very easy without introducing a potential avenue for bugs in terragrunt (I am nervous about injecting something in the pipeline to induce a panic intentionally)...

Is it worth spending 30 min looking for a way on Windows to pass a file purely in memory? I know it can be done on *nix...

Ok will look into it, but it'll be a while before I'll have time available to set up my dev env on my windows boxes again.

Let's then just revert back to the original debug mode version (option 3) for now so that we can ship something useful to satisfy this need of terragrunt debugging, and we don't block that on my ability to dedicate free nights and weekends to work on this.

yorinasub17 · 2020-08-01T13:43:39Z

UPDATE: rebased on #1263 so that this PR will automatically become an extension of that branch when that is merged, so that we don't throw away the work here.

brikis98 · 2020-08-02T10:28:10Z

We should prob have tests that validate we clean up the file correctly, including in the face of a panic

I don't think this kind of testing is going to be very easy without introducing a potential avenue for bugs in terragrunt (I am nervous about injecting something in the pipeline to induce a panic intentionally)...

Is it worth spending 30 min looking for a way on Windows to pass a file purely in memory? I know it can be done on *nix...

Ok will look into it, but it'll be a while before I'll have time available to set up my dev env on my windows boxes again.

Let's then just revert back to the original debug mode version (option 3) for now so that we can ship something useful to satisfy this need of terragrunt debugging, and we don't block that on my ability to dedicate free nights and weekends to work on this.

Roger

…terraform with that instead of env vars

…R env var

Co-authored-by: Yevgeniy Brikman <[email protected]>

…ars file will happen frequently

abeluck · 2020-09-10T08:18:31Z

Just wanted to drop a comment here thanking you for considering security in this case.

If terragrunt started writing secrets to disk we'd have to immediately stop using it. We've stringent security policies in place that disallow secrets being written to plaintext on disk (even though the workstations/servers are full disk encrypted).... even for a few seconds.

It is common for workstation backup software to run in a continuous mode that constantly backs up changes to disk as they happen. So writing secrets to disk could easily result in them being swept into a backup archive somewhere.

That said, I'm definitely interested in the original motivation of this issue.

lorengordon · 2022-07-03T16:19:54Z

To manage normal values vs secret values, could we maybe use two different inputs blocks, or a function? One input block would always write a tfvars file, the other would always export ENVs. Or check if that value is wrapped in the function, and exclude it from the tfvars and instead export it as an env...

yorinasub17 · 2023-09-30T23:24:05Z

Closing as stale

yorinasub17 requested a review from brikis98 as a code owner July 26, 2020 20:15

yorinasub17 mentioned this pull request Jul 26, 2020

Implement minimal debug feature #1263

Merged

brikis98 reviewed Jul 27, 2020

View reviewed changes

brikis98 previously approved these changes Jul 27, 2020

View reviewed changes

yorinasub17 force-pushed the yori-generatetfvars branch from 150b34b to a169425 Compare August 1, 2020 13:41

yorinasub17 and others added 9 commits August 3, 2020 08:23

Remove debug mode and instead always generate tfvars json and invoke …

3548520

…terraform with that instead of env vars

Update terragrunt to generate tfvars.json file instead of using TF_VA…

2c46f3e

…R env var

Fix test failures

e367392

Add better logs

2685df6

Fix hook check now that workingDir is logged

2a4f37b

Update docs/_docs/02_features/inputs.md

ca64ec0

Co-authored-by: Yevgeniy Brikman <[email protected]>

Adjust log and fix tests

4b71164

Log warning using debug mode given that overwriting the generated tfv…

e35192e

…ars file will happen frequently

Add back in debug mode flag and ensure autogenerated file is removed

533cdd4

yorinasub17 force-pushed the yori-generatetfvars branch from a169425 to 533cdd4 Compare August 3, 2020 13:23

yorinasub17 mentioned this pull request Sep 23, 2020

do not pass null value variables #1367

Open

yorinasub17 mentioned this pull request Mar 9, 2021

Introduce validate-inputs, which can be used to check for variable alignment #1572

Merged

yorinasub17 dismissed brikis98’s stale review via 533cdd4 July 29, 2021 15:24

yorinasub17 mentioned this pull request Sep 27, 2021

Don't convert inputs to env vars. Instead, generate terragrunt.tfvars.json and use as -var-file #752

Open

yorinasub17 closed this Sep 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update terragrunt to generate tfvars.json file instead of using TF_VAR env var #1267

Update terragrunt to generate tfvars.json file instead of using TF_VAR env var #1267

yorinasub17 commented Jul 26, 2020

brikis98 Jul 27, 2020

yorinasub17 Jul 27, 2020

brikis98 Jul 27, 2020

brikis98 Jul 27, 2020

yorinasub17 Jul 27, 2020

brikis98 Jul 27, 2020

brikis98 Jul 27, 2020

yorinasub17 Jul 27, 2020

brikis98 Jul 27, 2020

yorinasub17 Jul 27, 2020

brikis98 left a comment

brikis98 Jul 27, 2020

yorinasub17 Jul 28, 2020

brikis98 Jul 27, 2020

brikis98 Jul 27, 2020

yorinasub17 Jul 28, 2020

brikis98 Jul 28, 2020

yorinasub17 Jul 30, 2020

brikis98 Jul 27, 2020

yorinasub17 commented Jul 30, 2020

brikis98 commented Jul 31, 2020 •

edited

Loading

yorinasub17 commented Aug 1, 2020 •

edited

Loading

brikis98 commented Aug 1, 2020 •

edited

Loading

yorinasub17 commented Aug 1, 2020

yorinasub17 commented Aug 1, 2020

brikis98 commented Aug 2, 2020

abeluck commented Sep 10, 2020

lorengordon commented Jul 3, 2022

yorinasub17 commented Sep 30, 2023

Update terragrunt to generate tfvars.json file instead of using TF_VAR env var #1267

Update terragrunt to generate tfvars.json file instead of using TF_VAR env var #1267

Conversation

yorinasub17 commented Jul 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brikis98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yorinasub17 commented Jul 30, 2020

brikis98 commented Jul 31, 2020 • edited Loading

yorinasub17 commented Aug 1, 2020 • edited Loading

brikis98 commented Aug 1, 2020 • edited Loading

yorinasub17 commented Aug 1, 2020

yorinasub17 commented Aug 1, 2020

brikis98 commented Aug 2, 2020

abeluck commented Sep 10, 2020

lorengordon commented Jul 3, 2022

yorinasub17 commented Sep 30, 2023

brikis98 commented Jul 31, 2020 •

edited

Loading

yorinasub17 commented Aug 1, 2020 •

edited

Loading

brikis98 commented Aug 1, 2020 •

edited

Loading