r/aws_lb: mark subnets as ForceNew for network load balancers (#2310)

* Add test verifying broken subnet setting for aws_lb

- subnets can't be updated for lbs of type `network`
- the `UPDATE` method tries to update the `subnets`, which fails, even
on first apply

* fix ordering

* Mark Subnets as forcenew for network load balancers

aws/resource_aws_lb.go

@@ -24,6 +24,8 @@ func resourceAwsLb() *schema.Resource {
 		Read:   resoureAwsLbRead,
 		Update: resourceAwsLbUpdate,
 		Delete: resourceAwsLbDelete,
+		// Subnets are ForceNew for Network Load Balancers
+		CustomizeDiff: customizeDiffNLBSubnets,
 		Importer: &schema.ResourceImporter{
 			State: schema.ImportStatePassthrough,
 		},
@@ -387,7 +389,11 @@ func resourceAwsLbUpdate(d *schema.ResourceData, meta interface{}) error {
 
 	}
 
-	if d.HasChange("subnets") {
+	// subnets are assigned at Create; the 'change' here is an empty map for old
+	// and current subnets for new, so this change is redundant when the
+	// resource is just created, so we don't attempt if it is a newly created
+	// resource.
+	if d.HasChange("subnets") && !d.IsNewResource() {
 		subnets := expandStringList(d.Get("subnets").(*schema.Set).List())
 
 		params := &elbv2.SetSubnetsInput{
@@ -687,3 +693,50 @@ func flattenAwsLbResource(d *schema.ResourceData, meta interface{}, lb *elbv2.Lo
 
 	return nil
 }
+
+// Load balancers of type 'network' cannot have their subnets updated at
+// this time. If the type is 'network' and subnets have changed, mark the
+// diff as a ForceNew operation
+func customizeDiffNLBSubnets(diff *schema.ResourceDiff, v interface{}) error {
+	// The current criteria for determining if the operation should be ForceNew:
+	// - lb of type "network"
+	// - existing resource (id is not "")
+	// - there are actual changes to be made in the subnets
+	//
+	// Any other combination should be treated as normal. At this time, subnet
+	// handling is the only known difference between Network Load Balancers and
+	// Application Load Balancers, so the logic below is simple individual checks.
+	// If other differences arise we'll want to refactor to check other
+	// conditions in combinations, but for now all we handle is subnets
+	lbType := diff.Get("load_balancer_type").(string)
+	if "network" != lbType {
+		return nil
+	}
+
+	if "" == diff.Id() {
+		return nil
+	}
+
+	o, n := diff.GetChange("subnets")
+	if o == nil {
+		o = new(schema.Set)
+	}
+	if n == nil {
+		n = new(schema.Set)
+	}
+	os := o.(*schema.Set)
+	ns := n.(*schema.Set)
+	remove := os.Difference(ns).List()
+	add := ns.Difference(os).List()
+	delta := len(remove) > 0 || len(add) > 0
+	if delta {
+		if err := diff.SetNew("subnets", n); err != nil {
+			return err
+		}
+
+		if err := diff.ForceNew("subnets"); err != nil {
+			return err
+		}
+	}
+	return nil
+}

aws/resource_aws_lb_test.go

@@ -458,6 +458,31 @@ func TestAccAWSLB_accesslogs(t *testing.T) {
 	})
 }
 
+func TestAccAWSLB_networkLoadbalancer_subnet_change(t *testing.T) {
+	var conf elbv2.LoadBalancer
+	lbName := fmt.Sprintf("testaccawslb-basic-%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_subnets(lbName),
+				Check: resource.ComposeAggregateTestCheckFunc(
+					testAccCheckAWSLBExists("aws_lb.lb_test", &conf),
+					resource.TestCheckResourceAttr("aws_lb.lb_test", "name", lbName),
+					resource.TestCheckResourceAttr("aws_lb.lb_test", "internal", "true"),
+					resource.TestCheckResourceAttr("aws_lb.lb_test", "tags.%", "1"),
+					resource.TestCheckResourceAttr("aws_lb.lb_test", "tags.Name", "testAccAWSLBConfig_networkLoadbalancer_subnets"),
+					resource.TestCheckResourceAttr("aws_lb.lb_test", "load_balancer_type", "network"),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckAWSlbARNs(pre, post *elbv2.LoadBalancer) resource.TestCheckFunc {
 	return func(s *terraform.State) error {
 		if *pre.LoadBalancerArn != *post.LoadBalancerArn {
@@ -818,6 +843,65 @@ resource "aws_security_group" "alb_test" {
 }`, lbName)
 }
 
+func testAccAWSLBConfig_networkLoadbalancer_subnets(lbName string) string {
+	return fmt.Sprintf(`resource "aws_vpc" "alb_test" {
+  cidr_block = "10.0.0.0/16"
+
+  tags {
+    Name = "testAccAWSLBConfig_networkLoadbalancer_subnets"
+  }
+}
+
+resource "aws_lb" "lb_test" {
+  name = "%s"
+
+  subnets = [
+    "${aws_subnet.alb_test_1.id}",
+    "${aws_subnet.alb_test_2.id}",
+    "${aws_subnet.alb_test_3.id}",
+  ]
+
+  load_balancer_type         = "network"
+  internal                   = true
+  idle_timeout               = 60
+  enable_deletion_protection = false
+
+  tags {
+    Name = "testAccAWSLBConfig_networkLoadbalancer_subnets"
+  }
+}
+
+resource "aws_subnet" "alb_test_1" {
+  vpc_id            = "${aws_vpc.alb_test.id}"
+  cidr_block        = "10.0.1.0/24"
+  availability_zone = "us-west-2a"
+
+  tags {
+    Name = "testAccAWSLBConfig_networkLoadbalancer_subnets"
+  }
+}
+
+resource "aws_subnet" "alb_test_2" {
+  vpc_id            = "${aws_vpc.alb_test.id}"
+  cidr_block        = "10.0.2.0/24"
+  availability_zone = "us-west-2b"
+
+  tags {
+    Name = "testAccAWSLBConfig_networkLoadbalancer_subnets"
+  }
+}
+
+resource "aws_subnet" "alb_test_3" {
+  vpc_id            = "${aws_vpc.alb_test.id}"
+  cidr_block        = "10.0.3.0/24"
+  availability_zone = "us-west-2c"
+
+  tags {
+    Name = "testAccAWSLBConfig_networkLoadbalancer_subnets"
+  }
+}
+`, lbName)
+}
 func testAccAWSLBConfig_networkLoadbalancer(lbName string) string {
 	return fmt.Sprintf(`resource "aws_lb" "lb_test" {
   name            = "%s"

website/docs/r/lb.html.markdown

@@ -50,9 +50,11 @@ Terraform will autogenerate a name beginning with `tf-lb`.
 * `name_prefix` - (Optional) Creates a unique name beginning with the specified prefix. Conflicts with `name`.
 * `internal` - (Optional) If true, the LB will be internal.
 * `load_balancer_type` - (Optional) The type of load balancer to create. Possible values are `application` or `network`. The default value is `application`.
-* `security_groups` - (Optional) A list of security group IDs to assign to the LB.
+* `security_groups` - (Optional) A list of security group IDs to assign to the LB. Only valid for Load Balancers of type `application`.
 * `access_logs` - (Optional) An Access Logs block. Access Logs documented below.
-* `subnets` - (Optional) A list of subnet IDs to attach to the LB.
+* `subnets` - (Optional) A list of subnet IDs to attach to the LB. Subnets
+cannot be updated for Load Balancers of type `network`. Changing this value
+will for load balancers of type `network` will force a recreation of the resource. 
 * `subnet_mapping` - (Optional) A subnet mapping block as documented below.
 * `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