From 5008597d9b7a0c5308aefec7b020da46b8692d6e Mon Sep 17 00:00:00 2001 From: Jared Baker Date: Wed, 6 Nov 2024 16:54:29 -0500 Subject: [PATCH 1/3] r/aws_lb_listener: remove `tcp_idle_timeout_seconds` default The `tcp_idle_timeout_seconds` default value of `350` has been removed in favor or marking this argument optional and computed. In practice, this means the default value of `350` is still read and stored in state for protocols which support this feature, but unless explicitly configured by the user, the `ModifyListenerAttributes` API will not be called during create or update operations. This change fixes a regression introduced by the addition of the `tcp_idle_timeout_seconds` argument with a default value, which caused any existing configuration using the `aws_lb_listener` resource to now require the calling principal to include the `elasticloadbalancing:ModifyLoadBalancerAttributes` IAM action in an attached IAM policy. By only sending this value when explicitly configured, existing configurations can continue to operate as expected with no modifications to the IAM permissions of the calling principal. To confirm the API is no longer being called when `tcp_idle_timeout_seconds` is not explicitly configured, I've grepped the debug logs of the same configuration applied with `v5.74.0` of the AWS provider and a locally built binary from this branch. The former includes a result with the API request body where the TCP idle timeout is set to the default of `350`. The latter includes no result indicating the `ModifyListenerAttributes` API was not called, as expected. __`v5.74.0`__: ```console % rg "Action=ModifyListenerAttributes" apply-old.out 2672: | Action=ModifyListenerAttributes&Attributes.member.1.Key=tcp.idle_timeout.seconds&Attributes.member.1.Value=350&ListenerArn=arn%3Aaws%3Aelasticloadbalancing%3Aus-west-2%3A%3Alistener%2Fnet%2Fjb-test%2Fb0f926497af0c80f%2F819828f77724007f&Version=2015-12-01 ``` __This branch__: ```console % rg "Action=ModifyListenerAttributes" apply.out ``` ```console % make testacc PKG=elbv2 TESTARGS="-run=TestAccELBV2Listener_.*_basic" make: Verifying source code with gofmt... ==> Checking that code complies with gofmt requirements... TF_ACC=1 go1.23.2 test ./internal/service/elbv2/... -v -count 1 -parallel 20 -run=TestAccELBV2Listener_.*_basic -timeout 360m 2024/11/06 16:43:21 Initializing Terraform AWS Provider... --- PASS: TestAccELBV2Listener_Gateway_basic (191.64s) --- PASS: TestAccELBV2Listener_Network_basic (214.10s) --- PASS: TestAccELBV2Listener_Application_basic (229.81s) PASS ok github.com/hashicorp/terraform-provider-aws/internal/service/elbv2 236.141s ``` --- internal/service/elbv2/listener.go | 19 +++++++++---------- internal/service/elbv2/listener_test.go | 3 +++ 2 files changed, 12 insertions(+), 10 deletions(-) diff --git a/internal/service/elbv2/listener.go b/internal/service/elbv2/listener.go index 3f42df19768..89da733492b 100644 --- a/internal/service/elbv2/listener.go +++ b/internal/service/elbv2/listener.go @@ -393,7 +393,7 @@ func resourceListener() *schema.Resource { "tcp_idle_timeout_seconds": { Type: schema.TypeInt, Optional: true, - Default: 350, + Computed: true, ValidateFunc: validation.IntBetween(60, 6000), // Attribute only valid for TCP (NLB) and GENEVE (GWLB) listeners DiffSuppressFunc: suppressIfListenerProtocolNot(awstypes.ProtocolEnumGeneve, awstypes.ProtocolEnumTcp), @@ -536,9 +536,7 @@ func resourceListenerCreate(ctx context.Context, d *schema.ResourceData, meta in } // Listener attributes like TCP idle timeout are not supported on create - var attributes []awstypes.ListenerAttribute - - attributes = append(attributes, listenerAttributes.expand(d, listenerProtocolType, false)...) + attributes := listenerAttributes.expand(d, listenerProtocolType, false) if len(attributes) > 0 { if err := modifyListenerAttributes(ctx, conn, d.Id(), attributes); err != nil { @@ -750,12 +748,13 @@ func (m listenerAttributeMap) expand(d *schema.ResourceData, listenerType awstyp continue } - if attributeInfo.tfType == schema.TypeInt { - v := (d.Get(tfAttributeName)).(int) - attributes = append(attributes, awstypes.ListenerAttribute{ - Key: aws.String(attributeInfo.apiAttributeKey), - Value: flex.IntValueToString(v), - }) + if v, ok := d.GetOk(tfAttributeName); ok { + if attributeInfo.tfType == schema.TypeInt { + attributes = append(attributes, awstypes.ListenerAttribute{ + Key: aws.String(attributeInfo.apiAttributeKey), + Value: flex.IntValueToString(v.(int)), + }) + } } } diff --git a/internal/service/elbv2/listener_test.go b/internal/service/elbv2/listener_test.go index 9265819937c..341ccca3cd9 100644 --- a/internal/service/elbv2/listener_test.go +++ b/internal/service/elbv2/listener_test.go @@ -62,6 +62,7 @@ func TestAccELBV2Listener_Application_basic(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "ssl_policy", ""), resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, "0"), resource.TestCheckResourceAttr(resourceName, acctest.CtTagsAllPercent, "0"), + resource.TestCheckNoResourceAttr(resourceName, "tcp_idle_timeout_seconds"), ), }, { @@ -114,6 +115,7 @@ func TestAccELBV2Listener_Network_basic(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "ssl_policy", ""), resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, "0"), resource.TestCheckResourceAttr(resourceName, acctest.CtTagsAllPercent, "0"), + resource.TestCheckResourceAttr(resourceName, "tcp_idle_timeout_seconds", "350"), ), }, { @@ -165,6 +167,7 @@ func TestAccELBV2Listener_Gateway_basic(t *testing.T) { resource.TestCheckResourceAttr(resourceName, "ssl_policy", ""), resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, "0"), resource.TestCheckResourceAttr(resourceName, acctest.CtTagsAllPercent, "0"), + resource.TestCheckResourceAttr(resourceName, "tcp_idle_timeout_seconds", "350"), ), }, { From a16df31ff4d10256949509efe0617b6ea3762d43 Mon Sep 17 00:00:00 2001 From: Jared Baker Date: Wed, 6 Nov 2024 16:34:14 -0500 Subject: [PATCH 2/3] r/aws_lb_listener: isolate `tcp_idle_timeout_seconds` update flow Updates to the `tcp_idle_timeout_seconds` argument are now handled distinctly from changes to other non-tag related arguments. Additionally, the logic applied to appropriately infer the protocol for gateway load balancers during the create operation has been replicated during update. Together these changes prevent failures when updating the `tcp_idle_timeout_seconds` argument for gateway load balancers like the following: ``` Error: modifying ELBv2 Listener (arn:aws:elasticloadbalancing:us-west-2::listener/gwy/tf-acc-test-5332588832756138226/541226271e9ad4ec/2bbf23d4354df440): operation error Elastic Load Balancing v2: ModifyListener, https response error StatusCode: 400, RequestID: c6f51e60-0511-443d-a98e-6f51b48fce11, api error ValidationError: An action must be specified ``` Also updates the `_attributes` acceptances tests to fully exercise the update workflow: ```console % make testacc PKG=elbv2 TESTS=TestAccELBV2Listener_attributes make: Verifying source code with gofmt... ==> Checking that code complies with gofmt requirements... TF_ACC=1 go1.23.2 test ./internal/service/elbv2/... -v -count 1 -parallel 20 -run='TestAccELBV2Listener_attributes' -timeout 360m 2024/11/06 16:12:02 Initializing Terraform AWS Provider... --- PASS: TestAccELBV2Listener_attributes_gwlb_TCPIdleTimeoutSeconds (210.83s) --- PASS: TestAccELBV2Listener_attributes_nlb_TCPIdleTimeoutSeconds (238.02s) PASS ok github.com/hashicorp/terraform-provider-aws/internal/service/elbv2 244.440s ``` --- internal/service/elbv2/listener.go | 29 ++++++++----- internal/service/elbv2/listener_test.go | 57 ++++++++++++++----------- 2 files changed, 49 insertions(+), 37 deletions(-) diff --git a/internal/service/elbv2/listener.go b/internal/service/elbv2/listener.go index 89da733492b..3bbf5f434dd 100644 --- a/internal/service/elbv2/listener.go +++ b/internal/service/elbv2/listener.go @@ -602,9 +602,7 @@ func resourceListenerUpdate(ctx context.Context, d *schema.ResourceData, meta in var diags diag.Diagnostics conn := meta.(*conns.AWSClient).ELBV2Client(ctx) - var attributes []awstypes.ListenerAttribute - - if d.HasChangesExcept(names.AttrTags, names.AttrTagsAll) { + if d.HasChangesExcept(names.AttrTags, names.AttrTagsAll, "tcp_idle_timeout_seconds") { input := &elasticloadbalancingv2.ModifyListenerInput{ ListenerArn: aws.String(d.Id()), } @@ -642,14 +640,6 @@ func resourceListenerUpdate(ctx context.Context, d *schema.ResourceData, meta in input.SslPolicy = aws.String(v.(string)) } - attributes = append(attributes, listenerAttributes.expand(d, awstypes.ProtocolEnum(d.Get(names.AttrProtocol).(string)), true)...) - - if len(attributes) > 0 { - if err := modifyListenerAttributes(ctx, conn, d.Id(), attributes); err != nil { - return sdkdiag.AppendFromErr(diags, err) - } - } - _, err := tfresource.RetryWhenIsA[*awstypes.CertificateNotFoundException](ctx, d.Timeout(schema.TimeoutUpdate), func() (interface{}, error) { return conn.ModifyListener(ctx, input) }) @@ -659,6 +649,23 @@ func resourceListenerUpdate(ctx context.Context, d *schema.ResourceData, meta in } } + if d.HasChanges("tcp_idle_timeout_seconds") { + lbARN := d.Get("load_balancer_arn").(string) + listenerProtocolType := awstypes.ProtocolEnum(d.Get(names.AttrProtocol).(string)) + // Protocol does not need to be explicitly set with GWLB listeners, nor is it returned by the API + // If protocol is not set, use the load balancer ARN to determine if listener is gateway type and set protocol appropriately + if listenerProtocolType == awstypes.ProtocolEnum("") && strings.Contains(lbARN, "loadbalancer/gwy/") { + listenerProtocolType = awstypes.ProtocolEnumGeneve + } + + attributes := listenerAttributes.expand(d, listenerProtocolType, true) + if len(attributes) > 0 { + if err := modifyListenerAttributes(ctx, conn, d.Id(), attributes); err != nil { + return sdkdiag.AppendFromErr(diags, err) + } + } + } + return append(diags, resourceListenerRead(ctx, d, meta)...) } diff --git a/internal/service/elbv2/listener_test.go b/internal/service/elbv2/listener_test.go index 341ccca3cd9..cd55a5274dc 100644 --- a/internal/service/elbv2/listener_test.go +++ b/internal/service/elbv2/listener_test.go @@ -8,6 +8,7 @@ import ( "fmt" "regexp" "slices" + "strconv" "testing" "github.com/YakDriver/regexache" @@ -1016,7 +1017,7 @@ func TestAccELBV2Listener_attributes_gwlb_TCPIdleTimeoutSeconds(t *testing.T) { lbResourceName := "aws_lb.test" resourceName := "aws_lb_listener.test" tcpTimeout1 := 60 - // tcpTimeout2 := 6000 + tcpTimeout2 := 6000 if testing.Short() { t.Skip("skipping long-running test in short mode") @@ -1035,8 +1036,7 @@ func TestAccELBV2Listener_attributes_gwlb_TCPIdleTimeoutSeconds(t *testing.T) { resource.TestCheckResourceAttrPair(resourceName, "load_balancer_arn", lbResourceName, names.AttrARN), resource.TestCheckResourceAttr(resourceName, names.AttrProtocol, ""), resource.TestCheckResourceAttr(resourceName, names.AttrPort, "0"), - resource.TestCheckResourceAttr(resourceName, "tcp_idle_timeout_seconds", "60"), - resource.TestCheckResourceAttr(resourceName, "tags.Name", rName), + resource.TestCheckResourceAttr(resourceName, "tcp_idle_timeout_seconds", strconv.Itoa(tcpTimeout1)), ), }, { @@ -1047,6 +1047,16 @@ func TestAccELBV2Listener_attributes_gwlb_TCPIdleTimeoutSeconds(t *testing.T) { "default_action.0.forward", }, }, + { + Config: testAccListenerConfig_attributes_gwlbTCPIdleTimeoutSeconds(rName, tcpTimeout2), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckListenerExists(ctx, resourceName, &conf), + resource.TestCheckResourceAttrPair(resourceName, "load_balancer_arn", lbResourceName, names.AttrARN), + resource.TestCheckResourceAttr(resourceName, names.AttrProtocol, ""), + resource.TestCheckResourceAttr(resourceName, names.AttrPort, "0"), + resource.TestCheckResourceAttr(resourceName, "tcp_idle_timeout_seconds", strconv.Itoa(tcpTimeout2)), + ), + }, }, }) } @@ -1056,6 +1066,8 @@ func TestAccELBV2Listener_attributes_nlb_TCPIdleTimeoutSeconds(t *testing.T) { var conf awstypes.Listener resourceName := "aws_lb_listener.test" rName := sdkacctest.RandomWithPrefix(acctest.ResourcePrefix) + tcpTimeout1 := 60 + tcpTimeout2 := 6000 resource.ParallelTest(t, resource.TestCase{ PreCheck: func() { acctest.PreCheck(ctx, t) }, @@ -1064,31 +1076,13 @@ func TestAccELBV2Listener_attributes_nlb_TCPIdleTimeoutSeconds(t *testing.T) { CheckDestroy: testAccCheckListenerDestroy(ctx), Steps: []resource.TestStep{ { - Config: testAccListenerConfig_attributes_nlbTCPIdleTimeoutSeconds(rName), + Config: testAccListenerConfig_attributes_nlbTCPIdleTimeoutSeconds(rName, tcpTimeout1), Check: resource.ComposeAggregateTestCheckFunc( testAccCheckListenerExists(ctx, resourceName, &conf), - resource.TestCheckResourceAttr("aws_lb.test", "load_balancer_type", "network"), resource.TestCheckResourceAttrPair(resourceName, "load_balancer_arn", "aws_lb.test", names.AttrARN), - acctest.MatchResourceAttrRegionalARN(resourceName, names.AttrARN, "elasticloadbalancing", regexache.MustCompile("listener/.+$")), - resource.TestCheckNoResourceAttr(resourceName, "alpn_policy"), - resource.TestCheckNoResourceAttr(resourceName, names.AttrCertificateARN), - resource.TestCheckResourceAttr(resourceName, "default_action.#", "1"), - resource.TestCheckResourceAttr(resourceName, "default_action.0.order", "1"), - resource.TestCheckResourceAttr(resourceName, "default_action.0.type", "forward"), - resource.TestCheckResourceAttrPair(resourceName, "default_action.0.target_group_arn", "aws_lb_target_group.test", names.AttrARN), - resource.TestCheckResourceAttr(resourceName, "default_action.0.authenticate_cognito.#", "0"), - resource.TestCheckResourceAttr(resourceName, "default_action.0.authenticate_oidc.#", "0"), - resource.TestCheckResourceAttr(resourceName, "default_action.0.fixed_response.#", "0"), - resource.TestCheckResourceAttr(resourceName, "default_action.0.forward.#", "0"), - resource.TestCheckResourceAttr(resourceName, "default_action.0.redirect.#", "0"), - resource.TestCheckResourceAttrPair(resourceName, names.AttrID, resourceName, names.AttrARN), - resource.TestCheckResourceAttr(resourceName, "mutual_authentication.#", "0"), resource.TestCheckResourceAttr(resourceName, names.AttrProtocol, "TCP"), resource.TestCheckResourceAttr(resourceName, names.AttrPort, "80"), - resource.TestCheckResourceAttr(resourceName, "ssl_policy", ""), - resource.TestCheckResourceAttr(resourceName, "tcp_idle_timeout_seconds", "60"), - resource.TestCheckResourceAttr(resourceName, acctest.CtTagsPercent, "0"), - resource.TestCheckResourceAttr(resourceName, acctest.CtTagsAllPercent, "0"), + resource.TestCheckResourceAttr(resourceName, "tcp_idle_timeout_seconds", strconv.Itoa(tcpTimeout1)), ), }, { @@ -1099,6 +1093,16 @@ func TestAccELBV2Listener_attributes_nlb_TCPIdleTimeoutSeconds(t *testing.T) { "default_action.0.forward", }, }, + { + Config: testAccListenerConfig_attributes_nlbTCPIdleTimeoutSeconds(rName, tcpTimeout2), + Check: resource.ComposeAggregateTestCheckFunc( + testAccCheckListenerExists(ctx, resourceName, &conf), + resource.TestCheckResourceAttrPair(resourceName, "load_balancer_arn", "aws_lb.test", names.AttrARN), + resource.TestCheckResourceAttr(resourceName, names.AttrProtocol, "TCP"), + resource.TestCheckResourceAttr(resourceName, names.AttrPort, "80"), + resource.TestCheckResourceAttr(resourceName, "tcp_idle_timeout_seconds", strconv.Itoa(tcpTimeout2)), + ), + }, }, }) } @@ -2967,7 +2971,7 @@ resource "aws_lb_listener" "test" { `, rName, seconds)) } -func testAccListenerConfig_attributes_nlbTCPIdleTimeoutSeconds(rName string) string { +func testAccListenerConfig_attributes_nlbTCPIdleTimeoutSeconds(rName string, seconds int) string { return acctest.ConfigCompose( testAccListenerConfig_base(rName), fmt.Sprintf(` resource "aws_lb_listener" "test" { @@ -2979,7 +2983,8 @@ resource "aws_lb_listener" "test" { target_group_arn = aws_lb_target_group.test.id type = "forward" } - tcp_idle_timeout_seconds = 60 + + tcp_idle_timeout_seconds = %[2]d } resource "aws_lb" "test" { @@ -3015,7 +3020,7 @@ resource "aws_lb_target_group" "test" { Name = %[1]q } } -`, rName)) +`, rName, seconds)) } func testAccListenerConfig_Forward_changeWeightedToBasic(rName, rName2 string) string { From 5aa38a323fd38c410f60783beec37a408ed95d62 Mon Sep 17 00:00:00 2001 From: Jared Baker Date: Thu, 7 Nov 2024 11:22:36 -0500 Subject: [PATCH 3/3] chore: changelog --- .changelog/40039.txt | 6 ++++++ 1 file changed, 6 insertions(+) create mode 100644 .changelog/40039.txt diff --git a/.changelog/40039.txt b/.changelog/40039.txt new file mode 100644 index 00000000000..abed6875eba --- /dev/null +++ b/.changelog/40039.txt @@ -0,0 +1,6 @@ +```release-note:bug +resource/aws_lb_listener: Remove the default `tcp_idle_timeout_seconds` value, preventing `ModifyListenerAttributes` API calls when a value is not explicitly configured +``` +```release-note:bug +resource/aws_lb_listener: Fix errors when updating the `tcp_idle_timeout_seconds` argument for gateway load balancers +```