Merge pull request #4864 from hashicorp/phinze/aws-min-elb-cap-regression

aws: undeprecate min_elb_capacity; restore min capacity waiting

builtin/providers/aws/resource_aws_autoscaling_group.go

@@ -51,9 +51,8 @@ func resourceAwsAutoscalingGroup() *schema.Resource {
 			},
 
 			"min_elb_capacity": &schema.Schema{
-				Type:       schema.TypeInt,
+				Type:     schema.TypeInt,
-				Optional:   true,
+				Optional: true,
-				Deprecated: "Please use 'wait_for_elb_capacity' instead.",
 			},
 
 			"min_size": &schema.Schema{
@@ -222,7 +221,7 @@ func resourceAwsAutoscalingGroupCreate(d *schema.ResourceData, meta interface{})
 	d.SetId(d.Get("name").(string))
 	log.Printf("[INFO] AutoScaling Group ID: %s", d.Id())
 
-	if err := waitForASGCapacity(d, meta); err != nil {
+	if err := waitForASGCapacity(d, meta, capacitySatifiedCreate); err != nil {
 		return err
 	}
 
@@ -377,7 +376,7 @@ func resourceAwsAutoscalingGroupUpdate(d *schema.ResourceData, meta interface{})
 	}
 
 	if shouldWaitForCapacity {
-		waitForASGCapacity(d, meta)
+		waitForASGCapacity(d, meta, capacitySatifiedUpdate)
 	}
 
 	return resourceAwsAutoscalingGroupRead(d, meta)
@@ -513,94 +512,6 @@ func resourceAwsAutoscalingGroupDrain(d *schema.ResourceData, meta interface{})
 	})
 }
 
-// Waits for a minimum number of healthy instances to show up as healthy in the
-// ASG before continuing. Waits up to `waitForASGCapacityTimeout` for
-// "desired_capacity", or "min_size" if desired capacity is not specified.
-//
-// If "wait_for_elb_capacity" is specified, will also wait for that number of
-// instances to show up InService in all attached ELBs. See "Waiting for
-// Capacity" in docs for more discussion of the feature.
-func waitForASGCapacity(d *schema.ResourceData, meta interface{}) error {
-	wantASG := d.Get("min_size").(int)
-	if v := d.Get("desired_capacity").(int); v > 0 {
-		wantASG = v
-	}
-	wantELB := d.Get("wait_for_elb_capacity").(int)
-
-	// Covers deprecated field support
-	wantELB += d.Get("min_elb_capacity").(int)
-
-	wait, err := time.ParseDuration(d.Get("wait_for_capacity_timeout").(string))
-	if err != nil {
-		return err
-	}
-
-	if wait == 0 {
-		log.Printf("[DEBUG] Capacity timeout set to 0, skipping capacity waiting.")
-		return nil
-	}
-
-	log.Printf("[DEBUG] Waiting %s for capacity: %d ASG, %d ELB",
-		wait, wantASG, wantELB)
-
-	return resource.Retry(wait, func() error {
-		g, err := getAwsAutoscalingGroup(d, meta)
-		if err != nil {
-			return resource.RetryError{Err: err}
-		}
-		if g == nil {
-			return nil
-		}
-		lbis, err := getLBInstanceStates(g, meta)
-		if err != nil {
-			return resource.RetryError{Err: err}
-		}
-
-		haveASG := 0
-		haveELB := 0
-
-		for _, i := range g.Instances {
-			if i.HealthStatus == nil || i.InstanceId == nil || i.LifecycleState == nil {
-				continue
-			}
-
-			if !strings.EqualFold(*i.HealthStatus, "Healthy") {
-				continue
-			}
-
-			if !strings.EqualFold(*i.LifecycleState, "InService") {
-				continue
-			}
-
-			haveASG++
-
-			if wantELB > 0 {
-				inAllLbs := true
-				for _, states := range lbis {
-					state, ok := states[*i.InstanceId]
-					if !ok || !strings.EqualFold(state, "InService") {
-						inAllLbs = false
-					}
-				}
-				if inAllLbs {
-					haveELB++
-				}
-			}
-		}
-
-		log.Printf("[DEBUG] %q Capacity: %d/%d ASG, %d/%d ELB",
-			d.Id(), haveASG, wantASG, haveELB, wantELB)
-
-		if haveASG == wantASG && haveELB == wantELB {
-			return nil
-		}
-
-		return fmt.Errorf(
-			"Still waiting for %q instances. Current/Desired: %d/%d ASG, %d/%d ELB",
-			d.Id(), haveASG, wantASG, haveELB, wantELB)
-	})
-}
-
 // Returns a mapping of the instance states of all the ELBs attached to the
 // provided ASG.
 //

builtin/providers/aws/resource_aws_autoscaling_group_test.go

@@ -585,14 +585,12 @@ resource "aws_subnet" "alt" {
 }
 
 resource "aws_launch_configuration" "foobar" {
-  name = "vpc-asg-test"
   image_id = "ami-b5b3fc85"
   instance_type = "t2.micro"
 }
 
 resource "aws_autoscaling_group" "bar" {
   availability_zones = ["us-west-2a"]
-  name = "vpc-asg-test"
   max_size = 2
   min_size = 1
   health_check_grace_period = 300
@@ -631,7 +629,6 @@ resource "aws_subnet" "alt" {
 }
 
 resource "aws_launch_configuration" "foobar" {
-  name = "vpc-asg-test"
   image_id = "ami-b5b3fc85"
   instance_type = "t2.micro"
 }
@@ -641,7 +638,6 @@ resource "aws_autoscaling_group" "bar" {
     "${aws_subnet.main.id}",
     "${aws_subnet.alt.id}",
   ]
-  name = "vpc-asg-test"
   max_size = 2
   min_size = 1
   health_check_grace_period = 300

builtin/providers/aws/resource_aws_autoscaling_group_waiting.go

@@ -0,0 +1,129 @@
+package aws
+
+import (
+	"fmt"
+	"log"
+	"strings"
+	"time"
+
+	"github.com/hashicorp/terraform/helper/resource"
+	"github.com/hashicorp/terraform/helper/schema"
+)
+
+// waitForASGCapacityTimeout gathers the current numbers of healthy instances
+// in the ASG and its attached ELBs and yields these numbers to a
+// capacitySatifiedFunction. Loops for up to wait_for_capacity_timeout until
+// the capacitySatisfiedFunc returns true.
+//
+// See "Waiting for Capacity" in docs for more discussion of the feature.
+func waitForASGCapacity(
+	d *schema.ResourceData,
+	meta interface{},
+	satisfiedFunc capacitySatisfiedFunc) error {
+	wait, err := time.ParseDuration(d.Get("wait_for_capacity_timeout").(string))
+	if err != nil {
+		return err
+	}
+
+	if wait == 0 {
+		log.Printf("[DEBUG] Capacity timeout set to 0, skipping capacity waiting.")
+		return nil
+	}
+
+	log.Printf("[DEBUG] Waiting on %s for capacity...", d.Id())
+
+	return resource.Retry(wait, func() error {
+		g, err := getAwsAutoscalingGroup(d, meta)
+		if err != nil {
+			return resource.RetryError{Err: err}
+		}
+		if g == nil {
+			return nil
+		}
+		lbis, err := getLBInstanceStates(g, meta)
+		if err != nil {
+			return resource.RetryError{Err: err}
+		}
+
+		haveASG := 0
+		haveELB := 0
+
+		for _, i := range g.Instances {
+			if i.HealthStatus == nil || i.InstanceId == nil || i.LifecycleState == nil {
+				continue
+			}
+
+			if !strings.EqualFold(*i.HealthStatus, "Healthy") {
+				continue
+			}
+
+			if !strings.EqualFold(*i.LifecycleState, "InService") {
+				continue
+			}
+
+			haveASG++
+
+			inAllLbs := true
+			for _, states := range lbis {
+				state, ok := states[*i.InstanceId]
+				if !ok || !strings.EqualFold(state, "InService") {
+					inAllLbs = false
+				}
+			}
+			if inAllLbs {
+				haveELB++
+			}
+		}
+
+		satisfied, reason := satisfiedFunc(d, haveASG, haveELB)
+
+		log.Printf("[DEBUG] %q Capacity: %d ASG, %d ELB, satisfied: %t, reason: %q",
+			d.Id(), haveASG, haveELB, satisfied, reason)
+
+		if satisfied {
+			return nil
+		}
+
+		return fmt.Errorf("%q: Waiting up to %s: %s", d.Id(), wait, reason)
+	})
+}
+
+type capacitySatisfiedFunc func(*schema.ResourceData, int, int) (bool, string)
+
+// capacitySatifiedCreate treats all targets as minimums
+func capacitySatifiedCreate(d *schema.ResourceData, haveASG, haveELB int) (bool, string) {
+	minASG := d.Get("min_size").(int)
+	if wantASG := d.Get("desired_capacity").(int); wantASG > 0 {
+		minASG = wantASG
+	}
+	if haveASG < minASG {
+		return false, fmt.Sprintf(
+			"Need at least %d healthy instances in ASG, have %d", minASG, haveASG)
+	}
+	minELB := d.Get("min_elb_capacity").(int)
+	if wantELB := d.Get("wait_for_elb_capacity").(int); wantELB > 0 {
+		minELB = wantELB
+	}
+	if haveELB < minELB {
+		return false, fmt.Sprintf(
+			"Need at least %d healthy instances in ELB, have %d", minELB, haveELB)
+	}
+	return true, ""
+}
+
+// capacitySatifiedUpdate only cares about specific targets
+func capacitySatifiedUpdate(d *schema.ResourceData, haveASG, haveELB int) (bool, string) {
+	if wantASG := d.Get("desired_capacity").(int); wantASG > 0 {
+		if haveASG != wantASG {
+			return false, fmt.Sprintf(
+				"Need exactly %d healthy instances in ASG, have %d", wantASG, haveASG)
+		}
+	}
+	if wantELB := d.Get("wait_for_elb_capacity").(int); wantELB > 0 {
+		if haveELB != wantELB {
+			return false, fmt.Sprintf(
+				"Need exactly %d healthy instances in ELB, have %d", wantELB, haveELB)
+		}
+	}
+	return true, ""
+}

builtin/providers/aws/resource_aws_autoscaling_group_waiting_test.go

@@ -0,0 +1,224 @@
+package aws
+
+import "testing"
+
+func TestCapacitySatisfiedCreate(t *testing.T) {
+	cases := map[string]struct {
+		Data            map[string]interface{}
+		HaveASG         int
+		HaveELB         int
+		ExpectSatisfied bool
+		ExpectReason    string
+	}{
+		"min_size, have less": {
+			Data: map[string]interface{}{
+				"min_size": 5,
+			},
+			HaveASG:         2,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need at least 5 healthy instances in ASG, have 2",
+		},
+		"min_size, got it": {
+			Data: map[string]interface{}{
+				"min_size": 5,
+			},
+			HaveASG:         5,
+			ExpectSatisfied: true,
+		},
+		"min_size, have more": {
+			Data: map[string]interface{}{
+				"min_size": 5,
+			},
+			HaveASG:         10,
+			ExpectSatisfied: true,
+		},
+		"desired_capacity, have less": {
+			Data: map[string]interface{}{
+				"desired_capacity": 5,
+			},
+			HaveASG:         2,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need at least 5 healthy instances in ASG, have 2",
+		},
+		"desired_capacity overrides min_size": {
+			Data: map[string]interface{}{
+				"min_size":         2,
+				"desired_capacity": 5,
+			},
+			HaveASG:         2,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need at least 5 healthy instances in ASG, have 2",
+		},
+		"desired_capacity, got it": {
+			Data: map[string]interface{}{
+				"desired_capacity": 5,
+			},
+			HaveASG:         5,
+			ExpectSatisfied: true,
+		},
+		"desired_capacity, have more": {
+			Data: map[string]interface{}{
+				"desired_capacity": 5,
+			},
+			HaveASG:         10,
+			ExpectSatisfied: true,
+		},
+
+		"min_elb_capacity, have less": {
+			Data: map[string]interface{}{
+				"min_elb_capacity": 5,
+			},
+			HaveELB:         2,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need at least 5 healthy instances in ELB, have 2",
+		},
+		"min_elb_capacity, got it": {
+			Data: map[string]interface{}{
+				"min_elb_capacity": 5,
+			},
+			HaveELB:         5,
+			ExpectSatisfied: true,
+		},
+		"min_elb_capacity, have more": {
+			Data: map[string]interface{}{
+				"min_elb_capacity": 5,
+			},
+			HaveELB:         10,
+			ExpectSatisfied: true,
+		},
+		"wait_for_elb_capacity, have less": {
+			Data: map[string]interface{}{
+				"wait_for_elb_capacity": 5,
+			},
+			HaveELB:         2,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need at least 5 healthy instances in ELB, have 2",
+		},
+		"wait_for_elb_capacity, got it": {
+			Data: map[string]interface{}{
+				"wait_for_elb_capacity": 5,
+			},
+			HaveELB:         5,
+			ExpectSatisfied: true,
+		},
+		"wait_for_elb_capacity, have more": {
+			Data: map[string]interface{}{
+				"wait_for_elb_capacity": 5,
+			},
+			HaveELB:         10,
+			ExpectSatisfied: true,
+		},
+		"wait_for_elb_capacity overrides min_elb_capacity": {
+			Data: map[string]interface{}{
+				"min_elb_capacity":      2,
+				"wait_for_elb_capacity": 5,
+			},
+			HaveELB:         2,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need at least 5 healthy instances in ELB, have 2",
+		},
+	}
+
+	r := resourceAwsAutoscalingGroup()
+	for tn, tc := range cases {
+		d := r.TestResourceData()
+		for k, v := range tc.Data {
+			if err := d.Set(k, v); err != nil {
+				t.Fatalf("err: %s", err)
+			}
+		}
+		gotSatisfied, gotReason := capacitySatifiedCreate(d, tc.HaveASG, tc.HaveELB)
+
+		if gotSatisfied != tc.ExpectSatisfied {
+			t.Fatalf("%s: expected satisfied: %t, got: %t (reason: %s)",
+				tn, tc.ExpectSatisfied, gotSatisfied, gotReason)
+		}
+
+		if gotReason != tc.ExpectReason {
+			t.Fatalf("%s: expected reason: %s, got: %s",
+				tn, tc.ExpectReason, gotReason)
+		}
+	}
+}
+
+func TestCapacitySatisfiedUpdate(t *testing.T) {
+	cases := map[string]struct {
+		Data            map[string]interface{}
+		HaveASG         int
+		HaveELB         int
+		ExpectSatisfied bool
+		ExpectReason    string
+	}{
+		"default is satisfied": {
+			Data:            map[string]interface{}{},
+			ExpectSatisfied: true,
+		},
+		"desired_capacity, have less": {
+			Data: map[string]interface{}{
+				"desired_capacity": 5,
+			},
+			HaveASG:         2,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need exactly 5 healthy instances in ASG, have 2",
+		},
+		"desired_capacity, got it": {
+			Data: map[string]interface{}{
+				"desired_capacity": 5,
+			},
+			HaveASG:         5,
+			ExpectSatisfied: true,
+		},
+		"desired_capacity, have more": {
+			Data: map[string]interface{}{
+				"desired_capacity": 5,
+			},
+			HaveASG:         10,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need exactly 5 healthy instances in ASG, have 10",
+		},
+		"wait_for_elb_capacity, have less": {
+			Data: map[string]interface{}{
+				"wait_for_elb_capacity": 5,
+			},
+			HaveELB:         2,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need exactly 5 healthy instances in ELB, have 2",
+		},
+		"wait_for_elb_capacity, got it": {
+			Data: map[string]interface{}{
+				"wait_for_elb_capacity": 5,
+			},
+			HaveELB:         5,
+			ExpectSatisfied: true,
+		},
+		"wait_for_elb_capacity, have more": {
+			Data: map[string]interface{}{
+				"wait_for_elb_capacity": 5,
+			},
+			HaveELB:         10,
+			ExpectSatisfied: false,
+			ExpectReason:    "Need exactly 5 healthy instances in ELB, have 10",
+		},
+	}
+
+	r := resourceAwsAutoscalingGroup()
+	for tn, tc := range cases {
+		d := r.TestResourceData()
+		for k, v := range tc.Data {
+			if err := d.Set(k, v); err != nil {
+				t.Fatalf("err: %s", err)
+			}
+		}
+		gotSatisfied, gotReason := capacitySatifiedUpdate(d, tc.HaveASG, tc.HaveELB)
+
+		if gotSatisfied != tc.ExpectSatisfied {
+			t.Fatalf("%s: expected satisfied: %t, got: %t (reason: %s)",
+				tn, tc.ExpectSatisfied, gotSatisfied, gotReason)
+		}
+
+		if gotReason != tc.ExpectReason {
+			t.Fatalf("%s: expected reason: %s, got: %s",
+				tn, tc.ExpectReason, gotReason)
+		}
+	}
+}

helper/schema/resource.go

@@ -264,6 +264,13 @@ func (r *Resource) InternalValidate(topSchemaMap schemaMap) error {
 	return schemaMap(r.Schema).InternalValidate(tsm)
 }
 
+// TestResourceData Yields a ResourceData filled with this resource's schema for use in unit testing
+func (r *Resource) TestResourceData() *ResourceData {
+	return &ResourceData{
+		schema: r.Schema,
+	}
+}
+
 // Returns true if the resource is "top level" i.e. not a sub-resource.
 func (r *Resource) isTopLevel() bool {
 	// TODO: This is a heuristic; replace with a definitive attribute?

website/source/docs/providers/aws/r/autoscaling_group.html.markdown

@@ -75,8 +75,14 @@ The following arguments are supported:
   wait for ASG instances to be healthy before timing out.  (See also [Waiting
   for Capacity](#waiting-for-capacity) below.) Setting this to "0" causes
   Terraform to skip all Capacity Waiting behavior.
+* `min_elb_capacity` - (Optional) Setting this causes Terraform to wait for
+  this number of instances to show up healthy in the ELB only on creation.
+  Updates will not wait on ELB instance number changes.
+  (See also [Waiting for Capacity](#waiting-for-capacity) below.)
 * `wait_for_elb_capacity` - (Optional) Setting this will cause Terraform to wait
-  for this number of healthy instances all attached load balancers.
+  for exactly this number of healthy instances in all attached load balancers
+  on both create and update operations. (Takes precedence over
+  `min_elb_capacity` behavior.)
   (See also [Waiting for Capacity](#waiting-for-capacity) below.)
 
 Tags support the following:
@@ -86,10 +92,6 @@ Tags support the following:
 * `propagate_at_launch` - (Required) Enables propagation of the tag to
    Amazon EC2 instances launched via this ASG
 
-The following fields are deprecated:
-
-* `min_elb_capacity` - Please use `wait_for_elb_capacity` instead.
-
 ## Attributes Reference
 
 The following attributes are exported:
@@ -117,6 +119,9 @@ A newly-created ASG is initially empty and begins to scale to `min_size` (or
 `desired_capacity`, if specified) by launching instances using the provided
 Launch Configuration. These instances take time to launch and boot.
 
+On ASG Update, changes to these values also take time to result in the target
+number of instances providing service.
+
 Terraform provides two mechanisms to help consistently manage ASG scale up
 time across dependent resources.
 
@@ -144,14 +149,28 @@ Setting `wait_for_capacity_timeout` to `"0"` disables ASG Capacity waiting.
 
 #### Waiting for ELB Capacity
 
-The second mechanism is optional, and affects ASGs with attached Load
+The second mechanism is optional, and affects ASGs with attached ELBs specified
-Balancers. If `wait_for_elb_capacity` is set, Terraform will wait for that
+via the `load_balancers` attribute.
-number of Instances to be `"InService"` in all attached `load_balancers`. This
+
-can be used to ensure that service is being provided before Terraform moves on.
+The `min_elb_capacity` parameter causes Terraform to wait for at least the
+requested number of instances to show up `"InService"` in all attached ELBs
+during ASG creation.  It has no effect on ASG updates.
+
+If `wait_for_elb_capacity` is set, Terraform will wait for exactly that number
+of Instances to be `"InService"` in all attached ELBs on both creation and
+updates.
+
+These parameters can be used to ensure that service is being provided before
+Terraform moves on. If new instances don't pass the ELB's health checks for any
+reason, the Terraform apply will time out, and the ASG will be marked as
+tainted (i.e. marked to be destroyed in a follow up run).
 
 As with ASG Capacity, Terraform will wait for up to `wait_for_capacity_timeout`
-(for `"InService"` instances. If ASG creation takes more than a few minutes,
+for the proper number of instances to be healthy.
-this could indicate one of a number of configuration problems. See the [AWS
+
-Docs on Load Balancer
+#### Troubleshooting Capacity Waiting Timeouts
+
+If ASG creation takes more than a few minutes, this could indicate one of a
+number of configuration problems. See the [AWS Docs on Load Balancer
 Troubleshooting](https://docs.aws.amazon.com/ElasticLoadBalancing/latest/DeveloperGuide/elb-troubleshooting.html)
 for more information.