hashicorp / terraform

Terraform enables you to safely and predictably create, change, and improve infrastructure. It is a source-available tool that codifies APIs into declarative configuration files that can be shared amongst team members, treated as code, edited, reviewed, and versioned.
https://www.terraform.io/
Other
42.35k stars 9.49k forks source link

Feature Request: Defining variables/locals inside a resource block #20672

Open rahulwa opened 5 years ago

rahulwa commented 5 years ago

I trying to define some variables inside a resource so that it can be referred when needed and this resource block is in the loop so I can't define these as locals outside the resource. Is this possible in some way?:

resource "aws_cloudwatch_metric_alarm" "httpcode_target_5xx_count" {
  locals {
      len = "${length(data.aws_lb.this.target_group_arn_suffixes)}"
      current_index = "${element(data.aws_lb.this.target_group_arn_suffixes, count.index)}"
      current_thresold = "${var.target_5xx_thresold["$local.current_index"] != "" ? var.target_5xx_thresold["$local.current_index"] : "${var.lb_5xx_thresold}"}"
  }
  count                     = "${local.len}"
  alarm_name                = "${var.prefix_resources}: ALB-5XX-ERROR: TARGET: ${replace(local.current_index,"/(targetgroup/)|(/\\w+$)/","")}"
  comparison_operator       = "GreaterThanThreshold"
  evaluation_periods        = "${local.evaluation_periods}"
  metric_name               = "HTTPCode_Target_5XX_Count"
  namespace                 = "AWS/ApplicationELB"
  period                    = "300"
  statistic                 = "Sum"
  threshold                 = "${local.current_thresold}"
  treat_missing_data        = "missing"
  alarm_description         = "HTTPCode 5XX count for ${local.current_index} over ${local.current_thresold} last 300 seconds over 1 period(s)"
  alarm_actions             = "${concat(var.notify_sns_arns, aws_sns_topic.opsgenie_sns_topic.arn)}"
  dimensions = {
    "TargetGroup"  = "${local.current_index}"
    "LoadBalancer" = "${data.aws_lb.this.arn_suffix}"
  }
}

Thank you very much, i tried searching but couldn't got anything hence a github issue. Tried a IRC (#terraform-tool) also but it seems that nobody was there.

apparentlymart commented 5 years ago

Hi @rahulwa,

That isn't possible in Terraform today. For Terraform v0.12 (in the master branch) we've actually reserved the name locals in this context to allow us to potentially implement that feature in the future, since indeed it would be very useful for the reasons you state here, but we've not yet been able to implement that feature.

If it's okay with you, I'd like to turn this issue into a feature request for that, because I don't think we have one open already. That'll give a place for other users to vote for the feature (by adding :+1: reactions to your original message, which we can use as an input for prioritization).


For this particular use-case, we are also planning to add a new for_each feature to Terraform, described in #17179, which once implemented would at least remove the need for your len and current_index values because the built-in feature would manage those for you:

resource "aws_cloudwatch_metric_alarm" "httpcode_target_5xx_count" {
  for_each = {
    for s in data.aws_lb.this.target_group_arn_suffixes : s => {
      threshold = lookup(var.target_5xx_threshold, s, var.lb_5xx_threshold)
    }
  }

  alarm_name                = "${var.prefix_resources}: ALB-5XX-ERROR: TARGET: ${replace(each.key ,"/(targetgroup/)|(/\\w+$)/","")}"
  comparison_operator       = "GreaterThanThreshold"
  evaluation_periods        = local.evaluation_periods
  metric_name               = "HTTPCode_Target_5XX_Count"
  namespace                 = "AWS/ApplicationELB"
  period                    = "300"
  statistic                 = "Sum"
  threshold                 = each.value.threshold
  treat_missing_data        = "missing"
  alarm_description         = "HTTPCode 5XX count for ${local.current_index} over ${local.current_thresold} last 300 seconds over 1 period(s)"
  alarm_actions             = concat(var.notify_sns_arns, aws_sns_topic.opsgenie_sns_topic.arn)
  dimensions = {
    "TargetGroup"  = each.key
    "LoadBalancer" = data.aws_lb.this.arn_suffix
  }
}

This feature also won't be fully complete in v0.12.0 due to the already-large scope of that release, but it's closer to being completed and we're hoping to complete it shortly after in a subsequent release.

One reason we've de-prioritized the ability to use locals inside resource blocks for now is that we suspect that the for_each feature may meet the same use-cases in a simpler way, so we want to release that first and see how it is used to see if it can cover all of the use-cases or if a resource-specific locals block would be helpful for other reasons.


For now, unfortunately inserting the full expressions into the attributes directly is the only answer for current Terraform releases. We know this hurts readability and maintainability, so these other planned features are the intended answer to that once we are able to complete the other foundational work that is needed to support them.

rahulwa commented 5 years ago

@apparentlymart You can go ahead and create it as a feature request and thank you very much for replying so promptly. so, for now, I will be going forward with normal flow. I also think that for_each is very important for readability so 👍 💯 for this feature.

apparentlymart commented 5 years ago

The feature described here is likely to be the solution for #3267. I'm not going to close either of them as a duplicate of the other for now, since this one is a description of a solution while the other is a description of a use-case and the other has a lot of votes attached to it that I don't want to lose, but this comment should help us track the relationship here so we can update them both together in future.

OJFord commented 5 years ago

I would also very much like to see this. Locals have been a big improvement, but I find myself using them a lot for look-up tables that would be much cleaner if the values were simply attached to the resource that the key references, as suggested here.

StefanoFrazzetto commented 4 years ago

I'll add myself to the list of those who would like to have this feature implemented 🔧

mltsy commented 4 years ago

Just to address the question of whether the resource locals block will still be useful after for_each is working: I'm using for_each in a resource, but would still find a locals block inside the resource very useful for cleaning up the resource by adding some data translation (applying functions to pieces of each.value, and re-using the result) inside each instance of the resource without implementing another level of modules just to hold the translated data. Great feature! 👍 🙏

apparentlymart commented 4 years ago

Hi all!

I just wanted to clarify what I meant in my earlier comment about how the for_each feature might solve a bunch of the use-cases. Since you can use for expressions to produce a transformed map, you can in principle sneak some extra expressions in there for any transformations you might want to re-use multiple times.

Here's a contrived example because it's late in my day and I'm lacking imagination. :grinning:

variable "names" {
  type = set(string)
}

resource "example" "example" {
  for_each = {
    for n in var.names : n => {
      # The attributes defined in here will be available via each.value
      name_upper = upper(name)
      name_lower = lower(name)
    }
  }

  name_as_given = each.key
  name_upper    = each.value.name_upper
  name_lower    = each.value.name_lower
}

The elements of the for_each collection effectively create a local scope containing each.key and each.value on each repetition, and each.value is entirely under your control because Terraform doesn't use the map values for any purpose. (each.key is more constrained because it also becomes the tracking key for the instances.)

A similar approach can also work for dynamic blocks when nested block repetition is the goal, because the for_each in there behaves in a similar way.

I don't mean to imply that the above necessarily solves all the problems that a local locals would solve, but I wanted to share it in case it's useful to folks with goals like this in the meantime, since resource-level for_each was implemented in Terraform v0.12.6. (I would love to hear about anything you come up with that would require the first-class locals block I described in my earlier comment, though!)

Toady00 commented 3 years ago

Is this a thing yet or has this issue been forgotten?

captainfalcon23 commented 2 years ago

Any updates on this? Seems this is not implemented in tf 1.1.9 :(

apparentlymart commented 2 years ago

It looks like the status of this issue was that we were looking for feedback on any situations where using for_each with a for expression (as I described in earlier comments) isn't sufficient, since the priority of this is necessarily lower if an existing feature can already meet the same use-cases.

If you have tried to use for_each with object values and found that it didn't entirely solve the problem and that "local locals" could solve it, please let us know in a comment. It's important to start from use-cases because otherwise we risk designing this "local locals" feature with the same limitations as the current language feature, and missing whatever additional use-cases folks might have in mind.

OJFord commented 2 years ago

For what it's worth, I've been happily using for_each whenever I've wanted this since, and as far as I can tell there's almost^ no circumstance in which that wouldn't work, it just might look neater with 'local locals'.

^ The one I thought of was destroy-time provisioners where you can't use each, but hypothetically I suppose you might use a 'local local' as long as it didn't refer to each. Since they're fraught with issues anyway, and AIUI being considered for dropping/redesigning, that's probably not a use-case in favour of 'local locals'.

captainfalcon23 commented 2 years ago

s of the for_each collecti

Ah got you. Well, my usecase which led me here is something along the lines of this: we have 3 layers to most of our modules:

implementation module (raw resources) abstraction module end user implementation tf

In my current case, I am developing some terraform code to manage log monitors in Sumologic. In my abstraction module, I have defined all of my log monitors, and their corrosponding search syntax etc. One of the annoying problems is, within each of the log monitors, I use the name of the monitor for various functions. It's annoying to have to have the same name defined 2-3 times when a local local would allow me to define it once and use it N number of times.

Another use case was I wanted to pass in something like this:

{
check1: {destination1: abc, destination2: def}
check2: {destination1: abc, destination2: def}
check3: {destination1: abc, destination2: def}
}

and I wanted to use a local local on each of the checks to dynamically grab the relevant element based on the check name and perform some data manipulation and then pass that directly as one of the variables.

Anyways, I managed to get around my issues by redesigning some inputs and using the experiment "module_variable_optional_attrs".

But long story short, I think local locals, which are relevant only to a module call make great sense - espeically when you want to reuse the same value multiple times e.g. name/description/tag etc.

james-callahan commented 1 year ago

I found myself wishing for this today:

resource "aws_instance" "control_plane" {
  count = var.num_instances

  locals {
    subnet  = values(data.aws_subnet.control_plane)[count.index % var.num_instances]
    hostnum = floor(count.index / var.num_instances)
  }

  availability_zone = local.subnet.availability_zone
  private_ip        = cidrhost(aws_ec2_subnet_cidr_reservation.control_plane_ipv4[local.subnet.id].cidr_block, local.hostnum)
  ipv6_addresses    = [cidrhost(aws_ec2_subnet_cidr_reservation.control_plane_ipv6[local.subnet.id].cidr_block, local.hostnum)]

Inlining those local value gets very hard to read and debug.


@OJFord

For what it's worth, I've been happily using for_each whenever I've wanted this since, and as far as I can tell there's almost^ no circumstance in which that wouldn't work, it just might look neater with 'local locals'.

^ The one I thought of was destroy-time provisioners where you can't use each, but hypothetically I suppose you might use a 'local local' as long as it didn't refer to each. Since they're fraught with issues anyway, and AIUI being considered for dropping/redesigning, that's probably not a use-case in favour of 'local locals'.

One such example of when you can't use for_each is when you're using count!

apparentlymart commented 1 year ago

Hi @james-callahan! Thanks for sharing that example.

It's true that count doesn't provide the same opportunity to associate an arbitrary value with each instance of the resource. In situations where the goal is to create a number of fungible objects (all exactly equal, such that if one were destroyed it wouldn't make any difference which one), count is often the best choice and in situations like that you would not currently be able to concisely associate a value with each element.

That is a good observation and leads me to a question: are there situations where both instances are totally fungible but you also have specific varying values on a per-instance basis? I expect there are some examples of that to be found, but I don't yet feel sure that the one you've given here is an example of that.

If I'm reading this correctly it seems like your goal is to spread a larger number of instances over a smaller set of subnets, and to assign predictable IP addresses to each one. When I think about fungible instances I tend to think that it shouldn't matter exactly what their IP addresses are, because e.g. I'd want to be able to add and remove arbitrary instances at any time without worrying about exactly which one I'm working with and what its IP address ought to be. Your example here seems like it would also cause a large amount of churn if there were a change in either the number of instances or the number of subnets, because the modulo operator would change the relationships between a particular count.index and a particular subnet.

With those observations in mind, on the surface at least it seems like the situation you described is a prime example of a situation where for_each would make sense, using composite instance keys that include both the subnet ID (or something more stable derived from it, like the availability zone name) and an index within that zone.

For example:

locals {
  instance_groups = chunklist(range(var.num_instances), length(data.aws_subnet.control_plane))
  instances = flatten([
    for subnet_idx, instance_group in local.instance_groups : [
      for host_num, instance_idx in instance_group : {
        host_num     = host_num
        instance_idx = instance_idx
        subnet       = data.aws_subnet.control_plane[subnet_idx]
      }
    ]
  ])
}

resource "aws_instance" "control_plane" {
  for_each = {
    for inst in local.instances : "${inst.subnet.availability_zone}:${inst.host_num}" => inst
  }

  subnet_id = each.value.subnet.id
  private_ip = cidrhost(aws_ec2_subnet_cidr_reservation.control_plane_ipv4[each.value.subnet.id].cidr_block, each.value.host_num)
  ipv6_addresses = [cidrhost(aws_ec2_subnet_cidr_reservation.control_plane_ipv6[each.value.subnet.id].cidr_block, each.value.host_num)]
}

This produces instances that are identified by which availability zone they belong to and their position within that availability zone. Since the IP addresses are calculated based on the subnet and the host number (assuming that each AZ has only one subnet relevant to this question) the instance identifiers are now effectively identifying the same thing as the IP addresses are, and so these two sets of identifiers won't "skew" as you change var.num_instances or the number of elements of data.aws_subnet.control_plane, and so you won't get the same churn of renumbering a bunch of existing instances in that case. If you add a new subnet without changing the total instance count then a few of the instances will be destroyed in favor of instances in the new subnet, but the others will retain the same instance keys and the same IP addresses and so Terraform won't propose to change them at all.

I will concede that in order to make the above readable I did have to hoist the calculations of instance groups and individual instances out into separate local values rather than doing those entirely inline in the aws_instance block, but I don't think that a per-instance locals block would've helped with that because those calculations are determining the full set of instances, not just calculating values that vary on a per-instance basis. (It calculates some per-instance values as a side-effect, and records them in the generated objects so that they'll be part of each.value in the resource block.)

I don't mean this to say that there is no possible situation where you might need per-instance calculated values while using count, but just that this particular case seems expressible using for_each too and is arguably better to write with for_each, because it leads to instance identifiers that better reflect how the remote system is identifying those objects.

It would be interesting to see if we can find an example using count where it would not be arguably better to recast it as for_each. I think a key characteristic of such an example would be that consecutive incrementing integers really are the most natural identifier to use for the instances, and there isn't some other more meaningful key that describes each instance's relationship to other parts of the broader system.

james-callahan commented 1 year ago

When I think about fungible instances I tend to think that it shouldn't matter exactly what their IP addresses are, because e.g. I'd want to be able to add and remove arbitrary instances at any time without worrying about exactly which one I'm working with and what its IP address ought to be

Many distributed programs need a consistent IP to know when one instance replaces another.

Your example here seems like it would also cause a large amount of churn if there were a change in either the number of instances

Not at all.

or the number of subnets

yes changing the number of subnets would be a big reshuffle. However it's such a rare change to make I can't see it becoming an issue.

instance_groups = chunklist(range(var.num_instances), length(data.aws_subnet.control_plane))

Ah ha! I didn't know about the range function. That lets me solve this in my own way.

OJFord commented 1 year ago

@OJFord One such example of when you can't use for_each is when you're using count!

I exceedingly rarely use count now that for_each exists, IMO it's a probable smell: something not captured as a dependency where it could be.

Anyway, it does have a use, but if that were the only problem here it's trivial to have a for_each set equivalent to a count.

A time I have wanted this recently was when self can't be used - I think I mentioned it above about destroy-time provisioners, but it's sometimes desirable (if only to avoid repetition) in other blocks or even top-level attributes too. I understand the problems with it, why it's not possible, but I think having a declared set of locals that are valid references would mitigate that.

ryan-dyer-sp commented 3 months ago

I find myself still wanting this functionality.

My TF snippet:

resource "aws_rds_cluster_instance" "this" {
  // adding 1 accounts for the writer, which is the 1st one created
  // this allows the read replica count to properly match what is passed in for replica_scale_min or replica_count
  count = (var.replica_scale_enabled ? var.replica_scale_min : var.replica_count) + 1

  # Creating a string of the format "e1-e2-e3-e4-" but any of those elements may not exist and AWS doesnt like -- in identifier_prefix, so we compact the list
  identifier_prefix = "${join("-", compact([
    # e1 - Name of the cluster
    aws_rds_cluster.this.id,
    # e2 - if an instance_parameters[count.index][instance_prefix] exists, use it.  Otherwise count.index.
    try(var.instances_parameters[count.index]["instance_prefix"], count.index),
    # e3 - instance_prefix_suffix
    var.instance_prefix_suffix,
    # e4 - a 4 char hash based on the instance_type . Ensures if instance_type ever changes, a B/G is performed (as we have create before destroy lifecycle)
    lower(substr(md5(try(var.instances_parameters[count.index]["instance_type"], var.instance_type)), 0, 4)),
  ]))}-"

a DB instance identifier has to be <64 characters. The aws_rds_cluster_instance provider appends a 27 character timestamp (not sure why they need the timestamp to be that long, but thats a different problem). This leaves the _prefix to have 36 characters. e4 always users 4, so we're left with ~30 characters for the other 3 fields which are all of variable length or existance. So I would like to be able determine the lengths of e2 and e3 and then truncate e1 based on the length still available. But I only have access to e2 from within this resource.

Welcome to workarounds if anyone has any.