-
Notifications
You must be signed in to change notification settings - Fork 9k
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
resource/aws_lb: enable_http2 flag for LBs and small refactor #3609
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -163,6 +163,12 @@ func resourceAwsLb() *schema.Resource { | |
Default: false, | ||
}, | ||
|
||
"enable_http2": { | ||
Type: schema.TypeBool, | ||
Optional: true, | ||
Default: true, | ||
}, | ||
|
||
"ip_address_type": { | ||
Type: schema.TypeString, | ||
Computed: true, | ||
|
@@ -324,56 +330,62 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error { | |
|
||
attributes := make([]*elbv2.LoadBalancerAttribute, 0) | ||
|
||
if d.HasChange("access_logs") { | ||
logs := d.Get("access_logs").([]interface{}) | ||
if len(logs) == 1 { | ||
log := logs[0].(map[string]interface{}) | ||
|
||
attributes = append(attributes, | ||
&elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("access_logs.s3.enabled"), | ||
Value: aws.String(strconv.FormatBool(log["enabled"].(bool))), | ||
}, | ||
&elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("access_logs.s3.bucket"), | ||
Value: aws.String(log["bucket"].(string)), | ||
}) | ||
|
||
if prefix, ok := log["prefix"]; ok { | ||
switch d.Get("load_balancer_type").(string) { | ||
case "application": | ||
if d.HasChange("access_logs") { | ||
logs := d.Get("access_logs").([]interface{}) | ||
if len(logs) == 1 { | ||
log := logs[0].(map[string]interface{}) | ||
|
||
attributes = append(attributes, | ||
&elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("access_logs.s3.enabled"), | ||
Value: aws.String(strconv.FormatBool(log["enabled"].(bool))), | ||
}, | ||
&elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("access_logs.s3.bucket"), | ||
Value: aws.String(log["bucket"].(string)), | ||
}) | ||
|
||
if prefix, ok := log["prefix"]; ok { | ||
attributes = append(attributes, &elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("access_logs.s3.prefix"), | ||
Value: aws.String(prefix.(string)), | ||
}) | ||
} | ||
} else if len(logs) == 0 { | ||
attributes = append(attributes, &elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("access_logs.s3.prefix"), | ||
Value: aws.String(prefix.(string)), | ||
Key: aws.String("access_logs.s3.enabled"), | ||
Value: aws.String("false"), | ||
}) | ||
} | ||
} else if len(logs) == 0 { | ||
} | ||
if d.HasChange("idle_timeout") { | ||
attributes = append(attributes, &elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("access_logs.s3.enabled"), | ||
Value: aws.String("false"), | ||
Key: aws.String("idle_timeout.timeout_seconds"), | ||
Value: aws.String(fmt.Sprintf("%d", d.Get("idle_timeout").(int))), | ||
}) | ||
} | ||
if d.HasChange("enable_http2") { | ||
attributes = append(attributes, &elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("routing.http2.enabled"), | ||
Value: aws.String(strconv.FormatBool(d.Get("enable_http2").(bool))), | ||
}) | ||
} | ||
case "network": | ||
if d.HasChange("enable_cross_zone_load_balancing") { | ||
attributes = append(attributes, &elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("load_balancing.cross_zone.enabled"), | ||
Value: aws.String(fmt.Sprintf("%t", d.Get("enable_cross_zone_load_balancing").(bool))), | ||
}) | ||
} | ||
default: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When would we hit this scenario? This currently will never hit. Also, I'm not sure the benefit of postulating if/when there may be new load balancer types and having any sort of implicit support of them. We should force a provider update with acceptance testing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was a mistake in my PR (which was coded offline on plane), I forgot Go's switch doesn't fallthrough by default |
||
if d.HasChange("enable_deletion_protection") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Given the note above about There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will add an acceptance test. (and fix the switch statement) |
||
attributes = append(attributes, &elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("deletion_protection.enabled"), | ||
Value: aws.String(fmt.Sprintf("%t", d.Get("enable_deletion_protection").(bool))), | ||
}) | ||
} | ||
} | ||
|
||
if d.HasChange("enable_deletion_protection") { | ||
attributes = append(attributes, &elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("deletion_protection.enabled"), | ||
Value: aws.String(fmt.Sprintf("%t", d.Get("enable_deletion_protection").(bool))), | ||
}) | ||
} | ||
|
||
// It's important to know that Idle timeout is not supported for Network Loadbalancers | ||
if d.Get("load_balancer_type").(string) != "network" && d.HasChange("idle_timeout") { | ||
attributes = append(attributes, &elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("idle_timeout.timeout_seconds"), | ||
Value: aws.String(fmt.Sprintf("%d", d.Get("idle_timeout").(int))), | ||
}) | ||
} | ||
|
||
// It's important to know that Idle timeout is only supported for Network Loadbalancers | ||
if d.Get("load_balancer_type").(string) == "network" && d.HasChange("enable_cross_zone_load_balancing") { | ||
attributes = append(attributes, &elbv2.LoadBalancerAttribute{ | ||
Key: aws.String("load_balancing.cross_zone.enabled"), | ||
Value: aws.String(fmt.Sprintf("%t", d.Get("enable_cross_zone_load_balancing").(bool))), | ||
}) | ||
} | ||
|
||
if len(attributes) != 0 { | ||
|
@@ -552,7 +564,7 @@ func waitForNLBNetworkInterfacesToDetach(conn *ec2.EC2, lbArn string) error { | |
// OperationNotPermitted: You are not allowed to manage 'ela-attach' attachments. | ||
// yet presence of these ENIs may prevent us from deleting EIPs associated w/ the NLB | ||
|
||
return resource.Retry(1*time.Minute, func() *resource.RetryError { | ||
return resource.Retry(5*time.Minute, func() *resource.RetryError { | ||
out, err := conn.DescribeNetworkInterfaces(&ec2.DescribeNetworkInterfacesInput{ | ||
Filters: []*ec2.Filter{ | ||
{ | ||
|
@@ -695,6 +707,10 @@ func flattenAwsLbResource(d *schema.ResourceData, meta interface{}, lb *elbv2.Lo | |
protectionEnabled := (*attr.Value) == "true" | ||
log.Printf("[DEBUG] Setting LB Deletion Protection Enabled: %t", protectionEnabled) | ||
d.Set("enable_deletion_protection", protectionEnabled) | ||
case "enable_http2": | ||
http2Enabled := (*attr.Value) == "true" | ||
log.Printf("[DEBUG] Setting ALB HTTP/2 Enabled: %t", http2Enabled) | ||
d.Set("enable_http2", http2Enabled) | ||
case "load_balancing.cross_zone.enabled": | ||
crossZoneLbEnabled := (*attr.Value) == "true" | ||
log.Printf("[DEBUG] Setting NLB Cross Zone Load Balancing Enabled: %t", crossZoneLbEnabled) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -121,16 +121,16 @@ func TestAccAWSLB_networkLoadbalancerEIP(t *testing.T) { | |
{ | ||
Config: testAccAWSLBConfig_networkLoadBalancerEIP(lbName), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
testAccCheckAWSLBExists("aws_lb.test", &conf), | ||
resource.TestCheckResourceAttr("aws_lb.test", "name", lbName), | ||
resource.TestCheckResourceAttr("aws_lb.test", "internal", "false"), | ||
resource.TestCheckResourceAttr("aws_lb.test", "ip_address_type", "ipv4"), | ||
resource.TestCheckResourceAttrSet("aws_lb.test", "zone_id"), | ||
resource.TestCheckResourceAttrSet("aws_lb.test", "dns_name"), | ||
resource.TestCheckResourceAttrSet("aws_lb.test", "arn"), | ||
resource.TestCheckResourceAttr("aws_lb.test", "enable_cross_zone_load_balancing", "true"), | ||
resource.TestCheckResourceAttr("aws_lb.test", "load_balancer_type", "network"), | ||
resource.TestCheckResourceAttr("aws_lb.test", "subnet_mapping.#", "2"), | ||
testAccCheckAWSLBExists("aws_lb.lb_test", &conf), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "name", lbName), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "internal", "false"), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "ip_address_type", "ipv4"), | ||
resource.TestCheckResourceAttrSet("aws_lb.lb_test", "zone_id"), | ||
resource.TestCheckResourceAttrSet("aws_lb.lb_test", "dns_name"), | ||
resource.TestCheckResourceAttrSet("aws_lb.lb_test", "arn"), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "load_balancer_type", "network"), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "enable_deletion_protection", "false"), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "subnet_mapping.#", "2"), | ||
), | ||
}, | ||
}, | ||
|
@@ -266,7 +266,7 @@ func TestAccAWSLB_tags(t *testing.T) { | |
} | ||
|
||
func TestAccAWSLB_networkLoadbalancer_updateCrossZone(t *testing.T) { | ||
var pre, post elbv2.LoadBalancer | ||
var pre, mid, post elbv2.LoadBalancer | ||
lbName := fmt.Sprintf("testaccawslb-nlbcz-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)) | ||
|
||
resource.Test(t, resource.TestCase{ | ||
|
@@ -285,9 +285,54 @@ func TestAccAWSLB_networkLoadbalancer_updateCrossZone(t *testing.T) { | |
{ | ||
Config: testAccAWSLBConfig_networkLoadbalancer(lbName, false), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
testAccCheckAWSLBExists("aws_lb.lb_test", &post), | ||
testAccCheckAWSLBExists("aws_lb.lb_test", &mid), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "enable_cross_zone_load_balancing", "false"), | ||
testAccCheckAWSlbARNs(&pre, &post), | ||
testAccCheckAWSlbARNs(&pre, &mid), | ||
), | ||
}, | ||
{ | ||
Config: testAccAWSLBConfig_networkLoadbalancer(lbName, true), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
testAccCheckAWSLBExists("aws_lb.lb_test", &post), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "enable_cross_zone_load_balancing", "true"), | ||
testAccCheckAWSlbARNs(&mid, &post), | ||
), | ||
}, | ||
}, | ||
}) | ||
} | ||
|
||
func TestAccAWSLB_applicationLoadBalancer_updateHttp2(t *testing.T) { | ||
var pre, mid, post elbv2.LoadBalancer | ||
lbName := fmt.Sprintf("testaccawsalb-http2-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum)) | ||
|
||
resource.Test(t, resource.TestCase{ | ||
PreCheck: func() { testAccPreCheck(t) }, | ||
IDRefreshName: "aws_lb.lb_test", | ||
Providers: testAccProviders, | ||
CheckDestroy: testAccCheckAWSLBDestroy, | ||
Steps: []resource.TestStep{ | ||
{ | ||
Config: testAccAWSLBConfig_basicWithHttp2Toggle(lbName, true), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
testAccCheckAWSLBExists("aws_lb.lb_test", &pre), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "enable_http2", "true"), | ||
), | ||
}, | ||
{ | ||
Config: testAccAWSLBConfig_basicWithHttp2Toggle(lbName, false), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
testAccCheckAWSLBExists("aws_lb.lb_test", &mid), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "enable_http2", "false"), | ||
testAccCheckAWSlbARNs(&pre, &mid), | ||
), | ||
}, | ||
{ | ||
Config: testAccAWSLBConfig_basicWithHttp2Toggle(lbName, true), | ||
Check: resource.ComposeAggregateTestCheckFunc( | ||
testAccCheckAWSLBExists("aws_lb.lb_test", &post), | ||
resource.TestCheckResourceAttr("aws_lb.lb_test", "enable_http2", "true"), | ||
testAccCheckAWSlbARNs(&mid, &post), | ||
), | ||
}, | ||
}, | ||
|
@@ -874,6 +919,75 @@ resource "aws_security_group" "alb_test" { | |
}`, lbName) | ||
} | ||
|
||
func testAccAWSLBConfig_basicWithHttp2Toggle(lbName string, http2 bool) string { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Minor nitpick: generally we would just name this |
||
return fmt.Sprintf(`resource "aws_lb" "lb_test" { | ||
name = "%s" | ||
internal = true | ||
security_groups = ["${aws_security_group.alb_test.id}"] | ||
subnets = ["${aws_subnet.alb_test.*.id}"] | ||
|
||
idle_timeout = 30 | ||
enable_deletion_protection = false | ||
|
||
enable_http2 = %t | ||
|
||
tags { | ||
Name = "TestAccAWSALB_basic" | ||
} | ||
} | ||
|
||
variable "subnets" { | ||
default = ["10.0.1.0/24", "10.0.2.0/24"] | ||
type = "list" | ||
} | ||
|
||
data "aws_availability_zones" "available" {} | ||
|
||
resource "aws_vpc" "alb_test" { | ||
cidr_block = "10.0.0.0/16" | ||
|
||
tags { | ||
Name = "terraform-testacc-lb-basic" | ||
} | ||
} | ||
|
||
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 = "TestAccAWSALB_basic" | ||
} | ||
} | ||
|
||
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 = "TestAccAWSALB_basic" | ||
} | ||
}`, lbName, http2) | ||
} | ||
|
||
func testAccAWSLBConfig_networkLoadbalancer_subnets(lbName string) string { | ||
return fmt.Sprintf(`resource "aws_vpc" "alb_test" { | ||
cidr_block = "10.0.0.0/16" | ||
|
@@ -1011,10 +1125,9 @@ resource "aws_route_table_association" "a" { | |
route_table_id = "${aws_route_table.public.id}" | ||
} | ||
|
||
resource "aws_lb" "test" { | ||
resource "aws_lb" "lb_test" { | ||
name = "%s" | ||
load_balancer_type = "network" | ||
enable_cross_zone_load_balancing = true | ||
subnet_mapping { | ||
subnet_id = "${aws_subnet.public.0.id}" | ||
allocation_id = "${aws_eip.lb.0.id}" | ||
|
@@ -1026,7 +1139,7 @@ resource "aws_lb" "test" { | |
} | ||
|
||
resource "aws_eip" "lb" { | ||
count = "${length(data.aws_availability_zones.available.names)}" | ||
count = "${length(data.aws_availability_zones.available.names)>2?2:length(data.aws_availability_zones.available.names)}" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there any regions with only 1 AZ? Seems like this should just be |
||
} | ||
`, lbName) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably PR and acceptance test this separately 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove it for now.