Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delay filename outputs until files exist #42

Merged
merged 6 commits into from
Jul 4, 2020

Conversation

aaronsteers
Copy link

Resolves #41

Aaron Steers added 2 commits July 1, 2020 18:00
Delay filename outputs until files exist.
@aaronsteers
Copy link
Author

aaronsteers commented Jul 2, 2020

  • Tested successfully in local dev environment.

@brcnblc

This comment has been minimized.

2 similar comments
@jamengual

This comment has been minimized.

@osterman
Copy link
Member

osterman commented Jul 2, 2020

/test all

outputs.tf Outdated
value = local.public_key_filename

# Prevent releasing filename to downstream consumers until file exists (aka not during plan):
value = length(tls_private_key.default[0].public_key_openssh) > 0 ? local.public_key_filename : local.public_key_filename
Copy link
Member

@osterman osterman Jul 2, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronsteers can you explain how this hack works?

On the surface, it looks more or less equivalent to:

value = true ? local.public_key_filename : local.public_key_filename

which is the same as:

value       = local.public_key_filename

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @osterman . Yes, it's very bizarre in terms of readability. Essentially, terraform runs the code twice. The first time (during plan) it follows the code as far as it can before getting blocked. And wherever it gets blocked, it marks anything downstream from that point as (known after apply).

An easy example is if you created some string manipulation based upon the arn (Amazon Resource ID) of a resource that doesn't exist yet - the compiler still checks that more-or-less all syntax rules and data types and valid, but it just doesn't know the text of the ARN yet, so it can't pass the results of the string manipulation downstream. That said, once the apply is performed, the entire code path is traveled again and this time all the known after apply blind spots are resolved.

Essentially, this trick works by hiding the rest of the result behind a calculation it can't evaluate. Terraform is careful not to evaluate code on the untraveled side of a switch (which is why statements like this never cause an error len(mylist) > 0 ? mylist[0] : null) - so even though the then and else statements are exactly the same in this workaround, terraform will not evaluate them because it cannot deterministically figure out which path to travel using static code analysis and the existing state info. Because it can't evaluate if length(tls_private_key.default[0].public_key_openssh) > 0 is true until apply has been executed, it won't pass along the filepath until the file actually exists, aka after apply is run. In contrast, if we simply put them behind a if 1 == 1 gate or similar, that won't block execution because it can be deterministically resolved without further info from the apply.

And most importantly for my use cases: anything tries to read the contents of the public or private key file is going to safely asked to "wait" until the key file exists. Meaning we'll get through plan safely with no errors and then once the files are created it'll still pass those filepath along so downstream objects can read them. 🙌

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Thanks for the extra clear explanation!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronsteers did you test it when var.generate_ssh_key == false and tls_private_key.default[0] does not exist?

resource "tls_private_key" "default" {
  count     = var.generate_ssh_key == true ? 1 : 0
  algorithm = var.ssh_key_algorithm
}

Maybe we could use splat+join which works in all cases:

value = length(join("", tls_private_key.default.*.public_key_openssh)) > 0 ? local.private_key_filename : local.private_key_filename

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aknysh - Good catch! I had not tested with var.generate_ssh_key == false but I see now what you mean, that would throw an error.

Your proposal seems to work well and I've committed that change. I had previously tried unsuccessfully to just do a count of items in that array (the splat without the join), but that didn't work because the count of items was known ahead of time because var.generate_ssh_key was known.

I've tested now both ways - with generate_ssh_key = true and false. No issues observed on my side and testing was successful & as-expected in the two scenarios I tried.

@osterman
Copy link
Member

osterman commented Jul 2, 2020

/terraform-fmt

@aaronsteers aaronsteers requested a review from a team as a code owner July 2, 2020 23:06
@aaronsteers aaronsteers requested review from Gowiem and RothAndrew and removed request for a team July 2, 2020 23:06
@osterman
Copy link
Member

osterman commented Jul 2, 2020

/rebuild-readme

@osterman
Copy link
Member

osterman commented Jul 2, 2020

/test all

osterman
osterman previously approved these changes Jul 3, 2020
@osterman
Copy link
Member

osterman commented Jul 3, 2020

@aknysh any thoughts on this?

Copy link
Contributor

@jamengual jamengual left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very interesting workaround

@jamengual jamengual merged commit f8e37b1 into cloudposse:master Jul 4, 2020
@aaronsteers aaronsteers deleted the patch-1 branch July 6, 2020 16:05
@aaronsteers
Copy link
Author

Awesome - thanks team!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hold for file creation before releasing output variables
6 participants