Skip to content
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.

Sign up for GitHub

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: Add Cross Zone Load Balancing support for NLBs #3537

Merged
merged 7 commits into from
Mar 2, 2018
Merged

resource/aws_lb: Add Cross Zone Load Balancing support for NLBs #3537

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9b8a2e6
resource/aws_lb: Add Cross Zone Load Balancing support
Feb 26, 2018
e95bda4
Add doc for cross-zone load balancing
Feb 26, 2018
4275243
Add a positive test
Mar 1, 2018
efe96e4
Fix dumb dumb errors
Mar 1, 2018
7d0b535
Fix typo
Mar 1, 2018
75b269e
Add test with progressive update of NLB
Mar 2, 2018
7c89b7c
More consistent use of LB vs ALB.
Mar 2, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 33 additions & 15 deletions aws/resource_aws_lb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import (
func resourceAwsLb() *schema.Resource {
return &schema.Resource{
Create: resourceAwsLbCreate,
Read: resoureAwsLbRead,
Read: resourceAwsLbRead,
Update: resourceAwsLbUpdate,
Delete: resourceAwsLbDelete,
// Subnets are ForceNew for Network Load Balancers
Expand Down Expand Up @@ -157,6 +157,12 @@ func resourceAwsLb() *schema.Resource {
Default: 60,
},

"enable_cross_zone_load_balancing": {
Type: schema.TypeBool,
Optional: true,
Default: false,
},

"ip_address_type": {
Type: schema.TypeString,
Computed: true,
Expand Down Expand Up @@ -247,7 +253,7 @@ func resourceAwsLbCreate(d *schema.ResourceData, meta interface{}) error {

lb := resp.LoadBalancers[0]
d.SetId(*lb.LoadBalancerArn)
log.Printf("[INFO] ALB ID: %s", d.Id())
log.Printf("[INFO] LB ID: %s", d.Id())

stateConf := &resource.StateChangeConf{
Pending: []string{"provisioning", "failed"},
Expand All @@ -265,7 +271,7 @@ func resourceAwsLbCreate(d *schema.ResourceData, meta interface{}) error {
}
dLb := describeResp.LoadBalancers[0]

log.Printf("[INFO] ALB state: %s", *dLb.State.Code)
log.Printf("[INFO] LB state: %s", *dLb.State.Code)

return describeResp, *dLb.State.Code, nil
},
Expand All @@ -281,7 +287,7 @@ func resourceAwsLbCreate(d *schema.ResourceData, meta interface{}) error {
return resourceAwsLbUpdate(d, meta)
}

func resoureAwsLbRead(d *schema.ResourceData, meta interface{}) error {
func resourceAwsLbRead(d *schema.ResourceData, meta interface{}) error {
elbconn := meta.(*AWSClient).elbv2conn
lbArn := d.Id()

Expand Down Expand Up @@ -362,6 +368,14 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {
})
}

// It's important to know that Idle timeout is only supported for Network Loadbalancers
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment references Idle timeout (probably meant to be cross zone flag)

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 {
input := &elbv2.ModifyLoadBalancerAttributesInput{
LoadBalancerArn: aws.String(d.Id()),
Expand All @@ -371,7 +385,7 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {
log.Printf("[DEBUG] ALB Modify Load Balancer Attributes Request: %#v", input)
_, err := elbconn.ModifyLoadBalancerAttributes(input)
if err != nil {
return fmt.Errorf("Failure configuring ALB attributes: %s", err)
return fmt.Errorf("Failure configuring LB attributes: %s", err)
}
}

Expand All @@ -384,7 +398,7 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {
}
_, err := elbconn.SetSecurityGroups(params)
if err != nil {
return fmt.Errorf("Failure Setting ALB Security Groups: %s", err)
return fmt.Errorf("Failure Setting LB Security Groups: %s", err)
}

}
Expand All @@ -403,7 +417,7 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {

_, err := elbconn.SetSubnets(params)
if err != nil {
return fmt.Errorf("Failure Setting ALB Subnets: %s", err)
return fmt.Errorf("Failure Setting LB Subnets: %s", err)
}
}

Expand All @@ -416,7 +430,7 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {

_, err := elbconn.SetIpAddressType(params)
if err != nil {
return fmt.Errorf("Failure Setting ALB IP Address Type: %s", err)
return fmt.Errorf("Failure Setting LB IP Address Type: %s", err)
}

}
Expand All @@ -437,7 +451,7 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {
}
dLb := describeResp.LoadBalancers[0]

log.Printf("[INFO] ALB state: %s", *dLb.State.Code)
log.Printf("[INFO] LB state: %s", *dLb.State.Code)

return describeResp, *dLb.State.Code, nil
},
Expand All @@ -450,20 +464,20 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {
return err
}

return resoureAwsLbRead(d, meta)
return resourceAwsLbRead(d, meta)
}

func resourceAwsLbDelete(d *schema.ResourceData, meta interface{}) error {
lbconn := meta.(*AWSClient).elbv2conn

log.Printf("[INFO] Deleting ALB: %s", d.Id())
log.Printf("[INFO] Deleting LB: %s", d.Id())

// Destroy the load balancer
deleteElbOpts := elbv2.DeleteLoadBalancerInput{
LoadBalancerArn: aws.String(d.Id()),
}
if _, err := lbconn.DeleteLoadBalancer(&deleteElbOpts); err != nil {
return fmt.Errorf("Error deleting ALB: %s", err)
return fmt.Errorf("Error deleting LB: %s", err)
}

conn := meta.(*AWSClient).ec2conn
Expand Down Expand Up @@ -642,7 +656,7 @@ func flattenAwsLbResource(d *schema.ResourceData, meta interface{}, lb *elbv2.Lo
ResourceArns: []*string{lb.LoadBalancerArn},
})
if err != nil {
return errwrap.Wrapf("Error retrieving ALB Tags: {{err}}", err)
return errwrap.Wrapf("Error retrieving LB Tags: {{err}}", err)
}

var et []*elbv2.Tag
Expand All @@ -658,7 +672,7 @@ func flattenAwsLbResource(d *schema.ResourceData, meta interface{}, lb *elbv2.Lo
LoadBalancerArn: aws.String(d.Id()),
})
if err != nil {
return errwrap.Wrapf("Error retrieving ALB Attributes: {{err}}", err)
return errwrap.Wrapf("Error retrieving LB Attributes: {{err}}", err)
}

accessLogMap := map[string]interface{}{}
Expand All @@ -679,8 +693,12 @@ func flattenAwsLbResource(d *schema.ResourceData, meta interface{}, lb *elbv2.Lo
d.Set("idle_timeout", timeout)
case "deletion_protection.enabled":
protectionEnabled := (*attr.Value) == "true"
log.Printf("[DEBUG] Setting ALB Deletion Protection Enabled: %t", protectionEnabled)
log.Printf("[DEBUG] Setting LB Deletion Protection Enabled: %t", protectionEnabled)
d.Set("enable_deletion_protection", protectionEnabled)
case "load_balancing.cross_zone.enabled":
crossZoneLbEnabled := (*attr.Value) == "true"
log.Printf("[DEBUG] Setting NLB Cross Zone Load Balancing Enabled: %t", crossZoneLbEnabled)
d.Set("enable_cross_zone_load_balancing", crossZoneLbEnabled)
}
}

Expand Down
53 changes: 44 additions & 9 deletions aws/resource_aws_lb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func TestAccAWSLB_basic(t *testing.T) {
})
}

func TestAccAWSLB_networkLoadbalancer(t *testing.T) {
func TestAccAWSLB_networkLoadbalancerBasic(t *testing.T) {
var conf elbv2.LoadBalancer
lbName := fmt.Sprintf("testaccawslb-basic-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))

Expand All @@ -90,7 +90,7 @@ func TestAccAWSLB_networkLoadbalancer(t *testing.T) {
CheckDestroy: testAccCheckAWSLBDestroy,
Steps: []resource.TestStep{
{
Config: testAccAWSLBConfig_networkLoadbalancer(lbName),
Config: testAccAWSLBConfig_networkLoadbalancer(lbName, false),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAWSLBExists("aws_lb.lb_test", &conf),
resource.TestCheckResourceAttr("aws_lb.lb_test", "name", lbName),
Expand Down Expand Up @@ -128,6 +128,7 @@ func TestAccAWSLB_networkLoadbalancerEIP(t *testing.T) {
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"),
),
Expand Down Expand Up @@ -157,6 +158,7 @@ func TestAccAWSLBBackwardsCompatibility(t *testing.T) {
resource.TestCheckResourceAttr("aws_alb.lb_test", "tags.%", "1"),
resource.TestCheckResourceAttr("aws_alb.lb_test", "tags.Name", "TestAccAWSALB_basic"),
resource.TestCheckResourceAttr("aws_alb.lb_test", "enable_deletion_protection", "false"),
resource.TestCheckResourceAttr("aws_alb.lb_test", "enable_cross_zone_load_balancing", "false"),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should create a test that ensures enabling/disabling works as expected 😄

Copy link
Author

@mpilar mpilar Mar 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any hints as to where I could look for an example? It's my first time digging into a largeish golang codebase.

Is it sufficient to add the directive to testAccAWSLBConfig_networkLoadBalancerEIP as true and assert on that test?

(See added commit)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was unable to run the acceptance tests after the commit. As soon as I have a break I will.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My general workflow does usually involve adding a directive/parameter to the test configuration function for the new attribute. Generally speaking though, its usually easiest to just create a whole new acceptance test and configuration function so you don't wind up with needing to pass in way too many directives to a function. Copy paste of an existing is okay and generally encouraged for this situation as the testing is more important than any (potential) later refactoring. 👍

resource.TestCheckResourceAttr("aws_alb.lb_test", "idle_timeout", "30"),
resource.TestCheckResourceAttr("aws_alb.lb_test", "ip_address_type", "ipv4"),
resource.TestCheckResourceAttr("aws_alb.lb_test", "load_balancer_type", "application"),
Expand Down Expand Up @@ -263,6 +265,35 @@ func TestAccAWSLB_tags(t *testing.T) {
})
}

func TestAccAWSLB_networkLoadbalancer_updateCrossZone(t *testing.T) {
var pre, post elbv2.LoadBalancer
lbName := fmt.Sprintf("testaccawslb-nlbcz-%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_networkLoadbalancer(lbName, true),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAWSLBExists("aws_lb.lb_test", &pre),
resource.TestCheckResourceAttr("aws_lb.lb_test", "enable_cross_zone_load_balancing", "true"),
),
},
{
Config: testAccAWSLBConfig_networkLoadbalancer(lbName, false),
Check: resource.ComposeAggregateTestCheckFunc(
testAccCheckAWSLBExists("aws_lb.lb_test", &post),
resource.TestCheckResourceAttr("aws_lb.lb_test", "enable_cross_zone_load_balancing", "false"),
testAccCheckAWSlbARNs(&pre, &post),
),
},
},
})
}

func TestAccAWSLB_updatedSecurityGroups(t *testing.T) {
var pre, post elbv2.LoadBalancer
lbName := fmt.Sprintf("testaccawslb-basic-%s", acctest.RandStringFromCharSet(10, acctest.CharSetAlphaNum))
Expand Down Expand Up @@ -861,10 +892,11 @@ resource "aws_lb" "lb_test" {
"${aws_subnet.alb_test_3.id}",
]

load_balancer_type = "network"
internal = true
idle_timeout = 60
enable_deletion_protection = false
load_balancer_type = "network"
internal = true
idle_timeout = 60
enable_deletion_protection = false
enable_cross_zone_load_balancing = false

tags {
Name = "testAccAWSLBConfig_networkLoadbalancer_subnets"
Expand Down Expand Up @@ -902,13 +934,15 @@ resource "aws_subnet" "alb_test_3" {
}
`, lbName)
}
func testAccAWSLBConfig_networkLoadbalancer(lbName string) string {

func testAccAWSLBConfig_networkLoadbalancer(lbName string, cz bool) string {
return fmt.Sprintf(`resource "aws_lb" "lb_test" {
name = "%s"
internal = true
load_balancer_type = "network"

enable_deletion_protection = false
enable_deletion_protection = false
enable_cross_zone_load_balancing = %t

subnet_mapping {
subnet_id = "${aws_subnet.alb_test.id}"
Expand Down Expand Up @@ -938,7 +972,7 @@ resource "aws_subnet" "alb_test" {
}
}

`, lbName)
`, lbName, cz)
}

func testAccAWSLBConfig_networkLoadBalancerEIP(lbName string) string {
Expand Down Expand Up @@ -980,6 +1014,7 @@ resource "aws_route_table_association" "a" {
resource "aws_lb" "test" {
name = "%s"
load_balancer_type = "network"
enable_cross_zone_load_balancing = true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my note above -- we'll probably want to just copy paste the handling of the new attribute to a new test and config or at least pass the true/false value for this attributes is a directive into the above config function and use enable_cross_zone_load_balancing = %v 😄

subnet_mapping {
subnet_id = "${aws_subnet.public.0.id}"
allocation_id = "${aws_eip.lb.0.id}"
Expand Down
2 changes: 2 additions & 0 deletions website/docs/r/lb.html.markdown
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ will for load balancers of type `network` will force a recreation of the resourc
* `idle_timeout` - (Optional) The time in seconds that the connection is allowed to be idle. Default: 60.
* `enable_deletion_protection` - (Optional) If true, deletion of the load balancer will be disabled via
the AWS API. This will prevent Terraform from deleting the load balancer. Defaults to `false`.
* `enable_cross_zone_load_balancing` - (Optional) If true, cross-zone load balancing of the load balancer will be enabled.
This is a `network` load balancer feature. Defaults to `false`.
* `ip_address_type` - (Optional) The type of IP addresses used by the subnets for your load balancer. The possible values are `ipv4` and `dualstack`
* `tags` - (Optional) A mapping of tags to assign to the resource.

Expand Down