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

12.0.0 broke userdata_template_file in worker_groups #882

Closed
1 of 4 tasks
dpiddockcmp opened this issue May 14, 2020 · 9 comments · Fixed by #883
Closed
1 of 4 tasks

12.0.0 broke userdata_template_file in worker_groups #882

dpiddockcmp opened this issue May 14, 2020 · 9 comments · Fixed by #883

Comments

@dpiddockcmp
Copy link
Contributor

I have issues

I'm submitting a...

  • bug report
  • feature request
  • support request - read the FAQ first!
  • kudos, thank you, warm fuzzy

What is the current behavior?

worker_groups key userdata_template_file used to contain the content of a template file. #854 accidentally changed this to being a filename

As reported here: b183b97#r39182495

If this is a bug, how to reproduce? Please include a code sample if relevant.

Try to set worker_groups["userdata_template_file"] to the contents of a userdata file. It'll explode trying to find a file of that name

What's the expected behavior?

Input passed as raw data to be used as the userdata template.

Are you able to fix this problem and submit a PR? Link here if you have already.

Later

Environment details

  • Affected module version: 12.0.0
  • OS: All
  • Terraform version: 0.12.24

Any other relevant info

My bad 😞

dpiddockcmp referenced this issue May 14, 2020
* Remove template_file for generating kubeconfig

Push logic from terraform down to the template. Makes the formatting
slightly easier to follow

* Remove template_file for generating userdata

Updates to the eks_cluster now do not trigger recreation of launch
configurations

* Remove template_file for LT userdata

* Remove template dependency
@mbarrien
Copy link
Contributor

mbarrien commented May 14, 2020

Copying parts of the contents of the issue I had almost written up:

What is the current behavior?

Error: Error in function call

  on .terraform/modules/eks-cluster.cluster/terraform-aws-eks-12.0.0/local.tf line 194, in locals:
 194:   launch_template_userdata = [for worker in var.worker_groups_launch_template : templatefile(
...
Call to function "templatefile" failed: failed to read Content-Type:
multipart/mixed; boundary="MIMEBOUNDARY"
MIME-Version: 1.0

where the Content-Type: ... was the contents of our template file

What's the expected behavior?

The user data is passed in and handled as a template already

Are you able to fix this problem and submit a PR? Link here if you have already.

In theory this is fixable in one of 4 ways:

  1. Revert improvement: Removed dependency on external template provider #854 and parts of chore: move all locals in locals.tf #865, say we forever will use template_file data source.
  2. Keep as is, say that you've change the type of userdata_template_file to be the filename instead of the contents of the template. This breaks ALL use cases where the user is generating a template file dynamically, since according to Terraform docs at https://www.terraform.io/docs/configuration/functions/templatefile.html: This function can be used only with files that already exist on disk at the beginning of a Terraform run.
  3. Reintroduce template_file for this use case but just when the user overrides with userdata_template_file. (This is kind of the worst of both worlds, using both templatefile() function and template_file data source.)
  4. Add a new field userdata_file/change the semantics of userdata_template_file to require that the user pre-render the file with their own template substitutions outside the module (this would work for my org, since we actually use a fixed userdata that doesn't need any of the template substitutions here.)

It's unclear which direction to go, although 1 or a combination of 2 and 4 would probably be sufficient.

Any other relevant info

Given that 12.0.0 also fixes the incompatibility with Terraform AWS Provider 2.61.0 in #868, this is pretty urgent to fix since now we can upgrade neither the provider nor this module in our production cluster until this is fixed.

@dpiddockcmp
Copy link
Contributor Author

I don't see a nice solution to this.

Terraform doesn't have a straight up template() function where you pass it a template and a vars block. I guess they assume you either want to pull stuff from a file or can use here docs. Or are happy with the template provider.

Either we revert #854 and go back to the obnoxious behaviour of the template_file data source or change how some use cases worked 😞

@barryib
Copy link
Member

barryib commented May 15, 2020

Oups. I don't see a nice solution either.

Keep as is, say that you've change the type of userdata_template_file to be the filename instead of the contents of the template. This breaks ALL use cases where the user is generating a template file dynamically, since according to Terraform docs at https://www.terraform.io/docs/configuration/functions/templatefile.html: This function can be used only with files that already exist on disk at the beginning of a Terraform run.

FWIW, after some tests, I figured out that for templatefile function the file should only exist, but if those variables come from outputs of another resource, then Terraform must wait for that resource to apply before reading the file. Terraform magic again 🤷‍♂️

Here is my test:

In main.tf

locals {
  template = templatefile(local_file.template.filename, {
    a = "test",
  })
}

resource "local_file" "template" {
  content  = "foo!"
  filename = "${path.module}/template.tpl"
}

output "a" {
  value = local.template
}

In template.tpl

# Fake template

Results

 ~ terraform apply
local_file.template: Refreshing state... [id=4bf3e335199107182c6f7638efaad377acc7f452]

An execution plan has been generated and is shown below.
Resource actions are indicated with the following symbols:
  + create

Terraform will perform the following actions:

  # local_file.template will be created
  + resource "local_file" "template" {
      + content              = "foo!"
      + directory_permission = "0777"
      + file_permission      = "0777"
      + filename             = "./template.tpl"
      + id                   = (known after apply)
    }

Plan: 1 to add, 0 to change, 0 to destroy.

Do you want to perform these actions?
  Terraform will perform the actions described above.
  Only 'yes' will be accepted to approve.

  Enter a value: yes

local_file.template: Creating...
local_file.template: Creation complete after 0s [id=4bf3e335199107182c6f7638efaad377acc7f452]

Apply complete! Resources: 1 added, 0 changed, 0 destroyed.

Outputs:

a = foo!

That means, we can probably follow the option 2 and write a small doc for users who want to dynamically generate their template. They must create an empty file in their module codebase before the running terraform.

@dpiddockcmp @mbarrien can you confirm these tests ?

@dpiddockcmp
Copy link
Contributor Author

That's really not a safe path. People seem to struggle to read instructions.

It will cause updates each time the plan is run on a clean environment, like a CI system. So for those users wanting to pass in their own template the situation will be even worse.

Updating the comment in locals.tf to say that userdata_template_file now means a file path is a bit rude and stops dynamic generation of the template.

Unfortunately I think we'll have to revert my changes. #883

Maybe I'll file a wishlist item against Terraform. See if they can add a generic template() function.

@mbarrien
Copy link
Contributor

#2 by itself won't work for us; we dynamically generate the contents, and can't point to a single file on disk. (We reuse this userdata generating mechanism for use cases outside of EKS.)

If we add a #4, by creating a new variable called userdata_raw_contents alongside the now-repurposed userdata_template_file we might be able to get away with it. That said, I looked closer and it does appear that my org's current method is relying on this double-templating within the context of EKS; either we need the full revert, or we'd need to be able to pre-calculate the variables that the terraform-aws-eks module would provide to the template so that we can apply that externally ourselves.

@barryib
Copy link
Member

barryib commented May 17, 2020

@dpiddockcmp agree that it's an ugly hack.

Maybe I'll file a wishlist item against Terraform. See if they can add a generic template() function.

To me this is the way to go. I just opened a PR hashicorp/terraform#24978 to add the template function. Let's see where it'll go.

I'm not happy with reverting #854. It adds consistency during plan/apply phases and avoid lot of known after apply issues. Is there any way to improve the "obnoxious random updates" without removing the template_file data source ?

@dpiddockcmp
Copy link
Contributor Author

dpiddockcmp commented May 24, 2020

It looks like the coalescelist trick can solve the issue of updating launch templates when the eks_cluster gets modified. I've pushed an update to #883. Appears to solve my original issue and hopefully makes enough other people happy. But we still have the template provider involved

@dpiddockcmp
Copy link
Contributor Author

The revert of this change has now been released in v12.1.0

@github-actions
Copy link

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants