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

lang/funcs: add template function #24978

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

barryib
Copy link

@barryib barryib commented May 17, 2020

The templatefile function is great way ton render template in a consistent way. But so far, we don't have the ability to render dynamic templates in the same way and are forced to use the template_file data source.

Here is our use case terraform-aws-modules/terraform-aws-eks#882 (comment).

This template (we can recall it templatestring if needed) function is more generic and take a template as a string and try to render it.

template("localhost:$${port}" { port = "8080" }))

# Generate from dynamically generated template
template(var.content_of_your_dynamic_template, { port = "8080" }))

It can work with the file function and will behave like the templatefile

template(file("backend.tpl"), { port = "8080" })

Maybe, this can replace the templatefile function ? Or at least, we can have both.

Related links

@hashicorp-cla
Copy link

hashicorp-cla commented May 17, 2020

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented May 17, 2020

Codecov Report

Merging #24978 (d86c215) into master (be26315) will increase coverage by 0.03%.
The diff coverage is 88.67%.

Impacted Files Coverage Δ
lang/funcs/string.go 90.16% <88.00%> (-9.84%) ⬇️
lang/functions.go 93.70% <100.00%> (+0.15%) ⬆️
dag/marshal.go 53.42% <0.00%> (-1.37%) ⬇️
terraform/evaluate.go 52.89% <0.00%> (-0.42%) ⬇️
terraform/node_resource_apply_instance.go 75.78% <0.00%> (+0.78%) ⬆️
terraform/eval_diff.go 67.43% <0.00%> (+0.91%) ⬆️

@apparentlymart
Copy link
Contributor

Hi @barryib! Thanks for working on this.

Not having dynamic templates was an intentional designed decision for templatefile after seeing how much confusion and frustration the design of the template_file data source had caused: folks would frequently get the double-escaping wrong and then be unsure how to interpret the error messages that would result due to them being returned at the Terraform layer instead of at the template rendering layer.

For that reason, I feel reluctant to add this as a core Terraform Language feature: while there are certainly some unusual situations where such a thing can be useful, having a prominent built-in language function often makes something appear to be a recommended practice rather than a pragmatic compromise.

I'd be curious to see if you can solve your original problem using the template_file data source instead. Although it's still there mostly for Terraform 0.11 forward-compatibility (so that 0.11 users can upgrade without a drastic rework of their template handling), we've also noted that its continued existence has been a reasonable answer to unusual situations where dynamic template rendering is needed, at least in all of the situations I've encountered while helping folks so far.

If you'd be willing, I'd also be curious to hear more about your use-case that benefits from dynamic template rendering, separately from how that might be solved. I've heard a few reasonable use-cases already, but it's always useful to hear the motivation for less-common requests to see if they might be addressed in a different way in future.

Thanks again for working on this!

@barryib
Copy link
Author

barryib commented May 19, 2020

Hi @apparentlymart Thanks for your answer.

Not having dynamic templates was an intentional designed decision for templatefile after seeing how much confusion and frustration the design of the template_file data source had caused: folks would frequently get the double-escaping wrong and then be unsure how to interpret the error messages that would result due to them being returned at the Terraform layer instead of at the template rendering layer.

I'm know getting some explanation about why we don't just have a template function which can be used with the file function if need. This would be more generic and would meet all needs.

If you'd be willing, I'd also be curious to hear more about your use-case that benefits from dynamic template rendering, separately from how that might be solved. I've heard a few reasonable use-cases already, but it's always useful to hear the motivation for less-common requests to see if they might be addressed in a different way in future.

I'm one of maintainers of terraform-aws-eks and we recently took the move to remove the template_file datasource. The template_file datasource was causing a lot of unexpected changes during plan phase.

As @dpiddockcmp mentioned in terraform-aws-modules/terraform-aws-eks#854

  • the resource would be marked as "unknown" in the Refresh stage
  • the data_template_file userdatas could not be refreshed and so were in an unknown state during Plan stage
  • thus generating unnecessary pending recreation of launch config/templates

Furthermore, in the template provider, there is a recommendation using the template function.

For Terraform 0.12 and later, the template_file data source has been superseded by the templatefile function, which can be used directly in expressions without creating a separate data resource.

Using the template function solves all those issue and we got more confident in our terraform plan. I'm feeling reluctant to go back to the template data source. But with that move, we broke all dynamic template rendering.

In the terraform-aws-eks module we allow users to provide their own template for the userdata. Some of them (like @mbarrien) need to generate some part of that userdata dynamically (gather components created by another team) before passing it to the module.

So I will happy to use the template file datasource, but so far, it sounds like there isn't the recommended way to deal with template in Terraform 0.12 and it adds noise during refreshes.

Maybe @mbarrien or @dpiddockcmp want to add more inputs.

@apparentlymart
Copy link
Contributor

Thanks for sharing those additional details, @barryib.

If I'm understanding correctly the issue you had with template_file, I think it may be addressed by the change in #24904 which is planned to be in the forthcoming Terraform 0.13. That change will allow data sources to be read during the plan phase as long as there's enough information during plan to fully evaluate their configuration. That requirement (that the arguments be known at plan time) is the same as the constraint on the evaluation of the templatefile function during planning, and would be the same as for your new template function too.

With that said, I'd like to wait and see how the situation changes with #24904 merged. If that resolves the problem of Terraform pessimistically marking the template result as unknown during planning then I expect we'd choose to resolve this by changing the recommendation in the template_file docs to make an exception for dynamic template rendering, for the reasons I mentioned in my earlier comment.


Thinking specifically about the use-case of rendering a user_data string, there is another approach here that I might offer as a way to leave the preparation of that user_data string entirely up to the calling module, where the caller can decide whether to use templatefile or construct the string any other way they like:

First, I'd add an output value to the module that includes all of the same data you were passing to vars on the template_file data source, like this:

output "user_data_vars" {
  value = {
    for worker in var.worker_groups : worker.name => merge(
      {
        platform            = lookup(worker, "platform", local.workers_group_defaults["platform"])
        cluster_name        = aws_eks_cluster.this[0].name
        endpoint            = aws_eks_cluster.this[0].endpoint
        cluster_auth_base64 = aws_eks_cluster.this[0].certificate_authority[0].data
        pre_userdata = lookup(
          worker,
          "pre_userdata",
          local.workers_group_defaults["pre_userdata"],
        )
        additional_userdata = lookup(
          worker,
          "additional_userdata",
          local.workers_group_defaults["additional_userdata"],
        )
        bootstrap_extra_args = lookup(
          worker,
          "bootstrap_extra_args",
          local.workers_group_defaults["bootstrap_extra_args"],
        )
        kubelet_extra_args = lookup(
          worker,
          "kubelet_extra_args",
          local.workers_group_defaults["kubelet_extra_args"],
        )
      },
      lookup(
        worker,
        "userdata_template_extra_args",
        local.workers_group_defaults["userdata_template_extra_args"]
      )
    )
    ) if var.create_eks
  }
}

You can then define an input variable for passing in user data for each worker:

variable "worker_user_data" {
  type        = map(string)
  description = "A map from worker names as defined in worker_groups to user data strings. Use values from the output value user_data_vars to construct these dynamically."
}

Your users can then produce the values from worker_user_data using any string manipulation they want, including both inline templates, separate template files, hard-coded strings, or whatever makes sense for their use-case. I'll use templatefile here for example because I expect that'd be the common case:

module "eks" {
  source = "terraform-aws-modules/eks/aws"

  # ...
  worker_user_data = {
    for name, vars in module.eks.user_data_vars :
    name => templatefile("${path.module}/eks_user_data.tpl", vars)
  }
}

If the change in #24904 ends up addressing the problem here then this would be moot and unnecessary, but I'm just including this here for completeness now so that other future readers of this can see some of the different options on the table to address this use-case.

@barryib
Copy link
Author

barryib commented May 20, 2020

@apparentlymart Indeed #24904 would improve the template_file datasource consistency in Terraform 0.13. Thanks for sharing this information.

With that said, I'd like to wait and see how the situation changes with #24904 merged. If that resolves the problem of Terraform pessimistically marking the template result as unknown during planning then I expect we'd choose to resolve this by changing the recommendation in the template_file docs to make an exception for dynamic template rendering, for the reasons I mentioned in my earlier comment.

I can see that #24904 is now merged. Did you get time to test it ?

But I'm still thinking that a it worth adding a generic template function which can be used for file contains or dynamic one. With all the recent improvement you've made in the templatefile (this PR duplicates the templatefile) function to give more feedback (understandable errors) to users, I think we can say the debug is now really easy with even the line and char number where is error is coming from.

Furthermore, the double escaping you're referring to will be used for only inline (with here docs) template. Users will probably do that, but for only a small (few lines) template.

Maybe we should let user decide what they want to use and in that way, I think we can just add template function and add a note saying that the best practice is to put the template in a file (template(file("filename.tpl"), vars)).


As for the workaround you shared, I would probably help in theory. But when I tried it, Terraform complains for Cycles.

Error: Cycle: module.eks.local.launch_template_userdata_vars, module.eks.output.workers_launch_template_user_data_vars, module.eks.var.worker_groups_launch_template

In your example, you use the module output as input for the same module.

module "eks" {
  source = "terraform-aws-modules/eks/aws"

  # ...
  worker_user_data = {
    for name, vars in module.eks.user_data_vars :
    name => templatefile("${path.module}/eks_user_data.tpl", vars)
  }
}

Any suggestion to solve this ?

Again, Thanks a lot for your time and your comments. They really help to move forward ❤️

@apparentlymart
Copy link
Contributor

apparentlymart commented May 20, 2020

I have not yet tested #24904 with your module and likely will not have time to in the very immediate term, because that PR belongs to a project that other members of the team are working on and so my development focus is currently elsewhere. However, if you'd like to test yourself in the meantime you could potentially build Terraform yourself from the master branch, or if you wait a few weeks then Terraform 0.13.0-beta1 will be released and you could test against that.


That cycle error suggests that some of the values included in the user_data_vars output depend (directly or indirectly) on whatever resource the user_data is eventually populated into. I don't have a good enough mental map of how that module works to make a specific suggestion to solve it right now, but there ought to be some way to make it work because otherwise you would've had the same cycle between objects within the module before as well.

The requirement here is that the value expression for output "user_data_vars" should not refer to anything that is derived from var.worker_groups_launch_template. Modules themselves are not nodes in the dependency graph -- the individual outputs and variables are -- so a module block with an argument referring back to itself is valid as long as there is no existing dependency between the output value(s) and input variable involved.

@barryib
Copy link
Author

barryib commented May 20, 2020

I'll wait Terraform 0.13.0-beta1 for tests. But it'll be for my personal knowledge, because users are still far from using it in production.

The requirement here is that the value expression for output "user_data_vars" should not refer to anything that is derived from var.worker_groups_launch_template. Modules themselves are not nodes in the dependency graph -- the individual outputs and variables are -- so a module block with an argument referring back to itself is valid as long as there is no existing dependency between the output value(s) and input variable involved.

Unfortunately, we can't fulfill that requirement.

var.worker_groups_launch_template is a map provided by users to define their worker groups (with launch template) and some vars for templating are computed from that.

In the module

locals { 
 launch_template_userdata_vars = [for worker in var.worker_groups_launch_template : merge(
    {
      platform            = lookup(worker, "platform", local.workers_group_defaults["platform"])
      cluster_name        = aws_eks_cluster.this[0].name
      endpoint            = aws_eks_cluster.this[0].endpoint
      cluster_auth_base64 = aws_eks_cluster.this[0].certificate_authority[0].data
      pre_userdata = lookup(
        worker,
        "pre_userdata",
        local.workers_group_defaults["pre_userdata"],
      )
      additional_userdata = lookup(
        worker,
        "additional_userdata",
        local.workers_group_defaults["additional_userdata"],
      )
      bootstrap_extra_args = lookup(
        worker,
        "bootstrap_extra_args",
        local.workers_group_defaults["bootstrap_extra_args"],
      )
      kubelet_extra_args = lookup(
        worker,
        "kubelet_extra_args",
        local.workers_group_defaults["kubelet_extra_args"],
      )
    },
    lookup(
      worker,
      "userdata_template_extra_args",
      local.workers_group_defaults["userdata_template_extra_args"]
    )
    ) if var.create_eks
  ]
}

output "workers_launch_template_user_data_vars" {
  description = "User data template vars of worker launch template groups"
  value       = local.launch_template_userdata_vars
}

In the root module

module "eks" {
  source       = "terraform-aws-modules/eks/aws"
  # ...

  worker_groups_launch_template = [
    {
      name                 = "worker-group-1"
      instance_type        = "t2.small"
      asg_desired_capacity = 2
      public_ip            = true
      # This are module.eks.workers_launch_template_user_data_vars passed by the module to it's template
      # Use this to create/expand your template whatever you like
      # For the example, we use `templatefile`, but you can use what you want
      userdata_template_raw = templatefile("${module.path}/custom_userdata.sh", module.eks.workers_launch_template_user_data_vars)
    },
    {
      name                 = "worker-group-2"
      instance_type        = "t2.medium"
      asg_desired_capacity = 1
      public_ip            = true
      # This are module.eks.workers_launch_template_user_data_vars passed by the module to it's template
      # Use this to create/expand your template whatever you like
      # For the example, we use `templatefile`, but you can use what you want
      userdata_template_raw = templatefile("${module.path}/custom_userdata.sh", module.eks.workers_launch_template_user_data_vars)
    },
  ]
}

So if I summarize

It sounds like there is no way for us to avoid using the "flaky" template_file datasource unless a function like template (or something else) is added ?

For you, it's a no go for this PR or any other proposition to add function to render string template ?

@barryib
Copy link
Author

barryib commented Sep 22, 2020

Hello @apparentlymart, it sounds like the template_file data source is now archived https://github.com/hashicorp/terraform-provider-template and superseded by the templatefile function.

With that we loose the ability dynamically generate templates (because the template file must be present before terraform runs) Shouldn't we reconsider this PR ?

@barryib
Copy link
Author

barryib commented Nov 3, 2020

Hi @pselle, how can we move forward on this ?

@pselle
Copy link
Contributor

pselle commented Nov 4, 2020

@barryib Since this likely pinged people on-thread, I'll share some information about what's going on with my assignment here for some clarity :)

The Terraform team recently took some time to evaluate some popular PRs, and this was one of them. What I'll be doing is evaluating whether or not to recommend this function addition move forward, before (or if at all) recommending updating this pull request to not-conflict with the latest Terraform codebase.

There's no timeline attached to this, but I picked up this PR with the intention to answer this question: should this function be added to the Terraform codebase, or do alternative solutions (for example community modules) fulfill this gap. For example, the hashicorp/dir/template implements a template directory strategy for that particular need.

I hope this gives some more context on what assignment "means" in this particular scenario, and I'll be returning to this PR at some point to share the decision of the Terraform Core team.

@barryib
Copy link
Author

barryib commented Nov 4, 2020

Thank you so much @pselle for sharing this information. Really appreciated.

I'll continue to update this PR and resolve conflict until a decision is made.

Thanks again for your answer.

PS: All my pings were about to get more information on the current decisions for this PR. My principal concern was to know if I should continue to work on this and keep updating this PR. And you answered that.

@nick4fake
Copy link

Just as an additional input for the decision making:

Looks like template provider has issues with the brand new terraform 0.14. I've got this issue while passing sensitive variable to template_file:
terraform-google-modules/terraform-google-kubernetes-engine#761

Provider is archived so I am not even sure if it is going to be resolved (or even should be resolved).

@barryib
Copy link
Author

barryib commented May 4, 2021

Hi, will this PR go somewhere ?

Since the template datasource depreciate, it'll be nice to continue to support dynamic templating with this template function.

@nitrocode
Copy link

nitrocode commented May 12, 2021

@barryib thank you for contributing this!

@pselle, I appreciate you taking to push this PR. Has there been an update from the terraform core team?

@barryib
Copy link
Author

barryib commented May 17, 2021

Maybe #28700 will address this ?

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.

6 participants