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

aws_lb_target_group_attachment: target_id should be a list #9901

Closed
dsantanu opened this issue Aug 28, 2019 · 14 comments
Closed

aws_lb_target_group_attachment: target_id should be a list #9901

dsantanu opened this issue Aug 28, 2019 · 14 comments
Assignees
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. service/elbv2 Issues and PRs that pertain to the elbv2 service.

Comments

@dsantanu
Copy link

dsantanu commented Aug 28, 2019

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or "me too" comments, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

When there are multiple target_groups for a LB and multiple instances needed to be attached to each target_group, there is no easy way to do that for reason target_id only being a string in aws_lb_target_group_attachment resource. Whilst AWS console allows to select multiple instances during the target attachment, terraform should behave the similar way as well.

The register-targets is the AWS CLI equivalent (to aws_lb_target_group_attachment) and there, --targets is a list. An example in that page says:

aws elbv2 register-targets --target-group-arn arn:aws:elasticloadbalancing:us-west-2:123456789012:targetgroup/my-targets/73e2d6bc24d8a067 --targets Id=i-1234567890abcdef0 Id=i-0abcdef1234567890

The Go SDK also has the similar RegisterTargets, where Targets also a list. So I think it's well supported in the API layer.

New or Affected Resource(s)

  • aws_lb_target_group_attachment

Potential Terraform Configuration

vars.tf

variable "nlb_listeners" {
  default = [
    {
      protocol     = "TCP"
      target_port  = "80"
      health_port  = "1936"
    },
    {
      protocol     = "TCP"
      target_port  = "443"
      health_port  = "1936"
    }
  ]
}

instances.tf

// EC2 instances
resource "aws_instance" "insts" {
  count         = var.inst_count
  instance_type = var.inst_type
  .......
}

balencer.tf

// Get the instance ids of the NLB members  
data "aws_instances" "nlb_insts" {
  instance_tags = {
   Name = "${var.vpc_names[var.idx]}${var.inst_role}0*"
  }
  instance_state_names = ["running", "stopped"]
}

// Creates the target-group
resource "aws_lb_target_group" "nlb_target_groups" {
  count                = length(var.nlb_listeners)
  name                 = "nlb-tgr-${lookup(var.nlb_listeners[count.index], "target_port")}"
  deregistration_delay = var.deregistration_delay
  port                 = lookup(var.nlb_listeners[count.index], "target_port")
  protocol             = lookup(var.nlb_listeners[count.index], "protocol")
  vpc_id               = var.vpc_ids[var.idx]

  health_check {
    port                = lookup(var.nlb_listeners[count.index], "health_port")
    protocol            = lookup(var.nlb_listeners[count.index], "protocol")
    interval            = var.health_check_interval
    unhealthy_threshold = var.unhealthy_threshold
    healthy_threshold   = var.healthy_threshold
  }
}

// Attach the target groups to the instance(s)
resource "aws_lb_target_group_attachment" "tgr_attachment" {
  count            = length(var.nlb_listeners)
  target_group_arn = element(aws_lb_target_group.nlb_target_groups.*.arn, count.index)
  target_id        = [for sc in range(var.inst_count) : data.aws_instances.nlb_insts.ids[sc]]
  port             = lookup(var.nlb_listeners[count.index], "target_port")
}

References

https://discuss.hashicorp.com/t/aws-lb-target-group-attachment-how-to-add-multiple-instances-per-lb-listeners
https://docs.aws.amazon.com/elasticloadbalancing/latest/APIReference/API_RegisterTargets.html
https://docs.aws.amazon.com/sdk-for-go/api/service/elbv2/#ELBV2.RegisterTargets
https://docs.aws.amazon.com/cli/latest/reference/elbv2/register-targets.html

  • #0000
@dsantanu dsantanu added the enhancement Requests to existing resources that expand the functionality or scope. label Aug 28, 2019
@ghost ghost added service/ec2 Issues and PRs that pertain to the ec2 service. service/elbv2 Issues and PRs that pertain to the elbv2 service. labels Aug 28, 2019
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Aug 28, 2019
@kvbik
Copy link

kvbik commented Jun 10, 2020

Seems like starting from 0.12 when used with count the behaviour is consistent with aws_instance.count.

Pitty I was not able to find it in official docs, but stackoverflow saved my day https://stackoverflow.com/a/58158252 kudos to user:5734474 there ;)

resource "aws_alb_target_group_attachment" "test" {
  count = length(aws_instance.test)
  target_group_arn = aws_alb_target_group.test.arn
  target_id = aws_instance.test[count.index].id
}

@choppedpork
Copy link

I've been struggling with a slightly different usecase which this proposal would also resolve. I've created an ALB module which sets up certificates, target groups, listeners, listener rules and attachments in addition to the load balancer itself. The module takes in a few lists of instance IDs (one list per target group). As the instances are produced by another module, it is currently (to the best of my understanding) impossible to create these attachments in any other way than passing in a separate variable per target group which specifies the size of the group - this is because terraform cannot resolve a plan if the number of resources is not known.

Here's a code snippet of what I was attempting to do:

locals {
  attachments = flatten([
    for group_name, group in local.target_groups : [
      for target in group.nodes : {
        target     = target
        group_name = group_name
      }
    ]
  ])
}

resource "aws_lb_target_group_attachment" "attachments" {
  for_each = { for attachment in local.attachments : "${attachment.group_name}-${attachment.target}" => [attachment.target, attachment.group_name] }

  target_group_arn = aws_lb_target_group.target_groups[each.value[1]].arn
  target_id        = each.value[0]
  port             = aws_lb_target_group.target_groups[each.value[1]].port
}

This works great if the instances already exist but fails completely if they don't - and also causes problems any time an instance is terminated outside of terraform (eg. when AWS retires one of them).

Being able to pass a list of targets to the aws_lb_target_group_attachment resource would allow me to create one aws_lb_target_group_attachment per target group - the number of target groups is always known, it's just the size of the group that's unknown in this case.

@jav-12
Copy link

jav-12 commented Aug 13, 2020

I'm having the same issue, instances are being created in a separate module so target_id makes more sense to be a list instead of a string.

@sbaier1
Copy link

sbaier1 commented Oct 9, 2020

This seems to be a huge problem for attaching a LB to an autoscaling group as well. When editing target groups manually, i can easily add instances with multiple ports at once, but if i try to work around it in the provider by creating multiple target groups for each port and corresponding attachments that attach my LB to the ASG, i will get multiple target groups but from the attachments, only one will actually attach to the ASG. So there appears to be no way currently to solve this.

@maddinek
Copy link

the only workaround if you have multiple target_id's and multiple target_group_arn's at the moment is basically copying the block to the amount of the target_id/target_group_arn, counting or for_each through the resources and referencing the other by [0], [1] and so on.

for instance, like this:
`resource "aws_lb_target_group_attachment" "network0" {
count = length(aws_instance.instance)
target_group_arn = aws_lb_target_group.network**[0]**.arn
target_id = aws_instance.instance[count.index].id
port = 8080
}

resource "aws_lb_target_group_attachment" "network1" {
count = length(aws_instance.instance)
target_group_arn = aws_lb_target_group.network**[1]**.arn
target_id = aws_instance.instance[count.index].id
port = 8081
}`

or

`resource "aws_lb_target_group_attachment" "network0" {
for_each = var.mapofports
target_group_arn = aws_lb_target_group.network[each.key].arn
target_id = aws_instance.dmaap-prod-vpn-l7-router**[0]**.id
port = each.value
}

resource "aws_lb_target_group_attachment" "network1" {
for_each = var.mapofports
target_group_arn = aws_lb_target_group.network[each.key].arn
target_id = aws_instance.dmaap-prod-vpn-l7-router**[1]**.id
port = each.value
}`

At least I don't know another way at the moment. Hope that example might help someone, or someone has a better way to handle it for now.

Cheers

@Ajay250496
Copy link

Ajay250496 commented Jul 29, 2021

I pretty much have the same requirement. I have a user controlled number of instances and a user controlled number of ports I want to configure on a load balancer. I can create target groups for every port (e.g 80,443,8089) but I cannot create the target group attachments for each instance AND each target group.
Manually creating the resources for the amount of target groups or instances aren't really a work around as I want this to be something supplied in variables.

Any other workarounds for this situation?

UPDATE
I was able to solve this by using setproduct to create the a list with the amount of instances and amount of target groups then using foreach to loop through this.

Initially I tried to reference the instance ids and target_group_arn directly but would get an error stating that I cannot use a resource that has not already been created. The error was specifically this:

The "for_each" value depends on resource attributes that cannot be determined until apply, so Terraform cannot predict how many instances will be created.
To work around this, use the -target argument to first apply only the resources that the for_each depends on

This caused me to use setproduct to build a list of indexes based on the input values.

variables

instance_count = 3  # amount of instances
listener_port = [22,80,443] #ports i want to listen on

Use setproduct to find all of the possible combinations of element

setproduct(range(length(var.listener_port)),range(var.instance_count))

Use foreach to create a map, loop through it and reference the the target group arn and instance ids based off of index:

resource "aws_alb_target_group_attachment" "target_group" {
  for_each = {
    for pair in setproduct(range(length(var.listener_port)),range(var.instance_count)) : "${pair[0]} ${pair[1]}" => {
      target_group_arn = pair[0]
      target_id        = pair[1]
    }if var.enable_listener
  }
  target_group_arn = aws_lb_target_group.target_group[each.value.target_group_arn].arn
  target_id        = aws_instance.aws_instances[each.value.target_id].id
}

This dynamically attaches instances to target groups based on the input values.

For clarity when using a var.listener_port of [80,443,22] and a var.instance_count of 2. This means I am creating 2 instances and want to attach it to 3 target groups (each serving a different port).
The values foreach loops through looks like this:

  "Test2" = {

    "0 0" = {

      "arn_key" = 0

      "instance_key" = 0

    }

    "0 1" = {

      "arn_key" = 0

      "instance_key" = 1

    }

    "1 0" = {

      "arn_key" = 1

      "instance_key" = 0

    }

    "1 1" = {

      "arn_key" = 1

      "instance_key" = 1

    }

    "2 0" = {

      "arn_key" = 2

      "instance_key" = 0

    }

    "2 1" = {

      "arn_key" = 2

      "instance_key" = 1

    }

Hope this helps someone.
Thanks!

@breathingdust breathingdust removed the needs-triage Waiting for first response or review from a maintainer. label Sep 22, 2021
@YakDriver
Copy link
Member

Changing target_id to a list would be a breaking change and require waiting until v5 of the AWS provider. However, perhaps a better solution would be to add a new argument target_ids which is a list and which is mutex of target_id. Then you have backward compatibility and feature you need. We could deprecate target_id but not remove it until v5 to give everyone plenty of time. Thoughts on this approach?

@dbsanfte
Copy link

Sounds good, get it done! Six months too late for me, sadly. One of those ugly workarounds is going into my code.

@richard-mck
Copy link

For others encountering this issue, as Ajay suggests above, setproduct will help solve this. It can be significantly simplified, I've got a working example here:

locals {
  # This produces a list of lists with contents like:
  # [[arn_1, instance_id_1], [arn_1, instance_id_2], [arn_2, instance_id_1], [arn_2, instance_id_2]] etc
  product_list = setproduct(aws_lb_target_group.dynamic[*].arn, var.instance_ids)
}

resource "aws_lb_target_group_attachment" "dynamic_attachment" {
  # We need to create an attachment for each instance on each additional port
  # Therefore we need numInstances * numPorts and use `setproduct` to find the possible combinations
  # There is an outstanding Github issue to change `target_id` to accept a list as input instead of a string
  # https://github.com/hashicorp/terraform-provider-aws/issues/9901
  count            = length(local.product_list)
  target_group_arn = local.product_list[count.index][0]
  target_id        = local.product_list[count.index][1]
}

If either input list is empty, setproduct produces an empty list as a result and therefore the resources aren't created.

@jar-b
Copy link
Member

jar-b commented Jul 5, 2023

Hello everyone - We've started work on this issue and wanted to communicate our proposed path forward.


Background

The aws_lb_target_group_attachment resource currently represents a single target group attachment per resource. This diverges from the underlying RegisterTargets and DeregisterTargets APIs, which accept an array of objects. To bring the practitioner experience in line with the AWS API or CLI, we have the following options.

  1. Create a new, plural target registration resource and deprecate aws_lb_target_group_attachment.
  2. Add a new, plural argument (target_ids) to the existing resource and deprecate the singular target_id argument.
  3. Add a new nested object argument (targets) to the existing resource and deprecate the target_id, port, and availability_zone arguments.

Each of these options requires a trade off. Option 1 (new, plural resource) would introduce a second method for registering target groups, at least until a major release where the singular attachment resource can be removed. Option 2 (new target_ids argument) is the simplest solution, but also limits practitioner flexibility because the root level port and availablity_zone arguments will be applied to all items in the target_ids list, rather than allowing each item to specify their own. Additionally, this behavior may not be obvious to users given the argument structure. Option 3 resolves the flexibility issue by creating a nested target object with target_id, port, and availability_zone arguments, but could again lead to confusion as the nested argument names are duplicated from the root arguments.

Proposal

The recommendation is to proceed with a new, plural target registration resource (Option 1) that more closely aligns with the AWS API. While this does introduce a second method for registering target groups, it does not directly conflict with the existing singular resource, and will not cause target groups defined with that resource to be removed. It also avoids overloading a single resource with multiple ways to specify the same infrastructure and repetitive argument names. Lastly, there is a clear path to deprecating the aws_lb_target_group_attachment resource.

The proposed resource configuration would be:

resource "aws_lb_target_registration" "example" {
  target_group_arn = aws_lb_target_group.example.arn

  target {
    target_id         = aws_instance.example1.id
    port              = 80
    availability_zone = "us-east-1a"
  }

  target {
    target_id         = aws_instance.example2.id
    port              = 8080
    availability_zone = "us-west-2c"
  }
}

With this approach, the existing target group attachment resources will be deprecated. The documentation should be updated to recommend usage of the plural registration resource, and an issue should be created for removal of the resource in a future major version.

@jar-b
Copy link
Member

jar-b commented Jul 18, 2023

Hello everyone - after some internal discussion, we decided to step back and assess how "relationship" resources have historically been handled in the AWS provider. This research has been collected in a new proposal #32552, currently open for community feedback.

In summary, the AWS provider has historically maintained a strong preference for "singular" representations of parent-child relationships (ie. one child per resource versus many in a single resource). For this feature request, this means we'd propose leaving aws_lb_target_group_attachment as-is, but supplementing the documentation with examples of how to register multiple targets via Terraform meta-arguments (see #32518). This would also mean closing the proposed plural target registration resource (#32380) without merging.

Given that this design decision has a practical impact on the resolution of this issue, please feel free to leave feedback on the proposal or documentation additions. We'll plan to leave it open for at least the next week to allow time for community suggestions. Thanks for your patience!

@jar-b
Copy link
Member

jar-b commented Jul 31, 2023

The "relationship" resource proposal from #32552 has been accepted. We're going to close #32380 and mark this issue as resolved with the documentation enhancements added in #32518.

@jar-b
Copy link
Member

jar-b commented Jul 31, 2023

Closed via #32518

@jar-b jar-b closed this as completed Jul 31, 2023
@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 Aug 31, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/ec2 Issues and PRs that pertain to the ec2 service. service/elbv2 Issues and PRs that pertain to the elbv2 service.
Projects
None yet
Development

Successfully merging a pull request may close this issue.