From bf37d5e112985973d7a3290508ad6b38f3c9ec84 Mon Sep 17 00:00:00 2001 From: Jamie Date: Fri, 2 Aug 2019 16:37:07 +0100 Subject: [PATCH 1/8] Adding Drift awareness for ALB Target Group Attachments Added - Check for drained/unused instances to show the diff in plans if this is the case. - Tests --- ...resource_aws_lb_target_group_attachment.go | 11 ++++ ...rce_aws_lb_target_group_attachment_test.go | 56 +++++++++++++++++-- 2 files changed, 63 insertions(+), 4 deletions(-) diff --git a/aws/resource_aws_lb_target_group_attachment.go b/aws/resource_aws_lb_target_group_attachment.go index d845a1d4020..3e6869ae327 100644 --- a/aws/resource_aws_lb_target_group_attachment.go +++ b/aws/resource_aws_lb_target_group_attachment.go @@ -126,6 +126,7 @@ func resourceAwsLbAttachmentRead(d *schema.ResourceData, meta interface{}) error TargetGroupArn: aws.String(d.Get("target_group_arn").(string)), Targets: []*elbv2.TargetDescription{target}, }) + if err != nil { if isAWSErr(err, elbv2.ErrCodeTargetGroupNotFoundException, "") { log.Printf("[WARN] Target group does not exist, removing target attachment %s", d.Id()) @@ -140,6 +141,16 @@ func resourceAwsLbAttachmentRead(d *schema.ResourceData, meta interface{}) error return fmt.Errorf("Error reading Target Health: %s", err) } + for _, targetDesc := range resp.TargetHealthDescriptions { + if *targetDesc.Target.Id == d.Get("target_id").(string) { + if (*targetDesc.TargetHealth.State == "unused") || (*targetDesc.TargetHealth.State == "draining") { + log.Printf("[WARN] Target Attachment does not exist, recreating attachment") + d.SetId("") + return nil + } + } + } + if len(resp.TargetHealthDescriptions) != 1 { log.Printf("[WARN] Target does not exist, removing target attachment %s", d.Id()) d.SetId("") diff --git a/aws/resource_aws_lb_target_group_attachment_test.go b/aws/resource_aws_lb_target_group_attachment_test.go index 584eaa30b20..9932c00918a 100644 --- a/aws/resource_aws_lb_target_group_attachment_test.go +++ b/aws/resource_aws_lb_target_group_attachment_test.go @@ -32,6 +32,26 @@ func TestAccAWSLBTargetGroupAttachment_basic(t *testing.T) { }) } +func TestAccAWSLBTargetGroupAttachment_missingAttachmentDrift(t *testing.T) { + targetGroupName := fmt.Sprintf("test-target-group-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)) + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + IDRefreshName: "aws_lb_target_group.test", + Providers: testAccProviders, + CheckDestroy: testAccCheckAWSLBTargetGroupAttachmentDestroy, + Steps: []resource.TestStep{ + { + Config: testAccAWSLBTargetGroupAttachmentConfig_basic(targetGroupName), + Check: resource.ComposeTestCheckFunc( + testAccCheckAWSLBTargetGroupAttachmentExists("aws_lb_target_group_attachment.test"), + deregisterTarget("aws_lb_target_group.test", "aws_instance.test"), + ), + ExpectNonEmptyPlan: true, + }, + }, + }) +} + func TestAccAWSLBTargetGroupAttachmentBackwardsCompatibility(t *testing.T) { targetGroupName := fmt.Sprintf("test-target-group-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)) @@ -108,6 +128,34 @@ func TestAccAWSALBTargetGroupAttachment_lambda(t *testing.T) { }) } +func deregisterTarget(attachmentAddress string, instanceAddress string) resource.TestCheckFunc { + return func(s *terraform.State) error { + attachmentRs, ok := s.RootModule().Resources[attachmentAddress] + if !ok { + return fmt.Errorf("Attachment not found: %s", attachmentAddress) + } + instanceRs, ok := s.RootModule().Resources[instanceAddress] + if !ok { + return fmt.Errorf("Instance not found: %s", instanceAddress) + } + + conn := testAccProvider.Meta().(*AWSClient).elbv2conn + + targetGroupArn := attachmentRs.Primary.Attributes["arn"] + instanceId := instanceRs.Primary.Attributes["id"] + + _, err := conn.DeregisterTargets(&elbv2.DeregisterTargetsInput{ + TargetGroupArn: aws.String(targetGroupArn), + Targets: []*elbv2.TargetDescription{ + { + Id: aws.String(instanceId), + }, + }, + }) + return err + } +} + func testAccCheckAWSLBTargetGroupAttachmentExists(n string) resource.TestCheckFunc { return func(s *terraform.State) error { rs, ok := s.RootModule().Resources[n] @@ -197,7 +245,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-f701cb97" + ami = "ami-0d8e27447ec2c8410" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -254,7 +302,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-f701cb97" + ami = "ami-0d8e27447ec2c8410" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -311,7 +359,7 @@ resource "aws_alb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-f701cb97" + ami = "ami-0d8e27447ec2c8410" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -368,7 +416,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-f701cb97" + ami = "ami-0d8e27447ec2c8410" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } From d8047d038e266a391afba3130648438516b773aa Mon Sep 17 00:00:00 2001 From: Jamie Date: Fri, 2 Aug 2019 16:38:21 +0100 Subject: [PATCH 2/8] Flipping AMIs back in Tests --- aws/resource_aws_lb_target_group_attachment_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/aws/resource_aws_lb_target_group_attachment_test.go b/aws/resource_aws_lb_target_group_attachment_test.go index 9932c00918a..51e7b9d6b58 100644 --- a/aws/resource_aws_lb_target_group_attachment_test.go +++ b/aws/resource_aws_lb_target_group_attachment_test.go @@ -245,7 +245,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-0d8e27447ec2c8410" + ami = "ami-f701cb97" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -302,7 +302,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-0d8e27447ec2c8410" + ami = "ami-f701cb97" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -359,7 +359,7 @@ resource "aws_alb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-0d8e27447ec2c8410" + ami = "ami-f701cb97" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -416,7 +416,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-0d8e27447ec2c8410" + ami = "ami-f701cb97" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } From 83ae4250b89266078e6ffa360148f4aab32247bb Mon Sep 17 00:00:00 2001 From: Jamie Date: Fri, 2 Aug 2019 17:26:31 +0100 Subject: [PATCH 3/8] Updating logic for determining target state Fixed - State now determiend based on reason to avoid conflict with attached but unused. --- aws/resource_aws_lb_target_group_attachment.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/aws/resource_aws_lb_target_group_attachment.go b/aws/resource_aws_lb_target_group_attachment.go index 3e6869ae327..6f5ec420eda 100644 --- a/aws/resource_aws_lb_target_group_attachment.go +++ b/aws/resource_aws_lb_target_group_attachment.go @@ -143,7 +143,10 @@ func resourceAwsLbAttachmentRead(d *schema.ResourceData, meta interface{}) error for _, targetDesc := range resp.TargetHealthDescriptions { if *targetDesc.Target.Id == d.Get("target_id").(string) { - if (*targetDesc.TargetHealth.State == "unused") || (*targetDesc.TargetHealth.State == "draining") { + // These will catch targets being removed by hand (draining as we plan) or that have been removed for a while + // without trying to re-create ones that are just not in use. For example, a target can be `unused` if the + // target group isnt assigned to anything, a scenario where we don't want to continuously recreate the resource. + if (*targetDesc.TargetHealth.Reason == "Target.NotRegistered") || (*targetDesc.TargetHealth.Reason == "Target.DeregistrationInProgress") { log.Printf("[WARN] Target Attachment does not exist, recreating attachment") d.SetId("") return nil From 2703afb13eea4ed258bdec2ea24de85bb1d60d86 Mon Sep 17 00:00:00 2001 From: Jamie Date: Fri, 2 Aug 2019 20:25:20 +0100 Subject: [PATCH 4/8] Updating logic for determining target state Fixed - State now determiend based on reason to avoid conflict with attached but unused. --- ...rce_aws_lb_target_group_attachment_test.go | 45 ++++++++++--------- 1 file changed, 25 insertions(+), 20 deletions(-) diff --git a/aws/resource_aws_lb_target_group_attachment_test.go b/aws/resource_aws_lb_target_group_attachment_test.go index 51e7b9d6b58..2de6a61ae89 100644 --- a/aws/resource_aws_lb_target_group_attachment_test.go +++ b/aws/resource_aws_lb_target_group_attachment_test.go @@ -3,14 +3,13 @@ package aws import ( "errors" "fmt" - "strconv" - "testing" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/elbv2" "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" + "strconv" + "testing" ) func TestAccAWSLBTargetGroupAttachment_basic(t *testing.T) { @@ -44,7 +43,7 @@ func TestAccAWSLBTargetGroupAttachment_missingAttachmentDrift(t *testing.T) { Config: testAccAWSLBTargetGroupAttachmentConfig_basic(targetGroupName), Check: resource.ComposeTestCheckFunc( testAccCheckAWSLBTargetGroupAttachmentExists("aws_lb_target_group_attachment.test"), - deregisterTarget("aws_lb_target_group.test", "aws_instance.test"), + deregisterTarget("aws_lb_target_group_attachment.test"), ), ExpectNonEmptyPlan: true, }, @@ -128,30 +127,36 @@ func TestAccAWSALBTargetGroupAttachment_lambda(t *testing.T) { }) } -func deregisterTarget(attachmentAddress string, instanceAddress string) resource.TestCheckFunc { +func deregisterTarget(n string) resource.TestCheckFunc { return func(s *terraform.State) error { - attachmentRs, ok := s.RootModule().Resources[attachmentAddress] - if !ok { - return fmt.Errorf("Attachment not found: %s", attachmentAddress) - } - instanceRs, ok := s.RootModule().Resources[instanceAddress] + rs, ok := s.RootModule().Resources[n] if !ok { - return fmt.Errorf("Instance not found: %s", instanceAddress) + return fmt.Errorf("Attachment not found: %s", n) } conn := testAccProvider.Meta().(*AWSClient).elbv2conn + targetGroupArn := rs.Primary.Attributes["target_group_arn"] + + target := &elbv2.TargetDescription{ + Id: aws.String(rs.Primary.Attributes["target_id"]), + } - targetGroupArn := attachmentRs.Primary.Attributes["arn"] - instanceId := instanceRs.Primary.Attributes["id"] + _, hasPort := rs.Primary.Attributes["port"] + if hasPort { + port, _ := strconv.Atoi(rs.Primary.Attributes["port"]) + target.Port = aws.Int64(int64(port)) + } - _, err := conn.DeregisterTargets(&elbv2.DeregisterTargetsInput{ + params := &elbv2.DeregisterTargetsInput{ TargetGroupArn: aws.String(targetGroupArn), - Targets: []*elbv2.TargetDescription{ - { - Id: aws.String(instanceId), - }, - }, - }) + Targets: []*elbv2.TargetDescription{target}, + } + + _, err := conn.DeregisterTargets(params) + if err != nil && !isAWSErr(err, elbv2.ErrCodeTargetGroupNotFoundException, "") { + return fmt.Errorf("Error deregistering Targets: %s", err) + } + return err } } From 7d6489f21c3c32fb4186f2ccb5470b220cc44c6e Mon Sep 17 00:00:00 2001 From: Jamie Date: Fri, 2 Aug 2019 20:27:54 +0100 Subject: [PATCH 5/8] Reordering imports back to original, auto linter refactored them. --- aws/resource_aws_lb_target_group_attachment_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/aws/resource_aws_lb_target_group_attachment_test.go b/aws/resource_aws_lb_target_group_attachment_test.go index 2de6a61ae89..6f078694980 100644 --- a/aws/resource_aws_lb_target_group_attachment_test.go +++ b/aws/resource_aws_lb_target_group_attachment_test.go @@ -3,13 +3,14 @@ package aws import ( "errors" "fmt" + "strconv" + "testing" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/elbv2" "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" - "strconv" - "testing" + ) func TestAccAWSLBTargetGroupAttachment_basic(t *testing.T) { From 29cf4a1390ade779eb577983ddeac3d10f891a4a Mon Sep 17 00:00:00 2001 From: Jamie Date: Fri, 2 Aug 2019 20:33:38 +0100 Subject: [PATCH 6/8] Applying gofmt fixes --- aws/resource_aws_lb_target_group_attachment.go | 3 +-- aws/resource_aws_lb_target_group_attachment_test.go | 5 ++--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/aws/resource_aws_lb_target_group_attachment.go b/aws/resource_aws_lb_target_group_attachment.go index 6f5ec420eda..f1a09383f67 100644 --- a/aws/resource_aws_lb_target_group_attachment.go +++ b/aws/resource_aws_lb_target_group_attachment.go @@ -2,12 +2,11 @@ package aws import ( "fmt" - "log" - "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/elbv2" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" + "log" ) func resourceAwsLbTargetGroupAttachment() *schema.Resource { diff --git a/aws/resource_aws_lb_target_group_attachment_test.go b/aws/resource_aws_lb_target_group_attachment_test.go index 6f078694980..2de6a61ae89 100644 --- a/aws/resource_aws_lb_target_group_attachment_test.go +++ b/aws/resource_aws_lb_target_group_attachment_test.go @@ -3,14 +3,13 @@ package aws import ( "errors" "fmt" - "strconv" - "testing" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/elbv2" "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" - + "strconv" + "testing" ) func TestAccAWSLBTargetGroupAttachment_basic(t *testing.T) { From ba8064ca76ad13736b7e17afa4a167ff23ce4e35 Mon Sep 17 00:00:00 2001 From: Jamie Date: Sun, 11 Aug 2019 14:50:02 +0100 Subject: [PATCH 7/8] Applying requested changed from review Added - Nil check handling Fixed - Now using AWS SDK strings rather than hard coded --- aws/resource_aws_lb_target_group_attachment.go | 15 +++++++++++++-- ...esource_aws_lb_target_group_attachment_test.go | 13 +++++++------ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/aws/resource_aws_lb_target_group_attachment.go b/aws/resource_aws_lb_target_group_attachment.go index f1a09383f67..66083b8bff3 100644 --- a/aws/resource_aws_lb_target_group_attachment.go +++ b/aws/resource_aws_lb_target_group_attachment.go @@ -2,11 +2,12 @@ package aws import ( "fmt" + "log" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/elbv2" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" - "log" ) func resourceAwsLbTargetGroupAttachment() *schema.Resource { @@ -141,11 +142,21 @@ func resourceAwsLbAttachmentRead(d *schema.ResourceData, meta interface{}) error } for _, targetDesc := range resp.TargetHealthDescriptions { + if targetDesc == nil || targetDesc.Target == nil { + continue + } + if *targetDesc.Target.Id == d.Get("target_id").(string) { // These will catch targets being removed by hand (draining as we plan) or that have been removed for a while // without trying to re-create ones that are just not in use. For example, a target can be `unused` if the // target group isnt assigned to anything, a scenario where we don't want to continuously recreate the resource. - if (*targetDesc.TargetHealth.Reason == "Target.NotRegistered") || (*targetDesc.TargetHealth.Reason == "Target.DeregistrationInProgress") { + if targetDesc.TargetHealth == nil { + continue + } + + reason := aws.StringValue(targetDesc.TargetHealth.Reason) + + if reason == elbv2.TargetHealthReasonEnumTargetNotRegistered || reason == elbv2.TargetHealthReasonEnumTargetDeregistrationInProgress { log.Printf("[WARN] Target Attachment does not exist, recreating attachment") d.SetId("") return nil diff --git a/aws/resource_aws_lb_target_group_attachment_test.go b/aws/resource_aws_lb_target_group_attachment_test.go index 2de6a61ae89..c53877d6e45 100644 --- a/aws/resource_aws_lb_target_group_attachment_test.go +++ b/aws/resource_aws_lb_target_group_attachment_test.go @@ -3,13 +3,14 @@ package aws import ( "errors" "fmt" + "strconv" + "testing" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/elbv2" "github.com/hashicorp/terraform/helper/acctest" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" - "strconv" - "testing" ) func TestAccAWSLBTargetGroupAttachment_basic(t *testing.T) { @@ -250,7 +251,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-f701cb97" + ami = "ami-0bdfa1adc3878cd23" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -307,7 +308,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-f701cb97" + ami = "ami-0bdfa1adc3878cd23" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -364,7 +365,7 @@ resource "aws_alb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-f701cb97" + ami = "ami-0bdfa1adc3878cd23" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -421,7 +422,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-f701cb97" + ami = "ami-0bdfa1adc3878cd23" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } From d61259059115e6bc7019fe1663b0783c4fbadb3e Mon Sep 17 00:00:00 2001 From: Jamie Date: Sun, 11 Aug 2019 15:02:08 +0100 Subject: [PATCH 8/8] Reverting AMIs back to original, were changed to make running tests in my environment easier --- aws/resource_aws_lb_target_group_attachment_test.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/aws/resource_aws_lb_target_group_attachment_test.go b/aws/resource_aws_lb_target_group_attachment_test.go index c53877d6e45..f2bb36f58b2 100644 --- a/aws/resource_aws_lb_target_group_attachment_test.go +++ b/aws/resource_aws_lb_target_group_attachment_test.go @@ -251,7 +251,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-0bdfa1adc3878cd23" + ami = "ami-f701cb97" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -308,7 +308,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-0bdfa1adc3878cd23" + ami = "ami-f701cb97" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -365,7 +365,7 @@ resource "aws_alb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-0bdfa1adc3878cd23" + ami = "ami-f701cb97" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" } @@ -422,7 +422,7 @@ resource "aws_lb_target_group_attachment" "test" { } resource "aws_instance" "test" { - ami = "ami-0bdfa1adc3878cd23" + ami = "ami-f701cb97" instance_type = "t2.micro" subnet_id = "${aws_subnet.subnet.id}" }