feat: Add target group attachment capabilities #191

veilig2000 · 2021-03-29T19:58:11Z

Description

Allows for the creation of a "targets" index inside the target_groups variable, where the parameters of the attachment can be defined. The same arguments that are used for the target_group_attachment resource, are the same arguments that should/can be defined. Any additional arguments are created during the "product mapping", but are not used in the resource, but allows for easy upgrading in the future if more arguments are added (only have to update on the resource function)

https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lb_target_group_attachment#argument-reference

Motivation and Context

Allow attaching EC2 instance(s) and Lambda functions to the target groups created by this module so all resources can be managed in one location. Currently, there's no functionality to add any attachments to the target groups this module creates and everything needs to be additionally defined/coded

Breaking Changes

This should be backwards compatible, if no "targets" are defined on the target_group variable, no attachments are created.

How Has This Been Tested?

[x] I have tested and validated these changes using one or more of the provided examples/* projects

This was manually tested in a sandbox AWS environment on actual ALB/EC2 resources. I currently don't work w/ lambda function attached to any type of LB, so I did not test that. But since the function calls are the same on the target_group_attachment resource between an EC2 instance and a Lambda Function and this is just a mapping of parameters to that function, I really don't anticipate any issues.

variable definition can look like this now

    target_groups = [
        {
            name = "tg-with-targets"
            backend_protocol = "HTTP"
            backend_port = "80"
            target_type = "instance"
            deregistration_delay = 300
            health_check = { ... }
            stickiness = { ... }
            targets = [
                {
                    target_id = "i-0123456789abcdefg"
                    port = 80
                },
               {
                    target_id = "i-a1b2c3d4e5f6g7h8i"
                    port = 8080
                }
            ]
        },
        {
            name = "tg-without-targets"
            backend_protocol = "HTTP"
            backend_port = "80"
            target_type = "instance"
            deregistration_delay = 300
            health_check = { ... }
            stickiness = { ... }
        }
    ]

Why is it necessary? (Bug fix, feature, improvements?) - Allow attaching EC2 instance(s) and Lambda functions to the target groups created by this module so all resources can be managed in one location How does the change address the issue? - Allows for the creation of a "targets" index inside the target_groups variable, where the parameters of the attachment can be defined. - The same arguments that are used for the target_group_attachment resource, are the same arguments that should/can be defined. Any additional arguments are created during the "product mapping", but are not used in the resource, but allows for easy upgrading in the future if more arguments are added (only have to update on the resource function) https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lb_target_group_attachment#argument-reference What side effects does this change have? - This should be backwards compatible, if no "targets" are defined on the target_group variable, no attachments are created. Include a link to the ticket, if any. - N/A

veilig2000 · 2021-03-31T17:17:05Z

@bryantbiggs - was curious how do you test your change for e5f2627 ? I don't even know if that is the problem that I'm facing. I just saw a previous PR that didn't look like it was failing (like mine is) 8 days ago, your fix 4 days ago, and my PR 2 days ago and its failing - so I started looking if that was causing it.

I'm trying to debug why my pre-commit build is failing for the Max TF pre-commit (0.14.9). Everything locally is passing for me after checking w/ a couple TF versions.

 ▲ ~/code/terraform-aws-alb terraform -v                                master  1d ⬢
Terraform v0.14.8
+ provider registry.terraform.io/hashicorp/aws v3.34.0

Your version of Terraform is out of date! The latest version
is 0.14.9. You can update by downloading from https://www.terraform.io/downloads.html
 ▲ ~/code/terraform-aws-alb pre-commit run -a                           master  1d ⬢
Terraform fmt............................................................Passed
Terraform validate.......................................................Passed
Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
Check for merge conflicts................................................Passed
 ▲ ~/code/terraform-aws-alb tfswitch                               6s   master  1d ⬢
Reading required version from terraform file, constraint: >= 0.12.6
Matched version: 0.14.9
Switched terraform to version "0.14.9"
 ▲ ~/code/terraform-aws-alb pre-commit run -a                           master  1d ⬢
Terraform fmt............................................................Passed
Terraform validate.......................................................Passed
Terraform docs...........................................................Passed
Terraform validate with tflint...........................................Passed
Check for merge conflicts................................................Passed
 ▲ ~/code/terraform-aws-alb                                        6s   master  1d ⬢

I tried downloading act to debug some more, but I'm running into this yaml: unmarshal error now. So I was just curious how you test things?

 ▲ ~/code/terraform-aws-alb act --version                          master  1d ⬢
act version 0.2.21
 ▲ ~/code/terraform-aws-alb act -l                                 master  1d ⬢
Error: yaml: unmarshal errors:
  line 33: cannot unmarshal !!str `${{ fro...` into []interface {}

It's just complaining that it can't find terraform-docs

ERROR: terraform-docs is required by terraform_docs pre-commit hook but is not installed or in the system's PATH.

veilig2000 · 2021-03-31T18:19:28Z

figured it out

curl -s https://api.github.com/repos/terraform-docs/terraform-docs/releases/latest | grep -o -E "https://.+?-v0.12.0-linux-amd64"

The grep on the latest version doesn't allow for patches and is hardcoded to v0.12.0, the latest right now is v0.12.1. Looking at best way to update that.

Why is it necessary? (Bug fix, feature, improvements?) - the latest release will not always be tied to the v0.12.0 tag. Allow patches to the release How does the change address the issue? - use a regex to search for patches What side effects does this change have? - As soon as the latest release goes above the v0.12 version, this is going to break. A better way to do this will be to fetch all the tags and correlate the highest v0.12 release via the API, but thats going to take a little more work, as there's currently no way to fetch releases associated to a major/minor version that I can see. - https://docs.github.com/en/rest/reference/repos#list-repository-tags - https://docs.github.com/en/rest/reference/repos#get-a-release-by-tag-name Include a link to the ticket, if any. - N/A

bryantbiggs · 2021-03-31T18:54:43Z

oy, this damn terraform docs - my apologies @veilig2000 , let me see what I can do

veilig2000 · 2021-03-31T18:57:34Z

lol - I just updated it - it worked fine on the latest ubuntu docker image locally (just getting the tf-docs pulled down), now some other weird error is being generated :/

veilig2000 · 2021-04-05T14:11:41Z

@antonbabenko, this look good to merge?

antonbabenko

Amazing piece of code! Just a few comments and questions.

antonbabenko · 2021-04-06T16:12:41Z

outputs.tf

+
+output "target_group_attachments" {
+  description = "ARNs of the target group attachment IDs."
+  value = [


I think it would be better to have this as a map so that users can identify both target_group (as a key) and a list of attachments (as a value)

main.tf

antonbabenko · 2021-04-06T16:17:22Z

examples/complete-alb/main.tf

@@ -293,6 +303,14 @@ module "alb" {
      name_prefix                        = "l1-"
      target_type                        = "lambda"
      lambda_multi_value_headers_enabled = true
+      targets = [
+        {
+          target_id = "arn:aws:lambda:us-east-1:123456789012:function:lambda_function_1"


Will it work if you specify non-existing ARN? If not, we need to update the code to create a real Lambda function and use it.

veilig2000 · 2021-04-08T14:03:13Z

Amazing piece of code! Just a few comments and questions.

I'll get all these reviewed quickly so we can merge, just been buried with work

antonbabenko · 2021-04-14T12:24:57Z

@veilig2000 I will work on this later today.

antonbabenko · 2021-04-14T16:34:48Z

Thanks for your contribution, @veilig2000 !

v5.14.0 has been just released.

lev112 · 2021-04-15T11:07:39Z

this PR introduces a breaking change
I'm using terraform 0.12.29, and depend on the module with ~> 5.0
so if I'm running a clean init, I'm getting:

Module xxxx (from "terraform-aws-modules/alb/aws") does not
support Terraform version 0.12.29

because of the updated version constraint

IMO, this should have been a major version change

antonbabenko · 2021-04-15T11:37:59Z

@lev112 #195 (comment)

github-actions · 2022-11-15T02:31:43Z

I'm going to lock this pull request 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 related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

veilig2000 and others added 2 commits March 29, 2021 15:35

Merge branch 'master' into master

93ebb36

veilig2000 changed the title ~~Add target group attachment capabilities~~ feat: Add target group attachment capabilities Mar 30, 2021

chore: fix precommit hook errors

cde89e0

chore: fixing number range to just get anything in v0.12 release

da37c18

Merge branch 'master' into master

51cd9c7

antonbabenko reviewed Apr 6, 2021

View reviewed changes

antonbabenko added 4 commits April 14, 2021 17:57

Fixed code to work with not yet known resources as targets

02b6706

Trying to fix for bothversions of Terraform 0.12&0.15

1ba7db7

Updated a minimum version of Terraform to 0.13

67b8498

Added depends_on in example

2d1dff3

antonbabenko merged commit e72ba5d into terraform-aws-modules:master Apr 14, 2021

antonbabenko mentioned this pull request Apr 14, 2021

How to use aws_lb_target_group_attachment #134

Closed

ducoterra mentioned this pull request Apr 14, 2021

Empty target groups appear to break module #193

Closed

rmasclef mentioned this pull request Sep 19, 2021

Missing NLB Example using Instance target ID's #218

Closed

github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add target group attachment capabilities #191

feat: Add target group attachment capabilities #191

veilig2000 commented Mar 29, 2021 •

edited

Loading

veilig2000 commented Mar 31, 2021

veilig2000 commented Mar 31, 2021 •

edited

Loading

bryantbiggs commented Mar 31, 2021

veilig2000 commented Mar 31, 2021

veilig2000 commented Apr 5, 2021

antonbabenko left a comment

antonbabenko Apr 6, 2021

antonbabenko Apr 6, 2021

twentythousandphantoms Apr 12, 2021

veilig2000 commented Apr 8, 2021

antonbabenko commented Apr 14, 2021

antonbabenko commented Apr 14, 2021

lev112 commented Apr 15, 2021

antonbabenko commented Apr 15, 2021

github-actions bot commented Nov 15, 2022

feat: Add target group attachment capabilities #191

feat: Add target group attachment capabilities #191

Conversation

veilig2000 commented Mar 29, 2021 • edited Loading

Description

Motivation and Context

Breaking Changes

How Has This Been Tested?

veilig2000 commented Mar 31, 2021

veilig2000 commented Mar 31, 2021 • edited Loading

bryantbiggs commented Mar 31, 2021

veilig2000 commented Mar 31, 2021

veilig2000 commented Apr 5, 2021

antonbabenko left a comment

Choose a reason for hiding this comment

antonbabenko Apr 6, 2021

Choose a reason for hiding this comment

antonbabenko Apr 6, 2021

Choose a reason for hiding this comment

twentythousandphantoms Apr 12, 2021

Choose a reason for hiding this comment

veilig2000 commented Apr 8, 2021

antonbabenko commented Apr 14, 2021

antonbabenko commented Apr 14, 2021

lev112 commented Apr 15, 2021

antonbabenko commented Apr 15, 2021

github-actions bot commented Nov 15, 2022

veilig2000 commented Mar 29, 2021 •

edited

Loading

veilig2000 commented Mar 31, 2021 •

edited

Loading