From 2c3582e44b2c674be7c17100a72c14236f925e50 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 6 Feb 2024 14:52:19 -0800 Subject: [PATCH 1/6] Revert "Removes unneeded `DiffSupressFunc`s" This reverts commit 24c51eb6600d9a89c4772897cbe6c86737fac7a0. --- internal/service/elbv2/listener.go | 64 +++++++++++++++++-------- internal/service/elbv2/listener_rule.go | 55 +++++++++++++++------ 2 files changed, 84 insertions(+), 35 deletions(-) diff --git a/internal/service/elbv2/listener.go b/internal/service/elbv2/listener.go index 159d62abf77..a77f8245898 100644 --- a/internal/service/elbv2/listener.go +++ b/internal/service/elbv2/listener.go @@ -75,9 +75,10 @@ func ResourceListener() *schema.Resource { Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "authenticate_cognito": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, + Type: schema.TypeList, + Optional: true, + DiffSuppressFunc: suppressIfDefaultActionTypeNot(awstypes.ActionTypeEnumAuthenticateCognito), + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "authentication_request_extra_params": { @@ -123,9 +124,10 @@ func ResourceListener() *schema.Resource { }, }, "authenticate_oidc": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, + Type: schema.TypeList, + Optional: true, + DiffSuppressFunc: suppressIfDefaultActionTypeNot(awstypes.ActionTypeEnumAuthenticateOidc), + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "authentication_request_extra_params": { @@ -183,9 +185,10 @@ func ResourceListener() *schema.Resource { }, }, "fixed_response": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, + Type: schema.TypeList, + Optional: true, + DiffSuppressFunc: suppressIfDefaultActionTypeNot(awstypes.ActionTypeEnumFixedResponse), + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "content_type": { @@ -214,11 +217,15 @@ func ResourceListener() *schema.Resource { }, }, "forward": { - Type: schema.TypeList, - Optional: true, - DiffSuppressOnRefresh: true, - DiffSuppressFunc: diffSuppressMissingForward("default_action"), - MaxItems: 1, + // Type: schema.TypeList, + // Optional: true, + // DiffSuppressOnRefresh: true, + // DiffSuppressFunc: diffSuppressMissingForward("default_action"), + // MaxItems: 1, + Type: schema.TypeList, + Optional: true, + DiffSuppressFunc: suppressIfDefaultActionTypeNot(awstypes.ActionTypeEnumForward), + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "target_group": { @@ -272,9 +279,10 @@ func ResourceListener() *schema.Resource { ValidateFunc: validation.IntBetween(listenerActionOrderMin, listenerActionOrderMax), }, "redirect": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, + Type: schema.TypeList, + Optional: true, + DiffSuppressFunc: suppressIfDefaultActionTypeNot(awstypes.ActionTypeEnumRedirect), + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "host": { @@ -319,9 +327,10 @@ func ResourceListener() *schema.Resource { }, }, "target_group_arn": { - Type: schema.TypeString, - Optional: true, - ValidateFunc: verify.ValidARN, + Type: schema.TypeString, + Optional: true, + DiffSuppressFunc: suppressIfDefaultActionTypeNot(awstypes.ActionTypeEnumForward), + ValidateFunc: verify.ValidARN, }, "type": { Type: schema.TypeString, @@ -393,6 +402,21 @@ func ResourceListener() *schema.Resource { } } +func suppressIfDefaultActionTypeNot(t awstypes.ActionTypeEnum) schema.SchemaDiffSuppressFunc { + return func(k, old, new string, d *schema.ResourceData) bool { + take := 2 + i := strings.IndexFunc(k, func(r rune) bool { + if r == '.' { + take -= 1 + return take == 0 + } + return false + }) + at := k[:i+1] + "type" + return awstypes.ActionTypeEnum(d.Get(at).(string)) != t + } +} + func resourceListenerCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { var diags diag.Diagnostics conn := meta.(*conns.AWSClient).ELBV2Client(ctx) diff --git a/internal/service/elbv2/listener_rule.go b/internal/service/elbv2/listener_rule.go index 047ee68dee7..49beb691d62 100644 --- a/internal/service/elbv2/listener_rule.go +++ b/internal/service/elbv2/listener_rule.go @@ -10,6 +10,7 @@ import ( "log" "sort" "strconv" + "strings" "time" "github.com/YakDriver/regexache" @@ -92,12 +93,17 @@ func ResourceListenerRule() *schema.Resource { }, "target_group_arn": { - Type: schema.TypeString, - Optional: true, - ValidateFunc: verify.ValidARN, + Type: schema.TypeString, + Optional: true, + DiffSuppressFunc: suppressIfActionTypeNot(awstypes.ActionTypeEnumForward), + ValidateFunc: verify.ValidARN, }, "forward": { + // Type: schema.TypeList, + // Optional: true, + // DiffSuppressFunc: suppressIfActionTypeNot(awstypes.ActionTypeEnumForward), + // MaxItems: 1, Type: schema.TypeList, Optional: true, DiffSuppressOnRefresh: true, @@ -151,9 +157,10 @@ func ResourceListenerRule() *schema.Resource { }, "redirect": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, + Type: schema.TypeList, + Optional: true, + DiffSuppressFunc: suppressIfActionTypeNot(awstypes.ActionTypeEnumRedirect), + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "host": { @@ -204,9 +211,10 @@ func ResourceListenerRule() *schema.Resource { }, "fixed_response": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, + Type: schema.TypeList, + Optional: true, + DiffSuppressFunc: suppressIfActionTypeNot(awstypes.ActionTypeEnumFixedResponse), + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "content_type": { @@ -238,9 +246,10 @@ func ResourceListenerRule() *schema.Resource { }, "authenticate_cognito": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, + Type: schema.TypeList, + Optional: true, + DiffSuppressFunc: suppressIfActionTypeNot(awstypes.ActionTypeEnumAuthenticateCognito), + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "authentication_request_extra_params": { @@ -287,9 +296,10 @@ func ResourceListenerRule() *schema.Resource { }, "authenticate_oidc": { - Type: schema.TypeList, - Optional: true, - MaxItems: 1, + Type: schema.TypeList, + Optional: true, + DiffSuppressFunc: suppressIfActionTypeNot(awstypes.ActionTypeEnumAuthenticateOidc), + MaxItems: 1, Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "authentication_request_extra_params": { @@ -481,6 +491,21 @@ func ResourceListenerRule() *schema.Resource { } } +func suppressIfActionTypeNot(t awstypes.ActionTypeEnum) schema.SchemaDiffSuppressFunc { + return func(k, old, new string, d *schema.ResourceData) bool { + take := 2 + i := strings.IndexFunc(k, func(r rune) bool { + if r == '.' { + take -= 1 + return take == 0 + } + return false + }) + at := k[:i+1] + "type" + return awstypes.ActionTypeEnum(d.Get(at).(string)) != t + } +} + func resourceListenerRuleCreate(ctx context.Context, d *schema.ResourceData, meta interface{}) diag.Diagnostics { var diags diag.Diagnostics conn := meta.(*conns.AWSClient).ELBV2Client(ctx) From 1b173f9b46df85ef31c4cb40f297867edfdf6c6e Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 6 Feb 2024 15:52:54 -0800 Subject: [PATCH 2/6] Adds test for invalid combination of `target_group_arn` with `type` other than `forward` --- internal/service/elbv2/listener_rule_test.go | 155 +++++++++++++++++++ internal/service/elbv2/listener_test.go | 99 ++++++++++++ 2 files changed, 254 insertions(+) diff --git a/internal/service/elbv2/listener_rule_test.go b/internal/service/elbv2/listener_rule_test.go index f28fc7c62ca..8d624b8306c 100644 --- a/internal/service/elbv2/listener_rule_test.go +++ b/internal/service/elbv2/listener_rule_test.go @@ -1510,6 +1510,48 @@ func TestAccELBV2ListenerRule_EmptyAction(t *testing.T) { } } +func TestAccELBV2ListenerRule_redirectWithTargetGroupARN(t *testing.T) { + ctx := acctest.Context(t) + var conf elbv2.Rule + lbName := fmt.Sprintf("testrule-redirect-%s", sdkacctest.RandString(14)) + + resourceName := "aws_lb_listener_rule.static" + frontEndListenerResourceName := "aws_lb_listener.front_end" + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, tfelbv2.AwsSdkId), + CheckDestroy: testAccCheckListenerRuleDestroy(ctx), + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.34.0", + }, + }, + Config: testAccListenerRuleConfig_redirectWithTargetGroupARN(lbName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckListenerRuleExists(ctx, resourceName, &conf), + acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "elasticloadbalancing", regexache.MustCompile(fmt.Sprintf(`listener-rule/app/%s/.+$`, lbName))), + resource.TestCheckResourceAttrPair(resourceName, "listener_arn", frontEndListenerResourceName, "arn"), + resource.TestCheckResourceAttr(resourceName, "action.#", "1"), + resource.TestCheckResourceAttr(resourceName, "action.0.type", "redirect"), + resource.TestCheckResourceAttr(resourceName, "action.0.redirect.#", "1"), + resource.TestCheckResourceAttr(resourceName, "action.0.forward.#", "0"), + resource.TestCheckResourceAttr(resourceName, "action.0.target_group_arn", ""), + ), + }, + { + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + Config: testAccListenerRuleConfig_redirectWithTargetGroupARN(lbName), + PlanOnly: true, + ExpectNonEmptyPlan: false, + }, + }, + }) +} + func TestAccELBV2ListenerRule_conditionAttributesCount(t *testing.T) { ctx := acctest.Context(t) err_many := regexache.MustCompile("Only one of host_header, http_header, http_request_method, path_pattern, query_string or source_ip can be set in a condition block") @@ -4318,6 +4360,119 @@ resource "aws_security_group" "test" { `, rName, action) } +func testAccListenerRuleConfig_redirectWithTargetGroupARN(lbName string) string { + return fmt.Sprintf(` +resource "aws_lb_listener_rule" "static" { + listener_arn = aws_lb_listener.front_end.arn + priority = 100 + + action { + type = "redirect" + + redirect { + port = "443" + protocol = "HTTPS" + status_code = "HTTP_301" + } + } + + condition { + path_pattern { + values = ["/static/*"] + } + } +} + +resource "aws_lb_listener" "front_end" { + load_balancer_arn = aws_lb.alb_test.id + protocol = "HTTP" + port = "80" + + default_action { + type = "redirect" + + redirect { + port = "443" + protocol = "HTTPS" + status_code = "HTTP_301" + } + } +} + +resource "aws_lb" "alb_test" { + name = %[1]q + internal = true + security_groups = [aws_security_group.alb_test.id] + subnets = aws_subnet.alb_test[*].id + + idle_timeout = 30 + enable_deletion_protection = false + + tags = { + Name = %[1]q + } +} + +variable "subnets" { + default = ["10.0.1.0/24", "10.0.2.0/24"] + type = list(string) +} + +data "aws_availability_zones" "available" { + state = "available" + + filter { + name = "opt-in-status" + values = ["opt-in-not-required"] + } +} + +resource "aws_vpc" "alb_test" { + cidr_block = "10.0.0.0/16" + + tags = { + Name = "terraform-testacc-lb-listener-rule-redirect" + } +} + +resource "aws_subnet" "alb_test" { + count = 2 + vpc_id = aws_vpc.alb_test.id + cidr_block = element(var.subnets, count.index) + map_public_ip_on_launch = true + availability_zone = element(data.aws_availability_zones.available.names, count.index) + + tags = { + Name = "tf-acc-lb-listener-rule-redirect-${count.index}" + } +} + +resource "aws_security_group" "alb_test" { + name = "allow_all_alb_test" + description = "Used for ALB Testing" + vpc_id = aws_vpc.alb_test.id + + ingress { + from_port = 0 + to_port = 0 + protocol = "-1" + cidr_blocks = ["0.0.0.0/0"] + } + + egress { + from_port = 0 + to_port = 0 + protocol = "-1" + cidr_blocks = ["0.0.0.0/0"] + } + + tags = { + Name = %[1]q + } +} +`, lbName) +} + func testAccListenerRuleConfig_condition_error(condition string) string { return fmt.Sprintf(` data "aws_partition" "current" {} diff --git a/internal/service/elbv2/listener_test.go b/internal/service/elbv2/listener_test.go index ce31802724d..f35437faa52 100644 --- a/internal/service/elbv2/listener_test.go +++ b/internal/service/elbv2/listener_test.go @@ -1636,6 +1636,48 @@ func TestAccELBV2Listener_EmptyDefaultAction(t *testing.T) { } } +func TestAccELBV2Listener_redirectWithTargetGroupARN(t *testing.T) { + ctx := acctest.Context(t) + var conf awstypes.Listener + resourceName := "aws_lb_listener.test" + rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + + resource.ParallelTest(t, resource.TestCase{ + PreCheck: func() { acctest.PreCheck(ctx, t) }, + ErrorCheck: acctest.ErrorCheck(t, tfelbv2.AwsSdkId), + CheckDestroy: testAccCheckListenerDestroy(ctx), + Steps: []resource.TestStep{ + { + ExternalProviders: map[string]resource.ExternalProvider{ + "aws": { + Source: "hashicorp/aws", + VersionConstraint: "5.34.0", + }, + }, + Config: testAccListenerConfig_redirectWithTargetGroupARN(rName), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckListenerExists(ctx, resourceName, &conf), + resource.TestCheckResourceAttrPair(resourceName, "load_balancer_arn", "aws_lb.test", "arn"), + acctest.MatchResourceAttrRegionalARN(resourceName, "arn", "elasticloadbalancing", regexache.MustCompile("listener/.+$")), + resource.TestCheckResourceAttr(resourceName, "protocol", "HTTP"), + resource.TestCheckResourceAttr(resourceName, "port", "80"), + resource.TestCheckResourceAttr(resourceName, "default_action.#", "1"), + resource.TestCheckResourceAttr(resourceName, "default_action.0.type", "redirect"), + resource.TestCheckResourceAttr(resourceName, "default_action.0.redirect.#", "1"), + resource.TestCheckResourceAttr(resourceName, "default_action.0.forward.#", "0"), + resource.TestCheckResourceAttr(resourceName, "default_action.0.target_group_arn", ""), + ), + }, + { + ProtoV5ProviderFactories: acctest.ProtoV5ProviderFactories, + Config: testAccListenerConfig_redirectWithTargetGroupARN(rName), + PlanOnly: true, + ExpectNonEmptyPlan: false, + }, + }, + }) +} + func testAccCheckListenerDefaultActionOrderDisappears(ctx context.Context, listener *awstypes.Listener, actionOrderToDelete int) resource.TestCheckFunc { return func(s *terraform.State) error { var newDefaultActions []awstypes.Action @@ -3706,3 +3748,60 @@ resource "aws_lb_target_group" "test" { } `, rName, action)) } + +func testAccListenerConfig_redirectWithTargetGroupARN(rName string) string { + return acctest.ConfigCompose(testAccListenerConfig_base(rName), fmt.Sprintf(` +resource "aws_lb_listener" "test" { + load_balancer_arn = aws_lb.test.id + protocol = "HTTP" + port = "80" + + default_action { + target_group_arn = aws_lb_target_group.test.arn + type = "redirect" + + redirect { + port = "443" + protocol = "HTTPS" + status_code = "HTTP_301" + } + } +} + +resource "aws_lb" "test" { + name = %[1]q + internal = true + security_groups = [aws_security_group.test.id] + subnets = aws_subnet.test[*].id + + idle_timeout = 30 + enable_deletion_protection = false + + tags = { + Name = %[1]q + } +} + +resource "aws_lb_target_group" "test" { + name = %[1]q + port = 8080 + protocol = "HTTP" + vpc_id = aws_vpc.test.id + + health_check { + path = "/health" + interval = 60 + port = 8081 + protocol = "HTTP" + timeout = 3 + healthy_threshold = 3 + unhealthy_threshold = 3 + matcher = "200-299" + } + + tags = { + Name = %[1]q + } +} +`, rName)) +} From b74960233ec2b68d05def85b5008fe7631be02a0 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 6 Feb 2024 16:03:18 -0800 Subject: [PATCH 3/6] Convert `aws_lb_listener_rule` tests to AWS SDK for Go v2 --- internal/service/elbv2/listener_rule_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/service/elbv2/listener_rule_test.go b/internal/service/elbv2/listener_rule_test.go index 8d624b8306c..e5b9ec36b17 100644 --- a/internal/service/elbv2/listener_rule_test.go +++ b/internal/service/elbv2/listener_rule_test.go @@ -1512,7 +1512,7 @@ func TestAccELBV2ListenerRule_EmptyAction(t *testing.T) { func TestAccELBV2ListenerRule_redirectWithTargetGroupARN(t *testing.T) { ctx := acctest.Context(t) - var conf elbv2.Rule + var conf awstypes.Rule lbName := fmt.Sprintf("testrule-redirect-%s", sdkacctest.RandString(14)) resourceName := "aws_lb_listener_rule.static" From c29cbe1297badaef13320935e02a92965d914aab Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Tue, 6 Feb 2024 16:38:47 -0800 Subject: [PATCH 4/6] `terrafmt` --- internal/service/elbv2/listener_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/service/elbv2/listener_test.go b/internal/service/elbv2/listener_test.go index f35437faa52..0830e2b8ab4 100644 --- a/internal/service/elbv2/listener_test.go +++ b/internal/service/elbv2/listener_test.go @@ -3757,8 +3757,8 @@ resource "aws_lb_listener" "test" { port = "80" default_action { - target_group_arn = aws_lb_target_group.test.arn - type = "redirect" + target_group_arn = aws_lb_target_group.test.arn + type = "redirect" redirect { port = "443" From e11f3b8ec69f16cd54127ad2997a986f19b2d004 Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Wed, 7 Feb 2024 13:24:06 -0800 Subject: [PATCH 5/6] Cleanup --- internal/service/elbv2/listener.go | 5 ----- internal/service/elbv2/listener_rule.go | 4 ---- 2 files changed, 9 deletions(-) diff --git a/internal/service/elbv2/listener.go b/internal/service/elbv2/listener.go index a77f8245898..5b358cc7683 100644 --- a/internal/service/elbv2/listener.go +++ b/internal/service/elbv2/listener.go @@ -217,11 +217,6 @@ func ResourceListener() *schema.Resource { }, }, "forward": { - // Type: schema.TypeList, - // Optional: true, - // DiffSuppressOnRefresh: true, - // DiffSuppressFunc: diffSuppressMissingForward("default_action"), - // MaxItems: 1, Type: schema.TypeList, Optional: true, DiffSuppressFunc: suppressIfDefaultActionTypeNot(awstypes.ActionTypeEnumForward), diff --git a/internal/service/elbv2/listener_rule.go b/internal/service/elbv2/listener_rule.go index 49beb691d62..3943c4e7ddf 100644 --- a/internal/service/elbv2/listener_rule.go +++ b/internal/service/elbv2/listener_rule.go @@ -100,10 +100,6 @@ func ResourceListenerRule() *schema.Resource { }, "forward": { - // Type: schema.TypeList, - // Optional: true, - // DiffSuppressFunc: suppressIfActionTypeNot(awstypes.ActionTypeEnumForward), - // MaxItems: 1, Type: schema.TypeList, Optional: true, DiffSuppressOnRefresh: true, From 7a3920f5c08d3f0cf7811c0c7c3157cb6d0e15bf Mon Sep 17 00:00:00 2001 From: Graham Davison Date: Wed, 7 Feb 2024 13:26:17 -0800 Subject: [PATCH 6/6] Adds CHANGELOG entry --- .changelog/35678.txt | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 .changelog/35678.txt diff --git a/.changelog/35678.txt b/.changelog/35678.txt new file mode 100644 index 00000000000..106f298c020 --- /dev/null +++ b/.changelog/35678.txt @@ -0,0 +1,7 @@ +```release-note:bug +resource/aws_lb_listener: Fixes unexpected diff when using `default_action` parameters which don't match the `type`. +``` + +```release-note:bug +resource/aws_lb_listener_rule: Fixes unexpected diff when using `action` parameters which don't match the `type`. +```